[PATCH] Resurrect diff-tree-helper -R

[PATCH] Resurrect diff-tree-helper -R

[Junio C Hamano]

Diff-tree-helper take two patch inadvertently dropped the
support of -R option, which is necessary to produce reverse diff
based on diff-cache and diff-files output (diff-tree does not
matter since you can feed two trees in reverse order).  This
patch restores it.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
diff-tree-helper.c |   17 +++++++++++------
1 files changed, 11 insertions(+), 6 deletions(-)

jit-diff 0 diff-tree-helper.c
# - Fix up d_type handling - we need to include <dirent.h> before
# + working-tree
--- k/diff-tree-helper.c  (mode:100644)
+++ l/diff-tree-helper.c  (mode:100644)
@@ -44,7 +44,8 @@ static int parse_oneside_change(const ch
 	return 0;
 }
 
-static int parse_diff_tree_output(const char *buf, const char **spec, int cnt)
+static int parse_diff_tree_output(const char *buf,
+				  const char **spec, int cnt, int reverse)
 {
 	struct diff_spec old, new;
 	char path[PATH_MAX];
@@ -98,8 +99,12 @@ static int parse_diff_tree_output(const 
 	default:
 		return -1;
 	}
-	if (!cnt || matches_pathspec(path, spec, cnt))
-		run_external_diff(path, &old, &new);
+	if (!cnt || matches_pathspec(path, spec, cnt)) {
+		if (reverse)
+			run_external_diff(path, &new, &old);
+		else
+			run_external_diff(path, &old, &new);
+	}
 	return 0;
 }
 
