On 24/10/15 19:08, Lars Schneider wrote:
On 21 Oct 2015, at 08:32, Luke Diamand <luke@xxxxxxxxxxx> wrote:
On 19/10/15 19:43, larsxschneider@xxxxxxxxx wrote:
From: Lars Schneider <larsxschneider@xxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html