[PATCH 51 of 55 RFC c-hglib:level1] hg_tip: creating a high level function for mercurial tip command

[Giovanni Gherdovich]

Hello Idan and Iulian,

2013/9/15 Idan Kamara <idankk86 at gmail.com>:
> On Sat, Sep 14, 2013 at 3:34 PM, Iulian Stana <julian.stana at gmail.com>
> wrote:
>>
>>
>>>> + *
>>>> + * \param handle The handle of the connection, wherewith I want to
>>>> communicate
>>>> + * \param callback A function that will handle error data.
>>>> + *                 A NULL pointer will ignore error data.
>>>
>>>
>>> What meaningful thing can a user do in this callback?
>>>
>>
>>
>> What ever he wants to do with error data, print it do the stderr, store in
>> some place
>> or I don't know parse this data in his own way is his decision.
>
>
> Parsing error data is a bit of a problem in Mercurial, since its output
> isn't guaranteed to be stable between releases.
> But more importantly, I don't think there's anything meaningful to do when
> 'hg tip' fails..
>
> You already have errno to check for command errors, does it also expose a
> generic 'command failed' error code?
> If so, you could use that and store the error string somewhere, and have
> strerror (or similar), return it.

They idea behind a callback to handle error messages is to let the user
decide if (1) print the err msg somewhere, like dialogue box or stderr or
(2) ignore.
Below I paste the IRC transcripts of the discussion we had with Matt
at the time.


>>>> + * \param argument The option list. Will contain all option that you
>>>> wish.
>>>
>>>
>>> If you let the user pass in more arguments, it has the potential of:
>>>
>>> 1) breaking your changeset parser (user passed --template)
>>> 2) leaving unparsed data after you successfully parsed the changeset (not
>>> sure how you deal with that)
>>>
>>
>> The last --template will overwrite all preview templates probably I must
>> tell in
>> documentation that is not allowed to use other template...
>>
>>
>> hg tip --template '{rev}' --template '{node}' --template 'Template - {rev}
>> - {node}\n'
>>
>>> I'm not sure you really want that.
>>
>>
>> I really don't want to leave unparse data.
>
>
> Right, so I'm not sure if you agree there's an issue here..

I agree that there is an issue here.
We don't just proxy commands trasparently to commandserver,
we inject a template that must be respected.

What does python-hglib and JavaHG do about that?

>
> My comment was directed at making you question whether you really want to
> allow
> the user to pass arbitrary arguments to this function (and every other 'top
> level' function
> such as this).

maybe "arbitrary options but not --template"...
I didn't think much of the implication of this.
What you say Iulian?

>>>> + * \retval cset The command return succesful, with a parse data.
>>>
>>>
>>> hg tip always returns a single changeset, wouldn't it be nicer if you
>>> received a pointer
>>> to hg_cset_entry and wrote the result there, rather than allocating one
>>> inside?
>>
>>
>> Probably, I don't know:) I thought that it would be nice to return a
>> cset_entry and
>> to give user less work to do :) But this thing can be easy changed.
>
>
> The current approach strikes me as more work for the user, because then he's
> forced
> to free() the malloc'ed cset. Where as if you let him pass a pointer, he
> could
> either pass a malloc'ed cset, or one off the stack, but the choice is his.

I second Idan here.
I don't know how common in the C world this is, but I personally find
disturbing when I have to free() things that I didn't malloc()ed myself.
It looks to me that the door is wide open to memory leaks because I
can forget something.

Cheers,
GGhh


-- -- >8 -- -- cut here -- -- >8 -- -- >8 -- -- cut here -- -- >8 -- -- >8
10:13 PM <mpm> What's your strategy for handling error messages?
10:15 PM <iulians> I save the output/error/debug data in some
hg_handle structure pointers.
         If the user what to see this data he can get it)
10:25 PM <mpm> The Python hglib typically uses callbacks for errors,
         which I think is probably a good fit for C.
10:27 PM <mpm> It lets the user say "I want to buffer my errors myself" or
         "I just want to set an error flag" or "I don't care about
anything but the exit code".
10:38 PM <iulians> mpm, How could this callback look? You enumerate 3
possibilities,
         les't say that the last one will pass to nowhere the data...
10:39 PM <mpm> Let's imagine the callback prototype looks like:
10:40 PM <mpm> int callback(const char *msg, size_t len);
10:40 PM <mpm> If the user wants to ignore the error message,
         she simply provides a function that returns right away.
10:41 PM <mpm> The library can even provide a pre-written version of
         this callback for convenience.
10:41 PM <mpm> Or treat a NULL callback pointer as 'ignore'.
10:43 PM <mpm> (Another more Pythonesque policy would be to treat a
NULL callback as 'fail',
         but C is used to ignoring things.)
10:44 PM <iulians> ok, let's say that NULL will be treat like 'ignore data'
10:48 PM <iulians> But how the callback function will look-like?
         What data will be pass to msg pointer?
10:52 PM <ggherdov> iulians: well, the message you get as output from
the cmd server I guess,
         carefully cooked in a single contiguous char*
10:53 PM <ggherdov> (since it might come over several 'e' writes,
         but you're polite and massage it a bit before passing to the fun ptr )
10:56 PM — ggherdov but actually the user would like to get
         that *msg content back at some point... uhm...
10:59 PM <ggherdov> ah no ! The user doesn't need it back !
         she just want a side effect to happen, based on that *msg...
         like printing it on screen or something
11:01 PM <ggherdov> iulians, does this make sense ?
11:03 PM <iulians> Not quite....
11:03 PM <ggherdov> What I mean is: do you have any operation that
         desperately need the content of *msg ?
         well, you put that operation inside the callback.
11:04 PM <mpm> "carefully cooked" <- No, don't cook it at all.
         When you get an 'e' packet, hand it off to the callback.
         Then go about your regular business.
11:05 PM <mpm> If an error message is split across multiple packets,
not hglib's problem.
11:05 PM <ggherdov> iulians: your only rule: *msg cannot go out of the
scope of the callback
         (said function only returns a int, status code).
         "Something" need *msg ? "something" goes into the callback
11:06 PM <ggherdov> ah mpm ok. iulians, take note !
11:06 PM <iulians> So everything that will be read from cmdserver
         will be pass immediately to the callback function
11:06 PM <ggherdov> iulians: error messages
11:07 PM <ggherdov> not "everything". your 'o' content needs to be
         parsed into structured data to allow programmatic manipulation
11:07 PM <mpm> Thing is, clients are probably not AIs so they'll
probably only do things
         like "print message immediately on stderr" or "show in a dialog box"
-- -- >8 -- -- cut here -- -- >8 -- -- >8 -- -- cut here -- -- >8 -- -- >8