Re: [PATCH] contrib/subtree: Remove --annotate

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 5, 2016, 17:33 UTC
Message-ID: <xmqqsi2cj5hu.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1451963101-4901-2-git-send-email-greened@obbligato.org>

David Greene <greened@obbligato.org> writes:

Show 9 quoted lines

> From: "David A. Greene" <greened@obbligato.org>
>
> Remove --annotate.  This obviates the need for an --unannotate
> command, which is both an obvious addition and difficult to define
> due to the numerous ways one might want to specify how to edit
> commit messages.  git has other tools more suited to rewriting
> commit messages and it's easy enough to use them after a subtree
> split.  Such tools include filter-branch, rebase -i and
> commit --amend.

I do not think that "there are other ways to do this" is a good justification for removing a feature, unless it can be shown that nobody is using it, of course.

Show 6 quoted lines

> @@ -319,7 +315,7 @@ copy_commit()
>  			GIT_COMMITTER_NAME \
>  			GIT_COMMITTER_EMAIL \
>  			GIT_COMMITTER_DATE
> -		(printf "%s" "$annotate"; cat ) |
> +		(echo -n ""; cat ) |

I can see that by changing "printf something" with 'echo -n ""', you are making it clear that we are stopping to add that something to the pipeline, but (1) I think the intended effect of running 'echo -n' on an empty string is to do nothing, and (2) 'echo -n' is not portable [*1*], so this leaves a puzzling code that makes future readers scratch their heads.

I wonder why this cannot be simply the removal of the entire line, making the resulting implementation more like this:

                git log -1 --pretty=format:... "$1" |
                (
                        read ... various variables ...
                        export ... various variables ...
        -		(printf "%s" "$annotate"; cat ) |
                        git commit-tree "$2" $3 # reads the rest of stdin
                ) || die "cannot copy"

That is, just feed the remainder of what is coming directly to the command?

[Footnote]

*1* http://pubs.opengroup.org/onlinepubs/9699919799/utilities/echo.html

says """Implementations shall not support any options."""; '-n' comes from BSD and SysV way of supressing the final newline is to end the string with "\c".