* diff-index --cc no longer permitted, gitk is now broken (slightly) @ 2021-08-30 8:03 Johannes Sixt 2021-08-30 13:05 ` Sergey Organov ` (2 more replies) 0 siblings, 3 replies; 29+ messages in thread From: Johannes Sixt @ 2021-08-30 8:03 UTC (permalink / raw) To: Sergey Organov; +Cc: Git Mailing List, Paul Mackerras Since 19b2517f95a0 (diff-merges: move specific diff-index "-m" handling to diff-index, 2021-05-21) git diff-index no longer accepts --cc. This breaks gitk: it invokes git diff-index --cached -p -C --cc --no-commit-id -U3 HEAD to show the staged changes (when the line "Local changes checked in to index but not committed" is selected). The man page of git diff-index does not mention --cc as an option. I haven't fully grokked the meaning of --cc, so I cannot tell whether this absence has any significance (is deliberate or an omission). Is gitk wrong to add --cc unconditionally? Should it do so only when there are conflicts? Or not at all? -- Hannes ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-08-30 8:03 diff-index --cc no longer permitted, gitk is now broken (slightly) Johannes Sixt @ 2021-08-30 13:05 ` Sergey Organov 2021-08-30 18:13 ` Jeff King 2021-08-30 20:26 ` Johannes Sixt 2021-08-30 17:12 ` Junio C Hamano 2021-09-01 16:52 ` Sergey Organov 2 siblings, 2 replies; 29+ messages in thread From: Sergey Organov @ 2021-08-30 13:05 UTC (permalink / raw) To: Johannes Sixt; +Cc: Git Mailing List, Paul Mackerras Johannes Sixt <j6t@kdbg.org> writes: > Since 19b2517f95a0 (diff-merges: move specific diff-index "-m" handling > to diff-index, 2021-05-21) git diff-index no longer accepts --cc. Yep, this is expected, and I even put corresponding comment in the source: /* * We need no diff for merges options, and we need to avoid conflict * with our own meaning of "-m". */ > This breaks gitk: it invokes > > git diff-index --cached -p -C --cc --no-commit-id -U3 HEAD > > to show the staged changes (when the line "Local changes checked in to > index but not committed" is selected). > > The man page of git diff-index does not mention --cc as an option. I > haven't fully grokked the meaning of --cc, so I cannot tell whether this > absence has any significance (is deliberate or an omission). > > Is gitk wrong to add --cc unconditionally? Should it do so only when > there are conflicts? Or not at all? As far as I can tell, --cc had no effect on diff-index, it was just silently consumed. If I'm right, this line in gitk never needed --cc. Then either gitk is to be fixed, or we can "fix" diff-index to silently consume --cc/-c again, for backward compatibility. If --cc did affect diff-index, then my commit in question is wrong and should be fixed. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-08-30 13:05 ` Sergey Organov @ 2021-08-30 18:13 ` Jeff King 2021-08-30 20:01 ` Sergey Organov 2021-08-30 20:26 ` Johannes Sixt 1 sibling, 1 reply; 29+ messages in thread From: Jeff King @ 2021-08-30 18:13 UTC (permalink / raw) To: Sergey Organov; +Cc: Johannes Sixt, Git Mailing List, Paul Mackerras On Mon, Aug 30, 2021 at 04:05:50PM +0300, Sergey Organov wrote: > > Is gitk wrong to add --cc unconditionally? Should it do so only when > > there are conflicts? Or not at all? > > As far as I can tell, --cc had no effect on diff-index, it was just > silently consumed. If I'm right, this line in gitk never needed --cc. > Then either gitk is to be fixed, or we can "fix" diff-index to silently > consume --cc/-c again, for backward compatibility. I think it does have an effect. Try this to generate a simple merge conflict: git init repo cd repo echo base >file git add file git commit -m base echo main >file git commit -am main git checkout -b side HEAD^ echo side >file git commit -am side git merge main ;# maybe master here depending on your config And then with pre-v2.33 Git, --cc does a combined diff: $ git.v2.32.0 diff-index -p HEAD diff --git a/file b/file index 2299c37..81768df 100644 --- a/file +++ b/file @@ -1 +1,5 @@ +<<<<<<< HEAD side +======= +main +>>>>>>> main $ git.v2.32.0 diff-index --cc HEAD diff --cc file index df967b9,2299c37..0000000 --- a/file +++ b/file @@@ -1,1 -1,1 +1,5 @@@ - base ++<<<<<<< HEAD + side ++======= ++main ++>>>>>>> main Likewise, --raw will show a combined diff, though it's a bit less useful because the unmerged entry has a null sha1: $ git.v2.32.0 diff-index HEAD :100644 100644 2299c37978265a95cbe835a4b0f0bbf15aad5549 0000000000000000000000000000000000000000 M file $ git.v2.32.0 diff-index --cc --raw HEAD ::100644 100644 100644 df967b96a579e45a18b8251732d16804b2e56a55 2299c37978265a95cbe835a4b0f0bbf15aad5549 0000000000000000000000000000000000000000 MM file -Peff ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-08-30 18:13 ` Jeff King @ 2021-08-30 20:01 ` Sergey Organov 0 siblings, 0 replies; 29+ messages in thread From: Sergey Organov @ 2021-08-30 20:01 UTC (permalink / raw) To: Jeff King; +Cc: Johannes Sixt, Git Mailing List, Paul Mackerras Jeff King <peff@peff.net> writes: > On Mon, Aug 30, 2021 at 04:05:50PM +0300, Sergey Organov wrote: > >> > Is gitk wrong to add --cc unconditionally? Should it do so only when >> > there are conflicts? Or not at all? >> >> As far as I can tell, --cc had no effect on diff-index, it was just >> silently consumed. If I'm right, this line in gitk never needed --cc. >> Then either gitk is to be fixed, or we can "fix" diff-index to silently >> consume --cc/-c again, for backward compatibility. > > I think it does have an effect. Try this to generate a simple merge > conflict: > > git init repo > cd repo > > echo base >file > git add file > git commit -m base > > echo main >file > git commit -am main > > git checkout -b side HEAD^ > echo side >file > git commit -am side > > git merge main ;# maybe master here depending on your config > > And then with pre-v2.33 Git, --cc does a combined diff: > > $ git.v2.32.0 diff-index -p HEAD > diff --git a/file b/file > index 2299c37..81768df 100644 > --- a/file > +++ b/file > @@ -1 +1,5 @@ > +<<<<<<< HEAD > side > +======= > +main > +>>>>>>> main > > $ git.v2.32.0 diff-index --cc HEAD > diff --cc file > index df967b9,2299c37..0000000 > --- a/file > +++ b/file > @@@ -1,1 -1,1 +1,5 @@@ > - base > ++<<<<<<< HEAD > + side > ++======= > ++main > ++>>>>>>> main > > Likewise, --raw will show a combined diff, though it's a bit less useful > because the unmerged entry has a null sha1: > > $ git.v2.32.0 diff-index HEAD > :100644 100644 2299c37978265a95cbe835a4b0f0bbf15aad5549 0000000000000000000000000000000000000000 M file > > $ git.v2.32.0 diff-index --cc --raw HEAD > ::100644 100644 100644 df967b96a579e45a18b8251732d16804b2e56a55 2299c37978265a95cbe835a4b0f0bbf15aad5549 0000000000000000000000000000000000000000 MM file > > -Peff Well, thanks! As it does make a difference, it's a pity it's neither documented nor being tested. I need to review the issue more closely to give a fix then. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-08-30 13:05 ` Sergey Organov 2021-08-30 18:13 ` Jeff King @ 2021-08-30 20:26 ` Johannes Sixt 2021-08-30 20:45 ` Sergey Organov 1 sibling, 1 reply; 29+ messages in thread From: Johannes Sixt @ 2021-08-30 20:26 UTC (permalink / raw) To: Sergey Organov; +Cc: Git Mailing List, Paul Mackerras Am 30.08.21 um 15:05 schrieb Sergey Organov: > As far as I can tell, --cc had no effect on diff-index, it was just > silently consumed. If I'm right, this line in gitk never needed --cc. > Then either gitk is to be fixed, or we can "fix" diff-index to silently > consume --cc/-c again, for backward compatibility. That latter would be much preferable. Gitk uses the combination -p --cc for *all* diff commands when it requests patch output regardless of the number of parents. It would be tedious to special-case diff-index to not pass --cc. -- Hannes ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-08-30 20:26 ` Johannes Sixt @ 2021-08-30 20:45 ` Sergey Organov 0 siblings, 0 replies; 29+ messages in thread From: Sergey Organov @ 2021-08-30 20:45 UTC (permalink / raw) To: Johannes Sixt; +Cc: Git Mailing List, Paul Mackerras Johannes Sixt <j6t@kdbg.org> writes: > Am 30.08.21 um 15:05 schrieb Sergey Organov: >> As far as I can tell, --cc had no effect on diff-index, it was just >> silently consumed. If I'm right, this line in gitk never needed --cc. >> Then either gitk is to be fixed, or we can "fix" diff-index to silently >> consume --cc/-c again, for backward compatibility. > > That latter would be much preferable. Gitk uses the combination -p --cc > for *all* diff commands when it requests patch output regardless of the > number of parents. It would be tedious to special-case diff-index to not > pass --cc. It's already noticed by Jeff that --cc was not in fact a no-op, so diff-index is to be fixed. Sorry for the breakage. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-08-30 8:03 diff-index --cc no longer permitted, gitk is now broken (slightly) Johannes Sixt 2021-08-30 13:05 ` Sergey Organov @ 2021-08-30 17:12 ` Junio C Hamano 2021-08-30 17:40 ` Sergey Organov 2021-08-30 18:09 ` Junio C Hamano 2021-09-01 16:52 ` Sergey Organov 2 siblings, 2 replies; 29+ messages in thread From: Junio C Hamano @ 2021-08-30 17:12 UTC (permalink / raw) To: Johannes Sixt; +Cc: Sergey Organov, Git Mailing List, Paul Mackerras Johannes Sixt <j6t@kdbg.org> writes: > Since 19b2517f95a0 (diff-merges: move specific diff-index "-m" handling > to diff-index, 2021-05-21) git diff-index no longer accepts --cc. This > breaks gitk: it invokes > > git diff-index --cached -p -C --cc --no-commit-id -U3 HEAD > > to show the staged changes (when the line "Local changes checked in to > index but not committed" is selected). > > The man page of git diff-index does not mention --cc as an option. I > haven't fully grokked the meaning of --cc, so I cannot tell whether this > absence has any significance (is deliberate or an omission). > > Is gitk wrong to add --cc unconditionally? Should it do so only when > there are conflicts? Or not at all? I think --cc is designed to naturally fall back to -p when there is only one parent. Use of both -p and --cc has also long been an acceptable combination, and even if we say the later --cc overrides -p, there is no reason not to show single parent patch here with --cc. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-08-30 17:12 ` Junio C Hamano @ 2021-08-30 17:40 ` Sergey Organov 2021-08-30 18:09 ` Junio C Hamano 1 sibling, 0 replies; 29+ messages in thread From: Sergey Organov @ 2021-08-30 17:40 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Sixt, Git Mailing List, Paul Mackerras Junio C Hamano <gitster@pobox.com> writes: > Johannes Sixt <j6t@kdbg.org> writes: > >> Since 19b2517f95a0 (diff-merges: move specific diff-index "-m" handling >> to diff-index, 2021-05-21) git diff-index no longer accepts --cc. This >> breaks gitk: it invokes >> >> git diff-index --cached -p -C --cc --no-commit-id -U3 HEAD >> >> to show the staged changes (when the line "Local changes checked in to >> index but not committed" is selected). >> >> The man page of git diff-index does not mention --cc as an option. I >> haven't fully grokked the meaning of --cc, so I cannot tell whether this >> absence has any significance (is deliberate or an omission). >> >> Is gitk wrong to add --cc unconditionally? Should it do so only when >> there are conflicts? Or not at all? > > I think --cc is designed to naturally fall back to -p when there is > only one parent. Use of both -p and --cc has also long been an > acceptable combination, and even if we say the later --cc overrides > -p, there is no reason not to show single parent patch here with > --cc. I'm pretty sure I've checked diff-index doesn't use the flag that --cc sets when I wrote the patch, so the only incompatibility this patch introduced is denying the command when --cc is given, i.e., it now behaves as if diff-index doesn't support --cc *option*, that makes sense to me and matches diff-index documentation. Irrespective to chosen solution, it still looks to me like gitk shouldn't had --cc in that command in the first place. I that correct, or do I miss something essential? That said, if you think that diff-index should silently accept --cc (and -c ?), for whatever reason, it's fine with me, provided it's properly documented and there are proper test-cases in place. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-08-30 17:12 ` Junio C Hamano 2021-08-30 17:40 ` Sergey Organov @ 2021-08-30 18:09 ` Junio C Hamano 2021-08-30 20:03 ` Sergey Organov 1 sibling, 1 reply; 29+ messages in thread From: Junio C Hamano @ 2021-08-30 18:09 UTC (permalink / raw) To: Johannes Sixt; +Cc: Sergey Organov, Git Mailing List, Paul Mackerras Junio C Hamano <gitster@pobox.com> writes: > Johannes Sixt <j6t@kdbg.org> writes: > >> Since 19b2517f95a0 (diff-merges: move specific diff-index "-m" handling >> to diff-index, 2021-05-21) git diff-index no longer accepts --cc. This >> breaks gitk: it invokes >> >> git diff-index --cached -p -C --cc --no-commit-id -U3 HEAD >> >> to show the staged changes (when the line "Local changes checked in to >> index but not committed" is selected). >> >> The man page of git diff-index does not mention --cc as an option. I >> haven't fully grokked the meaning of --cc, so I cannot tell whether this >> absence has any significance (is deliberate or an omission). >> >> Is gitk wrong to add --cc unconditionally? Should it do so only when >> there are conflicts? Or not at all? > > I think --cc is designed to naturally fall back to -p when there is > only one parent. Use of both -p and --cc has also long been an > acceptable combination, and even if we say the later --cc overrides > -p, there is no reason not to show single parent patch here with > --cc. Another tangent. I think the use of --cc with diff-index can make sense in another way. $ echo "# both" >>COPYING $ git add COPYING $ echo "# work" >>COPYING Now we have one extra line at the end in both the index and the working tree file, with yet another at the end of the latter. $ git diff-index --cc HEAD is a way to show combined diff to go to the working tree version starting from HEAD and starting from the index (I needed to use an old version because the 'maint' and upwards are broken as reported). $ rungit v1.5.3 diff-index --cc HEAD diff --cc COPYING index 8b9c100,536e555..0000000 --- a/COPYING +++ b/COPYING @@@ -358,4 -358,3 +358,5 @@@ proprietary programs. If your program consider it more useful to permit linking proprietary applications with the library. If this is what you want to do, use the GNU Lesser General Public License instead of this License. +# both ++# work Now the way "gitk" used is with "--cached", so there is no multi-way comparisons to be combined, and it is natural to fall back to "-p", so it is a different issue, but since we invented "--cc" to originally emulate, and to later improve, the output from gitk, I am reasonably sure that its use of "--cc" should be supported. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-08-30 18:09 ` Junio C Hamano @ 2021-08-30 20:03 ` Sergey Organov 0 siblings, 0 replies; 29+ messages in thread From: Sergey Organov @ 2021-08-30 20:03 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Sixt, Git Mailing List, Paul Mackerras Junio C Hamano <gitster@pobox.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Johannes Sixt <j6t@kdbg.org> writes: >> >>> Since 19b2517f95a0 (diff-merges: move specific diff-index "-m" handling >>> to diff-index, 2021-05-21) git diff-index no longer accepts --cc. This >>> breaks gitk: it invokes >>> >>> git diff-index --cached -p -C --cc --no-commit-id -U3 HEAD >>> >>> to show the staged changes (when the line "Local changes checked in to >>> index but not committed" is selected). >>> >>> The man page of git diff-index does not mention --cc as an option. I >>> haven't fully grokked the meaning of --cc, so I cannot tell whether this >>> absence has any significance (is deliberate or an omission). >>> >>> Is gitk wrong to add --cc unconditionally? Should it do so only when >>> there are conflicts? Or not at all? >> >> I think --cc is designed to naturally fall back to -p when there is >> only one parent. Use of both -p and --cc has also long been an >> acceptable combination, and even if we say the later --cc overrides >> -p, there is no reason not to show single parent patch here with >> --cc. > > Another tangent. > > I think the use of --cc with diff-index can make sense in another > way. > > $ echo "# both" >>COPYING > $ git add COPYING > $ echo "# work" >>COPYING > > Now we have one extra line at the end in both the index and the > working tree file, with yet another at the end of the latter. > > $ git diff-index --cc HEAD > > is a way to show combined diff to go to the working tree version > starting from HEAD and starting from the index (I needed to use an > old version because the 'maint' and upwards are broken as reported). > > $ rungit v1.5.3 diff-index --cc HEAD > diff --cc COPYING > index 8b9c100,536e555..0000000 > --- a/COPYING > +++ b/COPYING > @@@ -358,4 -358,3 +358,5 @@@ proprietary programs. If your program > consider it more useful to permit linking proprietary applications with the > library. If this is what you want to do, use the GNU Lesser General > Public License instead of this License. > +# both > ++# work > > Now the way "gitk" used is with "--cached", so there is no multi-way > comparisons to be combined, and it is natural to fall back to "-p", > so it is a different issue, but since we invented "--cc" to > originally emulate, and to later improve, the output from gitk, > I am reasonably sure that its use of "--cc" should be supported. If the patch breaks essential (even if undocumented and untested) behavior, as Jeff pointed, it should obviously be fixed. I'll look at it more closely to suggest a fix. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-08-30 8:03 diff-index --cc no longer permitted, gitk is now broken (slightly) Johannes Sixt 2021-08-30 13:05 ` Sergey Organov 2021-08-30 17:12 ` Junio C Hamano @ 2021-09-01 16:52 ` Sergey Organov 2021-09-07 18:19 ` Junio C Hamano 2021-09-07 20:32 ` Johannes Sixt 2 siblings, 2 replies; 29+ messages in thread From: Sergey Organov @ 2021-09-01 16:52 UTC (permalink / raw) To: Johannes Sixt; +Cc: Junio C Hamano, Jeff King, Paul Mackerras, Git Mailing List Johannes Sixt <j6t@kdbg.org> writes: > Since 19b2517f95a0 (diff-merges: move specific diff-index "-m" handling > to diff-index, 2021-05-21) git diff-index no longer accepts --cc. This > breaks gitk: it invokes > > git diff-index --cached -p -C --cc --no-commit-id -U3 HEAD > > to show the staged changes (when the line "Local changes checked in to > index but not committed" is selected). > > The man page of git diff-index does not mention --cc as an option. I > haven't fully grokked the meaning of --cc, so I cannot tell whether this > absence has any significance (is deliberate or an omission). > > Is gitk wrong to add --cc unconditionally? Should it do so only when > there are conflicts? Or not at all? > Here is a patch that fixes diff-index to accept --cc again: Subject: [PATCH] diff-index: restore -c/--cc options handling This fixes git:19b2517f95a0a908a8ada7417cf0717299e7e1aa "diff-merges: move specific diff-index "-m" handling to diff-index" That commit disabled handling of all diff for merges options in diff-index on an assumption that they are unused. However, it later appeared that -c and --cc, even though undocumented and not being covered by tests, happen to have had particular effect on diff-index output. Restore original -c/--cc options handling by diff-index. Signed-off-by: Sergey Organov <sorganov@gmail.com> --- builtin/diff-index.c | 6 +++--- diff-merges.c | 14 ++++---------- diff-merges.h | 2 +- 3 files changed, 8 insertions(+), 14 deletions(-) diff --git a/builtin/diff-index.c b/builtin/diff-index.c index cf09559e422d..5fd23ab5b6c5 100644 --- a/builtin/diff-index.c +++ b/builtin/diff-index.c @@ -29,10 +29,10 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix) prefix = precompose_argv_prefix(argc, argv, prefix); /* - * We need no diff for merges options, and we need to avoid conflict - * with our own meaning of "-m". + * We need (some of) diff for merges options (e.g., --cc), and we need + * to avoid conflict with our own meaning of "-m". */ - diff_merges_suppress_options_parsing(); + diff_merges_suppress_m_parsing(); argc = setup_revisions(argc, argv, &rev, NULL); for (i = 1; i < argc; i++) { diff --git a/diff-merges.c b/diff-merges.c index d897fd8a2933..5060ccd890bd 100644 --- a/diff-merges.c +++ b/diff-merges.c @@ -6,7 +6,7 @@ typedef void (*diff_merges_setup_func_t)(struct rev_info *); static void set_separate(struct rev_info *revs); static diff_merges_setup_func_t set_to_default = set_separate; -static int suppress_parsing; +static int suppress_m_parsing; static void suppress(struct rev_info *revs) { @@ -91,9 +91,9 @@ int diff_merges_config(const char *value) return 0; } -void diff_merges_suppress_options_parsing(void) +void diff_merges_suppress_m_parsing(void) { - suppress_parsing = 1; + suppress_m_parsing = 1; } int diff_merges_parse_opts(struct rev_info *revs, const char **argv) @@ -102,10 +102,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv) const char *optarg; const char *arg = argv[0]; - if (suppress_parsing) - return 0; - - if (!strcmp(arg, "-m")) { + if (!suppress_m_parsing && !strcmp(arg, "-m")) { set_to_default(revs); } else if (!strcmp(arg, "-c")) { set_combined(revs); @@ -153,9 +150,6 @@ void diff_merges_set_dense_combined_if_unset(struct rev_info *revs) void diff_merges_setup_revs(struct rev_info *revs) { - if (suppress_parsing) - return; - if (revs->combine_merges == 0) revs->dense_combined_merges = 0; if (revs->separate_merges == 0) diff --git a/diff-merges.h b/diff-merges.h index b5d57f6563e3..19639689bb05 100644 --- a/diff-merges.h +++ b/diff-merges.h @@ -11,7 +11,7 @@ struct rev_info; int diff_merges_config(const char *value); -void diff_merges_suppress_options_parsing(void); +void diff_merges_suppress_m_parsing(void); int diff_merges_parse_opts(struct rev_info *revs, const char **argv); -- 2.33.0.114.g9123bcff51bf ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-09-01 16:52 ` Sergey Organov @ 2021-09-07 18:19 ` Junio C Hamano 2021-09-07 19:53 ` Sergey Organov 2021-09-08 13:43 ` Sergey Organov 2021-09-07 20:32 ` Johannes Sixt 1 sibling, 2 replies; 29+ messages in thread From: Junio C Hamano @ 2021-09-07 18:19 UTC (permalink / raw) To: Sergey Organov; +Cc: Johannes Sixt, Jeff King, Paul Mackerras, Git Mailing List Sergey Organov <sorganov@gmail.com> writes: > Here is a patch that fixes diff-index to accept --cc again: Sorry for the delay; I did not notice there was a patch buried in a discussion thread. We might later need to do this suppression in more codepaths if we find more regressions, but let's have one fix at a time. Will queue. > builtin/diff-index.c | 6 +++--- > diff-merges.c | 14 ++++---------- > diff-merges.h | 2 +- This would deserve new tests that cover the existing use cases, given that both of us (and other reviewers in the original thread) did not notice how big a regression we are causing. We care about --cc naturally falling back to -p when there is only one other thing to compare with, and also we care about --cc that allows us to compare during conflict resolution, at least, I think. It can and should come as a separate step, of course. Unbreaking gitk for an already known breakage would be more urgent than hunting for other breakages, even though the latter might result in a more thorough fix in the end. Thanks. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-09-07 18:19 ` Junio C Hamano @ 2021-09-07 19:53 ` Sergey Organov 2021-09-08 13:43 ` Sergey Organov 1 sibling, 0 replies; 29+ messages in thread From: Sergey Organov @ 2021-09-07 19:53 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Sixt, Jeff King, Paul Mackerras, Git Mailing List Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> Here is a patch that fixes diff-index to accept --cc again: > > Sorry for the delay; I did not notice there was a patch buried in a > discussion thread. Sorry from my side as well. I was already about to re-submit the patch properly, but you happened to be faster with this ) > > We might later need to do this suppression in more codepaths if we > find more regressions, but let's have one fix at a time. > > Will queue. > >> builtin/diff-index.c | 6 +++--- >> diff-merges.c | 14 ++++---------- >> diff-merges.h | 2 +- > > This would deserve new tests that cover the existing use cases, > given that both of us (and other reviewers in the original thread) > did not notice how big a regression we are causing. Yep, it's too easy to break undocumented and untested behavior. > We care about --cc naturally falling back to -p when there is only > one other thing to compare with, and also we care about --cc that > allows us to compare during conflict resolution, at least, I think. I'm all for more tests, but I'm afraid I'm not in a good position to write them, especially for an undocumented behavior. I think somebody should first document what --cc/-c does in diff-index, and only then it makes sense to write some tests. > It can and should come as a separate step, of course. Unbreaking > gitk for an already known breakage would be more urgent than hunting > for other breakages, even though the latter might result in a more > thorough fix in the end. Makes sense. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-09-07 18:19 ` Junio C Hamano 2021-09-07 19:53 ` Sergey Organov @ 2021-09-08 13:43 ` Sergey Organov 2021-09-08 17:23 ` Johannes Sixt 2021-09-16 9:50 ` Sergey Organov 1 sibling, 2 replies; 29+ messages in thread From: Sergey Organov @ 2021-09-08 13:43 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Sixt, Jeff King, Paul Mackerras, Git Mailing List Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> Here is a patch that fixes diff-index to accept --cc again: > > Sorry for the delay; I did not notice there was a patch buried in a > discussion thread. > > We might later need to do this suppression in more codepaths if we > find more regressions, but let's have one fix at a time. I'm pretty positive there should be nothing left. This commit was diff-index specific, and doesn't affect anything else. Nowhere in entire series the semantics of --cc itself has been changed, it has been only disabled as particular option in diff-index command-line parsing. Overall, this is pretty local change. > > Will queue. > >> builtin/diff-index.c | 6 +++--- >> diff-merges.c | 14 ++++---------- >> diff-merges.h | 2 +- > > This would deserve new tests that cover the existing use cases, > given that both of us (and other reviewers in the original thread) > did not notice how big a regression we are causing. I don't see it as a "big regression", but no wonder the breakage was entirely unexpected, see below. > > We care about --cc naturally falling back to -p when there is only > one other thing to compare with, and also we care about --cc that > allows us to compare during conflict resolution, at least, I think. The problem here is not with -c/--cc itself, it is rather with diff-index. It's neither documented nor tested nor obvious what -c/--cc should mean in diff-index, given -c/--cc description (e.g., in "git help log"): -c With this option, diff output for a merge commit shows the differences [...] How an option to deal with merge commits is applicable to diff-index, that: git-diff-index - Compare a tree to the working tree or index ??? Besides, nobody yet told us why gitk uses --cc option in invocation of 'diff-index' in the first place. Does it actually *rely* on particular undocumented behavior of "diff-index --cc", or is it just a copy-paste *leftover*? Overall, the original commit had a mistake, as the commit that was meant to be pure refactoring had changed observable behavior, even if undocumented. However, the essence of the commit, disabling of "diff for merge /commits/" options in diff-index that does not deal with /commits/, could still be the right thing to do long term. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-09-08 13:43 ` Sergey Organov @ 2021-09-08 17:23 ` Johannes Sixt 2021-09-08 19:04 ` Sergey Organov 2021-09-09 17:07 ` Junio C Hamano 2021-09-16 9:50 ` Sergey Organov 1 sibling, 2 replies; 29+ messages in thread From: Johannes Sixt @ 2021-09-08 17:23 UTC (permalink / raw) To: Sergey Organov Cc: Junio C Hamano, Jeff King, Paul Mackerras, Git Mailing List Am 08.09.21 um 15:43 schrieb Sergey Organov: > Besides, nobody yet told us why gitk uses --cc option in invocation of > 'diff-index' in the first place. Does it actually *rely* on particular > undocumented behavior of "diff-index --cc", or is it just a copy-paste > *leftover*? No, it is not a left-over. The thing is, - there is one point in the code where gitk adds options -p -C --cc (and more) to the command line (around line 8034), - and there is a totally different point in the code where it is decided whether diff-index, diff-tree, or diff-files is invoked (proc diffcmd around line 7871). IOW, Gitk expects that these option combinations can always be passed to all three commands. Gitk does not want to look at a commit and then decide which incarnation of the command it wants to use (--cc vs. -p) depending on whether it is a merge commit or not. This decision is delegated to command that is invoked. Therefore, silent fall-back from --cc to -p in case of non-merge commits or non-conflicted index is absolutely necessary. -- Hannes ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-09-08 17:23 ` Johannes Sixt @ 2021-09-08 19:04 ` Sergey Organov 2021-09-09 17:07 ` Junio C Hamano 1 sibling, 0 replies; 29+ messages in thread From: Sergey Organov @ 2021-09-08 19:04 UTC (permalink / raw) To: Johannes Sixt; +Cc: Junio C Hamano, Jeff King, Paul Mackerras, Git Mailing List Johannes Sixt <j6t@kdbg.org> writes: > Am 08.09.21 um 15:43 schrieb Sergey Organov: >> Besides, nobody yet told us why gitk uses --cc option in invocation of >> 'diff-index' in the first place. Does it actually *rely* on particular >> undocumented behavior of "diff-index --cc", or is it just a copy-paste >> *leftover*? > > No, it is not a left-over. The thing is, > > - there is one point in the code where gitk adds options -p -C --cc (and > more) to the command line (around line 8034), > > - and there is a totally different point in the code where it is decided > whether diff-index, diff-tree, or diff-files is invoked (proc diffcmd > around line 7871). > > IOW, Gitk expects that these option combinations can always be passed to > all three commands. I see, but the problem here is that while diff-files and diff-tree both accept --cc according to their documentation, diff-index does not. This means that, strictly speaking, gitk makes a mistake treating all 3 commands universally with respect to command-line arguments when it uses --cc. > > Gitk does not want to look at a commit and then decide which incarnation > of the command it wants to use (--cc vs. -p) depending on whether it is > a merge commit or not. This decision is delegated to command that is > invoked. The problem is not in the kind of commit, the problem is in the command being invoked. diff-index doesn't support --cc according to its documentation, and thus gitk relies on undocumented behavior of diff-index. It might well be the case that it just happened to be "working", thus nobody cared. > Therefore, silent fall-back from --cc to -p in case of non-merge > commits or non-conflicted index is absolutely necessary. I didn't change semantics of --cc, so this thing was not broken at all. I just disabled the --cc option in diff-index command, to match the documentation. As a side note, in fact Git does no "silent fall-back from --cc to -p in case of non-merge commits", even though the behavior could be indeed seen like this. Instead, --cc implies -p, and, as --cc does not otherwise affect treating of non-merge commits, only -p is left active for them. Once again, this has not been recently changed, so does not need to be fixed. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-09-08 17:23 ` Johannes Sixt 2021-09-08 19:04 ` Sergey Organov @ 2021-09-09 17:07 ` Junio C Hamano 2021-09-09 20:07 ` Sergey Organov 1 sibling, 1 reply; 29+ messages in thread From: Junio C Hamano @ 2021-09-09 17:07 UTC (permalink / raw) To: Johannes Sixt; +Cc: Sergey Organov, Jeff King, Paul Mackerras, Git Mailing List Johannes Sixt <j6t@kdbg.org> writes: > Gitk does not want to look at a commit and then decide which incarnation > of the command it wants to use (--cc vs. -p) depending on whether it is > a merge commit or not. This decision is delegated to command that is > invoked. Therefore, silent fall-back from --cc to -p in case of > non-merge commits or non-conflicted index is absolutely necessary. Well explained. "-p" in general is an instruction to show some form of textual patch, and "--cc" and "-c" are the variants (i.e. compare with each parent and combine the comparison results) of it that naturally degenerates to the normal patch output when there is only one parent. "--cc" also flips the "m" bit, which controls if there is any tree comparison should be made for merge commits, which matters for "log" family of commands, so in that sense "--cc" was made to imply "-m", but "--cc" inherently means "-p" for non-merge commits without any need to say X implies Y. Thanks. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-09-09 17:07 ` Junio C Hamano @ 2021-09-09 20:07 ` Sergey Organov 0 siblings, 0 replies; 29+ messages in thread From: Sergey Organov @ 2021-09-09 20:07 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Sixt, Jeff King, Paul Mackerras, Git Mailing List Junio C Hamano <gitster@pobox.com> writes: > Johannes Sixt <j6t@kdbg.org> writes: > >> Gitk does not want to look at a commit and then decide which incarnation >> of the command it wants to use (--cc vs. -p) depending on whether it is >> a merge commit or not. This decision is delegated to command that is >> invoked. Therefore, silent fall-back from --cc to -p in case of >> non-merge commits or non-conflicted index is absolutely necessary. > > Well explained. > > "-p" in general is an instruction to show some form of textual > patch, and "--cc" and "-c" are the variants (i.e. compare with each > parent and combine the comparison results) of it that naturally > degenerates to the normal patch output when there is only one > parent. > > "--cc" also flips the "m" bit, which controls if there is any tree > comparison should be made for merge commits, which matters for "log" > family of commands, so in that sense "--cc" was made to imply "-m", > but "--cc" inherently means "-p" for non-merge commits without any > need to say X implies Y. Except both Git documentation and implementation are in some contradiction with the explanations above, as far as I can see, so this looks to me like a kind of wishful thinking, sorry. [One can read, say, year old Git documentation, prior to recent patches (to get rid of possible bias), to see that it describes different picture and explicitly contradicts some of the statements above, and then I definitely did no steps to move in the direction described above either, at least intentionally.] Anyway, here is the current reality the way I see it (this description assumes "-m imply -p" patch by me has been reverted due to introduction of slight backward incompatibility): -p enables diff for non-merge commits only --diff-merges=XXXX enables diff for merge commits only --cc => -p --diff-merges=cc -c => -p --diff-merges=c -m => --diff-merges=m where "=>" means "is shortcut for". Simple and clear, except -m is an outlier, as it does not imply -p. Further, -m produces inconvenient output unless --first-parent is also in use, and then --first-parent already implies -m, rendering explicit -m virtually useless. Due to this outlier, all the recent changes in this area were targeted to make -m useful, similar to --cc/-c, not to change --cc/-c, or -p semantics at all, so that finally we get perfect: --cc => -p --diff-merges=cc -c => -p --diff-merges=c -m => -p --diff-merges=m Please also notice that with this system there is no need for every (new) way of representing of merge commits to support "fall back to -p in case of non-merge commits" at options level, as it does not have to deal with non-merge commits at all. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-09-08 13:43 ` Sergey Organov 2021-09-08 17:23 ` Johannes Sixt @ 2021-09-16 9:50 ` Sergey Organov 2021-09-16 21:15 ` Junio C Hamano 1 sibling, 1 reply; 29+ messages in thread From: Sergey Organov @ 2021-09-16 9:50 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Sixt, Jeff King, Paul Mackerras, Git Mailing List Sergey Organov <sorganov@gmail.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Sergey Organov <sorganov@gmail.com> writes: >> >>> Here is a patch that fixes diff-index to accept --cc again: >> >> Sorry for the delay; I did not notice there was a patch buried in a >> discussion thread. >> >> We might later need to do this suppression in more codepaths if we >> find more regressions, but let's have one fix at a time. > > I'm pretty positive there should be nothing left. This commit was > diff-index specific, and doesn't affect anything else. Nowhere in entire > series the semantics of --cc itself has been changed, it has been only > disabled as particular option in diff-index command-line parsing. > Overall, this is pretty local change. I'm afraid this issue is left up in the air after application of the fix-up patch, as usage of --cc in the diff-index is still undocumented. I.e., the fix-up just restores the historical status quo that has a problem by itself. As current documentation of --cc elsewhere does not seem to be even suitable for diff-index, I don't feel like providing corresponding patch for the documentation, even less so as I still have no idea if current behavior is intended or accidental, and if it's the latter, do we need to keep or somehow fix it? Anybody? Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-09-16 9:50 ` Sergey Organov @ 2021-09-16 21:15 ` Junio C Hamano 2021-09-16 22:41 ` Sergey Organov 0 siblings, 1 reply; 29+ messages in thread From: Junio C Hamano @ 2021-09-16 21:15 UTC (permalink / raw) To: Sergey Organov; +Cc: Johannes Sixt, Jeff King, Paul Mackerras, Git Mailing List Sergey Organov <sorganov@gmail.com> writes: > I'm afraid this issue is left up in the air after application of the > fix-up patch, as usage of --cc in the diff-index is still undocumented. Yeah, I do think documentation update is needed, but being buried by other topics I haven't had a chance to revisit the way how --cc is described in the wider context in order to make an intellgent suggestion on how to present it in the context of "diff-index". > I.e., the fix-up just restores the historical status quo that has a > problem by itself. I do agree "show -p" on merge is an oddball that trips new people, because it does not imply the "do present the changes for merges" bit unlike "show -c/--cc" do, and from that point of view, the generalization --diff-merges tried to bring us was a good thing. But "-c/--cc" are explicit enough in what they want to do. It does want to present the changes to compare a single end state with possibly more than one starting state (e.g. a merge) and not requiring an explicit "-m" is quite natural. Even more, when it compares the end state with only one starting state (e.g. showing a single parent commit), there is only one pairwise result to "combine", so it is also natural that it ends up showing the same output as "-p". So I do not quite see the behaviour of "diff*/show --cc" as a problem, though. IOW, the use pattern in gitk is more than just "historical status quo", but is quite sensible, I would have to say. Thanks. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-09-16 21:15 ` Junio C Hamano @ 2021-09-16 22:41 ` Sergey Organov 2021-09-16 22:50 ` Junio C Hamano 0 siblings, 1 reply; 29+ messages in thread From: Sergey Organov @ 2021-09-16 22:41 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Sixt, Jeff King, Paul Mackerras, Git Mailing List Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> I'm afraid this issue is left up in the air after application of the >> fix-up patch, as usage of --cc in the diff-index is still undocumented. > > Yeah, I do think documentation update is needed, but being buried by > other topics I haven't had a chance to revisit the way how --cc is > described in the wider context in order to make an intellgent > suggestion on how to present it in the context of "diff-index". I tend to believe yet another description of --cc is needed for diff-index, separate from the current one. Just saying. > >> I.e., the fix-up just restores the historical status quo that has a >> problem by itself. > > I do agree "show -p" on merge is an oddball that trips new people, > because it does not imply the "do present the changes for merges" > bit unlike "show -c/--cc" do, and from that point of view, the > generalization --diff-merges tried to bring us was a good thing. I'm not sure I follow. What "show -p" has to do with "diff-index --cc"? My only point here is that usage of *--cc* in *diff-index* is entirely undocumneted, and that needs to be somehow resolved. > > But "-c/--cc" are explicit enough in what they want to do. It does > want to present the changes to compare a single end state with > possibly more than one starting state (e.g. a merge) and not > requiring an explicit "-m" is quite natural. Doesn't seem to be relevant for "diff-index --cc" lacking documentation, but -m and -c and --cc are rather *mutually exclusive*. I.e., they all set different formats for output of diffs for merges, so "--cc -m" == "-m", and "-m --cc" == "--cc", i.e., the latest overrides the format to be used. Therefore "requiring explicit -m for -cc" simply doesn't make sense. > Even more, when it compares the end state with only one starting state > (e.g. showing a single parent commit), there is only one pairwise > result to "combine", so it is also natural that it ends up showing the > same output as "-p". So I do not quite see the behaviour of > "diff*/show --cc" as a problem, though. I don't see it as a problem as well, so whom you are arguing with? The only problem in this particular case I see is that "diff-index --cc" is undocumented (and untested), and this has nothing to do with log/diff/show, unless I miss your point. > IOW, the use pattern in gitk is more than just "historical status > quo", but is quite sensible, I would have to say. "diff-index --cc" in gitk is a bug, as according to Git documentation "diff-index" does not accept "--cc", period. gitk trying to make sense of what is neither documented, nor tested, nor guaranteed is the problem, but I was talking even not about that problem, but rather about the cause of this: some undocumented processing of "git diff-index --cc". Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-09-16 22:41 ` Sergey Organov @ 2021-09-16 22:50 ` Junio C Hamano 2021-09-17 7:08 ` Sergey Organov 0 siblings, 1 reply; 29+ messages in thread From: Junio C Hamano @ 2021-09-16 22:50 UTC (permalink / raw) To: Sergey Organov; +Cc: Johannes Sixt, Jeff King, Paul Mackerras, Git Mailing List Sergey Organov <sorganov@gmail.com> writes: > I'm not sure I follow. What "show -p" has to do with "diff-index --cc"? > > My only point here is that usage of *--cc* in *diff-index* is entirely > undocumneted, and that needs to be somehow resolved. It was a response to your "historical status quo that is a problem." I do not think there is any problem with "diff-index --cc" (except for it wants a better documentation---but that we already agree) but I wanted to give you some credit for having worked on "--diff-merges", an effort to generalize things in a related area. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-09-16 22:50 ` Junio C Hamano @ 2021-09-17 7:08 ` Sergey Organov 2021-09-17 16:32 ` Junio C Hamano 2021-09-17 16:58 ` Philip Oakley 0 siblings, 2 replies; 29+ messages in thread From: Sergey Organov @ 2021-09-17 7:08 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Sixt, Jeff King, Paul Mackerras, Git Mailing List Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> I'm not sure I follow. What "show -p" has to do with "diff-index --cc"? >> >> My only point here is that usage of *--cc* in *diff-index* is entirely >> undocumneted, and that needs to be somehow resolved. > > It was a response to your "historical status quo that is a problem." > I do not think there is any problem with "diff-index --cc" (except > for it wants a better documentation---but that we already agree) but Ah, now I see, but it's exactly lack of documentation (and tests) that I was referring to as the "problem of the historical status quo" on the Git side, so I was somewhat confused by your original response. Also, it's still unclear, even if not very essential, what exactly that "status quo" is when seen from the point of view of gitk. Does gitk actually utilize *particular output* of "diff-index --cc" for better, or gitk would be just as happy if it were synonym for "diff-index -p", or even if it'd be just as happy if --cc were silently consumed by diff-index? > I wanted to give you some credit for having worked on "--diff-merges", > an effort to generalize things in a related area. Thanks for that! More to follow ) Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-09-17 7:08 ` Sergey Organov @ 2021-09-17 16:32 ` Junio C Hamano 2021-09-17 18:41 ` Sergey Organov 2021-09-17 16:58 ` Philip Oakley 1 sibling, 1 reply; 29+ messages in thread From: Junio C Hamano @ 2021-09-17 16:32 UTC (permalink / raw) To: Sergey Organov; +Cc: Johannes Sixt, Jeff King, Paul Mackerras, Git Mailing List Sergey Organov <sorganov@gmail.com> writes: > Ah, now I see, but it's exactly lack of documentation (and tests) that I > was referring to as the "problem of the historical status quo" on the > Git side, so I was somewhat confused by your original response. Well, you said the fixup "restores" the status quo, but in fact, with or without the fixup, before or after it, the lack of documentation was there. So I thought you were talking about something else. >> I wanted to give you some credit for having worked on "--diff-merges", >> an effort to generalize things in a related area. > > Thanks for that! More to follow ) I somehow expect there was need for no further work in this area, but there are also many other areas in Git where your talent is applicable and appreciated, I am sure ;-) Thanks. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-09-17 16:32 ` Junio C Hamano @ 2021-09-17 18:41 ` Sergey Organov 0 siblings, 0 replies; 29+ messages in thread From: Sergey Organov @ 2021-09-17 18:41 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Sixt, Jeff King, Paul Mackerras, Git Mailing List Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> Ah, now I see, but it's exactly lack of documentation (and tests) that I >> was referring to as the "problem of the historical status quo" on the >> Git side, so I was somewhat confused by your original response. > > Well, you said the fixup "restores" the status quo, but in fact, > with or without the fixup, before or after it, the lack of > documentation was there. No, what problematic patch did, it changed behavior of diff-index exactly in accordance with its *current* documentation that doesn't mention --cc as accepted command-line option for diff-index. So, with that patch applied, there were no this problem with documentation anymore. Implementation now actually matched the docs. Unfortunately, that brought worse problem: it unexpectedly broke gitk, that, as it appeared, depends on undocumented diff-index behavior. So, I re-enabled --cc in diff-index, lesser of two evils, that brought back the problem of lack of documentation and test cases for "diff-index --cc". This way, the status quo has been restored indeed. > So I thought you were talking about something else. > >>> I wanted to give you some credit for having worked on "--diff-merges", >>> an effort to generalize things in a related area. >> >> Thanks for that! More to follow ) > > I somehow expect there was need for no further work in this area, > but there are also many other areas in Git where your talent is > applicable and appreciated, I am sure ;-) I'm afraid we still didn't reach one of the ultimate goals of all this: letting -m be useful again, specifically, as suitable *user* option. Also, current --diff-merges options are incapable of providing current -m behavior, as has been noticed by Jonathan Nieder in another thread on reverting "-m implies -p" commit: "When I try it locally, -m shows no diff by default, whereas --diff-merges=separate shows a diff for merges." and I'm going to fix this by adding yet another feature for --diff-merges. This is to be pure addition, thus causing no backward compatibility problems. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-09-17 7:08 ` Sergey Organov 2021-09-17 16:32 ` Junio C Hamano @ 2021-09-17 16:58 ` Philip Oakley 2021-09-17 17:34 ` Sergey Organov 1 sibling, 1 reply; 29+ messages in thread From: Philip Oakley @ 2021-09-17 16:58 UTC (permalink / raw) To: Sergey Organov, Junio C Hamano Cc: Johannes Sixt, Jeff King, Paul Mackerras, Git Mailing List On 17/09/2021 08:08, Sergey Organov wrote: > Junio C Hamano <gitster@pobox.com> writes: > >> Sergey Organov <sorganov@gmail.com> writes: >> >>> I'm not sure I follow. What "show -p" has to do with "diff-index --cc"? >>> >>> My only point here is that usage of *--cc* in *diff-index* is entirely >>> undocumneted, and that needs to be somehow resolved. >> It was a response to your "historical status quo that is a problem." >> I do not think there is any problem with "diff-index --cc" (except >> for it wants a better documentation---but that we already agree) but > Ah, now I see, but it's exactly lack of documentation (and tests) that I > was referring to as the "problem of the historical status quo" on the > Git side, so I was somewhat confused by your original response. > > Also, it's still unclear, even if not very essential, what exactly that > "status quo" is when seen from the point of view of gitk. Does gitk > actually utilize *particular output* of "diff-index --cc" for better, or > gitk would be just as happy if it were synonym for "diff-index -p", or > even if it'd be just as happy if --cc were silently consumed by > diff-index? Did Johannes Sixt's earlier answer https://lore.kernel.org/git/cbd0d173-ef17-576b-ab7a-465d42c82265@kdbg.org/ help clarify the choices? >> I wanted to give you some credit for having worked on "--diff-merges", >> an effort to generalize things in a related area. > Thanks for that! More to follow ) > > Thanks, > -- Sergey Organov ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-09-17 16:58 ` Philip Oakley @ 2021-09-17 17:34 ` Sergey Organov 2021-09-18 17:56 ` Sergey Organov 0 siblings, 1 reply; 29+ messages in thread From: Sergey Organov @ 2021-09-17 17:34 UTC (permalink / raw) To: Philip Oakley Cc: Junio C Hamano, Johannes Sixt, Jeff King, Paul Mackerras, Git Mailing List Philip Oakley <philipoakley@iee.email> writes: > On 17/09/2021 08:08, Sergey Organov wrote: >> Junio C Hamano <gitster@pobox.com> writes: >> >>> Sergey Organov <sorganov@gmail.com> writes: >>> >>>> I'm not sure I follow. What "show -p" has to do with "diff-index --cc"? >>>> >>>> My only point here is that usage of *--cc* in *diff-index* is entirely >>>> undocumneted, and that needs to be somehow resolved. >>> It was a response to your "historical status quo that is a problem." >>> I do not think there is any problem with "diff-index --cc" (except >>> for it wants a better documentation---but that we already agree) but >> Ah, now I see, but it's exactly lack of documentation (and tests) that I >> was referring to as the "problem of the historical status quo" on the >> Git side, so I was somewhat confused by your original response. >> >> Also, it's still unclear, even if not very essential, what exactly that >> "status quo" is when seen from the point of view of gitk. Does gitk >> actually utilize *particular output* of "diff-index --cc" for better, or >> gitk would be just as happy if it were synonym for "diff-index -p", or >> even if it'd be just as happy if --cc were silently consumed by >> diff-index? > > Did Johannes Sixt's earlier answer > https://lore.kernel.org/git/cbd0d173-ef17-576b-ab7a-465d42c82265@kdbg.org/ > help clarify the choices? Sorry, no. I did read that carefully when it has been posted. Further explanations by Johannes also only tell that gitk expects --cc to be accepted by diff-index as it likes to treat multiple commands universally, but don't specify what output git expects from --cc when it passes it exactly to diff-index. Maybe it just shows the output and have no other expectations, dunno. "silent fall-back from --cc to -p in case of non-merge commits or non-conflicted index is absolutely necessary" that is stated there is a non-issue as it was always the case, and there are no questions about it, as --cc in fact implies -p, and the latter is applied to non-merge commits. So, how Git treats "non-conflicted index" and non-merge commits does not depend on --cc. It's how it treats "conflicting index" that is still unspecified. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-09-17 17:34 ` Sergey Organov @ 2021-09-18 17:56 ` Sergey Organov 0 siblings, 0 replies; 29+ messages in thread From: Sergey Organov @ 2021-09-18 17:56 UTC (permalink / raw) To: Philip Oakley Cc: Junio C Hamano, Johannes Sixt, Jeff King, Paul Mackerras, Git Mailing List Sergey Organov <sorganov@gmail.com> writes: > Philip Oakley <philipoakley@iee.email> writes: > >> On 17/09/2021 08:08, Sergey Organov wrote: >>> Junio C Hamano <gitster@pobox.com> writes: >>> >>>> Sergey Organov <sorganov@gmail.com> writes: >>>> >>>>> I'm not sure I follow. What "show -p" has to do with "diff-index --cc"? >>>>> >>>>> My only point here is that usage of *--cc* in *diff-index* is entirely >>>>> undocumneted, and that needs to be somehow resolved. >>>> It was a response to your "historical status quo that is a problem." >>>> I do not think there is any problem with "diff-index --cc" (except >>>> for it wants a better documentation---but that we already agree) but >>> Ah, now I see, but it's exactly lack of documentation (and tests) that I >>> was referring to as the "problem of the historical status quo" on the >>> Git side, so I was somewhat confused by your original response. >>> >>> Also, it's still unclear, even if not very essential, what exactly that >>> "status quo" is when seen from the point of view of gitk. Does gitk >>> actually utilize *particular output* of "diff-index --cc" for better, or >>> gitk would be just as happy if it were synonym for "diff-index -p", or >>> even if it'd be just as happy if --cc were silently consumed by >>> diff-index? >> >> Did Johannes Sixt's earlier answer >> https://lore.kernel.org/git/cbd0d173-ef17-576b-ab7a-465d42c82265@kdbg.org/ >> help clarify the choices? > > Sorry, no. I did read that carefully when it has been posted. Further > explanations by Johannes also only tell that gitk expects --cc to be > accepted by diff-index as it likes to treat multiple commands > universally, but don't specify what output git expects from --cc when it > passes it exactly to diff-index. Maybe it just shows the output and have > no other expectations, dunno. And, even more importantly, if gitk uses "git diff-index --cc" for its specific output, is there another, documented way to achieve the same goal? Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: diff-index --cc no longer permitted, gitk is now broken (slightly) 2021-09-01 16:52 ` Sergey Organov 2021-09-07 18:19 ` Junio C Hamano @ 2021-09-07 20:32 ` Johannes Sixt 1 sibling, 0 replies; 29+ messages in thread From: Johannes Sixt @ 2021-09-07 20:32 UTC (permalink / raw) To: Sergey Organov Cc: Junio C Hamano, Jeff King, Paul Mackerras, Git Mailing List Am 01.09.21 um 18:52 schrieb Sergey Organov: > Johannes Sixt <j6t@kdbg.org> writes: > >> Since 19b2517f95a0 (diff-merges: move specific diff-index "-m" handling >> to diff-index, 2021-05-21) git diff-index no longer accepts --cc. This >> breaks gitk: it invokes >> >> git diff-index --cached -p -C --cc --no-commit-id -U3 HEAD >> >> to show the staged changes (when the line "Local changes checked in to >> index but not committed" is selected). >> >> The man page of git diff-index does not mention --cc as an option. I >> haven't fully grokked the meaning of --cc, so I cannot tell whether this >> absence has any significance (is deliberate or an omission). >> >> Is gitk wrong to add --cc unconditionally? Should it do so only when >> there are conflicts? Or not at all? >> > > Here is a patch that fixes diff-index to accept --cc again: > > Subject: [PATCH] diff-index: restore -c/--cc options handling > > This fixes git:19b2517f95a0a908a8ada7417cf0717299e7e1aa > "diff-merges: move specific diff-index "-m" handling to diff-index" > > That commit disabled handling of all diff for merges options in > diff-index on an assumption that they are unused. However, it later > appeared that -c and --cc, even though undocumented and not being > covered by tests, happen to have had particular effect on diff-index > output. > > Restore original -c/--cc options handling by diff-index. Thank you! I can confirm that gitk works again as expected with this patch. -- Hannes > > Signed-off-by: Sergey Organov <sorganov@gmail.com> > --- > builtin/diff-index.c | 6 +++--- > diff-merges.c | 14 ++++---------- > diff-merges.h | 2 +- > 3 files changed, 8 insertions(+), 14 deletions(-) > > diff --git a/builtin/diff-index.c b/builtin/diff-index.c > index cf09559e422d..5fd23ab5b6c5 100644 > --- a/builtin/diff-index.c > +++ b/builtin/diff-index.c > @@ -29,10 +29,10 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix) > prefix = precompose_argv_prefix(argc, argv, prefix); > > /* > - * We need no diff for merges options, and we need to avoid conflict > - * with our own meaning of "-m". > + * We need (some of) diff for merges options (e.g., --cc), and we need > + * to avoid conflict with our own meaning of "-m". > */ > - diff_merges_suppress_options_parsing(); > + diff_merges_suppress_m_parsing(); > > argc = setup_revisions(argc, argv, &rev, NULL); > for (i = 1; i < argc; i++) { > diff --git a/diff-merges.c b/diff-merges.c > index d897fd8a2933..5060ccd890bd 100644 > --- a/diff-merges.c > +++ b/diff-merges.c > @@ -6,7 +6,7 @@ typedef void (*diff_merges_setup_func_t)(struct rev_info *); > static void set_separate(struct rev_info *revs); > > static diff_merges_setup_func_t set_to_default = set_separate; > -static int suppress_parsing; > +static int suppress_m_parsing; > > static void suppress(struct rev_info *revs) > { > @@ -91,9 +91,9 @@ int diff_merges_config(const char *value) > return 0; > } > > -void diff_merges_suppress_options_parsing(void) > +void diff_merges_suppress_m_parsing(void) > { > - suppress_parsing = 1; > + suppress_m_parsing = 1; > } > > int diff_merges_parse_opts(struct rev_info *revs, const char **argv) > @@ -102,10 +102,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv) > const char *optarg; > const char *arg = argv[0]; > > - if (suppress_parsing) > - return 0; > - > - if (!strcmp(arg, "-m")) { > + if (!suppress_m_parsing && !strcmp(arg, "-m")) { > set_to_default(revs); > } else if (!strcmp(arg, "-c")) { > set_combined(revs); > @@ -153,9 +150,6 @@ void diff_merges_set_dense_combined_if_unset(struct rev_info *revs) > > void diff_merges_setup_revs(struct rev_info *revs) > { > - if (suppress_parsing) > - return; > - > if (revs->combine_merges == 0) > revs->dense_combined_merges = 0; > if (revs->separate_merges == 0) > diff --git a/diff-merges.h b/diff-merges.h > index b5d57f6563e3..19639689bb05 100644 > --- a/diff-merges.h > +++ b/diff-merges.h > @@ -11,7 +11,7 @@ struct rev_info; > > int diff_merges_config(const char *value); > > -void diff_merges_suppress_options_parsing(void); > +void diff_merges_suppress_m_parsing(void); > > int diff_merges_parse_opts(struct rev_info *revs, const char **argv); > > ^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2021-09-18 17:56 UTC | newest] Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-30 8:03 diff-index --cc no longer permitted, gitk is now broken (slightly) Johannes Sixt 2021-08-30 13:05 ` Sergey Organov 2021-08-30 18:13 ` Jeff King 2021-08-30 20:01 ` Sergey Organov 2021-08-30 20:26 ` Johannes Sixt 2021-08-30 20:45 ` Sergey Organov 2021-08-30 17:12 ` Junio C Hamano 2021-08-30 17:40 ` Sergey Organov 2021-08-30 18:09 ` Junio C Hamano 2021-08-30 20:03 ` Sergey Organov 2021-09-01 16:52 ` Sergey Organov 2021-09-07 18:19 ` Junio C Hamano 2021-09-07 19:53 ` Sergey Organov 2021-09-08 13:43 ` Sergey Organov 2021-09-08 17:23 ` Johannes Sixt 2021-09-08 19:04 ` Sergey Organov 2021-09-09 17:07 ` Junio C Hamano 2021-09-09 20:07 ` Sergey Organov 2021-09-16 9:50 ` Sergey Organov 2021-09-16 21:15 ` Junio C Hamano 2021-09-16 22:41 ` Sergey Organov 2021-09-16 22:50 ` Junio C Hamano 2021-09-17 7:08 ` Sergey Organov 2021-09-17 16:32 ` Junio C Hamano 2021-09-17 18:41 ` Sergey Organov 2021-09-17 16:58 ` Philip Oakley 2021-09-17 17:34 ` Sergey Organov 2021-09-18 17:56 ` Sergey Organov 2021-09-07 20:32 ` Johannes Sixt
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.