| Message ID | 1402550829-13141-1-git-send-email-sjg@chromium.org |
|---|---|
| State | Accepted |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Masahiro, On 11 June 2014 23:27, Simon Glass <sjg@chromium.org> wrote: > When patman applies the patches it checks out a new branch, uses 'git am' > to apply the patches one by one, and then tries to go back to the old > branch. If you try this when the branch is 'undefined', this doesn't work > as patman cannot restore the correct branch after applying the patches. > It seems that 'undefined' is created by git and is persistent after it is > created, so that you can end up on quite an old branch. > > Add a check for the 'undefined' branch to avoid this. > > Reported-by: Masahiro Yamada <yamada.m@jp.panasonic.com> > Signed-off-by: Simon Glass <sjg@chromium.org> Can you please check if this fixes the problem you reported? If so I would like to get it into this release. Regards, Simon
Hi Simon, On Sat, 21 Jun 2014 10:27:06 -0600 Simon Glass <sjg@chromium.org> wrote: > Hi Masahiro, > > On 11 June 2014 23:27, Simon Glass <sjg@chromium.org> wrote: > > When patman applies the patches it checks out a new branch, uses 'git am' > > to apply the patches one by one, and then tries to go back to the old > > branch. If you try this when the branch is 'undefined', this doesn't work > > as patman cannot restore the correct branch after applying the patches. > > It seems that 'undefined' is created by git and is persistent after it is > > created, so that you can end up on quite an old branch. > > > > Add a check for the 'undefined' branch to avoid this. > > > > Reported-by: Masahiro Yamada <yamada.m@jp.panasonic.com> > > Signed-off-by: Simon Glass <sjg@chromium.org> > > Can you please check if this fixes the problem you reported? If so I > would like to get it into this release. I cannot understand why patman needs to apply the patches with 'git am'. Why isn't patman like this ? [1] Generate patches with 'git format-patch' [2] Parse the patman-tags in the generated patches and edit them. [3] Send the patches with 'git send-email' Best Regards Masahiro Yamada
Hi Masahiro, On 22 June 2014 21:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote: > Hi Simon, > > > On Sat, 21 Jun 2014 10:27:06 -0600 > Simon Glass <sjg@chromium.org> wrote: > >> Hi Masahiro, >> >> On 11 June 2014 23:27, Simon Glass <sjg@chromium.org> wrote: >> > When patman applies the patches it checks out a new branch, uses 'git am' >> > to apply the patches one by one, and then tries to go back to the old >> > branch. If you try this when the branch is 'undefined', this doesn't work >> > as patman cannot restore the correct branch after applying the patches. >> > It seems that 'undefined' is created by git and is persistent after it is >> > created, so that you can end up on quite an old branch. >> > >> > Add a check for the 'undefined' branch to avoid this. >> > >> > Reported-by: Masahiro Yamada <yamada.m@jp.panasonic.com> >> > Signed-off-by: Simon Glass <sjg@chromium.org> >> >> Can you please check if this fixes the problem you reported? If so I >> would like to get it into this release. > > > I cannot understand why patman needs to apply the patches with 'git am'. > > Why isn't patman like this ? > [1] Generate patches with 'git format-patch' > [2] Parse the patman-tags in the generated patches and edit them. 2a. Apply the patches to make sure there are no whitespace errors. > [3] Send the patches with 'git send-email' > The whitespace problems are not common but they do happen sometimes - or at least I have seen it at times. Unless perhaps checkpatch has got smarter? > > Best Regards > Masahiro Yamada > Regards, Simon
Hi Simon, On Sun, 22 Jun 2014 21:39:26 -0600 Simon Glass <sjg@chromium.org> wrote: > Hi Masahiro, > > On 22 June 2014 21:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote: > > Hi Simon, > > > > > > On Sat, 21 Jun 2014 10:27:06 -0600 > > Simon Glass <sjg@chromium.org> wrote: > > > >> Hi Masahiro, > >> > >> On 11 June 2014 23:27, Simon Glass <sjg@chromium.org> wrote: > >> > When patman applies the patches it checks out a new branch, uses 'git am' > >> > to apply the patches one by one, and then tries to go back to the old > >> > branch. If you try this when the branch is 'undefined', this doesn't work > >> > as patman cannot restore the correct branch after applying the patches. > >> > It seems that 'undefined' is created by git and is persistent after it is > >> > created, so that you can end up on quite an old branch. > >> > > >> > Add a check for the 'undefined' branch to avoid this. > >> > > >> > Reported-by: Masahiro Yamada <yamada.m@jp.panasonic.com> > >> > Signed-off-by: Simon Glass <sjg@chromium.org> > >> > >> Can you please check if this fixes the problem you reported? If so I > >> would like to get it into this release. > > > > > > I cannot understand why patman needs to apply the patches with 'git am'. > > > > Why isn't patman like this ? > > [1] Generate patches with 'git format-patch' > > [2] Parse the patman-tags in the generated patches and edit them. > > 2a. Apply the patches to make sure there are no whitespace errors. > > > [3] Send the patches with 'git send-email' > > > > The whitespace problems are not common but they do happen sometimes - > or at least I have seen it at times. Unless perhaps checkpatch has got > smarter? I still don't understand. Anyway, Patman invokes checkpatch.pl, right? Isn't whitespace error checking of checkpatch nice? Best Regards Masahiro Yamada
On Mon, Jun 23, 2014 at 12:39 AM, Simon Glass <sjg@chromium.org> wrote: > The whitespace problems are not common but they do happen sometimes - > or at least I have seen it at times. Unless perhaps checkpatch has got > smarter? Couldn't ./scripts/cleanpatch be used to fix the whitespace issues?
Hi Masahiro, On Jun 22, 2014 9:51 PM, "Masahiro Yamada" <yamada.m@jp.panasonic.com> wrote: > > Hi Simon, > > On Sun, 22 Jun 2014 21:39:26 -0600 > Simon Glass <sjg@chromium.org> wrote: > > > Hi Masahiro, > > > > On 22 June 2014 21:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote: > > > Hi Simon, > > > > > > > > > On Sat, 21 Jun 2014 10:27:06 -0600 > > > Simon Glass <sjg@chromium.org> wrote: > > > > > >> Hi Masahiro, > > >> > > >> On 11 June 2014 23:27, Simon Glass <sjg@chromium.org> wrote: > > >> > When patman applies the patches it checks out a new branch, uses 'git am' > > >> > to apply the patches one by one, and then tries to go back to the old > > >> > branch. If you try this when the branch is 'undefined', this doesn't work > > >> > as patman cannot restore the correct branch after applying the patches. > > >> > It seems that 'undefined' is created by git and is persistent after it is > > >> > created, so that you can end up on quite an old branch. > > >> > > > >> > Add a check for the 'undefined' branch to avoid this. > > >> > > > >> > Reported-by: Masahiro Yamada <yamada.m@jp.panasonic.com> > > >> > Signed-off-by: Simon Glass <sjg@chromium.org> > > >> > > >> Can you please check if this fixes the problem you reported? If so I > > >> would like to get it into this release. > > > > > > > > > I cannot understand why patman needs to apply the patches with 'git am'. > > > > > > Why isn't patman like this ? > > > [1] Generate patches with 'git format-patch' > > > [2] Parse the patman-tags in the generated patches and edit them. > > > > 2a. Apply the patches to make sure there are no whitespace errors. > > > > > [3] Send the patches with 'git send-email' > > > > > > > The whitespace problems are not common but they do happen sometimes - > > or at least I have seen it at times. Unless perhaps checkpatch has got > > smarter? > > I still don't understand. > Anyway, Patman invokes checkpatch.pl, right? > Isn't whitespace error checking of checkpatch nice? Not when I wrote patman, no. Maybe it has changed? I could perhaps invert the meaning of the -a flag :-) Regards, Simon > > > Best Regards > Masahiro Yamada >
Hi, On Jun 22, 2014 10:23 PM, "Fabio Estevam" <festevam@gmail.com> wrote: > > On Mon, Jun 23, 2014 at 12:39 AM, Simon Glass <sjg@chromium.org> wrote: > > > The whitespace problems are not common but they do happen sometimes - > > or at least I have seen it at times. Unless perhaps checkpatch has got > > smarter? > > Couldn't ./scripts/cleanpatch be used to fix the whitespace issues? Maybe. If you know how to test this please give it a try. Regards, Simon
Hi Simon, On Sun, 22 Jun 2014 22:54:43 -0600 Simon Glass <sjg@chromium.org> wrote: > Hi, > > On Jun 22, 2014 10:23 PM, "Fabio Estevam" <festevam@gmail.com> wrote: > > > > On Mon, Jun 23, 2014 at 12:39 AM, Simon Glass <sjg@chromium.org> wrote: > > > > > The whitespace problems are not common but they do happen sometimes - > > > or at least I have seen it at times. Unless perhaps checkpatch has got > > > smarter? > > > > Couldn't ./scripts/cleanpatch be used to fix the whitespace issues? > > Maybe. If you know how to test this please give it a try. Does Patman fix the whitespace issues? I'm getting confused. To make things clearer, please let me ask you some questions. [1] What does "whitespace errors" mean here? - Trailing whitespaces ? - Spaces before TAB indent ? - Or what else ? [2] What do you expect Patman to do when it finds those errors? - Just display an error message ? or - Fix the issues automatically ? [3] Which features were missing from scripts/checkpatch.pl when you wrote Patman first ? Best Regards Masahiro Yamada
+Doug who may have some thoughts here. HI Masahiro, On 23 June 2014 00:28, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote: > Hi Simon, > > On Sun, 22 Jun 2014 22:54:43 -0600 > Simon Glass <sjg@chromium.org> wrote: > > > Hi, > > > > On Jun 22, 2014 10:23 PM, "Fabio Estevam" <festevam@gmail.com> wrote: > > > > > > On Mon, Jun 23, 2014 at 12:39 AM, Simon Glass <sjg@chromium.org> > wrote: > > > > > > > The whitespace problems are not common but they do happen sometimes - > > > > or at least I have seen it at times. Unless perhaps checkpatch has > got > > > > smarter? > > > > > > Couldn't ./scripts/cleanpatch be used to fix the whitespace issues? > > > > Maybe. If you know how to test this please give it a try. > > Note, I do understand all the questions, but the problem is I am not completely sure how this happens. > Does Patman fix the whitespace issues? > > No, it just runs 'git am' which will then report them. > > I'm getting confused. > > To make things clearer, please let me ask you some questions. > > [1] What does "whitespace errors" mean here? > > - Trailing whitespaces ? > - Spaces before TAB indent ? > - Or what else ? > Both of those, plus a blank line at EOF as I understand it. > > [2] What do you expect Patman to do when it finds those errors? > - Just display an error message ? > Yes (it displays an error from 'git am') > or > - Fix the issues automatically ? > No - apart from removing tags and adding notes patman avoids changing patches as a matter of policy. > > [3] Which features were missing from scripts/checkpatch.pl > when you wrote Patman first ? > It's really a belts-and-braces check that nothing will go wrong when the patches are applied. I have had it report errors to me on my own patches, although not recently. I'm not sure if it is still needed though. Regards, Simon
On Mon, 23 Jun 2014 06:11:28 -0600 Simon Glass <sjg@chromium.org> wrote: > +Doug who may have some thoughts here. > > HI Masahiro, > > On 23 June 2014 00:28, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote: > > > Hi Simon, > > > > On Sun, 22 Jun 2014 22:54:43 -0600 > > Simon Glass <sjg@chromium.org> wrote: > > > > > Hi, > > > > > > On Jun 22, 2014 10:23 PM, "Fabio Estevam" <festevam@gmail.com> wrote: > > > > > > > > On Mon, Jun 23, 2014 at 12:39 AM, Simon Glass <sjg@chromium.org> > > wrote: > > > > > > > > > The whitespace problems are not common but they do happen sometimes - > > > > > or at least I have seen it at times. Unless perhaps checkpatch has > > got > > > > > smarter? > > > > > > > > Couldn't ./scripts/cleanpatch be used to fix the whitespace issues? > > > > > > Maybe. If you know how to test this please give it a try. > > > > Note, I do understand all the questions, but the problem is I am not > completely sure how this happens. > > > > Does Patman fix the whitespace issues? > > > > No, it just runs 'git am' which will then report them. > > > > > > I'm getting confused. > > > > To make things clearer, please let me ask you some questions. > > > > [1] What does "whitespace errors" mean here? > > > > - Trailing whitespaces ? > > - Spaces before TAB indent ? > > - Or what else ? > > > > Both of those, plus a blank line at EOF as I understand it. > > > > > > [2] What do you expect Patman to do when it finds those errors? > > - Just display an error message ? > > > > Yes (it displays an error from 'git am') > > > > or > > - Fix the issues automatically ? > > > > No - apart from removing tags and adding notes patman avoids changing > patches as a matter of policy. > > > > > [3] Which features were missing from scripts/checkpatch.pl > > when you wrote Patman first ? > > > > It's really a belts-and-braces check that nothing will go wrong when the > patches are applied. I have had it report errors to me on my own patches, > although not recently. I'm not sure if it is still needed though. OK. It seems better to check. I added some patches which include whitespace errors and ran Patman. It looks like nothing changes with/without -a option. $ tools/patman/patman -a -c 3 1 warnings for 0002-arm-commit-2.patch: Line 21/0 has space before tab 1 warnings for 0003-arm-commit-3.patch: Found possible blank line(s) at end of file 'None' Cleaned 3 patches 1 errors, 0 warnings, 0 checks for 0001-arm-commit-1.patch: error: common/cmd_mmc.c,22: trailing whitespace 1 errors, 2 warnings, 0 checks for 0002-arm-commit-2.patch: error: common/cmd_mmc.c,26: code indent should use tabs where possible warning: common/cmd_mmc.c,26: please, no space before tabs warning: common/cmd_mmc.c,26: please, no spaces at the start of a line checkpatch.pl found 2 error(s), 2 warning(s), 0 checks(s) $ tools/patman/patman -c 3 1 warnings for 0002-arm-commit-2.patch: Line 21/0 has space before tab 1 warnings for 0003-arm-commit-3.patch: Found possible blank line(s) at end of file 'None' Cleaned 3 patches 1 errors, 0 warnings, 0 checks for 0001-arm-commit-1.patch: error: common/cmd_mmc.c,22: trailing whitespace 1 errors, 2 warnings, 0 checks for 0002-arm-commit-2.patch: error: common/cmd_mmc.c,26: code indent should use tabs where possible warning: common/cmd_mmc.c,26: please, no space before tabs warning: common/cmd_mmc.c,26: please, no spaces at the start of a line checkpatch.pl found 2 error(s), 2 warning(s), 0 checks(s) Best Regards Masahiro Yamada
Hi, On Tue, Jun 24, 2014 at 3:24 AM, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote: > OK. It seems better to check. > > I added some patches which include whitespace errors > and ran Patman. > > It looks like nothing changes with/without -a option. > > > $ tools/patman/patman -a -c 3 > 1 warnings for 0002-arm-commit-2.patch: > Line 21/0 has space before tab > > 1 warnings for 0003-arm-commit-3.patch: > Found possible blank line(s) at end of file 'None' > > Cleaned 3 patches > 1 errors, 0 warnings, 0 checks for 0001-arm-commit-1.patch: > error: common/cmd_mmc.c,22: trailing whitespace > > 1 errors, 2 warnings, 0 checks for 0002-arm-commit-2.patch: > error: common/cmd_mmc.c,26: code indent should use tabs where possible > warning: common/cmd_mmc.c,26: please, no space before tabs > warning: common/cmd_mmc.c,26: please, no spaces at the start of a line > > checkpatch.pl found 2 error(s), 2 warning(s), 0 checks(s) > > > $ tools/patman/patman -c 3 > 1 warnings for 0002-arm-commit-2.patch: > Line 21/0 has space before tab > > 1 warnings for 0003-arm-commit-3.patch: > Found possible blank line(s) at end of file 'None' > > Cleaned 3 patches > 1 errors, 0 warnings, 0 checks for 0001-arm-commit-1.patch: > error: common/cmd_mmc.c,22: trailing whitespace > > 1 errors, 2 warnings, 0 checks for 0002-arm-commit-2.patch: > error: common/cmd_mmc.c,26: code indent should use tabs where possible > warning: common/cmd_mmc.c,26: please, no space before tabs > warning: common/cmd_mmc.c,26: please, no spaces at the start of a line > > checkpatch.pl found 2 error(s), 2 warning(s), 0 checks(s) It sure looks like checkpatch now catches things so it never gets to "git am". Making "-a" off by default seems reasonable to me. -Doug
Hi, On 24 June 2014 14:40, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Tue, Jun 24, 2014 at 3:24 AM, Masahiro Yamada > <yamada.m@jp.panasonic.com> wrote: > > OK. It seems better to check. > > > > I added some patches which include whitespace errors > > and ran Patman. > > > > It looks like nothing changes with/without -a option. > > > > > > $ tools/patman/patman -a -c 3 > > 1 warnings for 0002-arm-commit-2.patch: > > Line 21/0 has space before tab > > > > 1 warnings for 0003-arm-commit-3.patch: > > Found possible blank line(s) at end of file 'None' > > > > Cleaned 3 patches > > 1 errors, 0 warnings, 0 checks for 0001-arm-commit-1.patch: > > error: common/cmd_mmc.c,22: trailing whitespace > > > > 1 errors, 2 warnings, 0 checks for 0002-arm-commit-2.patch: > > error: common/cmd_mmc.c,26: code indent should use tabs where possible > > warning: common/cmd_mmc.c,26: please, no space before tabs > > warning: common/cmd_mmc.c,26: please, no spaces at the start of a line > > > > checkpatch.pl found 2 error(s), 2 warning(s), 0 checks(s) > > > > > > $ tools/patman/patman -c 3 > > 1 warnings for 0002-arm-commit-2.patch: > > Line 21/0 has space before tab > > > > 1 warnings for 0003-arm-commit-3.patch: > > Found possible blank line(s) at end of file 'None' > > > > Cleaned 3 patches > > 1 errors, 0 warnings, 0 checks for 0001-arm-commit-1.patch: > > error: common/cmd_mmc.c,22: trailing whitespace > > > > 1 errors, 2 warnings, 0 checks for 0002-arm-commit-2.patch: > > error: common/cmd_mmc.c,26: code indent should use tabs where possible > > warning: common/cmd_mmc.c,26: please, no space before tabs > > warning: common/cmd_mmc.c,26: please, no spaces at the start of a line > > > > checkpatch.pl found 2 error(s), 2 warning(s), 0 checks(s) > > It sure looks like checkpatch now catches things so it never gets to > "git am". Making "-a" off by default seems reasonable to me. Well, good. Still, we need this patch to fix the bug I thihnk. I'll do a new patch to invert the flag too. Regards, Simon
Hi Masahiro, On 24 June 2014 17:56, Simon Glass <sjg@chromium.org> wrote: > Hi, > > On 24 June 2014 14:40, Doug Anderson <dianders@chromium.org> wrote: >> It sure looks like checkpatch now catches things so it never gets to >> "git am". Making "-a" off by default seems reasonable to me. > > > Well, good. > > Still, we need this patch to fix the bug I thihnk. Aside from the discussion about apply patches, does this fix the bug you reported? > > I'll do a new patch to invert the flag too. > > Regards, > Simon Regards, Simon
Hi Simon, On Wed, 25 Jun 2014 10:53:08 -0600 Simon Glass <sjg@chromium.org> wrote: > Hi Masahiro, > > On 24 June 2014 17:56, Simon Glass <sjg@chromium.org> wrote: > > Hi, > > > > On 24 June 2014 14:40, Doug Anderson <dianders@chromium.org> wrote: > >> It sure looks like checkpatch now catches things so it never gets to > >> "git am". Making "-a" off by default seems reasonable to me. > > > > > > Well, good. > > > > Still, we need this patch to fix the bug I thihnk. > > Aside from the discussion about apply patches, does this fix the bug > you reported? Yes, that fixes the issue. Thanks! But, is '-a' option still necessary? Shoud we rip it off someday? Best Regards Masahiro Yamada
Hi Masahiro, On 25 June 2014 19:03, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote: > Hi Simon, > > > On Wed, 25 Jun 2014 10:53:08 -0600 > Simon Glass <sjg@chromium.org> wrote: > >> Hi Masahiro, >> >> On 24 June 2014 17:56, Simon Glass <sjg@chromium.org> wrote: >> > Hi, >> > >> > On 24 June 2014 14:40, Doug Anderson <dianders@chromium.org> wrote: >> >> It sure looks like checkpatch now catches things so it never gets to >> >> "git am". Making "-a" off by default seems reasonable to me. >> > >> > >> > Well, good. >> > >> > Still, we need this patch to fix the bug I thihnk. >> >> Aside from the discussion about apply patches, does this fix the bug >> you reported? > > Yes, that fixes the issue. Thanks! > But, is '-a' option still necessary? > Shoud we rip it off someday? I'm in favour of this, perhaps for the next release. Regards, Simon
Applied via x86 tree.
diff --git a/tools/patman/gitutil.py b/tools/patman/gitutil.py index 3ea256d..7b75c83 100644 --- a/tools/patman/gitutil.py +++ b/tools/patman/gitutil.py @@ -232,6 +232,10 @@ def ApplyPatches(verbose, args, start_point): print stdout return False old_head = stdout.splitlines()[0] + if old_head == 'undefined': + str = "Invalid HEAD '%s'" % stdout.strip() + print col.Color(col.RED, str) + return False # Checkout the required start point cmd = ['git', 'checkout', 'HEAD~%d' % start_point]
When patman applies the patches it checks out a new branch, uses 'git am' to apply the patches one by one, and then tries to go back to the old branch. If you try this when the branch is 'undefined', this doesn't work as patman cannot restore the correct branch after applying the patches. It seems that 'undefined' is created by git and is persistent after it is created, so that you can end up on quite an old branch. Add a check for the 'undefined' branch to avoid this. Reported-by: Masahiro Yamada <yamada.m@jp.panasonic.com> Signed-off-by: Simon Glass <sjg@chromium.org> --- tools/patman/gitutil.py | 4 ++++ 1 file changed, 4 insertions(+)