{"thread":{"id":"60286","subject":"[PATCH] diff: fix --merge-base with annotated tags","startedAt":"2023-10-01T15:19:21Z","lastAt":"2023-10-02T18:55:03Z","messageCount":2,"participants":["Alyssa Ross","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"482473","messageId":"20231001151845.3621551-1-hi@alyssa.is","threadId":"60286","inReplyTo":null,"subject":"[PATCH] diff: fix --merge-base with annotated tags","fromName":"Alyssa Ross","fromEmail":"hi@alyssa.is","sentAt":"2023-10-01T15:18:45Z","receivedAt":"2023-10-01T15:19:21Z","isPatch":true,"sender":{"key":"hi@alyssa.is","avatar":"https://avatars.githubusercontent.com/u/2768870?v=4"},"body":"Checking early for OBJ_COMMIT excludes other objects that can be\nresolved to commits, like annotated tags.  If we remove it, annotated\ntags will be resolved and handled just fine by\nlookup_commit_reference(), and if we are given something that can't be\nresolved to a commit, we'll still get a useful error message, e.g.:\n\n> error: object 21ab162211ac3ef13c37603ca88b27e9c7e0d40b is a tree, not a commit\n> fatal: no merge base found\n\nSigned-off-by: Alyssa Ross <hi@alyssa.is>\n---\n diff-lib.c                           |  2 --\n t/t4068-diff-symmetric-merge-base.sh | 13 ++++++++++---\n 2 files changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 6b0c6a7180..543398b4d8 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -572,8 +572,6 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)\n \t\tstruct object *obj = revs->pending.objects[i].item;\n \t\tif (obj->flags)\n \t\t\tdie(_(\"--merge-base does not work with ranges\"));\n-\t\tif (obj->type != OBJ_COMMIT)\n-\t\t\tdie(_(\"--merge-base only works with commits\"));\n \t}\n \n \t/*\ndiff --git a/t/t4068-diff-symmetric-merge-base.sh b/t/t4068-diff-symmetric-merge-base.sh\nindex 2d650d8f10..541642650f 100755\n--- a/t/t4068-diff-symmetric-merge-base.sh\n+++ b/t/t4068-diff-symmetric-merge-base.sh\n@@ -34,7 +34,7 @@ test_expect_success setup '\n \techo c >c &&\n \tgit add c &&\n \tgit commit -m C &&\n-\tgit tag commit-C &&\n+\tgit tag -m commit-C commit-C &&\n \tgit merge -m D main &&\n \tgit tag commit-D &&\n \tgit checkout main &&\n@@ -109,6 +109,13 @@ do\n \t\ttest_cmp expect actual\n \t'\n \n+\ttest_expect_success \"$cmd --merge-base with annotated tag\" '\n+\t\tgit checkout main &&\n+\t\tgit $cmd commit-C >expect &&\n+\t\tgit $cmd --merge-base commit-C >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+\n \ttest_expect_success \"$cmd --merge-base with one commit and unstaged changes\" '\n \t\tgit checkout main &&\n \t\ttest_when_finished git reset --hard &&\n@@ -143,7 +150,7 @@ do\n \ttest_expect_success \"$cmd --merge-base with non-commit\" '\n \t\tgit checkout main &&\n \t\ttest_must_fail git $cmd --merge-base main^{tree} 2>err &&\n-\t\ttest_i18ngrep \"fatal: --merge-base only works with commits\" err\n+\t\ttest_i18ngrep \"is a tree, not a commit\" err\n \t'\n \n \ttest_expect_success \"$cmd --merge-base with no merge bases and one commit\" '\n@@ -169,7 +176,7 @@ do\n \n \ttest_expect_success \"$cmd --merge-base commit and non-commit\" '\n \t\ttest_must_fail git $cmd --merge-base br2 main^{tree} 2>err &&\n-\t\ttest_i18ngrep \"fatal: --merge-base only works with commits\" err\n+\t\ttest_i18ngrep \"is a tree, not a commit\" err\n \t'\n \n \ttest_expect_success \"$cmd --merge-base with no merge bases and two commits\" '\n\nbase-commit: 43c8a30d150ecede9709c1f2527c8fba92c65f40\n-- \n2.42.0\n\n"},{"id":"482542","messageId":"xmqq4jj84zyb.fsf@gitster.g","threadId":"60286","inReplyTo":"20231001151845.3621551-1-hi@alyssa.is","subject":"Re: [PATCH] diff: fix --merge-base with annotated tags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-02T18:54:52Z","receivedAt":"2023-10-02T18:55:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alyssa Ross <hi@alyssa.is> writes:\n\n> Checking early for OBJ_COMMIT excludes other objects that can be\n> resolved to commits, like annotated tags.  If we remove it, annotated\n> tags will be resolved and handled just fine by\n> lookup_commit_reference(), and if we are given something that can't be\n> resolved to a commit, we'll still get a useful error message, e.g.:\n>\n>> error: object 21ab162211ac3ef13c37603ca88b27e9c7e0d40b is a tree, not a commit\n>> fatal: no merge base found\n\nInteresting.  0f5a1d44 (builtin/diff-index: learn --merge-base,\n2020-09-20) claims that it took inspiration from \"git diff A...B\"\nbut forgot that it needs to accept any commit-ish.\n\nWith a devil's advocate hat on, I have to wonder if it is really a\nuseful error message to spew a long hexadecimal string when the user\nwould certainly have gave a more mnemonic HEAD^{tree} or something,\nbut the original message does not say which command line argument it\ndid not like anyway, so the patch is a net improvement.\n\nWill queue.  Thanks.\n\n"}]}