threads / patch / 37018

v5, 7 partsreplace: add test for --graft

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

## tl;dr

17 messages between Jun 28, 2014 and Jul 3, 2014. Diffs are folded; open one to read it.

replies: 16people: 2as markdown or json

Christian Couder· Jun 28, 2014, 18:11 UTC · lore

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

Here is a small patch series to implement:
        git replace [-f] --graft <commit> [<parent>...]

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

The changes since v4, thanks to Junio and Peff, are:
- The series has been rebased on top of Peff's series
  to store the commit buffer length
  (jk/commit-buffer-length). This changes patch 1/7
  and fixes the problem that we lost everything after
  a NUL byte in the commit buffer. 
- Patches 5/7, 6/7 and 7/7 have been added to lose
  a gpg signature in the original commit buffer.
  
Notes:
- Maybe patches could be reordered or squashed, but
  I prefered not to do it in this round so that
  changes compared to previous version are easier to
  spot.
- Junio suggested to drop the merge-tag of a parent
  we are not going to keep, but to keep the merge-tag
  of a parent we are keeping. This has not yet been
  done. It is unfortunate that merge-tags don't use
  a format like the trailer format, because we could
  have factorised droping a merge-tag in the
  "git interpret-trailers" command.
Christian Couder (7):
  replace: add --graft option
  replace: add test for --graft
  Documentation: replace: add --graft option
  contrib: add convert-grafts-to-replace-refs.sh
  replace: refactor replacing parents
  replace: remove signature when using --graft
  replace: add test for --graft with signed commit
 Documentation/git-replace.txt             | 10 ++++
 builtin/replace.c                         | 79 ++++++++++++++++++++++++++++++-
 commit.c                                  | 34 +++++++++++++
 commit.h                                  |  2 +
 contrib/convert-grafts-to-replace-refs.sh | 28 +++++++++++
 t/t6050-replace.sh                        | 34 +++++++++++++
 6 files changed, 186 insertions(+), 1 deletion(-)
 create mode 100755 contrib/convert-grafts-to-replace-refs.sh
-- 
2.0.0.421.g786a89d.dirty
Christian Couder· Jun 28, 2014, 18:11 UTC · re: Christian Couder · lore

[PATCH v5 1/7] 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 | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)
Show changes to builtin/replace.c +65 −1
diff --git a/builtin/replace.c b/builtin/replace.c
index 1bb491d..3515979 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -17,6 +17,7 @@
 static const char * const git_replace_usage[] = {
 	N_("git replace [-f] <object> <replacement>"),
 	N_("git replace [-f] --edit <object>"),
+	N_("git replace [-f] --graft <commit> [<parent>...]"),
 	N_("git replace -d <object>..."),
 	N_("git replace [--format=<format>] [-l [<pattern>]]"),
 	NULL
@@ -294,6 +295,58 @@ static int edit_and_replace(const char *object_ref, int force)
 	return replace_object_sha1(object_ref, old, "replacement", new, force);
 }
 
+static int create_graft(int argc, const char **argv, int force)
+{
+	unsigned char old[20], new[20];
+	const char *old_ref = argv[0];
+	struct commit *commit;
+	struct strbuf buf = STRBUF_INIT;
+	struct strbuf new_parents = STRBUF_INIT;
+	const char *parent_start, *parent_end;
+	const char *buffer;
+	unsigned long size;
+	int i;
+
+	if (get_sha1(old_ref, old) < 0)
+		die(_("Not a valid object name: '%s'"), old_ref);
+	commit = lookup_commit_or_die(old, old_ref);
+
+	/* find existing parents */
+	buffer = get_commit_buffer(commit, &size);
+	strbuf_add(&buf, buffer, size);
+	unuse_commit_buffer(commit, buffer);
+	parent_start = buf.buf;
+	parent_start += 46; /* "tree " + "hex sha1" + "\n" */
+	parent_end = parent_start;
+
+	while (starts_with(parent_end, "parent "))
+		parent_end += 48; /* "parent " + "hex sha1" + "\n" */
+
+	/* prepare new parents */
+	for (i = 1; i < argc; i++) {
+		unsigned char sha1[20];
+		if (get_sha1(argv[i], sha1) < 0)
+			die(_("Not a valid object name: '%s'"), argv[i]);
+		lookup_commit_or_die(sha1, argv[i]);
+		strbuf_addf(&new_parents, "parent %s\n", sha1_to_hex(sha1));
+	}
+
+	/* replace existing parents with new ones */
+	strbuf_splice(&buf, parent_start - buf.buf, parent_end - parent_start,
+		      new_parents.buf, new_parents.len);
+
+	if (write_sha1_file(buf.buf, buf.len, commit_type, new))
+		die(_("could not write replacement commit for: '%s'"), old_ref);
+
+	strbuf_release(&new_parents);
+	strbuf_release(&buf);
+
+	if (!hashcmp(old, new))
+		return error("new commit is the same as the old one: '%s'", sha1_to_hex(old));
+
+	return replace_object_sha1(old_ref, old, "replacement", new, force);
+}
+
 int cmd_replace(int argc, const char **argv, const char *prefix)
 {
 	int force = 0;
@@ -303,12 +356,14 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
 		MODE_LIST,
 		MODE_DELETE,
 		MODE_EDIT,
+		MODE_GRAFT,
 		MODE_REPLACE
 	} cmdmode = MODE_UNSPECIFIED;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list replace refs"), MODE_LIST),
 		OPT_CMDMODE('d', "delete", &cmdmode, N_("delete replace refs"), MODE_DELETE),
 		OPT_CMDMODE('e', "edit", &cmdmode, N_("edit existing object"), MODE_EDIT),
+		OPT_CMDMODE('g', "graft", &cmdmode, N_("change a commit's parents"), MODE_GRAFT),
 		OPT_BOOL('f', "force", &force, N_("replace the ref if it exists")),
 		OPT_STRING(0, "format", &format, N_("format"), N_("use this format")),
 		OPT_END()
@@ -325,7 +380,10 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
 		usage_msg_opt("--format cannot be used when not listing",
 			      git_replace_usage, options);
 
-	if (force && cmdmode != MODE_REPLACE && cmdmode != MODE_EDIT)
+	if (force &&
+	    cmdmode != MODE_REPLACE &&
+	    cmdmode != MODE_EDIT &&
+	    cmdmode != MODE_GRAFT)
 		usage_msg_opt("-f only makes sense when writing a replacement",
 			      git_replace_usage, options);
 
@@ -348,6 +406,12 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
 				      git_replace_usage, options);
 		return edit_and_replace(argv[0], force);
 
+	case MODE_GRAFT:
+		if (argc < 1)
+			usage_msg_opt("-g needs at least one argument",
+				      git_replace_usage, options);
+		return create_graft(argc, argv, force);
+
 	case MODE_LIST:
 		if (argc > 1)
 			usage_msg_opt("only one pattern can be given with -l",
-- 
2.0.0.421.g786a89d.dirty
Christian Couder· Jun 28, 2014, 18:11 UTC · re: Christian Couder · lore
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t6050-replace.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)
Show changes to t/t6050-replace.sh +12 −0
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index 68b3cb2..ca45a84 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -351,4 +351,16 @@ test_expect_success 'replace ref cleanup' '
 	test -z "$(git replace)"
 '
 
+test_expect_success '--graft with and without already replaced object' '
+	test $(git log --oneline | wc -l) = 7 &&
+	git replace --graft $HASH5 &&
+	test $(git log --oneline | wc -l) = 3 &&
+	git cat-file -p $HASH5 | test_must_fail grep parent &&
+	test_must_fail git replace --graft $HASH5 $HASH4 $HASH3 &&
+	git replace --force -g $HASH5 $HASH4 $HASH3 &&
+	git cat-file -p $HASH5 | grep "parent $HASH4" &&
+	git cat-file -p $HASH5 | grep "parent $HASH3" &&
+	git replace -d $HASH5
+'
+
 test_done
-- 
2.0.0.421.g786a89d.dirty
Junio C Hamano· Jul 2, 2014, 20:49 UTC · re: Christian Couder · lore

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

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

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

	commit_has_parents $HASH5 $HASH4 $HASH3
