Re: [PATCH 1/1] git-p4: add format-patch subcommand
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Feb 26, 2018, 23:48 UTC
- Message-ID
- <CAPig+cRt3cdKOCFmbuUMQPuBnP+weD3HcY-5kz9RMkPH=aP63g@mail.gmail.com>
- In-Reply-To
- <20180226114822.1617-2-luke@diamand.org>
On Mon, Feb 26, 2018 at 6:48 AM, Luke Diamand <luke@diamand.org> wrote:
Show 11 quoted lines
> It takes a list of P4 changelists and generates a patch for
> each one, using "p4 describe".
> [...]
> Signed-off-by: Luke Diamand <luke@diamand.org>
> ---
> 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.
Show 8 quoted lines
> + 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?
Show 21 quoted lines
> + 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...$"?
Show 5 quoted lines
> + 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?
Show 26 quoted lines
> + 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?
Show 33 quoted lines
> + 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.
Show 16 quoted lines
> + 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 works/symblinks/symlinks/
Show 7 quoted lines
> +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.
Show 23 quoted lines
> + 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?
Show 6 quoted lines
> + p4 add symlink_source symlink && > + p4 submit -d "add symlink" # cl 6 > + ) > +' > + > +test_expect_success 'patch from symlink' '
Ditto SYMLINKS?
Show 8 quoted lines
> + 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 > + ) > +'