On 11/16/2014 08:08 PM, Junio C Hamano wrote: > Michael Haggerty <mhagger@xxxxxxxxxxxx> 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@xxxxxxxxxxxx -- 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