{"thread":{"id":"66153","subject":"[PATCH] git: avoid segfault on \"git --shallow-file\" without a value","startedAt":"2026-08-11T12:14:58Z","lastAt":"2026-08-13T07:38:12Z","messageCount":7,"participants":["Christian Couder","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"550280","messageId":"20260811121446.2080190-1-christian.couder@gmail.com","threadId":"66153","inReplyTo":null,"subject":"[PATCH] git: avoid segfault on \"git --shallow-file\" without a value","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-08-11T12:14:46Z","receivedAt":"2026-08-11T12:14:58Z","isPatch":true,"body":"In \"git.c\", the other `handle_options()` options that take their value\nas a separate argument, like `--git-dir`, `--namespace` or `-C`, check\nthat such an argument actually exists before using it, and error out\nwith a message and the usage string otherwise.\n\nThe `--shallow-file` option doesn't perform that check. It blindly\nadvances past the option and then dereferences the next element of\n`argv`, which is the NULL terminator when no value was given. So\n`git --shallow-file` segfaults:\n\n $ git --shallow-file\n Segmentation fault (core dumped)\n\nLet's fix that by checking that a value was given, in the same way and\nwith a message worded like the ones the other options use.\n\nWhile at it, let's also set the environment variable before advancing\npast the option, instead of advancing first and using `(*argv)[0]`, so\nthat this option looks like the other ones.\n\nNote that all the in-tree callers passing `--shallow-file` to a `git`\nsubprocess always pass a value after it, so they are not affected. In\n`upload-pack.c` that value is an empty string, which is still accepted.\n\nSigned-off-by: Christian Couder \n---\n\nWhile working on modernizing `git fast-import`, I noticed that\n`--shallow-file` was handled differently than the other options that\ntake an argument in \"git.c\", and found this segfault.\n\nI have started working on a better way to handle such options not only\nin \"git.c\" but also in other files. For now though, I think a small\nlocalized bugfix like this is the simplest solution.\n\nNot sure if \"t0041-usage.sh\" is the best place for testing this, but I\ncouldn't find a dedicated one.\n\nCI tests all pass, see:\n\nhttps://github.com/chriscool/git/actions/runs/31478034826\n\n git.c | 10 +++++++---\n t/t0041-usage.sh | 7 +++++++\n 2 files changed, 14 insertions(+), 3 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex e5f1811b6b..96df15b5cd 100644\n--- a/git.c\n+++ b/git.c\n@@ -304,11 +304,15 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"--shallow-file\")) {\n-\t\t\t(*argv)++;\n-\t\t\t(*argc)--;\n-\t\t\tsetenv(GIT_SHALLOW_FILE_ENVIRONMENT, (*argv)[0], 1);\n+\t\t\tif (*argc < 2) {\n+\t\t\t\tfprintf(stderr, _(\"no file given for '%s' option\\n\" ), \"--shallow-file\");\n+\t\t\t\tusage(git_usage_string);\n+\t\t\t}\n+\t\t\tsetenv(GIT_SHALLOW_FILE_ENVIRONMENT, (*argv)[1], 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n+\t\t\t(*argv)++;\n+\t\t\t(*argc)--;\n \t\t} else if (!strcmp(cmd, \"-C\")) {\n \t\t\tif (*argc < 2) {\n \t\t\t\tfprintf(stderr, _(\"no directory given for '%s' option\\n\" ), \"-C\");\ndiff --git a/t/t0041-usage.sh b/t/t0041-usage.sh\nindex 51af7cc030..2a9c5eafca 100755\n--- a/t/t0041-usage.sh\n+++ b/t/t0041-usage.sh\n@@ -107,4 +107,11 @@ test_expect_success 'for-each-ref usage error' '\n \ttest_grep \"usage\" actual.err\n '\n \n+test_expect_success 'git --shallow-file without a value' '\n+\ttest_must_fail git --shallow-file >actual 2>actual.err &&\n+\ttest_line_count = 0 actual &&\n+\ttest_grep \"no file given for \" actual.err &&\n+\ttest_grep \"usage\" actual.err\n+'\n+\n test_done\n-- \n2.55.0.530.gdb3615d990.dirty\n\n"},{"id":"550322","messageId":"xmqqcxvo1n8w.fsf@gitster.g","threadId":"66153","inReplyTo":"20260811121446.2080190-1-christian.couder@gmail.com","subject":"Re: [PATCH] git: avoid segfault on \"git --shallow-file\" without a value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-11T19:16:31Z","receivedAt":"2026-08-11T19:16:34Z","isPatch":true,"body":"Christian Couder writes:\n\nA great subject line ;-) It is the simplest reproducer of any bug.\n\n> In \"git.c\", the other `handle_options()` options that take their value\n> as a separate argument, like `--git-dir`, `--namespace` or `-C`, check\n> that such an argument actually exists before using it, and error out\n> with a message and the usage string otherwise.\n>\n> The `--shallow-file` option doesn't perform that check. It blindly\n> advances past the option and then dereferences the next element of\n> `argv`, which is the NULL terminator when no value was given. So\n> `git --shallow-file` segfaults:\n>\n> $ git --shallow-file\n> Segmentation fault (core dumped)\n> ...\n> diff --git a/git.c b/git.c\n> index e5f1811b6b..96df15b5cd 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -304,11 +304,15 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n> \t\t\tif (envchanged)\n> \t\t\t\t*envchanged = 1;\n> \t\t} else if (!strcmp(cmd, \"--shallow-file\")) {\n> -\t\t\t(*argv)++;\n> -\t\t\t(*argc)--;\n> -\t\t\tsetenv(GIT_SHALLOW_FILE_ENVIRONMENT, (*argv)[0], 1);\n> +\t\t\tif (*argc < 2) {\n> +\t\t\t\tfprintf(stderr, _(\"no file given for '%s' option\\n\" ), \"--shallow-file\");\n> +\t\t\t\tusage(git_usage_string);\n> +\t\t\t}\n> +\t\t\tsetenv(GIT_SHALLOW_FILE_ENVIRONMENT, (*argv)[1], 1);\n> \t\t\tif (envchanged)\n> \t\t\t\t*envchanged = 1;\n> +\t\t\t(*argv)++;\n> +\t\t\t(*argc)--;\n\nIt is curious that the fix needs to be so big, when the only change\nnecessary, as far as I can tell from your problem description, is to\ninsert 4 line \"if (... not enough args ...) { ... barf and die ...}\"\nblock and without anything else. I think the culprit is this \"while\nat it\" ...\n\n> While at it, let's also set the environment variable before advancing\n> past the option, instead of advancing first and using `(*argv)[0]`, so\n> that this option looks like the other ones.\n\n... that made the patch more confusing to read than otherwise.\n\nBut without reading the preimage of the patch, the result is just as\nunderstandable ;-) Let's take the patch as-is.\n\n> +test_expect_success 'git --shallow-file without a value' '\n> +\ttest_must_fail git --shallow-file >actual 2>actual.err &&\n> +\ttest_line_count = 0 actual &&\n> +\ttest_grep \"no file given for \" actual.err &&\n> +\ttest_grep \"usage\" actual.err\n> +'\n\nDo we have similar \"oops, you were supposed to give me a value\" test\nfor other things like \"--config-env=\", \"-C\", etc.? Just being\ncurious, because (1) if there are, this addition belongs there, not\nhere, and (2) if there aren't, this addition may not be needed, and\n(3) if there aren't or if the existing coverage is incomplete,\nperhaps we should give a more complete coverage while at it.\n\nWith (3), I mean something along the lines of ...\n\n\tfor opt in -C -c --git-dir --work-tree --namespace --config-env\n\tdo\n\t\ttest_expect_success \"git $opt without a value\" '\n\t\t\ttest_must_fail git $opt >actual 2>error &&\n\t\t\ttest_line_count 0 actual &&\n\t\t\ttest_grep usage error\n\t\t'\n\tdone\n\nI do not mean to say that (3) is my favorite among these three,\nthough.\n"},{"id":"550394","messageId":"anxXbnuRt4I4uPdI@pks.im","threadId":"66153","inReplyTo":"20260811121446.2080190-1-christian.couder@gmail.com","subject":"Re: [PATCH] git: avoid segfault on \"git --shallow-file\" without a value","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-12T11:22:22Z","receivedAt":"2026-08-12T11:22:30Z","isPatch":true,"body":"On Tue, Aug 11, 2026 at 02:14:46PM +0200, Christian Couder wrote:\n> diff --git a/git.c b/git.c\n> index e5f1811b6b..96df15b5cd 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -304,11 +304,15 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n> \t\t\tif (envchanged)\n> \t\t\t\t*envchanged = 1;\n> \t\t} else if (!strcmp(cmd, \"--shallow-file\")) {\n> -\t\t\t(*argv)++;\n> -\t\t\t(*argc)--;\n> -\t\t\tsetenv(GIT_SHALLOW_FILE_ENVIRONMENT, (*argv)[0], 1);\n> +\t\t\tif (*argc < 2) {\n> +\t\t\t\tfprintf(stderr, _(\"no file given for '%s' option\\n\" ), \"--shallow-file\");\n> +\t\t\t\tusage(git_usage_string);\n\nShould we maybe condense this into a single line?\n\n usage(_(\"no file given for '%s' option\\n\")), \"--shallow-file\")\n\nI think that also printing the usage string is only distracting and\ndoesn't really give the user a lot of extra context.\n\nOther than that this patch looks good to me, thanks!\n\nPatrick\n"},{"id":"550413","messageId":"CAP8UFD1XMY6N3UD5FhK_oeQDX7banP1e0oKM1WHUPhPv_vzbsQ@mail.gmail.com","threadId":"66153","inReplyTo":"anxXbnuRt4I4uPdI@pks.im","subject":"Re: [PATCH] git: avoid segfault on \"git --shallow-file\" without a value","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-08-12T15:42:00Z","receivedAt":"2026-08-12T15:42:12Z","isPatch":true,"body":"On Wed, Aug 12, 2026 at 1:22 PM Patrick Steinhardt wrote:\n>\n> On Tue, Aug 11, 2026 at 02:14:46PM +0200, Christian Couder wrote:\n> > diff --git a/git.c b/git.c\n> > index e5f1811b6b..96df15b5cd 100644\n> > --- a/git.c\n> > +++ b/git.c\n> > @@ -304,11 +304,15 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n> > if (envchanged)\n> > *envchanged = 1;\n> > } else if (!strcmp(cmd, \"--shallow-file\")) {\n> > - (*argv)++;\n> > - (*argc)--;\n> > - setenv(GIT_SHALLOW_FILE_ENVIRONMENT, (*argv)[0], 1);\n> > + if (*argc < 2) {\n> > + fprintf(stderr, _(\"no file given for '%s' option\\n\" ), \"--shallow-file\");\n> > + usage(git_usage_string);\n>\n> Should we maybe condense this into a single line?\n>\n> usage(_(\"no file given for '%s' option\\n\")), \"--shallow-file\")\n>\n> I think that also printing the usage string is only distracting and\n> doesn't really give the user a lot of extra context.\n\nThe goal of this patch is to fix the bug by using the same code as the\nother options that can be passed a value like \"--git-dir\",\n\"--namespace\", \"--work-tree\", and so on. Now all these options use the\nsame pattern for the error message:\n\ngit grep -A3 'if (\\*argc < 2)' git.c\ngit.c: if (*argc < 2) {\ngit.c- fprintf(stderr, _(\"no directory given\nfor '%s' option\\n\" ), \"--git-dir\");\ngit.c- usage(git_usage_string);\ngit.c- }\n--\ngit.c: if (*argc < 2) {\ngit.c- fprintf(stderr, _(\"no namespace given\nfor --namespace\\n\" ));\ngit.c- usage(git_usage_string);\ngit.c- }\n--\ngit.c: if (*argc < 2) {\ngit.c- fprintf(stderr, _(\"no directory given\nfor '%s' option\\n\" ), \"--work-tree\");\ngit.c- usage(git_usage_string);\ngit.c- }\n--\ngit.c: if (*argc < 2) {\ngit.c- fprintf(stderr, _(\"-c expects a\nconfiguration string\\n\" ));\ngit.c- usage(git_usage_string);\ngit.c- }\n--\ngit.c: if (*argc < 2) {\ngit.c- fprintf(stderr, _(\"no config key given\nfor --config-env\\n\" ));\ngit.c- usage(git_usage_string);\ngit.c- }\n--\ngit.c: if (*argc < 2) {\ngit.c- fprintf(stderr, _(\"no directory given\nfor '%s' option\\n\" ), \"-C\");\ngit.c- usage(git_usage_string);\ngit.c- }\n--\ngit.c: if (*argc < 2) {\ngit.c- fprintf(stderr, _(\"no attribute source\ngiven for --attr-source\\n\" ));\ngit.c- usage(git_usage_string);\ngit.c- }\n\nSo I don't think it makes sense for \"--shallow-file\" to not be\nconsistent with these other options.\n\nI could perhaps add a patch to the series to convert all of these to\nsomething like what you suggest, but it could also be done in a\nseparate patch series by someone else.\n\nAnyway thanks for reviewing this patch.\n"},{"id":"550418","messageId":"CAP8UFD1BoXTo-bNyaQeWeC1QhrpdBAOOW4BwXCi9XYMr7aRuZw@mail.gmail.com","threadId":"66153","inReplyTo":"xmqqcxvo1n8w.fsf@gitster.g","subject":"Re: [PATCH] git: avoid segfault on \"git --shallow-file\" without a value","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-08-12T16:15:42Z","receivedAt":"2026-08-12T16:15:55Z","isPatch":true,"body":"On Tue, Aug 11, 2026 at 9:16 PM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n\n[...]\n\n> > $ git --shallow-file\n> > Segmentation fault (core dumped)\n> > ...\n> > diff --git a/git.c b/git.c\n> > index e5f1811b6b..96df15b5cd 100644\n> > --- a/git.c\n> > +++ b/git.c\n> > @@ -304,11 +304,15 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n> > if (envchanged)\n> > *envchanged = 1;\n> > } else if (!strcmp(cmd, \"--shallow-file\")) {\n> > - (*argv)++;\n> > - (*argc)--;\n> > - setenv(GIT_SHALLOW_FILE_ENVIRONMENT, (*argv)[0], 1);\n> > + if (*argc < 2) {\n> > + fprintf(stderr, _(\"no file given for '%s' option\\n\" ), \"--shallow-file\");\n> > + usage(git_usage_string);\n> > + }\n> > + setenv(GIT_SHALLOW_FILE_ENVIRONMENT, (*argv)[1], 1);\n> > if (envchanged)\n> > *envchanged = 1;\n> > + (*argv)++;\n> > + (*argc)--;\n>\n> It is curious that the fix needs to be so big, when the only change\n> necessary, as far as I can tell from your problem description, is to\n> insert 4 line \"if (... not enough args ...) { ... barf and die ...}\"\n> block and without anything else. I think the culprit is this \"while\n> at it\" ...\n>\n> > While at it, let's also set the environment variable before advancing\n> > past the option, instead of advancing first and using `(*argv)[0]`, so\n> > that this option looks like the other ones.\n>\n> ... that made the patch more confusing to read than otherwise.\n\nSorry but the goal was to use similar code as other options that can\nbe passed a value like \"--git-dir\", \"--namespace\", \"--work-tree\", and\nso on.\n\n> But without reading the preimage of the patch, the result is just as\n> understandable ;-) Let's take the patch as-is.\n>\n> > +test_expect_success 'git --shallow-file without a value' '\n> > + test_must_fail git --shallow-file >actual 2>actual.err &&\n> > + test_line_count = 0 actual &&\n> > + test_grep \"no file given for \" actual.err &&\n> > + test_grep \"usage\" actual.err\n> > +'\n>\n> Do we have similar \"oops, you were supposed to give me a value\" test\n> for other things like \"--config-env=\", \"-C\", etc.?\n\nIn \"t/t1300-config.sh\" there is:\n\ntest_expect_success 'git --config-env with missing value' '\n test_must_fail env ENVVAR=value git --config-env 2>error &&\n test_grep \"no config key given for --config-env\" error &&\n test_must_fail env ENVVAR=value git --config-env config\ncore.name 2>error &&\n test_grep \"invalid config format: config\" error\n'\n\nI couldn't find anything else.\n\n> Just being\n> curious, because (1) if there are, this addition belongs there, not\n> here,\n\nI am not sure the `git --shallow-file` test belongs to \"t/t1300-config.sh\".\n\nMaybe the new test for --shallow-file with no value should be at the\nsame place as other tests for --shallow-file, unfortunately there are\nno such tests. It looks like this is an undocumented and internal only\noption which is only tested indirectly in the following files:\n\n- t5311-pack-bitmaps-shallow.sh\n- t5537-fetch-shallow.sh\n- t5538-push-shallow.sh\n- t5539-fetch-http-shallow.sh\n- t5542-push-http-shallow.sh\n- t5614-clone-submodules-shallow.sh\n\n> and (2) if there aren't, this addition may not be needed, and\n> (3) if there aren't or if the existing coverage is incomplete,\n> perhaps we should give a more complete coverage while at it.\n>\n> With (3), I mean something along the lines of ...\n>\n> for opt in -C -c --git-dir --work-tree --namespace --config-env\n> do\n> test_expect_success \"git $opt without a value\" '\n> test_must_fail git $opt >actual 2>error &&\n> test_line_count 0 actual &&\n> test_grep usage error\n> '\n> done\n>\n> I do not mean to say that (3) is my favorite among these three,\n> though.\n\nI am fine with (2) or (3), but they don't seem much better to me than\nthe test already in this patch.\n\nThanks.\n"},{"id":"550430","messageId":"xmqqecg3xnwx.fsf@gitster.g","threadId":"66153","inReplyTo":"CAP8UFD1BoXTo-bNyaQeWeC1QhrpdBAOOW4BwXCi9XYMr7aRuZw@mail.gmail.com","subject":"Re: [PATCH] git: avoid segfault on \"git --shallow-file\" without a value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-12T17:13:18Z","receivedAt":"2026-08-12T17:13:20Z","isPatch":true,"body":"Christian Couder writes:\n\n>> Just being\n>> curious, because (1) if there are, this addition belongs there, not\n>> here,\n> ...\n>> and (2) if there aren't, this addition may not be needed, and\n>> (3) if there aren't or if the existing coverage is incomplete,\n>> perhaps we should give a more complete coverage while at it.\n>>\n>> With (3), I mean something along the lines of ...\n>>\n>> for opt in -C -c --git-dir --work-tree --namespace --config-env\n>> do\n>> test_expect_success \"git $opt without a value\" '\n>> test_must_fail git $opt >actual 2>error &&\n>> test_line_count 0 actual &&\n>> test_grep usage error\n>> '\n>> done\n>>\n>> I do not mean to say that (3) is my favorite among these three,\n>> though.\n>\n> I am fine with (2) or (3), but they don't seem much better to me than\n> the test already in this patch.\n\nI think this is the case between (1) and (2), there is not much\ncoverage, and there is no coverage specific to \"git potty\" options.\n\nThe 't0041' test is a suitable place if we eventually aim for more\ncomplete coverage such as (3), instead of piecemeal tests, such as\n'test --config option with other config-related things in t1300' and\n'test --shallow-file option with other shallow-related things in\nt????'. So I think the patch is fine as-is. I will just leave a\n'#leftoverbits' comment here to remind others to consider whether it\nis worth extending the test to cover more 'git potty' options for\ncompleteness in the future.\n\nThanks.\n"},{"id":"550476","messageId":"an10XhFPo0nJWJIV@pks.im","threadId":"66153","inReplyTo":"CAP8UFD1XMY6N3UD5FhK_oeQDX7banP1e0oKM1WHUPhPv_vzbsQ@mail.gmail.com","subject":"Re: [PATCH] git: avoid segfault on \"git --shallow-file\" without a value","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-13T07:38:06Z","receivedAt":"2026-08-13T07:38:12Z","isPatch":true,"body":"On Wed, Aug 12, 2026 at 05:42:00PM +0200, Christian Couder wrote:\n> On Wed, Aug 12, 2026 at 1:22 PM Patrick Steinhardt wrote:\n> >\n> > On Tue, Aug 11, 2026 at 02:14:46PM +0200, Christian Couder wrote:\n> > > diff --git a/git.c b/git.c\n> > > index e5f1811b6b..96df15b5cd 100644\n> > > --- a/git.c\n> > > +++ b/git.c\n> > > @@ -304,11 +304,15 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n> > > if (envchanged)\n> > > *envchanged = 1;\n> > > } else if (!strcmp(cmd, \"--shallow-file\")) {\n> > > - (*argv)++;\n> > > - (*argc)--;\n> > > - setenv(GIT_SHALLOW_FILE_ENVIRONMENT, (*argv)[0], 1);\n> > > + if (*argc < 2) {\n> > > + fprintf(stderr, _(\"no file given for '%s' option\\n\" ), \"--shallow-file\");\n> > > + usage(git_usage_string);\n> >\n> > Should we maybe condense this into a single line?\n> >\n> > usage(_(\"no file given for '%s' option\\n\")), \"--shallow-file\")\n> >\n> > I think that also printing the usage string is only distracting and\n> > doesn't really give the user a lot of extra context.\n> \n> The goal of this patch is to fix the bug by using the same code as the\n> other options that can be passed a value like \"--git-dir\",\n> \"--namespace\", \"--work-tree\", and so on. Now all these options use the\n> same pattern for the error message:\n> \n> git grep -A3 'if (\\*argc < 2)' git.c\n> git.c: if (*argc < 2) {\n> git.c- fprintf(stderr, _(\"no directory given\n> for '%s' option\\n\" ), \"--git-dir\");\n> git.c- usage(git_usage_string);\n> git.c- }\n> --\n> git.c: if (*argc < 2) {\n> git.c- fprintf(stderr, _(\"no namespace given\n> for --namespace\\n\" ));\n> git.c- usage(git_usage_string);\n> git.c- }\n> --\n> git.c: if (*argc < 2) {\n> git.c- fprintf(stderr, _(\"no directory given\n> for '%s' option\\n\" ), \"--work-tree\");\n> git.c- usage(git_usage_string);\n> git.c- }\n> --\n> git.c: if (*argc < 2) {\n> git.c- fprintf(stderr, _(\"-c expects a\n> configuration string\\n\" ));\n> git.c- usage(git_usage_string);\n> git.c- }\n> --\n> git.c: if (*argc < 2) {\n> git.c- fprintf(stderr, _(\"no config key given\n> for --config-env\\n\" ));\n> git.c- usage(git_usage_string);\n> git.c- }\n> --\n> git.c: if (*argc < 2) {\n> git.c- fprintf(stderr, _(\"no directory given\n> for '%s' option\\n\" ), \"-C\");\n> git.c- usage(git_usage_string);\n> git.c- }\n> --\n> git.c: if (*argc < 2) {\n> git.c- fprintf(stderr, _(\"no attribute source\n> given for --attr-source\\n\" ));\n> git.c- usage(git_usage_string);\n> git.c- }\n> \n> So I don't think it makes sense for \"--shallow-file\" to not be\n> consistent with these other options.\n> \n> I could perhaps add a patch to the series to convert all of these to\n> something like what you suggest, but it could also be done in a\n> separate patch series by someone else.\n> \n> Anyway thanks for reviewing this patch.\n\nNo, I don't think that's really necessary. Given the existing usage I\nthink your patch looks sensible. Thanks!\n\nPatrick\n"}]}