Skip to content

Use longer conflict markers in recursive merge base - #4488

Merged
ethomson merged 3 commits into
masterfrom
ethomson/conflict_marker_size
Jan 31, 2018
Merged

Use longer conflict markers in recursive merge base#4488
ethomson merged 3 commits into
masterfrom
ethomson/conflict_marker_size

Conversation

@ethomson

Copy link
Copy Markdown
Member

When producing a virtual merge base during a recursive merge (because our branches have multiple merge bases from a criss-cross merge), our merge base may contain conflicts.

Git updated their virtual merge base building to use 9 character conflict markers, which allows users (and programs like rerere) to better identify conflicts that occur in the virtual merge base vs conflicts that occur in the actual merge.

Allow for a custom conflict marker size, allowing callers to override
the default size of the "<<<<<<<" and ">>>>>>>" markers in the
conflicted output file.
Git uses longer conflict markers in the recursive merge base - two more
than the default (thus, 9 character long conflict markers).  This allows
users to tell the difference between the recursive merge conflicts and
conflicts between the ours and theirs branches.

This was introduced in git d694a17986a28bbc19e2a6c32404ca24572e400f.

Update our tests to expect this as well.

@pks-t pks-t left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, except that the option init macro should be updated as well, I guess.

Comment thread include/git2/merge.h
GIT_MERGE_FILE_DIFF_MINIMAL = (1 << 7),
} git_merge_file_flag_t;

#define GIT_MERGE_CONFLICT_MARKER_SIZE 7

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you also update GIT_MERGE_FILE_OPTIONS_INIT?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔

To clarify: the current INIT macro initializes it to 0, which implies this default will be used. Functionally, this is correct.

We could leave this as-is or we could update the macro to explicitly assign this default. I feel like in other places (eg, checkout opts) this is the pattern we use, to leave things blank / null / zero and then assign a default after the fact.

I mention this only to be explicit about this. If you think updating this is better (more visibility by the users?) then I can change it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I guess that's fine. The only implication of this is that the user cannot ever set the conflict marker size to 0. I don't know whether there's any strange usecase that would want that, though.

@ethomson
ethomson merged commit 9d8510b into master Jan 31, 2018
@ethomson
ethomson deleted the ethomson/conflict_marker_size branch January 9, 2019 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants