librelist archives

« back to archive

git_status_foreach

git_status_foreach

From:
Nacho
Date:
2011-08-19 @ 22:14
Hey guys,

GIT_EXTERN(int) git_status_foreach(git_repository *repo, int
(*callback)(const char *, unsigned int, void *), void *payload);

what's the point for making the callback return an int? I understand what it
has to be returned, but is there any case
where the user have to return something different from GIT_SUCCESS? I don't
see why the user would have to set an
error, at the end it is suppose that the paths are correct and they are
there.

Regards.

-- 
Ignacio Casal Quinteiro

Re: [libgit2] git_status_foreach

From:
Kelly Leahy
Date:
2011-08-20 @ 00:52
I don't know about this particular case but normally callbacks in
enumeration functions should always have a return value to indicate whether
they want to continue the iteration or stop.  Again, I'm not sure if that is
the case here, just what I've come to expect from apis.
On Aug 19, 2011 3:15 PM, "Nacho" <nacho.resa@gmail.com> wrote:
> Hey guys,
>
> GIT_EXTERN(int) git_status_foreach(git_repository *repo, int
> (*callback)(const char *, unsigned int, void *), void *payload);
>
> what's the point for making the callback return an int? I understand what
it
> has to be returned, but is there any case
> where the user have to return something different from GIT_SUCCESS? I
don't
> see why the user would have to set an
> error, at the end it is suppose that the paths are correct and they are
> there.
>
> Regards.
>
> --
> Ignacio Casal Quinteiro

Re: [libgit2] git_status_foreach

From:
Nacho
Date:
2011-08-20 @ 10:49
dunno, for this particular case I do not see the reason for proxying the
value. I can understand a
0 or 1 to indicate going on or stop, but why proxying an error value? will
the user have the need
to provide something else than a boolean here? I would even say that there
is no need for the user
to stop the iteration but maybe I am missing some use case here.

Regards.

On Sat, Aug 20, 2011 at 2:52 AM, Kelly Leahy <kelly.p.leahy@gmail.com>wrote:

> I don't know about this particular case but normally callbacks in
> enumeration functions should always have a return value to indicate whether
> they want to continue the iteration or stop.  Again, I'm not sure if that is
> the case here, just what I've come to expect from apis.
> On Aug 19, 2011 3:15 PM, "Nacho" <nacho.resa@gmail.com> wrote:
> > Hey guys,
> >
> > GIT_EXTERN(int) git_status_foreach(git_repository *repo, int
> > (*callback)(const char *, unsigned int, void *), void *payload);
> >
> > what's the point for making the callback return an int? I understand what
> it
> > has to be returned, but is there any case
> > where the user have to return something different from GIT_SUCCESS? I
> don't
> > see why the user would have to set an
> > error, at the end it is suppose that the paths are correct and they are
> > there.
> >
> > Regards.
> >
> > --
> > Ignacio Casal Quinteiro
>



-- 
Ignacio Casal Quinteiro

Re: [libgit2] git_status_foreach

From:
Kelly Leahy
Date:
2011-08-20 @ 18:43
I think I agree with the point that this doesn't seem to need to be a "Error
value".  I do, however, think that the caller of an enumeration function
should always be able to stop the iteration.  For instance, what if the
caller is searching the index for a file.  Should they have to handle
spinning through the entire index after the file if they already found what
they are looking for?  Another example, what if an error occurs on the
caller's code.  Presumably they'd like to be able to stop iteration so that
error doesn't keep occurring for each file in the index, right?

Granted, the caller can always use some sort of state to "turn off" their
per-iteration logic if they want to, but it seems like an inconvenient
workaround, not the preferred way of doing things.

I'm assuming that void* payload is a caller-defined pointer that can be used
for tracking caller state between iterations.  If it isn't, there should be
some way of passing a user-defined data structure through the iteration as
well, since that's the "preferred" way in APIs of allowing the caller to
track state between iterations.

Kelly

On Sat, Aug 20, 2011 at 3:49 AM, Nacho <nacho.resa@gmail.com> wrote:

