git-hooks/pre-commit: Allow merging deps2submodules changes from upstream

git-hooks/pre-commit: Allow merging deps2submodules changes from upstream

This also checks the 2nd parent during a merge, to allow commiting a
merge that contains newer versions of the submodules. In particular
merging chromium master into a local branch will now work.

TEST=git checkout -b test origin/master~200
  git merge --no-ff --no-commit origin/master
  git commit

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259711

[szager1, 2014-03-17 05:18:10]

I'd prefer to do this slightly differently; if a merge is in progress, I'd like
the hook to verify that the index doesn't have any submodule or .gitmodules
changes other than what is in MERGE_HEAD.  I think you can do this:

gitdir=$(git rev-parse --git-dir)
if [ -e "${gitdir}/MERGE_HEAD" ]; then
  head_ref=MERGE_HEAD
else
  head_ref=HEAD
fi

... and then s/HEAD/\$head_ref/g in the rest of the script.

[szager1, 2014-03-17 05:18:37]

I'd prefer to do this slightly differently; if a merge is in progress, I'd like
the hook to verify that the index doesn't have any submodule or .gitmodules
changes other than what is in MERGE_HEAD.  I think you can do this:

gitdir=$(git rev-parse --git-dir)
if [ -e "${gitdir}/MERGE_HEAD" ]; then
  head_ref=MERGE_HEAD
else
  head_ref=HEAD
fi

... and then s/HEAD/\$head_ref/g in the rest of the script.

[spang, 2014-03-17 05:51:26]

On 2014/03/17 05:18:37, szager1 wrote:
> I'd prefer to do this slightly differently; if a merge is in progress, I'd
like
> the hook to verify that the index doesn't have any submodule or .gitmodules
> changes other than what is in MERGE_HEAD.  I think you can do this:
> 
> gitdir=$(git rev-parse --git-dir)
> if [ -e "${gitdir}/MERGE_HEAD" ]; then
>   head_ref=MERGE_HEAD
> else
>   head_ref=HEAD
> fi
> 
> ... and then s/HEAD/\$head_ref/g in the rest of the script.

That breaks the symmetric merge with the two parents swapped. That is, you won't
be able to do:

  git checkout -b master origin/master
  git merge topic-branch

If we want more accuracy than I have in this CL, then it's not too bad to work
out which parent contains a more recent copy of ToT, but I'm not sure it's
really necessary.

[szager1, 2014-03-17 06:27:33]

On 2014/03/17 05:51:26, spang wrote:
> On 2014/03/17 05:18:37, szager1 wrote:
> > I'd prefer to do this slightly differently; if a merge is in progress, I'd
> like
> > the hook to verify that the index doesn't have any submodule or .gitmodules
> > changes other than what is in MERGE_HEAD.  I think you can do this:
> > 
> > gitdir=$(git rev-parse --git-dir)
> > if [ -e "${gitdir}/MERGE_HEAD" ]; then
> >   head_ref=MERGE_HEAD
> > else
> >   head_ref=HEAD
> > fi
> > 
> > ... and then s/HEAD/\$head_ref/g in the rest of the script.
> 
> That breaks the symmetric merge with the two parents swapped. That is, you
won't
> be able to do:
> 
>   git checkout -b master origin/master
>   git merge topic-branch

True, but that is a very uncommon operation.  I think it's worth enforcing this
for the much more common case.

[szager1, 2014-03-17 06:34:20]

On 2014/03/17 05:51:26, spang wrote:
> On 2014/03/17 05:18:37, szager1 wrote:
> > I'd prefer to do this slightly differently; if a merge is in progress, I'd
> like
> > the hook to verify that the index doesn't have any submodule or .gitmodules
> > changes other than what is in MERGE_HEAD.  I think you can do this:
> > 
> > gitdir=$(git rev-parse --git-dir)
> > if [ -e "${gitdir}/MERGE_HEAD" ]; then
> >   head_ref=MERGE_HEAD
> > else
> >   head_ref=HEAD
> > fi
> > 
> > ... and then s/HEAD/\$head_ref/g in the rest of the script.
> 
> That breaks the symmetric merge with the two parents swapped. That is, you
won't
> be able to do:
> 
>   git checkout -b master origin/master
>   git merge topic-branch
> 
> If we want more accuracy than I have in this CL, then it's not too bad to work
> out which parent contains a more recent copy of ToT, but I'm not sure it's
> really necessary.

I guess if you really want to be pedantic, you could do this:

if [[ -e "${git_dir}/MERGE_HEAD" && -n $(git branch -r --contains MERGE_HEAD
origin/master) ]]

... but that, of course, assumes that your remote is named "origin", and you're
working off branch "master".

[spang, 2014-03-17 17:06:43]

On 2014/03/17 06:27:33, szager1 wrote:
> On 2014/03/17 05:51:26, spang wrote:
> > On 2014/03/17 05:18:37, szager1 wrote:
> > > I'd prefer to do this slightly differently; if a merge is in progress, I'd
> > like
> > > the hook to verify that the index doesn't have any submodule or
.gitmodules
> > > changes other than what is in MERGE_HEAD.  I think you can do this:
> > > 
> > > gitdir=$(git rev-parse --git-dir)
> > > if [ -e "${gitdir}/MERGE_HEAD" ]; then
> > >   head_ref=MERGE_HEAD
> > > else
> > >   head_ref=HEAD
> > > fi
> > > 
> > > ... and then s/HEAD/\$head_ref/g in the rest of the script.
> > 
> > That breaks the symmetric merge with the two parents swapped. That is, you
> won't
> > be able to do:
> > 
> >   git checkout -b master origin/master
> >   git merge topic-branch
> 
> True, but that is a very uncommon operation.  I think it's worth enforcing
this
> for the much more common case.

I definitely do this type of merge sometimes. Also, swapping the parents was one
of the workflows mentioned as a workaround to the current behavior.

[spang, 2014-03-17 17:12:35]

On 2014/03/17 06:34:20, szager1 wrote:
> On 2014/03/17 05:51:26, spang wrote:
> > On 2014/03/17 05:18:37, szager1 wrote:
> > > I'd prefer to do this slightly differently; if a merge is in progress, I'd
> > like
> > > the hook to verify that the index doesn't have any submodule or
.gitmodules
> > > changes other than what is in MERGE_HEAD.  I think you can do this:
> > > 
> > > gitdir=$(git rev-parse --git-dir)
> > > if [ -e "${gitdir}/MERGE_HEAD" ]; then
> > >   head_ref=MERGE_HEAD
> > > else
> > >   head_ref=HEAD
> > > fi
> > > 
> > > ... and then s/HEAD/\$head_ref/g in the rest of the script.
> > 
> > That breaks the symmetric merge with the two parents swapped. That is, you
> won't
> > be able to do:
> > 
> >   git checkout -b master origin/master
> >   git merge topic-branch
> > 
> > If we want more accuracy than I have in this CL, then it's not too bad to
work
> > out which parent contains a more recent copy of ToT, but I'm not sure it's
> > really necessary.
> 
> I guess if you really want to be pedantic, you could do this:
> 
> if [[ -e "${git_dir}/MERGE_HEAD" && -n $(git branch -r --contains MERGE_HEAD
> origin/master) ]]

This should work without relying on the remote origin being up-to-date:

merge_base=$(git merge-base HEAD MERGE_HEAD)

if test "$(submodules_diff $merge_base HEAD)"; then
  head_ref=HEAD
else
  head_ref=MERGE_HEAD
fi

> 
> ... but that, of course, assumes that your remote is named "origin", and
you're
> working off branch "master".

[spang, 2014-03-17 17:42:35]

On 2014/03/17 17:12:35, spang wrote:
> On 2014/03/17 06:34:20, szager1 wrote:
> > On 2014/03/17 05:51:26, spang wrote:
> > > On 2014/03/17 05:18:37, szager1 wrote:
> > > > I'd prefer to do this slightly differently; if a merge is in progress,
I'd
> > > like
> > > > the hook to verify that the index doesn't have any submodule or
> .gitmodules
> > > > changes other than what is in MERGE_HEAD.  I think you can do this:
> > > > 
> > > > gitdir=$(git rev-parse --git-dir)
> > > > if [ -e "${gitdir}/MERGE_HEAD" ]; then
> > > >   head_ref=MERGE_HEAD
> > > > else
> > > >   head_ref=HEAD
> > > > fi
> > > > 
> > > > ... and then s/HEAD/\$head_ref/g in the rest of the script.
> > > 
> > > That breaks the symmetric merge with the two parents swapped. That is, you
> > won't
> > > be able to do:
> > > 
> > >   git checkout -b master origin/master
> > >   git merge topic-branch
> > > 
> > > If we want more accuracy than I have in this CL, then it's not too bad to
> work
> > > out which parent contains a more recent copy of ToT, but I'm not sure it's
> > > really necessary.
> > 
> > I guess if you really want to be pedantic, you could do this:
> > 
> > if [[ -e "${git_dir}/MERGE_HEAD" && -n $(git branch -r --contains MERGE_HEAD
> > origin/master) ]]
> 
> This should work without relying on the remote origin being up-to-date:
> 
> merge_base=$(git merge-base HEAD MERGE_HEAD)
> 
> if test "$(submodules_diff $merge_base HEAD)"; then
>   head_ref=HEAD
> else
>   head_ref=MERGE_HEAD
> fi

