Re: git-clone --config order & fetching extra refs during initial clone
Jeff King writes: > Actually, it's not too bad, because we can pick up things like > option_origin from the globals. Here it is for reference. The nice thing > about it (IMHO) is that it makes the lifetimes of the helper variables > much more shorter and more clear. > > But I'm OK with any of Gábor's original, my earlier squash, or this one. > > (Also, as a side note, the free(refspec) in the context at the bottom > seems like it probably ought to be free_refspec() in the original and > free_refspecs() after this change, but I didn't investigate). I'll tentatively queue this on top of the original and wait for Gábor to say something, then ;-) as I do agree that this one looks reasonable. Thanks.
Re: git-clone --config order & fetching extra refs during initial clone
On Mon, May 08, 2017 at 10:10:28PM -0400, Jeff King wrote: > On Tue, May 09, 2017 at 10:33:39AM +0900, Junio C Hamano wrote: > > > >> My patch deals with 'remote..refspec', i.e. 'remote->fetch'. > > >> Apparently some extra care is necessary for 'remote..tagOpt' and > > >> 'remote->fetch_tags', too. Perhaps there are more, I haven't checked > > >> again, and maybe we'll add similar config variables in the future. So > > >> I don't think that dealing with such config variables one by one in > > >> 'git clone', too, is the right long-term solution... but perhaps it's > > >> sufficient for the time being? > > > > > > I think your patch is a strict improvement and we don't need to hold up > > > waiting for a perfect fix (and because of the --single-branch thing you > > > mentioned, this may be the best we can do anyway). > > > > OK, so where does this patch stand now? It already is too late for > > the upcoming release, but should we merge it to 'next' once the > > release is made, cook it in 'next' and shoot for the next release > > as-is, or do we want to allow minor tweaks before it hits 'next'? > > I'd be OK with it as-is, but here are my nitpicks as a diff (keep the > assignment of refspec_count in one place, and free fetch_patterns as > soon as it is no longer used). > > I actually think it might be nice to pull the whole thing out into its > own function, but the parameters it takes would be oddly specific. Actually, it's not too bad, because we can pick up things like option_origin from the globals. Here it is for reference. The nice thing about it (IMHO) is that it makes the lifetimes of the helper variables much more shorter and more clear. But I'm OK with any of Gábor's original, my earlier squash, or this one. (Also, as a side note, the free(refspec) in the context at the bottom seems like it probably ought to be free_refspec() in the original and free_refspecs() after this change, but I didn't investigate). diff --git a/builtin/clone.c b/builtin/clone.c index 0630202c8..645cfa4fd 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -841,6 +841,37 @@ static void dissociate_from_references(void) free(alternates); } +static struct refspec *get_clone_refspecs(const char *base_refspec, + unsigned int *count) +{ + char *key; + const struct string_list *config_fetch_patterns; + struct refspec *ret; + + key = xstrfmt("remote.%s.fetch", option_origin); + config_fetch_patterns = git_config_get_value_multi(key); + + if (!config_fetch_patterns) { + *count = 1; + ret = parse_fetch_refspec(1, &base_refspec); + } else { + const char **fetch_patterns; + struct string_list_item *fp; + unsigned int i = 1; + + *count = 1 + config_fetch_patterns->nr; + ALLOC_ARRAY(fetch_patterns, *count); + fetch_patterns[0] = base_refspec; + for_each_string_list_item(fp, config_fetch_patterns) + fetch_patterns[i++] = fp->string; + ret = parse_fetch_refspec(*count, fetch_patterns); + free(fetch_patterns); + } + + free(key); + return ret; +} + int cmd_clone(int argc, const char **argv, const char *prefix) { int is_bundle = 0, is_local; @@ -861,9 +892,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) int err = 0, complete_refs_before_fetch = 1; struct refspec *refspec; - unsigned int refspec_count = 1; - const char **fetch_patterns; - const struct string_list *config_fetch_patterns; + unsigned int refspec_count; packet_trace_identity("clone"); argc = parse_options(argc, argv, prefix, builtin_clone_options, @@ -982,7 +1011,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix) strbuf_addf(&branch_top, "refs/remotes/%s/", option_origin); } - strbuf_addf(&value, "+%s*:%s*", src_ref_prefix, branch_top.buf); strbuf_addf(&key, "remote.%s.url", option_origin); git_config_set(key.buf, repo); strbuf_reset(&key); @@ -990,21 +1018,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix) if (option_reference.nr) setup_reference(); - strbuf_addf(&key, "remote.%s.fetch", option_origin); - config_fetch_patterns = git_config_get_value_multi(key.buf); - if (config_fetch_patterns) - refspec_count = 1 + config_fetch_patterns->nr; - fetch_patterns = xcalloc(refspec_count, sizeof(*fetch_patterns)); - fetch_patterns[0] = value.buf; - if (config_fetch_patterns) { - struct string_list_item *fp; - unsigned int i = 1; - for_each_string_list_item(fp, config_fetch_patterns) - fetch_patterns[i++] = fp->string; - } - refspec = parse_fetch_refspec(refspe
Re: git-clone --config order & fetching extra refs during initial clone
On Tue, May 09, 2017 at 10:33:39AM +0900, Junio C Hamano wrote: > >> My patch deals with 'remote..refspec', i.e. 'remote->fetch'. > >> Apparently some extra care is necessary for 'remote..tagOpt' and > >> 'remote->fetch_tags', too. Perhaps there are more, I haven't checked > >> again, and maybe we'll add similar config variables in the future. So > >> I don't think that dealing with such config variables one by one in > >> 'git clone', too, is the right long-term solution... but perhaps it's > >> sufficient for the time being? > > > > I think your patch is a strict improvement and we don't need to hold up > > waiting for a perfect fix (and because of the --single-branch thing you > > mentioned, this may be the best we can do anyway). > > OK, so where does this patch stand now? It already is too late for > the upcoming release, but should we merge it to 'next' once the > release is made, cook it in 'next' and shoot for the next release > as-is, or do we want to allow minor tweaks before it hits 'next'? I'd be OK with it as-is, but here are my nitpicks as a diff (keep the assignment of refspec_count in one place, and free fetch_patterns as soon as it is no longer used). I actually think it might be nice to pull the whole thing out into its own function, but the parameters it takes would be oddly specific. diff --git a/builtin/clone.c b/builtin/clone.c index 0630202c8..577529a11 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -861,7 +861,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) int err = 0, complete_refs_before_fetch = 1; struct refspec *refspec; - unsigned int refspec_count = 1; + unsigned int refspec_count; const char **fetch_patterns; const struct string_list *config_fetch_patterns; @@ -994,6 +994,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix) config_fetch_patterns = git_config_get_value_multi(key.buf); if (config_fetch_patterns) refspec_count = 1 + config_fetch_patterns->nr; + else + refspec_count = 1; fetch_patterns = xcalloc(refspec_count, sizeof(*fetch_patterns)); fetch_patterns[0] = value.buf; if (config_fetch_patterns) { @@ -1003,6 +1005,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) fetch_patterns[i++] = fp->string; } refspec = parse_fetch_refspec(refspec_count, fetch_patterns); + free(fetch_patterns); strbuf_reset(&key); strbuf_reset(&value); @@ -1129,7 +1132,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix) strbuf_release(&value); junk_mode = JUNK_LEAVE_ALL; - free(fetch_patterns); free(refspec); return err; } -Peff
Re: git-clone --config order & fetching extra refs during initial clone
Jeff King writes: > Good point. We can't really consider clone to be a blind "init + config > + fetch + checkout" because those middle two steps sometimes overlap > each other. It really does need to be its own beast. > ... > The right solution there is probably pushing that logic down into the > transport layer. Or at the very least abstracting it into a function so > that both clone and fetch can call it without replicating the logic. > >> My patch deals with 'remote..refspec', i.e. 'remote->fetch'. >> Apparently some extra care is necessary for 'remote..tagOpt' and >> 'remote->fetch_tags', too. Perhaps there are more, I haven't checked >> again, and maybe we'll add similar config variables in the future. So >> I don't think that dealing with such config variables one by one in >> 'git clone', too, is the right long-term solution... but perhaps it's >> sufficient for the time being? > > I think your patch is a strict improvement and we don't need to hold up > waiting for a perfect fix (and because of the --single-branch thing you > mentioned, this may be the best we can do anyway). OK, so where does this patch stand now? It already is too late for the upcoming release, but should we merge it to 'next' once the release is made, cook it in 'next' and shoot for the next release as-is, or do we want to allow minor tweaks before it hits 'next'? Thanks.
Re: git-clone --config order & fetching extra refs during initial clone
On Wed, May 3, 2017 at 4:42 PM, SZEDER Gábor wrote: > Cc'ing Ævar because of his work on 'clone --no-tags'. > > On Wed, Mar 15, 2017 at 6:08 PM, Jeff King wrote: >> On Sat, Mar 11, 2017 at 01:41:34AM +0100, SZEDER Gábor wrote: >>> > 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. :) > > Whether its name is the _only_ thing keeping it from wider use, I > don't know :) > > All I can say is that I was aware of argv_array, but because of > its name it didn't even occur to me. And even if I had considered it, > I still wouldn't have used it here. Had it been called string_array, > I think I would have used it. > > On a related note, we have string_list, which is not a list but an > ALLOC_GROW()-able array, and not that of strings (i.e. plan char*), > but of structs with a string and an additional data field. > Oh, well :) > > >>> > 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. > > I did actually look into that, but don't think it's a good idea. > > write_refspec_config() nicely encapsulates writing the proper fetch > refspec configuration according to the given command line options. Of > course these options are already known right at the start, so solely > in this regard we could call this function earlier. However, in some > cases, e.g. '--single-branch', the refspec to be written to the config > depends not only on the given options but on the refs in the remote > repository, too, so it can only be written after we got the remote's > refs. > > > Unfortunately, there is more to this issue. Earlier I though that the > fetch refspec is the only config that is ignored during a clone. > However, Ævar's 'clone --no-tags' patches[1] drew my attention to the > 'remote..tagOpt' config variable, that I overlooked earlier... > and apparently 'git clone' overlooks it as well, grabbing all tags > even when it's set to '--no-tags'. > > The root issue is that 'git clone' calls directly into the fetch > machinery instead of running 'git fetch' (either via run_command() or > cmd_fetch()), and some of these "higher-level" config variables are > only handled in 'builtin/fetch.c' but not in 'git clone'. By > "handle" I mean "parse and act accordingly": as it happens, these > config values are parsed alright when 'git clone' calls remote_get(), > but it never looks at the relevant fields in the resulting 'struct > remote'. > > Luckily, many "lower-level" config variables are working properly even > during 'git clone', because they are handled in the transport layer, > e.g. 'git clone -c url.https://github.com/.insteadof=gh: gh:git/git' > does the right thing. > > > I'm not sure what the right way forward would be. > > My patch deals with 'remote..refspec', i.e. 'remote->fetch'. > Apparently some extra care is necessary for 'remote..tagOpt' and > 'remote->fetch_tags', too. Perhaps there are more, I haven't checked > again, and
Re: git-clone --config order & fetching extra refs during initial clone
On 5/3/2017 22:22, Jeff King wrote: >> My patch deals with 'remote..refspec', i.e. 'remote->fetch'. >> Apparently some extra care is necessary for 'remote..tagOpt' and >> 'remote->fetch_tags', too. Perhaps there are more, I haven't checked >> again, and maybe we'll add similar config variables in the future. So >> I don't think that dealing with such config variables one by one in >> 'git clone', too, is the right long-term solution... but perhaps it's >> sufficient for the time being? > > I think your patch is a strict improvement and we don't need to hold up > waiting for a perfect fix (and because of the --single-branch thing you > mentioned, this may be the best we can do anyway). I agree. Let's fix one thing at a time, and not make this a overarching "account for all previously ignored config variables during clone" fix. Esp. as specifying the refspec is explicitly documented to work durig clone [1] I think we should get this in ASAP. Thanks for your work so far! [1] https://git-scm.com/docs/git-clone#git-clone---configltkeygtltvaluegt Regards, Sebastian
Re: git-clone --config order & fetching extra refs during initial clone
On Wed, May 03, 2017 at 04:42:58PM +0200, SZEDER Gábor wrote: > write_refspec_config() nicely encapsulates writing the proper fetch > refspec configuration according to the given command line options. Of > course these options are already known right at the start, so solely > in this regard we could call this function earlier. However, in some > cases, e.g. '--single-branch', the refspec to be written to the config > depends not only on the given options but on the refs in the remote > repository, too, so it can only be written after we got the remote's > refs. Good point. We can't really consider clone to be a blind "init + config + fetch + checkout" because those middle two steps sometimes overlap each other. It really does need to be its own beast. > The root issue is that 'git clone' calls directly into the fetch > machinery instead of running 'git fetch' (either via run_command() or > cmd_fetch()), and some of these "higher-level" config variables are > only handled in 'builtin/fetch.c' but not in 'git clone'. By > "handle" I mean "parse and act accordingly": as it happens, these > config values are parsed alright when 'git clone' calls remote_get(), > but it never looks at the relevant fields in the resulting 'struct > remote'. > > Luckily, many "lower-level" config variables are working properly even > during 'git clone', because they are handled in the transport layer, > e.g. 'git clone -c url.https://github.com/.insteadof=gh: gh:git/git' > does the right thing. Yeah, and I think that insteadOf is working as intended there (clone sets it early enough that all of the rest of the code sees it). And the bug is just that there's special handling in builtin/fetch.c that clone needs to replicate. The right solution there is probably pushing that logic down into the transport layer. Or at the very least abstracting it into a function so that both clone and fetch can call it without replicating the logic. > My patch deals with 'remote..refspec', i.e. 'remote->fetch'. > Apparently some extra care is necessary for 'remote..tagOpt' and > 'remote->fetch_tags', too. Perhaps there are more, I haven't checked > again, and maybe we'll add similar config variables in the future. So > I don't think that dealing with such config variables one by one in > 'git clone', too, is the right long-term solution... but perhaps it's > sufficient for the time being? I think your patch is a strict improvement and we don't need to hold up waiting for a perfect fix (and because of the --single-branch thing you mentioned, this may be the best we can do anyway). > Running a fully-fledged 'git fetch' seems to be simpler and safer > conceptually, as it would naturally handle all fetch-related config > variables, present and future. However, it's not without drawbacks: > 'git clone' must set the proper config before running 'git fetch' (or > at least set equivalent cmdline options), which in some cases requires > the refs in the remote repository, making an additional "list remote > refs" step necessary (i.e. both 'clone' and 'fetch' would have to > retrieve the refs from the remote, resulting in more network I/O). I don't think we ever want to request two ref advertisements; they're too expensive. If clone needs to do work between the advertisement and the actual fetch (and it sounds like it does), then it should be using the transport layer directly. Which is what it's already doing. -Peff
Re: git-clone --config order & fetching extra refs during initial clone
Cc'ing Ævar because of his work on 'clone --no-tags'. On Wed, Mar 15, 2017 at 6:08 PM, Jeff King wrote: > On Sat, Mar 11, 2017 at 01:41:34AM +0100, SZEDER Gábor wrote: >> > 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. :) Whether its name is the _only_ thing keeping it from wider use, I don't know :) All I can say is that I was aware of argv_array, but because of its name it didn't even occur to me. And even if I had considered it, I still wouldn't have used it here. Had it been called string_array, I think I would have used it. On a related note, we have string_list, which is not a list but an ALLOC_GROW()-able array, and not that of strings (i.e. plan char*), but of structs with a string and an additional data field. Oh, well :) >> > 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. I did actually look into that, but don't think it's a good idea. write_refspec_config() nicely encapsulates writing the proper fetch refspec configuration according to the given command line options. Of course these options are already known right at the start, so solely in this regard we could call this function earlier. However, in some cases, e.g. '--single-branch', the refspec to be written to the config depends not only on the given options but on the refs in the remote repository, too, so it can only be written after we got the remote's refs. Unfortunately, there is more to this issue. Earlier I though that the fetch refspec is the only config that is ignored during a clone. However, Ævar's 'clone --no-tags' patches[1] drew my attention to the 'remote..tagOpt' config variable, that I overlooked earlier... and apparently 'git clone' overlooks it as well, grabbing all tags even when it's set to '--no-tags'. The root issue is that 'git clone' calls directly into the fetch machinery instead of running 'git fetch' (either via run_command() or cmd_fetch()), and some of these "higher-level" config variables are only handled in 'builtin/fetch.c' but not in 'git clone'. By "handle" I mean "parse and act accordingly": as it happens, these config values are parsed alright when 'git clone' calls remote_get(), but it never looks at the relevant fields in the resulting 'struct remote'. Luckily, many "lower-level" config variables are working properly even during 'git clone', because they are handled in the transport layer, e.g. 'git clone -c url.https://github.com/.insteadof=gh: gh:git/git' does the right thing. I'm not sure what the right way forward would be. My patch deals with 'remote..refspec', i.e. 'remote->fetch'. Apparently some extra care is necessary for 'remote..tagOpt' and 'remote->fetch_tags', too. Perhaps there are more, I haven't checked again, and maybe we'll add similar config variables in the future. So I don't think that dealing with such config variables one by one in 'git clone', too, is the right long-term solution... but perhaps
Re: git-clone --config order & fetching extra refs during initial clone
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..fetch=' matches the name given to > the '--origin='. 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
Re: git-clone --config order & fetching extra refs during initial clone
On Mon, Feb 27, 2017 at 10:12 PM, Jeff King wrote: > I didn't actually review it very carefully before, but I'll do so now > (spoiler: a few nits, but it looks fine). > >> 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'. >> @@ -856,7 +861,9 @@ int cmd_clone(int argc, const char **argv, const char >> *prefix) >> int submodule_progress; >> >> struct refspec *refspec; >> - const char *fetch_pattern; >> + unsigned int refspec_count = 1; >> + const char **fetch_patterns; >> + const struct string_list *config_fetch_patterns; > > This "1" seems funny to up here by itself, as it must be kept in sync > with the later logic that feeds exactly one non-configured refspec into > our list. The current code isn't wrong, but it would be nice to have it > all together. I.e., replacing: > >> + if (config_fetch_patterns) >> + refspec_count = 1 + config_fetch_patterns->nr; >> + fetch_patterns = xcalloc(refspec_count, sizeof(*fetch_patterns)); >> + fetch_patterns[0] = value.buf; > > with: > > refspec_count = 1; > if (config_fetch_patterns) > refspec_count += config_fetch_patterns->nr; > ... 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. However, leveraging get_remote() makes it moot anyway. >> + refspec = parse_fetch_refspec(refspec_count, fetch_patterns); >> >> + strbuf_reset(&key); >> strbuf_reset(&value); >> >> remote = remote_get(option_origin); > > 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... >> diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh >> index e4850b778c..3bed17783b 100755 >> --- a/t/t5611-clone-config.sh >> +++ b/t/t5611-clone-config.sh >> @@ -37,6 +37,30 @@ test_expect_success 'clone -c config is available during >> clone' ' >> test_cmp expect child/file >> ' >> >> +test_expect_success 'clone -c remote.origin.fetch= works' ' >> + rm -rf child && >> + git update-ref refs/grab/it refs/heads/master && >> + git update-ref refs/keep/out refs/heads/master && >> + git clone -c "remote.origin.fetch=+refs/grab/*:refs/grab/*" . child && >> + ( >> + cd child && >> + git for-each-ref --format="%(refname)" refs/grab/ >../actual >> + ) && >> + echo refs/grab/it >expect && >> + test_cmp expect actual >> +' >> + >> +test_expect_success 'git -c remote.origin.fetch= clone works' ' >> + rm -rf child && >> + git -c "remote.origin.fetch=+refs/grab/*:refs/grab/*" clone . child && >> + ( >> + cd child && >> + git for-each-ref --format="%(refname)" refs/grab/ >../actual >> + ) && >> + echo refs/grab/it >expect && >> + test_cmp expect actual >> +' > > These look reasonable. Using "git -C for-each-ref" would save a > subshell, but that's minor. Loosing a subshell is good, but I subjectively like how the indentation highlights that that command operates on a different repository. However, the tests should also check that refs matching the default fetch refspec are transferred, too, i.e. that the c
Re: git-clone --config order & fetching extra refs during initial clone
On Mon, Feb 27, 2017 at 11:16:35AM -0800, Junio C Hamano wrote: > >> TL;DR: git-clone ignores any fetch specs passed via --config. > > > > I agree that this is a bug. There's some previous discussion and an RFC > > patch from lat March (author cc'd): > > > > > > http://public-inbox.org/git/1457313062-10073-1-git-send-email-sze...@ira.uka.de/ > > > > That discussion veered off into alternatives, but I think the v2 posted > > in that thread is taking a sane approach. > > Let's see how well it fares by cooking it in 'next' ;-) > > I think it was <1459349623-16443-1-git-send-email-sze...@ira.uka.de>, > which needs a bit of massaging to apply to the current codebase. Yeah, that is the most recent one I found. I didn't actually review it very carefully before, but I'll do so now (spoiler: a few nits, but it looks fine). > 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. > @@ -856,7 +861,9 @@ int cmd_clone(int argc, const char **argv, const char > *prefix) > int submodule_progress; > > struct refspec *refspec; > - const char *fetch_pattern; > + unsigned int refspec_count = 1; > + const char **fetch_patterns; > + const struct string_list *config_fetch_patterns; This "1" seems funny to up here by itself, as it must be kept in sync with the later logic that feeds exactly one non-configured refspec into our list. The current code isn't wrong, but it would be nice to have it all together. I.e., replacing: > + if (config_fetch_patterns) > + refspec_count = 1 + config_fetch_patterns->nr; > + fetch_patterns = xcalloc(refspec_count, sizeof(*fetch_patterns)); > + fetch_patterns[0] = value.buf; with: refspec_count = 1; if (config_fetch_patterns) refspec_count += config_fetch_patterns->nr; ... Though if I'm bikeshedding, I'd probably have written the whole thing with an argv_array and avoided counting at all. > + refspec = parse_fetch_refspec(refspec_count, fetch_patterns); > > + strbuf_reset(&key); > strbuf_reset(&value); > > remote = remote_get(option_origin); 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. > diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh > index e4850b778c..3bed17783b 100755 > --- a/t/t5611-clone-config.sh > +++ b/t/t5611-clone-config.sh > @@ -37,6 +37,30 @@ test_expect_success 'clone -c config is available during > clone' ' > test_cmp expect child/file > ' > > +test_expect_success 'clone -c remote.origin.fetch= works' ' > + rm -rf child && > + git update-ref refs/grab/it refs/heads/master && > + git update-ref refs/keep/out refs/heads/master && > + git clone -c "remote.origin.fetch=+refs/grab/*:refs/grab/*" . child && > + ( > + cd child && > + git for-each-ref --format="%(refname)" refs/grab/ >../actual > + ) && > + echo refs/grab/it >expect && > + test_cmp expect actual > +' > + > +test_expect_success 'git -c remote.origin.fetch= clone works' ' > + rm -rf child && > + git -c "remote.origin.fetch=+refs/grab/*:refs/grab/*" clone . child && > + ( > + cd child && > + git for-each-ref --format="%(refname)" refs/grab/ >../actual > + ) && > + echo refs/grab/it >expect && > + test_cmp expect actual > +' These look reasonable. Using "git -C for-each-ref" would save a subshell, but that's minor. If we wanted to be thorough, we could also check that the feature works correctly with "--origin" (I think it does). -Peff
Re: git-clone --config order & fetching extra refs during initial clone
Jeff King writes: > [Re-sending, as I used an old address for Gábor on the original] > > On Sat, Feb 25, 2017 at 07:12:38PM +, Robin H. Johnson wrote: > >> TL;DR: git-clone ignores any fetch specs passed via --config. > > I agree that this is a bug. There's some previous discussion and an RFC > patch from lat March (author cc'd): > > > http://public-inbox.org/git/1457313062-10073-1-git-send-email-sze...@ira.uka.de/ > > That discussion veered off into alternatives, but I think the v2 posted > in that thread is taking a sane approach. Let's see how well it fares by cooking it in 'next' ;-) I think it was <1459349623-16443-1-git-send-email-sze...@ira.uka.de>, which needs a bit of massaging to apply to the current codebase. -- >8 -- From: SZEDER Gábor Date: Wed, 30 Mar 2016 16:53:43 +0200 Subject: [PATCH] clone: respect configured fetch respecs during initial fetch Conceptually 'git clone' should behave as if the following commands were run: git init git config ... # set default configuration and origin remote git fetch git checkout # unless '--bare' is given However, that initial 'git fetch' behaves differently from any subsequent fetches, because it takes only the default fetch refspec into account and ignores all other fetch refspecs that might have been explicitly specified on the command line (e.g. 'git -c remote.origin.fetch= clone' or 'git clone -c ...'). Check whether there are any fetch refspecs configured for the origin remote and take all of them into account during the initial fetch as well. Signed-off-by: SZEDER Gábor --- builtin/clone.c | 36 t/t5611-clone-config.sh | 24 2 files changed, 52 insertions(+), 8 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index 3f63edbbf9..97229268b6 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -516,7 +516,7 @@ static struct ref *find_remote_branch(const struct ref *refs, const char *branch } static struct ref *wanted_peer_refs(const struct ref *refs, - struct refspec *refspec) + struct refspec *refspec, unsigned int refspec_count) { struct ref *head = copy_ref(find_ref_by_name(refs, "HEAD")); struct ref *local_refs = head; @@ -537,13 +537,18 @@ static struct ref *wanted_peer_refs(const struct ref *refs, warning(_("Could not find remote branch %s to clone."), option_branch); else { - get_fetch_map(remote_head, refspec, &tail, 0); + unsigned int i; + for (i = 0; i < refspec_count; i++) + get_fetch_map(remote_head, &refspec[i], &tail, 0); /* if --branch=tag, pull the requested tag explicitly */ get_fetch_map(remote_head, tag_refspec, &tail, 0); } - } else - get_fetch_map(refs, refspec, &tail, 0); + } else { + unsigned int i; + for (i = 0; i < refspec_count; i++) + get_fetch_map(refs, &refspec[i], &tail, 0); + } if (!option_mirror && !option_single_branch) get_fetch_map(refs, tag_refspec, &tail, 0); @@ -856,7 +861,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix) int submodule_progress; struct refspec *refspec; - const char *fetch_pattern; + unsigned int refspec_count = 1; + const char **fetch_patterns; + const struct string_list *config_fetch_patterns; packet_trace_identity("clone"); argc = parse_options(argc, argv, prefix, builtin_clone_options, @@ -1002,9 +1009,21 @@ int cmd_clone(int argc, const char **argv, const char *prefix) if (option_required_reference.nr || option_optional_reference.nr) setup_reference(); - fetch_pattern = value.buf; - refspec = parse_fetch_refspec(1, &fetch_pattern); + strbuf_addf(&key, "remote.%s.fetch", option_origin); + config_fetch_patterns = git_config_get_value_multi(key.buf); + if (config_fetch_patterns) + refspec_count = 1 + config_fetch_patterns->nr; + fetch_patterns = xcalloc(refspec_count, sizeof(*fetch_patterns)); + fetch_patterns[0] = value.buf; + if (config_fetch_patterns) { + struct string_list_item *fp; + unsigned int i = 1; + for_each_string_list_item(fp, config_fetch_patterns) + fetch_patterns[i++] = fp->string; + } + refspec = parse_fetch_refspec(refspec_count, fetch_patterns); + strbuf_reset(&key); strbuf_reset(&value); remote = remote_get(option_origin); @@ -1058,7 +1077,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) refs = transport_get_remote_refs(transport); if (refs) { - mapped_refs = want
Re: git-clone --config order & fetching extra refs during initial clone
[Re-sending, as I used an old address for Gábor on the original] On Sat, Feb 25, 2017 at 07:12:38PM +, Robin H. Johnson wrote: > TL;DR: git-clone ignores any fetch specs passed via --config. I agree that this is a bug. There's some previous discussion and an RFC patch from lat March (author cc'd): http://public-inbox.org/git/1457313062-10073-1-git-send-email-sze...@ira.uka.de/ That discussion veered off into alternatives, but I think the v2 posted in that thread is taking a sane approach. -Peff
Re: git-clone --config order & fetching extra refs during initial clone
On Sat, Feb 25, 2017 at 07:12:38PM +, Robin H. Johnson wrote: > TL;DR: git-clone ignores any fetch specs passed via --config. I agree that this is a bug. There's some previous discussion and an RFC patch from lat March (author cc'd): http://public-inbox.org/git/1457313062-10073-1-git-send-email-sze...@ira.uka.de/ That discussion veered off into alternatives, but I think the v2 posted in that thread is taking a sane approach. -Peff
git-clone --config order & fetching extra refs during initial clone
TL;DR: git-clone ignores any fetch specs passed via --config. The documentation for git-clone --config says: | Set a configuration variable in the newly-created repository; this takes | effect immediately __AFTER__ the repository is initialized, but __BEFORE__ | the remote history is fetched or any files checked out. [...] (emphasis added) However, this doesn't seem be be true, right after the clone, the refs are NOT present, and the next fetch seems to pull the extra refs. This seems to be because the refspec building for the initial clone doesn't take into account any fetch lines added to the config. Testcase to reproduce (confirmed in v2.11.1, not tested 2.12.0 quite yet): # export REPOURI=https://github.com/openstack-dev/sandbox.git DIR=test # git clone \ -c remote.origin.fetch=+refs/notes/*:refs/notes/* \ -c remote.origin.fetch=+refs/changes/*:refs/remotes/origin/changes/* \ $REPOURI $DIR \ && cd $DIR \ && git fetch -- Robin Hugh Johnson Gentoo Linux: Dev, Infra Lead, Foundation Trustee & Treasurer E-Mail : robb...@gentoo.org GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85 GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136 signature.asc Description: Digital signature