Combined diff format documentation

Combined diff format documentation

[Jakub Narebski]

In Documentation/diff-format.txt we can find the following information about
combined diff format:

 combined diff format
 --------------------
 
 git-diff-tree and git-diff-files can take '-c' or '--cc' option
 to produce 'combined diff', which looks like this: 

 ------------
 diff --combined describe.c
 @@@ +98,7 @@@
    return (a_date > b_date) ? -1 : (a_date == b_date) ? 0 : 1;
   }
 
 - static void describe(char *arg)
  -static void describe(struct commit *cmit, int last_one)
 ++static void describe(char *arg, int last_one)
   {
  +     unsigned char sha1[20];
  +     struct commit *cmit;
 ------------

And it further goes how to read combined diff format, and how --cc
(condensed combined) differs from --combined.

There is no note about which of extended headers functions with combined
diff format, how they change, how chunk header changes.

From what I gathered, there are the following differences as compared to
ordinary (diff --git) git extended headers:

1. "git diff" header which looked like this

      diff --git a/file1 b/file2

    is now

      diff --combined file2

    (where instead of --combined we might have --cc). Not described in
    documentation.

2. the "index" extended header line changes from

     index <hash>..<hash> <mode>

   to
   
     index <hash>,<hash>..<hash>

   Mode information is put in separate line, only if mode changes, for
   example

     mode <mode>,<mode>..<mode>

   <mode> can be 000000 if file didn't exist in particular parent; if file
   was cerated by merge we have

     new file mode <mode>

   I haven't checked what happens if file is deleted, either by branch or by
   merge commit itself. Not described in documentation, I'm not sure about
   how this (wrt modes) works.

3. The "rename/copy" headers seems to be never present; see below.

4. From file/to file header _seems_ to function exactly like in ordinary
   diff format, namely

     --- a/file1
     +++ b/file2

   But it seems to function rather like in ordinary "git diff" header,
   i.e. we have a/file1 instead of /dev/null even for files created by
   merge. I have not checked if and how rename detection work here.

5. Hunk header is also modified: in ordinary diff we have

     @@ <from range> <to range> @@

   where <from range> is -<start line>,<number of lines>, and <to range>
   is +<start line>,<number of lines>. In combined diff format it changes
   similarly to "index" extended header, namely

     @@@ <from range> <from range> <to range> @@@

   It might be not obvoious that we have (number of parents + 1) '@'
   characters in chunk header for combined dif format.

   BTW. it is not mentioned in documentation that git diff uses hunk section
   indicator, and what regexp/expression it uses (and is it configurable).
   Not described in documentation.

6. Documentation/diff-format.txt explains combined and condensed combined
   format quite well, although it doesn't tell us if we can have plusses and
   minuses together in one line...


=====================================================================

Combined diff format an renames detection
-----------------------------------------

We have the following situation:
$ git ls-tree -r --abbrev HEAD
100644 blob 1ce3f81     greetings/goodbye.txt
100644 blob 980a0d5     greetings/hello.txt
$ git ls-tree -r --abbrev HEAD^1
100644 blob 980a0d5     greetings/hello.txt
$ git ls-tree -r --abbrev HEAD^2
100644 blob 1ce3f81     data/goodbye.txt
100644 blob 980a0d5     data/hello.txt

Below there are following diffs: with first parent, merge (with all parents)
with renames detection, combined, combined with rename detection. Is it all
expected?

$ git diff-tree -p HEAD^1 HEAD
diff --git a/greetings/goodbye.txt b/greetings/goodbye.txt
new file mode 100644
index 0000000..1ce3f81
--- /dev/null
+++ b/greetings/goodbye.txt
@@ -0,0 +1 @@
+Goodbye World!

$ git diff-tree -p -M -m HEAD
d0fdd886e3b768678832c8d826bb8b70f2ef7b8e
diff --git a/greetings/goodbye.txt b/greetings/goodbye.txt
new file mode 100644
index 0000000..1ce3f81
--- /dev/null
+++ b/greetings/goodbye.txt
@@ -0,0 +1 @@
+Goodbye World!
d0fdd886e3b768678832c8d826bb8b70f2ef7b8e
diff --git a/data/goodbye.txt b/greetings/goodbye.txt
similarity index 100%
rename from data/goodbye.txt
rename to greetings/goodbye.txt
diff --git a/data/hello.txt b/greetings/hello.txt
similarity index 100%
rename from data/hello.txt
rename to greetings/hello.txt

$ git diff-tree -p -c HEAD
d0fdd886e3b768678832c8d826bb8b70f2ef7b8e
diff --combined greetings/goodbye.txt
index 0000000,0000000..1ce3f81
new file mode 100644
--- a/greetings/goodbye.txt
+++ b/greetings/goodbye.txt
@@@ -1,0 -1,0 +1,1 @@@
++Goodbye World!

$ git diff-tree -p -c -M HEAD
d0fdd886e3b768678832c8d826bb8b70f2ef7b8e
diff --combined greetings/goodbye.txt
index 0000000,1ce3f81..1ce3f81
mode 000000,100644..100644
--- a/greetings/goodbye.txt
+++ b/greetings/goodbye.txt
@@@ -1,0 -1,1 +1,1 @@@
+ Goodbye World!

And to compare, latest with --cc (condensed combined) instead of -c:
$ git diff-tree -p --cc -M HEAD
d0fdd886e3b768678832c8d826bb8b70f2ef7b8e
diff --cc greetings/goodbye.txt
index 0000000,1ce3f81..1ce3f81
mode 000000,100644..100644
--- a/greetings/goodbye.txt
+++ b/greetings/goodbye.txt
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: Combined diff format documentation

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> 1. "git diff" header which looked like this
> 2. the "index" extended header line changes from
> 3. The "rename/copy" headers seems to be never present; see below.
>...

Thanks for starting this.  Your observation is correct.  It was
pretty much designed for quick _content_ inspection and renames
would work correctly to pick which blobs from each tree to
compare but otherwise not reflected in the output (the pathnames
are not shown as far as I know).  We could probably add it if
some users need it.

> 5. Hunk header is also modified: in ordinary diff we have
> ...
>    It might be not obvoious that we have (number of parents + 1) '@'
>    characters in chunk header for combined dif format.

Correct.  This was done to prevent people from accidentally
feeding it to "patch -p1".  In other words, we wanted to make it
so obvious that it is _not_ a patch.

There may be more information in "git log -- combine-diff.c"
output that ought to be collected into the documentation, and
now might be a good time to do so, given that that part of the
system is fairly stable and has not changed for quite some time
in git timescale.

>    BTW. it is not mentioned in documentation that git diff uses hunk section
>    indicator, and what regexp/expression it uses (and is it configurable).
>    Not described in documentation.

If you mean by "hunk section indicator" the output similar to
GNU diff -p option, I think it is not worth mentioning and we
are not ready to mention it yet (we have not etched the
expression in stone).  Nobody jumped up and down to say it needs
to be configurable, so it is left undocumented more or less
deliberately.

> 6. Documentation/diff-format.txt explains combined and condensed combined
>    format quite well, although it doesn't tell us if we can have plusses and
>    minuses together in one line...

But you already know the answer to that question, since you
asked me a few days ago ;-).

Patches to documentation would be easier to comment on and more
productive, I guess.

> Below there are following diffs: with first parent, merge (with all parents)
> with renames detection, combined, combined with rename detection. Is it all
> expected?

Yes.  I do not see anything obviously unexpected in your output.

Re: Combined diff format documentation

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
>> 6. Documentation/diff-format.txt explains combined and condensed combined
>>    format quite well, although it doesn't tell us if we can have plusses and
>>    minuses together in one line...
> 
> But you already know the answer to that question, since you
> asked me a few days ago ;-).

Yes, in "[RFC] Syntax highlighting for combined diff" thread
http://permalink.gmane.org/gmane.comp.version-control.git/29566

Well, the _documentation_ doesn't tell. I haven't fully grokked the code
for generating and coloring combined diff output besides the fact that
I think it uses last indicator ('+' or '-') to chose color for the rest
of line. You said that even if the possibility exist, it is extreme
unlikely.

> Patches to documentation would be easier to comment on and more
> productive, I guess.

I was not sure about output. All conclusions about combined diff output
are from examples; I've planned to send patch to documentation when I'll
be sure that at least _most_ of what I've added is correct.

Will do.
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: Combined diff format documentation

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> I was not sure about output. All conclusions about combined diff output
> are from examples; I've planned to send patch to documentation when I'll
> be sure that at least _most_ of what I've added is correct.
>
> Will do.

Thanks.

Re: Combined diff format documentation

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Well, the _documentation_ doesn't tell. I haven't fully grokked the code
> for generating and coloring combined diff output besides the fact that
> I think it uses last indicator ('+' or '-') to chose color for the rest
> of line. You said that even if the possibility exist, it is extreme
> unlikely.

Well if I said that I must have been on booze ;-).

A '-' in the nth column means that the line is from the nth
parent and does _not_ appear in the merge result.  A '+' in the
nth column means that the line _appears_ in the merge result,
and the nth parent does not have that line (i.e. added by the
merge itself, or inherited from other parents).

Hence, by definition, you cannot have '-' and '+' on the same
line (otherwise the line has to exist and not exist in the merge
result at the same time).

A ' ' is a bit tricky to interpret.  A ' ' on a line _without_
any '-' means the line is the same as in that parent and the
merge result (i.e. the result inherited the line from that
parent).  A ' ' on a line that has '-' talks nothing about the
merge result (because by definition '-' lines do not exist in
the merge result) nor the parent that has ' '; in other words,
it is a "don't care" bit.  In the example you quoted from the
commit log of af3feefa:

         - static void describe(char *arg)
          -static void describe(struct commit *cmit, int last_one)
         ++static void describe(char *arg, int last_one)
           {

The first parent had it as one-arg function, and the second one
two-arg but the first parameter was of type "struct commit *";
the merge result has it as two-arg with the first parameter of
type "char *".  The second parent does not know about the
one-arg form of the function so it has ' ' in its column for the
first line.

All versions start the function with an opening brace '{' so the
line has two ' ' prefixed, which is an example of ' ' on a line
without any '-'.

Re: Combined diff format documentation

[Jakub Narebski]

Junio C Hamano wrote:

>>    BTW. it is not mentioned in documentation that git diff uses hunk section
>>    indicator, and what regexp/expression it uses (and is it configurable).
>>    Not described in documentation.
> 
> If you mean by "hunk section indicator" the output similar to
> GNU diff -p option, I think it is not worth mentioning and we
> are not ready to mention it yet (we have not etched the
> expression in stone).  Nobody jumped up and down to say it needs
> to be configurable, so it is left undocumented more or less
> deliberately.

By the way, I have just checked that combined diff format doesn't have
(for unknown reason) "which section" indicator in chunk header.
Compare
$ git diff-tree -p -m fec9ebf16c948bcb4a8b88d0173ee63584bcde76
and
$ git diff-tree -p -c fec9ebf16c948bcb4a8b88d0173ee63584bcde76
(this is the source of example combined diff format in diff-formats.txt
which I've found via
$ git rev-list --parents HEAD -- describe.c | grep " .* "
i.e. finding all merges which included changes to describe.c; there
are only two such commits).
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: Combined diff format documentation

[Horst H. von Brand]

Junio C Hamano <junkio@cox.net> wrote:
> Jakub Narebski <jnareb@gmail.com> writes:

[...]

> > 5. Hunk header is also modified: in ordinary diff we have
> > ...
> >    It might be not obvoious that we have (number of parents + 1) '@'
> >    characters in chunk header for combined dif format.

> Correct.  This was done to prevent people from accidentally
> feeding it to "patch -p1".  In other words, we wanted to make it
> so obvious that it is _not_ a patch.

It isn't, really... perhaps it should be made /more/ obvious (not use @ but
e.g. &, ...)?
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                    Fono: +56 32 2654431
Universidad Tecnica Federico Santa Maria             +56 32 2654239

