Re: [PATCH] Documentation: replace 'vi' for 'editor' to reflect build-time option

Re: [PATCH] Documentation: replace 'vi' for 'editor' to reflect build-time option

[Jonathan Nieder]

Hi Rodrigo,

Rodrigo Silva (MestreLion) wrote:

> git-var(1) did not say that hardcoded fallback 'vi' may have been changed
> at build time. A user could be puzzled if 'nano' pops up even when none of the
> mentioned environment vars or config.editor are set.
>
> Ideally, the build system should be changed to reflect the chosen fallback
> editor when creating the man pages. Not sure if that is even possible though.

Good catch.

Filling in the blank at compile time would be possible, but I'm not
convinced it's a good idea.  Wouldn't the same user be just as puzzled
when

  http://git.kernel.org/?p=git/git-htmldocs.git;a=blob_plain;f=git-var.html

(or whatever page with a nicer URL arises to replace the old
www.kernel.org/...  pages) says the fallback is 'vi' and git behaves
differently?

How about something like this?

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Context: http://bugs.debian.org/666250

 Documentation/git-var.txt |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git i/Documentation/git-var.txt w/Documentation/git-var.txt
index 5317cc24..beef9e28 100644
--- i/Documentation/git-var.txt
+++ w/Documentation/git-var.txt
@@ -43,13 +43,15 @@ GIT_EDITOR::
     `$SOME_ENVIRONMENT_VARIABLE`, `"C:\Program Files\Vim\gvim.exe"
     --nofork`.  The order of preference is the `$GIT_EDITOR`
     environment variable, then `core.editor` configuration, then
-    `$VISUAL`, then `$EDITOR`, and then finally 'vi'.
+    `$VISUAL`, then `$EDITOR`, and then the default chosen at compile
+    time, which is usually 'vi'.
 
 GIT_PAGER::
     Text viewer for use by git commands (e.g., 'less').  The value
     is meant to be interpreted by the shell.  The order of preference
     is the `$GIT_PAGER` environment variable, then `core.pager`
-    configuration, then `$PAGER`, and then finally 'less'.
+    configuration, then `$PAGER`, and then the default chosen at
+    compile time (usually 'less').
 
 Diagnostics
 -----------
-- 

Re: [PATCH] Documentation: replace 'vi' for 'editor' to reflect build-time option

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Filling in the blank at compile time would be possible, but I'm not
> convinced it's a good idea.  Wouldn't the same user be just as puzzled
> when
>
>   http://git.kernel.org/?p=git/git-htmldocs.git;a=blob_plain;f=git-var.html
>
> (or whatever page with a nicer URL arises to replace the old
> www.kernel.org/...  pages) says the fallback is 'vi' and git behaves
> differently?

I've already rejected this patch once, but that was primarily because the
patch was not justified with the above "I read everybody else's git uses
'vi' on the Interweb, and even though my distro's manual page says it uses
'nano', I didn't bother to read it." scenario.

Bug#666250: [PATCH] Documentation: replace 'vi' for 'editor' to reflect build-time option

[Rodrigo Silva]

At 23:16 29/3/2012, Junio C Hamano wrote:
>Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Filling in the blank at compile time would be possible, but I'm not
>> convinced it's a good idea.  Wouldn't the same user be just as puzzled
>> when
>>
>>   http://git.kernel.org/?p=git/git-htmldocs.git;a=blob_plain;f=git-var.html
>>
>> (or whatever page with a nicer URL arises to replace the old
>> www.kernel.org/...  pages) says the fallback is 'vi' and git behaves
>> differently?
>
>I've already rejected this patch once, but that was primarily because the
>patch was not justified with the above "I read everybody else's git uses
>'vi' on the Interweb, and even though my distro's manual page says it uses
>'nano', I didn't bother to read it." scenario.

@Jonathan: I'm really sorry for this confusion... I forgot to mention this issue WAS taken to upstream already, and it was rejected because build-time options should be documented by whoever did the changes. So in this case, it's Debian's task to update their man pags to reflect their build-time changes. Junio suggested me to send this patch to Debian, that's why I filled the bug in Debian BTS. It was meant to be a Debian-only patch.

So upstream git man would still say "... and finally 'vi'", since this is upstream default, and Debian would read "... and finally 'editor'", since this is the default set by Debian at build-time. For reference, the original patch included this change, among others:

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 5317cc2..9c49163 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -43,7 +43,8 @@ GIT_EDITOR::
     `$SOME_ENVIRONMENT_VARIABLE`, `"C:\Program Files\Vim\gvim.exe"
     --nofork`.  The order of preference is the `$GIT_EDITOR`
     environment variable, then `core.editor` configuration, then
-    `$VISUAL`, then `$EDITOR`, and then finally 'vi'.
+    `$VISUAL`, then `$EDITOR`, and then finally a hardcoded fallback
+    editor set at build time, by default 'vi'.
 
 GIT_PAGER::
     Text viewer for use by git commands (e.g., 'less').  The value

The other changes were fully accepted upstream, this one was rejected. So I modified it to Debian's scenario and filled the bug in BTS.

Since this is meant for Debian, no need to be generic about the "fallback editor set at build time": Debian uses 'editor', so 'editor' should be explicitly mentioned to correctly reflect git's behavior on a Debian system.

@Junio, I'm really sorry for indirectly bringing this up twice to your attention. I've never expected Debian maitainers to be so resposinve, it was awesome fast by reading, analysing and forwarding upstream in a matter of hours. I'm impressed (and embarasssed by this confusion)

MestreLion

Re: [PATCH] Documentation: replace 'vi' for 'editor' to reflect build-time option

[Junio C Hamano]

Rodrigo Silva <linux@rodrigosilva.com> writes:

> At 23:16 29/3/2012, Junio C Hamano wrote:
> ...
>>I've already rejected this patch once, but that was primarily because the
>>patch was not justified with the above "I read everybody else's git uses
>>'vi' on the Interweb, and even though my distro's manual page says it uses
>>'nano', I didn't bother to read it." scenario.
>
> @Jonathan: I'm really sorry for this confusion...
> ... Junio suggested me to send this patch to
> Debian, that's why I filled the bug in Debian BTS. It was meant to be a
> Debian-only patch.

It is a Debian package bug that the git_*.diff patch they use to customize
the upstream source ends up making a resulting binary to use different
editor and pager but does so without editing Documentation/ files to match
the description to what their executable does.  The resulting package is
not self consistent.

But we aren't making it particularly easy for Debian to do so.

The debian/rules (the Makefile that invokes our Makefile) file in the
git_*.diff patch merely overrides DEFAULT_EDITOR and DEFAULT_PAGER,
instead of patching editor.c and pager.c to replace the fallback hardcoded
defaults "vi" and "less".  But these Makefile variables only affect that
goes in the executable, and they do not affect the documentation.

So in the shorter term, I think Jonathan's patch would be OK for me to
carry in my tree, but even in that case I think Debian still needs to
replace the "vi/less" in the documentation to tell what they chosen for
their users.

In the longer term, it may make more sense to let these Makefile variables
replace "vi" and "less" with whatever values set to them, while keeping
the "default chosen at the build time, usually" wording.

In any case, I'd appreciate it if either of you can turn Jonathan's "How
about something like this?" into the final version to be fed to "git am".

Thanks.

[PATCH 0/2] Re: Documentation: replace 'vi' for 'editor' to reflect build-time option

[Jonathan Nieder]

Junio C Hamano wrote:

> I've already rejected this patch once, but that was primarily because the
> patch was not justified with the above "I read everybody else's git uses
> 'vi' on the Interweb, and even though my distro's manual page says it uses
> 'nano', I didn't bother to read it." scenario.

Thanks.  Here's a pair of patches meant to combine the advantages of
both approaches. ;-)

Thoughts and improvements welcome, of course.

Sincerely,
Jonathan Nieder (2):
  var doc: default editor and pager are configurable at build time
  var doc: advertise current DEFAULT_PAGER and DEFAULT_EDITOR settings

 Documentation/Makefile    |   10 ++++++++++
 Documentation/git-var.txt |   16 ++++++++++++++--
 Makefile                  |    2 ++
 3 files changed, 26 insertions(+), 2 deletions(-)

[PATCH 1/2] var doc: default editor and pager are configurable at build time

[Jonathan Nieder]

Some distributors customize the fallback pager and editor used by git
commands when the user has not indicated a preference via the
core.editor/core.pager configuration or GIT_EDITOR, GIT_PAGER, VISUAL,
EDITOR, and PAGER environment variables, and git's build system
provides DEFAULT_PAGER and DEFAULT_EDITOR makefile settings to help
them with that (see v1.6.6-rc0~24, 2009-11-20).

Unfortunately those compile-time settings do not affect the
documentation, so the uninitiated user who tries to understand git by
reading the git-var(1) manpage can easily be confused when git falls
back to 'nano' and 'more' instead of 'vi' and 'less'.  Even if the
distributor patches the distributed docs to reflect the new default,
the user may read the official documentation from the git-htmldocs
repository online and be confused in the same way.

