# [PATCH] Adding list of p4 jobs to git commit message 5 messages from 2016-04-15 to 2016-04-16. Participants: Jan Durovec, Junio C Hamano, Luke Diamand, Lars Schneider. Thread: https://gitlist.dev/t/42039 ## Jan Durovec, 2016-04-15 19:51 Subject: [PATCH] Adding list of p4 jobs to git commit message Message-ID: <010201541b7a24b8-83b0f526-2f01-40e3-afc8-d81549af70d4-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/010201541b7a24b8-83b0f526-2f01-40e3-afc8-d81549af70d4-000000%40eu-west-1.amazonses.com ``` --- git-p4.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/git-p4.py b/git-p4.py index 527d44b..a81795f 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)) @@ -2696,6 +2706,8 @@ def commit(self, details, files, branch, parent = ""): (','.join(self.branchPrefixes), details["change"])) if len(details['options']) > 0: self.gitStream.write(": options = %s" % details['options']) + if len(jobs) > 0: + self.gitStream.write(": jobs = %s" % (','.join(jobs))) self.gitStream.write("]\nEOT\n\n") if len(parent) > 0: -- https://github.com/git/git/pull/225 ``` ## Junio C Hamano, 2016-04-15 20:27 Subject: Re: [PATCH] Adding list of p4 jobs to git commit message Message-ID: URL: https://gitlist.dev/e/xmqq4mb2bot7.fsf%40gitster.mtv.corp.google.com In-Reply-To: <010201541b7a24b8-83b0f526-2f01-40e3-afc8-d81549af70d4-000000@eu-west-1.amazonses.com> ``` Jan Durovec writes: > --- A few issues. Please: (1) Sign-off your work. (2) Try to find those who are familiar with the area and Cc them. "git shortlog -s -n --since=18.months --no-merges git-p4.py" may help. (3) Follow the style of existing commits when giving a title to your patch. "git shortlog --since=18.months --no-merges git-p4.py" may help you notice "git-p4: do this thing" is the common way to title "git p4" patches. (4) Justify why your change is a good thing in your log message. What you did, i.e. "list p4 jobs when making a commit", can be seen by the patch, but readers cannot guess why you thought it is a good idea to extract "job%d" out of the P4 commit and to record them in the resulting Git commit, unless you explain things like: - what goes wrong if you don't? - when would "job%d" appear in P4 commit? - is it sane to assume "job0", "job1",... appear consecutively? (5) Describe what your change does clearly. "Adding list" is not quite clear. Where in the "git commit message" are you adding the list, and why is that location in the message the most appropriate place to add it? Thanks. > git-p4.py | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/git-p4.py b/git-p4.py > index 527d44b..a81795f 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)) > @@ -2696,6 +2706,8 @@ def commit(self, details, files, branch, parent = ""): > (','.join(self.branchPrefixes), details["change"])) > if len(details['options']) > 0: > self.gitStream.write(": options = %s" % details['options']) > + if len(jobs) > 0: > + self.gitStream.write(": jobs = %s" % (','.join(jobs))) > self.gitStream.write("]\nEOT\n\n") > > if len(parent) > 0: > > -- > https://github.com/git/git/pull/225 ``` ## Luke Diamand, 2016-04-15 22:10 Subject: Re: [PATCH] Adding list of p4 jobs to git commit message Message-ID: URL: https://gitlist.dev/e/CAE5ih79hV-76SxjW0NHjD%2BWsvO1s6rthM%3Dpvqyrh10zbrFi2rQ%40mail.gmail.com In-Reply-To: ``` On 15 April 2016 at 21:27, Junio C Hamano wrote: > Jan Durovec writes: > >> --- > > A few issues. Please: > > (1) Sign-off your work. > > (2) Try to find those who are familiar with the area and Cc them. > > "git shortlog -s -n --since=18.months --no-merges git-p4.py" > may help. > > (3) Follow the style of existing commits when giving a title to > your patch. > > "git shortlog --since=18.months --no-merges git-p4.py" may > help you notice "git-p4: do this thing" is the common way to > title "git p4" patches. > > (4) Justify why your change is a good thing in your log message. > What you did, i.e. "list p4 jobs when making a commit", can be > seen by the patch, but readers cannot guess why you thought it > is a good idea to extract "job%d" out of the P4 commit and to > record them in the resulting Git commit, unless you explain > things like: > > - what goes wrong if you don't? > - when would "job%d" appear in P4 commit? > - is it sane to assume "job0", "job1",... appear consecutively? > > (5) Describe what your change does clearly. "Adding list" is not > quite clear. Where in the "git commit message" are you adding > the list, and why is that location in the message the most > appropriate place to add it? Additionally, it would be very useful to add a test case (see t/t98*). There are quite a few test cases for git-p4, and they make maintenance a lot easier. Some documentation (Documentation/git-p4.txt) would also be useful. Thanks Luke ``` ## Jan Durovec, 2016-04-16 13:46 Subject: Re: [PATCH] Adding list of p4 jobs to git commit message Message-ID: URL: https://gitlist.dev/e/CABEqOBwcb1BAmvcBLow38BnQoxRNVYtfXzGAodUv_qwEBWRFvQ%40mail.gmail.com In-Reply-To: ``` Hi, I tried to address all the issues and (among other things) I've added a new test case to t/t98 group (t9829-git-p4-jobs.sh). Unfortunately, Travis CI build now fails with "non-executable tests: t9829-git-p4-jobs.sh" (https://travis-ci.org/git/git/jobs/123555944) Can you please advise how to mark it as executable? Does it need to be added to some configuration file? Or am I interpreting the error message incorrectly? Regards, Jan On Sat, Apr 16, 2016 at 12:10 AM, Luke Diamand wrote: > On 15 April 2016 at 21:27, Junio C Hamano wrote: >> Jan Durovec writes: >> >>> --- >> >> A few issues. Please: >> >> (1) Sign-off your work. >> >> (2) Try to find those who are familiar with the area and Cc them. >> >> "git shortlog -s -n --since=18.months --no-merges git-p4.py" >> may help. >> >> (3) Follow the style of existing commits when giving a title to >> your patch. >> >> "git shortlog --since=18.months --no-merges git-p4.py" may >> help you notice "git-p4: do this thing" is the common way to >> title "git p4" patches. >> >> (4) Justify why your change is a good thing in your log message. >> What you did, i.e. "list p4 jobs when making a commit", can be >> seen by the patch, but readers cannot guess why you thought it >> is a good idea to extract "job%d" out of the P4 commit and to >> record them in the resulting Git commit, unless you explain >> things like: >> >> - what goes wrong if you don't? >> - when would "job%d" appear in P4 commit? >> - is it sane to assume "job0", "job1",... appear consecutively? >> >> (5) Describe what your change does clearly. "Adding list" is not >> quite clear. Where in the "git commit message" are you adding >> the list, and why is that location in the message the most >> appropriate place to add it? > > Additionally, it would be very useful to add a test case (see t/t98*). > There are quite a few test cases for git-p4, and they make maintenance > a lot easier. > > Some documentation (Documentation/git-p4.txt) would also be useful. > > Thanks > Luke ``` ## Lars Schneider, 2016-04-16 14:51 Subject: Re: [PATCH] Adding list of p4 jobs to git commit message Message-ID: <8D2BE57C-BE68-4014-ADED-3382DF164ECA@gmail.com> URL: https://gitlist.dev/e/8D2BE57C-BE68-4014-ADED-3382DF164ECA%40gmail.com In-Reply-To: ``` Hi Jan, Please look closely at the Travis CI output: non-executable tests: t9829-git-p4-jobs.sh make[1]: *** [test-lint-executable] Error 1 You haven't set the execution bit for your new test script. Did you try to run your tests locally? If yes, then I wonder how it worked... On Linux/Mac you need to do the following: chmod u+x t9829-git-p4-jobs.sh Afterwards it should work. Cheers, Lars On 16 Apr 2016, at 15:46, Jan Durovec wrote: > Hi, > > I tried to address all the issues and (among other things) I've added > a new test case to t/t98 group (t9829-git-p4-jobs.sh). > > Unfortunately, Travis CI build now fails with "non-executable tests: > t9829-git-p4-jobs.sh" (https://travis-ci.org/git/git/jobs/123555944) > > Can you please advise how to mark it as executable? Does it need to be > added to some configuration file? Or am I interpreting the error > message incorrectly? > > Regards, > Jan > > On Sat, Apr 16, 2016 at 12:10 AM, Luke Diamand wrote: >> On 15 April 2016 at 21:27, Junio C Hamano wrote: >>> Jan Durovec writes: >>> >>>> --- >>> >>> A few issues. Please: >>> >>> (1) Sign-off your work. >>> >>> (2) Try to find those who are familiar with the area and Cc them. >>> >>> "git shortlog -s -n --since=18.months --no-merges git-p4.py" >>> may help. >>> >>> (3) Follow the style of existing commits when giving a title to >>> your patch. >>> >>> "git shortlog --since=18.months --no-merges git-p4.py" may >>> help you notice "git-p4: do this thing" is the common way to >>> title "git p4" patches. >>> >>> (4) Justify why your change is a good thing in your log message. >>> What you did, i.e. "list p4 jobs when making a commit", can be >>> seen by the patch, but readers cannot guess why you thought it >>> is a good idea to extract "job%d" out of the P4 commit and to >>> record them in the resulting Git commit, unless you explain >>> things like: >>> >>> - what goes wrong if you don't? >>> - when would "job%d" appear in P4 commit? >>> - is it sane to assume "job0", "job1",... appear consecutively? >>> >>> (5) Describe what your change does clearly. "Adding list" is not >>> quite clear. Where in the "git commit message" are you adding >>> the list, and why is that location in the message the most >>> appropriate place to add it? >> >> Additionally, it would be very useful to add a test case (see t/t98*). >> There are quite a few test cases for git-p4, and they make maintenance >> a lot easier. >> >> Some documentation (Documentation/git-p4.txt) would also be useful. >> >> Thanks >> Luke > -- > 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 ```