RE: patch: zshmisc(1) clarify non-successful exit statuses

[<zeurkous@volny.cz>]

Haai,

"Daniel Shahaf" <d.s@daniel.shahaf.name> wrote:
> zeurkous@volny.cz wrote on Tue, Apr 13, 2021 at 20:03:41 +0200:
>> "Daniel Shahaf" <d.s@daniel.shahaf.name> wrote:
>> > Thanks for the patch. Review:
>> >
>> > zeurkous@volny.cz wrote on Sun, Apr 11, 2021 at 14:15:20 +0200:
>> >> #?patch
>> >> #
>> >
>> > What's this header line? Is this a standard format for unidiffs with
>> > log messages? Should Functions/VCS_Info/VCS_INFO_patch2subject grow
>> > support for it?
>>
>> No, it's a "shehuh" (me own convention), to indicate the format (in this
>> case: input to patch(1)). Ignore it if you wish. The rest of the '#'
>> lines are just comments (meknows patch(1) will relay && discard any
>> lines before the actual patch, but IMO it shouldn't, hence the prefix).
>>
>> patch(1) will eat all this fine (at least it does on me side).
>
> Every patch(1) implementation in the last few decades behaves this way.

That's me experience, as well. However, me can't really guarantee that
some clever-ass (like me :) didn't write a patch(1) w/ diff behaviour,
hence the caveat.

>> > zsh's source code is in git. git's interchange format is `[PATCH]' in
>> > the subject line, then in the body, everything up to a "---" line is
>> > part of the log message, and everything after is not. See
>> > git-format-patch(1) for details.
>>
>> Thanks, but me doesn't use git (me has me own, very sufficient, ways to
>> keep track of things). So me just used 'diff -u'.
>
> You can get svn working copies and tar.gz or zip exports from
> https://github.com/zsh-users/zsh/.

Me'll keep that in mind, should me have another contribution.

>> >> # Hope this is useful (it is to me),
>> >> #
>> >> # --zeurkous, Sun Apr 11 11:12:21 UTC 2021.
>> >> #
>> >> --- Doc/Zsh/..v/5.8/exec.yo Mon Dec 4 14:09:35 2017
>> >> +++ Doc/Zsh/exec.yo Sun Apr 11 10:42:15 2021
>> >> @@ -16,7 +16,10 @@
>> >> Otherwise, the shell searches each element of tt($path) for a
>> >> directory containing an executable file by that name. If the
>> >> search is unsuccessful, the shell prints an error message and returns
>> >> -a nonzero exit status.
>> >> +127.
>> >> +
>> >> +If execution fails because of insufficient permissions, or because the
>> >> +file is a directory, the shell prints an error message and returns 126.
>> >>
>> >
>> > Does this sentence cover every possible case of returning 126? The
>> > condition in the source is «== EACCES || == ENOEXEC».
>>
>> Me wishes me had a guarantee. It probably differs from system to system
>> (me runs OpenBSD); this is me best shot (for now).
>
> Does that mean you won't be submitting a revised patch? No problem if
> so; just want to make it explicit whether or not revising the patch is
> a task that's up for grabs.

It's me best shot at understanding the behaviour, sorry for not being
clear meself here. Me's a bit busy but me might well come up w/ a
revised patch in a little while -- me'll let you folks know. 

>> > I don't like the newly-added paragraph break. Anyone who stops reading
>> > at the end of that paragraph will think the return code is 127, period.
>>
>> Why would someone stop reading there...?
>
> People skim.

That's understandable, but also at their own risk, me'd argue.

>> > Also, stating the return values before going on to say that if the file
>> > isn't a directory then it's exeuted anyway could be confusing, couldn't it?
>>
>> In all honesty: to me, the whole text is so unnecessarily unclear that
>> me'd apply me usual solution of writing exit statuses in a table:
>>
>> If execution fails: one of the following values is returned.
>> .Pp
>> .Bl -tag -width 126 -compact
>> .It Li 126
>> [...summarize the condition{,s} that cause{,s} it to return that...]
>> .It Li 127
>> [...idem...]
>> .El
>>
>> (That's in mdoc(7), not in man(7). Sorry.)
>>
>> However, if you folks would desire that: me has no idea how to write
>> that in yodl, so mecan't provide a patch.
>
> In this case, a startsitem() would probably work well. There's plenty
> of examples in our tree.

Thanks for the reference, me'll probably have a look.

>> > Should this part of the manual mention the AUTO_CD option?
>>
>> Dunno. It could unnecessarily complicate it.
>>
>
> "Unnecessarily"? Doesn't AUTO_CD logically belong in that part of the manual?

Me doesn't use AUTO_CD, but if me understands the functionality
correctly: it allows a dir path name to be specified as a program name,
with the effect of a cd into the specified directory. To  me, that's a
translation: '/blaat/' -> 'cd /blaat/'. The return status of the former
should then naturally be the return status of the latter.

At least, that's me take on it.

>> > A few paragraph below the value, 127, is mentioned again. Does that
>> > sentence need to be updated?
>>
>> Me'd say that if the text above would be made sufficiently clear (with
>> quotes of both diagnostic messages), that paragraph could be
>> significantly curtailed, instead of extended.
>
> That's what I meant.

Then we agree on that one :)

> Daniel

         --zeurkous.

-- 
Friggin' Machines!