would then may be implemented like:
	commit_has_parents () {
		git cat-file commit "$1" >payload &&
                sed -n -e '/^$/q' -e 's/^parent //p' <payload >actual &&
		shift &&
                printf 'parent %s\n' "$@" >expect &&
		test_cmp expect actual
	}
Christian Couder· Jul 3, 2014, 13:39 UTC · re: Junio C Hamano · lore

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

On Wed, Jul 2, 2014 at 10:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 23 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 | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
>> index 68b3cb2..ca45a84 100755
>> --- a/t/t6050-replace.sh
>> +++ b/t/t6050-replace.sh
>> @@ -351,4 +351,16 @@ test_expect_success 'replace ref cleanup' '
>>       test -z "$(git replace)"
>>  '
>>
>> +test_expect_success '--graft with and without already replaced object' '
>> +     test $(git log --oneline | wc -l) = 7 &&
>> +     git replace --graft $HASH5 &&
>> +     test $(git log --oneline | wc -l) = 3 &&
>> +     git cat-file -p $HASH5 | test_must_fail grep parent &&
>
> Please do not run non-git command under test_must_fail.
Ok, I think I will just use the following then:
test_must_fail git rev-parse $HASH5^1
Show 27 quoted lines
>> +     test_must_fail git replace --graft $HASH5 $HASH4 $HASH3 &&
>> +     git replace --force -g $HASH5 $HASH4 $HASH3 &&
>> +     git cat-file -p $HASH5 | grep "parent $HASH4" &&
>> +     git cat-file -p $HASH5 | grep "parent $HASH3" &&
>> +     git replace -d $HASH5
>> +'
>> +
>>  test_done
>
> For all these "git cat-file -p $commit | grep ...", I would think
> you should add "commit_has_parents" helper function to allow a bit
> more careful testing.  As written, the test will mistake a commit
> with string "parent $HASHx" anywhere, not in the header part, and
> the test does not ensure that $HASH{3,4} is the {first,second}
> parent.
>
>         commit_has_parents $HASH5 $HASH4 $HASH3
>
> would then may be implemented like:
>
>         commit_has_parents () {
>                 git cat-file commit "$1" >payload &&
>                 sed -n -e '/^$/q' -e 's/^parent //p' <payload >actual &&
>                 shift &&
>                 printf 'parent %s\n' "$@" >expect &&
>                 test_cmp expect actual
>         }
I think I'll rather use something like:

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

Thanks, Christian.

Junio C Hamano· Jul 3, 2014, 18:45 UTC · re: Christian Couder · lore

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

Christian Couder <christian.couder@gmail.com> writes:
Show 29 quoted lines
> On Wed, Jul 2, 2014 at 10:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Christian Couder <chriscool@tuxfamily.org> writes:
>>
>>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>> ---
>>>  t/t6050-replace.sh | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
>>> index 68b3cb2..ca45a84 100755
>>> --- a/t/t6050-replace.sh
>>> +++ b/t/t6050-replace.sh
>>> @@ -351,4 +351,16 @@ test_expect_success 'replace ref cleanup' '
>>>       test -z "$(git replace)"
>>>  '
>>>
>>> +test_expect_success '--graft with and without already replaced object' '
>>> +     test $(git log --oneline | wc -l) = 7 &&
>>> +     git replace --graft $HASH5 &&
>>> +     test $(git log --oneline | wc -l) = 3 &&
>>> +     git cat-file -p $HASH5 | test_must_fail grep parent &&
>>
>> Please do not run non-git command under test_must_fail.
>
> Ok, I think I will just use the following then:
>
> test_must_fail git rev-parse $HASH5^1
> ...
See below.
Show 33 quoted lines
>>> +     test_must_fail git replace --graft $HASH5 $HASH4 $HASH3 &&
>>> +     git replace --force -g $HASH5 $HASH4 $HASH3 &&
>>> +     git cat-file -p $HASH5 | grep "parent $HASH4" &&
>>> +     git cat-file -p $HASH5 | grep "parent $HASH3" &&
>>> +     git replace -d $HASH5
>>> +'
>>> +
>>>  test_done
>>
>> For all these "git cat-file -p $commit | grep ...", I would think
>> you should add "commit_has_parents" helper function to allow a bit
>> more careful testing.  As written, the test will mistake a commit
>> with string "parent $HASHx" anywhere, not in the header part, and
>> the test does not ensure that $HASH{3,4} is the {first,second}
>> parent.
>>
>>         commit_has_parents $HASH5 $HASH4 $HASH3
>>
>> would then may be implemented like:
>>
>>         commit_has_parents () {
>>                 git cat-file commit "$1" >payload &&
>>                 sed -n -e '/^$/q' -e 's/^parent //p' <payload >actual &&
>>                 shift &&
>>                 printf 'parent %s\n' "$@" >expect &&
>>                 test_cmp expect actual
>>         }
>
> I think I'll rather use something like:
>
> test $(git rev-parse $HASH5^1) = "$HASH4" &&
> test $(git rev-parse $HASH5^2) = "$HASH3" &&
> ...

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

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

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

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

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

        commit=$1 parent_number=1
        shift
        for parent
        do
                test $(git rev-parse --verify "$commit^$parent_number") = "$parent" ||
		return 1
		parent_number=$(( $parent_number + 1 ))
	done
        test_must_fail git rev-parse $commit^$parent_number
