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

From: Lars Schneider <larsxschneider@gmail.com>
Date: Oct 24, 2015, 18:08 UTC
Message-ID: <F77F291C-89D1-48B6-9E9F-AD7220CE0141@gmail.com>
In-Reply-To: <56273197.3010505@diamand.org>

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