Allow lazily generated keyword regexps

[lionel-]

Closes #582

• Allow quoted forms in fontlock regexps variables. These should be used when regexp generation depends on user-configurable variables.

• Pick up bare keywords from ess-R-keywords so users can remove them.

• Use new multiple cursors feature to simplify fontlock tests

Using laziness in these variables was a great idea @vspinu.

[lionel-]

@vspinu why is the merge commit option disabled? I think that's the best option because it creates a grouping for a set of related commits.

[vspinu]

It created a lot of noise with merge commits. Rebase and merge creates an easy to follow linear history.

Ideally, there should be no "related" commits. I think one commit per feature is what we should strive for, given that we don't have a separate changelog.

[lionel-]

Ideally, there should be no "related" commits

I don't think so. Many commits are naturally related.

Rebase and merge creates an easy to follow linear history.

I always rebase before merging to keep a linear history. Can we agree to only create merge commits on a rebased branch?

[lionel-]

Many commits are naturally related.

And still make sense as separate commits for the purpose of history navigation.

[vspinu]

I always rebase before merging to keep a linear history.

This still creates a "non-linear" merge. Linear means one stream in my mind, no branching.

The restriction has been there for some time and IMO the magit log is much cleaner. Mostly because there are no explicit "Merge" commits and path intersections take separate lines. In old log one "merge" commit could take up to 4 lines! That's ugly and space inefficient.

And still make sense as separate commits for the purpose of history navigation.

You can still have those and related commits will show as consecutive in the history under your name. It's trivial to see related commits even without branching.

The only drawback that I see with the linear history is that pull numbers are not shown.

[vspinu]

Consider that most of the contributions consist of a single commit. So each time you merge you create noise with an extra merge commit. Re-basing is a convention on a quite some repos that I follow, it seems to be a common practice.

[lionel-]

Mostly because there are no explicit "Merge" commits and path intersections take separate lines. In old log one "merge" commit could take up to 4 lines! That's ugly and space inefficient.

To me linear history means no path intersection which you'll get by rebasing before merging. I think in the end we have a common understanding of what a readable history is (no path intersection), it's just that you're used to see non-rebased merged branches.

I won't fight for merge commits even though I think they are the best option but I find it a bit annoying that this workflow decision was taken without discussion...

[lionel-]

Consider that most of the contributions consist of a single commit.

That's where you use squash and merge so you get a single commit with a PR number.

[lionel-]

I think forbidding merge commits to avoid intersections mostly make sense because github doesn't provide an action for it and with many different contributors it's harder to get a rebasing discipline. Though it got a bit simpler when github gave permission to owners to push to branches in contributors' forked repos.

[vspinu]

That's where you use squash and merge so you get a single commit with a PR number.

Could be, but I always forget to do that and press default Merge button. Then there will always be 2-3 commit PRs which you don't want to squash.

nd with many different contributors it's harder to get a rebasing discipline.

Indeed. I don't think enforcing a rebase is an option.

In any case, I don't have a very strong opinion on this but logs in the past 2 months or so do look much nicer to me. What others think? @jabranham any specific opinion on this one?

[lionel-]

Relevant discussion: isaacs/github#1143

[lionel-]

Useful diagrams in isaacs/github#1017. They call it "semi-linear".

[jabranham]

I think the "safest" default is to create a merge commit because then you're guaranteed to keep all the commits. I'm willing to sacrifice a pretty git history for that. I really like the semi-linear history option, but it doesn't look like github supports that. Gitlab is free software (FOSS) and does support that workflow but I'm assuming people don't want to move. (Gitlab is actually really nice, you should check it out if you haven't already)

Of course the other options can make sense in a specific context that yall have already covered, like small one-commit PRs.

All that said, I don't have a super strong opinion and am just chiming in because you asked.

[jabranham]

Oh, and as an aside, if you haven't checked out recent magit development, they've added some features that make dealing with PRs super easy.

[vspinu]

I think the "safest" default is to create a merge commit because then you're guaranteed to keep all the commits.

I am not sure I understand what you are talking about. FF merge does keep all the commits. It doesn't keep the merge itself, but really, is it that important?

yall have already covered, like small one-commit PRs

This is an important case. After the dust has settled 99% of pulls should ideally implement one feature and each feature should be ideally implemented in one commit. Having one merge commit per feature is just ridiculous.

make dealing with PRs super easy.

