Add update.py to easily update the imported in-tree copy of Crashpad

Add update.py to easily update the imported in-tree copy of Crashpad

BUG=472900
Committed: https://crrev.com/adcc203a3f95a64d9bd7018adec276cfb7eadeb5
Cr-Commit-Position: refs/heads/master@{#365026}

[Mark Mentovai, 2015-12-09 21:37:04]

mark@chromium.org changed reviewers:
+ primiano@chromium.org

[Mark Mentovai, 2015-12-09 21:37:05]

Based on the discussion on and off https://codereview.chromium.org/1410853008, I
came up with this script. There’s enough text processing in here that a shell
script seemed like a bad idea, so I went with Python. That’ll also help Scott
stand a chance at being able to update Crashpad in Chromium from Windows,
although I didn’t test that.

[Mark Mentovai, 2015-12-09 21:37:51]

Meant to reference https://codereview.chromium.org/1505213004/.

[Mark Mentovai, 2015-12-09 22:40:59]

For persual of this script’s output, https://codereview.chromium.org/1511483006
is what it did all by itself (except I added the “not for checkin”).

[Primiano Tucci (use gerrit), 2015-12-10 09:31:21]

I see, clever, keeping the state in the README. Even if things should go
terribly wrong that would allow you to amend the situation manually without too
much trouble.

LGTM with some comments: I think mostly you don't need the remote and can
simplify this script.

Not 100% sure that --keep-redundant-commits will do what you want (no-oping %
commit message cherry-picks that you already ported in the chrome->crashpad
direction).
If that should fail, I think what you can do to fix it is:

instead of git cherry-pick last_sha1..FETCH_HEAD
do a 
git rev-list last_sha1..FETCH_HEAD --grep='^Cr-Commit-Position:
refs/heads/master' --invert-grep | git cherry-pick --stdin -x -Xsubtree bla bla

That would cherry-pick all the commits in the range but exclude the ones that
have a "Cr-Commit-Position" tag (which very likely were chrome commits in the
first place).

Or if you use the trick we discussed offline (adding the Message-Id: merged from
chromium) when merging in the chrome->crashpad direction you can grep for that
one instead of Cr-Commit-Position.

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update.py
File third_party/crashpad/update.py (right):

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update...
third_party/crashpad/update.py:49: original_head = \
I think our coding style is more in favor of parentheses rather than
backslashes.

"Do not use backslash line continuation." from
https://google.github.io/styleguide/pyguide.html#Line_length

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update...
third_party/crashpad/update.py:76: # The remote might already exist. Check for
it first, and make sure the
you don't need to add the remote here.
I think what you want to do is just a 
git fetch https://chromium.googlesource.com/blabla.git

that will fetch the packs and give you the head into the FETCH_HEAD name

Essentially looks like you can get rid of lines 74-89 and just pass the url to
git fetch

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update...
third_party/crashpad/update.py:92: commitish_new = parsed.update_to or project +
'/master'
and this would become FETCH_HEAD

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update...
third_party/crashpad/update.py:97: subprocess.check_call(['git',
Not necessary for this CL: I think you want to be a bit more conservative here.
There is a chance that this cherry-pick will fail because of merge conflicts, in
which case cherry-pick will return with !0 retcode (and leave your repo dirty).
I think what you want to do here is:
 - If you detect non-zero retcode you want to print a message saying: "Looks
like you have merge conflicts. Open a new terminal, solve them, and then do a
git cherry-pick --continue. Hit enter when done"
 - If the user aborts (KeyboardInterrupt) instead you want to fire a git
cherry-pick --abort to leave the repo in a clean state.

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update...
third_party/crashpad/update.py:99: '--keep-redundant',
Isn't this --keep-redundant-commits (with -commits at the end?)

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update...
third_party/crashpad/update.py:123: # Strip trailing periods from subjects.
such classy :)

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update...
third_party/crashpad/update.py:129: line =
re.sub(r'(\s)([0-9a-fA-F]{12})([0-9a-fA-F]{28})($|\s)',
Hmm dunno about this. This would make copy / grepping into another git log a bit
of a headache.
I find myself often searching for full text in git logs to find matching
commits.
I'd probably leave these as they are

[Mark Mentovai, 2015-12-10 15:13:55]

Thanks, Primiano! Updated.

> I see, clever, keeping the state in the README. Even if things should go
> terribly wrong that would allow you to amend the situation manually without
> too much trouble.

Thanks!

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update.py
File third_party/crashpad/update.py (right):

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update...
third_party/crashpad/update.py:49: original_head = \
Primiano Tucci wrote:
> I think our coding style is more in favor of parentheses rather than
> backslashes.
> 
> "Do not use backslash line continuation." from
> https://google.github.io/styleguide/pyguide.html#Line_length

Done.

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update...
third_party/crashpad/update.py:76: # The remote might already exist. Check for
it first, and make sure the
Primiano Tucci wrote:
> you don't need to add the remote here.
> I think what you want to do is just a 
> git fetch https://chromium.googlesource.com/blabla.git
> 
> that will fetch the packs and give you the head into the FETCH_HEAD name
> 
> Essentially looks like you can get rid of lines 74-89 and just pass the url to
> git fetch

That’s nicer. Done.

Let’s say that hypothetically, I wanted this script’s --update-to to accept a
remote ref. I know that I can say “git fetch repository_url ref_name” and
FETCH_HEAD will be set up to point to the ref. But how do I tell whether
--update-to was a hash or a ref? What does git do here? Does it treat any string
of 4 or more hex digits as a hash, and treat anything else as a ref? Does it do
anything to resolve ambiguity between a ref named abcd and a hash that starts
with abcd? Is there a way to get git to resolve the difference for me?

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update...
third_party/crashpad/update.py:92: commitish_new = parsed.update_to or project +
'/master'
Primiano Tucci wrote:
> and this would become FETCH_HEAD

Done.

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update...
third_party/crashpad/update.py:99: '--keep-redundant',
Primiano Tucci wrote:
> Isn't this --keep-redundant-commits (with -commits at the end?)

It is, but --keep-redundant worked too. Done.

> Not 100% sure that --keep-redundant-commits will do what you want (no-oping %
> commit message cherry-picks that you already ported in the chrome->crashpad
> direction).

https://chromium.googlesource.com/crashpad/crashpad/+/7efdc94f5982a1f9654c53f...
is already in Chromium’s local copy. Without --keep-redundant-commits, the
cherry-pick stops and prompts:

---
The previous cherry-pick is now empty, possibly due to conflict resolution.
If you wish to commit it anyway, use:

    git commit --allow-empty

If you wish to skip this commit, use:

    git reset

Then "git cherry-pick --continue" will resume cherry-picking
the remaining commits.
---

--allow-empty would be fine too because I’m squashing anyway, but
--keep-redundant-commits seems more in line with the spirit of what’s being
done. It does work correctly to update through 7efdc94f5982.

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update...
third_party/crashpad/update.py:123: # Strip trailing periods from subjects.
Primiano Tucci wrote:
> such classy :)

Thanks! :)

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update...
third_party/crashpad/update.py:129: line =
re.sub(r'(\s)([0-9a-fA-F]{12})([0-9a-fA-F]{28})($|\s)',
Primiano Tucci wrote:
> Hmm dunno about this. This would make copy / grepping into another git log a
bit
> of a headache.
> I find myself often searching for full text in git logs to find matching
> commits.
> I'd probably leave these as they are

72’s already very restrictive, and 13 of those are already being used up by the
commit’s own hash. If these guys were allowed to remain at their full
40-character lengths, it’d only leave 19 more characters on the line for prose.
The wrapping gets pretty nasty pretty quickly, and this’ll be just about
guaranteed to wrap which will interfere with clean copy/paste/grep anyway.

Since I’m abbreviating this commit’s hash to 12, I think it’s reasonable to
abbreviate another has that it references to the same length.

[Mark Mentovai, 2015-12-10 15:16:01]

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update.py
File third_party/crashpad/update.py (right):

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update...
third_party/crashpad/update.py:97: subprocess.check_call(['git',
Primiano Tucci wrote:
> Not necessary for this CL: I think you want to be a bit more conservative
here.
> There is a chance that this cherry-pick will fail because of merge conflicts,
in
> which case cherry-pick will return with !0 retcode (and leave your repo
dirty).
> I think what you want to do here is:
>  - If you detect non-zero retcode you want to print a message saying: "Looks
> like you have merge conflicts. Open a new terminal, solve them, and then do a
> git cherry-pick --continue. Hit enter when done"
>  - If the user aborts (KeyboardInterrupt) instead you want to fire a git
> cherry-pick --abort to leave the repo in a clean state.

I’ll do this in a follow-up unless you’re slow to review. :)

[Primiano Tucci (use gerrit), 2015-12-10 15:26:13]

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update.py
File third_party/crashpad/update.py (right):

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update...
third_party/crashpad/update.py:76: # The remote might already exist. Check for
it first, and make sure the
On 2015/12/10 15:13:55, Mark Mentovai wrote:
> Primiano Tucci wrote:
> > you don't need to add the remote here.
> > I think what you want to do is just a 
> > git fetch https://chromium.googlesource.com/blabla.git
> > 
> > that will fetch the packs and give you the head into the FETCH_HEAD name
> > 
> > Essentially looks like you can get rid of lines 74-89 and just pass the url
to
> > git fetch
> 
> That’s nicer. Done.
> 
> Let’s say that hypothetically, I wanted this script’s --update-to to accept a
> remote ref. I know that I can say “git fetch repository_url ref_name” and
> FETCH_HEAD will be set up to point to the ref. But how do I tell whether
> --update-to was a hash or a ref? What does git do here? Does it treat any
string
> of 4 or more hex digits as a hash, and treat anything else as a ref? Does it
do
> anything to resolve ambiguity between a ref named abcd and a hash that starts
> with abcd? Is there a way to get git to resolve the difference for me?

git rev-parse FETCH_HEAD will always give you a sha, regardless of what was the
original input

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update...
third_party/crashpad/update.py:99: '--keep-redundant',
On 2015/12/10 15:13:55, Mark Mentovai wrote:
> Primiano Tucci wrote:
> > Isn't this --keep-redundant-commits (with -commits at the end?)
> 
> It is, but --keep-redundant worked too. Done.
> 
> > Not 100% sure that --keep-redundant-commits will do what you want (no-oping
%
> > commit message cherry-picks that you already ported in the chrome->crashpad
> > direction).
> 
>
https://chromium.googlesource.com/crashpad/crashpad/+/7efdc94f5982a1f9654c53f...
> is already in Chromium’s local copy. Without --keep-redundant-commits, the
> cherry-pick stops and prompts:
> 
> ---
> The previous cherry-pick is now empty, possibly due to conflict resolution.
> If you wish to commit it anyway, use:
> 
>     git commit --allow-empty
> 
> If you wish to skip this commit, use:
> 
>     git reset
> 
> Then "git cherry-pick --continue" will resume cherry-picking
> the remaining commits.
> ---
> 
> --allow-empty would be fine too because I’m squashing anyway, but
> --keep-redundant-commits seems more in line with the spirit of what’s being
> done. It does work correctly to update through 7efdc94f5982.

ah ok. I never figured out how git really does patch detection and when decides
to just bail out :)

[Mark Mentovai, 2015-12-10 15:31:28]

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update.py
File third_party/crashpad/update.py (right):

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update...
third_party/crashpad/update.py:76: # The remote might already exist. Check for
it first, and make sure the
On 2015/12/10 15:26:13, Primiano Tucci wrote:
> On 2015/12/10 15:13:55, Mark Mentovai wrote:
> > Primiano Tucci wrote:
> > > you don't need to add the remote here.
> > > I think what you want to do is just a 
> > > git fetch https://chromium.googlesource.com/blabla.git
> > > 
> > > that will fetch the packs and give you the head into the FETCH_HEAD name
> > > 
> > > Essentially looks like you can get rid of lines 74-89 and just pass the
url
> to
> > > git fetch
> > 
> > That’s nicer. Done.
> > 
> > Let’s say that hypothetically, I wanted this script’s --update-to to accept
a
> > remote ref. I know that I can say “git fetch repository_url ref_name” and
> > FETCH_HEAD will be set up to point to the ref. But how do I tell whether
> > --update-to was a hash or a ref? What does git do here? Does it treat any
> string
> > of 4 or more hex digits as a hash, and treat anything else as a ref? Does it
> do
> > anything to resolve ambiguity between a ref named abcd and a hash that
starts
> > with abcd? Is there a way to get git to resolve the difference for me?
> 
> git rev-parse FETCH_HEAD will always give you a sha, regardless of what was
the
> original input

Sure, but if I say update.py --update-to=6789abcd, I’m expecting to update to a
hash, so I can do “git fetch repository_url” and then “git cherry-pick
whatever..6789abcd”. But if I say update.py --update-to=some_ref, I’d want to do
“git fetch repository_url some_ref” and then “git cherry-pick
whatever..FETCH_HEAD”. I’m asking if there’s some standard git way to
disambiguate between --update-to=6789abcd and --update-to=some_ref in light of
the fact that 6789abcd might be a ref instead of a commit hash.

If not, I can just split it into --update-to-hash and --update-to-ref.

[Mark Mentovai, 2015-12-10 15:50:01]

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update.py
File third_party/crashpad/update.py (right):

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update...
third_party/crashpad/update.py:76: # The remote might already exist. Check for
it first, and make sure the
I wrote:
> Sure, but if I say update.py --update-to=6789abcd, I’m expecting to update to
a
> hash, so I can do “git fetch repository_url” and then “git cherry-pick
> whatever..6789abcd”. But if I say update.py --update-to=some_ref, I’d want to
do
> “git fetch repository_url some_ref” and then “git cherry-pick
> whatever..FETCH_HEAD”. I’m asking if there’s some standard git way to
> disambiguate between --update-to=6789abcd and --update-to=some_ref in light of
> the fact that 6789abcd might be a ref instead of a commit hash.
> 
> If not, I can just split it into --update-to-hash and --update-to-ref.

Oh, another thing I was wondering: is there a way to verify that a hash came
from a given remote (or git fetch?) I know about git rev-list but that seems
like not the best way to do it. Also, it won’t catch a hash that came from
non-master but did come from the remote.

[Primiano Tucci (use gerrit), 2015-12-10 15:53:57]

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update.py
File third_party/crashpad/update.py (right):

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update...
third_party/crashpad/update.py:76: # The remote might already exist. Check for
it first, and make sure the
On 2015/12/10 15:31:28, Mark Mentovai wrote:

> Sure, but if I say update.py --update-to=6789abcd, I’m expecting to update to
a
> hash, so I can do “git fetch repository_url” and then “git cherry-pick
> whatever..6789abcd”. 

Ahh that's the part I was missing. I think the answer is easier than expected.
Just always do git fetch remote ref_or_sha1 and always use FETCH_HEAD (even in
the case of a sha1).

Fetching a SHA1 is not 100% officialy supported by the git-scm server, but it is
definitely a thing with GoB and hence the googlesource.com server, which is
probably what you really care about.

[Mark Mentovai, 2015-12-10 15:58:46]

On 2015/12/10 15:53:57, Primiano Tucci wrote:
>
https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update.py
> File third_party/crashpad/update.py (right):
> 
>
https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update...
> third_party/crashpad/update.py:76: # The remote might already exist. Check for
> it first, and make sure the
> On 2015/12/10 15:31:28, Mark Mentovai wrote:
> 
> > Sure, but if I say update.py --update-to=6789abcd, I’m expecting to update
to
> a
> > hash, so I can do “git fetch repository_url” and then “git cherry-pick
> > whatever..6789abcd”. 
> 
> Ahh that's the part I was missing. I think the answer is easier than expected.
> Just always do git fetch remote ref_or_sha1 and always use FETCH_HEAD (even in
> the case of a sha1).
> 
> Fetching a SHA1 is not 100% officialy supported by the git-scm server, but it
is
> definitely a thing with GoB and hence the http://googlesource.com server,
which is
> probably what you really care about.

That doesn’t work with an abbreviated hash, which practically, I do think we
want to work:

$ git fetch https://chromium.googlesource.com/crashpad/crashpad
From https://chromium.googlesource.com/crashpad/crashpad
 * branch                      HEAD       -> FETCH_HEAD
$ git rev-parse FETCH_HEAD
583d1dc3efa96ea50f62afa69a598eeed1534edc
$ git fetch https://chromium.googlesource.com/crashpad/crashpad
583d1dc3efa96ea50f62afa69a598eeed1534edc
From https://chromium.googlesource.com/crashpad/crashpad
 * branch                      583d1dc3efa96ea50f62afa69a598eeed1534edc ->
FETCH_HEAD
$ git rev-parse FETCH_HEAD
583d1dc3efa96ea50f62afa69a598eeed1534edc
$ git fetch https://chromium.googlesource.com/crashpad/crashpad 583d1dc3efa9 
fatal: Couldn't find remote ref 583d1dc3efa9

[Mark Mentovai, 2015-12-10 20:08:50]

Updated.

After a bunch of chatting, we recognized that --update-to and --fetch-ref needed
to be independent. This is now also able to check that the old hash is an
ancestor of the new hash, and that the new hash is an ancestor of what got
fetched.

This should be complete enough, and with few enough jagged edges, to check in
now.

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update.py
File third_party/crashpad/update.py (right):

https://codereview.chromium.org/1513843002/diff/1/third_party/crashpad/update...
third_party/crashpad/update.py:97: subprocess.check_call(['git',
On 2015/12/10 15:16:01, Mark Mentovai wrote:
> Primiano Tucci wrote:
> > Not necessary for this CL: I think you want to be a bit more conservative
> here.
> > There is a chance that this cherry-pick will fail because of merge
conflicts,
> in
> > which case cherry-pick will return with !0 retcode (and leave your repo
> dirty).
> > I think what you want to do here is:
> >  - If you detect non-zero retcode you want to print a message saying: "Looks
> > like you have merge conflicts. Open a new terminal, solve them, and then do
a
> > git cherry-pick --continue. Hit enter when done"
> >  - If the user aborts (KeyboardInterrupt) instead you want to fire a git
> > cherry-pick --abort to leave the repo in a clean state.
> 
> I’ll do this in a follow-up unless you’re slow to review. :)

Did this now.

[Primiano Tucci (use gerrit), 2015-12-14 10:22:11]

Still LGTM

[Mark Mentovai, 2015-12-14 15:04:25]

The CQ bit was checked by mark@chromium.org

[commit-bot: I haz the power, 2015-12-14 15:05:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1513843002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1513843002/40001

[commit-bot: I haz the power, 2015-12-14 15:27:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-14 15:29:09]

Description was changed from

==========
Add update.py to easily update the imported in-tree copy of Crashpad

BUG=472900
==========

to

==========
Add update.py to easily update the imported in-tree copy of Crashpad

BUG=472900

Committed: https://crrev.com/adcc203a3f95a64d9bd7018adec276cfb7eadeb5
Cr-Commit-Position: refs/heads/master@{#365026}
==========

[commit-bot: I haz the power, 2015-12-14 15:29:10]

Patchset 3 (id:??) landed as
https://crrev.com/adcc203a3f95a64d9bd7018adec276cfb7eadeb5
Cr-Commit-Position: refs/heads/master@{#365026}