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

> 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.
>