Windows Git users: Please check your core.fileMode setting

[Lei Zhang]

If you don't use Git on Windows, you can stop reading now

On Linux, running ls --color will show executable files as bright
green. Over time, I see more and more .cc / .gyp, etc files turning
green in my Chromium tree. This happens when files in SVN has the
property svn:executable. I'm sure nobody here goes out of their way to
check in .cc files with svn:executable set, so I looked into it and
found this is happening by accident with Cygwin Git users on Windows.
i.e. [1] (not to pick on anyone in particular.)

I believe setting core.fileMode to false will prevent this from happening. Run:

git config core.filemode false

What might be happening is as follows:
a. Cygwin treats all files created by Windows as rwx by default.
b. Cygwin Git sees the files with executable permission.
c. Cygwin Git flips the svn:executable bit on when you run git cl dcommit.

I've written tools/checkperms/checkperms.py [2] to detect files that
should not have svn;executable set. I will soon enable this on the
Chromium Linux buildbot as well as the Linux trybots. If your Windows
Git client is misconfigured and check in files with svn:executable
set, you will turn the Chromium Linux bot red.

[1] http://src.chromium.org/viewvc/chrome?view=rev&revision=48994
[2] http://src.chromium.org/viewvc/chrome/trunk/src/tools/checkperms/checkperms.py?view=markup

[Lei Zhang]

You can also tell if your Git client is doing the wrong thing when the
patch you send to the try server contains:

diff --git chrome/browser/renderer_host/resource_message_filter.cc
chrome/browser/renderer_host/resource_message_filter.cc
old mode 100644
new mode 100755 <--- [ Flipping the executable permission on. ]
index bb1c7a2..73898dc
--- chrome/browser/renderer_host/resource_message_filter.cc
+++ chrome/browser/renderer_host/resource_message_filter.cc

You can find the patch sent to try servers at:
http://build.chromium.org/buildbot/try-server/builders/[platform]/builds/[try
job number]/steps/gclient/logs/patch
i.e. http://build.chromium.org/buildbot/try-server/builders/linux/builds/12345/steps/gclient/logs/patch

[Evan Stade]

> I've written tools/checkperms/checkperms.py [2] to detect files that
> should not have svn;executable set. I will soon enable this on the
> Chromium Linux buildbot as well as the Linux trybots. If your Windows
> Git client is misconfigured and check in files with svn:executable
> set, you will turn the Chromium Linux bot red.

I'm sure you've thought of this and have a good reason, but satisfy my
curiosity: why not make it a presubmit check instead?

[Lei Zhang]

Presubmit checks can be bypasses. If there's no check on the buildbot,
then things can slip through.

[Evan Stade]

> Presubmit checks can be bypasses. If there's no check on the buildbot,
> then things can slip through.
>

sure. But the level of badness of a wrong executable bit seems more on
par with end of line whitespace than a broken compile. Shrug.

[Roland Steiner]

At least my Cygwin 1.7 does it as well... >_<

Now, apart from manually removing those lines from patch files, is there a way to change a mode back to 100644 within a commit? "chmod 644 offending-files; git add offending-files; git commit --amend " at least still retains the mode changes 100644 -> 100755...

- Roland

On Wed, Jun 23, 2010 at 10:41 PM, Marc-Antoine Ruel <mar...@chromium.org> wrote:

PRESUBMIT check would be fine, I don't mind.

The thing is: does it happen more on cygwin 1.5 or cygwin 1.7? I
wouldn't be surprised if it happens more on 1.5.

M-A

> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
>    http://groups.google.com/a/chromium.org/group/chromium-dev
>

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
   http://groups.google.com/a/chromium.org/group/chromium-dev

[Mattias Nissler]

On Thu, Jun 24, 2010 at 7:50 AM, Roland Steiner <roland...@google.com> wrote:

At least my Cygwin 1.7 does it as well... >_<

Now, apart from manually removing those lines from patch files, is there a way to change a mode back to 100644 within a commit? "chmod 644 offending-files; git add offending-files; git commit --amend " at least still retains the mode changes 100644 -> 100755...

Good question. I tried a forced rebase, then a cherry-pick, which both didn't work. I ended up doing soft resets and then rebuilding the commit using the -C flag, passing the old hash in order to keep the commit message.

[Jay Soffian]

On Thu, Jun 24, 2010 at 1:50 AM, Roland Steiner
<roland...@google.com> wrote:
> At least my Cygwin 1.7 does it as well... >_<
> Now, apart from manually removing those lines from patch files, is there a
> way to change a mode back to 100644 within a commit? "chmod 644
> offending-files; git add offending-files; git commit --amend " at least
> still retains the mode changes 100644 -> 100755...

If you've set core.fileMode to true, then that won't make a difference
since you specifically told git to ignore the file mode changes with
that option.

Try:

$ git update-index --chmod=-x <file>...
$ git checkout -- <file>...
$ GIT_EDITOR=: git commit --amend

1) sets the mode in the index
2) syncs the updated mode back to the working copy
3) amends the existing commit w/o bothering you with an editor

j.

[Randy Smith]

I'd love to see this in the presubmit check as well as on the
trybots--I just got bitten by it because I ran trybots, made a minor
change on Windows, and decided it was ok to run just the unit tests
(since that's where I made the change). I'll dive in and set the
option now, but the extra belt and suspenders wouldn't hurt.

-- randy

On Wed, Jun 23, 2010 at 6:41 AM, Marc-Antoine Ruel <mar...@chromium.org> wrote:
> PRESUBMIT check would be fine, I don't mind.
>
> The thing is: does it happen more on cygwin 1.5 or cygwin 1.7? I
> wouldn't be surprised if it happens more on 1.5.
>
> M-A
>
> Le 23 juin 2010 00:57, Evan Stade <est...@chromium.org> a écrit :

[Lei Zhang]

I tried putting the check_perm step on the trybots, but when the try
bots apply the patch, it ignores the permission change in the patch
file.

We need to have a presubmit check for this. If you can write one that
would be great.