From: Thomas Gummerer <t.gummerer@gmail.com>
To: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v10 19/21] stash: convert `stash--helper.c` into `stash.c`
Date: Mon, 15 Oct 2018 23:03:38 +0100 [thread overview]
Message-ID: <20181015220338.GB4883@hank.intra.tgummerer.com> (raw)
In-Reply-To: <004f2b2e3e8c4d1069a808034f3d6fe16853085c.1539553398.git.ungureanupaulsebastian@gmail.com>
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 <ungureanupaulsebastian@gmail.com>
> ---
> @@ -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, &info->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(&stash_msg_buf, "On %s: %s", branch_name,
> *stash_msg);
> - *stash_msg = strbuf_detach(&stash_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", &include_untracked,
> - N_("include untracked files in stash")),
> - OPT_STRING('m', "message", &stash_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(&stash_msg_buf, argc - 1, ++argv, ' ');
> + stash_msg = stash_msg_buf.buf;
stash_msg is just a pointer to stash_msg_buf.buf here..
>
> memset(&ps, 0, sizeof(ps));
> - ret = do_create_stash(ps, &stash_msg, include_untracked, 0, &info,
> - NULL, 0);
> + ret = do_create_stash(ps, &stash_msg, 0, 0, &info, NULL, 0);
>
> if (!ret)
> printf_ln("%s", oid_to_hex(&info.w_commit));
>
> + strbuf_release(&stash_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
they pass in (although it may be pointing to something different than
what was passed in).
diff --git a/builtin/stash.c b/builtin/stash.c
index e945c13c42..3b50f4bd53 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1199,6 +1199,7 @@ static int do_create_stash(struct pathspec ps, char **stash_msg,
else
strbuf_addf(&stash_msg_buf, "On %s: %s", branch_name,
*stash_msg);
+ free(*stash_msg);
*stash_msg = xstrdup(stash_msg_buf.buf);
/*
@@ -1241,7 +1242,7 @@ static int create_stash(int argc, const char **argv, const char *prefix)
/* Starting with argv[1], since argv[0] is "create" */
strbuf_join_argv(&stash_msg_buf, argc - 1, ++argv, ' ');
- stash_msg = stash_msg_buf.buf;
+ stash_msg = xstrdup(stash_msg_buf.buf);
memset(&ps, 0, sizeof(ps));
ret = do_create_stash(ps, &stash_msg, 0, 0, &info, NULL, 0);
@@ -1522,7 +1523,7 @@ static int save_stash(int argc, const char **argv, const char *prefix)
}
memset(&ps, 0, sizeof(ps));
- ret = do_push_stash(ps, stash_msg, quiet, keep_index, patch_mode,
+ ret = do_push_stash(ps, xstrdup_or_null(stash_msg), quiet, keep_index, patch_mode,
include_untracked);
strbuf_release(&buf);
The above, as well as the some of the changes I quoted above should
probably be squashed in to the relevant patches, rather than being
part of this patch ("stash: convert create to builtin", and "stash:
convert save to builtin").
next prev parent reply other threads:[~2018-10-15 22:03 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <https://public-inbox.org/git/cover.1537913094.git.ungureanupaulsebastian@gmail.com/>
2018-10-14 22:11 ` [PATCH v10 00/21] Convert "git stash" to C builtin Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 01/21] sha1-name.c: add `get_oidf()` which acts like `get_oid()` Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 02/21] strbuf.c: add `strbuf_join_argv()` Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 03/21] stash: improve option parsing test coverage Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 04/21] t3903: modernize style Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 05/21] stash: rename test cases to be more descriptive Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 06/21] stash: add tests for `git stash show` config Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 07/21] stash: mention options in `show` synopsis Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 08/21] stash: convert apply to builtin Paul-Sebastian Ungureanu
2018-10-15 9:10 ` Johannes Schindelin
2018-10-15 20:32 ` Thomas Gummerer
2018-10-14 22:11 ` [PATCH v10 09/21] stash: convert drop and clear " Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 10/21] stash: convert branch " Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 11/21] stash: convert pop " Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 12/21] stash: convert list " Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 13/21] stash: convert show " Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 14/21] stash: convert store " Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 15/21] stash: convert create " Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 16/21] stash: convert push " Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 17/21] stash: make push -q quiet Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 18/21] stash: convert save to builtin Paul-Sebastian Ungureanu
2018-10-15 22:05 ` Thomas Gummerer
2018-10-14 22:11 ` [PATCH v10 19/21] stash: convert `stash--helper.c` into `stash.c` Paul-Sebastian Ungureanu
2018-10-15 22:03 ` Thomas Gummerer [this message]
2018-10-14 22:11 ` [PATCH v10 20/21] stash: optimize `get_untracked_files()` and `check_changes()` Paul-Sebastian Ungureanu
2018-10-14 22:11 ` [PATCH v10 21/21] stash: replace all `write-tree` child processes with API calls Paul-Sebastian Ungureanu
2018-10-15 22:10 ` [PATCH v10 00/21] Convert "git stash" to C builtin Thomas Gummerer
2018-10-16 3:41 ` Junio C Hamano
2018-10-16 10:22 ` Johannes Schindelin
2018-10-16 19:59 ` Thomas Gummerer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181015220338.GB4883@hank.intra.tgummerer.com \
--to=t.gummerer@gmail.com \
--cc=git@vger.kernel.org \
--cc=ungureanupaulsebastian@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).