Re: [PATCH v5 23/35] lockfile: avoid transitory invalid states

[Jonathan Nieder <jrnieder@gmail.com>]

Michael Haggerty wrote:

> We could probably continue to use the filename field to encode the
> state by being careful to write characters 1..N-1 of the filename
> first, and then overwrite the NUL at filename[0] with the first
> character of the filename, but that would be awkward and error-prone.
> 
> So, instead of using the filename field to determine whether the
> lock_file object is active, add a new field "lock_file::active" for
> this purpose.

Nice.

[...]
> --- a/cache.h
> +++ b/cache.h
> @@ -576,6 +576,7 @@ extern int refresh_index(struct index_state *, unsigned int flags, const struct
>  
>  struct lock_file {
>  	struct lock_file *next;
> +	volatile sig_atomic_t active;
>  	int fd;
>  	pid_t owner;
>  	char on_list;
[...]
> +++ b/lockfile.c
> @@ -27,16 +27,19 @@
[...]
> @@ -189,9 +198,14 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
>  		atexit(remove_lock_file);
>  	}
>  
> +	if (lk->active)
> +		die("BUG: lock_file(\"%s\") called with an active lock_file object",
> +		    path);

The error message doesn't make it entirely obvious to me what went
wrong.

Maybe something like

		die("BUG: cannot lock_file(\"%s\") on active struct lock_file",
		    path);

lock_file already assumed on_list was initialized to zero but it
wasn't particularly obvious since everything else is blindly
scribbled over.  Probably worth mentioning in the API docs that
the lock_file should be zeroed before calling hold_lock_file_...

[...]
> @@ -326,6 +341,7 @@ int commit_lock_file(struct lock_file *lk)
>  	if (rename(lk->filename, result_file))
>  		goto rollback;
>  
> +	lk->active = 0;
>  	lk->filename[0] = 0;

Is it useful to set filename[0] any more?

It seems potentially fragile to set both, since new code can appear
that only sets or checks one or the other.  Would it make sense to
rename the filename field to make sure no new callers relying on the
filename[0] convention sneak in (with the new convention being that
the filename doesn't get cleared, to avoid problems)?

Jonathan