[PATCH] builtin-symbolic-ref: comment on the use of "resolve_ref" with reading == 0

[PATCH] builtin-symbolic-ref: comment on the use of "resolve_ref" with reading == 0

[Christian Couder]

The following thread:

http://thread.gmane.org/gmane.comp.version-control.git/42469

(the message ID from the first message is:

20070318020645.2444.75365.julian@quantumfyre.co.uk)

explains why "resolve_ref" is used with a "reading" parameter set to 0
instead of 1, but there was no comment saying that near the code.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin-symbolic-ref.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

	This comment may save other people some time.

diff --git a/builtin-symbolic-ref.c b/builtin-symbolic-ref.c
index bfc78bb..9490c47 100644
--- a/builtin-symbolic-ref.c
+++ b/builtin-symbolic-ref.c
@@ -12,6 +12,16 @@ static void check_symref(const char *HEAD, int quiet)
 {
 	unsigned char sha1[20];
 	int flag;
+
+	/*
+	 * It doesn't seem logical to use "resolve_ref" with reading == 0
+	 * as we are just checking if a ref exists, but some code depends
+	 * on the following to work:
+	 *
+	 *    $ git init-db
+	 *    $ git symbolic-ref HEAD
+	 *    refs/heads/master
+	 */
 	const char *refs_heads_master = resolve_ref(HEAD, sha1, 0, &flag);
 
 	if (!refs_heads_master)
-- 
1.6.0.1.338.g5e95.dirty

Re: [PATCH] builtin-symbolic-ref: comment on the use of "resolve_ref" with reading == 0

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> diff --git a/builtin-symbolic-ref.c b/builtin-symbolic-ref.c
> index bfc78bb..9490c47 100644
> --- a/builtin-symbolic-ref.c
> +++ b/builtin-symbolic-ref.c
> @@ -12,6 +12,16 @@ static void check_symref(const char *HEAD, int quiet)
> ...
> +	/*
> +	 * It doesn't seem logical to use "resolve_ref" with reading == 0
> +	 * as we are just checking if a ref exists,...
> ...
> +	 */

I have to say that this comment is confused.

When you have a full ref (as opposed to an abbreviated one that you might
give to dwim_ref()), you can use it for two kinds of things:

 (1) You can use it to find out what _object_ the ref points at.  This is
     "reading" the ref, and the ref, if it is not symbolic, has to exist,
     and if it is symbolic, it has to point at an existing ref, because
     the "read" goes through the symref to the ref it points at.

 (2) Anything else.  This could be a prelude to "writing" to the ref, in
     which case a write to a symref that points at yet-to-be-born ref will
     create the real ref pointed by the symref, so such a symref is not an
     error.  It has to answer "here is the real ref you should write into"
     (or, "we will write into").

     But the access that is not "reading" does not have to be "writing";
     it can be merely checking _where it leads to_.  And check_symref()
     uses this call for exactly that purpose.  This access is not
     "checking if a ref exists".

So reading==0 is the logical thing to do here.

Re: [PATCH] builtin-symbolic-ref: comment on the use of "resolve_ref" with reading == 0

[Christian Couder]

Le samedi 6 septembre 2008, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> > diff --git a/builtin-symbolic-ref.c b/builtin-symbolic-ref.c
> > index bfc78bb..9490c47 100644
> > --- a/builtin-symbolic-ref.c
> > +++ b/builtin-symbolic-ref.c
> > @@ -12,6 +12,16 @@ static void check_symref(const char *HEAD, int
> > quiet) ...
> > +	/*
> > +	 * It doesn't seem logical to use "resolve_ref" with reading == 0
> > +	 * as we are just checking if a ref exists,...
> > ...
> > +	 */
>
> I have to say that this comment is confused.
>
> When you have a full ref (as opposed to an abbreviated one that you might
> give to dwim_ref()), you can use it for two kinds of things:
>
>  (1) You can use it to find out what _object_ the ref points at.  This is
>      "reading" the ref, and the ref, if it is not symbolic, has to exist,
>      and if it is symbolic, it has to point at an existing ref, because
>      the "read" goes through the symref to the ref it points at.

Then the parameter should perhaps be 
called "get_object", "get_target", "full_dereference" or something like 
that instead of "reading".

>  (2) Anything else.  This could be a prelude to "writing" to the ref, in
>      which case a write to a symref that points at yet-to-be-born ref
>     will create the real ref pointed by the symref, so such a symref is
>     not an error.  It has to answer "here is the real ref you should write
>     into" (or, "we will write into").
>
>      But the access that is not "reading" does not have to be "writing";
>      it can be merely checking _where it leads to_.  And check_symref()
>      uses this call for exactly that purpose.  This access is not
>      "checking if a ref exists".

In "resolve_ref" in refs.c there is the following comment:

		/* Special case: non-existing file.
		 * Not having the refs/heads/new-branch is OK
		 * if we are writing into it, so is .git/HEAD
		 * that points at refs/heads/master still to be
		 * born.  It is NOT OK if we are resolving for
		 * reading.
		 */

that seems to mean that we are either "writing" or "reading".

> So reading==0 is the logical thing to do here.

It seems logical after your explanations, yes, and thank you for them, but I 
don't think it is logical when reading the existing source code or 
comments.

Regards,
Christian.

Re: [PATCH] builtin-symbolic-ref: comment on the use of "resolve_ref" with reading == 0

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> Le samedi 6 septembre 2008, Junio C Hamano a écrit :
>> Christian Couder <chriscool@tuxfamily.org> writes:
>> ...
>> > +	 * It doesn't seem logical to use "resolve_ref" with reading == 0
>> > +	 * as we are just checking if a ref exists,...
>> > ...
>> > +	 */
>>
>> I have to say that this comment is confused.
>>
>> When you have a full ref (as opposed to an abbreviated one that you might
>> give to dwim_ref()), you can use it for two kinds of things:
>>
>>  (1) You can use it to find out what _object_ the ref points at.  This is
>>      "reading" the ref, and the ref, if it is not symbolic, has to exist,
>>      and if it is symbolic, it has to point at an existing ref, because
>>      the "read" goes through the symref to the ref it points at.
>
> Then the parameter should perhaps be 
> called "get_object", "get_target", "full_dereference" or something like 
> that instead of "reading".

Another way to think about this is the difference is between:

	open(2)+read(2)+close(2)
        readlink(2)

> In "resolve_ref" in refs.c there is the following comment:
>
> 		/* Special case: non-existing file.
> 		 * Not having the refs/heads/new-branch is OK
> 		 * if we are writing into it, so is .git/HEAD
> 		 * that points at refs/heads/master still to be
> 		 * born.  It is NOT OK if we are resolving for
> 		 * reading.
> 		 */
>
> that seems to mean that we are either "writing" or "reading".

I never said the current comments are perfect.

Your patch was about adding comments to help later developers.  If you
think this "if we are writing into it" is wrong because implies "it is Ok
to be missing only when we are writing into it", I would very much agree
with _that_ observation.

But then, _this_ comment is what needs to be clarified.

Instead, your patch adds an incorrect observation in a single caller; that
is hardly an improvement.  The observation being incorrect would not help
later people, and one caller being commented would not help as much as the
callee getting correct comments.

So how about improving the comment that is misleading?

I am not sure what the right re-wording for "reading" would be.  It is
similar to "*_gently()" interface, but it is different.  You could call it
"must_exist", but I am not sure if that is much an improvement either.

 refs.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git i/refs.c w/refs.c
index 39a3b23..a712077 100644
--- i/refs.c
+++ w/refs.c
@@ -409,12 +409,15 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *
 		if (--depth < 0)
 			return NULL;
 
-		/* Special case: non-existing file.
-		 * Not having the refs/heads/new-branch is OK
-		 * if we are writing into it, so is .git/HEAD
-		 * that points at refs/heads/master still to be
-		 * born.  It is NOT OK if we are resolving for
-		 * reading.
+		/*
+		 * Special case: non-existing file.
+		 * Not having the refs/heads/new-branch is not OK
+		 * we are resolving for reading.  But not everybody
+		 * calls this function to learn what object the ref
+		 * points at.  E.g. it can be called to learn what the
+		 * symref points at.  Also if we are writing into it,
+		 * it is Ok for .git/HEAD to point at refs/heads/master
+		 * that does not exist yet.
 		 */
 		if (lstat(path, &st) < 0) {
 			struct ref_list *list = get_packed_refs();