[PATCH v5 0/7] Add --graft option to git replace

[PATCH v5 0/7] Add --graft option to git replace

[Christian Couder]

Here is a small patch series to implement:

        git replace [-f] --graft <commit> [<parent>...]

This patch series goes on top of the patch series that
implements --edit.

The changes since v4, thanks to Junio and Peff, are:

- The series has been rebased on top of Peff's series
  to store the commit buffer length
  (jk/commit-buffer-length). This changes patch 1/7
  and fixes the problem that we lost everything after
  a NUL byte in the commit buffer. 

- Patches 5/7, 6/7 and 7/7 have been added to lose
  a gpg signature in the original commit buffer.
  
Notes:

- Maybe patches could be reordered or squashed, but
  I prefered not to do it in this round so that
  changes compared to previous version are easier to
  spot.

- Junio suggested to drop the merge-tag of a parent
  we are not going to keep, but to keep the merge-tag
  of a parent we are keeping. This has not yet been
  done. It is unfortunate that merge-tags don't use
  a format like the trailer format, because we could
  have factorised droping a merge-tag in the
  "git interpret-trailers" command.

Christian Couder (7):
  replace: add --graft option
  replace: add test for --graft
  Documentation: replace: add --graft option
  contrib: add convert-grafts-to-replace-refs.sh
  replace: refactor replacing parents
  replace: remove signature when using --graft
  replace: add test for --graft with signed commit

 Documentation/git-replace.txt             | 10 ++++
 builtin/replace.c                         | 79 ++++++++++++++++++++++++++++++-
 commit.c                                  | 34 +++++++++++++
 commit.h                                  |  2 +
 contrib/convert-grafts-to-replace-refs.sh | 28 +++++++++++
 t/t6050-replace.sh                        | 34 +++++++++++++
 6 files changed, 186 insertions(+), 1 deletion(-)
 create mode 100755 contrib/convert-grafts-to-replace-refs.sh

-- 
2.0.0.421.g786a89d.dirty

[PATCH v5 1/7] replace: add --graft option

[Christian Couder]

The usage string for this option is:

git replace [-f] --graft <commit> [<parent>...]

First we create a new commit that is the same as <commit>
except that its parents are [<parents>...]

Then we create a replace ref that replace <commit> with
the commit we just created.

With this new option, it should be straightforward to
convert grafts to replace refs.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/replace.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 1bb491d..3515979 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -17,6 +17,7 @@
 static const char * const git_replace_usage[] = {
 	N_("git replace [-f] <object> <replacement>"),
 	N_("git replace [-f] --edit <object>"),
+	N_("git replace [-f] --graft <commit> [<parent>...]"),
 	N_("git replace -d <object>..."),
 	N_("git replace [--format=<format>] [-l [<pattern>]]"),
 	NULL
@@ -294,6 +295,58 @@ static int edit_and_replace(const char *object_ref, int force)
 	return replace_object_sha1(object_ref, old, "replacement", new, force);
 }
 
+static int create_graft(int argc, const char **argv, int force)
+{
+	unsigned char old[20], new[20];
+	const char *old_ref = argv[0];
+	struct commit *commit;
+	struct strbuf buf = STRBUF_INIT;
+	struct strbuf new_parents = STRBUF_INIT;
+	const char *parent_start, *parent_end;
+	const char *buffer;
+	unsigned long size;
+	int i;
+
+	if (get_sha1(old_ref, old) < 0)
+		die(_("Not a valid object name: '%s'"), old_ref);
+	commit = lookup_commit_or_die(old, old_ref);
+
+	/* find existing parents */
+	buffer = get_commit_buffer(commit, &size);
+	strbuf_add(&buf, buffer, size);
+	unuse_commit_buffer(commit, buffer);
+	parent_start = buf.buf;
+	parent_start += 46; /* "tree " + "hex sha1" + "\n" */
+	parent_end = parent_start;
+
+	while (starts_with(parent_end, "parent "))
+		parent_end += 48; /* "parent " + "hex sha1" + "\n" */
+
+	/* prepare new parents */
+	for (i = 1; i < argc; i++) {
+		unsigned char sha1[20];
+		if (get_sha1(argv[i], sha1) < 0)
+			die(_("Not a valid object name: '%s'"), argv[i]);
+		lookup_commit_or_die(sha1, argv[i]);
+		strbuf_addf(&new_parents, "parent %s\n", sha1_to_hex(sha1));
+	}
+
+	/* replace existing parents with new ones */
+	strbuf_splice(&buf, parent_start - buf.buf, parent_end - parent_start,
+		      new_parents.buf, new_parents.len);
+
+	if (write_sha1_file(buf.buf, buf.len, commit_type, new))
+		die(_("could not write replacement commit for: '%s'"), old_ref);
+
+	strbuf_release(&new_parents);
+	strbuf_release(&buf);
+
+	if (!hashcmp(old, new))
+		return error("new commit is the same as the old one: '%s'", sha1_to_hex(old));
+
+	return replace_object_sha1(old_ref, old, "replacement", new, force);
+}
+
 int cmd_replace(int argc, const char **argv, const char *prefix)
 {
 	int force = 0;
@@ -303,12 +356,14 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
 		MODE_LIST,
 		MODE_DELETE,
 		MODE_EDIT,
+		MODE_GRAFT,
 		MODE_REPLACE
 	} cmdmode = MODE_UNSPECIFIED;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list replace refs"), MODE_LIST),
 		OPT_CMDMODE('d', "delete", &cmdmode, N_("delete replace refs"), MODE_DELETE),
 		OPT_CMDMODE('e', "edit", &cmdmode, N_("edit existing object"), MODE_EDIT),
