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!

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

[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 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...

[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

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

[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: 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: [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

RE: The merge from hell...

[Brown, Len]

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

Seems I'm batting 1000 for entertainment value
over my last two kernel patch pushes:-)

The previous one I took abuse for unwittingly cluttering
history with "extra" merges.  The thread went on
and on, but buried in there were some interesting
observations on work flow, and somebody
asserted that the "cleanest" way to cherry pick
the topic branches onto the release branch was
with a multi-branch merge.

As git merge seemed to advertise support for it w/o
me needing to learn a new command, I tried it out
and it seemed to work fine -- including a nice colorful
diagram in gitk:-)

I can do 16 next time, or 22, or none -- or you can have
git merge under the covers do this via iteration instead
of all at once -- that's up to you guys.  My topic branches
tend to be disjoint topics with little expected overlap;
so grabbing a bunch of them when they're fully "cooked"
in -mm and plunking them down in one merge is actually
an example of history matching reality.

cheers,
-Len

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: The merge from hell...

[Linus Torvalds]

On Thu, 2 Feb 2006, Brown, Len wrote:
> 
> I can do 16 next time, or 22, or none

Actually, you can't do 22:

	/*
	 * Having more than two parents is not strange at all, and this is
	 * how multi-way merges are represented.
	 */
	#define MAXPARENT (16)

(commit-tree.c).

Now, admittedly you should literally need no more than to change that 
#define and recompile, but at least by default, git-write-tree won't 
accept more than 16 parents.

The 12-way merge was a bit over the top, but it worked. I'd suggest not 
beign quite _that_ aggressive in the future, though, but it's not a big 
deal.

One thing I'd ask for: would it be possible to have more descriptive 
branch names than just numbers? Even if you want to track it by bugzilla 
entry number, how about calling it "bugzilla-12345" instead? 

I can make the educated guess that it's the bugzilla.kernel.org tracking 
number, but still.. I think it would make the changelog more readable and 
understandable to outsiders.

		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: The merge from hell...

[Brown, Len]

>The 12-way merge was a bit over the top, but it worked. I'd 
>suggest not being quite _that_ aggressive in the future,
>though, but it's not a big deal.

I favor "assertive" over "aggressive" --
I assert that the tools I use must work as advertised;-)

> #define MAXPARENT (16)

How about setting this #define to the number
that you're comfortable with?  Then folks like me --
who wouldn't dream of checking code into Linux with a hopped-up git --
will simply obey the limit that comes with the tool?

>One thing I'd ask for: would it be possible to have more descriptive 
>branch names than just numbers? Even if you want to track it 
>by bugzilla entry number, how about calling it "bugzilla-12345" instead? 

bugzilla-#### -- no problem -- will do.

thanks,
-Len

ps. 
In the back of my head I was worried about using plain
numbers when I saw somebody refer to "shorthand SHA1".
Hopefully this is an idle worry and it is not possible
for the tool to confuse a numeric branch name with a SHA1 id.

RE: The merge from hell...

[Linus Torvalds]

On Fri, 3 Feb 2006, Brown, Len wrote:
>
> In the back of my head I was worried about using plain
> numbers when I saw somebody refer to "shorthand SHA1".
> Hopefully this is an idle worry and it is not possible
> for the tool to confuse a numeric branch name with a SHA1 id.

It _is_ possible, but the rule is that references will be resolved first. 

If you mis-type a reference and it could be construed as a short hex SHA1 
ID (it needs to have more than 5 characters in it to trigger, though), the 
auto-completion of SHA1 ID's could bite you, though.

It's pretty unlikely, of course. But it's one reason to try to avoid using 
ref names that are numeric.

		Linus

Re: The merge from hell...

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> The 12-way merge was a bit over the top, but it worked. I'd suggest not 
> beign quite _that_ aggressive in the future, though, but it's not a big 
> deal.

Heh, I was quietly planning to raise the limit, or lift it
altogether ;-).

I find Len's explanation that those topics cooked independently
and happened to mature at about the same time an excellent
excuse to record this as an Octopus, and with that usage there
is no inherent reason, other than making the diff completely
unreadable, to limit the number of parents.  But I tend to agree
that the current 16 is a sane limit in practice.

