Re: [GSoC][PATCH v7 05/26] stash: convert apply to builtin

[Junio C Hamano <gitster@pobox.com>]

Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> writes:

> From: Joel Teichroeb <joel@teichroeb.net>
>
> Add a builtin helper for performing stash commands. Converting
> all at once proved hard to review, so starting with just apply
> lets conversion get started without the other commands being
> finished.
>
> The helper is being implemented as a drop in replacement for
> stash so that when it is complete it can simply be renamed and
> the shell script deleted.
>
> Delete the contents of the apply_stash shell function and replace
> it with a call to stash--helper apply until pop is also
> converted.
>
> Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
> Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>

Good to see that the right way to forward a patch from another
person is used, but is this a GSoC project?

> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> new file mode 100644
> index 000000000..ef6a9d30d
> --- /dev/null
> +++ b/builtin/stash--helper.c
> @@ -0,0 +1,452 @@
> +#include "builtin.h"
> +#include "config.h"
> +#include "parse-options.h"
> +#include "refs.h"
> +#include "lockfile.h"
> +#include "cache-tree.h"
> +#include "unpack-trees.h"
> +#include "merge-recursive.h"
> +#include "argv-array.h"
> +#include "run-command.h"
> +#include "dir.h"
> +#include "rerere.h"

Wow, "apply" is a biggie, as you'd pretty much have to do
everything, like merging and updating the index and asking rerere to
auto-resolve.  Quite a many include files.

> +static const char * const git_stash_helper_usage[] = {
> +	N_("git stash--helper apply [--index] [-q|--quiet] [<stash>]"),
> +	NULL
> +};
> +
> +static const char * const git_stash_helper_apply_usage[] = {
> +	N_("git stash--helper apply [--index] [-q|--quiet] [<stash>]"),
> +	NULL
> +};
> + ...
> +static void assert_stash_like(struct stash_info *info, const char * revision)

This inherits an unfortunate name from the scripted version (it does
more than asserting), but it is OK to keep the original name,
especially in this early step in the series.

Lose the SP in "* revision"; the asterisk sticks to the variable/parameter name.

> +{
> +	if (get_oidf(&info->b_commit, "%s^1", revision) ||
> +	    get_oidf(&info->w_tree, "%s:", revision) ||
> +	    get_oidf(&info->b_tree, "%s^1:", revision) ||
> +	    get_oidf(&info->i_tree, "%s^2:", revision)) {
> +		free_stash_info(info);
> +		error(_("'%s' is not a stash-like commit"), revision);
> +		exit(128);
> +	}
> +}

> +static int reset_tree(struct object_id *i_tree, int update, int reset)
> +{
> ...
> +}

Kind-a surprising that there is no helper function to do this
already.  The implementation looked OK, though.

> +static int apply_cached(struct strbuf *out)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +
> +	/*
> +	 * Apply currently only reads either from stdin or a file, thus
> +	 * apply_all_patches would have to be updated to optionally take a
> +	 * buffer.
> +	 */
> +	cp.git_cmd = 1;
> +	argv_array_pushl(&cp.args, "apply", "--cached", NULL);
> +	return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);
> +}

Applying and updating the index is more resource intensive than
spawning a process, and not having to worry about the process dying
is a benefit, so overall, making this into an internal call would be
a lot lower priority, I would guess.

> +static int reset_head(const char *prefix)
> +{

This is resetting the index to the HEAD, right?  reset_head sounds
as if it takes a commit-ish and moves HEAD there.