[PATCH] moveconfig: Fix code relying on now-stripped newline characters

[PATCH] moveconfig: Fix code relying on now-stripped newline characters

[Alper Nebi Yasak]

Commit 37f815cad07d ("moveconfig: Use a function to read files") adds a
helper function that can read a file as lines, but strips the newline
characters. This change broke parts of moveconfig code that relied on
their existence, resulting in a few issues:

Configs that are defined as empty aren't removed from header files (e.g.
"#define CONFIG_REMAKE_ELF"). Make regex patterns use '\b' to match word
boundaries instead of '\W' (which matched the newlines) so these lines
still match and get removed.

All changes in defconfig are considered removed by savedefconfig even
if they weren't, and line continuations in the headers aren't recognized
and removed properly, because their checks explicitly look for a newline
character. Remove the character from both comparisons.

The printed diff of header files is wrongly formatted and raises an
IndexError if a blank line was removed. Let print() print the new lines,
and use size-independent ways to check strings to fix the diff output.

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---

 tools/moveconfig.py | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/tools/moveconfig.py b/tools/moveconfig.py
index 35fe6710d70a..1bcf58caf14c 100755
--- a/tools/moveconfig.py
+++ b/tools/moveconfig.py
@@ -205,12 +205,12 @@ def show_diff(alines, blines, file_path, color_enabled):
                                 tofile=os.path.join('b', file_path))
 
     for line in diff:
-        if line[0] == '-' and line[1] != '-':
-            print(color_text(color_enabled, COLOR_RED, line), end=' ')
-        elif line[0] == '+' and line[1] != '+':
-            print(color_text(color_enabled, COLOR_GREEN, line), end=' ')
+        if line.startswith('-') and not line.startswith('--'):
+            print(color_text(color_enabled, COLOR_RED, line))
+        elif line.startswith('+') and not line.startswith('++'):
+            print(color_text(color_enabled, COLOR_GREEN, line))
         else:
-            print(line, end=' ')
+            print(line)
 
 def extend_matched_lines(lines, matched, pre_patterns, post_patterns,
                          extend_pre, extend_post):
@@ -368,7 +368,7 @@ def cleanup_one_header(header_path, patterns, args):
 
     matched = []
     for i, line in enumerate(lines):
-        if i - 1 in matched and lines[i - 1][-2:] == '\\\n':
+        if i - 1 in matched and lines[i - 1].endswith('\\'):
             matched.append(i)
             continue
         for pattern in patterns:
@@ -380,9 +380,9 @@ def cleanup_one_header(header_path, patterns, args):
         return
 
     # remove empty #ifdef ... #endif, successive blank lines
-    pattern_if = re.compile(r'#\s*if(def|ndef)?\W') #  #if, #ifdef, #ifndef
-    pattern_elif = re.compile(r'#\s*el(if|se)\W')   #  #elif, #else
-    pattern_endif = re.compile(r'#\s*endif\W')      #  #endif
+    pattern_if = re.compile(r'#\s*if(def|ndef)?\b') #  #if, #ifdef, #ifndef
+    pattern_elif = re.compile(r'#\s*el(if|se)\b')   #  #elif, #else
+    pattern_endif = re.compile(r'#\s*endif\b')      #  #endif
     pattern_blank = re.compile(r'^\s*$')            #  empty line
 
     while True:
@@ -424,8 +424,8 @@ def cleanup_headers(configs, args):
 
     patterns = []
     for config in configs:
-        patterns.append(re.compile(r'#\s*define\s+%s\W' % config))
-        patterns.append(re.compile(r'#\s*undef\s+%s\W' % config))
+        patterns.append(re.compile(r'#\s*define\s+%s\b' % config))
+        patterns.append(re.compile(r'#\s*undef\s+%s\b' % config))
 
     for dir in 'include', 'arch', 'board':
         for (dirpath, dirnames, filenames) in os.walk(dir):
