# [PATCH] fetch: Print full url in header 3 messages from 2014-01-09 to 2014-01-10. Participants: Tom Miller, Junio C Hamano. Thread: https://gitlist.dev/t/35628 ## Tom Miller, 2014-01-09 02:39 Subject: [PATCH] fetch: Print full url in header Message-ID: <1389235158-21902-1-git-send-email-jackerran@gmail.com> URL: https://gitlist.dev/e/1389235158-21902-1-git-send-email-jackerran%40gmail.com ``` 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 --- 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 > 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 > Signed-off-by: Junio C Hamano 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 ``` ## Junio C Hamano, 2014-01-09 20:07 Subject: Re: [PATCH] fetch: Print full url in header Message-ID: URL: https://gitlist.dev/e/xmqqwqi8aph1.fsf%40gitster.dls.corp.google.com In-Reply-To: <1389235158-21902-1-git-send-email-jackerran@gmail.com> ``` Tom Miller 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. ``` ## Tom Miller, 2014-01-10 03:03 Subject: Re: [PATCH] fetch: Print full url in header Message-ID: <20140110030354.GA19887@gmail.com> URL: https://gitlist.dev/e/20140110030354.GA19887%40gmail.com In-Reply-To: ``` 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 ```