On Fri, Feb 26, 2016 at 6:29 PM, Jeff King <peff@xxxxxxxx> wrote: > 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 s/correct/& order/ > 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 s/It's/Its/ > _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 s/bogus/& argument/ > 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@xxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html