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

[Taylor Blau <me@ttaylorr.com>]

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