[v7,04/10] config: make scope_name non-static and rename it

Commit Message

Johannes Schindelin via GitGitGadget Feb. 10, 2020, 12:30 a.m. UTC

From: Matthew Rogers <[e-mail]>

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 <[e-mail]>
---
 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(-)

Comments

Junio C Hamano Feb. 10, 2020, 6:02 p.m. UTC | #1

"Matthew Rogers via GitGitGadget" <[e-mail]> writes:

> From: Matthew Rogers <[e-mail]>
>
> 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 <[e-mail]>
> ---
>  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 Feb. 10, 2020, 9:25 p.m. UTC | #2

Junio C Hamano <[e-mail]> 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

Matt Rogers Feb. 11, 2020, 12:30 a.m. UTC | #3

>
> 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.

Emily Shaffer Feb. 11, 2020, 1:58 a.m. UTC | #4

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

Junio C Hamano Feb. 11, 2020, 6:10 a.m. UTC | #5

Matt Rogers <[e-mail]> 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.

Matt Rogers Feb. 11, 2020, 12:37 p.m. UTC | #6

On Tue, Feb 11, 2020 at 1:10 AM Junio C Hamano <[e-mail]> wrote:
>
> Matt Rogers <[e-mail]> 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

Patch

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