Re: [PATCH 2/3] git-remote: add verbose mode to git remote update

[Junio C Hamano <gitster@pobox.com>]

crquan@gmail.com writes:

> 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.

> @@ -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.

> @@ -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