> dunno, for this particular case I do not see the reason for proxying the
> value. I can understand a
> 0 or 1 to indicate going on or stop, but why proxying an error value? will
> the user have the need
> to provide something else than a boolean here? I would even say that there
> is no need for the user
> to stop the iteration but maybe I am missing some use case here.
>
> Regards.
>
> On Sat, Aug 20, 2011 at 2:52 AM, Kelly Leahy <kelly.p.leahy@gmail.com>wrote:
>
>> I don't know about this particular case but normally callbacks in
>> enumeration functions should always have a return value to indicate whether
>> they want to continue the iteration or stop.  Again, I'm not sure if that is
>> the case here, just what I've come to expect from apis.
>>  On Aug 19, 2011 3:15 PM, "Nacho" <nacho.resa@gmail.com> wrote:
>> > Hey guys,
>> >
>> > GIT_EXTERN(int) git_status_foreach(git_repository *repo, int
>> > (*callback)(const char *, unsigned int, void *), void *payload);
>> >
>> > what's the point for making the callback return an int? I understand
>> what it
>> > has to be returned, but is there any case
>> > where the user have to return something different from GIT_SUCCESS? I
>> don't
>> > see why the user would have to set an
>> > error, at the end it is suppose that the paths are correct and they are
>> > there.
>> >
>> > Regards.
>> >
>> > --
>> > Ignacio Casal Quinteiro
>>
>
>
>
> --
> Ignacio Casal Quinteiro
>

Re: [libgit2] git_status_foreach

From:
Nacho
Date:
2011-08-20 @ 18:51
Yeah, the playload is to be able to pass user data to the callback. Anyway I
am fine with the callback returning
a value to stop the iteration but I don't think this should allow to return
a specific error as pointed before. My point
here it to make my binding library make it return a gboolean or a gint.

Regards.

On Sat, Aug 20, 2011 at 8:43 PM, Kelly Leahy <kelly.p.leahy@gmail.com>wrote:

> I think I agree with the point that this doesn't seem to need to be a
> "Error value".  I do, however, think that the caller of an enumeration
> function should always be able to stop the iteration.  For instance, what if
> the caller is searching the index for a file.  Should they have to handle
> spinning through the entire index after the file if they already found what
> they are looking for?  Another example, what if an error occurs on the
> caller's code.  Presumably they'd like to be able to stop iteration so that
> error doesn't keep occurring for each file in the index, right?
>
> Granted, the caller can always use some sort of state to "turn off" their
> per-iteration logic if they want to, but it seems like an inconvenient
> workaround, not the preferred way of doing things.
>
> I'm assuming that void* payload is a caller-defined pointer that can be
> used for tracking caller state between iterations.  If it isn't, there
> should be some way of passing a user-defined data structure through the
> iteration as well, since that's the "preferred" way in APIs of allowing the
> caller to track state between iterations.
>
> Kelly
>
> On Sat, Aug 20, 2011 at 3:49 AM, Nacho <nacho.resa@gmail.com> wrote:
>
>> dunno, for this particular case I do not see the reason for proxying the
>> value. I can understand a
>> 0 or 1 to indicate going on or stop, but why proxying an error value? will
>> the user have the need
>> to provide something else than a boolean here? I would even say that there
>> is no need for the user
>> to stop the iteration but maybe I am missing some use case here.
>>
>> Regards.
>>
>> On Sat, Aug 20, 2011 at 2:52 AM, Kelly Leahy <kelly.p.leahy@gmail.com>wrote:
>>
>>> I don't know about this particular case but normally callbacks in
>>> enumeration functions should always have a return value to indicate whether
>>> they want to continue the iteration or stop.  Again, I'm not sure if that is
>>> the case here, just what I've come to expect from apis.
>>>  On Aug 19, 2011 3:15 PM, "Nacho" <nacho.resa@gmail.com> wrote:
>>> > Hey guys,
>>> >
>>> > GIT_EXTERN(int) git_status_foreach(git_repository *repo, int
>>> > (*callback)(const char *, unsigned int, void *), void *payload);
>>> >
>>> > what's the point for making the callback return an int? I understand
>>> what it
>>> > has to be returned, but is there any case
>>> > where the user have to return something different from GIT_SUCCESS? I
>>> don't
>>> > see why the user would have to set an
>>> > error, at the end it is suppose that the paths are correct and they are
>>> > there.
>>> >
>>> > Regards.
>>> >
>>> > --
>>> > Ignacio Casal Quinteiro
>>>
>>
>>
>>
>> --
>> Ignacio Casal Quinteiro
>>
>
>


-- 
Ignacio Casal Quinteiro

Re: [libgit2] git_status_foreach

From:
Kelly Leahy
Date:
2011-08-20 @ 19:11
I think you're right on that count.  I'm not familiar with the reasoning
behind choosing the error return for the function.  Perhaps that was the
signature of a built-in function that someone was expecting to pass to the
'foreach' function (though I think that's unlikely, due to the extra
'payload' argument being there).

I also might recommend renaming "payload" to something more meaningful, like
"callerState", "state", "userState", "userObject", "callerObject", etc.

Obviously windows does this poorly, most of the functions take an argument
called "lParam", so that's a bad example.  A cursory search for unix / linux
/ X11 functions that follow this pattern didn't turn up anything and I can't
remember any off the top of my head, so I guess there's not as much of a
pattern as I had thought.

Kelly

On Sat, Aug 20, 2011 at 11:51 AM, Nacho <nacho.resa@gmail.com> wrote:

> Yeah, the playload is to be able to pass user data to the callback. Anyway
> I am fine with the callback returning
> a value to stop the iteration but I don't think this should allow to return
> a specific error as pointed before. My point
> here it to make my binding library make it return a gboolean or a gint.
>
> Regards.
>
>
> On Sat, Aug 20, 2011 at 8:43 PM, Kelly Leahy <kelly.p.leahy@gmail.com>wrote:
>
>> I think I agree with the point that this doesn't seem to need to be a
>> "Error value".  I do, however, think that the caller of an enumeration
>> function should always be able to stop the iteration.  For instance, what if
>> the caller is searching the index for a file.  Should they have to handle
>> spinning through the entire index after the file if they already found what
>> they are looking for?  Another example, what if an error occurs on the
>> caller's code.  Presumably they'd like to be able to stop iteration so that
>> error doesn't keep occurring for each file in the index, right?
>>
>> Granted, the caller can always use some sort of state to "turn off" their
>> per-iteration logic if they want to, but it seems like an inconvenient
>> workaround, not the preferred way of doing things.
>>
>> I'm assuming that void* payload is a caller-defined pointer that can be
>> used for tracking caller state between iterations.  If it isn't, there
>> should be some way of passing a user-defined data structure through the
>> iteration as well, since that's the "preferred" way in APIs of allowing the
>> caller to track state between iterations.
>>
>> Kelly
>>
>> On Sat, Aug 20, 2011 at 3:49 AM, Nacho <nacho.resa@gmail.com> wrote:
>>
>>> dunno, for this particular case I do not see the reason for proxying the
>>> value. I can understand a
>>> 0 or 1 to indicate going on or stop, but why proxying an error value?
>>> will the user have the need
>>> to provide something else than a boolean here? I would even say that
>>> there is no need for the user
>>> to stop the iteration but maybe I am missing some use case here.
>>>
>>> Regards.
>>>
>>> On Sat, Aug 20, 2011 at 2:52 AM, Kelly Leahy <kelly.p.leahy@gmail.com>wrote:
>>>
>>>> I don't know about this particular case but normally callbacks in
>>>> enumeration functions should always have a return value to indicate whether
>>>> they want to continue the iteration or stop.  Again, I'm not sure if that is
>>>> the case here, just what I've come to expect from apis.
>>>>  On Aug 19, 2011 3:15 PM, "Nacho" <nacho.resa@gmail.com> wrote:
>>>> > Hey guys,
>>>> >
>>>> > GIT_EXTERN(int) git_status_foreach(git_repository *repo, int
>>>> > (*callback)(const char *, unsigned int, void *), void *payload);
>>>> >
>>>> > what's the point for making the callback return an int? I understand
>>>> what it
>>>> > has to be returned, but is there any case
>>>> > where the user have to return something different from GIT_SUCCESS? I
>>>> don't
>>>> > see why the user would have to set an
>>>> > error, at the end it is suppose that the paths are correct and they
>>>> are
>>>> > there.
>>>> >
>>>> > Regards.
>>>> >
>>>> > --
>>>> > Ignacio Casal Quinteiro
>>>>
>>>
>>>
>>>
>>> --
>>> Ignacio Casal Quinteiro
>>>
>>
>>
>
>
> --
> Ignacio Casal Quinteiro
>

Re: [libgit2] git_status_foreach

From:
Nacho
Date:
2011-08-20 @ 19:23
Just to point out, in glib we user user_data for the name of it. IMHO
playload is also not very discoverable.

Regards.

On Sat, Aug 20, 2011 at 9:11 PM, Kelly Leahy <kelly.p.leahy@gmail.com>wrote:

> I think you're right on that count.  I'm not familiar with the reasoning
> behind choosing the error return for the function.  Perhaps that was the
> signature of a built-in function that someone was expecting to pass to the
> 'foreach' function (though I think that's unlikely, due to the extra
> 'payload' argument being there).
>
> I also might recommend renaming "payload" to something more meaningful,
> like "callerState", "state", "userState", "userObject", "callerObject", etc.
>
> Obviously windows does this poorly, most of the functions take an argument
> called "lParam", so that's a bad example.  A cursory search for unix / linux
> / X11 functions that follow this pattern didn't turn up anything and I can't
> remember any off the top of my head, so I guess there's not as much of a
> pattern as I had thought.
>
> Kelly
>
> On Sat, Aug 20, 2011 at 11:51 AM, Nacho <nacho.resa@gmail.com> wrote:
>
>> Yeah, the playload is to be able to pass user data to the callback. Anyway
>> I am fine with the callback returning
>> a value to stop the iteration but I don't think this should allow to
>> return a specific error as pointed before. My point
>> here it to make my binding library make it return a gboolean or a gint.
>>
>> Regards.
>>
>>
>> On Sat, Aug 20, 2011 at 8:43 PM, Kelly Leahy <kelly.p.leahy@gmail.com>wrote:
>>
>>> I think I agree with the point that this doesn't seem to need to be a
>>> "Error value".  I do, however, think that the caller of an enumeration
>>> function should always be able to stop the iteration.  For instance, what if
>>> the caller is searching the index for a file.  Should they have to handle
>>> spinning through the entire index after the file if they already found what
>>> they are looking for?  Another example, what if an error occurs on the
>>> caller's code.  Presumably they'd like to be able to stop iteration so that
>>> error doesn't keep occurring for each file in the index, right?
>>>
>>> Granted, the caller can always use some sort of state to "turn off" their
>>> per-iteration logic if they want to, but it seems like an inconvenient
>>> workaround, not the preferred way of doing things.
>>>
>>> I'm assuming that void* payload is a caller-defined pointer that can be
>>> used for tracking caller state between iterations.  If it isn't, there
>>> should be some way of passing a user-defined data structure through the
>>> iteration as well, since that's the "preferred" way in APIs of allowing the
>>> caller to track state between iterations.
>>>
>>> Kelly
>>>
>>> On Sat, Aug 20, 2011 at 3:49 AM, Nacho <nacho.resa@gmail.com> wrote:
>>>
>>>> dunno, for this particular case I do not see the reason for proxying the
>>>> value. I can understand a
>>>> 0 or 1 to indicate going on or stop, but why proxying an error value?
>>>> will the user have the need
>>>> to provide something else than a boolean here? I would even say that
>>>> there is no need for the user
>>>> to stop the iteration but maybe I am missing some use case here.
>>>>
>>>> Regards.
>>>>
>>>> On Sat, Aug 20, 2011 at 2:52 AM, Kelly Leahy <kelly.p.leahy@gmail.com>wrote:
>>>>
>>>>> I don't know about this particular case but normally callbacks in
>>>>> enumeration functions should always have a return value to indicate whether
>>>>> they want to continue the iteration or stop.  Again, I'm not sure if that is
>>>>> the case here, just what I've come to expect from apis.
>>>>>  On Aug 19, 2011 3:15 PM, "Nacho" <nacho.resa@gmail.com> wrote:
>>>>> > Hey guys,
>>>>> >
>>>>> > GIT_EXTERN(int) git_status_foreach(git_repository *repo, int
>>>>> > (*callback)(const char *, unsigned int, void *), void *payload);
>>>>> >
>>>>> > what's the point for making the callback return an int? I understand
>>>>> what it
>>>>> > has to be returned, but is there any case
>>>>> > where the user have to return something different from GIT_SUCCESS? I
>>>>> don't
>>>>> > see why the user would have to set an
>>>>> > error, at the end it is suppose that the paths are correct and they
>>>>> are
>>>>> > there.
>>>>> >
>>>>> > Regards.
>>>>> >
>>>>> > --
>>>>> > Ignacio Casal Quinteiro
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Ignacio Casal Quinteiro
>>>>
>>>
>>>
>>
>>
>> --
>> Ignacio Casal Quinteiro
>>
>
>


-- 
Ignacio Casal Quinteiro

Re: [libgit2] git_status_foreach

From:
Kelly Leahy
Date:
2011-08-20 @ 19:28
sounds good to me... I was looking for a "unix" example, since I figured one
from Windows even if good probably wasn't going to "fit in".  I too think
that "payload" isn't discoverable (at least not to me, given the fact that I
didn't immediately know what it was)

Kelly

On Sat, Aug 20, 2011 at 12:23 PM, Nacho <nacho.resa@gmail.com> wrote:

> Just to point out, in glib we user user_data for the name of it. IMHO
> playload is also not very discoverable.
>
> Regards.
>
>
> On Sat, Aug 20, 2011 at 9:11 PM, Kelly Leahy <kelly.p.leahy@gmail.com>wrote:
>
>> I think you're right on that count.  I'm not familiar with the reasoning
>> behind choosing the error return for the function.  Perhaps that was the
>> signature of a built-in function that someone was expecting to pass to the
>> 'foreach' function (though I think that's unlikely, due to the extra
>> 'payload' argument being there).
>>
>> I also might recommend renaming "payload" to something more meaningful,
>> like "callerState", "state", "userState", "userObject", "callerObject", etc.
>>
>> Obviously windows does this poorly, most of the functions take an argument
>> called "lParam", so that's a bad example.  A cursory search for unix / linux
>> / X11 functions that follow this pattern didn't turn up anything and I can't
>> remember any off the top of my head, so I guess there's not as much of a
>> pattern as I had thought.
>>
>> Kelly
>>
>> On Sat, Aug 20, 2011 at 11:51 AM, Nacho <nacho.resa@gmail.com> wrote:
>>
>>> Yeah, the playload is to be able to pass user data to the callback.
>>> Anyway I am fine with the callback returning
>>> a value to stop the iteration but I don't think this should allow to
>>> return a specific error as pointed before. My point
>>> here it to make my binding library make it return a gboolean or a gint.
>>>
>>> Regards.
>>>
>>>
>>> On Sat, Aug 20, 2011 at 8:43 PM, Kelly Leahy <kelly.p.leahy@gmail.com>wrote:
>>>
>>>> I think I agree with the point that this doesn't seem to need to be a
>>>> "Error value".  I do, however, think that the caller of an enumeration
>>>> function should always be able to stop the iteration.  For instance, what if
>>>> the caller is searching the index for a file.  Should they have to handle
>>>> spinning through the entire index after the file if they already found what
>>>> they are looking for?  Another example, what if an error occurs on the
>>>> caller's code.  Presumably they'd like to be able to stop iteration so that
>>>> error doesn't keep occurring for each file in the index, right?
>>>>
>>>> Granted, the caller can always use some sort of state to "turn off"
>>>> their per-iteration logic if they want to, but it seems like an inconvenient
>>>> workaround, not the preferred way of doing things.
>>>>
>>>> I'm assuming that void* payload is a caller-defined pointer that can be
>>>> used for tracking caller state between iterations.  If it isn't, there
>>>> should be some way of passing a user-defined data structure through the
>>>> iteration as well, since that's the "preferred" way in APIs of allowing the
>>>> caller to track state between iterations.
>>>>
>>>> Kelly
>>>>
>>>> On Sat, Aug 20, 2011 at 3:49 AM, Nacho <nacho.resa@gmail.com> wrote:
>>>>
>>>>> dunno, for this particular case I do not see the reason for proxying
>>>>> the value. I can understand a
>>>>> 0 or 1 to indicate going on or stop, but why proxying an error value?
>>>>> will the user have the need
>>>>> to provide something else than a boolean here? I would even say that
>>>>> there is no need for the user
>>>>> to stop the iteration but maybe I am missing some use case here.
>>>>>
>>>>> Regards.
>>>>>
>>>>> On Sat, Aug 20, 2011 at 2:52 AM, Kelly Leahy <kelly.p.leahy@gmail.com>wrote:
>>>>>
>>>>>> I don't know about this particular case but normally callbacks in
>>>>>> enumeration functions should always have a return value to indicate whether
>>>>>> they want to continue the iteration or stop.  Again, I'm not sure 
if that is
>>>>>> the case here, just what I've come to expect from apis.
>>>>>>  On Aug 19, 2011 3:15 PM, "Nacho" <nacho.resa@gmail.com> wrote:
>>>>>> > Hey guys,
>>>>>> >
>>>>>> > GIT_EXTERN(int) git_status_foreach(git_repository *repo, int
>>>>>> > (*callback)(const char *, unsigned int, void *), void *payload);
>>>>>> >
>>>>>> > what's the point for making the callback return an int? I understand
>>>>>> what it
>>>>>> > has to be returned, but is there any case
>>>>>> > where the user have to return something different from GIT_SUCCESS?
>>>>>> I don't
>>>>>> > see why the user would have to set an
>>>>>> > error, at the end it is suppose that the paths are correct and they
>>>>>> are
>>>>>> > there.
>>>>>> >
>>>>>> > Regards.
>>>>>> >
>>>>>> > --
>>>>>> > Ignacio Casal Quinteiro
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Ignacio Casal Quinteiro
>>>>>
>>>>
>>>>
>>>
>>>
>>> --
>>> Ignacio Casal Quinteiro
>>>
>>
>>
>
>
> --
> Ignacio Casal Quinteiro
>