Re: [PATCH 1/3] checkout.c: add strict usage of -- before file_path

Re: [PATCH 1/3] checkout.c: add strict usage of -- before file_path

[Matthieu Moy]

"Dannier Castro L" <danniercl@gmail.com> wrote:

> Currently, <checkout> is a complex command able to handle both
> branches and files without any distintion other than their names,
> taking into account that depending on the type (branch or file),
> the functionality is completely different, the easier example:
> 
> $ git checkout <branch>  # Switch from current branch to <branch>.
> $ git checkout <file>    # Restore <file> from HEAD, discarding
>                         # changes if it's necessary.
> $ git checkout -- <file> # The same as the last one, only with an
>                         # useless '--'.

It's not really "useless".

In the first two commands you give, git guesses whether the first
argument is a branch or a file. In the 3rd, the -- indicates that it
must be a file.

> For GIT new users,

Nit: we write Git (for the system) or git (for the command-line tool),
but usually avoid the all-caps GIT.

> The solution consists in detect '--' into command args, allowing
> the discard of changes and considering the following names as
> file paths, otherwise, they are branch names.

This answers the "what?" question, but does not explain why this is a
good change. A good commit message should focus more on the "why?"
question.

While I agree that "git checkout" is a complex command, and would love
to see a simpler syntax at least for the most common operations, I do
not think that this is a good change for several reasons:

* If one considers that this "--" syntax is an issue, then this patch
  is a very partial solution only. Many other commands use the same
  convention (for example "git log <commit>" Vs "git log -- <file>"),
  so changing only one makes git less consistent. Also, note that this
  "--" convention is not git-specific. Try "touch --help" and "touch
  -- --help" for example.

* This breaks backward compatibility. People used to "git checkout
  <file>" won't be able to use it anymore. Scripts using it will
  break. Git avoids breaking backward compatibility, and when there's
  a good reason to do so we need a good transition plan. In this case,
  one possible plan would be to 1) issue a warning whenever "git
  checkout <file>" is used for a while, and then 2) actually forbid
  it. But following this path, I don't think step 2) would actually be
  useful.

