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

From: Christian Couder <chriscool@tuxfamily.org>
Date: Sep 6, 2008, 12:03 UTC
Message-ID: <200809061403.14592.chriscool@tuxfamily.org>
In-Reply-To: <7vwshpsi1g.fsf@gitster.siamese.dyndns.org>

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

Show 22 quoted lines

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

Show 10 quoted lines

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