From: Luke Diamand Date: Mon, 26 Oct 2015 20:40:02 GMT Subject: Re: [PATCH v1] git-p4: Add option to ignore empty commits Message-ID: <562E8FA2.9050507@diamand.org> In-Reply-To: On 24/10/15 19:08, Lars Schneider wrote: > > On 21 Oct 2015, at 08:32, Luke Diamand wrote: > >> On 19/10/15 19:43, larsxschneider@gmail.com wrote: >>> From: Lars Schneider >>> >> >> 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