Re: [PATCH v2 3/5] status: add --[no-]ahead-behind to porcelain V2 output

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 21, 2017, 20:57 UTC
Message-ID: <20171221205749.GC58971@aiede.mtv.corp.google.com>
In-Reply-To: <20171221190909.62995-4-git@jeffhostetler.com>

Hi,

Jeff Hostetler wrote:

Show 7 quoted lines

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

[...]

Show 7 quoted lines

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

[...]

Show 9 quoted lines

> @@ -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/

Show 8 quoted lines

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

[...]

Show 16 quoted lines

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