Felipe Contreras <felipe.contreras@xxxxxxxxx> 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
