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

[Jeff King <peff@peff.net>]

On Wed, Jun 11, 2008 at 11:02:29AM +0200, Thomas Rast wrote:

> However, as in the earlier versions, the hunk is placed in the editing
> loop again.  In this version, I made it so it will be marked for use
> as if you had entered 'y', so if it was the last (or only) hunk, the
> files will be patched immediately.  But if you Ctrl-C out in the
> middle, nothing has been changed yet.

OK, I really think this is the most sane behavior. It naturally follows
what the user wants (if they edit, they then want to apply), but it
still waits until the end of the loop, so they can back out if they
want.

> By the way, current 'add -p' recounts the headers to account for all
> hunks that the user answered 'n'.  I haven't given it enough thought
> to put it in the code yet, but it may be possible to rip that out as
> well and unconditionally --recount.  Only the preimage line numbers
> matter, and those never change.

Ah, I hadn't thought about that, but of course it makes sense that it
would need to recount (I guess I should have looked a little more
closely at the other code). I think replacing that with --recount would
be great, but it should be a separate patch, and we should wait for
--recount to stabilize (all of this should be post 1.5.6, anyway).

> +# If the patch applies cleanly, the hunk will immediately be marked
> +# for staging as if you had answered 'y'.  (However, if you remove
> +# everything, nothing happens.)  Otherwise, you will be asked to edit
> +# again.

This "however" confused me the first time I read it. I assume you mean
that "if the diff is empty, then staging it will do nothing"? I wonder
if that is even worth mentioning, since it seems obvious.

Although I guess you do special-case "no lines" later on.

> +# Do not add @ lines unless you know what you are doing.  The original
> +# @ line will be reinserted if you remove it.

This can probably be moved from the "every time" instructions to the
manual, if we want to keep the size of the instructions a bit smaller.

> +		if (diff_applies($head,
> +				 @{$hunk}[0..$ix-1],
> +				 $newhunk,
> +				 @{$hunk}[$ix+1..$#{$hunk}])) {

I'm confused here...we are feeding _all_ of the hunks to git-apply. In
my patch, I simply fed the hunk of interest. Since we are recounting,
shouldn't the single hunk be enough? And if it isn't, then shouldn't we
be feeding only the hunks that are to be applied?

> +			my @display = color_diff(@{$text});
> +			$newhunk->{DISPLAY} = \@display;

Perl nit: you can create an arrayref with brackets:

  $newhunk->{DISPLAY} = [color_diff(@$text)];

> +		open $fh, '| git apply --cached'
> +			. ($need_recount ? ' --recount' : '');

Nice. I think this is much cleaner.

-Peff