WIP: Add -r / --revision argument for specifying which Git revision to compare to

[akaihola]

Fixes #42. Should be reviewed and merged only after #48 has been merged.

TODO: unit tests

• two commits in a repo, compare to first
• test case for when current files didn't exist at the given revision
• test case for git_get_unmodified_content() with a non-HEAD revision
• add cases to test_revision() with unmodified content compared to the given revision

[Carreau]

Ah well, I see we started working on the same thing at the same time twice !

[Carreau]

I was also thinking in a config file (pyproject.toml at the root of the repo?) having the ability to give a revision to be used by default. But +1.

[akaihola]

Ah well, I see we started working on the same thing at the same time twice !

😄 yes... the solutions are pretty similar.

Do you know if there's a difference in any situation between git show :./file.py and git show HEAD:./file.py?
I did notice that in one of the tests I did only git add without git commit, and that test broke when : changed to HEAD:.

I'm also considering different names for the option. Git documentation shows <commit> in command line examples, explains the syntax in "SPECIFYING REVISIONS", and I also see "commit-ish" and "committish" used.

From the glossary:

commit
As a noun: A single point in the Git history; the entire history of a project is represented as a set of interrelated commits. The word "commit" is often used by Git in the same places other revision control systems use the words "revision" or "version". Also used as a short hand for commit object.

commit-ish (also committish)
A commit object or an object that can be recursively dereferenced to a commit object. The following are all commit-ishes: a commit object, a tag object that points to a commit object, a tag object that points to a tag object that points to a commit object, etc.

revision
Synonym for commit (the noun).

So I see your point about "commit-ish" – for example a tag object is not a commit or a revision.

But I also think --commitish [sic] is a clumsy option name and probably an obscure term for the average user. That's why I ended up using --revision instead – also if you look at the gitrevisions documentation, that does use <rev> for the part of the git show expression we're using:

<rev>:<path>, e.g. HEAD:README, master:./README

These are the reasons I came up with --revision. What do you think?

[akaihola]

Summary of the main differences between mine vs. your solution:

• revision vs. commitish
• rename head_vs_lines() to revision_vs_lines() vs. keep the name
• default value of "HEAD" vs. ""
• handle missing files in the given revision (in my version)

The rest of the differences are mostly a matter of taste. @Carreau could you review my MR and point out any flaws and design decisions you dislike? Also if you accept my invite on GitHub to become a collaborator on this repository, I can assign the MR for you to review.

[akaihola]

@Carreau, I found a flaw in that untracked files were completely skipped. That's now fixed in #48, and this branch was re-built on top of that. Could you also review #48?

[Carreau]

Do you know if there's a difference in any situation between git show :./file.py and git show HEAD:./file.py?

Yeah, my main concern would be if users have staged only some ; or partial version of a file I think it may differ; but I'm not too sure. I think that's fine enough and can be refined in further iteration if there are complaint.

I actually like -r/--revisions, I think it will be understandable enough, and also acomodate for potentially other VCS later (though I'm doubtfull), it's neet that it seem to work with things like @{yesterday}, or master....

[Carreau]

I think that --revision + --check, will be really neet for CI. It will allow project to at least make sure that all contributions past a certain point are compliant, and I'll be able to likely update one of my bot, to actually comment on PRs using the github suggestion to actually automatically fix PRs.

[sourcery-ai]

Sourcery Code Quality Report (beta)

❌  Merging this PR will decrease code quality in the affected files by 0.19 out of 10.

Quality metrics	Before	After	Change
Complexity	1.28	1.56	0.28 🔴
Method Length	59.58	68.45	8.87 🔴
Quality	8.56	8.37	-0.19 🔴

Other metrics	Before	After	Change
Lines	817	927	110

Changed files	Quality Before	Quality After	Quality Change
src/darker/main.py	7.45	7.43	-0.02 🔴
src/darker/command_line.py	7.89	7.83	-0.06 🔴
src/darker/git.py	9.05	8.73	-0.32 🔴
src/darker/tests/conftest.py	9.33	9.21	-0.12 🔴
src/darker/tests/test_command_line.py	8.51	8.50	-0.01 🔴
src/darker/tests/test_git.py	8.48	8.09	-0.39 🔴
src/darker/tests/test_main.py	8.63	8.62	-0.01 🔴
src/darker/tests/test_main_revision.py		5.47

Here are some functions in these files that still need a tune-up:

File	Function	Complexity	Length	Overall	Recommendation
src/darker/main.py	format_edited_parts	21	215.89	4.05	Split out functionality
src/darker/main.py	main	16	184.82	4.81	Split out functionality
src/darker/tests/test_main_revision.py	test_revision	3	247.79	5.47	Split out functionality
src/darker/command_line.py	parse_command_line	2	200.38	5.99	Split out functionality
src/darker/tests/test_git.py	test_git_get_modified_files	4	166.53	6.19	Split out functionality

Please see our documentation here for details on how these metrics are calculated.

We are actively working on this report - lots more documentation and extra metrics to come!

Let us know what you think of it by mentioning @sourcery-ai in a comment.

[akaihola]

I was also thinking in a config file (pyproject.toml at the root of the repo?) having the ability to give a revision to be used by default. But +1.

That's a good idea, @Carreau. I created #49 to think about configuration files for Darker. I think we should solve that in a separate PR.

You didn't yet approve this pull request – do you have any additional remarks or could we merge?

[Carreau]

Sorry, approved !

I created #49 to think about configuration files for Darker.

Yes, I saw, and you tagged 1.2.0, which I think is reasonable. Just -r is I think the last thing needed to be able to run darker in ci as you can hardcode the revision in the test script.