Skip to content

import_srpm: make most cli parameters optional #749

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

import_srpm: make most cli parameters optional #749

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 22 additions & 14 deletions scripts/import_srpm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ def call_process(args):
def main():
parser = argparse.ArgumentParser(description='Imports the contents of a source RPM into a git repository')
parser.add_argument('source_rpm', help='local path to source RPM')
parser.add_argument('repository', help='local path to the repository')
parser.add_argument('parent_branch', help='git parent branch from which to branch')
parser.add_argument('branch', help='destination branch')
parser.add_argument('repository', default='./', nargs='?', help='local path to the repository')
parser.add_argument('parent_branch', nargs='?', help='git parent branch from which to branch')
parser.add_argument('branch', nargs='?', help='destination branch')
Comment on lines -14 to +16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could take this opportunity to use options for those? IIUC, specifying branch cannot be done without passing the first 2 ones (and parent_branch is indeed needed much less often than branch)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not using those parameters, so I can't really say.
@stormi ?

Comment on lines -14 to +16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default behavior when branch is not passed is not obvious. Is it a good idea to have that one optional?

Copy link
Member Author

@glehmann glehmann Aug 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It uses the branch currently checked out in the repository.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that, though:

  • I kind of dislike the idea of making unchecked assumptions about the checked out branch ... very much especially if we advise people to --push, and the previous behaviour, or even the new one when using --push, is to leave master being the current branch when done
  • if we still want it (and fix that inconsistency), it should appear in the param doc

Further more, since the idea is to get it run automatically soon, we don't gain much by making it optional.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a good idea to make the branch optional. And I don't think it's really useful to make the current path the default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get it that the CLI interface for this command is not optimal, but that's really one command for which we don't want people to spare some thinking before executing it. Hence the positional mandatory arguments.

One argument that is confusing is the "parent branch" argument, which could be removed if we updated the docs so that people created the branch by hand when it didn't exist yet. "parent branch" is an argument that could be a candidate for being optional. But then I think it would be more confusing than leaving it mandatory. If you're confused by the parameter, then you don't really know what the script does, so better not use it.

The merge to master option should go. I mostly used it back when I didn't go through a PR for such merges, and it is now useless.

Coming back to the initial branch after doing the work would be nicer than going to master.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, when running git push, git makes an unchecked assumption that this is currently the right branch. It doesn't force us to provide the branch name, even though we could push something wrong. It looks the same to me.

But yes, let's run it automatically, and we won't have to provide anything on the command line 🙂

parser.add_argument('tag', nargs='?', help='tag')
parser.add_argument('-v', '--verbose', action='count', default=0)
parser.add_argument('-p', '--push', action='store_true', help='pull and push')
Expand Down Expand Up @@ -56,8 +56,14 @@ def main():

if args.push:
call_process(['git', 'fetch'])
call_process(['git', 'checkout', args.parent_branch])
if args.push:
if args.parent_branch:
call_process(['git', 'checkout', args.parent_branch])
if (
args.push
# only pull if the branch is already tracked
and subprocess.call(['git', 'rev-parse', '--abbrev-ref', '--symbolic-full-name', '@{u}'],
stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) == 0
):
call_process(['git', 'pull'])

print(" removing everything from SOURCES and SPECS...")
Expand Down Expand Up @@ -91,10 +97,11 @@ def main():
open(os.path.join('SOURCES', "%s.deleted-by-XCP-ng.txt" % f), 'w').write(deletemsg)
deleted.append(f)

if subprocess.call(['git', 'rev-parse', '--quiet', '--verify', args.branch]) != 0:
call_process(['git', 'checkout', '-b', args.branch])
else:
call_process(['git', 'checkout', args.branch])
if args.branch:
if subprocess.call(['git', 'rev-parse', '--quiet', '--verify', args.branch]) != 0:
call_process(['git', 'checkout', '-b', args.branch])
else:
call_process(['git', 'checkout', args.branch])
call_process(['git', 'add', '--all'])

print(" committing...")
Expand All @@ -116,20 +123,21 @@ def main():
if args.tag is not None:
call_process(['git', 'tag', args.tag])

branch = args.branch or subprocess.check_output(['git', 'rev-parse', '--abbrev-ref', 'HEAD']).decode().strip()
# push to remote
if args.push:
call_process(['git', 'push', '--set-upstream', 'origin', args.branch])
call_process(['git', 'push', '--set-upstream', 'origin', branch])
if args.tag is not None:
call_process(['git', 'push', 'origin', args.tag])

print(" switching to master before leaving...")

call_process(['git', 'checkout', 'master'])
if args.branch:
print(" switching to master before leaving...")
call_process(['git', 'checkout', 'master'])
Comment on lines -125 to +135
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That "switching to master before leaving" is indeed a definitely bad idea in itself. It could make sense to restore what the current branch is in the end, OTOH.
But then, there is that "merging to master" below, which then does require switching to master (which is not necessarily "switching back").

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer that too. But so far, I've done the branch management by hand, so I can't really say.


# merge to master if needed
if args.push and args.master:
print(" merging to master...")
call_process(['git', 'push', 'origin', '%s:master' % args.branch])
call_process(['git', 'push', 'origin', '%s:master' % branch])
call_process(['git', 'pull'])


Expand Down