[PATCH/RFC 0/2] fix some rev-parse options in non-repos

[PATCH/RFC 0/2] fix some rev-parse options in non-repos

[Jeff King]

Michael Haggerty noticed recently (off-list) that "git rev-parse
--local-env-vars" doesn't work outside of a git repository. This turns
out to be a regression in v1.8.5, due to a patch by John Keeping that
lifted some other restrictions on how the option could be used.

This fixes it by reverting John's patch, which puts the original
restrictions back in place. I won't repeat the lengthy discussion from
patch 2's commit message here, but the gist of it is that probably
nobody cares about those restrictions, it's more important to fix the
original regression, and it's probably too hard to make both work.

The only thing that gives me pause (and hence the RFC) is that it has
been over 2 years since the original regression. So it's entirely
possible somebody will consider _this_ fix a regression.

  [1/2]: t1515: add tests for rev-parse out-of-repo helpers
  [2/2]: Revert "rev-parse: remove restrictions on some options"

-Peff

[PATCH 1/2] t1515: add tests for rev-parse out-of-repo helpers

[Jeff King]

The git-rev-parse command is a dumping ground for helpers
that let scripts make various queries of git. Many of these
are conceptually independent of being inside a git
repository.

With the exception of --parseopt, we do not directly test
most of these features in our test suite. Let's give them
some basic sanity checks, which reveals that some of them
have been broken for some time when run from outside a
repository.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t1515-rev-parse-outside-repo.sh | 45 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100755 t/t1515-rev-parse-outside-repo.sh

diff --git a/t/t1515-rev-parse-outside-repo.sh b/t/t1515-rev-parse-outside-repo.sh
new file mode 100755
index 0000000..ae33093
--- /dev/null
+++ b/t/t1515-rev-parse-outside-repo.sh
@@ -0,0 +1,45 @@
+#!/bin/sh
+
+test_description='check that certain rev-parse options work outside repo'
+. ./test-lib.sh
+
+test_expect_success 'set up non-repo directory' '
+	GIT_CEILING_DIRECTORIES=$(pwd) &&
+	export GIT_CEILING_DIRECTORIES &&
+	mkdir non-repo &&
+	cd non-repo &&
+	# confirm that git does not find a repo
+	test_must_fail git rev-parse --git-dir
+'
+
+# Rather than directly test the output of sq-quote directly,
+# make sure the shell can read back a tricky case, since
+# that's what we really care about anyway.
+tricky="really tricky with \\ and \" and '"
+dump_args () {
+	for i in "$@"; do
+		echo "arg: $i"
+	done
+}
+test_expect_success 'rev-parse --sq-quote' '
+	dump_args "$tricky" easy >expect &&
+	eval "dump_args $(git rev-parse --sq-quote "$tricky" easy)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_failure 'rev-parse --local-env-vars' '
+	git rev-parse --local-env-vars >actual &&
+	# we do not want to depend on the complete list here,
+	# so just look for something plausible
+	grep ^GIT_DIR actual
+'
+
+test_expect_failure 'rev-parse --resolve-git-dir' '
+	git init --separate-git-dir repo dir &&
+	test_must_fail git rev-parse --resolve-git-dir . &&
+	echo "$(pwd)/repo" >expect &&
+	git rev-parse --resolve-git-dir dir/.git >actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
2.7.2.767.g705917e

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

[Jeff King]

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>
---
 Documentation/git-rev-parse.txt   | 27 ++++++++++++++-------------
 builtin/rev-parse.c               | 31 +++++++++++++++----------------
 t/t1515-rev-parse-outside-repo.sh |  4 ++--
 3 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index b6c6326..0f2bb9b 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -28,7 +28,8 @@ OPTIONS
 Operation Modes
 ~~~~~~~~~~~~~~~
 
-Each of these options must appear first on the command line.
+Each of these options must appear first on the command line; they do not
+need to be run in a git repository.
 
 --parseopt::
 	Use 'git rev-parse' in option parsing mode (see PARSEOPT section below).
@@ -38,6 +39,18 @@ Each of these options must appear first on the command line.
 	section below). In contrast to the `--sq` option below, this
 	mode does only quoting. Nothing else is done to command input.
 
+--local-env-vars::
+	List the GIT_* environment variables that are local to the
+	repository (e.g. GIT_DIR or GIT_WORK_TREE, but not GIT_EDITOR).
+	Only the names of the variables are listed, not their value,
+	even if they are set.
+
+--resolve-git-dir <path>::
+	Check if <path> is a valid repository or a gitfile that
+	points at a valid repository, and print the location of the
+	repository.  If <path> is a gitfile then the resolved path
+	to the real repository is printed.
+
 Options for --parseopt
 ~~~~~~~~~~~~~~~~~~~~~~
 
@@ -201,12 +214,6 @@ explicitly.
 Options for Files
 ~~~~~~~~~~~~~~~~~
 
---local-env-vars::
-	List the GIT_* environment variables that are local to the
-	repository (e.g. GIT_DIR or GIT_WORK_TREE, but not GIT_EDITOR).
-	Only the names of the variables are listed, not their value,
-	even if they are set.
-
 --git-dir::
 	Show `$GIT_DIR` if defined. Otherwise show the path to
 	the .git directory. The path shown, when relative, is
@@ -230,12 +237,6 @@ print a message to stderr and exit with nonzero status.
 --is-bare-repository::
 	When the repository is bare print "true", otherwise "false".
 
---resolve-git-dir <path>::
-	Check if <path> is a valid repository or a gitfile that
-	points at a valid repository, and print the location of the
-	repository.  If <path> is a gitfile then the resolved path
-	to the real repository is printed.
-
 --git-path <path>::
 	Resolve "$GIT_DIR/<path>" and takes other path relocation
 	variables such as $GIT_OBJECT_DIRECTORY,
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index cf8487b..ccc0328 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -518,6 +518,21 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 	if (argc > 1 && !strcmp("--sq-quote", argv[1]))
 		return cmd_sq_quote(argc - 2, argv + 2);
 
+	if (argc == 2 && !strcmp("--local-env-vars", argv[1])) {
+		int i;
+		for (i = 0; local_repo_env[i]; i++)
+			printf("%s\n", local_repo_env[i]);
+		return 0;
+	}
+
+	if (argc > 2 && !strcmp(argv[1], "--resolve-git-dir")) {
+		const char *gitdir = resolve_gitdir(argv[2]);
+		if (!gitdir)
+			die("not a gitdir '%s'", argv[2]);
+		puts(gitdir);
+		return 0;
+	}
+
 	if (argc > 1 && !strcmp("-h", argv[1]))
 		usage(builtin_rev_parse_usage);
 
@@ -706,12 +721,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				add_ref_exclusion(&ref_excludes, arg + 10);
 				continue;
 			}
-			if (!strcmp(arg, "--local-env-vars")) {
-				int i;
-				for (i = 0; local_repo_env[i]; i++)
-					printf("%s\n", local_repo_env[i]);
-				continue;
-			}
 			if (!strcmp(arg, "--show-toplevel")) {
 				const char *work_tree = get_git_work_tree();
 				if (work_tree)
@@ -767,16 +776,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				puts(prefix_filename(pfx, strlen(pfx), get_git_common_dir()));
 				continue;
 			}
-			if (!strcmp(arg, "--resolve-git-dir")) {
-				const char *gitdir = argv[++i];
-				if (!gitdir)
-					die("--resolve-git-dir requires an argument");
-				gitdir = resolve_gitdir(gitdir);
-				if (!gitdir)
-					die("not a gitdir '%s'", argv[i]);
-				puts(gitdir);
-				continue;
-			}
 			if (!strcmp(arg, "--is-inside-git-dir")) {
 				printf("%s\n", is_inside_git_dir() ? "true"
 						: "false");
diff --git a/t/t1515-rev-parse-outside-repo.sh b/t/t1515-rev-parse-outside-repo.sh
index ae33093..3ec2971 100755
--- a/t/t1515-rev-parse-outside-repo.sh
+++ b/t/t1515-rev-parse-outside-repo.sh
@@ -27,14 +27,14 @@ test_expect_success 'rev-parse --sq-quote' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'rev-parse --local-env-vars' '
+test_expect_success 'rev-parse --local-env-vars' '
 	git rev-parse --local-env-vars >actual &&
 	# we do not want to depend on the complete list here,
 	# so just look for something plausible
 	grep ^GIT_DIR actual
 '
 
-test_expect_failure 'rev-parse --resolve-git-dir' '
+test_expect_success 'rev-parse --resolve-git-dir' '
 	git init --separate-git-dir repo dir &&
 	test_must_fail git rev-parse --resolve-git-dir . &&
 	echo "$(pwd)/repo" >expect &&
-- 
2.7.2.767.g705917e

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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

Yup.

But why do you even need to run local-env-vars from outside a
repository in the first place?

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

[Jeff King]

On Fri, Feb 26, 2016 at 03:44:01PM -0800, Junio C Hamano wrote:

> But why do you even need to run local-env-vars from outside a
> repository in the first place?

The short answer is: because it is about clearing the state to move into
a new repository, and we do not necessarily know what the old state was.

Here's a longer answer.

We (GitHub) have some scripts that preemptively clear the git env when
moving into another repository directory, so that the environment
doesn't lead to us operating in the wrong repository.

For example, we use alternates to share object storage between a series
of forks. So frequently in such scripts we may need to switch between
repositories (e.g., to sync a fork to the shared storage, and then go
back to the shared storage and run repack). To do so safely, we have to
clear the git env for each "cd" (otherwise things work fine when
$GIT_DIR is not set and we rely on auto-finding the repo, and break
horribly if the script is run with $GIT_DIR set).

There are a few corner cases where library code wants to "cd", but
doesn't know if it's coming from another repo or not.  So it clears the
git env to be on the safe side. We could fix it by always going to the
new repo and running "unset $(git rev-parse --local-env-vars)" there,
but I think that just has the opposite problem (you may be _leaving_ a
repository, and want to make sure you are no longer in one).

For us, it's mostly just an annoyance. rev-parse produces no output so
we don't clear any variables, and its stderr gets logged somewhere. We
really only care about $GIT_DIR, and if that is set to something valid,
then you are in a repo, rev-parse works, and we clear it. But you can
come up with cases where it does the wrong thing, like:

  # Work in some repo; set some git vars in the environment, but
  # rely on auto-discovery to find the actual repo.
  cd some-git-repo
  export GIT_WORK_TREE=/whatever
  git ...

  # Now go back to our root and do some work elsewhere.
  # We're no longer in a git repo.
  cd ..
  ... run some non-git commands ...

  # Now we want to go into a new repo. Clear the environment.
  unset $(git rev-parse --local-env-vars)
  cd ../another-git-repo
  git ...

In the third directory, you'd still have GIT_WORK_TREE set, even though
you asked to clear existing git state.

I don't think we have any scripts that do that, but it doesn't seem that
implausible to me.

-Peff

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

[Jeff King]

On Fri, Feb 26, 2016 at 06:34:49PM -0500, Jeff King wrote:

> > 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 ...
>    }

Actually, it is even easier than that. Those options don't care whether
they are in a repo or not, so we can just wait to call
setup_git_directory() in the loop.

IOW, I think my 2/2 should be replaced with this:

-- >8 --
Subject: [PATCH] rev-parse: let some options run outside repository

Once upon a time, you could use "--local-env-vars" and
"--resolve-git-dir" outside of any git repository, but they
had to come first on the command line. Commit 68889b4
(rev-parse: remove restrictions on some options, 2013-07-21)
put them into the normal option-parsing loop, fixing the
latter. But it inadvertently broke the former, as we call
setup_git_directory() before starting that loop.

We can note that those options don't care even conditionally
about whether we are in a git repo. So it's fine if we
simply wait to setup the repo until we see an option that
needs it.

However, there is one special exception we should make:
historically, rev-parse will set up the repository and read
config even if there are _no_ options. Some of the
tests in t1300 rely on this to check "git -c $config"
parsing. That's not mirroring real-world use, and we could
tweak the test.  But t0002 uses a bare "git rev-parse" to
check "are we in a git repository?". It's plausible that
real-world scripts are relying on this.

So let's cover this case specially, and treat an option-less
"rev-parse" as "see if we're in a repo".

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/rev-parse.c               | 50 +++++++++++++++++++++++++--------------
 t/t1515-rev-parse-outside-repo.sh |  4 ++--
 2 files changed, 34 insertions(+), 20 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index cf8487b..c961b74 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -505,6 +505,7 @@ N_("git rev-parse --parseopt [<options>] -- [<args>...]\n"
 int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 {
 	int i, as_is = 0, verify = 0, quiet = 0, revs_count = 0, type = 0;
+	int did_repo_setup = 0;
 	int has_dashdash = 0;
 	int output_prefix = 0;
 	unsigned char sha1[20];
@@ -528,11 +529,40 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	prefix = setup_git_directory();
-	git_config(git_default_config, NULL);
+	/* No options; just report on whether we're in a git repo or not. */
+	if (argc == 1) {
+		setup_git_directory();
+		git_config(git_default_config, NULL);
+		return 0;
+	}
+
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 
+		if (!strcmp(arg, "--local-env-vars")) {
+			int i;
+			for (i = 0; local_repo_env[i]; i++)
+				printf("%s\n", local_repo_env[i]);
+			continue;
+		}
+		if (!strcmp(arg, "--resolve-git-dir")) {
+			const char *gitdir = argv[++i];
+			if (!gitdir)
+				die("--resolve-git-dir requires an argument");
+			gitdir = resolve_gitdir(gitdir);
+			if (!gitdir)
+				die("not a gitdir '%s'", argv[i]);
+			puts(gitdir);
+			continue;
+		}
+
+		/* The rest of the options require a git repository. */
+		if (!did_repo_setup) {
+			prefix = setup_git_directory();
+			git_config(git_default_config, NULL);
+			did_repo_setup = 1;
+		}
+
 		if (!strcmp(arg, "--git-path")) {
 			if (!argv[i + 1])
 				die("--git-path requires an argument");
@@ -706,12 +736,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				add_ref_exclusion(&ref_excludes, arg + 10);
 				continue;
 			}
-			if (!strcmp(arg, "--local-env-vars")) {
-				int i;
-				for (i = 0; local_repo_env[i]; i++)
-					printf("%s\n", local_repo_env[i]);
-				continue;
-			}
 			if (!strcmp(arg, "--show-toplevel")) {
 				const char *work_tree = get_git_work_tree();
 				if (work_tree)
@@ -767,16 +791,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				puts(prefix_filename(pfx, strlen(pfx), get_git_common_dir()));
 				continue;
 			}
-			if (!strcmp(arg, "--resolve-git-dir")) {
-				const char *gitdir = argv[++i];
-				if (!gitdir)
-					die("--resolve-git-dir requires an argument");
-				gitdir = resolve_gitdir(gitdir);
-				if (!gitdir)
-					die("not a gitdir '%s'", argv[i]);
-				puts(gitdir);
-				continue;
-			}
 			if (!strcmp(arg, "--is-inside-git-dir")) {
 				printf("%s\n", is_inside_git_dir() ? "true"
 						: "false");
diff --git a/t/t1515-rev-parse-outside-repo.sh b/t/t1515-rev-parse-outside-repo.sh
index ae33093..3ec2971 100755
--- a/t/t1515-rev-parse-outside-repo.sh
+++ b/t/t1515-rev-parse-outside-repo.sh
@@ -27,14 +27,14 @@ test_expect_success 'rev-parse --sq-quote' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'rev-parse --local-env-vars' '
+test_expect_success 'rev-parse --local-env-vars' '
 	git rev-parse --local-env-vars >actual &&
 	# we do not want to depend on the complete list here,
 	# so just look for something plausible
 	grep ^GIT_DIR actual
 '
 
-test_expect_failure 'rev-parse --resolve-git-dir' '
+test_expect_success 'rev-parse --resolve-git-dir' '
 	git init --separate-git-dir repo dir &&
 	test_must_fail git rev-parse --resolve-git-dir . &&
 	echo "$(pwd)/repo" >expect &&
-- 
2.8.0.rc0.276.gddf4100

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> IOW, I think my 2/2 should be replaced with this:

This looks sensible.

Don't we still want the documentation updates from the previous 2/2?

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

[Jeff King]

On Mon, Feb 29, 2016 at 09:32:22AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > IOW, I think my 2/2 should be replaced with this:
> 
> This looks sensible.
> 
> Don't we still want the documentation updates from the previous 2/2?

I don't think so. They were primarily about moving those option blocks
to the "these options must come first..." section, which is no longer
true.

It also added "you don't have to be in a git repository for these" to
that section, but I think that is less important. We could add that
individually to each option, I guess.

-Peff

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

[John Keeping]

On Fri, Feb 26, 2016 at 06:29:57PM -0500, Jeff King wrote:
> This reverts commit 68889b416d5b6a5cf7d280a428281d635fe9b292.
[snip]
> 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. :)

The aim of the original series was to improve the documentation, so I
don't think it's unreasonable to consider this a regression and revert
the functional change.  Although I think we can improve the behaviour
slightly (see below).

> diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
> index b6c6326..0f2bb9b 100644
> --- a/Documentation/git-rev-parse.txt
> +++ b/Documentation/git-rev-parse.txt
> @@ -28,7 +28,8 @@ OPTIONS
>  Operation Modes
>  ~~~~~~~~~~~~~~~
>  
> -Each of these options must appear first on the command line.
> +Each of these options must appear first on the command line; they do not
> +need to be run in a git repository.
>  
>  --parseopt::
>  	Use 'git rev-parse' in option parsing mode (see PARSEOPT section below).
> @@ -38,6 +39,18 @@ Each of these options must appear first on the command line.
>  	section below). In contrast to the `--sq` option below, this
>  	mode does only quoting. Nothing else is done to command input.
>  
> +--local-env-vars::
> +	List the GIT_* environment variables that are local to the
> +	repository (e.g. GIT_DIR or GIT_WORK_TREE, but not GIT_EDITOR).
> +	Only the names of the variables are listed, not their value,
> +	even if they are set.

I think we should add:

	No other arguments may be supplied.

> +--resolve-git-dir <path>::
> +	Check if <path> is a valid repository or a gitfile that
> +	points at a valid repository, and print the location of the
> +	repository.  If <path> is a gitfile then the resolved path
> +	to the real repository is printed.

Again I think this should say that only the `path` argument may be
supplied.

>  --git-path <path>::
>  	Resolve "$GIT_DIR/<path>" and takes other path relocation
>  	variables such as $GIT_OBJECT_DIRECTORY,
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index cf8487b..ccc0328 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -518,6 +518,21 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>  	if (argc > 1 && !strcmp("--sq-quote", argv[1]))
>  		return cmd_sq_quote(argc - 2, argv + 2);
>  
> +	if (argc == 2 && !strcmp("--local-env-vars", argv[1])) {

Maybe:

	if (argc > 1 && !strcmp("--local-env-vars", argv[1])) {
		if (argc != 2)
			die("--local-env-vars must be the only argument");

since the behaviour of:

	$ git rev-parse --local-env-vars --
	--local-env-vars
	--

is quite surprising.

> +		int i;
> +		for (i = 0; local_repo_env[i]; i++)
> +			printf("%s\n", local_repo_env[i]);
> +		return 0;
> +	}
> +
> +	if (argc > 2 && !strcmp(argv[1], "--resolve-git-dir")) {

This is less bad, but again it might be nice to provide a better error
if the path argument isn't supplied.

> +		const char *gitdir = resolve_gitdir(argv[2]);
> +		if (!gitdir)
> +			die("not a gitdir '%s'", argv[2]);
> +		puts(gitdir);
> +		return 0;
> +	}
> +
>  	if (argc > 1 && !strcmp("-h", argv[1]))
>  		usage(builtin_rev_parse_usage);
>  
> @@ -706,12 +721,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>  				add_ref_exclusion(&ref_excludes, arg + 10);
>  				continue;
>  			}
> -			if (!strcmp(arg, "--local-env-vars")) {

What about leaving this in and replacing the body of the if statement
with:

	die("--local-env-vars must be the first argument");

?  I expect this will significantly reduce debugging time if anyone is
relying on the current behaviour.

> -				int i;
> -				for (i = 0; local_repo_env[i]; i++)
> -					printf("%s\n", local_repo_env[i]);
> -				continue;
> -			}
>  			if (!strcmp(arg, "--show-toplevel")) {
>  				const char *work_tree = get_git_work_tree();
>  				if (work_tree)
> @@ -767,16 +776,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>  				puts(prefix_filename(pfx, strlen(pfx), get_git_common_dir()));
>  				continue;
>  			}
> -			if (!strcmp(arg, "--resolve-git-dir")) {
> -				const char *gitdir = argv[++i];
> -				if (!gitdir)
> -					die("--resolve-git-dir requires an argument");
> -				gitdir = resolve_gitdir(gitdir);
> -				if (!gitdir)
> -					die("not a gitdir '%s'", argv[i]);
> -				puts(gitdir);
> -				continue;
> -			}
>  			if (!strcmp(arg, "--is-inside-git-dir")) {
>  				printf("%s\n", is_inside_git_dir() ? "true"
>  						: "false");
> diff --git a/t/t1515-rev-parse-outside-repo.sh b/t/t1515-rev-parse-outside-repo.sh
> index ae33093..3ec2971 100755
> --- a/t/t1515-rev-parse-outside-repo.sh
> +++ b/t/t1515-rev-parse-outside-repo.sh
> @@ -27,14 +27,14 @@ test_expect_success 'rev-parse --sq-quote' '
>  	test_cmp expect actual
>  '
>  
> -test_expect_failure 'rev-parse --local-env-vars' '
> +test_expect_success 'rev-parse --local-env-vars' '
>  	git rev-parse --local-env-vars >actual &&
>  	# we do not want to depend on the complete list here,
>  	# so just look for something plausible
>  	grep ^GIT_DIR actual
>  '
>  
> -test_expect_failure 'rev-parse --resolve-git-dir' '
> +test_expect_success 'rev-parse --resolve-git-dir' '
>  	git init --separate-git-dir repo dir &&
>  	test_must_fail git rev-parse --resolve-git-dir . &&
>  	echo "$(pwd)/repo" >expect &&
> -- 
> 2.7.2.767.g705917e
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeff King]

On Sat, Feb 27, 2016 at 12:25:11PM +0000, John Keeping wrote:

> On Fri, Feb 26, 2016 at 06:29:57PM -0500, Jeff King wrote:
> > This reverts commit 68889b416d5b6a5cf7d280a428281d635fe9b292.
> [snip]
> > 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. :)
> 
> The aim of the original series was to improve the documentation, so I
> don't think it's unreasonable to consider this a regression and revert
> the functional change.  Although I think we can improve the behaviour
> slightly (see below).

Thanks for looking this over. I agree that the changes you suggested
would be an improvement over what I posted. But I tried out the
alternate plan to handle the repo-setup more gracefully inside the loop.
I think that looks much simpler (at the very least, I had to spend a lot
fewer words trying to justify it in the commit message!).

And that makes most of your suggestions obsolete. I'll go through
them...

> > +--local-env-vars::
> > +	List the GIT_* environment variables that are local to the
> > +	repository (e.g. GIT_DIR or GIT_WORK_TREE, but not GIT_EDITOR).
> > +	Only the names of the variables are listed, not their value,
> > +	even if they are set.
> 
> I think we should add:
> 
> 	No other arguments may be supplied.

So now you can give other arguments. I doubt it's a _good idea_ to do
so, but you could certainly do:

  git rev-parse --git-dir --local-env-vars

if you wanted to. You can even do the opposite, and I guess it would be
correct as long as you popped the final line off the end as --git-dir.

So I guess we could restrict it, but I don't think it's an issue in
practice, and it does technically work.

> > +--resolve-git-dir <path>::
> > +	Check if <path> is a valid repository or a gitfile that
> > +	points at a valid repository, and print the location of the
> > +	repository.  If <path> is a gitfile then the resolved path
> > +	to the real repository is printed.
> 
> Again I think this should say that only the `path` argument may be
> supplied.

