Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

date: catch invalid date arguments #908

Merged
merged 1 commit into from
Jan 6, 2025
Merged

Conversation

mknos
Copy link
Contributor

@mknos mknos commented Jan 6, 2025

  • Extra format string arguments are ambiguous and should result in an error
  • select_format() is meant to select only one format; the use of grep() as a filter prevented it from catching invalid arguments
  • Arguments which are not a valid format string should result in an error
  • Found when testing against GNU date
  • test1: perl date # default in select_format()
  • test2: perl date yo+%a # not a valid date format
  • test3: perl date +%a +%a # too many formats
  • test4: perl date +%a%a # still works

* Extra format string arguments are ambiguous and should result in an error
* select_format() is meant to select only one format; the use of grep() as a filter prevented it from catching invalid arguments
* Arguments which are not a valid format string should result in an error
* test1: perl date    # default in select_format()
* test2: perl date yo+%a   # not a valid date format
* test3: perl date +%a +%a    # too many formats
* test4: perl date +%a%a   # still works
@github-actions github-actions bot added Type: enhancement improve a feature that already exists Priority: low get to this whenever Program: date The date program labels Jan 6, 2025
@mknos mknos had a problem deploying to automated_testing January 6, 2025 04:37 — with GitHub Actions Failure
@mknos mknos had a problem deploying to automated_testing January 6, 2025 04:37 — with GitHub Actions Failure
@mknos mknos had a problem deploying to automated_testing January 6, 2025 04:37 — with GitHub Actions Failure
@mknos mknos had a problem deploying to automated_testing January 6, 2025 04:37 — with GitHub Actions Failure
@mknos mknos had a problem deploying to automated_testing January 6, 2025 04:37 — with GitHub Actions Failure
@briandfoy briandfoy self-assigned this Jan 6, 2025
@briandfoy
Copy link
Owner

changes: all formats must start with + and there can only be one

@briandfoy briandfoy merged commit 1b7c1f4 into briandfoy:master Jan 6, 2025
1 of 22 checks passed
@briandfoy briandfoy added the Status: accepted The fix is accepted label Jan 6, 2025
@briandfoy briandfoy added Type: bug an existing feature does not work and removed Priority: low get to this whenever Type: enhancement improve a feature that already exists labels Jan 6, 2025
briandfoy added a commit that referenced this pull request Jan 7, 2025
@briandfoy
Copy link
Owner

I had to back out this change because it broke date -u, and maybe other things.

@@ -80,8 +80,19 @@ sub run {
}

sub select_format {
my( $supplied ) = grep { /\A\+/ } @_;
return $1 if $supplied =~ /\A\+(.+)/;
my $supplied;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up backing out this change. I should have paid attention to the test failing, but there was another test failing so I just ignored both. That's dumb.

A few things to note here:

  • don't exit from utility subs. In the fix I die and wrap the sub in an eval. That gives the main code a chance to ignore it. Note a huge deal here because it's all private code, but in general it's about separation of concerns.
  • we can't stop if there is an option that doesn't start with + because there are other options possible, and maybe we need better argument processing so nothing else is left.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Program: date The date program Status: accepted The fix is accepted Type: bug an existing feature does not work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants