On 08/08, Paul-Sebastian Ungureanu wrote:
> Add stash create to the helper.
>
> Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@xxxxxxxxx>
> ---
> builtin/stash--helper.c | 406 ++++++++++++++++++++++++++++++++++++++++
> git-stash.sh | 2 +-
> 2 files changed, 407 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> index 5ff810f8c..a4e57899b 100644
> --- a/builtin/stash--helper.c
> +++ b/builtin/stash--helper.c
> @@ -21,6 +21,7 @@ static const char * const git_stash_helper_usage[] = {
> N_("git stash--helper branch <branchname> [<stash>]"),
> N_("git stash--helper clear"),
> N_("git stash--helper store [-m|--message <message>] [-q|--quiet] <commit>"),
> + N_("git stash--helper create [<message>]"),
> NULL
> };
>
> @@ -64,6 +65,11 @@ static const char * const git_stash_helper_store_usage[] = {
> NULL
> };
>
> +static const char * const git_stash_helper_create_usage[] = {
> + N_("git stash--helper create [<message>]"),
> + NULL
> +};
> +
> static const char *ref_stash = "refs/stash";
> static int quiet;
> static struct strbuf stash_index_path = STRBUF_INIT;
> @@ -781,6 +787,404 @@ static int store_stash(int argc, const char **argv, const char *prefix)
> return do_store_stash(argv[0], stash_msg, quiet);
> }
>
> [...]
>
> +
> +static int do_create_stash(int argc, const char **argv, const char *prefix,
> + const char **stash_msg, int include_untracked,
> + int patch_mode, struct stash_info *info)
> +{
> + int untracked_commit_option = 0;
> + int ret = 0;
> + int subject_len;
> + int flags;
> + const char *head_short_sha1 = NULL;
> + const char *branch_ref = NULL;
> + const char *head_subject = NULL;
> + const char *branch_name = "(no branch)";
> + struct commit *head_commit = NULL;
> + struct commit_list *parents = NULL;
> + struct strbuf msg = STRBUF_INIT;
> + struct strbuf commit_tree_label = STRBUF_INIT;
> + struct strbuf out = STRBUF_INIT;
> + struct strbuf final_stash_msg = STRBUF_INIT;
> +
> + read_cache_preload(NULL);
> + refresh_cache(REFRESH_QUIET);
> +
> + if (!check_changes(argv, include_untracked, prefix)) {
> + ret = 1;
> + goto done;
I wonder if we can just 'exit(0)' here, instead of returning. This
whole command is a builtin, and I *think* outside of 'libgit.a' exiting
early is fine. It does mean that we're not free'ing the memory
though, which means a leak checker would probably complain. So
dunno. It would simplify the code a little, but not sure it's worth it.
> + }
> +
> + if (get_oid("HEAD", &info->b_commit)) {
> + fprintf_ln(stderr, "You do not have the initial commit yet");
> + ret = -1;
> + goto done;
> + } else {
> + head_commit = lookup_commit(the_repository, &info->b_commit);
> + }
> +
> + branch_ref = resolve_ref_unsafe("HEAD", 0, NULL, &flags);
> + if (flags & REF_ISSYMREF)
> + branch_name = strrchr(branch_ref, '/') + 1;
> + head_short_sha1 = find_unique_abbrev(&head_commit->object.oid,
> + DEFAULT_ABBREV);
> + subject_len = find_commit_subject(get_commit_buffer(head_commit, NULL),
> + &head_subject);
> + strbuf_addf(&msg, "%s: %s %.*s\n", branch_name, head_short_sha1,
> + subject_len, head_subject);
I think this can be written in a slightly simpler way:
head_short_sha1 = find_unique_abbrev(&head_commit->object.oid,
DEFAULT_ABBREV);
strbuf_addf(&msg, "%s: %s", branch_name, head_short_sha1);
pp_commit_easy(CMIT_FMT_ONELINE, head_commit, &msg);
strbuf_addch(&msg, '\n');
The other advantage this brings is that it is consistent with other
places where we print/use the subject of a commit (e.g. in 'git reset
--hard').
> +
> + strbuf_addf(&commit_tree_label, "index on %s\n", msg.buf);
> + commit_list_insert(head_commit, &parents);
> + if (write_cache_as_tree(&info->i_tree, 0, NULL) ||
> + commit_tree(commit_tree_label.buf, commit_tree_label.len,
> + &info->i_tree, parents, &info->i_commit, NULL, NULL)) {
> + fprintf_ln(stderr, "Cannot save the current index state");
Looks like this message is translated in the current 'git stash'
implementation, so it should be here as well. Same for the messages
below.
> + ret = -1;
> + goto done;
> + }
> +
> + if (include_untracked && get_untracked_files(argv, 1,
> + include_untracked, &out)) {
> + if (save_untracked_files(info, &msg, &out)) {
> + printf_ln("Cannot save the untracked files");
Why does this go to stdout, whereas "Cannot save the current index
state" above goes to stderr? In the shell version of git stash these
all go to stderr fwiw. There are a few similar cases, it would
probably be worth going through all the print statements here and see
if they need to be translated, and to which output stream they should
go.
> + 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.
> +
> + if (!ret)
> + printf_ln("%s", oid_to_hex(&info.w_commit));
> +
> + /*
> + * ret can be 1 if there were no changes. In this case, we should
> + * not error out.
> + */
> + return ret < 0;
> +}
> +
> int cmd_stash__helper(int argc, const char **argv, const char *prefix)
> {
> pid_t pid = getpid();
> @@ -817,6 +1221,8 @@ int cmd_stash__helper(int argc, const char **argv, const char *prefix)
> return !!show_stash(argc, argv, prefix);
> else if (!strcmp(argv[0], "store"))
> return !!store_stash(argc, argv, prefix);
> + else if (!strcmp(argv[0], "create"))
> + return !!create_stash(argc, argv, prefix);
>
> usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
> git_stash_helper_usage, options);
> diff --git a/git-stash.sh b/git-stash.sh
> index 5739c5152..ab06e4ffb 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -425,7 +425,7 @@ clear)
> ;;
> create)
> shift
> - create_stash -m "$*" && echo "$w_commit"
> + git stash--helper create --message "$*"
> ;;
> store)
> shift
> --
> 2.18.0.573.g56500d98f
>
