Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow changing the git branch #137

Closed
wants to merge 5 commits into from

Conversation

dontlaugh
Copy link
Contributor

@dontlaugh dontlaugh commented Apr 8, 2021

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 dontlaugh force-pushed the 116_default_branch branch 9 times, most recently from b0bcd7d to 3b414e8 Compare July 12, 2021 04:04
@dontlaugh
Copy link
Contributor Author

@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: git/git@32ba12d

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
Copy link
Contributor Author

@naggie bump ❤️

@naggie
Copy link
Owner

naggie commented Jul 30, 2021

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
Copy link
Contributor Author

I will look into this scenario. 👍

@naggie
Copy link
Owner

naggie commented Aug 1, 2021

Thank you

@dontlaugh
Copy link
Contributor Author

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
Copy link
Contributor Author

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
#!/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

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.

Refs naggie#116
A globally-set --ff-only means a git pull will refuse to fall back to
merge commit if a fast forward cannot be set. This causes sync errors
more frequently.

Let's prefer setting --ff explicitly, making the choice on behalf of the
user to prefer merge commits to failures. The only downside is the
non-linear commit history, but this seems minor, given our use case.

See `git pull --help`
@dontlaugh dontlaugh changed the title 116 default branch Allow changing the git branch Sep 14, 2021
@dontlaugh
Copy link
Contributor Author

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

@Dieterbe
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

Dieterbe commented Sep 15, 2021

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:

  • make sync can abort if there is no remote tracking branch
  • help users set up the branch, similar to how EnsureRepoExists() currently works.

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
Copy link
Contributor Author

No apology necessary!

@naggie
Copy link
Owner

naggie commented Sep 22, 2021

@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

@dontlaugh dontlaugh closed this Sep 30, 2021
@dontlaugh dontlaugh deleted the 116_default_branch branch September 30, 2021 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make git branch name configurable
3 participants