Re: [GSoC][PATCH v7 15/26] stash: convert create to builtin
On 08/18, Paul Sebastian Ungureanu wrote:
> On Thu, Aug 16, 2018 at 1:13 AM, Thomas Gummerer wrote:
> > On 08/08, Paul-Sebastian Ungureanu wrote:
> >>
> >> [...]
> >>
> >> + ret = -1;
> >> + goto done;
> >> + }
> >> + untracked_commit_option = 1;
> >> + }
> >> + if (patch_mode) {
> >> + ret = stash_patch(info, argv);
> >> + if (ret < 0) {
> >> + printf_ln("Cannot save the current worktree state");
> >> + goto done;
> >> + } else if (ret > 0) {
> >> + goto done;
> >> + }
> >> + } else {
> >> + if (stash_working_tree(info, argv)) {
> >> + printf_ln("Cannot save the current worktree state");
> >> + ret = -1;
> >> + goto done;
> >> + }
> >> + }
> >> +
> >> + if (!*stash_msg || !strlen(*stash_msg))
> >> + strbuf_addf(_stash_msg, "WIP on %s", msg.buf);
> >> + else
> >> + strbuf_addf(_stash_msg, "On %s: %s\n", branch_name,
> >> + *stash_msg);
> >> + *stash_msg = strbuf_detach(_stash_msg, NULL);
> >
> > strbuf_detach means we're taking ownership of the memory, so we'll
> > have to free it afterwards. Looking at this we may not even want to
> > re-use the 'stash_msg' variable here, but instead introduce another
> > variable for it, so it doesn't have a dual meaning in this function.
> >
> >> +
> >> + /*
> >> + * `parents` will be empty after calling `commit_tree()`, so there is
> >> + * no need to call `free_commit_list()`
> >> + */
> >> + parents = NULL;
> >> + if (untracked_commit_option)
> >> + commit_list_insert(lookup_commit(the_repository,
> >> >u_commit), );
> >> + commit_list_insert(lookup_commit(the_repository, >i_commit),
> >> );
> >> + commit_list_insert(head_commit, );
> >> +
> >> + if (commit_tree(*stash_msg, strlen(*stash_msg), >w_tree,
> >> + parents, >w_commit, NULL, NULL)) {
> >> + printf_ln("Cannot record working tree state");
> >> + ret = -1;
> >> + goto done;
> >> + }
> >> +
> >> +done:
> >> + strbuf_release(_tree_label);
> >> + strbuf_release();
> >> + strbuf_release();
> >> + strbuf_release(_stash_msg);
> >> + return ret;
> >> +}
> >> +
> >> +static int create_stash(int argc, const char **argv, const char *prefix)
> >> +{
> >> + int include_untracked = 0;
> >> + int ret = 0;
> >> + const char *stash_msg = NULL;
> >> + struct stash_info info;
> >> + struct option options[] = {
> >> + OPT_BOOL('u', "include-untracked", _untracked,
> >> + N_("include untracked files in stash")),
> >> + OPT_STRING('m', "message", _msg, N_("message"),
> >> + N_("stash message")),
> >> + OPT_END()
> >> + };
> >> +
> >> + argc = parse_options(argc, argv, prefix, options,
> >> + git_stash_helper_create_usage,
> >> + 0);
> >> +
> >> + ret = do_create_stash(argc, argv, prefix, _msg,
> >> + include_untracked, 0, );
> >
> > stash_msg doesn't have to be passed as a pointer to a pointer here, as
> > we never need the modified value after this function returns. I think
> > just passing 'stash_msg' here instead of '_msg' will make
> > 'do_create_stash' slightly easier to read.
>
> That's right, but `do_create_stash()` is also called by
> `do_push_stash()`, which will need the modified value.
Ah right, I didn't read that far yet when leaving this comment :)
Reading the original push_stash again though, do we actually need the
modified value in 'do_push_stash()'? The original lines are:
create_stash -m "$stash_msg" -u "$untracked" -- "$@"
store_stash -m "$stash_msg" -q $w_commit ||
die "$(gettext "Cannot save the current status")"
'$stash_msg' gets passed in to 'create_stash()', but is the
'stash_msg' variable inside of 'create_stash()' is local and only the
local copy is modified, so 'store_stash()' would still get the
original. Or am I missing something?
Re: [GSoC][PATCH v7 15/26] stash: convert create to builtin
On Thu, Aug 16, 2018 at 1:13 AM, Thomas Gummerer wrote:
> On 08/08, Paul-Sebastian Ungureanu wrote:
>> Add stash create to the helper.
>>
>> Signed-off-by: Paul-Sebastian Ungureanu
>> ---
>> builtin/stash--helper.c | 406
>> git-stash.sh| 2 +-
>> 2 files changed, 407 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
>> index 5ff810f8c..a4e57899b 100644
>> --- a/builtin/stash--helper.c
>> +++ b/builtin/stash--helper.c
>> @@ -21,6 +21,7 @@ static const char * const git_stash_helper_usage[] = {
>> N_("git stash--helper branch []"),
>> N_("git stash--helper clear"),
>> N_("git stash--helper store [-m|--message ] [-q|--quiet]
>> "),
>> + N_("git stash--helper create []"),
>> NULL
>> };
>>
>> @@ -64,6 +65,11 @@ static const char * const git_stash_helper_store_usage[]
>> = {
>> NULL
>> };
>>
>> +static const char * const git_stash_helper_create_usage[] = {
>> + N_("git stash--helper create []"),
>> + NULL
>> +};
>> +
>> static const char *ref_stash = "refs/stash";
>> static int quiet;
>> static struct strbuf stash_index_path = STRBUF_INIT;
>> @@ -781,6 +787,404 @@ static int store_stash(int argc, const char **argv,
>> const char *prefix)
>> return do_store_stash(argv[0], stash_msg, quiet);
>> }
>>
>> [...]
>>
>> +
>> +static int do_create_stash(int argc, const char **argv, const char *prefix,
>> +const char **stash_msg, int include_untracked,
>> +int patch_mode, struct stash_info *info)
>> +{
>> + int untracked_commit_option = 0;
>> + int ret = 0;
>> + int subject_len;
>> + int flags;
>> + const char *head_short_sha1 = NULL;
>> + const char *branch_ref = NULL;
>> + const char *head_subject = NULL;
>> + const char *branch_name = "(no branch)";
>> + struct commit *head_commit = NULL;
>> + struct commit_list *parents = NULL;
>> + struct strbuf msg = STRBUF_INIT;
>> + struct strbuf commit_tree_label = STRBUF_INIT;
>> + struct strbuf out = STRBUF_INIT;
>> + struct strbuf final_stash_msg = STRBUF_INIT;
>> +
>> + read_cache_preload(NULL);
>> + refresh_cache(REFRESH_QUIET);
>> +
>> + if (!check_changes(argv, include_untracked, prefix)) {
>> + ret = 1;
>> + goto done;
>
> I wonder if we can just 'exit(0)' here, instead of returning. This
> whole command is a builtin, and I *think* outside of 'libgit.a' exiting
> early is fine. It does mean that we're not free'ing the memory
> though, which means a leak checker would probably complain. So
> dunno. It would simplify the code a little, but not sure it's worth it.
Indeed, there shouldn't be any problem by calling exit(0).
>> + }
>> +
>> + if (get_oid("HEAD", >b_commit)) {
>> + fprintf_ln(stderr, "You do not have the initial commit yet");
>> + ret = -1;
>> + goto done;
>> + } else {
>> + head_commit = lookup_commit(the_repository, >b_commit);
>> + }
>> +
>> + branch_ref = resolve_ref_unsafe("HEAD", 0, NULL, );
>> + if (flags & REF_ISSYMREF)
>> + branch_name = strrchr(branch_ref, '/') + 1;
>> + head_short_sha1 = find_unique_abbrev(_commit->object.oid,
>> + DEFAULT_ABBREV);
>> + subject_len = find_commit_subject(get_commit_buffer(head_commit, NULL),
>> + _subject);
>> + strbuf_addf(, "%s: %s %.*s\n", branch_name, head_short_sha1,
>> + subject_len, head_subject);
>
> I think this can be written in a slightly simpler way:
>
> head_short_sha1 = find_unique_abbrev(_commit->object.oid,
> DEFAULT_ABBREV);
> strbuf_addf(, "%s: %s", branch_name, head_short_sha1);
> pp_commit_easy(CMIT_FMT_ONELINE, head_commit, );
> strbuf_addch(, '\n');
>
> The other advantage this brings is that it is consistent with other
> places where we print/use the subject of a commit (e.g. in 'git reset
> --hard').
Thanks for this suggestion.
>> +
>> + strbuf_addf(_tree_label, "index on %s\n", msg.buf);
>> + commit_list_insert(head_commit, );
>> + if (write_cache_as_tree(>i_tree, 0, NULL) ||
>> + commit_tree(commit_tree_label.buf, commit_tree_label.len,
>> + >i_tree, parents, >i_commit, NULL, NULL)) {
>> + fprintf_ln(stderr, "Cannot save the current index state");
>
> Looks like this message is translated in the current 'git stash'
> implementation, so it should be here as well. Same for the messages
> below.
>
>> + ret = -1;
>> + goto done;
>> + }
>> +
>> + if (include_untracked && get_untracked_files(argv, 1,
>> + include_untracked, ))
>> {
>> + if (save_untracked_files(info, , )) {
>> +
Re: [GSoC][PATCH v7 15/26] stash: convert create to builtin
On 08/08, Paul-Sebastian Ungureanu wrote:
> Add stash create to the helper.
>
> Signed-off-by: Paul-Sebastian Ungureanu
> ---
> builtin/stash--helper.c | 406
> git-stash.sh| 2 +-
> 2 files changed, 407 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> index 5ff810f8c..a4e57899b 100644
> --- a/builtin/stash--helper.c
> +++ b/builtin/stash--helper.c
> @@ -21,6 +21,7 @@ static const char * const git_stash_helper_usage[] = {
> N_("git stash--helper branch []"),
> N_("git stash--helper clear"),
> N_("git stash--helper store [-m|--message ] [-q|--quiet]
> "),
> + N_("git stash--helper create []"),
> NULL
> };
>
> @@ -64,6 +65,11 @@ static const char * const git_stash_helper_store_usage[] =
> {
> NULL
> };
>
> +static const char * const git_stash_helper_create_usage[] = {
> + N_("git stash--helper create []"),
> + NULL
> +};
> +
> static const char *ref_stash = "refs/stash";
> static int quiet;
> static struct strbuf stash_index_path = STRBUF_INIT;
> @@ -781,6 +787,404 @@ static int store_stash(int argc, const char **argv,
> const char *prefix)
> return do_store_stash(argv[0], stash_msg, quiet);
> }
>
> [...]
>
> +
> +static int do_create_stash(int argc, const char **argv, const char *prefix,
> +const char **stash_msg, int include_untracked,
> +int patch_mode, struct stash_info *info)
> +{
> + int untracked_commit_option = 0;
> + int ret = 0;
> + int subject_len;
> + int flags;
> + const char *head_short_sha1 = NULL;
> + const char *branch_ref = NULL;
> + const char *head_subject = NULL;
> + const char *branch_name = "(no branch)";
> + struct commit *head_commit = NULL;
> + struct commit_list *parents = NULL;
> + struct strbuf msg = STRBUF_INIT;
> + struct strbuf commit_tree_label = STRBUF_INIT;
> + struct strbuf out = STRBUF_INIT;
> + struct strbuf final_stash_msg = STRBUF_INIT;
> +
> + read_cache_preload(NULL);
> + refresh_cache(REFRESH_QUIET);
> +
> + if (!check_changes(argv, include_untracked, prefix)) {
> + ret = 1;
> + goto done;
I wonder if we can just 'exit(0)' here, instead of returning. This
whole command is a builtin, and I *think* outside of 'libgit.a' exiting
early is fine. It does mean that we're not free'ing the memory
though, which means a leak checker would probably complain. So
dunno. It would simplify the code a little, but not sure it's worth it.
> + }
> +
> + if (get_oid("HEAD", >b_commit)) {
> + fprintf_ln(stderr, "You do not have the initial commit yet");
> + ret = -1;
> + goto done;
> + } else {
> + head_commit = lookup_commit(the_repository, >b_commit);
> + }
> +
> + branch_ref = resolve_ref_unsafe("HEAD", 0, NULL, );
> + if (flags & REF_ISSYMREF)
> + branch_name = strrchr(branch_ref, '/') + 1;
> + head_short_sha1 = find_unique_abbrev(_commit->object.oid,
> + DEFAULT_ABBREV);
> + subject_len = find_commit_subject(get_commit_buffer(head_commit, NULL),
> + _subject);
> + strbuf_addf(, "%s: %s %.*s\n", branch_name, head_short_sha1,
> + subject_len, head_subject);
I think this can be written in a slightly simpler way:
head_short_sha1 = find_unique_abbrev(_commit->object.oid,
DEFAULT_ABBREV);
strbuf_addf(, "%s: %s", branch_name, head_short_sha1);
pp_commit_easy(CMIT_FMT_ONELINE, head_commit, );
strbuf_addch(, '\n');
The other advantage this brings is that it is consistent with other
places where we print/use the subject of a commit (e.g. in 'git reset
--hard').
> +
> + strbuf_addf(_tree_label, "index on %s\n", msg.buf);
> + commit_list_insert(head_commit, );
> + if (write_cache_as_tree(>i_tree, 0, NULL) ||
> + commit_tree(commit_tree_label.buf, commit_tree_label.len,
> + >i_tree, parents, >i_commit, NULL, NULL)) {
> + fprintf_ln(stderr, "Cannot save the current index state");
Looks like this message is translated in the current 'git stash'
implementation, so it should be here as well. Same for the messages
below.
> + ret = -1;
> + goto done;
> + }
> +
> + if (include_untracked && get_untracked_files(argv, 1,
> + include_untracked, )) {
> + if (save_untracked_files(info, , )) {
> + printf_ln("Cannot save the untracked files");
Why does this go to stdout, whereas "Cannot save the current index
state" above goes to stderr? In the shell version of git stash these
all go to stderr fwiw. There are a few similar cases, it would
probably be worth going
[GSoC][PATCH v7 15/26] stash: convert create to builtin
Add stash create to the helper.
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash--helper.c | 406
git-stash.sh| 2 +-
2 files changed, 407 insertions(+), 1 deletion(-)
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 5ff810f8c..a4e57899b 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -21,6 +21,7 @@ static const char * const git_stash_helper_usage[] = {
N_("git stash--helper branch []"),
N_("git stash--helper clear"),
N_("git stash--helper store [-m|--message ] [-q|--quiet]
"),
+ N_("git stash--helper create []"),
NULL
};
@@ -64,6 +65,11 @@ static const char * const git_stash_helper_store_usage[] = {
NULL
};
+static const char * const git_stash_helper_create_usage[] = {
+ N_("git stash--helper create []"),
+ NULL
+};
+
static const char *ref_stash = "refs/stash";
static int quiet;
static struct strbuf stash_index_path = STRBUF_INIT;
@@ -781,6 +787,404 @@ static int store_stash(int argc, const char **argv, const
char *prefix)
return do_store_stash(argv[0], stash_msg, quiet);
}
+/*
+ * `out` will be filled with the names of untracked files. The return value is:
+ *
+ * < 0 if there was a bug (any arg given outside the repo will be detected
+ * by `setup_revision()`)
+ * = 0 if there are not any untracked files
+ * > 0 if there are untracked files
+ */
+static int get_untracked_files(const char **argv, int line_term,
+ int include_untracked, struct strbuf *out)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+ cp.git_cmd = 1;
+ argv_array_pushl(, "ls-files", "-o", NULL);
+ if (line_term)
+ argv_array_push(, "-z");
+ if (include_untracked != 2)
+ argv_array_push(, "--exclude-standard");
+ argv_array_push(, "--");
+ if (argv)
+ argv_array_pushv(, argv);
+
+ if (pipe_command(, NULL, 0, out, 0, NULL, 0))
+ return -1;
+ return out->len;
+}
+
+/*
+ * The return value of `check_changes()` can be:
+ *
+ * < 0 if there was an error
+ * = 0 if there are no changes.
+ * > 0 if there are changes.
+ */
+static int check_changes(const char **argv, int include_untracked,
+const char *prefix)
+{
+ int result;
+ int ret = 0;
+ struct rev_info rev;
+ struct object_id dummy;
+ struct strbuf out = STRBUF_INIT;
+ struct argv_array args = ARGV_ARRAY_INIT;
+
+ init_revisions(, prefix);
+ parse_pathspec(_data, 0, PATHSPEC_PREFER_FULL,
+ prefix, argv);
+
+ rev.diffopt.flags.quick = 1;
+ rev.diffopt.flags.ignore_submodules = 1;
+ rev.abbrev = 0;
+
+ /* No initial commit. */
+ if (get_oid("HEAD", )) {
+ ret = -1;
+ goto done;
+ }
+
+ add_head_to_pending();
+ diff_setup_done();
+
+ if (read_cache() < 0) {
+ ret = -1;
+ goto done;
+ }
+ result = run_diff_index(, 1);
+ if (diff_result_code(, result)) {
+ ret = 1;
+ goto done;
+ }
+
+ object_array_clear();
+ result = run_diff_files(, 0);
+ if (diff_result_code(, result)) {
+ ret = 1;
+ goto done;
+ }
+
+ if (include_untracked && get_untracked_files(argv, 0,
+include_untracked, ))
+ ret = 1;
+
+done:
+ strbuf_release();
+ argv_array_clear();
+ return ret;
+}
+
+static int save_untracked_files(struct stash_info *info, struct strbuf *msg,
+ struct strbuf *out)
+{
+ int ret = 0;
+ struct strbuf untracked_msg = STRBUF_INIT;
+ struct strbuf out2 = STRBUF_INIT;
+ struct child_process cp = CHILD_PROCESS_INIT;
+ struct child_process cp2 = CHILD_PROCESS_INIT;
+
+ cp.git_cmd = 1;
+ argv_array_pushl(, "update-index", "-z", "--add",
+"--remove", "--stdin", NULL);
+ argv_array_pushf(_array, "GIT_INDEX_FILE=%s",
+stash_index_path.buf);
+
+ strbuf_addf(_msg, "untracked files on %s\n", msg->buf);
+ if (pipe_command(, out->buf, out->len, NULL, 0, NULL, 0)) {
+ ret = -1;
+ goto done;
+ }
+
+ cp2.git_cmd = 1;
+ argv_array_push(, "write-tree");
+ argv_array_pushf(_array, "GIT_INDEX_FILE=%s",
+stash_index_path.buf);
+ if (pipe_command(, NULL, 0, , 0,NULL, 0)) {
+ ret = -1;
+ goto done;
+ }
+ get_oid_hex(out2.buf, >u_tree);
+
+ if (commit_tree(untracked_msg.buf, untracked_msg.len,
+ >u_tree, NULL, >u_commit, NULL, NULL)) {
+ ret = -1;
+ goto done;
+ }
+
+done:
+
