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

Re: [PATCH v10 00/21] Convert "git stash" to C builtin

From
Thomas Gummerer <t.gummerer@gmail.com>
Date
Oct 16, 2018, 19:59 UTC
Message-ID
<20181016195949.GE4883@hank.intra.tgummerer.com>
In-Reply-To
<nycvar.QRO.7.76.6.1810161205530.4546@tvgsbejvaqbjf.bet>
On 10/16, Johannes Schindelin wrote:
Show 19 quoted lines
> Hi Thomas,
> 
> On Mon, 15 Oct 2018, Thomas Gummerer wrote:
> 
> >  2:  63f2e0e6f9 !  2:  2d45985676 strbuf.c: add `strbuf_join_argv()`
> >     @@ -14,19 +14,17 @@
> >       	strbuf_setlen(sb, sb->len + sb2->len);
> >       }
> >       
> >     -+const char *strbuf_join_argv(struct strbuf *buf,
> >     -+			     int argc, const char **argv, char delim)
> >     ++void strbuf_join_argv(struct strbuf *buf,
> >     ++		      int argc, const char **argv, char delim)
> 
> While the patch series does not use the return value, I have to ask
> whether it would really be useful to change it to return `void`. I could
> imagine that there may already be quite a few code paths that would love
> to use strbuf_join_argv(), *and* would benefit from the `const char *`
> return value.

Fair enough. I did suggest changing the return type to void here, as I found the API a bit odd compared to the rest of the strbuf API, however after looking at this again I agree with you, and returning a const char * here does seem more helpful. Sorry about the confusion Paul-Sebastian!

Show 14 quoted lines
> In other words: just because the *current* patches do not make use of that
> quite convenient return value does not mean that we should remove that
> convenience.
>
> >  7:  a2abd1b4bd !  8:  974dbaa492 stash: convert apply to builtin
> >     @@ -370,18 +370,20 @@
> >      +
> >      +			if (diff_tree_binary(&out, &info->w_commit)) {
> >      +				strbuf_release(&out);
> >     -+				return -1;
> >     ++				return error(_("Could not generate diff %s^!."),
> >     ++					     oid_to_hex(&info->w_commit));
> 
> Please start the argument of an `error()` call with a lower-case letter.

I think this comes from your fixup! commit ;) But I do agree, these should be lower-case.

Show 29 quoted lines
> >      +			}
> >      +
> >      +			ret = apply_cached(&out);
> >      +			strbuf_release(&out);
> >      +			if (ret)
> >     -+				return -1;
> >     ++				return error(_("Conflicts in index."
> >     ++					       "Try without --index."));
> 
> Same here.
> 
> >      +
> >      +			discard_cache();
> >      +			read_cache();
> >      +			if (write_cache_as_tree(&index_tree, 0, NULL))
> >     -+				return -1;
> >     ++				return error(_("Could not save index tree"));
> 
> And here.
> 
> > 15:  bd827be103 ! 15:  989db67e9a stash: convert create to builtin
> >     @@ -119,7 +119,6 @@
> >      +static int check_changes(struct pathspec ps, int include_untracked)
> >      +{
> >      +	int result;
> >     -+	int ret = 0;
> 
> I was curious about this change, and could not find it in the
> git-stash-v10 tag of https://github.com/ungps/git...

This line has been removed in v10, but did exist in v9, so the git-stash-v10 should indeed not have this line. I suggested removing it in [*1*], because it breaks compilation with DEVELOPER=1 at this step.

Show 14 quoted lines
> > 18:  1c501ad666 ! 18:  c90e30173a stash: convert save to builtin
> >     @@ -72,8 +72,10 @@
> >      +			     git_stash_helper_save_usage,
> >      +			     PARSE_OPT_KEEP_DASHDASH);
> >      +
> >     -+	if (argc)
> >     -+		stash_msg = (char*) strbuf_join_argv(&buf, argc, argv, ' ');
> >     ++	if (argc) {
> >     ++		strbuf_join_argv(&buf, argc, argv, ' ');
> >     ++		stash_msg = buf.buf;
> >     ++	}
> 
> Aha! So there *was* a user of that return value. I really would prefer a
> non-void return value here.

