--revision <branch>... compares to HEAD, not working tree

[akaihola]

In local development work, one use case of darker is to check formatting and linting against the branch point from master. This can be achieved using e.g. --revision master... – but the current implementation compares to HEAD and ignores uncommitted changes in the working tree and the staging index.

This could be fixed by first doing a git merge-base <branch> HEAD, followed by git diff --name-only --relative <revision>... -- <paths> with <revision> from the output of merge-base.

This requires a bit of parsing for the range given using the --revision argument.

On the other hand, this would make the meaning of revision expressions different between Git and Darker. In git diff, these revision expressions show changes between:

• <commit>: the specified <commit> and the current working tree – you could think <commit>..<working tree> (although there really isn't an identifier for the current working tree)
• <commit>..HEAD: <commit> and the current last commit, ignoring the staging index and the working tree
• <commit>...HEAD: <commit> and its latest ancestor which is common with HEAD (ignoring the staging index and the working tree)
• <commit1>...<commit2>: <commit1> and the latest common ancestor of <commit1> and <commit2>
• <commit>..: (alias for <commit>..HEAD)
• <commit>...: (alias for <commit>...HEAD)

So for use in CI, Darker's current --revision <commit>... option is fine, because in that situation there shouldn't be anything in the staging index, and no uncommitted changes in the working tree.

But for local use, the user probably most often wants to run Darker before committing and does want to include staging index and working tree content in the comparison. Without the --revision option, Darker already does this comparison correctly against HEAD (the latest commit), but when working on a feature branch, it's probably better to compare against the latest common ancestor of the feature branch and the main branch (e.g. master). For this we currently have no --revision expression, and there is none to adopt from Git either.

To illustrate the local use case, here's a log of a hypothetical repository in which we probably most often want to compare (branch point)..(working tree), which can be achieved in git by doing a git diff $(git merge-base master HEAD):

x   working tree changes
x   staging index changes
*   (my-feature) (HEAD)
*   previous commit on the my-feature branch
| * (master) new commit after the branch point
|/
*   branch point – master used to be here when my-feature was branched

Some of the alternatives to consider:

1. keep using Git syntax but extend it with keywords like master...WORKTREE, master...INDEX – but this would cause a collision if anyone needed to name their branches or tags WORKTREE or INDEX
2. keep using Git syntax but extend it with keywords form the working tree and the staging index which are invalid for Git branch/tag names – but it's challenging to come up with identifiers which make sense and aren't reserved in Git
3. keep using Git syntax with the exception that omitting the comparison target defaults to the working tree instead of HEAD – so master.. -> master..(worktree) and master... -> master...(worktree), and default Git behavior can still be achieved with e.g. master..HEAD and master...HEAD
4. come up with a different syntax than what Git uses, and don't accept Git syntax at all – but then arbitrary git revision expressions wouldn't be supported
5. use Git syntax for --revision, but add a separate command line option to use the working tree or staging index instead of HEAD as the comparison point when using --revision <commit>.. or --revision <commit>..., e.g. --git-worktree --revision master... or --git-index --revision master...
6. use Git syntax for --revision, but add a separate command line option to use the latest common ancestor when using --revision <commit>, e.g. -m -r master or -b -r master or --merge-base --revision master

[akaihola]

@Carreau @CorreyL @Mystic-Mirage @samoylovfp @CircleOnCircles @Pacu2 @DylanYoung

I have a tricky design issue with Darker in #79. Basically the question is how to design command line options for properly comparing the working tree in a feature branch to a main branch. We need to compare to the latest common ancestor of the feature and main branches, because the main branch might have commits after the branch point. But we also want to include the Git staging index and the working tree in the comparison.

It would be helpful if anyone could review my line of thought in the issue and give an opinion of what would be a good design here. Thanks!

[Carreau]

'master..' (two dots) compare with head, 'master...' (3dots) compare with the merge-base (aka common ancestor). At least git should do that. I'll reach the thing you point to in more detail.

[Carreau]

Sorry scratch what I said above, it's too early, and that what you said in the other issue.

[akaihola]

Just to clarify, here's a problematic example case:

A file has been removed in master after the branch point. Using -r master would completely reformat that file in my-feature even if there were no changes in the branch after the branch point.

The solution is to use -r master... which causes Git to compare (branch point)..HEAD instead of master..HEAD. But, now changes in the staging index and the working tree are ignored.

x   working tree – changes to other files but not to myfile.py
x   staging index – changes to other files but not to myfile.py
*   (my-feature) (HEAD) – changes to other files but not to myfile.py
| * (master) delete myfile.py
|/
*   branch point – myfile.py exists here

[akaihola]

Currently I'm liking options

• 3. (-r master... without target commit defaults to -r master...(worktree)); and
• 6. (-m option in -m -r master causes latest common ancestor to be used)

Other opinions?

[akaihola]

(ping @jasaarim to let you know what I'm currently wrestling with :)

[CorreyL]

Just my two cents, coming into this having read over what's been outlined so far (thanks for such great detail into your thought process btw @akaihola)

• 3. (-r master... without target commit defaults to -r master...(worktree)); and

This seems to be the most user friendly and obvious option from an outsiders perspective, without greater context of the discussion outside of this issue (if it has been discussed previously).

[wasdee]

I would like to help but, I'm very noob about git. 😓 I use Gitkraken on daily basis. I have difficulties to comprehend.

[akaihola]

but, I'm very noob about git

@CircleOnCircles, your input may be the most valuable one precisely because of that! It's best if Darker works in an obvious way no matter if you're a Git newbie or a seasoned expert.

@CorreyL as said, I like alternative 3 as well. Its only downside is deviating from Git semantics – git diff master... means git diff $(git merge-base master)..HEAD, and darker --diff master... will internally do a git diff $(git merge-base master) (comparing to the working tree) instead.

But, as also said, it will still be possible to say darker --diff master...HEAD to get Git's master... behavior.