Re: [PATCH v2] lockfile: add PID file for debugging stale locks

[Junio C Hamano <gitster@pobox.com>]

"Paulo Casaretto via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  /* Make sure errno contains a meaningful value on error */
>  static int lock_file(struct lock_file *lk, const char *path, int flags,
> -		     int mode)
> +		     int mode, enum lockfile_pid_component component)
> ...
>  }
>  
> @@ -102,7 +203,8 @@ static int lock_file(struct lock_file *lk, const char *path, int flags,
>   * exactly once. If timeout_ms is -1, try indefinitely.
>   */
>  static int lock_file_timeout(struct lock_file *lk, const char *path,
> -			     int flags, long timeout_ms, int mode)
> +			     int flags, long timeout_ms, int mode,
> +			     enum lockfile_pid_component component)
>  {
> ...
>  	if (timeout_ms == 0)
> -		return lock_file(lk, path, flags, mode);
> +		return lock_file(lk, path, flags, mode, component);
> -		fd = lock_file(lk, path, flags, mode);
> +		fd = lock_file(lk, path, flags, mode, component);

These are OK, but I expected these are rolled into an "unsigned
flags" word, so that ...

>  int hold_lock_file_for_update_timeout_mode(
> -		struct lock_file *lk, const char *path,
> -		int flags, long timeout_ms, int mode);
> +	struct lock_file *lk, const char *path,
> +	int flags, long timeout_ms, int mode,
> +	enum lockfile_pid_component component);

... things like this can be done without adding an extra parameter.
Compared to "what should we do when we see an error?", ...

> -	fd = hold_lock_file_for_update_timeout(&lock, path.buf, LOCK_DIE_ON_ERROR, -1);
> +	fd = hold_lock_file_for_update_timeout(&lock, path.buf, LOCK_DIE_ON_ERROR, -1,
> +					       LOCKFILE_PID_OTHER);

... "how would we name the lockfile for this action?" is *not* all
that special and should not occupy a separate parameter on its own.

Existing "flags" argument being "int" not "unsigned int" is a
historical mistake, by the way.

But maybe it is just me?  I dunno.