I've updated the patch with this strategy & tested several cases:

No merge:
- No submodule changes (pass)
- Submodule changes (fail)

Merge:
- HEAD is newer, correct merge (pass)
- MERGE_HEAD is newer, correct merge (pass)
- HEAD is newer, mismerged submodules (fail)
- MERGE_HEAD is newer, mismerged submodules (fail)




> 
> > 
> > ... but that, of course, assumes that your remote is named "origin", and
> you're
> > working off branch "master".

[szager1, 2014-03-17 18:12:02]

This doesn't address the case that has been by far the biggest source of errors:
developer on a local branch uses 'git commit -a', which includes a submodule
change because (1) the developer was testing a different version of a submodule,
and (2) their submodule.<name>.ignore config setting was set incorrectly.

As a practical matter, this hook only has value if it catches that case.

[spang, 2014-03-17 18:27:26]

On 2014/03/17 18:12:02, szager1 wrote:
> This doesn't address the case that has been by far the biggest source of
errors:
> developer on a local branch uses 'git commit -a', which includes a submodule
> change because (1) the developer was testing a different version of a
submodule,
> and (2) their submodule.<name>.ignore config setting was set incorrectly.
> 
> As a practical matter, this hook only has value if it catches that case.

I think this conclusion is wrong. I did test that the hook catches accidental
changes to submodules such as the above "git commit -a" scenario.

If you can provide a concrete example, that would be helpful.

[szager1, 2014-03-17 18:37:19]

On 2014/03/17 18:27:26, spang wrote:
> On 2014/03/17 18:12:02, szager1 wrote:
> > This doesn't address the case that has been by far the biggest source of
> errors:
> > developer on a local branch uses 'git commit -a', which includes a submodule
> > change because (1) the developer was testing a different version of a
> submodule,
> > and (2) their submodule.<name>.ignore config setting was set incorrectly.
> > 
> > As a practical matter, this hook only has value if it catches that case.
> 
> I think this conclusion is wrong. I did test that the hook catches accidental
> changes to submodules such as the above "git commit -a" scenario.
> 
> If you can provide a concrete example, that would be helpful.

It's exactly the case you mentioned before:

git checkout -b mybranch origin/master
# Make changes, including an inadvertent submodule change
git checkout origin/master
git merge mybranch

[spang, 2014-03-17 18:46:29]

On 2014/03/17 18:37:19, szager1 wrote:
> On 2014/03/17 18:27:26, spang wrote:
> > On 2014/03/17 18:12:02, szager1 wrote:
> > > This doesn't address the case that has been by far the biggest source of
> > errors:
> > > developer on a local branch uses 'git commit -a', which includes a
submodule
> > > change because (1) the developer was testing a different version of a
> > submodule,
> > > and (2) their submodule.<name>.ignore config setting was set incorrectly.
> > > 
> > > As a practical matter, this hook only has value if it catches that case.
> > 
> > I think this conclusion is wrong. I did test that the hook catches
accidental
> > changes to submodules such as the above "git commit -a" scenario.
> > 
> > If you can provide a concrete example, that would be helpful.
> 
> It's exactly the case you mentioned before:
> 
> git checkout -b mybranch origin/master
> # Make changes, including an inadvertent submodule change
> git checkout origin/master
> git merge mybranch

In the above scenario, we'll end up doing

  submodule_diff HEAD

which will contain the accidental edits, and the nonempty diff will fail the
hook.

You must merge correctly for the hook to succeed.

[szager1, 2014-03-17 18:56:28]

On 2014/03/17 18:46:29, spang wrote:
> On 2014/03/17 18:37:19, szager1 wrote:

> > git checkout -b mybranch origin/master
> > # Make changes, including an inadvertent submodule change
> > git checkout origin/master
> > git merge mybranch
> 
> In the above scenario, we'll end up doing
> 
>   submodule_diff HEAD
> 
> which will contain the accidental edits, and the nonempty diff will fail the
> hook.
> 
> You must merge correctly for the hook to succeed.

I don't follow you.  As I read it:


# This is 'true', because there is a merge in progress
 11 if git rev-parse --verify --quiet --no-revs MERGE_HEAD; then

# merge_base is something in the history of origin/master
 12   merge_base=$(git merge-base HEAD MERGE_HEAD)

# This is true if there are submodule commits between merge_base and
origin/master
 13   if test -z "$(submodule_diff $merge_base HEAD)"; then
 14     # Most up-to-date submodules are in MERGE_HEAD.
 15     head_ref=MERGE_HEAD
# ...

# This evaluates to 'submodule_diff MERGE_HEAD'
 25 submods=$(submodule_diff $head_ref)

[spang, 2014-03-17 19:03:51]

On 2014/03/17 18:56:28, szager1 wrote:
> On 2014/03/17 18:46:29, spang wrote:
> > On 2014/03/17 18:37:19, szager1 wrote:
> 
> > > git checkout -b mybranch origin/master
> > > # Make changes, including an inadvertent submodule change
> > > git checkout origin/master
> > > git merge mybranch
> > 
> > In the above scenario, we'll end up doing
> > 
> >   submodule_diff HEAD
> > 
> > which will contain the accidental edits, and the nonempty diff will fail the
> > hook.
> > 
> > You must merge correctly for the hook to succeed.
> 
> I don't follow you.  As I read it:
> 
> 
> # This is 'true', because there is a merge in progress
>  11 if git rev-parse --verify --quiet --no-revs MERGE_HEAD; then
> 
> # merge_base is something in the history of origin/master
>  12   merge_base=$(git merge-base HEAD MERGE_HEAD)
> 
> # This is true if there are submodule commits between merge_base and
> origin/master

It is true if there are NO submodule commits between merge_base and HEAD.

If there aren't any submodules changes in HEAD, then we diff vs MERGE_BASE.

If there are submodule changes in HEAD, then we diff vs HEAD.

Maybe I have written this confusingly by using "-z" here. I can reverse the
if/else if that's clearer.

>  13   if test -z "$(submodule_diff $merge_base HEAD)"; then
>  14     # Most up-to-date submodules are in MERGE_HEAD.
>  15     head_ref=MERGE_HEAD
> # ...
> 
> # This evaluates to 'submodule_diff MERGE_HEAD'
>  25 submods=$(submodule_diff $head_ref)

[szager1, 2014-03-17 19:49:49]

On 2014/03/17 19:03:51, spang wrote:
> On 2014/03/17 18:56:28, szager1 wrote:
> > On 2014/03/17 18:46:29, spang wrote:
> > > On 2014/03/17 18:37:19, szager1 wrote:
> > 
> > > > git checkout -b mybranch origin/master
> > > > # Make changes, including an inadvertent submodule change
> > > > git checkout origin/master
> > > > git merge mybranch
> > > 
> > > In the above scenario, we'll end up doing
> > > 
> > >   submodule_diff HEAD
> > > 
> > > which will contain the accidental edits, and the nonempty diff will fail
the
> > > hook.
> > > 
> > > You must merge correctly for the hook to succeed.
> > 
> > I don't follow you.  As I read it:
> > 
> > 
> > # This is 'true', because there is a merge in progress
> >  11 if git rev-parse --verify --quiet --no-revs MERGE_HEAD; then
> > 
> > # merge_base is something in the history of origin/master
> >  12   merge_base=$(git merge-base HEAD MERGE_HEAD)
> > 
> > # This is true if there are submodule commits between merge_base and
> > origin/master
> 
> It is true if there are NO submodule commits between merge_base and HEAD.
> 
> If there aren't any submodules changes in HEAD, then we diff vs MERGE_BASE.
> 
> If there are submodule changes in HEAD, then we diff vs HEAD.
> 
> Maybe I have written this confusingly by using "-z" here. I can reverse the
> if/else if that's clearer.

Ah, right.  OK, then what about this sequence:

git checkout -b mybranch origin/master
# Accidentally commit a submodule change
git fetch origin  # No submodule changes fetched
git checkout origin/master
git merge mybranch

[spang, 2014-03-17 21:10:15]

On 2014/03/17 19:49:49, szager1 wrote:
> On 2014/03/17 19:03:51, spang wrote:
> > On 2014/03/17 18:56:28, szager1 wrote:
> > > On 2014/03/17 18:46:29, spang wrote:
> > > > On 2014/03/17 18:37:19, szager1 wrote:
> > > 
> > > > > git checkout -b mybranch origin/master
> > > > > # Make changes, including an inadvertent submodule change
> > > > > git checkout origin/master
> > > > > git merge mybranch
> > > > 
> > > > In the above scenario, we'll end up doing
> > > > 
> > > >   submodule_diff HEAD
> > > > 
> > > > which will contain the accidental edits, and the nonempty diff will fail
> the
> > > > hook.
> > > > 
> > > > You must merge correctly for the hook to succeed.
> > > 
> > > I don't follow you.  As I read it:
> > > 
> > > 
> > > # This is 'true', because there is a merge in progress
> > >  11 if git rev-parse --verify --quiet --no-revs MERGE_HEAD; then
> > > 
> > > # merge_base is something in the history of origin/master
> > >  12   merge_base=$(git merge-base HEAD MERGE_HEAD)
> > > 
> > > # This is true if there are submodule commits between merge_base and
> > > origin/master
> > 
> > It is true if there are NO submodule commits between merge_base and HEAD.
> > 
> > If there aren't any submodules changes in HEAD, then we diff vs MERGE_BASE.
> > 
> > If there are submodule changes in HEAD, then we diff vs HEAD.
> > 
> > Maybe I have written this confusingly by using "-z" here. I can reverse the
> > if/else if that's clearer.
> 
> Ah, right.  OK, then what about this sequence:
> 
> git checkout -b mybranch origin/master
> # Accidentally commit a submodule change

You can't commit it since this fails the no-merge case.

> git fetch origin  # No submodule changes fetched
> git checkout origin/master
> git merge mybranch

[spang, 2014-03-21 18:34:07]

On 2014/03/17 21:10:15, spang wrote:
> On 2014/03/17 19:49:49, szager1 wrote:
> > On 2014/03/17 19:03:51, spang wrote:
> > > On 2014/03/17 18:56:28, szager1 wrote:
> > > > On 2014/03/17 18:46:29, spang wrote:
> > > > > On 2014/03/17 18:37:19, szager1 wrote:
> > > > 
> > > > > > git checkout -b mybranch origin/master
> > > > > > # Make changes, including an inadvertent submodule change
> > > > > > git checkout origin/master
> > > > > > git merge mybranch
> > > > > 
> > > > > In the above scenario, we'll end up doing
> > > > > 
> > > > >   submodule_diff HEAD
> > > > > 
> > > > > which will contain the accidental edits, and the nonempty diff will
fail
> > the
> > > > > hook.
> > > > > 
> > > > > You must merge correctly for the hook to succeed.
> > > > 
> > > > I don't follow you.  As I read it:
> > > > 
> > > > 
> > > > # This is 'true', because there is a merge in progress
> > > >  11 if git rev-parse --verify --quiet --no-revs MERGE_HEAD; then
> > > > 
> > > > # merge_base is something in the history of origin/master
> > > >  12   merge_base=$(git merge-base HEAD MERGE_HEAD)
> > > > 
> > > > # This is true if there are submodule commits between merge_base and
> > > > origin/master
> > > 
> > > It is true if there are NO submodule commits between merge_base and HEAD.
> > > 
> > > If there aren't any submodules changes in HEAD, then we diff vs
MERGE_BASE.
> > > 
> > > If there are submodule changes in HEAD, then we diff vs HEAD.
> > > 
> > > Maybe I have written this confusingly by using "-z" here. I can reverse
the
> > > if/else if that's clearer.
> > 
> > Ah, right.  OK, then what about this sequence:
> > 
> > git checkout -b mybranch origin/master
> > # Accidentally commit a submodule change
> 
> You can't commit it since this fails the no-merge case.
> 
> > git fetch origin  # No submodule changes fetched
> > git checkout origin/master
> > git merge mybranch

ping

[Rick Byers, 2014-03-26 15:59:40]

