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

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

 



On Wed, May 1, 2013 at 1:36 PM, Ramkumar Ramachandra <artagnon@xxxxxxxxx> wrote:
> Felipe Contreras wrote:
>> On Wed, May 1, 2013 at 11:20 AM, Ramkumar Ramachandra
>> <artagnon@xxxxxxxxx> 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.

I'd say you should start with a reorganization, and then a simplification.

>>> +                               /* 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.

I see.

>> 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?

But it is helpful.

>>> +                               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.

The burden of proof resides in the one that makes the claim, I don't
need to prove that there are functional changes, merely that you
haven't met your burden of proof.

That being said, perhaps there are no _behavioral_ changes, because
you are relegating some of the current functionality to dwim_log, but
the code most certainly is doing something completely different.
Before, get_sha1_basic recursively called itself when @{-N} resolved
to a branch name, now, you rely on dwim_log to do this for you without
recursion, which is nice, but functionally different.

>>> +                       }
>>>                 }
>>> -               /* 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.

That's irrelevant, this 'else' case is for !reflog_len, there might or
might not be @{u} in there.

>> 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?

Irrelevant conclusion fallacy; let's suppose that the Git project is
indeed suffering from a shortage of comments, that doesn't imply that
*these* comments in their present form are any good.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[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]