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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Thu, 5 Jun 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > 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.

As you pointed out, I wanted to support only add -e.  But that should not 
be an issue at all.  I think a "case ' ': case '\n':" should be enough, 
right?

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

How about stopping on "@@" and end of file only, and complaining 
otherwise?

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

Fine!

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

Point taken.

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

Is this really an issue?  fixup_counts() is only called after a hunk 
header was read, and that should be well after any "^-- $".

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

Right.

Will fix up and resubmit.

Ciao,
Dscho