[PATCH v6 00/10] Add --graft option to git replace

[PATCH v6 00/10] Add --graft option to git replace

[Christian Couder]

Here is a small 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 v5, thanks to Junio, are:

- new patch 1/10 to clean up redirection style in t6050

- new patches 8/10, 9/10 and 10/10 to check mergetags

- add functions to test parents in patch 3/10 and 7/10

- improve testing signed commits in patch 7/10

- improve warning when removing commit signature in
  patch 6/10

Christian Couder (10):
  replace: cleanup redirection style in tests
  replace: add --graft option
  replace: add test for --graft
  Documentation: replace: add --graft option
  contrib: add convert-grafts-to-replace-refs.sh
  replace: remove signature when using --graft
  replace: add test for --graft with signed commit
  commit: add for_each_mergetag()
  replace: check mergetags when using --graft
  replace: add test for --graft with a mergetag

 Documentation/git-replace.txt             |  10 +++
 builtin/replace.c                         | 126 +++++++++++++++++++++++++++-
 commit.c                                  |  47 +++++++++++
 commit.h                                  |   7 ++
 contrib/convert-grafts-to-replace-refs.sh |  28 +++++++
 log-tree.c                                |  15 +---
 t/t6050-replace.sh                        | 135 ++++++++++++++++++++++++------
 7 files changed, 332 insertions(+), 36 deletions(-)
 create mode 100755 contrib/convert-grafts-to-replace-refs.sh

-- 
2.0.0.421.g786a89d.dirty

[PATCH v6 01/10] replace: cleanup redirection style in tests

[Christian Couder]

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

diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index 68b3cb2..fb07ad2 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -27,36 +27,36 @@ HASH6=
 HASH7=
 
 test_expect_success 'set up buggy branch' '
-     echo "line 1" >> hello &&
-     echo "line 2" >> hello &&
-     echo "line 3" >> hello &&
-     echo "line 4" >> hello &&
+     echo "line 1" >>hello &&
+     echo "line 2" >>hello &&
+     echo "line 3" >>hello &&
+     echo "line 4" >>hello &&
      add_and_commit_file hello "4 lines" &&
      HASH1=$(git rev-parse --verify HEAD) &&
-     echo "line BUG" >> hello &&
-     echo "line 6" >> hello &&
-     echo "line 7" >> hello &&
-     echo "line 8" >> hello &&
+     echo "line BUG" >>hello &&
+     echo "line 6" >>hello &&
+     echo "line 7" >>hello &&
+     echo "line 8" >>hello &&
      add_and_commit_file hello "4 more lines with a BUG" &&
      HASH2=$(git rev-parse --verify HEAD) &&
-     echo "line 9" >> hello &&
-     echo "line 10" >> hello &&
+     echo "line 9" >>hello &&
+     echo "line 10" >>hello &&
      add_and_commit_file hello "2 more lines" &&
      HASH3=$(git rev-parse --verify HEAD) &&
-     echo "line 11" >> hello &&
+     echo "line 11" >>hello &&
      add_and_commit_file hello "1 more line" &&
      HASH4=$(git rev-parse --verify HEAD) &&
-     sed -e "s/BUG/5/" hello > hello.new &&
+     sed -e "s/BUG/5/" hello >hello.new &&
      mv hello.new hello &&
      add_and_commit_file hello "BUG fixed" &&
      HASH5=$(git rev-parse --verify HEAD) &&
-     echo "line 12" >> hello &&
-     echo "line 13" >> hello &&
+     echo "line 12" >>hello &&
+     echo "line 13" >>hello &&
      add_and_commit_file hello "2 more lines" &&
      HASH6=$(git rev-parse --verify HEAD) &&
-     echo "line 14" >> hello &&
-     echo "line 15" >> hello &&
-     echo "line 16" >> hello &&
+     echo "line 14" >>hello &&
+     echo "line 15" >>hello &&
+     echo "line 16" >>hello &&
      add_and_commit_file hello "again 3 more lines" &&
      HASH7=$(git rev-parse --verify HEAD)
 '
@@ -95,7 +95,7 @@ test_expect_success 'tag replaced commit' '
 '
 
 test_expect_success '"git fsck" works' '
-     git fsck master > fsck_master.out &&
+     git fsck master >fsck_master.out &&
      grep "dangling commit $R" fsck_master.out &&
      grep "dangling tag $(cat .git/refs/tags/mytag)" fsck_master.out &&
      test -z "$(git fsck)"
