{"thread":{"id":"60440","subject":"[PATCH] reflog: fix expire --single-worktree","startedAt":"2023-10-28T11:58:46Z","lastAt":"2023-10-31T06:16:34Z","messageCount":7,"participants":["René Scharfe","Junio C Hamano","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"484025","messageId":"63eade0e-bf2c-4906-8b4c-689797cff737@web.de","threadId":"60440","inReplyTo":null,"subject":"[PATCH] reflog: fix expire --single-worktree","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-10-28T11:58:41Z","receivedAt":"2023-10-28T11:58:46Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"33d7bdd645 (builtin/reflog.c: use parse-options api for expire, delete\nsubcommands, 2022-01-06) broke the option --single-worktree of git\nreflog expire and added a non-printable short flag for it, presumably by\naccident.  While before it set the variable \"all_worktrees\" to 0, now it\nsets it to 1, its default value.  --no-single-worktree is required now\nto set it to 0.\n\nFix it by replacing the variable with one that has the opposite meaning,\nto avoid the negation and its potential for confusion.  The new variable\n\"single_worktree\" directly captures whether --single-worktree was given.\n\nAlso remove the unprintable short flag SOH (start of heading) because it\nis undocumented, hard to use and is likely to have been added by mistake\nin connection with the negation bug above.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/reflog.c  |  6 +++---\n t/t1410-reflog.sh | 23 +++++++++++++++++++++++\n 2 files changed, 26 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex df63a5892e..21337292f5 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -243,7 +243,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n {\n \tstruct cmd_reflog_expire_cb cmd = { 0 };\n \ttimestamp_t now = time(NULL);\n-\tint i, status, do_all, all_worktrees = 1;\n+\tint i, status, do_all, single_worktree = 0;\n \tunsigned int flags = 0;\n \tint verbose = 0;\n \treflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;\n@@ -268,7 +268,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"stale-fix\", &cmd.stalefix,\n \t\t\t N_(\"prune any reflog entries that point to broken commits\")),\n \t\tOPT_BOOL(0, \"all\", &do_all, N_(\"process the reflogs of all references\")),\n-\t\tOPT_BOOL(1, \"single-worktree\", &all_worktrees,\n+\t\tOPT_BOOL(0, \"single-worktree\", &single_worktree,\n \t\t\t N_(\"limits processing to reflogs from the current worktree only\")),\n \t\tOPT_END()\n \t};\n@@ -318,7 +318,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n\n \t\tworktrees = get_worktrees();\n \t\tfor (p = worktrees; *p; p++) {\n-\t\t\tif (!all_worktrees && !(*p)->is_current)\n+\t\t\tif (single_worktree && !(*p)->is_current)\n \t\t\t\tcontinue;\n \t\t\tcollected.worktree = *p;\n \t\t\trefs_for_each_reflog(get_worktree_ref_store(*p),\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex 6c45965b1e..09e7f3cdac 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -446,6 +446,29 @@ test_expect_success 'expire with multiple worktrees' '\n \t)\n '\n\n+test_expect_success 'expire one of multiple worktrees' '\n+\tgit init main-wt2 &&\n+\t(\n+\t\tcd main-wt2 &&\n+\t\ttest_tick &&\n+\t\ttest_commit foo &&\n+\t\tgit worktree add link-wt &&\n+\t\ttest_tick &&\n+\t\ttest_commit -C link-wt foobar &&\n+\t\ttest_tick &&\n+\t\ttest-tool ref-store worktree:link-wt for-each-reflog-ent HEAD \\\n+\t\t\t>expect-link-wt &&\n+\t\tgit reflog expire --verbose --all --expire=$test_tick \\\n+\t\t\t--single-worktree &&\n+\t\ttest-tool ref-store worktree:main for-each-reflog-ent HEAD \\\n+\t\t\t>actual-main &&\n+\t\ttest-tool ref-store worktree:link-wt for-each-reflog-ent HEAD \\\n+\t\t\t>actual-link-wt &&\n+\t\ttest_must_be_empty actual-main &&\n+\t\ttest_cmp expect-link-wt actual-link-wt\n+\t)\n+'\n+\n test_expect_success REFFILES 'empty reflog' '\n \ttest_when_finished \"rm -rf empty\" &&\n \tgit init empty &&\n--\n2.42.0\n"},{"id":"484056","messageId":"xmqqa5s1hxhh.fsf@gitster.g","threadId":"60440","inReplyTo":"63eade0e-bf2c-4906-8b4c-689797cff737@web.de","subject":"Re: [PATCH] reflog: fix expire --single-worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-29T22:31:22Z","receivedAt":"2023-10-29T22:31:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> ... and added a non-printable short flag for it, presumably by\n> accident.\n\nVery well spotted.\n\nFWIW, with the following patch on top of this patch, all tests pass\n(and without your fix, of course this notices the \"\\001\" and breaks\nnumerous tests that use \"git reflog\").  So you seem to have found\nthe only one broken instance (among those that are tested, anyway).\n\n parse-options.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git i/parse-options.c w/parse-options.c\nindex 093eaf2db8..be8bedba29 100644\n--- i/parse-options.c\n+++ w/parse-options.c\n@@ -469,7 +469,8 @@ static void parse_options_check(const struct option *opts)\n \t\t\toptbug(opts, \"uses incompatible flags \"\n \t\t\t       \"LASTARG_DEFAULT and OPTARG\");\n \t\tif (opts->short_name) {\n-\t\t\tif (0x7F <= opts->short_name)\n+\t\t\tif (opts->short_name &&\n+\t\t\t    (opts->short_name < 0x21 || 0x7F <= opts->short_name))\n \t\t\t\toptbug(opts, \"invalid short name\");\n \t\t\telse if (short_opts[opts->short_name]++)\n \t\t\t\toptbug(opts, \"short name already used\");\n"},{"id":"484122","messageId":"82b832cb-da0b-47cf-9b5d-e8011a222151@web.de","threadId":"60440","inReplyTo":"xmqqa5s1hxhh.fsf@gitster.g","subject":"Re: [PATCH] reflog: fix expire --single-worktree","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-10-30T16:12:41Z","receivedAt":"2023-10-30T16:12:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 29.10.23 um 23:31 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>\n>> ... and added a non-printable short flag for it, presumably by\n>> accident.\n>\n> Very well spotted.\n>\n> FWIW, with the following patch on top of this patch, all tests pass\n> (and without your fix, of course this notices the \"\\001\" and breaks\n> numerous tests that use \"git reflog\").  So you seem to have found\n> the only one broken instance (among those that are tested, anyway).\n>\n>  parse-options.c | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git i/parse-options.c w/parse-options.c\n> index 093eaf2db8..be8bedba29 100644\n> --- i/parse-options.c\n> +++ w/parse-options.c\n> @@ -469,7 +469,8 @@ static void parse_options_check(const struct option *opts)\n>  \t\t\toptbug(opts, \"uses incompatible flags \"\n>  \t\t\t       \"LASTARG_DEFAULT and OPTARG\");\n>  \t\tif (opts->short_name) {\n> -\t\t\tif (0x7F <= opts->short_name)\n> +\t\t\tif (opts->short_name &&\n> +\t\t\t    (opts->short_name < 0x21 || 0x7F <= opts->short_name))\n\nGood idea.  This is equivalent to !isprint(opts->short_name), which I\nfind to be more readable here.  Seeing why \"char short_opts[128];\" a\nfew lines up is big enough would become a bit harder, though.\n\n>  \t\t\t\toptbug(opts, \"invalid short name\");\n>  \t\t\telse if (short_opts[opts->short_name]++)\n>  \t\t\t\toptbug(opts, \"short name already used\");\n"},{"id":"484132","messageId":"ZT/mN9RouiqzL9aT@nand.local","threadId":"60440","inReplyTo":"xmqqa5s1hxhh.fsf@gitster.g","subject":"Re: [PATCH] reflog: fix expire --single-worktree","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-30T17:21:59Z","receivedAt":"2023-10-30T17:22:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 30, 2023 at 07:31:22AM +0900, Junio C Hamano wrote:\n> René Scharfe <l.s.r@web.de> writes:\n>\n> > ... and added a non-printable short flag for it, presumably by\n> > accident.\n>\n> Very well spotted.\n>\n> FWIW, with the following patch on top of this patch, all tests pass\n> (and without your fix, of course this notices the \"\\001\" and breaks\n> numerous tests that use \"git reflog\").  So you seem to have found\n> the only one broken instance (among those that are tested, anyway).\n\nThis makes sense to me, but obviously won't catch non-tested cases.  I\nthought that a new Cocinelle rule might be appropriate here, but it is\nfrustratingly difficult to specify a constraint like:\n\n    OPT_BOOL(e1, e2, e3, ...)\n\nwith\n\n    !(e1 == 0 || (33 <= e1 && e1 <= 127))\n\nI'll think on it a little bit, but this seems low priority enough that I\ndon't feel compelled to urgently deal with adding a new Coccinelle rule.\n\nThanks,\nTaylor\n"},{"id":"484161","messageId":"xmqqlebjbt9y.fsf@gitster.g","threadId":"60440","inReplyTo":"82b832cb-da0b-47cf-9b5d-e8011a222151@web.de","subject":"Re: [PATCH] reflog: fix expire --single-worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-30T23:11:05Z","receivedAt":"2023-10-30T23:11:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Am 29.10.23 um 23:31 schrieb Junio C Hamano:\n>> René Scharfe <l.s.r@web.de> writes:\n>>\n>>> ... and added a non-printable short flag for it, presumably by\n>>> accident.\n>>\n>> Very well spotted.\n>>\n>> FWIW, with the following patch on top of this patch, all tests pass\n>> (and without your fix, of course this notices the \"\\001\" and breaks\n>> numerous tests that use \"git reflog\").  So you seem to have found\n>> the only one broken instance (among those that are tested, anyway).\n>>\n>>  parse-options.c | 3 ++-\n>>  1 file changed, 2 insertions(+), 1 deletion(-)\n>>\n>> diff --git i/parse-options.c w/parse-options.c\n>> index 093eaf2db8..be8bedba29 100644\n>> --- i/parse-options.c\n>> +++ w/parse-options.c\n>> @@ -469,7 +469,8 @@ static void parse_options_check(const struct option *opts)\n>>  \t\t\toptbug(opts, \"uses incompatible flags \"\n>>  \t\t\t       \"LASTARG_DEFAULT and OPTARG\");\n>>  \t\tif (opts->short_name) {\n>> -\t\t\tif (0x7F <= opts->short_name)\n>> +\t\t\tif (opts->short_name &&\n>> +\t\t\t    (opts->short_name < 0x21 || 0x7F <= opts->short_name))\n>\n> Good idea.  This is equivalent to !isprint(opts->short_name), which I\n> find to be more readable here.\n\nThanks---I didn't think of using !isprint() but you are right.  It\nis much shorter.\n\nI am not absolutely certain if it is easier to read, though.  I get\nalways confused when asking myself if SP, HT, and LF are printables.\n(in other words, I cannot immediately answer \"does 'printable' mean\n'can be sent to a teletype and have it do what is expected to be\ndone?\"---the question I should be asking myself is \"is 'printable'\nsynonym to 'when printed, some ink is consumed'?\").\n\n> Seeing why \"char short_opts[128];\" a\n> few lines up is big enough would become a bit harder, though.\n\nSorry, but I do not quite follow.  We used to allow anything below\n0x7e; now we clip that range further to reject anything below 0x21.\nIf [128] was big enough, it still is big enough, no?\n\nBecause the type of .short_name member is \"int\", we could have had\nnegative number in there and access to short_opts[] on the next line\nwould have been out of bounds.  By clipping the lower bound, we get\nrid of that risk, no?\n\n>>  \t\t\t\toptbug(opts, \"invalid short name\");\n>>  \t\t\telse if (short_opts[opts->short_name]++)\n>>  \t\t\t\toptbug(opts, \"short name already used\");\n"},{"id":"484162","messageId":"xmqqa5rzbsmy.fsf@gitster.g","threadId":"60440","inReplyTo":"ZT/mN9RouiqzL9aT@nand.local","subject":"Re: [PATCH] reflog: fix expire --single-worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-30T23:24:53Z","receivedAt":"2023-10-30T23:24:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> This makes sense to me, but obviously won't catch non-tested cases.\n\nTrue, but I think we can practically ignore non-tested cases.\n\nThe parse_options_check() is used to validate the whole options[]\narray that is passed to parse_options() family of API functions, and\nits validation is not limited to the options that are given from the\ncommand line in an invocation.  A non-tested case would happen when\na developer prepares and populates \"struct option options[];\" array\nfor a (possibly new) git subcommand *and* never uses that array to\ncall parse_options() in their implementation of that subcommand.\nThe compiler would catch the unused variable options[] in such a\ncase, and mark 1 eyeball would notice that none of the options\ndefined in that array are actually understood by the command, no?\n"},{"id":"484192","messageId":"59ea250d-364e-4a41-9511-4e2ff1a7cfb4@web.de","threadId":"60440","inReplyTo":"xmqqlebjbt9y.fsf@gitster.g","subject":"Re: [PATCH] reflog: fix expire --single-worktree","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-10-31T06:16:24Z","receivedAt":"2023-10-31T06:16:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 31.10.23 um 00:11 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>\n>> Am 29.10.23 um 23:31 schrieb Junio C Hamano:\n>>> René Scharfe <l.s.r@web.de> writes:\n>>>\n>>> diff --git i/parse-options.c w/parse-options.c\n>>> index 093eaf2db8..be8bedba29 100644\n>>> --- i/parse-options.c\n>>> +++ w/parse-options.c\n>>> @@ -469,7 +469,8 @@ static void parse_options_check(const struct option *opts)\n>>>  \t\t\toptbug(opts, \"uses incompatible flags \"\n>>>  \t\t\t       \"LASTARG_DEFAULT and OPTARG\");\n>>>  \t\tif (opts->short_name) {\n>>> -\t\t\tif (0x7F <= opts->short_name)\n>>> +\t\t\tif (opts->short_name &&\n>>> +\t\t\t    (opts->short_name < 0x21 || 0x7F <= opts->short_name))\n>>\n>> Good idea.  This is equivalent to !isprint(opts->short_name), which I\n>> find to be more readable here.\n>\n> Thanks---I didn't think of using !isprint() but you are right.  It\n> is much shorter.\n>\n> I am not absolutely certain if it is easier to read, though.  I get\n> always confused when asking myself if SP, HT, and LF are printables.\n> (in other words, I cannot immediately answer \"does 'printable' mean\n> 'can be sent to a teletype and have it do what is expected to be\n> done?\"---the question I should be asking myself is \"is 'printable'\n> synonym to 'when printed, some ink is consumed'?\").\n\nisprint() accepts SP, but not HT or LF.  Go figure.  And thus I made an\noff-by-one error by suggesting this macro, because your version rejects\nSP (0x20).  Am I unintentionally making a point here for using the\nis-macros because I can't read numeric comparisons? O_o\n\nisalnum() and ispunct() could be used instead.\n\n>> Seeing why \"char short_opts[128];\" a\n>> few lines up is big enough would become a bit harder, though.\n>\n> Sorry, but I do not quite follow.  We used to allow anything below\n> 0x7e; now we clip that range further to reject anything below 0x21.\n> If [128] was big enough, it still is big enough, no?\n>\n> Because the type of .short_name member is \"int\", we could have had\n> negative number in there and access to short_opts[] on the next line\n> would have been out of bounds.  By clipping the lower bound, we get\n> rid of that risk, no?\n\nYes, but if the allowed range is hidden behind macro invocations then\nthe boundaries are no longer as obvious as in your version.\n\n>>>  \t\t\t\toptbug(opts, \"invalid short name\");\n>>>  \t\t\telse if (short_opts[opts->short_name]++)\n>>>  \t\t\t\toptbug(opts, \"short name already used\");\n"}]}