threads / patch / 40590

patchgit-p4: Add option to ignore empty commits

Subject: [PATCH v1] git-p4: Add option to ignore empty commits

## tl;dr

8 messages between Oct 19, 2015 and Oct 28, 2015. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

larsxschneider@gmail.com· Oct 19, 2015, 18:43 UTC · lore
From: Lars Schneider <larsxschneider@gmail.com>

A changelist that contains only excluded files (e.g. via client spec or branch prefix) will be imported as empty commit. Add option "git-p4.ignoreEmptyCommits" to ignore these commits.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 Documentation/git-p4.txt               |   5 ++
 git-p4.py                              |  41 ++++++++-----
 t/t9826-git-p4-ignore-empty-commits.sh | 103 +++++++++++++++++++++++++++++++++
 3 files changed, 133 insertions(+), 16 deletions(-)
 create mode 100755 t/t9826-git-p4-ignore-empty-commits.sh
Show changes to 3 files +133 −16

Documentation/git-p4.txt, git-p4.py, t/t9826-git-p4-ignore-empty-commits.sh

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 82aa5d6..f096a6a 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -510,6 +510,11 @@ git-p4.useClientSpec::
 	option '--use-client-spec'.  See the "CLIENT SPEC" section above.
 	This variable is a boolean, not the name of a p4 client.
 
+git-p4.ignoreEmptyCommits::
+	A changelist that contains only excluded files will be imported
+	as empty commit. To ignore these commits set this boolean option
+	to 'true'.
+
 Submit variables
 ~~~~~~~~~~~~~~~~
 git-p4.detectRenames::
diff --git a/git-p4.py b/git-p4.py
index 0093fa3..6c50c74 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2288,12 +2288,6 @@ class P4Sync(Command, P4UserMap):
         filesToDelete = []
 
         for f in files:
-            # if using a client spec, only add the files that have
-            # a path in the client
-            if self.clientSpecDirs:
-                if self.clientSpecDirs.map_in_client(f['path']) == "":
-                    continue
-
             filesForCommit.append(f)
             if f['action'] in self.delete_actions:
                 filesToDelete.append(f)
@@ -2368,18 +2362,33 @@ class P4Sync(Command, P4UserMap):
         if self.verbose:
             print "commit into %s" % branch
 
-        # start with reading files; if that fails, we should not
-        # create a commit.
-        new_files = []
-        for f in files:
-            if [p for p in self.branchPrefixes if p4PathStartsWith(f['path'], p)]:
-                new_files.append (f)
-            else:
-                sys.stderr.write("Ignoring file outside of prefix: %s\n" % f['path'])
-
         if self.clientSpecDirs:
             self.clientSpecDirs.update_client_spec_path_cache(files)
 
+        def inClientSpec(path):
+            if not self.clientSpecDirs:
+                return True
+            inClientSpec = self.clientSpecDirs.map_in_client(path)
+            if not inClientSpec and self.verbose:
+                print '\n  Ignoring file outside of client spec' % path
+            return inClientSpec
+
+        def hasBranchPrefix(path):
+            if not self.branchPrefixes:
+                return True
+            hasPrefix = [p for p in self.branchPrefixes
+                            if p4PathStartsWith(path, p)]
+            if hasPrefix and self.verbose:
+                print '\n  Ignoring file outside of prefix: %s' % path
+            return hasPrefix
+
+        files = [f for f in files
+            if inClientSpec(f['path']) and hasBranchPrefix(f['path'])]
+
+        if not files and gitConfigBool('git-p4.ignoreEmptyCommits'):
+            print '\n  Ignoring change %s as it would produce an empty commit.'
+            return
+
         self.gitStream.write("commit %s\n" % branch)
 #        gitStream.write("mark :%s\n" % details["change"])
         self.committedChanges.add(int(details["change"]))
@@ -2403,7 +2412,7 @@ class P4Sync(Command, P4UserMap):
                 print "parent %s" % parent
             self.gitStream.write("from %s\n" % parent)
 
-        self.streamP4Files(new_files)
+        self.streamP4Files(files)
         self.gitStream.write("\n")
 
         change = int(details["change"])
