[PATCH] combo-layer: make Signed-off-by optional

[PATCH] combo-layer: make Signed-off-by optional

[Patrick Ohly]

It depends on the diligence of the person running the combo-layer tool
whether the Signed-off-by line added to each commit actually indicates
that the person was involved in validating the change.

When the import is purely automatic, it is better to not add the line,
because the history is more useful without it (searching for the person
really only lists changes he or she was involved with) and it would
be a false statement.

This needs to be configurable, achieved with a new global "signoff"
boolean property in combo-layer.conf, in the "DEFAULT" section.

Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
---
 scripts/combo-layer              | 4 +++-
 scripts/combo-layer.conf.example | 6 ++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/scripts/combo-layer b/scripts/combo-layer
index 19d64e6..60ead5b 100755
--- a/scripts/combo-layer
+++ b/scripts/combo-layer
@@ -75,6 +75,8 @@ class Configuration(object):
             self.parser.readfp(f)
 
         self.repos = {}
+        self.signoff = not self.parser.has_option('DEFAULT', 'signoff') or \
+                       self.parser.getboolean('DEFAULT', 'signoff')
         for repo in self.parser.sections():
             self.repos[repo] = {}
             readsection(self.parser, repo, repo)
@@ -471,7 +473,7 @@ def apply_patchlist(conf, repos):
                 if os.path.getsize(patchfile) == 0:
                     logger.info("(skipping %d/%d %s - no changes)" % (i, linecount, patchdisp))
                 else:
-                    cmd = "git am --keep-cr -s -p1 %s" % patchfile
+                    cmd = "git am --keep-cr %s-p1 %s" % ('-s ' if conf.signoff else '', patchfile)
                     logger.info("Applying %d/%d: %s" % (i, linecount, patchdisp))
                     try:
                         runcmd(cmd)
diff --git a/scripts/combo-layer.conf.example b/scripts/combo-layer.conf.example
index 010a692..8ad8615 100644
--- a/scripts/combo-layer.conf.example
+++ b/scripts/combo-layer.conf.example
@@ -1,5 +1,11 @@
 # combo-layer example configuration file
 
+# global options
+[DEFAULT]
+
+# Add 'Signed-off-by' to all commits that get imported automatically.
+signoff = True
+
 # component name
 [bitbake]
 # mandatory options
-- 
2.1.4

Re: [PATCH] combo-layer: make Signed-off-by optional

[Paul Eggleton]

Hi Patrick,

On Monday 09 March 2015 13:56:39 Patrick Ohly wrote:
> It depends on the diligence of the person running the combo-layer tool
> whether the Signed-off-by line added to each commit actually indicates
> that the person was involved in validating the change.
> 
> When the import is purely automatic, it is better to not add the line,
> because the history is more useful without it (searching for the person
> really only lists changes he or she was involved with) and it would
> be a false statement.
> 
> This needs to be configurable, achieved with a new global "signoff"
> boolean property in combo-layer.conf, in the "DEFAULT" section.
> 
> Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
> ---
>  scripts/combo-layer              | 4 +++-
>  scripts/combo-layer.conf.example | 6 ++++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/combo-layer b/scripts/combo-layer
> index 19d64e6..60ead5b 100755
> --- a/scripts/combo-layer
> +++ b/scripts/combo-layer
> @@ -75,6 +75,8 @@ class Configuration(object):
>              self.parser.readfp(f)
> 
>          self.repos = {}
> +        self.signoff = not self.parser.has_option('DEFAULT', 'signoff') or
> \ +                       self.parser.getboolean('DEFAULT', 'signoff') 
> for repo in self.parser.sections():
>              self.repos[repo] = {}
>              readsection(self.parser, repo, repo)
> @@ -471,7 +473,7 @@ def apply_patchlist(conf, repos):
>                  if os.path.getsize(patchfile) == 0:
>                      logger.info("(skipping %d/%d %s - no changes)" % (i,
> linecount, patchdisp)) else:
> -                    cmd = "git am --keep-cr -s -p1 %s" % patchfile
> +                    cmd = "git am --keep-cr %s-p1 %s" % ('-s ' if
> conf.signoff else '', patchfile) logger.info("Applying %d/%d: %s" % (i,
> linecount, patchdisp)) try:
>                          runcmd(cmd)
> diff --git a/scripts/combo-layer.conf.example
> b/scripts/combo-layer.conf.example index 010a692..8ad8615 100644
> --- a/scripts/combo-layer.conf.example
> +++ b/scripts/combo-layer.conf.example
> @@ -1,5 +1,11 @@
>  # combo-layer example configuration file
> 
> +# global options
> +[DEFAULT]
> +
> +# Add 'Signed-off-by' to all commits that get imported automatically.
> +signoff = True
> +
>  # component name
>  [bitbake]
>  # mandatory options

So I'm OK with adding this in as an option. However to me a name like DEFAULT 
implies you're establishing a general section to apply default settings for 
all components where the component can override those defaults if it chooses, 
which doesn't really represent what this does - so a different name might be 
more appropriate (GLOBAL or _global_ perhaps?)

Cheers,
Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre

Re: [PATCH] combo-layer: make Signed-off-by optional

[Patrick Ohly]

On Thu, 2015-03-12 at 18:21 +0000, Paul Eggleton wrote:
> Hi Patrick,
> 
> On Monday 09 March 2015 13:56:39 Patrick Ohly wrote:
> > +# global options
> > +[DEFAULT]
> > +
> > +# Add 'Signed-off-by' to all commits that get imported automatically.
> > +signoff = True
> > +
> >  # component name
> >  [bitbake]
> >  # mandatory options
> 
> So I'm OK with adding this in as an option. However to me a name like DEFAULT 
> implies you're establishing a general section to apply default settings for 
> all components where the component can override those defaults if it chooses, 
> which doesn't really represent what this does - so a different name might be 
> more appropriate (GLOBAL or _global_ perhaps?)

"DEFAULT" is the special string that Python's ConfigParser uses, well,
by default for that special section which does not show up in the list
of sections. I don't know how to rename that.

I'm probably abusing this concept a bit here: it seems that special
section is meant to provide default values that get returned also for
the other sections when they don't have their own value.

Here's a cleaner solution:
      * Get rid of the [DEFAULT] section in the file.
      * When reading it, on-the-fly prepend the string
        '[this-is-not-really-a-repo'].
      * When reading global properties, get it from that section.
      * When listing sections to find repos, ignore it.

How about that?

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

Re: [PATCH] combo-layer: make Signed-off-by optional

[Paul Eggleton]

On Thursday 12 March 2015 20:45:32 Patrick Ohly wrote:
> On Thu, 2015-03-12 at 18:21 +0000, Paul Eggleton wrote:
> > On Monday 09 March 2015 13:56:39 Patrick Ohly wrote:
> > > +# global options
> > > +[DEFAULT]
> > > +
> > > +# Add 'Signed-off-by' to all commits that get imported automatically.
> > > +signoff = True
> > > +
> > > 
> > >  # component name
> > >  [bitbake]
> > >  # mandatory options
> > 
> > So I'm OK with adding this in as an option. However to me a name like
> > DEFAULT implies you're establishing a general section to apply default
> > settings for all components where the component can override those
> > defaults if it chooses, which doesn't really represent what this does -
> > so a different name might be more appropriate (GLOBAL or _global_
> > perhaps?)
> 
> "DEFAULT" is the special string that Python's ConfigParser uses, well,
> by default for that special section which does not show up in the list
> of sections. I don't know how to rename that.
> 
> I'm probably abusing this concept a bit here: it seems that special
> section is meant to provide default values that get returned also for
> the other sections when they don't have their own value.
> 
> Here's a cleaner solution:
>       * Get rid of the [DEFAULT] section in the file.
>       * When reading it, on-the-fly prepend the string
>         '[this-is-not-really-a-repo'].
>       * When reading global properties, get it from that section.
>       * When listing sections to find repos, ignore it.
> 
> How about that?

Hmm, ok, I wasn't aware of this built-in "DEFAULT" section behaviour. A simple 
solution then would just be to make this setting per-component and keep the 
DEFAULT usage in the example config - although I do realise it's unlikely to be 
very useful on a per-component basis, at least it will work in the same way as 
other values we read from there.

Cheers,
Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre

Re: [PATCH] combo-layer: make Signed-off-by optional

[Patrick Ohly]

On Fri, 2015-03-13 at 08:57 +0000, Paul Eggleton wrote:
> On Thursday 12 March 2015 20:45:32 Patrick Ohly wrote:
> > On Thu, 2015-03-12 at 18:21 +0000, Paul Eggleton wrote:
> > > On Monday 09 March 2015 13:56:39 Patrick Ohly wrote:
> > > > +# global options
> > > > +[DEFAULT]
> > > > +
> > > > +# Add 'Signed-off-by' to all commits that get imported automatically.
> > > > +signoff = True
> > > > +
> > > > 
> > > >  # component name
> > > >  [bitbake]
> > > >  # mandatory options
> > > 
> > > So I'm OK with adding this in as an option. However to me a name like
> > > DEFAULT implies you're establishing a general section to apply default
> > > settings for all components where the component can override those
> > > defaults if it chooses, which doesn't really represent what this does -
> > > so a different name might be more appropriate (GLOBAL or _global_
> > > perhaps?)
> > 
> > "DEFAULT" is the special string that Python's ConfigParser uses, well,
> > by default for that special section which does not show up in the list
> > of sections. I don't know how to rename that.
> > 
> > I'm probably abusing this concept a bit here: it seems that special
> > section is meant to provide default values that get returned also for
> > the other sections when they don't have their own value.
> > 
> > Here's a cleaner solution:
> >       * Get rid of the [DEFAULT] section in the file.
> >       * When reading it, on-the-fly prepend the string
> >         '[this-is-not-really-a-repo'].
> >       * When reading global properties, get it from that section.
> >       * When listing sections to find repos, ignore it.
> > 
> > How about that?
> 
> Hmm, ok, I wasn't aware of this built-in "DEFAULT" section behaviour. A simple 
> solution then would just be to make this setting per-component and keep the 
> DEFAULT usage in the example config - although I do realise it's unlikely to be 
> very useful on a per-component basis, at least it will work in the same way as 
> other values we read from there.

Okay, I'll do that. It has the additional benefit of introducing the
[DEFAULT] section in the example. It may be useful also for other
properties, like a "branch = master".

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.