Re: [PATCH/WIP v3 03/31] am: implement skeletal builtin am
On Fri, Jun 19, 2015 at 4:26 AM, Junio C Hamano wrote:
> Paul Tan 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/WIP v3 03/31] am: implement skeletal builtin am
Paul Tan writes:
> For the purpose of rewriting git-am.sh into a C builtin, implement a
> skeletal builtin/am.c that redirects to $GIT_EXEC_PATH/git-am if the
> environment variable _GIT_USE_BUILTIN_AM is not defined. Since in the
> Makefile git-am.sh takes precedence over builtin/am.c,
> $GIT_EXEC_PATH/git-am will contain the shell script git-am.sh, and thus
> this allows us to fall back on the functional git-am.sh when running the
> test suite for tests that depend on a working git-am implementation.
>
> Since git-am.sh cannot handle any environment modifications by
> setup_git_directory(), "am" has to be declared as NO_SETUP in git.c. On
> the other hand, to re-implement git-am.sh in builtin/am.c, we do need to
> run all the git dir and work tree setup logic that git.c does for us. As
> such, we work around this temporarily by copying the logic in git.c's
> run_builtin(), which amounts to:
>
> prefix = setup_git_directory();
> trace_repo_setup(prefix);
> setup_work_tree();
>
> This redirection should be removed when all the features of git-am.sh
> have been re-implemented in builtin/am.c.
>
> Helped-by: Junio C Hamano
> Signed-off-by: Paul Tan
> ---
>
> Notes:
> v3
>
> * Style fixes
>
> * git-am.sh cannot handle the chdir() and GIT_DIR envionment variable
> that setup_git_directory() sets, so we work around it by copying the
> logic of git.c's run_builtin(), and running it only when we are using
> the builtin am.
>
> Makefile | 1 +
> builtin.h| 1 +
> builtin/am.c | 28
> git.c| 1 +
> 4 files changed, 31 insertions(+)
> create mode 100644 builtin/am.c
>
> diff --git a/Makefile b/Makefile
> index 93e4fa2..ff9bdc0 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -811,6 +811,7 @@ LIB_OBJS += xdiff-interface.o
> LIB_OBJS += zlib.o
>
> BUILTIN_OBJS += builtin/add.o
> +BUILTIN_OBJS += builtin/am.o
> BUILTIN_OBJS += builtin/annotate.o
> BUILTIN_OBJS += builtin/apply.o
> BUILTIN_OBJS += builtin/archive.o
> diff --git a/builtin.h b/builtin.h
> index ea3c834..f30cf00 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -30,6 +30,7 @@ extern int textconv_object(const char *path, unsigned mode,
> const unsigned char
> extern int is_builtin(const char *s);
>
> extern int cmd_add(int argc, const char **argv, const char *prefix);
> +extern int cmd_am(int argc, const char **argv, const char *prefix);
> extern int cmd_annotate(int argc, const char **argv, const char *prefix);
> extern int cmd_apply(int argc, const char **argv, const char *prefix);
> extern int cmd_archive(int argc, const char **argv, const char *prefix);
> diff --git a/builtin/am.c b/builtin/am.c
> new file mode 100644
> index 000..dbc8836
> --- /dev/null
> +++ b/builtin/am.c
> @@ -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"). Also mention that the entry in the
commands[] array needs "RUN_SETUP | NEED_WORK_TREE" added, I think.
> + 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.
> { "annotate", cmd_annotate, RUN_SETUP },
> { "apply", cmd_apply, RUN_SETUP_GENTLY },
> { "archive", cmd_archive },
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/WIP v3 03/31] am: implement skeletal builtin am
For the purpose of rewriting git-am.sh into a C builtin, implement a
skeletal builtin/am.c that redirects to $GIT_EXEC_PATH/git-am if the
environment variable _GIT_USE_BUILTIN_AM is not defined. Since in the
Makefile git-am.sh takes precedence over builtin/am.c,
$GIT_EXEC_PATH/git-am will contain the shell script git-am.sh, and thus
this allows us to fall back on the functional git-am.sh when running the
test suite for tests that depend on a working git-am implementation.
Since git-am.sh cannot handle any environment modifications by
setup_git_directory(), "am" has to be declared as NO_SETUP in git.c. On
the other hand, to re-implement git-am.sh in builtin/am.c, we do need to
run all the git dir and work tree setup logic that git.c does for us. As
such, we work around this temporarily by copying the logic in git.c's
run_builtin(), which amounts to:
prefix = setup_git_directory();
trace_repo_setup(prefix);
setup_work_tree();
This redirection should be removed when all the features of git-am.sh
have been re-implemented in builtin/am.c.
Helped-by: Junio C Hamano
Signed-off-by: Paul Tan
---
Notes:
v3
* Style fixes
* git-am.sh cannot handle the chdir() and GIT_DIR envionment variable
that setup_git_directory() sets, so we work around it by copying the
logic of git.c's run_builtin(), and running it only when we are using
the builtin am.
Makefile | 1 +
builtin.h| 1 +
builtin/am.c | 28
git.c| 1 +
4 files changed, 31 insertions(+)
create mode 100644 builtin/am.c
diff --git a/Makefile b/Makefile
index 93e4fa2..ff9bdc0 100644
--- a/Makefile
+++ b/Makefile
@@ -811,6 +811,7 @@ LIB_OBJS += xdiff-interface.o
LIB_OBJS += zlib.o
BUILTIN_OBJS += builtin/add.o
+BUILTIN_OBJS += builtin/am.o
BUILTIN_OBJS += builtin/annotate.o
BUILTIN_OBJS += builtin/apply.o
BUILTIN_OBJS += builtin/archive.o
diff --git a/builtin.h b/builtin.h
index ea3c834..f30cf00 100644
--- a/builtin.h
+++ b/builtin.h
@@ -30,6 +30,7 @@ extern int textconv_object(const char *path, unsigned mode,
const unsigned char
extern int is_builtin(const char *s);
extern int cmd_add(int argc, const char **argv, const char *prefix);
+extern int cmd_am(int argc, const char **argv, const char *prefix);
extern int cmd_annotate(int argc, const char **argv, const char *prefix);
extern int cmd_apply(int argc, const char **argv, const char *prefix);
extern int cmd_archive(int argc, const char **argv, const char *prefix);
diff --git a/builtin/am.c b/builtin/am.c
new file mode 100644
index 000..dbc8836
--- /dev/null
+++ b/builtin/am.c
@@ -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.
+*/
+ 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 },
{ "annotate", cmd_annotate, RUN_SETUP },
{ "apply", cmd_apply, RUN_SETUP_GENTLY },
{ "archive", cmd_archive },
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
