Re: [PATCH v2] git-p4: add P4 jobs to git commit message
On 20 April 2016 at 16:51, Junio C Hamano wrote: > Luke Diamand 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@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
> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@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 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@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
On 19 April 2016 at 22:39, Junio C Hamano wrote: > Jan Durovec writes: > >> On Tue, Apr 19, 2016 at 11:09 PM, Junio C Hamano 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@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
On 19 April 2016 at 22:44, Jan Durovec 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@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
> 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 unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@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
Jan Durovec writes: > On Tue, Apr 19, 2016 at 11:09 PM, Junio C Hamano 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@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
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 wrote: > Jan Durovec writes: > >> On Tue, Apr 19, 2016 at 8:50 PM, Jan Durovec 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@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
Jan Durovec writes: > On Tue, Apr 19, 2016 at 8:50 PM, Jan Durovec 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@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
Huh... seems that it works :) v3 sent in 2 parts On Tue, Apr 19, 2016 at 8:50 PM, Jan Durovec 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@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
> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@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
Jan Durovec writes: > On Tue, Apr 19, 2016 at 7:47 PM, Junio C Hamano 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@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
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 wrote: > Jan Durovec 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. > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@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
Junio C Hamano 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@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
Jan Durovec 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@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
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 wrote: > Luke Diamand 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@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 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@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
On 19 April 2016 at 02:15, Junio C Hamano wrote: > Jan Durovec 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 >> --- > > 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 <> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@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
On 16 Apr 2016, at 21:58, Jan Durovec 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 > --- > 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 < 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 000..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-spe
Re: [PATCH v2] git-p4: add P4 jobs to git commit message
Jan Durovec 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 > --- 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 < 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] git-p4: add P4 jobs to git commit message
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 --- 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 < 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 000..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 TESTJ