Re: [PATCH 2/4] teach ref iteration module about submodules

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 30, 2010, 20:37 UTC
Message-ID: <7vvd9045q3.fsf@alter.siamese.dyndns.org>
In-Reply-To: <26d63936d5a71de6a8e8aa823a69ddda8b999e4b.1277923844.git.hvoigt@hvoigt.net>

Heiko Voigt <hvoigt@hvoigt.net> writes:

Show 8 quoted lines

> +char *git_path_submodule(const char *path, const char *fmt, ...)
> +{
> +...
> +	strbuf_addch(&buf, '/');
> +
> +	strncpy(pathname, buf.buf, PATH_MAX);
> +	if (pathname[PATH_MAX-1] != '\0')
> +		return bad_path;

This may not be wrong per-se, but having strncpy() NUL-pad the remainder of the buffer only because you want to check overlong path by inspecting pathname[PATH_MAX-1] sounds somewhat stupid, no? Your buf.len knows how long the path is already at this point, doesn't it?

Show 10 quoted lines

> @@ -322,11 +352,12 @@ void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname)
>  	for_each_rawref(warn_if_dangling_symref, &data);
>  }
>  
> -static struct ref_list *get_loose_refs(void)
> +static struct ref_list *get_loose_refs(const char *submodule)
>  {
> -	if (!cached_refs.did_loose) {
> -		cached_refs.loose = get_ref_dir("refs", NULL);
> +	if (!cached_refs.did_loose || cached_refs.submodule != submodule) {

Do you really mean "!=" here? I do not see anywhere that you are "intern"-ing (a la Lisp implementations) names of submodules to make address comparison work as a cheap equality check.

> +		cached_refs.loose = get_ref_dir(submodule, "refs", NULL);

What happened to the old ref_list that had the refs from the toplevel project (or the last submodule you visited) if your "did_loose" is true? Leakage?

Show 5 quoted lines

>  		cached_refs.did_loose = 1;
> +		cached_refs.submodule = submodule;
>  	}
>  	return cached_refs.loose;
>  }

Once you grabbed loose refs for _any_ repository, you will have did_loose set, so the flag has now became pretty much useless.

More importantly, I wonder if you would instead want to have cached_refs structure for each submodule separately, or at least not nuke the cached_refs structure for the top-level project, only because you wanted to peek into one submodule. While your for_each_ref() is walking the refs of top-level project, its callback may stomp on the cached_refs by asking about submodule refs, and there is nothing in this code structure to help catching such a bug, is there?