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

Hi Rodrigo,

Rodrigo Silva (MestreLion) wrote:

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(-)

Jonathan Nieder <jrnieder@gmail.com> writes:

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.

At 23:16 29/3/2012, Junio C Hamano wrote:

@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:

Junio C Hamano wrote:

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(-)

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(-)

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(+)

Jonathan Nieder <jrnieder@gmail.com> writes:

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:

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.

Hi Junio,

Junio C Hamano wrote:

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

On Thu, Mar 29, 2012 at 10:16 PM, Junio C Hamano <gitster@pobox.com> wrote:

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

Rodrigo Silva <linux@rodrigosilva.com> writes:

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.

Hi Rodrigo,

Rodrigo Silva wrote:
 

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.

[...]

It is not hardcoded.

[...]

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