Re: [PATCH 3/3] replace: fix --graft when passing a tag
Hi Christian,
On Thu, Mar 28, 2019 at 06:17:22PM +0100, Christian Couder wrote:
Show 8 quoted lines
> 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.
Show 7 quoted lines
> 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.
Show 36 quoted lines
> 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 ;-).)
Show 23 quoted lines
> }
> - 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.
Show 8 quoted lines
> +'
> +
> 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
Christian Couder
