Re: [GSoC][PATCH v7 15/26] stash: convert create to builtin

[Thomas Gummerer <t.gummerer@gmail.com>]

On 08/18, Paul Sebastian Ungureanu wrote:
> On Thu, Aug 16, 2018 at 1:13 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > On 08/08, Paul-Sebastian Ungureanu wrote:
> >>
> >>  [...]
> >>
> >> +                     ret = -1;
> >> +                     goto done;
> >> +             }
> >> +             untracked_commit_option = 1;
> >> +     }
> >> +     if (patch_mode) {
> >> +             ret = stash_patch(info, argv);
> >> +             if (ret < 0) {
> >> +                     printf_ln("Cannot save the current worktree state");
> >> +                     goto done;
> >> +             } else if (ret > 0) {
> >> +                     goto done;
> >> +             }
> >> +     } else {
> >> +             if (stash_working_tree(info, argv)) {
> >> +                     printf_ln("Cannot save the current worktree state");
> >> +                     ret = -1;
> >> +                     goto done;
> >> +             }
> >> +     }
> >> +
> >> +     if (!*stash_msg || !strlen(*stash_msg))
> >> +             strbuf_addf(&final_stash_msg, "WIP on %s", msg.buf);
> >> +     else
> >> +             strbuf_addf(&final_stash_msg, "On %s: %s\n", branch_name,
> >> +                         *stash_msg);
> >> +     *stash_msg = strbuf_detach(&final_stash_msg, NULL);
> >
> > strbuf_detach means we're taking ownership of the memory, so we'll
> > have to free it afterwards. Looking at this we may not even want to
> > re-use the 'stash_msg' variable here, but instead introduce another
> > variable for it, so it doesn't have a dual meaning in this function.
> >
> >> +
> >> +     /*
> >> +      * `parents` will be empty after calling `commit_tree()`, so there is
> >> +      * no need to call `free_commit_list()`
> >> +      */
> >> +     parents = NULL;
> >> +     if (untracked_commit_option)
> >> +             commit_list_insert(lookup_commit(the_repository, &info->u_commit), &parents);
> >> +     commit_list_insert(lookup_commit(the_repository, &info->i_commit), &parents);
> >> +     commit_list_insert(head_commit, &parents);
> >> +
> >> +     if (commit_tree(*stash_msg, strlen(*stash_msg), &info->w_tree,
> >> +                     parents, &info->w_commit, NULL, NULL)) {
> >> +             printf_ln("Cannot record working tree state");
> >> +             ret = -1;
> >> +             goto done;
> >> +     }
> >> +
> >> +done:
> >> +     strbuf_release(&commit_tree_label);
> >> +     strbuf_release(&msg);
> >> +     strbuf_release(&out);
> >> +     strbuf_release(&final_stash_msg);
> >> +     return ret;
> >> +}
> >> +
> >> +static int create_stash(int argc, const char **argv, const char *prefix)
> >> +{
> >> +     int include_untracked = 0;
> >> +     int ret = 0;
> >> +     const char *stash_msg = NULL;
> >> +     struct stash_info info;
> >> +     struct option options[] = {
> >> +             OPT_BOOL('u', "include-untracked", &include_untracked,
> >> +                      N_("include untracked files in stash")),
> >> +             OPT_STRING('m', "message", &stash_msg, N_("message"),
> >> +                      N_("stash message")),
> >> +             OPT_END()
> >> +     };
> >> +
> >> +     argc = parse_options(argc, argv, prefix, options,
> >> +                          git_stash_helper_create_usage,
> >> +                          0);
> >> +
> >> +     ret = do_create_stash(argc, argv, prefix, &stash_msg,
> >> +                           include_untracked, 0, &info);
> >
> > stash_msg doesn't have to be passed as a pointer to a pointer here, as
> > we never need the modified value after this function returns.  I think
> > just passing 'stash_msg' here instead of '&stash_msg' will make
> > 'do_create_stash' slightly easier to read.
> 
> That's right, but `do_create_stash()` is also called by
> `do_push_stash()`, which will need the modified value.

Ah right, I didn't read that far yet when leaving this comment :)

Reading the original push_stash again though, do we actually need the
modified value in 'do_push_stash()'?  The original lines are:

	create_stash -m "$stash_msg" -u "$untracked" -- "$@"
	store_stash -m "$stash_msg" -q $w_commit ||
	die "$(gettext "Cannot save the current status")"

'$stash_msg' gets passed in to 'create_stash()', but is the
'stash_msg' variable inside of 'create_stash()' is local and only the
local copy is modified, so 'store_stash()' would still get the
original.  Or am I missing something?