{"thread":{"id":"50550","subject":"Re: git format-patch can clobber existing patch","startedAt":"2019-02-21T14:26:05Z","lastAt":"2019-02-23T09:54:31Z","messageCount":5,"participants":["Σταύρος Ντέντος","Junio C Hamano","brian m. carlson"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"369815","messageId":"CAHMHMxUfjnNXFikVD=rys_t5BUho=7uRw4LsXgAphFwSf3xOnw@mail.gmail.com","threadId":"50550","inReplyTo":"CAHMHMxVdpOnTkf9RHzCa+YjjvpqJApsSE03Jjyb_VbJp_4q-jw@mail.gmail.com","subject":"Re: git format-patch can clobber existing patch","fromName":"Σταύρος Ντέντος","fromEmail":"stdedos@gmail.com","sentAt":"2019-02-21T14:25:26Z","receivedAt":"2019-02-21T14:26:05Z","isPatch":false,"sender":{"key":"stdedos@gmail.com","avatar":null},"body":"Hello there,\n\nDuring my work flow, I tend to use `git format-patch` to move patches\naround (nasty, I know, but bear with me)\n\nIt has occured to me that, it is possible that `git format-patch` can\nclobber existing files, if they match in the name and in the \"patch\norder\".\nHowever, if the patch is one, then, it will normally start with `0001-x.patch`\n\nIt was fine for me when I pushed updates of the same \"patch series\".\nNow that I wanted to diff a \"previous patch file\" with the patch\nupstream, however, it was almost a \"disaster\". :-)\n\nWould it make sense / be easy enough to have some clobbering check / flag?\n\nΝτέντος Σταύρος\n"},{"id":"369872","messageId":"xmqqpnrksobq.fsf@gitster-ct.c.googlers.com","threadId":"50550","inReplyTo":"CAHMHMxUfjnNXFikVD=rys_t5BUho=7uRw4LsXgAphFwSf3xOnw@mail.gmail.com","subject":"Re: git format-patch can clobber existing patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-21T23:40:09Z","receivedAt":"2019-02-21T23:40:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Σταύρος Ντέντος  <stdedos@gmail.com> writes:\n\n> During my work flow, I tend to use `git format-patch` to move patches\n> around (nasty, I know, but bear with me)\n>\n> It has occured to me that, it is possible that `git format-patch` can\n> clobber existing files, if they match in the name and in the \"patch\n> order\".\n> However, if the patch is one, then, it will normally start with `0001-x.patch`\n>\n> It was fine for me when I pushed updates of the same \"patch series\".\n> Now that I wanted to diff a \"previous patch file\" with the patch\n> upstream, however, it was almost a \"disaster\". :-)\n>\n> Would it make sense / be easy enough to have some clobbering check / flag?\n\nGiven that use of '-o' to redirect to a fresh/new directory would\nreduce the risk of such clobbering, and use of '-v' to force\ndifferent filenames would reduce the risk of such clobbering,\nit seems to me that aborting the operation when we fail to open\nthe output, without any option to override and allow clobbering,\nwould make sense.  If existing files record 4 patch series\n0001-x.patch, 0002-y.patch, 0003-z.patch, and 0004-w.patch, and you\ngenerate with \"format-patch --allow-clobbering\" a three-patch series,\nit would overwrite 0001 thru 0003 but will not remove 0004, so the\nend result will still be confusing.\n\nThis is not even compile-tested, but something along these lines may\nwork.  I am reasonably sure that existing tests won't let this patch\nalone pass, as some may depend on being able to overwrite output\nfiles left behind by previous tests.\n\n builtin/log.c | 12 +++++++++++-\n 1 file changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 3e145fe502..cb7a9eb7f9 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -864,6 +864,16 @@ static int git_format_config(const char *var, const char *value, void *cb)\n static const char *output_directory = NULL;\n static int outdir_offset;\n \n+static FILE *fopen_excl(const char *filename)\n+{\n+\tint fd = open(filename, O_CREAT | O_EXCL | O_WRONLY, 0666);\n+\tif (fd < 0) {\n+\t\terror_errno(\"%s\", filename);\n+\t\treturn NULL;\n+\t}\n+\treturn fdopen(fd, \"w\");\n+}\n+\n static int open_next_file(struct commit *commit, const char *subject,\n \t\t\t struct rev_info *rev, int quiet)\n {\n@@ -890,7 +900,7 @@ static int open_next_file(struct commit *commit, const char *subject,\n \tif (!quiet)\n \t\tprintf(\"%s\\n\", filename.buf + outdir_offset);\n \n-\tif ((rev->diffopt.file = fopen(filename.buf, \"w\")) == NULL) {\n+\tif ((rev->diffopt.file = fopen_excl(filename.buf)) == NULL) {\n \t\terror_errno(_(\"Cannot open patch file %s\"), filename.buf);\n \t\tstrbuf_release(&filename);\n \t\treturn -1;\n"},{"id":"369875","messageId":"20190222001145.GD488342@genre.crustytoothpaste.net","threadId":"50550","inReplyTo":"xmqqpnrksobq.fsf@gitster-ct.c.googlers.com","subject":"Re: git format-patch can clobber existing patch","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-02-22T00:11:46Z","receivedAt":"2019-02-22T00:11:54Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Thu, Feb 21, 2019 at 03:40:09PM -0800, Junio C Hamano wrote:\n> Σταύρος Ντέντος  <stdedos@gmail.com> writes:\n> > Would it make sense / be easy enough to have some clobbering check / flag?\n> \n> Given that use of '-o' to redirect to a fresh/new directory would\n> reduce the risk of such clobbering, and use of '-v' to force\n> different filenames would reduce the risk of such clobbering,\n> it seems to me that aborting the operation when we fail to open\n> the output, without any option to override and allow clobbering,\n> would make sense.  If existing files record 4 patch series\n> 0001-x.patch, 0002-y.patch, 0003-z.patch, and 0004-w.patch, and you\n> generate with \"format-patch --allow-clobbering\" a three-patch series,\n> it would overwrite 0001 thru 0003 but will not remove 0004, so the\n> end result will still be confusing.\n\nI think a flag for this would be useful. For people that store tarballs\n(or pristine-tar files) and patches in their repository, overwriting the\nexisting files is definitely desired.\n\nMy personal preference is that the flag be --no-clobber, but I can see\narguments for the other side as well.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"369969","messageId":"xmqqva1bpq9d.fsf@gitster-ct.c.googlers.com","threadId":"50550","inReplyTo":"20190222001145.GD488342@genre.crustytoothpaste.net","subject":"Re: git format-patch can clobber existing patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-22T19:38:54Z","receivedAt":"2019-02-22T19:38:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> On Thu, Feb 21, 2019 at 03:40:09PM -0800, Junio C Hamano wrote:\n>> Σταύρος Ντέντος  <stdedos@gmail.com> writes:\n>> > Would it make sense / be easy enough to have some clobbering check / flag?\n>> \n>> Given that use of '-o' to redirect to a fresh/new directory would\n>> reduce the risk of such clobbering, and use of '-v' to force\n>> different filenames would reduce the risk of such clobbering,\n>> it seems to me that aborting the operation when we fail to open\n>> the output, without any option to override and allow clobbering,\n>> would make sense.  If existing files record 4 patch series\n>> 0001-x.patch, 0002-y.patch, 0003-z.patch, and 0004-w.patch, and you\n>> generate with \"format-patch --allow-clobbering\" a three-patch series,\n>> it would overwrite 0001 thru 0003 but will not remove 0004, so the\n>> end result will still be confusing.\n>\n> I think a flag for this would be useful. For people that store tarballs\n> (or pristine-tar files) and patches in their repository, overwriting the\n> existing files is definitely desired.\n>\n> My personal preference is that the flag be --no-clobber, but I can see\n> arguments for the other side as well.\n\nThat's actually simpler, which is good, as we do not have to worry\nabout adjusting the existing tests that rely on the clobbering\nbehaviour ;-).\n\nI'll find time before I leave for my offline week, but this\nobviously will not be merged before the upcoming release.\n\nThanks.  I think I've mostly outlined a three-patch series to do\nthis ready.\n\n\n\n\n\n\n\n"},{"id":"370011","messageId":"CAHMHMxXxo4zXcriBJE2k3mWgwAj7KGA_AChuEmyciESGOC_7Bg@mail.gmail.com","threadId":"50550","inReplyTo":"xmqqva1bpq9d.fsf@gitster-ct.c.googlers.com","subject":"Re: git format-patch can clobber existing patch","fromName":"Σταύρος Ντέντος","fromEmail":"stdedos@gmail.com","sentAt":"2019-02-23T09:53:50Z","receivedAt":"2019-02-23T09:54:31Z","isPatch":false,"sender":{"key":"stdedos@gmail.com","avatar":null},"body":"Ντέντος Σταύρος\nOn Fri, Feb 22, 2019 at 9:38 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n>\n> > On Thu, Feb 21, 2019 at 03:40:09PM -0800, Junio C Hamano wrote:\n> >> Σταύρος Ντέντος  <stdedos@gmail.com> writes:\n> >> > Would it make sense / be easy enough to have some clobbering check / flag?\n> >>\n> >> Given that use of '-o' to redirect to a fresh/new directory would\n> >> reduce the risk of such clobbering, and use of '-v' to force\n> >> different filenames would reduce the risk of such clobbering,\n> >> it seems to me that aborting the operation when we fail to open\n> >> the output, without any option to override and allow clobbering,\n> >> would make sense.  If existing files record 4 patch series\n> >> 0001-x.patch, 0002-y.patch, 0003-z.patch, and 0004-w.patch, and you\n> >> generate with \"format-patch --allow-clobbering\" a three-patch series,\n> >> it would overwrite 0001 thru 0003 but will not remove 0004, so the\n> >> end result will still be confusing.\n\nThere might be a handful of complexities / undefined behaviors coming\nout of this; however, I think \"not\" overwriting a file to be a higher\ndirective (given that then, it is unrecoverable).\nUsing any of the '-o', '-v' might be helpful - if you are re-running\ncommands from history, however, it wouldn't necessarily provide any\nprotection ;-)\n\n> > I think a flag for this would be useful. For people that store tarballs\n> > (or pristine-tar files) and patches in their repository, overwriting the\n> > existing files is definitely desired.\n> >\n> > My personal preference is that the flag be --no-clobber, but I can see\n> > arguments for the other side as well.\n\nOn my head, I was thinging that preferably `git config\nformat-patch.no-clobber true` could be set, instead of (continuously)\ncarrying around a `--no-clobber` flag.\nI should've probably be more clear about that. However, any way is \"as\ngood as any\".\n\n> That's actually simpler, which is good, as we do not have to worry\n> about adjusting the existing tests that rely on the clobbering\n> behaviour ;-).\n>\n> I'll find time before I leave for my offline week, but this\n> obviously will not be merged before the upcoming release.\n>\n> Thanks.  I think I've mostly outlined a three-patch series to do\n> this ready.\n>\n>\n>\n>\n>\n>\n>\n"}]}