Re: [PATCH v3 06/42] completion: use __gitcomp_builtin in _git_am

[Duy Nguyen]

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

[Junio C Hamano]

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

[Duy Nguyen]

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

[Junio C Hamano]

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

[Duy Nguyen]

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, _opt_string_list }
>> -#define OPT_UYN(s, l, v, h) { OPTION_CALLBACK, (s), (l), (v), NULL, 
>> \
>> - (h), PARSE_OPT_NOARG, 
>> _opt_tertiary }
>> +#define OPT_UYN(s, l, v, h, f)  { OPTION_CALLBACK, (s), (l), (v), NULL, 
>> \
>> + (h), PARSE_OPT_NOARG|(f), 
>> _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

[SZEDER Gábor]

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, _opt_string_list }
> -#define OPT_UYN(s, l, v, h) { OPTION_CALLBACK, (s), (l), (v), NULL, \
> - (h), PARSE_OPT_NOARG, 
> _opt_tertiary }
> +#define OPT_UYN(s, l, v, h, f)  { OPTION_CALLBACK, (s), (l), (v), NULL, \
> + (h), PARSE_OPT_NOARG|(f), 
> _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

[Nguyễn Thái Ngọc Duy]

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, _opt_string_list }
-#define OPT_UYN(s, l, v, h) { OPTION_CALLBACK, (s), (l), (v), NULL, \
- (h), PARSE_OPT_NOARG, _opt_tertiary 
}
+#define OPT_UYN(s, l, v, h, f)  { OPTION_CALLBACK, (s), (l), (v), NULL, \
+ (h), PARSE_OPT_NOARG|(f), 
_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