{"thread":{"id":"35628","subject":"[PATCH] fetch: Print full url in header","startedAt":"2014-01-09T02:39:18Z","lastAt":"2014-01-10T03:03:54Z","messageCount":3,"participants":["Tom Miller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"232940","messageId":"1389235158-21902-1-git-send-email-jackerran@gmail.com","threadId":"35628","inReplyTo":null,"subject":"[PATCH] fetch: Print full url in header","fromName":"Tom Miller","fromEmail":"jackerran@gmail.com","sentAt":"2014-01-09T02:39:18Z","receivedAt":"2014-01-09T02:39:18Z","isPatch":true,"sender":{"key":"jackerran@gmail.com","avatar":null},"body":"Do not remove \"/\" and \".git\" from the end of the header url when\nfetching. This affects the output of \"fetch\" and \"fetch --prune\"\nmaking the header url more consistent with \"remote --verbose\".\n\nAdd tests to verify that \"fetch\" and \"fetch --prune\" do not strip the\ntrailing characters from the header url.\n\nOutput before this patch:\n\n    $ git fetch remote-with-dot-git-and-slash\n    From https://github.com/git/git\n       a155a5f..5512ac5  maint      -> upstream/maint\n\nOutput after this patch:\n\n    $ git fetch remote-with-dot-git-and-slash\n    From https://github.com/git/git.git/\n       a155a5f..5512ac5  maint      -> upstream/maint\n\nSigned-off-by: Tom Miller <jackerran@gmail.com>\n---\n\nThis patch should be based on the tip of \"next\" because it is dependent\non the code from \"tm/fetch-prune\".\n\nInitially I thought I would stop anonymizing the header url as well.\nThen I ran across this commit.\n\n\t> commit 47abd85ba06ed7209d1caa3e5ac7cc6b232bece4\n\t> Author: Andreas Ericsson <ae@op5.se>\n\t> Date:   Fri Apr 17 10:20:11 2009 +0200\n\t>\n\t>     fetch: Strip usernames from url's before storing them\n\t>\n\t>     When pulling from a remote, the full URL including username\n\t>     is by default added to the commit message. Since it adds\n\t>     very little value but could be used by malicious people to\n\t>     glean valid usernames (with matching hostnames), we're far\n\t>     better off just stripping the username before storing the\n\t>     remote URL locally.\n\t>\n\t>     Note that this patch has no lasting visible effect when\n\t>     \"git pull\" does not create a merge commit. It simply\n\t>     alters what gets written to .git/FETCH_HEAD, which is used\n\t>     by \"git merge\" to automagically create its messages.\n\t>\n\t>     Signed-off-by: Andreas Ericsson <ae@op5.se>\n\t>     Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\nAfter reading this and trying different things with the code. I believe\nit would make sense to continue to anonymize the url for output to the\nterminal. I found if someone is using HTTP basic authentication and has\nthe username and password in the url. Then, one could unexpectedly\ncompromise their credentials during a fetch. I do not believe that\nanyone should store their credentials in this way, but it is better safe\nthan sorry.\n\nI also chose to continue to strip the trailing characters for the\nFETCH_HEAD file.  I wanted the input of the mailing list to see if we\nshould also stop striping the trailing characters off of the url written\nto FETCH_HEAD? If so, I'll do it as a seperate patch.\n\n builtin/fetch.c  | 15 +++------------\n t/t5510-fetch.sh | 29 ++++++++++++++++++++++++-----\n 2 files changed, 27 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 025bc3e..01df749 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -664,8 +664,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\t\t    *what ? what : \"HEAD\");\n \t\t\tif (note.len) {\n \t\t\t\tif (verbosity >= 0 && !shown_url) {\n-\t\t\t\t\tfprintf(stderr, _(\"From %.*s\\n\"),\n-\t\t\t\t\t\t\turl_len, url);\n+\t\t\t\t\tfprintf(stderr, _(\"From %s\\n\"), url);\n \t\t\t\t\tshown_url = 1;\n \t\t\t\t}\n \t\t\t\tif (verbosity >= 0)\n@@ -723,7 +722,7 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)\n static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map,\n \t\tconst char *raw_url)\n {\n-\tint url_len, i, result = 0;\n+\tint result = 0;\n \tstruct ref *ref, *stale_refs = get_stale_heads(refs, ref_count, ref_map);\n \tchar *url;\n \tconst char *dangling_msg = dry_run\n@@ -735,19 +734,11 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map,\n \telse\n \t\turl = xstrdup(\"foreign\");\n \n-\turl_len = strlen(url);\n-\tfor (i = url_len - 1; url[i] == '/' && 0 <= i; i--)\n-\t\t;\n-\n-\turl_len = i + 1;\n-\tif (4 < i && !strncmp(\".git\", url + i - 3, 4))\n-\t\turl_len = i - 3;\n-\n \tfor (ref = stale_refs; ref; ref = ref->next) {\n \t\tif (!dry_run)\n \t\t\tresult |= delete_ref(ref->name, NULL, 0);\n \t\tif (verbosity >= 0 && !shown_url) {\n-\t\t\tfprintf(stderr, _(\"From %.*s\\n\"), url_len, url);\n+\t\t\tfprintf(stderr, _(\"From %s\\n\"), url);\n \t\t\tshown_url = 1;\n \t\t}\n \t\tif (verbosity >= 0) {\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 12674ac..882bfa1 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -614,15 +614,34 @@ test_expect_success 'all boundary commits are excluded' '\n \ttest_bundle_object_count .git/objects/pack/pack-${pack##pack\t}.pack 3\n '\n \n-test_expect_success 'fetch --prune prints the remotes url' '\n+test_expect_success 'fetch prints the remotes full url' '\n+\tgit clone . fetch-url-repo.git &&\n+\tgit clone --mirror fetch-url-repo.git/ fetch-full-url &&\n+\t(\n+\t\tcd fetch-url-repo.git &&\n+\t\tgit branch hello\n+\t) &&\n+\t(\n+\t\tcd fetch-full-url &&\n+\t\tgit fetch origin 2>&1 | head -n1 >../actual\n+\t) &&\n+\techo \"From ${D}/fetch-url-repo.git/\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'fetch --prune prints the remotes full url' '\n \tgit branch goodbye &&\n-\tgit clone . only-prunes &&\n-\tgit branch -D goodbye &&\n+\tgit clone --mirror . fetch-prune-url-repo.git &&\n+\tgit clone fetch-prune-url-repo.git/ fetch-prune-full-url &&\n+\t(\n+\t\tcd fetch-prune-url-repo.git &&\n+\t\tgit branch -D goodbye\n+\t) &&\n \t(\n-\t\tcd only-prunes &&\n+\t\tcd fetch-prune-full-url &&\n \t\tgit fetch --prune origin 2>&1 | head -n1 >../actual\n \t) &&\n-\techo \"From ${D}/.\" >expect &&\n+\techo \"From ${D}/fetch-prune-url-repo.git/\" >expect &&\n \ttest_cmp expect actual\n '\n \n-- \n1.8.5.2.374.ga7e4b90\n"},{"id":"232964","messageId":"xmqqwqi8aph1.fsf@gitster.dls.corp.google.com","threadId":"35628","inReplyTo":"1389235158-21902-1-git-send-email-jackerran@gmail.com","subject":"Re: [PATCH] fetch: Print full url in header","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-09T20:07:38Z","receivedAt":"2014-01-09T20:07:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tom Miller <jackerran@gmail.com> writes:\n\n> After reading this and trying different things with the code. I believe\n> it would make sense to continue to anonymize the url for output to the\n> terminal.\n\nYes.  That is what the \"anonymize\" bit is all about.\n\n> I also chose to continue to strip the trailing characters for the\n> FETCH_HEAD file.  I wanted the input of the mailing list to see if we\n> should also stop striping the trailing characters off of the url written\n> to FETCH_HEAD? If so, I'll do it as a seperate patch.\n\nThese strings are used to come up with the log subject line for\nmerges, and there is a value in keeping them as short as possible by\nremoving unnecessary bits.\n\nI wouldn't mind, and actually I suspect that it is more preferrable,\nto make the consistency go the other way, that is ...\n\n> Do not remove \"/\" and \".git\" from the end of the header url when\n> fetching. This affects the output of \"fetch\" and \"fetch --prune\"\n> making the header url more consistent with \"remote --verbose\".\n\n... to make \"remote --verbose\" abbreviate to match what you see from\n\"fetch\".\n\nHaving said all that, the difference between the full URL shown by\n\"remote --verbose\" (which is used to interact with the remote in\nthis repository) and the abbreviated URL (which is shown by \"fetch\"\nand is designed to be sharable with others with a simple cut&paste)\nmatters only when there are a pair of ambiguously configured\nrepositories (e.g. there are two repositories \"git://host/a.git/\"\nand \"git://host/a/.git\") that serve different things and you are\ndebugging the situation.  And to me, \"remote --verbose\" looks more\nor less a debugging aid, nothing more.  So another alternative that\nmay be to leave everything as-is.\n\nThanks.\n"},{"id":"232988","messageId":"20140110030354.GA19887@gmail.com","threadId":"35628","inReplyTo":"xmqqwqi8aph1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] fetch: Print full url in header","fromName":"Tom Miller","fromEmail":"jackerran@gmail.com","sentAt":"2014-01-10T03:03:54Z","receivedAt":"2014-01-10T03:03:54Z","isPatch":true,"sender":{"key":"jackerran@gmail.com","avatar":null},"body":"On Thu, Jan 09, 2014 at 12:07:38PM -0800, Junio C Hamano wrote:\n> \n> Having said all that, the difference between the full URL shown by\n> \"remote --verbose\" (which is used to interact with the remote in\n> this repository) and the abbreviated URL (which is shown by \"fetch\"\n> and is designed to be sharable with others with a simple cut&paste)\n> matters only when there are a pair of ambiguously configured\n> repositories (e.g. there are two repositories \"git://host/a.git/\"\n> and \"git://host/a/.git\") that serve different things and you are\n> debugging the situation.  And to me, \"remote --verbose\" looks more\n> or less a debugging aid, nothing more.  So another alternative that\n> may be to leave everything as-is.\n> \n> Thanks.\n\nI like the alterantive option of \"leave everything as-is\", especially\nafter the arguments you've presented. There is still the problem of the\nlogic that has been duplicated. I think it should be put in a function,\nbut if you are ok with leaving it duplicated that is fine by me.\n\n\tif (raw_url)\n\t\turl = transport_anonymize_url(raw_url);\n\telse\n\t\turl = xstrdup(\"foreign\");\n\n\turl_len = strlen(url);\n\tfor (i = url_len - 1; url[i] == '/' && 0 <= i; i--)\n\t\t;\n\turl_len = i + 1;\n\tif (4 < i && !strncmp(\".git\", url + i - 3, 4))\n\t\turl_len = i - 3;\n\nThanks,\nTom Miller\n"}]}