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
[PATCH] am: terminate state files with a newline
On Thu, Aug 20, 2015 at 11:40:20AM -0700, Junio C Hamano wrote:
SZEDER Gábor sze...@ira.uka.de writes:
The format of the files '.git/rebase-apply/{next,last}' changed slightly
with the recent builtin 'git am' conversion: while these files were
newline-terminated when written by the scripted version, the ones written
by the builtin are not.
Thanks for noticing; that should be corrected, I think.
Okay then, this patch should correct this.
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?) :-/ .
Regards,
Paul
-- 8 --
Subject: [PATCH] am: terminate state files with a newline
Since builtin/am.c replaced git-am.sh in 783d7e8 (builtin-am: remove
redirection to git-am.sh, 2015-08-04), the state files written by git-am
did not terminate with a newline.
This is because the code in builtin/am.c did not write the newline to
the state files.
While the git codebase has no problems with the missing newline,
external software which read the contents of the state directory may be
strict about the existence of the terminating newline, and would thus
break.
Fix this by correcting the relevant calls to write_file() to ensure that
the state files written terminate with a newline, matching how git-am.sh
behaves.
While we are fixing the write_file() calls, fix the writing of the
dirtyindex file as well -- we should be creating an empty file to
match the behavior of git-am.sh.
Reported-by: SZEDER Gábor sze...@ira.uka.de
Signed-off-by: Paul Tan pyoka...@gmail.com
---
builtin/am.c | 30 +++---
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index 1399c8d..2e57fad 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -994,13 +994,13 @@ static void am_setup(struct am_state *state, enum
patch_format patch_format,
if (state-rebasing)
state-threeway = 1;
- write_file(am_path(state, threeway), 1, state-threeway ? t : f);
+ write_file(am_path(state, threeway), 1, %s\n, state-threeway ? t
: f);
- write_file(am_path(state, quiet), 1, state-quiet ? t : f);
+ write_file(am_path(state, quiet), 1, %s\n, state-quiet ? t :
f);
- write_file(am_path(state, sign), 1, state-signoff ? t : f);
+ write_file(am_path(state, sign), 1, %s\n, state-signoff ? t :
f);
- write_file(am_path(state, utf8), 1, state-utf8 ? t : f);
+ write_file(am_path(state, utf8), 1, %s\n, state-utf8 ? t : f);
switch (state-keep) {
case KEEP_FALSE:
@@ -1016,9 +1016,9 @@ static void am_setup(struct am_state *state, enum
patch_format patch_format,
die(BUG: invalid value for state-keep);
}
- write_file(am_path(state, keep), 1, %s, str);
+ write_file(am_path(state, keep), 1, %s\n, str);
- write_file(am_path(state, messageid), 1, state-message_id ? t :
f);
+ write_file(am_path(state, messageid), 1, %s\n, state-message_id ?
t : f);
switch (state-scissors) {
case SCISSORS_UNSET:
@@ -1034,10 +1034,10 @@ static void am_setup(struct am_state *state, enum
patch_format patch_format,
die(BUG: invalid value for state-scissors);
}
- write_file(am_path(state, scissors), 1, %s, str);
+ write_file(am_path(state, scissors), 1, %s\n, str);
sq_quote_argv(sb, state-git_apply_opts.argv, 0);
- write_file(am_path(state, apply-opt), 1, %s, sb.buf);
+ write_file(am_path(state, apply-opt), 1, %s\n, sb.buf);
if (state-rebasing)
write_file(am_path(state, rebasing), 1, %s, );
@@ -1045,7 +1045,7 @@ static void am_setup(struct am_state *state, enum
patch_format patch_format,
write_file(am_path(state, applying), 1, %s, );
if (!get_sha1(HEAD, curr_head)) {
- write_file(am_path(state, abort-safety), 1, %s,
sha1_to_hex(curr_head));
+ write_file(am_path(state, abort-safety), 1, %s\n,
sha1_to_hex(curr_head));
if (!state-rebasing)
update_ref(am, ORIG_HEAD, curr_head, NULL, 0,
UPDATE_REFS_DIE_ON_ERR);
@@ -1060,9 +1060,9 @@ static void am_setup(struct am_state *state, enum
patch_format patch_format,
* session is in progress, they should be written last.
*/
- write_file(am_path(state, next), 1, %d, state-cur);
+ write_file(am_path(state, next), 1, %d\n, state-cur);
- write_file(am_path(state, last), 1, %d, state-last);
+ write_file(am_path(state, last), 1, %d\n, state-last);
strbuf_release(sb);
}
@@ -1095,12 +1095,12 @@ static void am_next(struct am_state *state)
unlink(am_path(state, original-commit));
if (!get_sha1(HEAD, head))
- write_file
