gitweb using "--cc"?

gitweb using "--cc"?

[Linus Torvalds]

I just did an arm merge that needed some (very trivial) manual fixups 
(commit ID cce0cac1, in case anybody cares).

As usual, git-diff-tree --cc does a beautiful job on it, but I also 
checked the gitweb output, which seems to not do as well (the commit 
message about a manual conflict merge doesn't make any sense at all).

Now, in this case, what gitweb shows is actually "sensible": it will show 
the diff of what the merge "brought in" to the mainline kernel, and in 
that sense I can certainly understand it. It basically diffs the merge 
against the first parent.

So looking at that particular example, arguably gitweb does something 
"different" from what the commit message is talking about, but in many 
ways it's a perfectly logical thing.

However, diffing against the first parent, while it sometimes happens to 
be a sane thing to do, really isn't very sane in general. The merge may go 
the other way (subdevelopers merging my code), like in commit b2faf597, 
and sometimes there might not be a single reference tree, but more of a 
"couple of main branches" approach with merging back and forth). Then the 
current gitweb behaviour makes no sense at all.

So it would be much nicer if gitweb had some alternate approach to showing 
merge diffs. My suggested approach would be to just let the user choose: 
have separate "diff against fist/second[/third[/..]] parent" buttons. And 
one of the choices would be the "conflict view" that git-diff-tree --cc 
gives (I'd argue for that being the default one, because it's the only one 
that doesn't have a "preferred parent").

Kay?

		Linus

Re: gitweb using "--cc"?

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> So it would be much nicer if gitweb had some alternate approach to showing 
> merge diffs. My suggested approach would be to just let the user choose: 
> have separate "diff against fist/second[/third[/..]] parent" buttons. And 
> one of the choices would be the "conflict view" that git-diff-tree --cc 
> gives (I'd argue for that being the default one, because it's the only one 
> that doesn't have a "preferred parent").

I have something very rough, but I will be sending them out
anyways.

[PATCH] Use describe to come up with the closest tag.

[Junio C Hamano]

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

---

 * This gives saner output than what is currently there.  It
   does not have direct relation to the diff-tree --cc patch,
   but over there I already use describe so this patch is to
   make things consistent.

 gitweb.cgi |   15 +++------------
 1 files changed, 3 insertions(+), 12 deletions(-)

3fb28888448edc8b2d4aeab55bc13d78746e5a45
diff --git a/gitweb.cgi b/gitweb.cgi
index c1bb624..3c43695 100755
--- a/gitweb.cgi
+++ b/gitweb.cgi
@@ -2051,20 +2051,11 @@ sub git_commitdiff_plain {
 	close $fd or die_error(undef, "Reading diff-tree failed.");
 
 	# try to figure out the next tag after this commit
-	my $tagname;
 	my $refs = read_info_ref("tags");
-	open $fd, "-|", "$gitbin/git-rev-list HEAD";
-	chomp (my (@commits) = <$fd>);
+	open $fd, "-|", "$gitbin/git-describe $hash";
+	my ($tagname) = <$fd>;
+	chomp($tagname);
 	close $fd;
-	foreach my $commit (@commits) {
-		if (defined $refs->{$commit}) {
-			$tagname = $refs->{$commit}
-		}
-		if ($commit eq $hash) {
-			last;
-		}
-	}
-
 	print $cgi->header(-type => "text/plain", -charset => 'utf-8', '-content-disposition' => "inline; filename=\"git-$hash.patch\"");
 	my %co = git_read_commit($hash);
 	my %ad = date_str($co{'author_epoch'}, $co{'author_tz'});
-- 
1.1.6.gbb042

[PATCH] Allow using --cc when showing a merge.

[Junio C Hamano]

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

---

 * This does not do the colorized diff, but just to show ideas
   where to put the link to ask for the combined diff.

 gitweb.cgi |   60 +++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 47 insertions(+), 13 deletions(-)

1f19febaefbf90dc04a6b37d79ba3a9337decaff
diff --git a/gitweb.cgi b/gitweb.cgi
index c1bb624..d2659ea 100755
--- a/gitweb.cgi
+++ b/gitweb.cgi
@@ -191,6 +191,9 @@ if (!defined $action || $action eq "summ
 } elsif ($action eq "commitdiff_plain") {
 	git_commitdiff_plain();
 	exit;
+} elsif ($action eq "combinediff") {
+	git_combinediff();
+	exit;
 } elsif ($action eq "history") {
 	git_history();
 	exit;
@@ -1762,7 +1765,15 @@ sub git_commit {
 	      "</tr>\n";
 	print "<tr><td>committer</td><td>" . esc_html($co{'committer'}) . "</td></tr>\n";
 	print "<tr><td></td><td> $cd{'rfc2822'}" . sprintf(" (%02d:%02d %s)", $cd{'hour_local'}, $cd{'minute_local'}, $cd{'tz_local'}) . "</td></tr>\n";
-	print "<tr><td>commit</td><td style=\"font-family:monospace\">$co{'id'}</td></tr>\n";
+	print "<tr><td>commit</td><td style=\"font-family:monospace\">$co{'id'}</td>";
+	if (1 < @{$co{'parents'}}) {
+		print '<td class="link">';
+		print $cgi->a({-href => "$my_uri?" .
+				   esc_param("p=$project;a=combinediff;".
+					     "h=$hash")}, "combinediff");
+		print '</td>';
+	}
+	print "</tr>\n";
 	print "<tr>" .
 	      "<td>tree</td>" .
 	      "<td style=\"font-family:monospace\">" .
@@ -2044,6 +2055,38 @@ sub git_commitdiff {
 	git_footer_html();
 }
 
+sub git_combinediff {
+	mkdir($git_temp, 0700);
+	my $fd;
+	my $refs = read_info_ref("tags");
+	open $fd, "-|", "$gitbin/git-describe $hash";
+	my ($tagname) = <$fd>;
+	chomp($tagname);
+	close $fd;
+	print $cgi->header(-type => "text/plain", -charset => 'utf-8', '-content-disposition' => "inline; filename=\"git-$hash.patch\"");
+	my %co = git_read_commit($hash);
+	my %ad = date_str($co{'author_epoch'}, $co{'author_tz'});
+	my $comment = $co{'comment'};
+	print "From: $co{'author'}\n" .
+	      "Date: $ad{'rfc2822'} ($ad{'tz_local'})\n".
+	      "Subject: $co{'title'}\n";
+	if (defined $tagname) {
+	      print "X-Git-Tag: $tagname\n";
+	}
+	print "\n";
+
+	foreach my $line (@$comment) {;
+		print "$line\n";
+	}
+	print "---\n\n";
+
+	open $fd, "-|", "$gitbin/git-diff-tree --cc $hash";
+	while (<$fd>) {
+		print $_;
+	}
+	close $fd;
+}
+
 sub git_commitdiff_plain {
 	mkdir($git_temp, 0700);
 	open my $fd, "-|", "$gitbin/git-diff-tree -r $hash_parent $hash" or die_error(undef, "Open failed.");
@@ -2051,20 +2094,11 @@ sub git_commitdiff_plain {
 	close $fd or die_error(undef, "Reading diff-tree failed.");
 
 	# try to figure out the next tag after this commit
-	my $tagname;
 	my $refs = read_info_ref("tags");
-	open $fd, "-|", "$gitbin/git-rev-list HEAD";
-	chomp (my (@commits) = <$fd>);
+	open $fd, "-|", "$gitbin/git-describe $hash";
+	my ($tagname) = <$fd>;
+	chomp($tagname);
 	close $fd;
-	foreach my $commit (@commits) {
-		if (defined $refs->{$commit}) {
-			$tagname = $refs->{$commit}
-		}
-		if ($commit eq $hash) {
-			last;
-		}
-	}
-
 	print $cgi->header(-type => "text/plain", -charset => 'utf-8', '-content-disposition' => "inline; filename=\"git-$hash.patch\"");
 	my %co = git_read_commit($hash);
 	my %ad = date_str($co{'author_epoch'}, $co{'author_tz'});
-- 
1.1.6.gbb042

Re: gitweb using "--cc"?

[Brian Gerst]

Linus Torvalds wrote:
> I just did an arm merge that needed some (very trivial) manual fixups 
> (commit ID cce0cac1, in case anybody cares).
> 
> As usual, git-diff-tree --cc does a beautiful job on it, but I also 
> checked the gitweb output, which seems to not do as well (the commit 
> message about a manual conflict merge doesn't make any sense at all).
> 
> Now, in this case, what gitweb shows is actually "sensible": it will show 
> the diff of what the merge "brought in" to the mainline kernel, and in 
> that sense I can certainly understand it. It basically diffs the merge 
> against the first parent.
> 
> So looking at that particular example, arguably gitweb does something 
> "different" from what the commit message is talking about, but in many 
> ways it's a perfectly logical thing.
> 
> However, diffing against the first parent, while it sometimes happens to 
> be a sane thing to do, really isn't very sane in general. The merge may go 
> the other way (subdevelopers merging my code), like in commit b2faf597, 
> and sometimes there might not be a single reference tree, but more of a 
> "couple of main branches" approach with merging back and forth). Then the 
> current gitweb behaviour makes no sense at all.
> 
> So it would be much nicer if gitweb had some alternate approach to showing 
> merge diffs. My suggested approach would be to just let the user choose: 
> have separate "diff against fist/second[/third[/..]] parent" buttons. And 
> one of the choices would be the "conflict view" that git-diff-tree --cc 
> gives (I'd argue for that being the default one, because it's the only one 
> that doesn't have a "preferred parent").
> 
> Kay?
> 
> 		Linus
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

git-whatchanged doesn't show that merge commit either.

--
				Brian Gerst

Re: gitweb using "--cc"?

[Linus Torvalds]

On Wed, 8 Feb 2006, Brian Gerst wrote:
> 
> git-whatchanged doesn't show that merge commit either.

Actually, it does. You just have to ask it.

	git-whatchanged --cc

The thing is, "git-whatchanged" is different from "git diff" and other 
helpers, in that it by default shows the "raw" git representation. Which 
indeed doesn't show that merge as being anything interesting.

But with "--cc", the merge suddenly blossoms.

Now, arguably, the raw format should default to the same kind of "were 
there data conflicts" that "-c" does for merges, but it doesn't, so it's 
silent ;(

		Linus

Re: gitweb using "--cc"?

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> Now, arguably, the raw format should default to the same kind of "were 
> there data conflicts" that "-c" does for merges, but it doesn't, so it's 
> silent ;(

True.  There was a discussion to come up with a sensible
semantics for -c without -p (currently --cc and -c implies -p),
but I haven't got around to it, since --cc was more useful in
general.

Volunteers?

Re: gitweb using "--cc"?

[Linus Torvalds]

On Wed, 8 Feb 2006, Junio C Hamano wrote:
> 
> True.  There was a discussion to come up with a sensible
> semantics for -c without -p (currently --cc and -c implies -p),
> but I haven't got around to it, since --cc was more useful in
> general.
> 
> Volunteers?

This is a first try at it.

NOTE! This makes "-c" be the default, which effectively means that merges 
are never ignored any more, and "-m" is a no-op. So it changes semantics.

I would also like to make "--cc" the default if you do patches, but didn't 
actually do that.

The raw output format is not wonderfully pretty, but it's distinguishable 
from a "normal patch" in that a normal patch with just one parent has just 
one colon at the beginning, while a multi-parent raw diff has <n> colons 
for <n> parents.

So now, in the kernel, when you do

	git-diff-tree cce0cac125623f9b68f25dd1350f6d616220a8dd

(to see the manual ARM merge that had a conflict in arch/arm/Kconfig), you 
get

	cce0cac125623f9b68f25dd1350f6d616220a8dd
	::100644 100644 100644 4a63a8e2e45247a11c068c6ed66c6e7aba29ddd9 77eee38762d69d3de95ae45dd9278df9b8225e2c 2f61726d2f4b636f6e66696700dbf71a59dad287       arch/arm/Kconfig

ie you see two colons (two parents), then three modes (parent modes 
followed by result mode), then three sha1s (parent sha1s followed by
result sha1).

Which is pretty close to the normal raw diff output.

NOTE! There are a few known issues:

 - a normal raw output will also do the "what happened" status character. 
   I didn't. I'm stupid and lazy. It's not strictly needed (since it's 
   obvious from the multiple colons), but I suspect we should do 
   something to perhaps clarify what it is. Or just put "M" for "modified 
   in merge")

 - It doesn't honor the "EOL" character, so "git-diff-tree -z" does the 
   wrong thing. Gaah. I should have just passed down the whole 
   "diff_options" instead of just the format. I'm a retard.

but it's a beginning..

		Linus

---
diff --git a/combine-diff.c b/combine-diff.c
index 6a9f368..935ba2a 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -776,8 +776,35 @@ int show_combined_diff(struct combine_di
 	return shown_header;
 }
 
+#define COLONS "::::::::::::::::::::::::::::::::"
+
+static void show_raw_diff(struct combine_diff_path *p, int num_parent, const char *header)
+{
+	int i, offset;
+	const char *prefix;
+
+	if (header)
+		puts(header);
+	offset = strlen(COLONS) - num_parent;
+	if (offset < 0)
+		offset = 0;
+	prefix = COLONS + offset;
+
+	/* Show the modes */
+	for (i = 0; i < num_parent; i++) {
+		printf("%s%06o", prefix, p->parent[i].mode);
+		prefix = " ";
+	}
+	printf("%s%06o", prefix, p->mode);
+	for (i = 0; i < num_parent; i++) {
+		printf("%s%s", prefix, sha1_to_hex(p->parent[i].sha1));
+		prefix = " ";
+	}
+	printf("%s%s\t%s\n", prefix, sha1_to_hex(p->parent[i].sha1), p->path);
+}
+
 int diff_tree_combined_merge(const unsigned char *sha1,
-			     const char *header, int dense)
+			     const char *header, int dense, int format)
 {
 	struct commit *commit = lookup_commit(sha1);
 	struct diff_options diffopts;
@@ -815,6 +842,11 @@ int diff_tree_combined_merge(const unsig
 		for (p = paths; p; p = p->next) {
 			if (!p->len)
 				continue;
+			if (format == DIFF_FORMAT_RAW) {
+				show_raw_diff(p, num_parent, header);
+				header = NULL;
+				continue;
+			}
 			if (show_combined_diff(p, num_parent, dense, header))
 				header = NULL;
 		}
diff --git a/diff-tree.c b/diff-tree.c
index 7148323..78bce06 100644
--- a/diff-tree.c
+++ b/diff-tree.c
@@ -6,7 +6,7 @@ static int show_root_diff = 0;
 static int no_commit_id = 0;
 static int verbose_header = 0;
 static int ignore_merges = 1;
-static int combine_merges = 0;
+static int combine_merges = 1;
 static int dense_combined_merges = 0;
 static int read_stdin = 0;
 static int always_show_header = 0;
@@ -118,7 +118,8 @@ static int diff_tree_commit(struct commi
 		else if (combine_merges) {
 			header = generate_header(sha1, sha1, commit);
 			return diff_tree_combined_merge(sha1, header,
-							dense_combined_merges);
+							dense_combined_merges,
+							diff_options.output_format);
 		}
 	}
 
@@ -285,10 +286,12 @@ int main(int argc, const char **argv)
 		usage(diff_tree_usage);
 	}
 
-	if (combine_merges) {
-		diff_options.output_format = DIFF_FORMAT_PATCH;
+	if (combine_merges)
 		ignore_merges = 0;
-	}
+
+	/* We can only do dense combined merges with diff output */
+	if (dense_combined_merges)
+		diff_options.output_format = DIFF_FORMAT_PATCH;
 
 	if (diff_options.output_format == DIFF_FORMAT_PATCH)
 		diff_options.recursive = 1;
diff --git a/diff.h b/diff.h
index 5c5e7fa..f7b3d2a 100644
--- a/diff.h
+++ b/diff.h
@@ -77,7 +77,7 @@ struct combine_diff_path {
 int show_combined_diff(struct combine_diff_path *elem, int num_parent,
 		       int dense, const char *header);
 
-extern int diff_tree_combined_merge(const unsigned char *sha1, const char *, int);
+extern int diff_tree_combined_merge(const unsigned char *sha1, const char *, int, int);
 
 extern void diff_addremove(struct diff_options *,
 			   int addremove,

Re: gitweb using "--cc"?

[Linus Torvalds]

On Thu, 9 Feb 2006, Linus Torvalds wrote:
> 
> This is a first try at it.

Btw, if it wasn't clear, this patch _does_ fix the fact that 
"git-whatchanged" didn't show merges even if they have conflicts.

Of course, since the raw format is equivalent to "-c" and only bases its 
"should I show" logic on whether the file has changed at all, it very 
fundamentally will never be able to tell the difference between a real 
content conflict and something that just had file-level automatic merging. 

So "git-whatchanged" will now show a lot of merges that didn't really 
change anything, but that just merged on a file level (ie the whole merge 
just goes away when you specify "--cc").

Still, that's actually interesting information too, so you can consider 
this a feature.

		Linus

Re: gitweb using "--cc"?

[Linus Torvalds]

[ Apologies if this comes several times. I had a few failed attempts ]

On Thu, 9 Feb 2006, Linus Torvalds wrote:
> 
> NOTE! There are a few known issues:
> 
>  - a normal raw output will also do the "what happened" status character. 
>    I didn't. I'm stupid and lazy. It's not strictly needed (since it's 
>    obvious from the multiple colons), but I suspect we should do 
>    something to perhaps clarify what it is. Or just put "M" for "modified 
>    in merge")
> 
>  - It doesn't honor the "EOL" character, so "git-diff-tree -z" does the 
>    wrong thing. Gaah. I should have just passed down the whole 
>    "diff_options" instead of just the format. I'm a retard.

A few more:

 - it doesn't honor "--abbrev", which it really should

 - git-diff-tree doesn't do the right thing for "header_prefix".

Anyway, this updated patch (throw the old one away) should fix all these 
issues.

Cool/stupid exercise:

	git-whatchanged | grep '^::' | cut -f2- | sort | uniq -c | sort -n | less -S

will show which files have needed the most file-level merge conflict 
resolution. Useful? Probably not. But kind of interesting (for the kernel, 
it's 

     ....
     10 arch/ia64/Kconfig
     11 drivers/scsi/Kconfig
     12 drivers/net/Makefile
     17 include/linux/libata.h
     18 include/linux/pci_ids.h
     23 drivers/net/Kconfig
     24 drivers/scsi/libata-scsi.c
     28 drivers/scsi/libata-core.c
     43 MAINTAINERS

in case anybody cares).

		Linus

---
diff --git a/combine-diff.c b/combine-diff.c
index 6a9f368..15f369e 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -776,8 +776,52 @@ int show_combined_diff(struct combine_di
 	return shown_header;
 }
 
-int diff_tree_combined_merge(const unsigned char *sha1,
-			     const char *header, int dense)
+#define COLONS "::::::::::::::::::::::::::::::::"
+
+static void show_raw_diff(struct combine_diff_path *p, int num_parent, const char *header, struct diff_options *opt)
+{
+	int i, offset, mod_type = 'A';
+	const char *prefix;
+	int line_termination, inter_name_termination;
+
+	line_termination = opt->line_termination;
+	inter_name_termination = '\t';
+	if (!line_termination)
+		inter_name_termination = 0;
+
+	if (header)
+		puts(header);
+	offset = strlen(COLONS) - num_parent;
+	if (offset < 0)
+		offset = 0;
+	prefix = COLONS + offset;
+
+	/* Show the modes */
+	for (i = 0; i < num_parent; i++) {
+		int mode = p->parent[i].mode;
+		if (mode)
+			mod_type = 'M';
+		printf("%s%06o", prefix, mode);
+		prefix = " ";
+	}
+	printf("%s%06o", prefix, p->mode);
+	if (!p->mode)
+		mod_type = 'D';
+
+	/* Show sha1's */
+	for (i = 0; i < num_parent; i++) {
+		printf("%s%s", prefix, diff_unique_abbrev(p->parent[i].sha1, opt->abbrev));
+		prefix = " ";
+	}
+	printf("%s%s", prefix, diff_unique_abbrev(p->sha1, opt->abbrev));
+
+	/* Modification type, terminations, filename */
+	printf(" %c%c%s%c", mod_type, inter_name_termination, p->path, line_termination);
+}
+
+const char *diff_tree_combined_merge(const unsigned char *sha1,
+			     const char *header, int dense,
+			     struct diff_options *opt)
 {
 	struct commit *commit = lookup_commit(sha1);
 	struct diff_options diffopts;
@@ -815,6 +859,11 @@ int diff_tree_combined_merge(const unsig
 		for (p = paths; p; p = p->next) {
 			if (!p->len)
 				continue;
+			if (opt->output_format == DIFF_FORMAT_RAW) {
+				show_raw_diff(p, num_parent, header, opt);
+				header = NULL;
+				continue;
+			}
 			if (show_combined_diff(p, num_parent, dense, header))
 				header = NULL;
 		}
@@ -826,5 +875,5 @@ int diff_tree_combined_merge(const unsig
 		paths = paths->next;
 		free(tmp);
 	}
-	return 0;
+	return header;
 }
diff --git a/diff-tree.c b/diff-tree.c
index 7148323..df6fd97 100644
--- a/diff-tree.c
+++ b/diff-tree.c
@@ -6,7 +6,7 @@ static int show_root_diff = 0;
 static int no_commit_id = 0;
 static int verbose_header = 0;
 static int ignore_merges = 1;
-static int combine_merges = 0;
+static int combine_merges = 1;
 static int dense_combined_merges = 0;
 static int read_stdin = 0;
 static int always_show_header = 0;
@@ -117,8 +117,12 @@ static int diff_tree_commit(struct commi
 			return 0;
 		else if (combine_merges) {
 			header = generate_header(sha1, sha1, commit);
-			return diff_tree_combined_merge(sha1, header,
-							dense_combined_merges);
+			header = diff_tree_combined_merge(sha1, header,
+							dense_combined_merges,
+							&diff_options);
+			if (!header && verbose_header)
+				header_prefix = "\ndiff-tree ";
+			return 0;
 		}
 	}
 
@@ -285,10 +289,12 @@ int main(int argc, const char **argv)
 		usage(diff_tree_usage);
 	}
 
-	if (combine_merges) {
-		diff_options.output_format = DIFF_FORMAT_PATCH;
+	if (combine_merges)
 		ignore_merges = 0;
-	}
+
+	/* We can only do dense combined merges with diff output */
+	if (dense_combined_merges)
+		diff_options.output_format = DIFF_FORMAT_PATCH;
 
 	if (diff_options.output_format == DIFF_FORMAT_PATCH)
 		diff_options.recursive = 1;
diff --git a/diff.h b/diff.h
index 5c5e7fa..9088519 100644
--- a/diff.h
+++ b/diff.h
@@ -74,10 +74,10 @@ struct combine_diff_path {
 	(sizeof(struct combine_diff_path) + \
 	 sizeof(struct combine_diff_parent) * (n) + (l) + 1)
 
-int show_combined_diff(struct combine_diff_path *elem, int num_parent,
-		       int dense, const char *header);
+extern int show_combined_diff(struct combine_diff_path *elem, int num_parent,
+			      int dense, const char *header);
 
-extern int diff_tree_combined_merge(const unsigned char *sha1, const char *, int);
+extern const char *diff_tree_combined_merge(const unsigned char *sha1, const char *, int, struct diff_options *opt);
 
 extern void diff_addremove(struct diff_options *,
 			   int addremove,

Re: gitweb using "--cc"?

[Junio C Hamano]

I was wondering if we could teach not diff_tree_combined_merge
but show_combined_diff to do this, so that diff-files -c would
benefit from the raw output as wel.

That aside, one remaining nit with your patch is printing
p->path.  diff.c::diff_flush_raw() does something like this:

	if (line_termination) {
		path_one = quote_one(path_one);
		path_two = quote_one(path_two);
	}
	...
	printf("%s%c%s", status, inter_name_termination, path_one);

But otherwise from a cursory look the patch appears correct.

Thanks.

Re: gitweb using "--cc"?

[Linus Torvalds]

On Thu, 9 Feb 2006, Junio C Hamano wrote:
>
> I was wondering if we could teach not diff_tree_combined_merge
> but show_combined_diff to do this, so that diff-files -c would
> benefit from the raw output as wel.

I wanted to do it that way, but it seemed less intrusive this way.

I agree that it is the correct thing to do, though.

> That aside, one remaining nit with your patch is printing
> p->path.  diff.c::diff_flush_raw() does something like this:
> 
> 	if (line_termination) {
> 		path_one = quote_one(path_one);
> 		path_two = quote_one(path_two);
> 	}
> 	...
> 	printf("%s%c%s", status, inter_name_termination, path_one);

Good point.

I found another nitpick: file removal doesn't seem to generate a good 
diff in "git-diff-tree --cc" (but it's correct in the new "raw" format 
diff).

Here's a test-case, in case you care. Do "git-diff-tree --cc HEAD" in the 
merge-test directory.

Finally, I think it would be good to have a "--ignore-mode" flag that 
drops the mode info from the raw format (that repeating "100644" really 
isn't very interesting, and caring about mode changes is pretty rare).

		Linus

---
#!/bin/sh
rm -rf merge-test
mkdir merge-test
cd merge-test/
git-init-db 

echo "hello" > a
echo "hi there" > b
git add a b
git commit -m "Initial commit of 'a' and 'b'"
git branch other

echo "different hello" > a
git commit -m "Changed 'a'" a

git checkout other
echo "another different hello" > a
git commit -m "Changed 'a' differently" a

git checkout master
git merge "merge other" HEAD other >& /dev/null

echo "final hello" > a
rm -f b
echo "new file" > c
git-update-index --add --remove a b c
git commit -m "Evil merge"

Re: gitweb using "--cc"?

[Linus Torvalds]

On Thu, 9 Feb 2006, Linus Torvalds wrote:
> 
> Here's a test-case, in case you care. Do "git-diff-tree --cc HEAD" in the 
> merge-test directory.

Btw, that test-case is also designed to show the different M/A/D cases for 
the merge result. The merge diff obviously doesn't do rename/copy 
detection (I don't think it's necessarily even a well-defined op, or if 
it is, it's damn complicated).

		Linus

Re: gitweb using "--cc"?

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> On Thu, 9 Feb 2006, Linus Torvalds wrote:
>> 
>> Here's a test-case, in case you care. Do "git-diff-tree --cc HEAD" in the 
>> merge-test directory.
>
> Btw, that test-case is also designed to show the different M/A/D cases for 
> the merge result. The merge diff obviously doesn't do rename/copy 
> detection (I don't think it's necessarily even a well-defined op, or if 
> it is, it's damn complicated).

Although I've never seriously tried it, I think "diff-tree --cc"
with -M or -C should do a decent job.  The initial phase to feed
combine-diff runs with the supplied rename/copy options if I am
not mistaken, and from the result it grabs one->{sha1,mode} (for
parent) and two->{sha1,mode} (obviously for merge result) to
feed combine-diff logic, while discarding one->path information.

So obviously it would show the final paths and would not talk
about which different path from each parent contributed to the
result, but otherwise it should not be broken too much.  At
least that was the way I intended..

Re: gitweb using "--cc"?

[Junio C Hamano]

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

> So obviously it would show the final paths and would not talk
> about which different path from each parent contributed to the
> result, but otherwise it should not be broken too much.  At
> least that was the way I intended..

Sorry, I am wrong again.  That was the way how I planned to, but
I think I forgot to pass the diff-options from the caller to
diff_tree_combined_merge(), so it does not do renames/copies.

Shouldn't be too hard to change it though...

Re: gitweb using "--cc"?

[Junio C Hamano]

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

> Junio C Hamano <junkio@cox.net> writes:
>
>> So obviously it would show the final paths and would not talk
>> about which different path from each parent contributed to the
>> result, but otherwise it should not be broken too much.  At
>> least that was the way I intended..
>
> Sorry, I am wrong again.  That was the way how I planned to, but
> I think I forgot to pass the diff-options from the caller to
> diff_tree_combined_merge(), so it does not do renames/copies.
>
> Shouldn't be too hard to change it though...

On top of your patch, it was quite easy ;-)

After the "Evil merge" in your test script, I added these:

        for i in a b c d e f g h i j k l m n; do echo $i; done >d
        git-update-index --add d
        git commit -m 'Add d'

        git checkout other
        git merge fast HEAD master

        mv d e
        echo o >>e
        git-update-index --add --remove d e
        git commit -m 'Move-edit d to e'

        git checkout master
        git merge -s recursive 'Merge' HEAD other

        git diff-tree -M --cc HEAD

diff --git a/combine-diff.c b/combine-diff.c
index 15f369e..2a0ec10 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -829,7 +829,7 @@ const char *diff_tree_combined_merge(con
 	struct combine_diff_path *p, *paths = NULL;
 	int num_parent, i, num_paths;
 
-	diff_setup(&diffopts);
+	diffopts = *opt;
 	diffopts.output_format = DIFF_FORMAT_NO_OUTPUT;
 	diffopts.recursive = 1;
 

Re: gitweb using "--cc"?

[Junio C Hamano]

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

> On top of your patch, it was quite easy ;-)
>
> After the "Evil merge" in your test script, I added these:
>
>...
>
>         git diff-tree -M --cc HEAD

Sorry for the noise.  The test was broken.

Re: gitweb using "--cc"?

[Junio C Hamano]

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

> Junio C Hamano <junkio@cox.net> writes:
>
>> On top of your patch, it was quite easy ;-)
>>
>> After the "Evil merge" in your test script, I added these:
>>
>>...
>>
>>         git diff-tree -M --cc HEAD
>
> Sorry for the noise.  The test was broken.

Second try.  Sorry again for the previous noise.

This time I made sure I am getting double-colon output;
here is what I added after your test script:

        for i in a b c d e f g h i j k l m n; do echo $i; done >d
        git-update-index --add d
        git commit -m 'Add d'

        git checkout other
        git merge fast HEAD master

        echo o >>d
        git-update-index d
        git commit -m 'Edit d'

        git checkout master
        echo 'Another' >>a
        git-update-index a
        git commit -m 'Modify a'
        git merge --no-commit 'Merge' HEAD other
        echo Extra >>a
        mv d e
        echo extra >>e
        git update-index --add --remove a d e
        git commit -m 'Evil again'

        git diff-tree -M -c HEAD

But you are right.  Rename detection with combined diff has a
funny semantics:

    diff-tree 9df5f2d... (from parents)
    Merge: 1da47fa... 09eee61...
    Author: Junio C Hamano <junkio@cox.net>
    Date:   Thu Feb 9 14:11:13 2006 -0800

        Evil again

    ::100644 100644 100644 8c3beaf... aad9366... c54c990... M	a
    ::100644 100644 100644 4f7cbe7... f8c295c... 19d5d80... M	e

This is showing that what was "d" was somehow magically called
"e" in the merge result with its own changes.  --cc output is
more interesting but the point is there is no sign of "d" in its
output, which does not feel right.

If we really care, we could show a status letter for each parent
(both are renames in this case but it is plausible one parent is
rename-edit and another is modify) and the original path in each
parent.

Does it matter?  I presume that a Porcelain that cares would
rather use the traditional "diff-tree -m -r" to look at diff
with each parent.  I dunno.

---
diff --git a/combine-diff.c b/combine-diff.c
index 15f369e..6d78305 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -829,7 +829,7 @@ const char *diff_tree_combined_merge(con
 	struct combine_diff_path *p, *paths = NULL;
 	int num_parent, i, num_paths;
 
-	diff_setup(&diffopts);
+	diffopts = *opt;
 	diffopts.output_format = DIFF_FORMAT_NO_OUTPUT;
 	diffopts.recursive = 1;
 
@@ -846,6 +846,7 @@ const char *diff_tree_combined_merge(con
 		struct commit *parent = parents->item;
 		diff_tree_sha1(parent->object.sha1, commit->object.sha1, "",
 			       &diffopts);
+		diffcore_std(&diffopts);
 		paths = intersect_paths(paths, i, num_parent);
 		diff_flush(&diffopts);
 	}

Re: gitweb using "--cc"?

[Marco Costalba]

On 2/9/06, Junio C Hamano <junkio@cox.net> wrote:
>
> Does it matter?  I presume that a Porcelain that cares would
> rather use the traditional "diff-tree -m -r" to look at diff
> with each parent.  I dunno.
>

Yes, please preserve this behaviour.

I pulled today the diff-tree -c semantic change and now I see, in git archive:

$ git-diff-tree -r ca182053c7710a286d72102f4576cf32e0dafcfb
ca182053c7710a286d72102f4576cf32e0dafcfb
::100644 100644 100644 538d21d808b7ccc287e7bdd947f1583eadcda28b
30479b4a19805132a16facf6342b1438427486b7
59042d1bc9ee65063455b50a0968efb0b8182577 MM        Makefile

$ git-diff-tree -r -m ca182053c7710a286d72102f4576cf32e0dafcfb
ca182053c7710a286d72102f4576cf32e0dafcfb
:100644 100644 538d21d808b7ccc287e7bdd947f1583eadcda28b
59042d1bc9ee65063455b50a0968efb0b8182577 M      Makefile
:100644 100644 410b758aab7efc6d777f0344500f97b1cbc52946
6c47c3a3e1acb8badaadad42dfe3d0bd7a06cac3 M      entry.c
ca182053c7710a286d72102f4576cf32e0dafcfb
:100644 100644 30479b4a19805132a16facf6342b1438427486b7
59042d1bc9ee65063455b50a0968efb0b8182577 M      Makefile

Please _do not_ change this behaviour to make -m a no-op as stated in
"diff-tree -c raw output" patch message
(ee63802422af14e43eccce3c6dc4150a27ceb1a3).

qgit has the possibility to switch from "see all merge files" to "see
interesting only", so
we really need that difference between 'git-diff-tree -r' and
'git-diff-tree -r -m'

Anyhow I am very happy with this change because it broke qgit ;-) but when fixed
it will have a lot of code removed and will be faster too.

Thanks
Marco

Re: gitweb using "--cc"?

[Junio C Hamano]

Marco Costalba <mcostalba@gmail.com> writes:

> On 2/9/06, Junio C Hamano <junkio@cox.net> wrote:
>>
>> Does it matter?  I presume that a Porcelain that cares would
>> rather use the traditional "diff-tree -m -r" to look at diff
>> with each parent.  I dunno.
>
> Yes, please preserve this behaviour.
>...
> Please _do not_ change this behaviour to make -m a no-op as stated in
> "diff-tree -c raw output" patch message
> (ee63802422af14e43eccce3c6dc4150a27ceb1a3).

The one you pulled already contains another one to fix that
ee6380 change done by gittus.  What "diff-tree -r -m ca1820"
shows should be the same as traditional "diff-tree -r -m ca1820"
output.

What is different is "diff-tree ca1820".  It used to show
*nothing* only because it is a merge.  It now defaults to show
"diff-tree -c ca1820".

For the sake of backward compatibility we could change it to not
output anything, but I sort of feel that is backwards.  If a
Porcelain wants raw-diff for 1 (or more) parents, "diff-tree -r
-m" has been the way to do so before the ee6380 change, and that
output has not changed (well ee6380 might have changed it but
now it is fixed).

Re: gitweb using "--cc"?

[Junio C Hamano]

Marco Costalba <mcostalba@gmail.com> writes:

> Please _do not_ change this behaviour to make -m a no-op as stated in
> "diff-tree -c raw output" patch message
> (ee63802422af14e43eccce3c6dc4150a27ceb1a3).
>
> qgit has the possibility to switch from "see all merge files"
> to "see interesting only", so we really need that difference
> between 'git-diff-tree -r' and 'git-diff-tree -r -m'

Let me make sure I am not misreading you.  You are proposing to
revert making -m a no-op.  So '-r' and '-r -m' would do
different things, like illustrated in the log message below.

All of the above combinations of flags produces the same result
for non-merge commit, by the way.

Ack, or did I grossly misunderstand what you wanted?

-- >8 --
[PATCH] diff-tree: do not default to -c

Marco says it breaks qgit.  This makes the flags a bit more
orthogonal.

  $ git-diff-tree -r --abbrev ca18

    No output from this command because you asked to skip merge by
    not having -m there.

  $ git-diff-tree -r -m --abbrev ca18
  ca182053c7710a286d72102f4576cf32e0dafcfb
  :100644 100644 538d21d... 59042d1... M	Makefile
  :100644 100644 410b758... 6c47c3a... M	entry.c
  ca182053c7710a286d72102f4576cf32e0dafcfb
  :100644 100644 30479b4... 59042d1... M	Makefile

    The same "independent sets of diff" as before without -c.

  $ git-diff-tree -r -m -c --abbrev ca18
  ca182053c7710a286d72102f4576cf32e0dafcfb
  ::100644 100644 100644 538d21d... 30479b4... 59042d1... MM	Makefile

    Combined.

  $ git-diff-tree -r -c --abbrev ca18
  ca182053c7710a286d72102f4576cf32e0dafcfb
  ::100644 100644 100644 538d21d... 30479b4... 59042d1... MM	Makefile

    Asking for combined without -m does not make sense, so -c
    implies -m.

We need to supply -c as default to whatchanged, which is a
one-liner.

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

---
diff --git a/diff-tree.c b/diff-tree.c
index b170b03..f55a35a 100644
--- a/diff-tree.c
+++ b/diff-tree.c
@@ -6,7 +6,7 @@ static int show_root_diff = 0;
 static int no_commit_id = 0;
 static int verbose_header = 0;
 static int ignore_merges = 1;
-static int combine_merges = 1;
+static int combine_merges = 0;
 static int dense_combined_merges = 0;
 static int read_stdin = 0;
 static int always_show_header = 0;
@@ -248,7 +248,7 @@ int main(int argc, const char **argv)
 			continue;
 		}
 		if (!strcmp(arg, "-m")) {
-			combine_merges = ignore_merges = 0;
+			ignore_merges = 0;
 			continue;
 		}
 		if (!strcmp(arg, "-c")) {
diff --git a/git-whatchanged.sh b/git-whatchanged.sh
index 574fc35..1fb9feb 100755
--- a/git-whatchanged.sh
+++ b/git-whatchanged.sh
@@ -10,7 +10,7 @@ case "$0" in
 	count=
 	test -z "$diff_tree_flags" &&
 		diff_tree_flags=$(git-repo-config --get whatchanged.difftree)
-	diff_tree_default_flags='-M --abbrev' ;;
+	diff_tree_default_flags='-c -M --abbrev' ;;
 *show)
 	count=-n1
 	test -z "$diff_tree_flags" &&

Re: gitweb using "--cc"?

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> I found another nitpick: file removal doesn't seem to generate a good 
> diff in "git-diff-tree --cc" (but it's correct in the new "raw" format 
> diff).
>
> Here's a test-case, in case you care.

Actually, I've known about the removals and have excuse in one
of the commits why it does not show them.

It is an excuse (the internal data structure is not really
suited to show removal diff), but I think what the excuse gives
as the official reasoning behind it sort of make sense from
usability point of view as well.

Re: gitweb using "--cc"?

[Linus Torvalds]

On Thu, 9 Feb 2006, Junio C Hamano wrote:
> 
> Actually, I've known about the removals and have excuse in one
> of the commits why it does not show them.
> 
> It is an excuse (the internal data structure is not really
> suited to show removal diff), but I think what the excuse gives
> as the official reasoning behind it sort of make sense from
> usability point of view as well.

Fair enough. It looks a bit strange in gitk, but maybe that could be 
rectified by just making the new/deleted file case say so explcitly 
instead of having the "mode" line.

So instead of just "mode", how about saying "deleted file mode" or 
"new file mode" the way that the regular diffs do (but then not showing 
the contents for the deleted case).

		Linus

Re: gitweb using "--cc"?

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> So instead of just "mode", how about saying "deleted file mode" or 
> "new file mode" the way that the regular diffs do (but then not showing 
> the contents for the deleted case).

Sounds sensible.  Will do this evening after work.

[PATCH] combine-diff: Record diff status a bit more faithfully

[Junio C Hamano]

This shows "new file mode XXXX" and "deleted file mode XXXX"
lines like two-way diff-patch output does, by checking the
status from each parent.

The diff-raw output for combined diff is made a bit uglier by
showing diff status letters with each parent.  While most of the
case you would see "MM" in the output, an Evil Merge that
touches a path that was added by inheriting from one parent is
possible and it would be shown like these:

    $ git-diff-tree --abbrev -c HEAD
    2d7ca89675eb8888b0b88a91102f096d4471f09f
    ::000000 000000 100644 0000000... 0000000... 31dd686... AA	b
    ::000000 100644 100644 0000000... 6c884ae... c6d4fa8... AM	d
    ::100644 100644 100644 4f7cbe7... f8c295c... 19d5d80... RR	e

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

---

 * I also considered showing the rename detection scores but
   felt it was too much information, so refrained from it.
   Maybe MM might be too much, but knowing most of the merges
   are only two parents' kind, one extra column would not be too
   much noise.

   By looking at -c -p or --cc output, you cannot tell any
   renames, which makes me feel a bit uneasy.  I suspect this
   is going into purely academic realm and would not be useful
   in practice at all, so I'd say we should stop ;-).

 combine-diff.c |   32 +++++++++++++++++++++++++-------
 diff.h         |    1 +
 2 files changed, 26 insertions(+), 7 deletions(-)

dc33c79b0f69a1e9acee740a2f7ac5eacfdd49ce
diff --git a/combine-diff.c b/combine-diff.c
index 8ba6949..a38f01b 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -39,6 +39,7 @@ static struct combine_diff_path *interse
 			p->mode = q->queue[i]->two->mode;
 			memcpy(p->parent[n].sha1, q->queue[i]->one->sha1, 20);
 			p->parent[n].mode = q->queue[i]->one->mode;
+			p->parent[n].status = q->queue[i]->status;
 			*tail = p;
 			tail = &p->next;
 		}
@@ -62,6 +63,7 @@ static struct combine_diff_path *interse
 				memcpy(p->parent[n].sha1,
 				       q->queue[i]->one->sha1, 20);
 				p->parent[n].mode = q->queue[i]->one->mode;
+				p->parent[n].status = q->queue[i]->status;
 				break;
 			}
 		}
@@ -739,12 +741,25 @@ static int show_patch_diff(struct combin
 		printf("..%s\n", abb);
 
 		if (mode_differs) {
-			printf("mode ");
-			for (i = 0; i < num_parent; i++) {
-				printf("%s%06o", i ? "," : "",
-				       elem->parent[i].mode);
+			int added = !!elem->mode;
+			for (i = 0; added && i < num_parent; i++)
+				if (elem->parent[i].status !=
+				    DIFF_STATUS_ADDED)
+					added = 0;
+			if (added)
+				printf("new file mode %06o", elem->mode);
+			else {
+				if (!elem->mode)
+					printf("deleted file ");
+				printf("mode ");
+				for (i = 0; i < num_parent; i++) {
+					printf("%s%06o", i ? "," : "",
+					       elem->parent[i].mode);
+				}
+				if (elem->mode)
+					printf("..%06o", elem->mode);
 			}
-			printf("..%06o\n", elem->mode);
+			putchar('\n');
 		}
 		dump_sline(sline, cnt, num_parent);
 	}
@@ -811,8 +826,11 @@ static void show_raw_diff(struct combine
 	}
 
 	if (opt->output_format == DIFF_FORMAT_RAW ||
-	    opt->output_format == DIFF_FORMAT_NAME_STATUS)
-		printf("%c%c", mod_type, inter_name_termination);
+	    opt->output_format == DIFF_FORMAT_NAME_STATUS) {
+		for (i = 0; i < num_parent; i++)
+			putchar(p->parent[i].status);
+		putchar(inter_name_termination);
+	}
 
 	if (line_termination) {
 		if (quote_c_style(p->path, NULL, NULL, 0))
diff --git a/diff.h b/diff.h
index 946a406..8fac465 100644
--- a/diff.h
+++ b/diff.h
@@ -66,6 +66,7 @@ struct combine_diff_path {
 	unsigned int mode;
 	unsigned char sha1[20];
 	struct combine_diff_parent {
+		char status;
 		unsigned int mode;
 		unsigned char sha1[20];
 	} parent[FLEX_ARRAY];
-- 
1.1.6.g94c6

[PATCH] combine-diff: move formatting logic to show_combined_diff()

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> On Thu, 9 Feb 2006, Junio C Hamano wrote:
>>
>> I was wondering if we could teach not diff_tree_combined_merge
>> but show_combined_diff to do this, so that diff-files -c would
>> benefit from the raw output as wel.
>
> I wanted to do it that way, but it seemed less intrusive this way.
>
> I agree that it is the correct thing to do, though.

This comes on top of the earlier "use diffcore_std()" patch.

-- >8 --
This way, diff-files can make use of it.  Also implement the
full suite of what diff_flush_raw() supports just for
consistency.  With this, 'diff-tree -c -r --name-status' would
show what is expected.

There is no way to get the historical output (useful for
debugging and low-level Plumbing work) anymore, so tentatively
it makes '-m' to mean "do not combine and show individual diffs
with parents".

diff-files matches diff-tree to produce raw output for -c.  For
textual combined diff, use -p -c.

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

---

 combine-diff.c |   85 ++++++++++++++++++++++++++++++++++++++------------------
 diff-files.c   |    6 ++--
 diff-tree.c    |    2 +
 diff.h         |    3 +-
 4 files changed, 64 insertions(+), 32 deletions(-)

0a798076b8d1a4a31bf2b24c564e2a99fd1c43a1
diff --git a/combine-diff.c b/combine-diff.c
index 6d78305..9aa099b 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -618,8 +618,8 @@ static void reuse_combine_diff(struct sl
 	sline->p_lno[i] = sline->p_lno[j];
 }
 
-int show_combined_diff(struct combine_diff_path *elem, int num_parent,
-		       int dense, const char *header)
+static int show_patch_diff(struct combine_diff_path *elem, int num_parent,
+			   int dense, const char *header)
 {
 	unsigned long size, cnt, lno;
 	char *result, *cp, *ep;
@@ -791,32 +791,69 @@ static void show_raw_diff(struct combine
 
 	if (header)
 		puts(header);
-	offset = strlen(COLONS) - num_parent;
-	if (offset < 0)
-		offset = 0;
-	prefix = COLONS + offset;
 
-	/* Show the modes */
 	for (i = 0; i < num_parent; i++) {
-		int mode = p->parent[i].mode;
-		if (mode)
+		if (p->parent[i].mode)
 			mod_type = 'M';
-		printf("%s%06o", prefix, mode);
-		prefix = " ";
 	}
-	printf("%s%06o", prefix, p->mode);
 	if (!p->mode)
 		mod_type = 'D';
 
-	/* Show sha1's */
-	for (i = 0; i < num_parent; i++) {
-		printf("%s%s", prefix, diff_unique_abbrev(p->parent[i].sha1, opt->abbrev));
-		prefix = " ";
+	if (opt->output_format == DIFF_FORMAT_RAW) {
+		offset = strlen(COLONS) - num_parent;
+		if (offset < 0)
+			offset = 0;
+		prefix = COLONS + offset;
+
+		/* Show the modes */
+		for (i = 0; i < num_parent; i++) {
+			printf("%s%06o", prefix, p->parent[i].mode);
+			prefix = " ";
+		}
+		printf("%s%06o", prefix, p->mode);
+
+		/* Show sha1's */
+		for (i = 0; i < num_parent; i++)
+			printf(" %s", diff_unique_abbrev(p->parent[i].sha1,
+							 opt->abbrev));
+		printf(" %s ", diff_unique_abbrev(p->sha1, opt->abbrev));
+	}
+
+	if (opt->output_format == DIFF_FORMAT_RAW ||
+	    opt->output_format == DIFF_FORMAT_NAME_STATUS)
+		printf("%c%c", mod_type, inter_name_termination);
+
+	if (line_termination) {
+		if (quote_c_style(p->path, NULL, NULL, 0))
+			quote_c_style(p->path, NULL, stdout, 0);
+		else
+			printf("%s", p->path);
+		putchar(line_termination);
 	}
-	printf("%s%s", prefix, diff_unique_abbrev(p->sha1, opt->abbrev));
+	else {
+		printf("%s%c", p->path, line_termination);
+	}
+}
 
-	/* Modification type, terminations, filename */
-	printf(" %c%c%s%c", mod_type, inter_name_termination, p->path, line_termination);
+int show_combined_diff(struct combine_diff_path *p,
+		       int num_parent,
+		       int dense,
+		       const char *header,
+		       struct diff_options *opt)
+{
+	if (!p->len)
+		return 0;
+	switch (opt->output_format) {
+	case DIFF_FORMAT_RAW:
+	case DIFF_FORMAT_NAME_STATUS:
+	case DIFF_FORMAT_NAME:
+		show_raw_diff(p, num_parent, header, opt);
+		return 1;
+
+	default:
+	case DIFF_FORMAT_PATCH:
+		return show_patch_diff(p, num_parent, dense, header);
+	}
 }
 
 const char *diff_tree_combined_merge(const unsigned char *sha1,
@@ -858,14 +895,8 @@ const char *diff_tree_combined_merge(con
 	}
 	if (num_paths) {
 		for (p = paths; p; p = p->next) {
-			if (!p->len)
-				continue;
-			if (opt->output_format == DIFF_FORMAT_RAW) {
-				show_raw_diff(p, num_parent, header, opt);
-				header = NULL;
-				continue;
-			}
-			if (show_combined_diff(p, num_parent, dense, header))
+			if (show_combined_diff(p, num_parent, dense,
+					       header, opt))
 				header = NULL;
 		}
 	}
diff --git a/diff-files.c b/diff-files.c
index d24d11c..7db5ce6 100644
--- a/diff-files.c
+++ b/diff-files.c
@@ -88,9 +88,8 @@ int main(int argc, const char **argv)
 		}
 		argv++; argc--;
 	}
-	if (combine_merges) {
+	if (dense_combined_merges)
 		diff_options.output_format = DIFF_FORMAT_PATCH;
-	}
 
 	/* Find the directory, and set up the pathspec */
 	pathspec = get_pathspec(prefix, argv + 1);
@@ -166,7 +165,8 @@ int main(int argc, const char **argv)
 			if (combine_merges && num_compare_stages == 2) {
 				show_combined_diff(&combine.p, 2,
 						   dense_combined_merges,
-						   NULL);
+						   NULL,
+						   &diff_options);
 				free(combine.p.path);
 				continue;
 			}
diff --git a/diff-tree.c b/diff-tree.c
index df6fd97..b170b03 100644
--- a/diff-tree.c
+++ b/diff-tree.c
@@ -248,7 +248,7 @@ int main(int argc, const char **argv)
 			continue;
 		}
 		if (!strcmp(arg, "-m")) {
-			ignore_merges = 0;
+			combine_merges = ignore_merges = 0;
 			continue;
 		}
 		if (!strcmp(arg, "-c")) {
diff --git a/diff.h b/diff.h
index 9088519..946a406 100644
--- a/diff.h
+++ b/diff.h
@@ -75,7 +75,8 @@ struct combine_diff_path {
 	 sizeof(struct combine_diff_parent) * (n) + (l) + 1)
 
 extern int show_combined_diff(struct combine_diff_path *elem, int num_parent,
-			      int dense, const char *header);
+			      int dense, const char *header,
+			      struct diff_options *);
 
 extern const char *diff_tree_combined_merge(const unsigned char *sha1, const char *, int, struct diff_options *opt);
 
-- 
1.1.6.gce16

Re: gitweb using "--cc"?

[Kay Sievers]

On Wed, Feb 08, 2006 at 03:44:58PM -0800, Linus Torvalds wrote:
> 
> I just did an arm merge that needed some (very trivial) manual fixups 
> (commit ID cce0cac1, in case anybody cares).
> 
> As usual, git-diff-tree --cc does a beautiful job on it, but I also 
> checked the gitweb output, which seems to not do as well (the commit 
> message about a manual conflict merge doesn't make any sense at all).
> 
> Now, in this case, what gitweb shows is actually "sensible": it will show 
> the diff of what the merge "brought in" to the mainline kernel, and in 
> that sense I can certainly understand it. It basically diffs the merge 
> against the first parent.
> 
> So looking at that particular example, arguably gitweb does something 
> "different" from what the commit message is talking about, but in many 
> ways it's a perfectly logical thing.
> 
> However, diffing against the first parent, while it sometimes happens to 
> be a sane thing to do, really isn't very sane in general. The merge may go 
> the other way (subdevelopers merging my code), like in commit b2faf597, 
> and sometimes there might not be a single reference tree, but more of a 
> "couple of main branches" approach with merging back and forth). Then the 
> current gitweb behaviour makes no sense at all.
> 
> So it would be much nicer if gitweb had some alternate approach to showing 
> merge diffs. My suggested approach would be to just let the user choose: 
> have separate "diff against fist/second[/third[/..]] parent" buttons. And 
> one of the choices would be the "conflict view" that git-diff-tree --cc 
> gives (I'd argue for that being the default one, because it's the only one 
> that doesn't have a "preferred parent").

Hmm, I have no real clue what all the --cc is about. It's not obvious
for someone who never thought about "meta patches" or "complex merges". :)

If nobody else can do the changes to gitweb, sure, I'll do this and try
to understand what is needed, but then I will need it explained in more
details, what functionality we want to see here. At best with some commented
commandline examples that produce the data you want to see. So that I
can imagine what you are looking for and can give it a try ...

On the technical side for the kernel.org installation:
  does git diff use /usr/bin/diff?

  does git diff create temp files?

  how can i specify the location for the temp files?
  (wasn't possible some months ago, but needed on kernel.org)

  is the temp file naming safe for a lot of git diff running in parallel?

  is a --cc capable git already available on the kernel.org boxes?

Thanks,
Kay