Message ID | 14b0f278196ab9ab130402c2ef79adb0543655ef.1581294660.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | config: allow user to know scope of config options | expand |
"Matthew Rogers via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Matthew Rogers <mattr94@gmail.com> > > To prepare for the upcoming --show-scope option, we require the ability > to convert a config_scope enum to a string. As this was originally > implemented as a static function 'scope_name()' in > t/helper/test-config.c, we expose it via config.h and give it a less > ambiguous name 'config_scope_name()' > Signed-off-by: Matthew Rogers <mattr94@gmail.com> > --- > config.c | 16 ++++++++++++++++ > config.h | 2 ++ > t/helper/test-config.c | 17 +---------------- > t/t1308-config-set.sh | 2 +- > 4 files changed, 20 insertions(+), 17 deletions(-) > > diff --git a/config.c b/config.c > index d75f88ca0c..83bb98d65e 100644 > --- a/config.c > +++ b/config.c > @@ -3297,6 +3297,22 @@ const char *current_config_origin_type(void) > } > } > > +const char *config_scope_name(enum config_scope scope) > +{ > + switch (scope) { > + case CONFIG_SCOPE_SYSTEM: > + return "system"; > + case CONFIG_SCOPE_GLOBAL: > + return "global"; > + case CONFIG_SCOPE_REPO: > + return "repo"; > + case CONFIG_SCOPE_CMDLINE: > + return "command line"; The change from "cmdline" to "command line" does need to happen before the end of the series, but I do not think it should happen here, especialy given that the proposed log message explains that this step is to expose scope_name() under a better name (which is a very good split point). How are you reviewing the patches in your own series before sending them out? This round is better than the previous rounds where we didn't have a matching change to the tests so "make test" may not have passed in the middle of the series, though... > + default: > + return "unknown"; > + } > +} > + > const char *current_config_name(void) > { > const char *name; > diff --git a/config.h b/config.h > index 91fd4c5e96..dcb8c274d4 100644 > --- a/config.h > +++ b/config.h > @@ -35,6 +35,7 @@ struct object_id; > > #define CONFIG_REGEX_NONE ((void *)1) > > + > struct git_config_source { > unsigned int use_stdin:1; > const char *file; > @@ -301,6 +302,7 @@ enum config_scope { > CONFIG_SCOPE_REPO, > CONFIG_SCOPE_CMDLINE, > }; > +const char *config_scope_name(enum config_scope scope); > > enum config_scope current_config_scope(void); > const char *current_config_origin_type(void); > diff --git a/t/helper/test-config.c b/t/helper/test-config.c > index 214003d5b2..1e3bc7c8f4 100644 > --- a/t/helper/test-config.c > +++ b/t/helper/test-config.c > @@ -37,21 +37,6 @@ > * > */ > > -static const char *scope_name(enum config_scope scope) > -{ > - switch (scope) { > - case CONFIG_SCOPE_SYSTEM: > - return "system"; > - case CONFIG_SCOPE_GLOBAL: > - return "global"; > - case CONFIG_SCOPE_REPO: > - return "repo"; > - case CONFIG_SCOPE_CMDLINE: > - return "cmdline"; > - default: > - return "unknown"; > - } > -} > static int iterate_cb(const char *var, const char *value, void *data) > { > static int nr; > @@ -63,7 +48,7 @@ static int iterate_cb(const char *var, const char *value, void *data) > printf("value=%s\n", value ? value : "(null)"); > printf("origin=%s\n", current_config_origin_type()); > printf("name=%s\n", current_config_name()); > - printf("scope=%s\n", scope_name(current_config_scope())); > + printf("scope=%s\n", config_scope_name(current_config_scope())); > > return 0; > } > diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh > index 7b4e1a63eb..5f3e71a160 100755 > --- a/t/t1308-config-set.sh > +++ b/t/t1308-config-set.sh > @@ -265,7 +265,7 @@ test_expect_success 'iteration shows correct origins' ' > value=from-cmdline > origin=command line > name= > - scope=cmdline > + scope=command line > EOF > GIT_CONFIG_PARAMETERS=$cmdline_config test-tool config iterate >actual && > test_cmp expect actual
Junio C Hamano <gitster@pobox.com> writes: > The change from "cmdline" to "command line" does need to happen > before the end of the series, but I do not think it should happen > here, especialy given that the proposed log message explains that > this step is to expose scope_name() under a better name (which is a > very good split point). I'll tweak this step with the attached patch, and then adjust 06/10 as needed, while queuing. config.c | 2 +- config.h | 1 - t/t1308-config-set.sh | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/config.c b/config.c index 83bb98d65e..a922b136e5 100644 --- a/config.c +++ b/config.c @@ -3307,7 +3307,7 @@ const char *config_scope_name(enum config_scope scope) case CONFIG_SCOPE_REPO: return "repo"; case CONFIG_SCOPE_CMDLINE: - return "command line"; + return "cmdline"; default: return "unknown"; } diff --git a/config.h b/config.h index dcb8c274d4..c063f33ff6 100644 --- a/config.h +++ b/config.h @@ -35,7 +35,6 @@ struct object_id; #define CONFIG_REGEX_NONE ((void *)1) - struct git_config_source { unsigned int use_stdin:1; const char *file; diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh index 5f3e71a160..7b4e1a63eb 100755 --- a/t/t1308-config-set.sh +++ b/t/t1308-config-set.sh @@ -265,7 +265,7 @@ test_expect_success 'iteration shows correct origins' ' value=from-cmdline origin=command line name= - scope=command line + scope=cmdline EOF GIT_CONFIG_PARAMETERS=$cmdline_config test-tool config iterate >actual && test_cmp expect actual
> > How are you reviewing the patches in your own series before sending > them out? This round is better than the previous rounds where we > didn't have a matching change to the tests so "make test" may not > have passed in the middle of the series, though... > I went through each patch individually using rebase -i and built/tested it. Although just to save time I only did t1300 and t1308 since I believe those were the only ones that should be affected. I can write a script that would run the whole test suite overnight for me and make sure the series shakes out okay, if you'd like.
On Mon, Feb 10, 2020 at 07:30:22PM -0500, Matt Rogers wrote: > > > > How are you reviewing the patches in your own series before sending > > them out? This round is better than the previous rounds where we > > didn't have a matching change to the tests so "make test" may not > > have passed in the middle of the series, though... > > > > I went through each patch individually using rebase -i and built/tested it. > Although just to save time I only did t1300 and t1308 since I believe those were > the only ones that should be affected. I can write a script that > would run the whole > test suite overnight for me and make sure the series shakes out okay, > if you'd like. Not sure whether you intend to do this or not, but to maybe save you some scripting, I do this like so: git rebase -x \ "make -j16 && (cd t && prove -j16 -v --shuffle t[0-9]*.sh)" master If the tests fail, the rebase is paused, which can be understandably disappointing if you walk away from it overnight only to have it fail after 10 minutes, though. :) - Emily
Matt Rogers <mattr94@gmail.com> writes: >> How are you reviewing the patches in your own series before sending >> them out? This round is better than the previous rounds where we >> didn't have a matching change to the tests so "make test" may not >> have passed in the middle of the series, though... >> > > I went through each patch individually using rebase -i and built/tested it. > Although just to save time I only did t1300 and t1308 since I believe those were > the only ones that should be affected. I can write a script that > would run the whole > test suite overnight for me and make sure the series shakes out okay, > if you'd like. What I like does not matter. What I pointed out for 04/10 wouldn't have been caught by your testing anyway, as both the code and the test had matching unnecessry changes. I was wondering if you are relying too heavily on just tests and without actually proofreading the changes to see if they still make sense in the context of the updated series, and if my suspicion was correct, if there are something reviewers can do to help the authors.
On Tue, Feb 11, 2020 at 1:10 AM Junio C Hamano <gitster@pobox.com> wrote: > > Matt Rogers <mattr94@gmail.com> writes: > > >> How are you reviewing the patches in your own series before sending > >> them out? This round is better than the previous rounds where we > >> didn't have a matching change to the tests so "make test" may not > >> have passed in the middle of the series, though... > >> > > > > I went through each patch individually using rebase -i and built/tested it. > > Although just to save time I only did t1300 and t1308 since I believe those were > > the only ones that should be affected. I can write a script that > > would run the whole > > test suite overnight for me and make sure the series shakes out okay, > > if you'd like. > > What I like does not matter. > > What I pointed out for 04/10 wouldn't have been caught by your > testing anyway, as both the code and the test had matching > unnecessry changes. I was wondering if you are relying too heavily > on just tests and without actually proofreading the changes to see > if they still make sense in the context of the updated series, and > if my suspicion was correct, if there are something reviewers can do > to help the authors. > > I do try to proofread patches, I'm just not the most careful of reviewers at times, partially as a personal problem and partially as this is a new workflow for me. As for the particular issue, I just thought it was a good idea at the time and I didn't think it all the way through
diff --git a/config.c b/config.c index d75f88ca0c..83bb98d65e 100644 --- a/config.c +++ b/config.c @@ -3297,6 +3297,22 @@ const char *current_config_origin_type(void) } } +const char *config_scope_name(enum config_scope scope) +{ + switch (scope) { + case CONFIG_SCOPE_SYSTEM: + return "system"; + case CONFIG_SCOPE_GLOBAL: + return "global"; + case CONFIG_SCOPE_REPO: + return "repo"; + case CONFIG_SCOPE_CMDLINE: + return "command line"; + default: + return "unknown"; + } +} + const char *current_config_name(void) { const char *name; diff --git a/config.h b/config.h index 91fd4c5e96..dcb8c274d4 100644 --- a/config.h +++ b/config.h @@ -35,6 +35,7 @@ struct object_id; #define CONFIG_REGEX_NONE ((void *)1) + struct git_config_source { unsigned int use_stdin:1; const char *file; @@ -301,6 +302,7 @@ enum config_scope { CONFIG_SCOPE_REPO, CONFIG_SCOPE_CMDLINE, }; +const char *config_scope_name(enum config_scope scope); enum config_scope current_config_scope(void); const char *current_config_origin_type(void); diff --git a/t/helper/test-config.c b/t/helper/test-config.c index 214003d5b2..1e3bc7c8f4 100644 --- a/t/helper/test-config.c +++ b/t/helper/test-config.c @@ -37,21 +37,6 @@ * */ -static const char *scope_name(enum config_scope scope) -{ - switch (scope) { - case CONFIG_SCOPE_SYSTEM: - return "system"; - case CONFIG_SCOPE_GLOBAL: - return "global"; - case CONFIG_SCOPE_REPO: - return "repo"; - case CONFIG_SCOPE_CMDLINE: - return "cmdline"; - default: - return "unknown"; - } -} static int iterate_cb(const char *var, const char *value, void *data) { static int nr; @@ -63,7 +48,7 @@ static int iterate_cb(const char *var, const char *value, void *data) printf("value=%s\n", value ? value : "(null)"); printf("origin=%s\n", current_config_origin_type()); printf("name=%s\n", current_config_name()); - printf("scope=%s\n", scope_name(current_config_scope())); + printf("scope=%s\n", config_scope_name(current_config_scope())); return 0; } diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh index 7b4e1a63eb..5f3e71a160 100755 --- a/t/t1308-config-set.sh +++ b/t/t1308-config-set.sh @@ -265,7 +265,7 @@ test_expect_success 'iteration shows correct origins' ' value=from-cmdline origin=command line name= - scope=cmdline + scope=command line EOF GIT_CONFIG_PARAMETERS=$cmdline_config test-tool config iterate >actual && test_cmp expect actual