Suggestion for selecting all file changes since a revision

[markddavidoff]

locally i made a wrapper to call black/darker with $(git diff --name-only --diff-filter=d\* $(git rev-parse --abbrev-ref HEAD) master -- ':*.pyi' ':*.py') as the paths argument, which will specify all changes (excluding deletions) on a specific branch.

I know not everyone uses git, but i think its pretty useful to have built in for most cases and its easy enough to error when a flag is specified but git is not present and it should be easy enough to tweak to play with the -r flag.

[akaihola]

Hi @markddavidoff,
Thanks for your suggestion!

If I understand it correcly, the existing -r / --revision argument (see documentation) should already do this!

Update: This may be a duplicate of #85 and #159, fixed in Darker 1.3.0.

[markddavidoff]

@akaihola -r also requires passing the files you want to black, at least for me, with just -r <sha> i get darker: error: the following arguments are required: PATH

[akaihola]

@markddavidoff you can pass it a directory, and it will check all files in that tree recursively.

If that's unclear from the documentation, I consider it a documentation bug that needs fixing!

[akaihola]

I improved documentation in #69. Could you @markddavidoff review that?

[markddavidoff]

@akaihola I don't want to pass it a directory though, I'd like to explicitly pass it a list of files, the same way i do with black. (my directory has A LOT of files)

[akaihola]

@markddavidoff sorry I don't fully get what your use case is. Is it possible that you've missed the fact that Darker in fact does only reformat files which have been modified compared to the branch you specify using --revision?

I'll walk you through an example of what Darker does, because I think it somewhat resembles the use case you're describing.

Let's say your repository is in ~/my-repo/, master is your main branch and you have 100 .py files in your repository. You're in a my-feature branch in which you have edited 25 of those files across different parts of the repository:

$ cd ~/my-repo ; git stat
## my-feature
 M setup.py
 M setup.cfg
 M src/myrepo/__main__.py
 M src/myrepo/tests/test_main.py
# ... and so on

If you now run

$ darker --revision master ~/my-repo

then Darker will

1. ask Git which files have changed between master and my-feature
2. ask Git which lines were edited by you in each of those files between master and my-feature
3. reformat those files in-memory using Black (not writing to disk)
4. compare reformatted files to those still intact on disk
5. apply only those chunks of reformatting which hit lines previously edited by you
6. write the results on disk for each file

You may get a better sense of all the steps Darker does by running it with the -vv verbosity option.

Do you think this is anywhere near what you're trying to achieve?

[markddavidoff]

Darker in fact does only reformat files which have been modified compared to the branch you specify using --revision?

I believe explicitly darker only formats files which have been modified compared to the branch you specify using --revision and are also in the specified directory (~/my-repo) in your example.

That last part is the bit that is the point of confusion. since ~/my-repo can be subsituted for ~/my-repo/my_file.py i assume darker is not using ~/my-repo to find the repo to run darker in, but instead using it for 4.compare reformatted files to those still intact on disk. Is that correct? if the goal is to only look at the changed files, is specifying the path necessary since we know the paths of the changed files from git?

The reason i wanted to specify files, is that I expected darker to be a drop in replacement/analogue for black. That i could replace black in any black command with darker -r master and instead of blacking the whole file it would only black the diff between my changes and master. That does not seem to be the case as the file path argument works differently in darker then it does black

[markddavidoff]

when i run black $(git diff --name-only --diff-filter=d\* $(git rev-parse --abbrev-ref HEAD) master -- ':*.pyi' ':*.py') it blacks all modified, non-deleted python files in the diff between my current HEAD and master. The goal is to diff the files that have changed. The file paths argument here, the output of git diff is a list of file paths

with darker i can instead run darker -r master . which will do the same git diff part and try to compare that with all the files in my current folder, I get that, i just want to be able to a) specify a subset of the folder to check by being able to pass a list of files instead of a dir. this feature isn't required as you can work around it by calling darker multiple times but it would provide 1:1 command parity with black. b) didn't expect the file path to darker to work differently than to black

[akaihola]

darker <directory> will actually walk the whole directory tree recursively and find all .py files which Git says have been modified since the given revision.

