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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

> [...]
> 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...

> [...]
> @@ -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...


> @@ -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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux