{"thread":{"id":"48779","subject":"[GSoC][PATCH 0/1] sequencer: print an error message if append_todo_help() fails","startedAt":"2018-06-26T10:05:55Z","lastAt":"2018-06-26T21:52:29Z","messageCount":5,"participants":["Alban Gruin","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"350953","messageId":"20180626100429.10169-1-alban.gruin@gmail.com","threadId":"48779","inReplyTo":null,"subject":"[GSoC][PATCH 0/1] sequencer: print an error message if append_todo_help() fails","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-26T10:04:28Z","receivedAt":"2018-06-26T10:05:55Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Currently, append_todo_help() does not warn the user if an error occurs\nwhen trying to write to the todo file. This patch addresses this\nproblem.\n\nThis patch is based on ag/rebase-i-append-todo-help.\n\nAlban Gruin (1):\n sequencer: print an error message if append_todo_help() fails\n\n sequencer.c | 3 +++\n 1 file changed, 3 insertions(+)\n\n-- \n2.18.0\n\n"},{"id":"350954","messageId":"20180626100429.10169-2-alban.gruin@gmail.com","threadId":"48779","inReplyTo":"20180626100429.10169-1-alban.gruin@gmail.com","subject":"[GSoC][PATCH 1/1] sequencer: print an error message if append_todo_help() fails","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-26T10:04:29Z","receivedAt":"2018-06-26T10:05:58Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"This adds an error when append_todo_help() fails to write its message to\nthe todo file.\n\nSigned-off-by: Alban Gruin \n---\n sequencer.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 7cc76332e..7c4bdbb99 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4380,6 +4380,9 @@ int append_todo_help(unsigned edit_todo, unsigned keep_empty)\n \t}\n \n \tret = fputs(buf.buf, todo);\n+\tif (ret)\n+\t\terror_errno(_(\"Could not append help text to '%s'\"), rebase_path_todo());\n+\n \tfclose(todo);\n \tstrbuf_release(&buf);\n \n-- \n2.18.0\n\n"},{"id":"350967","messageId":"nycvar.QRO.7.76.6.1806261455510.21419@tvgsbejvaqbjf.bet","threadId":"48779","inReplyTo":"20180626100429.10169-2-alban.gruin@gmail.com","subject":"Re: [GSoC][PATCH 1/1] sequencer: print an error message if append_todo_help() fails","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-06-26T12:57:37Z","receivedAt":"2018-06-26T12:57:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Alban,\n\nOn Tue, 26 Jun 2018, Alban Gruin wrote:\n\n> This adds an error when append_todo_help() fails to write its message to\n> the todo file.\n> \n> Signed-off-by: Alban Gruin \n\nACK.\n\nWe *may* want to fold that into the commit that adds `append_todo_help()`.\nAnd, as I mentioned previously, I would love for that function to be used\nas an excuse to introduce the long-overdue `interactive-rebase.c`\n(`sequencer.c` is supposed to be the backend for cherry-pick and revert\nand interactive rebase, but not a catch-all for *all* of those things, it\nis already way too long to be readable, and I take blame for a large part\nof that.)\n\nCiao,\nDscho\n"},{"id":"351000","messageId":"xmqqk1qljpe0.fsf@gitster-ct.c.googlers.com","threadId":"48779","inReplyTo":"nycvar.QRO.7.76.6.1806261455510.21419@tvgsbejvaqbjf.bet","subject":"Re: [GSoC][PATCH 1/1] sequencer: print an error message if append_todo_help() fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-26T19:53:27Z","receivedAt":"2018-06-26T19:53:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Alban,\n>\n> On Tue, 26 Jun 2018, Alban Gruin wrote:\n>\n>> This adds an error when append_todo_help() fails to write its message to\n>> the todo file.\n>> \n>> Signed-off-by: Alban Gruin \n>\n> ACK.\n>\n> We *may* want to fold that into the commit that adds `append_todo_help()`.\n\nAbsolutely. This looks more like an \"oops, I made a mess and here\nis a fix on top\", and even worse, it does not make an effort to help\nreaders where the mess was made (iow, which commit it goes on to\nof); it is better to be squashed in.\n\nI do not know offhand who Alban's mentors are, but one thing I think\nis a good thing for them to teach is how to better organize the\nchanges with readers in mind. The author of a patch series knows\nhis or her patches and how they relate to each other a lot better\nthan the readers of patches, who are reading not just his or her\npatches but the ones from a lot wider set of contributors. Even\nthough append-todo-help and edit-todo may have been developed as\nseparate steps in author's mind, it is criminal to send them as if\nthey are completely separate topics that can independently applied,\nespecially when one depends on the other. It is a lot more helpful\nto the readers if they were sent as a larger single series, because\ndoing so _will_ tell the readers which order the dependency goes.\n\n> And, as I mentioned previously, I would love for that function to be used\n> as an excuse to introduce the long-overdue `interactive-rebase.c`\n\nI am not sure if I like this direction. \n\nAs newbies are often very bad at coming up with APIs and naming\nglobal functions, keeping everything as \"static\" inside a single\nsequencer.c tends to avoid contaminating the global namespace.\n"},{"id":"351043","messageId":"nycvar.QRO.7.76.6.1806262348340.21419@tvgsbejvaqbjf.bet","threadId":"48779","inReplyTo":"xmqqk1qljpe0.fsf@gitster-ct.c.googlers.com","subject":"Re: [GSoC][PATCH 1/1] sequencer: print an error message if append_todo_help() fails","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-06-26T21:52:18Z","receivedAt":"2018-06-26T21:52:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 26 Jun 2018, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > On Tue, 26 Jun 2018, Alban Gruin wrote:\n> >\n> >> This adds an error when append_todo_help() fails to write its message to\n> >> the todo file.\n> >> \n> >> Signed-off-by: Alban Gruin \n> >\n> > ACK.\n> >\n> > We *may* want to fold that into the commit that adds `append_todo_help()`.\n> \n> Absolutely. This looks more like an \"oops, I made a mess and here\n> is a fix on top\", and even worse, it does not make an effort to help\n> readers where the mess was made (iow, which commit it goes on to\n> of); it is better to be squashed in.\n> \n> I do not know offhand who Alban's mentors are, but one thing I think\n> is a good thing for them to teach is how to better organize the\n> changes with readers in mind. The author of a patch series knows\n> his or her patches and how they relate to each other a lot better\n> than the readers of patches, who are reading not just his or her\n> patches but the ones from a lot wider set of contributors. Even\n> though append-todo-help and edit-todo may have been developed as\n> separate steps in author's mind, it is criminal to send them as if\n> they are completely separate topics that can independently applied,\n> especially when one depends on the other. It is a lot more helpful\n> to the readers if they were sent as a larger single series, because\n> doing so _will_ tell the readers which order the dependency goes.\n\nChris & Stefan are Alban's mentors, and I spend quite a bit of my time on\nIRC, ready to help Alban when he has questions. Chris & Stephan mainly act\nas first-line reviewers.\n\n> > And, as I mentioned previously, I would love for that function to be\n> > used as an excuse to introduce the long-overdue `interactive-rebase.c`\n> \n> I am not sure if I like this direction.\n\nBlame me, not Alban. I am pretty familiar with sequencer.c, and I know\nthat it is way too large.\n\n> As newbies are often very bad at coming up with APIs and naming global\n> functions, keeping everything as \"static\" inside a single sequencer.c\n> tends to avoid contaminating the global namespace.\n\nThen I just need to make sure to suggest good names that are safe for the\nglobal namespace, don't I?\n\nSeeing as sequencer.c is so long already, it is own little mega namespace\nanyway, so we already have to be very careful *within* sequencer.c.\n\nCiao,\nDscho\n"}]}