Re: [PATCH 2/2] Revert "rev-parse: remove restrictions on some options"

[Jeff King <peff@peff.net>]

On Fri, Feb 26, 2016 at 06:29:57PM -0500, Jeff King wrote:

> The best solution here would be to have a full parsing loop
> that handles all options, but only calls setup_git_directory
> as appropriate. Unfortunately, that's a bit complicated to
> implement. We _have_ to handle each option in the order it
> appears on the command line. If the caller asked for:
> 
>   git rev-parse --resolve-git-dir foo/.git --show-toplevel
> 
> then it must receive the two lines of output in the correct
> to know which is which. But asking for:
> 
>   git rev-parse --show-toplevel --resolve-git-dir foo/.git
> 
> is weird; we have to setup_git_directory() for the first
> option.
> 
> So any implementation would probably have to either:
> 
>   - make two passes over the options, first figuring out
>     whether we need a git-dir, and then actually handling
>     the options. That's possible, but it's probably not
>     worth the trouble.
> 
>   - call setup_git_directory() on the fly when an option
>     needs it; that requires annotating all of the options,
>     and there are a considerable number of them.

Having just sent this, of course, it occurs to me that a loop like:

   setup_git_directory_gently(&nongit);
   for (i = 0; i < argc; i++) {
  	if (!strcmp(argv[i], "--local-env-vars"))
  	... and other nongit-ok options ...
  
  	if (nongit)
  		die("need a repo");
  
  	if (!strcmp(argv[i], "--git-dir"))
  	... and other options ...
   }

would probably work. It does still leave things like --parseopt and
--sq-quote as one-offs, though, because they consume the rest of the
command line. And the fact remains that --local-repo-env isn't really
suitable for use with other options.

So I'm still tempted by this "lazy" approach.

-Peff