{"thread":{"id":"15393","subject":"[PATCH] builtin-symbolic-ref: comment on the use of \"resolve_ref\" with reading == 0","startedAt":"2008-09-06T07:55:43Z","lastAt":"2008-09-08T00:33:26Z","messageCount":4,"participants":["Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"89896","messageId":"20080906095543.c627b692.chriscool@tuxfamily.org","threadId":"15393","inReplyTo":null,"subject":"[PATCH] builtin-symbolic-ref: comment on the use of \"resolve_ref\" with reading == 0","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-09-06T07:55:43Z","receivedAt":"2008-09-06T07:55:43Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"The following thread:\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/42469\n\n(the message ID from the first message is:\n\n20070318020645.2444.75365.julian@quantumfyre.co.uk)\n\nexplains why \"resolve_ref\" is used with a \"reading\" parameter set to 0\ninstead of 1, but there was no comment saying that near the code.\n\nSigned-off-by: Christian Couder \n---\n builtin-symbolic-ref.c | 10 ++++++++++\n 1 files changed, 10 insertions(+), 0 deletions(-)\n\n\tThis comment may save other people some time.\n\ndiff --git a/builtin-symbolic-ref.c b/builtin-symbolic-ref.c\nindex bfc78bb..9490c47 100644\n--- a/builtin-symbolic-ref.c\n+++ b/builtin-symbolic-ref.c\n@@ -12,6 +12,16 @@ static void check_symref(const char *HEAD, int quiet)\n {\n \tunsigned char sha1[20];\n \tint flag;\n+\n+\t/*\n+\t * It doesn't seem logical to use \"resolve_ref\" with reading == 0\n+\t * as we are just checking if a ref exists, but some code depends\n+\t * on the following to work:\n+\t *\n+\t * $ git init-db\n+\t * $ git symbolic-ref HEAD\n+\t * refs/heads/master\n+\t */\n \tconst char *refs_heads_master = resolve_ref(HEAD, sha1, 0, &flag);\n \n \tif (!refs_heads_master)\n-- \n1.6.0.1.338.g5e95.dirty\n"},{"id":"89899","messageId":"7vwshpsi1g.fsf@gitster.siamese.dyndns.org","threadId":"15393","inReplyTo":"20080906095543.c627b692.chriscool@tuxfamily.org","subject":"Re: [PATCH] builtin-symbolic-ref: comment on the use of \"resolve_ref\" with reading == 0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-09-06T10:08:43Z","receivedAt":"2008-09-06T10:08:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> diff --git a/builtin-symbolic-ref.c b/builtin-symbolic-ref.c\n> index bfc78bb..9490c47 100644\n> --- a/builtin-symbolic-ref.c\n> +++ b/builtin-symbolic-ref.c\n> @@ -12,6 +12,16 @@ static void check_symref(const char *HEAD, int quiet)\n> ...\n> +\t/*\n> +\t * It doesn't seem logical to use \"resolve_ref\" with reading == 0\n> +\t * as we are just checking if a ref exists,...\n> ...\n> +\t */\n\nI have to say that this comment is confused.\n\nWhen you have a full ref (as opposed to an abbreviated one that you might\ngive to dwim_ref()), you can use it for two kinds of things:\n\n (1) You can use it to find out what _object_ the ref points at. This is\n \"reading\" the ref, and the ref, if it is not symbolic, has to exist,\n and if it is symbolic, it has to point at an existing ref, because\n the \"read\" goes through the symref to the ref it points at.\n\n (2) Anything else. This could be a prelude to \"writing\" to the ref, in\n which case a write to a symref that points at yet-to-be-born ref will\n create the real ref pointed by the symref, so such a symref is not an\n error. It has to answer \"here is the real ref you should write into\"\n (or, \"we will write into\").\n\n But the access that is not \"reading\" does not have to be \"writing\";\n it can be merely checking _where it leads to_. And check_symref()\n uses this call for exactly that purpose. This access is not\n \"checking if a ref exists\".\n\nSo reading==0 is the logical thing to do here.\n"},{"id":"89902","messageId":"200809061403.14592.chriscool@tuxfamily.org","threadId":"15393","inReplyTo":"7vwshpsi1g.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] builtin-symbolic-ref: comment on the use of \"resolve_ref\" with reading == 0","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-09-06T12:03:13Z","receivedAt":"2008-09-06T12:03:13Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le samedi 6 septembre 2008, Junio C Hamano a écrit :\n> Christian Couder writes:\n> > diff --git a/builtin-symbolic-ref.c b/builtin-symbolic-ref.c\n> > index bfc78bb..9490c47 100644\n> > --- a/builtin-symbolic-ref.c\n> > +++ b/builtin-symbolic-ref.c\n> > @@ -12,6 +12,16 @@ static void check_symref(const char *HEAD, int\n> > quiet) ...\n> > +\t/*\n> > +\t * It doesn't seem logical to use \"resolve_ref\" with reading == 0\n> > +\t * as we are just checking if a ref exists,...\n> > ...\n> > +\t */\n>\n> I have to say that this comment is confused.\n>\n> When you have a full ref (as opposed to an abbreviated one that you might\n> give to dwim_ref()), you can use it for two kinds of things:\n>\n> (1) You can use it to find out what _object_ the ref points at. This is\n> \"reading\" the ref, and the ref, if it is not symbolic, has to exist,\n> and if it is symbolic, it has to point at an existing ref, because\n> the \"read\" goes through the symref to the ref it points at.\n\nThen the parameter should perhaps be \ncalled \"get_object\", \"get_target\", \"full_dereference\" or something like \nthat instead of \"reading\".\n\n> (2) Anything else. This could be a prelude to \"writing\" to the ref, in\n> which case a write to a symref that points at yet-to-be-born ref\n> will create the real ref pointed by the symref, so such a symref is\n> not an error. It has to answer \"here is the real ref you should write\n> into\" (or, \"we will write into\").\n>\n> But the access that is not \"reading\" does not have to be \"writing\";\n> it can be merely checking _where it leads to_. And check_symref()\n> uses this call for exactly that purpose. This access is not\n> \"checking if a ref exists\".\n\nIn \"resolve_ref\" in refs.c there is the following comment:\n\n\t\t/* Special case: non-existing file.\n\t\t * Not having the refs/heads/new-branch is OK\n\t\t * if we are writing into it, so is .git/HEAD\n\t\t * that points at refs/heads/master still to be\n\t\t * born. It is NOT OK if we are resolving for\n\t\t * reading.\n\t\t */\n\nthat seems to mean that we are either \"writing\" or \"reading\".\n\n> So reading==0 is the logical thing to do here.\n\nIt seems logical after your explanations, yes, and thank you for them, but I \ndon't think it is logical when reading the existing source code or \ncomments.\n\nRegards,\nChristian.\n"},{"id":"90031","messageId":"7v63p7jx2h.fsf@gitster.siamese.dyndns.org","threadId":"15393","inReplyTo":"200809061403.14592.chriscool@tuxfamily.org","subject":"Re: [PATCH] builtin-symbolic-ref: comment on the use of \"resolve_ref\" with reading == 0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-09-08T00:33:26Z","receivedAt":"2008-09-08T00:33:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Le samedi 6 septembre 2008, Junio C Hamano a écrit :\n>> Christian Couder writes:\n>> ...\n>> > +\t * It doesn't seem logical to use \"resolve_ref\" with reading == 0\n>> > +\t * as we are just checking if a ref exists,...\n>> > ...\n>> > +\t */\n>>\n>> I have to say that this comment is confused.\n>>\n>> When you have a full ref (as opposed to an abbreviated one that you might\n>> give to dwim_ref()), you can use it for two kinds of things:\n>>\n>> (1) You can use it to find out what _object_ the ref points at. This is\n>> \"reading\" the ref, and the ref, if it is not symbolic, has to exist,\n>> and if it is symbolic, it has to point at an existing ref, because\n>> the \"read\" goes through the symref to the ref it points at.\n>\n> Then the parameter should perhaps be \n> called \"get_object\", \"get_target\", \"full_dereference\" or something like \n> that instead of \"reading\".\n\nAnother way to think about this is the difference is between:\n\n\topen(2)+read(2)+close(2)\n readlink(2)\n\n> In \"resolve_ref\" in refs.c there is the following comment:\n>\n> \t\t/* Special case: non-existing file.\n> \t\t * Not having the refs/heads/new-branch is OK\n> \t\t * if we are writing into it, so is .git/HEAD\n> \t\t * that points at refs/heads/master still to be\n> \t\t * born. It is NOT OK if we are resolving for\n> \t\t * reading.\n> \t\t */\n>\n> that seems to mean that we are either \"writing\" or \"reading\".\n\nI never said the current comments are perfect.\n\nYour patch was about adding comments to help later developers. If you\nthink this \"if we are writing into it\" is wrong because implies \"it is Ok\nto be missing only when we are writing into it\", I would very much agree\nwith _that_ observation.\n\nBut then, _this_ comment is what needs to be clarified.\n\nInstead, your patch adds an incorrect observation in a single caller; that\nis hardly an improvement. The observation being incorrect would not help\nlater people, and one caller being commented would not help as much as the\ncallee getting correct comments.\n\nSo how about improving the comment that is misleading?\n\nI am not sure what the right re-wording for \"reading\" would be. It is\nsimilar to \"*_gently()\" interface, but it is different. You could call it\n\"must_exist\", but I am not sure if that is much an improvement either.\n\n refs.c | 15 +++++++++------\n 1 files changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git i/refs.c w/refs.c\nindex 39a3b23..a712077 100644\n--- i/refs.c\n+++ w/refs.c\n@@ -409,12 +409,15 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \t\tif (--depth < 0)\n \t\t\treturn NULL;\n \n-\t\t/* Special case: non-existing file.\n-\t\t * Not having the refs/heads/new-branch is OK\n-\t\t * if we are writing into it, so is .git/HEAD\n-\t\t * that points at refs/heads/master still to be\n-\t\t * born. It is NOT OK if we are resolving for\n-\t\t * reading.\n+\t\t/*\n+\t\t * Special case: non-existing file.\n+\t\t * Not having the refs/heads/new-branch is not OK\n+\t\t * we are resolving for reading. But not everybody\n+\t\t * calls this function to learn what object the ref\n+\t\t * points at. E.g. it can be called to learn what the\n+\t\t * symref points at. Also if we are writing into it,\n+\t\t * it is Ok for .git/HEAD to point at refs/heads/master\n+\t\t * that does not exist yet.\n \t\t */\n \t\tif (lstat(path, &st) < 0) {\n \t\t\tstruct ref_list *list = get_packed_refs();\n"}]}