Re: [PATCH] introduce git root

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> But the one place I do not agree is:
>
>> I think "sequence.editor" and "core.editor" are better because:
>> 
>> - they use the same syntax as the config variables, so they are easier
>> to remember and to discover, and
>
> I really don't like using "core.editor" here, because it has the same
> name as a config variable, but it is _not_ the config variable. It
> happens to use the config variable as one of the inputs to its
> computation, but in many cases:
>
>   git config core.editor
>
> and
>
>   git var core.editor
>
> will produce different values. They are entirely different namespaces.
> Using the same syntax and name seems unnecessarily confusing to me.

I think this is a valid concern.

It is a tangent, the current definition of "git_editor" helper reads
like this:

        git_editor() {
                if test -z "${GIT_EDITOR:+set}"
                then
                        GIT_EDITOR="$(git var GIT_EDITOR)" || return $?
                fi

                eval "$GIT_EDITOR" '"$@"'
        }

If "git var editor" were to compute a reasonable value from the
various user settings, and because GIT_EDITOR is among the sources
of user settings, I wonder if the surrounding "if test -z then...fi"
should be there.

The pager side seems to do (halfway) "the right thing":

        git_pager() {
                if test -t 1
                then
                        GIT_PAGER=$(git var GIT_PAGER)
                else
                        GIT_PAGER=cat
                fi
                : ${LESS=-FRX}
                : ${LV=-c}
                export LESS LV

                eval "$GIT_PAGER" '"$@"'
        }

The initial "test -t 1" is "we do not page to non-terminal", but we
ask "git var" to take care of PAGER/GIT_PAGER fallback/precedence.

It is tempting to argue that "we do not page to non-terminal" should
also be part of "various user settings" for "git var" to take into
account and fold this "if test -t 1...then...else...fi" also into
"git var", but because a typical command line "git var" will be used
on would look like this:

	GIT_PAGER=$(git var pager)
        eval "$GIT_PAGER" ...

with the standard output stream of "git var" not connected to
terminal, that would not fly very well, and I am not sure what
should be done about it.

This is another tangent that comes back to the original "how to name
them?" question, but I wonder if a convention like this would work.

 * When asking for a feature (e.g. "what editor to use"), if there
   is a git-specific environment variable that can be set to
   override all other settings (e.g. "$GIT_EDITOR" trumps "$EDITOR"
   environment or "core.editor" configuration), "git var" can be
   queried with the name of that "strong" environment variable.

 * All other features without such a strong environment variables
   should not be spelled as if there is such an environment variable
   the user can set in order to avoid confusion.

Here are some consequences that come to my mind, if we adopt such an
approach:

 * We would keep GIT_EDITOR and GIT_PAGER (e.g. "git var
   GIT_EDITOR");

 * GIT_SEQUENCE_EDITOR is an example of an overriding environment
   variable, and as such, "what editor to use when munging the
   rebase insn sheet?", would be "git var GIT_SEQUENCE_EDITOR".

 * We would deprecate GIT_AUTHOR_IDENT and GIT_COMMITTER_IDENT
   because setting such an environment variable would not do
   anything.  Add "git var author-ident" that is clear that it does
   not name an environment variable as its replacement.

 * It is OK to add support for new queries, e.g. GIT_AUTHOR_DATE,
   for existing environment variables that we already take into
   account but that the current "git var" does not support.

Arguably, we could go the other way around.  Instead of adding "git
var author-ident", we actually allow GIT_AUTHOR_IDENT environment
variable to be set and used instead of GIT_AUTHOR_{NAME,EMAIL,DATE}.
If that approach is practical, then we could stick to the syntax
that looks like an environment variable name.  A script could then
instead of doing:

	GIT_AUTHOR_IDENT=$(git var GIT_AUTHOR_IDENT)
        ... parse it into NAME,EMAIL,DATE and ...
        export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE

        ... complicated set of operations to prepare ...

        git commit-tree ...

do this instead:

	GIT_AUTHOR_IDENT=$(git var GIT_AUTHOR_IDENT)
        export GIT_AUTHOR_IDENT

        ... complicated set of operations to prepare ...

        git commit-tree ...