On 03/08, René Scharfe wrote: > Am 08.03.2017 um 16:43 schrieb Johannes Schindelin: > > In 4ac9006f832 (real_path: have callers use real_pathdup and > > strbuf_realpath, 2016-12-12), we changed the xstrdup(real_path()) > > pattern to use real_pathdup() directly. > > > > The only problem with this change is that real_path() calls > > strbuf_realpath() with die_on_error = 1 while real_pathdup() calls it > > with die_on_error = 0. Meaning that in cases where real_path() causes > > Git to die() with an error message, real_pathdup() is silent and returns > > NULL instead. > > > > The callers, however, are ill-prepared for that change, as they expect > > the return value to be non-NULL. > > > > This patch fixes that by extending real_pathdup()'s signature to accept > > the die_on_error flag and simply pass it through to strbuf_realpath(), > > and then adjust all callers after a careful audit whether they would > > handle NULLs well. > > > > Note: this fix not only prevents NULL pointer accesses, but it also > > reintroduces the error messages that were lost with the change to > > real_pathdup(). > > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx> > > --- > > abspath.c | 4 ++-- > > builtin/init-db.c | 6 +++--- > > cache.h | 2 +- > > dir.c | 4 ++-- > > environment.c | 2 +- > > setup.c | 4 ++-- > > submodule.c | 10 +++++----- > > t/t1501-work-tree.sh | 2 +- > > worktree.c | 2 +- > > 9 files changed, 18 insertions(+), 18 deletions(-) > > > > diff --git a/abspath.c b/abspath.c > > index 2f0c26e0e2c..b02e068aa34 100644 > > --- a/abspath.c > > +++ b/abspath.c > > @@ -214,12 +214,12 @@ const char *real_path_if_valid(const char *path) > > return strbuf_realpath(&realpath, path, 0); > > } > > > > -char *real_pathdup(const char *path) > > +char *real_pathdup(const char *path, int die_on_error) > > Adding a gentle variant (with the current implementation) and making > real_pathdup() die on error would be nicer, as it doesn't require > callers to pass magic flag values. Most cases use the dying variant, > so such a patch would have to touch less places: I agree with Junio and Rene that a gentle version would make the api slightly nicer (and more consistant with some of the other api's we have in git). This is exactly what I should have done back when I originally made the change. Sorry for missing this! > --- > abspath.c | 7 +++++++ > cache.h | 1 + > setup.c | 2 +- > t/t1501-work-tree.sh | 2 +- > 4 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/abspath.c b/abspath.c > index 2f0c26e0e2..f3fcff8b1b 100644 > --- a/abspath.c > +++ b/abspath.c > @@ -217,6 +217,13 @@ const char *real_path_if_valid(const char *path) > char *real_pathdup(const char *path) > { > struct strbuf realpath = STRBUF_INIT; > + strbuf_realpath(&realpath, path, 1); > + return strbuf_detach(&realpath, NULL); > +} > + > +char *real_pathdup_gently(const char *path) > +{ > + struct strbuf realpath = STRBUF_INIT; > char *retval = NULL; > > if (strbuf_realpath(&realpath, path, 0)) > diff --git a/cache.h b/cache.h > index 80b6372cf7..9dfbce702e 100644 > --- a/cache.h > +++ b/cache.h > @@ -1154,6 +1154,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path, > const char *real_path(const char *path); > const char *real_path_if_valid(const char *path); > char *real_pathdup(const char *path); > +char *real_pathdup_gently(const char *path); > const char *absolute_path(const char *path); > char *absolute_pathdup(const char *path); > const char *remove_leading_path(const char *in, const char *prefix); > diff --git a/setup.c b/setup.c > index f14cbcd338..398ea8a913 100644 > --- a/setup.c > +++ b/setup.c > @@ -806,7 +806,7 @@ static int canonicalize_ceiling_entry(struct string_list_item *item, > /* Keep entry but do not canonicalize it */ > return 1; > } else { > - char *real_path = real_pathdup(ceil); > + char *real_path = real_pathdup_gently(ceil); > if (!real_path) { > return 0; > } > diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh > index 046d9b7909..b06210ec5e 100755 > --- a/t/t1501-work-tree.sh > +++ b/t/t1501-work-tree.sh > @@ -423,7 +423,7 @@ test_expect_success '$GIT_WORK_TREE overrides $GIT_DIR/common' ' > ) > ' > > -test_expect_failure 'error out gracefully on invalid $GIT_WORK_TREE' ' > +test_expect_success 'error out gracefully on invalid $GIT_WORK_TREE' ' > ( > GIT_WORK_TREE=/.invalid/work/tree && > export GIT_WORK_TREE && > -- > 2.12.0 > -- Brandon Williams