Interesting. Could you be more specific? I don't see any PR specific functionality in the latest magit. Do you mean this https://github.com/sigma/magit-gh-pulls ?

[eddelbuettel]

I was also intrigued and saw eg this in the magit manual:

b Y (magit-branch-pull-request)
This command creates and configures a new branch from a Github pull-request, creating and configuring a new remote if necessary.

The name of the local branch is the same as the name of the remote branch that you are being asked to merge, unless the contributor could not be bother to properly name the branch before opening the pull-request. The most likely such case is when you are being asked to merge something like "fork/master" into "origin/master". In such cases the local branch will be named "pr-N", where N is the pull-request number.

These variables are always set by this command:

branch..pullRequest is set to the pull-request number.
branch..pullRequestRemote is set to the remote on which the pull-request branch is located.
branch..pushRemote is set to the same remote as branch..pullRequestRemote if that is possible, otherwise it is set to the upstream remote.
branch..description is set to the pull-request title.
branch..rebase is set to true because there should be no merge commits among the commits in a pull-request.
This command also configures the upstream and the push-remote of the local branch that it creates.

The branch against which the pull-request was opened, is always used as the upstream. This makes it easy to see what commits you are being asked to merge in the section titled something like "Unmerged into origin/master".

Like for other commands that create a branch it depends on the option magit-branch-prefer-remote-upstream whether the remote branch itself or the respective local branch is used as the upstream, so this section may also be titled e.g. "Unmerged into master".

When necessary and possible, then the remote pull-request branch is configured to be used as the push-target. This makes it easy to see what further changes the contributor has made since you last reviewed their changes in the section titled something like "Unpulled from origin/new-feature" or "Unpulled from fork/new-feature".

If the pull-request branch is located in the upstream repository, then you probably have set remote.pushDefault to that repository. However some users like to set that variable to their personal fork, even if they have push access to the upstream, so branch..pushRemote is set anyway.
If the pull-request branch is located inside a fork, then you are usually able to push to that branch, because Github by default allows the recipient of a pull-request to push to the remote pull-request branch even if it is located in a fork. The contributor has to explicitly disable this.
If you are not allowed to push to the pull-request branch on the fork, then a branch by the same name located in the upstream repository is configured as the push-target.
A — sadly rather common — special case is when the contributor didn’t bother to use a dedicated branch for the pull-request.
The most likely such case is when you are being asked to merge something like "fork/master" into "origin/master". The special push permission mentioned above is never granted for the branch that is the repository’s default branch, and that would almost certainly be the case in this scenario.

To enable you to easily push somewhere anyway, the local branch is named "pr-N" (where N is the pull-request number) and the upstream repository is used as the push-remote.

Finally, if you are allowed to push to the pull-request branch and the contributor had the foresight to use a dedicated branch, then the fork is configured as the push-remote.
The push-remote is configured using branch..pushRemote, even if the used value is identical to that of remote.pushDefault, just in case you change the value of the latter later on. Additionally the variable branch..pullRequestRemote is set to the remote on which the pull-request branch is located.

When you later delete the local pull-request branch, then you are offered to also delete the corresponding remote, provided it is not the upstream remote and that the tracking branch that corresponds to the deleted branch is the only remaining tracked branch. If you don’t confirm, then only the tracking branch itself is deleted in addition to the local branch.

Do not delete the tracking branch instead of the local branch. The cleanup mentioned in the previous paragraph is not performed if you do that.

b y (magit-checkout-pull-request)
This command creates and configures a new branch from a pull request, the same way magit-branch-pull-request does. Additionally it checks out the new branch.

[vspinu]

This is emacs24? Should be marked as such for an easy reference later on.

[vspinu]

This won't work if keywords if keywords is a regexp or a function or a cell with car a function. See the Info 23.6.2 Search-based Fontification.

I also wonder if the built-in font-lock functionality could be used here:

‘(eval . FORM)’
     Here FORM is an expression to be evaluated the first time this
     value of ‘font-lock-keywords’ is used in a buffer.  Its value
     should have one of the forms described in this table.

[lionel-]

Of course, good catch. The eval form seems relevant here, thanks.

[jabranham]

Yes, what @eddelbuettel mentioned is what I was referencing. magit-checkout-pull-request is probably the most useful. It somehow magically sets up the branches as needed and seems to do it right.

