From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 03 May 2012 19:48:05 GMT
Subject: Re: [PATCH v2 05/18] refs.c: extract function search_for_subdir()
Message-ID: <7v4nrxf3m2.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1335479227-7877-6-git-send-email-mhagger@alum.mit.edu>

mhagger@alum.mit.edu writes:

> From: Michael Haggerty <mhagger@alum.mit.edu>
>
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c |   34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 01fcdc7..5e51c10 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -277,6 +277,27 @@ static struct ref_entry *search_ref_dir(struct ref_dir *dir, const char *refname
>  }
>  
>  /*
> + * Search for a directory entry directly within dir (without
> + * recursing).  Sort dir if necessary.  subdirname must be a directory
> + * name (i.e., end in '/').  If mkdir is set, then create the
> + * directory if it is missing; otherwise, return NULL if the desired
> + * directory cannot be found.
> + */
> +static struct ref_entry *search_for_subdir(struct ref_dir *dir,
> +					   const char *subdirname, int mkdir)
> +{
> +	struct ref_entry *entry = search_ref_dir(dir, subdirname);
> +	if (!entry) {
> +		if (!mkdir)
> +			return NULL;
> +		entry = create_dir_entry(subdirname);
> +		add_entry_to_dir(dir, entry);
> +	}
> +	assert(entry->flag & REF_DIR);
> +	return entry;
> +}
> +
> +/*
>   * If refname is a reference name, find the ref_dir within the dir
>   * tree that should hold refname.  If refname is a directory name
>   * (i.e., ends in '/'), then return that ref_dir itself.  dir must
> @@ -294,17 +315,10 @@ static struct ref_dir *find_containing_dir(struct ref_dir *dir,
>  	for (slash = strchr(refname_copy, '/'); slash; slash = strchr(slash + 1, '/')) {
>  		char tmp = slash[1];
>  		slash[1] = '\0';
> -		entry = search_ref_dir(dir, refname_copy);
> -		if (!entry) {
> -			if (!mkdir) {
> -				dir = NULL;
> -				break;
> -			}
> -			entry = create_dir_entry(refname_copy);
> -			add_entry_to_dir(dir, entry);
> -		}
> +		entry = search_for_subdir(dir, refname_copy, mkdir);
>  		slash[1] = tmp;
> -		assert(entry->flag & REF_DIR);
> +		if (!entry)
> +			break;
>  		dir = &entry->u.subdir;
>  	}

Hrm.  The old code used to reset "dir" to NULL before breaking, so the
entire function used to return NULL.  Now, it calls search_for_subdir(),
which calls search_ref_dir() and gets NULL in entry, and returns NULL.

Wouldn't we end up returning the original parameter "dir" instead of NULL
in that case?  Would that make a difference?