git-p4.skipSubmitEdit

git-p4.skipSubmitEdit

[L. A. Linden Levy]

[-- Attachment #1: Type: text/plain, Size: 2848 bytes --]

Hi All,

I have been using git-p4 for a while and it has allowed me to completely
change the way I develop and still be able to use perforce which my
company has for its main VCS. One thing that was driving me nuts was
that "git p4 submit" cycles through all of my individual commits and
asks me if I want to change them. The way I develop I often am checking
in 20 to 50 different small commits each with a descriptive git comment.
I felt like I was doing double duty by having emacs open on every commit
into perforce. So I modified git-p4 to have an option to skip the
editor. This option coupled with git-p4.skipSubmitEditCheck will make
the submission non-interactive for "git p4 submit".

Below are the patch and environment results:


$ git config -l
...
user.name=Loren A. Linden Levy
git-p4.skipsubmitedit=true
git-p4.skipsubmiteditcheck=true
...

$ git format-patch origin/master --stdout

From 16c4344de0047cbaf3381eca590a3e59b0d0a25c Mon Sep 17 00:00:00 2001
From: "Loren A. Linden Levy" <lindenle@gmail.com>
Date: Thu, 8 Sep 2011 13:37:22 -0700
Subject: [PATCH] changed git-p4

---
 contrib/fast-import/git-p4 |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 2f7b270..a438b3e 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -935,18 +935,23 @@ class P4Submit(Command, P4UserMap):
             tmpFile.write(submitTemplate + separatorLine + diff +
newdiff)
             tmpFile.close()
             mtime = os.stat(fileName).st_mtime
-            if os.environ.has_key("P4EDITOR"):
-                editor = os.environ.get("P4EDITOR")
+            if gitConfig("git-p4.skipSubmitEdit") == "true":
+                pass
             else:
-                editor = read_pipe("git var GIT_EDITOR").strip()
-            system(editor + " " + fileName)
-
+                if os.environ.has_key("P4EDITOR"):
+                    editor = os.environ.get("P4EDITOR")
+                else:
+                    editor = read_pipe("git var GIT_EDITOR").strip()
+                    
+                    system(editor + " " + fileName)
+                    
             if gitConfig("git-p4.skipSubmitEditCheck") == "true":
                 checkModTime = False
             else:
                 checkModTime = True
 
             response = "y"
+
             if checkModTime and (os.stat(fileName).st_mtime <= mtime):
                 response = "x"
                 while response != "y" and response != "n":
-- 
1.7.6.347.g4db0d


-- 
Alex Linden Levy
Senior Software Engineer
MobiTV, Inc.
6425 Christie Avenue, 5th Floor, Emeryville, CA 94608
phone 510.450.5190 mobile 720.352.8394
email alevy@mobitv.com  web www.mobitv.com



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: git-p4.skipSubmitEdit

[Vitor Antunes]

L. A. Linden Levy <alevy <at> mobitv.com> writes:

> 
> Hi All,
> 
> I have been using git-p4 for a while and it has allowed me to completely
> change the way I develop and still be able to use perforce which my
> company has for its main VCS. One thing that was driving me nuts was
> that "git p4 submit" cycles through all of my individual commits and
> asks me if I want to change them. The way I develop I often am checking
> in 20 to 50 different small commits each with a descriptive git comment.
> I felt like I was doing double duty by having emacs open on every commit
> into perforce. So I modified git-p4 to have an option to skip the
> editor. This option coupled with git-p4.skipSubmitEditCheck will make
> the submission non-interactive for "git p4 submit".

Hi Loren,

This option was already included in a recent commit. The name that was
used is "skipSubmitEditCheck". Please make sure you are using the most
recent version of the script.

But don't let this discourage you from submitting patches. Just makesure
you clone git's repository and apply your patch over "maint" or "master"
branches. For more details on how to submit patches you can read
Documentation/SubmittingPatches.

Vitor

Re: git-p4.skipSubmitEdit

[Luke Diamand]

On 09/09/11 11:05, Vitor Antunes wrote:
> L. A. Linden Levy<alevy<at>  mobitv.com>  writes:
>
>>
>> Hi All,
>>
>> I have been using git-p4 for a while and it has allowed me to completely
>> change the way I develop and still be able to use perforce which my
>> company has for its main VCS. One thing that was driving me nuts was
>> that "git p4 submit" cycles through all of my individual commits and
>> asks me if I want to change them. The way I develop I often am checking
>> in 20 to 50 different small commits each with a descriptive git comment.
>> I felt like I was doing double duty by having emacs open on every commit
>> into perforce. So I modified git-p4 to have an option to skip the
>> editor. This option coupled with git-p4.skipSubmitEditCheck will make
>> the submission non-interactive for "git p4 submit".
>
> Hi Loren,
>
> This option was already included in a recent commit. The name that was
> used is "skipSubmitEditCheck". Please make sure you are using the most
> recent version of the script.

I put that option in - glad it's of use!

That option actually just skips the check of 'did the user edit the 
file'. git-p4 will still go ahead and bring up the file in the editor first.

I get around this myself by setting EDITOR=/bin/true. That works for me 
because I'm only using it in a script.

But it's possible that an additional option would actually be useful.



>
> But don't let this discourage you from submitting patches. Just makesure
> you clone git's repository and apply your patch over "maint" or "master"
> branches. For more details on how to submit patches you can read
> Documentation/SubmittingPatches.
>
> Vitor
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git-p4.skipSubmitEdit

[L. A. Linden Levy]

[-- Attachment #1: Type: text/plain, Size: 2576 bytes --]

I noticed that it only skipped the edit check. That is why I added the
skipSubmitEdit option. If they are both true then it never opens the
editor and never checks for an edit. Probably they should just be one
option. I think it should probably also be a command line option to skip
the editor.

- Alex

On Fri, 2011-09-09 at 13:47 -0400, Luke Diamand wrote:
> On 09/09/11 11:05, Vitor Antunes wrote:
> > L. A. Linden Levy<alevy<at>  mobitv.com>  writes:
> >
> >>
> >> Hi All,
> >>
> >> I have been using git-p4 for a while and it has allowed me to completely
> >> change the way I develop and still be able to use perforce which my
> >> company has for its main VCS. One thing that was driving me nuts was
> >> that "git p4 submit" cycles through all of my individual commits and
> >> asks me if I want to change them. The way I develop I often am checking
> >> in 20 to 50 different small commits each with a descriptive git comment.
> >> I felt like I was doing double duty by having emacs open on every commit
> >> into perforce. So I modified git-p4 to have an option to skip the
> >> editor. This option coupled with git-p4.skipSubmitEditCheck will make
> >> the submission non-interactive for "git p4 submit".
> >
> > Hi Loren,
> >
> > This option was already included in a recent commit. The name that was
> > used is "skipSubmitEditCheck". Please make sure you are using the most
> > recent version of the script.
> 
> I put that option in - glad it's of use!
> 
> That option actually just skips the check of 'did the user edit the 
> file'. git-p4 will still go ahead and bring up the file in the editor first.
> 
> I get around this myself by setting EDITOR=/bin/true. That works for me 
> because I'm only using it in a script.
> 
> But it's possible that an additional option would actually be useful.
> 
> 
> 
> >
> > But don't let this discourage you from submitting patches. Just makesure
> > you clone git's repository and apply your patch over "maint" or "master"
> > branches. For more details on how to submit patches you can read
> > Documentation/SubmittingPatches.
> >
> > Vitor
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe git" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Alex Linden Levy
Senior Software Engineer
MobiTV, Inc.
6425 Christie Avenue, 5th Floor, Emeryville, CA 94608
phone 510.450.5190 mobile 720.352.8394
email alevy@mobitv.com  web www.mobitv.com


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: git-p4.skipSubmitEdit

[Luke Diamand]

On 09/09/11 18:52, L. A. Linden Levy wrote:
> I noticed that it only skipped the edit check. That is why I added the
> skipSubmitEdit option. If they are both true then it never opens the
> editor and never checks for an edit. Probably they should just be one
> option. I think it should probably also be a command line option to skip
> the editor.

Sounds plausible. If the skipSubmitEdit is true then the edit check 
should always be skipped.


>
> - Alex
>
> On Fri, 2011-09-09 at 13:47 -0400, Luke Diamand wrote:
>> On 09/09/11 11:05, Vitor Antunes wrote:
>>> L. A. Linden Levy<alevy<at>   mobitv.com>   writes:
>>>
>>>>
>>>> Hi All,
>>>>
>>>> I have been using git-p4 for a while and it has allowed me to completely
>>>> change the way I develop and still be able to use perforce which my
>>>> company has for its main VCS. One thing that was driving me nuts was
>>>> that "git p4 submit" cycles through all of my individual commits and
>>>> asks me if I want to change them. The way I develop I often am checking
>>>> in 20 to 50 different small commits each with a descriptive git comment.
>>>> I felt like I was doing double duty by having emacs open on every commit
>>>> into perforce. So I modified git-p4 to have an option to skip the
>>>> editor. This option coupled with git-p4.skipSubmitEditCheck will make
>>>> the submission non-interactive for "git p4 submit".
>>>
>>> Hi Loren,
>>>
>>> This option was already included in a recent commit. The name that was
>>> used is "skipSubmitEditCheck". Please make sure you are using the most
>>> recent version of the script.
>>
>> I put that option in - glad it's of use!
>>
>> That option actually just skips the check of 'did the user edit the
>> file'. git-p4 will still go ahead and bring up the file in the editor first.
>>
>> I get around this myself by setting EDITOR=/bin/true. That works for me
>> because I'm only using it in a script.
>>
>> But it's possible that an additional option would actually be useful.
>>
>>
>>
>>>
>>> But don't let this discourage you from submitting patches. Just makesure
>>> you clone git's repository and apply your patch over "maint" or "master"
>>> branches. For more details on how to submit patches you can read
>>> Documentation/SubmittingPatches.
>>>
>>> Vitor
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe git" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

Re: git-p4.skipSubmitEdit

[Luke Diamand]

On 08/09/11 21:40, L. A. Linden Levy wrote:
> Hi All,
>
> I have been using git-p4 for a while and it has allowed me to completely
> change the way I develop and still be able to use perforce which my
> company has for its main VCS. One thing that was driving me nuts was
> that "git p4 submit" cycles through all of my individual commits and
> asks me if I want to change them. The way I develop I often am checking
> in 20 to 50 different small commits each with a descriptive git comment.
> I felt like I was doing double duty by having emacs open on every commit
> into perforce. So I modified git-p4 to have an option to skip the
> editor. This option coupled with git-p4.skipSubmitEditCheck will make
> the submission non-interactive for "git p4 submit".


Sorry - I've not had a chance to look at this before now. But a couple 
of comments:

  - Is there a line wrap problem in the patch? It doesn't seem to want 
to apply for me.

  - needs an update to the documentation in git-p4.txt

  - is there any way to eliminate the slightly ugly 
"if/pass/else/do-stuff" construct?

  - I'd think if you turned off the editor completely then there's no 
point doing the submit-edit-check.

  - You probably need to follow the instructions in 
Documentation/SubmittingPatches so that Junio will pay attention to the 
patch.

Regards!
Luke


>
> Below are the patch and environment results:
>
>
> $ git config -l
> ...
> user.name=Loren A. Linden Levy
> git-p4.skipsubmitedit=true
> git-p4.skipsubmiteditcheck=true
> ...
>
> $ git format-patch origin/master --stdout
>
>  From 16c4344de0047cbaf3381eca590a3e59b0d0a25c Mon Sep 17 00:00:00 2001
> From: "Loren A. Linden Levy"<lindenle@gmail.com>
> Date: Thu, 8 Sep 2011 13:37:22 -0700
> Subject: [PATCH] changed git-p4
>
> ---
>   contrib/fast-import/git-p4 |   15 ++++++++++-----
>   1 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
> index 2f7b270..a438b3e 100755
> --- a/contrib/fast-import/git-p4
> +++ b/contrib/fast-import/git-p4
> @@ -935,18 +935,23 @@ class P4Submit(Command, P4UserMap):
>               tmpFile.write(submitTemplate + separatorLine + diff +
> newdiff)
>               tmpFile.close()
>               mtime = os.stat(fileName).st_mtime
> -            if os.environ.has_key("P4EDITOR"):
> -                editor = os.environ.get("P4EDITOR")
> +            if gitConfig("git-p4.skipSubmitEdit") == "true":
> +                pass
>               else:
> -                editor = read_pipe("git var GIT_EDITOR").strip()
> -            system(editor + " " + fileName)
> -
> +                if os.environ.has_key("P4EDITOR"):
> +                    editor = os.environ.get("P4EDITOR")
> +                else:
> +                    editor = read_pipe("git var GIT_EDITOR").strip()
> +
> +                    system(editor + " " + fileName)
> +
>               if gitConfig("git-p4.skipSubmitEditCheck") == "true":
>                   checkModTime = False
>               else:
>                   checkModTime = True
>
>               response = "y"
> +
>               if checkModTime and (os.stat(fileName).st_mtime<= mtime):
>                   response = "x"
>                   while response != "y" and response != "n":

Re: git-p4.skipSubmitEdit

[L. A. Linden Levy]

[-- Attachment #1: Type: text/plain, Size: 13243 bytes --]

I agree with almost all your points. I have answered them each in-line
below.

On Mon, 2011-09-12 at 03:34 -0400, Luke Diamand wrote:
> On 08/09/11 21:40, L. A. Linden Levy wrote:
> > Hi All,
> >
> > I have been using git-p4 for a while and it has allowed me to completely
> > change the way I develop and still be able to use perforce which my
> > company has for its main VCS. One thing that was driving me nuts was
> > that "git p4 submit" cycles through all of my individual commits and
> > asks me if I want to change them. The way I develop I often am checking
> > in 20 to 50 different small commits each with a descriptive git comment.
> > I felt like I was doing double duty by having emacs open on every commit
> > into perforce. So I modified git-p4 to have an option to skip the
> > editor. This option coupled with git-p4.skipSubmitEditCheck will make
> > the submission non-interactive for "git p4 submit".
> 
> 
> Sorry - I've not had a chance to look at this before now. But a couple 
> of comments:
> 
>   - Is there a line wrap problem in the patch? It doesn't seem to want 
> to apply for me.

Probably. Below are the results from following the patch submission
instructions. 

From 16c4344de0047cbaf3381eca590a3e59b0d0a25c Mon Sep 17 00:00:00 2001
From: "Loren A. Linden Levy" <lindenle@gmail.com>
Date: Thu, 8 Sep 2011 13:37:22 -0700
Subject: [PATCH 1/5] changed git-p4

---
 contrib/fast-import/git-p4 |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 2f7b270..a438b3e 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -935,18 +935,23 @@ class P4Submit(Command, P4UserMap):
             tmpFile.write(submitTemplate + separatorLine + diff +
newdiff)
             tmpFile.close()
             mtime = os.stat(fileName).st_mtime
-            if os.environ.has_key("P4EDITOR"):
-                editor = os.environ.get("P4EDITOR")
+            if gitConfig("git-p4.skipSubmitEdit") == "true":
+                pass
             else:
-                editor = read_pipe("git var GIT_EDITOR").strip()
-            system(editor + " " + fileName)
-
+                if os.environ.has_key("P4EDITOR"):
+                    editor = os.environ.get("P4EDITOR")
+                else:
+                    editor = read_pipe("git var GIT_EDITOR").strip()
+                    
+                    system(editor + " " + fileName)
+                    
             if gitConfig("git-p4.skipSubmitEditCheck") == "true":
                 checkModTime = False
             else:
                 checkModTime = True
 
             response = "y"
+
             if checkModTime and (os.stat(fileName).st_mtime <= mtime):
                 response = "x"
                 while response != "y" and response != "n":
-- 
1.7.7.rc0.73.g16c43

From ed60bb737b89eea4b84719c458aeebebd666a2ae Mon Sep 17 00:00:00 2001
From: "Loren A. Linden Levy" <lindenle@gmail.com>
Date: Mon, 12 Sep 2011 09:52:52 -0700
Subject: [PATCH 2/5] removed white space for patch

---
 contrib/fast-import/git-p4 |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index a438b3e..edd2525 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -942,9 +942,7 @@ class P4Submit(Command, P4UserMap):
                     editor = os.environ.get("P4EDITOR")
                 else:
                     editor = read_pipe("git var GIT_EDITOR").strip()
-                    
                     system(editor + " " + fileName)
-                    
             if gitConfig("git-p4.skipSubmitEditCheck") == "true":
                 checkModTime = False
             else:
-- 
1.7.7.rc0.73.g16c43

From ee7eaf73cf52100359b08600eab1279e03ef777b Mon Sep 17 00:00:00 2001
From: "Loren A. Linden Levy" <lindenle@gmail.com>
Date: Mon, 12 Sep 2011 09:55:11 -0700
Subject: [PATCH 3/5] modified the documentation

---
 contrib/fast-import/git-p4.txt |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/contrib/fast-import/git-p4.txt
b/contrib/fast-import/git-p4.txt
index 52003ae..3dba636 100644
--- a/contrib/fast-import/git-p4.txt
+++ b/contrib/fast-import/git-p4.txt
@@ -202,10 +202,16 @@ able to find the relevant client.  This client
spec will be used to
 both filter the files cloned by git and set the directory layout as
 specified in the client (this implies --keep-path style semantics).
 
-git-p4.skipSubmitModTimeCheck
+git-p4.skipSubmitEdit
 
   git config [--global] git-p4.skipSubmitModTimeCheck false
 
+If true, submit will not invoke the editor to modify the p4 change
template.
+
+git-p4.skipSubmitEditCheck 
+
+  git config [--global] git-p4.skipSubmitEditCheck false
+
 If true, submit will not check if the p4 change template has been
modified.
 
 git-p4.preserveUser
-- 
1.7.7.rc0.73.g16c43

From ee7eaf73cf52100359b08600eab1279e03ef777b Mon Sep 17 00:00:00 2001
From: "Loren A. Linden Levy" <lindenle@gmail.com>
Date: Mon, 12 Sep 2011 09:55:11 -0700
Subject: [PATCH 3/6] modified the documentation

---
 contrib/fast-import/git-p4.txt |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/contrib/fast-import/git-p4.txt
b/contrib/fast-import/git-p4.txt
index 52003ae..3dba636 100644
--- a/contrib/fast-import/git-p4.txt
+++ b/contrib/fast-import/git-p4.txt
@@ -202,10 +202,16 @@ able to find the relevant client.  This client
spec will be used to
 both filter the files cloned by git and set the directory layout as
 specified in the client (this implies --keep-path style semantics).
 
-git-p4.skipSubmitModTimeCheck
+git-p4.skipSubmitEdit
 
   git config [--global] git-p4.skipSubmitModTimeCheck false
 
+If true, submit will not invoke the editor to modify the p4 change
template.
+
+git-p4.skipSubmitEditCheck 
+
+  git config [--global] git-p4.skipSubmitEditCheck false
+
 If true, submit will not check if the p4 change template has been
modified.
 
 git-p4.preserveUser
-- 
1.7.7.rc0.73.g16c43

From c9d3b4ea4dcf1f5fd489dfacde550dd4c648afc7 Mon Sep 17 00:00:00 2001
From: "Loren A. Linden Levy" <lindenle@gmail.com>
Date: Mon, 12 Sep 2011 10:01:08 -0700
Subject: [PATCH 4/6] changed the order a bit

---
 contrib/fast-import/git-p4 |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index edd2525..80bcc90 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -935,18 +935,19 @@ class P4Submit(Command, P4UserMap):
             tmpFile.write(submitTemplate + separatorLine + diff +
newdiff)
             tmpFile.close()
             mtime = os.stat(fileName).st_mtime
+            if gitConfig("git-p4.skipSubmitEditCheck") == "true":
+                checkModTime = False
+            else:
+                checkModTime = True
             if gitConfig("git-p4.skipSubmitEdit") == "true":
-                pass
+                checkModTime = False
+                pass        
             else:
                 if os.environ.has_key("P4EDITOR"):
                     editor = os.environ.get("P4EDITOR")
                 else:
                     editor = read_pipe("git var GIT_EDITOR").strip()
                     system(editor + " " + fileName)
-            if gitConfig("git-p4.skipSubmitEditCheck") == "true":
-                checkModTime = False
-            else:
-                checkModTime = True
 
             response = "y"
 
-- 
1.7.7.rc0.73.g16c43

From de3d67a301b49e4d986e83727b822aa5fd257527 Mon Sep 17 00:00:00 2001
From: "Loren A. Linden Levy" <lindenle@gmail.com>
Date: Mon, 12 Sep 2011 10:03:03 -0700
Subject: [PATCH 5/6] cleaned up whitespace for making a patch

---
 contrib/fast-import/git-p4     |    2 +-
 contrib/fast-import/git-p4.txt |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 80bcc90..1cd65a3 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -941,7 +941,7 @@ class P4Submit(Command, P4UserMap):
                 checkModTime = True
             if gitConfig("git-p4.skipSubmitEdit") == "true":
                 checkModTime = False
-                pass        
+                pass
             else:
                 if os.environ.has_key("P4EDITOR"):
                     editor = os.environ.get("P4EDITOR")
diff --git a/contrib/fast-import/git-p4.txt
b/contrib/fast-import/git-p4.txt
index 3dba636..8291e7a 100644
--- a/contrib/fast-import/git-p4.txt
+++ b/contrib/fast-import/git-p4.txt
@@ -208,7 +208,7 @@ git-p4.skipSubmitEdit
 
 If true, submit will not invoke the editor to modify the p4 change
template.
 
-git-p4.skipSubmitEditCheck 
+git-p4.skipSubmitEditCheck
 
   git config [--global] git-p4.skipSubmitEditCheck false
 
-- 
1.7.7.rc0.73.g16c43

From 64c4878d12748daeda637b3d7d2f018c316b4239 Mon Sep 17 00:00:00 2001
From: "Loren A. Linden Levy" <lindenle@gmail.com>
Date: Mon, 12 Sep 2011 10:10:22 -0700
Subject: [PATCH 6/6] made the docs correct for the new option

---
 contrib/fast-import/git-p4.txt |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/contrib/fast-import/git-p4.txt
b/contrib/fast-import/git-p4.txt
index 8291e7a..f1c1d0c 100644
--- a/contrib/fast-import/git-p4.txt
+++ b/contrib/fast-import/git-p4.txt
@@ -204,7 +204,7 @@ specified in the client (this implies --keep-path
style semantics).
 
 git-p4.skipSubmitEdit
 
-  git config [--global] git-p4.skipSubmitModTimeCheck false
+  git config [--global] git-p4.skipSubmitEdit false
 
 If true, submit will not invoke the editor to modify the p4 change
template.
 
-- 
1.7.7.rc0.73.g16c43



>   - needs an update to the documentation in git-p4.txt

Done. Also I think the skipSubmitEditCheck was incorrectly name
git-p4.skipSubmitModTimeCheck in the git-p4.txt file. I updated this as
well.

>   - is there any way to eliminate the slightly ugly 
> "if/pass/else/do-stuff" construct?

I think it is logically correct and describes the situation well. We are
skipping the editor so we pass it. If you still do not agree then
perhaps a suggestion of a better logical flow would help me. 

>   - I'd think if you turned off the editor completely then there's no 
> point doing the submit-edit-check.

I have changed the ordering so that at the moment this happens because I
agree with you.

>   - You probably need to follow the instructions in 
> Documentation/SubmittingPatches so that Junio will pay attention to the 
> patch.

See point #1.

> Regards!
> Luke
> 
> 
> >
> > Below are the patch and environment results:
> >
> >
> > $ git config -l
> > ...
> > user.name=Loren A. Linden Levy
> > git-p4.skipsubmitedit=true
> > git-p4.skipsubmiteditcheck=true
> > ...
> >
> > $ git format-patch origin/master --stdout
> >
> >  From 16c4344de0047cbaf3381eca590a3e59b0d0a25c Mon Sep 17 00:00:00 2001
> > From: "Loren A. Linden Levy"<lindenle@gmail.com>
> > Date: Thu, 8 Sep 2011 13:37:22 -0700
> > Subject: [PATCH] changed git-p4
> >
> > ---
> >   contrib/fast-import/git-p4 |   15 ++++++++++-----
> >   1 files changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
> > index 2f7b270..a438b3e 100755
> > --- a/contrib/fast-import/git-p4
> > +++ b/contrib/fast-import/git-p4
> > @@ -935,18 +935,23 @@ class P4Submit(Command, P4UserMap):
> >               tmpFile.write(submitTemplate + separatorLine + diff +
> > newdiff)
> >               tmpFile.close()
> >               mtime = os.stat(fileName).st_mtime
> > -            if os.environ.has_key("P4EDITOR"):
> > -                editor = os.environ.get("P4EDITOR")
> > +            if gitConfig("git-p4.skipSubmitEdit") == "true":
> > +                pass
> >               else:
> > -                editor = read_pipe("git var GIT_EDITOR").strip()
> > -            system(editor + " " + fileName)
> > -
> > +                if os.environ.has_key("P4EDITOR"):
> > +                    editor = os.environ.get("P4EDITOR")
> > +                else:
> > +                    editor = read_pipe("git var GIT_EDITOR").strip()
> > +
> > +                    system(editor + " " + fileName)
> > +
> >               if gitConfig("git-p4.skipSubmitEditCheck") == "true":
> >                   checkModTime = False
> >               else:
> >                   checkModTime = True
> >
> >               response = "y"
> > +
> >               if checkModTime and (os.stat(fileName).st_mtime<= mtime):
> >                   response = "x"
> >                   while response != "y" and response != "n":
> 

-- 
Alex Linden Levy
Senior Software Engineer
MobiTV, Inc.
6425 Christie Avenue, 5th Floor, Emeryville, CA 94608
phone 510.450.5190 mobile 720.352.8394
email alevy@mobitv.com  web www.mobitv.com


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: git-p4.skipSubmitEdit

[Pete Wyckoff]

alevy@mobitv.com wrote on Mon, 12 Sep 2011 10:12 -0700:
> I agree with almost all your points. I have answered them each in-line
> below.
> 
> On Mon, 2011-09-12 at 03:34 -0400, Luke Diamand wrote:
> > On 08/09/11 21:40, L. A. Linden Levy wrote:
> > > Hi All,
> > >
> > > I have been using git-p4 for a while and it has allowed me to completely
> > > change the way I develop and still be able to use perforce which my
> > > company has for its main VCS. One thing that was driving me nuts was
> > > that "git p4 submit" cycles through all of my individual commits and
> > > asks me if I want to change them. The way I develop I often am checking
> > > in 20 to 50 different small commits each with a descriptive git comment.
> > > I felt like I was doing double duty by having emacs open on every commit
> > > into perforce. So I modified git-p4 to have an option to skip the
> > > editor. This option coupled with git-p4.skipSubmitEditCheck will make
> > > the submission non-interactive for "git p4 submit".
> > 
> > 
> > Sorry - I've not had a chance to look at this before now. But a couple 
> > of comments:
> > 
> >   - Is there a line wrap problem in the patch? It doesn't seem to want 
> > to apply for me.
> 
> Probably. Below are the results from following the patch submission
> instructions. 

Sorry I sat on this for a month.  It is a good idea.  Your
patches were good in content, but they didn't apply well due to
being line-wrapped and having one duplicate.

Reading the code, though, I decided that the whole
skipSubmitEdit* checking was a bit convoluted even before you got
to it.  So I moved it all to a separate function.  And added a
unit test.

Tell me if you think this is okay instead.  If I got a bit too
wordy in the doc, please help with that too.

		-- Pete

--- 8< ---

Subject: [PATCH] git-p4: introduce skipSubmitEdit

Add a configuration variable to skip invoking the editor in the
submit path.

The existing variable skipSubmitEditCheck continues to make sure
that the submit template was indeed modified by the editor; but,
it is not considered is skipSubmitEdit is true.

Reported-by: Loren A. Linden Levy <lindenle@gmail.com>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 contrib/fast-import/git-p4     |   60 +++++++++++++++++++----------
 contrib/fast-import/git-p4.txt |   19 ++++++++-
 t/t9804-skip-submit-edit.sh    |   82 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 137 insertions(+), 24 deletions(-)
 create mode 100755 t/t9804-skip-submit-edit.sh

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index f885d70..abd6778 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -847,6 +847,39 @@ class P4Submit(Command, P4UserMap):
 
         return template
 
+    def edit_template(self, template_file):
+        """Invoke the editor to let the user change the submission
+           message.  Return true if okay to continue with the submit."""
+
+        # if configured to skip the editing part, just submit
+        if gitConfig("git-p4.skipSubmitEdit") == "true":
+            return True
+
+        # look at the modification time, to check later if the user saved
+        # the file
+        mtime = os.stat(template_file).st_mtime
+
+        # invoke the editor
+        if os.environ.has_key("P4EDITOR"):
+            editor = os.environ.get("P4EDITOR")
+        else:
+            editor = read_pipe("git var GIT_EDITOR").strip()
+        system(editor + " " + template_file)
+
+        # If the file was not saved, prompt to see if this patch should
+        # be skipped.  But skip this verification step if configured so.
+        if gitConfig("git-p4.skipSubmitEditCheck") == "true":
+            print "return true for skipSubmitEditCheck"
+            return True
+
+        if os.stat(template_file).st_mtime <= mtime:
+            while True:
+                response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
+                if response == 'y':
+                    return True
+                if response == 'n':
+                    return False
+
     def applyCommit(self, id):
         print "Applying %s" % (read_pipe("git log --max-count=1 --pretty=oneline %s" % id))
 
@@ -1001,7 +1034,7 @@ class P4Submit(Command, P4UserMap):
 
             separatorLine = "######## everything below this line is just the diff #######\n"
 
-            [handle, fileName] = tempfile.mkstemp()
+            (handle, fileName) = tempfile.mkstemp()
             tmpFile = os.fdopen(handle, "w+")
             if self.isWindows:
                 submitTemplate = submitTemplate.replace("\n", "\r\n")
@@ -1009,25 +1042,9 @@ class P4Submit(Command, P4UserMap):
                 newdiff = newdiff.replace("\n", "\r\n")
             tmpFile.write(submitTemplate + separatorLine + diff + newdiff)
             tmpFile.close()
-            mtime = os.stat(fileName).st_mtime
-            if os.environ.has_key("P4EDITOR"):
-                editor = os.environ.get("P4EDITOR")
-            else:
-                editor = read_pipe("git var GIT_EDITOR").strip()
-            system(editor + " " + fileName)
 
-            if gitConfig("git-p4.skipSubmitEditCheck") == "true":
-                checkModTime = False
-            else:
-                checkModTime = True
-
-            response = "y"
-            if checkModTime and (os.stat(fileName).st_mtime <= mtime):
-                response = "x"
-                while response != "y" and response != "n":
-                    response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
-
-            if response == "y":
+            if self.edit_template(fileName):
+                # read the edited message and submit
                 tmpFile = open(fileName, "rb")
                 message = tmpFile.read()
                 tmpFile.close()
@@ -1039,11 +1056,12 @@ class P4Submit(Command, P4UserMap):
                 if self.preserveUser:
                     if p4User:
                         # Get last changelist number. Cannot easily get it from
-                        # the submit command output as the output is unmarshalled.
+                        # the submit command output as the output is
+                        # unmarshalled.
                         changelist = self.lastP4Changelist()
                         self.modifyChangelistUser(changelist, p4User)
-
             else:
+                # skip this patch
                 for f in editedFiles:
                     p4_revert(f)
                 for f in filesToAdd:
diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt
index 52003ae..5044a12 100644
--- a/contrib/fast-import/git-p4.txt
+++ b/contrib/fast-import/git-p4.txt
@@ -202,11 +202,24 @@ able to find the relevant client.  This client spec will be used to
 both filter the files cloned by git and set the directory layout as
 specified in the client (this implies --keep-path style semantics).
 
-git-p4.skipSubmitModTimeCheck
+git-p4.skipSubmitEdit
 
-  git config [--global] git-p4.skipSubmitModTimeCheck false
+  git config [--global] git-p4.skipSubmitEdit false
 
-If true, submit will not check if the p4 change template has been modified.
+Normally, git-p4 invokes an editor after each commit is applied so
+that you can make changes to the submit message.  Setting this
+variable to true will skip the editing step, submitting the change as is.
+
+git-p4.skipSubmitEditCheck
+
+  git config [--global] git-p4.skipSubmitEditCheck false
+
+After the editor is invoked, git-p4 normally makes sure you saved the
+change description, as an indication that you did indeed read it over
+and edit it.  You can quit without saving to abort the submit (or skip
+this change and continue).  Setting this variable to true will cause
+git-p4 not to check if you saved the change description.  This variable
+only matters if git-p4.skipSubmitEdit has not been set to true.
 
 git-p4.preserveUser
 
diff --git a/t/t9804-skip-submit-edit.sh b/t/t9804-skip-submit-edit.sh
new file mode 100755
index 0000000..734ccf2
--- /dev/null
+++ b/t/t9804-skip-submit-edit.sh
@@ -0,0 +1,82 @@
+#!/bin/sh
+
+test_description='git-p4 skipSubmitEdit config variables'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	start_p4d
+'
+
+test_expect_success 'init depot' '
+	(
+		cd "$cli" &&
+		echo file1 >file1 &&
+		p4 add file1 &&
+		p4 submit -d "change 1"
+	)
+'
+
+# this works because EDITOR is set to :
+test_expect_success 'no config, unedited, say yes' '
+	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		echo line >>file1 &&
+		git commit -a -m "change 2" &&
+		echo y | "$GITP4" submit &&
+		p4 changes //depot/... >wc &&
+		test_line_count = 2 wc
+	)
+'
+
+test_expect_success 'no config, unedited, say no' '
+	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		echo line >>file1 &&
+		git commit -a -m "change 3 (not really)" &&
+		printf "bad response\nn\n" | "$GITP4" submit
+		p4 changes //depot/... >wc &&
+		test_line_count = 2 wc
+	)
+'
+
+test_expect_success 'skipSubmitEdit' '
+	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		git config git-p4.skipSubmitEdit true &&
+		# will fail if editor is even invoked
+		git config core.editor /bin/false &&
+		echo line >>file1 &&
+		git commit -a -m "change 3" &&
+		"$GITP4" submit &&
+		p4 changes //depot/... >wc &&
+		test_line_count = 3 wc
+	)
+'
+
+test_expect_success 'skipSubmitEditCheck' '
+	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		git config git-p4.skipSubmitEditCheck true &&
+		echo line >>file1 &&
+		git commit -a -m "change 4" &&
+		"$GITP4" submit &&
+		p4 changes //depot/... >wc &&
+		test_line_count = 4 wc
+	)
+'
+
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done
-- 
1.7.7

Re: git-p4.skipSubmitEdit

[L. A. Linden Levy]

[-- Attachment #1: Type: text/plain, Size: 11341 bytes --]

Hi Peter,

This is just fine for me. Like I said initially I did not approach the
problem from an engineering perspective, I just changed it to prevent
the patch questions so I could commit faster. Looking forward to seeing
the update in the repo.

Cheers,
Alex

On Mon, 2011-10-17 at 20:45 -0400, Pete Wyckoff wrote:
> alevy@mobitv.com wrote on Mon, 12 Sep 2011 10:12 -0700:
> > I agree with almost all your points. I have answered them each in-line
> > below.
> > 
> > On Mon, 2011-09-12 at 03:34 -0400, Luke Diamand wrote:
> > > On 08/09/11 21:40, L. A. Linden Levy wrote:
> > > > Hi All,
> > > >
> > > > I have been using git-p4 for a while and it has allowed me to completely
> > > > change the way I develop and still be able to use perforce which my
> > > > company has for its main VCS. One thing that was driving me nuts was
> > > > that "git p4 submit" cycles through all of my individual commits and
> > > > asks me if I want to change them. The way I develop I often am checking
> > > > in 20 to 50 different small commits each with a descriptive git comment.
> > > > I felt like I was doing double duty by having emacs open on every commit
> > > > into perforce. So I modified git-p4 to have an option to skip the
> > > > editor. This option coupled with git-p4.skipSubmitEditCheck will make
> > > > the submission non-interactive for "git p4 submit".
> > > 
> > > 
> > > Sorry - I've not had a chance to look at this before now. But a couple 
> > > of comments:
> > > 
> > >   - Is there a line wrap problem in the patch? It doesn't seem to want 
> > > to apply for me.
> > 
> > Probably. Below are the results from following the patch submission
> > instructions. 
> 
> Sorry I sat on this for a month.  It is a good idea.  Your
> patches were good in content, but they didn't apply well due to
> being line-wrapped and having one duplicate.
> 
> Reading the code, though, I decided that the whole
> skipSubmitEdit* checking was a bit convoluted even before you got
> to it.  So I moved it all to a separate function.  And added a
> unit test.
> 
> Tell me if you think this is okay instead.  If I got a bit too
> wordy in the doc, please help with that too.
> 
> 		-- Pete
> 
> --- 8< ---
> 
> Subject: [PATCH] git-p4: introduce skipSubmitEdit
> 
> Add a configuration variable to skip invoking the editor in the
> submit path.
> 
> The existing variable skipSubmitEditCheck continues to make sure
> that the submit template was indeed modified by the editor; but,
> it is not considered is skipSubmitEdit is true.
> 
> Reported-by: Loren A. Linden Levy <lindenle@gmail.com>
> Signed-off-by: Pete Wyckoff <pw@padd.com>
> ---
>  contrib/fast-import/git-p4     |   60 +++++++++++++++++++----------
>  contrib/fast-import/git-p4.txt |   19 ++++++++-
>  t/t9804-skip-submit-edit.sh    |   82 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 137 insertions(+), 24 deletions(-)
>  create mode 100755 t/t9804-skip-submit-edit.sh
> 
> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
> index f885d70..abd6778 100755
> --- a/contrib/fast-import/git-p4
> +++ b/contrib/fast-import/git-p4
> @@ -847,6 +847,39 @@ class P4Submit(Command, P4UserMap):
>  
>          return template
>  
> +    def edit_template(self, template_file):
> +        """Invoke the editor to let the user change the submission
> +           message.  Return true if okay to continue with the submit."""
> +
> +        # if configured to skip the editing part, just submit
> +        if gitConfig("git-p4.skipSubmitEdit") == "true":
> +            return True
> +
> +        # look at the modification time, to check later if the user saved
> +        # the file
> +        mtime = os.stat(template_file).st_mtime
> +
> +        # invoke the editor
> +        if os.environ.has_key("P4EDITOR"):
> +            editor = os.environ.get("P4EDITOR")
> +        else:
> +            editor = read_pipe("git var GIT_EDITOR").strip()
> +        system(editor + " " + template_file)
> +
> +        # If the file was not saved, prompt to see if this patch should
> +        # be skipped.  But skip this verification step if configured so.
> +        if gitConfig("git-p4.skipSubmitEditCheck") == "true":
> +            print "return true for skipSubmitEditCheck"
> +            return True
> +
> +        if os.stat(template_file).st_mtime <= mtime:
> +            while True:
> +                response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
> +                if response == 'y':
> +                    return True
> +                if response == 'n':
> +                    return False
> +
>      def applyCommit(self, id):
>          print "Applying %s" % (read_pipe("git log --max-count=1 --pretty=oneline %s" % id))
>  
> @@ -1001,7 +1034,7 @@ class P4Submit(Command, P4UserMap):
>  
>              separatorLine = "######## everything below this line is just the diff #######\n"
>  
> -            [handle, fileName] = tempfile.mkstemp()
> +            (handle, fileName) = tempfile.mkstemp()
>              tmpFile = os.fdopen(handle, "w+")
>              if self.isWindows:
>                  submitTemplate = submitTemplate.replace("\n", "\r\n")
> @@ -1009,25 +1042,9 @@ class P4Submit(Command, P4UserMap):
>                  newdiff = newdiff.replace("\n", "\r\n")
>              tmpFile.write(submitTemplate + separatorLine + diff + newdiff)
>              tmpFile.close()
> -            mtime = os.stat(fileName).st_mtime
> -            if os.environ.has_key("P4EDITOR"):
> -                editor = os.environ.get("P4EDITOR")
> -            else:
> -                editor = read_pipe("git var GIT_EDITOR").strip()
> -            system(editor + " " + fileName)
>  
> -            if gitConfig("git-p4.skipSubmitEditCheck") == "true":
> -                checkModTime = False
> -            else:
> -                checkModTime = True
> -
> -            response = "y"
> -            if checkModTime and (os.stat(fileName).st_mtime <= mtime):
> -                response = "x"
> -                while response != "y" and response != "n":
> -                    response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
> -
> -            if response == "y":
> +            if self.edit_template(fileName):
> +                # read the edited message and submit
>                  tmpFile = open(fileName, "rb")
>                  message = tmpFile.read()
>                  tmpFile.close()
> @@ -1039,11 +1056,12 @@ class P4Submit(Command, P4UserMap):
>                  if self.preserveUser:
>                      if p4User:
>                          # Get last changelist number. Cannot easily get it from
> -                        # the submit command output as the output is unmarshalled.
> +                        # the submit command output as the output is
> +                        # unmarshalled.
>                          changelist = self.lastP4Changelist()
>                          self.modifyChangelistUser(changelist, p4User)
> -
>              else:
> +                # skip this patch
>                  for f in editedFiles:
>                      p4_revert(f)
>                  for f in filesToAdd:
> diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt
> index 52003ae..5044a12 100644
> --- a/contrib/fast-import/git-p4.txt
> +++ b/contrib/fast-import/git-p4.txt
> @@ -202,11 +202,24 @@ able to find the relevant client.  This client spec will be used to
>  both filter the files cloned by git and set the directory layout as
>  specified in the client (this implies --keep-path style semantics).
>  
> -git-p4.skipSubmitModTimeCheck
> +git-p4.skipSubmitEdit
>  
> -  git config [--global] git-p4.skipSubmitModTimeCheck false
> +  git config [--global] git-p4.skipSubmitEdit false
>  
> -If true, submit will not check if the p4 change template has been modified.
> +Normally, git-p4 invokes an editor after each commit is applied so
> +that you can make changes to the submit message.  Setting this
> +variable to true will skip the editing step, submitting the change as is.
> +
> +git-p4.skipSubmitEditCheck
> +
> +  git config [--global] git-p4.skipSubmitEditCheck false
> +
> +After the editor is invoked, git-p4 normally makes sure you saved the
> +change description, as an indication that you did indeed read it over
> +and edit it.  You can quit without saving to abort the submit (or skip
> +this change and continue).  Setting this variable to true will cause
> +git-p4 not to check if you saved the change description.  This variable
> +only matters if git-p4.skipSubmitEdit has not been set to true.
>  
>  git-p4.preserveUser
>  
> diff --git a/t/t9804-skip-submit-edit.sh b/t/t9804-skip-submit-edit.sh
> new file mode 100755
> index 0000000..734ccf2
> --- /dev/null
> +++ b/t/t9804-skip-submit-edit.sh
> @@ -0,0 +1,82 @@
> +#!/bin/sh
> +
> +test_description='git-p4 skipSubmitEdit config variables'
> +
> +. ./lib-git-p4.sh
> +
> +test_expect_success 'start p4d' '
> +	start_p4d
> +'
> +
> +test_expect_success 'init depot' '
> +	(
> +		cd "$cli" &&
> +		echo file1 >file1 &&
> +		p4 add file1 &&
> +		p4 submit -d "change 1"
> +	)
> +'
> +
> +# this works because EDITOR is set to :
> +test_expect_success 'no config, unedited, say yes' '
> +	"$GITP4" clone --dest="$git" //depot &&
> +	test_when_finished cleanup_git &&
> +	(
> +		cd "$git" &&
> +		echo line >>file1 &&
> +		git commit -a -m "change 2" &&
> +		echo y | "$GITP4" submit &&
> +		p4 changes //depot/... >wc &&
> +		test_line_count = 2 wc
> +	)
> +'
> +
> +test_expect_success 'no config, unedited, say no' '
> +	"$GITP4" clone --dest="$git" //depot &&
> +	test_when_finished cleanup_git &&
> +	(
> +		cd "$git" &&
> +		echo line >>file1 &&
> +		git commit -a -m "change 3 (not really)" &&
> +		printf "bad response\nn\n" | "$GITP4" submit
> +		p4 changes //depot/... >wc &&
> +		test_line_count = 2 wc
> +	)
> +'
> +
> +test_expect_success 'skipSubmitEdit' '
> +	"$GITP4" clone --dest="$git" //depot &&
> +	test_when_finished cleanup_git &&
> +	(
> +		cd "$git" &&
> +		git config git-p4.skipSubmitEdit true &&
> +		# will fail if editor is even invoked
> +		git config core.editor /bin/false &&
> +		echo line >>file1 &&
> +		git commit -a -m "change 3" &&
> +		"$GITP4" submit &&
> +		p4 changes //depot/... >wc &&
> +		test_line_count = 3 wc
> +	)
> +'
> +
> +test_expect_success 'skipSubmitEditCheck' '
> +	"$GITP4" clone --dest="$git" //depot &&
> +	test_when_finished cleanup_git &&
> +	(
> +		cd "$git" &&
> +		git config git-p4.skipSubmitEditCheck true &&
> +		echo line >>file1 &&
> +		git commit -a -m "change 4" &&
> +		"$GITP4" submit &&
> +		p4 changes //depot/... >wc &&
> +		test_line_count = 4 wc
> +	)
> +'
> +
> +
> +test_expect_success 'kill p4d' '
> +	kill_p4d
> +'
> +
> +test_done

-- 
Alex Linden Levy
Senior Software Engineer
MobiTV, Inc.
6425 Christie Avenue, 5th Floor, Emeryville, CA 94608
phone 510.450.5190 mobile 720.352.8394
email alevy@mobitv.com  web www.mobitv.com


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: git-p4.skipSubmitEdit

[Pete Wyckoff]

alevy@mobitv.com wrote on Tue, 18 Oct 2011 09:51 -0700:
> This is just fine for me. Like I said initially I did not approach the
> problem from an engineering perspective, I just changed it to prevent
> the patch questions so I could commit faster. Looking forward to seeing
> the update in the repo.

Okay, thanks.  I'll see if I get some review comments, and will
gradually shepherd this toward the next release.

		-- Pete

Re: git-p4.skipSubmitEdit

[Luke Diamand]

On 18/10/11 01:45, Pete Wyckoff wrote:
> alevy@mobitv.com wrote on Mon, 12 Sep 2011 10:12 -0700:
>> I agree with almost all your points. I have answered them each in-line
>> below.
>>
>> On Mon, 2011-09-12 at 03:34 -0400, Luke Diamand wrote:
>>> On 08/09/11 21:40, L. A. Linden Levy wrote:
>>>> Hi All,
>>>>
>>>> I have been using git-p4 for a while and it has allowed me to completely
>>>> change the way I develop and still be able to use perforce which my
>>>> company has for its main VCS. One thing that was driving me nuts was
>>>> that "git p4 submit" cycles through all of my individual commits and
>>>> asks me if I want to change them. The way I develop I often am checking
>>>> in 20 to 50 different small commits each with a descriptive git comment.
>>>> I felt like I was doing double duty by having emacs open on every commit
>>>> into perforce. So I modified git-p4 to have an option to skip the
>>>> editor. This option coupled with git-p4.skipSubmitEditCheck will make
>>>> the submission non-interactive for "git p4 submit".
>>>
>>>
>>> Sorry - I've not had a chance to look at this before now. But a couple
>>> of comments:
>>>
>>>    - Is there a line wrap problem in the patch? It doesn't seem to want
>>> to apply for me.
>>
>> Probably. Below are the results from following the patch submission
>> instructions.
>
> Sorry I sat on this for a month.  It is a good idea.  Your
> patches were good in content, but they didn't apply well due to
> being line-wrapped and having one duplicate.
>
> Reading the code, though, I decided that the whole
> skipSubmitEdit* checking was a bit convoluted even before you got
> to it.  So I moved it all to a separate function.  And added a
> unit test.
>
> Tell me if you think this is okay instead.  If I got a bit too
> wordy in the doc, please help with that too.

Looks good, one minor nit (see below) and a comment.

Ack.

Luke


>
> 		-- Pete
>
> --- 8<  ---
>
> Subject: [PATCH] git-p4: introduce skipSubmitEdit
>
> Add a configuration variable to skip invoking the editor in the
> submit path.
>
> The existing variable skipSubmitEditCheck continues to make sure
> that the submit template was indeed modified by the editor; but,
> it is not considered is skipSubmitEdit is true.
>
> Reported-by: Loren A. Linden Levy<lindenle@gmail.com>
> Signed-off-by: Pete Wyckoff<pw@padd.com>
> ---
>   contrib/fast-import/git-p4     |   60 +++++++++++++++++++----------
>   contrib/fast-import/git-p4.txt |   19 ++++++++-
>   t/t9804-skip-submit-edit.sh    |   82 ++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 137 insertions(+), 24 deletions(-)
>   create mode 100755 t/t9804-skip-submit-edit.sh
>
> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
> index f885d70..abd6778 100755
> --- a/contrib/fast-import/git-p4
> +++ b/contrib/fast-import/git-p4
> @@ -847,6 +847,39 @@ class P4Submit(Command, P4UserMap):
>
>           return template
>
> +    def edit_template(self, template_file):
> +        """Invoke the editor to let the user change the submission
> +           message.  Return true if okay to continue with the submit."""
> +
> +        # if configured to skip the editing part, just submit
> +        if gitConfig("git-p4.skipSubmitEdit") == "true":
> +            return True
> +
> +        # look at the modification time, to check later if the user saved
> +        # the file
> +        mtime = os.stat(template_file).st_mtime
> +
> +        # invoke the editor
> +        if os.environ.has_key("P4EDITOR"):
> +            editor = os.environ.get("P4EDITOR")
> +        else:
> +            editor = read_pipe("git var GIT_EDITOR").strip()
> +        system(editor + " " + template_file)

This is where we should really check the return code. However, doing so 
seems to break lots of the existing tests so it's not as easy as it looks.

> +
> +        # If the file was not saved, prompt to see if this patch should
> +        # be skipped.  But skip this verification step if configured so.
> +        if gitConfig("git-p4.skipSubmitEditCheck") == "true":
> +            print "return true for skipSubmitEditCheck"

You print a helpful/annoying(?) message here, but not further up at 
skipSubmitEdit?


> +            return True
> +
> +        if os.stat(template_file).st_mtime<= mtime:
> +            while True:
> +                response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
> +                if response == 'y':
> +                    return True
> +                if response == 'n':
> +                    return False
> +
>       def applyCommit(self, id):
>           print "Applying %s" % (read_pipe("git log --max-count=1 --pretty=oneline %s" % id))
>
> @@ -1001,7 +1034,7 @@ class P4Submit(Command, P4UserMap):
>
>               separatorLine = "######## everything below this line is just the diff #######\n"
>
> -            [handle, fileName] = tempfile.mkstemp()
> +            (handle, fileName) = tempfile.mkstemp()
>               tmpFile = os.fdopen(handle, "w+")
>               if self.isWindows:
>                   submitTemplate = submitTemplate.replace("\n", "\r\n")
> @@ -1009,25 +1042,9 @@ class P4Submit(Command, P4UserMap):
>                   newdiff = newdiff.replace("\n", "\r\n")
>               tmpFile.write(submitTemplate + separatorLine + diff + newdiff)
>               tmpFile.close()
> -            mtime = os.stat(fileName).st_mtime
> -            if os.environ.has_key("P4EDITOR"):
> -                editor = os.environ.get("P4EDITOR")
> -            else:
> -                editor = read_pipe("git var GIT_EDITOR").strip()
> -            system(editor + " " + fileName)
>
> -            if gitConfig("git-p4.skipSubmitEditCheck") == "true":
> -                checkModTime = False
> -            else:
> -                checkModTime = True
> -
> -            response = "y"
> -            if checkModTime and (os.stat(fileName).st_mtime<= mtime):
> -                response = "x"
> -                while response != "y" and response != "n":
> -                    response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
> -
> -            if response == "y":
> +            if self.edit_template(fileName):
> +                # read the edited message and submit
>                   tmpFile = open(fileName, "rb")
>                   message = tmpFile.read()
>                   tmpFile.close()
> @@ -1039,11 +1056,12 @@ class P4Submit(Command, P4UserMap):
>                   if self.preserveUser:
>                       if p4User:
>                           # Get last changelist number. Cannot easily get it from
> -                        # the submit command output as the output is unmarshalled.
> +                        # the submit command output as the output is
> +                        # unmarshalled.
>                           changelist = self.lastP4Changelist()
>                           self.modifyChangelistUser(changelist, p4User)
> -
>               else:
> +                # skip this patch
>                   for f in editedFiles:
>                       p4_revert(f)
>                   for f in filesToAdd:
> diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt
> index 52003ae..5044a12 100644
> --- a/contrib/fast-import/git-p4.txt
> +++ b/contrib/fast-import/git-p4.txt
> @@ -202,11 +202,24 @@ able to find the relevant client.  This client spec will be used to
>   both filter the files cloned by git and set the directory layout as
>   specified in the client (this implies --keep-path style semantics).
>
> -git-p4.skipSubmitModTimeCheck
> +git-p4.skipSubmitEdit
>
> -  git config [--global] git-p4.skipSubmitModTimeCheck false
> +  git config [--global] git-p4.skipSubmitEdit false
>
> -If true, submit will not check if the p4 change template has been modified.
> +Normally, git-p4 invokes an editor after each commit is applied so
> +that you can make changes to the submit message.  Setting this
> +variable to true will skip the editing step, submitting the change as is.
> +
> +git-p4.skipSubmitEditCheck
> +
> +  git config [--global] git-p4.skipSubmitEditCheck false
> +
> +After the editor is invoked, git-p4 normally makes sure you saved the
> +change description, as an indication that you did indeed read it over
> +and edit it.  You can quit without saving to abort the submit (or skip
> +this change and continue).  Setting this variable to true will cause
> +git-p4 not to check if you saved the change description.  This variable
> +only matters if git-p4.skipSubmitEdit has not been set to true.
>
>   git-p4.preserveUser
>
> diff --git a/t/t9804-skip-submit-edit.sh b/t/t9804-skip-submit-edit.sh
> new file mode 100755
> index 0000000..734ccf2
> --- /dev/null
> +++ b/t/t9804-skip-submit-edit.sh
> @@ -0,0 +1,82 @@
> +#!/bin/sh
> +
> +test_description='git-p4 skipSubmitEdit config variables'
> +
> +. ./lib-git-p4.sh
> +
> +test_expect_success 'start p4d' '
> +	start_p4d
> +'
> +
> +test_expect_success 'init depot' '
> +	(
> +		cd "$cli"&&
> +		echo file1>file1&&
> +		p4 add file1&&
> +		p4 submit -d "change 1"
> +	)
> +'
> +
> +# this works because EDITOR is set to :
> +test_expect_success 'no config, unedited, say yes' '
> +	"$GITP4" clone --dest="$git" //depot&&
> +	test_when_finished cleanup_git&&
> +	(
> +		cd "$git"&&
> +		echo line>>file1&&
> +		git commit -a -m "change 2"&&
> +		echo y | "$GITP4" submit&&
> +		p4 changes //depot/...>wc&&
> +		test_line_count = 2 wc
> +	)
> +'
> +
> +test_expect_success 'no config, unedited, say no' '
> +	"$GITP4" clone --dest="$git" //depot&&
> +	test_when_finished cleanup_git&&
> +	(
> +		cd "$git"&&
> +		echo line>>file1&&
> +		git commit -a -m "change 3 (not really)"&&
> +		printf "bad response\nn\n" | "$GITP4" submit
> +		p4 changes //depot/...>wc&&
> +		test_line_count = 2 wc
> +	)
> +'
> +
> +test_expect_success 'skipSubmitEdit' '
> +	"$GITP4" clone --dest="$git" //depot&&
> +	test_when_finished cleanup_git&&
> +	(
> +		cd "$git"&&
> +		git config git-p4.skipSubmitEdit true&&
> +		# will fail if editor is even invoked
> +		git config core.editor /bin/false&&
> +		echo line>>file1&&
> +		git commit -a -m "change 3"&&
> +		"$GITP4" submit&&
> +		p4 changes //depot/...>wc&&
> +		test_line_count = 3 wc
> +	)
> +'
> +
> +test_expect_success 'skipSubmitEditCheck' '
> +	"$GITP4" clone --dest="$git" //depot&&
> +	test_when_finished cleanup_git&&
> +	(
> +		cd "$git"&&
> +		git config git-p4.skipSubmitEditCheck true&&
> +		echo line>>file1&&
> +		git commit -a -m "change 4"&&
> +		"$GITP4" submit&&
> +		p4 changes //depot/...>wc&&
> +		test_line_count = 4 wc
> +	)
> +'
> +
> +
> +test_expect_success 'kill p4d' '
> +	kill_p4d
> +'
> +
> +test_done

Re: git-p4.skipSubmitEdit

[Pete Wyckoff]

luke@diamand.org wrote on Tue, 18 Oct 2011 18:53 +0100:
> Looks good, one minor nit (see below) and a comment.
[..]
> >+        # invoke the editor
> >+        if os.environ.has_key("P4EDITOR"):
> >+            editor = os.environ.get("P4EDITOR")
> >+        else:
> >+            editor = read_pipe("git var GIT_EDITOR").strip()
> >+        system(editor + " " + template_file)
> 
> This is where we should really check the return code. However, doing
> so seems to break lots of the existing tests so it's not as easy as
> it looks.

Indeed.  I'll not fix that now, but agree it should be.

> >+
> >+        # If the file was not saved, prompt to see if this patch should
> >+        # be skipped.  But skip this verification step if configured so.
> >+        if gitConfig("git-p4.skipSubmitEditCheck") == "true":
> >+            print "return true for skipSubmitEditCheck"
> 
> You print a helpful/annoying(?) message here, but not further up at
> skipSubmitEdit?

Aargh.  Leaked debug code.  Thanks for noticing.  I got rid of
it.

		-- Pete

Re: git-p4.skipSubmitEdit

[Michael Horowitz]

All,

It appears that this change has introduced a bug that causes submit to
fail every time if you do not skip the submit edit.

From what I can tell, this is because the new edit_template method
does not return True at the end.

Thanks,

Mike



On Wed, Oct 19, 2011 at 9:16 PM, Pete Wyckoff <pw@padd.com> wrote:
> luke@diamand.org wrote on Tue, 18 Oct 2011 18:53 +0100:
>> Looks good, one minor nit (see below) and a comment.
> [..]
>> >+        # invoke the editor
>> >+        if os.environ.has_key("P4EDITOR"):
>> >+            editor = os.environ.get("P4EDITOR")
>> >+        else:
>> >+            editor = read_pipe("git var GIT_EDITOR").strip()
>> >+        system(editor + " " + template_file)
>>
>> This is where we should really check the return code. However, doing
>> so seems to break lots of the existing tests so it's not as easy as
>> it looks.
>
> Indeed.  I'll not fix that now, but agree it should be.
>
>> >+
>> >+        # If the file was not saved, prompt to see if this patch should
>> >+        # be skipped.  But skip this verification step if configured so.
>> >+        if gitConfig("git-p4.skipSubmitEditCheck") == "true":
>> >+            print "return true for skipSubmitEditCheck"
>>
>> You print a helpful/annoying(?) message here, but not further up at
>> skipSubmitEdit?
>
> Aargh.  Leaked debug code.  Thanks for noticing.  I got rid of
> it.
>
>                -- Pete
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git-p4.skipSubmitEdit

[Luke Diamand]

On 16/12/11 15:38, Michael Horowitz wrote:
> All,
>
> It appears that this change has introduced a bug that causes submit to
> fail every time if you do not skip the submit edit.
>
>  From what I can tell, this is because the new edit_template method
> does not return True at the end.

Could you say exactly what you're doing?

I've just tried it myself and it seems to work fine:

git-p4 clone
git commit -m 'a change'
git-p4 submit
<quit from editor, with/without modifying it>

And I couldn't see any paths in edit_template that returned without a 
value of True, except the one where the user decides to bail out.

This is with Pete's skipSubmitEdit change.

Thanks!
Luke

Re: git-p4.skipSubmitEdit

[Michael Horowitz]

Actually, it is the opposite.  Bailout works fine, it is when I ":wq"
in Vi for example, that it fails to submit and reverts all my changes.

        if os.stat(template_file).st_mtime <= mtime:
            while True:
                response = raw_input("Submit template unchanged.
Submit anyway? [y]es, [n]o (skip this patch) ")
                if response == 'y':
                    return True
                if response == 'n':
                    return False
        # I think this else needs to be added here, so when the file
has been modified since it was opened in the editor, it will properly
submit the change.
        else:
            return True


Mike



On Fri, Dec 16, 2011 at 2:50 PM, Luke Diamand <luke@diamand.org> wrote:
>
> On 16/12/11 15:38, Michael Horowitz wrote:
>>
>> All,
>>
>> It appears that this change has introduced a bug that causes submit to
>> fail every time if you do not skip the submit edit.
>>
>>  From what I can tell, this is because the new edit_template method
>> does not return True at the end.
>
>
> Could you say exactly what you're doing?
>
> I've just tried it myself and it seems to work fine:
>
> git-p4 clone
> git commit -m 'a change'
> git-p4 submit
> <quit from editor, with/without modifying it>
>
> And I couldn't see any paths in edit_template that returned without a value of True, except the one where the user decides to bail out.
>
> This is with Pete's skipSubmitEdit change.
>
> Thanks!
> Luke

Re: git-p4.skipSubmitEdit

[Michael Horowitz]

Oh, and in the case where you do edit the template, it doesn't give
you an error or anything, it looks like it succeeded, but you'll
notice the change never got submitted to Perforce.  If you look
carefully though, you can see it reverting each of your edited files
in the P4 tree.

Mike



On Fri, Dec 16, 2011 at 7:46 PM, Michael Horowitz
<michael.horowitz@ieee.org> wrote:
> Actually, it is the opposite.  Bailout works fine, it is when I ":wq"
> in Vi for example, that it fails to submit and reverts all my changes.
>
>        if os.stat(template_file).st_mtime <= mtime:
>            while True:
>                response = raw_input("Submit template unchanged.
> Submit anyway? [y]es, [n]o (skip this patch) ")
>                if response == 'y':
>                    return True
>                if response == 'n':
>                    return False
>        # I think this else needs to be added here, so when the file
> has been modified since it was opened in the editor, it will properly
> submit the change.
>        else:
>            return True
>
>
> Mike
>
>
>
> On Fri, Dec 16, 2011 at 2:50 PM, Luke Diamand <luke@diamand.org> wrote:
>>
>> On 16/12/11 15:38, Michael Horowitz wrote:
>>>
>>> All,
>>>
>>> It appears that this change has introduced a bug that causes submit to
>>> fail every time if you do not skip the submit edit.
>>>
>>>  From what I can tell, this is because the new edit_template method
>>> does not return True at the end.
>>
>>
>> Could you say exactly what you're doing?
>>
>> I've just tried it myself and it seems to work fine:
>>
>> git-p4 clone
>> git commit -m 'a change'
>> git-p4 submit
>> <quit from editor, with/without modifying it>
>>
>> And I couldn't see any paths in edit_template that returned without a value of True, except the one where the user decides to bail out.
>>
>> This is with Pete's skipSubmitEdit change.
>>
>> Thanks!
>> Luke

[PATCH] git-p4: fix skipSubmitEdit regression

[Pete Wyckoff]

Commit 7c766e5 (git-p4: introduce skipSubmitEdit, 2011-12-04)
made it easier to automate submission to p4, but broke the most
common case.

Add a test for when the user really does edit and save the change
template, and fix the bug that causes the test to fail.

Also add a confirmation message when submission is cancelled.

Reported-by: Michael Horowitz <michael.horowitz@ieee.org>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
michael.horowitz@ieee.org wrote on Fri, 16 Dec 2011 19:49 -0500:
> Oh, and in the case where you do edit the template, it doesn't give
> you an error or anything, it looks like it succeeded, but you'll
> notice the change never got submitted to Perforce.  If you look
> carefully though, you can see it reverting each of your edited files
> in the P4 tree.
[..]
> >> On 16/12/11 15:38, Michael Horowitz wrote:
> >>>
> >>> It appears that this change has introduced a bug that causes submit to
> >>> fail every time if you do not skip the submit edit.
> >>>
> >>> From what I can tell, this is because the new edit_template method
> >>> does not return True at the end.

Oops.  In adding this code, I failed to test what should be the
normal code path.  How sad.

Junio:  this bug is in master.  Could you apply this fix?

		-- Pete

 contrib/fast-import/git-p4  |   18 +++++++++++-------
 t/t9805-skip-submit-edit.sh |   24 +++++++++++++++++++++++-
 2 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 3571362..d501eac 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -878,13 +878,16 @@ class P4Submit(Command, P4UserMap):
         if gitConfig("git-p4.skipSubmitEditCheck") == "true":
             return True
 
-        if os.stat(template_file).st_mtime <= mtime:
-            while True:
-                response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
-                if response == 'y':
-                    return True
-                if response == 'n':
-                    return False
+        # modification time updated means user saved the file
+        if os.stat(template_file).st_mtime > mtime:
+            return True
+
+        while True:
+            response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
+            if response == 'y':
+                return True
+            if response == 'n':
+                return False
 
     def applyCommit(self, id):
         print "Applying %s" % (read_pipe("git log --max-count=1 --pretty=oneline %s" % id))
@@ -1068,6 +1071,7 @@ class P4Submit(Command, P4UserMap):
                         self.modifyChangelistUser(changelist, p4User)
             else:
                 # skip this patch
+                print "Submission cancelled, undoing p4 changes."
                 for f in editedFiles:
                     p4_revert(f)
                 for f in filesToAdd:
diff --git a/t/t9805-skip-submit-edit.sh b/t/t9805-skip-submit-edit.sh
index 734ccf2..df929e0 100755
--- a/t/t9805-skip-submit-edit.sh
+++ b/t/t9805-skip-submit-edit.sh
@@ -38,7 +38,7 @@ test_expect_success 'no config, unedited, say no' '
 		cd "$git" &&
 		echo line >>file1 &&
 		git commit -a -m "change 3 (not really)" &&
-		printf "bad response\nn\n" | "$GITP4" submit
+		printf "bad response\nn\n" | "$GITP4" submit &&
 		p4 changes //depot/... >wc &&
 		test_line_count = 2 wc
 	)
@@ -74,6 +74,28 @@ test_expect_success 'skipSubmitEditCheck' '
 	)
 '
 
+# check the normal case, where the template really is edited
+test_expect_success 'no config, edited' '
+	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
+	ed="$TRASH_DIRECTORY/ed.sh" &&
+	test_when_finished "rm \"$ed\"" &&
+	cat >"$ed" <<-EOF &&
+		#!$SHELL_PATH
+		sleep 1
+		touch "\$1"
+		exit 0
+	EOF
+	chmod 755 "$ed" &&
+	(
+		cd "$git" &&
+		echo line >>file1 &&
+		git commit -a -m "change 5" &&
+		EDITOR="\"$ed\"" "$GITP4" submit &&
+		p4 changes //depot/... >wc &&
+		test_line_count = 5 wc
+	)
+'
 
 test_expect_success 'kill p4d' '
 	kill_p4d
-- 
1.7.8.154.g767b7.dirty