Re: [PATCH v2 1/3] am: add --show-current-patch
Duy Nguyenwrites: > 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 > --- > 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(-) > 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. > 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. > 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_XX")); > > 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. > @@ -319,9 +322,12 @@ void setup_alternate_shallow(struct lock_file > *shallow_lock, Likewise. > @@ -366,9 +372,12 @@ void prune_shallow(int show_only) Likewise. All others I snipped looked like good changes. Thanks.
Re: [PATCH v2 1/3] am: add --show-current-patch
On Fri, Feb 2, 2018 at 4:46 PM, Eric Sunshinewrote: > On Fri, Feb 2, 2018 at 4:25 AM, Duy Nguyen wrote: >> On Wed, Jan 31, 2018 at 02:59:32PM -0800, Junio C Hamano wrote: >>> Eric Sunshine writes: >>> > On Wed, Jan 31, 2018 at 4:30 AM, Nguyễn Thái Ngọc Duy >>> > wrote: >>> >> + len = strbuf_read_file(, am_path(state, msgnum(state)), 0); >>> >> + if (len < 0) >>> >> + die_errno(_("failed to read '%s'"), >>> >> + am_path(state, msgnum(state))); >>> > >>> > Isn't this am_path() invocation inside die_errno() likely to clobber >>> > the 'errno' from strbuf_read_file() which you want to be reporting? >>> True. >> >> Thanks both. Good catch. Of course I will fix this in the re-roll, but >> should we also do something for the current code base with the >> following patch? >> >> - die_errno(_("could not read '%s'"), am_path(state, file)); >> + saved_errno = errno; >> + path = am_path(state, file); >> + errno = saved_errno; >> + die_errno(_("could not read '%s'"), path); > > Rather than worrying about catching these at review time, I had been > thinking about a solution which automates it using variadic macros. > Something like: > > #define die_errno(...) do { \ > int saved_errno_ = errno; \ > die_errno_(saved_errno_, __VA_ARGS__); \ > } while (0); That would be best. Unfortunately we have HAVE_VARIADIC_MACROS so we need to deal with no variadic macro support too. -- Duy
Re: [PATCH v2 1/3] am: add --show-current-patch
On Fri, Feb 2, 2018 at 4:25 AM, Duy Nguyenwrote: > On Wed, Jan 31, 2018 at 02:59:32PM -0800, Junio C Hamano wrote: >> Eric Sunshine writes: >> > On Wed, Jan 31, 2018 at 4:30 AM, Nguyễn Thái Ngọc Duy >> > wrote: >> >> + len = strbuf_read_file(, am_path(state, msgnum(state)), 0); >> >> + if (len < 0) >> >> + die_errno(_("failed to read '%s'"), >> >> + am_path(state, msgnum(state))); >> > >> > Isn't this am_path() invocation inside die_errno() likely to clobber >> > the 'errno' from strbuf_read_file() which you want to be reporting? >> True. > > Thanks both. Good catch. Of course I will fix this in the re-roll, but > should we also do something for the current code base with the > following patch? > > - die_errno(_("could not read '%s'"), am_path(state, file)); > + saved_errno = errno; > + path = am_path(state, file); > + errno = saved_errno; > + die_errno(_("could not read '%s'"), path); Rather than worrying about catching these at review time, I had been thinking about a solution which automates it using variadic macros. Something like: #define die_errno(...) do { \ int saved_errno_ = errno; \ die_errno_(saved_errno_, __VA_ARGS__); \ } while (0);
Re: [PATCH v2 1/3] am: add --show-current-patch
On Wed, Jan 31, 2018 at 02:59:32PM -0800, Junio C Hamano wrote: > Eric Sunshinewrites: > > > On Wed, Jan 31, 2018 at 4:30 AM, Nguyễn Thái Ngọc Duy > > wrote: > >> Pointing the user to $GIT_DIR/rebase-apply may encourage them to mess > >> around in there, which is not a good thing. With this, the user does > >> not have to keep the path around somewhere (because after a couple of > >> commands, the path may be out of scrollback buffer) when they need to > >> look at the patch. > >> > >> Signed-off-by: Nguyễn Thái Ngọc Duy > >> --- > >> diff --git a/builtin/am.c b/builtin/am.c > >> @@ -2121,6 +2120,22 @@ static void am_abort(struct am_state *state) > >> +static int show_patch(struct am_state *state) > >> +{ > >> + struct strbuf sb = STRBUF_INIT; > >> + int len; > >> + > >> + len = strbuf_read_file(, am_path(state, msgnum(state)), 0); > >> + if (len < 0) > >> + die_errno(_("failed to read '%s'"), > >> + am_path(state, msgnum(state))); > > > > Isn't this am_path() invocation inside die_errno() likely to clobber > > the 'errno' from strbuf_read_file() which you want to be reporting? > > True. Thanks both. Good catch. Of course I will fix this in the re-roll, but should we also do something for the current code base with the following patch? I think this is something coccinelle can help catch, but my coccinelle-foo is way too low to come up with something like "die_errno() must not take any arguments as function calls, except _() and N_()". -- 8< -- 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 --- 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(-) diff --git a/builtin/am.c b/builtin/am.c index acfe9d3c8c..ba67e187a4 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -244,6 +244,9 @@ static int am_in_progress(const struct am_state *state) static int read_state_file(struct strbuf *sb, const struct am_state *state, const char *file, int trim) { + int saved_errno; + const char *path; + strbuf_reset(sb); if (strbuf_read_file(sb, am_path(state, file), 0) >= 0) { @@ -256,7 +259,10 @@ static int read_state_file(struct strbuf *sb, const struct am_state *state, if (errno == ENOENT) return -1; - die_errno(_("could not read '%s'"), am_path(state, file)); + saved_errno = errno; + path = am_path(state, file); + errno = saved_errno; + die_errno(_("could not read '%s'"), path); } /** diff --git a/builtin/commit.c b/builtin/commit.c index 4610e3d8e3..39836b3734 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -780,8 +780,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix, } s->fp = fopen_for_writing(git_path_commit_editmsg()); - if (s->fp == NULL) - die_errno(_("could not open '%s'"), git_path_commit_editmsg()); + if (s->fp == NULL) { + int saved_errno = errno; + const char *path = git_path_commit_editmsg(); + errno = saved_errno; + die_errno(_("could not open '%s'"), path); + } /* Ignore status.displayCommentPrefix: we do need comments in COMMIT_EDITMSG. */ old_display_comment_prefix = s->display_comment_prefix; 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); + } } else { if (work_tree) 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.
Re: [PATCH v2 1/3] am: add --show-current-patch
Eric Sunshinewrites: > On Wed, Jan 31, 2018 at 4:30 AM, Nguyễn Thái Ngọc Duy > wrote: >> Pointing the user to $GIT_DIR/rebase-apply may encourage them to mess >> around in there, which is not a good thing. With this, the user does >> not have to keep the path around somewhere (because after a couple of >> commands, the path may be out of scrollback buffer) when they need to >> look at the patch. >> >> Signed-off-by: Nguyễn Thái Ngọc Duy >> --- >> diff --git a/builtin/am.c b/builtin/am.c >> @@ -2121,6 +2120,22 @@ static void am_abort(struct am_state *state) >> +static int show_patch(struct am_state *state) >> +{ >> + struct strbuf sb = STRBUF_INIT; >> + int len; >> + >> + len = strbuf_read_file(, am_path(state, msgnum(state)), 0); >> + if (len < 0) >> + die_errno(_("failed to read '%s'"), >> + am_path(state, msgnum(state))); > > Isn't this am_path() invocation inside die_errno() likely to clobber > the 'errno' from strbuf_read_file() which you want to be reporting? True. After coming up with the pathname to the current patch file, we are going to exit without ever calling am_path(), or underlying get_pathname() via mkpath(), again before exiting anyway, so perhaps it is sufficient to just do an am_path() just once upfront, feed it to strbuf_read_file() and also to die_errno().
Re: [PATCH v2 1/3] am: add --show-current-patch
On Wed, Jan 31, 2018 at 4:30 AM, Nguyễn Thái Ngọc Duywrote: > Pointing the user to $GIT_DIR/rebase-apply may encourage them to mess > around in there, which is not a good thing. With this, the user does > not have to keep the path around somewhere (because after a couple of > commands, the path may be out of scrollback buffer) when they need to > look at the patch. > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > diff --git a/builtin/am.c b/builtin/am.c > @@ -2121,6 +2120,22 @@ static void am_abort(struct am_state *state) > +static int show_patch(struct am_state *state) > +{ > + struct strbuf sb = STRBUF_INIT; > + int len; > + > + len = strbuf_read_file(, am_path(state, msgnum(state)), 0); > + if (len < 0) > + die_errno(_("failed to read '%s'"), > + am_path(state, msgnum(state))); Isn't this am_path() invocation inside die_errno() likely to clobber the 'errno' from strbuf_read_file() which you want to be reporting? > + setup_pager(); > + write_in_full(1, sb.buf, sb.len); > + strbuf_release(); > + return 0; > +}