Recommendations for adding strerror() in log statament
Hi,
Our team works on enhance logging practices by learning from historical log
revisions in evolution.
And we find 2 patches that add strerror() to the log statement which is printed
when the return value of commit_lock_file() is smaller than 0.
While applying this rule to git-2.14.2, we find 3 missed spots.
We suggest to add strerror() or just use error_errno() instead of error().
Here are the missed spots:
1) Line 307 in file git-2.14.2/sequencer.c:
static int write_message(const void *buf, size_t len, const char *filename,
int append_eol)
{
...
if (append_eol && write(msg_fd, "\n", 1) < 0) {
rollback_lock_file(_file);
return error_errno(_("could not write eol to '%s'"), filename);
}
if (commit_lock_file(_file) < 0) {
rollback_lock_file(_file);
return error(_("failed to finalize '%s'."), filename);
}
2) Line 1582 in file git-2.14.2/sequencer.c:
static int save_head(const char *head)
{
...
strbuf_addf(, "%s\n", head);
if (write_in_full(fd, buf.buf, buf.len) < 0) {
rollback_lock_file(_lock);
return error_errno(_("could not write to '%s'"),
git_path_head_file());
}
if (commit_lock_file(_lock) < 0) {
rollback_lock_file(_lock);
return error(_("failed to finalize '%s'."),
git_path_head_file());
}
3) Line 1706 in file git-2.14.2/sequencer.c:
static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
{
...
if (write_in_full(fd, todo_list->buf.buf + offset,
todo_list->buf.len - offset) < 0)
return error_errno(_("could not write to '%s'"), todo_path);
if (commit_lock_file(_lock) < 0)
return error(_("failed to finalize '%s'."), todo_path);
Following are the 2 patches that support our opinion:
1) Line 2147 in file git-2.6.4/config.c:
if (commit_lock_file(lock) < 0) {
- error("could not commit config file %s", config_filename);
+ error("could not write config file %s: %s", config_filename,
+ strerror(errno));
ret = CONFIG_NO_WRITE;
lock = NULL;
goto out_free;
}
2) Line 2333 in file git-2.6.4/config.c:
unlock_and_out:
if (commit_lock_file(lock) < 0)
- ret = error("could not commit config file %s", config_filename);
+ ret = error("could not write config file %s: %s",
+ config_filename, strerror(errno));
out:
free(filename_buf);
Thanks for reading, we are looking forward to your reply about the correctness
of our suggestion~
May you a good day ^^
Best regards,
Xu
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:
recommendations for log enhancement
Our team studies the consistent edits of git during evolution. And we find
several missed edits in the latest release of git. For example, there are two
consist edits we have figured out from historical commits:
1) . Version: git 2.3.9 – git-2.3.10
File: builtin/merge-tree.c
dst.size = size;
- xdi_diff(, , , , );
+ if (xdi_diff(, , , , ))
+ die("unable to generate diff");
free(src.ptr);
free(dst.ptr);
}
2) .Version: git 2.3.9 – git-2.3.10
File: combine-diff.c
- xdi_diff_outf(_file, result_file, consume_line, ,
- , );
+ if (xdi_diff_outf(_file, result_file, consume_line, ,
+ , ))
+ die("unable to generate combined diff for %s",
+ sha1_to_hex(parent));
free(parent_file.ptr);
Those two commits both add if structure and log messages for handling the
return value of xdi_diff_outf().
And in the latest release, we find one candidate that may also need log
statements inserted:
1) File: git-2.14.2/builtin/rerere.c
1 static int diff_two(const char *file1, const char *label1,
2 const char *file2, const char *label2)
3 {
….
20 ret = xdi_diff(, , , , );
21
22 free(minus.ptr);
23 free(plus.ptr);
24 return ret;
25 }
...
}
There are more examples of consistent update and corresponding suggestions in
attachment. It is so nice of you to read them and share me with your opinion on
the correctness of our suggestion. Thanks a lot. <>
<>
<>
Recommendation for consistent update on invoke of "sha1_to_hex()"
Our team researches on consistent update of Git during evolution. And we have
figured out several spots that may be missed.
By mining historical patches, we suggest that invokes of sha1_to_hex() should
be replaced with that of oid_to_hex(). One example for recommendation and
corresponding patch are listed as follows.
One example of missed spot:
1 void assert_sha1_type(const unsigned char *sha1, enum
object_type expect)
2 {
3 enum object_type type = sha1_object_info(sha1, NULL);
4 if (type < 0)
5 die("%s is not a valid object",sha1_to_hex(sha1));
6 if (type != expect)
7 die("%s is not a valid '%s' object", sha1_to_hex(sha1),
8 typename(expect));
9 }
One example of historical patch:
1 * We do not want to call parse_object here, because
2 * inflating blobs and trees could be very expensive.
3 * However, we do need to know the correct type for
4 * later processing, and the revision machinery expects
5 * commits and tags to have been parsed.
6 */
7 - type = sha1_object_info(sha1, NULL);
8 + type = sha1_object_info(oid->hash, NULL);
9 if (type < 0)
10 - die("unable to get object info for %s", sha1_to_hex(sha1));
11 + die("unable to get object info for %s", oid_to_hex(oid));
12
More recommendations and supporting patches are saved in attachments. It is so
kind of you to reply me about the correctness of our suggestions. And thank you
for your reading.
<>
<>
Patch recommendation for replace invoke of error() to that of error_errno()
Our team researches on consistent update of Git during evolution. And we have
figured out several spots that may be missed.
By mining historical patches, we suggest that invokes of error(...,
strerror(errno)) should be replaced with that of error_errno(). One example for
recommendation and corresponding patch are listed as follows.
One example of missed spot:
1 int cmd_fetch__tool(int argc, const char **argv, const char
*prefix)
2 {
31 filename = git_path_fetch_head();
32 fp = fopen(filename, "a");
33 if (!fp)
34 return error("cannot open %s: %s", filename, strerror(errno));
}
One example of historical patch:
1 if (!strcmp(*paths, "-"))
2 in = stdin;
3 else
4 in = fopen(*paths, "r");
5
6 if (!in)
7 - return error(_("could not open '%s' for reading: %s"),
8 - *paths, strerror(errno));
9 + return error_errno(_("could not open '%s' for reading"),
10 + *paths);
11
12 mail = mkpath("%s/%0*d", state->dir, state->prec, i + 1);
13
14 out = fopen(mail, "w");
15 if (!out)
16 - return error(_("could not open '%s' for writing: %s"),
17 - mail, strerror(errno));
18 + return error_errno(_("could not open '%s' for writing"),
19 + mail);
20
21 ret = fn(out, in, keep_cr);
22
23 fclose(out);
24 fclose(in);
25
More recommendations and supporting patches are saved in attachments. It is so
kind of you to reply me about the correctness of our suggestions. And thank you
for your reading. <>
<>
