rust-lang / rust-mode

Emacs configuration for Rust
Apache License 2.0
1.1k stars 176 forks source link

Differences between rustic.el and rust-mode.el #407

Closed tarsius closed 2 years ago

tarsius commented 3 years ago

These commits are not intended to be merged.

This demonstrates what differences remain between rustic.el and rust-mode.el after merging

(All the commits from #406 are included here as well.)

See https://github.com/rust-lang/rust-mode/pull/405 for an overview.

This pull-request was created by:

  1. Applying the above pull-requests.
  2. Evaluating:

    (progn
     (copy-file "~/.emacs.d/lib/rustic/rustic.el"
                "~/.emacs.d/lib/rust-mode/rust-mode.el"
                t)
     (with-current-buffer
         (find-file-noselect "~/.emacs.d/lib/rust-mode/rust-mode.el")
       (save-excursion
         (goto-char (point-min))
         (save-excursion (replace-regexp ":group 'rustic" ":group 'rust-mode"))
         (save-excursion (replace-regexp "rustic-mode" "rust-mode"))
         (save-excursion (replace-regexp "rustic" "rust"))
         (save-excursion (replace-regexp "\"1.2\"" "\"0.6.0\"")))))
  3. Manually spitting the differences across multiple commits.
rust-highfive commented 3 years ago

r? @brotzeit

(rust-highfive has picked a reviewer for you, use r? to override)

brotzeit commented 3 years ago

@tarsius can you please open another pull request that only contains the commits that moves code to other files, this way I still got the diffs(thanks for that).

brotzeit commented 3 years ago

Ah you did that already, thanks :P

tarsius commented 3 years ago

@tarsius can you please open another pull request that only contains the commits that moves code to other files, this way I still got the diffs(thanks for that).

Of course.

By the way, I meant to ask you how you perform the merges and offer some unsolicited advice on how to improve upon that. [I am guessing its one of the "merge" options offered by the Github web interface.] Since there were no new commits on master you could just have fast-forwarded master to the tip of the feature branch. Alternatively you could have created a merge commit. For some reason you picked a third option: create no merge commit (which could be used as a mechanism to signal who performed the integration into master) but for some reason you also did not just fast-forward.

Instead you recommitted each commit (changing the committer name and date). I would like to understand why you do that because I cannot see any benefits (except for the one that you could instead get by using a merge commit).

There is however the substantial drawback of the feature not actually being merged after such a "merge". One result of that is that you had to make this request, if you had just fast-forwarded, then the respective commits would automatically not have been considered part of this pull-request any more. Likewise I have to transplant some wip feature branches on the new "equal but not eq" history.

Also while Github displays such pull-requests as "merged" even though they haven't actually been merged, other tools don't know that the pull-request has been "merged" using Github.

I usually merge using proper "fast-forward" merges, i.e. no merge commit. I also use my Forge package, which records all tips of all pull-requests. The result is that despite using a linear history and in no way manipulating the contributed commits, the tips of merged pull-requests are still being recorded in the refs/pullreqs/N namespace and those refs are displayed in logs. With your merging style they are lost in time.

Um. That longer to describe than I had expected. I hope it was at least somewhat coherent.

tarsius commented 3 years ago

Some images are probably easier to understand.

Lost in time:

20210423-14:13:27

Extreme counter-example:

20210423-14:14:51

A pull-request can be visited by typing C-c C-v on its tip commit and now additional information has to be manually recorded to allow that.

brotzeit commented 3 years ago

Instead you recommitted each commit (changing the committer name and date). I would like to understand why you do that because I cannot see any benefits (except for the one that you could instead get by using a merge commit).

I was told by several people in the past that performing a "rebase and merge"(in github) is the way to go since you don't have the merge commit and get a cleaner history ?

tarsius commented 3 years ago

I was told by several people in the past that performing a "rebase and merge"(in github) is the way to go since you don't have the merge commit and get a cleaner history ?

I've that option did the sensible thing ("if possible, then just fast-forward"), then I would fully agree. But because that isn't the case I perform all my merges using Magit/Forge. (I check out the pull-request and then us m i ("merge into"), which also performs some useful cleanup.)

Or in other words "rebase and merge": sure. "In github"? No, never perform any git operations using github, they usually break things in subtle ways.

tarsius commented 3 years ago

But let's not focus on that right now (yes, I know I brought it up...). I should probably write a more coherent text about it at some time, including a howto on what I think people should be doing instead.

I have hidden my off-topic posts, but cannot do the same for your reply.

tarsius commented 3 years ago

What do you want to do about the commands that rustic no longer defines in rustic.el:

Should we do the same for rust-mode and move them to a separate library as you did with rustic-interactive.el. (But i would name it rust-commands.el; *-interactive.el is unusual.) Or should we instead move these commands back into rust-mode.el?

I looked at the differences and they are not big, so after a bit of backward and/or forward porting rustic can use the commands from rust-mode. I guess that means moving the commands back into rust-mode.el would be preferable. In any case I would like to first decide which package we rearrange to make it more like the other.

I favor moving these shared commands back into rustic.el.

brotzeit commented 2 years ago

This pull request helped a lot, thanks again.

tarsius commented 2 years ago

Great :)