Disable automatic scans in remote repositories, add option to re-renable

[projectgus]

New option magit-todos-update-remote controls whether todos are automatically updated in remote repos (accessed via TRAMP). Defaults to disabled.

New option was suggested in #132 (although not a fix for that issue), and is disabled by default as suggested in #157 (comment).

Thanks for developing magit-todos, it's very handy! 😁

[alphapapa]

Hi again,

Thanks, this seems like a good idea. Will target for v1.8.

[alphapapa]

Having heard from another user today who had problems with misbehavior over TRAMP, I suspect that this new option should probably be disabled by default. What do you think?

[projectgus]

@alphapapa I found it to be fairly intrusive when working with remote repos over TRAMP, so I'd also be OK with not scanning them by default.

Would you like me to change the default in this PR?

[alphapapa]

@projectgus Seems like a good idea to me. Thanks.

[projectgus]

@alphapapa Sorry I forgot about this PR! Have swapped the default, updated the docs to mention, and re-tested.

[mohkale]

Hi there, this looks like it would close #178 . Is there anything left pending for this (happy to help if I can) 🤔.

[alphapapa]

I appreciate your patience, as the past few weeks have been very busy for me.

Regarding this PR, I'm not especially happy with the size of the diff, caused by the change in indentation of a large form. As well, given the recent change adding caching of branch-diff items which affects this diff's range, this change would need to be rebased. And the option's docstring needs to be rewritten to follow documentation guidelines, be generally more concise, and explain its interaction with other options more clearly.

Finally, I'm not entirely sure about the name of the option, and maybe the option itself. It might be good to consider if this is the best way to handle the issue. For example, since a remote system might not have the same scanners installed, it might be good to address that problem in a unified way, e.g. testing for the scanner's presence and falling back to git diff, or disabling remote scanning entirely, etc.

Sometimes it's best to discuss a problem and potential solutions in an issue before writing code and submitting a PR. That way the time of the patch submitter isn't wasted writing code that may not be merged.

[mohkale]

Thanks @alphapapa . Lets discuss in the related issue.

[projectgus]

Hi @alphapapa,

Thanks for the review comments, and absolutely no problem with being busy. I fully support you not looking at this PR again until you have time (if ever!)

this looks like it would close #178

I agree that this doesn't fully resolve the issue linked there, all it does is prevents errors popping up automatically for remote repos if the scanner isn't found. The checking/fallback approach you mention seems like it could be a better long term fix.

this change would need to be rebased.

Have rebased again just now, and re-tested.

I'm not especially happy with the size of the diff, caused by the change in indentation of a large form

I haven't tried to address this, any alternative factoring that I can think of is a bigger change in terms of overall code structure and I'm not confident I'd avoid regressions. I'm happy to have a go at it if it'd be useful, though.

And the option's docstring needs to be rewritten to follow documentation guidelines, be generally more concise, and explain its interaction with other options more clearl

I've rewritten the docstring to be more concise. I don't know which documentation guidelines you're referring to, but if you want me to make more changes then let me know.

I'm not entirely sure about the name of the option

Agreed (insert joke about only one hard problem in computer science here).

Sometimes it's best to discuss a problem and potential solutions in an issue before writing code and submitting a PR. That way the time of the patch submitter isn't wasted writing code that may not be merged.

I would say in my defence that I did look at the issues before sending this patch, I originally based it on this discussion in #132 where you said an option like this "seems like a good idea", and I saw you 👍 someone else's older comment that they'd try to implement it. Then I kept updating the PR based on your initial positive comments above.

Of course, quite a bit of time has passed and it's reasonable to want to go a different way. My main reason for writing this code was to "scratch my own itch", and my straight.el is updated to use my fork so my personal problem is solved indefinitely. In particular, I fully appreciate not wanting to commit to a config option if it might end up replaced in a future release.

So, needless to say, if you prefer to close this and approach it another way, or leave it to the side while you get on with more rewarding parts of life, then it's fine by me. Alternatively, if you've specific requests for changes then I'm happy to make those changes in this PR. Up to you.

[alphapapa]

Hi @alphapapa,

Thanks for the review comments, and absolutely no problem with being busy. I fully support you not looking at this PR again until you have time (if ever!)

Thanks for the kind words and your understanding.

this looks like it would close #178

I agree that this doesn't fully resolve the issue linked there, all it does is prevents errors popping up automatically for remote repos if the scanner isn't found. The checking/fallback approach you mention seems like it could be a better long term fix.

Probably so. In lieu of that an option like this could still be helpful.

this change would need to be rebased.

Have rebased again just now, and re-tested.

I'm not especially happy with the size of the diff, caused by the change in indentation of a large form

I haven't tried to address this, any alternative factoring that I can think of is a bigger change in terms of overall code structure and I'm not confident I'd avoid regressions. I'm happy to have a go at it if it'd be useful, though.

Probably what's needed is a refactoring. The branch-diff to-do list was an add-on, not part of the original design. It would be better if the system supported any number of such scans/sections.

And the option's docstring needs to be rewritten to follow documentation guidelines, be generally more concise, and explain its interaction with other options more clearl

I've rewritten the docstring to be more concise. I don't know which documentation guidelines you're referring to, but if you want me to make more changes then let me know.

The Elisp manual appendices have some helpful guidelines.

I'm not entirely sure about the name of the option

Agreed (insert joke about only one hard problem in computer science here).

Of course. :)

Sometimes it's best to discuss a problem and potential solutions in an issue before writing code and submitting a PR. That way the time of the patch submitter isn't wasted writing code that may not be merged.

I would say in my defence that I did look at the issues before sending this patch, I originally based it on this discussion in #132 where you said an option like this "seems like a good idea", and I saw you 👍 someone else's older comment that they'd try to implement it. Then I kept updating the PR based on your initial positive comments above.

I didn't mean that as a criticism; more as a defense of myself not wanting to merge the code as-is. :)

Of course, quite a bit of time has passed and it's reasonable to want to go a different way. My main reason for writing this code was to "scratch my own itch", and my straight.el is updated to use my fork so my personal problem is solved indefinitely. In particular, I fully appreciate not wanting to commit to a config option if it might end up replaced in a future release.

Thanks, I'm glad to hear that.

So, needless to say, if you prefer to close this and approach it another way, or leave it to the side while you get on with more rewarding parts of life, then it's fine by me. Alternatively, if you've specific requests for changes then I'm happy to make those changes in this PR. Up to you.

I'm pretty busy right now, so I'll probably defer this for a while; and if a refactoring more suitable for the long term comes up in the meantime, I might merge that instead.

Either way, I appreciate your contribution and goodwill.

[alphapapa]

If you'll address these minor issues, I'll merge it. Please also add a changelog entry. Thanks.

[projectgus]

@alphapapa Thanks for the pointers, especially to the appendix in the Emacs Lisp manual - I'd never noticed this. PTAL when you get the time.

[projectgus]

(Sorry for the extra pushes, just fiddling with wording.)

[alphapapa]

Merged with a few small changes in additional commits. Thanks, Angus!