Re: [PATCH 03/35] refspec: rename struct refspec to struct refspec_item

Brandon Williams [off-list ref] writes:

In preperation for introducing an abstraction around a collection of
refspecs (much like how a 'struct pathspec' is a collection of 'struct
pathspec_item's) rename the existing 'struct refspec' to 'struct
refspec_item'.

Makes sense.

 /* See TAG_REFSPEC for the string version */
-const struct refspec *tag_refspec = &s_tag_refspec;
+const struct refspec_item *tag_refspec = &s_tag_refspec;
 
 /*
  * Parses 'refspec' and populates 'rs'.  returns 1 if successful and 0 if the
  * refspec is invalid.
  */
-static int parse_refspec(struct refspec *rs, const char *refspec, int fetch)
+static int parse_refspec(struct refspec_item *rs, const char *refspec, int fetch)

The new comment given to the function in the previous step talks
in terms of the variable name and not type, so technically it is
still valid after this change without updating.

I however wonder if it makes more sense to go one step further and
rename the "const char *" variable that is a string form of a single
refspec item to something other than "refspec".  Which would
invalidate the commit you gave to the function and make it necessary
to be updated in this step.

I wonder if the early part of the series becomes easier to read if
patches 2 and 3 are swapped.  That may result in less code/comment
churn.

On 05/15, Junio C Hamano wrote:

Brandon Williams [off-list ref] writes:

quoted

In preperation for introducing an abstraction around a collection of
refspecs (much like how a 'struct pathspec' is a collection of 'struct
pathspec_item's) rename the existing 'struct refspec' to 'struct
refspec_item'.

Makes sense.

quoted

 /* See TAG_REFSPEC for the string version */
-const struct refspec *tag_refspec = &s_tag_refspec;
+const struct refspec_item *tag_refspec = &s_tag_refspec;
 
 /*
  * Parses 'refspec' and populates 'rs'.  returns 1 if successful and 0 if the
  * refspec is invalid.
  */
-static int parse_refspec(struct refspec *rs, const char *refspec, int fetch)
+static int parse_refspec(struct refspec_item *rs, const char *refspec, int fetch)

The new comment given to the function in the previous step talks
in terms of the variable name and not type, so technically it is
still valid after this change without updating.

I however wonder if it makes more sense to go one step further and
rename the "const char *" variable that is a string form of a single
refspec item to something other than "refspec".  Which would
invalidate the commit you gave to the function and make it necessary
to be updated in this step.

I wonder if the early part of the series becomes easier to read if
patches 2 and 3 are swapped.  That may result in less code/comment
churn.

I'll go ahead and switch these two patches and fixup the comment and
function name.

-- 
Brandon Williams