threads / patch / 19924

patchgit-remote: fixed missing .uploadpack usage for show command

Subject: [PATCH] git-remote: fixed missing .uploadpack usage for show command

## tl;dr

4 messages between Jun 25, 2009 and Jun 25, 2009. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Chris Frey· Jun 25, 2009, 09:00 UTC · lore

When using 'git remote show <name>', the remote HEAD check did not use the uploadpack configuration setting.

Signed-off-by: Chris Frey <cdfrey@foursquare.net>
---
 builtin-remote.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to builtin-remote.c +1 −1
diff --git a/builtin-remote.c b/builtin-remote.c
index 658d578..ec1f903 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -787,7 +787,7 @@ static int get_remote_ref_states(const char *name,
 	read_branches();
 
 	if (query) {
-		transport = transport_get(NULL, states->remote->url_nr > 0 ?
+		transport = transport_get(states->remote, states->remote->url_nr > 0 ?
 			states->remote->url[0] : NULL);
 		remote_refs = transport_get_remote_refs(transport);
 		transport_disconnect(transport);

-- 
1.6.2.5
Junio C Hamano· Jun 25, 2009, 18:32 UTC · re: Chris Frey · lore

Re: [PATCH] git-remote: fixed missing .uploadpack usage for show command

Chris Frey <cdfrey@foursquare.net> writes:
> When using 'git remote show <name>', the remote HEAD check
> did not use the uploadpack configuration setting.
>
> Signed-off-by: Chris Frey <cdfrey@foursquare.net>
Thanks.

"X did not use Y" may be a good statement of the fact. From the patch text it can be seen that a NULL used to be passed and the patch makes it to pass states->remote instead, so "This patch make X use Y", even though left unsaid in the message, can be seen.

But it does not answer a more important question. How was it a problem that "X did not use Y"?

People who followed a recent discussion know the answer to this question, but ones who read this in the "git log" output 6 months down the line will not. Please make a habit of justifying the change by stating "why".

"X should have used Y because of such and such reasons, but it didn't. Instead of showing the correct result W, it gave Z, which may happen to be the same as W in default settings but otherwise is wrong."

Chris Frey· Jun 25, 2009, 21:21 UTC · re: Junio C Hamano · lore

For users pulling from machines with self compiled git installs, in non-PATH locations, they can set the config option remote.<name>.uploadpack to set the location of git-upload-pack.

When using 'git remote show <name>', the remote HEAD check did not use the uploadpack configuration setting, and would stall.

In builtin-remote.c, the config setting is already loaded with the call to remote_get(), so this patch passes that remote along to transport_get().

Signed-off-by: Chris Frey <cdfrey@foursquare.net>
---
A possibly clearer description...
 builtin-remote.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to builtin-remote.c +1 −1
diff --git a/builtin-remote.c b/builtin-remote.c
index 658d578..ec1f903 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -787,7 +787,7 @@ static int get_remote_ref_states(const char *name,
 	read_branches();
 
 	if (query) {
-		transport = transport_get(NULL, states->remote->url_nr > 0 ?
+		transport = transport_get(states->remote, states->remote->url_nr > 0 ?
 			states->remote->url[0] : NULL);
 		remote_refs = transport_get_remote_refs(transport);
 		transport_disconnect(transport);

-- 
1.6.2.5
Junio C Hamano· Jun 25, 2009, 21:48 UTC · re: Chris Frey · lore

Re: [PATCH] git-remote: fix missing .uploadpack usage for show command

Chris Frey <cdfrey@foursquare.net> writes:
Show 16 quoted lines
> For users pulling from machines with self compiled git installs,
> in non-PATH locations, they can set the config option
> remote.<name>.uploadpack to set the location of git-upload-pack.
>
> When using 'git remote show <name>', the remote HEAD check
> did not use the uploadpack configuration setting, and would
> not use the configured program.
>
> In builtin-remote.c, the config setting is already loaded
> with the call to remote_get(), so this patch passes that remote
> along to transport_get().
>
> Signed-off-by: Chris Frey <cdfrey@foursquare.net>
> ---
>
> A possibly clearer description...
Thanks, much clearer.  Will queue, aiming to eventually merge to 'maint'.

Do you have tests to protect this fix from getting broken in the future by other people?

← back to recent threads