Trigger only updates on odd numbered input and not even

Question

trigger trig_Opportunity_CreateOppOwner on Opportunity (before update,before insert) {

List<id>OppsID = new List<id>(); //List that will hold all id of all new Opportunities owners

for(Opportunity Opp:Trigger.new){ //Loop that only retrives new versions of the Opportunity 
    if(trigger.isInsert)
        OppsId.add(Opp.ID);
    else if (trigger.oldMap.get(Opp.id).Op_Owner__c != trigger.oldMap.get(Opp.id).OwnerId)//trigger.isInsert and trigger.isUpdate added to account when it's null, avoids NullPointerException. Now all inserted opportunities are processed.           
        OppsId.add(Opp.ID);//adds all new opportunity owners to OppsId list. //Checks to see if they are new owners and not already on the owner list. 
}
if(OppsID.size() <= 100) 
List<Opportunity> oppsFromDb =  [SELECT id, OwnerId FROM Opportunity WHERE id IN:OppsID];   


for(Opportunity useOpps:oppsFromDb){                            
    trigger.newMap.get(useOpps.Id).Op_Owner__c = useOpps.OwnerId;
}

The following trigger basically checks to see if OwnerId has been updated. If it has, it stores the new Owner in a custom field called Opp_Owner__c. However, the trigger seems to change only on odd numbered changes. For example:

1. We change Owner. Op_Owner__C gets updated with the new Owner
2. We change Owner again on the same opportunity. Op_Owner__c DOES NOT get updated with the second change on the owner we did.
3. We Change Owner a third time on the same Opportunity. Now the Op_Owner__c gets updated to the third owner change we did.

Answer 1

I think you need to check against the Trigger.new Opportunity in your else if. That way, you're checking the current value of the Opportunity.OwnerId against the old Op_Owner__c field.

Here's the line I'd recommend changing:

else if(trigger.oldMap.get(Opp.id).Op_Owner__c != Opp.OwnerId) { 

And, here's the full trigger:

trigger trig_Opportunity_CreateOppOwner on Opportunity(before update, before insert) {

  // List that will hold all id of all new Opportunities owners
  List <Id> OppsID = new List <Id> (); 

  // Loop that only retrives new versions of the Opportunity 
  for(Opportunity Opp : Trigger.new) { 
    if(trigger.isInsert) {
      OppsId.add(Opp.ID); 
    }
    // trigger.isInsert and trigger.isUpdate added to account when it's null, 
    //  avoids NullPointerException. Now all inserted opportunities are processed.    
    else if(trigger.oldMap.get(Opp.id).Op_Owner__c != Opp.OwnerId) { 
      // Adds all new opportunity owners to OppsId list. 
      //  Checks to see if they are new owners and not already on the owner list. 
      OppsId.add(Opp.ID); 
    }
  }

  if(OppsID.size() <= 100) {
    List <Opportunity> oppsFromDb 
      = [SELECT id, OwnerId FROM Opportunity WHERE id IN : OppsID];
  }

  for(Opportunity useOpps : oppsFromDb) {
    trigger.newMap.get(useOpps.Id).Op_Owner__c = useOpps.OwnerId;
  }
}

As pointed out, in the comments, the above code would only work for updates, not inserts due to IDs not being set until after insert.

Answer 2

This is a trigger I've needed several times, and can be done more simply than what you have there. Try this:

trigger trig_Opportunity_CreateOppOwner on Opportunity(before update, before insert) {

    for (Opportunity opp : Trigger.new) {
        opp.Op_Owner__c = opp.OwnerId;
    }
}