Community
Participate
Working Groups
We should be able to specify the committer and author names/mails for a commit.
*** Bug 351442 has been marked as a duplicate of this bug. ***
Fix: https://github.com/pjanik/orion.client/tree/bug350422 https://github.com/pjanik/orion.server/tree/bug350422 I wrote all this code and have the rights to contribute it to Eclipse under the eclipse.org web site terms of use.
See my comments regarding server side here: https://github.com/pjanik/orion.server/commit/ec6ae9b31ab56ce0141298a6eaaa1cc1b1ddf5fa
(In reply to comment #3) > See my comments regarding server side here: > https://github.com/pjanik/orion.server/commit/ec6ae9b31ab56ce0141298a6eaaa1cc1b1ddf5fa Fixed: https://github.com/pjanik/orion.server/tree/bug350422
Let's start from the server side, I have bad feelings about these lines: String committerName = requestObject.optString(GitConstants.KEY_COMMITTER_NAME, ""); ... if (!committerName.isEmpty() || !committerEmail.isEmpty()) commit.setCommitter(committerName, committerEmail); I would rather see those values defaulted to null and passed like that to the CommitCommand. According the its javadoc a null value should result in a fallback. However, after running a simple test for the command it turned out it doesn't seem to work as documented, if that's the case then we should file about a bug for JGit saying their API is not precise. Tests: You haven't touched the part in GitCommitTest#getPostGitCommitRequest where extra params for committer/author are added to the body. I would add null checks there. As for the UI part, I think we all agree that the Git Status page is already cluttered. Putting 4 more text fields there is not an option. They should be hidden in an expandable widget (a div?) and only visible on special occasion - when user decides to change them. Moreover, these fields could be pre-populated with default values. I guess EGit does a similar thing in its Commit dialog. Last but not least, are you able to set name or email to "" (empty string) with the proposed UI/server side logic? It's not crucial, but a valid scenario in git bash.
New version: https://github.com/pjanik/orion.client/tree/bug350422 https://github.com/pjanik/orion.server/tree/bug350422 (In reply to comment #5) > Let's start from the server side, I have bad feelings about these lines: > > String committerName = requestObject.optString(GitConstants.KEY_COMMITTER_NAME, > ""); > ... > if (!committerName.isEmpty() || !committerEmail.isEmpty()) > commit.setCommitter(committerName, committerEmail); > > I would rather see those values defaulted to null and passed like that to the > CommitCommand. According the its javadoc a null value should result in a > fallback. However, after running a simple test for the command it turned out it > doesn't seem to work as documented, if that's the case then we should file > about a bug for JGit saying their API is not precise. Yes, null support is broken/different from javadoc, so it was the cause of that approach. But I agree that it could be done better. Now GitCommitHandlerV1 handles null values itself, so it's workaround for the mentioned JGit bug. > Tests: You haven't touched the part in GitCommitTest#getPostGitCommitRequest > where extra params for committer/author are added to the body. I would add null > checks there. JSONObject.put(...) javadoc: "Put a key/value pair in the JSONObject. If the value is null, then the key will be removed from the JSONObject if it is present." So, I omit null checks, as null values won't be inserted anyway. > As for the UI part, I think we all agree that the Git Status page is already > cluttered. Putting 4 more text fields there is not an option. They should be > hidden in an expandable widget (a div?) and only visible on special occasion - > when user decides to change them. Moreover, these fields could be pre-populated > with default values. I guess EGit does a similar thing in its Commit dialog. Done. Inputs are hidden by default. There is a command, which shows them. They are pre-populated with default values. > Last but not least, are you able to set name or email to "" (empty string) with > the proposed UI/server side logic? It's not crucial, but a valid scenario in > git bash. Yes, now you are able to set name/email to "".
(In reply to comment #6) > > According the its javadoc a null value should result in a > > fallback. However, after running a simple test for the command it turned out > > it doesn't seem to work as documented, if that's the case then we should file > > about a bug for JGit saying their API is not precise. > Yes, null support is broken/different from javadoc, so it was the cause of that > approach. But I agree that it could be done better. Now GitCommitHandlerV1 > handles null values itself, so it's workaround for the mentioned JGit bug. Have you filed a bug against JGit?
JGit bug: https://bugs.eclipse.org/bugs/show_bug.cgi?id=352984
(In reply to comment #6) > Done. Inputs are hidden by default. There is a command, which shows them. They > are pre-populated with default values. Being able to show hidden fields is great, but I would like to be able to hide them again, when not needed. Just like on Repositories page when you want to clone a repo and you're picking target location: Choose target location / Default target location. Also, the default values are not accurate. Go to Profile page, change values for Git mail and Git name and clone a repo. Or just change these values for an existing repository. When you then go the Status page you will see that suggested values for committer and author are not those you would expect. They should be taken from the repo configuration or user profile. Finally, I would place label and both fields in a single line, so they take less space. Something like this would work, imo "Committer name: [...] mail: [...]". I'm fine with the server side changes, thanks for the update.
> Being able to show hidden fields is great, but I would like to be able to hide > them again, when not needed. Just like on Repositories page when you want to > clone a repo and you're picking target location: Choose target location / > Default target location. Done. > Also, the default values are not accurate. Go to Profile page, change values > for Git mail and Git name and clone a repo. Or just change these values for an > existing repository. When you then go the Status page you will see that > suggested values for committer and author are not those you would expect. They > should be taken from the repo configuration or user profile. Default values are taken from the repo configuration. > Finally, I would place label and both fields in a single line, so they take > less space. Something like this would work, imo "Committer name: [...] mail: > [...]". Done.
Fixed with: http://git.eclipse.org/c/orion/org.eclipse.orion.server.git/commit/?id=806c6a80be47766bddd5c3fab030d19d3cbb7846 and http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=50736124ee1e3c01b8f4e359951088702367f5a6. Filed bug 353096 for a minor issue with resetting the fields when staging/unstaging files.
