From: Johannes Schindelin <Johannes.Schindelin@gmx.de> To: Junio C Hamano <gitster@pobox.com> Cc: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>, git@vger.kernel.org, "Denton Liu" <liu.denton@gmail.com>, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com> Subject: Re: [PATCH v2 8/8] tests: disallow the use of abbreviated options (by default) Date: Wed, 17 Apr 2019 14:07:25 +0200 (CEST) Message-ID: <nycvar.QRO.7.76.6.1904171354270.46@tvgsbejvaqbjf.bet> (raw) In-Reply-To: <xmqqef63qt5e.fsf@gitster-ct.c.googlers.com> Hi Junio, On Mon, 15 Apr 2019, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > On Mon, 15 Apr 2019, Junio C Hamano wrote: > > > >> Junio C Hamano <gitster@pobox.com> writes: > >> > >> > >> > Do you mean more like > >> > ... > >> > I think I can agree with either of the two positions... > >> > >> I am guessing from the earlier iteration that you wanted to say > >> "unless it is given explicitly, we turn it on". > >> > >> As this last-minute style update that was botched, and a typofix in > >> the proposed log message in 8/8, are the only differences, let me > >> locally fix 8/8 up and replace it. > > > > Sure. I still would like the `isset` thing, as it makes things more > > consistent, but I'll not fight for it. > > ${var:+isset} is fine. Instead of > > +# Disallow the use of abbreviated options in the test suite by default > +if test -z "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS}" > +then > + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true > + export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS > +fi > + > > if you used > > if test -z "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:+isset}" > then > ... > > I won't object. After all, I know I introduced :+isset pattern to > our shell script codebase as there are cases where it makes the > result easier to follow. > > But the thing is that your patch had the polarity inverted. Ah! I finally understand what you are saying. Took me a while, sorry. > Where you must say "if this thing is not set, assign this value", you > said "if this thing is set, assign this value", which was totally bogus. > As long as that is corrected, that's fine. Of course! It should be `-z` instead of `-n` there. > Having said that. > > When you check if the variable is set, use of the ":+isset" pattern > makes the result often easier to follow by explicitly letting us > compare with an explicit "isset" token, e.g. > > case ",${VAR1:+isset},${VAR2:+isset}," in > *,isset,*) : at least one is set ;; > *) : neither is set ;; > esac > > This *does* make the code simpler and easier. But when checking for > "is it not set?", you can compare with an explicit literal "" and > that comparison is plenty clear enough. You won't get as much > benefit as the "is it set?" test would out of the pattern. I would > not say that it is pointless to use the ":+isset" pattern when > checking "is it not set?", but it is very close. Well, there is also the subtelty that somebody might *want* to set that environment variable to the empty string (and obviously they would not deem it appropriate for us to override their choice). But I do have bigger fish to fry, so if you're fine with the `isset` thing (fixed, of course), please go for it. If you'd prefer the version without it, that's fine enough with me, too. Ciao, Dscho
next prev parent reply other threads:[~2019-04-17 12:07 UTC|newest] Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-03-25 18:14 [PATCH 0/8] Do not use abbreviated options in tests Johannes Schindelin via GitGitGadget 2019-03-25 18:14 ` [PATCH 1/8] tests (rebase): spell out the `--keep-empty` option Johannes Schindelin via GitGitGadget 2019-03-25 18:14 ` [PATCH 3/8] t7810: do not abbreviate `--no-exclude-standard` nor `--invert-match` Johannes Schindelin via GitGitGadget 2019-03-25 18:14 ` [PATCH 2/8] tests (rebase): spell out the `--force-rebase` option Johannes Schindelin via GitGitGadget 2019-03-25 18:14 ` [PATCH 4/8] t5531: avoid using an abbreviated option Johannes Schindelin via GitGitGadget 2019-03-25 18:14 ` [PATCH 5/8] tests (push): do not abbreviate the `--follow-tags` option Johannes Schindelin via GitGitGadget 2019-03-25 18:14 ` [PATCH 6/8] tests (status): spell out the `--find-renames` option in full Johannes Schindelin via GitGitGadget 2019-03-25 18:14 ` [PATCH 7/8] tests (pack-objects): use the full, unabbreviated `--revs` option Johannes Schindelin via GitGitGadget 2019-03-25 18:14 ` [PATCH 8/8] tests: disallow the use of abbreviated options (by default) Johannes Schindelin via GitGitGadget 2019-03-25 18:35 ` Denton Liu 2019-03-25 20:26 ` Ævar Arnfjörð Bjarmason 2019-04-12 8:48 ` Johannes Schindelin 2019-04-12 8:50 ` Johannes Schindelin 2019-03-25 19:47 ` Ævar Arnfjörð Bjarmason 2019-04-12 8:59 ` Johannes Schindelin 2019-03-25 20:23 ` [PATCH 0/2] allow for configuring option abbreviation + fix Ævar Arnfjörð Bjarmason 2019-03-25 20:23 ` [PATCH 1/2] parse-options: allow for configuring option abbreviation Ævar Arnfjörð Bjarmason 2019-03-25 21:23 ` Eric Sunshine 2019-03-25 22:47 ` Ævar Arnfjörð Bjarmason 2019-03-26 4:14 ` Duy Nguyen 2019-03-26 6:28 ` Ævar Arnfjörð Bjarmason 2019-03-26 7:13 ` Duy Nguyen 2019-03-26 11:00 ` Ævar Arnfjörð Bjarmason 2019-04-01 10:47 ` Junio C Hamano 2019-04-12 9:06 ` Johannes Schindelin 2019-03-25 20:23 ` [PATCH 2/2] parse-options: don't emit "ambiguous option" for aliases Ævar Arnfjörð Bjarmason 2019-04-17 12:44 ` [PATCH v2] " Ævar Arnfjörð Bjarmason 2019-04-17 16:04 ` Duy Nguyen 2019-04-18 0:48 ` Junio C Hamano 2019-04-18 9:29 ` Duy Nguyen 2019-04-19 4:39 ` Junio C Hamano 2019-04-22 12:22 ` [PATCH] " Nguyễn Thái Ngọc Duy 2019-04-22 12:34 ` Duy Nguyen 2019-04-29 10:05 ` [PATCH v2] " Nguyễn Thái Ngọc Duy 2019-05-07 3:43 ` Junio C Hamano 2019-05-07 11:58 ` Duy Nguyen 2019-04-02 0:58 ` [PATCH 0/8] Do not use abbreviated options in tests Junio C Hamano 2019-04-12 9:37 ` [PATCH v2 " Johannes Schindelin via GitGitGadget 2019-04-12 9:37 ` [PATCH v2 1/8] tests (rebase): spell out the `--keep-empty` option Johannes Schindelin via GitGitGadget 2019-04-12 9:37 ` [PATCH v2 2/8] tests (rebase): spell out the `--force-rebase` option Johannes Schindelin via GitGitGadget 2019-04-12 9:37 ` [PATCH v2 3/8] t7810: do not abbreviate `--no-exclude-standard` nor `--invert-match` Johannes Schindelin via GitGitGadget 2019-04-12 9:37 ` [PATCH v2 4/8] t5531: avoid using an abbreviated option Johannes Schindelin via GitGitGadget 2019-04-12 9:37 ` [PATCH v2 5/8] tests (push): do not abbreviate the `--follow-tags` option Johannes Schindelin via GitGitGadget 2019-04-12 9:37 ` [PATCH v2 6/8] tests (status): spell out the `--find-renames` option in full Johannes Schindelin via GitGitGadget 2019-04-12 9:37 ` [PATCH v2 7/8] tests (pack-objects): use the full, unabbreviated `--revs` option Johannes Schindelin via GitGitGadget 2019-04-12 9:37 ` [PATCH v2 8/8] tests: disallow the use of abbreviated options (by default) Johannes Schindelin via GitGitGadget 2019-04-14 2:35 ` Junio C Hamano 2019-04-15 2:55 ` Junio C Hamano 2019-04-15 13:09 ` Johannes Schindelin 2019-04-15 13:45 ` Junio C Hamano 2019-04-17 12:07 ` Johannes Schindelin [this message] 2019-04-15 13:08 ` Johannes Schindelin
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=nycvar.QRO.7.76.6.1904171354270.46@tvgsbejvaqbjf.bet \ --to=johannes.schindelin@gmx.de \ --cc=avarab@gmail.com \ --cc=git@vger.kernel.org \ --cc=gitgitgadget@gmail.com \ --cc=gitster@pobox.com \ --cc=liu.denton@gmail.com \ /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 the 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