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