@@ -451,7 +451,7 @@ def cleanup_one_extra_option(defconfig_path, configs, args):
     """
 
     start = 'CONFIG_SYS_EXTRA_OPTIONS="'
-    end = '"\n'
+    end = '"'
 
     lines = read_file(defconfig_path)
 
@@ -812,7 +812,7 @@ def check_defconfig(self):
         for (action, value) in self.results:
             if action != ACTION_MOVE:
                 continue
-            if not value + '\n' in defconfig_lines:
+            if not value in defconfig_lines:
                 log += color_text(self.args.color, COLOR_YELLOW,
                                   "'%s' was removed by savedefconfig.\n" %
                                   value)
-- 
2.34.1

Re: [PATCH] moveconfig: Fix code relying on now-stripped newline characters

[Simon Glass]

Hi Alper,

On Sat, 29 Jan 2022 at 08:22, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> Commit 37f815cad07d ("moveconfig: Use a function to read files") adds a
> helper function that can read a file as lines, but strips the newline
> characters. This change broke parts of moveconfig code that relied on
> their existence, resulting in a few issues:
>
> Configs that are defined as empty aren't removed from header files (e.g.
> "#define CONFIG_REMAKE_ELF"). Make regex patterns use '\b' to match word
> boundaries instead of '\W' (which matched the newlines) so these lines
> still match and get removed.
>
> All changes in defconfig are considered removed by savedefconfig even
> if they weren't, and line continuations in the headers aren't recognized
> and removed properly, because their checks explicitly look for a newline
> character. Remove the character from both comparisons.
>
> The printed diff of header files is wrongly formatted and raises an
> IndexError if a blank line was removed. Let print() print the new lines,
> and use size-independent ways to check strings to fix the diff output.
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> ---
>
>  tools/moveconfig.py | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)

I was a little worried about the subtleties here. Thanks for fixing
it! I almost got annoyed and wrote some tests, but I guess we can limp
on without them.

>
> diff --git a/tools/moveconfig.py b/tools/moveconfig.py
> index 35fe6710d70a..1bcf58caf14c 100755
> --- a/tools/moveconfig.py
> +++ b/tools/moveconfig.py
> @@ -205,12 +205,12 @@ def show_diff(alines, blines, file_path, color_enabled):
>                                  tofile=os.path.join('b', file_path))
>
>      for line in diff:
> -        if line[0] == '-' and line[1] != '-':
> -            print(color_text(color_enabled, COLOR_RED, line), end=' ')
> -        elif line[0] == '+' and line[1] != '+':
> -            print(color_text(color_enabled, COLOR_GREEN, line), end=' ')
> +        if line.startswith('-') and not line.startswith('--'):
> +            print(color_text(color_enabled, COLOR_RED, line))
> +        elif line.startswith('+') and not line.startswith('++'):
> +            print(color_text(color_enabled, COLOR_GREEN, line))
>          else:
> -            print(line, end=' ')
> +            print(line)
>
>  def extend_matched_lines(lines, matched, pre_patterns, post_patterns,
>                           extend_pre, extend_post):
> @@ -368,7 +368,7 @@ def cleanup_one_header(header_path, patterns, args):
>
>      matched = []
>      for i, line in enumerate(lines):
> -        if i - 1 in matched and lines[i - 1][-2:] == '\\\n':
> +        if i - 1 in matched and lines[i - 1].endswith('\\'):
>              matched.append(i)
>              continue
>          for pattern in patterns:
> @@ -380,9 +380,9 @@ def cleanup_one_header(header_path, patterns, args):
>          return
>
>      # remove empty #ifdef ... #endif, successive blank lines
> -    pattern_if = re.compile(r'#\s*if(def|ndef)?\W') #  #if, #ifdef, #ifndef
> -    pattern_elif = re.compile(r'#\s*el(if|se)\W')   #  #elif, #else
> -    pattern_endif = re.compile(r'#\s*endif\W')      #  #endif
> +    pattern_if = re.compile(r'#\s*if(def|ndef)?\b') #  #if, #ifdef, #ifndef
> +    pattern_elif = re.compile(r'#\s*el(if|se)\b')   #  #elif, #else
> +    pattern_endif = re.compile(r'#\s*endif\b')      #  #endif
>      pattern_blank = re.compile(r'^\s*$')            #  empty line
>
>      while True:
> @@ -424,8 +424,8 @@ def cleanup_headers(configs, args):
>
>      patterns = []
>      for config in configs:
> -        patterns.append(re.compile(r'#\s*define\s+%s\W' % config))
> -        patterns.append(re.compile(r'#\s*undef\s+%s\W' % config))
> +        patterns.append(re.compile(r'#\s*define\s+%s\b' % config))
> +        patterns.append(re.compile(r'#\s*undef\s+%s\b' % config))
>
>      for dir in 'include', 'arch', 'board':
>          for (dirpath, dirnames, filenames) in os.walk(dir):
> @@ -451,7 +451,7 @@ def cleanup_one_extra_option(defconfig_path, configs, args):
>      """
>
>      start = 'CONFIG_SYS_EXTRA_OPTIONS="'
> -    end = '"\n'
> +    end = '"'

''

>
>      lines = read_file(defconfig_path)
>
> @@ -812,7 +812,7 @@ def check_defconfig(self):
>          for (action, value) in self.results:
>              if action != ACTION_MOVE:
>                  continue
> -            if not value + '\n' in defconfig_lines:
> +            if not value in defconfig_lines:
>                  log += color_text(self.args.color, COLOR_YELLOW,
>                                    "'%s' was removed by savedefconfig.\n" %
>                                    value)
> --
> 2.34.1
>

