git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Duy Nguyen <pclouds@gmail.com>
To: Derrick Stolee <stolee@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Stefan Beller <sbeller@google.com>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 07/14] parse-options: allow ll_callback with OPTION_CALLBACK
Date: Tue, 16 Apr 2019 15:52:45 +0700	[thread overview]
Message-ID: <CACsJy8DDKuK5VYhh0GNYSJK1_y3MZgK5Vcq99N4jYcusVFnvQQ@mail.gmail.com> (raw)
In-Reply-To: <02d2a828-b191-9d9a-7422-d76cdca69ef1@gmail.com>

On Mon, Apr 15, 2019 at 9:06 PM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 1/26/2019 7:35 PM, Nguyễn Thái Ngọc Duy wrote:
> > @@ -238,7 +249,10 @@ static enum parse_opt_result parse_short_opt(struct parse_opt_ctx_t *p,
> >                       len++;
> >               arg = xmemdupz(p->opt, len);
> >               p->opt = p->opt[len] ? p->opt + len : NULL;
> > -             rc = (*numopt->callback)(numopt, arg, 0) ? (-1) : 0;
> > +             if (numopt->callback)
> > +                     rc = (*numopt->callback)(numopt, arg, 0) ? (-1) : 0;
> > +             else
> > +                     rc = (*numopt->ll_callback)(p, numopt, arg, 0);
> >               free(arg);
> >               return rc;
> >       }
>
> Hi Duy,
>
> This "else" condition is unreachable. This block is only hit when we have a "-<n>"
> option, using OPT_NUMBER_CALLBACK, which is implemented by filling "callback", never
> "ll_callback".

It does not mean ll_callback cannot be used in the future though. We
have three options

1. drop the else clause
2. replace with "else BUG();"
3. implement proper else clause

Option #1 to me sounds wrong. If you don't support something, yell up.
Silently ignoring it only makes it harder to track down to this
unsupported location when it becomes reachable, however unlikely that
is.

Which leaves options #2 and #3. If you think this one line is risky
enough, I'll send a patch to replace it with BUG().

> I recommend reverting this diff segment, but please let me know if I'm missing something.
>
> Thanks,
> -Stolee



-- 
Duy

  reply	other threads:[~2019-04-16  8:53 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-27  0:35 [PATCH 00/14] nd/diff-parseopt part 1 Nguyễn Thái Ngọc Duy
2019-01-27  0:35 ` [PATCH 01/14] parse-options.h: remove extern on function prototypes Nguyễn Thái Ngọc Duy
2019-01-27  0:35 ` [PATCH 02/14] parse-options: add one-shot mode Nguyễn Thái Ngọc Duy
2019-01-27  0:35 ` [PATCH 03/14] parse-options: disable option abbreviation with PARSE_OPT_KEEP_UNKNOWN Nguyễn Thái Ngọc Duy
2019-01-27  0:35 ` [PATCH 04/14] parse-options: add OPT_BITOP() Nguyễn Thái Ngọc Duy
2019-01-27  0:35 ` [PATCH 05/14] parse-options: stop abusing 'callback' for lowlevel callbacks Nguyễn Thái Ngọc Duy
2019-01-27  0:35 ` [PATCH 06/14] parse-options: avoid magic return codes Nguyễn Thái Ngọc Duy
2019-01-27  0:35 ` [PATCH 07/14] parse-options: allow ll_callback with OPTION_CALLBACK Nguyễn Thái Ngọc Duy
2019-04-15 14:06   ` Derrick Stolee
2019-04-16  8:52     ` Duy Nguyen [this message]
2019-04-16 14:24       ` Derrick Stolee
2019-01-27  0:35 ` [PATCH 08/14] diff.h: keep forward struct declarations sorted Nguyễn Thái Ngọc Duy
2019-01-27  0:35 ` [PATCH 09/14] diff.h: avoid bit fields in struct diff_flags Nguyễn Thái Ngọc Duy
2019-01-27  0:35 ` [PATCH 10/14] diff.c: prepare to use parse_options() for parsing Nguyễn Thái Ngọc Duy
2019-01-27  0:35 ` [PATCH 11/14] diff.c: convert -u|-p|--patch Nguyễn Thái Ngọc Duy
2019-01-27  0:35 ` [PATCH 12/14] diff.c: convert -U|--unified Nguyễn Thái Ngọc Duy
2019-01-27  0:35 ` [PATCH 13/14] diff.c: convert -W|--[no-]function-context Nguyễn Thái Ngọc Duy
2019-01-27  0:35 ` [PATCH 14/14] diff.c: convert --raw Nguyễn Thái Ngọc Duy
2019-01-28  0:33 ` [PATCH 00/14] nd/diff-parseopt part 1 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=CACsJy8DDKuK5VYhh0GNYSJK1_y3MZgK5Vcq99N4jYcusVFnvQQ@mail.gmail.com \
    --to=pclouds@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=sbeller@google.com \
    --cc=stolee@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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).