Re: [PATCH 0/5] fc/transport-helper-sync-error-fix rebased

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 20, 2014, 21:52 UTC
Message-ID: <7v7g6jllck.fsf@alter.siamese.dyndns.org>
In-Reply-To: <535438246097c_4c30158d2ec11@nysa.notmuch>

Felipe Contreras <felipe.contreras@gmail.com> writes:

Show 23 quoted lines

> Junio C Hamano wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>> 
>> > Junio C Hamano wrote:
>> >> As I have said in the recent What's cooking reports, the original
>> >> posted here were based on older codebase and needed to be rebased,
>> >> but it had some conflicts and I wanted to see the result double
>> >> checked by the original author before we can merge it to 'next',
>> >> cooked there and hopefully merged to 'master' before tagging -rc1.
>> >> 
>> >> So here is the series that has been queued in 'pu' for the past
>> >> several days.
>> >> 
>> >> Felipe, can you double check it?
>> >
>> > These patches don't help much,...
>> 
>> What do you mean by that, exactly?  As long as your "don't help
>> much" is "would not hurt and will help some even for a small subset
>> of audience", that would be OK, but I am puzzled.
>
> I mean if purpose of these was for me to review the changes you did, it doesn't
> help as much as an interdiff does.

Ah, I misunderstood. The thing was, that the original was based on older codebase and I couldn't apply them to my tree cleanly. It would be unfair for you to expect _me_ to give you an interdiff in such a situation, especially when I am asking you to double check the conflict resolution based on that original.

OK, so I'll advance the topic to the 'next' and hopefully we can merge it to 'master' before rc1.

> There's nothing in Documentation/CodingGuidelines that says multiple piped
> commands should be one on each line even if togther the line doesn't exceed 80
> characters, nor does it says that file names should be between quotes, even if
> the file name can't possibly have spaces.

Heh, we don't spell out every possible rules. That is where "do as surrounding code" comes in.

As to "$1" vs $1 without quotes, I had to check the calling sites of clean_mark and cmp_marks, primarily because these functions do not say what their "$1" is, to see if an unquoted form is safe. The next person who reads the script has to do the same and quoting is an obvious way to avoid it.

If the functions said "$1 is a branch name", it would have been clear that unquoted $1 would be OK, but still, what these functions has to take (especially the "clean" one that takes pathnames) can change in the future, and a quoted form is an obvious way to futureproof and relieve readers and programmers from having to worry about the quoting issues.

So I think it is better to quote these in this particular case.

The pipe is purely subjective readability. I can go either way, but since I was the one who was applying the patch that needed other changes anyway... ;-)