{"thread":{"id":"40139","subject":"Minor builtin 'git am' side-effect","startedAt":"2015-08-20T13:22:47Z","lastAt":"2015-08-31T10:29:05Z","messageCount":36,"participants":["SZEDER Gábor","Junio C Hamano","Paul Tan","Jeff King","brian m. carlson","Duy Nguyen","Nguyễn Thái Ngọc Duy"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"268362","messageId":"20150820152247.Horde.3yFLIbhFFocB99yz8o1iwg1@webmail.informatik.kit.edu","threadId":"40139","inReplyTo":null,"subject":"Minor builtin 'git am' side-effect","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-08-20T13:22:47Z","receivedAt":"2015-08-20T13:22:47Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nHi,\n\nThe format of the files '.git/rebase-apply/{next,last}' changed slightly\nwith the recent builtin 'git am' conversion: while these files were\nnewline-terminated when written by the scripted version, the ones written\nby the builtin are not.\n\nThis probably makes no difference for shell scripts looking at these\nfiles, e.g. our __git_ps1() handles both just fine. However, it can\nbreak C programs, when after strtol()ing the contents of the files they\nget defensive and check for the terminating newline at *endptr (this is\nhow I noticed, as one of my pet projects did just that).\n\nI'm not saying that the new behavior is bad and should be fixed; I merely\npoint it out and leave the rest for you to decide.\n\nBest,\nGábor\n"},{"id":"268383","messageId":"xmqqa8tl7qi3.fsf@gitster.dls.corp.google.com","threadId":"40139","inReplyTo":"20150820152247.Horde.3yFLIbhFFocB99yz8o1iwg1@webmail.informatik.kit.edu","subject":"Re: Minor builtin 'git am' side-effect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-20T18:40:20Z","receivedAt":"2015-08-20T18:40:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> The format of the files '.git/rebase-apply/{next,last}' changed slightly\n> with the recent builtin 'git am' conversion: while these files were\n> newline-terminated when written by the scripted version, the ones written\n> by the builtin are not.\n\nThanks for noticing; that should be corrected, I think.\n"},{"id":"268522","messageId":"20150823055053.GA15849@yoshi.chippynet.com","threadId":"40139","inReplyTo":"xmqqa8tl7qi3.fsf@gitster.dls.corp.google.com","subject":"[PATCH] am: terminate state files with a newline","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-08-23T05:50:53Z","receivedAt":"2015-08-23T05:50:53Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Thu, Aug 20, 2015 at 11:40:20AM -0700, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n> \n> > The format of the files '.git/rebase-apply/{next,last}' changed slightly\n> > with the recent builtin 'git am' conversion: while these files were\n> > newline-terminated when written by the scripted version, the ones written\n> > by the builtin are not.\n> \n> Thanks for noticing; that should be corrected, I think.\n\nOkay then, this patch should correct this.\n\nDid we ever explictly allow external programs to poke around the\ncontents of the .git/rebase-apply directory? I think it may not be so\ngood, as it means that it may not be possible to switch the storage\nformat in the future (e.g. to allow atomic modifications, maybe?) :-/ .\n\nRegards,\nPaul\n\n-- >8 --\nSubject: [PATCH] am: terminate state files with a newline\n\nSince builtin/am.c replaced git-am.sh in 783d7e8 (builtin-am: remove\nredirection to git-am.sh, 2015-08-04), the state files written by git-am\ndid not terminate with a newline.\n\nThis is because the code in builtin/am.c did not write the newline to\nthe state files.\n\nWhile the git codebase has no problems with the missing newline,\nexternal software which read the contents of the state directory may be\nstrict about the existence of the terminating newline, and would thus\nbreak.\n\nFix this by correcting the relevant calls to write_file() to ensure that\nthe state files written terminate with a newline, matching how git-am.sh\nbehaves.\n\nWhile we are fixing the write_file() calls, fix the writing of the\n\"dirtyindex\" file as well -- we should be creating an empty file to\nmatch the behavior of git-am.sh.\n\nReported-by: SZEDER Gábor \nSigned-off-by: Paul Tan \n---\n builtin/am.c | 30 +++++++++++++++---------------\n 1 file changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 1399c8d..2e57fad 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -994,13 +994,13 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \tif (state->rebasing)\n \t\tstate->threeway = 1;\n \n-\twrite_file(am_path(state, \"threeway\"), 1, state->threeway ? \"t\" : \"f\");\n+\twrite_file(am_path(state, \"threeway\"), 1, \"%s\\n\", state->threeway ? \"t\" : \"f\");\n \n-\twrite_file(am_path(state, \"quiet\"), 1, state->quiet ? \"t\" : \"f\");\n+\twrite_file(am_path(state, \"quiet\"), 1, \"%s\\n\", state->quiet ? \"t\" : \"f\");\n \n-\twrite_file(am_path(state, \"sign\"), 1, state->signoff ? \"t\" : \"f\");\n+\twrite_file(am_path(state, \"sign\"), 1, \"%s\\n\", state->signoff ? \"t\" : \"f\");\n \n-\twrite_file(am_path(state, \"utf8\"), 1, state->utf8 ? \"t\" : \"f\");\n+\twrite_file(am_path(state, \"utf8\"), 1, \"%s\\n\", state->utf8 ? \"t\" : \"f\");\n \n \tswitch (state->keep) {\n \tcase KEEP_FALSE:\n@@ -1016,9 +1016,9 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \t\tdie(\"BUG: invalid value for state->keep\");\n \t}\n \n-\twrite_file(am_path(state, \"keep\"), 1, \"%s\", str);\n+\twrite_file(am_path(state, \"keep\"), 1, \"%s\\n\", str);\n \n-\twrite_file(am_path(state, \"messageid\"), 1, state->message_id ? \"t\" : \"f\");\n+\twrite_file(am_path(state, \"messageid\"), 1, \"%s\\n\", state->message_id ? \"t\" : \"f\");\n \n \tswitch (state->scissors) {\n \tcase SCISSORS_UNSET:\n@@ -1034,10 +1034,10 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \t\tdie(\"BUG: invalid value for state->scissors\");\n \t}\n \n-\twrite_file(am_path(state, \"scissors\"), 1, \"%s\", str);\n+\twrite_file(am_path(state, \"scissors\"), 1, \"%s\\n\", str);\n \n \tsq_quote_argv(&sb, state->git_apply_opts.argv, 0);\n-\twrite_file(am_path(state, \"apply-opt\"), 1, \"%s\", sb.buf);\n+\twrite_file(am_path(state, \"apply-opt\"), 1, \"%s\\n\", sb.buf);\n \n \tif (state->rebasing)\n \t\twrite_file(am_path(state, \"rebasing\"), 1, \"%s\", \"\");\n@@ -1045,7 +1045,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \t\twrite_file(am_path(state, \"applying\"), 1, \"%s\", \"\");\n \n \tif (!get_sha1(\"HEAD\", curr_head)) {\n-\t\twrite_file(am_path(state, \"abort-safety\"), 1, \"%s\", sha1_to_hex(curr_head));\n+\t\twrite_file(am_path(state, \"abort-safety\"), 1, \"%s\\n\", sha1_to_hex(curr_head));\n \t\tif (!state->rebasing)\n \t\t\tupdate_ref(\"am\", \"ORIG_HEAD\", curr_head, NULL, 0,\n \t\t\t\t\tUPDATE_REFS_DIE_ON_ERR);\n@@ -1060,9 +1060,9 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \t * session is in progress, they should be written last.\n \t */\n \n-\twrite_file(am_path(state, \"next\"), 1, \"%d\", state->cur);\n+\twrite_file(am_path(state, \"next\"), 1, \"%d\\n\", state->cur);\n \n-\twrite_file(am_path(state, \"last\"), 1, \"%d\", state->last);\n+\twrite_file(am_path(state, \"last\"), 1, \"%d\\n\", state->last);\n \n \tstrbuf_release(&sb);\n }\n@@ -1095,12 +1095,12 @@ static void am_next(struct am_state *state)\n \tunlink(am_path(state, \"original-commit\"));\n \n \tif (!get_sha1(\"HEAD\", head))\n-\t\twrite_file(am_path(state, \"abort-safety\"), 1, \"%s\", sha1_to_hex(head));\n+\t\twrite_file(am_path(state, \"abort-safety\"), 1, \"%s\\n\", sha1_to_hex(head));\n \telse\n \t\twrite_file(am_path(state, \"abort-safety\"), 1, \"%s\", \"\");\n \n \tstate->cur++;\n-\twrite_file(am_path(state, \"next\"), 1, \"%d\", state->cur);\n+\twrite_file(am_path(state, \"next\"), 1, \"%d\\n\", state->cur);\n }\n \n /**\n@@ -1461,7 +1461,7 @@ static int parse_mail_rebase(struct am_state *state, const char *mail)\n \twrite_commit_patch(state, commit);\n \n \thashcpy(state->orig_commit, commit_sha1);\n-\twrite_file(am_path(state, \"original-commit\"), 1, \"%s\",\n+\twrite_file(am_path(state, \"original-commit\"), 1, \"%s\\n\",\n \t\t\tsha1_to_hex(commit_sha1));\n \n \treturn 0;\n@@ -1764,7 +1764,7 @@ static void am_run(struct am_state *state, int resume)\n \trefresh_and_write_cache();\n \n \tif (index_has_changes(&sb)) {\n-\t\twrite_file(am_path(state, \"dirtyindex\"), 1, \"t\");\n+\t\twrite_file(am_path(state, \"dirtyindex\"), 1, \"%s\", \"\");\n \t\tdie(_(\"Dirty index: cannot apply patches (dirty: %s)\"), sb.buf);\n \t}\n \n-- \n2.5.0.400.gff86faf.dirty\n"},{"id":"268526","messageId":"20150823143055.Horde.3dpHsKGqU554G1p_Uz_Iiw1@webmail.informatik.kit.edu","threadId":"40139","inReplyTo":"20150823055053.GA15849@yoshi.chippynet.com","subject":"Re: [PATCH] am: terminate state files with a newline","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-08-23T12:30:55Z","receivedAt":"2015-08-23T12:30:55Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi,\n\nQuoting Paul Tan :\n\n> Did we ever explictly allow external programs to poke around the\n> contents of the .git/rebase-apply directory? I think it may not be so\n> good, as it means that it may not be possible to switch the storage\n> format in the future (e.g. to allow atomic modifications, maybe?) :-/ .\n\nThink of e.g. libgit2, JGit/EGit and all the other git implementations.\nThey should be able to look everywhere in .git, shouldn't they?\n\nI don't think we will just \"switch\" the storage format of any parts of the\nrepo. Whatever new formats may come (ref backends, index v5, pack v4),\nthey will be an opt-in feature for a long time before becoming default,\nand there must be an even longer deprecation period before the old format\ngets phased out, if ever.\n\n\nGábor\n"},{"id":"268544","messageId":"xmqqy4h16d1f.fsf@gitster.dls.corp.google.com","threadId":"40139","inReplyTo":"20150823055053.GA15849@yoshi.chippynet.com","subject":"Re: [PATCH] am: terminate state files with a newline","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-23T19:05:32Z","receivedAt":"2015-08-23T19:05:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan writes:\n\n> Did we ever explictly allow external programs to poke around the\n> contents of the .git/rebase-apply directory?\n\nWe tell users to take a peek into it when \"am\" fails, don't we, by\nnaming $GIT_DIR/rebase-apply/patch?\n\n> -\twrite_file(am_path(state, \"threeway\"), 1, state->threeway ? \"t\" : \"f\");\n> +\twrite_file(am_path(state, \"threeway\"), 1, \"%s\\n\", state->threeway ? \"t\" : \"f\");\n\nStepping back a bit, after realizing that \"write_file()\" is a\nshort-hand for \"I have all information necessary to produce the full\ncontents of a file, now go ahead and create and write that and\nclose\", I have to wonder what caller even wants to create a file\nwith an incomplete line at the end.\n\nAll callers outside builtin/am.c except one caller uses it to\nproduce a single line file. The oddball is \"git branch\" that uses\nit to prepare a temporary file used to edit branch description. \n\nbuiltin/branch.c:\tif (write_file(git_path(edit_description), 0, \"%s\", buf.buf)) {\n\nThe payload it prepares in buf.buf ends with a canned comment that\nends with LF. So in that sense it is not even an oddball.\n\nThe above analysis makes me wonder if this is a simpler and more\nfuture proof approach.\n\nOr did I miss any caller or a reasonable potential future use case\nthat wants to create a binary file or a text file that ends with an\nincomplete line?\n\n wrapper.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex e451463..7a92298 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -621,6 +621,13 @@ char *xgetcwd(void)\n \treturn strbuf_detach(&sb, NULL);\n }\n \n+/*\n+ * Create a TEXT file by specifying its full contents via fmt and the\n+ * remainder of args that are used like \"printf\". A terminating LF is\n+ * added at the end of the file if it is missing (it is simpler for\n+ * the callers because the function is often used to create a\n+ * single-liner file).\n+ */\n int write_file(const char *path, int fatal, const char *fmt, ...)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n@@ -634,6 +641,9 @@ int write_file(const char *path, int fatal, const char *fmt, ...)\n \tva_start(params, fmt);\n \tstrbuf_vaddf(&sb, fmt, params);\n \tva_end(params);\n+\tif (sb.len)\n+\t\tstrbuf_complete_line(&sb);\n+\n \tif (write_in_full(fd, sb.buf, sb.len) != sb.len) {\n \t\tint err = errno;\n \t\tclose(fd);\n"},{"id":"268551","messageId":"20150824051344.GA12490@sigill.intra.peff.net","threadId":"40139","inReplyTo":"xmqqy4h16d1f.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] am: terminate state files with a newline","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-24T05:13:44Z","receivedAt":"2015-08-24T05:13:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 23, 2015 at 12:05:32PM -0700, Junio C Hamano wrote:\n\n> > -\twrite_file(am_path(state, \"threeway\"), 1, state->threeway ? \"t\" : \"f\");\n> > +\twrite_file(am_path(state, \"threeway\"), 1, \"%s\\n\", state->threeway ? \"t\" : \"f\");\n> \n> Stepping back a bit, after realizing that \"write_file()\" is a\n> short-hand for \"I have all information necessary to produce the full\n> contents of a file, now go ahead and create and write that and\n> close\", I have to wonder what caller even wants to create a file\n> with an incomplete line at the end.\n\nFWIW, I had a similar thought when reading the original thread. I also\nnoted that all of the callers here pass \"1\" for the \"fatal\" parameter,\nand that they are either bools or single strings. I wonder if:\n\n void write_state_bool(struct am_state *state, const char *name, int v)\n {\n\twrite_file(am_path(state, name), 1, \"%s\\n\", v ? \"t\" : \"f\");\n }\n\nwould make the call-sites even easier to read (and of course the \"\\n\"\nwould be dropped here if it does migrate up to write_file()).\n\n> @@ -634,6 +641,9 @@ int write_file(const char *path, int fatal, const char *fmt, ...)\n> \tva_start(params, fmt);\n> \tstrbuf_vaddf(&sb, fmt, params);\n> \tva_end(params);\n> +\tif (sb.len)\n> +\t\tstrbuf_complete_line(&sb);\n> +\n\nI think the \"if\" here is redundant; strbuf_complete_line already handles\nit.\n\n-Peff\n"},{"id":"268557","messageId":"xmqqegit5ghf.fsf@gitster.dls.corp.google.com","threadId":"40139","inReplyTo":"20150824051344.GA12490@sigill.intra.peff.net","subject":"Re: [PATCH] am: terminate state files with a newline","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T06:48:44Z","receivedAt":"2015-08-24T06:48:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> FWIW, I had a similar thought when reading the original thread. I also\n> noted that all of the callers here pass \"1\" for the \"fatal\" parameter,\n> and that they are either bools or single strings. I wonder if:\n>\n> void write_state_bool(struct am_state *state, const char *name, int v)\n> {\n> \twrite_file(am_path(state, name), 1, \"%s\\n\", v ? \"t\" : \"f\");\n> }\n>\n> would make the call-sites even easier to read (and of course the \"\\n\"\n> would be dropped here if it does migrate up to write_file()).\n>\n>> @@ -634,6 +641,9 @@ int write_file(const char *path, int fatal, const char *fmt, ...)\n>> \tva_start(params, fmt);\n>> \tstrbuf_vaddf(&sb, fmt, params);\n>> \tva_end(params);\n>> +\tif (sb.len)\n>> +\t\tstrbuf_complete_line(&sb);\n>> +\n>\n> I think the \"if\" here is redundant; strbuf_complete_line already handles\n> it.\n\nTrue. And I like your write_state_bool() wrapper (which should be\n\"static void\" to the builtin/am.c) very much.\n\nOn top of that, I think the right thing to do to write_file() would\nbe to first clean-up the second parameter \"fatal\" to an \"unsigned\nflags\" whose (1<<0) bit is \"fatal\", (1<<1) bit is \"binary\", and make\nthis new call to \"strbuf_complete_line()\" only when \"binary\" bit is\nnot set.\n\nThe new comment I added before write_file() function needs to be\nadjusted if we were to do this, obviously.\n"},{"id":"268558","messageId":"20150824065033.GA4124@sigill.intra.peff.net","threadId":"40139","inReplyTo":"xmqqegit5ghf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] am: terminate state files with a newline","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-24T06:50:34Z","receivedAt":"2015-08-24T06:50:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 23, 2015 at 11:48:44PM -0700, Junio C Hamano wrote:\n\n> > I think the \"if\" here is redundant; strbuf_complete_line already handles\n> > it.\n> \n> True. And I like your write_state_bool() wrapper (which should be\n> \"static void\" to the builtin/am.c) very much.\n> \n> On top of that, I think the right thing to do to write_file() would\n> be to first clean-up the second parameter \"fatal\" to an \"unsigned\n> flags\" whose (1<<0) bit is \"fatal\", (1<<1) bit is \"binary\", and make\n> this new call to \"strbuf_complete_line()\" only when \"binary\" bit is\n> not set.\n> \n> The new comment I added before write_file() function needs to be\n> adjusted if we were to do this, obviously.\n\nYup, I agree with all of that. I'm about to go to bed, so I'll assume\nyou or Paul will cook up a patch. :)\n\n-Peff\n"},{"id":"268580","messageId":"1440436186-7894-1-git-send-email-gitster@pobox.com","threadId":"40139","inReplyTo":"20150824065033.GA4124@sigill.intra.peff.net","subject":"[PATCH 0/5] \"am\" state file fix with write_file() clean-up","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T17:09:41Z","receivedAt":"2015-08-24T17:09:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"So here is an solution based on the \"write_file() is primarily to\nproduce text, so it should be able to correct the incomplete line\nat the end\" approach.\n\nThe first one is Peff's idea to consolidate callers in \"am\", in a\nmore concrete form.\n\nThe second is the fix to $gmane/276238.\n\nThe remainder is to clean up write_file() helper function. All\ncallers except for two were passing 1 as one parameter, whose\nmeaning was not all obvious to a casual reader.\n\nIn patch 3/5, we flip the default behaviour of write_file() to die\nupon error unless explicitly asked not to with WRITE_FILE_GENTLY\nflag, and change the two oddball callers to pass this new flag.\n\nIn patch 4/5, we enhance the default behaviour of write_file() to\ncomplete an incomplete line at the end, unless asked not to with\nWRITE_FILE_BINARY flag; nobody passes this because all existing\ncallers want to produce a text file.\n\nIn patch 5/5, the transitional noise left by patches 3 and 4 are\ncleaned up by updating the non-binary callers not to add LF\nthemselves and by changing the callers that pass 1 as flags\nparameter to pass 0 (as bit (1<<0) is a no-op since patch 3/5).\n\nThe series is built on top of b5e8235, the current tip of the\npt/am-builtin-options topic.\n\n\nJunio C Hamano (5):\n builtin/am: introduce write_state_*() helper functions\n builtin/am: make sure state files are text\n write_file(): introduce an explicit WRITE_FILE_GENTLY request\n write_file(): do not leave incomplete line at the end\n write_file(): clean up transitional mess of flag words and terminating LF\n\n builtin/am.c | 68 ++++++++++++++++++++++++++++++++----------------------\n builtin/init-db.c | 2 +-\n builtin/worktree.c | 10 ++++----\n cache.h | 16 ++++++++++++-\n daemon.c | 2 +-\n setup.c | 2 +-\n submodule.c | 2 +-\n transport.c | 2 +-\n wrapper.c | 13 +++++++++--\n 9 files changed, 77 insertions(+), 40 deletions(-)\n\n-- \n2.5.0-568-g53a3e28\n"},{"id":"268585","messageId":"1440436186-7894-2-git-send-email-gitster@pobox.com","threadId":"40139","inReplyTo":"1440436186-7894-1-git-send-email-gitster@pobox.com","subject":"[PATCH 1/5] builtin/am: introduce write_state_*() helper functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T17:09:42Z","receivedAt":"2015-08-24T17:09:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"There are many calls to write_file() that repeats the same pattern\nin the implementation of the builtin version of \"am\", and they all\nshare the same traits, i.e they\n\n - produce a text file with a single string in it;\n\n - have enough information to produce the entire contents of that\n file;\n\n - generate the pathname of the file by making a call to am_path(); and\n\n - they ask write_file() to die() upon failure.\n\nThe slight differences among the call sites throw them into roughly\nthree variants:\n\n - many write either \"t\" or \"f\" based on a boolean value to a file;\n\n - some write the integer value in decimal text;\n\n - some others write more general string, e.g. an object name in\n hex, an empty string (i.e. the presense of the file itself serves\n as a flag), etc.\n\nIntroduce three helpers, write_state_bool(), write_state_count() and\nwrite_state_text(), to reduce direct calls to write_file().\n\nThis is a preparatory step for the next step to ensure that no\n\"state\" file this command leaves in $GIT_DIR is with an incomplete\nline at the end.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/am.c | 68 ++++++++++++++++++++++++++++++++++++------------------------\n 1 file changed, 41 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 634f7a7..4d34dc5 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -194,6 +194,27 @@ static inline const char *am_path(const struct am_state *state, const char *path\n }\n \n /**\n+ * For convenience to call write_file()\n+ */\n+static int write_state_text(const struct am_state *state,\n+\t\t\t const char *name, const char *string)\n+{\n+\treturn write_file(am_path(state, name), 1, \"%s\", string);\n+}\n+\n+static int write_state_count(const struct am_state *state,\n+\t\t\t const char *name, int value)\n+{\n+\treturn write_file(am_path(state, name), 1, \"%d\", value);\n+}\n+\n+static int write_state_bool(const struct am_state *state,\n+\t\t\t const char *name, int value)\n+{\n+\treturn write_state_text(state, name, value ? \"t\" : \"f\");\n+}\n+\n+/**\n * If state->quiet is false, calls fprintf(fp, fmt, ...), and appends a newline\n * at the end.\n */\n@@ -362,7 +383,7 @@ static void write_author_script(const struct am_state *state)\n \tsq_quote_buf(&sb, state->author_date);\n \tstrbuf_addch(&sb, '\\n');\n \n-\twrite_file(am_path(state, \"author-script\"), 1, \"%s\", sb.buf);\n+\twrite_state_text(state, \"author-script\", sb.buf);\n \n \tstrbuf_release(&sb);\n }\n@@ -1000,13 +1021,10 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \tif (state->rebasing)\n \t\tstate->threeway = 1;\n \n-\twrite_file(am_path(state, \"threeway\"), 1, state->threeway ? \"t\" : \"f\");\n-\n-\twrite_file(am_path(state, \"quiet\"), 1, state->quiet ? \"t\" : \"f\");\n-\n-\twrite_file(am_path(state, \"sign\"), 1, state->signoff ? \"t\" : \"f\");\n-\n-\twrite_file(am_path(state, \"utf8\"), 1, state->utf8 ? \"t\" : \"f\");\n+\twrite_state_bool(state, \"threeway\", state->threeway);\n+\twrite_state_bool(state, \"quiet\", state->quiet);\n+\twrite_state_bool(state, \"sign\", state->signoff);\n+\twrite_state_bool(state, \"utf8\", state->utf8);\n \n \tswitch (state->keep) {\n \tcase KEEP_FALSE:\n@@ -1022,9 +1040,8 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \t\tdie(\"BUG: invalid value for state->keep\");\n \t}\n \n-\twrite_file(am_path(state, \"keep\"), 1, \"%s\", str);\n-\n-\twrite_file(am_path(state, \"messageid\"), 1, state->message_id ? \"t\" : \"f\");\n+\twrite_state_text(state, \"keep\", str);\n+\twrite_state_bool(state, \"messageid\", state->message_id);\n \n \tswitch (state->scissors) {\n \tcase SCISSORS_UNSET:\n@@ -1039,24 +1056,23 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \tdefault:\n \t\tdie(\"BUG: invalid value for state->scissors\");\n \t}\n-\n-\twrite_file(am_path(state, \"scissors\"), 1, \"%s\", str);\n+\twrite_state_text(state, \"scissors\", str);\n \n \tsq_quote_argv(&sb, state->git_apply_opts.argv, 0);\n-\twrite_file(am_path(state, \"apply-opt\"), 1, \"%s\", sb.buf);\n+\twrite_state_text(state, \"apply-opt\", sb.buf);\n \n \tif (state->rebasing)\n-\t\twrite_file(am_path(state, \"rebasing\"), 1, \"%s\", \"\");\n+\t\twrite_state_text(state, \"rebasing\", \"\");\n \telse\n-\t\twrite_file(am_path(state, \"applying\"), 1, \"%s\", \"\");\n+\t\twrite_state_text(state, \"applying\", \"\");\n \n \tif (!get_sha1(\"HEAD\", curr_head)) {\n-\t\twrite_file(am_path(state, \"abort-safety\"), 1, \"%s\", sha1_to_hex(curr_head));\n+\t\twrite_state_text(state, \"abort-safety\", sha1_to_hex(curr_head));\n \t\tif (!state->rebasing)\n \t\t\tupdate_ref(\"am\", \"ORIG_HEAD\", curr_head, NULL, 0,\n \t\t\t\t\tUPDATE_REFS_DIE_ON_ERR);\n \t} else {\n-\t\twrite_file(am_path(state, \"abort-safety\"), 1, \"%s\", \"\");\n+\t\twrite_state_text(state, \"abort-safety\", \"\");\n \t\tif (!state->rebasing)\n \t\t\tdelete_ref(\"ORIG_HEAD\", NULL, 0);\n \t}\n@@ -1066,9 +1082,8 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \t * session is in progress, they should be written last.\n \t */\n \n-\twrite_file(am_path(state, \"next\"), 1, \"%d\", state->cur);\n-\n-\twrite_file(am_path(state, \"last\"), 1, \"%d\", state->last);\n+\twrite_state_count(state, \"next\", state->cur);\n+\twrite_state_count(state, \"last\", state->last);\n \n \tstrbuf_release(&sb);\n }\n@@ -1101,12 +1116,12 @@ static void am_next(struct am_state *state)\n \tunlink(am_path(state, \"original-commit\"));\n \n \tif (!get_sha1(\"HEAD\", head))\n-\t\twrite_file(am_path(state, \"abort-safety\"), 1, \"%s\", sha1_to_hex(head));\n+\t\twrite_state_text(state, \"abort-safety\", sha1_to_hex(head));\n \telse\n-\t\twrite_file(am_path(state, \"abort-safety\"), 1, \"%s\", \"\");\n+\t\twrite_state_text(state, \"abort-safety\", \"\");\n \n \tstate->cur++;\n-\twrite_file(am_path(state, \"next\"), 1, \"%d\", state->cur);\n+\twrite_state_count(state, \"next\", state->cur);\n }\n \n /**\n@@ -1479,8 +1494,7 @@ static int parse_mail_rebase(struct am_state *state, const char *mail)\n \twrite_commit_patch(state, commit);\n \n \thashcpy(state->orig_commit, commit_sha1);\n-\twrite_file(am_path(state, \"original-commit\"), 1, \"%s\",\n-\t\t\tsha1_to_hex(commit_sha1));\n+\twrite_state_text(state, \"original-commit\", sha1_to_hex(commit_sha1));\n \n \treturn 0;\n }\n@@ -1782,7 +1796,7 @@ static void am_run(struct am_state *state, int resume)\n \trefresh_and_write_cache();\n \n \tif (index_has_changes(&sb)) {\n-\t\twrite_file(am_path(state, \"dirtyindex\"), 1, \"t\");\n+\t\twrite_state_bool(state, \"dirtyindex\", 1);\n \t\tdie(_(\"Dirty index: cannot apply patches (dirty: %s)\"), sb.buf);\n \t}\n \n-- \n2.5.0-568-g53a3e28\n"},{"id":"268582","messageId":"1440436186-7894-3-git-send-email-gitster@pobox.com","threadId":"40139","inReplyTo":"1440436186-7894-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/5] builtin/am: make sure state files are text","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T17:09:43Z","receivedAt":"2015-08-24T17:09:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We forgot to terminate the payload given to write_file() with LF,\nresulting in files that ends with an incomplete line.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/am.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 4d34dc5..3423aa3 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -199,13 +199,13 @@ static inline const char *am_path(const struct am_state *state, const char *path\n static int write_state_text(const struct am_state *state,\n \t\t\t const char *name, const char *string)\n {\n-\treturn write_file(am_path(state, name), 1, \"%s\", string);\n+\treturn write_file(am_path(state, name), 1, \"%s\\n\", string);\n }\n \n static int write_state_count(const struct am_state *state,\n \t\t\t const char *name, int value)\n {\n-\treturn write_file(am_path(state, name), 1, \"%d\", value);\n+\treturn write_file(am_path(state, name), 1, \"%d\\n\", value);\n }\n \n static int write_state_bool(const struct am_state *state,\n-- \n2.5.0-568-g53a3e28\n"},{"id":"268584","messageId":"1440436186-7894-4-git-send-email-gitster@pobox.com","threadId":"40139","inReplyTo":"1440436186-7894-1-git-send-email-gitster@pobox.com","subject":"[PATCH 3/5] write_file(): introduce an explicit WRITE_FILE_GENTLY request","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T17:09:44Z","receivedAt":"2015-08-24T17:09:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"All callers except for two ask this function to die upon error by\npassing fatal=1; turn the parameter to a more generic \"unsigned flag\"\nbag of bits, introduce an explicit WRITE_FILE_GENTLY bit and change\nthese two callers to pass that bit.\n\nThis is in preparation to add one more bit to this flag word.\n\nSigned-off-by: Junio C Hamano \n---\n cache.h | 15 ++++++++++++++-\n setup.c | 2 +-\n transport.c | 2 +-\n wrapper.c | 3 ++-\n 4 files changed, 18 insertions(+), 4 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 6bb7119..f105235 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1539,8 +1539,21 @@ static inline ssize_t write_str_in_full(int fd, const char *str)\n {\n \treturn write_in_full(fd, str, strlen(str));\n }\n+\n+/*\n+ * Create a new file by specifying its full contents via fmt and the\n+ * remainder of args that are used like 'printf()' args. Die upon\n+ * an error unless WRITE_FILE_GENTLY flag is set, in which case return\n+ * a negative number to signal an error.\n+ *\n+ * For historical reasons, the LSB of flags word is set by many\n+ * callers to explicitly ask the function to die upon error, but now\n+ * it is the default.\n+ */\n+#define WRITE_FILE_UNUSED_0 (1<<0)\n+#define WRITE_FILE_GENTLY (1<<1)\n __attribute__((format (printf, 3, 4)))\n-extern int write_file(const char *path, int fatal, const char *fmt, ...);\n+extern int write_file(const char *path, unsigned flags, const char *fmt, ...);\n \n /* pager.c */\n extern void setup_pager(void);\ndiff --git a/setup.c b/setup.c\nindex 5f9f07d..718f4e1 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -404,7 +404,7 @@ static void update_linked_gitdir(const char *gitfile, const char *gitdir)\n \n \tstrbuf_addf(&path, \"%s/gitfile\", gitdir);\n \tif (stat(path.buf, &st) || st.st_mtime + 24 * 3600 < time(NULL))\n-\t\twrite_file(path.buf, 0, \"%s\\n\", gitfile);\n+\t\twrite_file(path.buf, WRITE_FILE_GENTLY, \"%s\\n\", gitfile);\n \tstrbuf_release(&path);\n }\n \ndiff --git a/transport.c b/transport.c\nindex 40692f8..e1821a4 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -291,7 +291,7 @@ static int write_one_ref(const char *name, const struct object_id *oid,\n \n \tstrbuf_addstr(buf, name);\n \tif (safe_create_leading_directories(buf->buf) ||\n-\t write_file(buf->buf, 0, \"%s\\n\", oid_to_hex(oid)))\n+\t write_file(buf->buf, WRITE_FILE_GENTLY, \"%s\\n\", oid_to_hex(oid)))\n \t\treturn error(\"problems writing temporary file %s: %s\",\n \t\t\t buf->buf, strerror(errno));\n \tstrbuf_setlen(buf, len);\ndiff --git a/wrapper.c b/wrapper.c\nindex e451463..68d45b6 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -621,8 +621,9 @@ char *xgetcwd(void)\n \treturn strbuf_detach(&sb, NULL);\n }\n \n-int write_file(const char *path, int fatal, const char *fmt, ...)\n+int write_file(const char *path, unsigned flags, const char *fmt, ...)\n {\n+\tint fatal = !(flags & WRITE_FILE_GENTLY);\n \tstruct strbuf sb = STRBUF_INIT;\n \tva_list params;\n \tint fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0666);\n-- \n2.5.0-568-g53a3e28\n"},{"id":"268583","messageId":"1440436186-7894-5-git-send-email-gitster@pobox.com","threadId":"40139","inReplyTo":"1440436186-7894-1-git-send-email-gitster@pobox.com","subject":"[PATCH 4/5] write_file(): do not leave incomplete line at the end","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T17:09:45Z","receivedAt":"2015-08-24T17:09:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"All existing callers to this function use it to produce a text file\nor an empty file, and a new callsite that mimick them must end their\npayload with a LF. If they forget to do so, the resulting file will\nend with an incomplete line.\n\nIntroduce WRITE_FILE_BINARY flag bit, which no existing callers pass,\nand unless that bit is set, make sure that write_file() adds an extra\nLF at the end of an incomplete line as necessary.\n\nSigned-off-by: Junio C Hamano \n---\n cache.h | 1 +\n wrapper.c | 3 +++\n 2 files changed, 4 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex f105235..dbfa4fa 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1552,6 +1552,7 @@ static inline ssize_t write_str_in_full(int fd, const char *str)\n */\n #define WRITE_FILE_UNUSED_0 (1<<0)\n #define WRITE_FILE_GENTLY (1<<1)\n+#define WRITE_FILE_BINARY (1<<2)\n __attribute__((format (printf, 3, 4)))\n extern int write_file(const char *path, unsigned flags, const char *fmt, ...);\n \ndiff --git a/wrapper.c b/wrapper.c\nindex 68d45b6..4cd2ca3 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -635,6 +635,9 @@ int write_file(const char *path, unsigned flags, const char *fmt, ...)\n \tva_start(params, fmt);\n \tstrbuf_vaddf(&sb, fmt, params);\n \tva_end(params);\n+\tif (!(flags & WRITE_FILE_BINARY))\n+\t\tstrbuf_complete_line(&sb);\n+\n \tif (write_in_full(fd, sb.buf, sb.len) != sb.len) {\n \t\tint err = errno;\n \t\tclose(fd);\n-- \n2.5.0-568-g53a3e28\n"},{"id":"268581","messageId":"1440436186-7894-6-git-send-email-gitster@pobox.com","threadId":"40139","inReplyTo":"1440436186-7894-1-git-send-email-gitster@pobox.com","subject":"[PATCH 5/5] write_file(): clean up transitional mess of flag words and terminating LF","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T17:09:46Z","receivedAt":"2015-08-24T17:09:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Because the function adds necessary LF at the end of an incomplete\nline for all callers that do not pass the WRITE_FILE_BINARY option,\nand no caller of the function calls with that option, stop callers\nto add LF at the end of the payload they pass to the function.\n\nAlso, change the callers that pass 1 to flags, that is now a no-op,\nto pass 0. In order to catch stray callers (and possible topics in\nflight) that still pass 1 to ask the function to die upon error,\nprotect it with an assert().\n\nSigned-off-by: Junio C Hamano \n---\n builtin/am.c | 4 ++--\n builtin/init-db.c | 2 +-\n builtin/worktree.c | 10 +++++-----\n daemon.c | 2 +-\n setup.c | 2 +-\n submodule.c | 2 +-\n transport.c | 2 +-\n wrapper.c | 7 ++++++-\n 8 files changed, 18 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 3423aa3..d804b12 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -199,13 +199,13 @@ static inline const char *am_path(const struct am_state *state, const char *path\n static int write_state_text(const struct am_state *state,\n \t\t\t const char *name, const char *string)\n {\n-\treturn write_file(am_path(state, name), 1, \"%s\\n\", string);\n+\treturn write_file(am_path(state, name), 0, \"%s\", string);\n }\n \n static int write_state_count(const struct am_state *state,\n \t\t\t const char *name, int value)\n {\n-\treturn write_file(am_path(state, name), 1, \"%d\\n\", value);\n+\treturn write_file(am_path(state, name), 0, \"%d\", value);\n }\n \n static int write_state_bool(const struct am_state *state,\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 49df78d..84d27b1 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -378,7 +378,7 @@ static void separate_git_dir(const char *git_dir)\n \t\t\tdie_errno(_(\"unable to move %s to %s\"), src, git_dir);\n \t}\n \n-\twrite_file(git_link, 1, \"gitdir: %s\\n\", git_dir);\n+\twrite_file(git_link, 0, \"gitdir: %s\", git_dir);\n }\n \n int init_db(const char *template_dir, unsigned int flags)\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 6a264ee..cc0981f 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -213,7 +213,7 @@ static int add_worktree(const char *path, const char **child_argv)\n \t * after the preparation is over.\n \t */\n \tstrbuf_addf(&sb, \"%s/locked\", sb_repo.buf);\n-\twrite_file(sb.buf, 1, \"initializing\\n\");\n+\twrite_file(sb.buf, 0, \"initializing\");\n \n \tstrbuf_addf(&sb_git, \"%s/.git\", path);\n \tif (safe_create_leading_directories_const(sb_git.buf))\n@@ -223,8 +223,8 @@ static int add_worktree(const char *path, const char **child_argv)\n \n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/gitdir\", sb_repo.buf);\n-\twrite_file(sb.buf, 1, \"%s\\n\", real_path(sb_git.buf));\n-\twrite_file(sb_git.buf, 1, \"gitdir: %s/worktrees/%s\\n\",\n+\twrite_file(sb.buf, 0, \"%s\", real_path(sb_git.buf));\n+\twrite_file(sb_git.buf, 0, \"gitdir: %s/worktrees/%s\",\n \t\t real_path(get_git_common_dir()), name);\n \t/*\n \t * This is to keep resolve_ref() happy. We need a valid HEAD\n@@ -241,10 +241,10 @@ static int add_worktree(const char *path, const char **child_argv)\n \t\tdie(_(\"unable to resolve HEAD\"));\n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/HEAD\", sb_repo.buf);\n-\twrite_file(sb.buf, 1, \"%s\\n\", sha1_to_hex(rev));\n+\twrite_file(sb.buf, 0, \"%s\", sha1_to_hex(rev));\n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/commondir\", sb_repo.buf);\n-\twrite_file(sb.buf, 1, \"../..\\n\");\n+\twrite_file(sb.buf, 0, \"../..\");\n \n \tfprintf_ln(stderr, _(\"Enter %s (identifier %s)\"), path, name);\n \ndiff --git a/daemon.c b/daemon.c\nindex d3d3e43..30a3fb4 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -1376,7 +1376,7 @@ int main(int argc, char **argv)\n \t\tsanitize_stdfds();\n \n \tif (pid_file)\n-\t\twrite_file(pid_file, 1, \"%\"PRIuMAX\"\\n\", (uintmax_t) getpid());\n+\t\twrite_file(pid_file, 0, \"%\"PRIuMAX, (uintmax_t) getpid());\n \n \t/* prepare argv for serving-processes */\n \tcld_argv = xmalloc(sizeof (char *) * (argc + 2));\ndiff --git a/setup.c b/setup.c\nindex 718f4e1..49675eb 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -404,7 +404,7 @@ static void update_linked_gitdir(const char *gitfile, const char *gitdir)\n \n \tstrbuf_addf(&path, \"%s/gitfile\", gitdir);\n \tif (stat(path.buf, &st) || st.st_mtime + 24 * 3600 < time(NULL))\n-\t\twrite_file(path.buf, WRITE_FILE_GENTLY, \"%s\\n\", gitfile);\n+\t\twrite_file(path.buf, WRITE_FILE_GENTLY, \"%s\", gitfile);\n \tstrbuf_release(&path);\n }\n \ndiff --git a/submodule.c b/submodule.c\nindex 700bbf4..c22fd04 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1103,7 +1103,7 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)\n \n \t/* Update gitfile */\n \tstrbuf_addf(&file_name, \"%s/.git\", work_tree);\n-\twrite_file(file_name.buf, 1, \"gitdir: %s\\n\",\n+\twrite_file(file_name.buf, 0, \"gitdir: %s\",\n \t\t relative_path(git_dir, real_work_tree, &rel_path));\n \n \t/* Update core.worktree setting */\ndiff --git a/transport.c b/transport.c\nindex e1821a4..e5638c0 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -291,7 +291,7 @@ static int write_one_ref(const char *name, const struct object_id *oid,\n \n \tstrbuf_addstr(buf, name);\n \tif (safe_create_leading_directories(buf->buf) ||\n-\t write_file(buf->buf, WRITE_FILE_GENTLY, \"%s\\n\", oid_to_hex(oid)))\n+\t write_file(buf->buf, WRITE_FILE_GENTLY, \"%s\", oid_to_hex(oid)))\n \t\treturn error(\"problems writing temporary file %s: %s\",\n \t\t\t buf->buf, strerror(errno));\n \tstrbuf_setlen(buf, len);\ndiff --git a/wrapper.c b/wrapper.c\nindex 4cd2ca3..4f464ea 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -626,7 +626,12 @@ int write_file(const char *path, unsigned flags, const char *fmt, ...)\n \tint fatal = !(flags & WRITE_FILE_GENTLY);\n \tstruct strbuf sb = STRBUF_INIT;\n \tva_list params;\n-\tint fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0666);\n+\tint fd;\n+\n+\tif ((flags & WRITE_FILE_UNUSED_0))\n+\t\tdie(\"BUG: write_file() called with bit 0 set\");\n+\n+\tfd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0666);\n \tif (fd < 0) {\n \t\tif (fatal)\n \t\t\tdie_errno(_(\"could not open %s for writing\"), path);\n-- \n2.5.0-568-g53a3e28\n"},{"id":"268589","messageId":"20150824174142.GA4794@sigill.intra.peff.net","threadId":"40139","inReplyTo":"1440436186-7894-1-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 0/5] \"am\" state file fix with write_file() clean-up","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-24T17:41:43Z","receivedAt":"2015-08-24T17:41:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 24, 2015 at 10:09:41AM -0700, Junio C Hamano wrote:\n\n> So here is an solution based on the \"write_file() is primarily to\n> produce text, so it should be able to correct the incomplete line\n> at the end\" approach.\n\nThis all looks good to me. The topics-in-flight compatibility stuff in\npatches 3 and 5\t is neatly done. Usually I would just cheat and change\nthe order of arguments to make the compiler notice such problems, but\nthat's hard to do here because of the varargs (you cannot just bump\n\"flags\" to the end).\n\n-Peff\n"},{"id":"268594","messageId":"xmqqlhd04ko4.fsf@gitster.dls.corp.google.com","threadId":"40139","inReplyTo":"20150824174142.GA4794@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] \"am\" state file fix with write_file() clean-up","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T18:15:55Z","receivedAt":"2015-08-24T18:15:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Aug 24, 2015 at 10:09:41AM -0700, Junio C Hamano wrote:\n>\n>> So here is an solution based on the \"write_file() is primarily to\n>> produce text, so it should be able to correct the incomplete line\n>> at the end\" approach.\n>\n> This all looks good to me. The topics-in-flight compatibility stuff in\n> patches 3 and 5 is neatly done. Usually I would just cheat and change\n> the order of arguments to make the compiler notice such problems, but\n> that's hard to do here because of the varargs (you cannot just bump\n> \"flags\" to the end).\n\nActually, I think my compatibility stuff is worthless. It would not\ncatch new callers that wants to only probe and do their own error\nhandling by passing 0 (and besides, assert() is a shoddy way to do\nthis---there is no guarantee that tests will trigger all the\ncodepaths in the first place).\n\nWe should deprecate and remove write_file() by renaming the one with\nthe updated semantics to something else, possibly with a backward\ncompatiblity thin wrapper around it that is called write_file(), or\nwithout it to force a link-time error.\n\nThanks for a dose of sanity.\n"},{"id":"268595","messageId":"20150824183554.GA5883@sigill.intra.peff.net","threadId":"40139","inReplyTo":"xmqqlhd04ko4.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/5] \"am\" state file fix with write_file() clean-up","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-24T18:35:55Z","receivedAt":"2015-08-24T18:35:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 24, 2015 at 11:15:55AM -0700, Junio C Hamano wrote:\n\n> > This all looks good to me. The topics-in-flight compatibility stuff in\n> > patches 3 and 5 is neatly done. Usually I would just cheat and change\n> > the order of arguments to make the compiler notice such problems, but\n> > that's hard to do here because of the varargs (you cannot just bump\n> > \"flags\" to the end).\n> \n> Actually, I think my compatibility stuff is worthless. It would not\n> catch new callers that wants to only probe and do their own error\n> handling by passing 0 (and besides, assert() is a shoddy way to do\n> this---there is no guarantee that tests will trigger all the\n> codepaths in the first place).\n\nOh, hrm, you're right. I was focused on making sure the common 1-passers\nwere not broken, but patch 3 does break 0-passers (obviously, because\nthey needed updated in the same patch ;) ).\n\nAnd I do agree that build-time assertions are much better than run-time\nones.\n\n> We should deprecate and remove write_file() by renaming the one with\n> the updated semantics to something else, possibly with a backward\n> compatiblity thin wrapper around it that is called write_file(), or\n> without it to force a link-time error.\n\nThat sounds reasonable. Maybe \"format_to_file\" or something?\n\n-Peff\n"},{"id":"268596","messageId":"xmqqh9no4jhk.fsf@gitster.dls.corp.google.com","threadId":"40139","inReplyTo":"1440436186-7894-4-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 3/5] write_file(): introduce an explicit WRITE_FILE_GENTLY request","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T18:41:27Z","receivedAt":"2015-08-24T18:41:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> All callers except for two ask this function to die upon error by\n> passing fatal=1; turn the parameter to a more generic \"unsigned flag\"\n> bag of bits, introduce an explicit WRITE_FILE_GENTLY bit and change\n> these two callers to pass that bit.\n\nThere is a huge iffyness around one of these two oddball callers.\n\n> diff --git a/setup.c b/setup.c\n> index 5f9f07d..718f4e1 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -404,7 +404,7 @@ static void update_linked_gitdir(const char *gitfile, const char *gitdir)\n> \n> \tstrbuf_addf(&path, \"%s/gitfile\", gitdir);\n> \tif (stat(path.buf, &st) || st.st_mtime + 24 * 3600 < time(NULL))\n> -\t\twrite_file(path.buf, 0, \"%s\\n\", gitfile);\n> +\t\twrite_file(path.buf, WRITE_FILE_GENTLY, \"%s\\n\", gitfile);\n> \tstrbuf_release(&path);\n> }\n\nThis comes from 23af91d1 (prune: strategies for linked checkouts,\n2014-11-30). I cannot tell what the justification is to treat a\nfailure to write a gitfile as a non-error event. Just a sloppy\ncoding that lets the program go through to its finish, ignoring the\nharm done by possibly corrupting user repository silently?\n\n> diff --git a/transport.c b/transport.c\n> index 40692f8..e1821a4 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -291,7 +291,7 @@ static int write_one_ref(const char *name, const struct object_id *oid,\n> \n> \tstrbuf_addstr(buf, name);\n> \tif (safe_create_leading_directories(buf->buf) ||\n> -\t write_file(buf->buf, 0, \"%s\\n\", oid_to_hex(oid)))\n> +\t write_file(buf->buf, WRITE_FILE_GENTLY, \"%s\\n\", oid_to_hex(oid)))\n> \t\treturn error(\"problems writing temporary file %s: %s\",\n> \t\t\t buf->buf, strerror(errno));\n> \tstrbuf_setlen(buf, len);\n\nThis one is OK, in that it is merely to give a better error\ndiagnosis than just \"oh, I cannot write so I die\".\n"},{"id":"268599","messageId":"xmqqzj1g31e5.fsf@gitster.dls.corp.google.com","threadId":"40139","inReplyTo":"20150824183554.GA5883@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] \"am\" state file fix with write_file() clean-up","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T19:57:38Z","receivedAt":"2015-08-24T19:57:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Aug 24, 2015 at 11:15:55AM -0700, Junio C Hamano wrote:\n>\n>> > This all looks good to me. The topics-in-flight compatibility stuff in\n>> > patches 3 and 5 is neatly done. Usually I would just cheat and change\n>> > the order of arguments to make the compiler notice such problems, but\n>> > that's hard to do here because of the varargs (you cannot just bump\n>> > \"flags\" to the end).\n>> \n>> Actually, I think my compatibility stuff is worthless. It would not\n>> catch new callers that wants to only probe and do their own error\n>> handling by passing 0 (and besides, assert() is a shoddy way to do\n>> this---there is no guarantee that tests will trigger all the\n>> codepaths in the first place).\n>\n> Oh, hrm, you're right. I was focused on making sure the common 1-passers\n> were not broken, but patch 3 does break 0-passers (obviously, because\n> they needed updated in the same patch ;) ).\n>\n> And I do agree that build-time assertions are much better than run-time\n> ones.\n>\n>> We should deprecate and remove write_file() by renaming the one with\n>> the updated semantics to something else, possibly with a backward\n>> compatiblity thin wrapper around it that is called write_file(), or\n>> without it to force a link-time error.\n>\n> That sounds reasonable. Maybe \"format_to_file\" or something?\n\nI am going into a slightly different tangent. Binary support is not\nsomething we need right now, so I'll keep the door open for that in\nthe future by\n\n - drop the \"int fatal\" altogether from write_file() without adding\n \"unsigned flags\";\n\n - add write_file_gently(), again without \"unsigned flags\";\n\n - make them call write_file_v() that takes \"unsigned flags\" and\n va_list.\n\nI earlier said there were 2 oddball callers, one of them being\nsuspicious, but it turns out that there are 3 callers that want\nwrite_file_gently(). Only the one in the setup codepath is asking\nfor \"fatal=0\" while discarding the error return, which is\nsuspicious; other two are handling an error themselves and are OK.\n"},{"id":"268600","messageId":"1440449890-29490-1-git-send-email-gitster@pobox.com","threadId":"40139","inReplyTo":"xmqqzj1g31e5.fsf@gitster.dls.corp.google.com","subject":"[PATCH v2 0/6] \"am\" state file fix with write_file() clean-up","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T20:58:04Z","receivedAt":"2015-08-24T20:58:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"git am\" was recently reimplemented in C. While the implementation\nwas done conservatively and followed the original logic in the\nscripted version fairly faithfully, the state files it left in the\n$GIT_DIR/rebase-apply directory were made slightly different by\nmistake---they lacked the final LF, leaving their last line\nincomplete.\n\nThe patch [1/6] is Peff's idea to consolidate callers in \"am\", in a\nmore concrete form.\n\nThe patch [2/6] is the fix to the state files with incomplete lines.\n\nThe workhorse helper function that implements \"we have this (short)\nbody of text; create a new file that contains it\" has a \"fatal\"\nparameter, to which 1 was passed by almost all callers, but to\ncasual readers, it was unclear what that 1 meant. The patch [3/6]\nsplits it to write_file() and write_file_gently() and drops this\nparameter that looks mysterious at the callsites. A common helper\nfunction write_file_v() is introduced to implement these two as thin\nwrappers of it.\n\nThe patch [4/6] updates write_file_v() so that it does the \"we are\nwriting a text file. Make sure it does not end with an incomplete\nline\" logic that [2/6] added only to builtin/am.c, thusly reverting\nwhat was done to builtin/am.c in [2/6].\n\nThe patch [5/6] stops all callers that creates a single-liner file\nusing write_file() and write_file_gently() from including the final\nLF to the format they pass. This should not change the behaviour,\nbut it probably makes it conceptually cleaner. You have the contents\nto be placed on a single line, and the helper turns the contents\ninto a proper \"line\".\n\nThe patch [6/6] drops the final LF from the parameter to create a\nmulti-line file; while this does not hurt in the sense that the\ncallee will add a necessary LF back, I do not think it should be\napplied. Conceptually, if you have a buffer that contains a bunch\nof lines and throw it at a helper to create a file, you'd better\nhave the terminating LF yourself before asking the helper to put\nthem in the file.\n\nJunio C Hamano (6):\n builtin/am: introduce write_state_*() helper functions\n builtin/am: make sure state files are text\n write_file(): drop \"fatal\" parameter\n write_file_v(): do not leave incomplete line at the end\n write_file(): drop caller-supplied LF from calls to create a one-liner\n file\n write_file(): drop caller-supplied LF from multi-line file\n\n builtin/am.c | 69 ++++++++++++++++++++++++++++++++----------------------\n builtin/branch.c | 4 ++--\n builtin/init-db.c | 2 +-\n builtin/worktree.c | 10 ++++----\n cache.h | 5 ++--\n daemon.c | 2 +-\n setup.c | 2 +-\n submodule.c | 2 +-\n transport.c | 2 +-\n wrapper.c | 36 ++++++++++++++++++++++++----\n 10 files changed, 88 insertions(+), 46 deletions(-)\n\n-- \n2.5.0-568-g53a3e28\n"},{"id":"268605","messageId":"1440449890-29490-2-git-send-email-gitster@pobox.com","threadId":"40139","inReplyTo":"1440449890-29490-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 1/6] builtin/am: introduce write_state_*() helper functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T20:58:05Z","receivedAt":"2015-08-24T20:58:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"There are many calls to write_file() that repeat the same pattern in\nthe implementation of the builtin version of \"am\". They all share\nthe same traits, i.e they\n\n - produce a text file with a single string in it;\n\n - have enough information to produce the entire contents of that\n file;\n\n - generate the pathname of the file by making a call to am_path(); and\n\n - they ask write_file() to die() upon failure.\n\nThe slight differences among the call sites throw them into roughly\nthree categories:\n\n - many write either \"t\" or \"f\" based on a boolean value to a file;\n\n - some write the integer value in decimal text;\n\n - some others write more general string, e.g. an object name in\n hex, an empty string (i.e. the presense of the file itself serves\n as a flag), etc.\n\nIntroduce three helpers, write_state_bool(), write_state_count() and\nwrite_state_text(), to reduce direct calls to write_file().\n\nThis is a preparatory step for the next step to ensure that no\n\"state\" file this command leaves in $GIT_DIR is with an incomplete\nline at the end.\n\nSuggested-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n builtin/am.c | 68 ++++++++++++++++++++++++++++++++++++------------------------\n 1 file changed, 41 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 634f7a7..4d34dc5 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -194,6 +194,27 @@ static inline const char *am_path(const struct am_state *state, const char *path\n }\n \n /**\n+ * For convenience to call write_file()\n+ */\n+static int write_state_text(const struct am_state *state,\n+\t\t\t const char *name, const char *string)\n+{\n+\treturn write_file(am_path(state, name), 1, \"%s\", string);\n+}\n+\n+static int write_state_count(const struct am_state *state,\n+\t\t\t const char *name, int value)\n+{\n+\treturn write_file(am_path(state, name), 1, \"%d\", value);\n+}\n+\n+static int write_state_bool(const struct am_state *state,\n+\t\t\t const char *name, int value)\n+{\n+\treturn write_state_text(state, name, value ? \"t\" : \"f\");\n+}\n+\n+/**\n * If state->quiet is false, calls fprintf(fp, fmt, ...), and appends a newline\n * at the end.\n */\n@@ -362,7 +383,7 @@ static void write_author_script(const struct am_state *state)\n \tsq_quote_buf(&sb, state->author_date);\n \tstrbuf_addch(&sb, '\\n');\n \n-\twrite_file(am_path(state, \"author-script\"), 1, \"%s\", sb.buf);\n+\twrite_state_text(state, \"author-script\", sb.buf);\n \n \tstrbuf_release(&sb);\n }\n@@ -1000,13 +1021,10 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \tif (state->rebasing)\n \t\tstate->threeway = 1;\n \n-\twrite_file(am_path(state, \"threeway\"), 1, state->threeway ? \"t\" : \"f\");\n-\n-\twrite_file(am_path(state, \"quiet\"), 1, state->quiet ? \"t\" : \"f\");\n-\n-\twrite_file(am_path(state, \"sign\"), 1, state->signoff ? \"t\" : \"f\");\n-\n-\twrite_file(am_path(state, \"utf8\"), 1, state->utf8 ? \"t\" : \"f\");\n+\twrite_state_bool(state, \"threeway\", state->threeway);\n+\twrite_state_bool(state, \"quiet\", state->quiet);\n+\twrite_state_bool(state, \"sign\", state->signoff);\n+\twrite_state_bool(state, \"utf8\", state->utf8);\n \n \tswitch (state->keep) {\n \tcase KEEP_FALSE:\n@@ -1022,9 +1040,8 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \t\tdie(\"BUG: invalid value for state->keep\");\n \t}\n \n-\twrite_file(am_path(state, \"keep\"), 1, \"%s\", str);\n-\n-\twrite_file(am_path(state, \"messageid\"), 1, state->message_id ? \"t\" : \"f\");\n+\twrite_state_text(state, \"keep\", str);\n+\twrite_state_bool(state, \"messageid\", state->message_id);\n \n \tswitch (state->scissors) {\n \tcase SCISSORS_UNSET:\n@@ -1039,24 +1056,23 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \tdefault:\n \t\tdie(\"BUG: invalid value for state->scissors\");\n \t}\n-\n-\twrite_file(am_path(state, \"scissors\"), 1, \"%s\", str);\n+\twrite_state_text(state, \"scissors\", str);\n \n \tsq_quote_argv(&sb, state->git_apply_opts.argv, 0);\n-\twrite_file(am_path(state, \"apply-opt\"), 1, \"%s\", sb.buf);\n+\twrite_state_text(state, \"apply-opt\", sb.buf);\n \n \tif (state->rebasing)\n-\t\twrite_file(am_path(state, \"rebasing\"), 1, \"%s\", \"\");\n+\t\twrite_state_text(state, \"rebasing\", \"\");\n \telse\n-\t\twrite_file(am_path(state, \"applying\"), 1, \"%s\", \"\");\n+\t\twrite_state_text(state, \"applying\", \"\");\n \n \tif (!get_sha1(\"HEAD\", curr_head)) {\n-\t\twrite_file(am_path(state, \"abort-safety\"), 1, \"%s\", sha1_to_hex(curr_head));\n+\t\twrite_state_text(state, \"abort-safety\", sha1_to_hex(curr_head));\n \t\tif (!state->rebasing)\n \t\t\tupdate_ref(\"am\", \"ORIG_HEAD\", curr_head, NULL, 0,\n \t\t\t\t\tUPDATE_REFS_DIE_ON_ERR);\n \t} else {\n-\t\twrite_file(am_path(state, \"abort-safety\"), 1, \"%s\", \"\");\n+\t\twrite_state_text(state, \"abort-safety\", \"\");\n \t\tif (!state->rebasing)\n \t\t\tdelete_ref(\"ORIG_HEAD\", NULL, 0);\n \t}\n@@ -1066,9 +1082,8 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \t * session is in progress, they should be written last.\n \t */\n \n-\twrite_file(am_path(state, \"next\"), 1, \"%d\", state->cur);\n-\n-\twrite_file(am_path(state, \"last\"), 1, \"%d\", state->last);\n+\twrite_state_count(state, \"next\", state->cur);\n+\twrite_state_count(state, \"last\", state->last);\n \n \tstrbuf_release(&sb);\n }\n@@ -1101,12 +1116,12 @@ static void am_next(struct am_state *state)\n \tunlink(am_path(state, \"original-commit\"));\n \n \tif (!get_sha1(\"HEAD\", head))\n-\t\twrite_file(am_path(state, \"abort-safety\"), 1, \"%s\", sha1_to_hex(head));\n+\t\twrite_state_text(state, \"abort-safety\", sha1_to_hex(head));\n \telse\n-\t\twrite_file(am_path(state, \"abort-safety\"), 1, \"%s\", \"\");\n+\t\twrite_state_text(state, \"abort-safety\", \"\");\n \n \tstate->cur++;\n-\twrite_file(am_path(state, \"next\"), 1, \"%d\", state->cur);\n+\twrite_state_count(state, \"next\", state->cur);\n }\n \n /**\n@@ -1479,8 +1494,7 @@ static int parse_mail_rebase(struct am_state *state, const char *mail)\n \twrite_commit_patch(state, commit);\n \n \thashcpy(state->orig_commit, commit_sha1);\n-\twrite_file(am_path(state, \"original-commit\"), 1, \"%s\",\n-\t\t\tsha1_to_hex(commit_sha1));\n+\twrite_state_text(state, \"original-commit\", sha1_to_hex(commit_sha1));\n \n \treturn 0;\n }\n@@ -1782,7 +1796,7 @@ static void am_run(struct am_state *state, int resume)\n \trefresh_and_write_cache();\n \n \tif (index_has_changes(&sb)) {\n-\t\twrite_file(am_path(state, \"dirtyindex\"), 1, \"t\");\n+\t\twrite_state_bool(state, \"dirtyindex\", 1);\n \t\tdie(_(\"Dirty index: cannot apply patches (dirty: %s)\"), sb.buf);\n \t}\n \n-- \n2.5.0-568-g53a3e28\n"},{"id":"268601","messageId":"1440449890-29490-3-git-send-email-gitster@pobox.com","threadId":"40139","inReplyTo":"1440449890-29490-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 2/6] builtin/am: make sure state files are text","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T20:58:06Z","receivedAt":"2015-08-24T20:58:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We forgot to terminate the payload given to write_file() with LF,\nresulting in files that end with an incomplete line. Teach the\nwrappers builtin/am uses to make sure it adds LF at the end as\nnecessary.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/am.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 4d34dc5..f0a046b 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -199,13 +199,19 @@ static inline const char *am_path(const struct am_state *state, const char *path\n static int write_state_text(const struct am_state *state,\n \t\t\t const char *name, const char *string)\n {\n-\treturn write_file(am_path(state, name), 1, \"%s\", string);\n+\tconst char *fmt;\n+\n+\tif (*string && string[strlen(string) - 1] != '\\n')\n+\t\tfmt = \"%s\\n\";\n+\telse\n+\t\tfmt = \"%s\";\n+\treturn write_file(am_path(state, name), 1, fmt, string);\n }\n \n static int write_state_count(const struct am_state *state,\n \t\t\t const char *name, int value)\n {\n-\treturn write_file(am_path(state, name), 1, \"%d\", value);\n+\treturn write_file(am_path(state, name), 1, \"%d\\n\", value);\n }\n \n static int write_state_bool(const struct am_state *state,\n-- \n2.5.0-568-g53a3e28\n"},{"id":"268606","messageId":"1440449890-29490-4-git-send-email-gitster@pobox.com","threadId":"40139","inReplyTo":"1440449890-29490-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 3/6] write_file(): drop \"fatal\" parameter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T20:58:07Z","receivedAt":"2015-08-24T20:58:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"All callers except three passed 1 for the \"fatal\" parameter to ask\nthis function to die upon error, but to a casual reader of the code,\nit was not all obvious what that 1 meant. Instead, split the\nfunction into two based on a common write_file_v() that takes the\nflag, introduce write_file_gently() as a new way to attempt creating\na file without dying on error, and make three callers to call it.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/am.c | 4 ++--\n builtin/branch.c | 2 +-\n builtin/init-db.c | 2 +-\n builtin/worktree.c | 10 +++++-----\n cache.h | 5 +++--\n daemon.c | 2 +-\n setup.c | 2 +-\n submodule.c | 2 +-\n transport.c | 2 +-\n wrapper.c | 28 ++++++++++++++++++++++++----\n 10 files changed, 40 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex f0a046b..9c57677 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -205,13 +205,13 @@ static int write_state_text(const struct am_state *state,\n \t\tfmt = \"%s\\n\";\n \telse\n \t\tfmt = \"%s\";\n-\treturn write_file(am_path(state, name), 1, fmt, string);\n+\treturn write_file(am_path(state, name), fmt, string);\n }\n \n static int write_state_count(const struct am_state *state,\n \t\t\t const char *name, int value)\n {\n-\treturn write_file(am_path(state, name), 1, \"%d\\n\", value);\n+\treturn write_file(am_path(state, name), \"%d\\n\", value);\n }\n \n static int write_state_bool(const struct am_state *state,\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 58aa84f..ff05869 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -776,7 +776,7 @@ static int edit_branch_description(const char *branch_name)\n \t\t \" %s\\n\"\n \t\t \"Lines starting with '%c' will be stripped.\\n\",\n \t\t branch_name, comment_line_char);\n-\tif (write_file(git_path(edit_description), 0, \"%s\", buf.buf)) {\n+\tif (write_file_gently(git_path(edit_description), \"%s\", buf.buf)) {\n \t\tstrbuf_release(&buf);\n \t\treturn error(_(\"could not write branch description template: %s\"),\n \t\t\t strerror(errno));\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 49df78d..bfe1d08 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -378,7 +378,7 @@ static void separate_git_dir(const char *git_dir)\n \t\t\tdie_errno(_(\"unable to move %s to %s\"), src, git_dir);\n \t}\n \n-\twrite_file(git_link, 1, \"gitdir: %s\\n\", git_dir);\n+\twrite_file(git_link, \"gitdir: %s\\n\", git_dir);\n }\n \n int init_db(const char *template_dir, unsigned int flags)\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 6a264ee..368502d 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -213,7 +213,7 @@ static int add_worktree(const char *path, const char **child_argv)\n \t * after the preparation is over.\n \t */\n \tstrbuf_addf(&sb, \"%s/locked\", sb_repo.buf);\n-\twrite_file(sb.buf, 1, \"initializing\\n\");\n+\twrite_file(sb.buf, \"initializing\\n\");\n \n \tstrbuf_addf(&sb_git, \"%s/.git\", path);\n \tif (safe_create_leading_directories_const(sb_git.buf))\n@@ -223,8 +223,8 @@ static int add_worktree(const char *path, const char **child_argv)\n \n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/gitdir\", sb_repo.buf);\n-\twrite_file(sb.buf, 1, \"%s\\n\", real_path(sb_git.buf));\n-\twrite_file(sb_git.buf, 1, \"gitdir: %s/worktrees/%s\\n\",\n+\twrite_file(sb.buf, \"%s\\n\", real_path(sb_git.buf));\n+\twrite_file(sb_git.buf, \"gitdir: %s/worktrees/%s\\n\",\n \t\t real_path(get_git_common_dir()), name);\n \t/*\n \t * This is to keep resolve_ref() happy. We need a valid HEAD\n@@ -241,10 +241,10 @@ static int add_worktree(const char *path, const char **child_argv)\n \t\tdie(_(\"unable to resolve HEAD\"));\n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/HEAD\", sb_repo.buf);\n-\twrite_file(sb.buf, 1, \"%s\\n\", sha1_to_hex(rev));\n+\twrite_file(sb.buf, \"%s\\n\", sha1_to_hex(rev));\n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/commondir\", sb_repo.buf);\n-\twrite_file(sb.buf, 1, \"../..\\n\");\n+\twrite_file(sb.buf, \"../..\\n\");\n \n \tfprintf_ln(stderr, _(\"Enter %s (identifier %s)\"), path, name);\n \ndiff --git a/cache.h b/cache.h\nindex 6bb7119..3f79e6b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1539,8 +1539,9 @@ static inline ssize_t write_str_in_full(int fd, const char *str)\n {\n \treturn write_in_full(fd, str, strlen(str));\n }\n-__attribute__((format (printf, 3, 4)))\n-extern int write_file(const char *path, int fatal, const char *fmt, ...);\n+\n+extern int write_file(const char *path, const char *fmt, ...);\n+extern int write_file_gently(const char *path, const char *fmt, ...);\n \n /* pager.c */\n extern void setup_pager(void);\ndiff --git a/daemon.c b/daemon.c\nindex d3d3e43..9154509 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -1376,7 +1376,7 @@ int main(int argc, char **argv)\n \t\tsanitize_stdfds();\n \n \tif (pid_file)\n-\t\twrite_file(pid_file, 1, \"%\"PRIuMAX\"\\n\", (uintmax_t) getpid());\n+\t\twrite_file(pid_file, \"%\"PRIuMAX\"\\n\", (uintmax_t) getpid());\n \n \t/* prepare argv for serving-processes */\n \tcld_argv = xmalloc(sizeof (char *) * (argc + 2));\ndiff --git a/setup.c b/setup.c\nindex 5f9f07d..feb8565 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -404,7 +404,7 @@ static void update_linked_gitdir(const char *gitfile, const char *gitdir)\n \n \tstrbuf_addf(&path, \"%s/gitfile\", gitdir);\n \tif (stat(path.buf, &st) || st.st_mtime + 24 * 3600 < time(NULL))\n-\t\twrite_file(path.buf, 0, \"%s\\n\", gitfile);\n+\t\twrite_file_gently(path.buf, \"%s\\n\", gitfile);\n \tstrbuf_release(&path);\n }\n \ndiff --git a/submodule.c b/submodule.c\nindex 700bbf4..5519f11 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1103,7 +1103,7 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)\n \n \t/* Update gitfile */\n \tstrbuf_addf(&file_name, \"%s/.git\", work_tree);\n-\twrite_file(file_name.buf, 1, \"gitdir: %s\\n\",\n+\twrite_file(file_name.buf, \"gitdir: %s\\n\",\n \t\t relative_path(git_dir, real_work_tree, &rel_path));\n \n \t/* Update core.worktree setting */\ndiff --git a/transport.c b/transport.c\nindex 40692f8..0254394 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -291,7 +291,7 @@ static int write_one_ref(const char *name, const struct object_id *oid,\n \n \tstrbuf_addstr(buf, name);\n \tif (safe_create_leading_directories(buf->buf) ||\n-\t write_file(buf->buf, 0, \"%s\\n\", oid_to_hex(oid)))\n+\t write_file_gently(buf->buf, \"%s\\n\", oid_to_hex(oid)))\n \t\treturn error(\"problems writing temporary file %s: %s\",\n \t\t\t buf->buf, strerror(errno));\n \tstrbuf_setlen(buf, len);\ndiff --git a/wrapper.c b/wrapper.c\nindex e451463..8c8925b 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -621,19 +621,17 @@ char *xgetcwd(void)\n \treturn strbuf_detach(&sb, NULL);\n }\n \n-int write_file(const char *path, int fatal, const char *fmt, ...)\n+static int write_file_v(const char *path, int fatal,\n+\t\t\tconst char *fmt, va_list params)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n-\tva_list params;\n \tint fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0666);\n \tif (fd < 0) {\n \t\tif (fatal)\n \t\t\tdie_errno(_(\"could not open %s for writing\"), path);\n \t\treturn -1;\n \t}\n-\tva_start(params, fmt);\n \tstrbuf_vaddf(&sb, fmt, params);\n-\tva_end(params);\n \tif (write_in_full(fd, sb.buf, sb.len) != sb.len) {\n \t\tint err = errno;\n \t\tclose(fd);\n@@ -652,6 +650,28 @@ int write_file(const char *path, int fatal, const char *fmt, ...)\n \treturn 0;\n }\n \n+int write_file(const char *path, const char *fmt, ...)\n+{\n+\tint status;\n+\tva_list params;\n+\n+\tva_start(params, fmt);\n+\tstatus = write_file_v(path, 1, fmt, params);\n+\tva_end(params);\n+\treturn status;\n+}\n+\n+int write_file_gently(const char *path, const char *fmt, ...)\n+{\n+\tint status;\n+\tva_list params;\n+\n+\tva_start(params, fmt);\n+\tstatus = write_file_v(path, 0, fmt, params);\n+\tva_end(params);\n+\treturn status;\n+}\n+\n void sleep_millisec(int millisec)\n {\n \tpoll(NULL, 0, millisec);\n-- \n2.5.0-568-g53a3e28\n"},{"id":"268604","messageId":"1440449890-29490-5-git-send-email-gitster@pobox.com","threadId":"40139","inReplyTo":"1440449890-29490-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 4/6] write_file_v(): do not leave incomplete line at the end","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T20:58:08Z","receivedAt":"2015-08-24T20:58:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"All existing callers to this function use it to produce a text file\nor an empty file, and a new callsite that mimick them must end their\npayload with a LF. If they forget to do so, the resulting file will\nend with an incomplete line.\n\nIntroduce WRITE_FILE_BINARY flag bit, which no existing callers\npass, and unless that bit is set, make sure that write_file_v() adds\nan extra LF at the end of an incomplete line as necessary.\n\nWith this, the caller-side fix in builtin/am.c becomes unnecessary.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/am.c | 10 ++--------\n wrapper.c | 14 +++++++++++---\n 2 files changed, 13 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 9c57677..486ff59 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -199,19 +199,13 @@ static inline const char *am_path(const struct am_state *state, const char *path\n static int write_state_text(const struct am_state *state,\n \t\t\t const char *name, const char *string)\n {\n-\tconst char *fmt;\n-\n-\tif (*string && string[strlen(string) - 1] != '\\n')\n-\t\tfmt = \"%s\\n\";\n-\telse\n-\t\tfmt = \"%s\";\n-\treturn write_file(am_path(state, name), fmt, string);\n+\treturn write_file(am_path(state, name), \"%s\", string);\n }\n \n static int write_state_count(const struct am_state *state,\n \t\t\t const char *name, int value)\n {\n-\treturn write_file(am_path(state, name), \"%d\\n\", value);\n+\treturn write_file(am_path(state, name), \"%d\", value);\n }\n \n static int write_state_bool(const struct am_state *state,\ndiff --git a/wrapper.c b/wrapper.c\nindex 8c8925b..db39e1b 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -621,17 +621,25 @@ char *xgetcwd(void)\n \treturn strbuf_detach(&sb, NULL);\n }\n \n-static int write_file_v(const char *path, int fatal,\n+\n+#define WRITE_FILE_GENTLY (1 << 0)\n+#define WRITE_FILE_BINARY (1 << 1)\n+\n+static int write_file_v(const char *path, unsigned flags,\n \t\t\tconst char *fmt, va_list params)\n {\n+\tint fatal = !(flags & WRITE_FILE_GENTLY);\n \tstruct strbuf sb = STRBUF_INIT;\n \tint fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0666);\n+\n \tif (fd < 0) {\n \t\tif (fatal)\n \t\t\tdie_errno(_(\"could not open %s for writing\"), path);\n \t\treturn -1;\n \t}\n \tstrbuf_vaddf(&sb, fmt, params);\n+\tif (!(flags & WRITE_FILE_BINARY))\n+\t\tstrbuf_complete_line(&sb);\n \tif (write_in_full(fd, sb.buf, sb.len) != sb.len) {\n \t\tint err = errno;\n \t\tclose(fd);\n@@ -656,7 +664,7 @@ int write_file(const char *path, const char *fmt, ...)\n \tva_list params;\n \n \tva_start(params, fmt);\n-\tstatus = write_file_v(path, 1, fmt, params);\n+\tstatus = write_file_v(path, 0, fmt, params);\n \tva_end(params);\n \treturn status;\n }\n@@ -667,7 +675,7 @@ int write_file_gently(const char *path, const char *fmt, ...)\n \tva_list params;\n \n \tva_start(params, fmt);\n-\tstatus = write_file_v(path, 0, fmt, params);\n+\tstatus = write_file_v(path, WRITE_FILE_GENTLY, fmt, params);\n \tva_end(params);\n \treturn status;\n }\n-- \n2.5.0-568-g53a3e28\n"},{"id":"268603","messageId":"1440449890-29490-6-git-send-email-gitster@pobox.com","threadId":"40139","inReplyTo":"1440449890-29490-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 5/6] write_file(): drop caller-supplied LF from calls to create a one-liner file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T20:58:09Z","receivedAt":"2015-08-24T20:58:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"All of the callsites covered by this change call write_file() or\nwrite_file_gently() to create a one-liner file. Drop the caller\nsupplied LF and let these callees to append it as necessary.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/init-db.c | 2 +-\n builtin/worktree.c | 10 +++++-----\n daemon.c | 2 +-\n setup.c | 2 +-\n submodule.c | 2 +-\n transport.c | 2 +-\n 6 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex bfe1d08..69323e1 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -378,7 +378,7 @@ static void separate_git_dir(const char *git_dir)\n \t\t\tdie_errno(_(\"unable to move %s to %s\"), src, git_dir);\n \t}\n \n-\twrite_file(git_link, \"gitdir: %s\\n\", git_dir);\n+\twrite_file(git_link, \"gitdir: %s\", git_dir);\n }\n \n int init_db(const char *template_dir, unsigned int flags)\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 368502d..bbb169a 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -213,7 +213,7 @@ static int add_worktree(const char *path, const char **child_argv)\n \t * after the preparation is over.\n \t */\n \tstrbuf_addf(&sb, \"%s/locked\", sb_repo.buf);\n-\twrite_file(sb.buf, \"initializing\\n\");\n+\twrite_file(sb.buf, \"initializing\");\n \n \tstrbuf_addf(&sb_git, \"%s/.git\", path);\n \tif (safe_create_leading_directories_const(sb_git.buf))\n@@ -223,8 +223,8 @@ static int add_worktree(const char *path, const char **child_argv)\n \n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/gitdir\", sb_repo.buf);\n-\twrite_file(sb.buf, \"%s\\n\", real_path(sb_git.buf));\n-\twrite_file(sb_git.buf, \"gitdir: %s/worktrees/%s\\n\",\n+\twrite_file(sb.buf, \"%s\", real_path(sb_git.buf));\n+\twrite_file(sb_git.buf, \"gitdir: %s/worktrees/%s\",\n \t\t real_path(get_git_common_dir()), name);\n \t/*\n \t * This is to keep resolve_ref() happy. We need a valid HEAD\n@@ -241,10 +241,10 @@ static int add_worktree(const char *path, const char **child_argv)\n \t\tdie(_(\"unable to resolve HEAD\"));\n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/HEAD\", sb_repo.buf);\n-\twrite_file(sb.buf, \"%s\\n\", sha1_to_hex(rev));\n+\twrite_file(sb.buf, \"%s\", sha1_to_hex(rev));\n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/commondir\", sb_repo.buf);\n-\twrite_file(sb.buf, \"../..\\n\");\n+\twrite_file(sb.buf, \"../..\");\n \n \tfprintf_ln(stderr, _(\"Enter %s (identifier %s)\"), path, name);\n \ndiff --git a/daemon.c b/daemon.c\nindex 9154509..f9eb296 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -1376,7 +1376,7 @@ int main(int argc, char **argv)\n \t\tsanitize_stdfds();\n \n \tif (pid_file)\n-\t\twrite_file(pid_file, \"%\"PRIuMAX\"\\n\", (uintmax_t) getpid());\n+\t\twrite_file(pid_file, \"%\"PRIuMAX, (uintmax_t) getpid());\n \n \t/* prepare argv for serving-processes */\n \tcld_argv = xmalloc(sizeof (char *) * (argc + 2));\ndiff --git a/setup.c b/setup.c\nindex feb8565..a206781 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -404,7 +404,7 @@ static void update_linked_gitdir(const char *gitfile, const char *gitdir)\n \n \tstrbuf_addf(&path, \"%s/gitfile\", gitdir);\n \tif (stat(path.buf, &st) || st.st_mtime + 24 * 3600 < time(NULL))\n-\t\twrite_file_gently(path.buf, \"%s\\n\", gitfile);\n+\t\twrite_file_gently(path.buf, \"%s\", gitfile);\n \tstrbuf_release(&path);\n }\n \ndiff --git a/submodule.c b/submodule.c\nindex 5519f11..4549c1b 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1103,7 +1103,7 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)\n \n \t/* Update gitfile */\n \tstrbuf_addf(&file_name, \"%s/.git\", work_tree);\n-\twrite_file(file_name.buf, \"gitdir: %s\\n\",\n+\twrite_file(file_name.buf, \"gitdir: %s\",\n \t\t relative_path(git_dir, real_work_tree, &rel_path));\n \n \t/* Update core.worktree setting */\ndiff --git a/transport.c b/transport.c\nindex 0254394..788cf20 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -291,7 +291,7 @@ static int write_one_ref(const char *name, const struct object_id *oid,\n \n \tstrbuf_addstr(buf, name);\n \tif (safe_create_leading_directories(buf->buf) ||\n-\t write_file_gently(buf->buf, \"%s\\n\", oid_to_hex(oid)))\n+\t write_file_gently(buf->buf, \"%s\", oid_to_hex(oid)))\n \t\treturn error(\"problems writing temporary file %s: %s\",\n \t\t\t buf->buf, strerror(errno));\n \tstrbuf_setlen(buf, len);\n-- \n2.5.0-568-g53a3e28\n"},{"id":"268602","messageId":"1440449890-29490-7-git-send-email-gitster@pobox.com","threadId":"40139","inReplyTo":"1440449890-29490-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 6/6] write_file(): drop caller-supplied LF from multi-line file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T20:58:10Z","receivedAt":"2015-08-24T20:58:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This is just to illustrate that we _could_ do this; I think it is\nbetter to leave these places as they are. The primary thing we\nwanted to do with the automatic addition of LF to an incomplete line\nwas to make it easier to write a caller that creates a single-liner\nfile. For callers that want to fully create a multi-line input, the\nresulting code is easier to see if we let them continue to do so.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/am.c | 1 -\n builtin/branch.c | 2 +-\n 2 files changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 486ff59..c544091 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -381,7 +381,6 @@ static void write_author_script(const struct am_state *state)\n \n \tstrbuf_addstr(&sb, \"GIT_AUTHOR_DATE=\");\n \tsq_quote_buf(&sb, state->author_date);\n-\tstrbuf_addch(&sb, '\\n');\n \n \twrite_state_text(state, \"author-script\", sb.buf);\n \ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex ff05869..cdf7f13 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -774,7 +774,7 @@ static int edit_branch_description(const char *branch_name)\n \tstrbuf_commented_addf(&buf,\n \t\t \"Please edit the description for the branch\\n\"\n \t\t \" %s\\n\"\n-\t\t \"Lines starting with '%c' will be stripped.\\n\",\n+\t\t \"Lines starting with '%c' will be stripped.\",\n \t\t branch_name, comment_line_char);\n \tif (write_file_gently(git_path(edit_description), \"%s\", buf.buf)) {\n \t\tstrbuf_release(&buf);\n-- \n2.5.0-568-g53a3e28\n"},{"id":"268617","messageId":"20150824233612.GE232027@vauxhall.crustytoothpaste.net","threadId":"40139","inReplyTo":"20150823055053.GA15849@yoshi.chippynet.com","subject":"Re: [PATCH] am: terminate state files with a newline","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-08-24T23:36:12Z","receivedAt":"2015-08-24T23:36:12Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sun, Aug 23, 2015 at 01:50:53PM +0800, Paul Tan wrote:\n> Did we ever explictly allow external programs to poke around the\n> contents of the .git/rebase-apply directory? I think it may not be so\n> good, as it means that it may not be possible to switch the storage\n> format in the future (e.g. to allow atomic modifications, maybe?) :-/ .\n\nzsh's vcs_info does read files in those directories in order to\ndetermine which patches have been applied. I just submitted a patch to\nzsh that fixed warnings when a conflict occurred with git rebase -m.\n\nI expect that unless we provide a programmatic way to discover all of\nthat information trivially (and maybe even then, due to compatibility\nwith older versions of git), people are going to poke around those\ndirectories.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"268618","messageId":"20150824235547.GB13261@sigill.intra.peff.net","threadId":"40139","inReplyTo":"1440449890-29490-3-git-send-email-gitster@pobox.com","subject":"Re: [PATCH v2 2/6] builtin/am: make sure state files are text","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-24T23:55:47Z","receivedAt":"2015-08-24T23:55:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 24, 2015 at 01:58:06PM -0700, Junio C Hamano wrote:\n\n> We forgot to terminate the payload given to write_file() with LF,\n> resulting in files that end with an incomplete line. Teach the\n> wrappers builtin/am uses to make sure it adds LF at the end as\n> necessary.\n\nIs it even worth doing this step? It's completely reverted later in the\nseries. I understand that we do not want to hold the fix to git-am\nhostage to write_file refactoring, but I don't see any reason these\ncannot all graduate as part of the same topic.\n\nIgnore me if you really are planning on doing the first two to \"maint\"\nand holding the others back for \"master\".\n\n-Peff\n"},{"id":"268619","messageId":"20150825000231.GC13261@sigill.intra.peff.net","threadId":"40139","inReplyTo":"1440449890-29490-1-git-send-email-gitster@pobox.com","subject":"Re: [PATCH v2 0/6] \"am\" state file fix with write_file() clean-up","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-25T00:02:31Z","receivedAt":"2015-08-25T00:02:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 24, 2015 at 01:58:04PM -0700, Junio C Hamano wrote:\n\n> The workhorse helper function that implements \"we have this (short)\n> body of text; create a new file that contains it\" has a \"fatal\"\n> parameter, to which 1 was passed by almost all callers, but to\n> casual readers, it was unclear what that 1 meant. The patch [3/6]\n> splits it to write_file() and write_file_gently() and drops this\n> parameter that looks mysterious at the callsites. A common helper\n> function write_file_v() is introduced to implement these two as thin\n> wrappers of it.\n\nTo be honest, I think the \"flags\" field is more maintainable going\nforward. Now you have _two_ functions, and any features you add to them\nhave to go in both places. In 4/6 you add the WRITE_FILE_BINARY flag,\nbut I notice that callers can't actually pass it. And adding it into\nwrite_file() would take us back to square-one with source compatibility.\n\n> The patch [4/6] updates write_file_v() so that it does the \"we are\n> writing a text file. Make sure it does not end with an incomplete\n> line\" logic that [2/6] added only to builtin/am.c, thusly reverting\n> what was done to builtin/am.c in [2/6].\n\nI notice this also converts \"fatal\" to \"flags\". It seemed weird to me\nthat did not go into patch 3, but I guess it is OK (we know that\nwrite_file_v has no outstanding callers, since we just added it).\n\n> The patch [5/6] stops all callers that creates a single-liner file\n> using write_file() and write_file_gently() from including the final\n> LF to the format they pass. This should not change the behaviour,\n> but it probably makes it conceptually cleaner. You have the contents\n> to be placed on a single line, and the helper turns the contents\n> into a proper \"line\".\n\nNice.\n\n> The patch [6/6] drops the final LF from the parameter to create a\n> multi-line file; while this does not hurt in the sense that the\n> callee will add a necessary LF back, I do not think it should be\n> applied. Conceptually, if you have a buffer that contains a bunch\n> of lines and throw it at a helper to create a file, you'd better\n> have the terminating LF yourself before asking the helper to put\n> them in the file.\n\nI agree we should drop this one.\n\n-Peff\n"},{"id":"268632","messageId":"CACsJy8A2sUEcaY2JryTHj3hvES-VDJt_eMgogP5WjVA3FiXDsg@mail.gmail.com","threadId":"40139","inReplyTo":"xmqqh9no4jhk.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/5] write_file(): introduce an explicit WRITE_FILE_GENTLY request","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-08-25T10:08:57Z","receivedAt":"2015-08-25T10:08:57Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Aug 25, 2015 at 1:41 AM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> All callers except for two ask this function to die upon error by\n>> passing fatal=1; turn the parameter to a more generic \"unsigned flag\"\n>> bag of bits, introduce an explicit WRITE_FILE_GENTLY bit and change\n>> these two callers to pass that bit.\n>\n> There is a huge iffyness around one of these two oddball callers.\n>\n>> diff --git a/setup.c b/setup.c\n>> index 5f9f07d..718f4e1 100644\n>> --- a/setup.c\n>> +++ b/setup.c\n>> @@ -404,7 +404,7 @@ static void update_linked_gitdir(const char *gitfile, const char *gitdir)\n>>\n>> strbuf_addf(&path, \"%s/gitfile\", gitdir);\n>> if (stat(path.buf, &st) || st.st_mtime + 24 * 3600 < time(NULL))\n>> - write_file(path.buf, 0, \"%s\\n\", gitfile);\n>> + write_file(path.buf, WRITE_FILE_GENTLY, \"%s\\n\", gitfile);\n>> strbuf_release(&path);\n>> }\n>\n> This comes from 23af91d1 (prune: strategies for linked checkouts,\n> 2014-11-30). I cannot tell what the justification is to treat a\n> failure to write a gitfile as a non-error event. Just a sloppy\n> coding that lets the program go through to its finish, ignoring the\n> harm done by possibly corrupting user repository silently?\n\nFailing to write to this file is not a big deal _if_ the file is not\ncorrupted because of this write operation. But we should not be so\nsilent about this. If the file content is corrupted and it's old\nenough, this checkout may be pruned. I think there's another bug\nhere... wrong name..\n-- \nDuy\n"},{"id":"268635","messageId":"1440498646-25663-1-git-send-email-pclouds@gmail.com","threadId":"40139","inReplyTo":"CACsJy8A2sUEcaY2JryTHj3hvES-VDJt_eMgogP5WjVA3FiXDsg@mail.gmail.com","subject":"[PATCH] setup: update the right file in multiple checkouts","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-08-25T10:30:46Z","receivedAt":"2015-08-25T10:30:46Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This code is introduced in 23af91d (prune: strategies for linked\ncheckouts - 2014-11-30), and it's supposed to implement this rule from\nthat commit's message:\n\n - linked checkouts are supposed to keep its location in $R/gitdir up\n to date. The use case is auto fixup after a manual checkout move.\n\nNote the name, \"$R/gitdir\", not \"$R/gitfile\". Correct the path to be\nupdated accordingly.\n\nWhile at there, make sure I/O errors are not silently dropped.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n The code was right in v2 [1] and became \"gitfile\" since v3 [2]. I\n need to reconsider my code quality after this :(\n\n [1] http://article.gmane.org/gmane.comp.version-control.git/239299\n [2] http://article.gmane.org/gmane.comp.version-control.git/242325\n\n setup.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 5f9f07d..64bf2b4 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -402,9 +402,9 @@ static void update_linked_gitdir(const char *gitfile, const char *gitdir)\n \tstruct strbuf path = STRBUF_INIT;\n \tstruct stat st;\n \n-\tstrbuf_addf(&path, \"%s/gitfile\", gitdir);\n+\tstrbuf_addf(&path, \"%s/gitdir\", gitdir);\n \tif (stat(path.buf, &st) || st.st_mtime + 24 * 3600 < time(NULL))\n-\t\twrite_file(path.buf, 0, \"%s\\n\", gitfile);\n+\t\twrite_file(path.buf, 1, \"%s\\n\", gitfile);\n \tstrbuf_release(&path);\n }\n \n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"268653","messageId":"xmqqtwrn1gu6.fsf@gitster.dls.corp.google.com","threadId":"40139","inReplyTo":"20150824235547.GB13261@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/6] builtin/am: make sure state files are text","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-25T16:19:13Z","receivedAt":"2015-08-25T16:19:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Aug 24, 2015 at 01:58:06PM -0700, Junio C Hamano wrote:\n>\n>> We forgot to terminate the payload given to write_file() with LF,\n>> resulting in files that end with an incomplete line. Teach the\n>> wrappers builtin/am uses to make sure it adds LF at the end as\n>> necessary.\n>\n> Is it even worth doing this step? It's completely reverted later in the\n> series. I understand that we do not want to hold the fix to git-am\n> hostage to write_file refactoring, but I don't see any reason these\n> cannot all graduate as part of the same topic.\n>\n> Ignore me if you really are planning on doing the first two to \"maint\"\n> and holding the others back for \"master\".\n\nNot really. The primary reason why this step exists and 1-2/6 make\na sufficient fix by themselves is because I wasn't even sure if\n3-6/6 were worth doing.\n\nAs to \"flags exposed to callers\" vs \"with and without gently\", when\nwe change the system to allow new modes of operations (e.g. somebody\nwants to write a binary file, or allocate more flag bits for their\nspecial case), I'd expect that we'd add a more general and verbose\n\"write_file_with_options(path, flags, fmt, ...)\"), gain experience\nwith that function, and then possibly introduce canned thin wrappers\n(e.g. write_binary_file() that is a synonym to passing BINARY but\nnot GENTLY) if the new thing proves widely useful, just like I left\nwrite_file() and write_file_gently() in as fairly common things to\ndo.\n"},{"id":"268655","messageId":"xmqqlhcz1fy8.fsf@gitster.dls.corp.google.com","threadId":"40139","inReplyTo":"1440498646-25663-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] setup: update the right file in multiple checkouts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-25T16:38:23Z","receivedAt":"2015-08-25T16:38:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> This code is introduced in 23af91d (prune: strategies for linked\n> checkouts - 2014-11-30), and it's supposed to implement this rule from\n> that commit's message:\n>\n> - linked checkouts are supposed to keep its location in $R/gitdir up\n> to date. The use case is auto fixup after a manual checkout move.\n>\n> Note the name, \"$R/gitdir\", not \"$R/gitfile\". Correct the path to be\n> updated accordingly.\n>\n> While at there, make sure I/O errors are not silently dropped.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> The code was right in v2 [1] and became \"gitfile\" since v3 [2]. I\n> need to reconsider my code quality after this :(\n\nHeh, don't sweat it. Everybody makes mistakes and sometimes becomes\nsloppy.\n\nThanks for double checking and correcting. Perhaps this could have\ncaught if we had some test coverage, I wonder?\n\n> [1] http://article.gmane.org/gmane.comp.version-control.git/239299\n> [2] http://article.gmane.org/gmane.comp.version-control.git/242325\n>\n> setup.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/setup.c b/setup.c\n> index 5f9f07d..64bf2b4 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -402,9 +402,9 @@ static void update_linked_gitdir(const char *gitfile, const char *gitdir)\n> \tstruct strbuf path = STRBUF_INIT;\n> \tstruct stat st;\n> \n> -\tstrbuf_addf(&path, \"%s/gitfile\", gitdir);\n> +\tstrbuf_addf(&path, \"%s/gitdir\", gitdir);\n> \tif (stat(path.buf, &st) || st.st_mtime + 24 * 3600 < time(NULL))\n> -\t\twrite_file(path.buf, 0, \"%s\\n\", gitfile);\n> +\t\twrite_file(path.buf, 1, \"%s\\n\", gitfile);\n> \tstrbuf_release(&path);\n> }\n"},{"id":"268656","messageId":"20150825164701.GA10060@sigill.intra.peff.net","threadId":"40139","inReplyTo":"xmqqtwrn1gu6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 2/6] builtin/am: make sure state files are text","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-25T16:47:01Z","receivedAt":"2015-08-25T16:47:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 25, 2015 at 09:19:13AM -0700, Junio C Hamano wrote:\n\n> As to \"flags exposed to callers\" vs \"with and without gently\", when\n> we change the system to allow new modes of operations (e.g. somebody\n> wants to write a binary file, or allocate more flag bits for their\n> special case), I'd expect that we'd add a more general and verbose\n> \"write_file_with_options(path, flags, fmt, ...)\"), gain experience\n> with that function, and then possibly introduce canned thin wrappers\n> (e.g. write_binary_file() that is a synonym to passing BINARY but\n> not GENTLY) if the new thing proves widely useful, just like I left\n> write_file() and write_file_gently() in as fairly common things to\n> do.\n\nYeah, that works. It is a bit of a gamble to me. If we never add a lot\nmore options, the end result is much nicer (callers do not deal with the\nflag option at all). But if we do, we end up with the mess that\nget_sha1_with_* and add_pending_object() got into.\n\nOne can always refactor later, too. In that sense, the BINARY flag is\nnot useful (nobody uses it, and we do not plan to do so). We could just\nmake write_file_v unconditionally complete lines[1].\n\nBut I'm OK with what you posted, as well. I think this interface is not\nworth spending a lot of time micro-nit-picking.\n\n-Peff\n\n[1] In fact, I'd be surprised if this function works well for non-text\n data anyway, as it relies on printf-style formatting. You cannot use\n it to write a string with embedded NULs, for example.\n\n If we wanted to support that case, we would probably break out:\n\n int write_buf_to_file(const char *filename,\n\t const char *buf, size_t len);\n\n as a thin wrapper for open/write_in_full/close. And then write_to_file()\n would format into a strbuf, complete a newline, and pass the result\n to it.\n"},{"id":"268676","messageId":"xmqq8u8zyzvk.fsf@gitster.dls.corp.google.com","threadId":"40139","inReplyTo":"20150825164701.GA10060@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/6] builtin/am: make sure state files are text","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-25T18:41:35Z","receivedAt":"2015-08-25T18:41:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Aug 25, 2015 at 09:19:13AM -0700, Junio C Hamano wrote:\n>\n>> As to \"flags exposed to callers\" vs \"with and without gently\", when\n>> we change the system to allow new modes of operations (e.g. somebody\n>> wants to write a binary file, or allocate more flag bits for their\n>> special case), I'd expect that we'd add a more general and verbose\n>> \"write_file_with_options(path, flags, fmt, ...)\"), gain experience\n>> with that function, and then possibly introduce canned thin wrappers\n>> (e.g. write_binary_file() that is a synonym to passing BINARY but\n>> not GENTLY) if the new thing proves widely useful, just like I left\n>> write_file() and write_file_gently() in as fairly common things to\n>> do.\n>\n> Yeah, that works. It is a bit of a gamble to me. If we never add a lot\n> more options, the end result is much nicer (callers do not deal with the\n> flag option at all). But if we do, we end up with the mess that\n> get_sha1_with_* and add_pending_object() got into.\n\nYeah. I do not know. Perhaps a good intermim solution for now\nwould be to make\n\n - write_file_l(path, flags, fmt, ...);\n\nthe low-level helper, with a single convenience wrapper:\n\n - write_file(path, fmt, ...)\n\nfor everybody other than two \"gently\" ones to use. Two \"gently\"\nones can call write_file_l(path, WRITE_FILE_GENTLY, fmt,...).\n\nYou are right about binary stuff. You could do fmt=\"...%c...\" and\npass '\\0' to corresponding place if your NUL is in the fixed part of\nthe data you are generating, but otherwise write_file() interface is\na very useful way to handle binary.\n"},{"id":"268983","messageId":"CACsJy8AegVLNkQ9fnxXeLKdh0PaU2EKAU2ti2G7qutdtAu-xag@mail.gmail.com","threadId":"40139","inReplyTo":"xmqqlhcz1fy8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] setup: update the right file in multiple checkouts","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-08-31T10:29:05Z","receivedAt":"2015-08-31T10:29:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Aug 25, 2015 at 11:38 PM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> This code is introduced in 23af91d (prune: strategies for linked\n>> checkouts - 2014-11-30), and it's supposed to implement this rule from\n>> that commit's message:\n>>\n>> - linked checkouts are supposed to keep its location in $R/gitdir up\n>> to date. The use case is auto fixup after a manual checkout move.\n>>\n>> Note the name, \"$R/gitdir\", not \"$R/gitfile\". Correct the path to be\n>> updated accordingly.\n>>\n>> While at there, make sure I/O errors are not silently dropped.\n>>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>> ---\n>> The code was right in v2 [1] and became \"gitfile\" since v3 [2]. I\n>> need to reconsider my code quality after this :(\n>\n> Heh, don't sweat it. Everybody makes mistakes and sometimes becomes\n> sloppy.\n>\n> Thanks for double checking and correcting. Perhaps this could have\n> caught if we had some test coverage, I wonder?\n\nThere's tests to check this prune functionality. They just don't\nexercise this function. Instead they manipulate \"gitdir\" file\ndirectly. I'll add a test to move repo around to make sure this code\nis exercised in the test suite.\n-- \nDuy\n"}]}