Page MenuHomePhabricator

rhg: implement the debugignorerhg subcommand
ClosedPublic

Authored by aalekseyev on Oct 27 2021, 7:42 AM.

Details

Summary

This can be used to inspect the generated pattern, but also to benchmark
the time it takes to parse hgignore.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

aalekseyev created this revision.Oct 27 2021, 7:42 AM
aalekseyev retitled this revision from rhg: implement the rhg-debugignore subcommand to rhg: implement the debugignorerhg subcommand.Oct 27 2021, 7:44 AM
aalekseyev added a comment.EditedOct 27 2021, 7:52 AM

<a comment posted in the wrong thread by accident>

aalekseyev updated this revision to Diff 30993.Oct 27 2021, 7:56 AM
aalekseyev updated this revision to Diff 30997.Oct 27 2021, 2:48 PM

I'll look at these patches and take them after 6.0 is out (this includes the one I've already accepted). It's been delayed quite a bit due to Windows issues.

Alphare requested changes to this revision.Nov 24 2021, 4:48 AM

Thanks for waiting for so long. 6.0 is out (it was delayed a lot for Windows reasons), so I'll be back to reviewing patches for default.

rust/hg-core/src/matchers.rs
602

It would be nice to have a type alias for this (and the one below). We already have one in hg-core/src/dirstate/status.rs, probably should be refactored.

rust/rhg/src/commands/debugignorerhg.rs
18

nit: we prefer new commands to have dashes in them now, but it's really not a big deal since this is temporary anyway

This revision now requires changes to proceed.Nov 24 2021, 4:48 AM
aalekseyev edited the summary of this revision. (Show Details)Nov 26 2021, 2:35 PM
aalekseyev updated this revision to Diff 31169.
aalekseyev updated this revision to Diff 31170.Nov 26 2021, 2:42 PM
aalekseyev updated this revision to Diff 31171.Nov 26 2021, 2:45 PM
aalekseyev updated this revision to Diff 31172.Nov 26 2021, 2:49 PM
aalekseyev added inline comments.Nov 26 2021, 2:49 PM
rust/hg-core/src/matchers.rs
602

I used the alias from status.rs now.

rust/rhg/src/commands/debugignorerhg.rs
18

I'm not sure how to do that, actually. I tried renaming the command to debugignorerhg, but got an error:

468 |     debugignore-rhg
    |                ^ no rules expected this token in macro call
Alphare accepted this revision.Nov 29 2021, 6:08 AM
Alphare added inline comments.
rust/rhg/src/commands/debugignorerhg.rs
18

Ah right, we'd need a smarter macro, this is well out of scope

This revision is now accepted and ready to land.Nov 29 2021, 6:08 AM
Alphare requested changes to this revision.Nov 29 2021, 6:18 AM

Actually this also results in a folded commit, please re-send

This revision now requires changes to proceed.Nov 29 2021, 6:18 AM
aalekseyev edited the summary of this revision. (Show Details)Nov 29 2021, 8:03 AM
aalekseyev updated this revision to Diff 31185.

@Alphare, folded commit seems better than the separate ones in this case, though, since the second commit is simply incorporating review comments, not really an independent change.
I ran hg fold --from .^ followed by hg phabsend . to make this less confusing.

If you were thinking of a certain logical split of this commit into two that I'm not aware of, please let me know.

@Alphare, folded commit seems better than the separate ones in this case, though, since the second commit is simply incorporating review comments, not really an independent change.
I ran hg fold --from .^ followed by hg phabsend . to make this less confusing.
If you were thinking of a certain logical split of this commit into two that I'm not aware of, please let me know.

Sure, most of the time, changes can be incorporated into the same changeset, but for ones that move a lot of code around (like refactor suggestions), we usually like to split them. This has a two-fold advantage of making the review of the actual change more obvious, and to make sure a mistake didn't appear in the refactor.

In this case, I should have been more explicit: I saw two concatenated commit messages and figured you already had two changesets, so I asked you to send them as separate.

Basically: one changeset == one phab diff

@Alphare, Ok. I understood you before, but those commits were not suitable for independent submission.

I only have one commit now. Would you like me to split something out into a separate commit?

Alphare requested changes to this revision.Nov 29 2021, 8:52 AM

Yes, I think in this case a separate changeset for the type refactor (of the old code) would make sense.

Also I was sure I added a comment about the println! usage, but I must have forgotten!

rust/rhg/src/commands/debugignorerhg.rs
38

This should use ui.write_stdout, which would also remove the need for the potentially lossy utf8 conversion.

This revision now requires changes to proceed.Nov 29 2021, 8:52 AM
aalekseyev edited the summary of this revision. (Show Details)
aalekseyev updated this revision to Diff 31187.
aalekseyev marked an inline comment as done.Nov 29 2021, 9:11 AM

Done. The refactor part was extracted into https://phab.mercurial-scm.org/D11818.

Alphare accepted this revision.Nov 29 2021, 9:12 AM

Thanks, both look good.

This revision is now accepted and ready to land.Nov 29 2021, 9:12 AM
This revision was automatically updated to reflect the committed changes.