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

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: May 1, 2013, 19:40 UTC
Message-ID: <CALkWK0=Z81f4c1X3uXO4O5q_Dj2hRJjSY-i-aDtZ0KqSyo5Wtg@mail.gmail.com>
In-Reply-To: <CAMP44s0sRqsmhbv-GA5x0FPc5msouSoGaU_Wg51E0F9bNsmkjg@mail.gmail.com>

Felipe Contreras wrote:

> On Wed, May 1, 2013 at 1:36 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
>> This is not a reorganization patch.  I said "simplify": not refactor.
>
> I'd say you should start with a reorganization, and then a simplification.

You don't think I already tried that? There is no way to sensibly reorganize the old logic sensibly, in a way that doesn't break anything.

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

Okay, we'll add it back if you feel strongly about it. I thought it would be a sore thumb when neither @{N} nor @{upstream} are explained.

Show 7 quoted lines

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

Oh, but I have.  All the tests (along with the new ones I added in [1/5]) pass.

Scientific theories stand until you can find one observation that disproves it.

Show 6 quoted lines

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

Your point being? That I shouldn't say "no functional changes" because the logic is changing non-trivially at this level?

Show 7 quoted lines

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

There's no need to associate one comment with one line of code. People can see clearly see the failure case following it.

Show 6 quoted lines

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

Is there anything _wrong_ with the comments, apart from the fact that they seem to be too big? I'm saying things that I cannot say in the commit message.