* [PATCH 0/3] Change "--diff-merges" to require parameter @ 2020-08-05 22:08 Sergey Organov 2020-08-05 22:08 ` [PATCH 1/3] revision: change "--diff-merges" option " Sergey Organov ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Sergey Organov @ 2020-08-05 22:08 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git, Sergey Organov These patches present a minimal set of changes to turn --diff-merges to require parameter. For now we only introduce --diff-merges=off as synonym for --no-diff-merges. These are on top of jk/log-fp-implies-m "making --first-parent imply -m" by Jeff King, that introduced --[no-]diff-merges as a boolean option to provide ability to override implied -m by --no-diff-merges. Sergey Organov (3): revision: change "--diff-merges" option to require parameter doc/git-log: describe --diff-merges=off t/t4013: add test for --diff-merges=off Documentation/git-log.txt | 6 +- revision.c | 9 ++- t/t4013-diff-various.sh | 1 + ...--diff-merges=off_-p_--first-parent_master | 78 +++++++++++++++++++ 4 files changed, 92 insertions(+), 2 deletions(-) create mode 100644 t/t4013/diff.log_--diff-merges=off_-p_--first-parent_master -- 2.20.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/3] revision: change "--diff-merges" option to require parameter 2020-08-05 22:08 [PATCH 0/3] Change "--diff-merges" to require parameter Sergey Organov @ 2020-08-05 22:08 ` Sergey Organov 2020-08-05 22:08 ` [PATCH 2/3] doc/git-log: describe --diff-merges=off Sergey Organov 2020-08-05 22:08 ` [PATCH 3/3] t/t4013: add test for --diff-merges=off Sergey Organov 2 siblings, 0 replies; 17+ messages in thread From: Sergey Organov @ 2020-08-05 22:08 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git, Sergey Organov --diff-merges=off is the only accepted form for now, a synonym for --no-diff-merges. This patch is a preparation for adding more values, as well as supporting --diff-merges=<parent>, where <parent> is single parent number to output diff against. Signed-off-by: Sergey Organov <sorganov@gmail.com> --- revision.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/revision.c b/revision.c index 669bc856694f..417659cfcb10 100644 --- a/revision.c +++ b/revision.c @@ -2323,8 +2323,15 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg revs->diff = 1; revs->diffopt.flags.recursive = 1; revs->diffopt.flags.tree_in_recursive = 1; - } else if (!strcmp(arg, "-m") || !strcmp(arg, "--diff-merges")) { + } else if (!strcmp(arg, "-m")) { revs->ignore_merges = 0; + } else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) { + if (!strcmp(optarg, "off")) { + revs->ignore_merges = 1; + } else { + die(_("unknown value for --diff-merges: %s"), optarg); + } + return argcount; } else if (!strcmp(arg, "--no-diff-merges")) { revs->ignore_merges = 1; } else if (!strcmp(arg, "-c")) { -- 2.20.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/3] doc/git-log: describe --diff-merges=off 2020-08-05 22:08 [PATCH 0/3] Change "--diff-merges" to require parameter Sergey Organov 2020-08-05 22:08 ` [PATCH 1/3] revision: change "--diff-merges" option " Sergey Organov @ 2020-08-05 22:08 ` Sergey Organov 2020-08-12 0:06 ` Junio C Hamano 2020-08-05 22:08 ` [PATCH 3/3] t/t4013: add test for --diff-merges=off Sergey Organov 2 siblings, 1 reply; 17+ messages in thread From: Sergey Organov @ 2020-08-05 22:08 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git, Sergey Organov Signed-off-by: Sergey Organov <sorganov@gmail.com> --- Documentation/git-log.txt | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt index 9ccba65469d7..f3727c786453 100644 --- a/Documentation/git-log.txt +++ b/Documentation/git-log.txt @@ -145,7 +145,6 @@ combined-diff option or with `--no-diff-merges`). rename or copy detection have been requested). -m:: ---diff-merges:: This flag makes the merge commits show the full diff like regular commits; for each merge parent, a separate log entry and diff is generated. An exception is that only diff against @@ -153,6 +152,11 @@ combined-diff option or with `--no-diff-merges`). in that case, the output represents the changes the merge brought _into_ the then-current branch. +--diff-merges=off:: +--no-diff-merges:: + Disable output of diffs for merge commits (default). Useful to + override `-m`, `-c`, or `--cc`. + :git-log: 1 include::diff-options.txt[] -- 2.20.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] doc/git-log: describe --diff-merges=off 2020-08-05 22:08 ` [PATCH 2/3] doc/git-log: describe --diff-merges=off Sergey Organov @ 2020-08-12 0:06 ` Junio C Hamano 2020-08-12 0:48 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2020-08-12 0:06 UTC (permalink / raw) To: Sergey Organov; +Cc: Jeff King, git Sergey Organov <sorganov@gmail.com> writes: > Signed-off-by: Sergey Organov <sorganov@gmail.com> > --- > Documentation/git-log.txt | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt > index 9ccba65469d7..f3727c786453 100644 > --- a/Documentation/git-log.txt > +++ b/Documentation/git-log.txt > @@ -145,7 +145,6 @@ combined-diff option or with `--no-diff-merges`). > rename or copy detection have been requested). > > -m:: > ---diff-merges:: Shouldn't this "--diff-merges" be removed from here? As [1/3] updated it like so: - } else if (!strcmp(arg, "-m") || !strcmp(arg, "--diff-merges")) { + } else if (!strcmp(arg, "-m")) { revs->ignore_merges = 0; + } else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) { + if (!strcmp(optarg, "off")) { + revs->ignore_merges = 1; + } else { + die(_("unknown value for --diff-merges: %s"), optarg); + } + return argcount; "git log --diff-merges" would get either an "option --diff-merges requires a value" error from diff.c::parse_long_opt(), or an "unknown value for --diff-merges: <opt>" error from the above code. Other than that, I think 1&2/3 looks good, and we've covered the tests with 3/3 already, so we are in a reasonably good shape. Thanks. > This flag makes the merge commits show the full diff like > regular commits; for each merge parent, a separate log entry > and diff is generated. An exception is that only diff against > @@ -153,6 +152,11 @@ combined-diff option or with `--no-diff-merges`). > in that case, the output represents the changes the merge > brought _into_ the then-current branch. > > +--diff-merges=off:: > +--no-diff-merges:: > + Disable output of diffs for merge commits (default). Useful to > + override `-m`, `-c`, or `--cc`. > + > :git-log: 1 > include::diff-options.txt[] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] doc/git-log: describe --diff-merges=off 2020-08-12 0:06 ` Junio C Hamano @ 2020-08-12 0:48 ` Junio C Hamano 2020-08-12 8:08 ` Sergey Organov 0 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2020-08-12 0:48 UTC (permalink / raw) To: Sergey Organov; +Cc: Jeff King, git Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> Signed-off-by: Sergey Organov <sorganov@gmail.com> >> --- >> Documentation/git-log.txt | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt >> index 9ccba65469d7..f3727c786453 100644 >> --- a/Documentation/git-log.txt >> +++ b/Documentation/git-log.txt >> @@ -145,7 +145,6 @@ combined-diff option or with `--no-diff-merges`). >> rename or copy detection have been requested). >> >> -m:: >> ---diff-merges:: > > Shouldn't this "--diff-merges" be removed from here? Sorry, my eyes. Yes, you are removing it from here. All is well, then. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] doc/git-log: describe --diff-merges=off 2020-08-12 0:48 ` Junio C Hamano @ 2020-08-12 8:08 ` Sergey Organov 0 siblings, 0 replies; 17+ messages in thread From: Sergey Organov @ 2020-08-12 8:08 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git Junio C Hamano <gitster@pobox.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Sergey Organov <sorganov@gmail.com> writes: >> >>> Signed-off-by: Sergey Organov <sorganov@gmail.com> >>> --- >>> Documentation/git-log.txt | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt >>> index 9ccba65469d7..f3727c786453 100644 >>> --- a/Documentation/git-log.txt >>> +++ b/Documentation/git-log.txt >>> @@ -145,7 +145,6 @@ combined-diff option or with `--no-diff-merges`). >>> rename or copy detection have been requested). >>> >>> -m:: >>> ---diff-merges:: >> >> Shouldn't this "--diff-merges" be removed from here? > > Sorry, my eyes. Yes, you are removing it from here. Don't mention it, -- these "---" caught me as well when I first looked at the diff! Thanks, -- Sergey ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/3] t/t4013: add test for --diff-merges=off 2020-08-05 22:08 [PATCH 0/3] Change "--diff-merges" to require parameter Sergey Organov 2020-08-05 22:08 ` [PATCH 1/3] revision: change "--diff-merges" option " Sergey Organov 2020-08-05 22:08 ` [PATCH 2/3] doc/git-log: describe --diff-merges=off Sergey Organov @ 2020-08-05 22:08 ` Sergey Organov 2020-08-05 22:31 ` Junio C Hamano 2 siblings, 1 reply; 17+ messages in thread From: Sergey Organov @ 2020-08-05 22:08 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git, Sergey Organov Signed-off-by: Sergey Organov <sorganov@gmail.com> --- t/t4013-diff-various.sh | 1 + ...--diff-merges=off_-p_--first-parent_master | 78 +++++++++++++++++++ 2 files changed, 79 insertions(+) create mode 100644 t/t4013/diff.log_--diff-merges=off_-p_--first-parent_master diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh index 40e222c94520..86fb11cecc61 100755 --- a/t/t4013-diff-various.sh +++ b/t/t4013-diff-various.sh @@ -298,6 +298,7 @@ log --root -c --patch-with-stat --summary master # improved by Timo's patch log --root --cc --patch-with-stat --summary master log --no-diff-merges -p --first-parent master +log --diff-merges=off -p --first-parent master log -p --first-parent master log -m -p --first-parent master log -m -p master diff --git a/t/t4013/diff.log_--diff-merges=off_-p_--first-parent_master b/t/t4013/diff.log_--diff-merges=off_-p_--first-parent_master new file mode 100644 index 000000000000..c878f13c9519 --- /dev/null +++ b/t/t4013/diff.log_--diff-merges=off_-p_--first-parent_master @@ -0,0 +1,78 @@ +$ git log --diff-merges=off -p --first-parent master +commit 59d314ad6f356dd08601a4cd5e530381da3e3c64 +Merge: 9a6d494 c7a2ab9 +Author: A U Thor <author@example.com> +Date: Mon Jun 26 00:04:00 2006 +0000 + + Merge branch 'side' into master + +commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0 +Author: A U Thor <author@example.com> +Date: Mon Jun 26 00:02:00 2006 +0000 + + Third + +diff --git a/dir/sub b/dir/sub +index 8422d40..cead32e 100644 +--- a/dir/sub ++++ b/dir/sub +@@ -2,3 +2,5 @@ A + B + C + D ++E ++F +diff --git a/file1 b/file1 +new file mode 100644 +index 0000000..b1e6722 +--- /dev/null ++++ b/file1 +@@ -0,0 +1,3 @@ ++A ++B ++C + +commit 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44 +Author: A U Thor <author@example.com> +Date: Mon Jun 26 00:01:00 2006 +0000 + + Second + + This is the second commit. + +diff --git a/dir/sub b/dir/sub +index 35d242b..8422d40 100644 +--- a/dir/sub ++++ b/dir/sub +@@ -1,2 +1,4 @@ + A + B ++C ++D +diff --git a/file0 b/file0 +index 01e79c3..b414108 100644 +--- a/file0 ++++ b/file0 +@@ -1,3 +1,6 @@ + 1 + 2 + 3 ++4 ++5 ++6 +diff --git a/file2 b/file2 +deleted file mode 100644 +index 01e79c3..0000000 +--- a/file2 ++++ /dev/null +@@ -1,3 +0,0 @@ +-1 +-2 +-3 + +commit 444ac553ac7612cc88969031b02b3767fb8a353a +Author: A U Thor <author@example.com> +Date: Mon Jun 26 00:00:00 2006 +0000 + + Initial +$ -- 2.20.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] t/t4013: add test for --diff-merges=off 2020-08-05 22:08 ` [PATCH 3/3] t/t4013: add test for --diff-merges=off Sergey Organov @ 2020-08-05 22:31 ` Junio C Hamano 2020-08-05 22:44 ` Sergey Organov 0 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2020-08-05 22:31 UTC (permalink / raw) To: Sergey Organov; +Cc: Jeff King, git Sergey Organov <sorganov@gmail.com> writes: > Signed-off-by: Sergey Organov <sorganov@gmail.com> > --- > t/t4013-diff-various.sh | 1 + > ...--diff-merges=off_-p_--first-parent_master | 78 +++++++++++++++++++ > 2 files changed, 79 insertions(+) > create mode 100644 t/t4013/diff.log_--diff-merges=off_-p_--first-parent_master > > diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh > index 40e222c94520..86fb11cecc61 100755 > --- a/t/t4013-diff-various.sh > +++ b/t/t4013-diff-various.sh > @@ -298,6 +298,7 @@ log --root -c --patch-with-stat --summary master > # improved by Timo's patch > log --root --cc --patch-with-stat --summary master > log --no-diff-merges -p --first-parent master > +log --diff-merges=off -p --first-parent master I think we want to also test it in a different order, e.g. log --first-parent --diff-merges=off -p master other than that, these three patches all look good to me. Thanks. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] t/t4013: add test for --diff-merges=off 2020-08-05 22:31 ` Junio C Hamano @ 2020-08-05 22:44 ` Sergey Organov 2020-08-05 23:19 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Sergey Organov @ 2020-08-05 22:44 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> Signed-off-by: Sergey Organov <sorganov@gmail.com> >> --- >> t/t4013-diff-various.sh | 1 + >> ...--diff-merges=off_-p_--first-parent_master | 78 +++++++++++++++++++ >> 2 files changed, 79 insertions(+) >> create mode 100644 >> t/t4013/diff.log_--diff-merges=off_-p_--first-parent_master >> >> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh >> index 40e222c94520..86fb11cecc61 100755 >> --- a/t/t4013-diff-various.sh >> +++ b/t/t4013-diff-various.sh >> @@ -298,6 +298,7 @@ log --root -c --patch-with-stat --summary master >> # improved by Timo's patch >> log --root --cc --patch-with-stat --summary master >> log --no-diff-merges -p --first-parent master >> +log --diff-merges=off -p --first-parent master > > I think we want to also test it in a different order, e.g. > > log --first-parent --diff-merges=off -p master Dunno, why original Jeff series didn't need log --first-parent --no-diff-merges -p master then? > other than that, these three patches all look good to me. Thanks, -- Sergey ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] t/t4013: add test for --diff-merges=off 2020-08-05 22:44 ` Sergey Organov @ 2020-08-05 23:19 ` Junio C Hamano 2020-08-05 23:44 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2020-08-05 23:19 UTC (permalink / raw) To: Sergey Organov; +Cc: Jeff King, git Sergey Organov <sorganov@gmail.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Sergey Organov <sorganov@gmail.com> writes: >> >>> Signed-off-by: Sergey Organov <sorganov@gmail.com> >>> --- >>> t/t4013-diff-various.sh | 1 + >>> ...--diff-merges=off_-p_--first-parent_master | 78 +++++++++++++++++++ >>> 2 files changed, 79 insertions(+) >>> create mode 100644 >>> t/t4013/diff.log_--diff-merges=off_-p_--first-parent_master >>> >>> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh >>> index 40e222c94520..86fb11cecc61 100755 >>> --- a/t/t4013-diff-various.sh >>> +++ b/t/t4013-diff-various.sh >>> @@ -298,6 +298,7 @@ log --root -c --patch-with-stat --summary master >>> # improved by Timo's patch >>> log --root --cc --patch-with-stat --summary master >>> log --no-diff-merges -p --first-parent master >>> +log --diff-merges=off -p --first-parent master >> >> I think we want to also test it in a different order, e.g. >> >> log --first-parent --diff-merges=off -p master > > Dunno, why original Jeff series didn't need > > log --first-parent --no-diff-merges -p master > > then? Who said it didn't need it? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] t/t4013: add test for --diff-merges=off 2020-08-05 23:19 ` Junio C Hamano @ 2020-08-05 23:44 ` Junio C Hamano 2020-08-06 8:34 ` Sergey Organov 0 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2020-08-05 23:44 UTC (permalink / raw) To: Sergey Organov; +Cc: Jeff King, git Junio C Hamano <gitster@pobox.com> writes: >> Dunno, why original Jeff series didn't need >> >> log --first-parent --no-diff-merges -p master >> >> then? > > Who said it didn't need it? To elaborate a bit more, we are all humans, and even reviewers should be given the second chance to suggest improvements to things that can use them, which s/he previously missed. If we keep saying "you say this is wrong, but the other guy did the same wrong thing last week", we can never make the world better. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] t/t4013: add test for --diff-merges=off 2020-08-05 23:44 ` Junio C Hamano @ 2020-08-06 8:34 ` Sergey Organov 2020-08-06 17:10 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Sergey Organov @ 2020-08-06 8:34 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git Junio C Hamano <gitster@pobox.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >>> Dunno, why original Jeff series didn't need >>> >>> log --first-parent --no-diff-merges -p master >>> >>> then? >> >> Who said it didn't need it? > > To elaborate a bit more, we are all humans, and even reviewers > should be given the second chance to suggest improvements to things > that can use them, which s/he previously missed. If we keep saying > "you say this is wrong, but the other guy did the same wrong thing > last week", we can never make the world better. I asked because I thought you see some essential difference between two tests, as you didn't suggest to add similar permutation test to the original. I think this reply resolves my doubt. I still think this additional test not needed unless we are going to test all the permutations, and there are already 6 of them even for this simple test, so we'd need to add 10 more tests to the 2 originals. If somebody is willing to implement machinery to test all the option permutations automatically... But that's out of scope of these patches. To be honest, if we don't test all the permutations anyway, I'd rather test most "human" variant only, that to me is: log --first-parent -p --no-diff-merges master because --first-parent tells how to travel in history, and the rest tells how to represent results, starting from generic "show me diffs" and followed by "oh, but don't bother me with merge diffs, please". And now we are back to my original suggestion to invert the order of parameters in these tests, but approached from another direction ;) Overall, I can obviously add such a test to the series if you insist, but to me it still looks pointless. What doesn't look pointless is replacing the original with the version you suggested, for the reasons that I've already explained in preliminary discussions of these patches. Finally, if you still insist on additional test(s), please also tell if I should add similar permutation to the original --no-diff-merges test. Thanks, -- Sergey ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] t/t4013: add test for --diff-merges=off 2020-08-06 8:34 ` Sergey Organov @ 2020-08-06 17:10 ` Junio C Hamano 2020-08-06 20:52 ` Sergey Organov 2020-08-07 23:11 ` Sergey Organov 0 siblings, 2 replies; 17+ messages in thread From: Junio C Hamano @ 2020-08-06 17:10 UTC (permalink / raw) To: Sergey Organov; +Cc: Jeff King, git Sergey Organov <sorganov@gmail.com> writes: > I asked because I thought you see some essential difference between two > tests, as you didn't suggest to add similar permutation test to the > original. I think this reply resolves my doubt. Yeah, I didn't explain this very well. The thing is that the way "--first-parent" interacts with other options that countermand "--diff-merges" (i.e. "--no-diff-merges" and "--diff-merges=off") needs to be highlighted with extra clarity, simply because "--first-parent" is different from a simple "combination" of "follow the first parent commit chain while traversing" and "--diff-merges" [*1*] It merely "implies" (or "defaults", which is the usual word we use) "--diff-merges", so anything the user says explicitly on the command line countermands it, i.e. git log --first-parent --no-diff-merges -p does not trigger "--diff-merges" because the user explicitly says that diffs for patches are unwanted after saying "--first-parent" (i.e. without "--no-diff-merges" later, we would have done the "diff-merges" implied by "--first-parent"). And git log --no-diff-merges --first-parent -p does not trigger "--diff-merges" because the user explicitly said that diffs for patches are unwanted by the time "--first-parent" is seen (i.e. without "--no-diff-merges" upfront, "--first-parent" would have weighed in to enable "--diff-merges" behavior, but because the user already said "no", it wouldn't). This is quite different from options that are orthogonal to each other. We do not need permutations of "log --root --cc" vs "log --cc --root" tested. We know from the code that they cannot possibly interact with each other. We could still test the permutations, and it may give us more "complete" coverage, but it may be of very low value. As to permutations of additive options, we do want to make sure they are truly additive (as opposed to being the "last one wins") and as a side effect of ensuring the additive nature, we would end up testing the effect of the order of options given, e.g. "git commit -m paragraph1 -m paragraph2". As to permutations of options that directly overrides or opposes each other, we may want to ensure e.g. that "git log -p -U2 -U4" uses four context lines and "git log -p -U4 -U2" uses two or that "git format-patch --attach --no-attach" does not use attachment and "git format-patch --no-attach --attach" does (i.e. "last one wins"). But again, especially for commands that use parse-options API correctly to implement their command line options, it is hard to get these two cases wrong (e.g. you need to deliberately write a callback to make "log -p -U2 -U4" additive to use six context lines), so it may be of lower value to throughly test permutations. It would not be as low as testing permutations of unrelated options, though. [Footnote] *1* It makes it worse that the "-m" option itself changes behavior (whether it is given explicitly or implied) when used together with "--first-parent". Unlike "git log -m -p" without "--first-parent", you cannot see the difference the mainline brought in to the side branch when the "--first-parent" option is in use. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] t/t4013: add test for --diff-merges=off 2020-08-06 17:10 ` Junio C Hamano @ 2020-08-06 20:52 ` Sergey Organov 2020-08-07 23:11 ` Sergey Organov 1 sibling, 0 replies; 17+ messages in thread From: Sergey Organov @ 2020-08-06 20:52 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> I asked because I thought you see some essential difference between two >> tests, as you didn't suggest to add similar permutation test to the >> original. I think this reply resolves my doubt. > > Yeah, I didn't explain this very well. > > The thing is that the way "--first-parent" interacts with other > options that countermand "--diff-merges" (i.e. "--no-diff-merges" > and "--diff-merges=off") needs to be highlighted with extra clarity, > simply because "--first-parent" is different from a simple > "combination" of "follow the first parent commit chain while > traversing" and "--diff-merges" [*1*] [...] Thanks a lot for thorough explanations, Junio! I appreciate it and will re-read them more carefully, in the morning. For now, can we leave these patches as-is and then add additional tests on top, please? I'm afraid that adding them now will force me to rewrite current descriptions, as the result won't be minimal anymore, and I feel uneasy about writing commit messages for these permuted tests anyway. Thanks, -- Sergey ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] t/t4013: add test for --diff-merges=off 2020-08-06 17:10 ` Junio C Hamano 2020-08-06 20:52 ` Sergey Organov @ 2020-08-07 23:11 ` Sergey Organov 2020-08-17 17:17 ` Junio C Hamano 1 sibling, 1 reply; 17+ messages in thread From: Sergey Organov @ 2020-08-07 23:11 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> I asked because I thought you see some essential difference between two >> tests, as you didn't suggest to add similar permutation test to the >> original. I think this reply resolves my doubt. > > Yeah, I didn't explain this very well. I think you actually did in a previous thread where I've suggested to change the order in the existing test to relax it slightly, but more explanations never hurt. > > The thing is that the way "--first-parent" interacts with other > options that countermand "--diff-merges" (i.e. "--no-diff-merges" > and "--diff-merges=off") needs to be highlighted with extra clarity, > simply because "--first-parent" is different from a simple > "combination" of "follow the first parent commit chain while > traversing" and "--diff-merges" [*1*] > > [ *1* It makes it worse that the "-m" option itself changes behavior > (whether it is given explicitly or implied) when used together with > "--first-parent". Unlike "git log -m -p" without "--first-parent", > you cannot see the difference the mainline brought in to the side > branch when the "--first-parent" option is in use. ] The more you explain it, the more I'm reinforced in my view that the whole thing is unnecessary complicated, and that we'd better fix it than try to cast it in stone. > It merely "implies" (or "defaults", which is the usual word we use) > "--diff-merges", so anything the user says explicitly on the command > line countermands it, i.e. This sounds exactly as if "implied" option were put at the beginning of the options list, no? By the way, could you please give an example in documentation where "defaults" is used to mean "implies", or is it informal and just for communication of intentions? I failed to find it myself, see below. > > git log --first-parent --no-diff-merges -p > > does not trigger "--diff-merges" because the user explicitly says > that diffs for patches are unwanted after saying "--first-parent" > (i.e. without "--no-diff-merges" later, we would have done the > "diff-merges" implied by "--first-parent"). > > And > > git log --no-diff-merges --first-parent -p > > does not trigger "--diff-merges" So, do these both effectively resolve to: git log -m --no-diff-merges --first-parent --patch where first -m is the one implied by --first-parent, and the rest are real options relieved from their side-effects, so that reals are mutually independent and thus immune to specific relative positions? > because the user explicitly said that diffs for patches are unwanted > by the time "--first-parent" is seen (i.e. without "--no-diff-merges" > upfront, "--first-parent" would have weighed in to enable > "--diff-merges" behavior, but because the user already said "no", it > wouldn't). This is too complicated for my taste. I believe useful outcome could be achieved without such inter-dependencies, only by using options that /actually/ imply other option(s) by simple substitution, instead of "defaulting", whatever it actually means, that would allow to technically describe options without resorting to guessing user intentions, and without need to explain behavior by outcome of a set of examples. Those particular options aside, there is yet another existing kind of option that we didn't take into account yet: --patch-with-raw Synonym for -p --raw. Synonym! Now, "synonym" means something very definite: that I can safely substitute one for another with no change of semantics, right? So, here it is, the "implies" that I mean -- true substitution, and also with no other side-effects! Synonym, perfect! For myself I call such option a "convenience option", an option that does nothing else but brings in (implies) specific set of other options. OK, let's remember we already have that. [The only problem with it is that even longest form "--patch --raw" is shorter than the option itself, so there doesn't seem to be a point of having this synonym. Looks like historical option converted to "convenience option" some time ago.] Digging further, in an attempt to actually find where "defaults" term is used, I stomped over acutal use of "imply", rather than "default", and then to yet another kind of option, "shorthand"! What a nice name for what I called "convenience option" above, I thought! Let's check: --oneline:: This is a shorthand for "--pretty=oneline --abbrev-commit" used together. Nice, "shorthand" sounds exactly like "shorter synonym", so it must be true substitution! But then, we have this: --no-abbrev-commit:: Show the full 40-byte hexadecimal commit object name. This negates `--abbrev-commit` and those options which imply it such as "--oneline". It also overrides the `log.abbrevCommit` variable. So, --oneline "implies", according to this piece of git documentation, --abbrev-commit, and simultaneously --oneline is "shorthand"=="synonym", that to me means it substitutes --abbrev-commit. The problem is that, as direct consequence of the above, "implies" must mean simple substitution, that contradicts the definition of "implies" being something called "defaults" that you gave at the beginning of your explanations. So, finally, I'm entirely lost in all this, again. There doesn't seem to be strict definition that could be relied upon, and that is not a good sign. [Unrelated, but mention not to forget. According to description above, when --oneline implies --abbrev-commit, the --no-abbrev-commit negates entire --oneline rather than implied --abbrev-commit! Is this a defect of the description, or an intended behavior, I wonder?] > > This is quite different from options that are orthogonal to each > other. We do not need permutations of "log --root --cc" vs "log > --cc --root" tested. We know from the code that they cannot > possibly interact with each other. We could still test the > permutations, and it may give us more "complete" coverage, but it > may be of very low value. These I like very much! Ideally, all the different options would be orthogonal to each other, making things much simpler to grasp, test, and describe, overall. I believe this should be one of the explicit rules or guidelines for design for new options, as well as for fixing old options whenever it's possible without breaking things. > As to permutations of additive options, we do want to make sure they > are truly additive (as opposed to being the "last one wins") and as > a side effect of ensuring the additive nature, we would end up > testing the effect of the order of options given, e.g. "git commit > -m paragraph1 -m paragraph2". These are to be rare exceptions, I think, with explicit tests indeed. > As to permutations of options that directly overrides or opposes > each other, we may want to ensure e.g. that "git log -p -U2 -U4" > uses four context lines and "git log -p -U4 -U2" uses two or that > "git format-patch --attach --no-attach" does not use attachment and > "git format-patch --no-attach --attach" does (i.e. "last one wins"). Yeah, the same option with different values should override itself in general, except additives that you've mentioned above. > > But again, especially for commands that use parse-options API > correctly to implement their command line options, it is hard to get > these two cases wrong (e.g. you need to deliberately write a > callback to make "log -p -U2 -U4" additive to use six context > lines), so it may be of lower value to throughly test permutations. > It would not be as low as testing permutations of unrelated options, > though. This is very good! The only problem remaining is to avoid creating new inter-dependencies and to get rid of existing wherever possible. BTW, the revision.c does not use the parse-options API, right? Is it possible to change that gradually, or should it be done all at once instead? I mean, if I implement new option for revision.c, should I somehow rather use the parse-options API instead of imitating existing code in revision.c? Thanks, -- Sergey ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] t/t4013: add test for --diff-merges=off 2020-08-07 23:11 ` Sergey Organov @ 2020-08-17 17:17 ` Junio C Hamano 2020-08-17 20:36 ` Sergey Organov 0 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2020-08-17 17:17 UTC (permalink / raw) To: Sergey Organov; +Cc: Jeff King, git Sergey Organov <sorganov@gmail.com> writes: > So, do these both effectively resolve to: > > git log -m --no-diff-merges --first-parent --patch > > where first -m is the one implied by --first-parent, ... Eh, why does --first-parent even affect the choice of -m/no-m when --no-diff-merges is explicitly given? > This is too complicated for my taste. No one is forcing you to rely on the implication by --first-parent. You can always give an explicit --[no-]diff-merges when you use the "--first-parent" option. Thanks. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] t/t4013: add test for --diff-merges=off 2020-08-17 17:17 ` Junio C Hamano @ 2020-08-17 20:36 ` Sergey Organov 0 siblings, 0 replies; 17+ messages in thread From: Sergey Organov @ 2020-08-17 20:36 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> So, do these both effectively resolve to: >> >> git log -m --no-diff-merges --first-parent --patch >> >> where first -m is the one implied by --first-parent, ... > > Eh, why does --first-parent even affect the choice of -m/no-m when > --no-diff-merges is explicitly given? Well, let's try your question on 3 abstract options: "Why does --X even affects the choice of --Y/no-Y when --Z is explicitly given?" And what is the assumed answer? "--X should only affect the choice of --Y/no-Y when no --Z is explicitly given" right? Or, in other terms: "--X implies --Y unless --Z is explicitly given (as opposed to implied?)" Do you see the problem here? I do see this as a problem. Simple "--X implies --Y" should be sufficient to achieve any of required behaviors. No need for such complications. I just try to figure if some more or less strict model is hidden behind all this, and it seems there is none. Thanks, -- Sergey ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-08-17 20:36 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-05 22:08 [PATCH 0/3] Change "--diff-merges" to require parameter Sergey Organov 2020-08-05 22:08 ` [PATCH 1/3] revision: change "--diff-merges" option " Sergey Organov 2020-08-05 22:08 ` [PATCH 2/3] doc/git-log: describe --diff-merges=off Sergey Organov 2020-08-12 0:06 ` Junio C Hamano 2020-08-12 0:48 ` Junio C Hamano 2020-08-12 8:08 ` Sergey Organov 2020-08-05 22:08 ` [PATCH 3/3] t/t4013: add test for --diff-merges=off Sergey Organov 2020-08-05 22:31 ` Junio C Hamano 2020-08-05 22:44 ` Sergey Organov 2020-08-05 23:19 ` Junio C Hamano 2020-08-05 23:44 ` Junio C Hamano 2020-08-06 8:34 ` Sergey Organov 2020-08-06 17:10 ` Junio C Hamano 2020-08-06 20:52 ` Sergey Organov 2020-08-07 23:11 ` Sergey Organov 2020-08-17 17:17 ` Junio C Hamano 2020-08-17 20:36 ` Sergey Organov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).