Re: Recommendations for updating error() to error_errno()

[Jeff King <peff@peff.net>]

On Wed, Oct 17, 2018 at 11:58:15AM +0800, 牛旭 wrote:

> Our team works on enhance logging practices by learning from historical log revisions in evolution.
> And we find three patches that update error(..., strerror(errmp)) to error_errno(...).
> 
> While applying this rule to git-2.14.2, we find 9 missed spot in file refs/files-backend.c, 1 in apply.c, 2 in trace.c, 4 in dir-iterator.c:.
> 
> Here are the missed spots:
> 1) Line 1734 in file git-2.14.2/refs/files-backend.c:
> ret = raceproof_create_file(path.buf, rename_tmp_log_callback, &cb);
> if (ret) {
> 	if (errno == EISDIR)
> 		error("directory not empty: %s", path.buf);
> 	else
> 		error("unable to move logfile %s to %s: %s",
> 			tmp.buf, path.buf,
> 			strerror(cb.true_errno));

This cannot be converted naively. Using error_errno() will always show
the value of "errno", but here we are using a saved value in
cb.true_errno.

It might work to do:

  errno = cb.true_errno;
  error_errno(...);

but you would first have to check if the function is depending on the
value of errno for other reasons (not to mention that it kind of defeats
the purpose of error_errno() being a shorthand).

> 2) Line 1795 in file git-2.14.2/refs/files-backend.c: 
> if (log && rename(sb_oldref.buf, tmp_renamed_log.buf)) {
> 	ret = error("unable to move logfile logs/%s to logs/"TMP_RENAMED_LOG": %s",
> 			oldrefname, strerror(errno));
> 	goto out;
> }

But this one, for example, would be fine.

-Peff