On Wed, Sep 29, 2021 at 1:42 AM Elijah Newren <newren@xxxxxxxxx> 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
> <gitgitgadget@xxxxxxxxx> wrote:
> >
> > From: Neeraj Singh <neerajsi@xxxxxxxxxxxxx>
> >
> > 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 <newren@xxxxxxxxx>
> 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 <newren@xxxxxxxxx>
> >
> > +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.
> > 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.
> > + */
> > + 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!
