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

[Jeff King]

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, );
> 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

Recommendations for updating error() to error_errno()

[牛旭]

Hi,
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, );
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));
}

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;
}

3) Line 1880, 1884 in file git-2.14.2/refs/files-backend.c:
rollbacklog:
if (logmoved && rename(sb_newref.buf, sb_oldref.buf))
error("unable to restore logfile %s from %s: %s",
oldrefname, newrefname, strerror(errno));
if (!logmoved && log &&
rename(tmp_renamed_log.buf, sb_oldref.buf))
error("unable to restore logfile %s from 
logs/"TMP_RENAMED_LOG": %s",
oldrefname, strerror(errno));
4) Line 2247 in file git-2.14.2/refs/files-backend.c:
if (commit_ref(lock) < 0)
return error("unable to write symref for %s: %s", refname,
 strerror(errno));

5) Line 2366 in file git-2.14.2/refs/files-backend.c:
if (fseek(logfp, 0, SEEK_END) < 0)
ret = error("cannot seek back reflog for %s: %s",
refname, strerror(errno));

6) Line 2378, 2384 in file git-2.14.2/refs/files-backend.c:
if (fseek(logfp, pos - cnt, SEEK_SET)) {
ret = error("cannot seek back reflog for %s: %s",
refname, strerror(errno));
break;
} 
nread = fread(buf, cnt, 1, logfp);
if (nread != 1) {
ret = error("cannot read %d bytes from reflog for %s: %s",
cnt, refname, strerror(errno));
break;
}

7) Line 3312 in file git-2.14.2/refs/files-backend.c:
cb.newlog = fdopen_lock_file(_lock, "w");
if (!cb.newlog) {
error("cannot fdopen %s (%s)",
  get_lock_file_path(_lock), strerror(errno));
goto failure;
}

8) Line 3337 in file git-2.14.2/refs/files-backend.c:
if (close_lock_file(_lock)) {
status |= error("couldn't write %s: %s", log_file,
   strerror(errno));

9) Line 3348 in file git-2.14.2/refs/files-backend.c:
static int files_reflog_expire(struct ref_store *ref_store,...
{
...
} else if (commit_lock_file(_lock)) {
status |= error("unable to write reflog '%s' (%s)",
log_file, strerror(errno));

10) Line 4859 in file git-2.14.2/apply.c:
fd = open(arg, O_RDONLY);
if (fd < 0) {
error(_("can't open patch '%s': %s"), arg, strerror(errno));
res = -128;
free(to_free);
goto end;
}

11) Line 64 in file git-2.14.2/trace.c:
int fd = open(trace, O_WRONLY | O_APPEND | O_CREAT, 0666);
if (fd == -1) {
warning("could not open '%s' for tracing: %s",
trace, strerror(errno));

12) Line 133 in file git-2.14.2/trace.c:
static void trace_write(struct trace_key *key, const void *buf, unsigned len)
{
if (write_in_full(get_trace_fd(key), buf, len) < 0) {
normalize_trace_key();
warning("unable to write trace for %s: %s",
key->key, strerror(errno));

13) Line 74 in file git-2.14.2/dir-iterator.c:
level->dir = opendir(iter->base.path.buf);
if (!level->dir && errno != ENOENT) {
warning("error opening directory %s: %s",
iter->base.path.buf, strerror(errno));
/* Popping the level is handled below */
}

14) Line 125, 128 in file git-2.14.2/dir-iterator.c:
de = readdir(level->dir);
if (!de) {
/* This level is exhausted; pop up a level. */
if (errno) {
warning("error reading directory %s: %s",
iter->base.path.buf, strerror(errno));
} else if (closedir(level->dir))
warning("error closing directory %s: %s",
iter->base.path.buf, strerror(errno));

15) Line 143 in file git-2.14.2/dir-iterator.c:
if (lstat(iter->base.path.buf, >base.st) < 0) {
if (errno != ENOENT)
warning("error reading path '%s': %s",
iter->base.path.buf,
strerror(errno));

16) Line 174 in file git-2.14.2/dir-iterator.c:
if (level->dir && closedir(level->dir)) {
strbuf_setlen(>base.path, level->prefix_len);
warning("error closing directory %s: