{"thread":{"id":"49665","subject":"[PATCH] sequencer: cleanup for gcc 8.2.1 warning","startedAt":"2018-10-25T00:14:20Z","lastAt":"2018-10-25T09:40:50Z","messageCount":7,"participants":["Carlo Marcelo Arenas Belón","Junio C Hamano","Carlo Arenas"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"361439","messageId":"20181025001406.6729-1-carenas@gmail.com","threadId":"49665","inReplyTo":null,"subject":"[PATCH] sequencer: cleanup for gcc 8.2.1 warning","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2018-10-25T00:14:06Z","receivedAt":"2018-10-25T00:14:20Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"sequencer.c: In function ‘write_basic_state’:\nsequencer.c:2392:37: warning: zero-length gnu_printf format string [-Wformat-zero-length]\n write_file(rebase_path_verbose(), \"\");\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 8dd6db5a01..358e83bf6b 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2335,7 +2335,7 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,\n \t\twrite_file(rebase_path_quiet(), \"\\n\");\n \n \tif (opts->verbose)\n-\t\twrite_file(rebase_path_verbose(), \"\");\n+\t\twrite_file(rebase_path_verbose(), \"\\n\");\n \tif (opts->strategy)\n \t\twrite_file(rebase_path_strategy(), \"%s\\n\", opts->strategy);\n \tif (opts->xopts_nr > 0)\n-- \n2.19.1\n\n"},{"id":"361472","messageId":"xmqqlg6mk2pd.fsf@gitster-ct.c.googlers.com","threadId":"49665","inReplyTo":"20181025001406.6729-1-carenas@gmail.com","subject":"Re: [PATCH] sequencer: cleanup for gcc 8.2.1 warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-25T05:36:14Z","receivedAt":"2018-10-25T05:36:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> sequencer.c: In function ‘write_basic_state’:\n> sequencer.c:2392:37: warning: zero-length gnu_printf format string [-Wformat-zero-length]\n> write_file(rebase_path_verbose(), \"\");\n\nThe change may squelch the above warning, but doesn't it change the\nbehaviour? You need to explain what the changed behaviour is and\nwhy it is OK to change that behaviour in this space, in addition to\nshow what warning you are working around.\n\nI _think_ the intent of this code is to create an empty file,\nbecause that is what the original version of \"git rebase\" did.\nwrite_file() does use strbuf_complete_line() to avoid creating\na file with incomplete last line, but it does allow us to create\nan empty file. By adding a LF, the created file is no longer an\nempty one.\n\nDoes it matter? IOW, does it cause a regression if we start adding\nan LF to the file?\n\nBy reading master:git-rebase.sh::read_basic_state(), I _think_ it is\nsafe to do so, as the side that consumes this $state_dir/verbose\nonly checks file's existence, and does not care what it contains,\neven if somehow the scripted version of \"git rebase\" ends up reading\nthe state file created by this newer version of \"git rebase\" in C.\nAlso next:sequencer.c::read_populate_opts() only checks for file's\nexistence, so the newer version of \"git rebase\" is also safe.\n\nSo as an immediate workaround for 'next', this patch happens to be\nOK, but that is only true because the consumer happens to ignore the\ndistinction between an empty file and a file with a single LF in it.\n\nI'd have to say that the ability to create an empty file is more\nimportant in the longer term. Can't the workaround be done to\nwrite_file() instead? I actually do not mind if the solution were\nto introduce a newhelper \"write_empty_file()\", but the way it is\nwritten in the code before this patch, i.e.\n\n\twrite_file(FILENAME, \"\")\n\nis so obvious a way to create an empty file, so if we do not have to\nresort to such a hackery to special case an empty file, that would\nbe preferrable.\n\nThanks.\n\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> sequencer.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 8dd6db5a01..358e83bf6b 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2335,7 +2335,7 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,\n> \t\twrite_file(rebase_path_quiet(), \"\\n\");\n> \n> \tif (opts->verbose)\n> -\t\twrite_file(rebase_path_verbose(), \"\");\n> +\t\twrite_file(rebase_path_verbose(), \"\\n\");\n> \tif (opts->strategy)\n> \t\twrite_file(rebase_path_strategy(), \"%s\\n\", opts->strategy);\n> \tif (opts->xopts_nr > 0)\n"},{"id":"361479","messageId":"xmqq8t2mk0jo.fsf@gitster-ct.c.googlers.com","threadId":"49665","inReplyTo":"xmqqlg6mk2pd.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] sequencer: cleanup for gcc 8.2.1 warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-25T06:22:51Z","receivedAt":"2018-10-25T06:22:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I'd have to say that the ability to create an empty file is more\n> important in the longer term. Can't the workaround be done to\n> write_file() instead? I actually do not mind if the solution were\n> to introduce a newhelper \"write_empty_file()\", but the way it is\n> written in the code before this patch, i.e.\n>\n> \twrite_file(FILENAME, \"\")\n>\n> is so obvious a way to create an empty file, so if we do not have to\n> resort to such a hackery to special case an empty file, that would\n> be preferrable.\n\nIt turns out that we have dealt with this before.\n\nThe trick employed by 7d7d6802 (\"silence a bunch of\nformat-zero-length warnings\", 2014-05-04) is still a caller side\nworkaround, but to do\n\n\t-\tstatus_printf_ln(s, GIT_COLOR_NORMAL, \"\");\n\t+\tstatus_printf_ln(s, GIT_COLOR_NORMAL, \"%s\", \"\");\n\nto the function whose third parameter is printf format.\n\nI do not know if it is a good idea to define a macro\n\n\t#define\tEMPTY_CONTENTS\t\"%s\", \"\"\n\nin git-compat-util.h and then replace all the occurrences of \"%s\", \"\"\nin the source code with it. That way, we'd be able to create an\nempty file with\n\n\twrite_file(FILENAME, EMPTY_CONTENTS);\n\nand write out an empty line with\n\n\tstatus_printf_ln(s, GIT_COLOR_NORMAL, EMPTY_CONTENTS);\n\nand they would read naturally. But may be it is a bit too cute an\nidea? I dunno.\n\n\n\t\n"},{"id":"361483","messageId":"CAPUEsphHsWVzdGXBB0EOSNuUb5gNgjyJ+i7PnDJx7LBqejQrNQ@mail.gmail.com","threadId":"49665","inReplyTo":"xmqq8t2mk0jo.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] sequencer: cleanup for gcc 8.2.1 warning","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2018-10-25T07:10:50Z","receivedAt":"2018-10-25T07:11:03Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Oct 24, 2018 at 11:22 PM Junio C Hamano wrote:\n> and they would read naturally. But may be it is a bit too cute an\n> idea? I dunno.\n\nmy first idea was to replace it with a helper called touch_file, since\nI was expecting it will be a popular operation as flag files are\ncommon in shell scripts and that name will be second nature to anyone\nporting them.\n\nthe fact that the quiet flag was created with a single '\\n' in the\ncode just immediately above this make me go for the proposed\n\"solution\" instead (which I verified wouldn't change behaviour as you\ndescribed in your post; I apologize for not documenting it in the\ncommit and wasting your time).\n\nwould something like this work better? (not to apply, and probably mangled)\n\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -35,6 +35,8 @@\n\n #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n\n+#define touch_file(path) write_file(path, \"%s\", \"\")\n+\n const char sign_off_header[] = \"Signed-off-by: \";\n static const char cherry_picked_prefix[] = \"(cherry picked from commit \";\n\n@@ -2389,7 +2391,7 @@ int write_basic_state(struct replay_opts *opts,\nconst char *head_name,\n write_file(rebase_path_quiet(), \"\\n\");\n\n if (opts->verbose)\n- write_file(rebase_path_verbose(), \"\");\n+ touch_file(rebase_path_verbose());\n if (opts->strategy)\n write_file(rebase_path_strategy(), \"%s\\n\", opts->strategy);\n if (opts->xopts_nr > 0)\n"},{"id":"361484","messageId":"xmqqwoq6ih34.fsf@gitster-ct.c.googlers.com","threadId":"49665","inReplyTo":"CAPUEsphHsWVzdGXBB0EOSNuUb5gNgjyJ+i7PnDJx7LBqejQrNQ@mail.gmail.com","subject":"Re: [PATCH] sequencer: cleanup for gcc 8.2.1 warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-25T08:08:31Z","receivedAt":"2018-10-25T08:08:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas writes:\n\n> would something like this work better? (not to apply, and probably mangled)\n\nAt least call it \"create_empty_file(path)\" instead.\n\n\"touch\" is primarily to update the last-modified-time timestamp of a\nfile. If the file does not exist, it is created while doing so, but\nwhen readers of your code sees a \"touch\", you make them anticipate\nthat you are relying on file timestamp somehow. Using it to create\nan empty file wastes time of readers who read your code by forcing\nthem wonder why you care about the file timestamp.\n\nFor a single-use, not using the macro and just using \"%s\", \"\" should\nsuffice. If we want to hide the \"%s\", \"\" trickery from distracting\nthe readers (which is what you are trying to address with your\ntouch_file() proposal, I think, and I also suspect that it may be a\nlegitimate one), I tend to think that it may be a better solution to\nintroduce the EMPTY_PRINTF_ARG macro and use it everywhere, in\nbuiltin/commit.c, builtin/difftool.c and wt-status.c and not not\njust here.\n\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -35,6 +35,8 @@\n>\n> #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n>\n> +#define touch_file(path) write_file(path, \"%s\", \"\")\n> +\n> const char sign_off_header[] = \"Signed-off-by: \";\n> static const char cherry_picked_prefix[] = \"(cherry picked from commit \";\n>\n> @@ -2389,7 +2391,7 @@ int write_basic_state(struct replay_opts *opts,\n> const char *head_name,\n> write_file(rebase_path_quiet(), \"\\n\");\n>\n> if (opts->verbose)\n> - write_file(rebase_path_verbose(), \"\");\n> + touch_file(rebase_path_verbose());\n> if (opts->strategy)\n> write_file(rebase_path_strategy(), \"%s\\n\", opts->strategy);\n> if (opts->xopts_nr > 0)\n"},{"id":"361495","messageId":"CAPUEsphFSs3gmo2N6Oi7rhhsgU2BC3ijZH4yxbcF1R8SKQLjhg@mail.gmail.com","threadId":"49665","inReplyTo":"xmqqwoq6ih34.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] sequencer: cleanup for gcc 8.2.1 warning","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2018-10-25T09:15:17Z","receivedAt":"2018-10-25T09:15:31Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Oct 25, 2018 at 1:08 AM Junio C Hamano wrote:\n> For a single-use, not using the macro and just using \"%s\", \"\" should\n> suffice.\n\nOK, will send it as v2 then but would think it will be better if\napplied as a \"fixup\" on top of the original branch:\n34b47315d9 (\"rebase -i: move rebase--helper modes to\nrebase--interactive\", 2018-09-27)\n\nwould be a good idea to include also enabling this warning in\ndeveloper mode so it doesn't sneak back?, presume as the last patch of\nthe refactor below?, FWIW this is the only case in current pu, and I\nsuspect the sooner we add it to next the less likely we will find\nmore.\n\n> If we want to hide the \"%s\", \"\" trickery from distracting\n> the readers (which is what you are trying to address with your\n> touch_file() proposal, I think, and I also suspect that it may be a\n> legitimate one), I tend to think that it may be a better solution to\n> introduce the EMPTY_PRINTF_ARG macro and use it everywhere, in\n> builtin/commit.c, builtin/difftool.c and wt-status.c and not not\n> just here.\n\nwill work on this in a different feature branch, but I had to admit I\ndon't like it for status_printf case where it was IMHO a hack to get\nthe new lines to begin with.\n\nI would think it would make more sense to call a function that says\n\"write_ttycolor_ln\" instead for those cases.\n\nCarlo\n"},{"id":"361499","messageId":"20181025093854.12783-1-carenas@gmail.com","threadId":"49665","inReplyTo":"CAPUEsphFSs3gmo2N6Oi7rhhsgU2BC3ijZH4yxbcF1R8SKQLjhg@mail.gmail.com","subject":"[PATCH v2] sequencer: cleanup for gcc warning in non developer mode","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2018-10-25T09:38:54Z","receivedAt":"2018-10-25T09:40:50Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"as shown by:\n\n sequencer.c: In function ‘write_basic_state’:\n sequencer.c:2392:37: warning: zero-length gnu_printf format string [-Wformat-zero-length]\n write_file(rebase_path_verbose(), \"\");\n\nwhere write_file will create an empty file if told to write an empty string\nas can be inferred by the previous call\n\nthe somehow more convoluted syntax works around the issue by providing a non\nempty format string and is already being used for the abort safety file since\n1e41229d96 (\"sequencer: make sequencer abort safer\", 2016-12-07)\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n\nNotes:\n Changes in v2\n \n - Avoid change of behaviour as suggested by Junio\n\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 8dd6db5a01..10f602c4d4 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2335,7 +2335,7 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,\n \t\twrite_file(rebase_path_quiet(), \"\\n\");\n \n \tif (opts->verbose)\n-\t\twrite_file(rebase_path_verbose(), \"\");\n+\t\twrite_file(rebase_path_verbose(), \"%s\", \"\");\n \tif (opts->strategy)\n \t\twrite_file(rebase_path_strategy(), \"%s\\n\", opts->strategy);\n \tif (opts->xopts_nr > 0)\n-- \n2.19.1\n\n"}]}