Re: [PATCH 3/6] object-name: make get_oid quietly return an error

> On Mar 16, 2022, at 10:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
>> A reasonable person looking at the signature and usage of get_oid and
>> friends might conclude that in the event of an error, it always returns
>> -1.  However, this is not the case.  Instead, get_oid_basic dies if we
>> go too far back into the history of a reflog (or, when quiet, simply
>> exits).
>> 
>> This is not especially useful, since in many cases, we might want to
>> handle this error differently.  Let's add a flag here to make it just
>> return -1 like elsewhere in these code paths.
>> 
>> Note that we cannot make this behavior the default, since we have many
>> other codepaths that rely on the existing behavior, including in tests.
>> 
>> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
>> ---
>> cache.h       | 21 +++++++++++----------
>> object-name.c |  6 +++++-
>> 2 files changed, 16 insertions(+), 11 deletions(-)
>> 
>> diff --git a/cache.h b/cache.h
>> index 825ec17198..416a9d9983 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -1366,16 +1366,17 @@ struct object_context {
>>    char *path;
>> };
>> 
>> -#define GET_OID_QUIETLY           01
>> -#define GET_OID_COMMIT            02
>> -#define GET_OID_COMMITTISH        04
>> -#define GET_OID_TREE             010
>> -#define GET_OID_TREEISH          020
>> -#define GET_OID_BLOB             040
>> -#define GET_OID_FOLLOW_SYMLINKS 0100
>> -#define GET_OID_RECORD_PATH     0200
>> -#define GET_OID_ONLY_TO_DIE    04000
>> -#define GET_OID_REQUIRE_PATH  010000
>> +#define GET_OID_QUIETLY            01
>> +#define GET_OID_COMMIT             02
>> +#define GET_OID_COMMITTISH         04
>> +#define GET_OID_TREE              010
>> +#define GET_OID_TREEISH           020
>> +#define GET_OID_BLOB              040
>> +#define GET_OID_FOLLOW_SYMLINKS  0100
>> +#define GET_OID_RECORD_PATH      0200
>> +#define GET_OID_ONLY_TO_DIE     04000
>> +#define GET_OID_REQUIRE_PATH   010000
>> +#define GET_OID_RETURN_FAILURE 020000
> 
> I do not think we want this change.  The next time somebody adds an
> overly long symbol, we reformat all the lines, making it hard to
> spot that the change is only adding a single new symbol?
> 
> I think we'd rather go the other way not to tempt people into
> right-aligning these constants, either by rewriting them into
> 
>    #define GET_OID_QUIETLY<TAB>(1U << 1)
>    #define GET_OID_COMMIT<TAB>(1U << 2)
>    #define GET_OID_COMMITTISH<TAB>(1U << 3)
>    ...
>    
> in a separate preliminary patch without adding a new symbol, or
> adding the new symbol unaligned and without touching existing lines.
> 
>> diff --git a/object-name.c b/object-name.c
>> index 92862eeb1a..daa3ef77ef 100644
>> --- a/object-name.c
>> +++ b/object-name.c
>> @@ -911,13 +911,17 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
>>                        len, str,
>>                        show_date(co_time, co_tz, DATE_MODE(RFC2822)));
>>                }
>> -            } else {
>> +            } else if (!(flags & GET_OID_RETURN_FAILURE)) {
>>                if (flags & GET_OID_QUIETLY) {
>>                    exit(128);
>>                }
>>                die(_("log for '%.*s' only has %d entries"),
>>                    len, str, co_cnt);
>>            }
>> +            if (flags & GET_OID_RETURN_FAILURE) {
>> +                free(real_ref);
>> +                return -1;
>> +            }
>>        }
> 
> So, without the new bit, we used to die loudly or quietly.  The new
> bit allows us to return an error to the caller without dying
> ourselves.
> 
> You can call the bit _RETURN_ERROR and not to worry about the
> right-alignment above ;-), but better yet, how about calling it
> _GENTLY, which is how we call such a variant of behaviour?
>