Re: [PATCH 3/6] refs.c: replace write_str_in_full by write_in_full
- From
Jeff King <peff@peff.net>
- Date
- Jan 21, 2015, 23:52 UTC
- Message-ID
- <20150121235213.GF11115@peff.net>
- In-Reply-To
- <CAGZ79kaTUraVYc1Th0-8ajw4kMuU-Sir2e9dW1uy90CuQRzgeg@mail.gmail.com>
On Wed, Jan 21, 2015 at 03:44:36PM -0800, Stefan Beller wrote:
Show 14 quoted lines
> 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...".
Show 8 quoted lines
> > 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