Re: [PATCH v2 2/2] rev-parse: fix several options when running in a subdirectory

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Feb 17, 2017, 16:53 UTC
Message-ID: <alpine.DEB.2.20.1702171749180.3496@virtualbox>
In-Reply-To: <xmqqo9y9lvqk.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Fri, 10 Feb 2017, Junio C Hamano wrote:

Show 56 quoted lines

> 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...

Show 10 quoted lines

> 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