{"thread":{"id":"50099","subject":"Parsing trailers","startedAt":"2018-12-23T22:41:38Z","lastAt":"2019-01-03T07:50:49Z","messageCount":8,"participants":["William Chargin","Christian Couder","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"365763","messageId":"CAFW+GMDazFSDzBrvzMqaPGwew=+CP7tw7G5FfDqcAUYd3qjPuQ@mail.gmail.com","threadId":"50099","inReplyTo":null,"subject":"Parsing trailers","fromName":"William Chargin","fromEmail":"wchargin@gmail.com","sentAt":"2018-12-23T22:41:20Z","receivedAt":"2018-12-23T22:41:38Z","isPatch":false,"sender":{"key":"wchargin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4317806?v=4"},"body":"I'm interested in parsing the output of `git-interpret-trailers` in a\nscript. I had hoped that the `--parse` option would make this easy, but\nit seems that the `trailer.separators` configuration option is used to\nspecify both the input format (which separators may indicate a trailer)\nand the output format of `git interpret-trailers --parse`. Given that\n`trailer.separators` may contain any (non-NUL) characters, including\nwhitespace, parsing the output is not straightforward.\n\nHere's what I've come up with. The output format is \" \",\nwhere \"\" and \"\" have been trimmed and so have no leading or\ntrailing whitespace, but \"\" may have internal whitespace while\n\"\" may not. Thus, the first space character in the output may\ncorrespond to either \"\" or the fixed space, but we should be able\nto determine which is the case: the first space is immediately followed\nby a second space if and only if the first space corresponds to \"\".\n\nAssuming that the above analysis is correct, the following procedure\nshould suffice to safely parse the output:\n\n - Let `i` be the index of the first space in `s`.\n - If `s[i+1]` is a space, let `sep_pos` be `i`. Otherwise, let\n `sep_pos` be `i - 1`.\n - The substring `s[:sep_pos]` is the token.\n - The character at index `sep_pos` is the separator.\n - The character at index `sep_pos + 1` is the fixed space.\n - The substring `s[sep_pos+2:nl]` is the value, where `nl` is the\n index of the first newline in `s` after `sep_pos`.\n\n(It seems unfortunately complicated when all we want to do is parse the\noutput of `--parse`, but I don't see a better approach!)\n\nMy questions:\n\n - Is this accurate?\n - Is this algorithm guaranteed to remain correct in future versions of\n Git?\n - Is there a simpler way to extract the token-value pairs from a\n commit message string?\n\nWould appreciate any advice.\n\nThanks!\nWC\n"},{"id":"365776","messageId":"CAP8UFD1ErRo7NQmCrAJLaELzV-1rKowyPsNCi3ecTqGN1qWxKQ@mail.gmail.com","threadId":"50099","inReplyTo":"CAFW+GMDazFSDzBrvzMqaPGwew=+CP7tw7G5FfDqcAUYd3qjPuQ@mail.gmail.com","subject":"Re: Parsing trailers","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-12-24T10:58:44Z","receivedAt":"2018-12-24T10:59:01Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Dec 23, 2018 at 11:44 PM William Chargin wrote:\n>\n> I'm interested in parsing the output of `git-interpret-trailers` in a\n> script. I had hoped that the `--parse` option would make this easy, but\n> it seems that the `trailer.separators` configuration option is used to\n> specify both the input format (which separators may indicate a trailer)\n> and the output format of `git interpret-trailers --parse`.\n\nYeah, but it can take many characters, not just one.\nFor example you might want to do something like:\n\nseps=$(git config trailer.separators)\ntest -z \"$seps\" && seps=':'\ngit -c trailer.separators=\"|$seps\" interpret-trailers --parse infile >outfile\n\nSo that the output uses '|' as a separator.\n\n> Given that\n> `trailer.separators` may contain any (non-NUL) characters, including\n> whitespace, parsing the output is not straightforward.\n\nChanging the default separator as shown above, should make it easier\nto parse the result.\n"},{"id":"365784","messageId":"CAFW+GMB4KW34qaOWNwShWM_ERSJtnvp86V_mFAse1biO=T-srw@mail.gmail.com","threadId":"50099","inReplyTo":"CAP8UFD1ErRo7NQmCrAJLaELzV-1rKowyPsNCi3ecTqGN1qWxKQ@mail.gmail.com","subject":"Re: Parsing trailers","fromName":"William Chargin","fromEmail":"wchargin@gmail.com","sentAt":"2018-12-24T18:52:29Z","receivedAt":"2018-12-24T18:52:48Z","isPatch":false,"sender":{"key":"wchargin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4317806?v=4"},"body":"Hi Christian: thanks for your reply.\n\n> Changing the default separator as shown above, should make it easier\n> to parse the result.\n\nBut this actually also changes which lines are considered trailers,\nright? If the commit message ends with\n\n Signed-off-by: one\n Signed-off-by| two\n\nand the user’s `trailer.separators` is set to `:`, then the correct\nresult should be only `Signed-off-by: one`. But when adding `|` as a\nseparator, we also see `Signed-off-by: two` in the result.\n\n $ printf '.\\n\\nSigned-off-by: one\\nSigned-off-by| two\\n' |\n > git interpret-trailers --parse\n Signed-off-by: one\n\n $ printf '.\\n\\nSigned-off-by: one\\nSigned-off-by| two\\n' |\n > git -c trailer.separators='|:' interpret-trailers --parse\n Signed-off-by| one\n Signed-off-by| two\n\nBest,\nWC\n"},{"id":"365807","messageId":"CAP8UFD2A1tX9qPZXr8ibPhy0B1zc5OtsWzP_XHoRJJF6hmkxRQ@mail.gmail.com","threadId":"50099","inReplyTo":"CAFW+GMB4KW34qaOWNwShWM_ERSJtnvp86V_mFAse1biO=T-srw@mail.gmail.com","subject":"Re: Parsing trailers","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-12-26T04:33:09Z","receivedAt":"2018-12-26T04:35:02Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi William,\n\nOn Mon, Dec 24, 2018 at 7:52 PM William Chargin wrote:\n>\n> Hi Christian: thanks for your reply.\n>\n> > Changing the default separator as shown above, should make it easier\n> > to parse the result.\n>\n> But this actually also changes which lines are considered trailers,\n> right?\n\nYes.\n\n> If the commit message ends with\n>\n> Signed-off-by: one\n> Signed-off-by| two\n>\n> and the user’s `trailer.separators` is set to `:`, then the correct\n> result should be only `Signed-off-by: one`. But when adding `|` as a\n> separator, we also see `Signed-off-by: two` in the result.\n\nYeah, but you can perhaps check that the input doesn't contain '|'\nbefore doing the above. If it does contain '|' then you can probably\nfind another char that it doesn't contain and use that char instead of\n'|'.\n\nAnother solution would be to develop a trailer.outputseparator config\noption, which should not be very difficult.\n\nBest,\nChristian.\n"},{"id":"365817","messageId":"CAFW+GMCP4_gce32Mhwn2Zf8DbH2YrJG4HhnLwV73+LcpQGWBPg@mail.gmail.com","threadId":"50099","inReplyTo":"CAP8UFD2A1tX9qPZXr8ibPhy0B1zc5OtsWzP_XHoRJJF6hmkxRQ@mail.gmail.com","subject":"Re: Parsing trailers","fromName":"William Chargin","fromEmail":"wchargin@gmail.com","sentAt":"2018-12-26T21:30:36Z","receivedAt":"2018-12-26T21:31:37Z","isPatch":false,"sender":{"key":"wchargin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4317806?v=4"},"body":"> Yeah, but you can perhaps check that the input doesn't contain '|'\n> before doing the above. If it does contain '|' then you can probably\n> find another char that it doesn't contain and use that char instead of\n> '|'.\n\nThis sounds true in the usual case, though of course there are\npathological cases of commit messages that use the entire character\nset. But it is starting to sound more complicated than the\nslightly-tricky whitespace indexing logic from my original message.\n\n> Another solution would be to develop a trailer.outputseparator config\n> option, which should not be very difficult.\n\nYes, something like this would be a nice addition for future versions of\nGit. Perhaps simpler would be to add a `-z` option to interpret-trailers\nthat would change the output format to NULNUL or similar.\nMaybe I’ll send out a patch if I find some free time. :-)\n\nBest,\nWC\n\nOn Tue, Dec 25, 2018 at 8:33 PM Christian Couder\n wrote:\n>\n> Hi William,\n>\n> On Mon, Dec 24, 2018 at 7:52 PM William Chargin wrote:\n> >\n> > Hi Christian: thanks for your reply.\n> >\n> > > Changing the default separator as shown above, should make it easier\n> > > to parse the result.\n> >\n> > But this actually also changes which lines are considered trailers,\n> > right?\n>\n> Yes.\n>\n> > If the commit message ends with\n> >\n> > Signed-off-by: one\n> > Signed-off-by| two\n> >\n> > and the user’s `trailer.separators` is set to `:`, then the correct\n> > result should be only `Signed-off-by: one`. But when adding `|` as a\n> > separator, we also see `Signed-off-by: two` in the result.\n>\n> Yeah, but you can perhaps check that the input doesn't contain '|'\n> before doing the above. If it does contain '|' then you can probably\n> find another char that it doesn't contain and use that char instead of\n> '|'.\n>\n> Another solution would be to develop a trailer.outputseparator config\n> option, which should not be very difficult.\n>\n> Best,\n> Christian.\n"},{"id":"366071","messageId":"20190103070744.GA24149@sigill.intra.peff.net","threadId":"50099","inReplyTo":"CAFW+GMDazFSDzBrvzMqaPGwew=+CP7tw7G5FfDqcAUYd3qjPuQ@mail.gmail.com","subject":"Re: Parsing trailers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-03T07:07:44Z","receivedAt":"2019-01-03T07:07:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 23, 2018 at 02:41:20PM -0800, William Chargin wrote:\n\n> I'm interested in parsing the output of `git-interpret-trailers` in a\n> script. I had hoped that the `--parse` option would make this easy, but\n> it seems that the `trailer.separators` configuration option is used to\n> specify both the input format (which separators may indicate a trailer)\n> and the output format of `git interpret-trailers --parse`. Given that\n> `trailer.separators` may contain any (non-NUL) characters, including\n> whitespace, parsing the output is not straightforward.\n\nIMHO this is a bug in --parse. It was always meant to give sane,\nnormalized output, that you could parse with something like:\n\n [^:]+: .*\n\nThat's what \"%(trailers:only)\" does (even if the original separator was\nsomething else). It also trims any extra whitespace.\n\nSo I think it would be reasonable to:\n\n 1. Add an --output-separator option. This should be uncontroversial.\n\n 2. Make --parse imply \"--output-separator=:\". This might be more\n controversial, because it does change the output. But as I said\n above, I consider the current behavior to simply be a bug.\n\n 3. (Optional) Add a \"-z\" option which uses \"\\0\" both\n within and between trailer records. This obviously solves your\n problem without steps 1 and 2, but I think we should have a way to\n do it without relying on NULs, since they're harder to work with in\n some tools.\n\n-Peff\n"},{"id":"366076","messageId":"CAFW+GMD9P-E4xCP+K5JAX70a6cgoHUzOXU-Tv3w6yhKGT_GaEg@mail.gmail.com","threadId":"50099","inReplyTo":"20190103070744.GA24149@sigill.intra.peff.net","subject":"Re: Parsing trailers","fromName":"William Chargin","fromEmail":"wchargin@gmail.com","sentAt":"2019-01-03T07:43:55Z","receivedAt":"2019-01-03T07:44:14Z","isPatch":false,"sender":{"key":"wchargin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4317806?v=4"},"body":"> That's what \"%(trailers:only)\" does (even if the original separator was\n> something else). It also trims any extra whitespace.\n\nOoh, this is good to know: thanks. (I had found `print_tok_val` in\n`trailer.c` and assumed that this was the only place with the output\nlogic, but I now see that `format_trailer_info` also exists in the same\nfile.)\n\n> IMHO this is a bug in --parse. It was always meant to give sane,\n> normalized output\n\nOkay; this is good to hear. In that case, what would you think about\nchanging `interpret-trailers` as a whole to always emit colons? (Note\nthat the command is already lossy: even with no flags, it replaces each\nseparator character with the first character of `trailer.separators`.)\nThis has the advantage that we avoid adding a configuration option of\ndubious value—it’s not clear to me why a user would actually _want_ to\nchange the separator to anything other than a colon. The patch should be\nquite simple, too (only tested lightly on my machine):\n\ndiff --git a/trailer.c b/trailer.c\nindex 0796f326b3..722040e48c 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -156,7 +156,7 @@ static void print_tok_val(FILE *outfile, const\nchar *tok, const char *val)\n if (strchr(separators, c))\n fprintf(outfile, \"%s%s\\n\", tok, val);\n else\n- fprintf(outfile, \"%s%c %s\\n\", tok, separators[0], val);\n+ fprintf(outfile, \"%s: %s\\n\", tok, val);\n }\n\nIs this veering too much from “bug fix” toward “backward-incompatible\nbehavior change” for your comfort?\n\nI agree that either this or your (1) and (2) would eliminate the need\nfor `-z`, which is nice.\n\nBest,\nWC\n"},{"id":"366078","messageId":"20190103075045.GD24925@sigill.intra.peff.net","threadId":"50099","inReplyTo":"CAFW+GMD9P-E4xCP+K5JAX70a6cgoHUzOXU-Tv3w6yhKGT_GaEg@mail.gmail.com","subject":"Re: Parsing trailers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-03T07:50:45Z","receivedAt":"2019-01-03T07:50:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 02, 2019 at 11:43:55PM -0800, William Chargin wrote:\n\n> > IMHO this is a bug in --parse. It was always meant to give sane,\n> > normalized output\n> \n> Okay; this is good to hear. In that case, what would you think about\n> changing `interpret-trailers` as a whole to always emit colons? (Note\n> that the command is already lossy: even with no flags, it replaces each\n> separator character with the first character of `trailer.separators`.)\n> This has the advantage that we avoid adding a configuration option of\n> dubious value—it’s not clear to me why a user would actually _want_ to\n> change the separator to anything other than a colon. The patch should be\n> quite simple, too (only tested lightly on my machine):\n> \n> diff --git a/trailer.c b/trailer.c\n> index 0796f326b3..722040e48c 100644\n> --- a/trailer.c\n> +++ b/trailer.c\n> @@ -156,7 +156,7 @@ static void print_tok_val(FILE *outfile, const\n> char *tok, const char *val)\n> if (strchr(separators, c))\n> fprintf(outfile, \"%s%s\\n\", tok, val);\n> else\n> - fprintf(outfile, \"%s%c %s\\n\", tok, separators[0], val);\n> + fprintf(outfile, \"%s: %s\\n\", tok, val);\n> }\n> \n> Is this veering too much from “bug fix” toward “backward-incompatible\n> behavior change” for your comfort?\n\nI think that gets weird in non-parse modes. For example, if I'm not\ntrying to parse, but rather to _add_ a new trailer (because I'm writing\nout a commit message to feed to git-commit-tree), then I'd presumably\nwant the normal configured separators.\n\nI dunno. I don't think I've ever seen a trailer with a non-colon\nseparator, nor have I ever used interpret-trailers for anything except\nparsing. But it obviously was designed with more flexibility in mind,\nand I suspect the patch above has a high chance of breaking something\nsomewhere. Tying it to --parse seems a lot safer, since that was\nintroduced exactly for this case.\n\n-Peff\n"}]}