| Message ID | 20191025025129.250049-2-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add git-bugreport tool | expand |
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?
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.
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
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
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
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.
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.
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.
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
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
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 },
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