{"thread":{"id":"51788","subject":"[PATCH 0/1] Make git rebase -r's label generation more resilient","startedAt":"2019-09-02T14:01:44Z","lastAt":"2019-11-21T00:16:15Z","messageCount":22,"participants":["Johannes Schindelin via GitGitGadget","Matt R via GitGitGadget","Phillip Wood","Junio C Hamano","Johannes Schindelin","brian m. carlson","Philip Oakley","Matt Rogers","Matthew Rogers via GitGitGadget","Danh Doan","Doan Tran Cong Danh"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"381648","messageId":"pull.327.git.gitgitgadget@gmail.com","threadId":"51788","inReplyTo":null,"subject":"[PATCH 0/1] Make git rebase -r's label generation more resilient","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-09-02T14:01:40Z","receivedAt":"2019-09-02T14:01:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Those labels must be valid ref names, and therefore valid file names.\n\nThis just came in via Git for Windows.\n\nMatt R (1):\n rebase -r: let `label` generate safer labels\n\n sequencer.c | 12 +++++++++++-\n t/t3430-rebase-merges.sh | 5 +++++\n 2 files changed, 16 insertions(+), 1 deletion(-)\n\n\nbase-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-327%2Fdscho%2Ffix-rebase-r-labels-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-327/dscho/fix-rebase-r-labels-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/327\n-- \ngitgitgadget\n"},{"id":"381649","messageId":"4a02c38442dd8a4c0381adc8db0dce81c253da09.1567432900.git.gitgitgadget@gmail.com","threadId":"51788","inReplyTo":"pull.327.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] rebase -r: let `label` generate safer labels","fromName":"Matt R via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-09-02T14:01:41Z","receivedAt":"2019-09-02T14:01:46Z","isPatch":true,"sender":{"key":"name:Matt R","avatar":null},"body":"From: Matt R \n\nThe `label` todo command in interactive rebases creates temporary refs\nin the `refs/rewritten/` namespace. These refs are stored as loose refs,\ni.e. as files in `.git/refs/rewritten/`, therefore they have to conform\nwith file name limitations on the current filesystem.\n\nThis poses a problem in particular on NTFS/FAT, where e.g. the colon\ncharacter is not a valid part of a file name.\n\nLet's safeguard against this by replacing not only white-space\ncharacters by dashes, but all non-alpha-numeric ones.\n\nHowever, we exempt non-ASCII UTF-8 characters from that, as it should be\nquite possible to reflect branch names such as `↯↯↯` in refs/file names.\n\nSigned-off-by: Matthew Rogers \nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 12 +++++++++++-\n t/t3430-rebase-merges.sh | 5 +++++\n 2 files changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 34ebf8ed94..23f4a0876a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4635,8 +4635,18 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \t\telse\n \t\t\tstrbuf_addbuf(&label, &oneline);\n \n+\t\t/*\n+\t\t * Sanitize labels by replacing non-alpha-numeric characters\n+\t\t * (including white-space ones) by dashes, as they might be\n+\t\t * illegal in file names (and hence in ref names).\n+\t\t *\n+\t\t * Note that we retain non-ASCII UTF-8 characters (identified\n+\t\t * via the most significant bit). They should be all acceptable\n+\t\t * in file names. We do not validate the UTF-8 here, that's not\n+\t\t * the job of this function.\n+\t\t */\n \t\tfor (p1 = label.buf; *p1; p1++)\n-\t\t\tif (isspace(*p1))\n+\t\t\tif (!(*p1 & 0x80) && !isalnum(*p1))\n \t\t\t\t*(char *)p1 = '-';\n \n \t\tstrbuf_reset(&buf);\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex 7b6c4847ad..737396f944 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -441,4 +441,9 @@ test_expect_success '--continue after resolving conflicts after a merge' '\n \ttest_path_is_missing .git/MERGE_HEAD\n '\n \n+test_expect_success '--rebase-merges with commit that can generate bad characters for filename' '\n+\tgit checkout -b colon-in-label E &&\n+\tgit merge -m \"colon: this should work\" G &&\n+\tgit rebase --rebase-merges --force-rebase E\n+'\n test_done\n-- \ngitgitgadget\n"},{"id":"381664","messageId":"444f3ec4-abdf-1aa9-e8a8-8b5346b939e8@gmail.com","threadId":"51788","inReplyTo":"4a02c38442dd8a4c0381adc8db0dce81c253da09.1567432900.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] rebase -r: let `label` generate safer labels","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-09-02T17:57:55Z","receivedAt":"2019-09-02T17:58:01Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Matt\n\nThis is definitely worth fixing, I've got a couple of comments below\n\nOn 02/09/2019 15:01, Matt R via GitGitGadget wrote:\n> From: Matt R \n> \n> The `label` todo command in interactive rebases creates temporary refs\n> in the `refs/rewritten/` namespace. These refs are stored as loose refs,\n> i.e. as files in `.git/refs/rewritten/`, therefore they have to conform\n> with file name limitations on the current filesystem.\n> \n> This poses a problem in particular on NTFS/FAT, where e.g. the colon\n> character is not a valid part of a file name.\n\nBeing picking I'll point out that ':' is not a valid in refs either. \nLooking at \nhttps://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file I \nthink only \" and | are not allowed on NTFS/FAT but are valid in refs \n(see the man page for git check-ref-format for all the details). So the \nmain limitation is actually what git allows in refs.\n\n> Let's safeguard against this by replacing not only white-space\n> characters by dashes, but all non-alpha-numeric ones.\n> \n> However, we exempt non-ASCII UTF-8 characters from that, as it should be\n> quite possible to reflect branch names such as `↯↯↯` in refs/file names.\n> \n> Signed-off-by: Matthew Rogers \n> Signed-off-by: Johannes Schindelin \n> ---\n> sequencer.c | 12 +++++++++++-\n> t/t3430-rebase-merges.sh | 5 +++++\n> 2 files changed, 16 insertions(+), 1 deletion(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 34ebf8ed94..23f4a0876a 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -4635,8 +4635,18 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n> \t\telse\n> \t\t\tstrbuf_addbuf(&label, &oneline);\n> \n> +\t\t/*\n> +\t\t * Sanitize labels by replacing non-alpha-numeric characters\n> +\t\t * (including white-space ones) by dashes, as they might be\n> +\t\t * illegal in file names (and hence in ref names).\n> +\t\t *\n> +\t\t * Note that we retain non-ASCII UTF-8 characters (identified\n> +\t\t * via the most significant bit). They should be all acceptable\n> +\t\t * in file names. We do not validate the UTF-8 here, that's not\n> +\t\t * the job of this function.\n> +\t\t */\n> \t\tfor (p1 = label.buf; *p1; p1++)\n> -\t\t\tif (isspace(*p1))\n> +\t\t\tif (!(*p1 & 0x80) && !isalnum(*p1))\n> \t\t\t\t*(char *)p1 = '-';\n\nI'm sightly concerned that this opens the possibility for unexpected \neffects if two different labels get sanitized to the same string. I \nsuspect it's unlikely to happen in practice but doing something like \npercent encoding non-alphanumeric characters would avoid the problem \nentirely.\n\nBest Wishes\n\nPhillip\n\n> \t\tstrbuf_reset(&buf);\n> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\n> index 7b6c4847ad..737396f944 100755\n> --- a/t/t3430-rebase-merges.sh\n> +++ b/t/t3430-rebase-merges.sh\n> @@ -441,4 +441,9 @@ test_expect_success '--continue after resolving conflicts after a merge' '\n> \ttest_path_is_missing .git/MERGE_HEAD\n> '\n> \n> +test_expect_success '--rebase-merges with commit that can generate bad characters for filename' '\n> +\tgit checkout -b colon-in-label E &&\n> +\tgit merge -m \"colon: this should work\" G &&\n> +\tgit rebase --rebase-merges --force-rebase E\n> +'\n> test_done\n> \n"},{"id":"381672","messageId":"xmqq5zmav9ej.fsf@gitster-ct.c.googlers.com","threadId":"51788","inReplyTo":"444f3ec4-abdf-1aa9-e8a8-8b5346b939e8@gmail.com","subject":"Re: [PATCH 1/1] rebase -r: let `label` generate safer labels","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-02T18:29:56Z","receivedAt":"2019-09-02T18:30:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Being picking I'll point out that ':' is not a valid in refs\n> either. Looking at\n> https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file I\n> think only \" and | are not allowed on NTFS/FAT but are valid in refs\n> (see the man page for git check-ref-format for all the details). So\n> the main limitation is actually what git allows in refs.\n\nYeah, trying to use the contents of the log message without\nsufficient sanitization is looking for trouble.\n\n>> \t\tfor (p1 = label.buf; *p1; p1++)\n>> -\t\t\tif (isspace(*p1))\n>> +\t\t\tif (!(*p1 & 0x80) && !isalnum(*p1))\n>> \t\t\t\t*(char *)p1 = '-';\n>\n> I'm sightly concerned that this opens the possibility for unexpected\n> effects if two different labels get sanitized to the same string. I\n> suspect it's unlikely to happen in practice but doing something like\n> percent encoding non-alphanumeric characters would avoid the problem\n> entirely.\n\nI'd rather see 'x' used instead of '-' (double-or-more dashes and\nleading dash in refnames may currently be allowed but double-or-more\nexes and leading ex would be much more likely to stay valid) if we\njust want to redact invalid characters.\n\nI see there are \"lets make sure it is unique by suffixing \"-%d\" in\nother codepaths; would that help if this piece of code yields a\nlabel that is not unique?\n"},{"id":"381684","messageId":"nycvar.QRO.7.76.6.1909022124350.46@tvgsbejvaqbjf.bet","threadId":"51788","inReplyTo":"444f3ec4-abdf-1aa9-e8a8-8b5346b939e8@gmail.com","subject":"Re: [PATCH 1/1] rebase -r: let `label` generate safer labels","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-09-02T19:42:18Z","receivedAt":"2019-09-02T19:42:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Mon, 2 Sep 2019, Phillip Wood wrote:\n\n> This is definitely worth fixing, I've got a couple of comments below\n>\n> On 02/09/2019 15:01, Matt R via GitGitGadget wrote:\n> > From: Matt R \n\nI just noticed that the surname is abbreviated. The full name of the\nauthor is \"Matt Rogers\". (Matt, Git uses the Signed-off-by: lines as\nsome sort of legally-binding assurance that you are free to submit these\nchanges under the GPLv2, so your full name is kinda required.)\n\n> > The `label` todo command in interactive rebases creates temporary refs\n> > in the `refs/rewritten/` namespace. These refs are stored as loose refs,\n> > i.e. as files in `.git/refs/rewritten/`, therefore they have to conform\n> > with file name limitations on the current filesystem.\n> >\n> > This poses a problem in particular on NTFS/FAT, where e.g. the colon\n> > character is not a valid part of a file name.\n>\n> Being picking I'll point out that ':' is not a valid in refs either.\n\nTrue, but that was not the primary concern here ;-)\n\n> Looking at\n> https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file I\n> think only \" and | are not allowed on NTFS/FAT but are valid in refs\n> (see the man page for git check-ref-format for all the details). So\n> the main limitation is actually what git allows in refs.\n\nRight. And this example shows that we really need to be a bit more\nconservative than just disallowing characters that would not be allowed\nin refs.\n\nI think it is more important to keep in mind the vagaries of various\ncurrent and future filesystems to justify the change in this patch.\n\n> > Let's safeguard against this by replacing not only white-space\n> > characters by dashes, but all non-alpha-numeric ones.\n> >\n> > However, we exempt non-ASCII UTF-8 characters from that, as it should be\n> > quite possible to reflect branch names such as `↯↯↯` in refs/file names.\n> >\n> > Signed-off-by: Matthew Rogers \n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > sequencer.c | 12 +++++++++++-\n> > t/t3430-rebase-merges.sh | 5 +++++\n> > 2 files changed, 16 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/sequencer.c b/sequencer.c\n> > index 34ebf8ed94..23f4a0876a 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -4635,8 +4635,18 @@ static int make_script_with_merges(struct\n> > pretty_print_context *pp,\n> > else\n> > strbuf_addbuf(&label, &oneline);\n> > +\t\t/*\n> > +\t\t * Sanitize labels by replacing non-alpha-numeric characters\n> > +\t\t * (including white-space ones) by dashes, as they might be\n> > +\t\t * illegal in file names (and hence in ref names).\n> > +\t\t *\n> > +\t\t * Note that we retain non-ASCII UTF-8 characters (identified\n> > +\t\t * via the most significant bit). They should be all\n> > acceptable\n> > +\t\t * in file names. We do not validate the UTF-8 here, that's\n> > not\n> > +\t\t * the job of this function.\n> > +\t\t */\n> > \t\tfor (p1 = label.buf; *p1; p1++)\n> > -\t\t\tif (isspace(*p1))\n> > +\t\t\tif (!(*p1 & 0x80) && !isalnum(*p1))\n> > *(char *)p1 = '-';\n>\n> I'm sightly concerned that this opens the possibility for unexpected effects\n> if two different labels get sanitized to the same string. I suspect it's\n> unlikely to happen in practice but doing something like percent encoding\n> non-alphanumeric characters would avoid the problem entirely.\n\nOh, but we make sure that the labels are unique, via the `label_oid()`\nfunction! Otherwise, we would not be able to label more than one merge\nparent ;-)\n\nCiao,\nDscho\n"},{"id":"381689","messageId":"20190902201230.GG11334@genre.crustytoothpaste.net","threadId":"51788","inReplyTo":"xmqq5zmav9ej.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] rebase -r: let `label` generate safer labels","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-09-02T20:12:31Z","receivedAt":"2019-09-02T20:12:40Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2019-09-02 at 18:29:56, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n> > Being picking I'll point out that ':' is not a valid in refs\n> > either. Looking at\n> > https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file I\n> > think only \" and | are not allowed on NTFS/FAT but are valid in refs\n> > (see the man page for git check-ref-format for all the details). So\n> > the main limitation is actually what git allows in refs.\n> \n> Yeah, trying to use the contents of the log message without\n> sufficient sanitization is looking for trouble.\n> \n> >> \t\tfor (p1 = label.buf; *p1; p1++)\n> >> -\t\t\tif (isspace(*p1))\n> >> +\t\t\tif (!(*p1 & 0x80) && !isalnum(*p1))\n> >> \t\t\t\t*(char *)p1 = '-';\n\nWhile we're thinking of things that could go wrong, note that it's also\npossible for the commit message to contain non-UTF-8 characters (if the\nuser is using a non-UTF-8 encoding), which will cause sadness on Windows\nand macOS. Non-Mac Unix systems aren't a problem here, but then again,\nthey aren't the reason for this patch.\n\n> > I'm sightly concerned that this opens the possibility for unexpected\n> > effects if two different labels get sanitized to the same string. I\n> > suspect it's unlikely to happen in practice but doing something like\n> > percent encoding non-alphanumeric characters would avoid the problem\n> > entirely.\n> \n> I see there are \"lets make sure it is unique by suffixing \"-%d\" in\n> other codepaths; would that help if this piece of code yields a\n> label that is not unique?\n\nI was thinking the same thing. Since we're being much less lenient on\nwhat's allowed (which is fine), we're at increased risk for collision.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"381691","messageId":"3edd55ed-b507-a14a-5cfb-0bfe471efbbc@iee.email","threadId":"51788","inReplyTo":"xmqq5zmav9ej.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] rebase -r: let `label` generate safer labels","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2019-09-02T21:24:34Z","receivedAt":"2019-09-02T21:24:36Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"On 02/09/2019 19:29, Junio C Hamano wrote:\n> I see there are \"lets make sure it is unique by suffixing \"-%d\" in\n> other codepaths; would that help if this piece of code yields a\n> label that is not unique?\nmaybe use a trailing 4 characters  of the oid to get a reasonably unique \nlabel?\n\nOh, just seen dscho's \"we make sure that the labels are unique, via the \n`label_oid()` function!\", maybe needs mentioning in the commit message \nif re-rolled.\n\nPhilip\n"},{"id":"381692","messageId":"b39e5215-f5d6-eca3-7f08-813b5508d779@iee.email","threadId":"51788","inReplyTo":"CAOjrSZtw+wYHxFRQCfb80xzm9OsGDh2rW8uD+AYYdmDPxk5DFQ@mail.gmail.com","subject":"Re: [PATCH 1/1] rebase -r: let `label` generate safer labels","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2019-09-02T22:13:17Z","receivedAt":"2019-09-02T22:13:20Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"On 02/09/2019 22:47, Matt Rogers wrote:\n> I can redo the commit, I had thought that I had previously fixed the \n> author but I guess I was mistaken.\n>\n> As for issues with non utf-8 encodings, I don't know of any simple way \n> to check for those except for restricting to asci alphanumeric characters\n\nIf I read the Wikipedia article [1] about the utf-8 design choices it is \npretty reasonable and robust most of the time, though that maybe a part \none way trapdoor.\n\n> Also the code given doesn't resolve onelines that consist only of \n> restricted file names (e.g. COM, NUL, etc. On windows)\n\nIt maybe that the rebase doc may need (if it happens) a short comment \nwarning of that.\n\nAlso need to check if the `label_oid()` function actually makes the \nlabel distinct, hence prevents such labels from being used as such a \nrestricted file name - i.e. does it include the oid element.\n\nUltimately the label could be tweaked to have say the 4char prefix to \nfool the Windows 'starts with' name detection - which assumes I \nunderstand how some of those bad filenames are detected...\n>\n>\n>\n> On Mon, Sep 2, 2019, 5:24 PM Philip Oakley wrote:\n>\n> On 02/09/2019 19:29, Junio C Hamano wrote:\n> > I see there are \"lets make sure it is unique by suffixing \"-%d\" in\n> > other codepaths; would that help if this piece of code yields a\n> > label that is not unique?\n> maybe use a trailing 4 characters  of the oid to get a reasonably\n> unique\n> label?\n>\n> Oh, just seen dscho's \"we make sure that the labels are unique,\n> via the\n> `label_oid()` function!\", maybe needs mentioning in the commit\n> message\n> if re-rolled.\n>\n> Philip\n>\n\n[1] https://en.wikipedia.org/wiki/UTF-8#Description\n"},{"id":"381715","messageId":"nycvar.QRO.7.76.6.1909031256290.46@tvgsbejvaqbjf.bet","threadId":"51788","inReplyTo":"xmqq5zmav9ej.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] rebase -r: let `label` generate safer labels","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-09-03T11:19:59Z","receivedAt":"2019-09-03T11:20:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 2 Sep 2019, Junio C Hamano wrote:\n\n> Phillip Wood writes:\n>\n> >> \t\tfor (p1 = label.buf; *p1; p1++)\n> >> -\t\t\tif (isspace(*p1))\n> >> +\t\t\tif (!(*p1 & 0x80) && !isalnum(*p1))\n> >> \t\t\t\t*(char *)p1 = '-';\n> >\n> > I'm sightly concerned that this opens the possibility for unexpected\n> > effects if two different labels get sanitized to the same string. I\n> > suspect it's unlikely to happen in practice but doing something like\n> > percent encoding non-alphanumeric characters would avoid the problem\n> > entirely.\n>\n> I'd rather see 'x' used instead of '-' (double-or-more dashes and\n> leading dash in refnames may currently be allowed but double-or-more\n> exes and leading ex would be much more likely to stay valid) if we\n> just want to redact invalid characters.\n\nHmm. Let's take a concrete example from the VFS for Git fork:\n\n\tMerge pull request #160: trace2:gvfs:experiment Add experimental regions and data events to help diagnose checkout and reset perf problems\n\n(Yes, we have some quite verbose merge commits, with very, very long\nonelines. Not a good practice, we stopped doing it, but it was well\nwithin what Git allows.)\n\nAnd now use dashes to encode all white-space:\n\n\tMerge-pull-request-#160:-trace2:gvfs:experiment-Add-experimental-regions-and-data-events-to-help-diagnose-checkout-and-reset-perf-problems\n\nPretty long, but looks okay. Of course, it does not work, because colons. So here is the label with Matt's patch:\n\n\tMerge-pull-request--160--trace2-gvfs-experiment-Add-experimental-regions-and-data-events-to-help-diagnose-checkout-and-reset-perf-problems\n\nAnd here is the label with your proposed xs.\n\n\tMergexpullxrequestxx160xxtrace2xgvfsxexperimentxAddxexperimentalxregionsxandxdataxeventsxtoxhelpxdiagnosexcheckoutxandxresetxperfxproblems\n\nI cannot speak for you, of course, but I can speak for myself: this is\nnot only way too reminiscent of xoxoxothxbye, but it is also really,\ntotally unreadable.\n\nIf you care deeply about double dashes and leading dashes, how about\nthis instead?\n\n\t\tchar *from, *to;\n\n \t\tfor (from = to = label.buf; *from; from++)\n\t\t\tif ((*from & 0x80) || isalnum(*from))\n\t\t\t\t*(to++) = *from;\n\t\t\t/* avoid leading dash and double-dashes */\n\t\t\telse if (to != label.buf && to[-1] != '-')\n \t\t\t\t*(to++) = '-';\n\t\tstrbuf_setlen(&label, to - label.buf);\n\nThat would result in\n\n\tMerge-pull-request-160-trace2-gvfs-experiment-Add-experimental-regions-and-data-events-to-help-diagnose-checkout-and-reset-perf-problems\n\n> I see there are \"lets make sure it is unique by suffixing \"-%d\" in\n> other codepaths; would that help if this piece of code yields a\n> label that is not unique?\n\nI'm way ahead of you. The sequencer already goes out of its way to\nguarantee the uniqueness of the labels (it's part of the design, as\napplied in 1644c73c6d4 (rebase-helper --make-script: introduce a flag to\nrebase merges, 2018-04-25)).\n\nThe patch you are looking at in this thread is only concerned about the\ninitial phase, `label_oid()` does a lot more. Not only does it make the\nlabel unique (case-insensitively!), it also prevents it from looking\nlike a full 40-hex digit SHA-1, so that we can guarantee that unique\nabbreviations of commit hashes will work as labels, too.\n\nCiao,\nDscho\n"},{"id":"381745","messageId":"xmqqo901tfn8.fsf@gitster-ct.c.googlers.com","threadId":"51788","inReplyTo":"nycvar.QRO.7.76.6.1909022124350.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/1] rebase -r: let `label` generate safer labels","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-03T18:10:19Z","receivedAt":"2019-09-03T18:10:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> I'm sightly concerned that this opens the possibility for unexpected effects\n>> if two different labels get sanitized to the same string. I suspect it's\n>> unlikely to happen in practice but doing something like percent encoding\n>> non-alphanumeric characters would avoid the problem entirely.\n>\n> Oh, but we make sure that the labels are unique, via the `label_oid()`\n> function! Otherwise, we would not be able to label more than one merge\n> parent ;-)\n\nIt somewhat feels suboptimal, from code followability's point of\nview, to have this \"pre-sanitization\" to replace isspace() to a\ndash, which is being extended to \"all non-alnums\", and the uniquefy\nof labels in label_oid(), in two separate places. I wonder if the\nresulting code becomes easier to follow and harder to introduce new\nbugs, if this part is made to just yield label.buf it obtained form\nthe log message as-is and leave the munging to label_oid()?\n\n"},{"id":"381765","messageId":"xmqq5zm9taza.fsf@gitster-ct.c.googlers.com","threadId":"51788","inReplyTo":"nycvar.QRO.7.76.6.1909031256290.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/1] rebase -r: let `label` generate safer labels","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-03T19:51:05Z","receivedAt":"2019-09-03T19:51:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> If you care deeply about double dashes and leading dashes, how about\n> this instead?\n>\n> \t\tchar *from, *to;\n>\n> \t\tfor (from = to = label.buf; *from; from++)\n> \t\t\tif ((*from & 0x80) || isalnum(*from))\n> \t\t\t\t*(to++) = *from;\n> \t\t\t/* avoid leading dash and double-dashes */\n> \t\t\telse if (to != label.buf && to[-1] != '-')\n> \t\t\t\t*(to++) = '-';\n> \t\tstrbuf_setlen(&label, to - label.buf);\n\nSimple enough and is a good change when judged locally.\n\nIt still would cause readers to wonder if label_oid() later append\n'-%d' to end up with double-dash near the end, etc., which made me\nwonder if the resulting code becomes better if sanitization and\nuniquefying are done at the same single place in the other message.\n"},{"id":"381776","messageId":"CAOjrSZsFeGsuNM2v=fPMnbHJH27Z6NU3UQrgoWt-peXjzWsD+w@mail.gmail.com","threadId":"51788","inReplyTo":"xmqq5zm9taza.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] rebase -r: let `label` generate safer labels","fromName":"Matt Rogers","fromEmail":"mattr94@gmail.com","sentAt":"2019-09-03T22:40:57Z","receivedAt":"2019-09-03T22:41:11Z","isPatch":true,"sender":{"key":"mattr94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5719846?v=4"},"body":"I agree that the code locally was simple enough.\n\nUltimately I feel that sanitizing and uniqueifying the label should\nprobably be done closer together/at the same place. I'm just not\nfamiliar enough with the codebase to know a good place (if any) to move\nthat to. Eventually though this would still need to be expanded further\nto protect against reserved filenames (e.g. NUL on windows). Although\nthe behavior around these (espescially with file extensions like\nNUL.txt) become less reliable, and although they are much more unlikely\nto be encountered in practice, are still allowed by git as oneliners.\n\n\nOn Tue, Sep 3, 2019 at 3:51 PM Junio C Hamano wrote:\n>\n> Johannes Schindelin writes:\n>\n> > If you care deeply about double dashes and leading dashes, how about\n> > this instead?\n> >\n> > char *from, *to;\n> >\n> > for (from = to = label.buf; *from; from++)\n> > if ((*from & 0x80) || isalnum(*from))\n> > *(to++) = *from;\n> > /* avoid leading dash and double-dashes */\n> > else if (to != label.buf && to[-1] != '-')\n> > *(to++) = '-';\n> > strbuf_setlen(&label, to - label.buf);\n>\n> Simple enough and is a good change when judged locally.\n>\n> It still would cause readers to wonder if label_oid() later append\n> '-%d' to end up with double-dash near the end, etc., which made me\n> wonder if the resulting code becomes better if sanitization and\n> uniquefying are done at the same single place in the other message.\n\n\n\n-- \nMatthew Rogers\n"},{"id":"386423","messageId":"pull.327.v2.git.1574032570.gitgitgadget@gmail.com","threadId":"51788","inReplyTo":"pull.327.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] Make git rebase -r's label generation more resilient","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-17T23:16:08Z","receivedAt":"2019-11-17T23:16:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Those labels must be valid ref names, and therefore valid file names. The\ninitial patch came in via Git for Windows.\n\nChange since v1:\n\n * moved the entire sanitizing logic to label_oid(), as a preparatory step.\n\nJohannes Schindelin (1):\n rebase-merges: move labels' whitespace mangling into `label_oid()`\n\nMatthew Rogers (1):\n rebase -r: let `label` generate safer labels\n\n sequencer.c | 72 +++++++++++++++++++++++++---------------\n t/t3430-rebase-merges.sh | 6 ++++\n 2 files changed, 51 insertions(+), 27 deletions(-)\n\n\nbase-commit: d9f6f3b6195a0ca35642561e530798ad1469bd41\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-327%2Fdscho%2Ffix-rebase-r-labels-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-327/dscho/fix-rebase-r-labels-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/327\n\nRange-diff vs v1:\n\n -: ---------- > 1: 3f4d086e80 rebase-merges: move labels' whitespace mangling into `label_oid()`\n 1: 4a02c38442 ! 2: adc22c8ba2 rebase -r: let `label` generate safer labels\n @@ -1,14 +1,15 @@\n -Author: Matt R \n +Author: Matthew Rogers \n \n rebase -r: let `label` generate safer labels\n \n The `label` todo command in interactive rebases creates temporary refs\n in the `refs/rewritten/` namespace. These refs are stored as loose refs,\n i.e. as files in `.git/refs/rewritten/`, therefore they have to conform\n - with file name limitations on the current filesystem.\n + with file name limitations on the current filesystem in addition to the\n + accepted ref format.\n \n - This poses a problem in particular on NTFS/FAT, where e.g. the colon\n - character is not a valid part of a file name.\n + This poses a problem in particular on NTFS/FAT, where e.g. the colon,\n + double-quote and pipe characters are disallowed as part of a file name.\n \n Let's safeguard against this by replacing not only white-space\n characters by dashes, but all non-alpha-numeric ones.\n @@ -23,8 +24,8 @@\n --- a/sequencer.c\n +++ b/sequencer.c\n @@\n - \t\telse\n - \t\t\tstrbuf_addbuf(&label, &oneline);\n + \t} else {\n + \t\tstruct strbuf *buf = &state->buf;\n \n +\t\t/*\n +\t\t * Sanitize labels by replacing non-alpha-numeric characters\n @@ -36,18 +37,26 @@\n +\t\t * in file names. We do not validate the UTF-8 here, that's not\n +\t\t * the job of this function.\n +\t\t */\n - \t\tfor (p1 = label.buf; *p1; p1++)\n --\t\t\tif (isspace(*p1))\n -+\t\t\tif (!(*p1 & 0x80) && !isalnum(*p1))\n - \t\t\t\t*(char *)p1 = '-';\n + \t\tfor (; *label; label++)\n +-\t\t\tstrbuf_addch(buf, isspace(*label) ? '-' : *label);\n ++\t\t\tif ((*label & 0x80) || isalnum(*label))\n ++\t\t\t\tstrbuf_addch(buf, *label);\n ++\t\t\t/* avoid leading dash and double-dashes */\n ++\t\t\telse if (buf->len && buf->buf[buf->len - 1] != '-')\n ++\t\t\t\tstrbuf_addch(buf, '-');\n ++\t\tif (!buf->len) {\n ++\t\t\tstrbuf_addstr(buf, \"rev-\");\n ++\t\t\tstrbuf_add_unique_abbrev(buf, oid, default_abbrev);\n ++\t\t}\n + \t\tlabel = buf->buf;\n \n - \t\tstrbuf_reset(&buf);\n + \t\tif ((buf->len == the_hash_algo->hexsz &&\n \n diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\n --- a/t/t3430-rebase-merges.sh\n +++ b/t/t3430-rebase-merges.sh\n @@\n - \ttest_path_is_missing .git/MERGE_HEAD\n + \ttest_cmp expect G.t\n '\n \n +test_expect_success '--rebase-merges with commit that can generate bad characters for filename' '\n @@ -55,4 +64,5 @@\n +\tgit merge -m \"colon: this should work\" G &&\n +\tgit rebase --rebase-merges --force-rebase E\n +'\n ++\n test_done\n\n-- \ngitgitgadget\n"},{"id":"386424","messageId":"adc22c8ba2f14ee541f61ea06a5423deb3613e78.1574032570.git.gitgitgadget@gmail.com","threadId":"51788","inReplyTo":"pull.327.v2.git.1574032570.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] rebase -r: let `label` generate safer labels","fromName":"Matthew Rogers via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-17T23:16:10Z","receivedAt":"2019-11-17T23:16:15Z","isPatch":true,"sender":{"key":"name:Matthew Rogers","avatar":null},"body":"From: Matthew Rogers \n\nThe `label` todo command in interactive rebases creates temporary refs\nin the `refs/rewritten/` namespace. These refs are stored as loose refs,\ni.e. as files in `.git/refs/rewritten/`, therefore they have to conform\nwith file name limitations on the current filesystem in addition to the\naccepted ref format.\n\nThis poses a problem in particular on NTFS/FAT, where e.g. the colon,\ndouble-quote and pipe characters are disallowed as part of a file name.\n\nLet's safeguard against this by replacing not only white-space\ncharacters by dashes, but all non-alpha-numeric ones.\n\nHowever, we exempt non-ASCII UTF-8 characters from that, as it should be\nquite possible to reflect branch names such as `↯↯↯` in refs/file names.\n\nSigned-off-by: Matthew Rogers \nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 20 +++++++++++++++++++-\n t/t3430-rebase-merges.sh | 6 ++++++\n 2 files changed, 25 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 85c66f489f..fece07b680 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4471,8 +4471,26 @@ static const char *label_oid(struct object_id *oid, const char *label,\n \t} else {\n \t\tstruct strbuf *buf = &state->buf;\n \n+\t\t/*\n+\t\t * Sanitize labels by replacing non-alpha-numeric characters\n+\t\t * (including white-space ones) by dashes, as they might be\n+\t\t * illegal in file names (and hence in ref names).\n+\t\t *\n+\t\t * Note that we retain non-ASCII UTF-8 characters (identified\n+\t\t * via the most significant bit). They should be all acceptable\n+\t\t * in file names. We do not validate the UTF-8 here, that's not\n+\t\t * the job of this function.\n+\t\t */\n \t\tfor (; *label; label++)\n-\t\t\tstrbuf_addch(buf, isspace(*label) ? '-' : *label);\n+\t\t\tif ((*label & 0x80) || isalnum(*label))\n+\t\t\t\tstrbuf_addch(buf, *label);\n+\t\t\t/* avoid leading dash and double-dashes */\n+\t\t\telse if (buf->len && buf->buf[buf->len - 1] != '-')\n+\t\t\t\tstrbuf_addch(buf, '-');\n+\t\tif (!buf->len) {\n+\t\t\tstrbuf_addstr(buf, \"rev-\");\n+\t\t\tstrbuf_add_unique_abbrev(buf, oid, default_abbrev);\n+\t\t}\n \t\tlabel = buf->buf;\n \n \t\tif ((buf->len == the_hash_algo->hexsz &&\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex 9efcf4808a..f728aba995 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -468,4 +468,10 @@ test_expect_success '--rebase-merges with strategies' '\n \ttest_cmp expect G.t\n '\n \n+test_expect_success '--rebase-merges with commit that can generate bad characters for filename' '\n+\tgit checkout -b colon-in-label E &&\n+\tgit merge -m \"colon: this should work\" G &&\n+\tgit rebase --rebase-merges --force-rebase E\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"386425","messageId":"3f4d086e805b69cf1cbf8058f382cc154239f703.1574032570.git.gitgitgadget@gmail.com","threadId":"51788","inReplyTo":"pull.327.v2.git.1574032570.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] rebase-merges: move labels' whitespace mangling into `label_oid()`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-17T23:16:09Z","receivedAt":"2019-11-17T23:16:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nOne of the trickier aspects of the design of `git rebase\n--rebase-merges` is the way labels are generated for the initial todo\nlist: those labels are supposed to be intuitive and first and foremost\nunique.\n\nTo that end, `label_oid()` appends a unique suffix when necessary.\n\nThose labels not only need to be unique, but they also need to be valid\nrefs. To make sure of that, `make_script_with_merges()` replaces\nwhitespace by dashes.\n\nThat would appear to be the wrong layer for that sanitizing step,\nthough: all callers of `label_oid()` should get that same benefit.\n\nEven if it does not make a difference currently (the only called of\n`label_oid()` that passes a label that might need to be sanitized _is_\n`make_script_with_merges()`), let's move the responsibility for\nsanitizing labels into the `label_oid()` function.\n\nThis commit is best viewed with `-w` because it unfortunately needs to\nchange the indentation of a large block of code in `label_oid()`.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 56 ++++++++++++++++++++++++++---------------------------\n 1 file changed, 28 insertions(+), 28 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 8952cfa89b..85c66f489f 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4423,7 +4423,6 @@ static const char *label_oid(struct object_id *oid, const char *label,\n \tstruct labels_entry *labels_entry;\n \tstruct string_entry *string_entry;\n \tstruct object_id dummy;\n-\tsize_t len;\n \tint i;\n \n \tstring_entry = oidmap_get(&state->commit2label, oid);\n@@ -4443,10 +4442,10 @@ static const char *label_oid(struct object_id *oid, const char *label,\n \t * abbreviation for any uninteresting commit's names that does not\n \t * clash with any other label.\n \t */\n+\tstrbuf_reset(&state->buf);\n \tif (!label) {\n \t\tchar *p;\n \n-\t\tstrbuf_reset(&state->buf);\n \t\tstrbuf_grow(&state->buf, GIT_MAX_HEXSZ);\n \t\tlabel = p = state->buf.buf;\n \n@@ -4469,32 +4468,37 @@ static const char *label_oid(struct object_id *oid, const char *label,\n \t\t\t\tp[i] = save;\n \t\t\t}\n \t\t}\n-\t} else if (((len = strlen(label)) == the_hash_algo->hexsz &&\n-\t\t !get_oid_hex(label, &dummy)) ||\n-\t\t (len == 1 && *label == '#') ||\n-\t\t hashmap_get_from_hash(&state->labels,\n-\t\t\t\t\t strihash(label), label)) {\n-\t\t/*\n-\t\t * If the label already exists, or if the label is a valid full\n-\t\t * OID, or the label is a '#' (which we use as a separator\n-\t\t * between merge heads and oneline), we append a dash and a\n-\t\t * number to make it unique.\n-\t\t */\n+\t} else {\n \t\tstruct strbuf *buf = &state->buf;\n \n-\t\tstrbuf_reset(buf);\n-\t\tstrbuf_add(buf, label, len);\n+\t\tfor (; *label; label++)\n+\t\t\tstrbuf_addch(buf, isspace(*label) ? '-' : *label);\n+\t\tlabel = buf->buf;\n \n-\t\tfor (i = 2; ; i++) {\n-\t\t\tstrbuf_setlen(buf, len);\n-\t\t\tstrbuf_addf(buf, \"-%d\", i);\n-\t\t\tif (!hashmap_get_from_hash(&state->labels,\n-\t\t\t\t\t\t strihash(buf->buf),\n-\t\t\t\t\t\t buf->buf))\n-\t\t\t\tbreak;\n-\t\t}\n+\t\tif ((buf->len == the_hash_algo->hexsz &&\n+\t\t !get_oid_hex(label, &dummy)) ||\n+\t\t (buf->len == 1 && *label == '#') ||\n+\t\t hashmap_get_from_hash(&state->labels,\n+\t\t\t\t\t strihash(label), label)) {\n+\t\t\t/*\n+\t\t\t * If the label already exists, or if the label is a\n+\t\t\t * valid full OID, or the label is a '#' (which we use\n+\t\t\t * as a separator between merge heads and oneline), we\n+\t\t\t * append a dash and a number to make it unique.\n+\t\t\t */\n+\t\t\tsize_t len = buf->len;\n \n-\t\tlabel = buf->buf;\n+\t\t\tfor (i = 2; ; i++) {\n+\t\t\t\tstrbuf_setlen(buf, len);\n+\t\t\t\tstrbuf_addf(buf, \"-%d\", i);\n+\t\t\t\tif (!hashmap_get_from_hash(&state->labels,\n+\t\t\t\t\t\t\t strihash(buf->buf),\n+\t\t\t\t\t\t\t buf->buf))\n+\t\t\t\t\tbreak;\n+\t\t\t}\n+\n+\t\t\tlabel = buf->buf;\n+\t\t}\n \t}\n \n \tFLEX_ALLOC_STR(labels_entry, label, label);\n@@ -4596,10 +4600,6 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \t\telse\n \t\t\tstrbuf_addbuf(&label, &oneline);\n \n-\t\tfor (p1 = label.buf; *p1; p1++)\n-\t\t\tif (isspace(*p1))\n-\t\t\t\t*(char *)p1 = '-';\n-\n \t\tstrbuf_reset(&buf);\n \t\tstrbuf_addf(&buf, \"%s -C %s\",\n \t\t\t cmd_merge, oid_to_hex(&commit->object.oid));\n-- \ngitgitgadget\n\n"},{"id":"386434","messageId":"xmqq8sod3l5a.fsf@gitster-ct.c.googlers.com","threadId":"51788","inReplyTo":"pull.327.v2.git.1574032570.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] Make git rebase -r's label generation more resilient","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-18T03:42:41Z","receivedAt":"2019-11-18T03:42:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> Those labels must be valid ref names, and therefore valid file names. The\n> initial patch came in via Git for Windows.\n>\n> Change since v1:\n>\n> * moved the entire sanitizing logic to label_oid(), as a preparatory step.\n>\n> Johannes Schindelin (1):\n> rebase-merges: move labels' whitespace mangling into `label_oid()`\n>\n> Matthew Rogers (1):\n> rebase -r: let `label` generate safer labels\n>\n> sequencer.c | 72 +++++++++++++++++++++++++---------------\n> t/t3430-rebase-merges.sh | 6 ++++\n> 2 files changed, 51 insertions(+), 27 deletions(-)\n\nI think Dscho meant to Cc you as these two patches are meant to be a\nmore complete solution to supersede your [*1*].\n\n\n[Reference]\n\n*1* <860dee65f49ea7eacf5a0c7c8ffe59095a51b1ce.1573205699.git.congdanhqx@gmail.com>\n"},{"id":"386445","messageId":"20191118112357.GA29922@danh.dev","threadId":"51788","inReplyTo":"xmqq8sod3l5a.fsf@gitster-ct.c.googlers.com","subject":"[PATCH] sequencer: handle rebase-merge for \"onto\" message","fromName":"Danh Doan","fromEmail":"congdanhqx@gmail.com","sentAt":"2019-11-18T11:23:57Z","receivedAt":"2019-11-18T11:24:02Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2019-11-18 12:42:41+0900, Junio C Hamano wrote:\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n> \n> > Those labels must be valid ref names, and therefore valid file names. The\n> > initial patch came in via Git for Windows.\n> >\n> > Change since v1:\n> >\n> > * moved the entire sanitizing logic to label_oid(), as a preparatory step.\n> >\n> > Johannes Schindelin (1):\n> > rebase-merges: move labels' whitespace mangling into `label_oid()`\n> >\n> > Matthew Rogers (1):\n> > rebase -r: let `label` generate safer labels\n> >\n> > sequencer.c | 72 +++++++++++++++++++++++++---------------\n> > t/t3430-rebase-merges.sh | 6 ++++\n> > 2 files changed, 51 insertions(+), 27 deletions(-)\n> \n> I think Dscho meant to Cc you as these two patches are meant to be a\n> more complete solution to supersede your [*1*].\n\nI've applied their patches over my branches,\nmy introduced test_expect_failure was flipped.\nThat's nice.\n\nAnyway, when reading their patch, I discovered a problem with\ngit rebase --rebase-merges when its message is onto.\n\nHere is a patch to fix it.\n\nI applied Dscho's patches over my dd/sequencer-utf8 then writing this\npatch, in case you have problem applying it.\n\nThe context for the diff is coming from Dscho's patches.\n\n-------8<--------------------\nFrom 48205889b404b82baa4b30c2eedd52243c349e3e Mon Sep 17 00:00:00 2001\nFrom: Doan Tran Cong Danh \nDate: Mon, 18 Nov 2019 18:02:05 +0700\nSubject: [PATCH] sequencer: handle rebase-merge for \"onto\" message\n\nIn order to work correctly, git-rebase --rebase-merges needs to make\ninitial todo list with unique labels.\n\nThose unique labels is being handled by employing a hashmap and\nsuffixing an unique number if any duplicate is found.\n\nBut we forgat that beside of those labels for side branches,\nwe also make a special label `onto' for our so-called new-base.\n\nIn a special case that any of those labels for side branches named\n`onto', git will run into trouble.\n\nCorrect it.\n\nSigned-off-by: Doan Tran Cong Danh \n---\n sequencer.c | 5 +++++\n t/t3430-rebase-merges.sh | 21 +++++++++++++++++++++\n 2 files changed, 26 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 350045b1b4..fc81e43f0f 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4569,10 +4569,15 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \tstrbuf_init(&state.buf, 32);\n \n \tif (revs->cmdline.nr && (revs->cmdline.rev[0].flags & BOTTOM)) {\n+\t\tstruct labels_entry *onto_label_entry;\n \t\tstruct object_id *oid = &revs->cmdline.rev[0].item->oid;\n \t\tFLEX_ALLOC_STR(entry, string, \"onto\");\n \t\toidcpy(&entry->entry.oid, oid);\n \t\toidmap_put(&state.commit2label, entry);\n+\n+\t\tFLEX_ALLOC_STR(onto_label_entry, label, \"onto\");\n+\t\thashmap_entry_init(&onto_label_entry->entry, strihash(\"onto\"));\n+\t\thashmap_add(&state.labels, &onto_label_entry->entry);\n \t}\n \n \t/*\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex f728aba995..4e2c0ede51 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -474,4 +474,25 @@ test_expect_success '--rebase-merges with commit that can generate bad character\n \tgit rebase --rebase-merges --force-rebase E\n '\n \n+test_expect_success '--rebase-merges with message matched with onto label' '\n+\tgit checkout -b onto-label E &&\n+\tgit merge -m onto G &&\n+\tgit rebase --rebase-merges --force-rebase E &&\n+\ttest_cmp_graph <<-\\EOF\n+\t* onto\n+\t|\\\n+\t| * G\n+\t| * F\n+\t* | E\n+\t|\\ \\\n+\t| * | B\n+\t* | | D\n+\t| |/\n+\t|/|\n+\t* | C\n+\t|/\n+\t* A\n+\tEOF\n+'\n+\n test_done\n-- \n2.24.0.10.gc61c3b979f.dirty\n\n"},{"id":"386447","messageId":"20191118115747.31533-1-congdanhqx@gmail.com","threadId":"51788","inReplyTo":"20191118112357.GA29922@danh.dev","subject":"[PATCH v2] sequencer: handle rebase-merges for \"onto\" message","fromName":"Doan Tran Cong Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2019-11-18T11:57:47Z","receivedAt":"2019-11-18T11:57:53Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"In order to work correctly, git-rebase --rebase-merges needs to make\ninitial todo list with unique labels.\n\nThose unique labels is being handled by employing a hashmap and\nappending an unique number if any duplicate is found.\n\nBut, we forget that beside those labels for side branches,\nwe also have a special label `onto' for our so-called new-base.\n\nIn a special case that any of those labels for side branches named\n`onto', git will run into trouble.\n\nCorrect it.\n\nSigned-off-by: Doan Tran Cong Danh \n---\nSorry for the noise, I forgot to check spelling for v1\n\nAnd I forgot to delete From line when append to my MUA.\n\nRange-diff against v1:\n1: 48205889b4 ! 1: 9246beacf2 sequencer: handle rebase-merge for \"onto\" message\n @@ Metadata\n Author: Doan Tran Cong Danh \n \n ## Commit message ##\n - sequencer: handle rebase-merge for \"onto\" message\n + sequencer: handle rebase-merges for \"onto\" message\n \n In order to work correctly, git-rebase --rebase-merges needs to make\n initial todo list with unique labels.\n \n Those unique labels is being handled by employing a hashmap and\n - suffixing an unique number if any duplicate is found.\n + appending an unique number if any duplicate is found.\n \n - But we forgat that beside of those labels for side branches,\n - we also make a special label `onto' for our so-called new-base.\n + But, we forget that beside those labels for side branches,\n + we also have a special label `onto' for our so-called new-base.\n \n In a special case that any of those labels for side branches named\n `onto', git will run into trouble.\n\n sequencer.c | 5 +++++\n t/t3430-rebase-merges.sh | 21 +++++++++++++++++++++\n 2 files changed, 26 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 350045b1b4..fc81e43f0f 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4569,10 +4569,15 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \tstrbuf_init(&state.buf, 32);\n \n \tif (revs->cmdline.nr && (revs->cmdline.rev[0].flags & BOTTOM)) {\n+\t\tstruct labels_entry *onto_label_entry;\n \t\tstruct object_id *oid = &revs->cmdline.rev[0].item->oid;\n \t\tFLEX_ALLOC_STR(entry, string, \"onto\");\n \t\toidcpy(&entry->entry.oid, oid);\n \t\toidmap_put(&state.commit2label, entry);\n+\n+\t\tFLEX_ALLOC_STR(onto_label_entry, label, \"onto\");\n+\t\thashmap_entry_init(&onto_label_entry->entry, strihash(\"onto\"));\n+\t\thashmap_add(&state.labels, &onto_label_entry->entry);\n \t}\n \n \t/*\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex f728aba995..4e2c0ede51 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -474,4 +474,25 @@ test_expect_success '--rebase-merges with commit that can generate bad character\n \tgit rebase --rebase-merges --force-rebase E\n '\n \n+test_expect_success '--rebase-merges with message matched with onto label' '\n+\tgit checkout -b onto-label E &&\n+\tgit merge -m onto G &&\n+\tgit rebase --rebase-merges --force-rebase E &&\n+\ttest_cmp_graph <<-\\EOF\n+\t* onto\n+\t|\\\n+\t| * G\n+\t| * F\n+\t* | E\n+\t|\\ \\\n+\t| * | B\n+\t* | | D\n+\t| |/\n+\t|/|\n+\t* | C\n+\t|/\n+\t* A\n+\tEOF\n+'\n+\n test_done\n-- \n2.24.0.10.gc61c3b979f.dirty\n\n"},{"id":"386466","messageId":"nycvar.QRO.7.76.6.1911182114030.46@tvgsbejvaqbjf.bet","threadId":"51788","inReplyTo":"20191118115747.31533-1-congdanhqx@gmail.com","subject":"Re: [PATCH v2] sequencer: handle rebase-merges for \"onto\" message","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-11-18T20:15:16Z","receivedAt":"2019-11-18T20:15:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 18 Nov 2019, Doan Tran Cong Danh wrote:\n\n> In order to work correctly, git-rebase --rebase-merges needs to make\n> initial todo list with unique labels.\n>\n> Those unique labels is being handled by employing a hashmap and\n> appending an unique number if any duplicate is found.\n>\n> But, we forget that beside those labels for side branches,\n> we also have a special label `onto' for our so-called new-base.\n>\n> In a special case that any of those labels for side branches named\n> `onto', git will run into trouble.\n>\n> Correct it.\n>\n> Signed-off-by: Doan Tran Cong Danh \n> ---\n\nLooks obviously correct to me. ACK!\n\nThank you,\nDscho\n\n> Sorry for the noise, I forgot to check spelling for v1\n>\n> And I forgot to delete From line when append to my MUA.\n>\n> Range-diff against v1:\n> 1: 48205889b4 ! 1: 9246beacf2 sequencer: handle rebase-merge for \"onto\" message\n> @@ Metadata\n> Author: Doan Tran Cong Danh \n>\n> ## Commit message ##\n> - sequencer: handle rebase-merge for \"onto\" message\n> + sequencer: handle rebase-merges for \"onto\" message\n>\n> In order to work correctly, git-rebase --rebase-merges needs to make\n> initial todo list with unique labels.\n>\n> Those unique labels is being handled by employing a hashmap and\n> - suffixing an unique number if any duplicate is found.\n> + appending an unique number if any duplicate is found.\n>\n> - But we forgat that beside of those labels for side branches,\n> - we also make a special label `onto' for our so-called new-base.\n> + But, we forget that beside those labels for side branches,\n> + we also have a special label `onto' for our so-called new-base.\n>\n> In a special case that any of those labels for side branches named\n> `onto', git will run into trouble.\n>\n> sequencer.c | 5 +++++\n> t/t3430-rebase-merges.sh | 21 +++++++++++++++++++++\n> 2 files changed, 26 insertions(+)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 350045b1b4..fc81e43f0f 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -4569,10 +4569,15 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n> \tstrbuf_init(&state.buf, 32);\n>\n> \tif (revs->cmdline.nr && (revs->cmdline.rev[0].flags & BOTTOM)) {\n> +\t\tstruct labels_entry *onto_label_entry;\n> \t\tstruct object_id *oid = &revs->cmdline.rev[0].item->oid;\n> \t\tFLEX_ALLOC_STR(entry, string, \"onto\");\n> \t\toidcpy(&entry->entry.oid, oid);\n> \t\toidmap_put(&state.commit2label, entry);\n> +\n> +\t\tFLEX_ALLOC_STR(onto_label_entry, label, \"onto\");\n> +\t\thashmap_entry_init(&onto_label_entry->entry, strihash(\"onto\"));\n> +\t\thashmap_add(&state.labels, &onto_label_entry->entry);\n> \t}\n>\n> \t/*\n> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\n> index f728aba995..4e2c0ede51 100755\n> --- a/t/t3430-rebase-merges.sh\n> +++ b/t/t3430-rebase-merges.sh\n> @@ -474,4 +474,25 @@ test_expect_success '--rebase-merges with commit that can generate bad character\n> \tgit rebase --rebase-merges --force-rebase E\n> '\n>\n> +test_expect_success '--rebase-merges with message matched with onto label' '\n> +\tgit checkout -b onto-label E &&\n> +\tgit merge -m onto G &&\n> +\tgit rebase --rebase-merges --force-rebase E &&\n> +\ttest_cmp_graph <<-\\EOF\n> +\t* onto\n> +\t|\\\n> +\t| * G\n> +\t| * F\n> +\t* | E\n> +\t|\\ \\\n> +\t| * | B\n> +\t* | | D\n> +\t| |/\n> +\t|/|\n> +\t* | C\n> +\t|/\n> +\t* A\n> +\tEOF\n> +'\n> +\n> test_done\n> --\n> 2.24.0.10.gc61c3b979f.dirty\n>\n>\n"},{"id":"386467","messageId":"nycvar.QRO.7.76.6.1911182150330.46@tvgsbejvaqbjf.bet","threadId":"51788","inReplyTo":"xmqqo901tfn8.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] rebase -r: let `label` generate safer labels","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-11-18T20:51:18Z","receivedAt":"2019-11-18T20:51:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 3 Sep 2019, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> >> I'm sightly concerned that this opens the possibility for unexpected effects\n> >> if two different labels get sanitized to the same string. I suspect it's\n> >> unlikely to happen in practice but doing something like percent encoding\n> >> non-alphanumeric characters would avoid the problem entirely.\n> >\n> > Oh, but we make sure that the labels are unique, via the `label_oid()`\n> > function! Otherwise, we would not be able to label more than one merge\n> > parent ;-)\n>\n> It somewhat feels suboptimal, from code followability's point of\n> view, to have this \"pre-sanitization\" to replace isspace() to a\n> dash, which is being extended to \"all non-alnums\", and the uniquefy\n> of labels in label_oid(), in two separate places. I wonder if the\n> resulting code becomes easier to follow and harder to introduce new\n> bugs, if this part is made to just yield label.buf it obtained form\n> the log message as-is and leave the munging to label_oid()?\n\nWhile the patch looks somewhat bulky without `-w` (due to the\nindentation change), I agree that it is conceptually a good idea,\ntherefore I made it so.\n\nCiao,\nDscho\n"},{"id":"386468","messageId":"nycvar.QRO.7.76.6.1911182151450.46@tvgsbejvaqbjf.bet","threadId":"51788","inReplyTo":"xmqq8sod3l5a.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/2] Make git rebase -r's label generation more resilient","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-11-18T20:52:00Z","receivedAt":"2019-11-18T20:52:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 18 Nov 2019, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > Those labels must be valid ref names, and therefore valid file names. The\n> > initial patch came in via Git for Windows.\n> >\n> > Change since v1:\n> >\n> > * moved the entire sanitizing logic to label_oid(), as a preparatory step.\n> >\n> > Johannes Schindelin (1):\n> > rebase-merges: move labels' whitespace mangling into `label_oid()`\n> >\n> > Matthew Rogers (1):\n> > rebase -r: let `label` generate safer labels\n> >\n> > sequencer.c | 72 +++++++++++++++++++++++++---------------\n> > t/t3430-rebase-merges.sh | 6 ++++\n> > 2 files changed, 51 insertions(+), 27 deletions(-)\n>\n> I think Dscho meant to Cc you as these two patches are meant to be a\n> more complete solution to supersede your [*1*].\n\nWhoops, totally forgot. Thanks!\nDscho\n\n>\n>\n> [Reference]\n>\n> *1* <860dee65f49ea7eacf5a0c7c8ffe59095a51b1ce.1573205699.git.congdanhqx@gmail.com>\n>\n"},{"id":"386667","messageId":"xmqqa78qaxto.fsf@gitster-ct.c.googlers.com","threadId":"51788","inReplyTo":"20191118112357.GA29922@danh.dev","subject":"Re: [PATCH] sequencer: handle rebase-merge for \"onto\" message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-21T00:16:03Z","receivedAt":"2019-11-21T00:16:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Danh Doan writes:\n\n> Anyway, when reading their patch, I discovered a problem with\n> git rebase --rebase-merges when its message is onto.\n>\n> Here is a patch to fix it.\n>\n> I applied Dscho's patches over my dd/sequencer-utf8 then writing this\n> patch, in case you have problem applying it.\n>\n> The context for the diff is coming from Dscho's patches.\n\nThanks. While technically this is independent from the \"safer\nrebase-merges labels\" topic (specifically its preparation step),\nin the larger picture, this too is to ensure we do not use a wrong\nstring as a label ;-), so I'll queue it on top of those two patches,\njust like how you developed.\n\nThanks.\n\n> -------8<--------------------\n> From 48205889b404b82baa4b30c2eedd52243c349e3e Mon Sep 17 00:00:00 2001\n> From: Doan Tran Cong Danh \n> Date: Mon, 18 Nov 2019 18:02:05 +0700\n> Subject: [PATCH] sequencer: handle rebase-merge for \"onto\" message\n>\n> In order to work correctly, git-rebase --rebase-merges needs to make\n> initial todo list with unique labels.\n>\n> Those unique labels is being handled by employing a hashmap and\n> suffixing an unique number if any duplicate is found.\n>\n> But we forgat that beside of those labels for side branches,\n> we also make a special label `onto' for our so-called new-base.\n>\n> In a special case that any of those labels for side branches named\n> `onto', git will run into trouble.\n>\n> Correct it.\n>\n> Signed-off-by: Doan Tran Cong Danh \n> ---\n> sequencer.c | 5 +++++\n> t/t3430-rebase-merges.sh | 21 +++++++++++++++++++++\n> 2 files changed, 26 insertions(+)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 350045b1b4..fc81e43f0f 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -4569,10 +4569,15 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n> \tstrbuf_init(&state.buf, 32);\n> \n> \tif (revs->cmdline.nr && (revs->cmdline.rev[0].flags & BOTTOM)) {\n> +\t\tstruct labels_entry *onto_label_entry;\n> \t\tstruct object_id *oid = &revs->cmdline.rev[0].item->oid;\n> \t\tFLEX_ALLOC_STR(entry, string, \"onto\");\n> \t\toidcpy(&entry->entry.oid, oid);\n> \t\toidmap_put(&state.commit2label, entry);\n> +\n> +\t\tFLEX_ALLOC_STR(onto_label_entry, label, \"onto\");\n> +\t\thashmap_entry_init(&onto_label_entry->entry, strihash(\"onto\"));\n> +\t\thashmap_add(&state.labels, &onto_label_entry->entry);\n> \t}\n> \n> \t/*\n> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\n> index f728aba995..4e2c0ede51 100755\n> --- a/t/t3430-rebase-merges.sh\n> +++ b/t/t3430-rebase-merges.sh\n> @@ -474,4 +474,25 @@ test_expect_success '--rebase-merges with commit that can generate bad character\n> \tgit rebase --rebase-merges --force-rebase E\n> '\n> \n> +test_expect_success '--rebase-merges with message matched with onto label' '\n> +\tgit checkout -b onto-label E &&\n> +\tgit merge -m onto G &&\n> +\tgit rebase --rebase-merges --force-rebase E &&\n> +\ttest_cmp_graph <<-\\EOF\n> +\t* onto\n> +\t|\\\n> +\t| * G\n> +\t| * F\n> +\t* | E\n> +\t|\\ \\\n> +\t| * | B\n> +\t* | | D\n> +\t| |/\n> +\t|/|\n> +\t* | C\n> +\t|/\n> +\t* A\n> +\tEOF\n> +'\n> +\n> test_done\n"}]}