Re: [PATCH v4 2/8] t5548: refactor to reuse setup_upstream() function

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 31, 2025, 14:35 UTC
Message-ID: <CAPig+cQLdyuhK4bgf=HDuX8nDdBv8Voqf0-EtQO-8f15sPOVkw@mail.gmail.com>
In-Reply-To: <20250131-pks-push-atomic-respect-exit-code-v4-2-a8b41f01a676@pks.im>

On Fri, Jan 31, 2025 at 5:53 AM Patrick Steinhardt <ps@pks.im> wrote:

Show 20 quoted lines

> Refactor the function setup_upstream_and_workbench(), extracting
> create_upstream_template() and setup_upstream() from it. The former is
> used to create the upstream repository template, while the latter is
> used to rebuild the upstream repository and will be reused in subsequent
> commits.
>
> To ensure that setup_upstream() works properly in both local and HTTP
> protocols, the HTTP settings have been moved to the setup_upstream() and
> setup_upstream_and_workbench() functions.
>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh
> @@ -54,29 +54,67 @@ format_and_save_expect () {
>  setup_upstream_and_workbench () {
> +       if test $# -ne 1
> +       then
> +               BUG "location of upstream repository is not provided"
> +       fi &&

It's not clear why &&-chaining is being used here considering that this function is not called by any tests and is itself not linked into any &&-chains, hence the use of && is superfluous and misleading.

> +       # Assign the first argument to the variable upstream;
> +       # we will use it in the subsequent test cases.
> +       upstream="$1"

Comment doesn't seem to add any value.

(Neither of these are worth a reroll on their own.)