git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: Nipunn Koorapati via GitGitGadget <gitgitgadget@gmail.com>
Cc: Git List <git@vger.kernel.org>,
	Nipunn Koorapati <nipunn1313@gmail.com>,
	Nipunn Koorapati <nipunn@dropbox.com>
Subject: Re: [PATCH v3 1/3] test-lib-functions: handle --add in test_config
Date: Mon, 21 Dec 2020 02:07:31 -0500
Message-ID: <CAPig+cSaq4vTK7CtvxB2bd0=WTW+d=s0H2RMquyCEf+q0YVn2w@mail.gmail.com> (raw)
In-Reply-To: <733c674bd1901c931a8917045eb72f661872f462.1608516320.git.gitgitgadget@gmail.com>

On Sun, Dec 20, 2020 at 9:05 PM Nipunn Koorapati via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> test_config fails to unset the configuration variable when
> using --add, as it tries to run git config --unset-all --add
>
> Tell test_config to invoke test_unconfig with the arg $2 when
> the arg $1 is --add
>
> Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com>
> ---
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> @@ -381,6 +381,7 @@ test_unconfig () {
>                 config_dir=$1
>                 shift
>         fi
> +       echo git ${config_dir:+-C "$config_dir"} config --unset-all "$@"

Stray debugging gunk?

> @@ -400,7 +401,13 @@ test_config () {
> -       test_when_finished "test_unconfig ${config_dir:+-C '$config_dir'} '$1'" &&
> +
> +       first_arg=$1
> +       if test "$1" = --add; then
> +               first_arg=$2
> +       fi
> +
> +       test_when_finished "test_unconfig ${config_dir:+-C '$config_dir'} '$first_arg'" &&

Several comments...

Style on this project is to place `then` on its own line (as seen a
few lines above this change):

    if test "$1" = --add
    then
        ...

This logic would be easier to understand if the variable was named
`varname` or `cfgvar` (or something), which better conveys intention,
rather than `first_arg`.

It feels odd to single out `--add` when there are other similar
options, such as `--replace-all`, `--fixed-value`, or even `--type`
which people might try using in the future.

This new option parsing is somewhat brittle. If a caller uses
`test_config --add -C <dir> ...`, it won't work as expected. Perhaps
that's not likely to happen, but it would be easy enough to fix by
unifying and generalizing option parsing a bit. Doing so would also
make it easy for the other options mentioned above to be added later
if ever needed. For instance:

    options=
    while test $# != 0
    do
        case "$1" in
        -C)
            config_dir=$2
            shift
            ;;
        --add)
            options="$options $1"
            ;;
        *)
            break
            ;;
        esac
        shift
    done

Finally, as this is a one-off case, it might be simpler just to drop
this patch altogether and open-code the cleanup in the test itself in
patch [2/3] rather than bothering with test_config() in that
particular case. For example:

    test_when_finished "test_unconfig -C two remote.one.push" &&
    git config -C two --add remote.one.push : &&
    test_must_fail git -C two push one &&
    git config -C two --add remote.one.push ^refs/heads/master &&
    git -C two push one

  reply	other threads:[~2020-12-21  7:09 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-19 17:23 [PATCH] negative-refspec: fix segfault on : refspec Nipunn Koorapati via GitGitGadget
2020-12-19 18:05 ` Junio C Hamano
2021-02-19  9:28   ` Jacob Keller
2020-12-19 21:58 ` [PATCH v2 0/2] " Nipunn Koorapati via GitGitGadget
2020-12-19 21:58   ` [PATCH v2 1/2] " Nipunn Koorapati via GitGitGadget
2020-12-20  2:57     ` Eric Sunshine
2020-12-19 21:58   ` [PATCH v2 2/2] negative-refspec: improve comment on query_matches_negative_refspec Nipunn Koorapati via GitGitGadget
2020-12-21  2:05   ` [PATCH v3 0/3] negative-refspec: fix segfault on : refspec Nipunn Koorapati via GitGitGadget
2020-12-21  2:05     ` [PATCH v3 1/3] test-lib-functions: handle --add in test_config Nipunn Koorapati via GitGitGadget
2020-12-21  7:07       ` Eric Sunshine [this message]
2020-12-21 19:00         ` Junio C Hamano
2020-12-21 20:08           ` Eric Sunshine
2020-12-22  0:00             ` Nipunn Koorapati
2020-12-22  0:13               ` Eric Sunshine
2020-12-22  2:25                 ` Nipunn Koorapati
2020-12-22  5:19                   ` Eric Sunshine
2020-12-21  2:05     ` [PATCH v3 2/3] negative-refspec: fix segfault on : refspec Nipunn Koorapati via GitGitGadget
2020-12-21  7:20       ` Eric Sunshine
2020-12-21  2:05     ` [PATCH v3 3/3] negative-refspec: improve comment on query_matches_negative_refspec Nipunn Koorapati via GitGitGadget
2020-12-22  1:11     ` [PATCH v4 0/2] negative-refspec: fix segfault on : refspec Nipunn Koorapati via GitGitGadget
2020-12-22  1:11       ` [PATCH v4 1/2] " Nipunn Koorapati via GitGitGadget
2020-12-22  2:08         ` Junio C Hamano
2020-12-22  2:28           ` Junio C Hamano
2020-12-22  1:11       ` [PATCH v4 2/2] negative-refspec: improve comment on query_matches_negative_refspec Nipunn Koorapati via GitGitGadget
2020-12-22  3:58       ` [PATCH v5 0/2] negative-refspec: fix segfault on : refspec Nipunn Koorapati via GitGitGadget
2020-12-22  3:58         ` [PATCH v5 1/2] " Nipunn Koorapati via GitGitGadget
2021-02-19  9:32           ` Jacob Keller
2020-12-22  3:58         ` [PATCH v5 2/2] negative-refspec: improve comment on query_matches_negative_refspec Nipunn Koorapati via GitGitGadget
2020-12-22  6:48         ` [PATCH v5 0/2] negative-refspec: fix segfault on : refspec Junio C Hamano
2020-12-23 23:56           ` Nipunn Koorapati
2020-12-24  0:00             ` Junio C Hamano
2021-01-11 20:22               ` Nipunn Koorapati
2021-01-12  2:01                 ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAPig+cSaq4vTK7CtvxB2bd0=WTW+d=s0H2RMquyCEf+q0YVn2w@mail.gmail.com' \
    --to=sunshine@sunshineco.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=nipunn1313@gmail.com \
    --cc=nipunn@dropbox.com \
    --subject='Re: [PATCH v3 1/3] test-lib-functions: handle --add in test_config' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git