Re: [PATCH v2 1/3] am: add --show-current-patch
From: Junio C Hamano <hidden>
Date: 2018-02-02 18:56:30
Duy Nguyen [off-list ref] writes:
Subject: [PATCH] Preserve errno in case case before calling sth_errno()
All these locations do something like this
sth_errno(..., somefunc(...));
where somefunc() can potentially change errno, which will be read by
sth_errno(). Call somefunc separately with errno preserved to avoid
this.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
builtin/am.c | 8 +++++++-
builtin/commit.c | 8 ++++++--
builtin/init-db.c | 9 ++++++---
rerere.c | 9 ++++++---
shallow.c | 27 ++++++++++++++++++---------
5 files changed, 43 insertions(+), 18 deletions(-)quoted hunk
diff --git a/builtin/init-db.c b/builtin/init-db.c index c9b7946bad..e384749f73 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c@@ -570,9 +570,12 @@ int cmd_init_db(int argc, const char **argv, const char *prefix) set_git_work_tree(work_tree); else set_git_work_tree(git_work_tree_cfg); - if (access(get_git_work_tree(), X_OK)) - die_errno (_("Cannot access work tree '%s'"), - get_git_work_tree()); + if (access(get_git_work_tree(), X_OK)) { + int saved_errno = errno; + const char *path = get_git_work_tree(); + errno = saved_errno; + die_errno(_("Cannot access work tree '%s'"), path); + } }
This one is the most faithful conversion from "mechanical rewrite"
point of view, but I wonder if we should instead take the returned
path from get_git_work_tree() and use it in both calls. After all,
this is hardly performance sensitive codepath, so even "an obviously
safe but wasteful with extra xstrdup/free" version
work_tree_path = xstrdup(get_git_work_tree());
if (access(work_tree_path, X_OK))
die_errno(_("msg..."), work_tree_path);
free(work_tree_path);
may be an improvement.
quoted hunk
diff --git a/rerere.c b/rerere.c index 1ce440f4bb..f19a53ff2c 100644 --- a/rerere.c +++ b/rerere.c@@ -683,9 +683,12 @@ static int merge(const struct rerere_id *id, const char *path) * A successful replay of recorded resolution. * Mark that "postimage" was used to help gc. */ - if (utime(rerere_path(id, "postimage"), NULL) < 0) - warning_errno("failed utime() on %s", - rerere_path(id, "postimage")); + if (utime(rerere_path(id, "postimage"), NULL) < 0) { + int saved_errno = errno; + const char *path = rerere_path(id, "postimage"); + errno = saved_errno; + warning_errno("failed utime() on %s", path); + }
Likewise.
quoted hunk
diff --git a/shallow.c b/shallow.c index df4d44ea7a..9da82f5292 100644 --- a/shallow.c +++ b/shallow.c@@ -295,9 +295,12 @@ const char *setup_temporary_shallow(const struct oid_array *extra) temp = xmks_tempfile(git_path("shallow_XXXXXX")); if (write_in_full(temp->fd, sb.buf, sb.len) < 0 || - close_tempfile_gently(temp) < 0) - die_errno("failed to write to %s", - get_tempfile_path(temp)); + close_tempfile_gently(temp) < 0) { + int saved_errno = errno; + const char *path = get_tempfile_path(temp); + errno = saved_errno; + die_errno("failed to write to %s", path); + }
It feels a bit questionable to my taste to pretend that we are truly oblivious to how trivial get_tempfile_path() is, i.e. no more than just a few field accesses to "tempfile" struct. It buries more important thing that is happening in the code in noise.
quoted hunk
@@ -319,9 +322,12 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,
Likewise.
quoted hunk
@@ -366,9 +372,12 @@ void prune_shallow(int show_only)
Likewise. All others I snipped looked like good changes. Thanks.