{"thread":{"id":"41632","subject":"[PATCH] fetch: show reference pointed by new tags","startedAt":"2016-03-06T22:34:54Z","lastAt":"2016-03-13T15:38:02Z","messageCount":5,"participants":["Eric Engestrom","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"280302","messageId":"1457303694-16153-1-git-send-email-eric@engestrom.ch","threadId":"41632","inReplyTo":null,"subject":"[PATCH] fetch: show reference pointed by new tags","fromName":"Eric Engestrom","fromEmail":"eric@engestrom.ch","sentAt":"2016-03-06T22:34:54Z","receivedAt":"2016-03-06T22:34:54Z","isPatch":true,"sender":{"key":"eric@engestrom.ch","avatar":"https://gravatar.com/avatar/b152263ecd779e57ac15f685d524c34e5e7954dc9c625351ab0e62a7195c0820?d=mp&s=160"},"body":"Before, new tags had their names shown twice:\n * [new tag] v4.5-rc6 -> v4.5-rc6\n\nInstead, show the hash of the commit pointed to:\n * [new tag] v4.5-rc6 -> fc77dbd\n\nSigned-off-by: Eric Engestrom \n---\n\nThis is my first dive into git's code, so it's likely I'm not doing things\nright. The first candidate for that is the literal `7`, which should probably\nbe a variable, but I couldn't find what I should use instead.\n\nI'd be happy to fix this for a v2 :]\n\nCheers,\n Eric\n\n\n builtin/fetch.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex e4639d8..93b2145 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -515,6 +515,7 @@ static int update_local_ref(struct ref *ref,\n \t\tif (starts_with(name, \"refs/tags/\")) {\n \t\t\tmsg = \"storing tag\";\n \t\t\twhat = _(\"[new tag]\");\n+\t\t\tpretty_ref = find_unique_abbrev(ref->new_oid.hash, 7);\n \t\t} else if (starts_with(name, \"refs/heads/\")) {\n \t\t\tmsg = \"storing head\";\n \t\t\twhat = _(\"[new branch]\");\n-- \n2.7.2\n"},{"id":"280308","messageId":"xmqq60wzta2s.fsf@gitster.mtv.corp.google.com","threadId":"41632","inReplyTo":"1457303694-16153-1-git-send-email-eric@engestrom.ch","subject":"Re: [PATCH] fetch: show reference pointed by new tags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-07T00:18:03Z","receivedAt":"2016-03-07T00:18:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Engestrom writes:\n\n> Before, new tags had their names shown twice:\n> * [new tag] v4.5-rc6 -> v4.5-rc6\n>\n> Instead, show the hash of the commit pointed to:\n> * [new tag] v4.5-rc6 -> fc77dbd\n>\n> Signed-off-by: Eric Engestrom \n\nThe report from update-local-refs is meant to show what \"local\"\nnames the updated ref has, i.e. what names you can refer the result\nas. If you fetched v4.5-rc6, the output tells you that you can\nrefer to it as v4.5-rc6 in your local repository.\n\nTechnically, you can also refer to the tag as fc77dbd, but I do not\nsee why a user would want to.\n\nIn addition, if I did:\n\n $ git fetch $his_repository 'refs/tags/*:refs/tags/his/*'\n\nI'd see his 'v1.0' tag locally stored as 'his/v1.0', and that is\nwhat is shown in the output as\n\n * [new tag] v1.0 -> his/v1.0\n\nso that I know I can refer to his v1.0 as \"his/v1.0\", to\ndifferenticate from the tag I happen to have created as v1.0 in my\nlocal repository.\n\nSo, why is this change an improvement?\n\n> ---\n>\n> This is my first dive into git's code, so it's likely I'm not doing things\n> right. The first candidate for that is the literal `7`, which should probably\n> be a variable, but I couldn't find what I should use instead.\n>\n> I'd be happy to fix this for a v2 :]\n>\n> Cheers,\n> Eric\n>\n>\n> builtin/fetch.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index e4639d8..93b2145 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -515,6 +515,7 @@ static int update_local_ref(struct ref *ref,\n> \t\tif (starts_with(name, \"refs/tags/\")) {\n> \t\t\tmsg = \"storing tag\";\n> \t\t\twhat = _(\"[new tag]\");\n> +\t\t\tpretty_ref = find_unique_abbrev(ref->new_oid.hash, 7);\n> \t\t} else if (starts_with(name, \"refs/heads/\")) {\n> \t\t\tmsg = \"storing head\";\n> \t\t\twhat = _(\"[new branch]\");\n"},{"id":"280309","messageId":"xmqqwppfrtai.fsf@gitster.mtv.corp.google.com","threadId":"41632","inReplyTo":"xmqq60wzta2s.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] fetch: show reference pointed by new tags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-07T01:05:57Z","receivedAt":"2016-03-07T01:05:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> This is my first dive into git's code, so it's likely I'm not doing things\n>> right. The first candidate for that is the literal `7`,...\n\nActually, the first candidate is not related to any code, but that\nyou did not explain \"why\" in your log message.\n\nI raised two issues with your change in my response, which are (1)\nloss of convenience (i.e. why would a user want to type fc77dbd when\nv4.5-rc6 is easier to remember) and (2) loss of information\n(i.e. renaming fetch loses the assurance from the report that v1.0\ndid indeed copied to his/v1.0). Both of these things tell us that\nthe change only makes the result worse.\n\nBut I am sure you didn't make this change to make the resulting\nsystem worse. That is why I asked \"why is this an improvement?\" and\nthat is _not_ a rhetorical question (i.e. I wasn't making a\nstatement: \"this cannot possibly be an improvement\"). You must have\nthought that the user would benefit in some way if the report showed\na raw abbreviated object name there. But you failed to say what\nexactly that benefit is, and I couldn't guess, and that is why I\nasked.\n\nBecause a raw abbreviated object name alone is a fairly useless\npiece of information (i.e. to get anything meaningful, the user\nneeds to give it to some Git command like \"git log\", \"git show\",\netc.), and the resulting local tag name is equally usable as a raw\nobject name is if the user is going to use these Git commands to\nlearn more about the object anyway, the only semi-sensible\njustification I can think of offhand for this change is that the\nuser somehow already has a list of raw object names for the refs\nexpected to be updated available and showing the raw object name in\nthe update-local report may serve as an assurance that the right\nobject was fetched. Perhaps there was an announcement e-mail\nmessage that said fc77dbd is the object name of the v4.5-rc6 tag,\nand the user can see that object was fetched without having to run\nextra Git command, or something like that.\n\nBut I am merely guessing from the your patch text what the reasoning\nbehind the change was and you are the one who had the original\nreason why you needed this change, so your \"why\" may be a lot more\nuseful use case than the one I made up and called \"semi-sensible\"\nhere. The proposed log message needs to explain your \"why\".\n\nAnd if you explained \"why\", you may have heard other people agreeing\nwith you that this new piece of information is nice to have. They\nmay even have helped you by suggesting to add this extra information\nsomewhere in the output, instead of replacing existing information\nin the output (which would lead to loss of convenience and\ninformation).\n\nAnyway, welcome to Git development community.\n"},{"id":"280350","messageId":"xmqqd1r5mw4p.fsf@gitster.mtv.corp.google.com","threadId":"41632","inReplyTo":"xmqqwppfrtai.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] fetch: show reference pointed by new tags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-08T04:28:06Z","receivedAt":"2016-03-08T04:28:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n> But I am merely guessing from the your patch text what the reasoning\n> behind the change was and you are the one who had the original\n> reason why you needed this change, so your \"why\" may be a lot more\n> useful use case than the one I made up and called \"semi-sensible\"\n> here. The proposed log message needs to explain your \"why\".\n>\n> And if you explained \"why\", you may have heard other people agreeing\n> with you that this new piece of information is nice to have. They\n> may even have helped you by suggesting to add this extra information\n> somewhere in the output, instead of replacing existing information\n> in the output (which would lead to loss of convenience and\n> information).\n\nI just thought of another possible explanation why you may have\nthought that it is a good idea to clobber the right hand side of the\nfetch report. Perhaps you thought that LHS and RHS say the same\nthing and that is redundant?\n\nBecause \"git fetch\" is flexible and allows you to store what the\nremote side called X locally as Y, the fetch report in the most\ngeneral form must say X on the remot side) was fetched and stored as\nY in the local repository, i.e.\n\n\t[new tag] X -> Y\n\nbut it is excusable that people new to Git who never saw such a\nrenaming fetch to misunderstand that we are giving redundant\ninformation.\n\nIf that was the motivation, a possible way to change the behaviour\nwould be to show\n\n\t[new tag] X\n\nif and only if the remote side and the local side uses exactly the\nsae name for the ref. The lack of \" -> \" can clearly tell the user\nthat the output is telling us that what they call X is fetched and\nstored as X (i.e. under the same name) locally. A fetched ref that\ndoes not update any local ref (i.e. the ones that are recorded only\nin the FETCH_HEAD file) is shown as\n\n\ttag X -> FETCH_HEAD\n\nso there is no ambiguity there, either.\n"},{"id":"280684","messageId":"20160313153802.GG30298@engestrom.ch","threadId":"41632","inReplyTo":"xmqqd1r5mw4p.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] fetch: show reference pointed by new tags","fromName":"Eric Engestrom","fromEmail":"eric@engestrom.ch","sentAt":"2016-03-13T15:38:02Z","receivedAt":"2016-03-13T15:38:02Z","isPatch":true,"sender":{"key":"eric@engestrom.ch","avatar":"https://gravatar.com/avatar/b152263ecd779e57ac15f685d524c34e5e7954dc9c625351ab0e62a7195c0820?d=mp&s=160"},"body":"Hi Junio,\n\nThanks for the warm welcome, but after your explanation on the purpose\nof the second field, it looks like my patch was just a plain bad idea.\n\nI'm not that new to git (I've been using it actively for 6+ years), but\nas you guessed, I thought it was just redundant info as I had never seen\na tag with a different remote name (unlike new branches, for which you\nalways see the `remote/branch` name), and I thought I might as well\nreplace it with an other info. As you mentioned though, a tag can be\nused everywhere its hash can, so there's no point showing that either.\n\nI guess I'll just take that as a lesson :]\n “Make sure you actually understand what the code is doing\n before trying to modify it”\n\nCheers,\nEric\n\n\nOn Mon, Mar 07, 2016 at 08:28:06PM -0800, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > Junio C Hamano writes:\n> >\n> > But I am merely guessing from the your patch text what the reasoning\n> > behind the change was and you are the one who had the original\n> > reason why you needed this change, so your \"why\" may be a lot more\n> > useful use case than the one I made up and called \"semi-sensible\"\n> > here. The proposed log message needs to explain your \"why\".\n> >\n> > And if you explained \"why\", you may have heard other people agreeing\n> > with you that this new piece of information is nice to have. They\n> > may even have helped you by suggesting to add this extra information\n> > somewhere in the output, instead of replacing existing information\n> > in the output (which would lead to loss of convenience and\n> > information).\n> \n> I just thought of another possible explanation why you may have\n> thought that it is a good idea to clobber the right hand side of the\n> fetch report. Perhaps you thought that LHS and RHS say the same\n> thing and that is redundant?\n> \n> Because \"git fetch\" is flexible and allows you to store what the\n> remote side called X locally as Y, the fetch report in the most\n> general form must say X on the remot side) was fetched and stored as\n> Y in the local repository, i.e.\n> \n> \t[new tag] X -> Y\n> \n> but it is excusable that people new to Git who never saw such a\n> renaming fetch to misunderstand that we are giving redundant\n> information.\n> \n> If that was the motivation, a possible way to change the behaviour\n> would be to show\n> \n> \t[new tag] X\n> \n> if and only if the remote side and the local side uses exactly the\n> sae name for the ref. The lack of \" -> \" can clearly tell the user\n> that the output is telling us that what they call X is fetched and\n> stored as X (i.e. under the same name) locally. A fetched ref that\n> does not update any local ref (i.e. the ones that are recorded only\n> in the FETCH_HEAD file) is shown as\n> \n> \ttag X -> FETCH_HEAD\n> \n> so there is no ambiguity there, either.\n> \n> \n> \n"}]}