From: Daniel Barkalow Date: Sat, 22 Mar 2008 20:45:20 GMT Subject: Re: [PATCH] remote.c: Fix overtight refspec validation Message-ID: In-Reply-To: <7viqzeilp0.fsf@gitster.siamese.dyndns.org> On Sat, 22 Mar 2008, Junio C Hamano wrote: > Daniel Barkalow writes: > > > On Fri, 21 Mar 2008, Junio C Hamano wrote: > > ... > >> 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. > > There are two commits on jc/refspec-fix branch merged to 'pu'. The > earlier one is my version, and the one on top is based on the above > suggestion. I do not know which one is clearer, more readable and > maintainable. I like the second one quite a bit. If nothing else, it's got, as the comments, the best documentation of refspecs I've seen, and then it's got code which clearly implements those rules. I think the explicitness is worth the extra lines. Thanks for taking care of this; as a result of starting a new job, I haven't had much in the way of coding attention for other stuff the past couple of weeks. -Daniel *This .sig left intentionally blank*