[PATCH] Enable gerrit/gitlab/github refspecs

[Drew Reed]

Support using Gerrit patchsets in repo refspecs in the form refs/changes/...
Supoort using GitLab merge requests in repo refspecs in the form refs/merge-requests/...
Support using GitHub pull requests in the repo refspecs in the form refs/pull/...

Signed-off-by: Drew Reed <drew...@arm.com>
---
kas/repos.py | 31 +++++++++++++++++++++++--------
1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/kas/repos.py b/kas/repos.py
index edd46ee..717d154 100644
--- a/kas/repos.py
+++ b/kas/repos.py
@@ -179,7 +179,6 @@ class RepoImpl(Repo):
cwd=get_context().kas_work_dir)
if retc == 0:
logging.info('Repository %s cloned', self.name)
- return retc

# Make sure the remote origin is set to the value
# in the kas file to avoid surprises
@@ -212,7 +211,8 @@ class RepoImpl(Repo):
# Try to fetch if refspec is missing or if --update argument was passed
(retc, output) = await run_cmd_async(self.fetch_cmd(),
cwd=self.path,
- fail=False)
+ fail=False,
+ liveupdate=False)
if retc:
logging.warning('Could not update repository %s: %s',
self.name, output)
@@ -336,6 +336,10 @@ class GitRepo(RepoImpl):
Provides the git functionality for a Repo.
"""

+ def remove_ref_prefix(self, refspec):
+ ref_prefix = 'refs/'
+ return refspec[refspec.startswith(ref_prefix) and len(ref_prefix):]
+
def add_cmd(self):
return ['git', 'add', '-A']

@@ -350,29 +354,40 @@ class GitRepo(RepoImpl):
'-m', 'msg']

def contains_refspec_cmd(self):
- return ['git', 'cat-file', '-t', self.refspec]
+ return ['git', 'cat-file', '-t', self.remove_ref_prefix(self.refspec)]

def fetch_cmd(self):
- return ['git', 'fetch']
+ cmd = ['git', 'fetch']
+ if (self.refspec.startswith('refs/changes/')
+ or self.refspec.startswith('refs/pull/')
+ or self.refspec.startswith('refs/merge-requests/')):
+ cmd.extend(['origin',
+ '+' + self.refspec
+ + ':refs/remotes/origin/'
+ + self.remove_ref_prefix(self.refspec)])
+
+ return cmd

def is_dirty_cmd(self):
return ['git', 'status', '-s']

def resolve_branch_cmd(self):
return ['git', 'rev-parse', '--verify', '-q',
- 'origin/{refspec}'.format(refspec=self.refspec)]
+ 'origin/{refspec}'.
+ format(refspec=self.remove_ref_prefix(self.refspec))]

def checkout_cmd(self, desired_ref, branch):
- cmd = ['git', 'checkout', '-q', desired_ref]
+ cmd = ['git', 'checkout', '-q', self.remove_ref_prefix(desired_ref)]
if branch:
- cmd.extend(['-B', self.refspec])
+ cmd.extend(['-B', self.remove_ref_prefix(self.refspec)])
if get_context().force_checkout:
cmd.append('--force')
return cmd

def prepare_patches_cmd(self):
return ['git', 'checkout', '-q', '-B',
- 'patched-{refspec}'.format(refspec=self.refspec)]
+ 'patched-{refspec}'.
+ format(refspec=self.remove_ref_prefix(self.refspec))]

def apply_patches_file_cmd(self, path):
return ['git', 'apply', path]
--
2.24.3 (Apple Git-128)

[Jan Kiszka]

On 25.06.21 11:31, Drew Reed wrote:
> Support using Gerrit patchsets in repo refspecs in the form refs/changes/...
> Supoort using GitLab merge requests in repo refspecs in the form refs/merge-requests/...
> Support using GitHub pull requests in the repo refspecs in the form refs/pull/...
>

Thanks for these enhancements. Just please elaborate more on what is
needed to enable that, ie. what makes those formats different from what
is so far supported. And also provide the use case along (the "why do we
want that?").

> Signed-off-by: Drew Reed <drew...@arm.com>
> ---
> kas/repos.py | 31 +++++++++++++++++++++++--------
> 1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/kas/repos.py b/kas/repos.py
> index edd46ee..717d154 100644
> --- a/kas/repos.py
> +++ b/kas/repos.py
> @@ -179,7 +179,6 @@ class RepoImpl(Repo):
> cwd=get_context().kas_work_dir)
> if retc == 0:
> logging.info('Repository %s cloned', self.name)
> - return retc
>

Why that?

> # Make sure the remote origin is set to the value
> # in the kas file to avoid surprises
> @@ -212,7 +211,8 @@ class RepoImpl(Repo):
> # Try to fetch if refspec is missing or if --update argument was passed
> (retc, output) = await run_cmd_async(self.fetch_cmd(),
> cwd=self.path,
> - fail=False)
> + fail=False,
> + liveupdate=False)

And why that?

Test cases are missing, though I'm not sure how easy they could be
created for the individual scenarios.

Jan

--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

[Drew Reed]

By default git uses the default refspec of "+refs/heads/*:refs/remotes/origin/*" when fetching from a remote repository and it restricts the available references to ones that exist under the refs/heads tree on the server. (This is kind of explained here if my explanation is hard to follow: https://git-scm.com/book/en/v2/Git-Internals-The-Refspec)

Gerrit/GitlLab and Github use references outside of the refs/heads tree when working with patchsets(refs/changes)/merge-requests(refs/merge-requests) and pull's(refs/pull) respectively. So to enable a local repo to checkout these references you have to expand the scope of the fetch. This can be done by either adding additional fetch refspecs into the local .git/config file or by manually specifying it on the fetch command line. For this patch I chose to use the fetch command line and restrict the additional reference scope the actual patchset being requested so as not to waste time fetching what could be a huge number of references.

The why is so that the repos referenced by kas can be "in development". By this I mean patchsets/merge-requests and pull requests that haven't yet been merged onto a branch but would like to be reference to enable per merge testing to be performed.

Drew

On 25/06/2021, 12:02, "Jan Kiszka" <jan.k...@siemens.com> wrote:

On 25.06.21 11:31, Drew Reed wrote:
> Support using Gerrit patchsets in repo refspecs in the form refs/changes/...
> Supoort using GitLab merge requests in repo refspecs in the form refs/merge-requests/...
> Support using GitHub pull requests in the repo refspecs in the form refs/pull/...
>

Thanks for these enhancements. Just please elaborate more on what is
needed to enable that, ie. what makes those formats different from what
is so far supported. And also provide the use case along (the "why do we
want that?").

> Signed-off-by: Drew Reed <drew...@arm.com>
> ---
> kas/repos.py | 31 +++++++++++++++++++++++--------
> 1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/kas/repos.py b/kas/repos.py
> index edd46ee..717d154 100644
> --- a/kas/repos.py
> +++ b/kas/repos.py
> @@ -179,7 +179,6 @@ class RepoImpl(Repo):
> cwd=get_context().kas_work_dir)
> if retc == 0:
> logging.info('Repository %s cloned', self.name)
> - return retc
>

Why that?

We need to ensure the fetch command is run or the first call or kas would fail as only the clone command would be run and so the requested reference would not be found. Unfortunately you can't add to the default refspec during a clone only during a fetch.

> # Make sure the remote origin is set to the value
> # in the kas file to avoid surprises
> @@ -212,7 +211,8 @@ class RepoImpl(Repo):
> # Try to fetch if refspec is missing or if --update argument was passed
> (retc, output) = await run_cmd_async(self.fetch_cmd(),
> cwd=self.path,
> - fail=False)
> + fail=False,
> + liveupdate=False)

And why that?

The fetch outputs information on the references fetched and it seemed different to the existing output, It can be safely removed I believe but the log will be noisier.

Yes, I don't know how to create ones without example changes let pending in gerrit/gitlab and github based servers. I'm open to suggestions on how to test it?

Jan

--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[Drew Reed]

Thinking about the testing side of things I could change the patch to work for any refsepc prefixed with refs/

That way a test could be created to work on both refspec = main and refspec = refs/heads/main and check they are identical?

It would also work with repos containing arbitrary references and would be a bit simpler.

What do you think?

Drew

[Jan Kiszka]

Hi Drew,

please no top-posting. And please also fix your mail client to produce
proper citations ("> ", rather than indentions). Your reply is not easy
to read.

On 25.06.21 13:38, Drew Reed wrote:
> By default git uses the default refspec of "+refs/heads/*:refs/remotes/origin/*" when fetching from a remote repository and it restricts the available references to ones that exist under the refs/heads tree on the server. (This is kind of explained here if my explanation is hard to follow: https://git-scm.com/book/en/v2/Git-Internals-The-Refspec)
>
> Gerrit/GitlLab and Github use references outside of the refs/heads tree when working with patchsets(refs/changes)/merge-requests(refs/merge-requests) and pull's(refs/pull) respectively. So to enable a local repo to checkout these references you have to expand the scope of the fetch. This can be done by either adding additional fetch refspecs into the local .git/config file or by manually specifying it on the fetch command line. For this patch I chose to use the fetch command line and restrict the additional reference scope the actual patchset being requested so as not to waste time fetching what could be a huge number of references.
>
> The why is so that the repos referenced by kas can be "in development". By this I mean patchsets/merge-requests and pull requests that haven't yet been merged onto a branch but would like to be reference to enable per merge testing to be performed.
>

Thanks for clarifying. Add those arguments to the commit log to document
them.

But this still looks fishy then. E.g., we would no longer get
"Repository cloned" if the repo exists but the refspec cannot be
resolved immediately. And if the refspec can be (retc==0), we needlessly
do another fetch.

>
> > # Make sure the remote origin is set to the value
> > # in the kas file to avoid surprises
> > @@ -212,7 +211,8 @@ class RepoImpl(Repo):
> > # Try to fetch if refspec is missing or if --update argument was passed
> > (retc, output) = await run_cmd_async(self.fetch_cmd(),
> > cwd=self.path,
> > - fail=False)
> > + fail=False,
> > + liveupdate=False)
>
> And why that?
> The fetch outputs information on the references fetched and it seemed different to the existing output, It can be safely removed I believe but the log will be noisier.
>

We probably need to rethink the strategy then. Maybe actually
edit/create a .git/config in the home bubble that kas sets up?

Well, at least we have github.com/siemens/kas under control. Would it
help creating something special there? Or would that mean keeping a test
merge request infinitely open?

[Drew Reed]

On 29/06/2021 12:59, Jan Kiszka wrote:
> Hi Drew,
>
> please no top-posting. And please also fix your mail client to produce
> proper citations ("> ", rather than indentions). Your reply is not easy
> to read.

Sorry about that I hope this is better, had to change mail clients as
Outlook on the mac doesn't give the option to use ">".

>
> On 25.06.21 13:38, Drew Reed wrote:
>> By default git uses the default refspec of "+refs/heads/*:refs/remotes/origin/*" when fetching from a remote repository and it restricts the available references to ones that exist under the refs/heads tree on the server. (This is kind of explained here if my explanation is hard to follow: https://git-scm.com/book/en/v2/Git-Internals-The-Refspec)
>>
>> Gerrit/GitlLab and Github use references outside of the refs/heads tree when working with patchsets(refs/changes)/merge-requests(refs/merge-requests) and pull's(refs/pull) respectively. So to enable a local repo to checkout these references you have to expand the scope of the fetch. This can be done by either adding additional fetch refspecs into the local .git/config file or by manually specifying it on the fetch command line. For this patch I chose to use the fetch command line and restrict the additional reference scope the actual patchset being requested so as not to waste time fetching what could be a huge number of references.
>>
>> The why is so that the repos referenced by kas can be "in development". By this I mean patchsets/merge-requests and pull requests that haven't yet been merged onto a branch but would like to be reference to enable per merge testing to be performed.
>>
>
> Thanks for clarifying. Add those arguments to the commit log to document
> them.

Will do

>> Why that?
>>
>> We need to ensure the fetch command is run or the first call or kas would fail as only the clone command would be run and so the requested reference would not be found. Unfortunately you can't add to the default refspec during a clone only during a fetch.
>
> But this still looks fishy then. E.g., we would no longer get
> "Repository cloned" if the repo exists but the refspec cannot be
> resolved immediately. And if the refspec can be (retc==0), we needlessly
> do another fetch.

I can retain the original behavior by only dropping through when we see
an absolute refspec given. I.E. one starting refs/

>
>>
>> > # Make sure the remote origin is set to the value
>> > # in the kas file to avoid surprises
>> > @@ -212,7 +211,8 @@ class RepoImpl(Repo):
>> > # Try to fetch if refspec is missing or if --update argument was passed
>> > (retc, output) = await run_cmd_async(self.fetch_cmd(),
>> > cwd=self.path,
>> > - fail=False)
>> > + fail=False,
>> > + liveupdate=False)
>>
>> And why that?
>> The fetch outputs information on the references fetched and it seemed different to the existing output, It can be safely removed I believe but the log will be noisier.
>>
>
> We probably need to rethink the strategy then. Maybe actually
> edit/create a .git/config in the home bubble that kas sets up?

Modifying the local .git/config won't help as you still need to call git
fetch following a clone if using refspecs outside of the default scope.

If I modify the patch to completely retain the original behavior when
not using absolute refspecs and add the --quiet option to the fetch when
using a specific refspec there will only be a small log difference when
using an absolute refpsec.

It would look something like this in the log:
2021-06-30 14:44:20 - INFO - kas 2.5 started
2021-06-30 14:44:20 - WARNING - kas: No supported distros found in
['darwin']. No default locales set.
2021-06-30 14:44:20 - INFO - /Users/dreree01/code/kas/temp_dir$ git
rev-parse --show-toplevel
2021-06-30 14:44:20 - INFO - /Users/dreree01/code/kas/temp_dir$ git
clone -q https://gerrit.oss.arm.com/iot-sw/meta-arm-image
/Users/dreree01/code/kas/temp_dir/kas_test
2021-06-30 14:44:26 - INFO - Repository kas_test cloned
2021-06-30 14:44:26 - INFO -
/Users/dreree01/code/kas/temp_dir/kas_test$ git remote set-url origin
https://gerrit.oss.arm.com/iot-sw/meta-arm-image
2021-06-30 14:44:26 - INFO -
/Users/dreree01/code/kas/temp_dir/kas_test$ git cat-file -t
changes/44/194744/3
2021-06-30 14:44:26 - INFO -
/Users/dreree01/code/kas/temp_dir/kas_test$ git fetch --quiet origin
+refs/changes/44/194744/3:refs/remotes/origin/changes/44/194744/3
2021-06-30 14:44:31 - INFO - Repository kas_test updated
2021-06-30 14:44:31 - INFO -
/Users/dreree01/code/kas/temp_dir/kas_test$ git status -s
2021-06-30 14:44:31 - INFO -
/Users/dreree01/code/kas/temp_dir/kas_test$ git rev-parse --verify -q
origin/changes/44/194744/3
2021-06-30 14:44:31 - INFO - 52a8bf72a98832a46115ee20ac8a439385ffa0f9
2021-06-30 14:44:31 - INFO -
/Users/dreree01/code/kas/temp_dir/kas_test$ git checkout -q
52a8bf72a98832a46115ee20ac8a439385ffa0f9 -B changes/44/194744/3

>>
>> Test cases are missing, though I'm not sure how easy they could be
>> created for the individual scenarios.
>>
>> Yes, I don't know how to create ones without example changes let pending in gerrit/gitlab and github based servers. I'm open to suggestions on how to test it?
>>
>
> Well, at least we have github.com/siemens/kas under control. Would it
> help creating something special there? Or would that mean keeping a test
> merge request infinitely open?

The best test would mean leaving a merge request open infinitely, but I
could change it to work with any absolute refspec and not just gerrit
patch sets, github pull requests and gitlab merge requests and then
create a test that compares using /refs/heads/master with just master

>
> Jan
>


--
Drew Reed

[Drew Reed]

By default git only fetches references under the refs/heads/ tree,
this patch adds support to kas to enable you to specify references
outside of the ref/heads tree. This is useful as it allows you to
use uncommitted gerrit patchsets, Gitlab merge requests or github
pull requests that live under refs/changes/, refs/merge-requests
and refs/pull as the reference for a repo allowing the use of
in development changes. When a refsepc is defined that starts
with refs/ an additional git fetch operation is preformed on the
repo to explicitly fetch the reference given so it can be checked
out for use.

Signed-off-by: Drew Reed <drew...@arm.com>
---

kas/repos.py | 28 +++++++++++++++++++++-------
tests/test_refspec.py | 24 ++++++++++++++++++++++++
tests/test_refspec/test3.yml | 13 +++++++++++++
3 files changed, 58 insertions(+), 7 deletions(-)
create mode 100644 tests/test_refspec/test3.yml

diff --git a/kas/repos.py b/kas/repos.py
index edd46ee..1eb2e5c 100644
--- a/kas/repos.py
+++ b/kas/repos.py
@@ -179,7 +179,8 @@ class RepoImpl(Repo):

cwd=get_context().kas_work_dir)
if retc == 0:
logging.info('Repository %s cloned', self.name)
- return retc

+ if not self.refspec.startswith('refs/'):
+ return retc

# Make sure the remote origin is set to the value
# in the kas file to avoid surprises

@@ -336,6 +337,10 @@ class GitRepo(RepoImpl):

Provides the git functionality for a Repo.
"""

+ def remove_ref_prefix(self, refspec):
+ ref_prefix = 'refs/'
+ return refspec[refspec.startswith(ref_prefix) and len(ref_prefix):]
+
def add_cmd(self):
return ['git', 'add', '-A']

@@ -350,29 +355,38 @@ class GitRepo(RepoImpl):

'-m', 'msg']

def contains_refspec_cmd(self):
- return ['git', 'cat-file', '-t', self.refspec]
+ return ['git', 'cat-file', '-t', self.remove_ref_prefix(self.refspec)]

def fetch_cmd(self):
- return ['git', 'fetch']
+ cmd = ['git', 'fetch']

+ if self.refspec.startswith('refs/'):
+ cmd.extend(['--quiet', 'origin',

diff --git a/tests/test_refspec.py b/tests/test_refspec.py
index e8495d9..edb798e 100644
--- a/tests/test_refspec.py
+++ b/tests/test_refspec.py
@@ -63,3 +63,27 @@ def test_refspec_switch(changedir, tmpdir):
fail=False, liveupdate=False)
assert rc == 0
assert output.strip() == '907816a5c4094b59a36aec12226e71c461c05b77'
+
+
+def test_refspec_absolute(changedir, tmpdir):
+ """
+ Test that the local git clone works when a absolute refspec
+ is givvn.
+ """
+ tdir = str(tmpdir.mkdir('test_refspec_absolute'))
+ shutil.rmtree(tdir, ignore_errors=True)
+ shutil.copytree('tests/test_refspec', tdir)
+ os.chdir(tdir)
+
+ kas.kas(['shell', 'test3.yml', '-c', 'true'])
+ (rc, output) = run_cmd(['git', 'symbolic-ref', '-q', 'HEAD'],
+ cwd='kas_abs', fail=False, liveupdate=False)
+ assert rc != 0
+ assert output.strip() == ''
+ (rc, output_kas_abs) = run_cmd(['git', 'rev-parse', 'HEAD'],
+ cwd='kas_abs', fail=False, liveupdate=False)
+ assert rc == 0
+ (rc, output_kas_rel) = run_cmd(['git', 'rev-parse', 'HEAD'],
+ cwd='kas_rel', fail=False, liveupdate=False)
+ assert rc == 0
+ assert output_kas_abs.strip() == output_kas_rel.strip()
diff --git a/tests/test_refspec/test3.yml b/tests/test_refspec/test3.yml
new file mode 100644
index 0000000..2557cfe
--- /dev/null
+++ b/tests/test_refspec/test3.yml
@@ -0,0 +1,13 @@
+header:
+ version: 8
+
+repos:
+ this:
+
+ kas_abs:
+ url: https://github.com/siemens/kas.git
+ refspec: refs/heads/master
+
+ kas_rel:
+ url: https://github.com/siemens/kas.git
+ refspec: master
--
2.24.3 (Apple Git-128)

[Jan Kiszka]

Henning, do you see any unwanted side effect on hg with this?

Looks good to me otherwise.

[Jan Kiszka]

Well, I'm sure Henning will complain later if things do not work with hg.

Applied to next now (before the kas menu series), thanks!