Add a few words stating that the defaults are customizable at
compile time to make the behavior crystal clear.

Reported-by: Rodrigo Silva (MestreLion) <linux@rodrigosilva.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/git-var.txt |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 5317cc24..beef9e28 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -43,13 +43,15 @@ GIT_EDITOR::
     `$SOME_ENVIRONMENT_VARIABLE`, `"C:\Program Files\Vim\gvim.exe"
     --nofork`.  The order of preference is the `$GIT_EDITOR`
     environment variable, then `core.editor` configuration, then
-    `$VISUAL`, then `$EDITOR`, and then finally 'vi'.
+    `$VISUAL`, then `$EDITOR`, and then the default chosen at compile
+    time, which is usually 'vi'.
 
 GIT_PAGER::
     Text viewer for use by git commands (e.g., 'less').  The value
     is meant to be interpreted by the shell.  The order of preference
     is the `$GIT_PAGER` environment variable, then `core.pager`
-    configuration, then `$PAGER`, and then finally 'less'.
+    configuration, then `$PAGER`, and then the default chosen at
+    compile time (usually 'less').
 
 Diagnostics
 -----------
-- 
1.7.10.rc3

Re: [PATCH 1/2] var doc: default editor and pager are configurable at build time

[Jonathan Nieder]

Hi Rodrigo,

Rodrigo Silva wrote:
 
>> Unfortunately those compile-time settings do not affect the
>> documentation, so the uninitiated user who tries to understand git by
>> reading the git-var(1) manpage can easily be confused when git falls
>> back to 'nano' and 'more' instead of 'vi' and 'less'.  Even if the
>> distributor patches the distributed docs to reflect the new default,
>> the user may read the official documentation from the git-htmldocs
>> repository online and be confused in the same way.
>
> I'm sorry, but I disagree with this approach. There is no "usually":
> upstream sources are *always* 'vi', and Debian packages are *always*
> 'editor'.

I disagree.  If I build the pristine source from Junio with
"./configure --with-shell=my-favorite-shell", I still have built the
pristine source from Junio.

[...]
> Upstream git /could/ say "... and finally, a hardcoded editor chosen
> at compile time, by default 'vi'"

It is not hardcoded.

[...]
> IMHO, only saying that "it can be customized", "it is usually 'vi'",
> without saying it /was/ customized and it /isn't/ vi, but editor, is
> not a good approach.

Ah, perhaps that is the source of misunderstanding.  This is a
two-patch series.  The two patches are meant to go together.  Does
that help?

Sorry for the lack of clarity,
Jonathan

[PATCH 2/2] var doc: advertise current DEFAULT_PAGER and DEFAULT_EDITOR settings

[Jonathan Nieder]

Document the default pager and editor chosen at compile time in the
git-var(1) manpage so users curious about what command _this_ copy of
git will fall back to when EDITOR, VISUAL, and PAGER are unset can
find the answer quickly.

In builds leaving those settings uncustomized, this patch makes the
manpage continue to say "usually vi" and "usually less" so the
formatted documentation is usable for a wide audience including users
of custom builds that change those settings.  If you would like your
copy of the docs to be less noncommittal, you will need to set
DEFAULT_PAGER=less and DEFAULT_EDITOR=vi explicitly.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Thanks for reading.

 Documentation/Makefile    |   10 ++++++++++
 Documentation/git-var.txt |   10 ++++++++++
 Makefile                  |    2 ++
 3 files changed, 22 insertions(+)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index d40e211f..9fee0b92 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -124,6 +124,16 @@ SHELL_PATH ?= $(SHELL)
 # Shell quote;
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
 
+ifdef DEFAULT_PAGER
+DEFAULT_PAGER_SQ = $(subst ','\'',$(DEFAULT_PAGER))
+ASCIIDOC_EXTRA += -a 'git-default-pager=$(DEFAULT_PAGER_SQ)'
+endif
+
+ifdef DEFAULT_EDITOR
+DEFAULT_EDITOR_SQ = $(subst ','\'',$(DEFAULT_EDITOR))
+ASCIIDOC_EXTRA += -a 'git-default-editor=$(DEFAULT_EDITOR_SQ)'
+endif
+
 #
 # Please note that there is a minor bug in asciidoc.
 # The version after 6.0.3 _will_ include the patch found here:
diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index beef9e28..44a5e356 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -44,14 +44,24 @@ GIT_EDITOR::
     --nofork`.  The order of preference is the `$GIT_EDITOR`
     environment variable, then `core.editor` configuration, then
     `$VISUAL`, then `$EDITOR`, and then the default chosen at compile
+ifndef::git-default-editor[]
     time, which is usually 'vi'.
+endif::git-default-editor[]
+ifdef::git-default-editor[]
+    time ('{git-default-editor}').
+endif::git-default-editor[]
 
 GIT_PAGER::
     Text viewer for use by git commands (e.g., 'less').  The value
     is meant to be interpreted by the shell.  The order of preference
     is the `$GIT_PAGER` environment variable, then `core.pager`
     configuration, then `$PAGER`, and then the default chosen at
+ifndef::git-default-pager[]
     compile time (usually 'less').
+endif::git-default-pager[]
+ifdef::git-default-pager[]
+    compile time ('{git-default-pager}').
+endif::git-default-pager[]
 
 Diagnostics
 -----------
diff --git a/Makefile b/Makefile
index be1957a5..adc48b0d 100644
--- a/Makefile
+++ b/Makefile
@@ -2258,6 +2258,8 @@ $(XDIFF_LIB): $(XDIFF_OBJS)
 $(VCSSVN_LIB): $(VCSSVN_OBJS)
 	$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $(VCSSVN_OBJS)
 
+export DEFAULT_EDITOR DEFAULT_PAGER
+
 doc:
 	$(MAKE) -C Documentation all
 
-- 
1.7.10.rc3

Re: [PATCH 2/2] var doc: advertise current DEFAULT_PAGER and DEFAULT_EDITOR settings

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Document the default pager and editor chosen at compile time in the
> git-var(1) manpage so users curious about what command _this_ copy of
> git will fall back to when EDITOR, VISUAL, and PAGER are unset can
> find the answer quickly.
>
> In builds leaving those settings uncustomized, this patch makes the
> manpage continue to say "usually vi" and "usually less" so the
> formatted documentation is usable for a wide audience including users
> of custom builds that change those settings.  If you would like your
> copy of the docs to be less noncommittal, you will need to set
> DEFAULT_PAGER=less and DEFAULT_EDITOR=vi explicitly.
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Thanks for reading.

Thanks for a patch.

I think the Makefile patch to use "-a" takes the right approach (assuming
that the name of the EDITOR/PAGER won't have funny character that would
trip up AsciiDoc, which I think is a reasonably assumption), but I think
even the customized package should say what it is "usually" set on other
people's systems.

In other words, instead of:

>      `$VISUAL`, then `$EDITOR`, and then the default chosen at compile
> +ifndef::git-default-editor[]
>      time, which is usually 'vi'.
> +endif::git-default-editor[]
> +ifdef::git-default-editor[]
> +    time ('{git-default-editor}').
> +endif::git-default-editor[]

something like:

	... then the default chosen at compile time, which is usually 'vi'.
ifdef::git-default-editor[]
	The build you are using chose '{git-default-editor}' as the default.
endif::git-default-editor[]

may be less confusing.

Re: [PATCH 2/2] var doc: advertise current DEFAULT_PAGER and DEFAULT_EDITOR settings

[Jonathan Nieder]

Hi Junio,

Junio C Hamano wrote:

> something like:
>
> 	... then the default chosen at compile time, which is usually 'vi'.
> ifdef::git-default-editor[]
> 	The build you are using chose '{git-default-editor}' as the default.
> endif::git-default-editor[]
>
> may be less confusing.

I was not sure if this text would interrupt the flow of reading
before, but I couldn't think of a good alternative so I didn't say
anything.  Sorry about that.  Now I've looked at the built result and
think the new text is good. :)

Thanks,
Jonathan

Re: [PATCH] Documentation: replace 'vi' for 'editor' to reflect build-time option

[Phil Hord]

On Thu, Mar 29, 2012 at 10:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Filling in the blank at compile time would be possible, but I'm not
>> convinced it's a good idea.  Wouldn't the same user be just as puzzled
>> when
>>
>>   http://git.kernel.org/?p=git/git-htmldocs.git;a=blob_plain;f=git-var.html
>>
>> (or whatever page with a nicer URL arises to replace the old
>> www.kernel.org/...  pages) says the fallback is 'vi' and git behaves
>> differently?
>
> I've already rejected this patch once, but that was primarily because the
> patch was not justified with the above "I read everybody else's git uses
> 'vi' on the Interweb, and even though my distro's manual page says it uses
> 'nano', I didn't bother to read it." scenario.

I like this original patch, and I like being able to refer to "the"
online docs wherever I and google may happen to find them.

Because "my distro" may be a resource-constrained minimal OS which
omitted the git manual.  The same system for similar reasons may
default to 'nano' & 'more' if vi and less are unavailable.

Phil