threads / patch / 42049

v2git-p4: add P4 jobs to git commit message

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

## tl;dr

21 messages between Apr 16, 2016 and Apr 21, 2016. Diffs are folded; open one to read it.

replies: 20people: 4as markdown or json

Jan Durovec· Apr 16, 2016, 19:58 UTC · lore

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>
---
 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
Show changes to 3 files +121 −1

git-p4.py, t/lib-git-p4.sh, 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
+        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() {
+	name=$1 &&
+	p4 job -f -i <<-EOF
+	Job: $name
+	Status: open
+	User: dummy
+	Description:
+	EOF
+}
+
 retry_until_success() {
 	timeout=$(($(time_in_seconds) + $RETRY_TIMEOUT))
 	until "$@" 2>/dev/null || test $(time_in_seconds) -gt $timeout
diff --git a/t/t9829-git-p4-jobs.sh b/t/t9829-git-p4-jobs.sh
new file mode 100755
index 0000000..2b9c98c
--- /dev/null
+++ b/t/t9829-git-p4-jobs.sh
@@ -0,0 +1,99 @@
+#!/bin/sh
+
+test_description='git p4 retrieve job info'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	start_p4d
+'
+
+test_expect_success 'add p4 jobs' '
+	(
+		p4_add_job TESTJOB-A &&
+		p4_add_job TESTJOB-B
+	)
+'
+
+test_expect_success 'add p4 files' '
+	client_view "//depot/... //client/..." &&
+	(
+		cd "$cli" &&
+		>file1 &&
+		p4 add file1 &&
+		p4 submit -d "Add file 1"
+	)
+'
+
+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
+		git log --format=%B >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'add TESTJOB-A to change 1' '
+	(
+		cd "$cli" &&
+		p4 fix -c 1 TESTJOB-A
+	)
+'
+
+test_expect_success 'check log message of changelist with one job' '
+	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
+Jobs: TESTJOB-A
+[git-p4: depot-paths = "//depot/": change = 1]
+
+		EOF
+		git log --format=%B >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'add TESTJOB-B to change 1' '
+	(
+		cd "$cli" &&
+		p4 fix -c 1 TESTJOB-B
+	)
+'
+
+test_expect_success 'check log message of changelist with more 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
+Jobs: TESTJOB-A TESTJOB-B
+[git-p4: depot-paths = "//depot/": change = 1]
+
+		EOF
+		git log --format=%B >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done

--
https://github.com/git/git/pull/225
Junio C Hamano· Apr 19, 2016, 01:15 UTC · re: Jan Durovec · lore

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

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 + 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).

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.
Luke Diamand· Apr 19, 2016, 08:13 UTC · re: Junio C Hamano · lore

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

On 19 April 2016 at 02:15, Junio C Hamano <gitster@pobox.com> wrote:
Show 47 quoted lines
> 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).

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.
Show 75 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 ().
>
>> +     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
Junio C Hamano· Apr 19, 2016, 16:45 UTC · re: Luke Diamand · lore

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

Luke Diamand <luke@diamand.org> writes:
Show 8 quoted lines
>> 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.

Ah, thanks, that is what I was missing and this part of the code makes perfect sense to me now.

Jan Durovec· Apr 19, 2016, 16:56 UTC · re: Junio C Hamano · lore

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

Hi,

given the fact that the rest of the code just follows existing source code style, i.e.

* using %s not %d to add number to string (see git-p4.py:2301)
* no space between function name and parentheses (see all functions
  in t/lib-git-p4.sh)
* no tab when specifying in-line expected output (see t/t9826...)
...is there anything left to fix in this patch or is it good as is?

I.e. would you prefer me to change the code mentioned above at the cost of code style consistency?

Is there something else that I have missed in my enumeration?

Regards, Jan

On Tue, Apr 19, 2016 at 6:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 13 quoted lines
> Luke Diamand <luke@diamand.org> writes:
>
>>> 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.
>
> Ah, thanks, that is what I was missing and this part of the code
> makes perfect sense to me now.
Junio C Hamano· Apr 19, 2016, 17:47 UTC · re: Jan Durovec · lore

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

Jan Durovec <jan.durovec@gmail.com> writes:
> given the fact that the rest of the code just follows existing
> source code style, i.e.
>
> * using %s not %d to add number to string (see git-p4.py:2301)

This one I do not care too deeply about, as formatting anything that can be formatted via '%s' could just be more Pythonic style, in which case "%s" is perfectly fine. It just didn't look kosher to my C trained eyes, that's all.

> * no space between function name and parentheses (see all functions
>   in t/lib-git-p4.sh)

I thought I said "Not a new issue, but..." to this one, and it appears that leaving <<- here-doc unindented, which is stupid as that shows the person who is writing the here-doc does not know what the dash s/he is writing means at all, is also not a new issue.

> * no tab when specifying in-line expected output (see t/t9826...)
> ...is there anything left to fix in this patch or is it good as is?
>
> I.e. would you prefer me to change the code mentioned above at the cost
> of code style consistency?
Not really.

If you really want to know the preference, we prefer a preliminary clean-up patch to correct existing style issues, followed by a new feature patch that builds on the cleaned up codebase.

Jan Durovec· Apr 19, 2016, 18:04 UTC · re: Junio C Hamano · lore

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

Would it be acceptable the other way around? I.e. this patch followed by the one that fixes code style (once this gets merged)?

Reason being that I don't know how to use submitGit to generate a patch against a state that is not already in git repo (ie. based on another patch).

In the following patch I'll
* add spaces before () for functions in t/lib-git-p4.sh
* remove name local variable in p4_add_job/user in t/lib-git-p4.sh
* fix t/t98* leading tabs where <<- is used
On Tue, Apr 19, 2016 at 7:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 33 quoted lines
> Jan Durovec <jan.durovec@gmail.com> writes:
>
>> given the fact that the rest of the code just follows existing
>> source code style, i.e.
>>
>> * using %s not %d to add number to string (see git-p4.py:2301)
>
> This one I do not care too deeply about, as formatting anything that
> can be formatted via '%s' could just be more Pythonic style, in
> which case "%s" is perfectly fine.  It just didn't look kosher to my
> C trained eyes, that's all.
>
>> * no space between function name and parentheses (see all functions
>>   in t/lib-git-p4.sh)
>
> I thought I said "Not a new issue, but..." to this one, and it
> appears that leaving <<- here-doc unindented, which is stupid as
> that shows the person who is writing the here-doc does not know what
> the dash s/he is writing means at all, is also not a new issue.
>
>> * no tab when specifying in-line expected output (see t/t9826...)
>
>> ...is there anything left to fix in this patch or is it good as is?
>>
>> I.e. would you prefer me to change the code mentioned above at the cost
>> of code style consistency?
>
> Not really.
>
> If you really want to know the preference, we prefer a preliminary
> clean-up patch to correct existing style issues, followed by a new
> feature patch that builds on the cleaned up codebase.
>
Junio C Hamano· Apr 19, 2016, 18:43 UTC · re: Jan Durovec · lore

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

Jan Durovec <jan.durovec@gmail.com> writes:
Show 8 quoted lines
> On Tue, Apr 19, 2016 at 7:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> If you really want to know the preference, we prefer a preliminary
>> clean-up patch to correct existing style issues, followed by a new
>> feature patch that builds on the cleaned up codebase.
>
> Would it be acceptable the other way around? I.e. this patch followed
> by the one that fixes code style (once this gets merged)?

The reason I said "preference" and not "requirement" is because the answer to that question is "It depends."

When the primary change is something small and urgent (e.g. an important bugfix that needs to be merged down to 'maint' and older), we typically do not want to keep the fix waiting for preliminary clean-up patch to be reviewed. Instead, we'd say "let's fix it first on top of a known-to-be-crappy codebase, and postpone the clean-up until the dust settles".

>From experience, we already know that sometimes clean-up happens,

but often it does not, when we take this route, and it harms the long-term code health, but we just say an urgent fix is more important than piling a bit more cruft on the existing technical debt.

And that experience is why we say "preference is to have preliminary clean-up first".

> Reason being that I don't know how to use submitGit to generate a patch
> against a state that is not already in git repo (ie. based on another
> patch).

Any submitGit users? I think it lets you throw multiple-patch series just fine. In this case, you'd prepare a two patch series on a branch, 1/2 being the clean-up and 2/2 being the new feature, and if you give that branch to submitGit as a whole it should do the right thing, I'd imagine.

Jan Durovec· Apr 19, 2016, 18:50 UTC · re: Junio C Hamano · lore

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

Show 5 quoted lines
> Any submitGit users?  I think it lets you throw multiple-patch
> series just fine.  In this case, you'd prepare a two patch series on
> a branch, 1/2 being the clean-up and 2/2 being the new feature, and
> if you give that branch to submitGit as a whole it should do the
> right thing, I'd imagine.
Hm... I'll see what it does with a pull request that has 2 commits.
Jan Durovec· Apr 19, 2016, 20:28 UTC · re: Jan Durovec · lore

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

