PR#2049: distgit: Support for remote refs in committishs - copr/copr

copr / copr

#2049 distgit: Support for remote refs in committishs

Merged 2 years ago by praiskup. Opened 2 years ago by churchyard.

copr/ churchyard/copr remote_refs into main

distgit: Support for remote refs in committish

Miro Hrončok • 2 years ago

5729639

file modified

		`@@ -307,5 +307,12 @@`
		`if committish:`
		`# Do the checkout only if explicitly requested, otherwise build against`
		`# the default branch.`
		`+`
		`+ # First, fetch a remote reference if used.`
		`+ # This is a guesstimate for GitHub, GitLab and Pagure.`
		`+ if committish.startswith("refs/"):`
		`+ fetch_cmd = ['git', 'fetch', 'origin', '{0}:{0}'.format(committish)]`
		`+ run_cmd(fetch_cmd, cwd=repo_path)`
		`+`
		`checkout_cmd = ['git', 'checkout', committish]`
		`run_cmd(checkout_cmd, cwd=repo_path)`

churchyard commented 2 years ago

Fixes https://pagure.io/copr/copr/issue/2048

churchyard commented 2 years ago

This is not tested (I don't really know how) and I am not quite sure I like the RuntimeError thing. It would be great if run_cmd supported standard subprocess API (e.g. check=False) instead, but I wanted to submit an easy to review proof of concept first. Let me know if I should polish this.

zuul commented 2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

csdiff-pylint : FAILURE in 1m 53s

rebased onto 74b2679a13c5ff0d9826e3ede2354218459c4689

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 1m 52s

praiskup commented 2 years ago

This is not tested (I don't really know how)

Though this one IMO deserves a fix. Can a simple local git repo with additional
"pull request" remote side be easily created?

praiskup commented 2 years ago

I'd prefer to avoid using the f-strings in rpmbuild code, for now, to fix the CI.
Otherwise I don't object against this, but it would be nice to have a test-case.

churchyard commented 2 years ago

I'll check.

1 new commit added

fixup! dsitgit: Support for remote refs in committish

2 years ago

churchyard commented 2 years ago

I didn't know you use Python 2.7 on EPEL 7 :( Is that something I can help with?

(Pushed a fixup.)

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 2m 07s

praiskup commented 2 years ago

This is not tested (I don't really know how)

Though this one IMO deserves a fix. Can a simple local git repo with additional
"pull request" remote side be easily created?

s/deserves fix/deserves test/

Against what repo do you plan to use this feature?

churchyard commented 2 years ago

Against any repo at https://src.fedoraproject.org/rpms/

praiskup commented 2 years ago

Ping on the update requested in #2048 :-)

churchyard commented 2 years ago

It'll take some time.

1 new commit added

Always fetch committish when the name resembles a remote one

2 years ago

3 new commits added

Always fetch committish when the name resembles a remote one
fixup! dsitgit: Support for remote refs in committish
dsitgit: Support for remote refs in committish

2 years ago

rebased onto 5729639

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 2m 37s

churchyard commented 2 years ago

For testing purposes:

$ mkdir playground
$ cd playground/
$ mkdir bare_repo
$ cd bare_repo/
$ git init --bare
Initialized empty Git repository in .../playground/bare_repo/
bare_repo (BARE:main)]$ cd ..

$ git clone bare_repo/ cloned_repo
Cloning into 'cloned_repo'...
warning: You appear to have cloned an empty repository.
done.
$ cd cloned_repo/
[cloned_repo (main #)]$ git commit --allow-empty -m 'Intial commit'
[main (root-commit) 785b1b7] Intial commit
[cloned_repo (main)]$ git push -u origin main 
...

[cloned_repo (main)]$ git switch -c pr
Switched to a new branch 'pr'
[cloned_repo (pr)]$ git commit --allow-empty -m 'Additional commit'
[pr 6b942d1] Additional commit
[cloned_repo (pr)]$ git push -u origin pr
...
[cloned_repo (pr)]$ git rev-parse HEAD
6b942d17337b41640512e6117c55e139e635e59b

[cloned_repo (pr)]$ cd ..
$ rm cloned_repo/ -rf
$ cd bare_repo/

[bare_repo (BARE:main)]$ tree .git
.git [error opening dir]

0 directories, 0 files
[bare_repo (BARE:main)]$ tree 
.
├── branches
├── config
├── description
├── HEAD
├── hooks
│   └── ....
├── info
│   └── exclude
├── objects
│   └── ...
└── refs
    ├── heads
    │   ├── main
    │   └── pr
    └── tags

12 directories, 22 files

[bare_repo (BARE:main)]$ mkdir -p refs/pull/50/
[bare_repo (BARE:main)]$ mv refs/heads/pr refs/pull/50/head
[bare_repo (BARE:main)]$ cd ..

$ git clone bare_repo/ cloned_repo
Cloning into 'cloned_repo'...
done.
$ cd cloned_repo/
[cloned_repo (main)]$ git checkout refs/pull/50/head
error: pathspec 'refs/pull/50/head' did not match any file(s) known to git
[cloned_repo (main)]$ git fetch origin  refs/pull/50/head:refs/pull/50/head
From .../playground/bare_repo
 * [new ref]         refs/pull/50/head -> refs/pull/50/head
[cloned_repo (main)]$ git checkout refs/pull/50/head
Note: switching to 'refs/pull/50/head'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 6b942d1 Additional commit
[cloned_repo ((6b942d1...))]$

Side note: Should we set advice.detachedHead to false?

churchyard commented 2 years ago

Could use some pointers about where to plug a test for this and how.

Metadata Update from @praiskup:
- Pull-request tagged with: wip

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 2m 37s

praiskup commented 2 years ago

Side note: Should we set advice.detachedHead to false?

I think it can be done just for the git checkout, but it is a low-priority thing IMO.

Could use some pointers about where to plug a test for this and how.

I would use test_distgit_client.py, and use some helper method doing the
scripting you propose. There already is similar helpers git_init(), and
modifier git_origin_url().

Updating of the copr-distgit-client is needed (self.config), to also match
the given file:///clone/url ...?

praiskup commented 2 years ago

May we help somehow with this PR?

churchyard commented 2 years ago

I wasn't able to dedicate my time to go explore the tests. If you know how to convert my shell example to a test, please do, I would really appreciate it.

If you prefer me to do it, it'll take weeks.

praiskup commented 2 years ago

Ok, moving to: https://pagure.io/copr/copr/pull-request/2081

Pull-Request has been closed by praiskup

2 years ago

praiskup commented 2 years ago

Commit 7ce18bf fixes this pull-request

Pull-Request has been merged by praiskup

2 years ago

churchyard commented 2 years ago

Thank you!