documentation/tools: Make *key.sh more portable

[kp]

I think that may be intentional.

Apparently (and I did not know this until I checked) ';&' is a control operator that causes it to continue with the next option. Basically where we'd use a /* fallthrough */ in C.

From the sh man page:

If a match is found, the corresponding list is executed. If the selected list
is terminated by the control operator ‘;&’ instead of ‘;;’, execution
continues with the next list, continuing until a list terminated with ‘;;’
or the end of the case command.

So in this case I think the intent is for the warning to be printed, but processing to continue as if it were actually a 16-digit keyid.

[markj]

In D46453#1058799, @kp wrote:

I think that may be intentional.

Apparently (and I did not know this until I checked) ';&' is a control operator that causes it to continue with the next option. Basically where we'd use a /* fallthrough */ in C.

Ooh, I didn't know this one either.

Igor, were you hitting this warning?

[igoro]

Aha, indeed. Now I know why I had the issue -- there is a difference between sh in FreeBSD and macOS. The latter one told me this:

> ./checkkey.sh 0x48FE9F99
./checkkey.sh: line 108: syntax error near unexpected token `;'
./checkkey.sh: line 108: `                      ;&'

The POSIX says :

An interesting undocumented feature of the KornShell is that using ";&" instead of ";;" as a terminator causes the exact opposite behavior—the flow of control continues with the next compound-list.

It explains why it's a rare beast in shell scripts. Well, it works as expected for FreeBSD's sh and it's intended to be used solely with FreeBSD -- the change is not required and is wrong.

At least, it was a good test of the git-arc -- an awesome tool.

[markj]

In D46453#1058803, @igoro wrote:

Aha, indeed. Now I know why I had the issue -- there is a difference between sh in FreeBSD and macOS. The latter one told me this:

> ./checkkey.sh 0x48FE9F99
./checkkey.sh: line 108: syntax error near unexpected token `;'
./checkkey.sh: line 108: `                      ;&'

The POSIX says :

An interesting undocumented feature of the KornShell is that using ";&" instead of ";;" as a terminator causes the exact opposite behavior—the flow of control continues with the next compound-list.

It explains why it's a rare beast in shell scripts. Well, it works as expected for FreeBSD's sh and it's intended to be used solely with FreeBSD -- the change is not required and is wrong.

Making the script more portable would be a worthy change IMO.

At least, it was a good test of the git-arc -- an awesome tool.

BTW, the source lives in tools/tools/git/git-arc.sh in the src repo, if you want to change anything there.

[igoro]

Make it portable instead

Having it a bit more portable seems to make sense. For instance, I used macOS because of my GnuPG setup is hosted there. The proposed patch seems to make the logic a bit more clear.

[kp]

That looks good to me. I've copied the original author, who's a bit stronger on shell scripting than I am.

[markj]

Looks good to me, but yes please give des@ some time to have a look.

[des]

documentation/tools/addkey.sh

80

There is a simpler solution, which is to add a continue to the * case and move the actual code out of the case..esac.

[igoro]

Implement des@ suggestion.

[des]

documentation/tools/checkkey.sh

116–121

Why is there a shift here? It's not in addkey and I can't see a reason for it, it causes checkkey to only check every other key listed on the command line.

[igoro]

It's turned out not to break the logic of iterating over the arguments, due to the for construct here has implicit in $@ which gets expanded before the loop itself.

As for me, the script makes a feeling that it's heavily based on arguments list as a storage for variables, and the parsing part from the very beginning shows that it tries to "consume" the arguments to keep the list empty for further re-usage or so. Probably, that's why we have shift here -- to consume it. Yes, the question is left open why the same is not done for addkey.sh.

I do not have strong opinions here how exactly we should deal with the legacy here, I'm open to any suggestion: 1) keep it as is, 2) add the same shift to addkey.sh, 3) remove it, 4) another option.

[des]

In D46453#1059500, @igoro wrote:

Yes, the question is left open why the same is not done for addkey.sh.

Not at all. I wrote addkey.sh, I know perfectly well that no shift is needed there. The question is why one was added here and if there's any reason not to remove it.

[igoro]

In D46453#1059780, @des wrote:

In D46453#1059500, @igoro wrote:

Yes, the question is left open why the same is not done for addkey.sh.

Not at all. I wrote addkey.sh, I know perfectly well that no shift is needed there. The question is why one was added here and if there's any reason not to remove it.

Yes, this question came up as findings during code reading, when two blocks of code that perform the same function have a difference in a single line and the history hides why. So far, I have not found a clear answer why the shift was added in the past. Okay, the option #3 then. Let’s remove it.

[igoro]

Remove redundant arg shift operation.

[kp]

Approved.

[jlduran]

Oh! Thank you! I just saw this revision. It was failing under a strictly POSIX shell.

Now to submit a bug to zsh (and ShellCheck):

% zsh ./documentation/tools/checkkey.sh
./documentation/tools/checkkey.sh:[:224: integer expression expected:  10 9 8 11

test_set.sh

#!/bin/sh

args_to_be="one two three"

set -- $args_to_be # XXX Do not quote

echo "$1"
echo "$2"
echo "$3"

% sh ./test_set.sh 
one
two
three

% zsh ./test_set.sh 
one two three