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

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

[Thomas Rast]

Adds a new option 'e' to the 'add -p' command loop that lets you
discard or keep one hunk line at a time.  This is useful if there are
no unchanged lines in the middle of the hunk, so 's' will not work,
but you would still like to split it.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

This is my first patch, and I had to dust off my Perl knowledge a bit,
so I hope it is up to your standards.

I could have used this a few times because I frequently hack around a
bit, then after some time notice that this should really go in two
separate commits.  Usually 'add -p' can separate the changes, but if
their diff lines are immediately adjacent, one has to go back and
undo some editing before the first commit, then redo later.

I'm not quite happy with the scheme I use to handle colored diffs.
'git diff' apparently does not always reset the color before newlines
(is this a bug?), so I insert extra resets.  However, I did not want
to implement "full" diff coloring directly in git-add--interactive.

Also, I would be glad to hear your comments on the "user interface" of
the edit command loop.  I haven't found a good way of saying "an
optional number followed by y or n" that is consistent with the
"[y/n]" format used in the rest of the patch loop.  Similiarly, an
option to undo an edit might be nice, but would complicate the code a
fair bit.

Best regards,
Thomas


 Documentation/git-add.txt |    1 +
 git-add--interactive.perl |  110 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index bb4abe2..8de4d4a 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -229,6 +229,7 @@ patch::
        k - leave this hunk undecided, see previous undecided hunk
        K - leave this hunk undecided, see previous hunk
        s - split the current hunk into smaller hunks
+       e - manually edit the current hunk
        ? - print help
 +
 After deciding the fate for all hunks, if there is any hunk
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 903953e..8af841a 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -18,6 +18,10 @@ my ($fraginfo_color) =
 	$diff_use_color ? (
 		$repo->get_color('color.diff.frag', 'cyan'),
 	) : ();
+my ($diff_plain_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.plain', ''),
+	) : ();
 
 my $normal_color = $repo->get_color("", "reset");
 
@@ -682,6 +686,107 @@ sub split_hunk {
 	return @split;
 }
 
+sub edit_hunk_manually {
+	my ($in_text, $in_display) = @_;
+
+	my @text = @$in_text;
+	my @display = @$in_display;
+
+	my ($o_ofs, $o_cnt, $n_ofs, $n_cnt) = parse_hunk_header($text[0]);
+	my $num = scalar @text;
+
+	my $ix = 0;
+
+      OUTER:
+	while (1) {
+		$text[0] = ("@@ -$o_ofs" .
+			    (($o_cnt != 1) ? ",$o_cnt" : '') .
+			    " +$n_ofs" .
+			    (($n_cnt != 1) ? ",$n_cnt" : '') .
+			    " @@\n");
+		if ($diff_use_color) {
+			$display[0] = colored $fraginfo_color, $text[0];
+		}
+		else {
+			$display[0] = $text[0];
+		}
+
+		while ($text[$ix] !~ /^[+-]/) {
+			$ix++;
+			last OUTER if $ix >= $num;
+		}
+		my $lineno = 0;
+		for (my $i = 0; $i < $num; $i++) {
+			if ($i >= $ix && $text[$i] =~ /^[+-]/) {
+				$lineno++;
+				if ($lineno == 1) {
+					print $normal_color . colored($prompt_color, ">1 ");
+				}
+				elsif ($lineno <  100) {
+					print $normal_color . colored($prompt_color, sprintf("%2d ", $lineno));
+				}
+				else {
+					print "   ";
+				}
+			}
+			else {
+				print "   ";
+			}
+			print $display[$i];
+		}
+		print colored $prompt_color, "Use line(s) [<num>y/n]? ";
+		my $line = <STDIN>;
+		if ($line) {
+			if ($line =~ /^(\d*)y/) {
+				my $repeat = $1 || 1;
+				while (1) {
+					last if ($repeat <= 0 || $ix >= $num);
+					$repeat-- if ($text[$ix] =~ /^[+-]/);
+					$ix++;
+				}
+			}
+			elsif ($line =~ /^(\d*)n/) {
+				# This is the interesting case.
+				# - lines become context, + lines are dropped
+				my $repeat = $1 || 1;
+				while (1) {
+					last if ($repeat <= 0 || $ix >= $num);
+					if ($text[$ix] =~ /^\+/) {
+						$repeat--;
+						splice(@text, $ix, 1);
+						splice(@display, $ix, 1);
+						$n_cnt--;
+						$num--;
+						$ix--;
+					}
+					elsif ($text[$ix] =~ /^-/) {
+						$repeat--;
+						$n_cnt++;
+						$text[$ix] =~ s/^-/ /;
+						# need a better way to do this
+						$display[$ix] = $normal_color . colored $diff_plain_color, $text[$ix];
+					}
+					$ix++;
+				}
+			}
+		}
+	}
+
+	while (1) {
+		for (@display) {
+			print;
+		}
+		print colored $prompt_color, "Accept and replace old hunk [y/n]? ";
+		my $line = <STDIN>;
+		if ($line =~ /^y/) {
+			return (\@text, \@display);
+		}
+		elsif ($line =~ /^n/) {
+			return ($in_text, $in_display);
+		}
+	}
+}
+
 sub find_last_o_ctx {
 	my ($it) = @_;
 	my $text = $it->{TEXT};
@@ -781,6 +886,7 @@ J - leave this hunk undecided, see next hunk
 k - leave this hunk undecided, see previous undecided hunk
 K - leave this hunk undecided, see previous hunk
 s - split the current hunk into smaller hunks
+e - manually edit the current hunk
 ? - print help
 EOF
 }
@@ -885,6 +991,7 @@ sub patch_update_file {
 		if (hunk_splittable($hunk[$ix]{TEXT})) {
 			$other .= '/s';
 		}
+		$other .= '/e';
 		for (@{$hunk[$ix]{DISPLAY}}) {
 			print;
 		}
@@ -949,6 +1056,9 @@ sub patch_update_file {
 				$num = scalar @hunk;
 				next;
 			}
+			elsif ($line =~ /^e/) {
+				($hunk[$ix]{TEXT}, $hunk[$ix]{DISPLAY}) = edit_hunk_manually($hunk[$ix]{TEXT}, $hunk[$ix]{DISPLAY});
+			}
 			else {
 				help_patch_cmd($other);
 				next;
-- 
1.5.4.5

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

[Ping Yin]

* Thomas Rast <trast@student.ethz.ch> [2008-05-23 22:21:43 +0200]:

> Adds a new option 'e' to the 'add -p' command loop that lets you
> discard or keep one hunk line at a time.  This is useful if there are
> no unchanged lines in the middle of the hunk, so 's' will not work,
> but you would still like to split it.
> 
Wow, nice feature, although i havn't tried.

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

[Thomas Rast]

You wrote:
> Adds a new option 'e' to the 'add -p' command loop that lets you
> discard or keep one hunk line at a time.  This is useful if there are
> no unchanged lines in the middle of the hunk, so 's' will not work,
> but you would still like to split it.

Any news on this?  I would greatly appreciate criticism if something
is wrong or inadequate :-)

Thanks,
Thomas

-- 
Thomas Rast
trast@student.ethz.ch

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

[Johannes Schindelin]

Hi,

On Thu, 29 May 2008, Thomas Rast wrote:

> You wrote:

Who "You"?  You did not say a name.

> > Adds a new option 'e' to the 'add -p' command loop that lets you 
> > discard or keep one hunk line at a time.  This is useful if there are 
> > no unchanged lines in the middle of the hunk, so 's' will not work, 
> > but you would still like to split it.
> 
> Any news on this?  I would greatly appreciate criticism if something is 
> wrong or inadequate :-)

Oh, probably was you, yourself.  Hrm.  Could have said that.

The splitting (even without common lines at the borders) is something I 
needed myself, but the concept got rebuked in

http://thread.gmane.org/gmane.comp.version-control.git/67854/focus=68108

(See the whole thread for more information)

Ciao,
Dscho

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

[Jeff King]

On Thu, May 29, 2008 at 05:37:51PM +0200, Thomas Rast wrote:

> You wrote:
> > Adds a new option 'e' to the 'add -p' command loop that lets you
> > discard or keep one hunk line at a time.  This is useful if there are
> > no unchanged lines in the middle of the hunk, so 's' will not work,
> > but you would still like to split it.
> 
> Any news on this?  I would greatly appreciate criticism if something
> is wrong or inadequate :-)

[please just repost the patch when issuing such gentle reminders -- it
saves the rest of us from digging it out of the archive]

I have often wanted to perform this operation, and your implementation
seems to work from my minimal testing (though I haven't looked closely
at the colorization implications).

But I find the interface a bit clunky. I would much rather get dumped in
my favorite editor, which happens to be quite fast at removing a subset
of lines. After editing, any lines remaining would be staged.

We would have to figure out what happens if lines are added or edited,
of course. It may be right to signal an error, or maybe there is some
other useful functionality that can come of that. I think other systems
have some diff-editing functionality (IIRC, cogito did). It is probably
worth looking at that for ideas.

-Peff

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

[Thomas Rast]

Johannes Schindelin wrote:
> Hi,
> 
> On Thu, 29 May 2008, Thomas Rast wrote:
> 
> > You wrote:
> 
> Who "You"?  You did not say a name.
> 
> > > Adds a new option 'e' to the 'add -p' command loop that lets you 
> > > discard or keep one hunk line at a time.  This is useful if there are 
> > > no unchanged lines in the middle of the hunk, so 's' will not work, 
> > > but you would still like to split it.
> > 
> 
> The splitting (even without common lines at the borders) is something I 
> needed myself, but the concept got rebuked in
> 
> http://thread.gmane.org/gmane.comp.version-control.git/67854/focus=68108

Aha, thanks for the pointer.  I'm not sure I'm in a position to
restart that discussion, but let's try anyway:

Note that what Junio said in

http://thread.gmane.org/gmane.comp.version-control.git/67854/focus=68108
> That is, , if you want to do finer grained hunk splitting than what "git
> add -p" lets you do, you do _not_ let user specify "I want to split the
> hunk into two, before this point and after this point".  Instead, let
> the user pick zero or more '-' line and zero or more '+' line, and
> adjust the context around it.  An unpicked '-' line becomes the common
> context, and an unpicked '+' line disappears.  After that, you recount
> the diff.  That way, you do not have to do any "unidiff zero" cop-out.

is precisely what my patch implements.  I did not implement the
stashing he recommends in the same message, but I believe the fact
that 'add -p' stages the end result should be enough (and works at
least for my use case).

Johannes Sixt replied:

http://thread.gmane.org/gmane.comp.version-control.git/67854/focus=68108
> In this case I would expect two adjacent hunks: one that covers the selected
> changes, another one with the remaining changes, but each against the original:

I didn't do this because there is no way to ensure that the "other
half" can be applied cleanly to the unpatched state, thus requiring
extra tracking of "hunk dependencies", so to speak.  It could be done
of course, if anyone really needs it.

You also mention

http://thread.gmane.org/gmane.comp.version-control.git/67854/focus=68108
> I thought about [the line-wise picking], but the UI is not trivial.

The y/n loop with count prefix works well for me.  It would be nice to
have it in an actual GUI of course, but I don't know enough Tcl/Tk to
implement that.

- Thomas

-- 
Thomas Rast
trast@student.ethz.ch

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

[Thomas Rast]

Thomas Rast wrote:
> > http://thread.gmane.org/gmane.comp.version-control.git/67854/focus=68108
> http://thread.gmane.org/gmane.comp.version-control.git/67854/focus=68108
> http://thread.gmane.org/gmane.comp.version-control.git/67854/focus=68108
> http://thread.gmane.org/gmane.comp.version-control.git/67854/focus=68108

Bah, obviously I failed to notice that the links are all the same.

The correct links:

Junio's remarks about +/- picking:
  http://article.gmane.org/gmane.comp.version-control.git/68131
Johannes Sixt's reply:
  http://article.gmane.org/gmane.comp.version-control.git/68138
Johannes Schindelin's reply:
  http://article.gmane.org/gmane.comp.version-control.git/68143

Sorry for the spam.

- Thomas

-- 
Thomas Rast
trast@student.ethz.ch

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

[Johannes Schindelin]

Hi,

On Thu, 29 May 2008, Jeff King wrote:

> I would much rather get dumped in my favorite editor, which happens to 
> be quite fast at removing a subset of lines. After editing, any lines 
> remaining would be staged.
> 
> We would have to figure out what happens if lines are added or edited,
> of course. It may be right to signal an error, or maybe there is some
> other useful functionality that can come of that. I think other systems
> have some diff-editing functionality (IIRC, cogito did). It is probably
> worth looking at that for ideas.

Maybe you want to start from this patch (only compile-tested):

-- snipsnap --
[PATCH] WIP: allow git-apply to fix up the line counts

Sometimes, the easiest way to fix up a patch is to edit it directly, even
adding or deleting lines.  Now, many people are not as divine as certain
benevolent dictators as to update the hunk headers correctly at the first
try.

So teach the tool to do it for us.
---
 Documentation/git-apply.txt |    6 +++-
 builtin-apply.c             |   66 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 64 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index 2dec2ec..8007d10 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -12,7 +12,7 @@ SYNOPSIS
 'git-apply' [--stat] [--numstat] [--summary] [--check] [--index]
 	  [--apply] [--no-add] [--build-fake-ancestor <file>] [-R | --reverse]
 	  [--allow-binary-replacement | --binary] [--reject] [-z]
-	  [-pNUM] [-CNUM] [--inaccurate-eof] [--cached]
+	  [-pNUM] [-CNUM] [--inaccurate-eof] [--fixup] [--cached]
 	  [--whitespace=<nowarn|warn|fix|error|error-all>]
 	  [--exclude=PATH] [--verbose] [<patch>...]
 
@@ -169,6 +169,10 @@ behavior:
 	correctly. This option adds support for applying such patches by
 	working around this bug.
 
+--fixup-line-counts::
+	Fix up the line counts (e.g. after editing the patch without
+	adjusting the hunk headers appropriately).
+
 -v, --verbose::
 	Report progress to stderr. By default, only a message about the
 	current patch being applied will be printed. This option will cause
diff --git a/builtin-apply.c b/builtin-apply.c
index c497889..c8f1cd9 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -153,6 +153,7 @@ struct patch {
 	unsigned int is_binary:1;
 	unsigned int is_copy:1;
 	unsigned int is_rename:1;
+	unsigned int fixup:1;
 	struct fragment *fragments;
 	char *result;
 	size_t resultsize;
@@ -882,11 +883,49 @@ static int parse_range(const char *line, int len, int offset, const char *expect
 	return offset + ex;
 }
 
+static int fixup_counts(char *line, int len, struct fragment *fragment)
+{
+
+	if (len < 1)
+		return -1;
+
+	fragment->oldlines = fragment->newlines = 0;
+
+	for (;;) {
+		char *newline = memchr(line, '\n', len);
+		if (!newline)
+			break;
+
+		switch (*line) {
+		case ' ':
+			fragment->oldlines++;
+			/* fall through */
+		case '+':
+			fragment->newlines++;
+			break;
+		case '-':
+			fragment->oldlines++;
+			break;
+		default:
+			return error("Unexpected first character: %c in line "
+				"'%.*s'",
+				*line, newline ? newline - line : len, line);
+		}
+
+		len -= newline + 1 - line;
+		line = newline + 1;
+		if (len < 2 || !prefixcmp(line, "@@"))
+			break;
+	}
+	return 0;
+}
+
 /*
  * Parse a unified diff fragment header of the
  * form "@@ -a,b +c,d @@"
  */
-static int parse_fragment_header(char *line, int len, struct fragment *fragment)
+static int parse_fragment_header(char *line, int len, struct fragment *fragment,
+	int fixup)
 {
 	int offset;
 
@@ -897,6 +936,10 @@ static int parse_fragment_header(char *line, int len, struct fragment *fragment)
 	offset = parse_range(line, len, 4, " +", &fragment->oldpos, &fragment->oldlines);
 	offset = parse_range(line, len, offset, " @@", &fragment->newpos, &fragment->newlines);
 
+	if (offset > 0 && fixup &&
+			fixup_counts(line + offset, len - offset, fragment))
+		return -1;
+
 	return offset;
 }
 
@@ -927,7 +970,8 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc
 		 */
 		if (!memcmp("@@ -", line, 4)) {
 			struct fragment dummy;
-			if (parse_fragment_header(line, len, &dummy) < 0)
+			if (parse_fragment_header(line, len, &dummy,
+					patch->fixup) < 0)
 				continue;
 			die("patch fragment without header at line %d: %.*s",
 			    linenr, (int)len-1, line);
@@ -1010,7 +1054,7 @@ static int parse_fragment(char *line, unsigned long size,
 	unsigned long oldlines, newlines;
 	unsigned long leading, trailing;
 
-	offset = parse_fragment_header(line, len, fragment);
+	offset = parse_fragment_header(line, len, fragment, patch->fixup);
 	if (offset < 0)
 		return -1;
 	oldlines = fragment->oldlines;
@@ -2912,7 +2956,8 @@ static void prefix_patches(struct patch *p)
 	}
 }
 
-static int apply_patch(int fd, const char *filename, int inaccurate_eof)
+static int apply_patch(int fd, const char *filename, int inaccurate_eof,
+		int fixup)
 {
 	size_t offset;
 	struct strbuf buf;
@@ -2929,6 +2974,7 @@ static int apply_patch(int fd, const char *filename, int inaccurate_eof)
 
 		patch = xcalloc(1, sizeof(*patch));
 		patch->inaccurate_eof = inaccurate_eof;
+		patch->fixup = fixup;
 		nr = parse_chunk(buf.buf + offset, buf.len - offset, patch);
 		if (nr < 0)
 			break;
@@ -2998,6 +3044,7 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 	int i;
 	int read_stdin = 1;
 	int inaccurate_eof = 0;
+	int fixup = 0;
 	int errs = 0;
 	int is_not_gitdir;
 
@@ -3015,7 +3062,8 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 		int fd;
 
 		if (!strcmp(arg, "-")) {
-			errs |= apply_patch(0, "<stdin>", inaccurate_eof);
+			errs |= apply_patch(0, "<stdin>", inaccurate_eof,
+					fixup);
 			read_stdin = 0;
 			continue;
 		}
@@ -3118,6 +3166,10 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 			inaccurate_eof = 1;
 			continue;
 		}
+		if (!strcmp(arg, "--fixup-line-counts")) {
+			fixup = 1;
+			continue;
+		}
 		if (0 < prefix_length)
 			arg = prefix_filename(prefix, prefix_length, arg);
 
@@ -3126,12 +3178,12 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 			die("can't open patch '%s': %s", arg, strerror(errno));
 		read_stdin = 0;
 		set_default_whitespace_mode(whitespace_option);
-		errs |= apply_patch(fd, arg, inaccurate_eof);
+		errs |= apply_patch(fd, arg, inaccurate_eof, fixup);
 		close(fd);
 	}
 	set_default_whitespace_mode(whitespace_option);
 	if (read_stdin)
