Re: [PATCH/WIP v3 03/31] am: implement skeletal builtin am

[Paul Tan <pyokagan@gmail.com>]

On Fri, Jun 19, 2015 at 4:26 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Paul Tan <pyokagan@gmail.com> writes:
>> @@ -0,0 +1,28 @@
>> +/*
>> + * Builtin "git am"
>> + *
>> + * Based on git-am.sh by Junio C Hamano.
>> + */
>> +#include "cache.h"
>> +#include "builtin.h"
>> +#include "exec_cmd.h"
>> +
>> +int cmd_am(int argc, const char **argv, const char *prefix)
>> +{
>> +     /*
>> +      * FIXME: Once all the features of git-am.sh have been re-implemented
>> +      * in builtin/am.c, this preamble can be removed.
>> +      */
>
> It's not broken, so "FIXME" is not quite appropriate (and that is
> why I sent you "NEEDSWORK").

OK.

> Also mention that the entry in the
> commands[] array needs "RUN_SETUP | NEED_WORK_TREE" added, I think.

OK.

>> +     if (!getenv("_GIT_USE_BUILTIN_AM")) {
>> +             const char *path = mkpath("%s/git-am", git_exec_path());
>> +
>> +             if (sane_execvp(path, (char **)argv) < 0)
>> +                     die_errno("could not exec %s", path);
>> +     } else {
>> +             prefix = setup_git_directory();
>> +             trace_repo_setup(prefix);
>> +             setup_work_tree();
>> +     }
>> +
>> +     return 0;
>> +}
>> diff --git a/git.c b/git.c
>> index e7a7713..a671535 100644
>> --- a/git.c
>> +++ b/git.c
>> @@ -370,6 +370,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
>>
>>  static struct cmd_struct commands[] = {
>>       { "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
>> +     { "am", cmd_am, NO_SETUP },
>
> NO_SETUP is for things like init and clone that start without a
> repository and then work in the one that they create.  I think
> imitating "archive" or "diff" is more appropriate.

Ah OK. I thought that handle_alias() in git.c would chdir() and set
GIT_DIR because it called setup_git_directory_gently(), and thus
requires NO_SETUP to restore the initial environment, but turns out it
chdir()s back to the original directory, and sets GIT_DIR
appropriately, so we're OK.

Thanks,
Paul