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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



@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@xxxxxxxxxxxxxx> 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 <sunshine@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]