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

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

[Jan Durovec]

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]
+            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

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

[Junio C Hamano]

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

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?

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

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

[Lars Schneider]

On 16 Apr 2016, at 21:58, Jan Durovec <jan.durovec@gmail.com> wrote:

> 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?


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


>         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

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

[Luke Diamand]

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

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

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

[Junio C Hamano]

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.

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

[Jan Durovec]

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:
> 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.

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

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

[Jan Durovec]

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:
> 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.
>

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

[Junio C Hamano]

Jan Durovec <jan.durovec@gmail.com> writes:

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

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

[Jan Durovec]

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

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

[Jan Durovec]

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:
>> 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.

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

[Junio C Hamano]

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.

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

[Jan Durovec]

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:
> 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.

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

[Junio C Hamano]

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.

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.

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

[Jan Durovec]

> 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

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

[Luke Diamand]

On 19 April 2016 at 22:44, Jan Durovec <jan.durovec@gmail.com> wrote:
>> 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

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

[Luke Diamand]

On 19 April 2016 at 22:39, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Junio C Hamano]

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.

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

[Jan Durovec]

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

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

[Luke Diamand]

On 20 April 2016 at 16:51, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

> 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