| Message ID | aa635bda21c43e0d82ce21e791df280ee6231d43.1604521275.git.matheus.bernardino@usp.br (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Parallel Checkout (part I) | expand |
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. > - 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. > -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.
On Sun, Dec 6, 2020 at 7:03 AM Christian Couder <christian.couder@gmail.com> wrote: > > 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). > > - 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. > > -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.
diff --git a/entry.c b/entry.c index 486712c3a9..9d79a5671f 100644 --- a/entry.c +++ b/entry.c @@ -440,12 +440,13 @@ static void mark_colliding_entries(const struct checkout *state, } } -int checkout_entry(struct cache_entry *ce, const struct checkout *state, - char *topath, int *nr_checkouts) +int checkout_entry_ca(struct cache_entry *ce, struct conv_attrs *ca, + const struct checkout *state, char *topath, + int *nr_checkouts) { static struct strbuf path = STRBUF_INIT; struct stat st; - struct conv_attrs ca_buf, *ca = NULL; + struct conv_attrs ca_buf; if (ce->ce_flags & CE_WT_REMOVE) { if (topath) @@ -459,7 +460,7 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, } if (topath) { - if (S_ISREG(ce->ce_mode)) { + if (S_ISREG(ce->ce_mode) && !ca) { convert_attrs(state->istate, &ca_buf, ce->name); ca = &ca_buf; } @@ -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; } - return write_entry(ce, path.buf, NULL, state, 0); + return write_entry(ce, path.buf, ca, state, 0); } void unlink_entry(const struct cache_entry *ce) diff --git a/entry.h b/entry.h index ea7290bcd5..d8244c5db2 100644 --- a/entry.h +++ b/entry.h @@ -26,9 +26,17 @@ struct checkout { * file named by ce, a temporary file is created by this function and * its name is returned in topath[], which must be able to hold at * least TEMPORARY_FILENAME_LENGTH bytes long. + * + * With checkout_entry_ca(), callers can optionally pass a preloaded + * conv_attrs struct (to avoid reloading it), when ce refers to a + * regular file. If ca is NULL, the attributes will be loaded + * internally when (and if) needed. */ -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) +int checkout_entry_ca(struct cache_entry *ce, struct conv_attrs *ca, + const struct checkout *state, char *topath, + int *nr_checkouts); void enable_delayed_checkout(struct checkout *state); int finish_delayed_checkout(struct checkout *state, int *nr_checkouts);
The parallel checkout machinery will call checkout_entry() for entries that could not be written in parallel due to path collisions. At this point, we will already be holding the conversion attributes for each entry, and it would be wasteful to let checkout_entry() load these again. Instead, let's add the checkout_entry_ca() variant, which optionally takes a preloaded conv_attrs struct. Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> --- entry.c | 13 +++++++------ entry.h | 12 ++++++++++-- 2 files changed, 17 insertions(+), 8 deletions(-)