On Sun, Dec 24, 2017 at 8:35 PM, Ævar Arnfjörð Bjarmason
<avarab@xxxxxxxxx> wrote:
>
> On Sun, Dec 24 2017, Kevin Daudt jotted:
>
>> On Sun, Dec 24, 2017 at 12:15:35PM +0400, Vadim Petrov wrote:
>>> Thank you for your replay.
>>>
>>> > I have to be honest: this commit message (including the subject) left me
>>> > quite puzzled as to the intent of this patch.
>>>
>>> I still only learn English and correctly express my thoughts while somewhat difficult.
>>>
>>> > If you also have a background story that motivated you to work on this
>>> > patch (for example, if you hit a huge performance bottleneck with some
>>> > tool that fed thousands of absolute paths to Git that needed to be turned
>>> > into paths relative to the worktree's top-level directory), I would
>>> > definitely put that into the commit message, too, if I were you.
>>>
>>> I have no such reason. I just saw it and wanted to change it.
>>
>> A commit message contains the reason why this is a good change to make.
>> It lets others know what problems it's trying to solve or what usecase
>> it tries to satisfy.
>>
>> The commit message basically needs to convince others why the change is
>> necessary / desired, now, and in the future.
>>
>> This will help others to follow your thought process and it gives you
>> the possiblity to communicate trade-offs you made, all which cannot
>> inferred from the patch.
>>
>> For simple changes, the motivation can be simple too.
>
> ...and a good example would be 6127ff63cf which introduced this logic
> Vadim is trying to change, i.e. does this still retain the fix for
> whatever issue that was trying to address?
>
> It's also good to CC the people who've directly worked on the code
> you're changing in the past, I've CC'd Martin.
>
>> To make it concrete: You are talking about a condition. What condition?
>> And you say that the previously obtained value will not be necessary.
>> What do you do with that value then? Why does this change improve the
>> situation?
>>
>> These are things you can state in your commit message.
>>
>> Hope this helps, Kevin
>>
>>> > Up until recently, we encouraged dropping the curly brackets from
>>> > single-line statements, but apparently that changed. It is now no longer
>>> > clear, and often left to the taste of the contributor. But not always.
>>> > Sometimes we start a beautiful thread discussion the pros and cons of
>>> > curly brackets in the middle of patch review, and drop altogether
>>> > reviewing the actual patch.
>>>
>>> I was guided by the rule from the Documentation/CodingGuidelines:
>>> When there are multiple arms to a conditional and some of them
>>> require braces, enclose even a single line block in braces for
>>> consistency.
>>> And other code from setup.c:
>>> from function get_common_dir:
>>> if (!has_common) {
>>> /* several commands */
>>> } else {
>>> free(candidate->work_tree);
>>> }
>>> from function get_common_dir_noenv:
>>> if (file_exists(path.buf)) {
>>> /* several commands */
>>> } else {
>>> strbuf_addstr(sb, gitdir);
>>> }
>>>
>>> > In short: I think your patch does the right thing, and I hope that you
>>> > find my suggestions to improve the patch useful.
>>>
>>> I fixed the patch according to your suggestions.
>>>
>>>
>>> Signed-off-by: Vadim Petrov <tridronet@xxxxxxxxx>
>>> ---
>>> setup.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/setup.c b/setup.c
>>> index 8cc34186c..1a414c256 100644
>>> --- a/setup.c
>>> +++ b/setup.c
>>> @@ -27,26 +27,26 @@ static int abspath_part_inside_repo(char *path)
>>> {
>>> size_t len;
>>> size_t wtlen;
>>> char *path0;
>>> int off;
>>> const char *work_tree = get_git_work_tree();
>>>
>>> if (!work_tree)
>>> return -1;
>>> wtlen = strlen(work_tree);
>>> len = strlen(path);
>>> - off = offset_1st_component(path);
>>>
>>> - /* check if work tree is already the prefix */
>>> - if (wtlen <= len && !strncmp(path, work_tree, wtlen)) {
>>> + if (wtlen > len || strncmp(path, work_tree, wtlen))
>>> + off = offset_1st_component(path);
>>> + else { /* check if work tree is already the prefix */
>>> if (path[wtlen] == '/') {
>>> memmove(path, path + wtlen + 1, len - wtlen);
>>> return 0;
>>> } else if (path[wtlen - 1] == '/' || path[wtlen] == '\0') {
>>> /* work tree is the root, or the whole path */
>>> memmove(path, path + wtlen, len - wtlen + 1);
>>> return 0;
>>> }
>>> /* work tree might match beginning of a symlink to work tree */
>>> off = wtlen;
>>> }
As far as I can tell this retains existing functionality.
Is this intended as just a style change or a speculative performance
change?
So the general concept is:
x = fa();
if (...) {
if (...) {
return 0;
}
x = fb();
}
being rewritten as
if (...) {
if (...) {
return 0;
}
x = fb();
} else {
x = fa();
}
or, in the last iteration
if (!...) {
x = fa();
} else {
if (...) {
return 0;
}
x = fb();
}
which (at least conceptually) avoids setting x unnecessarily when we do
the early return.
I think the last iteration might suffer a bit from the condition
inversion, since the comment feels a bit odd placed there at the
"} else {" line, and if it were to be placed at the top, it would have
to be negated "check if work tree is NOT already the prefix". Therefore
I think the original or the first iteration might be a tad better from a
readability perspective.
(Going down this path, We could potentially also remove the 'off'
variable completely, increment the 'path' pointer directly, and set
the 'path0' pointer before, not sure if that's a good idea though...)
--
Martin Erik Werner <martinerikwerner@xxxxxxxxx>
