Re: [PATCH v2] stash: pass pathspec as pointer

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 12, 2019, 22:35 UTC
Message-ID: <nycvar.QRO.7.76.6.1903122323460.41@tvgsbejvaqbjf.bet>
In-Reply-To: <20190311221624.GC16414@hank.intra.tgummerer.com>

Hi Thomas,

On Mon, 11 Mar 2019, Thomas Gummerer wrote:

> Passing the pathspec by value is potentially confusing, as the copy is
> only a shallow copy, so save the overhead of the copy, and pass the
> pathspec struct as a pointer.

Not only confusing, but also wasteful ;-)

> In addition use copy_pathspec to copy the pathspec into
> rev.prune_data, so the copy is a proper deep copy, and owned by the
> revision API, as that's what the API expects.

Good.

Show 11 quoted lines

> [...]
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 2f29d037c8..e0528d4cc8 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -826,11 +826,11 @@ static int store_stash(int argc, const char **argv, const char *prefix)
>  }
>  
>  static void add_pathspecs(struct argv_array *args,
> -			  struct pathspec ps) {
> +			  const struct pathspec *ps) {

I see that you added the `const` keyword. While it does not hurt, I would probably not have bothered...

Show 6 quoted lines

> [...]
> @@ -1042,6 +1049,7 @@ static int stash_working_tree(struct stash_info *info, struct pathspec ps)
>  	struct index_state istate = { NULL };
>  
>  	init_revisions(&rev, NULL);
> +	copy_pathspec(&rev.prune_data, ps);

This moved here... because...

>  
>  	set_alternate_index_output(stash_index_path.buf);
>  	if (reset_tree(&info->i_tree, 0, 0)) {

... this `if` block could jump to...

Show 9 quoted lines

> @@ -1050,7 +1058,6 @@ static int stash_working_tree(struct stash_info *info, struct pathspec ps)
>  	}
>  	set_alternate_index_output(NULL);
>  
> -	rev.prune_data = ps;
>  	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
>  	rev.diffopt.format_callback = add_diff_to_buf;
>  	rev.diffopt.format_callback_data = &diff_output;
> @@ -1089,12 +1096,13 @@ static int stash_working_tree(struct stash_info *info, struct pathspec ps)

... this point (the `done:` label is *just* one line further up, and this is a static diff, so we cannot just increase the context when we need to see more, unlike, say, GitHub PRs) and...

>  	discard_index(&istate);
>  	UNLEAK(rev);
>  	object_array_clear(&rev.pending);
> +	clear_pathspec(&rev.prune_data);

... we add this call here.

However, we would not have needed to move the initialization of `rev.prune_data`, I don't think, because `init_revision()` zeros the entire struct, including `prune_data`, which would have made `clear_pathspec()` safe to call, too.

Both of my comments need no action, and the rest of the patch looks good to me.

Thank you for going through this! Dscho