Re: [PATCH] Adding list of p4 jobs to git commit message

[Junio C Hamano <gitster@pobox.com>]

Jan Durovec <jan.durovec@gmail.com> 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