@@ -217,14 +217,14 @@ test_expect_success 'fetch branch with replacement' '
      (
 	  cd clone_dir &&
 	  git fetch origin refs/heads/tofetch:refs/heads/parallel3 &&
-	  git log --pretty=oneline parallel3 > output.txt &&
+	  git log --pretty=oneline parallel3 >output.txt &&
 	  ! grep $PARA3 output.txt &&
-	  git show $PARA3 > para3.txt &&
+	  git show $PARA3 >para3.txt &&
 	  grep "A U Thor" para3.txt &&
 	  git fetch origin "refs/replace/*:refs/replace/*" &&
-	  git log --pretty=oneline parallel3 > output.txt &&
+	  git log --pretty=oneline parallel3 >output.txt &&
 	  grep $PARA3 output.txt &&
-	  git show $PARA3 > para3.txt &&
+	  git show $PARA3 >para3.txt &&
 	  grep "O Thor" para3.txt
      )
 '
@@ -302,7 +302,7 @@ test_expect_success 'test --format medium' '
 		echo "$PARA3 -> $S" &&
 		echo "$MYTAG -> $HASH1"
 	} | sort >expected &&
-	git replace -l --format medium | sort > actual &&
+	git replace -l --format medium | sort >actual &&
 	test_cmp expected actual
 '
 
@@ -314,7 +314,7 @@ test_expect_success 'test --format long' '
 		echo "$PARA3 (commit) -> $S (commit)" &&
 		echo "$MYTAG (tag) -> $HASH1 (commit)"
 	} | sort >expected &&
-	git replace --format=long | sort > actual &&
+	git replace --format=long | sort >actual &&
 	test_cmp expected actual
 '
 
-- 
2.0.0.421.g786a89d.dirty

[PATCH v6 02/10] 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 | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 73 insertions(+), 1 deletion(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 1bb491d..ad47237 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,66 @@ static int edit_and_replace(const char *object_ref, int force)
 	return replace_object_sha1(object_ref, old, "replacement", new, force);
 }
 
+static void replace_parents(struct strbuf *buf, int argc, const char **argv)
+{
+	struct strbuf new_parents = STRBUF_INIT;
+	const char *parent_start, *parent_end;
+	int i;
+
+	/* find existing parents */
+	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);
+
+	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(&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 +364,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 +388,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 +414,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 v6 03/10] 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 | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index fb07ad2..d80a89e 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -18,6 +18,33 @@ add_and_commit_file()
     git commit --quiet -m "$_file: $_msg"
 }
 
+commit_buffer_contains_parents()
+{
+    git cat-file commit "$1" >payload &&
+    sed -n -e '/^$/q' -e '/^parent /p' <payload >actual &&
+    shift &&
+    : >expected &&
+    for _parent
+    do
+	echo "parent $_parent" >>expected || return 1
+    done &&
+    test_cmp actual expected
+}
+
+commit_has_parents()
+{
+    _parent_number=1
+    _commit="$1"
+    shift &&
+    for _parent
+    do
+	_found=$(git rev-parse --verify $_commit^$_parent_number) || return 1
+	test "$_found" = "$_parent" || return 1
+	_parent_number=$(( $_parent_number + 1 ))
+    done &&
+    test_must_fail git rev-parse --verify $_commit^$_parent_number
+}
+
 HASH1=
 HASH2=
 HASH3=
@@ -351,4 +378,17 @@ 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 &&
+	commit_buffer_contains_parents $HASH5 &&
+	commit_has_parents $HASH5 &&
+	test_must_fail git replace --graft $HASH5 $HASH4 $HASH3 &&
+	git replace --force -g $HASH5 $HASH4 $HASH3 &&
+	commit_buffer_contains_parents $HASH5 $HASH4 $HASH3 &&
+	commit_has_parents $HASH5 $HASH4 $HASH3 &&
+	git replace -d $HASH5
+'
+
 test_done
-- 
2.0.0.421.g786a89d.dirty

[PATCH v6 04/10] 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 v6 05/10] 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 v6 06/10] 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..cc29ef2 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."), old_ref);
+		warning(_("the signature will be removed in the replacement commit!"));
+	}
+
 	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 v6 07/10] replace: add test for --graft with signed commit

[Christian Couder]

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

diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index d80a89e..15fd541 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()
 {
@@ -391,4 +392,28 @@ 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 &&
+	commit_buffer_contains_parents $HASH8 &&
+	commit_has_parents $HASH8 &&
+	test_must_fail git verify-commit $HASH8 &&
+	sed -n -e "/^tree /p" -e "/^author /p" -e "/^committer /p" orig >expected &&
+	echo >>expected &&
+	sed -e "/^$/q" repl >actual &&
+	test_cmp expected actual &&
+	git replace -d $HASH8
+'
+
 test_done
-- 
2.0.0.421.g786a89d.dirty

[PATCH v6 08/10] commit: add for_each_mergetag()

[Christian Couder]

In the same way as there is for_each_ref() to
iterate on refs, it might be useful to have
for_each_mergetag() to iterate on the mergetags
of a given commit.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 commit.c   | 13 +++++++++++++
 commit.h   |  5 +++++
 log-tree.c | 15 ++++-----------
 3 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/commit.c b/commit.c
index 54e157d..0f83ff7 100644
--- a/commit.c
+++ b/commit.c
@@ -1348,6 +1348,19 @@ struct commit_extra_header *read_commit_extra_headers(struct commit *commit,
 	return extra;
 }
 
+void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data)
+{
+	struct commit_extra_header *extra, *to_free;
+
+	to_free = read_commit_extra_headers(commit, NULL);
+	for (extra = to_free; extra; extra = extra->next) {
+		if (strcmp(extra->key, "mergetag"))
+			continue; /* not a merge tag */
+		fn(commit, extra, data);
+	}
+	free_commit_extra_headers(to_free);
+}
+
 static inline int standard_header_field(const char *field, size_t len)
 {
 	return ((len == 4 && !memcmp(field, "tree ", 5)) ||
diff --git a/commit.h b/commit.h
index 4234dae..c81ba85 100644
--- a/commit.h
+++ b/commit.h
@@ -312,6 +312,11 @@ extern struct commit_extra_header *read_commit_extra_headers(struct commit *, co
 
 extern void free_commit_extra_headers(struct commit_extra_header *extra);
 
+typedef void (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra,
+				 void *cb_data);
+
+extern void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data);
+
 struct merge_remote_desc {
 	struct object *obj; /* the named object, could be a tag */
 	const char *name;
diff --git a/log-tree.c b/log-tree.c
index 10e6844..706ed4c 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -413,10 +413,11 @@ static int is_common_merge(const struct commit *commit)
 		&& !commit->parents->next->next);
 }
 
-static void show_one_mergetag(struct rev_info *opt,
+static void show_one_mergetag(struct commit *commit,
 			      struct commit_extra_header *extra,
-			      struct commit *commit)
+			      void *data)
 {
+	struct rev_info *opt = (struct rev_info *)data;
 	unsigned char sha1[20];
 	struct tag *tag;
 	struct strbuf verify_message;
@@ -463,15 +464,7 @@ static void show_one_mergetag(struct rev_info *opt,
 
 static void show_mergetag(struct rev_info *opt, struct commit *commit)
 {
-	struct commit_extra_header *extra, *to_free;
-
-	to_free = read_commit_extra_headers(commit, NULL);
-	for (extra = to_free; extra; extra = extra->next) {
-		if (strcmp(extra->key, "mergetag"))
-			continue; /* not a merge tag */
-		show_one_mergetag(opt, extra, commit);
-	}
-	free_commit_extra_headers(to_free);
+	for_each_mergetag(show_one_mergetag, commit, opt);
 }
 
 void show_log(struct rev_info *opt)
-- 
2.0.0.421.g786a89d.dirty

[PATCH v6 09/10] replace: check mergetags when using --graft

[Christian Couder]

When using --graft, with a mergetag in the original
commit, we should check that the commit pointed to by
the mergetag is still a parent of then new commit we
create, otherwise the mergetag could be misleading.

If the commit pointed to by the mergetag is no more
a parent of the new commit, we could remove the
mergetag, but in this case there is a good chance
that the title or other elements of the commit might
also be misleading. So let's just error out and
suggest to use --edit instead on the commit.

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

diff --git a/builtin/replace.c b/builtin/replace.c
index cc29ef2..2290529 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -13,6 +13,7 @@
 #include "refs.h"
 #include "parse-options.h"
 #include "run-command.h"
+#include "tag.h"
 
 static const char * const git_replace_usage[] = {
 	N_("git replace [-f] <object> <replacement>"),
@@ -325,6 +326,50 @@ static void replace_parents(struct strbuf *buf, int argc, const char **argv)
 	strbuf_release(&new_parents);
 }
 
+struct check_mergetag_data {
+	int argc;
+	const char **argv;
+};
+
+static void check_one_mergetag(struct commit *commit,
+			       struct commit_extra_header *extra,
+			       void *data)
+{
+	struct check_mergetag_data *mergetag_data = (struct check_mergetag_data *)data;
+	const char *ref = mergetag_data->argv[0];
+	unsigned char tag_sha1[20];
+	struct tag *tag;
+	int i;
+
+	hash_sha1_file(extra->value, extra->len, typename(OBJ_TAG), tag_sha1);
+	tag = lookup_tag(tag_sha1);
+	if (!tag)
+		die(_("bad mergetag in commit '%s'"), ref);
+	if (parse_tag_buffer(tag, extra->value, extra->len))
+		die(_("malformed mergetag in commit '%s'"), ref);
+
+	/* iterate over new parents */
+	for (i = 1; i < mergetag_data->argc; i++) {
+		unsigned char sha1[20];
+		if (get_sha1(mergetag_data->argv[i], sha1) < 0)
+			die(_("Not a valid object name: '%s'"), mergetag_data->argv[i]);
+		if (!hashcmp(tag->tagged->sha1, sha1))
+			return; /* found */
+	}
+
+	die(_("original commit '%s' contains mergetag '%s' that is discarded; "
+	      "use --edit instead of --graft"), ref, sha1_to_hex(tag_sha1));
+}
+
+static void check_mergetags(struct commit *commit, int argc, const char **argv)
+{
+	struct check_mergetag_data mergetag_data;
+
+	mergetag_data.argc = argc;
+	mergetag_data.argv = argv;
+	for_each_mergetag(check_one_mergetag, commit, &mergetag_data);
+}
+
 static int create_graft(int argc, const char **argv, int force)
 {
 	unsigned char old[20], new[20];
@@ -349,6 +394,8 @@ static int create_graft(int argc, const char **argv, int force)
 		warning(_("the signature will be removed in the replacement commit!"));
 	}
 
+	check_mergetags(commit, argc, argv);
+
 	if (write_sha1_file(buf.buf, buf.len, commit_type, new))
 		die(_("could not write replacement commit for: '%s'"), old_ref);
 
-- 
2.0.0.421.g786a89d.dirty

[PATCH v6 10/10] replace: add test for --graft with a mergetag

[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 15fd541..3bb8d06 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -416,4 +416,26 @@ test_expect_success GPG '--graft with a signed commit' '
 	git replace -d $HASH8
 '
 
+test_expect_success GPG 'set up a merge commit with a mergetag' '
+	git reset --hard HEAD &&
+	git checkout -b test_branch HEAD~2 &&
+	echo "line 1 from test branch" >>hello &&
+	echo "line 2 from test branch" >>hello &&
+	git add hello &&
+	test_tick &&
+	git commit -m "hello: 2 more lines from a test branch" &&
+	HASH9=$(git rev-parse --verify HEAD) &&
+	git tag -s -m "tag for testing with a mergetag" test_tag HEAD &&
+	git checkout master &&
+	git merge -s ours test_tag &&
+	HASH10=$(git rev-parse --verify HEAD) &&
+	git cat-file commit $HASH10 | grep "^mergetag object"
+'
+
+test_expect_success GPG '--graft on a commit with a mergetag' '
+	test_must_fail git replace --graft $HASH10 $HASH8^1 &&
+	git replace --graft $HASH10 $HASH8^1 $HASH9 &&
+	git replace -d $HASH10
+'
+
 test_done
-- 
2.0.0.421.g786a89d.dirty

Re: [PATCH v6 03/10] 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 | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
> index fb07ad2..d80a89e 100755
> --- a/t/t6050-replace.sh
> +++ b/t/t6050-replace.sh
> @@ -18,6 +18,33 @@ add_and_commit_file()
>      git commit --quiet -m "$_file: $_msg"
>  }
>  
> +commit_buffer_contains_parents()

SP before (), perhaps?

> +{
> +    git cat-file commit "$1" >payload &&
> +    sed -n -e '/^$/q' -e '/^parent /p' <payload >actual &&
> +    shift &&
> +    : >expected &&
> +    for _parent
> +    do
> +	echo "parent $_parent" >>expected || return 1
> +    done &&

You can redirect the entire loop to simplify the above 5 lines,
which would read better, no?

	for _parent
        do
		echo "parent $_parent"
	done >expect

> +    test_cmp actual expected

As "test_cmp" normally runs "diff", it is better to compare expect
with actual, not the other way around; running the test verbosely,
i.e. "t6050-replace.sh -v", shows how the actual output differs from
what is expected when diagnosing breakage and would be more useful
that way.

If your goal is to test both the object contents with this code
*and* the overall system behaviour with the "$child^$parent" below,
perhaps they should be in the same "commit_has_parents" function,
without forcing the caller to choose between the two or call both,
no?

> +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 &&
> +	commit_buffer_contains_parents $HASH5 &&
> +	commit_has_parents $HASH5 &&
> +	test_must_fail git replace --graft $HASH5 $HASH4 $HASH3 &&
> +	git replace --force -g $HASH5 $HASH4 $HASH3 &&
> +	commit_buffer_contains_parents $HASH5 $HASH4 $HASH3 &&
> +	commit_has_parents $HASH5 $HASH4 $HASH3 &&
> +	git replace -d $HASH5
> +'
> +
>  test_done

Re: [PATCH v6 09/10] replace: check mergetags when using --graft

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> When using --graft, with a mergetag in the original
> commit, we should check that the commit pointed to by
> the mergetag is still a parent of then new commit we
> create, otherwise the mergetag could be misleading.
>
> If the commit pointed to by the mergetag is no more
> a parent of the new commit, we could remove the
> mergetag, but in this case there is a good chance
> that the title or other elements of the commit might
> also be misleading. So let's just error out and
> suggest to use --edit instead on the commit.

I do not quite understand the reasoning.  If you have a merge you
earlier made with a signed tag, and then want to redo the merge with
an updated tip of that branch (perhaps the side branch was earlier
based on maint but now was rebased on master), it will perfectly be
normal to expect that the title or other elements of the resulting
merge to stay the same.  Why is it a good idea to error it out?

If the argument were '"replace --graft" that changes the parents is
likely to affect merge summary message, so error out and suggest to
use --edit instead', regardless of the 'mergetag', I'd understand
it, but that would essentially mean that 'replace --graft' should
never be used, so...

Re: [PATCH v6 08/10] commit: add for_each_mergetag()

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> In the same way as there is for_each_ref() to
> iterate on refs, it might be useful to have
> for_each_mergetag() to iterate on the mergetags
> of a given commit.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---

Heh, "might be useful" is an understatement ;-) We won't apply a
change that "might be useful" very lightly, but this refactoring
already uses the helper in existing code, showing that it *is*
useful, no?

Let's have this early in the series, or perhaps even independent of
the "replace" series.

Thanks.

>  commit.c   | 13 +++++++++++++
>  commit.h   |  5 +++++
>  log-tree.c | 15 ++++-----------
>  3 files changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/commit.c b/commit.c
> index 54e157d..0f83ff7 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1348,6 +1348,19 @@ struct commit_extra_header *read_commit_extra_headers(struct commit *commit,
>  	return extra;
>  }
>  
> +void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data)
> +{
> +	struct commit_extra_header *extra, *to_free;
> +
> +	to_free = read_commit_extra_headers(commit, NULL);
> +	for (extra = to_free; extra; extra = extra->next) {
> +		if (strcmp(extra->key, "mergetag"))
> +			continue; /* not a merge tag */
> +		fn(commit, extra, data);
> +	}
> +	free_commit_extra_headers(to_free);
> +}
> +
>  static inline int standard_header_field(const char *field, size_t len)
>  {
>  	return ((len == 4 && !memcmp(field, "tree ", 5)) ||
> diff --git a/commit.h b/commit.h
> index 4234dae..c81ba85 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -312,6 +312,11 @@ extern struct commit_extra_header *read_commit_extra_headers(struct commit *, co
>  
>  extern void free_commit_extra_headers(struct commit_extra_header *extra);
>  
> +typedef void (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra,
> +				 void *cb_data);
> +
> +extern void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data);
> +
>  struct merge_remote_desc {
>  	struct object *obj; /* the named object, could be a tag */
>  	const char *name;
> diff --git a/log-tree.c b/log-tree.c
> index 10e6844..706ed4c 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -413,10 +413,11 @@ static int is_common_merge(const struct commit *commit)
>  		&& !commit->parents->next->next);
>  }
>  
> -static void show_one_mergetag(struct rev_info *opt,
> +static void show_one_mergetag(struct commit *commit,
>  			      struct commit_extra_header *extra,
> -			      struct commit *commit)
> +			      void *data)
>  {
> +	struct rev_info *opt = (struct rev_info *)data;
>  	unsigned char sha1[20];
>  	struct tag *tag;
>  	struct strbuf verify_message;
> @@ -463,15 +464,7 @@ static void show_one_mergetag(struct rev_info *opt,
>  
>  static void show_mergetag(struct rev_info *opt, struct commit *commit)
>  {
> -	struct commit_extra_header *extra, *to_free;
> -
> -	to_free = read_commit_extra_headers(commit, NULL);
> -	for (extra = to_free; extra; extra = extra->next) {
> -		if (strcmp(extra->key, "mergetag"))
> -			continue; /* not a merge tag */
> -		show_one_mergetag(opt, extra, commit);
> -	}
> -	free_commit_extra_headers(to_free);
> +	for_each_mergetag(show_one_mergetag, commit, opt);
>  }
>  
>  void show_log(struct rev_info *opt)

Re: [PATCH v6 09/10] replace: check mergetags when using --graft

[Christian Couder]

From: Junio C Hamano <gitster@pobox.com>
>
> Christian Couder <chriscool@tuxfamily.org> writes:
> 
>> When using --graft, with a mergetag in the original
>> commit, we should check that the commit pointed to by
>> the mergetag is still a parent of then new commit we
>> create, otherwise the mergetag could be misleading.
>>
>> If the commit pointed to by the mergetag is no more
>> a parent of the new commit, we could remove the
>> mergetag, but in this case there is a good chance
>> that the title or other elements of the commit might
>> also be misleading. So let's just error out and
>> suggest to use --edit instead on the commit.
> 
> I do not quite understand the reasoning.  If you have a merge you
> earlier made with a signed tag, and then want to redo the merge with
> an updated tip of that branch (perhaps the side branch was earlier
> based on maint but now was rebased on master), it will perfectly be
> normal to expect that the title or other elements of the resulting
> merge to stay the same.  

Yeah, but then you might also want to have a mergetag for the updated
tip of the branch and --graft will not put it in the new commit, so it
would be better to use --edit in this case.

> Why is it a good idea to error it out?

Because sometimes, in complex cases, it is misleading to do as if you
can do the right thing, when there is a good chance you cannot.

> If the argument were '"replace --graft" that changes the parents is
> likely to affect merge summary message, so error out and suggest to
> use --edit instead', regardless of the 'mergetag', I'd understand
> it, but that would essentially mean that 'replace --graft' should
> never be used, so...

I think that when "replace --graft" is used on a regular commit there
is much better chance that the resulting replacement commit will be as
the user expect it to be.

Thanks,
Christian.

Re: [PATCH v6 03/10] replace: add test for --graft

[Christian Couder]

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

> 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 | 40 ++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 40 insertions(+)
>>
>> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
>> index fb07ad2..d80a89e 100755
>> --- a/t/t6050-replace.sh
>> +++ b/t/t6050-replace.sh
>> @@ -18,6 +18,33 @@ add_and_commit_file()
>>      git commit --quiet -m "$_file: $_msg"
>>  }
>>  
>> +commit_buffer_contains_parents()
> 
> SP before (), perhaps?

Ok.

>> +{
>> +    git cat-file commit "$1" >payload &&
>> +    sed -n -e '/^$/q' -e '/^parent /p' <payload >actual &&
>> +    shift &&
>> +    : >expected &&
>> +    for _parent
>> +    do
>> +	echo "parent $_parent" >>expected || return 1
>> +    done &&
> 
> You can redirect the entire loop to simplify the above 5 lines,
> which would read better, no?
> 
> 	for _parent
>         do
> 		echo "parent $_parent"
> 	done >expect

Ok, thanks.
 
>> +    test_cmp actual expected
> 
> As "test_cmp" normally runs "diff", it is better to compare expect
> with actual, not the other way around; running the test verbosely,
> i.e. "t6050-replace.sh -v", shows how the actual output differs from
> what is expected when diagnosing breakage and would be more useful
> that way.

Ok.

> If your goal is to test both the object contents with this code
> *and* the overall system behaviour with the "$child^$parent" below,
> perhaps they should be in the same "commit_has_parents" function,
> without forcing the caller to choose between the two or call both,
> no?

Yeah, will do.

Thanks,
Christian.

Re: [PATCH v6 08/10] commit: add for_each_mergetag()

[Christian Couder]

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

> Christian Couder <chriscool@tuxfamily.org> writes:
> 
>> In the same way as there is for_each_ref() to
>> iterate on refs, it might be useful to have
>> for_each_mergetag() to iterate on the mergetags
>> of a given commit.
>>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
> 
> Heh, "might be useful" is an understatement ;-) We won't apply a
> change that "might be useful" very lightly, but this refactoring
> already uses the helper in existing code, showing that it *is*
> useful, no?
> 
> Let's have this early in the series, or perhaps even independent of
> the "replace" series.

Ok, thanks,
Christian.

Re: [PATCH v6 02/10] replace: add --graft option

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

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

Sensible.

>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/replace.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 73 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/replace.c b/builtin/replace.c
> index 1bb491d..ad47237 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,66 @@ static int edit_and_replace(const char *object_ref, int force)
>  	return replace_object_sha1(object_ref, old, "replacement", new, force);
>  }
>  
> +static void replace_parents(struct strbuf *buf, int argc, const char **argv)
> +{
> +	struct strbuf new_parents = STRBUF_INIT;
> +	const char *parent_start, *parent_end;
> +	int i;
> +
> +	/* find existing parents */
> +	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++) {

It looks somewhat strange that both replace_parents() and
create_graft() take familiar-looking <argc, argv> pair, but one
ignores argv[0] and uses the remainder and the other uses argv[0].
Shouldn't this function consume argv[] starting from [0] for
consistency?  You'd obviously need to update the caller to adjust
the arguments it gives to this function.

> +static int create_graft(int argc, const char **argv, int force)
> +{
> +	unsigned char old[20], new[20];
> +	const char *old_ref = argv[0];
> +...
> +
> +	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(&buf);
> +
> +	if (!hashcmp(old, new))
> +		return error("new commit is the same as the old one: '%s'", sha1_to_hex(old));

Is this really an error?  It may be a warning-worthy situation for a
user or a script to end up doing a no-op graft, e.g.

	git replace --graft HEAD HEAD^

but I wonder if it is more convenient to signal an error (like this
patch does) or just ignore the request and return without adding the
replace ref.

Other than these two points, looks good to me.

Thanks.

Re: [PATCH v6 02/10] replace: add --graft option

[Christian Couder]

On Wed, Jul 9, 2014 at 4:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
>
>> +static void replace_parents(struct strbuf *buf, int argc, const char **argv)
>> +{
>> +     struct strbuf new_parents = STRBUF_INIT;
>> +     const char *parent_start, *parent_end;
>> +     int i;
>> +
>> +     /* find existing parents */
>> +     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++) {
>
> It looks somewhat strange that both replace_parents() and
> create_graft() take familiar-looking <argc, argv> pair, but one
> ignores argv[0] and uses the remainder and the other uses argv[0].
> Shouldn't this function consume argv[] starting from [0] for
> consistency?  You'd obviously need to update the caller to adjust
> the arguments it gives to this function.

Ok, will do.

>> +static int create_graft(int argc, const char **argv, int force)
>> +{
>> +     unsigned char old[20], new[20];
>> +     const char *old_ref = argv[0];
>> +...
>> +
>> +     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(&buf);
>> +
>> +     if (!hashcmp(old, new))
>> +             return error("new commit is the same as the old one: '%s'", sha1_to_hex(old));
>
> Is this really an error?  It may be a warning-worthy situation for a
> user or a script to end up doing a no-op graft, e.g.
>
>         git replace --graft HEAD HEAD^
>
> but I wonder if it is more convenient to signal an error (like this
> patch does) or just ignore the request and return without adding the
> replace ref.

As the user might expect that a new replace ref was created on success
(0 exit code), and as we should at least warn if we would create a
commit that is the same as an existing one, I think it is just simpler
to error out in this case.

Though maybe we could use a special exit code (for example 2) in this
case, so that the user might more easily ignore this error in a
script.

> Other than these two points, looks good to me.

Thanks,
Christian.

Re: [PATCH v6 02/10] replace: add --graft option

[Junio C Hamano]

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

>> Is this really an error?  It may be a warning-worthy situation for a
>> user or a script to end up doing a no-op graft, e.g.
>>
>>         git replace --graft HEAD HEAD^
>>
>> but I wonder if it is more convenient to signal an error (like this
>> patch does) or just ignore the request and return without adding the
>> replace ref.
>
> As the user might expect that a new replace ref was created on success
> (0 exit code), and as we should at least warn if we would create a
> commit that is the same as an existing one,...

Why is it an event that needs a warning?  I do not buy that "as we
should at least" at all.

If you say "make sure A's parent is B" and then you asked the same
thing again when there already is a replacement in place, that
should be a no-op.  "Making sure A's parent is B" would be an
idempotent operation, no?  Why not just make sure A's parent is
already B and report "Your wish has been granted" to the user?

Why would it be simpler for the user to get an error, inspect the
situation and realize that his wish has been granted after all?

Re: [PATCH v6 02/10] replace: add --graft option

[Junio C Hamano]

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

> Christian Couder <christian.couder@gmail.com> writes:
>
>>> Is this really an error?  It may be a warning-worthy situation for a
>>> user or a script to end up doing a no-op graft, e.g.
>>>
>>>         git replace --graft HEAD HEAD^
>>>
>>> but I wonder if it is more convenient to signal an error (like this
>>> patch does) or just ignore the request and return without adding the
>>> replace ref.
>>
>> As the user might expect that a new replace ref was created on success
>> (0 exit code), and as we should at least warn if we would create a
>> commit that is the same as an existing one,...
>
> Why is it an event that needs a warning?  I do not buy that "as we
> should at least" at all.

Ehh, it came a bit differently from what I meant.  Perhaps s/do not
buy/do not understand/ is closer to what I think---that is, it is
not like I with a strong conviction think you are wrong.  It is more
like I do not understand why you think it needs a warning, meaing
you would need to explain it better.

> If you say "make sure A's parent is B" and then you asked the same
> thing again when there already is a replacement in place, that
> should be a no-op.  "Making sure A's parent is B" would be an
> idempotent operation, no?  Why not just make sure A's parent is
> already B and report "Your wish has been granted" to the user?
>
> Why would it be simpler for the user to get an error, inspect the
> situation and realize that his wish has been granted after all?

Re: [PATCH v6 02/10] replace: add --graft option

[Christian Couder]

On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>>
>>> As the user might expect that a new replace ref was created on success
>>> (0 exit code), and as we should at least warn if we would create a
>>> commit that is the same as an existing one,...
>>
>> Why is it an event that needs a warning?  I do not buy that "as we
>> should at least" at all.

Here you ask why this event needs a warning...

> Ehh, it came a bit differently from what I meant.  Perhaps s/do not
> buy/do not understand/ is closer to what I think---that is, it is
> not like I with a strong conviction think you are wrong.  It is more
> like I do not understand why you think it needs a warning, meaing
> you would need to explain it better.
>
>> If you say "make sure A's parent is B" and then you asked the same
>> thing again when there already is a replacement in place, that
>> should be a no-op.

(When there is already a replacement in place we error out in
replace_object_sha1() unless --force is used. What we are talking
about here is what happens if the replacement commit is the same as
the original commit.)

>> "Making sure A's parent is B" would be an
>> idempotent operation, no?  Why not just make sure A's parent is
>> already B and report "Your wish has been granted" to the user?

... and here you say we should report "your wish has been granted"...

>> Why would it be simpler for the user to get an error, inspect the
>> situation and realize that his wish has been granted after all?

... but for me reporting to the user "your wish has been granted" and
warning (or errorring out) saying "the new commit would be the same as
the old one" are nearly the same thing.

So I wonder what exactly you are not happy with.

Is it the fact that I use the error() function, because it would
prefix the message with "fatal:" and that would be too scary?

Is it because with error() the exit code would not be 0?

Is it because the message "new commit is the same as the old one:
'%s'" is too scary or unnecessarily technical by itself?

Is it ok if I just print the message on stderr without "warning:" or
"fatal:" in front of it?

By the way, when I say "As ... and ..., I think it is just simpler to
error out in this case.", I mean that it is simpler from the
developer's point of view, not necessarily for the user.

Of course I am ok with improving things for the user even if it
requires more code/work, but it is difficult to properly do that if I
don't see how I could do it.

Thanks,
Christian.

Re: [PATCH v6 02/10] replace: add --graft option

[Junio C Hamano]

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

> On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>>> "Making sure A's parent is B" would be an
>>> idempotent operation, no?  Why not just make sure A's parent is
>>> already B and report "Your wish has been granted" to the user?
>
> ... and here you say we should report "your wish has been granted"...

Normal way for "git replace" to report that is to exit with status 0
and without any noise, I would think.

>>> Why would it be simpler for the user to get an error, inspect the
>>> situation and realize that his wish has been granted after all?
>
> ... but for me reporting to the user "your wish has been granted" and
> warning (or errorring out) saying "the new commit would be the same as
> the old one" are nearly the same thing.
>
> So I wonder what exactly you are not happy with.

See above.

Re: [PATCH v6 02/10] replace: add --graft option

[Christian Couder]

On Fri, Jul 11, 2014 at 4:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>>> "Making sure A's parent is B" would be an
>>>> idempotent operation, no?  Why not just make sure A's parent is
>>>> already B and report "Your wish has been granted" to the user?
>>
>> ... and here you say we should report "your wish has been granted"...
>
> Normal way for "git replace" to report that is to exit with status 0
> and without any noise, I would think.

In a similar case "git replace --edit" we error out instead of just
exiting (with status 0), see:

f22166b5fee7dc (replace: make sure --edit results in a different object)

Re: [PATCH v6 02/10] replace: add --graft option

[Junio C Hamano]

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

> On Fri, Jul 11, 2014 at 4:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Christian Couder <christian.couder@gmail.com> writes:
>>
>>> On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>>>> "Making sure A's parent is B" would be an
>>>>> idempotent operation, no?  Why not just make sure A's parent is
>>>>> already B and report "Your wish has been granted" to the user?
>>>
>>> ... and here you say we should report "your wish has been granted"...
>>
>> Normal way for "git replace" to report that is to exit with status 0
>> and without any noise, I would think.
>
> In a similar case "git replace --edit" we error out instead of just
> exiting (with status 0), see:
>
> f22166b5fee7dc (replace: make sure --edit results in a different object)

I do not care *too* deeply, but if you ask me, that may be a mistake
we may want to fix before the next release.

Re: [PATCH v6 02/10] replace: add --graft option

[Jeff King]

On Fri, Jul 11, 2014 at 11:25:43AM -0700, Junio C Hamano wrote:

> Christian Couder <christian.couder@gmail.com> writes:
> 
> > On Fri, Jul 11, 2014 at 4:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >> Christian Couder <christian.couder@gmail.com> writes:
> >>
> >>> On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >>>
> >>>>> "Making sure A's parent is B" would be an
> >>>>> idempotent operation, no?  Why not just make sure A's parent is
> >>>>> already B and report "Your wish has been granted" to the user?
> >>>
> >>> ... and here you say we should report "your wish has been granted"...
> >>
> >> Normal way for "git replace" to report that is to exit with status 0
> >> and without any noise, I would think.
> >
> > In a similar case "git replace --edit" we error out instead of just
> > exiting (with status 0), see:
> >
> > f22166b5fee7dc (replace: make sure --edit results in a different object)
> 
> I do not care *too* deeply, but if you ask me, that may be a mistake
> we may want to fix before the next release.

Yeah, I also do not care too deeply, but I mentioned in the earlier
review that I would expect it to just remove the replacement if it ends
up generating the same object.

-Peff