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

[<zeurkous@xxxxxxxx>]

Haai,

"Daniel Shahaf" <d.s@xxxxxxxxxxxxxxxxxx> wrote:
> Thanks for the patch. Review:
>
> zeurkous@xxxxxxxx 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).

> 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'.

> More below.
>
>> # These patches add, to the zshmisc(1) manual page, clarity about the
>> # exit status on exec failure.
>> #
>> # Me understands that, strictly considered, only Doc/Zsh/exec.yo needs
>> # updating; however, as me doesn't have yodl, me updated Doc/zshmisc.1
>> # as well.
>
> Thanks, but there's no need to manually update the .1 files; they aren't
> in version control.

Alright, me didn't realize. Me just has this little patch and me doesn't
know if me'll ever have anything to contribute again...

>
>> # 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).

> Moreover, the
> very next sentence says "the file is not in executable format", and it
> would be odd to refer to the same condition by different noun phrases in
> two consecutive sentences.

In me defense: the whole text is a bit vague, and me's not sure me
understands the subtleties enough to do a {,partial }rewrite of the
section.

> 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...?

> 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.

> Should this part of the manual mention the AUTO_CD option?

Dunno. It could unnecessarily complicate 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.

> Thanks for the patch!

You're welcome :)

> I realize the review is actually longer than the
> patch, but, it's still shorter than the cumulative number of times the
> new text will be read.

Yeah.

> Cheers,
>
> Daniel

Take care,

         --zeurkous.

>> If execution fails because the file is not in executable format,
>> and the file is not a directory, it is assumed to be a shell
>>

-- 
Friggin' Machines!