Re: [PATCH 3/6] refs.c: replace write_str_in_full by write_in_full

From: Jeff King <peff@peff.net>
To: Stefan Beller <sbeller@google.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>,
	Michael Haggerty <mhagger@alum.mit.edu>,
	Loic Dachary <loic@dachary.org>
Subject: Re: [PATCH 3/6] refs.c: replace write_str_in_full by write_in_full
Date: Wed, 21 Jan 2015 18:52:13 -0500	[thread overview]
Message-ID: <20150121235213.GF11115@peff.net> (raw)
In-Reply-To: <CAGZ79kaTUraVYc1Th0-8ajw4kMuU-Sir2e9dW1uy90CuQRzgeg@mail.gmail.com>

On Wed, Jan 21, 2015 at 03:44:36PM -0800, Stefan Beller wrote:

> On Wed, Jan 21, 2015 at 3:38 PM, Jeff King <peff@peff.net> wrote:
> > On Wed, Jan 21, 2015 at 03:23:42PM -0800, Stefan Beller wrote:
> >
> >> There is another occurrence where we could have used write_str_in_full
> >> (line 3107: write_in_full(lock->lk->fd, &term, 1)), so the current state
> >> is inconsistent. This replaces the only occurrence of write_str_in_full
> >> by write_in_full, so we only need to wrap write_in_full in the next patch.
> >
> > I had to read the first sentence a few times to figure out what you
> > meant. But I am not sure it is even relevant. We do not care about the
> > inconsistency.
> 
> You're not the first who needs to reread my stuff :/
> I have the impression my English worsened since coming into the USA.

Actually, it was my fault in this case. I read it as "_this_ is another
occurrence", and then I scratched my head wondering what the first
occurrence was (was there a previous change that you should have been
referencing?). I finally got it on the third try. :)

> We do not care about the inconsistency, but we may care about the
> change itself: "write_str_in_full is way better than write_in_full, so
> why the step backwards?" And  I am trying to explain that this is not
> a huge step backwards but rather improves consistency.

But you could improve consistency by going the other way, too. :) I
think the point is that you should lead in with the _real_ reason for
the change, not justifications. You can put in the justifications, too,
for the people who say "wait, but couldn't you do this other thing...".

> > It is just "we are about to change how callers of
> > write_in_full in this file behave, the wrapper gets in the way, and it
> > does not add enough value by itself to merit making our future changes
> > in two places".
> 
> That's actually true. Though that sounds as if we'd be lazy ("we only
> want to make
> one change, so let's bend over here")

It's not laziness. It's avoiding duplicating logic, which would end up
costing more lines and providing worse maintainability than just
dropping the wrapper, which is after all only saving us a few characters
(and not anything conceptually hard).

> I'll rethink the commit message.

Everything I said above is rather subjective, of course. I do appreciate
you breaking your commits apart and explaining each one in the first
place. IOW, while I have thoughts on improving them (obviously), the
current iteration is not so bad that I would be upset to see it go into
git. Don't waste too much time on it.

-Peff