From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Thomas Gummerer <t.gummerer@gmail.com>
Cc: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>,
git@vger.kernel.org
Subject: Re: [PATCH v10 00/21] Convert "git stash" to C builtin
Date: Tue, 16 Oct 2018 12:22:30 +0200 (DST) [thread overview]
Message-ID: <nycvar.QRO.7.76.6.1810161205530.4546@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20181015221040.GD4883@hank.intra.tgummerer.com>
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.
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.
> + }
> +
> + 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...
> 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.
> 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.
> @@ -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...
> 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?
> - *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
next prev parent reply other threads:[~2018-10-16 10:22 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <https://public-inbox.org/git/cover.1537913094.git.ungureanupaulsebastian@gmail.com/>
2018-10-14 22:11 ` [PATCH v10 00/21] Convert "git stash" to C builtin Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 01/21] sha1-name.c: add `get_oidf()` which acts like `get_oid()` Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 02/21] strbuf.c: add `strbuf_join_argv()` Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 03/21] stash: improve option parsing test coverage Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 04/21] t3903: modernize style Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 05/21] stash: rename test cases to be more descriptive Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 06/21] stash: add tests for `git stash show` config Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 07/21] stash: mention options in `show` synopsis Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 08/21] stash: convert apply to builtin Paul-Sebastian Ungureanu
2018-10-15 9:10 ` Johannes Schindelin
2018-10-15 20:32 ` Thomas Gummerer
2018-10-14 22:11 ` [PATCH v10 09/21] stash: convert drop and clear " Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 10/21] stash: convert branch " Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 11/21] stash: convert pop " Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 12/21] stash: convert list " Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 13/21] stash: convert show " Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 14/21] stash: convert store " Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 15/21] stash: convert create " Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 16/21] stash: convert push " Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 17/21] stash: make push -q quiet Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 18/21] stash: convert save to builtin Paul-Sebastian Ungureanu
2018-10-15 22:05 ` Thomas Gummerer
2018-10-14 22:11 ` [PATCH v10 19/21] stash: convert `stash--helper.c` into `stash.c` Paul-Sebastian Ungureanu
2018-10-15 22:03 ` Thomas Gummerer
2018-10-14 22:11 ` [PATCH v10 20/21] stash: optimize `get_untracked_files()` and `check_changes()` Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 21/21] stash: replace all `write-tree` child processes with API calls Paul-Sebastian Ungureanu
2018-10-15 22:10 ` [PATCH v10 00/21] Convert "git stash" to C builtin Thomas Gummerer
2018-10-16 3:41 ` Junio C Hamano
2018-10-16 10:22 ` Johannes Schindelin [this message]
2018-10-16 19:59 ` Thomas Gummerer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=nycvar.QRO.7.76.6.1810161205530.4546@tvgsbejvaqbjf.bet \
--to=johannes.schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=t.gummerer@gmail.com \
--cc=ungureanupaulsebastian@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).