Minor builtin 'git am' side-effect

Minor builtin 'git am' side-effect

[SZEDER Gábor]

Hi,

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.

This probably  makes no difference for shell scripts looking at these
files, e.g.  our __git_ps1() handles both just fine.  However, it can
break C programs, when after strtol()ing the contents of the files they
get defensive and check for the terminating newline at *endptr (this is
how I noticed, as one of my pet projects did just that).

I'm not saying that the new behavior is bad and should be fixed; I merely
point it out and leave the rest for you to decide.

Best,
Gábor

Re: Minor builtin 'git am' side-effect

[Junio C Hamano]

SZEDER Gábor <szeder@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.

[PATCH] am: terminate state files with a newline

[Paul Tan]

On Thu, Aug 20, 2015 at 11:40:20AM -0700, Junio C Hamano wrote:
> SZEDER Gábor <szeder@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 <szeder@ira.uka.de>
Signed-off-by: Paul Tan <pyokagan@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(am_path(state, "abort-safety"), 1, "%s", sha1_to_hex(head));
+		write_file(am_path(state, "abort-safety"), 1, "%s\n", sha1_to_hex(head));
 	else
 		write_file(am_path(state, "abort-safety"), 1, "%s", "");
 
 	state->cur++;
-	write_file(am_path(state, "next"), 1, "%d", state->cur);
+	write_file(am_path(state, "next"), 1, "%d\n", state->cur);
 }
 
 /**
@@ -1461,7 +1461,7 @@ static int parse_mail_rebase(struct am_state *state, const char *mail)
 	write_commit_patch(state, commit);
 
 	hashcpy(state->orig_commit, commit_sha1);
-	write_file(am_path(state, "original-commit"), 1, "%s",
+	write_file(am_path(state, "original-commit"), 1, "%s\n",
 			sha1_to_hex(commit_sha1));
 
 	return 0;
@@ -1764,7 +1764,7 @@ static void am_run(struct am_state *state, int resume)
 	refresh_and_write_cache();
 
 	if (index_has_changes(&sb)) {
-		write_file(am_path(state, "dirtyindex"), 1, "t");
+		write_file(am_path(state, "dirtyindex"), 1, "%s", "");
 		die(_("Dirty index: cannot apply patches (dirty: %s)"), sb.buf);
 	}
 
-- 
2.5.0.400.gff86faf.dirty

Re: [PATCH] am: terminate state files with a newline

[SZEDER Gábor]

Hi,

Quoting Paul Tan <pyokagan@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

Re: [PATCH] am: terminate state files with a newline

[Junio C Hamano]

Paul Tan <pyokagan@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);

Re: [PATCH] am: terminate state files with a newline

[Jeff King]

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

Re: [PATCH] am: terminate state files with a newline

[Junio C Hamano]

Jeff King <peff@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 (1<<0) bit is "fatal", (1<<1) 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.

Re: [PATCH] am: terminate state files with a newline

[Jeff King]

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 (1<<0) bit is "fatal", (1<<1) 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

[PATCH 0/5] "am" state file fix with write_file() clean-up

[Junio C Hamano]

So here is an solution based on the "write_file() is primarily to
produce text, so it should be able to correct the incomplete line
at the end" approach.

The first one is Peff's idea to consolidate callers in "am", in a
more concrete form.

The second is the fix to $gmane/276238.

The remainder is to clean up write_file() helper function.  All
callers except for two were passing 1 as one parameter, whose
meaning was not all obvious to a casual reader.

In patch 3/5, we flip the default behaviour of write_file() to die
upon error unless explicitly asked not to with WRITE_FILE_GENTLY
flag, and change the two oddball callers to pass this new flag.

In patch 4/5, we enhance the default behaviour of write_file() to
complete an incomplete line at the end, unless asked not to with
WRITE_FILE_BINARY flag; nobody passes this because all existing
callers want to produce a text file.

In patch 5/5, the transitional noise left by patches 3 and 4 are
cleaned up by updating the non-binary callers not to add LF
themselves and by changing the callers that pass 1 as flags
parameter to pass 0 (as bit (1<<0) is a no-op since patch 3/5).

The series is built on top of b5e8235, the current tip of the
pt/am-builtin-options topic.


Junio C Hamano (5):
  builtin/am: introduce write_state_*() helper functions
  builtin/am: make sure state files are text
  write_file(): introduce an explicit WRITE_FILE_GENTLY request
  write_file(): do not leave incomplete line at the end
  write_file(): clean up transitional mess of flag words and terminating LF

 builtin/am.c       | 68 ++++++++++++++++++++++++++++++++----------------------
 builtin/init-db.c  |  2 +-
 builtin/worktree.c | 10 ++++----
 cache.h            | 16 ++++++++++++-
 daemon.c           |  2 +-
 setup.c            |  2 +-
 submodule.c        |  2 +-
 transport.c        |  2 +-
 wrapper.c          | 13 +++++++++--
 9 files changed, 77 insertions(+), 40 deletions(-)

-- 
2.5.0-568-g53a3e28

[PATCH 1/5] builtin/am: introduce write_state_*() helper functions

[Junio C Hamano]

There are many calls to write_file() that repeats the same pattern
in the implementation of the builtin version of "am", and they all
share the same traits, i.e they

 - produce a text file with a single string in it;

 - have enough information to produce the entire contents of that
   file;

 - generate the pathname of the file by making a call to am_path(); and

 - they ask write_file() to die() upon failure.

The slight differences among the call sites throw them into roughly
three variants:

 - many write either "t" or "f" based on a boolean value to a file;

 - some write the integer value in decimal text;

 - some others write more general string, e.g. an object name in
   hex, an empty string (i.e. the presense of the file itself serves
   as a flag), etc.

Introduce three helpers, write_state_bool(), write_state_count() and
write_state_text(), to reduce direct calls to write_file().

This is a preparatory step for the next step to ensure that no
"state" file this command leaves in $GIT_DIR is with an incomplete
line at the end.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/am.c | 68 ++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 41 insertions(+), 27 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 634f7a7..4d34dc5 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -194,6 +194,27 @@ static inline const char *am_path(const struct am_state *state, const char *path
 }
 
 /**
+ * For convenience to call write_file()
+ */
+static int write_state_text(const struct am_state *state,
+			    const char *name, const char *string)
+{
+	return write_file(am_path(state, name), 1, "%s", string);
+}
+
+static int write_state_count(const struct am_state *state,
+			     const char *name, int value)
+{
+	return write_file(am_path(state, name), 1, "%d", value);
+}
+
+static int write_state_bool(const struct am_state *state,
+			    const char *name, int value)
+{
+	return write_state_text(state, name, value ? "t" : "f");
+}
+
+/**
  * If state->quiet is false, calls fprintf(fp, fmt, ...), and appends a newline
  * at the end.
  */
@@ -362,7 +383,7 @@ static void write_author_script(const struct am_state *state)
 	sq_quote_buf(&sb, state->author_date);
 	strbuf_addch(&sb, '\n');
 
-	write_file(am_path(state, "author-script"), 1, "%s", sb.buf);
+	write_state_text(state, "author-script", sb.buf);
 
 	strbuf_release(&sb);
 }
@@ -1000,13 +1021,10 @@ 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, "quiet"), 1, state->quiet ? "t" : "f");
-
-	write_file(am_path(state, "sign"), 1, state->signoff ? "t" : "f");
-
-	write_file(am_path(state, "utf8"), 1, state->utf8 ? "t" : "f");
+	write_state_bool(state, "threeway", state->threeway);
+	write_state_bool(state, "quiet", state->quiet);
+	write_state_bool(state, "sign", state->signoff);
+	write_state_bool(state, "utf8", state->utf8);
 
 	switch (state->keep) {
 	case KEEP_FALSE:
@@ -1022,9 +1040,8 @@ 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, "messageid"), 1, state->message_id ? "t" : "f");
+	write_state_text(state, "keep", str);
+	write_state_bool(state, "messageid", state->message_id);
 
 	switch (state->scissors) {
 	case SCISSORS_UNSET:
@@ -1039,24 +1056,23 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,
 	default:
 		die("BUG: invalid value for state->scissors");
 	}
-
-	write_file(am_path(state, "scissors"), 1, "%s", str);
+	write_state_text(state, "scissors", str);
 
 	sq_quote_argv(&sb, state->git_apply_opts.argv, 0);
-	write_file(am_path(state, "apply-opt"), 1, "%s", sb.buf);
+	write_state_text(state, "apply-opt", sb.buf);
 
 	if (state->rebasing)
-		write_file(am_path(state, "rebasing"), 1, "%s", "");
+		write_state_text(state, "rebasing", "");
 	else
-		write_file(am_path(state, "applying"), 1, "%s", "");
+		write_state_text(state, "applying", "");
 
 	if (!get_sha1("HEAD", curr_head)) {
-		write_file(am_path(state, "abort-safety"), 1, "%s", sha1_to_hex(curr_head));
+		write_state_text(state, "abort-safety", sha1_to_hex(curr_head));
 		if (!state->rebasing)
 			update_ref("am", "ORIG_HEAD", curr_head, NULL, 0,
 					UPDATE_REFS_DIE_ON_ERR);
 	} else {
-		write_file(am_path(state, "abort-safety"), 1, "%s", "");
+		write_state_text(state, "abort-safety", "");
 		if (!state->rebasing)
 			delete_ref("ORIG_HEAD", NULL, 0);
 	}
@@ -1066,9 +1082,8 @@ 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, "last"), 1, "%d", state->last);
+	write_state_count(state, "next", state->cur);
+	write_state_count(state, "last", state->last);
 
 	strbuf_release(&sb);
 }
@@ -1101,12 +1116,12 @@ static void am_next(struct am_state *state)
 	unlink(am_path(state, "original-commit"));
 
 	if (!get_sha1("HEAD", head))
-		write_file(am_path(state, "abort-safety"), 1, "%s", sha1_to_hex(head));
+		write_state_text(state, "abort-safety", sha1_to_hex(head));
 	else
-		write_file(am_path(state, "abort-safety"), 1, "%s", "");
+		write_state_text(state, "abort-safety", "");
 
 	state->cur++;
-	write_file(am_path(state, "next"), 1, "%d", state->cur);
+	write_state_count(state, "next", state->cur);
 }
 
 /**
@@ -1479,8 +1494,7 @@ static int parse_mail_rebase(struct am_state *state, const char *mail)
 	write_commit_patch(state, commit);
 
 	hashcpy(state->orig_commit, commit_sha1);
-	write_file(am_path(state, "original-commit"), 1, "%s",
-			sha1_to_hex(commit_sha1));
+	write_state_text(state, "original-commit", sha1_to_hex(commit_sha1));
 
 	return 0;
 }
@@ -1782,7 +1796,7 @@ static void am_run(struct am_state *state, int resume)
 	refresh_and_write_cache();
 
 	if (index_has_changes(&sb)) {
-		write_file(am_path(state, "dirtyindex"), 1, "t");
+		write_state_bool(state, "dirtyindex", 1);
 		die(_("Dirty index: cannot apply patches (dirty: %s)"), sb.buf);
 	}
 
-- 
2.5.0-568-g53a3e28

[PATCH 2/5] builtin/am: make sure state files are text

[Junio C Hamano]

We forgot to terminate the payload given to write_file() with LF,
resulting in files that ends with an incomplete line.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/am.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 4d34dc5..3423aa3 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -199,13 +199,13 @@ static inline const char *am_path(const struct am_state *state, const char *path
 static int write_state_text(const struct am_state *state,
 			    const char *name, const char *string)
 {
-	return write_file(am_path(state, name), 1, "%s", string);
+	return write_file(am_path(state, name), 1, "%s\n", string);
 }
 
 static int write_state_count(const struct am_state *state,
 			     const char *name, int value)
 {
-	return write_file(am_path(state, name), 1, "%d", value);
+	return write_file(am_path(state, name), 1, "%d\n", value);
 }
 
 static int write_state_bool(const struct am_state *state,
-- 
2.5.0-568-g53a3e28

[PATCH 3/5] write_file(): introduce an explicit WRITE_FILE_GENTLY request

[Junio C Hamano]

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 <gitster@pobox.com>
---
 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

Re: [PATCH 3/5] write_file(): introduce an explicit WRITE_FILE_GENTLY request

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> 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".

Re: [PATCH 3/5] write_file(): introduce an explicit WRITE_FILE_GENTLY request

[Duy Nguyen]

On Tue, Aug 25, 2015 at 1:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> 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

[PATCH] setup: update the right file in multiple checkouts

[Nguyễn Thái Ngọc Duy]

This code is introduced in 23af91d (prune: strategies for linked
checkouts - 2014-11-30), and it's supposed to implement this rule from
that commit's message:

 - linked checkouts are supposed to keep its location in $R/gitdir up
   to date. The use case is auto fixup after a manual checkout move.

Note the name, "$R/gitdir", not "$R/gitfile". Correct the path to be
updated accordingly.

While at there, make sure I/O errors are not silently dropped.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 The code was right in v2 [1] and became "gitfile" since v3 [2]. I
 need to reconsider my code quality after this :(

 [1] http://article.gmane.org/gmane.comp.version-control.git/239299
 [2] http://article.gmane.org/gmane.comp.version-control.git/242325

 setup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/setup.c b/setup.c
index 5f9f07d..64bf2b4 100644
--- a/setup.c
+++ b/setup.c
@@ -402,9 +402,9 @@ static void update_linked_gitdir(const char *gitfile, const char *gitdir)
 	struct strbuf path = STRBUF_INIT;
 	struct stat st;
 
-	strbuf_addf(&path, "%s/gitfile", gitdir);
+	strbuf_addf(&path, "%s/gitdir", 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, 1, "%s\n", gitfile);
 	strbuf_release(&path);
 }
 
-- 
2.3.0.rc1.137.g477eb31

Re: [PATCH] setup: update the right file in multiple checkouts

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> This code is introduced in 23af91d (prune: strategies for linked
> checkouts - 2014-11-30), and it's supposed to implement this rule from
> that commit's message:
>
>  - linked checkouts are supposed to keep its location in $R/gitdir up
>    to date. The use case is auto fixup after a manual checkout move.
>
> Note the name, "$R/gitdir", not "$R/gitfile". Correct the path to be
> updated accordingly.
>
> While at there, make sure I/O errors are not silently dropped.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  The code was right in v2 [1] and became "gitfile" since v3 [2]. I
>  need to reconsider my code quality after this :(

Heh, don't sweat it.  Everybody makes mistakes and sometimes becomes
sloppy.

Thanks for double checking and correcting.  Perhaps this could have
caught if we had some test coverage, I wonder?

>  [1] http://article.gmane.org/gmane.comp.version-control.git/239299
>  [2] http://article.gmane.org/gmane.comp.version-control.git/242325
>
>  setup.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index 5f9f07d..64bf2b4 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -402,9 +402,9 @@ static void update_linked_gitdir(const char *gitfile, const char *gitdir)
>  	struct strbuf path = STRBUF_INIT;
>  	struct stat st;
>  
> -	strbuf_addf(&path, "%s/gitfile", gitdir);
> +	strbuf_addf(&path, "%s/gitdir", 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, 1, "%s\n", gitfile);
>  	strbuf_release(&path);
>  }

Re: [PATCH] setup: update the right file in multiple checkouts

[Duy Nguyen]

On Tue, Aug 25, 2015 at 11:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> This code is introduced in 23af91d (prune: strategies for linked
>> checkouts - 2014-11-30), and it's supposed to implement this rule from
>> that commit's message:
>>
>>  - linked checkouts are supposed to keep its location in $R/gitdir up
>>    to date. The use case is auto fixup after a manual checkout move.
>>
>> Note the name, "$R/gitdir", not "$R/gitfile". Correct the path to be
>> updated accordingly.
>>
>> While at there, make sure I/O errors are not silently dropped.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  The code was right in v2 [1] and became "gitfile" since v3 [2]. I
>>  need to reconsider my code quality after this :(
>
> Heh, don't sweat it.  Everybody makes mistakes and sometimes becomes
> sloppy.
>
> Thanks for double checking and correcting.  Perhaps this could have
> caught if we had some test coverage, I wonder?

There's tests to check this prune functionality. They just don't
exercise this function. Instead they manipulate "gitdir" file
directly. I'll add a test to move repo around to make sure this code
is exercised in the test suite.
-- 
Duy

[PATCH 4/5] write_file(): do not leave incomplete line at the end

[Junio C Hamano]

All existing callers to this function use it to produce a text file
or an empty file, and a new callsite that mimick them must end their
payload with a LF.  If they forget to do so, the resulting file will
end with an incomplete line.

Introduce WRITE_FILE_BINARY flag bit, which no existing callers pass,
and unless that bit is set, make sure that write_file() adds an extra
LF at the end of an incomplete line as necessary.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h   | 1 +
 wrapper.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/cache.h b/cache.h
index f105235..dbfa4fa 100644
--- a/cache.h
+++ b/cache.h
@@ -1552,6 +1552,7 @@ static inline ssize_t write_str_in_full(int fd, const char *str)
  */
 #define WRITE_FILE_UNUSED_0 (1<<0)
 #define WRITE_FILE_GENTLY (1<<1)
+#define WRITE_FILE_BINARY (1<<2)
 __attribute__((format (printf, 3, 4)))
 extern int write_file(const char *path, unsigned flags, const char *fmt, ...);
 
diff --git a/wrapper.c b/wrapper.c
index 68d45b6..4cd2ca3 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -635,6 +635,9 @@ int write_file(const char *path, unsigned flags, const char *fmt, ...)
 	va_start(params, fmt);
 	strbuf_vaddf(&sb, fmt, params);
 	va_end(params);
+	if (!(flags & WRITE_FILE_BINARY))
+		strbuf_complete_line(&sb);
+
 	if (write_in_full(fd, sb.buf, sb.len) != sb.len) {
 		int err = errno;
 		close(fd);
-- 
2.5.0-568-g53a3e28

[PATCH 5/5] write_file(): clean up transitional mess of flag words and terminating LF

[Junio C Hamano]

Because the function adds necessary LF at the end of an incomplete
line for all callers that do not pass the WRITE_FILE_BINARY option,
and no caller of the function calls with that option, stop callers
to add LF at the end of the payload they pass to the function.

Also, change the callers that pass 1 to flags, that is now a no-op,
to pass 0.  In order to catch stray callers (and possible topics in
flight) that still pass 1 to ask the function to die upon error,
protect it with an assert().

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/am.c       |  4 ++--
 builtin/init-db.c  |  2 +-
 builtin/worktree.c | 10 +++++-----
 daemon.c           |  2 +-
 setup.c            |  2 +-
 submodule.c        |  2 +-
 transport.c        |  2 +-
 wrapper.c          |  7 ++++++-
 8 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 3423aa3..d804b12 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -199,13 +199,13 @@ static inline const char *am_path(const struct am_state *state, const char *path
 static int write_state_text(const struct am_state *state,
 			    const char *name, const char *string)
 {
-	return write_file(am_path(state, name), 1, "%s\n", string);
+	return write_file(am_path(state, name), 0, "%s", string);
 }
 
 static int write_state_count(const struct am_state *state,
 			     const char *name, int value)
 {
-	return write_file(am_path(state, name), 1, "%d\n", value);
+	return write_file(am_path(state, name), 0, "%d", value);
 }
 
 static int write_state_bool(const struct am_state *state,
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 49df78d..84d27b1 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -378,7 +378,7 @@ static void separate_git_dir(const char *git_dir)
 			die_errno(_("unable to move %s to %s"), src, git_dir);
 	}
 
-	write_file(git_link, 1, "gitdir: %s\n", git_dir);
+	write_file(git_link, 0, "gitdir: %s", git_dir);
 }
 
 int init_db(const char *template_dir, unsigned int flags)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 6a264ee..cc0981f 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -213,7 +213,7 @@ static int add_worktree(const char *path, const char **child_argv)
 	 * after the preparation is over.
 	 */
 	strbuf_addf(&sb, "%s/locked", sb_repo.buf);
-	write_file(sb.buf, 1, "initializing\n");
+	write_file(sb.buf, 0, "initializing");
 
 	strbuf_addf(&sb_git, "%s/.git", path);
 	if (safe_create_leading_directories_const(sb_git.buf))
@@ -223,8 +223,8 @@ static int add_worktree(const char *path, const char **child_argv)
 
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/gitdir", sb_repo.buf);
-	write_file(sb.buf, 1, "%s\n", real_path(sb_git.buf));
-	write_file(sb_git.buf, 1, "gitdir: %s/worktrees/%s\n",
+	write_file(sb.buf, 0, "%s", real_path(sb_git.buf));
+	write_file(sb_git.buf, 0, "gitdir: %s/worktrees/%s",
 		   real_path(get_git_common_dir()), name);
 	/*
 	 * This is to keep resolve_ref() happy. We need a valid HEAD
@@ -241,10 +241,10 @@ static int add_worktree(const char *path, const char **child_argv)
 		die(_("unable to resolve HEAD"));
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/HEAD", sb_repo.buf);
-	write_file(sb.buf, 1, "%s\n", sha1_to_hex(rev));
+	write_file(sb.buf, 0, "%s", sha1_to_hex(rev));
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
-	write_file(sb.buf, 1, "../..\n");
+	write_file(sb.buf, 0, "../..");
 
 	fprintf_ln(stderr, _("Enter %s (identifier %s)"), path, name);
 
diff --git a/daemon.c b/daemon.c
index d3d3e43..30a3fb4 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1376,7 +1376,7 @@ int main(int argc, char **argv)
 		sanitize_stdfds();
 
 	if (pid_file)
-		write_file(pid_file, 1, "%"PRIuMAX"\n", (uintmax_t) getpid());
+		write_file(pid_file, 0, "%"PRIuMAX, (uintmax_t) getpid());
 
 	/* prepare argv for serving-processes */
 	cld_argv = xmalloc(sizeof (char *) * (argc + 2));
diff --git a/setup.c b/setup.c
index 718f4e1..49675eb 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, WRITE_FILE_GENTLY, "%s\n", gitfile);
+		write_file(path.buf, WRITE_FILE_GENTLY, "%s", gitfile);
 	strbuf_release(&path);
 }
 
diff --git a/submodule.c b/submodule.c
index 700bbf4..c22fd04 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1103,7 +1103,7 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
 
 	/* Update gitfile */
 	strbuf_addf(&file_name, "%s/.git", work_tree);
