patchGit-p4: git-p4.changeOnSubmit to do 'change' instead of 'submit'.

> 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
>
> --
> 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
>

> > 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'.
> 

> 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

> 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!
> 

> 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
>

> 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.

> 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.

> 
> 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.