being nice to patch(1)

being nice to patch(1)

[Andrew Morton]

James's current git-scsi-misc has this commit in it:


commit a16efc1cbf0a9e5ea9f99ae98fb774b60d05c35b
Author: Kars de Jong <jongk@linux-m68k.org>
Date:   Sun Jun 17 14:47:08 2007 +0200

[SCSI] 53c700: Amiga 4000T NCR53c710 SCSI
    
    New driver for the Amiga 4000T built-in NCR53c710 SCSI controller, using the
    53c700 SCSI core.
    
    Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
    Signed-off-by: James Bottomley <James.Bottomley@SteelEye.com>


When one pulls that diff out of git with `git-show' or whatever, it doesn't
work - patch(1) has a heart attack over the "53c700":


|commit f98754960a9b25057ad5f249f877b3d6fab889ce
|Author: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
|Date:   Mon May 14 20:25:31 2007 +0900
|
|    [SCSI] hptiop: convert to use the data buffer accessors
|    
|    - remove the unnecessary map_single path.
|    
|    - convert to use the new accessors for the sg lists and the
|    parameters.
|    
|    Jens Axboe <jens.axboe@oracle.com> did the for_each_sg cleanup.
|    
|    Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
|    Acked-by: HighPoint Linux Team <linux@highpoint-tech.com>
|    Signed-off-by: James Bottomley <James.Bottomley@SteelEye.com>
|
|commit a16efc1cbf0a9e5ea9f99ae98fb774b60d05c35b
|Author: Kars de Jong <jongk@linux-m68k.org>
|Date:   Sun Jun 17 14:47:08 2007 +0200
|
|    [SCSI] 53c700: Amiga 4000T NCR53c710 SCSI
|    
|    New driver for the Amiga 4000T built-in NCR53c710 SCSI controller, using the
--------------------------
File to patch: 




This I assume is because ^[ ]*<number>c<number> is a magic marker for
contextual diffs.

So...  if someone is feeling really, really, really bored one day, it would
be nice to teach git to somehow escape such patch-magic-patterns in the
changelog when emitting plain old patches.

Re: being nice to patch(1)

[Linus Torvalds]

On Mon, 2 Jul 2007, Andrew Morton wrote:
> 
> James's current git-scsi-misc has this commit in it:
> 
> commit a16efc1cbf0a9e5ea9f99ae98fb774b60d05c35b
> Author: Kars de Jong <jongk@linux-m68k.org>
> Date:   Sun Jun 17 14:47:08 2007 +0200
> 
> [SCSI] 53c700: Amiga 4000T NCR53c710 SCSI
>     
>     New driver for the Amiga 4000T built-in NCR53c710 SCSI controller, using the
>     53c700 SCSI core.
>     
>     Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>     Signed-off-by: James Bottomley <James.Bottomley@SteelEye.com>
> 
> 
> When one pulls that diff out of git with `git-show' or whatever, it doesn't
> work - patch(1) has a heart attack over the "53c700":

There's really nothing git can do about this, this is a patch oddity about 
the free-form message. A really strange one too, because the line is 
literally four spaces followed by the 53c700, and the thing is, that's not 
even a valid olf-fashioned patch (_without_ the four spaces, I could see 
that "patch" might think that it's a really old ed-

I think you have two options:

 - tell patch to take it as a unified diff:

	git show | patch -p1 -u

   should work, since patch won't be trying to figure out what kind of 
   diff it is, and won't think that the 53c700 is some kind of odd ed 
   script.

 - suppress the free-form messages, by using (for example)

	git show --pretty=oneline | patch -p1

   and now "patch" doesn't get any random commit message except for the 
   first line (which always starts with the SHA1) and hopefully cannot 
   _possibly_ interpret that to be some strange patch format.

Or, of course, just use "git-apply" instead of patch to apply the thing.

			Linus

Re: being nice to patch(1)

[Andrew Morton]

On Mon, 2 Jul 2007 14:16:16 -0700 (PDT)
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> 
> On Mon, 2 Jul 2007, Andrew Morton wrote:
> > 
> > James's current git-scsi-misc has this commit in it:
> > 
> > commit a16efc1cbf0a9e5ea9f99ae98fb774b60d05c35b
> > Author: Kars de Jong <jongk@linux-m68k.org>
> > Date:   Sun Jun 17 14:47:08 2007 +0200
> > 
> > [SCSI] 53c700: Amiga 4000T NCR53c710 SCSI
> >     
> >     New driver for the Amiga 4000T built-in NCR53c710 SCSI controller, using the
> >     53c700 SCSI core.
> >     
> >     Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> >     Signed-off-by: James Bottomley <James.Bottomley@SteelEye.com>
> > 
> > 
> > When one pulls that diff out of git with `git-show' or whatever, it doesn't
> > work - patch(1) has a heart attack over the "53c700":
> 
> There's really nothing git can do about this, this is a patch oddity about 
> the free-form message. A really strange one too, because the line is 
> literally four spaces followed by the 53c700, and the thing is, that's not 
> even a valid olf-fashioned patch (_without_ the four spaces, I could see 
> that "patch" might think that it's a really old ed-
> 
> I think you have two options:
> 
>  - tell patch to take it as a unified diff:
> 
> 	git show | patch -p1 -u
> 
>    should work, since patch won't be trying to figure out what kind of 
>    diff it is, and won't think that the 53c700 is some kind of odd ed 
>    script.

yup, `patch -u' fixes it up.

>  - suppress the free-form messages, by using (for example)
> 
> 	git show --pretty=oneline | patch -p1
> 
>    and now "patch" doesn't get any random commit message except for the 
>    first line (which always starts with the SHA1) and hopefully cannot 
>    _possibly_ interpret that to be some strange patch format.
> 
> Or, of course, just use "git-apply" instead of patch to apply the thing.
> 

Thing is, changelog-followed-by-diff is a fairly standard format used by
quilt and other such toys.

Hopefully quilt is using -u so it won't encounter this oddity.

Re: being nice to patch(1)

[Linus Torvalds]

On Mon, 2 Jul 2007, Andrew Morton wrote:
> 
> Thing is, changelog-followed-by-diff is a fairly standard format used by
> quilt and other such toys.

Sure. And if a tool ends up eating the changelog as a diff, then that tool 
is broken. I really do think that this is a "patch" bug - I really don't 
think that that was a valid traditional diff with the four spaces at the 
head of the line.

Of course, if the changelog-followed-by-diff doesn't have any indentation 
or escaping at all, the changelog entry itself *could* actually have a 
real unified diff in it, and the tool would be unable to tell where the 
actual patch starts.

But at least "git show" and friends indent the changelog on purpose, 
exactly so that there is never any chance that there could be any real 
ambiguity, and this really was a "patch" bug as far as I can tell. 
Happily, one that is easy to work around, by just telling patch to always 
consider the patch a unified diff.

			Linus

Re: being nice to patch(1)

[Andrew Morton]

On Mon, 2 Jul 2007 14:40:36 -0700 (PDT)
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> 
> On Mon, 2 Jul 2007, Andrew Morton wrote:
> > 
> > Thing is, changelog-followed-by-diff is a fairly standard format used by
> > quilt and other such toys.
> 
> Sure. And if a tool ends up eating the changelog as a diff, then that tool 
> is broken. I really do think that this is a "patch" bug - I really don't 
> think that that was a valid traditional diff with the four spaces at the 
> head of the line.
> 
> Of course, if the changelog-followed-by-diff doesn't have any indentation 
> or escaping at all, the changelog entry itself *could* actually have a 
> real unified diff in it, and the tool would be unable to tell where the 
> actual patch starts.

erk, yes, sometimes people do like to quote a hunk of diff in the changelog
and yes, hell doth break loose.

> But at least "git show" and friends indent the changelog on purpose, 
> exactly so that there is never any chance that there could be any real 
> ambiguity, and this really was a "patch" bug as far as I can tell. 
> Happily, one that is easy to work around, by just telling patch to always 
> consider the patch a unified diff.

I'm afraid indenting the changelog with leading spaces doesn't help -
patch(1) still tries to apply the diff.

I guess quilt-and-friends could (should) strip away all text prior to the
first ^--- before feeding to patch(1).  That would reliably remove all
git changelog text.

Re: being nice to patch(1)

[Linus Torvalds]

On Mon, 2 Jul 2007, Andrew Morton wrote:
> 
> I'm afraid indenting the changelog with leading spaces doesn't help -
> patch(1) still tries to apply the diff.

Oh wow. I didn't believe you, so I decided to test.

I shouldn't have doubted you.

That also explains why it reacted to that 53c700 even though it wasn't at 
the beginning of a line.

That really is a piece of crap.

People who think that basic programs like "patch" should DWIM stuff like 
that are incompetent. Yes, I can see how it can be "convenient", but 
dammit, whoever added that convenince feature really is a total moron.

At the very least it should be off by default, and controlled by some flag 
(ie "patch --dwim"). As it is, it's on by default, and I don't see any way 
at all to disable it (not in the man-page, and not googling the source 
with google code-search).

That's just incredibly broken.

I guess I shouldn't be surprised. The whole "things should be convenient, 
not safe" approach is shown by the default high fuzz-factor too. But at 
least that one you can disable.

It's positively microsoftian to make programs blindly be "convenient", 
with no thinking about what that means for security and safety of the end 
result.

So I would suggest that in quilt and other systems, you either:

 - strip all headers manually

 - forget about "patch", and use "git-apply" instead that does things 
   right and doesn't screw up like this (and can do rename diffs etc too).

I guess the second choice generally isn't an option, but dammit, 
"git-apply" really is the better program here.

		Linus

Re: being nice to patch(1)

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> So I would suggest that in quilt and other systems, you either:
>
>  - strip all headers manually
>
>  - forget about "patch", and use "git-apply" instead that does things 
>    right and doesn't screw up like this (and can do rename diffs etc too).
>
> I guess the second choice generally isn't an option, but dammit, 
> "git-apply" really is the better program here.

Why not?  git-apply works outside of a git repo ;-)

Re: being nice to patch(1)

[Linus Torvalds]

On Mon, 2 Jul 2007, Junio C Hamano wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > So I would suggest that in quilt and other systems, you either:
> >
> >  - strip all headers manually
> >
> >  - forget about "patch", and use "git-apply" instead that does things 
> >    right and doesn't screw up like this (and can do rename diffs etc too).
> >
> > I guess the second choice generally isn't an option, but dammit, 
> > "git-apply" really is the better program here.
> 
> Why not?  git-apply works outside of a git repo ;-)

I was more thinking that people are not necessarily willing to install git 
just to get the "git-apply" program..

		Linus

Re: being nice to patch(1)

[Johannes Schindelin]

Hi,

On Mon, 2 Jul 2007, Linus Torvalds wrote:

> On Mon, 2 Jul 2007, Junio C Hamano wrote:
> > Linus Torvalds <torvalds@linux-foundation.org> writes:
> > 
> > > So I would suggest that in quilt and other systems, you either:
> > >
> > >  - strip all headers manually
> > >
> > >  - forget about "patch", and use "git-apply" instead that does things 
> > >    right and doesn't screw up like this (and can do rename diffs etc too).
> > >
> > > I guess the second choice generally isn't an option, but dammit, 
> > > "git-apply" really is the better program here.
> > 
> > Why not?  git-apply works outside of a git repo ;-)
> 
> I was more thinking that people are not necessarily willing to install git 
> just to get the "git-apply" program..

But maybe they would be willing to install git to get that wonderful 
git-apply program, and that wonderful rename-and-mode-aware git-diff, and 
the git-merge-file program, all of which can operate outside of a git 
repository. (Take that, hg!)

Ciao,
Dscho

Re: being nice to patch(1)

[Paolo Ciarrocchi]

On 7/3/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi,
> On Mon, 2 Jul 2007, Linus Torvalds wrote:
> > On Mon, 2 Jul 2007, Junio C Hamano wrote:
> > > Linus Torvalds <torvalds@linux-foundation.org> writes:
> > >
> > > > So I would suggest that in quilt and other systems, you either:
> > > >
> > > >  - strip all headers manually
> > > >
> > > >  - forget about "patch", and use "git-apply" instead that does things
> > > >    right and doesn't screw up like this (and can do rename diffs etc too).
> > > >
> > > > I guess the second choice generally isn't an option, but dammit,
> > > > "git-apply" really is the better program here.
> > >
> > > Why not?  git-apply works outside of a git repo ;-)
> >
> > I was more thinking that people are not necessarily willing to install git
> > just to get the "git-apply" program..
>
> But maybe they would be willing to install git to get that wonderful
> git-apply program, and that wonderful rename-and-mode-aware git-diff, and
> the git-merge-file program, all of which can operate outside of a git
> repository. (Take that, hg!)

How about shipping just these commands as a separate package?
Is that a cray idea?

ciao,
-- 
Paolo
"Tutto cio' che merita di essere fatto,merita di essere fatto bene"
Philip Stanhope IV conte di Chesterfield

Re: being nice to patch(1)

[Johannes Schindelin]

Hi,

On Tue, 3 Jul 2007, Paolo Ciarrocchi wrote:

> On 7/3/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > Hi,
> > On Mon, 2 Jul 2007, Linus Torvalds wrote:
> > > On Mon, 2 Jul 2007, Junio C Hamano wrote:
> > > > Linus Torvalds <torvalds@linux-foundation.org> writes:
> > > >
> > > > > So I would suggest that in quilt and other systems, you either:
> > > > >
> > > > >  - strip all headers manually
> > > > >
> > > > >  - forget about "patch", and use "git-apply" instead that does things
> > > > >    right and doesn't screw up like this (and can do rename diffs etc
> > too).
> > > > >
> > > > > I guess the second choice generally isn't an option, but dammit,
> > > > > "git-apply" really is the better program here.
> > > >
> > > > Why not?  git-apply works outside of a git repo ;-)
> > >
> > > I was more thinking that people are not necessarily willing to install
> > git
> > > just to get the "git-apply" program..
> > 
> > But maybe they would be willing to install git to get that wonderful
> > git-apply program, and that wonderful rename-and-mode-aware git-diff, and
> > the git-merge-file program, all of which can operate outside of a git
> > repository. (Take that, hg!)
> 
> How about shipping just these commands as a separate package?
> Is that a cray idea?

Heh, all three programs are "builtins", which means that you get almost 
the whole package of git anyway ;-)

Ciao,
Dscho

Re: being nice to patch(1)

[Theodore Tso]

On Tue, Jul 03, 2007 at 02:21:51PM +0200, Paolo Ciarrocchi wrote:
> >But maybe they would be willing to install git to get that wonderful
> >git-apply program, and that wonderful rename-and-mode-aware git-diff, and
> >the git-merge-file program, all of which can operate outside of a git
> >repository. (Take that, hg!)
> 
> How about shipping just these commands as a separate package?
> Is that a cray idea?

Or people could submit a bug report/feature request/patch to the
patch(1) maintainer.  :-)

						- Ted

Re: being nice to patch(1)

[Linus Torvalds]

[ Paul Eggert added to Cc: I'm not sure he actually maintains "patch" 
  or cares any more, but hopefully he at least knows who does ]

On Tue, 3 Jul 2007, Theodore Tso wrote:
> 
> Or people could submit a bug report/feature request/patch to the
> patch(1) maintainer.  :-)

Is there such a thing?

The latest official version of patch from GNU is 2.5.4 from 1999, I think.

I'm finding references to 2.5.9 in distributions (from 2003), but 2.5.4 is 
the latest I see on the GNU mirror at kernel.org, and that's also what 
Fedora 7 has too, it seems, so the 2.5.9 thing seems to be something 
unofficial or at least not widely known about..

Anyway, I tried to look at the patch sources, but I had to stop. That 
whole "intuit_diff_type()" function is probably designed as an initiation 
rite for any patch programmers, and to make sure that you have to be 
really serious about wanting to send patches before you can become part of 
the "in crowd". It's "mental hazing".

Yeah, git-apply sources aren't necessarily a thing of great beauty either, 
but in comparison to patch, I think it's a work of art. Of course, part of 
it is that it doesn't try to parse 'ed' scripts etc, but a large part of 
it really is that "patch" is an old program that has grown over time, and 
not seen a lot of cleanups, I suspect.

IOW, I tried to see how easy it would be to dismiss the code that 
takes care of "indent", but it wasn't totally obvious. It's set in many 
different places, and the logic for "skip_this_patch" is a bit confusing.

