Brew: Make `revision` mean only one thing.

[DomT4]

We keep confusing people by using the magical word revision to mean different things.

We have:

• Bottle revisions:

bottle do
  revision 1
  sha256 "41af286dc0b172ed2f1ca934fd2278de4a1192302ffa07087cea2682e7d372e3" => :blah
end

• Git revisions:

url "https://github.com/magic/unicorns.git",
    :tag => "unicorn-1",
    :revision => "eefc6ed865ca20447afc293c986676933dbfb137"

• Formulae revisions:

url "https://example.org/example.tar.gz"
sha256 "13550350a8681c84c861aac2e5b440161c2b33a3e4f302ac680ca5b686de48de"
revision 1

Particularly confusing to new contributors is that we have an audit mechanism that nudges people to remove an unnecessary revision, and we see repeated examples of people removing the git commit revision rather than the revision 1.

We need to move away, long-term, from using the same keyword to represent different things, particularly where those things are used in close proximity on a regular basis. It'd probably be easier to change formulae/bottle meaning of revision, IMO, but discuss at will.

[apjanke]

:+1: I still get confused about this myself sometimes, and a terminology change - especially w a keyword change to remove context sensitivity in formula files - would probably help.

Might be a bit harder to do the batch change, but maybe keeping 'revision' for formula would work too, with ':revision' for git resources becoming ':commit', since "commit" is in wide informal use that way and associated with Git, and 'bottle_revision' for the bottles. We could prob do a bulk search-and-replace of :revision => "\w+" since nothing else uses that symbol AFAIK, and formatting is consistent. I can't easily think of another term to use for formula revisions, but maybe that's just because I'm used to it now.

Though semantically and conversationally, it might be cleaner to change the terms for formula and bottle: since we track our formulae in git, and don't bump the formula revision with every file change, a formula revision may live for several git revisions of the formula file. And all the git revisions of a formula file over its life do not correspond to "revisions" of that formula. (E.g. If I say "what revision of asciidoc are you looking at?" I might be referring to the git revision SHA of asciidoc.rb or the revision 2 inside the file. Can't tell without more context.)

Git is the only one of these three concepts that's from outside Homebrew and has pre-existing terminology, and it uses "revision". So maybe that should take precedence.

[UniqMartin]

In my mind there shouldn't be any confusion, as we're asking them to remove the revision line, not some :revision => argument from a statement. But I can see how that may trip up some people.

In my opinion, the least invasive and only acceptable solution would be to change the :revision => argument for url (I think :commit is an excellent suggestion). I'm :-1: on changing revision for both the formula itself and bottle do blocks. (We're already asking our contributors to not touch the bottle do block anyway and that should make it clear that we can't be referring to the revision line in there.)

[UniqMartin]

Maybe to add to my confusion why that is an issue: People are dealing with natural languages all the time, where the same thing means something different (depending on context) all the time. But I guess if we can change things in a way that reduces confusion and thus maintenance burden, it's reasonable to pick the pragmatic solution instead of trying to educate people …

[MikeMcQuaid]

I'd propose bottle revisions become bottle_version or bottle_revision, SCM revisions become commit or revision becomes subversion or something.

[chdiza]

revison => bottle_revision
SCM revision => commit
formula revision => edition, or just formula_revision

Not subversion, if one wants to decrease confusion. (Imagine a formula's HEAD is a subversion repo, and also has a formula revision that we call "subversion").

[MikeMcQuaid]

I like edition.

[ilovezfs]

Keeping with the bottle theme, we could call bottle revisions the bottling run or lot number.

If you like edition and want to continue the book theme, we could call each bottle revision an impression.

[ilovezfs]

I think we should leave this unchanged. The meanings are each quite clear from context.

[DomT4]

The meanings are each quite clear from context.

This rather assumes everyone has a firm grasp on the context of each usage 😉.

[ilovezfs]

Chicken and egg. I don't think adding additional jargon actually helps with that.

[MikeMcQuaid]

@ilovezfs This is something people run into problems with with alarming regularity, unfortunately.

[ilovezfs]

What percent of that is people modifying the bottle block when they shouldn't be as opposed to some other kind of confusion?

[UniqMartin]

I believe the bottle block confusion could also be dealt with by making brew bottle surround the bottle with comments that say something like “Please do not modify this following bottle do block.” or something similar. (This would also help with people occasionally removing or otherwise modifying the bottle do block when they shouldn't.)

[ilovezfs]

Yeah, that's why I asked. I'm wondering how much of the problem would be solved with that change alone.

[MikeMcQuaid]

I'd say when we ask people for a revision bump they modify the bottle block 25-50% of the time. Even when we say not to modify it in the original message it's still 10-20% of the time.

[chdiza]

The proposal is about reducing confusion and preventing removals of the revisions that appear _outside_ the bottle block. The prevention of incorrect bottle-block modifications is a separate issue. So while that separate issue would likely not be fixed by the present proposal, that's no argument against the present proposal.

But on that separate issue, let me ask what I hope is not too silly a question. Can't the CI bots be made to handle that stuff? When a user submits a PR that contains a bottle-block modification (an outright deletion, or a less severe change) can't they be configured to auto-github-comment on that PR saying "Nope, don't change the bottle block; put it back the way it was"? Maybe also "If you were told by a maintainer to bump the revision, they meant blah blah, not the one in the bottle-block."

[MikeMcQuaid]

revison => bottle_revision
SCM revision => commit
formula revision => edition, or just formula_revision

Going to open a PR using bottle_revision, commit and formula_revision as those names are the most obvious/explicit for the human-readable DSL.

[MikeMcQuaid]

Opened PRs for this. To add weight to @DomT4's argument: this is currently (part of) a valid formula:

class Foo < Formula
  url "https://github.com/foo/bar.git", :revision => "abcdef"
  revision 1

  bottle do
    revision 1
    sha256 "6f23f874cf3997bb0635e5f7706db92c9e98b5089df3b3098045afe071dc992b" => :el_capitan
  end
end

Instead let's have:

class Foo < Formula
  url "https://github.com/foo/bar.git", :commit => "abcdef"
  formula_revision 1

  bottle do
    bottle_revision 1
    sha256 "6f23f874cf3997bb0635e5f7706db92c9e98b5089df3b3098045afe071dc992b" => :el_capitan
  end
end

[UniqMartin]

I know I'm probably on lost ground here, but it still feels weird to completely ignore the context in which these are used and to try to make the DSL context-independent only for this particular case. If we get formula_revision and bottle_revision, then why is it still fine and unambiguous to have sha256 instead of formula_sha256 and bottle_sha256 or url instead of formula_url and patch_url (in a patch do block)? If nothing else, this different treatment looks ugly to me.

(Yes, I realize several PRs have been created to address this, but my argument makes more sense when looking at the complete picture, hence I'm posting it here.)

[ilovezfs]

@UniqMartin I agree with you but I think this is already decided.

[MikeMcQuaid]

@UniqMartin They are being changed because it causes confusion in the same way that sha256 etc. do not. I'm open to alternate suggestions or names to resolve this problem if they are presented in the relatively near term.

[UniqMartin]

They are being changed because it causes confusion in the same way that sha256 etc. do not.

I know. I'm not debating that this causes confusion for some contributors, I'm just a bit afraid that we're solving a minor problem with a very invasive change (and the potential for breakage such a change can cause), not to mention that this is a change to our formula DSL that affects almost any formula and we have been previously cautious with such changes.

I'm open to alternate suggestions or names to resolve this problem if they are presented in the relatively near term.

From https://github.com/Homebrew/brew/issues/38#issuecomment-223781002 above:

I believe the bottle block confusion could also be dealt with by making brew bottle surround the bottle with comments that say something like “Please do not modify this following bottle do block.” or something similar.

I'm not opposed to renaming :revision in SCM urls though my gut feeling is that this has caused far less confusion than the formula/bottle revision, it just happens to have the same name and thus was prematurely included in this issue. (But as I said, this is just my gut feeling, as I haven't been following formula contributions that closely to see whether this use of :revision is indeed as problematic as stated.)

[MikeMcQuaid]

then why is it still fine and unambiguous to have sha256 instead of formula_sha256 and bottle_sha256 or url instead of formula_url and patch_url (in a patch do block)? If nothing else, this different treatment looks ugly to me.

This does not seem to cause confusion for people because it's more obvious how they are scoped whereas a formula_revision is sometimes not present when a bottle_revision or commit is and it's relatively difficult to describe which one to change.

@UniqMartin To be more explicit and general about how to handle issues/PRs like this: a problem has been raised by this issue and a solution proposed. If you're opposed to the solution the onus is on you to propose an alternate solution rather than point out perceived flaws with the current solution. What do you propose we do to solve this issue?

[UniqMartin]

To be more explicit and general about how to handle issues/PRs like this: a problem has been raised by this issue and a solution proposed. If you're opposed to the solution the onus is on you to propose an alternate solution rather than point out perceived flaws with the current solution. What do you propose we do to solve this issue?

I don't know how to resolve this in general, but for this particular issue, I have proposed an alternative solution (again, cf. https://github.com/Homebrew/brew/issues/38#issuecomment-223781002) that is less invasive than changing our formula DSL en masse and I did so well before any solution was picked and implemented.

What else can I do than make such suggestions? Nobody has commented on this suggestion, not even to point out flaws inherent to it and that I might not have realized yet.

I'm not here to block progress or improvements to our contribution experience, but I'm a bit wary of sweeping changes like the one currently implemented and the least I can do is point out possible problems to help avoid nasty surprises. If my argument is not convincing and you deem the benefits worth the risks, then please simply let me know. Ultimately, I'll defer to your much greater experience with Homebrew in judging the situation.

[MikeMcQuaid]

I don't know how to resolve this in general, but for this particular issue, I have proposed an alternative solution (again, cf. #38 (comment)) that is less invasive than changing our formula DSL en masse and I did so well before any solution was picked and implemented.

Ok, my apologies for not commenting specifically on that suggestion.

I don't like it because I think it assumes that every tap will use our method of autogenerating/updating bottle bottles rather than updating them by hand (which some decide to do). I think those taps would find this comment more abrasive than a DSL change.

That said, given you've scoped the problem as being just about the bottle block, I'm prepared to make just a change there for now. Do you have any other suggested names for bottle_revision that would be more clearly about the bottle and not the formula? Alternatively, do you have a suggestion for what to change formula_revision to (technically revisions for bottles predate revisions for formulae if that's at all relevant).

[UniqMartin]

Ok, my apologies for not commenting specifically on that suggestion.

Thanks!

I don't like it because I think it assumes that every tap will use our method of autogenerating/updating bottle bottles rather than updating them by hand (which some decide to do). I think those taps would find this comment more abrasive than a DSL change.

That's good to know and makes the decision you've made when creating the above three PRs feel less arbitrary. Another similar idea that just popped into my head would be to move the bottle do block further down to reduce this kind of confusion. It could become the last element in class Foo < Formula […] end or could even be moved into a separate code block at the end of the file (by relying on the ability to reopen classes in Ruby):

class Foo < Formula
  # Formula code created by a human.
end

class Foo
  bottle do
    # Bottle code typically created by `brew test-bot`/`brew bottle`.
  end
end

One possible downside is that this repeats the class name. An upside is that this is far easier to cut out and replace with a new bottle do block compared to the current regex solution.

That said, given you've scoped the problem as being just about the bottle block, I'm prepared to make just a change there for now. Do you have any other suggested names for bottle_revision that would be more clearly about the bottle and not the formula? Alternatively, do you have a suggestion for what to change formula_revision to (technically revisions for bottles predate revisions for formulae if that's at all relevant).

In general, I find a solution that keeps the (formula) revision as-is (for the benefit of all the contributors that have already understood this) and only changes the DSL used inside the bottle do block more desirable. The latter could be more easily seen as an implementation detail and if it changes, it doesn't really affect contributors (except for causing less confusion).

I'm not sure if this might cause new confusion, but build feels like a nice replacement for a bottle revision. After all it is indeed a new (binary) build of an otherwise unchanged formula with the same pkgversion (version+revision). If we stray a bit into book printing territory, reissue could also work. What do you think about these? Let me know while I'll ponder this a bit more in the back of my head and try to come up with terminology that is better yet.

[MikeMcQuaid]

I'm not sure if this might cause new confusion, but build feels like a nice replacement for a bottle revision.

build could work for me. It'll overload the use of build.with? etc. in the DSL but I can't see a reason why you'd ever use that inside the bottle block.

[ilovezfs]

It'll overload the use of build.with? etc. in the DSL but I can't see a reason why you'd ever use that inside the bottle block.

I'd considered suggesting build and rejected it out of hand for exactly that reason.

Alternatives for inside the bottle block:

bottling
impression
lot
number
rebuild
run

[MikeMcQuaid]

rebuild

This probably slightly trumps build for me.

[ilovezfs]

trumps

Please don't use this word. It's probably some sort of violation of the CODEOFCONDUCT. :)

[MikeMcQuaid]

I'll use it while, legally, I still can 😉

[MikeMcQuaid]

The more I mentally chew on rebuild the more I like it; I think it concisely explains the concept well.

[ilovezfs]

Unless it confuses maintainers since that's also the name of a button in Jenkins :trollface:

[UniqMartin]

Right, I totally missed the obvious clash with build. But rebuild is not a bad alternative. :+1: