From: Jeff King <peff@peff.net>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Joel Teichroeb <joel@teichroeb.net>,
Git Mailing List <git@vger.kernel.org>,
t.gummerer@gmail.com,
Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH v3 4/4] stash: implement builtin stash
Date: Sun, 28 May 2017 15:21:50 -0400 [thread overview]
Message-ID: <20170528192149.dgzibu334n5ja57e@sigill.intra.peff.net> (raw)
In-Reply-To: <CACBZZX6uXnE+BTfsiLNF2OT3Dsr-J99uUFEwcu-qK45OrU+1hQ@mail.gmail.com>
On Sun, May 28, 2017 at 08:51:07PM +0200, Ævar Arnfjörð Bjarmason wrote:
> On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb <joel@teichroeb.net> wrote:
> > Implement all git stash functionality as a builtin command
> >
> > Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
> > ---
>
> General note on this that I missed in my first E-Mail, you have ~20
> calls to argv_array_init() but none to argv_array_clear(). So you're
> leaking memory, and it obscures potential other issues with valgrind.
I haven't looked carefully at the patches, but note that the argv array
in a child_process is handled automatically by start/finish_command.
So:
> @@ -1107,9 +1111,9 @@ static int list_stash(int argc, const char
> **argv, const char *prefix)
> argv_array_pushv(&args, argv);
> argv_array_push(&args, ref_stash);
> if (cmd_log(args.argc, args.argv, prefix))
> - return 1;
> -
> - return 0;
> + ret = 1;
> + argv_array_clear(&args);
> + return ret;
> }
This one does need a clear, because it's calling an internal function.
But...
> @@ -741,13 +740,18 @@ static int do_push_stash(const char *prefix,
> const char *message,
> argv_array_push(&cp.args, "--");
> argv_array_pushv(&cp.args, argv);
> pipe_command(&cp, NULL, 0, &out, 0, NULL, 0);
> + argv_array_clear(&cp.args);
This one does not, because the array will have been cleared by calling
pipe_command (because it does the start/finish block itself; and yes,
before you go checking, start_command() clears it even when it returns
error).
> + child_process_clear(&cp);
This should not be necessary for the same reason.
> - cp2.git_cmd = 1;
> - argv_array_push(&cp2.args, "checkout-index");
> - argv_array_push(&cp2.args, "-z");
> - argv_array_push(&cp2.args, "--force");
> - argv_array_push(&cp2.args, "--stdin");
> - pipe_command(&cp2, out.buf, out.len, NULL, 0, NULL, 0);
> + child_process_init(&cp);
> + cp.git_cmd = 1;
> + argv_array_push(&cp.args, "checkout-index");
> + argv_array_push(&cp.args, "-z");
> + argv_array_push(&cp.args, "--force");
> + argv_array_push(&cp.args, "--stdin");
> + pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0);
> + argv_array_clear(&cp.args);
> + child_process_clear(&cp);
And ditto here.
I'd also encourage using CHILD_PROCESS_INIT and ARGV_ARRAY_INIT constant
initializers instead of their function-call counterparts, as that
matches our usual style.
-Peff
next prev parent reply other threads:[~2017-05-28 19:21 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-28 16:56 [PATCH v3 0/4] Implement git stash as a builtin command Joel Teichroeb
2017-05-28 16:56 ` [PATCH v3 1/4] stash: add test for stash create with no files Joel Teichroeb
2017-05-28 17:45 ` Ævar Arnfjörð Bjarmason
2017-05-29 6:35 ` Junio C Hamano
2017-05-28 16:56 ` [PATCH v3 2/4] stash: add test for stashing in a detached state Joel Teichroeb
2017-05-28 17:57 ` Ævar Arnfjörð Bjarmason
2017-05-29 6:41 ` Junio C Hamano
2017-05-28 16:56 ` [PATCH v3 3/4] close the index lock when not writing the new index Joel Teichroeb
2017-05-28 17:46 ` Ævar Arnfjörð Bjarmason
2017-05-29 6:46 ` Junio C Hamano
2017-05-28 16:56 ` [PATCH v3 4/4] stash: implement builtin stash Joel Teichroeb
2017-05-28 17:56 ` Christian Couder
2017-05-28 18:26 ` Ævar Arnfjörð Bjarmason
2017-05-28 18:31 ` Joel Teichroeb
2017-05-28 19:26 ` Jeff King
2017-05-28 18:51 ` Ævar Arnfjörð Bjarmason
2017-05-28 19:21 ` Jeff King [this message]
2017-05-29 18:18 ` Joel Teichroeb
2017-05-29 18:26 ` Ævar Arnfjörð Bjarmason
2017-06-01 3:29 ` Joel Teichroeb
2017-06-01 4:07 ` Jeff King
2017-05-29 7:16 ` Junio C Hamano
2017-05-28 19:08 ` [PATCH v3 0/4] Implement git stash as a builtin command Ævar Arnfjörð Bjarmason
2017-10-23 11:09 ` Johannes Schindelin
2017-10-23 18:35 ` Joel Teichroeb
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=20170528192149.dgzibu334n5ja57e@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=Johannes.Schindelin@gmx.de \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=joel@teichroeb.net \
--cc=t.gummerer@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).