+		OPT_CMDMODE('g', "graft", &cmdmode, N_("change a commit's parents"), MODE_GRAFT),
 		OPT_BOOL('f', "force", &force, N_("replace the ref if it exists")),
 		OPT_STRING(0, "format", &format, N_("format"), N_("use this format")),
 		OPT_END()
@@ -325,7 +380,10 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
 		usage_msg_opt("--format cannot be used when not listing",
 			      git_replace_usage, options);
 
-	if (force && cmdmode != MODE_REPLACE && cmdmode != MODE_EDIT)
+	if (force &&
+	    cmdmode != MODE_REPLACE &&
+	    cmdmode != MODE_EDIT &&
+	    cmdmode != MODE_GRAFT)
 		usage_msg_opt("-f only makes sense when writing a replacement",
 			      git_replace_usage, options);
 
@@ -348,6 +406,12 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
 				      git_replace_usage, options);
 		return edit_and_replace(argv[0], force);
 
+	case MODE_GRAFT:
+		if (argc < 1)
+			usage_msg_opt("-g needs at least one argument",
+				      git_replace_usage, options);
+		return create_graft(argc, argv, force);
+
 	case MODE_LIST:
 		if (argc > 1)
 			usage_msg_opt("only one pattern can be given with -l",
-- 
2.0.0.421.g786a89d.dirty

[PATCH v5 2/7] replace: add test for --graft

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t6050-replace.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index 68b3cb2..ca45a84 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -351,4 +351,16 @@ test_expect_success 'replace ref cleanup' '
 	test -z "$(git replace)"
 '
 
+test_expect_success '--graft with and without already replaced object' '
+	test $(git log --oneline | wc -l) = 7 &&
+	git replace --graft $HASH5 &&
+	test $(git log --oneline | wc -l) = 3 &&
+	git cat-file -p $HASH5 | test_must_fail grep parent &&
+	test_must_fail git replace --graft $HASH5 $HASH4 $HASH3 &&
+	git replace --force -g $HASH5 $HASH4 $HASH3 &&
+	git cat-file -p $HASH5 | grep "parent $HASH4" &&
+	git cat-file -p $HASH5 | grep "parent $HASH3" &&
+	git replace -d $HASH5
+'
+
 test_done
-- 
2.0.0.421.g786a89d.dirty

[PATCH v5 3/7] Documentation: replace: add --graft option

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-replace.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt
index 61461b9..491875e 100644
--- a/Documentation/git-replace.txt
+++ b/Documentation/git-replace.txt
@@ -10,6 +10,7 @@ SYNOPSIS
 [verse]
 'git replace' [-f] <object> <replacement>
 'git replace' [-f] --edit <object>
+'git replace' [-f] --graft <commit> [<parent>...]
 'git replace' -d <object>...
 'git replace' [--format=<format>] [-l [<pattern>]]
 
@@ -73,6 +74,13 @@ OPTIONS
 	newly created object. See linkgit:git-var[1] for details about
 	how the editor will be chosen.
 
+--graft <commit> [<parent>...]::
+	Create a graft commit. A new commit is created with the same
+	content as <commit> except that its parents will be
+	[<parent>...] instead of <commit>'s parents. A replacement ref
+	is then created to replace <commit> with the newly created
+	commit.
+
 -l <pattern>::
 --list <pattern>::
 	List replace refs for objects that match the given pattern (or
-- 
2.0.0.421.g786a89d.dirty

[PATCH v5 4/7] contrib: add convert-grafts-to-replace-refs.sh

[Christian Couder]

This patch adds into contrib/ an example script to convert
grafts from an existing grafts file into replace refs using
the new --graft option of "git replace".

While at it let's mention this new script in the
"git replace" documentation for the --graft option.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-replace.txt             |  4 +++-
 contrib/convert-grafts-to-replace-refs.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)
 create mode 100755 contrib/convert-grafts-to-replace-refs.sh

diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt
index 491875e..e1be2e6 100644
--- a/Documentation/git-replace.txt
+++ b/Documentation/git-replace.txt
@@ -79,7 +79,9 @@ OPTIONS
 	content as <commit> except that its parents will be
 	[<parent>...] instead of <commit>'s parents. A replacement ref
 	is then created to replace <commit> with the newly created
-	commit.
+	commit. See contrib/convert-grafts-to-replace-refs.sh for an
+	example script based on this option that can convert grafts to
+	replace refs.
 
 -l <pattern>::
 --list <pattern>::
diff --git a/contrib/convert-grafts-to-replace-refs.sh b/contrib/convert-grafts-to-replace-refs.sh
new file mode 100755
index 0000000..0cbc917
--- /dev/null
+++ b/contrib/convert-grafts-to-replace-refs.sh
@@ -0,0 +1,28 @@
+#!/bin/sh
+
+# You should execute this script in the repository where you
+# want to convert grafts to replace refs.
+
+GRAFTS_FILE="${GIT_DIR:-.git}/info/grafts"
+
+. $(git --exec-path)/git-sh-setup
+
+test -f "$GRAFTS_FILE" || die "Could not find graft file: '$GRAFTS_FILE'"
+
+grep '^[^# ]' "$GRAFTS_FILE" |
+while read definition
+do
+	if test -n "$definition"
+	then
+		echo "Converting: $definition"
+		git replace --graft $definition ||
+			die "Conversion failed for: $definition"
+	fi
+done
+
+mv "$GRAFTS_FILE" "$GRAFTS_FILE.bak" ||
+	die "Could not rename '$GRAFTS_FILE' to '$GRAFTS_FILE.bak'"
+
+echo "Success!"
+echo "All the grafts in '$GRAFTS_FILE' have been converted to replace refs!"
+echo "The grafts file '$GRAFTS_FILE' has been renamed: '$GRAFTS_FILE.bak'"
-- 
2.0.0.421.g786a89d.dirty

[PATCH v5 5/7] replace: refactor replacing parents

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/replace.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 3515979..ad47237 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -295,27 +295,14 @@ static int edit_and_replace(const char *object_ref, int force)
 	return replace_object_sha1(object_ref, old, "replacement", new, force);
 }
 
