Re: [PATCH v2 08/19] entry: move conv_attrs lookup up to checkout_entry()

[Jeff Hostetler <git@xxxxxxxxxxxxxxxxx>]

On 10/1/20 11:53 AM, Jeff Hostetler wrote:
> On 9/22/20 6:49 PM, Matheus Tavares wrote:
> > In a following patch, checkout_entry() will use conv_attrs to decide
> > whether an entry should be enqueued for parallel checkout or not. But
> > the attributes lookup only happens lower in this call stack. To avoid
> > the unnecessary work of loading the attributes twice, let's move it up
> > to checkout_entry(), and pass the loaded struct down to write_entry().
> >
> > Signed-off-by: Matheus Tavares <matheus.bernardino@xxxxxx>
> > ---
> >   entry.c | 38 +++++++++++++++++++++++++++-----------
> >   1 file changed, 27 insertions(+), 11 deletions(-)
> >
> > diff --git a/entry.c b/entry.c
> > index 1d2df188e5..8237859b12 100644
> > --- a/entry.c
> > +++ b/entry.c
> > @@ -263,8 +263,9 @@ void update_ce_after_write(const struct checkout 
> > *state, struct cache_entry *ce,
> >       }
> >   }
> > -static int write_entry(struct cache_entry *ce,
> > -               char *path, const struct checkout *state, int 
> > to_tempfile)
> > +/* Note: ca is used (and required) iff the entry refers to a regular 
> > file. */
> > +static int write_entry(struct cache_entry *ce, char *path, struct 
> > conv_attrs *ca,
> > +               const struct checkout *state, int to_tempfile)
> >   {
> >       unsigned int ce_mode_s_ifmt = ce->ce_mode & S_IFMT;
> >       struct delayed_checkout *dco = state->delayed_checkout;
> > @@ -281,8 +282,7 @@ static int write_entry(struct cache_entry *ce,
> >       clone_checkout_metadata(&meta, &state->meta, &ce->oid);
> >       if (ce_mode_s_ifmt == S_IFREG) {
> > -        struct stream_filter *filter = 
> > get_stream_filter(state->istate, ce->name,
> > -                                 &ce->oid);
> > +        struct stream_filter *filter = get_stream_filter_ca(ca, 
> > &ce->oid);
> >           if (filter &&
> >               !streaming_write_entry(ce, path, filter,
> >                          state, to_tempfile,
> > @@ -329,14 +329,17 @@ static int write_entry(struct cache_entry *ce,
> >            * Convert from git internal format to working tree format
> >            */
> >           if (dco && dco->state != CE_NO_DELAY) {
> > -            ret = async_convert_to_working_tree(state->istate, 
> > ce->name, new_blob,
> > -                                size, &buf, &meta, dco);
> > +            ret = async_convert_to_working_tree_ca(ca, ce->name,
> > +                                   new_blob, size,
> > +                                   &buf, &meta, dco);
> >               if (ret && string_list_has_string(&dco->paths, ce->name)) {
> >                   free(new_blob);
> >                   goto delayed;
> >               }
> > -        } else
> > -            ret = convert_to_working_tree(state->istate, ce->name, 
> > new_blob, size, &buf, &meta);
> > +        } else {
> > +            ret = convert_to_working_tree_ca(ca, ce->name, new_blob,
> > +                             size, &buf, &meta);
> > +        }
> >           if (ret) {
> >               free(new_blob);
> > @@ -442,6 +445,7 @@ int checkout_entry(struct cache_entry *ce, const 
> > struct checkout *state,
> >   {
> >       static struct strbuf path = STRBUF_INIT;
> >       struct stat st;
> > +    struct conv_attrs ca;
>
> I have to wonder if it would be clearer to move this declaration of `ca`
> into the two `if { ... }` blocks where it is used -- to indicate that it
> is only defined in two cases where we call `convert_attrs()`.
>
> There are several other calls to `write_entry()` that pass NULL and it
> could cause confusion.


Nevermind, I see what you did in step 9 and this makes sense.

> >       if (ce->ce_flags & CE_WT_REMOVE) {
> >           if (topath)
> > @@ -454,8 +458,13 @@ int checkout_entry(struct cache_entry *ce, const 
> > struct checkout *state,
> >           return 0;
> >       }
> > -    if (topath)
> > -        return write_entry(ce, topath, state, 1);
> > +    if (topath) {
> > +        if (S_ISREG(ce->ce_mode)) {
> > +            convert_attrs(state->istate, &ca, ce->name);
> > +            return write_entry(ce, topath, &ca, state, 1);
> > +        }
> > +        return write_entry(ce, topath, NULL, state, 1);
> > +    }
> >       strbuf_reset(&path);
> >       strbuf_add(&path, state->base_dir, state->base_dir_len);
> > @@ -517,9 +526,16 @@ int checkout_entry(struct cache_entry *ce, const 
> > struct checkout *state,
> >           return 0;
> >       create_directories(path.buf, path.len, state);
> > +
> >       if (nr_checkouts)
> >           (*nr_checkouts)++;
> > -    return write_entry(ce, path.buf, state, 0);
> > +
> > +    if (S_ISREG(ce->ce_mode)) {
> > +        convert_attrs(state->istate, &ca, ce->name);
> > +        return write_entry(ce, path.buf, &ca, state, 0);
> > +    }
> > +
> > +    return write_entry(ce, path.buf, NULL, state, 0);
> >   }
> >   void unlink_entry(const struct cache_entry *ce)