Re: [PATCH 1/4] Add test for showing discarded changes with diff --cc

2015-04-03 Thread Max Kirillov
On Thu, Apr 02, 2015 at 01:55:58PM -0700, Junio C Hamano wrote:
 So I'll give a preliminary nitpicks first ;-)

Thank you; fixed the issues you mentioned.

 + test_seq 11 | sed -e s/^7/7change1/ -e s/^11/11change2/ -e
 s/^2/2change2/ long/only2 
 +git add long/only2 
 
 Patch is line-wrapped around here?

I hope it was you email program which wrapped it (maybe at
quoting). It looks ok in my mutt and in source, and I uses
same git format-patch+git send-email as usual.

-- 
Max
--
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 1/4] Add test for showing discarded changes with diff --cc

2015-04-02 Thread Junio C Hamano
Max Kirillov m...@max630.net writes:

 If a hunk has been changed in both branches and conflict resolution
 fully takes one of branches, discarding all changes that are in the
 other, then the merge is not shown in 3-way diff which git uses by
 default.

 The advice is to use flag --cc and and explicitly add the mergebase was
 given in $gmane/191557. It was discovered though that it does not always
 work. If the whole file has not changed at all compared to one of
 branches then no difference is shown for this file.

 This looks inconsistent and for particular scenario of viewing discarded
 changes is undesirable.

 Add the test which demonstrates the issue.

 Signed-off-by: Max Kirillov m...@max630.net

Thanks.  I will not have time to review this properly at the moment
while preparing 2.4-rc1, and I do not want to spend time figuring
out if the parent culling you are chaning was done deliberately (in
which case this patch may be breaking things) or not.

So I'll give a preliminary nitpicks first ;-)

 Subject: Re: [PATCH 1/4] Add test for showing discarded changes with diff 
 --cc

Subject: t4059: test 'diff --cc' with a change from only few parents

or something?  area: what you did without capitalization of
the beginning of what you did and the full-stop at the end.

 ---
  t/t4059-diff-merge-with-base.sh | 100 
 
  1 file changed, 100 insertions(+)
  create mode 100755 t/t4059-diff-merge-with-base.sh

 diff --git a/t/t4059-diff-merge-with-base.sh b/t/t4059-diff-merge-with-base.sh
 new file mode 100755
 index 000..e650a33
 --- /dev/null
 +++ b/t/t4059-diff-merge-with-base.sh
 @@ -0,0 +1,100 @@
 +#!/bin/sh
 +
 +test_description='Diff aware of merge base'
 +
 +. ./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
 + done 

Two nits:

 - Style: have do on its own line.  A good rule of thumb is that
   you shouldn't have to type ';' when you are writing a shell
   script, unless you are terminating a case arm with ';;'.

for fn in ...
do
...
done

 - Correctness: aside from missing  after test_seq 3, if the
   last step git add long/$fn failed in an earlier iteration, you
   would not notice any breakage.  Either

(
for fn in ...
do
... 
... 
... || exit $?
done
)

or

for fn in ...
do
... 
... 
... || return $?
done

The latter is only valid in our test scripts, where the test
framework gives you an artificial caller of the subroutine
you are writing as the test body.

 + 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 

Patch is line-wrapped around here?
--
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