On Sat, 22 Dec 2007, Alex Riesen wrote:
>
> + if (!no_edit) {
This is unrelated to the rest of the patch, but I do think double
negations are horrible, so I thoink we should probably make the "no_edit"
flag change meaning, and call it "run_editor" or something.
That said, the thing that I'm actually reacting to is:
> + if (in_merge)
> + fprintf(fp,
> + "#\n"
> + "# It looks like you may be committing a MERGE.\n"
> + "# If this is not correct, please remove the file\n"
> + "# %s\n"
> + "# and try again.\n"
> + "#\n",
> + git_path("MERGE_HEAD"));
> + if (cleanup_mode != CLEANUP_NONE)
> + fprintf(fp,
> + "\n"
> + "# Please enter the commit message for your changes.\n");
> + if (cleanup_mode == CLEANUP_ALL)
> + fprintf(fp,
> + "# (Comment lines starting with '#' will not be included)\n");
> + if (only_include_assumed)
> + fprintf(fp, "# %s\n", only_include_assumed);
The thing that still worries me about this set is that if you have
"cleanup_mode == CLEANUP_NONE" or "cleanup_mode == CLEANUP_SPACE", we're
now adding all these messages, and we depend on the user knowing that *he*
has to remove them.
So I wonder if we should perhaps:
(a) Only add these messages at all when we do *not* do CLEANUP_ALL
In other words, change the
if (!no_edit) {
line to a
if (cleanup_mode == CLEANUP_ALL && !no_edit) {
instead, or
(b) Add a a new line to replace he "will not be included" message, ie
if (cleanup_mode != CLEANUP_ALL)
fprintf(p,
"# Please remove these comment lines manually, "
"they will not be automatically removed\n");
or something similar.
I personally would prefer (a) - since anybody who then explicitly uses
--cleanup={space|none} would presumably already know what he is doing.
But this is not a huge deal. Regardless, the patch looks ok, so you can
add a "Acked-by:" from me.
Linus
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
