Re: [PATCH v5 1/4] git: make super-prefix option

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 28, 2016, 22:19 UTC
Message-ID: <xmqqzimrhdlh.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1475099443-145608-2-git-send-email-bmwill@google.com>

Brandon Williams <bmwill@google.com> writes:

Show 6 quoted lines

> Add a super-prefix environment variable 'GIT_INTERNAL_SUPER_PREFIX'
> which can be used to specify a path from above a repository down to its
> root.  The immediate use of this option is by commands which have a
> --recurse-submodule option in order to give context to submodules about
> how they were invoked.  This option is currently only allowed for
> builtins which support a super-prefix.

Yes, it can be used to specify that, but it is unclear (1) how such a path thusly specified is used, (2) how it affects the outcome of the operations, and (3) what it means (the same applies to the documentation update).

It would help future readers of "git log", "tig blame", etc. to have a few sentences after the first sentence above. Perhaps

    ... specify a path from above ... down to its root.  When such a
    super-prefix is specified, the paths reported by Git are
    prefixed with it to make them relative to that directory
    "above".  The paths given by the users on the command line
    (e.g. "git subcmd --output-file=path/to/a/file" and pathspecs)
    are taken relative to that directory "above" to match.

or something like that?

Show 5 quoted lines

> @@ -468,6 +469,7 @@ extern int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);
>  extern int get_common_dir(struct strbuf *sb, const char *gitdir);
>  extern const char *get_git_namespace(void);
>  extern const char *strip_namespace(const char *namespaced_ref);
> +extern const char *get_super_prefix();

Nice, but

    extern const char *get_super_prefix(void);

> +const char *get_super_prefix()

Likewise.

Show 5 quoted lines

> +{
> +	if (!super_prefix)
> +		super_prefix = getenv(GIT_SUPER_PREFIX_ENVIRONMENT);
> +	return super_prefix;
> +}

Good.

Show 8 quoted lines

>  	commit_pager_choice();
>  
> +	if (!help && get_super_prefix()) {
> +		if (!(p->option & SUPPORT_SUPER_PREFIX))
> +			die("%s doesn't support --super-prefix", p->cmd);
> +		if (prefix)
> +			die("can't have both a prefix and super-prefix");
> +	}

Nice. I'd phrase the latter as "can't use--super-prefix from a subdirectory", though.