Re: [PATCH v2 1/3] dir: change the scope of function 'directory_exists_in_index()'

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 7, 2020, 18:05 UTC
Message-ID: <xmqq8sch3o8v.fsf@gitster.c.googlers.com>
In-Reply-To: <20201007074538.25891-2-shouryashukla.oo@gmail.com>

Shourya Shukla <shouryashukla.oo@gmail.com> writes:

Show 13 quoted lines

> diff --git a/dir.h b/dir.h
> index a3c40dec51..e46f240528 100644
> --- a/dir.h
> +++ b/dir.h
> @@ -370,6 +370,15 @@ int read_directory(struct dir_struct *, struct index_state *istate,
>  		   const char *path, int len,
>  		   const struct pathspec *pathspec);
>  
> +enum exist_status {
> +	index_nonexistent = 0,
> +	index_directory,
> +	index_gitdir
> +};

These were adequate as private names used within the wall of dir.c, but I doubt that they are named specific enough to stand out as public symbols.

Unlike say "index_state" (the name of a struct type), whose nature is quite global to any code that wants to access the in-core index, "index_directory" is *NOT* such a name. It is only of interest to those who want to see "I have a directory name---does it appear as a directory in the index?". It is not even interesting to those who want to ask similar and related questions like "I have this pathname---does it appear as anything in the index, and if so what type of entry is it?". A worse part of this is that even if such a helper function file_exists_in_index() were to be written, the "exist_status" enum won't be usable to return the answer that question, but yet the enum squats on a perfectly good name to express "status" for the whole class that it does not represent.

So, NAK. We need to come up with a better name for these symbols if we were to expose them to the outside world. The only good name this patch makes public is "directory_exists_in_index()", which is specific enough.

Show 6 quoted lines

> +enum exist_status directory_exists_in_index(struct index_state *istate,
> +					    const char *dirname, int len);
> +
>  enum pattern_match_result {
>  	UNDECIDED = -1,
>  	NOT_MATCHED = 0,