| Message ID | 1411673446-4593-1-git-send-email-scottwood@freescale.com |
|---|---|
| State | Accepted |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Scott, On 25 September 2014 13:30, Scott Wood <scottwood@freescale.com> wrote: > True commit lines start at column zero. Anything that is indented > is part of the commit message instead. I noticed this by trying to > run buildman with commit e3a4facdfc07179ebe017a07b8de6224a935a9f3 > as master, which contained a reference to a Linux commit inside > the commit message. ProcessLine saw that as a genuite commit > line, and thus buildman tried to build it, and died with an > exception because that SHA is not present in the U-Boot tree. > > Signed-off-by: Scott Wood <scottwood@freescale.com> > --- > tools/patman/patchstream.py | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py > index d630157..68e98b9 100644 > --- a/tools/patman/patchstream.py > +++ b/tools/patman/patchstream.py > @@ -139,6 +139,9 @@ class PatchStream: > # Initially we have no output. Prepare the input line string > out = [] > line = line.rstrip('\n') > + > + commit_match = re_commit.match(line) if self.is_log else None > + > if self.is_log: > if line[:4] == ' ': > line = line[4:] > @@ -146,7 +149,6 @@ class PatchStream: > # Handle state transition and skipping blank lines > series_tag_match = re_series_tag.match(line) > commit_tag_match = re_commit_tag.match(line) > - commit_match = re_commit.match(line) if self.is_log else None > cover_cc_match = re_cover_cc.match(line) > signoff_match = re_signoff.match(line) > tag_match = None > -- > 1.9.1 > Thanks for finding this bug. This could use a test in tools/patman/test.py The problem is that you are breaking the patch-processing part of this code. It operates in two modes - see the comment at the top of ProcessLine(). With your change it will not process patches correctly, e.g. to add Commit-notes: to the patch. Regards, Simon
On Sun, 2014-09-28 at 12:04 -0600, Simon Glass wrote: > Hi Scott, > > On 25 September 2014 13:30, Scott Wood <scottwood@freescale.com> wrote: > > True commit lines start at column zero. Anything that is indented > > is part of the commit message instead. I noticed this by trying to > > run buildman with commit e3a4facdfc07179ebe017a07b8de6224a935a9f3 > > as master, which contained a reference to a Linux commit inside > > the commit message. ProcessLine saw that as a genuite commit > > line, and thus buildman tried to build it, and died with an > > exception because that SHA is not present in the U-Boot tree. > > > > Signed-off-by: Scott Wood <scottwood@freescale.com> > > --- > > tools/patman/patchstream.py | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py > > index d630157..68e98b9 100644 > > --- a/tools/patman/patchstream.py > > +++ b/tools/patman/patchstream.py > > @@ -139,6 +139,9 @@ class PatchStream: > > # Initially we have no output. Prepare the input line string > > out = [] > > line = line.rstrip('\n') > > + > > + commit_match = re_commit.match(line) if self.is_log else None > > + > > if self.is_log: > > if line[:4] == ' ': > > line = line[4:] > > @@ -146,7 +149,6 @@ class PatchStream: > > # Handle state transition and skipping blank lines > > series_tag_match = re_series_tag.match(line) > > commit_tag_match = re_commit_tag.match(line) > > - commit_match = re_commit.match(line) if self.is_log else None > > cover_cc_match = re_cover_cc.match(line) > > signoff_match = re_signoff.match(line) > > tag_match = None > > -- > > 1.9.1 > > > > Thanks for finding this bug. > > This could use a test in tools/patman/test.py > > The problem is that you are breaking the patch-processing part of this > code. It operates in two modes - see the comment at the top of > ProcessLine(). With your change it will not process patches correctly, > e.g. to add Commit-notes: to the patch. How would this patch would make any difference in the "self.is_log == False" case? The whitespace removal that this patch reorders around won't be executed, and commit_match will be None regardless. -Scott
Hi Scott, On 29 September 2014 10:22, Scott Wood <scottwood@freescale.com> wrote: > On Sun, 2014-09-28 at 12:04 -0600, Simon Glass wrote: >> Hi Scott, >> >> On 25 September 2014 13:30, Scott Wood <scottwood@freescale.com> wrote: >> > True commit lines start at column zero. Anything that is indented >> > is part of the commit message instead. I noticed this by trying to >> > run buildman with commit e3a4facdfc07179ebe017a07b8de6224a935a9f3 >> > as master, which contained a reference to a Linux commit inside >> > the commit message. ProcessLine saw that as a genuite commit >> > line, and thus buildman tried to build it, and died with an >> > exception because that SHA is not present in the U-Boot tree. >> > >> > Signed-off-by: Scott Wood <scottwood@freescale.com> >> > --- >> > tools/patman/patchstream.py | 4 +++- >> > 1 file changed, 3 insertions(+), 1 deletion(-) >> > >> > diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py >> > index d630157..68e98b9 100644 >> > --- a/tools/patman/patchstream.py >> > +++ b/tools/patman/patchstream.py >> > @@ -139,6 +139,9 @@ class PatchStream: >> > # Initially we have no output. Prepare the input line string >> > out = [] >> > line = line.rstrip('\n') >> > + >> > + commit_match = re_commit.match(line) if self.is_log else None >> > + >> > if self.is_log: >> > if line[:4] == ' ': >> > line = line[4:] >> > @@ -146,7 +149,6 @@ class PatchStream: >> > # Handle state transition and skipping blank lines >> > series_tag_match = re_series_tag.match(line) >> > commit_tag_match = re_commit_tag.match(line) >> > - commit_match = re_commit.match(line) if self.is_log else None >> > cover_cc_match = re_cover_cc.match(line) >> > signoff_match = re_signoff.match(line) >> > tag_match = None >> > -- >> > 1.9.1 >> > >> >> Thanks for finding this bug. >> >> This could use a test in tools/patman/test.py >> >> The problem is that you are breaking the patch-processing part of this >> code. It operates in two modes - see the comment at the top of >> ProcessLine(). With your change it will not process patches correctly, >> e.g. to add Commit-notes: to the patch. > > How would this patch would make any difference in the "self.is_log == > False" case? The whitespace removal that this patch reorders around > won't be executed, and commit_match will be None regardless. You are changing it so that commit_match will always be None for patches. This means that the commit message in a patch will never be found, so the state machine that tracks where it is in the patch will not function. Try adding something like: Commit-notes: this is a note more stuff END to a commit and then run patman. You will see that the commit notes are not parsed. Regards, Simon
On Tue, 2014-09-30 at 20:25 -0600, Simon Glass wrote: > Hi Scott, > > On 29 September 2014 10:22, Scott Wood <scottwood@freescale.com> wrote: > > On Sun, 2014-09-28 at 12:04 -0600, Simon Glass wrote: > >> Hi Scott, > >> > >> On 25 September 2014 13:30, Scott Wood <scottwood@freescale.com> wrote: > >> > True commit lines start at column zero. Anything that is indented > >> > is part of the commit message instead. I noticed this by trying to > >> > run buildman with commit e3a4facdfc07179ebe017a07b8de6224a935a9f3 > >> > as master, which contained a reference to a Linux commit inside > >> > the commit message. ProcessLine saw that as a genuite commit > >> > line, and thus buildman tried to build it, and died with an > >> > exception because that SHA is not present in the U-Boot tree. > >> > > >> > Signed-off-by: Scott Wood <scottwood@freescale.com> > >> > --- > >> > tools/patman/patchstream.py | 4 +++- > >> > 1 file changed, 3 insertions(+), 1 deletion(-) > >> > > >> > diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py > >> > index d630157..68e98b9 100644 > >> > --- a/tools/patman/patchstream.py > >> > +++ b/tools/patman/patchstream.py > >> > @@ -139,6 +139,9 @@ class PatchStream: > >> > # Initially we have no output. Prepare the input line string > >> > out = [] > >> > line = line.rstrip('\n') > >> > + > >> > + commit_match = re_commit.match(line) if self.is_log else None > >> > + > >> > if self.is_log: > >> > if line[:4] == ' ': > >> > line = line[4:] > >> > @@ -146,7 +149,6 @@ class PatchStream: > >> > # Handle state transition and skipping blank lines > >> > series_tag_match = re_series_tag.match(line) > >> > commit_tag_match = re_commit_tag.match(line) > >> > - commit_match = re_commit.match(line) if self.is_log else None > >> > cover_cc_match = re_cover_cc.match(line) > >> > signoff_match = re_signoff.match(line) > >> > tag_match = None > >> > -- > >> > 1.9.1 > >> > > >> > >> Thanks for finding this bug. > >> > >> This could use a test in tools/patman/test.py > >> > >> The problem is that you are breaking the patch-processing part of this > >> code. It operates in two modes - see the comment at the top of > >> ProcessLine(). With your change it will not process patches correctly, > >> e.g. to add Commit-notes: to the patch. > > > > How would this patch would make any difference in the "self.is_log == > > False" case? The whitespace removal that this patch reorders around > > won't be executed, and commit_match will be None regardless. > > You are changing it so that commit_match will always be None for patches. If by "for patches" you mean when self.is_log == False, then it was always None. > This means that the commit message in a patch will never be found, so > the state machine that tracks where it is in the patch will not > function. > > Try adding something like: > > Commit-notes: > this is a note > more stuff > END Commit-notes is detected by commit_tag_match, not commit_match. > to a commit and then run patman. You will see that the commit notes > are not parsed. I just tried it and the commit notes work fine. BTW, tools/patman/README says "For most cases of using patman for U-Boot development, patman will locate and use the file 'doc/git-mailrc' in your U-Boot directory. This contains most of the aliases you will need." But, this did not work -- patman threw an exception at me saying alias not found -- until I ran "git config sendemail.aliasesfile doc/git-mailrc" which the README doesn't mention. -Scott
Hi Scott, On 2 October 2014 18:54, Scott Wood <scottwood@freescale.com> wrote: > On Tue, 2014-09-30 at 20:25 -0600, Simon Glass wrote: >> Hi Scott, >> >> On 29 September 2014 10:22, Scott Wood <scottwood@freescale.com> wrote: >> > On Sun, 2014-09-28 at 12:04 -0600, Simon Glass wrote: >> >> Hi Scott, >> >> >> >> On 25 September 2014 13:30, Scott Wood <scottwood@freescale.com> wrote: >> >> > True commit lines start at column zero. Anything that is indented >> >> > is part of the commit message instead. I noticed this by trying to >> >> > run buildman with commit e3a4facdfc07179ebe017a07b8de6224a935a9f3 >> >> > as master, which contained a reference to a Linux commit inside >> >> > the commit message. ProcessLine saw that as a genuite commit >> >> > line, and thus buildman tried to build it, and died with an >> >> > exception because that SHA is not present in the U-Boot tree. >> >> > >> >> > Signed-off-by: Scott Wood <scottwood@freescale.com> >> >> > --- >> >> > tools/patman/patchstream.py | 4 +++- >> >> > 1 file changed, 3 insertions(+), 1 deletion(-) >> >> > >> >> > diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py >> >> > index d630157..68e98b9 100644 >> >> > --- a/tools/patman/patchstream.py >> >> > +++ b/tools/patman/patchstream.py >> >> > @@ -139,6 +139,9 @@ class PatchStream: >> >> > # Initially we have no output. Prepare the input line string >> >> > out = [] >> >> > line = line.rstrip('\n') >> >> > + >> >> > + commit_match = re_commit.match(line) if self.is_log else None >> >> > + >> >> > if self.is_log: >> >> > if line[:4] == ' ': >> >> > line = line[4:] >> >> > @@ -146,7 +149,6 @@ class PatchStream: >> >> > # Handle state transition and skipping blank lines >> >> > series_tag_match = re_series_tag.match(line) >> >> > commit_tag_match = re_commit_tag.match(line) >> >> > - commit_match = re_commit.match(line) if self.is_log else None >> >> > cover_cc_match = re_cover_cc.match(line) >> >> > signoff_match = re_signoff.match(line) >> >> > tag_match = None >> >> > -- >> >> > 1.9.1 >> >> > >> >> >> >> Thanks for finding this bug. >> >> >> >> This could use a test in tools/patman/test.py >> >> >> >> The problem is that you are breaking the patch-processing part of this >> >> code. It operates in two modes - see the comment at the top of >> >> ProcessLine(). With your change it will not process patches correctly, >> >> e.g. to add Commit-notes: to the patch. >> > >> > How would this patch would make any difference in the "self.is_log == >> > False" case? The whitespace removal that this patch reorders around >> > won't be executed, and commit_match will be None regardless. >> >> You are changing it so that commit_match will always be None for patches. > > If by "for patches" you mean when self.is_log == False, then it was > always None. > >> This means that the commit message in a patch will never be found, so >> the state machine that tracks where it is in the patch will not >> function. >> >> Try adding something like: >> >> Commit-notes: >> this is a note >> more stuff >> END > > Commit-notes is detected by commit_tag_match, not commit_match. > >> to a commit and then run patman. You will see that the commit notes >> are not parsed. > > I just tried it and the commit notes work fine. Sorry you are right, I must have typed it incorrectly when I tried it. I just tried it again and it is fine. Acked-by: Simon Glass <sjg@chromium.org> > > BTW, tools/patman/README says "For most cases of using patman for U-Boot > development, patman will locate and use the file 'doc/git-mailrc' in > your U-Boot directory. This contains most of the aliases you will need." > But, this did not work -- patman threw an exception at me saying alias > not found -- until I ran "git config sendemail.aliasesfile > doc/git-mailrc" which the README doesn't mention. OK thanks the report, I sent a patch. Regards, Simon
On 3 October 2014 20:40, Simon Glass <sjg@chromium.org> wrote: > Hi Scott, > > On 2 October 2014 18:54, Scott Wood <scottwood@freescale.com> wrote: >> On Tue, 2014-09-30 at 20:25 -0600, Simon Glass wrote: >>> Hi Scott, >>> >>> On 29 September 2014 10:22, Scott Wood <scottwood@freescale.com> wrote: >>> > On Sun, 2014-09-28 at 12:04 -0600, Simon Glass wrote: >>> >> Hi Scott, >>> >> >>> >> On 25 September 2014 13:30, Scott Wood <scottwood@freescale.com> wrote: >>> >> > True commit lines start at column zero. Anything that is indented >>> >> > is part of the commit message instead. I noticed this by trying to >>> >> > run buildman with commit e3a4facdfc07179ebe017a07b8de6224a935a9f3 >>> >> > as master, which contained a reference to a Linux commit inside >>> >> > the commit message. ProcessLine saw that as a genuite commit >>> >> > line, and thus buildman tried to build it, and died with an >>> >> > exception because that SHA is not present in the U-Boot tree. >>> >> > >>> >> > Signed-off-by: Scott Wood <scottwood@freescale.com> >>> >> > --- >>> >> > tools/patman/patchstream.py | 4 +++- >>> >> > 1 file changed, 3 insertions(+), 1 deletion(-) >>> >> > >>> >> > diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py >>> >> > index d630157..68e98b9 100644 >>> >> > --- a/tools/patman/patchstream.py >>> >> > +++ b/tools/patman/patchstream.py >>> >> > @@ -139,6 +139,9 @@ class PatchStream: >>> >> > # Initially we have no output. Prepare the input line string >>> >> > out = [] >>> >> > line = line.rstrip('\n') >>> >> > + >>> >> > + commit_match = re_commit.match(line) if self.is_log else None >>> >> > + >>> >> > if self.is_log: >>> >> > if line[:4] == ' ': >>> >> > line = line[4:] >>> >> > @@ -146,7 +149,6 @@ class PatchStream: >>> >> > # Handle state transition and skipping blank lines >>> >> > series_tag_match = re_series_tag.match(line) >>> >> > commit_tag_match = re_commit_tag.match(line) >>> >> > - commit_match = re_commit.match(line) if self.is_log else None >>> >> > cover_cc_match = re_cover_cc.match(line) >>> >> > signoff_match = re_signoff.match(line) >>> >> > tag_match = None >>> >> > -- >>> >> > 1.9.1 >>> >> > >>> >> >>> >> Thanks for finding this bug. >>> >> >>> >> This could use a test in tools/patman/test.py >>> >> >>> >> The problem is that you are breaking the patch-processing part of this >>> >> code. It operates in two modes - see the comment at the top of >>> >> ProcessLine(). With your change it will not process patches correctly, >>> >> e.g. to add Commit-notes: to the patch. >>> > >>> > How would this patch would make any difference in the "self.is_log == >>> > False" case? The whitespace removal that this patch reorders around >>> > won't be executed, and commit_match will be None regardless. >>> >>> You are changing it so that commit_match will always be None for patches. >> >> If by "for patches" you mean when self.is_log == False, then it was >> always None. >> >>> This means that the commit message in a patch will never be found, so >>> the state machine that tracks where it is in the patch will not >>> function. >>> >>> Try adding something like: >>> >>> Commit-notes: >>> this is a note >>> more stuff >>> END >> >> Commit-notes is detected by commit_tag_match, not commit_match. >> >>> to a commit and then run patman. You will see that the commit notes >>> are not parsed. >> >> I just tried it and the commit notes work fine. > > Sorry you are right, I must have typed it incorrectly when I tried it. > I just tried it again and it is fine. > > Acked-by: Simon Glass <sjg@chromium.org> Applied to u-boot-x86/patman, thanks! > >> >> BTW, tools/patman/README says "For most cases of using patman for U-Boot >> development, patman will locate and use the file 'doc/git-mailrc' in >> your U-Boot directory. This contains most of the aliases you will need." >> But, this did not work -- patman threw an exception at me saying alias >> not found -- until I ran "git config sendemail.aliasesfile >> doc/git-mailrc" which the README doesn't mention. > > OK thanks the report, I sent a patch. > > Regards, > Simon
diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py index d630157..68e98b9 100644 --- a/tools/patman/patchstream.py +++ b/tools/patman/patchstream.py @@ -139,6 +139,9 @@ class PatchStream: # Initially we have no output. Prepare the input line string out = [] line = line.rstrip('\n') + + commit_match = re_commit.match(line) if self.is_log else None + if self.is_log: if line[:4] == ' ': line = line[4:] @@ -146,7 +149,6 @@ class PatchStream: # Handle state transition and skipping blank lines series_tag_match = re_series_tag.match(line) commit_tag_match = re_commit_tag.match(line) - commit_match = re_commit.match(line) if self.is_log else None cover_cc_match = re_cover_cc.match(line) signoff_match = re_signoff.match(line) tag_match = None
True commit lines start at column zero. Anything that is indented is part of the commit message instead. I noticed this by trying to run buildman with commit e3a4facdfc07179ebe017a07b8de6224a935a9f3 as master, which contained a reference to a Linux commit inside the commit message. ProcessLine saw that as a genuite commit line, and thus buildman tried to build it, and died with an exception because that SHA is not present in the U-Boot tree. Signed-off-by: Scott Wood <scottwood@freescale.com> --- tools/patman/patchstream.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)