Re: [PATCH v4 09/19] entry: add checkout_entry_ca() which takes preloaded conv_attrs

From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
Date: Dec 7, 2020, 16:47 UTC
Message-ID: <CAHd-oW5beC6bM-0SabCAry93S8tDL6uXD8-C6MAWj67Ss20xbQ@mail.gmail.com>
In-Reply-To: <CAP8UFD3SpzGbkeBas+yDEjDgtDiTdztD=6fMfG=8rHKN5B6kCQ@mail.gmail.com>

On Sun, Dec 6, 2020 at 7:03 AM Christian Couder <christian.couder@gmail.com> wrote:

Show 19 quoted lines

>
> On Wed, Nov 4, 2020 at 9:34 PM Matheus Tavares
> <matheus.bernardino@usp.br> wrote:
>
> The title might be a bit shorter with: s/which takes/taking/ or
> s/which takes/using/
>
> > @@ -530,12 +531,12 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state,
> >         if (nr_checkouts)
> >                 (*nr_checkouts)++;
> >
> > -       if (S_ISREG(ce->ce_mode)) {
> > +       if (S_ISREG(ce->ce_mode) && !ca) {
> >                 convert_attrs(state->istate, &ca_buf, ce->name);
> >                 ca = &ca_buf;
> >         }
>
> I wonder if it's possible to avoid repeating the above 4 lines twice
> in this function.

Yeah, I was thinking about that as well. But the only way I found to avoid the repetition would be to place this block before check_path(). The problem is that we would end up calling convert_attrs() even for the cases where we later decide not to checkout the entry (because, for example, state->not_new is true or the file is already up to date).

Show 5 quoted lines

> > -       return write_entry(ce, path.buf, NULL, state, 0);
> > +       return write_entry(ce, path.buf, ca, state, 0);
>
> It's good that ca is eventually passed here, but it might belong to
> the previous patch.

Right, I'll fix the previous patch.

Show 6 quoted lines

> > -int checkout_entry(struct cache_entry *ce, const struct checkout *state,
> > -                  char *topath, int *nr_checkouts);
> > +#define checkout_entry(ce, state, topath, nr_checkouts) \
> > +               checkout_entry_ca(ce, NULL, state, topath, nr_checkouts)
>
> I thought that we prefer inline functions over macros when possible.

Thanks, I didn't know about the preference. I'll change that.