| Message ID | 20200729201002.GA2989059@coredump.intra.peff.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | making log --first-parent imply -m | expand |
On Wed, Jul 29, 2020 at 1:10 PM Jeff King <peff@peff.net> wrote: > Here's a re-roll taking into account the discussion so far: > > - the escape hatch option name is flipped to "--no-diff-merges" (with > "--diff-merges" matching "-m") > > - the documentation now discusses the change as well as the existing > handling of merges; this involved a few extra cleanups. Try > "doc-diff" for a better view of the actual rendered changes. > > I do think longer term we'd be better off to stop having this maze > of ifdef'd inclusions, and just have gitdiff(7) which covers all of > the possibilities in human-readable text (so yes, you might see a > mention of diff-files while you're looking for info on git-log, but > that would also broaden your mind about how the different commands > work). But that's clearly outside the scope of this series. I think > what's here is a strict improvement. I agree. The new series looks good. Chris
Jeff King <peff@peff.net> writes: > On Tue, Jul 28, 2020 at 12:36:18PM -0400, Jeff King wrote: > >> This series just makes --first-parent imply -m. That doesn't change any >> output by itself, but does mean that diff options like "-p", "-S", etc, >> behave sensibly. This is definitely step in the right direction, thanks a lot for working on it! > > Here's a re-roll taking into account the discussion so far: > > - the escape hatch option name is flipped to "--no-diff-merges" (with > "--diff-merges" matching "-m") Rather than being just a synonym for -m, is there a chance for "--diff-merges" implementation to be turned to output diff to the first parent only, no matter if --first-parent is active or not? Alternatively, may it have a parameter such as "-m parent-number" of "git cherry-pick" being set to "1" by default? This -m output of diffs to all the parents is in fact primary source of confusion for me, even over all these mind-blowing inter-dependencies between --first-parent, --cc, -c, -m, -p and what not. Who ever needs these (potentially huge) diffs against other parents, anyway? Introduction of this new option is a great opportunity for improvement that would be a pity to miss. Thanks, -- Sergey
On Thu, Jul 30, 2020 at 12:41:39AM +0300, Sergey Organov wrote: > > Here's a re-roll taking into account the discussion so far: > > > > - the escape hatch option name is flipped to "--no-diff-merges" (with > > "--diff-merges" matching "-m") > > Rather than being just a synonym for -m, is there a chance for > "--diff-merges" implementation to be turned to output diff to the first > parent only, no matter if --first-parent is active or not? > > Alternatively, may it have a parameter such as "-m parent-number" of > "git cherry-pick" being set to "1" by default? Yes, I agree that would be a useful feature, but I don't think it needs to be part of this series. It could be implemented as --diff-merges=1 to show only the one against the first parent, or as its own option. But we can add that on top. > This -m output of diffs to all the parents is in fact primary source of > confusion for me, even over all these mind-blowing inter-dependencies > between --first-parent, --cc, -c, -m, -p and what not. Who ever needs > these (potentially huge) diffs against other parents, anyway? I've used "-m" second-parent diffs occasionally for hunting down mismerges, etc, but I agree that most of the time you just want to see the diff against the first parent. > Introduction of this new option is a great opportunity for improvement > that would be a pity to miss. Adding an optional value to the flag is something we can do later. We would miss the opportunity for "--diff-merges" to default to "--diff-merges=1", but I'm not sure I'd want to do that anyway. Having it be consistent with "-m" seems less confusing to me, and it is already too late to change that. If we want an option that defaults to "1", we can give it a new name. The only thing that is lost now is that --diff-merges would already be taken. :) But I think I'd probably call such an option "--diff-parents" or something like that anyway. -Peff
Jeff King <peff@peff.net> writes: > On Thu, Jul 30, 2020 at 12:41:39AM +0300, Sergey Organov wrote: > >> > Here's a re-roll taking into account the discussion so far: >> > >> > - the escape hatch option name is flipped to "--no-diff-merges" (with >> > "--diff-merges" matching "-m") >> >> Rather than being just a synonym for -m, is there a chance for >> "--diff-merges" implementation to be turned to output diff to the first >> parent only, no matter if --first-parent is active or not? >> >> Alternatively, may it have a parameter such as "-m parent-number" of >> "git cherry-pick" being set to "1" by default? > > Yes, I agree that would be a useful feature, but I don't think it needs > to be part of this series. It could be implemented as --diff-merges=1 to > show only the one against the first parent, or as its own option. But we > can add that on top. > >> This -m output of diffs to all the parents is in fact primary source of >> confusion for me, even over all these mind-blowing inter-dependencies >> between --first-parent, --cc, -c, -m, -p and what not. Who ever needs >> these (potentially huge) diffs against other parents, anyway? > > I've used "-m" second-parent diffs occasionally for hunting down > mismerges, etc, but I agree that most of the time you just want to see > the diff against the first parent. > >> Introduction of this new option is a great opportunity for improvement >> that would be a pity to miss. > > Adding an optional value to the flag is something we can do later. We > would miss the opportunity for "--diff-merges" to default to > "--diff-merges=1", but I'm not sure I'd want to do that anyway. Having > it be consistent with "-m" seems less confusing to me, and it is already > too late to change that. > > If we want an option that defaults to "1", we can give it a new name. > The only thing that is lost now is that --diff-merges would already be > taken. :) But I think I'd probably call such an option "--diff-parents" > or something like that anyway. Yeah, I see your point, thanks for considering! What I in fact have in mind is something like: --diff-merges[=<parent-number>|c|cc|all] to have a single point of definition of the needed format of merges representation. Then comes the question of the default. "all" is what'd make it behave as "-m" behaves now. If it's too late for --diff-merges to have different default, could the default possibly be a configuration option rather than yet another command-line option? -- Sergey
Sergey Organov <sorganov@gmail.com> writes: > What I in fact have in mind is something like: > > --diff-merges[=<parent-number>|c|cc|all] > > to have a single point of definition of the needed format of merges > representation. A command line option that takes _optional_ argument is evil; it hurts teachability (e.g. "why does this option always require "--opt=val" and refuses '--opt val'?"). Making the optional value configurable is even more so (e.g. "teacher said to use '--opt', but it does not behave the way she taught---ah, I have this config"). > Then comes the question of the default. "all" is what'd make it behave > as "-m" behaves now. If it's too late for --diff-merges to have > different default, could the default possibly be a configuration > option rather than yet another command-line option? Introduce --diff-parent=(none|<parent-number>|c|cc|all) that is different from --diff-merges, and -m and --[no-]diff-merges can be defined in terms of that, at which point we can gradually deprecate them if we wanted to, no?
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> What I in fact have in mind is something like: >> >> --diff-merges[=<parent-number>|c|cc|all] >> >> to have a single point of definition of the needed format of merges >> representation. > > A command line option that takes _optional_ argument is evil; it > hurts teachability (e.g. "why does this option always require > "--opt=val" and refuses '--opt val'?"). Yeah, I sympathize. > Making the optional value configurable is even more so (e.g. "teacher > said to use '--opt', but it does not behave the way she taught---ah, I > have this config"). OK, let's drop the suggestion then. > >> Then comes the question of the default. "all" is what'd make it behave >> as "-m" behaves now. If it's too late for --diff-merges to have >> different default, could the default possibly be a configuration >> option rather than yet another command-line option? > > Introduce --diff-parent=(none|<parent-number>|c|cc|all) that is > different from --diff-merges, and -m and --[no-]diff-merges can be > defined in terms of that, at which point we can gradually deprecate > them if we wanted to, no? Sounds great, I only hoped we can do it right now, with this new --diff-merges option, maybe as a pre-requisite to the patches in question, but Jeff said it's too late, dunno why. Thanks, -- Sergey
Sergey Organov <sorganov@gmail.com> writes: > Sounds great, I only hoped we can do it right now, with this new > --diff-merges option, maybe as a pre-requisite to the patches in > question, but Jeff said it's too late, dunno why. A follow-up patch or two to remove the "--diff-merges" option and add the "--diff-parents=(none|<number>|c|cc|all)" option on top of the jk/log-fp-implies-m topic BEFORE it graduates to 'master' is a possibility. But is it worth the delay? I dunno.
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> Sounds great, I only hoped we can do it right now, with this new >> --diff-merges option, maybe as a pre-requisite to the patches in >> question, but Jeff said it's too late, dunno why. > > A follow-up patch or two to remove the "--diff-merges" option and > add the "--diff-parents=(none|<number>|c|cc|all)" option on top of > the jk/log-fp-implies-m topic BEFORE it graduates to 'master' is a > possibility. > > But is it worth the delay? I dunno. I don't think it's worth the delay, provided yet another new --diff-parents is to be implemented rather that using --diff-merges for that. -- Sergey
Sergey Organov <sorganov@gmail.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Sergey Organov <sorganov@gmail.com> writes: >> >>> Sounds great, I only hoped we can do it right now, with this new >>> --diff-merges option, maybe as a pre-requisite to the patches in >>> question, but Jeff said it's too late, dunno why. >> >> A follow-up patch or two to remove the "--diff-merges" option and >> add the "--diff-parents=(none|<number>|c|cc|all)" option on top of >> the jk/log-fp-implies-m topic BEFORE it graduates to 'master' is a >> possibility. >> >> But is it worth the delay? I dunno. > > I don't think it's worth the delay, provided yet another new > --diff-parents is to be implemented rather that using --diff-merges for > that. I was responding to your "it's too late, dunno why", as you seemed not to want to waste an option "--diff-merges" that will become unused when "--diff-parents" come and also wanted it to happen right now. If you no longer want to see it happen right now, that's OK by me.
On Mon, Aug 03, 2020 at 06:47:20PM +0300, Sergey Organov wrote: > > A command line option that takes _optional_ argument is evil; it > > hurts teachability (e.g. "why does this option always require > > "--opt=val" and refuses '--opt val'?"). > > Yeah, I sympathize. Sorry, the optional argument was my suggestion. I don't think they're that evil, but I agree they require extra knowledge for the user. I don't mind avoiding them when possible (and it's definitely possible here). > > Introduce --diff-parent=(none|<parent-number>|c|cc|all) that is > > different from --diff-merges, and -m and --[no-]diff-merges can be > > defined in terms of that, at which point we can gradually deprecate > > them if we wanted to, no? > > Sounds great, I only hoped we can do it right now, with this new > --diff-merges option, maybe as a pre-requisite to the patches in > question, but Jeff said it's too late, dunno why. It's too late for "-m" to change semantics (we could do a long deprecation, but I don't see much point in doing so). But --diff-merges is definitely still changeable until we release v2.29. My resistance was mostly that I didn't want to complicate my series by adding new elements. But we could do something on top. -Peff
Jeff King <peff@peff.net> writes: > On Mon, Aug 03, 2020 at 06:47:20PM +0300, Sergey Organov wrote: > >> > A command line option that takes _optional_ argument is evil; it >> > hurts teachability (e.g. "why does this option always require >> > "--opt=val" and refuses '--opt val'?"). >> >> Yeah, I sympathize. > > Sorry, the optional argument was my suggestion. I don't think they're > that evil, but I agree they require extra knowledge for the user. I > don't mind avoiding them when possible (and it's definitely possible > here). > >> > Introduce --diff-parent=(none|<parent-number>|c|cc|all) that is >> > different from --diff-merges, and -m and --[no-]diff-merges can be >> > defined in terms of that, at which point we can gradually deprecate >> > them if we wanted to, no? >> >> Sounds great, I only hoped we can do it right now, with this new >> --diff-merges option, maybe as a pre-requisite to the patches in >> question, but Jeff said it's too late, dunno why. > > It's too late for "-m" to change semantics (we could do a long > deprecation, but I don't see much point in doing so). I thought not of changing semantics of -m. Suppose we introduce --diff-merges=(none|<parent-number>|c|cc|all) before your patch(es). Then your patch would read: "making --first-parent imply --diff-merges=1" and it'd miss that --[no-]diff-merges part, no? > But --diff-merges is definitely still changeable until we release > v2.29. My resistance was mostly that I didn't want to complicate my > series by adding new elements. But we could do something on top. Can't we do yours on top instead? I'd expect it'd then be even simpler. Thanks, -- Sergey
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> Junio C Hamano <gitster@pobox.com> writes: >> >>> Sergey Organov <sorganov@gmail.com> writes: >>> >>>> Sounds great, I only hoped we can do it right now, with this new >>>> --diff-merges option, maybe as a pre-requisite to the patches in >>>> question, but Jeff said it's too late, dunno why. >>> >>> A follow-up patch or two to remove the "--diff-merges" option and >>> add the "--diff-parents=(none|<number>|c|cc|all)" option on top of >>> the jk/log-fp-implies-m topic BEFORE it graduates to 'master' is a >>> possibility. >>> >>> But is it worth the delay? I dunno. >> >> I don't think it's worth the delay, provided yet another new >> --diff-parents is to be implemented rather that using --diff-merges for >> that. > > I was responding to your "it's too late, dunno why", as you seemed > not to want to waste an option "--diff-merges" that will become > unused when "--diff-parents" come and also wanted it to happen right > now. If you no longer want to see it happen right now, that's OK by > me. Eh, no, as I see it, I suggested to have --diff-merges=(none|<number>|c|cc|all) right now rather than introduce yet another option (--diff-parents) later, as well as to make --first-parent imply --diff-merges=1 rather than "-m" (the latter in turn being synonym for --diff-merges=all), and I thought that's what was rejected by Jeff on the ground that it's too late, but as he clarified in his recent response it was not. I mean, why introduce --[no-]diff-merges in the first place, if we do agree --xxx=(none|...) is where we'd like to end up? I thought the answer was "it's too late", but in fact it was an answer to changing semantics of -m that I don't think I suggest. As a side-note, my secret hope is for pure "git log -p" to give me diff against first parent for all the commits, no matter how many parents they happen to have. This desire still sounds like a job for configuration option though, rather than, or in addition to, --diff-merges or --diff-parents? We well can later introduce a config to assume --diff-merges=<config> when no explicit --diff-merges=<value> is specified, right? Thanks, -- Sergey
On Mon, Aug 03, 2020 at 11:00:11PM +0300, Sergey Organov wrote: > > It's too late for "-m" to change semantics (we could do a long > > deprecation, but I don't see much point in doing so). > > I thought not of changing semantics of -m. Suppose we introduce > > --diff-merges=(none|<parent-number>|c|cc|all) > > before your patch(es). Then your patch would read: "making --first-parent > imply --diff-merges=1" and it'd miss that --[no-]diff-merges part, no? Sure, that would be OK with me. You'd have --diff-merges=none to get the current behavior, and probably make --no-diff-merges an alias for that. > > But --diff-merges is definitely still changeable until we release > > v2.29. My resistance was mostly that I didn't want to complicate my > > series by adding new elements. But we could do something on top. > > Can't we do yours on top instead? I'd expect it'd then be even simpler. Mine is in 'next', so there is no rebuilding it on top of anything else without a revert. But I don't see any particular reason to do that versus just changing the behavior on top. What's in 'next' is generally not rewound, but the behaviors are not cemented with respect to backwards compatibility. -Peff
On Mon, Aug 03, 2020 at 11:25:05PM +0300, Sergey Organov wrote: > I mean, why introduce --[no-]diff-merges in the first place, if we do > agree --xxx=(none|...) is where we'd like to end up? I thought the > answer was "it's too late", but in fact it was an answer to changing > semantics of -m that I don't think I suggest. Spelling out the between the lines of my answer a bit more, it was really: I am happy enough with the topic as-is and do not want to rework it again. But if _you_ want to do so, I have no problem with it. :) > As a side-note, my secret hope is for pure "git log -p" to give me diff > against first parent for all the commits, no matter how many parents > they happen to have. This desire still sounds like a job for > configuration option though, rather than, or in addition to, > --diff-merges or --diff-parents? We well can later introduce a > config to assume --diff-merges=<config> when no explicit > --diff-merges=<value> is specified, right? Yes, I think a config there would be reasonable as long as: - we have the command-line options to counteract it when necessary (i.e., --diff-parents or your advanced --diff-merges exists, too) - it only impacts porcelain "git log", and not plumbing like diff-tree. -Peff
Jeff King <peff@peff.net> writes: > On Mon, Aug 03, 2020 at 11:25:05PM +0300, Sergey Organov wrote: > >> I mean, why introduce --[no-]diff-merges in the first place, if we do >> agree --xxx=(none|...) is where we'd like to end up? I thought the >> answer was "it's too late", but in fact it was an answer to changing >> semantics of -m that I don't think I suggest. > > Spelling out the between the lines of my answer a bit more, it was > really: I am happy enough with the topic as-is and do not want to rework > it again. But if _you_ want to do so, I have no problem with it. :) OK, I see! > >> As a side-note, my secret hope is for pure "git log -p" to give me diff >> against first parent for all the commits, no matter how many parents >> they happen to have. This desire still sounds like a job for >> configuration option though, rather than, or in addition to, >> --diff-merges or --diff-parents? We well can later introduce a >> config to assume --diff-merges=<config> when no explicit >> --diff-merges=<value> is specified, right? > > Yes, I think a config there would be reasonable as long as: > > - we have the command-line options to counteract it when necessary > (i.e., --diff-parents or your advanced --diff-merges exists, too) > > - it only impacts porcelain "git log", and not plumbing like > diff-tree. Yeah, these two are totally agreed upon. Thanks, -- Sergey
Jeff King <peff@peff.net> writes: > On Mon, Aug 03, 2020 at 11:00:11PM +0300, Sergey Organov wrote: > >> > It's too late for "-m" to change semantics (we could do a long >> > deprecation, but I don't see much point in doing so). >> >> I thought not of changing semantics of -m. Suppose we introduce >> >> --diff-merges=(none|<parent-number>|c|cc|all) >> >> before your patch(es). Then your patch would read: "making --first-parent >> imply --diff-merges=1" and it'd miss that --[no-]diff-merges part, no? > > Sure, that would be OK with me. You'd have --diff-merges=none to get > the current behavior, and probably make --no-diff-merges an alias for > that. Yes, keeping --no-diff-merges as an alias might make sense, especially if it's on top of yours. > >> > But --diff-merges is definitely still changeable until we release >> > v2.29. My resistance was mostly that I didn't want to complicate my >> > series by adding new elements. But we could do something on top. >> >> Can't we do yours on top instead? I'd expect it'd then be even simpler. > > Mine is in 'next', so there is no rebuilding it on top of anything else > without a revert. But I don't see any particular reason to do that > versus just changing the behavior on top. What's in 'next' is generally > not rewound, but the behaviors are not cemented with respect to > backwards compatibility. Ah, now I see, thanks! -- Sergey
Jeff King <peff@peff.net> writes: > It's too late for "-m" to change semantics (we could do a long > deprecation, but I don't see much point in doing so). But --diff-merges > is definitely still changeable until we release v2.29. My resistance was > mostly that I didn't want to complicate my series by adding new > elements. But we could do something on top. Attached is rather minimal incompatible change to --diff-merges that'd allow extensions in the future, to get out of urge for the discussed changes. I'm going to follow-up with actual improvements and I'm aware it lacks documentation changes. What do you think, is it OK to have something like this before v2.29? And, by the way, what's approximate timeline to v2.29? As for me, I'm not sure 'combined-all-paths' should be included and if it should, is it a good enough name. In addition, do we need more descriptive (additional) names for "c" and "cc" modes? -- Sergey
Sergey Organov <sorganov@gmail.com> writes: > Jeff King <peff@peff.net> writes: > >> It's too late for "-m" to change semantics (we could do a long >> deprecation, but I don't see much point in doing so). But --diff-merges >> is definitely still changeable until we release v2.29. My resistance was >> mostly that I didn't want to complicate my series by adding new >> elements. But we could do something on top. > > Attached is rather minimal incompatible change to --diff-merges that'd > allow extensions in the future, to get out of urge for the discussed > changes. I'm going to follow-up with actual improvements and I'm aware > it lacks documentation changes. The overall direction is probably OK, but I wouldn't call it minimal. > What do you think, is it OK to have something like this before v2.29? > And, by the way, what's approximate timeline to v2.29? tinyurl.com/gitCal > As for me, I'm not sure 'combined-all-paths' should be included and if > it should, is it a good enough name. As a user, I do not think I can guess, from the option name, what that option is trying to do. As a minimum patch, I think it is OK to have just 'all' and 'none' (not even c or cc, let alone the one with ultra-long name whose effect is mystery) before we let the result graduate to 'master'. Others can be added on top, as the primary focus of Peff's series is to make sure "-m" can be countermanded, for which being able to say "no" is sufficient, and the primary reason why we are further futzing with the series with this addition is to leave the door open for later additions of different "modes" in which how "--diff-merges" option can operate (iow, Peff's was merely on/off, but you are making sure others such as <num> can be added over time). Thanks.
On Tue, Aug 04, 2020 at 08:50:16PM +0300, Sergey Organov wrote: > Attached is rather minimal incompatible change to --diff-merges that'd > allow extensions in the future, to get out of urge for the discussed > changes. I'm going to follow-up with actual improvements and I'm aware > it lacks documentation changes. Thanks, I like the direction here. Definitely it would need documentation, but also tests (probably in t4013 alongside the ones my series added; in fact you'd probably need to adjust my tests for the non-optional argument). > diff --git a/revision.c b/revision.c > index 669bc856694f..dcdff59bc36a 100644 > --- a/revision.c > +++ b/revision.c > @@ -2323,10 +2323,31 @@ 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 ((argcount = parse_long_opt("diff-merges", argv, &optarg))) { > revs->ignore_merges = 0; > + if (!strcmp(optarg, "off")) { > + revs->ignore_merges = 1; > + } else if (!strcmp(optarg, "all")) { > + revs->diff = 0; Should this be revs->ignore_merges = 0? > + } else if (!strcmp(optarg, "c")) { > + revs->diff = 1; > + revs->dense_combined_merges = 0; > + revs->combine_merges = 1; > + } else if (!strcmp(optarg, "cc")) { > + revs->diff = 1; > + revs->dense_combined_merges = 1; > + revs->combine_merges = 1; > + } else if (!strcmp(optarg, "combined-all-paths")) { > + revs->diff = 1; > + revs->combined_all_paths = 1; I think Junio's suggestion to push these out to a separate patch is a good one. It's unfortunate that we have to duplicate all of the various options that get set (from the "--cc", etc, blocks). But I think the boilerplate for pushing it into a helper would make it even harder to read. > + } else { > + die("--diff-merges: unknown value '%s'.", optarg); > + } A few nits: - we usually don't have a period at the end of our error messages - this should probably be marked for translation, i.e., die(_("translated message"), optarg) - I think other similar messages are more like: unknown value for --diff-merges: %s > + return argcount; > } else if (!strcmp(arg, "--no-diff-merges")) { > revs->ignore_merges = 1; I thought at first that the --no- form would be handled by parse_long_opt() via the parseopt code, but it is not using parseopt at all. :) So it is correct to keep this. -Peff
On Tue, Aug 04, 2020 at 12:42:45PM -0700, Junio C Hamano wrote: > As a minimum patch, I think it is OK to have just 'all' and 'none' > (not even c or cc, let alone the one with ultra-long name whose > effect is mystery) before we let the result graduate to 'master'. > Others can be added on top, as the primary focus of Peff's series is > to make sure "-m" can be countermanded, for which being able to say > "no" is sufficient, and the primary reason why we are further > futzing with the series with this addition is to leave the door open > for later additions of different "modes" in which how > "--diff-merges" option can operate (iow, Peff's was merely on/off, > but you are making sure others such as <num> can be added over > time). I like that suggestion very much. It solves the "optional arguments are evil" problem without having to worry about the other bits. -Peff
Jeff King <peff@peff.net> writes: > On Tue, Aug 04, 2020 at 12:42:45PM -0700, Junio C Hamano wrote: > >> As a minimum patch, I think it is OK to have just 'all' and 'none' >> (not even c or cc, let alone the one with ultra-long name whose >> effect is mystery) before we let the result graduate to 'master'. >> Others can be added on top, as the primary focus of Peff's series is >> to make sure "-m" can be countermanded, for which being able to say >> "no" is sufficient, and the primary reason why we are further >> futzing with the series with this addition is to leave the door open >> for later additions of different "modes" in which how >> "--diff-merges" option can operate (iow, Peff's was merely on/off, >> but you are making sure others such as <num> can be added over >> time). > > I like that suggestion very much. It solves the "optional arguments are > evil" problem without having to worry about the other bits. So do I, will do in a few minutes. I only don't like --diff-merges=none (even though it sounds great for --diff-parents=none) and used --diff-merges=off instead. It's not a strong feeling though, and I'm fine with whatever we decide. Thanks, -- Sergey
Jeff King <peff@peff.net> writes: > On Tue, Aug 04, 2020 at 08:50:16PM +0300, Sergey Organov wrote: > >> Attached is rather minimal incompatible change to --diff-merges that'd >> allow extensions in the future, to get out of urge for the discussed >> changes. I'm going to follow-up with actual improvements and I'm aware >> it lacks documentation changes. > > Thanks, I like the direction here. Definitely it would need > documentation, but also tests (probably in t4013 alongside the ones my > series added; in fact you'd probably need to adjust my tests for the > non-optional argument). > >> diff --git a/revision.c b/revision.c >> index 669bc856694f..dcdff59bc36a 100644 >> --- a/revision.c >> +++ b/revision.c >> @@ -2323,10 +2323,31 @@ 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 ((argcount = parse_long_opt("diff-merges", argv, &optarg))) { >> revs->ignore_merges = 0; >> + if (!strcmp(optarg, "off")) { >> + revs->ignore_merges = 1; >> + } else if (!strcmp(optarg, "all")) { >> + revs->diff = 0; > > Should this be revs->ignore_merges = 0? It's 4 lines above, as it's in fact common for all the cases but the first one. -- Sergey
Jeff King <peff@peff.net> writes: > On Tue, Aug 04, 2020 at 12:42:45PM -0700, Junio C Hamano wrote: > >> As a minimum patch, I think it is OK to have just 'all' and 'none' >> (not even c or cc, let alone the one with ultra-long name whose >> effect is mystery) before we let the result graduate to 'master'. >> Others can be added on top, as the primary focus of Peff's series is >> to make sure "-m" can be countermanded, for which being able to say >> "no" is sufficient, and the primary reason why we are further >> futzing with the series with this addition is to leave the door open >> for later additions of different "modes" in which how >> "--diff-merges" option can operate (iow, Peff's was merely on/off, >> but you are making sure others such as <num> can be added over >> time). > > I like that suggestion very much. It solves the "optional arguments are > evil" problem without having to worry about the other bits. Here is the patch reduced to absolute minimum, both functionally and textually. I removed even 'all', as it has its own subtleties that need further discussion, so the patch only introduces --diff-merges=off. If it looks OK, I'll do documentation and tests parts. Thanks, -- Sergey
On Tue, Aug 04, 2020 at 11:55:19PM +0300, Sergey Organov wrote: > I only don't like --diff-merges=none (even though it sounds great for > --diff-parents=none) and used --diff-merges=off instead. It's not a > strong feeling though, and I'm fine with whatever we decide. I think that is fine. I took "none" to be "diff against none of the parents", which is the opposite of "all". But "off" conveys that, too. -Peff
On Tue, Aug 04, 2020 at 11:56:25PM +0300, Sergey Organov wrote: > >> diff --git a/revision.c b/revision.c > >> index 669bc856694f..dcdff59bc36a 100644 > >> --- a/revision.c > >> +++ b/revision.c > >> @@ -2323,10 +2323,31 @@ 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 ((argcount = parse_long_opt("diff-merges", argv, &optarg))) { > >> revs->ignore_merges = 0; > >> + if (!strcmp(optarg, "off")) { > >> + revs->ignore_merges = 1; > >> + } else if (!strcmp(optarg, "all")) { > >> + revs->diff = 0; > > > > Should this be revs->ignore_merges = 0? > > It's 4 lines above, as it's in fact common for all the cases but the > first one. Ah, I missed that. That raises more questions, though. ;) For "-m" we do not need to set revs->diff; why do we need to do so here? For "--cc", we do not need to set revs->ignore_merges. Why do we need to do so here? We do need it set eventually, but I think setup_revisions() later handles that, and wants ignore_merges untouched to decide whether the user asked for it or not. -Peff
On Wed, Aug 05, 2020 at 12:21:03AM +0300, Sergey Organov wrote: > Here is the patch reduced to absolute minimum, both functionally and > textually. I removed even 'all', as it has its own subtleties that need > further discussion, so the patch only introduces --diff-merges=off. Yeah, I agree this is the minimum (though I suspect the documentation may be easier with "all" or similar to explain "-m" in terms of --diff-merges). > If it looks OK, I'll do documentation and tests parts. My nits about the die() message remain, but other than that it looks OK to me. -Peff
Sergey Organov <sorganov@gmail.com> writes: > Jeff King <peff@peff.net> writes: > >>> + } else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) { >>> revs->ignore_merges = 0; >>> + if (!strcmp(optarg, "off")) { >>> + revs->ignore_merges = 1; >>> + } else if (!strcmp(optarg, "all")) { >>> + revs->diff = 0; >> >> Should this be revs->ignore_merges = 0? > > It's 4 lines above, as it's in fact common for all the cases but the > first one. I may be mistaken, but I thought Peff was asking about turning revs->diff off. I somehow thought that the equivalence planned for the short term is: (new) (peff's) (master) diff-merges=none == --no-diff-merges == ! -m diff-merges=all == --diff-merges == -m and future extension would add equivalents to --cc and -c, in addition to <parentNum>, which is not something we can do with the current UI and machinery. So, shouldn't the body of that else if clause for "all" be a no-op? i.e. } else if (!strcmp(optarg, "all")) { ; /* nothing */ }
Jeff King <peff@peff.net> writes: > On Tue, Aug 04, 2020 at 11:56:25PM +0300, Sergey Organov wrote: > >> >> diff --git a/revision.c b/revision.c >> >> index 669bc856694f..dcdff59bc36a 100644 >> >> --- a/revision.c >> >> +++ b/revision.c >> >> @@ -2323,10 +2323,31 @@ 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 ((argcount = parse_long_opt("diff-merges", argv, &optarg))) { >> >> revs->ignore_merges = 0; >> >> + if (!strcmp(optarg, "off")) { >> >> + revs->ignore_merges = 1; >> >> + } else if (!strcmp(optarg, "all")) { >> >> + revs->diff = 0; >> > >> > Should this be revs->ignore_merges = 0? >> >> It's 4 lines above, as it's in fact common for all the cases but the >> first one. > > Ah, I missed that. That raises more questions, though. ;) > > For "-m" we do not need to set revs->diff; why do we need to do so > here? Good question! I believe --diff-merges=all should reset revs->diff back to 0 to entirely undo all the effects of '-c' and '--cc', provided those appeared before on the command-line, that '-m' fails to do. Admittedly, I didn't yet check what is the outcome of, say, "git log -c -m". Is it = "-c", ="-m", or what? Also, this makes 'all' not the entire synonym for '-m', and that's why I removed 'all' from the second version of the patch ;-) > > For "--cc", we do not need to set revs->ignore_merges. Why do we need to > do so here? We do need it set eventually, but I think setup_revisions() > later handles that, and wants ignore_merges untouched to decide whether > the user asked for it or not. OK, I'll take further look at this for further changes, -- thanks for telling! My general approach though is that every of mutually exclusive options should better explicitly set all the involved variables appropriately. Thanks, -- Sergey
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> Jeff King <peff@peff.net> writes: >> >>>> + } else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) { >>>> revs->ignore_merges = 0; >>>> + if (!strcmp(optarg, "off")) { >>>> + revs->ignore_merges = 1; >>>> + } else if (!strcmp(optarg, "all")) { >>>> + revs->diff = 0; >>> >>> Should this be revs->ignore_merges = 0? >> >> It's 4 lines above, as it's in fact common for all the cases but the >> first one. > > I may be mistaken, but I thought Peff was asking about turning > revs->diff off. No, but this one was in his follow-up that I already answered a few minutes ago. > I somehow thought that the equivalence planned for > the short term is: > > (new) (peff's) (master) > diff-merges=none == --no-diff-merges == ! -m > diff-merges=all == --diff-merges == -m The second one is somewhat problematic, so I excluded it for now (see aforementioned answer for more discussion). Thanks, -- Sergey
Jeff King <peff@peff.net> writes: > On Tue, Aug 04, 2020 at 11:55:19PM +0300, Sergey Organov wrote: > >> I only don't like --diff-merges=none (even though it sounds great for >> --diff-parents=none) and used --diff-merges=off instead. It's not a >> strong feeling though, and I'm fine with whatever we decide. > > I think that is fine. I took "none" to be "diff against none of the > parents", which is the opposite of "all". But "off" conveys that, too. For now, "off" is OK, but then we'll regret when "all" comes, because "off" would not exactly sit opposite to "all". On the other hand, "none" would: Compare with all parents? Compare with none of the parents? Thanks.
Jeff King <peff@peff.net> writes: > On Tue, Aug 04, 2020 at 08:50:16PM +0300, Sergey Organov wrote: [...] > >> + } else { >> + die("--diff-merges: unknown value '%s'.", optarg); >> + } > > A few nits: I missed this the first time, sorry! > > - we usually don't have a period at the end of our error messages Oops, I got the dot from die("--unpacked=<packfile> no longer supported."); in the same file. Will fix. > - this should probably be marked for translation, i.e., > die(_("translated message"), optarg) OK, will do. > > - I think other similar messages are more like: > > unknown value for --diff-merges: %s Thanks, I'll change wording to this one. -- Sergey
Junio C Hamano <gitster@pobox.com> writes: > Jeff King <peff@peff.net> writes: > >> On Tue, Aug 04, 2020 at 11:55:19PM +0300, Sergey Organov wrote: >> >>> I only don't like --diff-merges=none (even though it sounds great for >>> --diff-parents=none) and used --diff-merges=off instead. It's not a >>> strong feeling though, and I'm fine with whatever we decide. >> >> I think that is fine. I took "none" to be "diff against none of the >> parents", which is the opposite of "all". But "off" conveys that, too. > > For now, "off" is OK, but then we'll regret when "all" comes, > because "off" would not exactly sit opposite to "all". IMHO, "off" does not need to be opposite for "all", as it suppresses diff output altogether. I read --diff-merge=off as "turn /off/ diff output for merge commits". Besides, "all", that I don't like either, is among "c" and "cc", all 3 being different versions of diffs against all the parents, no? > On the other hand, "none" would: Compare with all parents? Compare > with none of the parents? I think "none" would have been appropriate for --diff-parents indeed, but not for --diff-merges. Thanks, -- Sergey
On Wed, Aug 05, 2020 at 12:41:25AM +0300, Sergey Organov wrote: > >> It's 4 lines above, as it's in fact common for all the cases but the > >> first one. > > > > Ah, I missed that. That raises more questions, though. ;) > > > > For "-m" we do not need to set revs->diff; why do we need to do so > > here? > > Good question! > > I believe --diff-merges=all should reset revs->diff back to 0 to > entirely undo all the effects of '-c' and '--cc', provided those > appeared before on the command-line, that '-m' fails to do. Making it counteract "--cc" makes sense, but revs->diff is used for much more than that. So "--cc" sets revs->diff to 1, but so do many other unrelated options (e.g., "--full-diff" for one). I think to do it right you'd need to have this part of the code just set a single enum variable, like: ... else if (!strcmp(arg, "--cc")) { revs->diff_merges = DIFF_MERGES_DENSE_COMBINED; } else if (!strcmp(arg, "-m")) { revs->diff_merges = DIFF_MERGES_ALL_PARENTS; } ...etc... and then later resolve that into the set of flags we need: switch (revs->diff_merges) { case DIFF_MERGES_DENSE_COMBINED: revs->diff = 1; revs->dense_combined_merges = 1; revs->combine_merges = 1; revs->ignore_merges = 0; break; case DIFF_MERGES_ALL_PARENTS: revs->ignore_merges = 0; break; ...etc... } it may even be that we can get rid of the separate combine_merges and dense_combined_merges flag and just have callers look at rev.diff_merges, which would simplify the code even further. But that cleanup could also come later on top. > Admittedly, I didn't yet check what is the outcome of, say, > "git log -c -m". Is it = "-c", ="-m", or what? Having looked at the code, I'm pretty sure it behaves like "-c". IMHO that is a bug and the two should be mutually exclusive (i.e., override each other). Changing that would not be strictly backwards, but I think it may be OK under the notion that the current behavior is nonsense. -Peff
On Wed, Aug 05, 2020 at 12:58:03AM +0300, Sergey Organov wrote: > > - we usually don't have a period at the end of our error messages > > Oops, I got the dot from > > die("--unpacked=<packfile> no longer supported."); > > in the same file. Will fix. Yeah, there are a few that have snuck in. I wouldn't be opposed to a patch to fix that one. :) -Peff
On Wed, Aug 05, 2020 at 01:06:51AM +0300, Sergey Organov wrote: > > For now, "off" is OK, but then we'll regret when "all" comes, > > because "off" would not exactly sit opposite to "all". > > IMHO, "off" does not need to be opposite for "all", as it suppresses > diff output altogether. I read --diff-merge=off as "turn /off/ diff > output for merge commits". > > Besides, "all", that I don't like either, is among "c" and "cc", all 3 > being different versions of diffs against all the parents, no? I think "all-parents" is much more descriptive than "all" (which might make you think "all merges", but it has nothing to do with that). It would make more sense if we later add the building to say "diff against parent 1" or "diff against parents 1 and 3". You might also consider whether "combined" is actually mutually exclusive with parent selection. We have focused on which parents you'd want to "-m" against. But in the most general case, you could ask for a combined-diff between parents 1 and 3 of an octopus merge. That's just coming from the angle of "what is the most general and orthogonal set of features". I think the vast majority of what anyone would want to do would be covered by doing a diff against only a single parent, and then it would almost always be the first parent. And certainly you'd need to add a bunch of code to the combined diff machinery to make it support arbitrary sets of parents. So this probably isn't that interesting a direction to go, at least for now. I'm just raising the issue now because we'll be locked into the semantics of this option, which may not be able to express the full set of what's possible (so we'd be stuck adding another option later). -Peff
Jeff King <peff@peff.net> writes: > On Wed, Aug 05, 2020 at 12:41:25AM +0300, Sergey Organov wrote: > >> >> It's 4 lines above, as it's in fact common for all the cases but the >> >> first one. >> > >> > Ah, I missed that. That raises more questions, though. ;) >> > >> > For "-m" we do not need to set revs->diff; why do we need to do so >> > here? >> >> Good question! >> >> I believe --diff-merges=all should reset revs->diff back to 0 to >> entirely undo all the effects of '-c' and '--cc', provided those >> appeared before on the command-line, that '-m' fails to do. > > Making it counteract "--cc" makes sense, but revs->diff is used for much > more than that. So "--cc" sets revs->diff to 1, but so do many other > unrelated options (e.g., "--full-diff" for one). > > I think to do it right you'd need to have this part of the code just set > a single enum variable, like: > > ... > else if (!strcmp(arg, "--cc")) { > revs->diff_merges = DIFF_MERGES_DENSE_COMBINED; > } else if (!strcmp(arg, "-m")) { > revs->diff_merges = DIFF_MERGES_ALL_PARENTS; > } > ...etc... > > and then later resolve that into the set of flags we need: > > switch (revs->diff_merges) { > case DIFF_MERGES_DENSE_COMBINED: > revs->diff = 1; > revs->dense_combined_merges = 1; > revs->combine_merges = 1; > revs->ignore_merges = 0; > break; > case DIFF_MERGES_ALL_PARENTS: > revs->ignore_merges = 0; > break; > ...etc... > } > > it may even be that we can get rid of the separate combine_merges and > dense_combined_merges flag and just have callers look at > rev.diff_merges, which would simplify the code even further. But that > cleanup could also come later on top. Sounds like a plan! I like it. Thanks, -- Sergey
Jeff King <peff@peff.net> writes: > You might also consider whether "combined" is actually mutually > exclusive with parent selection. We have focused on which parents you'd > want to "-m" against. But in the most general case, you could ask for a > combined-diff between parents 1 and 3 of an octopus merge. Yeah, we want to specify a possibly empty set of integers (1..<num-parents>) to combine the diff; if it is empty set, we won't see any diff. If it is full set, we'd get the current c/cc behavior. Anything in between we cannot currently express. Fun ;-) > That's just coming from the angle of "what is the most general and > orthogonal set of features". I think the vast majority of what anyone > would want to do would be covered by doing a diff against only a single > parent, and then it would almost always be the first parent. And > certainly you'd need to add a bunch of code to the combined diff > machinery to make it support arbitrary sets of parents. So this probably > isn't that interesting a direction to go, at least for now. Yeah, it is mostly for fun--I do not see an immediate practical use case, either. > I'm just > raising the issue now because we'll be locked into the semantics of this > option, which may not be able to express the full set of what's possible > (so we'd be stuck adding another option later). Yeah, but a good thing is that we won't have to worry about this until much later, as long as we would just be introducing "diff against no parents" and nothing else (or together with "diff against all parents", which would make it easier to explain "-m").
Jeff King <peff@peff.net> writes: > On Wed, Aug 05, 2020 at 01:06:51AM +0300, Sergey Organov wrote: > >> > For now, "off" is OK, but then we'll regret when "all" comes, >> > because "off" would not exactly sit opposite to "all". >> >> IMHO, "off" does not need to be opposite for "all", as it suppresses >> diff output altogether. I read --diff-merge=off as "turn /off/ diff >> output for merge commits". >> >> Besides, "all", that I don't like either, is among "c" and "cc", all 3 >> being different versions of diffs against all the parents, no? > > I think "all-parents" is much more descriptive than "all" (which might > make you think "all merges", but it has nothing to do with that). It > would make more sense if we later add the building to say "diff against > parent 1" or "diff against parents 1 and 3". > > You might also consider whether "combined" is actually mutually > exclusive with parent selection. We have focused on which parents you'd > want to "-m" against. But in the most general case, you could ask for a > combined-diff between parents 1 and 3 of an octopus merge. > > That's just coming from the angle of "what is the most general and > orthogonal set of features". I think the vast majority of what anyone > would want to do would be covered by doing a diff against only a single > parent, and then it would almost always be the first parent. And > certainly you'd need to add a bunch of code to the combined diff > machinery to make it support arbitrary sets of parents. So this probably > isn't that interesting a direction to go, at least for now. I'm just > raising the issue now because we'll be locked into the semantics of this > option, which may not be able to express the full set of what's possible > (so we'd be stuck adding another option later). Makes sense, and I got an idea. --diff-merges=<parent> will still give diff against one specific parent. In case of combined/separate diffs, it will produce diffs against all the parents that, if happens to be needed, could later be refined by a new --diff-parents option that defaults to 'all'. Then, for example, --diff-merges=1 would finally be just a short-cut for --diff-merges=separate --diff-parents=1 while --diff-merges=separate --diff-parents=all would be the same as --diff-merges=separate (what we called "all" so far) and then we may have --diff-merges=condensed-combined --diff-parents=1-3,8 for the bravest ;-) This would lead us, currently to: --diff-merges=(off,separate,combined,condensed-combined,<parent>) and leave us ability to implement advanced parents selection, in an unlikely case it's needed, in a separate new option. Thanks, -- Sergey
Jeff King <peff@peff.net> writes: > On Tue, Aug 04, 2020 at 08:50:16PM +0300, Sergey Organov wrote: > >> Attached is rather minimal incompatible change to --diff-merges that'd >> allow extensions in the future, to get out of urge for the discussed >> changes. I'm going to follow-up with actual improvements and I'm aware >> it lacks documentation changes. > > Thanks, I like the direction here. Definitely it would need > documentation, but also tests (probably in t4013 alongside the ones my > series added; in fact you'd probably need to adjust my tests for the > non-optional argument). I turned to tests, and found that I have a doubt about the test you've added: git log --no-diff-merges -p --first-parent master In modified tests, I'd like to move --no-diff-merges to the end, for the test to be less restrictive: git log -p --first-parent --no-diff-merges master It should change nothing for now, but it will allow us in the future to get rid of mutual dependencies between in -m and --first-parent in favor of --first-parent to imply --diff-merges=1. We then will need to override the latter by subsequent --no-diff-merges: git log -p --first-parent [--diff-merges=1: implied] --no-diff-merges master In this case your original test: git log --no-diff-merges -p --first-parent [--diff-merges=1: implied] master would fail, as implied --diff-merges=1 then wins. Then I'm going to add a copy: git log -p --first-parent --diff-merges=off master to check that this form works as well. What do you think? Thanks, -- Sergey
Sergey Organov <sorganov@gmail.com> writes: > Jeff King <peff@peff.net> writes: > ... > In this case your original test: > > git log --no-diff-merges -p --first-parent [--diff-merges=1: implied] master > > would fail, as implied --diff-merges=1 then wins. IMHO, I think this is an absolutely wrong thing to do. At least to me (and I suspect it would be to many users), what "--first-parent implies 'when showing a diff, compare only with the first parent'" means is that it should do so unless told to do otherwise. git log --no-diff-merges -p --first-parent explicitly tells the command that the user does not want to see patches for merge commits. I do not see any reason why "--first-parent", which merely *implies* a specific diff generation preference for merges, countermand it. IOW the implication is conditional. It is like saying git log --first-parent should show patches because it *implies* comparing only with the first parent, but you can see why it is wrong. It is because that implication is conditional---it kicks in only when the command is told to compare with any parent (i.e. "-p"). I.e. the implication is "compare only with the first parent if told to compare, and if not told what to compare with explicitly".
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> Jeff King <peff@peff.net> writes: >> ... >> In this case your original test: >> >> git log --no-diff-merges -p --first-parent [--diff-merges=1: implied] master >> >> would fail, as implied --diff-merges=1 then wins. > > IMHO, I think this is an absolutely wrong thing to do. At least to > me (and I suspect it would be to many users), what "--first-parent > implies 'when showing a diff, compare only with the first parent'" > means is that it should do so unless told to do otherwise. > > git log --no-diff-merges -p --first-parent > > explicitly tells the command that the user does not want to see > patches for merge commits. I do not see any reason why > "--first-parent", which merely *implies* a specific diff generation > preference for merges, countermand it. IOW the implication is > conditional. > > It is like saying > > git log --first-parent > > should show patches because it *implies* comparing only with the > first parent, but you can see why it is wrong. It is because that > implication is conditional---it kicks in only when the command is > told to compare with any parent (i.e. "-p"). > > I.e. the implication is "compare only with the first parent if told > to compare, and if not told what to compare with explicitly". I believe my approach is more straightforward and is free from interpretations. To make my point let's get back to the subject (for a moment :-)). To me "--first-parent implies -m" is simple and unambiguous: (git log [*] --first-parent [*]) == (git log [*] --first-parent -m [*]) No further explanations are needed. The consequence is that an option that is supposed to override -m must follow -m, and thus --first-parent, not precede it, period. Yes, we can invent the rule that implied options don't participate in overriding of explicit options, or that explicit option always overrides all the implicit, or some such, but I see absolutely no reason to complicate the model. Thanks, -- Sergey
Sergey Organov <sorganov@gmail.com> writes:
> Yes, we can invent the rule that implied options don't ...
"invent"? It is nothing new, isn't it? IIRC, Peff's "first-parent
implies 'm' but can be countermanded with --no-diff-merges" defines
"implication" exactly that way. I do not think that is a recent
invention but it is just following the patterns set by other options
that has conditional implications.
IOW,
$ git log --no-diff-merges --first-parent -p next
$ git log --first-parent -p --no-diff-merges next
should both mean the same thing. The user said no patch is wanted
for merge commits with --no-diff-merges and --first-parent does not
affect it.
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> Yes, we can invent the rule that implied options don't ... > > "invent"? It is nothing new, isn't it? IIRC, Peff's "first-parent > implies 'm' but can be countermanded with --no-diff-merges" defines > "implication" exactly that way. I do not think that is a recent > invention but it is just following the patterns set by other options > that has conditional implications. > > IOW, > > $ git log --no-diff-merges --first-parent -p next > $ git log --first-parent -p --no-diff-merges next > > should both mean the same thing. The user said no patch is wanted > for merge commits with --no-diff-merges and --first-parent does not > affect it. I disagree, but I drop the issue for now for the goal of making sensible progress with --diff-merges. I'll do the patches without this modifications of the tests. Thanks, -- Sergey.
On Tue, Aug 04, 2020 at 03:49:17PM -0700, Junio C Hamano wrote: > > I'm just > > raising the issue now because we'll be locked into the semantics of this > > option, which may not be able to express the full set of what's possible > > (so we'd be stuck adding another option later). > > Yeah, but a good thing is that we won't have to worry about this > until much later, as long as we would just be introducing "diff > against no parents" and nothing else (or together with "diff against > all parents", which would make it easier to explain "-m"). Agreed. My only question is whether the possibility of later having those other options might influence how we name the two options we add now. I think it's clear to all of us in this thread how those two easy options should behave, but if the intent is to eventually allow these to be mutually exclusive: - no diff - combined - dense combined - individual diff against each parent but orthogonal to the selection of the parent-set (none, all, or selected ones) then e.g. "all" makes less sense for "individual diff against each parent". I don't have a good succinct name suggestion, though. TBH, I would be happy enough with any of the suggestions in the thread, so I am really just finishing the thought here, and not trying to derail progress. :) -Peff
Jeff King <peff@peff.net> writes: > On Tue, Aug 04, 2020 at 03:49:17PM -0700, Junio C Hamano wrote: > >> > I'm just >> > raising the issue now because we'll be locked into the semantics of this >> > option, which may not be able to express the full set of what's possible >> > (so we'd be stuck adding another option later). >> >> Yeah, but a good thing is that we won't have to worry about this >> until much later, as long as we would just be introducing "diff >> against no parents" and nothing else (or together with "diff against >> all parents", which would make it easier to explain "-m"). > > Agreed. My only question is whether the possibility of later having > those other options might influence how we name the two options we add > now. I think it's clear to all of us in this thread how those two easy > options should behave, but if the intent is to eventually allow these to > be mutually exclusive: > > - no diff > - combined > - dense combined > - individual diff against each parent > > but orthogonal to the selection of the parent-set (none, all, or > selected ones) then e.g. "all" makes less sense for "individual diff > against each parent". I don't have a good succinct name suggestion, > though. I have "split" and "separate" in mind, the latter likely shortened to "sep". Overall: --diff-merges=(off,none|comb|dense,dense-comb,comb-dense|sep,split) -- Sergey
Jeff King <peff@peff.net> writes: > Agreed. My only question is whether the possibility of later having > those other options might influence how we name the two options we add > now. I think it's clear to all of us in this thread how those two easy > options should behave, but if the intent is to eventually allow these to > be mutually exclusive: > > - no diff > - combined > - dense combined > - individual diff against each parent > > but orthogonal to the selection of the parent-set (none, all, or > selected ones) then e.g. "all" makes less sense for "individual diff > against each parent". I don't have a good succinct name suggestion, > though. > > TBH, I would be happy enough with any of the suggestions in the thread, > so I am really just finishing the thought here, and not trying to derail > progress. :) I agree in principle that the above is a good framework to think about the issue around "what to do with diff when showing a merge commit", but I suspect that overly spending our effort to cover the possibilities become mostly useless mental exercise, mostly because (1) comparing with second parent is mostly useful only when the merge was done in the wrong direction (i.e. an attempt by a leaf contributor to "catch up to the trunc"), (2) octopus merges are rare curiosity and discouraged due to bisect efficiency anyway, and (3) even when looking at an octopus merge, omitting some and using only a few selected parents to view with --cc/-c has dubious usefulness, as the postimage has to show contributions from all the parents plus "evil" adjustment anyway (iow, the primary effect of omitting parents while viewing --cc/-c is to make it fuzzy which part of apparently "evil" adjustment is what the merge did vs what the hidden parents did). These are all examples that show not all the combinations are useful. So...
Junio C Hamano <gitster@pobox.com> writes: > Jeff King <peff@peff.net> writes: > >> Agreed. My only question is whether the possibility of later having >> those other options might influence how we name the two options we add >> now. I think it's clear to all of us in this thread how those two easy >> options should behave, but if the intent is to eventually allow these to >> be mutually exclusive: >> >> - no diff >> - combined >> - dense combined >> - individual diff against each parent >> >> but orthogonal to the selection of the parent-set (none, all, or >> selected ones) then e.g. "all" makes less sense for "individual diff >> against each parent". I don't have a good succinct name suggestion, >> though. >> >> TBH, I would be happy enough with any of the suggestions in the thread, >> so I am really just finishing the thought here, and not trying to derail >> progress. :) > > I agree in principle that the above is a good framework to think > about the issue around "what to do with diff when showing a merge > commit", but I suspect that overly spending our effort to cover the > possibilities become mostly useless mental exercise, mostly because > (1) comparing with second parent is mostly useful only when the > merge was done in the wrong direction (i.e. an attempt by a leaf > contributor to "catch up to the trunc"), (2) octopus merges are rare > curiosity and discouraged due to bisect efficiency anyway, and (3) > even when looking at an octopus merge, omitting some and using only > a few selected parents to view with --cc/-c has dubious usefulness, > as the postimage has to show contributions from all the parents > plus "evil" adjustment anyway (iow, the primary effect of omitting > parents while viewing --cc/-c is to make it fuzzy which part of > apparently "evil" adjustment is what the merge did vs what the > hidden parents did). These are all examples that show not all the > combinations are useful. > > So... So, does --diff-merges=(off,none|comb|dense,dense-comb,comb-dense|sep,split) make sense as covering all the current features? I've put variations that came to my mind. Probably we'd better just select one for each case. Thanks, -- Sergey
Sergey Organov <sorganov@gmail.com> writes: >> I agree in principle that the above is a good framework to think >> about the issue around "what to do with diff when showing a merge >> commit", but I suspect that overly spending our effort to cover the >> possibilities become mostly useless mental exercise, mostly because >> (1) comparing with second parent is mostly useful only when the >> merge was done in the wrong direction (i.e. an attempt by a leaf >> contributor to "catch up to the trunc"), (2) octopus merges are rare >> curiosity and discouraged due to bisect efficiency anyway, and (3) >> even when looking at an octopus merge, omitting some and using only >> a few selected parents to view with --cc/-c has dubious usefulness, >> as the postimage has to show contributions from all the parents >> plus "evil" adjustment anyway (iow, the primary effect of omitting >> parents while viewing --cc/-c is to make it fuzzy which part of >> apparently "evil" adjustment is what the merge did vs what the >> hidden parents did). These are all examples that show not all the >> combinations are useful. >> >> So... > > So, does > > --diff-merges=(off,none|comb|dense,dense-comb,comb-dense|sep,split) > > make sense as covering all the current features? If we are primarily interested in theoretical completeness, it may. If we are interested more in practical usefulness, I am not sure if such a "full flexibility" matrix is a good way to present the feature to the end-users.
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >>> I agree in principle that the above is a good framework to think >>> about the issue around "what to do with diff when showing a merge >>> commit", but I suspect that overly spending our effort to cover the >>> possibilities become mostly useless mental exercise, mostly because >>> (1) comparing with second parent is mostly useful only when the >>> merge was done in the wrong direction (i.e. an attempt by a leaf >>> contributor to "catch up to the trunc"), (2) octopus merges are rare >>> curiosity and discouraged due to bisect efficiency anyway, and (3) >>> even when looking at an octopus merge, omitting some and using only >>> a few selected parents to view with --cc/-c has dubious usefulness, >>> as the postimage has to show contributions from all the parents >>> plus "evil" adjustment anyway (iow, the primary effect of omitting >>> parents while viewing --cc/-c is to make it fuzzy which part of >>> apparently "evil" adjustment is what the merge did vs what the >>> hidden parents did). These are all examples that show not all the >>> combinations are useful. >>> >>> So... >> >> So, does >> >> --diff-merges=(off,none|comb|dense,dense-comb,comb-dense|sep,split) >> >> make sense as covering all the current features? > > If we are primarily interested in theoretical completeness, it may. > If we are interested more in practical usefulness, I am not sure if > such a "full flexibility" matrix is a good way to present the > feature to the end-users. I thought it's just a -c, -cc, and -m in better wording. No any matrix: -c = --diff-merges=combined -cc = --diff-merges=dense -m = --diff-merges=split Just separate mutually exclusive options assembled into one multi-value option, so it's explicit they are mutually exclusive. I don't see any matrix here. Thanks, -- Sergey
On Fri, Aug 07, 2020 at 10:12:29PM +0300, Sergey Organov wrote: > I thought it's just a -c, -cc, and -m in better wording. No any > matrix: > > -c = --diff-merges=combined > -cc = --diff-merges=dense > -m = --diff-merges=split > > Just separate mutually exclusive options assembled into one multi-value > option, so it's explicit they are mutually exclusive. I don't see any > matrix here. FWIW, those names make sense to me. Coupled with "none" or "off" for disabling all of them. -Peff
Jeff King <peff@peff.net> writes: > On Fri, Aug 07, 2020 at 10:12:29PM +0300, Sergey Organov wrote: > >> I thought it's just a -c, -cc, and -m in better wording. No any >> matrix: >> >> -c = --diff-merges=combined >> -cc = --diff-merges=dense >> -m = --diff-merges=split >> >> Just separate mutually exclusive options assembled into one multi-value >> option, so it's explicit they are mutually exclusive. I don't see any >> matrix here. > > FWIW, those names make sense to me. Coupled with "none" or "off" for > disabling all of them. Thanks, we have "off" along with your --no-diff-merges in already submitted patches, and I see no harm in adding "none" as synonym, as Junio seems to prefer it over "off". Thanks, -- Sergey
Sergey Organov <sorganov@gmail.com> writes: >>> So, does >>> >>> --diff-merges=(off,none|comb|dense,dense-comb,comb-dense|sep,split) >>> >>> make sense as covering all the current features? >> >> If we are primarily interested in theoretical completeness, it may. >> If we are interested more in practical usefulness, I am not sure if >> such a "full flexibility" matrix is a good way to present the >> feature to the end-users. > > I thought it's just a -c, -cc, and -m in better wording. No any > matrix: > > -c = --diff-merges=combined > -cc = --diff-merges=dense > -m = --diff-merges=split > > Just separate mutually exclusive options assembled into one multi-value > option, so it's explicit they are mutually exclusive. I don't see any > matrix here. Oh, matrix comes from specifying the set of parents in a separate parameter. If we are not doing that, then you cannot even express "when showing a merge, compare only with the first parent", no? And when you add --diff-parents=1 (i.e. diff with first-parent), you are opening the interface up so that it can express dubious combinations like --diff-merges=dense-combined --diff-parents=1,3 (i.e. --cc but exclude the second parent as one of the preimages). You also have a redundant combination, e.g. --diff-merges=(off,combined,dense-combined,each) --diff-parents="" would be the same as --diff-merges=off without saying which parents to compare with.
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >>>> So, does >>>> >>>> --diff-merges=(off,none|comb|dense,dense-comb,comb-dense|sep,split) >>>> >>>> make sense as covering all the current features? >>> >>> If we are primarily interested in theoretical completeness, it may. >>> If we are interested more in practical usefulness, I am not sure if >>> such a "full flexibility" matrix is a good way to present the >>> feature to the end-users. >> >> I thought it's just a -c, -cc, and -m in better wording. No any >> matrix: >> >> -c = --diff-merges=combined >> -cc = --diff-merges=dense >> -m = --diff-merges=split >> >> Just separate mutually exclusive options assembled into one multi-value >> option, so it's explicit they are mutually exclusive. I don't see any >> matrix here. > > Oh, matrix comes from specifying the set of parents in a separate > parameter. If we are not doing that, then you cannot even express > "when showing a merge, compare only with the first parent", no? > > And when you add --diff-parents=1 (i.e. diff with first-parent), you > are opening the interface up so that it can express dubious > combinations like --diff-merges=dense-combined --diff-parents=1,3 > (i.e. --cc but exclude the second parent as one of the preimages). I had no intention to introduce --diff-parents, at least for now, and maybe never. What I said about it was theoretical discussion rather than actual proposal. If we agree on the above, I intended to instead propose something like: --diff-merges=first-parent or just =first for a start. Thanks, -- Sergey
Sergey Organov <sorganov@gmail.com> writes: > I had no intention to introduce --diff-parents, at least for now, and > maybe never. What I said about it was theoretical discussion rather than > actual proposal. > > If we agree on the above, I intended to instead propose something like: > > --diff-merges=first-parent or just =first OK, so the combined, combined-dense and split were meant to work with all the parents, and off is the only one that means comparison with no parents. That makes sense.
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> I had no intention to introduce --diff-parents, at least for now, and >> maybe never. What I said about it was theoretical discussion rather than >> actual proposal. >> >> If we agree on the above, I intended to instead propose something like: >> >> --diff-merges=first-parent or just =first > > OK, so the combined, combined-dense and split were meant to work > with all the parents, and off is the only one that means comparison > with no parents. That makes sense. Yeah, exactly, thanks! The only question regarding it I then have for now is what are preferences for names selection inside single option? Abbreviated yet somewhat sensible, or verbose? I mean: --diff-merges=first vs --diff-merges=first-parent --diff-merges=comb vs --diff-merges=combined etc. What's better? Thanks, -- Sergey
Sergey Organov <sorganov@gmail.com> writes: > The only question regarding it I then have for now is what are > preferences for names selection inside single option? Abbreviated yet > somewhat sensible, or verbose? I mean: > > --diff-merges=first vs --diff-merges=first-parent > > --diff-merges=comb vs --diff-merges=combined > > etc. What's better? If we were to shoot for easy-to-type, we could go for ultra-short abbreviations like 'no', 'c', 'cc', 'each' (the last one is the traditional "-m" when used without "--first-parent"; diff with each parent) and later add 'fp', but in a sense we are already lost the easy-to-type goal by "--diff-merges" being sufficiently long. I personally wouldn't choose "first" or "first-parent", but just use "1", so that we could support "2" when viewing a merge that was done in the wrong direction with "git show", though. IOW, even though I said that "use these parents but not those" (i.e. set of parents) smells overkill, at least to me, I think specifying a single parent that is not necessarily the first one would be a reasonable thing to do. So, if I were to vote, it would be "--diff-merges=" ( first-parent | 1 | combined | c | dense-combined | cc | each-parent | m ) "--no-diff-merges" leaving some room to add '2' and <any posInt> later.
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> The only question regarding it I then have for now is what are >> preferences for names selection inside single option? Abbreviated yet >> somewhat sensible, or verbose? I mean: >> >> --diff-merges=first vs --diff-merges=first-parent >> >> --diff-merges=comb vs --diff-merges=combined >> >> etc. What's better? > > If we were to shoot for easy-to-type, we could go for ultra-short > abbreviations like 'no', 'c', 'cc', 'each' (the last one is the > traditional "-m" when used without "--first-parent"; diff with each > parent) and later add 'fp', but in a sense we are already lost the > easy-to-type goal by "--diff-merges" being sufficiently long. Do I get it right that there are no common guidelines and every case is to be considered separately? Anyway: $ git log -d fatal: unrecognized argument: -d $ git show -d fatal: unrecognized argument: -d so it seems we still have -d to (ab)use for, say "-d 1" or "-d m", if we decide to. > > I personally wouldn't choose "first" or "first-parent", but just use > "1", so that we could support "2" when viewing a merge that was done > in the wrong direction with "git show", though. I fail to see how using "first-parent" would deny using a number either later or right now, though based on your own rating of octopus merges with which I whole-heartedly agree, the only thing we'd ever need seems to be "second-parent", or 2. > IOW, even though I said that "use these parents but not those" (i.e. > set of parents) smells overkill, at least to me, I think specifying a > single parent that is not necessarily the first one would be a > reasonable thing to do. The second parent, I'd agree. Others? Well, that's more a completeness theoretical issue to me rather than practical one. More relevant to plumbing than to porcelain thereof. And, as additional safety, we still have that "all|every|each|split" -m that will show needed diff among others. > > So, if I were to vote, it would be > > "--diff-merges=" ( first-parent | 1 | > combined | c | > dense-combined | cc | > each-parent | m ) > "--no-diff-merges" > > leaving some room to add '2' and <any posInt> later. Thanks for voting! I believe we still have the room for digits even if we use first-parent, and then won't "--first-parent implies --diff-merges=first-parent" sound really cool? Overall, I incline to support short (traditional and numbered) variants along with new longer spellings at the same time, similar to short and long options. Thanks, -- Sergey