Jan Durovec <jan.durovec@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html