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

Brandon Williams [off-list ref] writes:

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

On 05/15, Junio C Hamano wrote:

Brandon Williams [off-list ref] writes:

quoted

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.

quoted

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