From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
"Michael Rappazzo" <rappazzo@gmail.com>
Subject: Re: [PATCH v2 2/2] rev-parse: fix several options when running in a subdirectory
Date: Fri, 17 Feb 2017 17:53:13 +0100 (CET) [thread overview]
Message-ID: <alpine.DEB.2.20.1702171749180.3496@virtualbox> (raw)
In-Reply-To: <xmqqo9y9lvqk.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Fri, 10 Feb 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> > index ff13e59e1db..84af2802f6f 100644
> > --- a/builtin/rev-parse.c
> > +++ b/builtin/rev-parse.c
> > @@ -545,6 +545,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> > unsigned int flags = 0;
> > const char *name = NULL;
> > struct object_context unused;
> > + struct strbuf buf = STRBUF_INIT;
> >
> > if (argc > 1 && !strcmp("--parseopt", argv[1]))
> > return cmd_parseopt(argc - 1, argv + 1, prefix);
> > @@ -599,7 +600,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> > if (!strcmp(arg, "--git-path")) {
> > if (!argv[i + 1])
> > die("--git-path requires an argument");
> > - puts(git_path("%s", argv[i + 1]));
> > + strbuf_reset(&buf);
> > + puts(relative_path(git_path("%s", argv[i + 1]),
> > + prefix, &buf));
> > i++;
> > continue;
> > }
> > @@ -821,8 +824,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> > continue;
> > }
> > if (!strcmp(arg, "--git-common-dir")) {
> > - const char *pfx = prefix ? prefix : "";
> > - puts(prefix_filename(pfx, strlen(pfx), get_git_common_dir()));
> > + strbuf_reset(&buf);
> > + puts(relative_path(get_git_common_dir(),
> > + prefix, &buf));
> > continue;
> > }
> > if (!strcmp(arg, "--is-inside-git-dir")) {
> > @@ -845,7 +849,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> > die(_("Could not read the index"));
> > if (the_index.split_index) {
> > const unsigned char *sha1 = the_index.split_index->base_sha1;
> > - puts(git_path("sharedindex.%s", sha1_to_hex(sha1)));
> > + const char *path = git_path("sharedindex.%s", sha1_to_hex(sha1));
> > + strbuf_reset(&buf);
> > + puts(relative_path(path, prefix, &buf));
> > }
> > continue;
> > }
> > @@ -906,5 +912,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> > die_no_single_rev(quiet);
> > } else
> > show_default();
> > + strbuf_release(&buf);
>
> This uses "reset then use" pattern for repeated use of strbuf, and
> causes the string last held in the strbuf to leak on early return,
... which cannot happen due to the lack of an early return...
> which can be mitigated by using "use then reset" pattern. I.e.
>
> if (!strcmp(arg, "--git-common-dir")) {
> puts(relative_path(get_git_common_dir(),
> prefix, &buf));
> strbuf_reset(&buf);
> continue;
> }
>
> I'd think.
This would not release the memory, though:
#define strbuf_reset(sb) strbuf_setlen(sb, 0)
and
static inline void strbuf_setlen(struct strbuf *sb, size_t len)
{
if (len > (sb->alloc ? sb->alloc - 1 : 0))
die("BUG: strbuf_setlen() beyond buffer");
sb->len = len;
sb->buf[len] = '\0';
}
There is not a single free() statement there.
So the "use then reset" scheme would leak *just the same*.
> You'd still want to release it at the end anyway for good code hygiene,
> though.
Which I do.
Technically, this is not even necessary because all of the cmd_*()
functions are immediately followed by a call to exit(). Wasn't that the
genius idea in the early Git days, that we could simply get away with
sloppy memory management because the program exit()s shortly afterwards,
anyway? ;-)
In any case, I adjusted the commit message to clarify why the "reset then
use" scheme is correct here.
Ciao,
Johannes
next prev parent reply other threads:[~2017-02-17 16:53 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-02-08 12:17 [PATCH] rev-parse --git-path: fix output when running in a subdirectory Johannes Schindelin
2017-02-08 18:47 ` Junio C Hamano
2017-02-09 21:05 ` Johannes Schindelin
2017-02-09 21:50 ` Junio C Hamano
2017-02-10 4:21 ` Jeff King
2017-02-09 9:48 ` Duy Nguyen
2017-02-09 13:46 ` Mike Rappazzo
2017-02-09 21:11 ` Johannes Schindelin
2017-02-09 21:33 ` Junio C Hamano
2017-02-09 22:11 ` Johannes Schindelin
2017-02-09 22:54 ` Junio C Hamano
2017-02-10 3:52 ` Mike Rappazzo
2017-02-10 15:44 ` Johannes Schindelin
2017-02-10 15:33 ` [PATCH v2 0/2] Fix bugs in rev-parse's output when run " Johannes Schindelin
2017-02-10 15:33 ` [PATCH v2 1/2] rev-parse tests: add tests executed from " Johannes Schindelin
2017-02-10 18:50 ` Junio C Hamano
2017-02-17 16:55 ` Johannes Schindelin
2017-02-10 20:25 ` Junio C Hamano
2017-02-17 16:57 ` Johannes Schindelin
2017-02-10 15:33 ` [PATCH v2 2/2] rev-parse: fix several options when running in " Johannes Schindelin
2017-02-10 18:57 ` Junio C Hamano
2017-02-17 16:53 ` Johannes Schindelin [this message]
2017-02-10 18:59 ` [PATCH v2 0/2] Fix bugs in rev-parse's output when run " Junio C Hamano
2017-02-17 16:58 ` [PATCH v3 " Johannes Schindelin
2017-02-17 16:59 ` [PATCH v3 1/2] rev-parse tests: add tests executed from " Johannes Schindelin
2017-02-17 16:59 ` [PATCH v3 2/2] rev-parse: fix several options when running in " Johannes Schindelin
2017-02-17 18:25 ` [PATCH v3 0/2] Fix bugs in rev-parse's output when run " Junio C Hamano
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=alpine.DEB.2.20.1702171749180.3496@virtualbox \
--to=johannes.schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=pclouds@gmail.com \
--cc=rappazzo@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).