diff mbox series

[v4,08/19] entry: move conv_attrs lookup up to checkout_entry()

Message ID 5c993cc27f67109828390c7856d6c03d4a2cbb32.1604521275.git.matheus.bernardino@usp.br (mailing list archive)
State New, archived
Headers show
Series Parallel Checkout (part I) | expand

Commit Message

Matheus Tavares Nov. 4, 2020, 8:33 p.m. UTC 
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(-)

Comments

Christian Couder Dec. 6, 2020, 9:35 a.m. UTC | #1 
On Wed, Nov 4, 2020 at 9:34 PM Matheus Tavares
<matheus.bernardino@usp.br> wrote:

> +       if (topath) {
> +               if (S_ISREG(ce->ce_mode)) {
> +                       convert_attrs(state->istate, &ca_buf, ce->name);
> +                       ca = &ca_buf;
> +               }
> +               return write_entry(ce, topath, ca, state, 1);

We pass `ca` here instead of `&ca_buf` because ca is NULL if we are
not dealing with a regular file. Ok, I think it's indeed better to
pass NULL in this case.

> @@ -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_buf, ce->name);
> +               ca = &ca_buf;
> +       }
> +
> +       return write_entry(ce, path.buf, NULL, state, 0);

I am not sure why NULL is passed here though instead of `ca`.

The following comment is added in front of write_entry():

+/* Note: ca is used (and required) iff the entry refers to a regular file. */

So I would think that `ca` should be passed.
Matheus Tavares Dec. 7, 2020, 1:52 p.m. UTC | #2 
On Sun, Dec 6, 2020 at 6:36 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Wed, Nov 4, 2020 at 9:34 PM Matheus Tavares
> <matheus.bernardino@usp.br> wrote:
>
> > +       if (topath) {
> > +               if (S_ISREG(ce->ce_mode)) {
> > +                       convert_attrs(state->istate, &ca_buf, ce->name);
> > +                       ca = &ca_buf;
> > +               }
> > +               return write_entry(ce, topath, ca, state, 1);
>
> We pass `ca` here instead of `&ca_buf` because ca is NULL if we are
> not dealing with a regular file. Ok, I think it's indeed better to
> pass NULL in this case.
>
> > @@ -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_buf, ce->name);
> > +               ca = &ca_buf;
> > +       }
> > +
> > +       return write_entry(ce, path.buf, NULL, state, 0);
>
> I am not sure why NULL is passed here though instead of `ca`.

Oops, this is indeed wrong. I think I forgot to modify this line while
applying the changes from the last review round. Thanks for catching
it!
diff mbox series

Patch

diff --git a/entry.c b/entry.c
index 1d2df188e5..486712c3a9 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_buf, *ca = NULL;
 
 	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_buf, ce->name);
+			ca = &ca_buf;
+		}
+		return write_entry(ce, topath, ca, 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_buf, ce->name);
+		ca = &ca_buf;
+	}
+
+	return write_entry(ce, path.buf, NULL, state, 0);
 }
 
 void unlink_entry(const struct cache_entry *ce)