| Message ID | 20240406000902.3082301-2-gitster@pobox.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | local VAR="VAL" | expand |
On Fri, Apr 5, 2024 at 8:09 PM Junio C Hamano <gitster@pobox.com> wrote: > https://lore.kernel.org/git/201307081121.22769.tboegi@web.de/ > resulted in 9968ffff (test-lint: detect 'export FOO=bar', > 2013-07-08) to add a rule to t/check-non-portable-shell.pl script to > reject > > export VAR=VAL > > and suggest us to instead write it as "export VAR" followed by > "VAR=VAL". This however was not spelled out in the CodingGuidelines > document. I suspect you meant: ... and suggest us to instead write it as "VAR=VAL" followed by "export VAR". > We may want to re-evaluate the rule since it is from ages ago, but > for now, let's make the written rule and what the automation enforces > consistent. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines > @@ -188,6 +188,12 @@ For shell scripts specifically (not exhaustive): > hopefully nobody starts using "local" before they are reimplemented > in C ;-) > > + - Some versions of shell do not understand "export variable=value", > + so we write "export variable" and "variable=value" on separae s/separae/separate/ Here too, it might be clearer to swap around the pieces: ... so we write "variable=value" and "export variable" on... > + lines. Note that this was reported in 2013 and the situation might > + have changed since then. We'd need to re-evaluate this rule, > + together with the rule in t/check-non-portable-shell.pl script. The bit starting at "Note that..." seems more appropriate for the commit message (which is already the case) or a To-Do list. People reading this document are likely newcomers looking for concrete instructions about how to code for this project, and this sort of To-Do item isn't going to help them. (If anything, it might confuse them into ignoring the advice to split `export foo=bar` into two statements, which will result in reviewers asking them to reroll.)
Eric Sunshine <sunshine@sunshineco.com> writes: >> + lines. Note that this was reported in 2013 and the situation might >> + have changed since then. We'd need to re-evaluate this rule, >> + together with the rule in t/check-non-portable-shell.pl script. > > The bit starting at "Note that..." seems more appropriate for the > commit message (which is already the case) or a To-Do list. People > reading this document are likely newcomers looking for concrete > instructions about how to code for this project,... Very true. I thought I'd move some to the log message, but it turns out that enough is already described there. Thanks.
On Apr 06 2024, Eric Sunshine wrote: > On Fri, Apr 5, 2024 at 8:09 PM Junio C Hamano <gitster@pobox.com> wrote: >> https://lore.kernel.org/git/201307081121.22769.tboegi@web.de/ >> resulted in 9968ffff (test-lint: detect 'export FOO=bar', >> 2013-07-08) to add a rule to t/check-non-portable-shell.pl script to >> reject >> >> export VAR=VAL >> >> and suggest us to instead write it as "export VAR" followed by >> "VAR=VAL". This however was not spelled out in the CodingGuidelines >> document. > > I suspect you meant: > > ... and suggest us to instead write it as "VAR=VAL" followed by > "export VAR". There is no difference between them. The export command only marks the variable for export, independent of the current or future value of the variable. The exported value is always the last assigned one.
Andreas Schwab <schwab@linux-m68k.org> writes: >> I suspect you meant: >> >> ... and suggest us to instead write it as "VAR=VAL" followed by >> "export VAR". > > There is no difference between them. The export command only marks the > variable for export, independent of the current or future value of the > variable. The exported value is always the last assigned one. Correct. But we are talking about working around sub-standard (read: buggy) implementations and it is of dubious value to assume a compliant implementation when devising a workaround. It is easily imaginable that a sub-standard implementation uses a symbol table with a single "is it exported?" bit in addition to (name, value), without a way to say "this parameter is not set (yet)" (IOW, never value==NULL), and such an implementation would not be capable to have "this name is exported but nobody set the value to it yet". Using an assignment to make sure it is known before setting the exported bit is safer to protect against such an implementation.
On Sat, Apr 6, 2024 at 5:15 AM Andreas Schwab <schwab@linux-m68k.org> wrote: > On Apr 06 2024, Eric Sunshine wrote: > > On Fri, Apr 5, 2024 at 8:09 PM Junio C Hamano <gitster@pobox.com> wrote: > >> https://lore.kernel.org/git/201307081121.22769.tboegi@web.de/ > >> resulted in 9968ffff (test-lint: detect 'export FOO=bar', > >> 2013-07-08) to add a rule to t/check-non-portable-shell.pl script to > >> reject > >> > >> export VAR=VAL > >> > >> and suggest us to instead write it as "export VAR" followed by > >> "VAR=VAL". This however was not spelled out in the CodingGuidelines > >> document. > > > > I suspect you meant: > > > > ... and suggest us to instead write it as "VAR=VAL" followed by > > "export VAR". > > There is no difference between them. The export command only marks the > variable for export, independent of the current or future value of the > variable. The exported value is always the last assigned one. Yes, I know, but it is customary in this code-base to write it as: VAR=VAL && export VAR not the other way around, so it makes sense for CodingGuidelines to illustrate it in a fashion consistent with its use in the project.
diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines index 9495df835d..0a39205c48 100644 --- a/Documentation/CodingGuidelines +++ b/Documentation/CodingGuidelines @@ -188,6 +188,12 @@ For shell scripts specifically (not exhaustive): hopefully nobody starts using "local" before they are reimplemented in C ;-) + - Some versions of shell do not understand "export variable=value", + so we write "export variable" and "variable=value" on separae + lines. Note that this was reported in 2013 and the situation might + have changed since then. We'd need to re-evaluate this rule, + together with the rule in t/check-non-portable-shell.pl script. + - Use octal escape sequences (e.g. "\302\242"), not hexadecimal (e.g. "\xc2\xa2") in printf format strings, since hexadecimal escape sequences are not portable.
https://lore.kernel.org/git/201307081121.22769.tboegi@web.de/ resulted in 9968ffff (test-lint: detect 'export FOO=bar', 2013-07-08) to add a rule to t/check-non-portable-shell.pl script to reject export VAR=VAL and suggest us to instead write it as "export VAR" followed by "VAR=VAL". This however was not spelled out in the CodingGuidelines document. We may want to re-evaluate the rule since it is from ages ago, but for now, let's make the written rule and what the automation enforces consistent. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Documentation/CodingGuidelines | 6 ++++++ 1 file changed, 6 insertions(+)