From: Jeff Hostetler <git@jeffhostetler.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, peff@peff.net,
Jeff Hostetler <jeffhost@microsoft.com>
Subject: Re: [PATCH v3 2/5] status: add --[no-]ahead-behind to status and commit for V2 format.
Date: Fri, 5 Jan 2018 11:31:08 -0500 [thread overview]
Message-ID: <6fe41395-6cb5-cbfe-88cd-5b0b21fad7b8@jeffhostetler.com> (raw)
In-Reply-To: <xmqqlghde0uy.fsf@gitster.mtv.corp.google.com>
On 1/4/2018 5:05 PM, Junio C Hamano wrote:
> Jeff Hostetler <git@jeffhostetler.com> writes:
>
>> + sti = stat_tracking_info(branch, &nr_ahead, &nr_behind,
>> + &base, s->ahead_behind_flags);
>> if (base) {
>> base = shorten_unambiguous_ref(base, 0);
>> fprintf(s->fp, "# branch.upstream %s%c", base, eol);
>> free((char *)base);
>>
>> - if (ab_info)
>> - fprintf(s->fp, "# branch.ab +%d -%d%c", nr_ahead, nr_behind, eol);
>> + if (sti >= 0) {
>> + if (s->ahead_behind_flags == AHEAD_BEHIND_FULL)
>> + fprintf(s->fp, "# branch.ab +%d -%d%c",
>> + nr_ahead, nr_behind, eol);
>> + else if (s->ahead_behind_flags == AHEAD_BEHIND_QUICK)
>> + fprintf(s->fp, "# branch.equal %s%c",
>> + sti ? "neq" : "eq", eol);
>
> This is related to what I said in the review of [1/5], but this
> arrangement to require the caller to know that _QUICK request
> results in incomplete information smells like a bit of maintenance
> hassle.
>
> We'd rather allow the caller to tell if it was given incomplete
> information from the values returned by stat_tracking_info()
> function (notice the plural "values" here; what is returned in
> nr_{ahead,behind} also counts). For example, we can keep the (-1 =>
> "no relation", 0 => "identical", 1 => "different") as return values
> of the function, but have it clear nr_{ahead,behind} when it only
> knows the two are different but not by how many commits. That way,
> this step can instead do:
>
> ab_info = stat_tracking_info(...);
> if (base) {
> ... do the base thing ...
> if (ab_info > 0) {
> /* different */
> if (nr_ahead || nr_behind)
> fprintf(... +%d -%d ... nr_ahead, nr_behind, ...);
> else
> fprintf(... "neq" ...);
> } else if (!ab_info) {
> /* same */
> fprintf(... +%d -%d ... nr_ahead, nr_behind, ...);
> }
> ...
>
> That would allow us to later add different kinds of _QUICK that do
> not give full information without having to update this consumer
> with something like
>
> - else if (s->ahead_behind_flags == AHEAD_BEHIND_QUICK)
> + else if (s->ahead_behind_flags == AHEAD_BEHIND_QUICK ||
> + s->ahead_behind_flags == AHEAD_BEHIND_QUICK_DIFFERENTLY)
>
> when it happens.
>
> Two related tangents.
>
> - I do not see why "eq" need to exist at all. Even in _QUICK mode,
> when you determine that two are identical, you know your branch
> is ahead and behind by 0 commit each.
>
> - A short-format that is easy to parse by machines does not have to
> be cryptic to humans. s/neq/not-equal/ or something may be an
> improvement.
>
> Thanks.
>
Thanks for the review. Let me update it along the lines suggested here.
It felt odd to define the nr_{ahead,behind} as undefined, but short of
making them negative or something I wasn't sure what was best.
Thanks,
Jeff
next prev parent reply other threads:[~2018-01-05 16:31 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-03 21:47 [PATCH v3 0/5] Add --no-ahead-behind to status Jeff Hostetler
2018-01-03 21:47 ` [PATCH v3 1/5] stat_tracking_info: return +1 when branches not equal Jeff Hostetler
2018-01-04 21:41 ` Junio C Hamano
2018-01-03 21:47 ` [PATCH v3 2/5] status: add --[no-]ahead-behind to status and commit for V2 format Jeff Hostetler
2018-01-04 22:05 ` Junio C Hamano
2018-01-05 16:31 ` Jeff Hostetler [this message]
2018-01-03 21:47 ` [PATCH v3 3/5] status: update short status to respect --no-ahead-behind Jeff Hostetler
2018-01-03 21:47 ` [PATCH v3 4/5] status: support --no-ahead-behind in long format Jeff Hostetler
2018-01-03 21:47 ` [PATCH v3 5/5] status: add status.aheadBehind value for porcelain output Jeff Hostetler
2018-01-04 23:06 ` [PATCH v3 0/5] Add --no-ahead-behind to status Jeff King
2018-01-05 16:46 ` Jeff Hostetler
2018-01-05 19:56 ` Junio C Hamano
2018-01-08 6:37 ` Jeff King
2018-01-08 14:22 ` Jeff Hostetler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6fe41395-6cb5-cbfe-88cd-5b0b21fad7b8@jeffhostetler.com \
--to=git@jeffhostetler.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jeffhost@microsoft.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).