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

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

[Martin Ågren]

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_timeout_ms());
 	if (fd < 0) {
 		strbuf_addf(err, "could not open '%s' for writing: %s",
-- 
2.17.0.411.g9fd64c8e46

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

[David Turner]

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 Sun, 2018-05-06 at 16:10 +0200, Martin Ågren wrote:
> 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: [PATCH 2/5] refs.c: do not die if locking fails in `write_pseudoref()`

[Martin Ågren]

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

Martin

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

[David Turner]

On May 6, 2018 9:56:31 AM MDT, "Martin Ågren" <martin.agren@gmail.com> wrote:
>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."

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.

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

[Stefan Beller]

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

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

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

[Jeff King]

On Mon, May 07, 2018 at 05:12:27AM -0600, David Turner wrote:

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

No, it should be fine. After 422a21c6a0 (tempfile: remove deactivated
list entries, 2017-09-05) it became _possible_ to use a non-static
tempfile. But it was dangerous, because if you failed to clean up, bad
things would happen. So right after that in 076aa2cbda we switched to
using the heap, which means the tempfile code takes full ownership, and
the local lockfile variable is just a pointer to that storage.

-Peff