Re: [PATCH 2/4] teach ref iteration module about submodules
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:49:02
Heiko Voigt [off-list ref] 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?
quoted hunk ↗ jump to hunk
@@ -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?