{"thread":{"id":"15528","subject":"Re: [PATCH] Optional shrinking of RCS keywords in git-p4","startedAt":"2008-09-15T06:26:22Z","lastAt":"2008-09-16T17:32:08Z","messageCount":9,"participants":["dhruva","David Brown","Junio C Hamano","Tor Arvid Lund","Daniel Barkalow","Jing Xue"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"90711","messageId":"16219.81556.qm@web95005.mail.in2.yahoo.com","threadId":"15528","inReplyTo":null,"subject":"Re: [PATCH] Optional shrinking of RCS keywords in git-p4","fromName":"dhruva","fromEmail":"dhruva@ymail.com","sentAt":"2008-09-15T06:26:22Z","receivedAt":"2008-09-15T06:26:22Z","isPatch":true,"sender":{"key":"dhruva@ymail.com","avatar":null},"body":"Hi,\n\n If you have p4 files with 'ktext' enabled, it will expand RCS keywords. Here is how it goes wrong.\n\n1. Clone from p4 with files of 'ktext' type\n2. git-p4, converts \"$Id:........\" to \"$Id$\"\n3. So, the file in p4 and git are different as RCS keywords are modified\n4. You locally edit and commit into git a p4 file of type 'ktext'\n5. The change history in your local git commit will not have any hunks to track the RCS keyword as they are not modified locally\n6. Someone edits the same file on p4 and submits\n7. you do a git rebase (which pulls in the new modifications and strips the RCS keyword change, p4 submit would have incremented the $Id:....$)\n8. The git diffs is now not aware of the change in RCS keyword\n9. You try to submit your local changes back to p4\n10. Applying your local changes as patch sets will fail with missing hunks tracking RCS keyword changes\n\nI 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. \n\n-dhruva\n\nPS: 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.\n\n\n----- Original Message ----\n> From: David Brown \n> To: Dhruva Krishnamurthy \n> Cc: GIT SCM ; Junio C Hamano \n> Sent: Monday, 15 September, 2008 11:39:55 AM\n> Subject: Re: [PATCH] Optional shrinking of RCS keywords in git-p4\n> \n> On Mon, Sep 15, 2008 at 11:28:51AM +0530, Dhruva Krishnamurthy wrote:\n> \n> >Modifying RCS keywords prevents submitting to p4 from git due to missing hunks.\n> >New option git-p4.kwstrip set to true or false controls the behavior.\n> \n> I'm a little curious about what the problem here is. I've been\n> stripping keywords out of P4 and submitting changes for many years,\n> and never had a problem.\n> \n> I'm just wondering if we're not fixing the wrong problem here.\n> \n> David\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n\n\n\n Connect with friends all over the world. Get Yahoo! India Messenger at http://in.messenger.yahoo.com/?wm=n/\n"},{"id":"90713","messageId":"20080915063521.GA1533@linode.davidb.org","threadId":"15528","inReplyTo":"16219.81556.qm@web95005.mail.in2.yahoo.com","subject":"Re: [PATCH] Optional shrinking of RCS keywords in git-p4","fromName":"David Brown","fromEmail":"git@davidb.org","sentAt":"2008-09-15T06:35:21Z","receivedAt":"2008-09-15T06:35:21Z","isPatch":true,"sender":{"key":"git@davidb.org","avatar":"https://gravatar.com/avatar/94c86a2938470a74c2eac5e2b69afc0871f79a660295c02219597aba8cb101c1?d=mp&s=160"},"body":"On Mon, Sep 15, 2008 at 11:56:22AM +0530, dhruva wrote:\n\n>8. The git diffs is now not aware of the change in RCS keyword\n>9. You try to submit your local changes back to p4\n>10. Applying your local changes as patch sets will fail with missing hunks tracking RCS keyword changes\n\nIt sounds like you are trying to apply these as patches to a tree\nwhich doesn't have RCS headers. As far as I can tell, P4 completely\nignores whatever the $Id: ...$ headers happen to be expanded to at the\ntime of checking. You can put garbage there, and it check in fine.\n\nI've been checking in files for many years with stripped headers. I\nwrote a python script years ago to strip the P4 headers after Perforce\nwas unwilling to implement this as an option.\n\nI guess it isn't a problem to make this optional in git-p4, but I\ndon't think this patch is solving the right problem.\n\nDavid\n"},{"id":"90719","messageId":"7vy71tetvt.fsf@gitster.siamese.dyndns.org","threadId":"15528","inReplyTo":"20080915063521.GA1533@linode.davidb.org","subject":"Re: [PATCH] Optional shrinking of RCS keywords in git-p4","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-09-15T07:43:50Z","receivedAt":"2008-09-15T07:43:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Brown writes:\n\n> ... As far as I can tell, P4 completely\n> ignores whatever the $Id: ...$ headers happen to be expanded to at the\n> time of checking. You can put garbage there, and it check in fine.\n> ...\n> I guess it isn't a problem to make this optional in git-p4, but I\n> don't think this patch is solving the right problem.\n\nHmm. I do not do p4, but what I am guessing is that there probably is a\nconfiguration switch on the p4 side that lets you check in files with\n\"$Id: garbage $\" in them, while dhruva hasn't turned that switch on.\n\nIt could be (1) not flipping the switch on is a user mistake and dhruva\ncan just flip it to fix his problem, or (2) the policy of dhruva's project\nmandates the switch to stay off, and he needs the patch to work around the\nissue.\n\nI cannot judge which is the case myself, but if the situation is the\nformer, we would need a documentation to suggest that magic p4 switch as a\nworkaround that would work for everybody without hurting anybody. On the\nother hadn, if the situation is the latter, we would need this patch in\naddition to the suggestion of the magic p4 switch that the user _may_ be\nable to flip depending on the project policy on the p4 side.\n"},{"id":"90729","messageId":"1a6be5fa0809150402m6020698ci9204109a0b615c1c@mail.gmail.com","threadId":"15528","inReplyTo":"7vy71tetvt.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Optional shrinking of RCS keywords in git-p4","fromName":"Tor Arvid Lund","fromEmail":"torarvid@gmail.com","sentAt":"2008-09-15T11:02:32Z","receivedAt":"2008-09-15T11:02:32Z","isPatch":true,"sender":{"key":"torarvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/439758?v=4"},"body":"On Mon, Sep 15, 2008 at 9:43 AM, Junio C Hamano wrote:\n> David Brown writes:\n>\n>> ... As far as I can tell, P4 completely\n>> ignores whatever the $Id: ...$ headers happen to be expanded to at the\n>> time of checking. You can put garbage there, and it check in fine.\n>> ...\n>> I guess it isn't a problem to make this optional in git-p4, but I\n>> don't think this patch is solving the right problem.\n>\n> Hmm. I do not do p4, but what I am guessing is that there probably is a\n> configuration switch on the p4 side that lets you check in files with\n> \"$Id: garbage $\" in them, while dhruva hasn't turned that switch on.\n\nHmm.. I thought this was not a p4 problem. I think however, that\n\"git-p4 submit\" tries to do git format-patch and then git apply that\npatch to the p4 directory. In other words, I believe that git apply\nfails since the file in the p4 dir has the keywords expanded, while\nthe patch does not. I haven't done any careful investigation, but If\nmy assumption is true, it sounds like dhruvas patch should work...\n\n-Tor Arvid Lund-\n"},{"id":"90760","messageId":"alpine.LNX.1.00.0809151354040.19665@iabervon.org","threadId":"15528","inReplyTo":"7vy71tetvt.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Optional shrinking of RCS keywords in git-p4","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-09-15T19:22:33Z","receivedAt":"2008-09-15T19:22:33Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Mon, 15 Sep 2008, Junio C Hamano wrote:\n\n> David Brown writes:\n> \n> > ... As far as I can tell, P4 completely\n> > ignores whatever the $Id: ...$ headers happen to be expanded to at the\n> > time of checking. You can put garbage there, and it check in fine.\n> > ...\n> > I guess it isn't a problem to make this optional in git-p4, but I\n> > don't think this patch is solving the right problem.\n> \n> Hmm. I do not do p4, but what I am guessing is that there probably is a\n> configuration switch on the p4 side that lets you check in files with\n> \"$Id: garbage $\" in them, while dhruva hasn't turned that switch on.\n\nActually, the problem seems to be that git-p4 tries to create the modified \nfile by applying the git-generated diff to the p4-provided file, and this \nfails if the context for the git-generated diff contains a keyword, since \nthe p4-provided file has it expanded and git has it collapsed.\n\nI think the right solution is for git-p4 to check that p4 thinks the file \nis the correct file and then simply replace it rather than trying to \ngenerate the right result by patching. To be a bit more careful, git-p4 \ncould check that the contents it's replacing actually would exactly match \nthe git contents if the keywords were callapsed (if the p4 setting is to \nuse keywords in this file).\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"90798","messageId":"20080916041201.GA25033@linode.davidb.org","threadId":"15528","inReplyTo":"alpine.LNX.1.00.0809151354040.19665@iabervon.org","subject":"Re: [PATCH] Optional shrinking of RCS keywords in git-p4","fromName":"David Brown","fromEmail":"git@davidb.org","sentAt":"2008-09-16T04:12:01Z","receivedAt":"2008-09-16T04:12:01Z","isPatch":true,"sender":{"key":"git@davidb.org","avatar":"https://gravatar.com/avatar/94c86a2938470a74c2eac5e2b69afc0871f79a660295c02219597aba8cb101c1?d=mp&s=160"},"body":"On Mon, Sep 15, 2008 at 03:22:33PM -0400, Daniel Barkalow wrote:\n\n>Actually, the problem seems to be that git-p4 tries to create the modified \n>file by applying the git-generated diff to the p4-provided file, and this \n>fails if the context for the git-generated diff contains a keyword, since \n>the p4-provided file has it expanded and git has it collapsed.\n\nIt is very likely that I've never made a change within context-lines\nof a RCS header. The files we have with these headers tend to also\ncomment blocks at the top that don't change after the file is created.\n\n>I think the right solution is for git-p4 to check that p4 thinks the file \n>is the correct file and then simply replace it rather than trying to \n>generate the right result by patching. To be a bit more careful, git-p4 \n>could check that the contents it's replacing actually would exactly match \n>the git contents if the keywords were callapsed (if the p4 setting is to \n>use keywords in this file).\n\nPart of the problem is that p4 isn't very good at knowing whether\nfiles have changed or not. 'p4 sync' will update the file _if_ if\nthinks your version is out of date, but it does nothing if someone has\nlocally modified the file, hence the need for the 'p4 sync -f'.\n\nA simple way to be paranoid would be something (shell-ish) like:\n\n p4 print filename | collapse-keywords | git hash-object --stdin\n\nand make sure that is the version we think the file should have\nstarted with. I think we're really just making sure we didn't miss a\nP4 change that someone else made underneath, and we're about to back\nout.\n\nEven this isn't robust from p4's point of view. The p4 model is to do\na 'p4 edit' on the file, and then the later 'p4 submit' will give an\nerror if someone else has updated the file. This would require using\np4's conflict resolution, and I'm guessing someone using git-p4 would\nrather abort the submit and rebase.\n\nDavid\n"},{"id":"90829","messageId":"20080916125856.GB3069@jabba.hq.digizenstudio.com","threadId":"15528","inReplyTo":"20080916041201.GA25033@linode.davidb.org","subject":"Re: [PATCH] Optional shrinking of RCS keywords in git-p4","fromName":"Jing Xue","fromEmail":"jingxue@digizenstudio.com","sentAt":"2008-09-16T12:58:56Z","receivedAt":"2008-09-16T12:58:56Z","isPatch":true,"sender":{"key":"jingxue@digizenstudio.com","avatar":null},"body":"On Mon, Sep 15, 2008 at 09:12:01PM -0700, David Brown wrote:\n> A simple way to be paranoid would be something (shell-ish) like:\n>\n> p4 print filename | collapse-keywords | git hash-object --stdin\n>\n> and make sure that is the version we think the file should have\n> started with. I think we're really just making sure we didn't miss a\n> P4 change that someone else made underneath, and we're about to back\n> out.\n> Even this isn't robust from p4's point of view. The p4 model is to do\n> a 'p4 edit' on the file, and then the later 'p4 submit' will give an\n> error if someone else has updated the file. This would require using\n> p4's conflict resolution, and I'm guessing someone using git-p4 would\n> rather abort the submit and rebase.\n\nHow about collapsing the keywords in the _p4_ version after \"p4 edit\"\nbut before applying the patch, and just \"p4 submit\" the collapsed\nversion if patching succeeds? As pointed out earlier in this thread, p4\nsubmit doesn't care about whether keywords are expanded or not anyway.\n\nCheers.\n-- \nJing Xue\n"},{"id":"90849","messageId":"alpine.LNX.1.00.0809161211440.19665@iabervon.org","threadId":"15528","inReplyTo":"20080916041201.GA25033@linode.davidb.org","subject":"Re: [PATCH] Optional shrinking of RCS keywords in git-p4","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-09-16T17:12:31Z","receivedAt":"2008-09-16T17:12:31Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Mon, 15 Sep 2008, David Brown wrote:\n\n> On Mon, Sep 15, 2008 at 03:22:33PM -0400, Daniel Barkalow wrote:\n>\n> >I think the right solution is for git-p4 to check that p4 thinks the file is\n> >the correct file and then simply replace it rather than trying to generate\n> >the right result by patching. To be a bit more careful, git-p4 could check\n> >that the contents it's replacing actually would exactly match the git\n> >contents if the keywords were callapsed (if the p4 setting is to use keywords\n> >in this file).\n> \n> Part of the problem is that p4 isn't very good at knowing whether\n> files have changed or not. 'p4 sync' will update the file _if_ if\n> thinks your version is out of date, but it does nothing if someone has\n> locally modified the file, hence the need for the 'p4 sync -f'.\n\nI think losing those changes are what we're trying to be careful to avoid. \nWhat matter for making the submission correctly is that p4 think that your \nversion is the version you want to replace, and that the file contents are \nwhat you want it to end up with.\n\n> A simple way to be paranoid would be something (shell-ish) like:\n> \n> p4 print filename | collapse-keywords | git hash-object --stdin\n> \n> and make sure that is the version we think the file should have\n> started with. I think we're really just making sure we didn't miss a\n> P4 change that someone else made underneath, and we're about to back\n> out.\n\np4 keeps track of which revision of each file you have synced to in your \nclient (so that it can fail to update it sometimes, as you mention above), \nand will complain if the synced-to version isn't the latest when you try \nto submit. That's how it avoids having people accidentally back out each \nother's changes in ordinary operation. As long as we can be sure that the \nclient hasn't been synced to a later version than what the parent of the \ncommit we're submitting is an import of, which should be done with \"p4 \nsync \", rather than trying to spot check for having \naccidentally acknowledged more p4 history than we've accounted for.\n\n> Even this isn't robust from p4's point of view. The p4 model is to do\n> a 'p4 edit' on the file, and then the later 'p4 submit' will give an\n> error if someone else has updated the file. This would require using\n> p4's conflict resolution, and I'm guessing someone using git-p4 would\n> rather abort the submit and rebase.\n\np4 doesn't let you submit files that you don't do a \"p4 edit\" on (or an \nequivalent like add), so we can't help but do it correctly (assuming that \nwe haven't synced to a later version that the parent, of course). If you \nget an error on the submit, you just revert everything you editted, rebase \nthe git side, and try again (sync to the new parent of the git commit, \nedit the files, replace with the git content, and submit).\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"90851","messageId":"alpine.LNX.1.00.0809161326380.19665@iabervon.org","threadId":"15528","inReplyTo":"alpine.LNX.1.00.0809161211440.19665@iabervon.org","subject":"Re: [PATCH] Optional shrinking of RCS keywords in git-p4","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-09-16T17:32:08Z","receivedAt":"2008-09-16T17:32:08Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Tue, 16 Sep 2008, Daniel Barkalow wrote:\n\n> p4 keeps track of which revision of each file you have synced to in your \n> client (so that it can fail to update it sometimes, as you mention above), \n> and will complain if the synced-to version isn't the latest when you try \n> to submit. That's how it avoids having people accidentally back out each \n> other's changes in ordinary operation. As long as we can be sure that the \n> client hasn't been synced to a later version than what the parent of the \n> commit we're submitting is an import of, which should be done with \"p4 \n> sync \", rather than trying to spot check for having \n> accidentally acknowledged more p4 history than we've accounted for.\n\nThat is, \"p4 sync @\", of course.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"}]}