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

Handle APFS returning hash order. #3316

Merged
merged 13 commits into from Oct 18, 2017

Conversation

Projects
None yet
5 participants
@DomT4
Copy link
Contributor

DomT4 commented Oct 14, 2017

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you successfully run brew tests with your changes locally?

Fixes some of the more obvious cases of #3311 at least. Note I haven't marked this as closing the other issue in case I've missed things, which seems likely, I only did a glance through.

Before:

~> brew commands
Built-in commands
reinstall        .DS_Store        update-reset     desc             cat              command          pin
cask             analytics        search           vendor-install   --cache          log              postinstall
migrate          install          unpack           irb              diy              style            deps
commands         help             unlink           cleanup          sh               options          missing
--version        info             prune            update-report    untap            tap-pin          tap-unpin
doctor           unlinkapps       gist-logs        update           config           tap-info
--prefix         --env            readall          upgrade          home             --repository
unpin            leaves           tap              switch           list             link
outdated         uninstall        --cellar         linkapps         uses             fetch

Built-in developer commands
update-test                   release-notes                 aspell-dictionaries           pull
bottle                        bump-formula-pr               man                           linkage
tests                         edit                          audit                         formula
tap-new                       test                          mirror                        create

External commands
autoupdate                    fix                           prof                          services
bundle                        for-each-formula              pry                           vulnchecker
command-not-found-init        go-resources                  resource-versions             which-formula
fetch-gpg                     manpage-missing-commands      ruby                          which-update

After:

~> brew commands
Built-in commands
--cache          cat              fetch            linkapps         prune            tap-pin          update-report
--cellar         cleanup          gist-logs        list             readall          tap-unpin        update-reset
--env            command          help             log              reinstall        uninstall        upgrade
--prefix         commands         home             migrate          search           unlink           uses
--repository     config           info             missing          sh               unlinkapps       vendor-install
--version        deps             install          options          style            unpack
.DS_Store        desc             irb              outdated         switch           unpin
analytics        diy              leaves           pin              tap              untap
cask             doctor           link             postinstall      tap-info         update

Built-in developer commands
aspell-dictionaries           create                        man                           tap-new
audit                         edit                          mirror                        test
bottle                        formula                       pull                          tests
bump-formula-pr               linkage                       release-notes                 update-test

External commands
autoupdate                    fix                           prof                          services
bundle                        for-each-formula              pry                           vulnchecker
command-not-found-init        go-resources                  resource-versions             which-formula
fetch-gpg                     manpage-missing-commands      ruby                          which-update

Before:

~> brew search homebrew/php/php
==> Searching taps on GitHub...
homebrew/php/brew-php-switcher                               homebrew/php/php-cs-fixer
homebrew/php/php-install                                     caskroom/cask/netbeans-php
homebrew/php/php-plantumlwriter                              caskroom/cask/eclipse-php
homebrew/php/php-version                                     homebrew/nginx/php-session-nginx-module
homebrew/php/php-build                                       homebrew/php/php-code-sniffer@2.9
homebrew/php/php-code-sniffer

After:

~> brew search homebrew/php/php
==> Searching taps on GitHub...
caskroom/cask/eclipse-php                                    homebrew/php/php-code-sniffer@2.9
caskroom/cask/netbeans-php                                   homebrew/php/php-cs-fixer
homebrew/nginx/php-session-nginx-module                      homebrew/php/php-install
homebrew/php/brew-php-switcher                               homebrew/php/php-plantumlwriter
homebrew/php/php-build                                       homebrew/php/php-version
homebrew/php/php-code-sniffer

DomT4 added some commits Oct 14, 2017

@jmsundar

This comment has been minimized.

Copy link
Contributor

jmsundar commented Oct 14, 2017

@DomT4
working cases:

brew link -n
brew unlink -n
brew linkage
brew linkapps
brew unlinkapps
brew list
brew man
brew update

some more cases that are affected:

brew deps #all options for this
brew doctor
brew prune [-n]
brew options (--all|--installed)   #listing specific formulae works as intended
brew info --json=v1 (--all|--installed) #the json should still be sorted lexicographically IMO
brew leaves
brew list --unbrewed
brew missing
brew tap
brew tap-info (--installed|taps)

Those that aren't listed here, I wasn't able to test.

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Oct 14, 2017

Those that aren't listed here, I wasn't able to test.

Thanks, I'll chase around your list a bit more.

There's some fun chasing these because, for example, the external_commands in commands ends with end.sort so if you only glance at the final output of that command it can look like everything is completely fine, whereas the two outputs above that in the same command are completely off the expected alphabetisation.

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Oct 14, 2017

I did check deps and wasn't able to reproduce a problem with that earlier. I'll take another look. What are you running specifically to get that to bork?

@jmsundar

This comment has been minimized.

Copy link
Contributor

jmsundar commented Oct 14, 2017

brew deps --installed and brew deps --tree --installed. They both aren't sorting lexicographically at the top level.

DomT4 added some commits Oct 14, 2017

@DomT4 DomT4 force-pushed the DomT4:handle_apfs_love_of_hash_order branch from 388ba1a to f1b183b Oct 14, 2017

DomT4 added some commits Oct 14, 2017

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Oct 14, 2017

Am I missing much else here beside the # TODO: This still returns a non-alphabetised list on APFS. notation & prune?

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Oct 14, 2017

One question I have is whether these commits result in the same order on HFS+ as we currently get without these commits.

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Oct 14, 2017

From memory I can't see any noticeable difference, but someone is very welcome to run it through a non-APFS machine and do a before/after diff. I don't currently have any local VMs set up 😢.

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Oct 17, 2017

