Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config

[Michael Haggerty <mhagger@alum.mit.edu>]

On 11/16/2014 08:08 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Since time immemorial, the test of whether to set "core.filemode" has
>> been done by trying to toggle the u+x bit on $GIT_DIR/config and then
>> testing whether the change "took". It is somewhat odd to use the
>> config file for this test, but whatever.
> 
> The last sentence should read "We could create a test file and use
> it for this purpose and then remove it, but config is a file we know
> exists at this point in the code (and it is the only file we know
> that exists), so it was a very sensible trick".
> 
> Or remove it altogether.  In other words, do not sound as if you do
> not know what you are doing in your log message.  That would rob
> confidence in the change from the person who is reading "git log"
> output later.

The sentence is not meant to rob confidence in this change, but rather
to stimulate the reader's critical thinking about nearby code that I am
*not* changing.

By making this change without changing the function to use a temporary
file for its chmod experiments, I might otherwise give future readers
the impression that I like this shortcut, which I do not. For example,
if the original code had used a temporary file rather than "config",
then we would never have had the bug that I'm fixing. The "but whatever"
is meant to indicate that I don't disagree so strongly with the choice
of tradeoffs made by the original author that I think it is worth changing.

So maybe I am a coward (or lazy) for not proposing to change to using a
temporary file instead. But since this patch is suggested for maint, I
wanted to make the smallest change that would fix the bug.

Feel free to delete the controversial sentence if you prefer.

>> @@ -255,6 +255,7 @@ static int create_default_files(const char *template_path)
>>  		filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&
>>  				!lstat(path, &st2) &&
>>  				st1.st_mode != st2.st_mode);
>> +		filemode &= !chmod(path, st1.st_mode);
> 
> Sounds good.
> 
> You could also &&-chain this "flip it back" to the above statement.
> If filemode is not trustable on a filesytem, doing one extra chmod()
> to correct would not help us anyway, no?

Yes, that would be better. I will fix it.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu