Improve support for 'git config gc.reflogExpire never'

2019-03-08 Thread Mikko Rantalainen
If I configure bare repo with

git config gc.pruneExpire never
git config gc.reflogExpire never

then git will never garbage collect any commit ever stored in the repo.
This is what I want.

However, all commits referenced only via reflog are kept as loose
objects and will not be included in packs. In long run this will cause

warning: There are too many unreachable loose objects; run 'git prune'
to remove them.

and the performance of the repository will slowly decrease.


If I have understood correctly, git should notice that reflog will keep
referenced commits forever and include all those commits in packs
instead of keeping those as loose forever.

A more generic behavior might be to always compress all loose commits in
(one?) special pack so the performance would stay good even if
gc.reflogExpire is very long instead of "never".


Discussion about this case:
https://stackoverflow.com/questions/28092485/how-to-prevent-garbage-collection-in-git
https://stackoverflow.com/questions/55043693/is-it-possible-to-get-git-gc-to-pack-reflog-objects

-- 
Mikko


Re: Error fetching submodule from submodule

2019-03-08 Thread Jesper Rønn-Jensen
Thanks Jeff

You are completely right! It works as I expect if I remember the extra
parameter `--recursive` when doing `git submodule update --init
--recursive`

Thanks a lot for your feedback! This is really useful! I learned a
useful thing today :)

/Jesper

On Thu, Mar 7, 2019 at 7:08 PM Jeff King  wrote:
>
> On Thu, Mar 07, 2019 at 12:07:21PM +0100, Jesper Rønn-Jensen wrote:
>
> > Hi I think I may have found an error in the way git handles a
> > submodule's submodule. Read further for the example (extracted from a
> > real project).
>
> First off, thank you for the example script. It made understanding
> what's going on so much easier. :)
>
> > * I have a main repository which has some submodules defined.
> > * One of the submodules is a common submodule which is also included
> > in one of the other submodules
> > * When running `git fetch --recurse-submodules` I get an error.
>
> I think the presence of common_submodule in the main repo is actually a
> red herring. if you remove the last two lines of this stanza:
>
> > git setup main_repos
> > pushd main_repos
> > git submodule add ../common_submodule
> > git commit -m 'added submodule to main_repos'
>
> the outcome is the same.
>
> > # This line fails with error code 1 "Could not access submodule
> > 'common_submodule'"
> > git fetch --recurse-submodules
>
> It looks like "fetch" is smart enough to initialize a submodule when
> necessary, but not smart enough to do so recursively. If I replace that
> line with:
>
>   git submodule update --init --recursive
>
> then it works as I'd expect. Likewise, cloning the repository with:
>
>   git clone --recurse-submodules main_repos foo
>
> does what you'd want.
>
> After that, I think "git fetch --recurse-submodules" does what you want,
> because the submodule repository is already initialized.
>
> I'm not sure to what degree git-fetch intended to support initializing
> submodules. But it certainly seems like a bug that it handles the top
> layer but does not recurse (i.e., it should either handle all or none).
>
> Hopefully the commands above at least give you a workaround.
>
> -Peff



-- 

Jesper Rønn-Jensen
Nine A/S
Mobile: +45 2989 1822
Blog http://justaddwater.dk/
jespe...@gmail.com (Private e-mail and Google Talk IM)


Re: [PATCH v3 1/2] worktree: fix worktree add race.

2019-03-08 Thread Duy Nguyen
Junio, it seems 2/2 is stuck in an endless discussion. But 1/2 is good
regardless, maybe pick it up now and let 2/2 come later whenever it's
ready?

