Subject: Re: [PATCH v12 21/26] stash: optimize `get_untracked_files()` and `check_changes()`

From: Thomas Gummerer <[email-masked]>
Date: Sun, 6 Jan 2019 22:47:35 +0000

On 12/20, Paul-Sebastian Ungureanu wrote:
> This commits introduces a optimization by avoiding calling the
> same functions again. For example, `git stash push -u`
> would call at some points the following functions:
> 
>  * `check_changes()` (inside `do_push_stash()`)
>  * `do_create_stash()`, which calls: `check_changes()` and
> `get_untracked_files()`
> 
> Note that `check_changes()` also calls `get_untracked_files()`.
> So, `check_changes()` is called 2 times and `get_untracked_files()`
> 3 times.
> 
> The old function `check_changes()` now consists of two functions:
> `get_untracked_files()` and `check_changes_tracked_files()`.
> 
> These are the call chains for `push` and `create`:
> 
>  * `push_stash()` -> `do_push_stash()` -> `do_create_stash()`
> 
>  * `create_stash()` -> `do_create_stash()`
> 
> To prevent calling the same functions over and over again,
> `check_changes()` inside `do_create_stash()` is now placed
> in the caller functions (`create_stash()` and `do_push_stash()`).
> This way `check_changes()` and `get_untracked files()` are called
> only one time.
> 
> https://public-inbox.org/git/[email-masked]/

I missed this the other time, but having this link in the commit
message is unnecessary, and it may go stale (though in this case it
includes the Message-ID, so it is still useful as long as some archive
exists.  The commit message explains well enough what this change is
doing, even without the link to the review.

Sorry I missed these things in the earlier rounds somehow.

> Signed-off-by: Paul-Sebastian Ungureanu <[email-masked]>
> ---
>  builtin/stash--helper.c | 53 +++++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 23 deletions(-)
> 
> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> index 19ead63c46..4b63352927 100644
> --- a/builtin/stash--helper.c
> +++ b/builtin/stash--helper.c
> @@ -884,18 +884,18 @@ static int get_untracked_files(struct pathspec ps, int include_untracked,
>  }
>  
>  /*
> - * The return value of `check_changes()` can be:
> + * The return value of `check_changes_tracked_files()` can be:
>   *
>   * < 0 if there was an error
>   * = 0 if there are no changes.
>   * > 0 if there are changes.
>   */
> -static int check_changes(struct pathspec ps, int include_untracked)
> +

Unnecessary blank line after the comment here.

> +static int check_changes_tracked_files(struct pathspec ps)
>  {
>  	int result;
>  	struct rev_info rev;
>  	struct object_id dummy;
> -	struct strbuf out = STRBUF_INIT;
>  
>  	/* No initial commit. */
>  	if (get_oid("HEAD", &dummy))
> @@ -923,14 +923,26 @@ static int check_changes(struct pathspec ps, int include_untracked)
>  	if (diff_result_code(&rev.diffopt, result))
>  		return 1;
>  
> +	return 0;
> +}
> +
> +/*
> + * 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.
> + */
> +

Unnecessary blank line.

> +static int check_changes(struct pathspec ps, int include_untracked,
> +			 struct strbuf *untracked_files)
> +{
> +	int ret = 0;
> +	if (check_changes_tracked_files(ps))
> +		ret = 1;
> +
>  	if (include_untracked && get_untracked_files(ps, include_untracked,
> -						     &out)) {
> -		strbuf_release(&out);
> -		return 1;
> -	}
> +						     untracked_files))
> +		ret = 1;
>  
> -	strbuf_release(&out);
> -	return 0;
> +	return ret;
>  }
>  
>  static int save_untracked_files(struct stash_info *info, struct strbuf *msg,
> @@ -1141,7 +1153,7 @@ static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf,
>  		head_commit = lookup_commit(the_repository, &info->b_commit);
>  	}
>  
> -	if (!check_changes(ps, include_untracked)) {
> +	if (!check_changes(ps, include_untracked, &untracked_files)) {
>  		ret = 1;
>  		goto done;
>  	}
> @@ -1166,8 +1178,7 @@ static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf,
>  		goto done;
>  	}
>  
> -	if (include_untracked && get_untracked_files(ps, include_untracked,
> -						     &untracked_files)) {
> +	if (include_untracked) {
>  		if (save_untracked_files(info, &msg, untracked_files)) {
>  			if (!quiet)
>  				fprintf_ln(stderr, _("Cannot save "
> @@ -1252,20 +1263,15 @@ static int create_stash(int argc, const char **argv, const char *prefix)
>  			     0);
>  
>  	memset(&ps, 0, sizeof(ps));
> -	strbuf_addstr(&stash_msg_buf, stash_msg);
> -	ret = do_create_stash(ps, &stash_msg_buf, include_untracked, 0, &info,
> -			      NULL, 0);
> +	if (!check_changes_tracked_files(ps))
> +		return 0;
>  
> -	if (!ret)
> +	strbuf_addstr(&stash_msg_buf, stash_msg);
> +	if (!(ret = do_create_stash(ps, &stash_msg_buf, 0, 0, &info, NULL, 0)))

It's a bit odd to have do_create_stash moved inside the if here, even
though it was introduced in this patch series.  It makes the patch a
bit more noisy and harder to see what was changed here.

>  		printf_ln("%s", oid_to_hex(&info.w_commit));
>  
>  	strbuf_release(&stash_msg_buf);
> -
> -	/*
> -	 * ret can be 1 if there were no changes. In this case, we should
> -	 * not error out.
> -	 */
> -	return ret < 0;
> +	return ret;
>  }
>  
>  static int do_push_stash(struct pathspec ps, const char *stash_msg, int quiet,
> @@ -1275,6 +1281,7 @@ static int do_push_stash(struct pathspec ps, const char *stash_msg, int quiet,
>  	struct stash_info info;
>  	struct strbuf patch = STRBUF_INIT;
>  	struct strbuf stash_msg_buf = STRBUF_INIT;
> +	struct strbuf untracked_files = STRBUF_INIT;

Does this strbuf also need to be released at the end of the function?

>  
>  	if (patch_mode && keep_index == -1)
>  		keep_index = 1;
> @@ -1309,7 +1316,7 @@ static int do_push_stash(struct pathspec ps, const char *stash_msg, int quiet,
>  		goto done;
>  	}
>  
> -	if (!check_changes(ps, include_untracked)) {
> +	if (!check_changes(ps, include_untracked, &untracked_files)) {
>  		if (!quiet)
>  			printf_ln(_("No local changes to save"));
>  		goto done;
> -- 
> 2.20.1.441.g764a526393
>