Patchwork kernel-yocto: Inspect remote branches with git ls-remote

login
register
mail settings
Submitter Matt Fleming
Date March 5, 2014, 4:49 p.m.
Message ID <1394038191-26474-1-git-send-email-matt@console-pimps.org>
Download mbox | patch
Permalink /patch/68065/
State New
Headers show

Comments

Matt Fleming - March 5, 2014, 4:49 p.m.
From: Matt Fleming <matt.fleming@intel.com>

'git branch' may use ANSI escape codes in its output (to provide colour)
which doesn't play well with commands expecting pure plain text, e.g.

    fatal: '^[[31mmaster^[[m' is not a valid branch name.

Furthermore, the output of 'git branch' is subject to change and trying
to parse it could potentially lead to breakage in the future.

Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
 meta/classes/kernel-yocto.bbclass | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
Bruce Ashfield - March 5, 2014, 5:02 p.m.
Please cc' me directly on any kernel-yocto changes, I only noticed this
by chance.

On Wed, Mar 5, 2014 at 11:49 AM, Matt Fleming <matt@console-pimps.org> wrote:
> From: Matt Fleming <matt.fleming@intel.com>
>
> 'git branch' may use ANSI escape codes in its output (to provide colour)
> which doesn't play well with commands expecting pure plain text, e.g.
>
>     fatal: '^[[31mmaster^[[m' is not a valid branch name.

Nasty corner case, one I've never run into .. but that's why the --no-color
option exists. I'd prefer that it be used, and we reduce the footprint of
the change. See below.

>
> Furthermore, the output of 'git branch' is subject to change and trying
> to parse it could potentially lead to breakage in the future.

I've never had a problem with it in 7 years of the tools and their variants, so
the point is arguable and could be dropped from the commit log.

>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
>  meta/classes/kernel-yocto.bbclass | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/meta/classes/kernel-yocto.bbclass b/meta/classes/kernel-yocto.bbclass
> index fb8e04e..1115056 100644
> --- a/meta/classes/kernel-yocto.bbclass
> +++ b/meta/classes/kernel-yocto.bbclass
> @@ -196,7 +196,7 @@ do_kernel_checkout() {
>                 # If KMETA is defined, the branch must exist, but a machine branch
>         # can be missing since it may be created later by the tools.
>         if [ -n "${KMETA}" ]; then
> -               git branch -a | grep -q ${KMETA}
> +               git ls-remote --heads 2>/dev/null | awk '{print $NF}' | grep -q ${KMETA}

cut is lighter weight than awk, and doesn't have any variants (like
awk does), so I'd
prefer cut in the pipeline versus awk. Sure that's a preference, but I
support the
class, so I get the pain when it breaks (and it has).

Good catch on the issue, like I said, I've never run into it .. but I
can definitely see
the need for the tweak.

Cheers,

Bruce

