{"thread":{"id":"45037","subject":"[PATCH] git-parse-remote.sh: Remove op_prep argument","startedAt":"2017-02-03T18:28:47Z","lastAt":"2017-02-04T05:09:29Z","messageCount":3,"participants":["Siddharth Kannan","Pranit Bauva","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"310812","messageId":"1486146489-8877-1-git-send-email-kannan.siddharth12@gmail.com","threadId":"45037","inReplyTo":null,"subject":"[PATCH] git-parse-remote.sh: Remove op_prep argument","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-03T18:28:09Z","receivedAt":"2017-02-03T18:28:47Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"body":"- Remove the third argument of error_on_missing_default_upstream that is no\n  longer required\n- FIXME to remove this argument was added in commit 045fac5845\n- Run \"grep\" on the rest of the codebase to find and remove occurences of\n  the third argument and fix the function calls appropriately\n\nSigned-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>\n---\nThe contrib/examples/git-pull.sh file also has a variable op_prep which is used\nin one of the messages shown the user. Should I remove this variable as well?\n\n contrib/examples/git-pull.sh | 2 +-\n git-parse-remote.sh          | 3 +--\n git-rebase.sh                | 2 +-\n 3 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/contrib/examples/git-pull.sh b/contrib/examples/git-pull.sh\nindex 6b3a03f..1d51dc3 100755\n--- a/contrib/examples/git-pull.sh\n+++ b/contrib/examples/git-pull.sh\n@@ -267,7 +267,7 @@ error_on_no_merge_candidates () {\n \t\techo \"for your current branch, you must specify a branch on the command line.\"\n \telif [ -z \"$curr_branch\" -o -z \"$upstream\" ]; then\n \t\t. git-parse-remote\n-\t\terror_on_missing_default_upstream \"pull\" $op_type $op_prep \\\n+\t\terror_on_missing_default_upstream \"pull\" $op_type \\\n \t\t\t\"git pull <remote> <branch>\"\n \telse\n \t\techo \"Your configuration specifies to $op_type $op_prep the ref '${upstream#refs/heads/}'\"\ndiff --git a/git-parse-remote.sh b/git-parse-remote.sh\nindex d3c3998..9698a05 100644\n--- a/git-parse-remote.sh\n+++ b/git-parse-remote.sh\n@@ -56,8 +56,7 @@ get_remote_merge_branch () {\n error_on_missing_default_upstream () {\n \tcmd=\"$1\"\n \top_type=\"$2\"\n-\top_prep=\"$3\" # FIXME: op_prep is no longer used\n-\texample=\"$4\"\n+\texample=\"$3\"\n \tbranch_name=$(git symbolic-ref -q HEAD)\n \tdisplay_branch_name=\"${branch_name#refs/heads/}\"\n \t# If there's only one remote, use that in the suggestion\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex 04f6e44..b89f960 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -448,7 +448,7 @@ then\n \t\tthen\n \t\t\t. git-parse-remote\n \t\t\terror_on_missing_default_upstream \"rebase\" \"rebase\" \\\n-\t\t\t\t\"against\" \"git rebase $(gettext '<branch>')\"\n+\t\t\t\t\"git rebase $(gettext '<branch>')\"\n \t\tfi\n \n \t\ttest \"$fork_point\" = auto && fork_point=t\n-- \n2.1.4\n\n\n"},{"id":"310825","messageId":"CAFZEwPMGTzVuLMSzm8wiNxvia4AV0T79C1ZTfcuO4=Bydz_zQA@mail.gmail.com","threadId":"45037","inReplyTo":"1486146489-8877-1-git-send-email-kannan.siddharth12@gmail.com","subject":"Re: [PATCH] git-parse-remote.sh: Remove op_prep argument","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2017-02-04T00:04:08Z","receivedAt":"2017-02-04T00:04:15Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Siddharth,\n\nOn Fri, Feb 3, 2017 at 11:58 PM, Siddharth Kannan\n<kannan.siddharth12@gmail.com> wrote:\n> - Remove the third argument of error_on_missing_default_upstream that is no\n>   longer required\n> - FIXME to remove this argument was added in commit 045fac5845\n\nThis is not exactly correct. Well, this is the commit you get on\ngit-blame but it isn't really the one to look for. The \"real\" commit\nwhen the variable was introduced was 15a147e61898 and was used for\nwriting out the error message. The commit 045fac5845 changed the error\nmessage and the variable was not used then so it got redundant. So if\npossible you could document all this information in the commit message\nsomehow, then it would be really great! :)\n\n> - Run \"grep\" on the rest of the codebase to find and remove occurences of\n\n/s/occurences/occurrences/g     (spelling mistake ;))\n\n>   the third argument and fix the function calls appropriately\n>\n> Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>\n> ---\n\nSo if you want a better commit message then you could probably use this,\n\"parse-remote: remove reference to unused op_prep\n\nThis argument was introduced in the commit 15a147e618 to help in\nwriting out the error message but then in commit 045fac5845, the\nreference to op_prep got removed. Thus the argument is no longer\nuseful and is removed.\n\"\n\n> The contrib/examples/git-pull.sh file also has a variable op_prep which is used\n> in one of the messages shown the user. Should I remove this variable as well?\n\nNot really. We have kept the file git-pull.sh just as an example of\nhow git-pull was initially implemented. So previously git-pull was a\nshell script which was then ported to C code. After that conversion,\nthe shell script was just put as it is in contrib/examples/ as a use\ncase of how git-pull should be implemented. I don't think there is any\nneed to modify it, but there isn't really a very strong reason to not\nmodify it (except that we don't usually write out the new changes to\nit).\n\n>  contrib/examples/git-pull.sh | 2 +-\n>  git-parse-remote.sh          | 3 +--\n>  git-rebase.sh                | 2 +-\n>  3 files changed, 3 insertions(+), 4 deletions(-)\n>\n> diff --git a/contrib/examples/git-pull.sh b/contrib/examples/git-pull.sh\n> index 6b3a03f..1d51dc3 100755\n> --- a/contrib/examples/git-pull.sh\n> +++ b/contrib/examples/git-pull.sh\n> @@ -267,7 +267,7 @@ error_on_no_merge_candidates () {\n>                 echo \"for your current branch, you must specify a branch on the command line.\"\n>         elif [ -z \"$curr_branch\" -o -z \"$upstream\" ]; then\n>                 . git-parse-remote\n> -               error_on_missing_default_upstream \"pull\" $op_type $op_prep \\\n> +               error_on_missing_default_upstream \"pull\" $op_type \\\n>                         \"git pull <remote> <branch>\"\n>         else\n>                 echo \"Your configuration specifies to $op_type $op_prep the ref '${upstream#refs/heads/}'\"\n> diff --git a/git-parse-remote.sh b/git-parse-remote.sh\n> index d3c3998..9698a05 100644\n> --- a/git-parse-remote.sh\n> +++ b/git-parse-remote.sh\n> @@ -56,8 +56,7 @@ get_remote_merge_branch () {\n>  error_on_missing_default_upstream () {\n>         cmd=\"$1\"\n>         op_type=\"$2\"\n> -       op_prep=\"$3\" # FIXME: op_prep is no longer used\n> -       example=\"$4\"\n> +       example=\"$3\"\n>         branch_name=$(git symbolic-ref -q HEAD)\n>         display_branch_name=\"${branch_name#refs/heads/}\"\n>         # If there's only one remote, use that in the suggestion\n> diff --git a/git-rebase.sh b/git-rebase.sh\n> index 04f6e44..b89f960 100755\n> --- a/git-rebase.sh\n> +++ b/git-rebase.sh\n> @@ -448,7 +448,7 @@ then\n>                 then\n>                         . git-parse-remote\n>                         error_on_missing_default_upstream \"rebase\" \"rebase\" \\\n> -                               \"against\" \"git rebase $(gettext '<branch>')\"\n> +                               \"git rebase $(gettext '<branch>')\"\n>                 fi\n>\n>                 test \"$fork_point\" = auto && fork_point=t\n> --\n> 2.1.4\n>\n>\n"},{"id":"310832","messageId":"xmqqd1ey8rul.fsf@gitster.mtv.corp.google.com","threadId":"45037","inReplyTo":"CAFZEwPMGTzVuLMSzm8wiNxvia4AV0T79C1ZTfcuO4=Bydz_zQA@mail.gmail.com","subject":"Re: [PATCH] git-parse-remote.sh: Remove op_prep argument","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-04T05:09:22Z","receivedAt":"2017-02-04T05:09:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva <pranit.bauva@gmail.com> writes:\n\n> So if you want a better commit message then you could probably use this,\n> \"parse-remote: remove reference to unused op_prep\n>\n> This argument was introduced in the commit 15a147e618 to help in\n> writing out the error message but then in commit 045fac5845, the\n> reference to op_prep got removed. Thus the argument is no longer\n> useful and is removed.\n> \"\n\nExpand the reference to commits like so:\n\n    15a147e618 (\"rebase: use @{upstream} if no upstream specified\",\n    2011-02-09)\n\nAlso pay attention to the subject, in which it is unclear whose\nargument \"op_prep\" is.  Other than that, your rewrite is more\nreadable than the original.\n\n    The error_on_missing_default_upstream helper function learned to\n    take op_prep argument with 15a147e618 (\"rebase: use @{upstream}\n    if no upstream specified\", 2011-02-09), but as of 045fac5845\n    (\"i18n: git-parse-remote.sh: mark strings for translation\",\n    2016-04-19), the argument no longer is used.  Remove it.\n\n>> The contrib/examples/git-pull.sh file also has a variable op_prep which is used\n>> in one of the messages shown the user. Should I remove this variable as well?\n>\n> Not really. We have kept the file git-pull.sh just as an example of\n> how git-pull was initially implemented. So previously git-pull was a\n> shell script which was then ported to C code. After that conversion,\n> the shell script was just put as it is in contrib/examples/ as a use\n> case of how git-pull should be implemented. \n\nYes, with s/should/could/.  I agree with you that we should leave it\nas-is.\n"}]}