Hello,
On 08.08.2018 23:18, Junio C Hamano wrote:
Paul-Sebastian Ungureanu <ungureanupaulsebastian@xxxxxxxxx> writes:
From: Joel Teichroeb <joel@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxx>
Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@xxxxxxxxx>
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@xxxxxxxxx/
+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
