Re: fast forward merge overwriting my code

[Igor Djordjevic <igor.d.djordjevic@gmail.com>]

On 24/05/2021 08:13, Andre Ulrich wrote:
> 
> So this is how we proceed:
> - my prof has a repo on GitHub
> - I have forked the repo
> - I have cloned the forked repo
> - I have created a branch 'update' in my local clone
> - I edit a notebook on the branch 'update' and commit
> - I push 'update' to my forked repo on GitHub
> - I create a merge request
> - my prof reviews the changes and accepts them (if I have done 
>   acceptable work)
> 
> So the last point is where we still want to do some fine tuning. 
> Right now this looks about: my prof fetches my edits and locally 
> checks out a branch to compare the changes with git diff. But in this 
> diff view you can't edit the files. So you have to separately open up 
> another window to edit the changes (lets say my prof only wants to 
> keep some of my changes, but not all).

I think that last point highlights the issue you guys are having - 
using `merge` for doing both (1) actual merge, but also (2) review and 
edit at the same time, which is wrong (or very unconventional, to say 
the least).

In ideal case (meaning no conflicts, no matter if 3-way merge or a 
fast-forward one), merge should accept all the changes being merged 
in from the side branch and incorporate them into the main branch. 

From this basic and the most common scenario alone it is visible that 
merge should not "keep some changes, but not all" - the very point of 
a merge is to (try to) keep _all the changes_, period.

Now, as for the "try to" part - in some cases not all changes can be 
kept as they are, like when both branches changed same files and same 
lines (or close to), so that's when Git hands over the resolution to 
the user, to determine what is the desired outcome of a conflicting 
merge.

Still, even in this case, the final outcome should be considered a 
sum of all the changes, even though some might have been altered or 
rearranged in order to better work with each other (as different 
branches might have done the same thing in a different way).

In any case, it should not be up to the merge (process nor commit) to 
discard (nor add!) some of the non-conflicting changes you have made on 
your 'update' branch - it is possible to do (something usually called 
an "evil merge", and for a reason), yet is not a good practice.

As an example, imagine you have commits 1, 2 and 3 on your 'update' 
branch, and upon merging your professor decides to accept changes 
from commit 2 only, completely discarding changes from commits 1 and 3. 
Your history will end up looking something like this:

(1) ---X-----------M 'master'
        \         /
         1---2---3 'update'

... where M is the merge commit, merging branch 'update' into 'master'. 
As it is, it's reasonable to expect of M to contain all the changes 
brought in by 1, 2 and 3 - yet it is not the case, which could be 
rather confusing (on later history review).

What would be a more common/usual scenario is, after trying a local 
merge M and seeing some changes should not be accepted (like commits 
1 and 3), have your professor communicate the problem with you so you 
can fix the issues yourself, inside 'update' branch, and iteratively 
repeat this process as long as 'update' branch is not "perfect" - at 
which point it can be accepted _as a whole_, that is.

You professor should not accept to merge your changes as long as they 
are not all correct, and he specifically should not be using the 
merge to correct the issues himself.

Depending on your preference, he _could_ be doing the changes himself, 
too - but again doing so through standalone commits (on your 'update' 
branch, for example), _not_ through a merge commit.

Based on example (1) above, the finally merged changes history could 
instead look like this:

(2) ---X-------------------M1 'master'
        \                 /
         1---2---3---4---5 'update'

..., where commits 4 and 5 are fixes made on 'update' after your 
professor's comments on commits 1, 2 and 3, and M1 is the merge which 
finally accepts all the changes from 'update'.

Alternatively, if you use rebase, you can alter problematic commits 1 
and 3 directly instead, so the history would look something like this:

(3) ---X-----------M2 'master'
        \         /
         1'--2'--3' 'update'

..., where original commits 1 and 3 are changed in order to be 
acceptable for the merge, becoming commits 1' and 3', while commit 2' 
would stay the same as original commit 2. Again, merge commit M2 
accepts all the changes as they now are (all correct).

Also, if commits 1 and 3 are completely wrong and not required in the 
first place, yet another alternative (using rebase) would be to drop 
them altogether, ending up with a history like this:

(4) ---X----M3 'master'
        \  /
         2" 'update'

..., where commit 2" would be exactly the same as original commit 2, 
and commits 1 and 3 are dropped from the history completely - and 
transparently, _not_ using the merge to do so, as in original example (1)
(and your explained scenario).

I hope these examples somewhat help, the main point remaining that 
merge should not be used to discard/disapprove certain (especially 
non-conflicting) changes, but only to finally accept/approve _all_ 
the changes, possibly modified in the meantime as a result of an 
iterative review and additional work.

Note that there's nothing wrong in having your professor do his own 
local merges as part of this review process, but those should be only 
temporary, to be discarded and not accepted until everything can be 
merged (and accepted) as-is.

Regards, Buga