Show 80 quoted lines
>
>
> 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@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 37 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)
>>