Re: [PATCH v2] git-p4: add P4 jobs to git commit message
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 19, 2016, 01:15 UTC
- Message-ID
- <xmqqshyi2yb7.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <0102015420a6c30a-f2da55c9-1fc4-4df6-860e-228c5305f617-000000@eu-west-1.amazonses.com>
Jan Durovec <jan.durovec@gmail.com> writes:
Show 12 quoted lines
> 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?
Show 21 quoted lines
> 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 + 1I 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).
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?
Show 31 quoted lines
> + 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 ().
Show 8 quoted lines
> + name=$1 && > + p4 job -f -i <<-EOF > + Job: $name > + Status: open > + User: dummy > + Description: > + EOF > +}
It may be better without $name?
Show 12 quoted lines
> +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.