Re: [PATCH 04/35] refspec: introduce struct refspec

[Brandon Williams]

On 05/15, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > diff --git a/refspec.h b/refspec.h
> > index 173cea882..f6fb251f3 100644
> > --- a/refspec.h
> > +++ b/refspec.h
> > @@ -20,4 +20,29 @@ struct refspec_item *parse_push_refspec(int nr_refspec, 
> > const char **refspec);
> >  
> >  void free_refspec(int nr_refspec, struct refspec_item *refspec);
> >  
> > +#define REFSPEC_FETCH 1
> > +#define REFSPEC_PUSH 0
> 
> The reversed order of these two definitions looks somewhat unusual.
> I guess the reason why you made FETCH true and PUSH false is
> probably because quite a lot of places in the existing code we do

Yeah I really just made the choice based on what the existing logic did
(parse_refspec takes in a parameter 'fetch' which is 1 if we are parsing
the refspec for a fetch and 0 for push).  So it wouldn't be too
difficult to go and make this change here since all future callers use
the #defines themselves, because it is significantly easier to read
'REFSPEC_PUSH' than to read a 0 like you point out below :)

> 
>   if (fetch)
>   do the fetch thing;
>   else
>   do the push thing;
> 
> i.e. "fetch" variable is used as "is this a fetch: yes/no?"
> boolean, and a caller that mysteriously passes "1" (or "0")
> is harder to read than necessary.  Being able to pass REFSPEC_PUSH
> instead of "0" would certainly make the caller easier to read.  But
> as long as the variable is understood as "is_fetch? Yes/no", the
> caller can pass Yes or No and be still descriptive enough.
> 
> I think the way to make such a code more readable would not be to
> rewrite the above to
> 
>   if (fetch_or_push)
>   do the fetch thing;
>   else
>   do the push thing;
> 
> Rather it would be 
> 
>   if (fetch_or_push == REFSPEC_FETCH)
>   do the fetch thing;
>   else
>   do the push thing;
> 
> And once you have gone that far, the actual "enum" value assignment
> no longer makes difference.
> 
> > +#define REFSPEC_INIT_FETCH { .fetch = REFSPEC_FETCH }
> > +#define REFSPEC_INIT_PUSH { .fetch = REFSPEC_PUSH }
> > +
> > +struct refspec {
> > +   struct refspec_item *items;
> > +   int alloc;
> > +   int nr;
> > +
> > +   const char **raw;
> > +   int raw_alloc;
> > +   int raw_nr;
> > +
> > +   int fetch;
> > +};
> > +
> > +void refspec_item_init(struct refspec_item *item, const char *refspec, int 
> > fetch);
> > +void refspec_item_clear(struct refspec_item *item);
> > +void refspec_init(struct refspec *rs, int fetch);
> > +void refspec_append(struct refspec *rs, const char *refspec);
> > +void refspec_appendn(struct refspec *rs, const char **refspecs, int nr);
> > +void refspec_clear(struct refspec *rs);
> > +
> >  #endif /* REFSPEC_H */

-- 
Brandon Williams

Re: [PATCH 04/35] refspec: introduce struct refspec

[Junio C Hamano]

Brandon Williams  writes:

> diff --git a/refspec.h b/refspec.h
> index 173cea882..f6fb251f3 100644
> --- a/refspec.h
> +++ b/refspec.h
> @@ -20,4 +20,29 @@ struct refspec_item *parse_push_refspec(int nr_refspec, 
> const char **refspec);
>  
>  void free_refspec(int nr_refspec, struct refspec_item *refspec);
>  
> +#define REFSPEC_FETCH 1
> +#define REFSPEC_PUSH 0

The reversed order of these two definitions looks somewhat unusual.
I guess the reason why you made FETCH true and PUSH false is
probably because quite a lot of places in the existing code we do

if (fetch)
do the fetch thing;
else
do the push thing;

i.e. "fetch" variable is used as "is this a fetch: yes/no?"
boolean, and a caller that mysteriously passes "1" (or "0")
is harder to read than necessary.  Being able to pass REFSPEC_PUSH
instead of "0" would certainly make the caller easier to read.  But
as long as the variable is understood as "is_fetch? Yes/no", the
caller can pass Yes or No and be still descriptive enough.

I think the way to make such a code more readable would not be to
rewrite the above to

if (fetch_or_push)
do the fetch thing;
else
do the push thing;

Rather it would be 

if (fetch_or_push == REFSPEC_FETCH)
do the fetch thing;
else
do the push thing;

And once you have gone that far, the actual "enum" value assignment
no longer makes difference.

> +#define REFSPEC_INIT_FETCH { .fetch = REFSPEC_FETCH }
> +#define REFSPEC_INIT_PUSH { .fetch = REFSPEC_PUSH }
> +
> +struct refspec {
> + struct refspec_item *items;
> + int alloc;
> + int nr;
> +
> + const char **raw;
> + int raw_alloc;
> + int raw_nr;
> +
> + int fetch;
> +};
> +
> +void refspec_item_init(struct refspec_item *item, const char *refspec, int 
> fetch);
> +void refspec_item_clear(struct refspec_item *item);
> +void refspec_init(struct refspec *rs, int fetch);
> +void refspec_append(struct refspec *rs, const char *refspec);
> +void refspec_appendn(struct refspec *rs, const char **refspecs, int nr);
> +void refspec_clear(struct refspec *rs);
> +
>  #endif /* REFSPEC_H */

[PATCH 04/35] refspec: introduce struct refspec

[Brandon Williams]

Introduce 'struct refspec', an abstraction around a collection of
'struct refspec_item's much like how 'struct pathspec' holds a
collection of 'struct pathspec_item's.

A refspec struct also contains an array of the original refspec strings
which will be used to facilitate the migration to using this new
abstraction throughout the code base.

Signed-off-by: Brandon Williams 
---
 refspec.c | 64 +++
 refspec.h | 25 ++
 2 files changed, 89 insertions(+)

diff --git a/refspec.c b/refspec.c
index cef513ad8..2b898c922 100644
--- a/refspec.c
+++ b/refspec.c
@@ -178,3 +178,67 @@ void free_refspec(int nr_refspec, struct refspec_item 
*refspec)
}
free(refspec);
 }
+
+void refspec_item_init(struct refspec_item *item, const char *refspec, int 
fetch)
+{
+   memset(item, 0, sizeof(*item));
+
+   if (!parse_refspec(item, refspec, fetch))
+   die("Invalid refspec '%s'", refspec);
+}
+
+void refspec_item_clear(struct refspec_item *item)
+{
+   FREE_AND_NULL(item->src);
+   FREE_AND_NULL(item->dst);
+   item->force = 0;
+   item->pattern = 0;
+   item->matching = 0;
+   item->exact_sha1 = 0;
+}
+
+void refspec_init(struct refspec *rs, int fetch)
+{
+   memset(rs, 0, sizeof(*rs));
+   rs->fetch = fetch;
+}
+
+void refspec_append(struct refspec *rs, const char *refspec)
+{
+   struct refspec_item item;
+
+   refspec_item_init(, refspec, rs->fetch);
+
+   ALLOC_GROW(rs->items, rs->nr + 1, rs->alloc);
+   rs->items[rs->nr++] = item;
+
+   ALLOC_GROW(rs->raw, rs->raw_nr + 1, rs->raw_alloc);
+   rs->raw[rs->raw_nr++] = xstrdup(refspec);
+}
+
+void refspec_appendn(struct refspec *rs, const char **refspecs, int nr)
+{
+   int i;
+   for (i = 0; i < nr; i++)
+   refspec_append(rs, refspecs[i]);
+}
+
+void refspec_clear(struct refspec *rs)
+{
+   int i;
+
+   for (i = 0; i < rs->nr; i++)
+   refspec_item_clear(>items[i]);
+
+   FREE_AND_NULL(rs->items);
+   rs->alloc = 0;
+   rs->nr = 0;
+
+   for (i = 0; i < rs->raw_nr; i++)
+   free((char *)rs->raw[i]);
+   FREE_AND_NULL(rs->raw);
+   rs->raw_alloc = 0;
+   rs->raw_nr = 0;
+
+   rs->fetch = 0;
+}
diff --git a/refspec.h b/refspec.h
index 173cea882..f6fb251f3 100644
--- a/refspec.h
+++ b/refspec.h
@@ -20,4 +20,29 @@ struct refspec_item *parse_push_refspec(int nr_refspec, 
const char **refspec);
 
 void free_refspec(int nr_refspec, struct refspec_item *refspec);
 
+#define REFSPEC_FETCH 1
+#define REFSPEC_PUSH 0
+
+#define REFSPEC_INIT_FETCH { .fetch = REFSPEC_FETCH }
+#define REFSPEC_INIT_PUSH { .fetch = REFSPEC_PUSH }
+
+struct refspec {
+   struct refspec_item *items;
+   int alloc;
+   int nr;
+
+   const char **raw;
+   int raw_alloc;
+   int raw_nr;
+
+   int fetch;
+};
+
+void refspec_item_init(struct refspec_item *item, const char *refspec, int 
fetch);
+void refspec_item_clear(struct refspec_item *item);
+void refspec_init(struct refspec *rs, int fetch);
+void refspec_append(struct refspec *rs, const char *refspec);
+void refspec_appendn(struct refspec *rs, const char **refspecs, int nr);
+void refspec_clear(struct refspec *rs);
+
 #endif /* REFSPEC_H */
-- 
2.17.0.441.gb46fe60e1d-goog