RE: [Internet]Re: [PATCH v7 1/2] merge-tree.c: add --merge-base=<commit> option

From: kylezhao (赵柯宇) <kylezhao@tencent.com>
Date: Nov 22, 2022, 03:52 UTC
Message-ID: <3e5cdd224d76499bbddf55b0b649439b@tencent.com>
In-Reply-To: <xmqqsfibsn9o.fsf@gitster.g>

Show 25 quoted lines

> > +	const char *merge_base = NULL;
> >
> >  	const char * const merge_tree_usage[] = {
> >  		N_("git merge-tree [--write-tree] [<options>] <branch1>
> > <branch2>"), @@ -515,6 +533,10 @@ int cmd_merge_tree(int argc, const
> char **argv, const char *prefix)
> >  			   &o.use_stdin,
> >  			   N_("perform multiple merges, one per line of input"),
> >  			   PARSE_OPT_NONEG),
> > +		OPT_STRING(0, "merge-base",
> > +			   &merge_base,
> > +			   N_("commit"),
> > +			   N_("specify a merge-base for the merge")),
> >  		OPT_END()
> >  	};
> 
> This looks wrong, though.
> 
> Shouldn't "git merge-tree --merge-base=X --merge-base=Y A B"
> allow you to compute the merge between A and B in a history where there are
> two merge bases?
> 
> Unfortunately this is already in 'next', so let's see an incremental fix on top.
> 
> Thanks.

I agree.

OPT_STRING only use the last value of "--merge-base". It will mislead users, they may specify the merge-base multiple times, but found it doesn't work.

I went to check the api-parse-option.txt, but I didn't found an elegant solution to stop when the users uses the
second "--merge-base".  Did I miss it? Or we just need to mention this in the git-merge-tree.txt,
such as:
 --merge-base=<commit>::
 	Instead of finding the merge-bases for <branch1> and <branch2>,
-	specify a merge-base for the merge.
+            specify a merge-base for the merge. This option doesn't support
+            being specified multiple times, only the last value you provide will be used.

Thanks, Kyle