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