Re: [PATCH v10 19/21] stash: convert `stash--helper.c` into `stash.c`
On 10/15, Paul-Sebastian Ungureanu wrote:
> The old shell script `git-stash.sh` was removed and replaced
> entirely by `builtin/stash.c`. In order to do that, `create` and
> `push` were adapted to work without `stash.sh`. For example, before
> this commit, `git stash create` called `git stash--helper create
> --message "$*"`. If it called `git stash--helper create "$@"`, then
> some of these changes wouldn't have been necessary.
>
> This commit also removes the word `helper` since now stash is
> called directly and not by a shell script.
>
> Signed-off-by: Paul-Sebastian Ungureanu
> ---
> @@ -1138,7 +1133,6 @@ static int do_create_stash(struct pathspec ps, char
> **stash_msg,
> fprintf_ln(stderr, _("You do not have "
>"the initial commit yet"));
> ret = -1;
> - *stash_msg = NULL;
> goto done;
> } else {
> head_commit = lookup_commit(the_repository, >b_commit);
> @@ -1146,7 +1140,6 @@ static int do_create_stash(struct pathspec ps, char
> **stash_msg,
>
> if (!check_changes(ps, include_untracked)) {
> ret = 1;
> - *stash_msg = NULL;
> goto done;
> }
>
> @@ -1167,7 +1160,6 @@ static int do_create_stash(struct pathspec ps, char
> **stash_msg,
> fprintf_ln(stderr, _("Cannot save the current "
>"index state"));
> ret = -1;
> - *stash_msg = NULL;
> goto done;
> }
>
> @@ -1178,14 +1170,12 @@ static int do_create_stash(struct pathspec ps, char
> **stash_msg,
> fprintf_ln(stderr, _("Cannot save "
>"the untracked files"));
> ret = -1;
> - *stash_msg = NULL;
> goto done;
> }
> untracked_commit_option = 1;
> }
> if (patch_mode) {
> ret = stash_patch(info, ps, patch, quiet);
> - *stash_msg = NULL;
> if (ret < 0) {
> if (!quiet)
> fprintf_ln(stderr, _("Cannot save the current "
> @@ -1200,7 +1190,6 @@ static int do_create_stash(struct pathspec ps, char
> **stash_msg,
> fprintf_ln(stderr, _("Cannot save the current "
>"worktree state"));
> ret = -1;
> - *stash_msg = NULL;
> goto done;
> }
> }
> @@ -1210,7 +1199,7 @@ static int do_create_stash(struct pathspec ps, char
> **stash_msg,
> else
> strbuf_addf(_msg_buf, "On %s: %s", branch_name,
> *stash_msg);
> - *stash_msg = strbuf_detach(_msg_buf, NULL);
> + *stash_msg = xstrdup(stash_msg_buf.buf);
>
> /*
>* `parents` will be empty after calling `commit_tree()`, so there is
> @@ -1244,30 +1233,23 @@ static int do_create_stash(struct pathspec ps, char
> **stash_msg,
>
> static int create_stash(int argc, const char **argv, const char *prefix)
> {
> - int include_untracked = 0;
> int ret = 0;
> char *stash_msg = NULL;
> struct stash_info info;
> struct pathspec ps;
> - struct option options[] = {
> - OPT_BOOL('u', "include-untracked", _untracked,
> - N_("include untracked files in stash")),
> - OPT_STRING('m', "message", _msg, N_("message"),
> - N_("stash message")),
> - OPT_END()
> - };
> + struct strbuf stash_msg_buf = STRBUF_INIT;
>
> - argc = parse_options(argc, argv, prefix, options,
> - git_stash_helper_create_usage,
> - 0);
> + /* Starting with argv[1], since argv[0] is "create" */
> + strbuf_join_argv(_msg_buf, argc - 1, ++argv, ' ');
> + stash_msg = stash_msg_buf.buf;
stash_msg is just a pointer to stash_msg_buf.buf here..
>
> memset(, 0, sizeof(ps));
> - ret = do_create_stash(ps, _msg, include_untracked, 0, ,
> - NULL, 0);
> + ret = do_create_stash(ps, _msg, 0, 0, , NULL, 0);
>
> if (!ret)
> printf_ln("%s", oid_to_hex(_commit));
>
> + strbuf_release(_msg_buf);
We release the strbuf here, which means stash_msg_buf.buf is now
'strbuf_slopbuf', which is a global variable and can't be free'd. If
stash_msg is not changed within do_create_stash, it is now pointing to
'strbuf_slopbuf', and we try to free that below, which makes git
crash in t3903.44, which breaks bisection.
> free(stash_msg);
>
> /*
I think the following diff fixes memory management, by making
do_push_stash responsible for freeing stash_msg when it's done with
it, while the callers of do_create_stash have to free the parameter
[PATCH v10 19/21] stash: convert `stash--helper.c` into `stash.c`
The old shell script `git-stash.sh` was removed and replaced
entirely by `builtin/stash.c`. In order to do that, `create` and
`push` were adapted to work without `stash.sh`. For example, before
this commit, `git stash create` called `git stash--helper create
--message "$*"`. If it called `git stash--helper create "$@"`, then
some of these changes wouldn't have been necessary.
This commit also removes the word `helper` since now stash is
called directly and not by a shell script.
Signed-off-by: Paul-Sebastian Ungureanu
---
.gitignore | 1 -
Makefile | 3 +-
builtin.h| 2 +-
builtin/{stash--helper.c => stash.c} | 166 +++
git-stash.sh | 153
git.c| 2 +-
6 files changed, 96 insertions(+), 231 deletions(-)
rename builtin/{stash--helper.c => stash.c} (90%)
delete mode 100755 git-stash.sh
diff --git a/.gitignore b/.gitignore
index b59661cb88..ffceea7d59 100644
--- a/.gitignore
+++ b/.gitignore
@@ -157,7 +157,6 @@
/git-show-ref
/git-stage
/git-stash
-/git-stash--helper
/git-status
/git-stripspace
/git-submodule
diff --git a/Makefile b/Makefile
index f900c68e69..ac1787d113 100644
--- a/Makefile
+++ b/Makefile
@@ -617,7 +617,6 @@ SCRIPT_SH += git-quiltimport.sh
SCRIPT_SH += git-rebase.sh
SCRIPT_SH += git-remote-testgit.sh
SCRIPT_SH += git-request-pull.sh
-SCRIPT_SH += git-stash.sh
SCRIPT_SH += git-submodule.sh
SCRIPT_SH += git-web--browse.sh
@@ -1093,7 +1092,7 @@ BUILTIN_OBJS += builtin/shortlog.o
BUILTIN_OBJS += builtin/show-branch.o
BUILTIN_OBJS += builtin/show-index.o
BUILTIN_OBJS += builtin/show-ref.o
-BUILTIN_OBJS += builtin/stash--helper.o
+BUILTIN_OBJS += builtin/stash.o
BUILTIN_OBJS += builtin/stripspace.o
BUILTIN_OBJS += builtin/submodule--helper.o
BUILTIN_OBJS += builtin/symbolic-ref.o
diff --git a/builtin.h b/builtin.h
index 317bc338f7..e60f0fc58f 100644
--- a/builtin.h
+++ b/builtin.h
@@ -223,7 +223,7 @@ extern int cmd_show(int argc, const char **argv, const char
*prefix);
extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
extern int cmd_show_index(int argc, const char **argv, const char *prefix);
extern int cmd_status(int argc, const char **argv, const char *prefix);
-extern int cmd_stash__helper(int argc, const char **argv, const char *prefix);
+extern int cmd_stash(int argc, const char **argv, const char *prefix);
extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
extern int cmd_submodule__helper(int argc, const char **argv, const char
*prefix);
extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
diff --git a/builtin/stash--helper.c b/builtin/stash.c
similarity index 90%
rename from builtin/stash--helper.c
rename to builtin/stash.c
index a0413f5d00..e945c13c42 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash.c
@@ -16,75 +16,70 @@
#define INCLUDE_ALL_FILES 2
-static const char * const git_stash_helper_usage[] = {
- N_("git stash--helper list []"),
- N_("git stash--helper show [] []"),
- N_("git stash--helper drop [-q|--quiet] []"),
- N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet]
[]"),
- N_("git stash--helper branch []"),
- N_("git stash--helper clear"),
- N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index]
[-q|--quiet]\n"
+static const char * const git_stash_usage[] = {
+ N_("git stash list []"),
+ N_("git stash show [] []"),
+ N_("git stash drop [-q|--quiet] []"),
+ N_("git stash ( pop | apply ) [--index] [-q|--quiet] []"),
+ N_("git stash branch []"),
+ N_("git stash clear"),
+ N_("git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
" [-u|--include-untracked] [-a|--all] [-m|--message
]\n"
" [--] [...]]"),
- N_("git stash--helper save [-p|--patch] [-k|--[no-]keep-index]
[-q|--quiet]\n"
+ N_("git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
" [-u|--include-untracked] [-a|--all] []"),
NULL
};
-static const char * const git_stash_helper_list_usage[] = {
- N_("git stash--helper list []"),
+static const char * const git_stash_list_usage[] = {
+ N_("git stash list []"),
NULL
};
-static const char * const git_stash_helper_show_usage[] = {
- N_("git stash--helper show [] []"),
+static const char * const git_stash_show_usage[] = {
+ N_("git stash show [] []"),
NULL
};
-static const char * const git_stash_helper_drop_usage[] = {
- N_("git stash--helper drop [-q|--quiet] []"),
+static const char * const git_stash_drop_usage[] = {
+ N_("git stash drop [-q|--quiet] []"),
NULL
};
-static const char * const git_stash_helper_pop_usage[] = {
- N_("git stash--helper pop [--index] [-q|--quiet] []"),
