[PATCH] Only update the cygwin-related configuration during state auto-setup

[PATCH] Only update the cygwin-related configuration during state auto-setup

[Alex Riesen]

[-- Attachment #1: Type: text/plain, Size: 1506 bytes --]

Otherwise the other global settings which were already read and set up will
be overwritten because the auto-setup code can be called really late in
game.  For instance, t3901-i18n-patch and --encoding=something of revision
argument parser are actually broken at the moment. The parser
(handle_revision_opt) sets git_log_output_encoding, which is also updated
(or in this case - overwritten) in the default config handler.

The code still has the problem if someone loads the configuration,
sets trust_executable_bit according to other conditions (a future
command-line option, perhaps) and than causes the init_stat call.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 compat/cygwin.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

2008/10/13 Mark Levedahl <mlevedahl@gmail.com>:
>  static int git_cygwin_config(const char *var, const char *value, void *cb)
>  {
> -       if (!strcmp(var, "core.ignorecygwinfstricks"))
> -               native_stat = git_config_bool(var, value);
> -       return 0;
> +       if (!strcmp(var, "core.ignorecygwinfstricks")) {
> +                       native_stat = git_config_bool(var, value);
> +                       return 0;
> +       }
> +       return git_default_config(var, value, cb);
>  }

This actually breaks t3901-i18n-patch (and --encoding=something of
revision argument parser). The parser (handle_revision_opt) sets
git_log_output_encoding, which is also updated (or in this case - overwritten)
in the default config handler.

[-- Attachment #2: 0001-Only-update-the-cygwin-related-configuration-during.patch --]
[-- Type: application/xxxxx, Size: 1669 bytes --]

Re: [PATCH] Only update the cygwin-related configuration during state auto-setup

[Alex Riesen]

Err... It should be: "Only update the cygwin-related configuration
during stat auto-setup".

Re: [PATCH] Only update the cygwin-related configuration during state auto-setup

[Junio C Hamano]

"Alex Riesen" <raa.lkml@gmail.com> writes:

> Otherwise the other global settings which were already read and set up will
> be overwritten ...

This is the answer to the question I asked in:

 http://thread.gmane.org/gmane.comp.version-control.git/97986/focus=98066

Perhaps we should use a separate variable as the original patch did, in:

  http://article.gmane.org/gmane.comp.version-control.git/97987

How about doing it like this instead?


diff --git i/compat/cygwin.c w/compat/cygwin.c
index f196753..ebac148 100644
--- i/compat/cygwin.c
+++ w/compat/cygwin.c
@@ -91,26 +91,32 @@ static int cygwin_stat(const char *path, struct stat *buf)
  * functions should be used. The choice is determined by core.ignorecygwinfstricks.
  * Reading this option is not always possible immediately as git_dir may be
  * not be set yet. So until it is set, use cygwin lstat/stat functions.
- * However, if the trust_executable_bit is set, we must use the Cygwin posix
+ * However, if core.filemode is set, we must use the Cygwin posix
  * stat/lstat as the Windows stat fuctions do not determine posix filemode.
+ *
+ * Note that git_cygwin_config() does NOT call git_default_config() and this
+ * is deliberate.  Many commands read from config to establish initial
+ * values in variables and later tweak them from elsewhere (e.g. command line).
+ * init_stat() is called lazily on demand, typically much late in the program,
+ * and calling git_default_config() from here would break such variables.
  */
 static int native_stat = 1;
-extern int trust_executable_bit;
+static int core_filemode;
 
 static int git_cygwin_config(const char *var, const char *value, void *cb)
 {
-	if (!strcmp(var, "core.ignorecygwinfstricks")) {
+	if (!strcmp(var, "core.ignorecygwinfstricks"))
 		native_stat = git_config_bool(var, value);
-		return 0;
-	}
-	return git_default_config(var, value, cb);
+	else if (!strcmp(var, "core.filemode"))
+		core_filemode = git_config_bool(var, value);
+	return 0;
 }
 
 static int init_stat(void)
 {
 	if (have_git_dir()) {
 		git_config(git_cygwin_config, NULL);
-		if (!trust_executable_bit && native_stat) {
+		if (!core_filemode && native_stat) {
 			cygwin_stat_fn = cygwin_stat;
 			cygwin_lstat_fn = cygwin_lstat;
 		} else {

Re: [PATCH] Only update the cygwin-related configuration during state auto-setup

[Alex Riesen]

2008/10/24 Junio C Hamano <gitster@pobox.com>:
> Perhaps we should use a separate variable as the original patch did, in:
>
>  http://article.gmane.org/gmane.comp.version-control.git/97987
>
> How about doing it like this instead?

I like this. Will start testing it over next european night, as soon as
the current test run finishes. In about 6 hours, that is :-/

Re: [PATCH] Only update the cygwin-related configuration during state auto-setup

[Nanako Shiraishi]

Quoting Junio C Hamano <gitster@pobox.com>:

> This is the answer to the question I asked in:
> 
>  http://thread.gmane.org/gmane.comp.version-control.git/97986/focus=98066
> 
> Perhaps we should use a separate variable as the original patch did, in:
> 
>   http://article.gmane.org/gmane.comp.version-control.git/97987
> 
> How about doing it like this instead?

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?

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [PATCH] Only update the cygwin-related configuration during state auto-setup

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

> Quoting Junio C Hamano <gitster@pobox.com>:
>
>> This is the answer to the question I asked in:
>> 
>>  http://thread.gmane.org/gmane.comp.version-control.git/97986/focus=98066
>> 
>> Perhaps we should use a separate variable as the original patch did, in:
>> 
>>   http://article.gmane.org/gmane.comp.version-control.git/97987
>> 
>> How about doing it like this instead?
>
> 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?

Heh, Alex's ack is good enough for me as far as the code itself is
concerned, but I do want these "fixes" accompanied by additional tests to
reproduce to avoid future regressions, and this being a Cygwin fix, I am
not really the right person to write tests nor run them.

Re: [PATCH] Only update the cygwin-related configuration during state auto-setup

[Alex Riesen]

2008/10/28 Junio C Hamano <gitster@pobox.com>:
> Nanako Shiraishi <nanako3@lavabit.com> writes:
>> Quoting Junio C Hamano <gitster@pobox.com>:
>>
>>> This is the answer to the question I asked in:
>>>
>>>  http://thread.gmane.org/gmane.comp.version-control.git/97986/focus=98066
>>>
>>> Perhaps we should use a separate variable as the original patch did, in:
>>>
>>>   http://article.gmane.org/gmane.comp.version-control.git/97987
>>>
>>> How about doing it like this instead?
>>
>> 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?
>
> Heh, Alex's ack is good enough for me as far as the code itself is
> concerned, but I do want these "fixes" accompanied by additional tests to
> reproduce to avoid future regressions, and this being a Cygwin fix, I am
> not really the right person to write tests nor run them.

I suggest NOT writing the test for the workaround for just one platform I
personally call the most idiotic of the Microsoft's fallouts. As the
fix is located
in the code specific to that platform, it wont do any harm for anyone, whether
it works or not (even if the code in tree does not work). The Junio's fix is
definitely enough for me and, I'm very sure, for anyone still having to
deal with cygwin.

So for the Google's record: the patch to fix the --encoding option of git
format-patch broken on cygwin is in the Junio's mail, to be found, i.e.:
http://marc.info/?l=git&m=122482769817566&w=4

Nanako, as there is not many of such poor bastards left, it is not
even a problem
to keep the fix just in Git mailing list archives. There is just not
enough of a looser
base to press its development in whatever direction.