That reminds me of another practical limit I've known but did
nothing about for quite some time (you may not even remember
doing that parser anymore).  This does not work for Len's merge:

	$ git rev-parse --verify funmerge^10

You could do a 16-way merge but 12-way is already hitting
usability limit, depending on what you would want to do with
them.  For example, you cannot easily decompose the topic
branches out of that merge, like this:

	$ git checkout -b redo-3549 funmerge^2     ;# works
        $ git checkout -b redo-pnpacpi funmerge^12 ;# doesn't

> One thing I'd ask for: would it be possible to have more descriptive 
> branch names than just numbers? Even if you want to track it by bugzilla 
> entry number, how about calling it "bugzilla-12345" instead? 

When kernel people (not just Len) talk about a "bugzilla ID",
does that ID always come from the same namespace, or do some
subsystems have their own bugzilla?

Re: The merge from hell...

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> On Fri, 3 Feb 2006, Brown, Len wrote:
>>
>> In the back of my head I was worried about using plain
>> numbers when I saw somebody refer to "shorthand SHA1".
>> Hopefully this is an idle worry and it is not possible
>> for the tool to confuse a numeric branch name with a SHA1 id.
>
> It _is_ possible, but the rule is that references will be resolved first. 

I have to admit that I had this broken for a while.  The
breakage was when you have the same numeric branch name _and_
tagname, then ref resolution was skipped and short SHA1 was
taken.  It _might_ have bitten somebody in real life until I
fixed it.  But I believe this is fixed now.

> It's pretty unlikely, of course. But it's one reason to try to avoid using 
> ref names that are numeric.

Or even non numeric ones, like "deadbeef".

Re: The merge from hell...

[linux]

While we're stress-testing the ystem, does anyone feel like fixing
git-rev-parse 9fdb62af92c741addbea15545f214a6e89460865^10 ?

The following is my attempt, but it doesn't seem sufficient...

$ ~/git/git-rev-parse 9fdb62af92c741addbea15545f214a6e89460865^{1,2,3,4,5,6,7,8,9,10,11,12}
3ee68c4af3fd7228c1be63254b9f884614f9ebb2
876c184b31dc73cc3f38c5b86dee55d091a56769
729b4d4ce1982c52040bbf22d6711cdf8db07ad8
cf82478840188f8c8494c1d7a668a8ae170d0e07
dacd9b80355525be0e3c519687868410e304ad1c
63c94b68ec30847a6e2b36651703f41066f91480
35f652b5ef4ef145ac5514f6302b3f4cebfbbad4
1a38416cea8ac801ae8f261074721f35317613dc
4a90c7e86202f46fa9af011bdbcdf36e355d1721
fatal: '9fdb62af92c741addbea15545f214a6e89460865^10': No such file or directory

(Placed in the public domain; go nuts.)

