Improve Merge and Commit logic #75

Stummi · 2022-06-01T09:59:42Z

Type of Change

Bugfix
Enhancement / new feature
Refactoring
Documentation

Description

Create reproducible commit hashes by using a fixed date/time for the merge commit
Remove optimistic option (It is never actually used, and adds unnecessary complexity)
Preserve parent refs for dev merge commit
Replace hasDiff logic with a comparison on commit hashes

Checklist

Write tests
Make sure all tests pass
Update documentation
Review the Contributing Guideline and sign CLA
Reference relevant issue(s) and close them after merging

github-actions · 2022-06-01T10:00:02Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

Stummi · 2022-06-01T10:05:20Z

I have read the CLA Document and I hereby sign the CLA

staffbase-robert · 2022-06-01T17:52:52Z

src/autodev.ts

+      await exec(`git merge origin/${pull.branch}`, undefined, {
+        env: {
+          GIT_COMMITTER_DATE: commitDate,
+          GIT_AUTHOR_DATE: commitDate
+        }
+      })


The actual commit is done a few lines below. (see git commit -m', [message])

ah, yes, you are right. Is there a particular reason why it is done this way? Why not just keep a number of merge commits?

A single commit with X merged branches is much easier to read and understand compared to X merge commits + commits that have been made in the branches (we're not using squash merge). For some large repos you have to read trough 2 pages of commits to understand what has been merged into dev.

If it's the goal to create the same commit hash, when the same branches have been merged, then it should be enough to just change the commit + author dates of the last commit. source

Stummi · 2022-06-02T09:44:37Z

src/autodev.ts

+    `git show -s --format='%ci' origin/${base}`
+  )
+
+  await exec(`git checkout ${base}`)


Creation of new branch was moved to the end of logic. Its not yet needed, just start on the base branch

Stummi · 2022-06-02T09:45:03Z

src/autodev.ts

-      info('🎉 No Pull Requests found. Nothing to merge.')
-    }
-    return
+    info('🎉 No Pull Requests found. Nothing to merge.')


There is no need anymore to duplicate the check for a diff here

But I guess the return is useful because the following steps doesn't make sense with no changes, or? 🤔

No, I would still go to the push, so when all pulls get the "dev" label removed, the autodev action will revert the dev branch to main

Stummi · 2022-06-02T09:45:39Z

src/autodev.ts

-  const message = optimistic
-    ? await merge(base, pulls, updateComment, updateLabel)
-    : await mergeAll(pulls, updateComment, updateLabel)
+  await exec(`git checkout -B ${branch}`)


Create the new dev branch on the current commit (which is either $base if no dev pulls exists, or the newly created merge commit)

Stummi · 2022-06-02T09:47:28Z

src/autodev.ts


  // only push to defined branch if there are changes
+  await exec('git fetch')


fetch again. This reduces (though not completely eliminates) a race condition where another parallel invocation of the autodev action could result in having a inconsistent origin/dev ref

Stummi · 2022-06-02T09:48:15Z

src/autodev.ts

