PATCH: git-p4 optional handling of RCS keywords [was: Re: git-p4 and keyword expansion]

PATCH: git-p4 optional handling of RCS keywords [was: Re: git-p4 and keyword expansion]

[dhruva]

Hello,
 I would like to submit my first patch to the git community.

I have introduced a new configuration option to 'git-p4' "kwstrip". If enabled, the RCS keywords gets unexpanded like it is done with out the patch and disabling it explicitly retains the RCS keywords as in the original p4 source. The default (in the absence) is 'false' to ensure backward compatibility. To override, you can put the following lines in your '.gitconfig' file..

[git-p4]
        kwstrip = false



The patch to git-p4 (based on origin/next branch):

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 2216cac..ad37d0b 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -16,6 +16,9 @@ from sets import Set;

 verbose = False

+# Handling of RCS keyowrds. To ensure backward compatibility, the default
+# is to strip keywords. Default behavior is controlled here
+kwstrip = True

 def p4_build_cmd(cmd):
     """Build a suitable p4 command line.
@@ -975,10 +978,11 @@ class P4Sync(Command):
                 sys.stderr.write("p4 print fails with: %s\n" % repr(stat))
                 continue

-            if stat['type'] in ('text+ko', 'unicode+ko', 'binary+ko'):
-                text = re.sub(r'(?i)\$(Id|Header):[^$]*\$',r'$\1$', text)
-            elif stat['type'] in ('text+k', 'ktext', 'kxtext', 'unicode+k', 'binary+k'):
-                text = re.sub(r'\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$]*\$',r'$\1$', text)
+            if kwstrip:
+                if stat['type'] in ('text+ko', 'unicode+ko', 'binary+ko'):
+                    text = re.sub(r'(?i)\$(Id|Header):[^$]*\$',r'$\1$', text)
+                elif stat['type'] in ('text+k', 'ktext', 'kxtext', 'unicode+k', 'binary+k'):
+                    text = re.sub(r'\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$]*\$',r'$\1$', text)

             contents[stat['depotFile']] = text

@@ -1850,6 +1854,16 @@ def main():
         (cmd, args) = parser.parse_args(sys..argv[2:], cmd);
     global verbose
     verbose = cmd.verbose
+
+    global kwstrip
+    kwval = gitConfig("git-p4.kwstrip")
+    if len(kwval) > 0:
+       kwval = kwval.lower();
+        if "false" == kwval:
+            kwstrip = False
+        else if "true" == kwval:
+            kwstrip = True
+
     if cmd.needsGit:
         if cmd.gitdir == None:
             cmd.gitdir = os.path.abspath(".git")



----- Original Message ----
> From: Jing Xue <jingxue@digizenstudio.com>
> To: dhruva <dhruva@ymail.com>
> Cc: GIT SCM <git@vger.kernel.org>
> Sent: Tuesday, 9 September, 2008 11:08:48 PM
> Subject: Re: git-p4 and keyword expansion
> 
> On Tue, Sep 09, 2008 at 04:44:11PM +0530, dhruva wrote:
> > Hello,
> 
> Hi,
> 
> > I feel the configuration must be set the first time only, when you
> > clone using 'git-p4 clone'. Altering it in between will be very
> > confusing!
> > Ideally, the setting must be transferred when the git repo
> > (cloned from git-p4) is cloned using standard git. Is it something
> > possible (well, I am new to git and am exploring. Any extra
> > information would help).
> >
> > My proposal is as follows:
> > 1. Add an extra command line argument to 'git-p4 clone' to either  
> > enable/disable keyword expansion
> > 2. Store that information under the .git folder in a file that is  
> > copied when someone clones that repo
> > 3. Use the stored information in future 'git-p4 sync/rebase'
> 
> Any way to make it optional would be welcome by me.

Done


> If you do come up with a "formal" patch, you might want to
> explicitly add Simon Hausmann to the To list, for he's the git-p4
> author.

Done

-dhruva



      Add more friends to your messenger and enjoy! Go to http://in.messenger.yahoo.com/invite/

Re: PATCH: git-p4 optional handling of RCS keywords

[Junio C Hamano]

dhruva <dhruva@ymail.com> writes:

> @@ -975,10 +978,11 @@ class P4Sync(Command):
>                  sys.stderr.write("p4 print fails with: %s\n" % repr(stat))
>                  continue
>
> -            if stat['type'] in ('text+ko', 'unicode+ko', 'binary+ko'):
> -                text = re.sub(r'(?i)\$(Id|Header):[^$]*\$',r'$\1$', text)
> -            elif stat['type'] in ('text+k', 'ktext', 'kxtext', 'unicode+k', 'binary+k'):
> -                text = re.sub(r'\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$]*\$',r'$\1$', text)
> +            if kwstrip:
> +                if stat['type'] in ('text+ko', 'unicode+ko', 'binary+ko'):
> +                    text = re.sub(r'(?i)\$(Id|Header):[^$]*\$',r'$\1$', text)
> +                elif stat['type'] in ('text+k', 'ktext', 'kxtext', 'unicode+k', 'binary+k'):
> +                    text = re.sub(r'\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$]*\$',r'$\1$', text)

A style tip.

It makes it easier to convince others that you didn't screw up in the
conversion if you cascade the code this way instead:

-	if stat['type'] in ('text+ko', 'unicode+ko', 'binary+ko'):
+	if not kwstrip:
+		pass
+	elif stat['type'] in ('text+ko', 'unicode+ko', 'binary+ko'):
 		text = re.sub(r'(?i)\$(Id|Header):[^$]*\$',r'$\1$', text)
 	elif stat['type'] in ('text+k', 'ktext', 'kxtext', 'unicode+k', 'binary+k'):
 		text = re.sub(r'\$(Id|Header|Author|Date|DateTime|Change|...

This technique not just only makes the patch smaller and easier to review,
it also makes the result less deeply nested and easier to read as well.