Re: [PATCH v2] contrib/rerere-train: optionally overwrite existing resolutions

[Raman Gupta]

On 26/07/17 04:41 PM, Junio C Hamano wrote:
> Raman Gupta  writes:
> 
>> On 26/07/17 02:05 PM, Junio C Hamano wrote:
>>> I haven't tried this patch, but would this work well with options
>>> meant for the 'git rev-list --parents "$@"' that grabs the list of
>>> merge commits to learn from?  e.g.
>>>
>>> $ contrib/rerere-train.sh -n 4 --merges master
>>> $ contrib/rerere-train.sh --overwrite -n 4 --merges master
>>> $ contrib/rerere-train.sh -n 4 --overwrite --merges master
>>>
>>> I do not think it is necessary to make the last one work; as long as
>>> the first two work as expected, we are good even if the last one
>>> dies with a sensible message e.g. "options X, Y and Z must be given
>>> before other options" (currently "X, Y and Z" consists only of
>>> "--overwrite", but I think you get what I mean).
>>
>> You're right -- I didn't try all the cases. I wasn't able to figure
>> out how to get `rev-parse --parseopt` to deal with this situation, so
>> I did it manually. I'm not super-happy with the result, but it does
>> work. Look for PATCH v3.
> 
> Yes, I think you could squash the two case arms in the later loop
> into one i.e.
> 
>   -h|--help|-o|--overwrite)
>   die "please don't." ;;

I considered that but decided the non-collapsed version better
supports this list growing in the future.

> but still the repetition does look ugly.

Agreed.

> As a contrib/ material, I do not care too deeply about it, though.
> 
> Will queue.

Thanks.

Regards,
Raman

Re: [PATCH v2] contrib/rerere-train: optionally overwrite existing resolutions

[Junio C Hamano]

Raman Gupta  writes:

> On 26/07/17 02:05 PM, Junio C Hamano wrote:
>> I haven't tried this patch, but would this work well with options
>> meant for the 'git rev-list --parents "$@"' that grabs the list of
>> merge commits to learn from?  e.g.
>> 
>>  $ contrib/rerere-train.sh -n 4 --merges master
>>  $ contrib/rerere-train.sh --overwrite -n 4 --merges master
>>  $ contrib/rerere-train.sh -n 4 --overwrite --merges master
>> 
>> I do not think it is necessary to make the last one work; as long as
>> the first two work as expected, we are good even if the last one
>> dies with a sensible message e.g. "options X, Y and Z must be given
>> before other options" (currently "X, Y and Z" consists only of
>> "--overwrite", but I think you get what I mean).
>
> You're right -- I didn't try all the cases. I wasn't able to figure
> out how to get `rev-parse --parseopt` to deal with this situation, so
> I did it manually. I'm not super-happy with the result, but it does
> work. Look for PATCH v3.

Yes, I think you could squash the two case arms in the later loop
into one i.e.

-h|--help|-o|--overwrite)
die "please don't." ;;

but still the repetition does look ugly.

As a contrib/ material, I do not care too deeply about it, though.

Will queue.

Re: [PATCH v2] contrib/rerere-train: optionally overwrite existing resolutions

[Raman Gupta]

On 26/07/17 02:05 PM, Junio C Hamano wrote:
> I haven't tried this patch, but would this work well with options
> meant for the 'git rev-list --parents "$@"' that grabs the list of
> merge commits to learn from?  e.g.
> 
>   $ contrib/rerere-train.sh -n 4 --merges master
>   $ contrib/rerere-train.sh --overwrite -n 4 --merges master
>   $ contrib/rerere-train.sh -n 4 --overwrite --merges master
> 
> I do not think it is necessary to make the last one work; as long as
> the first two work as expected, we are good even if the last one
> dies with a sensible message e.g. "options X, Y and Z must be given
> before other options" (currently "X, Y and Z" consists only of
> "--overwrite", but I think you get what I mean).

You're right -- I didn't try all the cases. I wasn't able to figure
out how to get `rev-parse --parseopt` to deal with this situation, so
I did it manually. I'm not super-happy with the result, but it does
work. Look for PATCH v3.

Regards,
Raman

Re: [PATCH v2] contrib/rerere-train: optionally overwrite existing resolutions

[Junio C Hamano]

Raman Gupta  writes:

