From b0a642ac4624740187212aecec62b35a2eab38cb Mon Sep 17 00:00:00 2001 From: Ramsay Jones Date: Mon, 8 May 2017 21:45:56 +0100 Subject: git_fopen: fix a sparse 'not declared' warning If git is built with the FREAD_READS_DIRECTORIES build variable set, this would cause sparse to issue a 'not declared, should it be static?' warning on Linux. This is a result of the method employed by 'compat/fopen.c' to suppress the (possible) redefinition of the (system) fopen macro, which also removes the extern declaration of the git_fopen function. In order to suppress the warning, introduce a new macro to suppress the definition (or possibly the re-definition) of the fopen symbol as a macro override. This new macro (SUPPRESS_FOPEN_REDEFINITION) is only defined in 'compat/fopen.c', just prior to the inclusion of the 'git-compat-util.h' header file. Signed-off-by: Ramsay Jones Signed-off-by: Junio C Hamano --- compat/fopen.c | 4 ++-- git-compat-util.h | 10 ++++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/compat/fopen.c b/compat/fopen.c index b5ca142fed..107b3e8182 100644 --- a/compat/fopen.c +++ b/compat/fopen.c @@ -1,14 +1,14 @@ /* * The order of the following two lines is important. * - * FREAD_READS_DIRECTORIES is undefined before including git-compat-util.h + * SUPPRESS_FOPEN_REDEFINITION is defined before including git-compat-util.h * to avoid the redefinition of fopen within git-compat-util.h. This is * necessary since fopen is a macro on some platforms which may be set * based on compiler options. For example, on AIX fopen is set to fopen64 * when _LARGE_FILES is defined. The previous technique of merely undefining * fopen after including git-compat-util.h is inadequate in this case. */ -#undef FREAD_READS_DIRECTORIES +#define SUPPRESS_FOPEN_REDEFINITION #include "../git-compat-util.h" FILE *git_fopen(const char *path, const char *mode) diff --git a/git-compat-util.h b/git-compat-util.h index bd04564a69..6be55cf8b3 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -689,10 +689,12 @@ char *gitstrdup(const char *s); #endif #ifdef FREAD_READS_DIRECTORIES -#ifdef fopen -#undef fopen -#endif -#define fopen(a,b) git_fopen(a,b) +# if !defined(SUPPRESS_FOPEN_REDEFINITION) +# ifdef fopen +# undef fopen +# endif +# define fopen(a,b) git_fopen(a,b) +# endif extern FILE *git_fopen(const char*, const char*); #endif -- cgit v1.2.3 From 23a9e0712d76a63c5af1caeb816943f466f0300e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?= Date: Wed, 3 May 2017 17:16:46 +0700 Subject: use xfopen() in more places MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit xfopen() - provides error details - explains error on reading, or writing, or whatever operation - has l10n support - prints file name in the error Some of these are missing in the places that are replaced with xfopen(), which is a clear win. In some other places, it's just less code (not as clearly a win as the previous case but still is). The only slight regresssion is in remote-testsvn, where we don't report the file class (marks files) in the error messages anymore. But since this is a _test_ svn remote transport, I'm not too concerned. Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: Junio C Hamano --- bisect.c | 5 +---- builtin/am.c | 8 ++------ builtin/commit.c | 5 +---- builtin/fast-export.c | 4 +--- builtin/fsck.c | 3 +-- builtin/merge.c | 4 +--- builtin/pull.c | 3 +-- diff.c | 8 ++------ fast-import.c | 4 +--- remote-testsvn.c | 8 ++------ 10 files changed, 13 insertions(+), 39 deletions(-) diff --git a/bisect.c b/bisect.c index 08c9fb7266..469a3e9061 100644 --- a/bisect.c +++ b/bisect.c @@ -438,10 +438,7 @@ static void read_bisect_paths(struct argv_array *array) { struct strbuf str = STRBUF_INIT; const char *filename = git_path_bisect_names(); - FILE *fp = fopen(filename, "r"); - - if (!fp) - die_errno(_("Could not open file '%s'"), filename); + FILE *fp = xfopen(filename, "r"); while (strbuf_getline_lf(&str, fp) != EOF) { strbuf_trim(&str); diff --git a/builtin/am.c b/builtin/am.c index a95dd8b4e6..f5dac7783e 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1275,12 +1275,8 @@ static int parse_mail(struct am_state *state, const char *mail) die("BUG: invalid value for state->scissors"); } - mi.input = fopen(mail, "r"); - if (!mi.input) - die("could not open input"); - mi.output = fopen(am_path(state, "info"), "w"); - if (!mi.output) - die("could not open output 'info'"); + mi.input = xfopen(mail, "r"); + mi.output = xfopen(am_path(state, "info"), "w"); if (mailinfo(&mi, am_path(state, "msg"), am_path(state, "patch"))) die("could not parse patch"); diff --git a/builtin/commit.c b/builtin/commit.c index 1d805f5da8..eda0d32311 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1695,10 +1695,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) if (!reflog_msg) reflog_msg = "commit (merge)"; pptr = commit_list_append(current_head, pptr); - fp = fopen(git_path_merge_head(), "r"); - if (fp == NULL) - die_errno(_("could not open '%s' for reading"), - git_path_merge_head()); + fp = xfopen(git_path_merge_head(), "r"); while (strbuf_getline_lf(&m, fp) != EOF) { struct commit *parent; diff --git a/builtin/fast-export.c b/builtin/fast-export.c index e0220630d0..128b99e6da 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -905,9 +905,7 @@ static void export_marks(char *file) static void import_marks(char *input_file) { char line[512]; - FILE *f = fopen(input_file, "r"); - if (!f) - die_errno("cannot read '%s'", input_file); + FILE *f = xfopen(input_file, "r"); while (fgets(line, sizeof(line), f)) { uint32_t mark; diff --git a/builtin/fsck.c b/builtin/fsck.c index b5e13a4556..00beaaa4e6 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -280,8 +280,7 @@ static void check_unreachable_object(struct object *obj) free(filename); return; } - if (!(f = fopen(filename, "w"))) - die_errno("Could not open '%s'", filename); + f = xfopen(filename, "w"); if (obj->type == OBJ_BLOB) { if (stream_blob_to_fd(fileno(f), &obj->oid, NULL, 1)) die_errno("Could not write '%s'", filename); diff --git a/builtin/merge.c b/builtin/merge.c index 703827f006..65a1501858 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -839,9 +839,7 @@ static int suggest_conflicts(void) struct strbuf msgbuf = STRBUF_INIT; filename = git_path_merge_msg(); - fp = fopen(filename, "a"); - if (!fp) - die_errno(_("Could not open '%s' for writing"), filename); + fp = xfopen(filename, "a"); append_conflicts_hint(&msgbuf); fputs(msgbuf.buf, fp); diff --git a/builtin/pull.c b/builtin/pull.c index dd1a4a94e4..589c25becf 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -337,8 +337,7 @@ static void get_merge_heads(struct oid_array *merge_heads) struct strbuf sb = STRBUF_INIT; struct object_id oid; - if (!(fp = fopen(filename, "r"))) - die_errno(_("could not open '%s' for reading"), filename); + fp = xfopen(filename, "r"); while (strbuf_getline_lf(&sb, fp) != EOF) { if (get_oid_hex(sb.buf, &oid)) continue; /* invalid line: does not start with SHA1 */ diff --git a/diff.c b/diff.c index 11eef1c85d..b6597ce568 100644 --- a/diff.c +++ b/diff.c @@ -4071,9 +4071,7 @@ int diff_opt_parse(struct diff_options *options, DIFF_OPT_CLR(options, FUNCCONTEXT); else if ((argcount = parse_long_opt("output", av, &optarg))) { char *path = prefix_filename(prefix, optarg); - options->file = fopen(path, "w"); - if (!options->file) - die_errno("Could not open '%s'", path); + options->file = xfopen(path, "w"); options->close_file = 1; if (options->use_color != GIT_COLOR_ALWAYS) options->use_color = GIT_COLOR_NEVER; @@ -4807,9 +4805,7 @@ void diff_flush(struct diff_options *options) */ if (options->close_file) fclose(options->file); - options->file = fopen("/dev/null", "w"); - if (!options->file) - die_errno("Could not open /dev/null"); + options->file = xfopen("/dev/null", "w"); options->close_file = 1; for (i = 0; i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; diff --git a/fast-import.c b/fast-import.c index cf58f875b8..420b3a00d3 100644 --- a/fast-import.c +++ b/fast-import.c @@ -3274,9 +3274,7 @@ static void option_export_pack_edges(const char *edges) { if (pack_edges) fclose(pack_edges); - pack_edges = fopen(edges, "a"); - if (!pack_edges) - die_errno("Cannot open '%s'", edges); + pack_edges = xfopen(edges, "a"); } static int parse_one_option(const char *option) diff --git a/remote-testsvn.c b/remote-testsvn.c index f87bf851ba..50404ef343 100644 --- a/remote-testsvn.c +++ b/remote-testsvn.c @@ -124,10 +124,8 @@ static int note2mark_cb(const unsigned char *object_sha1, static void regenerate_marks(void) { int ret; - FILE *marksfile = fopen(marksfilename, "w+"); + FILE *marksfile = xfopen(marksfilename, "w+"); - if (!marksfile) - die_errno("Couldn't create mark file %s.", marksfilename); ret = for_each_note(NULL, 0, note2mark_cb, marksfile); if (ret) die("Regeneration of marks failed, returned %d.", ret); @@ -148,9 +146,7 @@ static void check_or_regenerate_marks(int latestrev) marksfile = fopen(marksfilename, "r"); if (!marksfile) { regenerate_marks(); - marksfile = fopen(marksfilename, "r"); - if (!marksfile) - die_errno("cannot read marks file %s!", marksfilename); + marksfile = xfopen(marksfilename, "r"); fclose(marksfile); } else { strbuf_addf(&sb, ":%d ", latestrev); -- cgit v1.2.3 From 02912f477586befaf46e0db4f7c47b52081d5b6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?= Date: Wed, 3 May 2017 17:16:47 +0700 Subject: clone: use xfopen() instead of fopen() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit copy_alternates() called fopen() without handling errors. By switching to xfopen(), this bug is fixed. Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: Junio C Hamano --- builtin/clone.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/clone.c b/builtin/clone.c index de85b85254..dde4fe73af 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -357,7 +357,7 @@ static void copy_alternates(struct strbuf *src, struct strbuf *dst, * to turn entries with paths relative to the original * absolute, so that they can be used in the new repository. */ - FILE *in = fopen(src->buf, "r"); + FILE *in = xfopen(src->buf, "r"); struct strbuf line = STRBUF_INIT; while (strbuf_getline(&line, in) != EOF) { -- cgit v1.2.3 From e2d90fd1c33ae57e4a6da5729ae53876107b3463 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?= Date: Wed, 3 May 2017 17:16:48 +0700 Subject: config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This variable is added [1] with the assumption that on a sane system, fopen(, "r") should return NULL. Linux and FreeBSD do not meet this expectation while at least Windows and AIX do. Let's make sure they behave the same way. I only tested one version on Linux (4.7.0 with glibc 2.22) and FreeBSD (11.0) but since GNU/kFreeBSD is fbsd kernel with gnu userspace, I'm pretty sure it shares the same problem. [1] cba22528fa (Add compat/fopen.c which returns NULL on attempt to open directory - 2008-02-08) Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: Junio C Hamano --- config.mak.uname | 3 +++ t/t1308-config-set.sh | 8 ++++++++ 2 files changed, 11 insertions(+) diff --git a/config.mak.uname b/config.mak.uname index 399fe19271..a25ffddb3e 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -36,6 +36,7 @@ ifeq ($(uname_S),Linux) NEEDS_LIBRT = YesPlease HAVE_GETDELIM = YesPlease SANE_TEXT_GREP=-a + FREAD_READS_DIRECTORIES = UnfortunatelyYes endif ifeq ($(uname_S),GNU/kFreeBSD) HAVE_ALLOCA_H = YesPlease @@ -43,6 +44,7 @@ ifeq ($(uname_S),GNU/kFreeBSD) HAVE_PATHS_H = YesPlease DIR_HAS_BSD_GROUP_SEMANTICS = YesPlease LIBC_CONTAINS_LIBINTL = YesPlease + FREAD_READS_DIRECTORIES = UnfortunatelyYes endif ifeq ($(uname_S),UnixWare) CC = cc @@ -201,6 +203,7 @@ ifeq ($(uname_S),FreeBSD) GMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes HAVE_BSD_SYSCTL = YesPlease PAGER_ENV = LESS=FRX LV=-c MORE=FRX + FREAD_READS_DIRECTORIES = UnfortunatelyYes endif ifeq ($(uname_S),OpenBSD) NO_STRCASESTR = YesPlease diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh index ff50960cca..72d5f1f931 100755 --- a/t/t1308-config-set.sh +++ b/t/t1308-config-set.sh @@ -183,6 +183,14 @@ test_expect_success 'proper error on non-existent files' ' test_cmp expect actual ' +test_expect_success 'proper error on directory "files"' ' + echo "Error (-1) reading configuration file a-directory." >expect && + mkdir a-directory && + test_expect_code 2 test-config configset_get_value foo.bar a-directory 2>output && + grep "^Error" output >actual && + test_cmp expect actual +' + test_expect_success POSIXPERM,SANITY 'proper error on non-accessible files' ' chmod -r .git/config && test_when_finished "chmod +r .git/config" && -- cgit v1.2.3 From 8e178ec4d072da4cd8f4449e17aef3aff5b57f6a Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 9 May 2017 21:44:33 -0700 Subject: config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too Signed-off-by: Junio C Hamano --- config.mak.uname | 1 + 1 file changed, 1 insertion(+) diff --git a/config.mak.uname b/config.mak.uname index a25ffddb3e..1743890164 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -110,6 +110,7 @@ ifeq ($(uname_S),Darwin) BASIC_CFLAGS += -DPRECOMPOSE_UNICODE BASIC_CFLAGS += -DPROTECT_HFS_DEFAULT=1 HAVE_BSD_SYSCTL = YesPlease + FREAD_READS_DIRECTORIES = UnfortunatelyYes endif ifeq ($(uname_S),SunOS) NEEDS_SOCKET = YesPlease -- cgit v1.2.3 From 11dc1fcb3fa53f5a46486daa7cb38ed387153f2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?= Date: Wed, 3 May 2017 17:16:49 +0700 Subject: wrapper.c: add and use warn_on_fopen_errors() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In many places, Git warns about an inaccessible file after a fopen() failed. To discern these cases from other cases where we want to warn about inaccessible files, introduce a new helper specifically to test whether fopen() failed because the current user lacks the permission to open file in question. Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: Junio C Hamano --- config.c | 3 +++ dir.c | 6 +++--- git-compat-util.h | 2 ++ t/t1308-config-set.sh | 3 ++- wrapper.c | 10 ++++++++++ 5 files changed, 20 insertions(+), 4 deletions(-) diff --git a/config.c b/config.c index b4a3205da3..2894fbb6d0 100644 --- a/config.c +++ b/config.c @@ -2640,6 +2640,9 @@ int git_config_rename_section_in_file(const char *config_filename, } if (!(config_file = fopen(config_filename, "rb"))) { + ret = warn_on_fopen_errors(config_filename); + if (ret) + goto out; /* no config file means nothing to rename, no error */ goto commit_and_out; } diff --git a/dir.c b/dir.c index f451bfa48c..be616e884e 100644 --- a/dir.c +++ b/dir.c @@ -745,9 +745,9 @@ static int add_excludes(const char *fname, const char *base, int baselen, fd = open(fname, O_RDONLY); if (fd < 0 || fstat(fd, &st) < 0) { - if (errno != ENOENT) - warn_on_inaccessible(fname); - if (0 <= fd) + if (fd < 0) + warn_on_fopen_errors(fname); + else close(fd); if (!check_index || (buf = read_skip_worktree_file_from_index(fname, &size, sha1_stat)) == NULL) diff --git a/git-compat-util.h b/git-compat-util.h index 6be55cf8b3..eb5c18c7fd 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -1101,6 +1101,8 @@ int access_or_die(const char *path, int mode, unsigned flag); /* Warn on an inaccessible file that ought to be accessible */ void warn_on_inaccessible(const char *path); +/* Warn on an inaccessible file if errno indicates this is an error */ +int warn_on_fopen_errors(const char *path); #ifdef GMTIME_UNRELIABLE_ERRORS struct tm *git_gmtime(const time_t *); diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh index 72d5f1f931..df92fdcd40 100755 --- a/t/t1308-config-set.sh +++ b/t/t1308-config-set.sh @@ -195,7 +195,8 @@ test_expect_success POSIXPERM,SANITY 'proper error on non-accessible files' ' chmod -r .git/config && test_when_finished "chmod +r .git/config" && echo "Error (-1) reading configuration file .git/config." >expect && - test_expect_code 2 test-config configset_get_value foo.bar .git/config 2>actual && + test_expect_code 2 test-config configset_get_value foo.bar .git/config 2>output && + grep "^Error" output >actual && test_cmp expect actual ' diff --git a/wrapper.c b/wrapper.c index d837417709..20c25e7e65 100644 --- a/wrapper.c +++ b/wrapper.c @@ -418,6 +418,16 @@ FILE *fopen_for_writing(const char *path) return ret; } +int warn_on_fopen_errors(const char *path) +{ + if (errno != ENOENT && errno != ENOTDIR) { + warn_on_inaccessible(path); + return -1; + } + + return 0; +} + int xmkstemp(char *template) { int fd; -- cgit v1.2.3 From e9d983f116c7de43f40a49aae60ebfe107f153ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?= Date: Wed, 3 May 2017 17:16:50 +0700 Subject: wrapper.c: add and use fopen_or_warn() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When fopen() returns NULL, it could be because the given path does not exist, but it could also be some other errors and the caller has to check. Add a wrapper so we don't have to repeat the same error check everywhere. Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: Junio C Hamano --- attr.c | 7 ++----- bisect.c | 2 +- builtin/blame.c | 2 +- commit.c | 2 +- config.c | 2 +- git-compat-util.h | 1 + ident.c | 8 +++----- remote.c | 4 ++-- rerere.c | 2 +- sequencer.c | 8 ++++---- server-info.c | 2 +- t/t1308-config-set.sh | 2 ++ t/t5512-ls-remote.sh | 13 ++++++++++--- wrapper.c | 11 +++++++++++ wt-status.c | 3 ++- 15 files changed, 43 insertions(+), 26 deletions(-) diff --git a/attr.c b/attr.c index 7e2134471c..821203e2a9 100644 --- a/attr.c +++ b/attr.c @@ -720,16 +720,13 @@ void git_attr_set_direction(enum git_attr_direction new_direction, static struct attr_stack *read_attr_from_file(const char *path, int macro_ok) { - FILE *fp = fopen(path, "r"); + FILE *fp = fopen_or_warn(path, "r"); struct attr_stack *res; char buf[2048]; int lineno = 0; - if (!fp) { - if (errno != ENOENT && errno != ENOTDIR) - warn_on_inaccessible(path); + if (!fp) return NULL; - } res = xcalloc(1, sizeof(*res)); while (fgets(buf, sizeof(buf), fp)) { char *bufp = buf; diff --git a/bisect.c b/bisect.c index 469a3e9061..bb28bf63b2 100644 --- a/bisect.c +++ b/bisect.c @@ -666,7 +666,7 @@ static int is_expected_rev(const struct object_id *oid) if (stat(filename, &st) || !S_ISREG(st.st_mode)) return 0; - fp = fopen(filename, "r"); + fp = fopen_or_warn(filename, "r"); if (!fp) return 0; diff --git a/builtin/blame.c b/builtin/blame.c index 07506a3e45..34445d7894 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -2071,7 +2071,7 @@ static int prepare_lines(struct scoreboard *sb) */ static int read_ancestry(const char *graft_file) { - FILE *fp = fopen(graft_file, "r"); + FILE *fp = fopen_or_warn(graft_file, "r"); struct strbuf buf = STRBUF_INIT; if (!fp) return -1; diff --git a/commit.c b/commit.c index 73c78c2b80..3eeda081f9 100644 --- a/commit.c +++ b/commit.c @@ -167,7 +167,7 @@ bad_graft_data: static int read_graft_file(const char *graft_file) { - FILE *fp = fopen(graft_file, "r"); + FILE *fp = fopen_or_warn(graft_file, "r"); struct strbuf buf = STRBUF_INIT; if (!fp) return -1; diff --git a/config.c b/config.c index 2894fbb6d0..e54d99d519 100644 --- a/config.c +++ b/config.c @@ -1422,7 +1422,7 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data) int ret = -1; FILE *f; - f = fopen(filename, "r"); + f = fopen_or_warn(filename, "r"); if (f) { flockfile(f); ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename, filename, f, data); diff --git a/git-compat-util.h b/git-compat-util.h index eb5c18c7fd..f74b401810 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -802,6 +802,7 @@ extern int xmkstemp(char *template); extern int xmkstemp_mode(char *template, int mode); extern char *xgetcwd(void); extern FILE *fopen_for_writing(const char *path); +extern FILE *fopen_or_warn(const char *path, const char *mode); #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc))) #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), (alloc))) diff --git a/ident.c b/ident.c index bea871c8e0..91c7609055 100644 --- a/ident.c +++ b/ident.c @@ -72,12 +72,10 @@ static int add_mailname_host(struct strbuf *buf) FILE *mailname; struct strbuf mailnamebuf = STRBUF_INIT; - mailname = fopen("/etc/mailname", "r"); - if (!mailname) { - if (errno != ENOENT) - warning_errno("cannot open /etc/mailname"); + mailname = fopen_or_warn("/etc/mailname", "r"); + if (!mailname) return -1; - } + if (strbuf_getline(&mailnamebuf, mailname) == EOF) { if (ferror(mailname)) warning_errno("cannot read /etc/mailname"); diff --git a/remote.c b/remote.c index 801137c72e..1f2453d0f6 100644 --- a/remote.c +++ b/remote.c @@ -251,7 +251,7 @@ static const char *skip_spaces(const char *s) static void read_remotes_file(struct remote *remote) { struct strbuf buf = STRBUF_INIT; - FILE *f = fopen(git_path("remotes/%s", remote->name), "r"); + FILE *f = fopen_or_warn(git_path("remotes/%s", remote->name), "r"); if (!f) return; @@ -277,7 +277,7 @@ static void read_branches_file(struct remote *remote) { char *frag; struct strbuf buf = STRBUF_INIT; - FILE *f = fopen(git_path("branches/%s", remote->name), "r"); + FILE *f = fopen_or_warn(git_path("branches/%s", remote->name), "r"); if (!f) return; diff --git a/rerere.c b/rerere.c index 3bd55caf3b..971bfedfb2 100644 --- a/rerere.c +++ b/rerere.c @@ -200,7 +200,7 @@ static struct rerere_id *new_rerere_id(unsigned char *sha1) static void read_rr(struct string_list *rr) { struct strbuf buf = STRBUF_INIT; - FILE *in = fopen(git_path_merge_rr(), "r"); + FILE *in = fopen_or_warn(git_path_merge_rr(), "r"); if (!in) return; diff --git a/sequencer.c b/sequencer.c index 10c3b4ff81..11b5c7c114 100644 --- a/sequencer.c +++ b/sequencer.c @@ -897,8 +897,8 @@ static void flush_rewritten_pending(void) { FILE *out; if (strbuf_read_file(&buf, rebase_path_rewritten_pending(), 82) > 0 && - !get_sha1("HEAD", newsha1) && - (out = fopen(rebase_path_rewritten_list(), "a"))) { + !get_sha1("HEAD", newsha1) && + (out = fopen_or_warn(rebase_path_rewritten_list(), "a"))) { char *bol = buf.buf, *eol; while (*bol) { @@ -917,7 +917,7 @@ static void flush_rewritten_pending(void) { static void record_in_rewritten(struct object_id *oid, enum todo_command next_command) { - FILE *out = fopen(rebase_path_rewritten_pending(), "a"); + FILE *out = fopen_or_warn(rebase_path_rewritten_pending(), "a"); if (!out) return; @@ -1378,7 +1378,7 @@ static int read_populate_todo(struct todo_list *todo_list, if (is_rebase_i(opts)) { struct todo_list done = TODO_LIST_INIT; - FILE *f = fopen(rebase_path_msgtotal(), "w"); + FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w"); if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 && !parse_insn_buffer(done.buf.buf, &done)) diff --git a/server-info.c b/server-info.c index f6c1a3dfb0..e01ac154a8 100644 --- a/server-info.c +++ b/server-info.c @@ -133,7 +133,7 @@ static int read_pack_info_file(const char *infofile) char line[1000]; int old_cnt = 0; - fp = fopen(infofile, "r"); + fp = fopen_or_warn(infofile, "r"); if (!fp) return 1; /* nonexistent is not an error. */ diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh index df92fdcd40..e495a61616 100755 --- a/t/t1308-config-set.sh +++ b/t/t1308-config-set.sh @@ -187,6 +187,7 @@ test_expect_success 'proper error on directory "files"' ' echo "Error (-1) reading configuration file a-directory." >expect && mkdir a-directory && test_expect_code 2 test-config configset_get_value foo.bar a-directory 2>output && + grep "^warning:" output && grep "^Error" output >actual && test_cmp expect actual ' @@ -196,6 +197,7 @@ test_expect_success POSIXPERM,SANITY 'proper error on non-accessible files' ' test_when_finished "chmod +r .git/config" && echo "Error (-1) reading configuration file .git/config." >expect && test_expect_code 2 test-config configset_get_value foo.bar .git/config 2>output && + grep "^warning:" output && grep "^Error" output >actual && test_cmp expect actual ' diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh index 94fc9be9ce..02106c9226 100755 --- a/t/t5512-ls-remote.sh +++ b/t/t5512-ls-remote.sh @@ -85,8 +85,15 @@ test_expect_success 'use branch..remote if possible' ' ' test_expect_success 'confuses pattern as remote when no remote specified' ' - cat >exp <<-\EOF && - fatal: '\''refs*master'\'' does not appear to be a git repository + if test_have_prereq MINGW + then + # Windows does not like asterisks in pathname + does_not_exist=master + else + does_not_exist="refs*master" + fi && + cat >exp <<-EOF && + fatal: '\''$does_not_exist'\'' does not appear to be a git repository fatal: Could not read from remote repository. Please make sure you have the correct access rights @@ -98,7 +105,7 @@ test_expect_success 'confuses pattern as remote when no remote specified' ' # fetch . # We could just as easily have used "master"; the "*" emphasizes its # role as a pattern. - test_must_fail git ls-remote refs*master >actual 2>&1 && + test_must_fail git ls-remote "$does_not_exist" >actual 2>&1 && test_i18ncmp exp actual ' diff --git a/wrapper.c b/wrapper.c index 20c25e7e65..6e513c904a 100644 --- a/wrapper.c +++ b/wrapper.c @@ -428,6 +428,17 @@ int warn_on_fopen_errors(const char *path) return 0; } +FILE *fopen_or_warn(const char *path, const char *mode) +{ + FILE *fp = fopen(path, mode); + + if (fp) + return fp; + + warn_on_fopen_errors(path); + return NULL; +} + int xmkstemp(char *template) { int fd; diff --git a/wt-status.c b/wt-status.c index 0375484962..cdf9f5eed2 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1065,7 +1065,8 @@ static void show_am_in_progress(struct wt_status *s, static char *read_line_from_git_path(const char *filename) { struct strbuf buf = STRBUF_INIT; - FILE *fp = fopen(git_path("%s", filename), "r"); + FILE *fp = fopen_or_warn(git_path("%s", filename), "r"); + if (!fp) { strbuf_release(&buf); return NULL; -- cgit v1.2.3 From 382fb07f7bbb1f94fa3e1293d5df151725a886a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?= Date: Mon, 8 May 2017 17:40:37 +0700 Subject: wrapper.c: make warn_on_inaccessible() static After the last patch, this function is not used outside anymore. Keep it static. Noticed-by: Ramsay Jones Signed-off-by: Junio C Hamano --- git-compat-util.h | 2 -- wrapper.c | 10 +++++----- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/git-compat-util.h b/git-compat-util.h index f74b401810..87f47828a5 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -1100,8 +1100,6 @@ int remove_or_warn(unsigned int mode, const char *path); int access_or_warn(const char *path, int mode, unsigned flag); int access_or_die(const char *path, int mode, unsigned flag); -/* Warn on an inaccessible file that ought to be accessible */ -void warn_on_inaccessible(const char *path); /* Warn on an inaccessible file if errno indicates this is an error */ int warn_on_fopen_errors(const char *path); diff --git a/wrapper.c b/wrapper.c index 6e513c904a..b117eb14a4 100644 --- a/wrapper.c +++ b/wrapper.c @@ -418,6 +418,11 @@ FILE *fopen_for_writing(const char *path) return ret; } +static void warn_on_inaccessible(const char *path) +{ + warning_errno(_("unable to access '%s'"), path); +} + int warn_on_fopen_errors(const char *path) { if (errno != ENOENT && errno != ENOTDIR) { @@ -597,11 +602,6 @@ int remove_or_warn(unsigned int mode, const char *file) return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file); } -void warn_on_inaccessible(const char *path) -{ - warning_errno(_("unable to access '%s'"), path); -} - static int access_error_is_ok(int err, unsigned flag) { return err == ENOENT || err == ENOTDIR || -- cgit v1.2.3 From 5118d7f4e6e00b7eac3ce08a16392a732edc0b2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?= Date: Wed, 3 May 2017 17:16:55 +0700 Subject: print errno when reporting a system call error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: Junio C Hamano --- builtin/log.c | 3 ++- rerere.c | 4 ++-- xdiff-interface.c | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index b3b10cc1ed..26d6a3cf14 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -858,7 +858,8 @@ static int open_next_file(struct commit *commit, const char *subject, printf("%s\n", filename.buf + outdir_offset); if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL) - return error(_("Cannot open patch file %s"), filename.buf); + return error_errno(_("Cannot open patch file %s"), + filename.buf); strbuf_release(&filename); return 0; diff --git a/rerere.c b/rerere.c index 971bfedfb2..1351b0c3fb 100644 --- a/rerere.c +++ b/rerere.c @@ -484,13 +484,13 @@ static int handle_file(const char *path, unsigned char *sha1, const char *output io.input = fopen(path, "r"); io.io.wrerror = 0; if (!io.input) - return error("Could not open %s", path); + return error_errno("Could not open %s", path); if (output) { io.io.output = fopen(output, "w"); if (!io.io.output) { fclose(io.input); - return error("Could not write %s", output); + return error_errno("Could not write %s", output); } } diff --git a/xdiff-interface.c b/xdiff-interface.c index 060038c2d6..d3f78ca2a7 100644 --- a/xdiff-interface.c +++ b/xdiff-interface.c @@ -164,9 +164,9 @@ int read_mmfile(mmfile_t *ptr, const char *filename) size_t sz; if (stat(filename, &st)) - return error("Could not stat %s", filename); + return error_errno("Could not stat %s", filename); if ((f = fopen(filename, "rb")) == NULL) - return error("Could not open %s", filename); + return error_errno("Could not open %s", filename); sz = xsize_t(st.st_size); ptr->ptr = xmalloc(sz ? sz : 1); if (sz && fread(ptr->ptr, sz, 1, f) != 1) { -- cgit v1.2.3 From f7566f073fccafdd9e0ace514b25897dd55d217a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?= Date: Tue, 9 May 2017 17:11:33 +0700 Subject: rerere.c: move error_errno() closer to the source system call We are supposed to report errno from fopen(). fclose() between fopen() and the report function could either change errno or reset it. Signed-off-by: Junio C Hamano --- rerere.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rerere.c b/rerere.c index 1351b0c3fb..c26c29f87a 100644 --- a/rerere.c +++ b/rerere.c @@ -489,8 +489,9 @@ static int handle_file(const char *path, unsigned char *sha1, const char *output if (output) { io.io.output = fopen(output, "w"); if (!io.io.output) { + error_errno("Could not write %s", output); fclose(io.input); - return error_errno("Could not write %s", output); + return -1; } } -- cgit v1.2.3 From 15d980a785f8c962bc032df40cb42fdc269c9dc6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?= Date: Wed, 3 May 2017 17:16:56 +0700 Subject: log: fix memory leak in open_next_file() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Noticed-by: Jeff King Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: Junio C Hamano --- builtin/log.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index 26d6a3cf14..f075838df9 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -842,8 +842,10 @@ static int open_next_file(struct commit *commit, const char *subject, if (output_directory) { strbuf_addstr(&filename, output_directory); if (filename.len >= - PATH_MAX - FORMAT_PATCH_NAME_MAX - suffix_len) + PATH_MAX - FORMAT_PATCH_NAME_MAX - suffix_len) { + strbuf_release(&filename); return error(_("name of output directory is too long")); + } strbuf_complete(&filename, '/'); } @@ -857,9 +859,11 @@ static int open_next_file(struct commit *commit, const char *subject, if (!quiet) printf("%s\n", filename.buf + outdir_offset); - if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL) - return error_errno(_("Cannot open patch file %s"), - filename.buf); + if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL) { + error_errno(_("Cannot open patch file %s"), filename.buf); + strbuf_release(&filename); + return -1; + } strbuf_release(&filename); return 0; -- cgit v1.2.3 From 13b57da83384e9523943ea9d8ecf3ec7993ce56a Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 29 May 2017 22:25:25 +0200 Subject: mingw: verify that paths are not mistaken for remote nicknames MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This added test case simply verifies that users will not be bothered with bogus complaints à la warning: unable to access '.git/remotes/D:\repo': Invalid argument when fetching from a Windows path (in this case, D:\repo). [j6t: mark the new test as test_expect_failure] Signed-off-by: Johannes Schindelin Signed-off-by: Johannes Sixt Signed-off-by: Junio C Hamano --- t/t5580-clone-push-unc.sh | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh index b195f71ea9..944730cddc 100755 --- a/t/t5580-clone-push-unc.sh +++ b/t/t5580-clone-push-unc.sh @@ -1,10 +1,10 @@ #!/bin/sh -test_description='various UNC path tests (Windows-only)' +test_description='various Windows-only path tests' . ./test-lib.sh if ! test_have_prereq MINGW; then - skip_all='skipping UNC path tests, requires Windows' + skip_all='skipping Windows-only path tests' test_done fi @@ -45,4 +45,10 @@ test_expect_success push ' test "$rev" = "$(git rev-parse --verify refs/heads/to-push)" ' +test_expect_failure 'remote nick cannot contain backslashes' ' + BACKSLASHED="$(pwd | tr / \\\\)" && + git ls-remote "$BACKSLASHED" >out 2>err && + test_i18ngrep ! "unable to access" err +' + test_done -- cgit v1.2.3 From e5b313442ab7c700d0851e9dbe7d2b029e3893e5 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Mon, 29 May 2017 22:27:35 +0200 Subject: mingw_fopen: report ENOENT for invalid file names On Windows, certain characters are prohibited in file names, most prominently the colon. When fopen() is called with such an invalid file name, the underlying Windows API actually reports a particular error, but since there is no suitable errno value, this error is translated to EINVAL. Detect the case and report ENOENT instead. Signed-off-by: Johannes Sixt Signed-off-by: Junio C Hamano --- compat/mingw.c | 2 ++ t/t5580-clone-push-unc.sh | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/compat/mingw.c b/compat/mingw.c index 3fbfda5978..b604a827e8 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -423,6 +423,8 @@ FILE *mingw_fopen (const char *filename, const char *otype) return NULL; } file = _wfopen(wfilename, wotype); + if (!file && GetLastError() == ERROR_INVALID_NAME) + errno = ENOENT; if (file && hide && set_hidden_flag(wfilename, 1)) warning("could not mark '%s' as hidden.", filename); return file; diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh index 944730cddc..b322c2f722 100755 --- a/t/t5580-clone-push-unc.sh +++ b/t/t5580-clone-push-unc.sh @@ -45,7 +45,7 @@ test_expect_success push ' test "$rev" = "$(git rev-parse --verify refs/heads/to-push)" ' -test_expect_failure 'remote nick cannot contain backslashes' ' +test_expect_success 'remote nick cannot contain backslashes' ' BACKSLASHED="$(pwd | tr / \\\\)" && git ls-remote "$BACKSLASHED" >out 2>err && test_i18ngrep ! "unable to access" err -- cgit v1.2.3