Re: [PATCH v10 00/21] Convert "git stash" to C builtin
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Oct 16, 2018, 10:22 UTC
- Message-ID
- <nycvar.QRO.7.76.6.1810161205530.4546@tvgsbejvaqbjf.bet>
- In-Reply-To
- <20181015221040.GD4883@hank.intra.tgummerer.com>
Hi Thomas,
On Mon, 15 Oct 2018, Thomas Gummerer wrote:
Show 9 quoted lines
> 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.
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.
Show 8 quoted lines
> 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.
Show 8 quoted lines
> + }
> +
> + ret = apply_cached(&out);
> + strbuf_release(&out);
> + if (ret)
> -+ return -1;
> ++ return error(_("Conflicts in index."
> ++ "Try without --index."));Same here.
Show 6 quoted lines
> +
> + discard_cache();
> + read_cache();
> + if (write_cache_as_tree(&index_tree, 0, NULL))
> -+ return -1;
> ++ return error(_("Could not save index tree"));And here.
Show 6 quoted lines
> 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...
Show 11 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.
Show 11 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.
Show 10 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.
Show 6 quoted lines
> 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...
Show 7 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?
Show 5 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)?
Thank you! Dscho