threads / patch / 37068

v6, 10 partsAdd --graft option to git replace

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

## tl;dr

26 messages between Jul 7, 2014 and Jul 11, 2014. Diffs are folded; open one to read it.

replies: 25people: 3as markdown or json

Christian Couder· Jul 7, 2014, 06:35 UTC · lore
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
Christian Couder· Jul 7, 2014, 06:35 UTC · re: Christian Couder · lore

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

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t6050-replace.sh | 48 ++++++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 24 deletions(-)
Show changes to t/t6050-replace.sh +24 −24
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
Christian Couder· Jul 7, 2014, 06:35 UTC · re: Christian Couder · lore

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

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(-)
Show changes to builtin/replace.c +73 −1
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
Junio C Hamano· Jul 9, 2014, 14:59 UTC · re: Christian Couder · lore

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

Christian Couder <chriscool@tuxfamily.org> writes:
Show 12 quoted lines
> 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.
Show 39 quoted lines
>
> 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.

Show 15 quoted lines
> +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.
Christian Couder· Jul 10, 2014, 09:30 UTC · re: Junio C Hamano · lore

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

On Wed, Jul 9, 2014 at 4:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 25 quoted lines
> 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.
Show 24 quoted lines
>> +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.

Junio C Hamano· Jul 10, 2014, 16:51 UTC · re: Christian Couder · lore

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

Christian Couder <christian.couder@gmail.com> writes:
Show 12 quoted lines
>> 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?

Junio C Hamano· Jul 10, 2014, 17:36 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 17 quoted lines
> 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.

Show 8 quoted lines
> 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?
Christian Couder· Jul 11, 2014, 08:59 UTC · re: Junio C Hamano · lore

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

On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
> 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...
Show 9 quoted lines
> 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.

Junio C Hamano· Jul 11, 2014, 14:22 UTC · re: Christian Couder · lore

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

Christian Couder <christian.couder@gmail.com> writes:
Show 7 quoted lines
> 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.

Show 8 quoted lines
>>> 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.
Christian Couder· Jul 11, 2014, 16:24 UTC · re: Junio C Hamano · lore

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

On Fri, Jul 11, 2014 at 4:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
> 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)
Junio C Hamano· Jul 11, 2014, 18:25 UTC · re: Christian Couder · lore

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

Christian Couder <christian.couder@gmail.com> writes:
Show 18 quoted lines
> 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.

Jeff King· Jul 11, 2014, 18:29 UTC · re: Junio C Hamano · lore

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

On Fri, Jul 11, 2014 at 11:25:43AM -0700, Junio C Hamano wrote:
Show 23 quoted lines
> 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
Christian Couder· Jul 7, 2014, 06:35 UTC · re: Christian Couder · lore

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

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(+)
Show changes to t/t6050-replace.sh +40 −0
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
Junio C Hamano· Jul 7, 2014, 22:16 UTC · re: Christian Couder · lore

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

Christian Couder <chriscool@tuxfamily.org> writes:
Show 15 quoted lines
> 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?
Show 9 quoted lines
> +{
> +    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?

Show 14 quoted lines
> +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
Christian Couder· Jul 8, 2014, 05:36 UTC · re: Junio C Hamano · lore

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

From: Junio C Hamano <gitster@pobox.com>
Show 19 quoted lines
> 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.
Show 17 quoted lines
>> +{
>> +    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.
 
Show 7 quoted lines
>> +    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.
Show 5 quoted lines
> 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.

Christian Couder· Jul 7, 2014, 06:35 UTC · re: Christian Couder · lore

[PATCH v6 04/10] Documentation: replace: add --graft option

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(+)
Show changes to Documentation/git-replace.txt +8 −0
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
Christian Couder· Jul 7, 2014, 06:35 UTC · re: Christian Couder · lore

[PATCH v6 05/10] contrib: add convert-grafts-to-replace-refs.sh

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
Show changes to 2 files +31 −1

Documentation/git-replace.txt, 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
Christian Couder· Jul 7, 2014, 06:35 UTC · re: Christian Couder · lore

[PATCH v6 06/10] replace: remove signature when using --graft

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(+)
Show changes to 3 files +41 −0

builtin/replace.c, commit.c, commit.h

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
Christian Couder· Jul 7, 2014, 06:35 UTC · re: Christian Couder · lore

[PATCH v6 07/10] replace: add test for --graft with signed commit

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t6050-replace.sh | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)
Show changes to t/t6050-replace.sh +25 −0
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
Christian Couder· Jul 7, 2014, 06:35 UTC · re: Christian Couder · lore

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

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(-)
Show changes to 3 files +22 −11

commit.c, commit.h, log-tree.c

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
Junio C Hamano· Jul 7, 2014, 22:30 UTC · re: Christian Couder · lore

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

Christian Couder <chriscool@tuxfamily.org> writes:
Show 7 quoted lines
> 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.
Show 80 quoted lines
>  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)
Christian Couder· Jul 8, 2014, 05:53 UTC · re: Junio C Hamano · lore

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

From: Junio C Hamano <gitster@pobox.com>
Show 17 quoted lines
> 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.

Christian Couder· Jul 7, 2014, 06:35 UTC · re: Christian Couder · lore

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

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(+)
Show changes to builtin/replace.c +47 −0
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
Junio C Hamano· Jul 7, 2014, 22:28 UTC · re: Christian Couder · lore

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

Christian Couder <chriscool@tuxfamily.org> writes:
Show 11 quoted lines
> 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...

Christian Couder· Jul 8, 2014, 05:35 UTC · re: Junio C Hamano · lore

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

From: Junio C Hamano <gitster@pobox.com>
Show 21 quoted lines
>
> 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.

Show 5 quoted lines
> 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.

Christian Couder· Jul 7, 2014, 06:35 UTC · re: Christian Couder · lore

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

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t6050-replace.sh | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
Show changes to t/t6050-replace.sh +22 −0
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

← back to recent threads