On Thu, 7 Aug 2014, Deepak Singh wrote:

> I have provided the patch sometime back, it is still in pending state.
>
> It would be great if you can review and provide your inputs on the solution
> provided.
>
> Once this patch is accepted I have a plan to provided few test cases for the
> same problem.

Thanks, this version seems to do the right thing for all of the command line
versions I threw at it. Nice! I do have some nits and questions still though:

> + static struct getout *_url = NULL;

This variable being static scares me, and there's no comments at all that
explains how it works or why the static. We already before handle options on a
per-URL basis so it is a bit strange to me that a static is now necessary for
this.

> else
> /* there was no free node, create one! */
> url = new_getout(config);
> + _url = url;

This indent level is wrong here, but does it imply you meant to have it as
part of the preceeding block?

I also believe I mentioned this before, but you have no documentation and no
tests for this new feature which makes me reluctant to accept it.

It still has an obvious race condition problem. Run 100 simultaneous command
lines with --no-clobber and I'm sure you will get a fair share of files still
ending up clobbered - to future users dismay and anger. I think you should
move the file-already-exists check into the tool_write_cb() callback so that
you can figure out the new name AND CREATE the new file in one atomic
operation.

And lastly, have you considered if perhaps --no-clobber would be more useful
as a flag that would affect all -o and -O options at once instead of once per
URL? I know I spoke to the idea of having it once per URL but that was
probably mostly because you started it semi-working that way. I'm just now
thinking that perhaps it is unlikely that users would want a mix of clobber
use in a single command line. Or what do you think?

-- 
  / daniel.haxx.se
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-users
FAQ:        http://curl.haxx.se/docs/faq.html
Etiquette:  http://curl.haxx.se/mail/etiquette.html