Hi Christian,
[please cull a *lot* more of the quoted mail when you do not reference any
of it... Thanks]
On Thu, 5 Apr 2018, Christian Couder wrote:
> On Thu, Apr 5, 2018 at 4:28 AM, Joel Teichroeb <joel@xxxxxxxxxxxxx> wrote:
> >
> > [...]
> > +
> > + revision = info->revision.buf;
> > +
> > + if (get_oid(revision, &info->w_commit)) {
> > + error(_("%s is not a valid reference"), revision);
> > + free_stash_info(info);
> > + return -1;
>
> Maybe:
>
> free_stash_info(info);
> return error(_("%s is not a valid reference"), revision);
>
> to save one line and be more consistent with above.
No. The parameter `revision` of the `error()` call is assigned just above
the `if()` block and clearly points into the `info` structure. So you must
not release that `info` before printing the error. The order of statements
is correct.
> > + if (grab_oid(&info->b_commit, "%s^1", revision) ||
> > + grab_oid(&info->w_tree, "%s:", revision) ||
> > + grab_oid(&info->b_tree, "%s^1:", revision) ||
> > + grab_oid(&info->i_tree, "%s^2:", revision)) {
> > +
> > + error(_("'%s' is not a stash-like commit"), revision);
> > + free_stash_info(info);
> > + return -1;
>
> Here also.
Yes, here, too, `revision` points at a field inside `info`, so we must not
release it before using it.
> > + if (info->has_u) {
> > + if (restore_untracked(&info->u_tree))
> > + return error(_("Could not restore untracked files from stash"));
> > + }
>
> Maybe:
>
> if (info->has_u && restore_untracked(&info->u_tree))
> return error(_("Could not restore untracked files from stash"));
I agree with this, as it avoids an unncessary indentation level.
> So maybe we can get rid of `result` and have something like:
>
> if (argc < 1) {
> error(_("at least one argument is required"));
> usage_with_options(git_stash_helper_usage, options);
> }
>
> if (!strcmp(argv[0], "apply"))
> return apply_stash(argc, argv, prefix);
... except we have to use !!apply_stash() here: apply_stash() probably
returns -1 in case of error (at least that would be consistent with our
coding conventions), and the return value from cmd_*() is handed to exit()
as exit status. The `!!` trick turns any non-zero value into a 1, also
consistent with our coding conventions where we set exit code 1 upon error
in the "business logic".
>
> error(_("unknown subcommand: %s"), argv[0]);
> usage_with_options(git_stash_helper_usage, options);
> }
Ciao,
Dscho
