Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature suggestion: Add support for running on all commits in a branch #14

Closed
timabbott opened this issue Jun 26, 2016 · 7 comments
Closed
Labels
enhancement User-facing feature enhancements

Comments

@timabbott
Copy link

One can in theory just pass each commit into gitlint in a shell loop or something, but since Python initialization is somewhat slow, it'd be significantly faster and cleaner if one could do e.g. gitlint origin/master..HEAD and have it lint all the commits in that series.

@jorisroovers
Copy link
Owner

Yes, this is something I've had in mind for a while. I think the ideal implementation would allow you to specify a single specific commit, a range of commits, the last x commits or as you suggested, all commits in the current branch.

I'm thinking something like:

# single specific commit
gitlint --commits 123abc
# list of commits
gitlint --commits 123abc,456def,789abc
# range of commits
gitlint --commits 123abc..456def
# last 4 commits
gitlint --commits -4
# all commits
gitlint --commits all

We should probably start with a simpler version though:

git log > mylog.txt
cat mylog.txt | gitlint

I'm thinking this might actually be sufficient because you can obviously slice and dice the log file that you pipe into gitlint as you like. The --commits parameter above is just a convenience thing at that point.

The 0.7.0 release actually did some refactoring that was required to make this possible in the future, the actual implementation is still left to do though. An open item is the output formatting (how to represent a list of gitlint results). This ties into structured formatting output (e.g. json/yaml); something I've also thought about.

Anyways, I don't have a specific timeline for this feature in mind (might be months, perhaps just weeks), but I'll add this eventually if nobody submits a PR before that :-)

Thanks!

@jorisroovers jorisroovers added the enhancement User-facing feature enhancements label Jun 27, 2016
@timabbott
Copy link
Author

Agreed, I suspect it'd be easiest to just support a git refspec as the argument format, especially if you use python-git to parse the data from the git repository...

I would probably represent the results compiler-style, e.g.:

In `first commit message`:
<gitlint error message>
<second gitlint error message>
In `third commit message`:
<another gitlint error message>
``

@tommyip
Copy link
Contributor

tommyip commented Mar 4, 2017

@jorisroovers @timabbott I will try to add this feature.

@jorisroovers
Copy link
Owner

jorisroovers commented Mar 5, 2017

Awesome!

I haven't come around to doing this, mostly because it's a good amount of work to do it right.

Also, just want to reiterate that there's an easy workaround for linting a range of commits using a bash for loop: http://jorisroovers.github.io/gitlint/#linting-a-range-of-commits

By implementing this in gitlint directly, we'd mostly gain speed (although I have no idea how much) because we'd cut out the python and git startup times per commit, and obviously we'd also gain some more elegance and usability compared to doing it using the bash for-loop. FYI that I ran the bash for-loop on the whole zulip git log on my 2015 MBP (2,3 GHz Intel Core i7, 16GB RAM) and that took 26m45.965s to complete (for future reference, that's for ~15,700 commits).

The reason I bring all of this up is because it might not be worth the effort implementing this feature (although I'd love it of course!) if you're just looking for a quick win which can be accomplished using bash. Especially because it's hard to tell how much faster it will be in "native" gitlint without actually implementing it. From that perspective, you also might want to give some thought about what would be acceptable for you in terms of linting time and whether we'd ever be able to achieve that using gitlint.

However, if you still want to implement it, here's some pointers :)

https://github.com/jorisroovers/gitlint/blob/master/gitlint/git.py#L120:

def from_local_repository(repository_path):

I recommend adding a second optional parameter refspec. If refspec is defined, you pass it to git and then parse the commits into the different commit objects (I actually don't use a python git library for that but just invoke the git command, that's mostly historical, I recall having some good reasons at the time but forgot the details).

https://github.com/jorisroovers/gitlint/blob/master/gitlint/cli.py#L113:
Add logic to deal with gitcontext.commits having more than one commit (i.e. add a loop). We also need to add some output in the loop to distinguish between commits. I think the best way to do this is adding a method print_commit to linter.py that prints the commit id of a given commit (in the future this probably belongs in display.py).

And then also add a good amount of tests of course :)

FYI that I'm still planning on doing the above at some point in the future if you don't do it, but no commitments on timelines :)

Thanks!!

@tommyip
Copy link
Contributor

tommyip commented Mar 5, 2017

Yeah I wouldn't be linting the whole Zulip repo, just upstream/master...HEAD, so either way it shouldn't take more than a few seconds. But I think it is still a nice and useful feature anyway.

@tommyip
Copy link
Contributor

tommyip commented Mar 5, 2017

Is there a particular reason why the commit in GitContext is stored in a list even though only the HEAD commit is linted currently?

@jorisroovers
Copy link
Owner

That's because I already did some preliminary work for this feature in the past. As you noticed, right now there's only 1 commit in GitContext.commits, but that list is where you should be adding the other ones.

tommyip added a commit to tommyip/gitlint that referenced this issue Mar 5, 2017
tommyip added a commit to tommyip/gitlint that referenced this issue Mar 6, 2017
tommyip added a commit to tommyip/gitlint that referenced this issue Mar 6, 2017
tommyip added a commit to tommyip/gitlint that referenced this issue Mar 6, 2017
tommyip added a commit to tommyip/gitlint that referenced this issue Mar 11, 2017
This allows user to specify a range of commits using the --commits
cli option. It currently accepts either a individual commit hash
(eg: gitlint --commits 6f29bf8) or a range of commits using the git
range syntax (eg: gitlint --commits 9c6f70d...HEAD).

Fixes jorisroovers#14.
tommyip added a commit to tommyip/gitlint that referenced this issue Mar 11, 2017
This allows user to specify a range of commits using the --commits
cli option. It currently accepts either an individual commit hash
(eg: gitlint --commits 6f29bf8) or a range of commits using the git
range syntax (eg: gitlint --commits 9c6f70d...HEAD).

Fixes jorisroovers#14.
tommyip added a commit to tommyip/gitlint that referenced this issue Mar 11, 2017
This allows user to specify a range of commits using the --commits
cli option. It currently accepts either an individual commit hash
(eg: gitlint --commits 6f29bf8) or a range of commits using the git
range syntax (eg: gitlint --commits 9c6f70d...HEAD).

Fixes jorisroovers#14.
tommyip added a commit to tommyip/gitlint that referenced this issue Mar 11, 2017
This allows user to specify a range of commits using the --commits
cli option. It currently accepts either an individual commit hash
(eg: gitlint --commits 6f29bf8) or a range of commits using the git
range syntax (eg: gitlint --commits 9c6f70d...HEAD).

Fixes jorisroovers#14.
jorisroovers pushed a commit that referenced this issue Mar 12, 2017
This allows user to specify a range of commits using the --commits
cli option. It currently accepts either an individual commit hash
(eg: gitlint --commits 6f29bf8) or a range of commits using the git
range syntax (eg: gitlint --commits 9c6f70d...HEAD).

Fixes #14.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement User-facing feature enhancements
Projects
None yet
Development

No branches or pull requests

3 participants