[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.

[createyourpersonalaccount]

@HazardyKnusperkeks Do you have an input on this? Right now it's just ignoring --verbose.

[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?

[createyourpersonalaccount]

@owenca ping

[owenca]

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)

Diagnostic output should go to stderr, but the informational output of git-clang-format is sent to stdout instead.

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?

If you want to run git add in git-clang-format, we can add an option for that.

[createyourpersonalaccount]

Diagnostic output should go to stderr, but the informational output of git-clang-format is sent to stdout instead.

What does that mean? The printing of null-terminated paths on stdout is pretty standard on unix utilities. What's "diagnostic" and "informational"?

The --null option is so that the paths can be safely handled by other tools in the shell pipeline.

If you want to run git add in git-clang-format, we can add an option for that.

No, that's limiting to the user. The right thing to do is to print the paths with null-terminated bytes in between, so that any utility wished can be executed.

Are you perhaps not aware of the issues at play here? Paths on linux/bsd/etc can contain any character. It's impossible to parse text output containing paths safely. You have to use some kind of format, and the approach used by all utilities is null-separated paths.

[owenca]

Diagnostic output should go to stderr, but the informational output of git-clang-format is sent to stdout instead.

What does that mean? The printing of null-terminated paths on stdout is pretty standard on unix utilities. What's "diagnostic" and "informational"?

Diagnostic messages are error/warning/informational messages.

The --null option is so that the paths can be safely handled by other tools in the shell pipeline.

If you want to run git add in git-clang-format, we can add an option for that.

No, that's limiting to the user. The right thing to do is to print the paths with null-terminated bytes in between, so that any utility wished can be executed.

As you already admitted in #123926 (comment) that you would only use this on git clang-format --staged with git add and didn't know about other use cases, how can you be sure it's the "right thing to do"?

Are you perhaps not aware of the issues at play here? Paths on linux/bsd/etc can contain any character. It's impossible to parse text output containing paths safely. You have to use some kind of format, and the approach used by all utilities is null-separated paths.

I used find -print0 ... | xargs -0 myself before. That's why I requested that you rename -0, --null to -print0.

I don't know what @mydeveloperday and @HazardyKnusperkeks think, but I wouldn't be in support of your proposed option for the reasons mentioned earlier.