On 19 April 2016 at 02:15, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Jan Durovec <jan.durovec@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>
> 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).
This is OK - P4 jobs have arbitrary names, but this code is just
extracting an array of them from the commit by index.
>
> Shouldn't the formatting be "job%d" % jnum, though, as you are using
> jnum as a number that begins at 0 and increments by 1?
Python seems to handle this by turning jnum into a string.
>
>> + 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.
Modulo Junio's other comments, this seems fine to me. I tried it out
on a scratch repo and it works very nicely, thanks!
Luke
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html