Re: [PATCH v2 1/2 / RFC] builtin/branch: stop supporting the use of --set-upstream option

["Martin Ågren" <martin.agren@gmail.com>]

On 8 August 2017 at 19:11, Kaartic Sivaraam
<kaarticsivaraam91196@gmail.com> wrote:
> The '--set-upstream' option of branch was deprecated in,
>
>     b347d06bf branch: deprecate --set-upstream and show help if we
>     detect possible mistaken use (Thu, 30 Aug 2012 19:23:13 +0200)
>
> It was deprecated for the reasons specified in the commit message of
> the referenced commit.
>
> Refactor 'branch' so that it dies with an appropraite error message
> when the '--set-upstream' is used.

appropriate. (Also, is this really a refactoring?)

>
> Note that there's a reason behind "dying with an error message" instead of
> "not accepting the '--set-upstream'". ;git branch' would still *accept*
> '--set-upstream' even after it's removal as a consequence of "unique
> prefix can be abbrievated in option names" AND '--set-upstream' is a unique
> prefix of '--set-upstream-to' when '--set-upstream' has been removed. In
> order to smooth the transition for users due to the "prefix issue" it was
> decided to make branch die when seeing the '--set-upstream' flag for a few
> years and let the users know that it would be removed some time in the future.
>
> The before/after behaviour for a simple case follows,
>
>     $ git remote
>     origin
>
> Before,
>
>     $ git branch
>     * master
>
>     $ git branch --set-upstream origin/master
>     The --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to
>     Branch origin/master set up to track local branch master.
>
>     $ echo $?
>     0
>
>     $ git branch
>     * master
>       origin/master
>
> After,
>
>     $ git branch
>     * master
>
>     $ git branch --set-upstream origin/master
>     fatal: the '--set-upstream' flag is no longer supported and will be removed. Consider using '--track' or '--set-upstream-to'
>
>     $ echo $?
>     128
>
>     $ git branch
>     * master
>
> Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
> ---
>  Changes in v2:
>
>  The previous patch removed the concerned option while the current patch
>  makes 'git branch' die on seeing the option.
>
>  The possibility of '--set-upstream' becoming an alias of  '--set-upstream-to'
>  was documented.
>
>  Documentation/git-branch.txt |  8 +++----
>  builtin/branch.c             | 21 +------------------
>  t/t3200-branch.sh            | 50 ++++----------------------------------------
>  t/t6040-tracking-info.sh     | 16 +++++++-------
>  4 files changed, 17 insertions(+), 78 deletions(-)
>
> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> index 81bd0a7b7..372107e0c 100644
> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> @@ -195,10 +195,10 @@ start-point is either a local or remote-tracking branch.
>         branch.autoSetupMerge configuration variable is true.
>
>  --set-upstream::
> -       If specified branch does not exist yet or if `--force` has been
> -       given, acts exactly like `--track`. Otherwise sets up configuration
> -       like `--track` would when creating the branch, except that where
> -       branch points to is not changed.
> +       This option is no longer supported and will be removed in the future.
> +       Consider using --track or --set-upstream-to instead.
> ++
> +Note: This could possibly become an alias of --set-upstream-to in the future.

Maybe the final note could be removed? Someone who is looking up
--set-upstream because Git just "crashed" on them will only want to know
what they should do instead. Our thoughts about the future are perhaps
not that interesting. (I sort of wonder if this option needs to be
documented at all, especially if this doesn't say anything more than
the die() just did.)

Also, I'm wondering if it should be "has been removed" instead of "will
be removed"? /Implementation-wise/, it has not been removed yet, but to
the user, it has. So maybe just "This option has been removed. Consider
using --track or --set-upstream-to instead." The same below.

I don't know if it's worth trying to use PARSE_OPT_HIDDEN in the
options-struct?

Martin

>  -u <upstream>::
>  --set-upstream-to=<upstream>::
> diff --git a/builtin/branch.c b/builtin/branch.c
> index a3bd2262b..2fcb6f7e5 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -755,8 +755,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>                 strbuf_release(&buf);
>         } else if (argc > 0 && argc <= 2) {
>                 struct branch *branch = branch_get(argv[0]);
> -               int branch_existed = 0, remote_tracking = 0;
> -               struct strbuf buf = STRBUF_INIT;
>
>                 if (!strcmp(argv[0], "HEAD"))
>                         die(_("it does not make sense to create 'HEAD' manually"));
> @@ -768,28 +766,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>                         die(_("-a and -r options to 'git branch' do not make sense with a branch name"));
>
>                 if (track == BRANCH_TRACK_OVERRIDE)
> -                       fprintf(stderr, _("The --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to\n"));
> +                       die(_("the '--set-upstream' flag is no longer supported and will be removed. Consider using '--track' or '--set-upstream-to'"));
>
> -               strbuf_addf(&buf, "refs/remotes/%s", branch->name);
> -               remote_tracking = ref_exists(buf.buf);
> -               strbuf_release(&buf);
> -
> -               branch_existed = ref_exists(branch->refname);
>                 create_branch(argv[0], (argc == 2) ? argv[1] : head,
>                               force, reflog, 0, quiet, track);
>
> -               /*
> -                * We only show the instructions if the user gave us
> -                * one branch which doesn't exist locally, but is the
> -                * name of a remote-tracking branch.
> -                */
> -               if (argc == 1 && track == BRANCH_TRACK_OVERRIDE &&
> -                   !branch_existed && remote_tracking) {
> -                       fprintf(stderr, _("\nIf you wanted to make '%s' track '%s', do this:\n\n"), head, branch->name);
> -                       fprintf(stderr, "    git branch -d %s\n", branch->name);
> -                       fprintf(stderr, "    git branch --set-upstream-to %s\n", branch->name);
> -               }
> -
>         } else
>                 usage_with_options(builtin_branch_usage, options);