git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Thomas Gummerer <t.gummerer@gmail.com>
Date
Jan 6, 2019, 22:47 UTC
Message-ID
<20190106224735.GI25639@hank.intra.tgummerer.com>
In-Reply-To
<17f45e4bb48bdf9588d87c7b134afd703dbd69e6.1545331726.git.ungureanupaulsebastian@gmail.com>
On 12/20, Paul-Sebastian Ungureanu wrote:
Show 28 quoted lines
> 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/20180818223329.GJ11326@hank.intra.tgummerer.com/

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.
Show 22 quoted lines
> Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
> ---
>  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.
Show 21 quoted lines
> +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.
Show 53 quoted lines
> +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.

Show 18 quoted lines
>  		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?
Show 15 quoted lines
>  
>  	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
>
Previous: Paul-Sebastian UngureanuNext: Paul-Sebastian Ungureanu
Message 24 of 106 in “Convert "git stash" to C builtin”
  1. 00/26 Convert "git stash" to C builtinPaul-Sebastian Ungureanu, Dec 20, 2018
  2. 01/26 sha1-name.c: add `get_oidf()` which acts like `get_oid()`Paul-Sebastian Ungureanu, Dec 20, 2018
  3. 02/26 strbuf.c: add `strbuf_join_argv()`Paul-Sebastian Ungureanu, Dec 20, 2018
  4. 03/26 strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()`Paul-Sebastian Ungureanu, Dec 20, 2018
  5. 05/26 stash: improve option parsing test coveragePaul-Sebastian Ungureanu, Dec 20, 2018
  6. 04/26 ident: add the ability to provide a "fallback identity"Paul-Sebastian Ungureanu, Dec 20, 2018
  7. Junio C HamanoDec 26, 2018
  8. Johannes SchindelinDec 27, 2018
  9. Junio C HamanoDec 28, 2018
  10. 06/26 t3903: modernize stylePaul-Sebastian Ungureanu, Dec 20, 2018
  11. 08/26 stash: add tests for `git stash show` configPaul-Sebastian Ungureanu, Dec 20, 2018
  12. 09/26 stash: mention options in `show` synopsisPaul-Sebastian Ungureanu, Dec 20, 2018
  13. 07/26 stash: rename test cases to be more descriptivePaul-Sebastian Ungureanu, Dec 20, 2018
  14. 10/26 stash: convert apply to builtinPaul-Sebastian Ungureanu, Dec 20, 2018
  15. 12/26 stash: convert branch to builtinPaul-Sebastian Ungureanu, Dec 20, 2018
  16. 15/26 stash: convert show to builtinPaul-Sebastian Ungureanu, Dec 20, 2018
  17. 16/26 stash: convert store to builtinPaul-Sebastian Ungureanu, Dec 20, 2018
  18. 14/26 stash: convert list to builtinPaul-Sebastian Ungureanu, Dec 20, 2018
  19. 13/26 stash: convert pop to builtinPaul-Sebastian Ungureanu, Dec 20, 2018
  20. 11/26 stash: convert drop and clear to builtinPaul-Sebastian Ungureanu, Dec 20, 2018
  21. 19/26 stash: make push -q quietPaul-Sebastian Ungureanu, Dec 20, 2018
  22. 22/26 stash: replace all `write-tree` child processes with API callsPaul-Sebastian Ungureanu, Dec 20, 2018
  23. 21/26 stash: optimize `get_untracked_files()` and `check_changes()`Paul-Sebastian Ungureanu, Dec 20, 2018
  24. Thomas GummererJan 6, 2019
  25. 26/26 tests: add a special setup where stash.useBuiltin is offPaul-Sebastian Ungureanu, Dec 20, 2018
  26. 24/26 stash: add back the original, scripted `git stash`Paul-Sebastian Ungureanu, Dec 20, 2018
  27. 20/26 stash: convert save to builtinPaul-Sebastian Ungureanu, Dec 20, 2018
  28. 18/26 stash: convert push to builtinPaul-Sebastian Ungureanu, Dec 20, 2018
  29. SZEDER GáborFeb 8, 2019
  30. Thomas GummererFeb 10, 2019
  31. SZEDER GáborFeb 11, 2019
  32. Thomas GummererFeb 12, 2019
  33. SZEDER GáborFeb 19, 2019
  34. Johannes SchindelinFeb 19, 2019
  35. Junio C HamanoFeb 19, 2019
  36. Johannes SchindelinFeb 20, 2019
  37. Thomas GummererFeb 19, 2019
  38. Junio C HamanoFeb 20, 2019
  39. Johannes SchindelinFeb 20, 2019
  40. Thomas GummererFeb 20, 2019
  41. 00/27 Convert "git stash" to C builtinThomas Gummerer, Feb 25, 2019
  42. 01/27 sha1-name.c: add `get_oidf()` which acts like `get_oid()`Thomas Gummerer, Feb 25, 2019
  43. 02/27 strbuf.c: add `strbuf_join_argv()`Thomas Gummerer, Feb 25, 2019
  44. 03/27 strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()`Thomas Gummerer, Feb 25, 2019
  45. 05/27 stash: improve option parsing test coverageThomas Gummerer, Feb 25, 2019
  46. 04/27 ident: add the ability to provide a "fallback identity"Thomas Gummerer, Feb 25, 2019
  47. 06/27 t3903: modernize styleThomas Gummerer, Feb 25, 2019
  48. 07/27 t3903: add test for --intent-to-add fileThomas Gummerer, Feb 25, 2019
  49. 08/27 stash: rename test cases to be more descriptiveThomas Gummerer, Feb 25, 2019
  50. 09/27 stash: add tests for `git stash show` configThomas Gummerer, Feb 25, 2019
  51. 10/27 stash: mention options in `show` synopsisThomas Gummerer, Feb 25, 2019
  52. 11/27 stash: convert apply to builtinThomas Gummerer, Feb 25, 2019
  53. regression in new built-in stash + fsmonitor (was Re: [PATCH v13 11/27] stash: convert apply to builtin)Ævar Arnfjörð Bjarmason, Mar 14, 2019
  54. Johannes SchindelinMar 14, 2019
  55. Ævar Arnfjörð BjarmasonMar 14, 2019
  56. Johannes SchindelinMar 14, 2019
  57. Ævar Arnfjörð BjarmasonMar 14, 2019
  58. Ben PeartMar 15, 2019
  59. 12/27 stash: convert drop and clear to builtinThomas Gummerer, Feb 25, 2019
  60. Jeff KingMar 7, 2019
  61. Thomas GummererMar 9, 2019
  62. Jeff KingMar 10, 2019
  63. Junio C HamanoMar 11, 2019
  64. Thomas GummererMar 11, 2019
  65. 13/27 stash: convert branch to builtinThomas Gummerer, Feb 25, 2019
  66. 14/27 stash: convert pop to builtinThomas Gummerer, Feb 25, 2019
  67. 15/27 stash: convert list to builtinThomas Gummerer, Feb 25, 2019
  68. 16/27 stash: convert show to builtinThomas Gummerer, Feb 25, 2019
  69. 17/27 stash: convert store to builtinThomas Gummerer, Feb 25, 2019
  70. 18/27 stash: convert create to builtinThomas Gummerer, Feb 25, 2019
  71. Jeff KingMar 7, 2019
  72. Johannes SchindelinMar 8, 2019
  73. Thomas GummererMar 9, 2019
  74. Junio C HamanoMar 11, 2019
  75. Junio C HamanoMar 11, 2019
  76. Thomas GummererMar 11, 2019
  77. stash: pass pathspec as pointerThomas Gummerer, Mar 11, 2019
  78. Junio C HamanoMar 12, 2019
  79. Johannes SchindelinMar 12, 2019
  80. Thomas GummererMar 12, 2019
  81. Junio C HamanoMar 13, 2019
  82. Johannes SchindelinMar 13, 2019
  83. Thomas GummererMar 15, 2019
  84. 19/27 stash: convert push to builtinThomas Gummerer, Feb 25, 2019
  85. 20/27 stash: make push -q quietThomas Gummerer, Feb 25, 2019
  86. 21/27 stash: convert save to builtinThomas Gummerer, Feb 25, 2019
  87. 22/27 stash: optimize `get_untracked_files()` and `check_changes()`Thomas Gummerer, Feb 25, 2019
  88. 23/27 stash: replace all `write-tree` child processes with API callsThomas Gummerer, Feb 25, 2019
  89. 24/27 stash: convert `stash--helper.c` into `stash.c`Thomas Gummerer, Feb 25, 2019
  90. 26/27 stash: optionally use the scripted version againThomas Gummerer, Feb 25, 2019
  91. 25/27 stash: add back the original, scripted `git stash`Thomas Gummerer, Feb 25, 2019
  92. 27/27 tests: add a special setup where stash.useBuiltin is offThomas Gummerer, Feb 25, 2019
  93. Johannes SchindelinFeb 26, 2019
  94. Thomas GummererFeb 26, 2019
  95. Ævar Arnfjörð BjarmasonFeb 26, 2019
  96. Johannes SchindelinFeb 26, 2019
  97. Junio C HamanoMar 3, 2019
  98. Junio C HamanoMar 3, 2019
  99. Thomas GummererMar 3, 2019
  100. 25/26 stash: optionally use the scripted version againPaul-Sebastian Ungureanu, Dec 20, 2018
  101. Thomas GummererJan 6, 2019
  102. 23/26 stash: convert `stash--helper.c` into `stash.c`Paul-Sebastian Ungureanu, Dec 20, 2018
  103. 17/26 stash: convert create to builtinPaul-Sebastian Ungureanu, Dec 20, 2018
  104. Junio C HamanoJan 3, 2019
  105. Johannes SchindelinJan 18, 2019
  106. Junio C HamanoJan 18, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.