Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi team, especially Stefan: your thorough investigation about a better name than range-diff gives me confidence that my decision to retract my objection against has merit: it seems to be by far the one name that everybody but me agrees on. And I can adapt easily. On Sat, 26 May 2018, Øyvind Rønningstad wrote: > Just want to throw my support in for range-diff since ranges is what you > pass to the command. `range-diff` it is. Ciao, Dscho
Fwd: [PATCH v2 02/18] Add a new builtin: branch-diff
Just want to throw my support in for range-diff since ranges is what you pass to the command. Alternatively, diff-diff since that's how I've crudely tried to accomplish this before. git diff A..B > diff1 git diff C..D > diff2 winmerge diff1 diff2
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Johannes, On IRC you wrote: And BTW this is not bike-shedding to me. Discussing the name of a variable, or indentation, or line wrapping, is. But improving the user experience is important. We *suck* on that, historically, and I do want to break with that habit. ... avar, _ikke_: so a colleague of mine whose opinion on naming I respect more than all Git developers combined *also* came up with the term `range-diff`, independently. ... Yes, you are looking at two ranges. But not *any* two ranges. *That* is my point. So I sat back and want to try again; IIUC your dislike for "range-diff" boils down to: (A) it doesn't diff any arbitrary range, as the output would become very cumbersome and hard to understand, (B) it is not a good intuitive name for users, as they would not think of range-diff when they'd want to have this feature. Regarding (A), I think the same can be said about input to the diff machinery, e.g. 'git diff v2.0.0 v2.17.0' is just very much text, and it is hardly useful (except as a patch fed to the machine). Over time there were added tons of options that make the diff output easier to digest, e.g. additional pathspecs to restrict to a sub tree or ignoring certain things (white spaces mostly), such that 'git diff -w v2.0.0 v2.17.0 -- refs.h' is easier for a human to grok. Regarding (B), I agree, but blame it on the nature of an open source project that provides a toolbox. So the way a user is going to discover this feature is via stackoverflow or via asking a coworker or finding the example output somewhere. I think that last point could be part of the feedback: git-diff has prominently hints at its name via "diff --git ..." in the first line of its output, so maybe the output of this feature also wants to name itself? Other thoughts: We could go down the route and trying to find a best possible technical name, for which I could offer: revision-walk-difference revwalk-diff As that literally describes the output: two rev walks are performed and then those outputs of the rev walks is diffed. Based off these technicals we could get more creative: redo-rev-walk-spot-the-difference re-walk-spot retravel-spot spot-diff But I think all these do not address the feedback (B). "What would a user find intuitive?"; I personally thought a bit about how I discovered cherry-pick. I just took it as a given name, without much thought, as I discovered it by tell tale, not looking for it in the docs. It sort of made sense as a command that I learned earlier about, "interactive rebase", also has had the "pick" command, such that "picking" made sense. I think I retroactively made sense of the "cherry" part. Now I tried to find it in the mailing list archive and actually learn about its origin, but no good stories are found there. For what the user might find most useful, I just looked at other tools in Gerrits landscape and there the expectation seems that you upload your code first and do the diff of the different patches serverside. I think the same holds for Github or other branch based reviewing systems. You can force push the branch that is pull requested and the web UI somehow makes sense of it. That leads me to the (weak) conclusion of branch-diff or tbdiff to be useful most for patch based / mailing list based workflows as there is no magic server helping you out. Searching for "kernel +tbdiff" to find the kernel devs using tbdiff gave me no results, so I may be mistaken there. Trying to find "interdiffs" (for the lack of a better name) between patches on the kernel mailing list also is not obvious to the uninitiated. So for the various workflows, I could come up with change-diff pullrequest-diff patch-series-diff but we do not look at diffs, rather we only use this tool to work on incremental things, so maybe instead: change-history pullrequest-history patch-series-evolution Note how these are 3 suggestions, one for each major workflow, and I'd *REALLY* would want to have a tool that is agnostic to the workflow on top (whether you use pull requests or Gerrit changes), but now I would like to step back and remind us that this tool is only mostly used for viewing the evolution of your new thing, but it can also be very useful to inspect non-new things. (backported patches to maint, or some -stable branch) Or rather: We do not know the major use case yet. Sure I will use it in my cover letter and that is on my mind now, but I think there are other use cases that are not explored yet, so we should rather make the naming decision based off of technicals rather than anticipated use case and user discovery methods. I hope this is actually useful feedback on the naming discovery. Thanks, Stefan
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
On Tue, May 08 2018, Jeff King wrote: > On Mon, May 07, 2018 at 03:24:59PM -0700, Stefan Beller wrote: > >> Hence I propose "git range-diff", similar to topic-diff, that >> was proposed earlier. >> >> * it "diffs ranges" of commits. >> * it can also deal with out-of-git things like patch series, >> but that is a mere by product and may not be desired. >> Just like git-diff can also compare two files outside a git >> repo, that would not be a good use case. >> Keep the name Git-centric! >> * it autocompletes well. > > FWIW, I like this by far of all of the suggested names. I agree, "range-diff" is the best one mentioned so far.
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Jeff King writes: >> I haven't really been following all of the discussion but from what I >> can tell the point of this command is to generate a diff based on two >> different versions of a series, so why not call it 'series-diff'? :) > > That's OK with me, though I prefer "range" as I think we use that term > elsewhere ("series" is usually part of "patch series", but many people > do not use a workflow with that term). FWIW, I am OK with either, with a bit of preference to "range" over "series". As long as this stays to be an independent command (as opposed to be made into a new mode to existing command) and the command name is not overly hard to type, I am OK with anything ;-)
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
On Mon, May 21, 2018 at 02:40:57PM -0700, Brandon Williams wrote: > > > > Most fellow German software engineers (who seem to have a knack for > > > > idiotically long variable/function names) would now probably suggest: > > > > > > > > git compare-patch-series-with-revised-patch-series > > > > > > or short: > > > > > > revision-compare > > > compare-revs > > > com-revs > > > > > > revised-diff > > > revise-diff > > > revised-compare > > > > > > diff-revise > > I haven't really been following all of the discussion but from what I > can tell the point of this command is to generate a diff based on two > different versions of a series, so why not call it 'series-diff'? :) That's OK with me, though I prefer "range" as I think we use that term elsewhere ("series" is usually part of "patch series", but many people do not use a workflow with that term). -Peff
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
On Mon, May 21, 2018 at 2:40 PM, Brandon Williams wrote: revised-compare >> > >> > diff-revise > > I haven't really been following all of the discussion but from what I > can tell the point of this command is to generate a diff based on two > different versions of a series, so why not call it 'series-diff'? :) Upon mentioning series-diff, I misheard Brandon and thought he proposed serious-diff :-) Stefan
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
On 05/21, Jeff King wrote: > On Mon, May 21, 2018 at 10:56:47AM -0700, Stefan Beller wrote: > > > > It is very much about > > > comparing two *ranges of* revisions, and not just any ranges, no. Those > > > ranges need to be so related as to contain mostly identical changes. > > > > range-diff, eh? > > > > > Most fellow German software engineers (who seem to have a knack for > > > idiotically long variable/function names) would now probably suggest: > > > > > > git compare-patch-series-with-revised-patch-series > > > > or short: > > > > revision-compare > > compare-revs > > com-revs > > > > revised-diff > > revise-diff > > revised-compare > > > > diff-revise I haven't really been following all of the discussion but from what I can tell the point of this command is to generate a diff based on two different versions of a series, so why not call it 'series-diff'? :) -- Brandon Williams
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
On Mon, May 21, 2018 at 10:56:47AM -0700, Stefan Beller wrote: > > It is very much about > > comparing two *ranges of* revisions, and not just any ranges, no. Those > > ranges need to be so related as to contain mostly identical changes. > > range-diff, eh? > > > Most fellow German software engineers (who seem to have a knack for > > idiotically long variable/function names) would now probably suggest: > > > > git compare-patch-series-with-revised-patch-series > > or short: > > revision-compare > compare-revs > com-revs > > revised-diff > revise-diff > revised-compare > > diff-revise I still like "range diff", but I think something around "revise" is a good line of thought, too. Because it implies that we expect the two ranges to be composed of almost-the-same commits. That leads to another use case where I think focusing on topic branches (or even branches at all) would be a misnomer. Imagine I cherry-pick a bunch of commits with: git cherry-pick -10 $old_commit I might then want to see how the result differs with something like: git range-diff $old_commit~10..$old_commit HEAD~10..HEAD I wouldn't think of this as a topic-branch operation, but just as comparing two sequences of commits. I guess "revise" isn't strictly accurate here either, as I'm not revising. But I do assume the two ranges share some kind of mapping of patches. -Peff PS I wish there were a nicer syntax to do that. Perhaps "git range-diff -10 $old_commit HEAD" could work, though occasionally the two ranges are not the same length (e.g., if you ended up skipping one of the cherry-picked commits). Anyway, those kind of niceties can easily come later on top. :)
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi Johannes, >> (2) git diff --branch topic-v1...topic-v2 > > From my point of view, `git diff --branch` indicates that I diff > *branches*. Which is not really something that makes sense, and definitely > not what this command is about. > > We are not comparing branches. > > We are comparing versions of the same branch. I happen to have a messier workflow than you have, as I develop a "resend" of a topic in a new branch (or I have to restore the old sent topic from the reflog). Now that I have the tool I also compare two branches, namely, the branch that Junio queued (origin/base..origin/sb/intelligent-name) vs the resend that I had locally (origin/base..foo). Next time I might compare Junios queued topic to the local format-patch'es that I already annotated. So in a way this diffs different versions of a topic, "diff-topic-versions". >> It`s all still a comparison between two revisions (pointed to by >> "topic-v1" and "topic-v2" branch heads in this specific example), but >> it differs in what we are comparing - (1) set of files contained in >> endpoints, or (2) set of revisions contained in (or "leading to") >> endpoints. > > It is very much not about comparing *two* revisions. I wonder if we can make the tool more intelligent to take two revisions and it figures out the range by finding the base branch itself. Probably as a follow up. > It is very much about > comparing two *ranges of* revisions, and not just any ranges, no. Those > ranges need to be so related as to contain mostly identical changes. range-diff, eh? > Most fellow German software engineers (who seem to have a knack for > idiotically long variable/function names) would now probably suggest: > > git compare-patch-series-with-revised-patch-series or short: revision-compare compare-revs com-revs revised-diff revise-diff revised-compare diff-revise > I hope you agree that that is better *and* worse than your suggestions, > depending from what angle you look at it: it is better because it > describes what the command is *actually* doing. But it is much worse at > the same time because it is too long. btw, you think very much in terms of *patch series*, but there are workflows without patches (pull requests at Github et Al., changes in Gerrit), and I would think the output of the tool under discussion would still be useful. In [1] Junio gives his use case, it is "before accepting them", which could be comparing an mbox or patch files against a branch, or first building up a local history on a detached head (and then wondering if to reset the branch to the new history), which would be all in Git. That use case still has 'patches' involved, but these are not the main selling point for the tool, as you could turn patches into commits before using this tool. [1] https://public-inbox.org/git/xmqqvabh1ung@gitster-ct.c.googlers.com/ Thanks, Stefan
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi Junio, On Tue, 8 May 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > > It would be easy to introduce, but I am wary about its usefulness. > > Unless you re-generate the branch from patches (which I guess you do a > > lot, but I don't), you are likely to compare incomplete patch series: say, > > when you call `git rebase -i` to reword 05/18's commit message, your > > command will only compare 05--18 of the patch series. > > Well that is exactly the point of that "..@{1} @{1}..", which turned > out to be very useful in practice at least for me when I am updating > a topic with "rebase -i", and then reviewing what I did with tbdiff. > > I do not want 01-04 in the above case as I already know I did not > touch them. And you are a seasoned veteran maintainer. To the occasional contributor, this information is not obvious, and it is not stored in their brain. It needs to be made explicit, which is why this here command outputs those `abcdef = 012345` lines: it lists all the commits, stating which ones are unchanged. In your 01-04 example, those lines would be of the form `abcdef = abcdef`, of course. Ciao, Dscho
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi Buga, On Mon, 7 May 2018, Igor Djordjevic wrote: > On 07/05/2018 09:48, Jeff King wrote: > > > > > > Let's, please, not fall into the trap of polluting git-branch with > > > > utterly unrelated functionality, as has happened a few times with > > > > other Git commands. Let's especially not do so merely for the sake of > > > > tab-completion. git-branch is for branch management; it's not for > > > > diff'ing. > > > > > > I totally disagree. `git branch` is *the* command to work with branches. > > > Yes, you can manage branches. But you can also list them. And now you can > > > also compare them. > > > > One of the things I don't like about "git branch --diff" is that this > > feature is not _just_ about branches at all. E.g., I could do: > > > > git tbdiff HEAD~10 HEAD~5 foo > > > > Or even: > > > > git tbdiff v2.16.0 v2.17.0 my-rewritten-v2.17.0 > > > > Those arguments really are just commitishes, not necessarily branches. > > One of the current interface rules for "git branch" is that the branch > > names we hand it are interpreted _exactly_ as branch names. You cannot > > "git branch -m v2.16.0", and there is no ambiguity in "git branch -d > > foo" if "foo" is both a tag and a branch. > > > > But this new mode does not fit the pattern at all. > > > > If we were to attach this to an existing command, I think it has more to > > do with "diff" than "branch". But I'm not sure we want to overload > > "diff" either (which has traditionally been about two endpoints, and > > does not really traverse at all, though arguably "foo...bar" is a bit of > > a cheat :) ). > > > > > > Of the suggestions thus far, Junio's git-topic-diff seems the least > > > > worse, and doesn't suffer from tab-completion problems. > > > > > > Except that this is too limited a view. > > > > Right, I agree with you. Topic branches are the intended use, but that's > > not what it _does_, and obviously it can be applied in other cases. So > > since "branch" is too specific, I think "topic branch" is even more so. > > > > It's really "diff-history" or something, I think. That's not very > > catchy, but I think the best name would imply that it was diffing a set > > of commits (so even "diff-commit" would not be right, because that again > > sounds like endpoints). > > This is exactly what I feel as well, thanks for concise and > to-the-point spelling out. > > From user interface perspective, I would expect something like this > to be possible (and natural): > > (1) git diff topic-v1...topic-v2 No, we cannot. The `git diff topic-v1...topic-v2` invocation has worked for a long time, and does something very different. We should not even allow ourselves to think of such a breakage. > (2) git diff --branch topic-v1...topic-v2 >From my point of view, `git diff --branch` indicates that I diff *branches*. Which is not really something that makes sense, and definitely not what this command is about. We are not comparing branches. We are comparing versions of the same branch. > (1) is what we are all familiar with, providing a diff between two > revisions with focus on file changes, where (2) shifts focus to > history changes. > > It`s all still a comparison between two revisions (pointed to by > "topic-v1" and "topic-v2" branch heads in this specific example), but > it differs in what we are comparing - (1) set of files contained in > endpoints, or (2) set of revisions contained in (or "leading to") > endpoints. It is very much not about comparing *two* revisions. It is very much about comparing two *ranges of* revisions, and not just any ranges, no. Those ranges need to be so related as to contain mostly identical changes. Otherwise, `git branch --diff` will spend a ton of time, just to come back with a series of `-` lines followed by a series of `+` lines (figuratively, not literally). Which would be stupid, to spend that much time on something that `git rev-list --left-right topic1...topic2` would have computed a lot faster. > Hmm... what about `git diff --history`? :/ It does seem more "true" > to what it does, though I still like `git diff --branch` more > (catchier, indeed). It certainly is catchier. But also a ton more puzzling. I do not want to compare histories, after all. That would be like saying: okay, topic1 and topic2 ended up at the same stage, but *how* did they get there? What I *want* to ask via the command implemented by this patch series is the question: there was a set of patches previously, and now I have a set of revised patches, what changed? Most fellow German software engineers (who seem to have a knack for idiotically long variable/function names) would now probably suggest: git compare-patch-series-with-revised-patch-series I hope you agree that that is better *and* worse than your suggestions, depending from what angle you look at it: it is better because it describes what the command is *actually* doing. But it is much worse at the same time because it is too lo
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
On Mon, May 07, 2018 at 11:44:29PM -0400, Jeff King wrote: > On Mon, May 07, 2018 at 03:24:59PM -0700, Stefan Beller wrote: > > > Hence I propose "git range-diff", similar to topic-diff, that > > was proposed earlier. > > > > * it "diffs ranges" of commits. > > * it can also deal with out-of-git things like patch series, > > but that is a mere by product and may not be desired. > > Just like git-diff can also compare two files outside a git > > repo, that would not be a good use case. > > Keep the name Git-centric! > > * it autocompletes well. > > FWIW, I like this by far of all of the suggested names. I hit "send" before I had a chance to expound. ;) The thing that I really like about it is that it names the _concept_. If I were writing a manual page describing what this output is, I would call it a "range diff". And naturally, the command to generate range diffs is "git range-diff". I think "git diff --range" would also be OK, but IMHO it's useful to keep the "git diff" family as always comparing end-points. -Peff
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
On Mon, May 07, 2018 at 03:24:59PM -0700, Stefan Beller wrote: > Hence I propose "git range-diff", similar to topic-diff, that > was proposed earlier. > > * it "diffs ranges" of commits. > * it can also deal with out-of-git things like patch series, > but that is a mere by product and may not be desired. > Just like git-diff can also compare two files outside a git > repo, that would not be a good use case. > Keep the name Git-centric! > * it autocompletes well. FWIW, I like this by far of all of the suggested names. -Peff
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Jeff King writes: > One of the things I don't like about "git branch --diff" is that this > feature is not _just_ about branches at all. I actually wouldn't be that much against the word "branch" in "branch-diff" on the ground that we are typically not feeding branches to the command (we are feeding two ranges, and one endpoint of each range typically gets expressed using branch name), as we have a precedent in "show-branch", for example, that often takes branches but does not have to. > It's really "diff-history" or something, I think. That's not very > catchy, but I think the best name would imply that it was diffing a set > of commits (so even "diff-commit" would not be right, because that again > sounds like endpoints). Sure. This should't be a submode "--diff" of "git branch" just like it shouldn't be a submode of "git commit" only because it is about comparing two sets of commits. "diff" is about comparing two endpoints, and not about comparing two sets. "log" is the closest thing, if we really want to coerce it into an existing set of commands, as it is about a set of commits, but it does not do multiple sets, let alone comparing them. "branch-diff" was just a good as "diff-history", except that both of them may irritate command line completion users. I do not think I care too much about which exact command name it gets, but I think it is a bad idea to tacked it to an existing command as a submode that does unrelated thing to what the main command does. So from that point of view, "branch-diff" and "diff-history" are equally good being a distinct command, and equally bad sharing prefix with common existing command.
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi Stefan, On 08/05/2018 00:24, Stefan Beller wrote: > > > List, rename, delete -- all these seem more as basic CRUD operations, > > where comparison is a more complex one. And not to get me wrong - I > > could see "branch diff" being part of "branch", but not really when > > "diff" already exists as a separate thing, already doing quite some > > (but still diff related, and configurable) stuff. > > If we go with "branch --diff", because it has the CRUD operations already > there for branches, I might ask for "remote --diff" to diff two remotes. ;) > (That command "remote --diff" would not make any sense, would it?) I`m not sure if this is a reply to me or in general, and whether you support what I sad, or argue against it...? Because what you`re saying was (or at least should have been) my exact point there :) > > Basically, what you (conceptually) call "two versions of the same > > branch", I simply call "two branches" (from usage standpoint). > > If I diff 2 (topic) branches, which are based on a different version > from upstream, then I see changes from commits that I don't care > about, but this tool explicitly excludes them. Instead it includes > the ordering of the commits as well as its commit messages to > the diff. Here, I was merely pointing out that you still need to provide two branch heads - which might be expected to resemble "two versions of the same topic", but they are still (just) "two branches" in Git world. > > And you may have a branch that got split, or more of them that got > > unified, so defining "previous branch version" may not be that > > straightforward - it`s really just "two commit ranges" (as man page > > defines it in general), with "two versions of a patch series" only > > being the most common/expected use case of the former. > > > > Finally, if user picks two totally unrelated "branches" to compare, > > he won`t get a really useful diff - but it`s the same as if he would > > compare two totally unrelated commits (where tree state massively > > changed in between, or having unrelated histories, even). > > I used just that, but narrowed down the comparison to one file > instead of the whole tree. Again, not sure if this should support the argument, or argue against it? :) My point was that there might be other use cases (as you seem to have supported now), and as "diff" is pretty forgiving, might be "diff branch" should be as well. > > With something like `git diff --branch ...` you > > would get yet another "diff look", useful for use case in question > > here. > > Personally I think this patch series should neither extend git-diff > nor git-branch. > > It should not extend git-diff, because currently git-diff can diff > tree-ishs (and does that very well) and comparing to > worktree/index. Hmm, are you saying that `git diff` actually has a too generic name for its (more specific) purpose? > It should also not extend git-branch, as that command is for > CRUD operations that you hinted at earlier (Earlier I proposed > git-remote --diff for diffing two remote, which makes no sense, > another one might be git-worktree, which also just does CRUD > for worktrees. It would be a bad idea to have "git worktree --diff") Agreed here. > Hence I propose "git range-diff", similar to topic-diff, that > was proposed earlier. I find it strange that we already have both "diff" and "diff-something" commands, and yet you still propose "something-diff" naming pattern instead (but I guess it`s mainly because of auto-complete concerns). Please forgive my lack of code base familiarity, but from what I`ve seen so far, and at least from end-user perspective, I may rather expect `git diff-range` as low level implementation, and possibly exposed through `git diff --range` (with a nice single letter abbreviation?). > * it "diffs ranges" of commits. Thus "diff-range", as your description says itself :) ("range-diff" might sound like it "ranges diffs"...?) > * it can also deal with out-of-git things like patch series, > but that is a mere by product and may not be desired. > Just like git-diff can also compare two files outside a git > repo, that would not be a good use case. Hmm, so still follows `git diff` in general... `git diff --range`? :D > * it autocompletes well. Only here I`m not sure if something like `git diff --range` (with accompanying single letter option) would be considered "auto-complete friendly", or not? Regards, Buga
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
On Mon, May 7, 2018 at 3:05 PM, Igor Djordjevic wrote: > List, rename, delete -- all these seem more as basic CRUD operations, > where comparison is a more complex one. And not to get me wrong - I > could see "branch diff" being part of "branch", but not really when > "diff" already exists as a separate thing, already doing quite some > (but still diff related, and configurable) stuff. If we go with "branch --diff", because it has the CRUD operations already there for branches, I might ask for "remote --diff" to diff two remotes. ;) (That command "remote --diff" would not make any sense, would it?) > Basically, what you (conceptually) call "two versions of the same > branch", I simply call "two branches" (from usage standpoint). If I diff 2 (topic) branches, which are based on a different version from upstream, then I see changes from commits that I don't care about, but this tool explicitly excludes them. Instead it includes the ordering of the commits as well as its commit messages to the diff. So I would not say this tool "diffs two branches", as that is understood as "diffing the trees, where each of the two branches points two", whereas this tool diffs a patch series, or if you give Git-ranges, then it would produce such a patch series in memory. > And you may have a branch that got split, or more of them that got > unified, so defining "previous branch version" may not be that > straightforward - it`s really just "two commit ranges" (as man page > defines it in general), with "two versions of a patch series" only > being the most common/expected use case of the former. > > Finally, if user picks two totally unrelated "branches" to compare, > he won`t get a really useful diff - but it`s the same as if he would > compare two totally unrelated commits (where tree state massively > changed in between, or having unrelated histories, even). I used just that, but narrowed down the comparison to one file instead of the whole tree. > With something like `git diff --branch ...` you > would get yet another "diff look", useful for use case in question > here. Personally I think this patch series should neither extend git-diff nor git-branch. It should not extend git-diff, because currently git-diff can diff tree-ishs (and does that very well) and comparing to worktree/index. It should also not extend git-branch, as that command is for CRUD operations that you hinted at earlier (Earlier I proposed git-remote --diff for diffing two remote, which makes no sense, another one might be git-worktree, which also just does CRUD for worktrees. It would be a bad idea to have "git worktree --diff") Hence I propose "git range-diff", similar to topic-diff, that was proposed earlier. * it "diffs ranges" of commits. * it can also deal with out-of-git things like patch series, but that is a mere by product and may not be desired. Just like git-diff can also compare two files outside a git repo, that would not be a good use case. Keep the name Git-centric! * it autocompletes well. Stefan
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi Dscho, On 07/05/2018 03:34, Johannes Schindelin wrote: > > > > I think Todd's idea to shift it from a full-blown builtin to a cmdmode > > > of `branch` makes tons of sense. > > > > I don`t know, I still find it a bit strange that in order to "diff > > something", you go to "something" and tell it to "diff itself" - not > > because it`s a weird concept (OOP, anyone? :]), but because we already > > have "diff" command that can accept different things, thus just teaching > > it to accept additional "something" (branch, in this case), seems more > > natural (to me) - "branch diff" being just another "diff" mode of > > operation. > > You also have to call `git branch` to list branches. And to rename > branches. And to delete them. So why not also compare them at the same > time? Maybe because we already have a command that specifically does comparison? :) List, rename, delete -- all these seem more as basic CRUD operations, where comparison is a more complex one. And not to get me wrong - I could see "branch diff" being part of "branch", but not really when "diff" already exists as a separate thing, already doing quite some (but still diff related, and configurable) stuff. > > What about that side thought you left out from my original message, > > making it `git diff --branch` instead? > > I really did not like this, as all of the `git diff` options really are > about comparing two revisions, not two *sets* of revisions. I see what you mean, but I would argue this being a deliberate user choice here, like picking a diff "strategy" - I`d say it still utterly does compare two revisions (branch tips, in this case), just putting focus on comparing revisions that lead to them (branch history), instead of just files found in them (branch files). > Further, if I put my unsuspecting user hat on, I would ask myself how you > can compare branches with one another? That is what I would expect `git > diff --branch` to do, not to compare two versions of *the same* branch. I totally agree with you here, and thus I have a question - what determines "two versions of *the same* branch"? :) Do you still explicitly provide both "old" and "new" version branch tips? I see "multiple versions of the same branch" more as a conceptual model, and not something Git is aware of (I think?) - BUT, even if it was, I don`t see why this should be a(n artificial) restriction? Basically, what you (conceptually) call "two versions of the same branch", I simply call "two branches" (from usage standpoint). And you may have a branch that got split, or more of them that got unified, so defining "previous branch version" may not be that straightforward - it`s really just "two commit ranges" (as man page defines it in general), with "two versions of a patch series" only being the most common/expected use case of the former. Finally, if user picks two totally unrelated "branches" to compare, he won`t get a really useful diff - but it`s the same as if he would compare two totally unrelated commits (where tree state massively changed in between, or having unrelated histories, even). Besides, while I might still not be much into the matter, but isn`t "branch" in Git just a pointer to revision? Being so, there is really no such thing as "branch" in terms of being a specific (sub)set of revisions (commits), other then "everything from branch head/pointer to root commit" (in general). Yes, we do perceive "a branch" being a specific set of topic related commits, but which *exact* commits we are interested in ("branch" lower bounds) may differ in regards to what we aim for - how far do we consider one branch to reach in the past depends solely on the use case. > So `git diff --branch` does not at all convey the same to me as `git > branch --diff`, and I find that the latter does match better what this > patch series tries to achieve. I agree with the first part, but it seems to me your finding is biased due to your (expected) use case. > > But if "branch diff" is considered to be too special-cased mode of > > "diff" so that supporting it from `diff` itself would make it feel > > awkward in both usage and maintenance (in terms of many other regular > > `diff` specific options being unsupported), I guess I would understand > > having it outside `diff` altogether (and implemented as proposed `git > > branch --diff`, or something)... for the time being, at least :) > > The branch diff is not even a special-cased mode of diff. It is *way* more > complicated than that. It tries to find 1:1 correspondences between *sets* > of commits, and then only outputs a "sort" of a diff between the commits > that correspond with each other. I say "sort" of a diff because that diff > does not look like `git diff ` at all! But there is not only one `git diff ` looks, it depends on other options (like --name-status, for example), which is my point exactly :) With something like `git diff --branch ...` you would get yet anoth
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
On 07/05/2018 09:48, Jeff King wrote: > > > > Let's, please, not fall into the trap of polluting git-branch with > > > utterly unrelated functionality, as has happened a few times with > > > other Git commands. Let's especially not do so merely for the sake of > > > tab-completion. git-branch is for branch management; it's not for > > > diff'ing. > > > > I totally disagree. `git branch` is *the* command to work with branches. > > Yes, you can manage branches. But you can also list them. And now you can > > also compare them. > > One of the things I don't like about "git branch --diff" is that this > feature is not _just_ about branches at all. E.g., I could do: > > git tbdiff HEAD~10 HEAD~5 foo > > Or even: > > git tbdiff v2.16.0 v2.17.0 my-rewritten-v2.17.0 > > Those arguments really are just commitishes, not necessarily branches. > One of the current interface rules for "git branch" is that the branch > names we hand it are interpreted _exactly_ as branch names. You cannot > "git branch -m v2.16.0", and there is no ambiguity in "git branch -d > foo" if "foo" is both a tag and a branch. > > But this new mode does not fit the pattern at all. > > If we were to attach this to an existing command, I think it has more to > do with "diff" than "branch". But I'm not sure we want to overload > "diff" either (which has traditionally been about two endpoints, and > does not really traverse at all, though arguably "foo...bar" is a bit of > a cheat :) ). > > > > Of the suggestions thus far, Junio's git-topic-diff seems the least > > > worse, and doesn't suffer from tab-completion problems. > > > > Except that this is too limited a view. > > Right, I agree with you. Topic branches are the intended use, but that's > not what it _does_, and obviously it can be applied in other cases. So > since "branch" is too specific, I think "topic branch" is even more so. > > It's really "diff-history" or something, I think. That's not very > catchy, but I think the best name would imply that it was diffing a set > of commits (so even "diff-commit" would not be right, because that again > sounds like endpoints). This is exactly what I feel as well, thanks for concise and to-the-point spelling out. >From user interface perspective, I would expect something like this to be possible (and natural): (1) git diff topic-v1...topic-v2 (2) git diff --branch topic-v1...topic-v2 (1) is what we are all familiar with, providing a diff between two revisions with focus on file changes, where (2) shifts focus to history changes. It`s all still a comparison between two revisions (pointed to by "topic-v1" and "topic-v2" branch heads in this specific example), but it differs in what we are comparing - (1) set of files contained in endpoints, or (2) set of revisions contained in (or "leading to") endpoints. Hmm... what about `git diff --history`? :/ It does seem more "true" to what it does, though I still like `git diff --branch` more (catchier, indeed). Regards, Buga
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
On Mon, May 7, 2018 at 8:28 AM, Duy Nguyen wrote: >>> I do hear you. Especially since I hate `git cherry` every single time that >>> I try to tab-complete `git cherry-pick`. >> >> Me too. :) > > Just so you know I'm also not happy with that "git cherry". Since I'm > updating git-completion.bash in this area and we got 3 "me too" votes > (four if we count Szeder in another thread), I'm going to implementing > something to at least let you exclude "cherry" from the completion > list if you want. And another "me too" here.
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
On Mon, May 7, 2018 at 9:50 AM, Jeff King wrote: > On Sat, May 05, 2018 at 11:57:26PM +0200, Johannes Schindelin wrote: > >> > It feels really petty complaining about the name, but I just want to >> > raise the point, since it will never be easier to change than right now. >> >> I do hear you. Especially since I hate `git cherry` every single time that >> I try to tab-complete `git cherry-pick`. > > Me too. :) Just so you know I'm also not happy with that "git cherry". Since I'm updating git-completion.bash in this area and we got 3 "me too" votes (four if we count Szeder in another thread), I'm going to implementing something to at least let you exclude "cherry" from the completion list if you want. -- Duy
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Johannes Schindelin writes: > It would be easy to introduce, but I am wary about its usefulness. > Unless you re-generate the branch from patches (which I guess you do a > lot, but I don't), you are likely to compare incomplete patch series: say, > when you call `git rebase -i` to reword 05/18's commit message, your > command will only compare 05--18 of the patch series. Well that is exactly the point of that "..@{1} @{1}..", which turned out to be very useful in practice at least for me when I am updating a topic with "rebase -i", and then reviewing what I did with tbdiff. I do not want 01-04 in the above case as I already know I did not touch them.
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
On Sat, May 05, 2018 at 11:57:26PM +0200, Johannes Schindelin wrote: > > It feels really petty complaining about the name, but I just want to > > raise the point, since it will never be easier to change than right now. > > I do hear you. Especially since I hate `git cherry` every single time that > I try to tab-complete `git cherry-pick`. Me too. :) I've wondered if "git pick" would be a good alias for cherry-pick (the "cherry" metaphor is probably not well understood by most users). And "revert" should just be "pick -R", but that is a whole other discussion. -Peff
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
On Sun, May 06, 2018 at 10:04:31PM -0400, Johannes Schindelin wrote: > > Let's, please, not fall into the trap of polluting git-branch with > > utterly unrelated functionality, as has happened a few times with > > other Git commands. Let's especially not do so merely for the sake of > > tab-completion. git-branch is for branch management; it's not for > > diff'ing. > > I totally disagree. `git branch` is *the* command to work with branches. > Yes, you can manage branches. But you can also list them. And now you can > also compare them. One of the things I don't like about "git branch --diff" is that this feature is not _just_ about branches at all. E.g., I could do: git tbdiff HEAD~10 HEAD~5 foo Or even: git tbdiff v2.16.0 v2.17.0 my-rewritten-v2.17.0 Those arguments really are just commitishes, not necessarily branches. One of the current interface rules for "git branch" is that the branch names we hand it are interpreted _exactly_ as branch names. You cannot "git branch -m v2.16.0", and there is no ambiguity in "git branch -d foo" if "foo" is both a tag and a branch. But this new mode does not fit the pattern at all. If we were to attach this to an existing command, I think it has more to do with "diff" than "branch". But I'm not sure we want to overload "diff" either (which has traditionally been about two endpoints, and does not really traverse at all, though arguably "foo...bar" is a bit of a cheat :) ). > > Of the suggestions thus far, Junio's git-topic-diff seems the least > > worse, and doesn't suffer from tab-completion problems. > > Except that this is too limited a view. Right, I agree with you. Topic branches are the intended use, but that's not what it _does_, and obviously it can be applied in other cases. So since "branch" is too specific, I think "topic branch" is even more so. It's really "diff-history" or something, I think. That's not very catchy, but I think the best name would imply that it was diffing a set of commits (so even "diff-commit" would not be right, because that again sounds like endpoints). -Peff
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi Junio, On Mon, 7 May 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > >> If tbdiff were "Thomas's branch diff", I would call this jbdiff ;-) > >> but I think the 't' in there stands for "topic", not "Thomas's". > >> > >> How about "git topic-diff"? > > > > Or `git topic-branch-diff`? > > Yeah something along that line, which is about comparing each step > in two iterations of a single topic. It would be wonderful if it > also supported a short-hand > > $ git tbdiff --reflog 1.day.ago js/branch-diff > > that turned into: > > $ git tbdiff js/branch-diff..js/branch-diff@{1.day.ago} \ > js/branch-diff@{1.day.ago}..js/branch-diff Or even easier: `git tbdiff js/branch-diff@{1.day.ago}...js/branch-diff`. > That compares "what was on the topic a day ago" with "what is new on > the topic since that time", which is exactly what an individual > contributor wants when reviewing how the topic was polished, I would > say. It would be easy to introduce, but I am wary about its usefulness. Unless you re-generate the branch from patches (which I guess you do a lot, but I don't), you are likely to compare incomplete patch series: say, when you call `git rebase -i` to reword 05/18's commit message, your command will only compare 05--18 of the patch series. Worse, if js/branch-diff needs to be uprooted (e.g. because it now depends on some different patch, or because it already depended on a separate patch series that was now updated), your `git branch --diff` call will compare more than just my patches: it will assume that those dependencies are part of the patch series, because they changed, too. > [Footnote] > > A variant I often use when accepting a rerolled series is > > $ git checkout js/branch-diff > $ git checkout master... > $ git am ./+js-branch-diff-v2 > $ git tbdiff ..@{-1} @{-1}.. > > so this is not only for individual contributors but also helps > integrators. Yes, and I also pointed out (twice) that it will help interested parties follow what I do with my merging-rebases in Git for Windows. Ciao, Dscho
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi Eric, On Sun, 6 May 2018, Eric Sunshine wrote: > On Sun, May 6, 2018 at 8:21 AM, Johannes Schindelin > wrote: > > On Sun, 6 May 2018, Junio C Hamano wrote: > >> Johannes Schindelin writes: > >> > On Sat, 5 May 2018, Jeff King wrote: > >> >> One minor point about the name: will it become annoying as a tab > >> >> completion conflict with git-branch? > >> > >> If tbdiff were "Thomas's branch diff", I would call this jbdiff ;-) > >> but I think the 't' in there stands for "topic", not "Thomas's". > >> How about "git topic-diff"? > > > > Or `git topic-branch-diff`? > > > > But then, we do not really use the term `topic branch` a lot in Git, *and* > > the operation in question is not really about showing differences between > > topic branches, but between revisions of topic branches. > > > > So far, the solution I like best is to use `git branch --diff <...>`, > > which also neatly side-steps the problem of cluttering the top-level > > command list (because tab completion). > > Let's, please, not fall into the trap of polluting git-branch with > utterly unrelated functionality, as has happened a few times with > other Git commands. Let's especially not do so merely for the sake of > tab-completion. git-branch is for branch management; it's not for > diff'ing. I totally disagree. `git branch` is *the* command to work with branches. Yes, you can manage branches. But you can also list them. And now you can also compare them. > Of the suggestions thus far, Junio's git-topic-diff seems the least > worse, and doesn't suffer from tab-completion problems. Except that this is too limited a view. Have you seen one of the more important tidbits in the cover letter, the one about Git for Windows' *branch thicket*? In this case, it is not *one* topic branch that we are talking about. And even worse: what this patch series introduces is not at all a feature to compare topic branches! Instead, it is a way to compare iterations of patch series, versions of topic branches, changes introduced into a topic branch by rebasing it, etc. And `git topic-diff` simply does not say this. It says something different, something that my patches cannot fulfill. > Building on Duy's suggestion: git-interdiff could be a superset of the > current git-branch-diff: > > # standard interdiff > git interdiff womp-v1 womp-v2 > # 'tbdiff'-like output > git interdiff --topic womp-v1 womp-v2 No, no, and no. An interdiff is an interdiff is an interdiff. See e.g. https://www.tutorialspoint.com/unix_commands/interdiff.htm for details. The operation introduced by this patch series, or for that matter tbdiff, *never ever* produced an interdiff. Get this "interdiff" label out of your mind immediately when you think about this here operation. One of my commit messages even talks about this, and says *why* we do not generate interdiffs: they are in general not even well-defined. Take my --rebase-merges patch series, for example. It is so long-running that at some stages, all I did was to resolve merge conflicts incurred from rebasing to `master`. That was literally all. Now, if you tried to produce an interdiff, you would *already fail in the first step*, as the previous overall diff does not apply in reverse on current `master`. Out of all the options so far, the one that I liked was `git branch --diff`. Seriously. I do not understand why you think that this is abusing the `git branch` command. It is no less abusing it than `git branch --edit-description`! And that is a *very good* command, and it is *very good* that it is an option to `git branch`. It makes a total lot of sense, I have never had to think "wait, in which Git command is this implemented already?" And I would expect the exact same thing to happen with `git branch --diff`. Ciao, Johannes
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Johannes Schindelin writes: >> If tbdiff were "Thomas's branch diff", I would call this jbdiff ;-) >> but I think the 't' in there stands for "topic", not "Thomas's". >> >> How about "git topic-diff"? > > Or `git topic-branch-diff`? Yeah something along that line, which is about comparing each step in two iterations of a single topic. It would be wonderful if it also supported a short-hand $ git tbdiff --reflog 1.day.ago js/branch-diff that turned into: $ git tbdiff js/branch-diff..js/branch-diff@{1.day.ago} \ js/branch-diff@{1.day.ago}..js/branch-diff That compares "what was on the topic a day ago" with "what is new on the topic since that time", which is exactly what an individual contributor wants when reviewing how the topic was polished, I would say. [Footnote] A variant I often use when accepting a rerolled series is $ git checkout js/branch-diff $ git checkout master... $ git am ./+js-branch-diff-v2 $ git tbdiff ..@{-1} @{-1}.. so this is not only for individual contributors but also helps integrators.
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi Buga, On Sun, 6 May 2018, Igor Djordjevic wrote: > On 06/05/2018 14:10, Johannes Schindelin wrote: > > > I think Todd's idea to shift it from a full-blown builtin to a cmdmode > > of `branch` makes tons of sense. > > I don`t know, I still find it a bit strange that in order to "diff > something", you go to "something" and tell it to "diff itself" - not > because it`s a weird concept (OOP, anyone? :]), but because we already > have "diff" command that can accept different things, thus just teaching > it to accept additional "something" (branch, in this case), seems more > natural (to me) - "branch diff" being just another "diff" mode of > operation. You also have to call `git branch` to list branches. And to rename branches. And to delete them. So why not also compare them at the same time? > What about that side thought you left out from my original message, > making it `git diff --branch` instead? I really did not like this, as all of the `git diff` options really are about comparing two revisions, not two *sets* of revisions. Further, if I put my unsuspecting user hat on, I would ask myself how you can compare branches with one another? That is what I would expect `git diff --branch` to do, not to compare two versions of *the same* branch. So `git diff --branch` does not at all convey the same to me as `git branch --diff`, and I find that the latter does match better what this patch series tries to achieve. I briefly considered `git branch --compare` instead, but then rejected it: it would again sound more like I try to compare two separate (and likely unrelated) branches with one another, and that simply does not make much sense, and tbdiff would not help with that, anyway. > But if "branch diff" is considered to be too special-cased mode of > "diff" so that supporting it from `diff` itself would make it feel > awkward in both usage and maintenance (in terms of many other regular > `diff` specific options being unsupported), I guess I would understand > having it outside `diff` altogether (and implemented as proposed `git > branch --diff`, or something)... for the time being, at least :) The branch diff is not even a special-cased mode of diff. It is *way* more complicated than that. It tries to find 1:1 correspondences between *sets* of commits, and then only outputs a "sort" of a diff between the commits that correspond with each other. I say "sort" of a diff because that diff does not look like `git diff ` at all! So I think it would just be confusing to add that mode to `git diff`. Ciao, Dscho
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
On Sun, May 6, 2018 at 8:21 AM, Johannes Schindelin wrote: > On Sun, 6 May 2018, Junio C Hamano wrote: >> Johannes Schindelin writes: >> > On Sat, 5 May 2018, Jeff King wrote: >> >> One minor point about the name: will it become annoying as a tab >> >> completion conflict with git-branch? >> >> If tbdiff were "Thomas's branch diff", I would call this jbdiff ;-) >> but I think the 't' in there stands for "topic", not "Thomas's". >> How about "git topic-diff"? > > Or `git topic-branch-diff`? > > But then, we do not really use the term `topic branch` a lot in Git, *and* > the operation in question is not really about showing differences between > topic branches, but between revisions of topic branches. > > So far, the solution I like best is to use `git branch --diff <...>`, > which also neatly side-steps the problem of cluttering the top-level > command list (because tab completion). Let's, please, not fall into the trap of polluting git-branch with utterly unrelated functionality, as has happened a few times with other Git commands. Let's especially not do so merely for the sake of tab-completion. git-branch is for branch management; it's not for diff'ing. Of the suggestions thus far, Junio's git-topic-diff seems the least worse, and doesn't suffer from tab-completion problems. Building on Duy's suggestion: git-interdiff could be a superset of the current git-branch-diff: # standard interdiff git interdiff womp-v1 womp-v2 # 'tbdiff'-like output git interdiff --topic womp-v1 womp-v2 (Substitute "--topic" by any other better name.)
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi Dscho, On 06/05/2018 14:10, Johannes Schindelin wrote: > > > > > > This builtin does not do a whole lot so far, apart from showing a > > > > > usage that is oddly similar to that of `git tbdiff`. And for a > > > > > good reason: the next commits will turn `branch-diff` into a > > > > > full-blown replacement for `tbdiff`. > > > > > > > > One minor point about the name: will it become annoying as a tab > > > > completion conflict with git-branch? > > > > > > I did mention this in the commit message of 18/18: > > > > > > Without this patch, we would only complete the `branch-diff` part but > > > not the options and other arguments. > > > > > > This of itself may already be slightly disruptive for well-trained > > > fingers that assume that `git braorimas` would expand > > > to > > > `git branch origin/master`, as we now no longer automatically append a > > > space after completing `git branch`: this is now ambiguous. > > > > > > > It feels really petty complaining about the name, but I just want > > > > to raise the point, since it will never be easier to change than > > > > right now. > > > > > > I do hear you. Especially since I hate `git cherry` every single > > > time that I try to tab-complete `git cherry-pick`. > > > > > > > (And no, I don't really have another name in mind; I'm just > > > > wondering if "subset" names like this might be a mild annoyance in > > > > the long run). > > > > > > They totally are, and if you can come up with a better name, I am > > > really interested in changing it before this hits `next`, even. > > > > I gave this just a quick glance so might be I`m missing something > > obvious or otherwise well-known here, bur why not `diff-branch` instead? > > I think that is just turning the problem from `branch` to `diff`. > > Of course, we have precedent with diff-index and diff-files. Except that > they don't auto-complete (because they are low-level commands) and I > *would* like the subcommand discussed in this here patch series to > auto-complete. Yeah, I did suspect it might be something like this (those other ones not auto-completing, where we do want it here), thanks for elaborating. > I think Todd's idea to shift it from a full-blown builtin to a cmdmode > of `branch` makes tons of sense. I don`t know, I still find it a bit strange that in order to "diff something", you go to "something" and tell it to "diff itself" - not because it`s a weird concept (OOP, anyone? :]), but because we already have "diff" command that can accept different things, thus just teaching it to accept additional "something" (branch, in this case), seems more natural (to me) - "branch diff" being just another "diff" mode of operation. What about that side thought you left out from my original message, making it `git diff --branch` instead? But if "branch diff" is considered to be too special-cased mode of "diff" so that supporting it from `diff` itself would make it feel awkward in both usage and maintenance (in terms of many other regular `diff` specific options being unsupported), I guess I would understand having it outside `diff` altogether (and implemented as proposed `git branch --diff`, or something)... for the time being, at least :) Regards, Buga
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi Junio, On Sun, 6 May 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > > On Sat, 5 May 2018, Jeff King wrote: > > > >> On Fri, May 04, 2018 at 05:34:32PM +0200, Johannes Schindelin wrote: > >> > >> > This builtin does not do a whole lot so far, apart from showing a usage > >> > that is oddly similar to that of `git tbdiff`. And for a good reason: > >> > the next commits will turn `branch-diff` into a full-blown replacement > >> > for `tbdiff`. > >> > >> One minor point about the name: will it become annoying as a tab > >> completion conflict with git-branch? > > If tbdiff were "Thomas's branch diff", I would call this jbdiff ;-) > but I think the 't' in there stands for "topic", not "Thomas's". > > How about "git topic-diff"? Or `git topic-branch-diff`? But then, we do not really use the term `topic branch` a lot in Git, *and* the operation in question is not really about showing differences between topic branches, but between revisions of topic branches. So far, the solution I like best is to use `git branch --diff <...>`, which also neatly side-steps the problem of cluttering the top-level command list (because tab completion). Ciao, Dscho
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi Buga, On Sun, 6 May 2018, Igor Djordjevic wrote: > On 05/05/2018 23:57, Johannes Schindelin wrote: > > > > > > This builtin does not do a whole lot so far, apart from showing a > > > > usage that is oddly similar to that of `git tbdiff`. And for a > > > > good reason: the next commits will turn `branch-diff` into a > > > > full-blown replacement for `tbdiff`. > > > > > > One minor point about the name: will it become annoying as a tab > > > completion conflict with git-branch? > > > > I did mention this in the commit message of 18/18: > > > > Without this patch, we would only complete the `branch-diff` part but > > not the options and other arguments. > > > > This of itself may already be slightly disruptive for well-trained > > fingers that assume that `git braorimas` would expand to > > `git branch origin/master`, as we now no longer automatically append a > > space after completing `git branch`: this is now ambiguous. > > > > > It feels really petty complaining about the name, but I just want > > > to raise the point, since it will never be easier to change than > > > right now. > > > > I do hear you. Especially since I hate `git cherry` every single > > time that I try to tab-complete `git cherry-pick`. > > > > > (And no, I don't really have another name in mind; I'm just > > > wondering if "subset" names like this might be a mild annoyance in > > > the long run). > > > > They totally are, and if you can come up with a better name, I am > > really interested in changing it before this hits `next`, even. > > I gave this just a quick glance so might be I`m missing something > obvious or otherwise well-known here, bur why not `diff-branch` instead? I think that is just turning the problem from `branch` to `diff`. Of course, we have precedent with diff-index and diff-files. Except that they don't auto-complete (because they are low-level commands) and I *would* like the subcommand discussed in this here patch series to auto-complete. I think Todd's idea to shift it from a full-blown builtin to a cmdmode of `branch` makes tons of sense. Ciao, Dscho
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi Duy, On Sun, 6 May 2018, Duy Nguyen wrote: > On Sun, May 6, 2018 at 6:53 AM, Jacob Keller wrote: > > On Sat, May 5, 2018 at 6:05 PM, Igor Djordjevic > > wrote: > >> > >> On 05/05/2018 23:57, Johannes Schindelin wrote: > >>> > >>> > > This builtin does not do a whole lot so far, apart from showing a > >>> > > usage that is oddly similar to that of `git tbdiff`. And for a > >>> > > good reason: the next commits will turn `branch-diff` into a > >>> > > full-blown replacement for `tbdiff`. > >>> > > >>> > One minor point about the name: will it become annoying as a tab > >>> > completion conflict with git-branch? > >>> > >>> I did mention this in the commit message of 18/18: > >>> > >>> Without this patch, we would only complete the `branch-diff` part but > >>> not the options and other arguments. > >>> > >>> This of itself may already be slightly disruptive for well-trained > >>> fingers that assume that `git braorimas` would expand > >>> to > >>> `git branch origin/master`, as we now no longer automatically append a > >>> space after completing `git branch`: this is now ambiguous. > >>> > >>> > It feels really petty complaining about the name, but I just want > >>> > to raise the point, since it will never be easier to change than > >>> > right now. > >>> > >>> I do hear you. Especially since I hate `git cherry` every single > >>> time that I try to tab-complete `git cherry-pick`. > >>> > >>> > (And no, I don't really have another name in mind; I'm just > >>> > wondering if "subset" names like this might be a mild annoyance in > >>> > the long run). > >>> > >>> They totally are, and if you can come up with a better name, I am > >>> really interested in changing it before this hits `next`, even. > >> > >> I gave this just a quick glance so might be I`m missing something > >> obvious or otherwise well-known here, bur why not `diff-branch` instead? > >> > >> From user interface perspective, I would (personally) rather expect a > >> command that does "diff of branches" to belong to "diff family" of > >> commands (just operating on branches, instead of "branch" command > >> knowing to "diff itself"), and I see we already have `diff-files`, > >> `diff-index` and `diff-tree`, for what that`s worth. > >> > >> Heck, I might even expect something like `git diff --branch ...` to work, > >> but I guess that is yet a different matter :) > >> > >> Thanks, Buga > > > > I like diff-branch, though I suppose that also conflicts with diff too. > > How about interdiff? No. An interdiff is well defined as the diff you would get by first applying the first of two patches in reverse and then the second patch forward. In other words, it turns two revisions of a patch into the diff between the result of applying both revisions. I tried very hard to avoid using that term in my patch series (tbdiff used the term incorrectly: what it called an interdiff is a diff of two patches, where a patch is an author line followed by the commit message followed by the commit diff). Ciao, Dscho
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi Todd, On Sat, 5 May 2018, Todd Zullinger wrote: > I wrote: > > Would it be possible and reasonable to teach 'git branch' to > > call this as a subcommand, i.e. as 'git branch diff'? Then > > the completion wouldn't offer git branch-diff. > > Of course right after I sent this, it occurred to me that > 'git branch diff' would make mask the ability to create a > branch named diff. Using 'git branch --diff ...' wouldn't > suffer that problem. Yep, I immediately thought of --diff instead of diff when I read your previous mail on that matter. And I like this idea! Of course, it will complicate the code to set up the pager a bit (for `branch-diff`, I could default to "on" all the time). But IIRC we recently changed the --list cmdmode to set the pager to "auto", so I'll just copy that. > It does add a bit more overhead to the 'git branch' command, > in terms of documentation and usage. I'm not sure it's too > much though. The git-branch summary wouldn't change much: > > -git-branch - List, create, or delete branches > +git-branch - List, create, delete, or diff branches Indeed. Unless I hear objections, I will work on moving to `git branch --diff` (it might take a while, though, I will be traveling for work this week). Ciao, Johannes
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
On Sun, May 6, 2018 at 6:53 AM, Jacob Keller wrote: > On Sat, May 5, 2018 at 6:05 PM, Igor Djordjevic > wrote: >> Hi Dscho, >> >> On 05/05/2018 23:57, Johannes Schindelin wrote: >>> >>> > > This builtin does not do a whole lot so far, apart from showing a >>> > > usage that is oddly similar to that of `git tbdiff`. And for a >>> > > good reason: the next commits will turn `branch-diff` into a >>> > > full-blown replacement for `tbdiff`. >>> > >>> > One minor point about the name: will it become annoying as a tab >>> > completion conflict with git-branch? >>> >>> I did mention this in the commit message of 18/18: >>> >>> Without this patch, we would only complete the `branch-diff` part but >>> not the options and other arguments. >>> >>> This of itself may already be slightly disruptive for well-trained >>> fingers that assume that `git braorimas` would expand to >>> `git branch origin/master`, as we now no longer automatically append a >>> space after completing `git branch`: this is now ambiguous. >>> >>> > It feels really petty complaining about the name, but I just want >>> > to raise the point, since it will never be easier to change than >>> > right now. >>> >>> I do hear you. Especially since I hate `git cherry` every single >>> time that I try to tab-complete `git cherry-pick`. >>> >>> > (And no, I don't really have another name in mind; I'm just >>> > wondering if "subset" names like this might be a mild annoyance in >>> > the long run). >>> >>> They totally are, and if you can come up with a better name, I am >>> really interested in changing it before this hits `next`, even. >> >> I gave this just a quick glance so might be I`m missing something >> obvious or otherwise well-known here, bur why not `diff-branch` instead? >> >> From user interface perspective, I would (personally) rather expect a >> command that does "diff of branches" to belong to "diff family" of >> commands (just operating on branches, instead of "branch" command >> knowing to "diff itself"), and I see we already have `diff-files`, >> `diff-index` and `diff-tree`, for what that`s worth. >> >> Heck, I might even expect something like `git diff --branch ...` to work, >> but I guess that is yet a different matter :) >> >> Thanks, Buga > > I like diff-branch, though I suppose that also conflicts with diff too. How about interdiff? -- Duy
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
On Sat, May 5, 2018 at 6:05 PM, Igor Djordjevic wrote: > Hi Dscho, > > On 05/05/2018 23:57, Johannes Schindelin wrote: >> >> > > This builtin does not do a whole lot so far, apart from showing a >> > > usage that is oddly similar to that of `git tbdiff`. And for a >> > > good reason: the next commits will turn `branch-diff` into a >> > > full-blown replacement for `tbdiff`. >> > >> > One minor point about the name: will it become annoying as a tab >> > completion conflict with git-branch? >> >> I did mention this in the commit message of 18/18: >> >> Without this patch, we would only complete the `branch-diff` part but >> not the options and other arguments. >> >> This of itself may already be slightly disruptive for well-trained >> fingers that assume that `git braorimas` would expand to >> `git branch origin/master`, as we now no longer automatically append a >> space after completing `git branch`: this is now ambiguous. >> >> > It feels really petty complaining about the name, but I just want >> > to raise the point, since it will never be easier to change than >> > right now. >> >> I do hear you. Especially since I hate `git cherry` every single >> time that I try to tab-complete `git cherry-pick`. >> >> > (And no, I don't really have another name in mind; I'm just >> > wondering if "subset" names like this might be a mild annoyance in >> > the long run). >> >> They totally are, and if you can come up with a better name, I am >> really interested in changing it before this hits `next`, even. > > I gave this just a quick glance so might be I`m missing something > obvious or otherwise well-known here, bur why not `diff-branch` instead? > > From user interface perspective, I would (personally) rather expect a > command that does "diff of branches" to belong to "diff family" of > commands (just operating on branches, instead of "branch" command > knowing to "diff itself"), and I see we already have `diff-files`, > `diff-index` and `diff-tree`, for what that`s worth. > > Heck, I might even expect something like `git diff --branch ...` to work, > but I guess that is yet a different matter :) > > Thanks, Buga I like diff-branch, though I suppose that also conflicts with diff too. Thanks, Jake
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Johannes Schindelin writes: > Hi Peff, > > On Sat, 5 May 2018, Jeff King wrote: > >> On Fri, May 04, 2018 at 05:34:32PM +0200, Johannes Schindelin wrote: >> >> > This builtin does not do a whole lot so far, apart from showing a usage >> > that is oddly similar to that of `git tbdiff`. And for a good reason: >> > the next commits will turn `branch-diff` into a full-blown replacement >> > for `tbdiff`. >> >> One minor point about the name: will it become annoying as a tab >> completion conflict with git-branch? If tbdiff were "Thomas's branch diff", I would call this jbdiff ;-) but I think the 't' in there stands for "topic", not "Thomas's". How about "git topic-diff"?
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi Dscho, On 05/05/2018 23:57, Johannes Schindelin wrote: > > > > This builtin does not do a whole lot so far, apart from showing a > > > usage that is oddly similar to that of `git tbdiff`. And for a > > > good reason: the next commits will turn `branch-diff` into a > > > full-blown replacement for `tbdiff`. > > > > One minor point about the name: will it become annoying as a tab > > completion conflict with git-branch? > > I did mention this in the commit message of 18/18: > > Without this patch, we would only complete the `branch-diff` part but > not the options and other arguments. > > This of itself may already be slightly disruptive for well-trained > fingers that assume that `git braorimas` would expand to > `git branch origin/master`, as we now no longer automatically append a > space after completing `git branch`: this is now ambiguous. > > > It feels really petty complaining about the name, but I just want > > to raise the point, since it will never be easier to change than > > right now. > > I do hear you. Especially since I hate `git cherry` every single > time that I try to tab-complete `git cherry-pick`. > > > (And no, I don't really have another name in mind; I'm just > > wondering if "subset" names like this might be a mild annoyance in > > the long run). > > They totally are, and if you can come up with a better name, I am > really interested in changing it before this hits `next`, even. I gave this just a quick glance so might be I`m missing something obvious or otherwise well-known here, bur why not `diff-branch` instead? >From user interface perspective, I would (personally) rather expect a command that does "diff of branches" to belong to "diff family" of commands (just operating on branches, instead of "branch" command knowing to "diff itself"), and I see we already have `diff-files`, `diff-index` and `diff-tree`, for what that`s worth. Heck, I might even expect something like `git diff --branch ...` to work, but I guess that is yet a different matter :) Thanks, Buga
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
I wrote: > Would it be possible and reasonable to teach 'git branch' to > call this as a subcommand, i.e. as 'git branch diff'? Then > the completion wouldn't offer git branch-diff. Of course right after I sent this, it occurred to me that 'git branch diff' would make mask the ability to create a branch named diff. Using 'git branch --diff ...' wouldn't suffer that problem. It does add a bit more overhead to the 'git branch' command, in terms of documentation and usage. I'm not sure it's too much though. The git-branch summary wouldn't change much: -git-branch - List, create, or delete branches +git-branch - List, create, delete, or diff branches I hesitate to hit send again, in case I'm once again overlooking a glaringly obvious problem with this idea. ;) -- Todd ~~ Quick to judge, quick to anger, slow to understand. Ignorance and prejudice and fear walk hand in hand.
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi Johannes, Johannes Schindelin wrote: > On Sat, 5 May 2018, Jeff King wrote: >> One minor point about the name: will it become annoying as a tab >> completion conflict with git-branch? > > I did mention this in the commit message of 18/18: > > Without this patch, we would only complete the `branch-diff` part but > not the options and other arguments. > > This of itself may already be slightly disruptive for well-trained > fingers that assume that `git braorimas` would expand to > `git branch origin/master`, as we now no longer automatically append a > space after completing `git branch`: this is now ambiguous. > >> It feels really petty complaining about the name, but I just want to >> raise the point, since it will never be easier to change than right now. > > I do hear you. Especially since I hate `git cherry` every single time that > I try to tab-complete `git cherry-pick`. > >> (And no, I don't really have another name in mind; I'm just wondering if >> "subset" names like this might be a mild annoyance in the long run). > > They totally are, and if you can come up with a better name, I am really > interested in changing it before this hits `next`, even. Would it be possible and reasonable to teach 'git branch' to call this as a subcommand, i.e. as 'git branch diff'? Then the completion wouldn't offer git branch-diff. Users could still call it directly if they wanted, though I'd tend to think that should be discouraged and have it treated as an implementation detail that it's a separate binary. We have a number of commands which take subcommands this way (bundle, bisect, notes, submodule, and stash come to mind). I don't know if any are used with and without a subcommand, but it doesn't seem too strange from a UI point of view, to me. (I don't know if it's coincidental that of the existing commands I noted above, 3 of the 5 are currently implemented as shell scripts. But they've all seen at least some work toward converting them to C, I believe). The idea might be gross and/or unreasonable from an implementation or UI view. I'm not sure, but I thought I would toss the idea out. This wouldn't work for git cherry{,-pick} where you wouldn't consider 'git cherry pick' as related to 'git cherry' though. We also have this with git show{,-branch} and some others. It's a mild annoyance, but muscle memory adapts eventually. -- Todd ~~ A budget is just a method of worrying before you spend money, as well as afterward.
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi Peff, On Sat, 5 May 2018, Jeff King wrote: > On Fri, May 04, 2018 at 05:34:32PM +0200, Johannes Schindelin wrote: > > > This builtin does not do a whole lot so far, apart from showing a usage > > that is oddly similar to that of `git tbdiff`. And for a good reason: > > the next commits will turn `branch-diff` into a full-blown replacement > > for `tbdiff`. > > One minor point about the name: will it become annoying as a tab > completion conflict with git-branch? I did mention this in the commit message of 18/18: Without this patch, we would only complete the `branch-diff` part but not the options and other arguments. This of itself may already be slightly disruptive for well-trained fingers that assume that `git braorimas` would expand to `git branch origin/master`, as we now no longer automatically append a space after completing `git branch`: this is now ambiguous. > It feels really petty complaining about the name, but I just want to > raise the point, since it will never be easier to change than right now. I do hear you. Especially since I hate `git cherry` every single time that I try to tab-complete `git cherry-pick`. > (And no, I don't really have another name in mind; I'm just wondering if > "subset" names like this might be a mild annoyance in the long run). They totally are, and if you can come up with a better name, I am really interested in changing it before this hits `next`, even. Ciao, Dscho
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
On Fri, May 04, 2018 at 05:34:32PM +0200, Johannes Schindelin wrote: > This builtin does not do a whole lot so far, apart from showing a usage > that is oddly similar to that of `git tbdiff`. And for a good reason: > the next commits will turn `branch-diff` into a full-blown replacement > for `tbdiff`. One minor point about the name: will it become annoying as a tab completion conflict with git-branch? It feels really petty complaining about the name, but I just want to raise the point, since it will never be easier to change than right now. (And no, I don't really have another name in mind; I'm just wondering if "subset" names like this might be a mild annoyance in the long run). -Peff
[PATCH v2 02/18] Add a new builtin: branch-diff
This builtin does not do a whole lot so far, apart from showing a usage that is oddly similar to that of `git tbdiff`. And for a good reason: the next commits will turn `branch-diff` into a full-blown replacement for `tbdiff`. At this point, we ignore tbdiff's color options as well as the --no-patches option, as they will all be implemented later using diff_options. Signed-off-by: Johannes Schindelin --- .gitignore| 1 + Makefile | 1 + builtin.h | 1 + builtin/branch-diff.c | 38 ++ command-list.txt | 1 + git.c | 1 + 6 files changed, 43 insertions(+) create mode 100644 builtin/branch-diff.c diff --git a/.gitignore b/.gitignore index 833ef3b0b78..1346a64492f 100644 --- a/.gitignore +++ b/.gitignore @@ -20,6 +20,7 @@ /git-bisect--helper /git-blame /git-branch +/git-branch-diff /git-bundle /git-cat-file /git-check-attr diff --git a/Makefile b/Makefile index 96f2e76a904..9b1984776d8 100644 --- a/Makefile +++ b/Makefile @@ -953,6 +953,7 @@ BUILTIN_OBJS += builtin/archive.o BUILTIN_OBJS += builtin/bisect--helper.o BUILTIN_OBJS += builtin/blame.o BUILTIN_OBJS += builtin/branch.o +BUILTIN_OBJS += builtin/branch-diff.o BUILTIN_OBJS += builtin/bundle.o BUILTIN_OBJS += builtin/cat-file.o BUILTIN_OBJS += builtin/check-attr.o diff --git a/builtin.h b/builtin.h index 42378f3aa47..e1c4d2a529a 100644 --- a/builtin.h +++ b/builtin.h @@ -135,6 +135,7 @@ extern int cmd_archive(int argc, const char **argv, const char *prefix); extern int cmd_bisect__helper(int argc, const char **argv, const char *prefix); extern int cmd_blame(int argc, const char **argv, const char *prefix); extern int cmd_branch(int argc, const char **argv, const char *prefix); +extern int cmd_branch_diff(int argc, const char **argv, const char *prefix); extern int cmd_bundle(int argc, const char **argv, const char *prefix); extern int cmd_cat_file(int argc, const char **argv, const char *prefix); extern int cmd_checkout(int argc, const char **argv, const char *prefix); diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c new file mode 100644 index 000..60a4b4fbe30 --- /dev/null +++ b/builtin/branch-diff.c @@ -0,0 +1,38 @@ +#include "cache.h" +#include "builtin.h" +#include "parse-options.h" + +static const char * const builtin_branch_diff_usage[] = { +N_("git branch-diff [] .. .."), +N_("git branch-diff [] ..."), +N_("git branch-diff [] "), +NULL +}; + +static int parse_creation_weight(const struct option *opt, const char *arg, +int unset) +{ + double *d = opt->value; + if (unset) + *d = 0.6; + else + *d = atof(arg); + return 0; +} + +int cmd_branch_diff(int argc, const char **argv, const char *prefix) +{ + double creation_weight = 0.6; + struct option options[] = { + { OPTION_CALLBACK, + 0, "creation-weight", &creation_weight, N_("factor"), + N_("Fudge factor by which creation is weighted [0.6]"), + 0, parse_creation_weight }, + OPT_END() + }; + + argc = parse_options(argc, argv, NULL, options, + builtin_branch_diff_usage, 0); + + return 0; +} diff --git a/command-list.txt b/command-list.txt index a1fad28fd82..d01b9063e81 100644 --- a/command-list.txt +++ b/command-list.txt @@ -19,6 +19,7 @@ git-archive mainporcelain git-bisect mainporcelain info git-blame ancillaryinterrogators git-branch mainporcelain history +git-branch-diff mainporcelain git-bundle mainporcelain git-cat-fileplumbinginterrogators git-check-attr purehelpers diff --git a/git.c b/git.c index f598fae7b7a..d2794fb6f5d 100644 --- a/git.c +++ b/git.c @@ -377,6 +377,7 @@ static struct cmd_struct commands[] = { { "bisect--helper", cmd_bisect__helper, RUN_SETUP }, { "blame", cmd_blame, RUN_SETUP }, { "branch", cmd_branch, RUN_SETUP | DELAY_PAGER_CONFIG }, + { "branch-diff", cmd_branch_diff, RUN_SETUP | USE_PAGER }, { "bundle", cmd_bundle, RUN_SETUP_GENTLY | NO_PARSEOPT }, { "cat-file", cmd_cat_file, RUN_SETUP }, { "check-attr", cmd_check_attr, RUN_SETUP }, -- 2.17.0.409.g71698f11835