Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:
> The rewrite in C inadvertently broke updating with remote groups: when you
> pass parameters to "git remote update", it used to look up "remotes.<group>"
> for every parameter, and interpret the value as a list of remotes to update.
>
> Also, no parameter, or a single parameter "default" should update all
> remotes that have not been marked with "skipDefaultUpdate".
My reading of "sub update_remote {}" seems to indicate there still are
some differences. What the scripted version did was:
- Without extra parameter, update_remote() is run with "default";
- With parameters, update_remote() is run for each of them;
Then inside update_remote():
- grab "remotes.$name" configuration; if exists, then it is split using
"split(' ')", which is different from split(/ /).
Compare:
my @a = split(/ /, " A quick\t brown fox ");
my @b = split(' ', " A quick\t brown fox ");
print "<" . join("><", @a), ">\n";
print "<" . join("><", @b), ">\n";
The latter returns ('A', 'quick', 'brown', 'fox').
- If no "remotes.$name" configuration exists, and if the name is
"default", then grab all remotes that do not have
remote.$foo.skipDefaultUpdate set to true.
- Otherwise complain and die.
Then fetch each of them.
> +static int get_one_remote_for_update(struct remote *remote, void *priv)
> {
> + struct path_list *list = priv;
> if (!remote->skip_default_update)
> + path_list_append(xstrdup(remote->name), list);
> + return 0;
> +}
This is called when no extra parameter is given or the name is default.
It behaves differently from the scripted version if remote.default
configuration exists, doesn't it?
I suspect this is a regression to a useful feature. The user can specify
an explicit list of remotes to fetch from when he says "update default" or
just "update", without having to say skipdefaultupdate for all the usually
uninteresting remotes.
> +struct remote_group {
> + const char *name;
> + struct path_list *list;
> +} remote_group;
> +
> +static int get_remote_group(const char *key, const char *value)
> +{
> + if (!prefixcmp(key, "remotes.") &&
> + !strcmp(key + 8, remote_group.name)) {
> + char *space = strchr(value, ' ');
> + while (space) {
> + path_list_append(xstrndup(value, space - value),
> + remote_group.list);
> + value = space + 1;
> + space = strchr(value, ' ');
> + }
> + path_list_append(xstrdup(value), remote_group.list);
> + }
> +
This does not mimick the magic "split(' ', $conf)" in the scripted
version. We probably need a library function to parse whitespace
separated list of tokens, which may help other config parsers (color? I
was too lazy to check).
> static int update(int argc, const char **argv)
> {
> + int i, result = 0;
> + struct path_list list = { NULL, 0, 0, 0 };
>
> + if (argc < 2 || (argc == 2 && !strcmp(argv[1], "default")))
> + result = for_each_remote(get_one_remote_for_update, &list);
> + else {
> + remote_group.list = &list;
> + for (i = 1; i < argc; i++) {
> + remote_group.name = argv[i];
> + result = git_config(get_remote_group);
> + }
> + }
> + if (result)
> + return result;
>
> + for (i = 0; i < list.nr; i++)
> + result |= fetch_remote(list.items[i].path);
> + list.strdup_paths = 1;
> + path_list_clear(&list, 0);
This setting of strdup_paths after the fact and causing clear to free
things that were allocated while strdup_paths were set to 0 made me go
"Huh?", which means it needs explanation or code clarification.
> +
> + return result;
The scripted one did not stop fetching upon error and this does not
regress it, but it reports error, which I think is a good change.
--
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
