# [PATCH] Documentation: fix default directory of git bugreport -o 8 messages from 2021-09-03 to 2021-09-07. Participants: Bagas Sanjaya, Carlo Marcelo Arenas Belón, Junio C Hamano. Thread: https://gitlist.dev/t/56437 ## Bagas Sanjaya, 2021-09-03 11:59 Subject: [PATCH] Documentation: fix default directory of git bugreport -o Message-ID: <20210903115933.622847-1-bagasdotme@gmail.com> URL: https://gitlist.dev/e/20210903115933.622847-1-bagasdotme%40gmail.com ``` git bugreport writes bug report to the current directory by default, instead of repository root. Fix the documentation. Signed-off-by: Bagas Sanjaya --- 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 :: --output-directory :: - Place the resulting bug report file in `` instead of the root of - the Git repository. + Place the resulting bug report file in `` instead of the current + directory. -s :: --suffix :: base-commit: 6c40894d2466d4e7fddc047a05116aa9d14712ee -- 2.25.1 ``` ## Carlo Marcelo Arenas Belón, 2021-09-04 02:12 Subject: bugreport papercuts Message-ID: <20210904021231.88534-1-carenas@gmail.com> URL: https://gitlist.dev/e/20210904021231.88534-1-carenas%40gmail.com In-Reply-To: <20210903115933.622847-1-bagasdotme@gmail.com> ``` 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 ``` ## Carlo Marcelo Arenas Belón, 2021-09-04 02:12 Subject: [PATCH 1/2] bugreport: avoid duplicating options in usage() Message-ID: <20210904021231.88534-2-carenas@gmail.com> URL: https://gitlist.dev/e/20210904021231.88534-2-carenas%40gmail.com In-Reply-To: <20210904021231.88534-1-carenas@gmail.com> ``` 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 --- 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 ] [-s|--suffix ]"), + 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 ``` ## Carlo Marcelo Arenas Belón, 2021-09-04 02:12 Subject: [PATCH 2/2] bugreport: slightly better memory management Message-ID: <20210904021231.88534-3-carenas@gmail.com> URL: https://gitlist.dev/e/20210904021231.88534-3-carenas%40gmail.com In-Reply-To: <20210904021231.88534-1-carenas@gmail.com> ``` 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 --- 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 ``` ## Bagas Sanjaya, 2021-09-04 06:01 Subject: Re: bugreport papercuts Message-ID: URL: https://gitlist.dev/e/b932088f-9ae0-63ee-fc13-fdb11f17c666%40gmail.com In-Reply-To: <20210904021231.88534-1-carenas@gmail.com> ``` 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 ``` ## Junio C Hamano, 2021-09-07 21:09 Subject: Re: [PATCH] Documentation: fix default directory of git bugreport -o Message-ID: URL: https://gitlist.dev/e/xmqqa6ko9jol.fsf%40gitster.g In-Reply-To: <20210903115933.622847-1-bagasdotme@gmail.com> ``` Bagas Sanjaya writes: > git bugreport writes bug report to the current directory by default, > instead of repository root. > > Fix the documentation. > > Signed-off-by: Bagas Sanjaya > --- > 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 :: > --output-directory :: > - Place the resulting bug report file in `` instead of the root of > - the Git repository. > + Place the resulting bug report file in `` instead of the current > + directory. > > -s :: > --suffix :: > > base-commit: 6c40894d2466d4e7fddc047a05116aa9d14712ee ``` ## Junio C Hamano, 2021-09-07 21:39 Subject: Re: [PATCH 1/2] bugreport: avoid duplicating options in usage() Message-ID: URL: https://gitlist.dev/e/xmqq35qg9iao.fsf%40gitster.g In-Reply-To: <20210904021231.88534-2-carenas@gmail.com> ``` Carlo Marcelo Arenas Belón 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 ] [-s|--suffix ]"), > + N_("git bugreport"), I do not quite see this as an improvement. Without this change, the user will see usage: git bugreport [-o ] [-s ] -o ... explanation of what -o does ... -s ... 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 []") 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, ``` ## Junio C Hamano, 2021-09-07 21:56 Subject: Re: [PATCH 2/2] bugreport: slightly better memory management Message-ID: URL: https://gitlist.dev/e/xmqqy28882xg.fsf%40gitster.g In-Reply-To: <20210904021231.88534-3-carenas@gmail.com> ``` Carlo Marcelo Arenas Belón 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 > --- > 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". ```