{"thread":{"id":"44714","subject":"index-pack outside of repository?","startedAt":"2016-12-15T20:40:41Z","lastAt":"2016-12-16T21:55:44Z","messageCount":14,"participants":["Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"307864","messageId":"20161215204000.avlcfaqjwstkptu2@sigill.intra.peff.net","threadId":"44714","inReplyTo":null,"subject":"index-pack outside of repository?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-15T20:40:00Z","receivedAt":"2016-12-15T20:40:41Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Running git on 'next', you can trigger a BUG:\n\n  $ cd /some/repo\n  $ git pack-objects --all --stdout </dev/null >/tmp/foo.pack\n  $ cd /not-a-git-repo\n  $ git index-pack --stdin </tmp/foo.pack\n  fatal: BUG: setup_git_env called without repository\n\nThis obviously comes from my b1ef400eec (setup_git_env: avoid blind\nfall-back to \".git\", 2016-10-20). What's going on is that index-pack\nuses RUN_SETUP_GENTLY, but never actually handles the out-of-repo case.\nWhen we use the internal git_dir to make \"objects/pack/pack-xxx.pack\",\nit barfs.\n\nIn older versions of git will just blindly write into\n\".git/objects/pack\", even though there's no repository there.\n\nSo I think complaining to the user is the right thing to do here. I\nstarted to write a patch to have index-pack notice when it needs a repo\nand doesn't have one, but the logic is actually a bit unclear.  Do we\nneed to complain early _just_ when --stdin is specified, or does that\nmiss somes cases?  Likewise, are there cases where --stdin can operate\nwithout a repo? I couldn't think of any.\n\nI'm actually wondering if the way it calls die() in 'next' is a pretty\nreasonable way for things to work in general. It happens when we lazily\ntry to ask for the repository directory. So we don't have to replicate\nlogic to say \"are we going to need a repo\"; at the moment we need it, we\nnotice we don't have it and die. The only problem is that it says \"BUG\"\nand not \"this operation must be run in a git repository\".\n\nThat strategy _might_ be a problem for some programs, which would want\nto notice the issue early before doing work. But it seems like a\nreasonable outcome for index-pack. Thoughts?\n\n-Peff\n"},{"id":"307870","messageId":"xmqqshpou3wt.fsf@gitster.mtv.corp.google.com","threadId":"44714","inReplyTo":"20161215204000.avlcfaqjwstkptu2@sigill.intra.peff.net","subject":"Re: index-pack outside of repository?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-16T00:13:38Z","receivedAt":"2016-12-16T00:13:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> In older versions of git will just blindly write into\n> \".git/objects/pack\", even though there's no repository there.\n>\n> So I think complaining to the user is the right thing to do here. I\n> started to write a patch to have index-pack notice when it needs a repo\n> and doesn't have one, but the logic is actually a bit unclear.  Do we\n> need to complain early _just_ when --stdin is specified, or does that\n> miss somes cases?  Likewise, are there cases where --stdin can operate\n> without a repo? I couldn't think of any.\n\nI think there are two and only two major modes; --stdin wants to put\nthe result in the repository it is working on, while the other mode\ntakes a filename to deposit the result in, so the latter does not\ntechnically need a repository.\n\n> I'm actually wondering if the way it calls die() in 'next' is a pretty\n> reasonable way for things to work in general. It happens when we lazily\n> try to ask for the repository directory. So we don't have to replicate\n> logic to say \"are we going to need a repo\"; at the moment we need it, we\n> notice we don't have it and die. The only problem is that it says \"BUG\"\n> and not \"this operation must be run in a git repository\".\n\nIsn't what we currently have is a good way to discover which\ncodepaths we missed to add a check to issue the latter error?\n\n> That strategy _might_ be a problem for some programs, which would want\n> to notice the issue early before doing work. But it seems like a\n> reasonable outcome for index-pack. Thoughts?\n\nThat is, once we know which codepaths should require a repository, I\nthink it is reasonable to add a check that is done earlier than the\nplace where we currently try to see where we have one (which could\nbe deep in the callchain).  But we are all human and can miss things,\nso the BUG() thing is probably fine.  We are cooking it exactly because\nwe would want to find such corner cases we missed, no?\n"},{"id":"307872","messageId":"20161216013728.in2dazshtarrnnq3@sigill.intra.peff.net","threadId":"44714","inReplyTo":"xmqqshpou3wt.fsf@gitster.mtv.corp.google.com","subject":"Re: index-pack outside of repository?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-16T01:37:28Z","receivedAt":"2016-12-16T01:38:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 15, 2016 at 04:13:38PM -0800, Junio C Hamano wrote:\n\n> > So I think complaining to the user is the right thing to do here. I\n> > started to write a patch to have index-pack notice when it needs a repo\n> > and doesn't have one, but the logic is actually a bit unclear.  Do we\n> > need to complain early _just_ when --stdin is specified, or does that\n> > miss somes cases?  Likewise, are there cases where --stdin can operate\n> > without a repo? I couldn't think of any.\n> \n> I think there are two and only two major modes; --stdin wants to put\n> the result in the repository it is working on, while the other mode\n> takes a filename to deposit the result in, so the latter does not\n> technically need a repository.\n\nOK. That's easy to check for, then.  Reverse-engineering that logic from\nthe actual calls in index-pack.c:final() is complicated.  But certainly\nbasing it on --stdin is what I would have expected.\n\n> > That strategy _might_ be a problem for some programs, which would want\n> > to notice the issue early before doing work. But it seems like a\n> > reasonable outcome for index-pack. Thoughts?\n> \n> That is, once we know which codepaths should require a repository, I\n> think it is reasonable to add a check that is done earlier than the\n> place where we currently try to see where we have one (which could\n> be deep in the callchain).  But we are all human and can miss things,\n> so the BUG() thing is probably fine.  We are cooking it exactly because\n> we would want to find such corner cases we missed, no?\n\nRight, that was my original intent in adding the BUG(): to catch\nunhandled cases, and then do the appropriate thing earlier. I was just\nquestioning whether the appropriate thing in some cases might be dying\nat the BUG(), just with a more friendly message. That has the benefit of\nbeing very easy to implement, and never wrong (e.g., forbidding a case\nthat actually _doesn't_ need to look at the repo).\n\nBut if this case really is just \"if (from_stdin)\" that's quite easy,\ntoo.\n\n-Peff\n"},{"id":"307877","messageId":"20161216022904.cjang6napnl2vkc6@sigill.intra.peff.net","threadId":"44714","inReplyTo":"20161216013728.in2dazshtarrnnq3@sigill.intra.peff.net","subject":"Re: index-pack outside of repository?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-16T02:29:04Z","receivedAt":"2016-12-16T02:29:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 15, 2016 at 08:37:28PM -0500, Jeff King wrote:\n\n> But if this case really is just \"if (from_stdin)\" that's quite easy,\n> too.\n\nSo here is that patch (with some associated refactoring and cleanups).\nThis is conceptually independent of jk/no-looking-at-dotgit-outside-repo-final,\nthough it should be fine to merge with that topic. The BUG will actually\npass the new test, because it calls die, too. I wonder if we should die\nwith a unique error code on BUGs, and catch them in test_must_fail\nsimilar to the way we catch signal death.\n\n  [1/3]: t5000: extract nongit function to test-lib-functions.sh\n  [2/3]: index-pack: complain when --stdin is used outside of a repo\n  [3/3]: t: use nongit() function where applicable\n\n builtin/index-pack.c     |  2 ++\n t/t1308-config-set.sh    | 10 ++--------\n t/t5000-tar-tree.sh      | 14 --------------\n t/t5300-pack-object.sh   | 15 +++++++++++++++\n t/t9100-git-svn-basic.sh | 17 ++---------------\n t/t9902-completion.sh    |  7 +------\n t/test-lib-functions.sh  | 14 ++++++++++++++\n 7 files changed, 36 insertions(+), 43 deletions(-)\n\n-Peff\n"},{"id":"307878","messageId":"20161216023011.7k4wyekid3zitl56@sigill.intra.peff.net","threadId":"44714","inReplyTo":"20161216022904.cjang6napnl2vkc6@sigill.intra.peff.net","subject":"[PATCH 1/3] t5000: extract nongit function to test-lib-functions.sh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-16T02:30:12Z","receivedAt":"2016-12-16T02:30:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function abstracts the idea of running a command\noutside of any repository (which is slightly awkward to do\nbecause even if you make a non-repo directory, git may keep\nwalking up outside of the trash directory). There are\nseveral scripts that use the same technique, so let's make\nthe function available for everyone.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI waffled on the name. Something like test_outside_repo() is more\ndescriptive, but as this is prepended to existing commands, the lines\nalready end up quite long.\n\n t/t5000-tar-tree.sh     | 14 --------------\n t/test-lib-functions.sh | 14 ++++++++++++++\n 2 files changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 830bf2a2f6..886b6953e4 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -94,20 +94,6 @@ check_tar() {\n \t'\n }\n \n-# run \"$@\" inside a non-git directory\n-nongit () {\n-\ttest -d non-repo ||\n-\tmkdir non-repo ||\n-\treturn 1\n-\n-\t(\n-\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n-\t\texport GIT_CEILING_DIRECTORIES &&\n-\t\tcd non-repo &&\n-\t\t\"$@\"\n-\t)\n-}\n-\n test_expect_success \\\n     'populate workdir' \\\n     'mkdir a &&\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex fdaeb3a96b..adab7f51f4 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -994,3 +994,17 @@ test_copy_bytes () {\n \t\t}\n \t' - \"$1\"\n }\n+\n+# run \"$@\" inside a non-git directory\n+nongit () {\n+\ttest -d non-repo ||\n+\tmkdir non-repo ||\n+\treturn 1\n+\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd non-repo &&\n+\t\t\"$@\"\n+\t)\n+}\n-- \n2.11.0.348.g960a0b554\n\n"},{"id":"307879","messageId":"20161216023058.a2ksbkhb2lwkiglf@sigill.intra.peff.net","threadId":"44714","inReplyTo":"20161216022904.cjang6napnl2vkc6@sigill.intra.peff.net","subject":"[PATCH 2/3] index-pack: complain when --stdin is used outside of a repo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-16T02:30:59Z","receivedAt":"2016-12-16T02:31:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The index-pack builtin is marked as RUN_SETUP_GENTLY,\nbecause it's perfectly fine to index a pack in the\nfilesystem outside of any repository. However, --stdin mode\nwill write the result to the object database, which does not\nmake sense outside of a repository. Doing so creates a bogus\n\".git\" directory with nothing in it except the newly-created\npack and its index.\n\nInstead, let's flag this as an error and abort.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/index-pack.c   |  2 ++\n t/t5300-pack-object.sh | 15 +++++++++++++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 0a27bab11b..d450a6ada2 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1730,6 +1730,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\tusage(index_pack_usage);\n \tif (fix_thin_pack && !from_stdin)\n \t\tdie(_(\"--fix-thin cannot be used without --stdin\"));\n+\tif (from_stdin && !startup_info->have_repository)\n+\t\tdie(_(\"--stdin requires a git repository\"));\n \tif (!index_name && pack_name)\n \t\tindex_name = derive_filename(pack_name, \".idx\", &index_name_buf);\n \tif (keep_msg && !keep_name && pack_name)\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 899e52d50f..43a672c345 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -406,6 +406,21 @@ test_expect_success 'verify resulting packs' '\n \tgit verify-pack test-11-*.pack\n '\n \n+test_expect_success 'set up pack for non-repo tests' '\n+\t# make sure we have a pack with no matching index file\n+\tcp test-1-*.pack foo.pack\n+'\n+\n+test_expect_success 'index-pack --stdin complains of non-repo' '\n+\tnongit test_must_fail git index-pack --stdin <foo.pack &&\n+\ttest_path_is_missing non-repo/.git\n+'\n+\n+test_expect_success 'index-pack <pack> works in non-repo' '\n+\tnongit git index-pack ../foo.pack &&\n+\ttest_path_is_file foo.idx\n+'\n+\n #\n # WARNING!\n #\n-- \n2.11.0.348.g960a0b554\n\n"},{"id":"307880","messageId":"20161216023159.pzimmmknyptgutnu@sigill.intra.peff.net","threadId":"44714","inReplyTo":"20161216022904.cjang6napnl2vkc6@sigill.intra.peff.net","subject":"[PATCH 3/3] t: use nongit() function where applicable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-16T02:31:59Z","receivedAt":"2016-12-16T02:38:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Many tests want to run a command outside of any git repo;\nwith the nongit() function this is now a one-liner. It saves\na few lines, but more importantly, it's immediately obvious\nwhat the code is trying to accomplish.\n\nThis doesn't convert every such case in the test suite; it\njust covers those that want to do a one-off command. Other\ncases, such as the ones in t4035, are part of a larger\nscheme of outside-repo files, and it's less confusing for\nthem to stay consistent with the surrounding tests.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis one is obviously not necessary for the rest of the series, but the\ndiffstat is certainly pleasing.\n\n t/t1308-config-set.sh    | 10 ++--------\n t/t9100-git-svn-basic.sh | 17 ++---------------\n t/t9902-completion.sh    |  7 +------\n 3 files changed, 5 insertions(+), 29 deletions(-)\n\ndiff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\nindex 7655c94c28..ff50960cca 100755\n--- a/t/t1308-config-set.sh\n+++ b/t/t1308-config-set.sh\n@@ -219,14 +219,8 @@ test_expect_success 'check line errors for malformed values' '\n '\n \n test_expect_success 'error on modifying repo config without repo' '\n-\tmkdir no-repo &&\n-\t(\n-\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n-\t\texport GIT_CEILING_DIRECTORIES &&\n-\t\tcd no-repo &&\n-\t\ttest_must_fail git config a.b c 2>err &&\n-\t\tgrep \"not in a git directory\" err\n-\t)\n+\tnongit test_must_fail git config a.b c 2>err &&\n+\tgrep \"not in a git directory\" err\n '\n \n cmdline_config=\"'foo.bar=from-cmdline'\"\ndiff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh\nindex 92a3aa8063..8a8ba65a2a 100755\n--- a/t/t9100-git-svn-basic.sh\n+++ b/t/t9100-git-svn-basic.sh\n@@ -17,25 +17,12 @@ case \"$GIT_SVN_LC_ALL\" in\n \t;;\n esac\n \n-deepdir=nothing-above\n-ceiling=$PWD\n-\n test_expect_success 'git svn --version works anywhere' '\n-\tmkdir -p \"$deepdir\" && (\n-\t\tGIT_CEILING_DIRECTORIES=\"$ceiling\" &&\n-\t\texport GIT_CEILING_DIRECTORIES &&\n-\t\tcd \"$deepdir\" &&\n-\t\tgit svn --version\n-\t)\n+\tnongit git svn --version\n '\n \n test_expect_success 'git svn help works anywhere' '\n-\tmkdir -p \"$deepdir\" && (\n-\t\tGIT_CEILING_DIRECTORIES=\"$ceiling\" &&\n-\t\texport GIT_CEILING_DIRECTORIES &&\n-\t\tcd \"$deepdir\" &&\n-\t\tgit svn help\n-\t)\n+\tnongit git svn help\n '\n \n test_expect_success \\\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 2ba62fbc17..a34e55f874 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -257,12 +257,7 @@ test_expect_success SYMLINKS '__gitdir - resulting path avoids symlinks' '\n '\n \n test_expect_success '__gitdir - not a git repository' '\n-\t(\n-\t\tcd subdir/subsubdir &&\n-\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY\" &&\n-\t\texport GIT_CEILING_DIRECTORIES &&\n-\t\ttest_must_fail __gitdir\n-\t)\n+\tnongit test_must_fail __gitdir\n '\n \n test_expect_success '__gitcomp - trailing space - options' '\n-- \n2.11.0.348.g960a0b554\n"},{"id":"307892","messageId":"xmqqd1gru5fw.fsf@gitster.mtv.corp.google.com","threadId":"44714","inReplyTo":"20161216022904.cjang6napnl2vkc6@sigill.intra.peff.net","subject":"Re: index-pack outside of repository?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-16T17:52:51Z","receivedAt":"2016-12-16T17:53:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Dec 15, 2016 at 08:37:28PM -0500, Jeff King wrote:\n>\n>> But if this case really is just \"if (from_stdin)\" that's quite easy,\n>> too.\n>\n> So here is that patch (with some associated refactoring and cleanups).\n> This is conceptually independent of jk/no-looking-at-dotgit-outside-repo-final,\n> though it should be fine to merge with that topic. The BUG will actually\n> pass the new test, because it calls die, too. I wonder if we should die\n> with a unique error code on BUGs, and catch them in test_must_fail\n> similar to the way we catch signal death.\n>\n>   [1/3]: t5000: extract nongit function to test-lib-functions.sh\n>   [2/3]: index-pack: complain when --stdin is used outside of a repo\n>   [3/3]: t: use nongit() function where applicable\n\nI think 2/3 is a good change to ensure we get a reasonable error for\n\"index-pack --stdin\", and 3/3 is a very good cleanup.  Both of them\nof course are enabled by 1/3.\n\nWe still fail \"nongit git index-pack tmp.pack\" with a BUG: though.\n\n\n\n\n"},{"id":"307894","messageId":"xmqq7f6zu55k.fsf@gitster.mtv.corp.google.com","threadId":"44714","inReplyTo":"xmqqd1gru5fw.fsf@gitster.mtv.corp.google.com","subject":"Re: index-pack outside of repository?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-16T17:59:03Z","receivedAt":"2016-12-16T17:59:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n>\n>> On Thu, Dec 15, 2016 at 08:37:28PM -0500, Jeff King wrote:\n>>\n>>> But if this case really is just \"if (from_stdin)\" that's quite easy,\n>>> too.\n>>\n>> So here is that patch (with some associated refactoring and cleanups).\n>> This is conceptually independent of jk/no-looking-at-dotgit-outside-repo-final,\n>> though it should be fine to merge with that topic. The BUG will actually\n>> pass the new test, because it calls die, too. I wonder if we should die\n>> with a unique error code on BUGs, and catch them in test_must_fail\n>> similar to the way we catch signal death.\n>>\n>>   [1/3]: t5000: extract nongit function to test-lib-functions.sh\n>>   [2/3]: index-pack: complain when --stdin is used outside of a repo\n>>   [3/3]: t: use nongit() function where applicable\n>\n> I think 2/3 is a good change to ensure we get a reasonable error for\n> \"index-pack --stdin\", and 3/3 is a very good cleanup.  Both of them\n> of course are enabled by 1/3.\n>\n> We still fail \"nongit git index-pack tmp.pack\" with a BUG: though.\n\nWait.  \n\nThis only happens with a stalled-and-to-be-discarded topic on 'pu'.\nPlease don't waste time digging it (yet).\n"},{"id":"307895","messageId":"xmqq37hnu50y.fsf@gitster.mtv.corp.google.com","threadId":"44714","inReplyTo":"xmqq7f6zu55k.fsf@gitster.mtv.corp.google.com","subject":"Re: index-pack outside of repository?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-16T18:01:49Z","receivedAt":"2016-12-16T18:02:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Jeff King <peff@peff.net> writes:\n>>\n>>> On Thu, Dec 15, 2016 at 08:37:28PM -0500, Jeff King wrote:\n>>>\n>>>> But if this case really is just \"if (from_stdin)\" that's quite easy,\n>>>> too.\n>>>\n>>> So here is that patch (with some associated refactoring and cleanups).\n>>> This is conceptually independent of jk/no-looking-at-dotgit-outside-repo-final,\n>>> though it should be fine to merge with that topic. The BUG will actually\n>>> pass the new test, because it calls die, too. I wonder if we should die\n>>> with a unique error code on BUGs, and catch them in test_must_fail\n>>> similar to the way we catch signal death.\n>>>\n>>>   [1/3]: t5000: extract nongit function to test-lib-functions.sh\n>>>   [2/3]: index-pack: complain when --stdin is used outside of a repo\n>>>   [3/3]: t: use nongit() function where applicable\n>>\n>> I think 2/3 is a good change to ensure we get a reasonable error for\n>> \"index-pack --stdin\", and 3/3 is a very good cleanup.  Both of them\n>> of course are enabled by 1/3.\n>>\n>> We still fail \"nongit git index-pack tmp.pack\" with a BUG: though.\n>\n> Wait.  \n>\n> This only happens with a stalled-and-to-be-discarded topic on 'pu'.\n> Please don't waste time digging it (yet).\n\nDon't wait ;-).  My mistake.  We can see t5300 broken with this\nchange and b1ef400eec (\"setup_git_env: avoid blind fall-back to\n\".git\"\", 2016-10-20) without anything else.  We still need to\naddress it.\n"},{"id":"307900","messageId":"xmqqlgvfso16.fsf@gitster.mtv.corp.google.com","threadId":"44714","inReplyTo":"xmqqshpou3wt.fsf@gitster.mtv.corp.google.com","subject":"Re: index-pack outside of repository?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-16T18:54:13Z","receivedAt":"2016-12-16T18:54:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n> ...\n>> I'm actually wondering if the way it calls die() in 'next' is a pretty\n>> reasonable way for things to work in general. It happens when we lazily\n>> try to ask for the repository directory. So we don't have to replicate\n>> logic to say \"are we going to need a repo\"; at the moment we need it, we\n>> notice we don't have it and die. The only problem is that it says \"BUG\"\n>> and not \"this operation must be run in a git repository\".\n>\n> Isn't what we currently have is a good way to discover which\n> codepaths we missed to add a check to issue the latter error?\n\nI am tempted to suggest an intermediate step that comes before\nb1ef400eec (\"setup_git_env: avoid blind fall-back to \".git\"\",\n2016-10-20), which is the attached, and publish that as part of an\nofficial release.  That way, we'll see what is broken without\nhurting people too much (unless they or their scripts care about\nextra message given to the standard error stream).  I suspect that\nreleased Git has a slightly larger user base than what is cooked on\n'next'.\n\n environment.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/environment.c b/environment.c\nindex 0935ec696e..88f857331e 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -167,8 +167,11 @@ static void setup_git_env(void)\n \tconst char *replace_ref_base;\n \n \tgit_dir = getenv(GIT_DIR_ENVIRONMENT);\n-\tif (!git_dir)\n+\tif (!git_dir) {\n+\t\tif (!startup_info->have_repository)\n+\t\t\twarning(\"BUG: please report this at git@vger.kernel.org\");\n \t\tgit_dir = DEFAULT_GIT_DIR_ENVIRONMENT;\n+\t}\n \tgitfile = read_gitfile(git_dir);\n \tgit_dir = xstrdup(gitfile ? gitfile : git_dir);\n \tif (get_common_dir(&sb, git_dir))\n"},{"id":"307924","messageId":"20161216214322.xibllaw2iibhc5nv@sigill.intra.peff.net","threadId":"44714","inReplyTo":"xmqq37hnu50y.fsf@gitster.mtv.corp.google.com","subject":"Re: index-pack outside of repository?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-16T21:43:22Z","receivedAt":"2016-12-16T21:43:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 16, 2016 at 10:01:49AM -0800, Junio C Hamano wrote:\n\n> >> I think 2/3 is a good change to ensure we get a reasonable error for\n> >> \"index-pack --stdin\", and 3/3 is a very good cleanup.  Both of them\n> >> of course are enabled by 1/3.\n> >>\n> >> We still fail \"nongit git index-pack tmp.pack\" with a BUG: though.\n> >\n> > Wait.  \n> >\n> > This only happens with a stalled-and-to-be-discarded topic on 'pu'.\n> > Please don't waste time digging it (yet).\n> \n> Don't wait ;-).  My mistake.  We can see t5300 broken with this\n> change and b1ef400eec (\"setup_git_env: avoid blind fall-back to\n> \".git\"\", 2016-10-20) without anything else.  We still need to\n> address it.\n\nAh, I only checked that it did not do anything terrible like write into\n\".git\". But it looks like it still looks at the git_dir value as part of\nthe collision check.\n\nHere's a patch, on top of the rest of the series.\n\n-- >8 --\nSubject: [PATCH] index-pack: skip collision check when not in repository\n\nYou can run \"git index-pack path/to/foo.pack\" outside of a\nrepository to generate an index file, or just to verify the\ncontents. There's no point in doing a collision check, since\nwe obviously do not have any objects to collide with.\n\nThe current code will blindly look in .git/objects based on\nthe result of setup_git_env(). That effectively gives us the\nright answer (since we won't find any objects), but it's a\nwaste of time, and it conflicts with our desire to\neventually get rid of the \"fallback to .git\" behavior of\nsetup_git_env().\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI didn't do a test, as this doesn't really have any user-visible\nbehavior change. I guess technically if you had a non-repo with\n\".git/objects/12/3456...\" in it we would erroneously read it, but that's\nkind of bizarre. The interesting test is that when merged with\njk/no-looking-at-dotgit-outside-repo-final, the test in t5300 doesn't\ndie.\n\n builtin/index-pack.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex d450a6ada2..f4b87c6c9f 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -787,13 +787,15 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\tconst unsigned char *sha1)\n {\n \tvoid *new_data = NULL;\n-\tint collision_test_needed;\n+\tint collision_test_needed = 0;\n \n \tassert(data || obj_entry);\n \n-\tread_lock();\n-\tcollision_test_needed = has_sha1_file_with_flags(sha1, HAS_SHA1_QUICK);\n-\tread_unlock();\n+\tif (startup_info->have_repository) {\n+\t\tread_lock();\n+\t\tcollision_test_needed = has_sha1_file_with_flags(sha1, HAS_SHA1_QUICK);\n+\t\tread_unlock();\n+\t}\n \n \tif (collision_test_needed && !data) {\n \t\tread_lock();\n-- \n2.11.0.348.g960a0b554\n\n"},{"id":"307925","messageId":"20161216214424.o3x4xqttrao3i73v@sigill.intra.peff.net","threadId":"44714","inReplyTo":"xmqqlgvfso16.fsf@gitster.mtv.corp.google.com","subject":"Re: index-pack outside of repository?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-16T21:44:25Z","receivedAt":"2016-12-16T21:44:32Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 16, 2016 at 10:54:13AM -0800, Junio C Hamano wrote:\n\n> I am tempted to suggest an intermediate step that comes before\n> b1ef400eec (\"setup_git_env: avoid blind fall-back to \".git\"\",\n> 2016-10-20), which is the attached, and publish that as part of an\n> official release.  That way, we'll see what is broken without\n> hurting people too much (unless they or their scripts care about\n> extra message given to the standard error stream).  I suspect that\n> released Git has a slightly larger user base than what is cooked on\n> 'next'.\n> \n>  environment.c | 5 ++++-\n>  1 file changed, 4 insertions(+), 1 deletion(-)\n> \n> diff --git a/environment.c b/environment.c\n> index 0935ec696e..88f857331e 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -167,8 +167,11 @@ static void setup_git_env(void)\n>  \tconst char *replace_ref_base;\n>  \n>  \tgit_dir = getenv(GIT_DIR_ENVIRONMENT);\n> -\tif (!git_dir)\n> +\tif (!git_dir) {\n> +\t\tif (!startup_info->have_repository)\n> +\t\t\twarning(\"BUG: please report this at git@vger.kernel.org\");\n>  \t\tgit_dir = DEFAULT_GIT_DIR_ENVIRONMENT;\n> +\t}\n\nYes, I think this is a nice way to ease into the change. I wish I had\nthought of it when doing the original series, and we could have shipped\nit in v2.11. :)\n\n-Peff\n"},{"id":"307927","messageId":"xmqqh963r12h.fsf@gitster.mtv.corp.google.com","threadId":"44714","inReplyTo":"20161216214322.xibllaw2iibhc5nv@sigill.intra.peff.net","subject":"Re: index-pack outside of repository?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-16T21:55:34Z","receivedAt":"2016-12-16T21:55:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Ah, I only checked that it did not do anything terrible like write into\n> \".git\". But it looks like it still looks at the git_dir value as part of\n> the collision check.\n>\n> Here's a patch, on top of the rest of the series.\n\nThanks for a quick turnaround, as always.\n\n> -- >8 --\n> Subject: [PATCH] index-pack: skip collision check when not in repository\n>\n> You can run \"git index-pack path/to/foo.pack\" outside of a\n> repository to generate an index file, or just to verify the\n> contents. There's no point in doing a collision check, since\n> we obviously do not have any objects to collide with.\n>\n> The current code will blindly look in .git/objects based on\n> the result of setup_git_env(). That effectively gives us the\n> right answer (since we won't find any objects), but it's a\n> waste of time, and it conflicts with our desire to\n> eventually get rid of the \"fallback to .git\" behavior of\n> setup_git_env().\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I didn't do a test, as this doesn't really have any user-visible\n> behavior change.\n>\n> I guess technically if you had a non-repo with\n> \".git/objects/12/3456...\" in it we would erroneously read it, but that's\n> kind of bizarre. The interesting test is that when merged with\n> jk/no-looking-at-dotgit-outside-repo-final, the test in t5300 doesn't\n> die.\n\nYes.\n\n>\n>  builtin/index-pack.c | 10 ++++++----\n>  1 file changed, 6 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index d450a6ada2..f4b87c6c9f 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -787,13 +787,15 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n>  \t\t\tconst unsigned char *sha1)\n>  {\n>  \tvoid *new_data = NULL;\n> -\tint collision_test_needed;\n> +\tint collision_test_needed = 0;\n>  \n>  \tassert(data || obj_entry);\n>  \n> -\tread_lock();\n> -\tcollision_test_needed = has_sha1_file_with_flags(sha1, HAS_SHA1_QUICK);\n> -\tread_unlock();\n> +\tif (startup_info->have_repository) {\n> +\t\tread_lock();\n> +\t\tcollision_test_needed = has_sha1_file_with_flags(sha1, HAS_SHA1_QUICK);\n> +\t\tread_unlock();\n> +\t}\n>  \n>  \tif (collision_test_needed && !data) {\n>  \t\tread_lock();\n"}]}