Re: [PATCH] setup.c: move statement under condition

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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>




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux