[emacs][clang-format] Add elisp API for clang-format on git diffs #112792
Conversation
|
@llvm/pr-subscribers-clang-format Author: None (goldsteinn) ChangesNew proposed function It is the same as calling The motivation is many project (LLVM included) both have code that is Full diff: https://github.com/llvm/llvm-project/pull/112792.diff 1 Files Affected:
diff --git a/clang/tools/clang-format/clang-format.el b/clang/tools/clang-format/clang-format.el
index f3da5415f8672b..dfdef2260de06e 100644
--- a/clang/tools/clang-format/clang-format.el
+++ b/clang/tools/clang-format/clang-format.el
@@ -132,18 +132,97 @@ is a zero-based file offset, assuming ‘utf-8-unix’ coding."
(lambda (byte &optional _quality _coding-system)
(byte-to-position (1+ byte)))))
-;;;###autoload
-(defun clang-format-region (start end &optional style assume-file-name)
- "Use clang-format to format the code between START and END according to STYLE.
-If called interactively uses the region or the current statement if there is no
-no active region. If no STYLE is given uses `clang-format-style'. Use
-ASSUME-FILE-NAME to locate a style config file, if no ASSUME-FILE-NAME is given
-uses the function `buffer-file-name'."
- (interactive
- (if (use-region-p)
- (list (region-beginning) (region-end))
- (list (point) (point))))
+(defun clang-format--git-diffs-get-diff-lines (file-orig file-new)
+ "Return all line regions that contain diffs between FILE-ORIG and
+FILE-NEW. If there is no diff 'nil' is returned. Otherwise the
+return is a 'list' of lines in the format '--lines=<start>:<end>'
+which can be passed directly to 'clang-format'"
+ ;; Temporary buffer for output of diff.
+ (with-temp-buffer
+ (let ((status (call-process
+ "diff"
+ nil
+ (current-buffer)
+ nil
+ ;; Binary diff has different behaviors that we
+ ;; aren't interested in.
+ "-a"
+ ;; Printout changes as only the line groups.
+ "--changed-group-format=--lines=%dF:%dL "
+ ;; Ignore unchanged content.
+ "--unchanged-group-format="
+ file-orig
+ file-new
+ )
+ )
+ (stderr (concat (if (zerop (buffer-size)) "" ": ")
+ (buffer-substring-no-properties
+ (point-min) (line-end-position)))))
+ (when (stringp status)
+ (error "(diff killed by signal %s%s)" status stderr))
+ (unless (= status 0)
+ (unless (= status 1)
+ (error "(diff returned unsuccessfully %s%s)" status stderr)))
+
+
+ (if (= status 0)
+ ;; Status == 0 -> no Diff.
+ nil
+ (progn
+ ;; Split "--lines=<S0>:<E0>... --lines=<SN>:<SN>" output to
+ ;; a list for return.
+ (s-split
+ " "
+ (string-trim
+ (buffer-substring-no-properties
+ (point-min) (point-max)))))))))
+
+(defun clang-format--git-diffs-get-git-head-file ()
+ "Returns a temporary file with the content of 'buffer-file-name' at
+git revision HEAD. If the current buffer is either not a file or not
+in a git repo, this results in an error"
+ ;; Needs current buffer to be a file
+ (unless (buffer-file-name)
+ (error "Buffer is not visiting a file"))
+ ;; Need to be able to find version control (git) root
+ (unless (vc-root-dir)
+ (error "File not known to git"))
+ ;; Need version control to in fact be git
+ (unless (string-equal (vc-backend (buffer-file-name)) "Git")
+ (error "Not using git"))
+
+ (let ((tmpfile-git-head (make-temp-file "clang-format-tmp-git-head-content")))
+ ;; Get filename relative to git root
+ (let ((git-file-name (substring
+ (expand-file-name (buffer-file-name))
+ (string-width (expand-file-name (vc-root-dir)))
+ nil)))
+ (let ((status (call-process
+ "git"
+ nil
+ `(:file, tmpfile-git-head)
+ nil
+ "show" (concat "HEAD:" git-file-name)))
+ (stderr (with-temp-buffer
+ (unless (zerop (cadr (insert-file-contents tmpfile-git-head)))
+ (insert ": "))
+ (buffer-substring-no-properties
+ (point-min) (line-end-position)))))
+ (when (stringp status)
+ (error "(git show HEAD:%s killed by signal %s%s)"
+ git-file-name status stderr))
+ (unless (zerop status)
+ (error "(git show HEAD:%s returned unsuccessfully %s%s)"
+ git-file-name status stderr))))
+ ;; Return temporary file so we can diff it.
+ tmpfile-git-head))
+(defun clang-format--region-impl (start end &optional style assume-file-name lines)
+ "Common implementation for 'clang-format-buffer',
+'clang-format-region', and 'clang-format-git-diffs'. START and END
+refer to the region to be formatter. STYLE and ASSUME-FILE-NAME are
+used for configuring the clang-format. And LINES is used to pass
+specific locations for reformatting (i.e diff locations)."
(unless style
(setq style clang-format-style))
@@ -176,8 +255,12 @@ uses the function `buffer-file-name'."
(list "--assume-filename" assume-file-name))
,@(and style (list "--style" style))
"--fallback-style" ,clang-format-fallback-style
- "--offset" ,(number-to-string file-start)
- "--length" ,(number-to-string (- file-end file-start))
+ ,@(and lines lines)
+ ,@(and (not lines)
+ (list
+ "--offset" (number-to-string file-start)
+ "--length" (number-to-string
+ (- file-end file-start))))
"--cursor" ,(number-to-string cursor))))
(stderr (with-temp-buffer
(unless (zerop (cadr (insert-file-contents temp-file)))
@@ -205,6 +288,48 @@ uses the function `buffer-file-name'."
(delete-file temp-file)
(when (buffer-name temp-buffer) (kill-buffer temp-buffer)))))
+;;;###autoload
+(defun clang-format-git-diffs (&optional style assume-file-name)
+ "The same as 'clang-format-buffer' but only operates on the git
+diffs from HEAD in the buffer. If no STYLE is given uses
+`clang-format-style'. Use ASSUME-FILE-NAME to locate a style config
+file. If no ASSUME-FILE-NAME is given uses the function
+`buffer-file-name'."
+ (interactive)
+ (let ((tmpfile-git-head
+ (clang-format--git-diffs-get-git-head-file))
+ (tmpfile-curbuf (make-temp-file "clang-format-git-tmp")))
+ ;; Move current buffer to a temporary file to take a diff. Even if
+ ;; current-buffer is backed by a file, we want to diff the buffer
+ ;; contents which might not be saved.
+ (write-region nil nil tmpfile-curbuf nil 'nomessage)
+ ;; Git list of lines with a diff.
+ (let ((diff-lines
+ (clang-format--git-diffs-get-diff-lines
+ tmpfile-git-head tmpfile-curbuf)))
+ ;; If we have any diffs, format them.
+ (when diff-lines
+ (clang-format--region-impl
+ (point-min)
+ (point-max)
+ style
+ assume-file-name
+ diff-lines)))))
+
+;;;###autoload
+(defun clang-format-region (start end &optional style assume-file-name)
+ "Use clang-format to format the code between START and END according
+to STYLE. If called interactively uses the region or the current
+statement if there is no no active region. If no STYLE is given uses
+`clang-format-style'. Use ASSUME-FILE-NAME to locate a style config
+file, if no ASSUME-FILE-NAME is given uses the function
+`buffer-file-name'."
+ (interactive
+ (if (use-region-p)
+ (list (region-beginning) (region-end))
+ (list (point) (point))))
+ (clang-format--region-impl start end style assume-file-name))
+
;;;###autoload
(defun clang-format-buffer (&optional style assume-file-name)
"Use clang-format to format the current buffer according to STYLE.
@@ -212,7 +337,11 @@ If no STYLE is given uses `clang-format-style'. Use ASSUME-FILE-NAME
to locate a style config file. If no ASSUME-FILE-NAME is given uses
the function `buffer-file-name'."
(interactive)
- (clang-format-region (point-min) (point-max) style assume-file-name))
+ (clang-format--region-impl
+ (point-min)
+ (point-max)
+ style
+ assume-file-name))
;;;###autoload
(defalias 'clang-format 'clang-format-region)
|
|
ping |
|
ping2 |
|
@ideasman42 can you review this PR? Thanks! |
|
@luke957 as well also can probably review the elisp code for quality. |
There was a problem hiding this comment.
Note that I'm not a regular clang contributor, I just submitted a small improvement to clang-format.el, and maintain some emacs packages in elpa & melpa.
Overall the PR looks like it needs more attention to detail, as far as I can tell it's creating temporary files and never removing them, various minor issues noted inline.
- This PR needs to be rebased on top of the recently added
clang-format-on-save-modecommit. - Running
package-lintreports.
156:19: warning: Closing parens should not be wrapped onto new lines.
157:18: warning: Closing parens should not be wrapped onto new lines.
176:12: error: You should depend on (emacs "24.4") or the compat package if you need `string-trim'.
188:11: error: You should depend on (emacs "25.1") if you need `vc-root-dir'.
198:59: error: You should depend on (emacs "25.1") if you need `vc-root-dir'.
| (stderr (concat (if (zerop (buffer-size)) "" ": ") | ||
| (buffer-substring-no-properties | ||
| (point-min) (line-end-position))))) | ||
| (when (stringp status) |
There was a problem hiding this comment.
*picky* I think a (cond ...) would read better than unless & if statements.
example:
(cond
((stringp status)
...snip...)
((= status 0)
nil)
((= status 1)
...snip...)
(t
(error "...snip...")))| (progn | ||
| ;; Split "--lines=<S0>:<E0>... --lines=<SN>:<SN>" output to | ||
| ;; a list for return. | ||
| (s-split |
There was a problem hiding this comment.
s-split is not part of emacs standard library, depending on https://github.com/magnars/s.el doesn't seem worthwhile, use built-in emacs functionality.
| (unless (buffer-file-name) | ||
| (error "Buffer is not visiting a file")) | ||
| ;; Need to be able to find version control (git) root | ||
| (unless (vc-root-dir) |
There was a problem hiding this comment.
*picky* let-bind to a variable and reuse it instead of calling twice.
| ;; Ignore unchanged content. | ||
| "--unchanged-group-format=" | ||
| file-orig | ||
| file-new |
There was a problem hiding this comment.
*picky* don't use dangling parenthesis.
| (unless (string-equal (vc-backend (buffer-file-name)) "Git") | ||
| (error "Not using git")) | ||
|
|
||
| (let ((tmpfile-git-head (make-temp-file "clang-format-tmp-git-head-content"))) |
There was a problem hiding this comment.
From what I can tell this temporary file is never deleted, further, any errors after creating the temporary file would leave it created.
Without attempting to do this myself, I'm not sure of the best solution but suggest: (with-temp-file ...)
The caller would need to use this and pass in the temporary file. If the behavior of with-temp-file isn't what your after, you could write your own macro that creates a scoped temporary file that gets removed in case of errors.
There was a problem hiding this comment.
with-temp-file doesn't quite work here, V2 I just wrap the thing in an unwind-protect and delete the temp files in it.
| (interactive) | ||
| (let ((tmpfile-git-head | ||
| (clang-format--git-diffs-get-git-head-file)) | ||
| (tmpfile-curbuf (make-temp-file "clang-format-git-tmp"))) |
There was a problem hiding this comment.
It looks like this temporary file isn't removed either.
| (when (buffer-name temp-buffer) (kill-buffer temp-buffer))))) | ||
|
|
||
| ;;;###autoload | ||
| (defun clang-format-git-diffs (&optional style assume-file-name) |
There was a problem hiding this comment.
A more general point, would it make sense to replace the term git with something more generic like vc.
This way we wouldn't need clang-format-hg-diffs or separate commands to support other version control systems in the future.
It can be documented that git is currently the only supported version control, others could be added.
I also find the term diffs a bit confusing, would changed make more sense?
There was a problem hiding this comment.
I think the term diff would be more consistent with existing emacs functions e.g. vc-diff, diff-delete-trailing-whitespace etc.
There was a problem hiding this comment.
The reason I chose diffs was to indicate it wasn't just the diff at point or something, but Ill rename clang-format-vc-diff
New proposed function `clang-format-git-diffs`.
It is the same as calling `clang-format-region` on all diffs between
the content of a buffer-file and the content of the file at git
revision HEAD. This is essentially the same thing as:
`git-clang-format -f {filename}`
If the current buffer is saved.
The motivation is many project (LLVM included) both have code that is
non-compliant with there clang-format style and disallow unrelated
format diffs in PRs. This means users can't just run
`clang-format-buffer` on the buffer they are working on, and need to
manually go through all the regions by hand to get them
formatted. This is both an error prone and annoying workflow.
Thank you for the detailed feedback, I will have v2 up tomorrow. Regarding the package lints, is requiring 25.1 acceptable? I'm not really sure how to implement this without |
goldsteinn
force-pushed
the
goldsteinn/emacs-clang-format-git
branch
from
3e7bac3 to
2c8395d
Compare
|
NB: The new API is 'clang-format-vc-diff'. I will update commit title when merging. |
|
Current return from |
|
Suggesting a fairly large change to this PR, although it's quite opinionated. If I were maintaining Exactly how this is done is not be so important... it could for example support a custom function that generates a list of line number pairs.
The function can simply return an ordered list of integer pairs representing lines. Then there can be a package on MELPA There are a few reasons this has benefits.
The main down side is users who rely on this behavior need to install an additional package although I don't see this as a big down-side. |
So IIUC, you are proposing a create an entirely seperate project for "vc-diff-lines" or something like that then making that a dependency for for
At least IMO, this is the other way around. Relying on external projects I think typically creates a higher maintainer burden, particularly in this case where I think LLVM would be the only user of the new package, so whenever changes where needed for the diff/vc stuff, it would require coordination with an external project. Further, if the external project got its own users and
Overall, I'm pretty down on this. IMO, the vc/diff functionality is pretty specific to the use-case we have in I think a similiar argument would imply the |
I would not depend on it - instead, it would be loosely coupled - if
This can happen for intricate API's - in this case - a single function that returns sorted line-ranges:
Fair enough - as noted, this is fairly opinionated - and I don't think your "wrong" for holding the contrary position. Keep in mind many people have been using There are some potential bugs that could bite us:
This is not an attempt to deminish the contribution, just to note that there are all sorts of cases where things can go wrong - where a feature like this turns out not to be all that simple and there are corner cases that need to be investigated/resolved. As noted, this is my suggestion to support a customizable line-range generator so |
I don't disagree these are all potential pitfalls (and there are certainly more), I just don't see how having the diff code in a separate project ameliorates any of them. And as stated earlier, I think it in fact complicates them. |
The issue of "complexity" is quite subjective, a customizable function that returns line ranges doesn't strike me as a complex API that's likely to break/change with the benefit of easily integrating other diffing methods. From a user perspective it likely just means one extra package, possibly setting a configuration value. Or, to avoid this PR having to handle system & version-control spesific details - we could consider calling |
All things considered, I think requiring |
Not sure what you mean exactly by a wrapper, if git/diff logic can be abstracted away - that seems a net gain. Anyway, I was only mentioning that we could consider abstracting away the logic - if it's not practical, there is no need to go into details discussing it, although I did mail the emacs-devel mailing list to check if this might be supported: |
Well the wrapped is to hide the ugly git/diff stuff.
My preference would be the get this in as is. |
|
ping |
1 similar comment
|
ping |
|
ping @ideasman42 |
|
ping @ideasman42 or @lukel97 |
|
Checking again and am still considering this patch too spesific/incomplete, checking vc's diff calls to git - they are considerably more involved than in this PR, meaning this PR will likely require follow up commits to fix problems (see Attached a patch that allows for formatting line-ranges, the line range generation must be implemented externally.
Patch files:
Perhaps support for version control diff's can be supported by clang-format-plus. As mentioned earlier, calling diff can be quite involved if all corner cases are properly handled. |
I'm not sure why the complexity of Can you expand on what is incomplete or too specific about it. We have
That seems far more specific and incomplete... but ultimately if its the only part that can be accepted its better than nothing.
Ultimately I really want to get this into actual clang-format where it will be maintained (by myself included) and kept up to date. I also think this functionality will be useful for other developers (lukel97 at least seemed to express he would find it useful). |
lukel97
left a comment
lukel97
left a comment
There was a problem hiding this comment.
Apologies for the delay on this again. However I tried it out locally and it now seems to work on macOS, thanks for fixing that!
I really can't speak much for the elisp, but we don't have many reviewers for the emacs stuff and this feature would be really handy to have.
So LGTM, if it needs more review I think it can be done post-commit :)
|
| (message "(clang-format: incomplete (syntax errors)%s)" stderr) | ||
| (message "(clang-format: success%s)" stderr)))) | ||
| (delete-file temp-file) | ||
| (ignore-errors (delete-file temp-file)) |
There was a problem hiding this comment.
suggest with-demoted-errors here too, could be a function as it's used elsewhere.
| (delete-file (pop ,bind-files-to-delete))))))) | ||
|
|
||
|
|
||
| (defun clang-format--vc-diff-get-diff-lines (file-orig file-new) |
There was a problem hiding this comment.
*picky* - personal preference, but I find it odd that the function returns command line arguments for clang-format.
It can return a list of cons-pairs of instead (list (cons int int) ...) instead, then the clang-format function can convert them to the arguments used by clang-format.
(the patch I attached did this), it's not a big change, I'd expect a formatting function that can operate on multiple ranges to take int-pairs instead of a list of strings.
ideasman42
left a comment
ideasman42
left a comment
There was a problem hiding this comment.
Works well on Linux, generally seems fine although I still think it's worth considering splitting out "getting-changes" into a separate shared code-base to avoid dealing with details of calling diff for different version control system here.
| (nreverse diff-lines)) | ||
| ;; Any return != 0 && != 1 indicates some level of error. | ||
| (t | ||
| (error "(diff returned unsuccessfully %s%s)" status stderr)))))) |
There was a problem hiding this comment.
All errors should use the "clang-format: " prefix, otherwise it can be difficult to track down errors when these functions are called indirectly.
|
``
Think lukel97 also tested on MacOS. I see where you're coming from regarding the split, although still feel quite strongly that features that don't make it into the mainline project have a high risk of becoming derelict. |
|
ping |
|
@ideasman42 ping |
|
Just a heads up I've been using this locally for a bit now and it's been great, thanks for working on this. Haven't run into any issues so far. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/14086 Here is the relevant piece of the build log for the reference |
…vm#112792) New proposed function `clang-format-vc-diff`. It is the same as calling `clang-format-region` on all diffs between the content of a buffer-file and the content of the file at git revision HEAD. This is essentially the same thing as: `git-clang-format -f {filename}` If the current buffer is saved. The motivation is many project (LLVM included) both have code that is non-compliant with there clang-format style and disallow unrelated format diffs in PRs. This means users can't just run `clang-format-buffer` on the buffer they are working on, and need to manually go through all the regions by hand to get them formatted. This is both an error prone and annoying workflow.
6 participants
New proposed function
clang-format-vc-diff.It is the same as calling
clang-format-regionon all diffs betweenthe content of a buffer-file and the content of the file at git
revision HEAD. This is essentially the same thing as:
git-clang-format -f {filename}If the current buffer is saved.
The motivation is many project (LLVM included) both have code that is
non-compliant with there clang-format style and disallow unrelated
format diffs in PRs. This means users can't just run
clang-format-bufferon the buffer they are working on, and need tomanually go through all the regions by hand to get them
formatted. This is both an error prone and annoying workflow.