Re: [PATCH 04/17] Name local variables more consistently

2012-08-27 Thread Michael Haggerty 
On 08/26/2012 07:39 PM, Junio C Hamano wrote:
 Jeff King p...@peff.net writes:
 
 On Thu, Aug 23, 2012 at 10:10:29AM +0200, mhag...@alum.mit.edu wrote:

 From: Michael Haggerty mhag...@alum.mit.edu

 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, nr_thing, thing has what we
 asked for, and refs 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 things 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

2012-08-27 Thread Jeff King 
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, nr_thing, thing has what we
  asked for, and refs 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 things 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

2012-08-27 Thread Junio C Hamano 
Michael Haggerty mhag...@alum.mit.edu 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, nr_thing, thing has what we
 asked for, and refs 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 things 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

2012-08-27 Thread Junio C Hamano 
Jeff King p...@peff.net 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

2012-08-26 Thread Junio C Hamano 
Jeff King p...@peff.net writes:

 On Thu, Aug 23, 2012 at 10:10:29AM +0200, mhag...@alum.mit.edu wrote:

 From: Michael Haggerty mhag...@alum.mit.edu
 
 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, nr_thing, thing has what we
asked for, and refs 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 things 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

[PATCH 04/17] Name local variables more consistently

2012-08-23 Thread mhagger 
From: Michael Haggerty mhag...@alum.mit.edu

Use the names (nr_heads, heads) consistently across functions, instead
of sometimes naming the same values (nr_match, match).

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 builtin/fetch-pack.c | 27 +--
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index cc21047..f11545e 100644
--- 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;
 
-   if (nr_match  !args.fetch_all) {
-   if (ARRAY_SIZE(fastarray)  nr_match)
-   return_refs = xcalloc(nr_match, sizeof(struct ref *));
+   if (nr_heads  !args.fetch_all) {
+   if (ARRAY_SIZE(fastarray)  nr_heads)
+   return_refs = xcalloc(nr_heads, sizeof(struct ref *));
else {
return_refs = fastarray;
-   memset(return_refs, 0, sizeof(struct ref *) * nr_match);
+   memset(return_refs, 0, sizeof(struct ref *) * nr_heads);
}
}
else
@@ -556,12 +556,12 @@ static void filter_refs(struct ref **refs, int nr_match, 
char **match)
}
else {
int cmp = -1;
-   while (match_pos  nr_match) {
-   cmp = strcmp(ref-name, match[match_pos]);
+   while (match_pos  nr_heads) {
+   cmp = strcmp(ref-name, heads[match_pos]);
if (cmp  0) /* definitely do not have it */
break;
else if (cmp == 0) { /* definitely have it */
-   match[match_pos][0] = '\0';
+   heads[match_pos][0] = '\0';
return_refs[match_pos] = ref;
break;
}
@@ -576,7 +576,7 @@ static void filter_refs(struct ref **refs, int nr_match, 
char **match)
 
if (!args.fetch_all) {
int i;
-   for (i = 0; i  nr_match; i++) {
+   for (i = 0; i  nr_heads; i++) {
ref = return_refs[i];
if (ref) {
*newtail = ref;
@@ -595,7 +595,7 @@ static void mark_alternate_complete(const struct ref *ref, 
void *unused)
mark_complete(NULL, ref-old_sha1, 0, NULL);
 }
 
-static int everything_local(struct ref **refs, int nr_match, char **match)
+static int everything_local(struct ref **refs, int nr_heads, char **heads)
 {
struct ref *ref;
int retval;
@@ -646,7 +646,7 @@ static int everything_local(struct ref **refs, int 
nr_match, char **match)
}
}
 
-   filter_refs(refs, nr_match, match);
+   filter_refs(refs, nr_heads, heads);
 
for (retval = 1, ref = *refs; ref ; ref = ref-next) {
const unsigned char *remote = ref-old_sha1;
@@ -777,8 +777,7 @@ static int get_pack(int xd[2], char **pack_lockfile)
 
 static struct ref *do_fetch_pack(int fd[2],
const struct ref *orig_ref,
-   int nr_match,
-   char **match,
+   int nr_heads, char **heads,
char **pack_lockfile)
 {
struct ref *ref = copy_ref_list(orig_ref);
@@ -819,7 +818,7 @@ static struct ref *do_fetch_pack(int fd[2],
fprintf(stderr, Server supports ofs-delta\n);
} else
prefer_ofs_delta = 0;
-   if (everything_local(ref, nr_match, match)) {
+   if (everything_local(ref, nr_heads, heads)) {
packet_flush(fd[1]);
goto all_done;
}
-- 
1.7.11.3

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

2012-08-23 Thread Jeff King 
On Thu, Aug 23, 2012 at 10:10:29AM +0200, mhag...@alum.mit.edu wrote:

 From: Michael Haggerty mhag...@alum.mit.edu
 
 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