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

From: Tor Arvid Lund <torarvid@gmail.com>
Date: Oct 14, 2011, 22:31 UTC
Message-ID: <CA+DMoH-HqA0DCyUSttO-iYO0rUHq1nLqM9W0imAOjHC5H1r_9w@mail.gmail.com>
In-Reply-To: <1318629110-15232-1-git-send-email-andreiw@vmware.com>

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

Show 19 quoted lines

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

Show 39 quoted lines

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

Show 11 quoted lines

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