Re: [PATCH 2/4] lock_ref_sha1_basic: simplify errno handling

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Nov 19, 2014, 01:54 UTC
Message-ID: <20141119015452.GQ6527@google.com>
In-Reply-To: <20141119013730.GB2135@peff.net>

Jeff King wrote:

Show 13 quoted lines

> Now that error() does not clobber errno, we do not have to
> take pains to save it ourselves.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  refs.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 5ff457e..169a46d 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2232,7 +2232,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,

All the caller to lock_ref_sha1_basic cares about is whether errno == ENOTDIR, since in that case we can print a message suggesting running "git remote prune".

Longer term, I suspect the caller (transaction_commit) should call is_refname_available to check for conflicting refs and return early to give "git fetch" a chance to print its advice.

If a conflicting ref appears after that point, then just printing a reasonable error message is enough --- it is not so useful to give the 'remote prune' advice when people are doing funny things like running fetch in one terminal and a ref update creating a D/F conflict against that fetch in another terminal.[*]

By the way, Stefan was mentioning the other day that it might make sense for transaction_commit to prevent a conflicting ref from appearing mid-transaction by locking 'refs/heads' in addition to 'refs/heads/master'.

Anyway, in the meantime this is a nice cleanup.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

[*] If we want to handle that, the 'struct strbuf *err' could be replaced with a larger struct with room for structured data, like Junio was hinting at in another thread.