[PATCH] bitbake: bb.fetch2.git: Fix _latest_revision function while using tags

[PATCH] bitbake: bb.fetch2.git: Fix _latest_revision function while using tags

[Andrei Gherzan]

When getting the revision we must take into consideration if the name we are
looking for is a tag and in that case we need the dereferenced commit ID in
order to check for it existance in a specific branch.

So first search for the reference^{} commit ID and only if that returns nothing
get the name as it is.

Signed-off-by: Andrei Gherzan <andrei@gherzan.ro>
---
 bitbake/lib/bb/fetch2/git.py | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/bitbake/lib/bb/fetch2/git.py b/bitbake/lib/bb/fetch2/git.py
index b4b9368..1584232 100644
--- a/bitbake/lib/bb/fetch2/git.py
+++ b/bitbake/lib/bb/fetch2/git.py
@@ -318,9 +318,13 @@ class Git(FetchMethod):
               (ud.basecmd, ud.proto, username, ud.host, ud.path, ud.unresolvedrev[name])
         if ud.proto.lower() != 'file':
             bb.fetch2.check_network_access(d, cmd)
-        output = runfetchcmd(cmd, d, True)
+        # Maybe the ref is a tag so try to dereference it first
+        output = runfetchcmd(cmd + '^{}', d, True)
         if not output:
-            raise bb.fetch2.FetchError("The command %s gave empty output unexpectedly" % cmd, ud.url)
+            # Dereference gave nothing so try to get it as it is
+            output = runfetchcmd(cmd, d, True)
+            if not output:
+                raise bb.fetch2.FetchError("The command %s gave empty output unexpectedly" % cmd, ud.url)
         return output.split()[0]
 
     def _build_revision(self, ud, d, name):
-- 
1.8.1.4

Re: [PATCH] bitbake: bb.fetch2.git: Fix _latest_revision function while using tags

[Olof Johansson]

On 14-01-05 02:53 +0100, Andrei Gherzan wrote:
> When getting the revision we must take into consideration if the name we are
> looking for is a tag and in that case we need the dereferenced commit ID in
> order to check for it existance in a specific branch.
> 
> So first search for the reference^{} commit ID and only if that returns nothing
> get the name as it is.

I think this is the same issue I've tried to solve in a patch i
sent in December (subject: bb.fetch2.git: support resolving both
tags and branches). I haven't heard anything about it yet though.

-- 
olofjn

Re: [PATCH] bitbake: bb.fetch2.git: Fix _latest_revision function while using tags

[Richard Purdie]

On Tue, 2014-01-07 at 11:36 +0100, Olof Johansson wrote:
> On 14-01-05 02:53 +0100, Andrei Gherzan wrote:
> > When getting the revision we must take into consideration if the name we are
> > looking for is a tag and in that case we need the dereferenced commit ID in
> > order to check for it existance in a specific branch.
> > 
> > So first search for the reference^{} commit ID and only if that returns nothing
> > get the name as it is.
> 
> I think this is the same issue I've tried to solve in a patch i
> sent in December (subject: bb.fetch2.git: support resolving both
> tags and branches). I haven't heard anything about it yet though.

I put this off as I wanted to spend some time and see if we couldn't
come up with something simpler. Andrei's patch is simpler, the question
is whether it covers all the cases and whether we even need the fallback
code?

I've merged the nobranch code since its clear there are cases when that
is the right thing to do and its a seperate issue to this.

Cheers,

Richard

Re: [PATCH] bitbake: bb.fetch2.git: Fix _latest_revision function while using tags

[Olof Johansson]

On 14-01-07 14:40 +0100, Richard Purdie wrote:
> On Tue, 2014-01-07 at 11:36 +0100, Olof Johansson wrote:
> > On 14-01-05 02:53 +0100, Andrei Gherzan wrote:
> > > When getting the revision we must take into consideration if the name we are
> > > looking for is a tag and in that case we need the dereferenced commit ID in
> > > order to check for it existance in a specific branch.
> > > 
> > > So first search for the reference^{} commit ID and only if that returns nothing
> > > get the name as it is.
> > 
> > I think this is the same issue I've tried to solve in a patch i
> > sent in December (subject: bb.fetch2.git: support resolving both
> > tags and branches). I haven't heard anything about it yet though.
> 
> I put this off as I wanted to spend some time and see if we couldn't
> come up with something simpler. Andrei's patch is simpler, the question
> is whether it covers all the cases and whether we even need the fallback
> code?

I agree with your reasoning with regards to not needing a
fallback for Andrei's patch, but my patch tries to first resolve
refs/tags/<tag> (fully qualified) and then falls back to try
refs/heads/<branch>. The reason for this is that ls-remote will
return things like refs/heads/lalala/<tag> if you only do
ls-remote <tag>.

