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
