desktop: LFS Merge Conflict Merges Pointers
Description
When merging into a branch while using Git LFS, it attempts to merge the LFS pointers together. We then have to manually change the files by removing the pointer we don’t want. I would have expected to be prompted with “Take Incoming File” or “Keep Current File” for our LFS files.
Version
- GitHub Desktop: 1.6.5
- Operating system: Microsoft Windows [Version 10.0.17763.379]
Steps to Reproduce
- Create a merge conflict between two LFS files in two different branches.
- Attempt to merge one branch into the other branch with the merge conflict, and you will see that the conflict was “automatically resolved” when all it did was merge the pointers together.
Expected Behavior
Be prompted with “Take Incoming File” or “Keep Current File” for LFS files.
Actual Behavior
The LFS pointers are merged together into one file and breaks the LFS pointer.
Additional Information
Logs
About this issue
- Original URL
- State: open
- Created 5 years ago
- Reactions: 6
- Comments: 57 (13 by maintainers)
Commits related to this issue
- Added fix for Github Desktop merges https: //github.com/desktop/desktop/issues/7166 Co-Authored-By: Edwin Osorio <30908546+edwin1106@users.noreply.github.com> Co-Authored-By: Juan Felipe Cañizares Co... — committed to equilaterus/ue4-gitlfs-baseproject by dacanizares 4 years ago
- Added fix for Github Desktop merges https: //github.com/desktop/desktop/issues/7166 Co-Authored-By: Juan Felipe Cañizares Corrales <pipecaniza@outlook.com> Co-Authored-By: Edwin Osorio <30908546+edwi... — committed to equilaterus-gamestudios/ue4-gitlfs-baseproject by dacanizares 4 years ago
@shiftkey Hey thanks for getting back with me 😃 I created a test repo here. All you have to do is clone the repo in GitHub Desktop. Merge the branch ‘master’ into ‘DevBranch2’ and commit the merge. Then open ‘LFSTrackedFile.asset’ in a text editor (I tested it in Notepad, Notepad++, and VS Code). The results should look like this:
I realize all we have to do is pick the merge ourselves in a merge tool / text editor and is plenty easy for us developers to do, but this is not intuitive for our artists (we are a game dev team). We simply want to be able to do an “Accept Incoming Change” or “Accept Current Change” from GitHub Desktop like we currently have to do in VS Code.
Here are some example pics of what we see:
Before comitting merge:
What we expect to see before committing merge. Pic was taken with a normal merge conflict (not LFS).
The file after merging:
The during merge screen would be nice if there were “Accept Incoming Change” or “Keep My Change” buttons. This would be much more intuitive for our artists. However, we don’t even get the option as seen in the expected image. Currently our artists have to open a command prompt and enter
git mergetooland take the merges by hand.Again thanks for looking into this for us 😃
Thanks for the thorough and helpful discussion on this issue! I’ve encountered this problem too (also with art files in a Unity project) and so I’ve tried the work-around using the
binaryattribute. I found some details that may be helpful to others.The work-around described above is to append the
binaryattribute to the usual LFS attribute list, producing this attribute list:filter=lfs diff=lfs merge=lfs -text binary. I can confirm that GitHub Desktop does use manual conflict resolution for files using this attribute list, which resolves the issue. But how does this actually affect the attributes that git is applying to these files? I usedgit check-attrto find out.With the usual LFS attribute list (
filter=lfs diff=lfs merge=lfs -text) we get these attributes:And with
binaryadded (filter=lfs diff=lfs merge=lfs -text binary) we get these attributes:The
binaryattribute is set, but now thediffandmergeattributes are unset! This is becausebinaryis in fact a macro attribute that, when applied, unsets thediff,merge, andtextattributes.What is the effect of having
diffandmergeunset instead of set to the valuelfs? Unclear! Unlikefilter, I believe that git LFS does not currently do anything with these attributes and that they are reserved for some future use.Ideally, you’d want to keep those attributes as-is to avoid potential issues with LFS. You can do this by putting the
binaryattribute before the LFS attributes, as in:binary filter=lfs diff=lfs merge=lfs -text. This produces the attributes:This seems like exactly what we want! Except that GitHub Desktop does not use manual conflict resolution for files with these attributes (which is the whole point of this work-around).
By experimentation, I’ve found that GitHub Desktop will only use manual conflict resolution on files that have the
diffattribute unset. Why? My best guess is this: https://github.com/desktop/desktop/blob/8bdb92182138ffc23691f81b4278b1a3002abbe8/app/src/lib/git/diff.ts#L438-L456 This function is callinggit diff --numstatand scanning the output for patterns that indicate a “binary” file, according to git. In my testing, I’ve found that the cases where GitHub Desktop uses manual conflict resolution correspond exactly to whengit diff --numstatclassifies a file as “binary”.TL;DR A “minimal” work-around for this issue is to change the attributes in your
.gitattributesfile for LFS files toBut be aware that this (or the
binaryattribute) may cause issues with future versions of LFS.Hey @billygriffin @wycfutures @PseudoNinja @shiftkey @outofambit @pkane @theNth @TerryChan I found a workaround to get the behaviour we are looking for! If you simply mark each LFS file as binary in .gitattributes, it behaves as expected. Here is the .gitattributes file my team is now using:
Sorry if I’m doing something wrong tagging all of you in here, but I know some of you were abandoning LFS completely because of this. I wanted to make sure you saw this as some of you made it seem critical to your project. I hope it helps. It’s not really a solution as it is still a problem how GitHub Desktop handles LFS merge conflicts, but it does cause the expected behaviour to occur. I’m not sure if there will be any unexpected side effects from this. I will post more if I see anything negative happening. As of now, it seems like a nice workaround.
This is a git-lfs internal issue. “Git cannot merge the changes from two different versions of a binary file even if both versions have a common parent. If two people are working on the same file at the same time, they must work together to reconcile their changes to avoid overwriting the other’s work. Git LFS provides file locking to help. Users must still take care to always pull the latest copy of a binary asset before beginning work.”
@billygriffin Thanks for the update! 😃 I look forward to seeing how this turns out when time is found to work on it 😃
I just want to thank @TCROC for this workaround. Without it, using Github Desktop for any kinds of conflicts with binary files would just be painful. This saved me a whole heap of time!
Awesome! I will test it after the break.
Would also like an update, we are starting to utilize GitHub to manage our Silk scripts via LFS and having the same issue. This is causing issues within our team.
Hi @TCROC and thanks for the excellent reproduction example! I reproduced this on my machine and am going to start looking into a fix. I’ll post updates here!
@billygriffin After testing, I am sad to say that it does not resolve the issue 😞 . GitHub Desktop still merges the pointers of the LFS file:
GitHub Desktop incorrectly merging LFS pointer together and marking conflicts as fixed
Visual Studio Code correctly detecting merge conflicts with LFS file
Visual Studio correctly detecting merge conflicts with LFS file
The conflicted file is a png image and marked with this in the .gitattributes file:
TL; DR;
Visual Studio Code = works Visual Studio = works GitHub Desktop = broken
The reason I mention the other tools is because there has been much discussion about whether it is an LFS issue or a GitHub Desktop issue. Because other tools work and GitHub desktop does not, I am lead to believe that it is a GitHub Desktop issue.
However, I did make one interesting discovery in this test. I found that when modifying a file created with the Unity Game Engine that is marked to handle merge conflicts like this:
It worked. Therefore, it could be a combination of an issue with LFS and GitHub Desktop. The only way to know if it is an LFS issue would be to recreate this with the command line. Because I have only done it with GitHub Desktop GUI, tried other GUIs, and these are the results I see, I still conclude that it is an issue with GitHub Desktop and will be keeping this issue open.
UPDATE This can also be seen in the below comment. Edited in here for ease of reading.
I just tested it with the command line. I can confirm that it works as expected in the command line. This clarifies that it is not an issue with Git LFS. It is in fact an issue with GitHub Desktop.
@TCROC Sorry about that! I totally should’ve included a link in my original response. Here it is: https://github.com/desktop/desktop#beta-channel
Forgive me, I was rebasing my feature branch onto master and master does not have the updates to the gitattributes file. So github desktop did not use those new settings during the rebase.
@billygriffin All good. Completely understand. Thanks for the response. I hadn’t’ heard from any of you guys since July so I wanted to make sure it wasn’t forgotten. I do look forward to hearing what comes from the discussions. 👍
I have abandoned LFS and excluded large files from source control to prevent this issue from impeding our progress.
Indeed. However, so far you have been trying to select the content of branch A or B. I’m talking about cases where you want to merge changes A + B; simply selecting one of the branches is not sufficient. I suppose this comes up much more frequently when maintaining a linear history or rewriting history. Somebody makes a change to
fooand pushes it to master. In the meantime, you also changefooand need to rebase on top of master in order to preserve linear history. This results in two version offoothat you need to combine together in some context specific way.Looking at this some more, the problem appears to reside with git LFS itself - not GitHub Desktop. Apologies for posting on the wrong project.
Also, sorry for the delay in getting back to you.
@TerryChan I believe that change will help with this issue, but I don’t think its the complete solution. (we will probably need to also improve desktop’s logic for detecting conflicts in LFS files, too)
I come up with a solution for this issue in this comment https://github.com/desktop/desktop/issues/8059#issuecomment-517353605 Could you give some comments on this solution?
I got the same issue on LFS conflict. I found GitHub Desktop has resolution on text file conflict and binary file(non-lfs) conflict, but has no resolution for LFS conflict, hope this issue can be resolved soon.
Hi @TCROC, thanks for the gentle nudges, and sorry for the delay. We’ve got a couple folks out right now, so we haven’t gotten to this yet. It’ll be prioritized alongside the other priority 2 bugs, so I don’t expect that it’ll happen in the next couple weeks but as I mentioned before, we agree that it should be fixed. If you want to dig into what might be causing it to enable us to knock it out more quickly when we are able to get to it, you’re welcome to do so, but certainly no obligation. Thanks again!
Other priorities have jumped up which I need to tackle first before I can investigate this.
Unassigning myself in the meantime to show it’s available for someone else to try and reproduce.
@TCROC thanks so much for putting together a repro for it. I’m going to try it out and see if I can propose a solution for this (we have detecting in place for binary files but nothing LFS-specific).