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

Just update the original commit rather then having a second commit #16

Labels
enhancement New feature or request hacktoberfest

Comments

@ChrisWiles
Copy link

If the action runs and changes the file I would rather it just update the original commit and force push rather then having a second commit.

Is something like this possible?

Thanks

@ChrisWiles ChrisWiles added the enhancement New feature or request label Jun 9, 2020
@creyD
Copy link
Owner

creyD commented Jul 25, 2020

Sorry for taking so long to get back to you: Yes that would be possible but I don't think it would be a good idea. Git -as a versioning system- can technically revert a commit, adjust the code and commit again. As for a feature, I don't think that would be a good idea, as many people use multiple actions in one repository and this could lead to strange behavior on all parts.

@jorenbroekema
Copy link
Contributor

jorenbroekema commented Aug 6, 2020

Can you name an example where this could lead to strange behavior?

If I am running tasks myself locally instead of automated in the CI, amending commits is usually part of it, let's take an example:

  • Change code --> multiple commits
  • Get it reviewed and approved ✅
  • Time to make it ready for release
  • Squash my commit into one, because it's only one feature
  • Rebase to latest master --> amends the commit
  • Format the code with prettier --> amends the commit
  • Create a release changelog entry --> amends the commit

In this typical workflow that I use, nothing goes wrong for the many times that I amend the commit, even the author stays the same of the initial commit was made by someone else. Now I do it in a CI, and nothing changes other than the fact that I am doing it automatically instead of manually (and you probably wouldn't do the rebase/squashing in a CI but it's just to show as example that amending/changing commit hash is not problematic in isolation).

I for one would also like a feature to use the original commit and have it amended, because I like to keep my commit history clean where possible. I think it's fair to discuss whether it should be the default though. In my opinion, yes, formatting is just formatting, there's no use in checking a formatting commit on its own, it's never the commit you want if you wanna check on something, it's essentially "useless".

@creyD
Copy link
Owner

creyD commented Aug 6, 2020

When using multiple actions the commits made by the actions themselves often don't get checked again, as they should be fine, as the original commit was already checked. If an action doesn't finish that check and the commit is adjusted in the meantime, it could lead to a state where this action thinks it checked a commit when in reality it didn't check the current state (after prettier ran). I suppose we could test this, but sadly I don't have the time right now.

@jorenbroekema
Copy link
Contributor

I would need a real scenario here to understand what the problem would be, I don't see how a changed commit hash would mess up actions or workflows with multiple actions. To me it still sounds like a very very specific scenario where this could happen, and for that a user configuration option to make it a separate commit is definitely nice.

Still, a commit that just does formatting is essentially commit history bloat, especially if you merge to master often, half your commit history could be just formatting commits. The reason why commit history bloat is annoying is because people use commit history to backtrack changes and see where they need to revert it. A few formatting commits are not an issue by itself but that stuff really stacks up.

@creyD
Copy link
Owner

creyD commented Aug 10, 2020

I see your point. We can implement this as an option and warn users, that it might interfere with other actions in the same repository. How does this sound to you?

@jorenbroekema
Copy link
Contributor

Seems alright to me yep :) I'm cool with sending a PR for it too if you like, just gotta know which name to give the option/flag, perhaps --amend-commit or --same-commit

@creyD
Copy link
Owner

creyD commented Aug 11, 2020

Yeah sure, name it whatever seems right to you! Maybe the option inplace or replace could help? Please reference this issue in your PR!

@creyD
Copy link
Owner

creyD commented Oct 16, 2020

@jorenbroekema Any updates on this? I am preparing the next version in dev right now and would love to include this change!

@jorenbroekema
Copy link
Contributor

Ahh I must have forgotten, I have a few hours to kill today so I'll let you know at the end if I managed ;)

@creyD
Copy link
Owner

creyD commented Oct 17, 2020

Causes some problems as you can see: https://github.com/creyD/prettier_test/commits/main

@creyD
Copy link
Owner

creyD commented Oct 17, 2020

If you want to help debug, just create a similar test repository, I am running out of ideas... At first it stated that it is behind the remote, when I added git pull nothing happened, if I didn't add new files it still didn't work and in the end it destroyed the history of this test repository: I forced the push and then it creates a merge because of the conflicts and then on this the action automatically creates a new commit. In the end -as you might have seen- it redid all the 36 commits to this repo into one single commit.

@jorenbroekema
Copy link
Contributor

Amended commits have to be force pushed, pulling first will indeed create a merge commit which defeats the purpose of the same_commit option. I didn't think of that when I made the PR. I'll send another PR with the fix when I get home in an hour or so ;)

@jorenbroekema
Copy link
Contributor

#26 there, I tested it also ;)

@creyD
Copy link
Owner

creyD commented Oct 17, 2020

Sorry, but it still destroys the entire history and leaves only one commit.

@jorenbroekema
Copy link
Contributor

Did you set the fetch depth for the checkout action to fetch the entire history instead of the most recent?

@creyD
Copy link
Owner

creyD commented Oct 18, 2020

I tried this, you may take a look at this test repo: https://github.com/creyD/prettier_test . Can you help me debug why this doesn't work? It changes the author and echos the right stuff, but doesn't apply prettier to the files.

@jorenbroekema
Copy link
Contributor

#27 resolved here :)

@creyD
Copy link
Owner

creyD commented Oct 18, 2020

Looks good, thanks for all your help!

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