Re: [PATCH] config: match both symlink & realpath versions in IncludeIf.gitdir:*
- Date: Tue, 16 May 2017 10:15:40 +0900
- From: Junio C Hamano <gitster@xxxxxxxxx>
- Subject: Re: [PATCH] config: match both symlink & realpath versions in IncludeIf.gitdir:*
Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> Change the conditional inclusion mechanism to support
> e.g. gitdir:~/git_tree/repo where ~/git_tree is a symlink to to
> This worked in the initial version of this facility, but regressed
> later in the series while solving a related bug.
> Now gitdir: will match against the symlinked
> path (e.g. gitdir:~/git_tree/repo) in addition to the current
> /mnt/stuff/repo path.
> Since this is already in a release version note in the documentation
> that this behavior changed, so users who expect their configuration to
> work on both v2.13.0 and some future version of git with this fix
> aren't utterly confused.
> 1. commit 3efd0bedc6 ("config: add conditional include", 2017-03-01)
> 2. commit 86f9515708 ("config: resolve symlinks in conditional
> include's patterns", 2017-04-05)
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> Here's a non-RFC patch to fix this bug.
> Documentation/config.txt | 11 +++++++++++
> config.c | 16 ++++++++++++++++
> t/t1305-config-include.sh | 23 +++++++++++++++++++++++
> 3 files changed, 50 insertions(+)
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 475e874d51..137502a289 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -140,6 +140,17 @@ A few more notes on matching via `gitdir` and `gitdir/i`:
> * Symlinks in `$GIT_DIR` are not resolved before matching.
> + * Both the symlink & realpath versions of paths will be matched
> + outside of `$GIT_DIR`. E.g. if ~/git is a symlink to
> + /mnt/storage/git, both `gitdir:~/git` and `gitdir:/mnt/storage/git`
> + will match.
> + This was not the case in the initial release of this feature in
> + v2.13.0, which only matched the realpath version. Configuration
> + that wants to be compatible with the initial release of this
> + feature needs to either specify only the realpath version, or both
> + versions.
Does the second paragraph format correctly, or do we need the usual
(and ugly) "+ by itself on a line and then dedent the next
> diff --git a/config.c b/config.c
> index b4a3205da3..0498746112 100644
> --- a/config.c
> +++ b/config.c
> @@ -214,6 +214,7 @@ static int include_by_gitdir(const struct config_options *opts,
> struct strbuf pattern = STRBUF_INIT;
> int ret = 0, prefix;
> const char *git_dir;
> + int retry = 1;
I think your earlier RFC used a variable with a better name than
this; it is not like we are preparing ourselves for adding a
mechanism here to retry for many random reasons.
The logic would be clearer With an "already_tried_absolute" variable
that is initially 0, retrying unless it is already set, and set it
when we retry, like you did in your RFC.