{"thread":{"id":"62317","subject":"[RFC PATCH] builtin/shortlog: explicitly set hash algo when there is no repo","startedAt":"2024-10-11T18:40:45Z","lastAt":"2024-10-17T22:02:54Z","messageCount":36,"participants":["Wolfgang Müller","Kristoffer Haugsbakk","Eric Sunshine","Taylor Blau","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"504873","messageId":"20241011183445.229228-1-wolf@oriole.systems","threadId":"62317","inReplyTo":null,"subject":"[RFC PATCH] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Wolfgang Müller","fromEmail":"wolf@oriole.systems","sentAt":"2024-10-11T18:34:45Z","receivedAt":"2024-10-11T18:40:45Z","isPatch":true,"sender":{"key":"wolf@oriole.systems","avatar":"https://avatars.githubusercontent.com/u/397518?v=4"},"body":"Whilst git-shortlog(1) does not explicitly need any repository\ninformation when run without reference to one, it still parses some of\nits arguments with parse_revision_opt() which assumes that the hash\nalgorithm is set. However, in c8aed5e8da (repository: stop setting SHA1\nas the default object hash, 2024-05-07) we stopped setting up a default\nhash algorithm and instead require commands to set it up explicitly.\n\nThis was done for most other commands like in ab274909d4 (builtin/diff:\nexplicitly set hash algo when there is no repo, 2024-05-07) but was\nmissed for builtin/shortlog, making git-shortlog(1) segfault outside of\na repository when given arguments like --author that trigger a call to\nparse_revision_opt().\n\nFix this for now by explicitly setting the hash algorithm to SHA1.\n\nSigned-off-by: Wolfgang Müller \n---\nI think there is a much cleaner patch here if one would look at\ndisentangling the parsing machinery split between cmd_shortlog() and\nparse_revision_opt() such that the latter is only called if there is an\nactual repository, but I'm not at all familiar enough with the codebase\nto do that. Like the commit says some other commands were fixed like\nthis as well, so I thought to send this patch your way.\n\n builtin/shortlog.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 3ed5c46078..6422f088b2 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -387,6 +387,14 @@ int cmd_shortlog(int argc,\n \tstruct rev_info rev;\n \tint nongit = !startup_info->have_repository;\n \n+\t/*\n+\t * Later on we'll call parse_revision_opt which relies on the hash\n+\t * algorithm being set but since we are operating outside of a Git\n+\t * repository we cannot determine one. For now default to SHA1.\n+\t */\n+\tif (nongit && !the_hash_algo)\n+\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n+\n \tconst struct option options[] = {\n \t\tOPT_BIT('c', \"committer\", &log.groups,\n \t\t\tN_(\"group by committer rather than author\"),\n\nbase-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2\n-- \n2.47.0\n\n"},{"id":"505127","messageId":"r4btv3hmwv6haflwhmjbjshova4wnth5mkbknsuuqcvose5lix@bvy5e6gxnit6","threadId":"62317","inReplyTo":"20241011183445.229228-1-wolf@oriole.systems","subject":"Re: [RFC PATCH] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Wolfgang Müller","fromEmail":"wolf@oriole.systems","sentAt":"2024-10-15T09:33:35Z","receivedAt":"2024-10-15T09:33:44Z","isPatch":true,"sender":{"key":"wolf@oriole.systems","avatar":"https://avatars.githubusercontent.com/u/397518?v=4"},"body":"On 2024-10-11 20:34, Wolfgang Müller wrote:\n> Whilst git-shortlog(1) does not explicitly need any repository\n> information when run without reference to one, it still parses some of\n> its arguments with parse_revision_opt() which assumes that the hash\n> algorithm is set. However, in c8aed5e8da (repository: stop setting SHA1\n> as the default object hash, 2024-05-07) we stopped setting up a default\n> hash algorithm and instead require commands to set it up explicitly.\n> \n> This was done for most other commands like in ab274909d4 (builtin/diff:\n> explicitly set hash algo when there is no repo, 2024-05-07) but was\n> missed for builtin/shortlog, making git-shortlog(1) segfault outside of\n> a repository when given arguments like --author that trigger a call to\n> parse_revision_opt().\n> \n> Fix this for now by explicitly setting the hash algorithm to SHA1.\n> \n> Signed-off-by: Wolfgang Müller \n> ---\n> I think there is a much cleaner patch here if one would look at\n> disentangling the parsing machinery split between cmd_shortlog() and\n> parse_revision_opt() such that the latter is only called if there is an\n> actual repository, but I'm not at all familiar enough with the codebase\n> to do that. Like the commit says some other commands were fixed like\n> this as well, so I thought to send this patch your way.\n\nWas this topic maybe missed? It's not a critical fix but would solve a\nsegfault that is pretty easy for a user to trigger.\n\nThanks!\n\n-- \nWolf\n"},{"id":"505132","messageId":"08bdd8dd-06d8-47a6-8b44-fa6add14d60a@app.fastmail.com","threadId":"62317","inReplyTo":"r4btv3hmwv6haflwhmjbjshova4wnth5mkbknsuuqcvose5lix@bvy5e6gxnit6","subject":"Re: [RFC PATCH] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-10-15T09:47:13Z","receivedAt":"2024-10-15T09:47:35Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Tue, Oct 15, 2024, at 11:33, Wolfgang Müller wrote:\n> > […]\n>\n> Was this topic maybe missed? It's not a critical fix but would solve a\n> segfault that is pretty easy for a user to trigger.\n\nI can confirm that this gives me a segfault:\n\n```\n# Directory which is not tracked\ncd ~\ngit shortlog --author=Kristoffer\n```\n\nMaybe a few things could make the change more ready to be included:\n\n1. A regression test for that segfault\n2. A quickfix (which you indicate below the three-dash/hyphens) can be\n documented in the comment above the code with a `NEEDSWORK`\n\n-- \nKristoffer Haugsbakk\n\n"},{"id":"505157","messageId":"20241015114826.715158-1-wolf@oriole.systems","threadId":"62317","inReplyTo":"20241011183445.229228-1-wolf@oriole.systems","subject":"[RFC PATCH v2] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Wolfgang Müller","fromEmail":"wolf@oriole.systems","sentAt":"2024-10-15T11:48:26Z","receivedAt":"2024-10-15T11:48:45Z","isPatch":true,"sender":{"key":"wolf@oriole.systems","avatar":"https://avatars.githubusercontent.com/u/397518?v=4"},"body":"Whilst git-shortlog(1) does not explicitly need any repository\ninformation when run without reference to one, it still parses some of\nits arguments with parse_revision_opt() which assumes that the hash\nalgorithm is set. However, in c8aed5e8da (repository: stop setting SHA1\nas the default object hash, 2024-05-07) we stopped setting up a default\nhash algorithm and instead require commands to set it up explicitly.\n\nThis was done for most other commands like in ab274909d4 (builtin/diff:\nexplicitly set hash algo when there is no repo, 2024-05-07) but was\nmissed for builtin/shortlog, making git-shortlog(1) segfault outside of\na repository when given arguments like --author that trigger a call to\nparse_revision_opt().\n\nFix this for now by explicitly setting the hash algorithm to SHA1. Also\nadd a regression test for the segfault.\n\nSigned-off-by: Wolfgang Müller \n---\n builtin/shortlog.c | 12 ++++++++++++\n t/t4201-shortlog.sh | 5 +++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 3ed5c46078..0fa35202ed 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -387,6 +387,18 @@ int cmd_shortlog(int argc,\n \tstruct rev_info rev;\n \tint nongit = !startup_info->have_repository;\n \n+\t/*\n+\t * NEEDSWORK: Later on we'll call parse_revision_opt which relies on\n+\t * the hash algorithm being set but since we are operating outside of a\n+\t * Git repository we cannot determine one. This is only needed because\n+\t * parse_revision_opt expects hexsz for --abbrev which is irrelevant\n+\t * for shortlog outside of a git repository. For now explicitly set\n+\t * SHA1, but ideally the parsing machinery would be split between\n+\t * git/nongit so that we do not have to do this.\n+\t */\n+\tif (nongit && !the_hash_algo)\n+\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n+\n \tconst struct option options[] = {\n \t\tOPT_BIT('c', \"committer\", &log.groups,\n \t\t\tN_(\"group by committer rather than author\"),\ndiff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\nindex c20c885724..ed39c67ba1 100755\n--- a/t/t4201-shortlog.sh\n+++ b/t/t4201-shortlog.sh\n@@ -143,6 +143,11 @@ fuzz()\n \ttest_grep \"too many arguments\" out\n '\n \n+test_expect_success 'shortlog --author from non-git directory does not segfault' '\n+\tgit log --no-expand-tabs HEAD >log &&\n+\tenv GIT_DIR=non-existing git shortlog --author=author out\n+'\n+\n test_expect_success 'shortlog should add newline when input line matches wraplen' '\n \tcat >expect <<\\EOF &&\n A U Thor (2):\n\nRange-diff against v1:\n1: 42516cc02d ! 1: d3047a0291 builtin/shortlog: explicitly set hash algo when there is no repo\n @@ Commit message\n a repository when given arguments like --author that trigger a call to\n parse_revision_opt().\n \n - Fix this for now by explicitly setting the hash algorithm to SHA1.\n + Fix this for now by explicitly setting the hash algorithm to SHA1. Also\n + add a regression test for the segfault.\n \n Signed-off-by: Wolfgang Müller \n \n @@ builtin/shortlog.c: int cmd_shortlog(int argc,\n \tint nongit = !startup_info->have_repository;\n \n +\t/*\n -+\t * Later on we'll call parse_revision_opt which relies on the hash\n -+\t * algorithm being set but since we are operating outside of a Git\n -+\t * repository we cannot determine one. For now default to SHA1.\n ++\t * NEEDSWORK: Later on we'll call parse_revision_opt which relies on\n ++\t * the hash algorithm being set but since we are operating outside of a\n ++\t * Git repository we cannot determine one. This is only needed because\n ++\t * parse_revision_opt expects hexsz for --abbrev which is irrelevant\n ++\t * for shortlog outside of a git repository. For now explicitly set\n ++\t * SHA1, but ideally the parsing machinery would be split between\n ++\t * git/nongit so that we do not have to do this.\n +\t */\n +\tif (nongit && !the_hash_algo)\n +\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n @@ builtin/shortlog.c: int cmd_shortlog(int argc,\n \tconst struct option options[] = {\n \t\tOPT_BIT('c', \"committer\", &log.groups,\n \t\t\tN_(\"group by committer rather than author\"),\n +\n + ## t/t4201-shortlog.sh ##\n +@@ t/t4201-shortlog.sh: fuzz()\n + \ttest_grep \"too many arguments\" out\n + '\n + \n ++test_expect_success 'shortlog --author from non-git directory does not segfault' '\n ++\tgit log --no-expand-tabs HEAD >log &&\n ++\tenv GIT_DIR=non-existing git shortlog --author=author out\n ++'\n ++\n + test_expect_success 'shortlog should add newline when input line matches wraplen' '\n + \tcat >expect <<\\EOF &&\n + A U Thor (2):\n-- \n2.47.0\n\n"},{"id":"505179","messageId":"CAPig+cSGzgfa+0MbRD10vktmx3O3v3q_TremWs7dANrwALf2Fg@mail.gmail.com","threadId":"62317","inReplyTo":"20241015114826.715158-1-wolf@oriole.systems","subject":"Re: [RFC PATCH v2] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-10-15T17:20:20Z","receivedAt":"2024-10-15T17:20:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Oct 15, 2024 at 7:48 AM Wolfgang Müller wrote:\n> Whilst git-shortlog(1) does not explicitly need any repository\n> information when run without reference to one, it still parses some of\n> its arguments with parse_revision_opt() which assumes that the hash\n> algorithm is set. However, in c8aed5e8da (repository: stop setting SHA1\n> as the default object hash, 2024-05-07) we stopped setting up a default\n> hash algorithm and instead require commands to set it up explicitly.\n>\n> This was done for most other commands like in ab274909d4 (builtin/diff:\n> explicitly set hash algo when there is no repo, 2024-05-07) but was\n> missed for builtin/shortlog, making git-shortlog(1) segfault outside of\n> a repository when given arguments like --author that trigger a call to\n> parse_revision_opt().\n>\n> Fix this for now by explicitly setting the hash algorithm to SHA1. Also\n> add a regression test for the segfault.\n>\n> Signed-off-by: Wolfgang Müller \n\nCc:'ing Patrick since I believe he's been patching similar cases.\n\n> diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\n> @@ -143,6 +143,11 @@ fuzz()\n> +test_expect_success 'shortlog --author from non-git directory does not segfault' '\n> + git log --no-expand-tabs HEAD >log &&\n> + env GIT_DIR=non-existing git shortlog --author=author out\n> +'\n\nt/test-lib-functions.sh has a handy nongit() function for running a\ncommand in a non-Git directory:\n\n nongit git shortlog --author=author out\n\nBy the way, what is the purpose of capturing output to file \"out\"?\nThat file is never consulted.\n\nAlso, can the original crash be reproduced without having to invoke\nthe additional `git log`? In my tests, this is sufficient:\n\n echo | nongit git shortlog --author=author\n"},{"id":"505182","messageId":"2j4vvyo2nthifofp3susoq5qmbyoocht7pjw6bzrxoffuui2ca@pbpxtwnyrr66","threadId":"62317","inReplyTo":"CAPig+cSGzgfa+0MbRD10vktmx3O3v3q_TremWs7dANrwALf2Fg@mail.gmail.com","subject":"Re: [RFC PATCH v2] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Wolfgang Müller","fromEmail":"wolf@oriole.systems","sentAt":"2024-10-15T17:51:17Z","receivedAt":"2024-10-15T17:51:20Z","isPatch":true,"sender":{"key":"wolf@oriole.systems","avatar":"https://avatars.githubusercontent.com/u/397518?v=4"},"body":"On 2024-10-15 13:20, Eric Sunshine wrote:\n> t/test-lib-functions.sh has a handy nongit() function for running a\n> command in a non-Git directory:\n> \n> nongit git shortlog --author=author out\n\nAh, nice, I'll be using that instead, then.\n\n> By the way, what is the purpose of capturing output to file \"out\"?\n> That file is never consulted.\n\nOh, my bad, that was a leftover from local testing.\n\n> Also, can the original crash be reproduced without having to invoke\n> the additional `git log`? In my tests, this is sufficient:\n> \n> echo | nongit git shortlog --author=author\n\nYeah, same as above. I'll take that into account for the next version,\nthanks!\n\nThinking more about this, any opinion on the specific test for --author?\nIt is a succinct test but also relies on the fact that it \"just happens\"\nto be one of the flags handled in parse_revision_opt(). In fact, any\nsuch flag (even non-existing ones, like \"--foo\") would crash shortlog\nright now. I'm wondering whether there's a more universal way of testing\nthis.\n\n-- \nWolf\n"},{"id":"505205","messageId":"Zw7IiwK5mi1rmqFN@nand.local","threadId":"62317","inReplyTo":"r4btv3hmwv6haflwhmjbjshova4wnth5mkbknsuuqcvose5lix@bvy5e6gxnit6","subject":"Re: [RFC PATCH] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-15T19:54:51Z","receivedAt":"2024-10-15T19:54:54Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 15, 2024 at 11:33:35AM +0200, Wolfgang Müller wrote:\n> On 2024-10-11 20:34, Wolfgang Müller wrote:\n> > Whilst git-shortlog(1) does not explicitly need any repository\n> > information when run without reference to one, it still parses some of\n> > its arguments with parse_revision_opt() which assumes that the hash\n> > algorithm is set. However, in c8aed5e8da (repository: stop setting SHA1\n> > as the default object hash, 2024-05-07) we stopped setting up a default\n> > hash algorithm and instead require commands to set it up explicitly.\n> >\n> > This was done for most other commands like in ab274909d4 (builtin/diff:\n> > explicitly set hash algo when there is no repo, 2024-05-07) but was\n> > missed for builtin/shortlog, making git-shortlog(1) segfault outside of\n> > a repository when given arguments like --author that trigger a call to\n> > parse_revision_opt().\n> >\n> > Fix this for now by explicitly setting the hash algorithm to SHA1.\n> >\n> > Signed-off-by: Wolfgang Müller \n> > ---\n> > I think there is a much cleaner patch here if one would look at\n> > disentangling the parsing machinery split between cmd_shortlog() and\n> > parse_revision_opt() such that the latter is only called if there is an\n> > actual repository, but I'm not at all familiar enough with the codebase\n> > to do that. Like the commit says some other commands were fixed like\n> > this as well, so I thought to send this patch your way.\n>\n> Was this topic maybe missed? It's not a critical fix but would solve a\n> segfault that is pretty easy for a user to trigger.\n\nYeah. It looks like this was sent out on Friday, which was Junio's last\nworking day before vacation. I think we probably both assumed the other\npicked it up ;-).\n\nThanks,\nTaylor\n"},{"id":"505215","messageId":"Zw76iYXJQq9gJCj8@nand.local","threadId":"62317","inReplyTo":"Zw7IiwK5mi1rmqFN@nand.local","subject":"Re: [RFC PATCH] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-15T23:28:09Z","receivedAt":"2024-10-15T23:28:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 15, 2024 at 03:54:51PM -0400, Taylor Blau wrote:\n> On Tue, Oct 15, 2024 at 11:33:35AM +0200, Wolfgang Müller wrote:\n> > On 2024-10-11 20:34, Wolfgang Müller wrote:\n> > > Whilst git-shortlog(1) does not explicitly need any repository\n> > > information when run without reference to one, it still parses some of\n> > > its arguments with parse_revision_opt() which assumes that the hash\n> > > algorithm is set. However, in c8aed5e8da (repository: stop setting SHA1\n> > > as the default object hash, 2024-05-07) we stopped setting up a default\n> > > hash algorithm and instead require commands to set it up explicitly.\n> > >\n> > > This was done for most other commands like in ab274909d4 (builtin/diff:\n> > > explicitly set hash algo when there is no repo, 2024-05-07) but was\n> > > missed for builtin/shortlog, making git-shortlog(1) segfault outside of\n> > > a repository when given arguments like --author that trigger a call to\n> > > parse_revision_opt().\n> > >\n> > > Fix this for now by explicitly setting the hash algorithm to SHA1.\n> > >\n> > > Signed-off-by: Wolfgang Müller \n> > > ---\n> > > I think there is a much cleaner patch here if one would look at\n> > > disentangling the parsing machinery split between cmd_shortlog() and\n> > > parse_revision_opt() such that the latter is only called if there is an\n> > > actual repository, but I'm not at all familiar enough with the codebase\n> > > to do that. Like the commit says some other commands were fixed like\n> > > this as well, so I thought to send this patch your way.\n> >\n> > Was this topic maybe missed? It's not a critical fix but would solve a\n> > segfault that is pretty easy for a user to trigger.\n>\n> Yeah. It looks like this was sent out on Friday, which was Junio's last\n> working day before vacation. I think we probably both assumed the other\n> picked it up ;-).\n\nI picked this up today and applied it to 'seen', but ejected it before\npushing the result out, as this appears to break CI, presumably due to\nthe mixed declaration and code in the patch.\n\nThanks,\nTaylor\n"},{"id":"505226","messageId":"Zw9P90_QtiAAH3yz@pks.im","threadId":"62317","inReplyTo":"20241015114826.715158-1-wolf@oriole.systems","subject":"Re: [RFC PATCH v2] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-16T05:32:45Z","receivedAt":"2024-10-16T05:32:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Oct 15, 2024 at 01:48:26PM +0200, Wolfgang Müller wrote:\n> +\t/*\n> +\t * NEEDSWORK: Later on we'll call parse_revision_opt which relies on\n> +\t * the hash algorithm being set but since we are operating outside of a\n> +\t * Git repository we cannot determine one. This is only needed because\n> +\t * parse_revision_opt expects hexsz for --abbrev which is irrelevant\n> +\t * for shortlog outside of a git repository. For now explicitly set\n> +\t * SHA1, but ideally the parsing machinery would be split between\n> +\t * git/nongit so that we do not have to do this.\n> +\t */\n> +\tif (nongit && !the_hash_algo)\n> +\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n> +\n\nMakes sense. As you say, git-shortlog(1) should ideally recognize the\nhash function used by the input. The next-best thing would be to provide\na command line option to switch it. But punting on that should be fine\nfor now.\n\n> \tconst struct option options[] = {\n> \t\tOPT_BIT('c', \"committer\", &log.groups,\n> \t\t\tN_(\"group by committer rather than author\"),\n> diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\n> index c20c885724..ed39c67ba1 100755\n> --- a/t/t4201-shortlog.sh\n> +++ b/t/t4201-shortlog.sh\n> @@ -143,6 +143,11 @@ fuzz()\n> \ttest_grep \"too many arguments\" out\n> '\n> \n> +test_expect_success 'shortlog --author from non-git directory does not segfault' '\n> +\tgit log --no-expand-tabs HEAD >log &&\n> +\tenv GIT_DIR=non-existing git shortlog --author=author out\n> +'\n> +\n\nI'd like to see another testcase added that exercises behaviour when\ngit-shortlog(1) is passed SHA256 output outside of a repo, as I'm\ncurious how it'll behave in that scenario.\n\nThanks!\n\nPatrick\n"},{"id":"505250","messageId":"whst5ap7snkdns5k47tysuguusad5jp7vybahirshkypufhk2s@pnar2yiwcphv","threadId":"62317","inReplyTo":"Zw76iYXJQq9gJCj8@nand.local","subject":"Re: [RFC PATCH] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Wolfgang Müller","fromEmail":"wolf@oriole.systems","sentAt":"2024-10-16T08:15:31Z","receivedAt":"2024-10-16T08:15:37Z","isPatch":true,"sender":{"key":"wolf@oriole.systems","avatar":"https://avatars.githubusercontent.com/u/397518?v=4"},"body":"On 2024-10-15 19:28, Taylor Blau wrote:\n> I picked this up today and applied it to 'seen', but ejected it before\n> pushing the result out, as this appears to break CI, presumably due to\n> the mixed declaration and code in the patch.\n\nOh, I had missed setting DEVELOPER=1 in my builds so I didn't see this\nfailure myself. Will fix in the upcoming version.\n\n-- \nWolf\n"},{"id":"505254","messageId":"gtdjnvonjodr435wuv2pofnww2pdcd33s22xxz5ypwkpvdythc@vrajz5ttkjbz","threadId":"62317","inReplyTo":"Zw9P90_QtiAAH3yz@pks.im","subject":"Re: [RFC PATCH v2] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Wolfgang Müller","fromEmail":"wolf@oriole.systems","sentAt":"2024-10-16T08:47:03Z","receivedAt":"2024-10-16T08:47:06Z","isPatch":true,"sender":{"key":"wolf@oriole.systems","avatar":"https://avatars.githubusercontent.com/u/397518?v=4"},"body":"On 2024-10-16 07:32, Patrick Steinhardt wrote:\n> On Tue, Oct 15, 2024 at 01:48:26PM +0200, Wolfgang Müller wrote:\n> > diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\n> > index c20c885724..ed39c67ba1 100755\n> > --- a/t/t4201-shortlog.sh\n> > +++ b/t/t4201-shortlog.sh\n> > @@ -143,6 +143,11 @@ fuzz()\n> > \ttest_grep \"too many arguments\" out\n> > '\n> > \n> > +test_expect_success 'shortlog --author from non-git directory does not segfault' '\n> > +\tgit log --no-expand-tabs HEAD >log &&\n> > +\tenv GIT_DIR=non-existing git shortlog --author=author out\n> > +'\n> > +\n> \n> I'd like to see another testcase added that exercises behaviour when\n> git-shortlog(1) is passed SHA256 output outside of a repo, as I'm\n> curious how it'll behave in that scenario.\n\nI had a look at this in builtin/shortlog.c's read_from_stdin() and am\npretty sure that git-shortlog(1), when reading from stdin, simply\nignores anything but either the \"Author:\" or \"Commit:\" lines, depending\non the value given by --group. The --group=format: option is not\nsupported when reading from stdin. Neither is --format handled at all.\n\nSo I don't think there is actually a way to make git-shortlog(1)\nencounter and handle a commit hash when reading from stdin; the hash\nalgorithm seems completely meaningless for its user-facing behaviour. As\nfar as I have seen the closest it comes to getting into contact with a\nhash (or more specifically, hexsz) is when cmd_shortlog() sets:\n\n\tlog.abbrev = rev.abbrev;\n\nThis relies on the parsing machinery in parse_revision_opt() - the one\nthis patch is for. Technically --abbrev is honored by git-shortlog(1)\nwhen reading from stdin, but its value goes unused because of the\ndifference in code paths when reading from stdin. Do take this with a\ngrain of salt, however, this is my first look at the inner workings of\ngit-shortlog(1).\n\nAs for the test, I'd be happy to provide one if this is still deemed\nnecessary after considering the above. There's two questions I have:\n\n1) Is this already covered by GIT_TEST_DEFAULT_HASH=sha256? Running the\nt4201-shortlog testsuite with that passes.\n\n2) I've already experimented with setting up a test for this and am\nunsure how to cleanly set up a sha256 repository. Ordinarily it should\nbe a simple init/add (perhaps with test_commit), but t4201-shortlog is\nalready running within a git repository if I understand the setup step\ncorrectly. Is there a clean way to escape from there, or would it simply\nbe fine creating another repository within that one?\n\nThanks!\n\n-- \nWolf\n"},{"id":"505256","messageId":"Zw9_46fbvbGhjmYw@pks.im","threadId":"62317","inReplyTo":"gtdjnvonjodr435wuv2pofnww2pdcd33s22xxz5ypwkpvdythc@vrajz5ttkjbz","subject":"Re: [RFC PATCH v2] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-16T08:57:13Z","receivedAt":"2024-10-16T08:57:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Oct 16, 2024 at 10:47:03AM +0200, Wolfgang Müller wrote:\n> On 2024-10-16 07:32, Patrick Steinhardt wrote:\n> > On Tue, Oct 15, 2024 at 01:48:26PM +0200, Wolfgang Müller wrote:\n> > > diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\n> > > index c20c885724..ed39c67ba1 100755\n> > > --- a/t/t4201-shortlog.sh\n> > > +++ b/t/t4201-shortlog.sh\n> > > @@ -143,6 +143,11 @@ fuzz()\n> > > \ttest_grep \"too many arguments\" out\n> > > '\n> > > \n> > > +test_expect_success 'shortlog --author from non-git directory does not segfault' '\n> > > +\tgit log --no-expand-tabs HEAD >log &&\n> > > +\tenv GIT_DIR=non-existing git shortlog --author=author out\n> > > +'\n> > > +\n> > \n> > I'd like to see another testcase added that exercises behaviour when\n> > git-shortlog(1) is passed SHA256 output outside of a repo, as I'm\n> > curious how it'll behave in that scenario.\n> \n> I had a look at this in builtin/shortlog.c's read_from_stdin() and am\n> pretty sure that git-shortlog(1), when reading from stdin, simply\n> ignores anything but either the \"Author:\" or \"Commit:\" lines, depending\n> on the value given by --group. The --group=format: option is not\n> supported when reading from stdin. Neither is --format handled at all.\n> \n> So I don't think there is actually a way to make git-shortlog(1)\n> encounter and handle a commit hash when reading from stdin; the hash\n> algorithm seems completely meaningless for its user-facing behaviour. As\n> far as I have seen the closest it comes to getting into contact with a\n> hash (or more specifically, hexsz) is when cmd_shortlog() sets:\n> \n> \tlog.abbrev = rev.abbrev;\n> \n> This relies on the parsing machinery in parse_revision_opt() - the one\n> this patch is for. Technically --abbrev is honored by git-shortlog(1)\n> when reading from stdin, but its value goes unused because of the\n> difference in code paths when reading from stdin. Do take this with a\n> grain of salt, however, this is my first look at the inner workings of\n> git-shortlog(1).\n\nOkay, thanks for the explanation.\n\nGiven that we do set `log.abbrev` I think we should be hitting code\npaths in git-shortlog(1) that use it. `git shortlog --format=%h` for\nexample would use `log.abbrev`, wouldn't it? It would be nice to figure\nout whether this can be made to misbehave based on which object hash we\nhave in the input.\n\n> As for the test, I'd be happy to provide one if this is still deemed\n> necessary after considering the above. There's two questions I have:\n> \n> 1) Is this already covered by GIT_TEST_DEFAULT_HASH=sha256? Running the\n> t4201-shortlog testsuite with that passes.\n\nI think it doesn't hurt to have an explicit test for this scenario, even\nif it just demonstrates that things don't crash or behave in weird ways.\n\n> 2) I've already experimented with setting up a test for this and am\n> unsure how to cleanly set up a sha256 repository. Ordinarily it should\n> be a simple init/add (perhaps with test_commit), but t4201-shortlog is\n> already running within a git repository if I understand the setup step\n> correctly. Is there a clean way to escape from there, or would it simply\n> be fine creating another repository within that one?\n\nYou can take e.g. b2dbf97f47 (builtin/index-pack: fix segfaults when\nrunning outside of a repo, 2024-09-04) as inspiration for how to achieve\nthis.\n\nPatrick\n"},{"id":"505258","messageId":"yxfpbmwy5bhtjwmffzrfoagugs4rsxw7wn2cvve5ygzcnlolpu@lhjgpz4riogd","threadId":"62317","inReplyTo":"Zw9_46fbvbGhjmYw@pks.im","subject":"Re: [RFC PATCH v2] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Wolfgang Müller","fromEmail":"wolf@oriole.systems","sentAt":"2024-10-16T09:07:02Z","receivedAt":"2024-10-16T09:07:05Z","isPatch":true,"sender":{"key":"wolf@oriole.systems","avatar":"https://avatars.githubusercontent.com/u/397518?v=4"},"body":"On 2024-10-16 10:57, Patrick Steinhardt wrote:\n> Given that we do set `log.abbrev` I think we should be hitting code\n> paths in git-shortlog(1) that use it. `git shortlog --format=%h` for\n> example would use `log.abbrev`, wouldn't it? It would be nice to figure\n> out whether this can be made to misbehave based on which object hash we\n> have in the input.\n\nI did try this, but like I said, --format seems not supported when\ngit-shortlog(1) is reading from stdin. It always outputs the same\nsummary format, either grouped on authors or committers. This is not\nexplicitly documented anywhere - the manual only says that \"git shortlog\nwill output a summary of the log read from standard input\" and then goes\non to document all the options with no mention of a difference in\nbehaviour when reading from stdin. So I'm still not entirely convinced\nthat this is impossible to trigger (also given the complexity of the\nargument parsing machinery), but I have not been able to find a way.\n\n> I think it doesn't hurt to have an explicit test for this scenario, even\n> if it just demonstrates that things don't crash or behave in weird ways.\n\nSure, that makes sense.\n\n> You can take e.g. b2dbf97f47 (builtin/index-pack: fix segfaults when\n> running outside of a repo, 2024-09-04) as inspiration for how to achieve\n> this.\n\nThanks much for the pointer, I'll have a look!\n\n-- \nWolf\n"},{"id":"505261","messageId":"w6vzuumbd5vrdluvnheescdgw36teuj6k5anlyiphvpr6slcgc@2gkwe2ye26a6","threadId":"62317","inReplyTo":"Zw9_46fbvbGhjmYw@pks.im","subject":"Re: [RFC PATCH v2] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Wolfgang Müller","fromEmail":"wolf@oriole.systems","sentAt":"2024-10-16T09:48:24Z","receivedAt":"2024-10-16T09:48:32Z","isPatch":true,"sender":{"key":"wolf@oriole.systems","avatar":"https://avatars.githubusercontent.com/u/397518?v=4"},"body":"On 2024-10-16 10:57, Patrick Steinhardt wrote:\n> Given that we do set `log.abbrev` I think we should be hitting code\n> paths in git-shortlog(1) that use it. `git shortlog --format=%h` for\n> example would use `log.abbrev`, wouldn't it? It would be nice to\n> figure out whether this can be made to misbehave based on which object\n> hash we have in the input.\n\nI dove into the code again and now I'm fairly sure custom formatting is\nonly ever done when in a repository. shortlog_output() itself, called at\nthe end of cmd_shortlog(), doesn't do any formatting, only possibly\nwrapping the lines already present in the shortlog struct.\n\nThat struct is filled either by read_from_stdin() or get_from_rev(). The\nlatter is only ever called when in a repository:\n\n>\tif (!nongit && !rev.pending.nr && isatty(0))\n>\t\tadd_head_to_pending(&rev);\n>\tif (rev.pending.nr == 0) {\n>\t\tif (isatty(0))\n>\t\t\tfprintf(stderr, _(\"(reading log message from standard input)\\n\"));\n>\t\tread_from_stdin(&log);\n>\t}\n>\telse\n>\t\tget_from_rev(&rev, &log);\n\nget_from_rev() handles formatting through shortlog_add_commit(),\ndirectly formatting the commit into the oneline buffer that is later\nadded to the shortlog struct:\n\n> \tif (!log->summary) {\n> \t\tif (log->user_format)\n> \t\t\tpretty_print_commit(&ctx, commit, &oneline);\n> \t\telse\n> \t\t\trepo_format_commit_message(the_repository, commit,\n> \t\t\t\t\t\t \"%s\", &oneline, &ctx);\n> \t}\n>\toneline_str = oneline.len ? oneline.buf : \"\";\n>\n>\tinsert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);\n>\tinsert_records_from_format(log, &dups, commit, &ctx, oneline_str);\n\nread_from_stdin(), in comparison, skips all lines from stdin not\nmatching either \"Author: \" or \"Commit: \", and then adds only the\nauthor/committer name and the subject to its log:\n\n>\t\tconst char *v;\n>\t\tif (!skip_prefix(ident.buf, match[0], &v) &&\n>\t\t !skip_prefix(ident.buf, match[1], &v))\n>\t\t\tcontinue;\n>\t\twhile (strbuf_getline_lf(&oneline, stdin) != EOF &&\n>\t\t oneline.len)\n>\t\t\t; /* discard headers */\n>\t\twhile (strbuf_getline_lf(&oneline, stdin) != EOF &&\n>\t\t !oneline.len)\n>\t\t\t; /* discard blanks */\n>\n>\t\tstrbuf_reset(&mapped_ident);\n>\t\tif (parse_ident(log, &mapped_ident, v) < 0)\n>\t\t\tcontinue;\n>\n>\t\tinsert_one_record(log, mapped_ident.buf, oneline.buf);\n\nSo whilst we parse all the relevant options like --abbrev and --format,\nwe take a shortcut through read_from_stdin() and never get to apply a\ncustom format. Commit hashes from stdin are discarded.\n\nI'm not sure a test case for different hash algorithms would test\nanything meaningful here, unless the plan in the future is to have\ngit-shortlog(1) support formatting when reading from stdin.\n\n-- \nWolf\n"},{"id":"505277","messageId":"20241016182124.48148-1-wolf@oriole.systems","threadId":"62317","inReplyTo":"20241011183445.229228-1-wolf@oriole.systems","subject":"[PATCH v3 0/2] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Wolfgang Müller","fromEmail":"wolf@oriole.systems","sentAt":"2024-10-16T18:21:22Z","receivedAt":"2024-10-16T18:21:31Z","isPatch":true,"sender":{"key":"wolf@oriole.systems","avatar":"https://avatars.githubusercontent.com/u/397518?v=4"},"body":"Here's v3 of this series. CI should now pass as the code has been moved\nbelow any declarations. The regression test has been simplified, thanks\nto Eric's input.\n\nI added another commit with a test for git-shortlog(1) that makes sure\nit can process logs obtained from a repo with --object-format=sha256.\nThanks to Patrick for the suggestion and pointers.\n\nWolfgang Müller (2):\n builtin/shortlog: explicitly set hash algo when there is no repo\n shortlog: Test reading a log from a SHA256 repo in a non-git directory\n\n builtin/shortlog.c | 12 ++++++++++++\n t/t4201-shortlog.sh | 12 ++++++++++++\n 2 files changed, 24 insertions(+)\n\nRange-diff against v2:\n1: d3047a0291 ! 1: 4813b458ac builtin/shortlog: explicitly set hash algo when there is no repo\n @@ Commit message\n add a regression test for the segfault.\n \n Signed-off-by: Wolfgang Müller \n + Thanks-to: Eric Sunshine \n \n ## builtin/shortlog.c ##\n @@ builtin/shortlog.c: int cmd_shortlog(int argc,\n - \tstruct rev_info rev;\n - \tint nongit = !startup_info->have_repository;\n + \n + \tstruct parse_opt_ctx_t ctx;\n \n +\t/*\n +\t * NEEDSWORK: Later on we'll call parse_revision_opt which relies on\n @@ builtin/shortlog.c: int cmd_shortlog(int argc,\n +\tif (nongit && !the_hash_algo)\n +\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n +\n - \tconst struct option options[] = {\n - \t\tOPT_BIT('c', \"committer\", &log.groups,\n - \t\t\tN_(\"group by committer rather than author\"),\n + \tgit_config(git_default_config, NULL);\n + \tshortlog_init(&log);\n + \trepo_init_revisions(the_repository, &rev, prefix);\n \n ## t/t4201-shortlog.sh ##\n @@ t/t4201-shortlog.sh: fuzz()\n @@ t/t4201-shortlog.sh: fuzz()\n '\n \n +test_expect_success 'shortlog --author from non-git directory does not segfault' '\n -+\tgit log --no-expand-tabs HEAD >log &&\n -+\tenv GIT_DIR=non-existing git shortlog --author=author out\n ++\techo | nongit git shortlog --author=author\n +'\n +\n test_expect_success 'shortlog should add newline when input line matches wraplen' '\n-: ---------- > 2: 9dfdc7510f shortlog: Test reading a log from a SHA256 repo in a non-git directory\n-- \n2.47.0\n\n"},{"id":"505278","messageId":"20241016182124.48148-2-wolf@oriole.systems","threadId":"62317","inReplyTo":"20241016182124.48148-1-wolf@oriole.systems","subject":"[PATCH v3 1/2] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Wolfgang Müller","fromEmail":"wolf@oriole.systems","sentAt":"2024-10-16T18:21:23Z","receivedAt":"2024-10-16T18:21:33Z","isPatch":true,"sender":{"key":"wolf@oriole.systems","avatar":"https://avatars.githubusercontent.com/u/397518?v=4"},"body":"Whilst git-shortlog(1) does not explicitly need any repository\ninformation when run without reference to one, it still parses some of\nits arguments with parse_revision_opt() which assumes that the hash\nalgorithm is set. However, in c8aed5e8da (repository: stop setting SHA1\nas the default object hash, 2024-05-07) we stopped setting up a default\nhash algorithm and instead require commands to set it up explicitly.\n\nThis was done for most other commands like in ab274909d4 (builtin/diff:\nexplicitly set hash algo when there is no repo, 2024-05-07) but was\nmissed for builtin/shortlog, making git-shortlog(1) segfault outside of\na repository when given arguments like --author that trigger a call to\nparse_revision_opt().\n\nFix this for now by explicitly setting the hash algorithm to SHA1. Also\nadd a regression test for the segfault.\n\nSigned-off-by: Wolfgang Müller \nThanks-to: Eric Sunshine \n---\n builtin/shortlog.c | 12 ++++++++++++\n t/t4201-shortlog.sh | 4 ++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 3ed5c46078..c86b75d981 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -407,6 +407,18 @@ int cmd_shortlog(int argc,\n \n \tstruct parse_opt_ctx_t ctx;\n \n+\t/*\n+\t * NEEDSWORK: Later on we'll call parse_revision_opt which relies on\n+\t * the hash algorithm being set but since we are operating outside of a\n+\t * Git repository we cannot determine one. This is only needed because\n+\t * parse_revision_opt expects hexsz for --abbrev which is irrelevant\n+\t * for shortlog outside of a git repository. For now explicitly set\n+\t * SHA1, but ideally the parsing machinery would be split between\n+\t * git/nongit so that we do not have to do this.\n+\t */\n+\tif (nongit && !the_hash_algo)\n+\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n+\n \tgit_config(git_default_config, NULL);\n \tshortlog_init(&log);\n \trepo_init_revisions(the_repository, &rev, prefix);\ndiff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\nindex c20c885724..0cc2aa5089 100755\n--- a/t/t4201-shortlog.sh\n+++ b/t/t4201-shortlog.sh\n@@ -143,6 +143,10 @@ fuzz()\n \ttest_grep \"too many arguments\" out\n '\n \n+test_expect_success 'shortlog --author from non-git directory does not segfault' '\n+\techo | nongit git shortlog --author=author\n+'\n+\n test_expect_success 'shortlog should add newline when input line matches wraplen' '\n \tcat >expect <<\\EOF &&\n A U Thor (2):\n-- \n2.47.0\n\n"},{"id":"505279","messageId":"20241016182124.48148-3-wolf@oriole.systems","threadId":"62317","inReplyTo":"20241016182124.48148-1-wolf@oriole.systems","subject":"[PATCH v3 2/2] shortlog: Test reading a log from a SHA256 repo in a non-git directory","fromName":"Wolfgang Müller","fromEmail":"wolf@oriole.systems","sentAt":"2024-10-16T18:21:24Z","receivedAt":"2024-10-16T18:21:36Z","isPatch":true,"sender":{"key":"wolf@oriole.systems","avatar":"https://avatars.githubusercontent.com/u/397518?v=4"},"body":"git-shortlog(1) might be used outside of a git repository to process a\nlog obtained from a repository initialized with --object-format=sha256.\nSince git-shortlog(1) has no information on the hash algorithm, make\nsure that it can still successfully process the log regardless.\n\nSigned-off-by: Wolfgang Müller \nThanks-to: Patrick Steinhardt \n---\n t/t4201-shortlog.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\nindex 0cc2aa5089..50d987cbe4 100755\n--- a/t/t4201-shortlog.sh\n+++ b/t/t4201-shortlog.sh\n@@ -147,6 +147,14 @@ fuzz()\n \techo | nongit git shortlog --author=author\n '\n \n+test_expect_success 'shortlog from non-git directory reads log from SHA256 repository' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init --object-format=sha256 repo &&\n+\ttest_commit -C repo initial &&\n+\tgit -C repo log HEAD >log &&\n+\tnongit git shortlog expect <<\\EOF &&\n A U Thor (2):\n-- \n2.47.0\n\n"},{"id":"505280","messageId":"ZxAF4JEA/3r9Jbn4@nand.local","threadId":"62317","inReplyTo":"whst5ap7snkdns5k47tysuguusad5jp7vybahirshkypufhk2s@pnar2yiwcphv","subject":"Re: [RFC PATCH] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-16T18:28:48Z","receivedAt":"2024-10-16T18:28:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 16, 2024 at 10:15:31AM +0200, Wolfgang Müller wrote:\n> On 2024-10-15 19:28, Taylor Blau wrote:\n> > I picked this up today and applied it to 'seen', but ejected it before\n> > pushing the result out, as this appears to break CI, presumably due to\n> > the mixed declaration and code in the patch.\n>\n> Oh, I had missed setting DEVELOPER=1 in my builds so I didn't see this\n> failure myself. Will fix in the upcoming version.\n\nThanks for fixing.\n\nThanks,\nTaylor\n"},{"id":"505282","messageId":"ZxALiDHkkq4yIP8R@nand.local","threadId":"62317","inReplyTo":"yxfpbmwy5bhtjwmffzrfoagugs4rsxw7wn2cvve5ygzcnlolpu@lhjgpz4riogd","subject":"Re: [RFC PATCH v2] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-16T18:52:56Z","receivedAt":"2024-10-16T18:53:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 16, 2024 at 11:07:02AM +0200, Wolfgang Müller wrote:\n> On 2024-10-16 10:57, Patrick Steinhardt wrote:\n> > Given that we do set `log.abbrev` I think we should be hitting code\n> > paths in git-shortlog(1) that use it. `git shortlog --format=%h` for\n> > example would use `log.abbrev`, wouldn't it? It would be nice to figure\n> > out whether this can be made to misbehave based on which object hash we\n> > have in the input.\n>\n> I did try this, but like I said, --format seems not supported when\n> git-shortlog(1) is reading from stdin. It always outputs the same\n> summary format, either grouped on authors or committers. This is not\n> explicitly documented anywhere - the manual only says that \"git shortlog\n> will output a summary of the log read from standard input\" and then goes\n> on to document all the options with no mention of a difference in\n> behaviour when reading from stdin. So I'm still not entirely convinced\n> that this is impossible to trigger (also given the complexity of the\n> argument parsing machinery), but I have not been able to find a way.\n\nYeah, I think this is correct. For the purposes of this series, I think\nthat what Woflgang has done is sufficient on the testing front.\n\nAs a nice piece of #leftoverbits, it would be appreciated to have a\npatch that amends git-shortlog(1) to indicate that '--format' is ignored\nwhen reading input from stdin.\n\nThanks,\nTaylor\n"},{"id":"505283","messageId":"wmx7z4qhvldtshatda73uzlcceyrmqjqnqmy5scivjfdzmfriq@x73trwmxjcdi","threadId":"62317","inReplyTo":"ZxALiDHkkq4yIP8R@nand.local","subject":"Re: [RFC PATCH v2] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Wolfgang Müller","fromEmail":"wolf@oriole.systems","sentAt":"2024-10-16T19:01:48Z","receivedAt":"2024-10-16T19:01:51Z","isPatch":true,"sender":{"key":"wolf@oriole.systems","avatar":"https://avatars.githubusercontent.com/u/397518?v=4"},"body":"On 2024-10-16 14:52, Taylor Blau wrote:\n> As a nice piece of #leftoverbits, it would be appreciated to have a\n> patch that amends git-shortlog(1) to indicate that '--format' is\n> ignored when reading input from stdin.\n\nThink I shall give that a look in the coming days, then, this is\nsomething I'd like to see fixed as well. Immediately what comes to my\nmind is that not only --format is ignored (--author is too if I'm not\nmistaken), so I'll see whether there's a nice way to indicate which\nspecific options are valid when git-shortlog(1) reads from stdin.\n\n-- \nWolf\n"},{"id":"505284","messageId":"ZxANnCw/yVzLl0yn@nand.local","threadId":"62317","inReplyTo":"w6vzuumbd5vrdluvnheescdgw36teuj6k5anlyiphvpr6slcgc@2gkwe2ye26a6","subject":"Re: [RFC PATCH v2] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-16T19:01:48Z","receivedAt":"2024-10-16T19:01:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 16, 2024 at 11:48:24AM +0200, Wolfgang Müller wrote:\n> On 2024-10-16 10:57, Patrick Steinhardt wrote:\n> > Given that we do set `log.abbrev` I think we should be hitting code\n> > paths in git-shortlog(1) that use it. `git shortlog --format=%h` for\n> > example would use `log.abbrev`, wouldn't it? It would be nice to\n> > figure out whether this can be made to misbehave based on which object\n> > hash we have in the input.\n>\n> I dove into the code again and now I'm fairly sure custom formatting is\n> only ever done when in a repository. shortlog_output() itself, called at\n> the end of cmd_shortlog(), doesn't do any formatting, only possibly\n> wrapping the lines already present in the shortlog struct.\n>\n> That struct is filled either by read_from_stdin() or get_from_rev(). The\n> latter is only ever called when in a repository:\n>\n> [...]\n\nThanks; I agree with your analysis here.\n\n> So whilst we parse all the relevant options like --abbrev and --format,\n> we take a shortcut through read_from_stdin() and never get to apply a\n> custom format. Commit hashes from stdin are discarded.\n>\n> I'm not sure a test case for different hash algorithms would test\n> anything meaningful here, unless the plan in the future is to have\n> git-shortlog(1) support formatting when reading from stdin.\n\nI think that in general it would be difficult to support the full range\nof --format specifiers when operating outside of a repository, because\nwe don't have all of the information necessary to assemble all of the\npossible formatting options.\n\nFor instance, let's say I want to take Patrick's example to test 'git\nshortlog' with '--format=\"%H\"' outside of the repository. There's no way\nto disambiguate whether, say, a SHA-256 hash is either (a) a correctly\nformatted SHA-256 hash, or (b) a corrupted / too-long SHA-1 hash.\n\nSo that means that '%H', '%h', '%T', and '%t' are off the table. '%an'\nand '%ae' seem reasonable to implement, but '%aN' and '%aE' less so,\nbecause we don't have a .mailmap file to read. The same goes for the\ncommitter variants of all of those.\n\nI don't think there is any reasonable interpretation of '%d'/'%D', and\nlikewise for '%(decorate)' as well as '%(describe)'. We could probably\ngo on, but I am getting tired of looking through the 'PRETTY FORMATS'\nsection of git-log(1) and trying to figure out how they'd work (or not)\nwithout a repository ;-).\n\nIn any event, my feeling is that while we could probably implement a\nhandful of these formatting options, that it would likely be kind of\nawkward to do so. Not to mention the user-visible awkwardness of\nsupporting some '--format' specifiers but not others[^1].\n\nSo I think that the best course of action would be to document the\nlimitation and move on ;-).\n\nThanks,\nTaylor\n\n[^1]: Playing devil's advocate, though, perhaps it is OK to document\n well which formatting options do and don't work, and accept that a\n user asking for '--format=\"%(describe)\"' (etc.) outside of a\n repository is nonsensical and warn / return nothing appropriately.\n"},{"id":"505285","messageId":"4dnthnr3ksoirvltn4smfyasyl5g4edzznheuo7hji4zy7h7gn@uomlcpchbvar","threadId":"62317","inReplyTo":"ZxANnCw/yVzLl0yn@nand.local","subject":"Re: [RFC PATCH v2] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Wolfgang Müller","fromEmail":"wolf@oriole.systems","sentAt":"2024-10-16T19:14:35Z","receivedAt":"2024-10-16T19:14:40Z","isPatch":true,"sender":{"key":"wolf@oriole.systems","avatar":"https://avatars.githubusercontent.com/u/397518?v=4"},"body":"On 2024-10-16 15:01, Taylor Blau wrote:\n> So that means that '%H', '%h', '%T', and '%t' are off the table. '%an'\n> and '%ae' seem reasonable to implement, but '%aN' and '%aE' less so,\n> because we don't have a .mailmap file to read. The same goes for the\n> committer variants of all of those.\n\nAn interesting tidbit here is that git-shortlog(1) actually does read a\n.mailmap file when processing stdin, but it takes it from the current\ndirectory instead. Having read the full manual today, it's one of the\nonly times it mentions a difference in behaviour regarding standard\ninput.\n\n-- \nWolf\n"},{"id":"505286","messageId":"ZxASXLz4lA+E9MuQ@nand.local","threadId":"62317","inReplyTo":"20241016182124.48148-2-wolf@oriole.systems","subject":"Re: [PATCH v3 1/2] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-16T19:22:04Z","receivedAt":"2024-10-16T19:22:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 16, 2024 at 08:21:23PM +0200, Wolfgang Müller wrote:\n> Whilst git-shortlog(1) does not explicitly need any repository\n> information when run without reference to one, it still parses some of\n> its arguments with parse_revision_opt() which assumes that the hash\n> algorithm is set. However, in c8aed5e8da (repository: stop setting SHA1\n> as the default object hash, 2024-05-07) we stopped setting up a default\n> hash algorithm and instead require commands to set it up explicitly.\n>\n> This was done for most other commands like in ab274909d4 (builtin/diff:\n> explicitly set hash algo when there is no repo, 2024-05-07) but was\n> missed for builtin/shortlog, making git-shortlog(1) segfault outside of\n> a repository when given arguments like --author that trigger a call to\n> parse_revision_opt().\n\nGood analysis.\n\n> Fix this for now by explicitly setting the hash algorithm to SHA1. Also\n> add a regression test for the segfault.\n\nMakes sense.\n\n> Signed-off-by: Wolfgang Müller \n> Thanks-to: Eric Sunshine \n\nIn the future, please ensure that your Signed-off-by is the last trailer\nin the section to indicate that you have certified everything above it\n(which in that case would include your Thanks-to here).\n\n> @@ -143,6 +143,10 @@ fuzz()\n> \ttest_grep \"too many arguments\" out\n> '\n>\n> +test_expect_success 'shortlog --author from non-git directory does not segfault' '\n> +\techo | nongit git shortlog --author=author\n> +'\n> +\n\nInstead of 'echo |', I wonder if it would be just as good to write this\ntest as:\n\n nongit git shortlog --author=author git-shortlog(1) might be used outside of a git repository to process a\n> log obtained from a repository initialized with --object-format=sha256.\n> Since git-shortlog(1) has no information on the hash algorithm, make\n> sure that it can still successfully process the log regardless.\n>\n> Signed-off-by: Wolfgang Müller \n> Thanks-to: Patrick Steinhardt \n\nSame note here about ordering Signed-off-by: and Thanks-to: lines as in\nthe previous patch.\n\n> @@ -147,6 +147,14 @@ fuzz()\n> \techo | nongit git shortlog --author=author\n> '\n>\n> +test_expect_success 'shortlog from non-git directory reads log from SHA256 repository' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init --object-format=sha256 repo &&\n\nI wondered why we needed a separate repo for this test, but I understand\nnow that we need to have a SHA-256 repository to test this on.\n\nI'm still dubious as to the value of this test overall, but I think it\nmay be nice to s/repo/sha256-repo/ or something here to indicate its\npurpose more clearly in the test itself.\n\nI don't feel strongly about it, though.\n\nThanks,\nTaylor\n"},{"id":"505288","messageId":"ZxAUwxswtz4HmFeg@nand.local","threadId":"62317","inReplyTo":"20241016182124.48148-1-wolf@oriole.systems","subject":"Re: [PATCH v3 0/2] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-16T19:32:19Z","receivedAt":"2024-10-16T19:32:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 16, 2024 at 08:21:22PM +0200, Wolfgang Müller wrote:\n> Here's v3 of this series. CI should now pass as the code has been moved\n> below any declarations. The regression test has been simplified, thanks\n> to Eric's input.\n>\n> I added another commit with a test for git-shortlog(1) that makes sure\n> it can process logs obtained from a repo with --object-format=sha256.\n> Thanks to Patrick for the suggestion and pointers.\n>\n> Wolfgang Müller (2):\n> builtin/shortlog: explicitly set hash algo when there is no repo\n> shortlog: Test reading a log from a SHA256 repo in a non-git directory\n\nThanks. I took a look at this new round and it is looking close to me.\n\nI had a couple of small-ish comments that I think would be good to\naddress in a new round. But if you feel strongly that the current round\nis good as-is, I'm happy to hear arguments in that direction, too ;-).\n\nThanks,\nTaylor\n"},{"id":"505289","messageId":"jpbyxh7o3kip6mef7ie545futmfer2o4qjryvq7yta7qvsqj6f@4zsqyn7t7hnf","threadId":"62317","inReplyTo":"ZxATKIRGv4QCM9Td@nand.local","subject":"Re: [PATCH v3 2/2] shortlog: Test reading a log from a SHA256 repo in a non-git directory","fromName":"Wolfgang Müller","fromEmail":"wolf@oriole.systems","sentAt":"2024-10-16T19:35:46Z","receivedAt":"2024-10-16T19:35:49Z","isPatch":true,"sender":{"key":"wolf@oriole.systems","avatar":"https://avatars.githubusercontent.com/u/397518?v=4"},"body":"On 2024-10-16 15:25, Taylor Blau wrote:\n> Same note here about ordering Signed-off-by: and Thanks-to: lines as in\n> the previous patch.\n\nShall keep in mind, thanks!\n\n> > @@ -147,6 +147,14 @@ fuzz()\n> > \techo | nongit git shortlog --author=author\n> > '\n> >\n> > +test_expect_success 'shortlog from non-git directory reads log from SHA256 repository' '\n> > +\ttest_when_finished \"rm -rf repo\" &&\n> > +\tgit init --object-format=sha256 repo &&\n> \n> I wondered why we needed a separate repo for this test, but I understand\n> now that we need to have a SHA-256 repository to test this on.\n> \n> I'm still dubious as to the value of this test overall, but I think it\n> may be nice to s/repo/sha256-repo/ or something here to indicate its\n> purpose more clearly in the test itself.\n> \n> I don't feel strongly about it, though.\n\nI personally lean towards skipping out on this test, but I can also see\nvalue in it coming from a point of view of future changes to\ngit-shortlog(1) behaviour. Then again, perhaps it would be better then\nto add a relevant test only if new behaviour is added. No strong\nfeelings here either, I'd defer to what regular maintainers and\ndevelopers here prefer :-)\n\n-- \nWolf\n"},{"id":"505291","messageId":"2b6k5bhqqs7ce4glglmqccsrr3wftggyj6nnltrkkmho3xbdvg@noiq2dpkfqqz","threadId":"62317","inReplyTo":"ZxASXLz4lA+E9MuQ@nand.local","subject":"Re: [PATCH v3 1/2] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Wolfgang Müller","fromEmail":"wolf@oriole.systems","sentAt":"2024-10-16T19:37:04Z","receivedAt":"2024-10-16T19:37:07Z","isPatch":true,"sender":{"key":"wolf@oriole.systems","avatar":"https://avatars.githubusercontent.com/u/397518?v=4"},"body":"> Instead of 'echo |', I wonder if it would be just as good to write this\n> test as:\n> \n> nongit git shortlog --author=author I had a couple of small-ish comments that I think would be good to\n> address in a new round. But if you feel strongly that the current round\n> is good as-is, I'm happy to hear arguments in that direction, too ;-).\n\nNew round sounds perfectly fine to me, I'll send it along once there is\nsome consensus on whether or not to keep the additional test.\n\n-- \nWolf\n"},{"id":"505293","messageId":"ZxAXxp19p9rWeJVK@nand.local","threadId":"62317","inReplyTo":"jpbyxh7o3kip6mef7ie545futmfer2o4qjryvq7yta7qvsqj6f@4zsqyn7t7hnf","subject":"Re: [PATCH v3 2/2] shortlog: Test reading a log from a SHA256 repo in a non-git directory","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-16T19:45:10Z","receivedAt":"2024-10-16T19:45:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 16, 2024 at 09:35:46PM +0200, Wolfgang Müller wrote:\n> > > @@ -147,6 +147,14 @@ fuzz()\n> > > \techo | nongit git shortlog --author=author\n> > > '\n> > >\n> > > +test_expect_success 'shortlog from non-git directory reads log from SHA256 repository' '\n> > > +\ttest_when_finished \"rm -rf repo\" &&\n> > > +\tgit init --object-format=sha256 repo &&\n> >\n> > I wondered why we needed a separate repo for this test, but I understand\n> > now that we need to have a SHA-256 repository to test this on.\n> >\n> > I'm still dubious as to the value of this test overall, but I think it\n> > may be nice to s/repo/sha256-repo/ or something here to indicate its\n> > purpose more clearly in the test itself.\n> >\n> > I don't feel strongly about it, though.\n>\n> I personally lean towards skipping out on this test, but I can also see\n> value in it coming from a point of view of future changes to\n> git-shortlog(1) behaviour. Then again, perhaps it would be better then\n> to add a relevant test only if new behaviour is added. No strong\n> feelings here either, I'd defer to what regular maintainers and\n> developers here prefer :-)\n\nI have a vague preference towards just getting rid of it, but I wouldn't\nbe sad to see it stay, either.\n\nThanks,\nTaylor\n"},{"id":"505341","messageId":"ZxCa2X7bo5JFelRu@pks.im","threadId":"62317","inReplyTo":"ZxALiDHkkq4yIP8R@nand.local","subject":"Re: [RFC PATCH v2] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-17T05:04:25Z","receivedAt":"2024-10-17T05:04:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Oct 16, 2024 at 02:52:56PM -0400, Taylor Blau wrote:\n> On Wed, Oct 16, 2024 at 11:07:02AM +0200, Wolfgang Müller wrote:\n> > On 2024-10-16 10:57, Patrick Steinhardt wrote:\n> > > Given that we do set `log.abbrev` I think we should be hitting code\n> > > paths in git-shortlog(1) that use it. `git shortlog --format=%h` for\n> > > example would use `log.abbrev`, wouldn't it? It would be nice to figure\n> > > out whether this can be made to misbehave based on which object hash we\n> > > have in the input.\n> >\n> > I did try this, but like I said, --format seems not supported when\n> > git-shortlog(1) is reading from stdin. It always outputs the same\n> > summary format, either grouped on authors or committers. This is not\n> > explicitly documented anywhere - the manual only says that \"git shortlog\n> > will output a summary of the log read from standard input\" and then goes\n> > on to document all the options with no mention of a difference in\n> > behaviour when reading from stdin. So I'm still not entirely convinced\n> > that this is impossible to trigger (also given the complexity of the\n> > argument parsing machinery), but I have not been able to find a way.\n> \n> Yeah, I think this is correct. For the purposes of this series, I think\n> that what Woflgang has done is sufficient on the testing front.\n> \n> As a nice piece of #leftoverbits, it would be appreciated to have a\n> patch that amends git-shortlog(1) to indicate that '--format' is ignored\n> when reading input from stdin.\n\nYeah, I'm happy with the in-depth explanation that he gave and am fine\nwith not adding a test.\n\nPatrick\n"},{"id":"505353","messageId":"20241017093528.18157-1-wolf@oriole.systems","threadId":"62317","inReplyTo":"20241011183445.229228-1-wolf@oriole.systems","subject":"[PATCH v4] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Wolfgang Müller","fromEmail":"wolf@oriole.systems","sentAt":"2024-10-17T09:35:28Z","receivedAt":"2024-10-17T09:35:51Z","isPatch":true,"sender":{"key":"wolf@oriole.systems","avatar":"https://avatars.githubusercontent.com/u/397518?v=4"},"body":"Whilst git-shortlog(1) does not explicitly need any repository\ninformation when run without reference to one, it still parses some of\nits arguments with parse_revision_opt() which assumes that the hash\nalgorithm is set. However, in c8aed5e8da (repository: stop setting SHA1\nas the default object hash, 2024-05-07) we stopped setting up a default\nhash algorithm and instead require commands to set it up explicitly.\n\nThis was done for most other commands like in ab274909d4 (builtin/diff:\nexplicitly set hash algo when there is no repo, 2024-05-07) but was\nmissed for builtin/shortlog, making git-shortlog(1) segfault outside of\na repository when given arguments like --author that trigger a call to\nparse_revision_opt().\n\nFix this for now by explicitly setting the hash algorithm to SHA1. Also\nadd a regression test for the segfault.\n\nThanks-to: Eric Sunshine \nSigned-off-by: Wolfgang Müller \n---\nHere's v4 with the trailers fixed and a small improvement to the test.\nThe commit including the additional test has been dropped.\n\nThanks!\n\n builtin/shortlog.c | 12 ++++++++++++\n t/t4201-shortlog.sh | 4 ++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 3ed5c46078..c86b75d981 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -407,6 +407,18 @@ int cmd_shortlog(int argc,\n \n \tstruct parse_opt_ctx_t ctx;\n \n+\t/*\n+\t * NEEDSWORK: Later on we'll call parse_revision_opt which relies on\n+\t * the hash algorithm being set but since we are operating outside of a\n+\t * Git repository we cannot determine one. This is only needed because\n+\t * parse_revision_opt expects hexsz for --abbrev which is irrelevant\n+\t * for shortlog outside of a git repository. For now explicitly set\n+\t * SHA1, but ideally the parsing machinery would be split between\n+\t * git/nongit so that we do not have to do this.\n+\t */\n+\tif (nongit && !the_hash_algo)\n+\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n+\n \tgit_config(git_default_config, NULL);\n \tshortlog_init(&log);\n \trepo_init_revisions(the_repository, &rev, prefix);\ndiff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\nindex c20c885724..5b5d3b637c 100755\n--- a/t/t4201-shortlog.sh\n+++ b/t/t4201-shortlog.sh\n@@ -143,6 +143,10 @@ fuzz()\n \ttest_grep \"too many arguments\" out\n '\n \n+test_expect_success 'shortlog --author from non-git directory does not segfault' '\n+\tnongit git shortlog --author=author expect <<\\EOF &&\n A U Thor (2):\n\nRange-diff against v3:\n1: 4813b458ac ! 1: 1a2959c0de builtin/shortlog: explicitly set hash algo when there is no repo\n @@ Commit message\n Fix this for now by explicitly setting the hash algorithm to SHA1. Also\n add a regression test for the segfault.\n \n - Signed-off-by: Wolfgang Müller \n Thanks-to: Eric Sunshine \n + Signed-off-by: Wolfgang Müller \n \n ## builtin/shortlog.c ##\n @@ builtin/shortlog.c: int cmd_shortlog(int argc,\n @@ t/t4201-shortlog.sh: fuzz()\n '\n \n +test_expect_success 'shortlog --author from non-git directory does not segfault' '\n -+\techo | nongit git shortlog --author=author\n ++\tnongit git shortlog --author=author On Wed, Oct 16, 2024 at 08:21:23PM +0200, Wolfgang Müller wrote:\n> > Whilst git-shortlog(1) does not explicitly need any repository\n> > information when run without reference to one, it still parses some of\n> > its arguments with parse_revision_opt() which assumes that the hash\n> > algorithm is set. However, in c8aed5e8da (repository: stop setting SHA1\n> > as the default object hash, 2024-05-07) we stopped setting up a default\n> > hash algorithm and instead require commands to set it up explicitly.\n> >\n> > This was done for most other commands like in ab274909d4 (builtin/diff:\n> > explicitly set hash algo when there is no repo, 2024-05-07) but was\n> > missed for builtin/shortlog, making git-shortlog(1) segfault outside of\n> > a repository when given arguments like --author that trigger a call to\n> > parse_revision_opt().\n> \n> Good analysis.\n> \n> > Fix this for now by explicitly setting the hash algorithm to SHA1. Also\n> > add a regression test for the segfault.\n> \n> Makes sense.\n> \n> > Signed-off-by: Wolfgang Müller \n> > Thanks-to: Eric Sunshine \n> \n> In the future, please ensure that your Signed-off-by is the last trailer\n> in the section to indicate that you have certified everything above it\n> (which in that case would include your Thanks-to here).\n\nI think it's also more common to use \"Helped-by\" instead of \"Thanks-to\".\nI see you have the same trailer in v4, but don't necessarily think that\nit is a good-enough reason to reroll.\n\nPatrick\n"},{"id":"505364","messageId":"bjk4i7bnlc3bajzjy7izt56mm2ynh6jofbdmqegsmfrzkwqnud@t7qiaonbktuw","threadId":"62317","inReplyTo":"ZxD7j-ciyAGHrGwM@pks.im","subject":"Re: [PATCH v3 1/2] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Wolfgang Müller","fromEmail":"wolf@oriole.systems","sentAt":"2024-10-17T12:09:12Z","receivedAt":"2024-10-17T12:09:21Z","isPatch":true,"sender":{"key":"wolf@oriole.systems","avatar":"https://avatars.githubusercontent.com/u/397518?v=4"},"body":"On 2024-10-17 13:58, Patrick Steinhardt wrote:\n> I think it's also more common to use \"Helped-by\" instead of \"Thanks-to\".\n> I see you have the same trailer in v4, but don't necessarily think that\n> it is a good-enough reason to reroll.\n\nThat is good to know, thanks. Is there by chance any document that\noutlines usage guidelines for when to use which trailer or is it more of\nan undocumented custom?\n\n-- \nWolf\n"},{"id":"505365","messageId":"ZxD-5alMaQinK4G2@pks.im","threadId":"62317","inReplyTo":"bjk4i7bnlc3bajzjy7izt56mm2ynh6jofbdmqegsmfrzkwqnud@t7qiaonbktuw","subject":"Re: [PATCH v3 1/2] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-17T12:11:17Z","receivedAt":"2024-10-17T12:11:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Oct 17, 2024 at 02:09:12PM +0200, Wolfgang Müller wrote:\n> On 2024-10-17 13:58, Patrick Steinhardt wrote:\n> > I think it's also more common to use \"Helped-by\" instead of \"Thanks-to\".\n> > I see you have the same trailer in v4, but don't necessarily think that\n> > it is a good-enough reason to reroll.\n> \n> That is good to know, thanks. Is there by chance any document that\n> outlines usage guidelines for when to use which trailer or is it more of\n> an undocumented custom?\n\nWe have Documentation/SubmittingPatches, which has a section named\n\"commit-trailers\".\n\nPatrick\n"},{"id":"505388","messageId":"ZxFvQsV7FHel/pZe@nand.local","threadId":"62317","inReplyTo":"20241017093528.18157-1-wolf@oriole.systems","subject":"Re: [PATCH v4] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-17T20:10:42Z","receivedAt":"2024-10-17T20:10:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Oct 17, 2024 at 11:35:28AM +0200, Wolfgang Müller wrote:\n> builtin/shortlog.c | 12 ++++++++++++\n> t/t4201-shortlog.sh | 4 ++++\n> 2 files changed, 16 insertions(+)\n\nThis version looks great. Thanks, Wolfgang -- let's start merging this\none down.\n\nThanks,\nTaylor\n"},{"id":"505398","messageId":"cinnj37fsmmgn6x5gibkvj7mtbn6vyv4pclxyv5sgpz75b5uim@ptzc3xjk3lfu","threadId":"62317","inReplyTo":"ZxFvQsV7FHel/pZe@nand.local","subject":"Re: [PATCH v4] builtin/shortlog: explicitly set hash algo when there is no repo","fromName":"Wolfgang Müller","fromEmail":"wolf@oriole.systems","sentAt":"2024-10-17T22:02:47Z","receivedAt":"2024-10-17T22:02:54Z","isPatch":true,"sender":{"key":"wolf@oriole.systems","avatar":"https://avatars.githubusercontent.com/u/397518?v=4"},"body":"On 2024-10-17 16:10, Taylor Blau wrote:\n> This version looks great. Thanks, Wolfgang -- let's start merging this\n> one down.\n\nThanks! And thanks to the community as well for the help and feedback,\nas well as the thorough discussion. It was fun to contribute, and I feel\nlike I learned a lot :-)\n\n-- \nWolf\n"}]}