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

fix commits are based on HEAD of target branch, not PR #12

Open
scarf005 opened this issue Dec 28, 2023 · 1 comment
Open

fix commits are based on HEAD of target branch, not PR #12

scarf005 opened this issue Dec 28, 2023 · 1 comment

Comments

@scarf005
Copy link

Summary

- A (merge base) - B (remote HEAD)
   \
	C (PR HEAD) - C' (auto-fixed commit)

when autofix.ci creates commit C' from C, it seems to apply changes on top of B, not C.

Example

let's assume repo for above graph

  • only tracks file.txt
  • uses formatter that converts lines to CapitalCase, so foo -> Foo

on commit A:

1. Hello
2. World

on commit B:

- 1. Hello
2. World

on commit C:

1. Hello
2. World
+ 3. person

on commit C', the auto-fixed one also applies changes from commit B:

- 1. Hello
2. World
- 3. person
+ 3. Person

I find this a bit odd, because i was expecting the formatting to be localized to C, like:

1. Hello
2. World
- 3. person
+ 3. Person

Workflow Run With the issue

before formatting

image

In locations_commercial.json, line 343 is inserted.

During formatting

diff --git a/data/json/itemgroups/Locations_MapExtras/locations_commercial.json b/data/json/itemgroups/Locations_MapExtras/locations_commercial.json
index faa4415..17dcf92 100644
--- a/data/json/itemgroups/Locations_MapExtras/locations_commercial.json
+++ b/data/json/itemgroups/Locations_MapExtras/locations_commercial.json
@@ -337,7 +337,7 @@
       [ "bandolier_shotgun", 8 ],
       [ "torso_bandolier_shotgun", 3 ],
       [ "holster", 8 ],
-	  [ "shoulder_holster", 4 ],
+      [ "shoulder_holster", 4 ],
       [ "bootstrap", 2 ],
       [ "wristholster", 1 ],
       [ "bholster", 5 ],

In the log, we can find out that for locations_commercial.json, only line 343 is formatted.

After formatting

image

However, in the commit autofix.ci has made, line 14 to 16 is also removed.

image

corresponding commit URLs since the PR branch is now deleted

This is unexpected because the PR's merge base was older than its removal.

Additional Context

cataclysmbnteam/Cataclysm-BN#3981 (comment)

@scarf005 scarf005 changed the title fix commits is based on HEAD of target branch, not PR fix commits are based on HEAD of target branch, not PR Dec 28, 2023
@mhils
Copy link
Member

mhils commented Jan 18, 2024

Ughs, thanks for catching this! This definitely is a bug in the action right now. I guess we need to do something like this to fix it:

  1. Create commit with all changes.
  2. Check out PR head.
  3. Cherry-pick the commit from step 1.
  4. Generate autofix.json as we already do.

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

No branches or pull requests

2 participants