[PATCH] Git-p4: git-p4.changeOnSubmit to do 'change' instead of 'submit'.

[PATCH] Git-p4: git-p4.changeOnSubmit to do 'change' instead of 'submit'.

[Andrei Warkentin]

Many users of p4/sd use changelists for review, regression
tests and batch builds, thus changes are almost never directly
submitted.

This new config option lets a 'p4 change -i' run instead of
the 'p4 submit -i'.

Signed-off-by: Andrei Warkentin <andreiw@vmware.com>
---
 contrib/fast-import/git-p4     |   16 ++++++++++++----
 contrib/fast-import/git-p4.txt |   10 ++++++++++
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 2f7b270..19c295b 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -959,7 +959,10 @@ class P4Submit(Command, P4UserMap):
                 submitTemplate = message[:message.index(separatorLine)]
                 if self.isWindows:
                     submitTemplate = submitTemplate.replace("\r\n", "\n")
-                p4_write_pipe("submit -i", submitTemplate)
+                if gitConfig("git-p4.changeOnSubmit"):
+                    p4_write_pipe("change -i", submitTemplate)
+                else:
+                    p4_write_pipe("subadasdmit -i", submitTemplate)
 
                 if self.preserveUser:
                     if p4User:
@@ -981,9 +984,14 @@ class P4Submit(Command, P4UserMap):
             file = open(fileName, "w+")
             file.write(self.prepareLogMessage(template, logMessage))
             file.close()
-            print ("Perforce submit template written as %s. "
-                   + "Please review/edit and then use p4 submit -i < %s to submit directly!"
-                   % (fileName, fileName))
+            if gitConfig("git-p4.changeOnSubmit"):
+                print ("Perforce submit template written as %s. "
+                       + "Please review/edit and then use p4 change -i < %s to create changelist!"
+                       % (fileName, fileName))
+            else:
+                print ("Perforce submit template written as %s. "
+                       + "Please review/edit and then use p4 submit -i < %s to submit directly!"
+                       % (fileName, fileName))
 
     def run(self, args):
         if len(args) == 0:
diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt
index 52003ae..3a3a815 100644
--- a/contrib/fast-import/git-p4.txt
+++ b/contrib/fast-import/git-p4.txt
@@ -180,6 +180,16 @@ git-p4.allowSubmit
 
   git config [--global] git-p4.allowSubmit false
 
+git-p4.changeOnSubmit
+
+  git config [--global] git-p4.changeOnSubmit false
+
+Most places using p4/sourcedepot don't actually want you submit
+changes directly, and changelists are used to do regression testing,
+batch builds and review, hence, by setting this parameter to
+true you acknowledge you end up creating a changelist which you
+must then manually commit.
+
 git-p4.syncFromOrigin
 
 A useful setup may be that you have a periodically updated git repository
-- 
1.7.4.1

Re: [PATCH] Git-p4: git-p4.changeOnSubmit to do 'change' instead of 'submit'.

[Tor Arvid Lund]

Hi Andrei, and thanks for trying to help improve git-p4! :-)

2011/10/14 Andrei Warkentin <andreiw@vmware.com>:
> Many users of p4/sd use changelists for review, regression
> tests and batch builds, thus changes are almost never directly
> submitted.

Just out of curiosity... what is 'sd'?

> This new config option lets a 'p4 change -i' run instead of
> the 'p4 submit -i'.

Well... I have to say that I'm not crazy about this patch... I don't
think it is very elegant to have a config flag that says that "when
the user says 'git p4 submit', then don't submit, but do something
else instead".

I would much rather have made a patch to introduce some new command
like 'git p4 change'.

> Signed-off-by: Andrei Warkentin <andreiw@vmware.com>
> ---
>  contrib/fast-import/git-p4     |   16 ++++++++++++----
>  contrib/fast-import/git-p4.txt |   10 ++++++++++
>  2 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
> index 2f7b270..19c295b 100755
> --- a/contrib/fast-import/git-p4
> +++ b/contrib/fast-import/git-p4
> @@ -959,7 +959,10 @@ class P4Submit(Command, P4UserMap):
>                 submitTemplate = message[:message.index(separatorLine)]
>                 if self.isWindows:
>                     submitTemplate = submitTemplate.replace("\r\n", "\n")
> -                p4_write_pipe("submit -i", submitTemplate)
> +                if gitConfig("git-p4.changeOnSubmit"):
> +                    p4_write_pipe("change -i", submitTemplate)
> +                else:
> +                    p4_write_pipe("subadasdmit -i", submitTemplate)

... 'subadasdmit'? Did some debug/test code sneak in to your patch?

>
>                 if self.preserveUser:
>                     if p4User:
> @@ -981,9 +984,14 @@ class P4Submit(Command, P4UserMap):
>             file = open(fileName, "w+")
>             file.write(self.prepareLogMessage(template, logMessage))
>             file.close()
> -            print ("Perforce submit template written as %s. "
> -                   + "Please review/edit and then use p4 submit -i < %s to submit directly!"
> -                   % (fileName, fileName))
> +            if gitConfig("git-p4.changeOnSubmit"):
> +                print ("Perforce submit template written as %s. "
> +                       + "Please review/edit and then use p4 change -i < %s to create changelist!"
> +                       % (fileName, fileName))
> +            else:
> +                print ("Perforce submit template written as %s. "
> +                       + "Please review/edit and then use p4 submit -i < %s to submit directly!"
> +                       % (fileName, fileName))
>
>     def run(self, args):
>         if len(args) == 0:
> diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt
> index 52003ae..3a3a815 100644
> --- a/contrib/fast-import/git-p4.txt
> +++ b/contrib/fast-import/git-p4.txt
> @@ -180,6 +180,16 @@ git-p4.allowSubmit
>
>   git config [--global] git-p4.allowSubmit false
>
> +git-p4.changeOnSubmit
> +
> +  git config [--global] git-p4.changeOnSubmit false
> +
> +Most places using p4/sourcedepot don't actually want you submit
> +changes directly, and changelists are used to do regression testing,
> +batch builds and review, hence, by setting this parameter to
> +true you acknowledge you end up creating a changelist which you
> +must then manually commit.
> +

It might be just me, but I never heard of 'sourcedepot' before this
patch... Google tells me that it might be some old version of p4 that
Microsoft used many many years ago. If that's the case, maybe it
doesn't add much value to talk about it in git-p4 docs.. (??)

And you claim 'most places don't want people to submit directly'. I'm
not sure I agree with that, and anyway it seems like the git-p4 docs
should be phrased more neutral than that.


My advice would be, as I mentioned earlier, to rework this into a
patch introducing a separate command 'git p4 change' instead of this
config flag.

Have a good one!

   Tor Arvid

>  git-p4.syncFromOrigin
>
>  A useful setup may be that you have a periodically updated git repository
> --
> 1.7.4.1
>
> --
> 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: [PATCH] Git-p4: git-p4.changeOnSubmit to do 'change' instead of 'submit'.

[Andrei Warkentin]

Hi Tor,

Thanks for the review!

----- Original Message -----
> Just out of curiosity... what is 'sd'?
> 

SourceDepot, a p4 fork that is used elsewhere, not by me though ;).

> > This new config option lets a 'p4 change -i' run instead of
> > the 'p4 submit -i'.
> 
> Well... I have to say that I'm not crazy about this patch... I don't
> think it is very elegant to have a config flag that says that "when
> the user says 'git p4 submit', then don't submit, but do something
> else instead".
> 
> I would much rather have made a patch to introduce some new command
> like 'git p4 change'.
> 

Agreed, how about something like this?

The commands dict maps command name to class and optional dict passed to cmd.run(). That way 'change'
can really mean P4Submit with an extra parameter not to submit but to do a changelist instead. The
reason why I initially made the config flag was because I didn't want to copy-paste P4Submit into P4Change.

commands = {
    "debug" : [ P4Debug, {} ]
    "submit" : [ P4Submit, { "doChange" : 0 } ]
    "commit" : [ P4Submit, { "doChange" : 0 } ]
    "change" : [ P4Submit, { "doChange" : 1 } ]
    "sync" : [ P4Sync, {} ],
    "rebase" : [ P4Rebase, {} ],
    "clone" : [ P4Clone, {} ],
    "rollback" : [ P4RollBack, {} ],
    "branches" : [ P4Branches, {} ]
}

A

Re: [PATCH] Git-p4: git-p4.changeOnSubmit to do 'change' instead of 'submit'.

[Luke Diamand]

On 14/10/11 22:51, Andrei Warkentin wrote:
> Many users of p4/sd use changelists for review, regression
> tests and batch builds, thus changes are almost never directly
> submitted.
>
> This new config option lets a 'p4 change -i' run instead of
> the 'p4 submit -i'.
>
> Signed-off-by: Andrei Warkentin<andreiw@vmware.com>
> ---
>   contrib/fast-import/git-p4     |   16 ++++++++++++----
>   contrib/fast-import/git-p4.txt |   10 ++++++++++
>   2 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
> index 2f7b270..19c295b 100755
> --- a/contrib/fast-import/git-p4
> +++ b/contrib/fast-import/git-p4
> @@ -959,7 +959,10 @@ class P4Submit(Command, P4UserMap):
>                   submitTemplate = message[:message.index(separatorLine)]
>                   if self.isWindows:
>                       submitTemplate = submitTemplate.replace("\r\n", "\n")
> -                p4_write_pipe("submit -i", submitTemplate)
> +                if gitConfig("git-p4.changeOnSubmit"):
> +                    p4_write_pipe("change -i", submitTemplate)
> +                else:
> +                    p4_write_pipe("subadasdmit -i", submitTemplate)


What does "p4 subadasmit" do? That's a new command to me!

(This patch also fails to apply cleanly to my shell-metacharacter patch).

>
>                   if self.preserveUser:
>                       if p4User:
> @@ -981,9 +984,14 @@ class P4Submit(Command, P4UserMap):
>               file = open(fileName, "w+")
>               file.write(self.prepareLogMessage(template, logMessage))
>               file.close()
> -            print ("Perforce submit template written as %s. "
> -                   + "Please review/edit and then use p4 submit -i<  %s to submit directly!"
> -                   % (fileName, fileName))
> +            if gitConfig("git-p4.changeOnSubmit"):
> +                print ("Perforce submit template written as %s. "
> +                       + "Please review/edit and then use p4 change -i<  %s to create changelist!"
> +                       % (fileName, fileName))
> +            else:
> +                print ("Perforce submit template written as %s. "
> +                       + "Please review/edit and then use p4 submit -i<  %s to submit directly!"
> +                       % (fileName, fileName))
>
>       def run(self, args):
>           if len(args) == 0:
> diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt
> index 52003ae..3a3a815 100644
> --- a/contrib/fast-import/git-p4.txt
> +++ b/contrib/fast-import/git-p4.txt
> @@ -180,6 +180,16 @@ git-p4.allowSubmit
>
>     git config [--global] git-p4.allowSubmit false
>
> +git-p4.changeOnSubmit
> +
> +  git config [--global] git-p4.changeOnSubmit false
> +
> +Most places using p4/sourcedepot don't actually want you submit

Small typo: should be "want you *to* submit"

> +changes directly, and changelists are used to do regression testing,
> +batch builds and review, hence, by setting this parameter to
> +true you acknowledge you end up creating a changelist which you
> +must then manually commit.
> +
>   git-p4.syncFromOrigin
>
>   A useful setup may be that you have a periodically updated git repository

Regards!
Luke

Re: [PATCH] Git-p4: git-p4.changeOnSubmit to do 'change' instead of 'submit'.

[Andrei Warkentin]

Hi,

----- Original Message -----
> From: "Luke Diamand" <luke@diamand.org>
> To: "Andrei Warkentin" <andreiw@vmware.com>
> Cc: git@vger.kernel.org, gitster@pobox.com, "Pete Wyckoff" <pw@padd.com>
> Sent: Saturday, October 15, 2011 4:10:58 PM
> Subject: Re: [PATCH] Git-p4: git-p4.changeOnSubmit to do 'change' instead of 'submit'.
> 
> On 14/10/11 22:51, Andrei Warkentin wrote:
> > Many users of p4/sd use changelists for review, regression
> > tests and batch builds, thus changes are almost never directly
> > submitted.
> >
> > This new config option lets a 'p4 change -i' run instead of
> > the 'p4 submit -i'.
> >
> > Signed-off-by: Andrei Warkentin<andreiw@vmware.com>
> > ---
> >   contrib/fast-import/git-p4     |   16 ++++++++++++----
> >   contrib/fast-import/git-p4.txt |   10 ++++++++++
> >   2 files changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/contrib/fast-import/git-p4
> > b/contrib/fast-import/git-p4
> > index 2f7b270..19c295b 100755
> > --- a/contrib/fast-import/git-p4
> > +++ b/contrib/fast-import/git-p4
> > @@ -959,7 +959,10 @@ class P4Submit(Command, P4UserMap):
> >                   submitTemplate =
> >                   message[:message.index(separatorLine)]
> >                   if self.isWindows:
> >                       submitTemplate =
> >                       submitTemplate.replace("\r\n", "\n")
> > -                p4_write_pipe("submit -i", submitTemplate)
> > +                if gitConfig("git-p4.changeOnSubmit"):
> > +                    p4_write_pipe("change -i", submitTemplate)
> > +                else:
> > +                    p4_write_pipe("subadasdmit -i",
> > submitTemplate)
> 
> 
> What does "p4 subadasmit" do? That's a new command to me!
> 

Ack, that's emabarrasing. How did that get there :-)?

Anyway, the other suggestion I had was to create a new command
instead of overriding behaviour of an existing one. Of course,
copy-pasting P4Submit into P4Change is silly, so...

How about something like this?

The commands dict maps command name to class and optional dict passed to cmd.run(). That way 'change'
can really mean P4Submit with an extra parameter not to submit but to do a changelist instead. The
reason why I initially made the config flag was because I didn't want to copy-paste P4Submit into P4Change.

commands = {
    "debug" : [ P4Debug, {} ]
    "submit" : [ P4Submit, { "doChange" : 0 } ]
    "commit" : [ P4Submit, { "doChange" : 0 } ]
    "change" : [ P4Submit, { "doChange" : 1 } ]
    "sync" : [ P4Sync, {} ],
    "rebase" : [ P4Rebase, {} ],
    "clone" : [ P4Clone, {} ],
    "rollback" : [ P4RollBack, {} ],
    "branches" : [ P4Branches, {} ]
}

Thanks for the review,
A

Re: [PATCH] Git-p4: git-p4.changeOnSubmit to do 'change' instead of 'submit'.

[Luke Diamand]

On 17/10/11 17:18, Andrei Warkentin wrote:
> Hi,
>
> ----- Original Message -----
> Anyway, the other suggestion I had was to create a new command
> instead of overriding behaviour of an existing one. Of course,
> copy-pasting P4Submit into P4Change is silly, so...
>
> How about something like this?
>
> The commands dict maps command name to class and optional dict passed to cmd.run(). That way 'change'
> can really mean P4Submit with an extra parameter not to submit but to do a changelist instead. The
> reason why I initially made the config flag was because I didn't want to copy-paste P4Submit into P4Change.
>
> commands = {
>      "debug" : [ P4Debug, {} ]
>      "submit" : [ P4Submit, { "doChange" : 0 } ]
>      "commit" : [ P4Submit, { "doChange" : 0 } ]
>      "change" : [ P4Submit, { "doChange" : 1 } ]
>      "sync" : [ P4Sync, {} ],
>      "rebase" : [ P4Rebase, {} ],
>      "clone" : [ P4Clone, {} ],
>      "rollback" : [ P4RollBack, {} ],
>      "branches" : [ P4Branches, {} ]
> }
>
> Thanks for the review,
> A
>

Sounds plausible to me. The alternative would be a command line 
parameter, although that could get annoying and error prone, especially 
as you can't easily unsubmit a perforce change.

Re: [PATCH] Git-p4: git-p4.changeOnSubmit to do 'change' instead of 'submit'.

[Pete Wyckoff]

luke@diamand.org wrote on Mon, 17 Oct 2011 19:53 +0100:
> On 17/10/11 17:18, Andrei Warkentin wrote:
> >Hi,
> >
> >----- Original Message -----
> >Anyway, the other suggestion I had was to create a new command
> >instead of overriding behaviour of an existing one. Of course,
> >copy-pasting P4Submit into P4Change is silly, so...
> >
> >How about something like this?
> >
> >The commands dict maps command name to class and optional dict passed to cmd.run(). That way 'change'
> >can really mean P4Submit with an extra parameter not to submit but to do a changelist instead. The
> >reason why I initially made the config flag was because I didn't want to copy-paste P4Submit into P4Change.
> >
> >commands = {
> >     "debug" : [ P4Debug, {} ]
> >     "submit" : [ P4Submit, { "doChange" : 0 } ]
> >     "commit" : [ P4Submit, { "doChange" : 0 } ]
> >     "change" : [ P4Submit, { "doChange" : 1 } ]
> >     "sync" : [ P4Sync, {} ],
> >     "rebase" : [ P4Rebase, {} ],
> >     "clone" : [ P4Clone, {} ],
> >     "rollback" : [ P4RollBack, {} ],
> >     "branches" : [ P4Branches, {} ]
> >}
> >
> >Thanks for the review,
> >A
> >
> 
> Sounds plausible to me. The alternative would be a command line
> parameter, although that could get annoying and error prone,
> especially as you can't easily unsubmit a perforce change.

