[PATCH] scripts/setlocalversion: fix a bug when LOCALVERSION is empty

[PATCH] scripts/setlocalversion: fix a bug when LOCALVERSION is empty

[Mikulas Patocka]

The patch 042da426f8ebde012be9429ff705232af7ad7469 
("scripts/setlocalversion: simplify the short version part") reduces the 
indentation. Unfortunatelly, it also changes behavior in a subtle way - if 
the user has empty "LOCALVERSION" variable, the plus sign is appended to 
the kernel version. It wasn't appended before.

This patch reverts to the old behavior - we append the plus sign only if
the LOCALVERSION variable is not set.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Fixes: 042da426f8eb ("scripts/setlocalversion: simplify the short version part")

---
 scripts/setlocalversion |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/scripts/setlocalversion
===================================================================
--- linux-2.6.orig/scripts/setlocalversion	2021-07-12 15:29:07.000000000 +0200
+++ linux-2.6/scripts/setlocalversion	2021-07-12 15:50:29.000000000 +0200
@@ -131,7 +131,7 @@ res="${res}${CONFIG_LOCALVERSION}${LOCAL
 if test "$CONFIG_LOCALVERSION_AUTO" = "y"; then
 	# full scm version string
 	res="$res$(scm_version)"
-elif [ -z "${LOCALVERSION}" ]; then
+elif [ "${LOCALVERSION+set}" != "set" ]; then
 	# append a plus sign if the repository is not in a clean
 	# annotated or signed tagged state (as git describe only
 	# looks at signed or annotated tags - git tag -a/-s) and

Re: [PATCH] scripts/setlocalversion: fix a bug when LOCALVERSION is empty

[Greg Kroah-Hartman]

On Mon, Jul 12, 2021 at 10:00:59AM -0400, Mikulas Patocka wrote:
> The patch 042da426f8ebde012be9429ff705232af7ad7469 
> ("scripts/setlocalversion: simplify the short version part") reduces the 
> indentation. Unfortunatelly, it also changes behavior in a subtle way - if 
> the user has empty "LOCALVERSION" variable, the plus sign is appended to 
> the kernel version. It wasn't appended before.
> 
> This patch reverts to the old behavior - we append the plus sign only if
> the LOCALVERSION variable is not set.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Fixes: 042da426f8eb ("scripts/setlocalversion: simplify the short version part")
> 
> ---
>  scripts/setlocalversion |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux-2.6/scripts/setlocalversion
> ===================================================================
> --- linux-2.6.orig/scripts/setlocalversion	2021-07-12 15:29:07.000000000 +0200
> +++ linux-2.6/scripts/setlocalversion	2021-07-12 15:50:29.000000000 +0200
> @@ -131,7 +131,7 @@ res="${res}${CONFIG_LOCALVERSION}${LOCAL
>  if test "$CONFIG_LOCALVERSION_AUTO" = "y"; then
>  	# full scm version string
>  	res="$res$(scm_version)"
> -elif [ -z "${LOCALVERSION}" ]; then
> +elif [ "${LOCALVERSION+set}" != "set" ]; then

That's really subtle, can you add a comment here that this handles an
empty file?

thanks,

greg k-h

[PATCH v2] scripts/setlocalversion: fix a bug when LOCALVERSION is empty

[Mikulas Patocka]

On Mon, 12 Jul 2021, Greg Kroah-Hartman wrote:

> On Mon, Jul 12, 2021 at 10:00:59AM -0400, Mikulas Patocka wrote:
> > The patch 042da426f8ebde012be9429ff705232af7ad7469 
> > ("scripts/setlocalversion: simplify the short version part") reduces the 
> > indentation. Unfortunatelly, it also changes behavior in a subtle way - if 
> > the user has empty "LOCALVERSION" variable, the plus sign is appended to 
> > the kernel version. It wasn't appended before.
> > 
> > This patch reverts to the old behavior - we append the plus sign only if
> > the LOCALVERSION variable is not set.
> > 
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > Fixes: 042da426f8eb ("scripts/setlocalversion: simplify the short version part")
> > 
> > ---
> >  scripts/setlocalversion |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > Index: linux-2.6/scripts/setlocalversion
> > ===================================================================
> > --- linux-2.6.orig/scripts/setlocalversion	2021-07-12 15:29:07.000000000 +0200
> > +++ linux-2.6/scripts/setlocalversion	2021-07-12 15:50:29.000000000 +0200
> > @@ -131,7 +131,7 @@ res="${res}${CONFIG_LOCALVERSION}${LOCAL
> >  if test "$CONFIG_LOCALVERSION_AUTO" = "y"; then
> >  	# full scm version string
> >  	res="$res$(scm_version)"
> > -elif [ -z "${LOCALVERSION}" ]; then
> > +elif [ "${LOCALVERSION+set}" != "set" ]; then
> 
> That's really subtle, can you add a comment here that this handles an
> empty file?
> 
> thanks,
> 
> greg k-h

