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

From: Lars Schneider <larsxschneider@gmail.com>
Date: Oct 24, 2015, 16:28 UTC
Message-ID: <C5352225-D67D-4ECD-83DB-D5E5A8ED13A7@gmail.com>
In-Reply-To: <xmqqmvvd8n7t.fsf@gitster.mtv.corp.google.com>

On 20 Oct 2015, at 19:27, Junio C Hamano <gitster@pobox.com> wrote:

Show 31 quoted lines

> larsxschneider@gmail.com writes:
> 
>> 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)
> 
> Earlier, the paths outside the clientspec were not in filesToDelete
> (or filesToRead that is below the context here).  Now they all go to
> these arrays, and will hit this loop beyond the context:
> 
>        # deleted files...
>        for f in filesToDelete:
>            self.streamOneP4Deletion(f)
> 
> after leaving the above for loop.  I cannot quite see where this
> "stream one deletion" is turned into a no-op for paths outside after
> this patch gets applied.

Earlier 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.

Show 7 quoted lines

> Also I have this suspicion that those who do want to use client spec
> to get a narrowed view into the history would almost always want
> this "ignore empty" behaviour (I'd even say the current behaviour to
> leave empty commits by default is a bug).  What are the advantages
> of keeping empty commits?  If there aren't many, perhaps git-p4
> should by the default skip empties and require p4.keepEmpty
> configuration to keep them?

I agree. @Luke: What option do you prefer? "git-p4.keepEmptyCommits" or "git-p4.ignoreEmptyCommits" ?

Thanks, Lars