Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()
- From
Felipe Contreras <felipe.contreras@gmail.com>
- Date
- May 1, 2013, 19:23 UTC
- Message-ID
- <CAMP44s0sRqsmhbv-GA5x0FPc5msouSoGaU_Wg51E0F9bNsmkjg@mail.gmail.com>
- In-Reply-To
- <CALkWK0nTSMYvh8VMgQ6Q0EoPMmRa2vyodz+tDmpPp1d6KYmq8w@mail.gmail.com>
On Wed, May 1, 2013 at 1:36 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
Show 11 quoted lines
> Felipe Contreras wrote:
>> 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.I'd say you should start with a reorganization, and then a simplification.
Show 13 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.I see.
Show 6 quoted lines
>> 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.
Show 14 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.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.
Show 14 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.That's irrelevant, this 'else' case is for !reflog_len, there might or might not be @{u} in there.
Show 5 quoted lines
>> 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