git-apply{,mbox,patch} should default to --unidiff-zero

git-apply{,mbox,patch} should default to --unidiff-zero

[Adrian Bunk]

I just ran into the following issue:

I sent someone a patch that purposefully contained a chunk without 
context, and git-apply of the recipient refused to apply it without
an explicit --unidiff-zero.

git-apply{,mbox,patch} should default to doing --unidiff-zero:
Generating a patch without context is something I have to do explicitely 
by giving "diff" an option or by manually editing the patch. I know 
about the dangers of having no context, but there are use cases where
I know that replacing and/or deleting one or more lines is safe even 
without context and where I want to avoid context e.g. for avoiding to 
clash with other patches.

Example use case:
Look at the file Documentation/feature-removal-schedule.txt in the Linux 
kernel. If I want to send someone two independent patches removing 
adjanced entries in this file, the patches can be applied in any order 
exactly as long as this chunk does not contain any context. Removing an 
entry from this file is obviously safe even without any context.

TIA
Adrian

BTW: Please Cc me on replies.

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: git-apply{,mbox,patch} should default to --unidiff-zero

[Johannes Schindelin]

Hi,

On Fri, 6 Jul 2007, Adrian Bunk wrote:

> git-apply{,mbox,patch} should default to doing --unidiff-zero:

But is that not dangerous? At least now the committer has some safeguard 
against this kind of mistakes. Because you can easily introduce mistakes 
that way.

Ciao,
Dscho

Re: git-apply{,mbox,patch} should default to --unidiff-zero

[Adrian Bunk]

On Fri, Jul 06, 2007 at 02:18:46AM +0100, Johannes Schindelin wrote:

> Hi,

Hi Johannes,

> On Fri, 6 Jul 2007, Adrian Bunk wrote:
> 
> > git-apply{,mbox,patch} should default to doing --unidiff-zero:
> 
> But is that not dangerous? At least now the committer has some safeguard 
> against this kind of mistakes. Because you can easily introduce mistakes 
> that way.

you are saying "easily".

Did you ever actually run into such a problem?

You must do something like "diff -U0" or manually editing patches for 
creating such patches, and that's very unusual.

And although GNU patch (which has a much bigger userbase than git) 
applies such patches without any warning I don't remember having ever 
seen what you call "easily".

> Ciao,
> Dscho

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: git-apply{,mbox,patch} should default to --unidiff-zero

[Johannes Schindelin]

Hi,

On Fri, 6 Jul 2007, Adrian Bunk wrote:

> On Fri, Jul 06, 2007 at 02:18:46AM +0100, Johannes Schindelin wrote:
> 
> > On Fri, 6 Jul 2007, Adrian Bunk wrote:
> > 
> > > git-apply{,mbox,patch} should default to doing --unidiff-zero:
> > 
> > But is that not dangerous? At least now the committer has some 
> > safeguard against this kind of mistakes. Because you can easily 
> > introduce mistakes that way.
> 
> you are saying "easily".
> 
> Did you ever actually run into such a problem?

Not yet, thankfully.

> You must do something like "diff -U0" or manually editing patches for 
> creating such patches, and that's very unusual.

The point is that the _committer_ is not necessarily involved in that 
business.

And "git apply" is strict for a reason. It catches possibly unwanted 
things much earlier than patch. I _want_ to be warned that somebody is 
introducing some code at a certain position, which might, or might not be 
correct. apply has no way to tell, since there is no context to at least 
minimally verify.

> And although GNU patch (which has a much bigger userbase than git) 
> applies such patches without any warning I don't remember having ever 
> seen what you call "easily".

GNU patch is very sloppy. And I had to fix up quite a number of patches 
which were "successfully" applied, but did not do what they were supposed 
to do. The recent "GNU patch applies _indented_ _context_ diffs" fracass 
is only one example why I prefer git apply.

