Re: [PATCH 2/5] refs.c: do not die if locking fails in `write_pseudoref()`

From: Stefan Beller <sbeller@google.com>
Date: May 7, 2018, 18:48 UTC
Message-ID: <CAGZ79kZ=7uv3-2mr97uS4Y=1AS5u=koYfH=fMSWfu4L8ajNoKA@mail.gmail.com>
In-Reply-To: <20180506141031.30204-3-martin.agren@gmail.com>

+cc Michael, who did extensive work in the refs.c code.

On Sun, May 6, 2018 at 7:10 AM, Martin Ågren <martin.agren@gmail.com> wrote:

Show 7 quoted lines

> 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.

This looks good to me.

> 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.

We seem to have a lot of static lockfiles in the code base. IIRC that was due to some technicality of the lockfiles, as they would also be cleaned up atexit() and for that it had to be static(?)

Maybe mention why it was static and why we can drop the static
now? Given that you found these answers in a reply below, this is
Reviewed-by: Stefan Beller <sbeller@google.com>

Thanks, Stefan

Show 32 quoted lines

>
> 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
>