Anyway, with Paul Eggert Cc'd, maybe he can help us sort it out.

Paul - the issue here isn't actually with git at all, but the fact that 
Andrew Morton noticed that he cannot apply one of the series of patches he 
has with "patch" (well, with his scripts that are designed _around_ 
patch, to be exact).

The reason? Part of the patch *description* looked like this:

    [SCSI] 53c700: Amiga 4000T NCR53c710 SCSI

    New driver for the Amiga 4000T built-in NCR53c710 SCSI controller, using the
    53c700 SCSI core.

where it really *was* indented by four characters (and that's where git 
comes in: git indents the patch descriptions exactly so that you cannot 
*possibly* confuse the patch itself with the description).

It turns out that "patch" would actually think there is a patch there: the 
line

    53c700 SCSI core.

was determined to be an ed-script ("53c700") _despite_ the fact that it's 
indented.

Andrew was able to fix that particular damage by using "-u" and forcing 
anything but unified diffs to be ignored, but that isn't an option for all 
quilt users, since some projects use old-fashioned context diffs or a 
mixture.

Besides, the explanations can certainly contain patch fragments anyway (in 
the kernel, we put things like example code in them).

And it really boils down to a really simple thing: when scripting, you DO 
NOT WANT "patch" to make random guesses. And that whole "indentation" 
thing by patch is a pure guess, and should simply NOT BE DONE. And there's 
no way to tell patch to not do it.

So Paul, you're our only hope.

I'm personally trying to tell people not to use "patch" at all (this isn't 
the first time patch has done insane things by default, but it's the first 
time you cannot even _disable_ the insane behaviour), but Ted has a point: 
regardless of whether people learn to use "git-apply" to apply patches, 
the old "patch" binary would be better off just improved.

In this case, the improvement would be to simply ignore indented patches 
(preferably by default, but at least have the option to do so).

		Linus

Re: being nice to patch(1)

[Paul Eggert]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> Anyway, I tried to look at the patch sources, but I had to stop. That 
> whole "intuit_diff_type()" function is probably designed as an initiation 
> rite for any patch programmers, and to make sure that you have to be 
> really serious about wanting to send patches before you can become part of 
> the "in crowd". It's "mental hazing".

You should have seen it in the good old days when Larry Wall wrote it.
It was at least -- at least! -- 10% worse.

> In this case, the improvement would be to simply ignore indented patches 
> (preferably by default, but at least have the option to do so).

I agree.  POSIX has tied our hands to some extent, though, since it
_requires_ patch to accept indented patches by default.  It's too late
to fix this in the current POSIX go-round, but we can fix it in the
next.  And in the mean time we can add an option, I suppose defaulting
to not stripping indentation unless POSIXLY_CORRECT is set.  That
would be fine with me.

I'll add it to my list of things to do.

Re: being nice to patch(1)

[Linus Torvalds]

On Tue, 3 Jul 2007, Paul Eggert wrote:
>
> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > Anyway, I tried to look at the patch sources, but I had to stop. That 
> > whole "intuit_diff_type()" function is probably designed as an initiation 
> > rite for any patch programmers, and to make sure that you have to be 
> > really serious about wanting to send patches before you can become part of 
> > the "in crowd". It's "mental hazing".
> 
> You should have seen it in the good old days when Larry Wall wrote it.
> It was at least -- at least! -- 10% worse.

Heh.

Anyway, I figured out a sane way to do this - I had been thinking about it 
all wrong. Instead of worrying about all the places that change (and look 
at) "p_indent" - which is the real variable - I just made it not calculate 
"indent" in the first place.

That makes it catch it all in one place, and then quite naturally ignore 
any indented hunks because it won't recognize them as patches any more. At 
least I _think_ so, from just reading the source code.

So a patch like the following may or may not work. It compiles, but quite 
frankly, while it makes sense and worked for the single test-case I 
bothered with, somebody should double-check the logic.

> > In this case, the improvement would be to simply ignore indented patches 
> > (preferably by default, but at least have the option to do so).
> 
> I agree.  POSIX has tied our hands to some extent, though, since it
> _requires_ patch to accept indented patches by default.  It's too late
> to fix this in the current POSIX go-round, but we can fix it in the
> next.  And in the mean time we can add an option, I suppose defaulting
> to not stripping indentation unless POSIXLY_CORRECT is set.  That
> would be fine with me.
> 
> I'll add it to my list of things to do.

Ok, so this doesn't do the POSIXLY_CORRECT thing, and you may not agree 
with the flag name either ("--strip-indent" is a lot of characters to 
write, but I thought it was so esoteric that I think it's ok. I never even 
realized "patch" would ever do something as strange as that, and I'm 
hoping a lot of other people didn't realize either, so that changing this 
isn't going to matter, and very few people would hopefully ever need to 
use the "--strip-indent" flag).

But maybe you can use this patch as a starting point, at least.

(And if you wonder why I put the "if (!strip_indentation)" thing inside 
the loop, even though it doesn't ever change in the loop: it generated a 
smaller patch, and I didn't want to re-indent that code and make it even 
worse. So the logic is kind of stupid, but whatever..)

		Linus

---
diff --git a/common.h b/common.h
index c7fc5c2..45fecdc 100644
--- a/common.h
+++ b/common.h
@@ -174,6 +174,7 @@ XTERN bool canonicalize;
 XTERN int patch_get;
 XTERN bool set_time;
 XTERN bool set_utc;
+XTERN bool strip_indentation;
 
 enum diff
   {
diff --git a/patch.c b/patch.c
index 9e04daf..6e25d2a 100644
--- a/patch.c
+++ b/patch.c
@@ -522,6 +522,7 @@ static struct option const longopts[] =
   {"no-backup-if-mismatch", no_argument, NULL, CHAR_MAX + 6},
   {"posix", no_argument, NULL, CHAR_MAX + 7},
   {"quoting-style", required_argument, NULL, CHAR_MAX + 8},
+  {"strip-indent", no_argument, NULL, CHAR_MAX + 9},
   {NULL, no_argument, NULL, 0}
 };
 
@@ -580,6 +581,7 @@ static char const *const option_help[] =
 "  --verbose  Output extra information about the work being done.",
 "  --dry-run  Do not actually change any files; just print what would happen.",
 "  --posix  Conform to the POSIX standard.",
+"  --strip-indent handle indented patches.",
 "",
 "  -d DIR  --directory=DIR  Change the working directory to DIR first.",
 #if HAVE_SETMODE_DOS
@@ -779,6 +781,9 @@ get_some_switches (void)
 				     (enum quoting_style) i);
 		}
 		break;
+	    case CHAR_MAX + 9: /* --strip-indent */
+		strip_indentation = true;
+		break;
 	    default:
 		usage (stderr, 2);
 	}
diff --git a/pch.c b/pch.c
index d98af86..fcb08c5 100644
--- a/pch.c
+++ b/pch.c
@@ -345,6 +345,8 @@ intuit_diff_type (void)
 	}
 	strip_trailing_cr = 2 <= chars_read && buf[chars_read - 2] == '\r';
 	for (s = buf; *s == ' ' || *s == '\t' || *s == 'X'; s++) {
+	    if (!strip_indentation)
+		break;
 	    if (*s == '\t')
 		indent = (indent + 8) & ~7;
 	    else

Re: being nice to patch(1)

[Linus Torvalds]

On Tue, 3 Jul 2007, Linus Torvalds wrote:
> 
> But maybe you can use this patch as a starting point, at least.

Oh, Paul - I forgot to mention, and since it wasn't necessarily clear from 
the patch..

I used the patch-2.5.9 sources as a base for this. I don't know how 
official that source base is, I picked it up from a Debian package as the 
"original tar-ball": patch_2.5.9.orig.tar.gz

I suspect it applies to just about any version of patch with no 
modifications, but just to clarify what the base was. Judging by the 
ChangeLog, you're the one who has been doing the later 2.5.x releases even 
though the last version on the GNU sites is 2.5.4.

Confusing.

			Linus

Re: being nice to patch(1)

[David Kastrup]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> But maybe they would be willing to install git to get that wonderful
> git-apply program, and that wonderful rename-and-mode-aware
> git-diff, and the git-merge-file program, all of which can operate
> outside of a git repository. (Take that, hg!)

As long as git-diff lists all added files in a second non-git dirtree
as "/dev/null" when doing
git-diff --name-status -B -M -C dir1 dir2
its usefulness is limited.

git-diff --name-status -B -M -C dir1 dir2
D	dir1/auctex-11.84/CHANGES
D	dir1/auctex-11.84/COPYING
D	dir1/auctex-11.84/ChangeLog

[...]

D	dir1/auctex-11.84/preview/preview-latex.spec
D	dir1/auctex-11.84/preview/prv-emacs.el
D	dir1/auctex-11.84/preview/prv-install.el
D	dir1/auctex-11.84/tex-site.el.in
D	dir1/auctex-11.84/tex-wizard.el
A	/dev/null
A	/dev/null
R100	dir1/auctex-11.84/images/amstex.xpm	dir2/etc/auctex/images/amstex.xpm
R100	dir1/auctex-11.84/images/bibtex.xpm	dir2/etc/auctex/images/bibtex.xpm
R100	dir1/auctex-11.84/images/dropdown.xpm	dir2/etc/auctex/images/dropdown.xpm

[...]

R100	dir1/auctex-11.84/images/viewdvi.xpm	dir2/etc/auctex/images/viewdvi.xpm
R100	dir1/auctex-11.84/images/viewpdf.xpm	dir2/etc/auctex/images/viewpdf.xpm
R100	dir1/auctex-11.84/images/viewps.xpm	dir2/etc/auctex/images/viewps.xpm
A	/dev/null
A	/dev/null
A	/dev/null
A	/dev/null
A	/dev/null
A	/dev/null

and so on.

git --version
git version 1.5.2.2.565.gde09

-- 
David Kastrup

Re: being nice to patch(1)

[Johannes Schindelin]

Hi David,

[please Cc me, since I will be more likely to miss replies if you do not]

On Tue, 3 Jul 2007, David Kastrup wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > But maybe they would be willing to install git to get that wonderful 
> > git-apply program, and that wonderful rename-and-mode-aware git-diff, 
> > and the git-merge-file program, all of which can operate outside of a 
> > git repository. (Take that, hg!)
> 
> As long as git-diff lists all added files in a second non-git dirtree
> as "/dev/null" when doing
> git-diff --name-status -B -M -C dir1 dir2
> its usefulness is limited.
> 
> git-diff --name-status -B -M -C dir1 dir2
> D	dir1/auctex-11.84/CHANGES
> D	dir1/auctex-11.84/COPYING
> D	dir1/auctex-11.84/ChangeLog
> 
> [...]

Yes, directories are a problem. There our DWIMery does not really help. 
But there is a solution: say

	git diff --name-status --no-index -B -M -C dir1 dir2

Hth,
Dscho

Re: being nice to patch(1)

[David Kastrup]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi David,
>
> [please Cc me, since I will be more likely to miss replies if you do not]
>
> On Tue, 3 Jul 2007, David Kastrup wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>> > But maybe they would be willing to install git to get that wonderful 
>> > git-apply program, and that wonderful rename-and-mode-aware git-diff, 
>> > and the git-merge-file program, all of which can operate outside of a 
>> > git repository. (Take that, hg!)
>> 
>> As long as git-diff lists all added files in a second non-git dirtree
>> as "/dev/null" when doing
>> git-diff --name-status -B -M -C dir1 dir2
>> its usefulness is limited.
>> 
>> git-diff --name-status -B -M -C dir1 dir2
>> D	dir1/auctex-11.84/CHANGES
>> D	dir1/auctex-11.84/COPYING
>> D	dir1/auctex-11.84/ChangeLog
>> 
>> [...]
>
> Yes, directories are a problem. There our DWIMery does not really help. 
> But there is a solution: say
>
> 	git diff --name-status --no-index -B -M -C dir1 dir2

It would help if you actually read what you are replying to.  The
problem is that added files are listed as "/dev/null", and --no-index
does not make a difference here.  It actually makes no apparent
difference at all when outside of a non-git dirtree.  Hardly
surprising, since no index file that could be consulted is present in
the first place.

The output still is (editing somewhat more so that it becomes even
more obvious):

git-diff -B -M -C --no-index --name-status dir1 dir2
D	dir1/auctex-11.84/CHANGES

[...]

A	/dev/null
A	/dev/null
R100	dir1/auctex-11.84/images/amstex.xpm	dir2/etc/auctex/images/amstex.xpm

[...]

_All_ lines starting in A end with /dev/null.

-- 
David Kastrup

[PATCH] diff --no-index: fix --name-status with added files

[Johannes Schindelin]

Without this patch, an added file would be reported as /dev/null.

Noticed by David Kastrup.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	Would be nice, next time, to have a bug report which is not 
	embedded in a thread.

 diff.c                                   |    3 ++-
 t/t4013-diff-various.sh                  |    2 ++
 t/t4013/diff.diff_--name-status_dir2_dir |    3 +++
 3 files changed, 7 insertions(+), 1 deletions(-)
 create mode 100644 t/t4013/diff.diff_--name-status_dir2_dir

diff --git a/diff.c b/diff.c
index b6eb72b..1958970 100644
--- a/diff.c
+++ b/diff.c
@@ -2418,7 +2418,8 @@ static void diff_flush_raw(struct diff_filepair *p,
 		printf("%s ",
 		       diff_unique_abbrev(p->two->sha1, abbrev));
 	}
-	printf("%s%c%s", status, inter_name_termination, path_one);
+	printf("%s%c%s", status, inter_name_termination,
+			two_paths || p->one->mode ?  path_one : path_two);
 	if (two_paths)
 		printf("%c%s", inter_name_termination, path_two);
 	putchar(line_termination);
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index b453b42..9eec754 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -17,6 +17,7 @@ test_expect_success setup '
 	export GIT_AUTHOR_DATE GIT_COMMITTER_DATE &&
 
 	mkdir dir &&
+	mkdir dir2 &&
 	for i in 1 2 3; do echo $i; done >file0 &&
 	for i in A B; do echo $i; done >dir/sub &&
 	cat file0 >file2 &&
@@ -254,6 +255,7 @@ diff --patch-with-stat initial..side
 diff --patch-with-raw initial..side
 diff --patch-with-stat -r initial..side
 diff --patch-with-raw -r initial..side
+diff --name-status dir2 dir
 EOF
 
 test_done
diff --git a/t/t4013/diff.diff_--name-status_dir2_dir b/t/t4013/diff.diff_--name-status_dir2_dir
new file mode 100644
index 0000000..ef7fdb7
--- /dev/null
+++ b/t/t4013/diff.diff_--name-status_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --name-status dir2 dir
+A	dir/sub
+$
-- 
1.5.3.rc0.2637.g1dd84-dirty

Re: being nice to patch(1)

[Johannes Schindelin]

Hi,

On Tue, 3 Jul 2007, David Kastrup wrote:

> It would help if you actually read what you are replying to.

Actually, your second explanation helped. Fix posted separately.

Ciao,
Dscho

Re: being nice to patch(1)

[David Kastrup]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Tue, 3 Jul 2007, David Kastrup wrote:
>
>> It would help if you actually read what you are replying to.
>
> Actually, your second explanation helped. Fix posted separately.

Thanks.

-- 
David Kastrup

Re: being nice to patch(1)

[David Kastrup]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> > >
>> > > I guess the second choice generally isn't an option, but dammit, 
>> > > "git-apply" really is the better program here.
>> > 
>> > Why not?  git-apply works outside of a git repo ;-)
>> 
>> I was more thinking that people are not necessarily willing to install git 
>> just to get the "git-apply" program..
>
> But maybe they would be willing to install git to get that wonderful
> git-apply program, and that wonderful rename-and-mode-aware
> git-diff, and the git-merge-file program, all of which can operate
> outside of a git repository. (Take that, hg!)

Well, hmph!  I just rewrote my git-diff-using script to not check
stuff into a throw-away git repository, and guess what: with real-life
use cases (diffing trees of about 500MB size), git-diff runs out of
memory (the machine probably has something like 1.5GB of virtual memory
size) when operating outside of a git repository.

So the usefulness still seems limited, even now that the output format
of --name-status has been fixed.

Any idea whether this is a bug, sloppy programming, or an inherent
restriction/necessity?

Also an idea which of the following scenarios would be best for
catching all of moves/renames/deletes/adds?  Note: any repository is
strictly throw-away.

Experiments are somewhat time-consuming, so every hunch helps.

a) diff directories outside of git (works, but fatal memory footprint
                                    for large cases)
b) diff index against work directory
c) diff revision against work directory
d) diff revision against index
e) diff revision against revision (works, but high disk footprint and
                                   likely slower than alternatives)

Thanks,

-- 
David Kastrup

git-diff memory/speed/disk impacts (was: being nice to patch(1))

[David Kastrup]

Some more experiments:

David Kastrup <dak@gnu.org> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> > >
>>> > > I guess the second choice generally isn't an option, but dammit, 
>>> > > "git-apply" really is the better program here.
>>> > 
>>> > Why not?  git-apply works outside of a git repo ;-)
>>> 
>>> I was more thinking that people are not necessarily willing to install git 
>>> just to get the "git-apply" program..
>>
>> But maybe they would be willing to install git to get that wonderful
>> git-apply program, and that wonderful rename-and-mode-aware
>> git-diff, and the git-merge-file program, all of which can operate
>> outside of a git repository. (Take that, hg!)
>
> Well, hmph!  I just rewrote my git-diff-using script to not check
> stuff into a throw-away git repository, and guess what: with real-life
> use cases (diffing trees of about 500MB size), git-diff runs out of
> memory (the machine probably has something like 1.5GB of virtual memory
> size) when operating outside of a git repository.
>
> So the usefulness still seems limited, even now that the output format
> of --name-status has been fixed.
>
> Any idea whether this is a bug, sloppy programming, or an inherent
> restriction/necessity?
>
> Also an idea which of the following scenarios would be best for
> catching all of moves/renames/deletes/adds?  Note: any repository is
> strictly throw-away.
>
> Experiments are somewhat time-consuming, so every hunch helps.
>
> a) diff directories outside of git (works, but fatal memory footprint
>                                     for large cases)
> b) diff index against work directory
fatal memory footprint
> c) diff revision against work directory
fatal memory footprint
> d) diff revision against index
does not detect copies/renames
> e) diff revision against revision (works, but high disk footprint and
>                                    likely slower than alternatives)

So it seems like option e) is the only feasible option.  In the total
numbers, git-add is by far the slowest operation, followed by
git-commit.  git-diff on revisions is quite fast and with moderate
memory footprint.

Committing itself does not seem to add much disk space: adding into
the index seems to be the main disk space allocation.

So while the behavior of d) appears puzzling, doing another commit
before the diff is cheap, so the motivation for asking people to find
out the problems with d) is low for me.

Somewhat dissatisfactory that rewriting my script for using the
repository-less variant of git-diff fails for seriously large use
cases due to out-of-memory conditions.

I suppose that's life.

-- 
David Kastrup

Re: being nice to patch(1)

[Linus Torvalds]

On Fri, 6 Jul 2007, David Kastrup wrote:
> 
> Well, hmph!  I just rewrote my git-diff-using script to not check
> stuff into a throw-away git repository, and guess what: with real-life
> use cases (diffing trees of about 500MB size), git-diff runs out of
> memory (the machine probably has something like 1.5GB of virtual memory
> size) when operating outside of a git repository.

Ok, that's probably some huge memory leak that just doesn't show up with 
any normal git operations, likely simply because all the normal git 
operations will have thrown out the case of "identical files" without ever 
even looking at the file.

I'd guess that when using the diff logic on outside files, we'll read them 
all in, compare them, and keep them all in memory even though they are 
identical.

Generally, though, "git diff" has a much higher memory footprint than any 
normal file-by-file recursive diff, exactly because of the rename logic. 
An external "diff" won't ever have any reason to keep more than two files 
in memory at a time, but because git diff does rename and copy detection, 
it wants to keep the file data in memory over much longer times.

But I bet there is some stupid bug where we just make it much much worse 
for the "no git tree/index" case, and keep the whole tree in memory or 
something.

(The same is true of "git apply", btw, for a different reason: because 
git-apply will refuse to write out partial results in case some later 
patch fails, git-apply will keep the whole result in memory until the very 
end, and then do the write-out in one go. Again, that obviously means 
that it will potentially use a lot more memory than the "one patch at a 
time" approach that regular "patch" does)

			Linus

Re: [Quilt-dev] Re: being nice to patch(1)

[Andreas Gruenbacher]

On Tuesday 03 July 2007 02:28, Linus Torvalds wrote:
> So I would suggest that in quilt and other systems, you either:
>
>  - strip all headers manually
>
>  - forget about "patch", and use "git-apply" instead that does things
>    right and doesn't screw up like this (and can do rename diffs etc too).
>
> I guess the second choice generally isn't an option, but dammit,
> "git-apply" really is the better program here.

I'm in bit of a conflict with choice one: when applying patches in an 
automated build process or similar, the likely way to do so is a simple loop 
over the series file. So the less magic when applying patches with quilt, the 
better.

Turning off the insane heuristic with patch -u will do well enough I hope. 
Quilt does not use that option by default because it also supports context 
diffs (some people / projects prefer them), but that can easily be customized 
in .quiltrc:

    QUILT_PATCH_OPTS=-u

Andreas

Re: [Quilt-dev] Re: being nice to patch(1)

[Andrew Morton]

On Tue, 3 Jul 2007 15:34:46 +0200 Andreas Gruenbacher <agruen@suse.de> wrote:

> On Tuesday 03 July 2007 02:28, Linus Torvalds wrote:
> > So I would suggest that in quilt and other systems, you either:
> >
> >  - strip all headers manually
> >
> >  - forget about "patch", and use "git-apply" instead that does things
> >    right and doesn't screw up like this (and can do rename diffs etc too).
> >
> > I guess the second choice generally isn't an option, but dammit,
> > "git-apply" really is the better program here.
> 
> I'm in bit of a conflict with choice one: when applying patches in an 
> automated build process or similar, the likely way to do so is a simple loop 
> over the series file. So the less magic when applying patches with quilt, the 
> better.
> 
> Turning off the insane heuristic with patch -u will do well enough I hope. 
> Quilt does not use that option by default because it also supports context 
> diffs (some people / projects prefer them), but that can easily be customized 
> in .quiltrc:
> 
>     QUILT_PATCH_OPTS=-u
> 

I guess one could try `patch -p1' and if that failed, `patch -p1 -u'.

But the problem is that patch will get stuck in interactive mode prompting
for a filename.  I've never actually worked how to make patch(1) just fail
rather than going interactive, not that I've tried terribly hard.  Any
hints there?

Thanks.

Re: [Quilt-dev] Re: being nice to patch(1)

[Linus Torvalds]

On Tue, 3 Jul 2007, Andrew Morton wrote:
> 
> But the problem is that patch will get stuck in interactive mode prompting
> for a filename.  I've never actually worked how to make patch(1) just fail
> rather than going interactive, not that I've tried terribly hard.  Any
> hints there?

"patch -t" (or "--batch") should do it, I suspect.

But even with "patch -tu -p1" you do seem to end up having patch notice 
indented patch fragments (ie things that obviously are *not* part of the 
patch, but some explanation).

		Linus

Re: [Quilt-dev] Re: being nice to patch(1)

[Andreas Gruenbacher]

On Tuesday 03 July 2007 17:49, Andrew Morton wrote:
> I guess one could try `patch -p1' and if that failed, `patch -p1 -u'.

Hmm, I'll think about that, thanks.

> But the problem is that patch will get stuck in interactive mode prompting
> for a filename.  I've never actually worked how to make patch(1) just fail
> rather than going interactive, not that I've tried terribly hard.  Any
> hints there?

Patch -f will turn off those questions.

Andreas

Re: [Quilt-dev] Re: being nice to patch(1)

[Andrew Morton]

On Tue, 3 Jul 2007 18:03:15 +0200 Andreas Gruenbacher <agruen@suse.de> wrote:

> On Tuesday 03 July 2007 17:49, Andrew Morton wrote:
> > I guess one could try `patch -p1' and if that failed, `patch -p1 -u'.
> 
> Hmm, I'll think about that, thanks.
> 
> > But the problem is that patch will get stuck in interactive mode prompting
> > for a filename.  I've never actually worked how to make patch(1) just fail
> > rather than going interactive, not that I've tried terribly hard.  Any
> > hints there?
> 
> Patch -f will turn off those questions.
> 

darnit, both `-f' and `-t' work.  Sigh.  I blame the manpage: too long ;)

Incidentally, the offending patch
(http://userweb.kernel.org/~akpm/git-scsi-misc.patch) sends patch(1) into
an infinite loop with `patch -p1 -f' and `patch -p1 -t'.  Presumably
it will do the same when that patch is offered to quilt...

Re: Re: being nice to patch(1)

[Andrew Morton]

On Tue, 3 Jul 2007 18:03:15 +0200 Andreas Gruenbacher <agruen@suse.de> wrote:

> On Tuesday 03 July 2007 17:49, Andrew Morton wrote:
> > I guess one could try `patch -p1' and if that failed, `patch -p1 -u'.
> 
> Hmm, I'll think about that, thanks.
> 
> > But the problem is that patch will get stuck in interactive mode prompting
> > for a filename.  I've never actually worked how to make patch(1) just fail
> > rather than going interactive, not that I've tried terribly hard.  Any
> > hints there?
> 
> Patch -f will turn off those questions.
> 

darnit, both `-f' and `-t' work.  Sigh.  I blame the manpage: too long ;)

Incidentally, the offending patch
(http://userweb.kernel.org/~akpm/git-scsi-misc.patch) sends patch(1) into
an infinite loop with `patch -p1 -f' and `patch -p1 -t'.  Presumably
it will do the same when that patch is offered to quilt...

Re: Re: being nice to patch(1)

[Andrew Morton]

On Tue, 3 Jul 2007 15:34:46 +0200 Andreas Gruenbacher <agruen@suse.de> wrote:

> On Tuesday 03 July 2007 02:28, Linus Torvalds wrote:
> > So I would suggest that in quilt and other systems, you either:
> >
> >  - strip all headers manually
> >
> >  - forget about "patch", and use "git-apply" instead that does things
> >    right and doesn't screw up like this (and can do rename diffs etc too).
> >
> > I guess the second choice generally isn't an option, but dammit,
> > "git-apply" really is the better program here.
> 
> I'm in bit of a conflict with choice one: when applying patches in an 
> automated build process or similar, the likely way to do so is a simple loop 
> over the series file. So the less magic when applying patches with quilt, the 
> better.
> 
> Turning off the insane heuristic with patch -u will do well enough I hope. 
> Quilt does not use that option by default because it also supports context 
> diffs (some people / projects prefer them), but that can easily be customized 
> in .quiltrc:
> 
>     QUILT_PATCH_OPTS=-u
> 

I guess one could try `patch -p1' and if that failed, `patch -p1 -u'.

But the problem is that patch will get stuck in interactive mode prompting
for a filename.  I've never actually worked how to make patch(1) just fail
rather than going interactive, not that I've tried terribly hard.  Any
hints there?

Thanks.