From: Denton Liu <liu.denton@gmail.com>
To: Git Mailing List <git@vger.kernel.org>
Cc: Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>
Subject: [PATCH v4 00/10] builtin/diff: learn --merge-base
Date: Sun, 20 Sep 2020 04:22:17 -0700 [thread overview]
Message-ID: <cover.1600600823.git.liu.denton@gmail.com> (raw)
In-Reply-To: <cover.1600328335.git.liu.denton@gmail.com>
The range-notation in `git diff` has been cited as a mistake since diff
compares two endpoints, not whole ranges.[0] In fact, the ranges seem
to take on the opposite meanings when compared to range notation in
`git log`.
In an effort to reduce the use of range notation as much as possible,
introduce the `--merge-base` flag, slightly modified from a suggestion
by Jonathan Nieder.[1] This flag allows us to replace the first commit
given on the command-line with its merge base between the first and
second commits.
One additional bonus is that this flag allows the "after" side to be not
just constrained to a commit (like with `...` notation). It can now be
the working tree or the index as well.
The `--merge-base` name isn't very satisfying. If anyone has any better
suggestions, please let me know.
Changes since v1:
* Implement Junio's documentation suggestions
* Update git diff's usage to include this option
Changes since v2:
* Teach diff-index and diff-tree the --merge-base option as well
Changes since v3:
* Don't use bitfields directly; extract them to an intermediate variable
* Use explicit literals in diff_get_merge_base() for clarity
* Fix xstrdup() leak
* I misunderstood and thought we wanted to deprecate `...` notation;
this is not the case so remove all references to "gentle deprecation"
and don't remove usage text that references it
[0]: https://lore.kernel.org/git/xmqqy2v26hu0.fsf@gitster-ct.c.googlers.com/
[1]: https://lore.kernel.org/git/20191223215928.GB38316@google.com/
Denton Liu (10):
t4068: remove unnecessary >tmp
git-diff-index.txt: make --cached description a proper sentence
git-diff.txt: backtick quote command text
contrib/completion: extract common diff/difftool options
diff-lib: accept option flags in run_diff_index()
diff-lib: define diff_get_merge_base()
t4068: add --merge-base tests
builtin/diff-index: learn --merge-base
builtin/diff-tree: learn --merge-base
contrib/completion: complete `git diff --merge-base`
Documentation/git-diff-index.txt | 9 +-
Documentation/git-diff-tree.txt | 7 +-
Documentation/git-diff.txt | 36 +++--
builtin/diff-index.c | 10 +-
builtin/diff-tree.c | 18 +++
builtin/diff.c | 49 +++++--
contrib/completion/git-completion.bash | 15 +-
diff-lib.c | 63 +++++++-
diff.h | 7 +-
t/t4068-diff-symmetric-merge-base.sh | 193 +++++++++++++++++++++++++
t/t4068-diff-symmetric.sh | 91 ------------
11 files changed, 358 insertions(+), 140 deletions(-)
create mode 100755 t/t4068-diff-symmetric-merge-base.sh
delete mode 100755 t/t4068-diff-symmetric.sh
Range-diff against v3:
1: 80e9066a59 = 1: 80e9066a59 t4068: remove unnecessary >tmp
2: 21b20281e6 = 2: 21b20281e6 git-diff-index.txt: make --cached description a proper sentence
3: ca9568c2ea = 3: ca9568c2ea git-diff.txt: backtick quote command text
4: 1ac8459541 = 4: 1ac8459541 contrib/completion: extract common diff/difftool options
5: 496908ac10 ! 5: 99d8b51585 diff-lib: accept option flags in run_diff_index()
@@ diff-lib.c: static int diff_cache(struct rev_info *revs,
+int run_diff_index(struct rev_info *revs, unsigned int option)
{
struct object_array_entry *ent;
++ int cached = !!(option & DIFF_INDEX_CACHED);
-@@ diff-lib.c: int run_diff_index(struct rev_info *revs, int cached)
-
- trace_performance_enter();
- ent = revs->pending.objects;
-- if (diff_cache(revs, &ent->item->oid, ent->name, cached))
-+ if (diff_cache(revs, &ent->item->oid, ent->name, !!(option & DIFF_INDEX_CACHED)))
- exit(128);
-
-- diff_set_mnemonic_prefix(&revs->diffopt, "c/", cached ? "i/" : "w/");
-+ diff_set_mnemonic_prefix(&revs->diffopt, "c/", (option & DIFF_INDEX_CACHED) ? "i/" : "w/");
- diffcore_fix_diff_index();
- diffcore_std(&revs->diffopt);
- diff_flush(&revs->diffopt);
+ if (revs->pending.nr != 1)
+ BUG("run_diff_index must be passed exactly one tree");
## diff.h ##
@@ diff.h: const char *diff_aligned_abbrev(const struct object_id *sha1, int);
@@ diff.h: const char *diff_aligned_abbrev(const struct object_id *sha1, int);
#define DIFF_RACY_IS_MODIFIED 02
int run_diff_files(struct rev_info *revs, unsigned int option);
-int run_diff_index(struct rev_info *revs, int cached);
++
+#define DIFF_INDEX_CACHED 01
+int run_diff_index(struct rev_info *revs, unsigned int option);
6: 6aac57ca02 ! 6: 3287e649f1 diff-lib: define diff_get_merge_base()
@@ diff-lib.c: static int diff_cache(struct rev_info *revs,
+ * ranges produce three pending commits, resulting in a
+ * misleading error message.
+ */
-+ if (revs->pending.nr > ARRAY_SIZE(mb_child))
-+ die(_("--merge-base does not work with more than two commits"));
++ if (revs->pending.nr < 1 || revs->pending.nr > 2)
++ BUG("unexpected revs->pending.nr: %d", revs->pending.nr);
+
+ for (i = 0; i < revs->pending.nr; i++)
+ mb_child[i] = lookup_commit_reference(the_repository, &revs->pending.objects[i].item->oid);
-+ if (revs->pending.nr < ARRAY_SIZE(mb_child)) {
++ if (revs->pending.nr == 1) {
+ struct object_id oid;
+
-+ if (revs->pending.nr != 1)
-+ BUG("unexpected revs->pending.nr: %d", revs->pending.nr);
-+
+ if (get_oid("HEAD", &oid))
+ die(_("unable to get HEAD"));
+
7: c9225a0440 = 7: 27930d4476 t4068: add --merge-base tests
8: 1e4f805e57 ! 8: f54baa4ecd builtin/diff-index: learn --merge-base
@@ builtin/diff.c: static int builtin_diff_index(struct rev_info *revs,
argv++; argc--;
## diff-lib.c ##
-@@ diff-lib.c: void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)
- int run_diff_index(struct rev_info *revs, unsigned int option)
+@@ diff-lib.c: int run_diff_index(struct rev_info *revs, unsigned int option)
{
struct object_array_entry *ent;
+ int cached = !!(option & DIFF_INDEX_CACHED);
++ int merge_base = !!(option & DIFF_INDEX_MERGE_BASE);
+ struct object_id oid;
+ const char *name;
++ char merge_base_hex[GIT_MAX_HEXSZ + 1];
if (revs->pending.nr != 1)
BUG("run_diff_index must be passed exactly one tree");
trace_performance_enter();
ent = revs->pending.objects;
-- if (diff_cache(revs, &ent->item->oid, ent->name, !!(option & DIFF_INDEX_CACHED)))
+- if (diff_cache(revs, &ent->item->oid, ent->name, cached))
+
-+ if (option & DIFF_INDEX_MERGE_BASE) {
++ if (merge_base) {
+ diff_get_merge_base(revs, &oid);
-+ name = xstrdup(oid_to_hex(&oid));
++ name = oid_to_hex_r(merge_base_hex, &oid);
+ } else {
+ oidcpy(&oid, &ent->item->oid);
+ name = ent->name;
+ }
+
-+ if (diff_cache(revs, &oid, name, !!(option & DIFF_INDEX_CACHED)))
++ if (diff_cache(revs, &oid, name, cached))
exit(128);
- diff_set_mnemonic_prefix(&revs->diffopt, "c/", (option & DIFF_INDEX_CACHED) ? "i/" : "w/");
+ diff_set_mnemonic_prefix(&revs->diffopt, "c/", cached ? "i/" : "w/");
## diff.h ##
@@ diff.h: void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);
- #define DIFF_RACY_IS_MODIFIED 02
int run_diff_files(struct rev_info *revs, unsigned int option);
+
#define DIFF_INDEX_CACHED 01
+#define DIFF_INDEX_MERGE_BASE 02
int run_diff_index(struct rev_info *revs, unsigned int option);
9: c0d27b125e ! 9: 4880d21119 builtin/diff-tree: learn --merge-base
@@ Metadata
## Commit message ##
builtin/diff-tree: learn --merge-base
- In order to get the diff between a commit and its merge base, the
- currently preferred method is to use `git diff A...B`. However, the
- range-notation with diff has, time and time again, been noted as a point
- of confusion and thus, it should be avoided. Although we have a
- substitute for the double-dot notation, we don't have any replacement
- for the triple-dot notation.
+ The previous commit introduced ---merge-base a way to take the diff
+ between the working tree or index and the merge base between an arbitrary
+ commit and HEAD. It makes sense to extend this option to support the
+ case where two commits are given too and behave in a manner identical to
+ `git diff A...B`.
- Introduce the --merge-base flag as a replacement for triple-dot
+ Introduce the --merge-base flag as an alternative to triple-dot
notation. Thus, we would be able to write the above as
- `git diff --merge-base A B`, allowing us to gently deprecate
- range-notation completely.
+ `git diff --merge-base A B`.
## Documentation/git-diff-tree.txt ##
@@ Documentation/git-diff-tree.txt: SYNOPSIS
@@ Documentation/git-diff.txt: SYNOPSIS
'git diff' [<options>] [<commit>] [--] [<path>...]
'git diff' [<options>] --cached [--merge-base] [<commit>] [--] [<path>...]
-'git diff' [<options>] <commit> [<commit>...] <commit> [--] [<path>...]
--'git diff' [<options>] <commit>...<commit> [--] [<path>...]
+'git diff' [<options>] [--merge-base] <commit> [<commit>...] <commit> [--] [<path>...]
+ 'git diff' [<options>] <commit>...<commit> [--] [<path>...]
'git diff' [<options>] <blob> <blob>
'git diff' [<options>] --no-index [--] <path> <path>
-
@@ Documentation/git-diff.txt: of <commit> and HEAD. `git diff --merge-base A` is equivalent to
branch name to compare with the tip of a different
branch.
@@ builtin/diff.c
"git diff [<options>] [<commit>] [--] [<path>...]\n"
" or: git diff [<options>] --cached [<commit>] [--] [<path>...]\n"
-" or: git diff [<options>] <commit> [<commit>...] <commit> [--] [<path>...]\n"
--" or: git diff [<options>] <commit>...<commit>] [--] [<path>...]\n"
+" or: git diff [<options>] <commit> [--merge-base] [<commit>...] <commit> [--] [<path>...]\n"
+ " or: git diff [<options>] <commit>...<commit>] [--] [<path>...]\n"
" or: git diff [<options>] <blob> <blob>]\n"
" or: git diff [<options>] --no-index [--] <path> <path>]\n"
- COMMON_DIFF_OPTIONS_HELP;
@@ builtin/diff.c: static int builtin_diff_tree(struct rev_info *revs,
struct object_array_entry *ent1)
{
10: 42a8c9b665 = 10: 32e3e52b5f contrib/completion: complete `git diff --merge-base`
--
2.28.0.760.g8d73e04208
next prev parent reply other threads:[~2020-09-20 11:22 UTC|newest]
Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-05 19:08 [PATCH 0/4] builtin/diff: learn --merge-base Denton Liu
2020-09-05 19:08 ` [PATCH 1/4] t4068: remove unnecessary >tmp Denton Liu
2020-09-05 19:08 ` [PATCH 2/4] git-diff.txt: backtick quote command text Denton Liu
2020-09-05 19:08 ` [PATCH 3/4] builtin/diff: parse --no-index using parse_options() Denton Liu
2020-09-05 19:08 ` [PATCH 4/4] builtin/diff: learn --merge-base Denton Liu
2020-09-06 21:58 ` Junio C Hamano
2020-09-10 7:32 ` [PATCH v2 0/4] " Denton Liu
2020-09-10 7:32 ` [PATCH v2 1/4] t4068: remove unnecessary >tmp Denton Liu
2020-09-10 7:32 ` [PATCH v2 2/4] git-diff.txt: backtick quote command text Denton Liu
2020-09-10 7:32 ` [PATCH v2 3/4] builtin/diff: parse --no-index using parse_options() Denton Liu
2020-09-10 18:35 ` Junio C Hamano
2020-09-13 8:31 ` Denton Liu
2020-09-13 21:45 ` Junio C Hamano
2020-09-10 7:32 ` [PATCH v2 4/4] builtin/diff: learn --merge-base Denton Liu
2020-09-17 7:44 ` [PATCH v3 00/10] " Denton Liu
2020-09-17 7:44 ` [PATCH v3 01/10] t4068: remove unnecessary >tmp Denton Liu
2020-09-17 7:44 ` [PATCH v3 02/10] git-diff-index.txt: make --cached description a proper sentence Denton Liu
2020-09-17 7:44 ` [PATCH v3 03/10] git-diff.txt: backtick quote command text Denton Liu
2020-09-17 7:44 ` [PATCH v3 04/10] contrib/completion: extract common diff/difftool options Denton Liu
2020-09-17 7:44 ` [PATCH v3 05/10] diff-lib: accept option flags in run_diff_index() Denton Liu
2020-09-17 17:00 ` Junio C Hamano
2020-09-17 7:44 ` [PATCH v3 06/10] diff-lib: define diff_get_merge_base() Denton Liu
2020-09-17 17:16 ` Junio C Hamano
2020-09-18 10:34 ` Denton Liu
2020-09-19 0:33 ` Junio C Hamano
2020-09-17 7:44 ` [PATCH v3 07/10] t4068: add --merge-base tests Denton Liu
2020-09-17 7:44 ` [PATCH v3 08/10] builtin/diff-index: learn --merge-base Denton Liu
2020-09-17 17:28 ` Junio C Hamano
2020-09-17 18:13 ` Jeff King
2020-09-18 5:11 ` Junio C Hamano
2020-09-18 18:12 ` Jeff King
2020-09-17 7:44 ` [PATCH v3 09/10] builtin/diff-tree: " Denton Liu
2020-09-17 18:23 ` Junio C Hamano
2020-09-18 10:48 ` Denton Liu
2020-09-18 16:52 ` Junio C Hamano
2020-09-20 11:01 ` Denton Liu
2020-09-21 16:05 ` Junio C Hamano
2020-09-21 17:27 ` Denton Liu
2020-09-21 21:09 ` Junio C Hamano
2020-09-21 21:19 ` Junio C Hamano
2020-09-21 21:54 ` Denton Liu
2020-09-21 22:18 ` Junio C Hamano
2020-09-23 9:47 ` Denton Liu
2020-09-25 21:02 ` Junio C Hamano
2020-09-26 1:52 ` Denton Liu
2020-09-17 7:44 ` [PATCH v3 10/10] contrib/completion: complete `git diff --merge-base` Denton Liu
2020-09-20 11:22 ` Denton Liu [this message]
2020-09-20 11:22 ` [PATCH v4 01/10] t4068: remove unnecessary >tmp Denton Liu
2020-09-20 11:22 ` [PATCH v4 02/10] git-diff-index.txt: make --cached description a proper sentence Denton Liu
2020-09-20 11:22 ` [PATCH v4 03/10] git-diff.txt: backtick quote command text Denton Liu
2020-09-20 11:22 ` [PATCH v4 04/10] contrib/completion: extract common diff/difftool options Denton Liu
2020-09-20 11:22 ` [PATCH v4 05/10] diff-lib: accept option flags in run_diff_index() Denton Liu
2020-09-20 11:22 ` [PATCH v4 06/10] diff-lib: define diff_get_merge_base() Denton Liu
2020-09-20 11:22 ` [PATCH v4 07/10] t4068: add --merge-base tests Denton Liu
2020-09-20 11:22 ` [PATCH v4 08/10] builtin/diff-index: learn --merge-base Denton Liu
2020-09-29 19:53 ` Martin Ågren
2020-09-20 11:22 ` [PATCH v4 09/10] builtin/diff-tree: " Denton Liu
2020-09-20 11:22 ` [PATCH v4 10/10] contrib/completion: complete `git diff --merge-base` Denton Liu
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=cover.1600600823.git.liu.denton@gmail.com \
--to=liu.denton@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.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.
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.