Re: [PATCH v5] contrib/subtree: fix "subtree split" skipped-merge bug
- From
David A. Greene <greened@obbligato.org>
- Date
- Jan 13, 2016, 03:27 UTC
- Message-ID
- <87y4bunopj.fsf@waller.obbligato.org>
- In-Reply-To
- <1449695853-24929-1-git-send-email-davidw@realtimegenomics.com>
Dave Ware <davidw@realtimegenomics.com> writes:
[ I am sorry I took so long to respond. This one slipped by me. Thank you for tracking this problem down and fixing it! ]
Show 23 quoted lines
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh > index 9f06571..ebf99d9 100755 > --- a/contrib/subtree/git-subtree.sh > +++ b/contrib/subtree/git-subtree.sh > @@ -479,8 +479,16 @@ copy_or_skip() > p="$p -p $parent" > fi > done > - > - if [ -n "$identical" ]; then > + > + copycommit= > + if [ -n "$identical" ] && [ -n "$nonidentical" ]; then > + extras=$(git rev-list --count $identical..$nonidentical) > + if [ "$extras" -ne 0 ]; then > + # we need to preserve history along the other branch > + copycommit=1 > + fi > + fi > + if [ -n "$identical" ] && [ -z "$copycommit" ]; then > echo $identical > else > copy_commit $rev $tree "$p" || exit $?
I don't see anything objectionable here. I am just learning the split code myself. :)
However, when I apply this against master, the test doesn't actually pass and a gitk --all shows the merge commit still missing. At least if I understand the problem correctly. Can you verify whether it works for you?
Show 13 quoted lines
> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh > index 9051982..4fe4820 100755 > --- a/contrib/subtree/t/t7900-subtree.sh > +++ b/contrib/subtree/t/t7900-subtree.sh > @@ -468,4 +468,56 @@ test_expect_success 'verify one file change per commit' ' > )) > ' > > +test_expect_success 'subtree descendant check' ' > + mkdir git_subtree_split_check && > + ( > + cd git_subtree_split_check && > + git init &&
This shouldn't be necessary. If you look at the other tests in t7900-subtree.sh, they all start with:
next_test test_expect_success '<blah>' ' subtree_test_create_repo "$subtree_test_count"
The "subtree_test_create_repo" takes care of creating a subdirectory and initializing a repository. Perhaps you didn't (or still don't) have the test script rewrite patch that got merged a month or so ago. If not, please update to it and reformulate your test to follow the established convention. It helps *a lot* when debugging regressions.
Show 5 quoted lines
> + mkdir folder && > + > + echo a >folder/a && > + git add . && > + git commit -m "first commit" &&
You can use "test_create_commit" to do these "generate commit" operations. It's on my TODO list to update the subtree tests to use more of the standard test infrastructure. For now, just go ahead and use what the other tests use.
> + git branch noop_branch &&
[...]
> + git checkout noop_branch && > + echo moreText >anotherText.txt && > + git add . && > + git commit -m "irrelevant" &&
This is unfortunate naming. Why is the branch a no-op and why is the commit irrelevant? Does the test test the same thing without them? I not they should have different names. If so, why are these needed in the test?
Show 6 quoted lines
> + git checkout master && > + git merge -m "second merge" noop_branch && > + > + git subtree split --prefix folder/ --branch subtree_tip master && > + git subtree split --prefix folder/ --branch subtree_branch branch && > + git push . subtree_tip:subtree_branch
I understand the problem was discovered because of an inability to push and it probably makes sense to test that since that's what exposed the bug. However, I wonder if there are some additional checks that should be done. What do you expect subtree_tip and subtree_branch to look like and how do you expect them to relate to each other? Should subtree_branch be an ancestor of subtree_tip? If so we should explicitly test that.
Again, thanks for your work on this! I think I actually may have hit this bug in my own work but I couldn't be sure I hadn't done something wrong. The sequence of commands and splits is eerily similar to something I tried a while back. I'm *very* glad you were able to track it down!
-David