Unfortunately, I do not off-hand remember if I had to fix up a 
unified-zero patch that GNU patch applied, but I do know this:

	if "git am" learns to apply unified-zero by default, the first 
	thing I will do is patch it in my Git branch to _not_ do that. I 
	do _not_ want that. I want to be warned.

	I can still decide that it is probably okay, but I will make 
	_damned_ _well_ sure afterwards that it did something sensible. I 
	will _only_ apply such a scrutiny when git apply refused to apply 
	a unified-zero patch, and I decided to apply it nevertheless.

Ciao,
Dscho

Re: git-apply{,mbox,patch} should default to --unidiff-zero

[Adrian Bunk]

On Fri, Jul 06, 2007 at 02:51:07AM +0100, Johannes Schindelin wrote:
> Hi,
> 
> On Fri, 6 Jul 2007, Adrian Bunk wrote:
> 
> > On Fri, Jul 06, 2007 at 02:18:46AM +0100, Johannes Schindelin wrote:
> > 
> > > On Fri, 6 Jul 2007, Adrian Bunk wrote:
> > > 
> > > > git-apply{,mbox,patch} should default to doing --unidiff-zero:
> > > 
> > > But is that not dangerous? At least now the committer has some 
> > > safeguard against this kind of mistakes. Because you can easily 
> > > introduce mistakes that way.
> > 
> > you are saying "easily".
> > 
> > Did you ever actually run into such a problem?
> 
> Not yet, thankfully.
> 
> > You must do something like "diff -U0" or manually editing patches for 
> > creating such patches, and that's very unusual.
> 
> The point is that the _committer_ is not necessarily involved in that 
> business.
> 
> And "git apply" is strict for a reason. It catches possibly unwanted 
> things much earlier than patch. I _want_ to be warned that somebody is 
> introducing some code at a certain position, which might, or might not be 
> correct. apply has no way to tell, since there is no context to at least 
> minimally verify.
>...

That's wrong.

My use cases are replacing or deleting lines.

In these cases there is context in the deleted lines that is already 
being verified even with --unidiff-zero.

> Ciao,
> Dscho

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: git-apply{,mbox,patch} should default to --unidiff-zero

[Johannes Schindelin]

Hi,

On Fri, 6 Jul 2007, Adrian Bunk wrote:

> On Fri, Jul 06, 2007 at 02:51:07AM +0100, Johannes Schindelin wrote:
> > 
> > On Fri, 6 Jul 2007, Adrian Bunk wrote:
> > 
> > > You must do something like "diff -U0" or manually editing patches 
> > > for creating such patches, and that's very unusual.
> > 
> > The point is that the _committer_ is not necessarily involved in that 
> > business.

BTW this still holds true, and you have not addressed that.  It really is 
a serious issue.  "git apply" is a committer's tool.  So it should help 
the committer.

> > And "git apply" is strict for a reason. It catches possibly unwanted 
> > things much earlier than patch. I _want_ to be warned that somebody is 
> > introducing some code at a certain position, which might, or might not 
> > be correct. apply has no way to tell, since there is no context to at 
> > least minimally verify.
> >...
> 
> That's wrong.
> 
> My use cases are replacing or deleting lines.

That is _your_ use case.

> In these cases there is context in the deleted lines that is already 
> being verified even with --unidiff-zero.

With --unidiff-zero, also _adding_ lines will be handled as if there were 
no problem.

Yes, in your case it fixes a problem.

Yet, in other cases it introduces a problem.

Okay?

Ciao,
Dscho

Re: git-apply{,mbox,patch} should default to --unidiff-zero

[Linus Torvalds]

On Fri, 6 Jul 2007, Johannes Schindelin wrote:
> 
> With --unidiff-zero, also _adding_ lines will be handled as if there were 
> no problem.
> 
> Yes, in your case it fixes a problem.
> 
> Yet, in other cases it introduces a problem.

Well, we could make the rule be that ew require --unidiff-zero only if 
there really is _no_ old data to verify in a hunk. No deleted lines, and 
no context around it.

Adrian has a point in that if there are lines to be deleted, that in 
itself is context, and then the strict behaviour of "git-apply" is 
arguably unnecessaily strict.

