On Mon, Feb 26, 2018 at 6:48 AM, Luke Diamand <luke@xxxxxxxxxxx> wrote:
> It takes a list of P4 changelists and generates a patch for
> each one, using "p4 describe".
> [...]
> Signed-off-by: Luke Diamand <luke@xxxxxxxxxxx>
> ---
> diff --git a/git-p4.py b/git-p4.py
> @@ -3749,6 +3761,277 @@ class P4Branches(Command):
> +class P4MakePatch(Command,P4UserMap):
> + def run(self, args):
> + if self.output and not os.path.isdir(self.output):
> + sys.exit("output directory %s does not exist" % self.output)
For consistency with "git format-patch", this could create the output
directory automatically rather than erroring out.
> + if self.strip_depot_prefix:
> + self.clientSpec = getClientSpec()
> + else:
> + self.clientSpec = None
> +
> + self.loadUserMapFromCache()
> + if len(args) < 1:
> + return False
Would it make sense to handle this no-arguments case earlier before
doing work, such as loading the user map from cache?
> + for change in args:
> + self.make_patch(int(change))
> +
> + return True
> +
> + def p4_fetch_delta(self, change, files, shelved=False):
> + cmd = ["describe"]
> + if shelved:
> + cmd += ["-S"]
> + cmd += ["-du"]
> + cmd += ["%s" % change]
> + cmd = p4_build_cmd(cmd)
> +
> + p4 = subprocess.Popen(cmd, shell=False, stdout=subprocess.PIPE)
> + try:
> + result = p4.stdout.readlines()
> + except EOFError:
> + pass
> + in_diff = False
> + matcher = re.compile('====\s+(.*)#(\d+)\s+\(text\)\s+====')
> + diffmatcher = re.compile("Differences ...")
I'm not familiar with the output of "p4 describe", but does it include
user-supplied text? If so, would it be safer to anchor 'diffmatcher',
for instance, "^Diferences...$"?
> + delta = ""
> + skip_next_blank_line = False
> +
> + for line in result:
> + if diffmatcher.match(line):
Stepping back, does "Differences..." appear on a line of its own? If
so, why does this need to be a regex at all? Can't it just do a direct
string comparison?
> + in_diff = True
> + continue
> +
> + if in_diff:
> +
> + if skip_next_blank_line and \
> + line.rstrip() == "":
> + skip_next_blank_line = False
> + continue
> +
> + m = matcher.match(line)
> + if m:
> + file = self.map_path(m.group(1))
> + ver = m.group(2)
> + delta += "diff --git a%s b%s\n" % (file, file)
> + delta += "--- a%s\n" % file
> + delta += "+++ b%s\n" % file
> + skip_next_blank_line = True
> + else:
> + delta += line
> +
> + delta += "\n"
> +
> + exitCode = p4.wait()
> + if exitCode != 0:
> + raise IOError("p4 '%s' command failed" % str(cmd))
Since p4.stdout.readlines() gathered all output from the command
before massaging it into Git format, can't the p4.wait() be done
earlier, just after the output has been read?
> + return delta
> +
> + def make_patch(self, change):
> + [...]
> + # add new or deleted files
> + for file in files:
> + [...]
> + if add or delete:
> + if add:
> + [...]
> + else:
> + [...]
> +
> + [...]
> +
> + if add:
> + prefix = "+"
> + else:
> + prefix = "-"
> +
> + if delete:
> + [...]
> + else:
> + # added
> + [...]
> +
> + (lines, delta_content) = self.read_file_contents(prefix, path_rev)
> +
> + if add:
> + if lines > 0:
> + delta += "@@ -0,0 +1,%d @@\n" % lines
> + else:
> + delta += "@@ -1,%d +0,0 @@\n" % lines
It's not clear why you sometimes check 'add' but other times 'delete'.
Perhaps always used one or the other? For instance:
action = file["action"]
if action == "add" or action == "delete":
if action == "add":
before = "/dev/null"
...
else
...
delta += ...
if action == "add":
...
or something.
> + delta += delta_content
> +
> + if self.output:
> + with open("%s/%s.patch" % (self.output, change), "w") as f:
> + f.write(delta)
> + else:
> + print(delta)
> diff --git a/t/t9832-make-patch.sh b/t/t9832-make-patch.sh
> @@ -0,0 +1,135 @@
> +# Converting P4 changes into patches
> +#
> +# - added, deleted, modified files
> +# - regular commits, shelved commits
> +#
> +# directories and symblinks don't yet work
> +# binary files will never work
s/symblinks/symlinks/
> +test_expect_success 'init depot' '
> + (
> + cd "$cli" &&
> + echo file1 >file1 &&
> + p4 add file1 &&
> + p4 submit -d "change 1" && # cl 1
> + cat >file_to_delete <<-EOF &&
Using -\EOF would signal that you don't expect interpolation within the heredoc.
> + LINE1
> + LINE2
> + EOF
> + echo "non-empty" >file_to_delete &&
> + p4 add file_to_delete &&
> + p4 submit -d "change 2" && # cl 2
> + p4 edit file1 &&
> + cat >>file1 <<-EOF &&
> + LINE1
> + LINE2
> + EOF
> + p4 submit -d "change 3" && # cl 3
> + p4 delete file_to_delete &&
> + echo "file2" >file2 &&
> + p4 add file2 &&
> + p4 submit -d "change 4" # cl 4
> + )
> +'
> +test_expect_success 'add p4 symlink' '
> + (
> + cd "$cli" &&
> + echo "symlink_source" >symlink_source &&
> + ln -s symlink_source symlink &&
Should this test be protected with the SYMLINKS prerequisite?
> + p4 add symlink_source symlink &&
> + p4 submit -d "add symlink" # cl 6
> + )
> +'
> +
> +test_expect_success 'patch from symlink' '
Ditto SYMLINKS?
> + test_when_finished cleanup_git &&
> + cd "$git" &&
> + (
> + git p4 format-patch 6 | patch -p1 &&
> + test_path_is_file depot/symlink_source &&
> + test -L depot/symlink
> + )
> +'
