From: Luke Diamand Date: Wed, 21 Oct 2015 06:32:55 GMT Subject: Re: [PATCH v1] git-p4: Add option to ignore empty commits Message-ID: <56273197.3010505@diamand.org> In-Reply-To: <1445280239-39840-1-git-send-email-larsxschneider@gmail.com> On 19/10/15 19:43, larsxschneider@gmail.com wrote: > From: Lars Schneider > > 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 > --- > 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