On Tue, Apr 12, 2016 at 12:47:23PM -0700, Junio C Hamano wrote:
>Xiaolong Ye <xiaolong.ye@xxxxxxxxx> writes:
>
>> +static int config_base_commit;
>
>This variable is used as a simple boolean whose name is overly broad
>(if it were named "config_base_auto" this complaint would not
>apply). If you envision possible future enhancements for this
>configuration variable, "int config_base_commit" might make sense
>but I don't think of anything offhand that would be happy with
>"int".
>
>> @@ -786,6 +787,12 @@ static int git_format_config(const char *var, const char *value, void *cb)
>> }
>> if (!strcmp(var, "format.outputdirectory"))
>> return git_config_string(&config_output_directory, var, value);
>> + if (!strcmp(var, "format.base")){
>
>Style. s/)){/)) {/
>
>> + if (value && !strcasecmp(value, "auto")) {
>
>Does it make sense to allow "Auto" here? Given that the command
>line parsing uses strcmp() to require "auto", I do not think so.
>
>> + config_base_commit = 1;
>> + return 0;
>> + }
>
>When a value other than "auto" is given, is it sane to ignore them
>without even warning?
>
>I am wondering if this wants to be a format.useAutoBase boolean
>variable.
>
Thanks for the reminder, seems boolean variable is more suitable for
this case, I'll adopt to it in next iteration.
>> @@ -1215,7 +1222,12 @@ static void prepare_bases(struct base_tree_info *bases,
>> DIFF_OPT_SET(&diffopt, RECURSIVE);
>> diff_setup_done(&diffopt);
>>
>> - if (!strcmp(base_commit, "auto")) {
>> + if (base_commit && strcmp(base_commit, "auto")) {
>> + base = lookup_commit_reference_by_name(base_commit);
>> + if (!base)
>> + die(_("Unknown commit %s"), base_commit);
>> + oidcpy(&bases->base_commit, &base->object.oid);
>> + } else if ((base_commit && !strcmp(base_commit, "auto")) || config_base_commit) {
>
>It may be a poor design to teach prepare_bases() about "auto" thing.
>Doesn't it belong to the caller? The caller used to say "If a base
Good point, as I understand your comments, we need to extract the "auto"
thing from prepare_bases() and call it early, then we could have a
concrete base before calling prepare_bases().
Thanks,
Xiaolong.
>is given, then call that function, by the way, the base must be a
>concrete one", and with the new "auto" feature, the caller loosens
>the last part of the statement and says "If a base is given, call
>that function, but if it is specified as "auto", I'd have to compute
>it for the user before doing so".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html