Re: [Internet]Re: [PATCH] merge-tree.c: add --merge-base=<commit> option

["kylezhao(赵柯宇)" <kylezhao@tencent.com>]

> Note that this has been implemented previously.  I may have implemented it previously and ripped it out (about a year ago), or maybe I just avoided it once I ran across the reasons below.  Dscho also implemented it over at https://lore.kernel.org/git/nycvar.QRO.7.76.6.2202210956430.26495@tvgsbejvaqbjf.bet/
>
> You may want to take a look at his code for comparison, especially the
merge_incore_nonrecursive() stuff.

Thanks. I'll take a look later.

> This capability is something we may ultimately want for completeness,
> but it definitely should not be marketed as a building block for
> implementing cherry-pick or rebase.  There are several reasons for
> that:
> 
> Performance reasons:
>  * it's a design with a separate fork for every commit to be picked,
> ....
> Maintenance reason:
>   * suggesting this code as a basis for cherry-pick or rebase is
> likely to lead to yet another shell-scripted rebase; we've been trying
> to generally nuke shell scripts from Git for a variety of reasons.
> It'd be sad to regress here.

Thank you for your very detailed explanation, I feel like I have a deeper understanding of rebase and cherry-pick.

> My apologies for having very limited git time (which is often entirely used up just in reviewing/responding to patches and other queries on the list instead of on new features like this, or maybe on making some nice slides for a conference); if I had more time, I think git-replay could have easily been done many months ago (or perhaps even last year).  Then you'd have the high level tool you need for server side cherry-picking and rebasing.  But, I haven't had much time.  So, it makes sense that folks might want to use an interim hack while waiting for the more robust tool to materialize.

git-replay sounds great, looking forward to it one day.

> Would it make sense to have o->merge_base be a struct commit *, and move this lookup/die logic out to the parsing logic in cmd_merge_tree()?
Done.

> and the next line of code, not showing in the context here, is a call
> to merge_incore_recursive().  While that technically works, it doesn't
> make logical sense.  You're not doing a recursive merge when you have
> a specified merge base, so I think the code should instead call
> merge_incore_nonrecursive() in such a case (see Dscho's code for a
> comparison here).
Done. I've changed it to merge_incore_recursive(). It works,  but I don't know if it's written correctly, especially opt.ancestor.

> So, the O-A-B thing thing was taken from t6423; it appears I didn't copy the comment at the top of t6423 over to t4301 to explain this.
> Sorry about that.  Anyway, for these types of comments in these files, O is always the merge base of both A and B, and neither A nor B contain each other in their history.  From that basis, your description here makes no sense (A would be the tip of some branch, not the parent of anything).  I'd call your commits something else (maybe just C1, C2, and C3 or something?).

Done.

> Would test_commit be useful here, given that you aren't worried about
> the exact contents of files?

It's useful, which makes the test clearer.