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