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

Lucien Kong [off-list ref] 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.

Junio C Hamano [off-list ref] a écrit :

quoted

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

The fact is that the input of sed is "$1" itself. An redirecting the output
of sed to the same input doesn't seem to work.

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.

True. It would be annoying for people that rewrite consciously published
history. From this point of view, a better idea would be to show the
remote branch name, as Matthieu suggested, so that the user could know
which branch is involved.