Don't abort when `repos.json` and `repos_filtered.json` are not present #376

tpwo · 2025-05-22T19:13:13Z

These files are not present if all-repos exits unexpectedly, e.g. due to SIGINT.

Then, the error message:

output_dir should only contain repos.json, repos_filtered.json, and directories

is shown, as these two files are missing.

If they're recreated manually as empty, the error is gone and all-repos-clone works again, so the current check can be less strict.

These files are not present if `all-repos` exits unexpectedly, e.g. due to SIGINT. Then, the error message: output_dir should only contain repos.json, repos_filtered.json, and directories is shown, as these two files are missing. If they're recreated manually as empty, the error is gone and `all-repos-clone` works again.

asottile · 2025-05-23T20:35:05Z

all_repos/config.py

        if not (
-                contents >= REPOS_JSON_FILES and
                all(
                    os.path.isdir(os.path.join(output_dir, d))


yeah this is definitely not correct -- this breaks the safeguard that's in place

a better fix would be to avoid the situation which breaks the invariant (perhaps in all-repos-clone ?)

Got it. Now I back up both files when all-repos-clone is running and restore them if anything goes wrong (and a newer version of a file wasn't created).

I also added status messages of what's going on. What do you think?

…ot present" This reverts commit 662aaf9.

Top level try..except was added, so both files can be restored when all-repos-clone encounters any issues. Files are restored only if new version hasn't appeared.

asottile · 2025-05-24T19:08:49Z

probably best to write this as a context manager which only does something on error

also on error the state is ~sort of unknown so it is probably best to write empty json files rather than restoring the previous ones (as they may refer to removed repositories)

as CI indicates you'll also need to write some tests for this behaviour

tpwo · 2025-06-03T22:54:25Z

Done. What do you think?

asottile · 2025-06-08T20:29:13Z

all_repos/clone.py

+@contextlib.contextmanager
+def safe_write_json(paths: tuple[str, str]) -> Generator[None]:
+    try:
+        yield
+    except KeyboardInterrupt:
+        print('Aborting...')
+        for path in paths:
+            with open(path, 'w') as f:
+                f.write('{}')
+    except Exception:
+        for path in paths:
+            with open(path, 'w') as f:
+                f.write('{}')
+        raise
+
+


the word "safe" usually isn't a good name for a function (often that's exactly where to look for unsafe things) -- it should probably say something more specifically about what it does (specifically it restores blank files on failure)

I think you can also combine the two except blocks by using except BaseException (this will include SystemExit / KeyboardInterrupt / other crashes)

I also had another idea while thinking about this

what instead of attempting to restore these files, all-repos instead wrote a little marker file that's just always there? that would make this check a lot more consistent (does .all-repos exist? cool we're good to go!) but we'd also need to think about how to upgrade an existing setup to have that file (perhaps by using the same logic that's already there?)

what do you think of this other idea?

sorry again for taking so long on this. I have had zero free time recently and been back and forth across the country travelling :(

what do you think of this other idea?

Sounds cool, but let's make sure we're on the same page ;)

we'd also need to think about how to upgrade an existing setup to have that file (perhaps by using the same logic that's already there?)

By "the same logic", you mean _check_output_dir?

all-repos/all_repos/config.py

Lines 50 to 66 in ee3e0a0

def _check_output_dir(output_dir: str) -> None:

if os.path.exists(output_dir):

contents = set(os.listdir(output_dir))

if not contents: # empty dir is ok!

return

if not (

contents >= REPOS_JSON_FILES and

all(

os.path.isdir(os.path.join(output_dir, d))

for d in contents - REPOS_JSON_FILES

)

):

raise SystemExit(

'output_dir should only contain repos.json, '

'repos_filtered.json, and directories',

)

So the new flow in _check_output_dir would be:

1st check: "Empty dir is ok"

2nd check: Check only for .all-repos file, if it's present we're good to go

3rd check: (upgrade existing setup) use the current JSON files check from _check_output_dir, and create .all-repos file if other files are there

Fail otherwise

Do I get it right?

yeah I think that's right

Done, now it's a lot simpler :)

Coverage is at 100% w/o new tests, but do you think we need anything additional here?

Should I update readme as well? I see the directory structure is listed there. E.g.

all-repos/README.md

Lines 204 to 210 in ee3e0a0

output/

+--- repos.json

