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  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  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  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  and 
> 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: Re: [PATCH 1/3] checkout.c: add strict usage of -- before file_path

[Philip Oakley]

From: "Dannier Castro L" 

On 13/05/2018 00:03, Duy Nguyen wrote:


On Sun, May 13, 2018 at 4:23 AM, Dannier Castro L 
wrote:

For GIT new users, this complicated versatility of  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  and 
with the same name, you have to give "--" to to tell git what the
first argument means.


Sure Duy, you're right, probably "completely useless" is not the correct
definition, even according with the code I didn't find another useful
case that is not file and branch with the same name. The program is able
to know the type using only the name, turning "--" into an extra flag in
most of cases.

I think this solution could please you more: By default the configuration
is the current, but the user has the chance to set this, for example:

git config --global flag.strictdashdash true

Thank you so much for the spent time reviewing the patch, this is my
first one in this repository.


It maybe that after review you could suggest an appropriate rewording or
re-arrangement of the man page to better highlight the proper use of the
'--' disambiguation.

Perhaps frame the man page as if it is normal for the '--' to be included
within command lines (which should be the case for scripts anyway!).

Then indicate that it isn't mandatory if the file/branch/dwim distinction is
obvious. i.e. make sure that the man page is educational as well as being a
reference that may be misunderstood.

Those well versed in the Git cli will normally omit the '--', only using it
where necessary, however for a new users/readers of the man page, it may be
better to be more explicit and avoid future misunderstandings.

--
Philip

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,  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   # Switch from current branch to .
> $ git checkout # Restore  from HEAD, discarding
>  # changes if it's necessary.
> $ git checkout --  # The same as the last one, only with an
>  # useless '--'.
> 
> For GIT new users, this complicated versatility of  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 '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  command (including
> all types of flags), as a "defense barrier" to make sure about
> user's knowledge and intension running .
> 
> 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 

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: Re: [PATCH 1/3] checkout.c: add strict usage of -- before file_path

[SZEDER Gábor]

> On 13/05/2018 00:03, Duy Nguyen wrote:
> 
> > On Sun, May 13, 2018 at 4:23 AM, Dannier Castro L  
> > wrote:
> >> For GIT new users, this complicated versatility of  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  and 
> > with the same name, you have to give "--" to to tell git what the
> > first argument means.
> 
> Sure Duy, you're right, probably "completely useless" is not the correct
> definition,

No, "completely useless" is just plain wrong.

> even according with the code I didn't find another useful
> case that is not file and branch with the same name.

The optional disambiguating doubledash is the standard way to erm, well,
to disambiguate whatever there is to disambiguate.  Not only refs and
filenames, but also e.g. options and filenames:

  $ git checkout --option-looking-file
  error: unknown option `option-looking-file'
  usage: git checkout [] 
  <...>
  $ git checkout -- --option-looking-file
  error: pathspec '--option-looking-file' did not match any file(s) known to 
git.

Note that this is not at all specific to Git, many other programs
support the optional disambiguating doubledash as well:

  $ touch --option-looking-file
  touch: unrecognized option '--option-looking-file'
  Try 'touch --help' for more information.
  $ touch -- --option-looking-file
  $ ls --option-looking-file
  ls: unrecognized option '--option-looking-file'
  Try 'ls --help' for more information.
  $ ls -- --option-looking-file
  --option-looking-file

Please do not make it mandatory.

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

[Dannier Castro L]

On 13/05/2018 00:03, Duy Nguyen wrote:


On Sun, May 13, 2018 at 4:23 AM, Dannier Castro L  wrote:

For GIT new users, this complicated versatility of  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  and 
with the same name, you have to give "--" to to tell git what the
first argument means.


Sure Duy, you're right, probably "completely useless" is not the correct
definition, even according with the code I didn't find another useful
case that is not file and branch with the same name. The program is able
to know the type using only the name, turning "--" into an extra flag in
most of cases.

I think this solution could please you more: By default the configuration
is the current, but the user has the chance to set this, for example:

git config --global flag.strictdashdash true

Thank you so much for the spent time reviewing the patch, this is my
first one in this repository.

-Dannier CL

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

[Matthieu Moy]

"Dannier Castro L"  wrote:

> Currently,  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   # Switch from current branch to .
> $ git checkout # Restore  from HEAD, discarding
> # changes if it's necessary.
> $ git checkout --  # 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 " Vs "git log -- "),
  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
  " 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 " 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 Sun, May 13, 2018 at 4:23 AM, Dannier Castro L  wrote:
> Currently,  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   # Switch from current branch to .
> $ git checkout # Restore  from HEAD, discarding
>  # changes if it's necessary.
> $ git checkout --  # The same as the last one, only with an
>  # useless '--'.
>
> For GIT new users, this complicated versatility of  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  and 
with the same name, you have to give "--" to to tell git what the
first argument means.

> Regarding the '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  command (including
> all types of flags), as a "defense barrier" to make sure about
> user's knowledge and intension running .
>
> 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,  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   # Switch from current branch to .
$ git checkout # Restore  from HEAD, discarding
 # changes if it's necessary.
$ git checkout --  # The same as the last one, only with an
 # useless '--'.

For GIT new users, this complicated versatility of  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 '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  command (including
all types of flags), as a "defense barrier" to make sure about
user's knowledge and intension running .

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 
---
 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  -- []
 *
-*must be a valid tree, everything after the '--' must be
-*   a path.
+*must be a valid tree. '--' must always be before any path,
+*   it means, everything after the '--' must be a path.
 *
 * case 2: git checkout -- []
 *
@@ -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  is _not_ a commit, either "--" is present
-*   or  is not a path, no -t or -b was given, and
-*   and there is a tracking branch whose name is 
-*   in one and only one remote, then this is a short-hand to
-*   fork local  from that remote-tracking branch.
+*   (b) If  is _not_ a commit, either "--" is present,
+*   no -t or -b was given, and there is a tracking branch
+*   whose name is  in one and only one remote,
+*   then this is a short-hand to fork local  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  
+*can never be a path (at least without '--' before).
+*
+* case 4: git checkout  -- 
 *
 *   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.
 *
+*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