threads / patch / 16024

patchOnly update the cygwin-related configuration during state auto-setup

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

## tl;dr

7 messages between Oct 23, 2008 and Oct 28, 2008. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Alex Riesen· Oct 23, 2008, 13:07 UTC · lore

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>:
Show 11 quoted lines
>  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.

Junio C Hamano· Oct 24, 2008, 05:54 UTC · re: Alex Riesen · lore

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

"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?
Show changes to diff +13 −7
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 {
Alex Riesen· Oct 24, 2008, 07:47 UTC · re: Junio C Hamano · lore

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

2008/10/24 Junio C Hamano <gitster@pobox.com>:
Show 5 quoted lines
> 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 :-/

Nanako Shiraishi· Oct 27, 2008, 10:20 UTC · re: Junio C Hamano · lore

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

Quoting Junio C Hamano <gitster@pobox.com>:
Show 9 quoted lines
> 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/
Junio C Hamano· Oct 28, 2008, 03:27 UTC · re: Nanako Shiraishi · lore

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

Nanako Shiraishi <nanako3@lavabit.com> writes:
Show 13 quoted lines
> 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.

Alex Riesen· Oct 28, 2008, 12:21 UTC · re: Junio C Hamano · lore

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

2008/10/28 Junio C Hamano <gitster@pobox.com>:
Show 19 quoted lines
> 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.

← back to recent threads