Hi Junio, I got your points now, I will upload another patch soon. Thank you. Regards Lin -----Original Message----- From: lin.sun@xxxxxxx <lin.sun@xxxxxxx> Sent: Tuesday, July 7, 2020 14:39 To: 'Junio C Hamano' <gitster@xxxxxxxxx>; 'sunlin via GitGitGadget' <gitgitgadget@xxxxxxxxx> Cc: git@xxxxxxxxxxxxxxx; 'sunlin' <sunlin7@xxxxxxxxx> Subject: RE: [PATCH v10] Support auto-merge for meld to follow the vim-diff behavior Hi Junio, It seems our mails crossed. I'll read your comments in this mail and reply you later. Lin -----Original Message----- From: Junio C Hamano <gitster@xxxxxxxxx> Sent: Tuesday, July 7, 2020 14:26 To: sunlin via GitGitGadget <gitgitgadget@xxxxxxxxx> Cc: git@xxxxxxxxxxxxxxx; sunlin <sunlin7@xxxxxxxxx>; Lin Sun <lin.sun@xxxxxxx> Subject: Re: [PATCH v10] Support auto-merge for meld to follow the vim-diff behavior "sunlin via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > + meld_use_auto_merge_option=$(git config mergetool.meld.useAutoMerge) > + case "$meld_use_auto_merge_option" in > + "") > + meld_use_auto_merge_option=false > + ;; I somehow thought that I already pointed out that this is wrong, didn't I? You cannot tell between a "[section] var" (which is "true") and not having any "[section] var = val" (which I think you are trying to treat as "not configured---do not use") from the output of "git config section.var". Perhaps our mails crossed? > + [Tt]ure|TRUE) > + meld_use_auto_merge_option=true > + ;; > + [Ff]alse|FALSE) > + meld_use_auto_merge_option=false > + ;; These are probably premature optimizations. > + [Aa]uto|AUTO) Sigh. I somehow thought that I already said we shouldn't do this "aCCEpt AnY CaSES" unless all other variables that take 'auto' take it case insensitively. > + # testing the "--auto-merge" option only if config is "auto" > + init_meld_help_msg > + > + case "$meld_help_msg" in > + *"--auto-merge"*) > + meld_use_auto_merge_option=true > + ;; > + *) > + meld_use_auto_merge_option=false > + ;; > + esac > + ;; > + *) > + # try detect boolean for 'on'||'yes'||numberic value > + bool_value=$(git config --bool mergetool.meld.useAutoMerge 2>/dev/null) > + if test -n "$bool_value" > + then > + meld_use_auto_merge_option="$bool_value" > + else > + meld_use_auto_merge_option=false I think this case (i.e. set to a non-bool value, and we do not recognise because it is not 'auto') should be flagged as an error, instead of treated as a silent "do not use", as it would leave the user scratching his or her head without realizing that there is a typo in the configuration file. > + fi > + ;; > + esac > fi > } > > base-commit: 07d8ea56f2ecb64b75b92264770c0a664231ce17