Right, I'd argue we're mis-using the API here though. do_push_stash who we later pass stash_msg to takes ownership and later free's the memory before returning. This doesn't cause issues in the test suite at the moment, because do_create_stash() doesn't always free stash_msg before assigning a new value to the pointer, but would cause issues when do_create_stash exits early.

Rather than the solution I proposed in I think it would be nicer to use 'stash_msg = strbuf_detach(...)' above.

I'm still happy with the function returning buf->buf as const char *, but I'm not sure we should use that return value here.

Show 14 quoted lines
> > 19:  c4401b21db ! 19:  4360ea875d stash: convert `stash--helper.c` into `stash.c`
> >     @@ -264,9 +320,9 @@
> >      -	argc = parse_options(argc, argv, prefix, options,
> >      -			     git_stash_helper_create_usage,
> >      -			     0);
> >     -+	/* Startinf with argv[1], since argv[0] is "create" */
> >     -+	stash_msg = (char*) strbuf_join_argv(&stash_msg_buf, argc - 1,
> >     -+					     ++argv, ' ');
> >     ++	/* Starting with argv[1], since argv[0] is "create" */
> >     ++	strbuf_join_argv(&stash_msg_buf, argc - 1, ++argv, ' ');
> >     ++	stash_msg = stash_msg_buf.buf;
> 
> Again, I would strongly prefer the convenience of assigning the return
> value directly, rather than having two lines.

This is a similar case as above, where I think using strbuf_detach would be best, again instead of the 'xstrdup()' I mentioned in [*2*].

Show 21 quoted lines
> >     @@ -375,10 +425,8 @@
> >      +			 * they need to be immediately followed by a string
> >      +			 * (i.e.`-m"foobar"` or `--message="foobar"`).
> >      +			 */
> >     -+			if ((strlen(argv[i]) > 2 &&
> >     -+			     !strncmp(argv[i], "-m", 2)) ||
> >     -+			    (strlen(argv[i]) > 10 &&
> >     -+			     !strncmp(argv[i], "--message=", 10)))
> >     ++			if (starts_with(argv[i], "-m") ||
> >     ++			    starts_with(argv[i], "--message="))
> 
> Very nice.
> 
> > 20:  92dc11fd16 ! 20:  a384b05008 stash: optimize `get_untracked_files()` and `check_changes()`
> >     @@ -52,7 +52,6 @@
> >      +static int check_changes_tracked_files(struct pathspec ps)
> >       {
> >       	int result;
> >     --	int ret = 0;
> 
> I also wonder about this change, in light of...

The double - in the beginning of the range diff indicates that the removal of this line was removed from this particular patch (the removal is now done in patch 15 instead). I think this is one of those cases where the range-diff is a bit hard to interpret, especially without the --dual-color mode :)

Show 10 quoted lines
> >       	struct rev_info rev;
> >       	struct object_id dummy;
> >      -	struct strbuf out = STRBUF_INIT;
> >     @@ -99,8 +98,8 @@
> >      -	if (!check_changes(ps, include_untracked)) {
> >      +	if (!check_changes(ps, include_untracked, &untracked_files)) {
> >       		ret = 1;
> 
> this here line. How does that work, if `ret` is removed? And why didn't
> the `make DEVELOPER=1` complain about that unused `ret` variable before?

This is a different function, the above is in the 'check_changes_tracked_files()' function, while we are in the 'do_create_stash()' function, which has a local 'ret' variable.

Show 13 quoted lines
> 
> >     - 		*stash_msg = NULL;
> >       		goto done;
> >     + 	}
> >      @@
> >       		goto done;
> 
> The rest of the changes looks pretty sensible to me (indentation/wrapping
> changes, mostly, with a couple of commit message/typo fixes thrown in).
> 
> Maybe you have a commit hash, or even better, a tag in a public Git
> repository somewhere, so that Paul can pick it up more easily (and compare
> the changes with his latest local branch)?

The range-diff here between Paul's v9 and v10 series. I just applied both series, and sent out the range-diff in case someone else prefered not applying both series, or fetching both from Paul's repo, but reading the range-diff (and I got you to comment on it, which I already found helpful :)). There are no changes of my own included in this.

