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

[Luke Diamand <luke@diamand.org>]

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