From: greened@obbligato•org (David A. Greene)
To: Junio C Hamano <gitster@pobox•com>
Cc: git@vger•kernel.org, Dave Ware <davidw@realtimegenomics•com>,
Eric Sunshine <sunshine@sunshineco•com>
Subject: Re: [PATCH v7] contrib/subtree: fix "subtree split" skipped-merge bug
Date: Sun, 17 Jan 2016 16:41:40 -0600 [thread overview]
Message-ID: <8737tvbzh7.fsf@waller.obbligato.org> (raw)
In-Reply-To: <xmqq4meeisas.fsf@gitster.mtv.corp.google.com> (Junio C. Hamano's message of "Fri, 15 Jan 2016 10:58:03 -0800")
Junio C Hamano <gitster@pobox•com> writes:
> Dave Ware <davidw@realtimegenomics•com> writes:
>
>> 'git subtree split' can incorrectly skip a merge even when both parents
>> act on the subtree, provided the merge results in a tree identical to
>> one of the parents. Fix by copying the merge if at least one parent is
>> non-identical, and the non-identical parent is not an ancestor of the
>> identical parent.
>>
>> Also, add a test case which checks that a descendant remains a
>> descendent on the subtree in this case.
>>
>> Signed-off-by: Dave Ware <davidw@realtimegenomics•com>
>> ---
>
> David, how does this round look? Can we proceed with your (and Eric's)
> Reviewed-by: with this version (with one grammo fix Eric pointed out)?
Yes, this looks great to me! Thanks Dave!
-David
>>
>> Notes:
>> Many thanks to Eric Sunshine and Junio Hamano for adivce on this patch
>> Also many thanks to David A. Greene for help with subtree test style
>>
>> Changes since v6
>> - I forgot the notes when I sumbitted v6. (I have now set notes.rewriteRef,
>> so hopefully this wont happen again).
>> - Fixed some missing && in my test rewrite.
>> Changes since v5
>> - Rewrote test case to use subtree test repo and commit creation methods
>> - Added comments on what the test does and which bits are checked
>> - Added comments to test on related bugs which aren't fixed yet
>> Changes since v4
>> - Minor spelling and style fixes to test case
>> Changes since v3:
>> - Improvements to commit message
>> - Removed incorrect use of --boundary on rev-list
>> - Changed use of rev-list to use --count
>> Changes since v2:
>> - Minor improvements to commit message
>> - Changed space indentation to tab indentation in test case
>> - Changed use of rev-list for obtaining commit id to use rev-parse instead
>> Changes since v1:
>> - Minor improvements to commit message
>> - Added sign off
>> - Moved test case from own file into t7900-subtree.sh
>> - Added subshell to test around 'cd'
>> - Moved record of commit for cherry-pick to variable instead of dumping into file
>>
>> [v6]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=284095> [v5]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=282197> [v4]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=282182> [v3]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=282176> [v2]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=282121> [v1]: http://thread.gmane.org/gmane.comp.version-control.git/282065>
>> contrib/subtree/git-subtree.sh | 12 ++++++--
>> contrib/subtree/t/t7900-subtree.sh | 60 ++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 70 insertions(+), 2 deletions(-)
>>
>> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
>> index edf36f8..5c83727 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 $?
>> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
>> index 751aee3..3bf96a9 100755
>> --- a/contrib/subtree/t/t7900-subtree.sh
>> +++ b/contrib/subtree/t/t7900-subtree.sh
>> @@ -1014,4 +1014,64 @@ test_expect_success 'push split to subproj' '
>> )
>> '
>>
>> +#
>> +# This test covers 2 cases in subtree split copy_or_skip code
>> +# 1) Merges where one parent is a superset of the changes of the other
>> +# parent regarding changes to the subtree, in this case the merge
>> +# commit should be copied
>> +# 2) Merges where only one parent operate on the subtree, and the merge
>> +# commit should be skipped
>> +#
>> +# (1) is checked by ensuring subtree_tip is a descendent of subtree_branch
>> +# (2) should have a check added (not_a_subtree_change shouldn't be present
>> +# on the produced subtree)
>> +#
>> +# Other related cases which are not tested (or currently handled correctly)
>> +# - Case (1) where there are more than 2 parents, it will sometimes correctly copy
>> +# the merge, and sometimes not
>> +# - Merge commit where both parents have same tree as the merge, currently
>> +# will always be skipped, even if they reached that state via different
>> +# set of commits.
>> +#
>> +
>> +next_test
>> +test_expect_success 'subtree descendant check' '
>> + subtree_test_create_repo "$subtree_test_count" &&
>> + test_create_commit "$subtree_test_count" folder_subtree/a &&
>> + (
>> + cd "$subtree_test_count" &&
>> + git branch branch
>> + ) &&
>> + test_create_commit "$subtree_test_count" folder_subtree/0 &&
>> + test_create_commit "$subtree_test_count" folder_subtree/b &&
>> + cherry=$(cd "$subtree_test_count"; git rev-parse HEAD) &&
>> + (
>> + cd "$subtree_test_count" &&
>> + git checkout branch
>> + ) &&
>> + test_create_commit "$subtree_test_count" commit_on_branch &&
>> + (
>> + cd "$subtree_test_count" &&
>> + git cherry-pick $cherry &&
>> + git checkout master &&
>> + git merge -m "merge should be kept on subtree" branch &&
>> + git branch no_subtree_work_branch
>> + ) &&
>> + test_create_commit "$subtree_test_count" folder_subtree/d &&
>> + (
>> + cd "$subtree_test_count" &&
>> + git checkout no_subtree_work_branch
>> + ) &&
>> + test_create_commit "$subtree_test_count" not_a_subtree_change &&
>> + (
>> + cd "$subtree_test_count" &&
>> + git checkout master &&
>> + git merge -m "merge should be skipped on subtree" no_subtree_work_branch &&
>> +
>> + git subtree split --prefix folder_subtree/ --branch subtree_tip master &&
>> + git subtree split --prefix folder_subtree/ --branch subtree_branch branch &&
>> + check_equal $(git rev-list --count subtree_tip..subtree_branch) 0
>> + )
>> +'
>> +
>> test_done
next prev parent reply other threads:[~2016-01-17 22:41 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-06 22:09 git subtree bug produces divergent descendants David Ware
2015-12-07 4:53 ` Eric Sunshine
2015-12-07 20:50 ` [PATCH] contrib/subtree: fix "subtree split" skipped-merge bug Dave Ware
2015-12-08 6:49 ` Eric Sunshine
2015-12-08 20:39 ` [PATCH v3] " Dave Ware
2015-12-08 21:23 ` Junio C Hamano
2015-12-09 0:16 ` David Ware
2015-12-09 0:19 ` [PATCH v4] " Dave Ware
2015-12-09 7:52 ` Eric Sunshine
2015-12-09 21:17 ` [PATCH v5] " Dave Ware
2016-01-13 3:27 ` David A. Greene
2016-01-13 19:33 ` David Ware
2016-01-14 3:12 ` David A. Greene
2016-01-14 20:45 ` David Ware
2016-01-17 22:40 ` David A. Greene
2016-01-14 21:26 ` [PATCH v6] " Dave Ware
2016-01-15 0:41 ` [PATCH v7] " Dave Ware
2016-01-15 1:06 ` Eric Sunshine
2016-01-15 18:58 ` Junio C Hamano
2016-01-15 23:24 ` Eric Sunshine
2016-01-17 22:41 ` David A. Greene [this message]
2015-12-07 21:01 ` git subtree bug produces divergent descendants David Ware
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8737tvbzh7.fsf@waller.obbligato.org \
--to=greened@obbligato$(echo .)org \
--cc=davidw@realtimegenomics$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=sunshine@sunshineco$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox