Hi, On Mon, 4 Feb 2008, Kirill wrote:
> If the next time, you prefer one answer to three patch reviews, please, > let me know. For now, two others will follow. I like to have one answer per email... > Big question first: can we use some parts of git? For example, > git-compat-util.h? Or regex.[ch]? Hmm. Since libgit.a is really not meant as a public library (too many die()s in there, for one), I would try to refrain from that. Also having parts of it instead of linking to libgit.a would seem wrong to me... > > -----Original Message----- > > From: Johannes Schindelin [mailto:[EMAIL PROTECTED] > > Sent: Sunday, February 03, 2008 6:16 PM > > > > diff --git a/dll.c b/dll.c > > > index bab9d9a..40f52ae 100644 > > > --- a/dll.c > > > +++ b/dll.c > > > > + static char module_filename[MAX_PATH] = { '\0' }; > > > > AFAIK static variables are initialised all-zero anyway. > > It seems to me the code that explicitly initializes [all] variables is > easier to read. As a side note, IIRC it depends on (and/or configurable in) > the compiler. > So, unless you insist, I'd like to keep explicit initialization. AFAIK it is C89 standard, and I am used to it from git.git's source code. IOW when I read "static" my mind tells me automatically that it's initialised to all-zero. But I really do not care all that much. > > > + /* TODO: find an elegant way to pass the installation type > > > + down here */ > > > > Why not a global variable? > > Probably because I did not find global variables for this kind of use > elegant enough :) But given that it's the "last resort" thing, you are > probably right. Well, it _is_ a global variable. It contains a property that is the same for all the code, so you do not really need to pass around the value via function parameters. > > > +/* replaces a substring pattern with a string > > > +static char *strreplace(char *string, > > > + const char *pattern, > > > + const char *replacement) > > > > This is not safe <snip> > > Oh! I was really hoping for something like "well, well... lack of Unix > experience shows... why not use regex library" :) > This function alone is the reason for the big question. You mean strbuf? Sure, why not? > > > +static const REG_VALUE registry_info[] = { > > > + { "SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Shell > > > Extensions\\Approved", "@@CLSID@@", "@@PROGRAM_NAME@@" }, > > > > How about > > #define SHELLEXT "SOFTWARE\\...\\Shell Extensions" > > static const REG_VALUE registry_info[] = { > > { SHELLEXT "\\Approved", "@@CLSID@@", "@@PROGRAM_NAME@@" }, > > I'm afraid I don't fully understand this comment. Do you mean #define > repeated parts? If so, would you prefer to see: > > #define CLASSESROOT "Software\\Classess\\" > #define CONTEXTMENUHANDLER "shellex\\ContextMenuHandlers\\@@PROGRAM_NAME@@" > > static const reg_value registry_info[] = { > { CLASSESROOT "Drive\\" CONTEXTMENUHANDLER, NULL, "@@CLSID@@" } > > In other words, how small the part needs to be to warrant a define? Yes, you understood correctly. How small the part really depends on your taste, but I'd rather use pretty large common prefixes, personally. > > Also, please use a lower-case name for reg_value (upper-case in the C > > world is commonly used for #define'd stuff). > > Just to satisfy my curiocity: have I mistaken Windows' way of defining > types, structures, enums for "common in C"? In other words, isn't it common > to use REG_VALUE as a __type__ (I do understand that a variable would > usually be reg_value). If it is a #define, then it should be upper-case. If it is a typedef, it should be lower-case. And yes, Windows' usage of upper-case structs is, uhm, another instance of that annoying we-behave-differently-than-everybody-else behaviour. > > > +static char *get_registry_path(const char *src, char > > > + dst[MAX_REGISTRY_PATH]) > > > +{ > > > > Since this function is not called in multi-threaded manner, why not > > just having a "static char dst[MAX_PATH]"? > > I'm afraid I don't understand. The reason to pass the destination buffer as > a parameter is because I need to hold three different dst at the same time > (see the use case in create_reg_entries). So, I don't see how static char > dst[MAX_PATH] will work. Oh, oops. I thought I saw only one-at-a-time. > > > +HRESULT PASCAL DllInstall(BOOL bInstall, LPCWSTR pszCmdLine) > > > { > > > - if (reason == DLL_PROCESS_ATTACH) { > > > - object_count = lock_count = 0; > > > - DisableThreadLibraryCalls(instance); > > > + BOOL bMachine = NULL != wcsstr(pszCmdLine, L"machine"); > > > > That is not enough, right? > > (just imagine somebody happens to call "regsrv32 > > C:/machine/git-cheetah.dll"). > > At first, you do realize that "if (reason...)" is from completely different > function, don't you? I just want to make it clear that the patch is not > obvious because I haven't really changed DllMain, I just added bunch of code > to the rest of the file. And... I had to re-format-patch 3 times because > could not understand it :) > > At second, the implementation of DllInstall seems enough (with a caveat, > described in its comments about debug-only machine-wide). The reason is: > regsvr32 has [seemingly] complicated way to identify what to call. Here is a > simplified version: > (no keys) = DllRegister > -i = DllRegister + DllInstall > -n -i = DllInstall > So, your example will call DllRegister, which currently performs a normal > registration for the current user. To perform machine registration only - > "regsvr32 -n -i:machine git-cheetah.dll". > > Or I misunderstood the comment. Ah! That is indeed funny. But what about $ regsvr32 -i:debug C:\machine\git-cheetah.dll Would that not pick up the "machine" erroneously in DllInstall? Thanks, Dscho
