{"thread":{"id":"58822","subject":"[PATCH] prune: quiet ENOENT on missing directories","startedAt":"2022-11-19T20:12:17Z","lastAt":"2022-11-22T23:16:23Z","messageCount":10,"participants":["Eric Wong","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"467595","messageId":"20221119201213.2398081-1-e@80x24.org","threadId":"58822","inReplyTo":null,"subject":"[PATCH] prune: quiet ENOENT on missing directories","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2022-11-19T20:12:13Z","receivedAt":"2022-11-19T20:12:17Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"$GIT_DIR/objects/pack may be removed to save inodes in shared\nrepositories.  Quiet down prune in cases where either\n$GIT_DIR/objects or $GIT_DIR/objects/pack is non-existent,\nbut emit the system error in other cases to help users diagnose\npermissions problems or resource constraints.\n\nSigned-off-by: Eric Wong <e@80x24.org>\n---\n builtin/prune.c  | 4 +++-\n t/t5304-prune.sh | 8 ++++++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex df376b2ed1..2719220108 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -127,7 +127,9 @@ static void remove_temporary_files(const char *path)\n \n \tdir = opendir(path);\n \tif (!dir) {\n-\t\tfprintf(stderr, \"Unable to open directory %s\\n\", path);\n+\t\tif (errno != ENOENT)\n+\t\t\tfprintf(stderr, \"Unable to open directory %s: %s\\n\",\n+\t\t\t\tpath, strerror(errno));\n \t\treturn;\n \t}\n \twhile ((de = readdir(dir)) != NULL)\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 8ae314af58..d65a5f94b4 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -29,6 +29,14 @@ test_expect_success setup '\n \tgit gc\n '\n \n+test_expect_success 'bare repo prune is quiet without $GIT_DIR/objects/pack' '\n+\tgit clone -q --shared --template= --bare . bare.git &&\n+\trmdir bare.git/objects/pack &&\n+\tgit --git-dir=bare.git prune --no-progress 2>prune.err &&\n+\ttest_must_be_empty prune.err &&\n+\trm -r bare.git prune.err\n+'\n+\n test_expect_success 'prune stale packs' '\n \torig_pack=$(echo .git/objects/pack/*.pack) &&\n \t>.git/objects/tmp_1.pack &&\n"},{"id":"467660","messageId":"xmqq5yf8yhe0.fsf@gitster.g","threadId":"58822","inReplyTo":"20221119201213.2398081-1-e@80x24.org","subject":"Re: [PATCH] prune: quiet ENOENT on missing directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-21T06:02:31Z","receivedAt":"2022-11-21T06:02:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <e@80x24.org> writes:\n\n> $GIT_DIR/objects/pack may be removed to save inodes in shared\n> repositories.  Quiet down prune in cases where either\n> $GIT_DIR/objects or $GIT_DIR/objects/pack is non-existent,\n\nWouldn't setup.c::is_git_directory() say \"nope, you do not have a\nrepository there\" if you are missing $GIT_DIR/objects?  So I suspect\nthat the only case this matters in practice is a missing pack/\nsubdirectory.\n\nI agree that silently ignoring missing objects/pack/ is perfectly\nfine, whether we auto-vivify it when we actually create a pack.\n\n> but emit the system error in other cases to help users diagnose\n> permissions problems or resource constraints.\n\nOK.\n\n> @@ -127,7 +127,9 @@ static void remove_temporary_files(const char *path)\n>  \n>  \tdir = opendir(path);\n>  \tif (!dir) {\n> -\t\tfprintf(stderr, \"Unable to open directory %s\\n\", path);\n> +\t\tif (errno != ENOENT)\n> +\t\t\tfprintf(stderr, \"Unable to open directory %s: %s\\n\",\n> +\t\t\t\tpath, strerror(errno));\n>  \t\treturn;\n>  \t}\n\nThis is called twice, with $GIT_OBJECT_DIRECTORY and its pack\nsubdirectory, as it does not recurse.  \n\nThis is a tangent, I have to wonder how effective the first call\nwould be, though.  When writing a loose object file, we compute its\nobject name first in-core and determine the final filename, create a\ntemporary file in the same directory as the final file, write into\nit and then finally rename the temporary to the final name.  The\nfan-out $GIT_OBJECT_DIRECTORY/??/ directories may have temporary\nfiles left when such a process crashed, but do we create cruft \"git\nprune\" should remove in $GIT_OBJECT_DIRECTORY/ itself?\n\n> diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\n> index 8ae314af58..d65a5f94b4 100755\n> --- a/t/t5304-prune.sh\n> +++ b/t/t5304-prune.sh\n> @@ -29,6 +29,14 @@ test_expect_success setup '\n>  \tgit gc\n>  '\n>  \n> +test_expect_success 'bare repo prune is quiet without $GIT_DIR/objects/pack' '\n> +\tgit clone -q --shared --template= --bare . bare.git &&\n> +\trmdir bare.git/objects/pack &&\n> +\tgit --git-dir=bare.git prune --no-progress 2>prune.err &&\n> +\ttest_must_be_empty prune.err &&\n> +\trm -r bare.git prune.err\n> +'\n> +\n>  test_expect_success 'prune stale packs' '\n>  \torig_pack=$(echo .git/objects/pack/*.pack) &&\n>  \t>.git/objects/tmp_1.pack &&\n"},{"id":"467668","messageId":"20221121104427.M268307@dcvr","threadId":"58822","inReplyTo":"xmqq5yf8yhe0.fsf@gitster.g","subject":"Re: [PATCH] prune: quiet ENOENT on missing directories","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2022-11-21T10:44:27Z","receivedAt":"2022-11-21T10:44:32Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Wong <e@80x24.org> writes:\n> \n> > $GIT_DIR/objects/pack may be removed to save inodes in shared\n> > repositories.  Quiet down prune in cases where either\n> > $GIT_DIR/objects or $GIT_DIR/objects/pack is non-existent,\n> \n> Wouldn't setup.c::is_git_directory() say \"nope, you do not have a\n> repository there\" if you are missing $GIT_DIR/objects?  So I suspect\n> that the only case this matters in practice is a missing pack/\n> subdirectory.\n\nRight.  Removing $GIT_DIR/objects isn't currently OK, but maybe\nsomeday it could be...  Supporting missing pack/ is the primary\nreason for this change, but making a small step towards allowing\nobjects/-free $GIT_DIR doesn't seem harmful.\n\n> I agree that silently ignoring missing objects/pack/ is perfectly\n> fine, whether we auto-vivify it when we actually create a pack.\n> \n> > but emit the system error in other cases to help users diagnose\n> > permissions problems or resource constraints.\n> \n> OK.\n> \n> > @@ -127,7 +127,9 @@ static void remove_temporary_files(const char *path)\n> >  \n> >  \tdir = opendir(path);\n> >  \tif (!dir) {\n> > -\t\tfprintf(stderr, \"Unable to open directory %s\\n\", path);\n> > +\t\tif (errno != ENOENT)\n> > +\t\t\tfprintf(stderr, \"Unable to open directory %s: %s\\n\",\n> > +\t\t\t\tpath, strerror(errno));\n> >  \t\treturn;\n> >  \t}\n> \n> This is called twice, with $GIT_OBJECT_DIRECTORY and its pack\n> subdirectory, as it does not recurse.  \n\nRight.\n\n> This is a tangent, I have to wonder how effective the first call\n> would be, though.  When writing a loose object file, we compute its\n> object name first in-core and determine the final filename, create a\n> temporary file in the same directory as the final file, write into\n> it and then finally rename the temporary to the final name.  The\n> fan-out $GIT_OBJECT_DIRECTORY/??/ directories may have temporary\n> files left when such a process crashed, but do we create cruft \"git\n> prune\" should remove in $GIT_OBJECT_DIRECTORY/ itself?\n\nGood question, perhaps this could be a followup:\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 2719220108..041c45ecbe 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -188,7 +188,6 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \t\t\t\t      prune_cruft, prune_subdir, &revs);\n \n \tprune_packed_objects(show_only ? PRUNE_PACKED_DRY_RUN : 0);\n-\tremove_temporary_files(get_object_directory());\n \ts = mkpathdup(\"%s/pack\", get_object_directory());\n \tremove_temporary_files(s);\n \tfree(s);\n\nOTOH, perhaps there's some 3rd-party tools (e.g. backup tools)\nthat leave stuff in top-level objects/ and we'd risk breaking\na rare setup via ENOSPC.\n"},{"id":"467669","messageId":"221121.86bkp0czz2.gmgdl@evledraar.gmail.com","threadId":"58822","inReplyTo":"20221119201213.2398081-1-e@80x24.org","subject":"Re: [PATCH] prune: quiet ENOENT on missing directories","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-21T11:16:27Z","receivedAt":"2022-11-21T11:29:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Nov 19 2022, Eric Wong wrote:\n\n> $GIT_DIR/objects/pack may be removed to save inodes in shared\n> repositories.  Quiet down prune in cases where either\n> $GIT_DIR/objects or $GIT_DIR/objects/pack is non-existent,\n> but emit the system error in other cases to help users diagnose\n> permissions problems or resource constraints.\n>\n> Signed-off-by: Eric Wong <e@80x24.org>\n> ---\n>  builtin/prune.c  | 4 +++-\n>  t/t5304-prune.sh | 8 ++++++++\n>  2 files changed, 11 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/prune.c b/builtin/prune.c\n> index df376b2ed1..2719220108 100644\n> --- a/builtin/prune.c\n> +++ b/builtin/prune.c\n> @@ -127,7 +127,9 @@ static void remove_temporary_files(const char *path)\n>  \n>  \tdir = opendir(path);\n>  \tif (!dir) {\n> -\t\tfprintf(stderr, \"Unable to open directory %s\\n\", path);\n> +\t\tif (errno != ENOENT)\n> +\t\t\tfprintf(stderr, \"Unable to open directory %s: %s\\n\",\n> +\t\t\t\tpath, strerror(errno));\n\nWe sometimes use fprintf() instead of \"error\" or \"warning\" for output\ncompatibility with an older version, or because it's written in an old\nstyle.\n\nBut as you're changing the anyway let's not re-invent error_errno() or\nwarning_errno(), but just use those.\n\nWe could also s/^Unable/unable/ in the message while at it, per\nCodingGuidelines.\n\n>  \t\treturn;\n>  \t}\n>  \twhile ((de = readdir(dir)) != NULL)\n> diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\n> index 8ae314af58..d65a5f94b4 100755\n> --- a/t/t5304-prune.sh\n> +++ b/t/t5304-prune.sh\n> @@ -29,6 +29,14 @@ test_expect_success setup '\n>  \tgit gc\n>  '\n>  \n> +test_expect_success 'bare repo prune is quiet without $GIT_DIR/objects/pack' '\n> +\tgit clone -q --shared --template= --bare . bare.git &&\n> +\trmdir bare.git/objects/pack &&\n> +\tgit --git-dir=bare.git prune --no-progress 2>prune.err &&\n> +\ttest_must_be_empty prune.err &&\n> +\trm -r bare.git prune.err\n> +'\n> +\n>  test_expect_success 'prune stale packs' '\n>  \torig_pack=$(echo .git/objects/pack/*.pack) &&\n>  \t>.git/objects/tmp_1.pack &&\n\nThis seems like a good isolated change, but FWIW I think what we really\nshould be doing here is using the \"report_garbage\" facility added in\n543c5caa6c9 (count-objects: report garbage files in pack directory too,\n2013-02-15) and 478f34d2b6e (gc: remove garbage .idx files from pack\ndir, 2015-11-03) for \"pack\".\n\nI.e. we have already iterated over \"pack\" and found all the files\ntherein, and in packfile.c error_errno() etc. That we're\nre-opendir()-ing the \"pack\", walking it again etc. doesn't make much\nsense, or does it?\n\nThen the:\n\n\tremove_temporary_files(get_object_directory());\n\nAlso seems odd, just a few lines above we passed \"prune_cruft\" to\n\"for_each_loose_file_in_objdir()\", haven't we already walked the loose\nobject dir & removed temporary cruft there?\n\n\n\n"},{"id":"467683","messageId":"xmqqpmdgv4it.fsf@gitster.g","threadId":"58822","inReplyTo":"20221121104427.M268307@dcvr","subject":"Re: [PATCH] prune: quiet ENOENT on missing directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-21T13:08:42Z","receivedAt":"2022-11-21T13:08:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <e@80x24.org> writes:\n\n> Good question, perhaps this could be a followup:\n>\n> diff --git a/builtin/prune.c b/builtin/prune.c\n> index 2719220108..041c45ecbe 100644\n> --- a/builtin/prune.c\n> +++ b/builtin/prune.c\n> @@ -188,7 +188,6 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n>  \t\t\t\t      prune_cruft, prune_subdir, &revs);\n>  \n>  \tprune_packed_objects(show_only ? PRUNE_PACKED_DRY_RUN : 0);\n> -\tremove_temporary_files(get_object_directory());\n>  \ts = mkpathdup(\"%s/pack\", get_object_directory());\n>  \tremove_temporary_files(s);\n>  \tfree(s);\n\nI actually was hinting at making the remove_temporary_files()\nrecurse, so that you do not need the separate invocation in pack/\nsubdirectory.\n\nOr make 256 calls for each of the fan-out subdirectory, in which\ncase the ENOENT silencing you did would really matter and shine.\n"},{"id":"467722","messageId":"xmqqleo3vraj.fsf@gitster.g","threadId":"58822","inReplyTo":"xmqqpmdgv4it.fsf@gitster.g","subject":"Re: [PATCH] prune: quiet ENOENT on missing directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-21T23:09:08Z","receivedAt":"2022-11-21T23:09:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>>  \tprune_packed_objects(show_only ? PRUNE_PACKED_DRY_RUN : 0);\n>> -\tremove_temporary_files(get_object_directory());\n>>  \ts = mkpathdup(\"%s/pack\", get_object_directory());\n>>  \tremove_temporary_files(s);\n>>  \tfree(s);\n>\n> I actually was hinting at making the remove_temporary_files()\n> recurse, so that you do not need the separate invocation in pack/\n> subdirectory.\n>\n> Or make 256 calls for each of the fan-out subdirectory, in which\n> case the ENOENT silencing you did would really matter and shine.\n\nBut of course, neither is any part of this topic.  They are possible\nfollow-on works.\n\nThanks and sorry for making a confusing statement that could be\nmistaken as \"let's do this too\", which wasn't what I meant.\n"},{"id":"467732","messageId":"20221122000927.M873500@dcvr","threadId":"58822","inReplyTo":"xmqqleo3vraj.fsf@gitster.g","subject":"[PATCH] prune: recursively prune objects directory","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2022-11-22T00:09:27Z","receivedAt":"2022-11-22T00:09:34Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> >>  \tprune_packed_objects(show_only ? PRUNE_PACKED_DRY_RUN : 0);\n> >> -\tremove_temporary_files(get_object_directory());\n> >>  \ts = mkpathdup(\"%s/pack\", get_object_directory());\n> >>  \tremove_temporary_files(s);\n> >>  \tfree(s);\n> >\n> > I actually was hinting at making the remove_temporary_files()\n> > recurse, so that you do not need the separate invocation in pack/\n> > subdirectory.\n> >\n> > Or make 256 calls for each of the fan-out subdirectory, in which\n> > case the ENOENT silencing you did would really matter and shine.\n> \n> But of course, neither is any part of this topic.  They are possible\n> follow-on works.\n> \n> Thanks and sorry for making a confusing statement that could be\n> mistaken as \"let's do this too\", which wasn't what I meant.\n\nOh, no worries.  I already wrote this earlier and got distracted\nwith something else while waiting for tests :x.  Anyways, the\nbelow supercedes my original patch and I think it's better in\nevery way.\n\nI am unsure about duplicating ishex() from name-rev.c, however...\n\n------8<-----\nSubject: [PATCH] prune: recursively prune objects directory\n\n$GIT_DIR/objects/pack may be removed to save inodes in shared\nrepositories, so avoid scanning it if it does not exist.  Loose\nobject directories ($GIT_DIR/objects/??) may have old temporary\nfiles, so we now prune those, too.\n\nRecursion is limited to a single level since git doesn't use\ndeeper levels.  This avoids the risk of stack overflows via\ninfinite recursion when pruning untrusted repos.\n\nWe'll also emit the system error in case a directory cannot be\nopened to help users diagnose permissions problems or resource\nconstraints.\n\nSigned-off-by: Eric Wong <e@80x24.org>\n---\n builtin/prune.c  | 28 ++++++++++++++++++++--------\n t/t5304-prune.sh | 16 ++++++++++++++++\n 2 files changed, 36 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex df376b2ed1..0f6a33690a 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -114,25 +114,41 @@ static int prune_subdir(unsigned int nr, const char *path, void *data)\n \treturn 0;\n }\n \n+/*\n+ * XXX ishex is duplicated in builtin/name-rev.c, perhaps git-compat-util.h\n+ * is a better home for it\n+ */\n+#define ishex(x) (isdigit((x)) || ((x) >= 'a' && (x) <= 'f'))\n+static int is_loose_prefix(const char *d_name)\n+{\n+\treturn strlen(d_name) == 2 && ishex(d_name[0]) && ishex(d_name[1]);\n+}\n+\n /*\n  * Write errors (particularly out of space) can result in\n  * failed temporary packs (and more rarely indexes and other\n  * files beginning with \"tmp_\") accumulating in the object\n  * and the pack directories.\n  */\n-static void remove_temporary_files(const char *path)\n+static void remove_temporary_files(const char *path, int recurse)\n {\n \tDIR *dir;\n \tstruct dirent *de;\n \n \tdir = opendir(path);\n \tif (!dir) {\n-\t\tfprintf(stderr, \"Unable to open directory %s\\n\", path);\n+\t\twarning_errno(_(\"unable to open directory %s\"), path);\n \t\treturn;\n \t}\n \twhile ((de = readdir(dir)) != NULL)\n-\t\tif (starts_with(de->d_name, \"tmp_\"))\n+\t\tif (starts_with(de->d_name, \"tmp_\")) {\n \t\t\tprune_tmp_file(mkpath(\"%s/%s\", path, de->d_name));\n+\t\t} else if (recurse && (strcmp(de->d_name, \"packs\") == 0 ||\n+\t\t\t\t\tis_loose_prefix(de->d_name))) {\n+\t\t\tchar *s = mkpathdup(\"%s/%s\", path, de->d_name);\n+\t\t\tremove_temporary_files(s, 0);\n+\t\t\tfree(s);\n+\t\t}\n \tclosedir(dir);\n }\n \n@@ -150,7 +166,6 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"limit traversal to objects outside promisor packfiles\")),\n \t\tOPT_END()\n \t};\n-\tchar *s;\n \n \texpire = TIME_MAX;\n \tsave_commit_buffer = 0;\n@@ -186,10 +201,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \t\t\t\t      prune_cruft, prune_subdir, &revs);\n \n \tprune_packed_objects(show_only ? PRUNE_PACKED_DRY_RUN : 0);\n-\tremove_temporary_files(get_object_directory());\n-\ts = mkpathdup(\"%s/pack\", get_object_directory());\n-\tremove_temporary_files(s);\n-\tfree(s);\n+\tremove_temporary_files(get_object_directory(), 1);\n \n \tif (is_repository_shallow(the_repository)) {\n \t\tperform_reachability_traversal(&revs);\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 8ae314af58..8c2278035e 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -29,6 +29,22 @@ test_expect_success setup '\n \tgit gc\n '\n \n+test_expect_success 'prune stale loose objects' '\n+\tmkdir .git/objects/aa &&\n+\t>.git/objects/aa/tmp_foo &&\n+\ttest-tool chmtime =-86501 .git/objects/aa/tmp_foo &&\n+\tgit prune --expire 1.day &&\n+\ttest_path_is_missing .git/objects/aa/tmp_foo\n+'\n+\n+test_expect_success 'bare repo prune is quiet without $GIT_DIR/objects/pack' '\n+\tgit clone -q --shared --template= --bare . bare.git &&\n+\trmdir bare.git/objects/pack &&\n+\tgit --git-dir=bare.git prune --no-progress 2>prune.err &&\n+\ttest_must_be_empty prune.err &&\n+\trm -r bare.git prune.err\n+'\n+\n test_expect_success 'prune stale packs' '\n \torig_pack=$(echo .git/objects/pack/*.pack) &&\n \t>.git/objects/tmp_1.pack &&\n"},{"id":"467739","messageId":"xmqq7cznu69p.fsf@gitster.g","threadId":"58822","inReplyTo":"20221122000927.M873500@dcvr","subject":"Re: [PATCH] prune: recursively prune objects directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-22T01:28:34Z","receivedAt":"2022-11-22T01:28:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <e@80x24.org> writes:\n\n> I am unsure about duplicating ishex() from name-rev.c, however...\n\nYeah, I wonder why name-rev.c does not use isxdigit() in the first\nplace.\n\n> ------8<-----\n> Subject: [PATCH] prune: recursively prune objects directory\n>\n> $GIT_DIR/objects/pack may be removed to save inodes in shared\n> repositories, so avoid scanning it if it does not exist.  Loose\n> object directories ($GIT_DIR/objects/??) may have old temporary\n> files, so we now prune those, too.\n>\n> Recursion is limited to a single level since git doesn't use\n> deeper levels.  This avoids the risk of stack overflows via\n> infinite recursion when pruning untrusted repos.\n>\n> We'll also emit the system error in case a directory cannot be\n> opened to help users diagnose permissions problems or resource\n> constraints.\n>\n> Signed-off-by: Eric Wong <e@80x24.org>\n> ---\n>  builtin/prune.c  | 28 ++++++++++++++++++++--------\n>  t/t5304-prune.sh | 16 ++++++++++++++++\n>  2 files changed, 36 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/prune.c b/builtin/prune.c\n> index df376b2ed1..0f6a33690a 100644\n> --- a/builtin/prune.c\n> +++ b/builtin/prune.c\n> @@ -114,25 +114,41 @@ static int prune_subdir(unsigned int nr, const char *path, void *data)\n>  \treturn 0;\n>  }\n>  \n> +/*\n> + * XXX ishex is duplicated in builtin/name-rev.c, perhaps git-compat-util.h\n> + * is a better home for it\n> + */\n> +#define ishex(x) (isdigit((x)) || ((x) >= 'a' && (x) <= 'f'))\n> +static int is_loose_prefix(const char *d_name)\n> +{\n> +\treturn strlen(d_name) == 2 && ishex(d_name[0]) && ishex(d_name[1]);\n> +}\n> +\n>  /*\n>   * Write errors (particularly out of space) can result in\n>   * failed temporary packs (and more rarely indexes and other\n>   * files beginning with \"tmp_\") accumulating in the object\n>   * and the pack directories.\n>   */\n> -static void remove_temporary_files(const char *path)\n> +static void remove_temporary_files(const char *path, int recurse)\n>  {\n>  \tDIR *dir;\n>  \tstruct dirent *de;\n>  \n>  \tdir = opendir(path);\n>  \tif (!dir) {\n> -\t\tfprintf(stderr, \"Unable to open directory %s\\n\", path);\n> +\t\twarning_errno(_(\"unable to open directory %s\"), path);\n>  \t\treturn;\n>  \t}\n>  \twhile ((de = readdir(dir)) != NULL)\n> -\t\tif (starts_with(de->d_name, \"tmp_\"))\n> +\t\tif (starts_with(de->d_name, \"tmp_\")) {\n>  \t\t\tprune_tmp_file(mkpath(\"%s/%s\", path, de->d_name));\n> +\t\t} else if (recurse && (strcmp(de->d_name, \"packs\") == 0 ||\n> +\t\t\t\t\tis_loose_prefix(de->d_name))) {\n\nOK, the intent is to be careful and deal only with the fan-out\ndirectories objects/[0-9a-f]{2}/ and objects/pack/ and leave crufts\nin objects/info and any other unknown subdirectories, which makes\nsense.\n\nTwo nits are:\n\n - \"packs\" wants to be \"pack\".\n - \"strcmp() == 0\" wants to be \"!strcmp()\".\n\n> +\t\t\tchar *s = mkpathdup(\"%s/%s\", path, de->d_name);\n> +\t\t\tremove_temporary_files(s, 0);\n> +\t\t\tfree(s);\n> +\t\t}\n>  \tclosedir(dir);\n>  }\n>  \n> @@ -150,7 +166,6 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n>  \t\t\t N_(\"limit traversal to objects outside promisor packfiles\")),\n>  \t\tOPT_END()\n>  \t};\n> -\tchar *s;\n>  \n>  \texpire = TIME_MAX;\n>  \tsave_commit_buffer = 0;\n> @@ -186,10 +201,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n>  \t\t\t\t      prune_cruft, prune_subdir, &revs);\n>  \n>  \tprune_packed_objects(show_only ? PRUNE_PACKED_DRY_RUN : 0);\n> -\tremove_temporary_files(get_object_directory());\n> -\ts = mkpathdup(\"%s/pack\", get_object_directory());\n> -\tremove_temporary_files(s);\n> -\tfree(s);\n> +\tremove_temporary_files(get_object_directory(), 1);\n>  \n>  \tif (is_repository_shallow(the_repository)) {\n>  \t\tperform_reachability_traversal(&revs);\n> diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\n> index 8ae314af58..8c2278035e 100755\n> --- a/t/t5304-prune.sh\n> +++ b/t/t5304-prune.sh\n> @@ -29,6 +29,22 @@ test_expect_success setup '\n>  \tgit gc\n>  '\n>  \n> +test_expect_success 'prune stale loose objects' '\n> +\tmkdir .git/objects/aa &&\n> +\t>.git/objects/aa/tmp_foo &&\n> +\ttest-tool chmtime =-86501 .git/objects/aa/tmp_foo &&\n> +\tgit prune --expire 1.day &&\n> +\ttest_path_is_missing .git/objects/aa/tmp_foo\n> +'\n> +\n> +test_expect_success 'bare repo prune is quiet without $GIT_DIR/objects/pack' '\n> +\tgit clone -q --shared --template= --bare . bare.git &&\n> +\trmdir bare.git/objects/pack &&\n> +\tgit --git-dir=bare.git prune --no-progress 2>prune.err &&\n> +\ttest_must_be_empty prune.err &&\n> +\trm -r bare.git prune.err\n> +'\n\nIs the last \"clean-up\" step necessary?\n\n> +\n>  test_expect_success 'prune stale packs' '\n>  \torig_pack=$(echo .git/objects/pack/*.pack) &&\n>  \t>.git/objects/tmp_1.pack &&\n\nOther than that, looks like a good idea.\n\nThanks.\n"},{"id":"467757","messageId":"20221122095943.M478967@dcvr","threadId":"58822","inReplyTo":"xmqq7cznu69p.fsf@gitster.g","subject":"Re: [PATCH] prune: recursively prune objects directory","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2022-11-22T09:59:43Z","receivedAt":"2022-11-22T09:59:48Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Wong <e@80x24.org> writes:\n> \n> > I am unsure about duplicating ishex() from name-rev.c, however...\n> \n> Yeah, I wonder why name-rev.c does not use isxdigit() in the first\n> place.\n\nisxdigit includes uppercase [A-F].  I think being strict is\nbetter, here.  I don't want to open up a can of worms if we\nbecome tolerant of 3rd-party git implementations developed on\ncase-insensitive FSes.\n\n> > -static void remove_temporary_files(const char *path)\n> > +static void remove_temporary_files(const char *path, int recurse)\n> >  {\n> >  \tDIR *dir;\n> >  \tstruct dirent *de;\n> >  \n> >  \tdir = opendir(path);\n> >  \tif (!dir) {\n> > -\t\tfprintf(stderr, \"Unable to open directory %s\\n\", path);\n> > +\t\twarning_errno(_(\"unable to open directory %s\"), path);\n> >  \t\treturn;\n> >  \t}\n> >  \twhile ((de = readdir(dir)) != NULL)\n> > -\t\tif (starts_with(de->d_name, \"tmp_\"))\n> > +\t\tif (starts_with(de->d_name, \"tmp_\")) {\n> >  \t\t\tprune_tmp_file(mkpath(\"%s/%s\", path, de->d_name));\n> > +\t\t} else if (recurse && (strcmp(de->d_name, \"packs\") == 0 ||\n> > +\t\t\t\t\tis_loose_prefix(de->d_name))) {\n> \n> OK, the intent is to be careful and deal only with the fan-out\n> directories objects/[0-9a-f]{2}/ and objects/pack/ and leave crufts\n> in objects/info and any other unknown subdirectories, which makes\n> sense.\n> \n> Two nits are:\n> \n>  - \"packs\" wants to be \"pack\".\n\nOK, fixed.  Along with existing test cases, since packs handling\nwasn't being tested properly.\n\n>  - \"strcmp() == 0\" wants to be \"!strcmp()\".\n\nOK\n\n> > diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\n> > index 8ae314af58..8c2278035e 100755\n> > --- a/t/t5304-prune.sh\n> > +++ b/t/t5304-prune.sh\n> > @@ -29,6 +29,22 @@ test_expect_success setup '\n> >  \tgit gc\n> >  '\n> >  \n> > +test_expect_success 'prune stale loose objects' '\n> > +\tmkdir .git/objects/aa &&\n> > +\t>.git/objects/aa/tmp_foo &&\n> > +\ttest-tool chmtime =-86501 .git/objects/aa/tmp_foo &&\n> > +\tgit prune --expire 1.day &&\n> > +\ttest_path_is_missing .git/objects/aa/tmp_foo\n> > +'\n> > +\n> > +test_expect_success 'bare repo prune is quiet without $GIT_DIR/objects/pack' '\n> > +\tgit clone -q --shared --template= --bare . bare.git &&\n> > +\trmdir bare.git/objects/pack &&\n> > +\tgit --git-dir=bare.git prune --no-progress 2>prune.err &&\n> > +\ttest_must_be_empty prune.err &&\n> > +\trm -r bare.git prune.err\n> > +'\n> \n> Is the last \"clean-up\" step necessary?\n\nGuess not, removed in v2 below.\n\n> > +\n> >  test_expect_success 'prune stale packs' '\n> >  \torig_pack=$(echo .git/objects/pack/*.pack) &&\n> >  \t>.git/objects/tmp_1.pack &&\n> \n> Other than that, looks like a good idea.\n\n'prune stale packs' was actually insufficient for catching\nthe extraneous `s' in `pack'.  I've kept existing checks against\nobjects/tmp_*, but added extra checks for objects/pack/tmp_*\n\nv2 fixes:\n* `pack' directory fixed, tests added\n* !strcmp\n* remove needless cleanup step in test\n\n-----8<-----\nSubject: [PATCH] prune: recursively prune objects directory\n\n$GIT_DIR/objects/pack may be removed to save inodes in shared\nrepositories, so avoid scanning it if it does not exist.  Loose\nobject directories ($GIT_DIR/objects/??) may have old temporary\nfiles, so we now prune those, too.\n\nRecursion is limited to a single level since git doesn't use\ndeeper levels.  This avoids the risk of stack overflows via\ninfinite recursion when pruning untrusted repos.\n\nWe'll also emit the system error in case a directory cannot be\nopened to help users diagnose permissions problems or resource\nconstraints.\n\nSigned-off-by: Eric Wong <e@80x24.org>\n---\nInterdiff:\n  diff --git a/builtin/prune.c b/builtin/prune.c\n  index 0f6a33690a..a05f1a2704 100644\n  --- a/builtin/prune.c\n  +++ b/builtin/prune.c\n  @@ -143,7 +143,7 @@ static void remove_temporary_files(const char *path, int recurse)\n   \twhile ((de = readdir(dir)) != NULL)\n   \t\tif (starts_with(de->d_name, \"tmp_\")) {\n   \t\t\tprune_tmp_file(mkpath(\"%s/%s\", path, de->d_name));\n  -\t\t} else if (recurse && (strcmp(de->d_name, \"packs\") == 0 ||\n  +\t\t} else if (recurse && (!strcmp(de->d_name, \"pack\") ||\n   \t\t\t\t\tis_loose_prefix(de->d_name))) {\n   \t\t\tchar *s = mkpathdup(\"%s/%s\", path, de->d_name);\n   \t\t\tremove_temporary_files(s, 0);\n  diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\n  index 8c2278035e..64d5f4e5b3 100755\n  --- a/t/t5304-prune.sh\n  +++ b/t/t5304-prune.sh\n  @@ -41,19 +41,23 @@ test_expect_success 'bare repo prune is quiet without $GIT_DIR/objects/pack' '\n   \tgit clone -q --shared --template= --bare . bare.git &&\n   \trmdir bare.git/objects/pack &&\n   \tgit --git-dir=bare.git prune --no-progress 2>prune.err &&\n  -\ttest_must_be_empty prune.err &&\n  -\trm -r bare.git prune.err\n  +\ttest_must_be_empty prune.err\n   '\n   \n   test_expect_success 'prune stale packs' '\n   \torig_pack=$(echo .git/objects/pack/*.pack) &&\n   \t>.git/objects/tmp_1.pack &&\n   \t>.git/objects/tmp_2.pack &&\n  -\ttest-tool chmtime =-86501 .git/objects/tmp_1.pack &&\n  +\t>.git/objects/pack/tmp_3.pack &&\n  +\t>.git/objects/pack/tmp_4.pack &&\n  +\ttest-tool chmtime =-86501 .git/objects/tmp_1.pack \\\n  +\t\t.git/objects/pack/tmp_3.pack &&\n   \tgit prune --expire 1.day &&\n   \ttest_path_is_file $orig_pack &&\n   \ttest_path_is_file .git/objects/tmp_2.pack &&\n  -\ttest_path_is_missing .git/objects/tmp_1.pack\n  +\ttest_path_is_file .git/objects/pack/tmp_4.pack &&\n  +\ttest_path_is_missing .git/objects/tmp_1.pack &&\n  +\ttest_path_is_missing .git/objects/pack/tmp_3.pack\n   '\n   \n   test_expect_success 'prune --expire' '\n\n builtin/prune.c  | 28 ++++++++++++++++++++--------\n t/t5304-prune.sh | 24 ++++++++++++++++++++++--\n 2 files changed, 42 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex df376b2ed1..a05f1a2704 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -114,25 +114,41 @@ static int prune_subdir(unsigned int nr, const char *path, void *data)\n \treturn 0;\n }\n \n+/*\n+ * XXX ishex is duplicated in builtin/name-rev.c, perhaps git-compat-util.h\n+ * is a better home for it\n+ */\n+#define ishex(x) (isdigit((x)) || ((x) >= 'a' && (x) <= 'f'))\n+static int is_loose_prefix(const char *d_name)\n+{\n+\treturn strlen(d_name) == 2 && ishex(d_name[0]) && ishex(d_name[1]);\n+}\n+\n /*\n  * Write errors (particularly out of space) can result in\n  * failed temporary packs (and more rarely indexes and other\n  * files beginning with \"tmp_\") accumulating in the object\n  * and the pack directories.\n  */\n-static void remove_temporary_files(const char *path)\n+static void remove_temporary_files(const char *path, int recurse)\n {\n \tDIR *dir;\n \tstruct dirent *de;\n \n \tdir = opendir(path);\n \tif (!dir) {\n-\t\tfprintf(stderr, \"Unable to open directory %s\\n\", path);\n+\t\twarning_errno(_(\"unable to open directory %s\"), path);\n \t\treturn;\n \t}\n \twhile ((de = readdir(dir)) != NULL)\n-\t\tif (starts_with(de->d_name, \"tmp_\"))\n+\t\tif (starts_with(de->d_name, \"tmp_\")) {\n \t\t\tprune_tmp_file(mkpath(\"%s/%s\", path, de->d_name));\n+\t\t} else if (recurse && (!strcmp(de->d_name, \"pack\") ||\n+\t\t\t\t\tis_loose_prefix(de->d_name))) {\n+\t\t\tchar *s = mkpathdup(\"%s/%s\", path, de->d_name);\n+\t\t\tremove_temporary_files(s, 0);\n+\t\t\tfree(s);\n+\t\t}\n \tclosedir(dir);\n }\n \n@@ -150,7 +166,6 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"limit traversal to objects outside promisor packfiles\")),\n \t\tOPT_END()\n \t};\n-\tchar *s;\n \n \texpire = TIME_MAX;\n \tsave_commit_buffer = 0;\n@@ -186,10 +201,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \t\t\t\t      prune_cruft, prune_subdir, &revs);\n \n \tprune_packed_objects(show_only ? PRUNE_PACKED_DRY_RUN : 0);\n-\tremove_temporary_files(get_object_directory());\n-\ts = mkpathdup(\"%s/pack\", get_object_directory());\n-\tremove_temporary_files(s);\n-\tfree(s);\n+\tremove_temporary_files(get_object_directory(), 1);\n \n \tif (is_repository_shallow(the_repository)) {\n \t\tperform_reachability_traversal(&revs);\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 8ae314af58..64d5f4e5b3 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -29,15 +29,35 @@ test_expect_success setup '\n \tgit gc\n '\n \n+test_expect_success 'prune stale loose objects' '\n+\tmkdir .git/objects/aa &&\n+\t>.git/objects/aa/tmp_foo &&\n+\ttest-tool chmtime =-86501 .git/objects/aa/tmp_foo &&\n+\tgit prune --expire 1.day &&\n+\ttest_path_is_missing .git/objects/aa/tmp_foo\n+'\n+\n+test_expect_success 'bare repo prune is quiet without $GIT_DIR/objects/pack' '\n+\tgit clone -q --shared --template= --bare . bare.git &&\n+\trmdir bare.git/objects/pack &&\n+\tgit --git-dir=bare.git prune --no-progress 2>prune.err &&\n+\ttest_must_be_empty prune.err\n+'\n+\n test_expect_success 'prune stale packs' '\n \torig_pack=$(echo .git/objects/pack/*.pack) &&\n \t>.git/objects/tmp_1.pack &&\n \t>.git/objects/tmp_2.pack &&\n-\ttest-tool chmtime =-86501 .git/objects/tmp_1.pack &&\n+\t>.git/objects/pack/tmp_3.pack &&\n+\t>.git/objects/pack/tmp_4.pack &&\n+\ttest-tool chmtime =-86501 .git/objects/tmp_1.pack \\\n+\t\t.git/objects/pack/tmp_3.pack &&\n \tgit prune --expire 1.day &&\n \ttest_path_is_file $orig_pack &&\n \ttest_path_is_file .git/objects/tmp_2.pack &&\n-\ttest_path_is_missing .git/objects/tmp_1.pack\n+\ttest_path_is_file .git/objects/pack/tmp_4.pack &&\n+\ttest_path_is_missing .git/objects/tmp_1.pack &&\n+\ttest_path_is_missing .git/objects/pack/tmp_3.pack\n '\n \n test_expect_success 'prune --expire' '\n"},{"id":"467826","messageId":"xmqqbkoyr35p.fsf@gitster.g","threadId":"58822","inReplyTo":"20221122095943.M478967@dcvr","subject":"Re: [PATCH] prune: recursively prune objects directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-22T23:16:18Z","receivedAt":"2022-11-22T23:16:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <e@80x24.org> writes:\n\n> Junio C Hamano <gitster@pobox.com> wrote:\n>> Eric Wong <e@80x24.org> writes:\n>> \n>> > I am unsure about duplicating ishex() from name-rev.c, however...\n>> \n>> Yeah, I wonder why name-rev.c does not use isxdigit() in the first\n>> place.\n>\n> isxdigit includes uppercase [A-F].  I think being strict is\n> better, here.  I don't want to open up a can of worms if we\n> become tolerant of 3rd-party git implementations developed on\n> case-insensitive FSes.\n\nOK, we do not recurse into .git/objects/AA/ for the same reason why\nwe do not recurse into .git/objects/info/.  We do expect [0-9a-f]{2}\nand pack to be directories, so we go silent if they are missing, but\nwe do complain if somebody creates a regular file .git/objects/aa\nfor fun.\n\nI agree that isxdigit() is not a good match.  I also agree with what\nyou said about it belong to git-compat-util.h but let's leave it for\na future clean-up patch to remove both copies of ishex(), introduce\nislxdigit() in git-compat-util.h and use it as its replacement.\n\n"}]}