@@ -108,14 +113,14 @@ static const char *diff_tree_helper_usag
 
 int main(int ac, const char **av) {
 	struct strbuf sb;
-	int reverse_diff = 0;
+	int reverse = 0;
 	int line_termination = '\n';
 
 	strbuf_init(&sb);
 
 	while (1 < ac && av[1][0] == '-') {
 		if (av[1][1] == 'R')
-			reverse_diff = 1;
+			reverse = 1;
 		else if (av[1][1] == 'z')
 			line_termination = 0;
 		else
@@ -129,7 +134,7 @@ int main(int ac, const char **av) {
 		read_line(&sb, stdin, line_termination);
 		if (sb.eof)
 			break;
-		status = parse_diff_tree_output(sb.buf, av+1, ac-1);
+		status = parse_diff_tree_output(sb.buf, av+1, ac-1, reverse);
 		if (status)
 			fprintf(stderr, "cannot parse %s\n", sb.buf);
 	}

Re: [PATCH] Resurrect diff-tree-helper -R

[Linus Torvalds]

On Sat, 30 Apr 2005, Junio C Hamano wrote:
>
> Diff-tree-helper take two patch inadvertently dropped the
> support of -R option

Talking about the diffs, I'm beginning to hate those "mode" things.

Not only do they screw up diffstat (big deal), but they are pointless, 
since 99.9% of the time the mode stays the same.

So it would be much nicer (I think) if mode changes are handled 
separately, with a simple separate line before the diff saying

	"Mode change: %o->%o %s", oldmode, newmode, path

and not mess up the diff header. That way, you only see it when it
actually makes any difference, and it's more readable both for humans
_and_ machines as a result.

Normal "patch" will just ignore the extra lines before the diff anyway, so 
it won't matter there.

Comments?

		Linus

Re: [PATCH] Resurrect diff-tree-helper -R

[Daniel Jacobowitz]

On Sat, Apr 30, 2005 at 06:09:53PM -0700, Linus Torvalds wrote:
> So it would be much nicer (I think) if mode changes are handled 
> separately, with a simple separate line before the diff saying
> 
> 	"Mode change: %o->%o %s", oldmode, newmode, path
> 
> and not mess up the diff header. That way, you only see it when it
> actually makes any difference, and it's more readable both for humans
> _and_ machines as a result.
> 
> Normal "patch" will just ignore the extra lines before the diff anyway, so 
> it won't matter there.
> 
> Comments?

It sounds good - but could you efficiently collect them before any diff
output?  If you have something like this, it'll be easy to read:

Mode change: 644->755 foo.sh
Mode change: 644->755 bar.sh

--- ChangeLog
+++ ChangeLog
@@ -1,0 +1,1 @@
+New line
--- copyright
+++ copyright
@@ -1,0 +1,1 @@
+New line


But if you generate this then you might as well not generate the mode
lines at all, for all a human looking at the diff is going to notice
them:

--- ChangeLog
+++ ChangeLog
@@ -1,0 +1,1 @@
+New line
Mode change: 644->755 foo.sh
--- copyright
+++ copyright
@@ -1,0 +1,1 @@
+New line
Mode change: 644->755 bar.sh


The latter is how diff does its "Only in" messages.  I never see them
when I'm looking through a diff of any size; only via diffstat, where
they're clearly disambiguated.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] Resurrect diff-tree-helper -R

[Linus Torvalds]

On Sat, 30 Apr 2005, Daniel Jacobowitz wrote:
> 
> It sounds good - but could you efficiently collect them before any diff
> output?  If you have something like this, it'll be easy to read:
> 
> Mode change: 644->755 foo.sh
> Mode change: 644->755 bar.sh
> 
> --- ChangeLog
> +++ ChangeLog

That may sound like a good idea, but it's horrid.

You'd only have to gather them back later anyway, since you can only apply 
the mode change _after_ you've done the diff. Why? The diff may be the 
thing that creates the file in the first place. Sp you should consider the 
mode changes as part of the "stream", not as something separate from the 
stream.

So I'd really much rather see it more as an "Index" line, which gets
prepended as part of the patch for that file (of course, either patch or
modeline can be missing).

		Linus

Re: [PATCH] Resurrect diff-tree-helper -R

[Junio C Hamano]

>>>>> "LT" == Linus Torvalds <torvalds@osdl.org> writes:

LT> Talking about the diffs, I'm beginning to hate those "mode" things.

Likewise.

LT> Not only do they screw up diffstat (big deal), but they are pointless, 
LT> since 99.9% of the time the mode stays the same.

Pointless, yes.  mode is not what screwing up diffstat but
comparing against /dev/null is, so it is not a reason to hate
mode, and my fingers learned to say diffstat -p1 already so it
is not a big deal anymore.

LT> Normal "patch" will just ignore the extra lines before the
LT> diff anyway, so it won't matter there.

LT> Comments?

I am 100% in agreement with you here.  The only reason I added
it was to match what Pasky does so that his cg-patch can eat its
output.  To me, pleasing cg-patch is far lower priority than
pleasing l-k developers, so your veto counts.

My JIT tools do not use that mode thing in the patch.  I apply a
patch between two commits (or trees) to the work tree by doing
something like this:

    GIT_EXTERNAL_DIFF=jit-diff-extract \
    jit-diff "$@" | {
        cd "${GIT_PROJECT_TOP}"
        sh
    }

Here jit-diff-extract is the gem that creates a small shell
script that patches the file and runs "chmod +x" or "chmod -x"
when necessary, and does git-update-cache for added or removed
files.  Its output would look something like this:

    patch -p1 <<\EOF
    --- /dev/null
    +++ fs/ext9/Makefile
    @@ ....
    EOF
    chmod -x 'fs/ext9/Makefile'
    git-update-cache --add --remove -- 'fs/ext9/Makefile'

Maybe I can make the default diff output just like the above?
As you say, normal patch would not look at those shell script
part at all anyway.

Re: [PATCH] Resurrect diff-tree-helper -R

[Linus Torvalds]

On Sat, 30 Apr 2005, Junio C Hamano wrote:
>  Its output would look something like this:
> 
>     patch -p1 <<\EOF
>     --- /dev/null
>     +++ fs/ext9/Makefile
>     @@ ....
>     EOF
>     chmod -x 'fs/ext9/Makefile'
>     git-update-cache --add --remove -- 'fs/ext9/Makefile'
> 
> Maybe I can make the default diff output just like the above?
> As you say, normal patch would not look at those shell script
> part at all anyway.

I actually do end up looking at diffs, and I'd hate it. I'd much rather
have as little extra fluff as possible, and putting shell scipt fragments
in it definitely counts as distraction.

The fewer lines there are that don't usually tell a human anything, the 
better. Dense is good. 

		Linus

Re: [PATCH] Resurrect diff-tree-helper -R

[Junio C Hamano]

>>>>> "LT" == Linus Torvalds <torvalds@osdl.org> writes:

LT> I actually do end up looking at diffs, and I'd hate it. I'd much rather
LT> have as little extra fluff as possible, and putting shell scipt fragments
LT> in it definitely counts as distraction.

Well I was half joking when I suggested that and I am glad to
see that you have a good aesthetics ;-).  How about:

 - Stop attempting to be compatible with cg-patch, and drop
   (mode:XXXXXX) bits from the diff.

 - Do keep the /dev/null change for created and deleted case.

 - No "Index:" line, no "Mode change:" line, anywhere in the
   output.  Anything that wants the mode bits and sha1 hash can
   do things from GIT_EXTERNAL_DIFF mechanism.  Maybe document
   suggested usage mechanism better.

I'll whip something up along the above lines and submit it.

[PATCH] Add git-apply-patch-script.

[Junio C Hamano]

I said:

     - Stop attempting to be compatible with cg-patch, and drop
       (mode:XXXXXX) bits from the diff.

     - Do keep the /dev/null change for created and deleted case.

     - No "Index:" line, no "Mode change:" line, anywhere in the
       output.  Anything that wants the mode bits and sha1 hash can
       do things from GIT_EXTERNAL_DIFF mechanism.  Maybe document
       suggested usage better.

This adds an example script git-apply-patch-script, that can be
used as the GIT_EXTERNAL_DIFF to apply changes between two trees
directly on the current work tree, like this:

 GIT_EXTERNAL_DIFF=git-apply-patch-script git-diff-tree -p <tree> <tree>

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

Makefile               |    4 +--
git-apply-patch-script |   60 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 62 insertions(+), 2 deletions(-)

# - [PATCH] Rework built-in diff to make its output more dense.
# + [PATCH] Add git-apply-patch-script.
--- k/Makefile
+++ l/Makefile
@@ -12,8 +12,8 @@ CFLAGS=-g -O2 -Wall
 CC=gcc
 AR=ar
 
-SCRIPTS=git-merge-one-file-script git-prune-script git-pull-script \
-	git-tag-script
+SCRIPTS=git-apply-patch-script git-merge-one-file-script git-prune-script \
+	git-pull-script git-tag-script
 
 PROG=   git-update-cache git-diff-files git-init-db git-write-tree \
 	git-read-tree git-commit-tree git-cat-file git-fsck-cache \
Created: git-apply-patch-script (mode:100755)
--- /dev/null
+++ l/git-apply-patch-script
@@ -0,0 +1,60 @@
+#!/bin/sh
+# Copyright (C) 2005 Junio C Hamano
+#
+# Applying diff between two trees to the work tree can be
+# done with the following single command:
+#
+# GIT_EXTERNAL_DIFF=git-apply-patch-script git-diff-tree -p $tree1 $tree2
+#
+
+case "$#" in
+2)    exit 1 ;; # do not feed unmerged diff to me!
+esac
+name="$1" tmp1="$2" hex1="$3" mode1="$4" tmp2="$5" hex2="$6" mode2="$7"
+case "$mode1" in *7??) mode1=+x ;; *6??) mode1=-x ;; esac
+case "$mode2" in *7??) mode2=+x ;; *6??) mode2=-x ;; esac
+
+if test -f "$name.orig" || test -f "$name.rej"
+then
+    echo >&2 "Unresolved patch conflicts in the previous run found."
+    exit 1
+fi
+# This will say "patching ..." so we do not say anything outselves.
+
+diff -u -L "a/$name" -L "b/$name" "$tmp1" "$tmp2" | patch -p1
+test -f "$name.rej" || {
+    case "$mode1,$mode2" in
+    .,?x)
+	# newly created
+	case "$mode2" in
+	+x)
+	    echo >&2 "created $name with mode +x."
+	    chmod "$mode2" "$name"
+	    ;;
+	-)
+	    echo >&2 "created $name."
+	    ;;
+	esac
+	git-update-cache --add -- "$name"
+	;;
+    ?x,.)
+	# deleted
+	echo >&2 "deleted $name."
+	rm -f "$name"
+	git-update-cache --remove -- "$name"
+	;;
+    *)
+	# changed
+	case "$mode1,$mode2" in
+	"$mode2,$mode1") ;;
+	*)
+	    echo >&2 "changing mode from $mode1 to $mode2."
+	    chmod "$mode2" "$name"
+	    ;;
+	esac
+    esac
+    # This bit is debatable---the SCM may not want to keep
+    # cache in sync with the work tree (JIT does want to).
+    git-update-cache -- "$name"
+}
+exit 0

[PATCH] Rework built-in diff to make its output more dense.

[Junio C Hamano]

Linus says,

    The fewer lines there are that don't usually tell a human
    anything, the better. Dense is good.

This patch makes the default diff output more dense.  This
removes the previous misguided attempt to be cg-patch
compatible.

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

diff.c |   26 +++++++++++++++-----------
1 files changed, 15 insertions(+), 11 deletions(-)

# - [PATCH] Resurrect diff-tree-helper -R
# + [PATCH] Rework built-in diff to make its output more dense.
--- k/diff.c
+++ l/diff.c
@@ -82,35 +82,32 @@ static void builtin_diff(const char *nam
 			 struct diff_tempfile *temp)
 {
 	int i, next_at;
-	const char *diff_cmd = "diff -L'%s%s%s' -L'%s%s%s'";
+	const char *diff_cmd = "diff -L'%s%s' -L'%s%s'";
 	const char *diff_arg  = "'%s' '%s'";
 	const char *input_name_sq[2];
 	const char *path0[2];
 	const char *path1[2];
-	char mode[2][20];
 	const char *name_sq = sq_expand(name);
 	char *cmd;
 	
-	/* diff_cmd and diff_arg have 8 %s in total which makes
-	 * the sum of these strings 16 bytes larger than required.
+	/* diff_cmd and diff_arg have 6 %s in total which makes
+	 * the sum of these strings 12 bytes larger than required.
 	 * we use 2 spaces around diff-opts, and we need to count
-	 * terminating NUL, so we subtract 13 here.
+	 * terminating NUL, so we subtract 9 here.
 	 */
 	int cmd_size = (strlen(diff_cmd) + strlen(diff_opts) +
-			strlen(diff_arg) - 13);
+			strlen(diff_arg) - 9);
 	for (i = 0; i < 2; i++) {
 		input_name_sq[i] = sq_expand(temp[i].name);
 		if (!strcmp(temp[i].name, "/dev/null")) {
 			path0[i] = "/dev/null";
 			path1[i] = "";
-			mode[i][0] = 0;
 		} else {
 			path0[i] = i ? "l/" : "k/";
 			path1[i] = name_sq;
-			sprintf(mode[i], "  (mode:%s)", temp[i].mode);
 		}
 		cmd_size += (strlen(path0[i]) + strlen(path1[i]) +
-			     strlen(mode[i]) + strlen(input_name_sq[i]));
+			     strlen(input_name_sq[i]));
 	}
 
 	cmd = xmalloc(cmd_size);
