[PATCH] Resurrect diff-tree-helper -R

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 <redacted>
---
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);
 	}

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

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

On Sat, 30 Apr 2005, Daniel Jacobowitz wrote:

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

Dear diary, on Sun, May 01, 2005 at 03:09:53AM CEST, I got a letter
where Linus Torvalds [off-list ref] told me that...

On Sat, 30 Apr 2005, Junio C Hamano wrote:

quoted

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