{"thread":{"id":"40590","subject":"[PATCH v1] git-p4: Add option to ignore empty commits","startedAt":"2015-10-19T18:43:59Z","lastAt":"2015-10-28T22:35:04Z","messageCount":8,"participants":["larsxschneider@gmail.com","Junio C Hamano","Luke Diamand","Lars Schneider"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"271944","messageId":"1445280239-39840-1-git-send-email-larsxschneider@gmail.com","threadId":"40590","inReplyTo":null,"subject":"[PATCH v1] git-p4: Add option to ignore empty commits","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-10-19T18:43:59Z","receivedAt":"2015-10-19T18:43:59Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider <larsxschneider@gmail.com>\n\nA changelist that contains only excluded files (e.g. via client spec or\nbranch prefix) will be imported as empty commit. Add option\n\"git-p4.ignoreEmptyCommits\" to ignore these commits.\n\nSigned-off-by: Lars Schneider <larsxschneider@gmail.com>\n---\n Documentation/git-p4.txt               |   5 ++\n git-p4.py                              |  41 ++++++++-----\n t/t9826-git-p4-ignore-empty-commits.sh | 103 +++++++++++++++++++++++++++++++++\n 3 files changed, 133 insertions(+), 16 deletions(-)\n create mode 100755 t/t9826-git-p4-ignore-empty-commits.sh\n\ndiff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\nindex 82aa5d6..f096a6a 100644\n--- a/Documentation/git-p4.txt\n+++ b/Documentation/git-p4.txt\n@@ -510,6 +510,11 @@ git-p4.useClientSpec::\n \toption '--use-client-spec'.  See the \"CLIENT SPEC\" section above.\n \tThis variable is a boolean, not the name of a p4 client.\n \n+git-p4.ignoreEmptyCommits::\n+\tA changelist that contains only excluded files will be imported\n+\tas empty commit. To ignore these commits set this boolean option\n+\tto 'true'.\n+\n Submit variables\n ~~~~~~~~~~~~~~~~\n git-p4.detectRenames::\ndiff --git a/git-p4.py b/git-p4.py\nindex 0093fa3..6c50c74 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -2288,12 +2288,6 @@ class P4Sync(Command, P4UserMap):\n         filesToDelete = []\n \n         for f in files:\n-            # if using a client spec, only add the files that have\n-            # a path in the client\n-            if self.clientSpecDirs:\n-                if self.clientSpecDirs.map_in_client(f['path']) == \"\":\n-                    continue\n-\n             filesForCommit.append(f)\n             if f['action'] in self.delete_actions:\n                 filesToDelete.append(f)\n@@ -2368,18 +2362,33 @@ class P4Sync(Command, P4UserMap):\n         if self.verbose:\n             print \"commit into %s\" % branch\n \n-        # start with reading files; if that fails, we should not\n-        # create a commit.\n-        new_files = []\n-        for f in files:\n-            if [p for p in self.branchPrefixes if p4PathStartsWith(f['path'], p)]:\n-                new_files.append (f)\n-            else:\n-                sys.stderr.write(\"Ignoring file outside of prefix: %s\\n\" % f['path'])\n-\n         if self.clientSpecDirs:\n             self.clientSpecDirs.update_client_spec_path_cache(files)\n \n+        def inClientSpec(path):\n+            if not self.clientSpecDirs:\n+                return True\n+            inClientSpec = self.clientSpecDirs.map_in_client(path)\n+            if not inClientSpec and self.verbose:\n+                print '\\n  Ignoring file outside of client spec' % path\n+            return inClientSpec\n+\n+        def hasBranchPrefix(path):\n+            if not self.branchPrefixes:\n+                return True\n+            hasPrefix = [p for p in self.branchPrefixes\n+                            if p4PathStartsWith(path, p)]\n+            if hasPrefix and self.verbose:\n+                print '\\n  Ignoring file outside of prefix: %s' % path\n+            return hasPrefix\n+\n+        files = [f for f in files\n+            if inClientSpec(f['path']) and hasBranchPrefix(f['path'])]\n+\n+        if not files and gitConfigBool('git-p4.ignoreEmptyCommits'):\n+            print '\\n  Ignoring change %s as it would produce an empty commit.'\n+            return\n+\n         self.gitStream.write(\"commit %s\\n\" % branch)\n #        gitStream.write(\"mark :%s\\n\" % details[\"change\"])\n         self.committedChanges.add(int(details[\"change\"]))\n@@ -2403,7 +2412,7 @@ class P4Sync(Command, P4UserMap):\n                 print \"parent %s\" % parent\n             self.gitStream.write(\"from %s\\n\" % parent)\n \n-        self.streamP4Files(new_files)\n+        self.streamP4Files(files)\n         self.gitStream.write(\"\\n\")\n \n         change = int(details[\"change\"])\ndiff --git a/t/t9826-git-p4-ignore-empty-commits.sh b/t/t9826-git-p4-ignore-empty-commits.sh\nnew file mode 100755\nindex 0000000..5ddccde\n--- /dev/null\n+++ b/t/t9826-git-p4-ignore-empty-commits.sh\n@@ -0,0 +1,103 @@\n+#!/bin/sh\n+\n+test_description='Clone repositories and ignore empty commits'\n+\n+. ./lib-git-p4.sh\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d\n+'\n+\n+test_expect_success 'Create a repo' '\n+\tclient_view \"//depot/... //client/...\" &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\n+\t\tmkdir -p subdir &&\n+\n+\t\t>subdir/file1.txt &&\n+\t\tp4 add subdir/file1.txt &&\n+\t\tp4 submit -d \"Add file 1\" &&\n+\n+\t\t>file2.txt &&\n+\t\tp4 add file2.txt &&\n+\t\tp4 submit -d \"Add file 2\" &&\n+\n+\t\t>subdir/file3.txt &&\n+\t\tp4 add subdir/file3.txt &&\n+\t\tp4 submit -d \"Add file 3\"\n+\t)\n+'\n+\n+test_expect_success 'Clone repo root path with all history' '\n+\tclient_view \"//depot/... //client/...\" &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit init . &&\n+\t\tgit p4 clone --use-client-spec --destination=\"$git\" //depot@all &&\n+\t\tcat >expect <<-\\EOF &&\n+Add file 3\n+[git-p4: depot-paths = \"//depot/\": change = 3]\n+\n+Add file 2\n+[git-p4: depot-paths = \"//depot/\": change = 2]\n+\n+Add file 1\n+[git-p4: depot-paths = \"//depot/\": change = 1]\n+\n+\t\tEOF\n+\t\tgit log --format=%B >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'Clone repo subdir with all history' '\n+\tclient_view \"//depot/subdir/... //client/subdir/...\" &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit init . &&\n+\t\tgit p4 clone --use-client-spec --destination=\"$git\" //depot@all &&\n+\t\tcat >expect <<-\\EOF &&\n+Add file 3\n+[git-p4: depot-paths = \"//depot/\": change = 3]\n+\n+Add file 2\n+[git-p4: depot-paths = \"//depot/\": change = 2]\n+\n+Add file 1\n+[git-p4: depot-paths = \"//depot/\": change = 1]\n+\n+\t\tEOF\n+\t\tgit log --format=%B >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'Clone repo subdir with all history but ignore empty commits' '\n+\tclient_view \"//depot/subdir/... //client/subdir/...\" &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit init . &&\n+\t\tgit config git-p4.ignoreEmptyCommits true &&\n+\t\tgit p4 clone --use-client-spec --destination=\"$git\" //depot@all &&\n+\t\tcat >expect <<-\\EOF &&\n+Add file 3\n+[git-p4: depot-paths = \"//depot/\": change = 3]\n+\n+Add file 1\n+[git-p4: depot-paths = \"//depot/\": change = 1]\n+\n+\t\tEOF\n+\t\tgit log --format=%B >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'kill p4d' '\n+\tkill_p4d\n+'\n+\n+test_done\n-- \n2.5.1\n"},{"id":"271982","messageId":"xmqqmvvd8n7t.fsf@gitster.mtv.corp.google.com","threadId":"40590","inReplyTo":"1445280239-39840-1-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v1] git-p4: Add option to ignore empty commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-20T17:27:18Z","receivedAt":"2015-10-20T17:27:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"larsxschneider@gmail.com writes:\n\n> diff --git a/git-p4.py b/git-p4.py\n> index 0093fa3..6c50c74 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -2288,12 +2288,6 @@ class P4Sync(Command, P4UserMap):\n>          filesToDelete = []\n>  \n>          for f in files:\n> -            # if using a client spec, only add the files that have\n> -            # a path in the client\n> -            if self.clientSpecDirs:\n> -                if self.clientSpecDirs.map_in_client(f['path']) == \"\":\n> -                    continue\n> -\n>              filesForCommit.append(f)\n>              if f['action'] in self.delete_actions:\n>                  filesToDelete.append(f)\n\nEarlier, the paths outside the clientspec were not in filesToDelete\n(or filesToRead that is below the context here).  Now they all go to\nthese arrays, and will hit this loop beyond the context:\n\n        # deleted files...\n        for f in filesToDelete:\n            self.streamOneP4Deletion(f)\n\nafter leaving the above for loop.  I cannot quite see where this\n\"stream one deletion\" is turned into a no-op for paths outside after\nthis patch gets applied.\n\nAlso I have this suspicion that those who do want to use client spec\nto get a narrowed view into the history would almost always want\nthis \"ignore empty\" behaviour (I'd even say the current behaviour to\nleave empty commits by default is a bug).  What are the advantages\nof keeping empty commits?  If there aren't many, perhaps git-p4\nshould by the default skip empties and require p4.keepEmpty\nconfiguration to keep them?\n"},{"id":"272026","messageId":"56273197.3010505@diamand.org","threadId":"40590","inReplyTo":"1445280239-39840-1-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v1] git-p4: Add option to ignore empty commits","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-10-21T06:32:55Z","receivedAt":"2015-10-21T06:32:55Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 19/10/15 19:43, larsxschneider@gmail.com wrote:\n> From: Lars Schneider <larsxschneider@gmail.com>\n>\n> A changelist that contains only excluded files (e.g. via client spec or\n> branch prefix) will be imported as empty commit. Add option\n> \"git-p4.ignoreEmptyCommits\" to ignore these commits.\n>\n> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>\n> ---\n>   Documentation/git-p4.txt               |   5 ++\n>   git-p4.py                              |  41 ++++++++-----\n>   t/t9826-git-p4-ignore-empty-commits.sh | 103 +++++++++++++++++++++++++++++++++\n>   3 files changed, 133 insertions(+), 16 deletions(-)\n>   create mode 100755 t/t9826-git-p4-ignore-empty-commits.sh\n>\n> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\n> index 82aa5d6..f096a6a 100644\n> --- a/Documentation/git-p4.txt\n> +++ b/Documentation/git-p4.txt\n> @@ -510,6 +510,11 @@ git-p4.useClientSpec::\n>   \toption '--use-client-spec'.  See the \"CLIENT SPEC\" section above.\n>   \tThis variable is a boolean, not the name of a p4 client.\n>\n> +git-p4.ignoreEmptyCommits::\n> +\tA changelist that contains only excluded files will be imported\n> +\tas empty commit. To ignore these commits set this boolean option\n> +\tto 'true'.\n\ns/as empty/as an empty/\n\nPossibly putting 'true' in quotes could be confusing.\n\n> +\n>   Submit variables\n>   ~~~~~~~~~~~~~~~~\n>   git-p4.detectRenames::\n> diff --git a/git-p4.py b/git-p4.py\n> index 0093fa3..6c50c74 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -2288,12 +2288,6 @@ class P4Sync(Command, P4UserMap):\n>           filesToDelete = []\n>\n>           for f in files:\n> -            # if using a client spec, only add the files that have\n> -            # a path in the client\n> -            if self.clientSpecDirs:\n> -                if self.clientSpecDirs.map_in_client(f['path']) == \"\":\n> -                    continue\n> -\n>               filesForCommit.append(f)\n>               if f['action'] in self.delete_actions:\n>                   filesToDelete.append(f)\n> @@ -2368,18 +2362,33 @@ class P4Sync(Command, P4UserMap):\n>           if self.verbose:\n>               print \"commit into %s\" % branch\n>\n> -        # start with reading files; if that fails, we should not\n> -        # create a commit.\n> -        new_files = []\n> -        for f in files:\n> -            if [p for p in self.branchPrefixes if p4PathStartsWith(f['path'], p)]:\n> -                new_files.append (f)\n> -            else:\n> -                sys.stderr.write(\"Ignoring file outside of prefix: %s\\n\" % f['path'])\n> -\n>           if self.clientSpecDirs:\n>               self.clientSpecDirs.update_client_spec_path_cache(files)\n>\n> +        def inClientSpec(path):\n\nThis seems to be adding a new function in the middle of an existing \nfunction. Is that right?\n\n> +            if not self.clientSpecDirs:\n> +                return True\n> +            inClientSpec = self.clientSpecDirs.map_in_client(path)\n> +            if not inClientSpec and self.verbose:\n> +                print '\\n  Ignoring file outside of client spec' % path\n> +            return inClientSpec\n\nAny particular reason for putting a \\n at the start of the message?\n\nAlso, could you use python3 style print stmnts, print(\"whatever\") ?\n\n\n\n> +\n> +        def hasBranchPrefix(path):\n> +            if not self.branchPrefixes:\n> +                return True\n> +            hasPrefix = [p for p in self.branchPrefixes\n> +                            if p4PathStartsWith(path, p)]\n> +            if hasPrefix and self.verbose:\n> +                print '\\n  Ignoring file outside of prefix: %s' % path\n> +            return hasPrefix\n> +\n> +        files = [f for f in files\n> +            if inClientSpec(f['path']) and hasBranchPrefix(f['path'])]\n> +\n> +        if not files and gitConfigBool('git-p4.ignoreEmptyCommits'):\n> +            print '\\n  Ignoring change %s as it would produce an empty commit.'\n> +            return\n\nAs with Junio's comment elsewhere, I worry about deletion here.\n\n\n> +\n>           self.gitStream.write(\"commit %s\\n\" % branch)\n>   #        gitStream.write(\"mark :%s\\n\" % details[\"change\"])\n>           self.committedChanges.add(int(details[\"change\"]))\n> @@ -2403,7 +2412,7 @@ class P4Sync(Command, P4UserMap):\n>                   print \"parent %s\" % parent\n>               self.gitStream.write(\"from %s\\n\" % parent)\n>\n> -        self.streamP4Files(new_files)\n> +        self.streamP4Files(files)\n>           self.gitStream.write(\"\\n\")\n>\n>           change = int(details[\"change\"])\n> diff --git a/t/t9826-git-p4-ignore-empty-commits.sh b/t/t9826-git-p4-ignore-empty-commits.sh\n> new file mode 100755\n> index 0000000..5ddccde\n> --- /dev/null\n> +++ b/t/t9826-git-p4-ignore-empty-commits.sh\n> @@ -0,0 +1,103 @@\n> +#!/bin/sh\n> +\n> +test_description='Clone repositories and ignore empty commits'\n> +\n> +. ./lib-git-p4.sh\n> +\n> +test_expect_success 'start p4d' '\n> +\tstart_p4d\n> +'\n> +\n> +test_expect_success 'Create a repo' '\n> +\tclient_view \"//depot/... //client/...\" &&\n> +\t(\n> +\t\tcd \"$cli\" &&\n> +\n> +\t\tmkdir -p subdir &&\n> +\n> +\t\t>subdir/file1.txt &&\n> +\t\tp4 add subdir/file1.txt &&\n> +\t\tp4 submit -d \"Add file 1\" &&\n> +\n> +\t\t>file2.txt &&\n> +\t\tp4 add file2.txt &&\n> +\t\tp4 submit -d \"Add file 2\" &&\n> +\n> +\t\t>subdir/file3.txt &&\n> +\t\tp4 add subdir/file3.txt &&\n> +\t\tp4 submit -d \"Add file 3\"\n> +\t)\n> +'\n> +\n> +test_expect_success 'Clone repo root path with all history' '\n> +\tclient_view \"//depot/... //client/...\" &&\n> +\ttest_when_finished cleanup_git &&\n> +\t(\n> +\t\tcd \"$git\" &&\n> +\t\tgit init . &&\n> +\t\tgit p4 clone --use-client-spec --destination=\"$git\" //depot@all &&\n> +\t\tcat >expect <<-\\EOF &&\n> +Add file 3\n> +[git-p4: depot-paths = \"//depot/\": change = 3]\n> +\n> +Add file 2\n> +[git-p4: depot-paths = \"//depot/\": change = 2]\n> +\n> +Add file 1\n> +[git-p4: depot-paths = \"//depot/\": change = 1]\n\nCould you not just test for existence of these files?\n\nIf the format of the magic comments that git-p4 ever changes, this will \nbreak.\n\n(There's a patch out there that gets git-p4 to use git notes, so it's \nnot as far-fetched as it sounds).\n\n\n> +\n> +\t\tEOF\n> +\t\tgit log --format=%B >actual &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n> +test_expect_success 'Clone repo subdir with all history' '\n> +\tclient_view \"//depot/subdir/... //client/subdir/...\" &&\n> +\ttest_when_finished cleanup_git &&\n> +\t(\n> +\t\tcd \"$git\" &&\n> +\t\tgit init . &&\n> +\t\tgit p4 clone --use-client-spec --destination=\"$git\" //depot@all &&\n> +\t\tcat >expect <<-\\EOF &&\n> +Add file 3\n> +[git-p4: depot-paths = \"//depot/\": change = 3]\n> +\n> +Add file 2\n> +[git-p4: depot-paths = \"//depot/\": change = 2]\n> +\n> +Add file 1\n> +[git-p4: depot-paths = \"//depot/\": change = 1]\n> +\n> +\t\tEOF\n> +\t\tgit log --format=%B >actual &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n> +test_expect_success 'Clone repo subdir with all history but ignore empty commits' '\n> +\tclient_view \"//depot/subdir/... //client/subdir/...\" &&\n> +\ttest_when_finished cleanup_git &&\n> +\t(\n> +\t\tcd \"$git\" &&\n> +\t\tgit init . &&\n> +\t\tgit config git-p4.ignoreEmptyCommits true &&\n> +\t\tgit p4 clone --use-client-spec --destination=\"$git\" //depot@all &&\n> +\t\tcat >expect <<-\\EOF &&\n> +Add file 3\n> +[git-p4: depot-paths = \"//depot/\": change = 3]\n> +\n> +Add file 1\n> +[git-p4: depot-paths = \"//depot/\": change = 1]\n> +\n> +\t\tEOF\n> +\t\tgit log --format=%B >actual &&\n\n\nA deletion test would make me feel more comfortable!\n\nThanks!\nLuke\n"},{"id":"272172","messageId":"C5352225-D67D-4ECD-83DB-D5E5A8ED13A7@gmail.com","threadId":"40590","inReplyTo":"xmqqmvvd8n7t.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1] git-p4: Add option to ignore empty commits","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-10-24T16:28:07Z","receivedAt":"2015-10-24T16:28:07Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 20 Oct 2015, at 19:27, Junio C Hamano <gitster@pobox.com> wrote:\n\n> larsxschneider@gmail.com writes:\n> \n>> diff --git a/git-p4.py b/git-p4.py\n>> index 0093fa3..6c50c74 100755\n>> --- a/git-p4.py\n>> +++ b/git-p4.py\n>> @@ -2288,12 +2288,6 @@ class P4Sync(Command, P4UserMap):\n>>         filesToDelete = []\n>> \n>>         for f in files:\n>> -            # if using a client spec, only add the files that have\n>> -            # a path in the client\n>> -            if self.clientSpecDirs:\n>> -                if self.clientSpecDirs.map_in_client(f['path']) == \"\":\n>> -                    continue\n>> -\n>>             filesForCommit.append(f)\n>>             if f['action'] in self.delete_actions:\n>>                 filesToDelete.append(f)\n> \n> Earlier, the paths outside the clientspec were not in filesToDelete\n> (or filesToRead that is below the context here).  Now they all go to\n> these arrays, and will hit this loop beyond the context:\n> \n>        # deleted files...\n>        for f in filesToDelete:\n>            self.streamOneP4Deletion(f)\n> \n> after leaving the above for loop.  I cannot quite see where this\n> \"stream one deletion\" is turned into a no-op for paths outside after\n> this patch gets applied.\n\nEarlier 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.\n\n\n> Also I have this suspicion that those who do want to use client spec\n> to get a narrowed view into the history would almost always want\n> this \"ignore empty\" behaviour (I'd even say the current behaviour to\n> leave empty commits by default is a bug).  What are the advantages\n> of keeping empty commits?  If there aren't many, perhaps git-p4\n> should by the default skip empties and require p4.keepEmpty\n> configuration to keep them?\n\nI agree. \n@Luke: What option do you prefer? \"git-p4.keepEmptyCommits\" or \"git-p4.ignoreEmptyCommits\" ?\n\nThanks,\nLars"},{"id":"272176","messageId":"F77F291C-89D1-48B6-9E9F-AD7220CE0141@gmail.com","threadId":"40590","inReplyTo":"56273197.3010505@diamand.org","subject":"Re: [PATCH v1] git-p4: Add option to ignore empty commits","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-10-24T18:08:02Z","receivedAt":"2015-10-24T18:08:02Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 21 Oct 2015, at 08:32, Luke Diamand <luke@diamand.org> wrote:\n\n> On 19/10/15 19:43, larsxschneider@gmail.com wrote:\n>> From: Lars Schneider <larsxschneider@gmail.com>\n>> \n>> A changelist that contains only excluded files (e.g. via client spec or\n>> branch prefix) will be imported as empty commit. Add option\n>> \"git-p4.ignoreEmptyCommits\" to ignore these commits.\n>> \n>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>\n>> ---\n>>  Documentation/git-p4.txt               |   5 ++\n>>  git-p4.py                              |  41 ++++++++-----\n>>  t/t9826-git-p4-ignore-empty-commits.sh | 103 +++++++++++++++++++++++++++++++++\n>>  3 files changed, 133 insertions(+), 16 deletions(-)\n>>  create mode 100755 t/t9826-git-p4-ignore-empty-commits.sh\n>> \n>> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\n>> index 82aa5d6..f096a6a 100644\n>> --- a/Documentation/git-p4.txt\n>> +++ b/Documentation/git-p4.txt\n>> @@ -510,6 +510,11 @@ git-p4.useClientSpec::\n>>  \toption '--use-client-spec'.  See the \"CLIENT SPEC\" section above.\n>>  \tThis variable is a boolean, not the name of a p4 client.\n>> \n>> +git-p4.ignoreEmptyCommits::\n>> +\tA changelist that contains only excluded files will be imported\n>> +\tas empty commit. To ignore these commits set this boolean option\n>> +\tto 'true'.\n> \n> s/as empty/as an empty/\n> \n> Possibly putting 'true' in quotes could be confusing.\nOK. Will fix.\n\n> \n>> +\n>>  Submit variables\n>>  ~~~~~~~~~~~~~~~~\n>>  git-p4.detectRenames::\n>> diff --git a/git-p4.py b/git-p4.py\n>> index 0093fa3..6c50c74 100755\n>> --- a/git-p4.py\n>> +++ b/git-p4.py\n>> @@ -2288,12 +2288,6 @@ class P4Sync(Command, P4UserMap):\n>>          filesToDelete = []\n>> \n>>          for f in files:\n>> -            # if using a client spec, only add the files that have\n>> -            # a path in the client\n>> -            if self.clientSpecDirs:\n>> -                if self.clientSpecDirs.map_in_client(f['path']) == \"\":\n>> -                    continue\n>> -\n>>              filesForCommit.append(f)\n>>              if f['action'] in self.delete_actions:\n>>                  filesToDelete.append(f)\n>> @@ -2368,18 +2362,33 @@ class P4Sync(Command, P4UserMap):\n>>          if self.verbose:\n>>              print \"commit into %s\" % branch\n>> \n>> -        # start with reading files; if that fails, we should not\n>> -        # create a commit.\n>> -        new_files = []\n>> -        for f in files:\n>> -            if [p for p in self.branchPrefixes if p4PathStartsWith(f['path'], p)]:\n>> -                new_files.append (f)\n>> -            else:\n>> -                sys.stderr.write(\"Ignoring file outside of prefix: %s\\n\" % f['path'])\n>> -\n>>          if self.clientSpecDirs:\n>>              self.clientSpecDirs.update_client_spec_path_cache(files)\n>> \n>> +        def inClientSpec(path):\n> \n> This seems to be adding a new function in the middle of an existing function. Is that right?\nThat 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.\n\n\n> \n>> +            if not self.clientSpecDirs:\n>> +                return True\n>> +            inClientSpec = self.clientSpecDirs.map_in_client(path)\n>> +            if not inClientSpec and self.verbose:\n>> +                print '\\n  Ignoring file outside of client spec' % path\n>> +            return inClientSpec\n> \n> Any particular reason for putting a \\n at the start of the message?\nI 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? \n\n\n> \n> Also, could you use python3 style print stmnts, print(\"whatever\") ?\nSure. How do you prefer the formatting? Using \"format\" would be true Python 3 style I think:\nprint('Ignoring file outside of client spec: {}'.format(path))\nOK?\n\n> \n>> +\n>> +        def hasBranchPrefix(path):\n>> +            if not self.branchPrefixes:\n>> +                return True\n>> +            hasPrefix = [p for p in self.branchPrefixes\n>> +                            if p4PathStartsWith(path, p)]\n>> +            if hasPrefix and self.verbose:\n>> +                print '\\n  Ignoring file outside of prefix: %s' % path\n>> +            return hasPrefix\n>> +\n>> +        files = [f for f in files\n>> +            if inClientSpec(f['path']) and hasBranchPrefix(f['path'])]\n>> +\n>> +        if not files and gitConfigBool('git-p4.ignoreEmptyCommits'):\n>> +            print '\\n  Ignoring change %s as it would produce an empty commit.'\n>> +            return\n> \n> As with Junio's comment elsewhere, I worry about deletion here.\nI believe this is right. See my answer to Junio.\n\n\n>> +\n>>          self.gitStream.write(\"commit %s\\n\" % branch)\n>>  #        gitStream.write(\"mark :%s\\n\" % details[\"change\"])\n>>          self.committedChanges.add(int(details[\"change\"]))\n>> @@ -2403,7 +2412,7 @@ class P4Sync(Command, P4UserMap):\n>>                  print \"parent %s\" % parent\n>>              self.gitStream.write(\"from %s\\n\" % parent)\n>> \n>> -        self.streamP4Files(new_files)\n>> +        self.streamP4Files(files)\n>>          self.gitStream.write(\"\\n\")\n>> \n>>          change = int(details[\"change\"])\n>> diff --git a/t/t9826-git-p4-ignore-empty-commits.sh b/t/t9826-git-p4-ignore-empty-commits.sh\n>> new file mode 100755\n>> index 0000000..5ddccde\n>> --- /dev/null\n>> +++ b/t/t9826-git-p4-ignore-empty-commits.sh\n>> @@ -0,0 +1,103 @@\n>> +#!/bin/sh\n>> +\n>> +test_description='Clone repositories and ignore empty commits'\n>> +\n>> +. ./lib-git-p4.sh\n>> +\n>> +test_expect_success 'start p4d' '\n>> +\tstart_p4d\n>> +'\n>> +\n>> +test_expect_success 'Create a repo' '\n>> +\tclient_view \"//depot/... //client/...\" &&\n>> +\t(\n>> +\t\tcd \"$cli\" &&\n>> +\n>> +\t\tmkdir -p subdir &&\n>> +\n>> +\t\t>subdir/file1.txt &&\n>> +\t\tp4 add subdir/file1.txt &&\n>> +\t\tp4 submit -d \"Add file 1\" &&\n>> +\n>> +\t\t>file2.txt &&\n>> +\t\tp4 add file2.txt &&\n>> +\t\tp4 submit -d \"Add file 2\" &&\n>> +\n>> +\t\t>subdir/file3.txt &&\n>> +\t\tp4 add subdir/file3.txt &&\n>> +\t\tp4 submit -d \"Add file 3\"\n>> +\t)\n>> +'\n>> +\n>> +test_expect_success 'Clone repo root path with all history' '\n>> +\tclient_view \"//depot/... //client/...\" &&\n>> +\ttest_when_finished cleanup_git &&\n>> +\t(\n>> +\t\tcd \"$git\" &&\n>> +\t\tgit init . &&\n>> +\t\tgit p4 clone --use-client-spec --destination=\"$git\" //depot@all &&\n>> +\t\tcat >expect <<-\\EOF &&\n>> +Add file 3\n>> +[git-p4: depot-paths = \"//depot/\": change = 3]\n>> +\n>> +Add file 2\n>> +[git-p4: depot-paths = \"//depot/\": change = 2]\n>> +\n>> +Add file 1\n>> +[git-p4: depot-paths = \"//depot/\": change = 1]\n> \n> Could you not just test for existence of these files?\nWell, 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).\n\n\n> If the format of the magic comments that git-p4 ever changes, this will break.\nI understand your reasoning. But how can I check for the correct commit messages, change list number and their order in a efficient different way?\n\n\n> \n> (There's a patch out there that gets git-p4 to use git notes, so it's not as far-fetched as it sounds).\n> \n> \n>> +\n>> +\t\tEOF\n>> +\t\tgit log --format=%B >actual &&\n>> +\t\ttest_cmp expect actual\n>> +\t)\n>> +'\n>> +\n>> +test_expect_success 'Clone repo subdir with all history' '\n>> +\tclient_view \"//depot/subdir/... //client/subdir/...\" &&\n>> +\ttest_when_finished cleanup_git &&\n>> +\t(\n>> +\t\tcd \"$git\" &&\n>> +\t\tgit init . &&\n>> +\t\tgit p4 clone --use-client-spec --destination=\"$git\" //depot@all &&\n>> +\t\tcat >expect <<-\\EOF &&\n>> +Add file 3\n>> +[git-p4: depot-paths = \"//depot/\": change = 3]\n>> +\n>> +Add file 2\n>> +[git-p4: depot-paths = \"//depot/\": change = 2]\n>> +\n>> +Add file 1\n>> +[git-p4: depot-paths = \"//depot/\": change = 1]\n>> +\n>> +\t\tEOF\n>> +\t\tgit log --format=%B >actual &&\n>> +\t\ttest_cmp expect actual\n>> +\t)\n>> +'\n>> +\n>> +test_expect_success 'Clone repo subdir with all history but ignore empty commits' '\n>> +\tclient_view \"//depot/subdir/... //client/subdir/...\" &&\n>> +\ttest_when_finished cleanup_git &&\n>> +\t(\n>> +\t\tcd \"$git\" &&\n>> +\t\tgit init . &&\n>> +\t\tgit config git-p4.ignoreEmptyCommits true &&\n>> +\t\tgit p4 clone --use-client-spec --destination=\"$git\" //depot@all &&\n>> +\t\tcat >expect <<-\\EOF &&\n>> +Add file 3\n>> +[git-p4: depot-paths = \"//depot/\": change = 3]\n>> +\n>> +Add file 1\n>> +[git-p4: depot-paths = \"//depot/\": change = 1]\n>> +\n>> +\t\tEOF\n>> +\t\tgit log --format=%B >actual &&\n> \n> \n> A deletion test would make me feel more comfortable!\nAgreed! I will add one!\n\nThanks,\nLars"},{"id":"272177","messageId":"562BD709.1020307@diamand.org","threadId":"40590","inReplyTo":"C5352225-D67D-4ECD-83DB-D5E5A8ED13A7@gmail.com","subject":"Re: [PATCH v1] git-p4: Add option to ignore empty commits","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-10-24T19:07:53Z","receivedAt":"2015-10-24T19:07:53Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 24/10/15 17:28, Lars Schneider wrote:\n>\n\n>> Also I have this suspicion that those who do want to use client spec\n>> to get a narrowed view into the history would almost always want\n>> this \"ignore empty\" behaviour (I'd even say the current behaviour to\n>> leave empty commits by default is a bug).  What are the advantages\n>> of keeping empty commits?  If there aren't many, perhaps git-p4\n>> should by the default skip empties and require p4.keepEmpty\n>> configuration to keep them?\n>\n> I agree.\n> @Luke: What option do you prefer? \"git-p4.keepEmptyCommits\" or \"git-p4.ignoreEmptyCommits\" ?\n\nkeepEmptyCommits.\n"},{"id":"272268","messageId":"562E8FA2.9050507@diamand.org","threadId":"40590","inReplyTo":"F77F291C-89D1-48B6-9E9F-AD7220CE0141@gmail.com","subject":"Re: [PATCH v1] git-p4: Add option to ignore empty commits","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-10-26T20:40:02Z","receivedAt":"2015-10-26T20:40:02Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 24/10/15 19:08, Lars Schneider wrote:\n>\n> On 21 Oct 2015, at 08:32, Luke Diamand <luke@diamand.org> wrote:\n>\n>> On 19/10/15 19:43, larsxschneider@gmail.com wrote:\n>>> From: Lars Schneider <larsxschneider@gmail.com>\n>>>\n>>\n>> This seems to be adding a new function in the middle of an existing function. Is that right?\n> 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.\n\nIt just seemed a bit confusing, but I'm ok with them here as well.\n\n>\n>\n>>\n>>> +            if not self.clientSpecDirs:\n>>> +                return True\n>>> +            inClientSpec = self.clientSpecDirs.map_in_client(path)\n>>> +            if not inClientSpec and self.verbose:\n>>> +                print '\\n  Ignoring file outside of client spec' % path\n>>> +            return inClientSpec\n>>\n>> Any particular reason for putting a \\n at the start of the message?\n> 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?\n\nself.silent and self.verbose seem like two slightly different things. I \nwould expect silent to prevent any output at all. But actually it doesn't.\n\nIf we wanted to implement it properly, I think we'd need a new function \n(p4print?) which did the obvious right thing. Maybe something for a \nrainy day in the future.\n\n>\n>\n>>\n>> Also, could you use python3 style print stmnts, print(\"whatever\") ?\n> Sure. How do you prefer the formatting? Using \"format\" would be true Python 3 style I think:\n> print('Ignoring file outside of client spec: {}'.format(path))\n\nWill that breaker older versions of python? There's a statement \nsomewhere about how far back we support.\n\n> OK?\n>\n>>\n>>> +\n>>> +        def hasBranchPrefix(path):\n>>> +            if not self.branchPrefixes:\n>>> +                return True\n>>> +            hasPrefix = [p for p in self.branchPrefixes\n>>> +                            if p4PathStartsWith(path, p)]\n>>> +            if hasPrefix and self.verbose:\n>>> +                print '\\n  Ignoring file outside of prefix: %s' % path\n>>> +            return hasPrefix\n>>> +\n>>> +        files = [f for f in files\n>>> +            if inClientSpec(f['path']) and hasBranchPrefix(f['path'])]\n>>> +\n>>> +        if not files and gitConfigBool('git-p4.ignoreEmptyCommits'):\n>>> +            print '\\n  Ignoring change %s as it would produce an empty commit.'\n>>> +            return\n>>\n>> As with Junio's comment elsewhere, I worry about deletion here.\n> I believe this is right. See my answer to Junio.\n>\n>\n>>> +\n>>>           self.gitStream.write(\"commit %s\\n\" % branch)\n>>>   #        gitStream.write(\"mark :%s\\n\" % details[\"change\"])\n>>>           self.committedChanges.add(int(details[\"change\"]))\n>>> @@ -2403,7 +2412,7 @@ class P4Sync(Command, P4UserMap):\n>>>                   print \"parent %s\" % parent\n>>>               self.gitStream.write(\"from %s\\n\" % parent)\n>>>\n>>> -        self.streamP4Files(new_files)\n>>> +        self.streamP4Files(files)\n>>>           self.gitStream.write(\"\\n\")\n>>>\n>>>           change = int(details[\"change\"])\n>>> diff --git a/t/t9826-git-p4-ignore-empty-commits.sh b/t/t9826-git-p4-ignore-empty-commits.sh\n>>> new file mode 100755\n>>> index 0000000..5ddccde\n>>> --- /dev/null\n>>> +++ b/t/t9826-git-p4-ignore-empty-commits.sh\n>>> @@ -0,0 +1,103 @@\n>>> +#!/bin/sh\n>>> +\n>>> +test_description='Clone repositories and ignore empty commits'\n>>> +\n>>> +. ./lib-git-p4.sh\n>>> +\n>>> +test_expect_success 'start p4d' '\n>>> +\tstart_p4d\n>>> +'\n>>> +\n>>> +test_expect_success 'Create a repo' '\n>>> +\tclient_view \"//depot/... //client/...\" &&\n>>> +\t(\n>>> +\t\tcd \"$cli\" &&\n>>> +\n>>> +\t\tmkdir -p subdir &&\n>>> +\n>>> +\t\t>subdir/file1.txt &&\n>>> +\t\tp4 add subdir/file1.txt &&\n>>> +\t\tp4 submit -d \"Add file 1\" &&\n>>> +\n>>> +\t\t>file2.txt &&\n>>> +\t\tp4 add file2.txt &&\n>>> +\t\tp4 submit -d \"Add file 2\" &&\n>>> +\n>>> +\t\t>subdir/file3.txt &&\n>>> +\t\tp4 add subdir/file3.txt &&\n>>> +\t\tp4 submit -d \"Add file 3\"\n>>> +\t)\n>>> +'\n>>> +\n>>> +test_expect_success 'Clone repo root path with all history' '\n>>> +\tclient_view \"//depot/... //client/...\" &&\n>>> +\ttest_when_finished cleanup_git &&\n>>> +\t(\n>>> +\t\tcd \"$git\" &&\n>>> +\t\tgit init . &&\n>>> +\t\tgit p4 clone --use-client-spec --destination=\"$git\" //depot@all &&\n>>> +\t\tcat >expect <<-\\EOF &&\n>>> +Add file 3\n>>> +[git-p4: depot-paths = \"//depot/\": change = 3]\n>>> +\n>>> +Add file 2\n>>> +[git-p4: depot-paths = \"//depot/\": change = 2]\n>>> +\n>>> +Add file 1\n>>> +[git-p4: depot-paths = \"//depot/\": change = 1]\n>>\n>> Could you not just test for existence of these files?\n> 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).\n>\n>\n>> If the format of the magic comments that git-p4 ever changes, this will break.\n> 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?\n\nFair enough!\n\n>\n>\n\nThanks!\nLuke\n"},{"id":"272459","messageId":"9A4A2FFA-BCF0-46B6-A3A5-69D7F197FDA5@gmail.com","threadId":"40590","inReplyTo":"562E8FA2.9050507@diamand.org","subject":"Re: [PATCH v1] git-p4: Add option to ignore empty commits","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-10-28T22:35:04Z","receivedAt":"2015-10-28T22:35:04Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 26 Oct 2015, at 21:40, Luke Diamand <luke@diamand.org> wrote:\n\n> On 24/10/15 19:08, Lars Schneider wrote:\n>> \n>> On 21 Oct 2015, at 08:32, Luke Diamand <luke@diamand.org> wrote:\n>> \n>>> On 19/10/15 19:43, larsxschneider@gmail.com wrote:\n>>>> From: Lars Schneider <larsxschneider@gmail.com>\n>>>> \n-- snip --\n>> \n>>> Also, could you use python3 style print stmnts, print(\"whatever\") ?\n>> Sure. How do you prefer the formatting? Using \"format\" would be true Python 3 style I think:\n>> print('Ignoring file outside of client spec: {}'.format(path))\n> \n> Will that breaker older versions of python? There's a statement somewhere about how far back we support.\nThe \"format\" method requires Python 2.6 according to the Python docs:\nhttps://docs.python.org/2/library/functions.html#format\n\nLuckily this is also the version we aim to support according to Documentation/CodingGuidelines\n\nThanks,\nLars\n"}]}