Re: [PATCH] reset: Libify reset_index_file and print_new_head_line

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Jun 11, 2011, 06:07 UTC
Message-ID: <BANLkTi=UbGer-zbbviM_DM5JwzSE1LXy2A@mail.gmail.com>
In-Reply-To: <20110610103532.GA32119@elie>

Hi Jonathan,

Jonathan Nieder writes:

Show 6 quoted lines

> It should be easier to review this one with a caller in mind to give
> an indication about whether the API is right.  More generally, the
> excitement of patches exposing new library code comes when you can see
> the header with a pleasant API, ideally followed by an example or two
> to see how it plays out in practice.  See "git log --grep=API junio/pu"
> for a few recent examples of this.

Yes, I know. Unfortunately, I don't have those callers ready -- most of that work is in the form of RFC patches in the sequencer series. I wanted to post this in the same spirit as Junio's rerere series (which I initiated, but messed up quite badly).

Show 14 quoted lines

>> --- /dev/null
>> +++ b/reset.c
>> @@ -0,0 +1,81 @@
>> +#include "builtin.h"
>> +#include "refs.h"
>> +#include "diff.h"
>> +#include "diffcore.h"
>> +#include "tree.h"
>> +#include "branch.h"
>> +#include "tree.h"
>> +#include "unpack-trees.h"
>> +#include "reset.h"
>
> Are these all needed?  E.g., where is diffcore used?

Pruned, thanks.

Show 13 quoted lines

>> --- /dev/null
>> +++ b/reset.h
>> @@ -0,0 +1,11 @@
>> +#ifndef RESET_H
>> +#define RESET_H
>> +
>> +#include "commit.h"
>
> We can avoid unnecessarily rebuilding (if COMPUTE_HEADER_DEPENDENCIES
> is set) for callers that do not use commit.h when commit.h changes by
> just declaring
>
>        struct commit;

Thanks for the tip.

Show 8 quoted lines

>> +enum reset_type { MIXED, SOFT, HARD, MERGE, KEEP, NONE };
>
> The names of these identifiers (especially KEEP, MERGE, and NONE)
> would have a good chance to colliding with other uses after being
> exposed into a global namespace like this, no?
>
> Maybe RESET_MIXED, RESET_SOFT, etc could avoid this while still not
> being too verbose.

I should have got this right the first time. Anyway, fixed now.

Show 10 quoted lines

>> +int reset_index_file(const unsigned char *sha1, int reset_type, int quiet);
>> +void print_new_head_line(struct commit *commit);
>
> Warning: trivia ahead.
>
> For some odd reason I still remember a patch from Stefan Beyer:
> http://thread.gmane.org/gmane.comp.version-control.git/92023
> which was probably too invasive to apply but made the whole codebase
> oddly more pleasant when I tried it.  It simply added "extern" at the
> start of function prototypes in header files that lacked it.

Good tip. We can atleast use "extern" for the new functions that we write to avoid massive code churns.

Your review has been very helpful, as always. Thanks.

-- Ram