Re: [PATCH v9 1/9] tmp-objdir: new API for creating temporary writable databases

>
> On Mon, Nov 15, 2021 at 3:51 PM Neeraj Singh via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: Neeraj Singh <neerajsi@microsoft.com>
> >
> > The tmp_objdir API provides the ability to create temporary object
> > directories, but was designed with the goal of having subprocesses
> > access these object stores, followed by the main process migrating
> > objects from it to the main object store or just deleting it.  The
> > subprocesses would view it as their primary datastore and write to it.
> >
> > Here we add the tmp_objdir_replace_primary_odb function that replaces
> > the current process's writable "main" object directory with the
> > specified one. The previous main object directory is restored in either
> > tmp_objdir_migrate or tmp_objdir_destroy.
> >
> > For the --remerge-diff usecase, add a new `will_destroy` flag in `struct
> > object_database` to mark ephemeral object databases that do not require
> > fsync durability.
> >
> > Add 'git prune' support for removing temporary object databases, and
> > make sure that they have a name starting with tmp_ and containing an
> > operation-specific name.
> >
> > Based-on-patch-by: Elijah Newren <newren@gmail.com>
> >
> > Signed-off-by: Neeraj Singh <neerajsi@microsoft.com>
> > Signed-off-by: Junio C Hamano <gitster@pobox.com>
> > ---
> >  builtin/prune.c        | 23 +++++++++++++++---
> >  builtin/receive-pack.c |  2 +-
> >  environment.c          |  5 ++++
> >  object-file.c          | 44 +++++++++++++++++++++++++++++++--
> >  object-store.h         | 19 +++++++++++++++
> >  object.c               |  2 +-
> >  tmp-objdir.c           | 55 +++++++++++++++++++++++++++++++++++++++---
> >  tmp-objdir.h           | 29 +++++++++++++++++++---
> >  8 files changed, 165 insertions(+), 14 deletions(-)
> >
> > diff --git a/builtin/prune.c b/builtin/prune.c
> > index 485c9a3c56f..a76e6a5f0e8 100644
> > --- a/builtin/prune.c
> > +++ b/builtin/prune.c
> > @@ -18,6 +18,7 @@ static int show_only;
> >  static int verbose;
> >  static timestamp_t expire;
> >  static int show_progress = -1;
> > +static struct strbuf remove_dir_buf = STRBUF_INIT;
> >
> >  static int prune_tmp_file(const char *fullpath)
> >  {
> > @@ -26,10 +27,20 @@ static int prune_tmp_file(const char *fullpath)
> >                 return error("Could not stat '%s'", fullpath);
> >         if (st.st_mtime > expire)
> >                 return 0;
> > -       if (show_only || verbose)
> > -               printf("Removing stale temporary file %s\n", fullpath);
> > -       if (!show_only)
> > -               unlink_or_warn(fullpath);
> > +       if (S_ISDIR(st.st_mode)) {
> > +               if (show_only || verbose)
> > +                       printf("Removing stale temporary directory %s\n", fullpath);
> > +               if (!show_only) {
> > +                       strbuf_reset(&remove_dir_buf);
> > +                       strbuf_addstr(&remove_dir_buf, fullpath);
> > +                       remove_dir_recursively(&remove_dir_buf, 0);
>
> Why not just define remove_dir_buf here rather than as a global?  It'd
> not only make the code more readable by keeping everything localized,
> it would have prevented the forgotten strbuf_reset() bug from the
> earlier round of this patch.
>
> Sure, that'd be an extra memory allocation/free for each directory you
> hit, which should be negligible compared to the cost of
> remove_dir_recursively()...and I'm not sure this is performance
> critical anyway (I don't see why we'd expect more than O(1) cruft
> temporary directories).

> > +               }
> > +       } else {
> > +               if (show_only || verbose)
> > +                       printf("Removing stale temporary file %s\n", fullpath);
> > +               if (!show_only)
> > +                       unlink_or_warn(fullpath);
> > +       }
> >         return 0;
> >  }
> >
> > @@ -97,6 +108,9 @@ static int prune_cruft(const char *basename, const char *path, void *data)
> >
> >  static int prune_subdir(unsigned int nr, const char *path, void *data)
> >  {
> > +       if (verbose)
>
> Shouldn't this be
>     if (show_only || verbose)
> ?