From: Jeff King <peff@peff.net>
Date: Mon, 20 Apr 2026 00:04:40 GMT
Subject: Re: [PATCH] revision.c: implement --reverse=before for walks
Message-ID: <20260420000440.GA1238475@coredump.intra.peff.net>
In-Reply-To: <20260418164736.2367523-2-mroik@delayed.space>

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.

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