[PATCH] bugreport: add 'seconds' to default outfile name

[PATCH] bugreport: add 'seconds' to default outfile name

[Jacob Stopak]

Currently, git bugreport postfixes the default bugreport filename (and
diagnostics zip filename if --diagnose is supplied) with the current
calendar hour and minute values, assuming the -s flag is absent.

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).

This patch adds the calendar second value to the default bugreport
filename. This technically just shortens the window during which the
issue occurs, but a single second is a small enough time increment that
users will avoid the filename conflict in practice in this scenario.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index d2ae5c305d..b556c6e135 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -106,7 +106,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 	struct tm tm;
 	enum diagnose_mode diagnose = DIAGNOSE_NONE;
 	char *option_output = NULL;
-	char *option_suffix = "%Y-%m-%d-%H%M";
+	char *option_suffix = "%Y-%m-%d-%H%M%S";
 	const char *user_relative_path = NULL;
 	char *prefixed_filename;
 	size_t output_path_len;

base-commit: 493f4622739e9b64f24b465b21aa85870dd9dc09
-- 
2.42.0.297.g393e7d1581

Re: [PATCH] bugreport: add 'seconds' to default outfile name

[Kristoffer Haugsbakk]

Hi Jacob

On Sat, Oct 14, 2023, at 06:01, Jacob Stopak wrote:
> This patch adds the calendar second value to the default bugreport

Nitpick: you can just say “Add the calendar”. “This patch” is redundant.

See `Documentation/SubmittingPatches` at `imperative-mood`.

-- 
Kristoffer

Re: [PATCH] bugreport: add 'seconds' to default outfile name

[Junio C Hamano]

Jacob Stopak <jacob@initialcommit.io> writes:

> Currently, git bugreport postfixes the default bugreport filename (and
> diagnostics zip filename if --diagnose is supplied) with the current
> calendar hour and minute values, assuming the -s flag is absent.

Is "postfix" a verb that is commonly understood?  I would say
"append" would be understood by more readers.  Also, is "calendar"
hour different from other kinds of hours, perhaps stopwatch hours
and microwave-oven hours?

> 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.

This is totally expected and you made an excellent observation.

I personally do not think it is a problem, simply because a quality
bug report that would capture information necessary to diagnose any
issue concisely in a readable fashion would take at least 90 seconds
or more to produce, though.

Instead of lengthening the filename for all files by 2 digits, the
command can retry by adding say "+1", "+2", etc. after the failed
filename to find a unique suffix within the same minute.  It would
mean that after writing git-bugreport-2023-10-14-0920.txt and you
start another one without spending enough time, the new one may
become git-bugreport-2023-10-14-0920+1.txt or something unique.  It
would be really unlikely that you would run out after failing to
find a vacant single digit suffix nine times, i.e. trying "+9".  It
would also help preserve existing user's workflow, e.g. they may
have written automation that assumes the down-to-minute format and
it would keep working on their bug reports without breaking.

Re: [PATCH] bugreport: add 'seconds' to default outfile name

[Dragan Simic]

On 2023-10-14 18:27, Junio C Hamano wrote:
> Jacob Stopak <jacob@initialcommit.io> writes:
>> 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.
> 
> This is totally expected and you made an excellent observation.
> 
> I personally do not think it is a problem, simply because a quality
> bug report that would capture information necessary to diagnose any
> issue concisely in a readable fashion would take at least 90 seconds
> or more to produce, though.
> 
> Instead of lengthening the filename for all files by 2 digits, the
> command can retry by adding say "+1", "+2", etc. after the failed
> filename to find a unique suffix within the same minute.  It would
> mean that after writing git-bugreport-2023-10-14-0920.txt and you
> start another one without spending enough time, the new one may
> become git-bugreport-2023-10-14-0920+1.txt or something unique.

How about making the filename a bit shorter first, to make room for the 
additional two digits, so the example mentioned above would end up being 
named "git-bugreport-20231014-092037.txt"?

Re: [PATCH] bugreport: add 'seconds' to default outfile name

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

> How about making the filename a bit shorter first, to make room for
> the additional two digits, so the example mentioned above would end up
> being named "git-bugreport-20231014-092037.txt"?

The reason I stated not to unconditionally add two more digits is
*not* that I wanted to keep things shorter.  I wanted to keep names
stable and in the same shape as before for sensible people who spend
more than 60 seconds---only those who produce more than one within
the same minute will be affected.

What is your reason to want to make the filename shoter first?
It would break everybody, wouldn't it?

Re: [PATCH] bugreport: add 'seconds' to default outfile name

[Dragan Simic]

On 2023-10-14 19:45, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>> How about making the filename a bit shorter first, to make room for
>> the additional two digits, so the example mentioned above would end up
>> being named "git-bugreport-20231014-092037.txt"?
> 
> The reason I stated not to unconditionally add two more digits is
> *not* that I wanted to keep things shorter.  I wanted to keep names
> stable and in the same shape as before for sensible people who spend
> more than 60 seconds---only those who produce more than one within
> the same minute will be affected.
> 
> What is your reason to want to make the filename shoter first?
> It would break everybody, wouldn't it?

Please note that I haven't researched in detail what else depends on the 
current filename format, which presumably is a whole bunch of stuff, I 
just suggested this filename compaction because I understood that the 
filename length was becoming an issue.

Speaking in general, I somehow find "20220712" a bit more readable than 
"2022-07-12" as part of a filename, but that's of course just my 
personal preference.

Re: [PATCH] bugreport: add 'seconds' to default outfile name

[Jacob Stopak]

On Sat, Oct 14, 2023 at 07:52:32PM +0200, Dragan Simic wrote:
> 
> Speaking in general, I somehow find "20220712" a bit more readable than
> "2022-07-12" as part of a filename, but that's of course just my personal
> preference.

It's funny how we all have our own preferences for this kind of thing.
Mine happens to be separating the date part from the rest of the
filename with an underscore, but using a hyphen to separate individual
date components like:

filename_2022-07-12.ext

Re: [PATCH] bugreport: add 'seconds' to default outfile name

[Dragan Simic]

On 2023-10-15 05:07, Jacob Stopak wrote:
> On Sat, Oct 14, 2023 at 07:52:32PM +0200, Dragan Simic wrote:
>> 
>> Speaking in general, I somehow find "20220712" a bit more readable 
>> than
>> "2022-07-12" as part of a filename, but that's of course just my 
>> personal
>> preference.
> 
> It's funny how we all have our own preferences for this kind of thing.
> Mine happens to be separating the date part from the rest of the
> filename with an underscore, but using a hyphen to separate individual
> date components like:
> 
> filename_2022-07-12.ext

Yes, it's quite funny.  I gave it some thought, and I think that my 
preference is a result of dealing with many PDF files containing 
schematics, for which some kind of a defacto standard is the "-20220712" 
naming convention.

Re: [PATCH] bugreport: add 'seconds' to default outfile name

[Jacob Stopak]

On Sat, Oct 14, 2023 at 09:27:27AM -0700, Junio C Hamano wrote:
> Jacob Stopak <jacob@initialcommit.io> writes:
> 
> Is "postfix" a verb that is commonly understood?  I would say
> "append" would be understood by more readers.

It's probably true that "append" or "suffix" (which is used in the code)
would be more easily understood. I'll switch in my updated messages.

> Also, is "calendar"
> hour different from other kinds of hours, perhaps stopwatch hours
> and microwave-oven hours?

Lol! By saying "calendar" I mean "falling on the official boundaries
of", like 11:15:00 - 11:16:00. Unlike the time between 11:15:30 -
11:16:30 which is also a minute, but it's not a "calendar" minute
because it overlaps into the next minute. I guess in this case it's more
of a "clock" minute than a "calendar" minute though ':D... I guess
"calendar" terminology is used more for months/years...

> I personally do not think it is a problem, simply because a quality
> bug report that would capture information necessary to diagnose any
> issue concisely in a readable fashion would take at least 90 seconds
> or more to produce, though.

This is true, when the user intentionally opens the bugreport with the
intent to start filling it out immediately, I assume they would almost
always cross the minute barrier and avoid the issue.

However, there are edge cases like the one I outlined, where the user
might open and close the report quickly, followed by rerunning the
command. This could be someone learning to use the command for the first
time. Or the case where a user only fills in a small part of the report
before closing it and running the command again.

