Re: [PATCH v5 2/5] stash: convert apply to builtin

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 5, 2018, 13:34 UTC
Message-ID: <nycvar.QRO.7.76.6.1804051528560.55@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>
In-Reply-To: <CAP8UFD3SM9e+_VGpxVNzMoEYnyzkF0EzjAPjvbFakr9t9Z-muQ@mail.gmail.com>

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:

Show 17 quoted lines

> On Thu, Apr 5, 2018 at 4:28 AM, Joel Teichroeb <joel@teichroeb.net> 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.

Show 10 quoted lines

> > +       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.

Show 9 quoted lines

> > +       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.

Show 9 quoted lines

> 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