Re: [PATCH 2/4] rev-list: refactor early option parsing

From: Justin Tobler <jltobler@gmail.com>
Date: Mar 12, 2025, 21:39 UTC
Message-ID: <zozymxxh4s2qbh2cknqyy7z4ktgppm7i62bhkgupnij4enkq5n@6v2icwdt77nc>
In-Reply-To: <xmqqr03461z7.fsf@gitster.g>

On 25/03/10 01:54PM, Junio C Hamano wrote:

Show 26 quoted lines

> Justin Tobler <jltobler@gmail.com> writes:
> 
> > @@ -639,19 +640,15 @@ int cmd_rev_list(int argc,
> >  		if (!strcmp(arg, "--exclude-promisor-objects")) {
> >  			fetch_if_missing = 0;
> >  			revs.exclude_promisor_objects = 1;
> > -			break;
> > -		}
> > -	}
> > -	for (i = 1; i < argc; i++) {
> > -		const char *arg = argv[i];
> > -		if (skip_prefix(arg, "--missing=", &arg)) {
> > -			if (revs.exclude_promisor_objects)
> > -				die(_("options '%s' and '%s' cannot be used together"), "--exclude-promisor-objects", "--missing");
> > -			if (parse_missing_action_value(arg))
> > -				break;
> > +		} else if (skip_prefix(arg, "--missing=", &arg)) {
> > +			parse_missing_action_value(arg);
> >  		}
> >  	}
> 
> There is a huge NEEDSWORK comment that essentially says that the
> above two loops that this patch combines into one is fundamentally
> broken.  I suspect that the remaining two patches in this series
> would punt and not improve them, but offhand I am not sure if this
> change is making it harder to fix them properly easier or harder.

My understanding here is that `setup_revisions()` does not provide a mechanism to reject certain individual or combinations of parsed options that do not make sense for a command. As you mentioned, this patch is just punting on that issue, but I don't think it is really making the problem any harder to be resolved in the future.

In this case, the `-z` option is parsed early to avoid is being handled by the diff options parsing in `setup_revisions()`. From the perspective of git-rev-list(1), I don't think there is any reason to be parsing diff options at all. We could introduce an option to disable diff options parsing in `setup_revisions()`, but now that we also want to modify stdin argument parsing to be NUL-delimited, we would still have to parse the -z option early in order to configure `setup_revisions()` appropriately.

Another way we could potentially resolve these issues would be to optionally separate the argument/option parsing logic out of `setup_revisions()` and expose it directly. That could give the caller more control over how options are handled before fully setting up the `rev_info` structure. I suspect this would be non-trivial though, so I think it would preferrable to keep the current approach for now as it isn't making the problem harder to fix IMO.

-Justin