Re: [PATCH] userdiff: add support for Fountain documents

public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Junio C Hamano <gitster@pobox•com>
To: "Zoë Blade" <zoe@bytenoise•co.uk>
Cc: git@vger•kernel.org
Subject: Re: [PATCH] userdiff: add support for Fountain documents
Date: Tue, 21 Jul 2015 12:33:41 -0700	[thread overview]
Message-ID: <xmqqk2ttuwyy.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: 1437484966-664-1-git-send-email-zoe@bytenoise.co.uk

Zoë Blade <zoe@bytenoise•co.uk> writes:

Hmmmm, the pattern has too many question marks to make a simple
panda brain spin.

> "^((\\.|((int|est|ext)?\\.?|i(nt)?\\.?/e(xt)?\\.?) ).+)$"

it can start with a dot, or match "something" at the beginning,
followed by a SP (is a tab allowed there instead of SP, I have to
wonder).

One problem I noticed immediately: this allows a line that begins
with "...", which I learned in http://fountain.io/syntax when I
wrote $gmane/274127 is explicitly excluded.  A line that begin with
a dot followed by a non-dot is a forced scene heading.

Now disecting that "something" (i.e. not a forced scene heading),
which is this part ...

> ((int|est|ext)?\\.?|i(nt)?\\.?/e(xt)?\\.?)

... that gives us largely two choices:

 - It can begin with zero or one int/est/ext and can optionally be
   followed by a dot, or

 - It can be one of (i, int), optionally followed by a dot, followed
   by slash, followed by one of (e, ext), optionally followed by a
   dot.

Second problem.  Doesn't the early part of this "something" pattern
allow an empty string to match by having zero "int" and zero "."?

With this edit to the test vector (add either one of these two,
removing the other, before you run this test twice), you can see
that these over-eager matches in action.
----------------------------------------------------------------

 t/t4018/fountain-scene | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/t4018/fountain-scene b/t/t4018/fountain-scene
index 6b3257d..94f0513 100644
--- a/t/t4018/fountain-scene
+++ b/t/t4018/fountain-scene
@@ -1,4 +1,7 @@
 EXT. STREET RIGHT OUTSIDE - DAY

+ An indented line is WRONG.
+...we do not want ellipses.
+
 CHARACTER
 You didn't say the magic phrase, "ChangeMe".

----------------------------------------------------------------

Perhaps the pattern is trying to be too clever for its own good.
I'd prefer to see us doing simple, stupid and obviously correct.

That "syntax" page says:

    You can "force" a Scene Heading by starting the line with a
    single period. ... Note that only a single leading period
    followed by an alphanumeric character will force a Scene
    Heading. This allows the writer to begin Action and Dialogue
    elements with ellipses ...

which would give us the first part, i.e. the line may start with a
dot and then an alnum

\\.[a-z0-9]

And then

    A line beginning with any of the following, followed by either a dot
    or a space, is considered a Scene Heading (unless the line is
    preceded by an exclamation point !). Case insensitive.

    INT
    EXT
    EST
    INT./EXT
    INT/EXT
    I/E

which translates to

(int|ext|est|int\\.?/ext|i/e)[. ]

So taking these all together, something like this?

^((\\.[a-z0-9]|(int|ext|est|int\\.?/ext|i/e)[. ]).*)$

I personally prefer to make it slightly lenient to exclude dot
instead of forcing US-ASCII view of alnum, i.e.

^((\\.[^.]|(int|ext|est|int\\.?/ext|i/e)[. ]).*)$

I'll queue a SQUASH??? on top while waiting for a response.

Thanks.

next prev parent reply	other threads:[~2015-07-21 19:33 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-07-21 13:22 [PATCH] userdiff: add support for Fountain documents Zoë Blade
2015-07-21 19:33 ` Junio C Hamano [this message]
2015-07-22 16:31   ` Zoë Blade
2015-07-29 11:19     ` Zoë Blade
  -- strict thread matches above, loose matches on Subject: below --
2015-07-19 12:31 Zoë Blade
2015-07-20 21:17 ` Junio C Hamano
2015-07-21 13:23   ` Zoë Blade
2015-07-17 14:21 Zoë Blade
2015-07-17 17:12 ` Junio C Hamano
2015-07-17 22:43   ` Junio C Hamano
2015-07-19 12:30     ` Zoë Blade
2015-07-17 11:59 Zoë Blade
2015-07-17 13:03 ` Johannes Schindelin
2015-07-17 14:03   ` Zoë Blade
2015-07-17 16:20     ` Johannes Schindelin

find likely ancestor, descendant, or conflicting patches for this message:
( dfblob:6b3257d dfblob:94f0513 )
 OR (
bs:"Re: [PATCH] userdiff: add support for Fountain documents" )
	(help)

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

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

  git send-email \
    --in-reply-to=xmqqk2ttuwyy.fsf@gitster.dls.corp.google.com \
    --to=gitster@pobox$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    --cc=zoe@bytenoise$(echo .)co.uk \
    /path/to/YOUR_REPLY

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

Be sure your reply has a Subject: header at the top and a blank line before the message body.

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox