[clang-format] Add null-terminated path option (#123921)

[createyourpersonalaccount]

This makes the git clang-format tool compose nicely with other shell utilities in case of maliciously (or innocently) crafted filenames.

Closes #123921.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang-format

Author: Nikolaos Chatzikonstantinou (createyourpersonalaccount)

Changes

This makes the git clang-format tool compose nicely with other shell utilities in case of maliciously (or innocently) crafted filenames.

---

Full diff: https://github.com/llvm/llvm-project/pull/123926.diff

1 Files Affected:

• (modified) clang/tools/clang-format/git-clang-format (+16-3)

diff --git a/clang/tools/clang-format/git-clang-format b/clang/tools/clang-format/git-clang-format
index da271bbe6e3a07..04c49e8910d0ac 100755
--- a/clang/tools/clang-format/git-clang-format
+++ b/clang/tools/clang-format/git-clang-format
@@ -205,6 +205,12 @@ def main():
             "commits"
         ),
     )
+    p.add_argument(
+        "-0",
+        "--null",
+        action="store_true",
+        help="print the affected paths with null-terminated characters",
+    )
     # We gather all the remaining positional arguments into 'args' since we need
     # to use some heuristics to determine whether or not <commit> was present.
     # However, to print pretty messages, we make use of metavar and help.
@@ -261,11 +267,11 @@ def main():
                 "ignored by clang-format):"
             )
             for filename in ignored_files:
-                print("    %s" % filename)
+                print_filename(filename, opts.null)
         if changed_lines:
             print("Running clang-format on the following files:")
             for filename in changed_lines:
-                print("    %s" % filename)
+                print_filename(filename, opts.null)
 
     if not changed_lines:
         if opts.verbose >= 0:
@@ -304,7 +310,7 @@ def main():
     if (opts.verbose >= 0 and not opts.patch) or opts.verbose >= 1:
         print("changed files:")
         for filename in changed_files:
-            print("    %s" % filename)
+            print_filename(filename, opts.null)
 
     return 1
 
@@ -869,5 +875,12 @@ def convert_string(bytes_input):
         return str(bytes_input)
 
 
+def print_filename(filename, null=False):
+    if null:
+        print(filename + "\0", end="")
+    else:
+        print("    %s" % filename)
+
+
 if __name__ == "__main__":
     sys.exit(main())

[mydeveloperday]

This seems reasonable to me, but wait for the others, maybe @owenca

[createyourpersonalaccount]

This seems reasonable to me, but wait for the others, maybe @owenca

I'm wondering if -0 should turn off all informational messaging as well and only print the paths. As it is, it will print things like Running clang-format on the following files:, even if a null-separated list of files follows.

So right now the usage would be (example)

git clang-format --staged -0 | tail -n +2 | xargs -0 git add

but perhaps it should not print informational messages and have the usage be simplified:

git clang-format --staged -0 | xargs -0 git add

[HazardyKnusperkeks]

This seems reasonable to me, but wait for the others, maybe @owenca

I'm wondering if -0 should turn off all informational messaging as well and only print the paths. As it is, it will print things like Running clang-format on the following files:, even if a null-separated list of files follows.

So right now the usage would be (example)

git clang-format --staged -0 | tail -n +2 | xargs -0 git add

but perhaps it should not print informational messages and have the usage be simplified:

git clang-format --staged -0 | xargs -0 git add

With that I can see why you don't indent. But you changed only the format of the file name output.

[createyourpersonalaccount]

This seems reasonable to me, but wait for the others, maybe @owenca

I'm wondering if -0 should turn off all informational messaging as well and only print the paths. As it is, it will print things like Running clang-format on the following files:, even if a null-separated list of files follows.
So right now the usage would be (example)

git clang-format --staged -0 | tail -n +2 | xargs -0 git add

but perhaps it should not print informational messages and have the usage be simplified:

git clang-format --staged -0 | xargs -0 git add

With that I can see why you don't indent. But you changed only the format of the file name output.

What else should change?

[mydeveloperday]

This seems reasonable to me, but wait for the others, maybe @owenca

I'm wondering if -0 should turn off all informational messaging as well and only print the paths. As it is, it will print things like Running clang-format on the following files:, even if a null-separated list of files follows.

