Heiko Voigt <hvoigt@xxxxxxxxxx> writes:
> +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?
> @@ -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?
> 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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html