| Message ID | 20231015034238.100675-2-jacob@initialcommit.io (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | bugreport: include +i in outfile suffix as needed | expand |
Jacob Stopak <jacob@initialcommit.io> writes: > When the -s flag is absent, git bugreport includes the current hour and > minute values in the default bugreport filename (and diagnostics zip > filename if --diagnose is supplied). > > If a user runs the bugreport command more than once within a calendar > minute, a filename conflict with an existing file occurs and the program > errors, since the new output filename was already used for the previous > file. If the user waits anywhere from 1 to 60 seconds (depending on > _when during the calendar minute_ the first command was run) the command Drop "calendar" here (see below). "If the user waits from running the command within the same minute" may be easier to understand than "from 1 to 60 seconds"; after all, the user does not have to wait for more than 0.5 seconds if the previous attempt was within 0.5 seconds from the minute boundary. > works again with no error since the default filename is now unique, and > multiple bug reports are able to be created with default settings. > > This is a minor thing but can cause confusion especially for first time > users of the bugreport command, who are likely to run it multiple times > in quick succession to learn how it works, (like I did). Perhaps we should refine the error message we give in this case and we are done, then? $ GIT_EDITOR=: git bugreport ; GIT_EDITOR=: git bugreport Created new report at 'git-bugreport-2023-10-15-1008.txt'. fatal: unable to create 'git-bugreport-2023-10-15-1008.txt': File exists The second message can be a bit more friendly and suggest removing the stale file. > Add a '+i' into the bugreport filename suffix to make the filename > unique, where 'i' is an integer starting at 1 and able to grow up to 9 > in the unlikely event a user runs the command 9 times in a single > minute. This leads to default output filenames like: What downside do you see in using 2023-10-15-1008+10 after you tried 9 of them? The code to limit the upper bound smells like a wasted effort that helps nobody in practice because it is "unlikely". And limiting the upper bound also means you now have to have extra code to deal with "we ran out---error out and help the user how to recover" anyway. Notice that you said "in a single minute" without "calendar" and the sentence is perfectly understandable? (see above and below). > This means the user will end up with multiple bugreport files being > created if they run the command multiple times quickly, but that feels > more intuitive and consistent than an error arbitrarily occuring within > a calendar minute, especially given that the time window in which the > error currently occurs is variable as described above. And drop "calendar" here, too (see above). "Within the same minute" is fine. > @@ -3,7 +3,6 @@ > #include "editor.h" > #include "gettext.h" > #include "parse-options.h" > -#include "strbuf.h" Looks like an unrelated change here. The updated code does use strbuf service, so even if other header files happen to include it, keep including it here is a good discipline for readability. > @@ -101,12 +101,13 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) > { > struct strbuf buffer = STRBUF_INIT; > struct strbuf report_path = STRBUF_INIT; > + struct strbuf option_suffix = STRBUF_INIT; > + struct strbuf default_option_suffix = STRBUF_INIT; > int report = -1; > time_t now = time(NULL); > struct tm tm; > enum diagnose_mode diagnose = DIAGNOSE_NONE; > char *option_output = NULL; > - char *option_suffix = "%Y-%m-%d-%H%M"; > const char *user_relative_path = NULL; > char *prefixed_filename; > size_t output_path_len; > @@ -118,11 +119,14 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) > PARSE_OPT_OPTARG, option_parse_diagnose), > OPT_STRING('o', "output-directory", &option_output, N_("path"), > N_("specify a destination for the bugreport file(s)")), > - OPT_STRING('s', "suffix", &option_suffix, N_("format"), > + OPT_STRING('s', "suffix", &option_suffix.buf, N_("format"), > N_("specify a strftime format suffix for the filename(s)")), > OPT_END() > }; > > + strbuf_addstr(&default_option_suffix, "%Y-%m-%d-%H%M"); > + strbuf_addstr(&option_suffix, default_option_suffix.buf); It usually pays for a reviewer when two variables, one called default and the other not, gets initialized to the same value, because it is a sign that there is something fishy going on. A more normal pattern is to set up the default, do whatever is needed and if the non-default one has not been touched, then copy that default value to the real one, and the code needed deviation from it for whatever reason. Let's read on. > @@ -134,9 +138,20 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) > output_path_len = report_path.len; > > strbuf_addstr(&report_path, "git-bugreport-"); > - strbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0); > + strbuf_addftime(&report_path, option_suffix.buf, localtime_r(&now, &tm), 0, 0); > strbuf_addstr(&report_path, ".txt"); OK. > + if (strbuf_cmp(&option_suffix, &default_option_suffix) == 0) { Style: never compare with 0 or NULL inside a conditional. if (!compare(...)) { is the idiom to use. You may have written it this way to avoid appending after what the user gave you as a custom pattern, but this if() statement is a failed way to do so. The user can give what happens to be the same as the hardcoded default pattern and you cannot tell if it came from them or your initially hardcoded one by string comparison. > + int i = 1; > + int pos = report_path.len - 4; Totally unclear where the magic "4" comes from. > + while (file_exists(report_path.buf) && i < 10) { > + if (i > 1) > + strbuf_remove(&report_path, pos, 2); > + strbuf_insertf(&report_path, pos, "+%d", i); > + i++; > + } > + } We see TOCTOU here. Do it more like this instead. * Discard default_option_suffix variable. * Introduce a boolean option_suffix_is_from_user and initialize it to false. * Initialize option_suffix to an empty string. * After parse_options() returns, if option_suffix is still empty, then add the default pattern. Otherwise toggle option_suffix_is_from_user true. * Prepare the code so that you can recompute report_path as you need to increment the suffix added to option_suffix. Then where we do xopen() on report_path.buf, have a fallback loop, and you can do something like again: report = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666); if (report < 0 && errno == EEXISTS && !option_suffix_is_from_user) { increment_suffix(&report_path); goto again; } else if (report < 0) { die_errno(_("unable to open '%s'", report_path.buf)); } to avoid TOCTOU. HTH.
diff --git a/builtin/bugreport.c b/builtin/bugreport.c index d2ae5c305d..71ee7d7f4b 100644 --- a/builtin/bugreport.c +++ b/builtin/bugreport.c @@ -3,7 +3,6 @@ #include "editor.h" #include "gettext.h" #include "parse-options.h" -#include "strbuf.h" #include "help.h" #include "compat/compiler.h" #include "hook.h" @@ -11,6 +10,7 @@ #include "diagnose.h" #include "object-file.h" #include "setup.h" +#include "dir.h" static void get_system_info(struct strbuf *sys_info) { @@ -101,12 +101,13 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) { struct strbuf buffer = STRBUF_INIT; struct strbuf report_path = STRBUF_INIT; + struct strbuf option_suffix = STRBUF_INIT; + struct strbuf default_option_suffix = STRBUF_INIT; int report = -1; time_t now = time(NULL); struct tm tm; enum diagnose_mode diagnose = DIAGNOSE_NONE; char *option_output = NULL; - char *option_suffix = "%Y-%m-%d-%H%M"; const char *user_relative_path = NULL; char *prefixed_filename; size_t output_path_len; @@ -118,11 +119,14 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) PARSE_OPT_OPTARG, option_parse_diagnose), OPT_STRING('o', "output-directory", &option_output, N_("path"), N_("specify a destination for the bugreport file(s)")), - OPT_STRING('s', "suffix", &option_suffix, N_("format"), + OPT_STRING('s', "suffix", &option_suffix.buf, N_("format"), N_("specify a strftime format suffix for the filename(s)")), OPT_END() }; + strbuf_addstr(&default_option_suffix, "%Y-%m-%d-%H%M"); + strbuf_addstr(&option_suffix, default_option_suffix.buf); + argc = parse_options(argc, argv, prefix, bugreport_options, bugreport_usage, 0); @@ -134,9 +138,20 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) output_path_len = report_path.len; strbuf_addstr(&report_path, "git-bugreport-"); - strbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0); + strbuf_addftime(&report_path, option_suffix.buf, localtime_r(&now, &tm), 0, 0); strbuf_addstr(&report_path, ".txt"); + if (strbuf_cmp(&option_suffix, &default_option_suffix) == 0) { + int i = 1; + int pos = report_path.len - 4; + while (file_exists(report_path.buf) && i < 10) { + if (i > 1) + strbuf_remove(&report_path, pos, 2); + strbuf_insertf(&report_path, pos, "+%d", i); + i++; + } + } + switch (safe_create_leading_directories(report_path.buf)) { case SCLD_OK: case SCLD_EXISTS: @@ -151,7 +166,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) struct strbuf zip_path = STRBUF_INIT; strbuf_add(&zip_path, report_path.buf, output_path_len); strbuf_addstr(&zip_path, "git-diagnostics-"); - strbuf_addftime(&zip_path, option_suffix, localtime_r(&now, &tm), 0, 0); + strbuf_addftime(&zip_path, option_suffix.buf, localtime_r(&now, &tm), 0, 0); strbuf_addstr(&zip_path, ".zip"); if (create_diagnostics_archive(&zip_path, diagnose)) @@ -188,6 +203,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) free(prefixed_filename); strbuf_release(&buffer); + strbuf_release(&default_option_suffix); ret = !!launch_editor(report_path.buf, NULL, NULL); strbuf_release(&report_path);
When the -s flag is absent, git bugreport includes the current hour and minute values in the default bugreport filename (and diagnostics zip filename if --diagnose is supplied). If a user runs the bugreport command more than once within a calendar minute, a filename conflict with an existing file occurs and the program errors, since the new output filename was already used for the previous file. If the user waits anywhere from 1 to 60 seconds (depending on _when during the calendar minute_ the first command was run) the command works again with no error since the default filename is now unique, and multiple bug reports are able to be created with default settings. This is a minor thing but can cause confusion especially for first time users of the bugreport command, who are likely to run it multiple times in quick succession to learn how it works, (like I did). Add a '+i' into the bugreport filename suffix to make the filename unique, where 'i' is an integer starting at 1 and able to grow up to 9 in the unlikely event a user runs the command 9 times in a single minute. This leads to default output filenames like: git-bugreport-%Y-%m-%d-%H%M+1.txt git-bugreport-%Y-%m-%d-%H%M+2.txt ... git-bugreport-%Y-%m-%d-%H%M+9.txt This means the user will end up with multiple bugreport files being created if they run the command multiple times quickly, but that feels more intuitive and consistent than an error arbitrarily occuring within a calendar minute, especially given that the time window in which the error currently occurs is variable as described above. Signed-off-by: Jacob Stopak <jacob@initialcommit.io> --- builtin/bugreport.c | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-)