naggie / dstask

Git powered terminal-based todo/note manager -- markdown note page per task. Single binary!
https://calbryant.uk/blog/dstask-a-taskwarrior-alternative/
MIT License
793 stars 47 forks source link

Allow changing the git branch #137

Closed dontlaugh closed 2 years ago

dontlaugh commented 3 years ago

We should be able to rely on pushing and pulling from the default branch without explicitly setting "master", or anything else. dstask will simply push and pull from the remote named "origin" using whatever default tracking branch is set.

In git 2.32, it's possible to specify the default initial branch for git repositories. This is globally configurable with init.defaultBranch. See man git-init for details.

Closes #116

dontlaugh commented 3 years ago

@naggie This is finally ready. I was stumped for a long time, because the tests were relying on git features that were too new. Specifcally this: https://github.com/git/git/commit/32ba12dab2acf1ad11836a627956d1473f6b851a

So, I was confused because I could not reproduce the error locally on Manjaro, which gives me the newer git version 2.32

I don't believe we really need to set a different default in the tests, however. It should be sufficient to check that nothing breaks.

dontlaugh commented 3 years ago

@naggie bump :heart:

naggie commented 3 years ago

Sorry -- caught up in too much stuff at the moment.

I'm worried this could break some existing repositories without an upstream tracking master/main branch. Is that an issue, or are we somehow protected? If not perhaps dstask should guide to a solution?

dontlaugh commented 3 years ago

I will look into this scenario. :+1:

naggie commented 3 years ago

Thank you

dontlaugh commented 3 years ago

You are right, it is a potential problem. I created a test environment for this branch as a linux container.

With no default upstream, dstask sync returns this error:

testuser@56c1851c0dd0:~$ dstask sync
You asked to pull from the remote 'origin', but did not specify
a branch. Because this is not the default configured remote
for your current branch, you must specify a branch on the command line.
Failed to run git cmd.

Git itself nudges the user in the right direction, but it's not the best.

I was hoping to implement this feature with no additional config, but now I am leaning towards a new configuration variable: DSTASK_BRANCH

We have at least 3 options

  1. Create a new DSTASK_BRANCH environment variable. If empty, use master branch, just like now. If set, explicitly push and pull from the value set in DSTASK_BRANCH.
  2. Use git commands to try and determine if there is a missing tracking branch, and if there is no such branch, error out with a dstask-specific error message (and our own instructions).
  3. Do nothing, and write docs around this issue. Rely on git itself to print guidance to stderr.
dontlaugh commented 3 years ago

If you are interested in running my test container, it's public here. This branch's version of dstask is already on the path, but no task database exists. I also used an older Ubuntu, to get an older version of git.

podman run -it quay.io/colemanx/dstask-test-116:latest

I prefer podman, but docker should work.

The build script (again with buildah, not docker)

script ```bash #!/bin/bash set -ex container_path="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/local/go/bin" golang_url="https://golang.org/dl/go1.14.15.linux-amd64.tar.gz" feature_branch="https://github.com/dontlaugh/dstask/archive/refs/heads/116_default_branch.zip" pkgs="git wget unzip make" id=$(buildah from --name dstask-test-116 --pull "docker://ubuntu:18.04") buildah run $id -- apt update -y buildah run $id -- apt install -y $pkgs buildah run $id -- wget $golang_url buildah run $id -- tar -C /usr/local -xzf "go1.14.15.linux-amd64.tar.gz" buildah run $id -- useradd -m testuser -s /bin/bash buildah config \ --env PATH=$container_path \ --user testuser \ --cmd /bin/bash \ --workingdir /home/testuser \ $id buildah run --user "testuser:testuser" $id -- wget $feature_branch buildah run --user "testuser:testuser" $id -- unzip 116_default_branch.zip buildah run --user "testuser:testuser" $id -- \ bash -c "cd dstask-116_default_branch && make" buildah run --user "root:root" $id -- \ bash -c "cd dstask-116_default_branch && make install" buildah commit $id quay.io/colemanx/dstask-test-116:latest ```
dontlaugh commented 3 years ago

@naggie I believe this is ready for re-review. Let me know if you want me to squash this down.

Dieterbe commented 3 years ago

So which option out of the 3 are we pursuing now? If there is an origin, then why would we default to master rather than looking at which branch is used to sync with origin?

dontlaugh commented 3 years ago

I have implemented option 1.

I was hoping to avoid any config. And I do think that number 2 is possible. But using an explicit DSTASK_BRANCH configuration variable leads us to a simpler implementation (probably).

The current behavior is hardcoded to "master".

Dieterbe commented 2 years ago

I know it's annoying when people chime in after much of the work has already been done, so apologies for being late. But I don't see why we would add configuration options / environment variables when this seems better solved by being a bit opinionated.

It sounds like the issue brought up by @naggie is that of users wishing to sync, but don't have a remote tracking branch set up. So why don't we make it a requirement then, that sync only works if a remote tracking branch is set up? This seems very reasonable to me. We can then:

I think adding config options and env vars should not be done lightly. Especially when they must be set if you want the tool to work. It complicates things, especially for new users. I rather focus more on initial setup rather than an env var which must be permanently set.

As a reminder, one of the values we set out in #90 is:

dstask is opinionated. Should not be over-configurable

dontlaugh commented 2 years ago

No apology necessary!

naggie commented 2 years ago

@Dieterbe I think that's a really good pragmatic solution that effectively delegates this configuration to git such that we don't need to add unnecessary config to git.

Sorry @dontlaugh I think we should abandon this implementation in favour of requiring a remote tracking branch to be set up such that git push/pull work implicitly.

Thanks both