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

From: Matthieu Moy <matthieu.moy@univ-lyon1.fr>
Date: May 13, 2018, 16:52 UTC
Message-ID: <191675463.13930902.1526230351223.JavaMail.zimbra@inria.fr>
In-Reply-To: <9e3a36eea5d34dc2941560b96046dc27@BPMBX2013-01.univ-lyon1.fr>

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

Show 10 quoted lines

> 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.

Show 6 quoted lines

> @@ -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/