check-webkit-style --git-commit=HEAD doesn't check any files (assuming there's no local edit in the index). When you want to check the most recent commit, you need to pass --git-commit=HEAD^. $ ./check-webkit-style --git-commit=HEAD Total errors found: 0 in 0 files This behavior is not compatible with prepare-ChangeLog; when it is given --git-commit=FOO, it edits ChangeLog using commits from FOO^, not from FOO. The following code snippet took from prepare-ChangeLog describes the behavior well: sub diffFromToString() { return "" if $isSVN; return $gitCommit if $gitCommit =~ m/.+\.\..+/; return "\"$gitCommit^\" \"$gitCommit\"" if $gitCommit; return "--cached" if $gitIndex; return "HEAD" if $isGit; } It's easy to misuse current --git-commit option of check-webkit-style. I think it should mimic the behavior of prepare-ChangeLog.
(In reply to comment #0) > check-webkit-style --git-commit=HEAD doesn't check any files (assuming there's > no local edit in the index). I use the git-commit option pretty often. If I remember right (I'm away from my dev machine right now), one nice thing about the current behavior is that it behaves the same as git-diff. So "check-webkit-style --git-commit <commit>" checks style for "git diff <commit>". Either way, it seems like we should choose based on what semantics are more "git-like" rather than necessarily aligning with WebKit scripts. Is it possible the prepare-Changelog semantics are what should be corrected?
I agree with Yuta. I'm happy if --git-commit works against the change in the specified commit, and we rename the current --git-commit to --since-git-commit or --after-git-commit.
To me, "--git-commit=FOO" sounds like a single commit (FOO^..FOO) or a series of commits including FOO (FOO^..HEAD), but not FOO..HEAD (which does not contain FOO itself). If the name of this option was --git-commit-since=FOO or something like that, I would be happy about the current behavior. So I feel the current prepare-ChangeLog is doing the right thing, and I think check-webkit-style needs to act like prepare-ChangeLog. Furthermore, adding --git-index option (available in prepare-ChangeLog) might be nice. I want to hear opinions from other folks. Once an agreement is reached, I'd like to fix it.
(In reply to comment #3) > To me, "--git-commit=FOO" sounds like a single commit (FOO^..FOO) or a series > of commits including FOO (FOO^..HEAD), but not FOO..HEAD (which does not > contain FOO itself). If the name of this option was --git-commit-since=FOO or > something like that, I would be happy about the current behavior. Note that there is a FIXME to support ranges for the --git-commit flag: http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/check-webkit-style#L94 # FIXME: If the range is a "...", the code should find the common ancestor and # start there (see git diff --help for information about how ... usually works). If check-webkit-style supports ranges like <commit1>..<commit2>, it seems like it would make the most sense for the semantics to be the same as the semantics for git-diff (I'm not sure what natural alternative there is): http://www.kernel.org/pub/software/scm/git/docs/git-diff.html And the case of a single commit is a special case. I agree the name of the flag could be better. How about --git-diff to indicate that the value should be interpreted as it is interpreted for git-diff?
Sorry for late reply... (In reply to comment #4) > (In reply to comment #3) > > To me, "--git-commit=FOO" sounds like a single commit (FOO^..FOO) or a series > > of commits including FOO (FOO^..HEAD), but not FOO..HEAD (which does not > > contain FOO itself). If the name of this option was --git-commit-since=FOO or > > something like that, I would be happy about the current behavior. > > Note that there is a FIXME to support ranges for the --git-commit flag: > > http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/check-webkit-style#L94 > > # FIXME: If the range is a "...", the code should find the common ancestor and > # start there (see git diff --help for information about how ... usually > works). > > If check-webkit-style supports ranges like <commit1>..<commit2>, it seems like > it would make the most sense for the semantics to be the same as the semantics > for git-diff (I'm not sure what natural alternative there is): > > http://www.kernel.org/pub/software/scm/git/docs/git-diff.html > > And the case of a single commit is a special case. I agree the name of the > flag could be better. How about --git-diff to indicate that the value should > be interpreted as it is interpreted for git-diff? It's probably useful to provide the functionality of creating diff from a single commit object (like current prepare-ChangeLog), because many people want to check the most recent local commit. "git-show HEAD" (or just "git-show") just does this. I believe it's very natural to specify a commit in this way, because a commit FOO consists of a diff from the last revision (which is FOO^..FOO) and metadata about the commit (e.g. commit log, author name, etc.). By using git-show, you can see how a commit is organized. Among other git commands, git-diff is somewhat special in terms of specifying commits. It receives a range of commits, not a single commit object. I'm happy about renaming current --git-commit to --git-diff. To be in harmony with prepare-ChangeLog, I want to propose the following set of flags: --git-commit=FOO => one commit: diff between FOO^..FOO (which is shown by "git show FOO", same as current prepare-ChangeLog) (gracefully renamed to indicate multiple commits might be included) --git-commits-diff=FOO..BAR => diff between FOO..BAR --git-commits-diff=FOO => implies FOO..HEAD (same as git-diff) --git-index => "git-diff --cached" This is almost same as current prepare-ChangeLog, except --git-commit and --git-commits-diff are distinguished. I hope I explained well... Please feel free to ask if you have any questions or suggestions.
I have almost no opinions on this bug, but please note that it's not trivial to implement --git-commits-diff=FOO..BAR . To support this style, we need to checkout BAR into somewhere first. Otherwise, users may be confused when there are changes after BAR. Personally, I've never wanted to check styles for FOO..BAR where BAR isn't the working tree. So, I'm not sure if it's worth supporting ranges.
(In reply to comment #6) > I have almost no opinions on this bug, but please note that it's not trivial to > implement --git-commits-diff=FOO..BAR . To support this style, we need to > checkout BAR into somewhere first. Otherwise, users may be confused when there > are changes after BAR. Personally, I've never wanted to check styles for > FOO..BAR where BAR isn't the working tree. So, I'm not sure if it's worth > supporting ranges. Good point - I didn't know that. Then, instead of --git-commits-diff, we might want to have: --git-commits-since=FOO => diff between FOO..HEAD (option name should need some discussion) How about this?
(In reply to comment #7) > (In reply to comment #6) > > I have almost no opinions on this bug, but please note that it's not trivial to > > implement --git-commits-diff=FOO..BAR . To support this style, we need to > > checkout BAR into somewhere first. Otherwise, users may be confused when there > > are changes after BAR. Personally, I've never wanted to check styles for > > FOO..BAR where BAR isn't the working tree. So, I'm not sure if it's worth > > supporting ranges. > > Good point - I didn't know that. Then, instead of --git-commits-diff, we might > want to have: > > --git-commits-since=FOO => diff between FOO..HEAD > (option name should need some discussion) > > How about this? Thanks for considering my thoughts, Yuta. I should be okay with exposing a short-cut option for examining a single commit (e.g. using the --git-commit flag). But I have some clarifying questions. In particular, I'd like to understand if there will ever be overlap between the functionality provided by the different flags we're considering supporting. Like, do you know what short-cut syntaxes git-diff provides for specifying a single commit (i.e. the diff between a commit and the commit following)? > --git-commits-since=FOO => diff between FOO..HEAD Assuming the line above represents the current functionality, shouldn't the "since" option also include changes in the working directory (i.e. uncommitted changes) -- or did you intentionally leave that out? I thought that's what check-webkit-style did today, but I could be wrong. I'm somewhat new to git, so sorry if I'm overlooking something obvious. > (option name should need some discussion) I'd prefer that the option names be short if possible. If we don't foresee supporting ranges more general than "since" ranges anytime soon, then how about something like --git-since (if not --git-diff which I suggested before)? (And again to clarify, is --git-since essentially the current behavior?) Finally, how would check-webkit-style behave with no flags passed at all? Would that be distinct from all of the flags you're suggesting, or would it be the same as one of them? Thanks.
It sounds like changing the name of the option would be a fine solution to avoid the confusion that people are having (perhaps --git-from= although --git -commits-since= also works). (In reply to comment #6) > I have almost no opinions on this bug, but please note that it's not trivial to > implement --git-commits-diff=FOO..BAR . To support this style, we need to > checkout BAR into somewhere first. Otherwise, users may be confused when there > are changes after BAR. Personally, I've never wanted to check styles for > FOO..BAR where BAR isn't the working tree. So, I'm not sure if it's worth > supporting ranges. I agree with Hamaji that there are several technical and user issues: 1. You need to checkout BAR somewhere, so that the tool only checks the lines as they appeared in BAR. 2. Then how do you report the lines? do you use the line numbers as they are in BAR or the current line numbers. The line numbers as they are in BAR is confusing. The line numbers as they are now is more complicated and what do you do with lines that were deleted. Then, you need to consider not only doing these changes but maintaining their correctness as other changes as done. As I understand it, there isn't a common use case -- just some confusion over how it works. If one want to do this, they could do this: git checkout -b junk && git reset --hard BAR && check-webkit-style --git-from=BAR FileName. It is a little bit involved but if you're not doing it often, then not a big deal.
FYI, the change here-- https://bugs.webkit.org/show_bug.cgi?id=36394 may make the change suggested here less critical. The reason is that bug 36934 will make check-webkit-style check all differences from the trunk by default. Since the most common scenario for most people is probably all changes being in HEAD (possibly also with changes in the working directory), bug 36934 means that in those cases people will be able to type-- > check-webkit-style instead of-- > check-webkit-style --git-commit HEAD^ (thus eliminating having to type the ^ in the most common case). Am I right in that summary?