diff --git a/t/t9826-git-p4-ignore-empty-commits.sh b/t/t9826-git-p4-ignore-empty-commits.sh
new file mode 100755
index 0000000..5ddccde
--- /dev/null
+++ b/t/t9826-git-p4-ignore-empty-commits.sh
@@ -0,0 +1,103 @@
+#!/bin/sh
+
+test_description='Clone repositories and ignore empty commits'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	start_p4d
+'
+
+test_expect_success 'Create a repo' '
+	client_view "//depot/... //client/..." &&
+	(
+		cd "$cli" &&
+
+		mkdir -p subdir &&
+
+		>subdir/file1.txt &&
+		p4 add subdir/file1.txt &&
+		p4 submit -d "Add file 1" &&
+
+		>file2.txt &&
+		p4 add file2.txt &&
+		p4 submit -d "Add file 2" &&
+
+		>subdir/file3.txt &&
+		p4 add subdir/file3.txt &&
+		p4 submit -d "Add file 3"
+	)
+'
+
+test_expect_success 'Clone repo root path with all history' '
+	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 3
+[git-p4: depot-paths = "//depot/": change = 3]
+
+Add file 2
+[git-p4: depot-paths = "//depot/": change = 2]
+
+Add file 1
+[git-p4: depot-paths = "//depot/": change = 1]
+
+		EOF
+		git log --format=%B >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'Clone repo subdir with all history' '
+	client_view "//depot/subdir/... //client/subdir/..." &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		git init . &&
+		git p4 clone --use-client-spec --destination="$git" //depot@all &&
+		cat >expect <<-\EOF &&
+Add file 3
+[git-p4: depot-paths = "//depot/": change = 3]
+
+Add file 2
+[git-p4: depot-paths = "//depot/": change = 2]
+
+Add file 1
+[git-p4: depot-paths = "//depot/": change = 1]
+
+		EOF
+		git log --format=%B >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'Clone repo subdir with all history but ignore empty commits' '
+	client_view "//depot/subdir/... //client/subdir/..." &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		git init . &&
+		git config git-p4.ignoreEmptyCommits true &&
+		git p4 clone --use-client-spec --destination="$git" //depot@all &&
+		cat >expect <<-\EOF &&
+Add file 3
+[git-p4: depot-paths = "//depot/": change = 3]
+
+Add file 1
+[git-p4: depot-paths = "//depot/": change = 1]
+
+		EOF
+		git log --format=%B >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done
-- 
2.5.1
Junio C Hamano· Oct 20, 2015, 17:27 UTC · re: larsxschneider@gmail.com · lore

Re: [PATCH v1] git-p4: Add option to ignore empty commits

larsxschneider@gmail.com writes:
Show 17 quoted lines
> diff --git a/git-p4.py b/git-p4.py
> index 0093fa3..6c50c74 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -2288,12 +2288,6 @@ class P4Sync(Command, P4UserMap):
>          filesToDelete = []
>  
>          for f in files:
> -            # if using a client spec, only add the files that have
> -            # a path in the client
> -            if self.clientSpecDirs:
> -                if self.clientSpecDirs.map_in_client(f['path']) == "":
> -                    continue
> -
>              filesForCommit.append(f)
>              if f['action'] in self.delete_actions:
>                  filesToDelete.append(f)

Earlier, the paths outside the clientspec were not in filesToDelete (or filesToRead that is below the context here). Now they all go to these arrays, and will hit this loop beyond the context:

        # deleted files...
        for f in filesToDelete:
            self.streamOneP4Deletion(f)

after leaving the above for loop. I cannot quite see where this "stream one deletion" is turned into a no-op for paths outside after this patch gets applied.

Also I have this suspicion that those who do want to use client spec to get a narrowed view into the history would almost always want this "ignore empty" behaviour (I'd even say the current behaviour to leave empty commits by default is a bug). What are the advantages of keeping empty commits? If there aren't many, perhaps git-p4 should by the default skip empties and require p4.keepEmpty configuration to keep them?

Lars Schneider· Oct 24, 2015, 16:28 UTC · re: Junio C Hamano · lore

Re: [PATCH v1] git-p4: Add option to ignore empty commits

On 20 Oct 2015, at 19:27, Junio C Hamano <gitster@pobox.com> wrote:
Show 31 quoted lines
> larsxschneider@gmail.com writes:
> 
>> diff --git a/git-p4.py b/git-p4.py
>> index 0093fa3..6c50c74 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -2288,12 +2288,6 @@ class P4Sync(Command, P4UserMap):
>>         filesToDelete = []
>> 
>>         for f in files:
>> -            # if using a client spec, only add the files that have
>> -            # a path in the client
>> -            if self.clientSpecDirs:
>> -                if self.clientSpecDirs.map_in_client(f['path']) == "":
>> -                    continue
>> -
>>             filesForCommit.append(f)
>>             if f['action'] in self.delete_actions:
>>                 filesToDelete.append(f)
> 
> Earlier, the paths outside the clientspec were not in filesToDelete
> (or filesToRead that is below the context here).  Now they all go to
> these arrays, and will hit this loop beyond the context:
> 
>        # deleted files...
>        for f in filesToDelete:
>            self.streamOneP4Deletion(f)
> 
> after leaving the above for loop.  I cannot quite see where this
> "stream one deletion" is turned into a no-op for paths outside after
> this patch gets applied.
Earlier the client spec filtering happened in "def streamP4Files(self, files)". I moved the code up to the caller of this function into "def commit(...)" which now calls "streamP4Files" with an already filtered file list. Therefore the logic should be exactly the same.
Show 7 quoted lines
> Also I have this suspicion that those who do want to use client spec
> to get a narrowed view into the history would almost always want
> this "ignore empty" behaviour (I'd even say the current behaviour to
> leave empty commits by default is a bug).  What are the advantages
> of keeping empty commits?  If there aren't many, perhaps git-p4
> should by the default skip empties and require p4.keepEmpty
> configuration to keep them?

I agree. @Luke: What option do you prefer? "git-p4.keepEmptyCommits" or "git-p4.ignoreEmptyCommits" ?

Thanks, Lars

Luke Diamand· Oct 24, 2015, 19:07 UTC · re: Lars Schneider · lore

Re: [PATCH v1] git-p4: Add option to ignore empty commits

On 24/10/15 17:28, Lars Schneider wrote:
>
Show 10 quoted lines
>> Also I have this suspicion that those who do want to use client spec
>> to get a narrowed view into the history would almost always want
>> this "ignore empty" behaviour (I'd even say the current behaviour to
>> leave empty commits by default is a bug).  What are the advantages
>> of keeping empty commits?  If there aren't many, perhaps git-p4
>> should by the default skip empties and require p4.keepEmpty
>> configuration to keep them?
>
> I agree.
> @Luke: What option do you prefer? "git-p4.keepEmptyCommits" or "git-p4.ignoreEmptyCommits" ?
keepEmptyCommits.
Luke Diamand· Oct 21, 2015, 06:32 UTC · re: larsxschneider@gmail.com · lore

Re: [PATCH v1] git-p4: Add option to ignore empty commits

On 19/10/15 19:43, larsxschneider@gmail.com wrote:
Show 26 quoted lines
> From: Lars Schneider <larsxschneider@gmail.com>
>
> A changelist that contains only excluded files (e.g. via client spec or
> branch prefix) will be imported as empty commit. Add option
> "git-p4.ignoreEmptyCommits" to ignore these commits.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>   Documentation/git-p4.txt               |   5 ++
>   git-p4.py                              |  41 ++++++++-----
>   t/t9826-git-p4-ignore-empty-commits.sh | 103 +++++++++++++++++++++++++++++++++
>   3 files changed, 133 insertions(+), 16 deletions(-)
>   create mode 100755 t/t9826-git-p4-ignore-empty-commits.sh
>
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index 82aa5d6..f096a6a 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -510,6 +510,11 @@ git-p4.useClientSpec::
>   	option '--use-client-spec'.  See the "CLIENT SPEC" section above.
>   	This variable is a boolean, not the name of a p4 client.
>
> +git-p4.ignoreEmptyCommits::
> +	A changelist that contains only excluded files will be imported
> +	as empty commit. To ignore these commits set this boolean option
> +	to 'true'.
s/as empty/as an empty/
Possibly putting 'true' in quotes could be confusing.
Show 38 quoted lines
> +
>   Submit variables
>   ~~~~~~~~~~~~~~~~
>   git-p4.detectRenames::
> diff --git a/git-p4.py b/git-p4.py
> index 0093fa3..6c50c74 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -2288,12 +2288,6 @@ class P4Sync(Command, P4UserMap):
>           filesToDelete = []
>
>           for f in files:
> -            # if using a client spec, only add the files that have
> -            # a path in the client
> -            if self.clientSpecDirs:
> -                if self.clientSpecDirs.map_in_client(f['path']) == "":
> -                    continue
> -
>               filesForCommit.append(f)
>               if f['action'] in self.delete_actions:
>                   filesToDelete.append(f)
> @@ -2368,18 +2362,33 @@ class P4Sync(Command, P4UserMap):
>           if self.verbose:
>               print "commit into %s" % branch
>
> -        # start with reading files; if that fails, we should not
> -        # create a commit.
> -        new_files = []
> -        for f in files:
> -            if [p for p in self.branchPrefixes if p4PathStartsWith(f['path'], p)]:
> -                new_files.append (f)
> -            else:
> -                sys.stderr.write("Ignoring file outside of prefix: %s\n" % f['path'])
> -
>           if self.clientSpecDirs:
>               self.clientSpecDirs.update_client_spec_path_cache(files)
>
> +        def inClientSpec(path):

This seems to be adding a new function in the middle of an existing function. Is that right?

Show 6 quoted lines
> +            if not self.clientSpecDirs:
> +                return True
> +            inClientSpec = self.clientSpecDirs.map_in_client(path)
> +            if not inClientSpec and self.verbose:
> +                print '\n  Ignoring file outside of client spec' % path
> +            return inClientSpec
Any particular reason for putting a \n at the start of the message?
Also, could you use python3 style print stmnts, print("whatever") ?
Show 16 quoted lines
> +
> +        def hasBranchPrefix(path):
> +            if not self.branchPrefixes:
> +                return True
> +            hasPrefix = [p for p in self.branchPrefixes
> +                            if p4PathStartsWith(path, p)]
> +            if hasPrefix and self.verbose:
> +                print '\n  Ignoring file outside of prefix: %s' % path
> +            return hasPrefix
> +
> +        files = [f for f in files
> +            if inClientSpec(f['path']) and hasBranchPrefix(f['path'])]
> +
> +        if not files and gitConfigBool('git-p4.ignoreEmptyCommits'):
> +            print '\n  Ignoring change %s as it would produce an empty commit.'
> +            return
As with Junio's comment elsewhere, I worry about deletion here.
Show 66 quoted lines
> +
>           self.gitStream.write("commit %s\n" % branch)
>   #        gitStream.write("mark :%s\n" % details["change"])
>           self.committedChanges.add(int(details["change"]))
> @@ -2403,7 +2412,7 @@ class P4Sync(Command, P4UserMap):
>                   print "parent %s" % parent
>               self.gitStream.write("from %s\n" % parent)
>
> -        self.streamP4Files(new_files)
> +        self.streamP4Files(files)
>           self.gitStream.write("\n")
>
>           change = int(details["change"])
> diff --git a/t/t9826-git-p4-ignore-empty-commits.sh b/t/t9826-git-p4-ignore-empty-commits.sh
> new file mode 100755
> index 0000000..5ddccde
> --- /dev/null
> +++ b/t/t9826-git-p4-ignore-empty-commits.sh
> @@ -0,0 +1,103 @@
> +#!/bin/sh
> +
> +test_description='Clone repositories and ignore empty commits'
> +
> +. ./lib-git-p4.sh
> +
> +test_expect_success 'start p4d' '
> +	start_p4d
> +'
> +
> +test_expect_success 'Create a repo' '
> +	client_view "//depot/... //client/..." &&
> +	(
> +		cd "$cli" &&
> +
> +		mkdir -p subdir &&
> +
> +		>subdir/file1.txt &&
> +		p4 add subdir/file1.txt &&
> +		p4 submit -d "Add file 1" &&
> +
> +		>file2.txt &&
> +		p4 add file2.txt &&
> +		p4 submit -d "Add file 2" &&
> +
> +		>subdir/file3.txt &&
> +		p4 add subdir/file3.txt &&
> +		p4 submit -d "Add file 3"
> +	)
> +'
> +
> +test_expect_success 'Clone repo root path with all history' '
> +	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 3
> +[git-p4: depot-paths = "//depot/": change = 3]
> +
> +Add file 2
> +[git-p4: depot-paths = "//depot/": change = 2]
> +
> +Add file 1
> +[git-p4: depot-paths = "//depot/": change = 1]
Could you not just test for existence of these files?

If the format of the magic comments that git-p4 ever changes, this will break.

(There's a patch out there that gets git-p4 to use git notes, so it's not as far-fetched as it sounds).

Show 47 quoted lines
> +
> +		EOF
> +		git log --format=%B >actual &&
> +		test_cmp expect actual
> +	)
> +'
> +
> +test_expect_success 'Clone repo subdir with all history' '
> +	client_view "//depot/subdir/... //client/subdir/..." &&
> +	test_when_finished cleanup_git &&
> +	(
> +		cd "$git" &&
> +		git init . &&
> +		git p4 clone --use-client-spec --destination="$git" //depot@all &&
> +		cat >expect <<-\EOF &&
> +Add file 3
> +[git-p4: depot-paths = "//depot/": change = 3]
> +
> +Add file 2
> +[git-p4: depot-paths = "//depot/": change = 2]
> +
> +Add file 1
> +[git-p4: depot-paths = "//depot/": change = 1]
> +
> +		EOF
> +		git log --format=%B >actual &&
> +		test_cmp expect actual
> +	)
> +'
> +
> +test_expect_success 'Clone repo subdir with all history but ignore empty commits' '
> +	client_view "//depot/subdir/... //client/subdir/..." &&
> +	test_when_finished cleanup_git &&
> +	(
> +		cd "$git" &&
> +		git init . &&
> +		git config git-p4.ignoreEmptyCommits true &&
> +		git p4 clone --use-client-spec --destination="$git" //depot@all &&
> +		cat >expect <<-\EOF &&
> +Add file 3
> +[git-p4: depot-paths = "//depot/": change = 3]
> +
> +Add file 1
> +[git-p4: depot-paths = "//depot/": change = 1]
> +
> +		EOF
> +		git log --format=%B >actual &&
A deletion test would make me feel more comfortable!

Thanks! Luke

Lars Schneider· Oct 24, 2015, 18:08 UTC · re: Luke Diamand · lore

Re: [PATCH v1] git-p4: Add option to ignore empty commits

On 21 Oct 2015, at 08:32, Luke Diamand <luke@diamand.org> wrote:
Show 31 quoted lines
> On 19/10/15 19:43, larsxschneider@gmail.com wrote:
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> A changelist that contains only excluded files (e.g. via client spec or
>> branch prefix) will be imported as empty commit. Add option
>> "git-p4.ignoreEmptyCommits" to ignore these commits.
>> 
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>>  Documentation/git-p4.txt               |   5 ++
>>  git-p4.py                              |  41 ++++++++-----
>>  t/t9826-git-p4-ignore-empty-commits.sh | 103 +++++++++++++++++++++++++++++++++
>>  3 files changed, 133 insertions(+), 16 deletions(-)
>>  create mode 100755 t/t9826-git-p4-ignore-empty-commits.sh
>> 
>> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
>> index 82aa5d6..f096a6a 100644
>> --- a/Documentation/git-p4.txt
>> +++ b/Documentation/git-p4.txt
>> @@ -510,6 +510,11 @@ git-p4.useClientSpec::
>>  	option '--use-client-spec'.  See the "CLIENT SPEC" section above.
>>  	This variable is a boolean, not the name of a p4 client.
>> 
>> +git-p4.ignoreEmptyCommits::
>> +	A changelist that contains only excluded files will be imported
>> +	as empty commit. To ignore these commits set this boolean option
>> +	to 'true'.
> 
> s/as empty/as an empty/
> 
> Possibly putting 'true' in quotes could be confusing.
OK. Will fix.
Show 41 quoted lines
> 
>> +
>>  Submit variables
>>  ~~~~~~~~~~~~~~~~
>>  git-p4.detectRenames::
>> diff --git a/git-p4.py b/git-p4.py
>> index 0093fa3..6c50c74 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -2288,12 +2288,6 @@ class P4Sync(Command, P4UserMap):
>>          filesToDelete = []
>> 
>>          for f in files:
>> -            # if using a client spec, only add the files that have
>> -            # a path in the client
>> -            if self.clientSpecDirs:
>> -                if self.clientSpecDirs.map_in_client(f['path']) == "":
>> -                    continue
>> -
>>              filesForCommit.append(f)
>>              if f['action'] in self.delete_actions:
>>                  filesToDelete.append(f)
>> @@ -2368,18 +2362,33 @@ class P4Sync(Command, P4UserMap):
>>          if self.verbose:
>>              print "commit into %s" % branch
>> 
>> -        # start with reading files; if that fails, we should not
>> -        # create a commit.
>> -        new_files = []
>> -        for f in files:
>> -            if [p for p in self.branchPrefixes if p4PathStartsWith(f['path'], p)]:
>> -                new_files.append (f)
>> -            else:
>> -                sys.stderr.write("Ignoring file outside of prefix: %s\n" % f['path'])
>> -
>>          if self.clientSpecDirs:
>>              self.clientSpecDirs.update_client_spec_path_cache(files)
>> 
>> +        def inClientSpec(path):
> 
> This seems to be adding a new function in the middle of an existing function. Is that right?
That is true. I could move these functions into the P4Sync class if you don't like them here. I added them right there because it is the only place where they are used/useful.
Show 9 quoted lines
> 
>> +            if not self.clientSpecDirs:
>> +                return True
>> +            inClientSpec = self.clientSpecDirs.map_in_client(path)
>> +            if not inClientSpec and self.verbose:
>> +                print '\n  Ignoring file outside of client spec' % path
>> +            return inClientSpec
> 
> Any particular reason for putting a \n at the start of the message?
I did this because "sys.stdout.write("\rImporting revision ..." (line 2724) does not add a newline. However, I agree that this looks stupid. I will remove the "\n" and fix the "Import revision" print. Speaking of that one: this is only printed if "not self.silent". Is there a particular reason to have "self.silent" and "self.verbose"? Should we merge the two? 
> 
> Also, could you use python3 style print stmnts, print("whatever") ?

Sure. How do you prefer the formatting? Using "format" would be true Python 3 style I think: print('Ignoring file outside of client spec: {}'.format(path)) OK?

Show 19 quoted lines
> 
>> +
>> +        def hasBranchPrefix(path):
>> +            if not self.branchPrefixes:
>> +                return True
>> +            hasPrefix = [p for p in self.branchPrefixes
>> +                            if p4PathStartsWith(path, p)]
>> +            if hasPrefix and self.verbose:
>> +                print '\n  Ignoring file outside of prefix: %s' % path
>> +            return hasPrefix
>> +
>> +        files = [f for f in files
>> +            if inClientSpec(f['path']) and hasBranchPrefix(f['path'])]
>> +
>> +        if not files and gitConfigBool('git-p4.ignoreEmptyCommits'):
>> +            print '\n  Ignoring change %s as it would produce an empty commit.'
>> +            return
> 
> As with Junio's comment elsewhere, I worry about deletion here.
I believe this is right. See my answer to Junio.
Show 68 quoted lines
>> +
>>          self.gitStream.write("commit %s\n" % branch)
>>  #        gitStream.write("mark :%s\n" % details["change"])
>>          self.committedChanges.add(int(details["change"]))
>> @@ -2403,7 +2412,7 @@ class P4Sync(Command, P4UserMap):
>>                  print "parent %s" % parent
>>              self.gitStream.write("from %s\n" % parent)
>> 
>> -        self.streamP4Files(new_files)
>> +        self.streamP4Files(files)
>>          self.gitStream.write("\n")
>> 
>>          change = int(details["change"])
>> diff --git a/t/t9826-git-p4-ignore-empty-commits.sh b/t/t9826-git-p4-ignore-empty-commits.sh
>> new file mode 100755
>> index 0000000..5ddccde
>> --- /dev/null
>> +++ b/t/t9826-git-p4-ignore-empty-commits.sh
>> @@ -0,0 +1,103 @@
>> +#!/bin/sh
>> +
>> +test_description='Clone repositories and ignore empty commits'
>> +
>> +. ./lib-git-p4.sh
>> +
>> +test_expect_success 'start p4d' '
>> +	start_p4d
>> +'
>> +
>> +test_expect_success 'Create a repo' '
>> +	client_view "//depot/... //client/..." &&
>> +	(
>> +		cd "$cli" &&
>> +
>> +		mkdir -p subdir &&
>> +
>> +		>subdir/file1.txt &&
>> +		p4 add subdir/file1.txt &&
>> +		p4 submit -d "Add file 1" &&
>> +
>> +		>file2.txt &&
>> +		p4 add file2.txt &&
>> +		p4 submit -d "Add file 2" &&
>> +
>> +		>subdir/file3.txt &&
>> +		p4 add subdir/file3.txt &&
>> +		p4 submit -d "Add file 3"
>> +	)
>> +'
>> +
>> +test_expect_success 'Clone repo root path with all history' '
>> +	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 3
>> +[git-p4: depot-paths = "//depot/": change = 3]
>> +
>> +Add file 2
>> +[git-p4: depot-paths = "//depot/": change = 2]
>> +
>> +Add file 1
>> +[git-p4: depot-paths = "//depot/": change = 1]
> 
> Could you not just test for existence of these files?
Well, I assume the right files are in there as this is covered with other tests. I want to check that these particular commits are mentioned in the logs (including the commit message and the change list number).
> If the format of the magic comments that git-p4 ever changes, this will break.
I understand your reasoning. But how can I check for the correct commit messages, change list number and their order in a efficient different way?
Show 54 quoted lines
> 
> (There's a patch out there that gets git-p4 to use git notes, so it's not as far-fetched as it sounds).
> 
> 
>> +
>> +		EOF
>> +		git log --format=%B >actual &&
>> +		test_cmp expect actual
>> +	)
>> +'
>> +
>> +test_expect_success 'Clone repo subdir with all history' '
>> +	client_view "//depot/subdir/... //client/subdir/..." &&
>> +	test_when_finished cleanup_git &&
>> +	(
>> +		cd "$git" &&
>> +		git init . &&
>> +		git p4 clone --use-client-spec --destination="$git" //depot@all &&
>> +		cat >expect <<-\EOF &&
>> +Add file 3
>> +[git-p4: depot-paths = "//depot/": change = 3]
>> +
>> +Add file 2
>> +[git-p4: depot-paths = "//depot/": change = 2]
>> +
>> +Add file 1
>> +[git-p4: depot-paths = "//depot/": change = 1]
>> +
>> +		EOF
>> +		git log --format=%B >actual &&
>> +		test_cmp expect actual
>> +	)
>> +'
>> +
>> +test_expect_success 'Clone repo subdir with all history but ignore empty commits' '
>> +	client_view "//depot/subdir/... //client/subdir/..." &&
>> +	test_when_finished cleanup_git &&
>> +	(
>> +		cd "$git" &&
>> +		git init . &&
>> +		git config git-p4.ignoreEmptyCommits true &&
>> +		git p4 clone --use-client-spec --destination="$git" //depot@all &&
>> +		cat >expect <<-\EOF &&
>> +Add file 3
>> +[git-p4: depot-paths = "//depot/": change = 3]
>> +
>> +Add file 1
>> +[git-p4: depot-paths = "//depot/": change = 1]
>> +
>> +		EOF
>> +		git log --format=%B >actual &&
> 
> 
> A deletion test would make me feel more comfortable!
Agreed! I will add one!

Thanks, Lars

Luke Diamand· Oct 26, 2015, 20:40 UTC · re: Lars Schneider · lore

Re: [PATCH v1] git-p4: Add option to ignore empty commits

On 24/10/15 19:08, Lars Schneider wrote:
Show 9 quoted lines
>
> On 21 Oct 2015, at 08:32, Luke Diamand <luke@diamand.org> wrote:
>
>> On 19/10/15 19:43, larsxschneider@gmail.com wrote:
>>> From: Lars Schneider <larsxschneider@gmail.com>
>>>
>>
>> This seems to be adding a new function in the middle of an existing function. Is that right?
> That is true. I could move these functions into the P4Sync class if you don't like them here. I added them right there because it is the only place where they are used/useful.
It just seemed a bit confusing, but I'm ok with them here as well.
Show 12 quoted lines
>
>
>>
>>> +            if not self.clientSpecDirs:
>>> +                return True
>>> +            inClientSpec = self.clientSpecDirs.map_in_client(path)
>>> +            if not inClientSpec and self.verbose:
>>> +                print '\n  Ignoring file outside of client spec' % path
>>> +            return inClientSpec
>>
>> Any particular reason for putting a \n at the start of the message?
> I did this because "sys.stdout.write("\rImporting revision ..." (line 2724) does not add a newline. However, I agree that this looks stupid. I will remove the "\n" and fix the "Import revision" print. Speaking of that one: this is only printed if "not self.silent". Is there a particular reason to have "self.silent" and "self.verbose"? Should we merge the two?

self.silent and self.verbose seem like two slightly different things. I would expect silent to prevent any output at all. But actually it doesn't.

If we wanted to implement it properly, I think we'd need a new function (p4print?) which did the obvious right thing. Maybe something for a rainy day in the future.

Show 6 quoted lines
>
>
>>
>> Also, could you use python3 style print stmnts, print("whatever") ?
> Sure. How do you prefer the formatting? Using "format" would be true Python 3 style I think:
> print('Ignoring file outside of client spec: {}'.format(path))

Will that breaker older versions of python? There's a statement somewhere about how far back we support.

Show 97 quoted lines
> OK?
>
>>
>>> +
>>> +        def hasBranchPrefix(path):
>>> +            if not self.branchPrefixes:
>>> +                return True
>>> +            hasPrefix = [p for p in self.branchPrefixes
>>> +                            if p4PathStartsWith(path, p)]
>>> +            if hasPrefix and self.verbose:
>>> +                print '\n  Ignoring file outside of prefix: %s' % path
>>> +            return hasPrefix
>>> +
>>> +        files = [f for f in files
>>> +            if inClientSpec(f['path']) and hasBranchPrefix(f['path'])]
>>> +
>>> +        if not files and gitConfigBool('git-p4.ignoreEmptyCommits'):
>>> +            print '\n  Ignoring change %s as it would produce an empty commit.'
>>> +            return
>>
>> As with Junio's comment elsewhere, I worry about deletion here.
> I believe this is right. See my answer to Junio.
>
>
>>> +
>>>           self.gitStream.write("commit %s\n" % branch)
>>>   #        gitStream.write("mark :%s\n" % details["change"])
>>>           self.committedChanges.add(int(details["change"]))
>>> @@ -2403,7 +2412,7 @@ class P4Sync(Command, P4UserMap):
>>>                   print "parent %s" % parent
>>>               self.gitStream.write("from %s\n" % parent)
>>>
>>> -        self.streamP4Files(new_files)
>>> +        self.streamP4Files(files)
>>>           self.gitStream.write("\n")
>>>
>>>           change = int(details["change"])
>>> diff --git a/t/t9826-git-p4-ignore-empty-commits.sh b/t/t9826-git-p4-ignore-empty-commits.sh
>>> new file mode 100755
>>> index 0000000..5ddccde
>>> --- /dev/null
>>> +++ b/t/t9826-git-p4-ignore-empty-commits.sh
>>> @@ -0,0 +1,103 @@
>>> +#!/bin/sh
>>> +
>>> +test_description='Clone repositories and ignore empty commits'
>>> +
>>> +. ./lib-git-p4.sh
>>> +
>>> +test_expect_success 'start p4d' '
>>> +	start_p4d
>>> +'
>>> +
>>> +test_expect_success 'Create a repo' '
>>> +	client_view "//depot/... //client/..." &&
>>> +	(
>>> +		cd "$cli" &&
>>> +
>>> +		mkdir -p subdir &&
>>> +
>>> +		>subdir/file1.txt &&
>>> +		p4 add subdir/file1.txt &&
>>> +		p4 submit -d "Add file 1" &&
>>> +
>>> +		>file2.txt &&
>>> +		p4 add file2.txt &&
>>> +		p4 submit -d "Add file 2" &&
>>> +
>>> +		>subdir/file3.txt &&
>>> +		p4 add subdir/file3.txt &&
>>> +		p4 submit -d "Add file 3"
>>> +	)
>>> +'
>>> +
>>> +test_expect_success 'Clone repo root path with all history' '
>>> +	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 3
>>> +[git-p4: depot-paths = "//depot/": change = 3]
>>> +
>>> +Add file 2
>>> +[git-p4: depot-paths = "//depot/": change = 2]
>>> +
>>> +Add file 1
>>> +[git-p4: depot-paths = "//depot/": change = 1]
>>
>> Could you not just test for existence of these files?
> Well, I assume the right files are in there as this is covered with other tests. I want to check that these particular commits are mentioned in the logs (including the commit message and the change list number).
>
>
>> If the format of the magic comments that git-p4 ever changes, this will break.
> I understand your reasoning. But how can I check for the correct commit messages, change list number and their order in a efficient different way?
Fair enough!
>
>

Thanks! Luke

Lars Schneider· Oct 28, 2015, 22:35 UTC · re: Luke Diamand · lore

Re: [PATCH v1] git-p4: Add option to ignore empty commits

On 26 Oct 2015, at 21:40, Luke Diamand <luke@diamand.org> wrote:
Show 7 quoted lines
> On 24/10/15 19:08, Lars Schneider wrote:
>> 
>> On 21 Oct 2015, at 08:32, Luke Diamand <luke@diamand.org> wrote:
>> 
>>> On 19/10/15 19:43, larsxschneider@gmail.com wrote:
>>>> From: Lars Schneider <larsxschneider@gmail.com>
>>>> 
-- snip --
Show 6 quoted lines
>> 
>>> Also, could you use python3 style print stmnts, print("whatever") ?
>> Sure. How do you prefer the formatting? Using "format" would be true Python 3 style I think:
>> print('Ignoring file outside of client spec: {}'.format(path))
> 
> Will that breaker older versions of python? There's a statement somewhere about how far back we support.

The "format" method requires Python 2.6 according to the Python docs: https://docs.python.org/2/library/functions.html#format

Luckily this is also the version we aim to support according to Documentation/CodingGuidelines

Thanks, Lars

← back to recent threads