[git-llvm] Do not reinvent `@{upstream}`

[davezarzycki]

Summary

Make git-llvm more robust when used with a nontrivial repository.

[mehdi_amini]

LGTM.
Nice! I didn't this notation, apparently it's been there since 1.7.
You can also use @{u} shortcut for it, I'll try to remember this

[leonardchan]

Hi! I think this might've broken the normal workflow for using this helper script shown in https://llvm.org/docs/GettingStarted.html#for-developers-to-commit-changes-from-git

Normally, my workflow is

$ git checkout branch-to-do-work-on
$ # make and add my changes
$ git commit 
$ git llvm push

But on the git llvm push I get

`git merge-base HEAD '@{upstream}'` returned 128
fatal: no upstream configured for branch 'branch-to-do-work-on'

I think this patch might be the cause of this. Could you look into it?

[davezarzycki]

Hi @leonardchan,

1. Working with and configuring upstream branches is fundamental to using git.
2. It was a mistake for the git-llvm script to assume that the remote named "origin" is the official LLVM repository. For many people, "origin" is probably correct, but for many others, and the remote named origin depends on which repository was cloned first.
3. Your local branch isn't setup correctly. You can fix that with git branch --set-upstream-to=origin/master (assuming that origin is the correct name of the remote server in your git repository).
4. When you use git branch or git checkout, please remember to use --track if that is what you want/need.

I hope this helps,
Dave

[mehdi_amini]

Do we have the same ability as git push to specify the remote? The equivalent of git push origin master (which works without having an upstream branch)?

In any case the doc should be updated!

[davezarzycki]

The script doesn't have the ability to specify the name of the remote server. That being said, the script is only designed to work with one backend server due to how it hides SVN and in the future how it will hide GitHub status check magic.

What do you see as needing updating in the documentation?

[thakis]

Similar to leonardchan, this broke my workflow too. Please don't break user's workflows. If there's no good fix for this, I think this should be reverted.

[thakis]

To add some more words, previously we could git checkout -b foo, hack a bit commit, and run this script. Now this requires additional, long flags. I don't think making a corner case work that nobody noticed being broken for many months is worth making everything more inconvenient for everyone.

[mehdi_amini]

I took the responsibility of reverting in r371480, since I approved this revision without anticipating the behavior change.

To add some more words, previously we could git checkout -b foo, hack a bit commit, and run this script. Now this requires additional, long flags. I don't think making a corner case work that nobody noticed being broken for many months is worth making everything more inconvenient for everyone.

This is a very good point. How do we move forward though?
With the migration to GitHub around the corner, I think it'll be important to have git llvm behaves as much as possible as thin wrapper on top of git (in case you don't know git llvm push will be mandatory to push to LLVM on GitHub, as far as I know).

utils/git-svn/git-llvm

199

Losing the fallback here wasn't really intended I guess, or at least it wasn't acknowledged and made explicit in the commit message.

[davezarzycki]

In D67262#1664146, @mehdi_amini wrote:

I took the responsibility of reverting in r371480, since I approved this revision without anticipating the behavior change.

To add some more words, previously we could git checkout -b foo, hack a bit commit, and run this script. Now this requires additional, long flags. I don't think making a corner case work that nobody noticed being broken for many months is worth making everything more inconvenient for everyone.

This is a very good point. How do we move forward though?
With the migration to GitHub around the corner, I think it'll be important to have git llvm behaves as much as possible as thin wrapper on top of git (in case you don't know git llvm push will be mandatory to push to LLVM on GitHub, as far as I know).

I think it really depends on what the community wants. Is git-llvm a transitional tool? Or do people want to keep using it long after the transition?

Personally, I hope that git-llvm can go away after the transition, so deprecating deviations from normal git behavior is preferable (like the assumption that the remote is named "origin"). That being said, there seems to be some desire in the community to use git-llvm as a way to workaround GitHub quirks (like the lack of pre-receive hooks to enforce community norms like "no merge commits").

In any case, we can certainly fall back to origin/master if @{upstream} fails and warn about this behavior being different from git proper.

Dave

PS – Hi @thakis. I think it is unfair and incorrect to say that "nobody noticed" that the existing behavior was problematic. I noticed. And I'd wager others did also. That being said, the kinds of people that would notice that "origin" is hardcoded into git-llvm also tend to be the kinds of people that can workaround this git-llvm bug until they get tired of doing so and submit a patch.

[thakis]

Upon re-reading this sounds a bit accusatory. That's not how I meant this, apologies for that! I understand you're solving a problem you're having and that's a fine thing to do of course. I'm just trying to say that this particular approach (unintentionally!) caused a problem for me, and I imagine for many people using the monorepo.

I'm trying to brainstorm ways forward that solve your problem without breaking me.

[davezarzycki]

In D67262#1664562, @thakis wrote:

Upon re-reading this sounds a bit accusatory. That's not how I meant this, apologies for that! I understand you're solving a problem you're having and that's a fine thing to do of course. I'm just trying to say that this particular approach (unintentionally!) caused a problem for me, and I imagine for many people using the monorepo.

I'm trying to brainstorm ways forward that solve your problem without breaking me.

No worries. FYI – If you type: git checkout -b myNewBranch origin/master then the upstream tracking will automatically be setup.

[thakis]

I don't know what the long-term plans for this script are. I don't have a strong opinion, but weakly agree that it'd be good if it went away. In that case, I personally wouldn't add a warning and just make it temporarily do the (usually :P) right thing of using origin/master as upstream if there's no explicit upstream. If there's a warning however, it'd be nice if it included git checkout -b myNewBranch origin/master as suggested command to run, since I found that difficult to find. (Thanks for mentioning it!)

Alternatively, accepting something like git llvm push origin like mehdi suggested further up sounds good to me.

[mehdi_amini]

I don't know what the long-term plans for this script are.

The only way for us to enforce linear history on GitHub at the moment is to mandate the use of the script unfortunately.

My preference if the script stays after migration would be to not differ from git native behavior to not be surprising (I mean that git llvm push should be aligned on git push).
I hope that the issue faced here can be circumvented with better newcomers documentation. Aligning on the native git behavior means also that any git documentation (or StackOverflow answer) would apply equally to git llvm.

[davezarzycki]

In D67262#1665661, @mehdi_amini wrote:

I don't know what the long-term plans for this script are.

The only way for us to enforce linear history on GitHub at the moment is to mandate the use of the script unfortunately.

Food for thought:

1. git-llvm is run client side, so the linear history check isn't technically "enforced".
2. with SVN, nothing stopped people from merging into master other than social norms and the world didn't end.
3. why not just trust people after a few months to not do merge commits and then warn/punish people that do? Then git-llvm can go away.
4. alternatively, one could just have a frequently run script force update master to the last pre-merge commit and send out an automated and angrily worded email.
5. nag/beg GitHub to make merge commits optionally require project administrator privileges. I'd hope for a high-profile project like LLVM, they would consider it.

[mehdi_amini]

In D67262#1665677, @davezarzycki wrote:

In D67262#1665661, @mehdi_amini wrote:

I don't know what the long-term plans for this script are.

The only way for us to enforce linear history on GitHub at the moment is to mandate the use of the script unfortunately.

Food for thought:

1. git-llvm is run client side, so the linear history check isn't technically "enforced".

Someone would have to be really malicious to work around the server-side checks that goes with the client though. This is in the range of getting commit access removed IMO.

5. nag/beg GitHub to make merge commits optionally require project administrator privileges. I'd hope for a high-profile project like LLVM, they would consider it.

I think we requested it a couple of time already, I don't think it got anywhere though.

utils/git-svn/git-llvm

if sys.platform == 'win32' and not cmd.endswith('.exe'):
         cmd += '.exe'
     for path in os.environ["PATH"].split(os.pathsep):
         if os.access(os.path.join(path, cmd), os.X_OK):
             return True
     return False
 
 
 def get_default_rev_range():
-    # Get the branch tracked by the current branch, as set by
-    # git branch --set-upstream-to  See http://serverfault.com/a/352236/38694.
-    cur_branch = git('rev-parse', '--symbolic-full-name', 'HEAD')
-    upstream_branch = git('for-each-ref', '--format=%(upstream:short)',
-                          cur_branch)
-    if not upstream_branch:
-        upstream_branch = 'origin/master'
-
     # Get the newest common ancestor between HEAD and our upstream branch.
-    upstream_rev = git('merge-base', 'HEAD', upstream_branch)
+    upstream_rev = git('merge-base', 'HEAD', '@{upstream}')
     return '%s..' % upstream_rev
 
 
 def get_revs_to_push(rev_range):
     if not rev_range:
         rev_range = get_default_rev_range()
     # Use git show rather than some plumbing command to figure out which revs
     # are in rev_range because it handles single revs (HEAD^) and ranges
parser_push.add_argument(
         '--force',
         action='store_true',
         help='Do not ask for confirmation when pushing multiple commits.')
     parser_push.add_argument(
         'rev_range',
         metavar='GIT_REVS',
         type=str,
         nargs='?',
-        help="revs to push (default: everything not in the branch's "
-        'upstream, or not in origin/master if the branch lacks '
-        'an explicit upstream)')
+        help="revs to push (default: everything not in the upstream branch).")
     parser_push.set_defaults(func=cmd_push)
 
     parser_revert = subcommands.add_parser(
         'revert', description=cmd_revert.__doc__,
         help='Revert a commit locally.')
     parser_revert.add_argument(
         'revision',
         help='Revision to revert. Can either be an SVN revision number '