Hi,
On Sun, 27 Jan 2008, Kirill wrote:
> On Jan 25, 2008 8:54 AM, Johannes Schindelin
> <[EMAIL PROTECTED]> wrote:
>
> > I just thought about install-user.reg.in looking awfully like
> > install.reg (that thought occurred to me because of what I wrote in
> > that load.reg snippet in my other mail). So would it be possible to
> > generate install-user.reg from install.reg instead of
> > install-user.reg.in?
>
> Please, comment on the following patch.
> I hope you don't mind a bit of cleaning up in the install.reg.in, namely:
> - remove redundant txtfile and lnkfile keys (they're covered by the * key);
> - bring HKEY_LOCAL_MACHINE keys closer to each other.
That seems like a good cleanup to me!
> > > diff --git a/systeminfo.c b/systeminfo.c
> > > index 08429bf..07444d6 100644
> > > --- a/systeminfo.c
> > > +++ b/systeminfo.c
> > > @@ -8,32 +8,45 @@ TCHAR * msys_path(void)
> > > + /* try to find user-specific settings first */
> > > + lRet = RegOpenKeyEx(HKEY_CURRENT_USER,
> > > TEXT(GIT_CHEETAH_REG_PATH),
> > > 0, KEY_QUERY_VALUE, &hKey);
> > > -
> > > - if (lRet == ERROR_SUCCESS)
> > > - {
> > > - DWORD msysPathLen = MAX_PATH * sizeof(TCHAR);
> > > -
> > > + if (ERROR_SUCCESS == lRet) {
> >
> > We have the same code here:
> >
> > > lRet = RegQueryValueEx(hKey,
> > > TEXT(GIT_CHEETAH_REG_PATHTOMSYS),
> > > NULL, NULL,
> > > (LPBYTE)msysPath,
> > > &msysPathLen);
> > > RegCloseKey(hKey);
> > > + }
> > >
> > > - if (lRet == ERROR_SUCCESS)
> > > - {
> > > - found_path = 1;
> > > - return msysPath;
> > > + /* if current user does not have the path, try machine-wide */
> > > + if (ERROR_SUCCESS != lRet) {
> > > + lRet = RegOpenKeyEx(HKEY_LOCAL_MACHINE,
> > > + TEXT(GIT_CHEETAH_REG_PATH),
> > > + 0, KEY_QUERY_VALUE, &hKey);
> > > + if (ERROR_SUCCESS == lRet) {
> >
> > ... and here:
> >
> > > + lRet = RegQueryValueEx(hKey,
> > > + TEXT(GIT_CHEETAH_REG_PATHTOMSYS),
> > > + NULL, NULL,
> > > + (LPBYTE)msysPath,
> > > + &msysPathLen);
> > > + RegCloseKey(hKey);
> >
> > Would it not be better to check if the user key could be found, if not,
> > try the system key, and if that was not found, error out. Otherwise
> > proceed to actually read the value.
> I had that version, but it does not work very well if:
> - there is a user key __and__
> - there is no value under that key.
>
> So, the code essentially verifies both conditions: the key is present
> in user space and it has a readable value. If either of them is not
> met, the code goes into machine space. Makes sense?
Absolutely. But I am a fan of the DRY principle: Don't Repeat Yourself.
It might make sense to make a small function (maybe
"get_msys_path_from_registry()"?), refactoring that piece of code. That
would serve three purposes:
- you don't repeat yourself (and therefore, future changes do not have to
be carried out several times),
- the code and patch are shorter (thus bugs have a harder time to hide),
and
- the code is more self-documenting (making it easier for others to guess
your intent).
Ciao,
Dscho
