On Fri, 21 Mar 2008, Junio C Hamano wrote:
> 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.
I think it's more breadth but less depth. It would make the internal
implementation not depend on fetch, and put the checks that only apply to
fetch out of the push code path and vice versa.
Or just have a section
if (fetch) {
// checks for fetch LHS
// checks for fetch RHS
} else {
// checks for push LHS
// checks for push RHS
}
The body of the condition is only four lines, after all.
> >> + 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.
Considering that it's already stored values into the struct, it might as
well use those, and those are presumably more familiar to the average
reader, because all of the git code that uses refspecs other than this
function uses them.
> Ahh... do you mean:
>
> (*rs[i].src) === (is lhs non empty?) === !!llen
>
> I guess using "llen" there is more consistent and is moderately cleaner.
I'd go the other way, but having them all from the same set makes more
sense than one from one set and two from the other, regardless of which
way. If you go this way, you should probably also include the the rhs
checks, and the argument to check_ref_format().
-Daniel
*This .sig left intentionally blank*
--
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
