RE: [Internet]Re: [PATCH v4 2/2] merge-tree.c: support --merge-base in conjunction with --stdin

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

Thanks for your review.

>>
>> The previous change add "--merge-base" option in order to allow users 
>> to
>
> s/add/added/ ?

Done.


> Perhaps something like:
> 
> """
> The previous commit added a `--merge-base` option in order to allow using a specified merge-base for the merge.  Extend the input accepted by `--stdin` to also allow a specified merge-base with each merge requested.  For example:
> 
 >    printf "--merge-base=<b3> <b1> <b2>" | git merge-tree --stdin
>

Done.


> However, I'm a bit curious about using `--merge-base=` on the input line as opposed to just using a simpler marker; something like
> 
>     printf "<b3> -- <b1> <b2>" | git merge-tree --stdin
> 
> (which follows a precedent set by git-merge-recursive).  Your version is a bit more self-documenting, but what if we want to allow users to specify multiple merge bases in the future (for use in passing to merge_incore_recursive())?  Is it annoying to need to prefix each one?

Actually, I thought about it at the time, but didn't think of any good input format. Your idea is great, I think I'll use it.

> I'm not sure "passed into the input line" will be clear to readers.
> Perhaps we want to have --stdin documented (looks like I forgot to do that in my series; oops), mentioning the input format.

I just added the documentation for --input, please see what else needs to be improved.

> > +       printf "1\0" >>expect &&
> > +       git merge-tree --write-tree -z --merge-base=c2 c1 c3 >>expect &&
> > +       printf "\0" >>expect &&
> > +       test_cmp expect actual
> > +'
>
> This last test seems odd.  You are merely testing that the output of "git merge-tree --stdin" matches the output of repeated calls to "git merge-tree", not that the merges involved produce correct results?
> Maybe that's fine since earlier tests verify that individual merge-tree calls are doing the right thing, I was just a bit surprised.  Maybe a comment in the code that this was your intent would be helpful?

Yeah, I have added the comment.