@@ -118,13 +115,20 @@ static void builtin_diff(const char *nam
 	next_at = 0;
 	next_at += snprintf(cmd+next_at, cmd_size-next_at,
 			    diff_cmd,
-			    path0[0], path1[0], mode[0],
-			    path0[1], path1[1], mode[1]);
+			    path0[0], path1[0], path0[1], path1[1]);
 	next_at += snprintf(cmd+next_at, cmd_size-next_at,
 			    " %s ", diff_opts);
 	next_at += snprintf(cmd+next_at, cmd_size-next_at,
 			    diff_arg, input_name_sq[0], input_name_sq[1]);
 
+	if (!path1[0][0])
+		printf("Created: %s (mode:%s)\n", name, temp[1].mode);
+	else if (!path1[1][0])
+		printf("Deleted: %s\n", name);
+	else if (strcmp(temp[0].mode, temp[1].mode))
+		printf("Mode changed: %s (%s->%s)\n", name,
+		       temp[0].mode, temp[1].mode);
+	fflush(NULL);
 	execlp("/bin/sh","sh", "-c", cmd, NULL);
 }
 

Re: [PATCH] Resurrect diff-tree-helper -R

[Petr Baudis]

Dear diary, on Sun, May 01, 2005 at 03:09:53AM CEST, I got a letter
where Linus Torvalds <torvalds@osdl.org> told me that...
> 
> 
> On Sat, 30 Apr 2005, Junio C Hamano wrote:
> >
> > Diff-tree-helper take two patch inadvertently dropped the
> > support of -R option
> 
> Talking about the diffs, I'm beginning to hate those "mode" things.
> 
> Not only do they screw up diffstat (big deal), but they are pointless, 
> since 99.9% of the time the mode stays the same.
> 
> So it would be much nicer (I think) if mode changes are handled 
> separately, with a simple separate line before the diff saying
> 
> 	"Mode change: %o->%o %s", oldmode, newmode, path
> 
> and not mess up the diff header. That way, you only see it when it
> actually makes any difference, and it's more readable both for humans
> _and_ machines as a result.
> 
> Normal "patch" will just ignore the extra lines before the diff anyway, so 
> it won't matter there.
> 
> Comments?

Sorry for replying after so much time, it looks like I missed this and
got here only after checking what change removed the mode: bits...

I'd personally prefer something like

	@.Mode change:

that is, using a '@.' prefix for those. It seems to be unique enough and
'@' is one of the four magic characters prefixing diff lines. Just using
the plain string seems too volatile, and I need to grep all the
interesting bits out of the diff file. This is because patch can
otherwise complain "only garbage found in the patch" when processing the
diff, which confuses my users greatly.

What do you think?

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
C++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor

Re: [PATCH] Resurrect diff-tree-helper -R

[Junio C Hamano]

Have you checked what the current one does?

Re: [PATCH] Resurrect diff-tree-helper -R

[Junio C Hamano]

PB> Sorry for replying after so much time, it looks like I missed this and
PB> got here only after checking what change removed the mode: bits...

PB> What do you think?

FYI, here is a demonsrtation of what you have right now.
Temporarily slurp in the test framework patch you hate so much
to get t/test-lib.sh ;-), apply this to get t/t2000-diff.sh, cd
to t and say sh ./t2000-diff.sh

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
jit-diff 1: t/t2000-diff.sh
# - HEAD: Fix git-diff-files for symlinks.
# + (working tree)
Created: t/t2000-diff.sh (mode:100755)
--- /dev/null
+++ b/t/t2000-diff.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+#
+# Copyright (c) 2005 Junio C Hamano
+#
+
+test_description='Test built-in diff output engine.
+
+'
+. ./test-lib.sh
+
+echo >path0 'Line 1
+Line 2
+line 3'
+cat path0 >path1
+chmod +x path1
+git-update-cache --add path0 path1
+mv path0 path0-
+sed -e 's/line/Line/' <path0- >path0
+chmod +x path0
+rm -f path1
+git-diff-files -p >current
+cat >expected <<\EOF
+Mode changed: path0 (100644->100755)
+--- a/path0
++++ b/path0
+@@ -1,3 +1,3 @@
+ Line 1
+ Line 2
+-line 3
++Line 3
+Deleted: path1
+--- a/path1
++++ /dev/null
+@@ -1,3 +0,0 @@
+-Line 1
+-Line 2
+-line 3
+EOF
+
+test_expect_success 'cmp -s current expected'
+test_done

Re: [PATCH] Resurrect diff-tree-helper -R

[Petr Baudis]

Dear diary, on Sat, May 14, 2005 at 12:59:31AM CEST, I got a letter
where Junio C Hamano <junkio@cox.net> told me that...
> Created: t/t2000-diff.sh (mode:100755)
> +Mode changed: path0 (100644->100755)

Great, so it's even worse than before. :/

My issues:

