Re: [PATCH 00/25] worktree lock, move, remove and unlock

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 15, 2016, 01:21 UTC
Message-ID: <CAPc5daWHxyb5viwabR1-Tmx+7FNH4yxQJL7Oqd1FU9XXs4bNog@mail.gmail.com>
In-Reply-To: <CACsJy8DK863+rgseeYrQJ1db+xSeFfm8WsNvGBmJwD_pr1yMJQ@mail.gmail.com>

On Thu, Apr 14, 2016 at 5:40 PM, Duy Nguyen <pclouds@gmail.com> wrote:

Show 11 quoted lines

> On Thu, Apr 14, 2016 at 11:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>
>>> This is basically a resend from last time, which happened during rc
>>> time.
>>
>> It would have made them a much more pleasant read if you re-read
>> them during that time and added the missing "why" to many of the
>> commit log message.
>
> Hmm... I thought I didn't receive any comments last time.

I think you've been here long enough to know that absense of comments does not mean anything more than that: lack of interest at that moment in time.

Show 6 quoted lines

>> This looks parallel to die_errno(); isn't error_errno() a better name?
>
> To me, no. Duplicating the "err" looks weird. error_no() does not look
> good either. Though there's a couple of warning(..., strerror()),
> which could become warning_errno(). Then maybe error_errno() makes
> more sense because all three follow the same naming convention.

So in the end error_errno() would be a better name to you after all ;-) I agree the stuttering sound coming from repeating error twice feels somewhat odd, but warning_errno() would be so natural and obvious future direction, so...

Show 13 quoted lines

>>>   [03/25] copy.c: import copy_file() from busybox
>>>   [04/25] copy.c: delete unused code in copy_file()
>>>   [05/25] copy.c: convert bb_(p)error_msg to (sys_)error
>>>   [06/25] copy.c: style fix
>>>   [07/25] copy.c: convert copy_file() to copy_dir_recursively()
>>
>> Somewhere among these, there needs to be a single overview of why we
>> want "cp" implementation of busybox, e.g. what part of "cp" we want?
>> the whole thing?  or "because this is to be used from this and that
>> codepaths to make copy of these things, we only need these parts and
>> can remove other features like this and that?"
>
> We need directory move functionality.

Yeah, I know all that but you do not want to explain that to me only when I asked in a mailing list response. You want to get into the habit of having that in the log message to help reviewers, not just me, before they ask such a question.

Show 11 quoted lines

>> But such judgement is better done when we know what are the final
>> elements that are to be listed, i.e. closer to where new things are
>> introduced.  This is especially true, as the log messages of patches
>> leading to 21 are all sketchy and do not give the readers a good
>> birds-view picture.
>
> Well. I think all the commands are there now at the end of this
> series. So we have add, list, prune, move, remove, lock and unlock. I
> guess we can group list/add/move/remove together and the rest as
> support commands. I might add "git worktree migrate" for converting
> between worktree v0 and v1. But it's not clear yet.

Just so that there is no confusion, I am not opposed to reordering. I was just saying that the decision on what the right ordering is can be easier to make when the readers more or less know what the final set of things in the set are, and because that becomes only clear when they start reading 21, and because the log messages of patches leading to 21 do not show the direction clearly enough, it is hard to make that judgment at this point so early in the series.