[PATCH] Documentation: fix default directory of git bugreport -o

[PATCH] Documentation: fix default directory of git bugreport -o

[Bagas Sanjaya]

git bugreport writes bug report to the current directory by default,
instead of repository root.

Fix the documentation.

Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>
---
 Documentation/git-bugreport.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt
index 66e88c2e31..d8817bf3ce 100644
--- a/Documentation/git-bugreport.txt
+++ b/Documentation/git-bugreport.txt
@@ -40,8 +40,8 @@ OPTIONS
 -------
 -o <path>::
 --output-directory <path>::
-	Place the resulting bug report file in `<path>` instead of the root of
-	the Git repository.
+	Place the resulting bug report file in `<path>` instead of the current
+	directory.
 
 -s <format>::
 --suffix <format>::

base-commit: 6c40894d2466d4e7fddc047a05116aa9d14712ee
-- 
2.25.1

bugreport papercuts

[Carlo Marcelo Arenas Belón]

While reviewing this patch, noticed the following other minor issues
as well:

  [PATCH 1/2] bugreport: avoid duplicating options in usage()
  [PATCH 2/2] bugreport: slightly better memory management

Maybe could we join them all in one single "papercuts" thread for
easy of management?

Carlo

[PATCH 1/2] bugreport: avoid duplicating options in usage()

[Carlo Marcelo Arenas Belón]

238b439d69 (bugreport: add tool to generate debugging info, 2020-04-16)
includes the options with the commandline, which then means they will
be duplicated in the output of `git bugreport -h`.

remove them and while at it, make sure usage() is called if the wrong
number of parameters is provided (ex: `git bugreport help`)

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 builtin/bugreport.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 9915a5841d..17042381c3 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -5,7 +5,6 @@
 #include "compat/compiler.h"
 #include "run-command.h"
 
-
 static void get_system_info(struct strbuf *sys_info)
 {
 	struct utsname uname_info;
@@ -87,7 +86,7 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)
 }
 
 static const char * const bugreport_usage[] = {
-	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"),
+	N_("git bugreport"),
 	NULL
 };
 
@@ -141,6 +140,8 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, prefix, bugreport_options,
 			     bugreport_usage, 0);
