Patchwork [1/3] UBUNTU: git-ubuntu-log -- commonise duplicated log handling

login
register
mail settings
Submitter Andy Whitcroft
Date Feb. 10, 2010, 10:13 a.m.
Message ID <1265796839-15820-2-git-send-email-apw@canonical.com>
Download mbox | patch
Permalink /patch/45005/
State Changes Requested
Delegated to: Andy Whitcroft
Headers show

Comments

Andy Whitcroft - Feb. 10, 2010, 10:13 a.m.
We have two copies of the handling for Bug: et al in get-ubuntu-log
only differing in whether Ignore: handling is active.  Refactor this
code to share a single copy of this processing.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 debian/scripts/misc/git-ubuntu-log |   45 +++++++++++++----------------------
 1 files changed, 17 insertions(+), 28 deletions(-)
Stefan Bader - Feb. 10, 2010, 11:19 a.m.
Andy Whitcroft wrote:
> We have two copies of the handling for Bug: et al in get-ubuntu-log
> only differing in whether Ignore: handling is active.  Refactor this
> code to share a single copy of this processing.
> 
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
> ---
>  debian/scripts/misc/git-ubuntu-log |   45 +++++++++++++----------------------
>  1 files changed, 17 insertions(+), 28 deletions(-)
> 
> diff --git a/debian/scripts/misc/git-ubuntu-log b/debian/scripts/misc/git-ubuntu-log
> index 666e831..b7ca943 100755
> --- a/debian/scripts/misc/git-ubuntu-log
> +++ b/debian/scripts/misc/git-ubuntu-log
> @@ -168,6 +168,7 @@ sub changelog_input {
>  		elsif ($pstate == 4) {
>  			next unless /^\s*?(.*)/;
>  			my $ignore = 0;
> +			my $do_ignore = 0;
>  			my $bug = undef;
>  			my %bugz = ();
>  			my $k;
> @@ -180,42 +181,30 @@ sub changelog_input {
>  			$desc = $1;
>  
>  			if ($desc =~ /^ *(Revert "|)UBUNTU:/) {
> -				while (<STDIN>) {
> -					$ignore  =  1 if /^ *Ignore: yes/i;
> -					if    (/^ *Bug: *(#|)(.*)/i) {
> -						foreach $k (split('(,|)*\s*#', $2)) {
> -							$bugz{$k} = 1 if (($k ne '') and ($k =~ /[0-9]+/));
> -						}
> -					}
> -					elsif (/^ *BugLink: *http.*:\/\/.*\/([0-9]+)/i) {
> -						$bugz{$1} = 1;
> -					}
> -					elsif (/^ *(CVE-.*)/) {
> -						$cve = $1
> -					}
> -					last if /^commit /;
> -				}
> +				$do_ignore = 1;
>  			} else {

> +				$do_ignore = 0;
>  				$author = $kernel_auth;
>  				$ignore = 1 if $desc =~ /Merge /;

I believe this will be not doing what we intended (hopefully not again my lack
of pearl). If do_ignore is 0 then the record will not get ignored, even if it is
a merge (which was intended)

> -				while (<STDIN>) {
> -					if    (/^ *Bug: *(#|)(.*)/i) {
> -						foreach $k (split('(,|)*\s*#', $2)) {
> -							$bugz{$k} = 1 if (($k ne '') and ($k =~ /[0-9]+/));
> -						}
> -					}
> -					elsif (/^ *BugLink: *http.*:\/\/.*\/([0-9]+)/i) {
> -						$bugz{$1} = 1;
> -					}
> -					elsif (/^ *(CVE-.*)/) {
> -						$cve = $1
> +			}
> +			while (<STDIN>) {

> +				$ignore  =  1 if /^ *Ignore: yes/i;

If it would be possible to have it set ignore to 1 if the match is true and
do_ignore is true, then it might be what we want and you can make the below more
smb safe.

> +				if    (/^ *Bug: *(#|)(.*)/i) {
> +					foreach $k (split('(,|)*\s*#', $2)) {
> +						$bugz{$k} = 1 if (($k ne '') and ($k =~ /[0-9]+/));
>  					}
> -					last if /^commit /;
>  				}
> +				elsif (/^ *BugLink: *http.*:\/\/.*\/([0-9]+)/i) {
> +					$bugz{$1} = 1;
> +				}
> +				elsif (/^ *(CVE-.*)/) {
> +					$cve = $1
> +				}
> +				last if /^commit /;
>  			}
>  
>                          $bug = join(", #", keys(%bugz));
> -			if (!$ignore) {
> +			if (!$do_ignore || !$ignore) {

Could be if (!ignore) again if the above change is possible and what is intended.


-Stefan

>  				&shortlog_entry($author, $desc, $bug,
>  						$cve, $commit, 0);
>  			}
Andy Whitcroft - Feb. 10, 2010, 12:09 p.m.
On Wed, Feb 10, 2010 at 12:19:57PM +0100, Stefan Bader wrote:

> > +				$do_ignore = 0;
> >  				$author = $kernel_auth;
> >  				$ignore = 1 if $desc =~ /Merge /;
> 
> I believe this will be not doing what we intended (hopefully not again my lack
> of pearl). If do_ignore is 0 then the record will not get ignored, even if it is
> a merge (which was intended)

Well spotted.  I'll sort that out and submit it.

-apw

Patch

diff --git a/debian/scripts/misc/git-ubuntu-log b/debian/scripts/misc/git-ubuntu-log
index 666e831..b7ca943 100755
--- a/debian/scripts/misc/git-ubuntu-log
+++ b/debian/scripts/misc/git-ubuntu-log
@@ -168,6 +168,7 @@  sub changelog_input {
 		elsif ($pstate == 4) {
 			next unless /^\s*?(.*)/;
 			my $ignore = 0;
+			my $do_ignore = 0;
 			my $bug = undef;
 			my %bugz = ();
 			my $k;
@@ -180,42 +181,30 @@  sub changelog_input {
 			$desc = $1;
 
 			if ($desc =~ /^ *(Revert "|)UBUNTU:/) {
-				while (<STDIN>) {
-					$ignore  =  1 if /^ *Ignore: yes/i;
-					if    (/^ *Bug: *(#|)(.*)/i) {
-						foreach $k (split('(,|)*\s*#', $2)) {
-							$bugz{$k} = 1 if (($k ne '') and ($k =~ /[0-9]+/));
-						}
-					}
-					elsif (/^ *BugLink: *http.*:\/\/.*\/([0-9]+)/i) {
-						$bugz{$1} = 1;
-					}
-					elsif (/^ *(CVE-.*)/) {
-						$cve = $1
-					}
-					last if /^commit /;
-				}
+				$do_ignore = 1;
 			} else {
+				$do_ignore = 0;
 				$author = $kernel_auth;
 				$ignore = 1 if $desc =~ /Merge /;
-				while (<STDIN>) {
-					if    (/^ *Bug: *(#|)(.*)/i) {
-						foreach $k (split('(,|)*\s*#', $2)) {
-							$bugz{$k} = 1 if (($k ne '') and ($k =~ /[0-9]+/));
-						}
-					}
-					elsif (/^ *BugLink: *http.*:\/\/.*\/([0-9]+)/i) {
-						$bugz{$1} = 1;
-					}
-					elsif (/^ *(CVE-.*)/) {
-						$cve = $1
+			}
+			while (<STDIN>) {
+				$ignore  =  1 if /^ *Ignore: yes/i;
+				if    (/^ *Bug: *(#|)(.*)/i) {
+					foreach $k (split('(,|)*\s*#', $2)) {
+						$bugz{$k} = 1 if (($k ne '') and ($k =~ /[0-9]+/));
 					}
-					last if /^commit /;
 				}
+				elsif (/^ *BugLink: *http.*:\/\/.*\/([0-9]+)/i) {
+					$bugz{$1} = 1;
+				}
+				elsif (/^ *(CVE-.*)/) {
+					$cve = $1
+				}
+				last if /^commit /;
 			}
 
                         $bug = join(", #", keys(%bugz));
-			if (!$ignore) {
+			if (!$do_ignore || !$ignore) {
 				&shortlog_entry($author, $desc, $bug,
 						$cve, $commit, 0);
 			}