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

From: Brandon Williams <bmwill@google.com>
Date: May 15, 2018, 18:37 UTC
Message-ID: <20180515183702.GG72551@google.com>
In-Reply-To: <xmqqefid8djg.fsf@gitster-ct.c.googlers.com>

On 05/15, Junio C Hamano wrote:

Show 16 quoted lines

> Brandon Williams <bmwill@google.com> 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 :)

Show 54 quoted lines

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