-	write_file(file_name.buf, 1, "gitdir: %s\n",
+	write_file(file_name.buf, 0, "gitdir: %s",
 		   relative_path(git_dir, real_work_tree, &rel_path));
 
 	/* Update core.worktree setting */
diff --git a/transport.c b/transport.c
index e1821a4..e5638c0 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, WRITE_FILE_GENTLY, "%s\n", oid_to_hex(oid)))
+	    write_file(buf->buf, WRITE_FILE_GENTLY, "%s", 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 4cd2ca3..4f464ea 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -626,7 +626,12 @@ 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);
+	int fd;
+
+	if ((flags & WRITE_FILE_UNUSED_0))
+		die("BUG: write_file() called with bit 0 set");
+
+	fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0666);
 	if (fd < 0) {
 		if (fatal)
 			die_errno(_("could not open %s for writing"), path);
-- 
2.5.0-568-g53a3e28

Re: [PATCH 0/5] "am" state file fix with write_file() clean-up

[Jeff King]

On Mon, Aug 24, 2015 at 10:09:41AM -0700, Junio C Hamano wrote:

> So here is an solution based on the "write_file() is primarily to
> produce text, so it should be able to correct the incomplete line
> at the end" approach.

This all looks good to me. The topics-in-flight compatibility stuff in
patches 3 and 5	 is neatly done. Usually I would just cheat and change
the order of arguments to make the compiler notice such problems, but
that's hard to do here because of the varargs (you cannot just bump
"flags" to the end).

-Peff

Re: [PATCH 0/5] "am" state file fix with write_file() clean-up

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Aug 24, 2015 at 10:09:41AM -0700, Junio C Hamano wrote:
>
>> So here is an solution based on the "write_file() is primarily to
>> produce text, so it should be able to correct the incomplete line
>> at the end" approach.
>
> This all looks good to me. The topics-in-flight compatibility stuff in
> patches 3 and 5 is neatly done. Usually I would just cheat and change
> the order of arguments to make the compiler notice such problems, but
> that's hard to do here because of the varargs (you cannot just bump
> "flags" to the end).

Actually, I think my compatibility stuff is worthless.  It would not
catch new callers that wants to only probe and do their own error
handling by passing 0 (and besides, assert() is a shoddy way to do
this---there is no guarantee that tests will trigger all the
codepaths in the first place).

We should deprecate and remove write_file() by renaming the one with
the updated semantics to something else, possibly with a backward
compatiblity thin wrapper around it that is called write_file(), or
without it to force a link-time error.

Thanks for a dose of sanity.

Re: [PATCH 0/5] "am" state file fix with write_file() clean-up

[Jeff King]

On Mon, Aug 24, 2015 at 11:15:55AM -0700, Junio C Hamano wrote:

> > This all looks good to me. The topics-in-flight compatibility stuff in
> > patches 3 and 5 is neatly done. Usually I would just cheat and change
> > the order of arguments to make the compiler notice such problems, but
> > that's hard to do here because of the varargs (you cannot just bump
> > "flags" to the end).
> 
> Actually, I think my compatibility stuff is worthless.  It would not
> catch new callers that wants to only probe and do their own error
> handling by passing 0 (and besides, assert() is a shoddy way to do
> this---there is no guarantee that tests will trigger all the
> codepaths in the first place).

Oh, hrm, you're right. I was focused on making sure the common 1-passers
were not broken, but patch 3 does break 0-passers (obviously, because
they needed updated in the same patch ;) ).

And I do agree that build-time assertions are much better than run-time
ones.

> We should deprecate and remove write_file() by renaming the one with
> the updated semantics to something else, possibly with a backward
> compatiblity thin wrapper around it that is called write_file(), or
> without it to force a link-time error.

That sounds reasonable. Maybe "format_to_file" or something?

-Peff

Re: [PATCH 0/5] "am" state file fix with write_file() clean-up

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Aug 24, 2015 at 11:15:55AM -0700, Junio C Hamano wrote:
>
>> > This all looks good to me. The topics-in-flight compatibility stuff in
>> > patches 3 and 5 is neatly done. Usually I would just cheat and change
>> > the order of arguments to make the compiler notice such problems, but
>> > that's hard to do here because of the varargs (you cannot just bump
>> > "flags" to the end).
>> 
>> Actually, I think my compatibility stuff is worthless.  It would not
>> catch new callers that wants to only probe and do their own error
>> handling by passing 0 (and besides, assert() is a shoddy way to do
>> this---there is no guarantee that tests will trigger all the
>> codepaths in the first place).
>
> Oh, hrm, you're right. I was focused on making sure the common 1-passers
> were not broken, but patch 3 does break 0-passers (obviously, because
> they needed updated in the same patch ;) ).
>
> And I do agree that build-time assertions are much better than run-time
> ones.
>
>> We should deprecate and remove write_file() by renaming the one with
>> the updated semantics to something else, possibly with a backward
>> compatiblity thin wrapper around it that is called write_file(), or
>> without it to force a link-time error.
>
> That sounds reasonable. Maybe "format_to_file" or something?

I am going into a slightly different tangent.  Binary support is not
something we need right now, so I'll keep the door open for that in
the future by

 - drop the "int fatal" altogether from write_file() without adding
   "unsigned flags";

 - add write_file_gently(), again without "unsigned flags";

 - make them call write_file_v() that takes "unsigned flags" and
   va_list.

I earlier said there were 2 oddball callers, one of them being
suspicious, but it turns out that there are 3 callers that want
write_file_gently().  Only the one in the setup codepath is asking
for "fatal=0" while discarding the error return, which is
suspicious; other two are handling an error themselves and are OK.

[PATCH v2 0/6] "am" state file fix with write_file() clean-up

[Junio C Hamano]

"git am" was recently reimplemented in C.  While the implementation
was done conservatively and followed the original logic in the
scripted version fairly faithfully, the state files it left in the
$GIT_DIR/rebase-apply directory were made slightly different by
mistake---they lacked the final LF, leaving their last line
incomplete.

The patch [1/6] is Peff's idea to consolidate callers in "am", in a
more concrete form.

The patch [2/6] is the fix to the state files with incomplete lines.

The workhorse helper function that implements "we have this (short)
body of text; create a new file that contains it" has a "fatal"
parameter, to which 1 was passed by almost all callers, but to
casual readers, it was unclear what that 1 meant.  The patch [3/6]
splits it to write_file() and write_file_gently() and drops this
parameter that looks mysterious at the callsites.  A common helper
function write_file_v() is introduced to implement these two as thin
wrappers of it.

The patch [4/6] updates write_file_v() so that it does the "we are
writing a text file.  Make sure it does not end with an incomplete
line" logic that [2/6] added only to builtin/am.c, thusly reverting
what was done to builtin/am.c in [2/6].

The patch [5/6] stops all callers that creates a single-liner file
using write_file() and write_file_gently() from including the final
LF to the format they pass.  This should not change the behaviour,
but it probably makes it conceptually cleaner.  You have the contents
to be placed on a single line, and the helper turns the contents
into a proper "line".

The patch [6/6] drops the final LF from the parameter to create a
multi-line file; while this does not hurt in the sense that the
callee will add a necessary LF back, I do not think it should be
applied.  Conceptually, if you have a buffer that contains a bunch
of lines and throw it at a helper to create a file, you'd better
have the terminating LF yourself before asking the helper to put
them in the file.

Junio C Hamano (6):
  builtin/am: introduce write_state_*() helper functions
  builtin/am: make sure state files are text
  write_file(): drop "fatal" parameter
  write_file_v(): do not leave incomplete line at the end
  write_file(): drop caller-supplied LF from calls to create a one-liner
    file
  write_file(): drop caller-supplied LF from multi-line file

 builtin/am.c       | 69 ++++++++++++++++++++++++++++++++----------------------
 builtin/branch.c   |  4 ++--
 builtin/init-db.c  |  2 +-
 builtin/worktree.c | 10 ++++----
 cache.h            |  5 ++--
 daemon.c           |  2 +-
 setup.c            |  2 +-
 submodule.c        |  2 +-
 transport.c        |  2 +-
 wrapper.c          | 36 ++++++++++++++++++++++++----
 10 files changed, 88 insertions(+), 46 deletions(-)

