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