Re: [PATCH] fast-import: properly fanout notes when tree is imported

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 20, 2016, 20:56 UTC
Message-ID: <xmqqbmw6jp59.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20161220204841.awvabgwsxudxfzca@glandium.org>

Mike Hommey <mh@glandium.org> writes:

Show 28 quoted lines

> On Tue, Dec 20, 2016 at 11:34:04AM -0800, Junio C Hamano wrote:
>> Mike Hommey <mh@glandium.org> writes:
>> 
>> > In typical uses of fast-import, trees are inherited from a parent
>> > commit. In that case, the tree_entry for the branch looks like:
>> > ...
>> > +# Create another notes tree from the one above
>> > +cat >>input <<INPUT_END
>> > +...
>> > +M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) 
>> 
>> There is a trailing SP that cannot be seen by anybody.
>> 
>> Don't do this.  It makes it very easy to miss what is going on and
>> wastes reviewers' time.
>> 
>> Protect it by doing something like:
>> 
>> 	sed -e 's/Z$//' >>input <<INPUT_END
>> 	...
>> 	M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) Z
>
> How about
> EMPTY=
> ...
> M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) $EMPTY
>
> ?

Notice I said "something like" ;-)

I think you are bringing that up to avoid sed, but if you want to go that route, the long string $EMPTY is distracting, and makes readers wonder why something that is loud but expands to nothing has to be there. It hides the true intention, which is that the SP that comes before it is the most important thing on that line.

I would think a lot more understandable variant would be to do this instead:

	SP=" "
	...
	M a lot of garbage $(and command)$SP