The merge from hell...

The merge from hell...

[Linus Torvalds]

Ok,
 I've been holding off on having octopus merges in the kernel tree, but I 
just merged an ACPI update that had used one of them, and I didn't really 
see any real reason not to take it.

That octopus commit has got _twelve_ parents.

It's commit 9fdb62af92c741addbea15545f214a6e89460865, and passing it to 
git-diff-tree with the "--cc" option seems to do the largely the right 
thing (although arguably, since one of the parents always matches the end 
result in all the files, it shouldn't have shown anything at all, so I 
think it could do with some tweaking).

git-diff-tree takes almost three seconds to get its result, though.

However, if git-diff-tree is a bit slow, then gitk gets _really_ upset 
when you click on it. Apparently the colorized multi-way diff just breaks 
down into some O(2**n) hell.

I'm sure it's making progress, it's just very slow, and gitk is 
essentially unusable on that commit (gitk also gets visually confused by 
the fact that it forces a wider set of history than gitk expects, since it 
can't prune it down with the arrow notation).

[ Ahh. gitk just came back. After a _loong_ time thinking, and having run
  out of different colors, but looking otherwise fairly nice ;]

So think of it as a correctness/scalability test.

I haven't tried qgit or gitview on it, but am cc'ing Marco and Aneesh in 
case they want to hone their octopus skills on it.

Btw, I think you're better off parsing the "git-diff-tree --cc" output 
than doing it yourself like gitk does, now that core git has support for 
things like that.

			Linus

Re: The merge from hell...

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

>  I've been holding off on having octopus merges in the kernel tree, but I 
> just merged an ACPI update that had used one of them, and I didn't really 
> see any real reason not to take it.
>
> That octopus commit has got _twelve_ parents.

Len must have been smoking something really good.  Even I would
not attempt to do such an octopus and expect to keep my sanity,
but why not? ;-)

> It's commit 9fdb62af92c741addbea15545f214a6e89460865, and passing it to 
> git-diff-tree with the "--cc" option seems to do the largely the right 
> thing (although arguably, since one of the parents always matches the end 
> result in all the files, it shouldn't have shown anything at all, so I 
> think it could do with some tweaking).

Hmph.  Do you mean a hunk like this?

diff --cc kernel/sys.c
@@@@@@@@@@@@@ +33,7 @@@@@@@@@@@@@

            #include <linux/compat.h>
            #include <linux/syscalls.h>
   + +++    #include <linux/kprobes.h>

            #include <asm/uaccess.h>
            #include <asm/io.h>

Currently I cull hunks that have changes from only one parent,
or when the changes are the same from all but one parent.  This
hunk does not match either criteria.

Instead I could cull all hunks that have either "all whitespace"
or "plus and minus in the same set of columns" hunks.  If I did
so, this would become uninteresting:

diff --cc arch/ia64/pci/pci.c
@@@@@@@@@@@@@ +709,7 @@@@@@@@@@@@@
             */
            int ia64_pci_legacy_write(struct pci_dev *bus, u16...
            {
  ------ -  	int ret = 0;
  ++++++ +  	int ret = size;
            
            	switch (size) {
            	case 1:

But this is still interesting:

@@@@@@@@@@@@@ +308,35 @@@@@@@@@@@@@
            			goto end;
            		}
            	}
  --        	cx->usage++;
  --        
            
     +++    #ifdef CONFIG_HOTPLUG_CPU
     +++    	/*


> git-diff-tree takes almost three seconds to get its result, though.

One trivial thing I should be able to do to speed things up is
to reuse previous diff with other parents.  For example, that
commit does this to kernel/sys.c from its 12 parents.

:100644 100644 d09cac2... 0929c69... M  kernel/sys.c
:100644 100644 eecf845... 0929c69... M  kernel/sys.c
:100644 100644 c3b1874... 0929c69... M  kernel/sys.c
:100644 100644 bce933e... 0929c69... M  kernel/sys.c
:100644 100644 eecf845... 0929c69... M  kernel/sys.c
:100644 100644 bce933e... 0929c69... M  kernel/sys.c
:100644 100644 bce933e... 0929c69... M  kernel/sys.c
:100644 100644 bce933e... 0929c69... M  kernel/sys.c
:100644 100644 eecf845... 0929c69... M  kernel/sys.c
:100644 100644 eecf845... 0929c69... M  kernel/sys.c
:100644 100644 eecf845... 0929c69... M  kernel/sys.c
:100644 100644 eecf845... 0929c69... M  kernel/sys.c

Running "sort -u" on these would leave only 4 lines.

I did not expect anybody to be _that_ sick (eh, pardon my
language) to do a 12-way octpus, so I did not consider this
optimization possibility, but I should be doing only 4 diffs to
format -c for this commit.  Currently I do 12.

> So think of it as a correctness/scalability test.

Heh.  I like this one as a practice.  Thanks!

[PATCH] combine-diff: reuse diff from the same blob.

[Junio C Hamano]

When dealing with an insanely large Octopus, it is possible to
optimize by noticing that more than one parents have the same
blob and avoid running diff between a parent and the merge
result by reusing an earlier result.

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

 Junio C Hamano <junkio@cox.net> writes:

 > One trivial thing I should be able to do to speed things up is
 > to reuse previous diff with other parents.  For example, that
 > commit does this to kernel/sys.c from its 12 parents.
 >
 > :100644 100644 d09cac2... 0929c69... M  kernel/sys.c
 > :100644 100644 eecf845... 0929c69... M  kernel/sys.c
 > :100644 100644 c3b1874... 0929c69... M  kernel/sys.c
 > :100644 100644 bce933e... 0929c69... M  kernel/sys.c
 > :100644 100644 eecf845... 0929c69... M  kernel/sys.c
 > :100644 100644 bce933e... 0929c69... M  kernel/sys.c
 > :100644 100644 bce933e... 0929c69... M  kernel/sys.c
 > :100644 100644 bce933e... 0929c69... M  kernel/sys.c
 > :100644 100644 eecf845... 0929c69... M  kernel/sys.c
 > :100644 100644 eecf845... 0929c69... M  kernel/sys.c
 > :100644 100644 eecf845... 0929c69... M  kernel/sys.c
 > :100644 100644 eecf845... 0929c69... M  kernel/sys.c
 >
 > Running "sort -u" on these would leave only 4 lines.
 >
 > I did not expect anybody to be _that_ sick (eh, pardon my
 > language) to do a 12-way octpus, so I did not consider this
 > optimization possibility, but I should be doing only 4 diffs to
 > format -c for this commit.  Currently I do 12.

 On my Duron 750 w/ 770MB RAM here is the results.

 Without this optimization:
 real	0m11.117s	user	0m9.230s	sys	0m1.860s
 With this optimization:
 real	0m7.339s	user	0m6.730s	sys	0m0.610s

 combine-diff.c |   39 +++++++++++++++++++++++++++++++++++++--
 1 files changed, 37 insertions(+), 2 deletions(-)

7bf761b73cfb74917454e179d95f0dab1cab8f0b
diff --git a/combine-diff.c b/combine-diff.c
index 243f967..0cc18fe 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -523,6 +523,30 @@ static void dump_sline(struct sline *sli
 	}
 }
 
+static void reuse_combine_diff(struct sline *sline, unsigned long cnt,
+			       int i, int j)
+{
+	/* We have already examined parent j and we know parent i
+	 * and parent j are the same, so reuse the combined result
+	 * of parent j for parent i.
+	 */
+	unsigned long lno, imask, jmask;
+	imask = (1UL<<i);
+	jmask = (1UL<<j);
+
+	for (lno = 0; lno < cnt; lno++) {
+		struct lline *ll = sline->lost_head;
+		while (ll) {
+			if (ll->parent_map & jmask)
+				ll->parent_map |= imask;
+			ll = ll->next;
+		}
+		if (!(sline->flag & jmask))
+			sline->flag &= ~imask;
+		sline++;
+	}
+}
+
 int show_combined_diff(struct combine_diff_path *elem, int num_parent,
 		       int dense, const char *header, int show_empty)
 {
@@ -596,8 +620,19 @@ int show_combined_diff(struct combine_di
 		sline[cnt-1].flag = (1UL<<num_parent) - 1;
 	}
 
-	for (i = 0; i < num_parent; i++)
-		combine_diff(elem->parent_sha1[i], ourtmp, sline, cnt, i);
+	for (i = 0; i < num_parent; i++) {
+		int j;
+		for (j = 0; j < i; j++) {
+			if (!memcmp(elem->parent_sha1[i],
+				    elem->parent_sha1[j], 20)) {
+				reuse_combine_diff(sline, cnt, i, j);
+				break;
+			}
+		}
+		if (i <= j)
+			combine_diff(elem->parent_sha1[i], ourtmp, sline,
+				     cnt, i);
+	}
 
 	show_hunks = make_hunks(sline, cnt, num_parent, dense);
 
-- 
1.1.6.g2672

Re: The merge from hell...

[Linus Torvalds]

On Wed, 1 Feb 2006, Junio C Hamano wrote:
> 
> > It's commit 9fdb62af92c741addbea15545f214a6e89460865, and passing it to 
> > git-diff-tree with the "--cc" option seems to do the largely the right 
> > thing (although arguably, since one of the parents always matches the end 
> > result in all the files, it shouldn't have shown anything at all, so I 
> > think it could do with some tweaking).
> 
> Hmph.  Do you mean a hunk like this?
> 
> diff --cc kernel/sys.c
> @@@@@@@@@@@@@ +33,7 @@@@@@@@@@@@@
> 
>             #include <linux/compat.h>
>             #include <linux/syscalls.h>
>    + +++    #include <linux/kprobes.h>
> 
>             #include <asm/uaccess.h>
>             #include <asm/io.h>

Yes.

> Currently I cull hunks that have changes from only one parent,
> or when the changes are the same from all but one parent.  This
> hunk does not match either criteria.

Correct. I think "git-diff-tree --cc" actually works as you advertized, 
but that we should change the rules. More akin to

 - if there are only two versions of a hunk, and one of those versions 
   ended up in the result, the hunk is considered successfully merged
   and should be ignored under "--cc"

Under that rule, the kernel/sys.c hunk you specify is indeed 
uninteresting, because there are only two versions: the version that 
didn't have the addition, and the version that did.

> or "plus and minus in the same set of columns" hunks.  If I did
> so, this would become uninteresting:
> 
> diff --cc arch/ia64/pci/pci.c
> @@@@@@@@@@@@@ +709,7 @@@@@@@@@@@@@
>              */
>             int ia64_pci_legacy_write(struct pci_dev *bus, u16...
>             {
>   ------ -  	int ret = 0;
>   ++++++ +  	int ret = size;
>             
>             	switch (size) {
>             	case 1:

Yes. Again, only two versions. Either it was "ret = 0" or it was "ret = 
size". No other version existed in any branch or in the end result, so 
there really were only two versions, and one of them ended up being the 
end result.

> But this is still interesting:
> 
> @@@@@@@@@@@@@ +308,35 @@@@@@@@@@@@@
>             			goto end;
>             		}
>             	}
>   --        	cx->usage++;
>   --        
>             
>      +++    #ifdef CONFIG_HOTPLUG_CPU
>      +++    	/*

Not under my suggested "two version" rule.

The above has _three_ versions: the one with no changes, the one with 
"cx->usage++" removed and the one with "#ifdef CONFIG_HOTPLUG_CPU" added.

So under the two-version rule, the last one would still be interesting.

> > git-diff-tree takes almost three seconds to get its result, though.
> 
> One trivial thing I should be able to do to speed things up is
> to reuse previous diff with other parents.

Yes. In this example, it would apparently cut down the work by a third for 
that file, although it migt make it harder to calculate the proper "-/+" 
patterns..

> I did not expect anybody to be _that_ sick (eh, pardon my
> language) to do a 12-way octpus, so I did not consider this
> optimization possibility, but I should be doing only 4 diffs to
> format -c for this commit.  Currently I do 12.

I agree, I wouldn't have considered that 12-way merge sane either, but 
hey, it worked, and our octopus merge rules are simple enough that I guess 
there's not really any downside to it.

And I was actually happy to have "git-diff-tree --cc" to look at it, and 
that "gitk" eventually gave results that looked decent. I didn't go 
through it all that closely, but the ones I _looked_ at looked fine, and 
it was mighty pleasant to have the tools to look more closely.

So thanks for "--cc" yet again.

> > So think of it as a correctness/scalability test.
> 
> Heh.  I like this one as a practice.  Thanks!

Thank Len. He may have done it as a way to avoid having extra merges, 
since I complained about those last time ;)

		Linus

Re: The merge from hell...

[Linus Torvalds]

On Wed, 1 Feb 2006, Linus Torvalds wrote:
> 
> Not under my suggested "two version" rule.

That "Not" is just me being stupid. I'm agreeing with you, not 
disagreeing, I just misread your mail.

I _think_ your version of the rule ("pluses and minuses in all the same 
columns") ends up being exactly the same as my rule ("only two different 
versions").

		Linus

Re: The merge from hell...

[Linus Torvalds]

On Wed, 1 Feb 2006, Linus Torvalds wrote:
> 
> I _think_ your version of the rule ("pluses and minuses in all the same 
> columns") ends up being exactly the same as my rule ("only two different 
> versions").

Actually, I take that back.

My version of the rule was not just that there were only two versions: the 
merge _result_ had to match one of the versions in one of the branches 
too.

That's important. It's important because you _can_ actually have a merge 
where both branches have the exact same content, but some evil merger 
ended up editing just the merge result.

So there are literally just two versions, but one of the versions _only_ 
exists in the merge result. Such a thing must always be considered a 
conflict, since basically the merge result is obviously "interesting".

That requirement can still be rewritten as a slight variation of your 
version, I think: "the pluses and minuses are all in the same columns, and 
the final column has to match one other column"

Maybe I'm not thinking clearly. Somebody else should double-check my 
thinking. My brain is tired, and I'm going to bed.

			Linus

[PATCH] combine-diff: update --cc "uninteresting hunks" logic.

[Junio C Hamano]

Earlier logic was discarding hunks that has difference from only
one parent or the same difference from all but one parent.  This
changes it to check if the differences on all lines are from the
same sets of parents.  This discards more uninteresting hunks
and seems to match expectations more naturally.

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

 Linus Torvalds <torvalds@osdl.org> writes:

 > On Wed, 1 Feb 2006, Linus Torvalds wrote:
 >> 
 >> I _think_ your version of the rule ("pluses and minuses in all the same 
 >> columns") ends up being exactly the same as my rule ("only two different 
 >> versions").
 >
 > Actually, I take that back.

 I do not do that "result to match the final" check in this
 version yet.  I'll need to revisit it before placing this in
 the master, but I am going to bed.

 combine-diff.c |  102 ++++++++++++++++++--------------------------------------
 1 files changed, 33 insertions(+), 69 deletions(-)

bcb331e2e87c2d7221751994b5ead4a79dd2a17b
diff --git a/combine-diff.c b/combine-diff.c
index 0cc18fe..44931b2 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -262,58 +262,6 @@ static int interesting(struct sline *sli
 	return ((sline->flag & all_mask) != all_mask || sline->lost_head);
 }
 
-static unsigned long line_common_diff(struct sline *sline, unsigned long all_mask)
-{
-	/*
-	 * Look at the line and see from which parents we have the
-	 * same difference.
-	 */
-
-	/* Lower bits of sline->flag records if the parent had this
-	 * line, so XOR with all_mask gives us on-bits for parents we
-	 * have differences with.
-	 */
-	unsigned long common_adds = (sline->flag ^ all_mask) & all_mask;
-	unsigned long common_removes = all_mask;
-
-	/* If all the parents have this line, that also counts as
-	 * having the same difference.
-	 */
-	if (!common_adds)
-		common_adds = all_mask;
-
-	if (sline->lost_head) {
-		/* Lost head list records the lines removed from
-		 * the parents, and parent_map records from which
-		 * parent the line was removed.
-		 */
-		struct lline *ll;
-		for (ll = sline->lost_head; ll; ll = ll->next) {
-			common_removes &= ll->parent_map;
-		}
-	}
-	return common_adds & common_removes;
-}
-
-static unsigned long line_all_diff(struct sline *sline, unsigned long all_mask)
-{
-	/*
-	 * Look at the line and see from which parents we have some difference.
-	 */
-	unsigned long different = (sline->flag ^ all_mask) & all_mask;
-	if (sline->lost_head) {
-		/* Lost head list records the lines removed from
-		 * the parents, and parent_map records from which
-		 * parent the line was removed.
-		 */
-		struct lline *ll;
-		for (ll = sline->lost_head; ll; ll = ll->next) {
-			different |= ll->parent_map;
-		}
-	}
-	return different;
-}
-
 static unsigned long adjust_hunk_tail(struct sline *sline,
 				      unsigned long all_mask,
 				      unsigned long hunk_begin,
@@ -417,8 +365,7 @@ static int make_hunks(struct sline *slin
 	i = 0;
 	while (i < cnt) {
 		unsigned long j, hunk_begin, hunk_end;
-		int same, diff;
-		unsigned long same_diff, all_diff;
+		unsigned long same_diff;
 		while (i < cnt && !(sline[i].flag & mark))
 			i++;
 		if (cnt <= i)
@@ -449,23 +396,40 @@ static int make_hunks(struct sline *slin
 		}
 		hunk_end = j;
 
-		/* [i..hunk_end) are interesting.  Now does it have
-		 * the same change with all but one parent?
+		/* [i..hunk_end) are interesting.  Now is it really
+		 * interesting?
 		 */
-		same_diff = all_mask;
-		all_diff = 0;
-		for (j = i; j < hunk_end; j++) {
-			same_diff &= line_common_diff(sline + j, all_mask);
-			all_diff |= line_all_diff(sline + j, all_mask);
-		}
-		diff = same = 0;
-		for (j = 0; j < num_parent; j++) {
-			if (same_diff & (1UL<<j))
-				same++;
-			if (all_diff & (1UL<<j))
-				diff++;
+		same_diff = 0;
+		has_interesting = 0;
+		for (j = i; j < hunk_end && !has_interesting; j++) {
+			unsigned long this_diff = ~sline[j].flag & all_mask;
+			struct lline *ll = sline[j].lost_head;
+			if (this_diff) {
+				/* This has some changes.  Is it the
+				 * same as others?
+				 */
+				if (!same_diff)
+					same_diff = this_diff;
+				else if (same_diff != this_diff) {
+					has_interesting = 1;
+					break;
+				}
+			}
+			while (ll && !has_interesting) {
+				/* Lost this line from these parents;
+				 * who are they?  Are they the same?
+				 */
+				this_diff = ll->parent_map;
+				if (!same_diff)
+					same_diff = this_diff;
+				else if (same_diff != this_diff) {
+					has_interesting = 1;
+				}
+				ll = ll->next;
+			}
 		}
-		if ((num_parent - 1 <= same) || (diff == 1)) {
+
+		if (!has_interesting) {
 			/* This hunk is not that interesting after all */
 			for (j = hunk_begin; j < hunk_end; j++)
 				sline[j].flag &= ~mark;
-- 
1.1.6.g2672

[PATCH] combine-diff: add safety check to --cc.

[Junio C Hamano]

The earlier change implemented "only two version" check but
without checking if the change rewrites from all the parents.
This implements a check to make sure that a change introduced
by the merge from all the parents is caught to be interesting.

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

 Junio C Hamano <junkio@cox.net> writes:

 >  Linus Torvalds <torvalds@osdl.org> writes:
 >
 >  > On Wed, 1 Feb 2006, Linus Torvalds wrote:
 >  >
 >  > Actually, I take that back.
 >
 >  I do not do that "result to match the final" check in this
 >  version yet.  I'll need to revisit it before placing this in
 >  the master, but I am going to bed.

 Well, I didn't ;-).

 combine-diff.c |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)

43fef6678c8e925307197fb705e499a023bba838
diff --git a/combine-diff.c b/combine-diff.c
index 45f1822..69e19ed 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -397,7 +397,23 @@ static int make_hunks(struct sline *slin
 		hunk_end = j;
 
 		/* [i..hunk_end) are interesting.  Now is it really
-		 * interesting?
+		 * interesting?  We check if there are only two versions
+		 * and the result matches one of them.  That is, we look
+		 * at:
+		 *   (+) line, which records lines added to which parents;
+		 *       this line appears in the result.
+		 *   (-) line, which records from what parents the line
+		 *       was removed; this line does not appear in the result.
+		 * then check the set of parents the result has difference
+		 * from, from all lines.  If there are lines that has
+		 * different set of parents that the result has differences
+		 * from, that means we have more than two versions.
+		 *
+		 * Even when we have only two versions, if the result does
+		 * not match any of the parents, the it should be considered
+		 * interesting.  In such a case, we would have all '+' line.
+		 * After passing the above "two versions" test, that would
+		 * appear as "the same set of parents" to be "all parents".
 		 */
 		same_diff = 0;
 		has_interesting = 0;
@@ -429,7 +445,7 @@ static int make_hunks(struct sline *slin
 			}
 		}
 
-		if (!has_interesting) {
+		if (!has_interesting && same_diff != all_mask) {
 			/* This hunk is not that interesting after all */
 			for (j = hunk_begin; j < hunk_end; j++)
 				sline[j].flag &= ~mark;
-- 
1.1.6.g2672

Re: [PATCH] combine-diff: add safety check to --cc.

[Linus Torvalds]

On Thu, 2 Feb 2006, Junio C Hamano wrote:
>
> The earlier change implemented "only two version" check but
> without checking if the change rewrites from all the parents.
> This implements a check to make sure that a change introduced
> by the merge from all the parents is caught to be interesting.

Ok, my testing shows that this is all wonderful.

In fact, git-diff-tree now gets the subtle cases right for things that 
"gitk" for some reason gets wrong. I haven't figured out what's wrong with 
gitk, but I don't think it's even worth it: it would be better to just 
teach gitk to use git-diff-tree --cc.

And now when I look at Len's "Merge from hell", not only does it take less 
than 2 seconds for git-diff-tree to calculate, it looks correct too. At 
least I don't see anything that I consider extraneous, although it might, 
of course, have removed too much, and I'd not notice. But it looks great 
(well, as great as it can look without colorization and/or years of 
experience with it - multi-way diffs really aren't very readable ;)

Paul, I'm not able to do something like this in tcl/tk, but could you look 
at trying to make gitk use "git-rev-list --cc" for the colorization?

			Linus

Re: [PATCH] combine-diff: add safety check to --cc.

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> Ok, my testing shows that this is all wonderful.

Thanks for the compliment.

> And now when I look at Len's "Merge from hell", not only does it take less 
> than 2 seconds for git-diff-tree to calculate, it looks correct too. At 
> least I don't see anything that I consider extraneous, although it might, 
> of course, have removed too much, and I'd not notice.

Two seconds?  You must be using CPUs/disks/memory that are lot
faster than what I use (the kernel.org machine available to me
seems to do it at around 3.5 wallclock seconds).  Envy, envy,...

I've done a couple of fixups and added one missing feature and
have pushed it out in 'pu' after some final testing.  The
missing feature was line numbers from each parent.  I could not
count them right for a long time for some unknown silliness.

Before setting the output format in stone by having gitk
interpret it, I'd like to do a quick sanity-check poll.

Len's merge is a bit too wide, so I'll use GIT 1.0.0 commit as
an example.  Here is what I have right now.

        $ git diff-tree --cc v1.0.0 -- debian/changelog | head -n 15
        c2f3bf071ee90b01f2d629921bb04c4f798f02fa
        diff --cc debian/changelog
        index d36904c..376f0fa->4fa6c16
        @@@ +1,93 -1,87 -1,3 @@@
        ++git-core (1.0.0-0) unstable; urgency=low
        ++
        ++  * GIT 1.0.0
        ++
        ++ -- Junio C Hamano <junkio@cox.net>  Wed, 21 Dec 2005...
        ++
         +git-core (0.99.9n-0) unstable; urgency=low
         +
         +  * GIT 0.99.9n aka 1.0rc6
         +
         + -- Junio C Hamano <junkio@cox.net>  Wed, 14 Dec 2005...

Two things to note.

 * Somebody said he missed "index" lines.  There is one now, but
   I am wondering if it might be just be an added noise.  It
   gets absolutely horrible if you run diff-tree on Len's merge.
   On the other hand, being able to cut&paste them to "git
   cat-file blob" command line might be handy.

   Do we want to keep it?

   If we were to keep it, is the format OK?  It lists parent
   blob names (double-dot separated), an arrow, and then result
   blob name.  An alternative would be parent,parent..result,
   like this:

        index d36904c,376f0fa..4fa6c16

   which might be more consistent with the normal ones.

 * I show the line number from the result (+1,93) and then
   parents' line numbers (-1,87 for the first parent, -1,3 for
   the second parent).  To be consistent with the normal ones, I
   am thinking it might be better to move the line number for
   the result to the last.  One downside of that change is I
   tend to use the line number of the result to look up the full
   result more often than to use the line number of the parent,
   and something like Len's merge would push the most important
   line number off the edge of the screen.

   Do we want to keep it the way it is, or do we want to do
   this instead?

        @@@ -1,87 -1,3 +1,93 @@@

Re: [PATCH] combine-diff: add safety check to --cc.

[Linus Torvalds]

On Thu, 2 Feb 2006, Junio C Hamano wrote:
> 
> Two seconds?  You must be using CPUs/disks/memory that are lot
> faster than what I use (the kernel.org machine available to me
> seems to do it at around 3.5 wallclock seconds).  Envy, envy,...

That's not even the fastest machine I have.

However, a lot of it may be due to packing. Packed repositories tend to be 
a lot faster, and this was fully packed.

> Two things to note.
> 
>  * Somebody said he missed "index" lines.  There is one now, but
>    I am wondering if it might be just be an added noise.  It
>    gets absolutely horrible if you run diff-tree on Len's merge.
>    On the other hand, being able to cut&paste them to "git
>    cat-file blob" command line might be handy.
> 
>    Do we want to keep it?

I don't mind it, I have no strong opinions.

>  * I show the line number from the result (+1,93) and then
>    parents' line numbers (-1,87 for the first parent, -1,3 for
>    the second parent).  To be consistent with the normal ones, I
>    am thinking it might be better to move the line number for
>    the result to the last.

I think you're right. The "far off to the right" case is the unusual case, 
I think, and consistency is more important.

But it's certainly not a huge deal for me either.

		Linus

[Attn - repository browser authors] diff-tree combined format.

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> Before setting the output format in stone by having gitk
> interpret it, I'd like to do a quick sanity-check poll.

My current thinking is:

    (1) keep the index line, but format it like this:

	index d36904c,376f0fa..4fa6c16

	That is, parents separated with comma, double dots and
	the result.

    (2) match line numbers in the hunk header to normal diff's
        order:

	@@@ -1,87 -1,3 +1,93 @@@

	That is, counts for parents prefixed with '-', and
	the count for result prefixed with '+'.

If somebody has a strong feeling against the above please raise
your hand.

I said "poll" but I mistakenly dropped all the important /
affected people from the CC list, so in case you missed it, here
is a reproduction.  I think gitweb could also use combined diff
so I added Kay to the list.

> Len's merge is a bit too wide, so I'll use GIT 1.0.0 commit as
> an example.  Here is what I have right now.
>
>         $ git diff-tree --cc v1.0.0 -- debian/changelog | head -n 15
>         c2f3bf071ee90b01f2d629921bb04c4f798f02fa
>         diff --cc debian/changelog
>         index d36904c..376f0fa->4fa6c16
>         @@@ +1,93 -1,87 -1,3 @@@
>         ++git-core (1.0.0-0) unstable; urgency=low
>         ++
>         ++  * GIT 1.0.0
>         ++
>         ++ -- Junio C Hamano <junkio@cox.net>  Wed, 21 Dec 2005...
>         ++
>          +git-core (0.99.9n-0) unstable; urgency=low
>          +
>          +  * GIT 0.99.9n aka 1.0rc6
>          +
>          + -- Junio C Hamano <junkio@cox.net>  Wed, 14 Dec 2005...
>
> Two things to note.
>
>  * Somebody said he missed "index" lines.  There is one now, but
>    I am wondering if it might be just be an added noise.  It
>    gets absolutely horrible if you run diff-tree on Len's merge.
>    On the other hand, being able to cut&paste them to "git
>    cat-file blob" command line might be handy.
>
>    Do we want to keep it?
>
>    If we were to keep it, is the format OK?  It lists parent
>    blob names (double-dot separated), an arrow, and then result
>    blob name.  An alternative would be parent,parent..result,
>    like this:
>
>         index d36904c,376f0fa..4fa6c16
>
>    which might be more consistent with the normal ones.
>
>  * I show the line number from the result (+1,93) and then
>    parents' line numbers (-1,87 for the first parent, -1,3 for
>    the second parent).  To be consistent with the normal ones, I
>    am thinking it might be better to move the line number for
>    the result to the last.  One downside of that change is I
>    tend to use the line number of the result to look up the full
>    result more often than to use the line number of the parent,
>    and something like Len's merge would push the most important
>    line number off the edge of the screen.
>
>    Do we want to keep it the way it is, or do we want to do
>    this instead?
>
>         @@@ -1,87 -1,3 +1,93 @@@

To be complete, I'd also describe the rules for combined diff
text here.

1. Unlike normal unidiff that has one column prefix, this has N
   column prefix for N parents (usually 2 -- your branch and the
   other branch that is merged into it -- but more for an
   Octopus).  Column 1 talks about the first parent, column 2
   about the second, etc.

2. The hunk header starts and ends with N+1 (usually 2+1=3, more
   for an Octopus) '@' characters.

3. The column prefix is one of the following two shapes and a
   half:

   * one or more minus '-' and whitespace in others: the line
     does not appear in the result, and parents with '-' has
     it.

   * one or more plus '+' and whitespace in others: the line
     appears in the result, and parents without '+' has it.

   * all whitespace: the line appears in the result and all
     parents have it (this is a special case of the above).

Re: [Attn - repository browser authors] diff-tree combined format.

[Marco Costalba]

On 2/3/06, Junio C Hamano <junkio@cox.net> wrote:
> Junio C Hamano <junkio@cox.net> writes:
>

>
>     (2) match line numbers in the hunk header to normal diff's
>         order:
>
>         @@@ -1,87 -1,3 +1,93 @@@
>
>         That is, counts for parents prefixed with '-', and
>         the count for result prefixed with '+'.
>

It's OK for me. Just one (documentation) note. I found, by means of a
past qgit annotate bug ;-)  in case of small files (1 line files as
VERSION files) the diff header format is slightly different.

Sorry, I am not able to post now the diff output but I think should be
not a problem to reproduce.

This is just, as said above, a note to avoid someone else falls in the
same bug assuming

@ -1,87 -1,3 +1,93 @

is the only possible header format.


Regarding the rest is all OK for me. I choose to do not alter/coloring
the patch as gitk does, but to always use red for removed lines and
green for added and to keep the patch output _as is_ . I found this
more simple and clear, at least for me.

Marco

Re: [Attn - repository browser authors] diff-tree combined format.

[Junio C Hamano]

Marco Costalba <mcostalba@gmail.com> writes:

> This is just, as said above, a note to avoid someone else falls in the
> same bug assuming
>
> @ -1,87 -1,3 +1,93 @
>
> is the only possible header format.

When you parse normal unidiff, you may need to watch out for
things like these:

    --- a/H          --- a/H            --- a/H            
    +++ b/H          +++ b/H            +++ b/H            
    @@ -1 +1 @@      @@ -2,2 +1,0 @@    @@ -1 +0,0 @@      
    -A               -2                 -1                 
    +1               -3                                    

 * When a hunk affects only one line, the line count is omitted.

 * When a hunk is only removal of lines, the line count of the
   result is zero.

 * When such a hunk removes lines from the beginning of the file,
   the line offset in the result is also zero.

I do not do the "omit 1" optimizatino in combined output format
(yet -- should I???), so that would not be a problem.  The other
two you would see only when you do not have context, and
combined output format has its own context length you cannot
override, so they probably may not matter (I do not know offhand
what 'diff-tree -c' would do in such cases).

Re: [Attn - repository browser authors] diff-tree combined format.

[Junio C Hamano]

Marco Costalba <mcostalba@gmail.com> writes:

> Regarding the rest is all OK for me. I choose to do not alter/coloring
> the patch as gitk does, but to always use red for removed lines and
> green for added and to keep the patch output _as is_ . I found this
> more simple and clear, at least for me.

This reminds me of one thing.  One thing I did not like about
the colouring of gitk on an Octopus was that it repeats the same
parents in different colors, wasting lines.  From diff-tree -c
output, one thing coloured diff browsers _could_ do is to let
the user pick one parent and paint the lines to make the
differences from that parent alone stand out.

For example, If I had this, and I wanted to focus on the
difference from the first parent:

        diff --cc foo
        index 92cfee4,50392ff..e9bf860
        @@@ -55,A -55,B +55,C @@@
          # Define USE_STDEV below if you want git to care...
          # change being considered an inode change from t...

        - GIT_VERSION = 0.99.9n
         -GIT_VERSION = 0.99.9.GIT
        ++GIT_VERSION = 1.0.0
         +# This line was already in the first parent
        + # Not in the first parent but in the second already

          # CFLAGS and LDFLAGS are for the users to overri...

You could paint '  ' (unchanged from both) and ' +' (unchanged
from first parent) lines the same way (say white background with
black letters), '- ' (first parent had it but it is gone) lines
red and '+ ' and '++' (either way, first parent did not have it)
lines green.  And ' -' (disappeared from second parent, but
neither first parent nor the result has it) can even be dimmed
(say white background with gray letters).

Re: [Attn - repository browser authors] diff-tree combined format.

[Marco Costalba]

On 2/3/06, Junio C Hamano <junkio@cox.net> wrote:
> Marco Costalba <mcostalba@gmail.com> writes:
>
> This reminds me of one thing.  One thing I did not like about
> the colouring of gitk on an Octopus was that it repeats the same
> parents in different colors, wasting lines.  From diff-tree -c
> output, one thing coloured diff browsers _could_ do is to let
> the user pick one parent and paint the lines to make the
> differences from that parent alone stand out.
>

Both qgit and gitk have the 'diff to selected rev' feature.

What about integrate combined output with exsisting 'diff to selected'
functionality to reach what you are proposing?

Put in other words, an user selects the parent (CTRL + right click in
qgit) and only the
corresponding interesting diffs is shown.

For this to work it is necessary (at least in qgit) something like

   git-diff-tree -c -r -m -p <sha1> <sha2>


Marco

Re: [Attn - repository browser authors] diff-tree combined format.

[Paul Mackerras]

Junio C Hamano writes:

>     (1) keep the index line, but format it like this:

At this stage, I don't think gitk will need the index line.

>     (2) match line numbers in the hunk header to normal diff's
>         order:
> 
> 	@@@ -1,87 -1,3 +1,93 @@@

I think that will look better.

Paul.

Re: [PATCH] combine-diff: add safety check to --cc.

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> And now when I look at Len's "Merge from hell", not only does it take less 
> than 2 seconds for git-diff-tree to calculate, it looks correct too. At 
> least I don't see anything that I consider extraneous, although it might, 
> of course, have removed too much, and I'd not notice.

I've run "diff -u0" between -c output and --cc output and what
was dropped looked sane.

Re: [PATCH] combine-diff: add safety check to --cc.

[Paul Mackerras]

Linus Torvalds writes:

> In fact, git-diff-tree now gets the subtle cases right for things that 
> "gitk" for some reason gets wrong. I haven't figured out what's wrong with 
> gitk, but I don't think it's even worth it: it would be better to just 
> teach gitk to use git-diff-tree --cc.

Working on it now.  That will let me cut out about 500 lines of pretty
hairy Tcl code from gitk, which is nice.

Paul.

Re: [PATCH] combine-diff: add safety check to --cc.

[Junio C Hamano]

Paul Mackerras <paulus@samba.org> writes:

> Linus Torvalds writes:
>
>> In fact, git-diff-tree now gets the subtle cases right for things that 
>> "gitk" for some reason gets wrong. I haven't figured out what's wrong with 
>> gitk, but I don't think it's even worth it: it would be better to just 
>> teach gitk to use git-diff-tree --cc.
>
> Working on it now.  That will let me cut out about 500 lines of pretty
> hairy Tcl code from gitk, which is nice.
>
> Paul.

Excited to hear that.  Please be sure to base your work on the
latest updated format, described in my earlier message

    Subject: [Attn - repository browser authors] diff-tree combined format.

Re: The merge from hell...

[Paul Mackerras]

Junio C Hamano writes:

> Linus Torvalds <torvalds@osdl.org> writes:
> > That octopus commit has got _twelve_ parents.

Making it a dodecapus, or something? :)

> But this is still interesting:
> 
> @@@@@@@@@@@@@ +308,35 @@@@@@@@@@@@@
>             			goto end;
>             		}
>             	}
>   --        	cx->usage++;
>   --        
>             
>      +++    #ifdef CONFIG_HOTPLUG_CPU
>      +++    	/*

Why is that interesting?  It seems to me that two independent changes
were made that just happened to be within a couple of lines of each
other, but didn't interact.  The reason that one change appears in two
branches, and the other in 3, is I think just related to where the
branches start from.  So IMHO this hunk isn't interesting.

Paul.

Re: The merge from hell...

[Junio C Hamano]

Paul Mackerras <paulus@samba.org> writes:

> Why is that interesting?  It seems to me that two independent changes
> were made that just happened to be within a couple of lines of each
> other, but didn't interact.

I agree th hunk in that particular case is not that interesting.

Things are not black and white but you have to draw a line
somewhere.  "Happend to be within a couple of lines" is where we
currently draw that line (it is called "context").

Re: The merge from hell...

[Linus Torvalds]

On Sat, 4 Feb 2006, Paul Mackerras wrote:
> 
> > But this is still interesting:
> > 
> > @@@@@@@@@@@@@ +308,35 @@@@@@@@@@@@@
> >             			goto end;
> >             		}
> >             	}
> >   --        	cx->usage++;
> >   --        
> >             
> >      +++    #ifdef CONFIG_HOTPLUG_CPU
> >      +++    	/*
> 
> Why is that interesting?  It seems to me that two independent changes
> were made that just happened to be within a couple of lines of each
> other, but didn't interact.

Correct. It's "interesting" only because the context of three lines 
overlapped. So _technically_ it's not that interesting.

> The reason that one change appears in two branches, and the other in 3, 
> is I think just related to where the branches start from.  So IMHO this 
> hunk isn't interesting.

I actually think that whenever there are edits this close (even if they 
aren't strictly overlapping) they actually _are_ interesting. Even if it 
merged automatically, you may well want to know that the automated merge 
did something like this.

Doing a 

	git-rev-list --parents HEAD |
		egrep '^.{90}' |
		cut -d' ' -f1 | 
		git-diff-tree --pretty --cc --stdin
		| less -S

on the kernel is actually interesting. It's interesting because it shows 
that out of 1391 merges, in the kernel, only _19_ actually had these close 
calls. Some - but certainly not all - of them actually did need manual 
fixup.

So I actually prefer the "show close misses" case. But I guess we could 
have a "-cN" line to tell how many lines of context to use..

		Linus

Re: The merge from hell...

[Linus Torvalds]

On Sat, 4 Feb 2006, Linus Torvalds wrote:
> 
> Doing a 
> 
> 	git-rev-list --parents HEAD |
> 		egrep '^.{90}' |
> 		cut -d' ' -f1 | 
> 		git-diff-tree --pretty --cc --stdin
> 		| less -S
> 
> on the kernel is actually interesting. It's interesting because it shows 
> that out of 1391 merges, in the kernel, only _19_ actually had these close 
> calls. Some - but certainly not all - of them actually did need manual 
> fixup.

There are some doubly interesting lessons when I looked closer.

In particular, some merges that needed manual fixups do _not_ show up. I 
found that surprising, at first. I expected that if I had to fix something 
manually, it would obviously show up in the "--cc" output.

Not so.  In fact, the one I looked closer at didn't show up even in the 
"long" version, aka "-c".

The reason? A lot of the manual fixups end up selecting one version or the 
other - the clash is because two people fixes the same bug slightly 
differently, and the manual merge will end up just selecting one of them. 
So then even "-c" won't show it, because it will notice that the whole 
file was actually the same as one of the branches merged.

That may be a bit non-intuitive (maybe it shouldn't be, and it was just me 
who didn't think about it the right way when I was initially surprised), 
but it was definitely the right thing (both from a merge standpoint _and_ 
from a "what happened" standpoint) in the cases I looked at. The merge may 
have been manual, but the end _result_ was trivial, and thus isn't shown.

So even after looking at it more, and searching for "interesting" cases 
from the other side, I really like the current git-diff-tree --cc output. 
It sometimes shows you things you wouldn't expect, and it sometimes 
doesn't show you things you'd expect to show up, but in both cases it 
shows/avoids the _right_ things.

However, the point that a "diff" itself isn't totally unambigious is well 
taken. You're right that the very first hunk of the 12-way merge is really 
not interesting.

However, looking at the other cases, it seems to not really be a huge 
problem - that seems to be the only case in the whole kernel - and the 
git-diff-tree algorithm may show an unnecessary hunk once in a blue moon, 
but that's better than having the heuristics fail the other way around (ie 
not showing a hunk).

That's what the gitk problem was, btw (showing too little, not too much). 
Current gitk fails on this trivial case:

	mkdir test-merge
	cd test-merge/
	git-init-db 

	#
	# Initial silly contents
	#
	echo "Hello" > hello
	echo "Hi" > hi
	git add hello hi
	git commit -m "Initial"

	#
	# Create another branch
	#
	git branch other

	#
	# Edit the contents on the master branch,
	# commit it.
	#
	echo "Hello there" > hello
	git commit -m "first change" hello

	#
	# Edit/commit the other differently
	#
	git checkout other
	echo "Hello differently" > hello
	git commit -m "second change" hello

	#
	# Try to merge - this will fail
	#
	git checkout master
	git merge "Clashing merge" HEAD other

	#
	# Do an evil merge conflict that also edits a 
	# nonconflicting file
	#
	echo "Hello third version" > hello
	echo "Hidden hi change" > hi 
	git commit -m "Evil merge conflict" hello hi

At this point, "git-diff-tree --cc HEAD" shows exactly the right thing, 
but "gitk" doesn't show anything at all for that merge (it shows the 
"hello" file in the file pane, but no diff at all, and certainly not the 
"hi" file which it _should_ show).

		Linus

Re: The merge from hell...

[Marco Costalba]

On 2/2/06, Linus Torvalds <torvalds@osdl.org> wrote:
>

> Btw, I think you're better off parsing the "git-diff-tree --cc" output
> than doing it yourself like gitk does, now that core git has support for
> things like that.
>

Currently the public git repo version of qgit uses "git-diff-tree -c"
for merges, It's not a problem to change qgit to use --cc option
instead. But I would like to use just one kind of option to filter
merges files.

So I would like to hear some feedback on using --cc vs -c.

Marco

Re: The merge from hell...

[Linus Torvalds]

On Thu, 2 Feb 2006, Marco Costalba wrote:
> 
> Currently the public git repo version of qgit uses "git-diff-tree -c"
> for merges, It's not a problem to change qgit to use --cc option
> instead. But I would like to use just one kind of option to filter
> merges files.

I think using "-c" rather than "--cc" is fine, and if it makes parsing 
easier..

The "--cc" option should show less "noise", an dI'm actually arguing that 
it should show even less than it does now (for when the branches agreed 
with each other, and there was no actual conflict, which is what I think 
is more useful), but I don't think "-c" is _wrong_ either.

Using "-c" is a strange kind of "show all differences in a file that had 
file-level merges" while at least to me "--cc" is meant to be more of a 
"show all actual file-level CONFLICTS where the merge actually did 
something different"

		Linus

Re: The merge from hell...

[Aneesh Kumar]

On 2/2/06, Linus Torvalds <torvalds@osdl.org> wrote:
>
>
> On Thu, 2 Feb 2006, Marco Costalba wrote:
> >
> > Currently the public git repo version of qgit uses "git-diff-tree -c"
> > for merges, It's not a problem to change qgit to use --cc option
> > instead. But I would like to use just one kind of option to filter
> > merges files.
>
> I think using "-c" rather than "--cc" is fine, and if it makes parsing
> easier..
>
> The "--cc" option should show less "noise", an dI'm actually arguing that
> it should show even less than it does now (for when the branches agreed
> with each other, and there was no actual conflict, which is what I think
> is more useful), but I don't think "-c" is _wrong_ either.
>
> Using "-c" is a strange kind of "show all differences in a file that had
> file-level merges" while at least to me "--cc" is meant to be more of a
> "show all actual file-level CONFLICTS where the merge actually did
> something different"


Ok i tried using --cc. But then as per the man page if the
optimization makes the all the hunks disapper then commit log is not
show unless -m is used. Is there a way to get the commit log printed.
BTW using -m didn't show any difference even though man page said
about some difference

-aneesh

Re: The merge from hell...

[Junio C Hamano]

Aneesh Kumar <aneesh.kumar@gmail.com> writes:

> Ok i tried using --cc. But then as per the man page if the
> optimization makes the all the hunks disapper then commit log is not
> show unless -m is used. Is there a way to get the commit log printed.

Yes, there is a way.  I need to fix a bug ;-).

This patch was done on top of the other two patches I sent
earlier tonight, but should apply on top of master even if you
have not applied the other two.

--

diff --git a/combine-diff.c b/combine-diff.c
index 44931b2..45f1822 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -675,6 +675,8 @@ int diff_tree_combined_merge(const unsig
 					       show_empty_merge))
 				header = NULL;
 		}
+		if (header)
+			puts(header);
 	}
 
 	/* Clean things up */

Re: The merge from hell...

[Linus Torvalds]

On Thu, 2 Feb 2006, Aneesh Kumar wrote:
> 
> Ok i tried using --cc. But then as per the man page if the
> optimization makes the all the hunks disapper then commit log is not
> show unless -m is used.

Ahh. You're mis-using git-diff-tree.

git-diff-tree will _never_ show the commit log if the diff ends up being 
empty. That can actually happen even for a regular commit, because we had 
some early bugs that allowed them.

See commit 69d37960b578be0a69383bd71d06c1fcfb86e8b9, for example. Notice 
how

	git-diff-tree --pretty 69d37960

is entirely silent, even though 

	git log 69d37960

clearly shows that the commit exists. I screwed up a commit, and 
effectively applied an empty patch (this was before git-apply would 
notice, and consider it an error - something I fixed very soon 
afterwards ;^).

> Is there a way to get the commit log printed.

Yes. What you should do is to do

	git-rev-list --parents --pretty

which gives the revision _log_ in pretty format. 

The difference between "git-rev-list --pretty" and "git-diff-tree 
--pretty" is really exactly that "git-rev-list" lists every revision, 
while git-diff-tree only lists revisions that _differ_ in the trees.

This, btw, is also really the main difference between doing a "git log" 
and doing a "git whatchanged". A "git log" shows all revisions (and uses 
"git-rev-list --pretty"). While "git whatchanged" shows what _changed_ 
(and uses "git-diff-tree --pretty").

For a revision tool like gitview, you want to use "git-rev-list --pretty".

[ SIDE NOTE! Actually, there's another format that you may find easier to 
  parse automatically: "git-rev-list --header". It uses the raw format, 
  and adds a NUL character between each rev. That makes things like the 
  date etc trivial to parse, and it's also slightly easier to see the end 
  of a commit message vs just a normal end-of-line. 

  So "gitk", for example, really parses the output of

	git-rev-list --header --topo-order --parents

  to generate the revision list. The "--topo-order" part guarantees that 
  all children will always be shown before any parents. ]

Hope that clarifies the issue.

		Linus

Re: The merge from hell...

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> On Thu, 2 Feb 2006, Aneesh Kumar wrote:
>> 
>> Ok i tried using --cc. But then as per the man page if the
>> optimization makes the all the hunks disapper then commit log is not
>> show unless -m is used.
>
> Ahh. You're mis-using git-diff-tree.
>
> git-diff-tree will _never_ show the commit log if the diff ends up being 
> empty.

True, I should fix the documentation.

It _might_ make sense for certain users like gitk and gitview if
we had a single tool that gives --pretty and its diff even if
the diff is empty.  Having said that, the flag --cc -m is too
specific.  If some uses want to see the commit log even for an
empty diff, that flag should not be something only --cc honors.

Re: The merge from hell...

[Linus Torvalds]

On Thu, 2 Feb 2006, Junio C Hamano wrote:
>
> Linus Torvalds <torvalds@osdl.org> writes:
> >
> > Ahh. You're mis-using git-diff-tree.
> >
> > git-diff-tree will _never_ show the commit log if the diff ends up being 
> > empty.
> 
> True, I should fix the documentation.
> 
> It _might_ make sense for certain users like gitk and gitview if
> we had a single tool that gives --pretty and its diff even if
> the diff is empty.  Having said that, the flag --cc -m is too
> specific.  If some uses want to see the commit log even for an
> empty diff, that flag should not be something only --cc honors.

Here's an "--always" flag that does that.

		Linus

---
diff --git a/diff-tree.c b/diff-tree.c
index 6593a69..2df23c6 100644
--- a/diff-tree.c
+++ b/diff-tree.c
@@ -10,6 +10,7 @@ static int show_empty_combined = 0;
 static int combine_merges = 0;
 static int dense_combined_merges = 0;
 static int read_stdin = 0;
+static int always_show_header = 0;
 
 static const char *header = NULL;
 static const char *header_prefix = "";
@@ -93,6 +94,10 @@ static const char *generate_header(const
 	offset += pretty_print_commit(commit_format, commit, len,
 				      this_header + offset,
 				      sizeof(this_header) - offset, abbrev);
+	if (always_show_header) {
+		puts(this_header);
+		return NULL;
+	}
 	return this_header;
 }
 
@@ -262,6 +267,10 @@ int main(int argc, const char **argv)
 			no_commit_id = 1;
 			continue;
 		}
+		if (!strcmp(arg, "--always")) {
+			always_show_header = 1;
+			continue;
+		}
 		usage(diff_tree_usage);
 	}
 	if (diff_options.output_format == DIFF_FORMAT_PATCH)