-    await exec('git push -f')
+    // ignore any errors
+    await exec('git push -f', undefined, {
+      ignoreReturnCode: true


Command will still fail if run into the (earlier mentioned) race condition here. But in this case we can just ignore the failure (basically it means another invocation of the dev workflow beat us in pushing the same state)

Stummi · 2022-06-02T09:48:50Z

src/autodev.ts

-  })
-
-  return ret
+  return (


Since we produce reproducible commit hashes now, just compare them to see if the dev state changed

staffbase-robert

The action fails now with this message

 /usr/bin/git fetch
/usr/bin/git config --global user.email staffbot@staffbase.com
/usr/bin/git config --global user.name autodev-action
/usr/bin/git show -s --format='%ci' origin/main
'2022-06-03 13:55:05 +0200'
/usr/bin/git checkout main
Switched to a new branch 'main'
branch 'main' set up to track 'origin/main'.
/usr/bin/git merge origin/feature
Updating 94e3cc6..767c0a4
Fast-forward
 README.md | 2 ++
 1 file changed, 2 insertions(+)
/usr/bin/git reset origin/main
Unstaged changes after reset:
M	README.md
/usr/bin/git add -A
/usr/bin/git commit -m AutoDev Merge

The following branches have been merged:
- feature
Author identity unknown

*** Please tell me who you are.

Run

  git config --global user.email "you@example.com"
  git config --global user.name "Your Name"

to set your account's default identity.
Omit --global to set the identity only in this repository.

fatal: empty ident name (for <runner@fv-az39-531.yumyzuo2tp0ejekjjwodrrku4b.bx.internal.cloudapp.net>) not allowed

flaxel

Great improvement! 🎉

flaxel · 2022-06-03T12:46:18Z

src/autodev.ts

-      info('🎉 No Pull Requests found. Nothing to merge.')
-    }
-    return
+    info('🎉 No Pull Requests found. Nothing to merge.')


But I guess the return is useful because the following steps doesn't make sense with no changes, or? 🤔

flaxel · 2022-06-03T13:17:54Z

The action fails now with this message

You can use this example PR to reproduce the issue.

As a first check can you can add the command to check the configuration:

git config --global --list

Otherwise I think we could add the information as environment variables. Or we could use the option --reset-author.

Stummi · 2022-06-13T11:30:48Z

I am not sure what the exact reason is for the original issue, however it seems to work when I set the git config locally.

src/autodev.ts

flaxel · 2022-06-13T13:17:25Z

Looks really good, but my local changes differ from the dist folder. Could you run the command again?

npm run all

And I think we missed something else.

fatal: The current branch dev has no upstream branch.
To push the current branch and set the remote as upstream, use
    git push --set-upstream origin dev

Stummi · 2022-06-15T06:36:59Z

I resolved the issue now, and did some real tests with an own repository:

Merge commit does work as expected
Reproducible Commit hashes do work now

flaxel · 2022-06-20T05:29:49Z

src/autodev.ts

+    overrideDate
+  )
+  const rev = await execAndSlurp('git rev-parse HEAD')
+  await exec(`git checkout replace/${rev}`)


I guess this is normal and we can't prevent it?

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

git switch -c

Or undo this operation with:

git switch -

Turn off this advice by setting config variable advice.detachedHead to false

And I don't know what I do differently, but when I run npm run all, I have differences again?!

I guess this is normal and we can't prevent it?

You mean the message? I don't think so

And I don't know what I do differently, but when I run npm run all, I have differences again?!

I guess the result of npm run all is not exactly reproducible,

I did a rebase now, removed yarn.lock, ran yarn again and then reran npm run all. Hope this result is now reproducible on your end

It is never used, and just add unnecessary complexity

staffbase-robert

It works for me now.
The action now adds all commits of all merged pull requests to the commit history on the dev-branch. It doesn't hurt, but is this really necessary? I rarely check this commit history and only check out the "squash" commit at the end to understand which branches have been combined.

A "squash" option would be cool :)

Stummi requested review from staffbase-robert, 0x46616c6b and flaxel as code owners June 1, 2022 09:59

github-actions bot added a commit that referenced this pull request Jun 1, 2022

@Stummi has signed the CLA from Pull Request #75

896c620

Stummi force-pushed the various-improvements branch 3 times, most recently from f5dd3a2 to ef0d9ed Compare June 1, 2022 10:11

staffbase-robert reviewed Jun 1, 2022

View reviewed changes

Stummi force-pushed the various-improvements branch 7 times, most recently from 1a96a05 to 3fed51e Compare June 2, 2022 09:32

Stummi changed the title ~~Use fixed commit date to produce reproducible dev states~~ Improve Merge and Commit logic Jun 2, 2022

Stummi commented Jun 2, 2022

View reviewed changes

Stummi requested a review from staffbase-robert June 2, 2022 09:50

staffbase-robert reviewed Jun 3, 2022

View reviewed changes

flaxel reviewed Jun 3, 2022

View reviewed changes

Stummi force-pushed the various-improvements branch 2 times, most recently from d319bf7 to 146d385 Compare June 10, 2022 11:31

Stummi force-pushed the various-improvements branch 4 times, most recently from 9c15d8b to c797fa8 Compare June 13, 2022 11:29

flaxel reviewed Jun 13, 2022

View reviewed changes

src/autodev.ts Show resolved Hide resolved

Stummi force-pushed the various-improvements branch 4 times, most recently from 2e1195f to 181a6db Compare June 15, 2022 06:28

Stummi requested review from staffbase-robert and flaxel June 17, 2022 09:12

flaxel reviewed Jun 20, 2022

View reviewed changes

Stummi added 9 commits June 20, 2022 09:41

Use fixed commit date to produce reproducible dev states

3cc13a1

Simplify hasDiff logic

084c9cf

Remove optimistic option

00070e9

It is never used, and just add unnecessary complexity

Improve merge logic

ae2f080

Set local config instead of global

dc172e7

Add comment

d3a8e79

add origin branch to git push

800e868

use origin/ for graft commit

ec344e5

npm run all

ef96d46

Stummi force-pushed the various-improvements branch from 181a6db to ef96d46 Compare June 20, 2022 07:42

flaxel approved these changes Jun 20, 2022

View reviewed changes

staffbase-robert approved these changes Jun 20, 2022

View reviewed changes

Stummi merged commit 2df605a into main Jun 20, 2022

Stummi deleted the various-improvements branch June 20, 2022 10:39

github-actions bot locked and limited conversation to collaborators Jun 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Merge and Commit logic #75

Improve Merge and Commit logic #75

Stummi commented Jun 1, 2022 •

edited

Loading

github-actions bot commented Jun 1, 2022 •

edited

Loading

Stummi commented Jun 1, 2022

staffbase-robert Jun 1, 2022

Stummi Jun 2, 2022

staffbase-robert Jun 2, 2022

staffbase-robert Jun 2, 2022

Stummi Jun 2, 2022

Stummi Jun 2, 2022

flaxel Jun 3, 2022

Stummi Jun 3, 2022

Stummi Jun 2, 2022

Stummi Jun 2, 2022

Stummi Jun 2, 2022

Stummi Jun 2, 2022

staffbase-robert left a comment •

edited

Loading

flaxel left a comment

flaxel Jun 3, 2022

flaxel commented Jun 3, 2022

Stummi commented Jun 13, 2022

flaxel commented Jun 13, 2022 •

edited

Loading

Stummi commented Jun 15, 2022 •

edited

Loading

flaxel Jun 20, 2022

flaxel Jun 20, 2022

Stummi Jun 20, 2022

Stummi Jun 20, 2022

Stummi Jun 20, 2022

staffbase-robert left a comment


		// only push to defined branch if there are changes
		await exec('git fetch')

Improve Merge and Commit logic #75

Improve Merge and Commit logic #75

Conversation

Stummi commented Jun 1, 2022 • edited Loading

Type of Change

Description

Checklist

github-actions bot commented Jun 1, 2022 • edited Loading

Stummi commented Jun 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staffbase-robert left a comment • edited Loading

Choose a reason for hiding this comment

flaxel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flaxel commented Jun 3, 2022

Stummi commented Jun 13, 2022

flaxel commented Jun 13, 2022 • edited Loading

Stummi commented Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staffbase-robert left a comment

Choose a reason for hiding this comment

Stummi commented Jun 1, 2022 •

edited

Loading

github-actions bot commented Jun 1, 2022 •

edited

Loading

staffbase-robert left a comment •

edited

Loading

flaxel commented Jun 13, 2022 •

edited

Loading

Stummi commented Jun 15, 2022 •

edited

Loading