PATCH: _diff

PATCH: _diff

[Oliver Kiddle]

This may have already been fixed but the diff completion seemed to call
diff with /dev/null as stdin but not stdout and stderr. This causes me
to get an error messages about -v not being an option in the middle of
my command line. I take it that redirecting stdin was a typo?

I seem to remember a recent discussion which I didn't follow fully about
how to call programs from completion functions and a proposed
configurable function for doing it so I haven't patched the use of
'command diff'. I leave the original diff and link GNU diff to 'gdiff'.
With the current _diff, I can't then get GNU style completion for gdiff.
Having both the GNU and supplied utilites are fairly common place on
commercial UNIX installations. For this reason, I object to using
non-local variables like _diff_is_gnu and calling 'diff' instead of
pulling out the first word on the command-line.

Oliver Kiddle

--- _diff_options.bak	Sat Jan 29 19:42:48 2000
+++ _diff_options	Thu Feb 10 12:34:33 2000
@@ -3,8 +3,8 @@
 local of ofwuc ouc oss ofwy ofwg ofwl
 
 (( $+_diff_is_gnu )) || {
-	_diff_is_gnu=0;
-        [[ $(command diff -v </dev/null) == *GNU* ]] && _diff_is_gnu=1
+	_diff_is_gnu=0
+        [[ $(command diff -v >/dev/null 2>&1) == *GNU* ]] &&
_diff_is_gnu=1
 }
 
 if (( _diff_is_gnu ))

Re: PATCH: _diff

[Bart Schaefer]

On Feb 10, 12:58pm, Oliver Kiddle wrote:
} Subject: PATCH: _diff
}
} This may have already been fixed but the diff completion seemed to call
} diff with /dev/null as stdin but not stdout and stderr.

I got "malformed patch" from this; line wrapping.

But why not use the more compact zsh syntax?  This instead of Oliver's:

Index: Completion/User/_diff_options
===================================================================
@@ -4,7 +4,7 @@
 
 (( $+_diff_is_gnu )) || {
 	_diff_is_gnu=0;
-        [[ $(command diff -v </dev/null) == *GNU* ]] && _diff_is_gnu=1
+        [[ $(command diff -v >&/dev/null) == *GNU* ]] && _diff_is_gnu=1
 }
 
 if (( _diff_is_gnu ))

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Re: PATCH: _diff

[Tanaka Akira]

In article <38A2B5EB.20014972@u.genie.co.uk>,
  Oliver Kiddle <opk@u.genie.co.uk> writes:

> -	_diff_is_gnu=0;
> -        [[ $(command diff -v </dev/null) == *GNU* ]] && _diff_is_gnu=1
> +	_diff_is_gnu=0
> +        [[ $(command diff -v >/dev/null 2>&1) == *GNU* ]] &&
> _diff_is_gnu=1

