{"thread":{"id":"50847","subject":"[PATCH v2 0/4] fix `git replace --graft` tag related issues","startedAt":"2019-03-31T13:47:21Z","lastAt":"2019-04-01T08:48:44Z","messageCount":6,"participants":["Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"372829","messageId":"20190331134659.28808-1-chriscool@tuxfamily.org","threadId":"50847","inReplyTo":null,"subject":"[PATCH v2 0/4] fix `git replace --graft` tag related issues","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-31T13:46:55Z","receivedAt":"2019-03-31T13:47:21Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This is version 2 of a small patch series to fix tag related issues in\n`git replace --graft` that Andreas Schwab reported in:\n\nhttps://public-inbox.org/git/mvmd0mcsjkf.fsf@suse.de/\n\nThe first version of this patch series was:\n\nhttps://public-inbox.org/git/20190328171722.9753-1-chriscool@tuxfamily.org/\n\nChanges compared to the previous version are the following:\n\n  - patch 4/4 has been added to fix issues when a tag is passed first\n\n  - in patch 2/4 the error message is redirected to a file instead of\n    /dev/null as suggested by Ævar after a comment by Eric\n\n  - title of patch 3/4 has been extended with \"as parent\" to make it\n    different from title of patch 4/4\n\nI saw that t6050 has existing 4-space-instead-of-tabs issues, but it\ncan perhaps be dealt with if some bigger changes are made to this test\nscript.\n\nThanks Taylor for the review, and Eric and Ævar for the\ncomments/suggestions!\n\nChristian Couder (4):\n  t6050: use test_line_count instead of wc -l\n  t6050: redirect expected error output to a file\n  replace: fix --graft when passing a tag as parent\n  replace: fix --graft when passing a tag first\n\n builtin/replace.c  | 20 +++++++++++++-------\n t/t6050-replace.sh | 31 ++++++++++++++++++++++++++++---\n 2 files changed, 41 insertions(+), 10 deletions(-)\n\n-- \n2.21.0.69.g2d86a08d09\n\n"},{"id":"372830","messageId":"20190331134659.28808-2-chriscool@tuxfamily.org","threadId":"50847","inReplyTo":"20190331134659.28808-1-chriscool@tuxfamily.org","subject":"[PATCH v2 1/4] t6050: use test_line_count instead of wc -l","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-31T13:46:56Z","receivedAt":"2019-03-31T13:47:32Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This modernizes a test and makes it more portable.\n\nReviewed-by: Taylor Blau <me@ttaylorr.com>\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n t/t6050-replace.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\nindex d638119750..41b177936e 100755\n--- a/t/t6050-replace.sh\n+++ b/t/t6050-replace.sh\n@@ -393,9 +393,11 @@ test_expect_success 'replace ref cleanup' '\n '\n \n test_expect_success '--graft with and without already replaced object' '\n-\ttest $(git log --oneline | wc -l) = 7 &&\n+\tgit log --oneline >log &&\n+\ttest_line_count = 7 log &&\n \tgit replace --graft $HASH5 &&\n-\ttest $(git log --oneline | wc -l) = 3 &&\n+\tgit log --oneline >log &&\n+\ttest_line_count = 3 log &&\n \tcommit_has_parents $HASH5 &&\n \ttest_must_fail git replace --graft $HASH5 $HASH4 $HASH3 &&\n \tgit replace --force -g $HASH5 $HASH4 $HASH3 &&\n-- \n2.21.0.69.g2d86a08d09\n\n"},{"id":"372831","messageId":"20190331134659.28808-4-chriscool@tuxfamily.org","threadId":"50847","inReplyTo":"20190331134659.28808-1-chriscool@tuxfamily.org","subject":"[PATCH v2 3/4] replace: fix --graft when passing a tag as parent","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-31T13:46:58Z","receivedAt":"2019-03-31T13:47:32Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"When passing a tag as a parent argument to `git replace --graft`,\nit can be useful to accept it and use the underlying commit as a\nparent.\n\nThis already works for lightweight tags, but unfortunately\nfor annotated tags we have been using the hash of the tag object\ninstead of the hash of the underlying commit as a parent in the\nreplacement object we create.\n\nThis created invalid objects, but the replace succeeded even if\nit showed an error like:\n\nerror: object A is a tag, not a commit\n\nThis patch fixes that by using the hash of the underlying commit\nwhen an annotated tag is passed.\n\nWhile at it, let's also update an error message to make it\nclearer.\n\nReviewed-by: Taylor Blau <me@ttaylorr.com>\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n builtin/replace.c  |  9 ++++++---\n t/t6050-replace.sh | 11 +++++++++++\n 2 files changed, 17 insertions(+), 3 deletions(-)\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, int argc, const char **argv)\n \t/* prepare new parents */\n \tfor (i = 0; i < argc; i++) {\n \t\tstruct object_id oid;\n+\t\tstruct commit *commit;\n+\n \t\tif (get_oid(argv[i], &oid) < 0) {\n \t\t\tstrbuf_release(&new_parents);\n \t\t\treturn error(_(\"not a valid object name: '%s'\"),\n \t\t\t\t     argv[i]);\n \t\t}\n-\t\tif (!lookup_commit_reference(the_repository, &oid)) {\n+\t\tcommit = lookup_commit_reference(the_repository, &oid);\n+\t\tif (!commit) {\n \t\t\tstrbuf_release(&new_parents);\n-\t\t\treturn error(_(\"could not parse %s\"), argv[i]);\n+\t\t\treturn error(_(\"could not parse %s as a commit\"), argv[i]);\n \t\t}\n-\t\tstrbuf_addf(&new_parents, \"parent %s\\n\", oid_to_hex(&oid));\n+\t\tstrbuf_addf(&new_parents, \"parent %s\\n\", oid_to_hex(&commit->object.oid));\n \t}\n \n \t/* replace existing parents with new ones */\ndiff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\nindex 948d278482..2385a60f68 100755\n--- a/t/t6050-replace.sh\n+++ b/t/t6050-replace.sh\n@@ -406,6 +406,17 @@ test_expect_success '--graft with and without already replaced object' '\n \tgit replace -d $HASH5\n '\n \n+test_expect_success '--graft using a tag as the new parent' '\n+\tgit tag new_parent $HASH5 &&\n+\tgit replace --graft $HASH7 new_parent &&\n+\tcommit_has_parents $HASH7 $HASH5 &&\n+\tgit replace -d $HASH7 &&\n+\tgit tag -a -m \"annotated new parent tag\" annotated_new_parent $HASH5 &&\n+\tgit replace --graft $HASH7 annotated_new_parent &&\n+\tcommit_has_parents $HASH7 $HASH5 &&\n+\tgit replace -d $HASH7\n+'\n+\n test_expect_success GPG 'set up a signed commit' '\n \techo \"line 17\" >>hello &&\n \techo \"line 18\" >>hello &&\n-- \n2.21.0.69.g2d86a08d09\n\n"},{"id":"372832","messageId":"20190331134659.28808-3-chriscool@tuxfamily.org","threadId":"50847","inReplyTo":"20190331134659.28808-1-chriscool@tuxfamily.org","subject":"[PATCH v2 2/4] t6050: redirect expected error output to a file","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-31T13:46:57Z","receivedAt":"2019-03-31T13:47:32Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Otherwise the error from `git rev-parse` is uselessly\npolluting the debug output.\n\nRedirecting to a file, instead of /dev/null, makes it\npossible to check that we got the error we expected, so\nlet's check that too.\n\nReviewed-by: Taylor Blau <me@ttaylorr.com>\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nHelped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n t/t6050-replace.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\nindex 41b177936e..948d278482 100755\n--- a/t/t6050-replace.sh\n+++ b/t/t6050-replace.sh\n@@ -40,7 +40,8 @@ commit_peeling_shows_parents ()\n \ttest \"$_found\" = \"$_parent\" || return 1\n \t_parent_number=$(( $_parent_number + 1 ))\n     done &&\n-    test_must_fail git rev-parse --verify $_commit^$_parent_number\n+    test_must_fail git rev-parse --verify $_commit^$_parent_number 2>err &&\n+    test_i18ngrep \"Needed a single revision\" err\n }\n \n commit_has_parents ()\n-- \n2.21.0.69.g2d86a08d09\n\n"},{"id":"372833","messageId":"20190331134659.28808-5-chriscool@tuxfamily.org","threadId":"50847","inReplyTo":"20190331134659.28808-1-chriscool@tuxfamily.org","subject":"[PATCH v2 4/4] replace: fix --graft when passing a tag first","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-31T13:46:59Z","receivedAt":"2019-03-31T13:47:34Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"When passing a tag as the first argument to `git replace --graft`,\nit can be useful to accept it and use the underlying commit as a\nthe commit that will be replaced.\n\nThis already works for lightweight tags, but unfortunately\nfor annotated tags we have been using the hash of the tag object\ninstead of the hash of the underlying commit.\n\nEspecially we would pass the hash of the tag object to\nreplace_object_oid() where we would likely fail with an error\nlike:\n\n\"error: Objects must be of the same type.\n'annotated_replaced_object' points to a replaced object of type 'tag'\nwhile 'replacement' points to a replacement object of type 'commit'.\"\n\nThis patch fixes that by using the hash of the underlying commit\nwhen an annotated tag is passed.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n builtin/replace.c  | 11 +++++++----\n t/t6050-replace.sh | 11 +++++++++++\n 2 files changed, 18 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex b0a9227f9a..644b21ca8d 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -481,15 +481,18 @@ static int create_graft(int argc, const char **argv, int force, int gentle)\n \n \tstrbuf_release(&buf);\n \n-\tif (oideq(&old_oid, &new_oid)) {\n+\tif (oideq(&commit->object.oid, &new_oid)) {\n \t\tif (gentle) {\n-\t\t\twarning(_(\"graft for '%s' unnecessary\"), oid_to_hex(&old_oid));\n+\t\t\twarning(_(\"graft for '%s' unnecessary\"),\n+\t\t\t\toid_to_hex(&commit->object.oid));\n \t\t\treturn 0;\n \t\t}\n-\t\treturn error(_(\"new commit is the same as the old one: '%s'\"), oid_to_hex(&old_oid));\n+\t\treturn error(_(\"new commit is the same as the old one: '%s'\"),\n+\t\t\t     oid_to_hex(&commit->object.oid));\n \t}\n \n-\treturn replace_object_oid(old_ref, &old_oid, \"replacement\", &new_oid, force);\n+\treturn replace_object_oid(old_ref, &commit->object.oid,\n+\t\t\t\t  \"replacement\", &new_oid, force);\n }\n \n static int convert_graft_file(int force)\ndiff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\nindex 2385a60f68..e7e64e085d 100755\n--- a/t/t6050-replace.sh\n+++ b/t/t6050-replace.sh\n@@ -417,6 +417,17 @@ test_expect_success '--graft using a tag as the new parent' '\n \tgit replace -d $HASH7\n '\n \n+test_expect_success '--graft using a tag as the replaced object' '\n+\tgit tag replaced_object $HASH7 &&\n+\tgit replace --graft replaced_object $HASH5 &&\n+\tcommit_has_parents $HASH7 $HASH5 &&\n+\tgit replace -d $HASH7 &&\n+\tgit tag -a -m \"annotated replaced object tag\" annotated_replaced_object $HASH7 &&\n+\tgit replace --graft annotated_replaced_object $HASH5 &&\n+\tcommit_has_parents $HASH7 $HASH5 &&\n+\tgit replace -d $HASH7\n+'\n+\n test_expect_success GPG 'set up a signed commit' '\n \techo \"line 17\" >>hello &&\n \techo \"line 18\" >>hello &&\n-- \n2.21.0.69.g2d86a08d09\n\n"},{"id":"372858","messageId":"xmqqmulaxg9k.fsf@gitster-ct.c.googlers.com","threadId":"50847","inReplyTo":"20190331134659.28808-5-chriscool@tuxfamily.org","subject":"Re: [PATCH v2 4/4] replace: fix --graft when passing a tag first","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-01T08:48:39Z","receivedAt":"2019-04-01T08:48:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> When passing a tag as the first argument to `git replace --graft`,\n> it can be useful to accept it and use the underlying commit as a\n> the commit that will be replaced.\n\nYeah, another plausible fix would be to make it an error, but the\nmessage for the error would most likely tell the user to peel the\ntag, so peeling it ourselves would make sense.\n\n"}]}