[PATCH 1/3] t6050: use test_line_count instead of wc -l

[PATCH 1/3] t6050: use test_line_count instead of wc -l

[Christian Couder]

This modernizes a test and makes it more portable.

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

diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index d638119750..41b177936e 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -393,9 +393,11 @@ test_expect_success 'replace ref cleanup' '
 '
 
 test_expect_success '--graft with and without already replaced object' '
-	test $(git log --oneline | wc -l) = 7 &&
+	git log --oneline >log &&
+	test_line_count = 7 log &&
 	git replace --graft $HASH5 &&
-	test $(git log --oneline | wc -l) = 3 &&
+	git log --oneline >log &&
+	test_line_count = 3 log &&
 	commit_has_parents $HASH5 &&
 	test_must_fail git replace --graft $HASH5 $HASH4 $HASH3 &&
 	git replace --force -g $HASH5 $HASH4 $HASH3 &&
-- 
2.21.0.68.gd997bba285.dirty

[PATCH 2/3] t6050: redirect expected error output to /dev/null

[Christian Couder]

Otherwise the error from `git rev-parse` is uselessly
polluting the debug output.

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

diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index 41b177936e..5cb8281bab 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -40,7 +40,7 @@ commit_peeling_shows_parents ()
 	test "$_found" = "$_parent" || return 1
 	_parent_number=$(( $_parent_number + 1 ))
     done &&
-    test_must_fail git rev-parse --verify $_commit^$_parent_number
+    test_must_fail git rev-parse --verify $_commit^$_parent_number 2>/dev/null
 }
 
 commit_has_parents ()
-- 
2.21.0.68.gd997bba285.dirty

[PATCH 3/3] replace: fix --graft when passing a tag

[Christian Couder]

When passing a tag as a parent argument to `git replace --graft`,
it can be useful to accept it and use the underlying commit as a
parent.

This already works for lightweight tags, but unfortunately
for annotated tags we have been using the hash of the tag object
instead of the hash of the underlying commit as a parent in the
replacement object we create.

This created invalid objects, but the replace succeeded even if
it showed an error like:

error: object A is a tag, not a commit

This patch fixes that by using the hash of the underlying commit
when an annotated tag is passed.

While at it, let's also update an error message to make it
clearer.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---

