Lucien Kong <Lucien.Kong@xxxxxxxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html