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

> This reverts commit 68889b416d5b6a5cf7d280a428281d635fe9b292.
>
> That commit bumped some rev-parse options into the main
> option-parsing loop, which meant that they no longer had to
> come at the very beginning of the option list. However, that
> also means that they now came after our call to
> setup_git_directory(), and will no longer run outside of a
> git repository.
>
> For --local-env-vars, this is semi-questionable. The main
> use of that option is to get a list of environment variables
> to clear, and if you are not in a repository, there
> _probably_ isn't anything to clear. But it places an
> unnecessary restriction on callers who may be using it
> preemptively.
>
> For --resolve-git-dir, it is almost certainly a regression.
> That option is about finding a git dir in the first place,
> so it is reasonably likely to be called from outside an
> existing repository.
>
> 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.
>
> The original patch was not spurred by an actual bug report,
> but by an observation[1] that was essentially "eh, this
> looks unnecessarily restrictive". It _is_ restrictive, but
> it turns out to be necessarily so. :)
>
> And in practice, it is unlikely anybody was bothered by the
> restriction. It's not really sane to use --local-env-vars
> with other options, anyway, as it produces unbounded output
> (so the caller only know it ends at EOF). It's more
> plausible for --resolve-git-dir to be used with other
> options, but still unlikely. It's main use is accessing

> _other_ repositories (e.g., submodules), so making a query
> on the main repository at the same time isn't very useful.
>
> This patch therefore just reverts 68889b416, with a few
> caveats:
>
>   1. Since the original change, --resolve-git-dir learned to
>      avoid segfaulting on a bogus. We don't know need to

>      backport that, because the "restricted" form checks
>      argc.
>
>   2. The original patch mentioned that we didn't need to
>      clean up any documentation, because the restriction
>      wasn't documented. We can at least fix that by
>      mentioning the restriction in the manpage.
>
>   3. We can now mark our newly-added tests as passing. :)
>
> [1] http://article.gmane.org/gmane.comp.version-control.git/230849
>
> Signed-off-by: Jeff King <peff@peff.net>