Re: [PATCH 3/5] write_file(): introduce an explicit WRITE_FILE_GENTLY request
On Tue, Aug 25, 2015 at 1:41 AM, Junio C Hamano wrote: > Junio C Hamano writes: > >> All callers except for two ask this function to die upon error by >> passing fatal=1; turn the parameter to a more generic "unsigned flag" >> bag of bits, introduce an explicit WRITE_FILE_GENTLY bit and change >> these two callers to pass that bit. > > There is a huge iffyness around one of these two oddball callers. > >> diff --git a/setup.c b/setup.c >> index 5f9f07d..718f4e1 100644 >> --- a/setup.c >> +++ b/setup.c >> @@ -404,7 +404,7 @@ static void update_linked_gitdir(const char *gitfile, >> const char *gitdir) >> >> strbuf_addf(&path, "%s/gitfile", gitdir); >> if (stat(path.buf, &st) || st.st_mtime + 24 * 3600 < time(NULL)) >> - write_file(path.buf, 0, "%s\n", gitfile); >> + write_file(path.buf, WRITE_FILE_GENTLY, "%s\n", gitfile); >> strbuf_release(&path); >> } > > This comes from 23af91d1 (prune: strategies for linked checkouts, > 2014-11-30). I cannot tell what the justification is to treat a > failure to write a gitfile as a non-error event. Just a sloppy > coding that lets the program go through to its finish, ignoring the > harm done by possibly corrupting user repository silently? Failing to write to this file is not a big deal _if_ the file is not corrupted because of this write operation. But we should not be so silent about this. If the file content is corrupted and it's old enough, this checkout may be pruned. I think there's another bug here... wrong name.. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] write_file(): introduce an explicit WRITE_FILE_GENTLY request
Junio C Hamano writes:
> All callers except for two ask this function to die upon error by
> passing fatal=1; turn the parameter to a more generic "unsigned flag"
> bag of bits, introduce an explicit WRITE_FILE_GENTLY bit and change
> these two callers to pass that bit.
There is a huge iffyness around one of these two oddball callers.
> diff --git a/setup.c b/setup.c
> index 5f9f07d..718f4e1 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -404,7 +404,7 @@ static void update_linked_gitdir(const char *gitfile,
> const char *gitdir)
>
> strbuf_addf(&path, "%s/gitfile", gitdir);
> if (stat(path.buf, &st) || st.st_mtime + 24 * 3600 < time(NULL))
> - write_file(path.buf, 0, "%s\n", gitfile);
> + write_file(path.buf, WRITE_FILE_GENTLY, "%s\n", gitfile);
> strbuf_release(&path);
> }
This comes from 23af91d1 (prune: strategies for linked checkouts,
2014-11-30). I cannot tell what the justification is to treat a
failure to write a gitfile as a non-error event. Just a sloppy
coding that lets the program go through to its finish, ignoring the
harm done by possibly corrupting user repository silently?
> diff --git a/transport.c b/transport.c
> index 40692f8..e1821a4 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -291,7 +291,7 @@ static int write_one_ref(const char *name, const struct
> object_id *oid,
>
> strbuf_addstr(buf, name);
> if (safe_create_leading_directories(buf->buf) ||
> - write_file(buf->buf, 0, "%s\n", oid_to_hex(oid)))
> + write_file(buf->buf, WRITE_FILE_GENTLY, "%s\n", oid_to_hex(oid)))
> return error("problems writing temporary file %s: %s",
>buf->buf, strerror(errno));
> strbuf_setlen(buf, len);
This one is OK, in that it is merely to give a better error
diagnosis than just "oh, I cannot write so I die".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/5] write_file(): introduce an explicit WRITE_FILE_GENTLY request
All callers except for two ask this function to die upon error by
passing fatal=1; turn the parameter to a more generic "unsigned flag"
bag of bits, introduce an explicit WRITE_FILE_GENTLY bit and change
these two callers to pass that bit.
This is in preparation to add one more bit to this flag word.
Signed-off-by: Junio C Hamano
---
cache.h | 15 ++-
setup.c | 2 +-
transport.c | 2 +-
wrapper.c | 3 ++-
4 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/cache.h b/cache.h
index 6bb7119..f105235 100644
--- a/cache.h
+++ b/cache.h
@@ -1539,8 +1539,21 @@ static inline ssize_t write_str_in_full(int fd, const
char *str)
{
return write_in_full(fd, str, strlen(str));
}
+
+/*
+ * Create a new file by specifying its full contents via fmt and the
+ * remainder of args that are used like 'printf()' args. Die upon
+ * an error unless WRITE_FILE_GENTLY flag is set, in which case return
+ * a negative number to signal an error.
+ *
+ * For historical reasons, the LSB of flags word is set by many
+ * callers to explicitly ask the function to die upon error, but now
+ * it is the default.
+ */
+#define WRITE_FILE_UNUSED_0 (1<<0)
+#define WRITE_FILE_GENTLY (1<<1)
__attribute__((format (printf, 3, 4)))
-extern int write_file(const char *path, int fatal, const char *fmt, ...);
+extern int write_file(const char *path, unsigned flags, const char *fmt, ...);
/* pager.c */
extern void setup_pager(void);
diff --git a/setup.c b/setup.c
index 5f9f07d..718f4e1 100644
--- a/setup.c
+++ b/setup.c
@@ -404,7 +404,7 @@ static void update_linked_gitdir(const char *gitfile, const
char *gitdir)
strbuf_addf(&path, "%s/gitfile", gitdir);
if (stat(path.buf, &st) || st.st_mtime + 24 * 3600 < time(NULL))
- write_file(path.buf, 0, "%s\n", gitfile);
+ write_file(path.buf, WRITE_FILE_GENTLY, "%s\n", gitfile);
strbuf_release(&path);
}
diff --git a/transport.c b/transport.c
index 40692f8..e1821a4 100644
--- a/transport.c
+++ b/transport.c
@@ -291,7 +291,7 @@ static int write_one_ref(const char *name, const struct
object_id *oid,
strbuf_addstr(buf, name);
if (safe_create_leading_directories(buf->buf) ||
- write_file(buf->buf, 0, "%s\n", oid_to_hex(oid)))
+ write_file(buf->buf, WRITE_FILE_GENTLY, "%s\n", oid_to_hex(oid)))
return error("problems writing temporary file %s: %s",
buf->buf, strerror(errno));
strbuf_setlen(buf, len);
diff --git a/wrapper.c b/wrapper.c
index e451463..68d45b6 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -621,8 +621,9 @@ char *xgetcwd(void)
return strbuf_detach(&sb, NULL);
}
-int write_file(const char *path, int fatal, const char *fmt, ...)
+int write_file(const char *path, unsigned flags, const char *fmt, ...)
{
+ int fatal = !(flags & WRITE_FILE_GENTLY);
struct strbuf sb = STRBUF_INIT;
va_list params;
int fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0666);
--
2.5.0-568-g53a3e28
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
