Idea: use (magit-get-upstream-ref) as ref commit

[geza-herman]

This is just an idea/suggestion. I see that magit-todos uses (magit-main-branch) to figure out (using merge-base) the revision to compare the HEAD against.

How about using (magit-get-upstream-ref) instead? I'm suggesting this, because people might not update their main branch for a long time, so it gets out of sync, and merge-base may find some very ancient revision, so we get a lot of entries in the TODO list.
On the other hand, (magit-get-upstream-ref) is usually more up-to-date, and it also more likely contains a more relevant revision to find the merge base.

What do you think?

I've been using this idea for a while, and it seems to work for me. If anyone interested, I use this snippet to achieve this:

  (defun my-magit-mode-hook ()
    (let ((upstream (magit-get-upstream-ref)))
      (when upstream
        (setq magit-todos-branch-list-merge-base-ref upstream)))
  (add-hook 'magit-mode-hook 'my-magit-mode-hook)

[alphapapa]

I don't know. From what I can tell in its docstring, magit-get-upstream-ref doesn't do quite the same thing. I can imagine that it might be useful or preferred in some cases. But IIUC, when the user is working on a local, unpushed branch, it's unlikely to work.

Return the upstream branch of BRANCH as a fully qualified ref.
It BRANCH is nil, then return the upstream of the current branch,
if any, nil otherwise.  If the upstream is not configured, the
configured remote is an url, or the named branch does not exist,
then return nil.  I.e.,  return an existing local or
remote-tracking branch ref.

However, it also looks like the docstring needs to be fixed, so...

Maybe the function used to find the "merge base" (all this jargon, it's hard to even know what the correct term is) should be configurable. Patches to that effect are welcome!

[geza-herman]

I think I wasn't clear. I mean that instead of calling git merge-base HEAD (magit-main-branch), call git merge-base HEAD (magit-get-upstream-ref). In magit, usually there is an upstream branch set (I believe this is what (magit-get-upstream-ref) returns). It makes sense to use that branch instead of (magit-main-branch). Usually, (magit-get-upstream-ref) is set to upstream/master, so it's related to master. The behavior of magit-todos won't change too much if my idea is accepted. And even, we can say that upstream/master is a better setting, because upsteam/master is more likely to be more up-to-date than master. For example, I usually don't keep master up-to-date, because I almost always work on some topic branch. On the other hand, upstream/master is usually up-to-date, because a git fetch updates it (whenever I merge upstream's master to my topic branch, it gets updated). And if the user doesn't use upstream/master to pull changes, they set the upstream branch to something else. It's likely, that for your example at magit-todos-branch-list-merge-base-ref's documentation, the upstream branch for topic2 will be topic, so magit-todos will do the right thing without any additional configuration.

Note that this suggestion may not work for others, I'm not too familiar with all the possible git workflows. But for me, using (magit-get-upstream-ref) is a better default.

[alphapapa]

You were clear, and I understood what you meant. It seems that you didn't quite understand my reply. Did you read carefully the docstring I quoted? As I said, it sounds like they won't do exactly the same thing. So while that change might be better for your workflow, it might not for others.

I'm also not convinced about, e.g. upstream/master being "more up-to-date than master." Git is designed to work well offline; ISTM that if you're not keeping your local master ref up-to-date with upstream/master, that's probably not a good pattern to follow in general.

But regardless of that, I think what matters is not whether master is up-to-date with upstream/master--what matters is which branch a topic branch is branched from. If you made topic starting from upstream/master, then whatever commit that was at the time is what the current topic HEAD should be diffed against; that commit may or may not still be equivalent to upstream/master, master, or no branch at all. That's why we call git merge-base.

Anyway, like I said: "Maybe the function used to find the "merge base" (all this jargon, it's hard to even know what the correct term is) should be configurable. Patches to that effect are welcome!"

[geza-herman]

I think it's a trivial +3 -3, change the 3 occurrences of

(or magit-todos-branch-list-merge-base-ref (magit-main-branch))

to

(or magit-todos-branch-list-merge-base-ref (magit-get-upstream-ref) (magit-main-branch))

But I think the point is not this, but whether this change is good or not.

I'm also not convinced about, e.g. upstream/master being "more up-to-date than master."

I think this is should be true most of the time. Unless someone pushes to master directly.

if you're not keeping your local master ref up-to-date with upstream/master, that's probably not a good pattern to follow in general.

That is true, my master branch is almost always far behind. If I'm working on a topic branch, why should I keep my master branch up-to-date? It doesn't give me anything. It happens me a lot that my master branch is more than a half year old, simply because I don't use it for anything. On the other hand, upstream/master is almost always up-to-date, because I use it to pull new changes from master frequently. As upstream is designed to be have this role, i.e., pulling the changes from it (or at least, a lot of people use it like this), and this is the place where the topic branch will be merged, it makes sense to use it for merge-base. At least, for a certain style of git usage, this is a very sensible thing.

what matters is which branch a topic branch is branched from.

Exactly. And actually, if you create a branch from a remote branch, magit will automatically set the remote branch as the upstream branch. So (magit-get-upstream-ref) will tell you which branch was the topic branch branched from. Maybe I set something in magit to behave like this, maybe it works like this out of the box, I'm not sure (or maybe this depends on git, not on magit).

Is there any case where using (magit-get-upstream-ref) (if it is set) a wrong thing to do? Because in my use case (which is a very popular way of using git, as far as I know), this idea works well. But I'm not aware of any case where it does the wrong thing.

[alphapapa]

That is true, my master branch is almost always far behind. If I'm working on a topic branch, why should I keep my master branch up-to-date? It doesn't give me anything.

Well, it's probably a good idea in general to keep it up to date, because that can help avoid mistakes made when rebasing. But that probably depends on one's specific workflow. Git is intended to support a variety of workflows, and I'm not here to tell anyone how to use git in any certain circumstances (even though I might make a general suggestion).

Is there any case where using (magit-get-upstream-ref) (if it is set) a wrong thing to do? Because in my use case (which is a very popular way of using git, as far as I know), this idea works well. But I'm not aware of any case where it does the wrong thing.

I don't know. IME these issues are complicated and non-intuitive, and, especially, there are a variety of workflows, so it's not clear what a best default is. If I pick one, then some user (like yourself) will come and say that it's not the best default for them. If I pick yours, someone else will likely come along and say the same thing.

So, again, if you want this change, let's make it an option, and patches welcome.

[geza-herman]

Well, it's probably a good idea in general to keep it up to date, because that can help avoid mistakes made when rebasing.

It shouldn't be a problem with magit, because F u just does the right thing.

So, again, if you want this change, let's make it an option, and patches welcome.

I am fine with my little code snippet I presented in the first comment. I just wanted to make other's life easier, because I think that my suggestion is a better default. For the usual git workflow, it works in a more sensible way. But I don't really want to argue about this further, neither design a full-fledged feature with options and whatnot, and then write extra code which seems unnecessary. So I'm closing this issue.

[alphapapa]

It shouldn't be a problem with magit, because F u just does the right thing.

IME that depends on how the repository and branch in question are configured. It's not guaranteed to be the same in all cases. And so far I've yet to encounter a situation where the current code fails. So it's not clear to me that making this change would be more likely to be generally correct.

I am fine with my little code snippet I presented in the first comment. I just wanted to make other's life easier, because I think that my suggestion is a better default. For the usual git workflow, it works in a more sensible way. But I don't really want to argue about this further, neither design a full-fledged feature with options and whatnot, and then write extra code which seems unnecessary. So I'm closing this issue.

A patch to call a function selected in an option would be trivial, something like 5 lines of code including docstring--certainly far fewer characters than any of these messages we've exchanged.

If you really think that your suggestion is a better default, it would help to convince me if you'd respond to the points made in the function's docstring, which I mentioned twice. This is a decision that can't be made without concrete examples of the problem and how your suggestion solves them. Otherwise, as I said, we'll likely just bounce between "this works better for me" and "but that works better for me."

Anyway, the idea is worth considering, so there's no need to close the issue yet. If you don't want to write such a patch, maybe another user will want to.

[geza-herman]

It's not guaranteed to be the same in all cases.

Compared to what? (sorry, I genuinely don't know what you're referring to by "same")

And so far I've yet to encounter a situation where the current code fails

It failed for me (more than 3 years ago, I don't remember the details), because as I said, my master is usually way behind, because I don't really use this branch for anything. So, git found some super-old merge base (I didn't investigate it exactly why), and magit-todos presented thousands of TODOs for my branch which contained ~5 TODOs. So my first solution was to use upstream/master instead, so I didn't have to keep my master up to date. Until I realized that what I really want to have is that magit-todos-branch-list-merge-base-ref should always be the current upstream branch. Which is clear as day to me, why this is a good idea. And also, why the fixed master is not a good default (not every branch is branched from master). Sometimes we're lucky, and the merge base against master will be a good rev. Sometimes we are not, and the merge base will be some completely wrong rev.

A patch to call a function selected in an option would be trivial, something like 5 lines of code including docstring--certainly far fewer characters than any of these messages we've exchanged.

Not how I usually think about this. If it's an option, then the question arises, should it be a new option? Or should it be that if magit-todos-branch-list-merge-base-ref is a string, then it works as before, but if its value is 'upstream, then call (magit-get-upstream-ref)? Should there be an 'origin setting? What if (magit-get-upstream-ref) returns nil? Should it fallback to (magit-main-branch) then? If not, should it have a 'upstream-fallback-to-main option which falls back automatically? If it's a new option, how it interacts with magit-todos-branch-list-merge-base-ref? Etc., etc. But, until such a configuration need is justified, I don't really want to design this. I like to know the reason why I'm implementing a feature. So far I haven't heard any case where my simple +3 -3 patch idea doesn't work. Maybe there is, but then I'd like to know about it.

Regarding the docstring question. I'm not sure exactly what the problem is here (this functions just returns the <branch>@{u} rev, as far as I see). You made one specific question that I forgot to respond to:

when the user is working on a local, unpushed branch, it's unlikely to work.

If, for any reason, the upstream is not set ((magit-get-upstream-ref) returns nil), magit-todo needs to handle it. Like falling back to (magit-main-branch) (just like my +3 -3 patch does it). But if it is set, I am not aware of any case where it is a bad thing to calculate the merge base against it.

I don't want to convince you. I just suggested an idea. If you like it, you can implement it. If not, then don't. To me, it doesn't matter too much, because I already have my solution for this problem. I just created this issue because to me it seems a better default behavior, from which other people can benefit. But maybe I'm wrong about this, so apparently some feedback is definitely needed from others who are more experienced with the different kind of git workflows.

[alphapapa]

Compared to what? (sorry, I genuinely don't know what you're referring to by "same")

The branch configuration, i.e. what's listed when pressing F C in Magit.

Really, what's needed is a list of scenarios and the various values produced by each possibility so we can see what the results of the status quo and this proposed change would be.