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

From: Thomas Rast <trast@student.ethz.ch>
Date: Jun 5, 2008, 07:53 UTC
Message-ID: <200806050954.13244.trast@student.ethz.ch>
In-Reply-To: <20080605014618.GA27381@sigill.intra.peff.net>

Jeff King wrote:

> 
> But that being said, I think your approach is much more powerful, since
> you are allowing arbitrary editing of the hunk (as much as that is
> possible).

[...]

> So perhaps manual hunk editing is simply something for advanced users
> who are comfortable with the patch format.

I'll think about this for a while. Somehow I don't like the idea of editing the actual patch _contents_ for the user, meaning that what the user needs to edit his patch into is not what we are going to apply.

On the other hand it would be just as powerful. Manually splitting a hunk is, in the general case, only possible in "my" scheme. However, to make any difference, you later have to answer 'n' to some of the sub-hunks. So in "your" scheme, you could just have deleted the lines in question.

Of course, manually editing the '+' lines or even introducing new stuff into the patch is not possible, but then you should probably have edited the actual file contents, not the patch. (The working directory will still have the old version, so if you commit after add -p, it will appear to undo part of the last commit.)

Then again, if this is just about editor convenience, maybe make a macro that toggles between '-'/'+' and ' '/'#', respectively? (Which makes me wonder if it would be useful to keep the '#' lines, minus the help message, around until the final git-apply in case you change your mind and re-edit.)

> > +# Empty lines and lines starting with # will be removed.
> 
> What about lines starting with characters besides -, +, space, or @?
> They will normally be ignored by diff.

Diff doesn't really have a say in this, does it? And looking in builtin-apply.c:

	switch (first) {  // line 1858 as of v1.5.6-rc1-122-g3859f95
	case '\n':
		/* Newer GNU diff, empty context line */
		// actual work snipped
		break;
	// cases ' ', '+', '-' also covered
	default:
		if (apply_verbosely)
			error("invalid start of line: '%c'", first);
		return -1;

so it appears invalid lines are actually not ignored, but abort hunk processing. While the error checking will be handled by apply --check, I don't think it would be a good idea to silently drop all other lines from the edit, as they probably indicate user error.

On the other hand, this also shows that dropping empty lines is wrong...

>  - Minor fixups and style comments. All of my style comments are "I
>    would have done it as..." and not "Oh God, this is horrible" so I
>    don't think any block acceptance.

From a Perl POV, they probably _are_ horrible. I'm just not used to the idioms, and tend to fall for semantic differences to Python as well.

Thank you for the very thorough review! I'll improve the patch accordingly.

- Thomas

-- 
Thomas Rast
trast@student.ethz.ch