OK.


From: Mikulas Patocka <mpatocka@redhat.com>

The patch 042da426f8ebde012be9429ff705232af7ad7469
("scripts/setlocalversion: simplify the short version part") reduces
indentation. Unfortunatelly, it also changes behavior in a subtle way - if
the user has empty "LOCALVERSION" variable, the plus sign is appended to
the kernel version. It wasn't appended before.

This patch reverts to the old behavior - we append the plus sign only if
the LOCALVERSION variable is not set.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Fixes: 042da426f8eb ("scripts/setlocalversion: simplify the short version part")

---
 scripts/setlocalversion |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Index: linux-2.6/scripts/setlocalversion
===================================================================
--- linux-2.6.orig/scripts/setlocalversion	2021-07-12 15:29:07.000000000 +0200
+++ linux-2.6/scripts/setlocalversion	2021-07-12 21:00:59.000000000 +0200
@@ -131,11 +131,14 @@ res="${res}${CONFIG_LOCALVERSION}${LOCAL
 if test "$CONFIG_LOCALVERSION_AUTO" = "y"; then
 	# full scm version string
 	res="$res$(scm_version)"
-elif [ -z "${LOCALVERSION}" ]; then
-	# append a plus sign if the repository is not in a clean
-	# annotated or signed tagged state (as git describe only
-	# looks at signed or annotated tags - git tag -a/-s) and
-	# LOCALVERSION= is not specified
+elif [ "${LOCALVERSION+set}" != "set" ]; then
+	# If the variable LOCALVERSION is not set, append a plus
+	# sign if the repository is not in a clean annotated or
+	# signed tagged state (as git describe only looks at signed
+	# or annotated tags - git tag -a/-s).
+	#
+	# If the variable LOCALVERSION is set (including being set
+	# to an empty string), we don't want to append a plus sign.
 	scm=$(scm_version --short)
 	res="$res${scm:++}"
 fi

Re: [PATCH v2] scripts/setlocalversion: fix a bug when LOCALVERSION is empty

[Greg Kroah-Hartman]

On Mon, Jul 12, 2021 at 03:06:48PM -0400, Mikulas Patocka wrote:
> 
> 
> On Mon, 12 Jul 2021, Greg Kroah-Hartman wrote:
> 
> > On Mon, Jul 12, 2021 at 10:00:59AM -0400, Mikulas Patocka wrote:
> > > The patch 042da426f8ebde012be9429ff705232af7ad7469 
> > > ("scripts/setlocalversion: simplify the short version part") reduces the 
> > > indentation. Unfortunatelly, it also changes behavior in a subtle way - if 
> > > the user has empty "LOCALVERSION" variable, the plus sign is appended to 
> > > the kernel version. It wasn't appended before.
> > > 
> > > This patch reverts to the old behavior - we append the plus sign only if
> > > the LOCALVERSION variable is not set.
> > > 
> > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > > Fixes: 042da426f8eb ("scripts/setlocalversion: simplify the short version part")
> > > 
> > > ---
> > >  scripts/setlocalversion |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > Index: linux-2.6/scripts/setlocalversion
> > > ===================================================================
> > > --- linux-2.6.orig/scripts/setlocalversion	2021-07-12 15:29:07.000000000 +0200
> > > +++ linux-2.6/scripts/setlocalversion	2021-07-12 15:50:29.000000000 +0200
> > > @@ -131,7 +131,7 @@ res="${res}${CONFIG_LOCALVERSION}${LOCAL
> > >  if test "$CONFIG_LOCALVERSION_AUTO" = "y"; then
> > >  	# full scm version string
> > >  	res="$res$(scm_version)"
> > > -elif [ -z "${LOCALVERSION}" ]; then
> > > +elif [ "${LOCALVERSION+set}" != "set" ]; then
> > 
> > That's really subtle, can you add a comment here that this handles an
> > empty file?
> > 
> > thanks,
> > 
> > greg k-h
> 
> OK.
> 
> 
> From: Mikulas Patocka <mpatocka@redhat.com>
> 

