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

[Luke Diamand <luke@diamand.org>]

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