Jonathan Nieder [off-list ref] writes:
The problem is that although diff generation options are only relevant
for the displayed diff, a script author can imagine them affecting
path limiting. For example, I might run
I am somewhat puzzled. What does "can imagine" exactly mean and
justify this change? A script author may imagine "git cat-file" can
be expected to meow, but the command actually does not meow and end
up disappointing the author, but that wouldn't justify a rename of
"cat-file" to something else.
git log -w --format=%H -- README
hoping to list commits that edited README, excluding whitespace-only
changes. In fact, a whitespace-only change is not TREESAME so the use
of -w here has no effect (since we don't apply these diff generation
flags to the diff_options struct rev_info::pruning used for this
purpose), but the documentation suggests that it should work
Suppose you specified foo as the <paths>. We shall call
commits that modify foo !TREESAME, and the rest TREESAME. (In
a diff filtered for foo, they look different and equal,
respectively.)
and a script author who has not tested whitespace-only changes
wouldn't notice.
It would need to be corrected by a bugfix of either TREESAME
computation, or a documentation fix, I would think. I fail to see
the similarity you perceive to the "-m" issue at hand, though.
Similarly, a script author could include
git log -m --first-parent --format=%H -- README
to filter the first-parent history for commits that modified README.
The -m is a no-op but it reflects the script author's intent.
So the expectation is with "-m" we'd give single parent commits on
the fp chain, and merges from side branches that change README, in
addition to merges from side branches that was forked way before the
README was updated on the trunk (hence had ancient README but the
merge kept the version from the trunk)?
For
example, until 1e20a407fe2 (stash list: stop passing "-m" to "git
log", 2021-05-21), "git stash list" did this.
This is not a example that supports your conclusion, though. The
reason why 288c67ca (stash: default listing to working-tree diff,
2014-08-06) added "-m" on the command line to make it:
git log --format="%gd: %gs" -g --first-parent -m "$@" $ref_stash --
is to prepare for the users who may pass "-p" as part of the "$@";
they wil get no patches out of these merge commits that represent
stash entries otherwise, and they'd have to pass "-m -p" instead,
without the change.
As a result, we can't safely change "-m" to imply "-p" without fear of
breaking such scripts. Restore the previous behavior.
So the above is *not* an example of a script that would have been
broken with this change.
Hi,
Junio C Hamano wrote:
I am somewhat puzzled. What does "can imagine" exactly mean and
justify this change? A script author may imagine "git cat-file" can
be expected to meow, but the command actually does not meow and end
up disappointing the author, but that wouldn't justify a rename of
"cat-file" to something else.
Sorry for the lack of clarity. I was describing what leads a script
author to include "-m" in a place where it has no effect.
You might be inclined to wonder why it matters _why_ a script author
would do such a thing, if the script author is wrong. To me, it
matters because it allows us to estimate how common it is for scripts
to use "-m" in this way.
The motivating example (Rust) shows that there is at least one script
that _did_ use "-m" in this way. Rust has mitigation, but the above
logic leads me to believe that they are not the only project that will
be affected. And more generally, when a script author has a
reasonable reason to believe something will work, they write scripts
where it _does_ work, and then an update breaks their script, I think
it's reasonable for them not to be happy.
Jonathan
Am 07.08.21 um 03:55 schrieb Jonathan Nieder:
The motivating example (Rust) shows that there is at least one script
that _did_ use "-m" in this way. Rust has mitigation, but the above
logic leads me to believe that they are not the only project that will
be affected. And more generally, when a script author has a
reasonable reason to believe something will work, they write scripts
where it _does_ work, and then an update breaks their script, I think
it's reasonable for them not to be happy.
As you know, we have "plumbing" commands with a stable interface and
"porcelain" commands for which we reserve to change the behavior without
advance notice. By your reasoning we would not need to distinguish
between the two categories and were forced to keep all behavior stable.
This undoing of a behavior change in a "porcelain" command with the
argument that one script depended on the old behavior and that others
might do so as well would set an unwanted precedent.
Perhaps we need to point script authors to "plumbing" commands more clearly?
(BTW, I have no opinion on whether -m should or should not imply -p.)
-- Hannes
Hi Hannes,
Johannes Sixt wrote:
Am 07.08.21 um 03:55 schrieb Jonathan Nieder:
quoted
The motivating example (Rust) shows that there is at least one script
that _did_ use "-m" in this way. Rust has mitigation, but the above
logic leads me to believe that they are not the only project that will
be affected. And more generally, when a script author has a
reasonable reason to believe something will work, they write scripts
where it _does_ work, and then an update breaks their script, I think
it's reasonable for them not to be happy.
As you know, we have "plumbing" commands with a stable interface and
"porcelain" commands for which we reserve to change the behavior without
advance notice. By your reasoning we would not need to distinguish
between the two categories and were forced to keep all behavior stable.
This undoing of a behavior change in a "porcelain" command with the
argument that one script depended on the old behavior and that others
might do so as well would set an unwanted precedent.
Hm, this is worth elucidating a bit more, since I am definitely in
favor of continuing to change porcelain commands for the better where
we can. If we decide that "git log --format=<fmt>" is no longer part
of the stable scripting interface we provide, then that would be a
huge change for our callers (and it's probably too late), but I would
certainly be in favor of us going back in time and doing that. :)
More generally, we've been able to make changes to porcelain commands
that don't hurt our ability to act as a platform for scripts, and I
want us to continue to be able to do that. "Do not break any script"
is certainly not the standard I think we should apply, as illustrated
by my thoughts upthread when I thought '-m' in this Rust example was a
typo.
But by now it's very clear to me that it was not a typo.
In other words:
- this isn't only about one obscure script. The point of the "this
was not a typo" logic is to illustrate that in addition to the
examples that we know about it is very likely that there are
examples that we don't know about, in teams' script collections
beyond the reach of search engines.
- In fact, in addition to the motivating example that makes it
possible to build Rust, we had multiple in-tree scripts that would
also have broken by this, if they had not been adapted to work
around that in the same series! I should have noticed that in
review, and I'm sorry that I didn't.
Perhaps we need to point script authors to "plumbing" commands more clearly?
I think the existence of "plumbing" is fairly well known, but users
don't always have an easy time using it. The "porcelain" is what ends
up getting the most attention in improvements, and so while I
encourage script authors to use 'git rev-list <revs> | git diff-tree
-s --stdin --format=<fmt>' in place of 'git log --format=<fmt>
<revs>', most do not listen, and I can't really blame them given how
much more convenient the latter is and how many more options it
supports.
I don't think that situation will change unless we
a. Maintain a second, parallel implementation of each porcelain
command that only uses plumbing. This would provide an example of
how to use plumbing and would ensure that the plumbing grows in
capability at the same time as the corresponding porcelain. Or
b. Expose a library interface, so that we can expose the actual
helpers that support the standard implementation of porcelain
commands.
I tried a little of (a) years ago by updating contrib/examples/ to
pass tests: https://lore.kernel.org/git/20100817065147.GA18293@burratino/.
It was fun but I don't think it's really sustainable.
In the long term, I think (b) is going to be an important thing to do,
and I think it will be helpful. Some automated callers would
appreciate the ability to pass structured input instead of having to
pretend to be shell scripts. :) True shell scripts would also benefit
because the plumbing commands can more directly map to such a library
API.
(BTW, I have no opinion on whether -m should or should not imply -p.)
Nevertheless, thanks for weighing in.
Jonathan