Re: [PATCH v14 06/40] refs.c: add an err argument to repack_without_refs
Thanks.
On Tue, Jun 10, 2014 at 1:10 PM, Jonathan Nieder wrote:
> Ronnie Sahlberg wrote:
>
>> Update repack_without_refs to take an err argument and update it if there
>> is a failure. Pass the err variable from ref_transaction_commit to this
>> function so that callers can print a meaningful error message if _commit
>> fails due to a problem in repack_without_refs.
>>
>> Add a new function unable_to_lock_message that takes a strbuf argument and
>> fills in the reason for the failure.
>>
>> In commit_packed_refs, make sure that we propagate any errno that
>> commit_lock_file might have set back to our caller.
>>
>> Make sure both commit_packed_refs and lock_file has errno set to a meaningful
>> value on error. Update a whole bunch of other places to keep errno from
>> beeing clobbered.
>
> This patch is doing several (all nice) things. Are they connected?
> Would splitting some of them out into separate patches make sense or
> would it be too much fuss to be worth the trouble?
>
> [...]
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -559,6 +559,8 @@ struct lock_file {
>> #define LOCK_DIE_ON_ERROR 1
>> #define LOCK_NODEREF 2
>> extern int unable_to_lock_error(const char *path, int err);
>> +extern void unable_to_lock_message(const char *path, int err,
>> +struct strbuf *buf);
>
> Introducing a new unable_to_lock_message helper, which has nicer
> semantics than unable_to_lock_error and cleans up lockfile.c a little.
>
Done.
> [...]
>> --- a/lockfile.c
>> +++ b/lockfile.c
>> @@ -121,7 +121,7 @@ static char *resolve_symlink(char *p, size_t s)
>> return p;
>> }
>>
>> -
>> +/* Make sure errno contains a meaningful value on error */
>> static int lock_file(struct lock_file *lk, const char *path, int flags)
>
> Making errno when returning from lock_file() meaningful, which should
> fix
>
> * an existing almost-bug in lock_ref_sha1_basic where it assumes
>errno==ENOENT is meaningful and could waste some work on retries
>
> * an existing bug in repack_without_refs where it prints
>strerror(errno) and picks advice based on errno, despite errno
>potentially being zero and potentially having been clobbered by
>that point
>
> To make sure we don't lose these fixes, it would be helpful to also
> mention that errno is set in the API documentation for the relevant
> functions:
>
> * hold_lock_file_for_update (declared in cache.h)
> * lock_packed_refs (declared in refs.h)
Done.
>
> [...]
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1333,8 +1333,10 @@ const char *resolve_ref_unsafe(const char *refname,
>> unsigned char *sha1, int rea
>
> Making errno when returning from resolve_ref_unsafe() meaningful,
> which should fix
>
> * a bug in lock_ref_sha1_basic, where it assumes EISDIR
>means it failed due to a directory being in the way
>
> To make sure we don't lose this fix, it would be helpful to change
> Michael's understated comment (why wasn't it marked with NEEDSWORK,
> btw?)
>
> * errno is sometimes set on errors, but not always.
>
> to describe the new guarantee.
Done.
>
> [...]
>> @@ -1908,14 +1918,17 @@ static struct ref_lock *verify_lock(struct ref_lock
>> *lock,
>
> Making errno when returning from verify_lock() meaningful, which
> should almost but not completely fix
>
> * a bug in "git fetch"'s s_update_ref, which trusts the result of an
>errno == ENOTDIR check to detect D/F conflicts
>
> To make sure we don't lose this fix, it would be helpful to also
> mention that errno is set in the API documentation for the relevant
> functions:
>
> * lock_any_ref_for_update (declared in refs.h), after handling the
>check_refname_format case
> * lock_ref_sha1_basic
>
> ENOTDIR makes sense as a sign that a file was in the way of a
> directory we wanted to create. Should "git fetch" also look for
> ENOTEMPTY or EEXIST to catch cases where a directory was in the way
> of a file to be created?
>
Done.
>> @@ -1928,13 +1941,15 @@ static int remove_empty_directories(const char *file)
>
> Making errno when returning from remove_empty_directories() more
> obviously meaningful, which should provide some peace of mind for
> people auditing lock_ref_sha1_basic.
>
Done.
> [...]
>> @@ -2203,11 +2218,16 @@ int lock_packed_refs(int flags)
> [...]
>> int commit_packed_refs(void)
>
> Making errno when returning from commit_packed_refs() meaningful,
> which should fix
>
> * a bug in "git clone" where it prints strerror(errno) based on
>errno, despite errno possibly being zero and potentially having
>been clobbered by that point
> * the same kind of bug in "git pack-refs"
>
> and prepares for repack_without_refs() to get a meaningful
> error message when commit_packed_refs() fails without falling into
> the same bug.
>
> To make sure we don't lose this fix, it would be helpful to also
> mention that errno is set in the API documentation for
> commit_packed_refs in refs.h.
>
Done.
> [...]
>> @@ -2427,12 +2450,12 @@ static int curate_packed_ref_f
Re: [PATCH v14 06/40] refs.c: add an err argument to repack_without_refs
Ronnie Sahlberg wrote:
> Update repack_without_refs to take an err argument and update it if there
> is a failure. Pass the err variable from ref_transaction_commit to this
> function so that callers can print a meaningful error message if _commit
> fails due to a problem in repack_without_refs.
>
> Add a new function unable_to_lock_message that takes a strbuf argument and
> fills in the reason for the failure.
>
> In commit_packed_refs, make sure that we propagate any errno that
> commit_lock_file might have set back to our caller.
>
> Make sure both commit_packed_refs and lock_file has errno set to a meaningful
> value on error. Update a whole bunch of other places to keep errno from
> beeing clobbered.
This patch is doing several (all nice) things. Are they connected?
Would splitting some of them out into separate patches make sense or
would it be too much fuss to be worth the trouble?
[...]
> --- a/cache.h
> +++ b/cache.h
> @@ -559,6 +559,8 @@ struct lock_file {
> #define LOCK_DIE_ON_ERROR 1
> #define LOCK_NODEREF 2
> extern int unable_to_lock_error(const char *path, int err);
> +extern void unable_to_lock_message(const char *path, int err,
> +struct strbuf *buf);
Introducing a new unable_to_lock_message helper, which has nicer
semantics than unable_to_lock_error and cleans up lockfile.c a little.
[...]
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -121,7 +121,7 @@ static char *resolve_symlink(char *p, size_t s)
> return p;
> }
>
> -
> +/* Make sure errno contains a meaningful value on error */
> static int lock_file(struct lock_file *lk, const char *path, int flags)
Making errno when returning from lock_file() meaningful, which should
fix
* an existing almost-bug in lock_ref_sha1_basic where it assumes
errno==ENOENT is meaningful and could waste some work on retries
* an existing bug in repack_without_refs where it prints
strerror(errno) and picks advice based on errno, despite errno
potentially being zero and potentially having been clobbered by
that point
To make sure we don't lose these fixes, it would be helpful to also
mention that errno is set in the API documentation for the relevant
functions:
* hold_lock_file_for_update (declared in cache.h)
* lock_packed_refs (declared in refs.h)
[...]
> --- a/refs.c
> +++ b/refs.c
> @@ -1333,8 +1333,10 @@ const char *resolve_ref_unsafe(const char *refname,
> unsigned char *sha1, int rea
Making errno when returning from resolve_ref_unsafe() meaningful,
which should fix
* a bug in lock_ref_sha1_basic, where it assumes EISDIR
means it failed due to a directory being in the way
To make sure we don't lose this fix, it would be helpful to change
Michael's understated comment (why wasn't it marked with NEEDSWORK,
btw?)
* errno is sometimes set on errors, but not always.
to describe the new guarantee.
[...]
> @@ -1908,14 +1918,17 @@ static struct ref_lock *verify_lock(struct ref_lock
> *lock,
Making errno when returning from verify_lock() meaningful, which
should almost but not completely fix
* a bug in "git fetch"'s s_update_ref, which trusts the result of an
errno == ENOTDIR check to detect D/F conflicts
To make sure we don't lose this fix, it would be helpful to also
mention that errno is set in the API documentation for the relevant
functions:
* lock_any_ref_for_update (declared in refs.h), after handling the
check_refname_format case
* lock_ref_sha1_basic
ENOTDIR makes sense as a sign that a file was in the way of a
directory we wanted to create. Should "git fetch" also look for
ENOTEMPTY or EEXIST to catch cases where a directory was in the way
of a file to be created?
> @@ -1928,13 +1941,15 @@ static int remove_empty_directories(const char *file)
Making errno when returning from remove_empty_directories() more
obviously meaningful, which should provide some peace of mind for
people auditing lock_ref_sha1_basic.
[...]
> @@ -2203,11 +2218,16 @@ int lock_packed_refs(int flags)
[...]
> int commit_packed_refs(void)
Making errno when returning from commit_packed_refs() meaningful,
which should fix
* a bug in "git clone" where it prints strerror(errno) based on
errno, despite errno possibly being zero and potentially having
been clobbered by that point
* the same kind of bug in "git pack-refs"
and prepares for repack_without_refs() to get a meaningful
error message when commit_packed_refs() fails without falling into
the same bug.
To make sure we don't lose this fix, it would be helpful to also
mention that errno is set in the API documentation for
commit_packed_refs in refs.h.
[...]
> @@ -2427,12 +2450,12 @@ static int curate_packed_ref_fn(struct ref_entry
> *entry, void *cb_data)
> return 0;
> }
>
> -static int repack_without_refs(const char **refnames, int n)
> +static int repack_without_refs(const char **refnames, int n, struct strbuf
> *err)
Adding an 'err' argument to repack_without_refs and writing to it on
error.
[...]
> @@
