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

From: Jeff King <peff@peff.net>
Date: Jun 12, 2008, 04:49 UTC
Message-ID: <20080612044932.GA25992@sigill.intra.peff.net>
In-Reply-To: <200806111102.31481.trast@student.ethz.ch>

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

Show 5 quoted lines

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

Show 5 quoted lines

> 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