Alex Riesen <raa.lkml@xxxxxxxxx> writes:
> Junio C Hamano, Fri, Dec 21, 2007 22:43:49 +0100:
>> Alex Riesen <raa.lkml@xxxxxxxxx> writes:
>>
>> > The patch is on top of my previos patches. Junio, if you wish, I can
>> > squash and resend.
>>
>> That would be a sane thing to do.
>
> Done
Thanks. I am afraid I have to ask you to either refute my
misunderstanding or a second round.
> +--cleanup=<mode>::
> + This option sets how the commit message is cleaned up.
> + The '<mode>' can be one of 'verbatim', 'whitespace', 'strip',
> + and 'default'. The 'default' mode will strip leading and
> + trailing empty lines and #commentary from the commit message
> + only if the message is to be edited. Otherwise only whitespace
> + removed. The 'verbatim' mode wont change message at all,
> + 'whitespace' removes just leading/trailing whitespace lines
> + and 'strip' removes both whitespace and commentary.
(minor) s/wont/does not/
> @@ -394,20 +410,24 @@ static int prepare_log_message(const char *index_file, const char *prefix)
> return !!DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES);
> }
>
> + if (!no_edit) {
> + 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_DEFAULT)
> + fprintf(fp,
> + "# (Comment lines starting with '#' will not be included)\n");
Can't cleanup_mode be CLEANUP_ALL at this point, and if so
shouldn't this insn be given as well?
In addition, if:
- we are talking with editor, and
- the mode is verbatim, and
- we added any "#" line ourselves (e.g. in_merge adds the insn
shown above, or perhaps we added "git status" output),
then perhaps we would want to say that "#" lines need to be
stripped by the user; otherwise it will be left in the commit.
> @@ -431,10 +451,13 @@ static int message_is_empty(struct strbuf *sb, int start)
> const char *nl;
> int eol, i;
>
> + if (cleanup_mode == CLEANUP_NONE && sb->len)
> + return 0;
> +
> /* See if the template is just a prefix of the message. */
> strbuf_init(&tmpl, 0);
> if (template_file && strbuf_read_file(&tmpl, template_file, 0) > 0) {
> - stripspace(&tmpl, 1);
> + stripspace(&tmpl, !no_edit);
We at this point know that some stripping would happen. The
template needs to be cleaned the same way as the commit message
was stripped. Is checking no_edit the right thing to do? What
if cleanup_mode was CLEANUP_ALL and there is no editing going
on?
> @@ -813,7 +846,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
> if (p != NULL)
> strbuf_setlen(&sb, p - sb.buf + 1);
>
> - stripspace(&sb, 1);
> + if (cleanup_mode != CLEANUP_NONE)
> + stripspace(&sb, cleanup_mode == CLEANUP_DEFAULT ?
> + !no_edit: cleanup_mode == CLEANUP_ALL);
When writing such a long ? : expression, laying it out like:
condition
? if-true
: if-false
would be easier to read. You tilt your head sideways 90
degrees, just like you read a smiley ;-), and you will see the
parse tree.
But I suspect, assuming that the two issues around CLEANUP_DEFAULT vs
CLEANUP_ALL I mentioned above are indeed bugs, it may be cleaner
to:
- parse the options; you get one of NONE/DEFAULT/SPACE/ALL
- convert DEFAULT to SPACE or ALL if editor is in effect.
- do not worry about no_editor or DEFAULT in the rest of the
code when calling stripspace(). The only values you will
see are NONE, SPACE or ALL.
> diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
> index 21ac785..6219378 100755
> --- a/t/t7502-commit.sh
> +++ b/t/t7502-commit.sh
> @@ -89,4 +89,44 @@ test_expect_success 'verbose' '
>
> '
>
> +test_expect_success 'verbatim commit messages' '
> +
These tests check too many things at once, and it would be very
hard to diagnose when breakage does happen. Please split them
perhaps into three separate tests like this:
> + echo >>negative &&
> + { echo;echo "# text";echo; } >expect &&
> + git commit --cleanup=verbatim -t expect -a &&
> + git cat-file -p HEAD |sed -e "1,/^\$/d" |head -n 3 >actual &&
> + diff -u expect actual &&
This is one test --- making sure the commit body match the
unedited template verbatim, except the status part to be shown
in the editor. But we are not using editor in this test and I
thought the point of this --cleanup=verbatim exercise was that
the "status" part is not even have to be added to the commit log
message when editor is not in effect... Isn't the "head -n 3"
just sweeping a bug under the rug?
> + echo >>negative &&
> + git commit --cleanup=verbatim -F expect -a &&
> + git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
> + diff -u expect actual &&
And this is another test.
> + echo >>negative &&
> + git commit --cleanup=verbatim -m "$(cat expect)" -a &&
> + git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
> + diff -u expect actual
And this is yet another.
> +'
Also, the tests do not check "-F file -e", "-m msg -e" etc. We
would want to make sure the right insn are shown to the user in
the editor, wouldn't we?
-
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
