Hi,
On Sat, 2 Feb 2008, Kirill wrote:
> TODO: think about moving env_for_git() and exec_with_status() to
> systeminfo. It does not really belong there, but even less so to menu.
How about exec.[ch]?
> TODO: decide on a structure and actions of added menu items.
Yep. I think a good starting point is to look at what
TortoiseCVS/TortoiseSVN provide.
Personally, the thing I used most (back when I still had to work with CVS
on Windows), was the diff operation. I needed to know what I changed, to
clean up my temporary debug hacks before committing.
Another really nice thing was to see the history of a file (I guess this
would be "gitk HEAD -- <file>").
> + // store the folder, if provided
Please convert to C style comments.
> diff --git a/menu.c b/menu.c
> index f3e876c..7f034db 100644
> --- a/menu.c
> +++ b/menu.c
> @@ -1,12 +1,13 @@
> #include <shlobj.h>
> -#include <stdarg.h>
> -#include <tchar.h>
> #include <stdio.h>
> #include "menu.h"
> #include "ext.h"
> #include "debug.h"
> #include "systeminfo.h"
>
> +#define LONGEST_MENU_ITEM 40
> +#define MAX_PROCESSING_TIME (60 * 1000)
These may want to be dynamic/configurable, but let's do this commit first,
put a TODO into a comment, and take care of it later.
> + if (CreateProcess(NULL, cmd,
> + NULL, NULL, FALSE, 0,
> + env_for_git(), wd, &si, &pi)) {
Please indent the second and third line either 4 spaces more than the
"if", or two tabs, not just one. But here, I think that all parameters
would fit on one line anyway.
> + if (WAIT_OBJECT_0 == WaitForSingleObject(pi.hProcess,
> + MAX_PROCESSING_TIME)) {
Same goes for this indent.
> +
> + if (! GetExitCodeProcess(pi.hProcess, &status)) {
> + debug_git("[ERROR] GetExitCode failed (%d); "
> + "wd: %s; cmd: %s",
> + GetLastError(), wd, cmd);
> + }
These curly brackets are unnecessary (as a few times in this patch).
> @@ -67,16 +108,52 @@ static STDMETHODIMP query_context_menu(void *p, HMENU
> menu,
> {
> struct git_menu *this_menu = p;
> struct git_data *this_ = this_menu->git_data;
> + char *wd;
> + BOOL bDirSelected = TRUE;
> +
> + UINT original_first = first_command;
> + char menu_item[LONGEST_MENU_ITEM];
> +
> + int status;
>
> if (flags & CMF_DEFAULTONLY)
> return MAKE_HRESULT(SEVERITY_SUCCESS, FACILITY_NULL, 0);
>
> + // figure out the directory
-> C style comment
> + wd = strdup(this_->name);
> + if (! (FILE_ATTRIBUTE_DIRECTORY & GetFileAttributes(wd))) {
Please lose the space after the exclamation mark.
> + /* TODO: when the above block is fixed, we'll just have
> + return MAKE_RESULT(..., build_menu_items()); */
Personally, I prefer multiline comments to start with a "/*" in one line,
continue with " * <text>" on the next line, and end with " */" like this:
/*
* TODO: when the above block is fixed, we'll just have
* return MAKE_RESULT(..., build_menu_items());
*/
But that is just personal preference...
> @@ -147,13 +224,11 @@ static STDMETHODIMP invoke_command(void *p,
> STARTUPINFO si = { sizeof(si) };
> PROCESS_INFORMATION pi;
>
> - TCHAR command[1024];
> + char command[1024];
> const char *wd;
> DWORD dwAttr, fa;
>
> - wsprintf(command, TEXT("wish.exe \"%s/bin/git-gui\""),
> - msys_path());
> -
> + sprintf(command, "wish.exe \"%s/bin/git-gui\"", msys_path());
>
>
This line should be empty, but has white space.
Besides, this whole hunk is unrelated to the commit message, correct?
> @@ -202,7 +277,7 @@ static STDMETHODIMP get_command_string(void *p, UINT id,
>
>
> if (flags & GCS_HELPTEXT) {
> - LPCTSTR text = _T("Launch the GIT Gui in the local or chosen
> directory.");
> + char *text = "Launch the GIT Gui in the local or chosen
> directory.";
Same goes for this hunk...
Okay, reading my comments again, it seems like all of them are negative.
Please bear in mind, though:
- if I did not like your patch, I would not bother reviewing it, so let's
just make this clear once and for all: reviewing a patch is already a
display of respect to the author of the patch.
- quite a few comments are about style, but you told me that you want me
to comment on the style. Besides, I think it is really, really
important for a software project to have more or less one style:
different styles just distract from the content, and besides, let's
admit it: looking at neat code beats looking at messy code.
- I only want the things changed that I do not like, that's why I comment
on them ;-)
- I am a lazy bastard...
Thanks,
Dscho
