{"thread":{"id":"16024","subject":"[PATCH] Only update the cygwin-related configuration during state auto-setup","startedAt":"2008-10-23T13:07:22Z","lastAt":"2008-10-28T12:21:59Z","messageCount":7,"participants":["Alex Riesen","Junio C Hamano","Nanako Shiraishi"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"93779","messageId":"81b0412b0810230607sfea05ddm62bd03f837fc922e@mail.gmail.com","threadId":"16024","inReplyTo":null,"subject":"[PATCH] Only update the cygwin-related configuration during state auto-setup","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-10-23T13:07:22Z","receivedAt":"2008-10-23T13:07:22Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Otherwise the other global settings which were already read and set up will\nbe overwritten because the auto-setup code can be called really late in\ngame. For instance, t3901-i18n-patch and --encoding=something of revision\nargument parser are actually broken at the moment. The parser\n(handle_revision_opt) sets git_log_output_encoding, which is also updated\n(or in this case - overwritten) in the default config handler.\n\nThe code still has the problem if someone loads the configuration,\nsets trust_executable_bit according to other conditions (a future\ncommand-line option, perhaps) and than causes the init_stat call.\n\nSigned-off-by: Alex Riesen \n---\n compat/cygwin.c | 9 +++++----\n 1 files changed, 5 insertions(+), 4 deletions(-)\n\n2008/10/13 Mark Levedahl :\n> static int git_cygwin_config(const char *var, const char *value, void *cb)\n> {\n> - if (!strcmp(var, \"core.ignorecygwinfstricks\"))\n> - native_stat = git_config_bool(var, value);\n> - return 0;\n> + if (!strcmp(var, \"core.ignorecygwinfstricks\")) {\n> + native_stat = git_config_bool(var, value);\n> + return 0;\n> + }\n> + return git_default_config(var, value, cb);\n> }\n\nThis actually breaks t3901-i18n-patch (and --encoding=something of\nrevision argument parser). The parser (handle_revision_opt) sets\ngit_log_output_encoding, which is also updated (or in this case - overwritten)\nin the default config handler.\n"},{"id":"93798","messageId":"20081023183630.GA4905@blimp.localdomain","threadId":"16024","inReplyTo":"81b0412b0810230607sfea05ddm62bd03f837fc922e@mail.gmail.com","subject":"Re: [PATCH] Only update the cygwin-related configuration during state auto-setup","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-10-23T18:36:30Z","receivedAt":"2008-10-23T18:36:30Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Err... It should be: \"Only update the cygwin-related configuration\nduring stat auto-setup\".\n"},{"id":"93827","messageId":"7viqri35dq.fsf@gitster.siamese.dyndns.org","threadId":"16024","inReplyTo":"81b0412b0810230607sfea05ddm62bd03f837fc922e@mail.gmail.com","subject":"Re: [PATCH] Only update the cygwin-related configuration during state auto-setup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-24T05:54:09Z","receivedAt":"2008-10-24T05:54:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alex Riesen\" writes:\n\n> Otherwise the other global settings which were already read and set up will\n> be overwritten ...\n\nThis is the answer to the question I asked in:\n\n http://thread.gmane.org/gmane.comp.version-control.git/97986/focus=98066\n\nPerhaps we should use a separate variable as the original patch did, in:\n\n http://article.gmane.org/gmane.comp.version-control.git/97987\n\nHow about doing it like this instead?\n\n\ndiff --git i/compat/cygwin.c w/compat/cygwin.c\nindex f196753..ebac148 100644\n--- i/compat/cygwin.c\n+++ w/compat/cygwin.c\n@@ -91,26 +91,32 @@ static int cygwin_stat(const char *path, struct stat *buf)\n * functions should be used. The choice is determined by core.ignorecygwinfstricks.\n * Reading this option is not always possible immediately as git_dir may be\n * not be set yet. So until it is set, use cygwin lstat/stat functions.\n- * However, if the trust_executable_bit is set, we must use the Cygwin posix\n+ * However, if core.filemode is set, we must use the Cygwin posix\n * stat/lstat as the Windows stat fuctions do not determine posix filemode.\n+ *\n+ * Note that git_cygwin_config() does NOT call git_default_config() and this\n+ * is deliberate. Many commands read from config to establish initial\n+ * values in variables and later tweak them from elsewhere (e.g. command line).\n+ * init_stat() is called lazily on demand, typically much late in the program,\n+ * and calling git_default_config() from here would break such variables.\n */\n static int native_stat = 1;\n-extern int trust_executable_bit;\n+static int core_filemode;\n \n static int git_cygwin_config(const char *var, const char *value, void *cb)\n {\n-\tif (!strcmp(var, \"core.ignorecygwinfstricks\")) {\n+\tif (!strcmp(var, \"core.ignorecygwinfstricks\"))\n \t\tnative_stat = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\treturn git_default_config(var, value, cb);\n+\telse if (!strcmp(var, \"core.filemode\"))\n+\t\tcore_filemode = git_config_bool(var, value);\n+\treturn 0;\n }\n \n static int init_stat(void)\n {\n \tif (have_git_dir()) {\n \t\tgit_config(git_cygwin_config, NULL);\n-\t\tif (!trust_executable_bit && native_stat) {\n+\t\tif (!core_filemode && native_stat) {\n \t\t\tcygwin_stat_fn = cygwin_stat;\n \t\t\tcygwin_lstat_fn = cygwin_lstat;\n \t\t} else {\n"},{"id":"93834","messageId":"81b0412b0810240047h4a9b1d04m44a1a76df722d075@mail.gmail.com","threadId":"16024","inReplyTo":"7viqri35dq.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Only update the cygwin-related configuration during state auto-setup","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-10-24T07:47:03Z","receivedAt":"2008-10-24T07:47:03Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2008/10/24 Junio C Hamano :\n> Perhaps we should use a separate variable as the original patch did, in:\n>\n> http://article.gmane.org/gmane.comp.version-control.git/97987\n>\n> How about doing it like this instead?\n\nI like this. Will start testing it over next european night, as soon as\nthe current test run finishes. In about 6 hours, that is :-/\n"},{"id":"94038","messageId":"20081027192018.6117@nanako3.lavabit.com","threadId":"16024","inReplyTo":"7viqri35dq.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Only update the cygwin-related configuration during state auto-setup","fromName":"Nanako Shiraishi","fromEmail":"nanako3@lavabit.com","sentAt":"2008-10-27T10:20:18Z","receivedAt":"2008-10-27T10:20:18Z","isPatch":true,"sender":{"key":"nanako3@lavabit.com","avatar":"https://gravatar.com/avatar/3777b9e201c5883a62b1a6fdf7c53f2d712d1d80989146063ea861e33aad72a8?d=mp&s=160"},"body":"Quoting Junio C Hamano :\n\n> This is the answer to the question I asked in:\n> \n> http://thread.gmane.org/gmane.comp.version-control.git/97986/focus=98066\n> \n> Perhaps we should use a separate variable as the original patch did, in:\n> \n> http://article.gmane.org/gmane.comp.version-control.git/97987\n> \n> How about doing it like this instead?\n\nJunio, may I ask what the status of this patch is? I see you did not write tests nor commit message --- are you waiting for others to write them?\n\n-- \nNanako Shiraishi\nhttp://ivory.ap.teacup.com/nanako3/\n"},{"id":"94089","messageId":"7vhc6xh010.fsf@gitster.siamese.dyndns.org","threadId":"16024","inReplyTo":"20081027192018.6117@nanako3.lavabit.com","subject":"Re: [PATCH] Only update the cygwin-related configuration during state auto-setup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-28T03:27:23Z","receivedAt":"2008-10-28T03:27:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nanako Shiraishi writes:\n\n> Quoting Junio C Hamano :\n>\n>> This is the answer to the question I asked in:\n>> \n>> http://thread.gmane.org/gmane.comp.version-control.git/97986/focus=98066\n>> \n>> Perhaps we should use a separate variable as the original patch did, in:\n>> \n>> http://article.gmane.org/gmane.comp.version-control.git/97987\n>> \n>> How about doing it like this instead?\n>\n> Junio, may I ask what the status of this patch is? I see you did not write tests nor commit message --- are you waiting for others to write them?\n\nHeh, Alex's ack is good enough for me as far as the code itself is\nconcerned, but I do want these \"fixes\" accompanied by additional tests to\nreproduce to avoid future regressions, and this being a Cygwin fix, I am\nnot really the right person to write tests nor run them.\n"},{"id":"94100","messageId":"81b0412b0810280521x5982315ahe1d81c2c7176f1c8@mail.gmail.com","threadId":"16024","inReplyTo":"7vhc6xh010.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Only update the cygwin-related configuration during state auto-setup","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-10-28T12:21:59Z","receivedAt":"2008-10-28T12:21:59Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2008/10/28 Junio C Hamano :\n> Nanako Shiraishi writes:\n>> Quoting Junio C Hamano :\n>>\n>>> This is the answer to the question I asked in:\n>>>\n>>> http://thread.gmane.org/gmane.comp.version-control.git/97986/focus=98066\n>>>\n>>> Perhaps we should use a separate variable as the original patch did, in:\n>>>\n>>> http://article.gmane.org/gmane.comp.version-control.git/97987\n>>>\n>>> How about doing it like this instead?\n>>\n>> Junio, may I ask what the status of this patch is? I see you did not write tests nor commit message --- are you waiting for others to write them?\n>\n> Heh, Alex's ack is good enough for me as far as the code itself is\n> concerned, but I do want these \"fixes\" accompanied by additional tests to\n> reproduce to avoid future regressions, and this being a Cygwin fix, I am\n> not really the right person to write tests nor run them.\n\nI suggest NOT writing the test for the workaround for just one platform I\npersonally call the most idiotic of the Microsoft's fallouts. As the\nfix is located\nin the code specific to that platform, it wont do any harm for anyone, whether\nit works or not (even if the code in tree does not work). The Junio's fix is\ndefinitely enough for me and, I'm very sure, for anyone still having to\ndeal with cygwin.\n\nSo for the Google's record: the patch to fix the --encoding option of git\nformat-patch broken on cygwin is in the Junio's mail, to be found, i.e.:\nhttp://marc.info/?l=git&m=122482769817566&w=4\n\nNanako, as there is not many of such poor bastards left, it is not\neven a problem\nto keep the fix just in Git mailing list archives. There is just not\nenough of a looser\nbase to press its development in whatever direction.\n"}]}