>                 if [ $? -ne 0 ]; then
>                         echo "ERROR. The branch '${KMETA}' is required and was not"
>                         echo "found. Ensure that the SRC_URI points to a valid linux-yocto"
> @@ -214,11 +214,11 @@ do_kernel_checkout() {
>         fi
>
>         # convert any remote branches to local tracking ones
> -       for i in `git branch -a | grep remotes | grep -v HEAD`; do
> -               b=`echo $i | cut -d' ' -f2 | sed 's%remotes/origin/%%'`;
> -               git show-ref --quiet --verify -- "refs/heads/$b"
> +       for i in `git ls-remote --heads 2>/dev/null | awk '{print $NF}'`; do
> +               b=`echo $i | sed 's%refs/heads/%%'`;
> +               git show-ref --quiet --verify -- "$i"
>                 if [ $? -ne 0 ]; then
> -                       git branch $b $i > /dev/null
> +                       git branch $b remotes/origin/$b > /dev/null
>                 fi
>         done
>
> --
> 1.8.5.3
>
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core
Richard Purdie - March 5, 2014, 5:47 p.m.
On Wed, 2014-03-05 at 16:49 +0000, Matt Fleming wrote:
> From: Matt Fleming <matt.fleming@intel.com>
> 
> 'git branch' may use ANSI escape codes in its output (to provide colour)
> which doesn't play well with commands expecting pure plain text, e.g.
> 
>     fatal: '^[[31mmaster^[[m' is not a valid branch name.
> 
> Furthermore, the output of 'git branch' is subject to change and trying
> to parse it could potentially lead to breakage in the future.
> 
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
>  meta/classes/kernel-yocto.bbclass | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

My worry with this change is that it will cause the system to hit the
network. We specifically support builds from archives of the
repositories so I'd worry about using ls-remote like this :/.

Cheers,

Richard
Chris Larson - March 5, 2014, 7:53 p.m.
On Wed, Mar 5, 2014 at 10:47 AM, Richard Purdie <
richard.purdie@linuxfoundation.org> wrote:

> On Wed, 2014-03-05 at 16:49 +0000, Matt Fleming wrote:
> > From: Matt Fleming <matt.fleming@intel.com>
> >
> > 'git branch' may use ANSI escape codes in its output (to provide colour)
> > which doesn't play well with commands expecting pure plain text, e.g.
> >
> >     fatal: '^[[31mmaster^[[m' is not a valid branch name.
> >
> > Furthermore, the output of 'git branch' is subject to change and trying
> > to parse it could potentially lead to breakage in the future.
> >
> > Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> > ---
> >  meta/classes/kernel-yocto.bbclass | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
>
> My worry with this change is that it will cause the system to hit the
> network. We specifically support builds from archives of the
> repositories so I'd worry about using ls-remote like this :/.


What about git for-each-ref? If we just need a list of refs, that's what
for-each-ref is ideal for.
Bruce Ashfield - March 5, 2014, 8:06 p.m.
On Wed, Mar 5, 2014 at 2:53 PM, Chris Larson <clarson@kergoth.com> wrote:
>
> On Wed, Mar 5, 2014 at 10:47 AM, Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
>>
>> On Wed, 2014-03-05 at 16:49 +0000, Matt Fleming wrote:
>> > From: Matt Fleming <matt.fleming@intel.com>
>> >
>> > 'git branch' may use ANSI escape codes in its output (to provide colour)
>> > which doesn't play well with commands expecting pure plain text, e.g.
>> >
>> >     fatal: '^[[31mmaster^[[m' is not a valid branch name.
>> >
>> > Furthermore, the output of 'git branch' is subject to change and trying
>> > to parse it could potentially lead to breakage in the future.
>> >
>> > Signed-off-by: Matt Fleming <matt.fleming@intel.com>
>> > ---
>> >  meta/classes/kernel-yocto.bbclass | 10 +++++-----
>> >  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> My worry with this change is that it will cause the system to hit the
>> network. We specifically support builds from archives of the
>> repositories so I'd worry about using ls-remote like this :/.
>
>
> What about git for-each-ref? If we just need a list of refs, that's what
> for-each-ref is ideal for.

Good suggestion, that can definitely work. With the format, it saves the
awkward awking and cutting :

I've used this in the past:

   git for-each-ref --shell --format="%(refname)"  refs/heads/ refs/remotes

The minimum footprint change is still just --no-color, that way we don't change
just for the sake of changing.

Cheers,

Bruce


> --
> Christopher Larson
> clarson at kergoth dot com
> Founder - BitBake, OpenEmbedded, OpenZaurus
> Maintainer - Tslib
> Senior Software Engineer, Mentor Graphics
>
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core
>
Matt Fleming - March 6, 2014, 11:02 a.m.
On Wed, 05 Mar, at 12:02:27PM, Bruce Ashfield wrote:
> Please cc' me directly on any kernel-yocto changes, I only noticed this
> by chance.
 
Noted.

> On Wed, Mar 5, 2014 at 11:49 AM, Matt Fleming <matt@console-pimps.org> wrote:
> >
> > Furthermore, the output of 'git branch' is subject to change and trying
> > to parse it could potentially lead to breakage in the future.
> 
> I've never had a problem with it in 7 years of the tools and their variants, so
> the point is arguable and could be dropped from the commit log.
 
I'm referring to this,

  http://git-blame.blogspot.co.uk/2013/06/checking-current-branch-programatically.html

> Good catch on the issue, like I said, I've never run into it .. but I
> can definitely see the need for the tweak.

OK, I'll send an updated version. Thanks for the review.
Bruce Ashfield - March 6, 2014, 2:13 p.m.
On Thu, Mar 6, 2014 at 6:02 AM, Matt Fleming <matt@console-pimps.org> wrote:
> On Wed, 05 Mar, at 12:02:27PM, Bruce Ashfield wrote:
>> Please cc' me directly on any kernel-yocto changes, I only noticed this
>> by chance.
>
> Noted.
>
>> On Wed, Mar 5, 2014 at 11:49 AM, Matt Fleming <matt@console-pimps.org> wrote:
>> >
>> > Furthermore, the output of 'git branch' is subject to change and trying
>> > to parse it could potentially lead to breakage in the future.
>>
>> I've never had a problem with it in 7 years of the tools and their variants, so
>> the point is arguable and could be dropped from the commit log.
>
> I'm referring to this,

Yah, I've seen similar warnings as well, I'm a fan of symbolic-ref after getting
burned poking directly into the .git directory structure in the past :) The more
usable primitives are much better than they used to be. But I'm in a pragmatic
mood lately and will keep an eye out for a real world break .. and then everyone
can tell me that they told me so.

Cheers,

Bruce

>
>   http://git-blame.blogspot.co.uk/2013/06/checking-current-branch-programatically.html
>
>> Good catch on the issue, like I said, I've never run into it .. but I
>> can definitely see the need for the tweak.
>
> OK, I'll send an updated version. Thanks for the review.
>
> --
> Matt Fleming, Intel Open Source Technology Center

Patch

diff --git a/meta/classes/kernel-yocto.bbclass b/meta/classes/kernel-yocto.bbclass
index fb8e04e..1115056 100644
--- a/meta/classes/kernel-yocto.bbclass
+++ b/meta/classes/kernel-yocto.bbclass
@@ -196,7 +196,7 @@  do_kernel_checkout() {
        	# If KMETA is defined, the branch must exist, but a machine branch
 	# can be missing since it may be created later by the tools.
 	if [ -n "${KMETA}" ]; then
-		git branch -a | grep -q ${KMETA}
+		git ls-remote --heads 2>/dev/null | awk '{print $NF}' | grep -q ${KMETA}
 		if [ $? -ne 0 ]; then
 			echo "ERROR. The branch '${KMETA}' is required and was not"
 			echo "found. Ensure that the SRC_URI points to a valid linux-yocto"
@@ -214,11 +214,11 @@  do_kernel_checkout() {
 	fi
 
 	# convert any remote branches to local tracking ones
-	for i in `git branch -a | grep remotes | grep -v HEAD`; do
-		b=`echo $i | cut -d' ' -f2 | sed 's%remotes/origin/%%'`;
-		git show-ref --quiet --verify -- "refs/heads/$b"
+	for i in `git ls-remote --heads 2>/dev/null | awk '{print $NF}'`; do
+		b=`echo $i | sed 's%refs/heads/%%'`;
+		git show-ref --quiet --verify -- "$i"
 		if [ $? -ne 0 ]; then
-			git branch $b $i > /dev/null
+			git branch $b remotes/origin/$b > /dev/null
 		fi
 	done