Re: [PATCH v3 2/7] invalidate_ref_cache(): take the submodule as parameter

On 10/17/2011 08:00 PM, Junio C Hamano wrote:

Michael Haggerty [off-list ref] writes:

quoted

On 10/12/2011 09:19 PM, Junio C Hamano wrote:

quoted

Michael Haggerty [off-list ref] writes:
 ...
Probably that is what all the existing callers want, but I would have
expected that an existing feature would be kept, perhaps like this
instead:

	if (!submodule) {
		struct ref_cache *c;
                for (c = ref_cache; c; c = c->next)
                	clear_ref_cache(c);
	} else {
		clear_ref_cache(get_ref_cache(submodule);
	}

...
Your specific suggestion would not work because currently
submodule==NULL signifies the main module.  However, it would be easy to
add the few-line function when/if it is needed.

I think "submodule==NULL" is probably a mistake; "" would make more sense
given that you are storing the string in name[FLEX_ARRAY] field.

Sorry I didn't respond to this earlier.

The public API convention (which predates my changes) is that "char
*submodule" arguments either point at the relative path to the submodule
or are NULL to denote the main module.  But since these are stored
internally in a name[FLEX_ARRAY] field, I have been using "" internally
to denote the main module.  I believe that everything is done correctly,
but I admit that the use of different conventions internally and
externally is a potential source of programming errors.

If this is viewed as something that needs changing, the easiest thing
would probably be to add a "const char *submodule" in the ref_cache data
structure that either contains NULL or points at the name field, and to
consistently use the convention that the main module must always be
denoted by NULL.  The space overhead would be negligible because the
number of ref_cache objects is limited to the number of submodules plus 1.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/