From: Mirko Faina <mroik@delayed.space>
Date: Mon, 20 Apr 2026 09:22:09 GMT
Subject: Re: [PATCH] revision.c: implement --reverse=before for walks
Message-ID: <aeXvuuhjTmHyumGq@exploit>
In-Reply-To: <20260420000440.GA1238475@coredump.intra.peff.net>

On Sun, Apr 19, 2026 at 08:04:40PM -0400, Jeff King wrote:
> On Sat, Apr 18, 2026 at 06:47:35PM +0200, Mirko Faina wrote:
> 
> > @@ -2685,8 +2685,26 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
> >  		else
> >  			git_log_output_encoding = xstrdup("");
> >  		return argcount;
> > -	} else if (!strcmp(arg, "--reverse")) {
> > -		revs->reverse ^= 1;
> > +	} else if (starts_with(arg, "--reverse")) {
> > +		if (!skip_prefix(arg, "--reverse=", &optarg)) {
> > +			if (argc < 2) {
> > +				revs->reverse = 1;
> > +				return 1;
> > +			} else {
> > +				optarg = argv[1];
> > +			}
> > +		}
> 
> It looks like you're trying to support "--reverse after" here, but don't
> do that. Flags with optional arguments must use the "stuck" form,
> "--reverse=after", which is covered in the "gitcli" manpage.

Oh I see, I thought I had to parse both ways. If I just have to allow
for the stuck form then it becomes way easier to deal with.

> That's to prevent "--reverse --foo" from being ambiguous. It looks like
> you try to limit that with the final "else" here:
> 
> > +
> > +		if (!strcmp(optarg, "after")) {
> > +			revs->reverse = 1;
> > +		} else if (!strcmp(optarg, "before")) {
> > +			revs->reverse = 2;
> > +		} else {
> > +			revs->reverse = 1;
> > +			return 1;
> > +		}
> 
> but that just makes things more complicated:
> 
>   - doing "git log --reverse=bogus" is silently accepted
> 
>   - trying to show a branch named "after" with "git log --reverse after"
>     has changed meanings
> 
> So I think you really just want to handle "--reverse=" separately from
> "--reverse", and the latter should behave as it always has.
> 
> -Peff

Thanks you