{"thread":{"id":"56437","subject":"[PATCH] Documentation: fix default directory of git bugreport -o","startedAt":"2021-09-03T12:00:00Z","lastAt":"2021-09-07T21:57:06Z","messageCount":8,"participants":["Bagas Sanjaya","Carlo Marcelo Arenas Belón","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"434631","messageId":"20210903115933.622847-1-bagasdotme@gmail.com","threadId":"56437","inReplyTo":null,"subject":"[PATCH] Documentation: fix default directory of git bugreport -o","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-09-03T11:59:34Z","receivedAt":"2021-09-03T12:00:00Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"git bugreport writes bug report to the current directory by default,\ninstead of repository root.\n\nFix the documentation.\n\nSigned-off-by: Bagas Sanjaya \n---\n Documentation/git-bugreport.txt | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt\nindex 66e88c2e31..d8817bf3ce 100644\n--- a/Documentation/git-bugreport.txt\n+++ b/Documentation/git-bugreport.txt\n@@ -40,8 +40,8 @@ OPTIONS\n -------\n -o ::\n --output-directory ::\n-\tPlace the resulting bug report file in `` instead of the root of\n-\tthe Git repository.\n+\tPlace the resulting bug report file in `` instead of the current\n+\tdirectory.\n \n -s ::\n --suffix ::\n\nbase-commit: 6c40894d2466d4e7fddc047a05116aa9d14712ee\n-- \n2.25.1\n\n"},{"id":"434673","messageId":"20210904021231.88534-1-carenas@gmail.com","threadId":"56437","inReplyTo":"20210903115933.622847-1-bagasdotme@gmail.com","subject":"bugreport papercuts","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-04T02:12:29Z","receivedAt":"2021-09-04T02:13:23Z","isPatch":false,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"While reviewing this patch, noticed the following other minor issues\nas well:\n\n [PATCH 1/2] bugreport: avoid duplicating options in usage()\n [PATCH 2/2] bugreport: slightly better memory management\n\nMaybe could we join them all in one single \"papercuts\" thread for\neasy of management?\n\nCarlo\n\n"},{"id":"434674","messageId":"20210904021231.88534-2-carenas@gmail.com","threadId":"56437","inReplyTo":"20210904021231.88534-1-carenas@gmail.com","subject":"[PATCH 1/2] bugreport: avoid duplicating options in usage()","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-04T02:12:30Z","receivedAt":"2021-09-04T02:13:39Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"238b439d69 (bugreport: add tool to generate debugging info, 2020-04-16)\nincludes the options with the commandline, which then means they will\nbe duplicated in the output of `git bugreport -h`.\n\nremove them and while at it, make sure usage() is called if the wrong\nnumber of parameters is provided (ex: `git bugreport help`)\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n builtin/bugreport.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/bugreport.c b/builtin/bugreport.c\nindex 9915a5841d..17042381c3 100644\n--- a/builtin/bugreport.c\n+++ b/builtin/bugreport.c\n@@ -5,7 +5,6 @@\n #include \"compat/compiler.h\"\n #include \"run-command.h\"\n \n-\n static void get_system_info(struct strbuf *sys_info)\n {\n \tstruct utsname uname_info;\n@@ -87,7 +86,7 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)\n }\n \n static const char * const bugreport_usage[] = {\n-\tN_(\"git bugreport [-o|--output-directory ] [-s|--suffix ]\"),\n+\tN_(\"git bugreport\"),\n \tNULL\n };\n \n@@ -141,6 +140,8 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix, bugreport_options,\n \t\t\t bugreport_usage, 0);\n+\tif (argc)\n+\t\tusage_with_options(bugreport_usage, bugreport_options);\n \n \t/* Prepare the path to put the result */\n \tprefixed_filename = prefix_filename(prefix,\n-- \n2.33.0.481.g26d3bed244\n\n"},{"id":"434675","messageId":"20210904021231.88534-3-carenas@gmail.com","threadId":"56437","inReplyTo":"20210904021231.88534-1-carenas@gmail.com","subject":"[PATCH 2/2] bugreport: slightly better memory management","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-04T02:12:31Z","receivedAt":"2021-09-04T02:13:59Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"238b439d69 (bugreport: add tool to generate debugging info, 2020-04-16)\nintroduces an UNLEAK for a strbuf that contains the buffer that gets\nflushed to disk earlier, instead of simply cleaning the buffer.\n\ndo so, and while at it, move the free() call for another temporary string\ncloser to its creator, so it is easier to keep track of.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n builtin/bugreport.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/bugreport.c b/builtin/bugreport.c\nindex 17042381c3..a9bedde1e8 100644\n--- a/builtin/bugreport.c\n+++ b/builtin/bugreport.c\n@@ -152,6 +152,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \tstrbuf_addstr(&report_path, \"git-bugreport-\");\n \tstrbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);\n \tstrbuf_addstr(&report_path, \".txt\");\n+\tfree(prefixed_filename);\n \n \tswitch (safe_create_leading_directories(report_path.buf)) {\n \tcase SCLD_OK:\n@@ -181,6 +182,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \t\tdie_errno(_(\"unable to write to %s\"), report_path.buf);\n \n \tclose(report);\n+\tstrbuf_release(&buffer);\n \n \t/*\n \t * We want to print the path relative to the user, but we still need the\n@@ -191,8 +193,6 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \tfprintf(stderr, _(\"Created new report at '%s'.\\n\"),\n \t\tuser_relative_path);\n \n-\tfree(prefixed_filename);\n-\tUNLEAK(buffer);\n \tUNLEAK(report_path);\n \treturn !!launch_editor(report_path.buf, NULL, NULL);\n }\n-- \n2.33.0.481.g26d3bed244\n\n"},{"id":"434677","messageId":"b932088f-9ae0-63ee-fc13-fdb11f17c666@gmail.com","threadId":"56437","inReplyTo":"20210904021231.88534-1-carenas@gmail.com","subject":"Re: bugreport papercuts","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-09-04T06:01:50Z","receivedAt":"2021-09-04T06:02:01Z","isPatch":false,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 04/09/21 09.12, Carlo Marcelo Arenas Belón wrote:\n> While reviewing this patch, noticed the following other minor issues\n> as well:\n> \n> [PATCH 1/2] bugreport: avoid duplicating options in usage()\n> [PATCH 2/2] bugreport: slightly better memory management\n> \n> Maybe could we join them all in one single \"papercuts\" thread for\n> easy of management?\n\nOK, please add [1].\n\n[1]: \nhttps://lore.kernel.org/git/20210903115933.622847-1-bagasdotme@gmail.com/\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"434948","messageId":"xmqqa6ko9jol.fsf@gitster.g","threadId":"56437","inReplyTo":"20210903115933.622847-1-bagasdotme@gmail.com","subject":"Re: [PATCH] Documentation: fix default directory of git bugreport -o","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-07T21:09:46Z","receivedAt":"2021-09-07T21:09:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bagas Sanjaya writes:\n\n> git bugreport writes bug report to the current directory by default,\n> instead of repository root.\n>\n> Fix the documentation.\n>\n> Signed-off-by: Bagas Sanjaya \n> ---\n> Documentation/git-bugreport.txt | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n\nEmily, ack? It seems that after we cdup to the top, we adjust\neither the value of -o or \"\" with the prefix to come up with the\noutput location, so the default seems to be \"the current\" as the\nupdated documentation says.\n\n\n> diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt\n> index 66e88c2e31..d8817bf3ce 100644\n> --- a/Documentation/git-bugreport.txt\n> +++ b/Documentation/git-bugreport.txt\n> @@ -40,8 +40,8 @@ OPTIONS\n> -------\n> -o ::\n> --output-directory ::\n> -\tPlace the resulting bug report file in `` instead of the root of\n> -\tthe Git repository.\n> +\tPlace the resulting bug report file in `` instead of the current\n> +\tdirectory.\n> \n> -s ::\n> --suffix ::\n>\n> base-commit: 6c40894d2466d4e7fddc047a05116aa9d14712ee\n"},{"id":"434959","messageId":"xmqq35qg9iao.fsf@gitster.g","threadId":"56437","inReplyTo":"20210904021231.88534-2-carenas@gmail.com","subject":"Re: [PATCH 1/2] bugreport: avoid duplicating options in usage()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-07T21:39:43Z","receivedAt":"2021-09-07T21:39:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> 238b439d69 (bugreport: add tool to generate debugging info, 2020-04-16)\n> includes the options with the commandline, which then means they will\n> be duplicated in the output of `git bugreport -h`.\n>\n> remove them and while at it, make sure usage() is called if the wrong\n> number of parameters is provided (ex: `git bugreport help`)\n\n'remove' -> 'Remove'.\n\n> static const char * const bugreport_usage[] = {\n> -\tN_(\"git bugreport [-o|--output-directory ] [-s|--suffix ]\"),\n> +\tN_(\"git bugreport\"),\n\nI do not quite see this as an improvement. Without this change, the\nuser will see\n\n\tusage: git bugreport [-o ] [-s ]\n\n\t -o \n\t\t... explanation of what -o does ...\n\t -s \n\t\t... explanation of what -s does ...\n\nand with the patch, it becomes unclear, especially for those who are\nnot used to \"git subcommand -h\" output convention, as we'd see only\n\n\tusage: git bugreport\n\non the first line, no? If the patch is to use\n\n\tN_(\"git bugreport []\")\n\nas the new text, then that would be an improvement, though.\n\n> @@ -141,6 +140,8 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n> \n> \targc = parse_options(argc, argv, prefix, bugreport_options,\n> \t\t\t bugreport_usage, 0);\n> +\tif (argc)\n> +\t\tusage_with_options(bugreport_usage, bugreport_options);\n\nThis is a good change (until we gain positional argument to the\nsubcommand, at which time we'd need to rethink the error checking).\n\n> \t/* Prepare the path to put the result */\n> \tprefixed_filename = prefix_filename(prefix,\n"},{"id":"434961","messageId":"xmqqy28882xg.fsf@gitster.g","threadId":"56437","inReplyTo":"20210904021231.88534-3-carenas@gmail.com","subject":"Re: [PATCH 2/2] bugreport: slightly better memory management","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-07T21:56:59Z","receivedAt":"2021-09-07T21:57:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> 238b439d69 (bugreport: add tool to generate debugging info, 2020-04-16)\n> introduces an UNLEAK for a strbuf that contains the buffer that gets\n> flushed to disk earlier, instead of simply cleaning the buffer.\n>\n> do so, and while at it, move the free() call for another temporary string\n> closer to its creator, so it is easier to keep track of.\n\n'do so' -> 'Do so'.\n\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> builtin/bugreport.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/bugreport.c b/builtin/bugreport.c\n> index 17042381c3..a9bedde1e8 100644\n> --- a/builtin/bugreport.c\n> +++ b/builtin/bugreport.c\n> @@ -152,6 +152,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n> \tstrbuf_addstr(&report_path, \"git-bugreport-\");\n> \tstrbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);\n> \tstrbuf_addstr(&report_path, \".txt\");\n> +\tfree(prefixed_filename);\n\nCorrect, but it can be raised even further. We can free it after we\naddstr to report_path.\n\nI looked at the existing callers of prefix_filename(), hoping that\nmany of them might take it in a strbuf they have, in which case we\nmay be able to expose an alternative interface to take a caller\nsupplied strbuf to clean this one up. But it seems this is the only\none, so \"store in a temporary, strbuf_addstr it, and immediately\nfree the temporary\" here would be good.\n\n> \tswitch (safe_create_leading_directories(report_path.buf)) {\n> \tcase SCLD_OK:\n> @@ -181,6 +182,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n> \t\tdie_errno(_(\"unable to write to %s\"), report_path.buf);\n> \n> \tclose(report);\n> +\tstrbuf_release(&buffer);\n\nWe are done with the strbuf once write_in_full() returns, but this\nis close enough.\n\n> @@ -191,8 +193,6 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n> \tfprintf(stderr, _(\"Created new report at '%s'.\\n\"),\n> \t\tuser_relative_path);\n> \n> -\tfree(prefixed_filename);\n> -\tUNLEAK(buffer);\n> \tUNLEAK(report_path);\n> \treturn !!launch_editor(report_path.buf, NULL, NULL);\n\nHaving reviewed all, I am not sure if my reaction is \"good, now we\nare cleaner\" or \"meh, for the same reason why report_path can be\nleft alive, it is fine to leave buffer alive, too\".\n\n"}]}