This one I think is more reasonable to use along with other options. Or
you could even pass a sequence of:

  git rev-parse --resolve-git-dir foo --resolve-git-dir bar

Again, I doubt it's very useful in practice, but it does the obvious
thing (whereas with my original patch, it silently ignored subsequent
options).

> > +	if (argc == 2 && !strcmp("--local-env-vars", argv[1])) {
> 
> Maybe:
> 
> 	if (argc > 1 && !strcmp("--local-env-vars", argv[1])) {
> 		if (argc != 2)
> 			die("--local-env-vars must be the only argument");
> 
> since the behaviour of:
> 
> 	$ git rev-parse --local-env-vars --
> 	--local-env-vars
> 	--
> 
> is quite surprising.

This now does what you'd expect (it's probably not _useful_, but at
least it isn't horrifying and surprising, like the v1 behavior).

> > +	if (argc > 2 && !strcmp(argv[1], "--resolve-git-dir")) {
> 
> This is less bad, but again it might be nice to provide a better error
> if the path argument isn't supplied.

This one is OK now, too.

> > @@ -706,12 +721,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> >  				add_ref_exclusion(&ref_excludes, arg + 10);
> >  				continue;
> >  			}
> > -			if (!strcmp(arg, "--local-env-vars")) {
> 
> What about leaving this in and replacing the body of the if statement
> with:
> 
> 	die("--local-env-vars must be the first argument");
> 
> ?  I expect this will significantly reduce debugging time if anyone is
> relying on the current behaviour.

The new version I sent covers this, too (and it does the right thing).


Thanks for a careful review of the corner cases. Even though my response
to all of them is "yeah, but your suggestion is now obsolete", it makes
me feel better about the v2 patch to see that it magically clears up all
of these issues.

-Peff

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

[Eric Sunshine]

On Fri, Feb 26, 2016 at 6:29 PM, Jeff King <peff@peff.net> 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@peff.net>

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

[Jeff King]

On Sat, Feb 27, 2016 at 07:53:02PM -0500, Eric Sunshine wrote:

> > then it must receive the two lines of output in the correct
> 
> s/correct/& order/

I fixed this and all of the other typos by switching to a patch that
needs about one tenth as much explanation. :)

I'm sure it's not possible that I introduced any new ones...

-Peff