> Provide the user an option to overwrite existing resolutions using an
> `--overwrite` flag. This might be used, for example, if the user knows
> that they already have an entry in their rerere cache for a conflict,
> but wish to drop it and retrain based on the merge commit(s) passed to
> the rerere-train script.
>
> Signed-off-by: Raman Gupta 
> ---
>  contrib/rerere-train.sh | 36 ++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/rerere-train.sh b/contrib/rerere-train.sh
> index 52ad9e4..e25bf8a 100755
> --- a/contrib/rerere-train.sh
> +++ b/contrib/rerere-train.sh
> @@ -3,10 +3,38 @@
>  # Prime rerere database from existing merge commits
>  
>  me=rerere-train
> -USAGE="$me rev-list-args"
>  
>  SUBDIRECTORY_OK=Yes
> -OPTIONS_SPEC=
> +OPTS_SPEC="\
> +$me [--overwrite] 
> +--
> +h,helpshow the help
> +o,overwrite   overwrite any existing rerere cache
> +"
> +eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit 
> $?)"
> +
> +overwrite=0

It is very good that you initialize overwrite explicitly here, to
prevent it from seeping through from the caller's environment.

> +while test $# -gt 0
> +do
> + opt="$1"
> + case "$opt" in
> + -o)
> + overwrite=1
> + shift
> + shift
> + ;;
> + --)
> + shift
> + break
> + ;;
> + *)
> + break
> + exit 1
> + ;;
> + esac
> +done

I haven't tried this patch, but would this work well with options
meant for the 'git rev-list --parents "$@"' that grabs the list of
merge commits to learn from?  e.g.

$ contrib/rerere-train.sh -n 4 --merges master
$ contrib/rerere-train.sh --overwrite -n 4 --merges master
$ contrib/rerere-train.sh -n 4 --overwrite --merges master

I do not think it is necessary to make the last one work; as long as
the first two work as expected, we are good even if the last one
dies with a sensible message e.g. "options X, Y and Z must be given
before other options" (currently "X, Y and Z" consists only of
"--overwrite", but I think you get what I mean).

>  . "$(git --exec-path)/git-sh-setup"
>  require_work_tree
>  cd_to_toplevel
> @@ -34,6 +62,10 @@ do
>   # Cleanly merges
>   continue
>   fi
> + if [ $overwrite == 1 ]

if test "$overwrite" = 1

cf. Documentation/CodingGuidelines.

> + then
> + git rerere forget .
> + fi
>   if test -s "$GIT_DIR/MERGE_RR"
>   then
>   git show -s --pretty=format:"Learning from %h %s" "$commit"

[PATCH v2] contrib/rerere-train: optionally overwrite existing resolutions

[Raman Gupta]

Provide the user an option to overwrite existing resolutions using an
`--overwrite` flag. This might be used, for example, if the user knows
that they already have an entry in their rerere cache for a conflict,
but wish to drop it and retrain based on the merge commit(s) passed to
the rerere-train script.

Signed-off-by: Raman Gupta 
---
 contrib/rerere-train.sh | 36 ++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/contrib/rerere-train.sh b/contrib/rerere-train.sh
index 52ad9e4..e25bf8a 100755
--- a/contrib/rerere-train.sh
+++ b/contrib/rerere-train.sh
@@ -3,10 +3,38 @@
 # Prime rerere database from existing merge commits
 
 me=rerere-train
-USAGE="$me rev-list-args"
 
 SUBDIRECTORY_OK=Yes
-OPTIONS_SPEC=
+OPTS_SPEC="\
+$me [--overwrite] 
+--
+h,helpshow the help
+o,overwrite   overwrite any existing rerere cache
+"
+eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)"
+
+overwrite=0
+
+while test $# -gt 0
+do
+   opt="$1"
+   case "$opt" in
+   -o)
+   overwrite=1
+   shift
+   shift
+   ;;
+   --)
+   shift
+   break
+   ;;
+   *)
+   break
+   exit 1
+   ;;
+   esac
+done
+
 . "$(git --exec-path)/git-sh-setup"
 require_work_tree
 cd_to_toplevel
@@ -34,6 +62,10 @@ do
# Cleanly merges
continue
fi
+   if [ $overwrite == 1 ]
+   then
+   git rerere forget .
+   fi
if test -s "$GIT_DIR/MERGE_RR"
then
git show -s --pretty=format:"Learning from %h %s" "$commit"
-- 
2.9.4