Re: [PATCH v2 1/4] t4059: test 'diff --cc' with a change from only few parents
On Sat, Apr 11, 2015 at 10:51:10PM -0700, Junio C Hamano wrote: Max Kirillov m...@max630.net writes: My exact case was that there was a change in one branch which was overwritten during merge conflict resolution by fully acepting the other branch - in a 2-parent merge. I started looking for a way to visualize such cases. They are not visible in usual diff, because they look same as accepting change compared to the unchange branch. Hmph, isn't that exactly why diff -c exists, not diff --cc that omits (usually) uninteresting hunks? No, this shows too many. If a change is done in one branch but the other branch did not introduce any changes since mergebase and they merged cleanly the merge should not be shown, and `diff -c` seems to show them. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] t4059: test 'diff --cc' with a change from only few parents
On Sat, Apr 11, 2015 at 2:07 PM, Junio C Hamano gits...@pobox.com wrote: Especially, we need to pay close attention to the discussion that germinated the current behaviour: http://thread.gmane.org/gmane.comp.version-control.git/15486/focus=15519 I recall that the diff --cc before that change was not discarding uninteresting merges sufficiently and the two commits were a deliberate attempt to reject what your series wants to show as uninteresting hunks. Two suggestions. - This is primarily for 2/4, but can we make it more clear in the Eh, sorry, I obviously meant 3/4. And let me half-retract this suggestion and put it on hold; the code-flow has become sufficiently different that special-casing two-way merge may not be worth it after all. code that we do this include more change only on Octopus merges? This change should not make any difference for two-way merges and I'd prefer to avoid extra processing of finding matching hunks and combining, only to discard the result. - Can you run diff --cc with and without your patches to the merge from hell commit mentioned in the original thread and see if we show more hunks with your patches, and make sure what are shown additionally looked really interesting? This one still stands. We'd be interested to see what difference this makes in the real world project history. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] t4059: test 'diff --cc' with a change from only few parents
Max Kirillov m...@max630.net writes: If `git diff --cc` is used with 2 or more parents, then it shows all hunks which have changed compared to at least 2 parents. Which is reasonable, because those places are likely places for conflicts, and it should be displayed how they were resolved. But, preliminary path filtering passes a path only it was changed compared to each parent. So, if a hunk which has not changed compared to some of parents is shown if there are more changed hunks in the file, but not shown if it is the only change. This looks inconsistent and for some scenarios it is desirable to show such changes. Add the test which demonstrates the issue. Signed-off-by: Max Kirillov m...@max630.net --- t/t4059-diff-cc-not-affected-by-path-filtering.sh | 108 ++ 1 file changed, 108 insertions(+) create mode 100755 t/t4059-diff-cc-not-affected-by-path-filtering.sh diff --git a/t/t4059-diff-cc-not-affected-by-path-filtering.sh b/t/t4059-diff-cc-not-affected-by-path-filtering.sh new file mode 100755 index 000..3e6e59b --- /dev/null +++ b/t/t4059-diff-cc-not-affected-by-path-filtering.sh @@ -0,0 +1,108 @@ ... + test_seq 11 | sed -e s/^7/7change1/ -e s/^11/11change2/ long/base + git add long/base + test_seq 11 | sed -e s/^7/7change1/ -e s/^11/11change2/ -e s/^2/2change1/ long/win1 + git add long/win1 + test_seq 11 | sed -e s/^7/7change1/ -e s/^11/11change2/ -e s/^2/2change2/ long/win2 + git add long/win2 + test_seq 11 | sed -e s/^7/7change1/ -e s/^11/11change2/ -e s/^2/2merged/ long/merge + git add long/merge + test_seq 11 | sed -e s/^7/7change1/ -e s/^11/11change2/ -e /^2/d long/delete + git add long/delete + test_seq 11 | sed -e s/^7/7change1/ -e s/^11/11change2/ -e s/^2/2change1/ long/only1 + git add long/only1 + test_seq 11 | sed -e s/^7/7change1/ -e s/^11/11change2/ -e s/^2/2change2/ long/only2 Hmph. Is it gmane who is munging these lines? The other copy of this message I received in my mbox (which I read in the same MUA) does not seem to have this corruption, and I do not expect vger.kernel.org to do this kind of munging without getting yelled at by the kernel folks. Anyway, thanks; I'll take a deeper look once I got a chance to. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] t4059: test 'diff --cc' with a change from only few parents
Max Kirillov m...@max630.net writes: If `git diff --cc` is used with 2 or more parents, then it shows all hunks which have changed compared to at least 2 parents. Which is reasonable, because those places are likely places for conflicts, and it should be displayed how they were resolved. OK. But, preliminary path filtering passes a path only it was changed compared to each parent. That is true, but I am a bit confused by the above, especially the word But that begins the sentence. Are you talking about this comment that describes what the caller wants to do? /* find set of paths that every parent touches */ static struct combine_diff_path *find_paths_generic(const unsigned char *sha1, const struct sha1_array *parents, struct diff_options *opt) When the result of a merge exactly matches one (or more) of the parent of the merge, we do not want to show it in the combined format, so intersect_paths() does want to find paths that are different from all parents. Isn't that a good thing? So, if a hunk which has not changed compared to some of parents is shown if there are more changed hunks in the file, but not shown if it is the only change. This looks inconsistent and for some scenarios it is desirable to show such changes. Hmm, that may be true. So help me see if I understand your goal by checking if I rephrased you correctly below: - We want to show a combined hunk when the number of parent variants for the hunk is more than 2 (i.e. interesting octopus) or the result does not match any of the parents (i.e. conflict resolution of a pairwise merge). We want to drop a hunk whose result came from only one set of parent and the other parents had the same original that is different from the result. - The current code filters out a path that matches one of the parents very early. This is OK for a two-way merge. If the result matches one of the parent's, then any hunk we might produce by not pre-filtering would have the result that came from one parent (i.e. the one identical to the result) and there is only one other parent, which cannot make it an interesting octopus by definition. But an octopus may merge three variants and pick the result from one of the parents as a whole. With the pre-filtering, no hunk from such a path is shown, even when the other two variants in discarded parents are not identical. The original to refer to are two commits bf1c32bd (combine-diff: update --cc uninteresting hunks logic., 2006-02-02) and fd4b1d21 (combine-diff: add safety check to --cc., 2006-02-02). Especially, we need to pay close attention to the discussion that germinated the current behaviour: http://thread.gmane.org/gmane.comp.version-control.git/15486/focus=15519 I recall that the diff --cc before that change was not discarding uninteresting merges sufficiently and the two commits were a deliberate attempt to reject what your series wants to show as uninteresting hunks. Two suggestions. - This is primarily for 2/4, but can we make it more clear in the code that we do this include more change only on Octopus merges? This change should not make any difference for two-way merges and I'd prefer to avoid extra processing of finding matching hunks and combining, only to discard the result. - Can you run diff --cc with and without your patches to the merge from hell commit mentioned in the original thread and see if we show more hunks with your patches, and make sure what are shown additionally looked really interesting? Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] t4059: test 'diff --cc' with a change from only few parents
On Sat, Apr 11, 2015 at 02:07:25PM -0700, Junio C Hamano wrote: Max Kirillov m...@max630.net writes: If `git diff --cc` is used with 2 or more parents, then it shows all hunks which have changed compared to at least 2 parents. Which is reasonable, because those places are likely places for conflicts, and it should be displayed how they were resolved. OK. But, preliminary path filtering passes a path only it was changed compared to each parent. That is true, but I am a bit confused by the above, especially the word But that begins the sentence. Are you talking about this comment that describes what the caller wants to do? /* find set of paths that every parent touches */ static struct combine_diff_path *find_paths_generic(const unsigned char *sha1, const struct sha1_array *parents, struct diff_options *opt) When the result of a merge exactly matches one (or more) of the parent of the merge, we do not want to show it in the combined format, so intersect_paths() does want to find paths that are different from all parents. Isn't that a good thing? If it is a good thing, then probably for file which has not been filtered out it is also a good thing, then diff --cc should have been hiding hunks which fully preserve one of the parent. So, if a hunk which has not changed compared to some of parents is shown if there are more changed hunks in the file, but not shown if it is the only change. This looks inconsistent and for some scenarios it is desirable to show such changes. Hmm, that may be true. So help me see if I understand your goal by checking if I rephrased you correctly below: - We want to show a combined hunk when the number of parent variants for the hunk is more than 2 (i.e. interesting octopus) or the result does not match any of the parents (i.e. conflict resolution of a pairwise merge). We want to drop a hunk whose result came from only one set of parent and the other parents had the same original that is different from the result. - The current code filters out a path that matches one of the parents very early. This is OK for a two-way merge. If the result matches one of the parent's, then any hunk we might produce by not pre-filtering would have the result that came from one parent (i.e. the one identical to the result) and there is only one other parent, which cannot make it an interesting octopus by definition. But an octopus may merge three variants and pick the result from one of the parents as a whole. With the pre-filtering, no hunk from such a path is shown, even when the other two variants in discarded parents are not identical. Yes, I meant that. The original to refer to are two commits bf1c32bd (combine-diff: update --cc uninteresting hunks logic., 2006-02-02) and fd4b1d21 (combine-diff: add safety check to --cc., 2006-02-02). Especially, we need to pay close attention to the discussion that germinated the current behaviour: http://thread.gmane.org/gmane.comp.version-control.git/15486/focus=15519 I recall that the diff --cc before that change was not discarding uninteresting merges sufficiently and the two commits were a deliberate attempt to reject what your series wants to show as uninteresting hunks. Two suggestions. - This is primarily for 2/4, but can we make it more clear in the code that we do this include more change only on Octopus merges? This change should not make any difference for two-way merges and I'd prefer to avoid extra processing of finding matching hunks and combining, only to discard the result. - Can you run diff --cc with and without your patches to the merge from hell commit mentioned in the original thread and see if we show more hunks with your patches, and make sure what are shown additionally looked really interesting? The diffs: older version (without my patch): https://gist.githubusercontent.com/max630/70080d38e8e9951b58a4/raw/diffcc_old.txt newer version: https://gist.githubusercontent.com/max630/70080d38e8e9951b58a4/raw/diffcc_new.txt The outcome is quite different, yes. First, I can see that file removal/deletion are passed without filtering, even if file was only added/deleted compared to some parents and not changed compared to any, so it's in some way 2 versions diff which should not be shown. This I think is something should be fixed in addition to this series. Other than that, all other changes look interesting, meaning they contain more than 2 versions. Sometimes it's not easy to spot because a long hunk is almost inherited from one parent with all others being the same, and only in couple of lines there when they differ. But I always find some difference. It was also quite slow, 11 seconds compared to some 0.4. But for 2-way merges speed did not change. My exact case was that there was a change in one branch which was overwritten during
Re: [PATCH v2 1/4] t4059: test 'diff --cc' with a change from only few parents
Max Kirillov m...@max630.net writes: My exact case was that there was a change in one branch which was overwritten during merge conflict resolution by fully acepting the other branch - in a 2-parent merge. I started looking for a way to visualize such cases. They are not visible in usual diff, because they look same as accepting change compared to the unchange branch. Hmph, isn't that exactly why diff -c exists, not diff --cc that omits (usually) uninteresting hunks? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/4] t4059: test 'diff --cc' with a change from only few parents
If `git diff --cc` is used with 2 or more parents, then it shows all hunks which have changed compared to at least 2 parents. Which is reasonable, because those places are likely places for conflicts, and it should be displayed how they were resolved. But, preliminary path filtering passes a path only it was changed compared to each parent. So, if a hunk which has not changed compared to some of parents is shown if there are more changed hunks in the file, but not shown if it is the only change. This looks inconsistent and for some scenarios it is desirable to show such changes. Add the test which demonstrates the issue. Signed-off-by: Max Kirillov m...@max630.net --- t/t4059-diff-cc-not-affected-by-path-filtering.sh | 108 ++ 1 file changed, 108 insertions(+) create mode 100755 t/t4059-diff-cc-not-affected-by-path-filtering.sh diff --git a/t/t4059-diff-cc-not-affected-by-path-filtering.sh b/t/t4059-diff-cc-not-affected-by-path-filtering.sh new file mode 100755 index 000..3e6e59b --- /dev/null +++ b/t/t4059-diff-cc-not-affected-by-path-filtering.sh @@ -0,0 +1,108 @@ +#!/bin/sh + +test_description='combined diff filtering is not affected by preliminary path filtering' +# Since diff --cc allows use not only real parents but any commits, use merge +# base here as the 3rd parent. The trick was suggested in $gmane/191557 to +# spot changes which were discarded during conflict resolution. + +. ./test-lib.sh + +test_expect_success setup ' + mkdir short + mkdir long + for fn in win1 win2 merge delete base only1 only2 + do + test_seq 3 short/$fn + git add short/$fn + test_seq 11 long/$fn + git add long/$fn || return $? + done + git commit -m mergebase + git branch mergebase + + for fn in win1 win2 merge delete base only1 + do + for dir in short long + do + sed -e s/^2/2change1/ -e s/^7/7change1/ $dir/$fn sed.new + mv sed.new $dir/$fn + git add $dir/$fn || return $? + done || return $? + done + sed -e s/^7/7change1/ long/only2 sed.new + mv sed.new long/only2 + git add long/only2 + git commit -m branch1 + git branch branch1 + + git reset --hard mergebase + for fn in win1 win2 merge delete base only2 + do + for dir in short long + do + sed -e s/^2/2change2/ -e s/^11/11change2/ $dir/$fn sed.new + mv sed.new $dir/$fn + git add $dir/$fn || return $? + done || return $? + done + sed -e s/^11/11change2/ long/only1 sed.new + mv sed.new long/only1 + git add long/only1 + git commit -m branch2 + git branch branch2 + + test_must_fail git merge branch1 + git checkout mergebase -- . + test_seq 11 | sed -e s/^7/7change1/ -e s/^11/11change2/ long/base + git add long/base + test_seq 11 | sed -e s/^7/7change1/ -e s/^11/11change2/ -e s/^2/2change1/ long/win1 + git add long/win1 + test_seq 11 | sed -e s/^7/7change1/ -e s/^11/11change2/ -e s/^2/2change2/ long/win2 + git add long/win2 + test_seq 11 | sed -e s/^7/7change1/ -e s/^11/11change2/ -e s/^2/2merged/ long/merge + git add long/merge + test_seq 11 | sed -e s/^7/7change1/ -e s/^11/11change2/ -e /^2/d long/delete + git add long/delete + test_seq 11 | sed -e s/^7/7change1/ -e s/^11/11change2/ -e s/^2/2change1/ long/only1 + git add long/only1 + test_seq 11 | sed -e s/^7/7change1/ -e s/^11/11change2/ -e s/^2/2change2/ long/only2 + git add long/only2 + test_seq 3 short/base + git add short/base + test_seq 3 | sed -e s/^2/2change1/ short/win1 + git add short/win1 + test_seq 3 | sed -e s/^2/2change2/ short/win2 + git add short/win2 + test_seq 3 | sed -e s/^2/2merged/ short/merge + git add short/merge + test_seq 3 | sed -e /^2/d short/delete + git add short/delete + test_seq 3 | sed -e s/^2/2change1/ short/only1 + git add short/only1 + test_seq 3 | sed -e s/^2/2change2/ short/only2 + git add short/only2 + git commit -m merge + git branch merge +' + +test_expect_success 'diff with mergebase shows discarded change from parent 2 in merged file' ' + git diff --cc merge branch1 branch2 mergebase -- long/win1 actual + test -s actual +' + +test_expect_success 'diff with mergebase shows discarded change from parent 1 in merged file' ' + git diff --cc merge branch1 branch2 mergebase -- long/win2 actual + test -s actual +' + +test_expect_failure 'diff with mergebase shows fully discarded file from parent 2' ' + git diff --cc merge branch1 branch2 mergebase -- short/win1
