[v2] stash: pass pathspec as pointer
diff mbox series

Message ID 20190311221624.GC16414@hank.intra.tgummerer.com
State New
Headers show
Series
  • [v2] stash: pass pathspec as pointer
Related show

Commit Message

Thomas Gummerer March 11, 2019, 10:16 p.m. UTC
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.

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.

Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---

> Good catch on both acconts.  I'll send a new patch soon, adding the
> clear_pathspec() calls and rebasing it on top of Dscho's fix.

Here it is.  Thanks for the review of the first round Junio!

This is on top of Dscho's series at
<pull.159.git.gitgitgadget@gmail.com> applied to ps/stash-in-c.

 builtin/stash.c | 68 +++++++++++++++++++++++++++----------------------
 1 file changed, 38 insertions(+), 30 deletions(-)

Comments

Junio C Hamano March 12, 2019, 6:50 a.m. UTC | #1
Thomas Gummerer <t.gummerer@gmail.com> writes:

>> Good catch on both acconts.  I'll send a new patch soon, adding the
>> clear_pathspec() calls and rebasing it on top of Dscho's fix.
>
> Here it is.  Thanks for the review of the first round Junio!
>
> This is on top of Dscho's series at
> <pull.159.git.gitgitgadget@gmail.com> applied to ps/stash-in-c.

That's called js/stash-in-c-pathspec-fix; as this patch is also
about pathspec, I guess it is a good idea to just keep them in a
single topic, so I'll apply it there.
Johannes Schindelin March 12, 2019, 10:35 p.m. UTC | #2
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
Thomas Gummerer March 12, 2019, 11:40 p.m. UTC | #3
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
Junio C Hamano March 13, 2019, 1:47 a.m. UTC | #4
Thomas Gummerer <t.gummerer@gmail.com> writes:

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

Yes, when you prepare a struct at a callsite and pass it thru a long
callchain, it is very helpful to both humans and compilers reading
the code to declare that the structure would not be modified, if the
code indeed keeps it constant.  A caller that used to passed the
structure by value certainly hasn't been expecting the callee would
modify its contents and it needs to read back the updated value, so
I find that most of these constifing, if not all, very much in line
with the original's spirit.
Johannes Schindelin March 13, 2019, 10:14 p.m. UTC | #5
Hi Thomas,

On Tue, 12 Mar 2019, Thomas Gummerer wrote:

> On 03/12, Johannes Schindelin wrote:
> 
> > On Mon, 11 Mar 2019, Thomas Gummerer wrote:
> > > [...]
> > > @@ -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.

In this case, it does not need to check for NULL, as `&rev.prune_data`
will always be non-NULL: `rev`'s `prune_data` field is of type `struct
patchspec`, i.e. *not* a pointer (in which case the type would be `struct
pathspec *`). See for yourself:

	https://github.com/git/git/blob/v2.21.0/revision.h#L91

Ciao,
Dscho
Thomas Gummerer March 15, 2019, 10:33 p.m. UTC | #6
On 03/13, Johannes Schindelin wrote:
> Hi Thomas,
> 
> On Tue, 12 Mar 2019, Thomas Gummerer wrote:
> 
> > On 03/12, Johannes Schindelin wrote:
> > > 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.
> 
> In this case, it does not need to check for NULL, as `&rev.prune_data`
> will always be non-NULL: `rev`'s `prune_data` field is of type `struct
> patchspec`, i.e. *not* a pointer (in which case the type would be `struct
> pathspec *`). See for yourself:
> 
> 	https://github.com/git/git/blob/v2.21.0/revision.h#L91

Doh, you're right of course, I totally missed that.  Thanks for the
pointer, and sorry for the noise!

Patch
diff mbox series

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) {
 	int i;
 
-	for (i = 0; i < ps.nr; i++)
-		argv_array_push(args, ps.items[i].original);
+	for (i = 0; i < ps->nr; i++)
+		argv_array_push(args, ps->items[i].original);
 }
 
 /*
@@ -840,7 +840,7 @@  static void add_pathspecs(struct argv_array *args,
  * = 0 if there are not any untracked files
  * > 0 if there are untracked files
  */
-static int get_untracked_files(struct pathspec ps, int include_untracked,
+static int get_untracked_files(const struct pathspec *ps, int include_untracked,
 			       struct strbuf *untracked_files)
 {
 	int i;
@@ -853,12 +853,12 @@  static int get_untracked_files(struct pathspec ps, int include_untracked,
 	if (include_untracked != INCLUDE_ALL_FILES)
 		setup_standard_excludes(&dir);
 
-	seen = xcalloc(ps.nr, 1);
+	seen = xcalloc(ps->nr, 1);
 
-	max_len = fill_directory(&dir, the_repository->index, &ps);
+	max_len = fill_directory(&dir, the_repository->index, ps);
 	for (i = 0; i < dir.nr; i++) {
 		struct dir_entry *ent = dir.entries[i];
-		if (dir_path_match(&the_index, ent, &ps, max_len, seen)) {
+		if (dir_path_match(&the_index, ent, ps, max_len, seen)) {
 			found++;
 			strbuf_addstr(untracked_files, ent->name);
 			/* NUL-terminate: will be fed to update-index -z */
@@ -881,11 +881,12 @@  static int get_untracked_files(struct pathspec ps, int include_untracked,
  * = 0 if there are no changes.
  * > 0 if there are changes.
  */
-static int check_changes_tracked_files(struct pathspec ps)
+static int check_changes_tracked_files(const struct pathspec *ps)
 {
 	int result;
 	struct rev_info rev;
 	struct object_id dummy;
+	int ret = 0;
 
 	/* No initial commit. */
 	if (get_oid("HEAD", &dummy))
@@ -895,7 +896,7 @@  static int check_changes_tracked_files(struct pathspec ps)
 		return -1;
 
 	init_revisions(&rev, NULL);
-	rev.prune_data = ps;
+	copy_pathspec(&rev.prune_data, ps);
 
 	rev.diffopt.flags.quick = 1;
 	rev.diffopt.flags.ignore_submodules = 1;
@@ -905,22 +906,28 @@  static int check_changes_tracked_files(struct pathspec ps)
 	diff_setup_done(&rev.diffopt);
 
 	result = run_diff_index(&rev, 1);
-	if (diff_result_code(&rev.diffopt, result))
-		return 1;
+	if (diff_result_code(&rev.diffopt, result)) {
+		ret = 1;
+		goto done;
+	}
 
 	object_array_clear(&rev.pending);
 	result = run_diff_files(&rev, 0);
-	if (diff_result_code(&rev.diffopt, result))
-		return 1;
+	if (diff_result_code(&rev.diffopt, result)) {
+		ret = 1;
+		goto done;
+	}
 
-	return 0;
+done:
+	clear_pathspec(&rev.prune_data);
+	return ret;
 }
 
 /*
  * The function will fill `untracked_files` with the names of untracked files
  * It will return 1 if there were any changes and 0 if there were not.
  */
-static int check_changes(struct pathspec ps, int include_untracked,
+static int check_changes(const struct pathspec *ps, int include_untracked,
 			 struct strbuf *untracked_files)
 {
 	int ret = 0;
@@ -974,7 +981,7 @@  static int save_untracked_files(struct stash_info *info, struct strbuf *msg,
 	return ret;
 }
 
-static int stash_patch(struct stash_info *info, struct pathspec ps,
+static int stash_patch(struct stash_info *info, const struct pathspec *ps,
 		       struct strbuf *out_patch, int quiet)
 {
 	int ret = 0;
@@ -1033,7 +1040,7 @@  static int stash_patch(struct stash_info *info, struct pathspec ps,
 	return ret;
 }
 
-static int stash_working_tree(struct stash_info *info, struct pathspec ps)
+static int stash_working_tree(struct stash_info *info, const struct pathspec *ps)
 {
 	int ret = 0;
 	struct rev_info rev;
@@ -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);
 
 	set_alternate_index_output(stash_index_path.buf);
 	if (reset_tree(&info->i_tree, 0, 0)) {
@@ -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)
 	discard_index(&istate);
 	UNLEAK(rev);
 	object_array_clear(&rev.pending);
+	clear_pathspec(&rev.prune_data);
 	strbuf_release(&diff_output);
 	remove_path(stash_index_path.buf);
 	return ret;
 }
 
-static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf,
+static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_buf,
 			   int include_untracked, int patch_mode,
 			   struct stash_info *info, struct strbuf *patch,
 			   int quiet)
@@ -1226,10 +1234,10 @@  static int create_stash(int argc, const char **argv, const char *prefix)
 	strbuf_join_argv(&stash_msg_buf, argc - 1, ++argv, ' ');
 
 	memset(&ps, 0, sizeof(ps));
-	if (!check_changes_tracked_files(ps))
+	if (!check_changes_tracked_files(&ps))
 		return 0;
 
-	ret = do_create_stash(ps, &stash_msg_buf, 0, 0, &info,
+	ret = do_create_stash(&ps, &stash_msg_buf, 0, 0, &info,
 			      NULL, 0);
 	if (!ret)
 		printf_ln("%s", oid_to_hex(&info.w_commit));
@@ -1238,7 +1246,7 @@  static int create_stash(int argc, const char **argv, const char *prefix)
 	return ret;
 }
 
-static int do_push_stash(struct pathspec ps, const char *stash_msg, int quiet,
+static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int quiet,
 			 int keep_index, int patch_mode, int include_untracked)
 {
 	int ret = 0;
@@ -1258,15 +1266,15 @@  static int do_push_stash(struct pathspec ps, const char *stash_msg, int quiet,
 	}
 
 	read_cache_preload(NULL);
-	if (!include_untracked && ps.nr) {
+	if (!include_untracked && ps->nr) {
 		int i;
-		char *ps_matched = xcalloc(ps.nr, 1);
+		char *ps_matched = xcalloc(ps->nr, 1);
 
 		for (i = 0; i < active_nr; i++)
-			ce_path_match(&the_index, active_cache[i], &ps,
+			ce_path_match(&the_index, active_cache[i], ps,
 				      ps_matched);
 
-		if (report_path_error(ps_matched, &ps, NULL)) {
+		if (report_path_error(ps_matched, ps, NULL)) {
 			fprintf_ln(stderr, _("Did you forget to 'git add'?"));
 			ret = -1;
 			free(ps_matched);
@@ -1313,7 +1321,7 @@  static int do_push_stash(struct pathspec ps, const char *stash_msg, int quiet,
 			  stash_msg_buf.buf);
 
 	if (!patch_mode) {
-		if (include_untracked && !ps.nr) {
+		if (include_untracked && !ps->nr) {
 			struct child_process cp = CHILD_PROCESS_INIT;
 
 			cp.git_cmd = 1;
@@ -1327,7 +1335,7 @@  static int do_push_stash(struct pathspec ps, const char *stash_msg, int quiet,
 			}
 		}
 		discard_cache();
-		if (ps.nr) {
+		if (ps->nr) {
 			struct child_process cp_add = CHILD_PROCESS_INIT;
 			struct child_process cp_diff = CHILD_PROCESS_INIT;
 			struct child_process cp_apply = CHILD_PROCESS_INIT;
@@ -1468,7 +1476,7 @@  static int push_stash(int argc, const char **argv, const char *prefix)
 
 	parse_pathspec(&ps, 0, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,
 		       prefix, argv);
-	return do_push_stash(ps, stash_msg, quiet, keep_index, patch_mode,
+	return do_push_stash(&ps, stash_msg, quiet, keep_index, patch_mode,
 			     include_untracked);
 }
 
@@ -1505,7 +1513,7 @@  static int save_stash(int argc, const char **argv, const char *prefix)
 		stash_msg = strbuf_join_argv(&stash_msg_buf, argc, argv, ' ');
 
 	memset(&ps, 0, sizeof(ps));
-	ret = do_push_stash(ps, stash_msg, quiet, keep_index,
+	ret = do_push_stash(&ps, stash_msg, quiet, keep_index,
 			    patch_mode, include_untracked);
 
 	strbuf_release(&stash_msg_buf);