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