Re: Combined diff format documentation

[Junio C Hamano]

"Horst H. von Brand" <vonbrand@inf.utfsm.cl> writes:

>> Correct.  This was done to prevent people from accidentally
>> feeding it to "patch -p1".  In other words, we wanted to make it
>> so obvious that it is _not_ a patch.
>
> It isn't, really... perhaps it should be made /more/ obvious (not use @ but
> e.g. &, ...)?

Eh, sorry, what I meant was "obvious to the tool", so "patch"
would take notice.

[PATCH] diff-format.txt: Combined diff format documentation supplement

[Jakub Narebski]

Update example combined diff format to the current version
$ git diff-tree -p -c fec9ebf16c948bcb4a8b88d0173ee63584bcde76
and provide complete first chunk in example.

Document combined diff format headers: how "diff header" look like,
which of "extended diff headers" are used with combined diff and how
they look like, differences in two-line from-file/to-file header from
non-combined diff format, chunk header format.

It should be noted that combined diff format was designed for quick
_content_ inspection and renames would work correctly to pick which
blobs from each tree to compare but otherwise not reflected in the
output (the pathnames are not shown).

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Junio C Hamano napisał:
> Patches to documentation would be easier to comment on and more
> productive, I guess.

So here you have. It should perhaps get review on validity by someone
well versed in the combined diff generation code. There are some guesses
here...

It compiles, but the output was not inspected.


 Documentation/diff-format.txt |   70 ++++++++++++++++++++++++++++++++++++++---
 1 files changed, 65 insertions(+), 5 deletions(-)

diff --git a/Documentation/diff-format.txt b/Documentation/diff-format.txt
index 617d8f5..0d04b03 100644
--- a/Documentation/diff-format.txt
+++ b/Documentation/diff-format.txt
@@ -156,18 +156,78 @@ to produce 'combined diff', which looks 
 
 ------------
 diff --combined describe.c
-@@@ +98,7 @@@
-   return (a_date > b_date) ? -1 : (a_date == b_date) ? 0 : 1;
+index fabadb8,cc95eb0..4866510
+--- a/describe.c
++++ b/describe.c
+@@@ -98,20 -98,12 +98,20 @@@
+  	return (a_date > b_date) ? -1 : (a_date == b_date) ? 0 : 1;
   }
