[hunspell] Script to automatically generate google.patch

[hunspell] Script to automatically generate google.patch

R=brettw@chromium.org

Committed: 285621

[groby-ooo-7-16, 2014-05-29 02:45:03]

brettw: Added a new (Chromium) script file to third_party/hunspell. It falls
somewhere between the two cases outlined in OWNERS, so PTAL :)

[brettw, 2014-05-29 17:06:17]

https://codereview.chromium.org/306793002/diff/40001/update_google_patch.sh
File update_google_patch.sh (right):

https://codereview.chromium.org/306793002/diff/40001/update_google_patch.sh#n...
update_google_patch.sh:3: # Copyright (c) 2014 The Chromium Authors. All rights
reserved.
Remove the "(c)"

(see http://www.chromium.org/developers/coding-style )

https://codereview.chromium.org/306793002/diff/40001/update_google_patch.sh#n...
update_google_patch.sh:7: # Creates an updated google.patch that reflects all
checked-in  changes in the
Got two spaces after "in"

Can this list some more background of what exactly your repo should look like
here, and an overview of what this script does? For example, I was initially
confused about what it diffed against until I realized it actually seems to
check out the CVS code to diff against. You're also doing some git diff
operations which imply that you're on a branch and the upstream branch has some
specific meaning.

This script seems to update Hunspell. Naming it update_google_patch implies to
me that it just generates the diff file. Maybe a different name is in order
(actually, the existing sentence in the README you modified is similarly
misleading).

[groby-ooo-7-16, 2014-05-29 17:35:37]

On 2014/05/29 17:06:17, brettw wrote:
> https://codereview.chromium.org/306793002/diff/40001/update_google_patch.sh
> File update_google_patch.sh (right):
> 
>
https://codereview.chromium.org/306793002/diff/40001/update_google_patch.sh#n...
> update_google_patch.sh:3: # Copyright (c) 2014 The Chromium Authors. All
rights
> reserved.
> Remove the "(c)"
> 
> (see http://www.chromium.org/developers/coding-style )
> 
>
https://codereview.chromium.org/306793002/diff/40001/update_google_patch.sh#n...
> update_google_patch.sh:7: # Creates an updated google.patch that reflects all
> checked-in  changes in the
> Got two spaces after "in"
> 
> Can this list some more background of what exactly your repo should look like
> here, and an overview of what this script does?

Amended - let me know what you think. I'll give a bit more context than that in
the explanation here, but I don't want to have an endless header comment in the
script.

> For example, I was initially
> confused about what it diffed against until I realized it actually seems to
> check out the CVS code to diff against. You're also doing some git diff
> operations which imply that you're on a branch and the upstream branch has
some
> specific meaning.

Not really. The assumption is merely that upstream is used the same way as
git cl upload uses it.

> This script seems to update Hunspell. Naming it update_google_patch implies to
> me that it just generates the diff file. 

That's exactly what it does. To do that, it needs to check out the baseline from
CVS
into a separate directory, make the CVS checkout consistent with what's in 
third_party/hunspell, produce a diff of _that_ against the CVS baseline, and
update
google.patch

It does the same thing as the sequence of events outlined in the README, except
that it's automated. (It's rather easy to accidentally apply your changes in the
wrong
place otherwise, messing up google.patch)

> Maybe a different name is in order
> (actually, the existing sentence in the README you modified is similarly
> misleading).
Is it still, now that I've (hopefully) made the purpose of the script more
clear?

[groby-ooo-7-16, 2014-05-29 17:37:48]

And fixes for the two nits. Separate from the comments because I missed that.

https://codereview.chromium.org/306793002/diff/40001/update_google_patch.sh
File update_google_patch.sh (right):

https://codereview.chromium.org/306793002/diff/40001/update_google_patch.sh#n...
update_google_patch.sh:3: # Copyright (c) 2014 The Chromium Authors. All rights
reserved.
On 2014/05/29 17:06:17, brettw wrote:
> Remove the "(c)"
> 
> (see http://www.chromium.org/developers/coding-style )

Done.

https://codereview.chromium.org/306793002/diff/40001/update_google_patch.sh#n...
update_google_patch.sh:7: # Creates an updated google.patch that reflects all
checked-in  changes in the
On 2014/05/29 17:06:17, brettw wrote:
> Got two spaces after "in"
> 
> Can this list some more background of what exactly your repo should look like
> here, and an overview of what this script does? For example, I was initially
> confused about what it diffed against until I realized it actually seems to
> check out the CVS code to diff against. You're also doing some git diff
> operations which imply that you're on a branch and the upstream branch has
some
> specific meaning.
> 
> This script seems to update Hunspell. Naming it update_google_patch implies to
> me that it just generates the diff file. Maybe a different name is in order
> (actually, the existing sentence in the README you modified is similarly
> misleading).

Done. (Two spaces after in - comment discussion moved into replies)

[brettw, 2014-06-02 18:18:07]

lgtm

[groby-ooo-7-16, 2014-06-04 18:41:22]

The CQ bit was checked by groby@chromium.org

[commit-bot: I haz the power, 2014-06-09 11:09:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-09 11:09:54]

Commit queue rejected this change because it did not recognize the base URL.
Please commit your change manually.

[groby-ooo-7-16, 2014-07-25 18:35:53]

Committed patchset #8 manually as r285621 (presubmit successful).

[Nico, 2014-08-02 00:20:36]

I noticed that DEPS wasn't updated after this went in – probably because it
didn't have an effect? In general, it's a good idea to always update DEPS so
that the next person trying to roll can be sure only their change is in the roll
they want to make. (In this case, I'll just roll this in too as it looks
harmless.)