> @@ -928,6 +931,7 @@ static int parse_branchname_arg(int argc, const char **argv,
> 	dash_dash_pos = -1;
> 	for (i = 0; i < argc; i++) {
> 		if (!strcmp(argv[i], "--")) {
> +			opts->discard_changes = 1;
> 			dash_dash_pos = i;

Wouldn't "dash_dash_pos != -1" be enough to know whether there's a --?

-- 
Matthieu Moy
https://matthieu-moy.fr/

Re: [PATCH 1/3] checkout.c: add strict usage of -- before file_path

[Duy Nguyen]

On Mon, May 14, 2018 at 04:52:53PM +0200, Duy Nguyen wrote:
> On Sun, May 13, 2018 at 11:02 PM, Kevin Daudt <me@ikke.info> wrote:
> > One data point indicating this is giving issues is that today on IRC a
> > user was confused why `git checkout pt` did not show any message and did
> > not checkout a remote branch called 'pt' as they expected. It turned out
> > they also had a local file/dir called 'pt', which caused git to checkout
> > that file/dir rather than creating a local branch based on the remote
> > branch.
> 
> Now this is something we should fix. When an argument can be
> interpreted in more than one way Git should reject it, but I think
> this ambiguation logic does not take dwim (i.e. create a new branch
> beased on remote) into account.

This is a quick draft to make this happen

-- 8< --
diff --git a/builtin/checkout.c b/builtin/checkout.c
index b49b582071..f4f6951f05 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -953,8 +953,19 @@ static int parse_branchname_arg(int argc, const char **argv,
 		 */
 		int recover_with_dwim = dwim_new_local_branch_ok;
 
-		if (!has_dash_dash &&
-		    (check_filename(opts->prefix, arg) || !no_wildcard(arg)))
+		if (!has_dash_dash && check_filename(opts->prefix, arg) &&
+		    recover_with_dwim) {
+			const char *remote = unique_tracking_name(arg, rev);
+			if (remote)
+				die(_("don't know whether to create a tracking "
+				      "branch from remote %s or to checkout path %s, "
+				      "use -- to disambiguate"),
+				    remote, arg);
+			printf("here?\n");
+			recover_with_dwim = 0;
+		}
+
+		if (!has_dash_dash && !no_wildcard(arg))
 			recover_with_dwim = 0;
 		/*
 		 * Accept "git checkout foo" and "git checkout foo --"
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index 3e5ac81bd2..ea95fb8668 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -215,4 +215,35 @@ test_expect_success 'loosely defined local base branch is reported correctly' '
 	test_cmp expect actual
 '
 
+test_expect_success 'reject when arg could be part of dwim branch' '
+	git remote add foo file://non-existent-place &&
+	git update-ref refs/remotes/foo/dwim-arg HEAD &&
+	echo foo >dwim-arg &&
+	git add dwim-arg &&
+	echo bar >dwim-arg &&
+	test_must_fail git checkout dwim-arg &&
+	test_must_fail git rev-parse refs/heads/dwim-arg -- &&
+	grep bar dwim-arg
+'
+
+test_expect_success 'disambiguate dwim branch and checkout path (1)' '
+	git update-ref refs/remotes/foo/dwim-arg1 HEAD &&
+	echo foo >dwim-arg1 &&
+	git add dwim-arg1 &&
+	echo bar >dwim-arg1 &&
+	git checkout -- dwim-arg1 &&
+	test_must_fail git rev-parse refs/heads/dwim-arg1 -- &&
+	grep foo dwim-arg1
+'
+
+test_expect_success 'disambiguate dwim branch and checkout path (2)' '
+	git update-ref refs/remotes/foo/dwim-arg2 HEAD &&
+	echo foo >dwim-arg2 &&
+	git add dwim-arg2 &&
+	echo bar >dwim-arg2 &&
+	git checkout dwim-arg2 -- &&
+	git rev-parse refs/heads/dwim-arg2 -- &&
+	grep bar dwim-arg2
+'
+
 test_done
-- 8< --

Re: [PATCH 1/3] checkout.c: add strict usage of -- before file_path

[Duy Nguyen]

On Sun, May 13, 2018 at 11:02 PM, Kevin Daudt <me@ikke.info> wrote:
> One data point indicating this is giving issues is that today on IRC a
> user was confused why `git checkout pt` did not show any message and did
> not checkout a remote branch called 'pt' as they expected. It turned out
> they also had a local file/dir called 'pt', which caused git to checkout
> that file/dir rather than creating a local branch based on the remote
> branch.

Now this is something we should fix. When an argument can be
interpreted in more than one way Git should reject it, but I think
this ambiguation logic does not take dwim (i.e. create a new branch
beased on remote) into account.
-- 
Duy

Re: [PATCH 1/3] checkout.c: add strict usage of -- before file_path

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

>
> I would like an option to revert back to current behavior. I'm not a
> new user. I know what I'm doing. Please don't make me type more.

I can guarantee that nobody will stay a newbie.  They either become
more proficient and aware of what they are doing without having to
think, at which point they start feeling the same way as you are.
Or they leave the Git ecosystem and move to better things ;-)

> And '--" is not completely useless. If you have <file> and <branch>
> with the same name, you have to give "--" to to tell git what the
> first argument means.

Correct.

We _could_ do better than the corrent code, though, when we happen
to have a file called 'master'.  "git checkout master master" cannot
mean anything other than "I want to make the index and the working
tree copies of 'master' file to match what is recorded on the master
branch", but I think we do require "git checkout master -- master"
disambiguation.

Re: [PATCH 1/3] checkout.c: add strict usage of -- before file_path

[Kevin Daudt]

On Sat, May 12, 2018 at 08:23:32PM -0600, Dannier Castro L wrote:
> Currently, <checkout> is a complex command able to handle both
> branches and files without any distintion other than their names,
> taking into account that depending on the type (branch or file),
> the functionality is completely different, the easier example:
> 
> $ git checkout <branch>  # Switch from current branch to <branch>.
> $ git checkout <file>    # Restore <file> from HEAD, discarding
>                          # changes if it's necessary.
> $ git checkout -- <file> # The same as the last one, only with an
>                          # useless '--'.
> 
> For GIT new users, this complicated versatility of <checkout> could
> be very confused, also considering that actually the flag '--' is
> completely useless (added or not, there is not any difference for
> this command), when the same program messages promote the use of
> this flag.
> 
> Regarding the <checkout>'s power to overwrite any file, discarding
> changes if they exist without some way of recovering them, the
> solution propuses that the usage of '--' is strict before to
> specify the file(s) path(s) for any <checkout> command (including
> all types of flags), as a "defense barrier" to make sure about
> user's knowledge and intension running <checkout>.
> 
> The solution consists in detect '--' into command args, allowing
> the discard of changes and considering the following names as
> file paths, otherwise, they are branch names.
> 
> Signed-off-by: Dannier Castro L <danniercl@gmail.com>

One data point indicating this is giving issues is that today on IRC a
user was confused why `git checkout pt` did not show any message and did
not checkout a remote branch called 'pt' as they expected. It turned out
they also had a local file/dir called 'pt', which caused git to checkout
that file/dir rather than creating a local branch based on the remote
branch.

Kevin

Re: [PATCH 1/3] checkout.c: add strict usage of -- before file_path

[Duy Nguyen]

On Sun, May 13, 2018 at 4:23 AM, Dannier Castro L <danniercl@gmail.com> wrote:
> Currently, <checkout> is a complex command able to handle both
> branches and files without any distintion other than their names,
> taking into account that depending on the type (branch or file),
> the functionality is completely different, the easier example:
>
> $ git checkout <branch>  # Switch from current branch to <branch>.
> $ git checkout <file>    # Restore <file> from HEAD, discarding
>                          # changes if it's necessary.
> $ git checkout -- <file> # The same as the last one, only with an
>                          # useless '--'.
>
> For GIT new users, this complicated versatility of <checkout> could
> be very confused, also considering that actually the flag '--' is
> completely useless (added or not, there is not any difference for
> this command), when the same program messages promote the use of
> this flag.

I would like an option to revert back to current behavior. I'm not a
new user. I know what I'm doing. Please don't make me type more.

And '--" is not completely useless. If you have <file> and <branch>
with the same name, you have to give "--" to to tell git what the
first argument means.

> Regarding the <checkout>'s power to overwrite any file, discarding
> changes if they exist without some way of recovering them, the
> solution propuses that the usage of '--' is strict before to
> specify the file(s) path(s) for any <checkout> command (including
> all types of flags), as a "defense barrier" to make sure about
> user's knowledge and intension running <checkout>.
>
> The solution consists in detect '--' into command args, allowing
> the discard of changes and considering the following names as
> file paths, otherwise, they are branch names.
-- 
Duy

[PATCH 1/3] checkout.c: add strict usage of -- before file_path

[Dannier Castro L]

Currently, <checkout> is a complex command able to handle both
branches and files without any distintion other than their names,
taking into account that depending on the type (branch or file),
the functionality is completely different, the easier example:

$ git checkout <branch>  # Switch from current branch to <branch>.
$ git checkout <file>    # Restore <file> from HEAD, discarding
                         # changes if it's necessary.
$ git checkout -- <file> # The same as the last one, only with an
                         # useless '--'.

For GIT new users, this complicated versatility of <checkout> could
be very confused, also considering that actually the flag '--' is
completely useless (added or not, there is not any difference for
this command), when the same program messages promote the use of
this flag.

Regarding the <checkout>'s power to overwrite any file, discarding
changes if they exist without some way of recovering them, the
solution propuses that the usage of '--' is strict before to
specify the file(s) path(s) for any <checkout> command (including
all types of flags), as a "defense barrier" to make sure about
user's knowledge and intension running <checkout>.

The solution consists in detect '--' into command args, allowing
the discard of changes and considering the following names as
file paths, otherwise, they are branch names.

Signed-off-by: Dannier Castro L <danniercl@gmail.com>
---
 builtin/checkout.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index d76e13c..ec577b3 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -40,6 +40,7 @@ struct checkout_opts {
 	int ignore_skipworktree;
 	int ignore_other_worktrees;
 	int show_progress;
+	int discard_changes;
 
 	const char *new_branch;
 	const char *new_branch_force;
@@ -885,8 +886,8 @@ static int parse_branchname_arg(int argc, const char **argv,
 	/*
 	 * case 1: git checkout <ref> -- [<paths>]
 	 *
-	 *   <ref> must be a valid tree, everything after the '--' must be
-	 *   a path.
+	 *   <ref> must be a valid tree. '--' must always be before any path,
+	 *   it means, everything after the '--' must be a path.
 	 *
 	 * case 2: git checkout -- [<paths>]
 	 *
@@ -900,26 +901,28 @@ static int parse_branchname_arg(int argc, const char **argv,
 	 *       omit at most one side), and if there is a unique merge base
 	 *       between A and B, A...B names that merge base.
 	 *
-	 *   (b) If <something> is _not_ a commit, either "--" is present
-	 *       or <something> is not a path, no -t or -b was given, and
-	 *       and there is a tracking branch whose name is <something>
-	 *       in one and only one remote, then this is a short-hand to
-	 *       fork local <something> from that remote-tracking branch.
+	 *   (b) If <something> is _not_ a commit, either "--" is present,
+	 *       no -t or -b was given, and there is a tracking branch
+	 *       whose name is <something> in one and only one remote,
+	 *       then this is a short-hand to fork local <something> from
+	 *       that remote-tracking branch.
 	 *
 	 *   (c) Otherwise, if "--" is present, treat it like case (1).
 	 *
 	 *   (d) Otherwise :
 	 *       - if it's a reference, treat it like case (1)
-	 *       - else if it's a path, treat it like case (2)
 	 *       - else: fail.
 	 *
-	 * case 4: git checkout <something> <paths>
+	 *   <something> can never be a path (at least without '--' before).
+	 *
+	 * case 4: git checkout <something> -- <paths>
 	 *
 	 *   The first argument must not be ambiguous.
 	 *   - If it's *only* a reference, treat it like case (1).
-	 *   - If it's only a path, treat it like case (2).
 	 *   - else: fail.
 	 *
+	 *   <something> can never be a path (at least without '--' before).
+	 *
 	 */
 	if (!argc)
 		return 0;
@@ -928,6 +931,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 	dash_dash_pos = -1;
 	for (i = 0; i < argc; i++) {
 		if (!strcmp(argv[i], "--")) {
+			opts->discard_changes = 1;
 			dash_dash_pos = i;
 			break;
 		}
@@ -957,8 +961,8 @@ static int parse_branchname_arg(int argc, const char **argv,
 		    (check_filename(opts->prefix, arg) || !no_wildcard(arg)))
 			recover_with_dwim = 0;
 		/*
-		 * Accept "git checkout foo" and "git checkout foo --"
-		 * as candidates for dwim.
+		 * Accept "git checkout foo_branch" and
+		 * "git checkout foo_branch --" as candidates for dwim.
 		 */
 		if (!(argc == 1 && !has_dash_dash) &&
 		    !(argc == 2 && has_dash_dash))
@@ -1254,7 +1258,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	}
 
 	UNLEAK(opts);
-	if (opts.patch_mode || opts.pathspec.nr)
+	if (opts.patch_mode || (opts.pathspec.nr && opts.discard_changes))
 		return checkout_paths(&opts, new_branch_info.name);
 	else
 		return checkout_branch(&opts, &new_branch_info);
-- 
2.7.4