This seems like a useful thing to do, but needs some care.

Git can have multiple commits outstanding that touch the same
file, but p4 cannot really have multiple pending changes in the
same workspace that touch the same file.

If you call "git-p4 change", it would build a p4 change for each
of those commits.  If the commits happen to touch the same file,
the changes get rearranged as far as p4 is concerned so that all
changes to a given file are lumped in the first change that sees
the file.  This is highly counterintuitive from a git mindset.

The most restrictive implementation would have to:

    1.  ensure no pending changes in the P4 clientPath
    2.  ensure number of commits ("git rev-list") is 1

You could be more permissive, allowing multiple pending changes
if the file sets do not conflict.  In that case, the first test
would look at the files in pending changes and allow the
operation if they did not intersect with files in origin..master.
The second would make sure that each file appears in no more than
1 commit in origin..master.

Also make sure this works with preserveUser.  Not sure if an
unsubmitted change can be handled the same way.

Because it feels like a delicate operation that could have big
negative consequences, this needs a few unit tests.

For the code structure, I'd like to see a proper subclass instead
of the dictionary idea.  Something like, e.g.:

class P4Submit(...):
    def __init__(self, change_only=0)
	...
	self.change_only = change_only

class P4Change(P4Submit):
    def __init__(self):
	P4Submit.__init__(self, change_only=1)

Sorry this is looking so difficult now.

		-- Pete

Re: [PATCH] Git-p4: git-p4.changeOnSubmit to do 'change' instead of 'submit'.

[Andrei Warkentin]

Hi,

----- Original Message -----
> From: "Pete Wyckoff" <pw@padd.com>
> To: "Andrei Warkentin" <awarkentin@vmware.com>
> Cc: git@vger.kernel.org, "Andrei Warkentin" <andreiw@vmware.com>, "Tor Arvid Lund" <torarvid@gmail.com>, "Luke
> Diamand" <luke@diamand.org>, gitster@pobox.com
> Sent: Monday, October 17, 2011 6:32:02 PM
> Subject: Re: [PATCH] Git-p4: git-p4.changeOnSubmit to do 'change' instead of 'submit'.
> 
> Git can have multiple commits outstanding that touch the same
> file, but p4 cannot really have multiple pending changes in the
> same workspace that touch the same file.
> 
> If you call "git-p4 change", it would build a p4 change for each
> of those commits.  If the commits happen to touch the same file,
> the changes get rearranged as far as p4 is concerned so that all
> changes to a given file are lumped in the first change that sees
> the file.  This is highly counterintuitive from a git mindset.
> 
> The most restrictive implementation would have to:
> 
>     1.  ensure no pending changes in the P4 clientPath
>     2.  ensure number of commits ("git rev-list") is 1
> 
> You could be more permissive, allowing multiple pending changes
> if the file sets do not conflict.  In that case, the first test
> would look at the files in pending changes and allow the
> operation if they did not intersect with files in origin..master.
> The second would make sure that each file appears in no more than
> 1 commit in origin..master.

Hmmm...I see. I'll think some more about it, then!

> 
> Also make sure this works with preserveUser.  Not sure if an
> unsubmitted change can be handled the same way.
> 
> Because it feels like a delicate operation that could have big
> negative consequences, this needs a few unit tests.
> 
> For the code structure, I'd like to see a proper subclass instead
> of the dictionary idea.  Something like, e.g.:
> 
> class P4Submit(...):
>     def __init__(self, change_only=0)
> 	...
> 	self.change_only = change_only
> 
> class P4Change(P4Submit):
>     def __init__(self):
> 	P4Submit.__init__(self, change_only=1)
> 
> Sorry this is looking so difficult now.

No problem!

A