Re: [PATCH/RFC/GSOC] make git-pull a builtin

[Paul Tan <pyokagan@gmail.com>]

Hi,

Thanks for your suggestions, I agree with most of them :).

On Wed, Mar 18, 2015 at 5:00 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
>> +static int parse_opt_rebase(const struct option *opt, const char
>> *arg, int unset)
>> +{
>> +     if (arg)
>> +             *(int *)opt->value = parse_rebase(arg);
>> +     else
>> +             *(int *)opt->value = unset ? REBASE_FALSE : REBASE_TRUE;
>> +     return (*(int *)opt->value) >= 0 ? 0 : -1;
>> +}
>
> In this function (and also in other places below), there is this pattern that a `struct option` pointer is passed to the function, but then only `*(int *)opt->value` is written to. Therefore, I would suggest to change the signature of the function and pass `(int *)opt->value` as function parameter.

It's used as a callback for the argument parser though, so the
callback signature is required.

>
>> +static int has_unstaged_changes(void)
>
> Yeah, this function, as well as the ones below it, look as if they are so common that they *should* be already somewhere in libgit.a. But I did not find them, either...
>
> Of course it *would* be nice to identify places where essentially the same code is needed, and refactor accordingly. But I think that is outside the scope of this project.

Actually, I think that identifying the places where code can be
trivially shared (without requiring major refactoring) should be part
of the project, otherwise lots of code may be duplicated and cause
code bloat. The obvious one would be fork_point() in this patch, which
is copied from the merge-base builtin and not accessible because it
has static linkage. The project should, at the very least, allow the
function to be shared between git-pull and git-merge-base, as well as
to modify the function so that it can fail without die()-ing.

Regards,
Paul