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@xxxxxxxxxx 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@xxxxxxxxx>
> Signed-off-by: Pete Wyckoff <pw@xxxxxxxx>
> ---
> 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@xxxxxxxxxx web www.mobitv.com
Attachment:
signature.asc
Description: This is a digitally signed message part