If <directory> is the root of your repository, it will find each and every modified .py file in the whole repository.

We don't want darker -r master (without a path) to do anything since that could lead to "accidents".

What you can do is specify the default --revision and path/directory to process in your pyproject.toml. For example, if you don't want files like setup.py in your repository root to be reformatted, and also want to leave test modules intact but still format a helpers module in there, you could do this:

[tool.darker]
revision = "master"
src = [
    "src/mypackage",
    "tests/helpers.py",
]

Then running darker without arguments inside your repository would be equivalent to

$ darker -r master src/mypackage tests/helpers.py

[akaihola]

@markddavidoff you might want to take a look at darker.git.git_get_modified_files() to understand how Darker decides which files to process.

When that function is called,

• paths corresponds to the paths given on the command line (files or directories)
• revision is what was given using the -r/--revision option, HEAD by default
• cwd is the root of the Git repository which contains all the paths listed on the command line

[akaihola]

@markddavidoff

the file path argument works differently in darker then it does black

This is certainly not the intention. Darker should only consider files listed on the command line, and in those files, only those regions which have changed compared to the commit specified using the -r/--revision option.

Could you give an example of where this is not true?

[markddavidoff]

hmmm, when i run with more than one file path, i often get an error, perhaps that is why I assumed it did not work as expected.

[akaihola]

@markddavidoff, I would very much like to reproduce that problem on my own laptop. Are you running Darker on an Open Source project so I could try it on the same Git repository? Could you copy-paste what you see on screen? Could you also add the -vv command line option to get debug output, and attach that here?

I'm sure I'll be able to help you once I get more information about the problem.

[akaihola]

Hi @markddavidoff,

Do you remember this Darker problem from last September? Any new insight into it?

[markddavidoff]

@akaihola yeah, the main issue is how darker handles new files that are not in the specified previous revision:
darker --revision master .
yields
fatal: path 'xxxxxx' exists on disk, but not in 'master'
and doesn't actually do anything.

[markddavidoff]

so that's why i ended up running this per-file as that avoids those files that are on disk but not in head.

That is also why I wanted to pass a list of specific files, because what I do is run darker for each individual file that is returned from the git diff call in the first message.

So i need some way to avoid that fatal path error or some way to fix it.

[akaihola]

fatal: path 'xxxxxx' exists on disk, but not in 'master' is an error printed by Git. What Darker does in such cases is ignore the error, consider the whole file as changed, and run Black on all of its contents.

Does this match what you're seeing? Apart from maybe suppressing the error from Git, would you like to see Darker behave differently?

$ mkdir /tmp/not-in-branch

$ cd /tmp/not-in-branch
$ git init

$ touch README

$ git add README

$ git commit -m "Initial commit"
[master (root-commit) b41a90a] Initial commit
1 file changed, 0 insertions(+), 0 deletions(-)
create mode 100644 README

$ cat >test.py
print( "something to reformat" )

$ darker --diff . --revision=master
fatal: Path 'test.py' exists on disk, but not in 'master'.
--- test.py
+++ test.py
@@ -1 +1 @@
-print( "something to reformat" )
+print("something to reformat")

$ darker --revision=master .
fatal: Path 'test.py' exists on disk, but not in 'master'.

$ cat test.py
print("something to reformat")

[akaihola]

@markddavidoff, Darker 1.3.0 now hides the fatal error from Git. The above example looks like this after upgrading to Darker 1.3.0:

$ mkdir /tmp/not-in-branch

$ cd /tmp/not-in-branch
$ git init
Initialized empty Git repository in /tmp/not-in-branch/.git/

$ touch README

$ git add README

$ git commit -m "Initial commit"
[master (root-commit) d6c7f97] Initial commit
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 README

$ cat >test.py
print( "something to reformat" )

$ darker --diff . --revision=master
--- test.py	2021-09-06 18:46:29.418269 +0000
+++ test.py	2021-09-06 18:46:32.399737 +0000
@@ -1 +1 @@
-print( "something to reformat" )
+print("something to reformat")

$ darker --revision=master .

$ cat test.py
print("something to reformat")

[akaihola]

@markddavidoff, I'm closing this issue. Please send a note if you think this issue still exists.