On 08/18, Paul Sebastian Ungureanu wrote:
> On Thu, Aug 16, 2018 at 1:13 AM, Thomas Gummerer <t.gummerer@xxxxxxxxx> 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?