I can't take a patch like this :(

> The patch 042da426f8ebde012be9429ff705232af7ad7469
> ("scripts/setlocalversion: simplify the short version part") reduces

Properly quote commits, the documentation says you do not need to use
the full sha1.


> indentation. Unfortunatelly, it also changes behavior in a subtle way - if
> the user has empty "LOCALVERSION" variable, the plus sign is appended to
> the kernel version. It wasn't appended before.
> 
> This patch reverts to the old behavior - we append the plus sign only if
> the LOCALVERSION variable is not set.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Fixes: 042da426f8eb ("scripts/setlocalversion: simplify the short version part")

See, use that way to quote a commit.

thanks,

greg k-h

[PATCH v3] scripts/setlocalversion: fix a bug when LOCALVERSION is empty

[Mikulas Patocka]

On Mon, 12 Jul 2021, Greg Kroah-Hartman wrote:

> On Mon, Jul 12, 2021 at 03:06:48PM -0400, Mikulas Patocka wrote:
> > 
> > 
> > On Mon, 12 Jul 2021, Greg Kroah-Hartman wrote:
> > 
> > > On Mon, Jul 12, 2021 at 10:00:59AM -0400, Mikulas Patocka wrote:
> > > > The patch 042da426f8ebde012be9429ff705232af7ad7469 
> > > > ("scripts/setlocalversion: simplify the short version part") reduces the 
> > > > indentation. Unfortunatelly, it also changes behavior in a subtle way - if 
> > > > the user has empty "LOCALVERSION" variable, the plus sign is appended to 
> > > > the kernel version. It wasn't appended before.
> > > > 
> > > > This patch reverts to the old behavior - we append the plus sign only if
> > > > the LOCALVERSION variable is not set.
> > > > 
> > > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > > > Fixes: 042da426f8eb ("scripts/setlocalversion: simplify the short version part")
> > > > 
> > > > ---
> > > >  scripts/setlocalversion |    2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > Index: linux-2.6/scripts/setlocalversion
> > > > ===================================================================
> > > > --- linux-2.6.orig/scripts/setlocalversion	2021-07-12 15:29:07.000000000 +0200
> > > > +++ linux-2.6/scripts/setlocalversion	2021-07-12 15:50:29.000000000 +0200
> > > > @@ -131,7 +131,7 @@ res="${res}${CONFIG_LOCALVERSION}${LOCAL
> > > >  if test "$CONFIG_LOCALVERSION_AUTO" = "y"; then
> > > >  	# full scm version string
> > > >  	res="$res$(scm_version)"
> > > > -elif [ -z "${LOCALVERSION}" ]; then
> > > > +elif [ "${LOCALVERSION+set}" != "set" ]; then
> > > 
> > > That's really subtle, can you add a comment here that this handles an
> > > empty file?
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > OK.
> > 
> > 
> > From: Mikulas Patocka <mpatocka@redhat.com>
> > 
> 
> I can't take a patch like this :(
> 
> > The patch 042da426f8ebde012be9429ff705232af7ad7469
> > ("scripts/setlocalversion: simplify the short version part") reduces
> 
> Properly quote commits, the documentation says you do not need to use
> the full sha1.



From: Mikulas Patocka <mpatocka@redhat.com>

