Re: [PATCH 1/6] Add docstrings for lookup_replace_object() and do_lookup_replace_object()
Michael Haggerty writes: > Junio, what would be easiest for you? I suggest that I rebase this > patch series back on top of mh/replace-refs-variable-rename when re-rolling. Hmph, I suspect I do not care too deeply either way, as a mismerge would be fairly obvious (nobody should be adding any new string "read_replace_refs" or deleting existing ones and the linker would catch it even if I don't), but having on top of the patch that renames the variable would make sense. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] Add docstrings for lookup_replace_object() and do_lookup_replace_object()
On 02/24/2014 10:24 AM, Christian Couder wrote:
> On Fri, Feb 21, 2014 at 5:32 PM, Michael Haggerty
> wrote:
>> Signed-off-by: Michael Haggerty
>> ---
>> cache.h | 16
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/cache.h b/cache.h
>> index dc040fb..0ecd1c8 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -788,13 +788,29 @@ static inline void *read_sha1_file(const unsigned char
>> *sha1, enum object_type *
>> {
>> return read_sha1_file_extended(sha1, type, size,
>> LOOKUP_REPLACE_OBJECT);
>> }
>> +
>> +/*
>> + * If a replacement for object sha1 has been set up, return the
>> + * replacement object's name (replaced recursively, if necessary).
>> + * The return value is either sha1 or a pointer to a
>> + * permanently-allocated value. This function always respects replace
>> + * references, regardless of the value of check_replace_refs.
>
> Here you talk about "check_replace_refs" ...
>
>> + */
>> extern const unsigned char *do_lookup_replace_object(const unsigned char
>> *sha1);
>> +
>> +/*
>> + * If object sha1 should be replaced, return the replacement object's
>> + * name. This function is similar to do_lookup_replace_object(),
>> + * except that it when object replacement is suppressed, it always
>> + * returns its argument unchanged.
>> + */
>> static inline const unsigned char *lookup_replace_object(const unsigned
>> char *sha1)
>> {
>> if (!read_replace_refs)
>
> ... but here "read_replace_refs" is used.
>
>> return sha1;
>> return do_lookup_replace_object(sha1);
>> }
You're right; thanks for noticing. I originally implemented this patch
on top of mh/replace-refs-variable-rename but then separated them after
all, in the hopes that the latter would be straightforward enough to be
merged quickly, before conflicting patch series appear.
Junio, what would be easiest for you? I suggest that I rebase this
patch series back on top of mh/replace-refs-variable-rename when re-rolling.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] Add docstrings for lookup_replace_object() and do_lookup_replace_object()
On Fri, Feb 21, 2014 at 5:32 PM, Michael Haggerty wrote:
> Signed-off-by: Michael Haggerty
> ---
> cache.h | 16
> 1 file changed, 16 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index dc040fb..0ecd1c8 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -788,13 +788,29 @@ static inline void *read_sha1_file(const unsigned char
> *sha1, enum object_type *
> {
> return read_sha1_file_extended(sha1, type, size,
> LOOKUP_REPLACE_OBJECT);
> }
> +
> +/*
> + * If a replacement for object sha1 has been set up, return the
> + * replacement object's name (replaced recursively, if necessary).
> + * The return value is either sha1 or a pointer to a
> + * permanently-allocated value. This function always respects replace
> + * references, regardless of the value of check_replace_refs.
Here you talk about "check_replace_refs" ...
> + */
> extern const unsigned char *do_lookup_replace_object(const unsigned char
> *sha1);
> +
> +/*
> + * If object sha1 should be replaced, return the replacement object's
> + * name. This function is similar to do_lookup_replace_object(),
> + * except that it when object replacement is suppressed, it always
> + * returns its argument unchanged.
> + */
> static inline const unsigned char *lookup_replace_object(const unsigned char
> *sha1)
> {
> if (!read_replace_refs)
... but here "read_replace_refs" is used.
> return sha1;
> return do_lookup_replace_object(sha1);
> }
Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] Add docstrings for lookup_replace_object() and do_lookup_replace_object()
On 02/21/2014 07:21 PM, Junio C Hamano wrote:
> Michael Haggerty writes:
>
>> Signed-off-by: Michael Haggerty
>> ---
>> cache.h | 16
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/cache.h b/cache.h
>> index dc040fb..0ecd1c8 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -788,13 +788,29 @@ static inline void *read_sha1_file(const unsigned char
>> *sha1, enum object_type *
>> {
>> return read_sha1_file_extended(sha1, type, size, LOOKUP_REPLACE_OBJECT);
>> }
>> +
>> +/*
>> + * If a replacement for object sha1 has been set up, return the
>> + * replacement object's name (replaced recursively, if necessary).
>> + * The return value is either sha1 or a pointer to a
>> + * permanently-allocated value. This function always respects replace
>> + * references, regardless of the value of check_replace_refs.
>> + */
>> extern const unsigned char *do_lookup_replace_object(const unsigned char
>> *sha1);
>> +
>> +/*
>> + * If object sha1 should be replaced, return the replacement object's
>> + * name. This function is similar to do_lookup_replace_object(),
>> + * except that it when object replacement is suppressed, it always
>> + * returns its argument unchanged.
>> + */
>> static inline const unsigned char *lookup_replace_object(const unsigned
>> char *sha1)
>> {
>> if (!read_replace_refs)
>> return sha1;
>> return do_lookup_replace_object(sha1);
>> }
>> +
>> static inline const unsigned char *lookup_replace_object_extended(const
>> unsigned char *sha1, unsigned flag)
>> {
>> if (!(flag & LOOKUP_REPLACE_OBJECT))
>
> The above description is good, but after reading ecef (inline
> lookup_replace_object() calls, 2011-05-15) that introduced this
> ugliness, I have to wonder if do_lookup_replace(), which nobody
> except lookup_replace_object() ever calls, is better removed from
> the public API, making lookup_replace_object() an extern definition.
>
> We do name functions that are purely helpers that are internal
> implementation detals of the API as "do_blah", but exporting that
> kind of name as if that is part of the API people are expected to
> call feels very wrong.
I assume that the current design was to avoid the overhead of a function
call in the case that no replace references exist. If we're willing to
eat that cost, then sure, we should bury do_lookup_replace_object() in
the implementation file.
Unless you say otherwise, I will work that change into my patch series.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] Add docstrings for lookup_replace_object() and do_lookup_replace_object()
Michael Haggerty writes:
> Signed-off-by: Michael Haggerty
> ---
> cache.h | 16
> 1 file changed, 16 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index dc040fb..0ecd1c8 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -788,13 +788,29 @@ static inline void *read_sha1_file(const unsigned char
> *sha1, enum object_type *
> {
> return read_sha1_file_extended(sha1, type, size, LOOKUP_REPLACE_OBJECT);
> }
> +
> +/*
> + * If a replacement for object sha1 has been set up, return the
> + * replacement object's name (replaced recursively, if necessary).
> + * The return value is either sha1 or a pointer to a
> + * permanently-allocated value. This function always respects replace
> + * references, regardless of the value of check_replace_refs.
> + */
> extern const unsigned char *do_lookup_replace_object(const unsigned char
> *sha1);
> +
> +/*
> + * If object sha1 should be replaced, return the replacement object's
> + * name. This function is similar to do_lookup_replace_object(),
> + * except that it when object replacement is suppressed, it always
> + * returns its argument unchanged.
> + */
> static inline const unsigned char *lookup_replace_object(const unsigned char
> *sha1)
> {
> if (!read_replace_refs)
> return sha1;
> return do_lookup_replace_object(sha1);
> }
> +
> static inline const unsigned char *lookup_replace_object_extended(const
> unsigned char *sha1, unsigned flag)
> {
> if (!(flag & LOOKUP_REPLACE_OBJECT))
The above description is good, but after reading ecef (inline
lookup_replace_object() calls, 2011-05-15) that introduced this
ugliness, I have to wonder if do_lookup_replace(), which nobody
except lookup_replace_object() ever calls, is better removed from
the public API, making lookup_replace_object() an extern definition.
We do name functions that are purely helpers that are internal
implementation detals of the API as "do_blah", but exporting that
kind of name as if that is part of the API people are expected to
call feels very wrong.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/6] Add docstrings for lookup_replace_object() and do_lookup_replace_object()
Signed-off-by: Michael Haggerty
---
cache.h | 16
1 file changed, 16 insertions(+)
diff --git a/cache.h b/cache.h
index dc040fb..0ecd1c8 100644
--- a/cache.h
+++ b/cache.h
@@ -788,13 +788,29 @@ static inline void *read_sha1_file(const unsigned char
*sha1, enum object_type *
{
return read_sha1_file_extended(sha1, type, size, LOOKUP_REPLACE_OBJECT);
}
+
+/*
+ * If a replacement for object sha1 has been set up, return the
+ * replacement object's name (replaced recursively, if necessary).
+ * The return value is either sha1 or a pointer to a
+ * permanently-allocated value. This function always respects replace
+ * references, regardless of the value of check_replace_refs.
+ */
extern const unsigned char *do_lookup_replace_object(const unsigned char
*sha1);
+
+/*
+ * If object sha1 should be replaced, return the replacement object's
+ * name. This function is similar to do_lookup_replace_object(),
+ * except that it when object replacement is suppressed, it always
+ * returns its argument unchanged.
+ */
static inline const unsigned char *lookup_replace_object(const unsigned char
*sha1)
{
if (!read_replace_refs)
return sha1;
return do_lookup_replace_object(sha1);
}
+
static inline const unsigned char *lookup_replace_object_extended(const
unsigned char *sha1, unsigned flag)
{
if (!(flag & LOOKUP_REPLACE_OBJECT))
--
1.8.5.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
