Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: May 1, 2013, 18:36 UTC
Message-ID: <CALkWK0nTSMYvh8VMgQ6Q0EoPMmRa2vyodz+tDmpPp1d6KYmq8w@mail.gmail.com>
In-Reply-To: <CAMP44s1tHC+i+Wug_UuPnprZNvaPgLMNBX9MZi49SFv4iO62SQ@mail.gmail.com>

Felipe Contreras wrote:

Show 8 quoted lines

> On Wed, May 1, 2013 at 11:20 AM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:
>> +                       refs_found = dwim_log(str, len, sha1, &real_ref);
>> +
>> +                       if (!refs_found && str[2] == '-') {
>
> You are changing the behavior, there's no need for that in this
> reorganization patch.

This is not a reorganization patch.  I said "simplify": not refactor.

Show 7 quoted lines

>> +                               /* The @{-N} case that resolves to a
>> +                                * detached HEAD (ie. not a ref)
>> +                                */
>
> This is not true, it resolves to a ref.
>
> git rev-parse --symbolic-full-name @{-1}

> git co @~1; git co -; git rev-parse --symbolic-full-name @{-1}

If it did resolve to a ref, dwim_log() would have found it. The constraint guarding this `if (!refs_found && str[2] == '-')` wouldn't have been satisfied, and we wouldn't be here.

> Also, you removed a useful comment:
>
> /* try the @{-N} syntax for n-th checkout */

I've changed the entire logic and written expensive comments; and I'm not allowed to remove one comment which I didn't find helpful?

Show 10 quoted lines

>> +                               struct strbuf buf = STRBUF_INIT;
>> +                               if (interpret_branch_name(str, &buf) > 0) {
>> +                                       get_sha1_hex(buf.buf, sha1);
>> +                                       refs_found = 1;
>> +                                       reflog_len = 0;
>> +                               }
>> +                               strbuf_release(&buf);
>
> I'm pretty sure this is doing something totally different now. This is
> certainly not "no functional changes".

I'm claiming that there is no functional change at the program level, with the commenting*. If you want to disprove my claim, you have to write a test that breaks after this patch.

* And even if there is, it's probably an accidental corner case with
the wrong behavior.  The new logic is a straightforward implementation
of the rules.

Show 11 quoted lines

>> +                       }
>>                 }
>> -               /* allow "@{...}" to mean the current branch reflog */
>> -               refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
>> -       } else if (reflog_len)
>> -               refs_found = dwim_log(str, len, sha1, &real_ref);
>> -       else
>> +       } else
>> +               /* The @{u[pstream]} case */
>
> It's not true, there might not be any @{u} in there.

This entire structure is a success-filter. At the end of this, if !refs_found, then there has been a failure.

> Some of these changes might be good, but I would do them truly without
> introducing functional changes, without removing useful comments, and
> without adding paragraphs of explanation for what you are doing.

The functional changes part is for you to prove. And it's not even worth proving, because I'm claiming that the new logic is an obviously correct implementation of the rules.

> With the principle of self-documenting code, if you need paragraphs to
> explain what you are doing, you already lost.

The Git project is already suffering from a severe shortage of comments [1], and you're complaining about too many comments?

Have you tried to read and understand the old version? Some shining example of self-documenting code you've brought up.

[1]: https://www.ohloh.net/p/git/factoids#FactoidCommentsLow