On Wed, Feb 20, 2019 at 11:16 PM Michal Suchanek  wrote:
>
> Git runs a stat loop to find a worktree name that's available and then does
> mkdir on the found name. Turn it to mkdir loop to avoid another invocation of
> worktree add finding the same free name and creating the directory first.
>
> Signed-off-by: Michal Suchanek 
> ---
> v2:
> - simplify loop exit condition
> - exit early if the mkdir fails for reason other than already present
> worktree
> - make counter unsigned
> ---
>  builtin/worktree.c | 12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 3f9907fcc994..85a604cfe98c 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -268,10 +268,10 @@ static int add_worktree(const char *path, const char 
> *refname,
> struct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT;
> struct strbuf sb = STRBUF_INIT;
> const char *name;
> -   struct stat st;
> struct child_process cp = CHILD_PROCESS_INIT;
> struct argv_array child_env = ARGV_ARRAY_INIT;
> -   int counter = 0, len, ret;
> +   unsigned int counter = 0;
> +   int len, ret;
> struct strbuf symref = STRBUF_INIT;
> struct commit *commit = NULL;
> int is_branch = 0;
> @@ -295,8 +295,12 @@ static int add_worktree(const char *path, const char 
> *refname,
> if (safe_create_leading_directories_const(sb_repo.buf))
> die_errno(_("could not create leading directories of '%s'"),
>   sb_repo.buf);
> -   while (!stat(sb_repo.buf, &st)) {
> +
> +   while (mkdir(sb_repo.buf, 0777)) {
> counter++;
> +   if ((errno != EEXIST) || !counter /* overflow */)
> +   die_errno(_("could not create directory of '%s'"),
> + sb_repo.buf);
> strbuf_setlen(&sb_repo, len);
> strbuf_addf(&sb_repo, "%d", counter);
> }
> @@ -306,8 +310,6 @@ static int add_worktree(const char *path, const char 
> *refname,
> atexit(remove_junk);
> sigchain_push_common(remove_junk_on_signal);
>
> -   if (mkdir(sb_repo.buf, 0777))
> -   die_errno(_("could not create directory of '%s'"), 
> sb_repo.buf);
> junk_git_dir = xstrdup(sb_repo.buf);
> is_junk = 1;
>
> --
> 2.20.1
>


-- 
Duy


[PATCH v5 1/1] worktree add: sanitize worktree names

2019-03-08 Thread Nguyễn Thái Ngọc Duy
Worktree names are based on $(basename $GIT_WORK_TREE). They aren't
significant until 3a3b9d8cde (refs: new ref types to make per-worktree
refs visible to all worktrees - 2018-10-21), where worktree name could
be part of a refname and must follow refname rules.

Update 'worktree add' code to remove special characters to follow
these rules. In the future the user will be able to specify the
worktree name by themselves if they're not happy with this dumb
character substitution.

Reported-by: Konstantin Kharlamov 
Helped-by: Jeff King 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/worktree.c  |  10 +++-
 refs.c  | 103 
 refs.h  |   6 +++
 t/t2400-worktree-add.sh |   5 ++
 4 files changed, 104 insertions(+), 20 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 6cc094a453..756cf3a417 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -275,6 +275,7 @@ static int add_worktree(const char *path, const char 
*refname,
struct strbuf symref = STRBUF_INIT;
struct commit *commit = NULL;
int is_branch = 0;
+   struct strbuf sb_name = STRBUF_INIT;
 
validate_worktree_add(path, opts);
 
@@ -290,7 +291,13 @@ static int add_worktree(const char *path, const char 
*refname,
die(_("invalid reference: %s"), refname);
 
name = worktree_basename(path, &len);
-   git_path_buf(&sb_repo, "worktrees/%.*s", (int)(path + len - name), 
name);
+   strbuf_add(&sb, name, path + len - name);
+   sanitize_refname_component(sb.buf, &sb_name);
+   if (!sb_name.len)
+   BUG("How come '%s' becomes empty after sanitization?", sb.buf);
+   strbuf_reset(&sb);
+   name = sb_name.buf;
+   git_path_buf(&sb_repo, "worktrees/%s", name);
len = sb_repo.len;
if (safe_create_leading_directories_const(sb_repo.buf))
die_errno(_("could not create leading directories of '%s'"),
@@ -416,6 +423,7 @@ static int add_worktree(const char *path, const char 
*refname,
strbuf_release(&symref);
strbuf_release(&sb_repo);
strbuf_release(&sb_git);
+   strbuf_release(&sb_name);
return ret;
 }
 
diff --git a/refs.c b/refs.c
index 142888a40a..e9f83018f0 100644
--- a/refs.c
+++ b/refs.c
@@ -72,30 +72,57 @@ static unsigned char refname_disposition[256] = {
  * - it ends with ".lock", or
  * - it contains a "@{" portion
  */
-static int check_refname_component(const char *refname, int *flags)
+static int check_refname_component(const char *refname, int *flags,
+  struct strbuf *sanitized)
 {
const char *cp;
char last = '\0';
+   size_t component_start;
+
+   if (sanitized)
+   component_start = sanitized->len;
 
for (cp = refname; ; cp++) {
int ch = *cp & 255;
unsigned char disp = refname_disposition[ch];
+
+   if (sanitized && disp != 1)
+   strbuf_addch(sanitized, ch);
+
switch (disp) {
case 1:
goto out;
case 2:
-   if (last == '.')
-   return -1; /* Refname contains "..". */
+   if (last == '.') { /* Refname contains "..". */
+   if (sanitized)
+   sanitized->len--; /* collapse ".." to 
single "." */
+   else
+   return -1;
+   }
break;
case 3:
-   if (last == '@')
-   return -1; /* Refname contains "@{". */
+   if (last == '@') { /* Refname contains "@{". */
+   if (sanitized)
+   sanitized->buf[sanitized->len-1] = '-';
+   else
+   return -1;
+   }
break;
case 4:
-   return -1;
+   /* forbidden char */
+   if (sanitized)
+   sanitized->buf[sanitized->len-1] = '-';
+   else
+   return -1;
+   break;
case 5:
-   if (!(*flags & REFNAME_REFSPEC_PATTERN))
-   return -1; /* refspec can't be a pattern */
+   if (!(*flags & REFNAME_REFSPEC_PATTERN)) {
+   /* refspec can't be a pattern */
+   if (sanitized)
+   sanitized->buf[sanitized->len-1] = '-';
+   else
+   return -1;
+   }
 
  

[PATCH v5 0/1] worktree add: sanitize worktree names

2019-03-08 Thread Nguyễn Thái Ngọc Duy
v5 is basically Jeff's version from one of the replies in v4, where
check_refname_component is enhanced to optionally sanitize.

I was reluctant to go this way because it makes check_refname_component
more complex (turns out still manageable) and burns worktree rules in
it. But there may never be the second sanitization user, we deal with
it when it comes.

As said, refs.c is pretty much Jeff's except two major changes:

 - handle foo.lock.lock correctly by stripping .lock repeatedly

 - sanitize refname _components_ instead of full refs. I could construct
   worktrees/ and pass to Jeff's sanitize_refname. But then I need
   to strip worktrees/ after that.

I took credits so that bugs come to me first (then I'll blame him
anyway while doing some evil laughs)

Nguyễn Thái Ngọc Duy (1):
  worktree add: sanitize worktree names

 builtin/worktree.c  |  10 +++-
 refs.c  | 103 
 refs.h  |   6 +++
 t/t2400-worktree-add.sh |   5 ++
 4 files changed, 104 insertions(+), 20 deletions(-)

-- 
2.21.0.rc1.337.gdf7f8d0522



ls-remote set timeout

2019-03-08 Thread Jakobus Schürz
Hi there!

Im new to this list - so hello, hope I'm welcome.


My problem is: I have a configuration for my bash saved on a private
git-repo. Every time, i start bash, my .bashrc checks this repo out to
get all changes (alias, some functions, $PS1 and so on). So i can have
my working environment on all my servers with personal login.


Now I'm working on a new customer, where github.com is not reachable
(firewall/proxy). Parts of my configuration (some plugins/scripts for
vim) cannot be updated there, because they are hosted on github.com. :-/

Now i tried to fiddle in a check, if a repo is reachable in my .bashrc.
And i found out, that git ls-remote is a good choice, because it handles
redirections from http to https correctly behind this proxy. (direct
https links to git-repos do even not work in this surrounding... don't
ask why, please).

I can check, if my private repo (git bare repo with gitweb) is reachable
(http pull, ssh is also closed!!!) with git ls-remote. But this check
hangs on github.com in case of a redirection from the proxy to a "this
is forbidden"-site... . And it hangs forever (1 Minute, 2 Minutes or
even really forever!)

Is it possible, to include a "--connection-timeout" and/or the
"--max-time" option for curl, that i can give to my "git ls-remote"
command? So i can call

git --connection-timeout 3 -m 3 ls-remote 

and the command stops after 3 seconds with an errorcode, which I can
evaluate?

I tried netcat and curl directly. In this environment only git ls-remote
will work correctly on reachable repos, but it hangs on blocked... :-/


Thank you for your interests


Jakob




Re: [PATCH v3 1/2] worktree: fix worktree add race.

2019-03-08 Thread Eric Sunshine
On Fri, Mar 8, 2019 at 4:20 AM Duy Nguyen  wrote:
> Junio, it seems 2/2 is stuck in an endless discussion. But 1/2 is good
> regardless, maybe pick it up now and let 2/2 come later whenever it's
> ready?

Yep, 1/2 seems a good idea and has not been controversial. It may not
solve all the race conditions, but it is a good step forward.


Re: [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s

2019-03-08 Thread Eric Sunshine
On Fri, Mar 8, 2019 at 12:38 AM Junio C Hamano  wrote:
> An unrelated tangent, but what do you think of this patch?  In the
> context of testing "git rm", if foo is a dangling symbolic link,
> "git rm foo && test_path_is_missing foo" would need something like
> this to work correctly, I would think.
>
>  test_path_is_missing () {
> -   if test -e "$1"
> +   if test -e "$1" || test -L "$1"
> then
> echo "Path exists:"
> ls -ld "$1"

Makes sense. Won't we also want:

test_path_exists () {
-if ! test -e "$1"
+   if ! test -e "$1" && ! test -L "$1"
   then
echo "Path $1 doesn't exist. $2"

or something like that?


PRIVEE

2019-03-08 Thread WILLIAM DAVID
Good day , my name is David William , i sent you a mail and there was
no response , please confirm that you did get this mail for more
details.

Regards.

David William


[PATCH v3 02/21] git-checkout.txt: fix one syntax line

2019-03-08 Thread Nguyễn Thái Ngọc Duy
 can be omitted in this syntax, and it's actually documented a
few paragraphs down:

  You could omit , in which case the command degenerates to
  "check out the current branch", which is a glorified no-op with
  rather expensive side-effects to show only the tracking information,
  if exists, for the current branch.
---
 Documentation/git-checkout.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 99c8c0dc0f..28817cfa41 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -23,7 +23,7 @@ or the specified tree.  If no paths are given, 'git checkout' 
will
 also update `HEAD` to set the specified branch as the current
 branch.
 
-'git checkout' ::
+'git checkout' []::
To prepare for working on , switch to it by updating
the index and the files in the working tree, and by pointing
HEAD at the branch. Local modifications to the files in the
-- 
2.21.0.rc1.337.gdf7f8d0522



[PATCH v3 00/21] Add new command "switch"

2019-03-08 Thread Nguyễn Thái Ngọc Duy
v3 contains document and completion updates based on v2's feedback. It
also contains some extra git-checkout.txt updates (blame Eric for this,
he points out problems in git-switch.txt and makes me want to go fix
git-checkout.txt too).

The series is now based on 'master' (yay!)

Nguyễn Thái Ngọc Duy (21):
  git-checkout.txt: spell out --no-option
  git-checkout.txt: fix one syntax line
  doc: document --overwrite-ignore
  git-checkout.txt: fix monospace typeset
  t: rename t2014-switch.sh to t2014-checkout-switch.sh
  checkout: factor out some code in parse_branchname_arg()
  checkout: make "opts" in cmd_checkout() a pointer
  checkout: move 'confict_style' and 'dwim_..' to checkout_opts
  checkout: split options[] array in three pieces
  checkout: split part of it to new command 'switch'
  switch: better names for -b and -B
  switch: remove -l
  switch: stop accepting pathspec
  switch: reject "do nothing" case
  switch: only allow explicit detached HEAD
  switch: add short option for --detach
  switch: no implicit dwim, use --guess to dwim
  switch: no worktree status unless real branch switch happens
  t: add tests for switch
  completion: support switch
  doc: promote "git switch"

 .gitignore|   1 +
 Documentation/config/advice.txt   |  13 +-
 Documentation/config/branch.txt   |   4 +-
 Documentation/config/checkout.txt |  17 +-
 Documentation/config/diff.txt |   3 +-
 Documentation/git-branch.txt  |  12 +-
 Documentation/git-check-ref-format.txt|   3 +-
 Documentation/git-checkout.txt| 119 +++--
 Documentation/git-format-patch.txt|   2 +-
 Documentation/git-merge-base.txt  |   2 +-
 Documentation/git-merge.txt   |   5 +
 Documentation/git-rebase.txt  |   2 +-
 Documentation/git-remote.txt  |   2 +-
 Documentation/git-rerere.txt  |  10 +-
 Documentation/git-reset.txt   |  20 +-
 Documentation/git-stash.txt   |   9 +-
 Documentation/git-switch.txt  | 259 ++
 Documentation/gitattributes.txt   |   3 +-
 Documentation/gitcore-tutorial.txt|  19 +-
 Documentation/giteveryday.txt |  24 +-
 Documentation/githooks.txt|   8 +-
 Documentation/gittutorial.txt |   4 +-
 Documentation/gitworkflows.txt|   3 +-
 Documentation/revisions.txt   |   2 +-
 Documentation/user-manual.txt |  56 +--
 Makefile  |   1 +
 advice.c  |  11 +-
 builtin.h |   1 +
 builtin/checkout.c| 471 +-
 command-list.txt  |   1 +
 contrib/completion/git-completion.bash|  27 +
 git.c |   1 +
 parse-options-cb.c|  17 +
 parse-options.h   |   1 +
 sha1-name.c   |   2 +-
 t/t1090-sparse-checkout-scope.sh  |  14 -
 ...014-switch.sh => t2014-checkout-switch.sh} |   0
 t/t2020-checkout-detach.sh|  16 +-
 t/t2060-switch.sh |  87 
 39 files changed, 837 insertions(+), 415 deletions(-)
 create mode 100644 Documentation/git-switch.txt
 rename t/{t2014-switch.sh => t2014-checkout-switch.sh} (100%)
 create mode 100755 t/t2060-switch.sh

Range-diff dựa trên v2:
 -:  -- >  1:  949f3dd4fd git-checkout.txt: spell out --no-option
 1:  8358b9ca36 =  2:  1ddbbae3e2 git-checkout.txt: fix one syntax line
 2:  1686ccbf8d !  3:  b0cb2372db doc: document --overwrite-ignore
@@ -14,14 +14,15 @@
out anyway. In other words, the ref can be held by more than one
worktree.
  
-+--[no-]overwrite-ignore::
++--overwrite-ignore::
++--no-overwrite-ignore::
 +  Silently overwrite ignored files when switching branches. This
-+  is the default behavior. Use --no-overwrite-ignore to abort
++  is the default behavior. Use `--no-overwrite-ignore` to abort
 +  the operation when the new branch contains ignored files.
 +
- --[no-]recurse-submodules::
+ --recurse-submodules::
+ --no-recurse-submodules::
Using --recurse-submodules will update the content of all initialized
-   submodules according to the commit recorded in the superproject. If
 
  diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
  --- a/Documentation/git-merge.txt
@@ -30,9 +31,10 @@
Allow the rerere mechanism to update the index with the
result of auto-conflict resolution if possible.
  
-+--[no-]overwrite-ignore::
++--overwrite-ignore::
++--no-overwrite-ignore::
 +  Silently overwrite ignored files from 

[PATCH v3 03/21] doc: document --overwrite-ignore

2019-03-08 Thread Nguyễn Thái Ngọc Duy
I added this option in git-checkout and git-merge in c1d7036b6b
(checkout,merge: disallow overwriting ignored files with
--no-overwrite-ignore - 2011-11-27) but did not remember to update
documentation. This completes that commit.
---
 Documentation/git-checkout.txt | 6 ++
 Documentation/git-merge.txt| 5 +
 2 files changed, 11 insertions(+)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 28817cfa41..5280d1f9ed 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -271,6 +271,12 @@ Note that this option uses the no overlay mode by default 
(see also
out anyway. In other words, the ref can be held by more than one
worktree.
 
+--overwrite-ignore::
+--no-overwrite-ignore::
+   Silently overwrite ignored files when switching branches. This
+   is the default behavior. Use `--no-overwrite-ignore` to abort
+   the operation when the new branch contains ignored files.
+
 --recurse-submodules::
 --no-recurse-submodules::
Using --recurse-submodules will update the content of all initialized
diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 4cc86469f3..6a9163d8fe 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -87,6 +87,11 @@ will be appended to the specified message.
Allow the rerere mechanism to update the index with the
result of auto-conflict resolution if possible.
 
+--overwrite-ignore::
+--no-overwrite-ignore::
+   Silently overwrite ignored files from the merge result. This
+   is the default behavior. Use `--no-overwrite-ignore` to abort.
+
 --abort::
Abort the current conflict resolution process, and
try to reconstruct the pre-merge state.
-- 
2.21.0.rc1.337.gdf7f8d0522



[PATCH v3 01/21] git-checkout.txt: spell out --no-option

2019-03-08 Thread Nguyễn Thái Ngọc Duy
It's easier to search for and also less cryptic.
---
 Documentation/git-checkout.txt | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index f179b43732..99c8c0dc0f 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -118,7 +118,8 @@ OPTIONS
 --quiet::
Quiet, suppress feedback messages.
 
---[no-]progress::
+--progress::
+--no-progress::
Progress status is reported on the standard error stream
by default when it is attached to a terminal, unless `--quiet`
is specified. This flag enables progress reporting even if not
@@ -262,7 +263,7 @@ edits from your current working tree. See the ``Interactive 
Mode''
 section of linkgit:git-add[1] to learn how to operate the `--patch` mode.
 +
 Note that this option uses the no overlay mode by default (see also
-`--[no-]overlay`), and currently doesn't support overlay mode.
+`--overlay`), and currently doesn't support overlay mode.
 
 --ignore-other-worktrees::
`git checkout` refuses when the wanted ref is already checked
@@ -270,7 +271,8 @@ Note that this option uses the no overlay mode by default 
(see also
out anyway. In other words, the ref can be held by more than one
worktree.
 
---[no-]recurse-submodules::
+--recurse-submodules::
+--no-recurse-submodules::
Using --recurse-submodules will update the content of all initialized
submodules according to the commit recorded in the superproject. If
local modifications in a submodule would be overwritten the checkout
@@ -283,7 +285,8 @@ Note that this option uses the no overlay mode by default 
(see also
Do not attempt to create a branch if a remote tracking branch
of the same name exists.
 
---[no-]overlay::
+--overlay::
+--no-overlay::
In the default overlay mode, `git checkout` never
removes files from the index or the working tree.  When
specifying `--no-overlay`, files that appear in the index and
-- 
2.21.0.rc1.337.gdf7f8d0522



[PATCH v3 06/21] checkout: factor out some code in parse_branchname_arg()

2019-03-08 Thread Nguyễn Thái Ngọc Duy
This is in preparation for the new command restore, which also
needs to parse opts->source_tree but does not need all the
disambiguation logic.
---
 builtin/checkout.c | 51 --
 1 file changed, 31 insertions(+), 20 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 0e6037b296..cbdcb1417f 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1081,6 +1081,34 @@ static int git_checkout_config(const char *var, const 
char *value, void *cb)
return git_xmerge_config(var, value, NULL);
 }
 
+static void setup_new_branch_info_and_source_tree(
+   struct branch_info *new_branch_info,
+   struct checkout_opts *opts,
+   struct object_id *rev,
+   const char *arg)
+{
+   struct tree **source_tree = &opts->source_tree;
+   struct object_id branch_rev;
+
+   new_branch_info->name = arg;
+   setup_branch_path(new_branch_info);
+
+   if (!check_refname_format(new_branch_info->path, 0) &&
+   !read_ref(new_branch_info->path, &branch_rev))
+   oidcpy(rev, &branch_rev);
+   else
+   new_branch_info->path = NULL; /* not an existing branch */
+
+   new_branch_info->commit = 
lookup_commit_reference_gently(the_repository, rev, 1);
+   if (!new_branch_info->commit) {
+   /* not a commit */
+   *source_tree = parse_tree_indirect(rev);
+   } else {
+   parse_commit_or_die(new_branch_info->commit);
+   *source_tree = get_commit_tree(new_branch_info->commit);
+   }
+}
+
 static int parse_branchname_arg(int argc, const char **argv,
int dwim_new_local_branch_ok,
struct branch_info *new_branch_info,
@@ -1088,10 +1116,8 @@ static int parse_branchname_arg(int argc, const char 
**argv,
struct object_id *rev,
int *dwim_remotes_matched)
 {
-   struct tree **source_tree = &opts->source_tree;
const char **new_branch = &opts->new_branch;
int argcount = 0;
-   struct object_id branch_rev;
const char *arg;
int dash_dash_pos;
int has_dash_dash = 0;
@@ -1213,26 +1239,11 @@ static int parse_branchname_arg(int argc, const char 
**argv,
argv++;
argc--;
 
-   new_branch_info->name = arg;
-   setup_branch_path(new_branch_info);
-
-   if (!check_refname_format(new_branch_info->path, 0) &&
-   !read_ref(new_branch_info->path, &branch_rev))
-   oidcpy(rev, &branch_rev);
-   else
-   new_branch_info->path = NULL; /* not an existing branch */
+   setup_new_branch_info_and_source_tree(new_branch_info, opts, rev, arg);
 
-   new_branch_info->commit = 
lookup_commit_reference_gently(the_repository, rev, 1);
-   if (!new_branch_info->commit) {
-   /* not a commit */
-   *source_tree = parse_tree_indirect(rev);
-   } else {
-   parse_commit_or_die(new_branch_info->commit);
-   *source_tree = get_commit_tree(new_branch_info->commit);
-   }
-
-   if (!*source_tree)   /* case (1): want a tree */
+   if (!opts->source_tree)   /* case (1): want a tree */
die(_("reference is not a tree: %s"), arg);
+
if (!has_dash_dash) {   /* case (3).(d) -> (1) */
/*
 * Do not complain the most common case
-- 
2.21.0.rc1.337.gdf7f8d0522



[PATCH v3 05/21] t: rename t2014-switch.sh to t2014-checkout-switch.sh

2019-03-08 Thread Nguyễn Thái Ngọc Duy
The old name does not really say that this is about 'checkout -b'. See
49d833dc07 (Revert "checkout branch: prime cache-tree fully" -
2009-05-12) for more information
---
 t/{t2014-switch.sh => t2014-checkout-switch.sh} | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename t/{t2014-switch.sh => t2014-checkout-switch.sh} (100%)

diff --git a/t/t2014-switch.sh b/t/t2014-checkout-switch.sh
similarity index 100%
rename from t/t2014-switch.sh
rename to t/t2014-checkout-switch.sh
-- 
2.21.0.rc1.337.gdf7f8d0522



[PATCH v3 04/21] git-checkout.txt: fix monospace typeset

2019-03-08 Thread Nguyễn Thái Ngọc Duy
---
 Documentation/git-checkout.txt | 60 +-
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 5280d1f9ed..1b9d689933 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -24,14 +24,14 @@ also update `HEAD` to set the specified branch as the 
current
 branch.
 
 'git checkout' []::
-   To prepare for working on , switch to it by updating
+   To prepare for working on ``, switch to it by updating
the index and the files in the working tree, and by pointing
HEAD at the branch. Local modifications to the files in the
working tree are kept, so that they can be committed to the
-   .
+   ``.
 +
-If  is not found but there does exist a tracking branch in
-exactly one remote (call it ) with a matching name, treat as
+If `` is not found but there does exist a tracking branch in
+exactly one remote (call it ``) with a matching name, treat as
 equivalent to
 +
 
@@ -47,7 +47,7 @@ branches from there if `` is ambiguous but exists on 
the
 'origin' remote. See also `checkout.defaultRemote` in
 linkgit:git-config[1].
 +
-You could omit , in which case the command degenerates to
+You could omit ``, in which case the command degenerates to
 "check out the current branch", which is a glorified no-op with
 rather expensive side-effects to show only the tracking information,
 if exists, for the current branch.
@@ -61,7 +61,7 @@ if exists, for the current branch.
`--track` without `-b` implies branch creation; see the
description of `--track` below.
 +
-If `-B` is given,  is created if it doesn't exist; otherwise, it
+If `-B` is given, `` is created if it doesn't exist; otherwise, it
 is reset. This is the transactional equivalent of
 +
 
@@ -75,25 +75,25 @@ successful.
 'git checkout' --detach []::
 'git checkout' [--detach] ::
 
-   Prepare to work on top of , by detaching HEAD at it
+   Prepare to work on top of ``, by detaching HEAD at it
(see "DETACHED HEAD" section), and updating the index and the
files in the working tree.  Local modifications to the files
in the working tree are kept, so that the resulting working
tree will be the state recorded in the commit plus the local
modifications.
 +
-When the  argument is a branch name, the `--detach` option can
+When the `` argument is a branch name, the `--detach` option can
 be used to detach HEAD at the tip of the branch (`git checkout
-` would check out that branch without detaching HEAD).
+``` would check out that branch without detaching HEAD).
 +
-Omitting  detaches HEAD at the tip of the current branch.
+Omitting `` detaches HEAD at the tip of the current branch.
 
 'git checkout' [] [--] ...::
 
Overwrite paths in the working tree by replacing with the
-   contents in the index or in the  (most often a
-   commit).  When a  is given, the paths that
-   match the  are updated both in the index and in
+   contents in the index or in the `` (most often a
+   commit).  When a `` is given, the paths that
+   match the `` are updated both in the index and in
the working tree.
 +
 The index may contain unmerged entries because of a previous failed merge.
@@ -155,12 +155,12 @@ on your side branch as `theirs` (i.e. "one contributor's 
work on top
 of it").
 
 -b ::
-   Create a new branch named  and start it at
-   ; see linkgit:git-branch[1] for details.
+   Create a new branch named `` and start it at
+   ``; see linkgit:git-branch[1] for details.
 
 -B ::
-   Creates the branch  and start it at ;
-   if it already exists, then reset it to . This is
+   Creates the branch `` and start it at ``;
+   if it already exists, then reset it to ``. This is
equivalent to running "git branch" with "-f"; see
linkgit:git-branch[1] for details.
 
@@ -191,19 +191,19 @@ explicitly give a name with `-b` in such a case.
Rather than checking out a branch to work on it, check out a
commit for inspection and discardable experiments.
This is the default behavior of "git checkout " when
-is not a branch name.  See the "DETACHED HEAD" section
+   `` is not a branch name.  See the "DETACHED HEAD" section
below for details.
 
 --orphan ::
-   Create a new 'orphan' branch, named , started from
-and switch to it.  The first commit made on this
+   Create a new 'orphan' branch, named ``, started from
+   `` and switch to it.  The first commit made on this
new branch will have no parents and it will be the root of a new
history totally disconnected from all the other branches and
commits.
 +
 The index and the working tree are adjusted as if you had previously run
 "git checkout ".  This allows you to start a new history
-that records a set of paths similar 

[PATCH v3 12/21] switch: remove -l

2019-03-08 Thread Nguyễn Thái Ngọc Duy
This option is ancient. Nowadays reflog is enabled by default and
automatically created for new branches. Keep it in git-checkout only.
---
 builtin/checkout.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 4c3f0f6ac7..a731f983c4 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1372,7 +1372,6 @@ static struct option *add_common_switch_branch_options(
struct checkout_opts *opts, struct option *prevopts)
 {
struct option options[] = {
-   OPT_BOOL('l', NULL, &opts->new_branch_log, N_("create reflog 
for new branch")),
OPT_BOOL(0, "detach", &opts->force_detach, N_("detach HEAD at 
named commit")),
OPT_SET_INT('t', "track",  &opts->track, N_("set upstream info 
for new branch"),
BRANCH_TRACK_EXPLICIT),
@@ -1573,6 +1572,7 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
   N_("create and checkout a new branch")),
OPT_STRING('B', NULL, &opts.new_branch_force, N_("branch"),
   N_("create/reset and checkout a branch")),
+   OPT_BOOL('l', NULL, &opts.new_branch_log, N_("create reflog for 
new branch")),
OPT_END()
};
int ret;
-- 
2.21.0.rc1.337.gdf7f8d0522



[PATCH v3 08/21] checkout: move 'confict_style' and 'dwim_..' to checkout_opts

2019-03-08 Thread Nguyễn Thái Ngọc Duy
These local variables are referenced by struct option[]. This struct
will soon be broken down, moved away and we can't rely on local
variables anymore. Move these two to struct checkout_opts in
preparation for that.
---
 builtin/checkout.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index baefe3c942..4b43433941 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -47,6 +47,8 @@ struct checkout_opts {
int show_progress;
int count_checkout_paths;
int overlay_mode;
+   int no_dwim_new_local_branch;
+
/*
 * If new checkout options are added, skip_merge_working_tree
 * should be updated accordingly.
@@ -58,6 +60,7 @@ struct checkout_opts {
int new_branch_log;
enum branch_track track;
struct diff_options diff_options;
+   char *conflict_style;
 
int branch_exists;
const char *prefix;
@@ -1344,8 +1347,7 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
struct checkout_opts real_opts;
struct checkout_opts *opts = &real_opts;
struct branch_info new_branch_info;
-   char *conflict_style = NULL;
-   int dwim_new_local_branch, no_dwim_new_local_branch = 0;
+   int dwim_new_local_branch;
int dwim_remotes_matched = 0;
struct option options[] = {
OPT__QUIET(&opts->quiet, N_("suppress progress reporting")),
@@ -1370,12 +1372,12 @@ int cmd_checkout(int argc, const char **argv, const 
char *prefix)
OPT_BOOL_F(0, "overwrite-ignore", &opts->overwrite_ignore,
   N_("update ignored files (default)"),
   PARSE_OPT_NOCOMPLETE),
-   OPT_STRING(0, "conflict", &conflict_style, N_("style"),
+   OPT_STRING(0, "conflict", &opts->conflict_style, N_("style"),
   N_("conflict style (merge or diff3)")),
OPT_BOOL('p', "patch", &opts->patch_mode, N_("select hunks 
interactively")),
OPT_BOOL(0, "ignore-skip-worktree-bits", 
&opts->ignore_skipworktree,
 N_("do not limit pathspecs to sparse entries only")),
-   OPT_BOOL(0, "no-guess", &no_dwim_new_local_branch,
+   OPT_BOOL(0, "no-guess", &opts->no_dwim_new_local_branch,
 N_("do not second guess 'git checkout 
'")),
OPT_BOOL(0, "ignore-other-worktrees", 
&opts->ignore_other_worktrees,
 N_("do not check if another worktree is holding the 
given ref")),
@@ -1393,6 +1395,7 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
opts->prefix = prefix;
opts->show_progress = -1;
opts->overlay_mode = -1;
+   opts->no_dwim_new_local_branch = 0;
 
git_config(git_checkout_config, opts);
 
@@ -1401,7 +1404,7 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
argc = parse_options(argc, argv, prefix, options, checkout_usage,
 PARSE_OPT_KEEP_DASHDASH);
 
-   dwim_new_local_branch = !no_dwim_new_local_branch;
+   dwim_new_local_branch = !opts->no_dwim_new_local_branch;
if (opts->show_progress < 0) {
if (opts->quiet)
opts->show_progress = 0;
@@ -1409,9 +1412,9 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
opts->show_progress = isatty(2);
}
 
-   if (conflict_style) {
+   if (opts->conflict_style) {
opts->merge = 1; /* implied */
-   git_xmerge_config("merge.conflictstyle", conflict_style, NULL);
+   git_xmerge_config("merge.conflictstyle", opts->conflict_style, 
NULL);
}
 
if ((!!opts->new_branch + !!opts->new_branch_force + 
!!opts->new_orphan_branch) > 1)
-- 
2.21.0.rc1.337.gdf7f8d0522



[PATCH v3 13/21] switch: stop accepting pathspec

2019-03-08 Thread Nguyễn Thái Ngọc Duy
This command is about switching branch (or creating a new one) and
should not accept pathspec. This helps simplify ambiguation
handling. The other two ("git checkout" and "git restore") of
course do accept pathspec as before.
---
 builtin/checkout.c | 20 +++-
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index a731f983c4..1b1181b220 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -53,6 +53,7 @@ struct checkout_opts {
int count_checkout_paths;
int overlay_mode;
int no_dwim_new_local_branch;
+   int accept_pathspec;
 
/*
 * If new checkout options are added, skip_merge_working_tree
@@ -1175,10 +1176,16 @@ static int parse_branchname_arg(int argc, const char 
**argv,
if (!argc)
return 0;
 
+   if (!opts->accept_pathspec) {
+   if (argc > 1)
+   die(_("only one reference expected"));
+   has_dash_dash = 1; /* helps disambiguate */
+   }
+
arg = argv[0];
dash_dash_pos = -1;
for (i = 0; i < argc; i++) {
-   if (!strcmp(argv[i], "--")) {
+   if (opts->accept_pathspec && !strcmp(argv[i], "--")) {
dash_dash_pos = i;
break;
}
@@ -1212,11 +1219,12 @@ static int parse_branchname_arg(int argc, const char 
**argv,
recover_with_dwim = 0;
 
/*
-* Accept "git checkout foo" and "git checkout foo --"
-* as candidates for dwim.
+* Accept "git checkout foo", "git checkout foo --"
+* and "git switch foo" as candidates for dwim.
 */
if (!(argc == 1 && !has_dash_dash) &&
-   !(argc == 2 && has_dash_dash))
+   !(argc == 2 && has_dash_dash) &&
+   opts->accept_pathspec)
recover_with_dwim = 0;
 
if (recover_with_dwim) {
@@ -1261,7 +1269,7 @@ static int parse_branchname_arg(int argc, const char 
**argv,
 */
if (argc)
verify_non_filename(opts->prefix, arg);
-   } else {
+   } else if (opts->accept_pathspec) {
argcount++;
argv++;
argc--;
@@ -1579,6 +1587,7 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
 
memset(&opts, 0, sizeof(opts));
opts.no_dwim_new_local_branch = 0;
+   opts.accept_pathspec = 1;
 
options = parse_options_dup(checkout_options);
options = add_common_options(&opts, options);
@@ -1606,6 +1615,7 @@ int cmd_switch(int argc, const char **argv, const char 
*prefix)
 
memset(&opts, 0, sizeof(opts));
opts.no_dwim_new_local_branch = 0;
+   opts.accept_pathspec = 0;
 
options = parse_options_dup(switch_options);
options = add_common_options(&opts, options);
-- 
2.21.0.rc1.337.gdf7f8d0522



[PATCH v3 14/21] switch: reject "do nothing" case

2019-03-08 Thread Nguyễn Thái Ngọc Duy
"git checkout" can be executed without any arguments. What it does is
not exactly great: it switches from HEAD to HEAD and shows worktree
modification as a side effect.

Make switch reject this case. Just use "git status" if you want
that side effect. For switch, you have to either

- really switch a branch
- (explicitly) detach from the current branch
- create a new branch
---
 builtin/checkout.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 1b1181b220..f9f7ee2936 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -54,6 +54,7 @@ struct checkout_opts {
int overlay_mode;
int no_dwim_new_local_branch;
int accept_pathspec;
+   int switch_branch_doing_nothing_is_ok;
 
/*
 * If new checkout options are added, skip_merge_working_tree
@@ -1334,6 +1335,12 @@ static int checkout_branch(struct checkout_opts *opts,
die(_("Cannot switch branch to a non-commit '%s'"),
new_branch_info->name);
 
+   if (!opts->switch_branch_doing_nothing_is_ok &&
+   !new_branch_info->name &&
+   !opts->new_branch &&
+   !opts->force_detach)
+   die(_("missing branch or commit argument"));
+
if (new_branch_info->path && !opts->force_detach && !opts->new_branch &&
!opts->ignore_other_worktrees) {
int flag;
@@ -1587,6 +1594,7 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
 
memset(&opts, 0, sizeof(opts));
opts.no_dwim_new_local_branch = 0;
+   opts.switch_branch_doing_nothing_is_ok = 1;
opts.accept_pathspec = 1;
 
options = parse_options_dup(checkout_options);
@@ -1616,6 +1624,7 @@ int cmd_switch(int argc, const char **argv, const char 
*prefix)
memset(&opts, 0, sizeof(opts));
opts.no_dwim_new_local_branch = 0;
opts.accept_pathspec = 0;
+   opts.switch_branch_doing_nothing_is_ok = 0;
 
options = parse_options_dup(switch_options);
options = add_common_options(&opts, options);
-- 
2.21.0.rc1.337.gdf7f8d0522



[PATCH v3 15/21] switch: only allow explicit detached HEAD

2019-03-08 Thread Nguyễn Thái Ngọc Duy
"git checkout " will checkout the commit in question and
detach HEAD from the current branch. It is naturally a right thing to
do once you get git references. But detached HEAD is a scary concept
to new users because we show a lot of warnings and stuff, and it could
be hard to get out of (until you know better).

To keep switch a bit more friendly to new users, we only allow
entering detached HEAD mode when --detach is given. "git
switch" must take a branch (unless you create a new branch,
then of course switch can take any commit-ish)
---
 builtin/checkout.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index f9f7ee2936..2e150f0175 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -45,6 +45,7 @@ struct checkout_opts {
int merge;
int force;
int force_detach;
+   int implicit_detach;
int writeout_stage;
int overwrite_ignore;
int ignore_skipworktree;
@@ -1341,6 +1342,14 @@ static int checkout_branch(struct checkout_opts *opts,
!opts->force_detach)
die(_("missing branch or commit argument"));
 
+   if (!opts->implicit_detach &&
+   !opts->force_detach &&
+   !opts->new_branch &&
+   !opts->new_branch_force &&
+   new_branch_info->name &&
+   !new_branch_info->path)
+   die(_("a branch is expected, got %s"), new_branch_info->name);
+
if (new_branch_info->path && !opts->force_detach && !opts->new_branch &&
!opts->ignore_other_worktrees) {
int flag;
@@ -1596,6 +1605,7 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
opts.no_dwim_new_local_branch = 0;
opts.switch_branch_doing_nothing_is_ok = 1;
opts.accept_pathspec = 1;
+   opts.implicit_detach = 1;
 
options = parse_options_dup(checkout_options);
options = add_common_options(&opts, options);
@@ -1625,6 +1635,7 @@ int cmd_switch(int argc, const char **argv, const char 
*prefix)
opts.no_dwim_new_local_branch = 0;
opts.accept_pathspec = 0;
opts.switch_branch_doing_nothing_is_ok = 0;
+   opts.implicit_detach = 0;
 
options = parse_options_dup(switch_options);
options = add_common_options(&opts, options);
-- 
2.21.0.rc1.337.gdf7f8d0522



[PATCH v3 17/21] switch: no implicit dwim, use --guess to dwim

2019-03-08 Thread Nguyễn Thái Ngọc Duy
Similar to automatic detach, this behavior could be confusing because
it can sometimes create a new branch without a user asking it to,
especially when the user is still not aware about this feature.

In the future, perhaps we could have a config key to disable these
safety nets and let 'switch' do automatic detach or dwim
again. But that will be opt-in after the user knows what is what. For
now give a short option if you want to use it often.
---
 Documentation/git-checkout.txt | 38 --
 builtin/checkout.c | 16 +++---
 2 files changed, 31 insertions(+), 23 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index ac355dc3f3..2b776c1269 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -31,22 +31,13 @@ branch.
``.
 +
 If `` is not found but there does exist a tracking branch in
-exactly one remote (call it ``) with a matching name, treat as
-equivalent to
+exactly one remote (call it ``) with a matching name and
+`--no-guess` is not specified, treat as equivalent to
 +
 
 $ git checkout -b  --track /
 
 +
-If the branch exists in multiple remotes and one of them is named by
-the `checkout.defaultRemote` configuration variable, we'll use that
-one for the purposes of disambiguation, even if the `` isn't
-unique across all remotes. Set it to
-e.g. `checkout.defaultRemote=origin` to always checkout remote
-branches from there if `` is ambiguous but exists on the
-'origin' remote. See also `checkout.defaultRemote` in
-linkgit:git-config[1].
-+
 You could omit ``, in which case the command degenerates to
 "check out the current branch", which is a glorified no-op with
 rather expensive side-effects to show only the tracking information,
@@ -183,6 +174,27 @@ explicitly give a name with `-b` in such a case.
Do not set up "upstream" configuration, even if the
branch.autoSetupMerge configuration variable is true.
 
+--guess::
+--no-guess::
+   If `` is not found but there does exist a tracking
+   branch in exactly one remote (call it ``) with a
+   matching name, treat as equivalent to
++
+
+$ git checkout -b  --track /
+
++
+If the branch exists in multiple remotes and one of them is named by
+the `checkout.defaultRemote` configuration variable, we'll use that
+one for the purposes of disambiguation, even if the `` isn't
+unique across all remotes. Set it to
+e.g. `checkout.defaultRemote=origin` to always checkout remote
+branches from there if `` is ambiguous but exists on the
+'origin' remote. See also `checkout.defaultRemote` in
+linkgit:git-config[1].
++
+Use `--no-guess` to disable this.
+
 -l::
Create the new branch's reflog; see linkgit:git-branch[1] for
details.
@@ -287,10 +299,6 @@ Note that this option uses the no overlay mode by default 
(see also
Just like linkgit:git-submodule[1], this will detach the
submodules HEAD.
 
---no-guess::
-   Do not attempt to create a branch if a remote tracking branch
-   of the same name exists.
-
 --overlay::
 --no-overlay::
In the default overlay mode, `git checkout` never
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 0866aeba83..8a89df4f36 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -53,7 +53,7 @@ struct checkout_opts {
int show_progress;
int count_checkout_paths;
int overlay_mode;
-   int no_dwim_new_local_branch;
+   int dwim_new_local_branch;
int accept_pathspec;
int switch_branch_doing_nothing_is_ok;
 
@@ -1403,8 +1403,6 @@ static struct option *add_common_switch_branch_options(
OPT_BOOL_F(0, "overwrite-ignore", &opts->overwrite_ignore,
   N_("update ignored files (default)"),
   PARSE_OPT_NOCOMPLETE),
-   OPT_BOOL(0, "no-guess", &opts->no_dwim_new_local_branch,
-N_("second guess 'git checkout '")),
OPT_BOOL(0, "ignore-other-worktrees", 
&opts->ignore_other_worktrees,
 N_("do not check if another worktree is holding the 
given ref")),
OPT_END()
@@ -1441,7 +1439,6 @@ static int checkout_main(int argc, const char **argv, 
const char *prefix,
 {
struct branch_info new_branch_info;
int dwim_remotes_matched = 0;
-   int dwim_new_local_branch;
 
memset(&new_branch_info, 0, sizeof(new_branch_info));
opts->overwrite_ignore = 1;
@@ -1456,7 +1453,6 @@ static int checkout_main(int argc, const char **argv, 
const char *prefix,
argc = parse_options(argc, argv, prefix, options, usagestr,
 PARSE_OPT_KEEP_DASHDASH);
 
-   dwim_new_local_branch = !opts->no_dwim_new_local_branch;
if (opts->show_progress < 0) {
if (opts->quiet)
opts->show_progress = 0;
@@ -1516,7 +1512,7 @@ stati

[PATCH v3 16/21] switch: add short option for --detach

2019-03-08 Thread Nguyễn Thái Ngọc Duy
"git checkout" automatically detaches branches and --detach is not
that useful (--no-detach is more likely). But for "switch", you
may want to use it more often once you're used to detached HEAD. This
of course adds -d to git-checkout but it does not harm (yet?) to do it.
---
 builtin/checkout.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2e150f0175..0866aeba83 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1396,7 +1396,7 @@ static struct option *add_common_switch_branch_options(
struct checkout_opts *opts, struct option *prevopts)
 {
struct option options[] = {
-   OPT_BOOL(0, "detach", &opts->force_detach, N_("detach HEAD at 
named commit")),
+   OPT_BOOL('d', "detach", &opts->force_detach, N_("detach HEAD at 
named commit")),
OPT_SET_INT('t', "track",  &opts->track, N_("set upstream info 
for new branch"),
BRANCH_TRACK_EXPLICIT),
OPT_STRING(0, "orphan", &opts->new_orphan_branch, 
N_("new-branch"), N_("new unparented branch")),
-- 
2.21.0.rc1.337.gdf7f8d0522



[PATCH v3 10/21] checkout: split part of it to new command 'switch'

2019-03-08 Thread Nguyễn Thái Ngọc Duy
"git checkout" doing too many things is a source of confusion for many
users (and it even bites old timers sometimes). To remedy that, the
command will be split into two new ones: switch and
something-to-checkout-paths. The good old "git checkout" command is
still here and will be until all (or most of users) are sick of it.

See the new man page for the final design of switch. The actual
implementation though is still pretty much the same as "git checkout"
and not completely aligned with the man page. Following patches will
adjust their behavior to match the man page.
---
 .gitignore|   1 +
 Documentation/config/advice.txt   |  13 +-
 Documentation/config/branch.txt   |   4 +-
 Documentation/config/checkout.txt |   9 +-
 Documentation/config/diff.txt |   3 +-
 Documentation/git-checkout.txt|   4 +
 Documentation/git-switch.txt  | 259 ++
 Documentation/gitattributes.txt   |   3 +-
 Documentation/githooks.txt|   8 +-
 Makefile  |   1 +
 builtin.h |   1 +
 builtin/checkout.c|  60 +--
 command-list.txt  |   1 +
 git.c |   1 +
 14 files changed, 341 insertions(+), 27 deletions(-)
 create mode 100644 Documentation/git-switch.txt

diff --git a/.gitignore b/.gitignore
index 7374587f9d..c687b92b1c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -167,6 +167,7 @@
 /git-submodule
 /git-submodule--helper
 /git-svn
+/git-switch
 /git-symbolic-ref
 /git-tag
 /git-unpack-file
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index 88620429ea..239d479506 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -42,7 +42,8 @@ advice.*::
state in the output of linkgit:git-status[1], in
the template shown when writing commit messages in
linkgit:git-commit[1], and in the help message shown
-   by linkgit:git-checkout[1] when switching branch.
+   by linkgit:git-switch[1] or
+   linkgit:git-checkout[1] when switching branch.
statusUoption::
Advise to consider using the `-u` option to 
linkgit:git-status[1]
when the command takes more than 2 seconds to enumerate 
untracked
@@ -62,12 +63,14 @@ advice.*::
your information is guessed from the system username and
domain name.
detachedHead::
-   Advice shown when you used linkgit:git-checkout[1] to
-   move to the detach HEAD state, to instruct how to create
-   a local branch after the fact.
+   Advice shown when you used
+   linkgit:git-switch[1] or linkgit:git-checkout[1]
+   to move to the detach HEAD state, to instruct how to
+   create a local branch after the fact.
checkoutAmbiguousRemoteBranchName::
Advice shown when the argument to
-   linkgit:git-checkout[1] ambiguously resolves to a
+   linkgit:git-checkout[1] and linkgit:git-switch[1]
+   ambiguously resolves to a
remote tracking branch on more than one remote in
situations where an unambiguous argument would have
otherwise caused a remote-tracking branch to be
diff --git a/Documentation/config/branch.txt b/Documentation/config/branch.txt
index 019d60ede2..8050466159 100644
--- a/Documentation/config/branch.txt
+++ b/Documentation/config/branch.txt
@@ -1,5 +1,5 @@
 branch.autoSetupMerge::
-   Tells 'git branch' and 'git checkout' to set up new branches
+   Tells 'git branch', 'git switch' and 'git checkout' to set up new 
branches
so that linkgit:git-pull[1] will appropriately merge from the
starting point branch. Note that even if this option is not set,
this behavior can be chosen per-branch using the `--track`
@@ -11,7 +11,7 @@ branch.autoSetupMerge::
branch. This option defaults to true.
 
 branch.autoSetupRebase::
-   When a new branch is created with 'git branch' or 'git checkout'
+   When a new branch is created with 'git branch', 'git switch' or 'git 
checkout'
that tracks another branch, this variable tells Git to set
up pull to rebase instead of merge (see "branch..rebase").
When `never`, rebase is never automatically set to true.
diff --git a/Documentation/config/checkout.txt 
b/Documentation/config/checkout.txt
index c4118fa196..d6872ffa83 100644
--- a/Documentation/config/checkout.txt
+++ b/Documentation/config/checkout.txt
@@ -1,5 +1,6 @@
 checkout.defaultRemote::
-   When you run 'git checkout ' and only have one
+   When you run 'git checkout '
+   or 'git switch ' and only have one
remote, it may implicitly fall back on checking out and
tracking e.g. 'origin/'. This stops working as soon
as you have more than one re

[PATCH v3 09/21] checkout: split options[] array in three pieces

2019-03-08 Thread Nguyễn Thái Ngọc Duy
This is a preparation step for introducing new commands that do parts
of what checkout does. There will be two new commands, one is about
switching branches, detaching HEAD... one about checking out
paths. These share the a subset of command line options. The rest of
command line options are separate.
---
 builtin/checkout.c | 82 +-
 parse-options-cb.c | 17 ++
 parse-options.h|  1 +
 3 files changed, 77 insertions(+), 23 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 4b43433941..7d23083282 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1342,15 +1342,31 @@ static int checkout_branch(struct checkout_opts *opts,
return switch_branches(opts, new_branch_info);
 }
 
-int cmd_checkout(int argc, const char **argv, const char *prefix)
+static struct option *add_common_options(struct checkout_opts *opts,
+struct option *prevopts)
 {
-   struct checkout_opts real_opts;
-   struct checkout_opts *opts = &real_opts;
-   struct branch_info new_branch_info;
-   int dwim_new_local_branch;
-   int dwim_remotes_matched = 0;
struct option options[] = {
OPT__QUIET(&opts->quiet, N_("suppress progress reporting")),
+   { OPTION_CALLBACK, 0, "recurse-submodules", NULL,
+   "checkout", "control recursive updating of 
submodules",
+   PARSE_OPT_OPTARG, 
option_parse_recurse_submodules_worktree_updater },
+   OPT_BOOL(0, "progress", &opts->show_progress, N_("force 
progress reporting")),
+   OPT__FORCE(&opts->force, N_("force checkout (throw away local 
modifications)"),
+  PARSE_OPT_NOCOMPLETE),
+   OPT_BOOL('m', "merge", &opts->merge, N_("perform a 3-way merge 
with the new branch")),
+   OPT_STRING(0, "conflict", &opts->conflict_style, N_("style"),
+  N_("conflict style (merge or diff3)")),
+   OPT_END()
+   };
+   struct option *newopts = parse_options_concat(prevopts, options);
+   free(prevopts);
+   return newopts;
+}
+
+static struct option *add_switch_branch_options(struct checkout_opts *opts,
+   struct option *prevopts)
+{
+   struct option options[] = {
OPT_STRING('b', NULL, &opts->new_branch, N_("branch"),
   N_("create and checkout a new branch")),
OPT_STRING('B', NULL, &opts->new_branch_force, N_("branch"),
@@ -1360,34 +1376,49 @@ int cmd_checkout(int argc, const char **argv, const 
char *prefix)
OPT_SET_INT('t', "track",  &opts->track, N_("set upstream info 
for new branch"),
BRANCH_TRACK_EXPLICIT),
OPT_STRING(0, "orphan", &opts->new_orphan_branch, 
N_("new-branch"), N_("new unparented branch")),
+   OPT_BOOL_F(0, "overwrite-ignore", &opts->overwrite_ignore,
+  N_("update ignored files (default)"),
+  PARSE_OPT_NOCOMPLETE),
+   OPT_BOOL(0, "no-guess", &opts->no_dwim_new_local_branch,
+N_("second guess 'git checkout '")),
+   OPT_BOOL(0, "ignore-other-worktrees", 
&opts->ignore_other_worktrees,
+N_("do not check if another worktree is holding the 
given ref")),
+   OPT_END()
+   };
+   struct option *newopts = parse_options_concat(prevopts, options);
+   free(prevopts);
+   return newopts;
+}
+
+static struct option *add_checkout_path_options(struct checkout_opts *opts,
+   struct option *prevopts)
+{
+   struct option options[] = {
OPT_SET_INT_F('2', "ours", &opts->writeout_stage,
  N_("checkout our version for unmerged files"),
  2, PARSE_OPT_NONEG),
OPT_SET_INT_F('3', "theirs", &opts->writeout_stage,
  N_("checkout their version for unmerged files"),
  3, PARSE_OPT_NONEG),
-   OPT__FORCE(&opts->force, N_("force checkout (throw away local 
modifications)"),
-  PARSE_OPT_NOCOMPLETE),
-   OPT_BOOL('m', "merge", &opts->merge, N_("perform a 3-way merge 
with the new branch")),
-   OPT_BOOL_F(0, "overwrite-ignore", &opts->overwrite_ignore,
-  N_("update ignored files (default)"),
-  PARSE_OPT_NOCOMPLETE),
-   OPT_STRING(0, "conflict", &opts->conflict_style, N_("style"),
-  N_("conflict style (merge or diff3)")),
OPT_BOOL('p', "patch", &opts->patch_mode, N_("select hunks 
interactively")),
OPT_BOOL(0, "ignore-skip-worktree-bits", 
&opts->ignore_skipworktree,

[PATCH v3 11/21] switch: better names for -b and -B

2019-03-08 Thread Nguyễn Thái Ngọc Duy
The shortcut of these options do not make much sense when used with
switch. And their descriptions are also tied to checkout. Move -b/-B
to cmd_checkout() and new -c/-C with the same functionality in
cmd_switch_branch()
---
 builtin/checkout.c | 32 +---
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 1eff10dbef..4c3f0f6ac7 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1368,14 +1368,10 @@ static struct option *add_common_options(struct 
checkout_opts *opts,
return newopts;
 }
 
-static struct option *add_switch_branch_options(struct checkout_opts *opts,
-   struct option *prevopts)
+static struct option *add_common_switch_branch_options(
+   struct checkout_opts *opts, struct option *prevopts)
 {
struct option options[] = {
-   OPT_STRING('b', NULL, &opts->new_branch, N_("branch"),
-  N_("create and checkout a new branch")),
-   OPT_STRING('B', NULL, &opts->new_branch_force, N_("branch"),
-  N_("create/reset and checkout a branch")),
OPT_BOOL('l', NULL, &opts->new_branch_log, N_("create reflog 
for new branch")),
OPT_BOOL(0, "detach", &opts->force_detach, N_("detach HEAD at 
named commit")),
OPT_SET_INT('t', "track",  &opts->track, N_("set upstream info 
for new branch"),
@@ -1571,15 +1567,22 @@ static int checkout_main(int argc, const char **argv, 
const char *prefix,
 int cmd_checkout(int argc, const char **argv, const char *prefix)
 {
struct checkout_opts opts;
-   struct option *options = NULL;
+   struct option *options;
+   struct option checkout_options[] = {
+   OPT_STRING('b', NULL, &opts.new_branch, N_("branch"),
+  N_("create and checkout a new branch")),
+   OPT_STRING('B', NULL, &opts.new_branch_force, N_("branch"),
+  N_("create/reset and checkout a branch")),
+   OPT_END()
+   };
int ret;
 
memset(&opts, 0, sizeof(opts));
opts.no_dwim_new_local_branch = 0;
 
-   options = parse_options_dup(options);
+   options = parse_options_dup(checkout_options);
options = add_common_options(&opts, options);
-   options = add_switch_branch_options(&opts, options);
+   options = add_common_switch_branch_options(&opts, options);
options = add_checkout_path_options(&opts, options);
 
ret = checkout_main(argc, argv, prefix, &opts,
@@ -1592,14 +1595,21 @@ int cmd_switch(int argc, const char **argv, const char 
*prefix)
 {
struct checkout_opts opts;
struct option *options = NULL;
+   struct option switch_options[] = {
+   OPT_STRING('c', "create", &opts.new_branch, N_("branch"),
+  N_("create and switch to a new branch")),
+   OPT_STRING('C', "force-create", &opts.new_branch_force, 
N_("branch"),
+  N_("create/reset and switch to a branch")),
+   OPT_END()
+   };
int ret;
 
memset(&opts, 0, sizeof(opts));
opts.no_dwim_new_local_branch = 0;
 
-   options = parse_options_dup(options);
+   options = parse_options_dup(switch_options);
options = add_common_options(&opts, options);
-   options = add_switch_branch_options(&opts, options);
+   options = add_common_switch_branch_options(&opts, options);
 
ret = checkout_main(argc, argv, prefix, &opts,
options, switch_branch_usage);
-- 
2.21.0.rc1.337.gdf7f8d0522



[PATCH v3 07/21] checkout: make "opts" in cmd_checkout() a pointer

2019-03-08 Thread Nguyễn Thái Ngọc Duy
"opts" will soon be moved out of cmd_checkout(). To keep changes in
that patch smaller, convert "opts" to a pointer and keep the real
thing behind "real_opts".
---
 builtin/checkout.c | 115 +++--
 1 file changed, 58 insertions(+), 57 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index cbdcb1417f..baefe3c942 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1341,82 +1341,83 @@ static int checkout_branch(struct checkout_opts *opts,
 
 int cmd_checkout(int argc, const char **argv, const char *prefix)
 {
-   struct checkout_opts opts;
+   struct checkout_opts real_opts;
+   struct checkout_opts *opts = &real_opts;
struct branch_info new_branch_info;
char *conflict_style = NULL;
int dwim_new_local_branch, no_dwim_new_local_branch = 0;
int dwim_remotes_matched = 0;
struct option options[] = {
-   OPT__QUIET(&opts.quiet, N_("suppress progress reporting")),
-   OPT_STRING('b', NULL, &opts.new_branch, N_("branch"),
+   OPT__QUIET(&opts->quiet, N_("suppress progress reporting")),
+   OPT_STRING('b', NULL, &opts->new_branch, N_("branch"),
   N_("create and checkout a new branch")),
-   OPT_STRING('B', NULL, &opts.new_branch_force, N_("branch"),
+   OPT_STRING('B', NULL, &opts->new_branch_force, N_("branch"),
   N_("create/reset and checkout a branch")),
-   OPT_BOOL('l', NULL, &opts.new_branch_log, N_("create reflog for 
new branch")),
-   OPT_BOOL(0, "detach", &opts.force_detach, N_("detach HEAD at 
named commit")),
-   OPT_SET_INT('t', "track",  &opts.track, N_("set upstream info 
for new branch"),
+   OPT_BOOL('l', NULL, &opts->new_branch_log, N_("create reflog 
for new branch")),
+   OPT_BOOL(0, "detach", &opts->force_detach, N_("detach HEAD at 
named commit")),
+   OPT_SET_INT('t', "track",  &opts->track, N_("set upstream info 
for new branch"),
BRANCH_TRACK_EXPLICIT),
-   OPT_STRING(0, "orphan", &opts.new_orphan_branch, 
N_("new-branch"), N_("new unparented branch")),
-   OPT_SET_INT_F('2', "ours", &opts.writeout_stage,
+   OPT_STRING(0, "orphan", &opts->new_orphan_branch, 
N_("new-branch"), N_("new unparented branch")),
+   OPT_SET_INT_F('2', "ours", &opts->writeout_stage,
  N_("checkout our version for unmerged files"),
  2, PARSE_OPT_NONEG),
-   OPT_SET_INT_F('3', "theirs", &opts.writeout_stage,
+   OPT_SET_INT_F('3', "theirs", &opts->writeout_stage,
  N_("checkout their version for unmerged files"),
  3, PARSE_OPT_NONEG),
-   OPT__FORCE(&opts.force, N_("force checkout (throw away local 
modifications)"),
+   OPT__FORCE(&opts->force, N_("force checkout (throw away local 
modifications)"),
   PARSE_OPT_NOCOMPLETE),
-   OPT_BOOL('m', "merge", &opts.merge, N_("perform a 3-way merge 
with the new branch")),
-   OPT_BOOL_F(0, "overwrite-ignore", &opts.overwrite_ignore,
+   OPT_BOOL('m', "merge", &opts->merge, N_("perform a 3-way merge 
with the new branch")),
+   OPT_BOOL_F(0, "overwrite-ignore", &opts->overwrite_ignore,
   N_("update ignored files (default)"),
   PARSE_OPT_NOCOMPLETE),
OPT_STRING(0, "conflict", &conflict_style, N_("style"),
   N_("conflict style (merge or diff3)")),
-   OPT_BOOL('p', "patch", &opts.patch_mode, N_("select hunks 
interactively")),
-   OPT_BOOL(0, "ignore-skip-worktree-bits", 
&opts.ignore_skipworktree,
+   OPT_BOOL('p', "patch", &opts->patch_mode, N_("select hunks 
interactively")),
+   OPT_BOOL(0, "ignore-skip-worktree-bits", 
&opts->ignore_skipworktree,
 N_("do not limit pathspecs to sparse entries only")),
OPT_BOOL(0, "no-guess", &no_dwim_new_local_branch,
 N_("do not second guess 'git checkout 
'")),
-   OPT_BOOL(0, "ignore-other-worktrees", 
&opts.ignore_other_worktrees,
+   OPT_BOOL(0, "ignore-other-worktrees", 
&opts->ignore_other_worktrees,
 N_("do not check if another worktree is holding the 
given ref")),
{ OPTION_CALLBACK, 0, "recurse-submodules", NULL,
"checkout", "control recursive updating of 
submodules",
PARSE_OPT_OPTARG, 
option_parse_recurse_submodules_worktree_updater },
-   OPT_BOOL(0, "progress", &opts.show_progress, N_("force progress 
reporting")),
-   OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay mo

[PATCH v3 20/21] completion: support switch

2019-03-08 Thread Nguyễn Thái Ngọc Duy
Completion support for --guess could be made better. If no --detach is
given, we should only provide a list of refs/heads/* and dwim ones,
not the entire ref space. But I still can't penetrate that
__git_refs() function yet.
---
 contrib/completion/git-completion.bash | 27 ++
 1 file changed, 27 insertions(+)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 976e4a6548..7fcf28d437 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2158,6 +2158,33 @@ _git_status ()
__git_complete_index_file "$complete_opt"
 }
 
+_git_switch ()
+{
+   case "$cur" in
+   --conflict=*)
+   __gitcomp "diff3 merge" "" "${cur##--conflict=}"
+   ;;
+   --*)
+   __gitcomp_builtin switch
+   ;;
+   *)
+   # check if ---guess was specified to enable DWIM mode
+   local track_opt= only_local_ref=n
+   if [ -n "$(__git_find_on_cmdline "-g --guess")" ]; then
+   track_opt='--track'
+   fi
+   if [ -z "$(__git_find_on_cmdline "-d --detach")" ]; then
+   only_local_ref=y
+   fi
+   if [ $only_local_ref = y -a -n "$track_opt"]; then
+   __gitcomp_direct "$(__git_heads "" "$cur" " ")"
+   else
+   __git_complete_refs $track_opt
+   fi
+   ;;
+   esac
+}
+
 __git_config_get_set_variables ()
 {
local prevword word config_file= c=$cword
-- 
2.21.0.rc1.337.gdf7f8d0522



[PATCH v3 21/21] doc: promote "git switch"

2019-03-08 Thread Nguyễn Thái Ngọc Duy
The new command "git switch" is added to avoid the confusion of
one-command-do-all "git checkout" for new users. They are also helpful
to avoid ambiguation context.

For these reasons, promote it everywhere possible. This includes
documentation, suggestions/advice from other commands...
---
 Documentation/git-branch.txt   | 12 +++---
 Documentation/git-check-ref-format.txt |  3 +-
 Documentation/git-format-patch.txt |  2 +-
 Documentation/git-merge-base.txt   |  2 +-
 Documentation/git-rebase.txt   |  2 +-
 Documentation/git-remote.txt   |  2 +-
 Documentation/git-rerere.txt   | 10 ++---
 Documentation/git-reset.txt| 20 -
 Documentation/git-stash.txt|  9 +++--
 Documentation/gitcore-tutorial.txt | 19 +
 Documentation/giteveryday.txt  | 24 +--
 Documentation/gittutorial.txt  |  4 +-
 Documentation/gitworkflows.txt |  3 +-
 Documentation/revisions.txt|  2 +-
 Documentation/user-manual.txt  | 56 +-
 advice.c   | 11 +++--
 sha1-name.c|  2 +-
 t/t2020-checkout-detach.sh | 16 
 18 files changed, 101 insertions(+), 98 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 0cd87ddeff..1e2d89b174 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -48,7 +48,7 @@ The command's second form creates a new branch head named 

 which points to the current `HEAD`, or  if given.
 
 Note that this will create the new branch, but it will not switch the
-working tree to it; use "git checkout " to switch to the
+working tree to it; use "git switch " to switch to the
 new branch.
 
 When a local branch is started off a remote-tracking branch, Git sets up the
@@ -198,7 +198,7 @@ This option is only applicable in non-verbose mode.
 +
 This behavior is the default when the start point is a remote-tracking branch.
 Set the branch.autoSetupMerge configuration variable to `false` if you
-want `git checkout` and `git branch` to always behave as if `--no-track`
+want `git switch`, `git checkout` and `git branch` to always behave as if 
`--no-track`
 were given. Set it to `always` if you want this behavior when the
 start-point is either a local or remote-tracking branch.
 
@@ -297,7 +297,7 @@ Start development from a known tag::
 $ git clone git://git.kernel.org/pub/scm/.../linux-2.6 my2.6
 $ cd my2.6
 $ git branch my2.6.14 v2.6.14   <1>
-$ git checkout my2.6.14
+$ git switch my2.6.14
 
 +
 <1> This step and the next one could be combined into a single step with
@@ -322,9 +322,9 @@ $ git branch -D test<2>
 NOTES
 -
 
-If you are creating a branch that you want to checkout immediately, it is
-easier to use the git checkout command with its `-b` option to create
-a branch and check it out with a single command.
+If you are creating a branch that you want to switch to immediately,
+it is easier to use the "git switch" command with its `-c` option to
+do the same thing with a single command.
 
 The options `--contains`, `--no-contains`, `--merged` and `--no-merged`
 serve four related but different purposes:
diff --git a/Documentation/git-check-ref-format.txt 
b/Documentation/git-check-ref-format.txt
index d9de992585..ee6a4144fb 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -88,7 +88,8 @@ but it is explicitly forbidden at the beginning of a branch 
name).
 When run with `--branch` option in a repository, the input is first
 expanded for the ``previous checkout syntax''
 `@{-n}`.  For example, `@{-1}` is a way to refer the last thing that
-was checked out using "git checkout" operation. This option should be
+was checked out using "git switch" or "git checkout" operation.
+This option should be
 used by porcelains to accept this syntax anywhere a branch name is
 expected, so they can act as if you typed the branch name. As an
 exception note that, the ``previous checkout operation'' might result
diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index 1af85d404f..0a24a5679e 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -421,7 +421,7 @@ One way to test if your MUA is set up correctly is:
 * Apply it:
 
 $ git fetch  master:test-apply
-$ git checkout test-apply
+$ git switch test-apply
 $ git reset --hard
 $ git am a.patch
 
diff --git a/Documentation/git-merge-base.txt b/Documentation/git-merge-base.txt
index 9f07f4f6ed..261d5c1164 100644
--- a/Documentation/git-merge-base.txt
+++ b/Documentation/git-merge-base.txt
@@ -149,7 +149,7 @@ instead.
 Discussion on fork-point mode
 -
 
-After working on the `topic` branch created with `git checkout -b
+After working on the `topic` branch created with `git switch -c
 topic

[PATCH v3 19/21] t: add tests for switch

2019-03-08 Thread Nguyễn Thái Ngọc Duy
---
 t/t2060-switch.sh | 87 +++
 1 file changed, 87 insertions(+)
 create mode 100755 t/t2060-switch.sh

diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh
new file mode 100755
index 00..1e1e834c1b
--- /dev/null
+++ b/t/t2060-switch.sh
@@ -0,0 +1,87 @@
+#!/bin/sh
+
+test_description='switch basic functionality'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+   test_commit first &&
+   git branch first-branch &&
+   test_commit second &&
+   test_commit third &&
+   git remote add origin nohost:/nopath &&
+   git update-ref refs/remotes/origin/foo first-branch
+'
+
+test_expect_success 'switch branch no arguments' '
+   test_must_fail git switch
+'
+
+test_expect_success 'switch branch' '
+   git switch first-branch &&
+   test_path_is_missing second.t
+'
+
+test_expect_success 'switch to a commit' '
+   test_must_fail git switch master^{commit}
+'
+
+test_expect_success 'switch and detach' '
+   test_when_finished git switch master &&
+   git switch --detach master^{commit} &&
+   test_must_fail git symbolic-ref HEAD
+'
+
+test_expect_success 'switch and detach current branch' '
+   test_when_finished git switch master &&
+   git switch master &&
+   git switch --detach &&
+   test_must_fail git symbolic-ref HEAD
+'
+
+test_expect_success 'switch and create branch' '
+   test_when_finished git switch master &&
+   git switch -c temp master^ &&
+   test_cmp_rev master^ refs/heads/temp &&
+   echo refs/heads/temp >expected-branch &&
+   git symbolic-ref HEAD >actual-branch &&
+   test_cmp expected-branch actual-branch
+'
+
+test_expect_success 'force create branch from HEAD' '
+   test_when_finished git switch master &&
+   git switch --detach master &&
+   git switch -C temp &&
+   test_cmp_rev master refs/heads/temp &&
+   echo refs/heads/temp >expected-branch &&
+   git symbolic-ref HEAD >actual-branch &&
+   test_cmp expected-branch actual-branch
+'
+
+test_expect_success 'new orphan branch' '
+   test_when_finished git switch master &&
+   git switch --orphan new-orphan master^ &&
+   test_commit orphan &&
+   git cat-file commit refs/heads/new-orphan >commit &&
+   ! grep ^parent commit
+'
+
+test_expect_success 'switching ignores file of same branch name' '
+   test_when_finished git switch master &&
+   : >first-branch &&
+   git switch first-branch &&
+   echo refs/heads/first-branch >expected &&
+   git symbolic-ref HEAD >actual &&
+   test_commit expected actual
+'
+
+test_expect_success 'guess and create branch ' '
+   test_when_finished git switch master &&
+   test_must_fail git switch foo &&
+   git switch --guess foo &&
+   echo refs/heads/foo >expected &&
+   git symbolic-ref HEAD >actual &&
+   test_cmp expected actual
+'
+
+test_done
-- 
2.21.0.rc1.337.gdf7f8d0522



[PATCH v3 18/21] switch: no worktree status unless real branch switch happens

2019-03-08 Thread Nguyễn Thái Ngọc Duy
When we switch from one branch to another, it makes sense to show a
summary of local changes since there could be conflicts, or some files
left modified When switch is used solely for creating a new
branch (and "switch" to the same commit) or detaching, we don't really
need to show anything.

"git checkout" does it anyway for historical reasons. But we can start
with a clean slate with switch and don't have to.

This essentially reverts fa655d8411 (checkout: optimize "git checkout
-b " - 2018-08-16) and make it default for switch,
but also for -B and --detach. Users of big repos are encouraged to
move to switch.
---
 Documentation/config/checkout.txt |   8 --
 builtin/checkout.c| 134 ++
 t/t1090-sparse-checkout-scope.sh  |  14 
 3 files changed, 8 insertions(+), 148 deletions(-)

diff --git a/Documentation/config/checkout.txt 
b/Documentation/config/checkout.txt
index d6872ffa83..6b646813ab 100644
--- a/Documentation/config/checkout.txt
+++ b/Documentation/config/checkout.txt
@@ -16,11 +16,3 @@ will checkout the '' branch on another remote,
 and by linkgit:git-worktree[1] when 'git worktree add' refers to a
 remote branch. This setting might be used for other checkout-like
 commands or functionality in the future.
-
-checkout.optimizeNewBranch::
-   Optimizes the performance of "git checkout -b " when
-   using sparse-checkout.  When set to true, git will not update the
-   repo based on the current sparse-checkout settings.  This means it
-   will not update the skip-worktree bit in the index nor add/remove
-   files in the working directory to reflect the current sparse checkout
-   settings nor will it show the local changes.
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8a89df4f36..4903359b49 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -26,8 +26,6 @@
 #include "submodule.h"
 #include "advice.h"
 
-static int checkout_optimize_new_branch;
-
 static const char * const checkout_usage[] = {
N_("git checkout [] "),
N_("git checkout [] [] -- ..."),
@@ -56,11 +54,7 @@ struct checkout_opts {
int dwim_new_local_branch;
int accept_pathspec;
int switch_branch_doing_nothing_is_ok;
-
-   /*
-* If new checkout options are added, skip_merge_working_tree
-* should be updated accordingly.
-*/
+   int only_merge_on_switching_branches;
 
const char *new_branch;
const char *new_branch_force;
@@ -564,112 +558,6 @@ static void setup_branch_path(struct branch_info *branch)
branch->path = strbuf_detach(&buf, NULL);
 }
 
-/*
- * Skip merging the trees, updating the index and working directory if and
- * only if we are creating a new branch via "git checkout -b ."
- */
-static int skip_merge_working_tree(const struct checkout_opts *opts,
-   const struct branch_info *old_branch_info,
-   const struct branch_info *new_branch_info)
-{
-   /*
-* Do the merge if sparse checkout is on and the user has not opted in
-* to the optimized behavior
-*/
-   if (core_apply_sparse_checkout && !checkout_optimize_new_branch)
-   return 0;
-
-   /*
-* We must do the merge if we are actually moving to a new commit.
-*/
-   if (!old_branch_info->commit || !new_branch_info->commit ||
-   !oideq(&old_branch_info->commit->object.oid,
-  &new_branch_info->commit->object.oid))
-   return 0;
-
-   /*
-* opts->patch_mode cannot be used with switching branches so is
-* not tested here
-*/
-
-   /*
-* opts->quiet only impacts output so doesn't require a merge
-*/
-
-   /*
-* Honor the explicit request for a three-way merge or to throw away
-* local changes
-*/
-   if (opts->merge || opts->force)
-   return 0;
-
-   /*
-* --detach is documented as "updating the index and the files in the
-* working tree" but this optimization skips those steps so fall through
-* to the regular code path.
-*/
-   if (opts->force_detach)
-   return 0;
-
-   /*
-* opts->writeout_stage cannot be used with switching branches so is
-* not tested here
-*/
-
-   /*
-* Honor the explicit ignore requests
-*/
-   if (!opts->overwrite_ignore || opts->ignore_skipworktree ||
-   opts->ignore_other_worktrees)
-   return 0;
-
-   /*
-* opts->show_progress only impacts output so doesn't require a merge
-*/
-
-   /*
-* opts->overlay_mode cannot be used with switching branches so is
-* not tested here
-*/
-
-   /*
-* If we aren't creating a new branch any changes or updates will
-* happen in the existing branch.  Since that could only be updating
-* the index and working directory, we d

[PATCH v1 07/11] restore: default to --source=HEAD when only --index is specified

2019-03-08 Thread Nguyễn Thái Ngọc Duy
"git restore --index" does not make much sense since we're told to
restore the index from the (by default) index. Set default source to
HEAD in this case. This is basically the same as "git reset -- ".

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/checkout.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 07b431be51..f06439dbeb 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1426,6 +1426,12 @@ static int checkout_main(int argc, const char **argv, 
const char *prefix,
}
if (opts->checkout_index < 0 || opts->checkout_worktree < 0)
BUG("these flags should be non-negative by now");
+   /*
+* convenient shortcut: "git restore --index" equals
+* "git restore --index --source HEAD"
+*/
+   if (!opts->from_treeish && opts->checkout_index && 
!opts->checkout_worktree)
+   opts->from_treeish = "HEAD";
 
/*
 * From here on, new_branch will contain the branch to be checked out,
-- 
2.21.0.rc1.337.gdf7f8d0522



[PATCH v1 09/11] t: add tests for restore

2019-03-08 Thread Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 t/lib-patch-mode.sh   |  12 
 t/t2070-restore.sh (new +x)   |  77 ++
 t/t2071-restore-patch.sh (new +x) | 105 ++
 3 files changed, 194 insertions(+)

diff --git a/t/lib-patch-mode.sh b/t/lib-patch-mode.sh
index 06c3c91762..cfd76bf987 100644
--- a/t/lib-patch-mode.sh
+++ b/t/lib-patch-mode.sh
@@ -2,28 +2,40 @@
 
 . ./test-lib.sh
 
+# set_state   
+#
+# Prepare the content for path in worktree and the index as specified.
 set_state () {
echo "$3" > "$1" &&
git add "$1" &&
echo "$2" > "$1"
 }
 
+# save_state 
+#
+# Save index/worktree content of  in the files _worktree_
+# and _index_
 save_state () {
noslash="$(echo "$1" | tr / _)" &&
cat "$1" > _worktree_"$noslash" &&
git show :"$1" > _index_"$noslash"
 }
 
+# set_and_save_state   
 set_and_save_state () {
set_state "$@" &&
save_state "$1"
 }
 
+# verify_state   
 verify_state () {
test "$(cat "$1")" = "$2" &&
test "$(git show :"$1")" = "$3"
 }
 
+# verify_saved_state 
+#
+# Call verify_state with expected contents from the last save_state
 verify_saved_state () {
noslash="$(echo "$1" | tr / _)" &&
verify_state "$1" "$(cat _worktree_"$noslash")" "$(cat 
_index_"$noslash")"
diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh
new file mode 100755
index 00..df91bf54bc
--- /dev/null
+++ b/t/t2070-restore.sh
@@ -0,0 +1,77 @@
+#!/bin/sh
+
+test_description='restore basic functionality'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+   test_commit first &&
+   echo first-and-a-half >>first.t &&
+   git add first.t &&
+   test_commit second &&
+   echo one >one &&
+   echo two >two &&
+   echo untracked >untracked &&
+   echo ignored >ignored &&
+   echo /ignored >.gitignore &&
+   git add one two .gitignore &&
+   git update-ref refs/heads/one master
+'
+
+test_expect_success 'restore without pathspec is not ok' '
+   test_must_fail git restore &&
+   test_must_fail git restore --source=first
+'
+
+test_expect_success 'restore -p without pathspec is fine' '
+   echo q >cmd &&
+   git restore -p expected &&
+   echo dirty >>one &&
+   git restore one &&
+   test_cmp expected one
+'
+
+test_expect_success 'restore a file on worktree from another branch' '
+   test_when_finished git reset --hard &&
+   git cat-file blob first:./first.t >expected &&
+   git restore --source=first first.t &&
+   test_cmp expected first.t &&
+   git cat-file blob HEAD:./first.t >expected &&
+   git show :first.t >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success 'restore a file in the index from another branch' '
+   test_when_finished git reset --hard &&
+   git cat-file blob first:./first.t >expected &&
+   git restore --source=first --index first.t &&
+   git show :first.t >actual &&
+   test_cmp expected actual &&
+   git cat-file blob HEAD:./first.t >expected &&
+   test_cmp expected first.t
+'
+
+test_expect_success 'restore a file in both the index and worktree from 
another branch' '
+   test_when_finished git reset --hard &&
+   git cat-file blob first:./first.t >expected &&
+   git restore --source=first --index --worktree first.t &&
+   git show :first.t >actual &&
+   test_cmp expected actual &&
+   test_cmp expected first.t
+'
+
+test_expect_success 'restore --index uses HEAD as source' '
+   test_when_finished git reset --hard &&
+   git cat-file blob :./first.t >expected &&
+   echo index-dirty >>first.t &&
+   git add first.t &&
+   git restore --index first.t &&
+   git cat-file blob :./first.t >actual &&
+   test_cmp expected actual
+'
+
+test_done
diff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh
new file mode 100755
index 00..46ebcb2413
--- /dev/null
+++ b/t/t2071-restore-patch.sh
@@ -0,0 +1,105 @@
+#!/bin/sh
+
+test_description='git restore --patch'
+
+. ./lib-patch-mode.sh
+
+test_expect_success PERL 'setup' '
+   mkdir dir &&
+   echo parent >dir/foo &&
+   echo dummy >bar &&
+   git add bar dir/foo &&
+   git commit -m initial &&
+   test_tick &&
+   test_commit second dir/foo head &&
+   set_and_save_state bar bar_work bar_index &&
+   save_head
+'
+
+# note: bar sorts before dir/foo, so the first 'n' is always to skip 'bar'
+
+test_expect_success PERL 'saying "n" does nothing' '
+   set_and_save_state dir/foo work head &&
+   test_write_lines n n | git restore -p &&
+   verify_saved_state bar &&
+   verify_saved_state dir/foo
+'
+
+test_expect_success PERL 'git restore -p' '
+   set_and_save_state dir/foo work head &&
+   test_write_lines n y | git restore -p &&
+   verify_saved_state bar &&
+   verify_state dir/foo head head
+'
+
+test_expect_success PERL 'git re

[PATCH v1 05/11] checkout: factor out worktree checkout code

2019-03-08 Thread Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/checkout.c | 108 +
 1 file changed, 59 insertions(+), 49 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 9e59bf792f..5fb85e7b73 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -326,17 +326,73 @@ static void mark_ce_for_checkout_no_overlay(struct 
cache_entry *ce,
}
 }
 
+static int checkout_worktree(const struct checkout_opts *opts)
+{
+   struct checkout state = CHECKOUT_INIT;
+   int nr_checkouts = 0, nr_unmerged = 0;
+   int errs = 0;
+   int pos;
+
+   state.force = 1;
+   state.refresh_cache = 1;
+   state.istate = &the_index;
+
+   enable_delayed_checkout(&state);
+   for (pos = 0; pos < active_nr; pos++) {
+   struct cache_entry *ce = active_cache[pos];
+   if (ce->ce_flags & CE_MATCHED) {
+   if (!ce_stage(ce)) {
+   errs |= checkout_entry(ce, &state,
+  NULL, &nr_checkouts);
+   continue;
+   }
+   if (opts->writeout_stage)
+   errs |= checkout_stage(opts->writeout_stage,
+  ce, pos,
+  &state,
+  &nr_checkouts, 
opts->overlay_mode);
+   else if (opts->merge)
+   errs |= checkout_merged(pos, &state,
+   &nr_unmerged);
+   pos = skip_same_name(ce, pos) - 1;
+   }
+   }
+   remove_marked_cache_entries(&the_index, 1);
+   remove_scheduled_dirs();
+   errs |= finish_delayed_checkout(&state, &nr_checkouts);
+
+   if (opts->count_checkout_paths) {
+   if (nr_unmerged)
+   fprintf_ln(stderr, Q_("Recreated %d merge conflict",
+ "Recreated %d merge conflicts",
+ nr_unmerged),
+  nr_unmerged);
+   if (opts->source_tree)
+   fprintf_ln(stderr, Q_("Updated %d path from %s",
+ "Updated %d paths from %s",
+ nr_checkouts),
+  nr_checkouts,
+  
find_unique_abbrev(&opts->source_tree->object.oid,
+ DEFAULT_ABBREV));
+   else if (!nr_unmerged || nr_checkouts)
+   fprintf_ln(stderr, Q_("Updated %d path from the index",
+ "Updated %d paths from the index",
+ nr_checkouts),
+  nr_checkouts);
+   }
+
+   return errs;
+}
+
 static int checkout_paths(const struct checkout_opts *opts,
  const char *revision)
 {
int pos;
-   struct checkout state = CHECKOUT_INIT;
static char *ps_matched;
struct object_id rev;
struct commit *head;
int errs = 0;
struct lock_file lock_file = LOCK_INIT;
-   int nr_checkouts = 0, nr_unmerged = 0;
 
trace2_cmd_mode(opts->patch_mode ? "patch" : "path");
 
@@ -422,53 +478,7 @@ static int checkout_paths(const struct checkout_opts *opts,
return 1;
 
/* Now we are committed to check them out */
-   state.force = 1;
-   state.refresh_cache = 1;
-   state.istate = &the_index;
-
-   enable_delayed_checkout(&state);
-   for (pos = 0; pos < active_nr; pos++) {
-   struct cache_entry *ce = active_cache[pos];
-   if (ce->ce_flags & CE_MATCHED) {
-   if (!ce_stage(ce)) {
-   errs |= checkout_entry(ce, &state,
-  NULL, &nr_checkouts);
-   continue;
-   }
-   if (opts->writeout_stage)
-   errs |= checkout_stage(opts->writeout_stage,
-  ce, pos,
-  &state,
-  &nr_checkouts, 
opts->overlay_mode);
-   else if (opts->merge)
-   errs |= checkout_merged(pos, &state,
-   &nr_unmerged);
-   pos = skip_same_name(ce, pos) - 1;
-   }
-   }
-   remove_marked_cache_entries(&the_index, 1);
-   remove_scheduled_dirs();
-   errs |= finish_delayed_checkout(&

[PATCH v1 08/11] restore: support --patch

2019-03-08 Thread Nguyễn Thái Ngọc Duy
git-restore is different from git-checkout that it only restores the
worktree by default, not both worktree and index. add--interactive
needs some update to support this mode.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/checkout.c|  5 ++--
 git-add--interactive.perl | 52 +++
 2 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index f06439dbeb..73de110d42 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -436,9 +436,10 @@ static int checkout_paths(const struct checkout_opts *opts,
patch_mode = "--patch=checkout";
else if (opts->checkout_index && !opts->checkout_worktree)
patch_mode = "--patch=reset";
+   else if (!opts->checkout_index && opts->checkout_worktree)
+   patch_mode = "--patch=worktree";
else
-   die(_("'%s' with only '%s' is not currently supported"),
-   "--patch", "--worktree");
+   BUG("either flag must have been set");
return run_add_interactive(revision, patch_mode, 
&opts->pathspec);
}
 
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 20eb81cc92..3dfb3629c9 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -149,6 +149,20 @@ sub colored {
FILTER => undef,
IS_REVERSE => 0,
},
+   'worktree_head' => {
+   DIFF => 'diff-index -p',
+   APPLY => sub { apply_patch 'apply -R', @_ },
+   APPLY_CHECK => 'apply -R',
+   FILTER => undef,
+   IS_REVERSE => 1,
+   },
+   'worktree_nothead' => {
+   DIFF => 'diff-index -R -p',
+   APPLY => sub { apply_patch 'apply', @_ },
+   APPLY_CHECK => 'apply',
+   FILTER => undef,
+   IS_REVERSE => 0,
+   },
 );
 
 $patch_mode = 'stage';
@@ -1049,6 +1063,12 @@ sub color_diff {
 marked for discarding."),
checkout_nothead => N__(
 "If the patch applies cleanly, the edited hunk will immediately be
+marked for applying."),
+   worktree_head => N__(
+"If the patch applies cleanly, the edited hunk will immediately be
+marked for discarding."),
+   worktree_nothead => N__(
+"If the patch applies cleanly, the edited hunk will immediately be
 marked for applying."),
 );
 
@@ -1259,6 +1279,18 @@ sub edit_hunk_loop {
 n - do not apply this hunk to index and worktree
 q - quit; do not apply this hunk or any of the remaining ones
 a - apply this hunk and all later hunks in the file
+d - do not apply this hunk or any of the later hunks in the file"),
+   worktree_head => N__(
+"y - discard this hunk from worktree
+n - do not discard this hunk from worktree
+q - quit; do not discard this hunk or any of the remaining ones
+a - discard this hunk and all later hunks in the file
+d - do not discard this hunk or any of the later hunks in the file"),
+   worktree_nothead => N__(
+"y - apply this hunk to worktree
+n - do not apply this hunk to worktree
+q - quit; do not apply this hunk or any of the remaining ones
+a - apply this hunk and all later hunks in the file
 d - do not apply this hunk or any of the later hunks in the file"),
 );
 
@@ -1421,6 +1453,16 @@ sub display_hunks {
deletion => N__("Apply deletion to index and worktree 
[y,n,q,a,d%s,?]? "),
hunk => N__("Apply this hunk to index and worktree 
[y,n,q,a,d%s,?]? "),
},
+   worktree_head => {
+   mode => N__("Discard mode change from worktree [y,n,q,a,d%s,?]? 
"),
+   deletion => N__("Discard deletion from worktree 
[y,n,q,a,d%s,?]? "),
+   hunk => N__("Discard this hunk from worktree [y,n,q,a,d%s,?]? 
"),
+   },
+   worktree_nothead => {
+   mode => N__("Apply mode change to worktree [y,n,q,a,d%s,?]? "),
+   deletion => N__("Apply deletion to worktree [y,n,q,a,d%s,?]? "),
+   hunk => N__("Apply this hunk to worktree [y,n,q,a,d%s,?]? "),
+   },
 );
 
 sub patch_update_file {
@@ -1756,6 +1798,16 @@ sub process_args {
   'checkout_head' : 
'checkout_nothead');
$arg = shift @ARGV or die __("missing 
--");
}
+   } elsif ($1 eq 'worktree') {
+   $arg = shift @ARGV or die __("missing --");
+   if ($arg eq '--') {
+   $patch_mode = 'checkout_index';
+   } else {
+   $patch_mode_revision = $arg;
+   $patch_mode = ($arg eq 'HEAD' ?
+  'worktree_head' : 
'worktree_nothead');
+ 

[PATCH v1 06/11] restore: add --worktree and --index

2019-03-08 Thread Nguyễn Thái Ngọc Duy
'git checkout  ' updates both index and
worktree. But updating the index when you want to restore worktree
files is non-intuitive. The index contains the data ready for the next
commit, and there's no indication that the user will want to commit
the restored versions.

'git restore' therefore by default only touches worktree. The user has
the option to update either the index with

git restore --source= --index   (1)

or update both with

git restore --source= --index --worktree  (2)

PS. Orignally I wanted to make worktree update default and form (1)
would add index update while also updating the worktree, and the user
would need to do "--index --no-worktree" to update index only. But it
looks really confusing that "--index" option alone updates both. So
now form (2) is used for both, which reads much more obvious.

PPS. Yes form (1) overlaps with "git reset  ". I don't know
if we can ever turn "git reset" back to "_always_ reset HEAD and
optionally do something else".

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/checkout.c | 74 ++
 1 file changed, 68 insertions(+), 6 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5fb85e7b73..07b431be51 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -62,6 +62,8 @@ struct checkout_opts {
int switch_branch_doing_nothing_is_ok;
int only_merge_on_switching_branches;
int empty_pathspec_ok;
+   int checkout_index;
+   int checkout_worktree;
 
const char *new_branch;
const char *new_branch_force;
@@ -393,6 +395,7 @@ static int checkout_paths(const struct checkout_opts *opts,
struct commit *head;
int errs = 0;
struct lock_file lock_file = LOCK_INIT;
+   int checkout_index;
 
trace2_cmd_mode(opts->patch_mode ? "patch" : "path");
 
@@ -418,9 +421,26 @@ static int checkout_paths(const struct checkout_opts *opts,
die(_("Cannot update paths and switch to branch '%s' at the 
same time."),
opts->new_branch);
 
-   if (opts->patch_mode)
-   return run_add_interactive(revision, "--patch=checkout",
-  &opts->pathspec);
+   if (!opts->checkout_worktree && !opts->checkout_index)
+   die(_("neither '%s' or '%s' is specified"),
+   "--index", "--worktree");
+
+   if (!opts->source_tree && !opts->checkout_worktree)
+   die(_("'%s' must be used when '%s' is not specified"),
+   "--worktree", "--source");
+
+   if (opts->patch_mode) {
+   const char *patch_mode;
+
+   if (opts->checkout_index && opts->checkout_worktree)
+   patch_mode = "--patch=checkout";
+   else if (opts->checkout_index && !opts->checkout_worktree)
+   patch_mode = "--patch=reset";
+   else
+   die(_("'%s' with only '%s' is not currently supported"),
+   "--patch", "--worktree");
+   return run_add_interactive(revision, patch_mode, 
&opts->pathspec);
+   }
 
repo_hold_locked_index(the_repository, &lock_file, LOCK_DIE_ON_ERROR);
if (read_cache_preload(&opts->pathspec) < 0)
@@ -478,10 +498,30 @@ static int checkout_paths(const struct checkout_opts 
*opts,
return 1;
 
/* Now we are committed to check them out */
-   errs |= checkout_worktree(opts);
+   if (opts->checkout_worktree)
+   errs |= checkout_worktree(opts);
 
-   if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
-   die(_("unable to write new index file"));
+   /*
+* Allow updating the index when checking out from the index.
+* This is to save new stat info.
+*/
+   if (opts->checkout_worktree && !opts->checkout_index && 
!opts->source_tree)
+   checkout_index = 1;
+   else
+   checkout_index = opts->checkout_index;
+
+   if (checkout_index) {
+   if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
+   die(_("unable to write new index file"));
+   } else {
+   /*
+* NEEDSWORK: if --worktree is not specified, we
+* should save stat info of checked out files in the
+* index to avoid the next (potentially costly)
+* refresh. But it's a bit tricker to do...
+*/
+   rollback_lock_file(&lock_file);
+   }
 
read_ref_full("HEAD", 0, &rev, NULL);
head = lookup_commit_reference_gently(the_repository, &rev, 1);
@@ -1373,6 +1413,20 @@ static int checkout_main(int argc, const char **argv, 
const char *prefix,
if (opts->overlay_mode == 1 && opts->patch_mode)
die(_("-p and --overlay are mutually exclusive"));
 
+   if (opts->checkout_index >= 0 || opts->checkout_w

[PATCH v1 00/11] And new command "restore"

2019-03-08 Thread Nguyễn Thái Ngọc Duy
This is the companion of "git switch" [1] and is based on that topic.
This command peforms the "checkout paths" from git-checkout, git-reset
and also has a third mode to reset only worktree, leaving the index
alone.

For new people not aware of previous discussions, this command is
supposed to be a friendlier replacement for "git checkout" and
hopefully fixes some warts that have grown over the time in git-checkout.
For this reason, the last patch starts to recommend "git restore"
everywhere.

For old people, I'm surprised nobody reacts to the command rename from
restore-files to restore. "restore" vs "reset" is certainly confusing.
My hope was to remove "common porcelain" status from reset, but I
probably will not succeed in doing that.

Some open issues from the last discussion [2]

- intent-to-add support. This will come but maybe later.

- --index has a different meaning in git-apply. I don't have a good
  suggestion except "yeah we have two different worlds now, the old
  and the new one"

The series is also available at [3]

[1] https://public-inbox.org/git/20190308095752.8574-1-pclo...@gmail.com
[2] 
https://public-inbox.org/git/cacsjy8cqhwec3b6egpepurejfox7c17x61-wqq5koirzykr...@mail.gmail.com/
[3] https://gitlab.com/pclouds/git/tree/switch-and-restore

Nguyễn Thái Ngọc Duy (11):
  checkout: split part of it to new command 'restore'
  restore: take tree-ish from --source option instead
  restore: make pathspec mandatory
  restore: disable overlay mode by default
  checkout: factor out worktree checkout code
  restore: add --worktree and --index
  restore: default to --source=HEAD when only --index is specified
  restore: support --patch
  t: add tests for restore
  completion: support restore
  doc: promote "git restore"

 .gitignore |   1 +
 Documentation/config/interactive.txt   |   3 +-
 Documentation/git-checkout.txt |   1 +
 Documentation/git-clean.txt|   2 +-
 Documentation/git-commit.txt   |   4 +-
 Documentation/git-format-patch.txt |   2 +-
 Documentation/git-reset.txt|   6 +-
 Documentation/git-restore.txt (new)| 172 
 Documentation/git-revert.txt   |   4 +-
 Documentation/gitcli.txt   |   4 +-
 Documentation/giteveryday.txt  |   4 +-
 Documentation/gittutorial-2.txt|   4 +-
 Documentation/gittutorial.txt  |   2 +-
 Documentation/user-manual.txt  |  14 +-
 Makefile   |   1 +
 builtin.h  |   1 +
 builtin/checkout.c | 259 +++--
 builtin/clone.c|   2 +-
 builtin/commit.c   |   2 +-
 command-list.txt   |   3 +-
 contrib/completion/git-completion.bash |  15 ++
 git-add--interactive.perl  |  52 +
 git.c  |   1 +
 t/lib-patch-mode.sh|  12 ++
 t/t2070-restore.sh (new +x)|  77 
 t/t2071-restore-patch.sh (new +x)  | 105 ++
 t/t7508-status.sh  |  82 
 t/t7512-status-help.sh |  20 +-
 wt-status.c|  26 ++-
 29 files changed, 736 insertions(+), 145 deletions(-)
 create mode 100644 Documentation/git-restore.txt
 create mode 100755 t/t2070-restore.sh
 create mode 100755 t/t2071-restore-patch.sh

-- 
2.21.0.rc1.337.gdf7f8d0522



[PATCH v1 01/11] checkout: split part of it to new command 'restore'

2019-03-08 Thread Nguyễn Thái Ngọc Duy
Previously the switching branch business of 'git checkout' becomes a
new command. This adds restore command for the checking out
paths path.

Similar to switch, a new man page is added to describe what the
command will become. The implementation will be updated shortly to
match the man page.

A couple of differences from 'git checkout' worth highlighting:

- 'restore' by default will only update worktree. This matters more
  when --source is specified ('checkout  ' updates both
  worktree and index).

- 'restore --index' can be used to restore the index. This command
  overlaps with 'git reset '.

- both worktree and index could also be restored at the same time
  (from a tree). This overlaps with 'git checkout  '

- default source for restoring worktree and index is the index and
  HEAD respectively. A different (tree) source could be specified as
  with --source (*).

- when both index and worktree are restored, --source must be
  specified since the default source for these two individual targets
  are different (**)

- --no-overlay is enabled by default, if an entry is missing in the
  source, restoring means deleting the entry

(*) I originally went with --from instead of --source. I still think
  --from is a better name. The short option -f however is already
  taken by force. And I do think short option is good to have, e.g. to
  write -s@ or -s@^ instead of --source=HEAD.

(**) If you sit down and think about it, moving worktree's source from
  the index to HEAD makes sense, but nobody is really thinking it
  through when they type the commands.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 .gitignore   |   1 +
 Documentation/config/interactive.txt |   3 +-
 Documentation/git-checkout.txt   |   1 +
 Documentation/git-restore.txt (new)  | 172 +++
 Makefile |   1 +
 builtin.h|   1 +
 builtin/checkout.c   |  26 
 command-list.txt |   1 +
 git.c|   1 +
 9 files changed, 206 insertions(+), 1 deletion(-)

diff --git a/.gitignore b/.gitignore
index c687b92b1c..fb377106be 100644
--- a/.gitignore
+++ b/.gitignore
@@ -143,6 +143,7 @@
 /git-request-pull
 /git-rerere
 /git-reset
+/git-restore
 /git-rev-list
 /git-rev-parse
 /git-revert
diff --git a/Documentation/config/interactive.txt 
b/Documentation/config/interactive.txt
index ad846dd7c9..a2d3c7ec44 100644
--- a/Documentation/config/interactive.txt
+++ b/Documentation/config/interactive.txt
@@ -2,7 +2,8 @@ interactive.singleKey::
In interactive commands, allow the user to provide one-letter
input with a single key (i.e., without hitting enter).
Currently this is used by the `--patch` mode of
-   linkgit:git-add[1], linkgit:git-checkout[1], linkgit:git-commit[1],
+   linkgit:git-add[1], linkgit:git-checkout[1],
+   linkgit:git-restore[1], linkgit:git-commit[1],
linkgit:git-reset[1], and linkgit:git-stash[1]. Note that this
setting is silently ignored if portable keystroke input
is not available; requires the Perl module Term::ReadKey.
diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 2b776c1269..e107099c8c 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -571,6 +571,7 @@ $ git add frotz
 SEE ALSO
 
 linkgit:git-switch[1]
+linkgit:git-restore[1]
 
 GIT
 ---
diff --git a/Documentation/git-restore.txt b/Documentation/git-restore.txt
new file mode 100644
index 00..a667a5ced4
--- /dev/null
+++ b/Documentation/git-restore.txt
@@ -0,0 +1,172 @@
+git-restore(1)
+==
+
+NAME
+
+git-restore - Restore working tree files
+
+SYNOPSIS
+
+[verse]
+'git restore' [] [--source=] ...
+'git restore' (-p|--patch) [--source=] [...]
+
+DESCRIPTION
+---
+Restore paths in the working tree by replacing with the contents in
+the restore source or remove if the paths do not exist in the restore
+source. The source is by default the index but could be from a commit.
+The command can also optionally restore content in the index from a
+commit.
+
+When a `` is given, the paths that match the `` are
+updated both in the index and in the working tree.
+
+OPTIONS
+---
+-s::
+--source=::
+   Restore the working tree files with the content from the given
+   tree or any revision that leads to a tree (e.g. a commit or a
+   branch).
+
+-p::
+--patch::
+   Interactively select hunks in the difference between the
+   `` (or the index, if unspecified) and the working
+   tree. See the ``Interactive Mode'' section of linkgit:git-add[1]
+   to learn how to operate the `--patch` mode.
+
+-W::
+--worktree::
+-I::
+--index::
+   Specify the restore location. If neither option is specified,
+   by default the working tree is restored. If `--index` is
+   specified without `--worktree` or `--source`, `--source=HEAD`

[PATCH v1 03/11] restore: make pathspec mandatory

2019-03-08 Thread Nguyễn Thái Ngọc Duy
"git restore" without arguments does not make much sense when
it's about restoring files (what files now?). We could default to
either

git restore .

or

git restore :/

Neither is intuitive. Make the user always give pathspec, force the
user to think the scope of restore they want because this is a
destructive operation.

"git restore -p" without pathspec is an exception to this
because it really is a separate mode. It will be treated as running
patch mode on the whole worktree.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/checkout.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 838343d6aa..c52ce13d2a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -61,6 +61,7 @@ struct checkout_opts {
int accept_pathspec;
int switch_branch_doing_nothing_is_ok;
int only_merge_on_switching_branches;
+   int empty_pathspec_ok;
 
const char *new_branch;
const char *new_branch_force;
@@ -1427,6 +1428,10 @@ static int checkout_main(int argc, const char **argv, 
const char *prefix,
die(_("reference is not a tree: %s"), 
opts->from_treeish);
}
 
+   if (opts->accept_pathspec && !opts->empty_pathspec_ok && !argc &&
+   !opts->patch_mode)  /* patch mode is special */
+   die(_("pathspec is required"));
+
if (argc) {
parse_pathspec(&opts->pathspec, 0,
   opts->patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0,
@@ -1511,6 +1516,7 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
opts.accept_ref = 1;
opts.accept_pathspec = 1;
opts.implicit_detach = 1;
+   opts.empty_pathspec_ok = 1;
 
options = parse_options_dup(checkout_options);
options = add_common_options(&opts, options);
@@ -1570,6 +1576,7 @@ int cmd_restore(int argc, const char **argv, const char 
*prefix)
memset(&opts, 0, sizeof(opts));
opts.accept_ref = 0;
opts.accept_pathspec = 1;
+   opts.empty_pathspec_ok = 0;
 
options = parse_options_dup(restore_options);
options = add_common_options(&opts, options);
-- 
2.21.0.rc1.337.gdf7f8d0522



[PATCH v1 10/11] completion: support restore

2019-03-08 Thread Nguyễn Thái Ngọc Duy
Completion for restore is straightforward. We could still do better
though by give the list of just tracked files instead of all present
ones. But let's leave it for later.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 7fcf28d437..0b22e68187 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2483,6 +2483,21 @@ _git_reset ()
__git_complete_refs
 }
 
+_git_restore ()
+{
+   case "$cur" in
+   --conflict=*)
+   __gitcomp "diff3 merge" "" "${cur##--conflict=}"
+   ;;
+   --source=*)
+   __git_complete_refs --cur="${cur##--source=}"
+   ;;
+   --*)
+   __gitcomp_builtin restore
+   ;;
+   esac
+}
+
 __git_revert_inprogress_options="--continue --quit --abort"
 
 _git_revert ()
-- 
2.21.0.rc1.337.gdf7f8d0522



[PATCH v1 04/11] restore: disable overlay mode by default

2019-03-08 Thread Nguyễn Thái Ngọc Duy
Overlay mode is considered confusing when the command is about
restoring files on worktree. Disable it by default. The user can still
turn it on, or use 'git checkout' which still has overlay mode on by
default.

While at there make the check in checkout_branch() stricter. Neither
--overlay or --no-overlay should be accepted in branch switching mode.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/checkout.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index c52ce13d2a..9e59bf792f 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1196,9 +1196,9 @@ static int checkout_branch(struct checkout_opts *opts,
die(_("'%s' cannot be used with switching branches"),
"--patch");
 
-   if (!opts->overlay_mode)
+   if (opts->overlay_mode != -1)
die(_("'%s' cannot be used with switching branches"),
-   "--no-overlay");
+   "--[no]-overlay");
 
if (opts->writeout_stage)
die(_("'%s' cannot be used with switching branches"),
@@ -1313,7 +1313,6 @@ static struct option *add_checkout_path_options(struct 
checkout_opts *opts,
OPT_BOOL('p', "patch", &opts->patch_mode, N_("select hunks 
interactively")),
OPT_BOOL(0, "ignore-skip-worktree-bits", 
&opts->ignore_skipworktree,
 N_("do not limit pathspecs to sparse entries only")),
-   OPT_BOOL(0, "overlay", &opts->overlay_mode, N_("use overlay 
mode (default)")),
OPT_END()
};
struct option *newopts = parse_options_concat(prevopts, options);
@@ -1333,7 +1332,6 @@ static int checkout_main(int argc, const char **argv, 
const char *prefix,
opts->overwrite_ignore = 1;
opts->prefix = prefix;
opts->show_progress = -1;
-   opts->overlay_mode = -1;
 
git_config(git_checkout_config, opts);
 
@@ -1505,6 +1503,7 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
OPT_BOOL('l', NULL, &opts.new_branch_log, N_("create reflog for 
new branch")),
OPT_BOOL(0, "guess", &opts.dwim_new_local_branch,
 N_("second guess 'git checkout ' 
(default)")),
+   OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay mode 
(default)")),
OPT_END()
};
int ret;
@@ -1517,6 +1516,7 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
opts.accept_pathspec = 1;
opts.implicit_detach = 1;
opts.empty_pathspec_ok = 1;
+   opts.overlay_mode = -1;
 
options = parse_options_dup(checkout_options);
options = add_common_options(&opts, options);
@@ -1551,6 +1551,7 @@ int cmd_switch(int argc, const char **argv, const char 
*prefix)
opts.switch_branch_doing_nothing_is_ok = 0;
opts.only_merge_on_switching_branches = 1;
opts.implicit_detach = 0;
+   opts.overlay_mode = -1;
 
options = parse_options_dup(switch_options);
options = add_common_options(&opts, options);
@@ -1569,6 +1570,7 @@ int cmd_restore(int argc, const char **argv, const char 
*prefix)
struct option restore_options[] = {
OPT_STRING('s', "source", &opts.from_treeish, "",
   N_("where the checkout from")),
+   OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay 
mode")),
OPT_END()
};
int ret;
@@ -1577,6 +1579,7 @@ int cmd_restore(int argc, const char **argv, const char 
*prefix)
opts.accept_ref = 0;
opts.accept_pathspec = 1;
opts.empty_pathspec_ok = 0;
+   opts.overlay_mode = 0;
 
options = parse_options_dup(restore_options);
options = add_common_options(&opts, options);
-- 
2.21.0.rc1.337.gdf7f8d0522



[PATCH v1 02/11] restore: take tree-ish from --source option instead

2019-03-08 Thread Nguyễn Thái Ngọc Duy
This is another departure from 'git checkout' syntax, which uses -- to
separate ref and pathspec. The observation is restore (or "git
checkout ,, ") is most often used to restore some files from
the index. If this is correct, we can simplify it by taking a way the
ref, so that we can write

git restore some-file

without worrying about some-file being a ref and whether we need to do

git restore -- some-file

for safety. If the source of the restore comes from a tree, it will be
in the form of an option with value, e.g.

git restore --source=this-tree some-file

This is of course longer to type than using "--". But hopefully it
will not be used as often, and it is clearly easier to understand.

dwim_new_local_branch is no longer set (or unset) in cmd_restore_files()
because it's irrelevant because we don't really care about dwim-ing.
With accept_ref being unset, dwim can't happen.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/checkout.c | 42 ++
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 11dd2ae44c..838343d6aa 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -38,7 +38,7 @@ static const char * const switch_branch_usage[] = {
 };
 
 static const char * const restore_files_usage[] = {
-   N_("git restore [] [] -- ..."),
+   N_("git restore [] [--source=] ..."),
NULL,
 };
 
@@ -57,6 +57,7 @@ struct checkout_opts {
int count_checkout_paths;
int overlay_mode;
int dwim_new_local_branch;
+   int accept_ref;
int accept_pathspec;
int switch_branch_doing_nothing_is_ok;
int only_merge_on_switching_branches;
@@ -72,6 +73,7 @@ struct checkout_opts {
int branch_exists;
const char *prefix;
struct pathspec pathspec;
+   const char *from_treeish;
struct tree *source_tree;
 };
 
@@ -1324,6 +1326,7 @@ static int checkout_main(int argc, const char **argv, 
const char *prefix,
 {
struct branch_info new_branch_info;
int dwim_remotes_matched = 0;
+   int parseopt_flags = 0;
 
memset(&new_branch_info, 0, sizeof(new_branch_info));
opts->overwrite_ignore = 1;
@@ -1335,8 +1338,13 @@ static int checkout_main(int argc, const char **argv, 
const char *prefix,
 
opts->track = BRANCH_TRACK_UNSPECIFIED;
 
-   argc = parse_options(argc, argv, prefix, options, usagestr,
-PARSE_OPT_KEEP_DASHDASH);
+   if (!opts->accept_pathspec && !opts->accept_ref)
+   BUG("make up your mind, you need to take _something_");
+   if (opts->accept_pathspec && opts->accept_ref)
+   parseopt_flags = PARSE_OPT_KEEP_DASHDASH;
+
+   argc = parse_options(argc, argv, prefix, options,
+usagestr, parseopt_flags);
 
if (opts->show_progress < 0) {
if (opts->quiet)
@@ -1393,7 +1401,7 @@ static int checkout_main(int argc, const char **argv, 
const char *prefix,
 * including "last branch" syntax and DWIM-ery for names of
 * remote branches, erroring out for invalid or ambiguous cases.
 */
-   if (argc) {
+   if (argc && opts->accept_ref) {
struct object_id rev;
int dwim_ok =
!opts->patch_mode &&
@@ -1405,6 +1413,18 @@ static int checkout_main(int argc, const char **argv, 
const char *prefix,
 &dwim_remotes_matched);
argv += n;
argc -= n;
+   } else if (!opts->accept_ref && opts->from_treeish) {
+   struct object_id rev;
+
+   if (get_oid_mb(opts->from_treeish, &rev))
+   die(_("could not resolve %s"), opts->from_treeish);
+
+   setup_new_branch_info_and_source_tree(&new_branch_info,
+ opts, &rev,
+ opts->from_treeish);
+
+   if (!opts->source_tree)
+   die(_("reference is not a tree: %s"), 
opts->from_treeish);
}
 
if (argc) {
@@ -1488,6 +1508,7 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
opts.dwim_new_local_branch = 1;
opts.switch_branch_doing_nothing_is_ok = 1;
opts.only_merge_on_switching_branches = 0;
+   opts.accept_ref = 1;
opts.accept_pathspec = 1;
opts.implicit_detach = 1;
 
@@ -1519,6 +1540,7 @@ int cmd_switch(int argc, const char **argv, const char 
*prefix)
 
memset(&opts, 0, sizeof(opts));
opts.dwim_new_local_branch = 0;
+   opts.accept_ref = 1;
opts.accept_pathspec = 0;
opts.switch_branch_doing_nothing_is_ok = 0;
opts.only_merge_on_switching_branches = 1;
@@ -1537,15 +1559,19 @@ int cmd_switch(int argc, const char **argv, const char 
*prefix)
 int cmd_restore(int argc, const char **argv,

[PATCH v1 11/11] doc: promote "git restore"

2019-03-08 Thread Nguyễn Thái Ngọc Duy
The new command "git restore" (together with "git switch") are added
to avoid the confusion of one-command-do-all "git checkout" for new
users. They are also helpful to avoid ambiguation context.

For these reasons, promote it everywhere possible. This includes
documentation, suggestions/advice from other commands.

One nice thing about git-restore is the ability to restore
"everything", so it can be used in "git status" advice instead of both
"git checkout" and "git reset".  The three commands suggested by "git
status" are add, rm and restore.

"git checkout" is also removed from "git help" (i.e. it's no longer
considered a commonly used command)

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git-clean.txt|  2 +-
 Documentation/git-commit.txt   |  4 +-
 Documentation/git-format-patch.txt |  2 +-
 Documentation/git-reset.txt|  6 +--
 Documentation/git-revert.txt   |  4 +-
 Documentation/gitcli.txt   |  4 +-
 Documentation/giteveryday.txt  |  4 +-
 Documentation/gittutorial-2.txt|  4 +-
 Documentation/gittutorial.txt  |  2 +-
 Documentation/user-manual.txt  | 14 +++--
 builtin/clone.c|  2 +-
 builtin/commit.c   |  2 +-
 command-list.txt   |  2 +-
 t/t7508-status.sh  | 82 +++---
 t/t7512-status-help.sh | 20 
 wt-status.c| 26 +++---
 16 files changed, 95 insertions(+), 85 deletions(-)

diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
index 03056dad0d..8a31ccffea 100644
--- a/Documentation/git-clean.txt
+++ b/Documentation/git-clean.txt
@@ -64,7 +64,7 @@ OPTIONS
directory) and $GIT_DIR/info/exclude, but do still use the ignore
rules given with `-e` options.  This allows removing all untracked
files, including build products.  This can be used (possibly in
-   conjunction with 'git reset') to create a pristine
+   conjunction with 'git restore' or 'git reset') to create a pristine
working directory to test a clean build.
 
 -X::
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index a85c2c2a4c..e7ac8eb9e8 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -56,7 +56,7 @@ summary of what is included by any of the above for the next
 commit by giving the same set of parameters (options and paths).
 
 If you make a commit and then find a mistake immediately after
-that, you can recover from it with 'git reset'.
+that, you can recover from it with 'git restore' or 'git reset'.
 
 
 OPTIONS
@@ -359,7 +359,7 @@ When recording your own work, the contents of modified 
files in
 your working tree are temporarily stored to a staging area
 called the "index" with 'git add'.  A file can be
 reverted back, only in the index but not in the working tree,
-to that of the last commit with `git reset HEAD -- `,
+to that of the last commit with `git restore --index `,
 which effectively reverts 'git add' and prevents the changes to
 this file from participating in the next commit.  After building
 the state to be committed incrementally with these commands,
diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index 0a24a5679e..71c9fe3af3 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -422,7 +422,7 @@ One way to test if your MUA is set up correctly is:
 
 $ git fetch  master:test-apply
 $ git switch test-apply
-$ git reset --hard
+$ git restore --source=HEAD --index --worktree :/
 $ git am a.patch
 
 If it does not apply correctly, there can be various reasons.
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index cbf901efb4..b70281677f 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -28,9 +28,9 @@ This means that `git reset ` is the opposite of `git 
add
 `.
 +
 After running `git reset ` to update the index entry, you can
-use linkgit:git-checkout[1] to check the contents out of the index to
-the working tree.
-Alternatively, using linkgit:git-checkout[1] and specifying a commit, you
+use linkgit:git-restore[1] to check the contents out of the index to
+the working tree. Alternatively, using linkgit:git-restore[1]
+and specifying a commit with `--source`, you
 can copy the contents of a path out of a commit to the index and to the
 working tree in one go.
 
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index 837707a8fd..c38bc54439 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -26,8 +26,8 @@ effect of some earlier commits (often only a faulty one).  If 
you want to
 throw away all uncommitted changes in your working directory, you
 should see linkgit:git-reset[1], particularly the `--hard` option.  If
 you want to extract specific files as they were in another commit, you
-should see linkgit:git-checkout[1], specifi

One failed self test on Fedora 29

2019-03-08 Thread Jeffrey Walton
Fedora 29, x86_64. One failed self test:

*** t0021-conversion.sh ***
ok 1 - setup
ok 2 - check
ok 3 - expanded_in_repo
ok 4 - filter shell-escaped filenames
ok 5 - required filter should filter data
ok 6 - required filter smudge failure
ok 7 - required filter clean failure
ok 8 - filtering large input to small output should use little memory
ok 9 - filter that does not read is fine
ok 10 # skip filter large file (missing EXPENSIVE)
ok 11 - filter: clean empty file
ok 12 - filter: smudge empty file
not ok 13 - disable filter with empty override
#
#   test_config_global filter.disable.smudge false &&
#   test_config_global filter.disable.clean false &&
#   test_config filter.disable.smudge false &&
#   test_config filter.disable.clean false &&
#
#   echo "*.disable filter=disable" >.gitattributes &&
#
#   echo test >test.disable &&
#   git -c filter.disable.clean= add test.disable 2>err &&
#   test_must_be_empty err &&
#   rm -f test.disable &&
#   git -c filter.disable.smudge= checkout -- test.disable 2>err &&
#   test_must_be_empty err
#
ok 14 - diff does not reuse worktree files that need cleaning
ok 15 - required process filter should filter data
ok 16 - required process filter takes precedence
ok 17 - required process filter should be used only for "clean" operation only
ok 18 - required process filter should process multiple packets
ok 19 - required process filter with clean error should fail
ok 20 - process filter should restart after unexpected write failure
ok 21 - process filter should not be restarted if it signals an error
ok 22 - process filter abort stops processing of all further files
ok 23 - invalid process filter must fail (and not hang!)
ok 24 - delayed checkout in process filter
ok 25 - missing file in delayed checkout
ok 26 - invalid file in delayed checkout
# failed 1 among 26 test(s)
1..26
gmake[2]: *** [Makefile:56: t0021-conversion.sh] Error 1

Does anyone need a config.log or other test data?


fast-import on existing branches

2019-03-08 Thread Norbert Nemec
Hi there,

I've struggled for quite some time to sort out documented, intended and actual 
behavior of git fast-import. Unless I'm completely mistaken, it seems to be a 
straightforward bug, but if that is the case, I am really surprised why nobody 
else has stumbled over it before:

I managed to use fast-import for a chain of commits onto a new branch into an 
empty repository.
I managed to use fast-import to create a new branch starting from an existing 
parent using the 'from' command as documented.

What I failed to do is to add commits on top of an existing branch in a new 
fast-import stream. As it seems, the variant of using 'commit' without 'from' 
only works on branches that were created within the same fast-import stream!

The different approaches I tried (each with new fast-import stream on existing 
repo with existing branch)
* 'commit' without 'from'
-> Error: "Not updating  (new tip  does not contain )
And indeed looking into the repo afterwards, a new commit exists without any 
parent.
* 'commit' with 'from' both naming the same branch
-> Error: "Can't create a branch from itself"
The only workarounds that I could find are to either explicitly looking up the 
top commit on the target branch and hand that to fast-import or create a 
temporary branch with a different name.

Looking through the code of fast-import.c, I can indeed lookup_branch and 
new_branch only deal with internal data structures and the only point were 
read_ref is called to actually read existing branches from the repo is in 
update_branch to check whether the parent was set correctly. What is missing is 
a call to read_ref in either lookup_branch or new_branch (probably both have to 
be reworked in some way to handle this cleanly). From all I can see a fix 
should be fairly straightforward to implement, but I am really not sure whether 
I have the full picture on this.

(I found all of this struggling with git-p4.py which appears to contains a 
complex and not fully correct mechanism to determine the 'initalParent' that 
appears to implement just such a workaround.)

I would be grateful for any input on this issue! Greetings,
Norbert



[PATCH 1/2] modified dir-iterator.c

2019-03-08 Thread sushmaunnibhavi
---
Some places in git use raw API opendir/readdir/closedir to traverse a directory 
recursively, which usually involves function recursion. Now that we have struct 
dir_iterator,we have to convert these to use the dir-iterator to simplify the 
code. 

Signed-off-by: Sushma Unnibhavi 
 dir-iterator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dir-iterator.c b/dir-iterator.c
index f2dcd82fde..a3b5b8929c 100644
--- a/dir-iterator.c
+++ b/dir-iterator.c
@@ -169,7 +169,7 @@ int dir_iterator_abort(struct dir_iterator *dir_iterator)
struct dir_iterator_level *level =
&iter->levels[iter->levels_nr - 1];
 
-   if (level->dir && closedir(level->dir)) {
+   if (level->dir && (struct dir_iterator_int *)(level->dir)) 
{//changed closedir to (struct dir_iterator_int *)
strbuf_setlen(&iter->base.path, level->prefix_len);
warning("error closing directory %s: %s",
iter->base.path.buf, strerror(errno));
-- 
2.17.1



[GSOC][PATCH 1/2] modified dir-iterator.c

2019-03-08 Thread sushmaunnibhavi
---
Some places in git use raw API opendir/readdir/closedir to traverse a directory 
recursively, which usually involves function recursion. Now that we have struct 
dir_iterator,we have to convert these to use the dir-iterator to simplify the 
code. 

Signed-off-by: Sushma Unnibhavi 
 dir-iterator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dir-iterator.c b/dir-iterator.c
index f2dcd82fde..a3b5b8929c 100644
--- a/dir-iterator.c
+++ b/dir-iterator.c
@@ -169,7 +169,7 @@ int dir_iterator_abort(struct dir_iterator *dir_iterator)
struct dir_iterator_level *level =
&iter->levels[iter->levels_nr - 1];
 
-   if (level->dir && closedir(level->dir)) {
+   if (level->dir && (struct dir_iterator_int *)(level->dir)) 
{//changed closedir to (struct dir_iterator_int *)
strbuf_setlen(&iter->base.path, level->prefix_len);
warning("error closing directory %s: %s",
iter->base.path.buf, strerror(errno));
-- 
2.17.1



Re: [PATCH v13 18/27] stash: convert create to builtin

2019-03-08 Thread Johannes Schindelin
Hi Peff,

On Thu, 7 Mar 2019, Jeff King wrote:

> On Mon, Feb 25, 2019 at 11:16:22PM +, Thomas Gummerer wrote:
> 
> > +static void add_pathspecs(struct argv_array *args,
> > + struct pathspec ps) {
> 
> Here and elsewhere in the series, I notice that we pass the pathspec
> struct by value, which is quite unusual for our codebase (and
> potentially confusing, if any of the callers were to mutate the pointers
> in the struct).
> 
> Is there any reason this shouldn't be "const struct pathspec *ps" pretty
> much throughout the file?

I am quite certain that this is merely an oversight. It totes slipped
by my review, for example.

Thanks for catching!
Dscho


how can i "gc" or "prune" objects related to a deleted remote?

2019-03-08 Thread Robert P. J. Day


  writing a short tutorial on how to add a remote and work with it
and, as a final step, show how, if one is uninterested in the remote
after all, one can simply delete it, but i also want to show how one
can then prune or garbage collect the objects related to that remote,
but i can't figure out how.

  as an example, i cloned the linux kernel source tree, then added
the linux-next repo as a remote -- the end result was two pack files
(the smaller one i'm assuming being for linux-next):

$ ls -l .git/objects/pack
total 2723632
-r--r--r--. 1 rpjday rpjday1215376 Mar  8 09:44 
pack-08cc266c0914e924961a1c8412fdf8746d327d7e.idx
-r--r--r--. 1 rpjday rpjday   38402840 Mar  8 09:44 
pack-08cc266c0914e924961a1c8412fdf8746d327d7e.pack
-r--r--r--. 1 rpjday rpjday  204032716 Mar  8 09:42 
pack-1036510bb74967c91093fc0cd8982683a66dbf5f.idx
-r--r--r--. 1 rpjday rpjday 254527 Mar  8 09:42 
pack-1036510bb74967c91093fc0cd8982683a66dbf5f.pac
$

  after playing with a couple branches from the new remote, i deleted
the remote, then also did things like clear the reflog, delete any
local tracking branches related to the deleted remote, and so on, but
i can't seem to prune the objects that should no longer be reachable
now that i deleted that remote.

  what am i overlooking? is it because those objects are in a separate
pack file? do i need to repack first? what is hanging onto those
objects in that second pack file such that they can't be garbage
collected?

rday

-- 


Robert P. J. Day Ottawa, Ontario, CANADA
  http://crashcourse.ca/dokuwiki

Twitter:   http://twitter.com/rpjday
LinkedIn:   http://ca.linkedin.com/in/rpjday



Re: [PATCH v2] line-log: suppress diff output with "-s"

2019-03-08 Thread Johannes Schindelin
Hi Peff,

On Thu, 7 Mar 2019, Jeff King wrote:

> When "-L" is in use, we ignore any diff output format that the user
> provides to us, and just always print a patch (with extra context lines
> covering the whole area of interest). It's not entirely clear what we
> should do with all formats (e.g., should "--stat" show just the diffstat
> of the touched lines, or the stat for the whole file?).
> 
> But "-s" is pretty clear: the user probably wants to see just the
> commits that touched those lines, without any diff at all. Let's at
> least make that work.

Agree. The patch looks obviously good.

Thank you,
Dscho

> 
> Signed-off-by: Jeff King 
> ---
> This is a repost from the thread at:
> 
>   
> https://public-inbox.org/git/caekqehdfu5zm4ay3ihn0pn1acneomy0wv07pryfab45jn-t...@mail.gmail.com/
> 
>  line-log.c  | 6 --
>  t/t4211-line-log.sh | 7 +++
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/line-log.c b/line-log.c
> index 24e21731c4..59248e37cc 100644
> --- a/line-log.c
> +++ b/line-log.c
> @@ -1103,10 +1103,12 @@ static int process_all_files(struct line_log_data 
> **range_out,
>  
>  int line_log_print(struct rev_info *rev, struct commit *commit)
>  {
> - struct line_log_data *range = lookup_line_range(rev, commit);
>  
>   show_log(rev);
> - dump_diff_hacky(rev, range);
> + if (!(rev->diffopt.output_format & DIFF_FORMAT_NO_OUTPUT)) {
> + struct line_log_data *range = lookup_line_range(rev, commit);
> + dump_diff_hacky(rev, range);
> + }
>   return 1;
>  }
>  
> diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
> index bd5fe4d148..c9f2036f68 100755
> --- a/t/t4211-line-log.sh
> +++ b/t/t4211-line-log.sh
> @@ -115,4 +115,11 @@ test_expect_success 'range_set_union' '
>   git log $(for x in $(test_seq 200); do echo -L $((2*x)),+1:c.c; done)
>  '
>  
> +test_expect_success '-s shows only line-log commits' '
> + git log --format="commit %s" -L1,24:b.c >expect.raw &&
> + grep ^commit expect.raw >expect &&
> + git log --format="commit %s" -L1,24:b.c -s >actual &&
> + test_cmp expect actual
> +'
> +
>  test_done
> -- 
> 2.21.0.787.g929e938557
> 


Re: fast-import on existing branches

2019-03-08 Thread Elijah Newren
Hi Norbert,

On Fri, Mar 8, 2019 at 2:51 AM Norbert Nemec
 wrote:
>
> Hi there,
>
> I've struggled for quite some time to sort out documented, intended and 
> actual behavior of git fast-import. Unless I'm completely mistaken, it seems 
> to be a straightforward bug, but if that is the case, I am really surprised 
> why nobody else has stumbled over it before:
>
> I managed to use fast-import for a chain of commits onto a new branch into an 
> empty repository.
> I managed to use fast-import to create a new branch starting from an existing 
> parent using the 'from' command as documented.
>
> What I failed to do is to add commits on top of an existing branch in a new 
> fast-import stream. As it seems, the variant of using 'commit' without 'from' 
> only works on branches that were created within the same fast-import stream!
>
> The different approaches I tried (each with new fast-import stream on 
> existing repo with existing branch)
> * 'commit' without 'from'
> -> Error: "Not updating  (new tip  does not contain )
> And indeed looking into the repo afterwards, a new commit exists without any 
> parent.
> * 'commit' with 'from' both naming the same branch
> -> Error: "Can't create a branch from itself"
> The only workarounds that I could find are to either explicitly looking up 
> the top commit on the target branch and hand that to fast-import or create a 
> temporary branch with a different name.

I would have just used "from " where  is something I look
up from the current branch I want to update.  But, re-looking at the
docs, it appears git-fast-import.txt covers this already with a
possibly easier syntax:

"""
The special case of restarting an incremental import from the
current branch value should be written as:

from refs/heads/branch^0

The `^0` suffix is necessary as fast-import does not permit a branch to
start from itself, and the branch is created in memory before the
`from` command is even read from the input.  Adding `^0` will force
fast-import to resolve the commit through Git's revision parsing library,
rather than its internal branch table, thereby loading in the
existing value of the branch.
"""

Perhaps try that?

> Looking through the code of fast-import.c, I can indeed lookup_branch and 
> new_branch only deal with internal data structures and the only point were 
> read_ref is called to actually read existing branches from the repo is in 
> update_branch to check whether the parent was set correctly. What is missing 
> is a call to read_ref in either lookup_branch or new_branch (probably both 
> have to be reworked in some way to handle this cleanly). From all I can see a 
> fix should be fairly straightforward to implement, but I am really not sure 
> whether I have the full picture on this.
>
> (I found all of this struggling with git-p4.py which appears to contains a 
> complex and not fully correct mechanism to determine the 'initalParent' that 
> appears to implement just such a workaround.)
>
> I would be grateful for any input on this issue! Greetings,
> Norbert

Hope that helps,
Elijah


Re: git reset error on Windows

2019-03-08 Thread Johannes Schindelin
Hi Adrian,

On Thu, 7 Mar 2019, Adrian Godong wrote:

> Windows 10, git version 2.21.0.windows.1
> 
> git reset tries to delete folder when last file is removed but failed
> to do so if shell is in the deleted folder.
> 
> Repro steps (powershell):
> mkdir test
> cd test
> git init
> mkdir dir
> cd dir
> add-content .\file ""
> git add .
> git commit -m "1"
> git mv .\file .\file2
> git commit -m "2"
> git reset --hard HEAD^
> > Deletion of directory 'dir' failed. Should I try again? (y/n)
> 
> Choosing y will repeat the same prompt. Choosing n will complete the
> operation correctly.

In contrast to Linux, it is not possible to delete a current working
directory (unless you delve into horrible hacks like Cygwin does).

So this is a restriction of the platform on which you are working, and
there is nothing we can do about it, at least as far as I can think of.

Ciao,
Johannes


New Ft. for Git : Allow resumable cloning of repositories.

2019-03-08 Thread Kapil Jain
Objective: Allow pause and resume functionality while cloning repositories.

Below is a rough idea on how this may be achieved.

1) Create a repository_name.json file.
2) repository_name.json will be an index file containing list of all
the files in the repository with default status being "False".
   "False" status of a file signifies that this file is not yet fully
downloaded.

Something like this:

{
  'file1.ext' : "False",
  'file2.ext' : "False",
  'file3.ext' : "False"
}

3) As a file finishes downloading, say 'file1.ext' and 'file2.ext'
have finished downloading, their status will change to:

Something like this:

{
  'file1.ext' : "True",
  'file2.ext' : "True",
  'file3.ext' : "False"
}

4) Suppose due to some reason, before 'file3.ext' could finish
download; cloning is interrupted.
5) After the interruption the repository_name.json and downloaded
files are preserved.
6) Now, when cloning of the same repository begins next time, files
would be downloaded based on information taken from
repository_name.json file.

Note 1: Doing this for cloning would be the main objective, further
this may be extended for fetching, pulling, and pushing too.

Note 2: Since this is gsoc time, please don't take this to be a
project idea for gsoc, as it was pointed out on irc that this would be
a time intensive functionality.

I want to work on building this functionality.
Please discuss thoughts on this, so as to make a technically sound to-do list.


[PATCH 1/1] mingw: allow building with an MSYS2 runtime v3.x

2019-03-08 Thread Johannes Schindelin via GitGitGadget
From: Johannes Schindelin 

Recently the Git for Windows project started the upgrade process to
a MSYS2 runtime version based on Cygwin v3.x.

This has the very notable consequence that `$(uname -r)` no longer
reports a version starting with "2", but a version with "3".

That breaks our build, as df5218b4c30b (config.mak.uname: support MSys2,
2016-01-13) simply did not expect the version reported by `uname -r` to
depend on the underlying Cygwin version: it expected the reported
version to match the "2" in "MSYS2".

So let's invert that test case to test for *anything else* than a
version starting with "1" (for MSys). That should safeguard us for the
future, even if Cygwin ends up releasing versionsl like 314.272.65536.

Signed-off-by: Johannes Schindelin 
---
 config.mak.uname | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/config.mak.uname b/config.mak.uname
index c8b0e34c31..0207e895a4 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -569,7 +569,7 @@ ifneq (,$(wildcard ../THIS_IS_MSYSGIT))
NO_GETTEXT = YesPlease
COMPAT_CLFAGS += -D__USE_MINGW_ACCESS
 else
-   ifeq ($(shell expr "$(uname_R)" : '2\.'),2)
+   ifneq ($(shell expr "$(uname_R)" : '1\.'),2)
# MSys2
prefix = /usr/
ifeq (MINGW32,$(MSYSTEM))
-- 
gitgitgadget


[PATCH 0/1] mingw: fix uname -r test

2019-03-08 Thread Johannes Schindelin via GitGitGadget
In df5218b4c30b (config.mak.uname: support MSys2, 2016-01-13), I obviously
made the assumption that calling uname -r in MSYS2 would always yield a
version number starting with "2".

That is incorrect, though, as uname -r reports the version of the Cygwin
runtime on which the current MSYS2 runtime is based.

This sadly breaks our build as soon as we upgrade to an MSYS2 runtime that
is based on Cygwin v3.0.2.

Happily, this patch fixes that.

Johannes Schindelin (1):
  mingw: allow building with an MSYS2 runtime v3.x

 config.mak.uname | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


base-commit: 6e0cc6776106079ed4efa0cc9abace4107657abf
Published-As: 
https://github.com/gitgitgadget/git/releases/tag/pr-160%2Fdscho%2Fmsys2-3.x-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
pr-160/dscho/msys2-3.x-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/160
-- 
gitgitgadget


Re: [PATCH 0/2] stash: handle pathspec magic again

2019-03-08 Thread Johannes Schindelin
Hi Junio,

On Fri, 8 Mar 2019, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" 
> writes:
> 
> > It was reported in https://github.com/git-for-windows/git/issues/2037 that 
> > git stash -- ':(glob)**/*.testextension is broken. The problem is not even
> > the stash operation itself, it happens when the git add part of dropping the
> > local changes is spawned: we simply passed the parsed pathspec instead of
> > the unparsed one.
> >
> > While verifying the fix, I also realized that the escape hatch was seriously
> > broken by my "backport of the -q option": It introduced four bogus eval 
> > statements, which really need to be dropped.
> 
> Thanks.
> 
> We seem to be piling many "oops" on top, even after the topic hits
> 'next'.  But fixes are better late than never ;-).

Yes, we do. At least now I do not have the impression that I have to
impose on Paul to integrate whatever diffs I came up with, I can now just
submit small patch series that are easily reviewed.

If you care deeply about the commit history, I hereby offer to you to
clean up the built-in stash patches when you say you're ready to advance
them to `master`; I will then squash the fixes into the proper places to
make it a nicer read, with the promise that the tree will be the same in
the end as with the oops-upon-oops patch history that we're piling up in
`next`. I would do that for you.

> Will queue.

Thanks,
Dscho


Re: [PATCH v2] line-log: suppress diff output with "-s"

2019-03-08 Thread Jeff King
On Fri, Mar 08, 2019 at 04:38:44PM +0100, Johannes Schindelin wrote:

> On Thu, 7 Mar 2019, Jeff King wrote:
> 
> > When "-L" is in use, we ignore any diff output format that the user
> > provides to us, and just always print a patch (with extra context lines
> > covering the whole area of interest). It's not entirely clear what we
> > should do with all formats (e.g., should "--stat" show just the diffstat
> > of the touched lines, or the stat for the whole file?).
> > 
> > But "-s" is pretty clear: the user probably wants to see just the
> > commits that touched those lines, without any diff at all. Let's at
> > least make that work.
> 
> Agree. The patch looks obviously good.

Thanks. This leaves the other formats as silently ignored. Do we want to
do something like this:

diff --git a/revision.c b/revision.c
index eb8e51bc63..a1b4fe2aa6 100644
--- a/revision.c
+++ b/revision.c
@@ -2689,6 +2689,10 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
if (revs->first_parent_only && revs->bisect)
die(_("--first-parent is incompatible with --bisect"));
 
+   if (revs->line_level_traverse &&
+   (revs->diffopt.output_format & 
~(DIFF_FORMAT_PATCH|DIFF_FORMAT_NO_OUTPUT)))
+   die(_("-L does not yet support diff formats besides -p and 
-s"));
+
if (revs->expand_tabs_in_log < 0)
revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
 

?

-Peff


Re: [PATCH 0/1] Drop last MakeMaker reference

2019-03-08 Thread Johannes Schindelin
Hi Junio,

On Fri, 8 Mar 2019, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > On Sun, 3 Mar 2019, Junio C Hamano wrote:
> >
> >> "Johannes Schindelin via GitGitGadget" 
> >> writes:
> >> 
> >> > Back when we stopped using MakeMaker, we forgot one reference...
> >> >
> >> > Johannes Schindelin (1):
> >> >   mingw: drop MakeMaker reference
> >> >
> >> >  config.mak.uname | 1 -
> >> >  1 file changed, 1 deletion(-)
> >> >
> >> >
> >> > base-commit: 8104ec994ea3849a968b4667d072fedd1e688642
> >> > Published-As: 
> >> > https://github.com/gitgitgadget/git/releases/tag/pr-146%2Fdscho%2Fno-perl-makemaker-v1
> >> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
> >> > pr-146/dscho/no-perl-makemaker-v1
> >> > Pull-Request: https://github.com/gitgitgadget/git/pull/146
> >> 
> >> Good ;-)
> >> Thanks.
> >
> > Gentle reminder that this has not made it into `pu` yet...
> 
> Thanks.
> 
> I'll try to make it a habit not to respond to 0/1 (but instead to
> 1/1) as it is quite inefficient to get to 1/1 from 0/1 at least for
> me X-<.

Hehe. I do have something for you there:

https://github.com/git-for-windows/build-extra/blob/master/apply-from-public-inbox.sh

-- snip --

> Or perhaps GGG can learn to avoid 0/1 for a single-patch topic ;-)

It is easier, and more consistent, to have a cover letter even then, for
things like the broader explanation of the context, the changes since the
previous iteration, and the range-diff (which would make v2, v3, v4, etc
utterly unreadable from my point of view if they were integrated into the
single patches, as I used to do with interdiffs).

> Thanks anyway.  Will try to apply directly on top of master.

Thank you!

While we're talking about "directly on top of master"... *after* it got
some review, I would love to ask for the same favor for
https://public-inbox.org/git/pull.160.git.gitgitgad...@gmail.com

On the other hand, it is an old bug, I know, and it will break all CI
builds for branches that are based off of older commits, anyway. I guess
we'll need some sort of horrible hack in the git-sdk-64-minimal thing, to
allow for patching this on the CI side, without adding commits to all of
those branches. :-(

So: I made up my mind, and that MSYS2 runtime v3.x patch does not need to
be fast-tracked; it would not fix all the CI builds...

Ciao,
Dscho


Re: [RFC PATCH 0/5] Fix some fast-import parsing issues

2019-03-08 Thread Elijah Newren
Hi,

On Wed, Feb 20, 2019 at 2:58 PM Elijah Newren  wrote:
>
> I found a few issues with parsing in fast-import (dating back to

> I've cc'ed the relevant folks, and have a few patches that fix the
> issue and I think make the parser more robust against future issues in
> a way that I think is safe enough for backward compatibility, but
> "backward compatible enough" might concern some folks; if so, please
> take a look at patches 4 and 5.

Just thought I'd ping to see if folks have any concerns with this
slight tweak to backward compatibility; if not, I'll just repost the
patches removing the RFC label.


RE: fast-import on existing branches

2019-03-08 Thread Norbert Nemec
Thanks, Elijah, I had indeed missed that block about the ^0 handling.

I still don't get why this awkward workaround is required. Why isn't that 
lookup done by default? Performance can't be the reason, since the same lookup 
is done lateron anyway, just as correctness check. The way I read the 
documentation, providing no "from" should continue committing to a branch in 
any case. I would never have seen the continuation of an incremental import a 
"special case". There is a number of tools around that sync a git repo from 
some other source and would regularly need to continue an existing branch.

Greetings,
Norbert


-Original Message-
From: Elijah Newren  
Sent: Friday, March 8, 2019 4:41 PM
To: Norbert Nemec 
Cc: git@vger.kernel.org
Subject: Re: fast-import on existing branches

Hi Norbert,

On Fri, Mar 8, 2019 at 2:51 AM Norbert Nemec  
wrote:
>
> Hi there,
>
> I've struggled for quite some time to sort out documented, intended and 
> actual behavior of git fast-import. Unless I'm completely mistaken, it seems 
> to be a straightforward bug, but if that is the case, I am really surprised 
> why nobody else has stumbled over it before:
>
> I managed to use fast-import for a chain of commits onto a new branch into an 
> empty repository.
> I managed to use fast-import to create a new branch starting from an existing 
> parent using the 'from' command as documented.
>
> What I failed to do is to add commits on top of an existing branch in a new 
> fast-import stream. As it seems, the variant of using 'commit' without 'from' 
> only works on branches that were created within the same fast-import stream!
>
> The different approaches I tried (each with new fast-import stream on 
> existing repo with existing branch)
> * 'commit' without 'from'
> -> Error: "Not updating  (new tip  does not contain 
> -> )
> And indeed looking into the repo afterwards, a new commit exists without any 
> parent.
> * 'commit' with 'from' both naming the same branch
> -> Error: "Can't create a branch from itself"
> The only workarounds that I could find are to either explicitly looking up 
> the top commit on the target branch and hand that to fast-import or create a 
> temporary branch with a different name.

I would have just used "from " where  is something I look up from 
the current branch I want to update.  But, re-looking at the docs, it appears 
git-fast-import.txt covers this already with a possibly easier syntax:

"""
The special case of restarting an incremental import from the current branch 
value should be written as:

from refs/heads/branch^0

The `^0` suffix is necessary as fast-import does not permit a branch to start 
from itself, and the branch is created in memory before the `from` command is 
even read from the input.  Adding `^0` will force fast-import to resolve the 
commit through Git's revision parsing library, rather than its internal branch 
table, thereby loading in the existing value of the branch.
"""

Perhaps try that?

> Looking through the code of fast-import.c, I can indeed lookup_branch and 
> new_branch only deal with internal data structures and the only point were 
> read_ref is called to actually read existing branches from the repo is in 
> update_branch to check whether the parent was set correctly. What is missing 
> is a call to read_ref in either lookup_branch or new_branch (probably both 
> have to be reworked in some way to handle this cleanly). From all I can see a 
> fix should be fairly straightforward to implement, but I am really not sure 
> whether I have the full picture on this.
>
> (I found all of this struggling with git-p4.py which appears to 
> contains a complex and not fully correct mechanism to determine the 
> 'initalParent' that appears to implement just such a workaround.)
>
> I would be grateful for any input on this issue! Greetings, Norbert

Hope that helps,
Elijah


Re: New Ft. for Git : Allow resumable cloning of repositories.

2019-03-08 Thread Jonathan Tan
> Objective: Allow pause and resume functionality while cloning repositories.
> 
> Below is a rough idea on how this may be achieved.

This is indeed a nice feature to have, and thanks for details of how
this would be accomplished.

> 1) Create a repository_name.json file.
> 2) repository_name.json will be an index file containing list of all
> the files in the repository with default status being "False".
>"False" status of a file signifies that this file is not yet fully
> downloaded.
> 
> Something like this:
> 
> {
>   'file1.ext' : "False",
>   'file2.ext' : "False",
>   'file3.ext' : "False"
> }

One issue is that when cloning a repository, we do not download many
files - we only download one dynamically generated packfile containing
all the objects we want.

You might be interested in some work I'm doing to offload part of the
packfile response to CDNs:

https://public-inbox.org/git/cover.1550963965.git.jonathanta...@google.com/

This means that when cloning/fetching, multiple files could be
downloaded, meaning that a scheme like you suggest would be more
worthwhile. (In fact, I allude to such a scheme in the design document
in patch 5.)


Re: One failed self test on Fedora 29

2019-03-08 Thread Todd Zullinger
Hi,

Jeffrey Walton wrote:
> Fedora 29, x86_64. One failed self test:
> 
> *** t0021-conversion.sh ***
[...]
> not ok 13 - disable filter with empty override
> #
> #   test_config_global filter.disable.smudge false &&
> #   test_config_global filter.disable.clean false &&
> #   test_config filter.disable.smudge false &&
> #   test_config filter.disable.clean false &&
> #
> #   echo "*.disable filter=disable" >.gitattributes &&
> #
> #   echo test >test.disable &&
> #   git -c filter.disable.clean= add test.disable 2>err &&
> #   test_must_be_empty err &&
> #   rm -f test.disable &&
> #   git -c filter.disable.smudge= checkout -- test.disable 2>err 
> &&
> #   test_must_be_empty err
> #
[...]
> # failed 1 among 26 test(s)
> 1..26
> gmake[2]: *** [Makefile:56: t0021-conversion.sh] Error 1
> 
> Does anyone need a config.log or other test data?

It would probably help to know what commit you're building.
The verbose test output would also be useful, e.g.:

cd t && ./t0021-conversion.sh -v -i

If it's not reliably reproducible, the --stress* options
might help catch a failing run.

FWIW, I just built and ran the tests on a Fedora 29
container for master, next, and pu a few times (some with
various --stress options) without any test failures.

I did this with and without a config.mak from the fedora git
packages.  I've never used the configure script, it seems
like unnecessary overhead.

$ git branch -v
  master 6e0cc67761 Start 2.22 cycle
  next   541d9dca55 Merge branch 'yb/utf-16le-bom-spellfix' into next
* pu 7eadd8ba98 Merge branch 'js/remote-curl-i18n' into pu

-- 
Todd


Re: [PATCH v3 00/21] Add new command "switch"

2019-03-08 Thread Ramsay Jones



On 08/03/2019 09:57, Nguyễn Thái Ngọc Duy wrote:
[snip]
> Range-diff dựa trên v2:
>  -:  -- >  1:  949f3dd4fd git-checkout.txt: spell out --no-option
>  1:  8358b9ca36 =  2:  1ddbbae3e2 git-checkout.txt: fix one syntax line
>  2:  1686ccbf8d !  3:  b0cb2372db doc: document --overwrite-ignore
> @@ -14,14 +14,15 @@
>   out anyway. In other words, the ref can be held by more than one
>   worktree.
>   
> -+--[no-]overwrite-ignore::
> ++--overwrite-ignore::
> ++--no-overwrite-ignore::

Just curious, but why? Is '--[no-]overwrite-ignore' thought to
be harder to read? What about the rest of the man-pages?

ATB,
Ramsay Jones



Re: [PATCH v1 01/11] checkout: split part of it to new command 'restore'

2019-03-08 Thread Elijah Newren
Thanks for working on this; overall looks really good.  I've got a few
comments here and there on the wording and combinations of options...

On Fri, Mar 8, 2019 at 2:17 AM Nguyễn Thái Ngọc Duy  wrote:
> +SYNOPSIS

It might be worth adding some words somewhere to differentiate between
`reset` and `restore`.  E.g.

`git restore` modifies the working tree (and maybe index) to change
file content to match some other (usually older) version, but does not
update your branch.  `git reset` is about modifying which commit your
branch points to, meaning possibly removing and/or adding many commits
to your branch.

It may also make sense to add whatever description you use to the reset manpage.

> +
> +[verse]
> +'git restore' [] [--source=] ...
> +'git restore' (-p|--patch) [--source=] [...]

So one cannot specify any special options with -p?  Does that mean one
cannot use it with --index (i.e. this command cannot replace 'git
reset -p')?  Or is this an oversight in the synopsis?

> +DESCRIPTION
> +---
> +Restore paths in the working tree by replacing with the contents in
> +the restore source or remove if the paths do not exist in the restore
> +source. The source is by default the index but could be from a commit.
> +The command can also optionally restore content in the index from a
> +commit.

The first sentence makes it sound like two different operations, when
I think it is one. Perhaps:

Restore paths in the working tree by replacing with the contents in
the restore source (and if the restore source is missing paths found
in the working tree, that means deleting those paths from the working
tree).

> +
> +When a `` is given, the paths that match the `` are
> +updated both in the index and in the working tree.

I thought the default was --worktree.  Is this sentence from an older
version of your patch series that you forgot to update?

> +
> +OPTIONS
> +---
> +-s::
> +--source=::
> +   Restore the working tree files with the content from the given
> +   tree or any revision that leads to a tree (e.g. a commit or a
> +   branch).

I think that's a little hard to parse.  We may not be able to avoid
"working tree" vs. "tree" confusion, but the spelling of  feels
like it should be a separate sentence.  Maybe:

Restore the working tree files with the content from the given tree.
It is common to specify the source tree by naming a commit, branch, or
tag.

?

> +
> +-p::
> +--patch::
> +   Interactively select hunks in the difference between the
> +   `` (or the index, if unspecified) and the working
> +   tree. See the ``Interactive Mode'' section of linkgit:git-add[1]
> +   to learn how to operate the `--patch` mode.
> +
> +-W::
> +--worktree::
> +-I::
> +--index::
> +   Specify the restore location. If neither option is specified,
> +   by default the working tree is restored. If `--index` is
> +   specified without `--worktree` or `--source`, `--source=HEAD`
> +   is implied. These options only make sense to use with
> +   `--source`.

Seems like this contains a minor contradiction.  Perhaps start the
final sentence with: "Otherwise, ..." ?

> +-q::
> +--quiet::
> +   Quiet, suppress feedback messages.
> +
> +--progress::
> +--no-progress::
> +   Progress status is reported on the standard error stream
> +   by default when it is attached to a terminal, unless `--quiet`
> +   is specified. This flag enables progress reporting even if not
> +   attached to a terminal, regardless of `--quiet`.

I'm assuming this means there are feedback messages other than
progress feedback?

> +-f::
> +--force::
> +   If `--source` is not specified, unmerged entries are left alone
> +   and will not fail the operation. Unmerged entries are always
> +   replaced if `--source` is specified, regardless of `--force`.

This may be slightly confusing, in particular it suggests that --index
(or --worktree and --index) are the default.  Is --force only useful
when --index is specified?  If it has utility with --worktree only,
what does it do?  Also, what happens when there are unmerged entries
in the index and someone tries to restore just working tree files --
are the ones corresponding to unmerged entries skipped (if so,
silently or with warnings printed for the user?), or does something
else happen?

> +--ours::
> +--theirs::
> +   Check out stage #2 ('ours') or #3 ('theirs') for unmerged
> +   paths.
> ++
> +Note that during `git rebase` and `git pull --rebase`, 'ours' and
> +'theirs' may appear swapped. See the explanation of the same options
> +in linkgit:git-checkout[1] for details.

So sad, but yes you need to mention it.  I'm curious what we say for
cherry-pick; it's not clear to me whether people think of the current
branch as "ours" or the commit they wrote themselves and are trying to
bring to this branch as "ours".  There are probably examples of both.

Not that I think you can do anything about that here or need to ch

Re: [PATCH v2] line-log: suppress diff output with "-s"

2019-03-08 Thread Johannes Schindelin
Hi Peff,

On Fri, 8 Mar 2019, Jeff King wrote:

> On Fri, Mar 08, 2019 at 04:38:44PM +0100, Johannes Schindelin wrote:
> 
> > On Thu, 7 Mar 2019, Jeff King wrote:
> > 
> > > When "-L" is in use, we ignore any diff output format that the user
> > > provides to us, and just always print a patch (with extra context lines
> > > covering the whole area of interest). It's not entirely clear what we
> > > should do with all formats (e.g., should "--stat" show just the diffstat
> > > of the touched lines, or the stat for the whole file?).
> > > 
> > > But "-s" is pretty clear: the user probably wants to see just the
> > > commits that touched those lines, without any diff at all. Let's at
> > > least make that work.
> > 
> > Agree. The patch looks obviously good.
> 
> Thanks. This leaves the other formats as silently ignored.

I'd be fine with that... but...

> Do we want to do something like this:
> 
> diff --git a/revision.c b/revision.c
> index eb8e51bc63..a1b4fe2aa6 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2689,6 +2689,10 @@ int setup_revisions(int argc, const char **argv, 
> struct rev_info *revs, struct s
>   if (revs->first_parent_only && revs->bisect)
>   die(_("--first-parent is incompatible with --bisect"));
>  
> + if (revs->line_level_traverse &&
> + (revs->diffopt.output_format & 
> ~(DIFF_FORMAT_PATCH|DIFF_FORMAT_NO_OUTPUT)))
> + die(_("-L does not yet support diff formats besides -p and 
> -s"));
> +
>   if (revs->expand_tabs_in_log < 0)
>   revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;

Since you already have that patch, why not go wild and apply it, too? ;-)

I guess you copy-edited the code from somewhere because you usually do
leave spaces around the `|`... I don't care, though ;-)

Ciao,
Dscho


[PATCH] upload-pack.c: fix a sparse 'NULL pointer' warning

2019-03-08 Thread Ramsay Jones


Signed-off-by: Ramsay Jones 
---

Hi Jonathan,

If you need to re-roll your 'jt/fetch-cdn-offload' branch, could you
please squash this into the relevant patch (commit 0e821b4427
("upload-pack: send part of packfile response as uri", 2019-02-23)).

Thanks!

ATB,
Ramsay Jones

 upload-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/upload-pack.c b/upload-pack.c
index 534e8951a2..a421df2bbb 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1164,7 +1164,7 @@ void upload_pack(struct upload_pack_options *options)
if (want_obj.nr) {
struct object_array have_obj = OBJECT_ARRAY_INIT;
get_common_commits(&reader, &have_obj, &want_obj);
-   create_pack_file(&have_obj, &want_obj, 0);
+   create_pack_file(&have_obj, &want_obj, NULL);
}
 }
 
-- 
2.21.0


Re: [PATCH v5 02/10] add--helper: create builtin helper for interactive add

2019-03-08 Thread Johannes Schindelin
Hi Junio,

On Thu, 21 Feb 2019, Junio C Hamano wrote:

> "Daniel Ferreira via GitGitGadget"  writes:
> 
> > diff --git a/git.c b/git.c
> > index 2dd588674f..cb42591f37 100644
> > --- a/git.c
> > +++ b/git.c
> > @@ -444,6 +444,7 @@ static int run_builtin(struct cmd_struct *p, int argc, 
> > const char **argv)
> >  
> >  static struct cmd_struct commands[] = {
> > { "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
> > +   { "add--helper", cmd_add__helper, RUN_SETUP | NEED_WORK_TREE },
> > { "am", cmd_am, RUN_SETUP | NEED_WORK_TREE },
> > { "annotate", cmd_annotate, RUN_SETUP | NO_PARSEOPT },
> > { "apply", cmd_apply, RUN_SETUP_GENTLY },
> 
> When adding a more complex replacement command we often cannot write
> RUN_SETUP nor NEED_WORK_TREE here and instead do the job ourselves
> in cmd_ourcommand() the hard way.  But because this one is only for
> helping "add -i", we can let the git.c::run_builtin() aka "git
> potty" do the hard part, which is very nice.

Sadly, I ran into a quite curious (and seemingly equally insurmountable)
problem with this approach: when I use `system()` to execute a Git
built-in from the Perl script, and when that built-in then consumes even a
single byte of stdin, that file descriptor gets closed in the Perl
process, and no amount of file descriptor duplication and redirecting that
I could come up helped. After calling the built-in helper to run, say, the
`update` command, the main interactive add process always exited on me due
to EOF on stdin, and I could not make it work.

To be sure, this happens only with MSYS2's Perl. If I try the same on
Linux (WSL, really), everything works Just Fine.

So my current approach is actually the other way round: I started
implementing the main loop in C, and implemented all commands except for
`patch` so far, and I think I will just hand off to the Perl script for
that command for now.

Unfortunately that means that my current state looks already quite a bit
different than what you reviewed. My hope is, however, that it will make
for a nice "story" once I have a version to show that I think could be
applied as-is.

Ciao,
Dscho


Re: Bug: git for Windows spawning a lot of processes recursively

2019-03-08 Thread Johannes Schindelin
Hi Pierre,

On Fri, 22 Feb 2019, Garcia, Pierre wrote:

> I'd like to report an issue with git for Windows
> 
> Git version 2.20.1
> Windows 7 x64 (Build 7601: Service Pack 1)
> 
> 
> Issue:
> When running from Git-bash (not even inside a repo), git commands from
> built-in are OK, but if I use the exe that is located at C:\Program
> Files\Git\bin\git.exe, the process takes long to execute and then exits
> with error "error launching git: The filename or extension is too
> long.":

Is this still happening with v2.21.0? Or for that matter, with the latest
snapshot at https://wingit.blob.core.windows.net/files/index.html?

Ciao,
Johannes

> 
> $ git --version
> git version 2.20.1.windows.1
> 
> $ which git
> /mingw64/bin/git
> 
> $ /c/Program\ Files/Git/bin/git.exe --version
> error launching git: The filename or extension is too long.
> 
> 
> It started out of the blue, nothing special was done on the computer, the 
> previous day I cloned some repos, the next it was not working anymore.
> I tried uninstall-reinstall with no success.
> I'm using Gitextensions as well, but I verified that the problem was 
> occurring without Gitextensions (uninstalled at the time the traces were 
> taken).
> 
> 
> I ran procmon.exe to monitor the activity of the process and I found out that 
> git.exe was spawning itself 385 times in total in the trace I took, until all 
> processes exit with error code 1, here is an extract of the procmon trace, 
> showing thread and process activity for git.exe (I have more exhaustive trace 
> but the file is quite big), I included the environment variables on the first 
> call:
> 
> Time of Day   Process NamePID Operation   PathResult  Detail
> 8:57:20.7 git.exe 12744   Process Start   SUCCESS "Parent PID: 
> 8412, Command line: ""C:\Program Files\Git\bin\git.exe""  --version, Current 
> directory: C:\Users\gpierre\Desktop\, Environment: 
> ; =::=::\
> ; =C:=C:\Users\gpierre\Desktop
> ; =ExitCode=0001
> ; ALLUSERSPROFILE=C:\ProgramData
> ; CommonProgramFiles=C:\Program Files\Common Files
> ; CommonProgramFiles(x86)=C:\Program Files (x86)\Common Files
> ; CommonProgramW6432=C:\Program Files\Common Files
> ; ComSpec=C:\windows\system32\cmd.exe
> ; DEFLOGDIR=C:\ProgramData\McAfee\Endpoint Security\Logs
> ; FP_NO_HOST_CHECK=NO
> ; GTK_BASEPATH=C:\Program Files (x86)\GtkSharp\2.12\
> ; HasSCCM2012Client=1
> ; HOME=C:\Users\gpierre
> ; HOMEDRIVE=C:
> ; HOMEPATH=\Users\gpierre
> ; HOMESHARE=C:\Users\gpierre
> ; LOCALAPPDATA=C:\Users\gpierre\AppData\Local
> ; NUMBER_OF_PROCESSORS=4
> ; OS=Windows_NT
> ; 
> Path=C:\ProgramData\Oracle\Java\javapath;C:\windows\system32;C:\windows;C:\windows\System32\Wbem;C:\windows\System32\WindowsPowerShell\v1.0\;C:\Program
>  Files\Dell\Dell Data Protection\Encryption\;C:\Program Files (x86)\NVIDIA 
> Corporation\PhysX\Common;C:\Program Files\Microsoft SQL 
> Server\130\Tools\Binn\;C:\Program Files\dotnet\;C:\Program Files 
> (x86)\GtkSharp\2.12\bin;C:\windows\system32\config\systemprofile\.dnx\bin;C:\Program
>  Files\Microsoft 
> DNX\Dnvm\;C:\EASE\APEX12-4.4.0\bin;C:\EASE\APEX12-4.4.0\platforms\default\DEOS;C:\Program
>  
> Files\PuTTY\;C:\windows\System32\WindowsPowerShell\v1.0\;C:\windows\System32\WindowsPowerShell\v1.0\;C:\Program
>  
> Files\1E\NomadBranch\;C:\HashiCorp\Vagrant\bin;C:\windows\System32\WindowsPowerShell\v1.0\
> ; PATHEXT=.COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC
> ; PROCESSOR_ARCHITECTURE=AMD64
> ; PROCESSOR_IDENTIFIER=Intel64 Family 6 Model 94 Stepping 3, GenuineIntel
> ; PROCESSOR_LEVEL=6
> ; PROCESSOR_REVISION=5e03
> ; ProgramData=C:\ProgramData
> ; ProgramFiles=C:\Program Files
> ; ProgramFiles(x86)=C:\Program Files (x86)
> ; ProgramW6432=C:\Program Files
> ; PROMPT=$P$G
> ; PSModulePath=C:\Program 
> Files\WindowsPowerShell\Modules;C:\windows\system32\WindowsPowerShell\v1.0\Modules
> ; PUBLIC=C:\Users\Public
> ; SESSIONNAME=Console
> ; SNC_LIB=gsskrb5.dll
> ; SOEDataPartition=C:
> ; SOEDesktopAdminModel=User
> ; SOESystemPartitionLabel=SOE-Disk
> ; SystemDrive=C:
> ; SystemRoot=C:\windows
> ; TEMP=C:\Users\gpierre\AppData\Local\Temp
> ; TMP=C:\Users\gpierre\AppData\Local\Temp
> ; USERNAME=gpierre
> ; USERPROFILE=C:\Users\gpierre
> ; VBOX_MSI_INSTALL_PATH=C:\Program Files\Oracle\VirtualBox\
> ; VS140COMNTOOLS=C:\Program Files (x86)\Microsoft Visual Studio 
> 14.0\Common7\Tools\
> ; VS80COMNTOOLS=C:\Program Files (x86)\Microsoft Visual Studio 
> 8\Common7\Tools\
> ; windir=C:\windows
> ; windows_tracing_flags=3
> ; windows_tracing_logfile=C:\BVTBin\Tests\installpackage\csilogfile.l

[PATCH v2 1/8] http: use --stdin when getting dumb HTTP pack

2019-03-08 Thread Jonathan Tan
When Git fetches a pack using dumb HTTP, it reuses the server's name for
the packfile (which incorporates a hash), which is different from the
behavior of fetch-pack and receive-pack.

A subsequent patch will allow downloading packs over HTTP(S) as part of
a fetch. These downloads will not necessarily be from a Git repository,
and thus may not have a hash as part of its name.

Thus, teach http to pass --stdin to index-pack, so that we have no
reliance on the server's name for the packfile.

Signed-off-by: Jonathan Tan 
Signed-off-by: Junio C Hamano 
---
 http.c | 33 +++--
 1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/http.c b/http.c
index a32ad36ddf..34f82af87c 100644
--- a/http.c
+++ b/http.c
@@ -2204,9 +2204,9 @@ int finish_http_pack_request(struct http_pack_request 
*preq)
 {
struct packed_git **lst;
struct packed_git *p = preq->target;
-   char *tmp_idx;
-   size_t len;
struct child_process ip = CHILD_PROCESS_INIT;
+   int tmpfile_fd;
+   int ret = 0;
 
close_pack_index(p);
 
@@ -2218,35 +2218,24 @@ int finish_http_pack_request(struct http_pack_request 
*preq)
lst = &((*lst)->next);
*lst = (*lst)->next;
 
-   if (!strip_suffix(preq->tmpfile.buf, ".pack.temp", &len))
-   BUG("pack tmpfile does not end in .pack.temp?");
-   tmp_idx = xstrfmt("%.*s.idx.temp", (int)len, preq->tmpfile.buf);
+   tmpfile_fd = xopen(preq->tmpfile.buf, O_RDONLY);
 
argv_array_push(&ip.args, "index-pack");
-   argv_array_pushl(&ip.args, "-o", tmp_idx, NULL);
-   argv_array_push(&ip.args, preq->tmpfile.buf);
+   argv_array_push(&ip.args, "--stdin");
ip.git_cmd = 1;
-   ip.no_stdin = 1;
+   ip.in = tmpfile_fd;
ip.no_stdout = 1;
 
if (run_command(&ip)) {
-   unlink(preq->tmpfile.buf);
-   unlink(tmp_idx);
-   free(tmp_idx);
-   return -1;
-   }
-
-   unlink(sha1_pack_index_name(p->sha1));
-
-   if (finalize_object_file(preq->tmpfile.buf, sha1_pack_name(p->sha1))
-|| finalize_object_file(tmp_idx, sha1_pack_index_name(p->sha1))) {
-   free(tmp_idx);
-   return -1;
+   ret = -1;
+   goto cleanup;
}
 
install_packed_git(the_repository, p);
-   free(tmp_idx);
-   return 0;
+cleanup:
+   close(tmpfile_fd);
+   unlink(preq->tmpfile.buf);
+   return ret;
 }
 
 struct http_pack_request *new_http_pack_request(
-- 
2.19.0.271.gfe8321ec05.dirty



[PATCH v2 0/8] CDN offloading of fetch response

2019-03-08 Thread Jonathan Tan
Here's my current progress - the only thing that is lacking is more
tests, maybe, so I think it's ready for review.

I wrote in version 1:

> I know
> that there are some implementation details that could be improved (e.g.
> parallelization of the CDN downloads, starting CDN downloads *after*
> closing the first HTTP request, holding on to the .keep locks until
> after the refs are set), but will work on those once the overall design
> is more or less finalized.

This code now starts CDN downloads after closing the first HTTP request,
and it holds on to the .keep files until after the refs are set. I'll
leave parallelization of the CDN downloads for later work.

One relatively significant change: someone pointed out that the issue fixed by 
50d3413740 ("http: make redirects more obvious", 2016-12-06) could also
occur here, so I have changed it so that the server is required to send
the packfile's hash along with the URI.

This does mean that Ævar's workflow described in [1] would not work.
Quoting Ævar:

> More concretely, I'd like to have a setup where a server can just dumbly
> point to some URL that probably has most of the data, without having any
> idea what OIDs are in it. So that e.g. some machine entirely
> disconnected from the server (and with just a regular clone) can
> continually generating an up-to-date-enough packfile.

With 50d3413740, it seems to me that it's important for the server to
know details about the URIs that it points to, so such a disconnection
would not work.

[1] https://public-inbox.org/git/87k1hv6eel@evledraar.gmail.com/

Jonathan Tan (8):
  http: use --stdin when getting dumb HTTP pack
  http: improve documentation of http_pack_request
  http-fetch: support fetching packfiles by URL
  Documentation: order protocol v2 sections
  Documentation: add Packfile URIs design doc
  upload-pack: refactor reading of pack-objects out
  fetch-pack: support more than one pack lockfile
  upload-pack: send part of packfile response as uri

 Documentation/git-http-fetch.txt |   8 +-
 Documentation/technical/packfile-uri.txt |  78 
 Documentation/technical/protocol-v2.txt  |  44 +--
 builtin/fetch-pack.c |  17 ++-
 builtin/pack-objects.c   |  76 +++
 connected.c  |   8 +-
 fetch-pack.c | 129 +--
 fetch-pack.h |   2 +-
 http-fetch.c |  64 --
 http.c   |  82 +++-
 http.h   |  32 -
 t/t5550-http-fetch-dumb.sh   |  25 
 t/t5702-protocol-v2.sh   |  57 +
 transport-helper.c   |   5 +-
 transport.c  |  14 +-
 transport.h  |   6 +-
 upload-pack.c| 155 +--
 17 files changed, 670 insertions(+), 132 deletions(-)
 create mode 100644 Documentation/technical/packfile-uri.txt

Range-diff against v1:
1:  987c9b686b < -:  -- http: use --stdin and --keep when downloading 
pack
-:  -- > 1:  87173d0ad1 http: use --stdin when getting dumb HTTP pack
2:  4b53a6f48c ! 2:  66d31720a0 http: improve documentation of http_pack_request
@@ -45,4 +45,4 @@
 + */
  extern struct http_pack_request *new_http_pack_request(
struct packed_git *target, const char *base_url);
- int finish_http_pack_request(struct http_pack_request *preq, char 
**lockfile);
+ extern int finish_http_pack_request(struct http_pack_request *preq);
3:  afe73a282d ! 3:  02373f6afe http-fetch: support fetching packfiles by URL
@@ -31,7 +31,8 @@
 +--packfile::
 +  Instead of a commit id on the command line (which is not expected in
 +  this case), 'git http-fetch' fetches the packfile directly at the given
-+  URL and generates the corresponding .idx file.
++  URL and uses index-pack to generate corresponding .idx and .keep files.
++  The output of index-pack is printed to stdout.
 +
  --recover::
Verify that everything reachable from target is fetched.  Used after
@@ -101,12 +102,12 @@
 +  struct http_pack_request *preq;
 +  struct slot_results results;
 +  int ret;
-+  char *lockfile;
 +
 +  preq = new_http_pack_request(NULL, url);
 +  if (preq == NULL)
 +  die("couldn't create http pack request");
 +  preq->slot->results = &results;
++  preq->generate_keep = 1;
 +
 +  if (start_active_slot(preq->slot)) {
 +  run_active_slot(preq->slot);
@@ -118,9 +119,8 @@
 +  die("Unable to start request");
 +  }
 +
-+  if ((ret = finish_http_pack_request(preq, &lockfile)))
++  if ((ret = finish_http_pack_request(preq)))
 

[PATCH v2 6/8] upload-pack: refactor reading of pack-objects out

2019-03-08 Thread Jonathan Tan
Subsequent patches will change how the output of pack-objects is
processed, so extract that processing into its own function.

Currently, at most 1 character can be buffered (in the "buffered" local
variable). One of those patches will require a larger buffer, so replace
that "buffered" local variable with a buffer array.

Signed-off-by: Jonathan Tan 
Signed-off-by: Junio C Hamano 
---
 upload-pack.c | 81 ++-
 1 file changed, 47 insertions(+), 34 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index d098ef5982..987d2e139b 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -102,14 +102,52 @@ static int write_one_shallow(const struct commit_graft 
*graft, void *cb_data)
return 0;
 }
 
+struct output_state {
+   char buffer[8193];
+   int used;
+};
+
+static int relay_pack_data(int pack_objects_out, struct output_state *os)
+{
+   /*
+* We keep the last byte to ourselves
+* in case we detect broken rev-list, so that we
+* can leave the stream corrupted.  This is
+* unfortunate -- unpack-objects would happily
+* accept a valid packdata with trailing garbage,
+* so appending garbage after we pass all the
+* pack data is not good enough to signal
+* breakage to downstream.
+*/
+   ssize_t readsz;
+
+   readsz = xread(pack_objects_out, os->buffer + os->used,
+  sizeof(os->buffer) - os->used);
+   if (readsz < 0) {
+   return readsz;
+   }
+   os->used += readsz;
+
+   if (os->used > 1) {
+   send_client_data(1, os->buffer, os->used - 1);
+   os->buffer[0] = os->buffer[os->used - 1];
+   os->used = 1;
+   } else {
+   send_client_data(1, os->buffer, os->used);
+   os->used = 0;
+   }
+
+   return readsz;
+}
+
 static void create_pack_file(const struct object_array *have_obj,
 const struct object_array *want_obj)
 {
struct child_process pack_objects = CHILD_PROCESS_INIT;
-   char data[8193], progress[128];
+   struct output_state output_state = {0};
+   char progress[128];
char abort_msg[] = "aborting due to possible repository "
"corruption on the remote side.";
-   int buffered = -1;
ssize_t sz;
int i;
FILE *pipe_fd;
@@ -239,39 +277,15 @@ static void create_pack_file(const struct object_array 
*have_obj,
continue;
}
if (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) {
-   /* Data ready; we keep the last byte to ourselves
-* in case we detect broken rev-list, so that we
-* can leave the stream corrupted.  This is
-* unfortunate -- unpack-objects would happily
-* accept a valid packdata with trailing garbage,
-* so appending garbage after we pass all the
-* pack data is not good enough to signal
-* breakage to downstream.
-*/
-   char *cp = data;
-   ssize_t outsz = 0;
-   if (0 <= buffered) {
-   *cp++ = buffered;
-   outsz++;
-   }
-   sz = xread(pack_objects.out, cp,
- sizeof(data) - outsz);
-   if (0 < sz)
-   ;
-   else if (sz == 0) {
+   int result = relay_pack_data(pack_objects.out,
+&output_state);
+
+   if (result == 0) {
close(pack_objects.out);
pack_objects.out = -1;
-   }
-   else
+   } else if (result < 0) {
goto fail;
-   sz += outsz;
-   if (1 < sz) {
-   buffered = data[sz-1] & 0xFF;
-   sz--;
}
-   else
-   buffered = -1;
-   send_client_data(1, data, sz);
}
 
/*
@@ -296,9 +310,8 @@ static void create_pack_file(const struct object_array 
*have_obj,
}
 
/* flush the data */
-   if (0 <= buffered) {
-   data[0] = buffered;
-   send_client_data(1, data, 1);
+   if (output_state.used > 0) {
+   send_client_data(1, output_state.buffer, output_state.used);
fprintf(stderr, "flushed.\n");
}
if (use_sideband)
-- 
2.19.0.271.gfe8321ec05.dirty



[PATCH v2 7/8] fetch-pack: support more than one pack lockfile

2019-03-08 Thread Jonathan Tan
Whenever a fetch results in a packfile being downloaded, a .keep file is
generated, so that the packfile can be preserved (from, say, a running
"git repack") until refs are written referring to the contents of the
packfile.

In a subsequent patch, a successful fetch using protocol v2 may result
in more than one .keep file being generated. Therefore, teach
fetch_pack() and the transport mechanism to support multiple .keep
files.

Implementation notes:

 - builtin/fetch-pack.c normally does not generate .keep files, and thus
   is unaffected by this or future changes. However, it has an
   undocumented "--lock-pack" feature, used by remote-curl.c when
   implementing the "fetch" remote helper command. In keeping with the
   remote helper protocol, only one "lock" line will ever be written;
   the rest will result in warnings to stderr. However, in practice,
   warnings will never be written because the remote-curl.c "fetch" is
   only used for protocol v0/v1 (which will not generate multiple .keep
   files). (Protocol v2 uses the "stateless-connect" command, not the
   "fetch" command.)

 - connected.c has an optimization in that connectivity checks on a ref
   need not be done if the target object is in a pack known to be
   self-contained and connected. If there are multiple packfiles, this
   optimization can no longer be done.

Signed-off-by: Jonathan Tan 
---
 builtin/fetch-pack.c | 17 +++--
 connected.c  |  8 +---
 fetch-pack.c | 23 ---
 fetch-pack.h |  2 +-
 transport-helper.c   |  5 +++--
 transport.c  | 14 --
 transport.h  |  6 +++---
 7 files changed, 43 insertions(+), 32 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 153a2bd282..709fc4c44b 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -48,8 +48,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
struct ref **sought = NULL;
int nr_sought = 0, alloc_sought = 0;
int fd[2];
-   char *pack_lockfile = NULL;
-   char **pack_lockfile_ptr = NULL;
+   struct string_list pack_lockfiles = STRING_LIST_INIT_DUP;
+   struct string_list *pack_lockfiles_ptr = NULL;
struct child_process *conn;
struct fetch_pack_args args;
struct oid_array shallow = OID_ARRAY_INIT;
@@ -134,7 +134,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
}
if (!strcmp("--lock-pack", arg)) {
args.lock_pack = 1;
-   pack_lockfile_ptr = &pack_lockfile;
+   pack_lockfiles_ptr = &pack_lockfiles;
continue;
}
if (!strcmp("--check-self-contained-and-connected", arg)) {
@@ -235,10 +235,15 @@ int cmd_fetch_pack(int argc, const char **argv, const 
char *prefix)
}
 
ref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,
-&shallow, pack_lockfile_ptr, version);
-   if (pack_lockfile) {
-   printf("lock %s\n", pack_lockfile);
+&shallow, pack_lockfiles_ptr, version);
+   if (pack_lockfiles.nr) {
+   int i;
+
+   printf("lock %s\n", pack_lockfiles.items[0].string);
fflush(stdout);
+   for (i = 1; i < pack_lockfiles.nr; i++)
+   warning(_("Lockfile created but not reported: %s"),
+   pack_lockfiles.items[i].string);
}
if (args.check_self_contained_and_connected &&
args.self_contained_and_connected) {
diff --git a/connected.c b/connected.c
index 1bba888eff..a866fcf9e8 100644
--- a/connected.c
+++ b/connected.c
@@ -40,10 +40,12 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 
if (transport && transport->smart_options &&
transport->smart_options->self_contained_and_connected &&
-   transport->pack_lockfile &&
-   strip_suffix(transport->pack_lockfile, ".keep", &base_len)) {
+   transport->pack_lockfiles.nr == 1 &&
+   strip_suffix(transport->pack_lockfiles.items[0].string,
+".keep", &base_len)) {
struct strbuf idx_file = STRBUF_INIT;
-   strbuf_add(&idx_file, transport->pack_lockfile, base_len);
+   strbuf_add(&idx_file, transport->pack_lockfiles.items[0].string,
+  base_len);
strbuf_addstr(&idx_file, ".idx");
new_pack = add_packed_git(idx_file.buf, idx_file.len, 1);
strbuf_release(&idx_file);
diff --git a/fetch-pack.c b/fetch-pack.c
index 812be15d7e..cf89af21d9 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -755,7 +755,7 @@ static int sideband_demux(int in, int out, void *data)
 }
 
 static int get_pack(struct fetch_pack_args *args,
-   int xd[2], char **pack_lockfile)
+   int 

[PATCH v2 2/8] http: improve documentation of http_pack_request

2019-03-08 Thread Jonathan Tan
struct http_pack_request and the functions that use it will be modified
in a subsequent patch. Using it is complicated (to use, call the
initialization function, then set some but not all fields in the
returned struct), so add some documentation to help future users.

Signed-off-by: Jonathan Tan 
Signed-off-by: Junio C Hamano 
---
 http.h | 19 ++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/http.h b/http.h
index 4eb4e808e5..ded1edcca4 100644
--- a/http.h
+++ b/http.h
@@ -202,14 +202,31 @@ extern int http_get_info_packs(const char *base_url,
struct packed_git **packs_head);
 
 struct http_pack_request {
+   /*
+* Initialized by new_http_pack_request().
+*/
char *url;
struct packed_git *target;
+   struct active_request_slot *slot;
+
+   /*
+* After calling new_http_pack_request(), point lst to the head of the
+* pack list that target is in. finish_http_pack_request() will remove
+* target from lst and call install_packed_git() on target.
+*/
struct packed_git **lst;
+
+   /*
+* State managed by functions in http.c.
+*/
FILE *packfile;
struct strbuf tmpfile;
-   struct active_request_slot *slot;
 };
 
+/*
+ * target must be an element in a pack list obtained from
+ * http_get_info_packs().
+ */
 extern struct http_pack_request *new_http_pack_request(
struct packed_git *target, const char *base_url);
 extern int finish_http_pack_request(struct http_pack_request *preq);
-- 
2.19.0.271.gfe8321ec05.dirty



[PATCH v2 3/8] http-fetch: support fetching packfiles by URL

2019-03-08 Thread Jonathan Tan
Teach http-fetch the ability to download packfiles directly, given a
URL, and to verify them.

The http_pack_request suite of functions have been modified to support a
NULL target. When target is NULL, the given URL is downloaded directly
instead of being treated as the root of a repository.

Signed-off-by: Jonathan Tan 
Signed-off-by: Junio C Hamano 
---
 Documentation/git-http-fetch.txt |  8 +++-
 http-fetch.c | 64 +---
 http.c   | 49 +---
 http.h   | 19 --
 t/t5550-http-fetch-dumb.sh   | 25 +
 5 files changed, 135 insertions(+), 30 deletions(-)

diff --git a/Documentation/git-http-fetch.txt b/Documentation/git-http-fetch.txt
index 666b042679..8357359a9b 100644
--- a/Documentation/git-http-fetch.txt
+++ b/Documentation/git-http-fetch.txt
@@ -9,7 +9,7 @@ git-http-fetch - Download from a remote Git repository via HTTP
 SYNOPSIS
 
 [verse]
-'git http-fetch' [-c] [-t] [-a] [-d] [-v] [-w filename] [--recover] [--stdin] 
 
+'git http-fetch' [-c] [-t] [-a] [-d] [-v] [-w filename] [--recover] [--stdin | 
--packfile | ] 
 
 DESCRIPTION
 ---
@@ -40,6 +40,12 @@ commit-id::
 
['\t']
 
+--packfile::
+   Instead of a commit id on the command line (which is not expected in
+   this case), 'git http-fetch' fetches the packfile directly at the given
+   URL and uses index-pack to generate corresponding .idx and .keep files.
+   The output of index-pack is printed to stdout.
+
 --recover::
Verify that everything reachable from target is fetched.  Used after
an earlier fetch is interrupted.
diff --git a/http-fetch.c b/http-fetch.c
index a32ac118d9..a9764d6f96 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -5,7 +5,7 @@
 #include "walker.h"
 
 static const char http_fetch_usage[] = "git http-fetch "
-"[-c] [-t] [-a] [-v] [--recover] [-w ref] [--stdin] commit-id url";
+"[-c] [-t] [-a] [-v] [--recover] [-w ref] [--stdin | --packfile | commit-id] 
url";
 
 int cmd_main(int argc, const char **argv)
 {
@@ -19,6 +19,7 @@ int cmd_main(int argc, const char **argv)
int rc = 0;
int get_verbosely = 0;
int get_recover = 0;
+   int packfile = 0;
 
while (arg < argc && argv[arg][0] == '-') {
if (argv[arg][1] == 't') {
@@ -35,43 +36,80 @@ int cmd_main(int argc, const char **argv)
get_recover = 1;
} else if (!strcmp(argv[arg], "--stdin")) {
commits_on_stdin = 1;
+   } else if (!strcmp(argv[arg], "--packfile")) {
+   packfile = 1;
}
arg++;
}
-   if (argc != arg + 2 - commits_on_stdin)
+   if (argc != arg + 2 - (commits_on_stdin || packfile))
usage(http_fetch_usage);
if (commits_on_stdin) {
commits = walker_targets_stdin(&commit_id, &write_ref);
+   } else if (packfile) {
+   /* URL will be set later */
} else {
commit_id = (char **) &argv[arg++];
commits = 1;
}
 
-   if (argv[arg])
-   str_end_url_with_slash(argv[arg], &url);
+   if (packfile) {
+   url = xstrdup(argv[arg]);
+   } else {
+   if (argv[arg])
+   str_end_url_with_slash(argv[arg], &url);
+   }
 
setup_git_directory();
 
git_config(git_default_config, NULL);
 
http_init(NULL, url, 0);
-   walker = get_http_walker(url);
-   walker->get_verbosely = get_verbosely;
-   walker->get_recover = get_recover;
 
-   rc = walker_fetch(walker, commits, commit_id, write_ref, url);
+   if (packfile) {
+   struct http_pack_request *preq;
+   struct slot_results results;
+   int ret;
+
+   preq = new_http_pack_request(NULL, url);
+   if (preq == NULL)
+   die("couldn't create http pack request");
+   preq->slot->results = &results;
+   preq->generate_keep = 1;
+
+   if (start_active_slot(preq->slot)) {
+   run_active_slot(preq->slot);
+   if (results.curl_result != CURLE_OK) {
+   die("Unable to get pack file %s\n%s", preq->url,
+   curl_errorstr);
+   }
+   } else {
+   die("Unable to start request");
+   }
+
+   if ((ret = finish_http_pack_request(preq)))
+   die("finish_http_pack_request gave result %d", ret);
+   release_http_pack_request(preq);
+   rc = 0;
+   } else {
+   walker = get_http_walker(url);
+   walker->get_verbosely = get_verbosely;
+   walker->get_recover = get_recover;
+
+   rc = walker_

[PATCH v2 8/8] upload-pack: send part of packfile response as uri

2019-03-08 Thread Jonathan Tan
Teach upload-pack to send part of its packfile response as URIs.

An administrator may configure a repository with one or more
"uploadpack.blobpackfileuri" lines, each line containing an OID, a pack
hash, and a URI. A client may configure fetch.uriprotocols to be a
comma-separated list of protocols that it is willing to use to fetch
additional packfiles - this list will be sent to the server. Whenever an
object with one of those OIDs would appear in the packfile transmitted
by upload-pack, the server may exclude that object, and instead send the
URI. The client will then download the packs referred to by those URIs
before performing the connectivity check.

Signed-off-by: Jonathan Tan 
Signed-off-by: Junio C Hamano 
---
 builtin/pack-objects.c |  76 
 fetch-pack.c   | 112 +++--
 t/t5702-protocol-v2.sh |  57 +
 upload-pack.c  |  78 +---
 4 files changed, 312 insertions(+), 11 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index a9fac7c128..2fa962c87d 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -110,6 +110,8 @@ static unsigned long window_memory_limit = 0;
 
 static struct list_objects_filter_options filter_options;
 
+static struct string_list uri_protocols = STRING_LIST_INIT_NODUP;
+
 enum missing_action {
MA_ERROR = 0,  /* fail if any missing objects are encountered */
MA_ALLOW_ANY,  /* silently allow ALL missing objects */
@@ -118,6 +120,15 @@ enum missing_action {
 static enum missing_action arg_missing_action;
 static show_object_fn fn_show_object;
 
+struct configured_exclusion {
+   struct oidmap_entry e;
+   char *pack_hash_hex;
+   char *uri;
+};
+static struct oidmap configured_exclusions;
+
+static struct oidset excluded_by_config;
+
 /*
  * stats
  */
@@ -832,6 +843,25 @@ static off_t write_reused_pack(struct hashfile *f)
return reuse_packfile_offset - sizeof(struct pack_header);
 }
 
+static void write_excluded_by_configs(void)
+{
+   struct oidset_iter iter;
+   const struct object_id *oid;
+
+   oidset_iter_init(&excluded_by_config, &iter);
+   while ((oid = oidset_iter_next(&iter))) {
+   struct configured_exclusion *ex =
+   oidmap_get(&configured_exclusions, oid);
+
+   if (!ex)
+   BUG("configured exclusion wasn't configured");
+   write_in_full(1, ex->pack_hash_hex, strlen(ex->pack_hash_hex));
+   write_in_full(1, " ", 1);
+   write_in_full(1, ex->uri, strlen(ex->uri));
+   write_in_full(1, "\n", 1);
+   }
+}
+
 static const char no_split_warning[] = N_(
 "disabling bitmap writing, packs are split due to pack.packSizeLimit"
 );
@@ -1125,6 +1155,25 @@ static int want_object_in_pack(const struct object_id 
*oid,
}
}
 
+   if (uri_protocols.nr) {
+   struct configured_exclusion *ex =
+   oidmap_get(&configured_exclusions, oid);
+   int i;
+   const char *p;
+
+   if (ex) {
+   for (i = 0; i < uri_protocols.nr; i++) {
+   if (skip_prefix(ex->uri,
+   uri_protocols.items[i].string,
+   &p) &&
+   *p == ':') {
+   oidset_insert(&excluded_by_config, oid);
+   return 0;
+   }
+   }
+   }
+   }
+
return 1;
 }
 
@@ -2726,6 +2775,29 @@ static int git_pack_config(const char *k, const char *v, 
void *cb)
pack_idx_opts.version);
return 0;
}
+   if (!strcmp(k, "uploadpack.blobpackfileuri")) {
+   struct configured_exclusion *ex = xmalloc(sizeof(*ex));
+   const char *oid_end, *pack_end;
+   /*
+* Stores the pack hash. This is not a true object ID, but is
+* of the same form.
+*/
+   struct object_id pack_hash;
+
+   if (parse_oid_hex(v, &ex->e.oid, &oid_end) ||
+   *oid_end != ' ' ||
+   parse_oid_hex(oid_end + 1, &pack_hash, &pack_end) ||
+   *pack_end != ' ')
+   die(_("value of uploadpack.blobpackfileuri must be "
+ "of the form '  ' 
(got '%s')"), v);
+   if (oidmap_get(&configured_exclusions, &ex->e.oid))
+   die(_("object already configured in another "
+ "uploadpack.blobpackfileuri (got '%s')"), v);
+   ex->pack_hash_hex = xcalloc(1, pack_end - oid_end);
+   memcpy(ex->pack_hash_hex, oid_end + 1, pack_end - oid_e

[PATCH v2 5/8] Documentation: add Packfile URIs design doc

2019-03-08 Thread Jonathan Tan
Signed-off-by: Jonathan Tan 
Signed-off-by: Junio C Hamano 
---
 Documentation/technical/packfile-uri.txt | 78 
 Documentation/technical/protocol-v2.txt  | 28 -
 2 files changed, 105 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/technical/packfile-uri.txt

diff --git a/Documentation/technical/packfile-uri.txt 
b/Documentation/technical/packfile-uri.txt
new file mode 100644
index 00..6a5a6440d5
--- /dev/null
+++ b/Documentation/technical/packfile-uri.txt
@@ -0,0 +1,78 @@
+Packfile URIs
+=
+
+This feature allows servers to serve part of their packfile response as URIs.
+This allows server designs that improve scalability in bandwidth and CPU usage
+(for example, by serving some data through a CDN), and (in the future) provides
+some measure of resumability to clients.
+
+This feature is available only in protocol version 2.
+
+Protocol
+
+
+The server advertises `packfile-uris`.
+
+If the client then communicates which protocols (HTTPS, etc.) it supports with
+a `packfile-uris` argument, the server MAY send a `packfile-uris` section
+directly before the `packfile` section (right after `wanted-refs` if it is
+sent) containing URIs of any of the given protocols. The URIs point to
+packfiles that use only features that the client has declared that it supports
+(e.g. ofs-delta and thin-pack). See protocol-v2.txt for the documentation of
+this section.
+
+Clients then should understand that the returned packfile could be incomplete,
+and that it needs to download all the given URIs before the fetch or clone is
+complete.
+
+Server design
+-
+
+The server can be trivially made compatible with the proposed protocol by
+having it advertise `packfile-uris`, tolerating the client sending
+`packfile-uris`, and never sending any `packfile-uris` section. But we should
+include some sort of non-trivial implementation in the Minimum Viable Product,
+at least so that we can test the client.
+
+This is the implementation: a feature, marked experimental, that allows the
+server to be configured by one or more `uploadpack.blobPackfileUri=
+` entries. Whenever the list of objects to be sent is assembled, a blob
+with the given sha1 can be replaced by the given URI. This allows, for example,
+servers to delegate serving of large blobs to CDNs.
+
+Client design
+-
+
+While fetching, the client needs to remember the list of URIs and cannot
+declare that the fetch is complete until all URIs have been downloaded as
+packfiles.
+
+The division of work (initial fetch + additional URIs) introduces convenient
+points for resumption of an interrupted clone - such resumption can be done
+after the Minimum Viable Product (see "Future work").
+
+The client can inhibit this feature (i.e. refrain from sending the
+`packfile-uris` parameter) by passing --no-packfile-uris to `git fetch`.
+
+Future work
+---
+
+The protocol design allows some evolution of the server and client without any
+need for protocol changes, so only a small-scoped design is included here to
+form the MVP. For example, the following can be done:
+
+ * On the server, a long-running process that takes in entire requests and
+   outputs a list of URIs and the corresponding inclusion and exclusion sets of
+   objects. This allows, e.g., signed URIs to be used and packfiles for common
+   requests to be cached.
+ * On the client, resumption of clone. If a clone is interrupted, information
+   could be recorded in the repository's config and a "clone-resume" command
+   can resume the clone in progress. (Resumption of subsequent fetches is more
+   difficult because that must deal with the user wanting to use the repository
+   even after the fetch was interrupted.)
+
+There are some possible features that will require a change in protocol:
+
+ * Additional HTTP headers (e.g. authentication)
+ * Byte range support
+ * Different file formats referenced by URIs (e.g. raw object)
diff --git a/Documentation/technical/protocol-v2.txt 
b/Documentation/technical/protocol-v2.txt
index 36239ec7e9..7b63c26ecd 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -323,13 +323,26 @@ included in the client's request:
indicating its sideband (1, 2, or 3), and the server may send "0005\2"
(a PKT-LINE of sideband 2 with no payload) as a keepalive packet.
 
+If the 'packfile-uris' feature is advertised, the following argument
+can be included in the client's request as well as the potential
+addition of the 'packfile-uris' section in the server's response as
+explained below.
+
+packfile-uris 
+   Indicates to the server that the client is willing to receive
+   URIs of any of the given protocols in place of objects in the
+   sent packfile. Before performing the connectivity check, the
+   client should download from all given URIs. Currently, the
+   protocols supported are "http" and "https".
+
 Th

[PATCH v2 4/8] Documentation: order protocol v2 sections

2019-03-08 Thread Jonathan Tan
The current C Git implementation expects Git servers to follow a
specific order of sections when transmitting protocol v2 responses, but
this is not explicit in the documentation. Make the order explicit.

Signed-off-by: Jonathan Tan 
Signed-off-by: Junio C Hamano 
---
 Documentation/technical/protocol-v2.txt | 18 --
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/Documentation/technical/protocol-v2.txt 
b/Documentation/technical/protocol-v2.txt
index ead85ce35c..36239ec7e9 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -325,11 +325,11 @@ included in the client's request:
 
 The response of `fetch` is broken into a number of sections separated by
 delimiter packets (0001), with each section beginning with its section
-header.
+header. Most sections are sent only when the packfile is sent.
 
-output = *section
-section = (acknowledgments | shallow-info | wanted-refs | packfile)
- (flush-pkt | delim-pkt)
+output = acknowledgements flush-pkt |
+[acknowledgments delim-pkt] [shallow-info delim-pkt]
+[wanted-refs delim-pkt] packfile flush-pkt
 
 acknowledgments = PKT-LINE("acknowledgments" LF)
  (nak | *ack)
@@ -351,9 +351,10 @@ header.
   *PKT-LINE(%x01-03 *%x00-ff)
 
 acknowledgments section
-   * If the client determines that it is finished with negotiations
- by sending a "done" line, the acknowledgments sections MUST be
- omitted from the server's response.
+   * If the client determines that it is finished with negotiations by
+ sending a "done" line (thus requiring the server to send a packfile),
+ the acknowledgments sections MUST be omitted from the server's
+ response.
 
* Always begins with the section header "acknowledgments"
 
@@ -404,9 +405,6 @@ header.
  which the client has not indicated was shallow as a part of
  its request.
 
-   * This section is only included if a packfile section is also
- included in the response.
-
 wanted-refs section
* This section is only included if the client has requested a
  ref using a 'want-ref' line and if a packfile section is also
-- 
2.19.0.271.gfe8321ec05.dirty



bitmaps by default? [was: prune: use bitmaps for reachability traversal]

2019-03-08 Thread Eric Wong
Jeff King  wrote:
> Pruning generally has to traverse the whole commit graph in order to
> see which objects are reachable. This is the exact problem that
> reachability bitmaps were meant to solve, so let's use them (if they're
> available, of course).

Perhaps this is good impetus for doing bitmaps by default?

It would make life easier for people new to hosting git servers
(and hopefully reduce centralization :)


I started working on it, but t0410-partial-clone.sh fails with
"Failed to write bitmap index. Packfile doesn't have full
closure"; so more work needs to be done w.r.t. default behavior
on partial clones...

Here's my WIP:

diff --git a/builtin/repack.c b/builtin/repack.c
index 67f8978043..ca98d32715 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -14,7 +14,7 @@
 
 static int delta_base_offset = 1;
 static int pack_kept_objects = -1;
-static int write_bitmaps;
+static int write_bitmaps = -1;
 static int use_delta_islands;
 static char *packdir, *packtmp;
 
@@ -344,10 +344,14 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
die(_("--keep-unreachable and -A are incompatible"));
 
if (pack_kept_objects < 0)
-   pack_kept_objects = write_bitmaps;
+   pack_kept_objects = write_bitmaps > 0;
 
-   if (write_bitmaps && !(pack_everything & ALL_INTO_ONE))
-   die(_(incremental_bitmap_conflict_error));
+   if (!(pack_everything & ALL_INTO_ONE)) {
+   if (write_bitmaps > 0)
+   die(_(incremental_bitmap_conflict_error));
+   } else if (write_bitmaps < 0) {
+   write_bitmaps = 1;
+   }
 
packdir = mkpathdup("%s/pack", get_object_directory());
packtmp = mkpathdup("%s/.tmp-%d-pack", packdir, (int)getpid());
@@ -368,7 +372,7 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
argv_array_push(&cmd.args, "--indexed-objects");
if (repository_format_partial_clone)
argv_array_push(&cmd.args, "--exclude-promisor-objects");
-   if (write_bitmaps)
+   if (write_bitmaps > 0)
argv_array_push(&cmd.args, "--write-bitmap-index");
if (use_delta_islands)
argv_array_push(&cmd.args, "--delta-islands");