On Fri, Jul 16 2021, Johannes Schindelin wrote:
> Hi Ævar,
>
> On Tue, 13 Jul 2021, Ævar Arnfjörð Bjarmason wrote:
>
>>
>> On Tue, Jul 13 2021, Jeff Hostetler wrote:
>>
>> > On 7/1/21 7:02 PM, Ævar Arnfjörð Bjarmason wrote:
>> >> On Thu, Jul 01 2021, Jeff Hostetler via GitGitGadget wrote:
>> >>
>> >>> From: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx>
>> >>>
>> >>> Teach the win32 backend to register a watch on the working tree
>> >>> root directory (recursively). Also watch the <gitdir> if it is
>> >>> not inside the working tree. And to collect path change notifications
>> >>> into batches and publish.
>> >>>
>> >>> Signed-off-by: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx>
>> >>> ---
>> >>> compat/fsmonitor/fsmonitor-fs-listen-win32.c | 530 +++++++++++++++++++
>> >> <bikeshed mode> Spying on the early history of this (looking for the
>> >> Linux backend) I saw that at some point we had just
>> >> compat/fsmonitor/linux.c, and presumably some of
>> >> compat/fsmonitor/{windows,win32,macos,darwin}.c.
>> >> At some point those filenames became much much longer.
>> >>
>> >
>> > Once upon a time having "foo/bar/win32.c" and "abc/def/win32.c"
>> > would cause confusion in the debugger (I've long since forgotten
>> > which). Breaking at win32.c:30 was no longer unique.
>> >
>> > Also, if the Makefile sends all .o's to the root directory or a
>> > unified OBJS directory rather than to the subdir containing the .c,
>> > then we have another issue during linking...
>> >
>> > So, having been burned too many times, I prefer to make source
>> > filenames unique when possible.
>>
>> A much shorter name like compat/fsmonitor/fsmon-win32.c would achieve
>> that goal.
>>
>> >> I've noticed you tend to prefer really long file and function names,
>> >> e.g. your borrowed daemonize() became
>> >> spawn_background_fsmonitor_daemon(), I think aiming for shorter
>> >> filenames & function names helps, e.g. these long names widen diffstats,
>> >> and many people who hack on the code stick religiously to 80 character
>> >> width terminals.
>> >>
>> >
>> > I prefer self-documenting code.
>>
>> Sure, I'm not saying daemonize() is an ideal name, just suggesting that
>> you can both get uniqueness & self-documentation and not need to split
>> to multiple lines in some common cases to stay within the "We try to
>> keep to at most 80 characters per line" in CodingGuidelines in this
>> series.
>
> While you are entitled to have your taste, I have to point out that Jeff
> is just as entitled to their taste, and I don't think that you can claim
> that yours is better.
>
> So I wonder what the intended outcome of this review is? To make the patch
> better? Or to pit taste against taste?
Neither, to address a misunderstanding.
Sure, if a reviewer points out "maybe change X to Y" and the reply is "I
like X better than Y", fair enough.
My reading of Jeff H.'s upthread was that he'd misunderstood my
suggesting of that Y for a Z.
I.e. that shortening a name like fsmonitor-fs-listen-win32.c (X)
necessarily had to mean that we'd have a win32.c (Z), negatively
impacting some debugging workflows, as opposed to just a
shorter-but-unique name like fsmon-win32.c (Y).
Ditto for daemonize() (X/Z) and spawn_background_fsmonitor_daemon() (X).
I'm certain that with this reply we're thoroughly into the "respectfully
disagree" territory as opposed to having a misunderstanding.
I also take and agree your implied point that there's no point in having
a yes/no/yes/no/yes argument on-list, and I did not mean to engage in
such a thing, only to clear up the misunderstanding, if any.
I'll only say that I don't think that something like long variable/file
etc. names is *just* a matter of taste, seeing as we have a fairly
strict "keep to at most 80 characters per line" as the 2nd item in the C
coding style (after "use tabs, not spaces").
That matter of taste for one developer objectively makes it harder to
stay within the bounds of the coding style for furute maintenance.
We do have active contributors that I understand actually use terminals
of that size to work on this project (CC'd, but maybe I misrecall that
for one/both). I'm not one of those people, but I do find that
maintaining code with needlessly long identifiers in this codebase is
painful.
E.g. in a patch I just submitted I've been working on similarly long
identifiers in the refs code[1], and with say a long variable/type name
and using a long-named function you get to the point of needing to place
each individual argument of the function on its own line, or near enough
to that.
1. https://lore.kernel.org/git/patch-7.7-cb32b5c0526-20210716T142032Z-avarab@xxxxxxxxx/
