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

Improve detection of the target branch #16

Merged
merged 1 commit into from Mar 8, 2017

Conversation

kemenaran
Copy link
Contributor

If the target branch isn't specified in the command arguments, attempts to find the name of the remotly tracked branch (before falling back on master).

This means that if the branch feature tracks upstream/dev, the code will correctly detect that the target branch is dev.

@caarlos0
Copy link
Member

caarlos0 commented Mar 8, 2017

Nice one, just need to fix the shellcheck warnings :)

test -z "$target" && target="$(git for-each-ref --format='%(upstream:short)' "$(git symbolic-ref -q HEAD)" | cut -d '/' -f 2)"

If the target branch isn't specified in the command arguments, attempts to find the name of the remotly tracked branch (before falling back on `master`).

This means that if the branch `feature` tracks `upstream/dev`, the code will correctly detect that the target branch is `dev`.
@kemenaran
Copy link
Contributor Author

@caarlos0 PR updated with the linter warning fixed.

@caarlos0
Copy link
Member

caarlos0 commented Mar 8, 2017

Thanks @kemenaran 👍

@caarlos0 caarlos0 merged commit 759f21b into caarlos0-graveyard:master Mar 8, 2017
@kemenaran kemenaran deleted the patch-1 branch March 9, 2017 08:51
@Turbo87
Copy link

Turbo87 commented Oct 10, 2017

@caarlos0 this actually broke the script for me. If I push branch foo to origin and then run the script it will point to /compare/foo...Turbo87:foo instead of /compare/master...Turbo87:foo

@kemenaran
Copy link
Contributor Author

@Turbo87 indeed it probably worked before incidentally, because no target branch was specified and master was the default fallback.

That said, if no remote-tracking branch is found, the script is still expected to fallback to master anyway. You results seem to imply that either this patch had a bug (which is very possible), or that your foo branch is tracking origin/foo. Is it the case? If so, does your workflow requires it?

@Turbo87
Copy link

Turbo87 commented Oct 10, 2017

or that your foo branch is tracking origin/foo

yes, that is the case.

If so, does your workflow requires it?

it's not required, but it would certainly be good if you want to modify the PR again after opening it. then you can just git push instead of having to manually specify the remote and branch name all the time.

@kemenaran
Copy link
Contributor Author

Good point.

Maybe the part of the script that attempts to infer the target branch could explicitly ignore the remote-tracking branch if it has the same name and remote than the local branch?

This would mean the following behavior:

  • dev doesn't track any branch. The target is inferred to be origin/master.
  • dev tracks upstream/stable. The target is inferred to be upstream/stable.
  • dev tracks upstream/dev. The target is inferred to be upstream/dev (as the tracking remote is not origin).
  • dev tracks origin/dev. The target branch is inferred to be origin/master (the fallback), as it would otherwise be origin/dev, which is the same name and remote than the local branch.

@Turbo87
Copy link

Turbo87 commented Oct 10, 2017

hmm, I'm not sure why it would ever target the origin remote?

@kemenaran
Copy link
Contributor Author

kemenaran commented Oct 10, 2017

I guess it would be sensible to target the origin remote either:

  • In the case where I am working on my own repository, but still want to make feature branches and PRs,
  • In the case where several people are working with feature branches on the same repository directly (rather than everyone having its own fork).

In both these cases PRs would be made with origin as the target remote, right?

@Turbo87
Copy link

Turbo87 commented Oct 10, 2017

In the case where I am working on my own repository, but still want to make feature branches and PRs

sounds like an uncommon case to me (--remote=origin option maybe?)

In the case where several people are working with feature branches on the same repository directly (rather than everyone having its own fork).

in that case I still use upstream as the remote name

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.

None yet

3 participants