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

From: Pete Wyckoff <pw@padd.com>
Date: Oct 17, 2011, 22:32 UTC
Message-ID: <20111017223202.GA1834@arf.padd.com>
In-Reply-To: <4E9C799E.70700@diamand.org>

luke@diamand.org wrote on Mon, 17 Oct 2011 19:53 +0100:

Show 33 quoted lines

> 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