-
Notifications
You must be signed in to change notification settings - Fork 58
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
Braid interacting with files outside braided directory #41
Comments
No ideas off the top of my head. I'll be happy to investigate if you can provide specific steps to reproduce. I tried to recreate the scenario you described and the update worked fine. |
I believe the following script can reproduce it.
|
Thanks. I see, git's rename detection is unintentionally triggering because of the way braid sets up the trees to merge after my change (#38). Before #38, we used:
After #38, we use:
I thought it was a harmless optimization to skip reading the "local rest" content into the index when generating the base and remote trees. But if a file X in "old upstream lib" (in your example, I'll write a PR to read the "local rest" content into the base and remote trees, so we should have exactly the same merge semantics as before. The fix itself is 2 lines, but I want to do a little code cleanup and add a test. |
When I changed Braid to prepare trees using a temporary index file in 092b8c5, I skipped reading non-mirror local content into the base and remote trees, thinking it was a harmless optimization. But when git diffs the base and local trees, this can cause it to falsely detect a file deleted from the mirror as being renamed to some unrelated file in the local tree. With this change, the trees are constructed the same way as before 092b8c5. Add a test case and rename some of the existing fixtures so I can remember what they're for. Fixes cristibalan#41.
Great work. I released your code and most of my tests seem to work however the above script with version updated to |
BTW Are you interested in coming onboard to help out with the project directly? |
This was triggered by the recently pushed realityforge/buildr_plus@8cf5978 and is unrelated to the original issue; the behavior is the same with 1.0.3. Between realityforge/buildr_plus@8cf5978 and realityforge/buildr_plus@c10a6e9, This has nothing to do with Braid: you would get the same conflict if you simply forked
That message is still missing one piece of information: the filename that git thinks was renamed to |
Righto. Thanks for the explanation. I will reclose this issue then ;) |
BTW:
I started on the implementation myself and it turned out to be not as hard as I expected, assuming the maintainers are satisfied with the way I wrote the test. Here's the thread. |
I appreciate the invitation! I guess it depends on just what you mean by this. I'm not promising at this point to work on issues that don't interest me personally. If you'd like me to be able to make changes or merge other people's changes in the event you're unavailable, I'm open to that, but I certainly prefer to have your review. |
Mostly I am hoping you continue contributing to the project in whatever way works for you. So yes if you wanted to make changes directly or help review pull requests if I am unavailable then that is great. I will send off a message to the original owner of the project... |
The latest version of braid (1.0.4) with the changes from @mattmccutchen has started to cause conflicts with files outside the braided directory. The scenario seems to come about when you have multiple braids into a single repository. So it is not uncommon for our projects to set up a braids for a set of projects such as;
And frequently when you delete several files from tool1 and then
braid update vendor/tools/tool1
you will end up with conflicts in directoriesvendor/tools/tool2
orvendor/tools/tool3
as braid tries to delete them. (Presumably the hash collides?)This only happens in braid (1.0.4) - @mattmccutchen do you have any ideas about what would be causing this and/or how to fix it?
The text was updated successfully, but these errors were encountered: