| Message ID | 20231016214045.146862-2-jacob@initialcommit.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bugreport: include +i in outfile suffix as needed | expand |
Jacob Stopak <jacob@initialcommit.io> writes: > builtin/bugreport.c | 83 +++++++++++++++++++++++++++++++-------------- > 1 file changed, 57 insertions(+), 26 deletions(-) Looking good. It is not easy to do an automated and reliable test for this one for obvious reasons ;-), so let's queue it as-is. Thanks. > - /* fopen doesn't offer us an O_EXCL alternative, except with glibc. */ > - report = xopen(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666); > + again: > + /* fopen doesn't offer us an O_EXCL alternative, except with glibc. */ > + report = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666); > + if (report < 0 && errno == EEXIST && !option_suffix_is_from_user) { > + build_path(&report_path, prefixed_filename, > + "git-bugreport-", option_suffix, now, &i, > + ".txt"); > + goto again; > + } else if (report < 0) { > + die_errno(_("unable to open '%s'"), report_path.buf); > + } I didn't expect a rewrite to add an extra level of indentation like this, though ;-).
On Mon, Oct 16, 2023 at 03:55:50PM -0700, Junio C Hamano wrote: > Jacob Stopak <jacob@initialcommit.io> writes: > > > builtin/bugreport.c | 83 +++++++++++++++++++++++++++++++-------------- > > 1 file changed, 57 insertions(+), 26 deletions(-) > > Looking good. It is not easy to do an automated and reliable test > for this one for obvious reasons ;-), so let's queue it as-is. > > Thanks. > Awesome! Thank you! > > - /* fopen doesn't offer us an O_EXCL alternative, except with glibc. */ > > - report = xopen(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666); > > + again: > > + /* fopen doesn't offer us an O_EXCL alternative, except with glibc. */ > > + report = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666); > > + if (report < 0 && errno == EEXIST && !option_suffix_is_from_user) { > > + build_path(&report_path, prefixed_filename, > > + "git-bugreport-", option_suffix, now, &i, > > + ".txt"); > > + goto again; > > + } else if (report < 0) { > > + die_errno(_("unable to open '%s'"), report_path.buf); > > + } > > I didn't expect a rewrite to add an extra level of indentation like > this, though ;-). Whoops... I looked in another file to check the indentation around a goto label and misread how it should be. Let me know if I should submit v4 with that corrected.
Jacob Stopak <jacob@initialcommit.io> writes: > int cmd_bugreport(int argc, const char **argv, const char *prefix) > { > struct strbuf buffer = STRBUF_INIT; > struct strbuf report_path = 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"; > + char *option_suffix = ""; > + int option_suffix_is_from_user = 0; > const char *user_relative_path = NULL; > char *prefixed_filename; > - size_t output_path_len; > int ret; > + int i = 0; OK, I think between me and you, we stared at this piece of code long enough to make ourselves numb. The original "at most one report per a minute" default came from the very original in 238b439d (bugreport: add tool to generate debugging info, 2020-04-16) and that is what we are changing, so let me summon its author as an area expert for a pair of fresh eyes to see if they can offer any new insights. Thanks. https://lore.kernel.org/git/20231016214045.146862-1-jacob@initialcommit.io/
Thanks Jack for drawing my attention to the summoning, and sorry for delay. On Mon, Oct 16, 2023 at 02:40:45PM -0700, Jacob Stopak wrote: > > 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 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 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 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). Or users who quickly > fill in a few details before closing and creating a new one. Sure, agreed - I guess I got in the habit of testing by running `rm git-bugreport*.txt && git bugreport --foo` and never thought about this being annoying for an actual reporter :) > > Add a '+i' into the bugreport filename suffix where 'i' is an integer > starting at 1 and growing as needed until a unique filename is obtained. > > 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+i.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 minute, especially given that the time window in which the error > currently occurs is variable as described above. Sure, and with the monotonic increases it's quite easy to locate the bugreport that's the final one the user generated. This scheme seems fine to me. > > If --diagnose is supplied, match the incremented suffix of the > diagnostics zip file to the bugreport. Nice. > > Signed-off-by: Jacob Stopak <jacob@initialcommit.io> > --- > builtin/bugreport.c | 83 +++++++++++++++++++++++++++++++-------------- > 1 file changed, 57 insertions(+), 26 deletions(-) > > diff --git a/builtin/bugreport.c b/builtin/bugreport.c > index d2ae5c305d..ed65735873 100644 > --- a/builtin/bugreport.c > +++ b/builtin/bugreport.c > @@ -11,6 +11,7 @@ > #include "diagnose.h" > #include "object-file.h" > #include "setup.h" > +#include "dir.h" > > static void get_system_info(struct strbuf *sys_info) > { > @@ -97,20 +98,41 @@ static void get_header(struct strbuf *buf, const char *title) > strbuf_addf(buf, "\n\n[%s]\n", title); > } > > +static void build_path(struct strbuf *buf, const char *dir_path, > + const char *prefix, const char *suffix, > + time_t t, int *i, const char *ext) > +{ > + struct tm tm; > + > + strbuf_reset(buf); > + strbuf_addstr(buf, dir_path); > + strbuf_complete(buf, '/'); > + > + strbuf_addstr(buf, prefix); > + strbuf_addftime(buf, suffix, localtime_r(&t, &tm), 0, 0); > + > + if (*i > 0) > + strbuf_addf(buf, "+%d", *i); > + > + strbuf_addstr(buf, ext); > + > + (*i)++; > +} I commented on the weirdness of having to decrement i for --diagnose below, but I think I generally just wish that instead of build_path() this function did create_file_with_optional_suffix() and returned the final modified option_suffix(). Better still would be if this function created (or at least tested) all the necessary output paths so you don't end up succeeding in creating a bugreport.txt but failing in creating the diagnostics.zip in some edge case, something like.... build_suffix(..., &option_suffix) { ... build timestamp ... while (...) for (final_path in eventual_paths) { err = select(final_path); if (err) final_path = strcat(most_of_path, i) else break (Yeah, that's very handwavey, but I hope you see what I'm getting at.) Really, though, I mostly don't think I like leaving the control variable i raw to the calling scope and making it be manipulated later. Fancy pre-guessing-path-availability aside, I think you could achieve a more pleasant solution even by letting build_path() become create_file_with_optional_suffix() (that reports the optional suffix eventually settled on). > + > int cmd_bugreport(int argc, const char **argv, const char *prefix) > { > struct strbuf buffer = STRBUF_INIT; > struct strbuf report_path = 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"; > + char *option_suffix = ""; > + int option_suffix_is_from_user = 0; > const char *user_relative_path = NULL; > char *prefixed_filename; > - size_t output_path_len; > int ret; > + int i = 0; > > const struct option bugreport_options[] = { > OPT_CALLBACK_F(0, "diagnose", &diagnose, N_("mode"), > @@ -126,16 +148,16 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) > argc = parse_options(argc, argv, prefix, bugreport_options, > bugreport_usage, 0); > > + if (!strlen(option_suffix)) > + option_suffix = "%Y-%m-%d-%H%M"; > + else > + option_suffix_is_from_user = 1; Looking at where this is used, it looks like you're saying "if the user specified the suffix manually and has this problem, then that sucks for them, they put their own foot in it". But I don't know if I necessarily follow that logic - I'd just as soon drop the exception, append an int to the user-provided suffix, and document that we'll do that in the manpage. (This isn't something I feel strongly about, except that I think it makes the code harder to follow for not very notable user benefit. I also didn't look through the reviews up until now, so if this was already hashed back and forth, just go ahead and ignore me.) > + > /* Prepare the path to put the result */ > prefixed_filename = prefix_filename(prefix, > option_output ? option_output : ""); > - strbuf_addstr(&report_path, prefixed_filename); > - strbuf_complete(&report_path, '/'); > - 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_addstr(&report_path, ".txt"); > + build_path(&report_path, prefixed_filename, "git-bugreport-", > + option_suffix, now, &i, ".txt"); > > switch (safe_create_leading_directories(report_path.buf)) { > case SCLD_OK: > @@ -146,20 +168,6 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) > report_path.buf); > } > > - /* Prepare diagnostics, if requested */ > - if (diagnose != DIAGNOSE_NONE) { > - 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_addstr(&zip_path, ".zip"); > - > - if (create_diagnostics_archive(&zip_path, diagnose)) > - die_errno(_("unable to create diagnostics archive %s"), zip_path.buf); > - > - strbuf_release(&zip_path); > - } > - > /* Prepare the report contents */ > get_bug_template(&buffer); > > @@ -169,14 +177,37 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) > get_header(&buffer, _("Enabled Hooks")); > get_populated_hooks(&buffer, !startup_info->have_repository); > > - /* fopen doesn't offer us an O_EXCL alternative, except with glibc. */ > - report = xopen(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666); > + again: > + /* fopen doesn't offer us an O_EXCL alternative, except with glibc. */ > + report = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666); > + if (report < 0 && errno == EEXIST && !option_suffix_is_from_user) { > + build_path(&report_path, prefixed_filename, > + "git-bugreport-", option_suffix, now, &i, > + ".txt"); > + goto again; > + } else if (report < 0) { Nit, but the double-checking of (report < 0) bothers me a little. Is it nicer if it's nested? if (report < 0) { if (errno == EEXIST) { build_path(...); goto again; } die_errno(_(...)); } I like it a little more, but that's up to taste, I suppose. > + die_errno(_("unable to open '%s'"), report_path.buf); > + } > > if (write_in_full(report, buffer.buf, buffer.len) < 0) > die_errno(_("unable to write to %s"), report_path.buf); > > close(report); > > + /* Prepare diagnostics, if requested */ > + if (diagnose != DIAGNOSE_NONE) { > + struct strbuf zip_path = STRBUF_INIT; > + i--; /* Undo last increment to match zipfile suffix to bugreport */ I understand why you're doing this, but I'd rather see it decremented (or more care taken in the increment logic elsewhere) closer to where it is being increment-and-checked. If someone wants to add another associated file besides the report and the diagnostics, then the logic for the decrement becomes complicated (what happens if I run `git bugreport --diagnostics --desktop_screencap`? what if I run only `git bugreport --desktop_screencap`?). Even without that potential pain, reading this comment here means I have to say "oh wait, what did I read above? hold on, let me page it back in". > + build_path(&zip_path, prefixed_filename, "git-diagnostics-", > + option_suffix, now, &i, ".zip"); > + > + if (create_diagnostics_archive(&zip_path, diagnose)) > + die_errno(_("unable to create diagnostics archive %s"), > + zip_path.buf); > + > + strbuf_release(&zip_path); > + } > + > /* > * We want to print the path relative to the user, but we still need the > * path relative to us to give to the editor. > -- > 2.42.0.297.g36452639b8 Last thing: it probably makes sense to mention this new behavior in the manpage, especially if you'll apply that behavior to user-provided suffixes too. Thanks for your effort on the patch so far and again, sorry for the late reply. - Emily
> > +static void build_path(struct strbuf *buf, const char *dir_path, > > + const char *prefix, const char *suffix, > > + time_t t, int *i, const char *ext) > > +{ > > + struct tm tm; > > + > > + strbuf_reset(buf); > > + strbuf_addstr(buf, dir_path); > > + strbuf_complete(buf, '/'); > > + > > + strbuf_addstr(buf, prefix); > > + strbuf_addftime(buf, suffix, localtime_r(&t, &tm), 0, 0); > > + > > + if (*i > 0) > > + strbuf_addf(buf, "+%d", *i); > > + > > + strbuf_addstr(buf, ext); > > + > > + (*i)++; > > +} > > I commented on the weirdness of having to decrement i for --diagnose > below, but I think I generally just wish that instead of build_path() > this function did create_file_with_optional_suffix() and returned the > final modified option_suffix(). Better still would be if this function > created (or at least tested) all the necessary output paths so you don't > end up succeeding in creating a bugreport.txt but failing in creating > the diagnostics.zip in some edge case, something like.... So the funny thing is that the existing behavior of the diagnostics file just overwrites any existing diagnostics file with the same name, which is at odds with how the bugreport throws an error in the case of a conflict. I wasn't sure whether it made sense to touch that here since it seemed possibly out of the scope of this change, given that there is a separate "git diagnose" command to take into account. But if we keep this behavior it basically means there's no need to test the necessary diagnostics output paths when determining the path of the bugreport itself, since whatever we pick for the bugreport will just overwrite anything for the diagnotics zip if it already exists. The one caveat is that any future files created should behave the same way... But I get that this is perhaps inconsistent and it might be worth it to make "git diagnose" work the same way as bugreport so it makes more sense to do the kind of validations you mentioned. > build_suffix(..., &option_suffix) { > ... build timestamp ... > while (...) > for (final_path in eventual_paths) { > err = select(final_path); > if (err) > final_path = strcat(most_of_path, i) > else > break > (Yeah, that's very handwavey, but I hope you see what I'm getting at.) > > Really, though, I mostly don't think I like leaving the control variable i raw > to the calling scope and making it be manipulated later. Fancy > pre-guessing-path-availability aside, I think you could achieve a more > pleasant solution even by letting build_path() become > create_file_with_optional_suffix() (that reports the optional suffix > eventually settled on). Hmm... so when you say "create_file_with_optional_suffix", do you mean a function that tests a set of paths to find the minimum incremented suffix that will work for all the paths? Would it use the current method the patch uses of trying to create the files and if creation fails we know the file exists -> increment -> try again? Repeat until all the files are able to be created and make sure to clean up any extras? (And maybe just clean up everything since the actual files with content will be created later on, now that the suffix is known). An alternative could be to wrap `build_path()` in a function that does this work, locally scope `i` in it, call build_path on each needed path, and test creation until all paths are valid, clean up all the paths, then return the suffix. > > + if (!strlen(option_suffix)) > > + option_suffix = "%Y-%m-%d-%H%M"; > > + else > > + option_suffix_is_from_user = 1; > > Looking at where this is used, it looks like you're saying "if the user > specified the suffix manually and has this problem, then that sucks for > them, they put their own foot in it". > > But I don't know if I necessarily > follow that logic - I'd just as soon drop the exception, append an int > to the user-provided suffix, and document that we'll do that in the > manpage. Haha - it's interesting how we each interpreted the user's experience here, and I was a bit torn about this too. But here are my thoughts on it: If the user specifies their own suffix, it seems more natural and even expected for them to just get an error if a file with that name already exists. To me it's not that they put their foot in anything, it's that they asked for a specific thing that we can't deliver since it's already taken, so just let them know so they can either delete the existing one or alter the custom suffix. Incremeting the filename without telling them in this case might not be what they actually want, we're kindof guessing. However, in the default case where no suffix is specified, the user likely has no assumption or awareness about the suffix at all, which is why it felt odd to throw an error out of the blue if the default suffix happens to conflict with an existing file that was also created by default. The user didn't ask for anything specific in this case, so would likely be confused as to why they are getting an error when it just worked a second ago. So I was thinking we can just handle it gracefully and give them the incremented report. > (This isn't something I feel strongly about, except that I think it > makes the code harder to follow for not very notable user benefit. I > also didn't look through the reviews up until now, so if this was > already hashed back and forth, just go ahead and ignore me.) Setting up the default variables like this was discussed, but not the difference in behavior based on whether the user specifies a suffix. Altho I do agree that we sacrifice some consistency here and it does add an extra pathway to the code, imo there is a good enough reason to do it as described above - to me it makes things more intuitive to the user. > > + /* fopen doesn't offer us an O_EXCL alternative, except with glibc. */ > > + report = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666); > > + if (report < 0 && errno == EEXIST && !option_suffix_is_from_user) { > > + build_path(&report_path, prefixed_filename, > > + "git-bugreport-", option_suffix, now, &i, > > + ".txt"); > > + goto again; > > + } else if (report < 0) { > Nit, but the double-checking of (report < 0) bothers me a little. Is it > nicer if it's nested? > > if (report < 0) { > if (errno == EEXIST) { > build_path(...); > goto again; > } > > die_errno(_(...)); > } > > I like it a little more, but that's up to taste, I suppose. I like yours better too :) > > + die_errno(_("unable to open '%s'"), report_path.buf); > > + } > > > > if (write_in_full(report, buffer.buf, buffer.len) < 0) > > die_errno(_("unable to write to %s"), report_path.buf); > > > > close(report); > > > > + /* Prepare diagnostics, if requested */ > > + if (diagnose != DIAGNOSE_NONE) { > > + struct strbuf zip_path = STRBUF_INIT; > > + i--; /* Undo last increment to match zipfile suffix to bugreport */ > I understand why you're doing this, but I'd rather see it decremented > (or more care taken in the increment logic elsewhere) closer to where it > is being increment-and-checked. If someone wants to add another > associated file besides the report and the diagnostics, then the logic > for the decrement becomes complicated (what happens if I run `git > bugreport --diagnostics --desktop_screencap`? what if I run only `git > bugreport --desktop_screencap`?). Even without that potential pain, > reading this comment here means I have to say "oh wait, what did I read > above? hold on, let me page it back in". > Yeah these are great points and I completely agree! Even if we stick with this method of doing things, the `i` decrement should be moved out of the conditional and up closer to where the value of `i` is set. > > + build_path(&zip_path, prefixed_filename, "git-diagnostics-", > > + option_suffix, now, &i, ".zip"); > > + > > + if (create_diagnostics_archive(&zip_path, diagnose)) > > + die_errno(_("unable to create diagnostics archive %s"), > > + zip_path.buf); > > + > > + strbuf_release(&zip_path); > > + } > > + > > /* > > * We want to print the path relative to the user, but we still need the > > * path relative to us to give to the editor. > > -- > > 2.42.0.297.g36452639b8 > > Last thing: it probably makes sense to mention this new behavior in the > manpage, especially if you'll apply that behavior to user-provided > suffixes too. Sure I can add some docs to the manpage, altho as described above I prefer throwing an error instead of adding the increment to the user-provided suffixes :D > Thanks for your effort on the patch so far and again, sorry for the late > reply. > - Emily No worries at all and thanks a lot for your review! I'll start working on a v3 - it would be great to get your thoughts on the unresolved items.
Hey Emily! Hope you had a great holiday! I think this thread might have gotten lost in the shuffle. I had replied to your feedback a couple months ago with a few questions. It would be great to get your further input before I continue working on this one. Best, Jack
diff --git a/builtin/bugreport.c b/builtin/bugreport.c index d2ae5c305d..ed65735873 100644 --- a/builtin/bugreport.c +++ b/builtin/bugreport.c @@ -11,6 +11,7 @@ #include "diagnose.h" #include "object-file.h" #include "setup.h" +#include "dir.h" static void get_system_info(struct strbuf *sys_info) { @@ -97,20 +98,41 @@ static void get_header(struct strbuf *buf, const char *title) strbuf_addf(buf, "\n\n[%s]\n", title); } +static void build_path(struct strbuf *buf, const char *dir_path, + const char *prefix, const char *suffix, + time_t t, int *i, const char *ext) +{ + struct tm tm; + + strbuf_reset(buf); + strbuf_addstr(buf, dir_path); + strbuf_complete(buf, '/'); + + strbuf_addstr(buf, prefix); + strbuf_addftime(buf, suffix, localtime_r(&t, &tm), 0, 0); + + if (*i > 0) + strbuf_addf(buf, "+%d", *i); + + strbuf_addstr(buf, ext); + + (*i)++; +} + int cmd_bugreport(int argc, const char **argv, const char *prefix) { struct strbuf buffer = STRBUF_INIT; struct strbuf report_path = 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"; + char *option_suffix = ""; + int option_suffix_is_from_user = 0; const char *user_relative_path = NULL; char *prefixed_filename; - size_t output_path_len; int ret; + int i = 0; const struct option bugreport_options[] = { OPT_CALLBACK_F(0, "diagnose", &diagnose, N_("mode"), @@ -126,16 +148,16 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, bugreport_options, bugreport_usage, 0); + if (!strlen(option_suffix)) + option_suffix = "%Y-%m-%d-%H%M"; + else + option_suffix_is_from_user = 1; + /* Prepare the path to put the result */ prefixed_filename = prefix_filename(prefix, option_output ? option_output : ""); - strbuf_addstr(&report_path, prefixed_filename); - strbuf_complete(&report_path, '/'); - 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_addstr(&report_path, ".txt"); + build_path(&report_path, prefixed_filename, "git-bugreport-", + option_suffix, now, &i, ".txt"); switch (safe_create_leading_directories(report_path.buf)) { case SCLD_OK: @@ -146,20 +168,6 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) report_path.buf); } - /* Prepare diagnostics, if requested */ - if (diagnose != DIAGNOSE_NONE) { - 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_addstr(&zip_path, ".zip"); - - if (create_diagnostics_archive(&zip_path, diagnose)) - die_errno(_("unable to create diagnostics archive %s"), zip_path.buf); - - strbuf_release(&zip_path); - } - /* Prepare the report contents */ get_bug_template(&buffer); @@ -169,14 +177,37 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) get_header(&buffer, _("Enabled Hooks")); get_populated_hooks(&buffer, !startup_info->have_repository); - /* fopen doesn't offer us an O_EXCL alternative, except with glibc. */ - report = xopen(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666); + again: + /* fopen doesn't offer us an O_EXCL alternative, except with glibc. */ + report = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666); + if (report < 0 && errno == EEXIST && !option_suffix_is_from_user) { + build_path(&report_path, prefixed_filename, + "git-bugreport-", option_suffix, now, &i, + ".txt"); + goto again; + } else if (report < 0) { + die_errno(_("unable to open '%s'"), report_path.buf); + } if (write_in_full(report, buffer.buf, buffer.len) < 0) die_errno(_("unable to write to %s"), report_path.buf); close(report); + /* Prepare diagnostics, if requested */ + if (diagnose != DIAGNOSE_NONE) { + struct strbuf zip_path = STRBUF_INIT; + i--; /* Undo last increment to match zipfile suffix to bugreport */ + build_path(&zip_path, prefixed_filename, "git-diagnostics-", + option_suffix, now, &i, ".zip"); + + if (create_diagnostics_archive(&zip_path, diagnose)) + die_errno(_("unable to create diagnostics archive %s"), + zip_path.buf); + + strbuf_release(&zip_path); + } + /* * We want to print the path relative to the user, but we still need the * path relative to us to give to the editor.
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 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 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 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). Or users who quickly fill in a few details before closing and creating a new one. Add a '+i' into the bugreport filename suffix where 'i' is an integer starting at 1 and growing as needed until a unique filename is obtained. 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+i.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 minute, especially given that the time window in which the error currently occurs is variable as described above. If --diagnose is supplied, match the incremented suffix of the diagnostics zip file to the bugreport. Signed-off-by: Jacob Stopak <jacob@initialcommit.io> --- builtin/bugreport.c | 83 +++++++++++++++++++++++++++++++-------------- 1 file changed, 57 insertions(+), 26 deletions(-)