> Thank you!
> Dscho

[*1*]: https://public-inbox.org/git/20181002201940.GH2253@hank.intra.tgummerer.com/ [*2*]: https://public-inbox.org/git/20181015220338.GB4883@hank.intra.tgummerer.com/

Previous: Johannes Schindelin
Message 30 of 30 in “Convert "git stash" to C builtin”
  1. 00/21 Convert "git stash" to C builtinPaul-Sebastian Ungureanu, Oct 14, 2018
  2. 01/21 sha1-name.c: add `get_oidf()` which acts like `get_oid()`Paul-Sebastian Ungureanu, Oct 14, 2018
  3. 02/21 strbuf.c: add `strbuf_join_argv()`Paul-Sebastian Ungureanu, Oct 14, 2018
  4. 03/21 stash: improve option parsing test coveragePaul-Sebastian Ungureanu, Oct 14, 2018
  5. 04/21 t3903: modernize stylePaul-Sebastian Ungureanu, Oct 14, 2018
  6. 05/21 stash: rename test cases to be more descriptivePaul-Sebastian Ungureanu, Oct 14, 2018
  7. 06/21 stash: add tests for `git stash show` configPaul-Sebastian Ungureanu, Oct 14, 2018
  8. 09/21 stash: convert drop and clear to builtinPaul-Sebastian Ungureanu, Oct 14, 2018
  9. 07/21 stash: mention options in `show` synopsisPaul-Sebastian Ungureanu, Oct 14, 2018
  10. 10/21 stash: convert branch to builtinPaul-Sebastian Ungureanu, Oct 14, 2018
  11. 11/21 stash: convert pop to builtinPaul-Sebastian Ungureanu, Oct 14, 2018
  12. 12/21 stash: convert list to builtinPaul-Sebastian Ungureanu, Oct 14, 2018
  13. 13/21 stash: convert show to builtinPaul-Sebastian Ungureanu, Oct 14, 2018
  14. 14/21 stash: convert store to builtinPaul-Sebastian Ungureanu, Oct 14, 2018
  15. 15/21 stash: convert create to builtinPaul-Sebastian Ungureanu, Oct 14, 2018
  16. 16/21 stash: convert push to builtinPaul-Sebastian Ungureanu, Oct 14, 2018
  17. 20/21 stash: optimize `get_untracked_files()` and `check_changes()`Paul-Sebastian Ungureanu, Oct 14, 2018
  18. 21/21 stash: replace all `write-tree` child processes with API callsPaul-Sebastian Ungureanu, Oct 14, 2018
  19. 17/21 stash: make push -q quietPaul-Sebastian Ungureanu, Oct 14, 2018
  20. 19/21 stash: convert `stash--helper.c` into `stash.c`Paul-Sebastian Ungureanu, Oct 14, 2018
  21. Thomas GummererOct 15, 2018
  22. 18/21 stash: convert save to builtinPaul-Sebastian Ungureanu, Oct 14, 2018
  23. Thomas GummererOct 15, 2018
  24. 08/21 stash: convert apply to builtinPaul-Sebastian Ungureanu, Oct 14, 2018
  25. Johannes SchindelinOct 15, 2018
  26. Thomas GummererOct 15, 2018
  27. Thomas GummererOct 15, 2018
  28. Junio C HamanoOct 16, 2018
  29. Johannes SchindelinOct 16, 2018
  30. Thomas GummererOct 16, 2018

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.