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

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.

Hi,

On Fri, 6 Jul 2007, Adrian Bunk wrote:

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

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

Hi Johannes,

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

cu Adrian

Hi,

On Fri, 6 Jul 2007, Adrian Bunk wrote:

Not yet, thankfully.

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.

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

On Fri, Jul 06, 2007 at 02:51:07AM +0100, Johannes Schindelin wrote:

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.

cu Adrian

Hi,

On Fri, 6 Jul 2007, Adrian Bunk wrote:

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.

That is _your_ use case.

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

On Fri, 6 Jul 2007, Johannes Schindelin wrote:

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

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

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.

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.

No question about it.

On Thu, Jul 05, 2007 at 10:41:51PM -0700, Junio C Hamano wrote:

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

Hi,

On Fri, 6 Jul 2007, Adrian Bunk wrote:

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