Re: [PATCH 2/2] commit: fix ending newline for template files

From: Patryk Obara <patryk.obara@gmail.com>
Date: May 30, 2015, 11:29 UTC
Message-ID: <CAJfL8+RtR+w+NQeFGJ7GPsPYgcn59XvWw8eXL12ph9EHwc14ww@mail.gmail.com>
In-Reply-To: <CAPig+cTrW9f1TGvpr4KH+EcOsy=FWvGRj6ZQM6nsFyXc15c4qg@mail.gmail.com>

@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 <sunshine@sunshineco.com> wrote:

Show 6 quoted lines

> 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 <sunshine@sunshineco.com> 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 <sunshine@sunshineco.com> 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 <gitster@pobox.com> 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 <sunshine@sunshineco.com> 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