From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#15461: 24.3; [PATCH] exec-path on ms windows should contain current directory Date: Thu, 26 Dec 2013 18:23:19 +0200 Message-ID: <834n5v60p4.fsf@gnu.org> References: <5242E42A.4050109@poczta.onet.pl> <52BB66E7.2060302@poczta.onet.pl> Reply-To: Eli Zaretskii NNTP-Posting-Host: plane.gmane.org X-Trace: ger.gmane.org 1388075062 31566 80.91.229.3 (26 Dec 2013 16:24:22 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Thu, 26 Dec 2013 16:24:22 +0000 (UTC) Cc: 15461@debbugs.gnu.org To: Jarek Czekalski Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Thu Dec 26 17:24:23 2013 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1VwDjM-0006oQ-Fg for geb-bug-gnu-emacs@m.gmane.org; Thu, 26 Dec 2013 17:24:16 +0100 Original-Received: from localhost ([::1]:46161 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VwDjM-0006FM-23 for geb-bug-gnu-emacs@m.gmane.org; Thu, 26 Dec 2013 11:24:16 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:38014) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VwDjE-0006Ep-GT for bug-gnu-emacs@gnu.org; Thu, 26 Dec 2013 11:24:14 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VwDj8-0007xL-Sy for bug-gnu-emacs@gnu.org; Thu, 26 Dec 2013 11:24:08 -0500 Original-Received: from debbugs.gnu.org ([140.186.70.43]:58281) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VwDj8-0007xE-PQ for bug-gnu-emacs@gnu.org; Thu, 26 Dec 2013 11:24:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.80) (envelope-from ) id 1VwDj7-0007Vm-P3 for bug-gnu-emacs@gnu.org; Thu, 26 Dec 2013 11:24:01 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 26 Dec 2013 16:24:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 15461 X-GNU-PR-Package: emacs,w32 X-GNU-PR-Keywords: Original-Received: via spool by 15461-submit@debbugs.gnu.org id=B15461.138807500528781 (code B ref 15461); Thu, 26 Dec 2013 16:24:01 +0000 Original-Received: (at 15461) by debbugs.gnu.org; 26 Dec 2013 16:23:25 +0000 Original-Received: from localhost ([127.0.0.1]:44067 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.80) (envelope-from ) id 1VwDiV-0007U5-VZ for submit@debbugs.gnu.org; Thu, 26 Dec 2013 11:23:25 -0500 Original-Received: from mtaout23.012.net.il ([80.179.55.175]:46543) by debbugs.gnu.org with esmtp (Exim 4.80) (envelope-from ) id 1VwDiR-0007Tr-Vf for 15461@debbugs.gnu.org; Thu, 26 Dec 2013 11:23:21 -0500 Original-Received: from conversion-daemon.a-mtaout23.012.net.il by a-mtaout23.012.net.il (HyperSendmail v2007.08) id <0MYF00M009BZTW00@a-mtaout23.012.net.il> for 15461@debbugs.gnu.org; Thu, 26 Dec 2013 18:23:18 +0200 (IST) Original-Received: from HOME-C4E4A596F7 ([87.69.4.28]) by a-mtaout23.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0MYF00M2E9ITAD50@a-mtaout23.012.net.il>; Thu, 26 Dec 2013 18:23:18 +0200 (IST) In-reply-to: <52BB66E7.2060302@poczta.onet.pl> X-012-Sender: halo1@inter.net.il X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.15 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 140.186.70.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.bugs:82625 Archived-At: > Date: Thu, 26 Dec 2013 00:14:47 +0100 > From: Jarek Czekalski > > 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.