I tested this on HFS+ on Sierra 10.12.6. It does appear to change the sort order for some commands, those that have a hyphen inside their names at the point where they differ from another command whose name is a prefix of the command name up to that point.

Before:

tap-info
tap-pin
tap-unpin
tap
...
update-report
update-reset
update

After:

tap
tap-info
tap-pin
tap-unpin
...
update
update-report
update-reset

I'd guess this is because the old way relying on file system sorting order was sorting on the full file names, like tap.rb and tap-info.rb, instead of just the command names, and '.' (U+2E) sorts after '-' (U+2D).

Aside from the fact that this is a change from existing behavior, I think the new way is better, because now the commands are sorted lexicographically by their actual names.

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Oct 17, 2017

The search behavior differs on my Sierra HFS+ system, too. Before the change, results are returned in a different order, and that order may vary between invocations.

Before:

$ brew search homebrew/php/php
==> Searching taps on GitHub...
homebrew/php/php-install                      homebrew/php/php-build                        caskroom/cask/eclipse-php
homebrew/php/php-plantumlwriter               homebrew/php/php-cs-fixer                     homebrew/nginx/php-session-nginx-module
homebrew/php/brew-php-switcher                homebrew/php/php-code-sniffer                 homebrew/php/php-code-sniffer@2.9
homebrew/php/php-version                      caskroom/cask/netbeans-php
[/usr/local/Homebrew on ⇄ master]
$ brew search homebrew/php/php
==> Searching taps on GitHub...
homebrew/php/brew-php-switcher                homebrew/php/php-build                        caskroom/cask/eclipse-php
homebrew/php/php-install                      homebrew/php/php-cs-fixer                     homebrew/nginx/php-session-nginx-module
homebrew/php/php-plantumlwriter               homebrew/php/php-code-sniffer                 homebrew/php/php-code-sniffer@2.9
homebrew/php/php-version                      caskroom/cask/netbeans-php
[/usr/local/Homebrew on ⇄ master]
$ brew search homebrew/php/php
==> Searching taps on GitHub...
homebrew/php/php-install                      homebrew/php/php-build                        caskroom/cask/eclipse-php
homebrew/php/php-plantumlwriter               homebrew/php/php-cs-fixer                     homebrew/nginx/php-session-nginx-module
homebrew/php/brew-php-switcher                homebrew/php/php-code-sniffer                 homebrew/php/php-code-sniffer@2.9
homebrew/php/php-version                      caskroom/cask/netbeans-php

After:

[/usr/local/Homebrew on ⇄ DomT4-handle_apfs_love_of_hash_order]
$ brew search homebrew/php/php
==> Searching taps on GitHub...
caskroom/cask/eclipse-php                     homebrew/php/php-build                        homebrew/php/php-install
caskroom/cask/netbeans-php                    homebrew/php/php-code-sniffer                 homebrew/php/php-plantumlwriter
homebrew/nginx/php-session-nginx-module       homebrew/php/php-code-sniffer@2.9             homebrew/php/php-version
homebrew/php/brew-php-switcher                homebrew/php/php-cs-fixer

Looks like the odd ordering for brew search may be due to the results from GitHub being returned in arbitrary order, instead of APFS affecting it?

Either way, I prefer the new behavior, and since it seems like the old behavior was "results returned in arbitrary order", this might not even count as a non-compatible behavior change. :)

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Oct 17, 2017

Looks like the odd ordering for brew search may be due to the results from GitHub being returned in arbitrary order, instead of APFS affecting it?

Yeah, I had a suspicion about this but I couldn't remember off the top of my head whether the behaviour was new or not.

Thanks for taking the time to look over this PR in such detail @apjanke, appreciated ❤️.

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

Great work again here @DomT4.

@MikeMcQuaid MikeMcQuaid merged commit 1d54180 into Homebrew:master Oct 18, 2017

2 of 3 checks passed

codecov/patch 50% of diff hit (target 66.63%)
Details
codecov/project 67.44% (+0.81%) compared to e1808bf
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Oct 18, 2017

Thanks everyone. Don't let me forget to look into the remaining TODO in tap-info with the --installed flag. That was giving me hassle the other night but I'm sure there's a solution somewhere 😅.

@DomT4 DomT4 deleted the DomT4:handle_apfs_love_of_hash_order branch Oct 18, 2017

@jmsundar

This comment has been minimized.

Copy link
Contributor

jmsundar commented Oct 18, 2017

Also, the list of unbrewed dylibs and headers output by brew doctor still is not sorted alphabetically. This is less of a problem, as it is sorted hierarchically, but as someone with hundreds of unbrewed headers and dylibs, it was much easier to make sure the list was only what I know about when dylibs with the same prefix were grouped together.

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Oct 18, 2017

That should be as simple to fix as inject_file_list(files.sort, message) in diagnostic.rb. If you wanted to double check that locally that'd be cool.

@DomT4 DomT4 referenced this pull request Oct 18, 2017

Merged

commands: filter out dotfiles from output #3334

4 of 4 tasks complete
@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Oct 20, 2017

@jmsundar Any chance you've had a moment to double check the above locally?

@jmsundar

This comment has been minimized.

Copy link
Contributor

jmsundar commented Oct 20, 2017

@jmsundar

This comment has been minimized.

Copy link
Contributor

jmsundar commented Oct 20, 2017

@DomT4 I just checked locally, and it does work.

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Oct 20, 2017

@jmsundar Thanks! Opened #3347.

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.