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

Require remote branch to be specified in Push dialog for non-fast forward mode since implementation only does force-push in that case #618

Merged
merged 1 commit into from Oct 28, 2016

Conversation

Projects
None yet
3 participants
@AndiDog
Contributor

AndiDog commented Oct 25, 2016

Even if "Fast Forward Only" was deselected, a branch wouldn't be force-pushed unless the remote branch is selected in the dialog. That's because of the implementation

def refspec(src, dst, ffwd, push=False):
    if push and src == dst:
        spec = src
    else:
        spec = '%s:%s' % (src, dst)
    if not ffwd:
        spec = '+' + spec # <-----------
    return spec


def refspec_arg(local_branch, remote_branch, ffwd, pull, push):
    """Return the refspec for a fetch or pull command"""
    if not pull and local_branch and remote_branch: # <-------------
        what = refspec(remote_branch, local_branch, ffwd, push=push)
    else:
        what = local_branch or remote_branch or None
    return what

This change therefore requires the remote branch to be selected. I could have changed the implementation, but I think since force-push is so dangerous, it's a safe choice to require the user to select the target.

Require remote branch to be specified in Push dialog for non-fast for…
…ward mode since implementation only does force-push in that case

Signed-off-by: Andreas Sommer <andreas.sommer87@googlemail.com>
@kkofler

This comment has been minimized.

Show comment
Hide comment
@kkofler

kkofler Oct 25, 2016

I think the code should just be fixed instead (i.e., make unchecking "Fast Forward Only" actually work, without silly restrictions).

kkofler commented Oct 25, 2016

I think the code should just be fixed instead (i.e., make unchecking "Fast Forward Only" actually work, without silly restrictions).

@AndiDog

This comment has been minimized.

Show comment
Hide comment
@AndiDog

AndiDog Oct 25, 2016

Contributor

I see the following options:

  1. Require selection in dialog (my solution)
  2. Just force-push to upstream branch if no remote branch was selected by the user. Don't know by heart how the command would look like.
  3. Have the implementation return an error in that case, and from that cause show the dialog (as in my solution, but error coming from implementation, instead of UI making assumptions about implementation).

I would prefer 1 or 2. If we opt for 2, I will research what safe options there are for the command line, or use --force otherwise.

Contributor

AndiDog commented Oct 25, 2016

I see the following options:

  1. Require selection in dialog (my solution)
  2. Just force-push to upstream branch if no remote branch was selected by the user. Don't know by heart how the command would look like.
  3. Have the implementation return an error in that case, and from that cause show the dialog (as in my solution, but error coming from implementation, instead of UI making assumptions about implementation).

I would prefer 1 or 2. If we opt for 2, I will research what safe options there are for the command line, or use --force otherwise.

@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Oct 26, 2016

Member

Hmm I do like that this makes it a little bit safer, but since we already prompt the user about the force-push then it probably makes sense to try and make it just work.

The syntax for force-pushing would be e.g. git push origin +branch-name so it's probably a small oversight that is preventing this from working.

While it does repeat the logic a little.. this might work for the else block in refspec_arg():

what = local_branch or remote_branch or None
if what and not ffwd:
    what = '+' + what
Member

davvid commented Oct 26, 2016

Hmm I do like that this makes it a little bit safer, but since we already prompt the user about the force-push then it probably makes sense to try and make it just work.

The syntax for force-pushing would be e.g. git push origin +branch-name so it's probably a small oversight that is preventing this from working.

While it does repeat the logic a little.. this might work for the else block in refspec_arg():

what = local_branch or remote_branch or None
if what and not ffwd:
    what = '+' + what
@kkofler

This comment has been minimized.

Show comment
Hide comment
@kkofler

kkofler Oct 27, 2016

Why not just use the -f flag (which is separate from the branch and can be given even without one) instead of the + shortcut (which has to be prepended to the branch)?

kkofler commented Oct 27, 2016

Why not just use the -f flag (which is separate from the branch and can be given even without one) instead of the + shortcut (which has to be prepended to the branch)?

@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Oct 28, 2016

Member

I've always wanted to refactor the code so that fetch, push, and pull are better handled. Passing force=<bool> to the underlying command object would probably make things simpler too.

Also, taking a look at the UI, Fetch and Push should probably say "Force" in the UI.

Only Pull should say "Fast-forward Only", and that checkbox is hidden for Pull (that's a mistake). unless someone beats me to it I'll take a look at fixing this shortly.

Member

davvid commented Oct 28, 2016

I've always wanted to refactor the code so that fetch, push, and pull are better handled. Passing force=<bool> to the underlying command object would probably make things simpler too.

Also, taking a look at the UI, Fetch and Push should probably say "Force" in the UI.

Only Pull should say "Fast-forward Only", and that checkbox is hidden for Pull (that's a mistake). unless someone beats me to it I'll take a look at fixing this shortly.

@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Oct 28, 2016

Member

As a short-term safety measure, this is good so I'll merge this for now before I start tweaking it.

Member

davvid commented Oct 28, 2016

As a short-term safety measure, this is good so I'll merge this for now before I start tweaking it.

@davvid davvid merged commit 43596ec into git-cola:master Oct 28, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

davvid added a commit that referenced this pull request Oct 28, 2016

remote: simplify and improve how fast-forward flags are handled
Use "push --force" and friends instead of using the "+" refspec syntax.
Expose the --ff-only and --no-ff flags supported by "git pull".

Related-to: #618
Suggested-by: Kevin Kofler <Kevin@tigcc.ticalc.org>
Signed-off-by: David Aguilar <davvid@gmail.com>

@AndiDog AndiDog deleted the AndiDog:feature/require-remote-branch-on-force-push branch Oct 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment