* [PATCH v3 01/10] t4068: remove unnecessary >tmp
2020-09-17 7:44 ` [PATCH v3 00/10] " Denton Liu
@ 2020-09-17 7:44 ` Denton Liu
2020-09-17 7:44 ` [PATCH v3 02/10] git-diff-index.txt: make --cached description a proper sentence Denton Liu
` (9 subsequent siblings)
10 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-17 7:44 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano
The many `git diff` invocations have a `>tmp` redirection even though
the file is not being used afterwards. Remove these unnecessary
redirections.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
t/t4068-diff-symmetric.sh | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric.sh
index 31d17a5af0..60c506c2b2 100755
--- a/t/t4068-diff-symmetric.sh
+++ b/t/t4068-diff-symmetric.sh
@@ -64,27 +64,27 @@ test_expect_success 'diff with two merge bases' '
'
test_expect_success 'diff with no merge bases' '
- test_must_fail git diff br2...br3 >tmp 2>err &&
+ test_must_fail git diff br2...br3 2>err &&
test_i18ngrep "fatal: br2...br3: no merge base" err
'
test_expect_success 'diff with too many symmetric differences' '
- test_must_fail git diff br1...master br2...br3 >tmp 2>err &&
+ test_must_fail git diff br1...master br2...br3 2>err &&
test_i18ngrep "usage" err
'
test_expect_success 'diff with symmetric difference and extraneous arg' '
- test_must_fail git diff master br1...master >tmp 2>err &&
+ test_must_fail git diff master br1...master 2>err &&
test_i18ngrep "usage" err
'
test_expect_success 'diff with two ranges' '
- test_must_fail git diff master br1..master br2..br3 >tmp 2>err &&
+ test_must_fail git diff master br1..master br2..br3 2>err &&
test_i18ngrep "usage" err
'
test_expect_success 'diff with ranges and extra arg' '
- test_must_fail git diff master br1..master commit-D >tmp 2>err &&
+ test_must_fail git diff master br1..master commit-D 2>err &&
test_i18ngrep "usage" err
'
--
2.28.0.618.gf4bc123cb7
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH v3 02/10] git-diff-index.txt: make --cached description a proper sentence
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 ` Denton Liu
2020-09-17 7:44 ` [PATCH v3 03/10] git-diff.txt: backtick quote command text Denton Liu
` (8 subsequent siblings)
10 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-17 7:44 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Documentation/git-diff-index.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/git-diff-index.txt b/Documentation/git-diff-index.txt
index f4bd8155c0..25fe165f00 100644
--- a/Documentation/git-diff-index.txt
+++ b/Documentation/git-diff-index.txt
@@ -27,7 +27,7 @@ include::diff-options.txt[]
The id of a tree object to diff against.
--cached::
- do not consider the on-disk file at all
+ Do not consider the on-disk file at all.
-m::
By default, files recorded in the index but not checked
--
2.28.0.618.gf4bc123cb7
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH v3 03/10] git-diff.txt: backtick quote command text
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 ` Denton Liu
2020-09-17 7:44 ` [PATCH v3 04/10] contrib/completion: extract common diff/difftool options Denton Liu
` (7 subsequent siblings)
10 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-17 7:44 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano
The modern way to quote commands in the documentation is to use
backticks instead of double-quotes as this renders the text with the
code style. Convert double-quoted command text to backtick-quoted
commands. While we're at it, quote one instance of `^@`.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Documentation/git-diff.txt | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 727f24d16e..8f7b4ed3ca 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -68,13 +68,13 @@ files on disk.
This form is to view the results of a merge commit. The first
listed <commit> must be the merge itself; the remaining two or
more commits should be its parents. A convenient way to produce
- the desired set of revisions is to use the {caret}@ suffix.
+ the desired set of revisions is to use the `^@` suffix.
For instance, if `master` names a merge commit, `git diff master
master^@` gives the same combined diff as `git show master`.
'git diff' [<options>] <commit>..<commit> [--] [<path>...]::
- This is synonymous to the earlier form (without the "..") for
+ This is synonymous to the earlier form (without the `..`) for
viewing the changes between two arbitrary <commit>. If <commit> on
one side is omitted, it will have the same effect as
using HEAD instead.
@@ -83,20 +83,20 @@ files on disk.
This form is to view the changes on the branch containing
and up to the second <commit>, starting at a common ancestor
- of both <commit>. "git diff A\...B" is equivalent to
- "git diff $(git merge-base A B) B". You can omit any one
+ of both <commit>. `git diff A...B` is equivalent to
+ `git diff $(git merge-base A B) B`. You can omit any one
of <commit>, which has the same effect as using HEAD instead.
Just in case you are doing something exotic, it should be
noted that all of the <commit> in the above description, except
-in the last two forms that use ".." notations, can be any
+in the last two forms that use `..` notations, can be any
<tree>.
For a more complete list of ways to spell <commit>, see
"SPECIFYING REVISIONS" section in linkgit:gitrevisions[7].
However, "diff" is about comparing two _endpoints_, not ranges,
-and the range notations ("<commit>..<commit>" and
-"<commit>\...<commit>") do not mean a range as defined in the
+and the range notations (`<commit>..<commit>` and
+`<commit>...<commit>`) do not mean a range as defined in the
"SPECIFYING RANGES" section in linkgit:gitrevisions[7].
'git diff' [<options>] <blob> <blob>::
@@ -144,9 +144,9 @@ $ git diff HEAD <3>
+
<1> Changes in the working tree not yet staged for the next commit.
<2> Changes between the index and your last commit; what you
- would be committing if you run "git commit" without "-a" option.
+ would be committing if you run `git commit` without `-a` option.
<3> Changes in the working tree since your last commit; what you
- would be committing if you run "git commit -a"
+ would be committing if you run `git commit -a`
Comparing with arbitrary commits::
+
--
2.28.0.618.gf4bc123cb7
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH v3 04/10] contrib/completion: extract common diff/difftool options
2020-09-17 7:44 ` [PATCH v3 00/10] " Denton Liu
` (2 preceding siblings ...)
2020-09-17 7:44 ` [PATCH v3 03/10] git-diff.txt: backtick quote command text Denton Liu
@ 2020-09-17 7:44 ` Denton Liu
2020-09-17 7:44 ` [PATCH v3 05/10] diff-lib: accept option flags in run_diff_index() Denton Liu
` (6 subsequent siblings)
10 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-17 7:44 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano
difftool parses its own options and then passes the remaining options
onto diff. As a result, they share common command-line options. Instead
of duplicating the list, use a shared $__git_diff_difftool_options list.
The completion for diff is missing --relative and the completion for
difftool is missing --no-index. Add both of these to the common list.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
contrib/completion/git-completion.bash | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 9147fba3d5..f68c8e0646 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1691,6 +1691,10 @@ __git_diff_common_options="--stat --numstat --shortstat --summary
--textconv --no-textconv
"
+__git_diff_difftool_options="--cached --staged --pickaxe-all --pickaxe-regex
+ --base --ours --theirs --no-index --relative
+ $__git_diff_common_options"
+
_git_diff ()
{
__git_has_doubledash && return
@@ -1713,10 +1717,7 @@ _git_diff ()
return
;;
--*)
- __gitcomp "--cached --staged --pickaxe-all --pickaxe-regex
- --base --ours --theirs --no-index
- $__git_diff_common_options
- "
+ __gitcomp "$__git_diff_difftool_options"
return
;;
esac
@@ -1738,11 +1739,7 @@ _git_difftool ()
return
;;
--*)
- __gitcomp_builtin difftool "$__git_diff_common_options
- --base --cached --ours --theirs
- --pickaxe-all --pickaxe-regex
- --relative --staged
- "
+ __gitcomp_builtin difftool "$__git_diff_difftool_options"
return
;;
esac
--
2.28.0.618.gf4bc123cb7
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH v3 05/10] diff-lib: accept option flags in run_diff_index()
2020-09-17 7:44 ` [PATCH v3 00/10] " Denton Liu
` (3 preceding siblings ...)
2020-09-17 7:44 ` [PATCH v3 04/10] contrib/completion: extract common diff/difftool options Denton Liu
@ 2020-09-17 7:44 ` 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
` (5 subsequent siblings)
10 siblings, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-17 7:44 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano
In a future commit, we will teach run_diff_index() to accept more
options via flag bits. For now, change `cached` into a flag in the
`option` bitfield. The behaviour should remain exactly the same.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
builtin/diff-index.c | 8 ++++----
builtin/diff.c | 8 ++++----
diff-lib.c | 6 +++---
diff.h | 3 ++-
4 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 93ec642423..c3878f7ad6 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -15,7 +15,7 @@ COMMON_DIFF_OPTIONS_HELP;
int cmd_diff_index(int argc, const char **argv, const char *prefix)
{
struct rev_info rev;
- int cached = 0;
+ unsigned int option = 0;
int i;
int result;
@@ -32,7 +32,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
const char *arg = argv[i];
if (!strcmp(arg, "--cached"))
- cached = 1;
+ option |= DIFF_INDEX_CACHED;
else
usage(diff_cache_usage);
}
@@ -46,7 +46,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
if (rev.pending.nr != 1 ||
rev.max_count != -1 || rev.min_age != -1 || rev.max_age != -1)
usage(diff_cache_usage);
- if (!cached) {
+ if (!(option & DIFF_INDEX_CACHED)) {
setup_work_tree();
if (read_cache_preload(&rev.diffopt.pathspec) < 0) {
perror("read_cache_preload");
@@ -56,7 +56,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
perror("read_cache");
return -1;
}
- result = run_diff_index(&rev, cached);
+ result = run_diff_index(&rev, option);
UNLEAK(rev);
return diff_result_code(&rev.diffopt, result);
}
diff --git a/builtin/diff.c b/builtin/diff.c
index cb98811c21..e45e19e37e 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -134,11 +134,11 @@ static int builtin_diff_blobs(struct rev_info *revs,
static int builtin_diff_index(struct rev_info *revs,
int argc, const char **argv)
{
- int cached = 0;
+ unsigned int option = 0;
while (1 < argc) {
const char *arg = argv[1];
if (!strcmp(arg, "--cached") || !strcmp(arg, "--staged"))
- cached = 1;
+ option |= DIFF_INDEX_CACHED;
else
usage(builtin_diff_usage);
argv++; argc--;
@@ -151,7 +151,7 @@ static int builtin_diff_index(struct rev_info *revs,
revs->max_count != -1 || revs->min_age != -1 ||
revs->max_age != -1)
usage(builtin_diff_usage);
- if (!cached) {
+ if (!(option & DIFF_INDEX_CACHED)) {
setup_work_tree();
if (read_cache_preload(&revs->diffopt.pathspec) < 0) {
perror("read_cache_preload");
@@ -161,7 +161,7 @@ static int builtin_diff_index(struct rev_info *revs,
perror("read_cache");
return -1;
}
- return run_diff_index(revs, cached);
+ return run_diff_index(revs, option);
}
static int builtin_diff_tree(struct rev_info *revs,
diff --git a/diff-lib.c b/diff-lib.c
index 50521e2093..0a0e69113c 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -518,7 +518,7 @@ static int diff_cache(struct rev_info *revs,
return unpack_trees(1, &t, &opts);
}
-int run_diff_index(struct rev_info *revs, int cached)
+int run_diff_index(struct rev_info *revs, unsigned int option)
{
struct object_array_entry *ent;
@@ -527,10 +527,10 @@ 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);
diff --git a/diff.h b/diff.h
index e0c0af6286..0cc874f2d5 100644
--- a/diff.h
+++ b/diff.h
@@ -585,7 +585,8 @@ const char *diff_aligned_abbrev(const struct object_id *sha1, int);
/* report racily-clean paths as modified */
#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);
int do_diff_cache(const struct object_id *, struct diff_options *);
int diff_flush_patch_id(struct diff_options *, struct object_id *, int, int);
--
2.28.0.618.gf4bc123cb7
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH v3 05/10] diff-lib: accept option flags in run_diff_index()
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
0 siblings, 0 replies; 58+ messages in thread
From: Junio C Hamano @ 2020-09-17 17:00 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List
Denton Liu <liu.denton@gmail.com> writes:
> diff --git a/diff-lib.c b/diff-lib.c
> index 50521e2093..0a0e69113c 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -518,7 +518,7 @@ static int diff_cache(struct rev_info *revs,
> return unpack_trees(1, &t, &opts);
> }
>
> -int run_diff_index(struct rev_info *revs, int cached)
> +int run_diff_index(struct rev_info *revs, unsigned int option)
> {
> struct object_array_entry *ent;
If we introduce
int cached = !!(option & DIFF_INDEX_CACHED);
we do not have to touch the remainder of the function, and it makes
it easier to read the place(s) where 'cached' is used. At that
point, the fact that we were instructed by a bit in the option flag
word is much much less important than we were instructed to compare
the tree-ish with the index and not the working tree files.
The same technique is used with DIFF_RACY_IS_MODIFIED flag in
run_diff_files().
> diff --git a/diff.h b/diff.h
> index e0c0af6286..0cc874f2d5 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -585,7 +585,8 @@ const char *diff_aligned_abbrev(const struct object_id *sha1, int);
> /* report racily-clean paths as modified */
> #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);
It is unclear from the above if the option word for run_diff_files()
and run_diff_index() share the same bit assignment. After reading
the series through to the end, I know they are independent set of
bits and never shared, but I wonder if we can make it more obvious
here.
Perhaps an extra blank line before this new #define is sufficient to
make it clear?
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v3 06/10] diff-lib: define diff_get_merge_base()
2020-09-17 7:44 ` [PATCH v3 00/10] " Denton Liu
` (4 preceding siblings ...)
2020-09-17 7:44 ` [PATCH v3 05/10] diff-lib: accept option flags in run_diff_index() Denton Liu
@ 2020-09-17 7:44 ` Denton Liu
2020-09-17 17:16 ` Junio C Hamano
2020-09-17 7:44 ` [PATCH v3 07/10] t4068: add --merge-base tests Denton Liu
` (4 subsequent siblings)
10 siblings, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-17 7:44 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano
In a future commit, we will be using this function to implement
--merge-base functionality in various diff commands.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
diff-lib.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
diff.h | 2 ++
2 files changed, 50 insertions(+)
diff --git a/diff-lib.c b/diff-lib.c
index 0a0e69113c..e01c3f0612 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -13,6 +13,7 @@
#include "submodule.h"
#include "dir.h"
#include "fsmonitor.h"
+#include "commit-reach.h"
/*
* diff-files
@@ -518,6 +519,53 @@ static int diff_cache(struct rev_info *revs,
return unpack_trees(1, &t, &opts);
}
+void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)
+{
+ int i;
+ struct commit *mb_child[2] = {0};
+ struct commit_list *merge_bases;
+
+ for (i = 0; i < revs->pending.nr; i++) {
+ struct object *obj = revs->pending.objects[i].item;
+ if (obj->flags)
+ die(_("--merge-base does not work with ranges"));
+ if (obj->type != OBJ_COMMIT)
+ die(_("--merge-base only works with commits"));
+ }
+
+ /*
+ * This check must go after the for loop above because A...B
+ * 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"));
+
+ 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)) {
+ 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"));
+
+ mb_child[1] = lookup_commit_reference(the_repository, &oid);
+ }
+
+ merge_bases = repo_get_merge_bases(the_repository, mb_child[0], mb_child[1]);
+ if (!merge_bases)
+ die(_("no merge base found"));
+ if (merge_bases->next)
+ die(_("multiple merge bases found"));
+
+ oidcpy(mb, &merge_bases->item->object.oid);
+
+ free_commit_list(merge_bases);
+}
+
int run_diff_index(struct rev_info *revs, unsigned int option)
{
struct object_array_entry *ent;
diff --git a/diff.h b/diff.h
index 0cc874f2d5..ae2bb7001a 100644
--- a/diff.h
+++ b/diff.h
@@ -580,6 +580,8 @@ void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc);
*/
const char *diff_aligned_abbrev(const struct object_id *sha1, int);
+void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);
+
/* do not report anything on removed paths */
#define DIFF_SILENT_ON_REMOVED 01
/* report racily-clean paths as modified */
--
2.28.0.618.gf4bc123cb7
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH v3 06/10] diff-lib: define diff_get_merge_base()
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
0 siblings, 1 reply; 58+ messages in thread
From: Junio C Hamano @ 2020-09-17 17:16 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List
Denton Liu <liu.denton@gmail.com> writes:
> +void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)
> +{
> + int i;
> + struct commit *mb_child[2] = {0};
> + struct commit_list *merge_bases;
> +
> + for (i = 0; i < revs->pending.nr; i++) {
> + struct object *obj = revs->pending.objects[i].item;
> + if (obj->flags)
> + die(_("--merge-base does not work with ranges"));
> + if (obj->type != OBJ_COMMIT)
> + die(_("--merge-base only works with commits"));
> + }
This is the first use of die() in this file, that is designed to
keep a set of reusable library functions so that the caller(s) can
do their own die(). They may want to become
return error(_(...));
The same comment applies to all die()s the patch adds.
> + /*
> + * This check must go after the for loop above because A...B
> + * ranges produce three pending commits, resulting in a
> + * misleading error message.
> + */
Should "git diff --merge-base A...B" be forbidden, or does it just
ask the same thing twice and is not a die-worthy offence?
> + if (revs->pending.nr > ARRAY_SIZE(mb_child))
> + die(_("--merge-base does not work with more than two commits"));
Compare with hardcoded '2' in the condition. The requirement for
mb_child[] is that it can hold at least 2 (changes in the future may
find it convenient if its size gets increased to 3 to hold NULL sentinel
to signal end-of-list, for example). Comparison with ARRAY_SIZE() may
be appropriate in different situations but not here where the code knows
it wants to reject more than two no matter how big mb_child[] is.
> + 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)) {
> + struct object_id oid;
> +
> + if (revs->pending.nr != 1)
> + BUG("unexpected revs->pending.nr: %d", revs->pending.nr);
This is an obviously impossible condition as we will not take more
than 2.
> + if (get_oid("HEAD", &oid))
> + die(_("unable to get HEAD"));
> + mb_child[1] = lookup_commit_reference(the_repository, &oid);
> + }
> +
> + merge_bases = repo_get_merge_bases(the_repository, mb_child[0], mb_child[1]);
> + if (!merge_bases)
> + die(_("no merge base found"));
> + if (merge_bases->next)
> + die(_("multiple merge bases found"));
> +
> + oidcpy(mb, &merge_bases->item->object.oid);
> +
> + free_commit_list(merge_bases);
> +}
OK.
> int run_diff_index(struct rev_info *revs, unsigned int option)
> {
> struct object_array_entry *ent;
> diff --git a/diff.h b/diff.h
> index 0cc874f2d5..ae2bb7001a 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -580,6 +580,8 @@ void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc);
> */
> const char *diff_aligned_abbrev(const struct object_id *sha1, int);
>
> +void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);
Without an actual caller, we cannot judge if this interface is
designed right, but we'll see soon in the next steps ;-)
Looking good so far.
Thanks.
> +
> /* do not report anything on removed paths */
> #define DIFF_SILENT_ON_REMOVED 01
> /* report racily-clean paths as modified */
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 06/10] diff-lib: define diff_get_merge_base()
2020-09-17 17:16 ` Junio C Hamano
@ 2020-09-18 10:34 ` Denton Liu
2020-09-19 0:33 ` Junio C Hamano
0 siblings, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-18 10:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
Hi Junio,
On Thu, Sep 17, 2020 at 10:16:51AM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
>
> > +void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)
> > +{
> > + int i;
> > + struct commit *mb_child[2] = {0};
> > + struct commit_list *merge_bases;
> > +
> > + for (i = 0; i < revs->pending.nr; i++) {
> > + struct object *obj = revs->pending.objects[i].item;
> > + if (obj->flags)
> > + die(_("--merge-base does not work with ranges"));
> > + if (obj->type != OBJ_COMMIT)
> > + die(_("--merge-base only works with commits"));
> > + }
>
> This is the first use of die() in this file, that is designed to
> keep a set of reusable library functions so that the caller(s) can
> do their own die(). They may want to become
Although this is the first instance of die(), run_diff_index() has an
exit(128), which is basically a die() in disguise.
> return error(_(...));
>
> The same comment applies to all die()s the patch adds.
I applied this change but then each callsite of diff_get_merge_base()
became something like
if (diff_get_merge_base(revs, &oid))
exit(128);
so I do agree with the spirit of the change but in reality, it just
creates more busywork for the callers.
> > + /*
> > + * This check must go after the for loop above because A...B
> > + * ranges produce three pending commits, resulting in a
> > + * misleading error message.
> > + */
>
> Should "git diff --merge-base A...B" be forbidden, or does it just
> ask the same thing twice and is not a die-worthy offence?
I think that it should be die-worthy because it's a logic error for a
user to do this. I can't think of any situation where it wouldn't be
more desirable error early to correct a user's thinking. Plus, we're
trying to move away from the `...` notation anyway ;)
> > + 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)) {
> > + struct object_id oid;
> > +
> > + if (revs->pending.nr != 1)
> > + BUG("unexpected revs->pending.nr: %d", revs->pending.nr);
>
> This is an obviously impossible condition as we will not take more
> than 2.
We also want to ensure that revs->pending.nr isn't 0 here. That being
said, I can explicitly check earlier in the function that the number of
pending is 1 or 2 so that it's more clearly written.
Thanks,
Denton
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 06/10] diff-lib: define diff_get_merge_base()
2020-09-18 10:34 ` Denton Liu
@ 2020-09-19 0:33 ` Junio C Hamano
0 siblings, 0 replies; 58+ messages in thread
From: Junio C Hamano @ 2020-09-19 0:33 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List
Denton Liu <liu.denton@gmail.com> writes:
> became something like
>
> if (diff_get_merge_base(revs, &oid))
> exit(128);
>
> so I do agree with the spirit of the change but in reality, it just
> creates more busywork for the callers.
OK, then lets keep them as die()s.
> I think that it should be die-worthy because it's a logic error for a
> user to do this. I can't think of any situation where it wouldn't be
> more desirable error early to correct a user's thinking. Plus, we're
> trying to move away from the `...` notation anyway ;)
I do not think so. We are *NOT* trying to move away from A...B;
what is mistake is A..B and that is what we want to move away from.
Luckily, there is no need to introduce a new option there, because
the user can just stop typing .. and instead type SP.
The primary value of the new option you are adding is that it allows
us to compare the index and the working tree with the merge base.
The current A...B notation can only be used to compare two trees.
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v3 07/10] t4068: add --merge-base tests
2020-09-17 7:44 ` [PATCH v3 00/10] " Denton Liu
` (5 preceding siblings ...)
2020-09-17 7:44 ` [PATCH v3 06/10] diff-lib: define diff_get_merge_base() Denton Liu
@ 2020-09-17 7:44 ` Denton Liu
2020-09-17 7:44 ` [PATCH v3 08/10] builtin/diff-index: learn --merge-base Denton Liu
` (3 subsequent siblings)
10 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-17 7:44 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano
In the future, we will be adding more --merge-base tests to this test
script. To prepare for that, rename the script accordingly and update
its description. Also, add two basic --merge-base tests that don't
require any functionality to be implemented yet.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
...ymmetric.sh => t4068-diff-symmetric-merge-base.sh} | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
rename t/{t4068-diff-symmetric.sh => t4068-diff-symmetric-merge-base.sh} (86%)
diff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric-merge-base.sh
similarity index 86%
rename from t/t4068-diff-symmetric.sh
rename to t/t4068-diff-symmetric-merge-base.sh
index 60c506c2b2..bd4cf254d9 100755
--- a/t/t4068-diff-symmetric.sh
+++ b/t/t4068-diff-symmetric-merge-base.sh
@@ -1,6 +1,6 @@
#!/bin/sh
-test_description='behavior of diff with symmetric-diff setups'
+test_description='behavior of diff with symmetric-diff setups and --merge-base'
. ./test-lib.sh
@@ -88,4 +88,13 @@ test_expect_success 'diff with ranges and extra arg' '
test_i18ngrep "usage" err
'
+test_expect_success 'diff --merge-base with no commits' '
+ test_must_fail git diff --merge-base
+'
+
+test_expect_success 'diff --merge-base with three commits' '
+ test_must_fail git diff --merge-base br1 br2 master 2>err &&
+ test_i18ngrep "usage" err
+'
+
test_done
--
2.28.0.618.gf4bc123cb7
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH v3 08/10] builtin/diff-index: learn --merge-base
2020-09-17 7:44 ` [PATCH v3 00/10] " Denton Liu
` (6 preceding siblings ...)
2020-09-17 7:44 ` [PATCH v3 07/10] t4068: add --merge-base tests Denton Liu
@ 2020-09-17 7:44 ` Denton Liu
2020-09-17 17:28 ` Junio C Hamano
2020-09-17 7:44 ` [PATCH v3 09/10] builtin/diff-tree: " Denton Liu
` (2 subsequent siblings)
10 siblings, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-17 7:44 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano
There is currently no easy way to take the diff between the working tree
or index and the merge base between an arbitrary commit and HEAD. Even
diff's `...` notation doesn't allow this because it only works between
commits. However, the ability to do this would be desirable to a user
who would like to see all the changes they've made on a branch plus
uncommitted changes without taking into account changes made in the
upstream branch.
Teach diff-index and diff (with one commit) the --merge-base option
which allows a user to use the merge base of a commit and HEAD as the
"before" side.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Documentation/git-diff-index.txt | 7 +++-
Documentation/git-diff.txt | 12 ++++--
builtin/diff-index.c | 2 +
builtin/diff.c | 2 +
diff-lib.c | 13 +++++-
diff.h | 1 +
t/t4068-diff-symmetric-merge-base.sh | 59 ++++++++++++++++++++++++++++
7 files changed, 90 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-diff-index.txt b/Documentation/git-diff-index.txt
index 25fe165f00..27acb31cbf 100644
--- a/Documentation/git-diff-index.txt
+++ b/Documentation/git-diff-index.txt
@@ -9,7 +9,7 @@ git-diff-index - Compare a tree to the working tree or index
SYNOPSIS
--------
[verse]
-'git diff-index' [-m] [--cached] [<common diff options>] <tree-ish> [<path>...]
+'git diff-index' [-m] [--cached] [--merge-base] [<common diff options>] <tree-ish> [<path>...]
DESCRIPTION
-----------
@@ -29,6 +29,11 @@ include::diff-options.txt[]
--cached::
Do not consider the on-disk file at all.
+--merge-base::
+ Instead of comparing <tree-ish> directly, use the merge base
+ between <tree-ish> and HEAD instead. <tree-ish> must be a
+ commit.
+
-m::
By default, files recorded in the index but not checked
out are reported as deleted. This flag makes
diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 8f7b4ed3ca..762ee6d074 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -10,7 +10,7 @@ SYNOPSIS
--------
[verse]
'git diff' [<options>] [<commit>] [--] [<path>...]
-'git diff' [<options>] --cached [<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>] <blob> <blob>
@@ -40,7 +40,7 @@ files on disk.
or when running the command outside a working tree
controlled by Git. This form implies `--exit-code`.
-'git diff' [<options>] --cached [<commit>] [--] [<path>...]::
+'git diff' [<options>] --cached [--merge-base] [<commit>] [--] [<path>...]::
This form is to view the changes you staged for the next
commit relative to the named <commit>. Typically you
@@ -49,6 +49,10 @@ files on disk.
If HEAD does not exist (e.g. unborn branches) and
<commit> is not given, it shows all staged changes.
--staged is a synonym of --cached.
++
+If --merge-base is given, instead of using <commit>, use the merge base
+of <commit> and HEAD. `git diff --merge-base A` is equivalent to
+`git diff $(git merge-base A HEAD)`.
'git diff' [<options>] <commit> [--] [<path>...]::
@@ -89,8 +93,8 @@ files on disk.
Just in case you are doing something exotic, it should be
noted that all of the <commit> in the above description, except
-in the last two forms that use `..` notations, can be any
-<tree>.
+in the `--merge-base` case and in the last two forms that use `..`
+notations, can be any <tree>.
For a more complete list of ways to spell <commit>, see
"SPECIFYING REVISIONS" section in linkgit:gitrevisions[7].
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index c3878f7ad6..7f5281c461 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -33,6 +33,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
if (!strcmp(arg, "--cached"))
option |= DIFF_INDEX_CACHED;
+ else if (!strcmp(arg, "--merge-base"))
+ option |= DIFF_INDEX_MERGE_BASE;
else
usage(diff_cache_usage);
}
diff --git a/builtin/diff.c b/builtin/diff.c
index e45e19e37e..1baea18ae0 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -139,6 +139,8 @@ static int builtin_diff_index(struct rev_info *revs,
const char *arg = argv[1];
if (!strcmp(arg, "--cached") || !strcmp(arg, "--staged"))
option |= DIFF_INDEX_CACHED;
+ else if (!strcmp(arg, "--merge-base"))
+ option |= DIFF_INDEX_MERGE_BASE;
else
usage(builtin_diff_usage);
argv++; argc--;
diff --git a/diff-lib.c b/diff-lib.c
index e01c3f0612..68bf86f289 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -569,13 +569,24 @@ 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)
{
struct object_array_entry *ent;
+ struct object_id oid;
+ const char *name;
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 (option & DIFF_INDEX_MERGE_BASE) {
+ diff_get_merge_base(revs, &oid);
+ name = xstrdup(oid_to_hex(&oid));
+ } else {
+ oidcpy(&oid, &ent->item->oid);
+ name = ent->name;
+ }
+
+ if (diff_cache(revs, &oid, name, !!(option & DIFF_INDEX_CACHED)))
exit(128);
diff_set_mnemonic_prefix(&revs->diffopt, "c/", (option & DIFF_INDEX_CACHED) ? "i/" : "w/");
diff --git a/diff.h b/diff.h
index ae2bb7001a..0485786b68 100644
--- a/diff.h
+++ b/diff.h
@@ -588,6 +588,7 @@ 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);
int do_diff_cache(const struct object_id *, struct diff_options *);
diff --git a/t/t4068-diff-symmetric-merge-base.sh b/t/t4068-diff-symmetric-merge-base.sh
index bd4cf254d9..49432379cb 100755
--- a/t/t4068-diff-symmetric-merge-base.sh
+++ b/t/t4068-diff-symmetric-merge-base.sh
@@ -97,4 +97,63 @@ test_expect_success 'diff --merge-base with three commits' '
test_i18ngrep "usage" err
'
+for cmd in diff-index diff
+do
+ test_expect_success "$cmd --merge-base with one commit" '
+ git checkout master &&
+ git $cmd commit-C >expect &&
+ git $cmd --merge-base br2 >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "$cmd --merge-base with one commit and unstaged changes" '
+ git checkout master &&
+ test_when_finished git reset --hard &&
+ echo unstaged >>c &&
+ git $cmd commit-C >expect &&
+ git $cmd --merge-base br2 >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "$cmd --merge-base with one commit and staged and unstaged changes" '
+ git checkout master &&
+ test_when_finished git reset --hard &&
+ echo staged >>c &&
+ git add c &&
+ echo unstaged >>c &&
+ git $cmd commit-C >expect &&
+ git $cmd --merge-base br2 >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "$cmd --merge-base --cached with one commit and staged and unstaged changes" '
+ git checkout master &&
+ test_when_finished git reset --hard &&
+ echo staged >>c &&
+ git add c &&
+ echo unstaged >>c &&
+ git $cmd --cached commit-C >expect &&
+ git $cmd --cached --merge-base br2 >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "$cmd --merge-base with non-commit" '
+ git checkout master &&
+ test_must_fail git $cmd --merge-base master^{tree} 2>err &&
+ test_i18ngrep "fatal: --merge-base only works with commits" err
+ '
+
+ test_expect_success "$cmd --merge-base with no merge bases and one commit" '
+ git checkout master &&
+ test_must_fail git $cmd --merge-base br3 2>err &&
+ test_i18ngrep "fatal: no merge base found" err
+ '
+
+ test_expect_success "$cmd --merge-base with multiple merge bases and one commit" '
+ git checkout master &&
+ test_must_fail git $cmd --merge-base br1 2>err &&
+ test_i18ngrep "fatal: multiple merge bases found" err
+ '
+done
+
test_done
--
2.28.0.618.gf4bc123cb7
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH v3 08/10] builtin/diff-index: learn --merge-base
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
0 siblings, 1 reply; 58+ messages in thread
From: Junio C Hamano @ 2020-09-17 17:28 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List
Denton Liu <liu.denton@gmail.com> writes:
> diff --git a/builtin/diff-index.c b/builtin/diff-index.c
> index c3878f7ad6..7f5281c461 100644
> --- a/builtin/diff-index.c
> +++ b/builtin/diff-index.c
> @@ -33,6 +33,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
>
> if (!strcmp(arg, "--cached"))
> option |= DIFF_INDEX_CACHED;
> + else if (!strcmp(arg, "--merge-base"))
> + option |= DIFF_INDEX_MERGE_BASE;
> else
> usage(diff_cache_usage);
> }
> diff --git a/builtin/diff.c b/builtin/diff.c
> index e45e19e37e..1baea18ae0 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -139,6 +139,8 @@ static int builtin_diff_index(struct rev_info *revs,
> const char *arg = argv[1];
> if (!strcmp(arg, "--cached") || !strcmp(arg, "--staged"))
> option |= DIFF_INDEX_CACHED;
> + else if (!strcmp(arg, "--merge-base"))
> + option |= DIFF_INDEX_MERGE_BASE;
> else
> usage(builtin_diff_usage);
> argv++; argc--;
OK.
> diff --git a/diff-lib.c b/diff-lib.c
> index e01c3f0612..68bf86f289 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -569,13 +569,24 @@ 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)
> {
> struct object_array_entry *ent;
> + struct object_id oid;
> + const char *name;
Let's do the same
int compare_with_merge_base = !!(option & DIFF_INDEX_MERGE_BASE);
here to keep the result easier to follow.
> 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 (option & DIFF_INDEX_MERGE_BASE) {
> + diff_get_merge_base(revs, &oid);
> + name = xstrdup(oid_to_hex(&oid));
Leak?
> + } else {
> + oidcpy(&oid, &ent->item->oid);
> + name = ent->name;
> + }
> +
> + if (diff_cache(revs, &oid, name, !!(option & DIFF_INDEX_CACHED)))
> exit(128);
>
> diff_set_mnemonic_prefix(&revs->diffopt, "c/", (option & DIFF_INDEX_CACHED) ? "i/" : "w/");
> +for cmd in diff-index diff
> +do
> + test_expect_success "$cmd --merge-base with one commit" '
> + git checkout master &&
> + git $cmd commit-C >expect &&
> + git $cmd --merge-base br2 >actual &&
> + test_cmp expect actual
> + '
OK, the same command, when comparing with commit-C and with
"--merge-base br2" that should compute the same commit-C, should
give the same answer. Good testing strategy.
Thanks.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 08/10] builtin/diff-index: learn --merge-base
2020-09-17 17:28 ` Junio C Hamano
@ 2020-09-17 18:13 ` Jeff King
2020-09-18 5:11 ` Junio C Hamano
0 siblings, 1 reply; 58+ messages in thread
From: Jeff King @ 2020-09-17 18:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Denton Liu, Git Mailing List
On Thu, Sep 17, 2020 at 10:28:53AM -0700, Junio C Hamano wrote:
> > - if (diff_cache(revs, &ent->item->oid, ent->name, !!(option & DIFF_INDEX_CACHED)))
> > +
> > + if (option & DIFF_INDEX_MERGE_BASE) {
> > + diff_get_merge_base(revs, &oid);
> > + name = xstrdup(oid_to_hex(&oid));
>
> Leak?
Using oid_to_hex_r() avoids an extra copy, and the leak goes away too:
char merge_base_hex[GIT_MAX_HEXSZ + 1];
...
name = oid_to_hex_r(merge_base_hex, &oid);
-Peff
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 08/10] builtin/diff-index: learn --merge-base
2020-09-17 18:13 ` Jeff King
@ 2020-09-18 5:11 ` Junio C Hamano
2020-09-18 18:12 ` Jeff King
0 siblings, 1 reply; 58+ messages in thread
From: Junio C Hamano @ 2020-09-18 5:11 UTC (permalink / raw)
To: Jeff King; +Cc: Denton Liu, Git Mailing List
Jeff King <peff@peff.net> writes:
> On Thu, Sep 17, 2020 at 10:28:53AM -0700, Junio C Hamano wrote:
>
>> > - if (diff_cache(revs, &ent->item->oid, ent->name, !!(option & DIFF_INDEX_CACHED)))
>> > +
>> > + if (option & DIFF_INDEX_MERGE_BASE) {
>> > + diff_get_merge_base(revs, &oid);
>> > + name = xstrdup(oid_to_hex(&oid));
>>
>> Leak?
>
> Using oid_to_hex_r() avoids an extra copy, and the leak goes away too:
>
> char merge_base_hex[GIT_MAX_HEXSZ + 1];
> ...
> name = oid_to_hex_r(merge_base_hex, &oid);
>
> -Peff
Yes, I was debating myself if I should mention it or trust/assume
that the contributor can easily figure it out.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 08/10] builtin/diff-index: learn --merge-base
2020-09-18 5:11 ` Junio C Hamano
@ 2020-09-18 18:12 ` Jeff King
0 siblings, 0 replies; 58+ messages in thread
From: Jeff King @ 2020-09-18 18:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Denton Liu, Git Mailing List
On Thu, Sep 17, 2020 at 10:11:56PM -0700, Junio C Hamano wrote:
> > Using oid_to_hex_r() avoids an extra copy, and the leak goes away too:
> >
> > char merge_base_hex[GIT_MAX_HEXSZ + 1];
> > ...
> > name = oid_to_hex_r(merge_base_hex, &oid);
> >
> > -Peff
>
> Yes, I was debating myself if I should mention it or trust/assume
> that the contributor can easily figure it out.
I was tempted to call "xstrdup(oid_to_hex())" an anti-pattern, but
looking at most of the other calls, they really do want a new string
that lasts longer than a stack variable.
And even the stack ones are a bit ugly in that you have to know to size
things correctly.
So I lost my enthusiasm to crusade against it. ;)
-Peff
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
2020-09-17 7:44 ` [PATCH v3 00/10] " Denton Liu
` (7 preceding siblings ...)
2020-09-17 7:44 ` [PATCH v3 08/10] builtin/diff-index: learn --merge-base Denton Liu
@ 2020-09-17 7:44 ` Denton Liu
2020-09-17 18:23 ` Junio C Hamano
2020-09-17 7:44 ` [PATCH v3 10/10] contrib/completion: complete `git diff --merge-base` Denton Liu
2020-09-20 11:22 ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
10 siblings, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-17 7:44 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano
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.
Introduce the --merge-base flag as a replacement for 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.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Documentation/git-diff-tree.txt | 7 ++++-
Documentation/git-diff.txt | 9 ++++---
builtin/diff-tree.c | 18 +++++++++++++
builtin/diff.c | 40 +++++++++++++++++++---------
t/t4068-diff-symmetric-merge-base.sh | 34 +++++++++++++++++++++++
5 files changed, 91 insertions(+), 17 deletions(-)
diff --git a/Documentation/git-diff-tree.txt b/Documentation/git-diff-tree.txt
index 5c8a2a5e97..2fc24c542f 100644
--- a/Documentation/git-diff-tree.txt
+++ b/Documentation/git-diff-tree.txt
@@ -10,7 +10,7 @@ SYNOPSIS
--------
[verse]
'git diff-tree' [--stdin] [-m] [-s] [-v] [--no-commit-id] [--pretty]
- [-t] [-r] [-c | --cc] [--combined-all-paths] [--root]
+ [-t] [-r] [-c | --cc] [--combined-all-paths] [--root] [--merge-base]
[<common diff options>] <tree-ish> [<tree-ish>] [<path>...]
DESCRIPTION
@@ -43,6 +43,11 @@ include::diff-options.txt[]
When `--root` is specified the initial commit will be shown as a big
creation event. This is equivalent to a diff against the NULL tree.
+--merge-base::
+ Instead of comparing the <tree-ish>s directly, use the merge
+ base between the two <tree-ish>s as the "before" side. There
+ must be two <tree-ish>s given and they must both be commits.
+
--stdin::
When `--stdin` is specified, the command does not take
<tree-ish> arguments from the command line. Instead, it
diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 762ee6d074..d3b526e00a 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -11,8 +11,7 @@ SYNOPSIS
[verse]
'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>] <blob> <blob>
'git diff' [<options>] --no-index [--] <path> <path>
@@ -62,10 +61,14 @@ of <commit> and HEAD. `git diff --merge-base A` is equivalent to
branch name to compare with the tip of a different
branch.
-'git diff' [<options>] <commit> <commit> [--] [<path>...]::
+'git diff' [<options>] [--merge-base] <commit> <commit> [--] [<path>...]::
This is to view the changes between two arbitrary
<commit>.
++
+If --merge-base is given, use the merge base of the two commits for the
+"before" side. `git diff --merge-base A B` is equivalent to
+`git diff $(git merge-base A B) B`.
'git diff' [<options>] <commit> <commit>... <commit> [--] [<path>...]::
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 802363d0a2..823d6678e5 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -111,6 +111,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
struct setup_revision_opt s_r_opt;
struct userformat_want w;
int read_stdin = 0;
+ int merge_base = 0;
if (argc == 2 && !strcmp(argv[1], "-h"))
usage(diff_tree_usage);
@@ -143,9 +144,26 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
read_stdin = 1;
continue;
}
+ if (!strcmp(arg, "--merge-base")) {
+ merge_base = 1;
+ continue;
+ }
usage(diff_tree_usage);
}
+ if (read_stdin && merge_base)
+ die(_("--stdin and --merge-base are mutually exclusive"));
+
+ if (merge_base) {
+ struct object_id oid;
+
+ if (opt->pending.nr != 2)
+ die(_("--merge-base only works with two commits"));
+
+ diff_get_merge_base(opt, &oid);
+ opt->pending.objects[0].item = lookup_object(the_repository, &oid);
+ }
+
/*
* NOTE! We expect "a..b" to expand to "^a b" but it is
* perfectly valid for revision range parser to yield "b ^a",
diff --git a/builtin/diff.c b/builtin/diff.c
index 1baea18ae0..ad78bc89b3 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -26,8 +26,7 @@
static const char builtin_diff_usage[] =
"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>] <blob> <blob>]\n"
" or: git diff [<options>] --no-index [--] <path> <path>]\n"
COMMON_DIFF_OPTIONS_HELP;
@@ -172,19 +171,34 @@ static int builtin_diff_tree(struct rev_info *revs,
struct object_array_entry *ent1)
{
const struct object_id *(oid[2]);
- int swap = 0;
+ struct object_id mb_oid;
+ int merge_base = 0;
- if (argc > 1)
- usage(builtin_diff_usage);
+ while (1 < argc) {
+ const char *arg = argv[1];
+ if (!strcmp(arg, "--merge-base"))
+ merge_base = 1;
+ else
+ usage(builtin_diff_usage);
+ argv++; argc--;
+ }
- /*
- * We saw two trees, ent0 and ent1. If ent1 is uninteresting,
- * swap them.
- */
- if (ent1->item->flags & UNINTERESTING)
- swap = 1;
- oid[swap] = &ent0->item->oid;
- oid[1 - swap] = &ent1->item->oid;
+ if (merge_base) {
+ diff_get_merge_base(revs, &mb_oid);
+ oid[0] = &mb_oid;
+ oid[1] = &revs->pending.objects[1].item->oid;
+ } else {
+ int swap = 0;
+
+ /*
+ * We saw two trees, ent0 and ent1. If ent1 is uninteresting,
+ * swap them.
+ */
+ if (ent1->item->flags & UNINTERESTING)
+ swap = 1;
+ oid[swap] = &ent0->item->oid;
+ oid[1 - swap] = &ent1->item->oid;
+ }
diff_tree_oid(oid[0], oid[1], "", &revs->diffopt);
log_tree_diff_flush(revs);
return 0;
diff --git a/t/t4068-diff-symmetric-merge-base.sh b/t/t4068-diff-symmetric-merge-base.sh
index 49432379cb..03487cc945 100755
--- a/t/t4068-diff-symmetric-merge-base.sh
+++ b/t/t4068-diff-symmetric-merge-base.sh
@@ -156,4 +156,38 @@ do
'
done
+for cmd in diff-tree diff
+do
+ test_expect_success "$cmd --merge-base with two commits" '
+ git $cmd commit-C master >expect &&
+ git $cmd --merge-base br2 master >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "$cmd --merge-base commit and non-commit" '
+ test_must_fail git $cmd --merge-base br2 master^{tree} 2>err &&
+ test_i18ngrep "fatal: --merge-base only works with commits" err
+ '
+
+ test_expect_success "$cmd --merge-base with no merge bases and two commits" '
+ test_must_fail git $cmd --merge-base br2 br3 2>err &&
+ test_i18ngrep "fatal: no merge base found" err
+ '
+
+ test_expect_success "$cmd --merge-base with multiple merge bases and two commits" '
+ test_must_fail git $cmd --merge-base master br1 2>err &&
+ test_i18ngrep "fatal: multiple merge bases found" err
+ '
+done
+
+test_expect_success 'diff-tree --merge-base with one commit' '
+ test_must_fail git diff-tree --merge-base master 2>err &&
+ test_i18ngrep "fatal: --merge-base only works with two commits" err
+'
+
+test_expect_success 'diff --merge-base with range' '
+ test_must_fail git diff --merge-base br2..br3 2>err &&
+ test_i18ngrep "fatal: --merge-base does not work with ranges" err
+'
+
test_done
--
2.28.0.618.gf4bc123cb7
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
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
0 siblings, 1 reply; 58+ messages in thread
From: Junio C Hamano @ 2020-09-17 18:23 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List
Denton Liu <liu.denton@gmail.com> writes:
> + if (read_stdin && merge_base)
> + die(_("--stdin and --merge-base are mutually exclusive"));
> +
> + if (merge_base) {
> + struct object_id oid;
> +
> + if (opt->pending.nr != 2)
> + die(_("--merge-base only works with two commits"));
> +
> + diff_get_merge_base(opt, &oid);
> + opt->pending.objects[0].item = lookup_object(the_repository, &oid);
> + }
> +
This looks quite straight-forward.
> - /*
> - * We saw two trees, ent0 and ent1. If ent1 is uninteresting,
> - * swap them.
> - */
> - if (ent1->item->flags & UNINTERESTING)
> - swap = 1;
> - oid[swap] = &ent0->item->oid;
> - oid[1 - swap] = &ent1->item->oid;
> + if (merge_base) {
> + diff_get_merge_base(revs, &mb_oid);
> + oid[0] = &mb_oid;
> + oid[1] = &revs->pending.objects[1].item->oid;
> + } else {
> + int swap = 0;
> +
> + /*
> + * We saw two trees, ent0 and ent1. If ent1 is uninteresting,
> + * swap them.
> + */
> + if (ent1->item->flags & UNINTERESTING)
> + swap = 1;
> + oid[swap] = &ent0->item->oid;
> + oid[1 - swap] = &ent1->item->oid;
> + }
It is not entirely clear why the original has to become an [else]
clause here, unlike the change we saw earlier in cmd_diff_tree().
It feels quite inconsistent.
Thanks.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
2020-09-17 18:23 ` Junio C Hamano
@ 2020-09-18 10:48 ` Denton Liu
2020-09-18 16:52 ` Junio C Hamano
0 siblings, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-18 10:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
On Thu, Sep 17, 2020 at 11:23:54AM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
>
> > + if (read_stdin && merge_base)
> > + die(_("--stdin and --merge-base are mutually exclusive"));
> > +
> > + if (merge_base) {
> > + struct object_id oid;
> > +
> > + if (opt->pending.nr != 2)
> > + die(_("--merge-base only works with two commits"));
> > +
> > + diff_get_merge_base(opt, &oid);
> > + opt->pending.objects[0].item = lookup_object(the_repository, &oid);
> > + }
> > +
>
> This looks quite straight-forward.
>
> > - /*
> > - * We saw two trees, ent0 and ent1. If ent1 is uninteresting,
> > - * swap them.
> > - */
> > - if (ent1->item->flags & UNINTERESTING)
> > - swap = 1;
> > - oid[swap] = &ent0->item->oid;
> > - oid[1 - swap] = &ent1->item->oid;
> > + if (merge_base) {
> > + diff_get_merge_base(revs, &mb_oid);
> > + oid[0] = &mb_oid;
> > + oid[1] = &revs->pending.objects[1].item->oid;
> > + } else {
> > + int swap = 0;
> > +
> > + /*
> > + * We saw two trees, ent0 and ent1. If ent1 is uninteresting,
> > + * swap them.
> > + */
> > + if (ent1->item->flags & UNINTERESTING)
> > + swap = 1;
> > + oid[swap] = &ent0->item->oid;
> > + oid[1 - swap] = &ent1->item->oid;
> > + }
>
> It is not entirely clear why the original has to become an [else]
> clause here, unlike the change we saw earlier in cmd_diff_tree().
> It feels quite inconsistent.
Since we're only interested in the oids, I thought that it would be
possible to save a lookup_object() and just use the oids directly. If
it's clearer, this can be written as something like this but the lookup
feels unnecessary:
/*
* We saw two trees, ent0 and ent1. If ent1 is uninteresting,
* swap them.
*/
if (ent1->item->flags & UNINTERESTING)
swap = 1;
if (merge_base) {
struct object_id mb_oid;
if (swap)
BUG("swap is unexpectedly set");
if (diff_get_merge_base(revs, &mb_oid))
exit(128);
ent0->item = lookup_object(the_repository, &mb_oid);
}
oid[swap] = &ent0->item->oid;
oid[1 - swap] = &ent1->item->oid;
Thanks,
Denton
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
2020-09-18 10:48 ` Denton Liu
@ 2020-09-18 16:52 ` Junio C Hamano
2020-09-20 11:01 ` Denton Liu
0 siblings, 1 reply; 58+ messages in thread
From: Junio C Hamano @ 2020-09-18 16:52 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List
Denton Liu <liu.denton@gmail.com> writes:
> Since we're only interested in the oids, I thought that it would be
> possible to save a lookup_object() and just use the oids directly. If
> it's clearer, this can be written as something like this but the lookup
> feels unnecessary:
When running the tree diff, we'd need the object anyway, and the
result of the look-up made here is cached, right?
That is why I expected it would just be an insertion before the
existing code, like the other side.
But the existing "if we got either ^A B or B ^A, treat it as A..B"
logic is just like "if we got '--merge-base A B', treat it as
something else" we are adding, and they (and any future such special
syntax) should not interact with each other. So in that sense, the
code structure you have in the originally posted patch (not the code
snippet in your message I am responding to) that does
...
if (using merge-base feature) {
do the merge base thing to populate oid[]
} else if (user used A..B) {
ensure "^A B" and "B ^A" both have A in oid[0] and B in oid[1]
}
...
call diff-tree between oid[0] and oid[1]
makes a lot more sense than anything else we discussed so far.
I wonder if turning the builtin/diff-tree.c to match that structure
make the result easier to understand (and I'll be perfectly happy if
the answer to this question turns out to be "no, the result of the
posted patch is the easiest to follow").
Thanks.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
2020-09-18 16:52 ` Junio C Hamano
@ 2020-09-20 11:01 ` Denton Liu
2020-09-21 16:05 ` Junio C Hamano
0 siblings, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-20 11:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
Hi Junio,
On Fri, Sep 18, 2020 at 09:52:39AM -0700, Junio C Hamano wrote:
> I wonder if turning the builtin/diff-tree.c to match that structure
> make the result easier to understand (and I'll be perfectly happy if
> the answer to this question turns out to be "no, the result of the
> posted patch is the easiest to follow").
git diff-tree does not even recognise ranges so as a result, the else
case does not even need to exist there, unlike in git diff.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
2020-09-20 11:01 ` Denton Liu
@ 2020-09-21 16:05 ` Junio C Hamano
2020-09-21 17:27 ` Denton Liu
0 siblings, 1 reply; 58+ messages in thread
From: Junio C Hamano @ 2020-09-21 16:05 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List
Denton Liu <liu.denton@gmail.com> writes:
> Hi Junio,
>
> On Fri, Sep 18, 2020 at 09:52:39AM -0700, Junio C Hamano wrote:
>> I wonder if turning the builtin/diff-tree.c to match that structure
>> make the result easier to understand (and I'll be perfectly happy if
>> the answer to this question turns out to be "no, the result of the
>> posted patch is the easiest to follow").
>
> git diff-tree does not even recognise ranges so as a result, the else
> case does not even need to exist there, unlike in git diff.
(caution: before morning caffeine so what I say may be totally off)
Do you mean "git diff-tree HEAD^..HEAD" would fail, or something
else?
Thanks.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
2020-09-21 16:05 ` Junio C Hamano
@ 2020-09-21 17:27 ` Denton Liu
2020-09-21 21:09 ` Junio C Hamano
0 siblings, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-21 17:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
Hi Junio,
On Mon, Sep 21, 2020 at 09:05:26AM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
>
> > Hi Junio,
> >
> > On Fri, Sep 18, 2020 at 09:52:39AM -0700, Junio C Hamano wrote:
> >> I wonder if turning the builtin/diff-tree.c to match that structure
> >> make the result easier to understand (and I'll be perfectly happy if
> >> the answer to this question turns out to be "no, the result of the
> >> posted patch is the easiest to follow").
> >
> > git diff-tree does not even recognise ranges so as a result, the else
> > case does not even need to exist there, unlike in git diff.
>
> (caution: before morning caffeine so what I say may be totally off)
>
> Do you mean "git diff-tree HEAD^..HEAD" would fail, or something
> else?
Yes, that is what I meant but I can see that what I wrote is totally
wrong. I was reading git-diff-tree.txt and I assumed that ranges were
not supported at all.
Anyway, now that I've realised my mistake, I've rewritten the diff-tree
part so that the structure matches what was written in diff and it
should be easier to follow.
-- >8 --
From: Denton Liu <liu.denton@gmail.com>
Date: Mon, 14 Sep 2020 11:36:52 -0700
Subject: [PATCH] builtin/diff-tree: learn --merge-base
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 an alternative to triple-dot
notation. Thus, we would be able to write the above as
`git diff --merge-base A B`.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Documentation/git-diff-tree.txt | 7 ++++-
Documentation/git-diff.txt | 8 ++++--
builtin/diff-tree.c | 17 +++++++++++-
builtin/diff.c | 39 +++++++++++++++++++---------
t/t4068-diff-symmetric-merge-base.sh | 34 ++++++++++++++++++++++++
5 files changed, 89 insertions(+), 16 deletions(-)
diff --git a/Documentation/git-diff-tree.txt b/Documentation/git-diff-tree.txt
index 5c8a2a5e97..2fc24c542f 100644
--- a/Documentation/git-diff-tree.txt
+++ b/Documentation/git-diff-tree.txt
@@ -10,7 +10,7 @@ SYNOPSIS
--------
[verse]
'git diff-tree' [--stdin] [-m] [-s] [-v] [--no-commit-id] [--pretty]
- [-t] [-r] [-c | --cc] [--combined-all-paths] [--root]
+ [-t] [-r] [-c | --cc] [--combined-all-paths] [--root] [--merge-base]
[<common diff options>] <tree-ish> [<tree-ish>] [<path>...]
DESCRIPTION
@@ -43,6 +43,11 @@ include::diff-options.txt[]
When `--root` is specified the initial commit will be shown as a big
creation event. This is equivalent to a diff against the NULL tree.
+--merge-base::
+ Instead of comparing the <tree-ish>s directly, use the merge
+ base between the two <tree-ish>s as the "before" side. There
+ must be two <tree-ish>s given and they must both be commits.
+
--stdin::
When `--stdin` is specified, the command does not take
<tree-ish> arguments from the command line. Instead, it
diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 762ee6d074..7f4c8a8ce7 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -11,7 +11,7 @@ SYNOPSIS
[verse]
'git diff' [<options>] [<commit>] [--] [<path>...]
'git diff' [<options>] --cached [--merge-base] [<commit>] [--] [<path>...]
-'git diff' [<options>] <commit> [<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>
@@ -62,10 +62,14 @@ of <commit> and HEAD. `git diff --merge-base A` is equivalent to
branch name to compare with the tip of a different
branch.
-'git diff' [<options>] <commit> <commit> [--] [<path>...]::
+'git diff' [<options>] [--merge-base] <commit> <commit> [--] [<path>...]::
This is to view the changes between two arbitrary
<commit>.
++
+If --merge-base is given, use the merge base of the two commits for the
+"before" side. `git diff --merge-base A B` is equivalent to
+`git diff $(git merge-base A B) B`.
'git diff' [<options>] <commit> <commit>... <commit> [--] [<path>...]::
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 802363d0a2..9fc95e959f 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -111,6 +111,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
struct setup_revision_opt s_r_opt;
struct userformat_want w;
int read_stdin = 0;
+ int merge_base = 0;
if (argc == 2 && !strcmp(argv[1], "-h"))
usage(diff_tree_usage);
@@ -143,9 +144,18 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
read_stdin = 1;
continue;
}
+ if (!strcmp(arg, "--merge-base")) {
+ merge_base = 1;
+ continue;
+ }
usage(diff_tree_usage);
}
+ if (read_stdin && merge_base)
+ die(_("--stdin and --merge-base are mutually exclusive"));
+ if (merge_base && opt->pending.nr != 2)
+ die(_("--merge-base only works with two commits"));
+
/*
* NOTE! We expect "a..b" to expand to "^a b" but it is
* perfectly valid for revision range parser to yield "b ^a",
@@ -165,7 +175,12 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
case 2:
tree1 = opt->pending.objects[0].item;
tree2 = opt->pending.objects[1].item;
- if (tree2->flags & UNINTERESTING) {
+ if (merge_base) {
+ struct object_id oid;
+
+ diff_get_merge_base(opt, &oid);
+ tree1 = lookup_object(the_repository, &oid);
+ } else if (tree2->flags & UNINTERESTING) {
SWAP(tree2, tree1);
}
diff_tree_oid(&tree1->oid, &tree2->oid, "", &opt->diffopt);
diff --git a/builtin/diff.c b/builtin/diff.c
index 1baea18ae0..b50fc68c2a 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -26,7 +26,7 @@
static const char builtin_diff_usage[] =
"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> [--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"
@@ -172,19 +172,34 @@ static int builtin_diff_tree(struct rev_info *revs,
struct object_array_entry *ent1)
{
const struct object_id *(oid[2]);
- int swap = 0;
+ struct object_id mb_oid;
+ int merge_base = 0;
- if (argc > 1)
- usage(builtin_diff_usage);
+ while (1 < argc) {
+ const char *arg = argv[1];
+ if (!strcmp(arg, "--merge-base"))
+ merge_base = 1;
+ else
+ usage(builtin_diff_usage);
+ argv++; argc--;
+ }
- /*
- * We saw two trees, ent0 and ent1. If ent1 is uninteresting,
- * swap them.
- */
- if (ent1->item->flags & UNINTERESTING)
- swap = 1;
- oid[swap] = &ent0->item->oid;
- oid[1 - swap] = &ent1->item->oid;
+ if (merge_base) {
+ diff_get_merge_base(revs, &mb_oid);
+ oid[0] = &mb_oid;
+ oid[1] = &revs->pending.objects[1].item->oid;
+ } else {
+ int swap = 0;
+
+ /*
+ * We saw two trees, ent0 and ent1. If ent1 is uninteresting,
+ * swap them.
+ */
+ if (ent1->item->flags & UNINTERESTING)
+ swap = 1;
+ oid[swap] = &ent0->item->oid;
+ oid[1 - swap] = &ent1->item->oid;
+ }
diff_tree_oid(oid[0], oid[1], "", &revs->diffopt);
log_tree_diff_flush(revs);
return 0;
diff --git a/t/t4068-diff-symmetric-merge-base.sh b/t/t4068-diff-symmetric-merge-base.sh
index 49432379cb..03487cc945 100755
--- a/t/t4068-diff-symmetric-merge-base.sh
+++ b/t/t4068-diff-symmetric-merge-base.sh
@@ -156,4 +156,38 @@ do
'
done
+for cmd in diff-tree diff
+do
+ test_expect_success "$cmd --merge-base with two commits" '
+ git $cmd commit-C master >expect &&
+ git $cmd --merge-base br2 master >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "$cmd --merge-base commit and non-commit" '
+ test_must_fail git $cmd --merge-base br2 master^{tree} 2>err &&
+ test_i18ngrep "fatal: --merge-base only works with commits" err
+ '
+
+ test_expect_success "$cmd --merge-base with no merge bases and two commits" '
+ test_must_fail git $cmd --merge-base br2 br3 2>err &&
+ test_i18ngrep "fatal: no merge base found" err
+ '
+
+ test_expect_success "$cmd --merge-base with multiple merge bases and two commits" '
+ test_must_fail git $cmd --merge-base master br1 2>err &&
+ test_i18ngrep "fatal: multiple merge bases found" err
+ '
+done
+
+test_expect_success 'diff-tree --merge-base with one commit' '
+ test_must_fail git diff-tree --merge-base master 2>err &&
+ test_i18ngrep "fatal: --merge-base only works with two commits" err
+'
+
+test_expect_success 'diff --merge-base with range' '
+ test_must_fail git diff --merge-base br2..br3 2>err &&
+ test_i18ngrep "fatal: --merge-base does not work with ranges" err
+'
+
test_done
--
2.28.0.760.g8d73e04208
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
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
0 siblings, 2 replies; 58+ messages in thread
From: Junio C Hamano @ 2020-09-21 21:09 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List
Denton Liu <liu.denton@gmail.com> writes:
> @@ -165,7 +175,12 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
> case 2:
> tree1 = opt->pending.objects[0].item;
> tree2 = opt->pending.objects[1].item;
> - if (tree2->flags & UNINTERESTING) {
> + if (merge_base) {
> + struct object_id oid;
> +
> + diff_get_merge_base(opt, &oid);
> + tree1 = lookup_object(the_repository, &oid);
> + } else if (tree2->flags & UNINTERESTING) {
> SWAP(tree2, tree1);
> }
> diff_tree_oid(&tree1->oid, &tree2->oid, "", &opt->diffopt);
OK. Handling this in that "case 2" does make sense.
However.
The above code as-is will allow something like
git diff --merge-base A..B
and it will be taken the same as
git diff --merge-base A B
But let's step back and think why we bother with SWAP() in the
normal case. This is due to the possibility that A..B, which
currently is left in the pending.objects[] array as ^A B, might
someday be stored as B ^A. If we leave that code to protect us from
the possibility, shouldn't we be protecting us from the same
"someday" for the new code, too?
That is "git diff --merge-base A..B", when the control reaches this
part of the code, may have tree1=B tree2=^A
Which suggests that a consistently written code would look like so:
tree1 = opt->pending.objects[0].item;
tree2 = opt->pending.objects[1].item;
if (tree2->flags & UNINTERESTING)
/*
* A..B currently becomes ^A B but it is perfectly
* ok for revision parser to leave us B ^A; detect
* and swap them in the original order.
*/
SWAP(tree2, tree1);
if (merge_base) {
struct object_id oid;
diff_get_merge_base(opt, &oid);
tree1 = lookup_object(the_repository, &oid);
}
diff_tree_oid(&tree1->oid, &tree2->oid, "", &opt->diffopt);
log_tree_diff_flush(opt);
Another possibility is to error out when "--merge-base A..B" is
given, which might be simpler. Then the code would look more like
tree1 = ...
tree2 = ...
if (merge_base) {
if ((tree1->flags | tree2->flags) & UNINTERESTING)
die(_("use of --merge-base with A..B forbidden"));
... get merge base and assign it to tree1 ...
} else if (tree2->flags & UNINTERESTING) {
SWAP();
}
While we are at it, what happens when "--merge-base A...B" is given?
In the original code without "--merge-base", "git diff-tree A...B"
places the merge base between A and B in pending.objects[0] and B in
pending.objects[1], I think. "git diff-tree --merge-base A...B"
would further compute the merge base between these two objects, but
luckily $(git merge-base $(merge-base A B) B) is the same as $(git
merge-base A B), so you won't get an incorrect answer from such a
request. Is this something we want to diagnose as an error? I am
inclined to say we should allow it (and if it hurts the user can
stop doing so) as there is no harm done.
Thanks.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
2020-09-21 21:09 ` Junio C Hamano
@ 2020-09-21 21:19 ` Junio C Hamano
2020-09-21 21:54 ` Denton Liu
1 sibling, 0 replies; 58+ messages in thread
From: Junio C Hamano @ 2020-09-21 21:19 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List
Junio C Hamano <gitster@pobox.com> writes:
> Another possibility is to error out when "--merge-base A..B" is
> given, which might be simpler. Then the code would look more like
> ...
>
> While we are at it, what happens when "--merge-base A...B" is given?
>
> ... Is this something we want to diagnose as an error? I am
> inclined to say we should allow it (and if it hurts the user can
> stop doing so) as there is no harm done.
My recommendation is to allow both "git --merge-base A..B" and "git
--merge-base A...B". The discussion about A..B and SWAP() would
equally apply to builtin/diff part of the patch. The posted patch
ignores the swap logic when --merge-base is given, but we should
apply the swap logic first and then make sure the merge_base logic
will have the oid[0] and oid[1] in the correct order.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
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
1 sibling, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-21 21:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
Hi Junio,
On Mon, Sep 21, 2020 at 02:09:24PM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
>
> > @@ -165,7 +175,12 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
> > case 2:
> > tree1 = opt->pending.objects[0].item;
> > tree2 = opt->pending.objects[1].item;
> > - if (tree2->flags & UNINTERESTING) {
> > + if (merge_base) {
> > + struct object_id oid;
> > +
> > + diff_get_merge_base(opt, &oid);
> > + tree1 = lookup_object(the_repository, &oid);
> > + } else if (tree2->flags & UNINTERESTING) {
> > SWAP(tree2, tree1);
> > }
> > diff_tree_oid(&tree1->oid, &tree2->oid, "", &opt->diffopt);
>
> OK. Handling this in that "case 2" does make sense.
>
> However.
>
> The above code as-is will allow something like
>
> git diff --merge-base A..B
>
> and it will be taken the same as
>
> git diff --merge-base A B
This does not happen because at the top of diff_get_merge_base(), we
have
for (i = 0; i < revs->pending.nr; i++) {
struct object *obj = revs->pending.objects[i].item;
if (obj->flags)
die(_("--merge-base does not work with ranges"));
if (obj->type != OBJ_COMMIT)
die(_("--merge-base only works with commits"));
}
which ensures that we don't accept any ranges at all. This is why I
considered the SWAP and merge_base cases to be mutually exclusive.
> Another possibility is to error out when "--merge-base A..B" is
> given, which might be simpler. Then the code would look more like
>
>
> tree1 = ...
> tree2 = ...
>
> if (merge_base) {
> if ((tree1->flags | tree2->flags) & UNINTERESTING)
> die(_("use of --merge-base with A..B forbidden"));
> ... get merge base and assign it to tree1 ...
> } else if (tree2->flags & UNINTERESTING) {
> SWAP();
> }
This is the route I picked, although the logic for this is in
diff_get_merge_base().
> While we are at it, what happens when "--merge-base A...B" is given?
>
> In the original code without "--merge-base", "git diff-tree A...B"
> places the merge base between A and B in pending.objects[0] and B in
> pending.objects[1], I think. "git diff-tree --merge-base A...B"
> would further compute the merge base between these two objects, but
> luckily $(git merge-base $(merge-base A B) B) is the same as $(git
> merge-base A B), so you won't get an incorrect answer from such a
> request. Is this something we want to diagnose as an error? I am
> inclined to say we should allow it (and if it hurts the user can
> stop doing so) as there is no harm done.
I think that we should error out for all ranges because this option
semantically only really makes sense on two endpoints, not a range of
commits. Since the check is cheap to protect users from themselves, we
might as well actually do it.
Worst case, if someone has a legimitate use case for --merge-base and
ranges, we can allow it in the future, which would be easier than
removing this feature.
Thanks,
Denton
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
2020-09-21 21:54 ` Denton Liu
@ 2020-09-21 22:18 ` Junio C Hamano
2020-09-23 9:47 ` Denton Liu
0 siblings, 1 reply; 58+ messages in thread
From: Junio C Hamano @ 2020-09-21 22:18 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List
Denton Liu <liu.denton@gmail.com> writes:
> This does not happen because at the top of diff_get_merge_base(), we
> have
>
> for (i = 0; i < revs->pending.nr; i++) {
> struct object *obj = revs->pending.objects[i].item;
> if (obj->flags)
> die(_("--merge-base does not work with ranges"));
> if (obj->type != OBJ_COMMIT)
> die(_("--merge-base only works with commits"));
> }
>
> which ensures that we don't accept any ranges at all.
I think we should lose that loop, or at least the first test.
If we are not removing the support for "A..B" notation and still
accept "diff A..B" happily, not accepting "diff --merge-base A..B"
would appear inconsistent to the users.
The same applies to "A...B".
Thanks.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
2020-09-21 22:18 ` Junio C Hamano
@ 2020-09-23 9:47 ` Denton Liu
2020-09-25 21:02 ` Junio C Hamano
0 siblings, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-23 9:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
Hi Junio,
On Mon, Sep 21, 2020 at 03:18:06PM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
>
> > This does not happen because at the top of diff_get_merge_base(), we
> > have
> >
> > for (i = 0; i < revs->pending.nr; i++) {
> > struct object *obj = revs->pending.objects[i].item;
> > if (obj->flags)
> > die(_("--merge-base does not work with ranges"));
> > if (obj->type != OBJ_COMMIT)
> > die(_("--merge-base only works with commits"));
> > }
> >
> > which ensures that we don't accept any ranges at all.
>
> I think we should lose that loop, or at least the first test.
>
> If we are not removing the support for "A..B" notation and still
> accept "diff A..B" happily, not accepting "diff --merge-base A..B"
> would appear inconsistent to the users.
I disagree, in the documentation, it clearly states that this option is
only available to the diff modes that accept endpoints, not
ranges:
'git diff' [<options>] --cached [--merge-base] [<commit>] [--] [<path>...]::
and
'git diff' [<options>] [--merge-base] <commit> <commit> [--] [<path>...]::
so it seems perfectly consistent to me. The documentation gives the
impression that the range notations are their own separate mode anyway.
And worst case scenario, if we receive user reports that they believe
the feature is inconsistent, it's 100x easier to change it to allow
ranges than attempting to remove support for ranges in the future.
Thanks,
Denton
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
2020-09-23 9:47 ` Denton Liu
@ 2020-09-25 21:02 ` Junio C Hamano
2020-09-26 1:52 ` Denton Liu
0 siblings, 1 reply; 58+ messages in thread
From: Junio C Hamano @ 2020-09-25 21:02 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List
Denton Liu <liu.denton@gmail.com> writes:
> And worst case scenario, if we receive user reports that they believe
> the feature is inconsistent, it's 100x easier to change it to allow
> ranges than attempting to remove support for ranges in the future.
If we allow ranges from day one, we do not even have to worry about
it, no?
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
2020-09-25 21:02 ` Junio C Hamano
@ 2020-09-26 1:52 ` Denton Liu
0 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-26 1:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
Hi Junio,
On Fri, Sep 25, 2020 at 02:02:11PM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
>
> > And worst case scenario, if we receive user reports that they believe
> > the feature is inconsistent, it's 100x easier to change it to allow
> > ranges than attempting to remove support for ranges in the future.
>
> If we allow ranges from day one, we do not even have to worry about
> it, no?
Yes, but I'm worried that being able to mix --merge-base with ranges
might cause more confusion for users since, in my opinion, it only
really makes sense for endpoints. That's why I restricted it in the
first place.
I think that since we're in disagreement, it makes more sense to take
the safer option where we can implement functionality later whereas if
we implement it and we want to remove it later, it'll be a much harder
time.
Thanks,
Denton
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v3 10/10] contrib/completion: complete `git diff --merge-base`
2020-09-17 7:44 ` [PATCH v3 00/10] " Denton Liu
` (8 preceding siblings ...)
2020-09-17 7:44 ` [PATCH v3 09/10] builtin/diff-tree: " Denton Liu
@ 2020-09-17 7:44 ` Denton Liu
2020-09-20 11:22 ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
10 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-17 7:44 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
contrib/completion/git-completion.bash | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index f68c8e0646..679d1ec8a8 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1692,7 +1692,7 @@ __git_diff_common_options="--stat --numstat --shortstat --summary
"
__git_diff_difftool_options="--cached --staged --pickaxe-all --pickaxe-regex
- --base --ours --theirs --no-index --relative
+ --base --ours --theirs --no-index --relative --merge-base
$__git_diff_common_options"
_git_diff ()
--
2.28.0.618.gf4bc123cb7
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH v4 00/10] builtin/diff: learn --merge-base
2020-09-17 7:44 ` [PATCH v3 00/10] " Denton Liu
` (9 preceding siblings ...)
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
2020-09-20 11:22 ` [PATCH v4 01/10] t4068: remove unnecessary >tmp Denton Liu
` (9 more replies)
10 siblings, 10 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-20 11:22 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano, Jeff King
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
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v4 01/10] t4068: remove unnecessary >tmp
2020-09-20 11:22 ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
@ 2020-09-20 11:22 ` Denton Liu
2020-09-20 11:22 ` [PATCH v4 02/10] git-diff-index.txt: make --cached description a proper sentence Denton Liu
` (8 subsequent siblings)
9 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-20 11:22 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano, Jeff King
The many `git diff` invocations have a `>tmp` redirection even though
the file is not being used afterwards. Remove these unnecessary
redirections.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
t/t4068-diff-symmetric.sh | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric.sh
index 31d17a5af0..60c506c2b2 100755
--- a/t/t4068-diff-symmetric.sh
+++ b/t/t4068-diff-symmetric.sh
@@ -64,27 +64,27 @@ test_expect_success 'diff with two merge bases' '
'
test_expect_success 'diff with no merge bases' '
- test_must_fail git diff br2...br3 >tmp 2>err &&
+ test_must_fail git diff br2...br3 2>err &&
test_i18ngrep "fatal: br2...br3: no merge base" err
'
test_expect_success 'diff with too many symmetric differences' '
- test_must_fail git diff br1...master br2...br3 >tmp 2>err &&
+ test_must_fail git diff br1...master br2...br3 2>err &&
test_i18ngrep "usage" err
'
test_expect_success 'diff with symmetric difference and extraneous arg' '
- test_must_fail git diff master br1...master >tmp 2>err &&
+ test_must_fail git diff master br1...master 2>err &&
test_i18ngrep "usage" err
'
test_expect_success 'diff with two ranges' '
- test_must_fail git diff master br1..master br2..br3 >tmp 2>err &&
+ test_must_fail git diff master br1..master br2..br3 2>err &&
test_i18ngrep "usage" err
'
test_expect_success 'diff with ranges and extra arg' '
- test_must_fail git diff master br1..master commit-D >tmp 2>err &&
+ test_must_fail git diff master br1..master commit-D 2>err &&
test_i18ngrep "usage" err
'
--
2.28.0.760.g8d73e04208
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH v4 02/10] git-diff-index.txt: make --cached description a proper sentence
2020-09-20 11:22 ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
2020-09-20 11:22 ` [PATCH v4 01/10] t4068: remove unnecessary >tmp Denton Liu
@ 2020-09-20 11:22 ` Denton Liu
2020-09-20 11:22 ` [PATCH v4 03/10] git-diff.txt: backtick quote command text Denton Liu
` (7 subsequent siblings)
9 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-20 11:22 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano, Jeff King
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Documentation/git-diff-index.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/git-diff-index.txt b/Documentation/git-diff-index.txt
index f4bd8155c0..25fe165f00 100644
--- a/Documentation/git-diff-index.txt
+++ b/Documentation/git-diff-index.txt
@@ -27,7 +27,7 @@ include::diff-options.txt[]
The id of a tree object to diff against.
--cached::
- do not consider the on-disk file at all
+ Do not consider the on-disk file at all.
-m::
By default, files recorded in the index but not checked
--
2.28.0.760.g8d73e04208
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH v4 03/10] git-diff.txt: backtick quote command text
2020-09-20 11:22 ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
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 ` Denton Liu
2020-09-20 11:22 ` [PATCH v4 04/10] contrib/completion: extract common diff/difftool options Denton Liu
` (6 subsequent siblings)
9 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-20 11:22 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano, Jeff King
The modern way to quote commands in the documentation is to use
backticks instead of double-quotes as this renders the text with the
code style. Convert double-quoted command text to backtick-quoted
commands. While we're at it, quote one instance of `^@`.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Documentation/git-diff.txt | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 727f24d16e..8f7b4ed3ca 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -68,13 +68,13 @@ files on disk.
This form is to view the results of a merge commit. The first
listed <commit> must be the merge itself; the remaining two or
more commits should be its parents. A convenient way to produce
- the desired set of revisions is to use the {caret}@ suffix.
+ the desired set of revisions is to use the `^@` suffix.
For instance, if `master` names a merge commit, `git diff master
master^@` gives the same combined diff as `git show master`.
'git diff' [<options>] <commit>..<commit> [--] [<path>...]::
- This is synonymous to the earlier form (without the "..") for
+ This is synonymous to the earlier form (without the `..`) for
viewing the changes between two arbitrary <commit>. If <commit> on
one side is omitted, it will have the same effect as
using HEAD instead.
@@ -83,20 +83,20 @@ files on disk.
This form is to view the changes on the branch containing
and up to the second <commit>, starting at a common ancestor
- of both <commit>. "git diff A\...B" is equivalent to
- "git diff $(git merge-base A B) B". You can omit any one
+ of both <commit>. `git diff A...B` is equivalent to
+ `git diff $(git merge-base A B) B`. You can omit any one
of <commit>, which has the same effect as using HEAD instead.
Just in case you are doing something exotic, it should be
noted that all of the <commit> in the above description, except
-in the last two forms that use ".." notations, can be any
+in the last two forms that use `..` notations, can be any
<tree>.
For a more complete list of ways to spell <commit>, see
"SPECIFYING REVISIONS" section in linkgit:gitrevisions[7].
However, "diff" is about comparing two _endpoints_, not ranges,
-and the range notations ("<commit>..<commit>" and
-"<commit>\...<commit>") do not mean a range as defined in the
+and the range notations (`<commit>..<commit>` and
+`<commit>...<commit>`) do not mean a range as defined in the
"SPECIFYING RANGES" section in linkgit:gitrevisions[7].
'git diff' [<options>] <blob> <blob>::
@@ -144,9 +144,9 @@ $ git diff HEAD <3>
+
<1> Changes in the working tree not yet staged for the next commit.
<2> Changes between the index and your last commit; what you
- would be committing if you run "git commit" without "-a" option.
+ would be committing if you run `git commit` without `-a` option.
<3> Changes in the working tree since your last commit; what you
- would be committing if you run "git commit -a"
+ would be committing if you run `git commit -a`
Comparing with arbitrary commits::
+
--
2.28.0.760.g8d73e04208
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH v4 04/10] contrib/completion: extract common diff/difftool options
2020-09-20 11:22 ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
` (2 preceding siblings ...)
2020-09-20 11:22 ` [PATCH v4 03/10] git-diff.txt: backtick quote command text Denton Liu
@ 2020-09-20 11:22 ` Denton Liu
2020-09-20 11:22 ` [PATCH v4 05/10] diff-lib: accept option flags in run_diff_index() Denton Liu
` (5 subsequent siblings)
9 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-20 11:22 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano, Jeff King
difftool parses its own options and then passes the remaining options
onto diff. As a result, they share common command-line options. Instead
of duplicating the list, use a shared $__git_diff_difftool_options list.
The completion for diff is missing --relative and the completion for
difftool is missing --no-index. Add both of these to the common list.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
contrib/completion/git-completion.bash | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 9147fba3d5..f68c8e0646 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1691,6 +1691,10 @@ __git_diff_common_options="--stat --numstat --shortstat --summary
--textconv --no-textconv
"
+__git_diff_difftool_options="--cached --staged --pickaxe-all --pickaxe-regex
+ --base --ours --theirs --no-index --relative
+ $__git_diff_common_options"
+
_git_diff ()
{
__git_has_doubledash && return
@@ -1713,10 +1717,7 @@ _git_diff ()
return
;;
--*)
- __gitcomp "--cached --staged --pickaxe-all --pickaxe-regex
- --base --ours --theirs --no-index
- $__git_diff_common_options
- "
+ __gitcomp "$__git_diff_difftool_options"
return
;;
esac
@@ -1738,11 +1739,7 @@ _git_difftool ()
return
;;
--*)
- __gitcomp_builtin difftool "$__git_diff_common_options
- --base --cached --ours --theirs
- --pickaxe-all --pickaxe-regex
- --relative --staged
- "
+ __gitcomp_builtin difftool "$__git_diff_difftool_options"
return
;;
esac
--
2.28.0.760.g8d73e04208
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH v4 05/10] diff-lib: accept option flags in run_diff_index()
2020-09-20 11:22 ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
` (3 preceding siblings ...)
2020-09-20 11:22 ` [PATCH v4 04/10] contrib/completion: extract common diff/difftool options Denton Liu
@ 2020-09-20 11:22 ` Denton Liu
2020-09-20 11:22 ` [PATCH v4 06/10] diff-lib: define diff_get_merge_base() Denton Liu
` (4 subsequent siblings)
9 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-20 11:22 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano, Jeff King
In a future commit, we will teach run_diff_index() to accept more
options via flag bits. For now, change `cached` into a flag in the
`option` bitfield. The behaviour should remain exactly the same.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
builtin/diff-index.c | 8 ++++----
builtin/diff.c | 8 ++++----
diff-lib.c | 3 ++-
diff.h | 4 +++-
4 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 93ec642423..c3878f7ad6 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -15,7 +15,7 @@ COMMON_DIFF_OPTIONS_HELP;
int cmd_diff_index(int argc, const char **argv, const char *prefix)
{
struct rev_info rev;
- int cached = 0;
+ unsigned int option = 0;
int i;
int result;
@@ -32,7 +32,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
const char *arg = argv[i];
if (!strcmp(arg, "--cached"))
- cached = 1;
+ option |= DIFF_INDEX_CACHED;
else
usage(diff_cache_usage);
}
@@ -46,7 +46,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
if (rev.pending.nr != 1 ||
rev.max_count != -1 || rev.min_age != -1 || rev.max_age != -1)
usage(diff_cache_usage);
- if (!cached) {
+ if (!(option & DIFF_INDEX_CACHED)) {
setup_work_tree();
if (read_cache_preload(&rev.diffopt.pathspec) < 0) {
perror("read_cache_preload");
@@ -56,7 +56,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
perror("read_cache");
return -1;
}
- result = run_diff_index(&rev, cached);
+ result = run_diff_index(&rev, option);
UNLEAK(rev);
return diff_result_code(&rev.diffopt, result);
}
diff --git a/builtin/diff.c b/builtin/diff.c
index cb98811c21..e45e19e37e 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -134,11 +134,11 @@ static int builtin_diff_blobs(struct rev_info *revs,
static int builtin_diff_index(struct rev_info *revs,
int argc, const char **argv)
{
- int cached = 0;
+ unsigned int option = 0;
while (1 < argc) {
const char *arg = argv[1];
if (!strcmp(arg, "--cached") || !strcmp(arg, "--staged"))
- cached = 1;
+ option |= DIFF_INDEX_CACHED;
else
usage(builtin_diff_usage);
argv++; argc--;
@@ -151,7 +151,7 @@ static int builtin_diff_index(struct rev_info *revs,
revs->max_count != -1 || revs->min_age != -1 ||
revs->max_age != -1)
usage(builtin_diff_usage);
- if (!cached) {
+ if (!(option & DIFF_INDEX_CACHED)) {
setup_work_tree();
if (read_cache_preload(&revs->diffopt.pathspec) < 0) {
perror("read_cache_preload");
@@ -161,7 +161,7 @@ static int builtin_diff_index(struct rev_info *revs,
perror("read_cache");
return -1;
}
- return run_diff_index(revs, cached);
+ return run_diff_index(revs, option);
}
static int builtin_diff_tree(struct rev_info *revs,
diff --git a/diff-lib.c b/diff-lib.c
index 50521e2093..d5b2c8af56 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -518,9 +518,10 @@ static int diff_cache(struct rev_info *revs,
return unpack_trees(1, &t, &opts);
}
-int run_diff_index(struct rev_info *revs, int cached)
+int run_diff_index(struct rev_info *revs, unsigned int option)
{
struct object_array_entry *ent;
+ int cached = !!(option & DIFF_INDEX_CACHED);
if (revs->pending.nr != 1)
BUG("run_diff_index must be passed exactly one tree");
diff --git a/diff.h b/diff.h
index e0c0af6286..aea0d5b96b 100644
--- a/diff.h
+++ b/diff.h
@@ -585,7 +585,9 @@ const char *diff_aligned_abbrev(const struct object_id *sha1, int);
/* report racily-clean paths as modified */
#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);
int do_diff_cache(const struct object_id *, struct diff_options *);
int diff_flush_patch_id(struct diff_options *, struct object_id *, int, int);
--
2.28.0.760.g8d73e04208
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH v4 06/10] diff-lib: define diff_get_merge_base()
2020-09-20 11:22 ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
` (4 preceding siblings ...)
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 ` Denton Liu
2020-09-20 11:22 ` [PATCH v4 07/10] t4068: add --merge-base tests Denton Liu
` (3 subsequent siblings)
9 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-20 11:22 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano, Jeff King
In a future commit, we will be using this function to implement
--merge-base functionality in various diff commands.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
diff-lib.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
diff.h | 2 ++
2 files changed, 47 insertions(+)
diff --git a/diff-lib.c b/diff-lib.c
index d5b2c8af56..fa64e64bbe 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -13,6 +13,7 @@
#include "submodule.h"
#include "dir.h"
#include "fsmonitor.h"
+#include "commit-reach.h"
/*
* diff-files
@@ -518,6 +519,50 @@ static int diff_cache(struct rev_info *revs,
return unpack_trees(1, &t, &opts);
}
+void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)
+{
+ int i;
+ struct commit *mb_child[2] = {0};
+ struct commit_list *merge_bases;
+
+ for (i = 0; i < revs->pending.nr; i++) {
+ struct object *obj = revs->pending.objects[i].item;
+ if (obj->flags)
+ die(_("--merge-base does not work with ranges"));
+ if (obj->type != OBJ_COMMIT)
+ die(_("--merge-base only works with commits"));
+ }
+
+ /*
+ * This check must go after the for loop above because A...B
+ * ranges produce three pending commits, resulting in a
+ * misleading error message.
+ */
+ 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 == 1) {
+ struct object_id oid;
+
+ if (get_oid("HEAD", &oid))
+ die(_("unable to get HEAD"));
+
+ mb_child[1] = lookup_commit_reference(the_repository, &oid);
+ }
+
+ merge_bases = repo_get_merge_bases(the_repository, mb_child[0], mb_child[1]);
+ if (!merge_bases)
+ die(_("no merge base found"));
+ if (merge_bases->next)
+ die(_("multiple merge bases found"));
+
+ oidcpy(mb, &merge_bases->item->object.oid);
+
+ free_commit_list(merge_bases);
+}
+
int run_diff_index(struct rev_info *revs, unsigned int option)
{
struct object_array_entry *ent;
diff --git a/diff.h b/diff.h
index aea0d5b96b..fedfeab7a2 100644
--- a/diff.h
+++ b/diff.h
@@ -580,6 +580,8 @@ void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc);
*/
const char *diff_aligned_abbrev(const struct object_id *sha1, int);
+void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);
+
/* do not report anything on removed paths */
#define DIFF_SILENT_ON_REMOVED 01
/* report racily-clean paths as modified */
--
2.28.0.760.g8d73e04208
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH v4 07/10] t4068: add --merge-base tests
2020-09-20 11:22 ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
` (5 preceding siblings ...)
2020-09-20 11:22 ` [PATCH v4 06/10] diff-lib: define diff_get_merge_base() Denton Liu
@ 2020-09-20 11:22 ` Denton Liu
2020-09-20 11:22 ` [PATCH v4 08/10] builtin/diff-index: learn --merge-base Denton Liu
` (2 subsequent siblings)
9 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-20 11:22 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano, Jeff King
In the future, we will be adding more --merge-base tests to this test
script. To prepare for that, rename the script accordingly and update
its description. Also, add two basic --merge-base tests that don't
require any functionality to be implemented yet.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
...ymmetric.sh => t4068-diff-symmetric-merge-base.sh} | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
rename t/{t4068-diff-symmetric.sh => t4068-diff-symmetric-merge-base.sh} (86%)
diff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric-merge-base.sh
similarity index 86%
rename from t/t4068-diff-symmetric.sh
rename to t/t4068-diff-symmetric-merge-base.sh
index 60c506c2b2..bd4cf254d9 100755
--- a/t/t4068-diff-symmetric.sh
+++ b/t/t4068-diff-symmetric-merge-base.sh
@@ -1,6 +1,6 @@
#!/bin/sh
-test_description='behavior of diff with symmetric-diff setups'
+test_description='behavior of diff with symmetric-diff setups and --merge-base'
. ./test-lib.sh
@@ -88,4 +88,13 @@ test_expect_success 'diff with ranges and extra arg' '
test_i18ngrep "usage" err
'
+test_expect_success 'diff --merge-base with no commits' '
+ test_must_fail git diff --merge-base
+'
+
+test_expect_success 'diff --merge-base with three commits' '
+ test_must_fail git diff --merge-base br1 br2 master 2>err &&
+ test_i18ngrep "usage" err
+'
+
test_done
--
2.28.0.760.g8d73e04208
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH v4 08/10] builtin/diff-index: learn --merge-base
2020-09-20 11:22 ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
` (6 preceding siblings ...)
2020-09-20 11:22 ` [PATCH v4 07/10] t4068: add --merge-base tests Denton Liu
@ 2020-09-20 11:22 ` 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
9 siblings, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-20 11:22 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano, Jeff King
There is currently no easy way to take the diff between the working tree
or index and the merge base between an arbitrary commit and HEAD. Even
diff's `...` notation doesn't allow this because it only works between
commits. However, the ability to do this would be desirable to a user
who would like to see all the changes they've made on a branch plus
uncommitted changes without taking into account changes made in the
upstream branch.
Teach diff-index and diff (with one commit) the --merge-base option
which allows a user to use the merge base of a commit and HEAD as the
"before" side.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Documentation/git-diff-index.txt | 7 +++-
Documentation/git-diff.txt | 12 ++++--
builtin/diff-index.c | 2 +
builtin/diff.c | 2 +
diff-lib.c | 15 ++++++-
diff.h | 1 +
t/t4068-diff-symmetric-merge-base.sh | 59 ++++++++++++++++++++++++++++
7 files changed, 92 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-diff-index.txt b/Documentation/git-diff-index.txt
index 25fe165f00..27acb31cbf 100644
--- a/Documentation/git-diff-index.txt
+++ b/Documentation/git-diff-index.txt
@@ -9,7 +9,7 @@ git-diff-index - Compare a tree to the working tree or index
SYNOPSIS
--------
[verse]
-'git diff-index' [-m] [--cached] [<common diff options>] <tree-ish> [<path>...]
+'git diff-index' [-m] [--cached] [--merge-base] [<common diff options>] <tree-ish> [<path>...]
DESCRIPTION
-----------
@@ -29,6 +29,11 @@ include::diff-options.txt[]
--cached::
Do not consider the on-disk file at all.
+--merge-base::
+ Instead of comparing <tree-ish> directly, use the merge base
+ between <tree-ish> and HEAD instead. <tree-ish> must be a
+ commit.
+
-m::
By default, files recorded in the index but not checked
out are reported as deleted. This flag makes
diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 8f7b4ed3ca..762ee6d074 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -10,7 +10,7 @@ SYNOPSIS
--------
[verse]
'git diff' [<options>] [<commit>] [--] [<path>...]
-'git diff' [<options>] --cached [<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>] <blob> <blob>
@@ -40,7 +40,7 @@ files on disk.
or when running the command outside a working tree
controlled by Git. This form implies `--exit-code`.
-'git diff' [<options>] --cached [<commit>] [--] [<path>...]::
+'git diff' [<options>] --cached [--merge-base] [<commit>] [--] [<path>...]::
This form is to view the changes you staged for the next
commit relative to the named <commit>. Typically you
@@ -49,6 +49,10 @@ files on disk.
If HEAD does not exist (e.g. unborn branches) and
<commit> is not given, it shows all staged changes.
--staged is a synonym of --cached.
++
+If --merge-base is given, instead of using <commit>, use the merge base
+of <commit> and HEAD. `git diff --merge-base A` is equivalent to
+`git diff $(git merge-base A HEAD)`.
'git diff' [<options>] <commit> [--] [<path>...]::
@@ -89,8 +93,8 @@ files on disk.
Just in case you are doing something exotic, it should be
noted that all of the <commit> in the above description, except
-in the last two forms that use `..` notations, can be any
-<tree>.
+in the `--merge-base` case and in the last two forms that use `..`
+notations, can be any <tree>.
For a more complete list of ways to spell <commit>, see
"SPECIFYING REVISIONS" section in linkgit:gitrevisions[7].
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index c3878f7ad6..7f5281c461 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -33,6 +33,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
if (!strcmp(arg, "--cached"))
option |= DIFF_INDEX_CACHED;
+ else if (!strcmp(arg, "--merge-base"))
+ option |= DIFF_INDEX_MERGE_BASE;
else
usage(diff_cache_usage);
}
diff --git a/builtin/diff.c b/builtin/diff.c
index e45e19e37e..1baea18ae0 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -139,6 +139,8 @@ static int builtin_diff_index(struct rev_info *revs,
const char *arg = argv[1];
if (!strcmp(arg, "--cached") || !strcmp(arg, "--staged"))
option |= DIFF_INDEX_CACHED;
+ else if (!strcmp(arg, "--merge-base"))
+ option |= DIFF_INDEX_MERGE_BASE;
else
usage(builtin_diff_usage);
argv++; argc--;
diff --git a/diff-lib.c b/diff-lib.c
index fa64e64bbe..79defdc6b8 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -567,13 +567,26 @@ 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, cached))
+
+ if (merge_base) {
+ diff_get_merge_base(revs, &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, cached))
exit(128);
diff_set_mnemonic_prefix(&revs->diffopt, "c/", cached ? "i/" : "w/");
diff --git a/diff.h b/diff.h
index fedfeab7a2..6c2efa16fd 100644
--- a/diff.h
+++ b/diff.h
@@ -589,6 +589,7 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);
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);
int do_diff_cache(const struct object_id *, struct diff_options *);
diff --git a/t/t4068-diff-symmetric-merge-base.sh b/t/t4068-diff-symmetric-merge-base.sh
index bd4cf254d9..49432379cb 100755
--- a/t/t4068-diff-symmetric-merge-base.sh
+++ b/t/t4068-diff-symmetric-merge-base.sh
@@ -97,4 +97,63 @@ test_expect_success 'diff --merge-base with three commits' '
test_i18ngrep "usage" err
'
+for cmd in diff-index diff
+do
+ test_expect_success "$cmd --merge-base with one commit" '
+ git checkout master &&
+ git $cmd commit-C >expect &&
+ git $cmd --merge-base br2 >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "$cmd --merge-base with one commit and unstaged changes" '
+ git checkout master &&
+ test_when_finished git reset --hard &&
+ echo unstaged >>c &&
+ git $cmd commit-C >expect &&
+ git $cmd --merge-base br2 >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "$cmd --merge-base with one commit and staged and unstaged changes" '
+ git checkout master &&
+ test_when_finished git reset --hard &&
+ echo staged >>c &&
+ git add c &&
+ echo unstaged >>c &&
+ git $cmd commit-C >expect &&
+ git $cmd --merge-base br2 >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "$cmd --merge-base --cached with one commit and staged and unstaged changes" '
+ git checkout master &&
+ test_when_finished git reset --hard &&
+ echo staged >>c &&
+ git add c &&
+ echo unstaged >>c &&
+ git $cmd --cached commit-C >expect &&
+ git $cmd --cached --merge-base br2 >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "$cmd --merge-base with non-commit" '
+ git checkout master &&
+ test_must_fail git $cmd --merge-base master^{tree} 2>err &&
+ test_i18ngrep "fatal: --merge-base only works with commits" err
+ '
+
+ test_expect_success "$cmd --merge-base with no merge bases and one commit" '
+ git checkout master &&
+ test_must_fail git $cmd --merge-base br3 2>err &&
+ test_i18ngrep "fatal: no merge base found" err
+ '
+
+ test_expect_success "$cmd --merge-base with multiple merge bases and one commit" '
+ git checkout master &&
+ test_must_fail git $cmd --merge-base br1 2>err &&
+ test_i18ngrep "fatal: multiple merge bases found" err
+ '
+done
+
test_done
--
2.28.0.760.g8d73e04208
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH v4 08/10] builtin/diff-index: learn --merge-base
2020-09-20 11:22 ` [PATCH v4 08/10] builtin/diff-index: learn --merge-base Denton Liu
@ 2020-09-29 19:53 ` Martin Ågren
0 siblings, 0 replies; 58+ messages in thread
From: Martin Ågren @ 2020-09-29 19:53 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List, Junio C Hamano, Jeff King
Hi Denton,
On Sun, 20 Sep 2020 at 13:24, Denton Liu <liu.denton@gmail.com> wrote:
> --- a/Documentation/git-diff-index.txt
> +++ b/Documentation/git-diff-index.txt
> +--merge-base::
> + Instead of comparing <tree-ish> directly, use the merge base
> + between <tree-ish> and HEAD instead. <tree-ish> must be a
> + commit.
If you end up rerolling this patch series for other reasons, you might
want to consider using `backticks` around `<tree-ish>` and `HEAD` so
they get typeset as monospace.
> If HEAD does not exist (e.g. unborn branches) and
> <commit> is not given, it shows all staged changes.
> --staged is a synonym of --cached.
> ++
> +If --merge-base is given, instead of using <commit>, use the merge base
> +of <commit> and HEAD. `git diff --merge-base A` is equivalent to
> +`git diff $(git merge-base A HEAD)`.
Similarly here. (I realize there are existing offenders. I think it's
fine or even preferable to leave them as they are so that you don't get
lost in a huge while-at-it.) This also applies to the next patch which
touches git-diff-tree.txt.
Martin
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v4 09/10] builtin/diff-tree: learn --merge-base
2020-09-20 11:22 ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
` (7 preceding siblings ...)
2020-09-20 11:22 ` [PATCH v4 08/10] builtin/diff-index: learn --merge-base Denton Liu
@ 2020-09-20 11:22 ` Denton Liu
2020-09-20 11:22 ` [PATCH v4 10/10] contrib/completion: complete `git diff --merge-base` Denton Liu
9 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-20 11:22 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano, Jeff King
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 an alternative to triple-dot
notation. Thus, we would be able to write the above as
`git diff --merge-base A B`.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Documentation/git-diff-tree.txt | 7 ++++-
Documentation/git-diff.txt | 8 ++++--
builtin/diff-tree.c | 18 +++++++++++++
builtin/diff.c | 39 +++++++++++++++++++---------
t/t4068-diff-symmetric-merge-base.sh | 34 ++++++++++++++++++++++++
5 files changed, 91 insertions(+), 15 deletions(-)
diff --git a/Documentation/git-diff-tree.txt b/Documentation/git-diff-tree.txt
index 5c8a2a5e97..2fc24c542f 100644
--- a/Documentation/git-diff-tree.txt
+++ b/Documentation/git-diff-tree.txt
@@ -10,7 +10,7 @@ SYNOPSIS
--------
[verse]
'git diff-tree' [--stdin] [-m] [-s] [-v] [--no-commit-id] [--pretty]
- [-t] [-r] [-c | --cc] [--combined-all-paths] [--root]
+ [-t] [-r] [-c | --cc] [--combined-all-paths] [--root] [--merge-base]
[<common diff options>] <tree-ish> [<tree-ish>] [<path>...]
DESCRIPTION
@@ -43,6 +43,11 @@ include::diff-options.txt[]
When `--root` is specified the initial commit will be shown as a big
creation event. This is equivalent to a diff against the NULL tree.
+--merge-base::
+ Instead of comparing the <tree-ish>s directly, use the merge
+ base between the two <tree-ish>s as the "before" side. There
+ must be two <tree-ish>s given and they must both be commits.
+
--stdin::
When `--stdin` is specified, the command does not take
<tree-ish> arguments from the command line. Instead, it
diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 762ee6d074..7f4c8a8ce7 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -11,7 +11,7 @@ SYNOPSIS
[verse]
'git diff' [<options>] [<commit>] [--] [<path>...]
'git diff' [<options>] --cached [--merge-base] [<commit>] [--] [<path>...]
-'git diff' [<options>] <commit> [<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>
@@ -62,10 +62,14 @@ of <commit> and HEAD. `git diff --merge-base A` is equivalent to
branch name to compare with the tip of a different
branch.
-'git diff' [<options>] <commit> <commit> [--] [<path>...]::
+'git diff' [<options>] [--merge-base] <commit> <commit> [--] [<path>...]::
This is to view the changes between two arbitrary
<commit>.
++
+If --merge-base is given, use the merge base of the two commits for the
+"before" side. `git diff --merge-base A B` is equivalent to
+`git diff $(git merge-base A B) B`.
'git diff' [<options>] <commit> <commit>... <commit> [--] [<path>...]::
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 802363d0a2..823d6678e5 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -111,6 +111,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
struct setup_revision_opt s_r_opt;
struct userformat_want w;
int read_stdin = 0;
+ int merge_base = 0;
if (argc == 2 && !strcmp(argv[1], "-h"))
usage(diff_tree_usage);
@@ -143,9 +144,26 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
read_stdin = 1;
continue;
}
+ if (!strcmp(arg, "--merge-base")) {
+ merge_base = 1;
+ continue;
+ }
usage(diff_tree_usage);
}
+ if (read_stdin && merge_base)
+ die(_("--stdin and --merge-base are mutually exclusive"));
+
+ if (merge_base) {
+ struct object_id oid;
+
+ if (opt->pending.nr != 2)
+ die(_("--merge-base only works with two commits"));
+
+ diff_get_merge_base(opt, &oid);
+ opt->pending.objects[0].item = lookup_object(the_repository, &oid);
+ }
+
/*
* NOTE! We expect "a..b" to expand to "^a b" but it is
* perfectly valid for revision range parser to yield "b ^a",
diff --git a/builtin/diff.c b/builtin/diff.c
index 1baea18ae0..b50fc68c2a 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -26,7 +26,7 @@
static const char builtin_diff_usage[] =
"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> [--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"
@@ -172,19 +172,34 @@ static int builtin_diff_tree(struct rev_info *revs,
struct object_array_entry *ent1)
{
const struct object_id *(oid[2]);
- int swap = 0;
+ struct object_id mb_oid;
+ int merge_base = 0;
- if (argc > 1)
- usage(builtin_diff_usage);
+ while (1 < argc) {
+ const char *arg = argv[1];
+ if (!strcmp(arg, "--merge-base"))
+ merge_base = 1;
+ else
+ usage(builtin_diff_usage);
+ argv++; argc--;
+ }
- /*
- * We saw two trees, ent0 and ent1. If ent1 is uninteresting,
- * swap them.
- */
- if (ent1->item->flags & UNINTERESTING)
- swap = 1;
- oid[swap] = &ent0->item->oid;
- oid[1 - swap] = &ent1->item->oid;
+ if (merge_base) {
+ diff_get_merge_base(revs, &mb_oid);
+ oid[0] = &mb_oid;
+ oid[1] = &revs->pending.objects[1].item->oid;
+ } else {
+ int swap = 0;
+
+ /*
+ * We saw two trees, ent0 and ent1. If ent1 is uninteresting,
+ * swap them.
+ */
+ if (ent1->item->flags & UNINTERESTING)
+ swap = 1;
+ oid[swap] = &ent0->item->oid;
+ oid[1 - swap] = &ent1->item->oid;
+ }
diff_tree_oid(oid[0], oid[1], "", &revs->diffopt);
log_tree_diff_flush(revs);
return 0;
diff --git a/t/t4068-diff-symmetric-merge-base.sh b/t/t4068-diff-symmetric-merge-base.sh
index 49432379cb..03487cc945 100755
--- a/t/t4068-diff-symmetric-merge-base.sh
+++ b/t/t4068-diff-symmetric-merge-base.sh
@@ -156,4 +156,38 @@ do
'
done
+for cmd in diff-tree diff
+do
+ test_expect_success "$cmd --merge-base with two commits" '
+ git $cmd commit-C master >expect &&
+ git $cmd --merge-base br2 master >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "$cmd --merge-base commit and non-commit" '
+ test_must_fail git $cmd --merge-base br2 master^{tree} 2>err &&
+ test_i18ngrep "fatal: --merge-base only works with commits" err
+ '
+
+ test_expect_success "$cmd --merge-base with no merge bases and two commits" '
+ test_must_fail git $cmd --merge-base br2 br3 2>err &&
+ test_i18ngrep "fatal: no merge base found" err
+ '
+
+ test_expect_success "$cmd --merge-base with multiple merge bases and two commits" '
+ test_must_fail git $cmd --merge-base master br1 2>err &&
+ test_i18ngrep "fatal: multiple merge bases found" err
+ '
+done
+
+test_expect_success 'diff-tree --merge-base with one commit' '
+ test_must_fail git diff-tree --merge-base master 2>err &&
+ test_i18ngrep "fatal: --merge-base only works with two commits" err
+'
+
+test_expect_success 'diff --merge-base with range' '
+ test_must_fail git diff --merge-base br2..br3 2>err &&
+ test_i18ngrep "fatal: --merge-base does not work with ranges" err
+'
+
test_done
--
2.28.0.760.g8d73e04208
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH v4 10/10] contrib/completion: complete `git diff --merge-base`
2020-09-20 11:22 ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
` (8 preceding siblings ...)
2020-09-20 11:22 ` [PATCH v4 09/10] builtin/diff-tree: " Denton Liu
@ 2020-09-20 11:22 ` Denton Liu
9 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-20 11:22 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano, Jeff King
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
contrib/completion/git-completion.bash | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index f68c8e0646..679d1ec8a8 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1692,7 +1692,7 @@ __git_diff_common_options="--stat --numstat --shortstat --summary
"
__git_diff_difftool_options="--cached --staged --pickaxe-all --pickaxe-regex
- --base --ours --theirs --no-index --relative
+ --base --ours --theirs --no-index --relative --merge-base
$__git_diff_common_options"
_git_diff ()
--
2.28.0.760.g8d73e04208
^ permalink raw reply related [flat|nested] 58+ messages in thread