On 21 Oct 2015, at 08:32, Luke Diamand <luke@xxxxxxxxxxx> wrote:
> On 19/10/15 19:43, larsxschneider@xxxxxxxxx wrote:
>> From: Lars Schneider <larsxschneider@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> ---
>> 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--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html