LFS Merge Conflict Merges Pointers #7166
Comments
shiftkey
added
the
investigation-needed
|
@TCROC thanks for the feedback! I think it'd be great to put together a sample project of this so we can verify the behaviour ourselves and confirm the fix. |
|
@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 Again thanks for looking into this for us :) |
|
@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). |
|
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. |
|
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! |
outofambit
added
priority-2
|
Hi @TCROC, just wanted to echo the others' thanks for the excellent issue and description of how it's falling down for y'all. We're currently working on several features that we're hoping to ship toward the end of this month and unfortunately aren't able to prioritize this until after that work is complete. However, we totally agree that it's confusing and it's a priority for us to fix when that work is complete. Just wanted to let you know and set expectations that it wouldn't be immediately worked on, as much as we'd love to do so. |
|
@billygriffin Thanks for the update! :) I look forward to seeing how this turns out when time is found to work on it :) |
|
@billygriffin @shiftkey @outofambit Hey. Just wanted to check in and see what the status is on this issue. |
|
Hey just checking in to see what the status is on this |
|
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! |
|
@billygriffin Thanks for the response :) . I'll come back to check in the next couple of weeks then :) |
|
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. |
|
I come up with a solution for this issue in this comment #8059 (comment) |
|
@TerryChan Yes that is what we are looking for in regards to our LFS merge conflicts via a GUI. A "Choose Mine" or "Choose Theirs" solution. A "Choose All Mine" or "Choose All Theirs" option would also be helpful to just act on all of the merge conflicts. Normally we do good about not having that many merge conflicts to worry about, but every once and a while when upgrading to a newer version of our game engine Unity, things don't go as smoothly and we need that "Choose All Mine" or "Choose all Their's" option. If anyone gets the chance to implement these features let me know and I'd love to try them out / provide feedback. In regards to the actual git commands being invoked I'm not sure. Based on my understanding of git LFS, they look like they should work, but git commands are not my expertise. I will let someone else verify those. I imagine the challenge probably falls more in line with determining if the file is in fact an LFS file and not just a txt file, but again I'll let the experts provide more light on the situation / what the challenges and potential solutions are in implementing this feature. |
|
@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) |
|
@outofambit yes, you are right, we still need to implement LFS conflict detection And, the output of So for the detection, maybe we can use regex |
|
If I'm not mistaken, there's a more fundamental, analogous issue with git rebase. Suppose you're working on a stack of commits where commits A and B both touch the text LFS file Trying to use |
|
Hey @theNth Thanks for the response! I'm slightly confused though. We aren't trying to perform a rebase. We are trying to resolve LFS merge conflicts when merging one branch into another branch. I don't consider myself an expert in git by any means (which is partially why I enjoy GUIs like GitHub Desktop). However, I don't see where rebasing comes into play here. If you could expand further that would be great! Thanks :) . |
|
@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. 👍 |
|
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. |
|
Awesome! I will test it after the break. |
nerdneha
moved this from Backlog (unprioritized)
to Prioritized backlog (ordered)
in Desktop 2.5 release
The binary flag doesn't seem to do anything in github desktop |
|
What version of GitHub Desktop are u using? It still seems to be working for us. |
|
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. |
|
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! |
|
Am I wrong in saying that the Doesn't that break something with LFS? Can you confirm your entire history for binary files isn't stored locally now? Instead of only the latest version, keeping versions remote. Which parts of LFS is required for it to work as intended? Is Trying @TCROC example only allows me to choose my own changes and results in an error 128 if I try to choose the changes from remote. Something about git credentials. |
|
@mortenblaa
Reading the docs, it says it "unsets" the
I just checked my remote history and the changes appear to be there. I can confirm that I do not have a problem pulling down to different machines using the
I am also not an expert in gitattributes or LFS. Just doing some quick Googling, I found some docs that might be worth a read:
Our team has not yet noticed any issues using the binary flag. Everything appears to store on the remote. In regards to your credential errors, it sounds like it is trying to access the remote changes but can't because you don't have the credentials for the remote. If that is the case, yes you would only be able to access things locally. I do not think it has any relation to LFS or the |
|
But doesn't
Regarding the credentials, I just signed in using my GitHub account and the repository was created by me. Besides that one error I can push/pull just fine. The error only appears when I add the Thanks for following up @TCROC :) |
|
@mortenblaa
I did not know that. If that is the case, I imagine the |
|
@TCROC I tried creating a new project on GitHub. Then I added the .gitignore and .gitattributes to the repo, comitted and pushed those. After, I cloned the repo to another computer. Then I added an 8k png to clearly see if there was any size changes. Pushed that from computer A and pulled on computer B. On both computers I then modified the png, and comitted and pushed from B. On A, I comitted but waited for B to finish, and followed with a pull on A. Finally the merge conflict appeared with an option between using the modified file from master or origin/master. Here I selected "origin/master", and I ended up with the following error: However, if I manually resolve the conflict with It succeeds. No issues with signing in. Maybe this is caused by another bug in Desktop. I would also like to point out, that my After the first commit, it was 47 MB. The original image is 49 MB. |
|
I had a look at my gitconfig and noted there were additions from various sources. Including sourcetree. So I removed it and re-signed in and tried again. However, same result. If anyone is interested my gitconfig looks like this now (I have removed the username and email): |
|
Actually, just watched the |
@mortenblaa Have you noticed your pushes to ever be strangely large? I had an instance where my pushes seem to be way larger than the files I am altering. I deleted my git repo, pulled it back down, and it seems to have fixed itself. If it happens again I'll see if |
|
@TCROC No, I haven't experienced that. I only noticed my Unity scene files compressed really well when set to binary, from about 22 MB to 1.6 MB. Maybe try pushing using the command line and see if you notice anything odd. It might give you more insight, where Desktop hides some information. Right now I'm working with a gitattributes looking something like this (though with many more types): # Source code
*.cs merge diff=csharp -text
# Unity (no automatic merge)
*.meta merge=binary diff -text
*.unity merge=binary diff -text
# LFS
*.unitypackage filter=lfs diff=lfs merge=lfs -text
*.png filter=lfs diff=lfs merge=lfs -textNote that I've unset I generally don't add Unity file types to LFS, since they are text files and diff well, so they don't have to rely on binary patches. At least the local repo growed faster, with Unity types in LFS, but may only be locally because of how LFS keeps versions. I'm unsure if the remote repo actually made better patches with Unity types in LFS and didn't increase that much in size. |
|
Hi @TCROC, sorry for the delay in getting back to you. #9702 is merged and now available on beta (and will be on production next week), which provides the option to choose either version for all file types now and not just explicitly binary files. Do you think that can also close this issue? It's not super clear to me whether there's something specific to LFS that requires further detection logic beyond providing the choice for all file types. |
|
@billygriffin I think being able to do this for all file types should solve this issue. I will test out the beta version quick and if it works as expected, I will close this issue. |
|
@billygriffin Where can I download the beta? |
|
@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 |
|
@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 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
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. |
|
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. |
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 Corrales <pipecaniza@outlook.com>
https: //github.com/desktop/desktop/issues/7166 Co-Authored-By: Juan Felipe Cañizares Corrales <pipecaniza@outlook.com> Co-Authored-By: Edwin Osorio <30908546+edwin1106@users.noreply.github.com>
|
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 The work-around described above is to append the With the usual LFS attribute list ( And with The What is the effect of having Ideally, you'd want to keep those attributes as-is to avoid potential issues with LFS. You can do this by putting the 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 desktop/app/src/lib/git/diff.ts Lines 438 to 456 in 8bdb921 git diff --numstat and 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 when git diff --numstat classifies a file as "binary".
TL;DR A "minimal" work-around for this issue is to change the attributes in your But be aware that this (or the |
I think I'm reproducing this issue with GitHub Desktop Version 2.6.5 Windows 10 Pro 20H2 (19042.804). Using @matteblair 's |
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
Steps to Reproduce
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
2019-03-26.desktop.production.log
The text was updated successfully, but these errors were encountered: