Re: [PATCH v2 08/19] entry: move conv_attrs lookup up to checkout_entry()
- From
- Jeff Hostetler <git@jeffhostetler.com>
- Date
- Oct 1, 2020, 15:53 UTC
- Message-ID
- <0b7d7704-159f-c186-9551-1989af8f572d@jeffhostetler.com>
- In-Reply-To
- <667ad0dea70cb7f0bbf8f52467f15129b3ae1325.1600814153.git.matheus.bernardino@usp.br>
On 9/22/20 6:49 PM, Matheus Tavares wrote:
Show 64 quoted lines
> 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@usp.br>
> ---
> 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.
Show 38 quoted lines
>
> 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)
>