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

[Thomas Gummerer <t.gummerer@gmail.com>]

On 03/12, Johannes Schindelin wrote:
> 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...

That's fair, I went with what seemed most common in the codebase.
More than half the parameters seem to be using "const struct
pathspec", so that seems to be the more common way if we don't require
the parameter to be modifyable.

$ git grep -F "struct pathspec *" | wc -l
81
$ git grep -F "const struct pathspec *" | wc -l
67


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

'clear_pathspec()' doesn't actually check whether the parameter passed
to it is NULL or not before dereferencing it.  The first few lines of
the function are:

	void clear_pathspec(struct pathspec *pathspec)
	{
		int i, j;

		for (i = 0; i < pathspec->nr; i++) {
		[...]

So I think moving the 'copy_pathspec()' earlier is actually required.
It may make sense to make 'clear_pathspec()' safe to call with a NULL
pointer, dunno.

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

Thanks for your review!

> Thank you for going through this!
> Dscho