Add GitCommitterIdentityHandler and change identity check

[metalhead95]

User should be asked for name and email only when none of the following conditions are satisfied -

• git config contains user.name and user.email
• GIT_COMMITTER_NAME and GIT_COMMITTER_EMAIL are set
• EMAIL is set

If any of the above is true, git doesn't show warning to set the author info.

[fcollonval]

Hey @metalhead95 thank you for this PR. I apologize for the review as it will require quite some changes:

• Don't create a new endpoint for this. The environment variables are overidding the git configuration. So instead of a new endpoint, please overidde the configuration after it is read by the environment value. So the code should be placed after those lines:

jupyterlab-git/jupyterlab_git/git.py

Lines 170 to 180 in 5ce2624

	cmd = ["git", "config", "--list"]
	code, output, error = await execute(cmd, cwd=top_repo_path)
	response = {"code": code}
	if code != 0:
	response["command"] = " ".join(cmd)
	response["message"] = error.strip()
	else:
	raw = output.strip()
	s = CONFIG_PATTERN.split(raw)
	response["options"] = {k:v for k, v in zip(s[1::2], s[2::2]) if k in ALLOWED_OPTIONS}

• Could you add the environment GIT_AUTHOR_* variables? From the documentation:

GIT_AUTHOR_NAME is the human-readable name in the “author” field.
GIT_AUTHOR_EMAIL is the email for the “author” field.
GIT_COMMITTER_NAME sets the human name for the “committer” field.
GIT_COMMITTER_EMAIL is the email address for the “committer” field.
EMAIL is the fallback email address in case the user.email configuration value isn’t set.

Comment - unneeded if you apply the above changes, I know the API REST is bad. But let's stick with it for now (i.e. do not use query argument).

[metalhead95]

Thanks @fcollonval for reviewing this pr.

I checked git source code to see when identity advice is displayed, it is platform dependent (code). For Windows, both name and email should be provided but for non-windows platform just email info is sufficient since name is read from gecos field by default.

The environment variables are overriding the git configuration. So instead of a new endpoint, please override the configuration after it is read by the environment value.

If we go ahead with this approach, in case of non-windows platform user.name needs to be set from gecos field which is repeating what git does by default. Should we use a separate handler or override config given this new info?

Could you add the environment GIT_AUTHOR_* variables? From the documentation:

GIT_AUTHOR_* is not checked while deciding if identity advice should be displayed. Only committer info is needed (code).

[fcollonval]

First of all, sorry for the late answer and thank you for the detailed answer.

As you describe, git is doing quite some actions to figure out the committer identity. So I wonder if the best approach is not to try first the commit action. And if it fails because of missing identity, this will prompt the user to set his identity.

This would simplify the code (and avoid duplicate complex code git is already doing) at the expense of having not nice identity for newbies that are not aware of git identity guess.

What do you think?

[metalhead95]

Yes, this approach is perfect. However, there isn't a way to fail if committer identity is not present, only way to identify this is to check if Your name and email address were configured automatically is present in output.

Does this check sound good to you?

[fcollonval]

Yes, this approach is perfect. However, there isn't a way to fail if committer identity is not present, only way to identify this is to check if Your name and email address were configured automatically is present in output.

Do you mean the git commit command is not failing (i.e. return code is not 0) if committer identity cannot be resolved?

[metalhead95]

Do you mean the git commit command is not failing (i.e. return code is not 0) if committer identity cannot be resolved?

Yes, exit code is 0. git commit runs successfully even if committer identity is not present, it only prints an advice to set name and email.

[fcollonval]

Ok then let's try catching the sentence in the output.

About the workflow, this means you will amend the commit with the wrong identity when it is set by the user?

[metalhead95]

First of all sorry for taking so long for this.

About the workflow, this means you will amend the commit with the wrong identity when it is set by the user?

Yes, workflow is commit -> prompt user for identity if required -> reset author identity.

[cboettig]

any news on this?