On 2014/03/21 18:34:07, spang wrote:
> On 2014/03/17 21:10:15, spang wrote:
> > On 2014/03/17 19:49:49, szager1 wrote:
> > > On 2014/03/17 19:03:51, spang wrote:
> > > > On 2014/03/17 18:56:28, szager1 wrote:
> > > > > On 2014/03/17 18:46:29, spang wrote:
> > > > > > On 2014/03/17 18:37:19, szager1 wrote:
> > > > > 
> > > > > > > git checkout -b mybranch origin/master
> > > > > > > # Make changes, including an inadvertent submodule change
> > > > > > > git checkout origin/master
> > > > > > > git merge mybranch
> > > > > > 
> > > > > > In the above scenario, we'll end up doing
> > > > > > 
> > > > > >   submodule_diff HEAD
> > > > > > 
> > > > > > which will contain the accidental edits, and the nonempty diff will
> fail
> > > the
> > > > > > hook.
> > > > > > 
> > > > > > You must merge correctly for the hook to succeed.
> > > > > 
> > > > > I don't follow you.  As I read it:
> > > > > 
> > > > > 
> > > > > # This is 'true', because there is a merge in progress
> > > > >  11 if git rev-parse --verify --quiet --no-revs MERGE_HEAD; then
> > > > > 
> > > > > # merge_base is something in the history of origin/master
> > > > >  12   merge_base=$(git merge-base HEAD MERGE_HEAD)
> > > > > 
> > > > > # This is true if there are submodule commits between merge_base and
> > > > > origin/master
> > > > 
> > > > It is true if there are NO submodule commits between merge_base and
HEAD.
> > > > 
> > > > If there aren't any submodules changes in HEAD, then we diff vs
> MERGE_BASE.
> > > > 
> > > > If there are submodule changes in HEAD, then we diff vs HEAD.
> > > > 
> > > > Maybe I have written this confusingly by using "-z" here. I can reverse
> the
> > > > if/else if that's clearer.
> > > 
> > > Ah, right.  OK, then what about this sequence:
> > > 
> > > git checkout -b mybranch origin/master
> > > # Accidentally commit a submodule change
> > 
> > You can't commit it since this fails the no-merge case.
> > 
> > > git fetch origin  # No submodule changes fetched
> > > git checkout origin/master
> > > git merge mybranch
> 
> ping

