* [PATCH v3 1/9] bugreport: add tool to generate debugging info
2019-10-25 2:51 ` [PATCH v3 0/9] add git-bugreport tool Emily Shaffer
@ 2019-10-25 2:51 ` Emily Shaffer
2019-10-29 20:29 ` Josh Steadmon
2019-11-16 3:11 ` Junio C Hamano
2019-10-25 2:51 ` [PATCH v3 2/9] bugreport: generate config whitelist based on docs Emily Shaffer
` (8 subsequent siblings)
9 siblings, 2 replies; 68+ messages in thread
From: Emily Shaffer @ 2019-10-25 2:51 UTC (permalink / raw)
To: git; +Cc: Emily Shaffer
Teach Git how to prompt the user for a good bug report: reproduction
steps, expected behavior, and actual behavior. Later, Git can learn how
to collect some diagnostic information from the repository.
If users can send us a well-written bug report which contains diagnostic
information we would otherwise need to ask the user for, we can reduce
the number of question-and-answer round trips between the reporter and
the Git contributor.
Users may also wish to send a report like this to their local "Git
expert" if they have put their repository into a state they are confused
by.
Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
Makefile | 1 +
builtin.h | 1 +
builtin/bugreport.c | 50 +++++++++++++++++++++++++++++++++++++++++++++
git.c | 1 +
4 files changed, 53 insertions(+)
create mode 100644 builtin/bugreport.c
diff --git a/Makefile b/Makefile
index 58b92af54b..132e2a52da 100644
--- a/Makefile
+++ b/Makefile
@@ -1039,6 +1039,7 @@ BUILTIN_OBJS += builtin/archive.o
BUILTIN_OBJS += builtin/bisect--helper.o
BUILTIN_OBJS += builtin/blame.o
BUILTIN_OBJS += builtin/branch.o
+BUILTIN_OBJS += builtin/bugreport.o
BUILTIN_OBJS += builtin/bundle.o
BUILTIN_OBJS += builtin/cat-file.o
BUILTIN_OBJS += builtin/check-attr.o
diff --git a/builtin.h b/builtin.h
index 5cf5df69f7..c6373d3289 100644
--- a/builtin.h
+++ b/builtin.h
@@ -135,6 +135,7 @@ int cmd_archive(int argc, const char **argv, const char *prefix);
int cmd_bisect__helper(int argc, const char **argv, const char *prefix);
int cmd_blame(int argc, const char **argv, const char *prefix);
int cmd_branch(int argc, const char **argv, const char *prefix);
+int cmd_bugreport(int argc, const char **argv, const char *prefix);
int cmd_bundle(int argc, const char **argv, const char *prefix);
int cmd_cat_file(int argc, const char **argv, const char *prefix);
int cmd_checkout(int argc, const char **argv, const char *prefix);
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
new file mode 100644
index 0000000000..2ef16440a0
--- /dev/null
+++ b/builtin/bugreport.c
@@ -0,0 +1,50 @@
+#include "builtin.h"
+#include "stdio.h"
+#include "strbuf.h"
+#include "time.h"
+
+int get_bug_template(struct strbuf *template)
+{
+ const char template_text[] =
+"Thank you for filling out a Git bug report!\n"
+"Please answer the following questions to help us understand your issue.\n"
+"\n"
+"What did you do before the bug happened? (Steps to reproduce your issue)\n"
+"\n"
+"What did you expect to happen? (Expected behavior)\n"
+"\n"
+"What happened instead? (Actual behavior)\n"
+"\n"
+"What's different between what you expected and what actually happened?\n"
+"\n"
+"Anything else you want to add:\n"
+"\n"
+"Please review the rest of the bug report below.\n"
+"You can delete any lines you don't wish to send.\n";
+
+ strbuf_reset(template);
+ strbuf_add(template, template_text, strlen(template_text));
+ return 0;
+}
+
+int cmd_bugreport(int argc, const char **argv, const char *prefix)
+{
+ struct strbuf buffer = STRBUF_INIT;
+ struct strbuf report_path = STRBUF_INIT;
+ FILE *report;
+ time_t now = time(NULL);
+
+ strbuf_addstr(&report_path, "git-bugreport-");
+ strbuf_addftime(&report_path, "%F", gmtime(&now), 0, 0);
+ strbuf_addstr(&report_path, ".txt");
+
+ report = fopen_for_writing(report_path.buf);
+
+ get_bug_template(&buffer);
+ strbuf_write(&buffer, report);
+
+ fclose(report);
+
+ launch_editor(report_path.buf, NULL, NULL);
+ return 0;
+}
diff --git a/git.c b/git.c
index ce6ab0ece2..2d6a64f019 100644
--- a/git.c
+++ b/git.c
@@ -473,6 +473,7 @@ static struct cmd_struct commands[] = {
{ "bisect--helper", cmd_bisect__helper, RUN_SETUP },
{ "blame", cmd_blame, RUN_SETUP },
{ "branch", cmd_branch, RUN_SETUP | DELAY_PAGER_CONFIG },
+ { "bugreport", cmd_bugreport, RUN_SETUP },
{ "bundle", cmd_bundle, RUN_SETUP_GENTLY | NO_PARSEOPT },
{ "cat-file", cmd_cat_file, RUN_SETUP },
{ "check-attr", cmd_check_attr, RUN_SETUP },
--
2.24.0.rc0.303.g954a862665-goog
^ permalink raw reply related [flat|nested] 68+ messages in thread* Re: [PATCH v3 1/9] bugreport: add tool to generate debugging info
2019-10-25 2:51 ` [PATCH v3 1/9] bugreport: add tool to generate debugging info Emily Shaffer
@ 2019-10-29 20:29 ` Josh Steadmon
2019-11-16 3:11 ` Junio C Hamano
1 sibling, 0 replies; 68+ messages in thread
From: Josh Steadmon @ 2019-10-29 20:29 UTC (permalink / raw)
To: Emily Shaffer; +Cc: git
On 2019.10.24 19:51, Emily Shaffer wrote:
> Teach Git how to prompt the user for a good bug report: reproduction
> steps, expected behavior, and actual behavior. Later, Git can learn how
> to collect some diagnostic information from the repository.
>
> If users can send us a well-written bug report which contains diagnostic
> information we would otherwise need to ask the user for, we can reduce
> the number of question-and-answer round trips between the reporter and
> the Git contributor.
>
> Users may also wish to send a report like this to their local "Git
> expert" if they have put their repository into a state they are confused
> by.
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
> Makefile | 1 +
> builtin.h | 1 +
> builtin/bugreport.c | 50 +++++++++++++++++++++++++++++++++++++++++++++
> git.c | 1 +
> 4 files changed, 53 insertions(+)
> create mode 100644 builtin/bugreport.c
>
> diff --git a/Makefile b/Makefile
> index 58b92af54b..132e2a52da 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1039,6 +1039,7 @@ BUILTIN_OBJS += builtin/archive.o
> BUILTIN_OBJS += builtin/bisect--helper.o
> BUILTIN_OBJS += builtin/blame.o
> BUILTIN_OBJS += builtin/branch.o
> +BUILTIN_OBJS += builtin/bugreport.o
> BUILTIN_OBJS += builtin/bundle.o
> BUILTIN_OBJS += builtin/cat-file.o
> BUILTIN_OBJS += builtin/check-attr.o
> diff --git a/builtin.h b/builtin.h
> index 5cf5df69f7..c6373d3289 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -135,6 +135,7 @@ int cmd_archive(int argc, const char **argv, const char *prefix);
> int cmd_bisect__helper(int argc, const char **argv, const char *prefix);
> int cmd_blame(int argc, const char **argv, const char *prefix);
> int cmd_branch(int argc, const char **argv, const char *prefix);
> +int cmd_bugreport(int argc, const char **argv, const char *prefix);
> int cmd_bundle(int argc, const char **argv, const char *prefix);
> int cmd_cat_file(int argc, const char **argv, const char *prefix);
> int cmd_checkout(int argc, const char **argv, const char *prefix);
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> new file mode 100644
> index 0000000000..2ef16440a0
> --- /dev/null
> +++ b/builtin/bugreport.c
> @@ -0,0 +1,50 @@
> +#include "builtin.h"
> +#include "stdio.h"
> +#include "strbuf.h"
> +#include "time.h"
> +
> +int get_bug_template(struct strbuf *template)
Compilation fails here for me with:
builtin/bugreport.c:6:5: error: no previous prototype
for ‘get_bug_template’ [-Werror=missing-prototypes]
Can you make this function static?
> +{
> + const char template_text[] =
> +"Thank you for filling out a Git bug report!\n"
> +"Please answer the following questions to help us understand your issue.\n"
> +"\n"
> +"What did you do before the bug happened? (Steps to reproduce your issue)\n"
> +"\n"
> +"What did you expect to happen? (Expected behavior)\n"
> +"\n"
> +"What happened instead? (Actual behavior)\n"
> +"\n"
> +"What's different between what you expected and what actually happened?\n"
> +"\n"
> +"Anything else you want to add:\n"
> +"\n"
> +"Please review the rest of the bug report below.\n"
> +"You can delete any lines you don't wish to send.\n";
> +
> + strbuf_reset(template);
> + strbuf_add(template, template_text, strlen(template_text));
> + return 0;
> +}
> +
> +int cmd_bugreport(int argc, const char **argv, const char *prefix)
> +{
> + struct strbuf buffer = STRBUF_INIT;
> + struct strbuf report_path = STRBUF_INIT;
> + FILE *report;
> + time_t now = time(NULL);
> +
> + strbuf_addstr(&report_path, "git-bugreport-");
> + strbuf_addftime(&report_path, "%F", gmtime(&now), 0, 0);
> + strbuf_addstr(&report_path, ".txt");
> +
> + report = fopen_for_writing(report_path.buf);
> +
> + get_bug_template(&buffer);
> + strbuf_write(&buffer, report);
> +
> + fclose(report);
> +
> + launch_editor(report_path.buf, NULL, NULL);
> + return 0;
> +}
> diff --git a/git.c b/git.c
> index ce6ab0ece2..2d6a64f019 100644
> --- a/git.c
> +++ b/git.c
> @@ -473,6 +473,7 @@ static struct cmd_struct commands[] = {
> { "bisect--helper", cmd_bisect__helper, RUN_SETUP },
> { "blame", cmd_blame, RUN_SETUP },
> { "branch", cmd_branch, RUN_SETUP | DELAY_PAGER_CONFIG },
> + { "bugreport", cmd_bugreport, RUN_SETUP },
> { "bundle", cmd_bundle, RUN_SETUP_GENTLY | NO_PARSEOPT },
> { "cat-file", cmd_cat_file, RUN_SETUP },
> { "check-attr", cmd_check_attr, RUN_SETUP },
> --
> 2.24.0.rc0.303.g954a862665-goog
Can you also add /git-bugreport to .gitignore?
^ permalink raw reply [flat|nested] 68+ messages in thread* Re: [PATCH v3 1/9] bugreport: add tool to generate debugging info
2019-10-25 2:51 ` [PATCH v3 1/9] bugreport: add tool to generate debugging info Emily Shaffer
2019-10-29 20:29 ` Josh Steadmon
@ 2019-11-16 3:11 ` Junio C Hamano
2019-11-19 20:25 ` Emily Shaffer
1 sibling, 1 reply; 68+ messages in thread
From: Junio C Hamano @ 2019-11-16 3:11 UTC (permalink / raw)
To: Emily Shaffer; +Cc: git
Emily Shaffer <emilyshaffer@google.com> writes:
> Teach Git how to prompt the user for a good bug report: reproduction
> steps, expected behavior, and actual behavior. Later, Git can learn how
> to collect some diagnostic information from the repository.
It makes sense, but I do not think of any good reason why this
should be implemented as a builtin. I'd expect it would probably
need to collect more info on the running environment than otherwise
necessary for the regular Git operation, and perhaps you'd want to
even link with libraries that are not needed for the regular Git
operation to achieve that.
Can you make it a standalone binary instead to avoid bloat of the
main "git" binary?
Thanks.
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v3 1/9] bugreport: add tool to generate debugging info
2019-11-16 3:11 ` Junio C Hamano
@ 2019-11-19 20:25 ` Emily Shaffer
2019-11-19 23:24 ` Johannes Schindelin
` (2 more replies)
0 siblings, 3 replies; 68+ messages in thread
From: Emily Shaffer @ 2019-11-19 20:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Sat, Nov 16, 2019 at 12:11:38PM +0900, Junio C Hamano wrote:
> Emily Shaffer <emilyshaffer@google.com> writes:
>
> > Teach Git how to prompt the user for a good bug report: reproduction
> > steps, expected behavior, and actual behavior. Later, Git can learn how
> > to collect some diagnostic information from the repository.
>
> It makes sense, but I do not think of any good reason why this
> should be implemented as a builtin. I'd expect it would probably
> need to collect more info on the running environment than otherwise
> necessary for the regular Git operation, and perhaps you'd want to
> even link with libraries that are not needed for the regular Git
> operation to achieve that.
>
> Can you make it a standalone binary instead to avoid bloat of the
> main "git" binary?
Sure. This would fix some other issues (needing to link against curl to
get the curl version, which is exactly what you implied). I wasn't
certain which circumstances a standalone binary was preferred, but I
agree with your reasoning here for sure.
- Emily
^ permalink raw reply [flat|nested] 68+ messages in thread* Re: [PATCH v3 1/9] bugreport: add tool to generate debugging info
2019-11-19 20:25 ` Emily Shaffer
@ 2019-11-19 23:24 ` Johannes Schindelin
2019-11-20 0:37 ` Junio C Hamano
2019-11-19 23:31 ` Johannes Schindelin
2019-11-20 0:32 ` Junio C Hamano
2 siblings, 1 reply; 68+ messages in thread
From: Johannes Schindelin @ 2019-11-19 23:24 UTC (permalink / raw)
To: Emily Shaffer; +Cc: Junio C Hamano, git
Hi,
On Tue, 19 Nov 2019, Emily Shaffer wrote:
> On Sat, Nov 16, 2019 at 12:11:38PM +0900, Junio C Hamano wrote:
> > Emily Shaffer <emilyshaffer@google.com> writes:
> >
> > > Teach Git how to prompt the user for a good bug report: reproduction
> > > steps, expected behavior, and actual behavior. Later, Git can learn how
> > > to collect some diagnostic information from the repository.
> >
> > It makes sense, but I do not think of any good reason why this
> > should be implemented as a builtin. I'd expect it would probably
> > need to collect more info on the running environment than otherwise
> > necessary for the regular Git operation, and perhaps you'd want to
> > even link with libraries that are not needed for the regular Git
> > operation to achieve that.
> >
> > Can you make it a standalone binary instead to avoid bloat of the
> > main "git" binary?
>
> Sure. This would fix some other issues (needing to link against curl to
> get the curl version, which is exactly what you implied). I wasn't
> certain which circumstances a standalone binary was preferred, but I
> agree with your reasoning here for sure.
FWIW I disagree with the idea that a tiny built-in command like
`bugreport` would "bloat" the main `git` binary.
In contrast, I think that stand-alone commands _do_ bloat. Look here:
$ ls -lh git-daemon.exe git-credential-store.exe
-rwxr-xr-x 1 me 4096 1.8M Nov 6 13:43 git-credential-store.exe*
-rwxr-xr-x 1 me 4096 1.8M Nov 6 13:43 git-daemon.exe*
In other words, even a super simple stand-alone like `credential-store`
(the `credential-store.c` file has only 198 lines!) weighs in with almost
two megabytes.
So I fear that the claim that a stand-alone command would add less bloat
than a built-in one, especially for a relatively small thing like
`bugreport` has it exactly backwards.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v3 1/9] bugreport: add tool to generate debugging info
2019-11-19 23:24 ` Johannes Schindelin
@ 2019-11-20 0:37 ` Junio C Hamano
2019-11-20 10:51 ` Johannes Schindelin
0 siblings, 1 reply; 68+ messages in thread
From: Junio C Hamano @ 2019-11-20 0:37 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Emily Shaffer, git
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> FWIW I disagree with the idea that a tiny built-in command like
> `bugreport` would "bloat" the main `git` binary.
>
> In contrast, I think that stand-alone commands _do_ bloat. Look here:
I probably should have clarified that "bloat" in the context of this
discussion is not about on-disk space. It is about resources needed
to run "git status/diff/etc" that are everyday local commands that
are expected to be lightweight, i.e. the same criteria applied when
making the networking part (which the user expects to be coffee time)
separate from them.
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v3 1/9] bugreport: add tool to generate debugging info
2019-11-20 0:37 ` Junio C Hamano
@ 2019-11-20 10:51 ` Johannes Schindelin
0 siblings, 0 replies; 68+ messages in thread
From: Johannes Schindelin @ 2019-11-20 10:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Emily Shaffer, git
Hi Junio,
On Wed, 20 Nov 2019, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > FWIW I disagree with the idea that a tiny built-in command like
> > `bugreport` would "bloat" the main `git` binary.
> >
> > In contrast, I think that stand-alone commands _do_ bloat. Look here:
>
> I probably should have clarified that "bloat" in the context of this
> discussion is not about on-disk space. It is about resources needed
> to run "git status/diff/etc" that are everyday local commands that
> are expected to be lightweight, i.e. the same criteria applied when
> making the networking part (which the user expects to be coffee time)
> separate from them.
I guess I still do not understand.
Are you suggesting that `bugreport` would be unwelcome as a built-in? If
so, I would really like to know why. Because I think it would make for a
very fine built-in. I interact with users all the time, and a good tool to
provide good information to accompany a bug report is definitely something
that would improve the current situation.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v3 1/9] bugreport: add tool to generate debugging info
2019-11-19 20:25 ` Emily Shaffer
2019-11-19 23:24 ` Johannes Schindelin
@ 2019-11-19 23:31 ` Johannes Schindelin
2019-11-20 0:39 ` Junio C Hamano
2019-11-20 2:09 ` Emily Shaffer
2019-11-20 0:32 ` Junio C Hamano
2 siblings, 2 replies; 68+ messages in thread
From: Johannes Schindelin @ 2019-11-19 23:31 UTC (permalink / raw)
To: Emily Shaffer; +Cc: Junio C Hamano, git
Hi Emily,
On Tue, 19 Nov 2019, Emily Shaffer wrote:
> [...] some other issues (needing to link against curl to get the curl
> version, which is exactly what you implied) [...]
I did suggest on IRC to teach `git-remote-https` an option where it prints
the cURL version (and build options) and exits.
This would have the further advantage of making sure that the correct
information is included. If you have two separate binaries that both link
to cURL, they could still be linked statically, to different versions of
cURL (this could happen e.g. if you have a `git-remote-https` from a
different build in your path).
In short: I still think that it would make for a much better idea to query
the `git-remote-https` executable for the version information than to link
`bugreport` to libcurl.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v3 1/9] bugreport: add tool to generate debugging info
2019-11-19 23:31 ` Johannes Schindelin
@ 2019-11-20 0:39 ` Junio C Hamano
2019-11-20 2:09 ` Emily Shaffer
1 sibling, 0 replies; 68+ messages in thread
From: Junio C Hamano @ 2019-11-20 0:39 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Emily Shaffer, git
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> I did suggest on IRC to teach `git-remote-https` an option where it prints
> the cURL version (and build options) and exits.
I like that. You ask the exact binary what (it thinks) it uses, so
that there won't be skew between the view by "git remote-http" and
"git bugreport" on the world.
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v3 1/9] bugreport: add tool to generate debugging info
2019-11-19 23:31 ` Johannes Schindelin
2019-11-20 0:39 ` Junio C Hamano
@ 2019-11-20 2:09 ` Emily Shaffer
1 sibling, 0 replies; 68+ messages in thread
From: Emily Shaffer @ 2019-11-20 2:09 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git
On Wed, Nov 20, 2019 at 12:31:42AM +0100, Johannes Schindelin wrote:
> Hi Emily,
>
> On Tue, 19 Nov 2019, Emily Shaffer wrote:
>
> > [...] some other issues (needing to link against curl to get the curl
> > version, which is exactly what you implied) [...]
>
> I did suggest on IRC to teach `git-remote-https` an option where it prints
> the cURL version (and build options) and exits.
>
> This would have the further advantage of making sure that the correct
> information is included. If you have two separate binaries that both link
> to cURL, they could still be linked statically, to different versions of
> cURL (this could happen e.g. if you have a `git-remote-https` from a
> different build in your path).
Yeah, it's a good point and an angle I hadn't thought of. Thanks.
> In short: I still think that it would make for a much better idea to query
> the `git-remote-https` executable for the version information than to link
> `bugreport` to libcurl.
Will do - the git-bugreport standalone will invoke the git-remote-https
standalone to ask for version info.
- Emily
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v3 1/9] bugreport: add tool to generate debugging info
2019-11-19 20:25 ` Emily Shaffer
2019-11-19 23:24 ` Johannes Schindelin
2019-11-19 23:31 ` Johannes Schindelin
@ 2019-11-20 0:32 ` Junio C Hamano
2 siblings, 0 replies; 68+ messages in thread
From: Junio C Hamano @ 2019-11-20 0:32 UTC (permalink / raw)
To: Emily Shaffer; +Cc: git
Emily Shaffer <emilyshaffer@google.com> writes:
>> Can you make it a standalone binary instead to avoid bloat of the
>> main "git" binary?
>
> Sure. This would fix some other issues (needing to link against curl to
> get the curl version, which is exactly what you implied).
Hmph, I actually was not thinking about the cURL library.
I imagined that your tool may need to learn more about the system in
order to make the report and for that there may be libraries that
makes it easy than say reading directly from the /proc filesystem
etc. that you may end up using. Unlike cURL, such a library would
have no use in the rest of Git anywhere.
^ permalink raw reply [flat|nested] 68+ messages in thread
* [PATCH v3 2/9] bugreport: generate config whitelist based on docs
2019-10-25 2:51 ` [PATCH v3 0/9] add git-bugreport tool Emily Shaffer
2019-10-25 2:51 ` [PATCH v3 1/9] bugreport: add tool to generate debugging info Emily Shaffer
@ 2019-10-25 2:51 ` Emily Shaffer
2019-10-28 13:27 ` Johannes Schindelin
2019-10-25 2:51 ` [PATCH v3 3/9] bugreport: add version and system information Emily Shaffer
` (7 subsequent siblings)
9 siblings, 1 reply; 68+ messages in thread
From: Emily Shaffer @ 2019-10-25 2:51 UTC (permalink / raw)
To: git; +Cc: Emily Shaffer
Add a new step to the build to generate a whitelist of git-config
variables which are appropriate to include in the output of
git-bugreport. New variables can be added to the whitelist by annotating
their documentation in Documentation/config with the line
"// bugreport-include".
Some configs are private in nature, and can contain remote URLs,
passwords, or other sensitive information. In the event that a user
doesn't notice their information while reviewing a bugreport, that user
may leak their credentials to other individuals, mailing lists, or bug
tracking tools inadvertently. Heuristic blacklisting of configuration
keys is imperfect and prone to false negatives; given the nature of the
information which can be leaked, a whitelist is more reliable.
In order to prevent staleness of the whitelist, add a mechanism to
generate the whitelist from annotations in the config documentation,
where contributors are already used to documenting their new config
keys.
Additionally, add annotations to the sendemail config documentation in
order to demonstrate a proof of concept.
Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
.gitignore | 1 +
Documentation/config/sendemail.txt | 68 +++++++++++++-------------
Makefile | 9 +++-
bugreport-generate-config-whitelist.sh | 4 ++
4 files changed, 47 insertions(+), 35 deletions(-)
create mode 100755 bugreport-generate-config-whitelist.sh
diff --git a/.gitignore b/.gitignore
index 89b3b79c1a..055a84c4a8 100644
--- a/.gitignore
+++ b/.gitignore
@@ -25,6 +25,7 @@
/git-bisect--helper
/git-blame
/git-branch
+/git-bugreport
/git-bundle
/git-cat-file
/git-check-attr
diff --git a/Documentation/config/sendemail.txt b/Documentation/config/sendemail.txt
index 0006faf800..69f3e4f219 100644
--- a/Documentation/config/sendemail.txt
+++ b/Documentation/config/sendemail.txt
@@ -1,63 +1,63 @@
-sendemail.identity::
+sendemail.identity:: // bugreport-exclude
A configuration identity. When given, causes values in the
'sendemail.<identity>' subsection to take precedence over
values in the 'sendemail' section. The default identity is
the value of `sendemail.identity`.
-sendemail.smtpEncryption::
+sendemail.smtpEncryption:: // bugreport-include
See linkgit:git-send-email[1] for description. Note that this
setting is not subject to the 'identity' mechanism.
-sendemail.smtpssl (deprecated)::
+sendemail.smtpssl (deprecated):: // bugreport-exclude
Deprecated alias for 'sendemail.smtpEncryption = ssl'.
-sendemail.smtpsslcertpath::
+sendemail.smtpsslcertpath:: // bugreport-exclude
Path to ca-certificates (either a directory or a single file).
Set it to an empty string to disable certificate verification.
-sendemail.<identity>.*::
+sendemail.<identity>.*:: // bugreport-exclude
Identity-specific versions of the 'sendemail.*' parameters
found below, taking precedence over those when this
identity is selected, through either the command-line or
`sendemail.identity`.
-sendemail.aliasesFile::
-sendemail.aliasFileType::
-sendemail.annotate::
-sendemail.bcc::
-sendemail.cc::
-sendemail.ccCmd::
-sendemail.chainReplyTo::
-sendemail.confirm::
-sendemail.envelopeSender::
-sendemail.from::
-sendemail.multiEdit::
-sendemail.signedoffbycc::
-sendemail.smtpPass::
-sendemail.suppresscc::
-sendemail.suppressFrom::
-sendemail.to::
-sendemail.tocmd::
-sendemail.smtpDomain::
-sendemail.smtpServer::
-sendemail.smtpServerPort::
-sendemail.smtpServerOption::
-sendemail.smtpUser::
-sendemail.thread::
-sendemail.transferEncoding::
-sendemail.validate::
-sendemail.xmailer::
+sendemail.aliasesFile:: // bugreport-exclude
+sendemail.aliasFileType:: // bugreport-exclude
+sendemail.annotate:: // bugreport-include
+sendemail.bcc:: // bugreport-include
+sendemail.cc:: // bugreport-include
+sendemail.ccCmd:: // bugreport-include
+sendemail.chainReplyTo:: // bugreport-include
+sendemail.confirm:: // bugreport-include
+sendemail.envelopeSender:: // bugreport-include
+sendemail.from:: // bugreport-include
+sendemail.multiEdit:: // bugreport-include
+sendemail.signedoffbycc:: // bugreport-include
+sendemail.smtpPass:: // bugreport-exclude
+sendemail.suppresscc:: // bugreport-include
+sendemail.suppressFrom:: // bugreport-include
+sendemail.to:: // bugreport-include
+sendemail.tocmd:: // bugreport-include
+sendemail.smtpDomain:: // bugreport-include
+sendemail.smtpServer:: // bugreport-include
+sendemail.smtpServerPort:: // bugreport-include
+sendemail.smtpServerOption:: // bugreport-include
+sendemail.smtpUser:: // bugreport-exclude
+sendemail.thread:: // bugreport-include
+sendemail.transferEncoding:: // bugreport-include
+sendemail.validate:: // bugreport-include
+sendemail.xmailer:: // bugreport-include
See linkgit:git-send-email[1] for description.
-sendemail.signedoffcc (deprecated)::
+sendemail.signedoffcc (deprecated):: // bugreport-exclude
Deprecated alias for `sendemail.signedoffbycc`.
-sendemail.smtpBatchSize::
+sendemail.smtpBatchSize:: // bugreport-include
Number of messages to be sent per connection, after that a relogin
will happen. If the value is 0 or undefined, send all messages in
one connection.
See also the `--batch-size` option of linkgit:git-send-email[1].
-sendemail.smtpReloginDelay::
+sendemail.smtpReloginDelay:: // bugreport-include
Seconds wait before reconnecting to smtp server.
See also the `--relogin-delay` option of linkgit:git-send-email[1].
diff --git a/Makefile b/Makefile
index 132e2a52da..78767ecdde 100644
--- a/Makefile
+++ b/Makefile
@@ -634,6 +634,10 @@ SCRIPT_PYTHON += git-p4.py
SCRIPT_SH_GEN = $(patsubst %.sh,%,$(SCRIPT_SH))
SCRIPT_PERL_GEN = $(patsubst %.perl,%,$(SCRIPT_PERL))
SCRIPT_PYTHON_GEN = $(patsubst %.py,%,$(SCRIPT_PYTHON))
+SCRIPT_DEPENDENCIES = git-bugreport-config-whitelist
+
+$(SCRIPT_DEPENDENCIES): Documentation/config/*.txt
+ sh bugreport-generate-config-whitelist.sh
# Individual rules to allow e.g.
# "make -C ../.. SCRIPT_PERL=contrib/foo/bar.perl build-perl-script"
@@ -651,17 +655,20 @@ install-perl-script: $(SCRIPT_PERL_GEN)
install-python-script: $(SCRIPT_PYTHON_GEN)
$(INSTALL) $^ '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-.PHONY: clean-perl-script clean-sh-script clean-python-script
+.PHONY: clean-perl-script clean-sh-script clean-python-script clean-script-dependencies
clean-sh-script:
$(RM) $(SCRIPT_SH_GEN)
clean-perl-script:
$(RM) $(SCRIPT_PERL_GEN)
clean-python-script:
$(RM) $(SCRIPT_PYTHON_GEN)
+clean-script-dependencies:
+ $(RM) $(SCRIPT_DEPENDENCIES)
SCRIPTS = $(SCRIPT_SH_GEN) \
$(SCRIPT_PERL_GEN) \
$(SCRIPT_PYTHON_GEN) \
+ $(SCRIPT_DEPENDENCIES) \
git-instaweb
ETAGS_TARGET = TAGS
diff --git a/bugreport-generate-config-whitelist.sh b/bugreport-generate-config-whitelist.sh
new file mode 100755
index 0000000000..ca6b232024
--- /dev/null
+++ b/bugreport-generate-config-whitelist.sh
@@ -0,0 +1,4 @@
+#!/bin/sh
+
+grep -RhPo ".*(?=:: \/\/ bugreport-include)" Documentation/config \
+ >git-bugreport-config-whitelist
--
2.24.0.rc0.303.g954a862665-goog
^ permalink raw reply related [flat|nested] 68+ messages in thread* Re: [PATCH v3 2/9] bugreport: generate config whitelist based on docs
2019-10-25 2:51 ` [PATCH v3 2/9] bugreport: generate config whitelist based on docs Emily Shaffer
@ 2019-10-28 13:27 ` Johannes Schindelin
0 siblings, 0 replies; 68+ messages in thread
From: Johannes Schindelin @ 2019-10-28 13:27 UTC (permalink / raw)
To: Emily Shaffer; +Cc: git
Hi Emily,
On Thu, 24 Oct 2019, Emily Shaffer wrote:
> diff --git a/bugreport-generate-config-whitelist.sh b/bugreport-generate-config-whitelist.sh
> new file mode 100755
> index 0000000000..ca6b232024
> --- /dev/null
> +++ b/bugreport-generate-config-whitelist.sh
> @@ -0,0 +1,4 @@
> +#!/bin/sh
> +
> +grep -RhPo ".*(?=:: \/\/ bugreport-include)" Documentation/config \
I am rather certain that `-P` is not supported by BSD grep (see
https://man.openbsd.org/grep).
Why not something portable, e.g.
find Documentation/config -type f -exec cat {} \; |
sed -n 's/^\(.*\):: \/\/ bugreport-include$/\1/p' \
?
Ciao,
Dscho
> + >git-bugreport-config-whitelist
> --
> 2.24.0.rc0.303.g954a862665-goog
>
>
^ permalink raw reply [flat|nested] 68+ messages in thread
* [PATCH v3 3/9] bugreport: add version and system information
2019-10-25 2:51 ` [PATCH v3 0/9] add git-bugreport tool Emily Shaffer
2019-10-25 2:51 ` [PATCH v3 1/9] bugreport: add tool to generate debugging info Emily Shaffer
2019-10-25 2:51 ` [PATCH v3 2/9] bugreport: generate config whitelist based on docs Emily Shaffer
@ 2019-10-25 2:51 ` Emily Shaffer
2019-10-28 13:49 ` Johannes Schindelin
2019-10-29 20:43 ` Josh Steadmon
2019-10-25 2:51 ` [PATCH v3 4/9] bugreport: add config values from whitelist Emily Shaffer
` (6 subsequent siblings)
9 siblings, 2 replies; 68+ messages in thread
From: Emily Shaffer @ 2019-10-25 2:51 UTC (permalink / raw)
To: git; +Cc: Emily Shaffer
Teach bugreport how to collect the Git, curl, and ldd versions, as well
as the uname string.
Learning the uname and versions in the user's environment will enable us
to reproduce behavior reported by the user; the versions will also give
us a good starting place while bisecting a newly introduced bug.
Put this functionality in a library, so that other commands can easily
include this debug info if necessary.
Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
Makefile | 1 +
bugreport.c | 55 +++++++++++++++++++++++++++++++++++++++++++++
bugreport.h | 7 ++++++
builtin/bugreport.c | 13 +++++++++++
4 files changed, 76 insertions(+)
create mode 100644 bugreport.c
create mode 100644 bugreport.h
diff --git a/Makefile b/Makefile
index 78767ecdde..045b22cb67 100644
--- a/Makefile
+++ b/Makefile
@@ -844,6 +844,7 @@ LIB_OBJS += bisect.o
LIB_OBJS += blame.o
LIB_OBJS += blob.o
LIB_OBJS += branch.o
+LIB_OBJS += bugreport.o
LIB_OBJS += bulk-checkin.o
LIB_OBJS += bundle.o
LIB_OBJS += cache-tree.o
diff --git a/bugreport.c b/bugreport.c
new file mode 100644
index 0000000000..ada54fe583
--- /dev/null
+++ b/bugreport.c
@@ -0,0 +1,55 @@
+#include "cache.h"
+
+#include "bugreport.h"
+#include "help.h"
+#include "run-command.h"
+#include "version.h"
+
+void get_system_info(struct strbuf *sys_info)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+ struct strbuf std_out = STRBUF_INIT;
+
+ strbuf_reset(sys_info);
+
+ // get git version from native cmd
+ strbuf_addstr(sys_info, "git version: ");
+ strbuf_addstr(sys_info, git_version_string);
+ strbuf_complete_line(sys_info);
+
+ // system call for other version info
+ argv_array_push(&cp.args, "uname");
+ argv_array_push(&cp.args, "-a");
+ capture_command(&cp, &std_out, 0);
+
+ strbuf_addstr(sys_info, "uname -a: ");
+ strbuf_addbuf(sys_info, &std_out);
+ strbuf_complete_line(sys_info);
+
+ argv_array_clear(&cp.args);
+ strbuf_reset(&std_out);
+
+
+ argv_array_push(&cp.args, "curl-config");
+ argv_array_push(&cp.args, "--version");
+ capture_command(&cp, &std_out, 0);
+
+ strbuf_addstr(sys_info, "curl-config --version: ");
+ strbuf_addbuf(sys_info, &std_out);
+ strbuf_complete_line(sys_info);
+
+ argv_array_clear(&cp.args);
+ strbuf_reset(&std_out);
+
+
+ argv_array_push(&cp.args, "ldd");
+ argv_array_push(&cp.args, "--version");
+ capture_command(&cp, &std_out, 0);
+
+ strbuf_addstr(sys_info, "ldd --version: ");
+ strbuf_addbuf(sys_info, &std_out);
+ strbuf_complete_line(sys_info);
+
+ argv_array_clear(&cp.args);
+ strbuf_reset(&std_out);
+}
diff --git a/bugreport.h b/bugreport.h
new file mode 100644
index 0000000000..ba216acf3f
--- /dev/null
+++ b/bugreport.h
@@ -0,0 +1,7 @@
+#include "strbuf.h"
+
+/**
+ * Adds the Git version, `uname -a`, and `curl-config --version` to sys_info.
+ * The previous contents of sys_info will be discarded.
+ */
+void get_system_info(struct strbuf *sys_info);
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 2ef16440a0..7232d31be7 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -1,4 +1,5 @@
#include "builtin.h"
+#include "bugreport.h"
#include "stdio.h"
#include "strbuf.h"
#include "time.h"
@@ -27,6 +28,13 @@ int get_bug_template(struct strbuf *template)
return 0;
}
+void add_header(FILE *report, const char *title)
+{
+ struct strbuf buffer = STRBUF_INIT;
+ strbuf_addf(&buffer, "\n\n[%s]\n", title);
+ strbuf_write(&buffer, report);
+}
+
int cmd_bugreport(int argc, const char **argv, const char *prefix)
{
struct strbuf buffer = STRBUF_INIT;
@@ -43,6 +51,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
get_bug_template(&buffer);
strbuf_write(&buffer, report);
+ // add other contents
+ add_header(report, "System Info");
+ get_system_info(&buffer);
+ strbuf_write(&buffer, report);
+
fclose(report);
launch_editor(report_path.buf, NULL, NULL);
--
2.24.0.rc0.303.g954a862665-goog
^ permalink raw reply related [flat|nested] 68+ messages in thread* Re: [PATCH v3 3/9] bugreport: add version and system information
2019-10-25 2:51 ` [PATCH v3 3/9] bugreport: add version and system information Emily Shaffer
@ 2019-10-28 13:49 ` Johannes Schindelin
2019-11-08 21:48 ` Emily Shaffer
2019-10-29 20:43 ` Josh Steadmon
1 sibling, 1 reply; 68+ messages in thread
From: Johannes Schindelin @ 2019-10-28 13:49 UTC (permalink / raw)
To: Emily Shaffer; +Cc: git
Hi Emily,
On Thu, 24 Oct 2019, Emily Shaffer wrote:
> diff --git a/bugreport.c b/bugreport.c
> new file mode 100644
> index 0000000000..ada54fe583
> --- /dev/null
> +++ b/bugreport.c
> @@ -0,0 +1,55 @@
> +#include "cache.h"
> +
> +#include "bugreport.h"
> +#include "help.h"
> +#include "run-command.h"
> +#include "version.h"
> +
> +void get_system_info(struct strbuf *sys_info)
> +{
> + struct child_process cp = CHILD_PROCESS_INIT;
> + struct strbuf std_out = STRBUF_INIT;
> +
> + strbuf_reset(sys_info);
> +
> + // get git version from native cmd
Please use C-style comments instead of C++ ones.
> + strbuf_addstr(sys_info, "git version: ");
> + strbuf_addstr(sys_info, git_version_string);
> + strbuf_complete_line(sys_info);
> +
> + // system call for other version info
> + argv_array_push(&cp.args, "uname");
> + argv_array_push(&cp.args, "-a");
> + capture_command(&cp, &std_out, 0);
Mmmkay. I am not too much of a fan of relying on the `uname` executable,
as it can very well be a 32-bit `uname.exe` on Windows, which obviously
would _not_ report the architecture of the machine, but something
misleading.
Why not use the `uname()` function (that we even define in
`compat/mingw.c`)?
Also, why not include the same information as `git version
--build-options`, most importantly `GIT_HOST_CPU`?
In any case, if you are capturing the output of `uname -a`, you
definitely will want to pass the `RUN_COMMAND_STDOUT_TO_STDERR` flag (in
case, say, the MSYS2 `uname.exe` fails with those dreaded Cygwin error
messages like "*** fatal error - add_item
("\??\D:\git\installation\Git", "/", ...) failed, errno 1").
> +
> + strbuf_addstr(sys_info, "uname -a: ");
> + strbuf_addbuf(sys_info, &std_out);
> + strbuf_complete_line(sys_info);
> +
> + argv_array_clear(&cp.args);
> + strbuf_reset(&std_out);
> +
> +
> + argv_array_push(&cp.args, "curl-config");
> + argv_array_push(&cp.args, "--version");
> + capture_command(&cp, &std_out, 0);
This will be almost certainly be incorrect, as most _regular_ users
won't have `curl-config`. I know, it is easy to forget that most Git
users are no hard-core C developers ;-)
A better alternative would be to use `curl_version()`, guarded, of
course, by `#ifndef NO_CURL`...
> +
> + strbuf_addstr(sys_info, "curl-config --version: ");
> + strbuf_addbuf(sys_info, &std_out);
> + strbuf_complete_line(sys_info);
> +
> + argv_array_clear(&cp.args);
> + strbuf_reset(&std_out);
> +
> +
> + argv_array_push(&cp.args, "ldd");
> + argv_array_push(&cp.args, "--version");
> + capture_command(&cp, &std_out, 0);
Again, this command will only be present in few setups. I am not
actually sure that the output of this is interesting to begin with.
What I _do_ think is that a much more interesting piece of information
would be the exact GLIBC version, the OS name and the OS version.
> +
> + strbuf_addstr(sys_info, "ldd --version: ");
> + strbuf_addbuf(sys_info, &std_out);
> + strbuf_complete_line(sys_info);
> +
> + argv_array_clear(&cp.args);
> + strbuf_reset(&std_out);
> +}
> diff --git a/bugreport.h b/bugreport.h
> new file mode 100644
> index 0000000000..ba216acf3f
> --- /dev/null
> +++ b/bugreport.h
> @@ -0,0 +1,7 @@
> +#include "strbuf.h"
> +
> +/**
> + * Adds the Git version, `uname -a`, and `curl-config --version` to sys_info.
> + * The previous contents of sys_info will be discarded.
> + */
> +void get_system_info(struct strbuf *sys_info);
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index 2ef16440a0..7232d31be7 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -1,4 +1,5 @@
> #include "builtin.h"
> +#include "bugreport.h"
> #include "stdio.h"
> #include "strbuf.h"
> #include "time.h"
> @@ -27,6 +28,13 @@ int get_bug_template(struct strbuf *template)
> return 0;
> }
>
> +void add_header(FILE *report, const char *title)
> +{
> + struct strbuf buffer = STRBUF_INIT;
> + strbuf_addf(&buffer, "\n\n[%s]\n", title);
> + strbuf_write(&buffer, report);
This leaks `buffer`. Why not write into `report` via `fprintf()`
directly?
Ciao,
Dscho
> +}
> +
> int cmd_bugreport(int argc, const char **argv, const char *prefix)
> {
> struct strbuf buffer = STRBUF_INIT;
> @@ -43,6 +51,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> get_bug_template(&buffer);
> strbuf_write(&buffer, report);
>
> + // add other contents
> + add_header(report, "System Info");
> + get_system_info(&buffer);
> + strbuf_write(&buffer, report);
> +
> fclose(report);
>
> launch_editor(report_path.buf, NULL, NULL);
> --
> 2.24.0.rc0.303.g954a862665-goog
>
>
^ permalink raw reply [flat|nested] 68+ messages in thread* Re: [PATCH v3 3/9] bugreport: add version and system information
2019-10-28 13:49 ` Johannes Schindelin
@ 2019-11-08 21:48 ` Emily Shaffer
2019-11-11 13:48 ` Johannes Schindelin
0 siblings, 1 reply; 68+ messages in thread
From: Emily Shaffer @ 2019-11-08 21:48 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
On Mon, Oct 28, 2019 at 02:49:29PM +0100, Johannes Schindelin wrote:
> Hi Emily,
>
> On Thu, 24 Oct 2019, Emily Shaffer wrote:
>
> > diff --git a/bugreport.c b/bugreport.c
> > new file mode 100644
> > index 0000000000..ada54fe583
> > --- /dev/null
> > +++ b/bugreport.c
> > @@ -0,0 +1,55 @@
> > +#include "cache.h"
> > +
> > +#include "bugreport.h"
> > +#include "help.h"
> > +#include "run-command.h"
> > +#include "version.h"
> > +
> > +void get_system_info(struct strbuf *sys_info)
> > +{
> > + struct child_process cp = CHILD_PROCESS_INIT;
> > + struct strbuf std_out = STRBUF_INIT;
> > +
> > + strbuf_reset(sys_info);
> > +
> > + // get git version from native cmd
>
> Please use C-style comments instead of C++ ones.
>
> > + strbuf_addstr(sys_info, "git version: ");
> > + strbuf_addstr(sys_info, git_version_string);
> > + strbuf_complete_line(sys_info);
> > +
> > + // system call for other version info
> > + argv_array_push(&cp.args, "uname");
> > + argv_array_push(&cp.args, "-a");
> > + capture_command(&cp, &std_out, 0);
>
> Mmmkay. I am not too much of a fan of relying on the `uname` executable,
> as it can very well be a 32-bit `uname.exe` on Windows, which obviously
> would _not_ report the architecture of the machine, but something
> misleading.
>
> Why not use the `uname()` function (that we even define in
> `compat/mingw.c`)?
Very glad to have your review and point this kind of thing out. :) It
simplifies the code, too. I wasn't sure about these system calls, so I
appreciate you suggesting alternatives.
>
> Also, why not include the same information as `git version
> --build-options`, most importantly `GIT_HOST_CPU`?
>
> In any case, if you are capturing the output of `uname -a`, you
> definitely will want to pass the `RUN_COMMAND_STDOUT_TO_STDERR` flag (in
> case, say, the MSYS2 `uname.exe` fails with those dreaded Cygwin error
> messages like "*** fatal error - add_item
> ("\??\D:\git\installation\Git", "/", ...) failed, errno 1").
>
> > +
> > + strbuf_addstr(sys_info, "uname -a: ");
> > + strbuf_addbuf(sys_info, &std_out);
> > + strbuf_complete_line(sys_info);
> > +
> > + argv_array_clear(&cp.args);
> > + strbuf_reset(&std_out);
> > +
> > +
> > + argv_array_push(&cp.args, "curl-config");
> > + argv_array_push(&cp.args, "--version");
> > + capture_command(&cp, &std_out, 0);
>
> This will be almost certainly be incorrect, as most _regular_ users
> won't have `curl-config`. I know, it is easy to forget that most Git
> users are no hard-core C developers ;-)
Heresy! :)
>
> A better alternative would be to use `curl_version()`, guarded, of
> course, by `#ifndef NO_CURL`...
>
> > +
> > + strbuf_addstr(sys_info, "curl-config --version: ");
> > + strbuf_addbuf(sys_info, &std_out);
> > + strbuf_complete_line(sys_info);
> > +
> > + argv_array_clear(&cp.args);
> > + strbuf_reset(&std_out);
> > +
> > +
> > + argv_array_push(&cp.args, "ldd");
> > + argv_array_push(&cp.args, "--version");
> > + capture_command(&cp, &std_out, 0);
>
> Again, this command will only be present in few setups. I am not
> actually sure that the output of this is interesting to begin with.
It was a suggestion, I believe, from Jonathan Nieder.
>
> What I _do_ think is that a much more interesting piece of information
> would be the exact GLIBC version, the OS name and the OS version.
The glibc version is easy; I've done that. It certainly looks nicer than
the ldd call.
I guess I may be missing something, because as I start to look into how
to the OS info, I fall down a hole of many, many preprocessor defines to
check. If that's the approach you want me to take, just say the word,
but it will be ugly :) I suppose I had hoped the uname info would give us
a close enough idea that full OS info isn't necessary.
>
> > +
> > + strbuf_addstr(sys_info, "ldd --version: ");
> > + strbuf_addbuf(sys_info, &std_out);
> > + strbuf_complete_line(sys_info);
> > +
> > + argv_array_clear(&cp.args);
> > + strbuf_reset(&std_out);
> > +}
> > diff --git a/bugreport.h b/bugreport.h
> > new file mode 100644
> > index 0000000000..ba216acf3f
> > --- /dev/null
> > +++ b/bugreport.h
> > @@ -0,0 +1,7 @@
> > +#include "strbuf.h"
> > +
> > +/**
> > + * Adds the Git version, `uname -a`, and `curl-config --version` to sys_info.
> > + * The previous contents of sys_info will be discarded.
> > + */
> > +void get_system_info(struct strbuf *sys_info);
> > diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> > index 2ef16440a0..7232d31be7 100644
> > --- a/builtin/bugreport.c
> > +++ b/builtin/bugreport.c
> > @@ -1,4 +1,5 @@
> > #include "builtin.h"
> > +#include "bugreport.h"
> > #include "stdio.h"
> > #include "strbuf.h"
> > #include "time.h"
> > @@ -27,6 +28,13 @@ int get_bug_template(struct strbuf *template)
> > return 0;
> > }
> >
> > +void add_header(FILE *report, const char *title)
> > +{
> > + struct strbuf buffer = STRBUF_INIT;
> > + strbuf_addf(&buffer, "\n\n[%s]\n", title);
> > + strbuf_write(&buffer, report);
>
> This leaks `buffer`. Why not write into `report` via `fprintf()`
> directly?
Rather, to match the style of the rest of the builtin, modified
get_header to add the header to a passed-in strbuf instead of
modifying the file directly.
>
> Ciao,
> Dscho
>
> > +}
> > +
> > int cmd_bugreport(int argc, const char **argv, const char *prefix)
> > {
> > struct strbuf buffer = STRBUF_INIT;
> > @@ -43,6 +51,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> > get_bug_template(&buffer);
> > strbuf_write(&buffer, report);
> >
> > + // add other contents
> > + add_header(report, "System Info");
> > + get_system_info(&buffer);
> > + strbuf_write(&buffer, report);
> > +
> > fclose(report);
> >
> > launch_editor(report_path.buf, NULL, NULL);
> > --
> > 2.24.0.rc0.303.g954a862665-goog
> >
> >
Based on Dscho's comments, each piece of system info is gathered
differently enough that I will do each in an independent commit now.
^ permalink raw reply [flat|nested] 68+ messages in thread* Re: [PATCH v3 3/9] bugreport: add version and system information
2019-11-08 21:48 ` Emily Shaffer
@ 2019-11-11 13:48 ` Johannes Schindelin
2019-11-14 21:42 ` Emily Shaffer
0 siblings, 1 reply; 68+ messages in thread
From: Johannes Schindelin @ 2019-11-11 13:48 UTC (permalink / raw)
To: Emily Shaffer; +Cc: git
Hi Emily,
On Fri, 8 Nov 2019, Emily Shaffer wrote:
> On Mon, Oct 28, 2019 at 02:49:29PM +0100, Johannes Schindelin wrote:
>
> > On Thu, 24 Oct 2019, Emily Shaffer wrote:
> >
> > > diff --git a/bugreport.c b/bugreport.c
> > > new file mode 100644
> > > index 0000000000..ada54fe583
> > > --- /dev/null
> > > +++ b/bugreport.c
> > > [...]
> > > + strbuf_addstr(sys_info, "curl-config --version: ");
> > > + strbuf_addbuf(sys_info, &std_out);
> > > + strbuf_complete_line(sys_info);
> > > +
> > > + argv_array_clear(&cp.args);
> > > + strbuf_reset(&std_out);
> > > +
> > > +
> > > + argv_array_push(&cp.args, "ldd");
> > > + argv_array_push(&cp.args, "--version");
> > > + capture_command(&cp, &std_out, 0);
> >
> > Again, this command will only be present in few setups. I am not
> > actually sure that the output of this is interesting to begin with.
>
> It was a suggestion, I believe, from Jonathan Nieder.
Yes, I guess Jonathan builds their Git locally, too.
It _is_ easy to forget that most users find this too involved to even
try.
Nothing like reading through a bug tracker quite frequently to learn
about the actual troubles actual users have :-)
> > What I _do_ think is that a much more interesting piece of information
> > would be the exact GLIBC version, the OS name and the OS version.
>
> The glibc version is easy; I've done that. It certainly looks nicer than
> the ldd call.
>
> I guess I may be missing something, because as I start to look into how
> to the OS info, I fall down a hole of many, many preprocessor defines to
> check. If that's the approach you want me to take, just say the word,
> but it will be ugly :) I suppose I had hoped the uname info would give us
> a close enough idea that full OS info isn't necessary.
We could go down the pre-processor route, but that would give us the OS
name and version at build time, not at run time. I think we will be
mostly interested in the latter, though.
We might need to enhance our `uname()` emulation in `compat/mingw.c`,
but I think we already have enough information there.
When it comes to glibc, I think `gnu_get_libc_version()` would get us
what we want. A trickier thing might be to determine whether we're
actually linking against glibc; I do not want to break musl builds
again, I already did that inadvertently when requiring `REG_STARTEND`
back in the days.
> > > diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> > > index 2ef16440a0..7232d31be7 100644
> > > --- a/builtin/bugreport.c
> > > +++ b/builtin/bugreport.c
> > > @@ -1,4 +1,5 @@
> > > #include "builtin.h"
> > > +#include "bugreport.h"
> > > #include "stdio.h"
> > > #include "strbuf.h"
> > > #include "time.h"
> > > @@ -27,6 +28,13 @@ int get_bug_template(struct strbuf *template)
> > > return 0;
> > > }
> > >
> > > +void add_header(FILE *report, const char *title)
> > > +{
> > > + struct strbuf buffer = STRBUF_INIT;
> > > + strbuf_addf(&buffer, "\n\n[%s]\n", title);
> > > + strbuf_write(&buffer, report);
> >
> > This leaks `buffer`. Why not write into `report` via `fprintf()`
> > directly?
>
> Rather, to match the style of the rest of the builtin, modified
> get_header to add the header to a passed-in strbuf instead of
> modifying the file directly.
Hmm. It makes the code less elegant in my opinion. I would rather either
render the entire bug report into a single `strbuf` and then write it
via `write_in_full()`, or use `fprintf()` directly.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 68+ messages in thread* Re: [PATCH v3 3/9] bugreport: add version and system information
2019-11-11 13:48 ` Johannes Schindelin
@ 2019-11-14 21:42 ` Emily Shaffer
0 siblings, 0 replies; 68+ messages in thread
From: Emily Shaffer @ 2019-11-14 21:42 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
On Mon, Nov 11, 2019 at 02:48:13PM +0100, Johannes Schindelin wrote:
> > > > diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> > > > index 2ef16440a0..7232d31be7 100644
> > > > --- a/builtin/bugreport.c
> > > > +++ b/builtin/bugreport.c
> > > > @@ -1,4 +1,5 @@
> > > > #include "builtin.h"
> > > > +#include "bugreport.h"
> > > > #include "stdio.h"
> > > > #include "strbuf.h"
> > > > #include "time.h"
> > > > @@ -27,6 +28,13 @@ int get_bug_template(struct strbuf *template)
> > > > return 0;
> > > > }
> > > >
> > > > +void add_header(FILE *report, const char *title)
> > > > +{
> > > > + struct strbuf buffer = STRBUF_INIT;
> > > > + strbuf_addf(&buffer, "\n\n[%s]\n", title);
> > > > + strbuf_write(&buffer, report);
> > >
> > > This leaks `buffer`. Why not write into `report` via `fprintf()`
> > > directly?
> >
> > Rather, to match the style of the rest of the builtin, modified
> > get_header to add the header to a passed-in strbuf instead of
> > modifying the file directly.
>
> Hmm. It makes the code less elegant in my opinion. I would rather either
> render the entire bug report into a single `strbuf` and then write it
> via `write_in_full()`, or use `fprintf()` directly.
Yeah, that sounds good. I will do that. :)
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v3 3/9] bugreport: add version and system information
2019-10-25 2:51 ` [PATCH v3 3/9] bugreport: add version and system information Emily Shaffer
2019-10-28 13:49 ` Johannes Schindelin
@ 2019-10-29 20:43 ` Josh Steadmon
1 sibling, 0 replies; 68+ messages in thread
From: Josh Steadmon @ 2019-10-29 20:43 UTC (permalink / raw)
To: Emily Shaffer; +Cc: git
On 2019.10.24 19:51, Emily Shaffer wrote:
[snip]
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index 2ef16440a0..7232d31be7 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -1,4 +1,5 @@
> #include "builtin.h"
> +#include "bugreport.h"
> #include "stdio.h"
> #include "strbuf.h"
> #include "time.h"
> @@ -27,6 +28,13 @@ int get_bug_template(struct strbuf *template)
> return 0;
> }
>
> +void add_header(FILE *report, const char *title)
I get the same compilation error here, can you make add_header() static
as well?
^ permalink raw reply [flat|nested] 68+ messages in thread
* [PATCH v3 4/9] bugreport: add config values from whitelist
2019-10-25 2:51 ` [PATCH v3 0/9] add git-bugreport tool Emily Shaffer
` (2 preceding siblings ...)
2019-10-25 2:51 ` [PATCH v3 3/9] bugreport: add version and system information Emily Shaffer
@ 2019-10-25 2:51 ` Emily Shaffer
2019-10-28 14:14 ` Johannes Schindelin
2019-10-29 20:58 ` Josh Steadmon
2019-10-25 2:51 ` [PATCH v3 5/9] bugreport: collect list of populated hooks Emily Shaffer
` (5 subsequent siblings)
9 siblings, 2 replies; 68+ messages in thread
From: Emily Shaffer @ 2019-10-25 2:51 UTC (permalink / raw)
To: git; +Cc: Emily Shaffer
Teach bugreport to gather the values of config options which are present
in 'git-bugreport-config-whitelist'.
Many config options are sensitive, and many Git add-ons use config
options which git-core does not know about; it is better only to gather
config options which we know to be safe, rather than excluding options
which we know to be unsafe.
Reading the whitelist into memory and sorting it saves us time -
since git_config_bugreport() is called for every option the user has
configured, limiting the file IO to one open/read/close and performing
option lookup in sublinear time is a useful optimization.
Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
bugreport.c | 50 +++++++++++++++++++++++++++++++++++++++++++++
bugreport.h | 7 +++++++
builtin/bugreport.c | 4 ++++
3 files changed, 61 insertions(+)
diff --git a/bugreport.c b/bugreport.c
index ada54fe583..afa4836ab1 100644
--- a/bugreport.c
+++ b/bugreport.c
@@ -1,10 +1,24 @@
#include "cache.h"
#include "bugreport.h"
+#include "config.h"
+#include "exec-cmd.h"
#include "help.h"
#include "run-command.h"
#include "version.h"
+/**
+ * A sorted list of config options which we will add to the bugreport. Managed
+ * by 'gather_whitelist(...)'.
+ */
+struct string_list whitelist = STRING_LIST_INIT_DUP;
+struct strbuf configs_and_values = STRBUF_INIT;
+
+// git version --build-options
+// uname -a
+// curl-config --version
+// ldd --version
+// echo $SHELL
void get_system_info(struct strbuf *sys_info)
{
struct child_process cp = CHILD_PROCESS_INIT;
@@ -53,3 +67,39 @@ void get_system_info(struct strbuf *sys_info)
argv_array_clear(&cp.args);
strbuf_reset(&std_out);
}
+
+void gather_whitelist(struct strbuf *path)
+{
+ struct strbuf tmp = STRBUF_INIT;
+ strbuf_read_file(&tmp, path->buf, 0);
+ string_list_init(&whitelist, 1);
+ string_list_split(&whitelist, tmp.buf, '\n', -1);
+ string_list_sort(&whitelist);
+}
+
+int git_config_bugreport(const char *var, const char *value, void *cb)
+{
+ if (string_list_has_string(&whitelist, var)) {
+ strbuf_addf(&configs_and_values,
+ "%s : %s\n",
+ var, value);
+ }
+
+ return 0;
+}
+
+void get_whitelisted_config(struct strbuf *config_info)
+{
+ struct strbuf path = STRBUF_INIT;
+
+ strbuf_addstr(&path, git_exec_path());
+ strbuf_addstr(&path, "/git-bugreport-config-whitelist");
+
+ gather_whitelist(&path);
+ strbuf_init(&configs_and_values, whitelist.nr);
+
+ git_config(git_config_bugreport, NULL);
+
+ strbuf_reset(config_info);
+ strbuf_addbuf(config_info, &configs_and_values);
+}
diff --git a/bugreport.h b/bugreport.h
index ba216acf3f..7413e7e1be 100644
--- a/bugreport.h
+++ b/bugreport.h
@@ -5,3 +5,10 @@
* The previous contents of sys_info will be discarded.
*/
void get_system_info(struct strbuf *sys_info);
+
+/**
+ * Adds the values of the config items listed in
+ * 'git-bugreport-config-whitelist' to config_info. The previous contents of
+ * config_info will be discarded.
+ */
+void get_whitelisted_config(struct strbuf *sys_info);
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 7232d31be7..70fe0d2b85 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -56,6 +56,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
get_system_info(&buffer);
strbuf_write(&buffer, report);
+ add_header(report, "Whitelisted Config");
+ get_whitelisted_config(&buffer);
+ strbuf_write(&buffer, report);
+
fclose(report);
launch_editor(report_path.buf, NULL, NULL);
--
2.24.0.rc0.303.g954a862665-goog
^ permalink raw reply related [flat|nested] 68+ messages in thread* Re: [PATCH v3 4/9] bugreport: add config values from whitelist
2019-10-25 2:51 ` [PATCH v3 4/9] bugreport: add config values from whitelist Emily Shaffer
@ 2019-10-28 14:14 ` Johannes Schindelin
2019-12-11 20:48 ` Emily Shaffer
2019-10-29 20:58 ` Josh Steadmon
1 sibling, 1 reply; 68+ messages in thread
From: Johannes Schindelin @ 2019-10-28 14:14 UTC (permalink / raw)
To: Emily Shaffer; +Cc: git
Hi Emily,
On Thu, 24 Oct 2019, Emily Shaffer wrote:
> Teach bugreport to gather the values of config options which are present
> in 'git-bugreport-config-whitelist'.
>
> Many config options are sensitive, and many Git add-ons use config
> options which git-core does not know about; it is better only to gather
> config options which we know to be safe, rather than excluding options
> which we know to be unsafe.
Should we still have the `// bugreport-exclude` comments, then?
>
> Reading the whitelist into memory and sorting it saves us time -
> since git_config_bugreport() is called for every option the user has
> configured, limiting the file IO to one open/read/close and performing
> option lookup in sublinear time is a useful optimization.
Maybe we even want a hashmap? That would reduce the time complexity even
further.
> diff --git a/bugreport.c b/bugreport.c
> index ada54fe583..afa4836ab1 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -1,10 +1,24 @@
> #include "cache.h"
>
> #include "bugreport.h"
> +#include "config.h"
> +#include "exec-cmd.h"
> #include "help.h"
> #include "run-command.h"
> #include "version.h"
>
> +/**
> + * A sorted list of config options which we will add to the bugreport. Managed
> + * by 'gather_whitelist(...)'.
> + */
> +struct string_list whitelist = STRING_LIST_INIT_DUP;
> +struct strbuf configs_and_values = STRBUF_INIT;
> +
> +// git version --build-options
> +// uname -a
> +// curl-config --version
> +// ldd --version
> +// echo $SHELL
These comments probably want to move to a single, C style comment, and
they probably want to be introduced together with `get_system_info()`.
I also have to admit that I might have missed where `$SHELL` was added
to the output...
> void get_system_info(struct strbuf *sys_info)
> {
> struct child_process cp = CHILD_PROCESS_INIT;
> @@ -53,3 +67,39 @@ void get_system_info(struct strbuf *sys_info)
> argv_array_clear(&cp.args);
> strbuf_reset(&std_out);
> }
> +
> +void gather_whitelist(struct strbuf *path)
> +{
> + struct strbuf tmp = STRBUF_INIT;
> + strbuf_read_file(&tmp, path->buf, 0);
> + string_list_init(&whitelist, 1);
> + string_list_split(&whitelist, tmp.buf, '\n', -1);
> + string_list_sort(&whitelist);
> +}
> +
> +int git_config_bugreport(const char *var, const char *value, void *cb)
> +{
> + if (string_list_has_string(&whitelist, var)) {
> + strbuf_addf(&configs_and_values,
> + "%s : %s\n",
> + var, value);
A quite useful piece of information would be the config source. Not sure
whether we can do that outside of `config.c` yet...
> + }
> +
> + return 0;
> +}
> +
> +void get_whitelisted_config(struct strbuf *config_info)
> +{
> + struct strbuf path = STRBUF_INIT;
> +
> + strbuf_addstr(&path, git_exec_path());
> + strbuf_addstr(&path, "/git-bugreport-config-whitelist");
Hmm. I would have expected this patch to come directly after the patch
2/9 that generates that white-list, and I would also have expected that
to be pre-sorted, and compiled in.
Do you want users to _edit_ the file in the exec path? In general, that
path will be write-protected, though. A better alternative would
probably be to compile in a hard-coded list, and to allow including more
values e.g. by offering command-line options to specify config setting
patterns. But if we allow patterns, we might actually want to have those
exclusions to prevent sensitive data from being included.
> +
> + gather_whitelist(&path);
> + strbuf_init(&configs_and_values, whitelist.nr);
> +
> + git_config(git_config_bugreport, NULL);
> +
> + strbuf_reset(config_info);
> + strbuf_addbuf(config_info, &configs_and_values);
> +}
> diff --git a/bugreport.h b/bugreport.h
> index ba216acf3f..7413e7e1be 100644
> --- a/bugreport.h
> +++ b/bugreport.h
> @@ -5,3 +5,10 @@
> * The previous contents of sys_info will be discarded.
> */
> void get_system_info(struct strbuf *sys_info);
> +
> +/**
I also frequently use JavaDoc-style `/**`, but I am not sure that this
is actually desired in Git's source code ;-)
> + * Adds the values of the config items listed in
> + * 'git-bugreport-config-whitelist' to config_info. The previous contents of
> + * config_info will be discarded.
> + */
> +void get_whitelisted_config(struct strbuf *sys_info);
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index 7232d31be7..70fe0d2b85 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -56,6 +56,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> get_system_info(&buffer);
> strbuf_write(&buffer, report);
>
> + add_header(report, "Whitelisted Config");
Quite honestly, I would like to avoid the term "whitelist" for good. How
about "Selected config settings" instead?
Thanks,
Dscho
> + get_whitelisted_config(&buffer);
> + strbuf_write(&buffer, report);
> +
> fclose(report);
>
> launch_editor(report_path.buf, NULL, NULL);
> --
> 2.24.0.rc0.303.g954a862665-goog
>
>
^ permalink raw reply [flat|nested] 68+ messages in thread* Re: [PATCH v3 4/9] bugreport: add config values from whitelist
2019-10-28 14:14 ` Johannes Schindelin
@ 2019-12-11 20:48 ` Emily Shaffer
2019-12-15 17:30 ` Johannes Schindelin
0 siblings, 1 reply; 68+ messages in thread
From: Emily Shaffer @ 2019-12-11 20:48 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
On Mon, Oct 28, 2019 at 03:14:35PM +0100, Johannes Schindelin wrote:
> Hi Emily,
Sorry for the delay in replying. This work has been backburnered and
this mail slipped through the cracks.
>
> On Thu, 24 Oct 2019, Emily Shaffer wrote:
>
> > Teach bugreport to gather the values of config options which are present
> > in 'git-bugreport-config-whitelist'.
> >
> > Many config options are sensitive, and many Git add-ons use config
> > options which git-core does not know about; it is better only to gather
> > config options which we know to be safe, rather than excluding options
> > which we know to be unsafe.
>
> Should we still have the `// bugreport-exclude` comments, then?
They were optional (useless) before too. I can remove them if you want;
I suppose I like the idea of having precedent if someone wants to build
their own internal version with opt-out configs rather than opt-in. I
can remove them if we want; it doesn't matter very much to me either
way.
>
> >
> > Reading the whitelist into memory and sorting it saves us time -
> > since git_config_bugreport() is called for every option the user has
> > configured, limiting the file IO to one open/read/close and performing
> > option lookup in sublinear time is a useful optimization.
>
> Maybe we even want a hashmap? That would reduce the time complexity even
> further.
Sure, we can do it. I'll make that change.
>
> > diff --git a/bugreport.c b/bugreport.c
> > index ada54fe583..afa4836ab1 100644
> > --- a/bugreport.c
> > +++ b/bugreport.c
> > @@ -1,10 +1,24 @@
> > #include "cache.h"
> >
> > #include "bugreport.h"
> > +#include "config.h"
> > +#include "exec-cmd.h"
> > #include "help.h"
> > #include "run-command.h"
> > #include "version.h"
> >
> > +/**
> > + * A sorted list of config options which we will add to the bugreport. Managed
> > + * by 'gather_whitelist(...)'.
> > + */
> > +struct string_list whitelist = STRING_LIST_INIT_DUP;
> > +struct strbuf configs_and_values = STRBUF_INIT;
> > +
> > +// git version --build-options
> > +// uname -a
> > +// curl-config --version
> > +// ldd --version
> > +// echo $SHELL
>
> These comments probably want to move to a single, C style comment, and
> they probably want to be introduced together with `get_system_info()`.
Yeah, it's stale and has been removed now. It was less commentary and
more todo list for author ;)
>
> I also have to admit that I might have missed where `$SHELL` was added
> to the output...
I skipped it entirely since bugreport doesn't run in shell anymore. If
you have advice for gathering the user's shell I can try to add it; is
there such a difference between, say, a Debian user using bash and a
Debian user using zsh? I suppose it could be useful if someone has an
issue with GIT_PS1, or with autocompletion. I'll look into gathering it.
>
> > void get_system_info(struct strbuf *sys_info)
> > {
> > struct child_process cp = CHILD_PROCESS_INIT;
> > @@ -53,3 +67,39 @@ void get_system_info(struct strbuf *sys_info)
> > argv_array_clear(&cp.args);
> > strbuf_reset(&std_out);
> > }
> > +
> > +void gather_whitelist(struct strbuf *path)
> > +{
> > + struct strbuf tmp = STRBUF_INIT;
> > + strbuf_read_file(&tmp, path->buf, 0);
> > + string_list_init(&whitelist, 1);
> > + string_list_split(&whitelist, tmp.buf, '\n', -1);
> > + string_list_sort(&whitelist);
> > +}
> > +
> > +int git_config_bugreport(const char *var, const char *value, void *cb)
> > +{
> > + if (string_list_has_string(&whitelist, var)) {
> > + strbuf_addf(&configs_and_values,
> > + "%s : %s\n",
> > + var, value);
>
> A quite useful piece of information would be the config source. Not sure
> whether we can do that outside of `config.c` yet...
It's possible. I can add it.
>
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +void get_whitelisted_config(struct strbuf *config_info)
> > +{
> > + struct strbuf path = STRBUF_INIT;
> > +
> > + strbuf_addstr(&path, git_exec_path());
> > + strbuf_addstr(&path, "/git-bugreport-config-whitelist");
>
> Hmm. I would have expected this patch to come directly after the patch
> 2/9 that generates that white-list, and I would also have expected that
> to be pre-sorted, and compiled in.
>
> Do you want users to _edit_ the file in the exec path? In general, that
> path will be write-protected, though. A better alternative would
> probably be to compile in a hard-coded list, and to allow including more
> values e.g. by offering command-line options to specify config setting
> patterns. But if we allow patterns, we might actually want to have those
> exclusions to prevent sensitive data from being included.
Hm, interesting. Do we have precedent for compiling in a header
generated during the build process? I think I saw one when I was adding
this script - I'll take a look.
>
> > +
> > + gather_whitelist(&path);
> > + strbuf_init(&configs_and_values, whitelist.nr);
> > +
> > + git_config(git_config_bugreport, NULL);
> > +
> > + strbuf_reset(config_info);
> > + strbuf_addbuf(config_info, &configs_and_values);
> > +}
> > diff --git a/bugreport.h b/bugreport.h
> > index ba216acf3f..7413e7e1be 100644
> > --- a/bugreport.h
> > +++ b/bugreport.h
> > @@ -5,3 +5,10 @@
> > * The previous contents of sys_info will be discarded.
> > */
> > void get_system_info(struct strbuf *sys_info);
> > +
> > +/**
>
> I also frequently use JavaDoc-style `/**`, but I am not sure that this
> is actually desired in Git's source code ;-)
>
> > + * Adds the values of the config items listed in
> > + * 'git-bugreport-config-whitelist' to config_info. The previous contents of
> > + * config_info will be discarded.
> > + */
> > +void get_whitelisted_config(struct strbuf *sys_info);
> > diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> > index 7232d31be7..70fe0d2b85 100644
> > --- a/builtin/bugreport.c
> > +++ b/builtin/bugreport.c
> > @@ -56,6 +56,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> > get_system_info(&buffer);
> > strbuf_write(&buffer, report);
> >
> > + add_header(report, "Whitelisted Config");
>
> Quite honestly, I would like to avoid the term "whitelist" for good. How
> about "Selected config settings" instead?
Will do - thanks for the callout.
>
> Thanks,
> Dscho
>
> > + get_whitelisted_config(&buffer);
> > + strbuf_write(&buffer, report);
> > +
> > fclose(report);
> >
> > launch_editor(report_path.buf, NULL, NULL);
> > --
> > 2.24.0.rc0.303.g954a862665-goog
> >
> >
^ permalink raw reply [flat|nested] 68+ messages in thread* Re: [PATCH v3 4/9] bugreport: add config values from whitelist
2019-12-11 20:48 ` Emily Shaffer
@ 2019-12-15 17:30 ` Johannes Schindelin
0 siblings, 0 replies; 68+ messages in thread
From: Johannes Schindelin @ 2019-12-15 17:30 UTC (permalink / raw)
To: Emily Shaffer; +Cc: git
Hi Emily,
On Wed, 11 Dec 2019, Emily Shaffer wrote:
> On Mon, Oct 28, 2019 at 03:14:35PM +0100, Johannes Schindelin wrote:
>
> >
> > On Thu, 24 Oct 2019, Emily Shaffer wrote:
> >
> > > Teach bugreport to gather the values of config options which are
> > > present in 'git-bugreport-config-whitelist'.
> > >
> > > Many config options are sensitive, and many Git add-ons use config
> > > options which git-core does not know about; it is better only to
> > > gather config options which we know to be safe, rather than
> > > excluding options which we know to be unsafe.
> >
> > Should we still have the `// bugreport-exclude` comments, then?
>
> They were optional (useless) before too. I can remove them if you want;
> I suppose I like the idea of having precedent if someone wants to build
> their own internal version with opt-out configs rather than opt-in. I
> can remove them if we want; it doesn't matter very much to me either
> way.
How are you guaranteeing that this information does not become stale,
like, immediately?
If you _still_ insist on keeping those comments even after answering this
question, at least please turn them into C-style comments: we have no
C++-style comments in Git and want to keep it this way.
> > > diff --git a/bugreport.c b/bugreport.c
> > > [...]
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +void get_whitelisted_config(struct strbuf *config_info)
> > > +{
> > > + struct strbuf path = STRBUF_INIT;
> > > +
> > > + strbuf_addstr(&path, git_exec_path());
> > > + strbuf_addstr(&path, "/git-bugreport-config-whitelist");
> >
> > Hmm. I would have expected this patch to come directly after the patch
> > 2/9 that generates that white-list, and I would also have expected that
> > to be pre-sorted, and compiled in.
> >
> > Do you want users to _edit_ the file in the exec path? In general, that
> > path will be write-protected, though. A better alternative would
> > probably be to compile in a hard-coded list, and to allow including more
> > values e.g. by offering command-line options to specify config setting
> > patterns. But if we allow patterns, we might actually want to have those
> > exclusions to prevent sensitive data from being included.
>
> Hm, interesting. Do we have precedent for compiling in a header
> generated during the build process? I think I saw one when I was adding
> this script - I'll take a look.
Yes, we do. The entire `command-list.h` is generated during the build.
Look for `GENERATED_H` in the `Makefile`.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v3 4/9] bugreport: add config values from whitelist
2019-10-25 2:51 ` [PATCH v3 4/9] bugreport: add config values from whitelist Emily Shaffer
2019-10-28 14:14 ` Johannes Schindelin
@ 2019-10-29 20:58 ` Josh Steadmon
2019-10-30 1:37 ` Junio C Hamano
1 sibling, 1 reply; 68+ messages in thread
From: Josh Steadmon @ 2019-10-29 20:58 UTC (permalink / raw)
To: Emily Shaffer; +Cc: git
On 2019.10.24 19:51, Emily Shaffer wrote:
[snip]
> diff --git a/bugreport.c b/bugreport.c
> index ada54fe583..afa4836ab1 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -1,10 +1,24 @@
> #include "cache.h"
>
> #include "bugreport.h"
> +#include "config.h"
> +#include "exec-cmd.h"
> #include "help.h"
> #include "run-command.h"
> #include "version.h"
>
> +/**
> + * A sorted list of config options which we will add to the bugreport. Managed
> + * by 'gather_whitelist(...)'.
> + */
> +struct string_list whitelist = STRING_LIST_INIT_DUP;
> +struct strbuf configs_and_values = STRBUF_INIT;
> +
> +// git version --build-options
> +// uname -a
> +// curl-config --version
> +// ldd --version
> +// echo $SHELL
> void get_system_info(struct strbuf *sys_info)
> {
> struct child_process cp = CHILD_PROCESS_INIT;
> @@ -53,3 +67,39 @@ void get_system_info(struct strbuf *sys_info)
> argv_array_clear(&cp.args);
> strbuf_reset(&std_out);
> }
> +
> +void gather_whitelist(struct strbuf *path)
This and git_config_bugreport() below should both be static as well.
Rather than repeating advice on the later patches, I'll just note that
any new functions that don't show up in the corresponding .h file should
be marked static.
^ permalink raw reply [flat|nested] 68+ messages in thread* Re: [PATCH v3 4/9] bugreport: add config values from whitelist
2019-10-29 20:58 ` Josh Steadmon
@ 2019-10-30 1:37 ` Junio C Hamano
2019-11-14 21:55 ` Emily Shaffer
0 siblings, 1 reply; 68+ messages in thread
From: Junio C Hamano @ 2019-10-30 1:37 UTC (permalink / raw)
To: Josh Steadmon; +Cc: Emily Shaffer, git
Josh Steadmon <steadmon@google.com> writes:
> This and git_config_bugreport() below should both be static as well.
> Rather than repeating advice on the later patches, I'll just note that
> any new functions that don't show up in the corresponding .h file should
> be marked static.
Good advice.
More importantly, given that "git bugreport" itself has no service
of itself to offer other existing parts of Git (it is just a "gather
various pieces of information from different places, and then
produce a text file output" application), I do not see much point in
it having its own header file that others would #include (i.e. the
include file is to define services that are offered by it). If
there are common enough service routines invented to support the
need of bugreport.c (e.g. perhaps it wants to give more info than
what is currently available via the existing API on the contents of
in-core index), by definition of being them common enough, they
should be added to the header that can be used by both bugreport.c
and other existing users of the same subsystem (e.g. if it is about
in-core index, perhaps cache.h).
It makes perfect sense for bugreport.c to #include header files for
the Git internals to collect pieces of information from inside Git,
though.
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v3 4/9] bugreport: add config values from whitelist
2019-10-30 1:37 ` Junio C Hamano
@ 2019-11-14 21:55 ` Emily Shaffer
0 siblings, 0 replies; 68+ messages in thread
From: Emily Shaffer @ 2019-11-14 21:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Josh Steadmon, git
On Wed, Oct 30, 2019 at 10:37:13AM +0900, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
>
> > This and git_config_bugreport() below should both be static as well.
> > Rather than repeating advice on the later patches, I'll just note that
> > any new functions that don't show up in the corresponding .h file should
> > be marked static.
>
> Good advice.
>
> More importantly, given that "git bugreport" itself has no service
> of itself to offer other existing parts of Git (it is just a "gather
> various pieces of information from different places, and then
> produce a text file output" application), I do not see much point in
> it having its own header file that others would #include (i.e. the
> include file is to define services that are offered by it). If
> there are common enough service routines invented to support the
> need of bugreport.c (e.g. perhaps it wants to give more info than
> what is currently available via the existing API on the contents of
> in-core index), by definition of being them common enough, they
> should be added to the header that can be used by both bugreport.c
> and other existing users of the same subsystem (e.g. if it is about
> in-core index, perhaps cache.h).
Are you asking me to have only builtin/bugreport.c and implement each
function there, and eliminate both <basedir>/bugreport.[ch]?
It may be true that deciding to put this functionality into a library
"in case someone needs it later" was premature, so I can do that - I
just want to make sure I understand you.
- Emily
^ permalink raw reply [flat|nested] 68+ messages in thread
* [PATCH v3 5/9] bugreport: collect list of populated hooks
2019-10-25 2:51 ` [PATCH v3 0/9] add git-bugreport tool Emily Shaffer
` (3 preceding siblings ...)
2019-10-25 2:51 ` [PATCH v3 4/9] bugreport: add config values from whitelist Emily Shaffer
@ 2019-10-25 2:51 ` Emily Shaffer
2019-10-28 14:31 ` Johannes Schindelin
2019-10-25 2:51 ` [PATCH v3 6/9] bugreport: count loose objects Emily Shaffer
` (4 subsequent siblings)
9 siblings, 1 reply; 68+ messages in thread
From: Emily Shaffer @ 2019-10-25 2:51 UTC (permalink / raw)
To: git; +Cc: Emily Shaffer
Occasionally a failure a user is seeing may be related to a specific
hook which is being run, perhaps without the user realizing. While the
contents of hooks can be sensitive - containing user data or process
information specific to the user's organization - simply knowing that a
hook is being run at a certain stage can help us to understand whether
something is going wrong.
Without a definitive list of hook names within the code, we compile our
own list from the documentation. This is likely prone to bitrot. To
reduce the amount of code humans need to read, we turn the list into a
string_list and iterate over it (as we are calling the same find_hook
operation on each string). However, since bugreport should primarily be
called by the user, the performance loss from massaging the string
seems acceptable.
Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
bugreport.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
bugreport.h | 6 ++++++
builtin/bugreport.c | 4 ++++
3 files changed, 54 insertions(+)
diff --git a/bugreport.c b/bugreport.c
index afa4836ab1..9d7f44ff28 100644
--- a/bugreport.c
+++ b/bugreport.c
@@ -103,3 +103,47 @@ void get_whitelisted_config(struct strbuf *config_info)
strbuf_reset(config_info);
strbuf_addbuf(config_info, &configs_and_values);
}
+
+void get_populated_hooks(struct strbuf *hook_info)
+{
+ /*
+ * Doesn't look like there is a list of all possible hooks; so below is
+ * a transcription of `git help hook`.
+ */
+ const char *hooks = "applypatch-msg,"
+ "pre-applypatch,"
+ "post-applypatch,"
+ "pre-commit,"
+ "pre-merge-commit,"
+ "prepare-commit-msg,"
+ "commit-msg,"
+ "post-commit,"
+ "pre-rebase,"
+ "post-checkout,"
+ "post-merge,"
+ "pre-push,"
+ "pre-receive,"
+ "update,"
+ "post-receive,"
+ "post-update,"
+ "push-to-checkout,"
+ "pre-auto-gc,"
+ "post-rewrite,"
+ "sendemail-validate,"
+ "fsmonitor-watchman,"
+ "p4-pre-submit,"
+ "post-index-changex";
+ struct string_list hooks_list = STRING_LIST_INIT_DUP;
+ struct string_list_item *iter = NULL;
+
+ strbuf_reset(hook_info);
+
+ string_list_split(&hooks_list, hooks, ',', -1);
+
+ for_each_string_list_item(iter, &hooks_list) {
+ if (find_hook(iter->string)) {
+ strbuf_addstr(hook_info, iter->string);
+ strbuf_complete_line(hook_info);
+ }
+ }
+}
diff --git a/bugreport.h b/bugreport.h
index 7413e7e1be..942a5436e3 100644
--- a/bugreport.h
+++ b/bugreport.h
@@ -12,3 +12,9 @@ void get_system_info(struct strbuf *sys_info);
* config_info will be discarded.
*/
void get_whitelisted_config(struct strbuf *sys_info);
+
+/**
+ * Adds the paths to all configured hooks (but not their contents). The previous
+ * contents of hook_info will be discarded.
+ */
+void get_populated_hooks(struct strbuf *hook_info);
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 70fe0d2b85..a0eefba498 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -60,6 +60,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
get_whitelisted_config(&buffer);
strbuf_write(&buffer, report);
+ add_header(report, "Populated Hooks");
+ get_populated_hooks(&buffer);
+ strbuf_write(&buffer, report);
+
fclose(report);
launch_editor(report_path.buf, NULL, NULL);
--
2.24.0.rc0.303.g954a862665-goog
^ permalink raw reply related [flat|nested] 68+ messages in thread* Re: [PATCH v3 5/9] bugreport: collect list of populated hooks
2019-10-25 2:51 ` [PATCH v3 5/9] bugreport: collect list of populated hooks Emily Shaffer
@ 2019-10-28 14:31 ` Johannes Schindelin
2019-12-11 20:51 ` Emily Shaffer
0 siblings, 1 reply; 68+ messages in thread
From: Johannes Schindelin @ 2019-10-28 14:31 UTC (permalink / raw)
To: Emily Shaffer; +Cc: git
Hi Emily,
On Thu, 24 Oct 2019, Emily Shaffer wrote:
> Occasionally a failure a user is seeing may be related to a specific
> hook which is being run, perhaps without the user realizing. While the
> contents of hooks can be sensitive - containing user data or process
> information specific to the user's organization - simply knowing that a
> hook is being run at a certain stage can help us to understand whether
> something is going wrong.
>
> Without a definitive list of hook names within the code, we compile our
> own list from the documentation. This is likely prone to bitrot. To
> reduce the amount of code humans need to read, we turn the list into a
> string_list and iterate over it (as we are calling the same find_hook
> operation on each string). However, since bugreport should primarily be
> called by the user, the performance loss from massaging the string
> seems acceptable.
Good idea!
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
> bugreport.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> bugreport.h | 6 ++++++
> builtin/bugreport.c | 4 ++++
> 3 files changed, 54 insertions(+)
>
> diff --git a/bugreport.c b/bugreport.c
> index afa4836ab1..9d7f44ff28 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -103,3 +103,47 @@ void get_whitelisted_config(struct strbuf *config_info)
> strbuf_reset(config_info);
> strbuf_addbuf(config_info, &configs_and_values);
> }
> +
> +void get_populated_hooks(struct strbuf *hook_info)
> +{
> + /*
> + * Doesn't look like there is a list of all possible hooks; so below is
> + * a transcription of `git help hook`.
> + */
> + const char *hooks = "applypatch-msg,"
> + "pre-applypatch,"
> + "post-applypatch,"
> + "pre-commit,"
> + "pre-merge-commit,"
> + "prepare-commit-msg,"
> + "commit-msg,"
> + "post-commit,"
> + "pre-rebase,"
> + "post-checkout,"
> + "post-merge,"
> + "pre-push,"
> + "pre-receive,"
> + "update,"
> + "post-receive,"
> + "post-update,"
> + "push-to-checkout,"
> + "pre-auto-gc,"
> + "post-rewrite,"
> + "sendemail-validate,"
> + "fsmonitor-watchman,"
> + "p4-pre-submit,"
> + "post-index-changex";
Well, this is disappointing: I tried to come up with a scripted way to
extract the commit names from our source code, and I failed! This is
where I gave up:
git grep run_hook |
sed -n 's/.*run_hook[^(]*([^,]*, *\([^,]*\).*/\1/p' |
sed -e '/^name$/d' -e '/^const char \*name$/d' \
-e 's/push_to_checkout_hook/"push-to-checkout"/' |
sort |
uniq
This prints only the following hook names:
"applypatch-msg"
"post-applypatch"
"post-checkout"
"post-index-change"
"post-merge"
"pre-applypatch"
"pre-auto-gc"
"pre-rebase"
"prepare-commit-msg"
"push-to-checkout"
But at least it was easy to script the extracting from the
Documentation:
sed -n '/^[a-z]/{N;/\n~~~/{s/\n.*//;p}}' \
Documentation/githooks.txt
> + struct string_list hooks_list = STRING_LIST_INIT_DUP;
> + struct string_list_item *iter = NULL;
> +
> + strbuf_reset(hook_info);
> +
> + string_list_split(&hooks_list, hooks, ',', -1);
> +
> + for_each_string_list_item(iter, &hooks_list) {
This should definitely be done at compile time, I think. We should be
able to generate an appropriate header via something like this:
cat >hook-names.h <<-EOF
static const char *hook_names[] = {
$(sed -n '/^[a-z]/{N;/\n~~~/{s/\n.*/",/;s/^/ "/;p}}' \
Documentation/githooks.txt)
};
EOF
Then you would use a simple `for()` loop using `ARRAY_SIZE()` to
iterate over the hook names.
> + if (find_hook(iter->string)) {
> + strbuf_addstr(hook_info, iter->string);
> + strbuf_complete_line(hook_info);
> + }
> + }
> +}
> diff --git a/bugreport.h b/bugreport.h
> index 7413e7e1be..942a5436e3 100644
> --- a/bugreport.h
> +++ b/bugreport.h
> @@ -12,3 +12,9 @@ void get_system_info(struct strbuf *sys_info);
> * config_info will be discarded.
> */
> void get_whitelisted_config(struct strbuf *sys_info);
> +
> +/**
> + * Adds the paths to all configured hooks (but not their contents). The previous
> + * contents of hook_info will be discarded.
> + */
> +void get_populated_hooks(struct strbuf *hook_info);
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index 70fe0d2b85..a0eefba498 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -60,6 +60,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> get_whitelisted_config(&buffer);
> strbuf_write(&buffer, report);
>
> + add_header(report, "Populated Hooks");
Wait! I should have stumbled over this in an earlier patch. The
`add_header()` function should not take a `FILE *` parameter at all, but
instead an `struct strbuf *` one!
Ciao,
Dscho
> + get_populated_hooks(&buffer);
> + strbuf_write(&buffer, report);
> +
> fclose(report);
>
> launch_editor(report_path.buf, NULL, NULL);
> --
> 2.24.0.rc0.303.g954a862665-goog
>
>
^ permalink raw reply [flat|nested] 68+ messages in thread* Re: [PATCH v3 5/9] bugreport: collect list of populated hooks
2019-10-28 14:31 ` Johannes Schindelin
@ 2019-12-11 20:51 ` Emily Shaffer
2019-12-15 17:40 ` Johannes Schindelin
0 siblings, 1 reply; 68+ messages in thread
From: Emily Shaffer @ 2019-12-11 20:51 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
On Mon, Oct 28, 2019 at 03:31:36PM +0100, Johannes Schindelin wrote:
> Hi Emily,
>
> On Thu, 24 Oct 2019, Emily Shaffer wrote:
>
> > Occasionally a failure a user is seeing may be related to a specific
> > hook which is being run, perhaps without the user realizing. While the
> > contents of hooks can be sensitive - containing user data or process
> > information specific to the user's organization - simply knowing that a
> > hook is being run at a certain stage can help us to understand whether
> > something is going wrong.
> >
> > Without a definitive list of hook names within the code, we compile our
> > own list from the documentation. This is likely prone to bitrot. To
> > reduce the amount of code humans need to read, we turn the list into a
> > string_list and iterate over it (as we are calling the same find_hook
> > operation on each string). However, since bugreport should primarily be
> > called by the user, the performance loss from massaging the string
> > seems acceptable.
>
> Good idea!
>
> >
> > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > ---
> > bugreport.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > bugreport.h | 6 ++++++
> > builtin/bugreport.c | 4 ++++
> > 3 files changed, 54 insertions(+)
> >
> > diff --git a/bugreport.c b/bugreport.c
> > index afa4836ab1..9d7f44ff28 100644
> > --- a/bugreport.c
> > +++ b/bugreport.c
> > @@ -103,3 +103,47 @@ void get_whitelisted_config(struct strbuf *config_info)
> > strbuf_reset(config_info);
> > strbuf_addbuf(config_info, &configs_and_values);
> > }
> > +
> > +void get_populated_hooks(struct strbuf *hook_info)
> > +{
> > + /*
> > + * Doesn't look like there is a list of all possible hooks; so below is
> > + * a transcription of `git help hook`.
> > + */
> > + const char *hooks = "applypatch-msg,"
> > + "pre-applypatch,"
> > + "post-applypatch,"
> > + "pre-commit,"
> > + "pre-merge-commit,"
> > + "prepare-commit-msg,"
> > + "commit-msg,"
> > + "post-commit,"
> > + "pre-rebase,"
> > + "post-checkout,"
> > + "post-merge,"
> > + "pre-push,"
> > + "pre-receive,"
> > + "update,"
> > + "post-receive,"
> > + "post-update,"
> > + "push-to-checkout,"
> > + "pre-auto-gc,"
> > + "post-rewrite,"
> > + "sendemail-validate,"
> > + "fsmonitor-watchman,"
> > + "p4-pre-submit,"
> > + "post-index-changex";
>
> Well, this is disappointing: I tried to come up with a scripted way to
> extract the commit names from our source code, and I failed! This is
> where I gave up:
>
> git grep run_hook |
> sed -n 's/.*run_hook[^(]*([^,]*, *\([^,]*\).*/\1/p' |
> sed -e '/^name$/d' -e '/^const char \*name$/d' \
> -e 's/push_to_checkout_hook/"push-to-checkout"/' |
> sort |
> uniq
>
> This prints only the following hook names:
>
> "applypatch-msg"
> "post-applypatch"
> "post-checkout"
> "post-index-change"
> "post-merge"
> "pre-applypatch"
> "pre-auto-gc"
> "pre-rebase"
> "prepare-commit-msg"
> "push-to-checkout"
>
> But at least it was easy to script the extracting from the
> Documentation:
>
> sed -n '/^[a-z]/{N;/\n~~~/{s/\n.*//;p}}' \
> Documentation/githooks.txt
>
To be totally frank, I'm not keen on spending a lot of time with
scripting this. My parallel work with config-based hooks will also
involve an in-code source of truth for available hooknames; I'd like to
punt on some scripting, putting it in the makefile, etc blah since I
know I'm planning to fix this particular annoyance at the source - and
since it looks like bugreport will stay in the review phase for at least
a little longer.
> > + struct string_list hooks_list = STRING_LIST_INIT_DUP;
> > + struct string_list_item *iter = NULL;
> > +
> > + strbuf_reset(hook_info);
> > +
> > + string_list_split(&hooks_list, hooks, ',', -1);
> > +
> > + for_each_string_list_item(iter, &hooks_list) {
>
> This should definitely be done at compile time, I think. We should be
> able to generate an appropriate header via something like this:
>
> cat >hook-names.h <<-EOF
> static const char *hook_names[] = {
> $(sed -n '/^[a-z]/{N;/\n~~~/{s/\n.*/",/;s/^/ "/;p}}' \
> Documentation/githooks.txt)
> };
> EOF
>
> Then you would use a simple `for()` loop using `ARRAY_SIZE()` to
> iterate over the hook names.
>
> > + if (find_hook(iter->string)) {
> > + strbuf_addstr(hook_info, iter->string);
> > + strbuf_complete_line(hook_info);
> > + }
> > + }
> > +}
> > diff --git a/bugreport.h b/bugreport.h
> > index 7413e7e1be..942a5436e3 100644
> > --- a/bugreport.h
> > +++ b/bugreport.h
> > @@ -12,3 +12,9 @@ void get_system_info(struct strbuf *sys_info);
> > * config_info will be discarded.
> > */
> > void get_whitelisted_config(struct strbuf *sys_info);
> > +
> > +/**
> > + * Adds the paths to all configured hooks (but not their contents). The previous
> > + * contents of hook_info will be discarded.
> > + */
> > +void get_populated_hooks(struct strbuf *hook_info);
> > diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> > index 70fe0d2b85..a0eefba498 100644
> > --- a/builtin/bugreport.c
> > +++ b/builtin/bugreport.c
> > @@ -60,6 +60,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> > get_whitelisted_config(&buffer);
> > strbuf_write(&buffer, report);
> >
> > + add_header(report, "Populated Hooks");
>
> Wait! I should have stumbled over this in an earlier patch. The
> `add_header()` function should not take a `FILE *` parameter at all, but
> instead an `struct strbuf *` one!
>
> Ciao,
> Dscho
>
> > + get_populated_hooks(&buffer);
> > + strbuf_write(&buffer, report);
> > +
> > fclose(report);
> >
> > launch_editor(report_path.buf, NULL, NULL);
> > --
> > 2.24.0.rc0.303.g954a862665-goog
> >
> >
^ permalink raw reply [flat|nested] 68+ messages in thread* Re: [PATCH v3 5/9] bugreport: collect list of populated hooks
2019-12-11 20:51 ` Emily Shaffer
@ 2019-12-15 17:40 ` Johannes Schindelin
0 siblings, 0 replies; 68+ messages in thread
From: Johannes Schindelin @ 2019-12-15 17:40 UTC (permalink / raw)
To: Emily Shaffer; +Cc: git
Hi Emily,
On Wed, 11 Dec 2019, Emily Shaffer wrote:
> On Mon, Oct 28, 2019 at 03:31:36PM +0100, Johannes Schindelin wrote:
>
> > On Thu, 24 Oct 2019, Emily Shaffer wrote:
> >
> > > diff --git a/bugreport.c b/bugreport.c
> > > index afa4836ab1..9d7f44ff28 100644
> > > --- a/bugreport.c
> > > +++ b/bugreport.c
> > > @@ -103,3 +103,47 @@ void get_whitelisted_config(struct strbuf *config_info)
> > > strbuf_reset(config_info);
> > > strbuf_addbuf(config_info, &configs_and_values);
> > > }
> > > +
> > > +void get_populated_hooks(struct strbuf *hook_info)
> > > +{
> > > + /*
> > > + * Doesn't look like there is a list of all possible hooks; so below is
> > > + * a transcription of `git help hook`.
> > > + */
> > > + const char *hooks = "applypatch-msg,"
> > > + "pre-applypatch,"
> > > + "post-applypatch,"
> > > + "pre-commit,"
> > > + "pre-merge-commit,"
> > > + "prepare-commit-msg,"
> > > + "commit-msg,"
> > > + "post-commit,"
> > > + "pre-rebase,"
> > > + "post-checkout,"
> > > + "post-merge,"
> > > + "pre-push,"
> > > + "pre-receive,"
> > > + "update,"
> > > + "post-receive,"
> > > + "post-update,"
> > > + "push-to-checkout,"
> > > + "pre-auto-gc,"
> > > + "post-rewrite,"
> > > + "sendemail-validate,"
> > > + "fsmonitor-watchman,"
> > > + "p4-pre-submit,"
> > > + "post-index-changex";
> >
> > Well, this is disappointing: I tried to come up with a scripted way to
> > extract the commit names from our source code, and I failed! This is
> > where I gave up:
> >
> > git grep run_hook |
> > sed -n 's/.*run_hook[^(]*([^,]*, *\([^,]*\).*/\1/p' |
> > sed -e '/^name$/d' -e '/^const char \*name$/d' \
> > -e 's/push_to_checkout_hook/"push-to-checkout"/' |
> > sort |
> > uniq
> >
> > This prints only the following hook names:
> >
> > "applypatch-msg"
> > "post-applypatch"
> > "post-checkout"
> > "post-index-change"
> > "post-merge"
> > "pre-applypatch"
> > "pre-auto-gc"
> > "pre-rebase"
> > "prepare-commit-msg"
> > "push-to-checkout"
> >
> > But at least it was easy to script the extracting from the
> > Documentation:
> >
> > sed -n '/^[a-z]/{N;/\n~~~/{s/\n.*//;p}}' \
> > Documentation/githooks.txt
> >
>
> To be totally frank, I'm not keen on spending a lot of time with
> scripting this. My parallel work with config-based hooks will also
> involve an in-code source of truth for available hooknames; I'd like to
> punt on some scripting, putting it in the makefile, etc blah since I
> know I'm planning to fix this particular annoyance at the source - and
> since it looks like bugreport will stay in the review phase for at least
> a little longer.
Fair enough, especially if it addresses my complaint about the
scriptability.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 68+ messages in thread
* [PATCH v3 6/9] bugreport: count loose objects
2019-10-25 2:51 ` [PATCH v3 0/9] add git-bugreport tool Emily Shaffer
` (4 preceding siblings ...)
2019-10-25 2:51 ` [PATCH v3 5/9] bugreport: collect list of populated hooks Emily Shaffer
@ 2019-10-25 2:51 ` Emily Shaffer
2019-10-28 15:07 ` Johannes Schindelin
2019-10-29 21:18 ` Josh Steadmon
2019-10-25 2:51 ` [PATCH v3 7/9] bugreport: add packed object summary Emily Shaffer
` (3 subsequent siblings)
9 siblings, 2 replies; 68+ messages in thread
From: Emily Shaffer @ 2019-10-25 2:51 UTC (permalink / raw)
To: git; +Cc: Emily Shaffer
The number of unpacked objects in a user's repository may help us
understand the root of the problem they're seeing, especially if a
command is running unusually slowly.
Rather than directly invoking 'git-count-objects', which may sometimes
fail unexpectedly on Git for Windows, manually count the contents of
.git/objects. Additionally, since we may wish to inspect other
directories' contents for bugreport in the future, put the directory
listing into a helper function.
Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
bugreport.c | 72 +++++++++++++++++++++++++++++++++++++++++++++
bugreport.h | 6 ++++
builtin/bugreport.c | 4 +++
3 files changed, 82 insertions(+)
diff --git a/bugreport.c b/bugreport.c
index 9d7f44ff28..54e1d47103 100644
--- a/bugreport.c
+++ b/bugreport.c
@@ -5,8 +5,11 @@
#include "exec-cmd.h"
#include "help.h"
#include "run-command.h"
+#include "strbuf.h"
#include "version.h"
+#include "dirent.h"
+
/**
* A sorted list of config options which we will add to the bugreport. Managed
* by 'gather_whitelist(...)'.
@@ -147,3 +150,72 @@ void get_populated_hooks(struct strbuf *hook_info)
}
}
}
+
+/**
+ * Fill 'contents' with the contents of the dir at 'dirpath'.
+ * If 'filter' is nonzero, the contents are filtered on d_type as 'type' - see
+ * 'man readdir'. opendir() doesn't take string length as an arg, so don't
+ * bother passing it in.
+ */
+void list_contents_of_dir(struct string_list *contents, struct strbuf *dirpath,
+ int filter, unsigned char type)
+{
+ struct dirent *dir = NULL;
+ DIR *dh = NULL;
+
+ dh = opendir(dirpath->buf);
+ while (dh && (dir = readdir(dh))) {
+ if (!filter || type == dir->d_type) {
+ string_list_append(contents, dir->d_name);
+ }
+ }
+}
+
+
+void get_object_counts(struct strbuf *obj_info)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+ struct strbuf std_out = STRBUF_INIT;
+
+ argv_array_push(&cp.args, "count-objects");
+ argv_array_push(&cp.args, "-vH");
+ cp.git_cmd = 1;
+ capture_command(&cp, &std_out, 0);
+
+ strbuf_reset(obj_info);
+ strbuf_addstr(obj_info, "git-count-objects -vH:\n");
+ strbuf_addbuf(obj_info, &std_out);
+}
+
+void get_loose_object_summary(struct strbuf *obj_info)
+{
+ struct strbuf dirpath = STRBUF_INIT;
+ struct string_list subdirs = STRING_LIST_INIT_DUP;
+ struct string_list_item *subdir;
+
+ strbuf_reset(obj_info);
+
+ strbuf_addstr(&dirpath, get_object_directory());
+ strbuf_complete(&dirpath, '/');
+
+ list_contents_of_dir(&subdirs, &dirpath, 1, DT_DIR);
+
+ for_each_string_list_item(subdir, &subdirs)
+ {
+ struct strbuf subdir_buf = STRBUF_INIT;
+ struct string_list objects = STRING_LIST_INIT_DUP;
+
+ /*
+ * Only interested in loose objects - so dirs named with the
+ * first byte of the object ID
+ */
+ if (strlen(subdir->string) != 2 || !strcmp(subdir->string, ".."))
+ continue;
+
+ strbuf_addbuf(&subdir_buf, &dirpath);
+ strbuf_addstr(&subdir_buf, subdir->string);
+ list_contents_of_dir(&objects, &subdir_buf, 0, 0);
+ strbuf_addf(obj_info, "%s: %d objects\n", subdir->string,
+ objects.nr);
+ }
+}
diff --git a/bugreport.h b/bugreport.h
index 942a5436e3..09ad0c2599 100644
--- a/bugreport.h
+++ b/bugreport.h
@@ -18,3 +18,9 @@ void get_whitelisted_config(struct strbuf *sys_info);
* contents of hook_info will be discarded.
*/
void get_populated_hooks(struct strbuf *hook_info);
+
+/**
+ * Adds the output of `git count-object -vH`. The previous contents of hook_info
+ * will be discarded.
+ */
+void get_loose_object_summary(struct strbuf *obj_info);
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index a0eefba498..b2ab194207 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -64,6 +64,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
get_populated_hooks(&buffer);
strbuf_write(&buffer, report);
+ add_header(report, "Object Counts");
+ get_loose_object_summary(&buffer);
+ strbuf_write(&buffer, report);
+
fclose(report);
launch_editor(report_path.buf, NULL, NULL);
--
2.24.0.rc0.303.g954a862665-goog
^ permalink raw reply related [flat|nested] 68+ messages in thread* Re: [PATCH v3 6/9] bugreport: count loose objects
2019-10-25 2:51 ` [PATCH v3 6/9] bugreport: count loose objects Emily Shaffer
@ 2019-10-28 15:07 ` Johannes Schindelin
2019-12-10 22:34 ` Emily Shaffer
2019-10-29 21:18 ` Josh Steadmon
1 sibling, 1 reply; 68+ messages in thread
From: Johannes Schindelin @ 2019-10-28 15:07 UTC (permalink / raw)
To: Emily Shaffer; +Cc: git
Hi Emily,
On Thu, 24 Oct 2019, Emily Shaffer wrote:
> The number of unpacked objects in a user's repository may help us
> understand the root of the problem they're seeing, especially if a
> command is running unusually slowly.
>
> Rather than directly invoking 'git-count-objects', which may sometimes
> fail unexpectedly on Git for Windows, manually count the contents of
> .git/objects. Additionally, since we may wish to inspect other
> directories' contents for bugreport in the future, put the directory
> listing into a helper function.
Thank you, much appreciated!
I guess the next step is to count the number of packs, and the number of
submodules ;-)
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
> bugreport.c | 72 +++++++++++++++++++++++++++++++++++++++++++++
> bugreport.h | 6 ++++
> builtin/bugreport.c | 4 +++
> 3 files changed, 82 insertions(+)
>
> diff --git a/bugreport.c b/bugreport.c
> index 9d7f44ff28..54e1d47103 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -5,8 +5,11 @@
> #include "exec-cmd.h"
> #include "help.h"
> #include "run-command.h"
> +#include "strbuf.h"
Why not append this to the end of the `#include` list, as is common in
Git's commit history?
> #include "version.h"
>
> +#include "dirent.h"
This header (although with pointy brackets instead of double quotes) is
already included in `git-compat-util.h`
> +
> /**
> * A sorted list of config options which we will add to the bugreport. Managed
> * by 'gather_whitelist(...)'.
> @@ -147,3 +150,72 @@ void get_populated_hooks(struct strbuf *hook_info)
> }
> }
> }
> +
> +/**
> + * Fill 'contents' with the contents of the dir at 'dirpath'.
Since you start this comment in JavaDoc style, there should be an almost
empty line after this one ("almost" because it still contains the
asterisk, of course).
> + * If 'filter' is nonzero, the contents are filtered on d_type as 'type' - see
> + * 'man readdir'. opendir() doesn't take string length as an arg, so don't
> + * bother passing it in.
> + */
> +void list_contents_of_dir(struct string_list *contents, struct strbuf *dirpath,
Shouldn't this be `static`?
> + int filter, unsigned char type)
> +{
> + struct dirent *dir = NULL;
> + DIR *dh = NULL;
> +
> + dh = opendir(dirpath->buf);
> + while (dh && (dir = readdir(dh))) {
> + if (!filter || type == dir->d_type) {
> + string_list_append(contents, dir->d_name);
> + }
> + }
> +}
> +
> +
> +void get_object_counts(struct strbuf *obj_info)
Oops. This function is no longer used.
> +{
> + struct child_process cp = CHILD_PROCESS_INIT;
> + struct strbuf std_out = STRBUF_INIT;
> +
> + argv_array_push(&cp.args, "count-objects");
> + argv_array_push(&cp.args, "-vH");
> + cp.git_cmd = 1;
> + capture_command(&cp, &std_out, 0);
> +
> + strbuf_reset(obj_info);
> + strbuf_addstr(obj_info, "git-count-objects -vH:\n");
> + strbuf_addbuf(obj_info, &std_out);
> +}
> +
> +void get_loose_object_summary(struct strbuf *obj_info)
> +{
> + struct strbuf dirpath = STRBUF_INIT;
> + struct string_list subdirs = STRING_LIST_INIT_DUP;
> + struct string_list_item *subdir;
> +
> + strbuf_reset(obj_info);
> +
> + strbuf_addstr(&dirpath, get_object_directory());
> + strbuf_complete(&dirpath, '/');
> +
> + list_contents_of_dir(&subdirs, &dirpath, 1, DT_DIR);
> +
> + for_each_string_list_item(subdir, &subdirs)
> + {
> + struct strbuf subdir_buf = STRBUF_INIT;
> + struct string_list objects = STRING_LIST_INIT_DUP;
> +
> + /*
> + * Only interested in loose objects - so dirs named with the
> + * first byte of the object ID
> + */
> + if (strlen(subdir->string) != 2 || !strcmp(subdir->string, ".."))
> + continue;
> +
> + strbuf_addbuf(&subdir_buf, &dirpath);
> + strbuf_addstr(&subdir_buf, subdir->string);
> + list_contents_of_dir(&objects, &subdir_buf, 0, 0);
> + strbuf_addf(obj_info, "%s: %d objects\n", subdir->string,
> + objects.nr);
Hmm. Not only does this leak `objects`, it also throws away the contents
that we so painfully constructed.
Wouldn't it make more sense to do something like this instead?
static int is_hex(const char *string, size_t count)
{
for (; count; string++, count--)
if (hexval(*string) < 0)
return 0;
return 1;
}
static ssize_t count_loose_objects(struct strbuf *objects_path)
{
ssize_t ret = 0;
size_t len;
struct dirent *d;
DIR *dir, *subdir;
dir = opendir(objects_path->buf);
if (!dir)
return -1;
strbuf_complete(objects_path, '/');
len = objects_path->len;
while ((d = readdir(dir))) {
if (d->d_type != DT_DIR)
continue;
strbuf_setlen(objects_path, len);
strbuf_addstr(objects_path, d->d_name);
subdir = opendir(objects_path->buf);
if (!subdir)
continue;
while ((d = readdir(subdir)))
if (d->dt_type == DT_REG &&
is_hex(dir->d_name, the_repository->hash_algo->hexsz))
ret++;
closedir(subdir);
}
closedir(dir);
strbuf_reset(objects_path, len);
return ret;
}
Ciao,
Dscho
> + }
> +}
> diff --git a/bugreport.h b/bugreport.h
> index 942a5436e3..09ad0c2599 100644
> --- a/bugreport.h
> +++ b/bugreport.h
> @@ -18,3 +18,9 @@ void get_whitelisted_config(struct strbuf *sys_info);
> * contents of hook_info will be discarded.
> */
> void get_populated_hooks(struct strbuf *hook_info);
> +
> +/**
> + * Adds the output of `git count-object -vH`. The previous contents of hook_info
> + * will be discarded.
> + */
> +void get_loose_object_summary(struct strbuf *obj_info);
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index a0eefba498..b2ab194207 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -64,6 +64,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> get_populated_hooks(&buffer);
> strbuf_write(&buffer, report);
>
> + add_header(report, "Object Counts");
> + get_loose_object_summary(&buffer);
> + strbuf_write(&buffer, report);
> +
> fclose(report);
>
> launch_editor(report_path.buf, NULL, NULL);
> --
> 2.24.0.rc0.303.g954a862665-goog
>
>
^ permalink raw reply [flat|nested] 68+ messages in thread* Re: [PATCH v3 6/9] bugreport: count loose objects
2019-10-28 15:07 ` Johannes Schindelin
@ 2019-12-10 22:34 ` Emily Shaffer
0 siblings, 0 replies; 68+ messages in thread
From: Emily Shaffer @ 2019-12-10 22:34 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
On Mon, Oct 28, 2019 at 04:07:40PM +0100, Johannes Schindelin wrote:
> Hi Emily,
>
> On Thu, 24 Oct 2019, Emily Shaffer wrote:
>
> > The number of unpacked objects in a user's repository may help us
> > understand the root of the problem they're seeing, especially if a
> > command is running unusually slowly.
> >
> > Rather than directly invoking 'git-count-objects', which may sometimes
> > fail unexpectedly on Git for Windows, manually count the contents of
> > .git/objects. Additionally, since we may wish to inspect other
> > directories' contents for bugreport in the future, put the directory
> > listing into a helper function.
>
> Thank you, much appreciated!
>
> I guess the next step is to count the number of packs, and the number of
> submodules ;-)
>
> >
> > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > ---
> > bugreport.c | 72 +++++++++++++++++++++++++++++++++++++++++++++
> > bugreport.h | 6 ++++
> > builtin/bugreport.c | 4 +++
> > 3 files changed, 82 insertions(+)
> >
> > diff --git a/bugreport.c b/bugreport.c
> > index 9d7f44ff28..54e1d47103 100644
> > --- a/bugreport.c
> > +++ b/bugreport.c
> > @@ -5,8 +5,11 @@
> > #include "exec-cmd.h"
> > #include "help.h"
> > #include "run-command.h"
> > +#include "strbuf.h"
>
> Why not append this to the end of the `#include` list, as is common in
> Git's commit history?
>
> > #include "version.h"
> >
> > +#include "dirent.h"
>
> This header (although with pointy brackets instead of double quotes) is
> already included in `git-compat-util.h`
>
> > +
> > /**
> > * A sorted list of config options which we will add to the bugreport. Managed
> > * by 'gather_whitelist(...)'.
> > @@ -147,3 +150,72 @@ void get_populated_hooks(struct strbuf *hook_info)
> > }
> > }
> > }
> > +
> > +/**
> > + * Fill 'contents' with the contents of the dir at 'dirpath'.
>
> Since you start this comment in JavaDoc style, there should be an almost
> empty line after this one ("almost" because it still contains the
> asterisk, of course).
>
> > + * If 'filter' is nonzero, the contents are filtered on d_type as 'type' - see
> > + * 'man readdir'. opendir() doesn't take string length as an arg, so don't
> > + * bother passing it in.
> > + */
> > +void list_contents_of_dir(struct string_list *contents, struct strbuf *dirpath,
>
> Shouldn't this be `static`?
>
> > + int filter, unsigned char type)
> > +{
> > + struct dirent *dir = NULL;
> > + DIR *dh = NULL;
> > +
> > + dh = opendir(dirpath->buf);
> > + while (dh && (dir = readdir(dh))) {
> > + if (!filter || type == dir->d_type) {
> > + string_list_append(contents, dir->d_name);
> > + }
> > + }
> > +}
> > +
> > +
> > +void get_object_counts(struct strbuf *obj_info)
>
> Oops. This function is no longer used.
>
> > +{
> > + struct child_process cp = CHILD_PROCESS_INIT;
> > + struct strbuf std_out = STRBUF_INIT;
> > +
> > + argv_array_push(&cp.args, "count-objects");
> > + argv_array_push(&cp.args, "-vH");
> > + cp.git_cmd = 1;
> > + capture_command(&cp, &std_out, 0);
> > +
> > + strbuf_reset(obj_info);
> > + strbuf_addstr(obj_info, "git-count-objects -vH:\n");
> > + strbuf_addbuf(obj_info, &std_out);
> > +}
> > +
> > +void get_loose_object_summary(struct strbuf *obj_info)
> > +{
> > + struct strbuf dirpath = STRBUF_INIT;
> > + struct string_list subdirs = STRING_LIST_INIT_DUP;
> > + struct string_list_item *subdir;
> > +
> > + strbuf_reset(obj_info);
> > +
> > + strbuf_addstr(&dirpath, get_object_directory());
> > + strbuf_complete(&dirpath, '/');
> > +
> > + list_contents_of_dir(&subdirs, &dirpath, 1, DT_DIR);
> > +
> > + for_each_string_list_item(subdir, &subdirs)
> > + {
> > + struct strbuf subdir_buf = STRBUF_INIT;
> > + struct string_list objects = STRING_LIST_INIT_DUP;
> > +
> > + /*
> > + * Only interested in loose objects - so dirs named with the
> > + * first byte of the object ID
> > + */
> > + if (strlen(subdir->string) != 2 || !strcmp(subdir->string, ".."))
> > + continue;
> > +
> > + strbuf_addbuf(&subdir_buf, &dirpath);
> > + strbuf_addstr(&subdir_buf, subdir->string);
> > + list_contents_of_dir(&objects, &subdir_buf, 0, 0);
> > + strbuf_addf(obj_info, "%s: %d objects\n", subdir->string,
> > + objects.nr);
>
> Hmm. Not only does this leak `objects`, it also throws away the contents
> that we so painfully constructed.
>
> Wouldn't it make more sense to do something like this instead?
>
> static int is_hex(const char *string, size_t count)
> {
> for (; count; string++, count--)
> if (hexval(*string) < 0)
> return 0;
> return 1;
> }
True if the string matches [0-9a-fA-F]*, false otherwise.
>
> static ssize_t count_loose_objects(struct strbuf *objects_path)
> {
> ssize_t ret = 0;
> size_t len;
> struct dirent *d;
> DIR *dir, *subdir;
>
> dir = opendir(objects_path->buf);
> if (!dir)
> return -1;
>
> strbuf_complete(objects_path, '/');
starting with .git/objects/...
> len = objects_path->len;
> while ((d = readdir(dir))) {
For all contents of dir...
> if (d->d_type != DT_DIR)
> continue;
..which are directories...
> strbuf_setlen(objects_path, len);
> strbuf_addstr(objects_path, d->d_name);
> subdir = opendir(objects_path->buf);
show all the contents of that subdirectory.
> if (!subdir)
> continue;
> while ((d = readdir(subdir)))
for each regular file there,
> if (d->dt_type == DT_REG &&
if it's a regular file,
> is_hex(dir->d_name, the_repository->hash_algo->hexsz))
and the directory is named like an object prefix,
> ret++;
increase the total count of numbers of loose objects.
> closedir(subdir);
> }
> closedir(dir);
> strbuf_reset(objects_path, len);
> return ret;
> }
The foremost difference here is that the loose object count previously
was not given in total - instead, it was divided up by object prefix. I
can't speak to whether that's actually very important to know about, but
the original request from stolee was to have the summary by dirname.
That's certainly still possible with a light modification to this code.
(Suggestion from Stolee, some months ago earlier in this thread:)
As mentioned before, I've sometimes found it helpful to know the data shape for the object
store. Having a few extra steps such as the following could be nice:
echo "[Loose Objects]"
for objdir in $(find "$GIT_DIR/objects/??" -type d)
do
echo "$objdir: $(ls $objdir | wc -l)"
done
echo
...
Checking that the directory we're about to inspect is hexval rather than
that it's only two characters long is also an interesting point. I'd
probably rather do both, though, since I think we both missed
futureproofing by a little bit.
Dropping the many string_list is fine by me - call it object-oriented
habits dying hard.
I worry somewhat on delving into every directory and only afterwards
checking whether the directory is one we care about, but that's an easy
modification too to your basic suggestion ("don't use a string list for
that, silly").
Thanks. I'll make the changes with the modifications I mentioned and add
your Helped-by line on this commit.
- Emily
>
> Ciao,
> Dscho
>
> > + }
> > +}
> > diff --git a/bugreport.h b/bugreport.h
> > index 942a5436e3..09ad0c2599 100644
> > --- a/bugreport.h
> > +++ b/bugreport.h
> > @@ -18,3 +18,9 @@ void get_whitelisted_config(struct strbuf *sys_info);
> > * contents of hook_info will be discarded.
> > */
> > void get_populated_hooks(struct strbuf *hook_info);
> > +
> > +/**
> > + * Adds the output of `git count-object -vH`. The previous contents of hook_info
> > + * will be discarded.
> > + */
> > +void get_loose_object_summary(struct strbuf *obj_info);
> > diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> > index a0eefba498..b2ab194207 100644
> > --- a/builtin/bugreport.c
> > +++ b/builtin/bugreport.c
> > @@ -64,6 +64,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> > get_populated_hooks(&buffer);
> > strbuf_write(&buffer, report);
> >
> > + add_header(report, "Object Counts");
> > + get_loose_object_summary(&buffer);
> > + strbuf_write(&buffer, report);
> > +
> > fclose(report);
> >
> > launch_editor(report_path.buf, NULL, NULL);
> > --
> > 2.24.0.rc0.303.g954a862665-goog
> >
> >
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v3 6/9] bugreport: count loose objects
2019-10-25 2:51 ` [PATCH v3 6/9] bugreport: count loose objects Emily Shaffer
2019-10-28 15:07 ` Johannes Schindelin
@ 2019-10-29 21:18 ` Josh Steadmon
1 sibling, 0 replies; 68+ messages in thread
From: Josh Steadmon @ 2019-10-29 21:18 UTC (permalink / raw)
To: Emily Shaffer; +Cc: git
On 2019.10.24 19:51, Emily Shaffer wrote:
[snip]
> diff --git a/bugreport.h b/bugreport.h
> index 942a5436e3..09ad0c2599 100644
> --- a/bugreport.h
> +++ b/bugreport.h
> @@ -18,3 +18,9 @@ void get_whitelisted_config(struct strbuf *sys_info);
> * contents of hook_info will be discarded.
> */
> void get_populated_hooks(struct strbuf *hook_info);
> +
> +/**
> + * Adds the output of `git count-object -vH`. The previous contents of hook_info
> + * will be discarded.
> + */
> +void get_loose_object_summary(struct strbuf *obj_info);
Looks like a copy/paste typo here, shouldn't "hook_info" be "obj_info"
in the comment?
^ permalink raw reply [flat|nested] 68+ messages in thread
* [PATCH v3 7/9] bugreport: add packed object summary
2019-10-25 2:51 ` [PATCH v3 0/9] add git-bugreport tool Emily Shaffer
` (5 preceding siblings ...)
2019-10-25 2:51 ` [PATCH v3 6/9] bugreport: count loose objects Emily Shaffer
@ 2019-10-25 2:51 ` Emily Shaffer
2019-10-28 15:43 ` Johannes Schindelin
2019-10-25 2:51 ` [PATCH v3 8/9] bugreport: list contents of $OBJDIR/info Emily Shaffer
` (2 subsequent siblings)
9 siblings, 1 reply; 68+ messages in thread
From: Emily Shaffer @ 2019-10-25 2:51 UTC (permalink / raw)
To: git; +Cc: Emily Shaffer
Alongside the list of loose objects, it's useful to see the list of
object packs as well. It can help us to examine what Git did and did not
pack.
Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
bugreport.c | 21 +++++++++++++++++++++
bugreport.h | 6 ++++++
builtin/bugreport.c | 4 ++++
3 files changed, 31 insertions(+)
diff --git a/bugreport.c b/bugreport.c
index 54e1d47103..79ddb8baaa 100644
--- a/bugreport.c
+++ b/bugreport.c
@@ -219,3 +219,24 @@ void get_loose_object_summary(struct strbuf *obj_info)
objects.nr);
}
}
+
+void get_packed_object_summary(struct strbuf *obj_info)
+{
+ struct strbuf dirpath = STRBUF_INIT;
+ struct string_list contents = STRING_LIST_INIT_DUP;
+ struct string_list_item *entry;
+
+ strbuf_reset(obj_info);
+
+ strbuf_addstr(&dirpath, get_object_directory());
+ strbuf_complete(&dirpath, '/');
+ strbuf_addstr(&dirpath, "pack/");
+ list_contents_of_dir(&contents, &dirpath, 0, 0);
+
+ // list full contents of $GIT_OBJECT_DIRECTORY/pack/
+ for_each_string_list_item(entry, &contents) {
+ strbuf_addbuf(obj_info, &dirpath);
+ strbuf_addstr(obj_info, entry->string);
+ strbuf_complete_line(obj_info);
+ }
+}
diff --git a/bugreport.h b/bugreport.h
index 09ad0c2599..11ff7df41b 100644
--- a/bugreport.h
+++ b/bugreport.h
@@ -24,3 +24,9 @@ void get_populated_hooks(struct strbuf *hook_info);
* will be discarded.
*/
void get_loose_object_summary(struct strbuf *obj_info);
+
+/**
+ * Adds a list of the contents of '.git/objects/pack'. The previous contents of
+ * hook_info will be discarded.
+ */
+void get_packed_object_summary(struct strbuf *obj_info);
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index b2ab194207..da91a3944e 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -68,6 +68,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
get_loose_object_summary(&buffer);
strbuf_write(&buffer, report);
+ add_header(report, "Packed Object Summary");
+ get_packed_object_summary(&buffer);
+ strbuf_write(&buffer, report);
+
fclose(report);
launch_editor(report_path.buf, NULL, NULL);
--
2.24.0.rc0.303.g954a862665-goog
^ permalink raw reply related [flat|nested] 68+ messages in thread* Re: [PATCH v3 7/9] bugreport: add packed object summary
2019-10-25 2:51 ` [PATCH v3 7/9] bugreport: add packed object summary Emily Shaffer
@ 2019-10-28 15:43 ` Johannes Schindelin
2019-12-11 0:29 ` Emily Shaffer
0 siblings, 1 reply; 68+ messages in thread
From: Johannes Schindelin @ 2019-10-28 15:43 UTC (permalink / raw)
To: Emily Shaffer; +Cc: git
Hi Emily,
On Thu, 24 Oct 2019, Emily Shaffer wrote:
> Alongside the list of loose objects, it's useful to see the list of
> object packs as well. It can help us to examine what Git did and did not
> pack.
Yes, I was write! Packs are next ;-)
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
> bugreport.c | 21 +++++++++++++++++++++
> bugreport.h | 6 ++++++
> builtin/bugreport.c | 4 ++++
> 3 files changed, 31 insertions(+)
>
> diff --git a/bugreport.c b/bugreport.c
> index 54e1d47103..79ddb8baaa 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -219,3 +219,24 @@ void get_loose_object_summary(struct strbuf *obj_info)
> objects.nr);
> }
> }
> +
> +void get_packed_object_summary(struct strbuf *obj_info)
> +{
> + struct strbuf dirpath = STRBUF_INIT;
> + struct string_list contents = STRING_LIST_INIT_DUP;
> + struct string_list_item *entry;
> +
> + strbuf_reset(obj_info);
> +
> + strbuf_addstr(&dirpath, get_object_directory());
> + strbuf_complete(&dirpath, '/');
> + strbuf_addstr(&dirpath, "pack/");
> + list_contents_of_dir(&contents, &dirpath, 0, 0);
> +
> + // list full contents of $GIT_OBJECT_DIRECTORY/pack/
> + for_each_string_list_item(entry, &contents) {
> + strbuf_addbuf(obj_info, &dirpath);
> + strbuf_addstr(obj_info, entry->string);
> + strbuf_complete_line(obj_info);
> + }
> +}
Okay, but I think that you will want to discern between regular `.pack`
files, `.idx` files and `tmp_*` files.
> diff --git a/bugreport.h b/bugreport.h
> index 09ad0c2599..11ff7df41b 100644
> --- a/bugreport.h
> +++ b/bugreport.h
> @@ -24,3 +24,9 @@ void get_populated_hooks(struct strbuf *hook_info);
> * will be discarded.
> */
> void get_loose_object_summary(struct strbuf *obj_info);
> +
> +/**
> + * Adds a list of the contents of '.git/objects/pack'. The previous contents of
> + * hook_info will be discarded.
> + */
> +void get_packed_object_summary(struct strbuf *obj_info);
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index b2ab194207..da91a3944e 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -68,6 +68,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> get_loose_object_summary(&buffer);
> strbuf_write(&buffer, report);
>
> + add_header(report, "Packed Object Summary");
> + get_packed_object_summary(&buffer);
> + strbuf_write(&buffer, report);
> +
Hmm. At this point, I am unclear whether you want to write into an
`strbuf`, or directly into a `FILE *`? I would rather have only one, not
a mix.
Ciao,
Dscho
> fclose(report);
>
> launch_editor(report_path.buf, NULL, NULL);
> --
> 2.24.0.rc0.303.g954a862665-goog
>
>
^ permalink raw reply [flat|nested] 68+ messages in thread* Re: [PATCH v3 7/9] bugreport: add packed object summary
2019-10-28 15:43 ` Johannes Schindelin
@ 2019-12-11 0:29 ` Emily Shaffer
2019-12-11 13:37 ` Johannes Schindelin
0 siblings, 1 reply; 68+ messages in thread
From: Emily Shaffer @ 2019-12-11 0:29 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
On Mon, Oct 28, 2019 at 04:43:50PM +0100, Johannes Schindelin wrote:
> Hi Emily,
>
> On Thu, 24 Oct 2019, Emily Shaffer wrote:
>
> > Alongside the list of loose objects, it's useful to see the list of
> > object packs as well. It can help us to examine what Git did and did not
> > pack.
>
> Yes, I was write! Packs are next ;-)
>
> >
> > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > ---
> > bugreport.c | 21 +++++++++++++++++++++
> > bugreport.h | 6 ++++++
> > builtin/bugreport.c | 4 ++++
> > 3 files changed, 31 insertions(+)
> >
> > diff --git a/bugreport.c b/bugreport.c
> > index 54e1d47103..79ddb8baaa 100644
> > --- a/bugreport.c
> > +++ b/bugreport.c
> > @@ -219,3 +219,24 @@ void get_loose_object_summary(struct strbuf *obj_info)
> > objects.nr);
> > }
> > }
> > +
> > +void get_packed_object_summary(struct strbuf *obj_info)
> > +{
> > + struct strbuf dirpath = STRBUF_INIT;
> > + struct string_list contents = STRING_LIST_INIT_DUP;
> > + struct string_list_item *entry;
> > +
> > + strbuf_reset(obj_info);
> > +
> > + strbuf_addstr(&dirpath, get_object_directory());
> > + strbuf_complete(&dirpath, '/');
> > + strbuf_addstr(&dirpath, "pack/");
> > + list_contents_of_dir(&contents, &dirpath, 0, 0);
> > +
> > + // list full contents of $GIT_OBJECT_DIRECTORY/pack/
> > + for_each_string_list_item(entry, &contents) {
> > + strbuf_addbuf(obj_info, &dirpath);
> > + strbuf_addstr(obj_info, entry->string);
> > + strbuf_complete_line(obj_info);
> > + }
> > +}
>
> Okay, but I think that you will want to discern between regular `.pack`
> files, `.idx` files and `tmp_*` files.
Discern in what way? How would you like to see them treated separately?
They're all being listed here, not just counted, so it seems to me like
I can read the generated bugreport and see which files are index, pack,
or temporary here.
>
> > diff --git a/bugreport.h b/bugreport.h
> > index 09ad0c2599..11ff7df41b 100644
> > --- a/bugreport.h
> > +++ b/bugreport.h
> > @@ -24,3 +24,9 @@ void get_populated_hooks(struct strbuf *hook_info);
> > * will be discarded.
> > */
> > void get_loose_object_summary(struct strbuf *obj_info);
> > +
> > +/**
> > + * Adds a list of the contents of '.git/objects/pack'. The previous contents of
> > + * hook_info will be discarded.
> > + */
> > +void get_packed_object_summary(struct strbuf *obj_info);
> > diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> > index b2ab194207..da91a3944e 100644
> > --- a/builtin/bugreport.c
> > +++ b/builtin/bugreport.c
> > @@ -68,6 +68,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> > get_loose_object_summary(&buffer);
> > strbuf_write(&buffer, report);
> >
> > + add_header(report, "Packed Object Summary");
> > + get_packed_object_summary(&buffer);
> > + strbuf_write(&buffer, report);
> > +
>
> Hmm. At this point, I am unclear whether you want to write into an
> `strbuf`, or directly into a `FILE *`? I would rather have only one, not
> a mix.
>
> Ciao,
> Dscho
>
> > fclose(report);
> >
> > launch_editor(report_path.buf, NULL, NULL);
> > --
> > 2.24.0.rc0.303.g954a862665-goog
> >
> >
^ permalink raw reply [flat|nested] 68+ messages in thread* Re: [PATCH v3 7/9] bugreport: add packed object summary
2019-12-11 0:29 ` Emily Shaffer
@ 2019-12-11 13:37 ` Johannes Schindelin
2019-12-11 20:52 ` Emily Shaffer
0 siblings, 1 reply; 68+ messages in thread
From: Johannes Schindelin @ 2019-12-11 13:37 UTC (permalink / raw)
To: Emily Shaffer; +Cc: git
Hi Emily,
On Tue, 10 Dec 2019, Emily Shaffer wrote:
> On Mon, Oct 28, 2019 at 04:43:50PM +0100, Johannes Schindelin wrote:
> > Hi Emily,
> >
> > On Thu, 24 Oct 2019, Emily Shaffer wrote:
> >
> > > Alongside the list of loose objects, it's useful to see the list of
> > > object packs as well. It can help us to examine what Git did and did not
> > > pack.
> >
> > Yes, I was write! Packs are next ;-)
> >
> > >
> > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > > ---
> > > bugreport.c | 21 +++++++++++++++++++++
> > > bugreport.h | 6 ++++++
> > > builtin/bugreport.c | 4 ++++
> > > 3 files changed, 31 insertions(+)
> > >
> > > diff --git a/bugreport.c b/bugreport.c
> > > index 54e1d47103..79ddb8baaa 100644
> > > --- a/bugreport.c
> > > +++ b/bugreport.c
> > > @@ -219,3 +219,24 @@ void get_loose_object_summary(struct strbuf *obj_info)
> > > objects.nr);
> > > }
> > > }
> > > +
> > > +void get_packed_object_summary(struct strbuf *obj_info)
> > > +{
> > > + struct strbuf dirpath = STRBUF_INIT;
> > > + struct string_list contents = STRING_LIST_INIT_DUP;
> > > + struct string_list_item *entry;
> > > +
> > > + strbuf_reset(obj_info);
> > > +
> > > + strbuf_addstr(&dirpath, get_object_directory());
> > > + strbuf_complete(&dirpath, '/');
> > > + strbuf_addstr(&dirpath, "pack/");
> > > + list_contents_of_dir(&contents, &dirpath, 0, 0);
> > > +
> > > + // list full contents of $GIT_OBJECT_DIRECTORY/pack/
> > > + for_each_string_list_item(entry, &contents) {
> > > + strbuf_addbuf(obj_info, &dirpath);
> > > + strbuf_addstr(obj_info, entry->string);
> > > + strbuf_complete_line(obj_info);
> > > + }
> > > +}
> >
> > Okay, but I think that you will want to discern between regular `.pack`
> > files, `.idx` files and `tmp_*` files.
>
> Discern in what way? How would you like to see them treated separately?
> They're all being listed here, not just counted, so it seems to me like
> I can read the generated bugreport and see which files are index, pack,
> or temporary here.
I take your word for it (sorry, it's been half an eternity since I wrapped
my head around the diff, I forgotten pretty much all about it ;-))
Ciao,
Dscho
>
> >
> > > diff --git a/bugreport.h b/bugreport.h
> > > index 09ad0c2599..11ff7df41b 100644
> > > --- a/bugreport.h
> > > +++ b/bugreport.h
> > > @@ -24,3 +24,9 @@ void get_populated_hooks(struct strbuf *hook_info);
> > > * will be discarded.
> > > */
> > > void get_loose_object_summary(struct strbuf *obj_info);
> > > +
> > > +/**
> > > + * Adds a list of the contents of '.git/objects/pack'. The previous contents of
> > > + * hook_info will be discarded.
> > > + */
> > > +void get_packed_object_summary(struct strbuf *obj_info);
> > > diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> > > index b2ab194207..da91a3944e 100644
> > > --- a/builtin/bugreport.c
> > > +++ b/builtin/bugreport.c
> > > @@ -68,6 +68,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> > > get_loose_object_summary(&buffer);
> > > strbuf_write(&buffer, report);
> > >
> > > + add_header(report, "Packed Object Summary");
> > > + get_packed_object_summary(&buffer);
> > > + strbuf_write(&buffer, report);
> > > +
> >
> > Hmm. At this point, I am unclear whether you want to write into an
> > `strbuf`, or directly into a `FILE *`? I would rather have only one, not
> > a mix.
> >
> > Ciao,
> > Dscho
> >
> > > fclose(report);
> > >
> > > launch_editor(report_path.buf, NULL, NULL);
> > > --
> > > 2.24.0.rc0.303.g954a862665-goog
> > >
> > >
>
^ permalink raw reply [flat|nested] 68+ messages in thread* Re: [PATCH v3 7/9] bugreport: add packed object summary
2019-12-11 13:37 ` Johannes Schindelin
@ 2019-12-11 20:52 ` Emily Shaffer
0 siblings, 0 replies; 68+ messages in thread
From: Emily Shaffer @ 2019-12-11 20:52 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
On Wed, Dec 11, 2019 at 02:37:10PM +0100, Johannes Schindelin wrote:
> Hi Emily,
>
> On Tue, 10 Dec 2019, Emily Shaffer wrote:
>
> > On Mon, Oct 28, 2019 at 04:43:50PM +0100, Johannes Schindelin wrote:
> > > Hi Emily,
> > >
> > > On Thu, 24 Oct 2019, Emily Shaffer wrote:
> > >
> > > > Alongside the list of loose objects, it's useful to see the list of
> > > > object packs as well. It can help us to examine what Git did and did not
> > > > pack.
> > >
> > > Yes, I was write! Packs are next ;-)
> > >
> > > >
> > > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > > > ---
> > > > bugreport.c | 21 +++++++++++++++++++++
> > > > bugreport.h | 6 ++++++
> > > > builtin/bugreport.c | 4 ++++
> > > > 3 files changed, 31 insertions(+)
> > > >
> > > > diff --git a/bugreport.c b/bugreport.c
> > > > index 54e1d47103..79ddb8baaa 100644
> > > > --- a/bugreport.c
> > > > +++ b/bugreport.c
> > > > @@ -219,3 +219,24 @@ void get_loose_object_summary(struct strbuf *obj_info)
> > > > objects.nr);
> > > > }
> > > > }
> > > > +
> > > > +void get_packed_object_summary(struct strbuf *obj_info)
> > > > +{
> > > > + struct strbuf dirpath = STRBUF_INIT;
> > > > + struct string_list contents = STRING_LIST_INIT_DUP;
> > > > + struct string_list_item *entry;
> > > > +
> > > > + strbuf_reset(obj_info);
> > > > +
> > > > + strbuf_addstr(&dirpath, get_object_directory());
> > > > + strbuf_complete(&dirpath, '/');
> > > > + strbuf_addstr(&dirpath, "pack/");
> > > > + list_contents_of_dir(&contents, &dirpath, 0, 0);
> > > > +
> > > > + // list full contents of $GIT_OBJECT_DIRECTORY/pack/
> > > > + for_each_string_list_item(entry, &contents) {
> > > > + strbuf_addbuf(obj_info, &dirpath);
> > > > + strbuf_addstr(obj_info, entry->string);
> > > > + strbuf_complete_line(obj_info);
> > > > + }
> > > > +}
> > >
> > > Okay, but I think that you will want to discern between regular `.pack`
> > > files, `.idx` files and `tmp_*` files.
> >
> > Discern in what way? How would you like to see them treated separately?
> > They're all being listed here, not just counted, so it seems to me like
> > I can read the generated bugreport and see which files are index, pack,
> > or temporary here.
>
> I take your word for it (sorry, it's been half an eternity since I wrapped
> my head around the diff, I forgotten pretty much all about it ;-))
That makes two of us. :)
>
> Ciao,
> Dscho
>
> >
> > >
> > > > diff --git a/bugreport.h b/bugreport.h
> > > > index 09ad0c2599..11ff7df41b 100644
> > > > --- a/bugreport.h
> > > > +++ b/bugreport.h
> > > > @@ -24,3 +24,9 @@ void get_populated_hooks(struct strbuf *hook_info);
> > > > * will be discarded.
> > > > */
> > > > void get_loose_object_summary(struct strbuf *obj_info);
> > > > +
> > > > +/**
> > > > + * Adds a list of the contents of '.git/objects/pack'. The previous contents of
> > > > + * hook_info will be discarded.
> > > > + */
> > > > +void get_packed_object_summary(struct strbuf *obj_info);
> > > > diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> > > > index b2ab194207..da91a3944e 100644
> > > > --- a/builtin/bugreport.c
> > > > +++ b/builtin/bugreport.c
> > > > @@ -68,6 +68,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> > > > get_loose_object_summary(&buffer);
> > > > strbuf_write(&buffer, report);
> > > >
> > > > + add_header(report, "Packed Object Summary");
> > > > + get_packed_object_summary(&buffer);
> > > > + strbuf_write(&buffer, report);
> > > > +
> > >
> > > Hmm. At this point, I am unclear whether you want to write into an
> > > `strbuf`, or directly into a `FILE *`? I would rather have only one, not
> > > a mix.
> > >
> > > Ciao,
> > > Dscho
> > >
> > > > fclose(report);
> > > >
> > > > launch_editor(report_path.buf, NULL, NULL);
> > > > --
> > > > 2.24.0.rc0.303.g954a862665-goog
> > > >
> > > >
> >
^ permalink raw reply [flat|nested] 68+ messages in thread
* [PATCH v3 8/9] bugreport: list contents of $OBJDIR/info
2019-10-25 2:51 ` [PATCH v3 0/9] add git-bugreport tool Emily Shaffer
` (6 preceding siblings ...)
2019-10-25 2:51 ` [PATCH v3 7/9] bugreport: add packed object summary Emily Shaffer
@ 2019-10-25 2:51 ` Emily Shaffer
2019-10-28 15:51 ` Johannes Schindelin
2019-10-25 2:51 ` [PATCH v3 9/9] bugreport: print contents of alternates file Emily Shaffer
2019-10-29 1:54 ` [PATCH v3 0/9] add git-bugreport tool Junio C Hamano
9 siblings, 1 reply; 68+ messages in thread
From: Emily Shaffer @ 2019-10-25 2:51 UTC (permalink / raw)
To: git; +Cc: Emily Shaffer
Miscellaneous information used about the object store can end up in
.git/objects/info; this can help us understand what may be going on with
the object store when the user is reporting a bug. Otherwise, it could
be difficult to track down what is going wrong with an object which
isn't kept locally to .git/objects/ or .git/objects/pack. Having some
understanding of where the user's objects may be kept can save us some
hops during the bug reporting process.
Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
bugreport.c | 58 +++++++++++++++++++++++++++++++++++++++++++++
bugreport.h | 6 +++++
builtin/bugreport.c | 7 ++++++
3 files changed, 71 insertions(+)
diff --git a/bugreport.c b/bugreport.c
index 79ddb8baaa..ce15904fec 100644
--- a/bugreport.c
+++ b/bugreport.c
@@ -6,6 +6,7 @@
#include "help.h"
#include "run-command.h"
#include "strbuf.h"
+#include "string-list.h"
#include "version.h"
#include "dirent.h"
@@ -171,6 +172,41 @@ void list_contents_of_dir(struct string_list *contents, struct strbuf *dirpath,
}
}
+/**
+ * Fills 'contents' with a list of all directories within the provided
+ * directory, recursing into each directory.
+ */
+void list_contents_of_dir_recursively(struct string_list *contents,
+ struct strbuf *dirpath)
+{
+ struct string_list current_contents = STRING_LIST_INIT_DUP;
+ struct string_list current_subdirs = STRING_LIST_INIT_DUP;
+ struct string_list_item *it;
+ struct strbuf buf = STRBUF_INIT;
+
+ list_contents_of_dir(¤t_contents, dirpath, 0, 0);
+ for_each_string_list_item(it, ¤t_contents) {
+ strbuf_reset(&buf);
+ strbuf_addbuf(&buf, dirpath);
+ strbuf_complete(&buf, '/');
+ strbuf_addstr(&buf, it->string);
+
+ string_list_append(contents, buf.buf);
+ }
+
+ list_contents_of_dir(¤t_subdirs, dirpath, 1, DT_DIR);
+ for_each_string_list_item(it, ¤t_subdirs) {
+ if (strcmp(it->string, ".") == 0
+ || strcmp(it->string, "..") == 0)
+ continue;
+ strbuf_reset(&buf);
+ strbuf_addbuf(&buf, dirpath);
+ strbuf_complete(&buf, '/');
+ strbuf_addstr(&buf, it->string);
+
+ list_contents_of_dir_recursively(contents, &buf);
+ }
+}
void get_object_counts(struct strbuf *obj_info)
{
@@ -240,3 +276,25 @@ void get_packed_object_summary(struct strbuf *obj_info)
strbuf_complete_line(obj_info);
}
}
+
+void get_object_info_summary(struct strbuf *obj_info)
+{
+ // strbuf += GITDIR/info/:
+ // recursively list contents of $GIT_OBJECT_DIRECTORY/info
+ struct strbuf dirpath = STRBUF_INIT;
+ struct string_list contents = STRING_LIST_INIT_DUP;
+ struct string_list_item *entry;
+
+ strbuf_reset(obj_info);
+
+ strbuf_addstr(&dirpath, get_object_directory());
+ strbuf_complete(&dirpath, '/');
+ strbuf_addstr(&dirpath, "info/");
+
+ list_contents_of_dir_recursively(&contents, &dirpath);
+
+ for_each_string_list_item(entry, &contents) {
+ strbuf_addstr(obj_info, entry->string);
+ strbuf_complete_line(obj_info);
+ }
+}
diff --git a/bugreport.h b/bugreport.h
index 11ff7df41b..4f5e2d1b9a 100644
--- a/bugreport.h
+++ b/bugreport.h
@@ -30,3 +30,9 @@ void get_loose_object_summary(struct strbuf *obj_info);
* hook_info will be discarded.
*/
void get_packed_object_summary(struct strbuf *obj_info);
+
+/**
+ * Adds a list of all contents (recursively) of '.git/objects/info'. The
+ * previous contents of hook_info will be discarded.
+ */
+void get_object_info_summary(struct strbuf *obj_info);
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index da91a3944e..8aad33a9b0 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -72,6 +72,13 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
get_packed_object_summary(&buffer);
strbuf_write(&buffer, report);
+ add_header(report, "Object Info Data");
+ get_object_info_summary(&buffer);
+ strbuf_write(&buffer, report);
+
+ // Close file
+ // open file in editor
+ launch_editor(report_path, NULL, NULL);
fclose(report);
launch_editor(report_path.buf, NULL, NULL);
--
2.24.0.rc0.303.g954a862665-goog
^ permalink raw reply related [flat|nested] 68+ messages in thread* Re: [PATCH v3 8/9] bugreport: list contents of $OBJDIR/info
2019-10-25 2:51 ` [PATCH v3 8/9] bugreport: list contents of $OBJDIR/info Emily Shaffer
@ 2019-10-28 15:51 ` Johannes Schindelin
0 siblings, 0 replies; 68+ messages in thread
From: Johannes Schindelin @ 2019-10-28 15:51 UTC (permalink / raw)
To: Emily Shaffer; +Cc: git
Hi Emily,
On Thu, 24 Oct 2019, Emily Shaffer wrote:
> Miscellaneous information used about the object store can end up in
> .git/objects/info; this can help us understand what may be going on with
> the object store when the user is reporting a bug. Otherwise, it could
> be difficult to track down what is going wrong with an object which
> isn't kept locally to .git/objects/ or .git/objects/pack. Having some
> understanding of where the user's objects may be kept can save us some
> hops during the bug reporting process.
Makes sense.
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
> bugreport.c | 58 +++++++++++++++++++++++++++++++++++++++++++++
> bugreport.h | 6 +++++
> builtin/bugreport.c | 7 ++++++
> 3 files changed, 71 insertions(+)
>
> diff --git a/bugreport.c b/bugreport.c
> index 79ddb8baaa..ce15904fec 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -6,6 +6,7 @@
> #include "help.h"
> #include "run-command.h"
> #include "strbuf.h"
> +#include "string-list.h"
> #include "version.h"
>
> #include "dirent.h"
> @@ -171,6 +172,41 @@ void list_contents_of_dir(struct string_list *contents, struct strbuf *dirpath,
> }
> }
>
> +/**
> + * Fills 'contents' with a list of all directories within the provided
> + * directory, recursing into each directory.
> + */
> +void list_contents_of_dir_recursively(struct string_list *contents,
> + struct strbuf *dirpath)
> +{
> + struct string_list current_contents = STRING_LIST_INIT_DUP;
> + struct string_list current_subdirs = STRING_LIST_INIT_DUP;
Rather than populating those string lists, why not write into the
`contents` directly, _while_ traversing the directories?
But if you really, really want to use string lists (hey, I'm a fan, I
introduced them to Git's source code, after all), then please make sure
to release them in the end, too.
> + struct string_list_item *it;
> + struct strbuf buf = STRBUF_INIT;
> +
> + list_contents_of_dir(¤t_contents, dirpath, 0, 0);
> + for_each_string_list_item(it, ¤t_contents) {
> + strbuf_reset(&buf);
> + strbuf_addbuf(&buf, dirpath);
> + strbuf_complete(&buf, '/');
> + strbuf_addstr(&buf, it->string);
You don't need a separate `strbuf` for that: just complete `dirpath`
with a trailing slash, remember the length, then in each iteration reset
the length and append the current subdirectory name.
> +
> + string_list_append(contents, buf.buf);
> + }
> +
> + list_contents_of_dir(¤t_subdirs, dirpath, 1, DT_DIR);
> + for_each_string_list_item(it, ¤t_subdirs) {
> + if (strcmp(it->string, ".") == 0
> + || strcmp(it->string, "..") == 0)
> + continue;
> + strbuf_reset(&buf);
> + strbuf_addbuf(&buf, dirpath);
> + strbuf_complete(&buf, '/');
> + strbuf_addstr(&buf, it->string);
> +
> + list_contents_of_dir_recursively(contents, &buf);
> + }
> +}
>
> void get_object_counts(struct strbuf *obj_info)
> {
> @@ -240,3 +276,25 @@ void get_packed_object_summary(struct strbuf *obj_info)
> strbuf_complete_line(obj_info);
> }
> }
> +
> +void get_object_info_summary(struct strbuf *obj_info)
> +{
> + // strbuf += GITDIR/info/:
> + // recursively list contents of $GIT_OBJECT_DIRECTORY/info
> + struct strbuf dirpath = STRBUF_INIT;
> + struct string_list contents = STRING_LIST_INIT_DUP;
> + struct string_list_item *entry;
> +
> + strbuf_reset(obj_info);
> +
> + strbuf_addstr(&dirpath, get_object_directory());
> + strbuf_complete(&dirpath, '/');
> + strbuf_addstr(&dirpath, "info/");
> +
> + list_contents_of_dir_recursively(&contents, &dirpath);
> +
> + for_each_string_list_item(entry, &contents) {
> + strbuf_addstr(obj_info, entry->string);
> + strbuf_complete_line(obj_info);
> + }
As mentioned earlier, I would rather write things into `obj_info`
directly, without detouring to a string list.
Or, if you decide that you want to write to a `FILE *` anyway, I'd
rather avoid the detour to the `strbuf` altogether.
Ciao,
Dscho
> +}
> diff --git a/bugreport.h b/bugreport.h
> index 11ff7df41b..4f5e2d1b9a 100644
> --- a/bugreport.h
> +++ b/bugreport.h
> @@ -30,3 +30,9 @@ void get_loose_object_summary(struct strbuf *obj_info);
> * hook_info will be discarded.
> */
> void get_packed_object_summary(struct strbuf *obj_info);
> +
> +/**
> + * Adds a list of all contents (recursively) of '.git/objects/info'. The
> + * previous contents of hook_info will be discarded.
> + */
> +void get_object_info_summary(struct strbuf *obj_info);
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index da91a3944e..8aad33a9b0 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -72,6 +72,13 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> get_packed_object_summary(&buffer);
> strbuf_write(&buffer, report);
>
> + add_header(report, "Object Info Data");
> + get_object_info_summary(&buffer);
> + strbuf_write(&buffer, report);
> +
> + // Close file
> + // open file in editor
> + launch_editor(report_path, NULL, NULL);
> fclose(report);
>
> launch_editor(report_path.buf, NULL, NULL);
> --
> 2.24.0.rc0.303.g954a862665-goog
>
>
^ permalink raw reply [flat|nested] 68+ messages in thread
* [PATCH v3 9/9] bugreport: print contents of alternates file
2019-10-25 2:51 ` [PATCH v3 0/9] add git-bugreport tool Emily Shaffer
` (7 preceding siblings ...)
2019-10-25 2:51 ` [PATCH v3 8/9] bugreport: list contents of $OBJDIR/info Emily Shaffer
@ 2019-10-25 2:51 ` Emily Shaffer
2019-10-28 15:57 ` Johannes Schindelin
2019-10-29 1:54 ` [PATCH v3 0/9] add git-bugreport tool Junio C Hamano
9 siblings, 1 reply; 68+ messages in thread
From: Emily Shaffer @ 2019-10-25 2:51 UTC (permalink / raw)
To: git; +Cc: Emily Shaffer
In some cases, it could be that the user is having a problem with an
object which isn't present in their normal object directory. We can get
a hint that that might be the case by examining the list of alternates
where their object may be stored instead.
Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
bugreport.c | 14 ++++++++++++++
bugreport.h | 6 ++++++
builtin/bugreport.c | 4 ++++
3 files changed, 24 insertions(+)
diff --git a/bugreport.c b/bugreport.c
index ce15904fec..a7bdc72b7f 100644
--- a/bugreport.c
+++ b/bugreport.c
@@ -298,3 +298,17 @@ void get_object_info_summary(struct strbuf *obj_info)
strbuf_complete_line(obj_info);
}
}
+
+void get_alternates_file(struct strbuf *alternates_info)
+{
+ struct strbuf alternates_path = STRBUF_INIT;
+
+ strbuf_addstr(&alternates_path, get_object_directory());
+ strbuf_complete(&alternates_path, '/');
+ strbuf_addstr(&alternates_path, "info/alternates");
+
+ strbuf_reset(alternates_info);
+ strbuf_addbuf(alternates_info, &alternates_path);
+ strbuf_complete_line(alternates_info);
+ strbuf_read_file(alternates_info, alternates_path.buf, 0);
+}
diff --git a/bugreport.h b/bugreport.h
index 4f5e2d1b9a..74d1f79960 100644
--- a/bugreport.h
+++ b/bugreport.h
@@ -36,3 +36,9 @@ void get_packed_object_summary(struct strbuf *obj_info);
* previous contents of hook_info will be discarded.
*/
void get_object_info_summary(struct strbuf *obj_info);
+
+/**
+ * Adds the contents of '.git/info/alternates'. The previous contents of
+ * alternates_info will be discarded.
+ */
+void get_alternates_file(struct strbuf *alt_info);
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 8aad33a9b0..0784bdc42a 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -76,6 +76,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
get_object_info_summary(&buffer);
strbuf_write(&buffer, report);
+ add_header(report, "Alternates File");
+ get_alternates_file(&buffer);
+ strbuf_write(&buffer, report);
+
// Close file
// open file in editor
launch_editor(report_path, NULL, NULL);
--
2.24.0.rc0.303.g954a862665-goog
^ permalink raw reply related [flat|nested] 68+ messages in thread* Re: [PATCH v3 9/9] bugreport: print contents of alternates file
2019-10-25 2:51 ` [PATCH v3 9/9] bugreport: print contents of alternates file Emily Shaffer
@ 2019-10-28 15:57 ` Johannes Schindelin
2019-11-19 20:40 ` Emily Shaffer
0 siblings, 1 reply; 68+ messages in thread
From: Johannes Schindelin @ 2019-10-28 15:57 UTC (permalink / raw)
To: Emily Shaffer; +Cc: git
Hi Emily,
On Thu, 24 Oct 2019, Emily Shaffer wrote:
> In some cases, it could be that the user is having a problem with an
> object which isn't present in their normal object directory. We can get
> a hint that that might be the case by examining the list of alternates
> where their object may be stored instead.
Doesn't this open the possibility of leaking project's (possibly NDA'ed) names?
I could imagine that we might rather want to count the alternates, and
maybe separate into those alternates that actually exist and alternates
that do not exist (which would produce a warning that the user might
have trained themselves to ignore).
Ciao,
Dscho
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
> bugreport.c | 14 ++++++++++++++
> bugreport.h | 6 ++++++
> builtin/bugreport.c | 4 ++++
> 3 files changed, 24 insertions(+)
>
> diff --git a/bugreport.c b/bugreport.c
> index ce15904fec..a7bdc72b7f 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -298,3 +298,17 @@ void get_object_info_summary(struct strbuf *obj_info)
> strbuf_complete_line(obj_info);
> }
> }
> +
> +void get_alternates_file(struct strbuf *alternates_info)
> +{
> + struct strbuf alternates_path = STRBUF_INIT;
> +
> + strbuf_addstr(&alternates_path, get_object_directory());
> + strbuf_complete(&alternates_path, '/');
> + strbuf_addstr(&alternates_path, "info/alternates");
> +
> + strbuf_reset(alternates_info);
> + strbuf_addbuf(alternates_info, &alternates_path);
> + strbuf_complete_line(alternates_info);
> + strbuf_read_file(alternates_info, alternates_path.buf, 0);
> +}
> diff --git a/bugreport.h b/bugreport.h
> index 4f5e2d1b9a..74d1f79960 100644
> --- a/bugreport.h
> +++ b/bugreport.h
> @@ -36,3 +36,9 @@ void get_packed_object_summary(struct strbuf *obj_info);
> * previous contents of hook_info will be discarded.
> */
> void get_object_info_summary(struct strbuf *obj_info);
> +
> +/**
> + * Adds the contents of '.git/info/alternates'. The previous contents of
> + * alternates_info will be discarded.
> + */
> +void get_alternates_file(struct strbuf *alt_info);
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index 8aad33a9b0..0784bdc42a 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -76,6 +76,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> get_object_info_summary(&buffer);
> strbuf_write(&buffer, report);
>
> + add_header(report, "Alternates File");
> + get_alternates_file(&buffer);
> + strbuf_write(&buffer, report);
> +
> // Close file
> // open file in editor
> launch_editor(report_path, NULL, NULL);
> --
> 2.24.0.rc0.303.g954a862665-goog
>
>
^ permalink raw reply [flat|nested] 68+ messages in thread* Re: [PATCH v3 9/9] bugreport: print contents of alternates file
2019-10-28 15:57 ` Johannes Schindelin
@ 2019-11-19 20:40 ` Emily Shaffer
0 siblings, 0 replies; 68+ messages in thread
From: Emily Shaffer @ 2019-11-19 20:40 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
On Mon, Oct 28, 2019 at 04:57:01PM +0100, Johannes Schindelin wrote:
> Hi Emily,
>
> On Thu, 24 Oct 2019, Emily Shaffer wrote:
>
> > In some cases, it could be that the user is having a problem with an
> > object which isn't present in their normal object directory. We can get
> > a hint that that might be the case by examining the list of alternates
> > where their object may be stored instead.
>
> Doesn't this open the possibility of leaking project's (possibly NDA'ed) names?
>
> I could imagine that we might rather want to count the alternates, and
> maybe separate into those alternates that actually exist and alternates
> that do not exist (which would produce a warning that the user might
> have trained themselves to ignore).
Sounds reasonable. Will do.
- Emily
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v3 0/9] add git-bugreport tool
2019-10-25 2:51 ` [PATCH v3 0/9] add git-bugreport tool Emily Shaffer
` (8 preceding siblings ...)
2019-10-25 2:51 ` [PATCH v3 9/9] bugreport: print contents of alternates file Emily Shaffer
@ 2019-10-29 1:54 ` Junio C Hamano
2019-10-29 11:13 ` Johannes Schindelin
9 siblings, 1 reply; 68+ messages in thread
From: Junio C Hamano @ 2019-10-29 1:54 UTC (permalink / raw)
To: Emily Shaffer; +Cc: git, Derrick Stolee, Johannes Schindelin
Emily Shaffer <emilyshaffer@google.com> writes:
> Thanks for the patience with the long wait, all. Here's an attempt at
> the rewrite in C; I think it does verbatim what the sh version did
> except that this doesn't print the reflog - Jonathan Nieder was good
> enough to point out to me that folks probably don't want to share their
> commit subjects all willy-nilly if they're working on something
> secretive.
Is the goal to give a tool the end users can type "git
bugreport<RET>" and automatically send the result to this mailing
list (or some bug tracker)? Or is this only about producing a
pre-filled bug report template to be slurped into their MUA and then
further manually edited before sending it out?
It probably is controversial if we exposed contents of hooks scripts
(I can imagine some people may trigger external process by pinging a
URL that includes credential material out of laziness), so the
presence test you have is probably a good stopping point. I do not
know how much it helps to know which hooks exist in order to
diagnose an anomaly without knowing what their contents are, but it
is a start.
By the way, I doubt that it is the best use of developer time to
write these in C---taking various pieces of information from
different places to prepare a text file sounds more suited to
scripting languages, especially while we are still learning what
kind of information we want to collect and how we want to present
the final result (iow, for the first handful of years after
deploying this command).
Thanks.
^ permalink raw reply [flat|nested] 68+ messages in thread* Re: [PATCH v3 0/9] add git-bugreport tool
2019-10-29 1:54 ` [PATCH v3 0/9] add git-bugreport tool Junio C Hamano
@ 2019-10-29 11:13 ` Johannes Schindelin
0 siblings, 0 replies; 68+ messages in thread
From: Johannes Schindelin @ 2019-10-29 11:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Emily Shaffer, git, Derrick Stolee
Hi Junio,
On Tue, 29 Oct 2019, Junio C Hamano wrote:
> Emily Shaffer <emilyshaffer@google.com> writes:
>
> > Thanks for the patience with the long wait, all. Here's an attempt at
> > the rewrite in C; I think it does verbatim what the sh version did
> > except that this doesn't print the reflog - Jonathan Nieder was good
> > enough to point out to me that folks probably don't want to share their
> > commit subjects all willy-nilly if they're working on something
> > secretive.
>
> Is the goal to give a tool the end users can type "git
> bugreport<RET>" and automatically send the result to this mailing
> list (or some bug tracker)? Or is this only about producing a
> pre-filled bug report template to be slurped into their MUA and then
> further manually edited before sending it out?
>
> It probably is controversial if we exposed contents of hooks scripts
> (I can imagine some people may trigger external process by pinging a
> URL that includes credential material out of laziness), so the
> presence test you have is probably a good stopping point. I do not
> know how much it helps to know which hooks exist in order to
> diagnose an anomaly without knowing what their contents are, but it
> is a start.
>
> By the way, I doubt that it is the best use of developer time to
> write these in C---taking various pieces of information from
> different places to prepare a text file sounds more suited to
> scripting languages, especially while we are still learning what
> kind of information we want to collect and how we want to present
> the final result (iow, for the first handful of years after
> deploying this command).
If you want to limit Git to Linux, then relying on a specific scripting
language like Unix shell scripting sounds okay.
If you target more platforms, then a range of scripting languages sounds
a lot more sensible, my top choice would be node.js.
I understand that that is not an option here, and there is a strong
preference for Unix shell scripting in Git. This is a bit unfortunate:
A good chunk of the bug reports I see in Git for Windows are _about_
problems with Unix shell scripting. There are fork problems, problems
where `add_item` failed (which is one of the least helpful error
messages I saw in my entire life), there are problems with anti-malware
causing segmentation faults in the Unix shell script interpreter (the
MSYS2 Bash), and there are even (believe it or not) problems with
_display drivers_. Yes, that is right, display drivers can interfere
with Unix shell script execution on Windows. (This is really rare, but
hey, bug reports are rare in Git for Windows to begin with...)
All of these problems _prevent_ Unix shell scripts from being
interpreted as expected on Windows. Which would ridicule the idea of
reporting any of these bugs using a tool that is implemented in Unix
shell.
And what is worse than a bug in an application? A bug in the
application's very own bug reporting tool.
I pointed this out to Emily, and that is the reason why this amount of
work was put in to _improve_ the tool, precisely by _not_ implementing
it as a Unix shell script, but instead in plain C (which is a _lot_ more
robust).
Ciao,
Dscho
^ permalink raw reply [flat|nested] 68+ messages in thread