Regards,
Simon

Re: [PATCH] moveconfig: Fix code relying on now-stripped newline characters

[Alper Nebi Yasak]

On 30/01/2022 00:09, Simon Glass wrote:
> On Sat, 29 Jan 2022 at 08:22, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>> Commit 37f815cad07d ("moveconfig: Use a function to read files") adds a
>> helper function that can read a file as lines, but strips the newline
>> characters. This change broke parts of moveconfig code that relied on
>> their existence, resulting in a few issues
> 
> I was a little worried about the subtleties here. Thanks for fixing
> it! I almost got annoyed and wrote some tests, but I guess we can limp
> on without them.

Can't guarantee I found everything, just lucky I was trying a migration
on REMAKE_ELF :)

>>
>> diff --git a/tools/moveconfig.py b/tools/moveconfig.py
>> index 35fe6710d70a..1bcf58caf14c 100755
>> --- a/tools/moveconfig.py
>> +++ b/tools/moveconfig.py
>> @@ -451,7 +451,7 @@ def cleanup_one_extra_option(defconfig_path, configs, args):
>>      """
>>
>>      start = 'CONFIG_SYS_EXTRA_OPTIONS="'
>> -    end = '"\n'
>> +    end = '"'
> 
> ''

Looks to me like the double-quote character is necessary here, if I add
CONFIG_SYS_EXTRA_OPTIONS="REMAKE_ELF" to chromebook_bob_defconfig and
try migrating CONFIG_REMAKE_ELF the line doesn't get removed otherwise.

Trying to add more lines of diff context below:

>>
>>      lines = read_file(defconfig_path)
>>
        for i, line in enumerate(lines):
            if line.startswith(start) and line.endswith(end):
                break
        else:
            # CONFIG_SYS_EXTRA_OPTIONS was not found in this defconfig
            return

        old_tokens = line[len(start):-len(end)].split(',')
        new_tokens = []

        for token in old_tokens:
            pos = token.find('=')
            if not (token[:pos] if pos >= 0 else token) in configs:
                new_tokens.append(token)

Re: [PATCH] moveconfig: Fix code relying on now-stripped newline characters

[Simon Glass]

Hi Alper,

On Sun, 30 Jan 2022 at 13:14, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 30/01/2022 00:09, Simon Glass wrote:
> > On Sat, 29 Jan 2022 at 08:22, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> >> Commit 37f815cad07d ("moveconfig: Use a function to read files") adds a
> >> helper function that can read a file as lines, but strips the newline
> >> characters. This change broke parts of moveconfig code that relied on
> >> their existence, resulting in a few issues
> >
> > I was a little worried about the subtleties here. Thanks for fixing
> > it! I almost got annoyed and wrote some tests, but I guess we can limp
> > on without them.
>
> Can't guarantee I found everything, just lucky I was trying a migration
> on REMAKE_ELF :)

Reviewed-by: Simon Glass <sjg@chromium.org>
Tested-by: Simon Glass <sjg@chromium.org>

>
> >>
> >> diff --git a/tools/moveconfig.py b/tools/moveconfig.py
> >> index 35fe6710d70a..1bcf58caf14c 100755
> >> --- a/tools/moveconfig.py
> >> +++ b/tools/moveconfig.py
> >> @@ -451,7 +451,7 @@ def cleanup_one_extra_option(defconfig_path, configs, args):
> >>      """
> >>
> >>      start = 'CONFIG_SYS_EXTRA_OPTIONS="'
> >> -    end = '"\n'
> >> +    end = '"'
> >
> > ''
>
> Looks to me like the double-quote character is necessary here, if I add
> CONFIG_SYS_EXTRA_OPTIONS="REMAKE_ELF" to chromebook_bob_defconfig and
> try migrating CONFIG_REMAKE_ELF the line doesn't get removed otherwise.

Yes you are right, ignore this. It is barely legible in the font I am
using here.

>
> Trying to add more lines of diff context below:
>
> >>
> >>      lines = read_file(defconfig_path)
> >>
>         for i, line in enumerate(lines):
>             if line.startswith(start) and line.endswith(end):
>                 break
>         else:
>             # CONFIG_SYS_EXTRA_OPTIONS was not found in this defconfig
>             return
>
>         old_tokens = line[len(start):-len(end)].split(',')
>         new_tokens = []
>
>         for token in old_tokens:
>             pos = token.find('=')
>             if not (token[:pos] if pos >= 0 else token) in configs:
>                 new_tokens.append(token)

Regards,
Simon