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

From: Heiko Voigt <hvoigt@hvoigt.net>
Date: Jul 6, 2010, 14:40 UTC
Message-ID: <20100706144010.GA50570@book.hvoigt.net>
In-Reply-To: <7vvd9045q3.fsf@alter.siamese.dyndns.org>

Hi,

On Wed, Jun 30, 2010 at 01:37:24PM -0700, Junio C Hamano wrote:

Show 15 quoted lines

> Heiko Voigt <hvoigt@hvoigt.net> 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?

Well I needed to copy it to pathname anyway so I thought this would be the simplest check. But you are right about the unnecessary padding. What do you think about rephrasing this as

	if (buf.len >= PATH_MAX)
		return bad_path;

	memcpy(pathname, buf.buf, buf.len);

Show 14 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.

Well it was a quick and dirty way of getting the cache overwritten in case the pointer is different. See further below for the correction.

Show 23 quoted lines

> 
> > +		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?

Thanks for spotting this. I forgot that this part which was still pretty much in its quick and dirty state from the first implementation. I think currently it does not make much sense to apply the caching logic for submodule refs. In the merge case we iterate over them once so caching does not gain us anything here. I will drop it and write it like this:

Show 14 quoted lines

> static struct ref_list *get_loose_refs(const char *submodule)
> {
> 	if (submodule) {
> 		free_ref_list(submodule_refs.loose);
> 		submodule_refs.loose = get_ref_dir(submodule, "refs", NULL);
> 		return submodule_refs.loose;
> 	}
> 
> 	if (!cached_refs.did_loose) {
> 		cached_refs.loose = get_ref_dir(NULL, "refs", NULL);
> 		cached_refs.did_loose = 1;
> 	}
> 	return cached_refs.loose;
> }

I think the same applies to the caching of packed refs:

Show 25 quoted lines

> static struct ref_list *get_packed_refs(const char *submodule)
> {
> 	const char *packed_refs_file;
> 	struct cached_refs *refs;
> 
> 	if (submodule) {
> 		packed_refs_file = git_path_submodule(submodule, "packed-refs");
> 		refs = &submodule_refs;
> 		free_ref_list(refs->packed);
> 	} else {
> 		packed_refs_file = git_path("packed-refs");
> 		refs = &cached_refs;
> 	}
> 
> 	if (!refs->did_packed || submodule) {
> 		FILE *f = fopen(packed_refs_file, "r");
> 		refs->packed = NULL;
> 		if (f) {
> 			read_packed_refs(f, refs);
> 			fclose(f);
> 		}
> 		refs->did_packed = 1;
> 	}
> 	return refs->packed;
> }

I also realized that I forgot to use the --ancestry-path option for setup_revisions() in this series. It will be added in the next one.

Anything else? I will send another iteration with corrected patches shortly.

cheers Heiko