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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 16, 2020, 18:31 UTC
Message-ID: <xmqqsgi8882t.fsf@gitster.c.googlers.com>
In-Reply-To: <CAHd-oW4OpoW0Qbp6_HfFiXRXasLXVnw3hDkzDG3S4WR3yH9Wuw@mail.gmail.com>

Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes:

Show 17 quoted lines

>> 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.
>
> Yes, I guess removing the prefix wouldn't make the names less
> descriptive. However, especially for "ss_excludes_file", I think using
> just "excludes_file" might induce the reader to think that the field
> refers to a "char *" holding a path. (We also have a "excludes_file"
> global variable in environment.c which is used like that). What if we
> renamed them to "oidst_info_exclude" and "oidst_excludes_file", would
> that be too verbose?

The potential for confusion with "path to these files" is real, I would think, so they may benefit from some prefix.

But instead of basing the prefix on their type, can we name it after what this struct holds about the excludes file, and what the data the struct holds is used for? Is "oidst" something that conveys it well to the readers of the code?

I guess the definition of "struct oid_stat" in dir.h should say what it is for and why it is called oid_stat, or even better yet, rename it to what is better than "this is a random bag to hold the file stat data and an object id for an unspecified purpose". IOW, it would be better for the name of a structure to say what's in it, but what the data it holds are for.

In a sense, this struct is a pared down version of cache_entry that keeps the filesystem stat data to allow us quickly find if the path was modified, and also lets us know if two contents are the same without comparing bytes. It is a mechanism for us to tell validity of our cached data. "struct path_validity" perhaps? I dunno.