On 25/03/10 01:54PM, Junio C Hamano wrote:
> Justin Tobler <jltobler@xxxxxxxxx> 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
