bug#15461: 24.3; [PATCH] exec-path on ms windows should contain current directory

[Eli Zaretskii]

> Date: Thu, 26 Dec 2013 00:14:47 +0100
> From: Jarek Czekalski <jarekczek@poczta.onet.pl>
> 
> I am attaching a patch to fix this thing.

Thanks.

> - Whether such broad changes in manual may be done together with a lisp 
> change (new variable)

If they are related, yes.

> - Whether an unnumbered subsubsection Shell Mode Completion Options in 
> Shell Mode Options is a good idea

It's not a catastrophe, but a @node is preferable.  Except that in
this case, I don't think even a node is justified, as you only added a
single variable to a node that was not very large anyway.

> - Whether anchors can be used, because our info viewer does not follow 
> them correctly

They work fine for me, both with the current trunk and with Emacs
24.3.  Do you see the problem in 'emacs -Q"?  If so, what exactly
doesn't work?

> - Whether this new variable may be introduced despite the feature freeze

I'm not sure it should be introduced at all.  My reasoning:

 . Users of Posix platforms that would want this behavior will most
   probably already have "." in their PATH, and exec-path will inherit
   that.

 . Users of Windows who do _not_ want this are IMO insane (pardon my
   French), because every Windows program and shell will behave like
   that, whether they want it or not

So I think we should just behave on each platform as its users should
expect.

> - Whether a new position in concept index (shell completion) is fine

It is fine, but you added 2 identical index entries, which is
generally not recommended, because they will be presented to the user
by the 'i' command as "shell completion<1>" and "shell completion<2>",
something that will make it hard to decide which one to choose.
Instead, qualify each entry with some context of what is described in
each place about shell completion.  Alternatively, decide which of the
two index entries is not really necessary, and remove it.

> Suggested commit message:
> 
> Introduce a variable shell-completion-cur-dir and document it.
> Document a related detail in exec-path variable.

It would be better to have only one header line in the commit message,
because that's what "bzr log --line" shows for each commit.  The rest
of the details will be visible from the ChangeLog entries that you
will copy into the commit message.

> - move documentation of shell-completion-fignore from Shell Mode to Shell
>    Mode Options,
> - group Shell Mode Completion Options into a new unnumbered subsubsection.

As I wrote above, I'm not sure a new subsection is justified in this
case.

>      * doc/misc.texi (Shell Mode): Document new variable
>      shell-completion-cur-dir.
>      Move documentation of shell-completion-fignore from Shell Mode to Shell
>      Mode Options.
>      Add an unnumbered subsubsection to group completion options. This also
>      required to move pushd paragraph above this group.

In general, don't start from a new line when you describe changes to
the same node.  Just continue.

> +   Shell completion is an extended version of filename completion,
> + @xref{Shell Mode Completion Options}.

@xref is not appropriate in the middle of a sentence, because it
generates text that begins with a capital letter.  (You don't see that
in Emacs, because Emacs by default hides that text and replaces it
with something it invents.  But it is clearly visible in the
stand-alone Info reader, and even more acutely visible in the printed
output.)  Instead, either use "see @ref", or start a new sentence with
@xref.

> + ---

Why "---"?  It means this does not need to be documented.  You want
"+++" instead, which means "already documented".

> + *** New customizable variable shell-completion-cur-dir, which controls
> + whether filenames from current directory will be found by
> + completion. Initialized to t on systems on which this is default
              ^^
Two spaces between sentences, please.

> + (defcustom shell-completion-cur-dir
> +   (member system-type '(windows-nt ms-dos t))

Why 'member' and not 'memq'?  And why did you put t at the end of the
list?

> +   "Non-nil if completion should match filenames from the current buffer's
> + directory. Initialized to t on systems in which this behavior is a default."
             ^^                                                      ^^^^^^^^^
Two spaces, and "the default".

Btw, it is OK to mention those systems explicitly, there's no need for
obscurity here.

I would also suggest a slight rewording:

  Non-nil if shell completion should match executable filenames from
  the current buffer's directory.

> *************** Returns t if successful."
> *** 1138,1144 ****
>            (start (if (zerop (length filename)) (point) (match-beginning 0)))
>            (end (if (zerop (length filename)) (point) (match-end 0)))
>        (filenondir (file-name-nondirectory filename))
> !      (path-dirs (cdr (reverse exec-path))) ;FIXME: Why `cdr'?
>        (cwd (file-name-as-directory (expand-file-name default-directory)))
>        (ignored-extensions
>         (and comint-completion-fignore
> --- 1146,1155 ----
>            (start (if (zerop (length filename)) (point) (match-beginning 0)))
>            (end (if (zerop (length filename)) (point) (match-end 0)))
>        (filenondir (file-name-nondirectory filename))
> !          ; why cdr? see `shell-dynamic-complete-command', however on Windows
> !          ; we have 3 library directories and this does not fully work
> !      (path-dirs (append (cdr (reverse exec-path))
> !            (if shell-completion-cur-dir '("."))))

The remark about Windows is no longer true on the trunk, and I think
comments should explain better than that.  In any case, this is a
completely separate issue, for which I will start a new thread.

>     DEFVAR_LISP ("exec-path", Vexec_path,
>              doc: /* List of directories to search programs to run in 
> subprocesses.
> ! Each element is a string (directory name) or nil (try default directory).
> ! 
> ! By default the last element of this list is Emacs library path. The
> ! last element is not always used, for example in shell completion
> ! (`shell-dynamic-complete-command').  */);

This also belongs to that separate issue (and "library path" is a term
that we don't use anywhere else, except in the part of the manual
which you modified, so I think we should not use it in the doc
string).  See there.

Bottom line: I think the change in shell--command-completion-data
should go in, modulo the comment.  I don't think we need the new
option, but if Stefan decides otherwise, I won't object.

As to the changes in the manual, if we don't add the option, I'd leave
the manual alone, except for the indexing.  If we do add the option, I
think it could be left in the same node where we describe the other
options.

Thanks again for working on this.