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

[Thomas Rast <trast@student.ethz.ch>]

[-- Attachment #1: Type: text/plain, Size: 2842 bytes --]

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




[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 194 bytes --]