Summary

Fixed RepositoryTool patching of files in uncreated directories

Review Request #10247 — Created Oct. 20, 2018 and submitted Nov. 10, 2018, 7:26 p.m.

Information

Owner

alextechcc

Repository

ReviewBot

Branch

master

Bugs

Depends On

Commit

8d24901...

Reviewers

Groups

reviewbot, students

People

Description

When a RepositoryTool is applying the review's patches to the cloned
repository, it simply writes using open(p, 'wb'). This does not create
directories that the patch implies.

This leads to the write failing with:
IOError: [Errno 2] No such file or directory: u'dir/file'

This patch creates directories for each file if the path does not exist
already.

Testing Done

Replciated the error by posting a review for a file in a new directory,
applied patch, and RepositoryTool successfully runs on patched files.

Issues

Description	From	Last Updated
F401 'os.path' imported but unused	reviewbot	Oct. 20, 2018, 12:38 a.m.
F811 redefinition of unused 'path' from line 8	reviewbot	Oct. 20, 2018, 12:45 a.m.
E722 do not use bare except'	reviewbot	Oct. 20, 2018, 12:45 a.m.
Blank line between these.	brennie	Oct. 25, 2018, 10:43 p.m.
This doesn't always create directories -- it ensures they exist, so let's call this ensure_directories.	brennie	Oct. 25, 2018, 11:08 p.m.
No space after "	brennie	Oct. 25, 2018, 10:45 p.m.
The path for which directories should be created if they don't exist.	brennie	Oct. 25, 2018, 10:45 p.m.
Blank line between here.	brennie	Oct. 25, 2018, 10:45 p.m.
E722 do not use bare except'	reviewbot	Oct. 20, 2018, 12:46 a.m.
We never want to use bare except. This can catch things that should never be caught, like KeyboardInterrupt or RuntimeError. …	brennie	Oct. 25, 2018, 11:08 p.m.
It seems like we'd probably want to surface the error so that the caller can bail out.	david	Oct. 31, 2018, 12:10 a.m.
Function name would be better as ensure_directories_exist	ammar	Oct. 31, 2018, 10:30 p.m.

flake8 failed.

JSHint passed.

flake8

bot/reviewbot/utils/filesystem.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F401 'os.path' imported but unused
```
bot/reviewbot/utils/filesystem.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F811 redefinition of unused 'path' from line 8
```
bot/reviewbot/utils/filesystem.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E722 do not use bare except'
```

Change Summary:

Removed unneccessary import

Commit:

c89f67ab14f7da66647f24441492effb299a19cd

68b16e6d2c54cd6f564379b0aced9d654dadacd3

Diff:

Revision 2 (+17 -1)

Show changes

	bot/reviewbot/tools/__init__.py
	bot/reviewbot/utils/filesystem.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

bot/reviewbot/utils/filesystem.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E722 do not use bare except'
```

bot/reviewbot/tools/__init__.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line between these.
```

bot/reviewbot/utils/filesystem.py (Diff revision 2)

The issue has been resolved. Show all issues

This doesn't always create directories -- it ensures they exist, so let's call this ensure_directories.

bot/reviewbot/utils/filesystem.py (Diff revision 2)
The issue has been resolved. Show all issues
```
No space after "
```
bot/reviewbot/utils/filesystem.py (Diff revision 2)
The issue has been resolved. Show all issues
```
The path for which directories should be created if they don't exist.
```
bot/reviewbot/utils/filesystem.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line between here.
```

bot/reviewbot/utils/filesystem.py (Diff revision 2)

The issue has been resolved. Show all issues

We never want to use bare except. This can catch things that should never be caught, like KeyboardInterrupt or RuntimeError. Instead, this needs to specify the exact exception we expect, which should be OSError.

alextechcc

Oct. 24, 2018, 2:38 a.m.

Should I fix the rest of the instances of this in all related files?

brennie

Oct. 24, 2018, 3:24 a.m.
```
No, that isn't related to your change.
```

Change Summary:

Renamed create_dirs, formatting, and catch OSError instead of bare except.

Commit:

68b16e6d2c54cd6f564379b0aced9d654dadacd3

598db0e96d2065bc5f9392d3f8206d1a4de1f0f9

Diff:

Revision 3 (+20 -1)

Show changes

	bot/reviewbot/tools/__init__.py
	bot/reviewbot/utils/filesystem.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

bot/reviewbot/utils/filesystem.py (Diff revision 3)

The issue has been resolved. Show all issues

It seems like we'd probably want to surface the error so that the caller can bail out.

Change Summary:

Converted to enforcing absolute paths.

Commit:

598db0e96d2065bc5f9392d3f8206d1a4de1f0f9

33d3cc806cc5c15ecb538468fb93541ca20ecc8d

Diff:

Revision 4 (+28 -1)

Show changes

	bot/reviewbot/tools/__init__.py
	bot/reviewbot/utils/filesystem.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

bot/reviewbot/utils/filesystem.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Function name would be better as ensure_directories_exist
```

Change Summary:

Renamed ensure_directories

Commit:

33d3cc806cc5c15ecb538468fb93541ca20ecc8d

8d249016336d5958db2d3de9f60ce683e1e2a941

Diff:

Revision 5 (+28 -1)

Show changes

	bot/reviewbot/tools/__init__.py
	bot/reviewbot/utils/filesystem.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to master (e092554)