Skip to content

Conversation

carlosmn
Copy link
Member

@carlosmn carlosmn commented Oct 2, 2015

It's not just us who wants fame and notoriety, some tools using the library want it to, and want their name in the User-Agent field; let's let them do that.

We do have to keep the "git/1.0" so that web servers still recognise us as wishing to talk to Git rather than a web server.

This is missing the plain/ssh support, since we don't currently send the agent over there. We should also add this capability, which might happen in this PR or not. It would be a new behaviour rather than a change, and would require us to potentially change the string given by the user, as we'd need a slug rather than a generic string.

@ethomson
Copy link
Member

Let's free the user-agent in git_libgit2_shutdown - otherwise, I think this is good to go.


cl_assert_equal_p(NULL, git_libgit2__user_agent());
cl_git_pass(git_libgit2_opts(GIT_OPT_SET_USER_AGENT, custom_name));
cl_assert_equal_s(custom_name, git_libgit2__user_agent());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test feels rather obvious. It would be oh-so-much-better if we could test that the generated HTTP requests do have the custom user agent. ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed it would be, but actually adding this to the testing is a large amount of work compared to the impact of this (and the impact of it breaking). We do want a way to test header fields in a more general way, and we will work on that to test higher-impact changes, but not for this largely cosmetic change.

@ethomson
Copy link
Member

👍 Let's commit to getting some broader test coverage here for things like custom headers, the user agent, etc. In the meantime, this is nice and useful and let's merge it and circle back on the tests.

(How many Microsoft-isms did I fit in to that one sentence? Hehe.)

We still prefix it with "git/1.0" since that's required in many
situations, but we replace the area which mentions libgit2.
We also keep the "git/1.0" prefix in order to maintain compatibility
with hosters.
carlosmn added a commit that referenced this pull request Nov 12, 2015
Support setting custom user-agent
@carlosmn carlosmn merged commit ecdc042 into master Nov 12, 2015
@carlosmn carlosmn deleted the cmn/custom-agent branch November 12, 2015 18:20
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 this pull request may close these issues.

4 participants