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

Configure merge=union for changelog.txt to reduce merge conflicts #381

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

TymurGubayev
Copy link
Contributor

from the gitattributes man page

union
Run 3-way file level merge for text files, but take lines from both versions, instead of leaving conflict markers. This tends to leave the added lines in the resulting file in random order and the user should verify the result. Do not use this if you do not understand the implications.

Copy link
Member

@myk002 myk002 left a comment

Choose a reason for hiding this comment

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

If this works as advertised, this is awesome and should be applied to the dfhack and df-structures repos as well

Copy link
Member

@lethosor lethosor left a comment

Choose a reason for hiding this comment

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

As long as this only considers lines that would ordinarily conflict, as opposed to lines that changed in just one branch (e.g. we fix a typo in the base repo but the feature branch doesn't have that fix) then I think this is ok. We can always take a pass at the changelog pre-release and make sure things have ended up in the right section and aren't duplicated.

I'll test a bit in a sandbox repo just to make sure.

Copy link
Member

@lethosor lethosor left a comment

Choose a reason for hiding this comment

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

It's not looking like this will work for the case I identified, unfortunately. I definitely picked a case where git wasn't likely to be able to pick the right merge behavior automatically... but in this case, git silently proceeded by adding both lines, when I really only wanted it to add one.

From my sandbox repo: lethosor/sandbox@267f861 is the result of merging lethosor/sandbox@dd12c33 (a change) and lethosor/sandbox@9978d44 (an unrelated addition).

I think I would rather err on the side of producing conflicts that are easy to resolve than resolving conflicts incorrectly. Also, even if we end up with obvious duplication in the changelog as a result of a union merge, we don't get the conflict markers to indicate which version came from where.

@lethosor
Copy link
Member

One test that did work as I expected was lethosor/sandbox@2a0ef33, which is a merge of lethosor/sandbox@2081e42 (a change) and lethosor/sandbox@f00753a (an addition). The difference here is the intermediate line starting with build-now, which causes git to treat the changes as separate hunks.

I don't think we can rely on this working out in all cases, though.

@myk002
Copy link
Member

myk002 commented May 10, 2022

fair enough. I'll close this PR unmerged, then. @TymurGubayev thank you for the idea. I'll keep my eyes open for other opportunities to reduce merge conflicts in changelog.txt. They certainly do crop up often!

@myk002 myk002 closed this May 10, 2022
@lethosor
Copy link
Member

I did see that custom merge drivers are an option, but I figured they would be tricky to write and a bit less convenient to set up (and still might not cover everything).

CPython does it by giving each entry its own file, like this. It solves the problem of identifying an entry whose contents change, but it does feel to me like overkill for the number of conflicts we run into.

@myk002
Copy link
Member

myk002 commented Dec 9, 2024

reopening in light of (re)discussion: https://discord.com/channels/793331351645323264/973411512783872000/1315318514579669084

@myk002 myk002 reopened this Dec 9, 2024
@myk002 myk002 merged commit ed3a7a7 into DFHack:master Dec 9, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants