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

[Junio C Hamano <gitster@xxxxxxxxx>]

Johannes Schindelin <johannes.schindelin@xxxxxx> 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 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.  You'd still want to release it at the end anyway for
good code hygiene, though.

Other than that, looks good to me.

Thanks.