Export the built tree as git repo #9

Merged
Ghost merged 24 commits from add_export into main 2022-11-02 14:17:24 +01:00
First-time contributor

The repo does not contain any files yet, it's just a list of empty commits building up the tree as git commits.

The repo does not contain any files yet, it's just a list of empty commits building up the tree as git commits.
Ghost added 3 commits 2022-10-31 12:29:33 +01:00
For smaller packages this doesn't matter as much, but for large ones
the N+1 queries can sum up badly
Walk the node tree and record the parents, then reverse the tree so we
can have the exact order in which to create git commits
Ghost added 1 commit 2022-10-31 16:31:39 +01:00
Ghost added 4 commits 2022-11-01 11:37:47 +01:00
This is technically incorrect but we need to handle them all the same anyway
Not using the database for that so that removing the repository directory will
automatically recreate it
Some cleanup of no longer used functions
Ghost added 1 commit 2022-11-01 11:45:48 +01:00
Ghost added 1 commit 2022-11-01 13:54:18 +01:00
The first merge we see in Factory determines if we keep the devel
commits in the factory chain or cut that branch.
Ghost added 1 commit 2022-11-01 18:45:21 +01:00
Ghost added 1 commit 2022-11-01 19:02:35 +01:00
Ghost added 1 commit 2022-11-01 19:31:01 +01:00
Ghost added 2 commits 2022-11-02 07:25:05 +01:00
Ghost added 5 commits 2022-11-02 08:59:42 +01:00
Author
First-time contributor

This outgrew the intended scope of this PR by a lot - but I enjoyed the silence of the bride day and parts of the public holidays (getting up at 6 and having until 12 until the family crawls out of bed is just too productive :)

This outgrew the intended scope of this PR by a lot - but I enjoyed the silence of the bride day and parts of the public holidays (getting up at 6 and having until 12 until the family crawls out of bed is just too productive :)
Ghost added 2 commits 2022-11-02 10:53:10 +01:00
aplanas reviewed 2022-11-02 11:38:01 +01:00
@ -115,3 +92,3 @@
if not args.repodir:
args.repodir = pathlib.Path(args.package)
args.repodir = pathlib.Path("repos/" + args.package)
Owner

if in L47 use a type pathlib.Path, like in L52, you will be sure that a path is given, and this line can be args.repodir = "repos" / args.package

if in L47 use a type pathlib.Path, like in L52, you will be sure that a path is given, and this line can be `args.repodir = "repos" / args.package`
Author
First-time contributor

But we need package for more than that

But we need package for more than that
Ghost marked this conversation as resolved
aplanas reviewed 2022-11-02 11:45:24 +01:00
@ -205,0 +211,4 @@
to_delete = []
if current_rev:
for entry in current_rev.files_list(db):
old_files[entry["name"]] = f"{entry['md5']}-{entry['size']}"
Owner

The last two lines: old_files = {e["name"]: f"{e['md5']}-{e['size']}" for e in current_rev.files_list(db)}

The last two lines: `old_files = {e["name"]: f"{e['md5']}-{e['size']}" for e in current_rev.files_list(db)}`
Author
First-time contributor

But current_rev is optional - no matter what you make out of it :)

But current_rev is optional - no matter what you make out of it :)
Owner

still the for two lines can be replaced

still the `for` two lines can be replaced
Author
First-time contributor

ah

ah
Ghost marked this conversation as resolved
aplanas reviewed 2022-11-02 11:57:28 +01:00
@ -202,6 +204,24 @@ class DBRevision:
self._files.sort(key=lambda x: x["name"])
return self._files
def calc_delta(self, db: DB, current_rev=None):
Owner

Reading the code seems that current_rev is not optional.

Reading the code seems that `current_rev` is not optional.
Author
First-time contributor

There is a if current_rev - and it's treated as empty dict if there is nothing before.

There is a `if current_rev` - and it's treated as empty dict if there is nothing before.
Owner

but not in the call?

but not in the call?
Author
First-time contributor

well, the =None is not needed as there is always a variable passed. I guess nowadays I can make it current_rev: Optional[DbRevision]. Would that be more of your liking?

well, the `=None` is not needed as there is always a variable passed. I guess nowadays I can make it `current_rev: Optional[DbRevision]`. Would that be more of your liking?
Owner

What I am wondering is the semantic of the function. calc_delta seems to be from some status to self. When the other status is not None the old_files is empty making all to download, when I would expect that none would be listed to download.

That makes me wonder if current_rev can be optional.

What I am wondering is the semantic of the function. `calc_delta` seems to be from some status to `self`. When the other status is not `None` the `old_files` is empty making all to download, when I would expect that none would be listed to download. That makes me wonder if `current_rev` can be optional.
Author
First-time contributor

I even added documentation! :)

self is the revision to download and current_rev is the state of the repository. If it's None, there is no state and everything needs to be downloaded.