-- 
2.5.0-568-g53a3e28

[PATCH v2 1/6] builtin/am: introduce write_state_*() helper functions

[Junio C Hamano]

There are many calls to write_file() that repeat the same pattern in
the implementation of the builtin version of "am".  They all share
the same traits, i.e they

 - produce a text file with a single string in it;

 - have enough information to produce the entire contents of that
   file;

 - generate the pathname of the file by making a call to am_path(); and

 - they ask write_file() to die() upon failure.

The slight differences among the call sites throw them into roughly
three categories:

 - many write either "t" or "f" based on a boolean value to a file;

 - some write the integer value in decimal text;

 - some others write more general string, e.g. an object name in
   hex, an empty string (i.e. the presense of the file itself serves
   as a flag), etc.

Introduce three helpers, write_state_bool(), write_state_count() and
write_state_text(), to reduce direct calls to write_file().

This is a preparatory step for the next step to ensure that no
"state" file this command leaves in $GIT_DIR is with an incomplete
line at the end.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/am.c | 68 ++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 41 insertions(+), 27 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 634f7a7..4d34dc5 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -194,6 +194,27 @@ static inline const char *am_path(const struct am_state *state, const char *path
 }
 
 /**
+ * For convenience to call write_file()
+ */
+static int write_state_text(const struct am_state *state,
+			    const char *name, const char *string)
+{
+	return write_file(am_path(state, name), 1, "%s", string);
+}
+
+static int write_state_count(const struct am_state *state,
+			     const char *name, int value)
+{
+	return write_file(am_path(state, name), 1, "%d", value);
+}
+
+static int write_state_bool(const struct am_state *state,
+			    const char *name, int value)
+{
+	return write_state_text(state, name, value ? "t" : "f");
+}
+
+/**
  * If state->quiet is false, calls fprintf(fp, fmt, ...), and appends a newline
  * at the end.
  */
@@ -362,7 +383,7 @@ static void write_author_script(const struct am_state *state)
 	sq_quote_buf(&sb, state->author_date);
 	strbuf_addch(&sb, '\n');
 
-	write_file(am_path(state, "author-script"), 1, "%s", sb.buf);
+	write_state_text(state, "author-script", sb.buf);
 
 	strbuf_release(&sb);
 }
@@ -1000,13 +1021,10 @@ 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, "quiet"), 1, state->quiet ? "t" : "f");
-
-	write_file(am_path(state, "sign"), 1, state->signoff ? "t" : "f");
-
-	write_file(am_path(state, "utf8"), 1, state->utf8 ? "t" : "f");
+	write_state_bool(state, "threeway", state->threeway);
+	write_state_bool(state, "quiet", state->quiet);
+	write_state_bool(state, "sign", state->signoff);
+	write_state_bool(state, "utf8", state->utf8);
 
 	switch (state->keep) {
 	case KEEP_FALSE:
@@ -1022,9 +1040,8 @@ 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, "messageid"), 1, state->message_id ? "t" : "f");
+	write_state_text(state, "keep", str);
+	write_state_bool(state, "messageid", state->message_id);
 
 	switch (state->scissors) {
 	case SCISSORS_UNSET:
@@ -1039,24 +1056,23 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,
 	default:
 		die("BUG: invalid value for state->scissors");
 	}
-
-	write_file(am_path(state, "scissors"), 1, "%s", str);
+	write_state_text(state, "scissors", str);
 
 	sq_quote_argv(&sb, state->git_apply_opts.argv, 0);
-	write_file(am_path(state, "apply-opt"), 1, "%s", sb.buf);
+	write_state_text(state, "apply-opt", sb.buf);
 
 	if (state->rebasing)
-		write_file(am_path(state, "rebasing"), 1, "%s", "");
+		write_state_text(state, "rebasing", "");
 	else
-		write_file(am_path(state, "applying"), 1, "%s", "");
+		write_state_text(state, "applying", "");
 
 	if (!get_sha1("HEAD", curr_head)) {
-		write_file(am_path(state, "abort-safety"), 1, "%s", sha1_to_hex(curr_head));
+		write_state_text(state, "abort-safety", sha1_to_hex(curr_head));
 		if (!state->rebasing)
 			update_ref("am", "ORIG_HEAD", curr_head, NULL, 0,
 					UPDATE_REFS_DIE_ON_ERR);
 	} else {
-		write_file(am_path(state, "abort-safety"), 1, "%s", "");
+		write_state_text(state, "abort-safety", "");
 		if (!state->rebasing)
 			delete_ref("ORIG_HEAD", NULL, 0);
 	}
@@ -1066,9 +1082,8 @@ 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, "last"), 1, "%d", state->last);
+	write_state_count(state, "next", state->cur);
+	write_state_count(state, "last", state->last);
 
 	strbuf_release(&sb);
 }
@@ -1101,12 +1116,12 @@ static void am_next(struct am_state *state)
 	unlink(am_path(state, "original-commit"));
 
 	if (!get_sha1("HEAD", head))
-		write_file(am_path(state, "abort-safety"), 1, "%s", sha1_to_hex(head));
+		write_state_text(state, "abort-safety", sha1_to_hex(head));
 	else
-		write_file(am_path(state, "abort-safety"), 1, "%s", "");
+		write_state_text(state, "abort-safety", "");
 
 	state->cur++;
-	write_file(am_path(state, "next"), 1, "%d", state->cur);
+	write_state_count(state, "next", state->cur);
 }
 
 /**
@@ -1479,8 +1494,7 @@ static int parse_mail_rebase(struct am_state *state, const char *mail)
 	write_commit_patch(state, commit);
 
 	hashcpy(state->orig_commit, commit_sha1);
-	write_file(am_path(state, "original-commit"), 1, "%s",
-			sha1_to_hex(commit_sha1));
+	write_state_text(state, "original-commit", sha1_to_hex(commit_sha1));
 
 	return 0;
 }
@@ -1782,7 +1796,7 @@ static void am_run(struct am_state *state, int resume)
 	refresh_and_write_cache();
 
 	if (index_has_changes(&sb)) {
-		write_file(am_path(state, "dirtyindex"), 1, "t");
+		write_state_bool(state, "dirtyindex", 1);
 		die(_("Dirty index: cannot apply patches (dirty: %s)"), sb.buf);
 	}
 
-- 
2.5.0-568-g53a3e28

[PATCH v2 2/6] builtin/am: make sure state files are text

[Junio C Hamano]

We forgot to terminate the payload given to write_file() with LF,
resulting in files that end with an incomplete line.  Teach the
wrappers builtin/am uses to make sure it adds LF at the end as
necessary.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/am.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 4d34dc5..f0a046b 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -199,13 +199,19 @@ static inline const char *am_path(const struct am_state *state, const char *path
 static int write_state_text(const struct am_state *state,
 			    const char *name, const char *string)
 {
-	return write_file(am_path(state, name), 1, "%s", string);
+	const char *fmt;
+
+	if (*string && string[strlen(string) - 1] != '\n')
+		fmt = "%s\n";
+	else
+		fmt = "%s";
+	return write_file(am_path(state, name), 1, fmt, string);
 }
 
 static int write_state_count(const struct am_state *state,
 			     const char *name, int value)
 {
-	return write_file(am_path(state, name), 1, "%d", value);
+	return write_file(am_path(state, name), 1, "%d\n", value);
 }
 
 static int write_state_bool(const struct am_state *state,
-- 
2.5.0-568-g53a3e28

Re: [PATCH v2 2/6] builtin/am: make sure state files are text

[Jeff King]

On Mon, Aug 24, 2015 at 01:58:06PM -0700, Junio C Hamano wrote:

> We forgot to terminate the payload given to write_file() with LF,
> resulting in files that end with an incomplete line.  Teach the
> wrappers builtin/am uses to make sure it adds LF at the end as
> necessary.

Is it even worth doing this step? It's completely reverted later in the
series. I understand that we do not want to hold the fix to git-am
hostage to write_file refactoring, but I don't see any reason these
cannot all graduate as part of the same topic.

Ignore me if you really are planning on doing the first two to "maint"
and holding the others back for "master".

-Peff

Re: [PATCH v2 2/6] builtin/am: make sure state files are text

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Aug 24, 2015 at 01:58:06PM -0700, Junio C Hamano wrote:
>
>> We forgot to terminate the payload given to write_file() with LF,
>> resulting in files that end with an incomplete line.  Teach the
>> wrappers builtin/am uses to make sure it adds LF at the end as
>> necessary.
>
> Is it even worth doing this step? It's completely reverted later in the
> series. I understand that we do not want to hold the fix to git-am
> hostage to write_file refactoring, but I don't see any reason these
> cannot all graduate as part of the same topic.
>
> Ignore me if you really are planning on doing the first two to "maint"
> and holding the others back for "master".

Not really.  The primary reason why this step exists and 1-2/6 make
a sufficient fix by themselves is because I wasn't even sure if
3-6/6 were worth doing.

As to "flags exposed to callers" vs "with and without gently", when
we change the system to allow new modes of operations (e.g. somebody
wants to write a binary file, or allocate more flag bits for their
special case), I'd expect that we'd add a more general and verbose
"write_file_with_options(path, flags, fmt, ...)"), gain experience
with that function, and then possibly introduce canned thin wrappers
(e.g. write_binary_file() that is a synonym to passing BINARY but
not GENTLY) if the new thing proves widely useful, just like I left
write_file() and write_file_gently() in as fairly common things to
do.

Re: [PATCH v2 2/6] builtin/am: make sure state files are text

[Jeff King]

On Tue, Aug 25, 2015 at 09:19:13AM -0700, Junio C Hamano wrote:

> As to "flags exposed to callers" vs "with and without gently", when
> we change the system to allow new modes of operations (e.g. somebody
> wants to write a binary file, or allocate more flag bits for their
> special case), I'd expect that we'd add a more general and verbose
> "write_file_with_options(path, flags, fmt, ...)"), gain experience
> with that function, and then possibly introduce canned thin wrappers
> (e.g. write_binary_file() that is a synonym to passing BINARY but
> not GENTLY) if the new thing proves widely useful, just like I left
> write_file() and write_file_gently() in as fairly common things to
> do.

Yeah, that works. It is a bit of a gamble to me. If we never add a lot
more options, the end result is much nicer (callers do not deal with the
flag option at all). But if we do, we end up with the mess that
get_sha1_with_* and add_pending_object() got into.

One can always refactor later, too.  In that sense, the BINARY flag is
not useful (nobody uses it, and we do not plan to do so). We could just
make write_file_v unconditionally complete lines[1].

But I'm OK with what you posted, as well. I think this interface is not
worth spending a lot of time micro-nit-picking.

-Peff

[1] In fact, I'd be surprised if this function works well for non-text
    data anyway, as it relies on printf-style formatting. You cannot use
    it to write a string with embedded NULs, for example.

    If we wanted to support that case, we would probably break out:

        int write_buf_to_file(const char *filename,
	                      const char *buf, size_t len);

    as a thin wrapper for open/write_in_full/close. And then write_to_file()
    would format into a strbuf, complete a newline, and pass the result
    to it.

Re: [PATCH v2 2/6] builtin/am: make sure state files are text

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Aug 25, 2015 at 09:19:13AM -0700, Junio C Hamano wrote:
>
>> As to "flags exposed to callers" vs "with and without gently", when
>> we change the system to allow new modes of operations (e.g. somebody
>> wants to write a binary file, or allocate more flag bits for their
>> special case), I'd expect that we'd add a more general and verbose
>> "write_file_with_options(path, flags, fmt, ...)"), gain experience
>> with that function, and then possibly introduce canned thin wrappers
>> (e.g. write_binary_file() that is a synonym to passing BINARY but
>> not GENTLY) if the new thing proves widely useful, just like I left
>> write_file() and write_file_gently() in as fairly common things to
>> do.
>
> Yeah, that works. It is a bit of a gamble to me. If we never add a lot
> more options, the end result is much nicer (callers do not deal with the
> flag option at all). But if we do, we end up with the mess that
> get_sha1_with_* and add_pending_object() got into.

Yeah.  I do not know.  Perhaps a good intermim solution for now
would be to make

  - write_file_l(path, flags, fmt, ...);

the low-level helper, with a single convenience wrapper:

  - write_file(path, fmt, ...)

for everybody other than two "gently" ones to use.  Two "gently"
ones can call write_file_l(path, WRITE_FILE_GENTLY, fmt,...).

You are right about binary stuff.  You could do fmt="...%c..."  and
pass '\0' to corresponding place if your NUL is in the fixed part of
the data you are generating, but otherwise write_file() interface is
a very useful way to handle binary.

[PATCH v2 3/6] write_file(): drop "fatal" parameter

[Junio C Hamano]

All callers except three passed 1 for the "fatal" parameter to ask
this function to die upon error, but to a casual reader of the code,
it was not all obvious what that 1 meant.  Instead, split the
function into two based on a common write_file_v() that takes the
flag, introduce write_file_gently() as a new way to attempt creating
a file without dying on error, and make three callers to call it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/am.c       |  4 ++--
 builtin/branch.c   |  2 +-
 builtin/init-db.c  |  2 +-
 builtin/worktree.c | 10 +++++-----
 cache.h            |  5 +++--
 daemon.c           |  2 +-
 setup.c            |  2 +-
 submodule.c        |  2 +-
 transport.c        |  2 +-
 wrapper.c          | 28 ++++++++++++++++++++++++----
 10 files changed, 40 insertions(+), 19 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index f0a046b..9c57677 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -205,13 +205,13 @@ static int write_state_text(const struct am_state *state,
 		fmt = "%s\n";
 	else
 		fmt = "%s";
-	return write_file(am_path(state, name), 1, fmt, string);
+	return write_file(am_path(state, name), fmt, string);
 }
 
 static int write_state_count(const struct am_state *state,
 			     const char *name, int value)
 {
-	return write_file(am_path(state, name), 1, "%d\n", value);
+	return write_file(am_path(state, name), "%d\n", value);
 }
 
 static int write_state_bool(const struct am_state *state,
diff --git a/builtin/branch.c b/builtin/branch.c
index 58aa84f..ff05869 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -776,7 +776,7 @@ static int edit_branch_description(const char *branch_name)
 		    "  %s\n"
 		    "Lines starting with '%c' will be stripped.\n",
 		    branch_name, comment_line_char);
-	if (write_file(git_path(edit_description), 0, "%s", buf.buf)) {
+	if (write_file_gently(git_path(edit_description), "%s", buf.buf)) {
 		strbuf_release(&buf);
 		return error(_("could not write branch description template: %s"),
 			     strerror(errno));
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 49df78d..bfe1d08 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -378,7 +378,7 @@ static void separate_git_dir(const char *git_dir)
 			die_errno(_("unable to move %s to %s"), src, git_dir);
 	}
 
-	write_file(git_link, 1, "gitdir: %s\n", git_dir);
+	write_file(git_link, "gitdir: %s\n", git_dir);
 }
 
 int init_db(const char *template_dir, unsigned int flags)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 6a264ee..368502d 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -213,7 +213,7 @@ static int add_worktree(const char *path, const char **child_argv)
 	 * after the preparation is over.
 	 */
 	strbuf_addf(&sb, "%s/locked", sb_repo.buf);
-	write_file(sb.buf, 1, "initializing\n");
+	write_file(sb.buf, "initializing\n");
 
 	strbuf_addf(&sb_git, "%s/.git", path);
 	if (safe_create_leading_directories_const(sb_git.buf))
@@ -223,8 +223,8 @@ static int add_worktree(const char *path, const char **child_argv)
 
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/gitdir", sb_repo.buf);
-	write_file(sb.buf, 1, "%s\n", real_path(sb_git.buf));
-	write_file(sb_git.buf, 1, "gitdir: %s/worktrees/%s\n",
+	write_file(sb.buf, "%s\n", real_path(sb_git.buf));
+	write_file(sb_git.buf, "gitdir: %s/worktrees/%s\n",
 		   real_path(get_git_common_dir()), name);
 	/*
 	 * This is to keep resolve_ref() happy. We need a valid HEAD
@@ -241,10 +241,10 @@ static int add_worktree(const char *path, const char **child_argv)
 		die(_("unable to resolve HEAD"));
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/HEAD", sb_repo.buf);
-	write_file(sb.buf, 1, "%s\n", sha1_to_hex(rev));
+	write_file(sb.buf, "%s\n", sha1_to_hex(rev));
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
-	write_file(sb.buf, 1, "../..\n");
+	write_file(sb.buf, "../..\n");
 
 	fprintf_ln(stderr, _("Enter %s (identifier %s)"), path, name);
 
diff --git a/cache.h b/cache.h
index 6bb7119..3f79e6b 100644
--- a/cache.h
+++ b/cache.h
@@ -1539,8 +1539,9 @@ static inline ssize_t write_str_in_full(int fd, const char *str)
 {
 	return write_in_full(fd, str, strlen(str));
 }
-__attribute__((format (printf, 3, 4)))
-extern int write_file(const char *path, int fatal, const char *fmt, ...);
+
+extern int write_file(const char *path, const char *fmt, ...);
+extern int write_file_gently(const char *path, const char *fmt, ...);
 
 /* pager.c */
 extern void setup_pager(void);
diff --git a/daemon.c b/daemon.c
index d3d3e43..9154509 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1376,7 +1376,7 @@ int main(int argc, char **argv)
 		sanitize_stdfds();
 
 	if (pid_file)
-		write_file(pid_file, 1, "%"PRIuMAX"\n", (uintmax_t) getpid());
+		write_file(pid_file, "%"PRIuMAX"\n", (uintmax_t) getpid());
 
 	/* prepare argv for serving-processes */
 	cld_argv = xmalloc(sizeof (char *) * (argc + 2));
diff --git a/setup.c b/setup.c
index 5f9f07d..feb8565 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_gently(path.buf, "%s\n", gitfile);
 	strbuf_release(&path);
 }
 
diff --git a/submodule.c b/submodule.c
index 700bbf4..5519f11 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1103,7 +1103,7 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
 
 	/* Update gitfile */
 	strbuf_addf(&file_name, "%s/.git", work_tree);
-	write_file(file_name.buf, 1, "gitdir: %s\n",
+	write_file(file_name.buf, "gitdir: %s\n",
 		   relative_path(git_dir, real_work_tree, &rel_path));
 
 	/* Update core.worktree setting */
diff --git a/transport.c b/transport.c
index 40692f8..0254394 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_gently(buf->buf, "%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..8c8925b 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -621,19 +621,17 @@ char *xgetcwd(void)
 	return strbuf_detach(&sb, NULL);
 }
 
-int write_file(const char *path, int fatal, const char *fmt, ...)
+static int write_file_v(const char *path, int fatal,
+			const char *fmt, va_list params)
 {
 	struct strbuf sb = STRBUF_INIT;
-	va_list params;
 	int fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0666);
 	if (fd < 0) {
 		if (fatal)
 			die_errno(_("could not open %s for writing"), path);
 		return -1;
 	}
-	va_start(params, fmt);
 	strbuf_vaddf(&sb, fmt, params);
-	va_end(params);
 	if (write_in_full(fd, sb.buf, sb.len) != sb.len) {
 		int err = errno;
 		close(fd);
@@ -652,6 +650,28 @@ int write_file(const char *path, int fatal, const char *fmt, ...)
 	return 0;
 }
 
+int write_file(const char *path, const char *fmt, ...)
+{
+	int status;
+	va_list params;
+
+	va_start(params, fmt);
+	status = write_file_v(path, 1, fmt, params);
+	va_end(params);
+	return status;
+}
+
+int write_file_gently(const char *path, const char *fmt, ...)
+{
+	int status;
+	va_list params;
+
+	va_start(params, fmt);
+	status = write_file_v(path, 0, fmt, params);
+	va_end(params);
+	return status;
+}
+
 void sleep_millisec(int millisec)
 {
 	poll(NULL, 0, millisec);
-- 
2.5.0-568-g53a3e28

[PATCH v2 4/6] write_file_v(): do not leave incomplete line at the end

[Junio C Hamano]

All existing callers to this function use it to produce a text file
or an empty file, and a new callsite that mimick them must end their
payload with a LF.  If they forget to do so, the resulting file will
end with an incomplete line.

Introduce WRITE_FILE_BINARY flag bit, which no existing callers
pass, and unless that bit is set, make sure that write_file_v() adds
an extra LF at the end of an incomplete line as necessary.

With this, the caller-side fix in builtin/am.c becomes unnecessary.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/am.c | 10 ++--------
 wrapper.c    | 14 +++++++++++---
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 9c57677..486ff59 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -199,19 +199,13 @@ static inline const char *am_path(const struct am_state *state, const char *path
 static int write_state_text(const struct am_state *state,
 			    const char *name, const char *string)
 {
-	const char *fmt;
-
-	if (*string && string[strlen(string) - 1] != '\n')
-		fmt = "%s\n";
-	else
-		fmt = "%s";
-	return write_file(am_path(state, name), fmt, string);
+	return write_file(am_path(state, name), "%s", string);
 }
 
 static int write_state_count(const struct am_state *state,
 			     const char *name, int value)
 {
-	return write_file(am_path(state, name), "%d\n", value);
+	return write_file(am_path(state, name), "%d", value);
 }
 
 static int write_state_bool(const struct am_state *state,
diff --git a/wrapper.c b/wrapper.c
index 8c8925b..db39e1b 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -621,17 +621,25 @@ char *xgetcwd(void)
 	return strbuf_detach(&sb, NULL);
 }
 
-static int write_file_v(const char *path, int fatal,
+
+#define WRITE_FILE_GENTLY (1 << 0)
+#define WRITE_FILE_BINARY (1 << 1)
+
+static int write_file_v(const char *path, unsigned flags,
 			const char *fmt, va_list params)
 {
+	int fatal = !(flags & WRITE_FILE_GENTLY);
 	struct strbuf sb = STRBUF_INIT;
 	int fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0666);
+
 	if (fd < 0) {
 		if (fatal)
 			die_errno(_("could not open %s for writing"), path);
 		return -1;
 	}
 	strbuf_vaddf(&sb, fmt, params);
+	if (!(flags & WRITE_FILE_BINARY))
+		strbuf_complete_line(&sb);
 	if (write_in_full(fd, sb.buf, sb.len) != sb.len) {
 		int err = errno;
 		close(fd);
@@ -656,7 +664,7 @@ int write_file(const char *path, const char *fmt, ...)
 	va_list params;
 
 	va_start(params, fmt);
-	status = write_file_v(path, 1, fmt, params);
+	status = write_file_v(path, 0, fmt, params);
 	va_end(params);
 	return status;
 }
@@ -667,7 +675,7 @@ int write_file_gently(const char *path, const char *fmt, ...)
 	va_list params;
 
 	va_start(params, fmt);
-	status = write_file_v(path, 0, fmt, params);
+	status = write_file_v(path, WRITE_FILE_GENTLY, fmt, params);
 	va_end(params);
 	return status;
 }
-- 
2.5.0-568-g53a3e28

[PATCH v2 5/6] write_file(): drop caller-supplied LF from calls to create a one-liner file

[Junio C Hamano]

All of the callsites covered by this change call write_file() or
write_file_gently() to create a one-liner file.  Drop the caller
supplied LF and let these callees to append it as necessary.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/init-db.c  |  2 +-
 builtin/worktree.c | 10 +++++-----
 daemon.c           |  2 +-
 setup.c            |  2 +-
 submodule.c        |  2 +-
 transport.c        |  2 +-
 6 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index bfe1d08..69323e1 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -378,7 +378,7 @@ static void separate_git_dir(const char *git_dir)
 			die_errno(_("unable to move %s to %s"), src, git_dir);
 	}
 
-	write_file(git_link, "gitdir: %s\n", git_dir);
+	write_file(git_link, "gitdir: %s", git_dir);
 }
 
 int init_db(const char *template_dir, unsigned int flags)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 368502d..bbb169a 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -213,7 +213,7 @@ static int add_worktree(const char *path, const char **child_argv)
 	 * after the preparation is over.
 	 */
 	strbuf_addf(&sb, "%s/locked", sb_repo.buf);
-	write_file(sb.buf, "initializing\n");
+	write_file(sb.buf, "initializing");
 
 	strbuf_addf(&sb_git, "%s/.git", path);
 	if (safe_create_leading_directories_const(sb_git.buf))
@@ -223,8 +223,8 @@ static int add_worktree(const char *path, const char **child_argv)
 
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/gitdir", sb_repo.buf);
-	write_file(sb.buf, "%s\n", real_path(sb_git.buf));
-	write_file(sb_git.buf, "gitdir: %s/worktrees/%s\n",
+	write_file(sb.buf, "%s", real_path(sb_git.buf));
+	write_file(sb_git.buf, "gitdir: %s/worktrees/%s",
 		   real_path(get_git_common_dir()), name);
 	/*
 	 * This is to keep resolve_ref() happy. We need a valid HEAD
@@ -241,10 +241,10 @@ static int add_worktree(const char *path, const char **child_argv)
 		die(_("unable to resolve HEAD"));
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/HEAD", sb_repo.buf);
-	write_file(sb.buf, "%s\n", sha1_to_hex(rev));
+	write_file(sb.buf, "%s", sha1_to_hex(rev));
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
-	write_file(sb.buf, "../..\n");
+	write_file(sb.buf, "../..");
 
 	fprintf_ln(stderr, _("Enter %s (identifier %s)"), path, name);
 
diff --git a/daemon.c b/daemon.c
index 9154509..f9eb296 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1376,7 +1376,7 @@ int main(int argc, char **argv)
 		sanitize_stdfds();
 
 	if (pid_file)
-		write_file(pid_file, "%"PRIuMAX"\n", (uintmax_t) getpid());
+		write_file(pid_file, "%"PRIuMAX, (uintmax_t) getpid());
 
 	/* prepare argv for serving-processes */
 	cld_argv = xmalloc(sizeof (char *) * (argc + 2));
diff --git a/setup.c b/setup.c
index feb8565..a206781 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_gently(path.buf, "%s\n", gitfile);
+		write_file_gently(path.buf, "%s", gitfile);
 	strbuf_release(&path);
 }
 
diff --git a/submodule.c b/submodule.c
index 5519f11..4549c1b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1103,7 +1103,7 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
 
 	/* Update gitfile */
 	strbuf_addf(&file_name, "%s/.git", work_tree);
-	write_file(file_name.buf, "gitdir: %s\n",
+	write_file(file_name.buf, "gitdir: %s",
 		   relative_path(git_dir, real_work_tree, &rel_path));
 
 	/* Update core.worktree setting */
diff --git a/transport.c b/transport.c
index 0254394..788cf20 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_gently(buf->buf, "%s\n", oid_to_hex(oid)))
+	    write_file_gently(buf->buf, "%s", oid_to_hex(oid)))
 		return error("problems writing temporary file %s: %s",
 			     buf->buf, strerror(errno));
 	strbuf_setlen(buf, len);
