Re: [PATCH v2] t6030: add test for git bisect skip started with --term* arguments

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 28, 2021, 17:48 UTC
Message-ID: <CAPig+cSzLFZeOyJAnkXx6M5L+vUKofV+KF8w1MzH+iAzxYycPw@mail.gmail.com>
In-Reply-To: <CAP8UFD10vLX_rF=P374BP-CeE5cxs1Rg6faxqf+m3J4MH9Vb8A@mail.gmail.com>

On Wed, Apr 28, 2021 at 1:17 PM Christian Couder <christian.couder@gmail.com> wrote:

Show 18 quoted lines

> On Wed, Apr 28, 2021 at 6:32 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Wed, Apr 28, 2021 at 4:12 AM Christian Couder
> > <christian.couder@gmail.com> wrote:
> > > On Sun, Apr 25, 2021 at 10:06 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
> > > > +       HASH_SKIPPED_FROM=$(git rev-parse --verify HEAD) &&
> > > > +       HASH_SKIPPED_TO=$(git rev-parse --verify HEAD) &&
> > > > +       test $HASH_SKIPPED_FROM != $HASH_SKIPPED_TO
> > >
> > >        test "$HASH_SKIPPED_FROM" != "$HASH_SKIPPED_TO"
> >
> > Also, is there a reason for upcasing these variable names
> > (HASH_SKIPPED_FROM and HASH_SKIPPED_TO), thus making them appear to be
> > globals even though they are used only in this test? More appropriate
> > and less misleading names might be `skipped_from` and `skipped_to`.
>
> In this test script many variables are called HASH1, HASH2, ... or
> PARA_HASH1, PARA_HASH2, ... So in this regard HASH_SKIPPED_FROM and
> HASH_SKIPPED_TO look ok to me.

Many of the values of the existing uppercase variables -- such as HASH1, HASH2 -- in this script are explicitly and intentionally shared among tests, so they are necessarily global, thus the conventional uppercase names make sense. However, the variables in this test are local to it (and there is no indication that their values will ever be shared with other tests, so using uppercase for the names is misleading and confusing for readers[1].

[1]: It's true that some of the uppercase names in existing tests are local to those tests, as well, so they ought not use uppercase names, and therefore share the same problem of misleading and confusing readers. It would be nice for the naming to be cleaned up, but that's outside the scope of this patch. Admittedly, considering the existing naming inconsistency, questioning the uppercase names in this new test is a relatively minor point, but as a reader, they confuse me and make me spend extra cycles wondering why they need to be global (even though they are not, but their uppercase names mislead me into thinking they are).