Web lists-archives.com

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

s/to to/to/;

> /mnt/stuff/repo.
>
> This worked in the initial version of this facility[1], but regressed
> later in the series while solving a related bug[2].
>
> 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
paragraph" trick?

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

Thanks.