clang-format hook suggestions |
||
Issue descriptionLooks like we somewhat recently started enforcing a clang-format repohook across most subdirectories of the platform2 repo. I ran into this as a developer for the first time and it gave me several issues that made it hard to upload my change: 1) Looks like it shells out to 'git diff' at some point without passing --no-ext-diff. For people who have diff.external set in their .gitconfig (e.g. to a vimdiff wrapper), it just dies in a fire and screws up your TTY. 2) The help text for how to fix your commit thinks that you're in the top level directory of the git repo even if you are not... so it will tell you to run clang-format.py --fix crash_reporter/ even if you're already in the platform2/crash_reporter/ subdirectory and should be running clang-format.py --fix . instead. (It also tells you to add a whole lot more directories on that line even if you didn't do any changes to them for some reason.) 3) When uploading a patch train, you have to rebase until you're on the patch it complains about before the command it tells you to run does anything, which is not obvious from the messaging. It's also really annoying that you have to supply the commit hash every time, especially if you have to fix up more than one patch (so that the later ones' hashes change and you have to run repo upload again to get the new command to copy&paste). It would be much nicer if you could just do --current-commit or something (or, if it accepts --commit HEAD, it should tell you that). 4) If you run it in the wrong way it will just output nothing, which is the same as when you run it the right way. Very confusing. It's also not really clear that it updates the working tree for you and expects you to run amend yourself. If a successful invocation (that makes changes) would output something like "Fixed up files:\n...<file list>...\nRun git commit --amend to apply the fixes to the current commit.", I think first-time usage would be a lot clearer. 5) I'm also not happy about the fact that it seems to think it knows better than me how to make my code look good, but I assume that's WAI...
,
May 8 2018
adjusting priority here as i don't see anything in your description that is blocking your work
,
May 8 2018
> (2) that doesn't really scale with commits across dirs, or when using `repo upload` on multiple git projects simultaneously. we should just add a sentence like "run in the top of the git repo". or just leave it be. Doesn't it? I think the output should always be relative to the current working directory, not some arbitrary point like the top of the repo. If that means you'll have some ../../.. in there, that's still better than giving you a command line to run that doesn't work, right? > 4) what do you mean by "the wrong way" ? in that it didn't make any changes when you passed it paths that didn't exist ? or something else ? it should probably error out if you gave it missing paths, Yes. Erroring out on a non-existent path would be a good start. But I think I ran it in other ways that didn't produce any output too, like maybe using the old commit hash after the commit had changed due to a rebase to apply fixes for the earlier patch and stuff... to be honest, I have no idea what all the options (like that commit hash) really mean... I just kept trying to run different commands in frustration (most of which just "succeeded" with no output) until I finally could get the pre-submit to pass. I think if you want to have this in the pre-submit and force everyone to deal with it, making it user-friendly and easy to understand is important... asking everyone to suddenly become an expert in invoking this tool they've never seen before just so they can continue to do their work isn't great. > i think the current behavior is correct and is inline with standard UNIX tools. it'd be like saying "running `sed -i` on files should output a summary of what it did". which isn't to say we couldn't request support for something like --verbose. This isn't sed or cat, though, you're not going to run it in a pipe and you're not going to run it so often in a row that the output would annoy you. When you run 'git push' or 'emerge mypackage', it also gives you some explanatory output on success. This makes even more sense because the result of this tool is not as predictable as that of most "simple" UNIX commands (it might change some files, but you don't know which beforehand). It's also helpful to know whether it changed anything at all or not so you know that you have to amend. Whether you want this to be under -v or have a -q to suppress it or something is a different question, but I think the command line that the pre-submit hook suggests you should run should have this output, so that developers who're unfamiliar with what it does have an easier time understanding what's going on.
,
May 9 2018
(2) the repo upload hooks don't have access to where you run `repo`. they have access to the top of the repo checkout and the current git checkout which is being checked. so it's not possible to do any relative paths. (4) wrt clang-format.py, the c-i-t maintains that tool, so requests for improvements/changes can be forked & sent to them. > I have no idea what all the options (like that commit hash) really mean honestly ? clang-format operates on git diffs, so the args seem fairly straight forward to me: - you passed it paths, therefore clang-format is only going to operate on those paths. - you passed it a git hash, so it's only going to operate on the changes made in that specific git commit instead of whatever is on disk atm. |
||
►
Sign in to add a comment |
||
Comment 1 by vapier@chromium.org
, May 8 2018