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

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

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, Apr 30, 2005 at 06:09:53PM -0700, Linus Torvalds 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

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

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.

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

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

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.

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

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

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

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?

Have you checked what the current one does?

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

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

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.

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

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.

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?

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

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

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

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

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

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

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

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.

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.

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(-)

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

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

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.

I have no issue with that.

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.

No issue with that.

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.

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.