It's newish though. I think it first appeared in magit 2.12.0, released March 29th.

[jabranham]

I am not sure I understand what you are talking about. FF merge does keep all the commits. It doesn't keep the merge itself, but really, is it that important?

It can be helpful to see that for example these four commits were worked on in a separate branch then merged in all at once. Usually not necessary, but nice for more complicated stuff.

This is an important case. After the dust has settled 99% of pulls should ideally implement one feature and each feature should be ideally implemented in one commit. Having one merge commit per feature is just ridiculous.

Yes, I agree that merge commits aren't needed for single commits, though honestly it doesn't bother me having them in there either.

[vspinu]

magit-checkout-pull-request is probably the most useful.

Just checked it up. Awesome! Will the github close the issue if you merge the PR from the magit interface? That might be a good reason to keep the simple merge the default. Surely it won't on a rebase.

Also, any good way to jump from magit PR diff to github PR in order to comment in the web interface?

[jabranham]

Yes, I believe it will close the PR. Not sure about the rebase option. I think magit-browse-pull-request will do that.

…

On Sun, Jul 29, 2018, 6:11 PM Vitalie Spinu ***@***.***> wrote: magit-checkout-pull-request is probably the most useful. Just checked it up. Awesome! Will the github close the issue if you merge the PR from the magit interface? That might be a good reason to keep the simple merge the default. Surely it won't on a rebase. Also, any good way to jump from magit PR diff to github PR in order to comment in the web interface? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#621 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALXMibr5bU9Ib7Rbt2HDQ8Ip8CdZhlMAks5uLkE1gaJpZM4VlZe_> .

[lionel-]

After the dust has settled 99% of pulls should ideally implement one feature and each feature should be ideally implemented in one commit.

I think this makes sense for new features but as soon as some refactoring is going on, atomic, self-explanatory commits are much better for history navigation. The dust never completely settles on a large project.

But yes, single-commit PRs should be squashed and ff-merged.

[lionel-]

@vspinu To demonstrate your point instead of mine, I just reduced the branch to 1 commit ;)

The eval form idea works great. Thanks for the review.

[lionel-]

Possible summary of the discussion:

• We want a linear history where paths don't intersect

• PRs are best kept small. This means ideally they only contain one commit.

• Sometimes it makes sense to have several commits within a context, and a semi-linear branch merge is best then. It requires rebasing the branch before merging until github gives us a button for this.

• It is useful to keep github PR refs, either in a squashed + ff-merge single commit or in a merge commit if there are several commits.

I think this means we should disallow the rebase and ff-merge feature. Or at least recognise it isn't ideal but still use it for convenience when in a hurry?

[lionel-]

It is useful to keep github PR refs

About github refs and discussions, I was thinking it might be useful to set up a read-only mailing list watching the repo. This way we keep an independent history of our github discussions and epic arguments. The github refs would be mentioned in the mail titles and so we'd have a natural linkage between refs in commits and refs in the mailing list history.

This mailing list would be distinct from the ess-core mailing list so it could be public and we'd only be registered if we want to. And read-only so no-one could reply to github threads.

cc @mmaechler

[vspinu]

It is useful to keep github PR refs, either in a squashed + ff-merge single commit or in a merge commit if there are several commits.

It looks like I was wrong regarding this workflow as "obvious" so I am activating back the merge option. Once semi-linear option is there we will switch to it.

PRs are best kept small. This means ideally they only contain one commit.

Even more importantly to keep one feature per commit if possible.

I was thinking it might be useful to set up a read-only mailing list watching the repo.

I don't understand the purpose of this. Aren't we already subscribed to github issues and can reference any earlier discussion?

I think magit-browse-pull-request will do that.

Thanks. It does work for open PRs. For historical PRs links in merge commits seem to be the best option. I am not aware of any tool which would infer the PR from the git commit. The best navigator I could find so far is bar-browse from browse-at-remote which does the right thing from all contexts except magit-blame. I am also experimenting with magithub but it has some issues at the moment (especially with ESS due to large number of issues).

[lionel-]

I don't understand the purpose of this. Aren't we already subscribed to github issues and can reference any earlier discussion?

The purpose is to make the historical information about ESS development independent of Github services.

[vspinu]

I don't think that's that email tracker is a good solution given that github allows edits. If migration to gitlab is the intent here then I am pretty sure there will (already are?) tools to migrate the entire issue tracker to a different forgery.