So right now the usage would be (example)

git clang-format --staged -0 | tail -n +2 | xargs -0 git add

but perhaps it should not print informational messages and have the usage be simplified:

git clang-format --staged -0 | xargs -0 git add

Aren't you really asking, just give me a list of all the files that have been changed by clang format, in which case -0 isn't really that descriptive, what not

--modified  or -m (similar to git ls-files)

or
-- affected (might avoid becausee of affected/effected spelling issues people have)

[mydeveloperday]

on reflection, I think better named arguments might be better

[owenca]

From #123921, it seems that you only want the new option to work with --staged, but should it also work with other options that may print a list of filenames?

[createyourpersonalaccount]

From #123921, it seems that you only want the new option to work with --staged, but should it also work with other options that may print a list of filenames?

I don't know; I only use git clang-format as in #123921 and I am not familiar with all its uses. I was hoping that you or someone other who is familiar could determine this.

What worries me is conditions where more than one list of files is printed, e.g. unchanged + changed files. It's difficult to parse these messages from the shell; when a null-separated list is printed from other command line utilities, it has one meaning, e.g. "here's the list of files you asked for", whereas git clang-format prints other diagnostics too.

If someone is familiar with the usages of this tool maybe they can tell me what is the logic that must be followed in terms of which list is the important one according to the options passed, so that only that list is printed with --print0.

[createyourpersonalaccount]

@owenca @mydeveloperday @HazardyKnusperkeks I'll ping now that it's the weekend, perhaps you have some time to look at this more; in particular now I got the --null option to work for --staged, but I'm not sure how you'd like it to work for other options. I've explained my concerns in #123926 (comment)

[HazardyKnusperkeks]

Is verbose and null a combination that should be supported?

[createyourpersonalaccount]

You have a good point, should they be mutually exclusive? I.e. if both are specified an error message is printed. I don't think --verbose is meaningful because --null does not have extra information to print. I will try to write a patch for that.

[owenca]

From #123921, it seems that you only want the new option to work with --staged, but should it also work with other options that may print a list of filenames?

I don't know; I only use git clang-format as in #123921 and I am not familiar with all its uses. I was hoping that you or someone other who is familiar could determine this.

What worries me is conditions where more than one list of files is printed, e.g. unchanged + changed files. It's difficult to parse these messages from the shell; when a null-separated list is printed from other command line utilities, it has one meaning, e.g. "here's the list of files you asked for", whereas git clang-format prints other diagnostics too.

If someone is familiar with the usages of this tool maybe they can tell me what is the logic that must be followed in terms of which list is the important one according to the options passed, so that only that list is printed with --print0.

It seems that git-clang-format was not designed for its output to be consumed by another utility as it prints everything to stdout instead of stderr. Adding an option like--print0 (especially just for --staged) looks odd to me.

[createyourpersonalaccount]

From #123921, it seems that you only want the new option to work with --staged, but should it also work with other options that may print a list of filenames?

I don't know; I only use git clang-format as in #123921 and I am not familiar with all its uses. I was hoping that you or someone other who is familiar could determine this.
What worries me is conditions where more than one list of files is printed, e.g. unchanged + changed files. It's difficult to parse these messages from the shell; when a null-separated list is printed from other command line utilities, it has one meaning, e.g. "here's the list of files you asked for", whereas git clang-format prints other diagnostics too.
If someone is familiar with the usages of this tool maybe they can tell me what is the logic that must be followed in terms of which list is the important one according to the options passed, so that only that list is printed with --print0.

It seems that git-clang-format was not designed for its output to be consumed by another utility as it prints everything to stdout instead of stderr. Adding an option like--print0 (especially just for --staged) looks odd to me.

No, the approach of printing to stderr for utilities is not accurate. I've heard of this before, even the claim that this is the original UNIX way, but it's not. I don't know the origin of this cargo-culting factoid but I assure you that it does not hold true. (The origin I suspect may be Microsoft with its Powershell, i.e. see about_Output_Streams) There is a lot of value in --null since it allows git clang-format to be used in git hooks. Why would a Python script designed as a git subcommand not work well with git?