The commit 042da426f8eb ("scripts/setlocalversion: simplify the short
version part") reduces indentation. Unfortunately, it also changes behavior
in a subtle way - if the user has empty "LOCALVERSION" variable, the plus
sign is appended to the kernel version. It wasn't appended before.

This patch reverts to the old behavior - we append the plus sign only if
the LOCALVERSION variable is not set.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Fixes: 042da426f8eb ("scripts/setlocalversion: simplify the short version part")

---
 scripts/setlocalversion |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Index: linux-2.6/scripts/setlocalversion
===================================================================
--- linux-2.6.orig/scripts/setlocalversion	2021-07-12 15:29:07.000000000 +0200
+++ linux-2.6/scripts/setlocalversion	2021-07-12 21:00:59.000000000 +0200
@@ -131,11 +131,14 @@ res="${res}${CONFIG_LOCALVERSION}${LOCAL
 if test "$CONFIG_LOCALVERSION_AUTO" = "y"; then
 	# full scm version string
 	res="$res$(scm_version)"
-elif [ -z "${LOCALVERSION}" ]; then
-	# append a plus sign if the repository is not in a clean
-	# annotated or signed tagged state (as git describe only
-	# looks at signed or annotated tags - git tag -a/-s) and
-	# LOCALVERSION= is not specified
+elif [ "${LOCALVERSION+set}" != "set" ]; then
+	# If the variable LOCALVERSION is not set, append a plus
+	# sign if the repository is not in a clean annotated or
+	# signed tagged state (as git describe only looks at signed
+	# or annotated tags - git tag -a/-s).
+	#
+	# If the variable LOCALVERSION is set (including being set
+	# to an empty string), we don't want to append a plus sign.
 	scm=$(scm_version --short)
 	res="$res${scm:++}"
 fi

Re: [PATCH v3] scripts/setlocalversion: fix a bug when LOCALVERSION is empty

[Nico Schottelius]

Sorry to say, but I am not convinced by the patch.

While yes, we might have changed the behaviour slightly, reading
something on the line of

if [ -z ... ]

is significantly more simple, elegant and easier to maintain, than
a rather atypical special case for setting a variable to empty,
using

if [ "${LOCALVERSION+set}" != "set" ] ..

*and* because it is so atypical, adding a long comment for it, too.

Additonally, -z should be correct if the variable *is* truly empty. I
assume it actually isn't and contains whitespace, which is not the same
as being set and empty.

Instead of re-adding complexity, could you consider changing the build
flow so that LOCALVERSION is either unset or empty?

Nico

Executed in bash:

% touch empty
% a=$(cat empty)
% [ -z "$a" ]  && echo "really empty"
really empty


Mikulas Patocka <mpatocka@redhat.com> writes:

> On Mon, 12 Jul 2021, Greg Kroah-Hartman wrote:
>
>> On Mon, Jul 12, 2021 at 03:06:48PM -0400, Mikulas Patocka wrote:
>> >
>> >
>> > On Mon, 12 Jul 2021, Greg Kroah-Hartman wrote:
>> >
>> > > On Mon, Jul 12, 2021 at 10:00:59AM -0400, Mikulas Patocka wrote:
>> > > > The patch 042da426f8ebde012be9429ff705232af7ad7469
>> > > > ("scripts/setlocalversion: simplify the short version part") reduces the
>> > > > indentation. Unfortunatelly, it also changes behavior in a subtle way - if
>> > > > the user has empty "LOCALVERSION" variable, the plus sign is appended to
>> > > > the kernel version. It wasn't appended before.
>> > > >
>> > > > This patch reverts to the old behavior - we append the plus sign only if
>> > > > the LOCALVERSION variable is not set.
>> > > >
>> > > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
>> > > > Fixes: 042da426f8eb ("scripts/setlocalversion: simplify the short version part")
>> > > >
>> > > > ---
>> > > >  scripts/setlocalversion |    2 +-
>> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > > >
>> > > > Index: linux-2.6/scripts/setlocalversion
>> > > > ===================================================================
>> > > > --- linux-2.6.orig/scripts/setlocalversion	2021-07-12 15:29:07.000000000 +0200
>> > > > +++ linux-2.6/scripts/setlocalversion	2021-07-12 15:50:29.000000000 +0200
>> > > > @@ -131,7 +131,7 @@ res="${res}${CONFIG_LOCALVERSION}${LOCAL
>> > > >  if test "$CONFIG_LOCALVERSION_AUTO" = "y"; then
>> > > >  	# full scm version string
>> > > >  	res="$res$(scm_version)"
>> > > > -elif [ -z "${LOCALVERSION}" ]; then
>> > > > +elif [ "${LOCALVERSION+set}" != "set" ]; then
>> > >
>> > > That's really subtle, can you add a comment here that this handles an
>> > > empty file?
>> > >
>> > > thanks,
>> > >
>> > > greg k-h
>> >
>> > OK.
>> >
>> >
>> > From: Mikulas Patocka <mpatocka@redhat.com>
>> >
>>
>> I can't take a patch like this :(
>>
>> > The patch 042da426f8ebde012be9429ff705232af7ad7469
>> > ("scripts/setlocalversion: simplify the short version part") reduces
>>
>> Properly quote commits, the documentation says you do not need to use
>> the full sha1.
>
>
>
> From: Mikulas Patocka <mpatocka@redhat.com>
>
> The commit 042da426f8eb ("scripts/setlocalversion: simplify the short
> version part") reduces indentation. Unfortunately, it also changes behavior
> in a subtle way - if the user has empty "LOCALVERSION" variable, the plus
> sign is appended to the kernel version. It wasn't appended before.
>
> This patch reverts to the old behavior - we append the plus sign only if
> the LOCALVERSION variable is not set.
>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Fixes: 042da426f8eb ("scripts/setlocalversion: simplify the short version part")
>
> ---
>  scripts/setlocalversion |   13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> Index: linux-2.6/scripts/setlocalversion
> ===================================================================
> --- linux-2.6.orig/scripts/setlocalversion	2021-07-12 15:29:07.000000000 +0200
> +++ linux-2.6/scripts/setlocalversion	2021-07-12 21:00:59.000000000 +0200
> @@ -131,11 +131,14 @@ res="${res}${CONFIG_LOCALVERSION}${LOCAL
>  if test "$CONFIG_LOCALVERSION_AUTO" = "y"; then
>  	# full scm version string
>  	res="$res$(scm_version)"
> -elif [ -z "${LOCALVERSION}" ]; then
> -	# append a plus sign if the repository is not in a clean
> -	# annotated or signed tagged state (as git describe only
> -	# looks at signed or annotated tags - git tag -a/-s) and
> -	# LOCALVERSION= is not specified
> +elif [ "${LOCALVERSION+set}" != "set" ]; then
> +	# If the variable LOCALVERSION is not set, append a plus
> +	# sign if the repository is not in a clean annotated or
> +	# signed tagged state (as git describe only looks at signed
> +	# or annotated tags - git tag -a/-s).
> +	#
> +	# If the variable LOCALVERSION is set (including being set
> +	# to an empty string), we don't want to append a plus sign.
>  	scm=$(scm_version --short)
>  	res="$res${scm:++}"
>  fi


--
Sustainable and modern Infrastructures by ungleich.ch

Re: [PATCH v3] scripts/setlocalversion: fix a bug when LOCALVERSION is empty

[Mikulas Patocka]

On Mon, 12 Jul 2021, Nico Schottelius wrote:

> 
> Sorry to say, but I am not convinced by the patch.
> 
> While yes, we might have changed the behaviour slightly, reading
> something on the line of
> 
> if [ -z ... ]
> 
> is significantly more simple, elegant and easier to maintain, than
> a rather atypical special case for setting a variable to empty,
> using
> 
> if [ "${LOCALVERSION+set}" != "set" ] ..
> 
> *and* because it is so atypical, adding a long comment for it, too.
> 
> Additonally, -z should be correct if the variable *is* truly empty. I
> assume it actually isn't and contains whitespace, which is not the same
> as being set and empty.
> 
> Instead of re-adding complexity, could you consider changing the build
> flow so that LOCALVERSION is either unset or empty?
> 
> Nico

I set LOCALVERSION to an empty string (with "export LOCALVERSION="). This 
prevented the kernel from adding a "+" sign to the kernel version. Since 
the commit 042da426f8eb, it no longer works and the kernel adds a "+" sign 
if LOCALVERSION is set and empty.

If you don't like "if [ "${LOCALVERSION+set}" != "set" ]", then please 
provide some other way how to test if the variable is set.

Mikulas

Re: [PATCH v3] scripts/setlocalversion: fix a bug when LOCALVERSION is empty

[Nico Schottelius]

Mikulas Patocka <mpatocka@redhat.com> writes:
> I set LOCALVERSION to an empty string (with "export LOCALVERSION="). This
> prevented the kernel from adding a "+" sign to the kernel version. Since
> the commit 042da426f8eb, it no longer works and the kernel adds a "+" sign
> if LOCALVERSION is set and empty.
>
> If you don't like "if [ "${LOCALVERSION+set}" != "set" ]", then please
> provide some other way how to test if the variable is set.

I fail to see the problem you are solving, as that case works exactly
like I wrote in my last mail:

[11:09:03] nb3:~$ export LOCALVERSION=; [ -z "${LOCALVERSION}" ] && echo unset
unset
[11:09:27] nb3:~$ echo $BASH_VERSION
5.1.8(1)-release

Did you try that in your environment?


--
Sustainable and modern Infrastructures by ungleich.ch

Re: [PATCH v3] scripts/setlocalversion: fix a bug when LOCALVERSION is empty

[Mikulas Patocka]

On Tue, 13 Jul 2021, Nico Schottelius wrote:

> 
> 
> Mikulas Patocka <mpatocka@redhat.com> writes:
> > I set LOCALVERSION to an empty string (with "export LOCALVERSION="). This
> > prevented the kernel from adding a "+" sign to the kernel version. Since
> > the commit 042da426f8eb, it no longer works and the kernel adds a "+" sign
> > if LOCALVERSION is set and empty.
> >
> > If you don't like "if [ "${LOCALVERSION+set}" != "set" ]", then please
> > provide some other way how to test if the variable is set.
> 
> I fail to see the problem you are solving, as that case works exactly
> like I wrote in my last mail:

The problem is this - I have a few patches applied to the kernel and I 
don't want the plus sign in the kernel version. So, I set
"export LOCALVERSION=". In the kernel 5.13 and previous versions, there is 
no plus sign at the end of version string. With the version 5.14-rc1, 
there is a plus sign.because of the patch 042da426f8eb.

> [11:09:03] nb3:~$ export LOCALVERSION=; [ -z "${LOCALVERSION}" ] && echo unset
> unset
> [11:09:27] nb3:~$ echo $BASH_VERSION
> 5.1.8(1)-release
> 
> Did you try that in your environment?

I tried and it and it doesn't work - the plus sign is present in the 
kernel version because [ -z "${LOCALVERSION}" ] return true.

Mikulas

Re: [PATCH v3] scripts/setlocalversion: fix a bug when LOCALVERSION is empty

[Masahiro Yamada]

On Tue, Jul 13, 2021 at 5:59 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
>
>
>
> On Mon, 12 Jul 2021, Nico Schottelius wrote:
>
> >
> > Sorry to say, but I am not convinced by the patch.
> >
> > While yes, we might have changed the behaviour slightly, reading
> > something on the line of
> >
> > if [ -z ... ]
> >
> > is significantly more simple, elegant and easier to maintain, than
> > a rather atypical special case for setting a variable to empty,
> > using
> >
> > if [ "${LOCALVERSION+set}" != "set" ] ..
> >
> > *and* because it is so atypical, adding a long comment for it, too.
> >
> > Additonally, -z should be correct if the variable *is* truly empty. I
> > assume it actually isn't and contains whitespace, which is not the same
> > as being set and empty.
> >
> > Instead of re-adding complexity, could you consider changing the build
> > flow so that LOCALVERSION is either unset or empty?
> >
> > Nico
>
> I set LOCALVERSION to an empty string (with "export LOCALVERSION="). This
> prevented the kernel from adding a "+" sign to the kernel version. Since
> the commit 042da426f8eb, it no longer works and the kernel adds a "+" sign
> if LOCALVERSION is set and empty.
>
> If you don't like "if [ "${LOCALVERSION+set}" != "set" ]", then please
> provide some other way how to test if the variable is set.

if [ -z "${LOCALVERSION+set}" ]

is the alternative way I came up with, but
I am OK with your v3.


I will pick it up for -rc2.




-- 
Best Regards
Masahiro Yamada

Re: [PATCH v3] scripts/setlocalversion: fix a bug when LOCALVERSION is empty

[Masahiro Yamada]

On Wed, Jul 14, 2021 at 8:54 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Tue, Jul 13, 2021 at 5:59 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> >
> >
> >
> > On Mon, 12 Jul 2021, Nico Schottelius wrote:
> >
> > >
> > > Sorry to say, but I am not convinced by the patch.
> > >
> > > While yes, we might have changed the behaviour slightly, reading
> > > something on the line of
> > >
> > > if [ -z ... ]
> > >
> > > is significantly more simple, elegant and easier to maintain, than
> > > a rather atypical special case for setting a variable to empty,
> > > using
> > >
> > > if [ "${LOCALVERSION+set}" != "set" ] ..
> > >
> > > *and* because it is so atypical, adding a long comment for it, too.
> > >
> > > Additonally, -z should be correct if the variable *is* truly empty. I
> > > assume it actually isn't and contains whitespace, which is not the same
> > > as being set and empty.
> > >
> > > Instead of re-adding complexity, could you consider changing the build
> > > flow so that LOCALVERSION is either unset or empty?
> > >
> > > Nico
> >
> > I set LOCALVERSION to an empty string (with "export LOCALVERSION="). This
> > prevented the kernel from adding a "+" sign to the kernel version. Since
> > the commit 042da426f8eb, it no longer works and the kernel adds a "+" sign
> > if LOCALVERSION is set and empty.
> >
> > If you don't like "if [ "${LOCALVERSION+set}" != "set" ]", then please
> > provide some other way how to test if the variable is set.
>
> if [ -z "${LOCALVERSION+set}" ]
>
> is the alternative way I came up with, but
> I am OK with your v3.
>
>
> I will pick it up for -rc2.
>
>

Applied to linux-kbuild.
Thanks.




-- 
Best Regards
Masahiro Yamada