| Message ID | 63eade0e-bf2c-4906-8b4c-689797cff737@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | reflog: fix expire --single-worktree | expand |
René Scharfe <l.s.r@web.de> writes: > ... and added a non-printable short flag for it, presumably by > accident. Very well spotted. FWIW, with the following patch on top of this patch, all tests pass (and without your fix, of course this notices the "\001" and breaks numerous tests that use "git reflog"). So you seem to have found the only one broken instance (among those that are tested, anyway). parse-options.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git i/parse-options.c w/parse-options.c index 093eaf2db8..be8bedba29 100644 --- i/parse-options.c +++ w/parse-options.c @@ -469,7 +469,8 @@ static void parse_options_check(const struct option *opts) optbug(opts, "uses incompatible flags " "LASTARG_DEFAULT and OPTARG"); if (opts->short_name) { - if (0x7F <= opts->short_name) + if (opts->short_name && + (opts->short_name < 0x21 || 0x7F <= opts->short_name)) optbug(opts, "invalid short name"); else if (short_opts[opts->short_name]++) optbug(opts, "short name already used");
Am 29.10.23 um 23:31 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > >> ... and added a non-printable short flag for it, presumably by >> accident. > > Very well spotted. > > FWIW, with the following patch on top of this patch, all tests pass > (and without your fix, of course this notices the "\001" and breaks > numerous tests that use "git reflog"). So you seem to have found > the only one broken instance (among those that are tested, anyway). > > parse-options.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git i/parse-options.c w/parse-options.c > index 093eaf2db8..be8bedba29 100644 > --- i/parse-options.c > +++ w/parse-options.c > @@ -469,7 +469,8 @@ static void parse_options_check(const struct option *opts) > optbug(opts, "uses incompatible flags " > "LASTARG_DEFAULT and OPTARG"); > if (opts->short_name) { > - if (0x7F <= opts->short_name) > + if (opts->short_name && > + (opts->short_name < 0x21 || 0x7F <= opts->short_name)) Good idea. This is equivalent to !isprint(opts->short_name), which I find to be more readable here. Seeing why "char short_opts[128];" a few lines up is big enough would become a bit harder, though. > optbug(opts, "invalid short name"); > else if (short_opts[opts->short_name]++) > optbug(opts, "short name already used");
On Mon, Oct 30, 2023 at 07:31:22AM +0900, Junio C Hamano wrote: > René Scharfe <l.s.r@web.de> writes: > > > ... and added a non-printable short flag for it, presumably by > > accident. > > Very well spotted. > > FWIW, with the following patch on top of this patch, all tests pass > (and without your fix, of course this notices the "\001" and breaks > numerous tests that use "git reflog"). So you seem to have found > the only one broken instance (among those that are tested, anyway). This makes sense to me, but obviously won't catch non-tested cases. I thought that a new Cocinelle rule might be appropriate here, but it is frustratingly difficult to specify a constraint like: OPT_BOOL(e1, e2, e3, ...) with !(e1 == 0 || (33 <= e1 && e1 <= 127)) I'll think on it a little bit, but this seems low priority enough that I don't feel compelled to urgently deal with adding a new Coccinelle rule. Thanks, Taylor
René Scharfe <l.s.r@web.de> writes: > Am 29.10.23 um 23:31 schrieb Junio C Hamano: >> René Scharfe <l.s.r@web.de> writes: >> >>> ... and added a non-printable short flag for it, presumably by >>> accident. >> >> Very well spotted. >> >> FWIW, with the following patch on top of this patch, all tests pass >> (and without your fix, of course this notices the "\001" and breaks >> numerous tests that use "git reflog"). So you seem to have found >> the only one broken instance (among those that are tested, anyway). >> >> parse-options.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git i/parse-options.c w/parse-options.c >> index 093eaf2db8..be8bedba29 100644 >> --- i/parse-options.c >> +++ w/parse-options.c >> @@ -469,7 +469,8 @@ static void parse_options_check(const struct option *opts) >> optbug(opts, "uses incompatible flags " >> "LASTARG_DEFAULT and OPTARG"); >> if (opts->short_name) { >> - if (0x7F <= opts->short_name) >> + if (opts->short_name && >> + (opts->short_name < 0x21 || 0x7F <= opts->short_name)) > > Good idea. This is equivalent to !isprint(opts->short_name), which I > find to be more readable here. Thanks---I didn't think of using !isprint() but you are right. It is much shorter. I am not absolutely certain if it is easier to read, though. I get always confused when asking myself if SP, HT, and LF are printables. (in other words, I cannot immediately answer "does 'printable' mean 'can be sent to a teletype and have it do what is expected to be done?"---the question I should be asking myself is "is 'printable' synonym to 'when printed, some ink is consumed'?"). > Seeing why "char short_opts[128];" a > few lines up is big enough would become a bit harder, though. Sorry, but I do not quite follow. We used to allow anything below 0x7e; now we clip that range further to reject anything below 0x21. If [128] was big enough, it still is big enough, no? Because the type of .short_name member is "int", we could have had negative number in there and access to short_opts[] on the next line would have been out of bounds. By clipping the lower bound, we get rid of that risk, no? >> optbug(opts, "invalid short name"); >> else if (short_opts[opts->short_name]++) >> optbug(opts, "short name already used");
Taylor Blau <me@ttaylorr.com> writes:
> This makes sense to me, but obviously won't catch non-tested cases.
True, but I think we can practically ignore non-tested cases.
The parse_options_check() is used to validate the whole options[]
array that is passed to parse_options() family of API functions, and
its validation is not limited to the options that are given from the
command line in an invocation. A non-tested case would happen when
a developer prepares and populates "struct option options[];" array
for a (possibly new) git subcommand *and* never uses that array to
call parse_options() in their implementation of that subcommand.
The compiler would catch the unused variable options[] in such a
case, and mark 1 eyeball would notice that none of the options
defined in that array are actually understood by the command, no?
Am 31.10.23 um 00:11 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > >> Am 29.10.23 um 23:31 schrieb Junio C Hamano: >>> René Scharfe <l.s.r@web.de> writes: >>> >>> diff --git i/parse-options.c w/parse-options.c >>> index 093eaf2db8..be8bedba29 100644 >>> --- i/parse-options.c >>> +++ w/parse-options.c >>> @@ -469,7 +469,8 @@ static void parse_options_check(const struct option *opts) >>> optbug(opts, "uses incompatible flags " >>> "LASTARG_DEFAULT and OPTARG"); >>> if (opts->short_name) { >>> - if (0x7F <= opts->short_name) >>> + if (opts->short_name && >>> + (opts->short_name < 0x21 || 0x7F <= opts->short_name)) >> >> Good idea. This is equivalent to !isprint(opts->short_name), which I >> find to be more readable here. > > Thanks---I didn't think of using !isprint() but you are right. It > is much shorter. > > I am not absolutely certain if it is easier to read, though. I get > always confused when asking myself if SP, HT, and LF are printables. > (in other words, I cannot immediately answer "does 'printable' mean > 'can be sent to a teletype and have it do what is expected to be > done?"---the question I should be asking myself is "is 'printable' > synonym to 'when printed, some ink is consumed'?"). isprint() accepts SP, but not HT or LF. Go figure. And thus I made an off-by-one error by suggesting this macro, because your version rejects SP (0x20). Am I unintentionally making a point here for using the is-macros because I can't read numeric comparisons? O_o isalnum() and ispunct() could be used instead. >> Seeing why "char short_opts[128];" a >> few lines up is big enough would become a bit harder, though. > > Sorry, but I do not quite follow. We used to allow anything below > 0x7e; now we clip that range further to reject anything below 0x21. > If [128] was big enough, it still is big enough, no? > > Because the type of .short_name member is "int", we could have had > negative number in there and access to short_opts[] on the next line > would have been out of bounds. By clipping the lower bound, we get > rid of that risk, no? Yes, but if the allowed range is hidden behind macro invocations then the boundaries are no longer as obvious as in your version. >>> optbug(opts, "invalid short name"); >>> else if (short_opts[opts->short_name]++) >>> optbug(opts, "short name already used");
diff --git a/builtin/reflog.c b/builtin/reflog.c index df63a5892e..21337292f5 100644 --- a/builtin/reflog.c +++ b/builtin/reflog.c @@ -243,7 +243,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix) { struct cmd_reflog_expire_cb cmd = { 0 }; timestamp_t now = time(NULL); - int i, status, do_all, all_worktrees = 1; + int i, status, do_all, single_worktree = 0; unsigned int flags = 0; int verbose = 0; reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent; @@ -268,7 +268,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix) OPT_BOOL(0, "stale-fix", &cmd.stalefix, N_("prune any reflog entries that point to broken commits")), OPT_BOOL(0, "all", &do_all, N_("process the reflogs of all references")), - OPT_BOOL(1, "single-worktree", &all_worktrees, + OPT_BOOL(0, "single-worktree", &single_worktree, N_("limits processing to reflogs from the current worktree only")), OPT_END() }; @@ -318,7 +318,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix) worktrees = get_worktrees(); for (p = worktrees; *p; p++) { - if (!all_worktrees && !(*p)->is_current) + if (single_worktree && !(*p)->is_current) continue; collected.worktree = *p; refs_for_each_reflog(get_worktree_ref_store(*p), diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh index 6c45965b1e..09e7f3cdac 100755 --- a/t/t1410-reflog.sh +++ b/t/t1410-reflog.sh @@ -446,6 +446,29 @@ test_expect_success 'expire with multiple worktrees' ' ) ' +test_expect_success 'expire one of multiple worktrees' ' + git init main-wt2 && + ( + cd main-wt2 && + test_tick && + test_commit foo && + git worktree add link-wt && + test_tick && + test_commit -C link-wt foobar && + test_tick && + test-tool ref-store worktree:link-wt for-each-reflog-ent HEAD \ + >expect-link-wt && + git reflog expire --verbose --all --expire=$test_tick \ + --single-worktree && + test-tool ref-store worktree:main for-each-reflog-ent HEAD \ + >actual-main && + test-tool ref-store worktree:link-wt for-each-reflog-ent HEAD \ + >actual-link-wt && + test_must_be_empty actual-main && + test_cmp expect-link-wt actual-link-wt + ) +' + test_expect_success REFFILES 'empty reflog' ' test_when_finished "rm -rf empty" && git init empty &&
33d7bdd645 (builtin/reflog.c: use parse-options api for expire, delete subcommands, 2022-01-06) broke the option --single-worktree of git reflog expire and added a non-printable short flag for it, presumably by accident. While before it set the variable "all_worktrees" to 0, now it sets it to 1, its default value. --no-single-worktree is required now to set it to 0. Fix it by replacing the variable with one that has the opposite meaning, to avoid the negation and its potential for confusion. The new variable "single_worktree" directly captures whether --single-worktree was given. Also remove the unprintable short flag SOH (start of heading) because it is undocumented, hard to use and is likely to have been added by mistake in connection with the negation bug above. Signed-off-by: René Scharfe <l.s.r@web.de> --- builtin/reflog.c | 6 +++--- t/t1410-reflog.sh | 23 +++++++++++++++++++++++ 2 files changed, 26 insertions(+), 3 deletions(-) -- 2.42.0