| Message ID | 179df6d6adfe460de7413e1fb1dff6bce185ae0e.1634077795.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Conditional config includes based on remote URL | expand |
On Tue, Oct 12, 2021 at 03:57:22PM -0700, Jonathan Tan wrote: > It is not used from outside the file in which it is declared. Makes sense. We used to use it from builtin/config.c, but that went away in e895589883 (git-config: use git_config_with_options, 2012-10-23). > diff --git a/config.h b/config.h > index 147f5e0490..b11b0be09a 100644 > --- a/config.h > +++ b/config.h > @@ -126,6 +126,8 @@ int git_default_config(const char *, const char *, void *); > /** > * Read a specific file in git-config format. > * This function takes the same callback and data parameters as `git_config`. > + * > + * Unlike git_config(), this function does not respect includes. > */ Breaking out the relevant caller-facing parts of the documentation like this is a nice touch. And the rest of the patch looks good to me. -Peff
Jonathan Tan <jonathantanmy@google.com> writes: > It is not used from outside the file in which it is declared. > > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > config.c | 12 +++++++++++- > config.h | 37 ++++--------------------------------- > 2 files changed, 15 insertions(+), 34 deletions(-) Nice.
On Tue, Oct 12 2021, Jonathan Tan wrote: > It is not used from outside the file in which it is declared. > > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > config.c | 12 +++++++++++- > config.h | 37 ++++--------------------------------- > 2 files changed, 15 insertions(+), 34 deletions(-) > > diff --git a/config.c b/config.c > index 2edf835262..365d57833b 100644 > --- a/config.c > +++ b/config.c > @@ -120,6 +120,16 @@ static long config_buf_ftell(struct config_source *conf) > return conf->u.buf.pos; > } > > +struct config_include_data { > + int depth; > + config_fn_t fn; > + void *data; > + const struct config_options *opts; > +}; > +#define CONFIG_INCLUDE_INIT { 0 } > + > +static int git_config_include(const char *var, const char *value, void *data); Can't we just move the function here? > #define MAX_INCLUDE_DEPTH 10 > static const char include_depth_advice[] = N_( > "exceeded maximum include depth (%d) while including\n" > @@ -306,7 +316,7 @@ static int include_condition_is_true(const struct config_options *opts, > return 0; > } > > -int git_config_include(const char *var, const char *value, void *data) > +static int git_config_include(const char *var, const char *value, void *data) ...and avoid the forward declaration? I've seen that in a few places, making the diff smaller here doesn't seem worth it v.s. maintaining the definition in two places.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Tue, Oct 12 2021, Jonathan Tan wrote: > >> It is not used from outside the file in which it is declared. >> >> Signed-off-by: Jonathan Tan <jonathantanmy@google.com> >> --- >> config.c | 12 +++++++++++- >> config.h | 37 ++++--------------------------------- >> 2 files changed, 15 insertions(+), 34 deletions(-) >> >> diff --git a/config.c b/config.c >> index 2edf835262..365d57833b 100644 >> --- a/config.c >> +++ b/config.c >> @@ -120,6 +120,16 @@ static long config_buf_ftell(struct config_source *conf) >> return conf->u.buf.pos; >> } >> >> +struct config_include_data { >> + int depth; >> + config_fn_t fn; >> + void *data; >> + const struct config_options *opts; >> +}; >> +#define CONFIG_INCLUDE_INIT { 0 } >> + >> +static int git_config_include(const char *var, const char *value, void *data); > > Can't we just move the function here? > >> #define MAX_INCLUDE_DEPTH 10 >> static const char include_depth_advice[] = N_( >> "exceeded maximum include depth (%d) while including\n" >> @@ -306,7 +316,7 @@ static int include_condition_is_true(const struct config_options *opts, >> return 0; >> } >> >> -int git_config_include(const char *var, const char *value, void *data) >> +static int git_config_include(const char *var, const char *value, void *data) > > ...and avoid the forward declaration? > > I've seen that in a few places, making the diff smaller here doesn't > seem worth it v.s. maintaining the definition in two places. Sounds good. If we are moving things around anyway, it is probably a good time to do that, too ;-)
> > ...and avoid the forward declaration? > > > > I've seen that in a few places, making the diff smaller here doesn't > > seem worth it v.s. maintaining the definition in two places. > > Sounds good. If we are moving things around anyway, it is probably > a good time to do that, too ;-) OK, I'll do this.
diff --git a/config.c b/config.c index 2edf835262..365d57833b 100644 --- a/config.c +++ b/config.c @@ -120,6 +120,16 @@ static long config_buf_ftell(struct config_source *conf) return conf->u.buf.pos; } +struct config_include_data { + int depth; + config_fn_t fn; + void *data; + const struct config_options *opts; +}; +#define CONFIG_INCLUDE_INIT { 0 } + +static int git_config_include(const char *var, const char *value, void *data); + #define MAX_INCLUDE_DEPTH 10 static const char include_depth_advice[] = N_( "exceeded maximum include depth (%d) while including\n" @@ -306,7 +316,7 @@ static int include_condition_is_true(const struct config_options *opts, return 0; } -int git_config_include(const char *var, const char *value, void *data) +static int git_config_include(const char *var, const char *value, void *data) { struct config_include_data *inc = data; const char *cond, *key; diff --git a/config.h b/config.h index 147f5e0490..b11b0be09a 100644 --- a/config.h +++ b/config.h @@ -126,6 +126,8 @@ int git_default_config(const char *, const char *, void *); /** * Read a specific file in git-config format. * This function takes the same callback and data parameters as `git_config`. + * + * Unlike git_config(), this function does not respect includes. */ int git_config_from_file(config_fn_t fn, const char *, void *); @@ -158,6 +160,8 @@ void read_very_early_config(config_fn_t cb, void *data); * will first feed the user-wide one to the callback, and then the * repo-specific one; by overwriting, the higher-priority repo-specific * value is left at the end). + * + * Unlike git_config_from_file(), this function respects includes. */ void git_config(config_fn_t fn, void *); @@ -339,39 +343,6 @@ const char *current_config_origin_type(void); const char *current_config_name(void); int current_config_line(void); -/** - * Include Directives - * ------------------ - * - * By default, the config parser does not respect include directives. - * However, a caller can use the special `git_config_include` wrapper - * callback to support them. To do so, you simply wrap your "real" callback - * function and data pointer in a `struct config_include_data`, and pass - * the wrapper to the regular config-reading functions. For example: - * - * ------------------------------------------- - * int read_file_with_include(const char *file, config_fn_t fn, void *data) - * { - * struct config_include_data inc = CONFIG_INCLUDE_INIT; - * inc.fn = fn; - * inc.data = data; - * return git_config_from_file(git_config_include, file, &inc); - * } - * ------------------------------------------- - * - * `git_config` respects includes automatically. The lower-level - * `git_config_from_file` does not. - * - */ -struct config_include_data { - int depth; - config_fn_t fn; - void *data; - const struct config_options *opts; -}; -#define CONFIG_INCLUDE_INIT { 0 } -int git_config_include(const char *name, const char *value, void *data); - /* * Match and parse a config key of the form: *
It is not used from outside the file in which it is declared. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- config.c | 12 +++++++++++- config.h | 37 ++++--------------------------------- 2 files changed, 15 insertions(+), 34 deletions(-)