and you would still use it like this:
  commit_has_parents $HASH5 ;# must have no parent
  commit_has_parents $HASH5 $HASH4 $HASH3 ;# must have these parents
Thanks.
Christian Couder· Jun 28, 2014, 18:11 UTC · re: Christian Couder · lore

[PATCH v5 3/7] 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· Jun 28, 2014, 18:11 UTC · re: Christian Couder · lore

[PATCH v5 4/7] 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· Jun 28, 2014, 18:11 UTC · re: Christian Couder · lore

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

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/replace.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)
Show changes to builtin/replace.c +25 −17
diff --git a/builtin/replace.c b/builtin/replace.c
index 3515979..ad47237 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -295,27 +295,14 @@ static int edit_and_replace(const char *object_ref, int force)
 	return replace_object_sha1(object_ref, old, "replacement", new, force);
 }
 
-static int create_graft(int argc, const char **argv, int force)
+static void replace_parents(struct strbuf *buf, int argc, const char **argv)
 {
-	unsigned char old[20], new[20];
-	const char *old_ref = argv[0];
-	struct commit *commit;
-	struct strbuf buf = STRBUF_INIT;
 	struct strbuf new_parents = STRBUF_INIT;
 	const char *parent_start, *parent_end;
-	const char *buffer;
-	unsigned long size;
 	int i;
 
-	if (get_sha1(old_ref, old) < 0)
-		die(_("Not a valid object name: '%s'"), old_ref);
-	commit = lookup_commit_or_die(old, old_ref);
-
 	/* find existing parents */
-	buffer = get_commit_buffer(commit, &size);
-	strbuf_add(&buf, buffer, size);
-	unuse_commit_buffer(commit, buffer);
-	parent_start = buf.buf;
+	parent_start = buf->buf;
 	parent_start += 46; /* "tree " + "hex sha1" + "\n" */
 	parent_end = parent_start;
 
@@ -332,13 +319,34 @@ static int create_graft(int argc, const char **argv, int force)
 	}
 
 	/* replace existing parents with new ones */
