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

From: Thomas Rast <trast@student.ethz.ch>
Date: Jul 2, 2008, 07:00 UTC
Message-ID: <200807020900.28690.trast@student.ethz.ch>
In-Reply-To: <7v7ic4hmj5.fsf@gitster.siamese.dyndns.org>

Junio C Hamano wrote:

> 
> People with minimum Perl installation should still be able to use "add -i"
> as long as they do not use 'e' subcommand, shouldn't they?  Shouldn't we
> do something like:

[...]

> and disable 'e' subcommand unless $can_use_temp?

I can just call the temporary editing file something constant, like the commit message already is. I don't care much about that point, and honestly assumed File::Temp came with every Perl (it's in perl-base here).

Show 5 quoted lines

> > +# To remove '-' lines, make them ' ' lines (context).
> > +# To remove '+' lines, delete them.
> > +# Lines starting with # will be removed.
> 
> Don't you want to say "Do not touch lines that begin with ' '"?

Why?  You can make them '-' instead if you really want to :-)

> > +	# Reinsert the first hunk header if the user accidentally deleted it
> 
> Hmm, perhaps not even giving the "@@ ... @@" lines to the editor would be
> a more robust solution?

I originally left it there because Emacs automatically recounts the header, and it also reminds the user of the function the hunk applies to.

> > +	open $fh, '| git apply --recount --cached --check';
> 
> Have to wonder where the potential error message would go, and if it would
> confuse the end users...

To the terminal. If we eat that error message, the user gets absolutely no indication as to _what_ might be wrong with the edited patch, so I kind of deliberately left it there.

Then again the message from git-apply is not exactly transparent either.

> I recall that the original "add--interactive" carefully counted numbers in
> hunks it reassembles (as it can let you split and then you can choose to
> use both parts, which requires it to merge overlapping hunks back), but if
> you are going to use --recount anyway, perhaps we can discard that logic?

We briefly talked about that[1], but Jeff thought it should be a separate patch, and I agree. Can't sneakily rip out two dozen lines of otherwise unrelated code in my feature patch, can I?

> It may make the patch application less robust, though.  I dunno.

I've become convinced it can't, apart from making it less likely to trip over bugs in the script itself of course.

> An alternative, and probably more robust, approach would be to recount
> what we have in @{$mode->{TEXT}}, after letting the user edit some of
> them, so that "add--interactive" still knows what it is doing after
> applying your patch without having to rely on "apply --recount".

You lost me here.  Why recount the mode part?

If you mean the _hunk_ headers, that's what the first three versions of this patch did, at the expense of a lot of code complication (and now that git-apply implements --recount, actually _duplication_).

- Thomas

[1] http://article.gmane.org/gmane.comp.version-control.git/84698

-- 
Thomas Rast
trast@student.ethz.ch