3 ms·

This doesn't look like a bug, that is, something overlooked in the logic. This seems like a deliberately introduced regression. Accepting an option and ignoring

by nine_k 1y ago

This doesn't look like a bug, that is, something overlooked in the logic. This seems like a deliberately introduced regression. Accepting an option and ignoring it is a deliberate action, and not crashing with an error message when an unsupported option is passed must be a deliberate, and wrong, decision.

imiric 1y ago
I would say that Canonical is more at fault in this case. I'm frankly appalled that an essential feature such as system updates didn't have an automated test that would catch this issue immediately after uutils was integrated. Nevermind the fact that this entire replacement of coreutils is done purely out of financial and political rather than technical reasons, and that they're willing to treat their users as guinea pigs. Despicable.
- nine_k 1y ago
  What surprises me is that the job seems rushed. Implementation is incomplete. Testing seems patchy. Things are released seemingly in a hurry, as if meeting a particular deadline was more important for the engineers or managers of a particular department than the qualify of the product as a whole. This feels like a large corporation, in the bad sense.
johnisgood 1y ago
> deliberately introduced regression > deliberate and wrong decision Yeah... I hope "we" will not switch to it just because it is written in Rust. There is much more than just the damn language behind it.
zahlman 1y ago
It's wrong (and coreutils get it right) but I don't see why it would have to be deliberate. It could easily just not occur to someone that the code needs to be tested with invalid options, or that it needs to handle invalid options by aborting rather than ignoring. (That in turn would depend on the crate they're using for argument parsing, I imagine.)
- nine_k 1y ago
  Could parsing the `-r` be added without noticing it somehow? If it was added in bulk, with many other still unsupported option names, why does the program not crash loudly if any such option is used? A fencepost error is a bug. A double-free is a bug. Accepting an unsupported option and silently ignoring it is not, it takes a deliberate and obviously wrong action.
  - aw1621107 1y ago
    At least from what I can find, here's the original version of the changed snippet [0]: let date_source = if let Some(date) = matches.value_of(OPT_DATE) { DateSource::Custom(date.into()) } else if let Some(file) = matches.value_of(OPT_FILE) { DateSource::File(file.into()) } else { DateSource::Now }; And after `-r` support was added (among other changes) [1]: let date_source = if let Some(date) = matches.get_one::<String>(OPT_DATE) { DateSource::Human(date.into()) } else if let Some(file) = matches.get_one::<String>(OPT_FILE) { match file.as_ref() { "-" => DateSource::Stdin, _ => DateSource::File(file.into()), } } else if let Some(file) = matches.get_one::<String>(OPT_REFERENCE) { DateSource::FileMtime(file.into()) } else { DateSource::Now }; Still the same fallback. Not sure one can discern from just looking at the code (and without knowing more about the context, in my case) whether the choice of fallback was intentional and handling the flag was forgotten about. [0]: https://github.com/yuankunzhang/coreutils/commit/850bd9c32d91ea4f933d2f4d3aae5c4b79a71e6f https://github.com/yuankunzhang/coreutils/commit/850bd9c32d9... [1]: https://github.com/yuankunzhang/coreutils/blob/88a7fa7adfa048dabdffc99451d7aba1d9e6a9b6/src/uu/date/src/date.rs https://github.com/yuankunzhang/coreutils/blob/88a7fa7adfa04...
  - zahlman 1y ago
    > Accepting an unsupported option and silently ignoring it is not, it takes a deliberate and obviously wrong action. No, it doesn't. For example, you could have code that recognizes that something "is an option", and silently discards anything that isn't on the recognized list.
    - 1y ago
none_to_remain 1y ago
It certainly doesn't look intentional to me- it looks like at some point someone added "-r" as a valid option, but until this surfaced as a bug, no one actually implemented anything for it (and the logic happens to fall through to using the current date).
- rtpg 1y ago
  a `todo!()` away from something being way more obvious. Unfortunate!

by nine_k 1y ago

imiric 1y ago

I would say that Canonical is more at fault in this case. I'm frankly appalled that an essential feature such as system updates didn't have an automated test that would catch this issue immediately after uutils was integrated. Nevermind the fact that this entire replacement of coreutils is done purely out of financial and political rather than technical reasons, and that they're willing to treat their users as guinea pigs. Despicable.

nine_k 1y ago
What surprises me is that the job seems rushed. Implementation is incomplete. Testing seems patchy. Things are released seemingly in a hurry, as if meeting a particular deadline was more important for the engineers or managers of a particular department than the qualify of the product as a whole. This feels like a large corporation, in the bad sense.

johnisgood 1y ago

> deliberately introduced regression > deliberate and wrong decision Yeah... I hope "we" will not switch to it just because it is written in Rust. There is much more than just the damn language behind it.

zahlman 1y ago

It's wrong (and coreutils get it right) but I don't see why it would have to be deliberate. It could easily just not occur to someone that the code needs to be tested with invalid options, or that it needs to handle invalid options by aborting rather than ignoring. (That in turn would depend on the crate they're using for argument parsing, I imagine.)

nine_k 1y ago
Could parsing the `-r` be added without noticing it somehow? If it was added in bulk, with many other still unsupported option names, why does the program not crash loudly if any such option is used? A fencepost error is a bug. A double-free is a bug. Accepting an unsupported option and silently ignoring it is not, it takes a deliberate and obviously wrong action.
- aw1621107 1y ago
  At least from what I can find, here's the original version of the changed snippet [0]: let date_source = if let Some(date) = matches.value_of(OPT_DATE) { DateSource::Custom(date.into()) } else if let Some(file) = matches.value_of(OPT_FILE) { DateSource::File(file.into()) } else { DateSource::Now }; And after `-r` support was added (among other changes) [1]: let date_source = if let Some(date) = matches.get_one::<String>(OPT_DATE) { DateSource::Human(date.into()) } else if let Some(file) = matches.get_one::<String>(OPT_FILE) { match file.as_ref() { "-" => DateSource::Stdin, _ => DateSource::File(file.into()), } } else if let Some(file) = matches.get_one::<String>(OPT_REFERENCE) { DateSource::FileMtime(file.into()) } else { DateSource::Now }; Still the same fallback. Not sure one can discern from just looking at the code (and without knowing more about the context, in my case) whether the choice of fallback was intentional and handling the flag was forgotten about. [0]: https://github.com/yuankunzhang/coreutils/commit/850bd9c32d91ea4f933d2f4d3aae5c4b79a71e6f https://github.com/yuankunzhang/coreutils/commit/850bd9c32d9... [1]: https://github.com/yuankunzhang/coreutils/blob/88a7fa7adfa048dabdffc99451d7aba1d9e6a9b6/src/uu/date/src/date.rs https://github.com/yuankunzhang/coreutils/blob/88a7fa7adfa04...
- zahlman 1y ago
  > Accepting an unsupported option and silently ignoring it is not, it takes a deliberate and obviously wrong action. No, it doesn't. For example, you could have code that recognizes that something "is an option", and silently discards anything that isn't on the recognized list.
  - 1y ago

none_to_remain 1y ago

It certainly doesn't look intentional to me- it looks like at some point someone added "-r" as a valid option, but until this surfaced as a bug, no one actually implemented anything for it (and the logic happens to fall through to using the current date).

rtpg 1y ago
a `todo!()` away from something being way more obvious. Unfortunate!