{"thread":{"id":"56337","subject":"[PATCH] multi-pack-index: fix --object-dir from outside repo","startedAt":"2021-08-20T19:35:18Z","lastAt":"2021-08-23T16:16:05Z","messageCount":11,"participants":["Johannes Berg","Derrick Stolee","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"433272","messageId":"20210820193504.37044-1-johannes@sipsolutions.net","threadId":"56337","inReplyTo":null,"subject":"[PATCH] multi-pack-index: fix --object-dir from outside repo","fromName":"Johannes Berg","fromEmail":"johannes@sipsolutions.net","sentAt":"2021-08-20T19:35:04Z","receivedAt":"2021-08-20T19:35:18Z","isPatch":true,"sender":{"key":"johannes@sipsolutions.net","avatar":"https://avatars.githubusercontent.com/u/5159728?v=4"},"body":"If using --object-dir to point into a repo, 'write' will\nsegfault trying to access the object-dir via the repo it\nfound, but that's not fully initialized. Fix it to use\nthe object_dir properly.\n\nFixes: 38ff7cabb6b8 (\"pack-revindex: write multi-pack reverse indexes\")\nSigned-off-by: Johannes Berg <johannes@sipsolutions.net>\n---\n midx.c                      | 10 +++++-----\n t/t5319-multi-pack-index.sh |  8 ++++++++\n 2 files changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 321c6fdd2f18..902e1a7a7d9d 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -882,7 +882,7 @@ static void write_midx_reverse_index(char *midx_name, unsigned char *midx_hash,\n \tstrbuf_release(&buf);\n }\n \n-static void clear_midx_files_ext(struct repository *r, const char *ext,\n+static void clear_midx_files_ext(const char *object_dir, const char *ext,\n \t\t\t\t unsigned char *keep_hash);\n \n static int midx_checksum_valid(struct multi_pack_index *m)\n@@ -1086,7 +1086,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \n \tif (flags & MIDX_WRITE_REV_INDEX)\n \t\twrite_midx_reverse_index(midx_name, midx_hash, &ctx);\n-\tclear_midx_files_ext(the_repository, \".rev\", midx_hash);\n+\tclear_midx_files_ext(object_dir, \".rev\", midx_hash);\n \n \tcommit_lock_file(&lk);\n \n@@ -1135,7 +1135,7 @@ static void clear_midx_file_ext(const char *full_path, size_t full_path_len,\n \t\tdie_errno(_(\"failed to remove %s\"), full_path);\n }\n \n-static void clear_midx_files_ext(struct repository *r, const char *ext,\n+static void clear_midx_files_ext(const char *object_dir, const char *ext,\n \t\t\t\t unsigned char *keep_hash)\n {\n \tstruct clear_midx_data data;\n@@ -1146,7 +1146,7 @@ static void clear_midx_files_ext(struct repository *r, const char *ext,\n \t\t\t\t    hash_to_hex(keep_hash), ext);\n \tdata.ext = ext;\n \n-\tfor_each_file_in_pack_dir(r->objects->odb->path,\n+\tfor_each_file_in_pack_dir(object_dir,\n \t\t\t\t  clear_midx_file_ext,\n \t\t\t\t  &data);\n \n@@ -1165,7 +1165,7 @@ void clear_midx_file(struct repository *r)\n \tif (remove_path(midx))\n \t\tdie(_(\"failed to clear multi-pack-index at %s\"), midx);\n \n-\tclear_midx_files_ext(r, \".rev\", NULL);\n+\tclear_midx_files_ext(r->objects->odb->path, \".rev\", NULL);\n \n \tfree(midx);\n }\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 3d4d9f10c31b..7f393e52409d 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -201,6 +201,14 @@ test_expect_success 'write midx with twelve packs' '\n \n compare_results_with_midx \"twelve packs\"\n \n+test_expect_success 'multi-pack-index with --object-dir need not be in repo' '\n+\tp=\"$(pwd)\" &&\n+\trm -f $objdir/multi-pack-index &&\n+\tcd / &&\n+\tgit multi-pack-index --object-dir=\"$p/$objdir\" write &&\n+\tcd \"$p\"\n+'\n+\n test_expect_success 'warn on improper hash version' '\n \tgit init --object-format=sha1 sha1 &&\n \t(\n-- \n2.31.1\n\n"},{"id":"433335","messageId":"04ed58aa-94fa-010e-f4db-f41cd51876a5@gmail.com","threadId":"56337","inReplyTo":"20210820193504.37044-1-johannes@sipsolutions.net","subject":"Re: [PATCH] multi-pack-index: fix --object-dir from outside repo","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-22T23:51:01Z","receivedAt":"2021-08-22T23:51:08Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/20/2021 3:35 PM, Johannes Berg wrote:\n> If using --object-dir to point into a repo, 'write' will\n> segfault trying to access the object-dir via the repo it\n> found, but that's not fully initialized. Fix it to use\n> the object_dir properly.\n\nThanks for finding this! It's difficult to cover all of\nthe cases, but I'm glad you found this and added a test.\n\n> +test_expect_success 'multi-pack-index with --object-dir need not be in repo' '\n> +\tp=\"$(pwd)\" &&\n> +\trm -f $objdir/multi-pack-index &&\n> +\tcd / &&\n> +\tgit multi-pack-index --object-dir=\"$p/$objdir\" write &&\n> +\tcd \"$p\"\n\nWhy are you using \"cd /\" here? Even if you mean to use \"cd\",\nplease do so within a sub-shell.\n\nCould you instead init a new repo within the current directory\nand point the object-dir to that location?\n\nIt could look something like this, (warning: I did not test this)\n\n\tgit init other &&\n\ttest_commit -C other first &&\n\tgit multi-pack-index --object-dir=other/.git/objects write\n\nAnd is the only post-condition you are checking that we do not\ncrash? Or is there a specific result you are looking for? For\ninstance, we can double check that the MIDX was written:\n\n\ttest_path_is_file other/.git/objects/pack/multi-pack-index\n\nbut also you seem to be touching areas that delete files. Could\nwe 'touch' some of those and then see them get deleted?\n\nThanks,\n-Stolee\n"},{"id":"433337","messageId":"YSLxqnxlyEUQ+ljJ@nand.local","threadId":"56337","inReplyTo":"20210820193504.37044-1-johannes@sipsolutions.net","subject":"Re: [PATCH] multi-pack-index: fix --object-dir from outside repo","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-08-23T00:54:02Z","receivedAt":"2021-08-23T00:54:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Aug 20, 2021 at 09:35:04PM +0200, Johannes Berg wrote:\n> If using --object-dir to point into a repo, 'write' will\n> segfault trying to access the object-dir via the repo it\n> found, but that's not fully initialized. Fix it to use\n> the object_dir properly.\n\nThanks for CC'ing me; I have definitely been wondering about the\nintended behavior of `--object-dir` on the list recently [1].\n\nI think your patch message could use some clarifying, though.  Invoking\n\n    cd $REPO/..\n    git multi-pack-index write --object-dir=$REPO/.git/objects\n\nhas... different behavior depending on which side of the \"write\"\nargument you put `--object-dir\". On the left-hand side (i.e.,\n\"--object-dir=... write\", you get something like:\n\n    cd $REPO/..\n    git multi-pack-index --object-dir=$REPO/.git/objects write\n\n    zsh: segmentation fault  git.compile multi-pack-index ...\n\nbecause the_repository->objects->odb isn't initialized (so reading\n`path` in `clear_midx_files_ext` crashes). But in the opposite order\n(i.e., \"write --object-dir=...\") you get:\n\n    BUG: environment.c:280: git environment hasn't been setup\n    zsh: abort      git.compile multi-pack-index write\n\nbecause we catch that case much earlier in get_object_directory(). Why?\nBecause cmd_multi_pack_index() fills in the value of object_dir with\nget_object_directory() if it isn't filled in already, but seeing \"write\"\ncauses us to stop parsing and dispatch to the sub-command\ncmd_multi_pack_index_write().\n\nI discussed this a little in [1] also (see the part about using\nRUN_SETUP instead). There are definitely different ways to handle that;\nyou could equally imagine only dying if we were both outside of a Git\nrepository and didn't point at one via `--object-dir`.\n\nBut that's separate from another issue which is fixed by your patch\nwhich is that we don't respect the value of `--object-dir` when cleaning\nup MIDX .rev files via clear_midx_files_ext().\n\nYour fix there (to use the path of an object_dir instead of a repository\nstruct) makes sense (since we don't ever fill in a repository struct\ncorresponding to the `--object-dir` parameter from the MIDX code).\n\nBut I think that's a separate issue than the RUN_SETUP thing I mentioned\nearlier, so I would probably consider breaking this into two patches,\nthe first which addresses the RUN_SETUP thing, and the second which is\nthis fix.\n\n>  static int midx_checksum_valid(struct multi_pack_index *m)\n> @@ -1086,7 +1086,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n>\n>  \tif (flags & MIDX_WRITE_REV_INDEX)\n>  \t\twrite_midx_reverse_index(midx_name, midx_hash, &ctx);\n> -\tclear_midx_files_ext(the_repository, \".rev\", midx_hash);\n> +\tclear_midx_files_ext(object_dir, \".rev\", midx_hash);\n\nWe can rely on this value always being non-NULL, so this is good.\n\n> -static void clear_midx_files_ext(struct repository *r, const char *ext,\n> +static void clear_midx_files_ext(const char *object_dir, const char *ext,\n>  \t\t\t\t unsigned char *keep_hash)\n>  {\n>  \tstruct clear_midx_data data;\n> @@ -1146,7 +1146,7 @@ static void clear_midx_files_ext(struct repository *r, const char *ext,\n>  \t\t\t\t    hash_to_hex(keep_hash), ext);\n>  \tdata.ext = ext;\n>\n> -\tfor_each_file_in_pack_dir(r->objects->odb->path,\n> +\tfor_each_file_in_pack_dir(object_dir,\n>  \t\t\t\t  clear_midx_file_ext,\n>  \t\t\t\t  &data);\n\nAnd here's the most important part of the change, which is obviously\ncorrect. But note to other reviewers that this has nothing to do with\nthe RUN_SETUP issue I mentioned earlier, since\nfor_each_file_in_pack_dir() doesn't care about that.\n\n> +test_expect_success 'multi-pack-index with --object-dir need not be in repo' '\n> +\tp=\"$(pwd)\" &&\n> +\trm -f $objdir/multi-pack-index &&\n> +\tcd / &&\n> +\tgit multi-pack-index --object-dir=\"$p/$objdir\" write &&\n> +\tcd \"$p\"\n> +'\n> +\n\nI agree with Stolee that there should be a new repo created within the\ncurrent working directory, that way you can \"cd ..\" and be both outside\nof the repo you just created, but not outside of the test environment.\n\nBut let's make sure that we're not deleting any files that we should be\nleaving alone. So it might be good to do something like:\n\n    git init repo &&\n    test_when_finished \"rm -fr repo\" &&\n    (\n      cd repo &&\n\n      test_commit base &&\n      git repack -d &&\n    ) &&\n\n    rev=\"$objdir/pack/multi-pack-index-$(midx_checksum $objdir).rev\" &&\n    touch $rev &&\n\n    git multi-pack-index write --object-dir=repo/.git/objects &&\n\n    test_path_is_file repo/.git/objects/pack/multi-pack-index &&\n    test_path_is_file repo/.git/objects/multi-pack-index &&\n    test_path_is_file $objdir/pack/multi-pack-index &&\n    test_path_is_file $rev\n\nThat isn't testing the \"invoked from a non-repo, but --object-dir\" is\ngiven case, but I think that's fine since they really are separate\nthings.\n\nNote also that midx_checksum doesn't exist, but it is merely a wrapper\nover a test-tool that prints out (for a multi_pack_index \"m\") `m->data +\nm->data_len - the_hash_algo->rawsz`.\n\nSo between splitting the patch, clarifying the patch message, and\nimplementing support for this new test helper, this may be more of a\nproject than you were bargaining for ;). Let me know if you want any\nhelp. I also don't mind taking care of it myself, since I promised in\n[1] that I'd fix this issue anyway.\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/YQMFIljXl7sAAA%2FL@nand.local/\n"},{"id":"433348","messageId":"4d65ef5b0a9e4104d763facc42d10a20557d054d.camel@sipsolutions.net","threadId":"56337","inReplyTo":"04ed58aa-94fa-010e-f4db-f41cd51876a5@gmail.com","subject":"Re: [PATCH] multi-pack-index: fix --object-dir from outside repo","fromName":"Johannes Berg","fromEmail":"johannes@sipsolutions.net","sentAt":"2021-08-23T07:21:10Z","receivedAt":"2021-08-23T07:21:15Z","isPatch":true,"sender":{"key":"johannes@sipsolutions.net","avatar":"https://avatars.githubusercontent.com/u/5159728?v=4"},"body":"Hi Derrick,\n\n> > +test_expect_success 'multi-pack-index with --object-dir need not be in repo' '\n> > +\tp=\"$(pwd)\" &&\n> > +\trm -f $objdir/multi-pack-index &&\n> > +\tcd / &&\n> > +\tgit multi-pack-index --object-dir=\"$p/$objdir\" write &&\n> > +\tcd \"$p\"\n> \n> Why are you using \"cd /\" here? \n> \n\nI just needed to go outside the current test git directory, the tests\nare running in a way that the current working directory is already the\ngit tree I'm operating in.\n\n> Even if you mean to use \"cd\",\n> please do so within a sub-shell.\n\nI thought about it, but clearly all the tests are run in a sub-shell, so\nit didn't seem necessary? But happy to change, I don't really care\neither way.\n\nCould you instead init a new repo within the current directory\nand point the object-dir to that location?\n\nI guess I could, but all the other stuff in here is already making a new\nrepo in the current working dir, and already initializing it with\nobjects, etc.\n\nDoing it all over again seemed like a waste of time?\n\nIt could look something like this, (warning: I did not test this)\n\n\tgit init other &&\n\ttest_commit -C other first &&\n\tgit multi-pack-index --object-dir=other/.git/objects write\n\nSure.\n\nActually, this won't work to test for the crash, I'd have to do\nsomething like\n\ngit init other\ntest_commit -C other first &&\n(\nmkdir non-git &&\ncd non-git &&\ngit multi-pack-index --object-dir=../other/.git/objects write\n)\n\nor so.\n\nAnd is the only post-condition you are checking that we do not\ncrash?\n\nYes, I was assuming that it'd actually work at that point - maybe not\nthe best assumption, it could (erroneously) exit with a 0 exit status\nbut have done nothing.\n\n> Or is there a specific result you are looking for? For\ninstance, we can double check that the MIDX was written:\n\n\ttest_path_is_file other/.git/objects/pack/multi-pack-index\n\nSo I guess that would be a good idea.\n\nbut also you seem to be touching areas that delete files. Could\nwe 'touch' some of those and then see them get deleted?\n\nAh, well, that's the underlying issue but I'm not sure we even ever get\nto that code? Then again, yes, the *.rev files should get removed, I'll\nsee - not even sure I know how to get them to be generated in the first\nplace, is that even supported already?\n\njohannes\n\n"},{"id":"433349","messageId":"c74ff2a21c09a9ac69b73c53deb3bb4f0159775b.camel@sipsolutions.net","threadId":"56337","inReplyTo":"YSLxqnxlyEUQ+ljJ@nand.local","subject":"Re: [PATCH] multi-pack-index: fix --object-dir from outside repo","fromName":"Johannes Berg","fromEmail":"johannes@sipsolutions.net","sentAt":"2021-08-23T07:32:15Z","receivedAt":"2021-08-23T07:32:21Z","isPatch":true,"sender":{"key":"johannes@sipsolutions.net","avatar":"https://avatars.githubusercontent.com/u/5159728?v=4"},"body":"Hi Taylor,\n\n> Thanks for CC'ing me; I have definitely been wondering about the\n> intended behavior of `--object-dir` on the list recently [1].\n\nOh, hah. Not really being a contributor I'm not following the list,\nthanks for the pointer.\n\n> I think your patch message could use some clarifying, though.  Invoking\n> \n>     cd $REPO/..\n>     git multi-pack-index write --object-dir=$REPO/.git/objects\n> \n> has... different behavior depending on which side of the \"write\"\n> argument you put `--object-dir\".\n\nWait what?! To be honest, I didn't expect it to even be valid on the\nright-hand side of \"write\".\n\n>  On the left-hand side (i.e.,\n> \"--object-dir=... write\", you get something like:\n> \n>     cd $REPO/..\n>     git multi-pack-index --object-dir=$REPO/.git/objects write\n> \n>     zsh: segmentation fault  git.compile multi-pack-index ...\n> \n> because the_repository->objects->odb isn't initialized (so reading\n> `path` in `clear_midx_files_ext` crashes).\n\nRight, this is what I was doing.\n\n>  But in the opposite order\n> (i.e., \"write --object-dir=...\") you get:\n> \n>     BUG: environment.c:280: git environment hasn't been setup\n>     zsh: abort      git.compile multi-pack-index write\n> \n> because we catch that case much earlier in get_object_directory(). Why?\n> Because cmd_multi_pack_index() fills in the value of object_dir with\n> get_object_directory() if it isn't filled in already, but seeing \"write\"\n> causes us to stop parsing and dispatch to the sub-command\n> cmd_multi_pack_index_write().\n\nGreat ...\n\nBut why do we even support both? What would the semantic difference be?\n\nI'd be happy with either one of them working I guess :)\n\n> I discussed this a little in [1] also (see the part about using\n> RUN_SETUP instead). There are definitely different ways to handle that;\n> you could equally imagine only dying if we were both outside of a Git\n> repository and didn't point at one via `--object-dir`.\n> \n> But that's separate from another issue which is fixed by your patch\n> which is that we don't respect the value of `--object-dir` when cleaning\n> up MIDX .rev files via clear_midx_files_ext().\n\nWell, I _mostly_ meant to fix the crash, but yes, this is really the\nunderlying issue, and indeed we should clean up the .rev files in this\ncase as well.\n\n> Your fix there (to use the path of an object_dir instead of a repository\n> struct) makes sense (since we don't ever fill in a repository struct\n> corresponding to the `--object-dir` parameter from the MIDX code).\n> \n> But I think that's a separate issue than the RUN_SETUP thing I mentioned\n> earlier, so I would probably consider breaking this into two patches,\n> the first which addresses the RUN_SETUP thing, and the second which is\n> this fix.\n\nI never wanted to fix the other issue though ;-)\n\nAnd honestly, I think I don't understand the discussion at [1] well\nenough to really submit a patch for it.\n\n> > +test_expect_success 'multi-pack-index with --object-dir need not be in repo' '\n> > +\tp=\"$(pwd)\" &&\n> > +\trm -f $objdir/multi-pack-index &&\n> > +\tcd / &&\n> > +\tgit multi-pack-index --object-dir=\"$p/$objdir\" write &&\n> > +\tcd \"$p\"\n> > +'\n> > +\n> \n> I agree with Stolee that there should be a new repo created within the\n> current working directory, that way you can \"cd ..\" and be both outside\n> of the repo you just created, but not outside of the test environment.\n\nOK, fair enough, I'll resubmit.\n\n> But let's make sure that we're not deleting any files that we should be\n> leaving alone. So it might be good to do something like:\n> \n>     git init repo &&\n>     test_when_finished \"rm -fr repo\" &&\n>     (\n>       cd repo &&\n> \n>       test_commit base &&\n>       git repack -d &&\n>     ) &&\n> \n>     rev=\"$objdir/pack/multi-pack-index-$(midx_checksum $objdir).rev\" &&\n>     touch $rev &&\n\nHah, so you just manually pretend it was there - and meanwhile I was\nlooking for a way to get git to generate one :)\n\n> \n>     git multi-pack-index write --object-dir=repo/.git/objects &&\n\nNow this has the order of arguments the other way around, why?\n\n>     test_path_is_file repo/.git/objects/pack/multi-pack-index &&\n>     test_path_is_file repo/.git/objects/multi-pack-index &&\n>     test_path_is_file $objdir/pack/multi-pack-index &&\n>     test_path_is_file $rev\n\nWhy would test_path_is_file? Seems like it should be !test_path_is_file?\n\n> \n> That isn't testing the \"invoked from a non-repo, but --object-dir\" is\n> given case, but I think that's fine since they really are separate\n> things.\n\nBut that's the one thing I really want to work :)\n\n> Note also that midx_checksum doesn't exist, but it is merely a wrapper\n> over a test-tool that prints out (for a multi_pack_index \"m\") `m->data +\n> m->data_len - the_hash_algo->rawsz`.\n> \n> So between splitting the patch, clarifying the patch message, and\n> implementing support for this new test helper, this may be more of a\n> project than you were bargaining for ;).\n> \n\nSounds like ;)\nBut actually we don't really care about the midx_checksum here, afaict?\nMIDX_WRITE_REV_INDEX isn't ever set, so the rev files are not created\ntoday?\n\n> Let me know if you want any\n> help. I also don't mind taking care of it myself, since I promised in\n> [1] that I'd fix this issue anyway.\n\nThanks :)\nHow about I resubmit this patch with some of the edits, especially with\ntest for the case I care about (--object-dir used from a non-git place\nto point elsewhere) and then you can build on top of that?\n\nThanks,\njohannes\n\n> [1]: https://lore.kernel.org/git/YQMFIljXl7sAAA%2FL@nand.local/\n> \n\n"},{"id":"433350","messageId":"xmqqo89osi0b.fsf@gitster.g","threadId":"56337","inReplyTo":"4d65ef5b0a9e4104d763facc42d10a20557d054d.camel@sipsolutions.net","subject":"Re: [PATCH] multi-pack-index: fix --object-dir from outside repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-23T08:05:40Z","receivedAt":"2021-08-23T08:05:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Berg <johannes@sipsolutions.net> writes:\n\n> I just needed to go outside the current test git directory, the tests\n> are running in a way that the current working directory is already the\n> git tree I'm operating in.\n>\n>> Even if you mean to use \"cd\",\n>> please do so within a sub-shell.\n>\n> I thought about it, but clearly all the tests are run in a sub-shell, so\n> it didn't seem necessary? But happy to change, I don't really care\n> either way.\n\nPlease learn to care before you write your next test, then ;-)\n\nThese tests are not run in a sub-shell; they are eval'ed, so that\nthe assignment they make to variables can persist and affect the\nnext test piece.\n\nThanks.\n"},{"id":"433351","messageId":"caafaf945ec43ba606b054bf4c4faa42e35a8db1.camel@sipsolutions.net","threadId":"56337","inReplyTo":"xmqqo89osi0b.fsf@gitster.g","subject":"Re: [PATCH] multi-pack-index: fix --object-dir from outside repo","fromName":"Johannes Berg","fromEmail":"johannes@sipsolutions.net","sentAt":"2021-08-23T08:10:10Z","receivedAt":"2021-08-23T08:10:24Z","isPatch":true,"sender":{"key":"johannes@sipsolutions.net","avatar":"https://avatars.githubusercontent.com/u/5159728?v=4"},"body":"On Mon, 2021-08-23 at 01:05 -0700, Junio C Hamano wrote:\n> Johannes Berg <johannes@sipsolutions.net> writes:\n> \n> > I just needed to go outside the current test git directory, the tests\n> > are running in a way that the current working directory is already the\n> > git tree I'm operating in.\n> > \n> > > Even if you mean to use \"cd\",\n> > > please do so within a sub-shell.\n> > \n> > I thought about it, but clearly all the tests are run in a sub-shell, so\n> > it didn't seem necessary? But happy to change, I don't really care\n> > either way.\n> \n> Please learn to care before you write your next test, then ;-)\n\nHey now, I'm fixing your segfaults ;-)\n\n> These tests are not run in a sub-shell; they are eval'ed, so that\n> the assignment they make to variables can persist and affect the\n> next test piece.\n\nMakes sense. FWIW, the test *did* restore the CWD so things worked, and\nsubshells are actually ugly (need to import test-lib-functions.sh again\nif you want to use those), but I'll make it work somehow.\n\n\nMore importantly, how do you feel about the \"cd /\"?\n\nThe tests are always run in a place where there's a parent git folder\n(even if it's git itself), so you cannot reproduce the segfault in a\ntest without the \"cd /\", though I guess \"cd /tmp\" would also work or\nsomething, but \"cd /\" felt pretty safe, hopefully not many people have\n\"/.git\" on their system.\n\njohannes\n\n"},{"id":"433440","messageId":"414ed641-2bd3-1316-8189-ad542988d091@gmail.com","threadId":"56337","inReplyTo":"caafaf945ec43ba606b054bf4c4faa42e35a8db1.camel@sipsolutions.net","subject":"Re: [PATCH] multi-pack-index: fix --object-dir from outside repo","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-23T13:19:23Z","receivedAt":"2021-08-23T13:19:28Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/23/2021 4:10 AM, Johannes Berg wrote:\n> On Mon, 2021-08-23 at 01:05 -0700, Junio C Hamano wrote:\n>> Johannes Berg <johannes@sipsolutions.net> writes:\n>>\n>>> I just needed to go outside the current test git directory, the tests\n>>> are running in a way that the current working directory is already the\n>>> git tree I'm operating in.\n>>>\n>>>> Even if you mean to use \"cd\",\n>>>> please do so within a sub-shell.\n>>>\n>>> I thought about it, but clearly all the tests are run in a sub-shell, so\n>>> it didn't seem necessary? But happy to change, I don't really care\n>>> either way.\n>>\n>> Please learn to care before you write your next test, then ;-)\n> \n> Hey now, I'm fixing your segfaults ;-)\n> \n>> These tests are not run in a sub-shell; they are eval'ed, so that\n>> the assignment they make to variables can persist and affect the\n>> next test piece.\n> \n> Makes sense. FWIW, the test *did* restore the CWD so things worked,\n\nThis assumes that your test completes to run the second \"cd\".\n\n> and\n> subshells are actually ugly (need to import test-lib-functions.sh again\n> if you want to use those), but I'll make it work somehow.\n\nWe just add subshells this way:\n\ntest_expect_success 'test name' '\n\tprep_step &&\n\t(\n\t\t# now in a subshell\n\t\tcd wherever &&\n\t\tdo things\n\t\t# don't need to cd again\n\t) &&\n\tcontinue test\n'\n\n> More importantly, how do you feel about the \"cd /\"?\n>\n> The tests are always run in a place where there's a parent git folder\n> (even if it's git itself), so you cannot reproduce the segfault in a\n> test without the \"cd /\", though I guess \"cd /tmp\" would also work or\n> something, but \"cd /\" felt pretty safe, hopefully not many people have\n> \"/.git\" on their system.\n\nDon't leave the directory your test is set up to run in.\n\nGit has a very large test suite full of examples to use for inspiration.\nIf you do not see a pattern used within the test suite, then there is\nprobably good reason to avoid that pattern.\n\nThanks,\n-Stolee\n"},{"id":"433443","messageId":"746f574d20c54b5f7d1eaae74f54a624573ad6bc.camel@sipsolutions.net","threadId":"56337","inReplyTo":"414ed641-2bd3-1316-8189-ad542988d091@gmail.com","subject":"Re: [PATCH] multi-pack-index: fix --object-dir from outside repo","fromName":"Johannes Berg","fromEmail":"johannes@sipsolutions.net","sentAt":"2021-08-23T13:40:10Z","receivedAt":"2021-08-23T13:40:16Z","isPatch":true,"sender":{"key":"johannes@sipsolutions.net","avatar":"https://avatars.githubusercontent.com/u/5159728?v=4"},"body":"On Mon, 2021-08-23 at 09:19 -0400, Derrick Stolee wrote:\n> \n> We just add subshells this way:\n> \n> test_expect_success 'test name' '\n> \tprep_step &&\n> \t(\n> \t\t# now in a subshell\n> \t\tcd wherever &&\n> \t\tdo things\n> \t\t# don't need to cd again\n> \t) &&\n> \tcontinue test\n> '\n\nSure. I know how to do subshells :)\n\nMy point was that inside the subshell you cannot do test_path_is_file\nand similar, because the subshell didn't import the libs.\n\n> > More importantly, how do you feel about the \"cd /\"?\n> > \n> > The tests are always run in a place where there's a parent git folder\n> > (even if it's git itself), so you cannot reproduce the segfault in a\n> > test without the \"cd /\", though I guess \"cd /tmp\" would also work or\n> > something, but \"cd /\" felt pretty safe, hopefully not many people have\n> > \"/.git\" on their system.\n> \n> Don't leave the directory your test is set up to run in.\n\nI was specifically asking Junio ;-)\n\nBut realistically, if this is the requirement you want to impose, then\nyou _cannot_ test for the segfault within git's test suite. Your loss.\n\njohannes\n\n\n"},{"id":"433452","messageId":"xmqq1r6krvrp.fsf@gitster.g","threadId":"56337","inReplyTo":"caafaf945ec43ba606b054bf4c4faa42e35a8db1.camel@sipsolutions.net","subject":"Re: [PATCH] multi-pack-index: fix --object-dir from outside repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-23T16:06:02Z","receivedAt":"2021-08-23T16:06:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Berg <johannes@sipsolutions.net> writes:\n\n> Makes sense. FWIW, the test *did* restore the CWD so things worked, and\n> subshells are actually ugly (need to import test-lib-functions.sh again\n> if you want to use those), but I'll make it work somehow.\n\nYou do not need to dot-include test-lib-functions or anything ugly\nor special.  The variables (not only the exported ones but regular\nshell variables) and shell functions that is visible immediately\nbefore you enter the opening \"(\" are all visible in the subshell.\n\nThe only notable difference you need to keep in mind when using\nsubshell is that you cannot affect variables and environment in\ngeneral of the calling shell.  In this case, you are taking\nadvantage of it---no matter where you chdir to, the main test\nprocedure that spawned the subshell will not be affected even if\nyour tests fail inside a subshell.  But it also disallows you from\ndoing certain things that rely on the ability to modify shell\nvariables, like setting up test_when_finished clean-up routine.\n\n> More importantly, how do you feel about the \"cd /\"?\n\nPlease don't.  If somebody had a repository in /.git and the program\nyou are testing is buggy, you'd risk destroying it.  In general, it\nis not a good idea to step outside the test directory you are given,\nespecially if you are *not* limiting yourself to read-only operation.\n\n> The tests are always run in a place where there's a parent git folder\n> (even if it's git itself), so you cannot reproduce the segfault in a\n> test without the \"cd /\"\n\nThere is a \"nongit\" test helper in the test suite.  Would that work\nfor your case?\n\nThanks.\n"},{"id":"433456","messageId":"xmqqwnocqgqm.fsf@gitster.g","threadId":"56337","inReplyTo":"746f574d20c54b5f7d1eaae74f54a624573ad6bc.camel@sipsolutions.net","subject":"Re: [PATCH] multi-pack-index: fix --object-dir from outside repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-23T16:16:01Z","receivedAt":"2021-08-23T16:16:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Berg <johannes@sipsolutions.net> writes:\n\n> On Mon, 2021-08-23 at 09:19 -0400, Derrick Stolee wrote:\n>> \n>> We just add subshells this way:\n>> \n>> test_expect_success 'test name' '\n>> \tprep_step &&\n>> \t(\n>> \t\t# now in a subshell\n>> \t\tcd wherever &&\n>> \t\tdo things\n>> \t\t# don't need to cd again\n>> \t) &&\n>> \tcontinue test\n>> '\n>\n> Sure. I know how to do subshells :)\n>\n> My point was that inside the subshell you cannot do test_path_is_file\n> and similar, because the subshell didn't import the libs.\n\nEverything the call to prep_step, and anything that came before that\ncall, did to the environment, like setting shell functions and\nvariables, is visible inside the ( ... subshell ... ).\n\n> I was specifically asking Junio ;-)\n>\n> But realistically, if this is the requirement you want to impose, then\n> you _cannot_ test for the segfault within git's test suite. Your loss.\n\nWith that \"cannot\", I think you are assuming too much.  \n\nBecause Git is a fairly long-lived project, we've had our share of\ncases where we needed to test for bugs that happen only when outside\na repository.\n\nAnd we have facility for just that, it's called \"nongit\" test helper\nthat comes from test-lib-functions.sh, which is dot-included already\nso your subshells get it for free.\n\nt5300-pack-object.sh, for example, wants to make sure that the \"git\nindex-pack --stdin\" command invoked in a directory that is not a\nrepository, but \"git index-pack <packfile>\" works outside a\nrepository, and has two tests that uses the nongit helper.\n\nThanks.\n"}]}