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.
Do you have any comments or thoughts on how we want to allow the user
to configure fsync settings?
Thanks,
Neeraj