* Highly inconsistent format. Created has the "old-style" mode stuff
while changed mode looks completely differently. If you look at the
regular diffs, do you notice any format differences between creating
a file and modifying it?

	Actually, I think it should just always look as

		@ Mode changed: A->B C

	where either A or B may be empty (for file creation/deletion).
	I can see no point in further Created/Deleted lines.

* Filename not last. That'd be much friendlier to scripting, then you
could just split the line by spaces and at a certain point slurp the
rest and say that that would be a filename.

* No special prefix. Even if you think I shouldn't grep (it makes no
sense to me to redo half of the stuff when I already have something
reusable done, for no apparent benefit), I feel quite uncomfortable with
picking up and interpreting random pieces of surrounding text which
aren't marked as special in any way.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
C++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor

Re: [PATCH] Resurrect diff-tree-helper -R

[Junio C Hamano]

>>>>> "PB" == Petr Baudis <pasky@ucw.cz> writes:

PB> Dear diary, on Sat, May 14, 2005 at 12:59:31AM CEST, I got a letter
PB> where Junio C Hamano <junkio@cox.net> told me that...
>> Created: t/t2000-diff.sh (mode:100755)
>> +Mode changed: path0 (100644->100755)

PB> Great, so it's even worse than before. :/

Depends on the definition of "before".  At the beginning, we did
not do anything special and always said l/foo k/foo even when
create/delete was involved.  Then we did a misguided attempt to
minimally be cg-diff compatible, which Linus complained that it
was too distracting for human consumption.  The current one is
something in between, a lot more human side.

Yes, it is off course worse than the minimally cg-diff
compatible one, from cg-patch'es point of view.

You have seen what the current "something in between" does.
What I think is that in order not to distract human (read:
Linus) who reads patches, they should not share the same special
characters like "@".  Which unfortunately completely contradicts
what you are attempting to do.  Another thing we did while you
were looking other way ;-) was that we say mode changed only
when things change, so in that sense it is "inconsistent" from
the scripting point of view.  These were all done to make the
output more readable by and less distracting for humans, per
request from Linus.

I do not think nobody uses that current textual "comment"
information in automated tools (I do not), so changing them
should not be a problem.  How about we do something like this:

  1. Invent an environment variable you can define.  Let's say
     GIT_DIFF_SHOW_MODES.  It could alternatively a flag you
     pass from git-diff-{files,cache,tree,tree-helper} to the
     internal diff engine but then you need to add the necessary
     command line parameter for all these commands.  I can be
     persuaded in either way.

  2. When it is defined, we are not interested in pleasing Linus
     by trying not to be distracting.  We are more interested in
     producing patch that is easily script processible.

  3. Keep the current behaviour for human comsumption when we
     are operating without the option we define in 1.

  4. Change the mode stuff when GIT_DIFF_SHOW_MODES is defined.
     It would produce one of the following for _all_ entries;

     @. (100644->100755) path/to/a/file/that/changed/mode
     @. (100644->120000) path/to/a/file/that/changed/to/symlink
     @. (100644->100644) path/to/a/file/with/no/mode/change
     @. (.->100644) path/to/a/new/file
     @. (100644->.) path/to/a/deleted/file

     I have to stress that these would come immediately before
     the patch for each file.  Not upfront, not grouped together
     at the beginning.

BTW, what do you think about renaming git-diff-tree-helper to
just git-diff-helper?  It used to be for grokking diff-tree's
output but now the family have the same raw output format it
does not make much sense to keep "tree" in its name.

Re: [PATCH] Resurrect diff-tree-helper -R

[Junio C Hamano]

Another possibility.  How about generating the following _only_
when mode changes (including create and delete), even for human
consumption?  There will be _no_ such line when mode or type
does not change.

# mode: 100644 100755 path/to/a/file/that/changed/mode
# mode: 100644 120000 path/to/a/file/that/changed/to/symlink
# mode: 100644 100644 path/to/a/file/with/no/mode/change
# mode: . 100644 path/to/a/new/file
# mode: 100644 . path/to/a/deleted/file

This is not "something like this", but a proposal for the exact
output format specification (I am going to code immediately).
Each token above is separated with exactly one ' ' (ASCII 0x20)
each, and such a line comes immediately before the patch for the
file.  Showing both mode bits is to prepare for the case you
would want to apply the patch in reverse.

This is for machine consumption and there is no need to force
them to parse out -> and (), so I dropped them.  And mode or
type change happens so rarely, it would be OK for human
consumption if we show these garbage (from human point of view)
only when things change.  Can you parse this, or do you always
want to have them even if nothing changes?

Let's see how this would look like to humans.

    # mode: 100644 100755 path0
    --- a/path0
    +++ b/path0
    @@ -1,3 +1,3 @@
     Line 1
     Line 2
    -line 3
    +Line 3
    # mode: 100644 . path1
    --- a/path1
    +++ /dev/null
    @@ -1,3 +0,0 @@
    -Line 1
    -Line 2
    -line 3
    --- a/path2
    +++ b/path2
    @@ -1,3 +1,3 @@
     Line 1
     Line 2
    -line 3
    +Line 3
    # mode: . 100755 t/t2000-diff.sh
    --- /dev/null
    +++ b/t/t2000-diff.sh
    @@ -0,0 +1,41 @@
    ...

Doesn't look too bad, does it?

Re: [PATCH] Resurrect diff-tree-helper -R

[Petr Baudis]

Dear diary, on Sat, May 14, 2005 at 02:33:11AM CEST, I got a letter
where Junio C Hamano <junkio@cox.net> told me that...
> Another possibility.  How about generating the following _only_
> when mode changes (including create and delete), even for human
> consumption?  There will be _no_ such line when mode or type
> does not change.
> 
> # mode: 100644 100755 path/to/a/file/that/changed/mode
> # mode: 100644 120000 path/to/a/file/that/changed/to/symlink
> # mode: 100644 100644 path/to/a/file/with/no/mode/change
> # mode: . 100644 path/to/a/new/file
> # mode: 100644 . path/to/a/deleted/file
> 
> This is not "something like this", but a proposal for the exact
> output format specification (I am going to code immediately).
> Each token above is separated with exactly one ' ' (ASCII 0x20)
> each, and such a line comes immediately before the patch for the
> file.  Showing both mode bits is to prepare for the case you
> would want to apply the patch in reverse.
> 
> This is for machine consumption and there is no need to force
> them to parse out -> and (), so I dropped them.  And mode or
> type change happens so rarely, it would be OK for human
> consumption if we show these garbage (from human point of view)
> only when things change.  Can you parse this, or do you always
> want to have them even if nothing changes?
> 
> Let's see how this would look like to humans.

For humans I'd say "Mode change" instead of "mode" would be better, and
for machines I still think "@" would be better than "#". "#" can occur
quite naturally in some code snippets or whatever pasted to the commit
message, which is extremely unlikely for "@". What are the advantages
of "#"?

I like the rest. That's basically what I've imagined, and without the
arrows it's even better. :-)

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
C++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor

Re: [PATCH] Resurrect diff-tree-helper -R

[Junio C Hamano]

>>>>> "PB" == Petr Baudis <pasky@ucw.cz> writes:

Now I understand which discussion I was missing ;-).

PB> For humans I'd say "Mode change" instead of "mode" would be better, and
PB> for machines I still think "@" would be better than "#". "#" can occur
PB> quite naturally in some code snippets or whatever pasted to the commit
PB> message, which is extremely unlikely for "@". What are the advantages
PB> of "#"?

Wait a minute.  Aren't we scanning starting from the first
'---\n'?  Why does what's in commit message matter?

And it is not really "Mode change" anymore.  If you used to have
file there and you replaced it with a symlink, that is 100644 to
120000 "mode change".  I experimented with different things in
where I have "# mode: " there and seriously considered to spell
it "# git:" instead, because that is not really mode and it is
something that means something special to git.  Also I tried to
say just "@. " --- it _was_ confusing to human eye, especially
if you are used to reading diffs.

What I think is that this should not really matter much for
human consumption, because mode change is rare and type change
is even more rare.

PB> I like the rest. That's basically what I've imagined, and
PB> without the arrows it's even better. :-)

Here is what I'd propose for you to do.  (1) Take the patch as
is and commit; (2) Change the definition of git_prefix in diff.c
to "\n@. " and commit; (3) If you already took the test suite,
match t/t2000-diff.sh for the "\n@. " format, and commit.  

It will look something like this. thanks to the leading newline,
the output becomes a bit less confusing (without that blank
line, it really is a disaster for human eyes).

    @. 100644 100755 path0
    --- a/path0
    +++ b/path0
    @@ -1,3 +1,3 @@
     Line 1
     Line 2
    -line 3
    +Line 3

    @. 100755 . path1
    --- a/path1
    +++ /dev/null
    @@ -1,3 +0,0 @@
    -Line 1
    -Line 2
    -line 3

Re: [PATCH] Resurrect diff-tree-helper -R

[Petr Baudis]

Dear diary, on Sat, May 14, 2005 at 06:27:27PM CEST, I got a letter
where Junio C Hamano <junkio@cox.net> told me that...
> >>>>> "PB" == Petr Baudis <pasky@ucw.cz> writes:
> 
> Now I understand which discussion I was missing ;-).
> 
> PB> For humans I'd say "Mode change" instead of "mode" would be better, and
> PB> for machines I still think "@" would be better than "#". "#" can occur
> PB> quite naturally in some code snippets or whatever pasted to the commit
> PB> message, which is extremely unlikely for "@". What are the advantages
> PB> of "#"?
> 
> Wait a minute.  Aren't we scanning starting from the first
> '---\n'?  Why does what's in commit message matter?

Ok, that changes the whole situation. I'll take your patches as they are
now in that case. :-)

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
C++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor

Re: [PATCH] Resurrect diff-tree-helper -R

[Junio C Hamano]

>>>>> "PB" == Petr Baudis <pasky@ucw.cz> writes:

>> Wait a minute.  Aren't we scanning starting from the first
>> '---\n'?  Why does what's in commit message matter?

PB> Ok, that changes the whole situation. I'll take your patches as they are
PB> now in that case. :-)

Shooooooooot.  Seriously.

I already am beginning to like "\n@. " very much; it is much
less distracting then the "# mode: " thing, especially with the
help from additional newline.

Could I have the following applied, pretty please?

------------
Tweak diff output a bit further to make a bit less distracting.

This adds a blank line before start of diffs for each file, and
also changes "# mode: " header to "@. ".  One justification is
that it tells more than just mode, and "@. " is visually a lot
less distracting.

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

--- a/diff.c
+++ b/diff.c
@@ -83,7 +83,7 @@ static void builtin_diff(const char *nam
 			 struct diff_tempfile *temp)
 {
 	int i, next_at;
-	const char *git_prefix = "# mode: ";
+	const char *git_prefix = "\n@. ";
 	const char *diff_cmd = "diff -L'%s%s' -L'%s%s'";
 	const char *diff_arg  = "'%s' '%s'||:"; /* "||:" is to return 0 */
 	const char *input_name_sq[2];
@@ -128,15 +128,17 @@ static void builtin_diff(const char *nam
 	else if (!path1[1][0])
 		printf("%s%s . %s\n", git_prefix, temp[0].mode, name);
 	else {
-		if (strcmp(temp[0].mode, temp[1].mode))
+		if (strcmp(temp[0].mode, temp[1].mode)) {
 			printf("%s%s %s %s\n", git_prefix,
 			       temp[0].mode, temp[1].mode, name);
-
-		if (strncmp(temp[0].mode, temp[1].mode, 3))
-			/* we do not run diff between different kind
-			 * of objects.
-			 */
-			exit(0);
+			if (strncmp(temp[0].mode, temp[1].mode, 3))
+				/* we do not run diff between different kind
+				 * of objects.
+				 */
+				exit(0);
+		}
+		else
+			putchar('\n');
 	}
 	fflush(NULL);
 	execlp("/bin/sh","sh", "-c", cmd, NULL);

Re: [PATCH] Resurrect diff-tree-helper -R

[Petr Baudis]

Dear diary, on Sun, May 15, 2005 at 08:25:46AM CEST, I got a letter
where Junio C Hamano <junkio@cox.net> told me that...
> >>>>> "PB" == Petr Baudis <pasky@ucw.cz> writes:
> 
> >> Wait a minute.  Aren't we scanning starting from the first
> >> '---\n'?  Why does what's in commit message matter?
> 
> PB> Ok, that changes the whole situation. I'll take your patches as they are
> PB> now in that case. :-)
> 
> Shooooooooot.  Seriously.
> 
> I already am beginning to like "\n@. " very much; it is much
> less distracting then the "# mode: " thing, especially with the
> help from additional newline.

I'd argue that it is too little distracting this way. But what I dislike
more is that the diff output is now visually inconsistent - some diffs
are separated by a newline and some aren't.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
C++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor

Re: [PATCH] Resurrect diff-tree-helper -R

[Junio C Hamano]

>>>>> "PB" == Petr Baudis <pasky@ucw.cz> writes:

PB> ... But what I dislike
PB> more is that the diff output is now visually inconsistent - some diffs
PB> are separated by a newline and some aren't.

That is already fixed in the second patch.

[PATCH] Tweak diff output further to make it a bit less distracting.

[Junio C Hamano]

Seriously...

Adds an newline between each diff.  Also change "#mode : "
string, which was misleading in that we are not showing just
mode when we talk about a file changing into a symlink.

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

diff.c                 |   18 ++++++++++--------
t/t4000-diff-format.sh |    6 ++++--
2 files changed, 14 insertions(+), 10 deletions(-)

--- a/diff.c
+++ b/diff.c
@@ -83,7 +83,7 @@
 			 struct diff_tempfile *temp)
 {
 	int i, next_at;
-	const char *git_prefix = "# mode: ";
+	const char *git_prefix = "\n@. ";
 	const char *diff_cmd = "diff -L'%s%s' -L'%s%s'";
 	const char *diff_arg  = "'%s' '%s'||:"; /* "||:" is to return 0 */
 	const char *input_name_sq[2];
@@ -128,15 +128,17 @@
 	else if (!path1[1][0])
 		printf("%s%s . %s\n", git_prefix, temp[0].mode, name);
 	else {
-		if (strcmp(temp[0].mode, temp[1].mode))
+		if (strcmp(temp[0].mode, temp[1].mode)) {
 			printf("%s%s %s %s\n", git_prefix,
 			       temp[0].mode, temp[1].mode, name);
-
-		if (strncmp(temp[0].mode, temp[1].mode, 3))
-			/* we do not run diff between different kind
-			 * of objects.
-			 */
-			exit(0);
+			if (strncmp(temp[0].mode, temp[1].mode, 3))
+				/* we do not run diff between different kind
+				 * of objects.
+				 */
+				exit(0);
+		}
+		else
+			putchar('\n');
 	}
 	fflush(NULL);
 	execlp("/bin/sh","sh", "-c", cmd, NULL);
--- a/t/t4000-diff-format.sh
+++ b/t/t4000-diff-format.sh
@@ -26,7 +26,8 @@
     'git-diff-files -p after editing work tree.' \
     'git-diff-files -p >current'
 cat >expected <<\EOF
-# mode: 100644 100755 path0
+
+@. 100644 100755 path0
 --- a/path0
 +++ b/path0
 @@ -1,3 +1,3 @@
@@ -34,7 +35,8 @@
  Line 2
 -line 3
 +Line 3
-# mode: 100755 . path1
+
+@. 100755 . path1
 --- a/path1
 +++ /dev/null
 @@ -1,3 +0,0 @@
------------------------------------------------

Re: [PATCH] Resurrect diff-tree-helper -R

[Petr Baudis]

Dear diary, on Sat, May 14, 2005 at 01:59:36AM CEST, I got a letter
where Junio C Hamano <junkio@cox.net> told me that...
> >>>>> "PB" == Petr Baudis <pasky@ucw.cz> writes:
> 
> PB> Dear diary, on Sat, May 14, 2005 at 12:59:31AM CEST, I got a letter
> PB> where Junio C Hamano <junkio@cox.net> told me that...
> >> Created: t/t2000-diff.sh (mode:100755)
> >> +Mode changed: path0 (100644->100755)
> 
> PB> Great, so it's even worse than before. :/
> 
> Depends on the definition of "before".  At the beginning, we did
> not do anything special and always said l/foo k/foo even when
> create/delete was involved.  Then we did a misguided attempt to
> minimally be cg-diff compatible, which Linus complained that it
> was too distracting for human consumption.  The current one is
> something in between, a lot more human side.

By "before" I meant the Linus proposal I was originally replying too.
It seems I'm still missing part of the history. :-)

> You have seen what the current "something in between" does.
> What I think is that in order not to distract human (read:
> Linus) who reads patches, they should not share the same special
> characters like "@".  Which unfortunately completely contradicts
> what you are attempting to do.

I don't think it discards humans, actually. I'd rather say it makes them
aware that this is something special. And if you show it only when the
mode changes, it will always be a special thing, not only something
which clutters the view.

So I'd say it's better for humans too, since it is clear for them that
this is not part of the commit message, and it carries special meaning
for the tool they will feed it to.

> Another thing we did while you were looking other way ;-) was that we
> say mode changed only when things change, so in that sense it is
> "inconsistent" from the scripting point of view.

I have no issue with that.

> I do not think nobody uses that current textual "comment"
> information in automated tools (I do not), so changing them
> should not be a problem.  How about we do something like this:
> 
>   1. Invent an environment variable you can define.  Let's say
>      GIT_DIFF_SHOW_MODES.  It could alternatively a flag you
>      pass from git-diff-{files,cache,tree,tree-helper} to the
>      internal diff engine but then you need to add the necessary
>      command line parameter for all these commands.  I can be
>      persuaded in either way.

I think this completely misses the point. You are viewing what I'm
suggesting as trying to just aid Cogito's internals using cg-diff |
cg-patch, but that's actually not my major reason for doing this at all.
I view that as a hack anyway and it should eventually do a three-way
merge too at those places.

What I'm trying to do is to figure out a good encapsulation for mode
changes which can be put in *all* the patches. So when you are sending
me some new testcases, I don't have to chmod them manually. That's the
main point of doing this. I could deal with mode changes completely
separately if it was only about Cogito's internal stuff.

> BTW, what do you think about renaming git-diff-tree-helper to
> just git-diff-helper?  It used to be for grokking diff-tree's
> output but now the family have the same raw output format it
> does not make much sense to keep "tree" in its name.

No issue with that.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
C++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor

Re: [PATCH] Resurrect diff-tree-helper -R

[Junio C Hamano]

>>>>> "PB" == Petr Baudis <pasky@ucw.cz> writes:

PB> .... Even if you think I shouldn't grep

I do not mean you should never grep.  There are cases when there
is no alternative.  One case I did not mention is that you would
want to accept patches via e-mail and apply.  In such a case,
GIT_EXTERNAL_DIFF with git-apply-patch-script way would not work
because that approach is essentially to patch while you are
generating diff locally.

Re: [PATCH] Resurrect diff-tree-helper -R

[Junio C Hamano]

>>>>> "PB" == Petr Baudis <pasky@ucw.cz> writes:

PB> that is, using a '@.' prefix for those. It seems to be unique enough and
PB> '@' is one of the four magic characters prefixing diff lines. Just using
PB> the plain string seems too volatile, and I need to grep all the
PB> interesting bits out of the diff file. This is because patch can
PB> otherwise complain "only garbage found in the patch" when processing the
PB> diff, which confuses my users greatly.

PB> What do you think?

Personally what I think is that grepping in the diff, especially
if the diff is something you are generating (I am assuming that
you are taling about cg-diff fed to cg-patch to port work tree
changes forward), is a wrong way to do things.

The way JIT does the equivalent is via git-apply-patch-script.
You run git-diff-{files,cache,tree}, setting GIT_EXTERNAL_DIFF
environment variable to git-apply-patch-script, and have the
apply-patch-script to take care of the mode changes, creation,
etc.  See the implementation of the jit-patch command if you are
interested.