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

From: Junio C Hamano <gitster@pobox.com>
To: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
Cc: git <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 11:31:54 -0700	[thread overview]
Message-ID: <xmqqsgi8882t.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <CAHd-oW4OpoW0Qbp6_HfFiXRXasLXVnw3hDkzDG3S4WR3yH9Wuw@mail.gmail.com> (Matheus Tavares Bernardino's message of "Mon, 16 Mar 2020 14:22:53 -0300")

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

>> 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.