Re: [PATCH] gc: remove unused launchctl_get_uid() call

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH] gc: remove unused launchctl_get_uid() call
Date: Thu, 9 Sep 2021 15:45:33 +0200 (CEST)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.2109091541370.59@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <87bl52dkv1.fsf@evledraar.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2185 bytes --]

Hi Ævar,

On Thu, 9 Sep 2021, Ævar Arnfjörð Bjarmason wrote:

>
> On Thu, Sep 09 2021, Johannes Schindelin wrote:
>
> > Hi Ævar,
> >
> > On Thu, 9 Sep 2021, Ævar Arnfjörð Bjarmason wrote:
> >
> >> When the launchctl_boot_plist() function was added in
> >> a16eb6b1ff3 (maintenance: skip bootout/bootstrap when plist is
> >> registered, 2021-08-24), an unused call to launchctl_get_uid() was
> >> added along with it. That call appears to have been copy/pasted from
> >> launchctl_boot_plist().
> >>
> >> Since we can remove that, we can also get rid of the "result"
> >> variable, whose only purpose was allow for the free() between its
> >> assignment and the return. That pattern also appears to have been
> >> copy/pasted from launchctl_boot_plist().
> >
> > I don't find the most crucial information in that commit message: what is
> > the fall-out of the removal of this call?
> >
> > Such an analysis (_with_ a summary of it in the commit message) is
> > definitely required. And it should not be left as an exercise for the
> > reader.
>
> Do you mean an assurance to the reader that the removed code doesn't
> have any side-effects? E.g. an addition of
>
>     As the patch shows the returned value wasn't used at all in this
>     function, the launchctl_get_uid() function itself just calls
>     xstrfmt() and getuid(), neither of which have any subtle global
>     side-effects, so this removal is safe.
>
> ?

Yes. You want to refrain from forcing every reader to have to go look at
the definition of that function at that revision. The accumulated time
spent tallies up rather in disfavor of doing the work diligently on the
contributor's side and save every reader some time. I mean, you forced me
to spend the time, and then to spend more time to point out the missing
analysis, and then you provided the paragraph as a question, forcing me to
spend even more time on answering. All this time could have been saved in
the first place. In this instance, it is too late to do anything about it.
But I'm sure you plan on contributing other patches. Hopefully it will be
more efficient next time.

Ciao,
Johannes