threads / patch / 26484

patch, 4 partsExtract function trim_url and optimize calls of it.

Subject: [PATCH 2/4] Extract function trim_url and optimize calls of it.

## tl;dr

12 messages between Feb 12, 2011 and Feb 17, 2011. Diffs are folded; open one to read it.

replies: 11people: 4as markdown or json

Vasyl' Vavrychuk· Feb 12, 2011, 22:38 UTC · lore

[PATCH 0/4] clean-up store_updated_refs

Vasyl' Vavrychuk (4):
  Reduce url_len variable scope
  Extract function trim_url and optimize calls of it.
  Dont use the same variable for different things
  Replace pointer arithmetic with strbuf
 builtin/fetch.c |   63 ++++++++++++++++++++++++++++++++----------------------
 1 files changed, 37 insertions(+), 26 deletions(-)
Vasyl' Vavrychuk· Feb 12, 2011, 22:38 UTC · re: Vasyl' Vavrychuk · lore

[PATCH 1/4] Reduce url_len variable scope

Signed-off-by: Vasyl' Vavrychuk <vvavrychuk@gmail.com>
---
 builtin/fetch.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
Show changes to builtin/fetch.c +3 −3
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 357f3cd..2b0b11e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -385,6 +385,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 		url_len = i + 1;
 		if (4 < i && !strncmp(".git", url + i - 3, 4))
 			url_len = i - 3;
+		url[url_len] = '\0';
 
 		note_len = 0;
 		if (*what) {
@@ -399,7 +400,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 				    rm->old_sha1),
 			rm->merge ? "" : "not-for-merge",
 			note);
-		for (i = 0; i < url_len; ++i)
+		for (i = 0; url[i]; ++i)
 			if ('\n' == url[i])
 				fputs("\\n", fp);
 			else
@@ -415,8 +416,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 				 REFCOL_WIDTH, *what ? what : "HEAD");
 		if (*note) {
 			if (verbosity >= 0 && !shown_url) {
-				fprintf(stderr, "From %.*s\n",
-						url_len, url);
+				fprintf(stderr, "From %s\n", url);
 				shown_url = 1;
 			}
 			if (verbosity >= 0)

-- 
1.7.1
Junio C Hamano· Feb 17, 2011, 01:43 UTC · re: Vasyl' Vavrychuk · lore

Re: [PATCH 1/4] Reduce url_len variable scope

Vasyl' Vavrychuk <vvavrychuk@gmail.com> writes:
> Subject: Re: [PATCH 1/4] Reduce url_len variable scope

Please mark the patch in such a way that the readers can tell what file/function this patch is about by reading "git shortlog" output without anything else, i.e.

	Subject: builtin/fetch.c: shorten lifetime of the url_len variable

Hmph. While this does not bring in any new bug to the code, what benefit do we gain from it?

The scope is not reduced (even though the variable's lifespan is shortened in the scope). Are we so short of registers that this change matters?

Show 36 quoted lines
>  builtin/fetch.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 357f3cd..2b0b11e 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -385,6 +385,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>  		url_len = i + 1;
>  		if (4 < i && !strncmp(".git", url + i - 3, 4))
>  			url_len = i - 3;
> +		url[url_len] = '\0';
>  
>  		note_len = 0;
>  		if (*what) {
> @@ -399,7 +400,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>  				    rm->old_sha1),
>  			rm->merge ? "" : "not-for-merge",
>  			note);
> -		for (i = 0; i < url_len; ++i)
> +		for (i = 0; url[i]; ++i)
>  			if ('\n' == url[i])
>  				fputs("\\n", fp);
>  			else
> @@ -415,8 +416,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>  				 REFCOL_WIDTH, *what ? what : "HEAD");
>  		if (*note) {
>  			if (verbosity >= 0 && !shown_url) {
> -				fprintf(stderr, "From %.*s\n",
> -						url_len, url);
> +				fprintf(stderr, "From %s\n", url);
>  				shown_url = 1;
>  			}
>  			if (verbosity >= 0)
> -- 
> 1.7.1
Vasyl' Vavrychuk· Feb 12, 2011, 22:38 UTC · re: Vasyl' Vavrychuk · lore
Extract compact code into trim_url. Dont call it every iteration in the loop since no reason.
Signed-off-by: Vasyl' Vavrychuk <vvavrychuk@gmail.com>
---
 builtin/fetch.c |   29 ++++++++++++++++++-----------
 1 files changed, 18 insertions(+), 11 deletions(-)
Show changes to builtin/fetch.c +18 −11
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 2b0b11e..46d8fd6 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -89,6 +89,19 @@ static void unlock_pack_on_signal(int signo)
 	raise(signo);
 }
 
+static void trim_url(char *url)
+{
+	int i, url_len;
+
+	url_len = strlen(url);
+	for (i = url_len - 1; url[i] == '/' && 0 <= i; i--)
+		;
+	url_len = i + 1;
+	if (4 < i && !strncmp(".git", url + i - 3, 4))
+		url_len = i - 3;
+	url[url_len] = '\0';
+}
+
 static void add_merge_config(struct ref **head,
 			   const struct ref *remote_refs,
 		           struct branch *branch,
@@ -329,7 +342,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 {
 	FILE *fp;
 	struct commit *commit;
-	int url_len, i, note_len, shown_url = 0, rc = 0;
+	int i, note_len, shown_url = 0, rc = 0;
 	char note[1024];
 	const char *what, *kind;
 	struct ref *rm;
@@ -339,10 +352,12 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 	if (!fp)
 		return error("cannot open %s: %s\n", filename, strerror(errno));
 
-	if (raw_url)
+	if (raw_url) {
 		url = transport_anonymize_url(raw_url);
-	else
+		trim_url(url);
+	} else {
 		url = xstrdup("foreign");
+	}
 	for (rm = ref_map; rm; rm = rm->next) {
 		struct ref *ref = NULL;
 
@@ -379,14 +394,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 			what = rm->name;
 		}
 
-		url_len = strlen(url);
-		for (i = url_len - 1; url[i] == '/' && 0 <= i; i--)
-			;
-		url_len = i + 1;
-		if (4 < i && !strncmp(".git", url + i - 3, 4))
-			url_len = i - 3;
-		url[url_len] = '\0';
-
 		note_len = 0;
 		if (*what) {
 			if (*kind)

-- 
1.7.1
Junio C Hamano· Feb 17, 2011, 01:44 UTC · re: Vasyl' Vavrychuk · lore

Re: [PATCH 2/4] Extract function trim_url and optimize calls of it.

Vasyl' Vavrychuk <vvavrychuk@gmail.com> writes:
> Extract compact code into trim_url. Dont call it every iteration in the loop since no reason.
Too long a line; just drop " since no reason".
Show 15 quoted lines
> @@ -379,14 +394,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>  			what = rm->name;
>  		}
>  
> -		url_len = strlen(url);
> -		for (i = url_len - 1; url[i] == '/' && 0 <= i; i--)
> -			;
> -		url_len = i + 1;
> -		if (4 < i && !strncmp(".git", url + i - 3, 4))
> -			url_len = i - 3;
> -		url[url_len] = '\0';
> -
>  		note_len = 0;
>  		if (*what) {
>  			if (*kind)

We repeatedly called strlen(url) for each entry in the ref-map when we know we do not have any more thing to do; silly. This is probably a good thing to do.

Vasyl' Vavrychuk· Feb 12, 2011, 22:38 UTC · re: Vasyl' Vavrychuk · lore

[PATCH 3/4] Dont use the same variable for different things

Signed-off-by: Vasyl' Vavrychuk <vvavrychuk@gmail.com>
---
 builtin/fetch.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)
Show changes to builtin/fetch.c +7 −6
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 46d8fd6..ddb40c6 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -343,7 +343,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 	FILE *fp;
 	struct commit *commit;
 	int i, note_len, shown_url = 0, rc = 0;
-	char note[1024];
+	char note[1024], display[1024];
 	const char *what, *kind;
 	struct ref *rm;
 	char *url, *filename = dry_run ? "/dev/null" : git_path("FETCH_HEAD");
@@ -415,19 +415,20 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 		fputc('\n', fp);
 
 		if (ref) {
-			rc |= update_local_ref(ref, what, note);
+			rc |= update_local_ref(ref, what, display);
 			free(ref);
-		} else
-			sprintf(note, "* %-*s %-*s -> FETCH_HEAD",
+		} else {
+			sprintf(display, "* %-*s %-*s -> FETCH_HEAD",
 				TRANSPORT_SUMMARY_WIDTH, *kind ? kind : "branch",
 				 REFCOL_WIDTH, *what ? what : "HEAD");
-		if (*note) {
+		}
+		if (*display) {
 			if (verbosity >= 0 && !shown_url) {
 				fprintf(stderr, "From %s\n", url);
 				shown_url = 1;
 			}
 			if (verbosity >= 0)
-				fprintf(stderr, " %s\n", note);
+				fprintf(stderr, " %s\n", display);
 		}
 	}
 	free(url);

-- 
1.7.1
Junio C Hamano· Feb 17, 2011, 01:44 UTC · re: Vasyl' Vavrychuk · lore

Re: [PATCH 3/4] Dont use the same variable for different things

Vasyl' Vavrychuk <vvavrychuk@gmail.com> writes:
> Subject: Re: [PATCH 3/4] Dont use the same variable for different things
	Subject: builtin/fetch.c: do not use ... different things
But more important question is why?

What benefit are we getting by almost doubling the stack footprint of this function? What problem are you fixing?

João P. Sampaio· Feb 17, 2011, 02:36 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/4] Dont use the same variable for different things

On Wed, Feb 16, 2011 at 23:44, Junio C Hamano <gitster@pobox.com> wrote:
> But more important question is why?
>
> What benefit are we getting by almost doubling the stack footprint of this
> function?  What problem are you fixing?

Well, in matters of code readability and maintainability, it's best to use different variables for different matters...

Nowadays, memory isn't really an issue anymore, is it? I think it's valid to make code more readable, if memory cost isn't absurd.

-- 
João Paulo Melo de Sampaio
Computer Engineering Student @ UFSCar
Website: http://www.jpmelos.com
Twitter: http://twitter.com/jpmelos (@jpmelos)
Vasyl' Vavrychuk· Feb 12, 2011, 22:38 UTC · re: Vasyl' Vavrychuk · lore

[PATCH 4/4] Replace pointer arithmetic with strbuf

Signed-off-by: Vasyl' Vavrychuk <vvavrychuk@gmail.com>
---
 builtin/fetch.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)
Show changes to builtin/fetch.c +12 −9
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ddb40c6..eaea475 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -342,8 +342,9 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 {
 	FILE *fp;
 	struct commit *commit;
-	int i, note_len, shown_url = 0, rc = 0;
-	char note[1024], display[1024];
+	int i, shown_url = 0, rc = 0;
+	struct strbuf note = STRBUF_INIT;
+	char display[1024];
 	const char *what, *kind;
 	struct ref *rm;
 	char *url, *filename = dry_run ? "/dev/null" : git_path("FETCH_HEAD");
@@ -394,19 +395,20 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 			what = rm->name;
 		}
 
-		note_len = 0;
 		if (*what) {
-			if (*kind)
-				note_len += sprintf(note + note_len, "%s ",
-						    kind);
-			note_len += sprintf(note + note_len, "'%s' of ", what);
+			if (*kind) {
+				strbuf_addstr(&note, kind);
+				strbuf_addch(&note, ' ');
+			}
+			strbuf_addf(&note, "'%s' of ", what);
 		}
-		note[note_len] = '\0';
 		fprintf(fp, "%s\t%s\t%s",
 			sha1_to_hex(commit ? commit->object.sha1 :
 				    rm->old_sha1),
 			rm->merge ? "" : "not-for-merge",
-			note);
+			note.buf);
+		strbuf_reset(&note);
+
 		for (i = 0; url[i]; ++i)
 			if ('\n' == url[i])
 				fputs("\\n", fp);
@@ -431,6 +433,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 				fprintf(stderr, " %s\n", display);
 		}
 	}
+	strbuf_release(&note);
 	free(url);
 	fclose(fp);
 	if (rc & STORE_REF_ERROR_DF_CONFLICT)

-- 
1.7.1
Sverre Rabbelier· Feb 13, 2011, 00:09 UTC · re: Vasyl' Vavrychuk · lore

Re: [PATCH 0/4] clean-up store_updated_refs

Heya,
On Sat, Feb 12, 2011 at 23:38, Vasyl' Vavrychuk <vvavrychuk@gmail.com> wrote:
> Vasyl' Vavrychuk (4):

Neither the patches, nor this cover letter explain _why_ this is being done. Motivation needed :).

-- 
Cheers,

Sverre Rabbelier
Vasyl'· Feb 13, 2011, 18:37 UTC · re: Sverre Rabbelier · lore

Re: [PATCH 0/4] clean-up store_updated_refs

Hi,

I've needed this as part of bigger change which is collecting all .git/FETCH_HEAD related code across different files to fetch-head.[ch]. Which in its case I do as part of git-pull.sh to C porting. So thats motivation chain :)

On Sun, Feb 13, 2011 at 2:09 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
Show 13 quoted lines
> Heya,
>
> On Sat, Feb 12, 2011 at 23:38, Vasyl' Vavrychuk <vvavrychuk@gmail.com> wrote:
>> Vasyl' Vavrychuk (4):
>
> Neither the patches, nor this cover letter explain _why_ this is being
> done. Motivation needed :).
>
> --
> Cheers,
>
> Sverre Rabbelier
>
Sverre Rabbelier· Feb 13, 2011, 18:46 UTC · re: Vasyl' · lore

Re: [PATCH 0/4] clean-up store_updated_refs

Heya,
On Sun, Feb 13, 2011 at 19:37, Vasyl' <vvavrychuk@gmail.com> wrote:
> I've needed this as part of bigger change which is collecting all
> .git/FETCH_HEAD related code across different files to
> fetch-head.[ch]. Which in its case I do as part of git-pull.sh to C
> porting. So thats motivation chain :)

Thanks, that's the kind of stuff that should go into the cover letter / the patch description.

-- 
Cheers,

Sverre Rabbelier

← back to recent threads