Re: [PATCH v2] clone: respect configured fetch respecs during initial fetch
Quoting Junio C Hamano : SZEDER Gábor writes: Quoting Junio C Hamano : IOW, special casing -c remote.origin.fetch=spec is a bad idea. I completely agree :) But it's the other way around. 'remote.origin.fetch=spec' during clone is special _now_, because the initial fetch ignores it, no matter where it is set. My patch makes it non-special, so that the initial fetch respects it, the same way it already respects 'fetch.fsckObjects' and 'fsck.unpackLimit', or the way the initial checkout respects e.g. 'core.eol'. ... but does "git -c core.eol clone" leave that configuration in the resulting repository's .git/config? "git -c user.name=foo" for that matter. No, and it shouldn't. 'git clone -c core.eol', however, should and indeed does. They may affect the one-shot operation but are not left in the resulting .git/config, which was what I was driving at. To make clone behave as if it is truly a short-hand of git init git config ;# with default and necessary tweaks to adjust ;# for things like -b, -o, --single-branch git fetch git checkout which by the way I think everybody agrees is a worth goal, then shouldn't the update to the current code be more like "prepare the default config, tweak with whatever configuration necessary, and re-read the config before driving the equivalent of 'git fetch'?" And the conclusion my rhetorical questions led to was that "adjust for things like..." should not include what comes from "-c var=val" because there is no sensible way to incorporate them in general. The most important point is that "-c var=val" is the wrong source of information to blindly propagete to the resulting .git/config. In case of 'git -c var=val clone', I agree, but then again, 'git clone -c var=val' was specifically designed to write that 'var=val' to the new repository's config file. Config variables set in the global or system-wide config files are not written to the new repository's config file either. And the point of "--branches" option is not that it would be short and tidy, but it is more targetted. With such an approach, nobody would imagine "git -c random.var=value clone" would be propagated into the resulting .git/config in a random and unspecified way. Once you learn what custom set of refs the user wants to fetch, you would need futzing of the refspecs like you did in your patch. That part of your patch is salvageable. The part that special cased the information that came from "-c remote.origin.fetch" while ignoring others like user.name that came from exactly the same mechanism via "-c user.name" is not. My patch did not special case anything and it did not change anything with respect to what config settings are written under what circumstances to the new repository's config file. All that already works properly and almost all those config settings are already taken into account when they should be by the commands in our conceptual model of 'git clone'. It doesn't matter at all where and how they were specified or whether they are written to the new repository's config file or not, almost all of them are taken into account. Almost all, because there is that one exception: additional fetch refspecs are ignored during the initial fetch. And all my patch did was to make the initial fetch aware of any additional fetch refspecs which are configured when the initial fetch is executed. (and again: no matter where those refspecs were specified and no matter whether they were written to the new config file) Eh, I guess I should have went for a refined version of the RFC's commit message, rewriting it based on that conceptual modell caused way too much confusion. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] clone: respect configured fetch respecs during initial fetch
Quoting Junio C Hamano : SZEDER Gábor writes: 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 ...'). Is it really 'git fetch' behaves differently? Certainly. What is missing in the above description is your expected behaviour of "git -c var=val clone", and without it we cannot tell if your expectation is sane to begin with. These 'git -c var=val cmd' one-shot configuration parameters exist during the lifespan of the command. Therefore, in case of 'git -c var=val clone' they should exist while all the commands in our mental model are executed. IOW, those commands should behave as if these configuration parameters were specified for them, see below. Is the expectation like this? git init git config ... # set default configuration and origin remote git config var val # update with what "-c var=val" told us git fetch git checkout # unless '--bare' is given or is it something else? For 'git -c var=val clone': git -c var=val init git -c var=val config ... # though this probably doesn't really # matter, as it is about writing the # configuration, and it gets the # to-be-written variables and values # in the "..." part anyway git -c var=val fetch git -c var=val checkout Being one-shot configuration parameters, they shouldn't be written to the new repository's config file. 'git clone -c var=val' is designed to be different: - it does write var=val into the new repository's config file - it specifies that var.val "takes effect immediately after the repository is initialized, but before the remote history is fetched or any files checked out". Additionally, there may be relevant config variables defined in the global and system-wide config files, which of course should be respected by all these commands. And it all works just fine as described above, even the initial fetch respects most of the config variables, wherever specified, except for fetch refspecs which are completely ignored. Is "-c var=val" adding to the config variables set by default, or is it replacing them? Does the choice depend on the nature of individual variables, and if so what is the criteria? It's up to the individual command how it treats a particular config variable: single-valued variables like 'fetch.fsckObjects' should override (they already do), multi-valued variables like fetch refspecs should be added. Running as part of 'git clone' shouldn't matter at all. This patch only affects how fetch refspecs are handled, the effects of other config variables are unchanged. Are all "-c var=val" update the configuration of the resulting repository? Or are certain "var"s treated as special and placed in the config but not other "var"s? If the latter, what makes these certain "var"s special? In this regard it doesn't matter what 'val=var' is. What matters is how the configuration parameter is specified (i.e. 'git -c var=val clone' vs. 'git clone -c var=val'). This patch doesn't change anything in this regard. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] clone: respect configured fetch respecs during initial fetch
SZEDER Gábor writes: > Quoting Junio C Hamano : > >> IOW, special casing -c remote.origin.fetch=spec >> is a bad idea. > > I completely agree :) > But it's the other way around. > > 'remote.origin.fetch=spec' during clone is special _now_, because the > initial fetch ignores it, no matter where it is set. > > My patch makes it non-special, so that the initial fetch respects it, > the same way it already respects 'fetch.fsckObjects' and > 'fsck.unpackLimit', or the way the initial checkout respects e.g. > 'core.eol'. ... but does "git -c core.eol clone" leave that configuration in the resulting repository's .git/config? "git -c user.name=foo" for that matter. They may affect the one-shot operation but are not left in the resulting .git/config, which was what I was driving at. To make clone behave as if it is truly a short-hand of git init git config ;# with default and necessary tweaks to adjust ;# for things like -b, -o, --single-branch git fetch git checkout which by the way I think everybody agrees is a worth goal, then shouldn't the update to the current code be more like "prepare the default config, tweak with whatever configuration necessary, and re-read the config before driving the equivalent of 'git fetch'?" And the conclusion my rhetorical questions led to was that "adjust for things like..." should not include what comes from "-c var=val" because there is no sensible way to incorporate them in general. The most important point is that "-c var=val" is the wrong source of information to blindly propagete to the resulting .git/config. And the point of "--branches" option is not that it would be short and tidy, but it is more targetted. With such an approach, nobody would imagine "git -c random.var=value clone" would be propagated into the resulting .git/config in a random and unspecified way. Once you learn what custom set of refs the user wants to fetch, you would need futzing of the refspecs like you did in your patch. That part of your patch is salvageable. The part that special cased the information that came from "-c remote.origin.fetch" while ignoring others like user.name that came from exactly the same mechanism via "-c user.name" is not. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] clone: respect configured fetch respecs during initial fetch
Quoting Junio C Hamano : IOW, special casing -c remote.origin.fetch=spec is a bad idea. I completely agree :) But it's the other way around. 'remote.origin.fetch=spec' during clone is special _now_, because the initial fetch ignores it, no matter where it is set. My patch makes it non-special, so that the initial fetch respects it, the same way it already respects 'fetch.fsckObjects' and 'fsck.unpackLimit', or the way the initial checkout respects e.g. 'core.eol'. So how about teaching "git clone" a new _option_ that is about what branches are followed? git clone $there --branches="master next pu" would give [remote "origin"] fetch = +refs/heads/master:refs/remotes/origin/master fetch = +refs/heads/next:refs/remotes/origin/next fetch = +refs/heads/pu:refs/remotes/origin/pu Without my patch the initial fetch would ignore these refspecs, too. instead of the usual [remote "origin"] fetch = +refs/heads/*:refs/remotes/origin/* Typing only branch names is much shorter and simpler than typing the name of a config var and full refspecs, so this would be a nice simplification of the UI for the case when the user is only interested in certain branches. But it wouldn't help if the user wants to include 'refs/interesting/*' in the initial fetch. And that can be made to work orthognonal to --single-branch by a small additional rule: if the branch given by -b (or their HEAD) is not part of --branches, then we add it to the set of branches to be followed (i.e. if you give only --single-branch, without --branches, the set of branches to be followed will become that single branch). Hmm? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] clone: respect configured fetch respecs during initial fetch
Junio C Hamano writes: > Is the expectation like this? > > git init > git config ... # set default configuration and origin remote > git config var val # update with what "-c var=val" told us > git fetch > git checkout # unless '--bare' is given > > or is it something else? > > Is "-c var=val" adding to the config variables set by default, or is > it replacing them? Does the choice depend on the nature of > individual variables, and if so what is the criteria? > > Are all "-c var=val" update the configuration of the resulting > repository? Or are certain "var"s treated as special and placed in > the config but not other "var"s? If the latter, what makes these > certain "var"s special? > > These design decisions need to be explained so that they will serve > to guide people to decide what other variables to propagate and how > when they have to add new configuration variables in the future. > Otherwise we'd end up with an inconsistent mess. The above did not start as rhetorical questions, but was merely me thinking aloud. However, it showed me a different approach might be more appropriate. Taken as rhetorical questions, the sane answers to them would revolve around "we do not know the semantics of each and every configuration variable that will be given to this codepath, and by definition we will never know in advance the ones that will be introduced later". IOW, special casing -c remote.origin.fetch=spec is a bad idea. So how about teaching "git clone" a new _option_ that is about what branches are followed? git clone $there --branches="master next pu" would give [remote "origin"] fetch = +refs/heads/master:refs/remotes/origin/master fetch = +refs/heads/next:refs/remotes/origin/next fetch = +refs/heads/pu:refs/remotes/origin/pu instead of the usual [remote "origin"] fetch = +refs/heads/*:refs/remotes/origin/* And that can be made to work orthognonal to --single-branch by a small additional rule: if the branch given by -b (or their HEAD) is not part of --branches, then we add it to the set of branches to be followed (i.e. if you give only --single-branch, without --branches, the set of branches to be followed will become that single branch). Hmm? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] clone: respect configured fetch respecs during initial fetch
SZEDER Gábor writes:
> 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 ...').
Is it really 'git fetch' behaves differently?
What is missing in the above description is your expected behaviour
of "git -c var=val clone", and without it we cannot tell if your
expectation is sane to begin with.
Is the expectation like this?
git init
git config ... # set default configuration and origin remote
git config var val # update with what "-c var=val" told us
git fetch
git checkout # unless '--bare' is given
or is it something else?
Is "-c var=val" adding to the config variables set by default, or is
it replacing them? Does the choice depend on the nature of
individual variables, and if so what is the criteria?
Are all "-c var=val" update the configuration of the resulting
repository? Or are certain "var"s treated as special and placed in
the config but not other "var"s? If the latter, what makes these
certain "var"s special?
These design decisions need to be explained so that they will serve
to guide people to decide what other variables to propagate and how
when they have to add new configuration variables in the future.
Otherwise we'd end up with an inconsistent mess.
> 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
> ---
> Changes since previous (RFC) version:
> - new commit message
> - additional configured fetch refspecs are taken into account with
>'--single-branch' as well
>
> builtin/clone.c | 36
> t/t5708-clone-config.sh | 24
> 2 files changed, 52 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 661639255c56..5e2d2c21e456 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -515,7 +515,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;
> @@ -536,13 +536,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);
> @@ -840,7 +845,9 @@ int cmd_clone(int argc, const char **argv, const char
> *prefix)
> int err = 0, complete_refs_before_fetch = 1;
>
> 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,
> @@ -967,9 +974,21 @@ int cmd_clone(int argc, const char **argv, const char
> *prefix)
> if (option_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;
> +
[PATCH v2] 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
---
Changes since previous (RFC) version:
- new commit message
- additional configured fetch refspecs are taken into account with
'--single-branch' as well
builtin/clone.c | 36
t/t5708-clone-config.sh | 24
2 files changed, 52 insertions(+), 8 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 661639255c56..5e2d2c21e456 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -515,7 +515,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;
@@ -536,13 +536,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);
@@ -840,7 +845,9 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
int err = 0, complete_refs_before_fetch = 1;
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,
@@ -967,9 +974,21 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
if (option_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);
@@ -1013,7 +1032,7 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
refs = transport_get_remote_refs(transport);
if (refs) {
- mapped_refs = wanted_peer_refs(refs, refspec);
+ mapped_refs = wanted_peer_refs(refs, refspec, refspec_count);
/*
* transport_get_remote_refs() may return refs with null sha-1
* in mapped_refs (see struct transport->get_refs_list
@@ -1094,6 +1113,7 @@ 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;
}
diff --git a/t/t5708-clone-config.sh b/t/t5708-clone-config.sh
index 27d730c0a720..136a8611c7f3 100755
--- a/t/t5708-clone-config.sh
+++ b/t/t5708-clone-config.sh
@@ -37,4 +37,28 @@ test_expect_success 'clone -c config is available during
c
