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

[Jeff King <peff@peff.net>]

On Fri, Jun 13, 2008 at 05:48:43PM +0200, Thomas Rast wrote:

> Adds a new option 'e' to the 'add -p' command loop that lets you edit
> the current hunk in your favourite editor.
> [...]
> Applying the changed hunk(s) relies on Johannes Schindelin's new
> --recount option for git-apply.

This version looks pretty good to me (though I do have a few comments
below), and now that we are in the new release cycle, I think it is time
to re-submit "for real".

The big question is what is happening with the recount work. Johannes,
are you planning on re-submitting those patches?

> > Right, but you may get false positives if a later conflicting hunk is
> > not staged. Though as you say, I think it is unlikely in either case.
> I'd rather reject early and offer to re-edit, than notice a problem
> much later, so I left it the way it was.

Yeah, thinking about it more, that is the sanest choice.

> I was tempted to reintroduce globbed unlinking of the temporary file
> as in v3 (that is, removing TMP* instead of just TMP).  In the absence
> of it, backup files made by the user's editor will remain in .git/.
> Eventually I didn't because it seems vi doesn't make backups anyway,
> and while emacs does, enabling that for GIT_EDITOR seems a user error.
> Besides, how do we know the backups match that pattern anyway.  Not
> sure though.

I would leave it; it seems like the editor's responsibility to handle
this. We don't know what patterns are used, or when it is safe to remove
such backups. And this is far from the only place where we invoke the
editor on a temporary file, so any solution should probably be applied
globally.

> +	my @newtext = grep { !/^#/ } <$fh>;
> [...]
> +	# Abort if nothing remains
> +	if (@newtext == 0) {
> +		return undef;
> +	}

Reading over this again, I wonder if it should be:

  # Abort if nothing remains
  if (!grep { /\S/ } @newtext) {
    return undef;
  }

That is, we can't eliminate blank lines on input, since they might be
meaningful to the diff. But if the user removes every non-comment line
_except_ a blank line or a line with only whitespace, we probably want
to abort, too.

-Peff