Introduce git merge driver for the blink reformatting

Introduce git merge driver for the blink reformatting

Based on CL from primiano@: https://codereview.chromium.org/2348793003/

This is a simple tool that can help automatically resolve conflicts
from clang-format reformatting patches.

BUG=574611
Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/ff84560ede8fbc53160a760c0284c0234eaeeff1

[dcheng, 2016-09-20 21:41:06]

dcheng@chromium.org changed reviewers:
+ maruel@chromium.org

[dcheng, 2016-09-20 21:41:07]

PS1 is primiano@'s original CL from https://codereview.chromium.org/2348793003/.
PS3 is the latest patch with tweaks, etc. I've tested it on Windows, Mac, and
Linux, and made sure that any custom clang format options that Blink uses are
correctly picked up.

https://codereview.chromium.org/2356933002/diff/1/gclient.py
File gclient.py (right):

https://codereview.chromium.org/2356933002/diff/1/gclient.py#newcode905
gclient.py:905: 'clang_format_merge_driver %O %A %B %L %P'])
%L is the length of the conflict markers, which we don't use anyway. So I just
removed it, since the python script had to ignore this parameter anyway. This is
problematic for people who already have the old config... but hopefully that
will just be thakis, primiano, and me.

https://codereview.chromium.org/2356933002/diff/40001/clang_format_merge_driv...
File clang_format_merge_driver.py (right):

https://codereview.chromium.org/2356933002/diff/40001/clang_format_merge_driv...
clang_format_merge_driver.py:25: with open(fpath, 'rb') as input_file:
Merge always appear to occur relative to the repo root. In order for
clang-format to pick up the .clang-format, we use --assume-filename to fake the
path name. To do this, we also need to use stdin / stdout instead though, so
I've hoisted reading / writing the file into the py as well.

[Nico, 2016-09-20 21:54:26]

Nice! Looks I'm not an owner here, so leaving this for maruel to approve

https://codereview.chromium.org/2356933002/diff/40001/clang_format_merge_driv...
File clang_format_merge_driver.py (right):

https://codereview.chromium.org/2356933002/diff/40001/clang_format_merge_driv...
clang_format_merge_driver.py:5: """Clang-format 3-way merge driver."""
maybe add some more here (what does this do, how does one use it)

https://codereview.chromium.org/2356933002/diff/40001/clang_format_merge_driv...
clang_format_merge_driver.py:25: with open(fpath, 'rb') as input_file:
On 2016/09/20 21:41:06, dcheng wrote:
> Merge always appear to occur relative to the repo root. In order for
> clang-format to pick up the .clang-format, we use --assume-filename to fake
the
> path name. To do this, we also need to use stdin / stdout instead though, so
> I've hoisted reading / writing the file into the py as well.

add a comment summarizing this

https://codereview.chromium.org/2356933002/diff/40001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/2356933002/diff/40001/gclient.py#newcode893
gclient.py:893: CFG_PREFIX = 'merge.clang_format_merge_driver'
I thought I had seen a way to manually pass this to `git rebase`, like `git
rebase --tool=clang-format` but I can't find that flag now that I try to find
it. If there's a flag like that, giving this a shorter name might be nice.

https://codereview.chromium.org/2356933002/diff/40001/gclient.py#newcode906
gclient.py:906: self._used_scm._Capture(['config', CFG_PREFIX + '.recursive',
'binary'])
do we need to set trustExitCode here too, or is that set elsewhere?

[dcheng, 2016-09-20 21:54:39]

Description was changed from

==========
Introduce git merge driver for the blink reformatting

- Introduce a script (clang_format_merge_driver) that can be used as a
  custom merge driver (see "Defining a custom merge driver" section of
  https://git-scm.com/docs/gitattributes)
- Automatically create a merge driver section in the .git/config of the
  main project, as follows:
  [merge "clang_format_merge_driver"]
     name = clang-format merge driver (http://crbug.com/574611)
     driver = clang_format_merge_driver %O %A %B %P
     recursive = binary

Note that this change does not have any impact other than tweaking the
git config of the main project). The custom merge driver will later be
activated by updating .gitattributes with lines like:

*.cpp   merge=clang_format_merge_driver
*.h     merge=clang_format_merge_driver

BUG=574611
==========

to

==========
Introduce git merge driver for the blink reformatting

Based on CL from primiano@: https://codereview.chromium.org/2348793003/

- Introduce a script (clang_format_merge_driver) that can be used as a
  custom merge driver (see "Defining a custom merge driver" section of
  https://git-scm.com/docs/gitattributes)
- Automatically create a merge driver section in the .git/config of the
  main project, as follows:
  [merge "clang_format_merge_driver"]
     name = clang-format merge driver (http://crbug.com/574611)
     driver = clang_format_merge_driver %O %A %B %P
     recursive = binary

Note that this change does not have any impact other than tweaking the
git config of the main project). The custom merge driver will later be
activated by updating .gitattributes with lines like:

*.cpp   merge=clang_format_merge_driver
*.h     merge=clang_format_merge_driver

BUG=574611
==========

[dcheng, 2016-09-20 22:11:19]

https://codereview.chromium.org/2356933002/diff/40001/clang_format_merge_driv...
File clang_format_merge_driver.py (right):

https://codereview.chromium.org/2356933002/diff/40001/clang_format_merge_driv...
clang_format_merge_driver.py:5: """Clang-format 3-way merge driver."""
On 2016/09/20 21:54:26, Nico wrote:
> maybe add some more here (what does this do, how does one use it)

Done.

https://codereview.chromium.org/2356933002/diff/40001/clang_format_merge_driv...
clang_format_merge_driver.py:25: with open(fpath, 'rb') as input_file:
On 2016/09/20 21:54:26, Nico wrote:
> On 2016/09/20 21:41:06, dcheng wrote:
> > Merge always appear to occur relative to the repo root. In order for
> > clang-format to pick up the .clang-format, we use --assume-filename to fake
> the
> > path name. To do this, we also need to use stdin / stdout instead though, so
> > I've hoisted reading / writing the file into the py as well.
> 
> add a comment summarizing this

Done.

https://codereview.chromium.org/2356933002/diff/40001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/2356933002/diff/40001/gclient.py#newcode893
gclient.py:893: CFG_PREFIX = 'merge.clang_format_merge_driver'
On 2016/09/20 21:54:26, Nico wrote:
> I thought I had seen a way to manually pass this to `git rebase`, like `git
> rebase --tool=clang-format` but I can't find that flag now that I try to find
> it. If there's a flag like that, giving this a shorter name might be nice.

I think it's:
git mergetool --tool=clang_format_merge_driver

I apparently come from the Java school of naming though, so I'm having trouble
thinking of a better name that's descriptive here. How does clang_format_merger
sound?

https://codereview.chromium.org/2356933002/diff/40001/gclient.py#newcode906
gclient.py:906: self._used_scm._Capture(['config', CFG_PREFIX + '.recursive',
'binary'])
On 2016/09/20 21:54:26, Nico wrote:
> do we need to set trustExitCode here too, or is that set elsewhere?

I'm not sure, honestly. git is magic. It seems to work automatically already
without it?

[Nico, 2016-09-20 22:26:48]

https://codereview.chromium.org/2356933002/diff/40001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/2356933002/diff/40001/gclient.py#newcode893
gclient.py:893: CFG_PREFIX = 'merge.clang_format_merge_driver'
On 2016/09/20 22:11:19, dcheng wrote:
> On 2016/09/20 21:54:26, Nico wrote:
> > I thought I had seen a way to manually pass this to `git rebase`, like `git
> > rebase --tool=clang-format` but I can't find that flag now that I try to
find
> > it. If there's a flag like that, giving this a shorter name might be nice.
> 
> I think it's:
> git mergetool --tool=clang_format_merge_driver
> 
> I apparently come from the Java school of naming though, so I'm having trouble
> thinking of a better name that's descriptive here. How does
clang_format_merger
> sound?

git mergetool --tool=reformat
git mergetool --tool=clang-format
git mergetool --tool=reformat-merge
git mergetool --tool=clang-format-merge
git mergetool --tool=clang_format_merger

Hm, not sure which one I like best. I'd personally prefer - over _, and I'd err
on the side of brevity -- I'd go with either "reformat" or
"clang-format-merge"...maybe the latter. But in the end it's a bikeshed and
you're writing the CL, so you get to pick the final color :-)

[dcheng, 2016-09-20 22:30:47]

https://codereview.chromium.org/2356933002/diff/40001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/2356933002/diff/40001/gclient.py#newcode893
gclient.py:893: CFG_PREFIX = 'merge.clang_format_merge_driver'
On 2016/09/20 22:26:48, Nico wrote:
> On 2016/09/20 22:11:19, dcheng wrote:
> > On 2016/09/20 21:54:26, Nico wrote:
> > > I thought I had seen a way to manually pass this to `git rebase`, like
`git
> > > rebase --tool=clang-format` but I can't find that flag now that I try to
> find
> > > it. If there's a flag like that, giving this a shorter name might be nice.
> > 
> > I think it's:
> > git mergetool --tool=clang_format_merge_driver
> > 
> > I apparently come from the Java school of naming though, so I'm having
trouble
> > thinking of a better name that's descriptive here. How does
> clang_format_merger
> > sound?
> 
> git mergetool --tool=reformat
> git mergetool --tool=clang-format
> git mergetool --tool=reformat-merge
> git mergetool --tool=clang-format-merge
> git mergetool --tool=clang_format_merger
> 
> Hm, not sure which one I like best. I'd personally prefer - over _, and I'd
err
> on the side of brevity -- I'd go with either "reformat" or
> "clang-format-merge"...maybe the latter. But in the end it's a bikeshed and
> you're writing the CL, so you get to pick the final color :-)

Oh right, we can still keep the script in clang_format_merge_driver.

I like clang-format best, assuming it works (it's short, and it's implicitly a
mergetool, due to where it lives in the config anyway)

[M-A Ruel, 2016-09-20 22:45:29]

maruel@chromium.org changed reviewers:
+ iannucci@chromium.org
- maruel@chromium.org

[M-A Ruel, 2016-09-20 22:45:29]

Deferring to Robbie, I'm not excited about such chromium/src specific logic in
gclient.

[Nico, 2016-09-20 22:52:36]

This is not specific to chromium, just to clang-format, which lives in
depot_tools.

On Sep 20, 2016 6:45 PM, <maruel@chromium.org> wrote:

> Deferring to Robbie, I'm not excited about such chromium/src specific
> logic in
> gclient.
>
> https://codereview.chromium.org/2356933002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2016-09-21 16:50:37]

Ping, I'd like to get this in.

Note that this isn't supposed to be permanent. The idea is that we'll land this,
wait a while for everyone to have this, then reformat blink and set a git attrib
to trigger this for blink merges, then keep this around for a few weeks so that
everyone can rebase their local branches over the reformat with 0 work, and then
we'll remove this again.

[Dirk Pranke, 2016-09-21 17:32:01]

On 2016/09/21 16:50:37, Nico wrote:
> Ping, I'd like to get this in.
> 
> Note that this isn't supposed to be permanent. The idea is that we'll land
this,
> wait a while for everyone to have this, then reformat blink and set a git
attrib
> to trigger this for blink merges, then keep this around for a few weeks so
that
> everyone can rebase their local branches over the reformat with 0 work, and
then
> we'll remove this again.

I'm not sure I understand this. Why do we need this functionality, if we don't
enforce any sort of merge driver settings already? Why isn't having presubmit
checks plus instructions for people that want to set the merge driver good
enough?

[dcheng, 2016-09-21 20:03:28]

On 2016/09/21 17:32:01, Dirk Pranke wrote:
> On 2016/09/21 16:50:37, Nico wrote:
> > Ping, I'd like to get this in.
> > 
> > Note that this isn't supposed to be permanent. The idea is that we'll land
> this,
> > wait a while for everyone to have this, then reformat blink and set a git
> attrib
> > to trigger this for blink merges, then keep this around for a few weeks so
> that
> > everyone can rebase their local branches over the reformat with 0 work, and
> then
> > we'll remove this again.
> 
> I'm not sure I understand this. Why do we need this functionality, if we don't
> enforce any sort of merge driver settings already? Why isn't having presubmit
> checks plus instructions for people that want to set the merge driver good
> enough?

I'm not sure what you mean: do you mean a PRESUBMIT would warn that the merge
tool settings are missing and prompt someone to add them? Or something else?

[iannucci, 2016-09-21 20:11:29]

I think I'm missing a lot of context on this... Why can't this hack be done as
part of chromium's DEPS hooks?

[Nico, 2016-09-21 20:13:44]

I chatted with iannucci, and he made the good point that we can just as well add
a hook to chromiums's DEPS file that adds this to .git/config . That sounds like
a good suggestion to me -- dcheng, wdyt?

[dcheng, 2016-09-21 20:16:33]

On 2016/09/21 20:13:44, Nico wrote:
> I chatted with iannucci, and he made the good point that we can just as well
add
> a hook to chromiums's DEPS file that adds this to .git/config . That sounds
like
> a good suggestion to me -- dcheng, wdyt?

OK, I'll give that a shot. Are there any utilities for working with .git/config,
or should I just be invoking git config <foo> to do what I need?

[iannucci, 2016-09-21 20:17:50]

On 2016/09/21 20:16:33, dcheng wrote:
> On 2016/09/21 20:13:44, Nico wrote:
> > I chatted with iannucci, and he made the good point that we can just as well
> add
> > a hook to chromiums's DEPS file that adds this to .git/config . That sounds
> like
> > a good suggestion to me -- dcheng, wdyt?
> 
> OK, I'll give that a shot. Are there any utilities for working with
.git/config,
> or should I just be invoking git config <foo> to do what I need?

just `git config <foo>` should work on all platforms

[Nico, 2016-09-21 20:18:01]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-09-21 20:18:01]

I would've expected `git config` to do everything.

[iannucci, 2016-09-21 20:19:36]

On 2016/09/21 20:17:50, iannucci wrote:
> On 2016/09/21 20:16:33, dcheng wrote:
> > On 2016/09/21 20:13:44, Nico wrote:
> > > I chatted with iannucci, and he made the good point that we can just as
well
> > add
> > > a hook to chromiums's DEPS file that adds this to .git/config . That
sounds
> > like
> > > a good suggestion to me -- dcheng, wdyt?
> > 
> > OK, I'll give that a shot. Are there any utilities for working with
> .git/config,
> > or should I just be invoking git config <foo> to do what I need?
> 
> just `git config <foo>` should work on all platforms

I would also point out that in general (both this CL and the DEPS hook approach
will also affect bots). I don't think that a merge hook will actually get poked
by anything on the bots, but one potential instance I can think of would be
patches in bot_update; specifically they get put into the git index. Some bots
will occasionally actually commit them as well.

[iannucci, 2016-09-21 20:20:03]

On 2016/09/21 20:19:36, iannucci wrote:
> On 2016/09/21 20:17:50, iannucci wrote:
> > On 2016/09/21 20:16:33, dcheng wrote:
> > > On 2016/09/21 20:13:44, Nico wrote:
> > > > I chatted with iannucci, and he made the good point that we can just as
> well
> > > add
> > > > a hook to chromiums's DEPS file that adds this to .git/config . That
> sounds
> > > like
> > > > a good suggestion to me -- dcheng, wdyt?
> > > 
> > > OK, I'll give that a shot. Are there any utilities for working with
> > .git/config,
> > > or should I just be invoking git config <foo> to do what I need?
> > 
> > just `git config <foo>` should work on all platforms
> 
> I would also point out that in general (both this CL and the DEPS hook
approach
> will also affect bots). I don't think that a merge hook will actually get
poked
> by anything on the bots, but one potential instance I can think of would be
> patches in bot_update; specifically they get put into the git index. Some bots
> will occasionally actually commit them as well.

Er paren fail... "(both this CL and the DEPS hook approach) will also affect
bots"

[dcheng, 2016-09-21 20:23:59]

OK, I'll close this CL and upload a new one using the DEPS approach. Thanks for
the feedback!

[Dirk Pranke, 2016-09-21 21:20:35]

On 2016/09/21 20:03:28, dcheng wrote:
> On 2016/09/21 17:32:01, Dirk Pranke wrote:
> > I'm not sure I understand this. Why do we need this functionality, if we
don't
> > enforce any sort of merge driver settings already? Why isn't having
presubmit
> > checks plus instructions for people that want to set the merge driver good
> > enough?
> 
> I'm not sure what you mean: do you mean a PRESUBMIT would warn that the merge
> tool settings are missing and prompt someone to add them? Or something else?

What I meant was, if we haven't need to set a merge.driver for clang-format 
so far in the project, why do we need it for the Blink reformatting? It seems
like
setting this will have a much broader effect than just reformatting in-flight 
Blink CLs?

[dcheng, 2016-09-21 21:24:56]

On 2016/09/21 21:20:35, Dirk Pranke wrote:
> On 2016/09/21 20:03:28, dcheng wrote:
> > On 2016/09/21 17:32:01, Dirk Pranke wrote:
> > > I'm not sure I understand this. Why do we need this functionality, if we
> don't
> > > enforce any sort of merge driver settings already? Why isn't having
> presubmit
> > > checks plus instructions for people that want to set the merge driver good
> > > enough?
> > 
> > I'm not sure what you mean: do you mean a PRESUBMIT would warn that the
merge
> > tool settings are missing and prompt someone to add them? Or something else?
> 
> What I meant was, if we haven't need to set a merge.driver for clang-format 
> so far in the project, why do we need it for the Blink reformatting? It seems
> like
> setting this will have a much broader effect than just reformatting in-flight 
> Blink CLs?

The Blink coding style differs fairly significantly from the Google coding
style. We'd like to make the coding styles consistent, and so we'd like to
clang-format Blink with Chromium style. To avoid rather tricky merge conflict
resolutions for every in-flight patch, we'd like to automate it to make the
process much smoother. The merge tool will only be activated for use in Blink,
so it won't affect any other CLs anyway.

[Dirk Pranke, 2016-09-21 21:31:40]

On 2016/09/21 21:24:56, dcheng wrote:
> On 2016/09/21 21:20:35, Dirk Pranke wrote:
> > On 2016/09/21 20:03:28, dcheng wrote:
> > > On 2016/09/21 17:32:01, Dirk Pranke wrote:
> > > > I'm not sure I understand this. Why do we need this functionality, if we
> > don't
> > > > enforce any sort of merge driver settings already? Why isn't having
> > presubmit
> > > > checks plus instructions for people that want to set the merge driver
good
> > > > enough?
> > > 
> > > I'm not sure what you mean: do you mean a PRESUBMIT would warn that the
> merge
> > > tool settings are missing and prompt someone to add them? Or something
else?
> > 
> > What I meant was, if we haven't need to set a merge.driver for clang-format 
> > so far in the project, why do we need it for the Blink reformatting? It
seems
> > like
> > setting this will have a much broader effect than just reformatting
in-flight 
> > Blink CLs?
> 
> The Blink coding style differs fairly significantly from the Google coding
> style. We'd like to make the coding styles consistent, and so we'd like to
> clang-format Blink with Chromium style. To avoid rather tricky merge conflict
> resolutions for every in-flight patch, we'd like to automate it to make the
> process much smoother. The merge tool will only be activated for use in Blink,
> so it won't affect any other CLs anyway.

What would cause this to only be activated for use in Blink?

[dcheng, 2016-09-21 21:43:47]

On 2016/09/21 21:31:40, Dirk Pranke wrote:
> On 2016/09/21 21:24:56, dcheng wrote:
> > On 2016/09/21 21:20:35, Dirk Pranke wrote:
> > > On 2016/09/21 20:03:28, dcheng wrote:
> > > > On 2016/09/21 17:32:01, Dirk Pranke wrote:
> > > > > I'm not sure I understand this. Why do we need this functionality, if
we
> > > don't
> > > > > enforce any sort of merge driver settings already? Why isn't having
> > > presubmit
> > > > > checks plus instructions for people that want to set the merge driver
> good
> > > > > enough?
> > > > 
> > > > I'm not sure what you mean: do you mean a PRESUBMIT would warn that the
> > merge
> > > > tool settings are missing and prompt someone to add them? Or something
> else?
> > > 
> > > What I meant was, if we haven't need to set a merge.driver for
clang-format 
> > > so far in the project, why do we need it for the Blink reformatting? It
> seems
> > > like
> > > setting this will have a much broader effect than just reformatting
> in-flight 
> > > Blink CLs?
> > 
> > The Blink coding style differs fairly significantly from the Google coding
> > style. We'd like to make the coding styles consistent, and so we'd like to
> > clang-format Blink with Chromium style. To avoid rather tricky merge
conflict
> > resolutions for every in-flight patch, we'd like to automate it to make the
> > process much smoother. The merge tool will only be activated for use in
Blink,
> > so it won't affect any other CLs anyway.
> 
> What would cause this to only be activated for use in Blink?

It's not automatically used unless you instruct git to do so. We would do so by
adding lines to .gitattributes in third_party/WebKit, so the merge tool is only
automatically used for source files in Blink.

[dcheng, 2016-09-21 21:44:01]

On 2016/09/21 21:43:47, dcheng wrote:
> On 2016/09/21 21:31:40, Dirk Pranke wrote:
> > On 2016/09/21 21:24:56, dcheng wrote:
> > > On 2016/09/21 21:20:35, Dirk Pranke wrote:
> > > > On 2016/09/21 20:03:28, dcheng wrote:
> > > > > On 2016/09/21 17:32:01, Dirk Pranke wrote:
> > > > > > I'm not sure I understand this. Why do we need this functionality,
if
> we
> > > > don't
> > > > > > enforce any sort of merge driver settings already? Why isn't having
> > > > presubmit
> > > > > > checks plus instructions for people that want to set the merge
driver
> > good
> > > > > > enough?
> > > > > 
> > > > > I'm not sure what you mean: do you mean a PRESUBMIT would warn that
the
> > > merge
> > > > > tool settings are missing and prompt someone to add them? Or something
> > else?
> > > > 
> > > > What I meant was, if we haven't need to set a merge.driver for
> clang-format 
> > > > so far in the project, why do we need it for the Blink reformatting? It
> > seems
> > > > like
> > > > setting this will have a much broader effect than just reformatting
> > in-flight 
> > > > Blink CLs?
> > > 
> > > The Blink coding style differs fairly significantly from the Google coding
> > > style. We'd like to make the coding styles consistent, and so we'd like to
> > > clang-format Blink with Chromium style. To avoid rather tricky merge
> conflict
> > > resolutions for every in-flight patch, we'd like to automate it to make
the
> > > process much smoother. The merge tool will only be activated for use in
> Blink,
> > > so it won't affect any other CLs anyway.
> > 
> > What would cause this to only be activated for use in Blink?
> 
> It's not automatically used unless you instruct git to do so. We would do so
by
> adding lines to .gitattributes in third_party/WebKit, so the merge tool is
only
> automatically used for source files in Blink.

(The CL description has the specifics of what this would look like)

[Dirk Pranke, 2016-09-21 21:45:26]

Ah, right. I forgot about the .gitattributes part. Thanks.

[dcheng, 2016-09-22 07:50:55]

Description was changed from

==========
Introduce git merge driver for the blink reformatting

Based on CL from primiano@: https://codereview.chromium.org/2348793003/

- Introduce a script (clang_format_merge_driver) that can be used as a
  custom merge driver (see "Defining a custom merge driver" section of
  https://git-scm.com/docs/gitattributes)
- Automatically create a merge driver section in the .git/config of the
  main project, as follows:
  [merge "clang_format_merge_driver"]
     name = clang-format merge driver (http://crbug.com/574611)
     driver = clang_format_merge_driver %O %A %B %P
     recursive = binary

Note that this change does not have any impact other than tweaking the
git config of the main project). The custom merge driver will later be
activated by updating .gitattributes with lines like:

*.cpp   merge=clang_format_merge_driver
*.h     merge=clang_format_merge_driver

BUG=574611
==========

to

==========
Introduce git merge driver for the blink reformatting

Based on CL from primiano@: https://codereview.chromium.org/2348793003/

This is a simple tool that can help automatically resolve conflicts
from clang-format reformatting patches.

BUG=574611
==========

[dcheng, 2016-09-22 07:53:26]

I removed the gclient.py hook and moved it into chromium's DEPS:
https://codereview.chromium.org/2359933005/

Note that I'm still leaving this CL open: since the driver uses the clang_format
helper in depot_tools to find the correct clang-format binary, it seems easier
to keep it here? And now it's now Chromium-specific at all.

[Dirk Pranke, 2016-09-22 18:19:29]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dirk Pranke, 2016-09-22 18:19:30]

lgtm to add since clang-format is already here.

[M-A Ruel, 2016-09-22 18:49:34]

maruel@chromium.org changed reviewers:
+ maruel@chromium.org

[M-A Ruel, 2016-09-22 18:49:35]

rs lgtm

[dcheng, 2016-09-23 07:52:11]

As an update, I'm trying to investigate the feasibility of placing this in
chrome's tree directly: that gets rid of the "must run gclient at least once
before the merges" problem.

[Primiano Tucci (use gerrit), 2016-09-23 09:05:15]

On 2016/09/23 07:52:11, dcheng wrote:
> As an update, I'm trying to investigate the feasibility of placing this in
> chrome's tree directly: that gets rid of the "must run gclient at least once
> before the merges" problem.

I think this is going to be hard.
Definitely using a hook in DEPS is less hacky. But both with my approach and
DEPS you can't drop the requirement of running gclient at least once.
Essentially you want the driver to be installed at the time developers do a "git
pull". There is nothing that comes to my mind which can let you install that.
So, yeah, the risk here is that if somebody doesn't run gclient sync for a week
(or whatever time between the DEPS hook and the actual reformat) they will be
busted.
Would be great if we can find some other magic, but nothing that comes to my
mind.

Perhaps this clang_format_merge_driver should have a --install method, and we
should send a PSA before the refactor saying:
"just in case, make sure you run clang_format_merge_driver --install if you have
patches on the flight in blink. It should be already installed, but if you don't
sync often, you should do that manually"

[dcheng, 2016-09-23 15:24:38]

On 2016/09/23 09:05:15, Primiano Tucci wrote:
> On 2016/09/23 07:52:11, dcheng wrote:
> > As an update, I'm trying to investigate the feasibility of placing this in
> > chrome's tree directly: that gets rid of the "must run gclient at least once
> > before the merges" problem.
> 
> I think this is going to be hard.
> Definitely using a hook in DEPS is less hacky. But both with my approach and
> DEPS you can't drop the requirement of running gclient at least once.
> Essentially you want the driver to be installed at the time developers do a
"git
> pull". There is nothing that comes to my mind which can let you install that.
> So, yeah, the risk here is that if somebody doesn't run gclient sync for a
week
> (or whatever time between the DEPS hook and the actual reformat) they will be
> busted.
> Would be great if we can find some other magic, but nothing that comes to my
> mind.
> 
> Perhaps this clang_format_merge_driver should have a --install method, and we
> should send a PSA before the refactor saying:
> "just in case, make sure you run clang_format_merge_driver --install if you
have
> patches on the flight in blink. It should be already installed, but if you
don't
> sync often, you should do that manually"

OK, yeah, you're right. I'll just do a final test to make sure I didn't break
anything with my (theoretically) comment-only changes, and land as-is.

[dcheng, 2016-09-23 15:25:20]

On 2016/09/23 15:24:38, dcheng wrote:
> On 2016/09/23 09:05:15, Primiano Tucci wrote:
> > On 2016/09/23 07:52:11, dcheng wrote:
> > > As an update, I'm trying to investigate the feasibility of placing this in
> > > chrome's tree directly: that gets rid of the "must run gclient at least
once
> > > before the merges" problem.
> > 
> > I think this is going to be hard.
> > Definitely using a hook in DEPS is less hacky. But both with my approach and
> > DEPS you can't drop the requirement of running gclient at least once.
> > Essentially you want the driver to be installed at the time developers do a
> "git
> > pull". There is nothing that comes to my mind which can let you install
that.
> > So, yeah, the risk here is that if somebody doesn't run gclient sync for a
> week
> > (or whatever time between the DEPS hook and the actual reformat) they will
be
> > busted.
> > Would be great if we can find some other magic, but nothing that comes to my
> > mind.
> > 
> > Perhaps this clang_format_merge_driver should have a --install method, and
we
> > should send a PSA before the refactor saying:
> > "just in case, make sure you run clang_format_merge_driver --install if you
> have
> > patches on the flight in blink. It should be already installed, but if you
> don't
> > sync often, you should do that manually"
> 
> OK, yeah, you're right. I'll just do a final test to make sure I didn't break
> anything with my (theoretically) comment-only changes, and land as-is.

(I don't think we need a specific install method though, we can just ask people
to run tools/clang_format_merge_driver/install_git_hook.py, right?)

[dcheng, 2016-09-23 18:49:53]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-09-23 18:50:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 18:56:22]

Description was changed from

==========
Introduce git merge driver for the blink reformatting

Based on CL from primiano@: https://codereview.chromium.org/2348793003/

This is a simple tool that can help automatically resolve conflicts
from clang-format reformatting patches.

BUG=574611
==========

to

==========
Introduce git merge driver for the blink reformatting

Based on CL from primiano@: https://codereview.chromium.org/2348793003/

This is a simple tool that can help automatically resolve conflicts
from clang-format reformatting patches.

BUG=574611

Committed:
https://chromium.googlesource.com/chromium/tools/depot_tools/+/ff84560ede8fbc...
==========

[commit-bot: I haz the power, 2016-09-23 18:56:23]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/tools/depot_tools/+/ff84560ede8fbc...