Re: [PATCH 04/17] Name local variables more consistently
Jeff King writes: > Yeah, I agree that "refnames" would be better. I think something like > "spec" or "refspec" would indicate better that they are to be matched > against, but then you run afoul of confusing that with colon-delimited > refspecs (which I do not think fetch-pack understands at all). Correct. It only takes the LHS of the refspecs, following the "one tool does one specific thing" philosophy. The arrangement was that the calling script interpreted refspecs, split them into LHS and RHS, fed the LHS of the refspecs to fetch-pack, and then read the output to learn which local refs (i.e. RHS of the refspecs) to update with what value. -- 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 04/17] Name local variables more consistently
Michael Haggerty writes: > On 08/26/2012 07:39 PM, Junio C Hamano wrote: > ... >> After all, this codepath is not limited to branches these days as >> the word "head" implies. Rather, has what we >> asked for, and has what the other sides have, and we are >> trying to make sure we haven't asked what they do not have in this >> function. >> >> And the way we do so is to match the "thing"s with what are in >> "refs" to find unmatched ones. >> >> So between the two, I would have chosen "match" instead of "heads" >> to call the "thing". > > When I decided between "heads" and "match", my main consideration was > that "match" sounds like something that has already been matched, not > something that is being matched against. The word "match" also implies > to me that some nontrivial matching process is going on, like glob > expansion. > > But I agree with you that "heads" also has disadvantages. > > What about a third option: "refnames"? This name makes it clear that we > are talking about simple names as opposed to "struct ref" or some kind > of refname glob patterns and also makes it clear that they are not > necessarily all branches. It would also be distinct from the "refs" > linked list that is often used in the same functions. "refnames" has a similar issue as "match", as you have pointed out, and more. Was it the remote or us who populated the "refnames" (answer: these are "our" refnames, and the other one "refs" is what they have)? What do we have that "refnames" for (answer: these specify what we are going to pick among what they have)? Perhaps "asked"? At the beginning of the processing, we have all that were asked for, and at the end, we leave what were asked for but were not fulfilled. -- 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 04/17] Name local variables more consistently
On Mon, Aug 27, 2012 at 11:22:36AM +0200, Michael Haggerty wrote: > > Using one name is better, but I wonder "heads" is the better one > > between the two. > > > > After all, this codepath is not limited to branches these days as > > the word "head" implies. Rather, has what we > > asked for, and has what the other sides have, and we are > > trying to make sure we haven't asked what they do not have in this > > function. > > > > And the way we do so is to match the "thing"s with what are in > > "refs" to find unmatched ones. > > > > So between the two, I would have chosen "match" instead of "heads" > > to call the "thing". > > When I decided between "heads" and "match", my main consideration was > that "match" sounds like something that has already been matched, not > something that is being matched against. The word "match" also implies > to me that some nontrivial matching process is going on, like glob > expansion. > > But I agree with you that "heads" also has disadvantages. > > What about a third option: "refnames"? This name makes it clear that we > are talking about simple names as opposed to "struct ref" or some kind > of refname glob patterns and also makes it clear that they are not > necessarily all branches. It would also be distinct from the "refs" > linked list that is often used in the same functions. Yeah, I agree that "refnames" would be better. I think something like "spec" or "refspec" would indicate better that they are to be matched against, but then you run afoul of confusing that with colon-delimited refspecs (which I do not think fetch-pack understands at all). -Peff -- 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 04/17] Name local variables more consistently
On 08/26/2012 07:39 PM, Junio C Hamano wrote:
> Jeff King writes:
>
>> On Thu, Aug 23, 2012 at 10:10:29AM +0200, mhag...@alum.mit.edu wrote:
>>
>>> From: Michael Haggerty
>>>
>>> Use the names (nr_heads, heads) consistently across functions, instead
>>> of sometimes naming the same values (nr_match, match).
>>
>> I think this is fine, although:
>>
>>> --- a/builtin/fetch-pack.c
>>> +++ b/builtin/fetch-pack.c
>>> @@ -521,7 +521,7 @@ static void mark_recent_complete_commits(unsigned long
>>> cutoff)
>>> }
>>> }
>>>
>>> -static void filter_refs(struct ref **refs, int nr_match, char **match)
>>> +static void filter_refs(struct ref **refs, int nr_heads, char **heads)
>>> {
>>> struct ref **return_refs;
>>> struct ref *newlist = NULL;
>>> @@ -530,12 +530,12 @@ static void filter_refs(struct ref **refs, int
>>> nr_match, char **match)
>>> struct ref *fastarray[32];
>>> int match_pos;
>>
>> This match_pos is an index into the "match" array, which becomes "head".
>> Should it become head_pos?
>>
>> And then bits like this:
>>
>>> - while (match_pos < nr_match) {
>>> - cmp = strcmp(ref->name, match[match_pos]);
>>> + while (match_pos < nr_heads) {
>>> + cmp = strcmp(ref->name, heads[match_pos]);
>>
>> Would be:
>>
>> while (head_pos < nr_heads)
>>
>> which makes more sense to me.
>
> Using one name is better, but I wonder "heads" is the better one
> between the two.
>
> After all, this codepath is not limited to branches these days as
> the word "head" implies. Rather, has what we
> asked for, and has what the other sides have, and we are
> trying to make sure we haven't asked what they do not have in this
> function.
>
> And the way we do so is to match the "thing"s with what are in
> "refs" to find unmatched ones.
>
> So between the two, I would have chosen "match" instead of "heads"
> to call the "thing".
When I decided between "heads" and "match", my main consideration was
that "match" sounds like something that has already been matched, not
something that is being matched against. The word "match" also implies
to me that some nontrivial matching process is going on, like glob
expansion.
But I agree with you that "heads" also has disadvantages.
What about a third option: "refnames"? This name makes it clear that we
are talking about simple names as opposed to "struct ref" or some kind
of refname glob patterns and also makes it clear that they are not
necessarily all branches. It would also be distinct from the "refs"
linked list that is often used in the same functions.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
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 04/17] Name local variables more consistently
Jeff King writes:
> On Thu, Aug 23, 2012 at 10:10:29AM +0200, mhag...@alum.mit.edu wrote:
>
>> From: Michael Haggerty
>>
>> Use the names (nr_heads, heads) consistently across functions, instead
>> of sometimes naming the same values (nr_match, match).
>
> I think this is fine, although:
>
>> --- a/builtin/fetch-pack.c
>> +++ b/builtin/fetch-pack.c
>> @@ -521,7 +521,7 @@ static void mark_recent_complete_commits(unsigned long
>> cutoff)
>> }
>> }
>>
>> -static void filter_refs(struct ref **refs, int nr_match, char **match)
>> +static void filter_refs(struct ref **refs, int nr_heads, char **heads)
>> {
>> struct ref **return_refs;
>> struct ref *newlist = NULL;
>> @@ -530,12 +530,12 @@ static void filter_refs(struct ref **refs, int
>> nr_match, char **match)
>> struct ref *fastarray[32];
>> int match_pos;
>
> This match_pos is an index into the "match" array, which becomes "head".
> Should it become head_pos?
>
> And then bits like this:
>
>> -while (match_pos < nr_match) {
>> -cmp = strcmp(ref->name, match[match_pos]);
>> +while (match_pos < nr_heads) {
>> +cmp = strcmp(ref->name, heads[match_pos]);
>
> Would be:
>
> while (head_pos < nr_heads)
>
> which makes more sense to me.
Using one name is better, but I wonder "heads" is the better one
between the two.
After all, this codepath is not limited to branches these days as
the word "head" implies. Rather, has what we
asked for, and has what the other sides have, and we are
trying to make sure we haven't asked what they do not have in this
function.
And the way we do so is to match the "thing"s with what are in
"refs" to find unmatched ones.
So between the two, I would have chosen "match" instead of "heads"
to call the "thing".
--
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 04/17] Name local variables more consistently
On 08/23/2012 10:39 AM, Jeff King wrote:
> On Thu, Aug 23, 2012 at 10:10:29AM +0200, mhag...@alum.mit.edu wrote:
>
>> From: Michael Haggerty
>>
>> Use the names (nr_heads, heads) consistently across functions, instead
>> of sometimes naming the same values (nr_match, match).
>
> I think this is fine, although:
>
>> --- a/builtin/fetch-pack.c
>> +++ b/builtin/fetch-pack.c
>> @@ -521,7 +521,7 @@ static void mark_recent_complete_commits(unsigned long
>> cutoff)
>> }
>> }
>>
>> -static void filter_refs(struct ref **refs, int nr_match, char **match)
>> +static void filter_refs(struct ref **refs, int nr_heads, char **heads)
>> {
>> struct ref **return_refs;
>> struct ref *newlist = NULL;
>> @@ -530,12 +530,12 @@ static void filter_refs(struct ref **refs, int
>> nr_match, char **match)
>> struct ref *fastarray[32];
>> int match_pos;
>
> This match_pos is an index into the "match" array, which becomes "head".
> Should it become head_pos?
>
> And then bits like this:
>
>> -while (match_pos < nr_match) {
>> -cmp = strcmp(ref->name, match[match_pos]);
>> +while (match_pos < nr_heads) {
>> +cmp = strcmp(ref->name, heads[match_pos]);
>
> Would be:
>
> while (head_pos < nr_heads)
>
> which makes more sense to me.
I was up in the air about this, because match_pos *is* the position at
which a match is attempted. But since the name also strikes you as
wrong, I will change it in the next version.
Thanks for this and all of your other comments!
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
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 04/17] Name local variables more consistently
On Thu, Aug 23, 2012 at 10:10:29AM +0200, mhag...@alum.mit.edu wrote:
> From: Michael Haggerty
>
> Use the names (nr_heads, heads) consistently across functions, instead
> of sometimes naming the same values (nr_match, match).
I think this is fine, although:
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -521,7 +521,7 @@ static void mark_recent_complete_commits(unsigned long
> cutoff)
> }
> }
>
> -static void filter_refs(struct ref **refs, int nr_match, char **match)
> +static void filter_refs(struct ref **refs, int nr_heads, char **heads)
> {
> struct ref **return_refs;
> struct ref *newlist = NULL;
> @@ -530,12 +530,12 @@ static void filter_refs(struct ref **refs, int
> nr_match, char **match)
> struct ref *fastarray[32];
> int match_pos;
This match_pos is an index into the "match" array, which becomes "head".
Should it become head_pos?
And then bits like this:
> - while (match_pos < nr_match) {
> - cmp = strcmp(ref->name, match[match_pos]);
> + while (match_pos < nr_heads) {
> + cmp = strcmp(ref->name, heads[match_pos]);
Would be:
while (head_pos < nr_heads)
which makes more sense to me.
-Peff
--
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
