On Sat, Mar 11, 2017 at 01:41:34AM +0100, SZEDER Gábor wrote: > >> static struct ref *wanted_peer_refs(const struct ref *refs, > >> - struct refspec *refspec) > >> + struct refspec *refspec, unsigned int refspec_count) > > > > Most of the changes here and elsewhere are just about passing along > > multiple refspecs instead of a single, which makes sense. > > The new parameter should perhaps be renamed to 'refspec_nr', though, > as '_nr' suffix seems to be more common in the codebase than '_count'.
Yeah, agreed. > > Though if I'm bikeshedding, I'd probably have written the whole thing > > with an argv_array and avoided counting at all. > > Yeah, I did notice that you love argv_array :) I had to raise an > eyebrow recently while looking at send-pack and how it uses argv_array > to read refspecs from stdin into an array. I think argv_array feels a > bit out of place in both cases. Yes, it does exactly what's needed. > However, it's called *argv*_array, indicating that its contents is > destined to become the options of some command. But that's not the > case in these two cases, we are not dealing with arguments to a > command, these are just arrays of strings. In my mind, "argv" is synonymous with "NULL-terminated array of strings". If the name is the only thing keeping it from wider use, I'd much prefer us to give it a more generic name. All I really care about is simplifying memory management. :) > However, leveraging get_remote() makes it moot anyway. Even better. > > I do also notice that right _after_ this parsing, we use remote_get(), > > which is supposed to give us this config anyway. Which makes me wonder > > if we could just reorder this to put remote_get() first, and then read > > the resulting refspecs from remote->fetch. > > Though get_remote() does give us this config, at this point the > default fetch refspec has not been configured yet, therefore it's not > included in the resulting remote->fetch array. The default refspec is > set in write_refspec_config(), but that's called only about two > screenfulls later. So there is a bit of extra work to do in order to > leverage get_remote()'s parsing. > > I think the simplest way is to keep parsing the default fetch refspec > manually, and then append it to the remote->fetch array. Definitely > shorter and simpler than that parsing in the current patch. > Alternatively, we could set the default fetch refspec in the > configuration temporarily, only for the duration of the get_remote() > call, but it feels a bit too hackish... Yeah, I think manually combining the two here is fine. Though I won't complain if you want to look into setting the config earlier. If the refactoring isn't too bad, it would probably provide the nicest outcome. > However, the tests should also check that refs matching the default > fetch refspec are transferred, too, i.e. that the clone has something > under refs/remotes/origin/ as well. Case in point is using the result > of get_remote(): at first I naively set out to use remote->fetch > as-is, which didn't include the default fetch refspec, hence didn't > fetch refs/heads/master, but the test succeeded nonetheless. Good point. > > If we wanted to be thorough, we could also check that the feature works > > correctly with "--origin" (I think it does). > > I think it works, but I'm not quite sure what you mean with "works > correctly with --origin". > > So just to be clear: the behaviour depends on whether the remote name > given in '-c remote.<name>.fetch=<refspec>' matches the name given to > the '--origin=<name>'. If they match, then refs matching the > additional refspec are transferred, too. That's good. However, if > the two remote names don't match, then refs matching the additional > refspec are NOT transferred. I think this is good, too, because only > the origin remote should be cloned, whatever it is called, and in this > case that additional refspec belongs to a different remote. Yes, exactly. Mostly I was suggesting that if you do "--origin=foo", then we do not fetch items named "remote.origin.fetch" (i.e., that the code correctly uses the origin variable and not the hard-coded name "origin"). -Peff