This doesn't fix issues when an annotated tag is passed as the
replaced object, that is when the tag is the first argument after
`git replace --graft`. But this can be done in subsequent patches
I already started to work on. 

 builtin/replace.c  |  9 ++++++---
 t/t6050-replace.sh | 11 +++++++++++
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index f5701629a8..b0a9227f9a 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -370,16 +370,19 @@ static int replace_parents(struct strbuf *buf, int argc, const char **argv)
 	/* prepare new parents */
 	for (i = 0; i < argc; i++) {
 		struct object_id oid;
+		struct commit *commit;
+
 		if (get_oid(argv[i], &oid) < 0) {
 			strbuf_release(&new_parents);
 			return error(_("not a valid object name: '%s'"),
 				     argv[i]);
 		}
-		if (!lookup_commit_reference(the_repository, &oid)) {
+		commit = lookup_commit_reference(the_repository, &oid);
+		if (!commit) {
 			strbuf_release(&new_parents);
-			return error(_("could not parse %s"), argv[i]);
+			return error(_("could not parse %s as a commit"), argv[i]);
 		}
-		strbuf_addf(&new_parents, "parent %s\n", oid_to_hex(&oid));
+		strbuf_addf(&new_parents, "parent %s\n", oid_to_hex(&commit->object.oid));
 	}
 
 	/* replace existing parents with new ones */
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index 5cb8281bab..72075983ac 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -405,6 +405,17 @@ test_expect_success '--graft with and without already replaced object' '
 	git replace -d $HASH5
 '
 
+test_expect_success '--graft using a tag as the new parent' '
+	git tag new_parent $HASH5 &&
+	git replace --graft $HASH7 new_parent &&
+	commit_has_parents $HASH7 $HASH5 &&
+	git replace -d $HASH7 &&
+	git tag -a -m "annotated new parent tag" annotated_new_parent $HASH5 &&
+	git replace --graft $HASH7 annotated_new_parent &&
+	commit_has_parents $HASH7 $HASH5 &&
+	git replace -d $HASH7
+'
+
 test_expect_success GPG 'set up a signed commit' '
 	echo "line 17" >>hello &&
 	echo "line 18" >>hello &&
-- 
2.21.0.68.gd997bba285.dirty

Re: [PATCH 3/3] replace: fix --graft when passing a tag

[Taylor Blau]

Hi Christian,

On Thu, Mar 28, 2019 at 06:17:22PM +0100, Christian Couder wrote:
> When passing a tag as a parent argument to `git replace --graft`,
> it can be useful to accept it and use the underlying commit as a
> parent.
>
> This already works for lightweight tags, but unfortunately
> for annotated tags we have been using the hash of the tag object
> instead of the hash of the underlying commit as a parent in the
> replacement object we create.

Ah. If I interpret your description correctly, the issue is that Git
doesn't follow the annotated tag through to the commit it points to.
Makes sense.

> This created invalid objects, but the replace succeeded even if
> it showed an error like:
>
> error: object A is a tag, not a commit
>
> This patch fixes that by using the hash of the underlying commit
> when an annotated tag is passed.

This seems like the straightforward fix.

> While at it, let's also update an error message to make it
> clearer.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>
> This doesn't fix issues when an annotated tag is passed as the
> replaced object, that is when the tag is the first argument after
> `git replace --graft`. But this can be done in subsequent patches
> I already started to work on.
>
>  builtin/replace.c  |  9 ++++++---
>  t/t6050-replace.sh | 11 +++++++++++
>  2 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/replace.c b/builtin/replace.c
> index f5701629a8..b0a9227f9a 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -370,16 +370,19 @@ static int replace_parents(struct strbuf *buf, int argc, const char **argv)
>  	/* prepare new parents */
>  	for (i = 0; i < argc; i++) {
>  		struct object_id oid;
> +		struct commit *commit;
> +
>  		if (get_oid(argv[i], &oid) < 0) {
>  			strbuf_release(&new_parents);
>  			return error(_("not a valid object name: '%s'"),
>  				     argv[i]);
>  		}
> -		if (!lookup_commit_reference(the_repository, &oid)) {
> +		commit = lookup_commit_reference(the_repository, &oid);
> +		if (!commit) {
>  			strbuf_release(&new_parents);
> -			return error(_("could not parse %s"), argv[i]);
> +			return error(_("could not parse %s as a commit"), argv[i]);

Good. After I read the commit message above, I was curious to see how
this worked if you provided an annotated tag that pointed to a
non-commit.

'lookup_commit_reference' should return NULL in that case, which you
handle here appropriately. (And thanks for improving the error message,
too ;-).)

>  		}
> -		strbuf_addf(&new_parents, "parent %s\n", oid_to_hex(&oid));
> +		strbuf_addf(&new_parents, "parent %s\n", oid_to_hex(&commit->object.oid));
>  	}
>
>  	/* replace existing parents with new ones */
> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
> index 5cb8281bab..72075983ac 100755
> --- a/t/t6050-replace.sh
> +++ b/t/t6050-replace.sh
> @@ -405,6 +405,17 @@ test_expect_success '--graft with and without already replaced object' '
>  	git replace -d $HASH5
>  '
>
> +test_expect_success '--graft using a tag as the new parent' '
> +	git tag new_parent $HASH5 &&
> +	git replace --graft $HASH7 new_parent &&
> +	commit_has_parents $HASH7 $HASH5 &&
> +	git replace -d $HASH7 &&
> +	git tag -a -m "annotated new parent tag" annotated_new_parent $HASH5 &&
> +	git replace --graft $HASH7 annotated_new_parent &&
> +	commit_has_parents $HASH7 $HASH5 &&
> +	git replace -d $HASH7

Very nice. I would normally write at the beginning of the test instead:

  test_when_finished "git replace -d $HASH7"

But I think your choice here is much better, since you need to delete
the replacement a few lines above, too. I think it would be jarring to
have both the inline `git replace -d`, and another from
`test_when_finished`, so this makes much more sense.

> +'
> +
>  test_expect_success GPG 'set up a signed commit' '
>  	echo "line 17" >>hello &&
>  	echo "line 18" >>hello &&
> --
> 2.21.0.68.gd997bba285.dirty
>

The previous two look good as well.

Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks,
Taylor

Re: [PATCH 2/3] t6050: redirect expected error output to /dev/null

[Eric Sunshine]

On Thu, Mar 28, 2019 at 1:17 PM Christian Couder
<christian.couder@gmail.com> wrote:
> Otherwise the error from `git rev-parse` is uselessly
> polluting the debug output.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
> @@ -40,7 +40,7 @@ commit_peeling_shows_parents ()
> -    test_must_fail git rev-parse --verify $_commit^$_parent_number
> +    test_must_fail git rev-parse --verify $_commit^$_parent_number 2>/dev/null

When I'm debugging test failures, I find it very helpful to see that a
command which was expected to fail did indeed fail with the expected
error message, so I don't consider such a message as "uselessly
polluting the debug output". Consequently, I'm rather negative on this
change.

Re: [PATCH 2/3] t6050: redirect expected error output to /dev/null

[Ævar Arnfjörð Bjarmason]

On Thu, Mar 28 2019, Eric Sunshine wrote:

> On Thu, Mar 28, 2019 at 1:17 PM Christian Couder
> <christian.couder@gmail.com> wrote:
>> Otherwise the error from `git rev-parse` is uselessly
>> polluting the debug output.
>>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
>> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
>> @@ -40,7 +40,7 @@ commit_peeling_shows_parents ()
>> -    test_must_fail git rev-parse --verify $_commit^$_parent_number
>> +    test_must_fail git rev-parse --verify $_commit^$_parent_number 2>/dev/null
>
> When I'm debugging test failures, I find it very helpful to see that a
> command which was expected to fail did indeed fail with the expected
> error message, so I don't consider such a message as "uselessly
> polluting the debug output". Consequently, I'm rather negative on this
> change.

I believe this instead should make both of you happy:

    diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
    index d638119750..2e3009f3cb 100755
    --- a/t/t6050-replace.sh
    +++ b/t/t6050-replace.sh
    @@ -40,7 +40,8 @@ commit_peeling_shows_parents ()
            test "$_found" = "$_parent" || return 1
            _parent_number=$(( $_parent_number + 1 ))
         done &&
    -    test_must_fail git rev-parse --verify $_commit^$_parent_number
    +    test_must_fail git rev-parse --verify $_commit^$_parent_number 2>err &&
    +    test_i18ngrep "Needed a single revision" err
     }

     commit_has_parents ()

Note that the file has existing 4-space-instead-of-tabs issues.