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

On Tue, Nov 30, 2021 at 1:27 PM Elijah Newren [off-list ref] wrote:

On Mon, Nov 15, 2021 at 3:51 PM Neeraj Singh via GitGitGadget
[off-list ref] wrote:

quoted

From: Neeraj Singh <redacted>

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 [off-list ref]

Signed-off-by: Neeraj Singh <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 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.

quoted

+               }
+       } 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