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

From: Junio C Hamano <gitster@pobox.com>
Date: May 15, 2018, 09:37 UTC
Message-ID: <xmqqefid8djg.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180514215626.164960-5-bmwill@google.com>

Brandon Williams <bmwill@google.com> writes:

Show 10 quoted lines

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

Show 23 quoted lines

> +#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 */