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

[Elijah Newren <newren@gmail.com>]

On Tue, Nov 30, 2021 at 1:52 PM Neeraj Singh <nksingh85@gmail.com> wrote:
>
> On Tue, Nov 30, 2021 at 1:27 PM Elijah Newren <newren@gmail.com> wrote:
> >
> > 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).
>
> I'll take this suggestion.
>
> > > +               }
> > > +       } 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)
> > ?
>
> Doing that breaks one of the tests, since we print extra stuff that's
> unexpected. I think I'm going to just revert this change, since it
> appears that we call this callback and try to remove the directory
> even if it's non-empty.

Makes sense.

> Do you have any comments or thoughts on how we want to allow the user
> to configure fsync settings?

I don't; sorry.