[PATCH 1/2] Allow git-apply to fix up the line counts

[PATCH 1/2] Allow git-apply to fix up the line counts

[Johannes Schindelin]

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 |    6 ++++-
 builtin-apply.c             |   55 +++++++++++++++++++++++++++++++++++++++---
 2 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index 2dec2ec..ba3dba7 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-line-counts] [--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..3fd80e8 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,6 +883,41 @@ static int parse_range(const char *line, int len, int offset, const char *expect
 	return offset + ex;
 }
 
+static int fixup_counts(char *line, int size, struct fragment *fragment)
+{
+	if (size < 1)
+		return -1;
+
+	fragment->oldlines = fragment->newlines = -1;
+
+	for (;;) {
+		int len = linelen(line, size);
+		if (!len)
+			break;
+
+		switch (*line) {
+		case ' ':
+			fragment->oldlines++;
+			/* fall through */
+		case '+':
+			fragment->newlines++;
+			break;
+		case '-':
+			fragment->oldlines++;
+			break;
+		default:
+			/* Probably "diff ..." */
+			return 0;
+		}
+
+		size -= len;
+		line += len;
+		if (size < 2 || !prefixcmp(line, "@@"))
+			break;
+	}
+	return 0;
+}
+
 /*
  * Parse a unified diff fragment header of the
  * form "@@ -a,b +c,d @@"
@@ -1013,6 +1049,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->fixup &&
+			fixup_counts(line + offset, size - offset, fragment))
+		return -1;
 	oldlines = fragment->oldlines;
 	newlines = fragment->newlines;
 	leading = 0;
@@ -2912,7 +2951,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 +2969,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 +3039,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 +3057,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 +3161,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 +3173,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.rc1.181.gb439d

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

[Johannes Schindelin]

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

	This was too useful to let slip by.  I even committed it using 
	"git add -e <files>" several times!

	Anyway, bed time.

 Documentation/git-add.txt |    9 ++++++-
 builtin-add.c             |   49 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 1afd0c6..dd744f1 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -8,8 +8,8 @@ git-add - Add file contents to the index
 SYNOPSIS
 --------
 [verse]
-'git-add' [-n] [-v] [-f] [--interactive | -i] [--patch | -p] [-u] [--refresh]
-	  [--ignore-errors] [--] <filepattern>...
+'git-add' [-n] [-v] [-f] [--interactive | -i] [--patch | -p] [--edit | -e]
+	  [-u] [--refresh] [--ignore-errors] [--] <filepattern>...
 
 DESCRIPTION
 -----------
@@ -70,6 +70,11 @@ 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.
+
 -u::
 	Update only files that git already knows about, staging modified
 	content for commit and marking deleted files for removal. This
diff --git a/builtin-add.c b/builtin-add.c
index 1da22ee..05ae40d 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,50 @@ int interactive_add(int argc, const char **argv, const char *prefix)
 	return status;
 }
 
+int edit_patch(int argc, const char **argv, const char *prefix)
+{
+	static struct lock_file lock;
+	struct child_process child;
+	int 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 = hold_lock_file_for_update(&lock, git_path("EDIT_PATCH"), 1);
+
+	if (run_command(&child))
+		return 1;
+	free(child.argv);
+
+	launch_editor(lock.filename, NULL, NULL);
+
+	if (stat(lock.filename, &st))
+		return 1;
+	if (!st.st_size) {
+		fprintf(stderr, "Empty patch. Aborted.\n");
+		return 0;
+	}
+
+	execl_git_cmd("apply", "--fixup-line-counts", "--cached",
+			lock.filename, NULL);
+
+	return 1;
+}
+
 static struct lock_file lock_file;
 
 static const char ignore_error[] =
@@ -200,6 +244,7 @@ static struct option builtin_add_options[] = {
 	OPT_GROUP(""),
 	OPT_BOOLEAN('i', "interactive", &add_interactive, "interactive picking"),
 	OPT_BOOLEAN('p', "patch", &patch_interactive, "interactive patching"),
+	OPT_BOOLEAN('e', "edit", &edit_interactive, "super-interactive patching"),
 	OPT_BOOLEAN('f', NULL, &ignored_too, "allow adding otherwise ignored files"),
 	OPT_BOOLEAN('u', NULL, &take_worktree_changes, "update tracked files"),
 	OPT_BOOLEAN( 0 , "refresh", &refresh_only, "don't add, only refresh the index"),
@@ -226,6 +271,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)
-- 
1.5.6.rc1.181.gb439d

Re: [PATCH 1/2] Allow git-apply to fix up the line counts

[Johannes Sixt]

Johannes Schindelin schrieb:
> +--fixup-line-counts::
> +	Fix up the line counts (e.g. after editing the patch without
> +	adjusting the hunk headers appropriately).

This sort of implies that there is some kind of output that tells the
correct line counts. But that isn't the case (if I read the patch
correctly). So I suggest to name the option --ignore-line-counts.

-- Hannes

Re: [PATCH 1/2] Allow git-apply to fix up the line counts

[Johannes Schindelin]

Hi,

On Thu, 5 Jun 2008, Johannes Sixt wrote:

> Johannes Schindelin schrieb:
> > +--fixup-line-counts::
> > +	Fix up the line counts (e.g. after editing the patch without
> > +	adjusting the hunk headers appropriately).
> 
> This sort of implies that there is some kind of output that tells the
> correct line counts. But that isn't the case (if I read the patch
> correctly). So I suggest to name the option --ignore-line-counts.

But there is some kind of output: the hunks themselves.  And the line 
counts are not ignored, but they are actively rewritten.  But if you have 
a suggestion which keeps the spirit, I am very interested...

Ciao,
Dscho

Re: [PATCH 1/2] Allow git-apply to fix up the line counts

[Johannes Sixt]

Johannes Schindelin schrieb:
> Hi,
> 
> On Thu, 5 Jun 2008, Johannes Sixt wrote:
> 
>> Johannes Schindelin schrieb:
>>> +--fixup-line-counts::
>>> +	Fix up the line counts (e.g. after editing the patch without
>>> +	adjusting the hunk headers appropriately).
>> This sort of implies that there is some kind of output that tells the
>> correct line counts. But that isn't the case (if I read the patch
>> correctly). So I suggest to name the option --ignore-line-counts.
> 
> But there is some kind of output: the hunks themselves.

Is there? I did this (it rewrites all line counts to 1):

$ git diff ..HEAD~1 |
	sed -e '/^@@/s/,[0-9]+ /,1 /g' |
	./git-apply --fixup-line-counts

and there was no output. Instead, the patch was applied.

>  And the line 
> counts are not ignored, but they are actively rewritten.

Of course, internally there is some sort of "output" from the fixup
routine, and the line counts are rewritten and then are not ignored. But
the user doesn't care about this internal procedure. From the user's
perspective, the line counts of the input patch are ignored.

Apart from this color of the bikeshed I like your patch.

-- Hannes

Re: [PATCH 1/2] Allow git-apply to fix up the line counts

[Johannes Schindelin]

Hi,

On Thu, 5 Jun 2008, Johannes Sixt wrote:

> Johannes Schindelin schrieb:
> 
> > On Thu, 5 Jun 2008, Johannes Sixt wrote:
> > 
> >> Johannes Schindelin schrieb:
> >>> +--fixup-line-counts::
> >>> +	Fix up the line counts (e.g. after editing the patch without
> >>> +	adjusting the hunk headers appropriately).
> >>>
> >> This sort of implies that there is some kind of output that tells the 
> >> correct line counts. But that isn't the case (if I read the patch 
> >> correctly). So I suggest to name the option --ignore-line-counts.
> > 
> > But there is some kind of output: the hunks themselves.
> 
> Is there?

Yes!

> I did this (it rewrites all line counts to 1):
> 
> $ git diff ..HEAD~1 |
> 	sed -e '/^@@/s/,[0-9]+ /,1 /g' |
> 	./git-apply --fixup-line-counts
> 
> and there was no output. Instead, the patch was applied.

As I said, the data is in the _hunks_, but I maybe should have added _not 
in the hunk headers_.

So in a very real sense, you edit the hunks, and the hunk headers are 
adjusted to that.  You did not adjust the hunks, so they got applied.

It seems that you think the hunk header's line counts are heeded, and the 
hunk adjusted, with --fixup-line-counts?  Sorry, I find that rather 
counterintuitive.

> >  And the line counts are not ignored, but they are actively rewritten.
> 
> Of course, internally there is some sort of "output" from the fixup 
> routine, and the line counts are rewritten and then are not ignored. But 
> the user doesn't care about this internal procedure. From the user's 
> perspective, the line counts of the input patch are ignored.

But they are not!

There are _two_ things that are the line counts.  Those numbers in the 
hunk header, and the real line counts of the hunks.

Now, if you say they are _ignored_, would that not imply in plain English 
that they are left unchanged (in limbo, because those two types of numbers 
contradict each other)?

Okay, how about shikebedding this to --adjust-line-counts?

Ciao,
Dscho

Re: [PATCH 1/2] Allow git-apply to fix up the line counts

[Johannes Sixt]

Johannes Schindelin schrieb:
> Hi,
> 
> On Thu, 5 Jun 2008, Johannes Sixt wrote:
> 
>> Johannes Schindelin schrieb:
>>
>>> On Thu, 5 Jun 2008, Johannes Sixt wrote:
>>>
>>>> Johannes Schindelin schrieb:
>>>>> +--fixup-line-counts::
>>>>> +	Fix up the line counts (e.g. after editing the patch without
>>>>> +	adjusting the hunk headers appropriately).
>>>>>
>>>> This sort of implies that there is some kind of output that tells the 
>>>> correct line counts. But that isn't the case (if I read the patch 
>>>> correctly). So I suggest to name the option --ignore-line-counts.
>>> But there is some kind of output: the hunks themselves.
>> Is there?
> 
> Yes!
> 
>> I did this (it rewrites all line counts to 1):
>>
>> $ git diff ..HEAD~1 |
>> 	sed -e '/^@@/s/,[0-9]+ /,1 /g' |
>> 	./git-apply --fixup-line-counts
>>
>> and there was no output. Instead, the patch was applied.
> 
> As I said, the data is in the _hunks_, but I maybe should have added _not 
> in the hunk headers_.

Yes, of course.

> So in a very real sense, you edit the hunks, and the hunk headers are 
> adjusted to that.  You did not adjust the hunks, so they got applied.

Yes, of course.

But the example pretends that the hunks have been edited so heavily that
they in no way match the line counts in the hunk headers.

> It seems that you think the hunk header's line counts are heeded, and the 
> hunk adjusted, with --fixup-line-counts?

NO, of course *NOT*.

>  Sorry, I find that rather 
> counterintuitive.

So would I.

>>>  And the line counts are not ignored, but they are actively rewritten.
>> Of course, internally there is some sort of "output" from the fixup 
>> routine, and the line counts are rewritten and then are not ignored. But 
>> the user doesn't care about this internal procedure. From the user's 
>> perspective, the line counts of the input patch are ignored.
> 
> But they are not!

> There are _two_ things that are the line counts.  Those numbers in the 
> hunk header, and the real line counts of the hunks.

And I was always talking about the numbers in the hunk headers.

> Now, if you say they are _ignored_, would that not imply in plain English 
> that they are left unchanged (in limbo, because those two types of numbers 
> contradict each other)?

That you *internally* rewrite those numbers and then do *not* ignore them
is totally pointless for the user. It's an implementation detail. The user
doesn't see what is going on nor should he care. From the user's
perspective, the hunk header line counts are _ignored_ (because if they
were not ignored, then there would be an error message in the
contradicting case).

> Okay, how about shikebedding this to --adjust-line-counts?

>From the user's perspective, nothing is "adjusted"; the hunk header line
counts are ... you guess it ... *ignored*.

-- Hannes

Re: [PATCH 1/2] Allow git-apply to fix up the line counts

[Johannes Schindelin]

Hi,

On Thu, 5 Jun 2008, Johannes Sixt wrote:

> > Now, if you say they are _ignored_, would that not imply in plain 
> > English that they are left unchanged (in limbo, because those two 
> > types of numbers contradict each other)?
> 
> That you *internally* rewrite those numbers and then do *not* ignore 
> them is totally pointless for the user. It's an implementation detail. 
> The user doesn't see what is going on nor should he care. From the 
> user's perspective, the hunk header line counts are _ignored_ (because 
> if they were not ignored, then there would be an error message in the 
> contradicting case).
> 
> > Okay, how about shikebedding this to --adjust-line-counts?
> 
> From the user's perspective, nothing is "adjusted"; the hunk header line 
> counts are ... you guess it ... *ignored*.

Oh... I start to see what you mean.  It's just that for me, the line 
counts are the actual line counts, not what is recorded in the hunk 
header.

In any case, I really do not feel strongly about it, since I do not want 
to use it, except with git add -e.  Which I really grew fond of in these 
last hours ;-)

So how about --ignore-hunk-headers?  I think this is much more 
descriptive, and catches your complaint, IMHO.

Ciao,
Dscho

Re: [PATCH 1/2] Allow git-apply to fix up the line counts

[Johannes Sixt]

Johannes Schindelin schrieb:
> So how about --ignore-hunk-headers?  I think this is much more 
> descriptive, and catches your complaint, IMHO.

Yes, that's good as well. :-)

-- Hannes

[PATCH v2 0/2] git add --edit

[Johannes Schindelin]

Changes relative to the first version:

- rename the apply option to --ignore-hunk-headers

- add a test

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

 Documentation/git-add.txt   |    9 +++-
 Documentation/git-apply.txt |    7 +++-
 builtin-add.c               |   47 +++++++++++++++++++++++-
 builtin-apply.c             |   57 ++++++++++++++++++++++++++--
 t/t3702-add-edit.sh         |   86 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 198 insertions(+), 8 deletions(-)
 create mode 100755 t/t3702-add-edit.sh

[PATCH v2 1/2] Allow git-apply to ignore the hunk headers

[Johannes Schindelin]

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             |   57 ++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 59 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index 2dec2ec..e4c5530 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] [--ignore-hunk-headers] [--cached]
 	  [--whitespace=<nowarn|warn|fix|error|error-all>]
 	  [--exclude=PATH] [--verbose] [<patch>...]
 
@@ -169,6 +169,11 @@ behavior:
 	correctly. This option adds support for applying such patches by
 	working around this bug.
 
+--ignore-hunk-headers::
+	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
 	current patch being applied will be printed. This option will cause
diff --git a/builtin-apply.c b/builtin-apply.c
index c497889..b357e35 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 ignore_hunk_headers:1;
 	struct fragment *fragments;
 	char *result;
 	size_t resultsize;
@@ -882,6 +883,41 @@ static int parse_range(const char *line, int len, int offset, const char *expect
 	return offset + ex;
 }
 
+static int fixup_counts(char *line, int size, struct fragment *fragment)
+{
+	if (size < 1)
+		return -1;
+
+	fragment->oldlines = fragment->newlines = -1;
+
+	for (;;) {
+		int len = linelen(line, size);
+		if (!len)
+			break;
+
+		switch (*line) {
+		case ' ':
+			fragment->oldlines++;
+			/* fall through */
+		case '+':
+			fragment->newlines++;
+			break;
+		case '-':
+			fragment->oldlines++;
+			break;
+		default:
+			/* Probably "diff ..." */
+			return 0;
+		}
+
+		size -= len;
+		line += len;
+		if (size < 2 || !prefixcmp(line, "@@"))
+			break;
+	}
+	return 0;
+}
+
 /*
  * Parse a unified diff fragment header of the
  * form "@@ -a,b +c,d @@"
@@ -1013,6 +1049,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->ignore_hunk_headers &&
+			fixup_counts(line + offset, size - offset, fragment))
+		return -1;
 	oldlines = fragment->oldlines;
 	newlines = fragment->newlines;
 	leading = 0;
@@ -2912,7 +2951,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 ignore_hunk_headers)
 {
 	size_t offset;
 	struct strbuf buf;
@@ -2929,6 +2969,7 @@ static int apply_patch(int fd, const char *filename, int inaccurate_eof)
 
 		patch = xcalloc(1, sizeof(*patch));
 		patch->inaccurate_eof = inaccurate_eof;
+		patch->ignore_hunk_headers = ignore_hunk_headers;
 		nr = parse_chunk(buf.buf + offset, buf.len - offset, patch);
 		if (nr < 0)
 			break;
@@ -2998,6 +3039,7 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 	int i;
 	int read_stdin = 1;
 	int inaccurate_eof = 0;
+	int ignore_hunk_headers = 0;
 	int errs = 0;
 	int is_not_gitdir;
 
@@ -3015,7 +3057,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,
+					ignore_hunk_headers);
 			read_stdin = 0;
 			continue;
 		}
@@ -3118,6 +3161,10 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 			inaccurate_eof = 1;
 			continue;
 		}
+		if (!strcmp(arg, "--ignore-hunk-headers")) {
+			ignore_hunk_headers = 1;
+			continue;
+		}
 		if (0 < prefix_length)
 			arg = prefix_filename(prefix, prefix_length, arg);
 
@@ -3126,12 +3173,14 @@ 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,
+				ignore_hunk_headers);
 		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,
+				ignore_hunk_headers);
 	if (whitespace_error) {
 		if (squelch_whitespace_errors &&
 		    squelch_whitespace_errors < whitespace_error) {
-- 
1.5.6.rc1.181.gb439d

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

[Johannes Schindelin]

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>
---
 Documentation/git-add.txt |    9 ++++-
 builtin-add.c             |   47 ++++++++++++++++++++++++-
 t/t3702-add-edit.sh       |   86 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 139 insertions(+), 3 deletions(-)
 create mode 100755 t/t3702-add-edit.sh

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 1afd0c6..dd744f1 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -8,8 +8,8 @@ git-add - Add file contents to the index
 SYNOPSIS
 --------
 [verse]
-'git-add' [-n] [-v] [-f] [--interactive | -i] [--patch | -p] [-u] [--refresh]
-	  [--ignore-errors] [--] <filepattern>...
+'git-add' [-n] [-v] [-f] [--interactive | -i] [--patch | -p] [--edit | -e]
+	  [-u] [--refresh] [--ignore-errors] [--] <filepattern>...
 
 DESCRIPTION
 -----------
@@ -70,6 +70,11 @@ 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.
+
 -u::
 	Update only files that git already knows about, staging modified
 	content for commit and marking deleted files for removal. This
diff --git a/builtin-add.c b/builtin-add.c
index 1da22ee..216f331 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,48 @@ int interactive_add(int argc, const char **argv, const char *prefix)
 	return status;
 }
 
+int edit_patch(int argc, const char **argv, const char *prefix)
+{
+	static struct lock_file lock;
+	struct child_process child;
+	int 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 = hold_lock_file_for_update(&lock, git_path("EDIT_PATCH"), 1);
+
+	if (run_command(&child))
+		return 1;
+	free(child.argv);
+
+	launch_editor(lock.filename, NULL, NULL);
+
+	if (stat(lock.filename, &st))
+		return 1;
+	if (!st.st_size)
+		die ("Empty patch. Aborted.");
+
+	execl_git_cmd("apply", "--ignore-hunk-headers", "--cached",
+			lock.filename, NULL);
+
+	return 1;
+}
+
 static struct lock_file lock_file;
 
 static const char ignore_error[] =
@@ -200,6 +242,7 @@ static struct option builtin_add_options[] = {
 	OPT_GROUP(""),
 	OPT_BOOLEAN('i', "interactive", &add_interactive, "interactive picking"),
 	OPT_BOOLEAN('p', "patch", &patch_interactive, "interactive patching"),
+	OPT_BOOLEAN('e', "edit", &edit_interactive, "super-interactive patching"),
 	OPT_BOOLEAN('f', NULL, &ignored_too, "allow adding otherwise ignored files"),
 	OPT_BOOLEAN('u', NULL, &take_worktree_changes, "update tracked files"),
 	OPT_BOOLEAN( 0 , "refresh", &refresh_only, "don't add, only refresh the index"),
@@ -226,6 +269,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..be2f4da
--- /dev/null
+++ b/t/t3702-add-edit.sh
@@ -0,0 +1,86 @@
+#!/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 "$1" orig-patch &&
+mv 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
+
+'
+
+test_done
-- 
1.5.6.rc1.181.gb439d

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

[Pieter de Bie]

On 5 jun 2008, at 18:20, Johannes Schindelin wrote:

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

Nice feature! However, the lockfile isn't deleted on my system (OS X),
perhaps because the atexit() isn't called after an exec(). How about  
this
patch?

diff --git a/builtin-add.c b/builtin-add.c
index 05ae40d..07fdd2e 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -192,6 +192,8 @@ int edit_patch(int argc, const char **argv, const  
char *prefix)
         struct child_process child;
         int ac;
         struct stat st;
+       const char * apply_args[] = { "apply", "--fixup-line-counts",
+                                     "--cached", lock.filename, NULL };

         memset(&child, 0, sizeof(child));
         child.argv = xcalloc(sizeof(const char *), (argc + 5));
@@ -224,10 +226,11 @@ int edit_patch(int argc, const char **argv,  
const char *prefix)
                 return 0;
         }

-       execl_git_cmd("apply", "--fixup-line-counts", "--cached",
-                       lock.filename, NULL);
+       child.argv = apply_args;
+       if (run_command(&child))
+               return 1;

-       return 1;
+       return 0;
  }

  static struct lock_file lock_file;

Re: [PATCH 1/2] Allow git-apply to fix up the line counts

[Junio C Hamano]

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

> As I said, the data is in the _hunks_, but I maybe should have added _not 
> in the hunk headers_.

So you _are_ ignoring the line counts recorded in the hunk headers.  It is
not even 'adjust' but 'count lines to guess'.

If the incoming patch text does not have confusing contents at the end,
the guessing is reasonably safe.  You need to watch out for blank lines,
which means the same as /^ $/, and mail signature separators /^-- $/.
They can confuse you into guessing wrongly and include more preimage lines
than there actually are.

So it would be more like

--ignore-line-counts::
	Ignore number of lines recorded in the hunk headers; instead count
        lines that look like hunk contents to determine how big each hunk
	is.

I haven't started to nitpick the actual code yet but I know the original
is a tricky pice of code, so we may find something interesting ;-)

Re: [PATCH v2 1/2] Allow git-apply to ignore the hunk headers

[Junio C Hamano]

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

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

Two comments and a half.

 * Latest POSIX draft talks about unified context and allows an empty line
   to represent an empty common context line.  GNU diff already emits such
   a diff.  fixup_counts() should take this into account.

 * I'd sleep better at night if 'Probably "diff ..."' part were written in
   a bit more robust way.

 * (minor) There is an established term for this operation: recountdiff,
   so --recount might be a better name.  fixup_counts() also is better
   called recount_diff() if we go this route.

If you are too narrowly focused to only support "git add -e", the first
issue does not matter, because we always emit "SP LF" for such a common
context.  The reason why I care about the first two points is because we
may want to teach git-am about this new option as well in 1.6.0.

And the robustness issue I worry about the second point also applies to a
line that is "^-- $", especially if we were to make this available to
git-am.  Perhaps when the line begins with a '-', the logic could be extra
careful to detect the case where the line looks like the e-mail signature
separator and check one line beyond it to see if it does not look anything
like part of a diff (in which case you stop, without considering the line
you are currently looking at, "^-- $", a deletion of "^- $", as part of
the preimage context).

As to code structure, we might want to make the later parameters to
apply_patch() an integer, of OR'ed flag values, or even a pointer to a
structure that holds options.

Other than that, the patch looks reasonably isolated and clean.

Re: [PATCH v2 1/2] Allow git-apply to ignore the hunk headers

[Johannes Schindelin]

Hi,

On Thu, 5 Jun 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > 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.
> 
> Two comments and a half.
> 
>  * Latest POSIX draft talks about unified context and allows an empty line
>    to represent an empty common context line.  GNU diff already emits such
>    a diff.  fixup_counts() should take this into account.

As you pointed out, I wanted to support only add -e.  But that should not 
be an issue at all.  I think a "case ' ': case '\n':" should be enough, 
right?

>  * I'd sleep better at night if 'Probably "diff ..."' part were written 
>    in a bit more robust way.

How about stopping on "@@" and end of file only, and complaining 
otherwise?

>  * (minor) There is an established term for this operation: recountdiff, 
>    so --recount might be a better name.  fixup_counts() also is better 
>    called recount_diff() if we go this route.

Fine!

> If you are too narrowly focused to only support "git add -e", the first 
> issue does not matter, because we always emit "SP LF" for such a common 
> context.  The reason why I care about the first two points is because we 
> may want to teach git-am about this new option as well in 1.6.0.

Point taken.

> And the robustness issue I worry about the second point also applies to 
> a line that is "^-- $", especially if we were to make this available to 
> git-am.  Perhaps when the line begins with a '-', the logic could be 
> extra careful to detect the case where the line looks like the e-mail 
> signature separator and check one line beyond it to see if it does not 
> look anything like part of a diff (in which case you stop, without 
> considering the line you are currently looking at, "^-- $", a deletion 
> of "^- $", as part of the preimage context).

Is this really an issue?  fixup_counts() is only called after a hunk 
header was read, and that should be well after any "^-- $".

> As to code structure, we might want to make the later parameters to 
> apply_patch() an integer, of OR'ed flag values, or even a pointer to a 
> structure that holds options.

Right.

Will fix up and resubmit.

Ciao,
Dscho

[PATCH v3 0/2] git add --edit

[Johannes Schindelin]

Changes relative to v2:

- it works now not by chance, but by design,

- empty lines are interpreted as if they contained a single space,

- it works when adding lines to the beginning or end of a file, and

- the apply option has been renamed to --recount, as per Junio's request.

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

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

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

[Johannes Schindelin]

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             |   64 ++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 66 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index 2dec2ec..2fa660e 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>...]
 
@@ -169,6 +169,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
 	current patch being applied will be printed. This option will cause
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.rc1.181.gb439d

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

[Johannes Schindelin]

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>
---
 Documentation/git-add.txt |   13 ++++-
 builtin-add.c             |   55 +++++++++++++++++++-
 t/t3702-add-edit.sh       |  126 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 191 insertions(+), 3 deletions(-)
 create mode 100755 t/t3702-add-edit.sh

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 1afd0c6..8620ae2 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -8,8 +8,8 @@ git-add - Add file contents to the index
 SYNOPSIS
 --------
 [verse]
-'git-add' [-n] [-v] [-f] [--interactive | -i] [--patch | -p] [-u] [--refresh]
-	  [--ignore-errors] [--] <filepattern>...
+'git-add' [-n] [-v] [-f] [--interactive | -i] [--patch | -p] [--edit | -e]
+	  [-u] [--refresh] [--ignore-errors] [--] <filepattern>...
 
 DESCRIPTION
 -----------
@@ -70,6 +70,15 @@ 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 only files that git already knows about, staging modified
 	content for commit and marking deleted files for removal. This
diff --git a/builtin-add.c b/builtin-add.c
index 1da22ee..fe31453 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[] =
@@ -200,6 +250,7 @@ static struct option builtin_add_options[] = {
 	OPT_GROUP(""),
 	OPT_BOOLEAN('i', "interactive", &add_interactive, "interactive picking"),
 	OPT_BOOLEAN('p', "patch", &patch_interactive, "interactive patching"),
+	OPT_BOOLEAN('e', "edit", &edit_interactive, "super-interactive patching"),
 	OPT_BOOLEAN('f', NULL, &ignored_too, "allow adding otherwise ignored files"),
 	OPT_BOOLEAN('u', NULL, &take_worktree_changes, "update tracked files"),
 	OPT_BOOLEAN( 0 , "refresh", &refresh_only, "don't add, only refresh the index"),
@@ -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.rc1.181.gb439d

Re: [PATCH v2 1/2] Allow git-apply to ignore the hunk headers

[Junio C Hamano]

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

>> And the robustness issue I worry about the second point also applies to 
>> a line that is "^-- $", especially if we were to make this available to 
>> git-am.  Perhaps when the line begins with a '-', the logic could be 
>> extra careful to detect the case where the line looks like the e-mail 
>> signature separator and check one line beyond it to see if it does not 
>> look anything like part of a diff (in which case you stop, without 
>> considering the line you are currently looking at, "^-- $", a deletion 
>> of "^- $", as part of the preimage context).
>
> Is this really an issue?  fixup_counts() is only called after a hunk 
> header was read, and that should be well after any "^-- $".

Are you talking about "^-- $" or "^---$"?  Yes we are way past the
three-dash separator at this point, but e-mail signature separator happens
at the very end after the patch.

You read a hunk header line "@@ -l,m +n,o @@", and start counting the diff
text because you do not trust m and o.  When you read the last hunk in a
patch e-mail, you may hit a e-mail signature separator, like what is given
by format-patch output at the end.  Mistaking that as an extra preimage
context to remove "^- $" is what I was worried about.

-- 
I worry, therefore I am...

Re: [PATCH v2 1/2] Allow git-apply to ignore the hunk headers

[Johannes Schindelin]

Hi,

On Thu, 5 Jun 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> And the robustness issue I worry about the second point also applies to 
> >> a line that is "^-- $", especially if we were to make this available to 
> >> git-am.  Perhaps when the line begins with a '-', the logic could be 
> >> extra careful to detect the case where the line looks like the e-mail 
> >> signature separator and check one line beyond it to see if it does not 
> >> look anything like part of a diff (in which case you stop, without 
> >> considering the line you are currently looking at, "^-- $", a deletion 
> >> of "^- $", as part of the preimage context).
> >
> > Is this really an issue?  fixup_counts() is only called after a hunk 
> > header was read, and that should be well after any "^-- $".
> 
> Are you talking about "^-- $" or "^---$"?  Yes we are way past the 
> three-dash separator at this point, but e-mail signature separator 
> happens at the very end after the patch.

Oh yes, I was thinking about the "^---$".

> You read a hunk header line "@@ -l,m +n,o @@", and start counting the 
> diff text because you do not trust m and o.  When you read the last hunk 
> in a patch e-mail, you may hit a e-mail signature separator, like what 
> is given by format-patch output at the end.  Mistaking that as an extra 
> preimage context to remove "^- $" is what I was worried about.
> 
> -- 
> I worry, therefore I am...

Nice signature...

I could check for garbage after a line that consists of exactly "^-- $" 
with something like this (on top of 1/2):

@@ -0,0 +0,0 @@
 	default:
-		return -1;
+		return len != 4 && memcmp(line - len, "-- \n", len);
 	}

Hmm?

However, this will not work if anybody has a signature starting with 
"@@ ", "+", " ", "-" or "diff "...

Ciao,
Dscho

-- 
Won't dorry, he bappy

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

[Junio C Hamano]

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

> +static int recount_diff(char *line, int size, struct fragment *fragment)
> +{
> +	int line_nr = 0;

At this point, line points at the beginning of the line that immediately
follows "@@ -oldpos,oldlines +newpos,newlines @@ ...\n", right?

> +	if (size < 1)
> +		return -1;
> +
> +	fragment->oldpos = 2;

Why do you discard oldpos information, and use magic number "2"?

> +	fragment->oldlines = fragment->newlines = 0;
> +
> +	for (;;) {
> +		int len = linelen(line, size);
> +		size -= len;
> +		line += len;

And you look at the line of the patch, measure how long it is, and you
already advance line to point at the next line without ever looking at the
contents of the line (you could look at line[-len], but that is crazy).

> +		if (size < 1)
> +			return 0;

Why?  It may be the last line in the hunk but you haven't done anything to
the current line yet.

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

Again, why muck with oldpos?  Also leading and trailing?  After you
recount the newlines and oldlines you ignored from the hunk header, the
caller will behave as if the original patch had the right numbers on the
hunk header to compute leading and trailing, doesn't it?

> +			break;
> +		case '@':
> +			return size < 3 || prefixcmp(line, "@@ ");
> +		case 'd':
> +			return size < 5 || prefixcmp(line, "diff ");
> +		default:
> +			return -1;

I do not understand these return values.  Your caller, parse_fragment()
with your patch, gives you chance to recount the old and new line count,
and you are responsible for only recounting them.  The change you made to
parse_fragment() returns error, aborting the whole git-apply, when
recount_diff() returns non-zero, but having extra lines after the patch
text is perfectly fine and there is no reason to force aborting from
here.  IOW, this function should not even have power to do so.  The one
and only thing this function should do well is to reliably count the
number of patch lines.

> @@ -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 this "return -1" is uncalled for.

Re: [PATCH v3 0/2] git add --edit

[Junio C Hamano]

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

> Changes relative to v2:
>
> - it works now not by chance, but by design,

Funny.  The old one worked by chance?

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

[Govind Salinas]

On Thu, Jun 5, 2008 at 6:06 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> +
> +               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++;
> +       }
> +}

Perhaps this is accounted for and I did not see, but I believe that
a backslash is used for the "no newline at end of file" line.  Does that
need to be allowed here?

Thanks,
Govind.

Re: [PATCH v2 1/2] Allow git-apply to ignore the hunk headers

[Paolo Bonzini]

> @@ -0,0 +0,0 @@
>  	default:
> -		return -1;
> +		return len != 4 && memcmp(line - len, "-- \n", len);
>  	}

You're never returning -1 here, right?

> However, this will not work if anybody has a signature starting with 
> "@@ ", "+", " ", "-" or "diff "...

I think that the main worry is the patches made with git-format-patch, 
and those are not problematic.

Paolo

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

[Olivier Marin]

Johannes Schindelin a écrit :
>  
> +int edit_patch(int argc, const char **argv, const char *prefix)
> +{

[...]

> +	if (!result)
> +		result = run_command(&child);
> +	free(child.argv);
> +
> +	launch_editor(file, NULL, NULL);

Here, it does not launch the editor I defined with core.editor because you
call edit_patch() before calling git_config() in cmd_add().

Also, wouldn't be better to have the edit_patch stuff in add--interactive
instead ? It seems to work the same way than the --patch option.

Just my thoughts.

Olivier.

Re: [PATCH v2 1/2] Allow git-apply to ignore the hunk headers

[Sergei Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> And the robustness issue I worry about the second point also applies to 
>>> a line that is "^-- $", especially if we were to make this available to 
>>> git-am.  Perhaps when the line begins with a '-', the logic could be 
>>> extra careful to detect the case where the line looks like the e-mail 
>>> signature separator and check one line beyond it to see if it does not 
>>> look anything like part of a diff (in which case you stop, without 
>>> considering the line you are currently looking at, "^-- $", a deletion 
>>> of "^- $", as part of the preimage context).
>>
>> Is this really an issue?  fixup_counts() is only called after a hunk 
>> header was read, and that should be well after any "^-- $".
>
> Are you talking about "^-- $" or "^---$"?  Yes we are way past the
> three-dash separator at this point, but e-mail signature separator happens
> at the very end after the patch.
>
> You read a hunk header line "@@ -l,m +n,o @@", and start counting the diff
> text because you do not trust m and o.  When you read the last hunk in a
> patch e-mail, you may hit a e-mail signature separator, like what is given
> by format-patch output at the end.  Mistaking that as an extra preimage
> context to remove "^- $" is what I was worried about.

Don't you think it's time to fix git-format-patch to put some reliable
"end-of-patch" marker line before the signature? This change (along with
refusal to generate brain-damaged empty lines inside hunks) will make
git diffs easily parseable without information from hunk headers.

-- Sergei.

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

[Johannes Schindelin]

Hi,

On Thu, 5 Jun 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > +static int recount_diff(char *line, int size, struct fragment *fragment)
> > +{
> > +	int line_nr = 0;
> 
> At this point, line points at the beginning of the line that immediately
> follows "@@ -oldpos,oldlines +newpos,newlines @@ ...\n", right?

No.  At this point, it points directly after the last "@@", but still on 
the same line.

> > +	if (size < 1)
> > +		return -1;
> > +
> > +	fragment->oldpos = 2;
> 
> Why do you discard oldpos information, and use magic number "2"?

Because the old information should be ignored.  If the first line is a "+" 
line, the line number needs to be set to 1, otherwise the patch will not 
apply.

Maybe the easiest would be to set it to 1 regardless of the hunk.

> > +	fragment->oldlines = fragment->newlines = 0;
> > +
> > +	for (;;) {
> > +		int len = linelen(line, size);
> > +		size -= len;
> > +		line += len;
> 
> And you look at the line of the patch, measure how long it is, and you
> already advance line to point at the next line without ever looking at the
> contents of the line (you could look at line[-len], but that is crazy).

No, as I said, the "line" parameter points just after "@@" of the hunk 
header.

BTW this is what I referred to when I said that it now works by design; 
formerly, it relied on a space being present after the "@@", which it 
would mistakenly count as a common line, which was the reason I set the 
counts to -1 initially.

> > +		if (size < 1)
> > +			return 0;
> 
> Why?  It may be the last line in the hunk but you haven't done anything to
> the current line yet.

No, see above.

> > +		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;
> 
> Again, why muck with oldpos?  Also leading and trailing?  After you
> recount the newlines and oldlines you ignored from the hunk header, the
> caller will behave as if the original patch had the right numbers on the
> hunk header to compute leading and trailing, doesn't it?

No.  Because I can never know if the _positions_ in the hunk header are 
right.

> > +			break;
> > +		case '@':
> > +			return size < 3 || prefixcmp(line, "@@ ");
> > +		case 'd':
> > +			return size < 5 || prefixcmp(line, "diff ");
> > +		default:
> > +			return -1;
> 
> I do not understand these return values.  Your caller, parse_fragment() 
> with your patch, gives you chance to recount the old and new line count, 
> and you are responsible for only recounting them.  The change you made 
> to parse_fragment() returns error, aborting the whole git-apply, when 
> recount_diff() returns non-zero, but having extra lines after the patch 
> text is perfectly fine and there is no reason to force aborting from 
> here.

Then I understood you not correctly at all when you complained about the 
"Probably a diff" part.

So what do you want?  Should it be anal, or lax?  You can't have both.

> IOW, this function should not even have power to do so.  The one
> and only thing this function should do well is to reliably count the
> number of patch lines.
> 
> > @@ -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 this "return -1" is uncalled for.

Again.  Lax or not lax?

Ciao,
Dscho
 

Re: [PATCH v3 0/2] git add --edit

[Johannes Schindelin]

Hi,

On Thu, 5 Jun 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Changes relative to v2:
> >
> > - it works now not by chance, but by design,
> 
> Funny.  The old one worked by chance?

Yes, it did.  In my tests, I did not realize that the "line" parameter was 
set just after the second "@@" in the hunk header.  Therefore, the code 
counted a space at the beginning of the hunk header comment as a common 
line.

In my later tests, I finally realized my error, and changed the code.

Ciao,
Dscho

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

[Johannes Schindelin]

Hi,

On Fri, 6 Jun 2008, Govind Salinas wrote:

> On Thu, Jun 5, 2008 at 6:06 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > +
> > +               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++;
> > +       }
> > +}
> 
> Perhaps this is accounted for and I did not see, but I believe that
> a backslash is used for the "no newline at end of file" line.  Does that
> need to be allowed here?

Will change.

Ciao,
Dscho

Re: [PATCH v2 1/2] Allow git-apply to ignore the hunk headers

[Johannes Schindelin]

Hi,

On Fri, 6 Jun 2008, Paolo Bonzini wrote:

> > @@ -0,0 +0,0 @@
> > 	default:
> > -		return -1;
> > +		return len != 4 && memcmp(line - len, "-- \n", len);
> >   }
> 
> You're never returning -1 here, right?

You are a clever guy!  I really do not return -1 here.  But then, the 
return value is only checked for non-zeroness.  As is obvious from the 
part you did not quote.

> > However, this will not work if anybody has a signature starting with 
> > "@@ ", "+", " ", "-" or "diff "...
> 
> I think that the main worry is the patches made with git-format-patch, 
> and those are not problematic.

Actually, this change was done in v3 on _explicit_ request from Junio who 
wants to be able to use the patch for git-am, where we cannot rely on 
format-patch.

So yes, they _are_ problematic.

Thanks,
Dscho

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

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 774 bytes --]

Hi,

On Fri, 6 Jun 2008, Olivier Marin wrote:

> Johannes Schindelin a écrit :
> > 
> > +int edit_patch(int argc, const char **argv, const char *prefix)
> > +{
> 
> [...]
> 
> > +	if (!result)
> > +		result = run_command(&child);
> > +	free(child.argv);
> > +
> > +	launch_editor(file, NULL, NULL);
> 
> Here, it does not launch the editor I defined with core.editor because 
> you call edit_patch() before calling git_config() in cmd_add().

Will fix.

> Also, wouldn't be better to have the edit_patch stuff in 
> add--interactive instead ? It seems to work the same way than the 
> --patch option.

Actually, no.  It does something completely different.  For example, it 
avoids calling a perl script.  At least as long as your editor is not a 
Perl script.

Ciao,
Dscho

Re: [PATCH v2 1/2] Allow git-apply to ignore the hunk headers

[Johannes Schindelin]

Hi,

On Fri, 6 Jun 2008, Sergei Organov wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > You read a hunk header line "@@ -l,m +n,o @@", and start counting the 
> > diff text because you do not trust m and o.  When you read the last 
> > hunk in a patch e-mail, you may hit a e-mail signature separator, like 
> > what is given by format-patch output at the end.  Mistaking that as an 
> > extra preimage context to remove "^- $" is what I was worried about.
> 
> Don't you think it's time to fix git-format-patch to put some reliable 
> "end-of-patch" marker line before the signature? This change (along with 
> refusal to generate brain-damaged empty lines inside hunks) will make 
> git diffs easily parseable without information from hunk headers.

Actually, what do you think the numbers in the hunk headers are good for?  
They _are_ reliable end-of-hunk markers.  And if the next line does not 
exist, or does not start, it is end-of-diff.  Reliable. Simple.

Only when you need to get sloppy, things get worse.  I need to get sloppy.

But that's hardly the fault of format-patch.

Ciao,
Dscho

Re: [PATCH v2 1/2] Allow git-apply to ignore the hunk headers

[Junio C Hamano]

Sergei Organov <osv@javad.com> writes:

> Don't you think it's time to fix git-format-patch to put some reliable
> "end-of-patch" marker line before the signature?

Not at all.  git-apply is designed to (and has to) grok any reasonable
e-mailed patch, not just format-patch output.  And we should consider
having e-mail signature separator at the end as "reasonable".

It only becomes an issue when you start deviating from the rule of
reliable diff parsing, such as ignoring the old/new line count, which is
the topic of Dscho's patch.

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

[Junio C Hamano]

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

> Then I understood you not correctly at all when you complained about the 
> "Probably a diff" part.

The wish was this.

When recounting this hunk:

	@@ -l,m +n,o @@$
         preimage$
        -deleted$
        -deleted$
        Some other text

I want you to say "three and one", and not error out.  Reading "Some other
text" and deciding that the stream of fragments for the current patch has
ended is the job for parse_single_patch().

If on the other hand the input were:

	@@ -l,m +n,o @@$
         preimage$
        -deleted$
        -deleted$
	-- $
        This space intentionally left blank.
	perhaps a few more lines of sig
	<<EOF>>

I do not want you to say "four and one" silently.  It is more likely that
the answer is "three and one", and the last line that begins with a '-' is
not part of the diff, but for the sake of robustness, I do not want to
hear "three and one" without warning either.  Erroring out, because you
cannot recount reliably, would be prudent.  We do not have a clever tool
that sometimes does a wrong thing silently.

I was fooled by the "where does the line begin when we come to this
function", so I need to re-read the code and see what you are doing.

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

[Junio C Hamano]

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

>> At this point, line points at the beginning of the line that immediately
>> follows "@@ -oldpos,oldlines +newpos,newlines @@ ...\n", right?
>
> No.  At this point, it points directly after the last "@@", but still on 
> the same line.

Ah, I personally think that is a crazy calling convention but the
function's loop consistently uses the "line at the beginning of iteration
points somewhere in the previous line to be skipped", so it would "work".
I was fooled by that.

>> > +	if (size < 1)
>> > +		return -1;
>> > +
>> > +	fragment->oldpos = 2;
>> 
>> Why do you discard oldpos information, and use magic number "2"?
>
> Because the old information should be ignored.  If the first line is a "+" 
> line, the line number needs to be set to 1, otherwise the patch will not 
> apply.

I do not think line number information should be discarded.  If you have
two blocks of lines that look alike, the line number information does get
used to see which place the hunk should apply.

Deleting the common context lines from the beginning, or adding a new
"+added line" at the very beginning of a hunk, is a user error for
somebody who edits the diff.  Because you are not calling apply with
unidiff-zero, the sanity check applies to such a hunk.  Working around the
sanity check by discarding the line number information to make the patch
application even more error prone is an unacceptable hackery.

> Maybe the easiest would be to set it to 1 regardless of the hunk.

And that is even worse, and I thought you knew a lot better than that.
Sigh...

> Then I understood you not correctly at all when you complained about the 
> "Probably a diff" part.
>
> So what do you want?  Should it be anal, or lax?  You can't have both.

I explained what I wanted to _happen_ in a separate message.  Now to _how_
you would make it happen...

The way you use the return value from here is to cause parse_fragment() to
say "The patch is corrupt".  You do _not_ detect that here.  You are only
counting number of preimage and postimage lines in this function.  If the
next line does not look like a part of the current hunk, you stop counting
(iow, the only side effect you cause is to update oldlines and newlines in
fragment structure) without including that non-patch line, and return.
You let the caller to decide what that next line you excluded from the
current hunk is, because the caller _already_ has logic to decide what is
part of the patch text (it knows not just how hunk meat looks like but
also how hunk headers and "diff " to start the next patch looks like).
You do not want that information or logic here.

So the answer to "anal or lax" is "Neither.  It's none of your business".

>> > +	if (offset > 0 && patch->recount &&
>> > +			recount_diff(line + offset, size - offset, fragment))
>> > +		return -1;
>> 
>> ... and this "return -1" is uncalled for.
>
> Again.  Lax or not lax?

Neither.  This calling site should not even decide.  The only thing
recount will tell its caller parse_fragment() is "I've recounted the
lines, so by iterating that many lines you will reach the end of the
current hunk as I determined.  Decide what the line beyond that is _your_
business, not mine".

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

[Johannes Schindelin]

Hi,

On Fri, 6 Jun 2008, Junio C Hamano wrote:

> Deleting the common context lines from the beginning, or adding a new
> "+added line" at the very beginning of a hunk, is a user error for
> somebody who edits the diff.

Unfortunately, that was exactly what I needed to do.  Not in a theoretical 
sense, but a very practical one.

Only in hindsight do I realize that I could have increased the context, 
but that was not easily possible with the way I drive diff-files via 
run_command().

Oh well, another iteration.

Ciao,
Dscho

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

[Junio C Hamano]

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

> Only in hindsight do I realize that I could have increased the context, 

Yeah, I was thinking about suggesting that, using -U7 or something before
dumping the output to the editor.

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

[Johannes Schindelin]

Hi,

On Fri, 6 Jun 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Only in hindsight do I realize that I could have increased the 
> > context,
> 
> Yeah, I was thinking about suggesting that, using -U7 or something 
> before dumping the output to the editor.

Good idea.

Ciao,
Dscho