Re: [PATCH 2/2] dir: improve naming of oid_stat fields in two structs

From: Patryk Obara <dreamer.tan@gmail.com>
To: Junio C Hamano <gitster@pobox.com>,
	Matheus Tavares <matheus.bernardino@usp.br>
Cc: git@vger.kernel.org, "René Scharfe" <l.s.r@web.de>,
	"Jeff King" <peff@peff.net>
Subject: Re: [PATCH 2/2] dir: improve naming of oid_stat fields in two structs
Date: Mon, 16 Mar 2020 13:31:20 +0100	[thread overview]
Message-ID: <938146aa-e095-66fb-da0b-10e8796eaa12@gmail.com> (raw)
In-Reply-To: <xmqq1rpsako1.fsf@gitster.c.googlers.com>

On 16/03/2020 07:17, Junio C Hamano wrote:
> Matheus Tavares <matheus.bernardino@usp.br> writes:
> 
>> Note: I choosed to use "st_*", as naming, for simplicity, and to keep
>> the code lines small. But should we use a more verbose "oidst_*" format,
>> instead, to avoid confusions with "struct stat"?
>> ...
>> @@ -334,8 +334,8 @@ struct dir_struct {
>>   
>>   	/* Enable untracked file cache if set */
>>   	struct untracked_cache *untracked;
>> -	struct oid_stat ss_info_exclude;
>> -	struct oid_stat ss_excludes_file;
>> +	struct oid_stat st_info_exclude;
>> +	struct oid_stat st_excludes_file;
>>   	unsigned unmanaged_exclude_files;
>>   };
> 
> I tend to agree with you that using prefix "st_" for anything other
> than "struct stat" proper would be confusing.  If "ss" used to stand
> for "sha1 stat", I would expect "oid stat" to abbreviate to "os", at
> least.

I think I stopped myself from more renames in that patch series, because
the number of touched lines across various functions started to grow too
fast and I was already stepping on brian's toes when touching oid
conversions.

> I also am wondering if we can do without any prefix, i.e. just call
> them "info_exclude" and "excludes_file", because the field names are
> private to each struct and there is no strong reason to have a
> common prefix among fields in a single struct.  Rather, it is more
> important for the fields of the same type in a single struct to have
> distinct names so that the reader can easily tell them apart and the
> reason for their use is straight-forward to understand, and the two
> names without any prefix convey their distinction pretty well, I
> would think.

The evolution of this name seems to be an artefact of refactorization
process, and not really a design decision:

info_exclude_sha1 (unsigned char[20]) changed to:
ss_info_exclude (struct sha1_stat) changed to:
ss_info_exclude (struct oid_stat)

There are some comments in dir.h still referring to info_exclude_sha1.

Therefore removing the prefix altogether (and fixing outdated comment!)
would be very welcome.

On the other hand, naming them sss_info_exclude or sos_info_exclude
would be quite funny, although I don't find hilarity to be a desirable
quality for a naming convention ;)

-- 
| ← Ceci n'est pas une pipe
Patryk Obara