-- 
2.5.0-568-g53a3e28

[PATCH v2 6/6] write_file(): drop caller-supplied LF from multi-line file

[Junio C Hamano]

This is just to illustrate that we _could_ do this; I think it is
better to leave these places as they are.  The primary thing we
wanted to do with the automatic addition of LF to an incomplete line
was to make it easier to write a caller that creates a single-liner
file.  For callers that want to fully create a multi-line input, the
resulting code is easier to see if we let them continue to do so.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/am.c     | 1 -
 builtin/branch.c | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 486ff59..c544091 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -381,7 +381,6 @@ static void write_author_script(const struct am_state *state)
 
 	strbuf_addstr(&sb, "GIT_AUTHOR_DATE=");
 	sq_quote_buf(&sb, state->author_date);
-	strbuf_addch(&sb, '\n');
 
 	write_state_text(state, "author-script", sb.buf);
 
diff --git a/builtin/branch.c b/builtin/branch.c
index ff05869..cdf7f13 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -774,7 +774,7 @@ static int edit_branch_description(const char *branch_name)
 	strbuf_commented_addf(&buf,
 		    "Please edit the description for the branch\n"
 		    "  %s\n"
-		    "Lines starting with '%c' will be stripped.\n",
+		    "Lines starting with '%c' will be stripped.",
 		    branch_name, comment_line_char);
 	if (write_file_gently(git_path(edit_description), "%s", buf.buf)) {
 		strbuf_release(&buf);
-- 
2.5.0-568-g53a3e28

Re: [PATCH v2 0/6] "am" state file fix with write_file() clean-up

[Jeff King]

On Mon, Aug 24, 2015 at 01:58:04PM -0700, Junio C Hamano wrote:

> The workhorse helper function that implements "we have this (short)
> body of text; create a new file that contains it" has a "fatal"
> parameter, to which 1 was passed by almost all callers, but to
> casual readers, it was unclear what that 1 meant.  The patch [3/6]
> splits it to write_file() and write_file_gently() and drops this
> parameter that looks mysterious at the callsites.  A common helper
> function write_file_v() is introduced to implement these two as thin
> wrappers of it.

To be honest, I think the "flags" field is more maintainable going
forward. Now you have _two_ functions, and any features you add to them
have to go in both places. In 4/6 you add the WRITE_FILE_BINARY flag,
but I notice that callers can't actually pass it. And adding it into
write_file() would take us back to square-one with source compatibility.

> The patch [4/6] updates write_file_v() so that it does the "we are
> writing a text file.  Make sure it does not end with an incomplete
> line" logic that [2/6] added only to builtin/am.c, thusly reverting
> what was done to builtin/am.c in [2/6].

I notice this also converts "fatal" to "flags". It seemed weird to me
that did not go into patch 3, but I guess it is OK (we know that
write_file_v has no outstanding callers, since we just added it).

> The patch [5/6] stops all callers that creates a single-liner file
> using write_file() and write_file_gently() from including the final
> LF to the format they pass.  This should not change the behaviour,
> but it probably makes it conceptually cleaner.  You have the contents
> to be placed on a single line, and the helper turns the contents
> into a proper "line".

Nice.

> The patch [6/6] drops the final LF from the parameter to create a
> multi-line file; while this does not hurt in the sense that the
> callee will add a necessary LF back, I do not think it should be
> applied.  Conceptually, if you have a buffer that contains a bunch
> of lines and throw it at a helper to create a file, you'd better
> have the terminating LF yourself before asking the helper to put
> them in the file.

I agree we should drop this one.

-Peff

Re: [PATCH] am: terminate state files with a newline

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 994 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 835 bytes --]