Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
fwag
force-pushed
the
feature/pr_forward
branch
from
49ecdc4 to
696583e
Compare
|
Why not "just" create PR from /pool#forward to /pool#slfo? This should be AGit created PR because we do not want branch following here anyway. And using consistent context allows for the PR to be updated by repeating the command if the source is updated. syntax and implementation: Ideally, this needs to also work via UI. |
osc/commands_git/pr_forward.py
Outdated
| # and fetch LFS objects for them from the appropriate remote. | ||
| lfs_remote = "source" if source_url else "upstream" | ||
| print(f"Fetching LFS objects from {lfs_remote} for incoming commits ...", file=sys.stderr) | ||
|
|
There was a problem hiding this comment.
If the repo from source to target is the same (different branch), there should be no need to move LFS objects around.
There was a problem hiding this comment.
In this case, there are two repositories: the user's forked repository and the pool repository. Normal users cannot create a PR in pool because they don't have the right permissions.
|
hi @AdamMajer, |
|
There are multiple problems with this from an UX point of view.
|
There was a problem hiding this comment.
I initially asked the question in the slack channel. I used this today to submit factory to slfo-main for gnuplot. Thank for writing this so quickly, it really reduces the overhead and potential mistakes associated with it. I added a few code comments with general thoughts and suggestions.
Overall I wish there was a solution which I wouldn't need to clone the repo for, but this is a great improvement. Thank you!
There is a bunch of trailing white space in the file.
| # Fork if not exists | ||
| print(f"Checking fork for {upstream_owner}/{upstream_repo} ...", file=sys.stderr) | ||
| try: | ||
| fork_obj = gitea_api.Fork.create(self.gitea_conn, upstream_owner, upstream_repo) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
If your "factory" branch is out of sync between your local git and the remote fork, git will block your push. That's why I added the --force parameter. In any case, using a temporary branch is a good idea.
|
|
||
| print("Creating Pull Request ...", file=sys.stderr) | ||
| try: | ||
| pr_obj = gitea_api.PullRequest.create( |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
ok added --edit, commit message is automatically created from PR title and PR description
| if not os.path.exists(repo_dir): | ||
| os.makedirs(repo_dir) | ||
| else: | ||
| repo_dir = tempfile.mkdtemp(prefix="git-obs-forward-") |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
+1 to your feedback, but it cleans up the working directory, except when you pass |
|
Regarding the white space: From 74f8f8aba15141b73095aca7aa1a2801a23e5558 Mon Sep 17 00:00:00 2001
From: Jan Fooken <jan.fooken@suse.com>
Date: Tue, 17 Feb 2026 16:47:55 +0100
Subject: [PATCH] Remove whitespace
---
osc/commands_git/pr_forward.py | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/osc/commands_git/pr_forward.py b/osc/commands_git/pr_forward.py
index 176dd1c3..6a0ba653 100644
--- a/osc/commands_git/pr_forward.py
+++ b/osc/commands_git/pr_forward.py
@@ -88,7 +88,7 @@ class PullRequestForwardCommand(osc.commandline_git.GitObsCommand):
upstream_repo_obj = gitea_api.Repo.get(self.gitea_conn, upstream_owner, upstream_repo)
fork_repo_obj = gitea_api.Repo.get(self.gitea_conn, fork_owner, fork_repo)
- upstream_url = upstream_repo_obj.ssh_url
+ upstream_url = upstream_repo_obj.ssh_url
fork_url = fork_repo_obj.ssh_url # Prefer SSH for push if possible
# Setup Workdir
@@ -149,7 +149,7 @@ class PullRequestForwardCommand(osc.commandline_git.GitObsCommand):
git.add_remote("source", source_url)
else:
git._run_git(["remote", "set-url", "source", source_url])
-
+
print("Fetching source ...", file=sys.stderr)
git.fetch("source")
source_ref = f"source/{source_branch}"
@@ -168,11 +168,11 @@ class PullRequestForwardCommand(osc.commandline_git.GitObsCommand):
# and fetch LFS objects for them from the appropriate remote.
lfs_remote = "source" if source_url else "upstream"
print(f"Fetching LFS objects from {lfs_remote} for incoming commits ...", file=sys.stderr)
-
+
try:
# Get list of commits unique to source_branch that are not in target_branch
commits = git._run_git(["rev-list", f"{lfs_remote}/{source_branch}", f"^{lfs_remote}/{target_branch}"]).splitlines()
-
+
if commits:
print(f" * Found {len(commits)} commits to fetch LFS objects for.", file=sys.stderr)
# Loop through commits as requested
@@ -204,7 +204,7 @@ class PullRequestForwardCommand(osc.commandline_git.GitObsCommand):
# Clean files not in Source
print("Cleaning files not present in source ...", file=sys.stderr)
-
+
# Get list of files in source (recurse)
source_files_output = git._run_git(["ls-tree", "-r", "--name-only", source_ref])
source_files = set(source_files_output.splitlines())
@@ -214,7 +214,7 @@ class PullRequestForwardCommand(osc.commandline_git.GitObsCommand):
head_files = set(head_files_output.splitlines())
files_to_remove = list(head_files - source_files)
-
+
if files_to_remove:
print(f"Removing {len(files_to_remove)} files not present in {source_branch} ...", file=sys.stderr)
# Batching git rm to avoid command line length limits
@@ -222,7 +222,7 @@ class PullRequestForwardCommand(osc.commandline_git.GitObsCommand):
for i in range(0, len(files_to_remove), chunk_size):
chunk = files_to_remove[i : i + chunk_size]
git._run_git(["rm", "-f", "--"] + chunk)
-
+
git.commit(f"Clean files not present in {source_branch}")
else:
print("No extra files to clean.", file=sys.stderr)
@@ -245,7 +245,7 @@ class PullRequestForwardCommand(osc.commandline_git.GitObsCommand):
# Create PR
title = args.title or f"Forward {source_branch} to {target_branch}"
description = args.description or f"Automated forward of {source_branch} to {target_branch} using git-obs."
-
+
if args.dry_run:
print(f"[DRY RUN] Would create PR: {title}", file=sys.stderr)
return
--
2.53.0 |
|
Don't really have a repo to test the new changes on, but the changes look good to me. Linking the branch name with the commit sha means that you can't reuse an already open PR, which I know my colleagues did in the past, but I'm not sure whether that's wanted anyways, nor am I a maintainer here :^) |
| action="store_true", | ||
| help="Allow merging unrelated histories", | ||
| ) | ||
|
|
There was a problem hiding this comment.
Please don't forget to add the "--merge" as an option. Creating a PR directly from the source branch to target branch without a merge commit on top should be default.
|
|
||
| description = "\n".join(lines).strip() | ||
|
|
||
| # Merge Source Branch (Theirs) |
There was a problem hiding this comment.
like above, all this IFF --merge only please.
osc/commands_git/pr_forward.py
Outdated
| merge_cmd = ["merge", "-X", "theirs"] | ||
| if args.allow_unrelated_histories: | ||
| merge_cmd.append("--allow-unrelated-histories") | ||
| merge_cmd.extend(["-m", commit_message, source_ref]) | ||
| git._run_git(merge_cmd) | ||
| except Exception as e: | ||
| print(f"{tty.colorize('ERROR', 'red,bold')}: Merge failed: {e}", file=sys.stderr) | ||
| sys.exit(1) | ||
|
|
||
| # Clean files not in Source | ||
| print("Cleaning files not present in source ...", file=sys.stderr) | ||
|
|
||
| # Get list of files in source (recurse) | ||
| source_files_output = git._run_git(["ls-tree", "-r", "--name-only", source_ref]) | ||
| source_files = set(source_files_output.splitlines()) | ||
|
|
||
| # Get list of files in current HEAD | ||
| head_files_output = git._run_git(["ls-tree", "-r", "--name-only", "HEAD"]) | ||
| head_files = set(head_files_output.splitlines()) | ||
|
|
||
| files_to_remove = list(head_files - source_files) | ||
|
|
||
| if files_to_remove: | ||
| print(f"Removing {len(files_to_remove)} files not present in {source_branch} ...", file=sys.stderr) | ||
| # Batching git rm to avoid command line length limits | ||
| chunk_size = 100 | ||
| for i in range(0, len(files_to_remove), chunk_size): | ||
| chunk = files_to_remove[i: i + chunk_size] | ||
| git._run_git(["rm", "-f", "--"] + chunk) | ||
|
|
||
| git.commit(f"Clean files not present in {source_branch}") | ||
| else: | ||
| print("No extra files to clean.", file=sys.stderr) |
There was a problem hiding this comment.
git merge --no-ff --no-commit -X theirs PR_HEAD_SHA
git read-tree -m PR_HEAD_SHA
git commit -m ..
git clean -fxd
The git-read-tree write into the index the info from that other Head and the extra files from current checkout become untracked. Hence need to clean after Merge. But then you no longer need to compare files ... especially since that would ahve to be done recursively.
There was a problem hiding this comment.
git read-tree -u -m <ref> includes the clean step
There was a problem hiding this comment.
git read-tree -u --reset <ref> may be better option, as no merging is done.
I guess we are trying to emulate git merge -s theirs while it doesn't exist.
I may be missing something, but that's not always correct, is it? My "factory" and "slfo-main" branches tend to have subtle differences, especially a different |
And how do you expect to merge Factory into other branch if the source revision is not updated? |
It feels wrong to me to have a I ususally have separate branches for slfo-main and factory in the source repo. More often than not, they point to the same commit, but they don't have to. |
This is for the case when you want a branch (git reference) other than factory to point to the same commit, or atleast same content as factory. When that's not what you are looking for you should look elsewhere. |
I was confused by the description of the PR, which mentions "theirs" strategy for merging. But there is no "theirs" strategy, there's just "ort" with "theirs" option, and that's what the PR uses. I guess this would "work" for my use case. The different The only problem being that If the two source repo commits were not pointing to the same sources, the merge might cause an inconsistent state, as the And that's why I am not convinced that the automatic merge is a good idea. This functionality makes certain assumptions about how a user has set up his branches in the source repository, and (AFAICS) it contains no safeguards to make sure that this is actually the case. Thus it has the potential to create inconsistent code unless users understand the prerequisites of using the command exactly, which I wouldn't assume. |
|
How would it cause out of sync result? The merge strategy is the take everything from the new sources, discarding everything in the current sources. |
No. |
|
So it's implemented incorrectly. |
What do you mean, If you want to force-merge, overwriting everything, you need to switch to the other ("theirs") branch, which then becomes "ours", and run |
|
That will create a backwards merge. Not that intelligible history is a concern in git workflow. The commit graphs in the pool repositories looks like barbed wire. |
Is it important if its backward or forward? That just determines on which side of the merge the history appears. I guess we have to live with the fact that git doesn't support a full "theirs" merge operation. |
|
I guess what you could also do is a It wouldn't work for me though, because I would want to preserve my change in |
Good catch here. Thanks! git read-tree solves this issues which is what is proposed here. |
|
We probably could use |
|
This PR did not consider Martin's use case. The purpose of this PR was mainly to report the exact state of one branch in another. @AdamMajer, the commands you suggested do not solve @mwilck's usecase. If there is a conflict, the changes from the other branch win as expected. @mwilck you can always report the change with a later commit. |
|
What are the 'wrong incentives' you envision here? It's not clear to me. |
|
Force-merging blindly. Force-forwarding from Factory to somewhere else without taking a closer look. |
|
I can't say about other users. My use case is that I forward simple packages I maintain to different OS releases. I do not look at them because I know I do not have separate content for different releases, the package should be the same everywhere where it's maintained. The other use case is forwarding a version of a package I have already built and tested for the release in question. I have already looked at it at that point. |
|
And I don't want some merge artifact in either of these cases, I want exactly the content I am forwarding.. |
|
I was checking how the individual scenarios work with real-life data and ended up with a script I'm pasting here for reference: (the script is for development purposes and better understanding how the underlying git commands work, it's not supposed to replace this PR) Considerations:
|
|
For git workflow we want to default to merging the upstream. That is for submit from Factory to Leap 16.0 either the Leap 16 branch can be fast-forwarded to Factory or a merge commit should be created. So the first thing would be to try the FF merge. for pool/package and its fork user/package
|
|
Also this might be a special case: since it's merging what is already in pool it does not need new LFS uploads, and as a result AGit might work. Reportedly the problem is writing LFS objects but for a merge inside pool no new LFS objects are needed. openSUSE/openSUSE-git#97 It would not work for submissions from develproject, though. |
|
https://src.opensuse.org/git-workflow/autogits/pulls/131 So, keep in mind, we do not always want to merge. It's project dependent. When Leap swtiches from 16.0 to 16.1 and has some maintenacne updates, PR from factory should probably switch history to factory branch instead of merging it on top. Merging decision should be maintainer and project specific here, so ideally, not by default, and one size fits all approach. Source remote URL is not guaranteed to stay as-is. Only in pool. But if we have it in pool, then it's already in history, which means commit message just duplicates the information for no reason.. The only reason to have a commit message like this is when reading in a tree based on another commit and not doing a merge. But keeping history is cheap, so why discard it? |
|
Switching history is not implemented, it must be a merge with current git workflow. |
Hence my linked PR, that allows such mode of operation 😉 |
…staged or unstaged changes in the current working copy
dmach
force-pushed
the
feature/pr_forward
branch
from
5fd05e4 to
02b249e
Compare
|
@fwag I've pushed a handful of commits on top of yours. Open questions:
|
…e the configured ssh key
dmach
force-pushed
the
feature/pr_forward
branch
from
02b249e to
548b50f
Compare
|
|
||
| # Get clone URLs | ||
| upstream_repo_obj = gitea_api.Repo.get(self.gitea_conn, upstream_owner, upstream_repo) | ||
| fork_repo_obj = gitea_api.Repo.get(self.gitea_conn, fork_owner, fork_repo) |
Check notice
Code scanning / CodeQL
Unused local variable Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 15 hours ago
To fix an unused local variable, either remove the variable binding entirely (if the value is not needed) or rename it to a clearly “unused” name (if the call has important side effects but the return value is irrelevant). This avoids confusion for readers and satisfies the static analysis rule.
In this case, we should preserve the call to gitea_api.Repo.get in case it has side effects, but bind the result to a conventional unused variable name. The minimal, behavior‑preserving change is to change fork_repo_obj to _fork_repo_obj (or similar) on line 92 and leave the rest of the code untouched.
Concretely, in osc/commands_git/pr_forward.py, locate the line:
fork_repo_obj = gitea_api.Repo.get(self.gitea_conn, fork_owner, fork_repo)and change it to:
_fork_repo_obj = gitea_api.Repo.get(self.gitea_conn, fork_owner, fork_repo)No new imports, methods, or additional definitions are required.
| @@ -89,7 +89,7 @@ | ||
|
|
||
| # Get clone URLs | ||
| upstream_repo_obj = gitea_api.Repo.get(self.gitea_conn, upstream_owner, upstream_repo) | ||
| fork_repo_obj = gitea_api.Repo.get(self.gitea_conn, fork_owner, fork_repo) | ||
| _fork_repo_obj = gitea_api.Repo.get(self.gitea_conn, fork_owner, fork_repo) | ||
|
|
||
| upstream_url = upstream_repo_obj.ssh_url | ||
|
|
Sounds like a good idea. Autodetection is good but it may not always produce the expected result. Note also the
I think it is required to copy over all LFS objects to produce correct PR.
Probably yes, because bots keep creating unrelated histories for us. Maybe make it optional and not the default so that when the bots are fixed and it's not expected anymore people see it as a warning flag. ie. pass through both the --alow-unrelated-histories flag and the error when it's missing or an equivalent.
I would suggest this is out of scope. Opening the PR is a gitea-specific feature and cannot be done when the source is not in gitea.
Submit form develproject to SLFO is such case. |
6 participants
Automates the workflow of forwarding sources from one branch to another (e.g., Factory to Leap/SLFO) via a PR.
The command handles: