{"thread":{"id":"49641","subject":"[PATCH v2 0/3] Fix rebase autostash","startedAt":"2018-10-22T22:15:04Z","lastAt":"2018-10-23T17:07:44Z","messageCount":9,"participants":["Johannes Schindelin via GitGitGadget","Eric Sunshine","SZEDER Gábor","Junio C Hamano","Johannes Schindelin","Alban Gruin"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"361217","messageId":"pull.52.v2.git.gitgitgadget@gmail.com","threadId":"49641","inReplyTo":"pull.52.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] Fix rebase autostash","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-22T22:15:00Z","receivedAt":"2018-10-22T22:15:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Gábor reported in \nhttps://public-inbox.org/git/20181019124625.GB30222@szeder.dev/ that \nt5520-pull.sh fails from time to time, and Alban root-caused this to a bug\nin the built-in rebase.\n\nThis patch series fixes that, and while at it also fixes an oversight of\nyours truly when helping Pratik with his GSoC project, and it also adds a\nchange on top that makes really, really certain that git stash apply \ninterprets the OID passed to it correctly (as opposed to an insanely large\nnumber for the stash reflog).\n\nPlease note that I based these patches on top of next (they might be most\nappropriately applied on top of rebase-in-c-6-final, though).\n\n(Sorry for the v2, v1 did not send due to an UTF-8 character in the Cc:\nlist, a bug that still needs to be fixed in GitGitGadget.)\n\nJohannes Schindelin (3):\n rebase (autostash): avoid duplicate call to state_dir_path()\n rebase (autostash): store the full OID in /autostash\n rebase (autostash): use an explicit OID to apply the stash\n\n builtin/rebase.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\n\nbase-commit: 500967bb5e73b67708a64a4360867cf2407ba880\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-52%2Fdscho%2Ffix-rebase-autostash-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-52/dscho/fix-rebase-autostash-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/52\n\nRange-diff vs v1:\n\n 1: 88241ad32 = 1: 88241ad32 rebase (autostash): avoid duplicate call to state_dir_path()\n 2: 86107a6d0 = 2: 86107a6d0 rebase (autostash): store the full OID in /autostash\n 3: d3b47a4c1 = 3: 07140a71d rebase (autostash): use an explicit OID to apply the stash\n\n-- \ngitgitgadget\n"},{"id":"361218","messageId":"07140a71dd9ed3f709970f0ce5eb6aa014417b25.1540246499.git.gitgitgadget@gmail.com","threadId":"49641","inReplyTo":"pull.52.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] rebase (autostash): use an explicit OID to apply the stash","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-22T22:15:05Z","receivedAt":"2018-10-22T22:15:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen `git stash apply ` sees an argument that consists only of\ndigits, it tries to be smart and interpret it as `stash@{}`.\n\nUnfortunately, an all-digit hash (which is unlikely but still possible)\nis therefore misinterpreted as `stash@{}` reflog.\n\nTo prevent that from happening, let's append `^0` after the stash hash,\nto make sure that it is interpreted as an OID rather than as a number.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rebase.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 418624837..30d58118c 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -253,6 +253,8 @@ static int apply_autostash(struct rebase_options *opts)\n \n \tif (read_one(path, &autostash))\n \t\treturn error(_(\"Could not read '%s'\"), path);\n+\t/* Ensure that the hash is not mistake for a number */\n+\tstrbuf_addstr(&autostash, \"^0\");\n \targv_array_pushl(&stash_apply.args,\n \t\t\t \"stash\", \"apply\", autostash.buf, NULL);\n \tstash_apply.git_cmd = 1;\n-- \ngitgitgadget\n"},{"id":"361219","messageId":"86107a6d0a044e3338e6f70313917066d200a1f4.1540246499.git.gitgitgadget@gmail.com","threadId":"49641","inReplyTo":"pull.52.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] rebase (autostash): store the full OID in /autostash","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-22T22:15:03Z","receivedAt":"2018-10-22T22:15:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIt was reported by Gábor Szeder and analyzed by Alban Gruin that the\nbuilt-in rebase stores only abbreviated stash hashes in the `autostash`\nfile.\n\nThis is problematic e.g. in t5520-pull.sh, where the abbreviated hash is\nso short that it sometimes consists only of digits, which are\nsubsequently mistaken (\"DWIMmed\") for numbers by `git stash apply`.\n\nLet's align the behavior of the built-in rebase with the scripted rebase\nand store the full stash hash instead. That makes it a lot less likely\nthat it consists only of digits.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rebase.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex d21504d9f..418624837 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1375,7 +1375,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tif (safe_create_leading_directories_const(autostash))\n \t\t\t\tdie(_(\"Could not create directory for '%s'\"),\n \t\t\t\t options.state_dir);\n-\t\t\twrite_file(autostash, \"%s\", buf.buf);\n+\t\t\twrite_file(autostash, \"%s\", oid_to_hex(&oid));\n \t\t\tprintf(_(\"Created autostash: %s\\n\"), buf.buf);\n \t\t\tif (reset_head(&head->object.oid, \"reset --hard\",\n \t\t\t\t NULL, 0, NULL, NULL) < 0)\n-- \ngitgitgadget\n\n"},{"id":"361220","messageId":"88241ad327ac42b94046a7e7ca208c84fd9f4578.1540246499.git.gitgitgadget@gmail.com","threadId":"49641","inReplyTo":"pull.52.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] rebase (autostash): avoid duplicate call to state_dir_path()","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-22T22:15:02Z","receivedAt":"2018-10-22T22:15:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWe already called that function at this point, and stored the result in\nthe `path` variable. We might just as well use it ;-)\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rebase.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 313a8263d..d21504d9f 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -251,7 +251,7 @@ static int apply_autostash(struct rebase_options *opts)\n \tif (!file_exists(path))\n \t\treturn 0;\n \n-\tif (read_one(state_dir_path(\"autostash\", opts), &autostash))\n+\tif (read_one(path, &autostash))\n \t\treturn error(_(\"Could not read '%s'\"), path);\n \targv_array_pushl(&stash_apply.args,\n \t\t\t \"stash\", \"apply\", autostash.buf, NULL);\n-- \ngitgitgadget\n\n"},{"id":"361221","messageId":"CAPig+cSgRttNJTScnYaE6G2kgpFzwYMnroH=bCKtc0a2Y5CiBQ@mail.gmail.com","threadId":"49641","inReplyTo":"07140a71dd9ed3f709970f0ce5eb6aa014417b25.1540246499.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/3] rebase (autostash): use an explicit OID to apply the stash","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-10-22T22:25:24Z","receivedAt":"2018-10-22T22:25:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Oct 22, 2018 at 6:15 PM Johannes Schindelin via GitGitGadget\n wrote:\n> When `git stash apply ` sees an argument that consists only of\n> digits, it tries to be smart and interpret it as `stash@{}`.\n>\n> Unfortunately, an all-digit hash (which is unlikely but still possible)\n> is therefore misinterpreted as `stash@{}` reflog.\n>\n> To prevent that from happening, let's append `^0` after the stash hash,\n> to make sure that it is interpreted as an OID rather than as a number.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> @@ -253,6 +253,8 @@ static int apply_autostash(struct rebase_options *opts)\n>\n> if (read_one(path, &autostash))\n> return error(_(\"Could not read '%s'\"), path);\n> + /* Ensure that the hash is not mistake for a number */\n\ns/mistake/mistaken/\n"},{"id":"361223","messageId":"20181022223256.GI30222@szeder.dev","threadId":"49641","inReplyTo":"07140a71dd9ed3f709970f0ce5eb6aa014417b25.1540246499.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/3] rebase (autostash): use an explicit OID to apply the stash","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-22T22:32:56Z","receivedAt":"2018-10-22T22:33:03Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Oct 22, 2018 at 03:15:05PM -0700, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> When `git stash apply ` sees an argument that consists only of\n> digits, it tries to be smart and interpret it as `stash@{}`.\n> \n> Unfortunately, an all-digit hash (which is unlikely but still possible)\n> is therefore misinterpreted as `stash@{}` reflog.\n> \n> To prevent that from happening, let's append `^0` after the stash hash,\n> to make sure that it is interpreted as an OID rather than as a number.\n\nOh, this is clever.\n\nFWIW, all patches look good to me, barring the typo below.\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> builtin/rebase.c | 2 ++\n> 1 file changed, 2 insertions(+)\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 418624837..30d58118c 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -253,6 +253,8 @@ static int apply_autostash(struct rebase_options *opts)\n> \n> \tif (read_one(path, &autostash))\n> \t\treturn error(_(\"Could not read '%s'\"), path);\n> +\t/* Ensure that the hash is not mistake for a number */\n\ns/mistake/mistaken/\n\n> +\tstrbuf_addstr(&autostash, \"^0\");\n> \targv_array_pushl(&stash_apply.args,\n> \t\t\t \"stash\", \"apply\", autostash.buf, NULL);\n> \tstash_apply.git_cmd = 1;\n> -- \n> gitgitgadget\n"},{"id":"361256","messageId":"xmqqzhv5s3lf.fsf@gitster-ct.c.googlers.com","threadId":"49641","inReplyTo":"20181022223256.GI30222@szeder.dev","subject":"Re: [PATCH v2 3/3] rebase (autostash): use an explicit OID to apply the stash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-23T04:13:00Z","receivedAt":"2018-10-23T04:13:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n>> To prevent that from happening, let's append `^0` after the stash hash,\n>> to make sure that it is interpreted as an OID rather than as a number.\n>\n> Oh, this is clever.\n\nYeah, we can do this as we know we'd be dealing with a commit-ish.\nIf we made a mistake to use a tree object to represent a stash, this\ntrick wouldn't have been possible ;-)\n\n> FWIW, all patches look good to me, barring the typo below.\n> ...\n>> +\t/* Ensure that the hash is not mistake for a number */\n>\n> s/mistake/mistaken/\n\nWill squash this in and add your reviewed-by before queuing.\n\nThanks, both.\n"},{"id":"361264","messageId":"nycvar.QRO.7.76.6.1810231134020.4546@tvgsbejvaqbjf.bet","threadId":"49641","inReplyTo":"xmqqzhv5s3lf.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 3/3] rebase (autostash): use an explicit OID to apply the stash","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-10-23T09:34:16Z","receivedAt":"2018-10-23T09:34:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 23 Oct 2018, Junio C Hamano wrote:\n\n> SZEDER Gábor writes:\n> \n> >> To prevent that from happening, let's append `^0` after the stash hash,\n> >> to make sure that it is interpreted as an OID rather than as a number.\n> >\n> > Oh, this is clever.\n> \n> Yeah, we can do this as we know we'd be dealing with a commit-ish.\n> If we made a mistake to use a tree object to represent a stash, this\n> trick wouldn't have been possible ;-)\n> \n> > FWIW, all patches look good to me, barring the typo below.\n> > ...\n> >> +\t/* Ensure that the hash is not mistake for a number */\n> >\n> > s/mistake/mistaken/\n> \n> Will squash this in and add your reviewed-by before queuing.\n> \n> Thanks, both.\n\nThank you all!\nDscho"},{"id":"361300","messageId":"9dab8e05-6520-0dc1-e08f-07236e65f25f@gmail.com","threadId":"49641","inReplyTo":"07140a71dd9ed3f709970f0ce5eb6aa014417b25.1540246499.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/3] rebase (autostash): use an explicit OID to apply the stash","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-10-23T17:07:24Z","receivedAt":"2018-10-23T17:07:44Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi Johannes,\n\nthis looks good to me, too!\n\nCheers,\nAlban\n\n"}]}