I stumbled over these issues (not really bugs) when I tested what happens if
we try to "fake" a tag. Fortunately this is handled by the UI layer, but the
way it is handled should be improved IMO.

2 patches for repository http://darcs.net/screened:

patch 88b3d7ced3ddbb81565ce039a33a410dc1c46f38
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Aug 24 15:03:14 CEST 2019
  * rename actually_get_log to get_log_using_editor

patch 238ffe4bb9aa170249bc359c64d81355ff5b8a82
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Aug 24 14:01:09 CEST 2019
  * fix prompting when we get a bad patch name
  
  If the user gives a patch name on the command line and that turns out to be
  bad, we want darcs to fail and not prompt the user. Think of using 'darcs
  record' in a script...
  Also, if we do prompt for a patch name and get a bad name, we should explain
  why it is bad before prompting again.
  Finally, when we get a bad name from reading a log file or from launching an
  editor, we also want to fail, rather than prompting the user.

Attachments

• patch-preview.txt
• rename-actually_get_log-to-get_log_using_editor.dpatch
• unnamed

Not screening yet as this is a change in behavior.

Ganesh, could you take a quick look at the patch description and tell me
if you agree with the UI change? Detailed review can wait for later but
I would like to screen this.

I'm fine with the UI change.

>  * fix prompting when we get a bad patch name

Fine

>  * fix prompting when we get a bad patch name

The duplication between check_badname and prompt_patchname could be
factored out. We just need is_badname :: String -> Maybe String.

Rest looks fine.

Following up on review. Note sure if this is strictly better. The
duplication is removed but the code is a perhaps harder to read now.

1 patch for repository http://darcs.net/screened:

patch 83ac6de5ab032ae7b1a418535efcd57b4c464b60
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Aug 31 03:02:24 CEST 2019
  * factor out common code of check_badname and prompt_patchname

Attachments

• factor-out-common-code-of-check_badname-and-prompt_patchname.dpatch
• patch-preview.txt
• unnamed

> Following up on review. Note sure if this is strictly better. The
> duplication is removed but the code is a perhaps harder to read now.

I think if it is harder to read, it's because of the use of 'maybe'
rather than an explicit case.

If you changed

  check_badname n =
    if null n then
      fail "The patch name must not be empty!"
    else if "TAG " `isPrefixOf` n then
      fail "The patch name must not start with \"TAG \"!"
    else
      return ()

into

  check_badname n =
    case just_a_badname n of
      Just err -> fail err
      Nothing -> return ()

Then I think it'd be very clear.

  check_badname = maybe (return ()) fail . just_a_badname

is obviously equivalent when we think about it, but perhaps a bit harder
to immediately follow. (But either is fine with me)

I have just screened my follow-up patch as is.