Huh... seems that it works :)
v3 sent in 2 parts
On Tue, Apr 19, 2016 at 8:50 PM, Jan Durovec <jan.durovec@gmail.com> wrote:
Show 7 quoted lines
>> Any submitGit users?  I think it lets you throw multiple-patch
>> series just fine.  In this case, you'd prepare a two patch series on
>> a branch, 1/2 being the clean-up and 2/2 being the new feature, and
>> if you give that branch to submitGit as a whole it should do the
>> right thing, I'd imagine.
>
> Hm... I'll see what it does with a pull request that has 2 commits.
Junio C Hamano· Apr 19, 2016, 21:09 UTC · re: Jan Durovec · lore

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

Jan Durovec <jan.durovec@gmail.com> writes:
Show 12 quoted lines
> On Tue, Apr 19, 2016 at 8:50 PM, Jan Durovec <jan.durovec@gmail.com> wrote:
>>> Any submitGit users?  I think it lets you throw multiple-patch
>>> series just fine.  In this case, you'd prepare a two patch series on
>>> a branch, 1/2 being the clean-up and 2/2 being the new feature, and
>>> if you give that branch to submitGit as a whole it should do the
>>> right thing, I'd imagine.
>>
>> Hm... I'll see what it does with a pull request that has 2 commits.
>
> Huh... seems that it works :)
>
> v3 sent in 2 parts
Thanks.

For a series this small it does not matter, but anything longer it would be easier to review with a cover letter (i.e. [PATCH 0/N]). I do not know if submitGit lets us do that, though.

Jan Durovec· Apr 19, 2016, 21:25 UTC · re: Junio C Hamano · lore

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

There's a comment on PR itself (in addition to individual commits) so theoretically it could.

It seems that for [PATCH ... n/m] e-mails the commit messages are used, so there's no reason why the PR comment couldn't be used for a cover letter.

In this case the PR comment was the same as for one of the commits. Maybe submitGit tried to be smart there? Or maybe it just really doesn't support it (yet?).

On Tue, Apr 19, 2016 at 11:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 20 quoted lines
> Jan Durovec <jan.durovec@gmail.com> writes:
>
>> On Tue, Apr 19, 2016 at 8:50 PM, Jan Durovec <jan.durovec@gmail.com> wrote:
>>>> Any submitGit users?  I think it lets you throw multiple-patch
>>>> series just fine.  In this case, you'd prepare a two patch series on
>>>> a branch, 1/2 being the clean-up and 2/2 being the new feature, and
>>>> if you give that branch to submitGit as a whole it should do the
>>>> right thing, I'd imagine.
>>>
>>> Hm... I'll see what it does with a pull request that has 2 commits.
>>
>> Huh... seems that it works :)
>>
>> v3 sent in 2 parts
>
> Thanks.
>
> For a series this small it does not matter, but anything longer it
> would be easier to review with a cover letter (i.e. [PATCH 0/N]).  I
> do not know if submitGit lets us do that, though.
Junio C Hamano· Apr 19, 2016, 21:39 UTC · re: Jan Durovec · lore

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

Jan Durovec <jan.durovec@gmail.com> writes:
Show 16 quoted lines
> On Tue, Apr 19, 2016 at 11:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> For a series this small it does not matter, but anything longer it
>> would be easier to review with a cover letter (i.e. [PATCH 0/N]).  I
>> do not know if submitGit lets us do that, though.
>
> There's a comment on PR itself (in addition to individual commits) so
> theoretically it could.
>
> It seems that for [PATCH ... n/m] e-mails the commit messages are used,
> so there's no reason why the PR comment couldn't be used for a cover
> letter.
>
> In this case the PR comment was the same as for one of the commits.
> Maybe submitGit tried to be smart there? Or maybe it just really
> doesn't support it (yet?).

In any case, I've queued the updated ones as they looked good. Thanks.

By the way, you may or may not have noticed that I've been reordering the lines of your message quoted in my responses; around here, top-posting is frowned upon.

Jan Durovec· Apr 19, 2016, 21:44 UTC · re: Junio C Hamano · lore

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

> By the way, you may or may not have noticed that I've been
> reordering the lines of your message quoted in my responses; around
> here, top-posting is frowned upon.
I haven't noticed. Thanks for pointing out.

As for the submitGit cover letter I wanted to raise at least an issue (if not create a fix itself) but it seems to be raised already as https://github.com/rtyley/submitgit/issues/9

Luke Diamand· Apr 20, 2016, 07:56 UTC · re: Jan Durovec · lore

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

On 19 April 2016 at 22:44, Jan Durovec <jan.durovec@gmail.com> wrote:
Show 9 quoted lines
>> By the way, you may or may not have noticed that I've been
>> reordering the lines of your message quoted in my responses; around
>> here, top-posting is frowned upon.
>
> I haven't noticed. Thanks for pointing out.
>
> As for the submitGit cover letter I wanted to raise at least an issue
> (if not create a fix itself) but it seems to be raised already as
> https://github.com/rtyley/submitgit/issues/9

To be honest, it would be just as easy to learn how to use git format-patch and git send-email. It makes your head hurt a bit the first few times you use it, but it's pretty straightforward.

And it also means that if there's a zombie apocalypse, and github gets overrun, you can still exchange patches to your anti-virus and save humanity from extinction.

Of course, if you don't care about the future of the human race, then that's up to you.... :-)

Luke
Luke Diamand· Apr 20, 2016, 07:58 UTC · re: Junio C Hamano · lore

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

On 19 April 2016 at 22:39, Junio C Hamano <gitster@pobox.com> wrote:
Show 21 quoted lines
> Jan Durovec <jan.durovec@gmail.com> writes:
>
>> On Tue, Apr 19, 2016 at 11:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> For a series this small it does not matter, but anything longer it
>>> would be easier to review with a cover letter (i.e. [PATCH 0/N]).  I
>>> do not know if submitGit lets us do that, though.
>>
>> There's a comment on PR itself (in addition to individual commits) so
>> theoretically it could.
>>
>> It seems that for [PATCH ... n/m] e-mails the commit messages are used,
>> so there's no reason why the PR comment couldn't be used for a cover
>> letter.
>>
>> In this case the PR comment was the same as for one of the commits.
>> Maybe submitGit tried to be smart there? Or maybe it just really
>> doesn't support it (yet?).
>
> In any case, I've queued the updated ones as they looked good.
> Thanks.

One thing I wondered about is whether this should be enabled by default or not. Long-time users of git-p4 might be a bit surprised to find their git commits suddenly gaining an extra Job: field.

Luke
Junio C Hamano· Apr 20, 2016, 15:51 UTC · re: Luke Diamand · lore

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

Luke Diamand <luke@diamand.org> writes:
> One thing I wondered about is whether this should be enabled by
> default or not. Long-time users of git-p4 might be a bit surprised to
> find their git commits suddenly gaining an extra Job: field.

Ahh, I didn't even wonder about but that is not because I didn't think it matters.

Does this change affect reproducibility of importing the history from P4, doesn't it? Would that be a problem?

How common is it to have the "extra" Job: thing in the history on P4 side? If the answer to this question is "on rare occasions and only when there is a very good reason to have 'jobs' associated with the changelist", then the 'might be a bit surprised' brought by this change can probably be explained away as "a fix to a (design) bug that used to discard crucial information" that (unfortunately) have to change the resulting Git object names.

Luke Diamand· Apr 21, 2016, 07:34 UTC · re: Junio C Hamano · lore

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

On 20 April 2016 at 16:51, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> Luke Diamand <luke@diamand.org> writes:
>
>> One thing I wondered about is whether this should be enabled by
>> default or not. Long-time users of git-p4 might be a bit surprised to
>> find their git commits suddenly gaining an extra Job: field.
>
> Ahh, I didn't even wonder about but that is not because I didn't
> think it matters.
>
> Does this change affect reproducibility of importing the history
> from P4, doesn't it?  Would that be a problem?

It would change the history created, but I don't see why that would be a problem.

>
> How common is it to have the "extra" Job: thing in the history on P4
> side?

Where I work currently we don't use jobs (at present). Where I worked before, jobs were created automatically to track issues in JIRA, and were (supposed to be) entered into commits. It's potentially quite useful so I guess might be quite widespread.

Show 7 quoted lines
> If the answer to this question is "on rare occasions and only
> when there is a very good reason to have 'jobs' associated with the
> changelist", then the 'might be a bit surprised' brought by this
> change can probably be explained away as "a fix to a (design) bug
> that used to discard crucial information" that (unfortunately) have
> to change the resulting Git object names.
>
Luke
Jan Durovec· Apr 20, 2016, 16:30 UTC · re: Luke Diamand · lore

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

> One thing I wondered about is whether this should be enabled by
> default or not. Long-time users of git-p4 might be a bit surprised to
> find their git commits suddenly gaining an extra Job: field.

I thought about that too when but then I realized that there's no switch for the reverse direction either. I.e. when committing to p4 from git the commit messages are parsed and "Jobs: ..." section is interpreted regardless of any switch, isn't it?

That's why I decided to keep this behaviour symmetrical. Otherwise I would have reused the same switch that enables/disables job parsing in git->p4 direction.

Junio C Hamano· Apr 19, 2016, 17:52 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 16 quoted lines
> 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?

Just so that I won't get misunderstood, with this I do not mean "Job: $name" line does not have to be there. I meant that there is no need to use name variable in this function; just writing $1 instead of $name there is better, as $name is not a function local variable in POSIX shells.

Lars Schneider· Apr 19, 2016, 07:34 UTC · re: Jan Durovec · lore

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

On 16 Apr 2016, at 21:58, Jan Durovec <jan.durovec@gmail.com> wrote:
Show 31 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>
> ---
> 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]

I tried to use the new format string syntax in the past. See: https://pyformat.info/ https://docs.python.org/2/library/string.html#format-string-syntax

'job{}'.format(jnum)
@Luke: What do you prefer going forward?
Show 21 quoted lines
> +            jobs.append(job)
> +            jnum = jnum + 1
> +        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)))

You could use the new string syntax here, too. How long is an avg job string? Could it make sense to write them into individual lines? Would it makes sense to add an extra new line after the commit message? I assume not because your commit messages says "same format as is expected when migrating"...

Show 136 quoted lines
>         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() {
> +	name=$1 &&
> +	p4 job -f -i <<-EOF
> +	Job: $name
> +	Status: open
> +	User: dummy
> +	Description:
> +	EOF
> +}
> +
> retry_until_success() {
> 	timeout=$(($(time_in_seconds) + $RETRY_TIMEOUT))
> 	until "$@" 2>/dev/null || test $(time_in_seconds) -gt $timeout
> diff --git a/t/t9829-git-p4-jobs.sh b/t/t9829-git-p4-jobs.sh
> new file mode 100755
> index 0000000..2b9c98c
> --- /dev/null
> +++ b/t/t9829-git-p4-jobs.sh
> @@ -0,0 +1,99 @@
> +#!/bin/sh
> +
> +test_description='git p4 retrieve job info'
> +
> +. ./lib-git-p4.sh
> +
> +test_expect_success 'start p4d' '
> +	start_p4d
> +'
> +
> +test_expect_success 'add p4 jobs' '
> +	(
> +		p4_add_job TESTJOB-A &&
> +		p4_add_job TESTJOB-B
> +	)
> +'
> +
> +test_expect_success 'add p4 files' '
> +	client_view "//depot/... //client/..." &&
> +	(
> +		cd "$cli" &&
> +		>file1 &&
> +		p4 add file1 &&
> +		p4 submit -d "Add file 1"
> +	)
> +'
> +
> +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
> +		git log --format=%B >actual &&
> +		test_cmp expect actual
> +	)
> +'
> +
> +test_expect_success 'add TESTJOB-A to change 1' '
> +	(
> +		cd "$cli" &&
> +		p4 fix -c 1 TESTJOB-A
> +	)
> +'
> +
> +test_expect_success 'check log message of changelist with one job' '
> +	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
> +Jobs: TESTJOB-A
> +[git-p4: depot-paths = "//depot/": change = 1]
> +
> +		EOF
> +		git log --format=%B >actual &&
> +		test_cmp expect actual
> +	)
> +'
> +
> +test_expect_success 'add TESTJOB-B to change 1' '
> +	(
> +		cd "$cli" &&
> +		p4 fix -c 1 TESTJOB-B
> +	)
> +'
> +
> +test_expect_success 'check log message of changelist with more 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
> +Jobs: TESTJOB-A TESTJOB-B
> +[git-p4: depot-paths = "//depot/": change = 1]
> +
> +		EOF
> +		git log --format=%B >actual &&
> +		test_cmp expect actual
> +	)
> +'
> +
> +test_expect_success 'kill p4d' '
> +	kill_p4d
> +'
> +
> +test_done
> 
> --
> https://github.com/git/git/pull/225
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

← back to recent threads