On 10/16, Johannes Schindelin wrote:
> 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!
> 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.
> > + }
> > +
> > + 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.
> > 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.
> > 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*].
> > @@ -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 :)
> > 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.
>
> > - *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@xxxxxxxxxxxxxxxxxxxxxxxx/
[*2*]: https://public-inbox.org/git/20181015220338.GB4883@xxxxxxxxxxxxxxxxxxxxxxxx/
