[PATCH] fetch: Print full url in header

[PATCH] fetch: Print full url in header

[Tom Miller]

Do not remove "/" and ".git" from the end of the header url when
fetching. This affects the output of "fetch" and "fetch --prune"
making the header url more consistent with "remote --verbose".

Add tests to verify that "fetch" and "fetch --prune" do not strip the
trailing characters from the header url.

Output before this patch:

    $ git fetch remote-with-dot-git-and-slash
    From https://github.com/git/git
       a155a5f..5512ac5  maint      -> upstream/maint

Output after this patch:

    $ git fetch remote-with-dot-git-and-slash
    From https://github.com/git/git.git/
       a155a5f..5512ac5  maint      -> upstream/maint

Signed-off-by: Tom Miller <jackerran@gmail.com>
---

This patch should be based on the tip of "next" because it is dependent
on the code from "tm/fetch-prune".

Initially I thought I would stop anonymizing the header url as well.
Then I ran across this commit.

	> commit 47abd85ba06ed7209d1caa3e5ac7cc6b232bece4
	> Author: Andreas Ericsson <ae@op5.se>
	> Date:   Fri Apr 17 10:20:11 2009 +0200
	>
	>     fetch: Strip usernames from url's before storing them
	>
	>     When pulling from a remote, the full URL including username
	>     is by default added to the commit message. Since it adds
	>     very little value but could be used by malicious people to
	>     glean valid usernames (with matching hostnames), we're far
	>     better off just stripping the username before storing the
	>     remote URL locally.
	>
	>     Note that this patch has no lasting visible effect when
	>     "git pull" does not create a merge commit. It simply
	>     alters what gets written to .git/FETCH_HEAD, which is used
	>     by "git merge" to automagically create its messages.
	>
	>     Signed-off-by: Andreas Ericsson <ae@op5.se>
	>     Signed-off-by: Junio C Hamano <gitster@pobox.com>

After reading this and trying different things with the code. I believe
it would make sense to continue to anonymize the url for output to the
terminal. I found if someone is using HTTP basic authentication and has
the username and password in the url. Then, one could unexpectedly
compromise their credentials during a fetch. I do not believe that
anyone should store their credentials in this way, but it is better safe
than sorry.

I also chose to continue to strip the trailing characters for the
FETCH_HEAD file.  I wanted the input of the mailing list to see if we
should also stop striping the trailing characters off of the url written
to FETCH_HEAD? If so, I'll do it as a seperate patch.

 builtin/fetch.c  | 15 +++------------
 t/t5510-fetch.sh | 29 ++++++++++++++++++++++++-----
 2 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 025bc3e..01df749 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -664,8 +664,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 					    *what ? what : "HEAD");
 			if (note.len) {
 				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)
@@ -723,7 +722,7 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)
 static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map,
 		const char *raw_url)
 {
-	int url_len, i, result = 0;
+	int result = 0;
 	struct ref *ref, *stale_refs = get_stale_heads(refs, ref_count, ref_map);
 	char *url;
 	const char *dangling_msg = dry_run
@@ -735,19 +734,11 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map,
 	else
 		url = xstrdup("foreign");
 
-	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;
-
 	for (ref = stale_refs; ref; ref = ref->next) {
 		if (!dry_run)
 			result |= delete_ref(ref->name, NULL, 0);
 		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) {
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 12674ac..882bfa1 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -614,15 +614,34 @@ test_expect_success 'all boundary commits are excluded' '
 	test_bundle_object_count .git/objects/pack/pack-${pack##pack	}.pack 3
 '
 
-test_expect_success 'fetch --prune prints the remotes url' '
+test_expect_success 'fetch prints the remotes full url' '
+	git clone . fetch-url-repo.git &&
+	git clone --mirror fetch-url-repo.git/ fetch-full-url &&
+	(
+		cd fetch-url-repo.git &&
+		git branch hello
+	) &&
+	(
+		cd fetch-full-url &&
+		git fetch origin 2>&1 | head -n1 >../actual
+	) &&
+	echo "From ${D}/fetch-url-repo.git/" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'fetch --prune prints the remotes full url' '
 	git branch goodbye &&
-	git clone . only-prunes &&
-	git branch -D goodbye &&
+	git clone --mirror . fetch-prune-url-repo.git &&
+	git clone fetch-prune-url-repo.git/ fetch-prune-full-url &&
+	(
+		cd fetch-prune-url-repo.git &&
+		git branch -D goodbye
+	) &&
 	(
-		cd only-prunes &&
+		cd fetch-prune-full-url &&
 		git fetch --prune origin 2>&1 | head -n1 >../actual
 	) &&
-	echo "From ${D}/." >expect &&
+	echo "From ${D}/fetch-prune-url-repo.git/" >expect &&
 	test_cmp expect actual
 '
 
-- 
1.8.5.2.374.ga7e4b90

Re: [PATCH] fetch: Print full url in header

[Junio C Hamano]

Tom Miller <jackerran@gmail.com> writes:

> After reading this and trying different things with the code. I believe
> it would make sense to continue to anonymize the url for output to the
> terminal.

Yes.  That is what the "anonymize" bit is all about.

> I also chose to continue to strip the trailing characters for the
> FETCH_HEAD file.  I wanted the input of the mailing list to see if we
> should also stop striping the trailing characters off of the url written
> to FETCH_HEAD? If so, I'll do it as a seperate patch.

These strings are used to come up with the log subject line for
merges, and there is a value in keeping them as short as possible by
removing unnecessary bits.

I wouldn't mind, and actually I suspect that it is more preferrable,
to make the consistency go the other way, that is ...

> Do not remove "/" and ".git" from the end of the header url when
> fetching. This affects the output of "fetch" and "fetch --prune"
> making the header url more consistent with "remote --verbose".

... to make "remote --verbose" abbreviate to match what you see from
"fetch".

Having said all that, the difference between the full URL shown by
"remote --verbose" (which is used to interact with the remote in
this repository) and the abbreviated URL (which is shown by "fetch"
and is designed to be sharable with others with a simple cut&paste)
matters only when there are a pair of ambiguously configured
repositories (e.g. there are two repositories "git://host/a.git/"
and "git://host/a/.git") that serve different things and you are
debugging the situation.  And to me, "remote --verbose" looks more
or less a debugging aid, nothing more.  So another alternative that
may be to leave everything as-is.

Thanks.

Re: [PATCH] fetch: Print full url in header

[Tom Miller]

On Thu, Jan 09, 2014 at 12:07:38PM -0800, Junio C Hamano wrote:
> 
> Having said all that, the difference between the full URL shown by
> "remote --verbose" (which is used to interact with the remote in
> this repository) and the abbreviated URL (which is shown by "fetch"
> and is designed to be sharable with others with a simple cut&paste)
> matters only when there are a pair of ambiguously configured
> repositories (e.g. there are two repositories "git://host/a.git/"
> and "git://host/a/.git") that serve different things and you are
> debugging the situation.  And to me, "remote --verbose" looks more
> or less a debugging aid, nothing more.  So another alternative that
> may be to leave everything as-is.
> 
> Thanks.

I like the alterantive option of "leave everything as-is", especially
after the arguments you've presented. There is still the problem of the
logic that has been duplicated. I think it should be put in a function,
but if you are ok with leaving it duplicated that is fine by me.

	if (raw_url)
		url = transport_anonymize_url(raw_url);
	else
		url = xstrdup("foreign");

	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;

Thanks,
Tom Miller