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

From: Jeff King <peff@peff.net>
Date: Oct 17, 2018, 08:44 UTC
Message-ID: <20181017084443.GD31932@sigill.intra.peff.net>
In-Reply-To: <6849fb3c.10199.166802c8ef3.Coremail.niuxu16@nudt.edu.cn>

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

Show 15 quoted lines

> 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).

Show 6 quoted lines

> 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