Re: [PATCH v3 2/3] wt-status: teach wt_status_collect about merges in progress
Samuel Lijin writes:
> To fix the breakages documented by t7501, the next patch in this series
> will teach wt_status_collect() to set the committable bit, instead of
> having wt_longstatus_print_updated() and show_merge_in_progress() set it
> (which is what currently happens). Unfortunately, wt_status_collect()
> needs to know whether or not there is a merge in progress to set the bit
> correctly,
s/correctly,/correctly (a brief desription of why),/
would be nicer. The description might be
(after a merge, it is OK for the result to be identical to HEAD,
which usually causes a "nothing to commit" error)
or something like that.
> so teach its (two) callers to create, initialize, and pass
> in instances of wt_status_state, which records this metadata.
>
> Since wt_longstatus_print() and show_merge_in_progress() are in the same
> callpaths and currently create and init copies of wt_status_state,
> remove that logic and instead pass wt_status_state through.
OK. Sounds like a good clean-up.
> Make wt_status_get_state easier to use, add a helper method to clean up
Your description so far marked function names with trailing ();
let's do so consistently for wt_status_get_state(), too.
> wt_status_state, const-ify as many struct pointers in method signatures
> as possible, and add a FIXME for a struct pointer which should be const
> but isn't (that this patch series will not address).
"should be but isn't" because...? I am wondering if it is better to
leave _all_ constifying to a later effort, if we are leaving some of
them behind anyway. It would be better only if it will make this
patch easier to read if we did so.
Also you did s/commitable/committable/ everywhere, which was
somewhat distracting. It would have been nicer to follow if that
were a separate preparatory clean-up patch.
>
> Signed-off-by: Samuel Lijin
> ---
> builtin/commit.c | 32
> ref-filter.c | 3 +-
> wt-status.c | 188 +++
> wt-status.h | 13 ++--
> 4 files changed, 120 insertions(+), 116 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 37fcb55ab..79ef4f11a 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -463,6 +463,7 @@ static const char *prepare_index(int argc, const char
> **argv, const char *prefix
> static int run_status(FILE *fp, const char *index_file, const char *prefix,
> int nowarn,
> struct wt_status *s)
> {
> + struct wt_status_state state;
> struct object_id oid;
>
> if (s->relative_paths)
> @@ -482,10 +483,12 @@ static int run_status(FILE *fp, const char *index_file,
> const char *prefix, int
> s->status_format = status_format;
> s->ignore_submodule_arg = ignore_submodule_arg;
>
> - wt_status_collect(s);
> - wt_status_print(s);
> + wt_status_get_state(s, &state);
> + wt_status_collect(s, &state);
> + wt_status_print(s, &state);
> + wt_status_clear_state(&state);
>
> - return s->commitable;
> + return s->committable;
> }
>
> static int is_a_merge(const struct commit *current_head)
> @@ -631,7 +634,7 @@ static int prepare_to_commit(const char *index_file,
> const char *prefix,
> {
> struct stat statbuf;
> struct strbuf committer_ident = STRBUF_INIT;
> - int commitable;
> + int committable;
> struct strbuf sb = STRBUF_INIT;
> const char *hook_arg1 = NULL;
> const char *hook_arg2 = NULL;
> @@ -848,7 +851,7 @@ static int prepare_to_commit(const char *index_file,
> const char *prefix,
>
> saved_color_setting = s->use_color;
> s->use_color = 0;
> - commitable = run_status(s->fp, index_file, prefix, 1, s);
> + committable = run_status(s->fp, index_file, prefix, 1, s);
> s->use_color = saved_color_setting;
> } else {
> struct object_id oid;
> @@ -866,7 +869,7 @@ static int prepare_to_commit(const char *index_file,
> const char *prefix,
> for (i = 0; i < active_nr; i++)
> if (ce_intent_to_add(active_cache[i]))
> ita_nr++;
> - commitable = active_nr - ita_nr > 0;
> + committable = active_nr - ita_nr > 0;
> } else {
> /*
>* Unless the user did explicitly request a submodule
> @@ -882,7 +885,7 @@ static int prepare_to_commit(const char *index_file,
> const char *prefix,
> if (ignore_submodule_arg &&
> !strcmp(ignore_submodule_arg, "all"))
> flags.ignore_submodules = 1;
> - commitable = index_differs_from(parent, &flags, 1);
> + committable = index_differs_from(parent, &flags, 1);
> }
> }
> strbuf_release(&committer_ident);
> @@ -894,7 +897,7 @@ static
[PATCH v3 2/3] wt-status: teach wt_status_collect about merges in progress
To fix the breakages documented by t7501, the next patch in this series
will teach wt_status_collect() to set the committable bit, instead of
having wt_longstatus_print_updated() and show_merge_in_progress() set it
(which is what currently happens). Unfortunately, wt_status_collect()
needs to know whether or not there is a merge in progress to set the bit
correctly, so teach its (two) callers to create, initialize, and pass
in instances of wt_status_state, which records this metadata.
Since wt_longstatus_print() and show_merge_in_progress() are in the same
callpaths and currently create and init copies of wt_status_state,
remove that logic and instead pass wt_status_state through.
Make wt_status_get_state easier to use, add a helper method to clean up
wt_status_state, const-ify as many struct pointers in method signatures
as possible, and add a FIXME for a struct pointer which should be const
but isn't (that this patch series will not address).
Signed-off-by: Samuel Lijin
---
builtin/commit.c | 32
ref-filter.c | 3 +-
wt-status.c | 188 +++
wt-status.h | 13 ++--
4 files changed, 120 insertions(+), 116 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 37fcb55ab..79ef4f11a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -463,6 +463,7 @@ static const char *prepare_index(int argc, const char
**argv, const char *prefix
static int run_status(FILE *fp, const char *index_file, const char *prefix,
int nowarn,
struct wt_status *s)
{
+ struct wt_status_state state;
struct object_id oid;
if (s->relative_paths)
@@ -482,10 +483,12 @@ static int run_status(FILE *fp, const char *index_file,
const char *prefix, int
s->status_format = status_format;
s->ignore_submodule_arg = ignore_submodule_arg;
- wt_status_collect(s);
- wt_status_print(s);
+ wt_status_get_state(s, &state);
+ wt_status_collect(s, &state);
+ wt_status_print(s, &state);
+ wt_status_clear_state(&state);
- return s->commitable;
+ return s->committable;
}
static int is_a_merge(const struct commit *current_head)
@@ -631,7 +634,7 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
{
struct stat statbuf;
struct strbuf committer_ident = STRBUF_INIT;
- int commitable;
+ int committable;
struct strbuf sb = STRBUF_INIT;
const char *hook_arg1 = NULL;
const char *hook_arg2 = NULL;
@@ -848,7 +851,7 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
saved_color_setting = s->use_color;
s->use_color = 0;
- commitable = run_status(s->fp, index_file, prefix, 1, s);
+ committable = run_status(s->fp, index_file, prefix, 1, s);
s->use_color = saved_color_setting;
} else {
struct object_id oid;
@@ -866,7 +869,7 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
for (i = 0; i < active_nr; i++)
if (ce_intent_to_add(active_cache[i]))
ita_nr++;
- commitable = active_nr - ita_nr > 0;
+ committable = active_nr - ita_nr > 0;
} else {
/*
* Unless the user did explicitly request a submodule
@@ -882,7 +885,7 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
if (ignore_submodule_arg &&
!strcmp(ignore_submodule_arg, "all"))
flags.ignore_submodules = 1;
- commitable = index_differs_from(parent, &flags, 1);
+ committable = index_differs_from(parent, &flags, 1);
}
}
strbuf_release(&committer_ident);
@@ -894,7 +897,7 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
* explicit --allow-empty. In the cherry-pick case, it may be
* empty due to conflict resolution, which the user should okay.
*/
- if (!commitable && whence != FROM_MERGE && !allow_empty &&
+ if (!committable && whence != FROM_MERGE && !allow_empty &&
!(amend && is_a_merge(current_head))) {
s->display_comment_prefix = old_display_comment_prefix;
run_status(stdout, index_file, prefix, 0, s);
@@ -1164,14 +1167,14 @@ static int parse_and_validate_options(int argc, const
char *argv[],
static int dry_run_commit(int argc, const char **argv, const char *prefix,
const struct commit *current_head, struct wt_status
*s)
{
- int commitable;
+ int committable;
const char *index_file;
index_file = prepare_index(argc, argv, prefix, current_head, 1
