# Re: [PATCH] Optional shrinking of RCS keywords in git-p4 9 messages from 2008-09-15 to 2008-09-16. Participants: dhruva, David Brown, Junio C Hamano, Tor Arvid Lund, Daniel Barkalow, Jing Xue. Thread: https://gitlist.dev/t/15528 ## dhruva, 2008-09-15 06:26 Subject: Re: [PATCH] Optional shrinking of RCS keywords in git-p4 Message-ID: <16219.81556.qm@web95005.mail.in2.yahoo.com> URL: https://gitlist.dev/e/16219.81556.qm%40web95005.mail.in2.yahoo.com ``` Hi, If you have p4 files with 'ktext' enabled, it will expand RCS keywords. Here is how it goes wrong. 1. Clone from p4 with files of 'ktext' type 2. git-p4, converts "$Id:........" to "$Id$" 3. So, the file in p4 and git are different as RCS keywords are modified 4. You locally edit and commit into git a p4 file of type 'ktext' 5. The change history in your local git commit will not have any hunks to track the RCS keyword as they are not modified locally 6. Someone edits the same file on p4 and submits 7. you do a git rebase (which pulls in the new modifications and strips the RCS keyword change, p4 submit would have incremented the $Id:....$) 8. The git diffs is now not aware of the change in RCS keyword 9. You try to submit your local changes back to p4 10. Applying your local changes as patch sets will fail with missing hunks tracking RCS keyword changes I have personally experienced more often and hence decided to dig into git-p4 and fix it. All C/C++ source code is created in our p4 repo as 'ktext' and I keep stumbling on this very often. Ideally, if they were just 'text' type in p4, I would never have seen this problem. -dhruva PS: Simon Hausmann, I missed adding you in CC of the patch as my .gitconfig was still under stablizing. I apologize for that. I have finally set up my ..gitconfig with 'git-p4' identity to add you in loop when I submit. ----- Original Message ---- > From: David Brown > To: Dhruva Krishnamurthy > Cc: GIT SCM ; Junio C Hamano > Sent: Monday, 15 September, 2008 11:39:55 AM > Subject: Re: [PATCH] Optional shrinking of RCS keywords in git-p4 > > On Mon, Sep 15, 2008 at 11:28:51AM +0530, Dhruva Krishnamurthy wrote: > > >Modifying RCS keywords prevents submitting to p4 from git due to missing hunks. > >New option git-p4.kwstrip set to true or false controls the behavior. > > I'm a little curious about what the problem here is. I've been > stripping keywords out of P4 and submitting changes for many years, > and never had a problem. > > I'm just wondering if we're not fixing the wrong problem here. > > David > -- > 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 Connect with friends all over the world. Get Yahoo! India Messenger at http://in.messenger.yahoo.com/?wm=n/ ``` ## David Brown, 2008-09-15 06:35 Subject: Re: [PATCH] Optional shrinking of RCS keywords in git-p4 Message-ID: <20080915063521.GA1533@linode.davidb.org> URL: https://gitlist.dev/e/20080915063521.GA1533%40linode.davidb.org In-Reply-To: <16219.81556.qm@web95005.mail.in2.yahoo.com> ``` On Mon, Sep 15, 2008 at 11:56:22AM +0530, dhruva wrote: >8. The git diffs is now not aware of the change in RCS keyword >9. You try to submit your local changes back to p4 >10. Applying your local changes as patch sets will fail with missing hunks tracking RCS keyword changes It sounds like you are trying to apply these as patches to a tree which doesn't have RCS headers. As far as I can tell, P4 completely ignores whatever the $Id: ...$ headers happen to be expanded to at the time of checking. You can put garbage there, and it check in fine. I've been checking in files for many years with stripped headers. I wrote a python script years ago to strip the P4 headers after Perforce was unwilling to implement this as an option. I guess it isn't a problem to make this optional in git-p4, but I don't think this patch is solving the right problem. David ``` ## Junio C Hamano, 2008-09-15 07:43 Subject: Re: [PATCH] Optional shrinking of RCS keywords in git-p4 Message-ID: <7vy71tetvt.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vy71tetvt.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20080915063521.GA1533@linode.davidb.org> ``` David Brown writes: > ... As far as I can tell, P4 completely > ignores whatever the $Id: ...$ headers happen to be expanded to at the > time of checking. You can put garbage there, and it check in fine. > ... > I guess it isn't a problem to make this optional in git-p4, but I > don't think this patch is solving the right problem. Hmm. I do not do p4, but what I am guessing is that there probably is a configuration switch on the p4 side that lets you check in files with "$Id: garbage $" in them, while dhruva hasn't turned that switch on. It could be (1) not flipping the switch on is a user mistake and dhruva can just flip it to fix his problem, or (2) the policy of dhruva's project mandates the switch to stay off, and he needs the patch to work around the issue. I cannot judge which is the case myself, but if the situation is the former, we would need a documentation to suggest that magic p4 switch as a workaround that would work for everybody without hurting anybody. On the other hadn, if the situation is the latter, we would need this patch in addition to the suggestion of the magic p4 switch that the user _may_ be able to flip depending on the project policy on the p4 side. ``` ## Tor Arvid Lund, 2008-09-15 11:02 Subject: Re: [PATCH] Optional shrinking of RCS keywords in git-p4 Message-ID: <1a6be5fa0809150402m6020698ci9204109a0b615c1c@mail.gmail.com> URL: https://gitlist.dev/e/1a6be5fa0809150402m6020698ci9204109a0b615c1c%40mail.gmail.com In-Reply-To: <7vy71tetvt.fsf@gitster.siamese.dyndns.org> ``` On Mon, Sep 15, 2008 at 9:43 AM, Junio C Hamano wrote: > David Brown writes: > >> ... As far as I can tell, P4 completely >> ignores whatever the $Id: ...$ headers happen to be expanded to at the >> time of checking. You can put garbage there, and it check in fine. >> ... >> I guess it isn't a problem to make this optional in git-p4, but I >> don't think this patch is solving the right problem. > > Hmm. I do not do p4, but what I am guessing is that there probably is a > configuration switch on the p4 side that lets you check in files with > "$Id: garbage $" in them, while dhruva hasn't turned that switch on. Hmm.. I thought this was not a p4 problem. I think however, that "git-p4 submit" tries to do git format-patch and then git apply that patch to the p4 directory. In other words, I believe that git apply fails since the file in the p4 dir has the keywords expanded, while the patch does not. I haven't done any careful investigation, but If my assumption is true, it sounds like dhruvas patch should work... -Tor Arvid Lund- ``` ## Daniel Barkalow, 2008-09-15 19:22 Subject: Re: [PATCH] Optional shrinking of RCS keywords in git-p4 Message-ID: URL: https://gitlist.dev/e/alpine.LNX.1.00.0809151354040.19665%40iabervon.org In-Reply-To: <7vy71tetvt.fsf@gitster.siamese.dyndns.org> ``` On Mon, 15 Sep 2008, Junio C Hamano wrote: > David Brown writes: > > > ... As far as I can tell, P4 completely > > ignores whatever the $Id: ...$ headers happen to be expanded to at the > > time of checking. You can put garbage there, and it check in fine. > > ... > > I guess it isn't a problem to make this optional in git-p4, but I > > don't think this patch is solving the right problem. > > Hmm. I do not do p4, but what I am guessing is that there probably is a > configuration switch on the p4 side that lets you check in files with > "$Id: garbage $" in them, while dhruva hasn't turned that switch on. Actually, the problem seems to be that git-p4 tries to create the modified file by applying the git-generated diff to the p4-provided file, and this fails if the context for the git-generated diff contains a keyword, since the p4-provided file has it expanded and git has it collapsed. I think the right solution is for git-p4 to check that p4 thinks the file is the correct file and then simply replace it rather than trying to generate the right result by patching. To be a bit more careful, git-p4 could check that the contents it's replacing actually would exactly match the git contents if the keywords were callapsed (if the p4 setting is to use keywords in this file). -Daniel *This .sig left intentionally blank* ``` ## David Brown, 2008-09-16 04:12 Subject: Re: [PATCH] Optional shrinking of RCS keywords in git-p4 Message-ID: <20080916041201.GA25033@linode.davidb.org> URL: https://gitlist.dev/e/20080916041201.GA25033%40linode.davidb.org In-Reply-To: ``` On Mon, Sep 15, 2008 at 03:22:33PM -0400, Daniel Barkalow wrote: >Actually, the problem seems to be that git-p4 tries to create the modified >file by applying the git-generated diff to the p4-provided file, and this >fails if the context for the git-generated diff contains a keyword, since >the p4-provided file has it expanded and git has it collapsed. It is very likely that I've never made a change within context-lines of a RCS header. The files we have with these headers tend to also comment blocks at the top that don't change after the file is created. >I think the right solution is for git-p4 to check that p4 thinks the file >is the correct file and then simply replace it rather than trying to >generate the right result by patching. To be a bit more careful, git-p4 >could check that the contents it's replacing actually would exactly match >the git contents if the keywords were callapsed (if the p4 setting is to >use keywords in this file). Part of the problem is that p4 isn't very good at knowing whether files have changed or not. 'p4 sync' will update the file _if_ if thinks your version is out of date, but it does nothing if someone has locally modified the file, hence the need for the 'p4 sync -f'. A simple way to be paranoid would be something (shell-ish) like: p4 print filename | collapse-keywords | git hash-object --stdin and make sure that is the version we think the file should have started with. I think we're really just making sure we didn't miss a P4 change that someone else made underneath, and we're about to back out. Even this isn't robust from p4's point of view. The p4 model is to do a 'p4 edit' on the file, and then the later 'p4 submit' will give an error if someone else has updated the file. This would require using p4's conflict resolution, and I'm guessing someone using git-p4 would rather abort the submit and rebase. David ``` ## Jing Xue, 2008-09-16 12:58 Subject: Re: [PATCH] Optional shrinking of RCS keywords in git-p4 Message-ID: <20080916125856.GB3069@jabba.hq.digizenstudio.com> URL: https://gitlist.dev/e/20080916125856.GB3069%40jabba.hq.digizenstudio.com In-Reply-To: <20080916041201.GA25033@linode.davidb.org> ``` On Mon, Sep 15, 2008 at 09:12:01PM -0700, David Brown wrote: > A simple way to be paranoid would be something (shell-ish) like: > > p4 print filename | collapse-keywords | git hash-object --stdin > > and make sure that is the version we think the file should have > started with. I think we're really just making sure we didn't miss a > P4 change that someone else made underneath, and we're about to back > out. > Even this isn't robust from p4's point of view. The p4 model is to do > a 'p4 edit' on the file, and then the later 'p4 submit' will give an > error if someone else has updated the file. This would require using > p4's conflict resolution, and I'm guessing someone using git-p4 would > rather abort the submit and rebase. How about collapsing the keywords in the _p4_ version after "p4 edit" but before applying the patch, and just "p4 submit" the collapsed version if patching succeeds? As pointed out earlier in this thread, p4 submit doesn't care about whether keywords are expanded or not anyway. Cheers. -- Jing Xue ``` ## Daniel Barkalow, 2008-09-16 17:12 Subject: Re: [PATCH] Optional shrinking of RCS keywords in git-p4 Message-ID: URL: https://gitlist.dev/e/alpine.LNX.1.00.0809161211440.19665%40iabervon.org In-Reply-To: <20080916041201.GA25033@linode.davidb.org> ``` On Mon, 15 Sep 2008, David Brown wrote: > On Mon, Sep 15, 2008 at 03:22:33PM -0400, Daniel Barkalow wrote: > > >I think the right solution is for git-p4 to check that p4 thinks the file is > >the correct file and then simply replace it rather than trying to generate > >the right result by patching. To be a bit more careful, git-p4 could check > >that the contents it's replacing actually would exactly match the git > >contents if the keywords were callapsed (if the p4 setting is to use keywords > >in this file). > > Part of the problem is that p4 isn't very good at knowing whether > files have changed or not. 'p4 sync' will update the file _if_ if > thinks your version is out of date, but it does nothing if someone has > locally modified the file, hence the need for the 'p4 sync -f'. I think losing those changes are what we're trying to be careful to avoid. What matter for making the submission correctly is that p4 think that your version is the version you want to replace, and that the file contents are what you want it to end up with. > A simple way to be paranoid would be something (shell-ish) like: > > p4 print filename | collapse-keywords | git hash-object --stdin > > and make sure that is the version we think the file should have > started with. I think we're really just making sure we didn't miss a > P4 change that someone else made underneath, and we're about to back > out. p4 keeps track of which revision of each file you have synced to in your client (so that it can fail to update it sometimes, as you mention above), and will complain if the synced-to version isn't the latest when you try to submit. That's how it avoids having people accidentally back out each other's changes in ordinary operation. As long as we can be sure that the client hasn't been synced to a later version than what the parent of the commit we're submitting is an import of, which should be done with "p4 sync ", rather than trying to spot check for having accidentally acknowledged more p4 history than we've accounted for. > Even this isn't robust from p4's point of view. The p4 model is to do > a 'p4 edit' on the file, and then the later 'p4 submit' will give an > error if someone else has updated the file. This would require using > p4's conflict resolution, and I'm guessing someone using git-p4 would > rather abort the submit and rebase. p4 doesn't let you submit files that you don't do a "p4 edit" on (or an equivalent like add), so we can't help but do it correctly (assuming that we haven't synced to a later version that the parent, of course). If you get an error on the submit, you just revert everything you editted, rebase the git side, and try again (sync to the new parent of the git commit, edit the files, replace with the git content, and submit). -Daniel *This .sig left intentionally blank* ``` ## Daniel Barkalow, 2008-09-16 17:32 Subject: Re: [PATCH] Optional shrinking of RCS keywords in git-p4 Message-ID: URL: https://gitlist.dev/e/alpine.LNX.1.00.0809161326380.19665%40iabervon.org In-Reply-To: ``` On Tue, 16 Sep 2008, Daniel Barkalow wrote: > p4 keeps track of which revision of each file you have synced to in your > client (so that it can fail to update it sometimes, as you mention above), > and will complain if the synced-to version isn't the latest when you try > to submit. That's how it avoids having people accidentally back out each > other's changes in ordinary operation. As long as we can be sure that the > client hasn't been synced to a later version than what the parent of the > commit we're submitting is an import of, which should be done with "p4 > sync ", rather than trying to spot check for having > accidentally acknowledged more p4 history than we've accounted for. That is, "p4 sync @", of course. -Daniel *This .sig left intentionally blank* ```