Re: [PATCH 10/17] path.c: drop git_path_submodule

From: Jeff King <peff@peff.net>
Date: Aug 11, 2015, 09:53 UTC
Message-ID: <20150811095318.GB10238@sigill.intra.peff.net>
In-Reply-To: <xmqqbneeu4ys.fsf@gitster.dls.corp.google.com>

On Mon, Aug 10, 2015 at 03:57:31PM -0700, Junio C Hamano wrote:

Show 10 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Jeff King <peff@peff.net> writes:
> >
> >> There are no callers of the slightly-dangerous static-buffer
> >> git_path_submodule left. Let's drop it.
> >
> > There are a few callers added on 'pu', though.
> 
> Actually there is only one.  Here is a proposed evil merge.

Thanks for catching. I (obviously) only checked against 'next' and not 'pu'. Rather than the evil merge, we could also just drop this patch. And then either leave it be, or fix this one up as a separate topic once merged. Though it really looks like this call site is potentially dangerous, with the assignment (I think it is OK, though, because read_info_alternates does not use git_path itself).

Show 20 quoted lines

> diff --git a/submodule.c b/submodule.c
> index dfe8b7b..7ab08a1 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -120,34 +120,35 @@ void stage_updated_gitmodules(void)
>  static int add_submodule_odb(const char *path)
> [...]
>  {
>  	struct alternate_object_database *alt_odb;
> -	const char *objects_directory;
> +	struct strbuf objects_directory = STRBUF_INIT;
>  	int ret = 0;
>  
> -	objects_directory = git_path_submodule(path, "objects/");
> -	if (!is_directory(objects_directory)) {
> +	strbuf_git_path_submodule(&objects_directory, "objects/", "%s", path);
> +	if (!is_directory(objects_directory.buf)) {
>  		ret = -1;
>  		goto done;
>  	}

Hmph, the change in 6659e74 would have been a lot nicer if strbuf_git_path_submodule were already available. Most of what you are doing here is undoing that commit's strbuf/buf transition. :-/

Show 7 quoted lines

>  	/* avoid adding it twice */
>  	for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)
> -		if (alt_odb->name - alt_odb->base == strlen(objects_directory) &&
> -				!strcmp(alt_odb->base, objects_directory))
> +		if (alt_odb->name - alt_odb->base == objects_directory.len &&
> +				!strcmp(alt_odb->base, objects_directory.buf))
>  			goto done;

Not really relevant to what we're talking about here, but this is the first time I've lookd at add_submodule_odb. It really looks like the whole second half of the function could be replaced with a call to link_alt_odb_entry.

-Peff