Re: index-pack outside of repository?

Jeff King [off-list ref] writes:

In older versions of git will just blindly write into
".git/objects/pack", even though there's no repository there.

So I think complaining to the user is the right thing to do here. I
started to write a patch to have index-pack notice when it needs a repo
and doesn't have one, but the logic is actually a bit unclear.  Do we
need to complain early _just_ when --stdin is specified, or does that
miss somes cases?  Likewise, are there cases where --stdin can operate
without a repo? I couldn't think of any.

I think there are two and only two major modes; --stdin wants to put
the result in the repository it is working on, while the other mode
takes a filename to deposit the result in, so the latter does not
technically need a repository.

I'm actually wondering if the way it calls die() in 'next' is a pretty
reasonable way for things to work in general. It happens when we lazily
try to ask for the repository directory. So we don't have to replicate
logic to say "are we going to need a repo"; at the moment we need it, we
notice we don't have it and die. The only problem is that it says "BUG"
and not "this operation must be run in a git repository".

Isn't what we currently have is a good way to discover which
codepaths we missed to add a check to issue the latter error?

That strategy _might_ be a problem for some programs, which would want
to notice the issue early before doing work. But it seems like a
reasonable outcome for index-pack. Thoughts?

That is, once we know which codepaths should require a repository, I
think it is reasonable to add a check that is done earlier than the
place where we currently try to see where we have one (which could
be deep in the callchain).  But we are all human and can miss things,
so the BUG() thing is probably fine.  We are cooking it exactly because
we would want to find such corner cases we missed, no?

On Thu, Dec 15, 2016 at 04:13:38PM -0800, Junio C Hamano wrote:

quoted

So I think complaining to the user is the right thing to do here. I
started to write a patch to have index-pack notice when it needs a repo
and doesn't have one, but the logic is actually a bit unclear.  Do we
need to complain early _just_ when --stdin is specified, or does that
miss somes cases?  Likewise, are there cases where --stdin can operate
without a repo? I couldn't think of any.

I think there are two and only two major modes; --stdin wants to put
the result in the repository it is working on, while the other mode
takes a filename to deposit the result in, so the latter does not
technically need a repository.

OK. That's easy to check for, then.  Reverse-engineering that logic from
the actual calls in index-pack.c:final() is complicated.  But certainly
basing it on --stdin is what I would have expected.

quoted

That strategy _might_ be a problem for some programs, which would want
to notice the issue early before doing work. But it seems like a
reasonable outcome for index-pack. Thoughts?

That is, once we know which codepaths should require a repository, I
think it is reasonable to add a check that is done earlier than the
place where we currently try to see where we have one (which could
be deep in the callchain).  But we are all human and can miss things,
so the BUG() thing is probably fine.  We are cooking it exactly because
we would want to find such corner cases we missed, no?

Right, that was my original intent in adding the BUG(): to catch
unhandled cases, and then do the appropriate thing earlier. I was just
questioning whether the appropriate thing in some cases might be dying
at the BUG(), just with a more friendly message. That has the benefit of
being very easy to implement, and never wrong (e.g., forbidding a case
that actually _doesn't_ need to look at the repo).

But if this case really is just "if (from_stdin)" that's quite easy,
too.

-Peff

On Thu, Dec 15, 2016 at 08:37:28PM -0500, Jeff King wrote:

But if this case really is just "if (from_stdin)" that's quite easy,
too.

So here is that patch (with some associated refactoring and cleanups).
This is conceptually independent of jk/no-looking-at-dotgit-outside-repo-final,
though it should be fine to merge with that topic. The BUG will actually
pass the new test, because it calls die, too. I wonder if we should die
with a unique error code on BUGs, and catch them in test_must_fail
similar to the way we catch signal death.

  [1/3]: t5000: extract nongit function to test-lib-functions.sh
  [2/3]: index-pack: complain when --stdin is used outside of a repo
  [3/3]: t: use nongit() function where applicable

 builtin/index-pack.c     |  2 ++
 t/t1308-config-set.sh    | 10 ++--------
 t/t5000-tar-tree.sh      | 14 --------------
 t/t5300-pack-object.sh   | 15 +++++++++++++++
 t/t9100-git-svn-basic.sh | 17 ++---------------
 t/t9902-completion.sh    |  7 +------
 t/test-lib-functions.sh  | 14 ++++++++++++++
 7 files changed, 36 insertions(+), 43 deletions(-)

-Peff

This function abstracts the idea of running a command
outside of any repository (which is slightly awkward to do
because even if you make a non-repo directory, git may keep
walking up outside of the trash directory). There are
several scripts that use the same technique, so let's make
the function available for everyone.

Signed-off-by: Jeff King <redacted>
---
I waffled on the name. Something like test_outside_repo() is more
descriptive, but as this is prepended to existing commands, the lines
already end up quite long.

 t/t5000-tar-tree.sh     | 14 --------------
 t/test-lib-functions.sh | 14 ++++++++++++++
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 830bf2a2f6..886b6953e4 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh

@@ -94,20 +94,6 @@ check_tar() {
 	'
 }
 
-# run "$@" inside a non-git directory
-nongit () {
-	test -d non-repo ||
-	mkdir non-repo ||
-	return 1
-
-	(
-		GIT_CEILING_DIRECTORIES=$(pwd) &&
-		export GIT_CEILING_DIRECTORIES &&
-		cd non-repo &&
-		"$@"
-	)
-}
-
 test_expect_success \
     'populate workdir' \
     'mkdir a &&

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index fdaeb3a96b..adab7f51f4 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh

@@ -994,3 +994,17 @@ test_copy_bytes () {
 		}
 	' - "$1"
 }
+
+# run "$@" inside a non-git directory
+nongit () {
+	test -d non-repo ||
+	mkdir non-repo ||
+	return 1
+
+	(
+		GIT_CEILING_DIRECTORIES=$(pwd) &&
+		export GIT_CEILING_DIRECTORIES &&
+		cd non-repo &&
+		"$@"
+	)
+}

-- 
2.11.0.348.g960a0b554

The index-pack builtin is marked as RUN_SETUP_GENTLY,
because it's perfectly fine to index a pack in the
filesystem outside of any repository. However, --stdin mode
will write the result to the object database, which does not
make sense outside of a repository. Doing so creates a bogus
".git" directory with nothing in it except the newly-created
pack and its index.

Instead, let's flag this as an error and abort.

Signed-off-by: Jeff King <redacted>
---
 builtin/index-pack.c   |  2 ++
 t/t5300-pack-object.sh | 15 +++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 0a27bab11b..d450a6ada2 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c

@@ -1730,6 +1730,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 		usage(index_pack_usage);
 	if (fix_thin_pack && !from_stdin)
 		die(_("--fix-thin cannot be used without --stdin"));
+	if (from_stdin && !startup_info->have_repository)
+		die(_("--stdin requires a git repository"));
 	if (!index_name && pack_name)
 		index_name = derive_filename(pack_name, ".idx", &index_name_buf);
 	if (keep_msg && !keep_name && pack_name)

diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 899e52d50f..43a672c345 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh

@@ -406,6 +406,21 @@ test_expect_success 'verify resulting packs' '
 	git verify-pack test-11-*.pack
 '
 
+test_expect_success 'set up pack for non-repo tests' '
+	# make sure we have a pack with no matching index file
+	cp test-1-*.pack foo.pack
+'
+
+test_expect_success 'index-pack --stdin complains of non-repo' '
+	nongit test_must_fail git index-pack --stdin <foo.pack &&
+	test_path_is_missing non-repo/.git
+'
+
+test_expect_success 'index-pack <pack> works in non-repo' '
+	nongit git index-pack ../foo.pack &&
+	test_path_is_file foo.idx
+'
+
 #
 # WARNING!
 #

-- 
2.11.0.348.g960a0b554

Many tests want to run a command outside of any git repo;
with the nongit() function this is now a one-liner. It saves
a few lines, but more importantly, it's immediately obvious
what the code is trying to accomplish.

This doesn't convert every such case in the test suite; it
just covers those that want to do a one-off command. Other
cases, such as the ones in t4035, are part of a larger
scheme of outside-repo files, and it's less confusing for
them to stay consistent with the surrounding tests.

Signed-off-by: Jeff King <redacted>
---
This one is obviously not necessary for the rest of the series, but the
diffstat is certainly pleasing.

 t/t1308-config-set.sh    | 10 ++--------
 t/t9100-git-svn-basic.sh | 17 ++---------------
 t/t9902-completion.sh    |  7 +------
 3 files changed, 5 insertions(+), 29 deletions(-)

diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 7655c94c28..ff50960cca 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh

@@ -219,14 +219,8 @@ test_expect_success 'check line errors for malformed values' '
 '
 
 test_expect_success 'error on modifying repo config without repo' '
-	mkdir no-repo &&
-	(
-		GIT_CEILING_DIRECTORIES=$(pwd) &&
-		export GIT_CEILING_DIRECTORIES &&
-		cd no-repo &&
-		test_must_fail git config a.b c 2>err &&
-		grep "not in a git directory" err
-	)
+	nongit test_must_fail git config a.b c 2>err &&
+	grep "not in a git directory" err
 '
 
 cmdline_config="'foo.bar=from-cmdline'"

diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh
index 92a3aa8063..8a8ba65a2a 100755
--- a/t/t9100-git-svn-basic.sh
+++ b/t/t9100-git-svn-basic.sh

@@ -17,25 +17,12 @@ case "$GIT_SVN_LC_ALL" in
 	;;
 esac
 
-deepdir=nothing-above
-ceiling=$PWD
-
 test_expect_success 'git svn --version works anywhere' '
-	mkdir -p "$deepdir" && (
-		GIT_CEILING_DIRECTORIES="$ceiling" &&
-		export GIT_CEILING_DIRECTORIES &&
-		cd "$deepdir" &&
-		git svn --version
-	)
+	nongit git svn --version
 '
 
 test_expect_success 'git svn help works anywhere' '
-	mkdir -p "$deepdir" && (
-		GIT_CEILING_DIRECTORIES="$ceiling" &&
-		export GIT_CEILING_DIRECTORIES &&
-		cd "$deepdir" &&
-		git svn help
-	)
+	nongit git svn help
 '
 
 test_expect_success \

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 2ba62fbc17..a34e55f874 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh

@@ -257,12 +257,7 @@ test_expect_success SYMLINKS '__gitdir - resulting path avoids symlinks' '
 '
 
 test_expect_success '__gitdir - not a git repository' '
-	(
-		cd subdir/subsubdir &&
-		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY" &&
-		export GIT_CEILING_DIRECTORIES &&
-		test_must_fail __gitdir
-	)
+	nongit test_must_fail __gitdir
 '
 
 test_expect_success '__gitcomp - trailing space - options' '

-- 
2.11.0.348.g960a0b554