Thanks for pushing on this Michael!  I agree we should be able to use a merge
workflow (and certainly it shouldn't fail in this totally confusing way).

[szager1, 2014-03-26 16:06:51]

Sorry for the delay; I have been interrupt-driven lately.

I still find the logic a bit squirrely, particularly where it relies on
"correct" behavior when the previous commit happened.

Having said that: the submodule flow is going to disappear soon enough, so
there's probably no harm in landing this now.

lgtm

[spang, 2014-03-26 17:33:15]

The CQ bit was checked by spang@chromium.org

[commit-bot: I haz the power, 2014-03-26 17:34:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/spang@chromium.org/192443006/20001

[commit-bot: I haz the power, 2014-03-26 17:34:54]

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

[commit-bot: I haz the power, 2014-03-26 17:34:54]

Failed to apply patch for build/git-hooks/pre-commit:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file build/git-hooks/pre-commit
  Hunk #2 FAILED at 45.
  1 out of 2 hunks FAILED -- saving rejects to file
build/git-hooks/pre-commit.rej

Patch:       build/git-hooks/pre-commit
Index: build/git-hooks/pre-commit
diff --git a/build/git-hooks/pre-commit b/build/git-hooks/pre-commit
index
3391a777e760afca4f6a1401863e04f87c095319..366b9ada930ab98cdcf29c9b4137a1c0e0ef006f
100755
--- a/build/git-hooks/pre-commit
+++ b/build/git-hooks/pre-commit
@@ -1,6 +1,28 @@
 #!/bin/sh
 
-submods=$(git diff-index --cached --ignore-submodules=dirty HEAD | grep -e
'^:160000' -e '^:...... 160000' | xargs)
+submodule_diff() {
+  if test -n "$2"; then
+    git diff-tree -r --ignore-submodules=dirty "$1" "$2" | grep -e '^:160000'
-e '^:...... 160000' | xargs
+  else
+    git diff-index --cached --ignore-submodules=dirty "$1" | grep -e '^:160000'
-e '^:...... 160000' | xargs
+  fi
+}
+
+if git rev-parse --verify --quiet --no-revs MERGE_HEAD; then
+  merge_base=$(git merge-base HEAD MERGE_HEAD)
+  if test -z "$(submodule_diff $merge_base HEAD)"; then
+    # Most up-to-date submodules are in MERGE_HEAD.
+    head_ref=MERGE_HEAD
+  else
+    # Most up-to-date submodules are in HEAD.
+    head_ref=HEAD
+  fi
+else
+  # No merge in progress. Submodules must match HEAD.
+  head_ref=HEAD
+fi
+
+submods=$(submodule_diff $head_ref)
 if test "$submods"; then
   echo "You are trying to commit changes to the following submodules:" 1>&2
   echo 1>&2
@@ -23,7 +45,11 @@ EOF
   exit 1
 fi
 
-if test "$(git diff-index --cached HEAD .gitmodules)"; then
+gitmodules_diff() {
+  git diff-index --cached "$1" .gitmodules
+}
+
+if test "$(gitmodules_diff $head_ref)"; then
   cat <<EOF 1>&2
 You are trying to commit a change to .gitmodules.  That is not allowed.
 To make changes to submodule names/paths, edit DEPS.

[spang, 2014-03-26 17:52:53]

The CQ bit was checked by spang@chromium.org

[commit-bot: I haz the power, 2014-03-26 17:53:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/spang@chromium.org/192443006/40001

[commit-bot: I haz the power, 2014-03-26 19:20:27]

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

[commit-bot: I haz the power, 2014-03-26 19:20:27]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[spang, 2014-03-26 20:03:51]

The CQ bit was checked by spang@chromium.org

[commit-bot: I haz the power, 2014-03-26 20:09:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/spang@chromium.org/192443006/40001

[commit-bot: I haz the power, 2014-03-26 22:05:41]

Change committed as 259711

[Ami GONE FROM CHROMIUM, 2014-04-04 18:07:13]

post-commit drive-by

https://codereview.chromium.org/192443006/diff/40001/build/git-hooks/pre-commit
File build/git-hooks/pre-commit (right):

https://codereview.chromium.org/192443006/diff/40001/build/git-hooks/pre-comm...
build/git-hooks/pre-commit:52: if test "$(git ls-files .gitmodules)" -a
"$(gitmodules_diff $head_ref)"; then
This change undid the benefit of r257470 because
test "$(/bin/false)" -a "$(echo foo >&2)"
doesn't short-circuit before running the second command, whereas
[ "$(/bin/false)" ] && [ "$(echo foo >&2)" ]
does short-circuit.

Was there any reason to make the change from [ to test other than aesthetics? 
(any objection to me flipping it back?)

[spang, 2014-04-04 18:14:02]

https://codereview.chromium.org/192443006/diff/40001/build/git-hooks/pre-commit
File build/git-hooks/pre-commit (right):

https://codereview.chromium.org/192443006/diff/40001/build/git-hooks/pre-comm...
build/git-hooks/pre-commit:52: if test "$(git ls-files .gitmodules)" -a
"$(gitmodules_diff $head_ref)"; then
On 2014/04/04 18:07:13, Ami Fischman wrote:
> This change undid the benefit of r257470 because
> test "$(/bin/false)" -a "$(echo foo >&2)"
> doesn't short-circuit before running the second command, whereas
> [ "$(/bin/false)" ] && [ "$(echo foo >&2)" ]
> does short-circuit.
> 

My bad.

> Was there any reason to make the change from [ to test other than aesthetics? 
> (any objection to me flipping it back?)

There's no intentional change here, since it was always "test".

Your CL changed it to [[, and then it changed to [.

[Ami GONE FROM CHROMIUM, 2014-04-04 18:19:37]

The [[ -> [ change was intentional to work with dash or some other
non-bash.
Will send a CL to flip test back to [.
Thanks.


On Fri, Apr 4, 2014 at 11:14 AM, <spang@chromium.org> wrote:

>
> https://codereview.chromium.org/192443006/diff/40001/
> build/git-hooks/pre-commit
> File build/git-hooks/pre-commit (right):
>
> https://codereview.chromium.org/192443006/diff/40001/
> build/git-hooks/pre-commit#newcode52
> build/git-hooks/pre-commit:52: if test "$(git ls-files .gitmodules)" -a
> "$(gitmodules_diff $head_ref)"; then
> On 2014/04/04 18:07:13, Ami Fischman wrote:
>
>> This change undid the benefit of r257470 because
>> test "$(/bin/false)" -a "$(echo foo >&2)"
>> doesn't short-circuit before running the second command, whereas
>> [ "$(/bin/false)" ] && [ "$(echo foo >&2)" ]
>> does short-circuit.
>>
>
>
> My bad.
>
>
>  Was there any reason to make the change from [ to test other than
>>
> aesthetics?
>
>> (any objection to me flipping it back?)
>>
>
> There's no intentional change here, since it was always "test".
>
> Your CL changed it to [[, and then it changed to [.
>
> https://codereview.chromium.org/192443006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[spang, 2014-04-04 18:23:57]

I'm just saying, I'm not really sure why you changed it from test in the
first place, so when I went to merge my change with yours I just left it as
test.

 On Fri, Apr 4, 2014 at 2:19 PM, Ami Fischman <fischman@chromium.org> wrote:

> The [[ -> [ change was intentional to work with dash or some other
> non-bash.
> Will send a CL to flip test back to [.
> Thanks.
>
>
> On Fri, Apr 4, 2014 at 11:14 AM, <spang@chromium.org> wrote:
>
>>
>> https://codereview.chromium.org/192443006/diff/40001/
>> build/git-hooks/pre-commit
>> File build/git-hooks/pre-commit (right):
>>
>> https://codereview.chromium.org/192443006/diff/40001/
>> build/git-hooks/pre-commit#newcode52
>> build/git-hooks/pre-commit:52: if test "$(git ls-files .gitmodules)" -a
>> "$(gitmodules_diff $head_ref)"; then
>> On 2014/04/04 18:07:13, Ami Fischman wrote:
>>
>>> This change undid the benefit of r257470 because
>>> test "$(/bin/false)" -a "$(echo foo >&2)"
>>> doesn't short-circuit before running the second command, whereas
>>> [ "$(/bin/false)" ] && [ "$(echo foo >&2)" ]
>>> does short-circuit.
>>>
>>
>>
>> My bad.
>>
>>
>>  Was there any reason to make the change from [ to test other than
>>>
>> aesthetics?
>>
>>> (any objection to me flipping it back?)
>>>
>>
>> There's no intentional change here, since it was always "test".
>>
>> Your CL changed it to [[, and then it changed to [.
>>
>> https://codereview.chromium.org/192443006/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ami GONE FROM CHROMIUM, 2014-04-04 18:35:55]

FTR https://codereview.chromium.org/225583008/


On Fri, Apr 4, 2014 at 11:23 AM, Michael Spang <spang@chromium.org> wrote:

> I'm just saying, I'm not really sure why you changed it from test in the
> first place, so when I went to merge my change with yours I just left it as
> test.
>
>  On Fri, Apr 4, 2014 at 2:19 PM, Ami Fischman <fischman@chromium.org>wrote:
>
>> The [[ -> [ change was intentional to work with dash or some other
>> non-bash.
>> Will send a CL to flip test back to [.
>> Thanks.
>>
>>
>> On Fri, Apr 4, 2014 at 11:14 AM, <spang@chromium.org> wrote:
>>
>>>
>>> https://codereview.chromium.org/192443006/diff/40001/
>>> build/git-hooks/pre-commit
>>> File build/git-hooks/pre-commit (right):
>>>
>>> https://codereview.chromium.org/192443006/diff/40001/
>>> build/git-hooks/pre-commit#newcode52
>>> build/git-hooks/pre-commit:52: if test "$(git ls-files .gitmodules)" -a
>>> "$(gitmodules_diff $head_ref)"; then
>>> On 2014/04/04 18:07:13, Ami Fischman wrote:
>>>
>>>> This change undid the benefit of r257470 because
>>>> test "$(/bin/false)" -a "$(echo foo >&2)"
>>>> doesn't short-circuit before running the second command, whereas
>>>> [ "$(/bin/false)" ] && [ "$(echo foo >&2)" ]
>>>> does short-circuit.
>>>>
>>>
>>>
>>> My bad.
>>>
>>>
>>>  Was there any reason to make the change from [ to test other than
>>>>
>>> aesthetics?
>>>
>>>> (any objection to me flipping it back?)
>>>>
>>>
>>> There's no intentional change here, since it was always "test".
>>>
>>> Your CL changed it to [[, and then it changed to [.
>>>
>>> https://codereview.chromium.org/192443006/
>>>
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.