Re: [PATCH v2 1/2] Allow git-apply to ignore the hunk headers

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 5, 2008, 21:16 UTC
Message-ID: <7vabhz1t2f.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <alpine.DEB.1.00.0806051720070.21190@racer>

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

Show 6 quoted lines

> Sometimes, the easiest way to fix up a patch is to edit it directly, even
> adding or deleting lines.  Now, many people are not as divine as certain
> benevolent dictators as to update the hunk headers correctly at the first
> try.
>
> So teach the tool to do it for us.

Two comments and a half.

 * Latest POSIX draft talks about unified context and allows an empty line
   to represent an empty common context line.  GNU diff already emits such
   a diff.  fixup_counts() should take this into account.

 * I'd sleep better at night if 'Probably "diff ..."' part were written in
   a bit more robust way.

 * (minor) There is an established term for this operation: recountdiff,
   so --recount might be a better name.  fixup_counts() also is better
   called recount_diff() if we go this route.

If you are too narrowly focused to only support "git add -e", the first issue does not matter, because we always emit "SP LF" for such a common context. The reason why I care about the first two points is because we may want to teach git-am about this new option as well in 1.6.0.

And the robustness issue I worry about the second point also applies to a line that is "^-- $", especially if we were to make this available to git-am. Perhaps when the line begins with a '-', the logic could be extra careful to detect the case where the line looks like the e-mail signature separator and check one line beyond it to see if it does not look anything like part of a diff (in which case you stop, without considering the line you are currently looking at, "^-- $", a deletion of "^- $", as part of the preimage context).

As to code structure, we might want to make the later parameters to apply_patch() an integer, of OR'ed flag values, or even a pointer to a structure that holds options.

Other than that, the patch looks reasonably isolated and clean.