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

Merging branches doesn't show any author name #6956

Closed
mthirani2021 opened this issue Jun 1, 2023 · 23 comments · Fixed by #7039
Closed

Merging branches doesn't show any author name #6956

mthirani2021 opened this issue Jun 1, 2023 · 23 comments · Fixed by #7039

Comments

@mthirani2021
Copy link

While trying to merge a branch "branchdemo" into "main", it seems there is no default author name added (not being sent as response)

What happened:
Tried to merge a branch into main using the below API and request payload:
POST trees/main%4058346f07f41dd2033082cec7be5311010b1644febb19c016777a44c4752cac25/history/merge

{"fromRefName":"branchdemo","fromHash":"621f99726e4263a32eedf80c2270c739054407adf0e4759518fb61670465b3f7"}

Response:

{
    "wasApplied": true,
    "wasSuccessful": true,
    "resultantTargetHash": "521c292e2b39a032fdd57c421d488120b90741c6560e9cebf470fbbb73d13ad2",
    "commonAncestor": "1b91af97348ae86e388e355c144409279bca48698454d8fcda4e197e09aac251",
    "targetBranch": "main",
    "effectiveTargetHash": "58346f07f41dd2033082cec7be5311010b1644febb19c016777a44c4752cac25",
    "expectedHash": "58346f07f41dd2033082cec7be5311010b1644febb19c016777a44c4752cac25",
    "details": []
}

What you expected to happen:
Response should contain the author name (defaulted to original user probably)

How to reproduce it (as minimally and precisely as possible):

  1. Create a common commit before creating a branch from main
  2. Create a branch from main
  3. Create a commit in branch (make sure that it has authorname passed to API)
  4. Merge the branch into main

Details:

  • Nessie server type (docker/built from source) and version: 0.60.1
  • Client type (Ex: UI/Spark/pynessie ...) and version:
@mthirani2021
Copy link
Author

@dimas-b FYI

@snazy
Copy link
Member

snazy commented Jun 2, 2023

org.projectnessie.model.MergeResponse is defined to not contain any commit details - it returns the commit-ID of the merge commit. Note that we cannot change MergeResponse, because REST API v2 is declared "GA" now.

@mthirani2021
Copy link
Author

I was expecting to get authorname in log entries response (if not in merge response).

Response for log entries looks like below:

GET /trees/main/history?fetch=MINIMAL&filter=&max-records=50

"logEntries": [
        {
            "commitMeta": {
                "hash": "dc793cd98254599e6be62ceb8f6c37e077c5afd913658c53883da34f6d92072d",
                "committer": "",
                "authors": [
                    ""
                ],
                "allSignedOffBy": [],
                "message": "Merged 3 commits from branchdemo at 68aebedb4981a6d023891c683f6e90ef38370be2a9be2c7101937efbc0f5349d into main at de378be9b84046e8b2ab6d8ed9e59f6548d2e7c340aa0618c21cb600b26164aa",
                "commitTime": "2023-06-01T18:23:29.242269960Z",
                "authorTime": "2023-06-01T18:23:29.242269960Z",
                "allProperties": {
                    "_merge_parent": [
                        "68aebedb4981a6d023891c683f6e90ef38370be2a9be2c7101937efbc0f5349d"
                    ]
                },
                "parentCommitHashes": [
                    "de378be9b84046e8b2ab6d8ed9e59f6548d2e7c340aa0618c21cb600b26164aa",
                    "68aebedb4981a6d023891c683f6e90ef38370be2a9be2c7101937efbc0f5349d"
                ]
            },
            "parentCommitHash": "de378be9b84046e8b2ab6d8ed9e59f6548d2e7c340aa0618c21cb600b26164aa"
        }

@snazy
Copy link
Member

snazy commented Jun 2, 2023

Do other commits have author & committer information?

@mthirani2021
Copy link
Author

mthirani2021 commented Jun 2, 2023

Yes, it does have author name.

"logEntries": [
        {
            "commitMeta": {
                "hash": "dc793cd98254599e6be62ceb8f6c37e077c5afd913658c53883da34f6d92072d",
                "committer": "",
                "authors": [
                    ""
                ],
                "allSignedOffBy": [],
                "message": "Merged 3 commits from branchdemo at 68aebedb4981a6d023891c683f6e90ef38370be2a9be2c7101937efbc0f5349d into main at de378be9b84046e8b2ab6d8ed9e59f6548d2e7c340aa0618c21cb600b26164aa",
                "commitTime": "2023-06-01T18:23:29.242269960Z",
                "authorTime": "2023-06-01T18:23:29.242269960Z",
                "allProperties": {
                    "_merge_parent": [
                        "68aebedb4981a6d023891c683f6e90ef38370be2a9be2c7101937efbc0f5349d"
                    ]
                },
                "parentCommitHashes": [
                    "de378be9b84046e8b2ab6d8ed9e59f6548d2e7c340aa0618c21cb600b26164aa",
                    "68aebedb4981a6d023891c683f6e90ef38370be2a9be2c7101937efbc0f5349d"
                ]
            },
            "parentCommitHash": "de378be9b84046e8b2ab6d8ed9e59f6548d2e7c340aa0618c21cb600b26164aa"
        },
        {
            "commitMeta": {
                "hash": "de378be9b84046e8b2ab6d8ed9e59f6548d2e7c340aa0618c21cb600b26164aa",
                "committer": "",
                "authors": [
                    ""
                ],
                "allSignedOffBy": [],
                "message": "Merged 2 commits from branchdemo at 621f99726e4263a32eedf80c2270c739054407adf0e4759518fb61670465b3f7 into main at 521c292e2b39a032fdd57c421d488120b90741c6560e9cebf470fbbb73d13ad2",
                "commitTime": "2023-06-01T18:22:27.637786091Z",
                "authorTime": "2023-06-01T18:22:27.637786091Z",
                "allProperties": {
                    "_merge_parent": [
                        "621f99726e4263a32eedf80c2270c739054407adf0e4759518fb61670465b3f7"
                    ]
                },
                "parentCommitHashes": [
                    "521c292e2b39a032fdd57c421d488120b90741c6560e9cebf470fbbb73d13ad2",
                    "621f99726e4263a32eedf80c2270c739054407adf0e4759518fb61670465b3f7"
                ]
            },
            "parentCommitHash": "521c292e2b39a032fdd57c421d488120b90741c6560e9cebf470fbbb73d13ad2"
        },
        {
            "commitMeta": {
                "hash": "521c292e2b39a032fdd57c421d488120b90741c6560e9cebf470fbbb73d13ad2",
                "committer": "",
                "authors": [
                    ""
                ],
                "allSignedOffBy": [],
                "message": "Merged 2 commits from branchdemo at 621f99726e4263a32eedf80c2270c739054407adf0e4759518fb61670465b3f7 into main at 58346f07f41dd2033082cec7be5311010b1644febb19c016777a44c4752cac25",
                "commitTime": "2023-06-01T18:11:57.648406123Z",
                "authorTime": "2023-06-01T18:11:57.648406123Z",
                "allProperties": {
                    "_merge_parent": [
                        "621f99726e4263a32eedf80c2270c739054407adf0e4759518fb61670465b3f7"
                    ]
                },
                "parentCommitHashes": [
                    "58346f07f41dd2033082cec7be5311010b1644febb19c016777a44c4752cac25",
                    "621f99726e4263a32eedf80c2270c739054407adf0e4759518fb61670465b3f7"
                ]
            },
            "parentCommitHash": "58346f07f41dd2033082cec7be5311010b1644febb19c016777a44c4752cac25"
        },
        {
            "commitMeta": {
                "hash": "58346f07f41dd2033082cec7be5311010b1644febb19c016777a44c4752cac25",
                "committer": "",
                "authors": [
                    "dremio"
                ],
                "allSignedOffBy": [],
                "message": "Create namespace key: folder3",
                "commitTime": "2023-06-01T18:11:27.090288067Z",
                "authorTime": "2023-06-01T18:11:27.090288067Z",
                "parentCommitHashes": [
                    "c182da8c7c2b85cf21a5e8607f4be843885e42f58e99c6b97722bd9e0393ae4c"
                ]
            },
            "parentCommitHash": "c182da8c7c2b85cf21a5e8607f4be843885e42f58e99c6b97722bd9e0393ae4c"
        }

@dimas-b
Copy link
Member

dimas-b commented Jun 5, 2023

@mthirani2021 : This should have been fixed by #6641 , but #4562 may be interfering.

Could you please re-test with the latest Nessie release and do just one merge?

@mthirani2021
Copy link
Author

mthirani2021 commented Jun 5, 2023

@dimas-b the behavior is same with one merge too with the latest Nessie - 0.60.1 .

@mthirani2021
Copy link
Author

@dimas-b any updates on this one ?

@dimas-b
Copy link
Member

dimas-b commented Jun 13, 2023

Confirmed. Author data appears to be lost on merge:

$ curl http://localhost:19120/api/v2/trees/-/history
{
  "token" : null,
  "logEntries" : [ {
    "commitMeta" : {
      "hash" : "08ab3f436883ced3301b2dc5152547f50f2a3125f42340ae5304a11769f513b3",
      "committer" : "",
      "authors" : [ "" ],
      "allSignedOffBy" : [ ],
      "message" : "Merged 1 commits from dev at d144e6a7f491245101647555d88f7f3a9b705930eaadbae6c817bb7596395d0e into main at 2004873d170e66f3f20da2fdcc8c7f9b50937f021d2fa925095da71c1dec76c5",
      "commitTime" : "2023-06-13T14:33:26.672258Z",
      "authorTime" : "2023-06-13T14:33:26.672258Z",
      "allProperties" : {
        "_merge_parent" : [ "d144e6a7f491245101647555d88f7f3a9b705930eaadbae6c817bb7596395d0e" ]
      },
      "parentCommitHashes" : [ "2004873d170e66f3f20da2fdcc8c7f9b50937f021d2fa925095da71c1dec76c5", "d144e6a7f491245101647555d88f7f3a9b705930eaadbae6c817bb7596395d0e" ]
    },
    "parentCommitHash" : "2004873d170e66f3f20da2fdcc8c7f9b50937f021d2fa925095da71c1dec76c5",
    "operations" : null
  }, {
    "commitMeta" : {
      "hash" : "2004873d170e66f3f20da2fdcc8c7f9b50937f021d2fa925095da71c1dec76c5",
      "committer" : "",
      "authors" : [ "test" ],
      "allSignedOffBy" : [ ],
      "message" : "Create namespace key: f1",
      "commitTime" : "2023-06-13T14:30:23.379376Z",
      "authorTime" : "2023-06-13T14:30:23.379376Z",
      "parentCommitHashes" : [ "2e1cfa82b035c26cbbbdae632cea070514eb8b773f616aaeaf668e2f0be8f10d" ]
    },
    "parentCommitHash" : "2e1cfa82b035c26cbbbdae632cea070514eb8b773f616aaeaf668e2f0be8f10d",
    "operations" : null
  } ],
  "hasMore" : false
}
$ curl http://localhost:19120/api/v2/trees/dev/history
{
  "token" : null,
  "logEntries" : [ {
    "commitMeta" : {
      "hash" : "d144e6a7f491245101647555d88f7f3a9b705930eaadbae6c817bb7596395d0e",
      "committer" : "",
      "authors" : [ "test" ],
      "allSignedOffBy" : [ ],
      "message" : "Create namespace key: f2",
      "commitTime" : "2023-06-13T14:32:17.837472Z",
      "authorTime" : "2023-06-13T14:32:17.837472Z",
      "parentCommitHashes" : [ "2004873d170e66f3f20da2fdcc8c7f9b50937f021d2fa925095da71c1dec76c5" ]
    },
    "parentCommitHash" : "2004873d170e66f3f20da2fdcc8c7f9b50937f021d2fa925095da71c1dec76c5",
    "operations" : null
  }, {
    "commitMeta" : {
      "hash" : "2004873d170e66f3f20da2fdcc8c7f9b50937f021d2fa925095da71c1dec76c5",
      "committer" : "",
      "authors" : [ "test" ],
      "allSignedOffBy" : [ ],
      "message" : "Create namespace key: f1",
      "commitTime" : "2023-06-13T14:30:23.379376Z",
      "authorTime" : "2023-06-13T14:30:23.379376Z",
      "parentCommitHashes" : [ "2e1cfa82b035c26cbbbdae632cea070514eb8b773f616aaeaf668e2f0be8f10d" ]
    },
    "parentCommitHash" : "2e1cfa82b035c26cbbbdae632cea070514eb8b773f616aaeaf668e2f0be8f10d",
    "operations" : null
  } ],
  "hasMore" : false
}

@snazy
Copy link
Member

snazy commented Jun 13, 2023

Sorry, I do not really understand what "lost" means here?

Is the author from the merge-operation input parameter commitMeta not applied?
Something else?

@dimas-b
Copy link
Member

dimas-b commented Jun 13, 2023

Merging without providing an explicit "author" in the commit meta of the merge request does not propagate the authors from the commits being merged.

I believe it is quite reasonable for Nessie to fill in authors from the merge source commit, if an override is not specified.

My merge request for the example above was something like this (sorry for not including it up front):

$ curl -H 'content-type: application/json' -X POST http://localhost:19120/api/v2/trees/main@2e1cfa82b035c26cbbbdae632cea070514eb8b773f616aaeaf668e2f0be8f10d/history/merge \
  -d '{"fromRefName":"dev", "fromHash":"d144e6a7f491245101647555d88f7f3a9b705930eaadbae6c817bb7596395d0e"}'

snazy added a commit to snazy/nessie that referenced this issue Jun 13, 2023
snazy added a commit that referenced this issue Jun 14, 2023
Move commit-meta-updater to separate class

... and add some tests

Fixes #6956
@mthirani2021
Copy link
Author

Should I be able to test this in next released version ?

@snazy
Copy link
Member

snazy commented Jun 14, 2023

It's merged and will be included in 0.62.0+

@snazy
Copy link
Member

snazy commented Jun 14, 2023

Also in the next published SNAPSHOT release

@mthirani2021
Copy link
Author

Got it, thanks!

@snazy
Copy link
Member

snazy commented Jun 15, 2023

(Duplicating the comment from #7039) Note: the "pull information from merged commits" clashes with #7035 / #4562, because that will support merging "heavily nested trees" and identifying the "merged commits" would become extremely difficult. Therefore the new storage model will only set the author to the committer.

@mthirani2021
Copy link
Author

We should be able to test in new storage model (default model) in OSS Nessie in 0.62.+? Is this what you mean here?
Also I see the PR #7039 gets removed from 0.62.0 Milestones so I am not sure if this means fix is not available in that version as well.

@snazy
Copy link
Member

snazy commented Jun 15, 2023

It means: "Therefore the new storage model will only set the author to the committer." (... of the merge)

@mthirani2021
Copy link
Author

Yes I understand, but this is what I have tested with new storage model in 0.60.1 / 0.61.0
and if you mean in 0.62.0 then I am seeing the dependent PR gets removed from Milestones.
So I am little confused here.

@snazy
Copy link
Member

snazy commented Jun 15, 2023

Not sure what you mean with „tested in 0.60/61“, because the associated PR has not been released and it’s behavior will not be released because of the merge-base PR

@mthirani2021
Copy link
Author

I see, to be clear this looks like a blocker to test until #7035 and #4562 gets done and then we can release the associated PR #7039

@snazy
Copy link
Member

snazy commented Jun 15, 2023

Again: #7039 will NOT be released with the functionality to collect stuff from „merged commits“.

@mthirani2021
Copy link
Author

Got it; So just wait till #7035 and #4562 gets released.

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 a pull request may close this issue.

3 participants