From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 07 Jun 2012 22:49:17 GMT
Subject: Re: [PATCH] Warnings before rebasing -i published history
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:

> +# 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.

> +	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.