Re: [PATCH] Added AUTH NTLM for SMTP
Hi Steve, 2011/8/24 Yang Tse wrote: Right now I see nothing relative to all this in already provided patches. I'll dig a bit further in case it is hidden somewhere, but you are the one who actually knows by heart where should we arrive. Yep, your initial consolidated patch already included moving the base64 encoding of type-1 and type-3 messages to Curl_ntlm_create_typeX_message functions. To stop all this 'if we should', 'if it is really desirable' on my side, given that it really seems to be the right thing to do, and to allow us to move on. I've just pushed this change into master. In a while I'll comment on the 'SMTP Modifications' thread. -- -=[Yang]=- --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
Re: [PATCH] Added AUTH NTLM for SMTP
Hi Steve, 2011/8/22 Steve Holme wrote: [...] The only thing I would like to clarify is the call to the Curl_base64_encode() that we talked about. Fixing of Curl_base64_encode() and Curl_base64_decode() interfaces pushed now into master. This fix touches more source files than I had initially thought. In any case, this fix is now finally done. So this should no longer be an issue for proper cleanup and error handling. Lets give autobuilds, or any human, the chance of detecting any pitfall with this commit. We have currently implemented Option 2 out of the two options... Should we change this to Option 1 (being called as part of Curl_ntlm_create_typeX_message() functions) now or after I add NTLM support to SMTP? Relative to this, it mostly depends if we really need protocol specific implementations of Curl_input_ntlm() and/or Curl_output_ntlm() or if existing ones can be used for all of them. You'll have to elaborate a bit on that before I'm actually capable of answering your question above. Next patch should be the code that handles sending initial response in the AUTH command or in the next one. Unless that moving the base64 encoding of type-1 and type-3 messages out to Curl_ntlm_create_typeX_message() functions first really makes a difference. Right now I see nothing relative to all this in already provided patches. I'll dig a bit further in case it is hidden somewhere, but you are the one who actually knows by heart where should we arrive. -- -=[Yang]=- --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
Re: [PATCH] Added AUTH NTLM for SMTP
2011/8/21 Steve Holme wrote: Last remaining bits extracted from 0001-Moved-ntlm-specific-code-into-separate-curl_ntlm-mod_3.patch have just been pushed to master. Is there really a need for so long names? 65 chars wow! Many thanks - it is much appreciated. I spent just over 2 days (about 20 hours) writing the original modification to Libcurl and since then nearly 35 hours on producing the various patches. We really appreciate your efforts and time in the same way we do with ours, with the added responsibility, efforts and time, on our side of trying not to break existing code and functionality when new features, or important changes, are introduced in order to keep libcurl top notch. Sometimes in order to achieve this it is necessary to push other, apparently non-related, changes in between. At what stage are we now? I see there hasn't been any feedback about the code moving to curl_ntlm.[ch] so can we start to introduce the smtp code changes to introduce the NTLM feature? Stage one is to be considered finished with last commit already done. So I would say, in front of stage two or three. This really depends if there are still pending changes to existing code that are independent of NTLM support for other protocols than http. For example, in first message of this thread you said: * Modified the order of the preferred authentication method to AUTH NTLM, AUTH CRAM-MD5, AUTH LOGIN then AUTH PLAIN. AUTH PLAIN should be the last as it is the most insecure. That would be stage two. Please start a new thread with proposed patch for that, and whatever comments you desire to convince others that the fix/change is convenient. You also said: * Added the ability for the user to specify whether authenticated connections (AUTH NTLM, AUTH LOGIN and AUTH PLAIN) send the initial response in the AUTH command or in the next command. This can be set via the CURLOPT_MAIL_SINGLE_AUTH option but defaults to TRUE to maintain compatibility with the v7.21.7 source. Given that CURLOPT_MAIL_SINGLE_AUTH would be a new option this should be considered stage three as a whole. On the other hand I understand that some glue has to be introduced in existing NTLM code in order to allow handling new option while retaining existing behavior when not used. So there has to be part of that whole that can be introduced now in preparation of stage three and that currently would simply keep existing NTLM operation relative to initial response sending. Provide patch for this preparation on this thread. But please, keep it to the minimum ;-) Don't squash something you have over current git. Ball on your side, -- -=[Yang]=- --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
RE: [PATCH] Added AUTH NTLM for SMTP
Hi Yang, We really appreciate your efforts and time in the same way we You're welcome :) do with ours, with the added responsibility, efforts and time, on our side of trying not to break existing code and functionality when new features, or important changes, are introduced in order to keep libcurl top notch. I know I'm stating the obvious here but I think it's safe to safe that everyone's efforts are fully appreciated and I do value your help and assistance on this patch. At what stage are we now? I see there hasn't been any feedback about the code moving to curl_ntlm.[ch] so can we start to introduce the smtp code changes to introduce the NTLM feature? Stage one is to be considered finished with last commit already done. So I would say, in front of stage two or three. This really depends if there are still pending changes to existing code that are independent of NTLM support for other protocols than http. I think we are about there with the NTLM changes... The only thing I would like to clarify is the call to the Curl_base64_encode() that we talked about. We have currently implemented Option 2 out of the two options... Should we change this to Option 1 (being called as part of Curl_ntlm_create_typeX_message() functions) now or after I add NTLM support to SMTP? That would be stage two. Please start a new thread with proposed patch for that, and whatever comments you desire to convince others that the fix/change is convenient. I should hopefully have this ready this evening, if not tomorrow at the latest. Kind Regards Steve --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
RE: [PATCH] Added AUTH NTLM for SMTP
Hi Yang, There's no real need for you to generate an additional patch, I can manage from this point without it. But of course you can provide it if you wish so ;-) Many thanks - it is much appreciated. I spent just over 2 days (about 20 hours) writing the original modification to Libcurl and since then nearly 35 hours on producing the various patches. At what stage are we now? I see there hasn't been any feedback about the code moving to curl_ntlm.[ch] so can we start to introduce the smtp code changes to introduce the NTLM feature? Kind Regards Steve --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
Re: [PATCH] Added AUTH NTLM for SMTP
Hi Steve, Given that 0001-Moved-ntlm-specific-code-into-separate-curl_ntlm-mod_3.patch still contained portions which were spacing, indentation, capitalization or comment related, I've extracted those parts from it in order to commit that. Additionally I've done some changes to simplify verification of code refactoring, and parts of http_ntlm.h have already been moved to curl_ntlm.h. I'll await tomorrow's autobuild results before going further. There's no real need for you to generate an additional patch, I can manage from this point without it. But of course you can provide it if you wish so ;-) But please no more spacing, indentation nor capitalization 'fixes' while moving code between files. If you are finally going to generate a patch against current git, please provide it in at most 12 hours from this message timestamp. Or simply tell me you won't generate it. Either way is ok. -- -=[Yang]=- --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
RE: [PATCH] Added AUTH NTLM for SMTP
Hi Yang, Sorry I didn't see your email before my patch today - but for some reason Outlook put you in my junk folder :( Pushed your last patch with a couple of edits. Sync with github repo. Cheers . Current interface of Curl_base64_encode and Curl_base64_decode have a slight glitch in a marginal case. The caller of these cannot know if a zero result from the function call is due to an out of memory condition or due to a zero length encoding or decoding. Yes - I had noticed that ;-) Without modifying Curl_base64_* right now, we have two options here... [..] As soon as other protocols ;-) start using the result of all this... Option 1 is a better refactoring result. Yes I agree option 1 is the better solution. Having, http_ntlm, smtp, pop3 (which I hope to work on later), etc... all call Curl_ntlm_create_typeX_message() and then call Curl_base64_encode() is not only long winded for the developer but also less efficient especially if each one has their own temporary ntlm buffer. Option 2 is easier to achieve right now, given that part of the existing code would not move. Arrgghh - That means I've got to rework my patch :( S. --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
Re: [PATCH] Added AUTH NTLM for SMTP
2011/8/10 Steve Holme wrote: Hiya, :-) No problem but could you please clarify when a function should be #defined as blank in a header file vs #ifdef it in a code file? We try follow two simple rules: 1) Code which is not intended to be used in the binary due to a compile time feature being disabled should not be compiled into the binary. This allows size reduction for those that don't desire all bells and whistles. 2) We attempt to keep to a minimum the number of '#if' preprocessor statements inside source code files in order to make code more readable and maintainable. This usually involves some extra preprocessor magic in internal header files. Being above the general rules, it might happen that you encounter different situations in actual code, for many different reasons. It is appreciated when new code tries to follow them. -- -=[Yang]=- --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
Re: [PATCH] Added AUTH NTLM for SMTP
2011/8/10 Steve Holme wrote: Since then I have cracked on with producing the second patch which is attached. This one moves the ntlm code to the new curl_ntlm module and tries to simple move the code (as was) and not rework it or reformat it, etc... I have spotted a few areas that still need tiding up, that I had missed yesterday, but I will take the opportunity at the end to produce a separate patch for those. I'll skim over this patch to see if I have further comments. But I would really prefer that the stuff related to the yet required tiding up that you are mentioning is provided before the actual refactoring. FYI today's autobuilds have not detected any problem with the already pushed work relative to this thread. You will note that the function declarations for Curl_ntlm_create_type1_message and Curl_ntlm_create_type3_message() are slightly different from my original patch, from last week, Already committed one also was different :-) Doing this step by step helps everyone and the result will be better. You will note that InitializeSecurityContextA() takes host as the third parameter but will see that it is never populated until much later on in the code (the native type 3 creation) and as such an empty string is passed to both InitializeSecurityContextA() calls. According to the MSDN documentation this should specify A pointer to a null-terminated string that indicates the service principal name (SPN) or the security context of the destination server - as such I would recommend renaming the variable to destination or target at some point, but I will do that in a final patch. Same applies here if it can be done before the refactoring step lets do it now. -- -=[Yang]=- --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
RE: [PATCH] Added AUTH NTLM for SMTP
Hi again, On Wed, 10 Aug 2011, Yang Tse wrote: I'll skim over this patch to see if I have further comments. But I would really prefer that the stuff related to the yet required tiding up that you are mentioning is provided before the actual refactoring. It was just the odd space and stuff that I noticed really - certainly nothing major. Already committed one also was different :-) Doing this step by step helps everyone and the result will be better. :) Same applies here if it can be done before the refactoring step lets do it now. I'll have a go at this this evening - as this should be do-able by introducing a new variable but I must admit I wasn't too sure what your preference would be here. Regards Steve --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
RE: [PATCH] Added AUTH NTLM for SMTP
Hi Yang, On Wed, 10 Aug 2011, Steve Holme wrote: I'll have a go at this this evening - as this should be do-able by introducing a new variable but I must admit I wasn't too sure what your preference would be here. Please find attached another patch file which contains the following modifications: * Tidied up more inconsistent spacing. * Move the position of NTLMSSP_SIGNATURE, HOSTNAME_MAX, SHORTPAIR and LONGQUARTET definitions in ready for move to curl_ntlm.c. * Used separate variables for Windows SSPI and native code to ease moving of code to curl_ntlm.c. * Fixed typographical errors where SPPI should be SSPI. * Fixed compilation warnings on 64-bit builds when calling Windows SSPI functions. As such, can you discard my last patch with the curl_ntlm move in? I will patch this up after this patch is pushed ;-) Kind Regards Steve 0001-Tidied-up-more-inconsistent-spacing.patch Description: Binary data --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
Re: [PATCH] Added AUTH NTLM for SMTP
Hi there, As such, can you discard my last patch with the curl_ntlm move in? I will patch this up after this patch is pushed ;-) Relative to that one... It would be nice to keep the handling of out of memory condition when calling Curl_base64_* in the same function where Curl_base64_* is called. Probably it would be better if Curl_ntlm_create_type1_message() and Curl_ntlm_create_type3_message() did not call Curl_base64_encode(). There's no problem with Curl_ntlm_decode_type2_message() given that it handles OOM condition of Curl_base64_decode(). -- -=[Yang]=- --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
Re: [PATCH] Added AUTH NTLM for SMTP
2011/8/10 Steve Holme wrote: Pushed your last patch with a couple of edits. Sync with github repo. So to clarify, are you saying that the calls to Curl_base64_encode () should be moved back into Curl_output_ntlm () ? Current interface of Curl_base64_encode and Curl_base64_decode have a slight glitch in a marginal case. The caller of these cannot know if a zero result from the function call is due to an out of memory condition or due to a zero length encoding or decoding. As a consequence, letting Curl_ntlm_create_type1_message() and Curl_ntlm_create_type3_message() directly return the result of a Curl_base64_encode() call only makes things worse. Without modifying Curl_base64_* right now, we have two options here... 1) Curl_ntlm_create_type1_message() and Curl_ntlm_create_type3_message() calls Curl_base64_encode() _and_ checks its result and cleans up whatever these have allocated in case of OOM and returns failure. 2) Curl_ntlm_create_type1_message() and Curl_ntlm_create_type3_message() do not call Curl_base64_encode() and the check and clean up is done where it is right now. As soon as other protocols ;-) start using the result of all this... Option 1 is a better refactoring result. Option 2 is easier to achieve right now, given that part of the existing code would not move. In my previous message I was suggesting option 2. But option 1 should be better in a not distant future. -- -=[Yang]=- --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
RE: [PATCH] Added AUTH NTLM for SMTP
Hi Yang, On Mon, 8 Aug 2011, Yang Tse wrote: i.e... In http_ntlm.c I have attached my first patch, which implements most of what you outlined (and a few other format issues that I found) tiding up the existing code in the http_ntlm module. Kind Regards Steve 0001-Tidied-up-http_ntlm-prior-to-splitting-the-ntlm-spec.patch Description: Binary data --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
Re: [PATCH] Added AUTH NTLM for SMTP
2011/8/9 Steve Holme wrote: I have attached my first patch, which implements most of what you outlined (and a few other format issues that I found) tiding up the existing code in the http_ntlm module. Pushed right now by followed by another one to fix some nits. TAB characters, lines longer than 79 columns, and trailing blanks. cURL Coding Standards at http://curl.haxx.se/dev/contribute.html has relevant info. Adjusted preprocessor block in now named Curl_http_ntlm_cleanup(), and also some preprocessor magic in http_ntlm.h in order to allow les usage of #idef's in code using that header. Additionally I removed an inclusion of sys/types.h that already was in http_ntlm.c and that is not required. Pull or pull and rebase to keep the good work. -- -=[Yang]=- --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
RE: [PATCH] Added AUTH NTLM for SMTP
Hiya, On Tues, 9 Aug 2011, Yang Tse wrote: I have attached my first patch, which implements most of what you outlined (and a few other format issues that I found) tiding up the existing code in the http_ntlm module. Pushed right now by followed by another one to fix some nits. TAB characters, lines longer than 79 columns, and trailing blanks. cURL Coding Standards at http://curl.haxx.se/dev/contribute.html has relevant info. Great and cheers for the tips. Sorry about the tabs though - at least I can blame Visual Studio for that as my everyday environment is set up for keep tabs and set to display as 4 characters per tab. I kept having to tidy them up manually replacing them with two spaces but obviously I missed a few :( Adjusted preprocessor block in now named Curl_http_ntlm_cleanup(), and also some preprocessor magic in http_ntlm.h in order to allow les usage of #idef's in code using that header. Additionally I removed an inclusion of sys/types.h that already was in http_ntlm.c and that is not required. No problem but could you please clarify when a function should be #defined as blank in a header file vs #ifdef it in a code file? For example: Curl_input_ntlm() and Curl_output_ntlm() are #ifdef USE_NTLM in http.c but Curl_http_ntlm_cleanup is #defined as blank when USE_NTLM is not defined. As such I would have thought it would be better to be consistent, which in my opinion would causes less confusion when reading the header file - then we would wrap the call to Curl_http_ntlm_cleanup() in a #ifdef USE_NTLM in url.c. Pull or pull and rebase to keep the good work. Hopefully I will get some more time tomorrow to rebase and move things out to curl_ntlm. How long do you want to leave it between submissions? Kind Regards Steve --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
Re: [PATCH] Added AUTH NTLM for SMTP
2011/8/5 Steve Holme wrote: Finally after my woes with Git I have the pleasure of including a single patch of my (various) changes in adding support for AUTH NTLM to SMTP. A summary of the changes is as follows: [...] I have attached the patch file for your review and would appreciate any feedback or suggestions. Steve, I reviewed this patch a couple of times and have allowed 24 hours before replying to you, so my comments here are not a first impression but a quite reflexive one. In order to allow NTLM auth for SMTP this basically refactors existing NTLM code and also adds the required bits for NTLM auth for SMTP support, while also changing parts of existing code. In order to properly integrate all this with existing code I would appreciate if we did all this actually in three different steps. First one, patch existing code with changes that you deem necessary. There are changes that I have not had the time to investigate if they are fixes that you introduce or if they are regressions for fixes introduced in git while you developed your version. In this phase all changes should be done to files already existing in git repo, no new files. Second one, refactor existing code into whatever functions you believe are appropriate. It is important to not change code while refactoring it into functions. If you estimate that curl_ntlm.c and curl_ntlm.h are necessary they can be created at this phase. We should be able to verify with text comparison tools that no change is introduced besides converting existing code into functions. Third one, introduce the NTLM auth for SMTP support. Each phase should be reviewed, and a whole run of autobuilds should be allowed in between to minimize the risk that something else gets disturbed. Thanks, -- -=[Yang]=- --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
RE: [PATCH] Added AUTH NTLM for SMTP
Hi, On Mon, 8 Aug 2011, Yang Tse wrote: In order to properly integrate all this with existing code I would appreciate if we did all this actually in three different steps. First one, patch existing code with changes that you deem necessary. I guess this would involve the changes to curl.h, urldata.h and url.c for the new MAIL_AUTH and MAIL_SINGLE_AUTH options as well as the modifications in smtp.c to support these options. Second one, refactor existing code into whatever functions you believe are appropriate. It is important to not change code while refactoring it into functions. I would say this is quite difficult... Whilst most of the crypto functions moved from http_ntlm to curl_ntlm without any modification , the actual ntlm message functions were embedded in the contents of Curl_input_ntlm() and Curl_output_ntlm (). As such they had to be reworked quite a bit to turn them into the stand-alone functions: Curl_ntlm_create_type1_message(), Curl_ntlm_decode_type2_message() and Curl_ntlm_create_type3_message(). Third one, introduce the NTLM auth for SMTP support. This would be the new functions that I added to the smtp module: smtp_auth_ntlm(), smtp_state_auth_ntlm_resp() and smtp_state_auth_ntlm_type2_resp() as well as the modifications to smtp_authenticate() and smtp_statemach_act () to support this. Do you want me to start producing a patch for part one or are you able to push the modified curl.h, urldata.h and url.c into the repository whilst I rework smtp.c to remove the ntlm code? In the meantime any more thoughts / clarification about part two would be appreciated as I'm not sure of the best approach to take here. Kind Regards Steve --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
Re: [PATCH] Added AUTH NTLM for SMTP
2011/8/8 Steve Holme wrote: First one, patch existing code with changes that you deem necessary. I guess this would involve the changes to curl.h, urldata.h and url.c for the new MAIL_AUTH and MAIL_SINGLE_AUTH options as well as the modifications in smtp.c to support these options. No. With this I meant changes to http_ntlm.c and http_ntlm.h that your patch seems to introduce, that I suppose you deem necessary for NTLM correctness or convinience and that can be performed on existing code without the need to moving it out of those two files. The smtp and mail stuff would belong to the third step. Second one, refactor existing code into whatever functions you believe are appropriate. It is important to not change code while refactoring it into functions. I would say this is quite difficult... Whilst most of the crypto functions moved from http_ntlm to curl_ntlm without any modification , the actual ntlm message functions were embedded in the contents of Curl_input_ntlm() and Curl_output_ntlm (). As such they had to be reworked quite a bit to turn them into the stand-alone functions: Curl_ntlm_create_type1_message(), Curl_ntlm_decode_type2_message() and Curl_ntlm_create_type3_message(). It is easy break this phase into two different patches, one the does the refactoring without actually adding nor changing functionality, and a second one that brings in the new stuff. Third one, introduce the NTLM auth for SMTP support. This would be the new functions that I added to the smtp module: smtp_auth_ntlm(), smtp_state_auth_ntlm_resp() and smtp_state_auth_ntlm_type2_resp() as well as the modifications to smtp_authenticate() and smtp_statemach_act () to support this. Plus the stuff in curl.h, urldata.h and url.c Do you want me to start producing a patch for part one Yes please taking in account what I've expressed here. -- -=[Yang]=- --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
RE: [PATCH] Added AUTH NTLM for SMTP
Hi Yang, On Mon, 8 Aug 2011, Yang Tse wrote: No. With this I meant changes to http_ntlm.c and http_ntlm.h that your patch seems to introduce, that I suppose you deem necessary for NTLM correctness or convinience and that can be performed on existing code without the need to moving it out of those two files. Do you have a specific example in mind as I am very confused now? The main changes in these files were: The removal of the ntlm / crypto code to curl_ntlm, the modification of Curl_output_ntlm() and Curl_input_ntlm() to call the functions in curl_ntlm (instead of having the message encode / decode code embedded in them), plus a small tidy up and refactor of Curl_ntlm_cleanup() to Curl_http_ntlm_cleanup() as I had written a Curl_ntlm_cleanup() in curl_ntlm. The smtp and mail stuff would belong to the third step. No problem. It is easy break this phase into two different patches, one the does the refactoring without actually adding nor changing functionality, and a second one that brings in the new stuff. Not really - As I mentioned above only one function in http_ntlm was refactored. All the other mods were related to code moving and then being fixed up to work as standalone functions whilst making sure the Windows SPPI code worked as well. Kind Regards Steve --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
Re: [PATCH] Added AUTH NTLM for SMTP
2011/8/8 Steve Holme wrote: Do you have a specific example in mind as I am very confused now? i.e... In http_ntlm.c 1) DEBUG_OUT macro definition. 2) renaming of print_flags() and print_hex(). 3) a couple of places where you appropiately introduce and use vars domlen, userlen, passwdlen. 4) your appropiate reformatting of code when calling s_pSecFn-AcquireCredentialsHandleA(). 5) your reformatting of if(status == SEC_I_COMPLETE_AND_CONTINUE || status == SEC_I_CONTINUE_NEEDED) does not need parenthesis on the 'if' branch. 6) Your appropiate reformatting of several function calls snprintf() LONGQUARTET() and fprint() among them. All of the above can merrily be done in a first patch of the first step, which won't disturb existing functionality. Remember that we are aiming at no reformatting allowed for the second step. -- -=[Yang]=- --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
RE: [PATCH] Added AUTH NTLM for SMTP
On Mon, 8 Aug 2011, Yang Tse wrote: i.e... In http_ntlm.c [..] All of the above can merrily be done in a first patch of the first step, which won't disturb existing functionality. You know my code better than I do - But then it is a good three weeks or so since I wrote most of this ;-) From an outsider's perspective, this seems quite long winded, is it always like this? The reason I ask is, I've got some more features I would like to add to the SMTP module as well as some fixes for bugs that I have noticed. Steve --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
Re: [PATCH] Added AUTH NTLM for SMTP
On Mon, Aug 08, 2011 at 08:07:12PM +0100, Steve Holme wrote: From an outsider's perspective, this seems quite long winded, is it always like this? The reason I ask is, I've got some more features I would like to add to the SMTP module as well as some fixes for bugs that I have noticed. It's very useful if problems are discovered in the future, as 'git bisect' can narrow the problem more precisely. It's also easier to review the code, since changes that are peripheral to the main NTLM changes don't get in the way as those more complex changes are reviewed. While doing it this way can be very useful and therefore encouraged since it's usually not too onerous (and 'git add -i' usually makes it pretty easy), it's negotiable if the submitter can claim undue hardship :-) Dan --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
Re: [PATCH] Added AUTH NTLM for SMTP
2011/8/8 Steve Holme wrote: From an outsider's perspective, this seems quite long winded, is it always like this? The reason I ask is, I've got some more features I would like to add to the SMTP module as well as some fixes for bugs that I have noticed. Nah, it happens only when patches get big and additional development has taken place recently in the same portions of code already in the repository, and I get involved in trying my best to not break existing code if I'm the one going to push it. You've been lucky, three out of three ;-) Other features won't need this amount of patch traceability, or you'll get someone else to review your patch ;-) But if they make substantial changes to existing code this is what should be done if someone has the time to do it. Dan has just expressed the same in a more technical reasoning. -- -=[Yang]=- --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
RE: [PATCH] Added AUTH NTLM for SMTP
On Mon, 8 Aug 2011, Steve Holme wrote: From an outsider's perspective, this seems quite long winded, is it always like this? Not really, most changes people contribute with are much smaller. I do want to stress that none of us ask you this to be annoying or hard to work with. We do this in the interest of getting the changes into the project in the best possible way so that we will be able to maintain them from then on. We very much appreciate your hard work and contributions! -- / daniel.haxx.se --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
RE: [PATCH] Added AUTH NTLM for SMTP
Hi all, On Mon, 8 Aug 2011, Daniel Stenberg wrote: Not really, most changes people contribute with are much smaller. I do want to stress that none of us ask you this to be annoying or hard to work with. We do this in the interest of getting the changes into the project in the best possible way so that we will be able to maintain them from then on. We very much appreciate your hard work and contributions! Thanks for clarification guys... I wasn't suggesting for one moment that anyone was being annoying or difficult. I have worked in a lot of different development environments, mostly as on a freelance basis, so I fully understand that every company / team / project has their different ways of working and this is no different. Not only does the code need reviewing, making sure it is fit for purpose and checking that it is defect free but it also need to be integrated with a constantly moving repository - especially some of which, the http_ntlm module in particular, has been changing quite a bit recently. I guess I am a little frustrated because Friday was a nightmare of a day, whilst I was struggling with GIT and trying to get an all in one patch put together (as requested) only to asked to then split the code up into three submissions - some of which I'm still not totally clear on :( Not only that but I am eager to see my changes pushed to the repository so I can crack on with the other features and fixes that I have it mind ;-) I will hopefully have some time tomorrow to prepare the first patch. Cheers Steve --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
Re: [PATCH] Added AUTH NTLM for SMTP
On 08/08/2011 02:39 PM, Steve Holme wrote: Hi all, On Mon, 8 Aug 2011, Daniel Stenberg wrote: Not really, most changes people contribute with are much smaller. I do want to stress that none of us ask you this to be annoying or hard to work with. We do this in the interest of getting the changes into the project in the best possible way so that we will be able to maintain them from then on. We very much appreciate your hard work and contributions! Thanks for clarification guys... I wasn't suggesting for one moment that anyone was being annoying or difficult. I have worked in a lot of different development environments, mostly as on a freelance basis, so I fully understand that every company / team / project has their different ways of working and this is no different. Not only does the code need reviewing, making sure it is fit for purpose and checking that it is defect free but it also need to be integrated with a constantly moving repository - especially some of which, the http_ntlm module in particular, has been changing quite a bit recently. I guess I am a little frustrated because Friday was a nightmare of a day, whilst I was struggling with GIT and trying to get an all in one patch put together (as requested) only to asked to then split the code up into three submissions - some of which I'm still not totally clear on :( Not only that but I am eager to see my changes pushed to the repository so I can crack on with the other features and fixes that I have it mind ;-) I will hopefully have some time tomorrow to prepare the first patch. In case you are working on Linux, you might find the 'stg' package useful. It can make creating and editing patch series easier Ben Cheers Steve --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html -- Ben Greear gree...@candelatech.com Candela Technologies Inc http://www.candelatech.com --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
RE: [PATCH] Added AUTH NTLM for SMTP (Part 2)
Hi Yang, On Thu, 4 Aug 2011, Yang Tse wrote: Could you provide a single consolidated patch rebased at tonight's git ? Taking in account that ... No problem. I have updated my branch with your suggestions and am able to provide a consolidated patch as requested. However, I am still getting to grips with Git (Well... TortoiseGit) and am struggling to with some of the terminology and understanding of how to catch-up from trunk / main / master. As you can problem tell, I am a long time TortoiseSVN, ClearCase and even have to admit it, SourceSafe user ;-) From what you mentioned, I thought I should be able to perform a rebase but I have committed my changes to my branch and as such am being warned: Current branch smtp_ntlm is up to date If you want to force the rebase even if the current branch is a descendent of the commit you are rebasing onto, please check Force Rebase checkbox Is this what I need to do, or is there another command I should be using to catch-up? Cheers Steve --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
Re: [PATCH] Added AUTH NTLM for SMTP (Part 2)
Hi On 5 August 2011 10:54, Steve Holme steve_ho...@hotmail.com wrote: Hi Yang, On Thu, 4 Aug 2011, Yang Tse wrote: Could you provide a single consolidated patch rebased at tonight's git ? Taking in account that ... No problem. I have updated my branch with your suggestions and am able to provide a consolidated patch as requested. However, I am still getting to grips with Git (Well... TortoiseGit) and am struggling to with some of the terminology and understanding of how to catch-up from trunk / main / master. As you can problem tell, I am a long time TortoiseSVN, ClearCase and even have to admit it, SourceSafe user ;-) From what you mentioned, I thought I should be able to perform a rebase but I have committed my changes to my branch and as such am being warned: Current branch smtp_ntlm is up to date If you want to force the rebase even if the current branch is a descendent of the commit you are rebasing onto, please check Force Rebase checkbox Is this what I need to do, or is there another command I should be using to catch-up? I've not used TortoiseGit, so I'm not sure exactly what you're doing. Basically, you should be doing a git fetch (or git fetch origin master) to update the origin/master remote branch to the latest version. Then you should rebase your current branch onto origin/master. -- Michael Wood esiot...@gmail.com --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
RE: [PATCH] Added AUTH NTLM for SMTP (Part 2)
On Fri, 5 Aug 2011, Michael Wood wrote: I've not used TortoiseGit, so I'm not sure exactly what you're doing. Basically, you should be doing a git fetch (or git fetch origin master) to update the origin/master remote branch to the latest version. Then you should rebase your current branch onto origin/master. Thank you Michael - that's got me going in the right direction now. I have performed the a TortoiseGit - Fetch, which invoked: git.exe fetch -v --progress origin ..and whilst rebase is looking more promising than earlier I am struggling a little with it at the moment as I had performed 3 commits and it seems to want to review conflicts at each stage :( Hopefully I will have something in a little while. Regards Steve --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
Re: [PATCH] Added AUTH NTLM for SMTP (Part 2)
On 5 August 2011 13:14, Steve Holme steve_ho...@hotmail.com wrote: On Fri, 5 Aug 2011, Michael Wood wrote: I've not used TortoiseGit, so I'm not sure exactly what you're doing. Basically, you should be doing a git fetch (or git fetch origin master) to update the origin/master remote branch to the latest version. Then you should rebase your current branch onto origin/master. Thank you Michael - that's got me going in the right direction now. I have performed the a TortoiseGit - Fetch, which invoked: git.exe fetch -v --progress origin ..and whilst rebase is looking more promising than earlier I am struggling a little with it at the moment as I had performed 3 commits and it seems to want to review conflicts at each stage :( Hopefully I will have something in a little while. Yes, merging the commits... I'm not sure what the best way is to do that. On the command line there's git rebase --interactive which allows you to squash multiple commits into one. Try git rebase --help in a console window and it will likely start up a web browser with the rebase documentation. -- Michael Wood esiot...@gmail.com --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
RE: [PATCH] Added AUTH NTLM for SMTP (Part 2)
On Fri, 5 Aug 2011, Michael Wood wrote: Yes, merging the commits... I'm not sure what the best way is to do that. I finally got there but unfortunately I had just finished the rebase before I received your email about the squash ;-) I then ran in issues of trying to produce a single patch file that contained all my changes rather than a patch file for each commit :( Anyway, I found a couple of solutions but ended up having to create another temporary branch, merge my changes from my smtp_ntlm branch into that branch (squashing them along the way) and then committing the merge. I will post the changes in a new email rather than to this thread so that ppl can find it easily. Steve --- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
