Re: [PATCH 2/2] commit: fix ending newline for template files
On Sat, May 30, 2015 at 7:29 AM, Patryk Obara wrote: > On Thu, May 28, 2015 at 4:29 PM, Eric Sunshine > wrote: >> Did you consider the alternate approach of handling newline processing >> immediately upon loading 'logfile' and 'template_file', rather than >> delaying processing until this point? Doing it that way would involve >> a bit of code repetition but might be easier to reason about since it >> would occur before possible interactions in following code (such as >> --signoff handling). > > Yes. I opted to place it in here, because newline was appended previously > also in "if (use_editor)" block. But I agree, appending this newline after > loading file will be cleaner - and code repetition may be avoided, if I'll > separate file loading code into new function. A need for this sort of functionality has come up before, so it might be reasonable to introduce a new strbuf function for appending a character if missing. In addition to the 'newline' case, appending '/' to a pathname is also somewhat common. > On Sat, May 30, 2015 at 12:25 AM, Eric Sunshine > wrote: >> If the user specified with the --cleanup option not to >> clean-up the result coming back from the editor, then the commented >> material needs to be removed in the editor by the user *anyway*. You misattributed this statement. It was from Junio, not I. > Why? Is it not ok to leave lines starting with hash in commit object? > --cleanup=whitespace|verbatim suggests, that it's a valid usecase. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] commit: fix ending newline for template files
Eric Sunshine writes: > On Fri, May 29, 2015 at 4:17 PM, Junio C Hamano wrote: >> By default, we should run clean-up after the editor we spawned gives >> us the edited result. Not adding one more LF after the template >> when it already ends with LF would not hurt, but an extra blank >> after the template material does not hurt, either, so I am honestly >> indifferent. > > I had a similar reaction. The one salient bit I picked up was that > Patryk finds it aesthetically offensive[1] when the template ends with > a comment line, and that comment line does not flow directly into the > comment lines provided by --status. That is: > > Template line 1 > # Template line 2 > > # Please enter the commit message... > # with '#' will be ignored... > > [1]: Quoting from the commit message of patch 1/2: "...which is very > annoying when template ends with line starting with '#'" As I said in the message you are responding to, I do not think it would hurt if we stopped adding an LF after a template that already ends with LF. I think I am OK with a patch that does so without doing anything else, like changing when clean-up happens, etc. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] commit: fix ending newline for template files
@Eric, Junio Thank you a lot for feedback - should I post new set of patches as new thread with new cover letter, or reply to first mail in this thread? On Thu, May 28, 2015 at 4:29 PM, Eric Sunshine wrote: > Did you consider the alternate approach of handling newline processing > immediately upon loading 'logfile' and 'template_file', rather than > delaying processing until this point? Doing it that way would involve > a bit of code repetition but might be easier to reason about since it > would occur before possible interactions in following code (such as > --signoff handling). Yes. I opted to place it in here, because newline was appended previously also in "if (use_editor)" block. But I agree, appending this newline after loading file will be cleaner - and code repetition may be avoided, if I'll separate file loading code into new function. On Thu, May 28, 2015 at 4:29 PM, Eric Sunshine wrote: > Moreover, it lacks justification and explanation of why you consider > the cleanup unnecessary. History [1] indicates that its application to > -F but not -t was intentional. That commit suggests, that cleanup was unintentional in one case, it says nothing about it being intentional for -F. Short story: currently cleanup on commit msg is performed many times (I am not sure if "only 2 times" or maybe more. I'll include more detailed analysis with second round of patches :) On Sat, May 30, 2015 at 12:25 AM, Eric Sunshine wrote: > I had a similar reaction. The one salient bit I picked up was that > Patryk finds it aesthetically offensive[1] (...) Yes, that is exactly issue, that I initially wanted to solve. I didn't even notice, that my template had newline appended until I ran git-commit in gdb. Then I saw, that I can't actually test changes to newlines and rest followed, because I didn't want to leave code with more tests disabled. nano, vim, gedit (and other editors, I guess) append _and_hide_ \n before eof from user in default configuration. This newline appended by git before status is completely unexpected (and unwanted) behaviour IMHO. On Fri, May 29, 2015 at 4:17 PM, Junio C Hamano wrote: > in other words, "if your template ends with an incomplete line and > it causes you trouble, then do not do that!". 1) But problem occurs, when template ends with complete line. To make it disappear, user needs to somehow remove trailing newline from his file. In vim it involves switching to non-default binary mode, in nano or gedit it's impossible. Anwer "use emacs" would be a bit disrespectful towards end user ;) 2) That commit addresses different issue - when user intentionally left whitespace in template file, then commit should not clean it up, because it might've beed a "form" to be filled. 3) Well, the exact same logic can be applied to logfile - it does not explain why logfiles and template files should be treated differently in this regard. In fact, when looking at 8b1ae67, I think that lack of this cleanup for logfiles might be an unintended ommision. After another look at that commit - included test doesn't actually verify implemented change (commit msg is stripped and "commit_msg_is" doesn't verify newlines anyway). On Sat, May 30, 2015 at 12:25 AM, Eric Sunshine wrote: > If the user specified with the --cleanup option not to > clean-up the result coming back from the editor, then the commented > material needs to be removed in the editor by the user *anyway*. Why? Is it not ok to leave lines starting with hash in commit object? --cleanup=whitespace|verbatim suggests, that it's a valid usecase. -- | ← Ceci n'est pas une pipe Patryk Obara -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] commit: fix ending newline for template files
On Fri, May 29, 2015 at 4:17 PM, Junio C Hamano wrote: > By default, we should run clean-up after the editor we spawned gives > us the edited result. Not adding one more LF after the template > when it already ends with LF would not hurt, but an extra blank > after the template material does not hurt, either, so I am honestly > indifferent. I had a similar reaction. The one salient bit I picked up was that Patryk finds it aesthetically offensive[1] when the template ends with a comment line, and that comment line does not flow directly into the comment lines provided by --status. That is: Template line 1 # Template line 2 # Please enter the commit message... # with '#' will be ignored... [1]: Quoting from the commit message of patch 1/2: "...which is very annoying when template ends with line starting with '#'" > If the user specified with the --cleanup option not to > clean-up the result coming back from the editor, then the commented > material needs to be removed in the editor by the user *anyway*, so > one more LF would not make that much of a difference in that case, > either. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] commit: fix ending newline for template files
Eric Sunshine writes: > On Thu, May 28, 2015 at 2:22 PM, Junio C Hamano wrote: >> Eric Sunshine writes: >> >>> Moreover, it lacks justification and explanation of why you consider >>> the cleanup unnecessary. History [1] indicates that its application to >>> -F but not -t was intentional. >>> >>> [1]: bc92377 (commit: fix ending newline for template files, 2015-05-26) >> >> Sorry, but the date of that commit seems to be too new to be >> considered "history"; I do not seem to have it, either. > > Indeed, I somehow botched that. I meant: 8b1ae67 (Do not strip empty > lines / trailing spaces from a commit message template, 2011-05-08) Yeah, that was what I had in mind when I read your response. And that one is pretty strong in its own opinion on the "issue" that was brought up by [PATCH 1/2] being discussed, which was: git-commit with -t or -F -e uses content of user-supplied file as initial value for commit msg in editor. There is no guarantee, that this file ends with newline ... The log message of 8b1ae67 argues: Templates should be just that: A form that the user fills out, and forms have blanks. If people are attached to not having extra whitespace in the editor, they can simply clean up their templates. in other words, "if your template ends with an incomplete line and it causes you trouble, then do not do that!". As a general principle I am OK with that. By default, we should run clean-up after the editor we spawned gives us the edited result. Not adding one more LF after the template when it already ends with LF would not hurt, but an extra blank after the template material does not hurt, either, so I am honestly indifferent. If the user specified with the --cleanup option not to clean-up the result coming back from the editor, then the commented material needs to be removed in the editor by the user *anyway*, so one more LF would not make that much of a difference in that case, either. So... -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] commit: fix ending newline for template files
On Thu, May 28, 2015 at 2:22 PM, Junio C Hamano wrote: > Eric Sunshine writes: > >> Moreover, it lacks justification and explanation of why you consider >> the cleanup unnecessary. History [1] indicates that its application to >> -F but not -t was intentional. >> >> [1]: bc92377 (commit: fix ending newline for template files, 2015-05-26) > > Sorry, but the date of that commit seems to be too new to be > considered "history"; I do not seem to have it, either. Indeed, I somehow botched that. I meant: 8b1ae67 (Do not strip empty lines / trailing spaces from a commit message template, 2011-05-08) > But I agree with you that I too failed to see why this change is > necessary or desirable in the explanation in the proposed log > message. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] commit: fix ending newline for template files
Eric Sunshine writes: > Moreover, it lacks justification and explanation of why you consider > the cleanup unnecessary. History [1] indicates that its application to > -F but not -t was intentional. > > [1]: bc92377 (commit: fix ending newline for template files, 2015-05-26) Sorry, but the date of that commit seems to be too new to be considered "history"; I do not seem to have it, either. But I agree with you that I too failed to see why this change is necessary or desirable in the explanation in the proposed log message. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] commit: fix ending newline for template files
On Tue, May 26, 2015 at 2:15 AM, Patryk Obara wrote:
> git-commit with -t or -F -e uses content of user-supplied file as
> initial value for commit msg in editor. There is no guarantee, that this
> file ends with newline - it depends on file content and editor used to
> create file (some editors append and hide last newline from user while
> others do not).
>
> When --status (default) is supplied, additional comment is placed after
> template content. If template file ended with newline this results in
> additional line being appended (which may be unexpected e.g. when last
> line of template is a comment). On the other hand, first line of status
> should never be concatenated to last line of template file.
>
> Append newline before status _only_ if template/logfile didn't end with
> one already. This way content of template is exactly the way user intended
> and there's no chance, that line of status will merge with last line of
> template.
There is also interaction with --signoff (which does its own handling
of present or missing newline)...
> Remove unnecessary premature cleanup of commit message, which was
> implemented for -F, but not for -t.
Is this change distinct from the rest of the patch? If so, it may
deserve its own patch.
Moreover, it lacks justification and explanation of why you consider
the cleanup unnecessary. History [1] indicates that its application to
-F but not -t was intentional.
[1]: bc92377 (commit: fix ending newline for template files, 2015-05-26)
> Signed-off-by: Patryk Obara
> ---
> diff --git a/builtin/commit.c b/builtin/commit.c
> index da79ac4..eb41e05 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -666,8 +666,8 @@ static int prepare_to_commit(const char *index_file,
> const char *prefix,
> struct strbuf sb = STRBUF_INIT;
> const char *hook_arg1 = NULL;
> const char *hook_arg2 = NULL;
> - int clean_message_contents = (cleanup_mode != CLEANUP_NONE);
> int old_display_comment_prefix;
> + int sb_ends_with_newline = 0;
What does 'sb' mean in sb_ends_with_newline? Is it a reference to
strbuf? If so, it doesn't make the variable name any more meaningful.
> /* This checks and barfs if author is badly specified */
> determine_author_info(author_ident);
> @@ -786,6 +782,9 @@ static int prepare_to_commit(const char *index_file,
> const char *prefix,
>
> if (auto_comment_line_char)
> adjust_comment_line_char(&sb);
> +
> + sb_ends_with_newline = ends_with(sb.buf, "\n");
> +
> strbuf_release(&sb);
>
> /* This checks if committer ident is explicitly given */
> @@ -794,6 +793,10 @@ static int prepare_to_commit(const char *index_file,
> const char *prefix,
> int ident_shown = 0;
> int saved_color_setting;
> struct ident_split ci, ai;
> + int append_newline = (template_file || logfile) ?
> !sb_ends_with_newline : 1;
> +
> + if (append_newline)
> + fprintf(s->fp, "\n");
Did you consider the alternate approach of handling newline processing
immediately upon loading 'logfile' and 'template_file', rather than
delaying processing until this point? Doing it that way would involve
a bit of code repetition but might be easier to reason about since it
would occur before possible interactions in following code (such as
--signoff handling).
> if (whence != FROM_COMMIT) {
> if (cleanup_mode == CLEANUP_SCISSORS)
> @@ -815,7 +818,6 @@ static int prepare_to_commit(const char *index_file,
> const char *prefix,
> : "CHERRY_PICK_HEAD"));
> }
>
> - fprintf(s->fp, "\n");
> if (cleanup_mode == CLEANUP_ALL)
> status_printf(s, GIT_COLOR_NORMAL,
> _("Please enter the commit message for your
> changes."
> --
> 2.4.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] commit: fix ending newline for template files
git-commit with -t or -F -e uses content of user-supplied file as
initial value for commit msg in editor. There is no guarantee, that this
file ends with newline - it depends on file content and editor used to
create file (some editors append and hide last newline from user while
others do not).
When --status (default) is supplied, additional comment is placed after
template content. If template file ended with newline this results in
additional line being appended (which may be unexpected e.g. when last
line of template is a comment). On the other hand, first line of status
should never be concatenated to last line of template file.
Append newline before status _only_ if template/logfile didn't end with
one already. This way content of template is exactly the way user intended
and there's no chance, that line of status will merge with last line of
template.
Remove unnecessary premature cleanup of commit message, which was
implemented for -F, but not for -t.
Signed-off-by: Patryk Obara
---
builtin/commit.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index da79ac4..eb41e05 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -666,8 +666,8 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
struct strbuf sb = STRBUF_INIT;
const char *hook_arg1 = NULL;
const char *hook_arg2 = NULL;
- int clean_message_contents = (cleanup_mode != CLEANUP_NONE);
int old_display_comment_prefix;
+ int sb_ends_with_newline = 0;
/* This checks and barfs if author is badly specified */
determine_author_info(author_ident);
@@ -737,7 +737,6 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
if (strbuf_read_file(&sb, template_file, 0) < 0)
die_errno(_("could not read '%s'"), template_file);
hook_arg1 = "template";
- clean_message_contents = 0;
}
/*
@@ -775,9 +774,6 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
*/
s->hints = 0;
- if (clean_message_contents)
- stripspace(&sb, 0);
-
if (signoff)
append_signoff(&sb, ignore_non_trailer(&sb), 0);
@@ -786,6 +782,9 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
if (auto_comment_line_char)
adjust_comment_line_char(&sb);
+
+ sb_ends_with_newline = ends_with(sb.buf, "\n");
+
strbuf_release(&sb);
/* This checks if committer ident is explicitly given */
@@ -794,6 +793,10 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
int ident_shown = 0;
int saved_color_setting;
struct ident_split ci, ai;
+ int append_newline = (template_file || logfile) ?
!sb_ends_with_newline : 1;
+
+ if (append_newline)
+ fprintf(s->fp, "\n");
if (whence != FROM_COMMIT) {
if (cleanup_mode == CLEANUP_SCISSORS)
@@ -815,7 +818,6 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
: "CHERRY_PICK_HEAD"));
}
- fprintf(s->fp, "\n");
if (cleanup_mode == CLEANUP_ALL)
status_printf(s, GIT_COLOR_NORMAL,
_("Please enter the commit message for your
changes."
--
2.4.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
