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

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

[larsxschneider]

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'.
+
 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

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

[Junio C Hamano]

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.

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?

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

[Luke Diamand]

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.

> +
>   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?

> +            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") ?



> +
> +        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.


> +
>           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).


> +
> +		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

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

[Lars Schneider]

On 20 Oct 2015, at 19:27, Junio C Hamano <gitster@pobox.com> wrote:

> 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.


> 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

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

[Lars Schneider]

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>
>> 
>> 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.

> 
>> +
>>  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.


> 
>> +            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?

> 
>> +
>> +        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?


> 
> (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

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

[Luke Diamand]

On 24/10/15 17:28, Lars Schneider wrote:
>

>> 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.

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

[Luke Diamand]

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>
>>>
>>
>> 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.

>
>
>>
>>> +            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.

>
>
>>
>> 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.

> 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

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

[Lars Schneider]

On 26 Oct 2015, at 21:40, Luke Diamand <luke@diamand.org> wrote:

> 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 --
>> 
>>> 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