* Can we clarify the purpose of `git diff -s`? @ 2023-05-11 3:14 Felipe Contreras 2023-05-11 11:59 ` Sergey Organov 0 siblings, 1 reply; 39+ messages in thread From: Felipe Contreras @ 2023-05-11 3:14 UTC (permalink / raw) To: git; +Cc: Sergey Organov, Matthieu Moy A recent discussion about a bug in the diff machinery [1] made me dig the history of the `-s` option, and turned out to be quite an archeological endeavor. The first indication of such flag comes from e2e5e98a40 ([PATCH] Silent flag for show-diff, 2005-04-13), only 54 commits after the initial commit. Not much later after, a `-z` option was added for some machine-readable output in d94c6128e6 ([PATCH] show-diff -z option for machine readable output., 2005-04-16). Linus Torvalds wanted to make the machine-readable output the only one and wrote 0a7668e99f (show-diff: match diff-tree and diff-cache output, 2005-04-26), but Junio Hamano disagreed and added a `-p` option for a human-readable patch in 4a6bf9e18a ([PATCH] Reactivate show-diff patch generation, 2005-04-27). You'll need "diff-tree-helper" to show the full diff, but Junio is dead set on adding a "-p" argument to all three to avoid it. That's next.. Right after that, Junio Hamano deprecated the `-s` flag, since `git-show-diff` didn't show the patch by default, so `-s` became a no-op. I presume at that point in time they didn't think of the possibility of doing `-p -s` together. The first introduction of DIFF_FORMAT_NO_OUTPUT was in a corner case of 6b14d7faf0 ([PATCH] Diffcore updates., 2005-05-22), but later on it was used explicitly to replace a global variable of `git-diff-tree -s` in d0309355c9 ([PATCH] Use DIFF_FORMAT_NO_OUTPUT to implement diff-tree -s option., 2005-05-24). When the equivalent of the modern `git diff` was added, the `-p` option was included by default: 940c1bb018 (Add "git diff" script, 2005-06-13). So later on when it was converted to C, DIFF_FORMAT_PATCH was the default 65056021f2 (built-in diff., 2006-04-28). But not for all commands, for example the default of `git diff-tree` is DIFF_FORMAT_RAW, and it remains the case to this day. So at this point it seems pretty clear that `-s` means `silent`, and whatever the default format of a diff command is (`--patch` or `--raw`), `-s` is meant to silence that format. Later on in c6744349df (Merge with_raw, with_stat and summary variables to output_format, 2006-06-24), the output format changed from an enum to a bit field, so now it was possible to do for example `DIFF_FORMAT_PATCH | DIFF_FORMAT_RAW`. This is when things become complicated, because now what is `-s` supposed to do? Is it supposed to silence only the default format? Or is it supposed to silence all formats? For example, these two commands are equivalent: git diff-tree @~ @ git diff-tree --raw @~ @ That's because the default format of `git diff-tree` is DIFF_FORMAT_RAW. But what happens if we do: git diff-tree -s --patch @~ @ Shall we silence only the RAW part, or the PATCH part as well? And then, should these two be different? git diff-tree --patch -s @~ @ git diff-tree -s --patch @~ @ This is something that wasn't discussed or explored at the time, so it is unclear. And then, finally, we have d09cd15d19 (diff: allow --no-patch as synonym for -s, 2013-07-16), which very clearly says: This follows the usual convention of having a --no-foo option to negate --foo. So, obviously the intention of `--no-patch` is to negate `--patch`, but it is linked to `-s`, which was linked to DIFF_FORMAT_NO_OUTPUT, which means `--no-patch` negates *all* output, not just the output of `--patch`. So what should the output of this command be: git diff-tree --patch --no-patch --raw @~ @ I think it very clearly should output the same as: git diff-tree @~ @ And the ordering does not matter, as this should output the same: git diff-tree --raw --patch --no-patch @~ @ If we can combine two formats, for example: git diff --patch --raw @~ Then we should be able to negate a single format, for example: git diff --patch --raw --no-patch @~ Which in my mind should be different from: git diff --patch --raw --silent @~ So, in short: I don't think `-s` and `--no-patch` are the same thing at all, and it was a mistake to link them together. If anybody thinks the intention behind `-s` and `--no-patch` is obviously clear, I think it would be helpful to explicitly say so for the record. Cheers. [1] https://lore.kernel.org/git/20230503134118.73504-1-sorganov@gmail.com/ -- Felipe Contreras ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-11 3:14 Can we clarify the purpose of `git diff -s`? Felipe Contreras @ 2023-05-11 11:59 ` Sergey Organov 2023-05-11 16:26 ` Junio C Hamano ` (3 more replies) 0 siblings, 4 replies; 39+ messages in thread From: Sergey Organov @ 2023-05-11 11:59 UTC (permalink / raw) To: Felipe Contreras; +Cc: git, Matthieu Moy Felipe Contreras <felipe.contreras@gmail.com> writes: > A recent discussion about a bug in the diff machinery [1] made me dig > the history of the `-s` option, and turned out to be quite an > archeological endeavor. > > The first indication of such flag comes from e2e5e98a40 ([PATCH] Silent > flag for show-diff, 2005-04-13), only 54 commits after the initial > commit. > > Not much later after, a `-z` option was added for some machine-readable > output in d94c6128e6 ([PATCH] show-diff -z option for machine readable > output., 2005-04-16). > > Linus Torvalds wanted to make the machine-readable output the only one > and wrote 0a7668e99f (show-diff: match diff-tree and diff-cache output, > 2005-04-26), but Junio Hamano disagreed and added a `-p` option for > a human-readable patch in 4a6bf9e18a ([PATCH] Reactivate show-diff patch > generation, 2005-04-27). > > You'll need "diff-tree-helper" to show the full diff, but Junio is > dead set on adding a "-p" argument to all three to avoid it. That's > next.. > > Right after that, Junio Hamano deprecated the `-s` flag, since > `git-show-diff` didn't show the patch by default, so `-s` became a > no-op. I presume at that point in time they didn't think of the > possibility of doing `-p -s` together. > > The first introduction of DIFF_FORMAT_NO_OUTPUT was in a corner case of > 6b14d7faf0 ([PATCH] Diffcore updates., 2005-05-22), but later on it was > used explicitly to replace a global variable of `git-diff-tree -s` in > d0309355c9 ([PATCH] Use DIFF_FORMAT_NO_OUTPUT to implement diff-tree -s > option., 2005-05-24). > > When the equivalent of the modern `git diff` was added, the `-p` option > was included by default: 940c1bb018 (Add "git diff" script, 2005-06-13). > So later on when it was converted to C, DIFF_FORMAT_PATCH was the > default 65056021f2 (built-in diff., 2006-04-28). > > But not for all commands, for example the default of `git diff-tree` is > DIFF_FORMAT_RAW, and it remains the case to this day. > > So at this point it seems pretty clear that `-s` means `silent`, and > whatever the default format of a diff command is (`--patch` or `--raw`), > `-s` is meant to silence that format. > > Later on in c6744349df (Merge with_raw, with_stat and summary variables > to output_format, 2006-06-24), the output format changed from an enum to > a bit field, so now it was possible to do for example > `DIFF_FORMAT_PATCH | DIFF_FORMAT_RAW`. > > This is when things become complicated, because now what is `-s` > supposed to do? Is it supposed to silence only the default format? Or is > it supposed to silence all formats? > > For example, these two commands are equivalent: > > git diff-tree @~ @ > git diff-tree --raw @~ @ > > That's because the default format of `git diff-tree` is DIFF_FORMAT_RAW. > > But what happens if we do: > > git diff-tree -s --patch @~ @ > > Shall we silence only the RAW part, or the PATCH part as well? > > And then, should these two be different? > > git diff-tree --patch -s @~ @ > git diff-tree -s --patch @~ @ > > This is something that wasn't discussed or explored at the time, so it > is unclear. > > And then, finally, we have d09cd15d19 (diff: allow --no-patch as synonym > for -s, 2013-07-16), which very clearly says: > > This follows the usual convention of having a --no-foo option to > negate --foo. > > So, obviously the intention of `--no-patch` is to negate `--patch`, but > it is linked to `-s`, which was linked to DIFF_FORMAT_NO_OUTPUT, which > means `--no-patch` negates *all* output, not just the output of > `--patch`. > > So what should the output of this command be: > > git diff-tree --patch --no-patch --raw @~ @ > > I think it very clearly should output the same as: > > git diff-tree @~ @ > > And the ordering does not matter, as this should output the same: > > git diff-tree --raw --patch --no-patch @~ @ > > If we can combine two formats, for example: > > git diff --patch --raw @~ > > Then we should be able to negate a single format, for example: > > git diff --patch --raw --no-patch @~ > > Which in my mind should be different from: > > git diff --patch --raw --silent @~ > > So, in short: I don't think `-s` and `--no-patch` are the same thing at > all, and it was a mistake to link them together. First, thanks a lot of the in-depth investigation of the historical background of the issue! I entirely agree with your conclusion: obviously, -s (--silent) and --no-patch are to be different for UI to be even remotely intuitive, and I'd vote for immediate fix of --no-patch semantics even though it's a backward-incompatible change. --patch should obviously produce something more or less suitable for patching the sources, and then its even move obvious that --no-patch should turn off this kind of output *only*. The exact desirable --silent semantics is questionable though. It could either mean turn off all the --<output_type> flags, i.e. simply a synonym for --no-patch --no-raw..., as it essentially is right now, or it could mean suppress actual output not touching any --<output_type> flags (in which case it probably should have --no-silent counterpart as well.) That said, there is another mystery in the diff interface as well: why do we have --patch-with-raw and --patch-with-stat, yet no --raw-with-stat, --numstat-with-shortstat, --patch-with-numstat, etc.? If, say, --patch turned off everything except usual diff output, then these combinatorical options would make sense, but currently they don't. Finally, there is another intrinsic problem in current Git CI: the "defaults to" part, as in "git show" that defaults to --patch for simple commits, --cc for merge commits, etc., except that it doesn't behave as if these options were explicitly provided at the beginning of the command-line. Instead it behaves in a way that "makes sense" in some definition of "sense", so that git show --raw produces only raw output, whereas git show --patch --raw produces both kinds of output. So, "git show" does not imply --patch, yet it produces corresponding output unless any diff format bit option is explicitly given on the command-line, and it's this behavior that leads to kludges like DIFF_FORMAT_NO_OUTPUT in the implementation. Even though such behavior indeed "makes sense" at the first glance, in fact this is irregularity and it's unfortunate, as it renders wrong simple interpretations like "git show" being just a synonym for "git log -n1 --patch --cc". I'd rather think about generic interface for setting/clearing (multiple) bits through CI than resorting to such convenience tricks. Once that is in place, one will be able to say "I need these bits only", "I need to turn these bit(s) on", and "I need to turn these bit(s) off" conveniently and universally in any part of Git CI where it's needed. Overall, the entire CI seems to be built on assumption that it's a good idea to guess user intentions (hmm, what could the user likely mean when they said "git show --raw"?), whereas I believe that it'd be better if CI forced user to strictly specify their intention according to established basic rules, and then provided convenience shortcuts that adhere to the same basic rules. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-11 11:59 ` Sergey Organov @ 2023-05-11 16:26 ` Junio C Hamano 2023-05-11 17:37 ` Junio C Hamano ` (2 subsequent siblings) 3 siblings, 0 replies; 39+ messages in thread From: Junio C Hamano @ 2023-05-11 16:26 UTC (permalink / raw) To: Sergey Organov; +Cc: Felipe Contreras, git, Matthieu Moy Sergey Organov <sorganov@gmail.com> writes: > That said, there is another mystery in the diff interface as well: why > do we have --patch-with-raw and --patch-with-stat, yet no > --raw-with-stat, --numstat-with-shortstat, --patch-with-numstat, etc.? > If, say, --patch turned off everything except usual diff output, then > these combinatorical options would make sense, but currently they don't. I can explain the mystery but the explanation may or may not justify it, depending on how lenient you are to developers of old days, with less experience than they have today ;-). Back when we started, "--raw" was the only thing there, and then we were to pass the raw-to-patch external filter, when you wanted a patch output. I then updated this arrangement to work in a single process [*] without having to produce textual hexadecimal output (raw) and piping it to another process, and that is how "--patch" mode was created [*]. After I added "--patch", without having foresight of the risk of the combinatorial explosion, "--patch-with-raw" was born. "--stat" and other niceties came much later, and made me finally realize the risk of the combinatorial explosion. "--patch-with-raw" stopped gaining siblings as a consequence. Yes, I was slow and inexperienced, but one has to do with the capability one has at the time. > Finally, there is another intrinsic problem in current Git CI: the > "defaults to" part, as in "git show" that defaults to --patch for simple > commits, --cc for merge commits, etc., except that it doesn't behave as > if these options were explicitly provided at the beginning of the > command-line. Yes, as you said in your message after this part, this is pretty much deliberate and is done for the sake of usability. Essentially the logic is to achieve two distinct goals: * we want to be able to combine the basic things like --patch, --raw, --stat to specify exact format (but some things may not mix well with others). * when the user does not give a specific preference, we want to give a reasonable default. So, when the user wants "--stat", we do not want to combine it with "--patch" that the user may have gotten by default if they did not give "--stat". If we implemented the default logic like the way (I guess) you are suggesting, i.e. * there is a default set of options, "--patch". * the user can add or subtract options with "--[no-]foo" where the value of foo may be patch, stat, etc. it would turn it to "--patch --stat" if the user gave "--stat" on the command line. The user needs to say "--no-patch --stat" if they want to see only the diffstat (which by itself is not necessarily bad; if it is more common to want --patch-with-stat than --stat, it may even be better), but the bad part is that the user need to remember what the built-in default is. [Footnote] * Not exactly a "single process"; until Linus imported xdiff machinery to make the textual patch generation in-process, we forked "diff -u" as a subprocess inside "git diff-*". But the end user no longer had to construct the "git diff-files | raw-to-patch" pipeline themselves. * Eventually it would become the in-process "diffcore transformation pipeline" to compute diff_queue, which is a moral equivalent of the input to "raw-to-patch filter", then transform the result of the previous step (e.g. rename detection) and then format the result of the previous step into various output, but none of that existed back then. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-11 11:59 ` Sergey Organov 2023-05-11 16:26 ` Junio C Hamano @ 2023-05-11 17:37 ` Junio C Hamano 2023-05-11 18:04 ` Sergey Organov 2023-05-11 18:17 ` Felipe Contreras 2023-05-11 17:41 ` Felipe Contreras [not found] ` <5bb24e0208dd4a8ca5f6697d578f3ae0@SAMBXP02.univ-lyon1.fr> 3 siblings, 2 replies; 39+ messages in thread From: Junio C Hamano @ 2023-05-11 17:37 UTC (permalink / raw) To: Sergey Organov; +Cc: Felipe Contreras, git, Matthieu Moy Sergey Organov <sorganov@gmail.com> writes: > I entirely agree with your conclusion: obviously, -s (--silent) and > --no-patch are to be different for UI to be even remotely intuitive, and > I'd vote for immediate fix of --no-patch semantics even though it's a > backward-incompatible change. I forgot to write about this part. tl;dr. While I do not think the current "--no-patch" that turns off things other than "--patch" is intuitive, an "immediate" change is not possible. Let's do one fix at a time. The behaviour came in the v1.8.4 days with a series that was merged by e2ecd252 (Merge branch 'mm/diff-no-patch-synonym-to-s', 2013-07-22), which * made "--no-patch" a synonym for "-s"; * fixed "-s --patch", in which the effect of "-s" got stuck and did not allow the patch output to be re-enabled again with "--patch"; * updated documentation to explain "--no-patch" as a synonym for "-s". While it is very clear that the intent of the author was to make it a synonym for "-s" and not a "feature-wise enable/disable" option, that is what we've run with for the past 10 years. You identified bugs related the "-s got stuck" problem and we recently fixed that. "Should --no-patch be changed" can be treated as a separate issue, and whenever we can treat two things separately, I want to do so, to keep the potential blast radius smaller. That way, if an earlier change turns out OK but the other change causes severe regression, we can only revert or rework the latter. An exception is if committing to one change (e.g. "fix '-s'") makes the other change (e.g. "redefine --no-patch") impossible, but we all know it is not the case here. I gave an outline of how to go about it in the log message of that "fix '-s'" patch. I do not think it will break established use cases too badly to fix the behaviour of "-s" so that it does not get stuck. We saw an existing breakage in one test, but asking the owners of scripts that make the same mistake of assuming "-s" gets stuck for some but not other options to fix that assumption based on an earlier faulty implementation is much easier. But "git diff --stat --patch --no-patch", which suddenly starts showing diffstat after you make "--no-patch" no longer a synonym for "-s", has a much larger potential to break the existing workflows. And I do not think asking the users who followed the documented "--no-patch is a synonym for -s" to change their script because we decided to make "--no-patch" no longer a synonym is much harder. So, no, I do not think we can immediately "fix". I do not think anybody knows if it can be done "immediately" or needs a careful planning to transition, and I offhand do not know if it is even possible to transition without fallout. THanks. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-11 17:37 ` Junio C Hamano @ 2023-05-11 18:04 ` Sergey Organov 2023-05-11 18:27 ` Junio C Hamano 2023-05-11 18:36 ` Felipe Contreras 2023-05-11 18:17 ` Felipe Contreras 1 sibling, 2 replies; 39+ messages in thread From: Sergey Organov @ 2023-05-11 18:04 UTC (permalink / raw) To: Junio C Hamano; +Cc: Felipe Contreras, git, Matthieu Moy Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> I entirely agree with your conclusion: obviously, -s (--silent) and >> --no-patch are to be different for UI to be even remotely intuitive, and >> I'd vote for immediate fix of --no-patch semantics even though it's a >> backward-incompatible change. > > I forgot to write about this part. > > tl;dr. While I do not think the current "--no-patch" that turns off > things other than "--patch" is intuitive, an "immediate" change is > not possible. Let's do one fix at a time. > > The behaviour came in the v1.8.4 days with a series that was merged > by e2ecd252 (Merge branch 'mm/diff-no-patch-synonym-to-s', > 2013-07-22), which > > * made "--no-patch" a synonym for "-s"; > > * fixed "-s --patch", in which the effect of "-s" got stuck and did > not allow the patch output to be re-enabled again with "--patch"; > > * updated documentation to explain "--no-patch" as a synonym for > "-s". > > While it is very clear that the intent of the author was to make it > a synonym for "-s" and not a "feature-wise enable/disable" option, > that is what we've run with for the past 10 years. You identified > bugs related the "-s got stuck" problem and we recently fixed that. I wonder, why this intention of the author has not been opposed at that time is beyond my understanding, sorry! What exactly did it bring to make --no-patch a synonym for -s? Not only it's illogical, it's even not useful as being more lengthy. Probably nobody actually cared at that time, me thinks. > > "Should --no-patch be changed" can be treated as a separate issue, > and whenever we can treat two things separately, I want to do so, to > keep the potential blast radius smaller. Sure it's a separate change. When I said "immediate" I meant that there is no need for some transition measures like config variables, not that it is to be included in the "fix -s". > That way, if an earlier change turns out OK but the other change > causes severe regression, we can only revert or rework the latter. An > exception is if committing to one change (e.g. "fix '-s'") makes the > other change (e.g. "redefine --no-patch") impossible, but we all know > it is not the case here. I gave an outline of how to go about it in > the log message of that "fix '-s'" patch. > > I do not think it will break established use cases too badly to fix > the behaviour of "-s" so that it does not get stuck. We saw an > existing breakage in one test, but asking the owners of scripts that > make the same mistake of assuming "-s" gets stuck for some but not > other options to fix that assumption based on an earlier faulty > implementation is much easier. > > But "git diff --stat --patch --no-patch", which suddenly starts > showing diffstat after you make "--no-patch" no longer a synonym for > "-s", has a much larger potential to break the existing workflows. > And I do not think asking the users who followed the documented > "--no-patch is a synonym for -s" to change their script because we > decided to make "--no-patch" no longer a synonym is much harder. Why somebody would use --no-patch instead of -s when she means -s? Is't it obvious that git diff --stat --patch -s is not only shorter but dramatically more clear than git diff --stat --patch --no-patch ??? Taking this into account, I'd predict no breakage at all. > So, no, I do not think we can immediately "fix". I do not think > anybody knows if it can be done "immediately" or needs a careful > planning to transition, and I offhand do not know if it is even > possible to transition without fallout. This has been expected, and this is one of the things that stops me from trying to "fix" anything in the Git UI recently. I think that perfectly legit carefulness from the maintainer to be conservative in accepting of such changes goes a bit too far, sorry! Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-11 18:04 ` Sergey Organov @ 2023-05-11 18:27 ` Junio C Hamano 2023-05-11 18:36 ` Felipe Contreras 1 sibling, 0 replies; 39+ messages in thread From: Junio C Hamano @ 2023-05-11 18:27 UTC (permalink / raw) To: Sergey Organov; +Cc: Felipe Contreras, git, Matthieu Moy Sergey Organov <sorganov@gmail.com> writes: > I wonder, why this intention of the author has not been opposed at that > time is beyond my understanding, sorry! What exactly did it bring to > make --no-patch a synonym for -s? Not only it's illogical, it's even not > useful as being more lengthy. Probably because we wanted to have a more descriptive synonym to make it discoverable [*]. The release notes for v1.8.4 (where "--no-patch" was added as a synonym for "-s") tells us this much. * "git show -s" was less discoverable than it should have been. It now has a natural synonym "git show --no-patch". In hindsight, "--silent" or "--squelch" might also have been viable choices, but if you really care, you have to ask Matthieu why "--no-patch" was chosen. If I have to guess, it came from the desire to phrase it as "--no-<something>", which some commands had started to adopt as a convention to defeat "<something>" between v1.7.0 and v1.8.4 (the latter is where "--no-patch" appeared), and "--no-everything" is awkward to be used with "git show" as we still want to see the commit message. > Why somebody would use --no-patch instead of -s when she means -s? Because "-s" does not have a longhand and not easily discoverable? As I explained above, I thing that was the original motivation behind wanting to have _some_ synonym. [Footnote] * In fact, I was surprised that somebody (I forgot who they were), who I have known to be a user of git for 10+ years, wrote "--no-patch" in an example in one of their recent messages, outside the context of, and I strongly suspect that it happened before, this "-s" vs "--no-patch" discussion. I took it to be showing their preference of "--no-patch" over "-s", or their not knowing about "-s" even they are a long-time Git user. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-11 18:04 ` Sergey Organov 2023-05-11 18:27 ` Junio C Hamano @ 2023-05-11 18:36 ` Felipe Contreras 1 sibling, 0 replies; 39+ messages in thread From: Felipe Contreras @ 2023-05-11 18:36 UTC (permalink / raw) To: Sergey Organov, Junio C Hamano; +Cc: Felipe Contreras, git, Matthieu Moy Sergey Organov wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Sergey Organov <sorganov@gmail.com> writes: > > > >> I entirely agree with your conclusion: obviously, -s (--silent) and > >> --no-patch are to be different for UI to be even remotely intuitive, and > >> I'd vote for immediate fix of --no-patch semantics even though it's a > >> backward-incompatible change. > > > > I forgot to write about this part. > > > > tl;dr. While I do not think the current "--no-patch" that turns off > > things other than "--patch" is intuitive, an "immediate" change is > > not possible. Let's do one fix at a time. > > > > The behaviour came in the v1.8.4 days with a series that was merged > > by e2ecd252 (Merge branch 'mm/diff-no-patch-synonym-to-s', > > 2013-07-22), which > > > > * made "--no-patch" a synonym for "-s"; > > > > * fixed "-s --patch", in which the effect of "-s" got stuck and did > > not allow the patch output to be re-enabled again with "--patch"; > > > > * updated documentation to explain "--no-patch" as a synonym for > > "-s". > > > > While it is very clear that the intent of the author was to make it > > a synonym for "-s" and not a "feature-wise enable/disable" option, > > that is what we've run with for the past 10 years. You identified > > bugs related the "-s got stuck" problem and we recently fixed that. > > I wonder, why this intention of the author has not been opposed at that > time is beyond my understanding, sorry! What exactly did it bring to > make --no-patch a synonym for -s? Not only it's illogical, it's even not > useful as being more lengthy. That's because it's not true. The intention wasn't to make `--no-patch` a synonym, the intention was to make disabling the patch output of `git diff` more accessible. The cover letter makes it clear [1]. "git show" will show the diff by default. For merge commits, it shows the --cc diff which is often empty, hence the behavior you see. You want to use "git show -s", which suppresses the patch output, and this "git show -s" is extraordinarily hard to discover, as it is only documented in "git log --help". Google has been my friend here, but we should really improve that. The code at the time did not allow turning off only one output, using DIFF_FORMAT_NO_OUTPUT was easy, so that's what they did. > > But "git diff --stat --patch --no-patch", which suddenly starts > > showing diffstat after you make "--no-patch" no longer a synonym for > > "-s", has a much larger potential to break the existing workflows. > > And I do not think asking the users who followed the documented > > "--no-patch is a synonym for -s" to change their script because we > > decided to make "--no-patch" no longer a synonym is much harder. > > Why somebody would use --no-patch instead of -s when she means -s? Is't > it obvious that > > git diff --stat --patch -s > > is not only shorter but dramatically more clear than > > git diff --stat --patch --no-patch > > ??? > > Taking this into account, I'd predict no breakage at all. I agree. I bet there's very few people relying on this behavior (if any), which is precisely why nobody noticed the bug until now. If we are going to break backwards-compatibility, we should break it correctly: split `--no-patch` from `-s`, and make it only negate `--patch`. > > So, no, I do not think we can immediately "fix". I do not think > > anybody knows if it can be done "immediately" or needs a careful > > planning to transition, and I offhand do not know if it is even > > possible to transition without fallout. > > This has been expected, and this is one of the things that stops me from > trying to "fix" anything in the Git UI recently. I think that perfectly > legit carefulness from the maintainer to be conservative in accepting of > such changes goes a bit too far, sorry! And you were correct. I did write a patch series that actually fixes the problem correctly, and the maintainer completely ignored it in favor of his partial solution. So you would have wasted your time trying to fix this correctly, as I probably did. Cheers. [1] https://lore.kernel.org/git/1373893639-13413-1-git-send-email-Matthieu.Moy@imag.fr/ -- Felipe Contreras ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-11 17:37 ` Junio C Hamano 2023-05-11 18:04 ` Sergey Organov @ 2023-05-11 18:17 ` Felipe Contreras 1 sibling, 0 replies; 39+ messages in thread From: Felipe Contreras @ 2023-05-11 18:17 UTC (permalink / raw) To: Junio C Hamano, Sergey Organov; +Cc: Felipe Contreras, git, Matthieu Moy Junio C Hamano wrote: > Sergey Organov <sorganov@gmail.com> writes: > > > I entirely agree with your conclusion: obviously, -s (--silent) and > > --no-patch are to be different for UI to be even remotely intuitive, and > > I'd vote for immediate fix of --no-patch semantics even though it's a > > backward-incompatible change. > While it is very clear that the intent of the author was to make it > a synonym for "-s" and not a "feature-wise enable/disable" option, I disgree, it's very clear the intention was to negate --patch, he explicitely said so multiple times: * This follows the usual convention of having a --no-foo option to negate --foo. * to cancel the effect of `--patch`. In particular, the purpose was to make silencing the output of `git diff` more accessible, the cover letter makes it abundantly clear [1]: ==== > Stefan Beller <stefanbeller@googlemail.com> writes: > >> However I sometimes also get: >> sb@sb:~/OSS/git$ git show --format="%ad" 0da7a53 >> Fri Jul 12 10:49:34 2013 -0700 >> >> diff --git a/Documentation/RelNotes/1.8.4.txt >> b/Documentation/RelNotes/1.8.4.txt >> index 0e50df8..4250e5a 100644 >> --- a/Documentation/RelNotes/1.8.4.txt >> +++ b/Documentation/RelNotes/1.8.4.txt > > "git show" will show the diff by default. For merge commits, it shows > the --cc diff which is often empty, hence the behavior you see. > > You want to use "git show -s", which suppresses the patch output. ... and this "git show -s" is extraordinarily hard to discover, as it is only documented in "git log --help". Google has been my friend here, but we should really improve that. This patch series does essentially two things: * Add a --no-patch synonym for -s. I'm actually wondering why the option wasn't called this way from the beginning. * Reorganize the doc so that "git show" actually mentions it. ==== Making it a synonym of `-s` was a means, not an end. > I do not think it will break established use cases too badly to fix > the behaviour of "-s" so that it does not get stuck. We saw an > existing breakage in one test, It's curious that your patch breaks one test case, while my approach breaks *zero* cases. > but asking the owners of scripts that make the same mistake of > assuming "-s" gets stuck for some but not other options to fix that > assumption based on an earlier faulty implementation is much easier. "The users are using the interface wrong" is an euphemism for "I want to break backwards-compatibility". According to Linus Torvalds if your users are relying on a bug in your interface, that bug is now a feature. If we want to break backwards-compatibility then let's do so. > So, no, I do not think we can immediately "fix". I do not think > anybody knows if it can be done "immediately" or needs a careful > planning to transition, and I offhand do not know if it is even > possible to transition without fallout. I know it can be done immediately, because my patch series already did it. [1] https://lore.kernel.org/git/1373893639-13413-1-git-send-email-Matthieu.Moy@imag.fr/ -- Felipe Contreras ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-11 11:59 ` Sergey Organov 2023-05-11 16:26 ` Junio C Hamano 2023-05-11 17:37 ` Junio C Hamano @ 2023-05-11 17:41 ` Felipe Contreras 2023-05-11 18:31 ` Sergey Organov [not found] ` <5bb24e0208dd4a8ca5f6697d578f3ae0@SAMBXP02.univ-lyon1.fr> 3 siblings, 1 reply; 39+ messages in thread From: Felipe Contreras @ 2023-05-11 17:41 UTC (permalink / raw) To: Sergey Organov, Felipe Contreras; +Cc: git, Matthieu Moy Sergey Organov wrote: > I'd rather think about generic interface for setting/clearing (multiple) > bits through CI than resorting to such convenience tricks. Once that is > in place, one will be able to say "I need these bits only", "I need to > turn these bit(s) on", and "I need to turn these bit(s) off" > conveniently and universally in any part of Git CI where it's needed. It's possible to achieve both. Imagine your ideal explicit interface. In that interface the default is no output, so you *have* to specify all the bits, for example: git show --patch Or: git show --raw In this ideal interface it's clear what the user wants to do, because it's explicit. git show --patch --raw --no-patch Agreed? My proposal achieves your ideal explicit interface, except when no format is specified (e.g. `git show`), a default format is chosen for the user, but that's *only* if the user hasn't specified any format. If you explicitely specify the output format that you want, then the default is irrelevant to you, thus you have your ideal explicit interface. This is the best of both worlds. Cheers. -- Felipe Contreras ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-11 17:41 ` Felipe Contreras @ 2023-05-11 18:31 ` Sergey Organov 2023-05-11 19:10 ` Felipe Contreras 0 siblings, 1 reply; 39+ messages in thread From: Sergey Organov @ 2023-05-11 18:31 UTC (permalink / raw) To: Felipe Contreras; +Cc: git, Matthieu Moy Felipe Contreras <felipe.contreras@gmail.com> writes: > Sergey Organov wrote: > >> I'd rather think about generic interface for setting/clearing >> (multiple) bits through CI than resorting to such convenience >> tricks. Once that is in place, one will be able to say "I need these >> bits only", "I need to turn these bit(s) on", and "I need to turn >> these bit(s) off" conveniently and universally in any part of Git CI >> where it's needed. > > It's possible to achieve both. > > Imagine your ideal explicit interface. In that interface the default > is no output, so you *have* to specify all the bits, for example: > > git show --patch No, that's not what I meant. There is no point in making "git show" to have no output by default, please see below. > > Or: > > git show --raw > > In this ideal interface it's clear what the user wants to do, because > it's explicit. > > git show --patch --raw --no-patch > > Agreed? > > My proposal achieves your ideal explicit interface, except when no > format is specified (e.g. `git show`), a default format is chosen for > the user, but that's *only* if the user hasn't specified any format. My point is that the default format should be selected as if it has been provided by existing options, rather than by some magic hidden in the code. > > If you explicitely specify the output format that you want, then the > default is irrelevant to you, thus you have your ideal explicit > interface. That's not what I had in mind, sorry. It'd rather be something like: --raw: set "raw" bit and clear all the rest --+raw set "raw" bit (== current --raw) ---raw clear "raw" bit (== --no-raw) In this model git show would be just an alias for git log -n1 --patch --cc and no support for a separate command would be need in the first place. git show --raw would then produce expected output that makes sense due to the common option processing rules, not because somebody had implemented some arbitrary "defaults" for the command. Then one can even think about supporting some niceties like git log --patch+raw In other words, if we provide full orthogonal access to (the diff output) bits using CL options, no special code wold be ever needed to implement some niceties and shortcuts. Disclaimer: please keep this with a grain of salt as this is not a carefully thought-through proposal. Not yet anyway. Just me thinking aloud. For *nix guys, the closest analogy to the above is probably: chmod og-wx Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-11 18:31 ` Sergey Organov @ 2023-05-11 19:10 ` Felipe Contreras 2023-05-11 19:32 ` Sergey Organov 0 siblings, 1 reply; 39+ messages in thread From: Felipe Contreras @ 2023-05-11 19:10 UTC (permalink / raw) To: Sergey Organov, Felipe Contreras; +Cc: git, Matthieu Moy Sergey Organov wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > Sergey Organov wrote: > > > >> I'd rather think about generic interface for setting/clearing > >> (multiple) bits through CI than resorting to such convenience > >> tricks. Once that is in place, one will be able to say "I need these > >> bits only", "I need to turn these bit(s) on", and "I need to turn > >> these bit(s) off" conveniently and universally in any part of Git CI > >> where it's needed. > > > > It's possible to achieve both. > > > > Imagine your ideal explicit interface. In that interface the default > > is no output, so you *have* to specify all the bits, for example: > > > > git show --patch > > No, that's not what I meant. There is no point in making "git show" to > have no output by default, please see below. > > > > > Or: > > > > git show --raw > > > > In this ideal interface it's clear what the user wants to do, because > > it's explicit. > > > > git show --patch --raw --no-patch > > > > Agreed? > > > > My proposal achieves your ideal explicit interface, except when no > > format is specified (e.g. `git show`), a default format is chosen for > > the user, but that's *only* if the user hasn't specified any format. > > My point is that the default format should be selected as if it has been > provided by existing options, rather than by some magic hidden in the > code. But why? I don't see any benefit, only drawbacks. > > If you explicitely specify the output format that you want, then the > > default is irrelevant to you, thus you have your ideal explicit > > interface. > > That's not what I had in mind, sorry. It'd rather be something like: > > --raw: set "raw" bit and clear all the rest > --+raw set "raw" bit (== current --raw) > ---raw clear "raw" bit (== --no-raw) > > In this model > > git show > > would be just an alias for > > git log -n1 --patch --cc > > and no support for a separate command would be need in the first place. > > git show --raw > > would then produce expected output that makes sense due to the common > option processing rules, not because somebody had implemented some > arbitrary "defaults" for the command. But now you are at the mercy of those "arbitrary defaults". Let's say those defaults change, and now the default output of `git show` is `--stat`. Now to generate the same output you have to do: git show --raw in one version of git, and: git show --no-stat --patch --raw in another. This forces the user to know what is the default of every command. Why force this mental burden? If I want both patch and raw, then why not explicitely say so: git show --patch --raw And forget about the current defaults for that command. -- Felipe Contreras ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-11 19:10 ` Felipe Contreras @ 2023-05-11 19:32 ` Sergey Organov 2023-05-11 19:54 ` Felipe Contreras 0 siblings, 1 reply; 39+ messages in thread From: Sergey Organov @ 2023-05-11 19:32 UTC (permalink / raw) To: Felipe Contreras; +Cc: git, Matthieu Moy Felipe Contreras <felipe.contreras@gmail.com> writes: > Sergey Organov wrote: >> Felipe Contreras <felipe.contreras@gmail.com> writes: >> > Sergey Organov wrote: >> > >> >> I'd rather think about generic interface for setting/clearing >> >> (multiple) bits through CI than resorting to such convenience >> >> tricks. Once that is in place, one will be able to say "I need these >> >> bits only", "I need to turn these bit(s) on", and "I need to turn >> >> these bit(s) off" conveniently and universally in any part of Git CI >> >> where it's needed. >> > >> > It's possible to achieve both. >> > >> > Imagine your ideal explicit interface. In that interface the default >> > is no output, so you *have* to specify all the bits, for example: >> > >> > git show --patch >> >> No, that's not what I meant. There is no point in making "git show" to >> have no output by default, please see below. >> >> > >> > Or: >> > >> > git show --raw >> > >> > In this ideal interface it's clear what the user wants to do, because >> > it's explicit. >> > >> > git show --patch --raw --no-patch >> > >> > Agreed? >> > >> > My proposal achieves your ideal explicit interface, except when no >> > format is specified (e.g. `git show`), a default format is chosen for >> > the user, but that's *only* if the user hasn't specified any format. >> >> My point is that the default format should be selected as if it has been >> provided by existing options, rather than by some magic hidden in the >> code. > > But why? > > I don't see any benefit, only drawbacks. > >> > If you explicitely specify the output format that you want, then the >> > default is irrelevant to you, thus you have your ideal explicit >> > interface. >> >> That's not what I had in mind, sorry. It'd rather be something like: >> >> --raw: set "raw" bit and clear all the rest >> --+raw set "raw" bit (== current --raw) >> ---raw clear "raw" bit (== --no-raw) >> >> In this model >> >> git show >> >> would be just an alias for >> >> git log -n1 --patch --cc >> >> and no support for a separate command would be need in the first place. >> >> git show --raw >> >> would then produce expected output that makes sense due to the common >> option processing rules, not because somebody had implemented some >> arbitrary "defaults" for the command. > > But now you are at the mercy of those "arbitrary defaults". No, see below. > > Let's say those defaults change, and now the default output of `git show` is > `--stat`. > > Now to generate the same output you have to do: > > git show --raw > > in one version of git, and: > > git show --no-stat --patch --raw > > in another. No: --raw in my model clears all the flags but --raw, so git show --raw will produce exactly the same result: raw output only. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-11 19:32 ` Sergey Organov @ 2023-05-11 19:54 ` Felipe Contreras 2023-05-11 20:24 ` Sergey Organov 0 siblings, 1 reply; 39+ messages in thread From: Felipe Contreras @ 2023-05-11 19:54 UTC (permalink / raw) To: Sergey Organov, Felipe Contreras; +Cc: git, Matthieu Moy Sergey Organov wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > Sergey Organov wrote: > >> Felipe Contreras <felipe.contreras@gmail.com> writes: > >> > Sergey Organov wrote: > >> > > >> >> I'd rather think about generic interface for setting/clearing > >> >> (multiple) bits through CI than resorting to such convenience > >> >> tricks. Once that is in place, one will be able to say "I need these > >> >> bits only", "I need to turn these bit(s) on", and "I need to turn > >> >> these bit(s) off" conveniently and universally in any part of Git CI > >> >> where it's needed. > >> > > >> > It's possible to achieve both. > >> > > >> > Imagine your ideal explicit interface. In that interface the default > >> > is no output, so you *have* to specify all the bits, for example: > >> > > >> > git show --patch > >> > >> No, that's not what I meant. There is no point in making "git show" to > >> have no output by default, please see below. > >> > >> > > >> > Or: > >> > > >> > git show --raw > >> > > >> > In this ideal interface it's clear what the user wants to do, because > >> > it's explicit. > >> > > >> > git show --patch --raw --no-patch > >> > > >> > Agreed? > >> > > >> > My proposal achieves your ideal explicit interface, except when no > >> > format is specified (e.g. `git show`), a default format is chosen for > >> > the user, but that's *only* if the user hasn't specified any format. > >> > >> My point is that the default format should be selected as if it has been > >> provided by existing options, rather than by some magic hidden in the > >> code. > > > > But why? > > > > I don't see any benefit, only drawbacks. > > > >> > If you explicitely specify the output format that you want, then the > >> > default is irrelevant to you, thus you have your ideal explicit > >> > interface. > >> > >> That's not what I had in mind, sorry. It'd rather be something like: > >> > >> --raw: set "raw" bit and clear all the rest > >> --+raw set "raw" bit (== current --raw) > >> ---raw clear "raw" bit (== --no-raw) > >> > >> In this model > >> > >> git show > >> > >> would be just an alias for > >> > >> git log -n1 --patch --cc > >> > >> and no support for a separate command would be need in the first place. > >> > >> git show --raw > >> > >> would then produce expected output that makes sense due to the common > >> option processing rules, not because somebody had implemented some > >> arbitrary "defaults" for the command. > > > > But now you are at the mercy of those "arbitrary defaults". > > No, see below. > > > > > Let's say those defaults change, and now the default output of `git show` is > > `--stat`. > > > > Now to generate the same output you have to do: > > > > git show --raw > > > > in one version of git, and: > > > > git show --no-stat --patch --raw > > > > in another. > > No: --raw in my model clears all the flags but --raw, so > > git show --raw > > will produce exactly the same result: raw output only. But that {--,--+,---} notion doesn't exist, and I think it's safe to say it will never exist. So, could we limit or solution-space to those solutions that could have the potential to be merged? What you suggest could be easily achieved with: git show --silent --raw But because no other format is explicitely specified, following my notion of defaults, that's the same as: git show --raw `---raw` can easily be achieved with `--no-raw`. The only thing that's missing is `--+raw`, but I don't see how this: git show --+raw Is more valuable than: git show --patch --raw If you know the default of `git show` is `--patch`, and you want to add `--raw`, then you can easily write the latter. Doesn't this approach achieve everything you want to do? Albeit with a different syntax. -- Felipe Contreras ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-11 19:54 ` Felipe Contreras @ 2023-05-11 20:24 ` Sergey Organov 2023-05-11 20:59 ` Felipe Contreras 0 siblings, 1 reply; 39+ messages in thread From: Sergey Organov @ 2023-05-11 20:24 UTC (permalink / raw) To: Felipe Contreras; +Cc: git, Matthieu Moy Felipe Contreras <felipe.contreras@gmail.com> writes: > Sergey Organov wrote: >> Felipe Contreras <felipe.contreras@gmail.com> writes: >> >> > Sergey Organov wrote: >> >> Felipe Contreras <felipe.contreras@gmail.com> writes: >> >> > Sergey Organov wrote: >> >> > >> >> >> I'd rather think about generic interface for setting/clearing >> >> >> (multiple) bits through CI than resorting to such convenience >> >> >> tricks. Once that is in place, one will be able to say "I need these >> >> >> bits only", "I need to turn these bit(s) on", and "I need to turn >> >> >> these bit(s) off" conveniently and universally in any part of Git CI >> >> >> where it's needed. >> >> > >> >> > It's possible to achieve both. >> >> > >> >> > Imagine your ideal explicit interface. In that interface the default >> >> > is no output, so you *have* to specify all the bits, for example: >> >> > >> >> > git show --patch >> >> >> >> No, that's not what I meant. There is no point in making "git show" to >> >> have no output by default, please see below. >> >> >> >> > >> >> > Or: >> >> > >> >> > git show --raw >> >> > >> >> > In this ideal interface it's clear what the user wants to do, because >> >> > it's explicit. >> >> > >> >> > git show --patch --raw --no-patch >> >> > >> >> > Agreed? >> >> > >> >> > My proposal achieves your ideal explicit interface, except when no >> >> > format is specified (e.g. `git show`), a default format is chosen for >> >> > the user, but that's *only* if the user hasn't specified any format. >> >> >> >> My point is that the default format should be selected as if it has been >> >> provided by existing options, rather than by some magic hidden in the >> >> code. >> > >> > But why? >> > >> > I don't see any benefit, only drawbacks. >> > >> >> > If you explicitely specify the output format that you want, then the >> >> > default is irrelevant to you, thus you have your ideal explicit >> >> > interface. >> >> >> >> That's not what I had in mind, sorry. It'd rather be something like: >> >> >> >> --raw: set "raw" bit and clear all the rest >> >> --+raw set "raw" bit (== current --raw) >> >> ---raw clear "raw" bit (== --no-raw) >> >> >> >> In this model >> >> >> >> git show >> >> >> >> would be just an alias for >> >> >> >> git log -n1 --patch --cc >> >> >> >> and no support for a separate command would be need in the first place. >> >> >> >> git show --raw >> >> >> >> would then produce expected output that makes sense due to the common >> >> option processing rules, not because somebody had implemented some >> >> arbitrary "defaults" for the command. >> > >> > But now you are at the mercy of those "arbitrary defaults". >> >> No, see below. >> >> > >> > Let's say those defaults change, and now the default output of `git show` is >> > `--stat`. >> > >> > Now to generate the same output you have to do: >> > >> > git show --raw >> > >> > in one version of git, and: >> > >> > git show --no-stat --patch --raw >> > >> > in another. >> >> No: --raw in my model clears all the flags but --raw, so >> >> git show --raw >> >> will produce exactly the same result: raw output only. > > But that {--,--+,---} notion doesn't exist, and I think it's safe to say it > will never exist. So, could we limit or solution-space to those solutions that > could have the potential to be merged? I didn't expect it to exist any time soon, just showed a different way of options design. > > What you suggest could be easily achieved with: > > git show --silent --raw > > But because no other format is explicitely specified, following my notion of > defaults, that's the same as: The problem that I tried to fight is this notion of defaults that is somewhat special, so, if I allow for it, the rest of my suggestions becomes pointless, and without the "defaults" with non-trivial behavior[*] git show --raw won't work as expected provided --raw still just sets "raw" bit and doesn't clear all the rest. What I described was not meant as an immediate proposal to quickly fix current CI. Please don't try to get that as such. [*] Defaults with trivial behavior is just initializing of internal variable holding flags with specific value, that is exactly the same as putting corresponding option(s) at the beginning. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-11 20:24 ` Sergey Organov @ 2023-05-11 20:59 ` Felipe Contreras 2023-05-11 22:49 ` Sergey Organov 0 siblings, 1 reply; 39+ messages in thread From: Felipe Contreras @ 2023-05-11 20:59 UTC (permalink / raw) To: Sergey Organov, Felipe Contreras; +Cc: git, Matthieu Moy Sergey Organov wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > Sergey Organov wrote: > >> Felipe Contreras <felipe.contreras@gmail.com> writes: > >> > >> > Sergey Organov wrote: > >> >> Felipe Contreras <felipe.contreras@gmail.com> writes: > >> >> > Sergey Organov wrote: > >> >> > > >> >> >> I'd rather think about generic interface for setting/clearing > >> >> >> (multiple) bits through CI than resorting to such convenience > >> >> >> tricks. Once that is in place, one will be able to say "I need these > >> >> >> bits only", "I need to turn these bit(s) on", and "I need to turn > >> >> >> these bit(s) off" conveniently and universally in any part of Git CI > >> >> >> where it's needed. > >> >> > > >> >> > It's possible to achieve both. > >> >> > > >> >> > Imagine your ideal explicit interface. In that interface the default > >> >> > is no output, so you *have* to specify all the bits, for example: > >> >> > > >> >> > git show --patch > >> >> > >> >> No, that's not what I meant. There is no point in making "git show" to > >> >> have no output by default, please see below. > >> >> > >> >> > > >> >> > Or: > >> >> > > >> >> > git show --raw > >> >> > > >> >> > In this ideal interface it's clear what the user wants to do, because > >> >> > it's explicit. > >> >> > > >> >> > git show --patch --raw --no-patch > >> >> > > >> >> > Agreed? > >> >> > > >> >> > My proposal achieves your ideal explicit interface, except when no > >> >> > format is specified (e.g. `git show`), a default format is chosen for > >> >> > the user, but that's *only* if the user hasn't specified any format. > >> >> > >> >> My point is that the default format should be selected as if it has been > >> >> provided by existing options, rather than by some magic hidden in the > >> >> code. > >> > > >> > But why? > >> > > >> > I don't see any benefit, only drawbacks. > >> > > >> >> > If you explicitely specify the output format that you want, then the > >> >> > default is irrelevant to you, thus you have your ideal explicit > >> >> > interface. > >> >> > >> >> That's not what I had in mind, sorry. It'd rather be something like: > >> >> > >> >> --raw: set "raw" bit and clear all the rest > >> >> --+raw set "raw" bit (== current --raw) > >> >> ---raw clear "raw" bit (== --no-raw) > >> >> > >> >> In this model > >> >> > >> >> git show > >> >> > >> >> would be just an alias for > >> >> > >> >> git log -n1 --patch --cc > >> >> > >> >> and no support for a separate command would be need in the first place. > >> >> > >> >> git show --raw > >> >> > >> >> would then produce expected output that makes sense due to the common > >> >> option processing rules, not because somebody had implemented some > >> >> arbitrary "defaults" for the command. > >> > > >> > But now you are at the mercy of those "arbitrary defaults". > >> > >> No, see below. > >> > >> > > >> > Let's say those defaults change, and now the default output of `git show` is > >> > `--stat`. > >> > > >> > Now to generate the same output you have to do: > >> > > >> > git show --raw > >> > > >> > in one version of git, and: > >> > > >> > git show --no-stat --patch --raw > >> > > >> > in another. > >> > >> No: --raw in my model clears all the flags but --raw, so > >> > >> git show --raw > >> > >> will produce exactly the same result: raw output only. > > > > But that {--,--+,---} notion doesn't exist, and I think it's safe to say it > > will never exist. So, could we limit or solution-space to those solutions that > > could have the potential to be merged? > > I didn't expect it to exist any time soon, just showed a different way > of options design. > > > > > What you suggest could be easily achieved with: > > > > git show --silent --raw > > > > But because no other format is explicitely specified, following my notion of > > defaults, that's the same as: > > The problem that I tried to fight is this notion of defaults that is > somewhat special, so, if I allow for it, the rest of my suggestions > becomes pointless, No, they don't, all you need to do is specify the default explicitely. > and without the "defaults" with non-trivial behavior[*] > > git show --raw > > won't work as expected provided --raw still just sets "raw" bit and > doesn't clear all the rest. It works perfectly fine. There are no bits to clear, because there are no bits set. That's the whole point of defaults: you don't have to use them. If you don't like the notion of defaults, then don't use them. If you specify *any* format option, then the defaults are ignored and no bits are set other than the ones that you explicitly specified. > [*] Defaults with trivial behavior is just initializing of internal > variable holding flags with specific value, that is exactly the same as > putting corresponding option(s) at the beginning. Those are not default arguments, those are initial arguments. In many cases they behave the same, but not all. -- Felipe Contreras ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-11 20:59 ` Felipe Contreras @ 2023-05-11 22:49 ` Sergey Organov 2023-05-11 23:28 ` Felipe Contreras 0 siblings, 1 reply; 39+ messages in thread From: Sergey Organov @ 2023-05-11 22:49 UTC (permalink / raw) To: Felipe Contreras; +Cc: git, Matthieu Moy Felipe Contreras <felipe.contreras@gmail.com> writes: > Sergey Organov wrote: >> Felipe Contreras <felipe.contreras@gmail.com> writes: >> >> > Sergey Organov wrote: >> >> Felipe Contreras <felipe.contreras@gmail.com> writes: >> >> >> >> > Sergey Organov wrote: >> >> >> Felipe Contreras <felipe.contreras@gmail.com> writes: >> >> >> > Sergey Organov wrote: >> >> >> > >> >> >> >> I'd rather think about generic interface for setting/clearing >> >> >> >> (multiple) bits through CI than resorting to such convenience >> >> >> >> tricks. Once that is in place, one will be able to say "I need these >> >> >> >> bits only", "I need to turn these bit(s) on", and "I need to turn >> >> >> >> these bit(s) off" conveniently and universally in any part of Git CI >> >> >> >> where it's needed. >> >> >> > >> >> >> > It's possible to achieve both. >> >> >> > >> >> >> > Imagine your ideal explicit interface. In that interface the default >> >> >> > is no output, so you *have* to specify all the bits, for example: >> >> >> > >> >> >> > git show --patch >> >> >> >> >> >> No, that's not what I meant. There is no point in making "git show" to >> >> >> have no output by default, please see below. >> >> >> >> >> >> > >> >> >> > Or: >> >> >> > >> >> >> > git show --raw >> >> >> > >> >> >> > In this ideal interface it's clear what the user wants to do, because >> >> >> > it's explicit. >> >> >> > >> >> >> > git show --patch --raw --no-patch >> >> >> > >> >> >> > Agreed? >> >> >> > >> >> >> > My proposal achieves your ideal explicit interface, except when no >> >> >> > format is specified (e.g. `git show`), a default format is chosen for >> >> >> > the user, but that's *only* if the user hasn't specified any format. >> >> >> >> >> >> My point is that the default format should be selected as if it has been >> >> >> provided by existing options, rather than by some magic hidden in the >> >> >> code. >> >> > >> >> > But why? >> >> > >> >> > I don't see any benefit, only drawbacks. >> >> > >> >> >> > If you explicitely specify the output format that you want, then the >> >> >> > default is irrelevant to you, thus you have your ideal explicit >> >> >> > interface. >> >> >> >> >> >> That's not what I had in mind, sorry. It'd rather be something like: >> >> >> >> >> >> --raw: set "raw" bit and clear all the rest >> >> >> --+raw set "raw" bit (== current --raw) >> >> >> ---raw clear "raw" bit (== --no-raw) >> >> >> >> >> >> In this model >> >> >> >> >> >> git show >> >> >> >> >> >> would be just an alias for >> >> >> >> >> >> git log -n1 --patch --cc >> >> >> >> >> >> and no support for a separate command would be need in the first place. >> >> >> >> >> >> git show --raw >> >> >> >> >> >> would then produce expected output that makes sense due to the common >> >> >> option processing rules, not because somebody had implemented some >> >> >> arbitrary "defaults" for the command. >> >> > >> >> > But now you are at the mercy of those "arbitrary defaults". >> >> >> >> No, see below. >> >> >> >> > >> >> > Let's say those defaults change, and now the default output of `git show` is >> >> > `--stat`. >> >> > >> >> > Now to generate the same output you have to do: >> >> > >> >> > git show --raw >> >> > >> >> > in one version of git, and: >> >> > >> >> > git show --no-stat --patch --raw >> >> > >> >> > in another. >> >> >> >> No: --raw in my model clears all the flags but --raw, so >> >> >> >> git show --raw >> >> >> >> will produce exactly the same result: raw output only. >> > >> > But that {--,--+,---} notion doesn't exist, and I think it's safe to say it >> > will never exist. So, could we limit or solution-space to those solutions that >> > could have the potential to be merged? >> >> I didn't expect it to exist any time soon, just showed a different way >> of options design. >> >> > >> > What you suggest could be easily achieved with: >> > >> > git show --silent --raw >> > >> > But because no other format is explicitely specified, following my notion of >> > defaults, that's the same as: >> >> The problem that I tried to fight is this notion of defaults that is >> somewhat special, so, if I allow for it, the rest of my suggestions >> becomes pointless, > > No, they don't, all you need to do is specify the default explicitely. > >> and without the "defaults" with non-trivial behavior[*] >> >> git show --raw >> >> won't work as expected provided --raw still just sets "raw" bit and >> doesn't clear all the rest. > > It works perfectly fine. There are no bits to clear, because there are no bits > set. When I set default value to a variable in C, it does have bits set, and they are kept unless overwritten, so they are set by default as well. Exactly the bits that I've set. Here I've proposed the same principle for handling of options. What you have in mind is exactly the current behavior and I do know how it works. My point is that I think it's not needed, as similar or better convenience could be achieved using more straightforward model. > > That's the whole point of defaults: you don't have to use them. If you don't > like the notion of defaults, then don't use them. Once again, the defaults in this form seem to be not needed to me. I already got it that you like them, and it looks like we need to agree to disagree. > If you specify *any* format option, then the defaults are ignored and no bits > are set other than the ones that you explicitly specified. That's exactly how it works now, and that's exactly what I'm arguing against. I think there is no need in introduction of this specific notion of defaults in the first place. «Entia non sunt multiplicanda praeter necessitatem» > >> [*] Defaults with trivial behavior is just initializing of internal >> variable holding flags with specific value, that is exactly the same as >> putting corresponding option(s) at the beginning. > > Those are not default arguments, those are initial arguments. In many cases > they behave the same, but not all. In my model they are both. When you set a bit initially, it's then on by default. In your model all initial bits are effectively cleared just before any bit is changed by option, and this is an additional rule. What I'm trying to explain is that this additional rule is not needed, as all the functionality could be achieved without it. From all the correct solutions of a problem the simplest one is the best. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-11 22:49 ` Sergey Organov @ 2023-05-11 23:28 ` Felipe Contreras 2023-05-12 8:40 ` Sergey Organov 0 siblings, 1 reply; 39+ messages in thread From: Felipe Contreras @ 2023-05-11 23:28 UTC (permalink / raw) To: Sergey Organov, Felipe Contreras; +Cc: git, Matthieu Moy Sergey Organov wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > Sergey Organov wrote: > >> Felipe Contreras <felipe.contreras@gmail.com> writes: > >> > Sergey Organov wrote: > >> >> Felipe Contreras <felipe.contreras@gmail.com> writes: > >> >> > Sergey Organov wrote: > >> >> >> Felipe Contreras <felipe.contreras@gmail.com> writes: > >> >> >> > Sergey Organov wrote: > >> >> >> > > >> >> >> >> I'd rather think about generic interface for setting/clearing > >> >> >> >> (multiple) bits through CI than resorting to such convenience > >> >> >> >> tricks. Once that is in place, one will be able to say "I need these > >> >> >> >> bits only", "I need to turn these bit(s) on", and "I need to turn > >> >> >> >> these bit(s) off" conveniently and universally in any part of Git CI > >> >> >> >> where it's needed. > >> >> >> > > >> >> >> > It's possible to achieve both. > >> >> >> > > >> >> >> > Imagine your ideal explicit interface. In that interface the default > >> >> >> > is no output, so you *have* to specify all the bits, for example: > >> >> >> > > >> >> >> > git show --patch > >> >> >> > >> >> >> No, that's not what I meant. There is no point in making "git show" to > >> >> >> have no output by default, please see below. > >> >> >> > >> >> >> > > >> >> >> > Or: > >> >> >> > > >> >> >> > git show --raw > >> >> >> > > >> >> >> > In this ideal interface it's clear what the user wants to do, because > >> >> >> > it's explicit. > >> >> >> > > >> >> >> > git show --patch --raw --no-patch > >> >> >> > > >> >> >> > Agreed? > >> >> >> > > >> >> >> > My proposal achieves your ideal explicit interface, except when no > >> >> >> > format is specified (e.g. `git show`), a default format is chosen for > >> >> >> > the user, but that's *only* if the user hasn't specified any format. > >> >> >> > >> >> >> My point is that the default format should be selected as if it has been > >> >> >> provided by existing options, rather than by some magic hidden in the > >> >> >> code. > >> >> > > >> >> > But why? > >> >> > > >> >> > I don't see any benefit, only drawbacks. > >> >> > > >> >> >> > If you explicitely specify the output format that you want, then the > >> >> >> > default is irrelevant to you, thus you have your ideal explicit > >> >> >> > interface. > >> >> >> > >> >> >> That's not what I had in mind, sorry. It'd rather be something like: > >> >> >> > >> >> >> --raw: set "raw" bit and clear all the rest > >> >> >> --+raw set "raw" bit (== current --raw) > >> >> >> ---raw clear "raw" bit (== --no-raw) > >> >> >> > >> >> >> In this model > >> >> >> > >> >> >> git show > >> >> >> > >> >> >> would be just an alias for > >> >> >> > >> >> >> git log -n1 --patch --cc > >> >> >> > >> >> >> and no support for a separate command would be need in the first place. > >> >> >> > >> >> >> git show --raw > >> >> >> > >> >> >> would then produce expected output that makes sense due to the common > >> >> >> option processing rules, not because somebody had implemented some > >> >> >> arbitrary "defaults" for the command. > >> >> > > >> >> > But now you are at the mercy of those "arbitrary defaults". > >> >> > >> >> No, see below. > >> >> > >> >> > > >> >> > Let's say those defaults change, and now the default output of `git show` is > >> >> > `--stat`. > >> >> > > >> >> > Now to generate the same output you have to do: > >> >> > > >> >> > git show --raw > >> >> > > >> >> > in one version of git, and: > >> >> > > >> >> > git show --no-stat --patch --raw > >> >> > > >> >> > in another. > >> >> > >> >> No: --raw in my model clears all the flags but --raw, so > >> >> > >> >> git show --raw > >> >> > >> >> will produce exactly the same result: raw output only. > >> > > >> > But that {--,--+,---} notion doesn't exist, and I think it's safe to say it > >> > will never exist. So, could we limit or solution-space to those solutions that > >> > could have the potential to be merged? > >> > >> I didn't expect it to exist any time soon, just showed a different way > >> of options design. > >> > >> > > >> > What you suggest could be easily achieved with: > >> > > >> > git show --silent --raw > >> > > >> > But because no other format is explicitely specified, following my notion of > >> > defaults, that's the same as: > >> > >> The problem that I tried to fight is this notion of defaults that is > >> somewhat special, so, if I allow for it, the rest of my suggestions > >> becomes pointless, > > > > No, they don't, all you need to do is specify the default explicitely. > > > >> and without the "defaults" with non-trivial behavior[*] > >> > >> git show --raw > >> > >> won't work as expected provided --raw still just sets "raw" bit and > >> doesn't clear all the rest. > > > > It works perfectly fine. There are no bits to clear, because there are no bits > > set. > > When I set default value to a variable in C, it does have bits set, and > they are kept unless overwritten, so they are set by default as well. > Exactly the bits that I've set. Here I've proposed the same principle > for handling of options. > > What you have in mind is exactly the current behavior No, it's very different. cur: git diff --raw --no-patch # no output new: git diff --raw --no-patch # raw output cur: git diff -s --raw # no output new: git diff -s --raw # raw output cur: git diff -s --patch --raw --no-patch # no output new: git diff -s --patch --raw --no-patch # raw output I've no idea what makes you think these are exactly the same. > > That's the whole point of defaults: you don't have to use them. If you don't > > like the notion of defaults, then don't use them. > > Once again, the defaults in this form seem to be not needed to me. I > already got it that you like them, and it looks like we need to agree to > disagree. What I (or anyone) think of the defaults is irrelevant. You don't have to use them. > > If you specify *any* format option, then the defaults are ignored and no bits > > are set other than the ones that you explicitly specified. > > That's exactly how it works now, No, it's not. Right now the code cannot distinguish between `git diff` and `git diff --no-patch`, which is precisely why the code can't turn off the DIFF_FORMAT_PATCH field. > >> [*] Defaults with trivial behavior is just initializing of internal > >> variable holding flags with specific value, that is exactly the same as > >> putting corresponding option(s) at the beginning. > > > > Those are not default arguments, those are initial arguments. In many cases > > they behave the same, but not all. > > In my model they are both. When you set a bit initially, it's then on by > default. > > In your model all initial bits are effectively cleared just before any > bit is changed by option, and this is an additional rule. What I'm > trying to explain is that this additional rule is not needed, I know it's not needed, but it's better for the end-user, as it doesn't require any mental baggage. > as all the functionality could be achieved without it. All the functionality can be achieved with it as well. "It can be done" is not an argument in favor of a change, the question is not if it _can_ be done, the question is if it *should*. > From all the correct solutions of a problem the simplest one is the > best. All absolutist claims are wrong... * almost all I think I'll have to send the patches implementing everything so you can see how my proposal is different from the status quo, not just with code, but with examples. Cheers. -- Felipe Contreras ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-11 23:28 ` Felipe Contreras @ 2023-05-12 8:40 ` Sergey Organov 2023-05-12 19:19 ` Felipe Contreras 0 siblings, 1 reply; 39+ messages in thread From: Sergey Organov @ 2023-05-12 8:40 UTC (permalink / raw) To: Felipe Contreras; +Cc: git, Matthieu Moy Felipe Contreras <felipe.contreras@gmail.com> writes: > Sergey Organov wrote: >> Felipe Contreras <felipe.contreras@gmail.com> writes: >> > Sergey Organov wrote: >> >> Felipe Contreras <felipe.contreras@gmail.com> writes: >> >> > Sergey Organov wrote: >> >> >> Felipe Contreras <felipe.contreras@gmail.com> writes: >> >> >> > Sergey Organov wrote: >> >> >> >> Felipe Contreras <felipe.contreras@gmail.com> writes: >> >> >> >> > Sergey Organov wrote: >> >> >> >> > >> >> >> >> >> I'd rather think about generic interface for setting/clearing >> >> >> >> >> (multiple) bits through CI than resorting to such convenience >> >> >> >> >> tricks. Once that is in place, one will be able to say "I need these >> >> >> >> >> bits only", "I need to turn these bit(s) on", and "I need to turn >> >> >> >> >> these bit(s) off" conveniently and universally in any part of Git CI >> >> >> >> >> where it's needed. >> >> >> >> > >> >> >> >> > It's possible to achieve both. >> >> >> >> > >> >> >> >> > Imagine your ideal explicit interface. In that interface the default >> >> >> >> > is no output, so you *have* to specify all the bits, for example: >> >> >> >> > >> >> >> >> > git show --patch >> >> >> >> >> >> >> >> No, that's not what I meant. There is no point in making "git show" to >> >> >> >> have no output by default, please see below. >> >> >> >> >> >> >> >> > >> >> >> >> > Or: >> >> >> >> > >> >> >> >> > git show --raw >> >> >> >> > >> >> >> >> > In this ideal interface it's clear what the user wants to do, because >> >> >> >> > it's explicit. >> >> >> >> > >> >> >> >> > git show --patch --raw --no-patch >> >> >> >> > >> >> >> >> > Agreed? >> >> >> >> > >> >> >> >> > My proposal achieves your ideal explicit interface, except when no >> >> >> >> > format is specified (e.g. `git show`), a default format is chosen for >> >> >> >> > the user, but that's *only* if the user hasn't specified any format. >> >> >> >> >> >> >> >> My point is that the default format should be selected as if it has been >> >> >> >> provided by existing options, rather than by some magic hidden in the >> >> >> >> code. >> >> >> > >> >> >> > But why? >> >> >> > >> >> >> > I don't see any benefit, only drawbacks. >> >> >> > >> >> >> >> > If you explicitely specify the output format that you want, then the >> >> >> >> > default is irrelevant to you, thus you have your ideal explicit >> >> >> >> > interface. >> >> >> >> >> >> >> >> That's not what I had in mind, sorry. It'd rather be something like: >> >> >> >> >> >> >> >> --raw: set "raw" bit and clear all the rest >> >> >> >> --+raw set "raw" bit (== current --raw) >> >> >> >> ---raw clear "raw" bit (== --no-raw) >> >> >> >> >> >> >> >> In this model >> >> >> >> >> >> >> >> git show >> >> >> >> >> >> >> >> would be just an alias for >> >> >> >> >> >> >> >> git log -n1 --patch --cc >> >> >> >> >> >> >> >> and no support for a separate command would be need in the first place. >> >> >> >> >> >> >> >> git show --raw >> >> >> >> >> >> >> >> would then produce expected output that makes sense due to the common >> >> >> >> option processing rules, not because somebody had implemented some >> >> >> >> arbitrary "defaults" for the command. >> >> >> > >> >> >> > But now you are at the mercy of those "arbitrary defaults". >> >> >> >> >> >> No, see below. >> >> >> >> >> >> > >> >> >> > Let's say those defaults change, and now the default output of `git show` is >> >> >> > `--stat`. >> >> >> > >> >> >> > Now to generate the same output you have to do: >> >> >> > >> >> >> > git show --raw >> >> >> > >> >> >> > in one version of git, and: >> >> >> > >> >> >> > git show --no-stat --patch --raw >> >> >> > >> >> >> > in another. >> >> >> >> >> >> No: --raw in my model clears all the flags but --raw, so >> >> >> >> >> >> git show --raw >> >> >> >> >> >> will produce exactly the same result: raw output only. >> >> > >> >> > But that {--,--+,---} notion doesn't exist, and I think it's safe to say it >> >> > will never exist. So, could we limit or solution-space to those solutions that >> >> > could have the potential to be merged? >> >> >> >> I didn't expect it to exist any time soon, just showed a different way >> >> of options design. >> >> >> >> > >> >> > What you suggest could be easily achieved with: >> >> > >> >> > git show --silent --raw >> >> > >> >> > But because no other format is explicitely specified, following my notion of >> >> > defaults, that's the same as: >> >> >> >> The problem that I tried to fight is this notion of defaults that is >> >> somewhat special, so, if I allow for it, the rest of my suggestions >> >> becomes pointless, >> > >> > No, they don't, all you need to do is specify the default explicitely. >> > >> >> and without the "defaults" with non-trivial behavior[*] >> >> >> >> git show --raw >> >> >> >> won't work as expected provided --raw still just sets "raw" bit and >> >> doesn't clear all the rest. >> > >> > It works perfectly fine. There are no bits to clear, because there are no bits >> > set. >> >> When I set default value to a variable in C, it does have bits set, and >> they are kept unless overwritten, so they are set by default as well. >> Exactly the bits that I've set. Here I've proposed the same principle >> for handling of options. >> >> What you have in mind is exactly the current behavior > > No, it's very different. > > cur: git diff --raw --no-patch # no output > new: git diff --raw --no-patch # raw output > > cur: git diff -s --raw # no output > new: git diff -s --raw # raw output > > cur: git diff -s --patch --raw --no-patch # no output > new: git diff -s --patch --raw --no-patch # raw output > > I've no idea what makes you think these are exactly the same. I was discussing the behavior of defaults rather than the behavior of particular option sets, and we already agreed about the latter from the very beginning. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-12 8:40 ` Sergey Organov @ 2023-05-12 19:19 ` Felipe Contreras 0 siblings, 0 replies; 39+ messages in thread From: Felipe Contreras @ 2023-05-12 19:19 UTC (permalink / raw) To: Sergey Organov, Felipe Contreras; +Cc: git, Matthieu Moy Sergey Organov wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > No, it's very different. > > > > cur: git diff --raw --no-patch # no output > > new: git diff --raw --no-patch # raw output > > > > cur: git diff -s --raw # no output > > new: git diff -s --raw # raw output > > > > cur: git diff -s --patch --raw --no-patch # no output > > new: git diff -s --patch --raw --no-patch # raw output > > > > I've no idea what makes you think these are exactly the same. > > I was discussing the behavior of defaults rather than the behavior of > particular option sets, and we already agreed about the latter from the > very beginning. OK, but that's a different topic than: Subject: Re: Can we clarify the purpose of `git diff -s`? I think if you want to discuss a different topic you should change the title. I'm not sure I'm interested in that discussion, but I'm glad we agree on what `-s` and `--no-patch` should do. Cheers. -- Felipe Contreras ^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <5bb24e0208dd4a8ca5f6697d578f3ae0@SAMBXP02.univ-lyon1.fr>]
* Re: Can we clarify the purpose of `git diff -s`? [not found] ` <5bb24e0208dd4a8ca5f6697d578f3ae0@SAMBXP02.univ-lyon1.fr> @ 2023-05-12 8:15 ` Matthieu Moy 2023-05-12 17:03 ` Junio C Hamano 2023-05-12 19:17 ` Felipe Contreras 0 siblings, 2 replies; 39+ messages in thread From: Matthieu Moy @ 2023-05-12 8:15 UTC (permalink / raw) To: Junio C Hamano, Sergey Organov; +Cc: Felipe Contreras, git [ I apologize for not being more reactive the last few years. I still love Git and this ml, but I'm struggling to find time to contribute. ] On 5/11/23 19:37, Junio C Hamano wrote: > The behaviour came in the v1.8.4 days with a series that was merged > by e2ecd252 (Merge branch 'mm/diff-no-patch-synonym-to-s', > 2013-07-22), which > > * made "--no-patch" a synonym for "-s"; > > * fixed "-s --patch", in which the effect of "-s" got stuck and did > not allow the patch output to be re-enabled again with "--patch"; > > * updated documentation to explain "--no-patch" as a synonym for > "-s". > > While it is very clear that the intent of the author was to make it > a synonym for "-s" and not a "feature-wise enable/disable" option, > that is what we've run with for the past 10 years. That's too old for me to remember exactly my state of mind, but if you want to do a bit of archeology, the origin is there: https://public-inbox.org/git/51E3DC47.70107@googlemail.com/ Essentially, Stefan Beller was using 'git show --format="%ad"' and expecting it to show only the author date, and for merge commits it also showed the patch (--cc). I suggested -s and noticed that the option wasn't easily discoverable, hence the patch series to better document it and add --no-patch as a synonym. Probably I did not get all the subtleties of the different kinds of outputs. I guess I considered the output of diff to be the one specified by --format plus the patch (not considering --raw, --stat & friends), hence "get only the output specified by --format" and "disable the patch" were synonym to me. Looking more closely, it's rather clear to me they are not, and that git show --raw --patch --no-patch should be equivalent to git show --raw Cheers, -- Matthieu Moy https://matthieu-moy.fr/ ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-12 8:15 ` Matthieu Moy @ 2023-05-12 17:03 ` Junio C Hamano 2023-05-12 18:21 ` Sergey Organov 2023-05-12 19:34 ` Felipe Contreras 2023-05-12 19:17 ` Felipe Contreras 1 sibling, 2 replies; 39+ messages in thread From: Junio C Hamano @ 2023-05-12 17:03 UTC (permalink / raw) To: Matthieu Moy; +Cc: Sergey Organov, Felipe Contreras, git Matthieu Moy <Matthieu.Moy@univ-lyon1.fr> writes: > https://public-inbox.org/git/51E3DC47.70107@googlemail.com/ > > Essentially, Stefan Beller was using 'git show --format="%ad"' and > expecting it to show only the author date, and for merge commits it > also showed the patch (--cc). I suggested -s and noticed that the > option wasn't easily discoverable, hence the patch series to better > document it and add --no-patch as a synonym. > > Probably I did not get all the subtleties of the different kinds of > outputs. I guess I considered the output of diff to be the one > specified by --format plus the patch (not considering --raw, --stat & > friends), hence "get only the output specified by --format" and > "disable the patch" were synonym to me. Thanks for double checking. It matches my recollection that we (you the author and other reviewers as well) added "--no-patch" back then to mean "no output from diff machinery, exactly the same as '-s' but use a name that is more discoverable". > Looking more closely, it's > rather clear to me they are not, and that > > git show --raw --patch --no-patch > > should be equivalent to > > git show --raw Yeah. If this were 10 years ago and we were designing from scratch, the "no output from diff machinery, more discoverable alias for '-s'" would have been "--silent" or "--squelch" and we would made any "--no-<format>" to defeat only "--<format>". It is a different matter if we can safely change what "--no-patch" means _now_. Given that "--no-patch" was introduced for the explicit purpose of giving "-s" a name that is easier to remember, and given that in the 10 years since we did so, we may have acquired at least a few more end users of Git than we used to have, hopefully your change have helped them discover and learn to use "--no-patch" to defeat any "--<format>" they gave earlier as initial options in their script, which will be broken and need to be updated to use a much less discoverable "-s". Thanks. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-12 17:03 ` Junio C Hamano @ 2023-05-12 18:21 ` Sergey Organov 2023-05-12 19:21 ` Junio C Hamano 2023-05-12 19:47 ` Felipe Contreras 2023-05-12 19:34 ` Felipe Contreras 1 sibling, 2 replies; 39+ messages in thread From: Sergey Organov @ 2023-05-12 18:21 UTC (permalink / raw) To: Junio C Hamano; +Cc: Matthieu Moy, Felipe Contreras, git Junio C Hamano <gitster@pobox.com> writes: > Matthieu Moy <Matthieu.Moy@univ-lyon1.fr> writes: > >> https://public-inbox.org/git/51E3DC47.70107@googlemail.com/ >> >> Essentially, Stefan Beller was using 'git show --format="%ad"' and >> expecting it to show only the author date, and for merge commits it >> also showed the patch (--cc). I suggested -s and noticed that the >> option wasn't easily discoverable, hence the patch series to better >> document it and add --no-patch as a synonym. >> >> Probably I did not get all the subtleties of the different kinds of >> outputs. I guess I considered the output of diff to be the one >> specified by --format plus the patch (not considering --raw, --stat & >> friends), hence "get only the output specified by --format" and >> "disable the patch" were synonym to me. So --no-patch, if it were made to disable only --patch from the beginning, would still serve the purpose of solving of the original problem, right? Please notice that --cc produces no output without --patch. Thus, making --no-patch a synonym for -s was a mistake in the first place that leaked through review process at that time, and git show --format="%ad" --no-patch will still work the same way even if we fix --no-patch to disable --patch only. > > Thanks for double checking. It matches my recollection that we (you > the author and other reviewers as well) added "--no-patch" back then > to mean "no output from diff machinery, exactly the same as '-s' but > use a name that is more discoverable". > >> Looking more closely, it's >> rather clear to me they are not, and that >> >> git show --raw --patch --no-patch >> >> should be equivalent to >> >> git show --raw > > Yeah. If this were 10 years ago and we were designing from scratch, > the "no output from diff machinery, more discoverable alias for > '-s'" would have been "--silent" or "--squelch" and we would made > any "--no-<format>" to defeat only "--<format>". > > It is a different matter if we can safely change what "--no-patch" > means _now_. Given that "--no-patch" was introduced for the > explicit purpose of giving "-s" a name that is easier to remember, > and given that in the 10 years since we did so, we may have acquired > at least a few more end users of Git than we used to have, hopefully > your change have helped them discover and learn to use "--no-patch" > to defeat any "--<format>" they gave earlier as initial options in > their script, which will be broken and need to be updated to use a > much less discoverable "-s". Fortunately, whoever used --no-patch are very unlikely to actually rely on it being a synonym for "-s", as it was always enough for them that --no-patch disables --patch, that will still hold after the fix. Taking this into account it should be pretty safe to fix that old mistake, and then to address "-s" discoverability issue separately. Finally, this safety concern is even less attractive provided recent "-s" fix changed behavior more aggressively yet gets no such resistance. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-12 18:21 ` Sergey Organov @ 2023-05-12 19:21 ` Junio C Hamano 2023-05-12 19:34 ` Junio C Hamano 2023-05-12 20:28 ` Felipe Contreras 2023-05-12 19:47 ` Felipe Contreras 1 sibling, 2 replies; 39+ messages in thread From: Junio C Hamano @ 2023-05-12 19:21 UTC (permalink / raw) To: Sergey Organov; +Cc: Matthieu Moy, Felipe Contreras, git Sergey Organov <sorganov@gmail.com> writes: > --patch. Thus, making --no-patch a synonym for -s was a mistake in the > first place that leaked through review process at that time, and > > git show --format="%ad" --no-patch > > will still work the same way even if we fix --no-patch to disable > --patch only. Not so fast. I have a show.outputFormat configuration variable to teach builtin/log.c::show_setup_revisions_tweak() to tweak the hardcoded default from DIFF_FORMAT_PATCH to others (primarily because I often find myself doing "git show -p --stat"). Changing "--no-patch" to toggle only "--patch" away will close the door for future improvement like that, and "will still work" is an illusion. The user needs to be told that "--no-patch" no longer means "-s" and somebody needs to apologize to them that we are deliberately breaking their reliance they held for 10 years, based on what we documented and prepare a smooth transition for them. Until the time when nobody uses "--no-patch" as a synonym for "-s" any longer, such a future improvement would be blocked. And that is another reason why I want to be much more careful about "should --no-patch be changed to mean something other than -s" than "should -s be fixed not to be sticky for some but not all options". The latter is not a documented "feature" and it clearly was a bug that "-s --raw" did not honor "--raw". The former was a documented "feature", even though it may have been a suboptimal one. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-12 19:21 ` Junio C Hamano @ 2023-05-12 19:34 ` Junio C Hamano 2023-05-12 20:28 ` Felipe Contreras 1 sibling, 0 replies; 39+ messages in thread From: Junio C Hamano @ 2023-05-12 19:34 UTC (permalink / raw) To: Sergey Organov; +Cc: Matthieu Moy, Felipe Contreras, git Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> --patch. Thus, making --no-patch a synonym for -s was a mistake in the >> first place that leaked through review process at that time, and >> >> git show --format="%ad" --no-patch >> >> will still work the same way even if we fix --no-patch to disable >> --patch only. > > Not so fast. I have a show.outputFormat configuration variable to Sorry, but please insert "patch cooking" before that "to" or I'd waste your time going through the configuration variable documentation. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-12 19:21 ` Junio C Hamano 2023-05-12 19:34 ` Junio C Hamano @ 2023-05-12 20:28 ` Felipe Contreras 2023-05-12 20:47 ` Junio C Hamano 1 sibling, 1 reply; 39+ messages in thread From: Felipe Contreras @ 2023-05-12 20:28 UTC (permalink / raw) To: Junio C Hamano, Sergey Organov; +Cc: Matthieu Moy, Felipe Contreras, git Junio C Hamano wrote: > Sergey Organov <sorganov@gmail.com> writes: > > > --patch. Thus, making --no-patch a synonym for -s was a mistake in the > > first place that leaked through review process at that time, and > > > > git show --format="%ad" --no-patch > > > > will still work the same way even if we fix --no-patch to disable > > --patch only. > > Not so fast. I have a show.outputFormat configuration variable to > teach builtin/log.c::show_setup_revisions_tweak() to tweak the > hardcoded default from DIFF_FORMAT_PATCH to others (primarily > because I often find myself doing "git show -p --stat"). Changing > "--no-patch" to toggle only "--patch" away will close the door for > future improvement like that, and "will still work" is an illusion. So your rationale to reject a perfectly logical behavior that *everyone* agrees with is that it might break a hypothetical patch? Just do `--silent` instead. > The user needs to be told that "--no-patch" no longer means "-s" and > somebody needs to apologize to them that we are deliberately > breaking their reliance they held for 10 years, Nothing is broken. And we never apologized for choosing the wrong default in `git pull`, did we? > based on what we documented and prepare a smooth transition for them. The transition is easy: just use `--silent` instead of `--no-patch`. We could add a warning that these semantics will change in the future, instead of just changing it right away. But I bet very few people will see that warning (if any), because it makes little sense to use `--no-patch` to turn off something other than `--patch`. > Until the time when nobody uses "--no-patch" as a synonym for "-s" any > longer, such a future improvement would be blocked. And that is another > reason why I want to be much more careful about "should --no-patch be changed > to mean something other than -s" than "should -s be fixed not to be sticky > for some but not all options". > The latter is not a documented "feature" and it clearly was a bug that "-s > --raw" did not honor "--raw". Users can rely on what you call "bugs". It's still a backwards-incompatible change for which you did not provide a transitioning plan in [1]. Or is it a backwards-incompatible change *only* if the person proposing the patch is somebody else other than the maintainer? [1] https://lore.kernel.org/git/20230505165952.335256-1-gitster@pobox.com/ -- Felipe Contreras ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-12 20:28 ` Felipe Contreras @ 2023-05-12 20:47 ` Junio C Hamano 2023-05-12 21:01 ` Felipe Contreras 2023-05-12 21:41 ` Sergey Organov 0 siblings, 2 replies; 39+ messages in thread From: Junio C Hamano @ 2023-05-12 20:47 UTC (permalink / raw) To: Felipe Contreras; +Cc: Sergey Organov, Matthieu Moy, git Felipe Contreras <felipe.contreras@gmail.com> writes: > So your rationale to reject a perfectly logical behavior that *everyone* agrees > with is that it might break a hypothetical patch? Everyone is an overstatement, as there are only Sergey and you, and as we all saw in public some members stated they will not engage in a discussion thread in which you were involved. In addition, at PLC I've seen people complain about how quickly a discussion that involves you becomes unproductive---they may have better sence of backward compatibility concern than you two, but they are staying silent (they are wiser than I am). > Just do `--silent` instead. I am *not* shutting the door for "--no-patch"; I am only saying that it shouldn't be done so hastily. Indeed "--silent" or "--squelch" is one of the things that I plan to suggest when we were to go with "--no-patch is no longer -s" topic. But conflating the two will delay the fix for "-s sticks unnecessarily" that is ready for this cycle. Anyway, I will be wiser and will stay out of this thread from now on, as long as you are involved. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-12 20:47 ` Junio C Hamano @ 2023-05-12 21:01 ` Felipe Contreras 2023-05-12 21:47 ` Junio C Hamano 2023-05-12 21:41 ` Sergey Organov 1 sibling, 1 reply; 39+ messages in thread From: Felipe Contreras @ 2023-05-12 21:01 UTC (permalink / raw) To: Junio C Hamano, Felipe Contreras; +Cc: Sergey Organov, Matthieu Moy, git Junio C Hamano wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > So your rationale to reject a perfectly logical behavior that *everyone* agrees > > with is that it might break a hypothetical patch? > > Everyone is an overstatement, as there are only Sergey and you, Matthieu Moy also agreed [1]: Looking more closely, it's rather clear to me they are not, and that git show --raw --patch --no-patch should be equivalent to git show --raw That's pretty much everyone that has participated in the discussion. > and as we all saw in public some members stated they will not engage in a > discussion thread in which you were involved. Smoke screen. > > Just do `--silent` instead. > > I am *not* shutting the door for "--no-patch"; I am only saying that > it shouldn't be done so hastily. > But conflating the two will delay the fix for "-s sticks unnecessarily" that > is ready for this cycle. That breaks backwards-compatibility. Why are your patches excempt from bacwards-compatibility considerations? [1] https://lore.kernel.org/git/4f713a29-1a34-2f71-ee54-c01020be903a@univ-lyon1.fr/ -- Felipe Contreras ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-12 21:01 ` Felipe Contreras @ 2023-05-12 21:47 ` Junio C Hamano 2023-05-12 21:48 ` Junio C Hamano 2023-05-12 23:21 ` Felipe Contreras 0 siblings, 2 replies; 39+ messages in thread From: Junio C Hamano @ 2023-05-12 21:47 UTC (permalink / raw) To: Felipe Contreras; +Cc: Sergey Organov, Matthieu Moy, git Felipe Contreras <felipe.contreras@gmail.com> writes: > Junio C Hamano wrote: >> Felipe Contreras <felipe.contreras@gmail.com> writes: >> >> > So your rationale to reject a perfectly logical behavior that *everyone* agrees >> > with is that it might break a hypothetical patch? >> >> Everyone is an overstatement, as there are only Sergey and you, > > Matthieu Moy also agreed [1]: > > Looking more closely, it's rather clear to me they are not, and that > > git show --raw --patch --no-patch > > should be equivalent to > > git show --raw > > That's pretty much everyone that has participated in the discussion. > >> and as we all saw in public some members stated they will not engage in a >> discussion thread in which you were involved. > > Smoke screen. > >> > Just do `--silent` instead. >> >> I am *not* shutting the door for "--no-patch"; I am only saying that >> it shouldn't be done so hastily. > >> But conflating the two will delay the fix for "-s sticks unnecessarily" that >> is ready for this cycle. > > That breaks backwards-compatibility. > > Why are your patches excempt from bacwards-compatibility considerations? It is not who wrote the patch. You either did not read what I wrote earlier in the thread ... is another reason why I want to be much more careful about "should --no-patch be changed to mean something other than -s" than "should -s be fixed not to be sticky for some but not all options". The latter is not a documented "feature" and it clearly was a bug that "-s --raw" did not honor "--raw". The former was a documented "feature", even though it may have been a suboptimal one. or you are trying to paint a picture that is different from reality with whatever motive you have. Either way, I am not done with the thread, as I said. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-12 21:47 ` Junio C Hamano @ 2023-05-12 21:48 ` Junio C Hamano 2023-05-12 23:21 ` Felipe Contreras 1 sibling, 0 replies; 39+ messages in thread From: Junio C Hamano @ 2023-05-12 21:48 UTC (permalink / raw) To: Felipe Contreras; +Cc: Sergey Organov, Matthieu Moy, git Junio C Hamano <gitster@pobox.com> writes: > ... Either way, I am not done with the > thread, as I said. Eh, yes I *am* done. drop "not". ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-12 21:47 ` Junio C Hamano 2023-05-12 21:48 ` Junio C Hamano @ 2023-05-12 23:21 ` Felipe Contreras 1 sibling, 0 replies; 39+ messages in thread From: Felipe Contreras @ 2023-05-12 23:21 UTC (permalink / raw) To: Junio C Hamano, Felipe Contreras; +Cc: Sergey Organov, Matthieu Moy, git Junio C Hamano wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > Junio C Hamano wrote: > >> Felipe Contreras <felipe.contreras@gmail.com> writes: > >> > >> > So your rationale to reject a perfectly logical behavior that *everyone* agrees > >> > with is that it might break a hypothetical patch? > >> > >> Everyone is an overstatement, as there are only Sergey and you, > > > > Matthieu Moy also agreed [1]: > > > > Looking more closely, it's rather clear to me they are not, and that > > > > git show --raw --patch --no-patch > > > > should be equivalent to > > > > git show --raw > > > > That's pretty much everyone that has participated in the discussion. > > > >> and as we all saw in public some members stated they will not engage in a > >> discussion thread in which you were involved. > > > > Smoke screen. > > > >> > Just do `--silent` instead. > >> > >> I am *not* shutting the door for "--no-patch"; I am only saying that > >> it shouldn't be done so hastily. > > > >> But conflating the two will delay the fix for "-s sticks unnecessarily" that > >> is ready for this cycle. > > > > That breaks backwards-compatibility. > > > > Why are your patches excempt from bacwards-compatibility considerations? > > It is not who wrote the patch. > > You either did not read what I wrote earlier in the thread > > ... is another reason why I want to be much more careful about > "should --no-patch be changed to mean something other than -s" than > "should -s be fixed not to be sticky for some but not all options". > The latter is not a documented "feature" and it clearly was a bug > that "-s --raw" did not honor "--raw". The former was a documented > "feature", even though it may have been a suboptimal one. > > or you are trying to paint a picture that is different from reality > with whatever motive you have. I replied to that comment quoting it in full [2]: ==== > Until the time when nobody uses "--no-patch" as a synonym for "-s" any > longer, such a future improvement would be blocked. And that is another > reason why I want to be much more careful about "should --no-patch be changed > to mean something other than -s" than "should -s be fixed not to be sticky > for some but not all options". > The latter is not a documented "feature" and it clearly was a bug that "-s > --raw" did not honor "--raw". Users can rely on what you call "bugs". It's still a backwards-incompatible change for which you did not provide a transitioning plan in [1]. Or is it a backwards-incompatible change *only* if the person proposing the patch is somebody else other than the maintainer? [1] https://lore.kernel.org/git/20230505165952.335256-1-gitster@pobox.com/ ==== How can I paint a picture different from reality when I'm quoting exactly what you said? > Either way, I am not done with the thread, as I said. You can say you are done with the thread, but your change [1] still breaks backwards compatibility. Does your patch change the behavior of this command? git show -s --raw The answer is a "yes" or a "no". Personal attacks are not a valid answer. [1] https://lore.kernel.org/git/20230505165952.335256-1-gitster@pobox.com/ [2] https://lore.kernel.org/git/645ea15eca6fa_21989f294f5@chronos.notmuch/ -- Felipe Contreras ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-12 20:47 ` Junio C Hamano 2023-05-12 21:01 ` Felipe Contreras @ 2023-05-12 21:41 ` Sergey Organov 2023-05-12 22:17 ` Junio C Hamano 2023-05-12 23:07 ` Felipe Contreras 1 sibling, 2 replies; 39+ messages in thread From: Sergey Organov @ 2023-05-12 21:41 UTC (permalink / raw) To: Junio C Hamano; +Cc: Felipe Contreras, Matthieu Moy, git Junio C Hamano <gitster@pobox.com> writes: > Felipe Contreras <felipe.contreras@gmail.com> writes: > >> So your rationale to reject a perfectly logical behavior that *everyone* agrees >> with is that it might break a hypothetical patch? > > Everyone is an overstatement, as there are only Sergey and you, Sorry, do you actually expect there is anybody here who disagrees that --no-patch logical behavior is to disable --patch? I thought you, in particular, have already agreed it's exactly "perfectly logical behavior". So there are at least 3 of us who explicitly agreed it is, and nobody who stated his disagreement. No? > and as we all saw in public some members stated they will not engage > in a discussion thread in which you were involved. In addition, at PLC > I've seen people complain about how quickly a discussion that involves > you becomes unproductive---they may have better sence of backward > compatibility concern than you two, but they are staying silent (they > are wiser than I am). The above statement with the word "everyone" was not about backward compatibility, where we obviously expect different opinions from different people. As for the sense, maybe there are people out there who do have better sense indeed, but then maybe some of them keep silence out of agreement? For what it's worth, @work I do have to maintain CI that is 600-pages long document and to take care of backward compatibility, so I do have at least some experience in this field beyond Git, and I do sympathize the conservatism in this field, and only argue about practical thresholds. As for backward compatibility itself, what I see as a problem is that the criteria of when backward incompatibility is to be considered a show-stopper, and when not, are unclear and look entirely voluntary from here. At least I was not able to correctly predict the outcome so far, that is rather discouraging. [...] > I am *not* shutting the door for "--no-patch"; That apparently confirms that you still do consider it "the perfectly logical behavior". > I am only saying that it shouldn't be done so hastily. I won't even try to insist on immediate fix, though I still don't see why shouldn't we just do it while we are at the issue, and be done with it. > Indeed "--silent" or "--squelch" is one of the things that I plan to > suggest when we were to go with "--no-patch is no longer -s" topic. While we are at this, may I vote against "--squelch", please? For me it'd be undiscoverable, as it's the first time I ever hear this word in such context. Moreover, from the meaning of the word I'd expect it to silence output unless the size of diff exceeds some limit, that in turn makes little sense. Or maybe it makes some sense? Hmm. "Show me only diffs that are more than 10 lines long". It'd be entirely different option anyway. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-12 21:41 ` Sergey Organov @ 2023-05-12 22:17 ` Junio C Hamano 2023-05-12 22:47 ` Sergey Organov 2023-05-12 23:07 ` Felipe Contreras 1 sibling, 1 reply; 39+ messages in thread From: Junio C Hamano @ 2023-05-12 22:17 UTC (permalink / raw) To: Sergey Organov; +Cc: Felipe Contreras, Matthieu Moy, git Sergey Organov <sorganov@gmail.com> writes: >> Indeed "--silent" or "--squelch" is one of the things that I plan to >> suggest when we were to go with "--no-patch is no longer -s" topic. > > While we are at this, may I vote against "--squelch", please? Sure. I actually do not think either "--silent" or "--squelch" is a good name in the context of "git show"; as the output from the command consists of the commit log message and the output from the diff machinery, and "-s" is about squelching only the latter. I have a name better than these two in mind, but as I said already, we are getting closer to the pre-release feature freeze, and I'd rather reserve my bandwidth to review rerolls of the topics that are in 'seen' and have seen some reviews already to merge them down to 'next' if they are ready, rather than staying on this thread, as "--no-patch is no longer -s" will not happen within this cycle. Thanks. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-12 22:17 ` Junio C Hamano @ 2023-05-12 22:47 ` Sergey Organov 0 siblings, 0 replies; 39+ messages in thread From: Sergey Organov @ 2023-05-12 22:47 UTC (permalink / raw) To: Junio C Hamano; +Cc: Felipe Contreras, Matthieu Moy, git Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >>> Indeed "--silent" or "--squelch" is one of the things that I plan to >>> suggest when we were to go with "--no-patch is no longer -s" topic. >> >> While we are at this, may I vote against "--squelch", please? > > Sure. I actually do not think either "--silent" or "--squelch" is a > good name in the context of "git show"; as the output from the > command consists of the commit log message and the output from the > diff machinery, and "-s" is about squelching only the latter. Yep. > > I have a name better than these two in mind, --no-diff? That said, in the context of git log/show, as opposes to git diff, it all could have been: --diff=<list> where <list> is comma-separated list of options to pass to diff, and then it'd be: --diff=off for the option in question with apparent synonym --no-diff, and less apparent synonym -s. Hold on! Now it starts to sound familiar... --diff-merges! ))) Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-12 21:41 ` Sergey Organov 2023-05-12 22:17 ` Junio C Hamano @ 2023-05-12 23:07 ` Felipe Contreras 2023-05-13 14:58 ` Philip Oakley 1 sibling, 1 reply; 39+ messages in thread From: Felipe Contreras @ 2023-05-12 23:07 UTC (permalink / raw) To: Sergey Organov, Junio C Hamano; +Cc: Felipe Contreras, Matthieu Moy, git Sergey Organov wrote: > Junio C Hamano <gitster@pobox.com> writes: > > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > >> So your rationale to reject a perfectly logical behavior that *everyone* agrees > >> with is that it might break a hypothetical patch? > > > > Everyone is an overstatement, as there are only Sergey and you, > > Sorry, do you actually expect there is anybody here who disagrees that > --no-patch logical behavior is to disable --patch? I thought you, in > particular, have already agreed it's exactly "perfectly logical > behavior". So there are at least 3 of us who explicitly agreed it is, > and nobody who stated his disagreement. No? Matthieu Moy as well, so it's 4 versus 0. > I do have at least some experience in this field beyond Git, and I do > sympathize the conservatism in this field, and only argue about practical > thresholds. I do have experience as well, as I was the one who proposed the preventive warning on changing the default of `git pull`, also changing the default of init.defaultbranch, and for the record I also warned against the move from `git-foo` to `git foo` without warning back in v1.6, which resulted in a debacle. If anything I would argue I've been more conscious of breaking backwards-compatibility for *actual* git users. This is an ad hominem red herring. > > I am *not* shutting the door for "--no-patch"; > > That apparently confirms that you still do consider it "the perfectly > logical behavior". > > > I am only saying that it shouldn't be done so hastily. > > I won't even try to insist on immediate fix, though I still don't see > why shouldn't we just do it while we are at the issue, and be done with > it. I as well. I could write yet another patch that throws a warning about a future change instead of doing the change. But I'm reasonably certain Junio will ignore that proposal as well. > > Indeed "--silent" or "--squelch" is one of the things that I plan to > > suggest when we were to go with "--no-patch is no longer -s" topic. > > While we are at this, may I vote against "--squelch", please? For me > it'd be undiscoverable, as it's the first time I ever hear this word in > such context. Also agree. I've never heard the word "squelch" outside of the git context, and I'm pretty sure my English vocabulary is not small. Multiple people have suggested "silent" and no one has suggested "squelch" (other than Junio). Cheers. -- Felipe Contreras ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-12 23:07 ` Felipe Contreras @ 2023-05-13 14:58 ` Philip Oakley 2023-05-13 17:45 ` Sergey Organov 0 siblings, 1 reply; 39+ messages in thread From: Philip Oakley @ 2023-05-13 14:58 UTC (permalink / raw) To: Felipe Contreras, Sergey Organov, Junio C Hamano; +Cc: Matthieu Moy, git On 13/05/2023 00:07, Felipe Contreras wrote: > Also agree. I've never heard the word "squelch" outside of the git context, and > I'm pretty sure my English vocabulary is not small. Multiple people have > suggested "silent" and no one has suggested "squelch" (other than Junio). Squelch is common in the right circles. It is the term for cutting out the static electricity based background audio noise from radio receivers. Back in the day, it was an adjustable dial, so anyone who used a radio (including ex-military and their family) would be familiar with the term. It's not so common nowadays because there is auto-squelch and auto tuning on most receivers, but it's still part of the lexicon ;-) Philip ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-13 14:58 ` Philip Oakley @ 2023-05-13 17:45 ` Sergey Organov 0 siblings, 0 replies; 39+ messages in thread From: Sergey Organov @ 2023-05-13 17:45 UTC (permalink / raw) To: Philip Oakley; +Cc: Felipe Contreras, Junio C Hamano, Matthieu Moy, git Philip Oakley <philipoakley@iee.email> writes: > On 13/05/2023 00:07, Felipe Contreras wrote: >> Also agree. I've never heard the word "squelch" outside of the git context, and >> I'm pretty sure my English vocabulary is not small. Multiple people have >> suggested "silent" and no one has suggested "squelch" (other than Junio). > Squelch is common in the right circles. > > It is the term for cutting out the static electricity based background > audio noise from radio receivers. Back in the day, it was an adjustable > dial, so anyone who used a radio (including ex-military and their > family) would be familiar with the term. Well, then we'd need to adopt something like --squelch=17, right? ;-) > > It's not so common nowadays because there is auto-squelch and auto > tuning on most receivers, but it's still part of the lexicon ;-) ... and then, finally, --squelch=auto ) And then it should still switch back to full output once input signal (size of diff?) exceeds specific margin. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-12 18:21 ` Sergey Organov 2023-05-12 19:21 ` Junio C Hamano @ 2023-05-12 19:47 ` Felipe Contreras 1 sibling, 0 replies; 39+ messages in thread From: Felipe Contreras @ 2023-05-12 19:47 UTC (permalink / raw) To: Sergey Organov, Junio C Hamano; +Cc: Matthieu Moy, Felipe Contreras, git Sergey Organov wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Matthieu Moy <Matthieu.Moy@univ-lyon1.fr> writes: > > > >> https://public-inbox.org/git/51E3DC47.70107@googlemail.com/ > >> > >> Essentially, Stefan Beller was using 'git show --format="%ad"' and > >> expecting it to show only the author date, and for merge commits it > >> also showed the patch (--cc). I suggested -s and noticed that the > >> option wasn't easily discoverable, hence the patch series to better > >> document it and add --no-patch as a synonym. > >> > >> Probably I did not get all the subtleties of the different kinds of > >> outputs. I guess I considered the output of diff to be the one > >> specified by --format plus the patch (not considering --raw, --stat & > >> friends), hence "get only the output specified by --format" and > >> "disable the patch" were synonym to me. > > So --no-patch, if it were made to disable only --patch from the > beginning, would still serve the purpose of solving of the original > problem, right? Please notice that --cc produces no output without > --patch. Thus, making --no-patch a synonym for -s was a mistake in the > first place that leaked through review process at that time, and > > git show --format="%ad" --no-patch > > will still work the same way even if we fix --no-patch to disable > --patch only. Indeed. > > Thanks for double checking. It matches my recollection that we (you > > the author and other reviewers as well) added "--no-patch" back then > > to mean "no output from diff machinery, exactly the same as '-s' but > > use a name that is more discoverable". > > > >> Looking more closely, it's > >> rather clear to me they are not, and that > >> > >> git show --raw --patch --no-patch > >> > >> should be equivalent to > >> > >> git show --raw > > > > Yeah. If this were 10 years ago and we were designing from scratch, > > the "no output from diff machinery, more discoverable alias for > > '-s'" would have been "--silent" or "--squelch" and we would made > > any "--no-<format>" to defeat only "--<format>". > > > > It is a different matter if we can safely change what "--no-patch" > > means _now_. Given that "--no-patch" was introduced for the > > explicit purpose of giving "-s" a name that is easier to remember, > > and given that in the 10 years since we did so, we may have acquired > > at least a few more end users of Git than we used to have, hopefully > > your change have helped them discover and learn to use "--no-patch" > > to defeat any "--<format>" they gave earlier as initial options in > > their script, which will be broken and need to be updated to use a > > much less discoverable "-s". > > Fortunately, whoever used --no-patch are very unlikely to actually rely > on it being a synonym for "-s", as it was always enough for them that > --no-patch disables --patch, that will still hold after the fix. That's right. And let's be realistic for a moment: nobody actually does `git diff-files --raw`, as that's essentially the same as `cat /dev/null`: a no-op. The reason `--no-patch` was added was to silenced the diff output of commands that show a diff *in addition* to something else by default, like `git show`, and `git show --no-patch` will keep working fine. Why would anybody do `git show --raw --no-patch` when they can do `git show --no-patch`? Yet once again we are doing premature defense for a set of users that probably don't even exist. > Finally, this safety concern is even less attractive provided recent > "-s" fix changed behavior more aggressively yet gets no such resistance. Exactly. --- And this is yet another example of why git's UI is stuck and cannot (and probably will never) be fixed. Cheers. -- Felipe Contreras ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-12 17:03 ` Junio C Hamano 2023-05-12 18:21 ` Sergey Organov @ 2023-05-12 19:34 ` Felipe Contreras 1 sibling, 0 replies; 39+ messages in thread From: Felipe Contreras @ 2023-05-12 19:34 UTC (permalink / raw) To: Junio C Hamano, Matthieu Moy; +Cc: Sergey Organov, Felipe Contreras, git Junio C Hamano wrote: > Matthieu Moy <Matthieu.Moy@univ-lyon1.fr> writes: > > > https://public-inbox.org/git/51E3DC47.70107@googlemail.com/ > > > > Essentially, Stefan Beller was using 'git show --format="%ad"' and > > expecting it to show only the author date, and for merge commits it > > also showed the patch (--cc). I suggested -s and noticed that the > > option wasn't easily discoverable, hence the patch series to better > > document it and add --no-patch as a synonym. > > > > Probably I did not get all the subtleties of the different kinds of > > outputs. I guess I considered the output of diff to be the one > > specified by --format plus the patch (not considering --raw, --stat & > > friends), hence "get only the output specified by --format" and > > "disable the patch" were synonym to me. > > Thanks for double checking. It matches my recollection that we (you > the author and other reviewers as well) added "--no-patch" back then > to mean "no output from diff machinery, exactly the same as '-s' but > use a name that is more discoverable". > > > Looking more closely, it's > > rather clear to me they are not, and that > > > > git show --raw --patch --no-patch > > > > should be equivalent to > > > > git show --raw > > Yeah. If this were 10 years ago and we were designing from scratch, > the "no output from diff machinery, more discoverable alias for > '-s'" would have been "--silent" or "--squelch" and we would made > any "--no-<format>" to defeat only "--<format>". > > It is a different matter if we can safely change what "--no-patch" > means _now_. We can. As we have been able to do backwards-incompatible changes in the past, will keep doing in the future. You yourself proposed a backwards-incompatible change here [1]. > Given that "--no-patch" was introduced for the explicit purpose of giving > "-s" a name that is easier to remember, and given that in the 10 years since > we did so, we may have acquired at least a few more end users of Git than we > used to have, hopefully your change have helped them discover and learn to > use "--no-patch" to defeat any "--<format>" they gave earlier as initial > options in their script, which will be broken and need to be updated to use a > much less discoverable "-s". That is not true. `--no-patch` is not used to defeat any `--<format>`, it's used to disable output, for example this: git show --no-patch There is zero point in writing: git show --patch --no-patch Because `--patch` is already the default. But all of these would keep working fine if we change the semantics of `--no-patch`. It's not true that they will be broken. It's only when the default is a format other than patch, or a format other than patch is explicitely specified, for example: git diff-files --no-patch git show --raw --no-patch Potentially the number of users who actually do this is *zero*. A few users for some reason may have come to rely on the above behavior, but a few users might have come to rely on the following behavior as well: git show -s --raw Which your patch [1] breaks. [1] https://lore.kernel.org/git/20230505165952.335256-1-gitster@pobox.com/ -- Felipe Contreras ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: Can we clarify the purpose of `git diff -s`? 2023-05-12 8:15 ` Matthieu Moy 2023-05-12 17:03 ` Junio C Hamano @ 2023-05-12 19:17 ` Felipe Contreras 1 sibling, 0 replies; 39+ messages in thread From: Felipe Contreras @ 2023-05-12 19:17 UTC (permalink / raw) To: Matthieu Moy, Junio C Hamano, Sergey Organov; +Cc: Felipe Contreras, git Matthieu Moy wrote: > [ I apologize for not being more reactive the last few years. I still > love Git and this ml, but I'm struggling to find time to contribute. ] > > On 5/11/23 19:37, Junio C Hamano wrote: > > The behaviour came in the v1.8.4 days with a series that was merged > > by e2ecd252 (Merge branch 'mm/diff-no-patch-synonym-to-s', > > 2013-07-22), which > > > > * made "--no-patch" a synonym for "-s"; > > > > * fixed "-s --patch", in which the effect of "-s" got stuck and did > > not allow the patch output to be re-enabled again with "--patch"; > > > > * updated documentation to explain "--no-patch" as a synonym for > > "-s". > > > > While it is very clear that the intent of the author was to make it > > a synonym for "-s" and not a "feature-wise enable/disable" option, > > that is what we've run with for the past 10 years. > > That's too old for me to remember exactly my state of mind, but if you > want to do a bit of archeology, the origin is there: > > https://public-inbox.org/git/51E3DC47.70107@googlemail.com/ Yes, I had already sent a link to that thread [1]. > Essentially, Stefan Beller was using 'git show --format="%ad"' and > expecting it to show only the author date, and for merge commits it also > showed the patch (--cc). I suggested -s and noticed that the option > wasn't easily discoverable, hence the patch series to better document it > and add --no-patch as a synonym. That was my understanding. So the goal was to make the silencing of `git show` output more accessible. > Probably I did not get all the subtleties of the different kinds of > outputs. I guess I considered the output of diff to be the one specified > by --format plus the patch (not considering --raw, --stat & friends), > hence "get only the output specified by --format" and "disable the > patch" were synonym to me. Looking more closely, it's rather clear to me > they are not, and that > > git show --raw --patch --no-patch > > should be equivalent to > > git show --raw Indeed, but at the time such funcionality was not easy to achieve, on the other hand making `--no-patch` be synonymous with `-s` was easy, so that's the path that was followed. But that was not the goal, that was a means to an end. Adding `--silent` as a synonym to `-s` would have also served a similar goal. I sent a patch to add such `--silent` alias [2], and I also sent a patch to decouple `--no-patch` from `-s` [3]. All these three keep working as it was originally intended by your patch: * git show -s * git show --silent * git show --no-patch The only difference is that now these are different: * git show --patch --raw --no-patch * git show --patch --raw --silent Which wasn't considered back then. Cheers. [1] https://lore.kernel.org/git/645d3122bd1d2_26011a2947a@chronos.notmuch/ [2] https://lore.kernel.org/git/20230512080339.2186324-7-felipe.contreras@gmail.com/ [3] https://lore.kernel.org/git/20230512080339.2186324-6-felipe.contreras@gmail.com/ -- Felipe Contreras ^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2023-05-13 17:45 UTC | newest] Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-05-11 3:14 Can we clarify the purpose of `git diff -s`? Felipe Contreras 2023-05-11 11:59 ` Sergey Organov 2023-05-11 16:26 ` Junio C Hamano 2023-05-11 17:37 ` Junio C Hamano 2023-05-11 18:04 ` Sergey Organov 2023-05-11 18:27 ` Junio C Hamano 2023-05-11 18:36 ` Felipe Contreras 2023-05-11 18:17 ` Felipe Contreras 2023-05-11 17:41 ` Felipe Contreras 2023-05-11 18:31 ` Sergey Organov 2023-05-11 19:10 ` Felipe Contreras 2023-05-11 19:32 ` Sergey Organov 2023-05-11 19:54 ` Felipe Contreras 2023-05-11 20:24 ` Sergey Organov 2023-05-11 20:59 ` Felipe Contreras 2023-05-11 22:49 ` Sergey Organov 2023-05-11 23:28 ` Felipe Contreras 2023-05-12 8:40 ` Sergey Organov 2023-05-12 19:19 ` Felipe Contreras [not found] ` <5bb24e0208dd4a8ca5f6697d578f3ae0@SAMBXP02.univ-lyon1.fr> 2023-05-12 8:15 ` Matthieu Moy 2023-05-12 17:03 ` Junio C Hamano 2023-05-12 18:21 ` Sergey Organov 2023-05-12 19:21 ` Junio C Hamano 2023-05-12 19:34 ` Junio C Hamano 2023-05-12 20:28 ` Felipe Contreras 2023-05-12 20:47 ` Junio C Hamano 2023-05-12 21:01 ` Felipe Contreras 2023-05-12 21:47 ` Junio C Hamano 2023-05-12 21:48 ` Junio C Hamano 2023-05-12 23:21 ` Felipe Contreras 2023-05-12 21:41 ` Sergey Organov 2023-05-12 22:17 ` Junio C Hamano 2023-05-12 22:47 ` Sergey Organov 2023-05-12 23:07 ` Felipe Contreras 2023-05-13 14:58 ` Philip Oakley 2023-05-13 17:45 ` Sergey Organov 2023-05-12 19:47 ` Felipe Contreras 2023-05-12 19:34 ` Felipe Contreras 2023-05-12 19:17 ` Felipe Contreras
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).