Hi,
Jeff Hostetler wrote:
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -141,6 +141,7 @@ static int sequencer_in_use;
> static int use_editor = 1, include_status = 1;
> static int show_ignored_in_status, have_option_m;
> static struct strbuf message = STRBUF_INIT;
> +static int ahead_behind_opt = -1;
nit: is there a logical place amid these constants to put the new option
instead of chronological order to make it easier to read through later?
That also has the side-benefit of making the new option less likely to
collidate with other patches that add a new option to commit.
That collection of options seems to be mostly about how the commit
message is generated. Maybe this one could go after status_format:
static enum wt_status_format status_format = ...;
static int ahead_behind;
Even better if it can be made into a local in cmd_status.
[...]
> @@ -1369,6 +1370,8 @@ int cmd_status(int argc, const char **argv, const char
> *prefix)
> N_("ignore changes to submodules, optional when: all, dirty,
> untracked. (Default: all)"),
> PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
> OPT_COLUMN(0, "column", &s.colopts, N_("list untracked files in
> columns")),
> + OPT_BOOL(0, "ahead-behind", &ahead_behind_opt,
> + N_("compute branch ahead/behind values")),
> OPT_END(),
Similar question: is there a natural place in "git status -h" to show
the new option instead of chronological order?
What does the value of the ahead_behind variable represent? -1 means
unset so that we use config? A comment might help.
[...]
> @@ -1389,6 +1392,21 @@ int cmd_status(int argc, const char **argv, const char
> *prefix)
> PATHSPEC_PREFER_FULL,
> prefix, argv);
>
> + /*
> + * Porcelain formats only look at the --[no-]ahead-behind command
> + * line argument and DO NOT look at the config setting. Non-porcelain
> + * formats use both.
> + */
nit: No need to shout: s/DO NOT/do not/
> + if (status_format == STATUS_FORMAT_PORCELAIN ||
> + status_format == STATUS_FORMAT_PORCELAIN_V2) {
> + if (ahead_behind_opt < 0)
> + ahead_behind_opt = ABF_FULL;
> + } else {
> + if (ahead_behind_opt < 0)
> + ahead_behind_opt = core_ahead_behind;
> + }
Can be more concise, to save the reader some time if they don't care
about the defaulting behavior:
if (ahead_behind_opt == -1) {
if (status_format == ...)
ahead_behind_opt = ...;
else
ahead_behind_opt = ...;
}
}
> + s.ab_flags = ((ahead_behind_opt) ? ABF_FULL : ABF_QUICK);
nit: both parens here are unnecessary and don't make the code clearer
[...]
> --- a/t/t7064-wtstatus-pv2.sh
> +++ b/t/t7064-wtstatus-pv2.sh
> @@ -390,6 +390,66 @@ test_expect_success 'verify upstream fields in branch
> header' '
> )
> '
>
> +test_expect_success 'verify --no-ahead-behind generates branch.qab' '
> + git checkout master &&
> + test_when_finished "rm -rf sub_repo" &&
> + git clone . sub_repo &&
> + (
> + ## Confirm local master tracks remote master.
> + cd sub_repo &&
> + HUF=$(git rev-parse HEAD) &&
> +
> + cat >expect <<-EOF &&
[...]
This looks like a collection of multiple tests. Is there a
straightforward way to split them into multiple independent
test_expect_successes?
That way, it's easier to tell which failed if there is a regression
later and to run only one of them (using GIT_SKIP_TESTS) when
debugging such a failure.
Thanks,
Jonathan
