{"thread":{"id":"50826","subject":"git replace --graft does error checking too late","startedAt":"2019-03-27T10:22:42Z","lastAt":"2019-03-28T07:35:28Z","messageCount":4,"participants":["Andreas Schwab","Christian Couder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"372564","messageId":"mvmd0mcsjkf.fsf@suse.de","threadId":"50826","inReplyTo":null,"subject":"git replace --graft does error checking too late","fromName":"Andreas Schwab","fromEmail":"schwab@suse.de","sentAt":"2019-03-27T10:22:40Z","receivedAt":"2019-03-27T10:22:42Z","isPatch":false,"sender":{"key":"schwab@suse.de","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"When running `git replace --graft A B' where B is a non-commit (eg. a\ntag) it displays an error, but creates the replace ref anyway. I think\nit should verify that B names a commit object before creating the ref.\n\nAndreas.\n\n-- \nAndreas Schwab, SUSE Labs, schwab@suse.de\nGPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7\n\"And now for something completely different.\"\n"},{"id":"372576","messageId":"CAP8UFD1qU_kJ97MdLFwzx+g3F6Q+fQ9LWOBxd=1m4vSi-fxF=Q@mail.gmail.com","threadId":"50826","inReplyTo":"mvmd0mcsjkf.fsf@suse.de","subject":"Re: git replace --graft does error checking too late","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-27T13:11:41Z","receivedAt":"2019-03-27T13:11:56Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Mar 27, 2019 at 11:24 AM Andreas Schwab wrote:\n>\n> When running `git replace --graft A B' where B is a non-commit (eg. a\n> tag) it displays an error,\n\nYeah, it seems that when A is a commit and B a tag I get:\n\n\"error: object A is a tag, not a commit\"\n\nwhich is wrong as A is a commit.\n\n> but creates the replace ref anyway. I think\n> it should verify that B names a commit object before creating the ref.\n\nAccepting a tag and using the commit the tag points to could be useful.\n\nFor example someone could look at the commit graph, then decide to tag\nthe commit(s) that should be used when replacing, and then use `git\nreplace --graft A ...`\nusing the created tag(s).\n\n(I checked the code in builtin/replace.c and it seems that we use\nlookup_commit_reference() on each of the new parents, so we should be\nsafe in case one of the given new parents cannot be peeled into a\ncommit.)\n\nSo it seems to me that the issue is that it shows a wrong error when\nit shouldn't show anything, or perhaps only a warning.\n"},{"id":"372577","messageId":"CAP8UFD111-zqa7Fhr1KnmWYAKpBN4ofqn0AvD_nP3s_WBFzLqw@mail.gmail.com","threadId":"50826","inReplyTo":"CAP8UFD1qU_kJ97MdLFwzx+g3F6Q+fQ9LWOBxd=1m4vSi-fxF=Q@mail.gmail.com","subject":"Re: git replace --graft does error checking too late","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-27T13:21:23Z","receivedAt":"2019-03-27T13:21:37Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Mar 27, 2019 at 2:11 PM Christian Couder\n wrote:\n>\n> On Wed, Mar 27, 2019 at 11:24 AM Andreas Schwab wrote:\n> >\n> > When running `git replace --graft A B' where B is a non-commit (eg. a\n> > tag) it displays an error,\n>\n> Yeah, it seems that when A is a commit and B a tag I get:\n>\n> \"error: object A is a tag, not a commit\"\n>\n> which is wrong as A is a commit.\n\nActually I get the above only if A is a commit but there is a tag\npointing to it. If there is no tag pointing to it I get:\n\nerror: object C is a tag, not a commit\n\nwhere C is the hash of the tag object B (C=$(git rev-parse B))\n\nSo yeah, this is weird.\n"},{"id":"372607","messageId":"CAP8UFD2yMOLKOdcXnPsCvAzymjLJX2A4U2DHfq1JjvZnQj9pLQ@mail.gmail.com","threadId":"50826","inReplyTo":"CAP8UFD111-zqa7Fhr1KnmWYAKpBN4ofqn0AvD_nP3s_WBFzLqw@mail.gmail.com","subject":"Re: git replace --graft does error checking too late","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-28T07:35:13Z","receivedAt":"2019-03-28T07:35:28Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Mar 27, 2019 at 2:21 PM Christian Couder\n wrote:\n>\n> On Wed, Mar 27, 2019 at 2:11 PM Christian Couder\n> wrote:\n> >\n> > On Wed, Mar 27, 2019 at 11:24 AM Andreas Schwab wrote:\n> > >\n> > > When running `git replace --graft A B' where B is a non-commit (eg. a\n> > > tag) it displays an error,\n> >\n> > Yeah, it seems that when A is a commit and B a tag I get:\n> >\n> > \"error: object A is a tag, not a commit\"\n> >\n> > which is wrong as A is a commit.\n>\n> Actually I get the above only if A is a commit but there is a tag\n> pointing to it. If there is no tag pointing to it I get:\n>\n> error: object C is a tag, not a commit\n>\n> where C is the hash of the tag object B (C=$(git rev-parse B))\n>\n> So yeah, this is weird.\n\nI think I understand what happens. It seems that we put the SHA-1 of\nthe tag in the new object we create instead of the SHA-1 of the\nunderlying commit and that's what generates the error message and\nother issues later.\n\nThe following patch (that unfortunately Gmail is likely to muck)\nshould fix at least part of the issue. I will send a real patch with\ntests later.\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex f5701629a8..b0a9227f9a 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -370,16 +370,19 @@ static int replace_parents(struct strbuf *buf,\nint argc, const char **argv)\n /* prepare new parents */\n for (i = 0; i < argc; i++) {\n struct object_id oid;\n+ struct commit *commit;\n+\n if (get_oid(argv[i], &oid) < 0) {\n strbuf_release(&new_parents);\n return error(_(\"not a valid object name: '%s'\"),\n argv[i]);\n }\n- if (!lookup_commit_reference(the_repository, &oid)) {\n+ commit = lookup_commit_reference(the_repository, &oid);\n+ if (!commit) {\n strbuf_release(&new_parents);\n- return error(_(\"could not parse %s\"), argv[i]);\n+ return error(_(\"could not parse %s as a\ncommit\"), argv[i]);\n }\n- strbuf_addf(&new_parents, \"parent %s\\n\", oid_to_hex(&oid));\n+ strbuf_addf(&new_parents, \"parent %s\\n\",\noid_to_hex(&commit->object.oid));\n }\n\n /* replace existing parents with new ones */\n"}]}