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

On Wed, Sep 29, 2021 at 1:42 AM Elijah Newren [off-list ref] wrote:

Hi,

Thanks for working on this, and for moving this up in your series near
the beginning.

On Tue, Sep 28, 2021 at 4:34 PM Neeraj Singh via GitGitGadget
[off-list ref] wrote:

quoted

From: Neeraj Singh <redacted>

This patch is based on work by Elijah Newren. Any bugs however are my
own.

This kind of information is often included in a commit message via a
trailer such as:
    Based-on-patch-by: Elijah Newren [off-list ref]
or Helped-by: or Co-authored-by: or Contributions-by: .

Will fix. I didn't know what some acceptable trailers were.  I'll use:
Based-on-patch-by: Elijah Newren [off-list ref]

quoted

+struct object_directory *set_temporary_primary_odb(const char *dir, int will_destroy)
+{
+       struct object_directory *new_odb;
+
+       /*
+        * Make sure alternates are initialized, or else our entry may be
+        * overwritten when they are.
+        */
+       prepare_alt_odb(the_repository);

This implicit dependence on the_repository is unfortunate.  My
versions passed the repository parameter explicitly.  While my
remerge-diff code doesn't really make use of that currently, it could
make sense to have temporary object stores for a submodule and do
remerge-diff work on them.  You've also got two more uses of
the_repository later in this function.

The core loose object code in object-file.c is riven with
the_repository assumptions. I'd have to refactor that code (including
the alternates code) to take repository arguments.  Given the
extensive assumptions, I'd like to push back on this suggestion and
all of the related suggestions.

quoted

diff --git a/object-store.h b/object-store.h
index 551639f173d..5bc9da6634e 100644
--- a/object-store.h
+++ b/object-store.h

@@ -31,7 +31,12 @@ struct object_directory {
         * This is a temporary object store, so there is no need to
         * create new objects via rename.
         */
-       int is_temp;
+       int is_temp : 8;
+
+       /*
+        * This object store is ephemeral, so there is no need to fsync.
+        */
+       int will_destroy : 8;

Why 8 bits wide rather than 1?  I thought these were boolean
values...was I mistaken?

(Also, if boolean and compressing to 1 bit, should probably be
unsigned rather than signed.)

This will go away when I drop the rename patch.  I wish we had a
standard bool_t type which is one char wide.  This is a
microoptimization, since accessing bits usually encodes to more or
larger instructions than accessing bytes.

quoted

+        */
+       strbuf_addf(&t->path, "%s/tmp_objdir-%s-XXXXXX", get_object_directory(), prefix);

        /*
         * Grow the strbuf beyond any filename we expect to be placed in it.

@@ -269,6 +279,15 @@ int tmp_objdir_migrate(struct tmp_objdir *t)
        if (!t)
                return 0;

+
+

Why so many blank lines?

This was an accident, will remove.

Other than those minor things, I couldn't find any problems.

Thanks for the review!