Re: [PATCH 3/3] replace: fix --graft when passing a tag
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Mar 28, 2019, 18:38 UTC
- Message-ID
- <20190328183812.GA24326@Taylors-MacBook-Pro.local>
- In-Reply-To
- <20190328171722.9753-3-chriscool@tuxfamily.org>
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