* [PATCH v2 01/21] Use xfopen() in more places
2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16 ` Nguyễn Thái Ngọc Duy
2017-05-03 10:16 ` [PATCH v2 02/21] clone: use xfopen() instead of fopen() Nguyễn Thái Ngọc Duy
` (20 subsequent siblings)
21 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
Nguyễn Thái Ngọc Duy
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 <pclouds@gmail.com>
---
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);
--
2.11.0.157.gd943d85
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH v2 02/21] clone: use xfopen() instead of fopen()
2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
2017-05-03 10:16 ` [PATCH v2 01/21] Use xfopen() in more places Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16 ` Nguyễn Thái Ngọc Duy
2017-05-03 15:02 ` Johannes Schindelin
2017-05-03 10:16 ` [PATCH v2 03/21] config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD Nguyễn Thái Ngọc Duy
` (19 subsequent siblings)
21 siblings, 1 reply; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
Nguyễn Thái Ngọc Duy
This code uses the result FILE pointer right away without the NULL
check. Let's use xfopen() and die if we could not open the file. That's
still better than crashing,
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
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) {
--
2.11.0.157.gd943d85
^ permalink raw reply related [flat|nested] 70+ messages in thread
* Re: [PATCH v2 02/21] clone: use xfopen() instead of fopen()
2017-05-03 10:16 ` [PATCH v2 02/21] clone: use xfopen() instead of fopen() Nguyễn Thái Ngọc Duy
@ 2017-05-03 15:02 ` Johannes Schindelin
0 siblings, 0 replies; 70+ messages in thread
From: Johannes Schindelin @ 2017-05-03 15:02 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, Jeff King, Johannes Sixt
[-- Attachment #1: Type: text/plain, Size: 508 bytes --]
Hi Duy,
On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:
> This code uses the result FILE pointer right away without the NULL
> check. Let's use xfopen() and die if we could not open the file. That's
> still better than crashing,
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
While funny to read, maybe it would be better to spell it out clearly:
copy_alternates() called fopen() without handling errors. By
switching to xfopen(), this bug is fixed.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 03/21] config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD
2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
2017-05-03 10:16 ` [PATCH v2 01/21] Use xfopen() in more places Nguyễn Thái Ngọc Duy
2017-05-03 10:16 ` [PATCH v2 02/21] clone: use xfopen() instead of fopen() Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16 ` Nguyễn Thái Ngọc Duy
2017-05-03 10:16 ` [PATCH v2 04/21] wrapper.c: add warn_on_fopen_errors() Nguyễn Thái Ngọc Duy
` (18 subsequent siblings)
21 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
Nguyễn Thái Ngọc Duy
This variable is added [1] with the assumption that on a sane system,
fopen(<dir>, "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 <pclouds@gmail.com>
---
config.mak.uname | 3 +++
1 file changed, 3 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
--
2.11.0.157.gd943d85
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH v2 04/21] wrapper.c: add warn_on_fopen_errors()
2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
` (2 preceding siblings ...)
2017-05-03 10:16 ` [PATCH v2 03/21] config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16 ` Nguyễn Thái Ngọc Duy
2017-05-03 15:07 ` Johannes Schindelin
2017-05-04 5:35 ` Junio C Hamano
2017-05-03 10:16 ` [PATCH v2 05/21] wrapper.c: add fopen_or_warn() Nguyễn Thái Ngọc Duy
` (17 subsequent siblings)
21 siblings, 2 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
dir.c | 3 +--
git-compat-util.h | 2 ++
wrapper.c | 10 ++++++++++
3 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/dir.c b/dir.c
index f451bfa48c..8218a24962 100644
--- a/dir.c
+++ b/dir.c
@@ -745,8 +745,7 @@ 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);
+ warn_on_fopen_errors(fname);
if (0 <= fd)
close(fd);
if (!check_index ||
diff --git a/git-compat-util.h b/git-compat-util.h
index bd04564a69..c5b59c23e8 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1099,6 +1099,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/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;
--
2.11.0.157.gd943d85
^ permalink raw reply related [flat|nested] 70+ messages in thread
* Re: [PATCH v2 04/21] wrapper.c: add warn_on_fopen_errors()
2017-05-03 10:16 ` [PATCH v2 04/21] wrapper.c: add warn_on_fopen_errors() Nguyễn Thái Ngọc Duy
@ 2017-05-03 15:07 ` Johannes Schindelin
2017-05-04 5:35 ` Junio C Hamano
1 sibling, 0 replies; 70+ messages in thread
From: Johannes Schindelin @ 2017-05-03 15:07 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, Jeff King, Johannes Sixt
[-- Attachment #1: Type: text/plain, Size: 1135 bytes --]
Hi Duy,
On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
This commit message is a bit empty. How about:
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.
> diff --git a/dir.c b/dir.c
> index f451bfa48c..8218a24962 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -745,8 +745,7 @@ 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);
> + warn_on_fopen_errors(fname);
> if (0 <= fd)
> close(fd);
I see you already converted one location. Why not simply fold all of them
into this patch? It's not like it would be easier to review those changes
if you separate patches addressing this class of problems.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 04/21] wrapper.c: add warn_on_fopen_errors()
2017-05-03 10:16 ` [PATCH v2 04/21] wrapper.c: add warn_on_fopen_errors() Nguyễn Thái Ngọc Duy
2017-05-03 15:07 ` Johannes Schindelin
@ 2017-05-04 5:35 ` Junio C Hamano
1 sibling, 0 replies; 70+ messages in thread
From: Junio C Hamano @ 2017-05-04 5:35 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Jeff King, Johannes Schindelin, Johannes Sixt
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> dir.c | 3 +--
> git-compat-util.h | 2 ++
> wrapper.c | 10 ++++++++++
> 3 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index f451bfa48c..8218a24962 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -745,8 +745,7 @@ 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);
> + warn_on_fopen_errors(fname);
At least in the original, a reader can guess that, because errno
cannot be NOENT if open(2) succeeded and fstat(2) failed, we call
warn_on_inaccessible() only when we fail to open.
This change makes it harder to see why this is OK when the failure
is not in open(2) but in fstat(2).
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);
... and the remainder of the original ...
or something like that, perhaps?
> --- 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)
> +{
Does this need to be returning "int", or should it be "void",
because it always warns when there is an issue, the caller does not
get to choose to give its own warning?
> + if (errno != ENOENT && errno != ENOTDIR) {
> + warn_on_inaccessible(path);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> int xmkstemp(char *template)
> {
> int fd;
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 05/21] wrapper.c: add fopen_or_warn()
2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
` (3 preceding siblings ...)
2017-05-03 10:16 ` [PATCH v2 04/21] wrapper.c: add warn_on_fopen_errors() Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16 ` Nguyễn Thái Ngọc Duy
2017-05-03 10:16 ` [PATCH v2 06/21] attr.c: use fopen_or_warn() Nguyễn Thái Ngọc Duy
` (16 subsequent siblings)
21 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
Nguyễn Thái Ngọc Duy
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 <pclouds@gmail.com>
---
git-compat-util.h | 1 +
wrapper.c | 11 +++++++++++
2 files changed, 12 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index c5b59c23e8..c1647b01e0 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -800,6 +800,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/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;
--
2.11.0.157.gd943d85
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH v2 06/21] attr.c: use fopen_or_warn()
2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
` (4 preceding siblings ...)
2017-05-03 10:16 ` [PATCH v2 05/21] wrapper.c: add fopen_or_warn() Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16 ` Nguyễn Thái Ngọc Duy
2017-05-03 15:09 ` Johannes Schindelin
2017-05-03 10:16 ` [PATCH v2 07/21] ident.c: " Nguyễn Thái Ngọc Duy
` (15 subsequent siblings)
21 siblings, 1 reply; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
attr.c | 7 ++-----
1 file changed, 2 insertions(+), 5 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;
--
2.11.0.157.gd943d85
^ permalink raw reply related [flat|nested] 70+ messages in thread
* Re: [PATCH v2 06/21] attr.c: use fopen_or_warn()
2017-05-03 10:16 ` [PATCH v2 06/21] attr.c: use fopen_or_warn() Nguyễn Thái Ngọc Duy
@ 2017-05-03 15:09 ` Johannes Schindelin
0 siblings, 0 replies; 70+ messages in thread
From: Johannes Schindelin @ 2017-05-03 15:09 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, Jeff King, Johannes Sixt
[-- Attachment #1: Type: text/plain, Size: 347 bytes --]
Hi Duy,
On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
As commented about warn_on_fopen_errors(), I do not see a benefit in
separating the introduction of this small function from the individual
files' changes to use it. Let's bring it all together.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 07/21] ident.c: use fopen_or_warn()
2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
` (5 preceding siblings ...)
2017-05-03 10:16 ` [PATCH v2 06/21] attr.c: use fopen_or_warn() Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16 ` Nguyễn Thái Ngọc Duy
2017-05-03 10:16 ` [PATCH v2 08/21] bisect: report on fopen() error Nguyễn Thái Ngọc Duy
` (14 subsequent siblings)
21 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
ident.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
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");
--
2.11.0.157.gd943d85
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH v2 08/21] bisect: report on fopen() error
2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
` (6 preceding siblings ...)
2017-05-03 10:16 ` [PATCH v2 07/21] ident.c: " Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16 ` Nguyễn Thái Ngọc Duy
2017-05-03 15:12 ` Johannes Schindelin
2017-05-03 10:16 ` [PATCH v2 09/21] blame: report error on open if graft_file is a directory Nguyễn Thái Ngọc Duy
` (13 subsequent siblings)
21 siblings, 1 reply; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
Nguyễn Thái Ngọc Duy
The main thing to catch here is when fopen() is called on a
directory. It's safe even without this change because a few lines
earlier we do check if "filename" is a regular file.
Regardless, let's stay on the safe side in case somebody changes those
lines. Unconditionally printing to stderr by warn_on_inaccessible()
should be fine, because the caller does unconditional fprintf(stderr,
too, no checking for quiet option or anything.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
bisect.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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;
--
2.11.0.157.gd943d85
^ permalink raw reply related [flat|nested] 70+ messages in thread
* Re: [PATCH v2 08/21] bisect: report on fopen() error
2017-05-03 10:16 ` [PATCH v2 08/21] bisect: report on fopen() error Nguyễn Thái Ngọc Duy
@ 2017-05-03 15:12 ` Johannes Schindelin
0 siblings, 0 replies; 70+ messages in thread
From: Johannes Schindelin @ 2017-05-03 15:12 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, Jeff King, Johannes Sixt
[-- Attachment #1: Type: text/plain, Size: 867 bytes --]
Hi Duy,
On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:
> The main thing to catch here is when fopen() is called on a
> directory. It's safe even without this change because a few lines
> earlier we do check if "filename" is a regular file.
>
> Regardless, let's stay on the safe side in case somebody changes those
> lines. Unconditionally printing to stderr by warn_on_inaccessible()
> should be fine, because the caller does unconditional fprintf(stderr,
> too, no checking for quiet option or anything.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
I appreciate that you mention specifically the use case that triggered my
complaint that in turn prodded you into starting working on this.
However, I think this should be a mere side note in a single commit that
introduces and uses fopen_or_warn().
Ciao,
Dscho
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 09/21] blame: report error on open if graft_file is a directory
2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
` (7 preceding siblings ...)
2017-05-03 10:16 ` [PATCH v2 08/21] bisect: report on fopen() error Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16 ` Nguyễn Thái Ngọc Duy
2017-05-03 15:15 ` Johannes Schindelin
2017-05-03 10:16 ` [PATCH v2 10/21] log: report errno on failure to fopen() a file Nguyễn Thái Ngọc Duy
` (12 subsequent siblings)
21 siblings, 1 reply; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/blame.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 07506a3e45..1648b396dc 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2071,10 +2071,12 @@ 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;
+
while (!strbuf_getwholeline(&buf, fp, '\n')) {
/* The format is just "Commit Parent1 Parent2 ...\n" */
struct commit_graft *graft = read_graft_line(buf.buf, buf.len);
--
2.11.0.157.gd943d85
^ permalink raw reply related [flat|nested] 70+ messages in thread
* Re: [PATCH v2 09/21] blame: report error on open if graft_file is a directory
2017-05-03 10:16 ` [PATCH v2 09/21] blame: report error on open if graft_file is a directory Nguyễn Thái Ngọc Duy
@ 2017-05-03 15:15 ` Johannes Schindelin
0 siblings, 0 replies; 70+ messages in thread
From: Johannes Schindelin @ 2017-05-03 15:15 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, Jeff King, Johannes Sixt
[-- Attachment #1: Type: text/plain, Size: 1221 bytes --]
Hi Duy,
On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> builtin/blame.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 07506a3e45..1648b396dc 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2071,10 +2071,12 @@ 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;
> +
> while (!strbuf_getwholeline(&buf, fp, '\n')) {
This is an excellent example to demonstrate why folding all conversions to
fopen_or_warn() into the same patch as introducing that function makes
sense: I had to close this mail, find the mail with the patch introducing
the function, verify that it returns NULL as before, close that mail, and
continue this reply.
Oh, and I do not think it is a good idea to introduce style fixes in the
middle of such a large patch series. It's a bit distracting from the real
meat here.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 10/21] log: report errno on failure to fopen() a file
2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
` (8 preceding siblings ...)
2017-05-03 10:16 ` [PATCH v2 09/21] blame: report error on open if graft_file is a directory Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16 ` Nguyễn Thái Ngọc Duy
2017-05-03 15:19 ` Johannes Schindelin
2017-05-03 10:16 ` [PATCH v2 11/21] log: fix memory leak in open_next_file() Nguyễn Thái Ngọc Duy
` (11 subsequent siblings)
21 siblings, 1 reply; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/log.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
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;
--
2.11.0.157.gd943d85
^ permalink raw reply related [flat|nested] 70+ messages in thread
* Re: [PATCH v2 10/21] log: report errno on failure to fopen() a file
2017-05-03 10:16 ` [PATCH v2 10/21] log: report errno on failure to fopen() a file Nguyễn Thái Ngọc Duy
@ 2017-05-03 15:19 ` Johannes Schindelin
0 siblings, 0 replies; 70+ messages in thread
From: Johannes Schindelin @ 2017-05-03 15:19 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, Jeff King, Johannes Sixt
[-- Attachment #1: Type: text/plain, Size: 1199 bytes --]
Hi Duy,
On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
I do think that this merits a one-line explanation why you do not use
fopen_or_warn() here.
> 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);
The patches before and after this one all convert fopen() to
fopen_or_warn(). If you *must* separate those into individual patches
(which just makes this patch series unnecessarily bloated, if you ask me),
you should at least group them a bit better so that the patch series tells
more of a consistent story rather than jumping back and forth like a
hyperactive four-year-old telling you about her latest adventure.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 11/21] log: fix memory leak in open_next_file()
2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
` (9 preceding siblings ...)
2017-05-03 10:16 ` [PATCH v2 10/21] log: report errno on failure to fopen() a file Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16 ` Nguyễn Thái Ngọc Duy
2017-05-03 10:16 ` [PATCH v2 12/21] commit.c: report error on failure to fopen() the graft file Nguyễn Thái Ngọc Duy
` (10 subsequent siblings)
21 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
Nguyễn Thái Ngọc Duy
Noticed-by: Jeff King <peff@peff.net>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
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;
--
2.11.0.157.gd943d85
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH v2 12/21] commit.c: report error on failure to fopen() the graft file
2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
` (10 preceding siblings ...)
2017-05-03 10:16 ` [PATCH v2 11/21] log: fix memory leak in open_next_file() Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16 ` Nguyễn Thái Ngọc Duy
2017-05-03 10:16 ` [PATCH v2 13/21] remote.c: report error on failure to fopen() Nguyễn Thái Ngọc Duy
` (9 subsequent siblings)
21 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
Nguyễn Thái Ngọc Duy
Suppressing the error (because the command requires --quiet) is not a
concern because we already call error() just a couple lines down.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
commit.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/commit.c b/commit.c
index 73c78c2b80..075607a6da 100644
--- a/commit.c
+++ b/commit.c
@@ -167,10 +167,12 @@ struct commit_graft *read_graft_line(char *buf, int len)
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;
+
while (!strbuf_getwholeline(&buf, fp, '\n')) {
/* The format is just "Commit Parent1 Parent2 ...\n" */
struct commit_graft *graft = read_graft_line(buf.buf, buf.len);
--
2.11.0.157.gd943d85
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH v2 13/21] remote.c: report error on failure to fopen()
2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
` (11 preceding siblings ...)
2017-05-03 10:16 ` [PATCH v2 12/21] commit.c: report error on failure to fopen() the graft file Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16 ` Nguyễn Thái Ngọc Duy
2017-05-03 15:22 ` Johannes Schindelin
2017-05-03 10:16 ` [PATCH v2 14/21] rerere.c: " Nguyễn Thái Ngọc Duy
` (8 subsequent siblings)
21 siblings, 1 reply; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
Nguyễn Thái Ngọc Duy
There's plenty of error() in this code to safely assume --quiet is not a
concern.
t5512 needs update because if we check the path 'refs*master' (i.e. the
asterisk is part of the path) then we'll get an EINVAL error.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
remote.c | 4 ++--
t/t5512-ls-remote.sh | 13 ++++++++++---
2 files changed, 12 insertions(+), 5 deletions(-)
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/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.<name>.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 <branch>.
# 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
'
--
2.11.0.157.gd943d85
^ permalink raw reply related [flat|nested] 70+ messages in thread
* Re: [PATCH v2 13/21] remote.c: report error on failure to fopen()
2017-05-03 10:16 ` [PATCH v2 13/21] remote.c: report error on failure to fopen() Nguyễn Thái Ngọc Duy
@ 2017-05-03 15:22 ` Johannes Schindelin
2017-05-09 10:16 ` Duy Nguyen
0 siblings, 1 reply; 70+ messages in thread
From: Johannes Schindelin @ 2017-05-03 15:22 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, Jeff King, Johannes Sixt
[-- Attachment #1: Type: text/plain, Size: 1794 bytes --]
Hi Duy,
On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:
> There's plenty of error() in this code to safely assume --quiet is not a
> concern.
>
> t5512 needs update because if we check the path 'refs*master' (i.e. the
> asterisk is part of the path) then we'll get an EINVAL error.
So the first change in this patch unmasks a bug that is fixed by the
second patch?
> 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.<name>.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 <branch>.
> # 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
> '
Sure enough. This totally looks like it needs to be a preparatory bug fix.
Please separate it out and make sure that it comes before the
fopen_or_warn() change in the patch series.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 13/21] remote.c: report error on failure to fopen()
2017-05-03 15:22 ` Johannes Schindelin
@ 2017-05-09 10:16 ` Duy Nguyen
2017-05-09 12:56 ` Johannes Schindelin
0 siblings, 1 reply; 70+ messages in thread
From: Duy Nguyen @ 2017-05-09 10:16 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Git Mailing List, Junio C Hamano, Jeff King, Johannes Sixt
Sorry for super late reply. I'm slowly catching up.
On Wed, May 3, 2017 at 10:22 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Duy,
>
>
> On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:
>
>> There's plenty of error() in this code to safely assume --quiet is not a
>> concern.
>>
>> t5512 needs update because if we check the path 'refs*master' (i.e. the
>> asterisk is part of the path) then we'll get an EINVAL error.
>
> So the first change in this patch unmasks a bug that is fixed by the
> second patch?
The change in read_branches_file() in this patch causes the failure.
See the original report [1],
[1] http://public-inbox.org/git/3a2686c2-6766-1235-001f-1b5283b5f408@kdbg.org/
--
Duy
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 13/21] remote.c: report error on failure to fopen()
2017-05-09 10:16 ` Duy Nguyen
@ 2017-05-09 12:56 ` Johannes Schindelin
0 siblings, 0 replies; 70+ messages in thread
From: Johannes Schindelin @ 2017-05-09 12:56 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Junio C Hamano, Jeff King, Johannes Sixt
[-- Attachment #1: Type: text/plain, Size: 1025 bytes --]
Hi Duy,
On Tue, 9 May 2017, Duy Nguyen wrote:
> Sorry for super late reply. I'm slowly catching up.
>
> On Wed, May 3, 2017 at 10:22 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Hi Duy,
> >
> >
> > On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:
> >
> >> There's plenty of error() in this code to safely assume --quiet is not a
> >> concern.
> >>
> >> t5512 needs update because if we check the path 'refs*master' (i.e. the
> >> asterisk is part of the path) then we'll get an EINVAL error.
> >
> > So the first change in this patch unmasks a bug that is fixed by the
> > second patch?
>
> The change in read_branches_file() in this patch causes the failure.
> See the original report [1],
>
> [1] http://public-inbox.org/git/3a2686c2-6766-1235-001f-1b5283b5f408@kdbg.org/
I disagree. I do not think that the first part of the patch causes the
failure. I think the failure was always there, we just did not bother to
report (and catch) it properly.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 14/21] rerere.c: report error on failure to fopen()
2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
` (12 preceding siblings ...)
2017-05-03 10:16 ` [PATCH v2 13/21] remote.c: report error on failure to fopen() Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16 ` Nguyễn Thái Ngọc Duy
2017-05-03 10:17 ` [PATCH v2 15/21] rerere.c: report correct errno Nguyễn Thái Ngọc Duy
` (7 subsequent siblings)
21 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
rerere.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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;
--
2.11.0.157.gd943d85
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH v2 15/21] rerere.c: report correct errno
2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
` (13 preceding siblings ...)
2017-05-03 10:16 ` [PATCH v2 14/21] rerere.c: " Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:17 ` Nguyễn Thái Ngọc Duy
2017-05-03 15:24 ` Johannes Schindelin
2017-05-03 10:17 ` [PATCH v2 16/21] sequencer.c: report error on failure to fopen() Nguyễn Thái Ngọc Duy
` (6 subsequent siblings)
21 siblings, 1 reply; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:17 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
Nguyễn Thái Ngọc Duy
In the first case, we should have reported the reason fopen() fails. In
the second case, fclose() might change errno but we want to report
fopen() failure.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
rerere.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/rerere.c b/rerere.c
index 971bfedfb2..c26c29f87a 100644
--- a/rerere.c
+++ b/rerere.c
@@ -484,13 +484,14 @@ 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) {
+ error_errno("Could not write %s", output);
fclose(io.input);
- return error("Could not write %s", output);
+ return -1;
}
}
--
2.11.0.157.gd943d85
^ permalink raw reply related [flat|nested] 70+ messages in thread
* Re: [PATCH v2 15/21] rerere.c: report correct errno
2017-05-03 10:17 ` [PATCH v2 15/21] rerere.c: report correct errno Nguyễn Thái Ngọc Duy
@ 2017-05-03 15:24 ` Johannes Schindelin
0 siblings, 0 replies; 70+ messages in thread
From: Johannes Schindelin @ 2017-05-03 15:24 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, Jeff King, Johannes Sixt
[-- Attachment #1: Type: text/plain, Size: 1076 bytes --]
Hi Duy,
On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:
> diff --git a/rerere.c b/rerere.c
> index 971bfedfb2..c26c29f87a 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -484,13 +484,14 @@ 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);
IMO the error() -> error_errno() changes should all be part of the same
commit, as they probably share the explanation why fopen_or_warn() is not
appropriate here.
> if (output) {
> io.io.output = fopen(output, "w");
> if (!io.io.output) {
> + error_errno("Could not write %s", output);
> fclose(io.input);
> - return error("Could not write %s", output);
> + return -1;
> }
This one is logically different from the change above, as it not only
cannot be replaced by fopen_or_warn(), but also requires the reordering
due to the different nature of the change.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 16/21] sequencer.c: report error on failure to fopen()
2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
` (14 preceding siblings ...)
2017-05-03 10:17 ` [PATCH v2 15/21] rerere.c: report correct errno Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:17 ` Nguyễn Thái Ngọc Duy
2017-05-03 10:17 ` [PATCH v2 17/21] server-info: " Nguyễn Thái Ngọc Duy
` (5 subsequent siblings)
21 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:17 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
sequencer.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
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))
--
2.11.0.157.gd943d85
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH v2 17/21] server-info: report error on failure to fopen()
2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
` (15 preceding siblings ...)
2017-05-03 10:17 ` [PATCH v2 16/21] sequencer.c: report error on failure to fopen() Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:17 ` Nguyễn Thái Ngọc Duy
2017-05-03 10:17 ` [PATCH v2 18/21] wt-status.c: " Nguyễn Thái Ngọc Duy
` (4 subsequent siblings)
21 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:17 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
server-info.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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. */
--
2.11.0.157.gd943d85
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH v2 18/21] wt-status.c: report error on failure to fopen()
2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
` (16 preceding siblings ...)
2017-05-03 10:17 ` [PATCH v2 17/21] server-info: " Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:17 ` Nguyễn Thái Ngọc Duy
2017-05-03 10:17 ` [PATCH v2 19/21] xdiff-interface.c: report errno on failure to stat() or fopen() Nguyễn Thái Ngọc Duy
` (3 subsequent siblings)
21 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:17 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
wt-status.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
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;
--
2.11.0.157.gd943d85
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH v2 19/21] xdiff-interface.c: report errno on failure to stat() or fopen()
2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
` (17 preceding siblings ...)
2017-05-03 10:17 ` [PATCH v2 18/21] wt-status.c: " Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:17 ` Nguyễn Thái Ngọc Duy
2017-05-03 10:17 ` [PATCH v2 20/21] config.c: handle error on failing to fopen() Nguyễn Thái Ngọc Duy
` (2 subsequent siblings)
21 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:17 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
xdiff-interface.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
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) {
--
2.11.0.157.gd943d85
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH v2 20/21] config.c: handle error on failing to fopen()
2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
` (18 preceding siblings ...)
2017-05-03 10:17 ` [PATCH v2 19/21] xdiff-interface.c: report errno on failure to stat() or fopen() Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:17 ` Nguyễn Thái Ngọc Duy
2017-05-03 15:29 ` Johannes Schindelin
2017-05-03 10:17 ` [PATCH v2 21/21] t1308: add a test case on open a config directory Nguyễn Thái Ngọc Duy
2017-05-04 5:45 ` [PATCH v2 00/21] Junio C Hamano
21 siblings, 1 reply; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:17 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
Nguyễn Thái Ngọc Duy
In the first case, we already correctly return -1 if fopen() fails to
open. But we should report something so people know what's wrong.
In the second case, config_file == NULL does not necessarily mean "no
config file". Bail out if needed.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
config.c | 5 ++++-
t/t1308-config-set.sh | 4 +++-
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/config.c b/config.c
index b4a3205da3..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);
@@ -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/t/t1308-config-set.sh b/t/t1308-config-set.sh
index ff50960cca..13e95561f4 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -187,7 +187,9 @@ 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 "^warning:" output &&
+ grep "^Error" output >actual &&
test_cmp expect actual
'
--
2.11.0.157.gd943d85
^ permalink raw reply related [flat|nested] 70+ messages in thread
* Re: [PATCH v2 20/21] config.c: handle error on failing to fopen()
2017-05-03 10:17 ` [PATCH v2 20/21] config.c: handle error on failing to fopen() Nguyễn Thái Ngọc Duy
@ 2017-05-03 15:29 ` Johannes Schindelin
0 siblings, 0 replies; 70+ messages in thread
From: Johannes Schindelin @ 2017-05-03 15:29 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, Jeff King, Johannes Sixt
[-- Attachment #1: Type: text/plain, Size: 805 bytes --]
Hi Duy,
On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:
> In the first case, we already correctly return -1 if fopen() fails to
> open. But we should report something so people know what's wrong.
>
> In the second case, config_file == NULL does not necessarily mean "no
> config file". Bail out if needed.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Again, it seems that your patch series tries to cut at the file boundary,
not at the "logically the same change" boundary, making it a bit more
cumbersome than necessary to follow this patch series.
I highly recommend squashing the first change into the big fopen() ->
fopen_or_warn() patch I hinted at earlier, and let the second change (with
the accompanying test case) stand on its own.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 21/21] t1308: add a test case on open a config directory
2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
` (19 preceding siblings ...)
2017-05-03 10:17 ` [PATCH v2 20/21] config.c: handle error on failing to fopen() Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:17 ` Nguyễn Thái Ngọc Duy
2017-05-03 15:33 ` Johannes Schindelin
2017-05-04 5:45 ` [PATCH v2 00/21] Junio C Hamano
21 siblings, 1 reply; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:17 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
Nguyễn Thái Ngọc Duy
We don't support config-as-a-directory (maybe someday we will?). Make
sure we consistently fail in this case, which should happen on platforms
where fopen(<directory>) returns non-NULL if FREAD_READS_DIRECTORIES is
defined.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
t/t1308-config-set.sh | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 13e95561f4..e495a61616 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -183,6 +183,15 @@ 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 "^warning:" 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" &&
--
2.11.0.157.gd943d85
^ permalink raw reply related [flat|nested] 70+ messages in thread
* Re: [PATCH v2 21/21] t1308: add a test case on open a config directory
2017-05-03 10:17 ` [PATCH v2 21/21] t1308: add a test case on open a config directory Nguyễn Thái Ngọc Duy
@ 2017-05-03 15:33 ` Johannes Schindelin
0 siblings, 0 replies; 70+ messages in thread
From: Johannes Schindelin @ 2017-05-03 15:33 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, Jeff King, Johannes Sixt
[-- Attachment #1: Type: text/plain, Size: 992 bytes --]
Hi Duy,
On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:
> We don't support config-as-a-directory (maybe someday we will?). Make
> sure we consistently fail in this case, which should happen on platforms
> where fopen(<directory>) returns non-NULL if FREAD_READS_DIRECTORIES is
> defined.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
How about saying that we make sure that this is the case by adding a test
case? Otherwise the reader might (and this reader certainly did) expect a
code change in a .c file.
I reviewed this patch series, and am happy with the overall diff.
My main problem: it should be organized in a more intuitively-graspable
way, i.e. put all the fopen() -> fopen_or_warn() changes into a single
patch, separate out changes (drive-by test fixing for Windows and, ahem,
whitespace changes) into their own commits. Then those commits should be
ordered to relate an easy-flowing story.
Other than that: well done,
Dscho
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 00/21]
2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
` (20 preceding siblings ...)
2017-05-03 10:17 ` [PATCH v2 21/21] t1308: add a test case on open a config directory Nguyễn Thái Ngọc Duy
@ 2017-05-04 5:45 ` Junio C Hamano
2017-05-07 4:20 ` Junio C Hamano
21 siblings, 1 reply; 70+ messages in thread
From: Junio C Hamano @ 2017-05-04 5:45 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Jeff King, Johannes Schindelin, Johannes Sixt
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Changes since v1:
>
> - fopen_or_warn() and warn_on_fopen_errors() are introduced. The
> latter's name is probably not great...
> - A new patch (first one) to convert a bunch to using xfopen()
> - Fix test failure on Windows, found by Johannes Sixt
> - Fix the memory leak in log.c, found by Jeff
>
> There are still a couple of fopen() remained, but I'm getting slow
> again so let's get these in first and worry about the rest when
> somebody has time.
>
> Nguyễn Thái Ngọc Duy (21):
> Use xfopen() in more places
> clone: use xfopen() instead of fopen()
> config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD
> wrapper.c: add warn_on_fopen_errors()
> wrapper.c: add fopen_or_warn()
> attr.c: use fopen_or_warn()
> ident.c: use fopen_or_warn()
> bisect: report on fopen() error
> blame: report error on open if graft_file is a directory
> log: report errno on failure to fopen() a file
> log: fix memory leak in open_next_file()
> commit.c: report error on failure to fopen() the graft file
> remote.c: report error on failure to fopen()
> rerere.c: report error on failure to fopen()
> rerere.c: report correct errno
> sequencer.c: report error on failure to fopen()
> server-info: report error on failure to fopen()
> wt-status.c: report error on failure to fopen()
> xdiff-interface.c: report errno on failure to stat() or fopen()
> config.c: handle error on failing to fopen()
> t1308: add a test case on open a config directory
Thanks. If the number of parts affected by this series were
smaller, it may have made the review easier to have the introduction
of a helper and its use in a single larger patch, but there are
spread across many, some with files that are touched by different
in-flight topics, and these "collection of smaller patches" makes it
easier to manage both while reviewing and also merging.
All looked good, even though I do share the doubt on the name
"warn-on-fopen-errors"; when something applies equally to fopen(3)
and underlying open(2), I would tend to call that with open not
fopen myself. But that is a minor point.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 00/21]
2017-05-04 5:45 ` [PATCH v2 00/21] Junio C Hamano
@ 2017-05-07 4:20 ` Junio C Hamano
2017-05-09 10:22 ` Duy Nguyen
0 siblings, 1 reply; 70+ messages in thread
From: Junio C Hamano @ 2017-05-07 4:20 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: Git Mailing List, Jeff King, Johannes Schindelin, Johannes Sixt
On Thu, May 4, 2017 at 2:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
> > Changes since v1:
> >
> > - fopen_or_warn() and warn_on_fopen_errors() are introduced. The
> > latter's name is probably not great...
> > - A new patch (first one) to convert a bunch to using xfopen()
> > - Fix test failure on Windows, found by Johannes Sixt
> > - Fix the memory leak in log.c, found by Jeff
> >
> > There are still a couple of fopen() remained, but I'm getting slow
> > again so let's get these in first and worry about the rest when
> > somebody has time.
Hmm, is this topic responsible for recent breakage Travis claims on MacOS X?
https://travis-ci.org/git/git/jobs/229585098#L3091
seems to expect an error when test-config is fed a-directory but we are
not getting the expected warning and error?
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 00/21]
2017-05-07 4:20 ` Junio C Hamano
@ 2017-05-09 10:22 ` Duy Nguyen
2017-05-09 22:52 ` Junio C Hamano
0 siblings, 1 reply; 70+ messages in thread
From: Duy Nguyen @ 2017-05-09 10:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git Mailing List, Jeff King, Johannes Schindelin, Johannes Sixt
On Sun, May 7, 2017 at 11:20 AM, Junio C Hamano <gitster@pobox.com> wrote:
> On Thu, May 4, 2017 at 2:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>>
>> > Changes since v1:
>> >
>> > - fopen_or_warn() and warn_on_fopen_errors() are introduced. The
>> > latter's name is probably not great...
>> > - A new patch (first one) to convert a bunch to using xfopen()
>> > - Fix test failure on Windows, found by Johannes Sixt
>> > - Fix the memory leak in log.c, found by Jeff
>> >
>> > There are still a couple of fopen() remained, but I'm getting slow
>> > again so let's get these in first and worry about the rest when
>> > somebody has time.
>
> Hmm, is this topic responsible for recent breakage Travis claims on MacOS X?
>
> https://travis-ci.org/git/git/jobs/229585098#L3091
>
> seems to expect an error when test-config is fed a-directory but we are
> not getting the expected warning and error?
Sounds about right. Let me see if defining FREAD_READS_DIRECTORIES on
MacOS X makes travis happy.
--
Duy
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 00/21]
2017-05-09 10:22 ` Duy Nguyen
@ 2017-05-09 22:52 ` Junio C Hamano
2017-05-10 5:12 ` [PATCH] config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too Junio C Hamano
0 siblings, 1 reply; 70+ messages in thread
From: Junio C Hamano @ 2017-05-09 22:52 UTC (permalink / raw)
To: Duy Nguyen
Cc: Git Mailing List, Jeff King, Johannes Schindelin, Johannes Sixt
Duy Nguyen <pclouds@gmail.com> writes:
> On Sun, May 7, 2017 at 11:20 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> On Thu, May 4, 2017 at 2:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>>>
>>> > Changes since v1:
>>> >
>>> > - fopen_or_warn() and warn_on_fopen_errors() are introduced. The
>>> > latter's name is probably not great...
>>> > - A new patch (first one) to convert a bunch to using xfopen()
>>> > - Fix test failure on Windows, found by Johannes Sixt
>>> > - Fix the memory leak in log.c, found by Jeff
>>> >
>>> > There are still a couple of fopen() remained, but I'm getting slow
>>> > again so let's get these in first and worry about the rest when
>>> > somebody has time.
>>
>> Hmm, is this topic responsible for recent breakage Travis claims on MacOS X?
>>
>> https://travis-ci.org/git/git/jobs/229585098#L3091
>>
>> seems to expect an error when test-config is fed a-directory but we are
>> not getting the expected warning and error?
>
> Sounds about right. Let me see if defining FREAD_READS_DIRECTORIES on
> MacOS X makes travis happy.
Thanks. I should have suspected that myself to save a round-trip.
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH] config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too
2017-05-09 22:52 ` Junio C Hamano
@ 2017-05-10 5:12 ` Junio C Hamano
0 siblings, 0 replies; 70+ messages in thread
From: Junio C Hamano @ 2017-05-10 5:12 UTC (permalink / raw)
To: Duy Nguyen
Cc: Git Mailing List, Jeff King, Johannes Schindelin, Johannes Sixt
Let's do the same for Macs, as it is BSD variant after all.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
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
--
2.13.0-336-gf73534b083
^ permalink raw reply related [flat|nested] 70+ messages in thread