Re: [PATCHv2 2/2] git p4: import/export of labels to/from p4

luke@diamand.org wrote on Wed, 11 Apr 2012 17:21 +0200:

The existing label import code looks at each commit being
imported, and then checks for labels at that commit. This
doesn't work in the real world though because it will drop
labels applied on changelists that have already been imported,
a common pattern.

This change adds a new --import-labels option. With this option,
at the end of the sync, git p4 gets sets of labels in p4 and git,
and then creates a git tag for each missing p4 label.

This means that tags created on older changelists are
still imported.

Tags that could not be imported are added to an ignore
list.

The same sets of git and p4 tags and labels can also be used to
derive a list of git tags to export to p4. This is enabled with
--export-labels in 'git p4 submit'.

This is a good approach.  Here's some late review comments.

+git-p4.validLabelRegexp::
+	Only p4 labels matching this regular expression will be imported. The
+	default value is '[A-Z0-9_\-.]+$'.
+

and

+git-p4.validLabelRegexp::
+	Only p4 labels matching this regular expression will be exported. The
+	default value is '[A-Z0-9_\-.]+$'.

This one wants to be validTagRegexp.  Or you could combine them
both into one.  Why no small a-z characters?

+            # Get the p4 commit this corresponds to
+            changelist = None
+            for l in read_pipe_lines(["git", "log", "--max-count=1", name]):
+                match = commit_re.match(l)
+                if match:
+                    changelist = match.group(1)

We have extractLogMessageFromGitCommit and extractSettingsGitLog
to grep out the "git-p4.. change" tag.  They're not beautiful,
but we should reuse them, in case this mechanism of connecting
changes to commits ever changes.

+            # Get the tag details.
+            inHeader = True
+            isAnnotated = False
+            body = []
+            for l in read_pipe_lines(["git", "cat-file", "-p", name]):
+                l = l.strip()
+                if inHeader:
+                    if re.match(r'tag\s+', l):
+                        isAnnotated = True
+                    elif re.match(r'\s*$', l):
+                        inHeader = False
+                        continue
+                else:
+                    body.append(l)
+
+            if not isAnnotated:
+                body = ["lightweight tag imported by git p4\n"]

The manual parsing, just to get the text in the tag (or ref),
seems a bit awkward.  I looked at "git show --pretty=format:%B"
as a way to get just the text, and "git cat-file -t ref" to get
the tag/ref difference.  But no easy replacement.

+            if change.has_key('change'):
+                # find the corresponding git commit; take the oldest commit
+                changelist = int(change['change'])
+                gitCommit = read_pipe(["git", "rev-list", "--max-count=1",
+                     "--reverse", ":/\[git-p4:.*change = %d\]" % changelist])
+                if len(gitCommit) == 0:
+                    print "could not find git commit for changelist %d" % changelist
+                else:
+                    gitCommit = gitCommit.strip()
+                    commitFound = True
+                    # Convert from p4 time format
+                    try:
+                        tmwhen = time.strptime(labelDetails['Update'], "%Y/%m/%d %H:%M:%S")
+                    except ValueError:
+                        print "Could not convert label time %s" % labelDetail['Update']
+                        tmwhen = 1
+
+                    when = int(time.mktime(tmwhen))
+                    self.streamTag(stream, name, labelDetails, gitCommit, when)
+                    if verbose:
+                        print "p4 label %s mapped to git commit %s" % (name, gitCommit)

Nice, even the icky but required p4 time parsing.  We don't have
a common function to go from change -> commit yet.

 class P4Rebase(Command):
     def __init__(self):
         Command.__init__(self)
-        self.options = [ ]
+        self.options = [
+                optparse.make_option("--import-labels", dest="importLabels", action="store_true"),
+                optparse.make_option("--verbose", dest="verbose", action="store_true"),
+        ]
+        self.verbose = False
+        self.importLabels = False
         self.description = ("Fetches the latest revision from perforce and "
                             + "rebases the current work (branch) against it")
-        self.verbose = False

All commands should have a --verbose.  Could you move the
"--verbose" description in the man page up into "General
options"?  Since it means the same thing in all the commands,
roughly.

     def run(self, args):
         sync = P4Sync()
+        sync.importLabels = self.importLabels
         sync.run([])

But no "sync.verbose = self.verbose"?  I wonder if P4Rebase should
inherit from P4Sync, like Clone does.  But that is a bit obscure
to me already.  Probably these all want to be split out into a
bunch of functions; there isn't a lot of state tracking happening
in the classes, and only one instance of each per invocation.

This whole set of tests can go it the existing t9804, right?  It
seems that the first few of these are duplicates of what is
already in there.

Nice test coverage, and you fixed the broken one in t9804.

		-- Pete