Christian Couder <chriscool@xxxxxxxxxxxxx> writes:
> Le samedi 6 septembre 2008, Junio C Hamano a écrit :
>> Christian Couder <chriscool@xxxxxxxxxxxxx> 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();
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