+--- repos_filtered.json

+--- {repo_key1}/

+--- {repo_key2}/

+--- {repo_key3}/

```

of course there should be a test -- otherwise how do we know it works and does what you intend to fix!

Hard to disagree ;)

I added two tests: one checking for the marker file, and the other checking if it's recreated after prior removal. This simulates the upgrade behavior for existing setups

…sent

…son-files

asottile · 2025-06-20T18:19:17Z

all_repos/clone.py

@@ -138,6 +138,7 @@ def main(argv: Sequence[str] | None = None) -> int:
        f.write(json.dumps(repos))
    with open(config.repos_filtered_path, 'w') as f:
        f.write(json.dumps(repos_filtered))
+    open(os.path.join(config.output_dir, '.all-repos'), 'w').close()


why is this here and the other place? surely it should be one or the other right?

tpwo mentioned this pull request May 22, 2025

Error on rerun when all-repos-clone exits unexpectedly #377

Open

tpwo referenced this pull request May 22, 2025

Raise an error when output_dir contents are unexpected

399495d

asottile reviewed May 23, 2025

View reviewed changes

tpwo added 4 commits May 24, 2025 00:23

Revert "Don't abort when repos.json and repos_filtered.json are n…

974371a

…ot present" This reverts commit 662aaf9.

Backup repos.json and repos_filtered.json allowing to restore them

f5856ca

Top level try..except was added, so both files can be restored when all-repos-clone encounters any issues. Files are restored only if new version hasn't appeared.

Remove correct files at the end

0709bc9

Update message

17d0587

tpwo added 3 commits June 3, 2025 23:53

Restore version from main

4808119

Use context manager

8f311db

Add tests for the new behavior

94e7e0e

asottile reviewed Jun 8, 2025

View reviewed changes

tpwo added 6 commits June 8, 2025 23:29

Restore the version from main

15a146c

feat: check for .all-repos file and create it if JSON files are pre…

6a5fe51

…sent

Merge remote-tracking branch 'upstream/main' into allow-to-recreate-j…

fe39c8e

…son-files

Add unit test for the new marker file behavior

5a9e286

Create marker file in the initial run and test if it is recreated

92fb30e

Add .all-repos to the presented structure of output dir

7fcb45d

asottile reviewed Jun 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Don't abort when `repos.json` and `repos_filtered.json` are not present #376

Don't abort when `repos.json` and `repos_filtered.json` are not present #376

Uh oh!

tpwo commented May 22, 2025 •

edited

Loading

Uh oh!

asottile May 23, 2025

Uh oh!

tpwo May 23, 2025

Uh oh!

asottile commented May 24, 2025

Uh oh!

tpwo commented Jun 3, 2025

Uh oh!

asottile Jun 8, 2025

Uh oh!

tpwo Jun 8, 2025

Uh oh!

asottile Jun 8, 2025

Uh oh!

tpwo Jun 8, 2025

Uh oh!

tpwo Jun 8, 2025

Uh oh!

asottile Jun 15, 2025

Uh oh!

tpwo Jun 15, 2025

Uh oh!

asottile Jun 20, 2025

Uh oh!

Uh oh!

	def _check_output_dir(output_dir: str) -> None:
	if os.path.exists(output_dir):
	contents = set(os.listdir(output_dir))
	if not contents: # empty dir is ok!
	return

	if not (
	contents >= REPOS_JSON_FILES and
	all(
	os.path.isdir(os.path.join(output_dir, d))
	for d in contents - REPOS_JSON_FILES
	)
	):
	raise SystemExit(
	'output_dir should only contain repos.json, '
	'repos_filtered.json, and directories',
	)

	output/
	+--- repos.json
	+--- repos_filtered.json
	+--- {repo_key1}/
	+--- {repo_key2}/
	+--- {repo_key3}/
	```

Uh oh!

Don't abort when repos.json and repos_filtered.json are not present #376

Are you sure you want to change the base?

Don't abort when repos.json and repos_filtered.json are not present #376

Uh oh!

Conversation

tpwo commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asottile commented May 24, 2025

Uh oh!

tpwo commented Jun 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Don't abort when `repos.json` and `repos_filtered.json` are not present #376

Don't abort when `repos.json` and `repos_filtered.json` are not present #376

tpwo commented May 22, 2025 •

edited

Loading