Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()
On Wed, May 1, 2013 at 5:26 PM, Ramkumar Ramachandra wrote:
> Felipe Contreras wrote:
>> On Wed, May 1, 2013 at 2:40 PM, Ramkumar Ramachandra
>> wrote:
>>> There's no need to associate one comment with one line of code.
>>> People can see clearly see the failure case following it.
>>
>> Is that the way you defend your comments? People can see that the
>> comment is wrong?
>
> In that case, all the comments are wrong. Even the ones about @{N}
> and @{-N}, because we never really check @{\d+} or @{-\d+}. Would you
> like to make the comments more painful to read and write?
If what I see in the code and what I read in the comments tell me
conflicting stories, I'd say the comments are not fulfilling their
purpose. Either add comments that explain what the code is _actually_
doing, or don't bother with them at all.
Cheers.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()
Felipe Contreras wrote:
> On Wed, May 1, 2013 at 2:40 PM, Ramkumar Ramachandra
> wrote:
>> 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.
>
> See, I tried to split your patch into logical changes, so I started with this:
>
> --- a/sha1_name.c
> +++ b/sha1_name.c
Thanks; I was finding this hard to do. I'll try to continue from here.
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -473,7 +473,7 @@ static int get_sha1_basic(const char *str, int
> len, unsigned char *sha1)
> return -1;
> }
> /* allow "@{...}" to mean the current branch reflog */
> - refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
> + refs_found = dwim_log("HEAD", 4, sha1, &real_ref);
Yeah, I noticed this just a few minutes ago. We really should have
tests testing @{N} against HEAD@{N}.
> Of course, this would be easy to see if you had bothered to split your
> patch into logical changes, but you didn't, so the change is lost in a
> mess. This is why it's not recommended to do that.
Right. I'll try to redo this as multiple parts.
>> There's no need to associate one comment with one line of code.
>> People can see clearly see the failure case following it.
>
> Is that the way you defend your comments? People can see that the
> comment is wrong?
In that case, all the comments are wrong. Even the ones about @{N}
and @{-N}, because we never really check @{\d+} or @{-\d+}. Would you
like to make the comments more painful to read and write?
> 2) @{-1}@{-1} now doesn't return an error
>
> 3) @{-1}{0} returns an invalid object
Thanks for the tests! I'll look into the problem.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()
On Wed, May 1, 2013 at 2:40 PM, Ramkumar Ramachandra wrote:
> Felipe Contreras wrote:
>> On Wed, May 1, 2013 at 1:36 PM, Ramkumar Ramachandra
>> 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.
There's always a way.
See, I tried to split your patch into logical changes, so I started with this:
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -460,23 +460,26 @@ static int get_sha1_basic(const char *str, int
len, unsigned char *sha1)
if (len && ambiguous_path(str, len))
return -1;
- if (!len && reflog_len) {
- struct strbuf buf = STRBUF_INIT;
- int ret;
- /* try the @{-N} syntax for n-th checkout */
- ret = interpret_branch_name(str+at, &buf);
- if (ret > 0) {
- /* substitute this branch name and restart */
- return get_sha1_1(buf.buf, buf.len, sha1, 0);
- } else if (ret == 0) {
- return -1;
+ if (reflog_len) {
+ if (!len) {
+ struct strbuf buf = STRBUF_INIT;
+ int ret;
+ /* try the @{-N} syntax for n-th checkout */
+ ret = interpret_branch_name(str+at, &buf);
+ if (ret > 0) {
+ /* substitute this branch name and restart */
+ return get_sha1_1(buf.buf, buf.len, sha1, 0);
+ } else if (ret == 0) {
+ return -1;
+ }
+ /* allow "@{...}" to mean the current branch reflog */
+ refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
+ } else {
+ refs_found = dwim_log(str, len, sha1, &real_ref);
}
- /* 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 {
refs_found = dwim_ref(str, len, sha1, &real_ref);
+ }
if (!refs_found)
return -1;
Truly no functional changes at this point, but then this:
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -473,7 +473,7 @@ static int get_sha1_basic(const char *str, int
len, unsigned char *sha1)
return -1;
}
/* allow "@{...}" to mean the current branch reflog */
- refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
+ refs_found = dwim_log("HEAD", 4, sha1, &real_ref);
} else {
refs_found = dwim_log(str, len, sha1, &real_ref);
}
Bam! Now we have a functional change. @{1} is different from HEAD@{1},
but this change makes them the same. Not only is this a functional
change, it's a behavioral change.
Of course, this would be easy to see if you had bothered to split your
patch into logical changes, but you didn't, so the change is lost in a
mess. This is why it's not recommended to do that.
>>> 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.
That is only proof that those tests pass.
> Scientific theories stand until you can find one observation that disproves
> it.
Yeah, I would like to see a scientist claiming "There are exactly
three multiverses. You don't think so? Prove me wrong. Na na na na!".
Not going to happen.
You are the one making a claim, you are the one that has the burden of
proof, and you haven't met it.
And even though I don't have to prove your claim is false, I already
did; @{1} is different now. If you want more, see below.
>> 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?
Exactly.
Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()
Ramkumar Ramachandra wrote:
> + if (!len)
> + /* In the @{N} case where there's nothing
> +* before the @.
> +*/
> + refs_found = dwim_log("HEAD", 4, sha1, &real_ref);
Minor mistake here: it should be dwim_ref, not dwim_log. Thanks to
Junio/ Jeff for poking.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()
Jonathan Nieder wrote: > Since this has been coming up from time to time: > [...] Thanks, I didn't know about 'git gui blame'. I think both comments and commit messages have their uses. One cannot do the job of the other. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()
Felipe Contreras wrote:
> On Wed, May 1, 2013 at 1:36 PM, Ramkumar Ramachandra
> 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.
>> 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.
> 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?
>>> 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.
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()
On Wed, May 1, 2013 at 1:36 PM, Ramkumar Ramachandra wrote:
> Felipe Contreras wrote:
>> On Wed, May 1, 2013 at 11:20 AM, Ramkumar Ramachandra
>> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()
Ramkumar Ramachandra wrote:
> [1]: https://www.ohloh.net/p/git/factoids#FactoidCommentsLow
Since this has been coming up from time to time:
I have nothing against including helpful comments where appropriate.
But one aspect which that factoid misses is that git has some very
detailed, very dense documentation available in its commit log. Tools
like "git gui blame" and "git log -S" can show detailed historical
information about the purpose of every line of code. A nice feature
of such documentation is that it is in a context where it cannot fall
out of date.
So for example I can do
$ git log -S'if (len && ambiguous_path(str, len))' -- sha1_name.c
commit 11cf8801
Author: Nicolas Pitre
Date: Thu Feb 1 17:29:33 2007 -0500
provide a nice @{...} syntax to always mean the current branch
reflog
This is shorter than HEAD@{...} and being nameless it has no
semantic
issues.
Signed-off-by: Nicolas Pitre
Signed-off-by: Junio C Hamano
and then "git show 11cf8801" will show me exactly what change prompted
that "len" test.
The same is true of the Linux kernel, too.
Hope that helps,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()
Felipe Contreras wrote:
> On Wed, May 1, 2013 at 11:20 AM, Ramkumar Ramachandra
> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()
On Wed, May 1, 2013 at 11:20 AM, Ramkumar Ramachandra
wrote:
> Presently, the logic for @-parsing is horribly convoluted. Prove that
> it is very straightforward by laying out the three cases (@{N},
> @{u[upstream], and @{-N}) and explaining how each case should be
> handled in comments. All tests pass, and no functional changes have
> been introduced.
> @@ -463,9 +463,26 @@ static int get_sha1_basic(const char *str, int len,
> unsigned char *sha1)
> */
> for (at = len - 4; at >= 0; at--) {
> if (str[at] == '@' && str[at+1] == '{') {
> + /* Set reflog_len only if we
> +* don't see a @{u[pstream]}. The
> +* @{N} and @{-N} forms have to do
> +* with reflog digging.
> +*/
> +
> + /* Setting len to at means that we are
> +* only going to process the part
> +* before the @ until we reach "if
> +* (reflog)" at the end of the
> +* function. That is only applicable
> +* in the @{N} case; in the @{-N} and
> +* @{u[pstream]} cases, we will run it
> +* through interpret_branch_name().
> +*/
> +
Overkill.
/* set reflog_len when using the form: @{N} */
> @@ -476,22 +493,34 @@ static int get_sha1_basic(const char *str, int len,
> unsigned char *sha1)
> if (len && ambiguous_path(str, len))
> return -1;
>
> - if (!len && reflog_len) {
> - struct strbuf buf = STRBUF_INIT;
> - int ret;
> - /* try the @{-N} syntax for n-th checkout */
> - ret = interpret_branch_name(str+at, &buf);
> - if (ret > 0) {
> - /* substitute this branch name and restart */
> - return get_sha1_1(buf.buf, buf.len, sha1, 0);
> - } else if (ret == 0) {
> - return -1;
> + if (reflog_len) {
> + if (!len)
> + /* In the @{N} case where there's nothing
> +* before the @.
> +*/
I think !len makes it clear.
> + refs_found = dwim_log("HEAD", 4, sha1, &real_ref);
> + else {
> + /* The @{N} case where there is something
> +* before the @ for dwim_log to figure out,
> +* and the @{-N} case.
> +*/
I think 'else' makes it clear.
> + 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.
> + /* 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}
Also, you removed a useful comment:
/* try the @{-N} syntax for n-th checkout */
> + 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".
> + }
> }
> - /* 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.
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.
With the principle of self-documenting code, if you need paragraphs to
explain what you are doing, you already lost.
Cheers.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
