Daniel Barkalow <barkalow@xxxxxxxxxxxx> writes:
>> + /*
>> + * Do we want to validate LHS?
>> + ...
>> + * Hence we check non-empty LHS for fetch, and
>> + * colonless or glob LHS for push here.
>> + */
>
> Wouldn't this be clearer and not meaningfully harder in
> parse_fetch_refspec and parse_push_refspec?
Do you mean you want the callers of this internal implementation to also
loop over the input set of refs? I think that would be more complex code
but I do not see much gain by doing so.
>> + if (fetch ? (*rs[i].src) : (!rhs || is_glob)) {
>
> This is an odd combination of locals and struct members.
> : (!rs[i].dst || rs[i].pattern) {
Sorry, I do not understand what's wrong about it.
!!rhs === (did we see a colon) === !!rs[].dst
is_glob === (did they both end with "/*") === rs[].pattern
They are equivalent, and local variables are primarily what the logic
works on and bases its decisions to store what in rs[] structures.
Ahh... do you mean:
(*rs[i].src) === (is lhs non empty?) === !!llen
I guess using "llen" there is more consistent and is moderately cleaner.
Perhaps squash this as a clean-up?
diff --git a/remote.c b/remote.c
index 4117bfc..86113b7 100644
--- a/remote.c
+++ b/remote.c
@@ -453,7 +453,7 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp
* "empty" for removal) in LHS, and we cannot check
* for error until it actually gets used.
*/
- if (fetch ? (*rs[i].src) : (!rhs || is_glob)) {
+ if (fetch ? llen : (!rhs || is_glob)) {
st = check_ref_format(rs[i].src);
if (st && st != CHECK_REF_FORMAT_ONELEVEL)
goto invalid;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