-		errs |= apply_patch(0, "<stdin>", inaccurate_eof);
+		errs |= apply_patch(0, "<stdin>", inaccurate_eof, fixup);
 	if (whitespace_error) {
 		if (squelch_whitespace_errors &&
 		    squelch_whitespace_errors < whitespace_error) {
-- 
1.5.6.rc0.206.g61199

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

[Jakub Narebski]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> -- snipsnap --
> [PATCH] WIP: allow git-apply to fix up the line counts
> 
> Sometimes, the easiest way to fix up a patch is to edit it directly, even
> adding or deleting lines.  Now, many people are not as divine as certain
> benevolent dictators as to update the hunk headers correctly at the first
> try.

Or do not use editor with diff editing mode, such as unidiff mode for
GNU Emacs (and derivatives)... :-)
-- 
Jakub Narebski
Poland
ShadeHawk on #git

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

[Thomas Rast]

Jeff King wrote:
> But I find the interface a bit clunky. I would much rather get dumped in
> my favorite editor, which happens to be quite fast at removing a subset
> of lines. After editing, any lines remaining would be staged.
> 
> We would have to figure out what happens if lines are added or edited,
> of course. It may be right to signal an error, or maybe there is some
> other useful functionality that can come of that. I think other systems
> have some diff-editing functionality (IIRC, cogito did). It is probably
> worth looking at that for ideas.

We could just see if the hunk applies to the unchanged index (still
assuming we're inside 'add -p'), and if not, reject the edit.

Unfortunately git-apply does not seem to have a --dry-run option.
(Even stranger, when given the option --dry-run it tries to open a
patch of that name.)  What is the recommended way to do such things?
Make a backup copy of the index and apply --cached anyway?

- Thomas

-- 
Thomas Rast
trast@student.ethz.ch

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

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> Unfortunately git-apply does not seem to have a --dry-run option.

--check?

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

[Thomas Rast]

Adds a new option 'e' to the 'add -p' command loop that lets you
edit the current hunk in your favourite editor.
---

This is a draft of Jeff King's idea

Jeff King wrote:
> But I find the interface a bit clunky. I would much rather get dumped in
> my favorite editor, which happens to be quite fast at removing a subset
> of lines. After editing, any lines remaining would be staged.
>
> We would have to figure out what happens if lines are added or edited,
> of course. It may be right to signal an error, or maybe there is some
> other useful functionality that can come of that.

in the hope that I might get some more ideas and pointers on the details.

The current implementation rejects edits that break the (whole) patch
(thanks Junio for pointing out --check...), but it does help the user
in one aspect: @ lines are edited to reflect their hunk contents, even
inferring the starting line numbers from the last hunk if they are
missing.  This means you can insert a line consisting just of an @,
and it will silently be fixed to start a new hunk at that point.

Some things that might need improvement/fixing:

- Does anyone need a way to force a broken diff past the git-apply
  check?  I don't know what use such a diff might have, but who knows,
  perhaps it applies cleanly once you exclude a hunk or two...

- Perhaps the instructions should go to the bottom, but then the odds
  are they would not be visible in many cases.

- Should I try to come up with a unique filename to support concurrent
  edits?  (git-commit doesn't...)

- Perl's autovivification is out to get me.  I've fixed a few, but
  there are probably still bugs.

Thomas

---
 git-add--interactive.perl |  171 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 171 insertions(+), 0 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 903953e..c752e20 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -18,6 +18,18 @@ my ($fraginfo_color) =
 	$diff_use_color ? (
 		$repo->get_color('color.diff.frag', 'cyan'),
 	) : ();
+my ($diff_plain_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.plain', ''),
+	) : ();
+my ($diff_old_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.old', 'red'),
+	) : ();
+my ($diff_new_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.new', 'green'),
+	) : ();
 
 my $normal_color = $repo->get_color("", "reset");
 
@@ -770,6 +782,158 @@ sub coalesce_overlapping_hunks {
 	return @out;
 }
 
+sub edit_hunk_manually {
+	my @oldtext;
+	for (@_) {
+		push @oldtext, @{$_->{TEXT}};
+	}
+
+	# use a .diff file to help editors with highlighting
+	my $editpath = $repo->repo_path() . "/ADDP_HUNK_EDIT.diff";
+	my $fh;
+	open $fh, '>', $editpath
+		or die "failed to open hunk edit file for writing: " . $!;
+	print $fh <<EOF;
+# MANUAL HUNK EDIT MODE
+#
+# You can change the hunk to your heart's content, but it will be
+# refused if the end result (the entire patch including your edited
+# hunk) does not apply cleanly.
+#
+# To remove '-' lines, make them ' ' lines (context).
+# To remove '+' lines, delete them.
+# Empty lines and lines starting with # will be removed.
+#
+# Lines starting with @ start a new hunk. Line counts will be adjusted
+# according to contents. If the line numbers are missing altogether,
+# they will be inferred from the previous hunk.
+EOF
+	print $fh @oldtext;
+	close $fh;
+
+	my $editor = $ENV{GIT_EDITOR} || $repo->config("core.editor")
+		|| $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+	system('sh', '-c', $editor.' "$@"', $editor, $editpath);
+
+	open $fh, '<', $editpath
+		or die "failed to open hunk edit file for reading: " . $!;
+	my @newtext;
+	while (<$fh>) {
+		push (@newtext, $_) unless /^#/ || /^$/;
+	}
+	close $fh;
+	my @heads = ();
+	my ($o_ofs, $n_ofs);
+	my $o_cnt = 0;
+	my $n_cnt = 0;
+	my ($guess_o_ofs, undef, $guess_n_ofs, undef) = parse_hunk_header($oldtext[0]);
+	for (my $i = 0; $i < @newtext; $i++) {
+		if ((scalar @heads) == 0 && $newtext[$i] =~ /^[ +-]/) {
+			splice @newtext, $i, 0, $oldtext[0];
+			push @heads, $i;
+		}
+		elsif ($newtext[$i] =~ /^ /) {
+			$o_cnt++;
+			$n_cnt++;
+		}
+		elsif ($newtext[$i] =~ /^-/) {
+			$o_cnt++;
+		}
+		elsif ($newtext[$i] =~ /^\+/) {
+			$n_cnt++;
+		}
+		elsif ($newtext[$i] =~ /^@/) {
+			if (@heads > 0) {
+				# fix up the previous header first
+				($o_ofs, undef, $n_ofs, undef)
+					= parse_hunk_header($newtext[$heads[-1]]);
+				$o_ofs = $guess_o_ofs unless defined $o_ofs;
+				$n_ofs = $guess_n_ofs unless defined $n_ofs;
+				$newtext[$heads[-1]] = (
+					"@@ -$o_ofs" . (($o_cnt != 1) ? ",$o_cnt" : '')
+					. " +$n_ofs" . (($n_cnt != 1) ? ",$n_cnt" : '')
+					. " @@\n");
+				$guess_o_ofs = $o_ofs + $o_cnt;
+				$guess_n_ofs = $n_ofs + $n_cnt;
+			}
+			$o_cnt = 0;
+			$n_cnt = 0;
+			push @heads, $i;
+		}
+	}
+	($o_ofs, undef, $n_ofs, undef)
+		= parse_hunk_header($newtext[$heads[-1]]);
+	$o_ofs = $guess_o_ofs unless defined $o_ofs;
+	$n_ofs = $guess_n_ofs unless defined $n_ofs;
+	$newtext[$heads[-1]] = (
+		"@@ -$o_ofs" . (($o_cnt != 1) ? ",$o_cnt" : '')
+		. " +$n_ofs" . (($n_cnt != 1) ? ",$n_cnt" : '')
+		. " @@\n");
+
+	push @heads, (scalar @newtext);
+	my (@hunks) = ();
+	for (my $i = 0; $i < @heads-1; $i++) {
+		my @hunktext = @newtext[$heads[$i]..$heads[$i+1]-1];
+		my @hunkdisplay = ();
+		for (@hunktext) {
+			if (/^@/) {
+				push @hunkdisplay, (colored $fraginfo_color, $_);
+			}
+			elsif (/^\+/) {
+				push @hunkdisplay, (colored $diff_new_color, $_);
+			}
+			elsif (/^-/) {
+				push @hunkdisplay, (colored $diff_old_color, $_);
+			}
+			else {
+				push @hunkdisplay, (colored $diff_plain_color, $_);
+			}
+		}
+		push @hunks, {TEXT => \@hunktext, DISPLAY => \@hunkdisplay};
+	}
+
+	return @hunks;
+}
+
+sub edit_hunk_loop {
+	my ($head, $hunks, $ix) = @_;
+
+	my @newhunks = ($hunks->[$ix]);
+
+      EDIT:
+	while (1) {
+		@newhunks = edit_hunk_manually(@newhunks);
+		my $fh;
+		open $fh, '| git apply --cached --check';
+		for my $h ($head,
+			   $ix > 0 ? @$hunks[0..$ix-1] : (),
+			   @newhunks,
+			   $ix < (scalar @$hunks)-2 ? @$hunks[$ix+1..@$hunks] : ()) {
+			for (@{$h->{TEXT}}) {
+				print $fh $_;
+			}
+		}
+		if (!close $fh) {
+			# didn't apply cleanly
+			while (1) {
+				print colored $prompt_color, "Your edited hunk does not apply. Edit again (saying \"no\" discards!) [y/n]? ";
+				my $line = <STDIN>;
+				if ($line =~ /^y/) {
+					redo EDIT;
+				}
+				elsif ($line =~ /^n/) {
+					return $hunks->[$ix];
+				}
+			}
+		}
+		if (1 < @newhunks) {
+			print colored $header_color, "Manually edited into ",
+			scalar(@newhunks), " hunks.\n";
+		}
+		return @newhunks;
+	}
+}
+
 sub help_patch_cmd {
 	print colored $help_color, <<\EOF ;
 y - stage this hunk
@@ -781,6 +945,7 @@ J - leave this hunk undecided, see next hunk
 k - leave this hunk undecided, see previous undecided hunk
 K - leave this hunk undecided, see previous hunk
 s - split the current hunk into smaller hunks
+e - manually edit the current hunk
 ? - print help
 EOF
 }
@@ -885,6 +1050,7 @@ sub patch_update_file {
 		if (hunk_splittable($hunk[$ix]{TEXT})) {
 			$other .= '/s';
 		}
+		$other .= '/e';
 		for (@{$hunk[$ix]{DISPLAY}}) {
 			print;
 		}
@@ -949,6 +1115,11 @@ sub patch_update_file {
 				$num = scalar @hunk;
 				next;
 			}
+			elsif ($line =~ /^e/) {
+				splice @hunk, $ix, 1, edit_hunk_loop($head, \@hunk, $ix);
+				$num = scalar @hunk;
+				next;
+			}
 			else {
 				help_patch_cmd($other);
 				next;
-- 
1.5.6.rc0.159.g710c6

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

[Thomas Rast]

Adds a new option 'e' to the 'add -p' command loop that lets you
edit the current hunk in your favourite editor.
---

Thomas Rast wrote:
>
> - Perl's autovivification is out to get me.  I've fixed a few, but
>   there are probably still bugs.

I also tripped over Perl's idea of array slicing.  This fixes an
off-by-one in the hunks forwared to apply --check.  The diff between
this and the previous patch is

# @@ -908,7 +908,7 @@ sub edit_hunk_loop {
 		for my $h ($head,
 			   $ix > 0 ? @$hunks[0..$ix-1] : (),
 			   @newhunks,
-			   $ix < (scalar @$hunks)-2 ? @$hunks[$ix+1..@$hunks] : ()) {
+			   $ix < (scalar @$hunks)-1 ? @$hunks[$ix+1..@$hunks-1] : ()) {
 			for (@{$h->{TEXT}}) {
 				print $fh $_;
 			}

As a side note, what's the "right" way to deal with this situation of
patch improvements?  I made a normal commit chain on a side branch to
keep the history, but format-patch wants to make that into two mails,
so I had to rebuild the message format from diff -p --stat.

Regards
Thomas


 git-add--interactive.perl |  171 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 171 insertions(+), 0 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 903953e..a3b2c75 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -18,6 +18,18 @@ my ($fraginfo_color) =
 	$diff_use_color ? (
 		$repo->get_color('color.diff.frag', 'cyan'),
 	) : ();
+my ($diff_plain_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.plain', ''),
+	) : ();
+my ($diff_old_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.old', 'red'),
+	) : ();
+my ($diff_new_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.new', 'green'),
+	) : ();
 
 my $normal_color = $repo->get_color("", "reset");
 
@@ -770,6 +782,158 @@ sub coalesce_overlapping_hunks {
 	return @out;
 }
 
+sub edit_hunk_manually {
+	my @oldtext;
+	for (@_) {
+		push @oldtext, @{$_->{TEXT}};
+	}
+
+	# use a .diff file to help editors with highlighting
+	my $editpath = $repo->repo_path() . "/ADDP_HUNK_EDIT.diff";
+	my $fh;
+	open $fh, '>', $editpath
+		or die "failed to open hunk edit file for writing: " . $!;
+	print $fh <<EOF;
+# MANUAL HUNK EDIT MODE
+#
+# You can change the hunk to your heart's content, but it will be
+# refused if the end result (the entire patch including your edited
+# hunk) does not apply cleanly.
+#
+# To remove '-' lines, make them ' ' lines (context).
+# To remove '+' lines, delete them.
+# Empty lines and lines starting with # will be removed.
+#
+# Lines starting with @ start a new hunk. Line counts will be adjusted
+# according to contents. If the line numbers are missing altogether,
+# they will be inferred from the previous hunk.
+EOF
+	print $fh @oldtext;
+	close $fh;
+
+	my $editor = $ENV{GIT_EDITOR} || $repo->config("core.editor")
+		|| $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+	system('sh', '-c', $editor.' "$@"', $editor, $editpath);
+
+	open $fh, '<', $editpath
+		or die "failed to open hunk edit file for reading: " . $!;
+	my @newtext;
+	while (<$fh>) {
+		push (@newtext, $_) unless /^#/ || /^$/;
+	}
+	close $fh;
+	my @heads = ();
+	my ($o_ofs, $n_ofs);
+	my $o_cnt = 0;
+	my $n_cnt = 0;
+	my ($guess_o_ofs, undef, $guess_n_ofs, undef) = parse_hunk_header($oldtext[0]);
+	for (my $i = 0; $i < @newtext; $i++) {
+		if ((scalar @heads) == 0 && $newtext[$i] =~ /^[ +-]/) {
+			splice @newtext, $i, 0, $oldtext[0];
+			push @heads, $i;
+		}
+		elsif ($newtext[$i] =~ /^ /) {
+			$o_cnt++;
+			$n_cnt++;
+		}
+		elsif ($newtext[$i] =~ /^-/) {
+			$o_cnt++;
+		}
+		elsif ($newtext[$i] =~ /^\+/) {
+			$n_cnt++;
+		}
+		elsif ($newtext[$i] =~ /^@/) {
+			if (@heads > 0) {
+				# fix up the previous header first
+				($o_ofs, undef, $n_ofs, undef)
+					= parse_hunk_header($newtext[$heads[-1]]);
+				$o_ofs = $guess_o_ofs unless defined $o_ofs;
+				$n_ofs = $guess_n_ofs unless defined $n_ofs;
+				$newtext[$heads[-1]] = (
+					"@@ -$o_ofs" . (($o_cnt != 1) ? ",$o_cnt" : '')
+					. " +$n_ofs" . (($n_cnt != 1) ? ",$n_cnt" : '')
+					. " @@\n");
+				$guess_o_ofs = $o_ofs + $o_cnt;
+				$guess_n_ofs = $n_ofs + $n_cnt;
+			}
+			$o_cnt = 0;
+			$n_cnt = 0;
+			push @heads, $i;
+		}
+	}
+	($o_ofs, undef, $n_ofs, undef)
+		= parse_hunk_header($newtext[$heads[-1]]);
+	$o_ofs = $guess_o_ofs unless defined $o_ofs;
+	$n_ofs = $guess_n_ofs unless defined $n_ofs;
+	$newtext[$heads[-1]] = (
+		"@@ -$o_ofs" . (($o_cnt != 1) ? ",$o_cnt" : '')
+		. " +$n_ofs" . (($n_cnt != 1) ? ",$n_cnt" : '')
+		. " @@\n");
+
+	push @heads, (scalar @newtext);
+	my (@hunks) = ();
+	for (my $i = 0; $i < @heads-1; $i++) {
+		my @hunktext = @newtext[$heads[$i]..$heads[$i+1]-1];
+		my @hunkdisplay = ();
+		for (@hunktext) {
+			if (/^@/) {
+				push @hunkdisplay, (colored $fraginfo_color, $_);
+			}
+			elsif (/^\+/) {
+				push @hunkdisplay, (colored $diff_new_color, $_);
+			}
+			elsif (/^-/) {
+				push @hunkdisplay, (colored $diff_old_color, $_);
+			}
+			else {
+				push @hunkdisplay, (colored $diff_plain_color, $_);
+			}
+		}
+		push @hunks, {TEXT => \@hunktext, DISPLAY => \@hunkdisplay};
+	}
+
+	return @hunks;
+}
+
+sub edit_hunk_loop {
+	my ($head, $hunks, $ix) = @_;
+
+	my @newhunks = ($hunks->[$ix]);
+
+      EDIT:
+	while (1) {
+		@newhunks = edit_hunk_manually(@newhunks);
+		my $fh;
+		open $fh, '| git apply --cached --check';
+		for my $h ($head,
+			   $ix > 0 ? @$hunks[0..$ix-1] : (),
+			   @newhunks,
+			   $ix < (scalar @$hunks)-1 ? @$hunks[$ix+1..@$hunks-1] : ()) {
+			for (@{$h->{TEXT}}) {
+				print $fh $_;
+			}
+		}
+		if (!close $fh) {
+			# didn't apply cleanly
+			while (1) {
+				print colored $prompt_color, "Your edited hunk does not apply. Edit again (saying \"no\" discards!) [y/n]? ";
+				my $line = <STDIN>;
+				if ($line =~ /^y/) {
+					redo EDIT;
+				}
+				elsif ($line =~ /^n/) {
+					return $hunks->[$ix];
+				}
+			}
+		}
+		if (1 < @newhunks) {
+			print colored $header_color, "Manually edited into ",
+			scalar(@newhunks), " hunks.\n";
+		}
+		return @newhunks;
+	}
+}
+
 sub help_patch_cmd {
 	print colored $help_color, <<\EOF ;
 y - stage this hunk
@@ -781,6 +945,7 @@ J - leave this hunk undecided, see next hunk
 k - leave this hunk undecided, see previous undecided hunk
 K - leave this hunk undecided, see previous hunk
 s - split the current hunk into smaller hunks
+e - manually edit the current hunk
 ? - print help
 EOF
 }
@@ -885,6 +1050,7 @@ sub patch_update_file {
 		if (hunk_splittable($hunk[$ix]{TEXT})) {
 			$other .= '/s';
 		}
+		$other .= '/e';
 		for (@{$hunk[$ix]{DISPLAY}}) {
 			print;
 		}
@@ -949,6 +1115,11 @@ sub patch_update_file {
 				$num = scalar @hunk;
 				next;
 			}
+			elsif ($line =~ /^e/) {
+				splice @hunk, $ix, 1, edit_hunk_loop($head, \@hunk, $ix);
+				$num = scalar @hunk;
+				next;
+			}
 			else {
 				help_patch_cmd($other);
 				next;
-- 
1.5.6.rc0.159.g710c6.dirty

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

[Jeff King]

On Sun, Jun 01, 2008 at 04:50:00PM +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.

I haven't had time to review your patch yet, but I'll hopefully get to
it in the next day or so. In the meantime, a quick comment on this
change:

> -			   $ix < (scalar @$hunks)-2 ? @$hunks[$ix+1..@$hunks] : ()) {
> +			   $ix < (scalar @$hunks)-1 ? @$hunks[$ix+1..@$hunks-1] : ()) {

An easier and less error prone way to write "@$hunks-1" is "$#{$hunks}".

> As a side note, what's the "right" way to deal with this situation of
> patch improvements?  I made a normal commit chain on a side branch to
> keep the history, but format-patch wants to make that into two mails,
> so I had to rebuild the message format from diff -p --stat.

Usually you would squash your changes together into a single commit
locally using "commit --amend" or "rebase -i".

-Peff

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

[Jeff King]

On Sun, Jun 01, 2008 at 02:41:48AM +0200, Thomas Rast wrote:

> The current implementation rejects edits that break the (whole) patch
> (thanks Junio for pointing out --check...), but it does help the user
> in one aspect: @ lines are edited to reflect their hunk contents, even
> inferring the starting line numbers from the last hunk if they are
> missing.  This means you can insert a line consisting just of an @,
> and it will silently be fixed to start a new hunk at that point.

Overall, I think this patch is reasonable. I have a few complaints, but
I'm not sure that there are better alternatives. See below.

> - Perhaps the instructions should go to the bottom, but then the odds
>   are they would not be visible in many cases.

They are at the bottom in "rebase -i", and I think there was much
discussion that led to that.

> - Should I try to come up with a unique filename to support concurrent
>   edits?  (git-commit doesn't...)

I think it makes sense to; there isn't really a downside.

> +my ($diff_plain_color) =
> +	$diff_use_color ? (
> +		$repo->get_color('color.diff.plain', ''),
> +	) : ();
> +my ($diff_old_color) =
> +	$diff_use_color ? (
> +		$repo->get_color('color.diff.old', 'red'),
> +	) : ();
> +my ($diff_new_color) =
> +	$diff_use_color ? (
> +		$repo->get_color('color.diff.new', 'green'),
> +	) : ();

Yuck. I was hoping we could avoid doing anything with diff colors here.
I had envisioned this as the user editing the hunk, and we would just
match up the lines. That is, they would only be deleting lines, and we
would simply read back each line, comparing against the original hunk.
Any lines which were removed by the user would become no-ops in the
hunk.

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). And in that case, I think the re-coloring is necessary, since
we can't just match the lines up to their previously colored versions
anymore.

> +sub edit_hunk_manually {
> +	my @oldtext;
> +	for (@_) {
> +		push @oldtext, @{$_->{TEXT}};
> +	}

I think this might be more readable as:

  my @oldtext = map { @{$_->{TEXT}} } @_;

I like to declare and initialize values in the same expression, so you
know that they have that value in all code paths. But that is a nit.

> +	# use a .diff file to help editors with highlighting
> +	my $editpath = $repo->repo_path() . "/ADDP_HUNK_EDIT.diff";
> +	my $fh;
> +	open $fh, '>', $editpath
> +		or die "failed to open hunk edit file for writing: " . $!;
> +	print $fh <<EOF;

Use File::Temp here to get a unique name.

> +# MANUAL HUNK EDIT MODE
> +#
> +# You can change the hunk to your heart's content, but it will be
> +# refused if the end result (the entire patch including your edited
> +# hunk) does not apply cleanly.

This paragraph can probably be omitted if we want to make the
instructions smaller (but I think just putting it at the end will be
fine).

> +# To remove '-' lines, make them ' ' lines (context).
> +# To remove '+' lines, delete them.

This makes perfect sense if you understand editing diffs, but is maybe a
little scary for less clueful users. I wonder if we could detect removed
'-' lines and munge them into context lines on behalf of the user.
Though that requires comparing old and new, which kind of goes against
the feature's "do whatever you want, and we will try to apply it" model.

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

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

> +	my $editor = $ENV{GIT_EDITOR} || $repo->config("core.editor")
> +		|| $ENV{VISUAL} || $ENV{EDITOR} || "vi";
> +	system('sh', '-c', $editor.' "$@"', $editor, $editpath);

I think there are a few other perl spots that figure out the editor.
Maybe this should be refactored into Git.pm?

> +	open $fh, '<', $editpath
> +		or die "failed to open hunk edit file for reading: " . $!;
> +	my @newtext;
> +	while (<$fh>) {
> +		push (@newtext, $_) unless /^#/ || /^$/;
> +	}
> +	close $fh;

Again, I might do:

  my @newtext = grep { !/^$/ && !/^#/ } <$fh>;

Though I wonder if what you really want is:

  grep { /^[@ +-]/ }

since other lines are generally ignored in patches.

> +	my @heads = ();
> +	my ($o_ofs, $n_ofs);
> +	my $o_cnt = 0;
> +	my $n_cnt = 0;
> +	my ($guess_o_ofs, undef, $guess_n_ofs, undef) = parse_hunk_header($oldtext[0]);
> +	for (my $i = 0; $i < @newtext; $i++) {
> +		if ((scalar @heads) == 0 && $newtext[$i] =~ /^[ +-]/) {
> +			splice @newtext, $i, 0, $oldtext[0];
> +			push @heads, $i;
> +		}

You can just say "@heads == 0" (or !@heads) here; the "==" implies
scalar context.

So it looks like we are restoring the old hunk header if they deleted it
for some reason. I guess this is a reasonable precaution. However, it
took me a minute to figure out what was going on. Maybe a comment is in
order.

> +		elsif ($newtext[$i] =~ /^@/) {
> +			if (@heads > 0) {
> +				# fix up the previous header first
> +				($o_ofs, undef, $n_ofs, undef)
> +					= parse_hunk_header($newtext[$heads[-1]]);
> +				$o_ofs = $guess_o_ofs unless defined $o_ofs;
> +				$n_ofs = $guess_n_ofs unless defined $n_ofs;
> +				$newtext[$heads[-1]] = (
> +					"@@ -$o_ofs" . (($o_cnt != 1) ? ",$o_cnt" : '')
> +					. " +$n_ofs" . (($n_cnt != 1) ? ",$n_cnt" : '')
> +					. " @@\n");
> +				$guess_o_ofs = $o_ofs + $o_cnt;
> +				$guess_n_ofs = $n_ofs + $n_cnt;
> +			}
> +			$o_cnt = 0;
> +			$n_cnt = 0;
> +			push @heads, $i;
> +		}
> +	}

I think this is right, but it would be nice to have a few testcases in
t3701 with some basic sanity checks and covering the border cases.

> +	($o_ofs, undef, $n_ofs, undef)
> +		= parse_hunk_header($newtext[$heads[-1]]);
> +	$o_ofs = $guess_o_ofs unless defined $o_ofs;
> +	$n_ofs = $guess_n_ofs unless defined $n_ofs;
> +	$newtext[$heads[-1]] = (
> +		"@@ -$o_ofs" . (($o_cnt != 1) ? ",$o_cnt" : '')
> +		. " +$n_ofs" . (($n_cnt != 1) ? ",$n_cnt" : '')
> +		. " @@\n");

Hmm. Repeating this chunk is a bit ugly. I wonder if the whole loop
might be a bit more readable in two sections: first parse into a list of
hunks, and then fixup each hunk.

> +	push @heads, (scalar @newtext);

Is that right? Before, heads referred to the "$i" of our loop; but
"scalar @newtext" is one _past_ the final element.

> +	my (@hunks) = ();
> +	for (my $i = 0; $i < @heads-1; $i++) {

A nicer way of spelling this is:

  $i < $#heads

> +		my @hunktext = @newtext[$heads[$i]..$heads[$i+1]-1];

Again, I think this might be more clear if you munged newtext into its
constituent hunks.

> +		my @hunkdisplay = ();
> +		for (@hunktext) {
> +			if (/^@/) {
> +				push @hunkdisplay, (colored $fraginfo_color, $_);
> +			}
> +			elsif (/^\+/) {
> +				push @hunkdisplay, (colored $diff_new_color, $_);
> +			}
> +			elsif (/^-/) {
> +				push @hunkdisplay, (colored $diff_old_color, $_);
> +			}
> +			else {
> +				push @hunkdisplay, (colored $diff_plain_color, $_);
> +			}
> +		}

Again, perhaps map instead of a series of pushes is more readable:

  my @hunkdisplay = map {
    colored
      /^@/  ? $fraginfo_color :
      /^\+/ ? $diff_new_color :
      /^-/  ? $diff_old_color :
              $diff_plain_color,
      $_;
  } @hunktext;

> +		my $fh;
> +		open $fh, '| git apply --cached --check';
> +		for my $h ($head,
> +			   $ix > 0 ? @$hunks[0..$ix-1] : (),
> +			   @newhunks,
> +			   $ix < (scalar @$hunks)-2 ? @$hunks[$ix+1..@$hunks] : ()) {
> +			for (@{$h->{TEXT}}) {
> +				print $fh $_;
> +			}
> +		}
> +		if (!close $fh) {
> +			# didn't apply cleanly

Another style nit: I think the edit_hunk_loop function would be much
more readable if this chunk became 'sub diff_applies'.

> +			while (1) {
> +				print colored $prompt_color, "Your edited hunk does not apply. Edit again (saying \"no\" discards!) [y/n]? ";
> +				my $line = <STDIN>;
> +				if ($line =~ /^y/) {

Probably should be "/^y/i".

> +				elsif ($line =~ /^n/) {

Ditto.

Overall, I think it looks reasonable. My complaints are:

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

 - The interface is a little less user-friendly, but a little more
   powerful than I was envisioning. However, nobody has said much, so I
   think the best thing might be to cook in next for a while and see if
   people find it useful.

 - I'd really like to see a few testcases before that, though.

-Peff

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

[Thomas Rast]

[-- 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 --]

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

[Jeff King]

On Thu, Jun 05, 2008 at 09:53:54AM +0200, Thomas Rast wrote:

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

Sorry, I've gotten lost in which is mine and which is yours. Yours is
the original patch or this patch? Mine is the proposal that spawned this
patch, or my comments on this patch?

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

Sorry, I meant to say "patch" here. But even so, I'm still wrong.
Arbitrary text is OK between _diffs_, but not between _hunks_. And by
definition, this format is hunks inside a single diff.

> 	default:
> 		if (apply_verbosely)
> 			error("invalid start of line: '%c'", first);
> 		return -1;

Right, so we signal the end of diff, and any hunks afterwards are "patch
fragment without header".

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

Good point. There's really no reason for such lines to occur, so the
best thing is probably to notice the situation and let the user re-edit.

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

Hmm. Yes, it looks like they are significant. Given that they are
generated by a particular version of GNU diff, and that these should be
"git diff"-generated patches, we aren't likely to encounter them. But
probably we should just leave them as-is, and let "git apply --check" do
what it will with them.

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

Heh. The Perl style in git is a bit tricky; it is such a small portion
of the code base, and there is not a real sense of ownership, so we seem
to have several different styles.

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

Great. I think this is a worthwhile feature, so please keep at it.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Exactly.  To them, "git diff >patch && vi patch && git apply --cached <patch"
would likely to be much handier, quicker and a more familiar way. That is
one of the reasons I somewhat doubt that we would want to have this patch.

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

Beware that a totally empty line is the same as an empty context line "SP LF".

For the rest of your comments, I agree with the Perl style (use of map and
grep instead of repeated push in loops).  The end user input, what the
code needs to parse and accept, can screw you up royally and your parsing
needs to be careful, and the code looks fragile.

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

[Jeff King]

On Thu, Jun 05, 2008 at 01:16:19AM -0700, Junio C Hamano wrote:

> > So perhaps manual hunk editing is simply something for advanced users
> > who are comfortable with the patch format.
> 
> Exactly.  To them, "git diff >patch && vi patch && git apply --cached <patch"
> would likely to be much handier, quicker and a more familiar way. That is
> one of the reasons I somewhat doubt that we would want to have this patch.

I'm not so sure. I think this patch buys you two things:

 1. It munges the hunk headers as appropriate.

 2. It fits more naturally into the "git add -p" workflow. That is, I
    don't start by saying "I need to edit this patch." Instead, I am
    staging bits of changes, and looking at a hunk I say "Oh, this needs
    to be tweaked"; this patch provides an easy route from looking at
    the hunk to editing the hunk.

    Granted, 99% of the time my tweak is "these adjacent lines are
    actually unrelated, so split them into two hunks." So maybe that is
    an argument for something like Thomas' original patch.

> For the rest of your comments, I agree with the Perl style (use of map and
> grep instead of repeated push in loops).  The end user input, what the
> code needs to parse and accept, can screw you up royally and your parsing
> needs to be careful, and the code looks fragile.

I think there are two extremes here:

 1. We provide a very limited interface. E.g., you pick a line to
    arbitrarily split the hunk.

 2. We provide a very flexible interface. You get dumped in your editor
    with the hunk text, and we try to apply the result. Powerful, but
    easy to shoot yourself in the foot.

And there is probably room for both. But I think it is important not to
fall into the middle, where we have a hard-to-use but featureless
interface. That is, if we dump the user in an editor, doing anything
besides (2) doesn't make sense; trying to infer what happened in the
user's editing session is a waste of time.

So I think we need to be not so much careful in parsing, as accepting.
Or we need to dump the idea of letting the user make arbitrary edits.

-Peff

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

[Thomas Rast]

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

You wrote:
> On Thu, Jun 05, 2008 at 09:53:54AM +0200, Thomas Rast wrote:
> 
> > 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.
> 
> Sorry, I've gotten lost in which is mine and which is yours. Yours is
> the original patch or this patch? Mine is the proposal that spawned this
> patch, or my comments on this patch?

I should have provided a definition, sorry.  By "your" scheme, I meant
the one where one is restricted to deleting existing lines, and thus
deletes '-' lines to disable them; by "my" scheme, where one is free
to edit and changes '-' lines to context to disable them.

At least it's my current understanding that, under the above
definition, "your" scheme is what you actually proposed in
  http://www.spinics.net/lists/git/msg67478.html
  [why doesn't Google find gmane?]
but I misunderstood and implemented "my" scheme.

[Let's just forget about the original patch at the top of the thread;
in retrospect, it _is_ clunky, and the UI didn't get any replies in
favour.]

- Thomas

-- 
Thomas Rast
trast@student.ethz.ch


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

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

[Thomas Rast]

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

Sorry, I sent it in reply to the wrong message accidentally.

(Somehow it seems rather ironic that I'm trying to implement a feature
that provides plenty of live bullets, yet keep shooting myself with
the _MUA_.)

You wrote:
> On Thu, Jun 05, 2008 at 09:53:54AM +0200, Thomas Rast wrote:
> 
> > 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.
> 
> Sorry, I've gotten lost in which is mine and which is yours. Yours is
> the original patch or this patch? Mine is the proposal that spawned this
> patch, or my comments on this patch?

I should have provided a definition, sorry.  By "your" scheme, I meant
the one where one is restricted to deleting existing lines, and thus
deletes '-' lines to disable them; by "my" scheme, where one is free
to edit and changes '-' lines to context to disable them.

At least it's my current understanding that, under the above
definition, "your" scheme is what you actually proposed in
  http://www.spinics.net/lists/git/msg67478.html
  [why doesn't Google find gmane?]
but I misunderstood and implemented "my" scheme.

[Let's just forget about the original patch at the top of the thread;
in retrospect, it _is_ clunky, and the UI didn't get any replies in
favour.]

- Thomas


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

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

[Jeff King]

On Thu, Jun 05, 2008 at 11:04:58AM +0200, Thomas Rast wrote:

> (Somehow it seems rather ironic that I'm trying to implement a feature
> that provides plenty of live bullets, yet keep shooting myself with
> the _MUA_.)

Heh.

> I should have provided a definition, sorry.  By "your" scheme, I meant
> the one where one is restricted to deleting existing lines, and thus
> deletes '-' lines to disable them; by "my" scheme, where one is free
> to edit and changes '-' lines to context to disable them.

OK, thanks for the clarification.

> At least it's my current understanding that, under the above
> definition, "your" scheme is what you actually proposed in
>   http://www.spinics.net/lists/git/msg67478.html
>   [why doesn't Google find gmane?]
> but I misunderstood and implemented "my" scheme.

For the record, "my" scheme was only half thought-through, and I think I
actually like "your" scheme better. Once we give the user an editor,
restricting them to a tiny subset of editor operations seems error-prone
and annoying, since we have no way of enforcing those operations except
to wait until after they edit and say "oops, you did something bad."

> [Let's just forget about the original patch at the top of the thread;
> in retrospect, it _is_ clunky, and the UI didn't get any replies in
> favour.]

It seems like Junio isn't all that keen on the raw patch-editing
interface. And even if we do like it, I think there is still room for a
less error-prone but more restrictive feature that mere mortals can use.
So maybe there is a better interface yet.

What about 'S' to do a "line split"; that is, take the current hunk, and
anywhere there are adjacent changed lines, split them into their own
hunks. I.e. the hunk,

   line 1
  +line 2
  -line 3
   line 4
  -line 5

becomes three hunks:

   line 1
  +line 2

  -line 3
   line 4

  -line 5

and then we proceed as usual, staging or not each split hunk. It would
be clunky to separate one or two lines from a huge chunk (since you
would inadvertently split the huge chunk and have to stage each
individually). But in many cases you can split into smaller hunks first
with 's'.

-Peff

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

[Thomas Rast]

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

Jeff King wrote:
> > [Let's just forget about the original patch at the top of the thread;
> > in retrospect, it _is_ clunky, and the UI didn't get any replies in
> > favour.]
> 
> It seems like Junio isn't all that keen on the raw patch-editing
> interface. And even if we do like it, I think there is still room for a
> less error-prone but more restrictive feature that mere mortals can use.
> So maybe there is a better interface yet.

Maybe git gui could do a sort of "toggle the lines I click" interface.
But I don't know anything about Tk, or Tcl, or git gui :-(

> What about 'S' to do a "line split"; that is, take the current hunk, and
> anywhere there are adjacent changed lines, split them into their own
> hunks.
[...]
> and then we proceed as usual, staging or not each split hunk. It would
> be clunky to separate one or two lines from a huge chunk (since you
> would inadvertently split the huge chunk and have to stage each
> individually). But in many cases you can split into smaller hunks first
> with 's'.

Now I'm slightly confused.

Doing it that way would be almost like my original patch

  http://www.spinics.net/lists/git/msg66971.html

minus the numeric prefixes -- meaning that you have to say y/n to
_every_ line in the patch, at least until all remaining hunks are the
same and you can answer the rest with a/d.

Except that it wouldn't work anyway, because git-apply refuses hunks
that have no context (even if just on one end).  Unless given
--unidiff-zero, but that apparently was one of the points of refusal
in the thread Dscho linked earlier:

  http://thread.gmane.org/gmane.comp.version-control.git/67854/focus=68127

Granted, we could insert extra context and/or make sure the mentioned
data loss can never happen (it's probably prevented by 'add -p's own
recounting before the final apply), but the first would make the UI
even more confusing and the second is potentially dangerous.

- Thomas

-- 
Thomas Rast
trast@student.ethz.ch



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

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

[Jeff King]

On Thu, Jun 05, 2008 at 11:38:15AM +0200, Thomas Rast wrote:

> Maybe git gui could do a sort of "toggle the lines I click" interface.
> But I don't know anything about Tk, or Tcl, or git gui :-(

That would probably be a reasonable interface. But I don't use git-gui,
so I personally am not interested in that. ;)

> Now I'm slightly confused.
> 
> Doing it that way would be almost like my original patch
> 
>   http://www.spinics.net/lists/git/msg66971.html
> 
> minus the numeric prefixes -- meaning that you have to say y/n to
> _every_ line in the patch, at least until all remaining hunks are the
> same and you can answer the rest with a/d.

Hmm, I suppose it is. Sorry, I'm just trying to brainstorm a bit about
other options, but I think that my proposal really isn't that good an
idea.

-Peff

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

[Johannes Schindelin]

Hi,

On Thu, 5 Jun 2008, Jeff King wrote:

> I think there are two extremes here:
> 
>  1. We provide a very limited interface. E.g., you pick a line to 
>     arbitrarily split the hunk.
> 
>  2. We provide a very flexible interface. You get dumped in your editor 
>     with the hunk text, and we try to apply the result. Powerful, but 
>     easy to shoot yourself in the foot.

We have a tradition of giving the users plenty of rope.

And I actually like having the power at my finger tips.  You would not 
believe how I enjoyed using "git add -e" to commit the final version of 
that very patch.

I, for one, will probably use this feature almost exclusively, 
_especially_ since I look at the diffs before committing _anyway_, and I 
cannot count the times when I had to go to another window to fix up a 
stupid debug message or typo.  Now I will do that right away, and be done 
with it.  I can even stash the debug messages for future use!

Ciao,
Dscho

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

[Thomas Rast]

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

Adds a new option 'e' to the 'add -p' command loop that lets you
edit the current hunk in your favourite editor.

---

Jeff King wrote:
> Hmm. Repeating this chunk is a bit ugly. I wonder if the whole loop
> might be a bit more readable in two sections: first parse into a list of
> hunks, and then fixup each hunk.
[...]
>  - 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.

This is the squashed patch, changed according to your comments, and
with the loop in question rewritten as you suggested.  (It's indeed
far less obscure.)

Not ready for commit yet because

>  - I'd really like to see a few testcases before that, though.

is still missing.

- Thomas


 git-add--interactive.perl |  164 ++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 147 insertions(+), 17 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 903953e..5fb8402 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -2,6 +2,7 @@
 
 use strict;
 use Git;
+use File::Temp qw/tempfile/;
 
 my $repo = Git->repository();
 
@@ -18,6 +19,18 @@ my ($fraginfo_color) =
 	$diff_use_color ? (
 		$repo->get_color('color.diff.frag', 'cyan'),
 	) : ();
+my ($diff_plain_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.plain', ''),
+	) : ();
+my ($diff_old_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.old', 'red'),
+	) : ();
+my ($diff_new_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.new', 'green'),
+	) : ();
 
 my $normal_color = $repo->get_color("", "reset");
 
@@ -581,6 +594,13 @@ sub parse_hunk_header {
 	return ($o_ofs, $o_cnt, $n_ofs, $n_cnt);
 }
 
+sub format_hunk_header {
+	my ($o_ofs, $o_cnt, $n_ofs, $n_cnt) = @_;
+	return ("@@ -$o_ofs" . (($o_cnt != 1) ? ",$o_cnt" : '')
+		. " +$n_ofs" . (($n_cnt != 1) ? ",$n_cnt" : '')
+		. " @@\n");
+}
+
 sub split_hunk {
 	my ($text, $display) = @_;
 	my @split = ();
@@ -667,11 +687,7 @@ sub split_hunk {
 		my $o_cnt = $hunk->{OCNT};
 		my $n_cnt = $hunk->{NCNT};
 
-		my $head = ("@@ -$o_ofs" .
-			    (($o_cnt != 1) ? ",$o_cnt" : '') .
-			    " +$n_ofs" .
-			    (($n_cnt != 1) ? ",$n_cnt" : '') .
-			    " @@\n");
+		my $head = format_hunk_header($o_ofs, $o_cnt, $n_ofs, $n_cnt);
 		my $display_head = $head;
 		unshift @{$hunk->{TEXT}}, $head;
 		if ($diff_use_color) {
@@ -741,11 +757,7 @@ sub merge_hunk {
 		}
 		push @line, $line;
 	}
-	my $head = ("@@ -$o0_ofs" .
-		    (($o_cnt != 1) ? ",$o_cnt" : '') .
-		    " +$n0_ofs" .
-		    (($n_cnt != 1) ? ",$n_cnt" : '') .
-		    " @@\n");
+	my $head = format_hunk_header($o0_ofs, $o_cnt, $n0_ofs, $n_cnt);
 	@{$prev->{TEXT}} = ($head, @line);
 }
 
@@ -770,6 +782,122 @@ sub coalesce_overlapping_hunks {
 	return @out;
 }
 
+sub edit_hunk_manually {
+	my @oldtext = map { @{$_->{TEXT}} } @_;
+
+	my ($fh, $editpath) = tempfile($repo->repo_path() . "/git-hunk-edit.XXXXXX",
+				       SUFFIX => ".diff", UNLINK => 0);
+	print $fh "# Manual hunk edit mode -- see bottom for a quick guide\n";
+	print $fh @oldtext;
+	print $fh <<EOF;
+# ---
+# To remove '-' lines, make them ' ' lines (context).
+# To remove '+' lines, delete them.
+# Empty lines and lines starting with # will be removed.
+#
+# Lines starting with @ start a new hunk. Line counts will be adjusted
+# according to contents. If the line numbers are missing altogether,
+# they will be inferred from the previous hunk.
+#
+# You can change the hunk to your heart's content, but it will be
+# refused if the end result (the entire patch including your edited
+# hunk) does not apply cleanly.
+EOF
+	close $fh;
+
+	my $editor = $ENV{GIT_EDITOR} || $repo->config("core.editor")
+		|| $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+	system('sh', '-c', $editor.' "$@"', $editor, $editpath);
+
+	open $fh, '<', $editpath
+		or die "failed to open hunk edit file for reading: " . $!;
+	my @newtext = grep { !/^#/ } <$fh>;
+	close $fh;
+	unlink(glob($editpath . "*"));
+	# Reinsert the first hunk header if the user accidentally deleted it
+	if ($newtext[0] !~ /^@/) {
+		splice @newtext, 0, 0, $oldtext[0];
+	}
+	# Split into hunks
+	my @hunktexts = ();
+	my $curhunk = [];
+	for (@newtext) {
+		if (/^@/ && @{$curhunk} > 0) {
+			push @hunktexts, $curhunk;
+			$curhunk = [];
+		}
+		push @{$curhunk}, $_;
+	}
+	push @hunktexts, $curhunk;
+	# Fix the hunk headers
+	my ($guess_o_ofs, undef, $guess_n_ofs, undef) = parse_hunk_header($oldtext[0]);
+	for my $hunk (@hunktexts) {
+		my ($o_ofs, undef, $n_ofs, undef) = parse_hunk_header($hunk->[0]);
+		$o_ofs = $guess_o_ofs unless defined $o_ofs;
+		$n_ofs = $guess_n_ofs unless defined $n_ofs;
+		my $plus_cnt = grep /^\+/, @{$hunk};
+		my $minus_cnt = grep /^-/, @{$hunk};
+		my $context_cnt = grep { /^ / || /^$/ } @{$hunk};
+		my $o_cnt = $context_cnt + $minus_cnt;
+		my $n_cnt = $context_cnt + $plus_cnt;
+		$hunk->[0] = format_hunk_header($o_ofs, $o_cnt, $n_ofs, $n_cnt);
+		$guess_o_ofs = $o_ofs + $o_cnt;
+		$guess_n_ofs = $n_ofs + $n_cnt;
+	}
+	# Recolor the hunks
+	my (@hunks) = ();
+	for my $hunk (@hunktexts) {
+		my @hunkdisplay = map {
+			colored((/^@/  ? $fraginfo_color :
+				 /^\+/ ? $diff_new_color :
+				 /^-/  ? $diff_old_color :
+				 $diff_plain_color),
+				$_);
+		} @{$hunk};
+		push @hunks, {TEXT => $hunk, DISPLAY => \@hunkdisplay};
+	}
+
+	return @hunks;
+}
+
+sub diff_applies {
+	my $fh;
+	open $fh, '| git apply --cached --check';
+	for my $h (@_) {
+		print $fh @{$h->{TEXT}};
+	}
+	return close $fh;
+}
+
+sub edit_hunk_loop {
+	my ($head, $hunks, $ix) = @_;
+
+	my @newhunks = ($hunks->[$ix]);
+
+      EDIT:
+	while (1) {
+		@newhunks = edit_hunk_manually(@newhunks);
+		if (!diff_applies($head, @$hunks[0..$ix-1], @newhunks,
+				  @$hunks[$ix+1..$#{$hunks}])) {
+			while (1) {
+				print colored $prompt_color, 'Your edited hunk does not apply. Edit again (saying "no" discards!) [y/n]? ';
+				my $line = <STDIN>;
+				if ($line =~ /^y/i) {
+					redo EDIT;
+				}
+				elsif ($line =~ /^n/i) {
+					return $hunks->[$ix];
+				}
+			}
+		}
+		if (1 < @newhunks) {
+			print colored $header_color, "Manually edited into ",
+			scalar(@newhunks), " hunks.\n";
+		}
+		return @newhunks;
+	}
+}
+
 sub help_patch_cmd {
 	print colored $help_color, <<\EOF ;
 y - stage this hunk
@@ -781,6 +909,7 @@ J - leave this hunk undecided, see next hunk
 k - leave this hunk undecided, see previous undecided hunk
 K - leave this hunk undecided, see previous hunk
 s - split the current hunk into smaller hunks
+e - manually edit the current hunk
 ? - print help
 EOF
 }
@@ -885,6 +1014,7 @@ sub patch_update_file {
 		if (hunk_splittable($hunk[$ix]{TEXT})) {
 			$other .= '/s';
 		}
+		$other .= '/e';
 		for (@{$hunk[$ix]{DISPLAY}}) {
 			print;
 		}
@@ -949,6 +1079,11 @@ sub patch_update_file {
 				$num = scalar @hunk;
 				next;
 			}
+			elsif ($line =~ /^e/) {
+				splice @hunk, $ix, 1, edit_hunk_loop($head, \@hunk, $ix);
+				$num = scalar @hunk;
+				next;
+			}
 			else {
 				help_patch_cmd($other);
 				next;
@@ -985,13 +1120,8 @@ sub patch_update_file {
 		else {
 			if ($n_lofs) {
 				$n_ofs += $n_lofs;
-				$text->[0] = ("@@ -$o_ofs" .
-					      (($o_cnt != 1)
-					       ? ",$o_cnt" : '') .
-					      " +$n_ofs" .
-					      (($n_cnt != 1)
-					       ? ",$n_cnt" : '') .
-					      " @@\n");
+				$text->[0] = format_hunk_header($o_ofs, $o_cnt,
+								$n_ofs, $n_cnt);
 			}
 			for (@$text) {
 				push @result, $_;
--
1.5.6.rc1.137.g537d1


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

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

[Jeff King]

On Thu, Jun 05, 2008 at 11:28:53AM +0100, Johannes Schindelin wrote:

> We have a tradition of giving the users plenty of rope.
> 
> And I actually like having the power at my finger tips.  You would not 
> believe how I enjoyed using "git add -e" to commit the final version of 
> that very patch.

I looked at your patch, and here are my complaints (versus what Thomas
has been working on):

  1. You edit the whole diff, not just a hunk. Actually, I think this is
     probably not a big deal, as any decent editor lets you find the
     spot you're looking for pretty trivially.

  2. It's not integrated into the git-add--interactive loop at all. That
     is, I don't start out saying "I want to edit this diff." I look at
     the diff while staging with "git add -p" and say "Oops, I need to
     edit this hunk." So I think it is better implemented as an "e"
     option in the hunk adding loop, with "git add -e" as a shortcut.
     Or maybe there is simply room for both (and "git add -e", rather
     than being a shortcut, just means "do this on the _whole_ file").

-Peff

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

[Jeff King]

On Fri, Jun 06, 2008 at 01:10:26AM -0400, Jeff King wrote:

>   2. It's not integrated into the git-add--interactive loop at all. That
>      is, I don't start out saying "I want to edit this diff." I look at
>      the diff while staging with "git add -p" and say "Oops, I need to
>      edit this hunk." So I think it is better implemented as an "e"
>      option in the hunk adding loop, with "git add -e" as a shortcut.
>      Or maybe there is simply room for both (and "git add -e", rather
>      than being a shortcut, just means "do this on the _whole_ file").

I wrote this after reading just your first patch, and it looks like
you've made much progress since. It seems like Thomas' patch could just
get rid of all the recounting entirely, and just pass off the edited
hunks to "git apply --recount". Which should make his patch much smaller
and more straightforward.

-Peff

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

[Johannes Schindelin]

Hi,

On Fri, 6 Jun 2008, Jeff King wrote:

> On Thu, Jun 05, 2008 at 11:28:53AM +0100, Johannes Schindelin wrote:
> 
> > We have a tradition of giving the users plenty of rope.
> > 
> > And I actually like having the power at my finger tips.  You would not 
> > believe how I enjoyed using "git add -e" to commit the final version of 
> > that very patch.
> 
> I looked at your patch, and here are my complaints (versus what Thomas
> has been working on):
> 
>   1. You edit the whole diff, not just a hunk. Actually, I think this is
>      probably not a big deal, as any decent editor lets you find the
>      spot you're looking for pretty trivially.

Actually, this is exactly what I wanted.  I positively _hate_ all the key 
presses I have to go through until I am finally where I want to be.  I 
cannot just search for a term and be done with it.  Reminds me of 
Windows-like dialog based configurations.

>   2. It's not integrated into the git-add--interactive loop at all. That
>      is, I don't start out saying "I want to edit this diff." I look at
>      the diff while staging with "git add -p" and say "Oops, I need to
>      edit this hunk." So I think it is better implemented as an "e"
>      option in the hunk adding loop, with "git add -e" as a shortcut.
>      Or maybe there is simply room for both (and "git add -e", rather
>      than being a shortcut, just means "do this on the _whole_ file").

This is very much on purpose.  I do not like "git add -i" at all.  It 
limits my work unduly.  That's why I tried to change the hunk editing in 
git-gui once upon a time, but I never got round to fix that, and it does 
not work well with ssh either.

So no, I do not want to use that perl script with that menu.  I want to 
have the raw diff in a raw editor, where I can change the things I need to 
change.

Ciao,
Dscho

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

[Thomas Rast]

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

Johannes Schindelin wrote:
> 
> On Fri, 6 Jun 2008, Jeff King wrote:
> >   2. It's not integrated into the git-add--interactive loop at all. That
> >      is, I don't start out saying "I want to edit this diff." I look at
> >      the diff while staging with "git add -p" and say "Oops, I need to
> >      edit this hunk." So I think it is better implemented as an "e"
> >      option in the hunk adding loop, with "git add -e" as a shortcut.
> >      Or maybe there is simply room for both (and "git add -e", rather
> >      than being a shortcut, just means "do this on the _whole_ file").
> 
> This is very much on purpose.  I do not like "git add -i" at all.  It 
> limits my work unduly.  That's why I tried to change the hunk editing in 
> git-gui once upon a time, but I never got round to fix that, and it does 
> not work well with ssh either.
> 
> So no, I do not want to use that perl script with that menu.  I want to 
> have the raw diff in a raw editor, where I can change the things I need to 
> change.

While there is obviously little point in trying to convince you, let
me briefly explain why I still think it is useful:

I usually run 'add -p' instead of adding specific files.  "Nodding
off" each hunk means that I get a last chance to review my changes,
and perhaps skip some of them (possibly for a later commit).  With my
proposed editing feature, I can split hunks in the middle too.  You
could of course argue that the right way to do this would be staring
at 'git diff', or perhaps scrolling through the entire patch in your
'add -e'.  I just got used to doing the reviews during 'add -p'.  As
Jeff said, by the time I decide to change a hunk, I might already be
halfway through the decisions and _definitely_ don't want to bail out
and restart with 'add -e'.

That, and my Maple worksheets have a tendency to show up when I didn't
want to commit them, so I can just say 'd' there.

- Thomas

-- 
Thomas Rast
trast@student.ethz.ch


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

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

[Thomas Rast]

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

Adds a new option 'e' to the 'add -p' command loop that lets you
edit the current hunk in your favourite editor.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

On top of the previous patch, this adds basic testing.

Compared to the competing bikeshed
  http://article.gmane.org/gmane.comp.version-control.git/83894 
this integrates into the 'add -p' loop.  I (still) prefer it because I
use 'add -p' a lot, thus having the editing feature at my fingertips.

- Thomas


 git-add--interactive.perl  |  164 +++++++++++++++++++++++++++++++++++++++-----
 t/t3701-add-interactive.sh |   57 +++++++++++++++
 2 files changed, 204 insertions(+), 17 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 903953e..5fb8402 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -2,6 +2,7 @@
 
 use strict;
 use Git;
+use File::Temp qw/tempfile/;
 
 my $repo = Git->repository();
 
@@ -18,6 +19,18 @@ my ($fraginfo_color) =
 	$diff_use_color ? (
 		$repo->get_color('color.diff.frag', 'cyan'),
 	) : ();
+my ($diff_plain_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.plain', ''),
+	) : ();
+my ($diff_old_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.old', 'red'),
+	) : ();
+my ($diff_new_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.new', 'green'),
+	) : ();
 
 my $normal_color = $repo->get_color("", "reset");
 
@@ -581,6 +594,13 @@ sub parse_hunk_header {
 	return ($o_ofs, $o_cnt, $n_ofs, $n_cnt);
 }
 
+sub format_hunk_header {
+	my ($o_ofs, $o_cnt, $n_ofs, $n_cnt) = @_;
+	return ("@@ -$o_ofs" . (($o_cnt != 1) ? ",$o_cnt" : '')
+		. " +$n_ofs" . (($n_cnt != 1) ? ",$n_cnt" : '')
+		. " @@\n");
+}
+
 sub split_hunk {
 	my ($text, $display) = @_;
 	my @split = ();
@@ -667,11 +687,7 @@ sub split_hunk {
 		my $o_cnt = $hunk->{OCNT};
 		my $n_cnt = $hunk->{NCNT};
 
-		my $head = ("@@ -$o_ofs" .
-			    (($o_cnt != 1) ? ",$o_cnt" : '') .
-			    " +$n_ofs" .
-			    (($n_cnt != 1) ? ",$n_cnt" : '') .
-			    " @@\n");
+		my $head = format_hunk_header($o_ofs, $o_cnt, $n_ofs, $n_cnt);
 		my $display_head = $head;
 		unshift @{$hunk->{TEXT}}, $head;
 		if ($diff_use_color) {
@@ -741,11 +757,7 @@ sub merge_hunk {
 		}
 		push @line, $line;
 	}
-	my $head = ("@@ -$o0_ofs" .
-		    (($o_cnt != 1) ? ",$o_cnt" : '') .
-		    " +$n0_ofs" .
-		    (($n_cnt != 1) ? ",$n_cnt" : '') .
-		    " @@\n");
+	my $head = format_hunk_header($o0_ofs, $o_cnt, $n0_ofs, $n_cnt);
 	@{$prev->{TEXT}} = ($head, @line);
 }
 
@@ -770,6 +782,122 @@ sub coalesce_overlapping_hunks {
 	return @out;
 }
 
+sub edit_hunk_manually {
+	my @oldtext = map { @{$_->{TEXT}} } @_;
+
+	my ($fh, $editpath) = tempfile($repo->repo_path() . "/git-hunk-edit.XXXXXX",
+				       SUFFIX => ".diff", UNLINK => 0);
+	print $fh "# Manual hunk edit mode -- see bottom for a quick guide\n";
+	print $fh @oldtext;
+	print $fh <<EOF;
+# ---
+# To remove '-' lines, make them ' ' lines (context).
+# To remove '+' lines, delete them.
+# Empty lines and lines starting with # will be removed.
+#
+# Lines starting with @ start a new hunk. Line counts will be adjusted
+# according to contents. If the line numbers are missing altogether,
+# they will be inferred from the previous hunk.
+#
+# You can change the hunk to your heart's content, but it will be
+# refused if the end result (the entire patch including your edited
+# hunk) does not apply cleanly.
+EOF
+	close $fh;
+
+	my $editor = $ENV{GIT_EDITOR} || $repo->config("core.editor")
+		|| $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+	system('sh', '-c', $editor.' "$@"', $editor, $editpath);
+
+	open $fh, '<', $editpath
+		or die "failed to open hunk edit file for reading: " . $!;
+	my @newtext = grep { !/^#/ } <$fh>;
+	close $fh;
+	unlink(glob($editpath . "*"));
+	# Reinsert the first hunk header if the user accidentally deleted it
+	if ($newtext[0] !~ /^@/) {
+		splice @newtext, 0, 0, $oldtext[0];
+	}
+	# Split into hunks
+	my @hunktexts = ();
+	my $curhunk = [];
+	for (@newtext) {
+		if (/^@/ && @{$curhunk} > 0) {
+			push @hunktexts, $curhunk;
+			$curhunk = [];
+		}
+		push @{$curhunk}, $_;
+	}
+	push @hunktexts, $curhunk;
+	# Fix the hunk headers
+	my ($guess_o_ofs, undef, $guess_n_ofs, undef) = parse_hunk_header($oldtext[0]);
+	for my $hunk (@hunktexts) {
+		my ($o_ofs, undef, $n_ofs, undef) = parse_hunk_header($hunk->[0]);
+		$o_ofs = $guess_o_ofs unless defined $o_ofs;
+		$n_ofs = $guess_n_ofs unless defined $n_ofs;
+		my $plus_cnt = grep /^\+/, @{$hunk};
+		my $minus_cnt = grep /^-/, @{$hunk};
+		my $context_cnt = grep { /^ / || /^$/ } @{$hunk};
+		my $o_cnt = $context_cnt + $minus_cnt;
+		my $n_cnt = $context_cnt + $plus_cnt;
+		$hunk->[0] = format_hunk_header($o_ofs, $o_cnt, $n_ofs, $n_cnt);
+		$guess_o_ofs = $o_ofs + $o_cnt;
+		$guess_n_ofs = $n_ofs + $n_cnt;
+	}
+	# Recolor the hunks
+	my (@hunks) = ();
+	for my $hunk (@hunktexts) {
+		my @hunkdisplay = map {
+			colored((/^@/  ? $fraginfo_color :
+				 /^\+/ ? $diff_new_color :
+				 /^-/  ? $diff_old_color :
+				 $diff_plain_color),
+				$_);
+		} @{$hunk};
+		push @hunks, {TEXT => $hunk, DISPLAY => \@hunkdisplay};
+	}
+
+	return @hunks;
+}
+
+sub diff_applies {
+	my $fh;
+	open $fh, '| git apply --cached --check';
+	for my $h (@_) {
+		print $fh @{$h->{TEXT}};
+	}
+	return close $fh;
+}
+
+sub edit_hunk_loop {
+	my ($head, $hunks, $ix) = @_;
+
+	my @newhunks = ($hunks->[$ix]);
+
+      EDIT:
+	while (1) {
+		@newhunks = edit_hunk_manually(@newhunks);
+		if (!diff_applies($head, @$hunks[0..$ix-1], @newhunks,
+				  @$hunks[$ix+1..$#{$hunks}])) {
+			while (1) {
+				print colored $prompt_color, 'Your edited hunk does not apply. Edit again (saying "no" discards!) [y/n]? ';
+				my $line = <STDIN>;
+				if ($line =~ /^y/i) {
+					redo EDIT;
+				}
+				elsif ($line =~ /^n/i) {
+					return $hunks->[$ix];
+				}
+			}
+		}
+		if (1 < @newhunks) {
+			print colored $header_color, "Manually edited into ",
+			scalar(@newhunks), " hunks.\n";
+		}
+		return @newhunks;
+	}
+}
+
 sub help_patch_cmd {
 	print colored $help_color, <<\EOF ;
 y - stage this hunk
@@ -781,6 +909,7 @@ J - leave this hunk undecided, see next hunk
 k - leave this hunk undecided, see previous undecided hunk
 K - leave this hunk undecided, see previous hunk
 s - split the current hunk into smaller hunks
+e - manually edit the current hunk
 ? - print help
 EOF
 }
@@ -885,6 +1014,7 @@ sub patch_update_file {
 		if (hunk_splittable($hunk[$ix]{TEXT})) {
 			$other .= '/s';
 		}
+		$other .= '/e';
 		for (@{$hunk[$ix]{DISPLAY}}) {
 			print;
 		}
@@ -949,6 +1079,11 @@ sub patch_update_file {
 				$num = scalar @hunk;
 				next;
 			}
+			elsif ($line =~ /^e/) {
+				splice @hunk, $ix, 1, edit_hunk_loop($head, \@hunk, $ix);
+				$num = scalar @hunk;
+				next;
+			}
 			else {
 				help_patch_cmd($other);
 				next;
@@ -985,13 +1120,8 @@ sub patch_update_file {
 		else {
 			if ($n_lofs) {
 				$n_ofs += $n_lofs;
-				$text->[0] = ("@@ -$o_ofs" .
-					      (($o_cnt != 1)
-					       ? ",$o_cnt" : '') .
-					      " +$n_ofs" .
-					      (($n_cnt != 1)
-					       ? ",$n_cnt" : '') .
-					      " @@\n");
+				$text->[0] = format_hunk_header($o_ofs, $o_cnt,
+								$n_ofs, $n_cnt);
 			}
 			for (@$text) {
 				push @result, $_;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index fae64ea..7c8d459 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -66,6 +66,63 @@ test_expect_success 'revert works (commit)' '
 	grep "unchanged *+3/-0 file" output
 '
 
+cat >expected <<EOF
+EOF
+cat >fake_editor.sh <<EOF
+EOF
+chmod a+x fake_editor.sh
+test_set_editor "$(pwd)/fake_editor.sh"
+test_expect_success 'dummy edit works' '
+	(echo e; echo a) | git add -p &&
+	git diff > diff &&
+	test_cmp expected diff
+'
+
+cat >patch <<EOF
+@@ -1 +1,4 @@
+this
+patch
+is
+malformed
+EOF
+echo "#!$SHELL_PATH" >fake_editor.sh
+cat >>fake_editor.sh <<\EOF
+mv -f "$1" oldpatch &&
+mv -f patch "$1"
+EOF
+chmod a+x fake_editor.sh
+test_set_editor "$(pwd)/fake_editor.sh"
+test_expect_success 'garbage edit rejected' '
+	git reset &&
+	(echo e; echo n; echo d) | git add -p >output &&
+	grep "hunk does not apply" output
+'
+
+cat >patch <<EOF
+@
+ baseline
++content
++newcontent
++lines
+EOF
+cat >expected <<EOF
+diff --git a/file b/file
+index b5dd6c9..f910ae9 100644
+--- a/file
++++ b/file
+@@ -1,4 +1,4 @@
+ baseline
+ content
+-newcontent
++more
+ lines
+EOF
+test_expect_success 'real edit works' '
+	(echo e; echo a) | git add -p &&
+	git diff >output &&
+	test_cmp expected output
+'
+
 if test "$(git config --bool core.filemode)" = false
 then
     say 'skipping filemode tests (filesystem does not properly support modes)'
-- 
1.5.6.rc2.129.gcd433

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

[Thomas Rast]

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

Jeff King wrote:
>
> I wrote this after reading just your first patch, and it looks like
> you've made much progress since. It seems like Thomas' patch could just
> get rid of all the recounting entirely, and just pass off the edited
> hunks to "git apply --recount". Which should make his patch much smaller
> and more straightforward.

I think there's no way to split hunks, in the way I currently "help"
with @@ line guessing, using just the --recount feature.  Unless the
editor helps you with adding complete correct @@ lines in the middle
of hunks (Emacs does that).  I don't think it is at all possible to
remove the middle part of a hunk in Johannes' scheme without somehow
figuring out the corresponding @@ line (or at least its old line
number) or editing away every +/- line.

Then again it's not always easy even with my patch: you may have to
manually insert extra context because of the rule against zero lines
of context.  We could guess the context too if we already guessed the
line numbers, but that would mean even more complexity, which you
don't seem to like...

- Thomas

-- 
Thomas Rast
trast@student.ethz.ch







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

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

[Johannes Schindelin]

Hi,

On Mon, 9 Jun 2008, Thomas Rast wrote:

> Johannes Schindelin wrote:
> > 
> > On Fri, 6 Jun 2008, Jeff King wrote:
> > >   2. It's not integrated into the git-add--interactive loop at all. 
> > >      That is, I don't start out saying "I want to edit this diff." I 
> > >      look at the diff while staging with "git add -p" and say "Oops, 
> > >      I need to edit this hunk." So I think it is better implemented 
> > >      as an "e"  option in the hunk adding loop, with "git add -e" as 
> > >      a shortcut.  Or maybe there is simply room for both (and "git 
> > >      add -e", rather than being a shortcut, just means "do this on 
> > >      the _whole_ file").
> > 
> > This is very much on purpose.  I do not like "git add -i" at all.  It 
> > limits my work unduly.  That's why I tried to change the hunk editing 
> > in git-gui once upon a time, but I never got round to fix that, and it 
> > does not work well with ssh either.
> > 
> > So no, I do not want to use that perl script with that menu.  I want 
> > to have the raw diff in a raw editor, where I can change the things I 
> > need to change.
> 
> While there is obviously little point in trying to convince you,

Oh, sorry, I really meant the "I do not want" literally.  It is just me.  
That does not mean that your hunk editing from within add -i has no merit.  
It's just that this guy is not very interested in that feature.

Ciao,
Dscho

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

[Johannes Schindelin]

Hi,

On Mon, 9 Jun 2008, Thomas Rast wrote:

> Jeff King wrote:
> >
> > I wrote this after reading just your first patch, and it looks like 
> > you've made much progress since. It seems like Thomas' patch could 
> > just get rid of all the recounting entirely, and just pass off the 
> > edited hunks to "git apply --recount". Which should make his patch 
> > much smaller and more straightforward.
> 
> I think there's no way to split hunks, in the way I currently "help" 
> with @@ line guessing, using just the --recount feature.  Unless the 
> editor helps you with adding complete correct @@ lines in the middle of 
> hunks (Emacs does that).  I don't think it is at all possible to remove 
> the middle part of a hunk in Johannes' scheme without somehow figuring 
> out the corresponding @@ line (or at least its old line number) or 
> editing away every +/- line.
> 
> Then again it's not always easy even with my patch: you may have to
> manually insert extra context because of the rule against zero lines
> of context.

That is actually where Junio convinced me that my approach is wrong: he 
said that you can _only_ reliably split a hunk at common lines.

The thing is: if you split _not_ at a common line, the context of the 
second part would _change_ depending if you want to apply the first part 
or not.

Ciao,
Dscho

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

[Johannes Schindelin]

Hi,

On Mon, 9 Jun 2008, Thomas Rast wrote:

> Compared to the competing bikeshed
>   http://article.gmane.org/gmane.comp.version-control.git/83894 

If you really think this is a bikeshed, I better spend my time elsewhere.

Hth,
Dscho

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

[Johan Herland]

On Monday 09 June 2008, Johannes Schindelin wrote:
> Hi,
>
> On Mon, 9 Jun 2008, Thomas Rast wrote:
> > Compared to the competing bikeshed
> >   http://article.gmane.org/gmane.comp.version-control.git/83894
>
> If you really think this is a bikeshed, I better spend my time elsewhere.

Is there a good reason against having *both*?

AFAICS, there's nothing stopping us from having both a "-e"-option to 
git-add, and an "e"-command inside git-add--interactive.

...just like we have "-p" and "p" today...

("git-add -e" would open the entire diff in an editor, as would "e" from the 
*main* menu of git-add--interactive. However, "e" from the *single hunk* 
menu would of course open only that single hunk within the editor. We could 
even have an "E" command to open all remaining/undecided hunks in an 
editor.)


Have fun! :)

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

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

[Jeff King]

On Mon, Jun 09, 2008 at 07:46:22AM +0200, Johan Herland wrote:

> Is there a good reason against having *both*?
> 
> AFAICS, there's nothing stopping us from having both a "-e"-option to 
> git-add, and an "e"-command inside git-add--interactive.

I agree (and I tried to make that point in an earlier mail).

And I was hoping the right way to do it was to simply build the
interactive "e" command on top of Johannes' git-apply work. But I don't
think that quite makes sense. His work is about fixing up the hunk
header as we apply the patch, but a working "e" command in the hunk
selection should probably not actually apply, but simply split into two
hunks for the loop.

> ("git-add -e" would open the entire diff in an editor, as would "e" from the 
> *main* menu of git-add--interactive. However, "e" from the *single hunk* 
> menu would of course open only that single hunk within the editor. We could 
> even have an "E" command to open all remaining/undecided hunks in an 
> editor.)

I agree with all of this, though I think the big question is what
happens to the edited portion. In the interactive command, I think it
becomes a new hunk that can be staged or not. In "git add -e" it makes
sense to simply stage the result.

-Peff

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

[Johannes Schindelin]

Hi,

On Mon, 9 Jun 2008, Jeff King wrote:

> On Mon, Jun 09, 2008 at 07:46:22AM +0200, Johan Herland wrote:
> 
> And I was hoping the right way to do it was to simply build the 
> interactive "e" command on top of Johannes' git-apply work. But I don't 
> think that quite makes sense. His work is about fixing up the hunk 
> header as we apply the patch, but a working "e" command in the hunk 
> selection should probably not actually apply, but simply split into two 
> hunks for the loop.

Well, maybe I am silly, but I thought that the idea with add -i (e) was 
to split off the first part, ask the user if that should be applied, and 
then go on with the rest.

But like I said, Junio convinced me that it makes not much sense to split 
somewhere else than common lines, and you have that already with the "s" 
command in add -i.

In any case, I am happy with "add -e", even more so than I though I would 
be, and therefore I do not need add -i (e).  Thus, I will just shut up and 
let you guys work it out.

Ciao,
Dscho

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

[Johan Herland]

On Monday 09 June 2008, Jeff King wrote:
> On Mon, Jun 09, 2008 at 07:46:22AM +0200, Johan Herland wrote:
> > Is there a good reason against having *both*?
> >
> > AFAICS, there's nothing stopping us from having both a "-e"-option to
> > git-add, and an "e"-command inside git-add--interactive.
>
> I agree (and I tried to make that point in an earlier mail).
>
> And I was hoping the right way to do it was to simply build the
> interactive "e" command on top of Johannes' git-apply work. But I don't
> think that quite makes sense.

Yeah, the two approaches don't merge easily...

> His work is about fixing up the hunk header as we apply the patch, but a
> working "e" command in the hunk selection should probably not actually
> apply, but simply split into two hunks for the loop.

By "split into two hunks", you mean splitting the original "index -> 
worktree" hunk (#0) into one hunk that represents "index -> edited" (#1), 
and another hunk that represents "edited -> worktree" (#2)?

>From a technical POV this might make sense, but AFAICS, users would always 
want to answer 'y' to #1, and 'n' to #2 (see [1]), so from a user POV, 
git-add--interactive should simply stage #1, and drop #2.

(Side note: AFAIR, some of the original rationale for this feature was to 
provide a more fine-grained split than 's'. Looking at the problem from 
this POV: What is the reason for splitting a hunk in the first place? It 
must be because one part of the hunk should be staged while leaving the 
other unstaged. With 's', it just splits, and lets the user select which 
parts of the hunk to stage, using 'y'/'n'. But 'e' introduces a much more 
powerful notion of letting user split AND select in ONE operation (i.e. the 
editor). Therefore, when the user has already selected which parts of the 
hunk to stage (#1), it is not necessary to re-ask the user whether or not 
#1 should be staged (and certainly not #2).

> > ("git-add -e" would open the entire diff in an editor, as would "e"
> > from the *main* menu of git-add--interactive. However, "e" from the
> > *single hunk* menu would of course open only that single hunk within
> > the editor. We could even have an "E" command to open all
> > remaining/undecided hunks in an editor.)
>
> I agree with all of this, though I think the big question is what
> happens to the edited portion. In the interactive command, I think it
> becomes a new hunk that can be staged or not. In "git add -e" it makes
> sense to simply stage the result.

Sounds acceptable to me (although I would also be ok with automatically 
staging the edited portion in the interactive command).


Have fun! :)

...Johan


[1]: AIUI #1 represents the hunk that the user want to stage at this moment. 
Conversely, #2 represents the changes that the user is NOT interested in 
staging at this point. Therefore, the only answers that make sense is 'y' 
(i.e. "stage this hunk") for #1 and 'n' (i.e. "do not stage this hunk") for 
#2. The only problem with this is if the user screwed up the hunk edit and 
wants to revert to the original hunk (#0). I don't know if this is worth 
supporting.

-- 
Johan Herland, <johan@herland.net>
www.herland.net

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> But like I said, Junio convinced me that it makes not much sense to split 
> somewhere else than common lines, and you have that already with the "s" 
> command in add -i.

That needs clarifying.  What I would have convinced you is not quite
"somewhere else than common lines".  It is more like "don't split between
two deleted lines that is followed by addition, or two added lines that
immediately follow deletion, without thinking".

Splitting the left into the right:

         1               1
        -2              -2
        +two            +two
         3               3
        -4
        +four
         5

so that only the first change is applicable makes sense and can easily be
explained.  This is "common lines" case.

The example I gave you long time ago about split that does not make much
sense was to split this:

         1      
        -2      
        -3      
        -4
        +two, three
        +four
         5

anywhere between "-2 .. +four" without giving the user any other ways to
control the result.

But you _can_ give users a meaningful split by reordering lines.

         1      
        -2      
        -3      
        +two, three
        -4
        +four
         5

It makes sense to then split this immediately before "-4" to apply "only
the first change that is to remove two lines and rewrite it with one
line", i.e.:

         1      
        -2      
        -3      
        +two, three
         4

with the leftover part, whose lines differ if the above is applied or not,
presented as the next hunk to the user:

 (if the above is unused) (if the above is used)

         3                   two, three 
        -4                  -4          
        +four               +four       
         5                   5          

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

[Jeff King]

On Mon, Jun 09, 2008 at 07:31:51PM +0200, Johan Herland wrote:

> > His work is about fixing up the hunk header as we apply the patch, but a
> > working "e" command in the hunk selection should probably not actually
> > apply, but simply split into two hunks for the loop.
> 
> By "split into two hunks", you mean splitting the original "index -> 
> worktree" hunk (#0) into one hunk that represents "index -> edited" (#1), 
> and another hunk that represents "edited -> worktree" (#2)?

I mean splitting the original "index -> worktree" hunk into two other
hunks, each of which is "index -> worktree" but which can be staged
separately. I.e., what the 's'plit command does, but with finer-grained
control.

But I think that is what you are trying to say...

> From a technical POV this might make sense, but AFAICS, users would always 
> want to answer 'y' to #1, and 'n' to #2 (see [1]), so from a user POV, 
> git-add--interactive should simply stage #1, and drop #2.

Yes. I assumed we wanted to maintain the separate splitting operation,
since that parallels the existing split (so the interface is consistent)
and it logically separates the two parts (you split, and then you choose
the part you want).

But honestly, I don't really see a use case that isn't covered by
"manually edit the diff and apply the hunk". And the rationale in your
"side note" indicates that you think the same way.

So now I wonder if we _can_ leverage Dscho's work here. I.e., can we
simply send the edited hunk to "git apply --recount --cached" (instead
of doing a "git apply --check")?

The main problem I see at this point is that it screws up the line
numbering for _every other hunk_, so later hunks in that file might not
apply (IIRC, we usually save up all of the "yes" hunks and apply at the
end). So it might be needed to do a --recount --check, and then actually
apply at the end.

I'll try to play around with that.

-Peff

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

[Johan Herland]

On Monday 09 June 2008, Jeff King wrote:
> But honestly, I don't really see a use case that isn't covered by
> "manually edit the diff and apply the hunk". And the rationale in your
> "side note" indicates that you think the same way.

Agreed.

> I'll try to play around with that.

Looking forward to play with the result.


Have fun! :)

...Johan


-- 
Johan Herland, <johan@herland.net>
www.herland.net

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

[Jeff King]

On Mon, Jun 09, 2008 at 11:19:23PM +0200, Johan Herland wrote:

> On Monday 09 June 2008, Jeff King wrote:
> > But honestly, I don't really see a use case that isn't covered by
> > "manually edit the diff and apply the hunk". And the rationale in your
> > "side note" indicates that you think the same way.

Below is a simple patch to build interactive edit support on top of
Dscho's "git apply --recount" patch. Rather than create new hunks, the
action is just "edit and apply": if the apply is successful, the hunk is
removed from further consideration. This is just for playing with, and
not for commit:

  - it's not very well tested :)

  - I think the recount stuff may have flaws; I didn't follow the thread
    too closely, but Junio seemed to have a lot of comments (I am
    working under the assumption that Dscho will get that part right,
    and we can just build on top of it)

  - I tried one or two simple tests where I edited and applied an early
    hunk, and then resumed the loop for later hunks. Everything seemed
    to work, but I am not convinced that it isn't possible to make the
    rest of the hunk selection loop useless by applying early and
    invalidating all of the other hunks. That might be a flaw with this
    approach. A more sane interface might be to simply jump from the
    hunk selection loop into manually editing _all_ hunks and then
    applying, ending the hunk selection loop.

This should be applied on top of "allow git-apply to ignore the hunk
headers" (v3) which can be found on the list.  Obviously significant
portions of the patch below are based on Thomas' version.

-- >8 --
git-add--interactive: manual hunk editing mode

Adds a new option 'e' to the 'add -p' command loop that lets you
edit the current hunk in your favourite editor, applying the
result.
---
 git-add--interactive.perl |   81 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 81 insertions(+), 0 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 903953e..62b5aed 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -2,6 +2,7 @@
 
 use strict;
 use Git;
+use File::Temp;
 
 my $repo = Git->repository();
 
@@ -770,6 +771,76 @@ sub coalesce_overlapping_hunks {
 	return @out;
 }
 
+sub edit_hunk_manually {
+	my ($oldtext) = @_;
+
+	my $t = File::Temp->new(
+		TEMPLATE => $repo->repo_path . "/git-hunk-edit.XXXXXX",
+		SUFFIX => '.diff'
+	);
+	print $t "# Manual hunk edit mode -- see bottom for a quick guide\n";
+	print $t @$oldtext;
+	print $t <<EOF;
+# ---
+# To remove '-' lines, make them ' ' lines (context).
+# To remove '+' lines, delete them.
+# Empty lines and lines starting with # will be removed.
+#
+# Lines starting with @ start a new hunk. Line counts will be adjusted
+# according to contents. If the line numbers are missing altogether,
+# they will be inferred from the previous hunk.
+#
+# You can change the hunk to your heart's content, but it will be
+# refused if the end result (the entire patch including your edited
+# hunk) does not apply cleanly.
+EOF
+	close $t;
+
+	my $editor = $ENV{GIT_EDITOR} || $repo->config("core.editor")
+		|| $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+	system('sh', '-c', $editor.' "$@"', $editor, $t);
+
+	open my $fh, '<', $t
+		or die "failed to open hunk edit file for reading: " . $!;
+	my @newtext = grep { !/^#/ } <$fh>;
+	close $fh;
+
+	# Reinsert the first hunk header if the user accidentally deleted it
+	if ($newtext[0] !~ /^@/) {
+		unshift @newtext, $oldtext->[0];
+	}
+	return \@newtext;
+}
+
+sub apply_diff {
+	open(my $fh, '|-', qw(git apply --cached --recount));
+	print $fh map { @$_ } @_;
+	return close $fh;
+}
+
+sub prompt_yesno {
+	my ($prompt) = @_;
+	while (1) {
+		print colored $prompt_color, $prompt;
+		my $line = <STDIN>;
+		return 0 if $line =~ /^n/i;
+		return 1 if $line =~ /^y/i;
+	}
+}
+
+sub edit_hunk_loop {
+	my ($head, $text) = @_;
+
+	while (1) {
+		$text = edit_hunk_manually($text);
+		apply_diff($head, $text) and return 1;
+		prompt_yesno(
+			'Your edited hunk does not apply. Edit again '
+			. '(saying "no" discards!) [y/n]? '
+		) or return 0;
+	}
+}
+
 sub help_patch_cmd {
 	print colored $help_color, <<\EOF ;
 y - stage this hunk
@@ -781,6 +852,7 @@ J - leave this hunk undecided, see next hunk
 k - leave this hunk undecided, see previous undecided hunk
 K - leave this hunk undecided, see previous hunk
 s - split the current hunk into smaller hunks
+e - manually edit the current hunk
 ? - print help
 EOF
 }
@@ -885,6 +957,7 @@ sub patch_update_file {
 		if (hunk_splittable($hunk[$ix]{TEXT})) {
 			$other .= '/s';
 		}
+		$other .= '/e';
 		for (@{$hunk[$ix]{DISPLAY}}) {
 			print;
 		}
@@ -949,6 +1022,14 @@ sub patch_update_file {
 				$num = scalar @hunk;
 				next;
 			}
+			elsif ($line =~ /^e/) {
+				if (edit_hunk_loop($head->{TEXT},
+						   $hunk[$ix]{TEXT})) {
+					splice @hunk, $ix, 1;
+					$num = @hunk;
+				}
+				next;
+			}
 			else {
 				help_patch_cmd($other);
 				next;
-- 
1.5.6.rc2.26.gda075.dirty

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

[Andreas Ericsson]

Jeff King wrote:
> 
> The main problem I see at this point is that it screws up the line
> numbering for _every other hunk_, so later hunks in that file might not
> apply (IIRC, we usually save up all of the "yes" hunks and apply at the
> end). So it might be needed to do a --recount --check, and then actually
> apply at the end.
> 

I for one would really hate if the apply-at-the-end approach currently
used would be swapped around to "apply-as-we-go", because I sometimes
realize I'm committing a feature on top of a bugfix I've forgotten to
commit (or some such). I know I'd forget quite a few times to revert
the added changes after Ctrl-C'ing out of such cases if the current
approach is changed.

I know that wasn't what was proposed but at least anyone who
proposes it will at least think twice now before doing so, and
hopefully providing a SIGHUP hook to restore the index+worktree
to exactly what they were at the start of the interactive session.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

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

[Thomas Rast]

Jeff King wrote:
>
> Below is a simple patch to build interactive edit support on top of
> Dscho's "git apply --recount" patch. Rather than create new hunks, the
> action is just "edit and apply": if the apply is successful, the hunk is
> removed from further consideration.

Ok, I finally see what you meant earlier.  So how about this, which is
basically "middle ground":

As in the parent, editing is restricted to a single hunk, no splitting
allowed (almost---you can still insert @ lines if you are good at
counting, but you're on your own).

However, as in the earlier versions, the hunk is placed in the editing
loop again.  In this version, I made it so it will be marked for use
as if you had entered 'y', so if it was the last (or only) hunk, the
files will be patched immediately.  But if you Ctrl-C out in the
middle, nothing has been changed yet.

The following two remarks also apply:

>   - it's not very well tested :)
[...]
> This should be applied on top of "allow git-apply to ignore the hunk
> headers" (v3) which can be found on the list.

By the way, current 'add -p' recounts the headers to account for all
hunks that the user answered 'n'.  I haven't given it enough thought
to put it in the code yet, but it may be possible to rip that out as
well and unconditionally --recount.  Only the preimage line numbers
matter, and those never change.

-- 8< --
 git-add--interactive.perl |  128 ++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 127 insertions(+), 1 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 903953e..8c39149 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -2,6 +2,7 @@
 
 use strict;
 use Git;
+use File::Temp;
 
 my $repo = Git->repository();
 
@@ -18,6 +19,18 @@ my ($fraginfo_color) =
 	$diff_use_color ? (
 		$repo->get_color('color.diff.frag', 'cyan'),
 	) : ();
+my ($diff_plain_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.plain', ''),
+	) : ();
+my ($diff_old_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.old', 'red'),
+	) : ();
+my ($diff_new_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.new', 'green'),
+	) : ();
 
 my $normal_color = $repo->get_color("", "reset");
 
@@ -770,6 +783,108 @@ sub coalesce_overlapping_hunks {
 	return @out;
 }
 
+sub color_diff {
+	return map {
+		colored((/^@/  ? $fraginfo_color :
+			 /^\+/ ? $diff_new_color :
+			 /^-/  ? $diff_old_color :
+			 $diff_plain_color),
+			$_);
+	} @_;
+}
+
+sub edit_hunk_manually {
+	my ($oldtext) = @_;
+
+	my $t = File::Temp->new(
+		TEMPLATE => $repo->repo_path . "/git-hunk-edit.XXXXXX",
+		SUFFIX => '.diff'
+	);
+	print $t "# Manual hunk edit mode -- see bottom for a quick guide\n";
+	print $t @$oldtext;
+	print $t <<EOF;
+# ---
+# To remove '-' lines, make them ' ' lines (context).
+# To remove '+' lines, delete them.
+# Lines starting with # will be removed.
+#
+# If the patch applies cleanly, the hunk will immediately be marked
+# for staging as if you had answered 'y'.  (However, if you remove
+# everything, nothing happens.)  Otherwise, you will be asked to edit
+# again.
+#
+# Do not add @ lines unless you know what you are doing.  The original
+# @ line will be reinserted if you remove it.
+EOF
+	close $t;
+
+	my $editor = $ENV{GIT_EDITOR} || $repo->config("core.editor")
+		|| $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+	system('sh', '-c', $editor.' "$@"', $editor, $t);
+
+	open my $fh, '<', $t
+		or die "failed to open hunk edit file for reading: " . $!;
+	my @newtext = grep { !/^#/ } <$fh>;
+	close $fh;
+
+	# Abort if nothing remains
+	if (@newtext == 0) {
+		return undef;
+	}
+
+	# Reinsert the first hunk header if the user accidentally deleted it
+	if ($newtext[0] !~ /^@/) {
+		unshift @newtext, $oldtext->[0];
+	}
+	return \@newtext;
+}
+
+sub diff_applies {
+	my $fh;
+	open $fh, '| git apply --recount --cached --check';
+	for my $h (@_) {
+		print $fh @{$h->{TEXT}};
+	}
+	return close $fh;
+}
+
+sub prompt_yesno {
+	my ($prompt) = @_;
+	while (1) {
+		print colored $prompt_color, $prompt;
+		my $line = <STDIN>;
+		return 0 if $line =~ /^n/i;
+		return 1 if $line =~ /^y/i;
+	}
+}
+
+sub edit_hunk_loop {
+	my ($head, $hunk, $ix) = @_;
+	my $text = $hunk->[$ix]->{TEXT};
+
+	while (1) {
+		$text = edit_hunk_manually($text);
+		if (!defined $text) {
+			return undef;
+		}
+		my $newhunk = { TEXT => $text, USE => 1 };
+		if (diff_applies($head,
+				 @{$hunk}[0..$ix-1],
+				 $newhunk,
+				 @{$hunk}[$ix+1..$#{$hunk}])) {
+			my @display = color_diff(@{$text});
+			$newhunk->{DISPLAY} = \@display;
+			return $newhunk;
+		}
+		else {
+			prompt_yesno(
+				'Your edited hunk does not apply. Edit again '
+				. '(saying "no" discards!) [y/n]? '
+				) or return undef;
+		}
+	}
+}
+
 sub help_patch_cmd {
 	print colored $help_color, <<\EOF ;
 y - stage this hunk
@@ -781,6 +896,7 @@ J - leave this hunk undecided, see next hunk
 k - leave this hunk undecided, see previous undecided hunk
 K - leave this hunk undecided, see previous hunk
 s - split the current hunk into smaller hunks
+e - manually edit the current hunk
 ? - print help
 EOF
 }
@@ -846,6 +962,7 @@ sub patch_update_file {
 
 	$num = scalar @hunk;
 	$ix = 0;
+	my $need_recount = 0;
 
 	while (1) {
 		my ($prev, $next, $other, $undecided, $i);
@@ -885,6 +1002,7 @@ sub patch_update_file {
 		if (hunk_splittable($hunk[$ix]{TEXT})) {
 			$other .= '/s';
 		}
+		$other .= '/e';
 		for (@{$hunk[$ix]{DISPLAY}}) {
 			print;
 		}
@@ -949,6 +1067,13 @@ sub patch_update_file {
 				$num = scalar @hunk;
 				next;
 			}
+			elsif ($line =~ /^e/) {
+				my $newhunk = edit_hunk_loop($head, \@hunk, $ix);
+				if (defined $newhunk) {
+					splice @hunk, $ix, 1, $newhunk;
+					$need_recount = 1;
+				}
+			}
 			else {
 				help_patch_cmd($other);
 				next;
@@ -1002,7 +1127,8 @@ sub patch_update_file {
 	if (@result) {
 		my $fh;
 
-		open $fh, '| git apply --cached';
+		open $fh, '| git apply --cached'
+			. ($need_recount ? ' --recount' : '');
 		for (@{$head->{TEXT}}, @result) {
 			print $fh $_;
 		}
-- 
1.5.6.rc1.134.g8dfb.dirty

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

[Jeff King]

On Wed, Jun 11, 2008 at 11:02:29AM +0200, Thomas Rast wrote:

> However, as in the earlier versions, the hunk is placed in the editing
> loop again.  In this version, I made it so it will be marked for use
> as if you had entered 'y', so if it was the last (or only) hunk, the
> files will be patched immediately.  But if you Ctrl-C out in the
> middle, nothing has been changed yet.

OK, I really think this is the most sane behavior. It naturally follows
what the user wants (if they edit, they then want to apply), but it
still waits until the end of the loop, so they can back out if they
want.

> By the way, current 'add -p' recounts the headers to account for all
> hunks that the user answered 'n'.  I haven't given it enough thought
> to put it in the code yet, but it may be possible to rip that out as
> well and unconditionally --recount.  Only the preimage line numbers
> matter, and those never change.

Ah, I hadn't thought about that, but of course it makes sense that it
would need to recount (I guess I should have looked a little more
closely at the other code). I think replacing that with --recount would
be great, but it should be a separate patch, and we should wait for
--recount to stabilize (all of this should be post 1.5.6, anyway).

> +# If the patch applies cleanly, the hunk will immediately be marked
> +# for staging as if you had answered 'y'.  (However, if you remove
> +# everything, nothing happens.)  Otherwise, you will be asked to edit
> +# again.

This "however" confused me the first time I read it. I assume you mean
that "if the diff is empty, then staging it will do nothing"? I wonder
if that is even worth mentioning, since it seems obvious.

Although I guess you do special-case "no lines" later on.

> +# Do not add @ lines unless you know what you are doing.  The original
> +# @ line will be reinserted if you remove it.

This can probably be moved from the "every time" instructions to the
manual, if we want to keep the size of the instructions a bit smaller.

> +		if (diff_applies($head,
> +				 @{$hunk}[0..$ix-1],
> +				 $newhunk,
> +				 @{$hunk}[$ix+1..$#{$hunk}])) {

I'm confused here...we are feeding _all_ of the hunks to git-apply. In
my patch, I simply fed the hunk of interest. Since we are recounting,
shouldn't the single hunk be enough? And if it isn't, then shouldn't we
be feeding only the hunks that are to be applied?

> +			my @display = color_diff(@{$text});
> +			$newhunk->{DISPLAY} = \@display;

Perl nit: you can create an arrayref with brackets:

  $newhunk->{DISPLAY} = [color_diff(@$text)];

> +		open $fh, '| git apply --cached'
> +			. ($need_recount ? ' --recount' : '');

Nice. I think this is much cleaner.

-Peff

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

[Thomas Rast]

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

Jeff King wrote:
> On Wed, Jun 11, 2008 at 11:02:29AM +0200, Thomas Rast wrote:
> 
> > +# If the patch applies cleanly, the hunk will immediately be marked
> > +# for staging as if you had answered 'y'.  (However, if you remove
> > +# everything, nothing happens.)  Otherwise, you will be asked to edit
> > +# again.
> 
> This "however" confused me the first time I read it. I assume you mean
> that "if the diff is empty, then staging it will do nothing"? I wonder
> if that is even worth mentioning, since it seems obvious.

I wanted to make the same special case that 'recount -i' has: Deleting
everything does not "do nothing", but actually aborts the edit.  So in
rebase, deleting everything will not rebase anything (instead of
rebasing "no patches" onto wherever you said).  Along the same lines,
in the above patch deleting everything does not patch "no changes",
but aborts editing the hunk and leaves it unchanged.

Do you think that behaviour confuses users?

> > +# Do not add @ lines unless you know what you are doing.  The original
> > +# @ line will be reinserted if you remove it.
> 
> This can probably be moved from the "every time" instructions to the
> manual, if we want to keep the size of the instructions a bit smaller.
> 
> > +		if (diff_applies($head,
> > +				 @{$hunk}[0..$ix-1],
> > +				 $newhunk,
> > +				 @{$hunk}[$ix+1..$#{$hunk}])) {
> 
> I'm confused here...we are feeding _all_ of the hunks to git-apply. In
> my patch, I simply fed the hunk of interest. Since we are recounting,
> shouldn't the single hunk be enough? And if it isn't, then shouldn't we
> be feeding only the hunks that are to be applied?

Feeding it the single hunk would be enough, but doing it this way
ensures that the edited hunk cannot step on another hunk's toes,
i.e. produce a conflict that may prevent us from applying the patch at
the end of the hunk selection loop.

It's fairly unlikely of course, because the user would deliberately
have to extend the hunk beyond its current borders by adding extra
context from the original file.

-- 
Thomas Rast
trast@student.ethz.ch


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

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

[Jeff King]

On Thu, Jun 12, 2008 at 08:55:24AM +0200, Thomas Rast wrote:

> > > +# If the patch applies cleanly, the hunk will immediately be marked
> > > +# for staging as if you had answered 'y'.  (However, if you remove
> > > +# everything, nothing happens.)  Otherwise, you will be asked to edit
> > > +# again.
> [...]
> I wanted to make the same special case that 'recount -i' has: Deleting
> everything does not "do nothing", but actually aborts the edit.  So in
> rebase, deleting everything will not rebase anything (instead of
> rebasing "no patches" onto wherever you said).  Along the same lines,
> in the above patch deleting everything does not patch "no changes",
> but aborts editing the hunk and leaves it unchanged.
> 
> Do you think that behaviour confuses users?

Ah, OK. That makes sense. I think it could be worded more explicitly,
though. Maybe:

  If the patch applies cleanly, the edited hunk will immediately be
  marked for staging. If it does not apply cleanly, you will be given an
  opportunity to edit again. If all lines of the hunk are removed, then
  the edit is aborted and the hunk is left unchanged.

> Feeding it the single hunk would be enough, but doing it this way
> ensures that the edited hunk cannot step on another hunk's toes,
> i.e. produce a conflict that may prevent us from applying the patch at
> the end of the hunk selection loop.
> 
> It's fairly unlikely of course, because the user would deliberately
> have to extend the hunk beyond its current borders by adding extra
> context from the original file.

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.

-Peff

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

[Thomas Rast]

Adds a new option 'e' to the 'add -p' command loop that lets you edit
the current hunk in your favourite editor.

If the resulting patch applies cleanly, the edited hunk will
immediately be marked for staging. If it does not apply cleanly, you
will be given an opportunity to edit again. If all lines of the hunk
are removed, then the edit is aborted and the hunk is left unchanged.

Applying the changed hunk(s) relies on Johannes Schindelin's new
--recount option for git-apply.

Note that the "real patch" test intentionally uses
  (echo e; echo n; echo d) | git add -p
even though the 'n' and 'd' are superfluous at first sight.  They
serve to get out of the interaction loop if git add -p wrongly
concludes the patch does not apply.

Many thanks to Jeff King <peff@peff.net> for lots of help and
suggestions.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

Once again trying to shape this up as a complete patch.

Jeff King wrote:
> On Thu, Jun 12, 2008 at 08:55:24AM +0200, Thomas Rast wrote:
> 
> > I wanted to make the same special case that 'recount -i' has: Deleting
> > everything does not "do nothing", but actually aborts the edit.
[...]
> Ah, OK. That makes sense. I think it could be worded more explicitly,
> though. [...]

I took the liberty of copying your wording to the help and commit
messages, as it is indeed much clearer that way.

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


Changes since the last version:

* The help message mentioned above

* Use [f(...)] instead of @tmp = f(); \@tmp

* Re-added tests from v3, with an added test for a correctly formatted
  patch that does not apply

* Re-added a line about 'e' to Documentation/git-add.txt (which was in
  the original patch but got lost)

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.

- Thomas
  

 Documentation/git-add.txt  |    1 +
 git-add--interactive.perl  |  124 +++++++++++++++++++++++++++++++++++++++++++-
 t/t3701-add-interactive.sh |   67 ++++++++++++++++++++++++
 3 files changed, 191 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 8620ae2..842430c 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -238,6 +238,7 @@ patch::
        k - leave this hunk undecided, see previous undecided hunk
        K - leave this hunk undecided, see previous hunk
        s - split the current hunk into smaller hunks
+       e - manually edit the current hunk
        ? - print help
 +
 After deciding the fate for all hunks, if there is any hunk
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 903953e..8cc275d 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -2,6 +2,7 @@
 
 use strict;
 use Git;
+use File::Temp;
 
 my $repo = Git->repository();
 
@@ -18,6 +19,18 @@ my ($fraginfo_color) =
 	$diff_use_color ? (
 		$repo->get_color('color.diff.frag', 'cyan'),
 	) : ();
+my ($diff_plain_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.plain', ''),
+	) : ();
+my ($diff_old_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.old', 'red'),
+	) : ();
+my ($diff_new_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.new', 'green'),
+	) : ();
 
 my $normal_color = $repo->get_color("", "reset");
 
@@ -770,6 +783,104 @@ sub coalesce_overlapping_hunks {
 	return @out;
 }
 
+sub color_diff {
+	return map {
+		colored((/^@/  ? $fraginfo_color :
+			 /^\+/ ? $diff_new_color :
+			 /^-/  ? $diff_old_color :
+			 $diff_plain_color),
+			$_);
+	} @_;
+}
+
+sub edit_hunk_manually {
+	my ($oldtext) = @_;
+
+	my $t = File::Temp->new(
+		TEMPLATE => $repo->repo_path . "/git-hunk-edit.XXXXXX",
+		SUFFIX => '.diff'
+	);
+	print $t "# Manual hunk edit mode -- see bottom for a quick guide\n";
+	print $t @$oldtext;
+	print $t <<EOF;
+# ---
+# To remove '-' lines, make them ' ' lines (context).
+# To remove '+' lines, delete them.
+# Lines starting with # will be removed.
+#
+# If the patch applies cleanly, the edited hunk will immediately be
+# marked for staging. If it does not apply cleanly, you will be given
+# an opportunity to edit again. If all lines of the hunk are removed,
+# then the edit is aborted and the hunk is left unchanged.
+EOF
+	close $t;
+
+	my $editor = $ENV{GIT_EDITOR} || $repo->config("core.editor")
+		|| $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+	system('sh', '-c', $editor.' "$@"', $editor, $t);
+
+	open my $fh, '<', $t
+		or die "failed to open hunk edit file for reading: " . $!;
+	my @newtext = grep { !/^#/ } <$fh>;
+	close $fh;
+
+	# Abort if nothing remains
+	if (@newtext == 0) {
+		return undef;
+	}
+
+	# Reinsert the first hunk header if the user accidentally deleted it
+	if ($newtext[0] !~ /^@/) {
+		unshift @newtext, $oldtext->[0];
+	}
+	return \@newtext;
+}
+
+sub diff_applies {
+	my $fh;
+	open $fh, '| git apply --recount --cached --check';
+	for my $h (@_) {
+		print $fh @{$h->{TEXT}};
+	}
+	return close $fh;
+}
+
+sub prompt_yesno {
+	my ($prompt) = @_;
+	while (1) {
+		print colored $prompt_color, $prompt;
+		my $line = <STDIN>;
+		return 0 if $line =~ /^n/i;
+		return 1 if $line =~ /^y/i;
+	}
+}
+
+sub edit_hunk_loop {
+	my ($head, $hunk, $ix) = @_;
+	my $text = $hunk->[$ix]->{TEXT};
+
+	while (1) {
+		$text = edit_hunk_manually($text);
+		if (!defined $text) {
+			return undef;
+		}
+		my $newhunk = { TEXT => $text, USE => 1 };
+		if (diff_applies($head,
+				 @{$hunk}[0..$ix-1],
+				 $newhunk,
+				 @{$hunk}[$ix+1..$#{$hunk}])) {
+			$newhunk->{DISPLAY} = [color_diff(@{$text})];
+			return $newhunk;
+		}
+		else {
+			prompt_yesno(
+				'Your edited hunk does not apply. Edit again '
+				. '(saying "no" discards!) [y/n]? '
+				) or return undef;
+		}
+	}
+}
+
 sub help_patch_cmd {
 	print colored $help_color, <<\EOF ;
 y - stage this hunk
@@ -781,6 +892,7 @@ J - leave this hunk undecided, see next hunk
 k - leave this hunk undecided, see previous undecided hunk
 K - leave this hunk undecided, see previous hunk
 s - split the current hunk into smaller hunks
+e - manually edit the current hunk
 ? - print help
 EOF
 }
@@ -846,6 +958,7 @@ sub patch_update_file {
 
 	$num = scalar @hunk;
 	$ix = 0;
+	my $need_recount = 0;
 
 	while (1) {
 		my ($prev, $next, $other, $undecided, $i);
@@ -885,6 +998,7 @@ sub patch_update_file {
 		if (hunk_splittable($hunk[$ix]{TEXT})) {
 			$other .= '/s';
 		}
+		$other .= '/e';
 		for (@{$hunk[$ix]{DISPLAY}}) {
 			print;
 		}
@@ -949,6 +1063,13 @@ sub patch_update_file {
 				$num = scalar @hunk;
 				next;
 			}
+			elsif ($line =~ /^e/) {
+				my $newhunk = edit_hunk_loop($head, \@hunk, $ix);
+				if (defined $newhunk) {
+					splice @hunk, $ix, 1, $newhunk;
+					$need_recount = 1;
+				}
+			}
 			else {
 				help_patch_cmd($other);
 				next;
@@ -1002,7 +1123,8 @@ sub patch_update_file {
 	if (@result) {
 		my $fh;
 
-		open $fh, '| git apply --cached';
+		open $fh, '| git apply --cached'
+			. ($need_recount ? ' --recount' : '');
 		for (@{$head->{TEXT}}, @result) {
 			print $fh $_;
 		}
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index fae64ea..e95663d 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -66,6 +66,73 @@ test_expect_success 'revert works (commit)' '
 	grep "unchanged *+3/-0 file" output
 '
 
+cat >expected <<EOF
+EOF
+cat >fake_editor.sh <<EOF
+EOF
+chmod a+x fake_editor.sh
+test_set_editor "$(pwd)/fake_editor.sh"
+test_expect_success 'dummy edit works' '
+	(echo e; echo a) | git add -p &&
+	git diff > diff &&
+	test_cmp expected diff
+'
+
+cat >patch <<EOF
+@@ -1,1 +1,4 @@
+ this
++patch
+-doesn't
+ apply
+EOF
+echo "#!$SHELL_PATH" >fake_editor.sh
+cat >>fake_editor.sh <<\EOF
+mv -f "$1" oldpatch &&
+mv -f patch "$1"
+EOF
+chmod a+x fake_editor.sh
+test_set_editor "$(pwd)/fake_editor.sh"
+test_expect_success 'bad edit rejected' '
+	git reset &&
+	(echo e; echo n; echo d) | git add -p >output &&
+	grep "hunk does not apply" output
+'
+
+cat >patch <<EOF
+this patch
+is garbage
+EOF
+test_expect_success 'garbage edit rejected' '
+	git reset &&
+	(echo e; echo n; echo d) | git add -p >output &&
+	grep "hunk does not apply" output
+'
+
+cat >patch <<EOF
+@@ -1,0 +1,0 @@
+ baseline
++content
++newcontent
++lines
+EOF
+cat >expected <<EOF
+diff --git a/file b/file
+index b5dd6c9..f910ae9 100644
+--- a/file
++++ b/file
+@@ -1,4 +1,4 @@
+ baseline
+ content
+-newcontent
++more
+ lines
+EOF
+test_expect_success 'real edit works' '
+	(echo e; echo n; echo d) | git add -p &&
+	git diff >output &&
+	test_cmp expected output
+'
+
 if test "$(git config --bool core.filemode)" = false
 then
     say 'skipping filemode tests (filesystem does not properly support modes)'
--
1.5.6.rc1.138.gb2840

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

[Jeff King]

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

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

[Johannes Schindelin]

Hi,

On Mon, 23 Jun 2008, Jeff King wrote:

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

Oh, so much to do.  I have 55 patches in my personal fork, on top of 
'next'.  And a few of them need resubmitting, such as the initHook work, 
and, yes, add --edit.

I am not sure when I will have time for that (particularly given that I 
got sidetracked with the OPTION_OPTIONS patch, when I should have worked 
on something completely different).

In the meantime, feel free to submit in my name.

Ciao,
Dscho

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

[Jeff King]

On Mon, Jun 23, 2008 at 07:54:29PM +0100, Johannes Schindelin wrote:

> I am not sure when I will have time for that (particularly given that I 
> got sidetracked with the OPTION_OPTIONS patch, when I should have worked 
> on something completely different).

Heh, I know the feeling. ;)

> In the meantime, feel free to submit in my name.

Do you remember off the top of your head whether more work was needed?
They looked good to me, but then I seem to recall you saying in another
thread that Junio pointed out some flaw with your approach (but I never
quite understood what the flaw was).

-Peff

apply --recount, was Re: [PATCH v4] git-add--interactive: manual hunk editing mode

[Johannes Schindelin]

Hi,

On Mon, 23 Jun 2008, Jeff King wrote:

> On Mon, Jun 23, 2008 at 07:54:29PM +0100, Johannes Schindelin wrote:
> 
> > I am not sure when I will have time for that (particularly given that 
> > I got sidetracked with the OPTION_OPTIONS patch, when I should have 
> > worked on something completely different).
> 
> Heh, I know the feeling. ;)

Yes, us mere mortals cannot hack on Linux and Git all the time, much as 
we would like to (be able to).

> > In the meantime, feel free to submit in my name.
> 
> Do you remember off the top of your head whether more work was needed? 
> They looked good to me, but then I seem to recall you saying in another 
> thread that Junio pointed out some flaw with your approach (but I never 
> quite understood what the flaw was).

I think it was not a flaw, but something to be worried about: 

http://mid.gmane.org/7v4p87zcv6.fsf@gitster.siamese.dyndns.org

To spare you following that link: Junio wanted to reuse "git apply 
--recount" to apply mboxes, where a separator "^-- $" to the signature is 
quite common, and could be mistaken for a "-" line of a hunk.

I am not quite sure how to deal with this; "--recount=nosig" (assume there 
is no signature) and "--recount=stripsig" (assume that "^-- $" is a 
lead-in to a signature) come to mind, defaulting to "nosig" with a 
warning.

However, I think that this issue should not concern us _now_.  As long as 
--recount is only to be used in "add -i" and "add -e", I think the patch 
is good as is:

http://repo.or.cz/w/git/dscho.git?a=commitdiff;h=c95d4b3da2e5f595f600720b91b8e396b43fe0ae

Ciao,
Dscho

Re: apply --recount, was Re: [PATCH v4] git-add--interactive: manual hunk editing mode

[Jeff King]

On Mon, Jun 23, 2008 at 10:16:08PM +0100, Johannes Schindelin wrote:

> I think it was not a flaw, but something to be worried about: 
> 
> http://mid.gmane.org/7v4p87zcv6.fsf@gitster.siamese.dyndns.org
> 
> To spare you following that link: Junio wanted to reuse "git apply 
> --recount" to apply mboxes, where a separator "^-- $" to the signature is 
> quite common, and could be mistaken for a "-" line of a hunk.

Ah, OK. Thanks for the recap, that makes sense to me now.

> However, I think that this issue should not concern us _now_.  As long as 
> --recount is only to be used in "add -i" and "add -e", I think the patch 
> is good as is:

I agree. We can worry about the other when and if somebody decides to
use it that way (but maybe Junio is already planning to do so...).

Thomas, do you want to just re-submit the "--recount" patches when you
re-submit your patch?

-Peff

[PATCH 0/3] Manual editing for 'add' and 'add -p'

[Thomas Rast]

Jeff King wrote:
> Thomas, do you want to just re-submit the "--recount" patches when you
> re-submit your patch?

If that is the right thing to do :-)

I rebased Johannes' patches to current 'next', which had some trivial
merge conflicts, and made one actual change: By analogy with one
earlier patch that caused some of the conflicts, I split '-e' and
'--edit' across two lines in the documentation.

I also integrated your last suggestion:

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

I'm not really convinced it is needed, but any such patch can
obviously not change anything, so it can't hurt.  Who knows, perhaps
there are editors brain-damaged enough to always save at least one
newline.  (This is the only change to my patch compared to v4.)

Johannes Schindelin wrote:
>
> To spare you following that link: Junio wanted to reuse "git apply 
> --recount" to apply mboxes, where a separator "^-- $" to the signature is 
> quite common, and could be mistaken for a "-" line of a hunk.
[...]
> However, I think that this issue should not concern us _now_.  As long as 
> --recount is only to be used in "add -i" and "add -e", I think the patch 
> is good as is:

I'm wondering if this should be turned into docs.  After all, it's not
some DWIM option.  It just says that instead of trusting the counts,
lines starting with "diff " or "@@ " start hunks, implying that all
/^[-+ ]/ lines in between are significant.


I hope this series turns out right in mail; I'm struggling a bit to
properly import the mails to KMail.  Most annoyingly, format-patch
apparently cannot turn other people's patches into mails by myself
with an appropriate "From:" line. :-(

- Thomas


Johannes Schindelin (2):
  Allow git-apply to ignore the hunk headers (AKA recountdiff)
  git-add: introduce --edit (to edit the diff vs. the index)

Thomas Rast (1):
  git-add--interactive: manual hunk editing mode

 Documentation/git-add.txt   |   13 ++++-
 Documentation/git-apply.txt |    7 ++-
 builtin-add.c               |   55 ++++++++++++++++++-
 builtin-apply.c             |   64 ++++++++++++++++++++--
 git-add--interactive.perl   |  124 +++++++++++++++++++++++++++++++++++++++++-
 t/t3701-add-interactive.sh  |   67 +++++++++++++++++++++++
 t/t3702-add-edit.sh         |  126 +++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 448 insertions(+), 8 deletions(-)
 create mode 100755 t/t3702-add-edit.sh

[PATCH 1/3] Allow git-apply to ignore the hunk headers (AKA recountdiff)

[Thomas Rast]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Sometimes, the easiest way to fix up a patch is to edit it directly, even
adding or deleting lines.  Now, many people are not as divine as certain
benevolent dictators as to update the hunk headers correctly at the first
try.

So teach the tool to do it for us.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 Documentation/git-apply.txt |    7 ++++-
 builtin-apply.c             |   64 ++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 66 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index c834763..b8ab6ed 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -12,7 +12,7 @@ SYNOPSIS
 'git-apply' [--stat] [--numstat] [--summary] [--check] [--index]
 	  [--apply] [--no-add] [--build-fake-ancestor <file>] [-R | --reverse]
 	  [--allow-binary-replacement | --binary] [--reject] [-z]
-	  [-pNUM] [-CNUM] [--inaccurate-eof] [--cached]
+	  [-pNUM] [-CNUM] [--inaccurate-eof] [--recount] [--cached]
 	  [--whitespace=<nowarn|warn|fix|error|error-all>]
 	  [--exclude=PATH] [--verbose] [<patch>...]
 
@@ -171,6 +171,11 @@ behavior:
 	correctly. This option adds support for applying such patches by
 	working around this bug.
 
+--recount::
+	Do not trust the line counts in the hunk headers, but infer them
+	by inspecting the patch (e.g. after editing the patch without
+	adjusting the hunk headers appropriately).
+
 -v::
 --verbose::
 	Report progress to stderr. By default, only a message about the
diff --git a/builtin-apply.c b/builtin-apply.c
index c497889..34c220f 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -153,6 +153,7 @@ struct patch {
 	unsigned int is_binary:1;
 	unsigned int is_copy:1;
 	unsigned int is_rename:1;
+	unsigned int recount:1;
 	struct fragment *fragments;
 	char *result;
 	size_t resultsize;
@@ -882,6 +883,50 @@ static int parse_range(const char *line, int len, int offset, const char *expect
 	return offset + ex;
 }
 
+static int recount_diff(char *line, int size, struct fragment *fragment)
+{
+	int line_nr = 0;
+
+	if (size < 1)
+		return -1;
+
+	fragment->oldpos = 2;
+	fragment->oldlines = fragment->newlines = 0;
+
+	for (;;) {
+		int len = linelen(line, size);
+		size -= len;
+		line += len;
+
+		if (size < 1)
+			return 0;
+
+		switch (*line) {
+		case ' ': case '\n':
+			fragment->newlines++;
+			/* fall through */
+		case '-':
+			fragment->oldlines++;
+			break;
+		case '+':
+			fragment->newlines++;
+			if (line_nr == 0) {
+				fragment->leading = 1;
+				fragment->oldpos = 1;
+			}
+			fragment->trailing = 1;
+			break;
+		case '@':
+			return size < 3 || prefixcmp(line, "@@ ");
+		case 'd':
+			return size < 5 || prefixcmp(line, "diff ");
+		default:
+			return -1;
+		}
+		line_nr++;
+	}
+}
+
 /*
  * Parse a unified diff fragment header of the
  * form "@@ -a,b +c,d @@"
@@ -1013,6 +1058,9 @@ static int parse_fragment(char *line, unsigned long size,
 	offset = parse_fragment_header(line, len, fragment);
 	if (offset < 0)
 		return -1;
+	if (offset > 0 && patch->recount &&
+			recount_diff(line + offset, size - offset, fragment))
+		return -1;
 	oldlines = fragment->oldlines;
 	newlines = fragment->newlines;
 	leading = 0;
@@ -2912,7 +2960,8 @@ static void prefix_patches(struct patch *p)
 	}
 }
 
-static int apply_patch(int fd, const char *filename, int inaccurate_eof)
+static int apply_patch(int fd, const char *filename, int inaccurate_eof,
+		int recount)
 {
 	size_t offset;
 	struct strbuf buf;
@@ -2929,6 +2978,7 @@ static int apply_patch(int fd, const char *filename, int inaccurate_eof)
 
 		patch = xcalloc(1, sizeof(*patch));
 		patch->inaccurate_eof = inaccurate_eof;
+		patch->recount = recount;
 		nr = parse_chunk(buf.buf + offset, buf.len - offset, patch);
 		if (nr < 0)
 			break;
@@ -2998,6 +3048,7 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 	int i;
 	int read_stdin = 1;
 	int inaccurate_eof = 0;
+	int recount = 0;
 	int errs = 0;
 	int is_not_gitdir;
 
@@ -3015,7 +3066,8 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 		int fd;
 
 		if (!strcmp(arg, "-")) {
-			errs |= apply_patch(0, "<stdin>", inaccurate_eof);
+			errs |= apply_patch(0, "<stdin>", inaccurate_eof,
+					recount);
 			read_stdin = 0;
 			continue;
 		}
@@ -3118,6 +3170,10 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 			inaccurate_eof = 1;
 			continue;
 		}
+		if (!strcmp(arg, "--recount")) {
+			recount = 1;
+			continue;
+		}
 		if (0 < prefix_length)
 			arg = prefix_filename(prefix, prefix_length, arg);
 
@@ -3126,12 +3182,12 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 			die("can't open patch '%s': %s", arg, strerror(errno));
 		read_stdin = 0;
 		set_default_whitespace_mode(whitespace_option);
-		errs |= apply_patch(fd, arg, inaccurate_eof);
+		errs |= apply_patch(fd, arg, inaccurate_eof, recount);
 		close(fd);
 	}
 	set_default_whitespace_mode(whitespace_option);
 	if (read_stdin)
-		errs |= apply_patch(0, "<stdin>", inaccurate_eof);
+		errs |= apply_patch(0, "<stdin>", inaccurate_eof, recount);
 	if (whitespace_error) {
 		if (squelch_whitespace_errors &&
 		    squelch_whitespace_errors < whitespace_error) {
-- 
1.5.6.84.ge5c1

[PATCH 2/3] git-add: introduce --edit (to edit the diff vs. the index)

[Thomas Rast]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

With "git add -e [<files>]", Git will fire up an editor with the current
diff relative to the index (i.e. what you would get with "git diff
[<files>]").

Now you can edit the patch as much as you like, including adding/removing
lines, editing the text, whatever.  Make sure, though, that the first
character of the hunk lines is still a space, a plus or a minus.

After you closed the editor, Git will adjust the line counts of the
hunks if necessary, thanks to the --fixup-line-counts option of apply,
and commit the patch.  Except if you deleted everything, in which case
nothing happens (for obvious reasons).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 Documentation/git-add.txt |   12 ++++-
 builtin-add.c             |   55 +++++++++++++++++++-
 t/t3702-add-edit.sh       |  126 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 191 insertions(+), 2 deletions(-)
 create mode 100755 t/t3702-add-edit.sh

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index b8e3fa6..c6de028 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git-add' [-n] [-v] [--force | -f] [--interactive | -i] [--patch | -p]
-	  [--update | -u] [--refresh] [--ignore-errors] [--]
+	  [--edit | -e] [--update | -u] [--refresh] [--ignore-errors] [--]
 	  <filepattern>...
 
 DESCRIPTION
@@ -76,6 +76,16 @@ OPTIONS
 	bypassed and the 'patch' subcommand is invoked using each of
 	the specified filepatterns before exiting.
 
+-e::
+--edit::
+	Open the diff vs. the index in an editor and let the user
+	edit it.  After the editor was closed, adjust the hunk headers
+	and apply the patch to the index.
++
+*NOTE*: Obviously, if you change anything else than the first character
+on lines beginning with a space or a minus, the patch will no longer
+apply.
+
 -u::
 --update::
 	Update only files that git already knows about, staging modified
diff --git a/builtin-add.c b/builtin-add.c
index 9930cf5..b9e9a17 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -19,7 +19,7 @@ static const char * const builtin_add_usage[] = {
 	"git-add [options] [--] <filepattern>...",
 	NULL
 };
-static int patch_interactive = 0, add_interactive = 0;
+static int patch_interactive = 0, add_interactive = 0, edit_interactive = 0;
 static int take_worktree_changes;
 
 static void prune_directory(struct dir_struct *dir, const char **pathspec, int prefix)
@@ -186,6 +186,56 @@ int interactive_add(int argc, const char **argv, const char *prefix)
 	return status;
 }
 
+int edit_patch(int argc, const char **argv, const char *prefix)
+{
+	char *file = xstrdup(git_path("ADD_EDIT.patch"));
+	const char *apply_argv[] = { "apply", "--recount", "--cached",
+		file, NULL };
+	struct child_process child;
+	int result = 0, ac;
+	struct stat st;
+
+	memset(&child, 0, sizeof(child));
+	child.argv = xcalloc(sizeof(const char *), (argc + 5));
+	ac = 0;
+	child.git_cmd = 1;
+	child.argv[ac++] = "diff-files";
+	child.argv[ac++] = "--no-color";
+	child.argv[ac++] = "-p";
+	child.argv[ac++] = "--";
+	if (argc) {
+		const char **pathspec = validate_pathspec(argc, argv, prefix);
+		if (!pathspec)
+			return -1;
+		memcpy(&(child.argv[ac]), pathspec, sizeof(*argv) * argc);
+		ac += argc;
+	}
+	child.argv[ac] = NULL;
+	child.out = open(file, O_CREAT | O_WRONLY, 0644);
+	result = child.out < 0 && error("Could not write to '%s'", file);
+
+	if (!result)
+		result = run_command(&child);
+	free(child.argv);
+
+	launch_editor(file, NULL, NULL);
+
+	if (!result)
+		result = stat(file, &st) && error("Could not stat '%s'", file);
+	if (!result && !st.st_size)
+		result = error("Empty patch. Aborted.");
+
+	memset(&child, 0, sizeof(child));
+	child.git_cmd = 1;
+	child.argv = apply_argv;
+	if (!result)
+		result = run_command(&child) &&
+			error("Could not apply '%s'", file);
+	if (!result)
+		unlink(file);
+	return result;
+}
+
 static struct lock_file lock_file;
 
 static const char ignore_error[] =
@@ -202,6 +252,7 @@ static struct option builtin_add_options[] = {
 	OPT_BOOLEAN('p', "patch", &patch_interactive, "interactive patching"),
 	OPT_BOOLEAN('f', "force", &ignored_too, "allow adding otherwise ignored files"),
 	OPT_BOOLEAN('u', "update", &take_worktree_changes, "update tracked files"),
+	OPT_BOOLEAN('e', "edit", &edit_interactive, "super-interactive patching"),
 	OPT_BOOLEAN( 0 , "refresh", &refresh_only, "don't add, only refresh the index"),
 	OPT_BOOLEAN( 0 , "ignore-errors", &ignore_add_errors, "just skip files which cannot be added because of errors"),
 	OPT_END(),
@@ -226,6 +277,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, builtin_add_options,
 			  builtin_add_usage, 0);
+	if (edit_interactive)
+		return(edit_patch(argc, argv, prefix));
 	if (patch_interactive)
 		add_interactive = 1;
 	if (add_interactive)
diff --git a/t/t3702-add-edit.sh b/t/t3702-add-edit.sh
new file mode 100755
index 0000000..decf727
--- /dev/null
+++ b/t/t3702-add-edit.sh
@@ -0,0 +1,126 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 Johannes E. Schindelin
+#
+
+test_description='add -e basic tests'
+. ./test-lib.sh
+
+
+cat > file << EOF
+LO, praise of the prowess of people-kings
+of spear-armed Danes, in days long sped,
+we have heard, and what honor the athelings won!
+Oft Scyld the Scefing from squadroned foes,
+from many a tribe, the mead-bench tore,
+awing the earls. Since erst he lay
+friendless, a foundling, fate repaid him:
+for he waxed under welkin, in wealth he throve,
+till before him the folk, both far and near,
+who house by the whale-path, heard his mandate,
+gave him gifts:  a good king he!
+EOF
+
+test_expect_success 'setup' '
+
+	git add file &&
+	test_tick &&
+	git commit -m initial file
+
+'
+
+cat > patch << EOF
+diff --git a/file b/file
+index b9834b5..ef6e94c 100644
+--- a/file
++++ b/file
+@@ -3,1 +3,333 @@ of spear-armed Danes, in days long sped,
+ we have heard, and what honor the athelings won!
++
+ Oft Scyld the Scefing from squadroned foes,
+@@ -2,7 +1,5 @@ awing the earls. Since erst he lay
+ friendless, a foundling, fate repaid him:
++
+ for he waxed under welkin, in wealth he throve,
+EOF
+
+cat > expected << EOF
+diff --git a/file b/file
+index b9834b5..ef6e94c 100644
+--- a/file
++++ b/file
+@@ -1,10 +1,12 @@
+ LO, praise of the prowess of people-kings
+ of spear-armed Danes, in days long sped,
+ we have heard, and what honor the athelings won!
++
+ Oft Scyld the Scefing from squadroned foes,
+ from many a tribe, the mead-bench tore,
+ awing the earls. Since erst he lay
+ friendless, a foundling, fate repaid him:
++
+ for he waxed under welkin, in wealth he throve,
+ till before him the folk, both far and near,
+ who house by the whale-path, heard his mandate,
+EOF
+
+echo "#!$SHELL_PATH" >fake-editor.sh
+cat >> fake-editor.sh <<\EOF
+mv -f "$1" orig-patch &&
+mv -f patch "$1"
+EOF
+
+test_set_editor "$(pwd)/fake-editor.sh"
+chmod a+x fake-editor.sh
+
+test_expect_success 'add -e' '
+
+	cp fake-editor.sh file &&
+	git add -e &&
+	test_cmp fake-editor.sh file &&
+	git diff --cached > out &&
+	test_cmp out expected
+
+'
+
+cat > patch << EOF
+diff --git a/file b/file
+--- a/file
++++ b/file
+@@ -1,1 +1,1 @@
+ gave him gifts:  a good king he!
++
+EOF
+
+test_expect_success 'add -e adds to the end of the file' '
+
+	test_tick &&
+	git commit -m update &&
+	git checkout &&
+	git add -e &&
+	git diff --cached > out &&
+	test "" = "$(git show :file | tail -n 1)"
+
+'
+
+cat > patch << EOF
+diff --git a/file b/file
+--- a/file
++++ b/file
+@@ -1,1 +1,1 @@
++
+ LO, praise of the prowess of people-kings
+EOF
+
+test_expect_success 'add -e adds to the beginning of the file' '
+
+	test_tick &&
+	git commit -m update &&
+	git checkout &&
+	git add -e &&
+	git diff --cached > out &&
+	test "" = "$(git show :file | head -n 1)"
+
+'
+
+test_done
-- 
1.5.6.84.ge5c1

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

[Thomas Rast]

Adds a new option 'e' to the 'add -p' command loop that lets you edit
the current hunk in your favourite editor.

If the resulting patch applies cleanly, the edited hunk will
immediately be marked for staging. If it does not apply cleanly, you
will be given an opportunity to edit again. If all lines of the hunk
are removed, then the edit is aborted and the hunk is left unchanged.

Applying the changed hunk(s) relies on Johannes Schindelin's new
--recount option for git-apply.

Note that the "real patch" test intentionally uses
  (echo e; echo n; echo d) | git add -p
even though the 'n' and 'd' are superfluous at first sight.  They
serve to get out of the interaction loop if git add -p wrongly
concludes the patch does not apply.

Many thanks to Jeff King <peff@peff.net> for lots of help and
suggestions.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 Documentation/git-add.txt  |    1 +
 git-add--interactive.perl  |  124 +++++++++++++++++++++++++++++++++++++++++++-
 t/t3701-add-interactive.sh |   67 ++++++++++++++++++++++++
 3 files changed, 191 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index c6de028..1d8d209 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -246,6 +246,7 @@ patch::
        k - leave this hunk undecided, see previous undecided hunk
        K - leave this hunk undecided, see previous hunk
        s - split the current hunk into smaller hunks
+       e - manually edit the current hunk
        ? - print help
 +
 After deciding the fate for all hunks, if there is any hunk
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 903953e..6bb117a 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -2,6 +2,7 @@
 
 use strict;
 use Git;
+use File::Temp;
 
 my $repo = Git->repository();
 
@@ -18,6 +19,18 @@ my ($fraginfo_color) =
 	$diff_use_color ? (
 		$repo->get_color('color.diff.frag', 'cyan'),
 	) : ();
+my ($diff_plain_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.plain', ''),
+	) : ();
+my ($diff_old_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.old', 'red'),
+	) : ();
+my ($diff_new_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.new', 'green'),
+	) : ();
 
 my $normal_color = $repo->get_color("", "reset");
 
@@ -770,6 +783,104 @@ sub coalesce_overlapping_hunks {
 	return @out;
 }
 
+sub color_diff {
+	return map {
+		colored((/^@/  ? $fraginfo_color :
+			 /^\+/ ? $diff_new_color :
+			 /^-/  ? $diff_old_color :
+			 $diff_plain_color),
+			$_);
+	} @_;
+}
+
+sub edit_hunk_manually {
+	my ($oldtext) = @_;
+
+	my $t = File::Temp->new(
+		TEMPLATE => $repo->repo_path . "/git-hunk-edit.XXXXXX",
+		SUFFIX => '.diff'
+	);
+	print $t "# Manual hunk edit mode -- see bottom for a quick guide\n";
+	print $t @$oldtext;
+	print $t <<EOF;
+# ---
+# To remove '-' lines, make them ' ' lines (context).
+# To remove '+' lines, delete them.
+# Lines starting with # will be removed.
+#
+# If the patch applies cleanly, the edited hunk will immediately be
+# marked for staging. If it does not apply cleanly, you will be given
+# an opportunity to edit again. If all lines of the hunk are removed,
+# then the edit is aborted and the hunk is left unchanged.
+EOF
+	close $t;
+
+	my $editor = $ENV{GIT_EDITOR} || $repo->config("core.editor")
+		|| $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+	system('sh', '-c', $editor.' "$@"', $editor, $t);
+
+	open my $fh, '<', $t
+		or die "failed to open hunk edit file for reading: " . $!;
+	my @newtext = grep { !/^#/ } <$fh>;
+	close $fh;
+
+	# Abort if nothing remains
+	if (!grep { /\S/ } @newtext) {
+		return undef;
+	}
+
+	# Reinsert the first hunk header if the user accidentally deleted it
+	if ($newtext[0] !~ /^@/) {
+		unshift @newtext, $oldtext->[0];
+	}
+	return \@newtext;
+}
+
+sub diff_applies {
+	my $fh;
+	open $fh, '| git apply --recount --cached --check';
+	for my $h (@_) {
+		print $fh @{$h->{TEXT}};
+	}
+	return close $fh;
+}
+
+sub prompt_yesno {
+	my ($prompt) = @_;
+	while (1) {
+		print colored $prompt_color, $prompt;
+		my $line = <STDIN>;
+		return 0 if $line =~ /^n/i;
+		return 1 if $line =~ /^y/i;
+	}
+}
+
+sub edit_hunk_loop {
+	my ($head, $hunk, $ix) = @_;
+	my $text = $hunk->[$ix]->{TEXT};
+
+	while (1) {
+		$text = edit_hunk_manually($text);
+		if (!defined $text) {
+			return undef;
+		}
+		my $newhunk = { TEXT => $text, USE => 1 };
+		if (diff_applies($head,
+				 @{$hunk}[0..$ix-1],
+				 $newhunk,
+				 @{$hunk}[$ix+1..$#{$hunk}])) {
+			$newhunk->{DISPLAY} = [color_diff(@{$text})];
+			return $newhunk;
+		}
+		else {
+			prompt_yesno(
+				'Your edited hunk does not apply. Edit again '
+				. '(saying "no" discards!) [y/n]? '
+				) or return undef;
+		}
+	}
+}
+
 sub help_patch_cmd {
 	print colored $help_color, <<\EOF ;
 y - stage this hunk
@@ -781,6 +892,7 @@ J - leave this hunk undecided, see next hunk
 k - leave this hunk undecided, see previous undecided hunk
 K - leave this hunk undecided, see previous hunk
 s - split the current hunk into smaller hunks
+e - manually edit the current hunk
 ? - print help
 EOF
 }
@@ -846,6 +958,7 @@ sub patch_update_file {
 
 	$num = scalar @hunk;
 	$ix = 0;
+	my $need_recount = 0;
 
 	while (1) {
 		my ($prev, $next, $other, $undecided, $i);
@@ -885,6 +998,7 @@ sub patch_update_file {
 		if (hunk_splittable($hunk[$ix]{TEXT})) {
 			$other .= '/s';
 		}
+		$other .= '/e';
 		for (@{$hunk[$ix]{DISPLAY}}) {
 			print;
 		}
@@ -949,6 +1063,13 @@ sub patch_update_file {
 				$num = scalar @hunk;
 				next;
 			}
+			elsif ($line =~ /^e/) {
+				my $newhunk = edit_hunk_loop($head, \@hunk, $ix);
+				if (defined $newhunk) {
+					splice @hunk, $ix, 1, $newhunk;
+					$need_recount = 1;
+				}
+			}
 			else {
 				help_patch_cmd($other);
 				next;
@@ -1002,7 +1123,8 @@ sub patch_update_file {
 	if (@result) {
 		my $fh;
 
-		open $fh, '| git apply --cached';
+		open $fh, '| git apply --cached'
+			. ($need_recount ? ' --recount' : '');
 		for (@{$head->{TEXT}}, @result) {
 			print $fh $_;
 		}
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index fae64ea..e95663d 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -66,6 +66,73 @@ test_expect_success 'revert works (commit)' '
 	grep "unchanged *+3/-0 file" output
 '
 
+cat >expected <<EOF
+EOF
+cat >fake_editor.sh <<EOF
+EOF
+chmod a+x fake_editor.sh
+test_set_editor "$(pwd)/fake_editor.sh"
+test_expect_success 'dummy edit works' '
+	(echo e; echo a) | git add -p &&
+	git diff > diff &&
+	test_cmp expected diff
+'
+
+cat >patch <<EOF
+@@ -1,1 +1,4 @@
+ this
++patch
+-doesn't
+ apply
+EOF
+echo "#!$SHELL_PATH" >fake_editor.sh
+cat >>fake_editor.sh <<\EOF
+mv -f "$1" oldpatch &&
+mv -f patch "$1"
+EOF
+chmod a+x fake_editor.sh
+test_set_editor "$(pwd)/fake_editor.sh"
+test_expect_success 'bad edit rejected' '
+	git reset &&
+	(echo e; echo n; echo d) | git add -p >output &&
+	grep "hunk does not apply" output
+'
+
+cat >patch <<EOF
+this patch
+is garbage
+EOF
+test_expect_success 'garbage edit rejected' '
+	git reset &&
+	(echo e; echo n; echo d) | git add -p >output &&
+	grep "hunk does not apply" output
+'
+
+cat >patch <<EOF
+@@ -1,0 +1,0 @@
+ baseline
++content
++newcontent
++lines
+EOF
+cat >expected <<EOF
+diff --git a/file b/file
+index b5dd6c9..f910ae9 100644
+--- a/file
++++ b/file
+@@ -1,4 +1,4 @@
+ baseline
+ content
+-newcontent
++more
+ lines
+EOF
+test_expect_success 'real edit works' '
+	(echo e; echo n; echo d) | git add -p &&
+	git diff >output &&
+	test_cmp expected output
+'
+
 if test "$(git config --bool core.filemode)" = false
 then
     say 'skipping filemode tests (filesystem does not properly support modes)'
-- 
1.5.6.84.ge5c1

Re: [PATCH 0/3] Manual editing for 'add' and 'add -p'

[Miklos Vajna]

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

On Tue, Jun 24, 2008 at 09:07:54PM +0200, Thomas Rast <trast@student.ethz.ch> wrote:
> I hope this series turns out right in mail; I'm struggling a bit to
> properly import the mails to KMail.  Most annoyingly, format-patch
> apparently cannot turn other people's patches into mails by myself
> with an appropriate "From:" line. :-(

Why don't you just use git-send-email? It already handles the situation.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/3] Allow git-apply to ignore the hunk headers (AKA recountdiff)

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> diff --git a/builtin-apply.c b/builtin-apply.c
> index c497889..34c220f 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> @@ -153,6 +153,7 @@ struct patch {
>  	unsigned int is_binary:1;
>  	unsigned int is_copy:1;
>  	unsigned int is_rename:1;
> +	unsigned int recount:1;
>  	struct fragment *fragments;
>  	char *result;
>  	size_t resultsize;

Why doesn't anybody find this quite wrong?

What is a "struct patch"?  It describes a change to a single file
(i.e. information contained from one "diff --git" til next "diff --git"),
groups the hunks (called "fragments") together and holds the postimage
after applying these hunks.  Is this new "recount" field a per file
attribute?

> +	fragment->oldpos = 2;
> +	fragment->oldlines = fragment->newlines = 0;

Why is this discarding the position information?  

It is none of this function's business.  The ONLY THING this function
should do is to discard fragment->oldlines and fragment->newlines, count
the contents and update these two fields, and nothing else.  Touching
oldpos, leading and other things is an absolute no-no.

> @@ -1013,6 +1058,9 @@ static int parse_fragment(char *line, unsigned long size,
>  	offset = parse_fragment_header(line, len, fragment);
>  	if (offset < 0)
>  		return -1;
> +	if (offset > 0 && patch->recount &&
> +			recount_diff(line + offset, size - offset, fragment))
> +		return -1;

And recount should not cause parse_fragment() to fail out either.  If you
miscounted, the codepath that follows this part knows how to handle broken
patch correctly anyway.

I think I've already mentioned the above two points when this was
originally posted.

Somewhat disgusted...

Re: [PATCH 1/3] Allow git-apply to ignore the hunk headers (AKA recountdiff)

[Jeff King]

On Tue, Jun 24, 2008 at 04:35:33PM -0700, Junio C Hamano wrote:

> I think I've already mentioned the above two points when this was
> originally posted.
> 
> Somewhat disgusted...

Sorry, this is my fault. I hadn't looked at the recount patches closely,
but I had thought they were submit-worthy, so I encouraged Thomas to
include them in his series (since his patch depends on them).

I'll try to look them over more closely in the next day or two.

-Peff

Re: [PATCH 1/3] Allow git-apply to ignore the hunk headers (AKA recountdiff)

[Johannes Schindelin]

Hi,

On Tue, 24 Jun 2008, Junio C Hamano wrote:

> Thomas Rast <trast@student.ethz.ch> writes:
> 
> > diff --git a/builtin-apply.c b/builtin-apply.c
> > index c497889..34c220f 100644
> > --- a/builtin-apply.c
> > +++ b/builtin-apply.c
> > @@ -153,6 +153,7 @@ struct patch {
> >  	unsigned int is_binary:1;
> >  	unsigned int is_copy:1;
> >  	unsigned int is_rename:1;
> > +	unsigned int recount:1;
> >  	struct fragment *fragments;
> >  	char *result;
> >  	size_t resultsize;
> 
> Why doesn't anybody find this quite wrong?
> 
> What is a "struct patch"?  It describes a change to a single file
> (i.e. information contained from one "diff --git" til next "diff --git"),
> groups the hunks (called "fragments") together and holds the postimage
> after applying these hunks.  Is this new "recount" field a per file
> attribute?

Actually, it is not.  But then, it is an attribute of the patch: it will 
be recounted.

In addition, the patch would get quite large and unwieldy with "recount" 
being passed between the functions, because we do not have "apply_options" 
yet.

I was even briefly working on apply_options, but this would be a _huge_ 
patch (I had an initial working version, but given my limited time, I 
could not clean it up for submission anyway, so I scrapped it).

> > +	fragment->oldpos = 2;
> > +	fragment->oldlines = fragment->newlines = 0;
> 
> Why is this discarding the position information?

Sorry, I forgot.

> > @@ -1013,6 +1058,9 @@ static int parse_fragment(char *line, unsigned long size,
> >  	offset = parse_fragment_header(line, len, fragment);
> >  	if (offset < 0)
> >  		return -1;
> > +	if (offset > 0 && patch->recount &&
> > +			recount_diff(line + offset, size - offset, fragment))
> > +		return -1;
> 
> And recount should not cause parse_fragment() to fail out either.  If 
> you miscounted, the codepath that follows this part knows how to handle 
> broken patch correctly anyway.

Okay.

> I think I've already mentioned the above two points when this was 
> originally posted.
> 
> Somewhat disgusted...

Sorry.

This is my updated patch (making changes to my "add -e" patch, which I 
may submit later), according to your comments so far:

-- snipsnap --
[PATCH] Allow git-apply to recount the lines in a hunk (AKA recountdiff)

Sometimes, the easiest way to fix up a patch is to edit it directly, even
adding or deleting lines.  Now, many people are not as divine as certain
benevolent dictators as to update the hunk headers correctly at the first
try.

So teach the tool to do it for us.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-apply.txt |    7 +++-
 builtin-apply.c             |   76 +++++++++++++++++++++++++++++++++++++++----
 2 files changed, 75 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index c834763..c5ee636 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -12,7 +12,7 @@ SYNOPSIS
 'git-apply' [--stat] [--numstat] [--summary] [--check] [--index]
 	  [--apply] [--no-add] [--build-fake-ancestor <file>] [-R | --reverse]
 	  [--allow-binary-replacement | --binary] [--reject] [-z]
-	  [-pNUM] [-CNUM] [--inaccurate-eof] [--cached]
+	  [-pNUM] [-CNUM] [--inaccurate-eof] [--recount] [--cached]
 	  [--whitespace=<nowarn|warn|fix|error|error-all>]
 	  [--exclude=PATH] [--verbose] [<patch>...]
 
@@ -177,6 +177,11 @@ behavior:
 	current patch being applied will be printed. This option will cause
 	additional information to be reported.
 
+--recount::
+	Do not trust the line counts in the hunk headers, but infer them
+	by inspecting the patch (e.g. after editing the patch without
+	adjusting the hunk headers appropriately).
+
 Configuration
 -------------
 
diff --git a/builtin-apply.c b/builtin-apply.c
index c497889..c819652 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -153,6 +153,7 @@ struct patch {
 	unsigned int is_binary:1;
 	unsigned int is_copy:1;
 	unsigned int is_rename:1;
+	unsigned int recount:1;
 	struct fragment *fragments;
 	char *result;
 	size_t resultsize;
@@ -882,6 +883,57 @@ static int parse_range(const char *line, int len, int offset, const char *expect
 	return offset + ex;
 }
 
+static int recount_diff(char *line, int size, struct fragment *fragment)
+{
+	int oldlines = 0, newlines = 0, ret = 0;
+
+	if (size < 1) {
+		warning("recount: ignore empty hunk");
+		return -1;
+	}
+
+	for (;;) {
+		int len = linelen(line, size);
+		size -= len;
+		line += len;
+
+		if (size < 1)
+			break;
+
+		switch (*line) {
+		case ' ': case '\n':
+			newlines++;
+			/* fall through */
+		case '-':
+			oldlines++;
+			continue;
+		case '+':
+			newlines++;
+			continue;
+		case '\\':
+			break;
+		case '@':
+			ret = size < 3 || prefixcmp(line, "@@ ");
+			break;
+		case 'd':
+			ret = size < 5 || prefixcmp(line, "diff ");
+			break;
+		default:
+			ret = -1;
+			break;
+		}
+		if (ret) {
+			warning("recount: unexpected line: %.*s",
+				(int)linelen(line, size), line);
+			return -1;
+		}
+		break;
+	}
+	fragment->oldlines = oldlines;
+	fragment->newlines = newlines;
+	return 0;
+}
+
 /*
  * Parse a unified diff fragment header of the
  * form "@@ -a,b +c,d @@"
@@ -1013,6 +1065,8 @@ static int parse_fragment(char *line, unsigned long size,
 	offset = parse_fragment_header(line, len, fragment);
 	if (offset < 0)
 		return -1;
+	if (offset > 0 && patch->recount)
+		recount_diff(line + offset, size - offset, fragment);
 	oldlines = fragment->oldlines;
 	newlines = fragment->newlines;
 	leading = 0;
@@ -2912,7 +2966,10 @@ static void prefix_patches(struct patch *p)
 	}
 }
 
-static int apply_patch(int fd, const char *filename, int inaccurate_eof)
+#define INACCURATE_EOF	(1<<0)
+#define RECOUNT		(1<<1)
+
+static int apply_patch(int fd, const char *filename, int options)
 {
 	size_t offset;
 	struct strbuf buf;
@@ -2928,7 +2985,8 @@ static int apply_patch(int fd, const char *filename, int inaccurate_eof)
 		int nr;
 
 		patch = xcalloc(1, sizeof(*patch));
-		patch->inaccurate_eof = inaccurate_eof;
+		patch->inaccurate_eof = !!(options & INACCURATE_EOF);
+		patch->recount =  !!(options & RECOUNT);
 		nr = parse_chunk(buf.buf + offset, buf.len - offset, patch);
 		if (nr < 0)
 			break;
@@ -2997,7 +3055,7 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 {
 	int i;
 	int read_stdin = 1;
-	int inaccurate_eof = 0;
+	int options = 0;
 	int errs = 0;
 	int is_not_gitdir;
 
@@ -3015,7 +3073,7 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 		int fd;
 
 		if (!strcmp(arg, "-")) {
-			errs |= apply_patch(0, "<stdin>", inaccurate_eof);
+			errs |= apply_patch(0, "<stdin>", options);
 			read_stdin = 0;
 			continue;
 		}
@@ -3115,7 +3173,11 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 			continue;
 		}
 		if (!strcmp(arg, "--inaccurate-eof")) {
-			inaccurate_eof = 1;
+			options |= INACCURATE_EOF;
+			continue;
+		}
+		if (!strcmp(arg, "--recount")) {
+			options |= RECOUNT;
 			continue;
 		}
 		if (0 < prefix_length)
@@ -3126,12 +3188,12 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 			die("can't open patch '%s': %s", arg, strerror(errno));
 		read_stdin = 0;
 		set_default_whitespace_mode(whitespace_option);
-		errs |= apply_patch(fd, arg, inaccurate_eof);
+		errs |= apply_patch(fd, arg, options);
 		close(fd);
 	}
 	set_default_whitespace_mode(whitespace_option);
 	if (read_stdin)
-		errs |= apply_patch(0, "<stdin>", inaccurate_eof);
+		errs |= apply_patch(0, "<stdin>", options);
 	if (whitespace_error) {
 		if (squelch_whitespace_errors &&
 		    squelch_whitespace_errors < whitespace_error) {
-- 
1.5.6.173.gde14c