[PATCH 00/28] clean-ups of static functions and returns

[PATCH 00/28] clean-ups of static functions and returns

[David Rientjes]

This patch series cleans up a number of static function returns that are either 
meaningless or could be more efficiently written.

		David

Re: [PATCH 00/28] clean-ups of static functions and returns

[Jakub Narebski]

David Rientjes wrote:

> This patch series cleans up a number of static function returns that are either 
> meaningless or could be more efficiently written.

Could you please make description of patch series email to be parent
(ancestor) of all patches emails, i.e. for patches either to be 
chain-replied to introduction email, or all be replies to introduction
email.

It makes for easier reading/viewing/applying/ignoring the series.

Thanks in advance
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH 00/28] clean-ups of static functions and returns

[David Rientjes]

On Mon, 14 Aug 2006, Jakub Narebski wrote:
> Could you please make description of patch series email to be parent
> (ancestor) of all patches emails, i.e. for patches either to be 
> chain-replied to introduction email, or all be replies to introduction
> email.
> 
> It makes for easier reading/viewing/applying/ignoring the series.
> 

Sure, but you might also want to include this request explicitly in 
Documentation/SubmittingPatches.

		David

Re: [PATCH 00/28] clean-ups of static functions and returns

[Johannes Schindelin]

Hi,

On Mon, 14 Aug 2006, David Rientjes wrote:

> On Mon, 14 Aug 2006, Jakub Narebski wrote:
> > Could you please make description of patch series email to be parent
> > (ancestor) of all patches emails, i.e. for patches either to be 
> > chain-replied to introduction email, or all be replies to introduction
> > email.
> > 
> > It makes for easier reading/viewing/applying/ignoring the series.
> > 
> 
> Sure, but you might also want to include this request explicitly in 
> Documentation/SubmittingPatches.

Well... there are people (like yours truly), who still hack there mails in 
pine, and who do not use git-send-email (I had too many problems with 
Perl, and git-send-email _is_ in Perl)... For me, it would be an undue 
burden.

Ciao,
Dscho

Re: [PATCH 00/28] clean-ups of static functions and returns

[Jakub Narebski]

Johannes Schindelin wrote:

> On Mon, 14 Aug 2006, David Rientjes wrote:
> 
>> On Mon, 14 Aug 2006, Jakub Narebski wrote:
>> > Could you please make description of patch series email to be parent
>> > (ancestor) of all patches emails, i.e. for patches either to be 
>> > chain-replied to introduction email, or all be replies to introduction
>> > email.
>> > 
>> > It makes for easier reading/viewing/applying/ignoring the series.
>> > 
>> 
>> Sure, but you might also want to include this request explicitly in 
>> Documentation/SubmittingPatches.
> 
> Well... there are people (like yours truly), who still hack there mails in 
> pine, and who do not use git-send-email (I had too many problems with 
> Perl, and git-send-email _is_ in Perl)... For me, it would be an undue 
> burden.

I also not always use git-send-email (because of the box connection to the
net); mu latest series of patches was send using KMail; still it is not
that hard to send patches as reply to introductory letter, or
chain-replied; just reply-all (without quoting) from your "sent" folder.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH 00/28] clean-ups of static functions and returns

[Junio C Hamano]

David Rientjes <rientjes@google.com> writes:

> This patch series cleans up a number of static function
> returns that are either meaningless or could be more
> efficiently written.

Interesting.  Did you use some automated tool to spot them?

I see two categories of changes in your series.

 * Making stricter error checking in the future harder.  There
   are three classes, but the lines between them are fuzzy.

        [PATCH 04/28] builtin-diff.c cleanup
        [PATCH 06/28] make cmd_log_walk void
        [PATCH 07/28] builtin-mailinfo.c cleanup
        [PATCH 09/28] makes prune_dir void
        [PATCH 11/28] makes append_ref and show_indepedent void
        [PATCH 12/28] makes generate_tar void
        [PATCH 13/28] builtin-unpack-objects.c cleanup
        [PATCH 14/28] make do_reupdate void
        [PATCH 16/28] daemon.c cleanup
        [PATCH 17/28] makes diff_cache void
        [PATCH 19/28] makes finish_pack void
        [PATCH 20/28] makes fetch_pack void
        [PATCH 23/28] makes peek_remote void

   The callers of the first group check their return values, so
   we could make error checking of these functions stricter in
   the future without affecting the rest of the code.  The ones
   that currently die() (or usage()) could be made into more
   libified form to return error codes.

   So I do not think it is worth doing these.
   
        [PATCH 03/28] makes checkout_all void
        [PATCH 15/28] makes sha1flush void
        [PATCH 21/28] makes fsck_dir void
        [PATCH 25/28] makes pack_objects void
        [PATCH 27/28] makes track_tree_refs void
        [PATCH 28/28] makes upload_pack void

   The callers of the second group do not check their return
   values, so making them void for now is fine, but if we wanted
   to make error checking of these functions stricter in the
   future, we would need to change them back and update the
   callers.

        [PATCH 02/28] makes pprint_tag void
	[PATCH 26/28] makes show_entry void

   These, strictly speaking, fall in the second category, but I
   do not think of a reasonable graceful error return case from
   them.  show_entry() dies when it cannot continue upon
   encountering a corrupt tree, which I think is reasonable.  So
   I think the change to make these void makes sense.

   I said the lines between these three categories are fuzzy.
   If we look at the functions more closely, I am reasonably
   sure that we would find some from the first and the second
   categories do not have a reasonable graceful error return
   case from them, in which case making them void would become
   reasonable like you did.

 * Style and readability.

        [PATCH 01/28] blame.c return cleanup
        [PATCH 05/28] builtin-grep.c cleanup
        [PATCH 08/28] remove conditional return
        [PATCH 10/28] builtin-push.c cleanup
        [PATCH 15/28] makes sha1flush void
        [PATCH 18/28] diff.c cleanup
        [PATCH 22/28] http-push.c cleanup
        [PATCH 24/28] read-cache.c cleanup

   Most of them use the well established idiom, "return
   !!(something)", and I think they are fine (15/28 do not even
   need !! -- the function already returns 0 or 1).

   I personally feel the original is more readable for 08/28.

   Of course, this distinction is subjective.

Re: [PATCH 00/28] clean-ups of static functions and returns

[David Rientjes]

On Mon, 14 Aug 2006, Junio C Hamano wrote:

> Interesting.  Did you use some automated tool to spot them?
> 

No, these changes are from my own personal tree that optimizes everything for 
speed since I am working with terabytes of data.  I only submitted changes that 
I thought would be beneficial for your project as well.

>  * Making stricter error checking in the future harder.  There
>    are three classes, but the lines between them are fuzzy.
> 
>         [PATCH 04/28] builtin-diff.c cleanup
>         [PATCH 06/28] make cmd_log_walk void
>         [PATCH 07/28] builtin-mailinfo.c cleanup
>         [PATCH 09/28] makes prune_dir void
>         [PATCH 11/28] makes append_ref and show_indepedent void
>         [PATCH 12/28] makes generate_tar void
>         [PATCH 13/28] builtin-unpack-objects.c cleanup
>         [PATCH 14/28] make do_reupdate void
>         [PATCH 16/28] daemon.c cleanup
>         [PATCH 17/28] makes diff_cache void
>         [PATCH 19/28] makes finish_pack void
>         [PATCH 20/28] makes fetch_pack void
>         [PATCH 23/28] makes peek_remote void
> 
>    The callers of the first group check their return values, so
>    we could make error checking of these functions stricter in
>    the future without affecting the rest of the code.  The ones
>    that currently die() (or usage()) could be made into more
>    libified form to return error codes.
> 
>    So I do not think it is worth doing these.
>    

I disagree.  Having static functions return ints that are the same for _every_ 
code path and are checked against upon return is never good style.  It implies 
that error checking is already done and the return value is of importance.  It 
also suggests you can program against a specific return value with expected 
results which are, in fact, true for any return.  Additionally, changes in the 
future will be easier, in my opinion, because a void -> int change is very 
simple and existant calls need not be changed (their return values are 
discarded) so that the implementer can make the checks where necessary.

I do not understand your point about making error checking of the functions 
stricter without affecting the rest of the code when int returns are discarded 
if not assigned upon return.  It would certainly be bad style to declare new 
error codes with a specific meaning without checking all the affected code and 
we certainly cannot predict this behavior now.

		David

Re: [PATCH 00/28] clean-ups of static functions and returns

[Junio C Hamano]

David Rientjes <rientjes@google.com> writes:

> On Mon, 14 Aug 2006, Junio C Hamano wrote:
>
>> Interesting.  Did you use some automated tool to spot them?
>> 
>
> No, these changes are from my own personal tree that optimizes everything for 
> speed since I am working with terabytes of data.  I only submitted changes that 
> I thought would be beneficial for your project as well.
>
>>  * Making stricter error checking in the future harder.  There
>>    are three classes, but the lines between them are fuzzy.
>> 
>>         [PATCH 04/28] builtin-diff.c cleanup
>>...
>>         [PATCH 23/28] makes peek_remote void
>> 
>>    The callers of the first group check their return values, so
>>    we could make error checking of these functions stricter in
>>    the future without affecting the rest of the code.  The ones
>>    that currently die() (or usage()) could be made into more
>>    libified form to return error codes.
>> 
>>    So I do not think it is worth doing these.
>
> I disagree.  Having static functions return ints that are the
> same for _every_ code path and are checked against upon return
> is never good style.  It implies that error checking is
> already done and the return value is of importance.

I would agree with your above statement about the second group,
but not the ones listed in the first group, whose callers are
prepared to receive error returns.  It just happens that these
callees do not currently detect errors, but some of them
certainly could be improved to return errors, instead of just
calling die().

Re: [PATCH 00/28] clean-ups of static functions and returns

[trajce nedev]

So you're just going to continue passing numbers around in the hope that 
someday they'll be used for something?

Trajce Nedev

>From: Junio C Hamano <junkio@cox.net>
>To: David Rientjes <rientjes@google.com>
>CC: git@vger.kernel.org
>Subject: Re: [PATCH 00/28] clean-ups of static functions and returns
>Date: Mon, 14 Aug 2006 18:04:24 -0700
>
>David Rientjes <rientjes@google.com> writes:
>
> > On Mon, 14 Aug 2006, Junio C Hamano wrote:
> >
> >> Interesting.  Did you use some automated tool to spot them?
> >>
> >
> > No, these changes are from my own personal tree that optimizes 
>everything for
> > speed since I am working with terabytes of data.  I only submitted 
>changes that
> > I thought would be beneficial for your project as well.
> >
> >>  * Making stricter error checking in the future harder.  There
> >>    are three classes, but the lines between them are fuzzy.
> >>
> >>         [PATCH 04/28] builtin-diff.c cleanup
> >>...
> >>         [PATCH 23/28] makes peek_remote void
> >>
> >>    The callers of the first group check their return values, so
> >>    we could make error checking of these functions stricter in
> >>    the future without affecting the rest of the code.  The ones
> >>    that currently die() (or usage()) could be made into more
> >>    libified form to return error codes.
> >>
> >>    So I do not think it is worth doing these.
> >
> > I disagree.  Having static functions return ints that are the
> > same for _every_ code path and are checked against upon return
> > is never good style.  It implies that error checking is
> > already done and the return value is of importance.
>
>I would agree with your above statement about the second group,
>but not the ones listed in the first group, whose callers are
>prepared to receive error returns.  It just happens that these
>callees do not currently detect errors, but some of them
>certainly could be improved to return errors, instead of just
>calling die().
>
>-
>To unsubscribe from this list: send the line "unsubscribe git" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

_________________________________________________________________
Express yourself instantly with MSN Messenger! Download today - it's FREE! 
http://messenger.msn.click-url.com/go/onm00200471ave/direct/01/

Re: [PATCH 00/28] clean-ups of static functions and returns

[Junio C Hamano]

"trajce nedev" <trajcenedev@hotmail.com> writes:

> So you're just going to continue passing numbers around in the hope
> that someday they'll be used for something?
>
> Trajce Nedev

It is more like "fearing someday we might modify the callees and
they may start returning error codes".

Admittedly, "someday" has a tendency not to come, but making the
callees void and changing the callers who are already checking
the error codes to ignore what are returned from them somehow
feel backwards and pushing that someday further into the future.

Either patches to implement the error checking in the callees,
or arguments that particular callees the series touch do not
have reasonably graceful error return path are welcome (the
former advocating against and the latter advocating for the
application of the patch).