These cases are certainly "the exception" but it seems the program could
be a bit more consistent/intuitive when they do occur.

> Instead of lengthening the filename for all files by 2 digits, the
> command can retry by adding say "+1", "+2", etc. after the failed
> filename to find a unique suffix within the same minute.  It would
> mean that after writing git-bugreport-2023-10-14-0920.txt and you
> start another one without spending enough time, the new one may
> become git-bugreport-2023-10-14-0920+1.txt or something unique.  It
> would be really unlikely that you would run out after failing to
> find a vacant single digit suffix nine times, i.e. trying "+9".  It
> would also help preserve existing user's workflow, e.g. they may
> have written automation that assumes the down-to-minute format and
> it would keep working on their bug reports without breaking.

I agree with all of this, and to me it's a better solution than
_appending_ the second value :). I have a patchset almost ready for this 
so I'll try to submit it later tonight.

Re: [PATCH] bugreport: add 'seconds' to default outfile name

[Junio C Hamano]

Jacob Stopak <jacob@initialcommit.io> writes:

> Lol! By saying "calendar" I mean "falling on the official boundaries
> of", like 11:15:00 - 11:16:00. Unlike the time between 11:15:30 -
> 11:16:30 which is also a minute, but it's not a "calendar" minute
> because it overlaps into the next minute. I guess in this case it's more
> of a "clock" minute than a "calendar" minute though ':D... I guess
> "calendar" terminology is used more for months/years...

People use "calendar" days usually in order to to differenciate it
with "business" days.  It may take your package to come cross
country and take 5 business days, but if you ship it out on Friday,
it will not arrive by Wednesday.

[PATCH v2 0/3] bugreport: include +i in outfile suffix as needed

[Jacob Stopak]

Update git bugreport to allow creation of multiple bugreports with
default settings during a given minute interval.

Address several edge cases where users might run git bugreport multiple
times within a minute and expect multiple reports to be created.

Handle these cases by checking to see if a file with the default
timestamped filename already exists, and if so, appending a '+1' value
to the filename suffix. Keep doing so until a unique filename is reached
or '+9' is reached. At this point, if uniqueness is still not found,
the previous error that a file with the given name already exists.

If the --diagnose flag is supplied, apply the same '+i' incremented
filename suffix to the diagnostics zip file.

Reorder the code block that creates the diagnostics zip file so it isn't
created in the event the bugreport itself isn't created due to an error.

Jacob Stopak (3):
  bugreport: include +i in outfile suffix as needed
  bugreport: match diagnostics filename with report
  bugreport: don't create --diagnose zip w/o report

 builtin/bugreport.c | 54 ++++++++++++++++++++++++++++++---------------
 1 file changed, 36 insertions(+), 18 deletions(-)


base-commit: 493f4622739e9b64f24b465b21aa85870dd9dc09
-- 
2.42.0.298.gd89efca819.dirty

[PATCH v2 1/3] bugreport: include +i in outfile suffix as needed

[Jacob Stopak]

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(-)

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);
-- 
2.42.0.298.gd89efca819.dirty

Re: [PATCH v2 1/3] bugreport: include +i in outfile suffix as needed

[Junio C Hamano]

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.

[PATCH v3 0/1] bugreport: include +i in outfile suffix as needed

[Jacob Stopak]

Similar functionality to the previous patch version but with the
following changes:

  * Remove the default_option_suffix variable and clean up the way the
    option_suffix value is set.

  * Refactor code for building report path and diagnostics zip file path
    into the new function build_path(...), which builds a fresh path
    from scratch each time it's called. Although it does take quite a few
    arguments, it reduces duplicated code and simplifies the code by
    removing the need for splicing or inserting the incrementable suffix.

  * Allow the increment value 'i' to grow as needed instead of stopping
    at 9.

  * Replace xopen() with open() so that the program doesn't die with an
    error if the file already exists.

  * Replace the while loop with a fallback loop to prevent TOCTOU.

  * Clean up commit message verbiage.

Some additional comments:

> Perhaps we should refine the error message we give in this case and
> we are done, then?

I had thought about this but due to the variable timed behavior I had
trouble coming up with a message that would convey clearly what's going
on and what the user should do. Here were some things that popped into
my head but they all sounded a bit silly to me:

"A bugreport with this name already exists, try again shortly"
or
"File exists: wait 1 minute and try again or use a custom suffix with -s"
or
"File exists: try again in 1 to 60 seconds :-P"

I think the reason they sound silly to me is that the user is only being
made aware of this info because the program happens to be operating this
way - instead of the program working in a smoother way where this type of
operational info wouldn't need to be conveyed.

> 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.

I completely agree with this and made these updates.

> Notice that you said "in a single minute" without "calendar" and the
> sentence is perfectly understandable?

I googled "calendar minute" and the only thing that came up was some
Java documentation... So I guess I just made it up... :'D

Jacob Stopak (1):
  bugreport: include +i in outfile suffix as needed

 builtin/bugreport.c | 83 +++++++++++++++++++++++++++++++--------------
 1 file changed, 57 insertions(+), 26 deletions(-)


base-commit: 493f4622739e9b64f24b465b21aa85870dd9dc09
-- 
2.42.0.297.g36452639b8

[PATCH v3 1/1] bugreport: include +i in outfile suffix as needed

[Jacob Stopak]

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(-)

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.
-- 
2.42.0.297.g36452639b8

Re: [PATCH v3 1/1] bugreport: include +i in outfile suffix as needed

[Junio C Hamano]

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 ;-).

Re: [PATCH v3 1/1] bugreport: include +i in outfile suffix as needed

[Jacob Stopak]

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.

Re: [PATCH v3 1/1] bugreport: include +i in outfile suffix as needed

[Junio C Hamano]

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/

Re: [PATCH v3 1/1] bugreport: include +i in outfile suffix as needed

[Emily Shaffer]

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

Re: [PATCH v3 1/1] bugreport: include +i in outfile suffix as needed

[Jacob Stopak]

> > +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.

Re: [PATCH v3 1/1] bugreport: include +i in outfile suffix as needed

[Jacob Stopak]

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

[PATCH v2 2/3] bugreport: match diagnostics filename with report

[Jacob Stopak]

When using the --diagnose flag, match the diagnostics zip filename with
the bugreport filename in the scenario where '+i' syntax is used.

Signed-off-by: Jacob Stopak <jacob@initialcommit.io>
---
 builtin/bugreport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 71ee7d7f4b..573d270677 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -112,6 +112,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 	char *prefixed_filename;
 	size_t output_path_len;
 	int ret;
+	int i = 1;
 
 	const struct option bugreport_options[] = {
 		OPT_CALLBACK_F(0, "diagnose", &diagnose, N_("mode"),
@@ -142,7 +143,6 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 	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)
@@ -167,6 +167,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 		strbuf_add(&zip_path, report_path.buf, output_path_len);
 		strbuf_addstr(&zip_path, "git-diagnostics-");
 		strbuf_addftime(&zip_path, option_suffix.buf, localtime_r(&now, &tm), 0, 0);
+		if (i > 1) strbuf_addf(&zip_path, "+%d", i-1);
 		strbuf_addstr(&zip_path, ".zip");
 
 		if (create_diagnostics_archive(&zip_path, diagnose))
-- 
2.42.0.298.gd89efca819.dirty

[PATCH v2 3/3] bugreport: don't create --diagnose zip w/o report

[Jacob Stopak]

Prevent the diagnostics zip file from being created when the bugreport
itself is not created due to an error.

Signed-off-by: Jacob Stopak <jacob@initialcommit.io>
---
 builtin/bugreport.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 573d270677..91567806c9 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -161,21 +161,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.buf, localtime_r(&now, &tm), 0, 0);
-		if (i > 1) strbuf_addf(&zip_path, "+%d", i-1);
-		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);
 
@@ -202,6 +187,22 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 	fprintf(stderr, _("Created new report at '%s'.\n"),
 		user_relative_path);
 
+	/* 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.buf, localtime_r(&now, &tm), 0, 0);
+		if (i > 1) strbuf_addf(&zip_path, "+%d", i-1);
+		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);
+	}
+
+
 	free(prefixed_filename);
 	strbuf_release(&buffer);
 	strbuf_release(&default_option_suffix);
-- 
2.42.0.298.gd89efca819.dirty