[clang-format] git-clang-format throws an assertion when removing files as part of the commit

[MyDeveloperDay]

Summary

Following a change D111273: [clang-format-diff] Fix missing formatting for zero length git diff lines to allow git-clang-format to see single lines being removed, we introduced a regression such that if you are removing a whole file it will assert in clang-format as its given the -lines=0:0 (lines are 1 based)

[lhames]

I think that this should fix the issue I saw, but clang-format will still crash when passed a -lines=0:X option. Would it make sense to either error out in that tool, or teach the tool to ignore -lines=0:0 options if they're guaranteed to be no-ops for formatting?

[MyDeveloperDay]

In D112056#3073142, @lhames wrote:

I think that this should fix the issue I saw, but clang-format will still crash when passed a -lines=0:X option. Would it make sense to either error out in that tool, or teach the tool to ignore -lines=0:0 options if they're guaranteed to be no-ops for formatting?

I think my concern with simply ignoring them is that clang-format could be given multiple -lines calls, what should it do? silently ignore the -lines:0:0 or error out on all the other changes, I'd be a little concerned because we then enter into other issues like what happens if someone give -lines:-1:3 should it error out or adjust itself to -lines:1:2

ultimately I think silently ignoring could be more dangerous rather than asserting or trying to correct it, this way it ensures that people fix up how they are calling it (as in this case), having said that I'm not a massive fan of asserting either.

I think maybe we should fix this as a regression and take sometime to think if we want a more guarded solution separately.

[HazardyKnusperkeks]

In D112056#3073569, @MyDeveloperDay wrote:

In D112056#3073142, @lhames wrote:

I think that this should fix the issue I saw, but clang-format will still crash when passed a -lines=0:X option. Would it make sense to either error out in that tool, or teach the tool to ignore -lines=0:0 options if they're guaranteed to be no-ops for formatting?

I think my concern with simply ignoring them is that clang-format could be given multiple -lines calls, what should it do? silently ignore the -lines:0:0 or error out on all the other changes, I'd be a little concerned because we then enter into other issues like what happens if someone give -lines:-1:3 should it error out or adjust itself to -lines:1:2

ultimately I think silently ignoring could be more dangerous rather than asserting or trying to correct it, this way it ensures that people fix up how they are calling it (as in this case), having said that I'm not a massive fan of asserting either.

I think maybe we should fix this as a regression and take sometime to think if we want a more guarded solution separately.

I would be in favor of returning an actual error instead of an assertion (what happens in release mode?). It is common to many programs to just print the error message if one argument was wrong and do not act on the others.

clang/tools/clang-format/git-clang-format

for line in patch_file:
     match = re.search(r'^@@ -[0-9,]+ \+(\d+)(,(\d+))?', line)
     if match:
       start_line = int(match.group(1))
       line_count = 1
       if match.group(3):
         line_count = int(match.group(3))
       if line_count == 0:
         line_count = 1
+      if start_line == 0:
+        continue
       matches.setdefault(filename, []).append(Range(start_line, line_count))
   return matches
 
 
 def filter_by_extension(dictionary, allowed_extensions):
   """Delete every key in `dictionary` that doesn't have an allowed extension.
 
   `allowed_extensions` must be a collection of lowercase file extensions,