I even added documentation! :) `self` is the revision to download and `current_rev` is the state of the repository. If it's `None`, there is no state and everything needs to be downloaded.
Ghost marked this conversation as resolved
aplanas reviewed 2022-11-02 11:57:42 +01:00
@ -205,0 +209,4 @@
old_files = dict()
to_download = []
to_delete = []
if current_rev:
Owner

(no optional)

(no optional)
Ghost marked this conversation as resolved
aplanas reviewed 2022-11-02 11:59:03 +01:00
@ -205,0 +213,4 @@
for entry in current_rev.files_list(db):
old_files[entry["name"]] = f"{entry['md5']}-{entry['size']}"
for entry in self.files_list(db):
if old_files.get(entry["name"], "") != f"{entry['md5']}-{entry['size']}":
Owner

old_files.get(entry["name"]) != ..., will return None if missing.

`old_files.get(entry["name"]) != ...`, will return `None` if missing.
Author
First-time contributor

true, but an old database afine developer doesn't like comparing None with strings :)

true, but an old database afine developer doesn't like comparing None with strings :)
Ghost marked this conversation as resolved
aplanas reviewed 2022-11-02 12:02:10 +01:00
@ -205,0 +215,4 @@
for entry in self.files_list(db):
if old_files.get(entry["name"], "") != f"{entry['md5']}-{entry['size']}":
logging.debug(f"Download {entry['name']}")
to_download.append(entry["name"])
Owner
to_download = [e["name"] for e in self.files_list(db) if old_files.get(e["name"]) != f"{e['md5']}-{e['size']}"]
logging.debug(f"Downloading {to_download}")
```python to_download = [e["name"] for e in self.files_list(db) if old_files.get(e["name"]) != f"{e['md5']}-{e['size']}"] logging.debug(f"Downloading {to_download}") ```
Author
First-time contributor

That's outputting an array, which I don't want.

That's outputting an array, which I don't want.
Owner

right, the last line can be replaced with a for, but this change is for the for and if lines

right, the last line can be replaced with a `for`, but this change is for the `for` and `if` lines
Author
First-time contributor

What would be the benefit? I don't see it as more readable

What would be the benefit? I don't see it as more readable
Ghost marked this conversation as resolved
Ghost added 1 commit 2022-11-02 13:29:41 +01:00
Author
First-time contributor

I extended the API documentation of the function now.

I extended the API documentation of the function now.
Ghost added 1 commit 2022-11-02 13:36:08 +01:00
Ghost merged commit 6ac0f90ec7 into main 2022-11-02 14:17:24 +01:00
aplanas reviewed 2022-11-02 14:19:47 +01:00
aplanas left a comment
Owner

(more later)

(more later)
@ -0,0 +3,4 @@
find interface classes preferable (java school)"""
def call(self, node, is_source):
pass
Owner
https://docs.python.org/3/library/abc.html
Author
First-time contributor

That confuses me more than it helps, but I'll dig into it :)

That confuses me more than it helps, but I'll dig into it :)
Ghost marked this conversation as resolved
@ -205,0 +219,4 @@
old_files.pop(entry["name"], None)
for entry in old_files.keys():
logging.debug(f"Delete {entry}")
to_delete.append(entry)
Owner

same

same
@ -0,0 +11,4 @@
def __str__(self) -> str:
p1_str = ""
if self.parent1:
p1_str = f" p1:{self.parent1.short_string()}"
Owner

p1_str = f" p1:{self.parent1.short_string()}" if self.parent1 else ""

`p1_str = f" p1:{self.parent1.short_string()}" if self.parent1 else ""`
Ghost marked this conversation as resolved
@ -0,0 +37,4 @@
if self.rebase_devel and node.parent and node.parent.merged_into:
self.add("devel", node.revision, node.parent.merged_into.revision)
return
if node.parent:
Owner

making it elif can remove the return

making it `elif` can remove the `return`
Ghost marked this conversation as resolved
@ -0,0 +123,4 @@
def limit_download(self, file):
if file.endswith(".spec") or file.endswith(".changes"):
return True
return False
Owner

return file.endswith((".spec", ".changes"))

But today it is better if all files are pathlib.Path objects, so you can do return file.suffix in (".spec", ".changes")

`return file.endswith((".spec", ".changes"))` But today it is better if all files are pathlib.Path objects, so you can do `return file.suffix in (".spec", ".changes")`
Ghost marked this conversation as resolved
Author
First-time contributor

Created https://gitea.opensuse.org/importers/git-importer/pulls/10 for the fixes - #9 was merged so that @nkrapp can base #2 on new code

Created https://gitea.opensuse.org/importers/git-importer/pulls/10 for the fixes - #9 was merged so that @nkrapp can base #2 on new code
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: importers/git-importer#9
No description provided.