From: Jeff King <peff@peff.net>
To: Jack Bates <bk874k@nottheoilrig.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
Jack Bates <jack@nottheoilrig.com>
Subject: Re: [PATCH v2] diff: handle --no-abbrev outside of repository
Date: Mon, 5 Dec 2016 01:01:16 -0500 [thread overview]
Message-ID: <20161205060116.szy5ojetg3znu4w7@sigill.intra.peff.net> (raw)
In-Reply-To: <20161202184840.2158-1-jack@nottheoilrig.com>
On Fri, Dec 02, 2016 at 11:48:40AM -0700, Jack Bates wrote:
> The "git diff --no-index" codepath didn't handle the --no-abbrev option.
> Also it didn't behave the same as find_unique_abbrev()
> in the case where abbrev == 0.
> find_unique_abbrev() returns the full, unabbreviated string in that
> case, but the "git diff --no-index" codepath returned an empty string.
If you've dug into what's wrong, I think it's often good to add some
notes in the commit message in case somebody has to revisit this later.
For example, I'd have written something like:
The "git diff --no-index" codepath doesn't handle the --no-abbrev
option, because it relies on diff_opt_parse(). Normally that function
is called as part of handle_revision_opt(), which handles the abbrev
options itself. Adding the option directly to diff_opt_parse() fixes
this. We don't need to do the same for --abbrev, because it's already
handled there.
Note that setting abbrev to "0" outside of a repository was broken
recently by 4f03666ac (diff: handle sha1 abbreviations outside of
repository, 2016-10-20). It adds a special out-of-repo code path for
handling abbreviations which behaves differently than find_unique_abbrev()
by truly giving a zero-length sha1, rather than taking "0" to mean "do
not abbreviate".
That bug was not triggerable until now, because there was no way to
set the value to zero (using --abbrev=0 silently bumps it to the
MINIMUM_ABBREV).
> t/t4013-diff-various.sh | 7 +++++++
> t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir | 3 +++
> t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir | 3 +++
> t/t4013/diff.diff_--no-index_--raw_dir2_dir | 3 +++
> t/t4013/diff.diff_--raw_--abbrev=4_initial | 6 ++++++
> t/t4013/diff.diff_--raw_--no-abbrev_initial | 6 ++++++
> t/t4013/diff.diff_--raw_initial | 6 ++++++
I wondered if the tests without --no-index were redundant with earlier
ones, but I don't think so. --abbrev=4 is tested with diff-tree, but
--no-abbrev is not covered at all, AFAICT.
> diff.c | 6 +++++-
The actual code changes look good to me.
Thanks.
-Peff
next prev parent reply other threads:[~2016-12-05 6:02 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-28 18:25 [PATCH] diff: handle --no-abbrev outside of repository Jack Bates
2016-11-28 23:03 ` Junio C Hamano
2016-11-29 7:06 ` Jeff King
2016-12-02 18:48 ` [PATCH v2] " Jack Bates
2016-12-05 6:01 ` Jeff King [this message]
2016-12-05 6:15 ` Jeff King
2016-12-05 6:58 ` Jeff King
2016-12-06 1:01 ` [PATCH v3] diff: handle --no-abbrev in no-index case Jack Bates
2016-12-06 16:53 ` [PATCH v4] " Jack Bates
2016-12-06 16:56 ` Jack Bates
2016-12-06 17:00 ` Jack Bates
2016-12-08 22:53 ` Junio C Hamano
2016-12-09 0:22 ` Jack Bates
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20161205060116.szy5ojetg3znu4w7@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=bk874k@nottheoilrig.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jack@nottheoilrig.com \
/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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.