diff --git a/sha1_name.c b/sha1_name.c
index ba0747c..adf49d2 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -388,52 +388,33 @@ static int peel_onion(const char *name, 
 
 static int get_sha1_1(const char *name, int len, unsigned char *sha1)
 {
-	int parent, ret;
-	const char *cp;
-
-	/* foo^[0-9] or foo^ (== foo^1); we do not do more than 9 parents. */
-	if (len > 2 && name[len-2] == '^' &&
-	    name[len-1] >= '0' && name[len-1] <= '9') {
-		parent = name[len-1] - '0';
-		len -= 2;
-	}
-	else if (len > 1 && name[len-1] == '^') {
-		parent = 1;
-		len--;
-	} else
-		parent = -1;
-
-	if (parent >= 0)
-		return get_parent(name, len, sha1, parent);
-
-	/* "name~3" is "name^^^",
-	 * "name~12" is "name^^^^^^^^^^^^", and
-	 * "name~" and "name~0" are name -- not "name^0"!
-	 */
-	parent = 0;
-	for (cp = name + len - 1; name <= cp; cp--) {
-		int ch = *cp;
-		if ('0' <= ch && ch <= '9')
-			continue;
-		if (ch != '~')
-			parent = -1;
-		break;
+	int parent = 0, pow10 = 1;
+	const char *cp = name + len;
+	char ch = 0;	/* In case len == 0 */
+
+	/* Parse trailing number and check for ^5 or ~5 */
+	while (cp > name && (ch = *--cp) >= '0' && ch <= '9') {
+		parent += ch - '0' * pow10;
+		pow10 *= 10;
 	}
-	if (!parent && *cp == '~') {
-		int len1 = cp - name;
-		cp++;
-		while (cp < name + len)
-			parent = parent * 10 + *cp++ - '0';
-		return get_nth_ancestor(name, len1, sha1, parent);
+
+	/* Handle foo^[0-9]* case */
+	if (ch == '^') {
+		/* foo^ means foo^1, first parent */
+		if (cp + 1 == name)
+			parent = 1;
+		return get_parent(name, cp - name, sha1, parent);
 	}
+	/* Handle foo~[0-9]* case.  name~ = name~0 = name (not name^0!) */
+	if (ch == '~')
+		return get_nth_ancestor(name, cp - name, sha1, parent);
 
-	ret = peel_onion(name, len, sha1);
-	if (!ret)
+	if (!peel_onion(name, len, sha1))
 		return 0;
 
-	ret = get_sha1_basic(name, len, sha1);
-	if (!ret)
+	if (!get_sha1_basic(name, len, sha1))
 		return 0;
+
 	return get_short_sha1(name, len, sha1, 0);
 }
 

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: The merge from hell...

[Dave Jones]

On Thu, Feb 02, 2006 at 10:28:43PM -0800, Junio C Hamano wrote:

 > > One thing I'd ask for: would it be possible to have more descriptive 
 > > branch names than just numbers? Even if you want to track it by bugzilla 
 > > entry number, how about calling it "bugzilla-12345" instead? 
 > 
 > When kernel people (not just Len) talk about a "bugzilla ID",
 > does that ID always come from the same namespace, or do some
 > subsystems have their own bugzilla?

Not only do some subsystems have their own bugtracker (ALSA for eg),
but referring to 'bugzilla' alone is meaningless, as it could
mean bugme.osdl.org, bugzilla.redhat.com, bugzilla.novell.com,
bugzilla.ubuntu.com etc etc, all of which are a prime source of
juicy kernel bugs.

		Dave

RE: The merge from hell...

[Brown, Len]

>On Thu, Feb 02, 2006 at 10:28:43PM -0800, Junio C Hamano wrote:
>
> > > One thing I'd ask for: would it be possible to have more 
>descriptive 
> > > branch names than just numbers? Even if you want to track 
>it by bugzilla 
> > > entry number, how about calling it "bugzilla-12345" instead? 
> > 
> > When kernel people (not just Len) talk about a "bugzilla ID",
> > does that ID always come from the same namespace, or do some
> > subsystems have their own bugzilla?
>
>Not only do some subsystems have their own bugtracker (ALSA for eg),
>but referring to 'bugzilla' alone is meaningless, as it could
>mean bugme.osdl.org, bugzilla.redhat.com, bugzilla.novell.com,
>bugzilla.ubuntu.com etc etc, all of which are a prime source of
>juicy kernel bugs.

Naming the branch is just eye-candy for the merge comment.
My topic branch labels in refs/my-branch never get to kernel.org, so you're
not going to see the pretty green tags on topic branches branches that I see.

I include the full-URL of the bug report in the original commit comments
for those who are interested.  I think this it the important place to put it,
and in practice I've found it to be extremely useful.

-Len

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: The merge from hell...

[Junio C Hamano]

"Brown, Len" <len.brown@intel.com> writes:

> Naming the branch is just eye-candy for the merge comment.
> My topic branch labels in refs/my-branch never get to kernel.org, so you're
> not going to see the pretty green tags on topic branches branches that I see.
>
> I include the full-URL of the bug report in the original commit comments
> for those who are interested.  I think this it the important place to put it,
> and in practice I've found it to be extremely useful.

Both excellent points.

If I may digress,...

It appears most of the topic branches in that merge have only
one commit since they forked from trunk (the development track
led to the first parent of that merge), but some seem to have
more than one commits.

It might make sense if we have a tool support to pre-format the
merge messages like this, given set of branch names:

    [ACPI] Merge 3549, 4320, 4485, 4588, 4980, 5483, 5651, acpica, asus, fops and pnpacpi branches into release

    3549: [ACPI] Disable C2/C3 for _all_ IBM R40e Laptops
    4320: [ACPI] fix reboot upon suspend-to-disk
    4485: [ACPI] handle BIOS with implicit C1 in _CST
    4588: [ACPI] fix acpi_os_wait_sempahore() finite timeout case (AE_TIME warning)
    4980 (5 commits): [ACPI] build EC driver on IA64
    5483 (3 commits): [ACPI] fix acpi_cpufreq.c build warrning
    5651: [ACPI] SMP S3 resume: evaluate _WAK after INIT
    acpica (23 commits): [ACPI] ACPICA 20060113
    asus (3 commits): [ACPI_ASUS] fix asus module param description
    fops (2 commits): [ACPI] make two processor functions static
    pnpacpi (3 commits): [PNPACPI] clean excluded_id_list[]

Here I am counting the number of commits on each topic since it
last diverged from trunk (`merge-base release branch`), and
showing the latest commit of the topic.

We could even go further and have "per branch annotation" that
lets you do something like this:

	$ git checkout -b 3549 \
          --description 'http://bugzilla.kernel.org/show_bug.cgi?id=3549'
	$ work work
        $ git commit
	... work on other topics in similar way ...
        ... later, on the 'release' branch ...
        $ git pull . 3549 4320 4485...

With that, we could give a default merge message formatted like this:

    Merge 3549, 4320, 4485, 4588, 4980, 5483, 5651, acpica, asus, fops and pnpacpi branches into release

    3549: http://bugzilla.kernel.org/show_bug.cgi?id=3549
     [ACPI] Disable C2/C3 for _all_ IBM R40e Laptops

    4320: http://bugzilla.kernel.org/show_bug.cgi?id=4320
     [ACPI] fix reboot upon suspend-to-disk

    5483: http://bugzilla.kernel.org/show_bug.cgi?id=5483
     [ACPI] fix acpi_cpufreq.c build warrning
     [ACPI] IA64 ZX1 buildfix for _PDC patch
     [ACPI] Avoid BIOS inflicted crashes by evaluating _PDC only once

    pnpacpi: work on PNP-ACPI issues
     [PNPACPI] clean excluded_id_list[]
     [PNPACPI] Ignore devices that have no resources
     [ACPI] enable PNPACPI support for resource types used by HP serial ports

This last digression might be too much, though.  It may be
something that is better computed by 'git log' while reviewing
history, except that the description of each branch cannot be
given that way.

Re: The merge from hell...

[Linus Torvalds]

On Fri, 3 Feb 2006, Junio C Hamano wrote:
> 
> It might make sense if we have a tool support to pre-format the
> merge messages like this, given set of branch names:
> 
>     [ACPI] Merge 3549, 4320, 4485, 4588, 4980, 5483, 5651, acpica, asus, fops and pnpacpi branches into release
> 
>     3549: [ACPI] Disable C2/C3 for _all_ IBM R40e Laptops
>     4320: [ACPI] fix reboot upon suspend-to-disk
>     4485: [ACPI] handle BIOS with implicit C1 in _CST
..

Well, this is actually not all that different from what gitk will show you 
(since I added the commit "explanation" names with my increadible 
copy-paste skills to it).

Just look in the details window in gitk on that merge, and that's pretty 
much exactly what you'll see, except you'll also have the nice clickable 
hyperlink features ;)

Yeah, it doesn't show the branch names _and_ it shows the commit that you 
merged into too, so it looks like

  Parent: 3ee68.. ([SPARC64]: Use compat_sys_futimesat in 32-bit syscall table.)
  Parent: 876c1.. ([ACPI] Disable C2/C3 for _all_ IBM R40e Laptops)
  Parent: 729b4.. ([ACPI] fix reboot upon suspend-to-disk)
  Parent: cf824.. ([ACPI] handle BIOS with implicit C1 in _CST)

but it's actually pretty readable there.

		Linus

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: [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: [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: 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...

[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: 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.

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.