Re: [PATCH 2/3] git-remote: add verbose mode to git remote update
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 17, 2008, 16:46 UTC
- Message-ID
- <7vljviwbav.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <1226920551-28303-2-git-send-email-crquan@gmail.com>
crquan@gmail.com writes:
Show 13 quoted lines
> From: Cheng Renquan <crquan@gmail.com> > > Pass the verbose mode parameter to the underlying fetch command. > > $ ./git remote -v update > Updating origin > From git://git.kernel.org/pub/scm/git/git > = [up to date] html -> origin/html > ... > - "git remote update [group]", > + "git remote update [-v | --verbose] [group]", > NULL > };
Hmm, ok. I do not think "git remote update -v" would work, though.
Show 16 quoted lines
> @@ -40,9 +40,13 @@ static int opt_parse_track(const struct option *opt, const char *arg, int not)
> return 0;
> }
>
> -static int fetch_remote(const char *name)
> +static int fetch_remote(const char *name, const char *url)
> {
> - const char *argv[] = { "fetch", name, NULL };
> + const char *argv[] = { "fetch", name, NULL, NULL };
> + if (verbose) {
> + argv[1] = "-v";
> + argv[2] = name;
> + }
> printf("Updating %s\n", name);
> if (run_command_v_opt(argv, RUN_GIT_CMD))
> return error("Could not fetch %s", name);I do not think this new parameter "url" is used anywhere in this function; please drop the change in the function signature. That would make your change to "add()", "get_one_remote_for_update()", and "update()" all unnecessary.
Show 14 quoted lines
> @@ -769,8 +773,12 @@ static int prune(int argc, const char **argv)
> static int get_one_remote_for_update(struct remote *remote, void *priv)
> {
> struct string_list *list = priv;
> +
> if (!remote->skip_default_update)
> - string_list_append(xstrdup(remote->name), list);
> + string_list_append(remote->name, list)->util =
> + remote->url_nr > 0
> + ? (void *)remote->url[remote->url_nr-1] : NULL;
> +
> return 0;
> }
> I notice that you dropped xstrdup() without explanation. While I think it is a valid leak fix, that should be done as a separate commit (shown below).
By the way, you fixed mismatch between the documentation and usage string in an earlier patch, but you broke it yourself ;-). Please fix it up.
Thanks.
-- >8 -- Subject: builtin-remote.c: plug a small memory leak in get_one_remote_for_updates()
We know that the string pointed at by remote->name won't change. It can be borrowed as the key in the string_list without copying. Other parts of existing code such as get_one_entry() already rely on this fact.
Noticed by Cheng Renquan.
Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin-remote.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/builtin-remote.c b/builtin-remote.c index d032f25..14774e3 100644 --- a/builtin-remote.c +++ b/builtin-remote.c @@ -770,7 +770,7 @@ static int get_one_remote_for_update(struct remote *remote, void *priv) { struct string_list *list = priv; if (!remote->skip_default_update) - string_list_append(xstrdup(remote->name), list); + string_list_append(remote->name, list); return 0; }
-- 1.6.0.4.772.g2ebfe