* [PATCH 1/3] t5000: extract nongit function to test-lib-functions.sh
2016-12-16 2:29 ` Jeff King
@ 2016-12-16 2:30 ` Jeff King
2016-12-16 2:30 ` [PATCH 2/3] index-pack: complain when --stdin is used outside of a repo Jeff King
` (2 subsequent siblings)
3 siblings, 0 replies; 14+ messages in thread
From: Jeff King @ 2016-12-16 2:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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 <peff@peff.net>
---
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/3] index-pack: complain when --stdin is used outside of a repo
2016-12-16 2:29 ` Jeff King
2016-12-16 2:30 ` [PATCH 1/3] t5000: extract nongit function to test-lib-functions.sh Jeff King
@ 2016-12-16 2:30 ` Jeff King
2016-12-16 2:31 ` [PATCH 3/3] t: use nongit() function where applicable Jeff King
2016-12-16 17:52 ` index-pack outside of repository? Junio C Hamano
3 siblings, 0 replies; 14+ messages in thread
From: Jeff King @ 2016-12-16 2:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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 <peff@peff.net>
---
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/3] t: use nongit() function where applicable
2016-12-16 2:29 ` Jeff King
2016-12-16 2:30 ` [PATCH 1/3] t5000: extract nongit function to test-lib-functions.sh Jeff King
2016-12-16 2:30 ` [PATCH 2/3] index-pack: complain when --stdin is used outside of a repo Jeff King
@ 2016-12-16 2:31 ` Jeff King
2016-12-16 17:52 ` index-pack outside of repository? Junio C Hamano
3 siblings, 0 replies; 14+ messages in thread
From: Jeff King @ 2016-12-16 2:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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 <peff@peff.net>
---
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: index-pack outside of repository?
2016-12-16 2:29 ` Jeff King
` (2 preceding siblings ...)
2016-12-16 2:31 ` [PATCH 3/3] t: use nongit() function where applicable Jeff King
@ 2016-12-16 17:52 ` Junio C Hamano
2016-12-16 17:59 ` Junio C Hamano
3 siblings, 1 reply; 14+ messages in thread
From: Junio C Hamano @ 2016-12-16 17:52 UTC (permalink / raw)
To: Jeff King; +Cc: git
Jeff King <peff@peff.net> writes:
> 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
I think 2/3 is a good change to ensure we get a reasonable error for
"index-pack --stdin", and 3/3 is a very good cleanup. Both of them
of course are enabled by 1/3.
We still fail "nongit git index-pack tmp.pack" with a BUG: though.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: index-pack outside of repository?
2016-12-16 17:52 ` index-pack outside of repository? Junio C Hamano
@ 2016-12-16 17:59 ` Junio C Hamano
2016-12-16 18:01 ` Junio C Hamano
0 siblings, 1 reply; 14+ messages in thread
From: Junio C Hamano @ 2016-12-16 17:59 UTC (permalink / raw)
To: Jeff King; +Cc: git
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
>
>> 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
>
> I think 2/3 is a good change to ensure we get a reasonable error for
> "index-pack --stdin", and 3/3 is a very good cleanup. Both of them
> of course are enabled by 1/3.
>
> We still fail "nongit git index-pack tmp.pack" with a BUG: though.
Wait.
This only happens with a stalled-and-to-be-discarded topic on 'pu'.
Please don't waste time digging it (yet).
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: index-pack outside of repository?
2016-12-16 17:59 ` Junio C Hamano
@ 2016-12-16 18:01 ` Junio C Hamano
2016-12-16 21:43 ` Jeff King
0 siblings, 1 reply; 14+ messages in thread
From: Junio C Hamano @ 2016-12-16 18:01 UTC (permalink / raw)
To: Jeff King; +Cc: git
Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Jeff King <peff@peff.net> writes:
>>
>>> 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
>>
>> I think 2/3 is a good change to ensure we get a reasonable error for
>> "index-pack --stdin", and 3/3 is a very good cleanup. Both of them
>> of course are enabled by 1/3.
>>
>> We still fail "nongit git index-pack tmp.pack" with a BUG: though.
>
> Wait.
>
> This only happens with a stalled-and-to-be-discarded topic on 'pu'.
> Please don't waste time digging it (yet).
Don't wait ;-). My mistake. We can see t5300 broken with this
change and b1ef400eec ("setup_git_env: avoid blind fall-back to
".git"", 2016-10-20) without anything else. We still need to
address it.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: index-pack outside of repository?
2016-12-16 18:01 ` Junio C Hamano
@ 2016-12-16 21:43 ` Jeff King
2016-12-16 21:55 ` Junio C Hamano
0 siblings, 1 reply; 14+ messages in thread
From: Jeff King @ 2016-12-16 21:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Fri, Dec 16, 2016 at 10:01:49AM -0800, Junio C Hamano wrote:
> >> I think 2/3 is a good change to ensure we get a reasonable error for
> >> "index-pack --stdin", and 3/3 is a very good cleanup. Both of them
> >> of course are enabled by 1/3.
> >>
> >> We still fail "nongit git index-pack tmp.pack" with a BUG: though.
> >
> > Wait.
> >
> > This only happens with a stalled-and-to-be-discarded topic on 'pu'.
> > Please don't waste time digging it (yet).
>
> Don't wait ;-). My mistake. We can see t5300 broken with this
> change and b1ef400eec ("setup_git_env: avoid blind fall-back to
> ".git"", 2016-10-20) without anything else. We still need to
> address it.
Ah, I only checked that it did not do anything terrible like write into
".git". But it looks like it still looks at the git_dir value as part of
the collision check.
Here's a patch, on top of the rest of the series.
-- >8 --
Subject: [PATCH] index-pack: skip collision check when not in repository
You can run "git index-pack path/to/foo.pack" outside of a
repository to generate an index file, or just to verify the
contents. There's no point in doing a collision check, since
we obviously do not have any objects to collide with.
The current code will blindly look in .git/objects based on
the result of setup_git_env(). That effectively gives us the
right answer (since we won't find any objects), but it's a
waste of time, and it conflicts with our desire to
eventually get rid of the "fallback to .git" behavior of
setup_git_env().
Signed-off-by: Jeff King <peff@peff.net>
---
I didn't do a test, as this doesn't really have any user-visible
behavior change. I guess technically if you had a non-repo with
".git/objects/12/3456..." in it we would erroneously read it, but that's
kind of bizarre. The interesting test is that when merged with
jk/no-looking-at-dotgit-outside-repo-final, the test in t5300 doesn't
die.
builtin/index-pack.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index d450a6ada2..f4b87c6c9f 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -787,13 +787,15 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,
const unsigned char *sha1)
{
void *new_data = NULL;
- int collision_test_needed;
+ int collision_test_needed = 0;
assert(data || obj_entry);
- read_lock();
- collision_test_needed = has_sha1_file_with_flags(sha1, HAS_SHA1_QUICK);
- read_unlock();
+ if (startup_info->have_repository) {
+ read_lock();
+ collision_test_needed = has_sha1_file_with_flags(sha1, HAS_SHA1_QUICK);
+ read_unlock();
+ }
if (collision_test_needed && !data) {
read_lock();
--
2.11.0.348.g960a0b554
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: index-pack outside of repository?
2016-12-16 21:43 ` Jeff King
@ 2016-12-16 21:55 ` Junio C Hamano
0 siblings, 0 replies; 14+ messages in thread
From: Junio C Hamano @ 2016-12-16 21:55 UTC (permalink / raw)
To: Jeff King; +Cc: git
Jeff King <peff@peff.net> writes:
> Ah, I only checked that it did not do anything terrible like write into
> ".git". But it looks like it still looks at the git_dir value as part of
> the collision check.
>
> Here's a patch, on top of the rest of the series.
Thanks for a quick turnaround, as always.
> -- >8 --
> Subject: [PATCH] index-pack: skip collision check when not in repository
>
> You can run "git index-pack path/to/foo.pack" outside of a
> repository to generate an index file, or just to verify the
> contents. There's no point in doing a collision check, since
> we obviously do not have any objects to collide with.
>
> The current code will blindly look in .git/objects based on
> the result of setup_git_env(). That effectively gives us the
> right answer (since we won't find any objects), but it's a
> waste of time, and it conflicts with our desire to
> eventually get rid of the "fallback to .git" behavior of
> setup_git_env().
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I didn't do a test, as this doesn't really have any user-visible
> behavior change.
>
> I guess technically if you had a non-repo with
> ".git/objects/12/3456..." in it we would erroneously read it, but that's
> kind of bizarre. The interesting test is that when merged with
> jk/no-looking-at-dotgit-outside-repo-final, the test in t5300 doesn't
> die.
Yes.
>
> builtin/index-pack.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index d450a6ada2..f4b87c6c9f 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -787,13 +787,15 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,
> const unsigned char *sha1)
> {
> void *new_data = NULL;
> - int collision_test_needed;
> + int collision_test_needed = 0;
>
> assert(data || obj_entry);
>
> - read_lock();
> - collision_test_needed = has_sha1_file_with_flags(sha1, HAS_SHA1_QUICK);
> - read_unlock();
> + if (startup_info->have_repository) {
> + read_lock();
> + collision_test_needed = has_sha1_file_with_flags(sha1, HAS_SHA1_QUICK);
> + read_unlock();
> + }
>
> if (collision_test_needed && !data) {
> read_lock();
^ permalink raw reply [flat|nested] 14+ messages in thread