Hi,
On Fri, 1 Feb 2008, Kirill wrote:
> <not part of the commit message>
> The minimal regedit engine is so big that it's tempting to move it
> from dll.c to a separate module
> </not part of the commit message>
That's a possible next commit, no? Or if you feel strongly, you can
introduce registry.[ch], right?
> msysGit (for PathToMsys) is searched in the following order:
> - %PATH%
> - $(TARGET)/..
> - $(TARGET)/../..
> - InstallLocation of uninstall info (machine first, then user).
May I suggest $(TARGET)/.. and $(TARGET)/../.. to be the first two, then
%PATH%, and then maybe InstallLocation?
> TODO: verify that the found git.exe is really Git
With git-cheetah being installed along the rest of Git (in a hopefully not
too distant future), this is a non-issue.
> TODO: search for InstallLocation according to the type of the current
> installation (default or -i:machine)
See above.
> diff --git a/dll.c b/dll.c
> index bab9d9a..40f52ae 100644
> --- a/dll.c
> +++ b/dll.c
> @@ -1,12 +1,129 @@
> #include <shlobj.h>
> +#include <stdio.h>
> #include "dll.h"
> #include "ext.h"
> #include "factory.h"
> +#include "systeminfo.h"
>
> /*
> * The following is just the necessary infrastructure for having a .dll
> * which can be registered as a COM object.
> */
> +static HINSTANCE hInst;
> +
> +static const char *get_module_filename() {
> + static char module_filename[MAX_PATH] = { '\0' };
AFAIK static variables are initialised all-zero anyway.
> + if ('\0' == module_filename[0]) {
We tend to write "if (!*module_filename)" for this in git.git.
> + DWORD module_size;
> +
> + module_size = GetModuleFileName(hInst, module_filename,
> MAX_PATH);
This line (and others in your patch) are longer than 80 characters.
Please cut them, unless there is a compelling reason not to.
> +/* as per "How to: Convert Between System::Guid and _GUID" */
> +static const char *get_class_id()
> +{
> + static char class_id[MAX_REGISTRY_PATH] = { '\0' };
> +
> + if ('\0' == class_id[0]) {
> + GUID guid = CLSID_git_shell_ext;
> + sprintf (class_id,
> + "{%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x}",
> + guid.Data1, guid.Data2, guid.Data3,
> + guid.Data4[0], guid.Data4[1], guid.Data4[2],
> guid.Data4[3],
> + guid.Data4[4], guid.Data4[5], guid.Data4[6],
> guid.Data4[7]);
Again, too long lines. Besides, could you skip the space after "sprintf"?
We only have spaces after operators, such as "for", "if", "switch".
Regular functions do not get a space after the name.
> +static char msysgit[MAX_PATH] = { '\0' };
Again, static variables do not need to be initialised.
> + if (0 == dwFound || // git.exe is not in the PATH or
Please avod C++-style comments (even if we are definitely married to
gcc/MSVC, which seem to be fine, both, I'd like to keep to the coding
guidelines of git.git, i.e. /* ... */ instead of // ...).
> + *(file - 5) = '\0'; // git exe is in "\bin\" from what we really need
You need to make sure that
- file > msysgit + 5, and
- !strncmp(file - 5, "\\bin\\", 5)
and then I'd write this as
file[-5] = '\0';
> +static const char *find_msysgit()
> +{
> + if ('\0' == msysgit[0]) {
> + if (find_msysgit_in_path())
> + return msysgit;
> +
> + // try .. from our own directory
I think that comments like this one are not necessary, as the code is
self-explanatory:
> + if (find_msysgit_relative(".."))
> + return msysgit;
>
> [...]
>
> + /* TODO: find an elegant way to pass the installation type
> + down here */
Why not a global variable?
> + if (! find_msysgit_uninstall(HKEY_LOCAL_MACHINE))
Please lose the space after the exclamation mark.
> +/* replaces a substring pattern with a string replacement within a string
> + the replacement occurs in-place, hence string must be large enough to
> + hold the result
> +
> + the function does not handle recursive replacements, e.g.
> + strreplace ("foo", "bar", "another bar"); // unexpected results
> +
> + always returns *string
> +*/
> +static char *strreplace(char *string,
> + const char *pattern,
> + const char *replacement)
> +{
> + char *tail;
> + char *found = strstr(string, pattern);
> +
> + while (found) {
> + tail = strdup(found + strlen(pattern));
> +
> + *found = '\0';
> + strcat(string, replacement);
> + strcat(string, tail);
> +
> + free(tail);
> +
> + found = strstr(string, pattern);
> + }
This is not safe: it does not check the size of "string" anywhere. I'd
just pass it in as "size_t size", have "size_t len = strlen(string),
pattern_len = strlen(pattern), replacement_len = strlen(replacemen_len)".
Then I'd check in the "while (found)" loop if "len + replacement_len -
pattern_len >= size" (because of the NUL termination, it has to include
"="), and if that still holds true,
if (pattern_len != replacement_len)
memmove(string + found + replacement_len - pattern_len,
string + found, len - found - pattern_len + 1);
memcpy(string + found, replacement, replacement_len);
> +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@@" },
?
Also, please use a lower-case name for reg_value (upper-case in the C
world is commonly used for #define'd stuff).
> +/* supports @@PROGRAM_NAME@@, @@PROGRAM_PATH@@, @@CLSID@@ patterns */
> +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]"?
> +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? But I agree, we can make that more fool-proof
later (just imagine somebody happens to call "regsrv32
C:/machine/git-cheetah.dll").
> + } else if (bDebug) {
> + result = create_reg_entries(HKEY_CURRENT_USER,
> + debug_info);
> + }
If there is only one line in the clause, please do not use { .. }.
Overall, very nicely done!
Ciao,
Dscho