-	strbuf_splice(&buf, parent_start - buf.buf, parent_end - parent_start,
+	strbuf_splice(buf, parent_start - buf->buf, parent_end - parent_start,
 		      new_parents.buf, new_parents.len);
 
+	strbuf_release(&new_parents);
+}
+
+static int create_graft(int argc, const char **argv, int force)
+{
+	unsigned char old[20], new[20];
+	const char *old_ref = argv[0];
+	struct commit *commit;
+	struct strbuf buf = STRBUF_INIT;
+	const char *buffer;
+	unsigned long size;
+
+	if (get_sha1(old_ref, old) < 0)
+		die(_("Not a valid object name: '%s'"), old_ref);
+	commit = lookup_commit_or_die(old, old_ref);
+
+	buffer = get_commit_buffer(commit, &size);
+	strbuf_add(&buf, buffer, size);
+	unuse_commit_buffer(commit, buffer);
+
+	replace_parents(&buf, argc, argv);
+
 	if (write_sha1_file(buf.buf, buf.len, commit_type, new))
 		die(_("could not write replacement commit for: '%s'"), old_ref);
 
-	strbuf_release(&new_parents);
 	strbuf_release(&buf);
 
 	if (!hashcmp(old, new))
-- 
2.0.0.421.g786a89d.dirty
Junio C Hamano· Jul 2, 2014, 21:05 UTC · re: Christian Couder · lore

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

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

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

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

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

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

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

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

Christian Couder <chriscool@tuxfamily.org> writes:
Show 7 quoted lines
> It could be misleading to keep a signature in a
> replacement commit, so let's remove it.
>
> Note that there should probably be a way to sign
> the replacement commit created when using --graft,
> but this can be dealt with in another commit or
> patch series.
Both paragraphs read very sensibly.
Show 19 quoted lines
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  builtin/replace.c |  5 +++++
>  commit.c          | 34 ++++++++++++++++++++++++++++++++++
>  commit.h          |  2 ++
>  3 files changed, 41 insertions(+)
>
> diff --git a/builtin/replace.c b/builtin/replace.c
> index ad47237..000db65 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -344,6 +344,11 @@ static int create_graft(int argc, const char **argv, int force)
>  
>  	replace_parents(&buf, argc, argv);
>  
> +	if (remove_signature(&buf))
> +		warning(_("the original commit '%s' has a gpg signature.\n"
> +			  "It will be removed in the replacement commit!"),

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

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

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

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

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

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

Show 19 quoted lines
> +	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);
Christian Couder· Jul 3, 2014, 14:09 UTC · re: Junio C Hamano · lore

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

On Wed, Jul 2, 2014 at 11:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> Christian Couder <chriscool@tuxfamily.org> writes:
>
>> It could be misleading to keep a signature in a
>> replacement commit, so let's remove it.
>>
>> Note that there should probably be a way to sign
>> the replacement commit created when using --graft,
>> but this can be dealt with in another commit or
>> patch series.
>
> Both paragraphs read very sensibly.
Thanks.
Show 12 quoted lines
>> --- a/builtin/replace.c
>> +++ b/builtin/replace.c
>> @@ -344,6 +344,11 @@ static int create_graft(int argc, const char **argv, int force)
>>
>>       replace_parents(&buf, argc, argv);
>>
>> +     if (remove_signature(&buf))
>> +             warning(_("the original commit '%s' has a gpg signature.\n"
>> +                       "It will be removed in the replacement commit!"),
>
> Hmmm...  does the second line of this message start with the usual
> "warning:" prefix?
Ok, I will use following:
if (remove_signature(&buf)) {
        warning(_("the original commit '%s' has a gpg signature."), old_ref);
        warning(_("the signature will be removed in the replacement commit!"));
}
Show 26 quoted lines
>> diff --git a/commit.c b/commit.c
>> index fb7897c..54e157d 100644
>> --- a/commit.c
>> +++ b/commit.c
>> @@ -1177,6 +1177,40 @@ int parse_signed_commit(const struct commit *commit,
>>       return saw_signature;
>>  }
>>
>> +int remove_signature(struct strbuf *buf)
>> +{
>> +     const char *line = buf->buf;
>> +     const char *tail = buf->buf + buf->len;
>> +     int in_signature = 0;
>> +     const char *sig_start = NULL;
>> +     const char *sig_end = NULL;
>> +
>> +     while (line < tail) {
>> +             const char *next = memchr(line, '\n', tail - line);
>> +             next = next ? next + 1 : tail;
>
> This almost makes me wonder if we want something similar to
> strchrnul() we use for NUL-terminated strings, and I suspect that
> you would find more instances by running "git grep -A2 memchr".
>
> I don't know what such a helper function should be named, though.
> Certainly not "memchrnul()".
I can add this to a GSoC microproject page for next year.
Show 23 quoted lines
>> +             if (in_signature && line[0] == ' ')
>> +                     sig_end = next;
>> +             else if (starts_with(line, gpg_sig_header) &&
>> +                      line[gpg_sig_header_len] == ' ') {
>> +                     sig_start = line;
>> +                     sig_end = next;
>> +                     in_signature = 1;
>> +             } else {
>> +                     if (*line == '\n')
>> +                             /* dump the whole remainder of the buffer */
>> +                             next = tail;
>> +                     in_signature = 0;
>> +             }
>> +             line = next;
>> +     }
>> +
>> +     if (sig_start)
>> +             strbuf_remove(buf, sig_start - buf->buf, sig_end - sig_start);
>
> If there are two instances of gpg_sig, this will remove only the
> last one, but there is no chance both signatures of such a commit
> can validate OK, and we won't be losing something in between anyway,
> so it should be fine.
Ok.

Thanks, Christian.

Christian Couder· Jun 28, 2014, 18:11 UTC · re: Christian Couder · lore

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

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

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

Christian Couder <chriscool@tuxfamily.org> writes:
Show 24 quoted lines
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  t/t6050-replace.sh | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
> index ca45a84..80b85e3 100755
> --- a/t/t6050-replace.sh
> +++ b/t/t6050-replace.sh
> @@ -7,6 +7,7 @@ test_description='Tests replace refs functionality'
>  exec </dev/null
>  
>  . ./test-lib.sh
> +. "$TEST_DIRECTORY/lib-gpg.sh"
>  
>  add_and_commit_file()
>  {
> @@ -363,4 +364,25 @@ test_expect_success '--graft with and without already replaced object' '
>  	git replace -d $HASH5
>  '
>  
> +test_expect_success GPG 'set up a signed commit' '
> +	echo "line 17" >> hello &&
> +	echo "line 18" >> hello &&
Style?
Show 14 quoted lines
> +	git add hello &&
> +	test_tick &&
> +	git commit --quiet -S -m "hello: 2 more lines in a signed commit" &&
> +	HASH8=$(git rev-parse --verify HEAD) &&
> +	git verify-commit $HASH8
> +'
> +
> +test_expect_success GPG '--graft with a signed commit' '
> +	git cat-file commit $HASH8 >orig &&
> +	git replace --graft $HASH8 &&
> +	git cat-file commit $HASH8 >repl &&
> +	test_must_fail grep gpgsig repl &&
> +	diff -u orig repl | grep "^-parent $HASH7" &&
> +	diff -u orig repl | grep "^-gpgsig -----BEGIN PGP SIGNATURE-----" &&
Almost the same comment as the one for 2/7 applies here.
Show 5 quoted lines
> +	test_must_fail git verify-commit $HASH8 &&
> +	git replace -d $HASH8
> +'
> +
>  test_done
Christian Couder· Jul 3, 2014, 14:17 UTC · re: Junio C Hamano · lore

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

On Wed, Jul 2, 2014 at 11:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
> Christian Couder <chriscool@tuxfamily.org> writes:
>
>>
>> +test_expect_success GPG 'set up a signed commit' '
>> +     echo "line 17" >> hello &&
>> +     echo "line 18" >> hello &&
>
> Style?
Yeah, I will change it to:
     echo "line 17" >>hello &&
     echo "line 18" >>hello &&
Show 16 quoted lines
>> +     git add hello &&
>> +     test_tick &&
>> +     git commit --quiet -S -m "hello: 2 more lines in a signed commit" &&
>> +     HASH8=$(git rev-parse --verify HEAD) &&
>> +     git verify-commit $HASH8
>> +'
>> +
>> +test_expect_success GPG '--graft with a signed commit' '
>> +     git cat-file commit $HASH8 >orig &&
>> +     git replace --graft $HASH8 &&
>> +     git cat-file commit $HASH8 >repl &&
>> +     test_must_fail grep gpgsig repl &&
>> +     diff -u orig repl | grep "^-parent $HASH7" &&
>> +     diff -u orig repl | grep "^-gpgsig -----BEGIN PGP SIGNATURE-----" &&
>
> Almost the same comment as the one for 2/7 applies here.
Ok, I will find a way to make it better.

Thanks, Christian.

← back to recent threads