[PATCH 1/3] git-remote: match usage string with the manual pages

[PATCH 1/3] git-remote: match usage string with the manual pages

[crquan]

From: Cheng Renquan <crquan@gmail.com>

Signed-off-by: Cheng Renquan <crquan@gmail.com>
---
 builtin-remote.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index 71696b5..d032f25 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -8,12 +8,12 @@
 #include "refs.h"
 
 static const char * const builtin_remote_usage[] = {
-	"git remote",
-	"git remote add <name> <url>",
+	"git remote [-v | --verbose]",
+	"git remote add [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>",
 	"git remote rename <old> <new>",
 	"git remote rm <name>",
-	"git remote show <name>",
-	"git remote prune <name>",
+	"git remote show [-n] <name>",
+	"git remote prune [-n | --dry-run] <name>",
 	"git remote update [group]",
 	NULL
 };
-- 
1.6.0.2

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

[crquan]

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
   = [up to date]      maint      -> origin/maint
   = [up to date]      man        -> origin/man
   = [up to date]      master     -> origin/master
   = [up to date]      next       -> origin/next
   = [up to date]      pu         -> origin/pu
   = [up to date]      todo       -> origin/todo

Signed-off-by: Cheng Renquan <crquan@gmail.com>
---
 builtin-remote.c |   20 ++++++++++++++------
 1 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index d032f25..fff9920 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -14,7 +14,7 @@ static const char * const builtin_remote_usage[] = {
 	"git remote rm <name>",
 	"git remote show [-n] <name>",
 	"git remote prune [-n | --dry-run] <name>",
-	"git remote update [group]",
+	"git remote update [-v | --verbose] [group]",
 	NULL
 };
 
@@ -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);
@@ -117,7 +121,7 @@ static int add(int argc, const char **argv)
 			return 1;
 	}
 
-	if (fetch && fetch_remote(name))
+	if (fetch && fetch_remote(name, url))
 		return 1;
 
 	if (master) {
@@ -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;
 }
 
@@ -818,7 +826,7 @@ static int update(int argc, const char **argv)
 		result = for_each_remote(get_one_remote_for_update, &list);
 
 	for (i = 0; i < list.nr; i++)
-		result |= fetch_remote(list.items[i].string);
+		result |= fetch_remote(list.items[i].string, list.items[i].util);
 
 	/* all names were strdup()ed or strndup()ed */
 	list.strdup_strings = 1;
-- 
1.6.0.2

[PATCH 3/3] git-remote: simplifying get_one_entry

[crquan]

From: Cheng Renquan <crquan@gmail.com>

The loop for remote->url_nr is really useless,
set to the last one directly is better.

Signed-off-by: Cheng Renquan <crquan@gmail.com>
---
 builtin-remote.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index fff9920..59d69a5 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -839,13 +839,9 @@ static int get_one_entry(struct remote *remote, void *priv)
 {
 	struct string_list *list = priv;
 
-	if (remote->url_nr > 0) {
-		int i;
-
-		for (i = 0; i < remote->url_nr; i++)
-			string_list_append(remote->name, list)->util = (void *)remote->url[i];
-	} else
-		string_list_append(remote->name, list)->util = NULL;
+	string_list_append(remote->name, list)->util =
+		remote->url_nr > 0
+		? (void *)remote->url[remote->url_nr-1] : NULL;
 
 	return 0;
 }
-- 
1.6.0.2

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

[Junio C Hamano]

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

Re: [PATCH 3/3] git-remote: simplifying get_one_entry

[Junio C Hamano]

crquan@gmail.com writes:

> From: Cheng Renquan <crquan@gmail.com>
>
> The loop for remote->url_nr is really useless, set to the last one
> directly is better.

Is it really useless?  Be more descriptive.

> -	if (remote->url_nr > 0) {
> -		int i;
> -
> -		for (i = 0; i < remote->url_nr; i++)
> -			string_list_append(remote->name, list)->util = (void *)remote->url[i];
> -	} else
> -		string_list_append(remote->name, list)->util = NULL;
> +	string_list_append(remote->name, list)->util =
> +		remote->url_nr > 0
> +		? (void *)remote->url[remote->url_nr-1] : NULL;

When you have more than one URL associated with the remote (this makes
sense only for pushing), the current code adds that many string_list_item
to the list, each holding the URL.  "git remote -v" shows all of them.

Your change instead creates only one string_list_item and hold the last
URL.  Doesn't it make show_all() to show only one URL for the remote?

Re: [PATCH 3/3] git-remote: simplifying get_one_entry

[rae l]

On Tue, Nov 18, 2008 at 12:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
> crquan@gmail.com writes:
>
>> From: Cheng Renquan <crquan@gmail.com>
>>
>> The loop for remote->url_nr is really useless, set to the last one
>> directly is better.
>
> Is it really useless?  Be more descriptive.
>
>> -     if (remote->url_nr > 0) {
>> -             int i;
>> -
>> -             for (i = 0; i < remote->url_nr; i++)
>> -                     string_list_append(remote->name, list)->util = (void *)remote->url[i];
>> -     } else
>> -             string_list_append(remote->name, list)->util = NULL;
>> +     string_list_append(remote->name, list)->util =
>> +             remote->url_nr > 0
>> +             ? (void *)remote->url[remote->url_nr-1] : NULL;
>
> When you have more than one URL associated with the remote (this makes
> sense only for pushing), the current code adds that many string_list_item
> to the list, each holding the URL.  "git remote -v" shows all of them.
>
> Your change instead creates only one string_list_item and hold the last
> URL.  Doesn't it make show_all() to show only one URL for the remote?
Sorry, this patch is totally wrong, I will regenerate the other two and resend.

Thanks for your patience.

-- 
Cheng Renquan, Shenzhen, China
Steven Wright  - "Cross country skiing is great if you live in a small country."