[PATCH] git-parse-remote.sh: Remove op_prep argument

[PATCH] git-parse-remote.sh: Remove op_prep argument

[Siddharth Kannan]

- Remove the third argument of error_on_missing_default_upstream that is no
  longer required
- FIXME to remove this argument was added in commit 045fac5845
- Run "grep" on the rest of the codebase to find and remove occurences of
  the third argument and fix the function calls appropriately

Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
The contrib/examples/git-pull.sh file also has a variable op_prep which is used
in one of the messages shown the user. Should I remove this variable as well?

 contrib/examples/git-pull.sh | 2 +-
 git-parse-remote.sh          | 3 +--
 git-rebase.sh                | 2 +-
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/contrib/examples/git-pull.sh b/contrib/examples/git-pull.sh
index 6b3a03f..1d51dc3 100755
--- a/contrib/examples/git-pull.sh
+++ b/contrib/examples/git-pull.sh
@@ -267,7 +267,7 @@ error_on_no_merge_candidates () {
 		echo "for your current branch, you must specify a branch on the command line."
 	elif [ -z "$curr_branch" -o -z "$upstream" ]; then
 		. git-parse-remote
-		error_on_missing_default_upstream "pull" $op_type $op_prep \
+		error_on_missing_default_upstream "pull" $op_type \
 			"git pull <remote> <branch>"
 	else
 		echo "Your configuration specifies to $op_type $op_prep the ref '${upstream#refs/heads/}'"
diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index d3c3998..9698a05 100644
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -56,8 +56,7 @@ get_remote_merge_branch () {
 error_on_missing_default_upstream () {
 	cmd="$1"
 	op_type="$2"
-	op_prep="$3" # FIXME: op_prep is no longer used
-	example="$4"
+	example="$3"
 	branch_name=$(git symbolic-ref -q HEAD)
 	display_branch_name="${branch_name#refs/heads/}"
 	# If there's only one remote, use that in the suggestion
diff --git a/git-rebase.sh b/git-rebase.sh
index 04f6e44..b89f960 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -448,7 +448,7 @@ then
 		then
 			. git-parse-remote
 			error_on_missing_default_upstream "rebase" "rebase" \
-				"against" "git rebase $(gettext '<branch>')"
+				"git rebase $(gettext '<branch>')"
 		fi
 
 		test "$fork_point" = auto && fork_point=t
-- 
2.1.4

Re: [PATCH] git-parse-remote.sh: Remove op_prep argument

[Pranit Bauva]

Hey Siddharth,

On Fri, Feb 3, 2017 at 11:58 PM, Siddharth Kannan
<kannan.siddharth12@gmail.com> wrote:
> - Remove the third argument of error_on_missing_default_upstream that is no
>   longer required
> - FIXME to remove this argument was added in commit 045fac5845

This is not exactly correct. Well, this is the commit you get on
git-blame but it isn't really the one to look for. The "real" commit
when the variable was introduced was 15a147e61898 and was used for
writing out the error message. The commit 045fac5845 changed the error
message and the variable was not used then so it got redundant. So if
possible you could document all this information in the commit message
somehow, then it would be really great! :)

> - Run "grep" on the rest of the codebase to find and remove occurences of

/s/occurences/occurrences/g     (spelling mistake ;))

>   the third argument and fix the function calls appropriately
>
> Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
> ---

So if you want a better commit message then you could probably use this,
"parse-remote: remove reference to unused op_prep

This argument was introduced in the commit 15a147e618 to help in
writing out the error message but then in commit 045fac5845, the
reference to op_prep got removed. Thus the argument is no longer
useful and is removed.
"

> The contrib/examples/git-pull.sh file also has a variable op_prep which is used
> in one of the messages shown the user. Should I remove this variable as well?

Not really. We have kept the file git-pull.sh just as an example of
how git-pull was initially implemented. So previously git-pull was a
shell script which was then ported to C code. After that conversion,
the shell script was just put as it is in contrib/examples/ as a use
case of how git-pull should be implemented. I don't think there is any
need to modify it, but there isn't really a very strong reason to not
modify it (except that we don't usually write out the new changes to
it).

>  contrib/examples/git-pull.sh | 2 +-
>  git-parse-remote.sh          | 3 +--
>  git-rebase.sh                | 2 +-
>  3 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/contrib/examples/git-pull.sh b/contrib/examples/git-pull.sh
> index 6b3a03f..1d51dc3 100755
> --- a/contrib/examples/git-pull.sh
> +++ b/contrib/examples/git-pull.sh
> @@ -267,7 +267,7 @@ error_on_no_merge_candidates () {
>                 echo "for your current branch, you must specify a branch on the command line."
>         elif [ -z "$curr_branch" -o -z "$upstream" ]; then
>                 . git-parse-remote
> -               error_on_missing_default_upstream "pull" $op_type $op_prep \
> +               error_on_missing_default_upstream "pull" $op_type \
>                         "git pull <remote> <branch>"
>         else
>                 echo "Your configuration specifies to $op_type $op_prep the ref '${upstream#refs/heads/}'"
> diff --git a/git-parse-remote.sh b/git-parse-remote.sh
> index d3c3998..9698a05 100644
> --- a/git-parse-remote.sh
> +++ b/git-parse-remote.sh
> @@ -56,8 +56,7 @@ get_remote_merge_branch () {
>  error_on_missing_default_upstream () {
>         cmd="$1"
>         op_type="$2"
> -       op_prep="$3" # FIXME: op_prep is no longer used
> -       example="$4"
> +       example="$3"
>         branch_name=$(git symbolic-ref -q HEAD)
>         display_branch_name="${branch_name#refs/heads/}"
>         # If there's only one remote, use that in the suggestion
> diff --git a/git-rebase.sh b/git-rebase.sh
> index 04f6e44..b89f960 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -448,7 +448,7 @@ then
>                 then
>                         . git-parse-remote
>                         error_on_missing_default_upstream "rebase" "rebase" \
> -                               "against" "git rebase $(gettext '<branch>')"
> +                               "git rebase $(gettext '<branch>')"
>                 fi
>
>                 test "$fork_point" = auto && fork_point=t
> --
> 2.1.4
>
>

Re: [PATCH] git-parse-remote.sh: Remove op_prep argument

[Junio C Hamano]

Pranit Bauva <pranit.bauva@gmail.com> writes:

> So if you want a better commit message then you could probably use this,
> "parse-remote: remove reference to unused op_prep
>
> This argument was introduced in the commit 15a147e618 to help in
> writing out the error message but then in commit 045fac5845, the
> reference to op_prep got removed. Thus the argument is no longer
> useful and is removed.
> "

Expand the reference to commits like so:

    15a147e618 ("rebase: use @{upstream} if no upstream specified",
    2011-02-09)

Also pay attention to the subject, in which it is unclear whose
argument "op_prep" is.  Other than that, your rewrite is more
readable than the original.

    The error_on_missing_default_upstream helper function learned to
    take op_prep argument with 15a147e618 ("rebase: use @{upstream}
    if no upstream specified", 2011-02-09), but as of 045fac5845
    ("i18n: git-parse-remote.sh: mark strings for translation",
    2016-04-19), the argument no longer is used.  Remove it.

>> The contrib/examples/git-pull.sh file also has a variable op_prep which is used
>> in one of the messages shown the user. Should I remove this variable as well?
>
> Not really. We have kept the file git-pull.sh just as an example of
> how git-pull was initially implemented. So previously git-pull was a
> shell script which was then ported to C code. After that conversion,
> the shell script was just put as it is in contrib/examples/ as a use
> case of how git-pull should be implemented. 

Yes, with s/should/could/.  I agree with you that we should leave it
as-is.