Skip to content
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

Few changes to the instructions how to compile a new kernel #1

Closed
wants to merge 9 commits into from

Conversation

halcon74
Copy link
Contributor

@halcon74 halcon74 commented Sep 20, 2020

Hi Ramon,

I am learning to make PRs... :)

I tried to make this PR especially for cherry-picking and, I need to say, it's not that easy.

I think in this case the best approach can be:

  1. To write the complete new version of the file separately
  2. Copy/Paste the lines from up to bottom, trying to split changes to the proper blocks, committing each block

And even then there will arise conflicts. For example, I rename a section and move this section up. OK, I split it to two steps... But if you like to merge the second step, not first, there will be a conflict :)

EDIT: IMHO, it contradicts the essence of a VCS; it's better to make multiple PRs.

@halcon74
Copy link
Contributor Author

How did you submit the reviews?

@halcon74
Copy link
Contributor Author

Experimenting: I used the same blue button on your commit, adding this README, and it resulted in a similar review.

@halcon74
Copy link
Contributor Author

halcon74 commented Sep 20, 2020

A note: the review that I submitted for the lines about Grub, is shown not only in that commit where I submitted the review, but in the last one too (because one of the lines about Grub is shown there too) :) GitHub whistles :)

@keks24 keks24 self-assigned this Sep 21, 2020
@keks24
Copy link
Owner

keks24 commented Sep 21, 2020

And even then there will arise conflicts.

These might happen. I am going to fix them, since I am the owner of the repository.

EDIT: IMHO, it contradicts the essence of a VCS; it's better to make multiple PRs.

I do not think so. Every commit is documented and I can choose to take all changes, pick some of them or reject everything.

Other pull requests would make sense, if it refers to another issue or topic.

Apropos issues: It is always good to report an issue and combine it with a pull request. Good examples I made so far:

Edit - 20200921:
Oh no. By adding the links of the issues here, GitHub automatically referenced them. Shit happens. :D

By the way: Since this is a mirror repository from my internal Gitea instance, I am going to do some experiments here and then adapt the changes locally, so your pull request might disappear here.

@keks24
Copy link
Owner

keks24 commented Sep 21, 2020

Concerning "how to checkout a pull request": On the upper right corner, there is a button called Open with. You can then check out the pull request via gh pr checkout <pull_request_number>. I totally forgot, that GitHub recently implemented a feature to manage GitHub repositories via CLI.

@halcon74
Copy link
Contributor Author

halcon74 commented Sep 21, 2020

Apropos issues: It is always good to report an issue and combine it with a pull request. Good examples I made so far:

Thanks. I'll keep it in mind.

Edit - 20200921:
Oh no. By adding the links of the issues here, GitHub automatically referenced them. Shit happens. :D

I'll keep it in mind too :D

By the way: Since this is a mirror repository from my internal Gitea instance, I am going to do some experiments here and then adapt the changes locally, so your pull request might disappear here.

OK.

@halcon74
Copy link
Contributor Author

Concerning "how to checkout a pull request": On the upper right corner, there is a button called Open with. You can then check out the pull request via gh pr checkout <pull_request_number>. I totally forgot, that GitHub recently implemented a feature to manage GitHub repositories via CLI.

Good to know, about 'gh'!

@keks24
Copy link
Owner

keks24 commented Sep 21, 2020

Here is what I did with your changes:

0. Simulating, that I work via GitHub:

# this generates a random directory and automatically navigates to it "cd $(mktemp -d)"; see 1.
$ cdt
$ git clone "[email protected]:keks24/dotfiles.git"
$ cd "dotfiles/"
  1. Since I am working with my local Gitea instance, I add a separate origin github:
$ git checkout master
$ git remote add github "[email protected]:keks24/dotfiles.git"
$ git remote -v
github  [email protected]:keks24/dotfiles.git (fetch)
github  [email protected]:keks24/dotfiles.git (push)
origin  [email protected]:keks24/dotfiles.git (fetch)
origin  [email protected]:keks24/dotfiles.git (push)
  1. Check out this pull request and automatically create a new branch, called enhancements_compiling_kernel2:
$ git fetch github pull/1/head:enhancements_compiling_kernel

This avoids to apply the changes to the current branch I am working on, which is currently master.

Apropos branch names: GitHub is about to change the branch name master to main. This anti-racism thing is going around like fire. At many points this is so ridiculous and it will only cause more issues...

  1. Check out the new branch:
$ git branch
enhancements_compiling_kernel
master
$ git checkout enhancements_compiling_kernel
  1. Take a look at the log:
$ git log
# the best cli tool, i found so far, when working with git repositories; see 3 and 4.
$ tig

I see here, that you state what you have changed. I see this in the diff, so this is actually redundant. It would be better to tell why you have changed it. For example:

Rename

Rename Section 1.2 to 1.1

to

correct typo

Associating the commit message correct typo with your change makes the explanation complete. A commit message should always answer the question why and not what.

  1. Edit and pick5 some of the commits and apply them:
$ git log --pretty="oneline"
$ git checkout master
# cherry picking from the branch "enhancements_compiling_kernel"
$ git cherry-pick dbdf88b8d3fd68319f9b6afd77b880f67d26187b
[...]
# when there are some conflicts, edit the file with an editor and then
$ git add usr/src/README
$ git cherry-pick --continue

Ah, I see. Cherry picking is also merging at the same time, where you also might have to solve merge conflicts. Now I understand your issues with my pull request. This is actually for advanced users. And this7 is how to abort a git cherry-pick.

I cherry picked some of your commits and applied my changes to them.

I also stumbled across this page6, which nicely adapts the output of git log.

  1. Push all changes:
$ git push
  1. Remove the origin github:
$ git remote remove github
$ git remote -v
origin  [email protected]:keks24/dotfiles.git (fetch)
origin  [email protected]:keks24/dotfiles.git (push)

1 https://github.com/grml/grml-etc-core/blob/v0.17.4/etc/zsh/zshrc#L3419
2 https://stackoverflow.com/a/30584951
3 https://github.com/jonas/tig
4 20200921-121934_screenshot
5 https://mattstauffer.com/blog/how-to-merge-only-specific-commits-from-a-pull-request/
6 https://ma.ttias.be/pretty-git-log-in-one-line/
7 https://stackoverflow.com/a/16826728

@halcon74
Copy link
Contributor Author

halcon74 commented Sep 21, 2020

Thanks for sharing your experience! I'll study it.

Mercurial (which I'm used to) branches are designed in somewhat another way. There are multiple heads, which can diverge-converge. (One commit can't be included in different branches)

@halcon74
Copy link
Contributor Author

Hi Ramon, I noticed also that there is not mentioned cd linux before make (I see no reason to create another PR for it)

@keks24
Copy link
Owner

keks24 commented Sep 23, 2020

Hi Ramon, I noticed also that there is not mentioned cd linux before make (I see no reason to create another PR for it)

Oh yeah, I forgot to add that!

You can still work with your fork and adapt it. The commit message will be shown here.

@keks24
Copy link
Owner

keks24 commented Sep 23, 2020

Done; see #1 (comment).

Thanks for the effort!

@keks24 keks24 closed this Sep 23, 2020
@halcon74
Copy link
Contributor Author

halcon74 commented Sep 23, 2020

Thanks for the information about cherry-pick (abort, continue) and other details.

About editing the file with an editor: my editor for git wasn't configured properly and wasn't opening (now it is).

You can still work with your fork and adapt it. The commit message will be shown here.

Shown where? I'm going to check it...

@keks24
Copy link
Owner

keks24 commented Sep 23, 2020

About editing the file with an editor: my editor for git wasn't configured properly and wasn't opening (now it is).

I meant this:

halcon74/shell-scripts#1 (comment)

You can edit that with any editor to solve merge conflicts. :)

Shown where? I'm going to check it...

I meant, if you do further editing on your fork and push it to your fork, all commit messages are shown in this pull request like this:

20200924-001456_screenshot

@halcon74
Copy link
Contributor Author

halcon74 commented Sep 23, 2020

I added a new commit and pushed it to origin, but I don't see it here...

halcon74@23010b9

Maybe, it's because the PR is closed?

@keks24
Copy link
Owner

keks24 commented Sep 24, 2020

Yes, that could be it. You could test it with your other pull request about g-cpan.

@halcon74
Copy link
Contributor Author

Possible. If/when I have something to add. Does it work only for master branch?

@keks24
Copy link
Owner

keks24 commented Sep 24, 2020

I never tested it, but I think, it depends on which branch of yours you want to merge. In your case, it is master.

20200924-114004_screenshot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants