Re: [PATCH] Warnings before rebasing -i published history

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 7, 2012, 22:49 UTC
Message-ID: <7vwr3ieo0i.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1339104053-7782-1-git-send-email-Lucien.Kong@ensimag.imag.fr>

Lucien Kong <Lucien.Kong@ensimag.imag.fr> writes:

Show 5 quoted lines

> +# Add a warning notification at the end of each pick or fixup/squash
> +# line of the todo list, providing the picking commit is already
> +# published.
> +warn_published () {
> +	cat "$1" | while read -r command sha1 message

Make it a habit to question yourself whenever you cat a single file and immediately pipe it to elsewhere, i.e.

	cat "$1" | anything

because 99% of the time you are much better off writing

	anything <"$1"

instead.

Show 7 quoted lines

> +	do
> +		test -n "$sha1" || break
> +		if test -n "$(git branch -r --contains "$sha1")"
> +		then
> +			printf "%s\n" "$(sed -e "/"$sha1"/ s|$| [Published]|" "$1")" >"$1"
> +		fi
> +	done

What's inside $() looks like it wants to say something like

	sed -e "/ $sha1 /s/$/ [Published]/" "$1"

but it has a few fishy double-quotes that makes it unclear why $sha1 wants to be outside the quotes.

Why does it need 'printf "%s" $()' in the first place?  Wouldn't

	sed ... >"$1"

sufficient? You let cat read "$1", sed read "$1" and then the loop overwrite "$1", which looks very fishy.

The logic is merely _guessing_ that the commit could have been published, no? The particular remote repository the test happens to find may not be for consumption by other people.

I am afraid that doing this would send users a wrong message that is unnecessarily alarming, especially the marker says "Published" as if it were a confirmed fact.

In short, I am not unsympathetic to the motivation, but I find the resulting user experience (mostly the wording) questionable, and I am not impressed by the implementation very much.