My attempt was also to break out the tasks into simple,
atomic(-ish) functions that would later be easier to write unit
tests for. You complained on IRC that changes to the fetcher
usually comes back to bite you. I believe increased unit test
coverage on low level functions, as a compliment to the existing
more high level suite, can be beneficial in this regard.


Also, if you are holding things off, I would really appreciate
you letting us know so that we can make appropriate workarounds
if needed (as in this case, where we have had to supsend testing
on master :-().

Regards,
-- 
olofjn

Re: [PATCH] bitbake: bb.fetch2.git: Fix _latest_revision function while using tags

[Richard Purdie]

On Tue, 2014-01-07 at 15:46 +0100, Olof Johansson wrote:
> I agree with your reasoning with regards to not needing a
> fallback for Andrei's patch, but my patch tries to first resolve
> refs/tags/<tag> (fully qualified) and then falls back to try
> refs/heads/<branch>. The reason for this is that ls-remote will
> return things like refs/heads/lalala/<tag> if you only do
> ls-remote <tag>.

Can we somehow anchor the expression instead e.g. refs/*/<xxx>^{}

> My attempt was also to break out the tasks into simple,
> atomic(-ish) functions that would later be easier to write unit
> tests for. You complained on IRC that changes to the fetcher
> usually comes back to bite you. I believe increased unit test
> coverage on low level functions, as a compliment to the existing
> more high level suite, can be beneficial in this regard.

I believe that my complaint could also be addressed by ensuring the high
level tests had more coverage of the variety of supported use cases. In
this case a range of tags of differing types, branches and hanging
commits would have helped ensure the changes to the fetcher didn't break
things.

I'm less keen on forcing an atom like set of functions on the fetcher
just for the purposes of the test suite. Sorry I was unclear on that, I
guess different people have different understandings of what increased
coverage would mean.

> Also, if you are holding things off, I would really appreciate
> you letting us know so that we can make appropriate workarounds
> if needed (as in this case, where we have had to supsend testing
> on master :-().

Well, it wasn't a conscious decision so much as the holidays came along
and I ran out of time to look at it. I'm trying to get this resolved now
since obviously various people are hitting the problem cases.

Cheers,

Richard

Re: [PATCH] bitbake: bb.fetch2.git: Fix _latest_revision function while using tags

[Olof Johansson]

On 14-01-07 16:02 +0100, Richard Purdie wrote:
> On Tue, 2014-01-07 at 15:46 +0100, Olof Johansson wrote:
> > I agree with your reasoning with regards to not needing a
> > fallback for Andrei's patch, but my patch tries to first resolve
> > refs/tags/<tag> (fully qualified) and then falls back to try
> > refs/heads/<branch>. The reason for this is that ls-remote will
> > return things like refs/heads/lalala/<tag> if you only do
> > ls-remote <tag>.
> 
> Can we somehow anchor the expression instead e.g. refs/*/<xxx>^{}

Sure, we can do ls-remote without any arguments and parse the
output. But this won't make the patch easier in any way. One less
fork though.

> > My attempt was also to break out the tasks into simple,
> > atomic(-ish) functions that would later be easier to write unit
> > tests for. You complained on IRC that changes to the fetcher
> > usually comes back to bite you. I believe increased unit test
> > coverage on low level functions, as a compliment to the existing
> > more high level suite, can be beneficial in this regard.
> 
> I believe that my complaint could also be addressed by ensuring the high
> level tests had more coverage of the variety of supported use cases. In
> this case a range of tags of differing types, branches and hanging
> commits would have helped ensure the changes to the fetcher didn't break
> things.

Yes, that probably possible. *I* find low level unit tests easier
to work with, and I wanted to do what I could to alleviate the
situation; making sure that (at least) our use cases are covered
by automated tests.

> I'm less keen on forcing an atom like set of functions on the fetcher
> just for the purposes of the test suite. Sorry I was unclear on that, I
> guess different people have different understandings of what increased
> coverage would mean.

I find it easier to work in functions with limited scopes, with
clear purposes --- with or without unittests (unittests being
preferable, obviously). But then again, I'm not the maintainer,
so your preferences are more important :-).

> > Also, if you are holding things off, I would really appreciate
> > you letting us know so that we can make appropriate workarounds
> > if needed (as in this case, where we have had to supsend testing
> > on master :-().
> 
> Well, it wasn't a conscious decision so much as the holidays came along
> and I ran out of time to look at it. I'm trying to get this resolved now
> since obviously various people are hitting the problem cases.

I really appreciate the hard work you are doing :-).


Regards,
-- 
olofjn

Re: [PATCH] bitbake: bb.fetch2.git: Fix _latest_revision function while using tags

[Andrei Gherzan]

[-- Attachment #1: Type: text/plain, Size: 2936 bytes --]

On Jan 7, 2014 5:52 PM, "Olof Johansson" <olof.johansson@axis.com> wrote:
>
> On 14-01-07 16:02 +0100, Richard Purdie wrote:
> > On Tue, 2014-01-07 at 15:46 +0100, Olof Johansson wrote:
> > > I agree with your reasoning with regards to not needing a
> > > fallback for Andrei's patch, but my patch tries to first resolve
> > > refs/tags/<tag> (fully qualified) and then falls back to try
> > > refs/heads/<branch>. The reason for this is that ls-remote will
> > > return things like refs/heads/lalala/<tag> if you only do
> > > ls-remote <tag>.
> >
> > Can we somehow anchor the expression instead e.g. refs/*/<xxx>^{}
>
> Sure, we can do ls-remote without any arguments and parse the
> output. But this won't make the patch easier in any way. One less
> fork though.
>
> > > My attempt was also to break out the tasks into simple,
> > > atomic(-ish) functions that would later be easier to write unit
> > > tests for. You complained on IRC that changes to the fetcher
> > > usually comes back to bite you. I believe increased unit test
> > > coverage on low level functions, as a compliment to the existing
> > > more high level suite, can be beneficial in this regard.
> >
> > I believe that my complaint could also be addressed by ensuring the high
> > level tests had more coverage of the variety of supported use cases. In
> > this case a range of tags of differing types, branches and hanging
> > commits would have helped ensure the changes to the fetcher didn't break
> > things.
>
> Yes, that probably possible. *I* find low level unit tests easier
> to work with, and I wanted to do what I could to alleviate the
> situation; making sure that (at least) our use cases are covered
> by automated tests.
>
> > I'm less keen on forcing an atom like set of functions on the fetcher
> > just for the purposes of the test suite. Sorry I was unclear on that, I
> > guess different people have different understandings of what increased
> > coverage would mean.
>
> I find it easier to work in functions with limited scopes, with
> clear purposes --- with or without unittests (unittests being
> preferable, obviously). But then again, I'm not the maintainer,
> so your preferences are more important :-).
>
> > > Also, if you are holding things off, I would really appreciate
> > > you letting us know so that we can make appropriate workarounds
> > > if needed (as in this case, where we have had to supsend testing
> > > on master :-().
> >
> > Well, it wasn't a conscious decision so much as the holidays came along
> > and I ran out of time to look at it. I'm trying to get this resolved now
> > since obviously various people are hitting the problem cases.
>
> I really appreciate the hard work you are doing :-).

Me too :-)

I agree that we really need to fix this as we are breaking layers out in
the wild with this issue.

@olof sorry for duplicate email.

ag

[-- Attachment #2: Type: text/html, Size: 3668 bytes --]

Re: [PATCH] bitbake: bb.fetch2.git: Fix _latest_revision function while using tags

[Richard Purdie]

On Sun, 2014-01-05 at 03:53 +0200, Andrei Gherzan wrote:
> When getting the revision we must take into consideration if the name we are
> looking for is a tag and in that case we need the dereferenced commit ID in
> order to check for it existance in a specific branch.
> 
> So first search for the reference^{} commit ID and only if that returns nothing
> get the name as it is.

In what case would reference^{} fail to work when reference would?
Reading the git documentation, I'm struggling to see a case when ^{}
wouldn't do the right thing and if that is the case we don't need the
fallback?

Cheers,

Richard

> Signed-off-by: Andrei Gherzan <andrei@gherzan.ro>
> ---
>  bitbake/lib/bb/fetch2/git.py | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/bitbake/lib/bb/fetch2/git.py b/bitbake/lib/bb/fetch2/git.py
> index b4b9368..1584232 100644
> --- a/bitbake/lib/bb/fetch2/git.py
> +++ b/bitbake/lib/bb/fetch2/git.py
> @@ -318,9 +318,13 @@ class Git(FetchMethod):
>                (ud.basecmd, ud.proto, username, ud.host, ud.path, ud.unresolvedrev[name])
>          if ud.proto.lower() != 'file':
>              bb.fetch2.check_network_access(d, cmd)
> -        output = runfetchcmd(cmd, d, True)
> +        # Maybe the ref is a tag so try to dereference it first
> +        output = runfetchcmd(cmd + '^{}', d, True)
>          if not output:
> -            raise bb.fetch2.FetchError("The command %s gave empty output unexpectedly" % cmd, ud.url)
> +            # Dereference gave nothing so try to get it as it is
> +            output = runfetchcmd(cmd, d, True)
> +            if not output:
> +                raise bb.fetch2.FetchError("The command %s gave empty output unexpectedly" % cmd, ud.url)
>          return output.split()[0]
>  
>      def _build_revision(self, ud, d, name):