Re: git-p4.skipSubmitEdit

["L. A. Linden Levy" <alevy@mobitv.com>]

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