From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Tue, 21 Dec 2021 14:16:57 GMT
Subject: Re: [PATCH v7 3/5] object-file.c: refactor write_loose_object() to reuse in stream version
Message-ID: <211221.867dbyrqe9.gmgdl@evledraar.gmail.com>
In-Reply-To: <20211221115201.12120-4-chiyutianyi@gmail.com>


On Tue, Dec 21 2021, Han Xin wrote:

> From: Han Xin <hanxin.hx@alibaba-inc.com>
> [...]
> @@ -1854,17 +1876,48 @@ static int create_tmpfile(struct strbuf *tmp, const char *filename)
>  		strbuf_reset(tmp);
>  		strbuf_add(tmp, filename, dirlen - 1);
>  		if (mkdir(tmp->buf, 0777) && errno != EEXIST)
> -			return -1;
> +			break;
>  		if (adjust_shared_perm(tmp->buf))
> -			return -1;
> +			break;
>  
>  		/* Try again */
>  		strbuf_addstr(tmp, "/tmp_obj_XXXXXX");
>  		fd = git_mkstemp_mode(tmp->buf, 0444);
> +	} while (0);
> +
> +	if (fd < 0 && !(flags & HASH_SILENT)) {
> +		if (errno == EACCES)
> +			return error(_("insufficient permission for adding an "
> +				       "object to repository database %s"),
> +				     get_object_directory());

This should be an error_errno() instead, ...

> +		else
> +			return error_errno(_("unable to create temporary file"));

...and we can just fold this whole if/else into one condition with a
briefer message, e.g.:

    error_errno(_("unable to add object to '%s'"), get_object_directory());

Or whatever, unless there's another bug here where you inverted these
conditions, and the "else" really should not use "error_errno" but
"error".... (I don't know...)