-static int create_graft(int argc, const char **argv, int force)
+static void replace_parents(struct strbuf *buf, int argc, const char **argv)
 {
-	unsigned char old[20], new[20];
-	const char *old_ref = argv[0];
-	struct commit *commit;
-	struct strbuf buf = STRBUF_INIT;
 	struct strbuf new_parents = STRBUF_INIT;
 	const char *parent_start, *parent_end;
-	const char *buffer;
-	unsigned long size;
 	int i;
 
-	if (get_sha1(old_ref, old) < 0)
-		die(_("Not a valid object name: '%s'"), old_ref);
-	commit = lookup_commit_or_die(old, old_ref);
-
 	/* find existing parents */
-	buffer = get_commit_buffer(commit, &size);
-	strbuf_add(&buf, buffer, size);
-	unuse_commit_buffer(commit, buffer);
-	parent_start = buf.buf;
+	parent_start = buf->buf;
 	parent_start += 46; /* "tree " + "hex sha1" + "\n" */
 	parent_end = parent_start;
 
@@ -332,13 +319,34 @@ static int create_graft(int argc, const char **argv, int force)
 	}
 
 	/* replace existing parents with new ones */
-	strbuf_splice(&buf, parent_start - buf.buf, parent_end - parent_start,
+	strbuf_splice(buf, parent_start - buf->buf, parent_end - parent_start,
 		      new_parents.buf, new_parents.len);
 
+	strbuf_release(&new_parents);
+}
+
+static int create_graft(int argc, const char **argv, int force)
+{
+	unsigned char old[20], new[20];
+	const char *old_ref = argv[0];
+	struct commit *commit;
+	struct strbuf buf = STRBUF_INIT;
+	const char *buffer;
+	unsigned long size;
+
+	if (get_sha1(old_ref, old) < 0)
+		die(_("Not a valid object name: '%s'"), old_ref);
+	commit = lookup_commit_or_die(old, old_ref);
+
+	buffer = get_commit_buffer(commit, &size);
+	strbuf_add(&buf, buffer, size);
+	unuse_commit_buffer(commit, buffer);
+
+	replace_parents(&buf, argc, argv);
+
 	if (write_sha1_file(buf.buf, buf.len, commit_type, new))
 		die(_("could not write replacement commit for: '%s'"), old_ref);
 
-	strbuf_release(&new_parents);
 	strbuf_release(&buf);
 
 	if (!hashcmp(old, new))
-- 
2.0.0.421.g786a89d.dirty

[PATCH v5 6/7] replace: remove signature when using --graft

[Christian Couder]

It could be misleading to keep a signature in a
replacement commit, so let's remove it.

Note that there should probably be a way to sign
the replacement commit created when using --graft,
but this can be dealt with in another commit or
patch series.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/replace.c |  5 +++++
 commit.c          | 34 ++++++++++++++++++++++++++++++++++
 commit.h          |  2 ++
 3 files changed, 41 insertions(+)

diff --git a/builtin/replace.c b/builtin/replace.c
index ad47237..000db65 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -344,6 +344,11 @@ static int create_graft(int argc, const char **argv, int force)
 
 	replace_parents(&buf, argc, argv);
 
+	if (remove_signature(&buf))
+		warning(_("the original commit '%s' has a gpg signature.\n"
+			  "It will be removed in the replacement commit!"),
+			old_ref);
+
 	if (write_sha1_file(buf.buf, buf.len, commit_type, new))
 		die(_("could not write replacement commit for: '%s'"), old_ref);
 
diff --git a/commit.c b/commit.c
index fb7897c..54e157d 100644
--- a/commit.c
+++ b/commit.c
@@ -1177,6 +1177,40 @@ int parse_signed_commit(const struct commit *commit,
 	return saw_signature;
 }
 
+int remove_signature(struct strbuf *buf)
+{
+	const char *line = buf->buf;
+	const char *tail = buf->buf + buf->len;
+	int in_signature = 0;
+	const char *sig_start = NULL;
+	const char *sig_end = NULL;
+
+	while (line < tail) {
+		const char *next = memchr(line, '\n', tail - line);
+		next = next ? next + 1 : tail;
+
+		if (in_signature && line[0] == ' ')
+			sig_end = next;
+		else if (starts_with(line, gpg_sig_header) &&
+			 line[gpg_sig_header_len] == ' ') {
+			sig_start = line;
+			sig_end = next;
+			in_signature = 1;
+		} else {
+			if (*line == '\n')
+				/* dump the whole remainder of the buffer */
+				next = tail;
+			in_signature = 0;
+		}
+		line = next;
+	}
+
+	if (sig_start)
+		strbuf_remove(buf, sig_start - buf->buf, sig_end - sig_start);
+
+	return sig_start != NULL;
+}
+
 static void handle_signed_tag(struct commit *parent, struct commit_extra_header ***tail)
 {
 	struct merge_remote_desc *desc;
diff --git a/commit.h b/commit.h
index 2e1492a..4234dae 100644
--- a/commit.h
+++ b/commit.h
@@ -327,6 +327,8 @@ struct commit *get_merge_parent(const char *name);
 
 extern int parse_signed_commit(const struct commit *commit,
 			       struct strbuf *message, struct strbuf *signature);
+extern int remove_signature(struct strbuf *buf);
+
 extern void print_commit_list(struct commit_list *list,
 			      const char *format_cur,
 			      const char *format_last);
-- 
2.0.0.421.g786a89d.dirty

[PATCH v5 7/7] replace: add test for --graft with signed commit

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t6050-replace.sh | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index ca45a84..80b85e3 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -7,6 +7,7 @@ test_description='Tests replace refs functionality'
 exec </dev/null
 
 . ./test-lib.sh
+. "$TEST_DIRECTORY/lib-gpg.sh"
 
 add_and_commit_file()
 {
@@ -363,4 +364,25 @@ test_expect_success '--graft with and without already replaced object' '
 	git replace -d $HASH5
 '
 
+test_expect_success GPG 'set up a signed commit' '
+	echo "line 17" >> hello &&
+	echo "line 18" >> hello &&
+	git add hello &&
+	test_tick &&
+	git commit --quiet -S -m "hello: 2 more lines in a signed commit" &&
+	HASH8=$(git rev-parse --verify HEAD) &&
+	git verify-commit $HASH8
+'
+
+test_expect_success GPG '--graft with a signed commit' '
+	git cat-file commit $HASH8 >orig &&
+	git replace --graft $HASH8 &&
+	git cat-file commit $HASH8 >repl &&
+	test_must_fail grep gpgsig repl &&
+	diff -u orig repl | grep "^-parent $HASH7" &&
+	diff -u orig repl | grep "^-gpgsig -----BEGIN PGP SIGNATURE-----" &&
+	test_must_fail git verify-commit $HASH8 &&
+	git replace -d $HASH8
+'
+
 test_done
-- 
2.0.0.421.g786a89d.dirty

Re: [PATCH v5 2/7] replace: add test for --graft

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/t6050-replace.sh | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
> index 68b3cb2..ca45a84 100755
> --- a/t/t6050-replace.sh
> +++ b/t/t6050-replace.sh
> @@ -351,4 +351,16 @@ test_expect_success 'replace ref cleanup' '
>  	test -z "$(git replace)"
>  '
>  
> +test_expect_success '--graft with and without already replaced object' '
> +	test $(git log --oneline | wc -l) = 7 &&
> +	git replace --graft $HASH5 &&
> +	test $(git log --oneline | wc -l) = 3 &&
> +	git cat-file -p $HASH5 | test_must_fail grep parent &&

Please do not run non-git command under test_must_fail.

> +	test_must_fail git replace --graft $HASH5 $HASH4 $HASH3 &&
> +	git replace --force -g $HASH5 $HASH4 $HASH3 &&
> +	git cat-file -p $HASH5 | grep "parent $HASH4" &&
> +	git cat-file -p $HASH5 | grep "parent $HASH3" &&
> +	git replace -d $HASH5
> +'
> +
>  test_done

For all these "git cat-file -p $commit | grep ...", I would think
you should add "commit_has_parents" helper function to allow a bit
more careful testing.  As written, the test will mistake a commit
with string "parent $HASHx" anywhere, not in the header part, and
the test does not ensure that $HASH{3,4} is the {first,second}
parent.

	commit_has_parents $HASH5 $HASH4 $HASH3

would then may be implemented like:

	commit_has_parents () {
		git cat-file commit "$1" >payload &&
                sed -n -e '/^$/q' -e 's/^parent //p' <payload >actual &&
		shift &&
                printf 'parent %s\n' "$@" >expect &&
		test_cmp expect actual
	}

Re: [PATCH v5 5/7] replace: refactor replacing parents

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  builtin/replace.c | 42 +++++++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/builtin/replace.c b/builtin/replace.c
> index 3515979..ad47237 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -295,27 +295,14 @@ static int edit_and_replace(const char *object_ref, int force)
>  	return replace_object_sha1(object_ref, old, "replacement", new, force);
>  }
>  
> -static int create_graft(int argc, const char **argv, int force)
> +static void replace_parents(struct strbuf *buf, int argc, const char **argv)

It is somewhat strange to see that a new function introduced earlier
in the series is rewritten with a "refactoring".  Shouldn't the new
function have been done right from the beginning instead?

>  {
> -	unsigned char old[20], new[20];
> -	const char *old_ref = argv[0];
> -	struct commit *commit;
> -	struct strbuf buf = STRBUF_INIT;
>  	struct strbuf new_parents = STRBUF_INIT;
>  	const char *parent_start, *parent_end;
> -	const char *buffer;
> -	unsigned long size;
>  	int i;
>  
> -	if (get_sha1(old_ref, old) < 0)
> -		die(_("Not a valid object name: '%s'"), old_ref);
> -	commit = lookup_commit_or_die(old, old_ref);
> -
>  	/* find existing parents */
> -	buffer = get_commit_buffer(commit, &size);
> -	strbuf_add(&buf, buffer, size);
> -	unuse_commit_buffer(commit, buffer);
> -	parent_start = buf.buf;
> +	parent_start = buf->buf;
>  	parent_start += 46; /* "tree " + "hex sha1" + "\n" */
>  	parent_end = parent_start;
>  
> @@ -332,13 +319,34 @@ static int create_graft(int argc, const char **argv, int force)
>  	}
>  
>  	/* replace existing parents with new ones */
> -	strbuf_splice(&buf, parent_start - buf.buf, parent_end - parent_start,
> +	strbuf_splice(buf, parent_start - buf->buf, parent_end - parent_start,
>  		      new_parents.buf, new_parents.len);
>  
> +	strbuf_release(&new_parents);
> +}
> +
> +static int create_graft(int argc, const char **argv, int force)
> +{
> +	unsigned char old[20], new[20];
> +	const char *old_ref = argv[0];
> +	struct commit *commit;
> +	struct strbuf buf = STRBUF_INIT;
> +	const char *buffer;
> +	unsigned long size;
> +
> +	if (get_sha1(old_ref, old) < 0)
> +		die(_("Not a valid object name: '%s'"), old_ref);
> +	commit = lookup_commit_or_die(old, old_ref);
> +
> +	buffer = get_commit_buffer(commit, &size);
> +	strbuf_add(&buf, buffer, size);
> +	unuse_commit_buffer(commit, buffer);
> +
> +	replace_parents(&buf, argc, argv);
> +
>  	if (write_sha1_file(buf.buf, buf.len, commit_type, new))
>  		die(_("could not write replacement commit for: '%s'"), old_ref);
>  
> -	strbuf_release(&new_parents);
>  	strbuf_release(&buf);
>  
>  	if (!hashcmp(old, new))

Re: [PATCH v5 6/7] replace: remove signature when using --graft

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> It could be misleading to keep a signature in a
> replacement commit, so let's remove it.
>
> Note that there should probably be a way to sign
> the replacement commit created when using --graft,
> but this can be dealt with in another commit or
> patch series.

Both paragraphs read very sensibly.

>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  builtin/replace.c |  5 +++++
>  commit.c          | 34 ++++++++++++++++++++++++++++++++++
>  commit.h          |  2 ++
>  3 files changed, 41 insertions(+)
>
> diff --git a/builtin/replace.c b/builtin/replace.c
> index ad47237..000db65 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -344,6 +344,11 @@ static int create_graft(int argc, const char **argv, int force)
>  
>  	replace_parents(&buf, argc, argv);
>  
> +	if (remove_signature(&buf))
> +		warning(_("the original commit '%s' has a gpg signature.\n"
> +			  "It will be removed in the replacement commit!"),

Hmmm...  does the second line of this message start with the usual
"warning:" prefix?

> diff --git a/commit.c b/commit.c
> index fb7897c..54e157d 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1177,6 +1177,40 @@ int parse_signed_commit(const struct commit *commit,
>  	return saw_signature;
>  }
>  
> +int remove_signature(struct strbuf *buf)
> +{
> +	const char *line = buf->buf;
> +	const char *tail = buf->buf + buf->len;
> +	int in_signature = 0;
> +	const char *sig_start = NULL;
> +	const char *sig_end = NULL;
> +
> +	while (line < tail) {
> +		const char *next = memchr(line, '\n', tail - line);
> +		next = next ? next + 1 : tail;

This almost makes me wonder if we want something similar to
strchrnul() we use for NUL-terminated strings, and I suspect that
you would find more instances by running "git grep -A2 memchr".

I don't know what such a helper function should be named, though.
Certainly not "memchrnul()".

> +
> +		if (in_signature && line[0] == ' ')
> +			sig_end = next;
> +		else if (starts_with(line, gpg_sig_header) &&
> +			 line[gpg_sig_header_len] == ' ') {
> +			sig_start = line;
> +			sig_end = next;
> +			in_signature = 1;
> +		} else {
> +			if (*line == '\n')
> +				/* dump the whole remainder of the buffer */
> +				next = tail;
> +			in_signature = 0;
> +		}
> +		line = next;
> +	}
> +
> +	if (sig_start)
> +		strbuf_remove(buf, sig_start - buf->buf, sig_end - sig_start);

If there are two instances of gpg_sig, this will remove only the
last one, but there is no chance both signatures of such a commit
can validate OK, and we won't be losing something in between anyway,
so it should be fine.

> +	return sig_start != NULL;
> +}
> +
>  static void handle_signed_tag(struct commit *parent, struct commit_extra_header ***tail)
>  {
>  	struct merge_remote_desc *desc;
> diff --git a/commit.h b/commit.h
> index 2e1492a..4234dae 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -327,6 +327,8 @@ struct commit *get_merge_parent(const char *name);
>  
>  extern int parse_signed_commit(const struct commit *commit,
>  			       struct strbuf *message, struct strbuf *signature);
> +extern int remove_signature(struct strbuf *buf);
> +
>  extern void print_commit_list(struct commit_list *list,
>  			      const char *format_cur,
>  			      const char *format_last);

Re: [PATCH v5 7/7] replace: add test for --graft with signed commit

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  t/t6050-replace.sh | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
> index ca45a84..80b85e3 100755
> --- a/t/t6050-replace.sh
> +++ b/t/t6050-replace.sh
> @@ -7,6 +7,7 @@ test_description='Tests replace refs functionality'
>  exec </dev/null
>  
>  . ./test-lib.sh
> +. "$TEST_DIRECTORY/lib-gpg.sh"
>  
>  add_and_commit_file()
>  {
> @@ -363,4 +364,25 @@ test_expect_success '--graft with and without already replaced object' '
>  	git replace -d $HASH5
>  '
>  
> +test_expect_success GPG 'set up a signed commit' '
> +	echo "line 17" >> hello &&
> +	echo "line 18" >> hello &&

Style?

> +	git add hello &&
> +	test_tick &&
> +	git commit --quiet -S -m "hello: 2 more lines in a signed commit" &&
> +	HASH8=$(git rev-parse --verify HEAD) &&
> +	git verify-commit $HASH8
> +'
> +
> +test_expect_success GPG '--graft with a signed commit' '
> +	git cat-file commit $HASH8 >orig &&
> +	git replace --graft $HASH8 &&
> +	git cat-file commit $HASH8 >repl &&
> +	test_must_fail grep gpgsig repl &&
> +	diff -u orig repl | grep "^-parent $HASH7" &&
> +	diff -u orig repl | grep "^-gpgsig -----BEGIN PGP SIGNATURE-----" &&

Almost the same comment as the one for 2/7 applies here.

> +	test_must_fail git verify-commit $HASH8 &&
> +	git replace -d $HASH8
> +'
> +
>  test_done

Re: [PATCH v5 2/7] replace: add test for --graft

[Christian Couder]

On Wed, Jul 2, 2014 at 10:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>  t/t6050-replace.sh | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
>> index 68b3cb2..ca45a84 100755
>> --- a/t/t6050-replace.sh
>> +++ b/t/t6050-replace.sh
>> @@ -351,4 +351,16 @@ test_expect_success 'replace ref cleanup' '
>>       test -z "$(git replace)"
>>  '
>>
>> +test_expect_success '--graft with and without already replaced object' '
>> +     test $(git log --oneline | wc -l) = 7 &&
>> +     git replace --graft $HASH5 &&
>> +     test $(git log --oneline | wc -l) = 3 &&
>> +     git cat-file -p $HASH5 | test_must_fail grep parent &&
>
> Please do not run non-git command under test_must_fail.

Ok, I think I will just use the following then:

test_must_fail git rev-parse $HASH5^1

>> +     test_must_fail git replace --graft $HASH5 $HASH4 $HASH3 &&
>> +     git replace --force -g $HASH5 $HASH4 $HASH3 &&
>> +     git cat-file -p $HASH5 | grep "parent $HASH4" &&
>> +     git cat-file -p $HASH5 | grep "parent $HASH3" &&
>> +     git replace -d $HASH5
>> +'
>> +
>>  test_done
>
> For all these "git cat-file -p $commit | grep ...", I would think
> you should add "commit_has_parents" helper function to allow a bit
> more careful testing.  As written, the test will mistake a commit
> with string "parent $HASHx" anywhere, not in the header part, and
> the test does not ensure that $HASH{3,4} is the {first,second}
> parent.
>
>         commit_has_parents $HASH5 $HASH4 $HASH3
>
> would then may be implemented like:
>
>         commit_has_parents () {
>                 git cat-file commit "$1" >payload &&
>                 sed -n -e '/^$/q' -e 's/^parent //p' <payload >actual &&
>                 shift &&
>                 printf 'parent %s\n' "$@" >expect &&
>                 test_cmp expect actual
>         }

I think I'll rather use something like:

test $(git rev-parse $HASH5^1) = "$HASH4" &&
test $(git rev-parse $HASH5^2) = "$HASH3" &&
...

Thanks,
Christian.

Re: [PATCH v5 5/7] replace: refactor replacing parents

[Christian Couder]

On Wed, Jul 2, 2014 at 11:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
>>  builtin/replace.c | 42 +++++++++++++++++++++++++-----------------
>>  1 file changed, 25 insertions(+), 17 deletions(-)
>>
>> diff --git a/builtin/replace.c b/builtin/replace.c
>> index 3515979..ad47237 100644
>> --- a/builtin/replace.c
>> +++ b/builtin/replace.c
>> @@ -295,27 +295,14 @@ static int edit_and_replace(const char *object_ref, int force)
>>       return replace_object_sha1(object_ref, old, "replacement", new, force);
>>  }
>>
>> -static int create_graft(int argc, const char **argv, int force)
>> +static void replace_parents(struct strbuf *buf, int argc, const char **argv)
>
> It is somewhat strange to see that a new function introduced earlier
> in the series is rewritten with a "refactoring".  Shouldn't the new
> function have been done right from the beginning instead?

Yeah, I will do it right from the beginning in the next patch series.

Re: [PATCH v5 6/7] replace: remove signature when using --graft

[Christian Couder]

On Wed, Jul 2, 2014 at 11:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
>
>> It could be misleading to keep a signature in a
>> replacement commit, so let's remove it.
>>
>> Note that there should probably be a way to sign
>> the replacement commit created when using --graft,
>> but this can be dealt with in another commit or
>> patch series.
>
> Both paragraphs read very sensibly.

Thanks.

>> --- a/builtin/replace.c
>> +++ b/builtin/replace.c
>> @@ -344,6 +344,11 @@ static int create_graft(int argc, const char **argv, int force)
>>
>>       replace_parents(&buf, argc, argv);
>>
>> +     if (remove_signature(&buf))
>> +             warning(_("the original commit '%s' has a gpg signature.\n"
>> +                       "It will be removed in the replacement commit!"),
>
> Hmmm...  does the second line of this message start with the usual
> "warning:" prefix?

Ok, I will use following:

if (remove_signature(&buf)) {
        warning(_("the original commit '%s' has a gpg signature."), old_ref);
        warning(_("the signature will be removed in the replacement commit!"));
}

>> diff --git a/commit.c b/commit.c
>> index fb7897c..54e157d 100644
>> --- a/commit.c
>> +++ b/commit.c
>> @@ -1177,6 +1177,40 @@ int parse_signed_commit(const struct commit *commit,
>>       return saw_signature;
>>  }
>>
>> +int remove_signature(struct strbuf *buf)
>> +{
>> +     const char *line = buf->buf;
>> +     const char *tail = buf->buf + buf->len;
>> +     int in_signature = 0;
>> +     const char *sig_start = NULL;
>> +     const char *sig_end = NULL;
>> +
>> +     while (line < tail) {
>> +             const char *next = memchr(line, '\n', tail - line);
>> +             next = next ? next + 1 : tail;
>
> This almost makes me wonder if we want something similar to
> strchrnul() we use for NUL-terminated strings, and I suspect that
> you would find more instances by running "git grep -A2 memchr".
>
> I don't know what such a helper function should be named, though.
> Certainly not "memchrnul()".

I can add this to a GSoC microproject page for next year.

>> +             if (in_signature && line[0] == ' ')
>> +                     sig_end = next;
>> +             else if (starts_with(line, gpg_sig_header) &&
>> +                      line[gpg_sig_header_len] == ' ') {
>> +                     sig_start = line;
>> +                     sig_end = next;
>> +                     in_signature = 1;
>> +             } else {
>> +                     if (*line == '\n')
>> +                             /* dump the whole remainder of the buffer */
>> +                             next = tail;
>> +                     in_signature = 0;
>> +             }
>> +             line = next;
>> +     }
>> +
>> +     if (sig_start)
>> +             strbuf_remove(buf, sig_start - buf->buf, sig_end - sig_start);
>
> If there are two instances of gpg_sig, this will remove only the
> last one, but there is no chance both signatures of such a commit
> can validate OK, and we won't be losing something in between anyway,
> so it should be fine.

Ok.

Thanks,
Christian.

Re: [PATCH v5 7/7] replace: add test for --graft with signed commit

[Christian Couder]

On Wed, Jul 2, 2014 at 11:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
>
>>
>> +test_expect_success GPG 'set up a signed commit' '
>> +     echo "line 17" >> hello &&
>> +     echo "line 18" >> hello &&
>
> Style?

Yeah, I will change it to:

     echo "line 17" >>hello &&
     echo "line 18" >>hello &&

>> +     git add hello &&
>> +     test_tick &&
>> +     git commit --quiet -S -m "hello: 2 more lines in a signed commit" &&
>> +     HASH8=$(git rev-parse --verify HEAD) &&
>> +     git verify-commit $HASH8
>> +'
>> +
>> +test_expect_success GPG '--graft with a signed commit' '
>> +     git cat-file commit $HASH8 >orig &&
>> +     git replace --graft $HASH8 &&
>> +     git cat-file commit $HASH8 >repl &&
>> +     test_must_fail grep gpgsig repl &&
>> +     diff -u orig repl | grep "^-parent $HASH7" &&
>> +     diff -u orig repl | grep "^-gpgsig -----BEGIN PGP SIGNATURE-----" &&
>
> Almost the same comment as the one for 2/7 applies here.

Ok, I will find a way to make it better.

Thanks,
Christian.

Re: [PATCH v5 2/7] replace: add test for --graft

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> On Wed, Jul 2, 2014 at 10:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Christian Couder <chriscool@tuxfamily.org> writes:
>>
>>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>> ---
>>>  t/t6050-replace.sh | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
>>> index 68b3cb2..ca45a84 100755
>>> --- a/t/t6050-replace.sh
>>> +++ b/t/t6050-replace.sh
>>> @@ -351,4 +351,16 @@ test_expect_success 'replace ref cleanup' '
>>>       test -z "$(git replace)"
>>>  '
>>>
>>> +test_expect_success '--graft with and without already replaced object' '
>>> +     test $(git log --oneline | wc -l) = 7 &&
>>> +     git replace --graft $HASH5 &&
>>> +     test $(git log --oneline | wc -l) = 3 &&
>>> +     git cat-file -p $HASH5 | test_must_fail grep parent &&
>>
>> Please do not run non-git command under test_must_fail.
>
> Ok, I think I will just use the following then:
>
> test_must_fail git rev-parse $HASH5^1
> ...

See below.

>>> +     test_must_fail git replace --graft $HASH5 $HASH4 $HASH3 &&
>>> +     git replace --force -g $HASH5 $HASH4 $HASH3 &&
>>> +     git cat-file -p $HASH5 | grep "parent $HASH4" &&
>>> +     git cat-file -p $HASH5 | grep "parent $HASH3" &&
>>> +     git replace -d $HASH5
>>> +'
>>> +
>>>  test_done
>>
>> For all these "git cat-file -p $commit | grep ...", I would think
>> you should add "commit_has_parents" helper function to allow a bit
>> more careful testing.  As written, the test will mistake a commit
>> with string "parent $HASHx" anywhere, not in the header part, and
>> the test does not ensure that $HASH{3,4} is the {first,second}
>> parent.
>>
>>         commit_has_parents $HASH5 $HASH4 $HASH3
>>
>> would then may be implemented like:
>>
>>         commit_has_parents () {
>>                 git cat-file commit "$1" >payload &&
>>                 sed -n -e '/^$/q' -e 's/^parent //p' <payload >actual &&
>>                 shift &&
>>                 printf 'parent %s\n' "$@" >expect &&
>>                 test_cmp expect actual
>>         }
>
> I think I'll rather use something like:
>
> test $(git rev-parse $HASH5^1) = "$HASH4" &&
> test $(git rev-parse $HASH5^2) = "$HASH3" &&
> ...

Even in that case, I'd suggest using the same "commit_has_parents"
abstraction, which you will also be using to check the "replaced to
be a new root" case in the earlier part of this test.

In case you do not get what I mean by "in that case", you are saying
that you will now be testing a different thing.  You can test what
your new code produces at the bit level by directly obtaining the
resulting object via "cat-file" and that lets you not to depend on
the rest of the system (i.e. the part that allows you to pretend an
existing object you have a corresponding replace ref for has contents
of a totally different object) working correctly.  Or you can choose
to test the system as a whole (i.e. not just the "git replace" produced
a new object with contents you planned to fabricate, but also by
having a replace ref, you can let the rest of the system use th
contents of that object when asked for the replaced object).

The implementation suggested in my previous message was in line with
the former, because your use of "cat-file" seemed to indicate that
you wanted to avoid depending on the rest of the system to test this
new feature in your new tests.  You seem to be saying that you now
want to take the other approach of testing both at the same time.

I am fine with either approach, but I want to make sure that you are
aware of the distinction.  The last thing I want to see is to flip
the approach you take to test not because "testing as a whole is
better than testing one thing without getting interfered by
potential breakage in other parts" but because "testing as a whole
is easier."

The implementation of commit_has_parents that tests the system as a
whole may look like

        commit=$1 parent_number=1
        shift
        for parent
        do
                test $(git rev-parse --verify "$commit^$parent_number") = "$parent" ||
		return 1
		parent_number=$(( $parent_number + 1 ))
	done
        test_must_fail git rev-parse $commit^$parent_number

and you would still use it like this:

  commit_has_parents $HASH5 ;# must have no parent
  commit_has_parents $HASH5 $HASH4 $HASH3 ;# must have these parents

Thanks.