Null pointer check added for system package

[Khushboo-Sharma-110597]

Hashtable does not store null values , so if systempackage comes as null it will throw null pointer exception
Fixes :- #1453

[gireeshpunathil]

congratulations @sharmakh for your first contribution to PDE! 🎉

[merks]

@gireeshpunathil

For future reference, there was a reason why I added @HannesWell to the comments because we don't generally want/accept multi-commit PRs. That's why I explicitly asked for the commit to be amended. We want nice comments in the commits that describe the changes so a commit with "remove whitespace" is just not a nice historical thing in the commit history where that space was added in the preceding commit. We want a clean history without intermediate states...

[gireeshpunathil]

@merks - no worries, I squashed the commits into one and amended the commit message (removed the fixup commit message) before landing - all in the interest of making first time contributor's life easy!

[merks]

The problem with that approach is that messes up the committer information:

So now this commit does not have good information to track the commit back to the account associated with the ECA email address of the contributor. If we'd given @HannesWell some time, that would all have been clarified.

[gireeshpunathil]

oops.. I never knew that the browser based merge is not followed! thanks for explaining @merks ! lets fix this!

[gireeshpunathil]

@sharmakh - can you make your email ID visible in your github profile? I will pick that up and amend this commit.

[Khushboo-Sharma-110597]

Hi , I enabled my email id it should be visible now, i will take care of such things from next time. Sorry for the inconvenience. Regards Khushboo

…

On Fri, Oct 25, 2024 at 5:50 PM Gireesh Punathil ***@***.***> wrote: @sharmakh <https://github.com/sharmakh> - can you make your email ID visible in your github profile? I will pick that up and amend this commit. — Reply to this email directly, view it on GitHub <#1455 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APRLNJN34H5J3BNC4UEAMB3Z5IZQDAVCNFSM6AAAAABQSTGNIGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMZXGYZTKOBWGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[merks]

@sharmakh

No need to apologize. It's not your fault. Your contribution is appreciated and welcome! It's the strange and unexpected behavior of how github "mangles" the author/committer details of the commit. Even I didn't know about that before @HannesWell explained it to me. The original commits would have never passed the ECA if the commits were not well-formed with proper email addresses. It's the squash and merge that's doing this under the covers and unless someone explains that, you would never guess it might do that...

[gireeshpunathil]

ok, I amended the commit locally, but cannot push the change because of the divergence with the remote, and force push is prohibited by protection branch hook on the master. any pointers?

[merks]

I think this could only be fixed by someone at the foundation with extra privileges. In the end, it's not your fault that you didn't know this would happen. Who would even guess that such a strange thing would happen? None of us would...

I've asked @HannesWell to open a helpdesk issue because in the end, something should have prevented this commit because you aren't the first and won't be the last to do this assuming all is well!

[gireeshpunathil]

thanks @merks ! much appreciated the help!
(different OSS projects with differing rules / conventions led me into this. but ofc, every day, new learning!)

[netomi]

fyi: support for noreply author / commit emails has been added to the ECA check some years ago, see for the reason here:

https://gitlab.eclipse.org/eclipsefdn/it/api/git-eca-rest-api/-/issues/55

This has nothing to do with squash and merge, but if the user decides that his normal email should not be visible in commits, github will add such an email instead. The ECA check does a reverse-lookup of the GitHub handle though to check if the user signed an ECA:

https://webdev.eclipse.org/docs/api/eclipsefdn-profile-api/#tag/User-Profile/operation/RetrieveUserGh

The ECA check for this specific PR also succeeded because of this:

As the user signed the ECA already:

https://api.eclipse.org/github/profile/sharmakh

[netomi]

There is also this setting in your GitHub profile that controls that behavior:

[gireeshpunathil]

thanks @netomi for the clarification. just wondering: much in the way The ECA check does a reverse-lookup of the GitHub handle, why can't the squash and merge flow capture the committer handle (and the associated email), as the button is enabled only for valid and authenticated committers? may be it is a github enhancement?

[laeubi]

thanks @netomi for the clarification. just wondering: much in the way The ECA check does a reverse-lookup of the GitHub handle, why can't the squash and merge flow capture the committer handle (and the associated email), as the button is enabled only for valid and authenticated committers? may be it is a github enhancement?

Even though the committer == the one that pushed the button and / or take action) might look unusual, the important part is the AUTHOR == the one that authored the commit / code change looks correct.

Github Web UI shows this quite nicely:

The same also happens regularly on other places e.g here

so I don't think that much to worry about.

If you squash and merge, then you will usually become the co-author == the one that has modified the original contribution of the author:

The only exception is if you are the author of the PR itself see here

Note: The email selector is not available for rebase merges, which do not create a merge commit.
For squash merges, the email selector is only shown if you are the pull request author and you have
more than one email address associated with your account.

There is also a GitHub blog post with some more details:
https://github.blog/changelog/2022-09-15-git-commit-author-shown-when-squash-merging-a-pull-request/

If you think this is weird, there is the place to join discussions here:

• https://github.com/orgs/community/discussions/69856

There are also many discussions going on if you search for github squash and merge author

• Pull request "Squash and merge" button changes author details isaacs/github#1368
• https://stackoverflow.com/questions/64568308/how-can-one-retain-authorship-of-code-even-when-they-are-merge-it-with-git-squas
• ... many others discussions as well ...

[HannesWell]

First of all thank you @sharmakh for this contribution and sorry for the confusion it triggered and don't worry about that.

The only thing that I would like to ask you is to use your full name as git user name and a 'real' email address as git user email for your submitted commits, like the one you have in your profile, so that the author info eventually is something like Jane Doe <jane.doe@mail>. From the commits you created for this PR I assume you used your GH noreply mail.

When using the noreply mails from GH it makes it hard or even impossible for anybody to reach you through mail, even if I'm a real person that has checked out the repo and have the best intends. I don't know if the EF provides a service for persons to do the reverse mapping, but if for example GH stops working one day even these reverse mappings could be come impossible.
Of course email addresses can also be abandoned, but there is at least a chance. Not using your primary private mail address is of course fine, but it would be preferable if you use at least a 'personal' address.

I have created https://gitlab.eclipse.org/eclipsefdn/helpdesk/-/issues/5209 in order to discuss if there should be general rules for Eclipse projects regarding the use of noreply mails.

Also thanks to @gireeshpunathil for helping to make reviews faster. You did your best to submit a good commit, unfortunately the behavior of GH Squash-and-merge action is not something one can expect without knowing it exactly.

The problem when using Squash-and-merge is not the value of the Author but the Committer field. The first commit's Author is retained as expected and that other authors are mentioned as co-authors is also perfectly fine.
But the Committer is always GitHub <noreply@github.com> (see screenshot in #1455 (comment)). So to find out who committed a change one has to visit the GH PR in the web UI and it's a little bit inconvenient to find it, even tough there are multiple ways to find it.
The screenshot in #1455 (comment) show the difference nicely. The first one was submitted using Squash-and-merge and the second one was Rebase-and-merge. And while the first one has GH as committer, the second one has the person that hit the button recorded as committer and also displayed in the UI.

And even if IP wise the Author is the most important part, knowing the committer is also important, as a duty and as an honor.
And last but not least, if one takes it literally GH didn't commit a change, that action was initiated/done by a person, the committer, so I don't find it not suitable to record GitHub as such. I don't know why GH doesn't use the committer for that like it does for rebase and merge, because obviously it should be possible and not hard.

Therefore I strongly discourage every committer from using Squash-and-merge as a PR. As a lead of PDE I have also requested the EF infra-team to disable Squash-And-merge for PDE via eclipse-pde/.eclipsefdn#4.
I'll also send a mail on the pde-dev mailing list, but unless one has really good reasons to use Squash-and-merge or until GH supports using the real committer as committer I prefer not to use it.
But the latter will probably not happen soon, see https://github.com/orgs/community/discussions/32934.

[netomi]

I can perfectly understand that the behavior of squash and merge wrt the actual committer info especially for PRs from external contributors is confusing at times and you would like to avoid it in some projects. This confusion is mostly related to using Eclipse / jgit as a tool imho as in all other tools that I use, the committer information is mostly hidden anyways and the focus is entirely on the author. So its perfectly fine to limit the use of squash and merge for projects that are predominantly used in an Eclipse IDE context.

From an ECA perspective, there are some identities that are whitelisted, e.g. noreply@github.com, as it just happens too often that somebody does something via the Web UI which also uses this committer info, and such commits would be rejected or flagged otherwise which we do not want as its just a technical detail imho.

[netomi]

btw. author information can easily be faked, requiring signed commits would actually be a safe-guard against that, however it might be tricky to instruct the community to do so when contributing patches. It might result in a lot of extra work, but at some point in time it will be necessary imho.