Re: [RFC PATCH] git-add--interactive: manual hunk editing mode v2

[Jeff King <peff@peff.net>]

On Thu, Jun 05, 2008 at 09:53:54AM +0200, Thomas Rast wrote:

> On the other hand it would be just as powerful.  Manually splitting a
> hunk is, in the general case, only possible in "my" scheme.  However,
> to make any difference, you later have to answer 'n' to some of the
> sub-hunks.  So in "your" scheme, you could just have deleted the lines
> in question.

Sorry, I've gotten lost in which is mine and which is yours. Yours is
the original patch or this patch? Mine is the proposal that spawned this
patch, or my comments on this patch?

> > What about lines starting with characters besides -, +, space, or @?
> > They will normally be ignored by diff.
> 
> Diff doesn't really have a say in this, does it?  And looking in
> builtin-apply.c:

Sorry, I meant to say "patch" here. But even so, I'm still wrong.
Arbitrary text is OK between _diffs_, but not between _hunks_. And by
definition, this format is hunks inside a single diff.

> 	default:
> 		if (apply_verbosely)
> 			error("invalid start of line: '%c'", first);
> 		return -1;

Right, so we signal the end of diff, and any hunks afterwards are "patch
fragment without header".

> so it appears invalid lines are actually not ignored, but abort hunk
> processing.  While the error checking will be handled by apply
> --check, I don't think it would be a good idea to silently drop all
> other lines from the edit, as they probably indicate user error.

Good point. There's really no reason for such lines to occur, so the
best thing is probably to notice the situation and let the user re-edit.

> On the other hand, this also shows that dropping empty lines is
> wrong...

Hmm. Yes, it looks like they are significant. Given that they are
generated by a particular version of GNU diff, and that these should be
"git diff"-generated patches, we aren't likely to encounter them. But
probably we should just leave them as-is, and let "git apply --check" do
what it will with them.

> From a Perl POV, they probably _are_ horrible.  I'm just not used to
> the idioms, and tend to fall for semantic differences to Python as
> well.

Heh. The Perl style in git is a bit tricky; it is such a small portion
of the code base, and there is not a real sense of ownership, so we seem
to have several different styles.

> Thank you for the very thorough review!  I'll improve the patch
> accordingly.

Great. I think this is a worthwhile feature, so please keep at it.

-Peff