Re: [PATCH v2] git-p4: add P4 jobs to git commit message

> Jan Durovec <jan.durovec@gmail.com> writes:
>
>> When migrating from Perforce to git the information about P4 jobs
>> associated with P4 changelists is lost.
>>
>> Having these jobs listed on messages of related git commits enables smooth
>> migration for projects that take advantage of e.g. JIRA integration
>> (which uses jobs on Perforce side and parses commit messages on git side).
>>
>> The jobs are added to the message in the same format as is expected when
>> migrating in the reverse direction.
>>
>> Signed-off-by: Jan Durovec <jan.durovec@gmail.com>
>> ---
>
> Thanks for describing the change more throughly than the previous
> round.
>
> Luke, how does this one look?
>
>>  git-p4.py              | 12 ++++++
>>  t/lib-git-p4.sh        | 10 +++++
>>  t/t9829-git-p4-jobs.sh | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 121 insertions(+)
>>  create mode 100755 t/t9829-git-p4-jobs.sh
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index 527d44b..8f869d7 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -2320,6 +2320,15 @@ def extractFilesFromCommit(self, commit):
>>              fnum = fnum + 1
>>          return files
>>
>> +    def extractJobsFromCommit(self, commit):
>> +        jobs = []
>> +        jnum = 0
>> +        while commit.has_key("job%s" % jnum):
>> +            job = commit["job%s" % jnum]
>> +            jobs.append(job)
>> +            jnum = jnum + 1
>
> I am not familiar with "Perforce jobs", but I assume that they are
> always named as "job" + small non-negative integer in a dense way
> and it is OK for this loop to always begin at 0 and immediately stop
> when job + num does not exist (i.e. if job7 is missing, it is
> guaranteed that we will not see job8).

>
>> +        return jobs
>> +
>>      def stripRepoPath(self, path, prefixes):
>>          """When streaming files, this is called to map a p4 depot path
>>             to where it should go in git.  The prefixes are either
>> @@ -2665,6 +2674,7 @@ def hasBranchPrefix(self, path):
>>      def commit(self, details, files, branch, parent = ""):
>>          epoch = details["time"]
>>          author = details["user"]
>> +        jobs = self.extractJobsFromCommit(details)
>>
>>          if self.verbose:
>>              print('commit into {0}'.format(branch))
>> @@ -2692,6 +2702,8 @@ def commit(self, details, files, branch, parent = ""):
>>
>>          self.gitStream.write("data <<EOT\n")
>>          self.gitStream.write(details["desc"])
>> +        if len(jobs) > 0:
>> +            self.gitStream.write("\nJobs: %s" % (' '.join(jobs)))
>>          self.gitStream.write("\n[git-p4: depot-paths = \"%s\": change = %s" %
>>                               (','.join(self.branchPrefixes), details["change"]))
>>          if len(details['options']) > 0:
>> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
>> index f9ae1d7..3907560 100644
>> --- a/t/lib-git-p4.sh
>> +++ b/t/lib-git-p4.sh
>> @@ -160,6 +160,16 @@ p4_add_user() {
>>       EOF
>>  }
>>
>> +p4_add_job() {
>
> Not a new problem in this script, but we'd prefer to spell this as
>
>     p4_add_job () {
>
> i.e. a space on both sides of ().
>
>> +     name=$1 &&
>> +     p4 job -f -i <<-EOF
>> +     Job: $name
>> +     Status: open
>> +     User: dummy
>> +     Description:
>> +     EOF
>> +}
>
> It may be better without $name?
>
>> +test_expect_success 'check log message of changelist with no jobs' '
>> +     client_view "//depot/... //client/..." &&
>> +     test_when_finished cleanup_git &&
>> +     (
>> +             cd "$git" &&
>> +             git init . &&
>> +             git p4 clone --use-client-spec --destination="$git" //depot@all &&
>> +             cat >expect <<-\EOF &&
>> +Add file 1
>> +[git-p4: depot-paths = "//depot/": change = 1]
>> +
>> +             EOF
>
> As you are using <<- to begin the here document, it is easier on the
> eyes if you indented the text with HT, i.e.
>
>                 cat >expect <<-\EOF &&
>                 Add file 1
>                 [git-p4: depot-paths = "//depot/": change = 1]
>
>                 EOF
>
> I won't repeat the same for other instances below.
>
> Thanks.