+	if (argc)
+		usage_with_options(bugreport_usage, bugreport_options);
 
 	/* Prepare the path to put the result */
 	prefixed_filename = prefix_filename(prefix,
-- 
2.33.0.481.g26d3bed244

Re: [PATCH 1/2] bugreport: avoid duplicating options in usage()

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> 238b439d69 (bugreport: add tool to generate debugging info, 2020-04-16)
> includes the options with the commandline, which then means they will
> be duplicated in the output of `git bugreport -h`.
>
> remove them and while at it, make sure usage() is called if the wrong
> number of parameters is provided (ex: `git bugreport help`)

'remove' -> 'Remove'.

>  static const char * const bugreport_usage[] = {
> -	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"),
> +	N_("git bugreport"),

I do not quite see this as an improvement.  Without this change, the
user will see

	usage: git bugreport [-o <file>] [-s <format>]

	    -o <file>
		... explanation of what -o does ...
	    -s <format>
		... explanation of what -s does ...

and with the patch, it becomes unclear, especially for those who are
not used to "git subcommand -h" output convention, as we'd see only

	usage: git bugreport

on the first line, no?  If the patch is to use

	N_("git bugreport [<options>]")

as the new text, then that would be an improvement, though.

> @@ -141,6 +140,8 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
>  
>  	argc = parse_options(argc, argv, prefix, bugreport_options,
>  			     bugreport_usage, 0);
> +	if (argc)
> +		usage_with_options(bugreport_usage, bugreport_options);

This is a good change (until we gain positional argument to the
subcommand, at which time we'd need to rethink the error checking).

>  	/* Prepare the path to put the result */
>  	prefixed_filename = prefix_filename(prefix,

[PATCH 2/2] bugreport: slightly better memory management

[Carlo Marcelo Arenas Belón]

238b439d69 (bugreport: add tool to generate debugging info, 2020-04-16)
introduces an UNLEAK for a strbuf that contains the buffer that gets
flushed to disk earlier, instead of simply cleaning the buffer.

do so, and while at it, move the free() call for another temporary string
closer to its creator, so it is easier to keep track of.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 builtin/bugreport.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 17042381c3..a9bedde1e8 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -152,6 +152,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 	strbuf_addstr(&report_path, "git-bugreport-");
 	strbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);
 	strbuf_addstr(&report_path, ".txt");
+	free(prefixed_filename);
 
 	switch (safe_create_leading_directories(report_path.buf)) {
 	case SCLD_OK:
@@ -181,6 +182,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 		die_errno(_("unable to write to %s"), report_path.buf);
 
 	close(report);
+	strbuf_release(&buffer);
 
 	/*
 	 * We want to print the path relative to the user, but we still need the
@@ -191,8 +193,6 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 	fprintf(stderr, _("Created new report at '%s'.\n"),
 		user_relative_path);
 
-	free(prefixed_filename);
-	UNLEAK(buffer);
 	UNLEAK(report_path);
 	return !!launch_editor(report_path.buf, NULL, NULL);
 }
-- 
2.33.0.481.g26d3bed244

Re: [PATCH 2/2] bugreport: slightly better memory management

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> 238b439d69 (bugreport: add tool to generate debugging info, 2020-04-16)
> introduces an UNLEAK for a strbuf that contains the buffer that gets
> flushed to disk earlier, instead of simply cleaning the buffer.
>
> do so, and while at it, move the free() call for another temporary string
> closer to its creator, so it is easier to keep track of.

'do so' -> 'Do so'.

>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  builtin/bugreport.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index 17042381c3..a9bedde1e8 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -152,6 +152,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
>  	strbuf_addstr(&report_path, "git-bugreport-");
>  	strbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);
>  	strbuf_addstr(&report_path, ".txt");
> +	free(prefixed_filename);

Correct, but it can be raised even further.  We can free it after we
addstr to report_path.

I looked at the existing callers of prefix_filename(), hoping that
many of them might take it in a strbuf they have, in which case we
may be able to expose an alternative interface to take a caller
supplied strbuf to clean this one up.  But it seems this is the only
one, so "store in a temporary, strbuf_addstr it, and immediately
free the temporary" here would be good.

>  	switch (safe_create_leading_directories(report_path.buf)) {
>  	case SCLD_OK:
> @@ -181,6 +182,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
>  		die_errno(_("unable to write to %s"), report_path.buf);
>  
>  	close(report);
> +	strbuf_release(&buffer);

We are done with the strbuf once write_in_full() returns, but this
is close enough.

> @@ -191,8 +193,6 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
>  	fprintf(stderr, _("Created new report at '%s'.\n"),
>  		user_relative_path);
>  
> -	free(prefixed_filename);
> -	UNLEAK(buffer);
>  	UNLEAK(report_path);
>  	return !!launch_editor(report_path.buf, NULL, NULL);

Having reviewed all, I am not sure if my reaction is "good, now we
are cleaner" or "meh, for the same reason why report_path can be
left alive, it is fine to leave buffer alive, too".

Re: bugreport papercuts

[Bagas Sanjaya]

On 04/09/21 09.12, Carlo Marcelo Arenas Belón wrote:
> While reviewing this patch, noticed the following other minor issues
> as well:
> 
>    [PATCH 1/2] bugreport: avoid duplicating options in usage()
>    [PATCH 2/2] bugreport: slightly better memory management
> 
> Maybe could we join them all in one single "papercuts" thread for
> easy of management?

OK, please add [1].

[1]: 
https://lore.kernel.org/git/20210903115933.622847-1-bagasdotme@gmail.com/

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH] Documentation: fix default directory of git bugreport -o

[Junio C Hamano]

Bagas Sanjaya <bagasdotme@gmail.com> writes:

> git bugreport writes bug report to the current directory by default,
> instead of repository root.
>
> Fix the documentation.
>
> Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>
> ---
>  Documentation/git-bugreport.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Emily, ack?  It seems that after we cdup to the top, we adjust
either the value of -o or "" with the prefix to come up with the
output location, so the default seems to be "the current" as the
updated documentation says.


> diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt
> index 66e88c2e31..d8817bf3ce 100644
> --- a/Documentation/git-bugreport.txt
> +++ b/Documentation/git-bugreport.txt
> @@ -40,8 +40,8 @@ OPTIONS
>  -------
>  -o <path>::
>  --output-directory <path>::
> -	Place the resulting bug report file in `<path>` instead of the root of
> -	the Git repository.
> +	Place the resulting bug report file in `<path>` instead of the current
> +	directory.
>  
>  -s <format>::
>  --suffix <format>::
>
> base-commit: 6c40894d2466d4e7fddc047a05116aa9d14712ee