Hi Torsten,
Thank you for taking the time to review my patch.
On 28 September 2014 04:52, Torsten Bögershausen <tboegi@xxxxxx> wrote:
> On 2014-09-28 02.37, Hilco Wijbenga wrote:
>> If "~/.gitconfig" contains a "core.filemode" entry then "git init"
>> should honour that setting.
>>
>> Signed-off-by: Hilco Wijbenga <hilco.wijbenga@xxxxxxxxx>
>> ---
>> This bit me at work where I have to work with Windows. Git on Cygwin
>> and the Eclipse Git plugin do not agree on file attributes so I had
>> set "filemode = false" in ~/.gitconfig.
> This feels strange.
> Each and every repo has a core.filemode setting.
> Or should have.
>
> Did you manage to create a repo without core.filemode in repo/.git/config ?
> And if yes, how?
Perhaps I completely misunderstand the meaning of core.filemode but I
thought it determined whether Git cared about changes in file
properties? So this is client OS related and independent of the repo?
>> A few weeks later, I did a "git init" and, some time later yet, I
>> noticed the strange behaviour of Cygwin/Eclipse again.
> I do not fully understand which "strange behaviour" you experied,
> so I need to guess.
The problem is simply that Eclipse's Git sees changes that Cygwin's
Git does not. It's some sort of unfortunate consequence of trying to
pretend to be Linux on Windows, I guess. The only way to get them to
agree was to set core.filemode to false. Now you might rightly argue
that Eclipse and/or Windows and/or Cygwin should be fixed but that's a
much bigger undertaking than simply setting an existing Git property.
:-)
> This was very
>> surprising because things had been working well until then. It took
>> quite a bit of research before I realized that "git init" always sets
>> "filemode". I think "filemode" should only be set if not set already
>> in the global config (similar to log_all_ref_updates).
>
> That is part of the whole story:
> In general, "git init" probes the file system, if the executable bit
> is working as expected.
> So if you create a Git repository under VFAT, the executable bit is not supported.
>
> Git will notice that, and set core.filemode = false.
>
> NTFS is a different story:
> Cygwin has support for the executable bit under NTFS, but Msysit does not.
Agreed. That is what I concluded from the code.
> So if you "share" a Git repository between Msysgit and cygwin, it may be better
> to set core.filemode to false.
Possibly. I would argue that that is up to the individual developer.
> There is however a problem with your patch, or 2:
>
> When you set core.filemode = false in your ~/.gitconfig,
> another developer may have core.filemode = true in his config.
> If you manage to share the repo using a network, git will behave different
> for the 2 users.
Isn't that what everything in ~/gitconfig is for? So that I can set
attributes appropriate to my working environment? Besides, that is
already the case if developer A uses a VFAT system and developer B
uses NTFS or JFS or EXTn or ..., right? (As you also indicated above.)
> Solution:
> Set core.filemode for this repo alwways in the repo. (as we do today in git.git)
I suppose you could set it to false, yes. But then it affects
everybody, that seems like going for the lowest common denominator?
> When you run "git init" with ~/.gitconfig = true, you should
> anyway probe the file system, as it may not support file mode, and core.filemode may be false.
>
>
> So the solution that I can see is:
> (Some pseudo-code:)
>
> if (git config (global config ) == false) ||
> (git config (~/.config ) == false) then
> git_config_set("core.filemode", "false");
> else
> probe the file system and set core.filemode as we do today
> fi
Yeah, I actually considered that (well, something less complete,
actually :-) ) but decided to go for the simpler approach that I
showed. My assumption is that the developer working with the repo
knows what he is doing. It seems wrong for Git to override that
decision. Then again, I don't really see any advantage in setting
core.filemode to true when working with, say, a VFAT filesystem, so
ignoring it in that case might be okay. Would such a setup do active
damage, though?
>> The usual caveat applies: this is my first patch. Having said that,
>> please feel free to be pedantic and strict. It's a small patch so I
>> would imagine that fixing any problems should not take long (assuming
>> it is acceptable at all, of course). I'd like to know I did it right.
>> :-)
>>
>> AFAICT, all tests passed. Should a separate test be added for this change?
> I think yes.
Okay, I'll have to figure out how to do that.
> Under which system did you test ?
>
> Windows?
> CYWGIN ?
> MingWW/Msysgit ?
> Linux ?
Only Linux. I don't really run Windows at home.
>> - /* Check filemode trustability */
>> - filemode = TEST_FILEMODE;
>> - if (TEST_FILEMODE && !lstat(path, &st1)) {
>> - struct stat st2;
>> - filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&
>> - !lstat(path, &st2) &&
>> - st1.st_mode != st2.st_mode);
>> + /* Do not override the global filemode setting. */
>> + if (trust_executable_bit == -1) {
>> + /* Check filemode trustability */
>> + filemode = TEST_FILEMODE;
>> + if (TEST_FILEMODE && !lstat(path, &st1)) {
>> + struct stat st2;
>> + filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&
>> + !lstat(path, &st2) &&
>> + st1.st_mode != st2.st_mode);
>> + }
>> + git_config_set("core.filemode", filemode ? "true" : "false");
> The indentation seems to be broken ?
> (We use one TAB, for better info please see Documentation/CodingGuidelines)
> [snip]
I did. :-) I followed an online tutorial geared to Google mail to
basically run git format-patch | git imap-send but the end result did
not have the tabs that I have in the code. I'll have to research it a
bit more then.
Cheers,
Hilco
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html