Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

merge: fix overlap between GIT_MERGE_FILE_FAVOR__CONFLICTED and GIT_MERGE_FILE_SIMPLIFY_ALNUM #6204

Merged
merged 3 commits into from
Feb 13, 2022

Conversation

boretrk
Copy link
Contributor

@boretrk boretrk commented Feb 7, 2022

Somewhere along the line GIT_MERGE_FILE_FAVOR__CONFLICTED seems to have moved from git_merge_file_favor_t to git_merge_file_flag_t without changing value, causing a possible conflict in file_opts.flags

This PR allocates flags in the enums of git2/merge.h for internal usage but marks them as such to avoid overlapping bits in the future.
The flags are #defined to their previous descriptive names in src/merge.h to keep their enum type.

GIT_MERGE_FILE__CONFLICTED

This is to avoid a possible problem where the value is set to the
same as GIT_MERGE_FILE_SIMPLIFY_ALNUM in git_merge_file_flag_t
Allocate flags in git_merge_flag_t and git_merge_file_flag_t
for internal usage to prevent accidental double allocation.
We made the flags that enable recursive merge internal, on the
assumption that nobody would want them and they're hard to reason about.
(Giving people an option that nobody wants is just extra noise.)

However, it made it hard for _us_ to reason about.  There's no good
reason to keep it private, let's just make it public and push that
cognitive load onto our poor users.  But they should expect it, they're
dealing with git, after all.
@ethomson
Copy link
Member

Ugh! Thanks for catching this. Let's just make these public? There's no real good reason not to...

How do you feel about...

commit 0fbf62cd2027e4ad6aed1d207099c17d2414073c
Author: Edward Thomson <ethomson@edwardthomson.com>
Date:   Sat Feb 12 08:46:55 2022 -0500

    merge: make the internal flags public
    
    We made the flags that enable recursive merge internal, on the
    assumption that nobody would want them and they're hard to reason about.
    (Giving people an option that nobody wants is just extra noise.)
    
    However, it made it hard for _us_ to reason about.  There's no good
    reason to keep it private, let's just make it public and push that
    cognitive load onto our poor users.  But they should expect it, they're
    dealing with git, after all.

diff --git a/include/git2/merge.h b/include/git2/merge.h
index a30833915..e90941a49 100644
--- a/include/git2/merge.h
+++ b/include/git2/merge.h
@@ -93,8 +93,13 @@ typedef enum {
 	 */
 	GIT_MERGE_NO_RECURSIVE = (1 << 3),
 
-	/* This flag is reserved for internal library use */
-	GIT_MERGE__INTERNAL_FLAG = (1 << 30)
+	/**
+	 * Treat this merge as if it is to produce the virtual base
+	 * of a recursive merge.  This will ensure that there are
+	 * no conflicts, any conflicting regions will keep conflict
+	 * markers in the merge result.
+	 */
+	GIT_MERGE_VIRTUAL_BASE = (1 << 4)
 } git_merge_flag_t;
 
 /**
@@ -167,8 +172,12 @@ typedef enum {
 	/** Create zdiff3 ("zealous diff3")-style files */
 	GIT_MERGE_FILE_STYLE_ZDIFF3 = (1 << 8),
 
-	/* This flag is reserved for internal library use */
-	GIT_MERGE_FILE__INTERNAL_FLAG = (1 << 30)
+	/**
+	 * Do not produce file conflicts when common regions have
+	 * changed; keep the conflict markers in the file and accept
+	 * that as the merge result.
+	 */
+	GIT_MERGE_FILE_ACCEPT_CONFLICTS = (1 << 9)
 } git_merge_file_flag_t;
 
 #define GIT_MERGE_CONFLICT_MARKER_SIZE	7
diff --git a/src/merge.c b/src/merge.c
index 5f3a37bbe..641b32632 100644
--- a/src/merge.c
+++ b/src/merge.c
@@ -2116,11 +2116,11 @@ int git_merge__iterators(
 	file_opts.flags = opts.file_flags;
 
 	/* use the git-inspired labels when virtual base building */
-	if (opts.flags & GIT_MERGE__VIRTUAL_BASE) {
+	if (opts.flags & GIT_MERGE_VIRTUAL_BASE) {
 		file_opts.ancestor_label = "merged common ancestors";
 		file_opts.our_label = "Temporary merge branch 1";
 		file_opts.their_label = "Temporary merge branch 2";
-		file_opts.flags |= GIT_MERGE_FILE__CONFLICTED;
+		file_opts.flags |= GIT_MERGE_FILE_ACCEPT_CONFLICTS;
 		file_opts.marker_size = GIT_MERGE_CONFLICT_MARKER_SIZE + 2;
 	}
 
@@ -2280,7 +2280,7 @@ static int create_virtual_base(
 		memcpy(&virtual_opts, opts, sizeof(git_merge_options));
 
 	virtual_opts.flags &= ~GIT_MERGE_FAIL_ON_CONFLICT;
-	virtual_opts.flags |= GIT_MERGE__VIRTUAL_BASE;
+	virtual_opts.flags |= GIT_MERGE_VIRTUAL_BASE;
 
 	if ((merge_annotated_commits(&index, NULL, repo, one, two,
 			recursion_level + 1, &virtual_opts)) < 0)
diff --git a/src/merge.h b/src/merge.h
index e033e876a..23932905e 100644
--- a/src/merge.h
+++ b/src/merge.h
@@ -25,16 +25,6 @@
 #define GIT_MERGE_DEFAULT_RENAME_THRESHOLD	50
 #define GIT_MERGE_DEFAULT_TARGET_LIMIT		1000
 
-
-/** Internal merge flags. */
-
-/** The merge is for a virtual base in a recursive merge. */
-#define GIT_MERGE__VIRTUAL_BASE (GIT_MERGE__INTERNAL_FLAG)
-
-/** Accept the conflict file, staging it as the merge result. */
-#define GIT_MERGE_FILE__CONFLICTED (GIT_MERGE_FILE__INTERNAL_FLAG)
-
-
 /** Types of changes when files are merged from branch to branch. */
 typedef enum {
 	/* No conflict - a change only occurs in one branch. */
diff --git a/src/merge_driver.c b/src/merge_driver.c
index 66b257a4f..19b35ac0e 100644
--- a/src/merge_driver.c
+++ b/src/merge_driver.c
@@ -93,7 +93,7 @@ int git_merge_driver__builtin_apply(
 		goto done;
 
 	if (!result.automergeable &&
-		!(file_opts.flags & GIT_MERGE_FILE__CONFLICTED)) {
+		!(file_opts.flags & GIT_MERGE_FILE_ACCEPT_CONFLICTS)) {
 		error = GIT_EMERGECONFLICT;
 		goto done;
 	}

@boretrk
Copy link
Contributor Author

boretrk commented Feb 12, 2022

Sure, I don't see any problem with making them public.
It's not like anyone is forced to set them.

@ethomson ethomson merged commit aded938 into libgit2:main Feb 13, 2022
@ethomson
Copy link
Member

Thanks @boretrk !

@boretrk boretrk deleted the merge_flags branch March 1, 2023 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants