github-education-resources / teachers_pet

Command line tool to help teachers use GitHub in their classrooms
https://education.github.com/guide
MIT License
187 stars 74 forks source link

`push_files` action #35

Closed kelleydv closed 10 years ago

kelleydv commented 10 years ago

EDITED for clarity:

What is the recommended procedure for pushing new files to student/team repos?

As an eample of the error, I get the following (typical error when the remote is ahead)

==================================================
Authenticating to GitHub...
Found organization at: https://api.github.com/orgs/MyOrg
Adding remotes and pushing files to student repositories.
userA --> git@github.com:MyOrg/userA-repo.git
fatal: remote userA already exists.
To git@github.com:myOrg/userA-repo.git
 ! [rejected]        master -> master (fetch first)
error: failed to push some refs to 'git@github.com:myOrg/userA-repo.git'
hint: Updates were rejected because the remote contains work that you do
hint: not have locally. This is usually caused by another repository pushing
hint: to the same ref. You may want to first merge the remote changes (e.g.,
hint: 'git pull') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

As a tangential note, I always seem to get this when pushing files even for the first time fatal: remote userA already exists.

The problem of students not getting updates after making commits makes sense. For some reason I thought I was going to get away with it as long as there were no direct conflicts in the files. How do we solve the problem of pushing updates to already-created-repos for students? Is this a problem that we should solve, or is it simply not recommended to push updates in this way?

afeld commented 10 years ago

The fatal: remote ____ already exists message comes up because you are running the script multiple times, and the remotes don't get removed after it's done. I'd say we can either change the script to clean up after itself, or have not try to re-add the remote if it's already present.

Adding the remote isn't the real problem here, though. The key is Updates were rejected because the remote contains work that you do not have locally. Since they have commits that aren't present in your local branch, you'd have to first merge their work down in order to push new changes. Unfortunately, since it's not a fast-forward, there is potential for merge conflicts, etc., which is a rabbit hole that we probably don't want to go down... maybe better simpler to put the onus on students via a fork syncing script? No great option here :confused:

Out of curiosity, what kinds of changes were you trying to push? While it's nice to have the instructions in the repository, it's also nice to have a single source of truth for instructions. For a missing or fix to a required setup file, that's a bit more grey-area.

kelleydv commented 10 years ago

I'd say we can either change the script to clean up after itself, or have not try to re-add the remote if it's already present.

Everything was working as expected, so I wasn't too worried, but "fatal" can be disconcerting. Addressing this makes sense.

which is a rabbit hole that we probably don't want to go down...

I agree, and I think that it means the onus is on me (read: teachers) to have a repo in a ready-to-deploy state if I am going to create a bunch of copies and assign teams. Any "fix" I can imagine would be pretty ugly and inefficient. This private repo model is a different paradigm than forking.

what kinds of changes were you trying to push?

I was trying to add a file not present in the original repo, which is why I naively thought I could get away with it since there would be no direct conflicts.

kelleydv commented 10 years ago

Let's return to the fatal: remote __ already exists at a later time.

mikehelmick commented 10 years ago

I agree that the warning can be downgraded.

It is, however, totally cool to run push files after you have already pushed the files. I've done this before when I make a mistake in the starter files.

The error above is going to continue to be an issue though, that is actually a fatal error and a git error, nothing to do with these scripts. If the student has already make a commit before you push out corrected starter files, then you're left w/ a problem.

The way I've handled this in the classroom is to do one of these things

kelleydv commented 10 years ago

Thanks for the input. Maybe we could add a Gotchas or Common Snafus section to the docs at some point that both warns users of pitfalls and explains workarounds.

afeld commented 10 years ago

I'm a bigger fan of contextual documentation. In this case, if it fails to push because the local branch is behind, it should explain right there in STDERR what happened and how to get around it.

kelleydv commented 10 years ago

That sounds best.