On 08/18, Paul Sebastian Ungureanu wrote:

On Thu, Aug 16, 2018 at 1:13 AM, Thomas Gummerer [off-list ref] wrote:

quoted

On 08/08, Paul-Sebastian Ungureanu wrote:

quoted

 [...]

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

quoted

+
+     /*
+      * `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?