On Tue, Nov 30, 2021 at 1:52 PM Neeraj Singh <nksingh85@xxxxxxxxx> wrote:
>
> On Tue, Nov 30, 2021 at 1:27 PM Elijah Newren <newren@xxxxxxxxx> wrote:
> >
> > On Mon, Nov 15, 2021 at 3:51 PM Neeraj Singh via GitGitGadget
> > <gitgitgadget@xxxxxxxxx> wrote:
> > >
> > > From: Neeraj Singh <neerajsi@xxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxx>
> > >
> > > Signed-off-by: Neeraj Singh <neerajsi@xxxxxxxxxxxxx>
> > > Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
> > > ---
> > > 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.
