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

From: Jeff King <peff@peff.net>
Date: Jun 23, 2008, 18:38 UTC
Message-ID: <20080623183840.GA28887@sigill.intra.peff.net>
In-Reply-To: <200806131748.44867.trast@student.ethz.ch>

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

Show 5 quoted lines

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

Show 7 quoted lines

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

Show 6 quoted lines

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