Re: [PATCH] am: terminate state files with a newline
On Sun, Aug 23, 2015 at 11:48:44PM -0700, Junio C Hamano wrote: I think the if here is redundant; strbuf_complete_line already handles it. True. And I like your write_state_bool() wrapper (which should be static void to the builtin/am.c) very much. On top of that, I think the right thing to do to write_file() would be to first clean-up the second parameter fatal to an unsigned flags whose (10) bit is fatal, (11) bit is binary, and make this new call to strbuf_complete_line() only when binary bit is not set. The new comment I added before write_file() function needs to be adjusted if we were to do this, obviously. Yup, I agree with all of that. I'm about to go to bed, so I'll assume you or Paul will cook up a patch. :) -Peff -- 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] am: terminate state files with a newline
Jeff King p...@peff.net writes:
FWIW, I had a similar thought when reading the original thread. I also
noted that all of the callers here pass 1 for the fatal parameter,
and that they are either bools or single strings. I wonder if:
void write_state_bool(struct am_state *state, const char *name, int v)
{
write_file(am_path(state, name), 1, %s\n, v ? t : f);
}
would make the call-sites even easier to read (and of course the \n
would be dropped here if it does migrate up to write_file()).
@@ -634,6 +641,9 @@ int write_file(const char *path, int fatal, const char
*fmt, ...)
va_start(params, fmt);
strbuf_vaddf(sb, fmt, params);
va_end(params);
+if (sb.len)
+strbuf_complete_line(sb);
+
I think the if here is redundant; strbuf_complete_line already handles
it.
True. And I like your write_state_bool() wrapper (which should be
static void to the builtin/am.c) very much.
On top of that, I think the right thing to do to write_file() would
be to first clean-up the second parameter fatal to an unsigned
flags whose (10) bit is fatal, (11) bit is binary, and make
this new call to strbuf_complete_line() only when binary bit is
not set.
The new comment I added before write_file() function needs to be
adjusted if we were to do this, obviously.
--
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] am: terminate state files with a newline
On Sun, Aug 23, 2015 at 01:50:53PM +0800, Paul Tan wrote: Did we ever explictly allow external programs to poke around the contents of the .git/rebase-apply directory? I think it may not be so good, as it means that it may not be possible to switch the storage format in the future (e.g. to allow atomic modifications, maybe?) :-/ . zsh's vcs_info does read files in those directories in order to determine which patches have been applied. I just submitted a patch to zsh that fixed warnings when a conflict occurred with git rebase -m. I expect that unless we provide a programmatic way to discover all of that information trivially (and maybe even then, due to compatibility with older versions of git), people are going to poke around those directories. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
Re: [PATCH] am: terminate state files with a newline
Paul Tan pyoka...@gmail.com writes:
Did we ever explictly allow external programs to poke around the
contents of the .git/rebase-apply directory?
We tell users to take a peek into it when am fails, don't we, by
naming $GIT_DIR/rebase-apply/patch?
- write_file(am_path(state, threeway), 1, state-threeway ? t : f);
+ write_file(am_path(state, threeway), 1, %s\n, state-threeway ? t
: f);
Stepping back a bit, after realizing that write_file() is a
short-hand for I have all information necessary to produce the full
contents of a file, now go ahead and create and write that and
close, I have to wonder what caller even wants to create a file
with an incomplete line at the end.
All callers outside builtin/am.c except one caller uses it to
produce a single line file. The oddball is git branch that uses
it to prepare a temporary file used to edit branch description.
builtin/branch.c: if (write_file(git_path(edit_description), 0, %s,
buf.buf)) {
The payload it prepares in buf.buf ends with a canned comment that
ends with LF. So in that sense it is not even an oddball.
The above analysis makes me wonder if this is a simpler and more
future proof approach.
Or did I miss any caller or a reasonable potential future use case
that wants to create a binary file or a text file that ends with an
incomplete line?
wrapper.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/wrapper.c b/wrapper.c
index e451463..7a92298 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -621,6 +621,13 @@ char *xgetcwd(void)
return strbuf_detach(sb, NULL);
}
+/*
+ * Create a TEXT file by specifying its full contents via fmt and the
+ * remainder of args that are used like printf. A terminating LF is
+ * added at the end of the file if it is missing (it is simpler for
+ * the callers because the function is often used to create a
+ * single-liner file).
+ */
int write_file(const char *path, int fatal, const char *fmt, ...)
{
struct strbuf sb = STRBUF_INIT;
@@ -634,6 +641,9 @@ int write_file(const char *path, int fatal, const char
*fmt, ...)
va_start(params, fmt);
strbuf_vaddf(sb, fmt, params);
va_end(params);
+ if (sb.len)
+ strbuf_complete_line(sb);
+
if (write_in_full(fd, sb.buf, sb.len) != sb.len) {
int err = errno;
close(fd);
--
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] am: terminate state files with a newline
Hi, Quoting Paul Tan pyoka...@gmail.com: Did we ever explictly allow external programs to poke around the contents of the .git/rebase-apply directory? I think it may not be so good, as it means that it may not be possible to switch the storage format in the future (e.g. to allow atomic modifications, maybe?) :-/ . Think of e.g. libgit2, JGit/EGit and all the other git implementations. They should be able to look everywhere in .git, shouldn't they? I don't think we will just switch the storage format of any parts of the repo. Whatever new formats may come (ref backends, index v5, pack v4), they will be an opt-in feature for a long time before becoming default, and there must be an even longer deprecation period before the old format gets phased out, if ever. Gábor -- 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] am: terminate state files with a newline
On Sun, Aug 23, 2015 at 12:05:32PM -0700, Junio C Hamano wrote:
- write_file(am_path(state, threeway), 1, state-threeway ? t : f);
+ write_file(am_path(state, threeway), 1, %s\n, state-threeway ? t
: f);
Stepping back a bit, after realizing that write_file() is a
short-hand for I have all information necessary to produce the full
contents of a file, now go ahead and create and write that and
close, I have to wonder what caller even wants to create a file
with an incomplete line at the end.
FWIW, I had a similar thought when reading the original thread. I also
noted that all of the callers here pass 1 for the fatal parameter,
and that they are either bools or single strings. I wonder if:
void write_state_bool(struct am_state *state, const char *name, int v)
{
write_file(am_path(state, name), 1, %s\n, v ? t : f);
}
would make the call-sites even easier to read (and of course the \n
would be dropped here if it does migrate up to write_file()).
@@ -634,6 +641,9 @@ int write_file(const char *path, int fatal, const char
*fmt, ...)
va_start(params, fmt);
strbuf_vaddf(sb, fmt, params);
va_end(params);
+ if (sb.len)
+ strbuf_complete_line(sb);
+
I think the if here is redundant; strbuf_complete_line already handles
it.
-Peff
--
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