Since this (and Bart's equivalent) makes always _diff_is_gnu 0,
_diff_options completes always non-GNUish options as:

Z(2):akr@is27e1u11% Src/zsh -f
is27e1u11% bindkey -e; autoload -U compinit; compinit -D; compdef _tst tst
is27e1u11% diff -v
diff - GNU diffutils version 2.7
is27e1u11% diff -<TAB>
-b -- skip trailing white spaces
-c -- output a context diff
-e -- output an ed script
-f -- output a reversed ed script
-r -- recursively compare subdirectories

So, following patch should be applied instead of 9654/9667.

Index: Completion/User/_diff_options
===================================================================
RCS file: /projects/zsh/zsh/Completion/User/_diff_options,v
retrieving revision 1.1.1.2
diff -u -r1.1.1.2 _diff_options
--- Completion/User/_diff_options	2000/01/27 20:09:32	1.1.1.2
+++ Completion/User/_diff_options	2000/02/11 15:22:10
@@ -4,7 +4,7 @@
 
 (( $+_diff_is_gnu )) || {
 	_diff_is_gnu=0;
-        [[ $(command diff -v </dev/null) == *GNU* ]] && _diff_is_gnu=1
+        [[ $(command diff -v </dev/null 2>/dev/null) == *GNU* ]] && _diff_is_gnu=1
 }
 
 if (( _diff_is_gnu ))
-- 
Tanaka Akira

Re: PATCH: _diff

[Peter Stephenson]

Sven Wischnowsky wrote:
> Will I ever learn that?
>
> -    _options _parameters _path_files _requested
> +    _options _parameters _path_files _prefix _requested

Note that I usually fix up these problems by hand straight away, since it's
necessary for the distribution, and don't bother posting it, since I've got
enough to do anyway.  Hence what's in the distributed versions and
Changelog doesn't necessarily reflect .distfiles patches as they appear on
the mailing list.

-- 
Peter Stephenson <pws@pwstephenson.fsnet.co.uk>

Re: PATCH: _diff

[Sven Wischnowsky]

Alexandre Duret-Lutz wrote:

> ...
> 
> Ah, thanks. So the patch below correct this call to 
> _diff_options. (and also add _texi and _matcher to .distfiles)

Will I ever learn that?

Bye
 Sven

diff -ru ../z.old/Completion/Core/.distfiles Completion/Core/.distfiles
--- ../z.old/Completion/Core/.distfiles	Fri Feb 11 14:18:29 2000
+++ Completion/Core/.distfiles	Fri Feb 11 14:19:37 2000
@@ -3,7 +3,7 @@
     _alternative _approximate _compalso _complete _correct _description
     _expand _files _funcall _list _main_complete _match 
     _matcher _menu _multi_parts _message _normal _oldlist 
-    _options _parameters _path_files _requested
+    _options _parameters _path_files _prefix _requested
     _sep_parts _set_options _setup _sort_tags _tags
     _unset_options _wanted
     compdump compinit compinstall

--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: PATCH: _diff

[Alexandre Duret-Lutz]

>>> "Sven" == Sven Wischnowsky <wischnow@informatik.hu-berlin.de> writes:

 Sven> Alexandre Duret-Lutz wrote:

[...]

 >> '--[introduce diff options]:*:diff options: _diff_options'
 >> 
 >> I can't use the `*::message:action' syntax here.

[...]

 Sven> Of course, `--:*::...' should work (reporting anything after the
 Sven> `--'), there was just a little thinko in ca_parse_line().

Ah, thanks. So the patch below correct this call to 
_diff_options. (and also add _texi and _matcher to .distfiles)

[...]

Index: Completion/User/.distfiles
--- Completion/User/.distfiles Wed, 02 Feb 2000 23:44:58 +0100 Alexandre
+++ Completion/User/.distfiles Fri, 11 Feb 2000 13:45:29 +0100 Alexandre
@@ -8,7 +8,7 @@
     _netscape _nslookup _other_accounts _pack _patch _pbm _pdf
     _perl_basepods _perl_builtin_funcs _perl_modules _perldoc
     _ports _prcs _prompt _ps _pspdf _rcs _rlogin _sh _socket
-    _ssh _strip _stty _su _sudo _tar _tar_archive _telnet _tex
+    _ssh _strip _stty _su _sudo _tar _tar_archive _telnet _tex _texi
     _tiff _uncompress _unpack _urls _use_lo _user_at_host _users
     _users_on _webbrowser _wget _whereis _whois _xargs _yodl _yp
     _zdump
Index: Completion/Core/.distfiles
--- Completion/Core/.distfiles Fri, 31 Dec 1999 13:32:44 +0100 Alexandre
+++ Completion/Core/.distfiles Fri, 11 Feb 2000 13:47:39 +0100 Alexandre
@@ -1,8 +1,9 @@
 DISTFILES_SRC='
     .distfiles
     _alternative _approximate _compalso _complete _correct _description
-    _expand _files _funcall _list _main_complete _match _menu _multi_parts
-    _message _normal _oldlist _options _parameters _path_files _requested
+    _expand _files _funcall _list _main_complete _match 
+    _matcher _menu _multi_parts _message _normal _oldlist 
+    _options _parameters _path_files _requested
     _sep_parts _set_options _setup _sort_tags _tags
     _unset_options _wanted
     compdump compinit compinstall
Index: Completion/User/_prcs
--- Completion/User/_prcs Wed, 02 Feb 2000 23:44:58 +0100 Alexandre
+++ Completion/User/_prcs Fri, 11 Feb 2000 13:44:34 +0100 Alexandre
@@ -133,7 +133,7 @@
       '(--new)-N[compare new files against empty files]' \
       "(-P)--exclude-project-file[don't diff the project file]" \
       "(--exclude-project-file)-P[don't diff the project file]" \
-      '--[introduce diff options]:*:diff options: _diff_options' \
+      '--[introduce diff options]:*::diff options: _diff_options' \
       ':project name:_prcs_projects' \
       '*:file or directory:_files'
     ;;


-- 
Alexandre Duret-Lutz

Re: PATCH: _diff

[Sven Wischnowsky]

Alexandre Duret-Lutz wrote:

> X-Seq: 9675
> 
> >>> "Sven" == Sven Wischnowsky <wischnow@informatik.hu-berlin.de> writes:
> 
>  Sven> Oliver Kiddle wrote:
> 
>  >> I object to using
>  >> non-local variables like _diff_is_gnu and calling 'diff' instead of
>  >> pulling out the first word on the command-line.
> 
>  Sven> Well, if we use something like _diff_is_gnu[$words[1]], using a
>  Sven> non-local variable should be fine, right?
> 
> I don't think so, because _diff_options may be called from places where
> $words[1] is not the command name, e.g. called from _prcs (maybe _cvs_diff
> could call it too).

Err, right, I forgot...

> Hum, looking closer to _prcs, I see there is a problem, if I do
> 
> % prcs diff -P zsh -- -u <TAB>
> 
> the values of $words when entering the _diff_options functions are
> 
> diff -P zsh -- -u 
> 
> (this begin with diff since $words was shifted in _prcs)
> and the -P option is removed from the diff options list, which is wrong.
> I'd like this array te be restricted to
> 
> -- -u
> 
> unfortunately, I don't see how to do that with _arguments.
> The _arguments line calling _diff_options reads
> 
> '--[introduce diff options]:*:diff options: _diff_options'
> 
> I can't use the `*::message:action' syntax here.  So I guess
> I have to write an intermediate function that shift the $words 
> array $CURRENT times before calling _diff_options.  Or I am missing
> some syntaxic sugar in _arguments?

Of course, `--:*::...' should work (reporting anything after the
`--'), there was just a little thinko in ca_parse_line().

Bye
 Sven

diff -ru ../z.old/Src/Zle/computil.c Src/Zle/computil.c
--- ../z.old/Src/Zle/computil.c	Fri Feb 11 09:23:09 2000
+++ Src/Zle/computil.c	Fri Feb 11 11:46:04 2000
@@ -1235,12 +1235,12 @@
 	    else {
 		LinkList l = state.oargs[state.curopt->num];
 
+		if (cur < compcurrent)
+		    memcpy(&ca_laststate, &state, sizeof(state));
 		PERMALLOC {
 		    for (; line; line = compwords[cur++])
 			addlinknode(l, ztrdup(line));
 		} LASTALLOC;
-		if (cur < compcurrent)
-		    memcpy(&ca_laststate, &state, sizeof(state));
 		ca_laststate.ddef = NULL;
 		ca_laststate.doff = 0;
 		break;

--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: PATCH: _diff

[Alexandre Duret-Lutz]

>>> "Sven" == Sven Wischnowsky <wischnow@informatik.hu-berlin.de> writes:

 Sven> Oliver Kiddle wrote:

 >> I object to using
 >> non-local variables like _diff_is_gnu and calling 'diff' instead of
 >> pulling out the first word on the command-line.

 Sven> Well, if we use something like _diff_is_gnu[$words[1]], using a
 Sven> non-local variable should be fine, right?

I don't think so, because _diff_options may be called from places where
$words[1] is not the command name, e.g. called from _prcs (maybe _cvs_diff
could call it too).

Hum, looking closer to _prcs, I see there is a problem, if I do

% prcs diff -P zsh -- -u <TAB>

the values of $words when entering the _diff_options functions are

diff -P zsh -- -u 

(this begin with diff since $words was shifted in _prcs)
and the -P option is removed from the diff options list, which is wrong.
I'd like this array te be restricted to

-- -u

unfortunately, I don't see how to do that with _arguments.
The _arguments line calling _diff_options reads

'--[introduce diff options]:*:diff options: _diff_options'

I can't use the `*::message:action' syntax here.  So I guess
I have to write an intermediate function that shift the $words 
array $CURRENT times before calling _diff_options.  Or I am missing
some syntaxic sugar in _arguments?

[...]

-- 
Alexandre Duret-Lutz

Re: PATCH: _diff

[Oliver Kiddle]

Sven Wischnowsky wrote:
> 
> Well, if we use something like _diff_is_gnu[$words[1]], using a
> non-local variable should be fine, right?

It'd be fine though I suppose that if we're going to be pedantic it
wouldn't: $words[1] could include a path to the command which is
relative to the current directory so it would refer to a different
command after a cd. I don't think that would matter much though.

Is there a quick and easy way to turn a directory name into a full
non-relative path (like the DOS truename command). I've done things like
a=$(cd $d;pwd) in scripts before. An expansion flag of some sort to do
it would be useful (if one doesn't already exist).

Oliver

Re: PATCH: _diff

[Sven Wischnowsky]

Oliver Kiddle wrote:

> This may have already been fixed but the diff completion seemed to call
> diff with /dev/null as stdin but not stdout and stderr. This causes me
> to get an error messages about -v not being an option in the middle of
> my command line. I take it that redirecting stdin was a typo?
> 
> I seem to remember a recent discussion which I didn't follow fully about
> how to call programs from completion functions and a proposed
> configurable function for doing it so I haven't patched the use of
> 'command diff'. I leave the original diff and link GNU diff to 'gdiff'.
> With the current _diff, I can't then get GNU style completion for gdiff.
> Having both the GNU and supplied utilites are fairly common place on
> commercial UNIX installations. For this reason, I object to using
> non-local variables like _diff_is_gnu and calling 'diff' instead of
> pulling out the first word on the command-line.

Well, if we use something like _diff_is_gnu[$words[1]], using a
non-local variable should be fine, right?

But you are right -- I had forgotten to integrate using $words[1] into 
my suggestion somehow. Hm.

Still no patch for any of this, though. The problem is that this would
probably be interesting in several places so that updating the docs
would be quite a bit of work. And since we also had the `discussion'
about trying to add some auto-documentation mechanism (using magic
comments in the functions) I wanted to delay it until we had thought
about that some more.

Bye
 Sven


--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de