-
+  
 - static void describe(char *arg)
  -static void describe(struct commit *cmit, int last_one)
 ++static void describe(char *arg, int last_one)
   {
- +     unsigned char sha1[20];
- +     struct commit *cmit;
+ +	unsigned char sha1[20];
+ +	struct commit *cmit;
+  	struct commit_list *list;
+  	static int initialized = 0;
+  	struct commit_name *n;
+  
+ +	if (get_sha1(arg, sha1) < 0)
+ +		usage(describe_usage);
+ +	cmit = lookup_commit_reference(sha1);
+ +	if (!cmit)
+ +		usage(describe_usage);
+ +
+  	if (!initialized) {
+  		initialized = 1;
+  		for_each_ref(get_name);
 ------------
 
+1.   It is preceded with a "git diff" header, that looks like
+     this (when '-c' option is used):
+
+       diff --combined fileM
+
+     or like this (when '--cc' option is used):
+
+       diff --c fileM
+
+2.   It is followed by one or more extended header lines
+     (we assume here that we have merge with two parents):
+
+       index <hash>,<hash>..<hash>
+       mode <mode>,<mode>..<mode>
+       new file mode <mode>
+
+     The "mode <mode>,<mode>..<mode>" appears only if at least
+     one of the <mode> is diferent from the rest. Extended headers
+     with information about detected contents movement (renames
+     and copying detection) are designed to work with diff of two
+     <tree-ish> and are not used by combined diff format. Currently
+     combined diff format cannot show files which were removed
+     by merge, so "deleted file mode <mode>,<mode>" is never used.
+
+3.   It is followed by two-line from-file/to-file header
+
+       --- a/fileM
+       +++ b/fileM
+
+     Contrary to two-line header for traditional 'unified' diff
+     format, and similar to filenames in ordinary "diff header",
+     /dev/null is not used for creation combined diff.
+
+4.   Chunk header format is modified to prevent people from
+     accidentally feeding it to 'patch -p1'. Combined diff format
+     was created for review of merge commit changes, and was not
+     meant for apply. The change is similar to the change in the
+     extended 'index' header
+
+       @@@ <from-file-range> <from-file-range> <to-file-range> @@@
+
+     It might be not obvious that we have number of parents + 1
+     '@' characters in chunk header for combined diff format.
+
 Unlike the traditional 'unified' diff format, which shows two
 files A and B with a single column that has `-` (minus --
 appears in A but removed in B), `+` (plus -- missing in A but
-- 
1.4.2.1



-- 
Jakub Narebski

Re: [PATCH] diff-format.txt: Combined diff format documentation supplement

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> So here you have. It should perhaps get review on validity by someone
> well versed in the combined diff generation code. There are some guesses
> here...

Thanks.

I guess review by the original author would be good enough;
this is entirely my code -- it was done while Linus and gang
was having fun in NZ, if I recall correctly ;-).

> It compiles, but the output was not inspected.

I've done minimal asciidoc mark-up fixes.  Troff man output look
horrible but that is not limited to this man page -- it looks
quite wrong whenever numbered list with displayed examples are
used.

Re: [PATCH] diff-format.txt: Combined diff format documentation supplement

[Junio C Hamano]

By the way, this is only minimally tested, but this patch fills
the blanks in your documentation.

The combine_diff_path structure _does_ keep track of the
original path in each parent, so renames _could_ be shown (we do
not keep the score, though), but the question is how.  "rename to"
would obviously be "the path in the merge result", but "rename from"
needs to be shown for all parents when we have rename from any
of the parents.  "rename from" line as in the usual one parent diff
does not have any place to say "which parent" (because there is
no need), so showing the usual "rename from" for only parents
that the result has rename from makes the output useless -- we
cannot tell "from which parent" from such an output.  I feel
that an evil merge is rare enough that worrying about showing
rename line is probably not worth the effort.

-- >8 --
[PATCH] combine-diff: a few more finishing touches.

"new file" and "deleted file" were already reported in the
original code, but the logic was not as transparent as it could
have.  This uses a few variables and more comments to clarify
the flow.  The rule is: (1) if a path exists in the merge result
when no parent had it, we report "new" (otherwise it came from
the parents, as opposed to have added by the evil merge). (2) if
the path does not exist in the merge result, it is "deleted".

Since we can say "new" and "deleted", there is no reason not to
follow the /dev/null convention.  This fixes it.

Appending function name after @@@ ... @@@ is trivial, so
implement it.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 combine-diff.c |   48 +++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index 46d9121..01a8437 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -489,6 +489,12 @@ static void show_parent_lno(struct sline
 	printf(" -%lu,%lu", l0, l1-l0);
 }
 
+static int hunk_comment_line(const char *bol)
+{
+	int ch = *bol & 0xff;
+	return (isalpha(ch) || ch == '_' || ch == '$');
+}
+
 static void dump_sline(struct sline *sline, unsigned long cnt, int num_parent,
 		       int use_color)
 {
@@ -508,8 +514,13 @@ static void dump_sline(struct sline *sli
 		struct sline *sl = &sline[lno];
 		unsigned long hunk_end;
 		unsigned long rlines;
-		while (lno <= cnt && !(sline[lno].flag & mark))
+		const char *hunk_comment = NULL;
+
+		while (lno <= cnt && !(sline[lno].flag & mark)) {
+			if (hunk_comment_line(sline[lno].bol))
+				hunk_comment = sline[lno].bol;
 			lno++;
+		}
 		if (cnt < lno)
 			break;
 		else {
@@ -526,6 +537,22 @@ static void dump_sline(struct sline *sli
 			show_parent_lno(sline, lno, hunk_end, i);
 		printf(" +%lu,%lu ", lno+1, rlines);
 		for (i = 0; i <= num_parent; i++) putchar(combine_marker);
+
+		if (hunk_comment) {
+			int comment_end = 0;
+			for (i = 0; i < 40; i++) {
+				int ch = hunk_comment[i] & 0xff;
+				if (!ch || ch == '\n')
+					break;
+				if (!isspace(ch))
+				    comment_end = i;
+			}
+			if (comment_end)
+				putchar(' ');
+			for (i = 0; i < comment_end; i++)
+				putchar(hunk_comment[i]);
+		}
+
 		printf("%s\n", c_reset);
 		while (lno < hunk_end) {
 			struct lline *ll;
@@ -707,6 +734,8 @@ static void show_patch_diff(struct combi
 		int use_color = opt->color_diff;
 		const char *c_meta = diff_get_color(use_color, DIFF_METAINFO);
 		const char *c_reset = diff_get_color(use_color, DIFF_RESET);
+		int added = 0;
+		int deleted = 0;
 
 		if (rev->loginfo)
 			show_log(rev, opt->msg_sep);
@@ -722,7 +751,10 @@ static void show_patch_diff(struct combi
 		printf("..%s%s\n", abb, c_reset);
 
 		if (mode_differs) {
-			int added = !!elem->mode;
+			deleted = !elem->mode;
+
+			/* We say it was added if nobody had it */
+			added = !deleted;
 			for (i = 0; added && i < num_parent; i++)
 				if (elem->parent[i].status !=
 				    DIFF_STATUS_ADDED)
@@ -731,7 +763,7 @@ static void show_patch_diff(struct combi
 				printf("%snew file mode %06o",
 				       c_meta, elem->mode);
 			else {
-				if (!elem->mode)
+				if (deleted)
 					printf("%sdeleted file ", c_meta);
 				printf("mode ");
 				for (i = 0; i < num_parent; i++) {
@@ -743,8 +775,14 @@ static void show_patch_diff(struct combi
 			}
 			printf("%s\n", c_reset);
 		}
-		dump_quoted_path("--- a/", elem->path, c_meta, c_reset);
-		dump_quoted_path("+++ b/", elem->path, c_meta, c_reset);
+		if (added)
+			dump_quoted_path("--- /dev/", "null", c_meta, c_reset);
+		else
+			dump_quoted_path("--- a/", elem->path, c_meta, c_reset);
+		if (deleted)
+			dump_quoted_path("+++ /dev/", "null", c_meta, c_reset);
+		else
+			dump_quoted_path("+++ b/", elem->path, c_meta, c_reset);
 		dump_sline(sline, cnt, num_parent, opt->color_diff);
 	}
 	free(result);

Re: [PATCH] diff-format.txt: Combined diff format documentation supplement

[Junio C Hamano]

This script minimally demonstrates a few interesting things an
evil merge can do.  Run it in a throw-away directory and view
the resulting merge with "git-show" with or without the patch I
sent out earlier.

One thing that you would notice is that the combined-diff code
chooses not to show the original contents of a deleted
file. while showing the whole result of a new file.  Strictly
speaking, this is inconsistent, but an evil merge is rare and
what ended up getting removed is not as interesting as what
remains as the result.

-- >8 --
#!/bin/sh

test -d .git && {
	echo Run me in an empty directory please
	exit 1
}

git init-db

echo one >file1.txt
git add file1.txt
git commit -m initial

git branch side

echo two >file2.txt
git add file2.txt
git commit -m second

git checkout side
echo uno >file1.txt
git commit -a -m side

git merge "Evil merge" HEAD master
rm -f file1.txt
echo added by the evil merge >file3.txt
echo modified by the evil merge >file2.txt
git update-index --add --remove file1.txt file2.txt file3.txt
EDITOR=: VISUAL=: git commit --amend