FW: 0.82 freebcp: -h option seems to be broken

Said Constantin Vasilyev:
> > freebcp tempdb.dbo.foo in /tmp/foo.bcp -S MSSQLSRV -c -h
> CHECK_CONSTRAINTS -U xxx -P yyy
> Msg 20076, Level 7
> bcp_init must be called before any other bcp routines
> 
> db-lib: Unable to set hint "CHECK_CONSTRAINTS"
> 
> I looked into the code and it appears like freebcp is trying to call
> bcp_options before bcp_init.
> 
> While being there, I also checked bcp_options function and found that
> list of valid
> bcp hints is incomplete (FIRE_TRIGGERS hint is missing).

Oh, crumb.  Actually, the -h option in freebcp is just broken.  

True, bcp_options() is called before bcp_init().  But that's technically
OK, or was, because bcp_options() just records the hint for later use.
The hint text is appended to the INSERT BULK statement generated by the
user calling bcp_init().  

The error you stumbled on is caused by parameter checking code added
later to bcp_options.  It's been there for a year at least and you're
the first to mention it.  :-/

freebcp simply grabs whatever is passed to the -h option and sends it
verbatim to bcp_options.  That was my original idea: let the user format
the hint text and let the server accept/reject it.  That was bcp.c
revision 1.45.  All it did was make sure the option text started with
something that looked like a hint.  Nothing checked that the hint text
was completely correct.  It could say "ROW_PER_BATCH IS A DUMB IDEA;
drop sysobjects" and that's what would go to the server.  

ISTR an objection somewhere along the line that we shouldn't pass
user-provided text unvetted to the server, and the code was changed to
pass the internal static validation string instead of the user-provided
one.  Consequently if you pass "ROWS_PER_BATCH = 100", only
"ROWS_PER_BATCH" goes to the server.  Which won't work. 

(Honestly, don't see a problem passing the text.  The freebcp user isn't
malicious, and a malformed INSERT BULK statement will likely result only
in the server dropping the connection, hardly a disaster.)

So, as of today:

1.  No hints work because they're stopped by the parameter checker. 
2.  If they were passed, only non-valued options would work because the
text is truncated before the "=" sign.  
3.  The option list is incomplete, as you said. 

And there's more...

The hint syntax is a mini-language of its own.  Consider parsing "ORDER
(column [ASC | DESC] [,...n])" and btw the "," is also used to separate
successive hints.  That's a job for regex, but the last time I pulled in
regex, Frediano rewrote the code to remove the dependency.  I don't want
to see a whole mini-parser for this written with strtok(3).  I'd rather
see either "no hints without HAVE_REGEX" or "raw hints without
HAVE_REGEX".  

And the bcp_options interface should be changed.  Sybase defines one
measly option, BCPLABELED, that we don't even support.  We should change
the signature to:

bcp_options( DBPROCESS *, bcp_option_t, char* value, int);

where bcp_option_t is an enumerated type representing the hints, and the
value is a good, old-fashioned C string.  

My proposal:

1.  Add regex to freebcp. (This might have other benefits, e.g. support
for CSV files.) 
2.  freebcp parses the hints and passes an enum-value pair to
bcp_options. 
3.  Make sure bcp_init is called first.  
4.  Support multiple hints.  tds_bcpinfo::hint becomes char**.  

That should do it.  Thanks for the problem report.  I'm so glad to have
something to work on, now that version-autodetection hasn't been
started.  Feel free to jump in any time.  ;-)

--jkl