That said, I do absolutely _hate_ how GNU patch will basically apply 
random line noise without complaints. So git-apply is designed to be much 
stricter on _so_ many levels. The thing that I personally always really 
detested about GNU patch was how it would apply part of a patch, then fail 
half-way, and leave the partial patch applied!

git-apply is about a million times better than standard "patch", exactly 
because it tries to make sure that what it does makes sense, and you 
actually need to use explicit flags to make it do things that may be hard 
to undo or slightly questionable.

		Linus

Re: git-apply{,mbox,patch} should default to --unidiff-zero

[Junio C Hamano]

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

> Well, we could make the rule be that we require --unidiff-zero only if 
> there really is _no_ old data to verify in a hunk. No deleted lines, and 
> no context around it.

There are two things --unidiff-zero affects, because git-apply
needs to disable otherwise reasonable sanity checks it does for
safety:

 - When we see a patch that has only one hunk, and its change
   consists of only deletion, we can verify and complain if the
   header does not say it delete the file.  Not so if the patch
   was created without "diff -u0".  The same applies for "only
   addition" vs.  "creation of the file".

 - When there is no leading context in the hunk, it usually has
   to match only at the beginning of the file (same for
   "following context" vs "at the end of the file"), and we do
   perform this sanity check.  However, "diff -u0" patch needs
   to bypass it, as not having any context is the norm.

Because "diff -u0" is unusual, these sanity checks are disabled
only when the user explicitly says --unidiff-zero when applying.

> Adrian has a point in that if there are lines to be deleted, that in 
> itself is context, and then the strict behaviour of "git-apply" is 
> arguably unnecessaily strict.

Not really.  That is true, unless you have two identical
instances of the group of lines being deleted, in which case you
cannot safely tell which instance is to be removed.

> That said, I do absolutely _hate_ how GNU patch will basically apply 
> random line noise without complaints. So git-apply is designed to be much 
> stricter on _so_ many levels. The thing that I personally always really 
> detested about GNU patch was how it would apply part of a patch, then fail 
> half-way, and leave the partial patch applied!
>
> git-apply is about a million times better than standard "patch", exactly 
> because it tries to make sure that what it does makes sense, and you 
> actually need to use explicit flags to make it do things that may be hard 
> to undo or slightly questionable.

No question about it.

Re: git-apply{,mbox,patch} should default to --unidiff-zero

[Adrian Bunk]

On Thu, Jul 05, 2007 at 10:41:51PM -0700, Junio C Hamano wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>...
> > Adrian has a point in that if there are lines to be deleted, that in 
> > itself is context, and then the strict behaviour of "git-apply" is 
> > arguably unnecessaily strict.
> 
> Not really.  That is true, unless you have two identical
> instances of the group of lines being deleted, in which case you
> cannot safely tell which instance is to be removed.
>...

The interesting thing is that you can never safely tell it for any 
amount of context - I've seen patches with three lines of context being 
applied at the wrong place simply because there were several matching 
contexts.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: git-apply{,mbox,patch} should default to --unidiff-zero

[Johannes Schindelin]

Hi,

On Fri, 6 Jul 2007, Adrian Bunk wrote:

> On Thu, Jul 05, 2007 at 10:41:51PM -0700, Junio C Hamano wrote:
> > Linus Torvalds <torvalds@linux-foundation.org> writes:
> >...
> > > Adrian has a point in that if there are lines to be deleted, that in 
> > > itself is context, and then the strict behaviour of "git-apply" is 
> > > arguably unnecessaily strict.
> > 
> > Not really.  That is true, unless you have two identical instances of 
> > the group of lines being deleted, in which case you cannot safely tell 
> > which instance is to be removed.
> >...
> 
> The interesting thing is that you can never safely tell it for any 
> amount of context - I've seen patches with three lines of context being 
> applied at the wrong place simply because there were several matching 
> contexts.

Yes, that is right. You can never safely tell. But now you want to allow 
even less context by default. In which you can even "more neverer" safely 
tell. That is why I am disagreeing with that change.

Ciao,
Dscho