patchbuiltin-symbolic-ref: comment on the use of "resolve_ref" with reading == 0

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.

Christian Couder <chriscool@tuxfamily.org> writes:

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.

Le samedi 6 septembre 2008, Junio C Hamano a écrit :

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

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".

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.

Christian Couder <chriscool@tuxfamily.org> writes:

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

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

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(-)