patch, 5 partsrefs.c: do not die if locking fails in `write_pseudoref()`

> If we could not take the lock, we add an error to the `strbuf err`
> and
> return. However, this code is dead. The reason is that we take the
> lock
> using `LOCK_DIE_ON_ERROR`. Drop the flag to allow our more gentle
> error-handling to actually kick in.
> 
> We could instead just drop the dead code and die here. But everything
> is
> prepared for gently propagating the error, so let's do that instead.
> 
> There is similar dead code in `delete_pseudoref()`, but let's save
> that
> for the next patch.
> 
> While at it, make the lock non-static.
> 
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
>  refs.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 8b7a77fe5e..8c50b8b139 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -644,7 +644,7 @@ static int write_pseudoref(const char *pseudoref,
> const struct object_id *oid,
>  {
>  	const char *filename;
>  	int fd;
> -	static struct lock_file lock;
> +	struct lock_file lock = LOCK_INIT;
>  	struct strbuf buf = STRBUF_INIT;
>  	int ret = -1;
>  
> @@ -654,8 +654,7 @@ static int write_pseudoref(const char *pseudoref,
> const struct object_id *oid,
>  	strbuf_addf(&buf, "%s\n", oid_to_hex(oid));
>  
>  	filename = git_path("%s", pseudoref);
> -	fd = hold_lock_file_for_update_timeout(&lock, filename,
> -					       LOCK_DIE_ON_ERROR,
> +	fd = hold_lock_file_for_update_timeout(&lock, filename, 0,
>  					       get_files_ref_lock_ti
> meout_ms());
>  	if (fd < 0) {
>  		strbuf_addf(err, "could not open '%s' for writing:
> %s",

> Re making the lock static, I wonder about the following case:
>
>       if (read_ref(pseudoref, &actual_old_oid))
>
> die("could not read ref '%s'", pseudoref);
>
> I think this calls exit(), and then atexit tries to clean up the lock
> files.  But since lock is no longer static, the stack may have been
> destroyed (I don't actually know whether this is true, so maybe someone
> else does).

>On 6 May 2018 at 17:48, David Turner <novalis@novalis.org> wrote:
>> On Sun, 2018-05-06 at 16:10 +0200, Martin Ågren wrote:
>>> While at it, make the lock non-static.
>
>> Re making the lock static, I wonder about the following case:
>>
>>       if (read_ref(pseudoref, &actual_old_oid))
>>
>> die("could not read ref '%s'", pseudoref);
>>
>> I think this calls exit(), and then atexit tries to clean up the lock
>> files.  But since lock is no longer static, the stack may have been
>> destroyed (I don't actually know whether this is true, so maybe
>someone
>> else does).
>
>Right. After commit 076aa2cbda (tempfile: auto-allocate tempfiles on
>heap, 2017-09-05) this is safe though. Quite a few locks have already
>been moved to the stack, e.g., in 14bca6c63c (sequencer: make lockfiles
>non-static, 2018-02-27) and 02ae242fdd (checkout-index: simplify
>locking
>logic, 2017-10-05).  I could add a note to the commit message to make
>this clear, like "After 076aa2cbda, locks no longer need to be static."

> >Right. After commit 076aa2cbda (tempfile: auto-allocate tempfiles on
> >heap, 2017-09-05) this is safe though. Quite a few locks have already
> >been moved to the stack, e.g., in 14bca6c63c (sequencer: make lockfiles
> >non-static, 2018-02-27) and 02ae242fdd (checkout-index: simplify
> >locking
> >logic, 2017-10-05).  I could add a note to the commit message to make
> >this clear, like "After 076aa2cbda, locks no longer need to be static."
> 
> I am going to reply now to keep the thread moving, but I am on my
> phone with bad connectivity (few cell towers in Bears Ears), so I
> can't really check the code. Feel free to disregard if I am still
> wrong.
> 
> I saw that patch, but I thought the new logic required that cleanup
> funtions be called before the lock goes out of scope.

> If we could not take the lock, we add an error to the `strbuf err` and
> return. However, this code is dead. The reason is that we take the lock
> using `LOCK_DIE_ON_ERROR`. Drop the flag to allow our more gentle
> error-handling to actually kick in.
>
> We could instead just drop the dead code and die here. But everything is
> prepared for gently propagating the error, so let's do that instead.

>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
>  refs.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 8b7a77fe5e..8c50b8b139 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -644,7 +644,7 @@ static int write_pseudoref(const char *pseudoref, const struct object_id *oid,
>  {
>         const char *filename;
>         int fd;
> -       static struct lock_file lock;
> +       struct lock_file lock = LOCK_INIT;
>         struct strbuf buf = STRBUF_INIT;
>         int ret = -1;
>
> @@ -654,8 +654,7 @@ static int write_pseudoref(const char *pseudoref, const struct object_id *oid,
>         strbuf_addf(&buf, "%s\n", oid_to_hex(oid));
>
>         filename = git_path("%s", pseudoref);
> -       fd = hold_lock_file_for_update_timeout(&lock, filename,
> -                                              LOCK_DIE_ON_ERROR,
> +       fd = hold_lock_file_for_update_timeout(&lock, filename, 0,
>                                                get_files_ref_lock_timeout_ms());
>         if (fd < 0) {
>                 strbuf_addf(err, "could not open '%s' for writing: %s",
> --
> 2.17.0.411.g9fd64c8e46
>