| Message ID | 20200206004108.261317-11-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add git-bugreport tool | expand |
On Wed, Feb 05, 2020 at 04:41:03PM -0800, Emily Shaffer wrote: > +static void get_safelisted_config(struct strbuf *config_info) > +{ > + size_t idx; > + struct string_list_item *it = NULL; > + struct key_value_info *kv_info = NULL; > + > + for (idx = 0; idx < ARRAY_SIZE(bugreport_config_safelist); idx++) { GCC 9 complains about this loop condition: CC bugreport.o bugreport.c: In function 'get_safelisted_config': bugreport.c:66:20: error: comparison of unsigned expression < 0 is always false [-Werror=type-limits] 66 | for (idx = 0; idx < ARRAY_SIZE(bugreport_config_safelist); idx++) { | ^ I don't understand this error, that autogenerated 'bugreport_config_safelist' array clearly has a a non-zero size. What am I missing? > + const struct string_list *list = > + git_config_get_value_multi(bugreport_config_safelist[idx]); > + > + if (!list) > + continue; > + > + strbuf_addf(config_info, "%s:\n", bugreport_config_safelist[idx]); > + for_each_string_list_item(it, list) { > + kv_info = it->util; > + strbuf_addf(config_info, " %s (%s)\n", it->string, > + kv_info ? config_scope_name(kv_info->scope) > + : "source unknown"); > + } > + } > +} > +
On Fri, Feb 07, 2020 at 03:47:25PM +0100, SZEDER Gábor wrote: > On Wed, Feb 05, 2020 at 04:41:03PM -0800, Emily Shaffer wrote: > > +static void get_safelisted_config(struct strbuf *config_info) > > +{ > > + size_t idx; > > + struct string_list_item *it = NULL; > > + struct key_value_info *kv_info = NULL; > > + > > + for (idx = 0; idx < ARRAY_SIZE(bugreport_config_safelist); idx++) { > > GCC 9 complains about this loop condition: > > CC bugreport.o > > bugreport.c: In function 'get_safelisted_config': > > bugreport.c:66:20: error: comparison of unsigned expression < 0 is always false [-Werror=type-limits] > > 66 | for (idx = 0; idx < ARRAY_SIZE(bugreport_config_safelist); idx++) { > > | ^ > > I don't understand this error, that autogenerated > 'bugreport_config_safelist' array clearly has a a non-zero size. > What am I missing? macOS 'sed', that's what I was missing :) So that array is in fact empty on macOS, because the entries of that array are generated with: # cat all regular files in Documentation/config find Documentation/config -type f -exec cat {} \; | # print the command name which matches the annotate-bugreport macro sed -n 's/^\(.*\) \+annotate:bugreport\[include\].* ::$/ "\1",/p' | sort and the 'sed' included in macOS apparently interprets that '\+' differently than GNU 'sed', and as a result won't match anything. FWIW, that '\+' doesn't seem to be necessary, though, and after removing it the resulting generated array looked good to me (and to the compiler) both on Linux and macOS.
On Fri, Feb 7, 2020 at 10:09 AM SZEDER Gábor <szeder.dev@gmail.com> wrote: > macOS 'sed', that's what I was missing :) > > sed -n 's/^\(.*\) \+annotate:bugreport\[include\].* ::$/ "\1",/p' | sort > > and the 'sed' included in macOS apparently interprets that '\+' > differently than GNU 'sed', and as a result won't match anything. More generally, this would be a problem with any 'sed' of BSD lineage. > FWIW, that '\+' doesn't seem to be necessary, though, and after > removing it the resulting generated array looked good to me [...] A reasonable replacement for "<SP>\+" would be "<SP><SP>*" (where <SP> represents 'space').
On Feb 07 2020, Eric Sunshine wrote: > On Fri, Feb 7, 2020 at 10:09 AM SZEDER Gábor <szeder.dev@gmail.com> wrote: >> macOS 'sed', that's what I was missing :) >> >> sed -n 's/^\(.*\) \+annotate:bugreport\[include\].* ::$/ "\1",/p' | sort >> >> and the 'sed' included in macOS apparently interprets that '\+' >> differently than GNU 'sed', and as a result won't match anything. > > More generally, this would be a problem with any 'sed' of BSD lineage. > >> FWIW, that '\+' doesn't seem to be necessary, though, and after >> removing it the resulting generated array looked good to me [...] > > A reasonable replacement for "<SP>\+" would be "<SP><SP>*" (where <SP> > represents 'space'). Another problem with that regexp is that it contains two adjacent repetitions matching the same character. When there are two or more spaces before "annotate:" all but the last of them can be matched by either '\(.*\)' or ' \+'. To fix that '\(.*\)' needs to be modified to not match a trailing space. Andreas.
On Fri, Feb 07, 2020 at 05:51:05PM +0100, Andreas Schwab wrote: > On Feb 07 2020, Eric Sunshine wrote: > > > On Fri, Feb 7, 2020 at 10:09 AM SZEDER Gábor <szeder.dev@gmail.com> wrote: > >> macOS 'sed', that's what I was missing :) > >> > >> sed -n 's/^\(.*\) \+annotate:bugreport\[include\].* ::$/ "\1",/p' | sort > >> > >> and the 'sed' included in macOS apparently interprets that '\+' > >> differently than GNU 'sed', and as a result won't match anything. > > > > More generally, this would be a problem with any 'sed' of BSD lineage. > > > >> FWIW, that '\+' doesn't seem to be necessary, though, and after > >> removing it the resulting generated array looked good to me [...] > > > > A reasonable replacement for "<SP>\+" would be "<SP><SP>*" (where <SP> > > represents 'space'). > > Another problem with that regexp is that it contains two adjacent > repetitions matching the same character. When there are two or more > spaces before "annotate:" all but the last of them can be matched by > either '\(.*\)' or ' \+'. To fix that '\(.*\)' needs to be modified to > not match a trailing space. Hum. I had assumed since the capture group was not greedy, it would not capture any trailing spaces that could be captured by ' \+'. I don't see a problem in making it non-explicit, though. I'll hack on this some more. I do find myself pretty annoyed at the matcher difference between 'sed' and 'also sed', though, and don't see in the manpage a way to guarantee which matcher 'sed' should use (a la 'grep -[EFGP]'). :) - Emily
diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt index 23265b0d74..4e9171d1bd 100644 --- a/Documentation/git-bugreport.txt +++ b/Documentation/git-bugreport.txt @@ -30,6 +30,7 @@ The following information is captured automatically: - Compiler-specific info string - 'git remote-https --build-info' - $SHELL + - Selected config values OPTIONS ------- diff --git a/Makefile b/Makefile index 2bc9f112ea..7b139230ae 100644 --- a/Makefile +++ b/Makefile @@ -2131,6 +2131,8 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS) help.sp help.s help.o: command-list.h +bugreport.sp bugreport.s bugreport.o: bugreport-config-safelist.h + builtin/help.sp builtin/help.s builtin/help.o: config-list.h GIT-PREFIX builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \ '-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \ diff --git a/bugreport.c b/bugreport.c index 9b51250155..16216bff8e 100644 --- a/bugreport.c +++ b/bugreport.c @@ -6,6 +6,9 @@ #include "help.h" #include "compat/compiler.h" #include "run-command.h" +#include "config.h" +#include "bugreport-config-safelist.h" +#include "khash.h" static void get_git_remote_https_version_info(struct strbuf *version_info) { @@ -52,6 +55,29 @@ static void get_system_info(struct strbuf *sys_info) strbuf_complete_line(sys_info); } +static void get_safelisted_config(struct strbuf *config_info) +{ + size_t idx; + struct string_list_item *it = NULL; + struct key_value_info *kv_info = NULL; + + for (idx = 0; idx < ARRAY_SIZE(bugreport_config_safelist); idx++) { + const struct string_list *list = + git_config_get_value_multi(bugreport_config_safelist[idx]); + + if (!list) + continue; + + strbuf_addf(config_info, "%s:\n", bugreport_config_safelist[idx]); + for_each_string_list_item(it, list) { + kv_info = it->util; + strbuf_addf(config_info, " %s (%s)\n", it->string, + kv_info ? config_scope_name(kv_info->scope) + : "source unknown"); + } + } +} + static const char * const bugreport_usage[] = { N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"), NULL @@ -94,6 +120,7 @@ int cmd_main(int argc, const char **argv) char *option_output = NULL; char *option_suffix = "%F-%H%M"; struct stat statbuf; + int nongit_ok = 0; const struct option bugreport_options[] = { OPT_STRING('o', "output-directory", &option_output, N_("path"), @@ -102,6 +129,10 @@ int cmd_main(int argc, const char **argv) N_("specify a strftime format suffix for the filename")), OPT_END() }; + + /* Prerequisite for hooks and config checks */ + setup_git_directory_gently(&nongit_ok); + argc = parse_options(argc, argv, "", bugreport_options, bugreport_usage, 0); @@ -123,6 +154,9 @@ int cmd_main(int argc, const char **argv) get_header(&buffer, "System Info"); get_system_info(&buffer); + get_header(&buffer, "Safelisted Config Info"); + get_safelisted_config(&buffer); + report = fopen_for_writing(report_path.buf); if (report == NULL) {
Teach bugreport to gather the values of config options which are present in 'bugreport-config-safelist.h', and show their origin scope. 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. Rather than including the path to someone's config, which can reveal filesystem layout and project names, just name the scope (e.g. system, global, local) of the config source. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- Documentation/git-bugreport.txt | 1 + Makefile | 2 ++ bugreport.c | 34 +++++++++++++++++++++++++++++++++ 3 files changed, 37 insertions(+)