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

[Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>]

Hello,

On 08.08.2018 23:18, Junio C Hamano wrote:
> 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?

Yes, it is. I forgot to add the [GSoC] tag in the last series of patches.

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

Will do so.

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

Indeed. In the last but one patch [1], I tried to convert all of the 
"apply" processes. At the moment, `apply_all_patches()` cannot take a 
buffer. A solution for this was to write the buffer into a file and pass 
the name of that file to the function. Of course, this might not be a 
bright idea and for this reason I am not sure if that patch is worth.

[1]
https://public-inbox.org/git/56500d98f9d5daaa5f21a43767885baede86e3a0.1533753605.git.ungureanupaulsebastian@gmail.com/

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

Yes, it resets the index to the HEAD.

Best,
Paul