Re: [PATCH v3 06/42] completion: use __gitcomp_builtin in _git_am
On Sat, Feb 24, 2018 at 1:08 AM, Junio C Hamano wrote: > Duy Nguyen writes: > >> On Fri, Feb 23, 2018 at 1:19 AM, Junio C Hamano wrote: >>> Duy Nguyen writes: >>> Now that you mention it, the only command that completes --rerere-autoupdate is git-merge. Since this is "auto" I don't think people want to type manually. >>> >>> Sorry, but I do not quite get the connection between "since this is >>> 'auto'" and the rest of the sentence. Is it just it is so lengthy >>> that people do not want to type and are likely to use completion? >> >> Well, if it is to be done automatically, I should not need to tell it >> manually (by typing the option on command line). Granted it's a weak >> argument. > > Perhaps I am not reading what the option does correctly, but my > understanding is that merge operations: > > - always allow rerere to intervene and auto-resolve in the working >tree, as long as rerere is enabled; > > - by default they however do not add the rerere-resolved result to >the index, so that the combined diff output from "git diff" can >be used as a way to sanity check the result; but > > - if the user says --rerere-autoupdate, they add the >rerere-resolved result to the index, letting the user blindly >trust it, so that such a trusting user can even commit the result >without looking at it by merely making sure there is no path >remaining in the "git ls-files -u" output. > > The "autoupdate" could be somewhat dangerous depending on your > workflow, and that is why the user can selectively enable it via the > command line option (when known to be safe) even if the user does > not set rerere.autoupdate to true. > > So I am not sure I understand "an option to allow something to be > done automatically should not be given manually" as a valid argument > at all, whether it is weak or strong. > > Or am I grossly confused? Nope. It's just me not understanding rerere functionality. -- Duy
Re: [PATCH v3 06/42] completion: use __gitcomp_builtin in _git_am
Duy Nguyen writes: > On Fri, Feb 23, 2018 at 1:19 AM, Junio C Hamano wrote: >> Duy Nguyen writes: >> >>> Now that you mention it, the only command that completes >>> --rerere-autoupdate is git-merge. Since this is "auto" I don't think >>> people want to type manually. >> >> Sorry, but I do not quite get the connection between "since this is >> 'auto'" and the rest of the sentence. Is it just it is so lengthy >> that people do not want to type and are likely to use completion? > > Well, if it is to be done automatically, I should not need to tell it > manually (by typing the option on command line). Granted it's a weak > argument. Perhaps I am not reading what the option does correctly, but my understanding is that merge operations: - always allow rerere to intervene and auto-resolve in the working tree, as long as rerere is enabled; - by default they however do not add the rerere-resolved result to the index, so that the combined diff output from "git diff" can be used as a way to sanity check the result; but - if the user says --rerere-autoupdate, they add the rerere-resolved result to the index, letting the user blindly trust it, so that such a trusting user can even commit the result without looking at it by merely making sure there is no path remaining in the "git ls-files -u" output. The "autoupdate" could be somewhat dangerous depending on your workflow, and that is why the user can selectively enable it via the command line option (when known to be safe) even if the user does not set rerere.autoupdate to true. So I am not sure I understand "an option to allow something to be done automatically should not be given manually" as a valid argument at all, whether it is weak or strong. Or am I grossly confused?
Re: [PATCH v3 06/42] completion: use __gitcomp_builtin in _git_am
On Fri, Feb 23, 2018 at 1:19 AM, Junio C Hamano wrote: > Duy Nguyen writes: > >> Now that you mention it, the only command that completes >> --rerere-autoupdate is git-merge. Since this is "auto" I don't think >> people want to type manually. > > Sorry, but I do not quite get the connection between "since this is > 'auto'" and the rest of the sentence. Is it just it is so lengthy > that people do not want to type and are likely to use completion? Well, if it is to be done automatically, I should not need to tell it manually (by typing the option on command line). Granted it's a weak argument. >> Maybe I should separate these changes >> _and_ remove --rerere-autoupdate from _git_merge() too? At least that >> it will be consistent that way. > > H. Why not complete this option? Is it because the current > completion script does not and we are trying to preserve the > behaviour? I do not have a strong opinion either way, but just > trying to understand the reasoning behind the choice. There's not a strong argument for not completing this option really. It may belong to the unpopular category and adding it may make people more for other options. But I think it's way too early to decide that. And even if that's true, I think we should let the user choose to not complete certain options they don't like. It's good that this is spotted. I think I'll just keep all --rerere-autoupdate completable. Well, v4's coming (in a few days)... -- Duy
Re: [PATCH v3 06/42] completion: use __gitcomp_builtin in _git_am
Duy Nguyen writes: > Now that you mention it, the only command that completes > --rerere-autoupdate is git-merge. Since this is "auto" I don't think > people want to type manually. Sorry, but I do not quite get the connection between "since this is 'auto'" and the rest of the sentence. Is it just it is so lengthy that people do not want to type and are likely to use completion? > Maybe I should separate these changes > _and_ remove --rerere-autoupdate from _git_merge() too? At least that > it will be consistent that way. H. Why not complete this option? Is it because the current completion script does not and we are trying to preserve the behaviour? I do not have a strong opinion either way, but just trying to understand the reasoning behind the choice. >>> diff --git a/rerere.h b/rerere.h >>> index c2961feaaa..5e5a312e4c 100644 >>> --- a/rerere.h >>> +++ b/rerere.h >>> @@ -37,6 +37,7 @@ extern void rerere_clear(struct string_list *); >>> extern void rerere_gc(struct string_list *); >>> >>> #define OPT_RERERE_AUTOUPDATE(v) OPT_UYN(0, "rerere-autoupdate", (v), \ >>> - N_("update the index with reused conflict resolution if possible")) >>> + N_("update the index with reused conflict resolution if possible"), >>> \ >>> + PARSE_OPT_NOCOMPLETE) >>> >>> #endif >>> -- >>> 2.16.1.207.gedba492059 >>>
Re: [PATCH v3 06/42] completion: use __gitcomp_builtin in _git_am
On Wed, Feb 14, 2018 at 7:53 PM, SZEDER Gábor wrote: > On Fri, Feb 9, 2018 at 12:01 PM, Nguyễn Thái Ngọc Duy > wrote: >> The new completable options are: >> >> --directory >> --exclude >> --gpg-sign >> --include >> --keep-cr >> --keep-non-patch >> --message-id >> --no-keep-cr >> --patch-format >> --quiet >> --reject >> --resolvemsg= >> >> In-progress options like --continue will be part of --git-completion-helper >> then filtered out by _git_am() unless the operation is in progress. This >> helps keep marking of these operations in just one place. >> >> Signed-off-by: Nguyễn Thái Ngọc Duy >> --- >> contrib/completion/git-completion.bash | 11 --- >> parse-options.h| 4 ++-- >> rerere.h | 3 ++- >> 3 files changed, 8 insertions(+), 10 deletions(-) >> >> diff --git a/contrib/completion/git-completion.bash >> b/contrib/completion/git-completion.bash >> index 1e0bd835fe..eba482eb9c 100644 >> --- a/contrib/completion/git-completion.bash >> +++ b/contrib/completion/git-completion.bash >> @@ -1105,12 +1105,13 @@ __git_count_arguments () >> } >> >> __git_whitespacelist="nowarn warn error error-all fix" >> +__git_am_inprogress_options="--skip --continue --resolved --abort" >> >> _git_am () >> { >> __git_find_repo_path >> if [ -d "$__git_repo_path"/rebase-apply ]; then >> - __gitcomp "--skip --continue --resolved --abort" >> + __gitcomp "$__git_am_inprogress_options" >> return >> fi >> case "$cur" in >> @@ -1119,12 +1120,8 @@ _git_am () >> return >> ;; >> --*) >> - __gitcomp " >> - --3way --committer-date-is-author-date --ignore-date >> - --ignore-whitespace --ignore-space-change >> - --interactive --keep --no-utf8 --signoff --utf8 >> - --whitespace= --scissors >> - " >> + __gitcomp_builtin am "--no-utf8" \ >> + "$__git_am_inprogress_options" >> return >> esac >> } >> diff --git a/parse-options.h b/parse-options.h >> index 3c32401736..009cd863e5 100644 >> --- a/parse-options.h >> +++ b/parse-options.h >> @@ -144,8 +144,8 @@ struct option { >> #define OPT_STRING_LIST(s, l, v, a, h) \ >> { OPTION_CALLBACK, (s), (l), (v), (a), \ >> (h), 0, &parse_opt_string_list } >> -#define OPT_UYN(s, l, v, h) { OPTION_CALLBACK, (s), (l), (v), NULL, >> \ >> - (h), PARSE_OPT_NOARG, >> &parse_opt_tertiary } >> +#define OPT_UYN(s, l, v, h, f) { OPTION_CALLBACK, (s), (l), (v), NULL, >> \ >> + (h), PARSE_OPT_NOARG|(f), >> &parse_opt_tertiary } >> #define OPT_DATE(s, l, v, h) \ >> { OPTION_CALLBACK, (s), (l), (v), N_("time"),(h), 0,\ >> parse_opt_approxidate_cb } > > Shouldn't this hunk go into a commit of its own? Or at least it would > deserve a mention in the commit message. It's not a standalone change. It is used by the OPT_RERERE_AUTOUPDATE below, which in turn is used by git-add. Together, --rerere-autoupdate is removed from the completion list of git-add (and also a few more commands). Now that you mention it, the only command that completes --rerere-autoupdate is git-merge. Since this is "auto" I don't think people want to type manually. Maybe I should separate these changes _and_ remove --rerere-autoupdate from _git_merge() too? At least that it will be consistent that way. >> diff --git a/rerere.h b/rerere.h >> index c2961feaaa..5e5a312e4c 100644 >> --- a/rerere.h >> +++ b/rerere.h >> @@ -37,6 +37,7 @@ extern void rerere_clear(struct string_list *); >> extern void rerere_gc(struct string_list *); >> >> #define OPT_RERERE_AUTOUPDATE(v) OPT_UYN(0, "rerere-autoupdate", (v), \ >> - N_("update the index with reused conflict resolution if possible")) >> + N_("update the index with reused conflict resolution if possible"), \ >> + PARSE_OPT_NOCOMPLETE) >> >> #endif >> -- >> 2.16.1.207.gedba492059 >> -- Duy
Re: [PATCH v3 06/42] completion: use __gitcomp_builtin in _git_am
On Fri, Feb 9, 2018 at 12:01 PM, Nguyễn Thái Ngọc Duy wrote: > The new completable options are: > > --directory > --exclude > --gpg-sign > --include > --keep-cr > --keep-non-patch > --message-id > --no-keep-cr > --patch-format > --quiet > --reject > --resolvemsg= > > In-progress options like --continue will be part of --git-completion-helper > then filtered out by _git_am() unless the operation is in progress. This > helps keep marking of these operations in just one place. > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > contrib/completion/git-completion.bash | 11 --- > parse-options.h| 4 ++-- > rerere.h | 3 ++- > 3 files changed, 8 insertions(+), 10 deletions(-) > > diff --git a/contrib/completion/git-completion.bash > b/contrib/completion/git-completion.bash > index 1e0bd835fe..eba482eb9c 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -1105,12 +1105,13 @@ __git_count_arguments () > } > > __git_whitespacelist="nowarn warn error error-all fix" > +__git_am_inprogress_options="--skip --continue --resolved --abort" > > _git_am () > { > __git_find_repo_path > if [ -d "$__git_repo_path"/rebase-apply ]; then > - __gitcomp "--skip --continue --resolved --abort" > + __gitcomp "$__git_am_inprogress_options" > return > fi > case "$cur" in > @@ -1119,12 +1120,8 @@ _git_am () > return > ;; > --*) > - __gitcomp " > - --3way --committer-date-is-author-date --ignore-date > - --ignore-whitespace --ignore-space-change > - --interactive --keep --no-utf8 --signoff --utf8 > - --whitespace= --scissors > - " > + __gitcomp_builtin am "--no-utf8" \ > + "$__git_am_inprogress_options" > return > esac > } > diff --git a/parse-options.h b/parse-options.h > index 3c32401736..009cd863e5 100644 > --- a/parse-options.h > +++ b/parse-options.h > @@ -144,8 +144,8 @@ struct option { > #define OPT_STRING_LIST(s, l, v, a, h) \ > { OPTION_CALLBACK, (s), (l), (v), (a), \ > (h), 0, &parse_opt_string_list } > -#define OPT_UYN(s, l, v, h) { OPTION_CALLBACK, (s), (l), (v), NULL, \ > - (h), PARSE_OPT_NOARG, > &parse_opt_tertiary } > +#define OPT_UYN(s, l, v, h, f) { OPTION_CALLBACK, (s), (l), (v), NULL, \ > + (h), PARSE_OPT_NOARG|(f), > &parse_opt_tertiary } > #define OPT_DATE(s, l, v, h) \ > { OPTION_CALLBACK, (s), (l), (v), N_("time"),(h), 0,\ > parse_opt_approxidate_cb } Shouldn't this hunk go into a commit of its own? Or at least it would deserve a mention in the commit message. > diff --git a/rerere.h b/rerere.h > index c2961feaaa..5e5a312e4c 100644 > --- a/rerere.h > +++ b/rerere.h > @@ -37,6 +37,7 @@ extern void rerere_clear(struct string_list *); > extern void rerere_gc(struct string_list *); > > #define OPT_RERERE_AUTOUPDATE(v) OPT_UYN(0, "rerere-autoupdate", (v), \ > - N_("update the index with reused conflict resolution if possible")) > + N_("update the index with reused conflict resolution if possible"), \ > + PARSE_OPT_NOCOMPLETE) > > #endif > -- > 2.16.1.207.gedba492059 >
[PATCH v3 06/42] completion: use __gitcomp_builtin in _git_am
The new completable options are: --directory --exclude --gpg-sign --include --keep-cr --keep-non-patch --message-id --no-keep-cr --patch-format --quiet --reject --resolvemsg= In-progress options like --continue will be part of --git-completion-helper then filtered out by _git_am() unless the operation is in progress. This helps keep marking of these operations in just one place. Signed-off-by: Nguyễn Thái Ngọc Duy --- contrib/completion/git-completion.bash | 11 --- parse-options.h| 4 ++-- rerere.h | 3 ++- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 1e0bd835fe..eba482eb9c 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -1105,12 +1105,13 @@ __git_count_arguments () } __git_whitespacelist="nowarn warn error error-all fix" +__git_am_inprogress_options="--skip --continue --resolved --abort" _git_am () { __git_find_repo_path if [ -d "$__git_repo_path"/rebase-apply ]; then - __gitcomp "--skip --continue --resolved --abort" + __gitcomp "$__git_am_inprogress_options" return fi case "$cur" in @@ -1119,12 +1120,8 @@ _git_am () return ;; --*) - __gitcomp " - --3way --committer-date-is-author-date --ignore-date - --ignore-whitespace --ignore-space-change - --interactive --keep --no-utf8 --signoff --utf8 - --whitespace= --scissors - " + __gitcomp_builtin am "--no-utf8" \ + "$__git_am_inprogress_options" return esac } diff --git a/parse-options.h b/parse-options.h index 3c32401736..009cd863e5 100644 --- a/parse-options.h +++ b/parse-options.h @@ -144,8 +144,8 @@ struct option { #define OPT_STRING_LIST(s, l, v, a, h) \ { OPTION_CALLBACK, (s), (l), (v), (a), \ (h), 0, &parse_opt_string_list } -#define OPT_UYN(s, l, v, h) { OPTION_CALLBACK, (s), (l), (v), NULL, \ - (h), PARSE_OPT_NOARG, &parse_opt_tertiary } +#define OPT_UYN(s, l, v, h, f) { OPTION_CALLBACK, (s), (l), (v), NULL, \ + (h), PARSE_OPT_NOARG|(f), &parse_opt_tertiary } #define OPT_DATE(s, l, v, h) \ { OPTION_CALLBACK, (s), (l), (v), N_("time"),(h), 0,\ parse_opt_approxidate_cb } diff --git a/rerere.h b/rerere.h index c2961feaaa..5e5a312e4c 100644 --- a/rerere.h +++ b/rerere.h @@ -37,6 +37,7 @@ extern void rerere_clear(struct string_list *); extern void rerere_gc(struct string_list *); #define OPT_RERERE_AUTOUPDATE(v) OPT_UYN(0, "rerere-autoupdate", (v), \ - N_("update the index with reused conflict resolution if possible")) + N_("update the index with reused conflict resolution if possible"), \ + PARSE_OPT_NOCOMPLETE) #endif -- 2.16.1.207.gedba492059