Add a "git show" command to show a commit

[Linus Torvalds]

It's basically just "git-diff-tree" with pretty flags and a pager.

Signed-off-by: Linus Torvalds <torvalds@osdl.org>
---

This uses the "--always" flag, btw, so it depends on the previous patch I 
sent. If you disagree with that, just remove it.

This is actually something I do pretty often, and I've grown tired of 
doing

	git-diff-tree -p --pretty cmit-id | less -S

so this is really nothing but a fairly flexible replacement for that.

It tries to have the same logic as "git diff" for the flags. 

diff --git a/Makefile b/Makefile
index 2aa2385..473e58d 100644
--- a/Makefile
+++ b/Makefile
@@ -112,7 +112,7 @@ SCRIPT_SH = \
 	git-applymbox.sh git-applypatch.sh git-am.sh \
 	git-merge.sh git-merge-stupid.sh git-merge-octopus.sh \
 	git-merge-resolve.sh git-merge-ours.sh git-grep.sh \
-	git-lost-found.sh
+	git-lost-found.sh git-show.sh
 
 SCRIPT_PERL = \
 	git-archimport.perl git-cvsimport.perl git-relink.perl \
diff --git a/git-show.sh b/git-show.sh
new file mode 100644
index 0000000..476bca4
--- /dev/null
+++ b/git-show.sh
@@ -0,0 +1,23 @@
+#!/bin/sh
+
+rev=$(git-rev-parse --verify --revs-only --no-flags --sq --default HEAD "$@") || exit
+flags=$(git-rev-parse --no-revs --flags --sq "$@")
+files=$(git-rev-parse --no-revs --no-flags --sq "$@")
+
+# Match the behaviour of "git diff":
+#  - if we do not have --name-status, --name-only nor -r, default to -p.
+#  - if we do not have -B nor -C, default to -M.
+case " $flags " in
+*" '--name-status' "* | *" '--name-only' "* | *" '-r' "* )
+	;;
+*)
+	flags="$flags'-p' " ;;
+esac
+case " $flags " in
+*" '-"[BCM]* | *" '--find-copies-harder' "*)
+	;; # something like -M50.
+*)
+	flags="$flags'-M' " ;;
+esac
+
+eval "git-diff-tree --always --cc --pretty $flags $rev -- $files" | LESS=-S ${PAGER:-less}

Fix git-rev-parse over-eager errors

[Linus Torvalds]

Using "--verify" together with "--no-flags" makes perfect sense, but 
git-rev-parse would complain about it when it saw a flag, even though it 
would never actually use/output that flag.

This fixes it.

Signed-off-by: Linus Torvalds <torvalds@osdl.org>
---

This is independent of the "git show" patches, although the problem was 
triggered by the "git show" usage of git-rev-parse. It's a bug whether git 
show is merged or not, though.

diff --git a/rev-parse.c b/rev-parse.c
index 6bf205a..9cec33b 100644
--- a/rev-parse.c
+++ b/rev-parse.c
@@ -107,12 +107,15 @@ static void show_rev(int type, const uns
 }
 
 /* Output a flag, only if filter allows it. */
-static void show_flag(char *arg)
+static int show_flag(char *arg)
 {
 	if (!(filter & DO_FLAGS))
-		return;
-	if (filter & (is_rev_argument(arg) ? DO_REVS : DO_NOREV))
+		return 0;
+	if (filter & (is_rev_argument(arg) ? DO_REVS : DO_NOREV)) {
 		show(arg);
+		return 1;
+	}
+	return 0;
 }
 
 static void show_default(void)
@@ -296,9 +299,8 @@ int main(int argc, char **argv)
 				show_datestring("--min-age=", arg+8);
 				continue;
 			}
-			if (verify)
+			if (show_flag(arg) && verify)
 				die("Needed a single revision");
-			show_flag(arg);
 			continue;
 		}
 

Re: Fix git-rev-parse over-eager errors

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> Using "--verify" together with "--no-flags" makes perfect sense, but 
> git-rev-parse would complain about it when it saw a flag, even though it 
> would never actually use/output that flag.

Ah, makes sense.

Re: Fix git-rev-parse over-eager errors

[Linus Torvalds]

On Sun, 5 Feb 2006, Junio C Hamano wrote:
>
> Linus Torvalds <torvalds@osdl.org> writes:
> > 
> > Using "--verify" together with "--no-flags" makes perfect sense, but 
> > git-rev-parse would complain about it when it saw a flag, even though it 
> > would never actually use/output that flag.
> 
> Ah, makes sense.

Btw, I think the exact same holds true for the "show_file()" case.

You had added a special-case for a similar problem (the "lstat()" check) 
to disable the check when the path wasn't actually printed out. Strictly 
speaking, I think that should be handled the same way wrt "verify" too.

Ie something like this.

Comments?

		Linus

---
diff --git a/rev-parse.c b/rev-parse.c
index d2f0864..124d3ee 100644
--- a/rev-parse.c
+++ b/rev-parse.c
@@ -143,11 +143,14 @@ static void show_datestring(const char *
 	show(buffer);
 }
 
-static void show_file(const char *arg)
+static int show_file(const char *arg)
 {
 	show_default();
-	if ((filter & (DO_NONFLAGS|DO_NOREV)) == (DO_NONFLAGS|DO_NOREV))
+	if ((filter & (DO_NONFLAGS|DO_NOREV)) == (DO_NONFLAGS|DO_NOREV)) {
 		show(arg);
+		return 1;
+	}
+	return 0;
 }
 
 int main(int argc, char **argv)
@@ -308,14 +311,13 @@ int main(int argc, char **argv)
 			show_rev(REVERSED, sha1, arg+1);
 			continue;
 		}
+		as_is = 1;
+		if (!show_file(arg))
+			continue;
 		if (verify)
 			die("Needed a single revision");
-		if ((filter & DO_REVS) &&
-		    (filter & DO_NONFLAGS) && /* !def && */
-		    lstat(arg, &st) < 0)
+		if (lstat(arg, &st) < 0)
 			die("'%s': %s", arg, strerror(errno));
-		as_is = 1;
-		show_file(arg);
 	}
 	show_default();
 	if (verify && revs_count != 1)

Re: Fix git-rev-parse over-eager errors

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> Btw, I think the exact same holds true for the "show_file()" case.
>
> You had added a special-case for a similar problem (the "lstat()" check) 
> to disable the check when the path wasn't actually printed out. Strictly 
> speaking, I think that should be handled the same way wrt "verify" too.
>
> Ie something like this.
>
> Comments?

Applied, thanks.

Re: Add a "git show" command to show a commit

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> It's basically just "git-diff-tree" with pretty flags and a pager.

Isn't this "git whatchanged -n1 --cc --always"?

> It tries to have the same logic as "git diff" for the flags. 

Except that --cc does not make much sense without being -p, so
the logic to do --name-status and friends are pretty much
wasted, no?

Re: Add a "git show" command to show a commit

[Linus Torvalds]

On Sun, 5 Feb 2006, Junio C Hamano wrote:
> 
> Isn't this "git whatchanged -n1 --cc --always"?

Getting closer, yes.

Especially these days that you can write "-1" instead of "--max-count=1", 
and if we make "--cc" the default for "git-whatchanged", I guess we could 
just drop this.

> Except that --cc does not make much sense without being -p, so
> the logic to do --name-status and friends are pretty much
> wasted, no?

You're right. The "--cc" should be in the default flags (instead of -p), 
not unconditionally on the command line.

			Linus

Re: Add a "git show" command to show a commit

[Linus Torvalds]

On Sun, 5 Feb 2006, Linus Torvalds wrote:
> 
> You're right. The "--cc" should be in the default flags (instead of -p), 
> not unconditionally on the command line.

In that vein, for "git diff":

		Linus

----
diff --git a/git-diff.sh b/git-diff.sh
index 4812ae4..7b3dbe3 100755
--- a/git-diff.sh
+++ b/git-diff.sh
@@ -22,13 +22,13 @@ case "$rev" in
 	esac
 esac
 
-# If we do not have --name-status, --name-only nor -r, default to -p.
+# If we do not have --name-status, --name-only nor -r, default to --cc.
 # If we do not have -B nor -C, default to -M.
 case " $flags " in
 *" '--name-status' "* | *" '--name-only' "* | *" '-r' "* )
 	;;
 *)
-	flags="$flags'-p' " ;;
+	flags="$flags'--cc' " ;;
 esac
 case " $flags " in
 *" '-"[BCM]* | *" '--find-copies-harder' "*)

Re: Add a "git show" command to show a commit

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> On Sun, 5 Feb 2006, Linus Torvalds wrote:
>> 
>> You're right. The "--cc" should be in the default flags (instead of -p), 
>> not unconditionally on the command line.
>
> In that vein, for "git diff":

Yes, except that would break "git diff HEAD" ;-).  Only
diff-tree and diff-files understands --cc.  It means slightly
different thing for diff-files, but I do not think the
difference would matter (it shows unmerged case nicer).

I'll fix it up.