Re: [PATCH v3 08/19] entry: move conv_attrs lookup up to checkout_entry()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 30, 2020, 21:58 UTC
- Message-ID
- <xmqqft5v5qbf.fsf@gitster.c.googlers.com>
- In-Reply-To
- <81e03baab1dd7e28262e1d721eac1646c5908b5a.1603937110.git.matheus.bernardino@usp.br>
Matheus Tavares <matheus.bernardino@usp.br> writes:
> +/* Note: ca is used (and required) iff the entry refers to a regular file. */
This reflects how the current code happens to work, and it is unlikely to change (in other words, I offhand do not think of a reason why attributes may affect checking out a symlink or a submodule), so that's probably OK. I mention this specifically because ...
Show 58 quoted lines
> +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;
>
> 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);
> + }... it looked somewhat upside-down at the first glance that we decide if lower level routines are allowed to use the ca at this high level in the callchain. But it is the point of this change to lift the point to make the decision to use attributes higher in the callchain, so it would be OK (or "unavoidable").
I wonder if it is worth to avoid early return from the inner block, like this:
struct conv_attrs *use_ca = NULL;
...
if (topath) {
struct conv_attrs ca;
if (S_ISREG(...)) {
convert_attrs(... &ca ...);
use_ca = &ca;
}
return write_entry(ce, topath, use_ca, state, 1);
}which would make it easier to further add code that is common to both regular file and other things before we call write_entry().
The same comment applies to the codepath where a new file gets created in the next hunk.
Show 18 quoted lines
> @@ -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)