{"thread":{"id":"60364","subject":"[PATCH] bugreport: add 'seconds' to default outfile name","startedAt":"2023-10-14T04:01:17Z","lastAt":"2024-01-06T04:54:45Z","messageCount":23,"participants":["Jacob Stopak","Kristoffer Haugsbakk","Junio C Hamano","Dragan Simic","Emily Shaffer"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"483239","messageId":"20231014040101.8333-1-jacob@initialcommit.io","threadId":"60364","inReplyTo":null,"subject":"[PATCH] bugreport: add 'seconds' to default outfile name","fromName":"Jacob Stopak","fromEmail":"jacob@initialcommit.io","sentAt":"2023-10-14T04:01:01Z","receivedAt":"2023-10-14T04:01:17Z","isPatch":true,"sender":{"key":"jacob@initialcommit.io","avatar":"https://avatars.githubusercontent.com/u/49353917?v=4"},"body":"Currently, git bugreport postfixes the default bugreport filename (and\ndiagnostics zip filename if --diagnose is supplied) with the current\ncalendar hour and minute values, assuming the -s flag is absent.\n\nIf a user runs the bugreport command more than once within a calendar\nminute, a filename conflict with an existing file occurs and the program\nerrors, since the new output filename was already used for the previous\nfile. If the user waits anywhere from 1 to 60 seconds (depending on\n_when_ during the calendar minute the first command was run) the command\nworks again with no error since the default filename is now unique, and\nmultiple bug reports are able to be created with default settings.\n\nThis is a minor thing but can cause confusion especially for first time\nusers of the bugreport command, who are likely to run it multiple times\nin quick succession to learn how it works, (like I did).\n\nThis patch adds the calendar second value to the default bugreport\nfilename. This technically just shortens the window during which the\nissue occurs, but a single second is a small enough time increment that\nusers will avoid the filename conflict in practice in this scenario.\n\nThis means the user will end up with multiple bugreport files being\ncreated if they run the command multiple times quickly, but that feels\nmore intuitive and consistent than an error arbitrarily occuring within\na calendar minute, especially given that the time window in which the\nerror currently occurs is variable as described above.\n\nSigned-off-by: Jacob Stopak <jacob@initialcommit.io>\n---\n builtin/bugreport.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/bugreport.c b/builtin/bugreport.c\nindex d2ae5c305d..b556c6e135 100644\n--- a/builtin/bugreport.c\n+++ b/builtin/bugreport.c\n@@ -106,7 +106,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \tstruct tm tm;\n \tenum diagnose_mode diagnose = DIAGNOSE_NONE;\n \tchar *option_output = NULL;\n-\tchar *option_suffix = \"%Y-%m-%d-%H%M\";\n+\tchar *option_suffix = \"%Y-%m-%d-%H%M%S\";\n \tconst char *user_relative_path = NULL;\n \tchar *prefixed_filename;\n \tsize_t output_path_len;\n\nbase-commit: 493f4622739e9b64f24b465b21aa85870dd9dc09\n-- \n2.42.0.297.g393e7d1581\n\n"},{"id":"483242","messageId":"90bf6f0e-6061-4670-aa04-9d6e44b1d246@app.fastmail.com","threadId":"60364","inReplyTo":"20231014040101.8333-1-jacob@initialcommit.io","subject":"Re: [PATCH] bugreport: add 'seconds' to default outfile name","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-14T10:35:06Z","receivedAt":"2023-10-14T10:35:33Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Hi Jacob\n\nOn Sat, Oct 14, 2023, at 06:01, Jacob Stopak wrote:\n> This patch adds the calendar second value to the default bugreport\n\nNitpick: you can just say “Add the calendar”. “This patch” is redundant.\n\nSee `Documentation/SubmittingPatches` at `imperative-mood`.\n\n-- \nKristoffer\n"},{"id":"483249","messageId":"xmqq4jitw4nk.fsf@gitster.g","threadId":"60364","inReplyTo":"20231014040101.8333-1-jacob@initialcommit.io","subject":"Re: [PATCH] bugreport: add 'seconds' to default outfile name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-14T16:27:27Z","receivedAt":"2023-10-14T16:27:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Stopak <jacob@initialcommit.io> writes:\n\n> Currently, git bugreport postfixes the default bugreport filename (and\n> diagnostics zip filename if --diagnose is supplied) with the current\n> calendar hour and minute values, assuming the -s flag is absent.\n\nIs \"postfix\" a verb that is commonly understood?  I would say\n\"append\" would be understood by more readers.  Also, is \"calendar\"\nhour different from other kinds of hours, perhaps stopwatch hours\nand microwave-oven hours?\n\n> If a user runs the bugreport command more than once within a calendar\n> minute, a filename conflict with an existing file occurs and the program\n> errors, since the new output filename was already used for the previous\n> file.\n\nThis is totally expected and you made an excellent observation.\n\nI personally do not think it is a problem, simply because a quality\nbug report that would capture information necessary to diagnose any\nissue concisely in a readable fashion would take at least 90 seconds\nor more to produce, though.\n\nInstead of lengthening the filename for all files by 2 digits, the\ncommand can retry by adding say \"+1\", \"+2\", etc. after the failed\nfilename to find a unique suffix within the same minute.  It would\nmean that after writing git-bugreport-2023-10-14-0920.txt and you\nstart another one without spending enough time, the new one may\nbecome git-bugreport-2023-10-14-0920+1.txt or something unique.  It\nwould be really unlikely that you would run out after failing to\nfind a vacant single digit suffix nine times, i.e. trying \"+9\".  It\nwould also help preserve existing user's workflow, e.g. they may\nhave written automation that assumes the down-to-minute format and\nit would keep working on their bug reports without breaking.\n"},{"id":"483250","messageId":"833da2d05d4b1dfa8e561aa638a927b0@manjaro.org","threadId":"60364","inReplyTo":"xmqq4jitw4nk.fsf@gitster.g","subject":"Re: [PATCH] bugreport: add 'seconds' to default outfile name","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2023-10-14T16:33:54Z","receivedAt":"2023-10-14T16:33:59Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2023-10-14 18:27, Junio C Hamano wrote:\n> Jacob Stopak <jacob@initialcommit.io> writes:\n>> If a user runs the bugreport command more than once within a calendar\n>> minute, a filename conflict with an existing file occurs and the \n>> program\n>> errors, since the new output filename was already used for the \n>> previous\n>> file.\n> \n> This is totally expected and you made an excellent observation.\n> \n> I personally do not think it is a problem, simply because a quality\n> bug report that would capture information necessary to diagnose any\n> issue concisely in a readable fashion would take at least 90 seconds\n> or more to produce, though.\n> \n> Instead of lengthening the filename for all files by 2 digits, the\n> command can retry by adding say \"+1\", \"+2\", etc. after the failed\n> filename to find a unique suffix within the same minute.  It would\n> mean that after writing git-bugreport-2023-10-14-0920.txt and you\n> start another one without spending enough time, the new one may\n> become git-bugreport-2023-10-14-0920+1.txt or something unique.\n\nHow about making the filename a bit shorter first, to make room for the \nadditional two digits, so the example mentioned above would end up being \nnamed \"git-bugreport-20231014-092037.txt\"?\n"},{"id":"483255","messageId":"xmqqa5slt7wa.fsf@gitster.g","threadId":"60364","inReplyTo":"833da2d05d4b1dfa8e561aa638a927b0@manjaro.org","subject":"Re: [PATCH] bugreport: add 'seconds' to default outfile name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-14T17:45:41Z","receivedAt":"2023-10-14T17:45:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic <dsimic@manjaro.org> writes:\n\n> How about making the filename a bit shorter first, to make room for\n> the additional two digits, so the example mentioned above would end up\n> being named \"git-bugreport-20231014-092037.txt\"?\n\nThe reason I stated not to unconditionally add two more digits is\n*not* that I wanted to keep things shorter.  I wanted to keep names\nstable and in the same shape as before for sensible people who spend\nmore than 60 seconds---only those who produce more than one within\nthe same minute will be affected.\n\nWhat is your reason to want to make the filename shoter first?\nIt would break everybody, wouldn't it?\n\n"},{"id":"483256","messageId":"438a5edf1a17ffac201436a950ce50fa@manjaro.org","threadId":"60364","inReplyTo":"xmqqa5slt7wa.fsf@gitster.g","subject":"Re: [PATCH] bugreport: add 'seconds' to default outfile name","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2023-10-14T17:52:32Z","receivedAt":"2023-10-14T17:52:36Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2023-10-14 19:45, Junio C Hamano wrote:\n> Dragan Simic <dsimic@manjaro.org> writes:\n> \n>> How about making the filename a bit shorter first, to make room for\n>> the additional two digits, so the example mentioned above would end up\n>> being named \"git-bugreport-20231014-092037.txt\"?\n> \n> The reason I stated not to unconditionally add two more digits is\n> *not* that I wanted to keep things shorter.  I wanted to keep names\n> stable and in the same shape as before for sensible people who spend\n> more than 60 seconds---only those who produce more than one within\n> the same minute will be affected.\n> \n> What is your reason to want to make the filename shoter first?\n> It would break everybody, wouldn't it?\n\nPlease note that I haven't researched in detail what else depends on the \ncurrent filename format, which presumably is a whole bunch of stuff, I \njust suggested this filename compaction because I understood that the \nfilename length was becoming an issue.\n\nSpeaking in general, I somehow find \"20220712\" a bit more readable than \n\"2022-07-12\" as part of a filename, but that's of course just my \npersonal preference.\n"},{"id":"483272","messageId":"ZStWB1/LX7cTbVGr.jacob@initialcommit.io","threadId":"60364","inReplyTo":"xmqq4jitw4nk.fsf@gitster.g","subject":"Re: [PATCH] bugreport: add 'seconds' to default outfile name","fromName":"Jacob Stopak","fromEmail":"jacob@initialcommit.io","sentAt":"2023-10-15T03:01:27Z","receivedAt":"2023-10-15T03:01:33Z","isPatch":true,"sender":{"key":"jacob@initialcommit.io","avatar":"https://avatars.githubusercontent.com/u/49353917?v=4"},"body":"On Sat, Oct 14, 2023 at 09:27:27AM -0700, Junio C Hamano wrote:\n> Jacob Stopak <jacob@initialcommit.io> writes:\n> \n> Is \"postfix\" a verb that is commonly understood?  I would say\n> \"append\" would be understood by more readers.\n\nIt's probably true that \"append\" or \"suffix\" (which is used in the code)\nwould be more easily understood. I'll switch in my updated messages.\n\n> Also, is \"calendar\"\n> hour different from other kinds of hours, perhaps stopwatch hours\n> and microwave-oven hours?\n\nLol! By saying \"calendar\" I mean \"falling on the official boundaries\nof\", like 11:15:00 - 11:16:00. Unlike the time between 11:15:30 -\n11:16:30 which is also a minute, but it's not a \"calendar\" minute\nbecause it overlaps into the next minute. I guess in this case it's more\nof a \"clock\" minute than a \"calendar\" minute though ':D... I guess\n\"calendar\" terminology is used more for months/years...\n\n> I personally do not think it is a problem, simply because a quality\n> bug report that would capture information necessary to diagnose any\n> issue concisely in a readable fashion would take at least 90 seconds\n> or more to produce, though.\n\nThis is true, when the user intentionally opens the bugreport with the\nintent to start filling it out immediately, I assume they would almost\nalways cross the minute barrier and avoid the issue.\n\nHowever, there are edge cases like the one I outlined, where the user\nmight open and close the report quickly, followed by rerunning the\ncommand. This could be someone learning to use the command for the first\ntime. Or the case where a user only fills in a small part of the report\nbefore closing it and running the command again.\n\nThese cases are certainly \"the exception\" but it seems the program could\nbe a bit more consistent/intuitive when they do occur.\n\n> Instead of lengthening the filename for all files by 2 digits, the\n> command can retry by adding say \"+1\", \"+2\", etc. after the failed\n> filename to find a unique suffix within the same minute.  It would\n> mean that after writing git-bugreport-2023-10-14-0920.txt and you\n> start another one without spending enough time, the new one may\n> become git-bugreport-2023-10-14-0920+1.txt or something unique.  It\n> would be really unlikely that you would run out after failing to\n> find a vacant single digit suffix nine times, i.e. trying \"+9\".  It\n> would also help preserve existing user's workflow, e.g. they may\n> have written automation that assumes the down-to-minute format and\n> it would keep working on their bug reports without breaking.\n\nI agree with all of this, and to me it's a better solution than\n_appending_ the second value :). I have a patchset almost ready for this \nso I'll try to submit it later tonight.\n"},{"id":"483274","messageId":"ZStXbjgFTlO11Pp7.jacob@initialcommit.io","threadId":"60364","inReplyTo":"438a5edf1a17ffac201436a950ce50fa@manjaro.org","subject":"Re: [PATCH] bugreport: add 'seconds' to default outfile name","fromName":"Jacob Stopak","fromEmail":"jacob@initialcommit.io","sentAt":"2023-10-15T03:07:26Z","receivedAt":"2023-10-15T03:07:30Z","isPatch":true,"sender":{"key":"jacob@initialcommit.io","avatar":"https://avatars.githubusercontent.com/u/49353917?v=4"},"body":"On Sat, Oct 14, 2023 at 07:52:32PM +0200, Dragan Simic wrote:\n> \n> Speaking in general, I somehow find \"20220712\" a bit more readable than\n> \"2022-07-12\" as part of a filename, but that's of course just my personal\n> preference.\n\nIt's funny how we all have our own preferences for this kind of thing.\nMine happens to be separating the date part from the rest of the\nfilename with an underscore, but using a hyphen to separate individual\ndate components like:\n\nfilename_2022-07-12.ext\n"},{"id":"483275","messageId":"d0e4d8f24a0d7a9bada2b05631a836a6@manjaro.org","threadId":"60364","inReplyTo":"ZStXbjgFTlO11Pp7.jacob@initialcommit.io","subject":"Re: [PATCH] bugreport: add 'seconds' to default outfile name","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2023-10-15T03:13:00Z","receivedAt":"2023-10-15T03:13:05Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2023-10-15 05:07, Jacob Stopak wrote:\n> On Sat, Oct 14, 2023 at 07:52:32PM +0200, Dragan Simic wrote:\n>> \n>> Speaking in general, I somehow find \"20220712\" a bit more readable \n>> than\n>> \"2022-07-12\" as part of a filename, but that's of course just my \n>> personal\n>> preference.\n> \n> It's funny how we all have our own preferences for this kind of thing.\n> Mine happens to be separating the date part from the rest of the\n> filename with an underscore, but using a hyphen to separate individual\n> date components like:\n> \n> filename_2022-07-12.ext\n\nYes, it's quite funny.  I gave it some thought, and I think that my \npreference is a result of dealing with many PDF files containing \nschematics, for which some kind of a defacto standard is the \"-20220712\" \nnaming convention.\n"},{"id":"483278","messageId":"20231015034238.100675-1-jacob@initialcommit.io","threadId":"60364","inReplyTo":"20231014040101.8333-1-jacob@initialcommit.io","subject":"[PATCH v2 0/3] bugreport: include +i in outfile suffix as needed","fromName":"Jacob Stopak","fromEmail":"jacob@initialcommit.io","sentAt":"2023-10-15T03:42:34Z","receivedAt":"2023-10-15T03:42:58Z","isPatch":true,"sender":{"key":"jacob@initialcommit.io","avatar":"https://avatars.githubusercontent.com/u/49353917?v=4"},"body":"Update git bugreport to allow creation of multiple bugreports with\ndefault settings during a given minute interval.\n\nAddress several edge cases where users might run git bugreport multiple\ntimes within a minute and expect multiple reports to be created.\n\nHandle these cases by checking to see if a file with the default\ntimestamped filename already exists, and if so, appending a '+1' value\nto the filename suffix. Keep doing so until a unique filename is reached\nor '+9' is reached. At this point, if uniqueness is still not found,\nthe previous error that a file with the given name already exists.\n\nIf the --diagnose flag is supplied, apply the same '+i' incremented\nfilename suffix to the diagnostics zip file.\n\nReorder the code block that creates the diagnostics zip file so it isn't\ncreated in the event the bugreport itself isn't created due to an error.\n\nJacob Stopak (3):\n  bugreport: include +i in outfile suffix as needed\n  bugreport: match diagnostics filename with report\n  bugreport: don't create --diagnose zip w/o report\n\n builtin/bugreport.c | 54 ++++++++++++++++++++++++++++++---------------\n 1 file changed, 36 insertions(+), 18 deletions(-)\n\n\nbase-commit: 493f4622739e9b64f24b465b21aa85870dd9dc09\n-- \n2.42.0.298.gd89efca819.dirty\n\n"},{"id":"483279","messageId":"20231015034238.100675-2-jacob@initialcommit.io","threadId":"60364","inReplyTo":"20231015034238.100675-1-jacob@initialcommit.io","subject":"[PATCH v2 1/3] bugreport: include +i in outfile suffix as needed","fromName":"Jacob Stopak","fromEmail":"jacob@initialcommit.io","sentAt":"2023-10-15T03:42:35Z","receivedAt":"2023-10-15T03:43:11Z","isPatch":true,"sender":{"key":"jacob@initialcommit.io","avatar":"https://avatars.githubusercontent.com/u/49353917?v=4"},"body":"When the -s flag is absent, git bugreport includes the current hour and\nminute values in the default bugreport filename (and diagnostics zip\nfilename if --diagnose is supplied).\n\nIf a user runs the bugreport command more than once within a calendar\nminute, a filename conflict with an existing file occurs and the program\nerrors, since the new output filename was already used for the previous\nfile. If the user waits anywhere from 1 to 60 seconds (depending on\n_when during the calendar minute_ the first command was run) the command\nworks again with no error since the default filename is now unique, and\nmultiple bug reports are able to be created with default settings.\n\nThis is a minor thing but can cause confusion especially for first time\nusers of the bugreport command, who are likely to run it multiple times\nin quick succession to learn how it works, (like I did).\n\nAdd a '+i' into the bugreport filename suffix to make the filename\nunique, where 'i' is an integer starting at 1 and able to grow up to 9\nin the unlikely event a user runs the command 9 times in a single\nminute. This leads to default output filenames like:\n\ngit-bugreport-%Y-%m-%d-%H%M+1.txt\ngit-bugreport-%Y-%m-%d-%H%M+2.txt\n...\ngit-bugreport-%Y-%m-%d-%H%M+9.txt\n\nThis means the user will end up with multiple bugreport files being\ncreated if they run the command multiple times quickly, but that feels\nmore intuitive and consistent than an error arbitrarily occuring within\na calendar minute, especially given that the time window in which the\nerror currently occurs is variable as described above.\n\nSigned-off-by: Jacob Stopak <jacob@initialcommit.io>\n---\n builtin/bugreport.c | 26 +++++++++++++++++++++-----\n 1 file changed, 21 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/bugreport.c b/builtin/bugreport.c\nindex d2ae5c305d..71ee7d7f4b 100644\n--- a/builtin/bugreport.c\n+++ b/builtin/bugreport.c\n@@ -3,7 +3,6 @@\n #include \"editor.h\"\n #include \"gettext.h\"\n #include \"parse-options.h\"\n-#include \"strbuf.h\"\n #include \"help.h\"\n #include \"compat/compiler.h\"\n #include \"hook.h\"\n@@ -11,6 +10,7 @@\n #include \"diagnose.h\"\n #include \"object-file.h\"\n #include \"setup.h\"\n+#include \"dir.h\"\n \n static void get_system_info(struct strbuf *sys_info)\n {\n@@ -101,12 +101,13 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf buffer = STRBUF_INIT;\n \tstruct strbuf report_path = STRBUF_INIT;\n+\tstruct strbuf option_suffix = STRBUF_INIT;\n+\tstruct strbuf default_option_suffix = STRBUF_INIT;\n \tint report = -1;\n \ttime_t now = time(NULL);\n \tstruct tm tm;\n \tenum diagnose_mode diagnose = DIAGNOSE_NONE;\n \tchar *option_output = NULL;\n-\tchar *option_suffix = \"%Y-%m-%d-%H%M\";\n \tconst char *user_relative_path = NULL;\n \tchar *prefixed_filename;\n \tsize_t output_path_len;\n@@ -118,11 +119,14 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \t\t\t       PARSE_OPT_OPTARG, option_parse_diagnose),\n \t\tOPT_STRING('o', \"output-directory\", &option_output, N_(\"path\"),\n \t\t\t   N_(\"specify a destination for the bugreport file(s)\")),\n-\t\tOPT_STRING('s', \"suffix\", &option_suffix, N_(\"format\"),\n+\t\tOPT_STRING('s', \"suffix\", &option_suffix.buf, N_(\"format\"),\n \t\t\t   N_(\"specify a strftime format suffix for the filename(s)\")),\n \t\tOPT_END()\n \t};\n \n+\tstrbuf_addstr(&default_option_suffix, \"%Y-%m-%d-%H%M\");\n+\tstrbuf_addstr(&option_suffix, default_option_suffix.buf);\n+\n \targc = parse_options(argc, argv, prefix, bugreport_options,\n \t\t\t     bugreport_usage, 0);\n \n@@ -134,9 +138,20 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \toutput_path_len = report_path.len;\n \n \tstrbuf_addstr(&report_path, \"git-bugreport-\");\n-\tstrbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);\n+\tstrbuf_addftime(&report_path, option_suffix.buf, localtime_r(&now, &tm), 0, 0);\n \tstrbuf_addstr(&report_path, \".txt\");\n \n+\tif (strbuf_cmp(&option_suffix, &default_option_suffix) == 0) {\n+\t\tint i = 1;\n+\t\tint pos = report_path.len - 4;\n+\t\twhile (file_exists(report_path.buf) && i < 10) {\n+\t\t\tif (i > 1)\n+\t\t\t\tstrbuf_remove(&report_path, pos, 2);\n+\t\t\tstrbuf_insertf(&report_path, pos, \"+%d\", i);\n+\t\t\ti++;\n+\t\t}\n+\t}\n+\n \tswitch (safe_create_leading_directories(report_path.buf)) {\n \tcase SCLD_OK:\n \tcase SCLD_EXISTS:\n@@ -151,7 +166,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \t\tstruct strbuf zip_path = STRBUF_INIT;\n \t\tstrbuf_add(&zip_path, report_path.buf, output_path_len);\n \t\tstrbuf_addstr(&zip_path, \"git-diagnostics-\");\n-\t\tstrbuf_addftime(&zip_path, option_suffix, localtime_r(&now, &tm), 0, 0);\n+\t\tstrbuf_addftime(&zip_path, option_suffix.buf, localtime_r(&now, &tm), 0, 0);\n \t\tstrbuf_addstr(&zip_path, \".zip\");\n \n \t\tif (create_diagnostics_archive(&zip_path, diagnose))\n@@ -188,6 +203,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \n \tfree(prefixed_filename);\n \tstrbuf_release(&buffer);\n+\tstrbuf_release(&default_option_suffix);\n \n \tret = !!launch_editor(report_path.buf, NULL, NULL);\n \tstrbuf_release(&report_path);\n-- \n2.42.0.298.gd89efca819.dirty\n\n"},{"id":"483280","messageId":"20231015034238.100675-3-jacob@initialcommit.io","threadId":"60364","inReplyTo":"20231015034238.100675-1-jacob@initialcommit.io","subject":"[PATCH v2 2/3] bugreport: match diagnostics filename with report","fromName":"Jacob Stopak","fromEmail":"jacob@initialcommit.io","sentAt":"2023-10-15T03:42:36Z","receivedAt":"2023-10-15T03:43:25Z","isPatch":true,"sender":{"key":"jacob@initialcommit.io","avatar":"https://avatars.githubusercontent.com/u/49353917?v=4"},"body":"When using the --diagnose flag, match the diagnostics zip filename with\nthe bugreport filename in the scenario where '+i' syntax is used.\n\nSigned-off-by: Jacob Stopak <jacob@initialcommit.io>\n---\n builtin/bugreport.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/bugreport.c b/builtin/bugreport.c\nindex 71ee7d7f4b..573d270677 100644\n--- a/builtin/bugreport.c\n+++ b/builtin/bugreport.c\n@@ -112,6 +112,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \tchar *prefixed_filename;\n \tsize_t output_path_len;\n \tint ret;\n+\tint i = 1;\n \n \tconst struct option bugreport_options[] = {\n \t\tOPT_CALLBACK_F(0, \"diagnose\", &diagnose, N_(\"mode\"),\n@@ -142,7 +143,6 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \tstrbuf_addstr(&report_path, \".txt\");\n \n \tif (strbuf_cmp(&option_suffix, &default_option_suffix) == 0) {\n-\t\tint i = 1;\n \t\tint pos = report_path.len - 4;\n \t\twhile (file_exists(report_path.buf) && i < 10) {\n \t\t\tif (i > 1)\n@@ -167,6 +167,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_add(&zip_path, report_path.buf, output_path_len);\n \t\tstrbuf_addstr(&zip_path, \"git-diagnostics-\");\n \t\tstrbuf_addftime(&zip_path, option_suffix.buf, localtime_r(&now, &tm), 0, 0);\n+\t\tif (i > 1) strbuf_addf(&zip_path, \"+%d\", i-1);\n \t\tstrbuf_addstr(&zip_path, \".zip\");\n \n \t\tif (create_diagnostics_archive(&zip_path, diagnose))\n-- \n2.42.0.298.gd89efca819.dirty\n\n"},{"id":"483281","messageId":"20231015034238.100675-4-jacob@initialcommit.io","threadId":"60364","inReplyTo":"20231015034238.100675-1-jacob@initialcommit.io","subject":"[PATCH v2 3/3] bugreport: don't create --diagnose zip w/o report","fromName":"Jacob Stopak","fromEmail":"jacob@initialcommit.io","sentAt":"2023-10-15T03:42:37Z","receivedAt":"2023-10-15T03:43:28Z","isPatch":true,"sender":{"key":"jacob@initialcommit.io","avatar":"https://avatars.githubusercontent.com/u/49353917?v=4"},"body":"Prevent the diagnostics zip file from being created when the bugreport\nitself is not created due to an error.\n\nSigned-off-by: Jacob Stopak <jacob@initialcommit.io>\n---\n builtin/bugreport.c | 31 ++++++++++++++++---------------\n 1 file changed, 16 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/bugreport.c b/builtin/bugreport.c\nindex 573d270677..91567806c9 100644\n--- a/builtin/bugreport.c\n+++ b/builtin/bugreport.c\n@@ -161,21 +161,6 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \t\t    report_path.buf);\n \t}\n \n-\t/* Prepare diagnostics, if requested */\n-\tif (diagnose != DIAGNOSE_NONE) {\n-\t\tstruct strbuf zip_path = STRBUF_INIT;\n-\t\tstrbuf_add(&zip_path, report_path.buf, output_path_len);\n-\t\tstrbuf_addstr(&zip_path, \"git-diagnostics-\");\n-\t\tstrbuf_addftime(&zip_path, option_suffix.buf, localtime_r(&now, &tm), 0, 0);\n-\t\tif (i > 1) strbuf_addf(&zip_path, \"+%d\", i-1);\n-\t\tstrbuf_addstr(&zip_path, \".zip\");\n-\n-\t\tif (create_diagnostics_archive(&zip_path, diagnose))\n-\t\t\tdie_errno(_(\"unable to create diagnostics archive %s\"), zip_path.buf);\n-\n-\t\tstrbuf_release(&zip_path);\n-\t}\n-\n \t/* Prepare the report contents */\n \tget_bug_template(&buffer);\n \n@@ -202,6 +187,22 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \tfprintf(stderr, _(\"Created new report at '%s'.\\n\"),\n \t\tuser_relative_path);\n \n+\t/* Prepare diagnostics, if requested */\n+\tif (diagnose != DIAGNOSE_NONE) {\n+\t\tstruct strbuf zip_path = STRBUF_INIT;\n+\t\tstrbuf_add(&zip_path, report_path.buf, output_path_len);\n+\t\tstrbuf_addstr(&zip_path, \"git-diagnostics-\");\n+\t\tstrbuf_addftime(&zip_path, option_suffix.buf, localtime_r(&now, &tm), 0, 0);\n+\t\tif (i > 1) strbuf_addf(&zip_path, \"+%d\", i-1);\n+\t\tstrbuf_addstr(&zip_path, \".zip\");\n+\n+\t\tif (create_diagnostics_archive(&zip_path, diagnose))\n+\t\t\tdie_errno(_(\"unable to create diagnostics archive %s\"), zip_path.buf);\n+\n+\t\tstrbuf_release(&zip_path);\n+\t}\n+\n+\n \tfree(prefixed_filename);\n \tstrbuf_release(&buffer);\n \tstrbuf_release(&default_option_suffix);\n-- \n2.42.0.298.gd89efca819.dirty\n\n"},{"id":"483289","messageId":"xmqqbkczstld.fsf@gitster.g","threadId":"60364","inReplyTo":"ZStWB1/LX7cTbVGr.jacob@initialcommit.io","subject":"Re: [PATCH] bugreport: add 'seconds' to default outfile name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-15T17:06:54Z","receivedAt":"2023-10-15T17:06:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Stopak <jacob@initialcommit.io> writes:\n\n> Lol! By saying \"calendar\" I mean \"falling on the official boundaries\n> of\", like 11:15:00 - 11:16:00. Unlike the time between 11:15:30 -\n> 11:16:30 which is also a minute, but it's not a \"calendar\" minute\n> because it overlaps into the next minute. I guess in this case it's more\n> of a \"clock\" minute than a \"calendar\" minute though ':D... I guess\n> \"calendar\" terminology is used more for months/years...\n\nPeople use \"calendar\" days usually in order to to differenciate it\nwith \"business\" days.  It may take your package to come cross\ncountry and take 5 business days, but if you ship it out on Friday,\nit will not arrive by Wednesday.\n"},{"id":"483291","messageId":"xmqqr0lvpz30.fsf@gitster.g","threadId":"60364","inReplyTo":"20231015034238.100675-2-jacob@initialcommit.io","subject":"Re: [PATCH v2 1/3] bugreport: include +i in outfile suffix as needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-15T17:36:35Z","receivedAt":"2023-10-15T17:36:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Stopak <jacob@initialcommit.io> writes:\n\n> When the -s flag is absent, git bugreport includes the current hour and\n> minute values in the default bugreport filename (and diagnostics zip\n> filename if --diagnose is supplied).\n>\n> If a user runs the bugreport command more than once within a calendar\n> minute, a filename conflict with an existing file occurs and the program\n> errors, since the new output filename was already used for the previous\n> file. If the user waits anywhere from 1 to 60 seconds (depending on\n> _when during the calendar minute_ the first command was run) the command\n\nDrop \"calendar\" here (see below).  \"If the user waits from running\nthe command within the same minute\" may be easier to understand than\n\"from 1 to 60 seconds\"; after all, the user does not have to wait\nfor more than 0.5 seconds if the previous attempt was within 0.5\nseconds from the minute boundary.\n\n> works again with no error since the default filename is now unique, and\n> multiple bug reports are able to be created with default settings.\n>\n> This is a minor thing but can cause confusion especially for first time\n> users of the bugreport command, who are likely to run it multiple times\n> in quick succession to learn how it works, (like I did).\n\nPerhaps we should refine the error message we give in this case and\nwe are done, then?\n\n$ GIT_EDITOR=: git bugreport ; GIT_EDITOR=: git bugreport\nCreated new report at 'git-bugreport-2023-10-15-1008.txt'.\nfatal: unable to create 'git-bugreport-2023-10-15-1008.txt': File exists\n\nThe second message can be a bit more friendly and suggest removing\nthe stale file.\n\n> Add a '+i' into the bugreport filename suffix to make the filename\n> unique, where 'i' is an integer starting at 1 and able to grow up to 9\n> in the unlikely event a user runs the command 9 times in a single\n> minute. This leads to default output filenames like:\n\nWhat downside do you see in using 2023-10-15-1008+10 after you tried\n9 of them?  The code to limit the upper bound smells like a wasted\neffort that helps nobody in practice because it is \"unlikely\".\n\nAnd limiting the upper bound also means you now have to have extra\ncode to deal with \"we ran out---error out and help the user how to\nrecover\" anyway.\n\nNotice that you said \"in a single minute\" without \"calendar\" and the\nsentence is perfectly understandable? (see above and below).\n\n> This means the user will end up with multiple bugreport files being\n> created if they run the command multiple times quickly, but that feels\n> more intuitive and consistent than an error arbitrarily occuring within\n> a calendar minute, especially given that the time window in which the\n> error currently occurs is variable as described above.\n\nAnd drop \"calendar\" here, too (see above).\n\"Within the same minute\" is fine.\n\n> @@ -3,7 +3,6 @@\n>  #include \"editor.h\"\n>  #include \"gettext.h\"\n>  #include \"parse-options.h\"\n> -#include \"strbuf.h\"\n\nLooks like an unrelated change here.  The updated code does use\nstrbuf service, so even if other header files happen to include\nit, keep including it here is a good discipline for readability.\n\n> @@ -101,12 +101,13 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n>  {\n>  \tstruct strbuf buffer = STRBUF_INIT;\n>  \tstruct strbuf report_path = STRBUF_INIT;\n> +\tstruct strbuf option_suffix = STRBUF_INIT;\n> +\tstruct strbuf default_option_suffix = STRBUF_INIT;\n>  \tint report = -1;\n>  \ttime_t now = time(NULL);\n>  \tstruct tm tm;\n>  \tenum diagnose_mode diagnose = DIAGNOSE_NONE;\n>  \tchar *option_output = NULL;\n> -\tchar *option_suffix = \"%Y-%m-%d-%H%M\";\n>  \tconst char *user_relative_path = NULL;\n>  \tchar *prefixed_filename;\n>  \tsize_t output_path_len;\n> @@ -118,11 +119,14 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n>  \t\t\t       PARSE_OPT_OPTARG, option_parse_diagnose),\n>  \t\tOPT_STRING('o', \"output-directory\", &option_output, N_(\"path\"),\n>  \t\t\t   N_(\"specify a destination for the bugreport file(s)\")),\n> -\t\tOPT_STRING('s', \"suffix\", &option_suffix, N_(\"format\"),\n> +\t\tOPT_STRING('s', \"suffix\", &option_suffix.buf, N_(\"format\"),\n>  \t\t\t   N_(\"specify a strftime format suffix for the filename(s)\")),\n>  \t\tOPT_END()\n>  \t};\n>  \n> +\tstrbuf_addstr(&default_option_suffix, \"%Y-%m-%d-%H%M\");\n> +\tstrbuf_addstr(&option_suffix, default_option_suffix.buf);\n\nIt usually pays for a reviewer when two variables, one called\ndefault and the other not, gets initialized to the same value,\nbecause it is a sign that there is something fishy going on.  A more\nnormal pattern is to set up the default, do whatever is needed and\nif the non-default one has not been touched, then copy that default\nvalue to the real one, and the code needed deviation from it for\nwhatever reason.  Let's read on.\n\n> @@ -134,9 +138,20 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n>  \toutput_path_len = report_path.len;\n>  \n>  \tstrbuf_addstr(&report_path, \"git-bugreport-\");\n> -\tstrbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);\n> +\tstrbuf_addftime(&report_path, option_suffix.buf, localtime_r(&now, &tm), 0, 0);\n>  \tstrbuf_addstr(&report_path, \".txt\");\n\nOK.\n\n> +\tif (strbuf_cmp(&option_suffix, &default_option_suffix) == 0) {\n\nStyle: never compare with 0 or NULL inside a conditional.\n\n\tif (!compare(...)) {\n\nis the idiom to use.\n\nYou may have written it this way to avoid appending after what the\nuser gave you as a custom pattern, but this if() statement is a\nfailed way to do so.  The user can give what happens to be the same\nas the hardcoded default pattern and you cannot tell if it came from\nthem or your initially hardcoded one by string comparison.\n\n> +\t\tint i = 1;\n> +\t\tint pos = report_path.len - 4;\n\nTotally unclear where the magic \"4\" comes from.\n\n> +\t\twhile (file_exists(report_path.buf) && i < 10) {\n> +\t\t\tif (i > 1)\n> +\t\t\t\tstrbuf_remove(&report_path, pos, 2);\n> +\t\t\tstrbuf_insertf(&report_path, pos, \"+%d\", i);\n> +\t\t\ti++;\n> +\t\t}\n> +\t}\n\nWe see TOCTOU here.\n\nDo it more like this instead.\n\n    * Discard default_option_suffix variable.\n\n    * Introduce a boolean option_suffix_is_from_user and\n      initialize it to false.\n\n    * Initialize option_suffix to an empty string.\n\n    * After parse_options() returns, if option_suffix is still\n      empty, then add the default pattern.  Otherwise toggle\n      option_suffix_is_from_user true.\n\n    * Prepare the code so that you can recompute report_path as you\n      need to increment the suffix added to option_suffix.\n\nThen where we do xopen() on report_path.buf, have a fallback loop,\nand you can do something like\n\n    again:\n\treport = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);\n\tif (report < 0 && errno == EEXISTS && !option_suffix_is_from_user) {\n\t\tincrement_suffix(&report_path);\n\t\tgoto again;\n\t} else if (report < 0) {\n\t\tdie_errno(_(\"unable to open '%s'\", report_path.buf));\n\t}\n\nto avoid TOCTOU.\n\nHTH.\n"},{"id":"483319","messageId":"20231016214045.146862-1-jacob@initialcommit.io","threadId":"60364","inReplyTo":"20231015034238.100675-2-jacob@initialcommit.io","subject":"[PATCH v3 0/1] bugreport: include +i in outfile suffix as needed","fromName":"Jacob Stopak","fromEmail":"jacob@initialcommit.io","sentAt":"2023-10-16T21:40:44Z","receivedAt":"2023-10-16T21:41:08Z","isPatch":true,"sender":{"key":"jacob@initialcommit.io","avatar":"https://avatars.githubusercontent.com/u/49353917?v=4"},"body":"Similar functionality to the previous patch version but with the\nfollowing changes:\n\n  * Remove the default_option_suffix variable and clean up the way the\n    option_suffix value is set.\n\n  * Refactor code for building report path and diagnostics zip file path\n    into the new function build_path(...), which builds a fresh path\n    from scratch each time it's called. Although it does take quite a few\n    arguments, it reduces duplicated code and simplifies the code by\n    removing the need for splicing or inserting the incrementable suffix.\n\n  * Allow the increment value 'i' to grow as needed instead of stopping\n    at 9.\n\n  * Replace xopen() with open() so that the program doesn't die with an\n    error if the file already exists.\n\n  * Replace the while loop with a fallback loop to prevent TOCTOU.\n\n  * Clean up commit message verbiage.\n\nSome additional comments:\n\n> Perhaps we should refine the error message we give in this case and\n> we are done, then?\n\nI had thought about this but due to the variable timed behavior I had\ntrouble coming up with a message that would convey clearly what's going\non and what the user should do. Here were some things that popped into\nmy head but they all sounded a bit silly to me:\n\n\"A bugreport with this name already exists, try again shortly\"\nor\n\"File exists: wait 1 minute and try again or use a custom suffix with -s\"\nor\n\"File exists: try again in 1 to 60 seconds :-P\"\n\nI think the reason they sound silly to me is that the user is only being\nmade aware of this info because the program happens to be operating this\nway - instead of the program working in a smoother way where this type of\noperational info wouldn't need to be conveyed.\n\n> What downside do you see in using 2023-10-15-1008+10 after you tried\n> 9 of them?  The code to limit the upper bound smells like a wasted\n> effort that helps nobody in practice because it is \"unlikely\".\n\n> And limiting the upper bound also means you now have to have extra\n> code to deal with \"we ran out---error out and help the user how to\n> recover\" anyway.\n\nI completely agree with this and made these updates.\n\n> Notice that you said \"in a single minute\" without \"calendar\" and the\n> sentence is perfectly understandable?\n\nI googled \"calendar minute\" and the only thing that came up was some\nJava documentation... So I guess I just made it up... :'D\n\nJacob Stopak (1):\n  bugreport: include +i in outfile suffix as needed\n\n builtin/bugreport.c | 83 +++++++++++++++++++++++++++++++--------------\n 1 file changed, 57 insertions(+), 26 deletions(-)\n\n\nbase-commit: 493f4622739e9b64f24b465b21aa85870dd9dc09\n-- \n2.42.0.297.g36452639b8\n\n"},{"id":"483320","messageId":"20231016214045.146862-2-jacob@initialcommit.io","threadId":"60364","inReplyTo":"20231016214045.146862-1-jacob@initialcommit.io","subject":"[PATCH v3 1/1] bugreport: include +i in outfile suffix as needed","fromName":"Jacob Stopak","fromEmail":"jacob@initialcommit.io","sentAt":"2023-10-16T21:40:45Z","receivedAt":"2023-10-16T21:41:08Z","isPatch":true,"sender":{"key":"jacob@initialcommit.io","avatar":"https://avatars.githubusercontent.com/u/49353917?v=4"},"body":"When the -s flag is absent, git bugreport includes the current hour and\nminute values in the default bugreport filename (and diagnostics zip\nfilename if --diagnose is supplied).\n\nIf a user runs the bugreport command more than once within a minute, a\nfilename conflict with an existing file occurs and the program errors,\nsince the new output filename was already used for the previous file. If\nthe user waits anywhere from 1 to 60 seconds (depending on when during\nthe minute the first command was run) the command works again with no\nerror since the default filename is now unique, and multiple bug reports\nare able to be created with default settings.\n\nThis is a minor thing but can cause confusion for first time users of\nthe bugreport command, who are likely to run it multiple times in quick\nsuccession to learn how it works, (like I did). Or users who quickly\nfill in a few details before closing and creating a new one.\n\nAdd a '+i' into the bugreport filename suffix where 'i' is an integer\nstarting at 1 and growing as needed until a unique filename is obtained.\n\nThis leads to default output filenames like:\n\ngit-bugreport-%Y-%m-%d-%H%M+1.txt\ngit-bugreport-%Y-%m-%d-%H%M+2.txt\n...\ngit-bugreport-%Y-%m-%d-%H%M+i.txt\n\nThis means the user will end up with multiple bugreport files being\ncreated if they run the command multiple times quickly, but that feels\nmore intuitive and consistent than an error arbitrarily occuring within\na minute, especially given that the time window in which the error\ncurrently occurs is variable as described above.\n\nIf --diagnose is supplied, match the incremented suffix of the\ndiagnostics zip file to the bugreport.\n\nSigned-off-by: Jacob Stopak <jacob@initialcommit.io>\n---\n builtin/bugreport.c | 83 +++++++++++++++++++++++++++++++--------------\n 1 file changed, 57 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/bugreport.c b/builtin/bugreport.c\nindex d2ae5c305d..ed65735873 100644\n--- a/builtin/bugreport.c\n+++ b/builtin/bugreport.c\n@@ -11,6 +11,7 @@\n #include \"diagnose.h\"\n #include \"object-file.h\"\n #include \"setup.h\"\n+#include \"dir.h\"\n \n static void get_system_info(struct strbuf *sys_info)\n {\n@@ -97,20 +98,41 @@ static void get_header(struct strbuf *buf, const char *title)\n \tstrbuf_addf(buf, \"\\n\\n[%s]\\n\", title);\n }\n \n+static void build_path(struct strbuf *buf, const char *dir_path,\n+\t\t       const char *prefix, const char *suffix,\n+\t\t       time_t t, int *i, const char *ext)\n+{\n+\tstruct tm tm;\n+\n+\tstrbuf_reset(buf);\n+\tstrbuf_addstr(buf, dir_path);\n+\tstrbuf_complete(buf, '/');\n+\n+\tstrbuf_addstr(buf, prefix);\n+\tstrbuf_addftime(buf, suffix, localtime_r(&t, &tm), 0, 0);\n+\n+\tif (*i > 0)\n+\t\tstrbuf_addf(buf, \"+%d\", *i);\n+\n+\tstrbuf_addstr(buf, ext);\n+\n+\t(*i)++;\n+}\n+\n int cmd_bugreport(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf buffer = STRBUF_INIT;\n \tstruct strbuf report_path = STRBUF_INIT;\n \tint report = -1;\n \ttime_t now = time(NULL);\n-\tstruct tm tm;\n \tenum diagnose_mode diagnose = DIAGNOSE_NONE;\n \tchar *option_output = NULL;\n-\tchar *option_suffix = \"%Y-%m-%d-%H%M\";\n+\tchar *option_suffix = \"\";\n+\tint option_suffix_is_from_user = 0;\n \tconst char *user_relative_path = NULL;\n \tchar *prefixed_filename;\n-\tsize_t output_path_len;\n \tint ret;\n+\tint i = 0;\n \n \tconst struct option bugreport_options[] = {\n \t\tOPT_CALLBACK_F(0, \"diagnose\", &diagnose, N_(\"mode\"),\n@@ -126,16 +148,16 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, bugreport_options,\n \t\t\t     bugreport_usage, 0);\n \n+\tif (!strlen(option_suffix))\n+\t\toption_suffix = \"%Y-%m-%d-%H%M\";\n+\telse\n+\t\toption_suffix_is_from_user = 1;\n+\n \t/* Prepare the path to put the result */\n \tprefixed_filename = prefix_filename(prefix,\n \t\t\t\t\t    option_output ? option_output : \"\");\n-\tstrbuf_addstr(&report_path, prefixed_filename);\n-\tstrbuf_complete(&report_path, '/');\n-\toutput_path_len = report_path.len;\n-\n-\tstrbuf_addstr(&report_path, \"git-bugreport-\");\n-\tstrbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);\n-\tstrbuf_addstr(&report_path, \".txt\");\n+\tbuild_path(&report_path, prefixed_filename, \"git-bugreport-\",\n+\t\t   option_suffix, now, &i, \".txt\");\n \n \tswitch (safe_create_leading_directories(report_path.buf)) {\n \tcase SCLD_OK:\n@@ -146,20 +168,6 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \t\t    report_path.buf);\n \t}\n \n-\t/* Prepare diagnostics, if requested */\n-\tif (diagnose != DIAGNOSE_NONE) {\n-\t\tstruct strbuf zip_path = STRBUF_INIT;\n-\t\tstrbuf_add(&zip_path, report_path.buf, output_path_len);\n-\t\tstrbuf_addstr(&zip_path, \"git-diagnostics-\");\n-\t\tstrbuf_addftime(&zip_path, option_suffix, localtime_r(&now, &tm), 0, 0);\n-\t\tstrbuf_addstr(&zip_path, \".zip\");\n-\n-\t\tif (create_diagnostics_archive(&zip_path, diagnose))\n-\t\t\tdie_errno(_(\"unable to create diagnostics archive %s\"), zip_path.buf);\n-\n-\t\tstrbuf_release(&zip_path);\n-\t}\n-\n \t/* Prepare the report contents */\n \tget_bug_template(&buffer);\n \n@@ -169,14 +177,37 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \tget_header(&buffer, _(\"Enabled Hooks\"));\n \tget_populated_hooks(&buffer, !startup_info->have_repository);\n \n-\t/* fopen doesn't offer us an O_EXCL alternative, except with glibc. */\n-\treport = xopen(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);\n+\tagain:\n+\t\t/* fopen doesn't offer us an O_EXCL alternative, except with glibc. */\n+\t\treport = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);\n+\t\tif (report < 0 && errno == EEXIST && !option_suffix_is_from_user) {\n+\t\t\tbuild_path(&report_path, prefixed_filename,\n+\t\t\t\t   \"git-bugreport-\", option_suffix, now, &i,\n+\t\t\t\t   \".txt\");\n+\t\t\tgoto again;\n+\t\t} else if (report < 0) {\n+\t\t\tdie_errno(_(\"unable to open '%s'\"), report_path.buf);\n+\t\t}\n \n \tif (write_in_full(report, buffer.buf, buffer.len) < 0)\n \t\tdie_errno(_(\"unable to write to %s\"), report_path.buf);\n \n \tclose(report);\n \n+\t/* Prepare diagnostics, if requested */\n+\tif (diagnose != DIAGNOSE_NONE) {\n+\t\tstruct strbuf zip_path = STRBUF_INIT;\n+\t\ti--; /* Undo last increment to match zipfile suffix to bugreport */\n+\t\tbuild_path(&zip_path, prefixed_filename, \"git-diagnostics-\",\n+\t\t\t   option_suffix, now, &i, \".zip\");\n+\n+\t\tif (create_diagnostics_archive(&zip_path, diagnose))\n+\t\t\tdie_errno(_(\"unable to create diagnostics archive %s\"),\n+\t\t\t\t  zip_path.buf);\n+\n+\t\tstrbuf_release(&zip_path);\n+\t}\n+\n \t/*\n \t * We want to print the path relative to the user, but we still need the\n \t * path relative to us to give to the editor.\n-- \n2.42.0.297.g36452639b8\n\n"},{"id":"483323","messageId":"xmqq4jiqkwi1.fsf@gitster.g","threadId":"60364","inReplyTo":"20231016214045.146862-2-jacob@initialcommit.io","subject":"Re: [PATCH v3 1/1] bugreport: include +i in outfile suffix as needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-16T22:55:50Z","receivedAt":"2023-10-16T22:55:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Stopak <jacob@initialcommit.io> writes:\n\n>  builtin/bugreport.c | 83 +++++++++++++++++++++++++++++++--------------\n>  1 file changed, 57 insertions(+), 26 deletions(-)\n\nLooking good.  It is not easy to do an automated and reliable test\nfor this one for obvious reasons ;-), so let's queue it as-is.\n\nThanks.\n\n> -\t/* fopen doesn't offer us an O_EXCL alternative, except with glibc. */\n> -\treport = xopen(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);\n> +\tagain:\n> +\t\t/* fopen doesn't offer us an O_EXCL alternative, except with glibc. */\n> +\t\treport = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);\n> +\t\tif (report < 0 && errno == EEXIST && !option_suffix_is_from_user) {\n> +\t\t\tbuild_path(&report_path, prefixed_filename,\n> +\t\t\t\t   \"git-bugreport-\", option_suffix, now, &i,\n> +\t\t\t\t   \".txt\");\n> +\t\t\tgoto again;\n> +\t\t} else if (report < 0) {\n> +\t\t\tdie_errno(_(\"unable to open '%s'\"), report_path.buf);\n> +\t\t}\n\nI didn't expect a rewrite to add an extra level of indentation like\nthis, though ;-).\n"},{"id":"483327","messageId":"ZS38ykTuqHXYnU57.jacob@initialcommit.io","threadId":"60364","inReplyTo":"xmqq4jiqkwi1.fsf@gitster.g","subject":"Re: [PATCH v3 1/1] bugreport: include +i in outfile suffix as needed","fromName":"Jacob Stopak","fromEmail":"jacob@initialcommit.io","sentAt":"2023-10-17T03:17:30Z","receivedAt":"2023-10-17T03:17:36Z","isPatch":true,"sender":{"key":"jacob@initialcommit.io","avatar":"https://avatars.githubusercontent.com/u/49353917?v=4"},"body":"On Mon, Oct 16, 2023 at 03:55:50PM -0700, Junio C Hamano wrote:\n> Jacob Stopak <jacob@initialcommit.io> writes:\n> \n> >  builtin/bugreport.c | 83 +++++++++++++++++++++++++++++++--------------\n> >  1 file changed, 57 insertions(+), 26 deletions(-)\n> \n> Looking good.  It is not easy to do an automated and reliable test\n> for this one for obvious reasons ;-), so let's queue it as-is.\n> \n> Thanks.\n> \n\nAwesome! Thank you!\n\n> > -\t/* fopen doesn't offer us an O_EXCL alternative, except with glibc. */\n> > -\treport = xopen(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);\n> > +\tagain:\n> > +\t\t/* fopen doesn't offer us an O_EXCL alternative, except with glibc. */\n> > +\t\treport = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);\n> > +\t\tif (report < 0 && errno == EEXIST && !option_suffix_is_from_user) {\n> > +\t\t\tbuild_path(&report_path, prefixed_filename,\n> > +\t\t\t\t   \"git-bugreport-\", option_suffix, now, &i,\n> > +\t\t\t\t   \".txt\");\n> > +\t\t\tgoto again;\n> > +\t\t} else if (report < 0) {\n> > +\t\t\tdie_errno(_(\"unable to open '%s'\"), report_path.buf);\n> > +\t\t}\n> \n> I didn't expect a rewrite to add an extra level of indentation like\n> this, though ;-).\n\nWhoops... I looked in another file to check the indentation around a\ngoto label and misread how it should be. Let me know if I should submit\nv4 with that corrected.\n"},{"id":"483613","messageId":"xmqqo7gsolka.fsf@gitster.g","threadId":"60364","inReplyTo":"20231016214045.146862-2-jacob@initialcommit.io","subject":"Re: [PATCH v3 1/1] bugreport: include +i in outfile suffix as needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-21T00:39:49Z","receivedAt":"2023-10-21T00:40:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Stopak <jacob@initialcommit.io> writes:\n\n>  int cmd_bugreport(int argc, const char **argv, const char *prefix)\n>  {\n>  \tstruct strbuf buffer = STRBUF_INIT;\n>  \tstruct strbuf report_path = STRBUF_INIT;\n>  \tint report = -1;\n>  \ttime_t now = time(NULL);\n> -\tstruct tm tm;\n>  \tenum diagnose_mode diagnose = DIAGNOSE_NONE;\n>  \tchar *option_output = NULL;\n> -\tchar *option_suffix = \"%Y-%m-%d-%H%M\";\n> +\tchar *option_suffix = \"\";\n> +\tint option_suffix_is_from_user = 0;\n>  \tconst char *user_relative_path = NULL;\n>  \tchar *prefixed_filename;\n> -\tsize_t output_path_len;\n>  \tint ret;\n> +\tint i = 0;\n\nOK, I think between me and you, we stared at this piece of code long\nenough to make ourselves numb.  The original \"at most one report per\na minute\" default came from the very original in 238b439d\n(bugreport: add tool to generate debugging info, 2020-04-16) and\nthat is what we are changing, so let me summon its author as an area\nexpert for a pair of fresh eyes to see if they can offer any new\ninsights.\n\nThanks.\n\nhttps://lore.kernel.org/git/20231016214045.146862-1-jacob@initialcommit.io/\n"},{"id":"483932","messageId":"ZTrX4PMYtbVT-tUu@google.com","threadId":"60364","inReplyTo":"20231016214045.146862-2-jacob@initialcommit.io","subject":"Re: [PATCH v3 1/1] bugreport: include +i in outfile suffix as needed","fromName":"Emily Shaffer","fromEmail":"nasamuffin@google.com","sentAt":"2023-10-26T21:19:28Z","receivedAt":"2023-10-26T21:19:37Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Thanks Jack for drawing my attention to the summoning, and sorry for\ndelay.\n\nOn Mon, Oct 16, 2023 at 02:40:45PM -0700, Jacob Stopak wrote:\n> \n> When the -s flag is absent, git bugreport includes the current hour and\n> minute values in the default bugreport filename (and diagnostics zip\n> filename if --diagnose is supplied).\n> \n> If a user runs the bugreport command more than once within a minute, a\n> filename conflict with an existing file occurs and the program errors,\n> since the new output filename was already used for the previous file. If\n> the user waits anywhere from 1 to 60 seconds (depending on when during\n> the minute the first command was run) the command works again with no\n> error since the default filename is now unique, and multiple bug reports\n> are able to be created with default settings.\n> \n> This is a minor thing but can cause confusion for first time users of\n> the bugreport command, who are likely to run it multiple times in quick\n> succession to learn how it works, (like I did). Or users who quickly\n> fill in a few details before closing and creating a new one.\n\nSure, agreed - I guess I got in the habit of testing by running `rm\ngit-bugreport*.txt && git bugreport --foo` and never thought about this\nbeing annoying for an actual reporter :)\n\n> \n> Add a '+i' into the bugreport filename suffix where 'i' is an integer\n> starting at 1 and growing as needed until a unique filename is obtained.\n> \n> This leads to default output filenames like:\n> \n> git-bugreport-%Y-%m-%d-%H%M+1.txt\n> git-bugreport-%Y-%m-%d-%H%M+2.txt\n> ...\n> git-bugreport-%Y-%m-%d-%H%M+i.txt\n> \n> This means the user will end up with multiple bugreport files being\n> created if they run the command multiple times quickly, but that feels\n> more intuitive and consistent than an error arbitrarily occuring within\n> a minute, especially given that the time window in which the error\n> currently occurs is variable as described above.\n\nSure, and with the monotonic increases it's quite easy to locate the\nbugreport that's the final one the user generated. This scheme seems\nfine to me.\n\n> \n> If --diagnose is supplied, match the incremented suffix of the\n> diagnostics zip file to the bugreport.\n\nNice.\n\n> \n> Signed-off-by: Jacob Stopak <jacob@initialcommit.io>\n> ---\n>  builtin/bugreport.c | 83 +++++++++++++++++++++++++++++++--------------\n>  1 file changed, 57 insertions(+), 26 deletions(-)\n> \n> diff --git a/builtin/bugreport.c b/builtin/bugreport.c\n> index d2ae5c305d..ed65735873 100644\n> --- a/builtin/bugreport.c\n> +++ b/builtin/bugreport.c\n> @@ -11,6 +11,7 @@\n>  #include \"diagnose.h\"\n>  #include \"object-file.h\"\n>  #include \"setup.h\"\n> +#include \"dir.h\"\n>  \n>  static void get_system_info(struct strbuf *sys_info)\n>  {\n> @@ -97,20 +98,41 @@ static void get_header(struct strbuf *buf, const char *title)\n>  \tstrbuf_addf(buf, \"\\n\\n[%s]\\n\", title);\n>  }\n>  \n> +static void build_path(struct strbuf *buf, const char *dir_path,\n> +\t\t       const char *prefix, const char *suffix,\n> +\t\t       time_t t, int *i, const char *ext)\n> +{\n> +\tstruct tm tm;\n> +\n> +\tstrbuf_reset(buf);\n> +\tstrbuf_addstr(buf, dir_path);\n> +\tstrbuf_complete(buf, '/');\n> +\n> +\tstrbuf_addstr(buf, prefix);\n> +\tstrbuf_addftime(buf, suffix, localtime_r(&t, &tm), 0, 0);\n> +\n> +\tif (*i > 0)\n> +\t\tstrbuf_addf(buf, \"+%d\", *i);\n> +\n> +\tstrbuf_addstr(buf, ext);\n> +\n> +\t(*i)++;\n> +}\n\nI commented on the weirdness of having to decrement i for --diagnose\nbelow, but I think I generally just wish that instead of build_path()\nthis function did create_file_with_optional_suffix() and returned the\nfinal modified option_suffix(). Better still would be if this function\ncreated (or at least tested) all the necessary output paths so you don't\nend up succeeding in creating a bugreport.txt but failing in creating\nthe diagnostics.zip in some edge case, something like....\n\nbuild_suffix(..., &option_suffix) {\n  ... build timestamp ...\n  while (...)\n    for (final_path in eventual_paths) {\n    \terr = select(final_path);\n\tif (err)\n\t  final_path = strcat(most_of_path, i)\n\telse\n\t  break\n(Yeah, that's very handwavey, but I hope you see what I'm getting at.)\n\nReally, though, I mostly don't think I like leaving the control variable i raw\nto the calling scope and making it be manipulated later. Fancy\npre-guessing-path-availability aside, I think you could achieve a more\npleasant solution even by letting build_path() become\ncreate_file_with_optional_suffix() (that reports the optional suffix\neventually settled on).\n\n> +\n>  int cmd_bugreport(int argc, const char **argv, const char *prefix)\n>  {\n>  \tstruct strbuf buffer = STRBUF_INIT;\n>  \tstruct strbuf report_path = STRBUF_INIT;\n>  \tint report = -1;\n>  \ttime_t now = time(NULL);\n> -\tstruct tm tm;\n>  \tenum diagnose_mode diagnose = DIAGNOSE_NONE;\n>  \tchar *option_output = NULL;\n> -\tchar *option_suffix = \"%Y-%m-%d-%H%M\";\n> +\tchar *option_suffix = \"\";\n> +\tint option_suffix_is_from_user = 0;\n>  \tconst char *user_relative_path = NULL;\n>  \tchar *prefixed_filename;\n> -\tsize_t output_path_len;\n>  \tint ret;\n> +\tint i = 0;\n>  \n>  \tconst struct option bugreport_options[] = {\n>  \t\tOPT_CALLBACK_F(0, \"diagnose\", &diagnose, N_(\"mode\"),\n> @@ -126,16 +148,16 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n>  \targc = parse_options(argc, argv, prefix, bugreport_options,\n>  \t\t\t     bugreport_usage, 0);\n>  \n> +\tif (!strlen(option_suffix))\n> +\t\toption_suffix = \"%Y-%m-%d-%H%M\";\n> +\telse\n> +\t\toption_suffix_is_from_user = 1;\n\nLooking at where this is used, it looks like you're saying \"if the user\nspecified the suffix manually and has this problem, then that sucks for\nthem, they put their own foot in it\". But I don't know if I necessarily\nfollow that logic - I'd just as soon drop the exception, append an int\nto the user-provided suffix, and document that we'll do that in the\nmanpage.\n\n(This isn't something I feel strongly about, except that I think it\nmakes the code harder to follow for not very notable user benefit. I\nalso didn't look through the reviews up until now, so if this was\nalready hashed back and forth, just go ahead and ignore me.)\n\n> +\n>  \t/* Prepare the path to put the result */\n>  \tprefixed_filename = prefix_filename(prefix,\n>  \t\t\t\t\t    option_output ? option_output : \"\");\n> -\tstrbuf_addstr(&report_path, prefixed_filename);\n> -\tstrbuf_complete(&report_path, '/');\n> -\toutput_path_len = report_path.len;\n> -\n> -\tstrbuf_addstr(&report_path, \"git-bugreport-\");\n> -\tstrbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);\n> -\tstrbuf_addstr(&report_path, \".txt\");\n> +\tbuild_path(&report_path, prefixed_filename, \"git-bugreport-\",\n> +\t\t   option_suffix, now, &i, \".txt\");\n>  \n>  \tswitch (safe_create_leading_directories(report_path.buf)) {\n>  \tcase SCLD_OK:\n> @@ -146,20 +168,6 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n>  \t\t    report_path.buf);\n>  \t}\n>  \n> -\t/* Prepare diagnostics, if requested */\n> -\tif (diagnose != DIAGNOSE_NONE) {\n> -\t\tstruct strbuf zip_path = STRBUF_INIT;\n> -\t\tstrbuf_add(&zip_path, report_path.buf, output_path_len);\n> -\t\tstrbuf_addstr(&zip_path, \"git-diagnostics-\");\n> -\t\tstrbuf_addftime(&zip_path, option_suffix, localtime_r(&now, &tm), 0, 0);\n> -\t\tstrbuf_addstr(&zip_path, \".zip\");\n> -\n> -\t\tif (create_diagnostics_archive(&zip_path, diagnose))\n> -\t\t\tdie_errno(_(\"unable to create diagnostics archive %s\"), zip_path.buf);\n> -\n> -\t\tstrbuf_release(&zip_path);\n> -\t}\n> -\n>  \t/* Prepare the report contents */\n>  \tget_bug_template(&buffer);\n>  \n> @@ -169,14 +177,37 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n>  \tget_header(&buffer, _(\"Enabled Hooks\"));\n>  \tget_populated_hooks(&buffer, !startup_info->have_repository);\n>  \n> -\t/* fopen doesn't offer us an O_EXCL alternative, except with glibc. */\n> -\treport = xopen(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);\n> +\tagain:\n> +\t\t/* fopen doesn't offer us an O_EXCL alternative, except with glibc. */\n> +\t\treport = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);\n> +\t\tif (report < 0 && errno == EEXIST && !option_suffix_is_from_user) {\n> +\t\t\tbuild_path(&report_path, prefixed_filename,\n> +\t\t\t\t   \"git-bugreport-\", option_suffix, now, &i,\n> +\t\t\t\t   \".txt\");\n> +\t\t\tgoto again;\n> +\t\t} else if (report < 0) {\nNit, but the double-checking of (report < 0) bothers me a little. Is it\nnicer if it's nested?\n\n\tif (report < 0) {\n\t\tif (errno == EEXIST) {\n\t\t\tbuild_path(...);\n\t\t\tgoto again;\n\t\t}\n\n\t\tdie_errno(_(...));\n\t}\n\nI like it a little more, but that's up to taste, I suppose.\n> +\t\t\tdie_errno(_(\"unable to open '%s'\"), report_path.buf);\n> +\t\t}\n>  \n>  \tif (write_in_full(report, buffer.buf, buffer.len) < 0)\n>  \t\tdie_errno(_(\"unable to write to %s\"), report_path.buf);\n>  \n>  \tclose(report);\n>  \n> +\t/* Prepare diagnostics, if requested */\n> +\tif (diagnose != DIAGNOSE_NONE) {\n> +\t\tstruct strbuf zip_path = STRBUF_INIT;\n> +\t\ti--; /* Undo last increment to match zipfile suffix to bugreport */\nI understand why you're doing this, but I'd rather see it decremented\n(or more care taken in the increment logic elsewhere) closer to where it\nis being increment-and-checked. If someone wants to add another\nassociated file besides the report and the diagnostics, then the logic\nfor the decrement becomes complicated (what happens if I run `git\nbugreport --diagnostics --desktop_screencap`? what if I run only `git\nbugreport --desktop_screencap`?). Even without that potential pain,\nreading this comment here means I have to say \"oh wait, what did I read\nabove? hold on, let me page it back in\".\n\n> +\t\tbuild_path(&zip_path, prefixed_filename, \"git-diagnostics-\",\n> +\t\t\t   option_suffix, now, &i, \".zip\");\n> +\n> +\t\tif (create_diagnostics_archive(&zip_path, diagnose))\n> +\t\t\tdie_errno(_(\"unable to create diagnostics archive %s\"),\n> +\t\t\t\t  zip_path.buf);\n> +\n> +\t\tstrbuf_release(&zip_path);\n> +\t}\n> +\n>  \t/*\n>  \t * We want to print the path relative to the user, but we still need the\n>  \t * path relative to us to give to the editor.\n> -- \n> 2.42.0.297.g36452639b8\n\nLast thing: it probably makes sense to mention this new behavior in the\nmanpage, especially if you'll apply that behavior to user-provided\nsuffixes too.\n\nThanks for your effort on the patch so far and again, sorry for the late\nreply.\n - Emily\n"},{"id":"483951","messageId":"ZTtZ5CbIGETy1ucV.jacob@initialcommit.io","threadId":"60364","inReplyTo":"ZTrX4PMYtbVT-tUu@google.com","subject":"Re: [PATCH v3 1/1] bugreport: include +i in outfile suffix as needed","fromName":"Jacob Stopak","fromEmail":"jacob@initialcommit.io","sentAt":"2023-10-27T06:34:12Z","receivedAt":"2023-10-27T06:34:18Z","isPatch":true,"sender":{"key":"jacob@initialcommit.io","avatar":"https://avatars.githubusercontent.com/u/49353917?v=4"},"body":"> > +static void build_path(struct strbuf *buf, const char *dir_path,\n> > +\t\t       const char *prefix, const char *suffix,\n> > +\t\t       time_t t, int *i, const char *ext)\n> > +{\n> > +\tstruct tm tm;\n> > +\n> > +\tstrbuf_reset(buf);\n> > +\tstrbuf_addstr(buf, dir_path);\n> > +\tstrbuf_complete(buf, '/');\n> > +\n> > +\tstrbuf_addstr(buf, prefix);\n> > +\tstrbuf_addftime(buf, suffix, localtime_r(&t, &tm), 0, 0);\n> > +\n> > +\tif (*i > 0)\n> > +\t\tstrbuf_addf(buf, \"+%d\", *i);\n> > +\n> > +\tstrbuf_addstr(buf, ext);\n> > +\n> > +\t(*i)++;\n> > +}\n> \n> I commented on the weirdness of having to decrement i for --diagnose\n> below, but I think I generally just wish that instead of build_path()\n> this function did create_file_with_optional_suffix() and returned the\n> final modified option_suffix(). Better still would be if this function\n> created (or at least tested) all the necessary output paths so you don't\n> end up succeeding in creating a bugreport.txt but failing in creating\n> the diagnostics.zip in some edge case, something like....\n\nSo the funny thing is that the existing behavior of the diagnostics file\njust overwrites any existing diagnostics file with the same name, which\nis at odds with how the bugreport throws an error in the case of a\nconflict. I wasn't sure whether it made sense to touch that here since it\nseemed possibly out of the scope of this change, given that there is\na separate \"git diagnose\" command to take into account.\n\nBut if we keep this behavior it basically means there's no need to test\nthe necessary diagnostics output paths when determining the path of the\nbugreport itself, since whatever we pick for the bugreport will just\noverwrite anything for the diagnotics zip if it already exists. The one\ncaveat is that any future files created should behave the same way...\n\nBut I get that this is perhaps inconsistent and it might be worth it to\nmake \"git diagnose\" work the same way as bugreport so it makes more sense\nto do the kind of validations you mentioned.\n\n> build_suffix(..., &option_suffix) {\n>   ... build timestamp ...\n>   while (...)\n>     for (final_path in eventual_paths) {\n>     \terr = select(final_path);\n> \tif (err)\n> \t  final_path = strcat(most_of_path, i)\n> \telse\n> \t  break\n> (Yeah, that's very handwavey, but I hope you see what I'm getting at.)\n> \n> Really, though, I mostly don't think I like leaving the control variable i raw\n> to the calling scope and making it be manipulated later. Fancy\n> pre-guessing-path-availability aside, I think you could achieve a more\n> pleasant solution even by letting build_path() become\n> create_file_with_optional_suffix() (that reports the optional suffix\n> eventually settled on).\n\n\nHmm... so when you say \"create_file_with_optional_suffix\", do you mean\na function that tests a set of paths to find the minimum incremented\nsuffix that will work for all the paths? Would it use the current method\nthe patch uses of trying to create the files and if creation fails we\nknow the file exists -> increment -> try again? Repeat until all the\nfiles are able to be created and make sure to clean up any extras?\n(And maybe just clean up everything since the actual files with content\nwill be created later on, now that the suffix is known).\n\nAn alternative could be to wrap `build_path()` in a function that does\nthis work, locally scope `i` in it, call build_path on each needed path,\nand test creation until all paths are valid, clean up all the paths,\nthen return the suffix.\n\n> > +\tif (!strlen(option_suffix))\n> > +\t\toption_suffix = \"%Y-%m-%d-%H%M\";\n> > +\telse\n> > +\t\toption_suffix_is_from_user = 1;\n> \n> Looking at where this is used, it looks like you're saying \"if the user\n> specified the suffix manually and has this problem, then that sucks for\n> them, they put their own foot in it\".\n>\n> But I don't know if I necessarily\n> follow that logic - I'd just as soon drop the exception, append an int\n> to the user-provided suffix, and document that we'll do that in the\n> manpage.\n\nHaha - it's interesting how we each interpreted the user's experience here,\nand I was a bit torn about this too. But here are my thoughts on it:\n\nIf the user specifies their own suffix, it seems more natural and even\nexpected for them to just get an error if a file with that name already\nexists. To me it's not that they put their foot in anything, it's that\nthey asked for a specific thing that we can't deliver since it's already\ntaken, so just let them know so they can either delete the existing one\nor alter the custom suffix. Incremeting the filename without telling them\nin this case might not be what they actually want, we're kindof guessing.\n\nHowever, in the default case where no suffix is specified, the user likely\nhas no assumption or awareness about the suffix at all, which is why it\nfelt odd to throw an error out of the blue if the default suffix happens\nto conflict with an existing file that was also created by default. The\nuser didn't ask for anything specific in this case, so would likely be\nconfused as to why they are getting an error when it just worked a second\nago. So I was thinking we can just handle it gracefully and give them the\nincremented report.\n\n> (This isn't something I feel strongly about, except that I think it\n> makes the code harder to follow for not very notable user benefit. I\n> also didn't look through the reviews up until now, so if this was\n> already hashed back and forth, just go ahead and ignore me.)\n\nSetting up the default variables like this was discussed, but not the\ndifference in behavior based on whether the user specifies a suffix.\nAltho I do agree that we sacrifice some consistency here and it does add\nan extra pathway to the code, imo there is a good enough reason to do it\nas described above - to me it makes things more intuitive to the user.\n\n> > +\t\t/* fopen doesn't offer us an O_EXCL alternative, except with glibc. */\n> > +\t\treport = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);\n> > +\t\tif (report < 0 && errno == EEXIST && !option_suffix_is_from_user) {\n> > +\t\t\tbuild_path(&report_path, prefixed_filename,\n> > +\t\t\t\t   \"git-bugreport-\", option_suffix, now, &i,\n> > +\t\t\t\t   \".txt\");\n> > +\t\t\tgoto again;\n> > +\t\t} else if (report < 0) {\n> Nit, but the double-checking of (report < 0) bothers me a little. Is it\n> nicer if it's nested?\n> \n> \tif (report < 0) {\n> \t\tif (errno == EEXIST) {\n> \t\t\tbuild_path(...);\n> \t\t\tgoto again;\n> \t\t}\n> \n> \t\tdie_errno(_(...));\n> \t}\n> \n> I like it a little more, but that's up to taste, I suppose.\n\nI like yours better too :)\n\n> > +\t\t\tdie_errno(_(\"unable to open '%s'\"), report_path.buf);\n> > +\t\t}\n> >  \n> >  \tif (write_in_full(report, buffer.buf, buffer.len) < 0)\n> >  \t\tdie_errno(_(\"unable to write to %s\"), report_path.buf);\n> >  \n> >  \tclose(report);\n> >  \n> > +\t/* Prepare diagnostics, if requested */\n> > +\tif (diagnose != DIAGNOSE_NONE) {\n> > +\t\tstruct strbuf zip_path = STRBUF_INIT;\n> > +\t\ti--; /* Undo last increment to match zipfile suffix to bugreport */\n> I understand why you're doing this, but I'd rather see it decremented\n> (or more care taken in the increment logic elsewhere) closer to where it\n> is being increment-and-checked. If someone wants to add another\n> associated file besides the report and the diagnostics, then the logic\n> for the decrement becomes complicated (what happens if I run `git\n> bugreport --diagnostics --desktop_screencap`? what if I run only `git\n> bugreport --desktop_screencap`?). Even without that potential pain,\n> reading this comment here means I have to say \"oh wait, what did I read\n> above? hold on, let me page it back in\".\n> \n\nYeah these are great points and I completely agree! Even if we stick with\nthis method of doing things, the `i` decrement should be moved out of the\nconditional and up closer to where the value of `i` is set.\n\n> > +\t\tbuild_path(&zip_path, prefixed_filename, \"git-diagnostics-\",\n> > +\t\t\t   option_suffix, now, &i, \".zip\");\n> > +\n> > +\t\tif (create_diagnostics_archive(&zip_path, diagnose))\n> > +\t\t\tdie_errno(_(\"unable to create diagnostics archive %s\"),\n> > +\t\t\t\t  zip_path.buf);\n> > +\n> > +\t\tstrbuf_release(&zip_path);\n> > +\t}\n> > +\n> >  \t/*\n> >  \t * We want to print the path relative to the user, but we still need the\n> >  \t * path relative to us to give to the editor.\n> > -- \n> > 2.42.0.297.g36452639b8\n> \n> Last thing: it probably makes sense to mention this new behavior in the\n> manpage, especially if you'll apply that behavior to user-provided\n> suffixes too.\n\nSure I can add some docs to the manpage, altho as described above I\nprefer throwing an error instead of adding the increment to the\nuser-provided suffixes :D\n\n> Thanks for your effort on the patch so far and again, sorry for the late\n> reply.\n>  - Emily\n\nNo worries at all and thanks a lot for your review! I'll start working\non a v3 - it would be great to get your thoughts on the unresolved items.\n"},{"id":"486358","messageId":"ZZjdEXb0ed4UTjiF.jacob@initialcommit.io","threadId":"60364","inReplyTo":"ZTtZ5CbIGETy1ucV.jacob@initialcommit.io","subject":"Re: [PATCH v3 1/1] bugreport: include +i in outfile suffix as needed","fromName":"Jacob Stopak","fromEmail":"jacob@initialcommit.io","sentAt":"2024-01-06T04:54:41Z","receivedAt":"2024-01-06T04:54:45Z","isPatch":true,"sender":{"key":"jacob@initialcommit.io","avatar":"https://avatars.githubusercontent.com/u/49353917?v=4"},"body":"Hey Emily!\n\nHope you had a great holiday!\n\nI think this thread might have gotten lost in the shuffle.\n\nI had replied to your feedback a couple months ago with a few questions.\n\nIt would be great to get your further input before I continue working on\nthis one.\n\nBest,\nJack\n"}]}