Web lists-archives.com

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




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