Re: [PATCH 4/6] config: return configset value for current_config_ functions

2016-05-26 Thread Duy Nguyen
On Thu, May 19, 2016 at 7:08 AM, Jeff King  wrote:
> On Wed, May 18, 2016 at 06:43:23PM -0400, Jeff King wrote:
>
>>  cache.h|  1 +
>>  config.c   | 51 
>> +-
>>  t/helper/test-config.c | 20 
>>  t/t1308-config-set.sh  | 23 +++
>> [...]
>> +test_expect_success 'iteration shows correct origins' '
>> + echo "[alias]test-config = !test-config" >.gitconfig &&

How about using 'which' to get absolute path for test-config and put
it here? Then we don't rely on $PATH anymore.

>> [...]
>> + git -c foo.bar=from-cmdline test-config iterate >actual &&
>
> While writing and testing this, I got bit by e6e7530 (test helpers: move
> test-* to t/helper/ subdirectory, 2016-04-13). I had an old test-config
> binary leftover in the root of my repository, and the new one was
> correctly built in t/helper/. Running "test-config" is fine, but inside
> the git alias, it sticks the repository root at the front of $PATH
> (because it's the exec-path). And so it ran the old version of
> test-config, which did not understand my new "iterate" option.
>
> Now I'll admit what I'm doing here is pretty funny (running test-* from
> an alias). I'm doing it because I want to see how the program operates
> with the "-c" config, and it's nicer to spell it as a user would,
> instead of munging $GIT_CONFIG_PARAMETERS directly.
>
> So I'm not sure if it's worth working around or not. The single tree
> state produced by this commit is fine, but it does behave badly if
> there's leftover cruft from a pre-e6e7530 build. A more robust version
> would look more like:
>
>   sq=\' ;# to ease quoting later
>   ...
>   GIT_CONFIG_PARAMETERS=${sq}foo.bar=from-cmdline${sq} test-config ...
>
> Which is ugly, but it's probably worth it to avoid the flakiness.
>
> The other option is to somehow make bin-wrappers more robust. E.g., it
> would be nice if we didn't actually point into the repository root
> directly, but rather somehow linked all of the git-* entries that
> _would_ be installed into the exec-path into a fake exec-path (or
> alternatively, actually build them directly into that fake exec-path).
>
> That's a much bigger change, though. Given how unlikely the sequence of
> steps in my test is, maybe it's better to just work around it in this
> one case.
>
> -Peff



-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC] fast-import: invalidate pack_id references after loosening

2016-05-26 Thread Eric Wong
Jeff King  wrote:
> On Wed, May 25, 2016 at 10:54:02PM +, Eric Wong wrote:
> > +   for (h = 0; h < ARRAY_SIZE(object_table); h++) {
> > +   struct object_entry *e;
> > +
> > +   for (e = object_table[h]; e; e = e->next)
> > +   if (e->pack_id == id)
> > +   e->pack_id = MAX_PACK_ID;
> > +   }



> This looks pretty straightforward. I do notice that we never shrink the
> number of items in the object table when checkpointing, and so our
> linear walk will grow ever larger. So if one were to checkpoint every
> k-th object, it makes the whole operation quadratic in the number of
> objects (actually, O(n^2/k) but k is a constant).

Good point, I'll work on a separate patch to fix it.

> That's probably OK in practice, as I think the actual pack-write already
> does a linear walk of the object table to generate the pack index. So
> presumably nobody checkpoints often enough for it to be a problem. And
> the solution would be to keep a separate list of pointers to objects for
> the current pack-id, which would trivially fix both this case and the
> one in create_index().  So we can punt on it until somebody actually
> runs into it, I think.

I might checkpoint that much and run into the problem soon :)
Too tired now; maybe in a day or two I'll be able to make sense
of C again :x
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/2] submodule-config: keep shallow recommendation around

2016-05-26 Thread Remi Galan Alfonso
Hi Stefan,

Stefan Beller  writes:
> [...]
> @ -353,6 +354,15 @@ static int parse_config(const char *var, const char 
> *value, void *data)
>  else if (parse_submodule_update_strategy(value,
>   &submodule->update_strategy) < 0)
>  die(_("invalid value for %s"), var);
> +} else if (!strcmp(item.buf, "shallow")) {
> +if (!me->overwrite &&
> + submodule->recommend_shallow != -1)

Nit: You seems to be able to keep the whole condition on the same line:

if (!me->overwrite && submodule->recommend_shallow != -1)

If you want to keep it in two line, you might want to align it:
if (!me->overwrite &&
submodule->recommend_shallow != -1)

Thanks,
Rémi
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [Bug?] log -p -W showing the whole file for a patch that adds to the end?

2016-05-26 Thread René Scharfe

Am 24.05.2016 um 20:16 schrieb Junio C Hamano:

René Scharfe  writes:


   diff: factor out match_func_rec()
   diff: handle appended chunks better with -W
   diff: ignore empty lines before added functions with -W
   diff: don't include common trailing empty lines with -W
   grep: don't extend context to trailing empty lines with -W

  grep.c| 28 --
  xdiff/xemit.c | 63 ---
  2 files changed, 82 insertions(+), 9 deletions(-)



It is curious that this much behaviour change does not need any
changes in the test scripts.  We do not have sufficient coverage,
perhaps?


Well, -W is not tested at all.  I'll include some in the next round.  No 
need to hurry, it's too late to land in 2.9.0 anyway.


René
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/2] submodule update: learn `--[no-]recommend-shallow` option

2016-05-26 Thread Remi Galan Alfonso
You forgot to update from recommend-depth to recommend-shallow

Stefan Beller  writes:
> [...]
>  'git submodule' [--quiet] init [--] [...]
>  'git submodule' [--quiet] deinit [-f|--force] (--all|[--] ...)
>  'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
> -  [-f|--force] [--rebase|--merge] [--reference ]
> -  [--depth ] [--recursive] [--jobs ] [--] [...]
> +  [--[no-]recommended-depth] [-f|--force] [--rebase|--merge]

Here...

> +--[no-]recommended-depth::
> +This option is only valid for the update command.
> +The initial clone of a submodule will use the recommended
> +`submodule..depth` as provided by the .gitmodules file.
> +

... and here.

Thanks,
Rémi
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [ANNOUNCE] Git v2.9.0-rc0

2016-05-26 Thread Johannes Schindelin
Hi Junio,

On Mon, 23 May 2016, Junio C Hamano wrote:

> An early preview release Git v2.9.0-rc0 is now available for
> testing at the usual places.

Thanks. I pushed out a tagged source-only Git for Windows v2.9.0-rc0 for
interested parties:

https://github.com/git-for-windows/git/releases/tag/v2.9.0-rc0.windows.1

>  * The test scripts for "git p4" (but not "git p4" implementation
>itself) has been updated so that they would work even on a system
>where the installed version of Python is python 3.

s/has/have/

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[RFC] Triangular Workflow: user friendly full implementation

2016-05-26 Thread Jordan DE GEA
We are working on full implementation of triangular workflow feature.
For now, the main options available are: 
 - branch..pushRemote
 - remote.pushDefault
And only setable by hands. 

As it can be difficult to understand, here is what we want to do. 


Context:
- One main remote repository, e.g. git/git.
- A remote fork (e.g. a GitHub fork) of git/git, e.g. me/git.
- A local clone of me/git on the machine

Purposes:
- the local branch master has to fetch to git/git by default
- the local branch master has to push to me/git by default

Configuration wanted:
- Add a remote to git/git e.g. `git remote add ...`
- Set the fetch remote for branch as default. 

For now, we can do that by setting: 
- branch..remote to git/git
- branch..pushRemote to me/git
But many options set `branch..remote`, a suitable solution is to
implement an option for the fetch for example. 


Here is what we want to implement: 

1. 
a. add the config var: remote.fetchDefault
b. add the config var: branch..fetchRemote
c. add `git fetch --set-default` in order to set remote.fetchDefault
d. add `git fetch --set-remote` in order to set 
branch..fetchRemote
e. add `git pull --set-default` in order to set remote.fetchDefault
f. add `git pull --set-remote` in order to set branch..fetchRemote

2. 
a. add `git push --set-default` in order to set remote.pushDefault
b. add `git push --set-remote` in order to set branch..pushRemote


What's your opinion about this feature ?




--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC] Triangular Workflow: user friendly full implementation

2016-05-26 Thread Matthieu Moy
Jordan DE GEA  writes:

> Here is what we want to implement: 

Your message contains the word "implement" too many times. Before
thinking about implementation, think, and discuss, about the design.

If your message is intended to be a discussion on the design, then avoid
using "implementation" in the subject.

> 1. 
>   a. add the config var: remote.fetchDefault
>   b. add the config var: branch..fetchRemote
>   c. add `git fetch --set-default` in order to set remote.fetchDefault
>   d. add `git fetch --set-remote` in order to set 
> branch..fetchRemote
>   e. add `git pull --set-default` in order to set remote.fetchDefault
>   f. add `git pull --set-remote` in order to set branch..fetchRemote
>
> 2. 
>   a. add `git push --set-default` in order to set remote.pushDefault
>   b. add `git push --set-remote` in order to set branch..pushRemote

This tells a lot about the "what?", but lacks the most important "why?"
(or "what for?") question.

The user doesn't want to "set configuration variables". He wants to work
with git, have commands do the right thing, avoid typing useless
keystrokes and avoid surprising behavior. Please explain how your
proposal would improve the situation. For example, show a typical
use-case with commands to type before and after your proposal is
implemented.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v4 1/2] rev-parse tests: add tests executed from a subdirectory

2016-05-26 Thread Michael Rappazzo
t2027-worktree-list has an incorrect expectation for --git-common-dir
which has been adjusted and marked to expect failure.

Some of the tests added have been marked to expect failure.  These
demonstrate a problem with the way that some options to git rev-parse
behave when executed from a subdirectory of the main worktree.

Signed-off-by: Michael Rappazzo 
---
 t/t1500-rev-parse.sh | 28 
 t/t1700-split-index.sh   | 17 +
 t/t2027-worktree-list.sh | 12 ++--
 3 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 038e24c..f39f783 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -87,4 +87,32 @@ test_rev_parse -C work -g ../repo.git -b t 
'GIT_DIR=../repo.git, core.bare = tru
 
 test_rev_parse -C work -g ../repo.git -b u 'GIT_DIR=../repo.git, core.bare 
undefined' false false true ''
 
+test_expect_success 'git-common-dir from worktree root' '
+   echo .git >expect &&
+   git rev-parse --git-common-dir >actual &&
+   test_cmp expect actual
+'
+
+test_expect_failure 'git-common-dir inside sub-dir' '
+   mkdir -p path/to/child &&
+   test_when_finished "rm -rf path" &&
+   echo "$(git -C path/to/child rev-parse --show-cdup).git" >expect &&
+   git -C path/to/child rev-parse --git-common-dir >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'git-path from worktree root' '
+   echo .git/objects >expect &&
+   git rev-parse --git-path objects >actual &&
+   test_cmp expect actual
+'
+
+test_expect_failure 'git-path inside sub-dir' '
+   mkdir -p path/to/child &&
+   test_when_finished "rm -rf path" &&
+   echo "$(git -C path/to/child rev-parse --show-cdup).git/objects" 
>expect &&
+   git -C path/to/child rev-parse --git-path objects >actual &&
+   test_cmp expect actual
+'
+
 test_done
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 8aef49f..8ca21bd 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -200,4 +200,21 @@ EOF
test_cmp expect actual
 '
 
+test_expect_failure 'rev-parse --shared-index-path' '
+   rm -rf .git &&
+   test_create_repo . &&
+   git update-index --split-index &&
+   ls -t .git/sharedindex* | tail -n 1 >expect &&
+   git rev-parse --shared-index-path >actual &&
+   test_cmp expect actual &&
+   mkdir work &&
+   test_when_finished "rm -rf work" &&
+   (
+   cd work &&
+   ls -t ../.git/sharedindex* | tail -n 1 >expect &&
+   git rev-parse --shared-index-path >actual &&
+   test_cmp expect actual
+   )
+'
+
 test_done
diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index 1b1b65a..53cc5d3 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -8,16 +8,24 @@ test_expect_success 'setup' '
test_commit init
 '
 
-test_expect_success 'rev-parse --git-common-dir on main worktree' '
+test_expect_failure 'rev-parse --git-common-dir on main worktree' '
git rev-parse --git-common-dir >actual &&
echo .git >expected &&
test_cmp expected actual &&
mkdir sub &&
git -C sub rev-parse --git-common-dir >actual2 &&
-   echo sub/.git >expected2 &&
+   echo ../.git >expected2 &&
test_cmp expected2 actual2
 '
 
+test_expect_failure 'rev-parse --git-path objects linked worktree' '
+   echo "$(git rev-parse 
--show-toplevel)/.git/worktrees/linked-tree/objects" >expect &&
+   test_when_finished "rm -rf linked-tree && git worktree prune" &&
+   git worktree add --detach linked-tree master &&
+   git -C linked-tree rev-parse --git-path objects >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success '"list" all worktrees from main' '
echo "$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) 
[$(git symbolic-ref --short HEAD)]" >expect &&
test_when_finished "rm -rf here && git worktree prune" &&
-- 
2.8.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v4 2/2] rev-parse: fix some options when executed from subpath of main tree

2016-05-26 Thread Michael Rappazzo
Executing `git-rev-parse` with `--git-common-dir`, `--git-path `,
or `--shared-index-path` from the root of the main worktree results in
a relative path to the git dir.

When executed from a subdirectory of the main tree, it can incorrectly
return a path which starts 'sub/path/.git'.  Change this to return the
proper relative path to the git directory.

Related tests marked to expect failure are updated to expect success

Signed-off-by: Michael Rappazzo 
---
 builtin/rev-parse.c  | 22 +-
 t/t1500-rev-parse.sh |  4 ++--
 t/t1700-split-index.sh   |  2 +-
 t/t2027-worktree-list.sh |  4 ++--
 4 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index c961b74..40579e0 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -564,10 +564,15 @@ int cmd_rev_parse(int argc, const char **argv, const char 
*prefix)
}
 
if (!strcmp(arg, "--git-path")) {
+   struct strbuf sb = STRBUF_INIT;
+   char *path;
if (!argv[i + 1])
die("--git-path requires an argument");
-   puts(git_path("%s", argv[i + 1]));
-   i++;
+
+   path = xstrfmt("%s/%s", get_git_dir(), argv[++i]);
+   puts(relative_path(path, prefix, &sb));
+   strbuf_release(&sb);
+   free(path);
continue;
}
if (as_is) {
@@ -787,8 +792,9 @@ int cmd_rev_parse(int argc, const char **argv, const char 
*prefix)
continue;
}
if (!strcmp(arg, "--git-common-dir")) {
-   const char *pfx = prefix ? prefix : "";
-   puts(prefix_filename(pfx, strlen(pfx), 
get_git_common_dir()));
+   struct strbuf sb = STRBUF_INIT;
+   puts(relative_path(get_git_common_dir(), 
prefix, &sb));
+   strbuf_release(&sb);
continue;
}
if (!strcmp(arg, "--is-inside-git-dir")) {
@@ -811,7 +817,13 @@ int cmd_rev_parse(int argc, const char **argv, const char 
*prefix)
die(_("Could not read the index"));
if (the_index.split_index) {
const unsigned char *sha1 = 
the_index.split_index->base_sha1;
-   puts(git_path("sharedindex.%s", 
sha1_to_hex(sha1)));
+   struct strbuf sb = STRBUF_INIT;
+   char *path;
+
+   path = xstrfmt("%s/sharedindex.%s", 
get_git_dir(), sha1_to_hex(sha1));
+   puts(relative_path(path, prefix, &sb));
+   strbuf_release(&sb);
+   free(path);
}
continue;
}
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index f39f783..d74f09a 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -93,7 +93,7 @@ test_expect_success 'git-common-dir from worktree root' '
test_cmp expect actual
 '
 
-test_expect_failure 'git-common-dir inside sub-dir' '
+test_expect_success 'git-common-dir inside sub-dir' '
mkdir -p path/to/child &&
test_when_finished "rm -rf path" &&
echo "$(git -C path/to/child rev-parse --show-cdup).git" >expect &&
@@ -107,7 +107,7 @@ test_expect_success 'git-path from worktree root' '
test_cmp expect actual
 '
 
-test_expect_failure 'git-path inside sub-dir' '
+test_expect_success 'git-path inside sub-dir' '
mkdir -p path/to/child &&
test_when_finished "rm -rf path" &&
echo "$(git -C path/to/child rev-parse --show-cdup).git/objects" 
>expect &&
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 8ca21bd..d2d9e02 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -200,7 +200,7 @@ EOF
test_cmp expect actual
 '
 
-test_expect_failure 'rev-parse --shared-index-path' '
+test_expect_success 'rev-parse --shared-index-path' '
rm -rf .git &&
test_create_repo . &&
git update-index --split-index &&
diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index 53cc5d3..16eec6e 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -8,7 +8,7 @@ test_expect_success 'setup' '
test_commit init
 '
 
-test_expect_failure 'rev-parse --git-common-dir on main worktree' '
+test_expect_success 'rev-parse --git-common-dir on main worktree' '
git rev-parse --git-common-dir >actual &&
echo .git >expe

[PATCH v4 0/2] rev-parse: fix some options when executed from subpath of main tree

2016-05-26 Thread Michael Rappazzo
Changes since v3 [1]:
 - Rebased onto 'pu' which includes the cleanup of t1500 by Eric Sunshine
 - Fixed a memory leak due to misusing xstrfmt()

[1] http://thread.gmane.org/gmane.comp.version-control.git/293778

Michael Rappazzo (2):
  rev-parse tests: add tests executed from a subdirectory
  rev-parse: fix some options when executed from subpath of main tree

 builtin/rev-parse.c  | 22 +-
 t/t1500-rev-parse.sh | 28 
 t/t1700-split-index.sh   | 17 +
 t/t2027-worktree-list.sh | 10 +-
 4 files changed, 71 insertions(+), 6 deletions(-)

-- 
2.8.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH] worktree: allow "-" short-hand for @{-1} in add command

2016-05-26 Thread Jordan DE GEA
From: Jordan DE GEA 

Since `git worktree add` uses `git checkout` when `[]` is used,
and `git checkout -` is already supported, it makes sense to allow the
same shortcut in `git worktree add`.

Signed-off-by: Matthieu Moy 
Signed-off-by: Jordan DE GEA 
---
 Documentation/git-worktree.txt |  3 ++-
 builtin/worktree.c |  3 +++
 t/t2025-worktree-add.sh| 18 ++
 3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index c622345..48e5fdf 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -48,7 +48,8 @@ add  []::
 
 Create `` and checkout `` into it. The new working directory
 is linked to the current repository, sharing everything except working
-directory specific files such as HEAD, index, etc.
+directory specific files such as HEAD, index, etc. The last branch you 
+were on can be specify with `-` as `` which is synonymous with 
`"@{-1}"`.
 +
 If `` is omitted and neither `-b` nor `-B` nor `--detached` used,
 then, as a convenience, a new branch based at HEAD is created automatically,
diff --git a/builtin/worktree.c b/builtin/worktree.c
index d8e3795..d800d47 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -340,6 +340,9 @@ static int add(int ac, const char **av, const char *prefix)
path = prefix ? prefix_filename(prefix, strlen(prefix), av[0]) : av[0];
branch = ac < 2 ? "HEAD" : av[1];
 
+   if (!strcmp(branch, "-"))
+   branch = "@{-1}";
+
opts.force_new_branch = !!new_branch_force;
if (opts.force_new_branch) {
struct strbuf symref = STRBUF_INIT;
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 3acb992..b713efb 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -18,6 +18,24 @@ test_expect_success '"add" an existing empty worktree' '
git worktree add --detach existing_empty master
 '
 
+test_expect_success '"add" using shorthand - fails when no previous branch' '
+   test_must_fail git worktree add existing -
+'
+
+test_expect_success '"add" using - shorthand' '
+   git checkout -b newbranch &&
+   echo hello >myworld &&
+   git add myworld &&
+   git commit -m myworld &&
+   git checkout master &&
+   git worktree add short-hand - &&
+   cd short-hand &&
+   test $(git rev-parse --symbolic-full-name HEAD) = "refs/heads/newbranch"
+   branch=$(cd short-hand && git rev-parse --symbolic-full-name HEAD) &&
+   test "$branch" = refs/heads/newbranch &&
+   cd ..
+'
+
 test_expect_success '"add" refuses to checkout locked branch' '
test_must_fail git worktree add zere master &&
! test -d zere &&
-- 
2.7.4 (Apple Git-66)

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[RFC/PATCH 0/2] Introduce "log.showSignature" config variable

2016-05-26 Thread Mehul Jain
Add a new configuratation variable "log.showSignature" for git-log and
git-show. "log.showSignature=true" will enable user to see GPG signature
by default while using git-log and git-show.

[Patch 1/2] introduce the config variable along with some tests.
[Patch 2/2] tackles the problem: what if user wants to disable the
setting of "log.showSignature=true" using a command line
switch.

Previous discussion on this: 
http://thread.gmane.org/gmane.comp.version-control.git/295405

Mehul Jain (2):
  log: add "log.showsignature" configuration variable
  log: add "--no-show-signature" command line option

 Documentation/git-log.txt |  4 
 builtin/log.c |  6 ++
 revision.c|  2 ++
 t/t4202-log.sh| 26 ++
 t/t7510-signed-commit.sh  |  7 +++
 5 files changed, 45 insertions(+)

-- 
2.9.0.rc0.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[RFC/PATCH 1/2] log: add "log.showsignature" configuration variable

2016-05-26 Thread Mehul Jain
People may want to always use "--show-signature" while using "git log"
or "git show".

When log.showsignature set true, "git log" and "git show" will behave
as "--show-signature" was given to them.

Signed-off-by: Mehul Jain 
---
 Documentation/git-log.txt |  4 
 builtin/log.c |  6 ++
 t/t4202-log.sh| 19 +++
 t/t7510-signed-commit.sh  |  7 +++
 4 files changed, 36 insertions(+)

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 03f9580..f39f800 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -196,6 +196,10 @@ log.showRoot::
`git log -p` output would be shown without a diff attached.
The default is `true`.
 
+log.showSignature::
+   If `true`, `git log` and `git show` will act as if `--show-signature`
+   option was passed to them.
+
 mailmap.*::
See linkgit:git-shortlog[1].
 
diff --git a/builtin/log.c b/builtin/log.c
index 099f4f7..7103217 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -33,6 +33,7 @@ static const char *default_date_mode = NULL;
 static int default_abbrev_commit;
 static int default_show_root = 1;
 static int default_follow;
+static int default_show_signature;
 static int decoration_style;
 static int decoration_given;
 static int use_mailmap_config;
@@ -119,6 +120,7 @@ static void cmd_log_init_defaults(struct rev_info *rev)
rev->abbrev_commit = default_abbrev_commit;
rev->show_root_diff = default_show_root;
rev->subject_prefix = fmt_patch_subject_prefix;
+   rev->show_signature = default_show_signature;
DIFF_OPT_SET(&rev->diffopt, ALLOW_TEXTCONV);
 
if (default_date_mode)
@@ -409,6 +411,10 @@ static int git_log_config(const char *var, const char 
*value, void *cb)
use_mailmap_config = git_config_bool(var, value);
return 0;
}
+   if (!strcmp(var, "log.showsignature")) {
+   default_show_signature = git_config_bool(var, value);
+   return 0;
+   }
 
if (grep_config(var, value, cb) < 0)
return -1;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 128ba93..36be9a1 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -890,6 +890,25 @@ test_expect_success GPG 'log --graph --show-signature for 
merged tag' '
grep "^| | gpg: Good signature" actual
 '
 
+test_expect_success GPG 'log.showsignature=true behaves like --show-signature' 
'
+   git checkout -b test_sign master &&
+   echo foo >foo &&
+   git add foo &&
+   git commit -S -m signed_commit &&
+   test_config log.showsignature true &&
+   git log -1 signed >actual &&
+   test_i18ngrep "gpg: Signature made" actual &&
+   test_i18ngrep "gpg: Good signature" actual
+'
+
+test_expect_success GPG '--show-signature overrides log.showsignature=false' '
+   test_when_finished "git reset --hard && git checkout master" &&
+   git config log.showsignature false &&
+   git log -1 --show-signature signed >actual &&
+   test_i18ngrep "gpg: Signature made" actual &&
+   test_i18ngrep "gpg: Good signature" actual
+'
+
 test_expect_success 'log --graph --no-walk is forbidden' '
test_must_fail git log --graph --no-walk
 '
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 4177a86..326dcc8 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -210,4 +210,11 @@ test_expect_success GPG 'show lack of signature with 
custom format' '
test_cmp expect actual
 '
 
+test_expect_success GPG 'log.showsignature behaves like --show-signature' '
+   git config log.showsignature true &&
+   git show initial > actual &&
+   test_i18ngrep "gpg: Signature made" actual &&
+   test_i18ngrep "gpg: Good signature" actual
+'
+
 test_done
-- 
2.9.0.rc0.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[RFC/PATCH 2/2] log: add "--no-show-signature" command line option

2016-05-26 Thread Mehul Jain
If "log.showsignature=true", then there is no way to override it using
command line switch.

Teach "git log" and "git show" about "--no-show-signature" command line
option.

Signed-off-by: Mehul Jain 
---
 revision.c | 2 ++
 t/t4202-log.sh | 7 +++
 2 files changed, 9 insertions(+)

diff --git a/revision.c b/revision.c
index d30d1c4..3546ff9 100644
--- a/revision.c
+++ b/revision.c
@@ -1871,6 +1871,8 @@ static int handle_revision_opt(struct rev_info *revs, int 
argc, const char **arg
revs->notes_opt.use_default_notes = 1;
} else if (!strcmp(arg, "--show-signature")) {
revs->show_signature = 1;
+   } else if (!strcmp(arg, "--no-show-signature")) {
+   revs->show_signature = 0;
} else if (!strcmp(arg, "--show-linear-break") ||
   starts_with(arg, "--show-linear-break=")) {
if (starts_with(arg, "--show-linear-break="))
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 36be9a1..ea24259 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -901,6 +901,13 @@ test_expect_success GPG 'log.showsignature=true behaves 
like --show-signature' '
test_i18ngrep "gpg: Good signature" actual
 '
 
+test_expect_success GPG '--no-show-signature overrides log.showsignature=true' 
'
+   git config log.showsignature true &&
+   git log -1 --no-show-signature signed >actual &&
+   test "$(test_i18ngrep "gpg: Signature made" actual)" = "" &&
+   test "$(test_i18ngrep "gpg: Good signature" actual)" = ""
+'
+
 test_expect_success GPG '--show-signature overrides log.showsignature=false' '
test_when_finished "git reset --hard && git checkout master" &&
git config log.showsignature false &&
-- 
2.9.0.rc0.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [ANNOUNCE] Git v2.9.0-rc0

2016-05-26 Thread Johannes Schindelin
Hi all,

On Thu, 26 May 2016, Johannes Schindelin wrote:

> On Mon, 23 May 2016, Junio C Hamano wrote:
> 
> > An early preview release Git v2.9.0-rc0 is now available for
> > testing at the usual places.
> 
> Thanks. I pushed out a tagged source-only Git for Windows v2.9.0-rc0 for
> interested parties:
> 
>   https://github.com/git-for-windows/git/releases/tag/v2.9.0-rc0.windows.1

Very sorry: the tagging revealed a flaw in one of the patches and I had to
re-tag. My apologies to anybody who already fetched the previous tag.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v4 2/2] rev-parse: fix some options when executed from subpath of main tree

2016-05-26 Thread Mike Hommey
On Thu, May 26, 2016 at 07:19:16AM -0400, Michael Rappazzo wrote:
> Executing `git-rev-parse` with `--git-common-dir`, `--git-path `,
> or `--shared-index-path` from the root of the main worktree results in
> a relative path to the git dir.
> 
> When executed from a subdirectory of the main tree, it can incorrectly
> return a path which starts 'sub/path/.git'.  Change this to return the
> proper relative path to the git directory.
> 
> Related tests marked to expect failure are updated to expect success

As mentioned previously (but late in the thread), I don't get why this
one case of --git-common-dir should not return the same thing as
--git-dir, which is an absolute directory. Especially when there is
other flag telling you whether you are in the main or another worktree,
so comparing the output for --git-dir and --git-common-dir is the
easiest way to do so, but then you have to normalize them on their own
because git returns different values pointing to the same directory.

Mike
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] mingw: make isatty() recognize MSYS2's pseudo terminals (/dev/pty*)

2016-05-26 Thread Johannes Schindelin
Hi Junio,

On Wed, 27 Apr 2016, Johannes Schindelin wrote:

> From: Karsten Blees 
> 
> MSYS2 emulates pseudo terminals via named pipes, and isatty() returns 0
> for such file descriptors. Therefore, some interactive functionality
> (such as launching a pager, asking if a failed unlink should be repeated
> etc.) doesn't work when run in a terminal emulator that uses MSYS2's
> ptys (such as mintty).
> 
> However, MSYS2 uses special names for its pty pipes ('msys-*-pty*'),
> which allows us to distinguish them from normal piped input / output.
> 
> On startup, check if stdin / stdout / stderr are connected to such pipes
> using the NtQueryObject API from NTDll.dll. If the names match, adjust
> the flags in MSVCRT's ioinfo structure accordingly.
> 
> Signed-off-by: Karsten Blees 
> Signed-off-by: Johannes Schindelin 

For the record, this patch is needed to make the output of `git log` *not*
whizz by when running in MinTTY, the default terminal of Git for Windows
and MSYS2.

I do not see this patch in 'pu'... Anything I can do to get this into
'master' eventually?

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC/PATCH 1/2] log: add "log.showsignature" configuration variable

2016-05-26 Thread Remi Galan Alfonso
Hi Mehul,

Mehul Jain  writes:
> People may want to always use "--show-signature" while using "git log"
> or "git show".
> 
> When log.showsignature set true, "git log" and "git show" will behave

'When log.showsignature is set to true' ?

> as "--show-signature" was given to them.

s/as/as if

> Signed-off-by: Mehul Jain 
> ---
>  Documentation/git-log.txt |  4 
>  builtin/log.c |  6 ++
>  t/t4202-log.sh| 19 +++
>  t/t7510-signed-commit.sh  |  7 +++
>  4 files changed, 36 insertions(+)
> [...]
> [...]
> +test_expect_success GPG 'log.showsignature=true behaves like 
> --show-signature' '
> +git checkout -b test_sign master &&
> +echo foo >foo &&
> +git add foo &&
> +git commit -S -m signed_commit &&
> +test_config log.showsignature true &&
> +git log -1 signed >actual &&
> +test_i18ngrep "gpg: Signature made" actual &&
> +test_i18ngrep "gpg: Good signature" actual
> +'
> +
> +test_expect_success GPG '--show-signature overrides log.showsignature=false' 
> '
> +test_when_finished "git reset --hard && git checkout master" &&
> +git config log.showsignature false &&

Any specific reason as to why you don't use test_config like in the
first test?

> +git log -1 --show-signature signed >actual &&
> +test_i18ngrep "gpg: Signature made" actual &&
> +test_i18ngrep "gpg: Good signature" actual
> +'
> +
>  test_expect_success 'log --graph --no-walk is forbidden' '
>  test_must_fail git log --graph --no-walk
>  '
> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
> index 4177a86..326dcc8 100755
> --- a/t/t7510-signed-commit.sh
> +++ b/t/t7510-signed-commit.sh
> @@ -210,4 +210,11 @@ test_expect_success GPG 'show lack of signature with 
> custom format' '
>  test_cmp expect actual
>  '
>  
> +test_expect_success GPG 'log.showsignature behaves like --show-signature' '
> +git config log.showsignature true &&

Same here.

> +git show initial > actual &&

Style: no space after redirection.

Thanks,
Rémi
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC] fast-import: invalidate pack_id references after loosening

2016-05-26 Thread Jeff King
On Thu, May 26, 2016 at 08:02:36AM +, Eric Wong wrote:

> > That's probably OK in practice, as I think the actual pack-write already
> > does a linear walk of the object table to generate the pack index. So
> > presumably nobody checkpoints often enough for it to be a problem. And
> > the solution would be to keep a separate list of pointers to objects for
> > the current pack-id, which would trivially fix both this case and the
> > one in create_index().  So we can punt on it until somebody actually
> > runs into it, I think.
> 
> I might checkpoint that much and run into the problem soon :)
> Too tired now; maybe in a day or two I'll be able to make sense
> of C again :x

In theory the list of objects in the allocation pool are contiguous for
a particular pack. So you could break out of the double-loop at the
first non-matching object you see:

diff --git a/fast-import.c b/fast-import.c
index 83558dc..f214bd3 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -900,10 +900,13 @@ static const char *create_index(void)
c = idx;
for (o = blocks; o; o = o->next_pool)
for (e = o->next_free; e-- != o->entries;)
if (pack_id == e->pack_id)
*c++ = &e->idx;
+   else
+   goto done;
+done:
last = idx + object_count;
if (c != last)
die("internal consistency error creating the index");
 
tmpfile = write_idx_file(NULL, idx, object_count, &pack_idx_opts, 
pack_data->sha1);

But that seems to break some of the tests, so I think there is some case
which does not match my theory. :)

Another strategy is to note that we cross-check against object_count
here. So you could simply break out of the loop when we have found
object_count matches.

We don't keep similar counters for branches/tags (which have a similar
quadratic problem, though presumably much smaller "n"), but it would be
easy to keep an offset in start_packfile() and just start the iteration
there.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/2] fetch: better alignment in ref summary

2016-05-26 Thread Marc Branchaud

On 2016-05-22 09:59 PM, Duy Nguyen wrote:

On Mon, May 23, 2016 at 7:58 AM, Junio C Hamano  wrote:

That is, I wonder if the above can become something like:


 From github.com:pclouds/git
  * [new branch]  { -> pclouds/}2nd-index
  * [new branch]  { -> pclouds/}3nd-index
  * [new branch]  { -> pclouds/}file-watcher
  ...


The above example borrows the idea used in diffstat label for
renaming patch and I think you can design a better notataion, but a
big point is that you can shorten the whole thing by not repeating
the common part twice.  The arrow aligns merely as a side effect of
the shortening, taking advantage of the fact that most people fetch
with "$their_prefix/*:$our_prefix/*" renaming refspec.


I did think of that. My only concern is, with the new output I can't
simply copy the ref name (e.g. pclouds/2nd-index) and use it to, say,
examine with git-log. But I'm more of a "tab tab tab" person than
"copy and paste", so I don't know how often people need to do that.


Why do we need any kind of "->" at all?  How about simply (with an 
update to "old-branch" for comparison to probably-more-common output):


From github.com:pclouds/git
   cafed0c..badfeed  pclouds/old-branch
 * [new branch]  pclouds/2nd-index
 * [new branch]  pclouds/3nd-index
 * [new branch]  pclouds/file-watcher

M.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] transport, send-pack: append period to up-to-date message

2016-05-26 Thread Yong Bakos
On May 25, 2016, at 5:55 PM, Junio C Hamano  wrote:
> 
> Jeff King  writes:
> 
>> I think messages to stderr are generally fair game for changing, even in
>> plumbing. In many cases they are also translated (and I would argue that
>> these messages probably should be translated, too).
> 
> I think I agree.  My first reaction to this thread indeed was "Why
> isn't this marked for translation?"; as to the change proposed by
> the patch itself, my reaction actually was "Meh" ;-)

Aw come one, Junio, you mean one "." doesn't excite you? :)

> 
>> That being said, CodingGuidelines says:
>> 
>>   - Do not end error messages with a full stop.
> 
> Thanks for pointing it out---I forgot about that one.
> 
> I do not think there was a concrete reason why they should not end
> with a full stop, other than "be consistent with existing ones that
> do not end with a full stop", though.
> 
>> This isn't an error message exactly, but I think it's in the same vein.
>> I will note that we have not historically been consistent here, though
>> (as evidenced by the noted message in git-merge).

Indeed, most of the output during a pull/merge/push workflow breaks the
"don't end with a full stop" guideline. I believe that this patch,
despite its candidacy for the "most insignificant patch award," is a
sane one.

Thanks for reviewing,
yong


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC/PATCH 1/2] log: add "log.showsignature" configuration variable

2016-05-26 Thread Mehul Jain
Hi Remi,

Thanks for your input.

On Thu, May 26, 2016 at 7:12 PM, Remi Galan Alfonso
 wrote:
> Hi Mehul,
>
> Mehul Jain  writes:
>> When log.showsignature set true, "git log" and "git show" will behave
>
> 'When log.showsignature is set to true' ?

Pardon me, but I don't understand your question.
I think you are suggesting me to write
"When log.showsignature is set to true"
instead of
"When log.showsignature set true".

>> +test_expect_success GPG '--show-signature overrides 
>> log.showsignature=false' '
>> +test_when_finished "git reset --hard && git checkout master" &&
>> +git config log.showsignature false &&
>
> Any specific reason as to why you don't use test_config like in the
> first test?

None, actually. It was just that I forgot to use test_config while
writing the tests. I will make changes  accordingly as test_config
automatically unset the config variable, which is necessary.

Thanks,
Mehul
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC/PATCH 1/2] log: add "log.showsignature" configuration variable

2016-05-26 Thread Remi Galan Alfonso
Mehul Jain  writes:
> Hi Remi,
> 
> Thanks for your input.
> 
> On Thu, May 26, 2016 at 7:12 PM, Remi Galan Alfonso
>  wrote:
> > Hi Mehul,
> >
> > Mehul Jain  writes:
> >> When log.showsignature set true, "git log" and "git show" will behave
> >
> > 'When log.showsignature is set to true' ?
> 
> Pardon me, but I don't understand your question.
> I think you are suggesting me to write
> "When log.showsignature is set to true"
> instead of
> "When log.showsignature set true".

Sorry, I should have made explicit what went through my mind.
"When log.showsignature set true" doesn't sound right to me, while
"When log.showsignature is set to true" sounds better, however not
being a native english speaker maybe it's just me being wrong.

Thanks,
Rémi
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC/PATCH 1/2] log: add "log.showsignature" configuration variable

2016-05-26 Thread Mehul Jain
On Thu, May 26, 2016 at 9:13 PM, Remi Galan Alfonso
 wrote:
> Sorry, I should have made explicit what went through my mind.
> "When log.showsignature set true" doesn't sound right to me, while
> "When log.showsignature is set to true" sounds better, however not
> being a native english speaker maybe it's just me being wrong.

I think "When log.showsignature is set to true" is better.
The one that I phrased does sound bit strange. I also
not being a native English speaker, have a history of making
grammatical mistakes. :)

Thanks,
Mehul
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC/PATCH] Formatting variables in the documentation

2016-05-26 Thread Junio C Hamano
Jeff King  writes:

> On Mon, May 23, 2016 at 07:57:43PM +0200, Matthieu Moy wrote:
>
>> Samuel GROOT  writes:
>> 
>> > Since 2.8.3 was out recently, we could flip MAN_BOLD_LITERAL on by
>> > default for this cycle to shake out problems as Jeff King suggested
>> > [2].
>> 
>> 2.8.3 was a bufix release, and flipping a controversial flag should
>> clearly not be done on a bugfix release. So, in this context, "beginning
>> of a cycle" means after a x.y.0 release.
>> 
>> Anyway, a patch enabling MAN_BOLD_LITERAL by default would need to cook
>> in pu and next as any other patches, so the time when the patch is sent
>> does not really matter.
>
> Yeah, I think a reasonable plan is:
>
>   1. Somebody produces a patch flipping the default. The patch is
>  trivial, but the commit message should tell why, and try to dig up
>  any possible problems we might see (e.g., why wasn't this the
>  default? Particular versions of tools? Some platforms?)

"git log -SBOLD_LITERAL Documentation/" tells me that it's not like
we had this on by default sometime in the past and then we flipped
the default back in response to some problems (which I forgot
about).

I just re-read the two iterations that introduced BOLD_LITERAL:

 * 
http://news.gmane.org/find-root.php?message_id=<1237881866-5497-1-git-send-email-chris_john...@pobox.com>

 * 
http://news.gmane.org/find-root.php?message_id=<1238136245-22853-1-git-send-email-chris_john...@pobox.com>

There was no particular "caveat" raised there to recommend against
using this on particular versions of tools or platforms.  It was
inertia that has kept the new optional feature "optional".

>   2. Assuming no problems, Junio merges the patch to "next". We get
>  any reports of issues from people using "next" day-to-day.

So I can do these steps myself up to this point.  After waiting for
a few days to see if somebody else with better memory tells me what
I forgot, perhaps.

>   3. Assuming no problems, Junio merges to "master". We hit more people
>  (who build from master). And also it would be part of the
>  pre-generated pages that Junio ships, so we might get reports
>  there.
>
>   4. Eventually it's released. We hope to get no problem reports there,
>  though it _does_ hit a wider audience at that point.
>
> Steps 1 and 2 can happen now. As we are in the -rc cycle right now,
> probably step 3 would happen post-v2.9. But there's no reason not to
> start the clock ticking now.
>
> -Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC/PATCH] Formatting variables in the documentation

2016-05-26 Thread Jeff King
On Thu, May 26, 2016 at 09:18:17AM -0700, Junio C Hamano wrote:

> >   1. Somebody produces a patch flipping the default. The patch is
> >  trivial, but the commit message should tell why, and try to dig up
> >  any possible problems we might see (e.g., why wasn't this the
> >  default? Particular versions of tools? Some platforms?)
> [...]
> There was no particular "caveat" raised there to recommend against
> using this on particular versions of tools or platforms.  It was
> inertia that has kept the new optional feature "optional".

Thanks for digging. That matches my recollection and the limited
research I did more recently.

> >   2. Assuming no problems, Junio merges the patch to "next". We get
> >  any reports of issues from people using "next" day-to-day.
> 
> So I can do these steps myself up to this point.  After waiting for
> a few days to see if somebody else with better memory tells me what
> I forgot, perhaps.

OK. I was trying to see if (1) could be low-hanging fruit for any of the
newcomers, but at this point it probably makes sense for you to just
write the patch.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/2] fetch: better alignment in ref summary

2016-05-26 Thread Jeff King
On Thu, May 26, 2016 at 10:22:25AM -0400, Marc Branchaud wrote:

> Why do we need any kind of "->" at all?  How about simply (with an update to
> "old-branch" for comparison to probably-more-common output):
> 
> From github.com:pclouds/git
>cafed0c..badfeed  pclouds/old-branch
>  * [new branch]  pclouds/2nd-index
>  * [new branch]  pclouds/3nd-index
>  * [new branch]  pclouds/file-watcher

That covers the common case of:

  refs/heads/*:refs/remotes/pclouds/*

but sometimes the remote and local names are not the same, and the
mapping is interesting. Like:

  $ git fetch origin refs/pull/*/head:refs/remotes/pr/*
  ...
   * [new ref] refs/pull/77/head -> pr/77

Or even:

  $ git fetch origin refs/pull/77/head refs/pull/78/head
  From ...
   * branchrefs/pull/77/head -> FETCH_HEAD
   * branchrefs/pull/78/head -> FETCH_HEAD

So I think we need a scheme that can show the interesting mappings, but
collapses to something simple for the common case.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC/PATCH 2/2] log: add "--no-show-signature" command line option

2016-05-26 Thread Jeff King
On Thu, May 26, 2016 at 06:36:47PM +0530, Mehul Jain wrote:

> If "log.showsignature=true", then there is no way to override it using
> command line switch.
> 
> Teach "git log" and "git show" about "--no-show-signature" command line
> option.

I think this is teaching all of the revision machinery about it (which
is a good thing).

> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 36be9a1..ea24259 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -901,6 +901,13 @@ test_expect_success GPG 'log.showsignature=true behaves 
> like --show-signature' '
>   test_i18ngrep "gpg: Good signature" actual
>  '
>  
> +test_expect_success GPG '--no-show-signature overrides 
> log.showsignature=true' '
> + git config log.showsignature true &&
> + git log -1 --no-show-signature signed >actual &&
> + test "$(test_i18ngrep "gpg: Signature made" actual)" = "" &&
> + test "$(test_i18ngrep "gpg: Good signature" actual)" = ""
> +'

Perhaps it would be more robust to simply grep for "gpg:". We should not
be seeing any gpg-related lines in the output. It probably isn't that
big a deal in practice, though. If the output from gpg changes, this
test could report a false success, but all of the other nearby tests
would show a breakage, so somebody would probably notice.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC/PATCH] Formatting variables in the documentation

2016-05-26 Thread Jeff King
On Thu, May 26, 2016 at 09:37:19AM -0700, Junio C Hamano wrote:

> >> There was no particular "caveat" raised there to recommend against
> >> using this on particular versions of tools or platforms.  It was
> >> inertia that has kept the new optional feature "optional".
> >
> > Thanks for digging. That matches my recollection and the limited
> > research I did more recently.
> 
> For completeness's sake I should point out that the discussion on
> the first thread did point out some version-dependent issues.  But
> with 79c461d5 (docs: default to more modern toolset, 2010-11-19), we
> declared the problematic versions obsolete; I suspect that it is
> safe to assume that those who would be hurt by flipping the default
> would already be extinct after 6 years since then.

Yeah, I'd agree, though that may be worth mentioning in the commit
message.

> > OK. I was trying to see if (1) could be low-hanging fruit for any of the
> > newcomers, but at this point it probably makes sense for you to just
> > write the patch.
> 
> Leaving it as low-hanging fruit is actually a good idea.
> 
> I was thinking about flipping it in Meta/dodoc.sh, which would
> update the git-manpages.git repository whose mirrors are found at

Ah, right. I was thinking that the patch would go to "master" first, but
there is no reason it could not be flipped independently for your build
process.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC/PATCH 2/2] log: add "--no-show-signature" command line option

2016-05-26 Thread Mehul Jain
Hi,

Thanks for your input.

On Thu, May 26, 2016 at 10:02 PM, Jeff King  wrote:
> On Thu, May 26, 2016 at 06:36:47PM +0530, Mehul Jain wrote:
>> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
>> index 36be9a1..ea24259 100755
>> --- a/t/t4202-log.sh
>> +++ b/t/t4202-log.sh
>> @@ -901,6 +901,13 @@ test_expect_success GPG 'log.showsignature=true behaves 
>> like --show-signature' '
>>   test_i18ngrep "gpg: Good signature" actual
>>  '
>>
>> +test_expect_success GPG '--no-show-signature overrides 
>> log.showsignature=true' '
>> + git config log.showsignature true &&
>> + git log -1 --no-show-signature signed >actual &&
>> + test "$(test_i18ngrep "gpg: Signature made" actual)" = "" &&
>> + test "$(test_i18ngrep "gpg: Good signature" actual)" = ""
>> +'
>
> Perhaps it would be more robust to simply grep for "gpg:". We should not
> be seeing any gpg-related lines in the output. It probably isn't that
> big a deal in practice, though. If the output from gpg changes, this
> test could report a false success, but all of the other nearby tests
> would show a breakage, so somebody would probably notice.

That's a very good point. I will make the changes accordingly.

Thanks,
Mehul
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 4/6] config: return configset value for current_config_ functions

2016-05-26 Thread Junio C Hamano
Duy Nguyen  writes:

> On Thu, May 19, 2016 at 7:08 AM, Jeff King  wrote:
>> On Wed, May 18, 2016 at 06:43:23PM -0400, Jeff King wrote:
>>
>>>  cache.h|  1 +
>>>  config.c   | 51 
>>> +-
>>>  t/helper/test-config.c | 20 
>>>  t/t1308-config-set.sh  | 23 +++
>>> [...]
>>> +test_expect_success 'iteration shows correct origins' '
>>> + echo "[alias]test-config = !test-config" >.gitconfig &&
>
> How about using 'which' to get absolute path for test-config and put
> it here? Then we don't rely on $PATH anymore.

Don't use which, which is not portable.

Remind me why we end up running ./test-config instead of
./bin-wrappers/test-config?  Should our tests be running 
bin-wrappers early in their $PATH, perhaps?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH/RFC 0/6] pack-objects hook for upload-pack

2016-05-26 Thread Junio C Hamano
Jeff King  writes:

> Yeah, I had the same thought while working on this, but just didn't want
> to have to tweak every config callback. As you say, I don't think this
> makes anything fundamentally worse, though. I'm inclined to go with this
> strategy, especially with the extra die("BUG") safety added here.

Fine by me.

> PS Did you have any thoughts on the t/helper problem mentioned in:
>
>  http://article.gmane.org/gmane.comp.version-control.git/295029
>
>I suspect it will bite you if you try merging/testing this.

It already did but thanks to that message I didn't have to panic ;-).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC/PATCH] Formatting variables in the documentation

2016-05-26 Thread Junio C Hamano
Jeff King  writes:

> On Thu, May 26, 2016 at 09:18:17AM -0700, Junio C Hamano wrote:
>
>> >   1. Somebody produces a patch flipping the default. The patch is
>> >  trivial, but the commit message should tell why, and try to dig up
>> >  any possible problems we might see (e.g., why wasn't this the
>> >  default? Particular versions of tools? Some platforms?)
>> [...]
>> There was no particular "caveat" raised there to recommend against
>> using this on particular versions of tools or platforms.  It was
>> inertia that has kept the new optional feature "optional".
>
> Thanks for digging. That matches my recollection and the limited
> research I did more recently.

For completeness's sake I should point out that the discussion on
the first thread did point out some version-dependent issues.  But
with 79c461d5 (docs: default to more modern toolset, 2010-11-19), we
declared the problematic versions obsolete; I suspect that it is
safe to assume that those who would be hurt by flipping the default
would already be extinct after 6 years since then.

>> >   2. Assuming no problems, Junio merges the patch to "next". We get
>> >  any reports of issues from people using "next" day-to-day.
>> 
>> So I can do these steps myself up to this point.  After waiting for
>> a few days to see if somebody else with better memory tells me what
>> I forgot, perhaps.
>
> OK. I was trying to see if (1) could be low-hanging fruit for any of the
> newcomers, but at this point it probably makes sense for you to just
> write the patch.

Leaving it as low-hanging fruit is actually a good idea.

I was thinking about flipping it in Meta/dodoc.sh, which would
update the git-manpages.git repository whose mirrors are found at

git://git.kernel.org/pub/scm/git/git-manpages.git/
git://repo.or.cz/git-manpages.git/
git://github.com/gitster/git-manpages.git/

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 4/6] config: return configset value for current_config_ functions

2016-05-26 Thread Jeff King
On Thu, May 26, 2016 at 09:42:48AM -0700, Junio C Hamano wrote:

> Duy Nguyen  writes:
> 
> > On Thu, May 19, 2016 at 7:08 AM, Jeff King  wrote:
> >> On Wed, May 18, 2016 at 06:43:23PM -0400, Jeff King wrote:
> >>
> >>>  cache.h|  1 +
> >>>  config.c   | 51 
> >>> +-
> >>>  t/helper/test-config.c | 20 
> >>>  t/t1308-config-set.sh  | 23 +++
> >>> [...]
> >>> +test_expect_success 'iteration shows correct origins' '
> >>> + echo "[alias]test-config = !test-config" >.gitconfig &&
> >
> > How about using 'which' to get absolute path for test-config and put
> > it here? Then we don't rely on $PATH anymore.
> 
> Don't use which, which is not portable.
> 
> Remind me why we end up running ./test-config instead of
> ./bin-wrappers/test-config?  Should our tests be running 
> bin-wrappers early in their $PATH, perhaps?

The problem is running test-config inside of a git alias. The
bin-wrappers will set the exec-path to the root-level of git's build
directory, which the git binary will then stick at the front of the
$PATH.

So the simplest solution really is: don't do that. The only debate in my
mind is whether this is rare enough that it won't bite somebody again in
the future, or if we should look into a solution that makes this Just
Work.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC/PATCH 1/2] log: add "log.showsignature" configuration variable

2016-05-26 Thread Jeff King
On Thu, May 26, 2016 at 06:36:46PM +0530, Mehul Jain wrote:

> diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
> index 03f9580..f39f800 100644
> --- a/Documentation/git-log.txt
> +++ b/Documentation/git-log.txt
> @@ -196,6 +196,10 @@ log.showRoot::
>   `git log -p` output would be shown without a diff attached.
>   The default is `true`.
>  
> +log.showSignature::
> + If `true`, `git log` and `git show` will act as if `--show-signature`
> + option was passed to them.

This should be:

  ...if the `--show-signature` option was...

or:

  ...if `--show-signature` was...

Either is correct; you just need an article when not referring directly
to the option by its name.

The documentation here mentions "log" and "show". But I think this will
affect other programs, too, including "whatchanged" and "reflog". Those
ones are probably good, but the documentation is a little misleading (I
think other options just say "git-log and related commands" or
something).

I thought at first it would affect format-patch, too, which would be
weird. But in that command we _do_ parse the variable and end up setting
default_show_signature, but we never call cmd_log_init_defaults(), which
is what copies that value into the rev_info struct. That's kind of a
weird way to split it, but it's certainly not something you introduced
here.

> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 128ba93..36be9a1 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -890,6 +890,25 @@ test_expect_success GPG 'log --graph --show-signature 
> for merged tag' '
>   grep "^| | gpg: Good signature" actual
>  '
>  
> +test_expect_success GPG 'log.showsignature=true behaves like 
> --show-signature' '
> + git checkout -b test_sign master &&
> + echo foo >foo &&
> + git add foo &&
> + git commit -S -m signed_commit &&
> + test_config log.showsignature true &&
> + git log -1 signed >actual &&
> + test_i18ngrep "gpg: Signature made" actual &&
> + test_i18ngrep "gpg: Good signature" actual
> +'

You can see in the context that we do not use test_i18ngrep for finding
gpg output in existing tests. I'm not sure if the new tests should be
consistent, or if they should be changed to use test_i18ngrep. I don't
think it's actually doing anything here, though. It's used with a
git-specific GETTEXT_POISON flag that tweaks the output generated by
git, but not by sub-programs like gpg.

> +test_expect_success GPG '--show-signature overrides log.showsignature=false' 
> '
> + test_when_finished "git reset --hard && git checkout master" &&
> + git config log.showsignature false &&

Should this be test_config?

> +test_expect_success GPG 'log.showsignature behaves like --show-signature' '
> + git config log.showsignature true &&

Ditto here.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC/PATCH 2/2] log: add "--no-show-signature" command line option

2016-05-26 Thread Jeff King
On Thu, May 26, 2016 at 10:12:30PM +0530, Mehul Jain wrote:

> On Thu, May 26, 2016 at 10:02 PM, Jeff King  wrote:
> > On Thu, May 26, 2016 at 06:36:47PM +0530, Mehul Jain wrote:
> >> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> >> index 36be9a1..ea24259 100755
> >> --- a/t/t4202-log.sh
> >> +++ b/t/t4202-log.sh
> >> @@ -901,6 +901,13 @@ test_expect_success GPG 'log.showsignature=true 
> >> behaves like --show-signature' '
> >>   test_i18ngrep "gpg: Good signature" actual
> >>  '
> >>
> >> +test_expect_success GPG '--no-show-signature overrides 
> >> log.showsignature=true' '
> >> + git config log.showsignature true &&
> >> + git log -1 --no-show-signature signed >actual &&
> >> + test "$(test_i18ngrep "gpg: Signature made" actual)" = "" &&
> >> + test "$(test_i18ngrep "gpg: Good signature" actual)" = ""
> >> +'
> >
> > Perhaps it would be more robust to simply grep for "gpg:". We should not
> > be seeing any gpg-related lines in the output. It probably isn't that
> > big a deal in practice, though. If the output from gpg changes, this
> > test could report a false success, but all of the other nearby tests
> > would show a breakage, so somebody would probably notice.
> 
> That's a very good point. I will make the changes accordingly.

While you are here, note that test_i18ngrep can already do the
"negative" grep, like:

  test_i18ngrep ! "^gpg:" actual

Though see my comments in the other part of the thread; I'm not sure
it's worth using i18ngrep at all.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] mingw: make isatty() recognize MSYS2's pseudo terminals (/dev/pty*)

2016-05-26 Thread Junio C Hamano
Johannes Schindelin  writes:

> I do not see this patch in 'pu'... Anything I can do to get this into
> 'master' eventually?

The reason why I left it in my inbox was because I couldn't tell if
this was a final submission with concensus among Git developers on
Windows, or I should be giving a chance to comment to some folks who
work on Windows port but are not necessarily closely communicating
with you.

If the message were Cc'ed to J6t, I would probably have queued it on
'pu' and marked it as "Will merge after waiting for a few days" in
What's cooking.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[feature request] find git commit log before rebase

2016-05-26 Thread ryenus
Assuming I have branches master (m), and a side branch (b), with a
history tree like below:

m0 --- m1 -- m2 -- m3 -- m4 --- master (m)
\  /  \
b1 -- b2   b3 -- b4 -- branch (b) (HEAD)
  |
  (tag:POINT_BEFORE_REBASE)

The history of branch b is can be described as:

1. branch b is forked at point of m1
2. branch b is merged to master into m3,
3. branch b then is rebased (fast-forward) from b2 to m4
4. then branch b started its new life as b3 b4 after rebase

With the following command: I can find b3 and b4 since last fork-point

git log --oneline $(git merge-base --fork-point master)..b

But how to find out commits b1 b2, given the fact that b2 is the point
before rebase?

I understand it can be achieved via:

git log --oneline m2..b2

That's because I know b2 is the point before rebase,
and m2 is another child of the subsequent merge commit m3

I wonder how to do this with an simple (enough) command without me
looking through the history and find b2 and m2 manually?

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [Bug?] log -p -W showing the whole file for a patch that adds to the end?

2016-05-26 Thread Junio C Hamano
René Scharfe  writes:

> Am 24.05.2016 um 20:16 schrieb Junio C Hamano:
>> René Scharfe  writes:
>>
>>>diff: factor out match_func_rec()
>>>diff: handle appended chunks better with -W
>>>diff: ignore empty lines before added functions with -W
>>>diff: don't include common trailing empty lines with -W
>>>grep: don't extend context to trailing empty lines with -W
>>>
>>>   grep.c| 28 --
>>>   xdiff/xemit.c | 63 
>>> ---
>>>   2 files changed, 82 insertions(+), 9 deletions(-)
>
>> It is curious that this much behaviour change does not need any
>> changes in the test scripts.  We do not have sufficient coverage,
>> perhaps?
>
> Well, -W is not tested at all.  I'll include some in the next round.
> No need to hurry, it's too late to land in 2.9.0 anyway.

True.

I'd say that these patches are fine as they are, and follow-up patch
for adding -W tests (instead of rerolling them) is sufficient,
though.



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC/PATCH 2/2] log: add "--no-show-signature" command line option

2016-05-26 Thread Junio C Hamano
Jeff King  writes:

> On Thu, May 26, 2016 at 06:36:47PM +0530, Mehul Jain wrote:
>
>> If "log.showsignature=true", then there is no way to override it using
>> command line switch.
>> 
>> Teach "git log" and "git show" about "--no-show-signature" command line
>> option.
>
> I think this is teaching all of the revision machinery about it (which
> is a good thing).

I agree that the proposed commit log message should be updated to
say so.

Because we do not want .showsignature configuration to affect
rev-list nor format-patch, and we will not make "--show-sig" the
default for them either.  From that point of view, there is no
reason for them to know about the "--no-show-signature" option.

The only reason why teaching the "--no-show-signature" option to
these commands is a good idea is because it would help people who
create an alias with "--show-sig" in early part of the command line,
e.g.

[alias] fp = format-patch --show-signature

by allowing them to countermand with --no-show-signature, i.e.

$ git fp --no-show-signature ...

If we are updating the log message in the final submission of this
patch, we'd want it to be clear that the presence of this option is
not an excuse to introduce .showsignature that affects rev-list
later to make sure we do not have to waste our time rejecting such a
patch in the future.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/2] submodule-config: keep shallow recommendation around

2016-05-26 Thread Stefan Beller
On Thu, May 26, 2016 at 2:02 AM, Remi Galan Alfonso
 wrote:
> Hi Stefan,
>
> Stefan Beller  writes:
>> [...]
>> @ -353,6 +354,15 @@ static int parse_config(const char *var, const char 
>> *value, void *data)
>>  else if (parse_submodule_update_strategy(value,
>>   &submodule->update_strategy) < 0)
>>  die(_("invalid value for %s"), var);
>> +} else if (!strcmp(item.buf, "shallow")) {
>> +if (!me->overwrite &&
>> + submodule->recommend_shallow != -1)
>
> Nit: You seems to be able to keep the whole condition on the same line:
>
> if (!me->overwrite && submodule->recommend_shallow != -1)
>
> If you want to keep it in two line, you might want to align it:
> if (!me->overwrite &&
> submodule->recommend_shallow != -1)

Thanks will fix!

>
> Thanks,
> Rémi
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/2] submodule update: learn `--[no-]recommend-shallow` option

2016-05-26 Thread Stefan Beller
On Thu, May 26, 2016 at 2:07 AM, Remi Galan Alfonso
 wrote:
> You forgot to update from recommend-depth to recommend-shallow
>
> Stefan Beller  writes:
>> [...]
>>  'git submodule' [--quiet] init [--] [...]
>>  'git submodule' [--quiet] deinit [-f|--force] (--all|[--] ...)
>>  'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
>> -  [-f|--force] [--rebase|--merge] [--reference ]
>> -  [--depth ] [--recursive] [--jobs ] [--] [...]
>> +  [--[no-]recommended-depth] [-f|--force] [--rebase|--merge]
>
> Here...
>
>> +--[no-]recommended-depth::
>> +This option is only valid for the update command.
>> +The initial clone of a submodule will use the recommended
>> +`submodule..depth` as provided by the .gitmodules file.
>> +
>
> ... and here.
>
> Thanks,
> Rémi

Thanks for the review,
these will be fixed in a reroll.

Thanks,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 4/6] config: return configset value for current_config_ functions

2016-05-26 Thread Junio C Hamano
Jeff King  writes:

> The problem is running test-config inside of a git alias. The
> bin-wrappers will set the exec-path to the root-level of git's build
> directory, which the git binary will then stick at the front of the
> $PATH.

I was wondering why exec-path does not point at bin-wrappers in the
first place.

A wrapper script needs to know where the real thing lives in order
to "exec" (or "exec gdb") anyway, and it hardcodes the path to it.
It happens to use GIT_EXEC_PATH to shorten the hardcoded path it
uses when it does "exec", but it does not have to.

Wouldn't we want to see "git" use any of these wrapped ones when it
invokes a non-builtin subcommand when it does so normally, honoring
GIT_EXEC_PATH?  Pointing GIT_EXEC_PATH at the top-level means that
wrappers are bypassed for such an invocation (if what is run happens
to have executable at the top-level), and possibly a totally wrong
thing is run (when we start generating the binaries in different
directories, which is what we are seeing here).

> So the simplest solution really is: don't do that. The only debate
> in my mind is whether this is rare enough that it won't bite
> somebody again in the future, or if we should look into a solution
> that makes this Just Work.

I think it was you who alluded to revamping the test framework along
the lines of preparing a "test installation" (aka "make install"
with DESTDIR set to somewhere) and making bin-wrappers to point into
that installation (or if we are testing an installed Git that may be
different from what we have source for, that final installed
location).  An installed version of Git will not have test-* helpers
so they need to come from a freshly built source tree, not from
"test installation" or "installed Git".  There may be other details
that need to be worked out, but as a longer term direction that may
not be a bad idea.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/2] fetch: better alignment in ref summary

2016-05-26 Thread Junio C Hamano
Jeff King  writes:

> On Thu, May 26, 2016 at 10:22:25AM -0400, Marc Branchaud wrote:
>
>> Why do we need any kind of "->" at all?  How about simply (with an update to
>> "old-branch" for comparison to probably-more-common output):
>> 
>> From github.com:pclouds/git
>>cafed0c..badfeed  pclouds/old-branch
>>  * [new branch]  pclouds/2nd-index
>>  * [new branch]  pclouds/3nd-index
>>  * [new branch]  pclouds/file-watcher
>
> That covers the common case of:
>
>   refs/heads/*:refs/remotes/pclouds/*
>
> but sometimes the remote and local names are not the same, and the
> mapping is interesting. Like:
>
>   $ git fetch origin refs/pull/*/head:refs/remotes/pr/*
>   ...
>* [new ref] refs/pull/77/head -> pr/77
>
> Or even:
>
>   $ git fetch origin refs/pull/77/head refs/pull/78/head
>   From ...
>* branchrefs/pull/77/head -> FETCH_HEAD
>* branchrefs/pull/78/head -> FETCH_HEAD
>
> So I think we need a scheme that can show the interesting mappings, but
> collapses to something simple for the common case.

True.  One of the entries in Marc's example is easily misread as
"pclouds/2nd-index branch at its refs/heads/pclouds/2nd-index was
fetched to its usual place", when Marc wanted to say "they had
2nd-index branch at refs/heads/2nd-index, and it was copied to our
refs/remotes/pclouds/2nd-index".

So even though we might be able to make sure it is unambiguous
without "this -> that" ("->" is pronounced as 'became'), it is
easily misread.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/2] fetch: better alignment in ref summary

2016-05-26 Thread Marc Branchaud

On 2016-05-26 01:42 PM, Junio C Hamano wrote:


True.  One of the entries in Marc's example is easily misread as
"pclouds/2nd-index branch at its refs/heads/pclouds/2nd-index was
fetched to its usual place", when Marc wanted to say "they had
2nd-index branch at refs/heads/2nd-index, and it was copied to our
refs/remotes/pclouds/2nd-index".

So even though we might be able to make sure it is unambiguous
without "this -> that" ("->" is pronounced as 'became'), it is
easily misread.


Actually, I tend to just think of it as "this is a local name you can 
use to refer to the new thing that was just fetched."  The left-hand 
side describes the thing being fetched (new or updated branch/tag/ref), 
and the right hand side shows how to locally refer to that thing.


The fact that something is buried in some odd part of the ref tree is 
less relevant, IMO.  If I'm using custom fetch refspecs or other 
oddities, I'll have that in the back of my head.  But what I really care 
about is what ref I can use with commands like log and checkout.


So, regarding Peff's examples:

> $ git fetch origin refs/pull/*/head:refs/remotes/pr/*

Just show me the "pr/foo" refs.  I know where things are coming from. 
Even if I configured that fetch refspec a long time ago, I don't need to 
see the exact mapping every time I fetch.


> $ git fetch origin refs/pull/77/head refs/pull/78/head

Ah, now this is an odd case.  Maybe there should be a different output 
format altogether for this one.  The problem is that the remote refs 
being fetched are stored without any kind of local ref.  (Commands like 
"git log FETCH_HEAD" only work with the last ref fetched, even though 
all the SHAs get added to the .git/FETCH_HEAD file.  Maybe if git 
supported a "FETCH_HEAD@{x}" syntax...)


I think the output should show the plain SHA values, since those are the 
only things the user can use to work with those refs.  Maybe something like:


From ...
 * origin:refs/pull/77/head  abcd0123
 * origin:refs/pull/78/head  453def00

(Not 100% sure about the "origin:" prefix...)

M.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCHv2 0/2] Submodule shallow recommendation [WAS: Submodules: have a depth field in the .gitmodules file]

2016-05-26 Thread Junio C Hamano
Stefan Beller  writes:

> Sometimes the history of a submodule is not considered important by
> the projects upstream. To make it easier for downstream users, allow
> a field 'submodule..depth' in .gitmodules, which can be used
> to indicate the recommended depth.

I have a design-level question.

If your project consists of 600 submodules, among which 40 of them
you would recommend making a shallow clone, are there traits, other
than "most people would not care about its history", that are shared
across these 40 subprojects?

What I am trying to get at is that after adding .shallow annotation
to these 40 submodules in .gitmodules, the project may need to add a
different annotation for the same 40 submodules to control another
operation.  Which would be cumbersome, and a level of redirection
might be a good way to solve it.

IIRC, earlier you had talked about a vision where you can just say
'submodule init --group=framework' to prepare your top-level project
tree with its submodules in a state suitable to work on 'the
framework part of the project', and the 'app' folks can substitute
'framework' with 'app' in that command.  I thought the earlier
defaultGroup work (and the attribute limited pathspec work that lays
groundwork for it) was part of that effort.

Perhaps when a user says "submodule init --group=framework", that
"framework" can choose some "developer profile" that indirectly
specifies things like which group of submodules to initialize, which
group of submodules can be shallow, etc.?

Just a strawman (without worrying about details and expressiveness
of the syntax), I am wondering if you want something like this in
your .gitmodules:

[profile "framework"]
initialize = $submodule_spec
shallow = $submodule_spec
...

[submodule "kernel"]
url = ...
path = ...

...

where $submodule_spec would be a way to choose modules by various
means.  You may name them by their names.  You may name them by
their paths.  With the submodule-pathspec topic graduated, you may
use ":(attr:framework)*" to choose them by attribute limited
pathspec.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC] Triangular Workflow: user friendly full implementation

2016-05-26 Thread Junio C Hamano
Jordan DE GEA  writes:

> We are working on full implementation of triangular workflow feature.
> For now, the main options available are: 
>- branch..pushRemote
>- remote.pushDefault
> And only setable by hands. 

And once it is set, you do not have to worry about it.  I am not
sure per-branch thing is all that useful, unless you are always
working on a single branch like 'master', but the latter would be
just set once and forget about it.

> Context:
>   - One main remote repository, e.g. git/git.
>   - A remote fork (e.g. a GitHub fork) of git/git, e.g. me/git.
>   - A local clone of me/git on the machine
> Purposes:
>   - the local branch master has to fetch to git/git by default
>   - the local branch master has to push to me/git by default

Wouldn't remote.pushDefault be the single thing you need to set just
once and forget about it?  Why would your users even want to do
these things ...

>   c. add `git fetch --set-default` in order to set remote.fetchDefault
>   d. add `git fetch --set-remote` in order to set 
> branch..fetchRemote
>   e. add `git pull --set-default` in order to set remote.fetchDefault
>   f. add `git pull --set-remote` in order to set branch..fetchRemote
>   a. add `git push --set-default` in order to set remote.pushDefault
>   b. add `git push --set-remote` in order to set branch..pushRemote

... just to configure many variables every time they work on a new
branch?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCHv2 0/2] Submodule shallow recommendation [WAS: Submodules: have a depth field in the .gitmodules file]

2016-05-26 Thread Stefan Beller
On Thu, May 26, 2016 at 11:13 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> Sometimes the history of a submodule is not considered important by
>> the projects upstream. To make it easier for downstream users, allow
>> a field 'submodule..depth' in .gitmodules, which can be used
>> to indicate the recommended depth.
>
> I have a design-level question.
>
> If your project consists of 600 submodules, among which 40 of them
> you would recommend making a shallow clone, are there traits, other
> than "most people would not care about its history", that are shared
> across these 40 subprojects?

>From my understanding these 40 subprojects are a large file storage
done different than Git LFS. In the repo world this was choosen to be
a separate repository, such that you had versioning available as the
large files change a few times (like a precompiled kernel for special
hardware, etc). And this is one of the missing pieces to translate the
current repo-tool workflow to submodules.

>
> What I am trying to get at is that after adding .shallow annotation
> to these 40 submodules in .gitmodules, the project may need to add a
> different annotation for the same 40 submodules to control another
> operation.  Which would be cumbersome, and a level of redirection
> might be a good way to solve it.
>
> IIRC, earlier you had talked about a vision where you can just say
> 'submodule init --group=framework' to prepare your top-level project
> tree with its submodules in a state suitable to work on 'the
> framework part of the project', and the 'app' folks can substitute
> 'framework' with 'app' in that command.  I thought the earlier
> defaultGroup work (and the attribute limited pathspec work that lays
> groundwork for it) was part of that effort.
>
> Perhaps when a user says "submodule init --group=framework", that
> "framework" can choose some "developer profile" that indirectly
> specifies things like which group of submodules to initialize, which
> group of submodules can be shallow, etc.?

So you are proposing another layer of indirection, i.e. instead of giving
a pathspec with ":(attr:label-framework)" we would want to give a profile
which then has the pathspec plus additional information on shallowness
an such.

>
> Just a strawman (without worrying about details and expressiveness
> of the syntax), I am wondering if you want something like this in
> your .gitmodules:
>
> [profile "framework"]
> initialize = $submodule_spec
> shallow = $submodule_spec
> ...

There could be more operations here, like update strategies.

>
> [submodule "kernel"]
> url = ...
> path = ...

instead here we could also put a (no-)init recommendation additionally
to the shallow recommendation.

>
> ...
>
> where $submodule_spec would be a way to choose modules by various
> means.  You may name them by their names.  You may name them by
> their paths.  With the submodule-pathspec topic graduated, you may
> use ":(attr:framework)*" to choose them by attribute limited
> pathspec.
>

And you reinvented submodule groups. ;)
IIRC we had a discussion if we want to have the submodule groups
stored at each submodule or at a central "profile/group" setting.
The advantage of putting the setting to each submodule is that it
is easier to organize, i.e. it produces better locality for the settings.

It is easier to know for the [submodule "kernel"] what to set for flags or
labels when looking at that submodule as you're likely to have knowledge
about it. However adding a new group/profile is cumbersome, so we'd want a

git config --add multi-set
submodule..label "initialize"

Another idea for having profiles would be to add conditional recommendations

 [submodule "kernel"]
 url = ...
 path = ...
 shallow = true if selected via :(attr:framework)

I have a worse feeling about these conditionals than about the profile, though

I think the profiles are selected using different repo manifest files, i.e. that
would be different .gitmodules/.gitattributes files on different branches.
However in each of these branches we would want to have a way to
recommend which submodules to initialize/checkout/shallow and such.

If keeping these different settings in different branches, we may desire better
merge support for .gitattributes, though (adjacent lines do not influence each
other, like in source code. So if one branch had a change like
 submodule0 ...
- /submodule1 label-foo
+ /submodule1 label-foo label-bar
 submodule2 ...

and andother branch had

 submodule1 label-foo
- /submodule2 label-baz
+ /submodule2 label-baz label-bar
 submodule3 ...

we would not want to see the merge conflict as we would with todays merge
strategy.)

Thanks,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] worktree: allow "-" short-hand for @{-1} in add command

2016-05-26 Thread Junio C Hamano
Jordan DE GEA  writes:

> From: Jordan DE GEA 
>
> Since `git worktree add` uses `git checkout` when `[]` is used,
> and `git checkout -` is already supported, it makes sense to allow the
> same shortcut in `git worktree add`.

OK.

>
> Signed-off-by: Matthieu Moy 
> Signed-off-by: Jordan DE GEA 
> ---
>  Documentation/git-worktree.txt |  3 ++-
>  builtin/worktree.c |  3 +++
>  t/t2025-worktree-add.sh| 18 ++
>  3 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index c622345..48e5fdf 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -48,7 +48,8 @@ add  []::
>  
>  Create `` and checkout `` into it. The new working directory
>  is linked to the current repository, sharing everything except working
> -directory specific files such as HEAD, index, etc.
> +directory specific files such as HEAD, index, etc. The last branch you 
> +were on can be specify with `-` as `` which is synonymous with 
> `"@{-1}"`.

You meant "can be specified", I think.  Fixing it would make the
line a bit too long, so fold it around the word "synonymous".

> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index 3acb992..b713efb 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -18,6 +18,24 @@ test_expect_success '"add" an existing empty worktree' '
>   git worktree add --detach existing_empty master
>  '
>  
> +test_expect_success '"add" using shorthand - fails when no previous branch' '
> + test_must_fail git worktree add existing -
> +'

Just an observation, but the error message we would see here might
be interesting.

> +test_expect_success '"add" using - shorthand' '
> + git checkout -b newbranch &&
> + echo hello >myworld &&
> + git add myworld &&
> + git commit -m myworld &&
> + git checkout master &&
> + git worktree add short-hand - &&


> + cd short-hand &&
> + test $(git rev-parse --symbolic-full-name HEAD) = "refs/heads/newbranch"

Broken &&-chain.

> + branch=$(cd short-hand && git rev-parse --symbolic-full-name HEAD) &&
> + test "$branch" = refs/heads/newbranch &&
> + cd ..

If any of the command between "cd short-hand" and "cd .." failed,
after correcting the broken &&-chain, the next test will end up
running in short-hand directory, which it is not expecting.  A
canonical way to avoid this problem is to replace the above with:

...
git worktree add short-hand - &&
(
cd short-hand &&
...
test "$branch" = refs/heads/newbranch
)

In this particular case, alternatively, you could also do something
like this:

git worktree add short-hand - &&
echo refs/heads/newbranch >expect &&
git -C short-hand rev-parse --symbolic-full-name HEAD >actual &&
test_cmp expect actual

and it would be sufficient.

It is not immediately obvious to me why you have two copies of the
same test in your patch to see where HEAD points at.  If the reason
is because you suspect that "git -C $there" form may give subtly
different behaviour and wanted to test both, then you could do
something like this:

git worktree add short-hand - &&
echo refs/heads/newbranch >expect &&
git -C short-hand rev-parse --symbolic-full-name HEAD >actual &&
test_cmp expect actual &&
(cd short-hand && git rev-parse --symbolic-full-name HEAD) >actual &&
test_cmp expect actual

but I do not think that is necessary.  This test is not about "does
rev-parse --symbolic-full-name work correctly with 'git -C $there'?"

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCHv2 0/2] Submodule shallow recommendation [WAS: Submodules: have a depth field in the .gitmodules file]

2016-05-26 Thread Junio C Hamano
Stefan Beller  writes:

> On Thu, May 26, 2016 at 11:13 AM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>
>>> Sometimes the history of a submodule is not considered important by
>>> the projects upstream. To make it easier for downstream users, allow
>>> a field 'submodule..depth' in .gitmodules, which can be used
>>> to indicate the recommended depth.
>>
>> I have a design-level question.
>>
>> If your project consists of 600 submodules, among which 40 of them
>> you would recommend making a shallow clone, are there traits, other
>> than "most people would not care about its history", that are shared
>> across these 40 subprojects?
>
> From my understanding these 40 subprojects are a large file storage
> done different than Git LFS. In the repo world this was choosen to be
> a separate repository, such that you had versioning available as the
> large files change a few times (like a precompiled kernel for special
> hardware, etc). And this is one of the missing pieces to translate the
> current repo-tool workflow to submodules.

I am not questioning the value of being able to say "this and that
submodule need only a shallow copy".  I am trying to see
"individually mark each and every such submodules" should be the
primary interface to do so.

> So you are proposing another layer of indirection, i.e. instead of giving
> a pathspec with ":(attr:label-framework)" we would want to give a profile
> which then has the pathspec plus additional information on shallowness
> an such.

I do not understand what you mean by "instead of giving a pathspec"
at all.

When you have 40 submodules, with your design the user has to
sprinkle 40 lines of

shallow = true

for each of [submodule "$n"] sections.  If there are other traits
(see my first question) that are similar, they will have some other
setting next to the "shallow = true" 40 times.  When a new submodule
is added to that same class, they will again have to add these two
lines.

I was wondering if a level of indirection that lets you say
"submodules in this group all share 'shallow=true' (by default)"
would improve that cumbersomeness.  When you add another similar
trait, you add that just once, not 40 times.  When you add another
submodule, you say "this submodule is also in that group", without
mentioning "shallow".

We probably _need_ shallow=true at individual module level, if only
to override the default given by such an indirection scheme.  So
don't take the message you are responding to as "no, your design is
not good, scrap it, and here is a better one".  It is more like "It
would be a good first step, but have you considered where it will
eventually lead us to?  Would the more complete future look like
this, and how well would this first step fit in that world?  Would
it be a good escape hatch, or would it have to be deprecated?"

> And you reinvented submodule groups. ;)
> IIRC we had a discussion if we want to have the submodule groups
> stored at each submodule or at a central "profile/group" setting.

As I said, it was not my intention to get into that; I am not
interested in the exact syntax, and I am not interested whether the
pointer goes from group to individual modules (i.e. [group "bar"]
says "foo" is one of its member modules), or individual modules have
pointers to groups (i.e. "module [foo]" declares its membership in
group "bar") at all.  I really do not care.

What matters in my suggestion was, after you established that group
"bar" exists, you can do:

[profile "framework"]
shallow = "some notation that refers to group bar"

so that you do not have to repeat "shallow = true" many times per
submodule.

By the way, I do not see the "profile" as about "submodules" or
"submodule groups".  It is more about "Who am I?  What am I working
on?  Give me an appropriate set of settings for the 600 submodules
you have, with the knowledge that I am a framework person".

grouping submodules would merely be one mechanism to help specify
that.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/2] fetch: better alignment in ref summary

2016-05-26 Thread Junio C Hamano
Marc Branchaud  writes:

> The fact that something is buried in some odd part of the ref tree is
> less relevant, IMO.  If I'm using custom fetch refspecs or other
> oddities, I'll have that in the back of my head.  But what I really
> care about is what ref I can use with commands like log and checkout.
>
> So, regarding Peff's examples:
>
>> $ git fetch origin refs/pull/*/head:refs/remotes/pr/*
>
> Just show me the "pr/foo" refs.  I know where things are coming
> from. Even if I configured that fetch refspec a long time ago, I don't
> need to see the exact mapping every time I fetch.

That is only because you are used to seeing them.  You cannot claim
"I'll remember forever without seeing them" without actually
experiencing not seeing them for a long time.

> I think the output should show the plain SHA values, since those are
> the only things the user can use to work with those refs.

When they tell others how to reproduce what they did (or record what
they did so that they can reproduce it later), they need what it is
called at the remote end.

I would hesitate to go in the approach based on discarding
information, as if it is the only way to shorten and clarify the
output.  Let's not do so before thinking things through to achieve
the same while keeping the information we give to the users.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] submodule operations: tighten pathspec errors

2016-05-26 Thread Junio C Hamano
Stefan Beller  writes:

> It's a first initial version with no tests (and probably conflicting with
> some topics in flight), but I was curious how involved this issue actually is,
> so I took a stab at implementing it.

I take it to mean "This is s/PATCH/RFC/".

> +--error-unmatch::
> + If the pathspec included a specification that did not match,
> + the usual operation is to error out. This switch suppresses
> + error reporting and continues the operation.

The behaviour described is a total opposite of the option with the
same name "ls-files" has, no?

If there were no default, --error-unmatch would make an unmatching
pathspec an error and --no-error-unmatch would make it a non-error.

If the default is to error out, there is no need for --error-unmatch
to exist, but you do want --no-error-unmatch aka --unmatch-ok.

If the default is not to error out, --error-unmatch should make it
notice and turn it into an error.

I am guessing that you were debating yourself which should be the
default and the patch ended up in an inconsistent state, the
description assuming a more strict default, while the option name
assuming a less strict default.

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 5295b72..91c49ec 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -19,7 +19,8 @@ struct module_list {
>  static int module_list_compute(int argc, const char **argv,
>  const char *prefix,
>  struct pathspec *pathspec,
> -struct module_list *list)
> +struct module_list *list,
> +int unmatch)

What is "unmatch"?  "Catch unmatch errors, please?"  "Do not check
and report unmatch errors?"

My cursory read of a few hunks below tells me that you meant the
latter, i.e. "unmatch_ok".

> @@ -36,10 +37,9 @@ static int module_list_compute(int argc, const char **argv,
>  
>   for (i = 0; i < active_nr; i++) {
>   const struct cache_entry *ce = active_cache[i];
> -
> - if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),
> - 0, ps_matched, 1) ||
> - !S_ISGITLINK(ce->ce_mode))
> + if (!S_ISGITLINK(ce->ce_mode) ||
> + !match_pathspec(pathspec, ce->name, ce_namelen(ce),
> + 0, ps_matched, 1))
>   continue;

OK, this is the crucial bit in this patch. pathspec matches are now
done only against gitlinks, so any unmatch is "pattern or path
given, but there was no such submodule".

> @@ -53,7 +53,9 @@ static int module_list_compute(int argc, const char **argv,
>   i++;
>   }
>  
> - if (ps_matched && report_path_error(ps_matched, pathspec, prefix))
> + if (!unmatch &&
> + ps_matched &&
> + report_path_error(ps_matched, pathspec, prefix))
>   result = -1;

If unmatch is not true, then check if ps_matched records "aw, this
pathspec element did not get used" and complain.  If unmatch is
true, we do not do that.

Which confirms my earlier "'unmatch' here means 'unmatch_ok'".

It is tempting to update report_path_error() return "OK" when its
first parameter is NULL.

> diff --git a/git-submodule.sh b/git-submodule.sh
> index fb68f1f..f10e10a 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -391,6 +391,9 @@ cmd_foreach()
>   --recursive)
>   recursive=1
>   ;;
> + --error-unmatch)
> + unmatch=1
> + ;;

So "--error-unmatch" does pass "--unmatch" which is "please ignore
unmatch errors".  That is a bit strange (see above).

> @@ -407,7 +410,7 @@ cmd_foreach()
>   # command in the subshell (and a recursive call to this function)
>   exec 3<&0
>  
> - git submodule--helper list --prefix "$wt_prefix"|
> + git submodule--helper list ${unmatch:+--unmatch} --prefix "$wt_prefix"|

For this to work, somebody must ensure that the variable unmatch is
either unset or set to empty unless the user gave --error-unmatch to
us.  There is a block of empty assignments hear the beginning of
this file for that very purpose, i.e. resetting a stray environment
variable that could be in user's environment.

The patch itself does not look too bad.  I do not have an opinion on
which one should be the default, and I certainly would understand if
you want to keep the default loose (i.e. not complaining) with an
optional error checking, but whichever default you choose, the
option and variable names need to be clarified to avoid confusion.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [feature request] find git commit log before rebase

2016-05-26 Thread Sebastian Staudt
Hi.

I think what you want is `git reflog` (http://git-scm.com/man/reflog).

git reflog b

Will tell you the commits b pointed to in the past.

Best regards,
Sebastian

2016-05-26 18:55 GMT+02:00 ryenus :
> Assuming I have branches master (m), and a side branch (b), with a
> history tree like below:
>
> m0 --- m1 -- m2 -- m3 -- m4 --- master (m)
> \  /  \
> b1 -- b2   b3 -- b4 -- branch (b) (HEAD)
>   |
>   (tag:POINT_BEFORE_REBASE)
>
> The history of branch b is can be described as:
>
> 1. branch b is forked at point of m1
> 2. branch b is merged to master into m3,
> 3. branch b then is rebased (fast-forward) from b2 to m4
> 4. then branch b started its new life as b3 b4 after rebase
>
> With the following command: I can find b3 and b4 since last fork-point
>
> git log --oneline $(git merge-base --fork-point master)..b
>
> But how to find out commits b1 b2, given the fact that b2 is the point
> before rebase?
>
> I understand it can be achieved via:
>
> git log --oneline m2..b2
>
> That's because I know b2 is the point before rebase,
> and m2 is another child of the subsequent merge commit m3
>
> I wonder how to do this with an simple (enough) command without me
> looking through the history and find b2 and m2 manually?
>
> Thanks!
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 1/2] submodule update: add `--init-default-path` switch

2016-05-26 Thread Stefan Beller
The new switch `--init-default-path` initializes the submodules which are
configured in `submodule.defaultUpdatePath` instead of those given as
command line arguments before updating. In the first implementation this
is made incompatible with further command line arguments as it is
unclear what the user means by

git submodule update --init --init-default-path 

This new switch allows to record more complex patterns as it saves
retyping them whenever you invoke update.

Signed-off-by: Stefan Beller 
---
 Documentation/config.txt|  5 
 Documentation/git-submodule.txt | 11 -
 git-submodule.sh| 21 +---
 t/t7400-submodule-basic.sh  | 53 +
 4 files changed, 86 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 53f00db..beb158d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2794,6 +2794,11 @@ submodule.fetchJobs::
in parallel. A value of 0 will give some reasonable default.
If unset, it defaults to 1.
 
+submodule.defaultUpdatePath::
+   Specifies a set of submodules to initialize when calling
+   `git submodule --init-default-group` by using the pathspec
+   syntax.
+
 tag.forceSignAnnotated::
A boolean to specify whether annotated tags created should be GPG 
signed.
If `--annotate` is specified on the command line, it takes
diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 9226c43..000231e 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -14,7 +14,7 @@ SYNOPSIS
 'git submodule' [--quiet] status [--cached] [--recursive] [--] [...]
 'git submodule' [--quiet] init [--] [...]
 'git submodule' [--quiet] deinit [-f|--force] (--all|[--] ...)
-'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
+'git submodule' [--quiet] update [--init[-default-path]] [--remote] 
[-N|--no-fetch]
  [-f|--force] [--rebase|--merge] [--reference ]
  [--depth ] [--recursive] [--jobs ] [--] [...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]
@@ -193,6 +193,10 @@ If the submodule is not yet initialized, and you just want 
to use the
 setting as stored in .gitmodules, you can automatically initialize the
 submodule with the `--init` option.
 
+You can configure a set of submodules using pathspec syntax in
+submodule.defaultUpdatePath you can use `--init-default-path` to initialize
+those before updating.
+
 If `--recursive` is specified, this command will recurse into the
 registered submodules, and update any nested submodules within.
 --
@@ -360,6 +364,11 @@ the submodule itself.
Initialize all submodules for which "git submodule init" has not been
called so far before updating.
 
+--init-default-path::
+   This option is only valid for the update command.
+   Initialize all submodules configured in "`submodule.defaultUpdatePath`"
+   that have not been updated before.
+
 --name::
This option is only valid for the add command. It sets the submodule's
name to the given string instead of defaulting to its path. The name
diff --git a/git-submodule.sh b/git-submodule.sh
index 5a4dec0..3d12145 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -9,7 +9,7 @@ USAGE="[--quiet] add [-b ] [-f|--force] [--name ] 
[--reference ...]
or: $dashless [--quiet] init [--] [...]
or: $dashless [--quiet] deinit [-f|--force] (--all| [--] ...)
-   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
[-f|--force] [--checkout|--merge|--rebase] [--reference ] 
[--recursive] [--] [...]
+   or: $dashless [--quiet] update [--init[-default-path]] [--remote] 
[-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--reference 
] [--recursive] [--] [...]
or: $dashless [--quiet] summary [--cached|--files] [--summary-limit ] 
[commit] [--] [...]
or: $dashless [--quiet] foreach [--recursive] 
or: $dashless [--quiet] sync [--recursive] [--] [...]"
@@ -528,7 +528,12 @@ cmd_update()
GIT_QUIET=1
;;
-i|--init)
-   init=1
+   test -z $init || test $init = by_args || die "$(gettext 
"Only one of --init or --init-default-path may be used.")"
+   init=by_args
+   ;;
+   --init-default-path)
+   test -z $init || test $init = by_config || die 
"$(gettext "Only one of --init or --init-default-path may be used.")"
+   init=by_config
;;
--remote)
remote=1
@@ -591,7 +596,17 @@ cmd_update()
 
if test -n "$init"
then
-   cmd_init "--" "$@" || return
+   if test "$init" = "by_config"
+   then
+   if test $# -gt 0
+ 

[PATCHv3 0/2] Persistent submodule pathspec specification

2016-05-26 Thread Stefan Beller
Changes since v2:
 * I replaced one 0 by NULL as pointed out by Ramsay, and reformatted the line
   to stay within 80 characters:
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -908,8 +908,10 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
struct string_list_item *item;
struct strbuf sb = STRBUF_INIT;
for_each_string_list_item(item, &init_submodules) {
-   strbuf_addf(&sb, "submodule.defaultUpdatePath=%s", 
item->string);
-   string_list_append(&option_config, strbuf_detach(&sb, 
0));
+   strbuf_addf(&sb, "submodule.defaultUpdatePath=%s",
+   item->string);
+   string_list_append(&option_config,
+  strbuf_detach(&sb, NULL));
}
}
 

Changes since v1:
 * fixed a broken && chain in a subshell for testing, as pointed out by Eric!

This was part of the former series 'submodule groups'.
However the labeling was ripped out and goes in its own series
sb/pathspec-label.

First we introduce a switch `--init-default-path` for `git submodule update`
which will read the pathspec to initialize the submodules not from the command
line but from `submodule.defaultUpdatePath`, which can be configured 
permanently.

The second patch utilizes this by having `clone` set that config option
and using that new option when calling to update the submodules.

Thanks,
Stefan

Stefan Beller (2):
  submodule update: add `--init-default-path` switch
  clone: add --init-submodule= switch

 Documentation/config.txt|   5 ++
 Documentation/git-clone.txt |  25 +---
 Documentation/git-submodule.txt |  11 +++-
 builtin/clone.c |  36 ++-
 git-submodule.sh|  21 ++-
 t/t7400-submodule-basic.sh  | 134 
 6 files changed, 218 insertions(+), 14 deletions(-)

-- 
2.9.0.rc0.2.g145fc64

base-commit: 3a0f269e7c82aa3a87323cb7ae04ac5f129f036b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 2/2] clone: add --init-submodule= switch

2016-05-26 Thread Stefan Beller
The new switch passes the pathspec to `git submodule update --init`
which is called after the actual clone is done.

Additionally this configures the submodule.defaultUpdatePath to
be the given pathspec, such that any future invocation of
`git submodule update --init-default-paths` will keep up
with the pathspec.

Signed-off-by: Stefan Beller 
---
 Documentation/git-clone.txt | 25 +-
 builtin/clone.c | 36 ++--
 t/t7400-submodule-basic.sh  | 81 +
 3 files changed, 132 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 1b15cd7..33e5894 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -14,8 +14,9 @@ SYNOPSIS
  [-o ] [-b ] [-u ] [--reference ]
  [--dissociate] [--separate-git-dir ]
  [--depth ] [--[no-]single-branch]
- [--recursive | --recurse-submodules] [--[no-]shallow-submodules]
- [--jobs ] [--]  []
+ [--recursive | --recurse-submodules] [--jobs ]
+ [--init-submodule ] [--] 
+ []
 
 DESCRIPTION
 ---
@@ -207,12 +208,20 @@ objects from the source repository into a pack in the 
cloned repository.
 
 --recursive::
 --recurse-submodules::
-   After the clone is created, initialize all submodules within,
-   using their default settings. This is equivalent to running
-   `git submodule update --init --recursive` immediately after
-   the clone is finished. This option is ignored if the cloned
-   repository does not have a worktree/checkout (i.e. if any of
-   `--no-checkout`/`-n`, `--bare`, or `--mirror` is given)
+   After the clone is created, initialize and clone all submodules
+   within, using their default settings. This is equivalent to
+   running `git submodule update --recursive --init `
+   immediately after the clone is finished. This option is ignored
+   if the cloned repository does not have a worktree/checkout (i.e.
+   if any of `--no-checkout`/`-n`, `--bare`, or `--mirror` is given)
+
+--init-submodule::
+   After the clone is created, initialize and clone specified
+   submodules within, using their default settings. It is possible
+   to give multiple specifications by giving this argument multiple
+   times. This is equivalent to configure `submodule.defaultUpdateGroup`
+   and then running `git submodule update --init-default-path`
+   immediately after the clone is finished.
 
 --[no-]shallow-submodules::
All submodules which are cloned will be shallow with a depth of 1.
diff --git a/builtin/clone.c b/builtin/clone.c
index 5f867e6..668c1f4 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -53,6 +53,16 @@ static struct string_list option_config;
 static struct string_list option_reference;
 static int option_dissociate;
 static int max_jobs = -1;
+static struct string_list init_submodules;
+
+static int init_submodules_cb(const struct option *opt, const char *arg, int 
unset)
+{
+   if (unset)
+   return -1;
+
+   string_list_append((struct string_list *)opt->value, arg);
+   return 0;
+}
 
 static struct option builtin_clone_options[] = {
OPT__VERBOSITY(&option_verbosity),
@@ -103,6 +113,9 @@ static struct option builtin_clone_options[] = {
TRANSPORT_FAMILY_IPV4),
OPT_SET_INT('6', "ipv6", &family, N_("use IPv6 addresses only"),
TRANSPORT_FAMILY_IPV6),
+   OPT_CALLBACK(0, "init-submodule", &init_submodules, N_(""),
+   N_("clone specific submodules. Pass ultiple times for 
complex pathspecs"),
+   init_submodules_cb),
OPT_END()
 };
 
@@ -734,14 +747,22 @@ static int checkout(void)
err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
   sha1_to_hex(sha1), "1", NULL);
 
-   if (!err && option_recursive) {
+   if (!err && (option_recursive || init_submodules.nr > 0)) {
struct argv_array args = ARGV_ARRAY_INIT;
-   argv_array_pushl(&args, "submodule", "update", "--init", 
"--recursive", NULL);
+   argv_array_pushl(&args, "submodule", "update", NULL);
+
+   if (init_submodules.nr > 0)
+   argv_array_pushf(&args, "--init-default-path");
+   else
+   argv_array_pushf(&args, "--init");
 
if (option_shallow_submodules == 1
|| (option_shallow_submodules == -1 && option_depth))
argv_array_push(&args, "--depth=1");
 
+   if (option_recursive)
+   argv_array_pushf(&args, "--recursive");
+
if (max_jobs != -1)
argv_array_pushf(&args, "--jobs=%d", max_jobs);
 
@@ -883,6 +904,17 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
opti

Re: [PATCH] userdiff: add built-in pattern for CSS

2016-05-26 Thread Johannes Sixt
Am 24.05.2016 um 16:25 schrieb William Duclot:
> +PATTERNS("css",
> +  "^([^,{}]+)((,[^}]*\\{)|([ \t]*\\{))$",

This hunk header pattern is a bit too restrictive for my taste. Find
below a few more test cases that you should squash in. One case fails
because only the first CSS selector is picked up, for which I do not
see a reason.

Another case fails because the opening brace is not on the line with
the CSS selectors.

I think what the hunk header pattern should do is:

1. reject lines containing a colon (because that are properties)
2. if a line begins with a name in column 1, pick the whole line

See the cpp patterns: a pattern beginning with ! is a "reject" pattern.


diff --git a/t/t4018/css-brace-in-col-1 b/t/t4018/css-brace-in-col-1
new file mode 100644
index 000..7831577
--- /dev/null
+++ b/t/t4018/css-brace-in-col-1
@@ -0,0 +1,5 @@
+RIGHT label.control-label
+{
+margin-top: 10px!important;
+border : 10px ChangeMe #C6C6C6;
+}
diff --git a/t/t4018/css-rule b/t/t4018/css-common
similarity index 100%
rename from t/t4018/css-rule
rename to t/t4018/css-common
diff --git a/t/t4018/css-long-selector-list b/t/t4018/css-long-selector-list
new file mode 100644
index 000..7ccd25d
--- /dev/null
+++ b/t/t4018/css-long-selector-list
@@ -0,0 +1,6 @@
+p.header,
+label.control-label,
+div ul#RIGHT {
+margin-top: 10px!important;
+border : 10px ChangeMe #C6C6C6;
+}
diff --git a/t/t4018/css-prop-sans-indent b/t/t4018/css-prop-sans-indent
new file mode 100644
index 000..a9e3c86
--- /dev/null
+++ b/t/t4018/css-prop-sans-indent
@@ -0,0 +1,5 @@
+RIGHT, label.control-label {
+margin-top: 10px!important;
+padding: 0;
+border : 10px ChangeMe #C6C6C6;
+}
diff --git a/t/t4018/css-short-selector-list b/t/t4018/css-short-selector-list
new file mode 100644
index 000..6a0bdee
--- /dev/null
+++ b/t/t4018/css-short-selector-list
@@ -0,0 +1,4 @@
+label.control, div ul#RIGHT {
+margin-top: 10px!important;
+border : 10px ChangeMe #C6C6C6;
+}
-- 
2.9.0.rc0.40.gb3c1388

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCHv3 0/2] Persistent submodule pathspec specification

2016-05-26 Thread Junio C Hamano
Thanks; will replace.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCHv2 0/2] Submodule shallow recommendation [WAS: Submodules: have a depth field in the .gitmodules file]

2016-05-26 Thread Stefan Beller
On Thu, May 26, 2016 at 12:16 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> On Thu, May 26, 2016 at 11:13 AM, Junio C Hamano  wrote:
>>> Stefan Beller  writes:
>>>
 Sometimes the history of a submodule is not considered important by
 the projects upstream. To make it easier for downstream users, allow
 a field 'submodule..depth' in .gitmodules, which can be used
 to indicate the recommended depth.
>>>
>>> I have a design-level question.
>>>
>>> If your project consists of 600 submodules, among which 40 of them
>>> you would recommend making a shallow clone, are there traits, other
>>> than "most people would not care about its history", that are shared
>>> across these 40 subprojects?
>>
>> From my understanding these 40 subprojects are a large file storage
>> done different than Git LFS. In the repo world this was choosen to be
>> a separate repository, such that you had versioning available as the
>> large files change a few times (like a precompiled kernel for special
>> hardware, etc). And this is one of the missing pieces to translate the
>> current repo-tool workflow to submodules.
>
> I am not questioning the value of being able to say "this and that
> submodule need only a shallow copy".  I am trying to see
> "individually mark each and every such submodules" should be the
> primary interface to do so.
>
>> So you are proposing another layer of indirection, i.e. instead of giving
>> a pathspec with ":(attr:label-framework)" we would want to give a profile
>> which then has the pathspec plus additional information on shallowness
>> an such.
>
> I do not understand what you mean by "instead of giving a pathspec"
> at all.
>
> When you have 40 submodules, with your design the user has to
> sprinkle 40 lines of
>
> shallow = true
>
> for each of [submodule "$n"] sections.  If there are other traits
> (see my first question) that are similar, they will have some other
> setting next to the "shallow = true" 40 times.  When a new submodule
> is added to that same class, they will again have to add these two
> lines.

I could not find out about another class or attribute matching like that.
Sure there are other attributes (Labels), but they don't match closely
these 40 submodules, but are arbitrary.

>
> I was wondering if a level of indirection that lets you say
> "submodules in this group all share 'shallow=true' (by default)"
> would improve that cumbersomeness.  When you add another similar,
> trait, you add that just once, not 40 times.  When you add another
> submodule, you say "this submodule is also in that group", without
> mentioning "shallow".
>
> We probably _need_ shallow=true at individual module level, if only
> to override the default given by such an indirection scheme.  So
> don't take the message you are responding to as "no, your design is
> not good, scrap it, and here is a better one".  It is more like "It
> would be a good first step, but have you considered where it will
> eventually lead us to?

Even if we decide we need it later, this doesn't seem to be a blocker
or contradicting such an enhancement.

> Would the more complete future look like
> this, and how well would this first step fit in that world?  Would
> it be a good escape hatch, or would it have to be deprecated?"

Looking at the Android open source project, we have 45 repos,
that are marked with clone-depth="1" , 37/45 are in a path starting
with "./prebuilts/", i.e. competing with Git LFS.
The remaining 8/45 seem to be device specific kernels.
37 of them (a different set than the prebuilts though) carry one label.

So with that data, I would expect that you would not need to mark a
submodule often as shallow, but it's quite obvious at creation time.
And specially at creation time it is easier to edit the module specific section
as opposed to adding the module in a profile or other indirect section.

I note, that we can extend to an indirection concept later if needed, though.

>
>> And you reinvented submodule groups. ;)
>> IIRC we had a discussion if we want to have the submodule groups
>> stored at each submodule or at a central "profile/group" setting.
>
> As I said, it was not my intention to get into that; I am not
> interested in the exact syntax, and I am not interested whether the
> pointer goes from group to individual modules (i.e. [group "bar"]
> says "foo" is one of its member modules), or individual modules have
> pointers to groups (i.e. "module [foo]" declares its membership in
> group "bar") at all.  I really do not care.
>
> What matters in my suggestion was, after you established that group
> "bar" exists, you can do:
>
> [profile "framework"]
> shallow = "some notation that refers to group bar"
>
> so that you do not have to repeat "shallow = true" many times per
> submodule.

Ok, I just wonder how hard that  "some notation that refers to group bar"
is to get right. (Both on the design level for us as well as for users to
understand how to e

[PATCHv3 0/2] Submodule shallow recommendation [WAS: Submodules: have a depth field in the .gitmodules file]

2016-05-26 Thread Stefan Beller
v3:
* fixed documentation to consistently mention recommend-shallow
* realigned code in the config patch
* Thanks Remi for reviewing! diff to v2:

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index c928c0d..bf3bb37 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -15,7 +15,7 @@ SYNOPSIS
 'git submodule' [--quiet] init [--] [...]
 'git submodule' [--quiet] deinit [-f|--force] (--all|[--] ...)
 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
- [--[no-]recommended-depth] [-f|--force] [--rebase|--merge]
+ [--[no-]recommend-shallow] [-f|--force] [--rebase|--merge]
  [--reference ] [--depth ] [--recursive]
  [--jobs ] [--] [...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]
@@ -385,10 +385,11 @@ for linkgit:git-clone[1]'s `--reference` and `--shared` 
options carefully.
clone with a history truncated to the specified number of revisions.
See linkgit:git-clone[1]
 
---[no-]recommended-depth::
+--[no-]recommend-shallow::
This option is only valid for the update command.
The initial clone of a submodule will use the recommended
-   `submodule..depth` as provided by the .gitmodules file.
+   `submodule..shallow` as provided by the .gitmodules file
+   by default. To ignore the suggestions use `--no-recommend-shallow`.
 
 -j ::
 --jobs ::
diff --git a/submodule-config.c b/submodule-config.c
index e11b35d..db1847f 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -355,8 +355,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
 &submodule->update_strategy) < 0)
die(_("invalid value for %s"), var);
} else if (!strcmp(item.buf, "shallow")) {
-   if (!me->overwrite &&
-submodule->recommend_shallow != -1)
+   if (!me->overwrite && submodule->recommend_shallow != -1)
warn_multiple_config(me->commit_sha1, submodule->name,
 "shallow");
else {


v2:
* Instead of storing the depth, we keep a boolean `shallow` field in the
  `.gitmodules` file.
* slightly renamed options (--recommend-shallow instead of --recommended-depth)
* one more test
* I dropped [PATCH 1/3] submodule update: make use of the existing 
fetch_in_submodule function
  as Junio picked it up separately as sb/submodule-misc-cleanups

v1:
Sometimes the history of a submodule is not considered important by
the projects upstream. To make it easier for downstream users, allow
a field 'submodule..depth' in .gitmodules, which can be used
to indicate the recommended depth.

Thanks,
Stefan


Stefan Beller (2):
  submodule-config: keep shallow recommendation around
  submodule update: learn `--[no-]recommend-shallow` option

 Documentation/git-submodule.txt | 11 +++--
 builtin/submodule--helper.c |  7 +-
 git-submodule.sh|  9 ++-
 submodule-config.c  |  9 +++
 submodule-config.h  |  1 +
 t/t5614-clone-submodules.sh | 52 +
 6 files changed, 85 insertions(+), 4 deletions(-)

-- 
2.9.0.rc0.2.g145fc64

base-commit: 3a0f269e7c82aa3a87323cb7ae04ac5f129f036b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 1/2] submodule-config: keep shallow recommendation around

2016-05-26 Thread Stefan Beller
The shallow field will be used in a later patch by `submodule update`.
To differentiate between the actual depth (which may be different),
we name it `recommend_shallow` as the field in the .gitmodules file
is only a recommendation by the project.

Signed-off-by: Stefan Beller 
---
 submodule-config.c | 9 +
 submodule-config.h | 1 +
 2 files changed, 10 insertions(+)

diff --git a/submodule-config.c b/submodule-config.c
index debab29..db1847f 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -199,6 +199,7 @@ static struct submodule *lookup_or_create_by_name(struct 
submodule_cache *cache,
submodule->update_strategy.command = NULL;
submodule->fetch_recurse = RECURSE_SUBMODULES_NONE;
submodule->ignore = NULL;
+   submodule->recommend_shallow = -1;
 
hashcpy(submodule->gitmodules_sha1, gitmodules_sha1);
 
@@ -353,6 +354,14 @@ static int parse_config(const char *var, const char 
*value, void *data)
else if (parse_submodule_update_strategy(value,
 &submodule->update_strategy) < 0)
die(_("invalid value for %s"), var);
+   } else if (!strcmp(item.buf, "shallow")) {
+   if (!me->overwrite && submodule->recommend_shallow != -1)
+   warn_multiple_config(me->commit_sha1, submodule->name,
+"shallow");
+   else {
+   submodule->recommend_shallow =
+   git_config_bool(var, value);
+   }
}
 
strbuf_release(&name);
diff --git a/submodule-config.h b/submodule-config.h
index e4857f5..b1fdcc0 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -18,6 +18,7 @@ struct submodule {
struct submodule_update_strategy update_strategy;
/* the sha1 blob id of the responsible .gitmodules file */
unsigned char gitmodules_sha1[20];
+   int recommend_shallow;
 };
 
 int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
-- 
2.9.0.rc0.2.g145fc64

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 2/2] submodule update: learn `--[no-]recommend-shallow` option

2016-05-26 Thread Stefan Beller
Sometimes the history of a submodule is not considered important by
the projects upstream. To make it easier for downstream users, allow
a boolean field 'submodule..shallow' in .gitmodules, which can
be used to recommend whether upstream considers the history important.

This field is honored in the initial clone by default, it can be
ignored by giving the `--no-recommend-shallow` option.

Signed-off-by: Stefan Beller 
---
 Documentation/git-submodule.txt | 11 +++--
 builtin/submodule--helper.c |  7 +-
 git-submodule.sh|  9 ++-
 t/t5614-clone-submodules.sh | 52 +
 4 files changed, 75 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 9226c43..bf3bb37 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -15,8 +15,9 @@ SYNOPSIS
 'git submodule' [--quiet] init [--] [...]
 'git submodule' [--quiet] deinit [-f|--force] (--all|[--] ...)
 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
- [-f|--force] [--rebase|--merge] [--reference ]
- [--depth ] [--recursive] [--jobs ] [--] [...]
+ [--[no-]recommend-shallow] [-f|--force] [--rebase|--merge]
+ [--reference ] [--depth ] [--recursive]
+ [--jobs ] [--] [...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]
  [commit] [--] [...]
 'git submodule' [--quiet] foreach [--recursive] 
@@ -384,6 +385,12 @@ for linkgit:git-clone[1]'s `--reference` and `--shared` 
options carefully.
clone with a history truncated to the specified number of revisions.
See linkgit:git-clone[1]
 
+--[no-]recommend-shallow::
+   This option is only valid for the update command.
+   The initial clone of a submodule will use the recommended
+   `submodule..shallow` as provided by the .gitmodules file
+   by default. To ignore the suggestions use `--no-recommend-shallow`.
+
 -j ::
 --jobs ::
This option is only valid for the update command.
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 8da263f..ca33408 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -581,6 +581,7 @@ struct submodule_update_clone {
 
/* configuration parameters which are passed on to the children */
int quiet;
+   int recommend_shallow;
const char *reference;
const char *depth;
const char *recursive_prefix;
@@ -593,7 +594,7 @@ struct submodule_update_clone {
unsigned quickstop : 1;
 };
 #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
-   SUBMODULE_UPDATE_STRATEGY_INIT, 0, NULL, NULL, NULL, NULL, \
+   SUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, NULL, NULL, NULL, NULL, \
STRING_LIST_INIT_DUP, 0}
 
 
@@ -698,6 +699,8 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
argv_array_push(&child->args, "--quiet");
if (suc->prefix)
argv_array_pushl(&child->args, "--prefix", suc->prefix, NULL);
+   if (suc->recommend_shallow && sub->recommend_shallow == 1)
+   argv_array_push(&child->args, "--depth=1");
argv_array_pushl(&child->args, "--path", sub->path, NULL);
argv_array_pushl(&child->args, "--name", sub->name, NULL);
argv_array_pushl(&child->args, "--url", url, NULL);
@@ -780,6 +783,8 @@ static int update_clone(int argc, const char **argv, const 
char *prefix)
  "specified number of revisions")),
OPT_INTEGER('j', "jobs", &max_jobs,
N_("parallel jobs")),
+   OPT_BOOL(0, "recommend-shallow", &suc.recommend_shallow,
+   N_("whether the initial clone should follow the 
shallow recommendation")),
OPT__QUIET(&suc.quiet, N_("don't print cloning progress")),
OPT_END()
};
diff --git a/git-submodule.sh b/git-submodule.sh
index 5a4dec0..42e0e9f 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -9,7 +9,7 @@ USAGE="[--quiet] add [-b ] [-f|--force] [--name ] 
[--reference ...]
or: $dashless [--quiet] init [--] [...]
or: $dashless [--quiet] deinit [-f|--force] (--all| [--] ...)
-   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
[-f|--force] [--checkout|--merge|--rebase] [--reference ] 
[--recursive] [--] [...]
+   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
[-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] 
[--reference ] [--recursive] [--] [...]
or: $dashless [--quiet] summary [--cached|--files] [--summary-limit ] 
[commit] [--] [...]
or: $dashless [--quiet] foreach [--recursive] 
or: $dashless [--quiet] sync [--recursive] [--] [...]"
@@ -559,6 +559,12 @@ cmd_update()
--checkout)
update="checkout"

Re: [PATCH 1/2] fetch: better alignment in ref summary

2016-05-26 Thread Marc Branchaud

On 2016-05-26 03:31 PM, Junio C Hamano wrote:

Marc Branchaud  writes:


The fact that something is buried in some odd part of the ref tree is
less relevant, IMO.  If I'm using custom fetch refspecs or other
oddities, I'll have that in the back of my head.  But what I really
care about is what ref I can use with commands like log and checkout.

So, regarding Peff's examples:


$ git fetch origin refs/pull/*/head:refs/remotes/pr/*


Just show me the "pr/foo" refs.  I know where things are coming
from. Even if I configured that fetch refspec a long time ago, I don't
need to see the exact mapping every time I fetch.


That is only because you are used to seeing them.  You cannot claim
"I'll remember forever without seeing them" without actually
experiencing not seeing them for a long time.


I don't expect (or even want) to forever remember the mapping.  It's a 
matter of context.


When fetching, I'm interested in shiny new refs and I want to work with 
them.


When configuring a remote repository, I'm interested in how to bring 
that repo's refs into my own repository.


These are two distinct modes of thought, and I don't think fetch's 
output always needs to display the latter.  Perhaps the --verbose switch 
could turn on showing how the remote refs get mapped.  I can see how 
that would occasionally be useful for debugging fetch refspecs.



I think the output should show the plain SHA values, since those are
the only things the user can use to work with those refs.


When they tell others how to reproduce what they did (or record what
they did so that they can reproduce it later), they need what it is
called at the remote end.


Which is why I included the refnames in my proposal to Peff's second 
example.  Really, the SHA and the remote's name for the SHA are the only 
meaningful things in this case.



I would hesitate to go in the approach based on discarding
information, as if it is the only way to shorten and clarify the
output.  Let's not do so before thinking things through to achieve
the same while keeping the information we give to the users.


I agree, this is not something to undertake lightly.  But I've yet to be 
convinced of the utility of always showing the ref mapping during a fetch.


I actually found fetch's output quite confusing when I first started 
using git.  It's really not obvious that the thing on the left of the 
"->" is the remote's local name, especially since to see what was 
fetched one has to use the thing on the right-hand side -- which is 
obviously in a remote-specific namespace.  (Admittedly, this was before 
checkout learned to search refs/remotes/ for things to check out.)


M.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2 16/22] i18n: rebase-interactive: mark comments of squash for translation

2016-05-26 Thread Junio C Hamano
Vasco Almeida  writes:

> Helper functions this_nth_commit_message and skip_nth_commit_message
> replace the previous method of making the comment messages (such as
> "This is the 2nd commit message:") aided by nth_string helper function.
> This step was taken as a workaround to enabled translation of entire
> sentences. However, doesn't change any text seen in English by the user,
> except for string "The first commit's message is:" which was changed to
> match the style of other instances.

If only the original were written as "This is the commit message
$N", you didn't have to do a lot of work like this, but such is
life.  Thanks for working on this.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2 14/22] i18n: rebase-interactive: mark strings for translation

2016-05-26 Thread Junio C Hamano
Vasco Almeida  writes:

> @@ -222,9 +223,10 @@ has_action () {
>  }
>  
>  is_empty_commit() {
> - tree=$(git rev-parse -q --verify "$1"^{tree} 2>/dev/null ||
> - die "$1: not a commit that can be picked")
> - ptree=$(git rev-parse -q --verify "$1"^^{tree} 2>/dev/null ||
> + sha1=$1
> + tree=$(git rev-parse -q --verify "$sha1"^{tree} 2>/dev/null ||
> + die "$(eval_gettext "\$sha1: not a commit that can be picked")")
> + ptree=$(git rev-parse -q --verify "$sha1"^^{tree} 2>/dev/null ||
>   ptree=4b825dc642cb6eb9a060e54bf8d69288fbee4904)
>   test "$tree" = "$ptree"
>  }

Both of the two callsites of this function use the variable $sha1,
and at least one of them uses that variable after this function
returns, but they pass it as the first parameter to this function,
so the assignment added by this patch does not break them, which is
good.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2 13/22] i18n: git-sh-setup.sh: mark strings for translation

2016-05-26 Thread Junio C Hamano
Vasco Almeida  writes:

>  require_work_tree_exists () {
> + program_name=$0
>   if test "z$(git rev-parse --is-bare-repository)" != zfalse
>   then
> - die "fatal: $0 cannot be used without a working tree."
> + die "$(gettext "fatal: \$program_name cannot be used without a 
> working tree.")"
>   fi
>  }

This is probably quite a minor point, but I'd prefer if clobbering
the variable program_name is done between "then" and "fi", i.e. when
we know we are going to die, so the caller would not care.  Because
we are not in control of the caller's namespace use, and we do not
want bash-ism "local" here, that is the best we could do to make it
safer.

>  require_work_tree () {
> + program_name=$0

Same here.

>   test "$(git rev-parse --is-inside-work-tree 2>/dev/null)" = true ||
> - die "fatal: $0 cannot be used without a working tree."
> + die "$(gettext "fatal: \$program_name cannot be used without a 
> working tree.")"
>  }
>  
>  require_clean_work_tree () {
> + action=$1
> + hint=$2

Likewise.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


What's cooking in git.git (May 2016, #08; Thu, 26)

2016-05-26 Thread Junio C Hamano
Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[Graduated to "master"]

* ar/diff-args-osx-precompose (2016-05-13) 1 commit
  (merged to 'next' on 2016-05-17 at 7b59b79)
 + diff: run arguments through precompose_argv

 Many commands normalize command line arguments from NFD to NFC
 variant of UTF-8 on OSX, but commands in the "diff" family did
 not, causing "git diff $path" to complain that no such path is
 known to Git.  They have been taught to do the normalization.


* da/difftool (2016-05-16) 2 commits
  (merged to 'next' on 2016-05-17 at ef5a435)
 + difftool: handle unmerged files in dir-diff mode
 + difftool: initialize variables for readability

 "git difftool" learned to handle unmerged paths correctly in
 dir-diff mode.


* jc/doc-lint (2016-05-10) 1 commit
  (merged to 'next' on 2016-05-17 at 9032aa5)
 + ci: validate "linkgit:" in documentation

 Find common mistakes when writing gitlink: in our documentation and
 drive the check from "make check-docs".


* jc/rerere-multi (2016-05-19) 2 commits
  (merged to 'next' on 2016-05-19 at 0520c90)
 + rerere: remove an null statement
  (merged to 'next' on 2016-05-13 at f4d1d82)
 + rerere: plug memory leaks upon "rerere forget" failure


* jc/test-parse-options-expect (2016-05-10) 4 commits
  (merged to 'next' on 2016-05-10 at 3ca5783)
 + t0040: convert a few tests to use test-parse-options --expect
 + t0040: remove unused test helpers
 + test-parse-options: --expect= option to simplify tests
 + test-parse-options: fix output when callback option fails
 (this branch uses pb/commit-verbose-config.)

 t0040 had too many unnecessary repetitions in its test data.  Teach
 test-parse-options program so that a caller can tell what it
 expects in its output, so that these repetitions can be cleaned up.


* jk/test-z-n-unquoted (2016-05-14) 6 commits
  (merged to 'next' on 2016-05-17 at 65372cf)
 + always quote shell arguments to test -z/-n
 + t9103: modernize test style
 + t9107: switch inverted single/double quotes in test
 + t9107: use "return 1" instead of "exit 1"
 + t9100,t3419: enclose all test code in single-quotes
 + t/lib-git-svn: drop $remote_git_svn and $git_svn_id

 t9xxx series has been updated primarily for readability, while
 fixing small bugs in it.  A few scripted Porcelains have also been
 updated to fix possible bugs around their use of "test -z" and
 "test -n".


* js/perf-rebase-i (2016-05-13) 3 commits
  (merged to 'next' on 2016-05-13 at eb51ddd)
 + perf: run "rebase -i" under perf
 + perf: make the tests work in worktrees
 + perf: let's disable symlinks when they are not available

 Add perf test for "rebase -i"


* nd/worktree-various-heads (2016-04-22) 13 commits
  (merged to 'next' on 2016-05-10 at 61d3415)
 + branch: do not rename a branch under bisect or rebase
 + worktree.c: check whether branch is bisected in another worktree
 + wt-status.c: split bisect detection out of wt_status_get_state()
 + worktree.c: check whether branch is rebased in another worktree
 + worktree.c: avoid referencing to worktrees[i] multiple times
 + wt-status.c: make wt_status_check_rebase() work on any worktree
 + wt-status.c: split rebase detection out of wt_status_get_state()
 + path.c: refactor and add worktree_git_path()
 + worktree.c: mark current worktree
 + worktree.c: make find_shared_symref() return struct worktree *
 + worktree.c: store "id" instead of "git_dir"
 + path.c: add git_common_path() and strbuf_git_common_path()
 + dir.c: rename str(n)cmp_icase to fspath(n)cmp
 (this branch is used by nd/worktree-cleanup-post-head-protection.)

 The experimental "multiple worktree" feature gains more safety to
 forbid operations on a branch that is checked out or being actively
 worked on elsewhere, by noticing that e.g. it is being rebased.


* pb/commit-verbose-config (2016-05-10) 7 commits
 + commit: add a commit.verbose config variable
 + t7507-commit-verbose: improve test coverage by testing number of diffs
 + parse-options.c: make OPTION_COUNTUP respect "unspecified" values
 + t/t7507: improve test coverage
 + t0040-parse-options: improve test coverage
 + test-parse-options: print quiet as integer
 + t0040-test-parse-options.sh: fix style issues
 (this branch is used by jc/test-parse-options-expect.)

 "git commit" learned to pay attention to "commit.verbose"
 configuration variable and act as if "--verbose" option was
 given from the command line.


* ss/commit-dry-run-resolve-merge-to-no-op (2016-02-17) 1 commit
  (merged to 'next' on 2016-05-10 at 2ada404)
 + wt-status.c: set commitable bit if there is a meaningful me

Re: [PATCH 4/6] config: return configset value for current_config_ functions

2016-05-26 Thread Jeff King
On Thu, May 26, 2016 at 12:50:33PM -0400, Jeff King wrote:

> > Remind me why we end up running ./test-config instead of
> > ./bin-wrappers/test-config?  Should our tests be running 
> > bin-wrappers early in their $PATH, perhaps?
> 
> The problem is running test-config inside of a git alias. The
> bin-wrappers will set the exec-path to the root-level of git's build
> directory, which the git binary will then stick at the front of the
> $PATH.
> 
> So the simplest solution really is: don't do that. The only debate in my
> mind is whether this is rare enough that it won't bite somebody again in
> the future, or if we should look into a solution that makes this Just
> Work.

That being said, it's easy enough to work around this one case, so we
don't have to stall the topic thinking about this (and maybe if it never
comes up again, we can just not think about it further :) ).

So here's a replacement patch for patch 4 of jk/upload-pack-hook (the
others are untouched, and don't even have conflicts rebasing on top).

I went with just setting $GIT_CONFIG_PARAMETERS. The full-path thing Duy
suggested would work, and you can avoid "which" by just pointing to
$TEST_DIRECTORY/helper/test-config. But besides being slightly brittle,
you also have to jump through some hoops to handle a $TEST_DIRECTORY
with spaces in it. The solution here is pretty straightforward, I think.

-- >8 --
Subject: config: return configset value for current_config_ functions

When 473166b (config: add 'origin_type' to config_source
struct, 2016-02-19) added accessor functions for the origin
type and name, it taught them only to look at the "cf"
struct that is filled in while we are parsing the config.
This is sufficient to make it work with git-config, which
uses git_config_with_options() under the hood. That function
freshly parses the config files and triggers the callback
when it parses each key.

Most git programs, however, use git_config(). This interface
will populate a cache during the actual parse, and then
serve values from the cache. Calling current_config_filename()
in a callback here will find a NULL cf and produce an error.
There are no such callers right now, but let's prepare for
adding some by making this work.

We already record source information in a struct attached to
each value. We just need to make it globally available and
then consult it from the accessor functions.

Signed-off-by: Jeff King 
---
 cache.h|  1 +
 config.c   | 51 +-
 t/helper/test-config.c | 20 
 t/t1308-config-set.sh  | 24 
 4 files changed, 87 insertions(+), 9 deletions(-)

diff --git a/cache.h b/cache.h
index 6049f86..1bce212 100644
--- a/cache.h
+++ b/cache.h
@@ -1696,6 +1696,7 @@ extern int ignore_untracked_cache_config;
 struct key_value_info {
const char *filename;
int linenr;
+   const char *origin_type;
 };
 
 extern NORETURN void git_die_config(const char *key, const char *err, ...) 
__attribute__((format(printf, 2, 3)));
diff --git a/config.c b/config.c
index 571151f..d555dee 100644
--- a/config.c
+++ b/config.c
@@ -38,7 +38,24 @@ struct config_source {
long (*do_ftell)(struct config_source *c);
 };
 
+/*
+ * These variables record the "current" config source, which
+ * can be accessed by parsing callbacks.
+ *
+ * The "cf" variable will be non-NULL only when we are actually parsing a real
+ * config source (file, blob, cmdline, etc).
+ *
+ * The "current_config_kvi" variable will be non-NULL only when we are feeding
+ * cached config from a configset into a callback.
+ *
+ * They should generally never be non-NULL at the same time. If they are both
+ * NULL, then we aren't parsing anything (and depending on the function looking
+ * at the variables, it's either a bug for it to be called in the first place,
+ * or it's a function which can be reused for non-config purposes, and should
+ * fall back to some sane behavior).
+ */
 static struct config_source *cf;
+static struct key_value_info *current_config_kvi;
 
 static int zlib_compression_seen;
 
@@ -1284,16 +1301,20 @@ static void configset_iter(struct config_set *cs, 
config_fn_t fn, void *data)
struct string_list *values;
struct config_set_element *entry;
struct configset_list *list = &cs->list;
-   struct key_value_info *kv_info;
 
for (i = 0; i < list->nr; i++) {
entry = list->items[i].e;
value_index = list->items[i].value_index;
values = &entry->value_list;
-   if (fn(entry->key, values->items[value_index].string, data) < 
0) {
-   kv_info = values->items[value_index].util;
-   git_die_config_linenr(entry->key, kv_info->filename, 
kv_info->linenr);
-   }
+
+   current_config_kvi = values->items[value_index].util;
+
+   if (fn(entry->key, values->items[value_index].string,

Re: What's cooking in git.git (May 2016, #08; Thu, 26)

2016-05-26 Thread Jeff King
On Thu, May 26, 2016 at 03:50:14PM -0700, Junio C Hamano wrote:

> * jk/upload-pack-hook (2016-05-24) 6 commits
>  - upload-pack: provide a hook for running pack-objects
>  - config: add a notion of "scope"
>  - config: return configset value for current_config_ functions
>  - config: set up config_source for command-line config
>  - git_config_parse_parameter: refactor cleanup code
>  - git_config_with_options: drop "found" counting
> 
>  Allow a custom "git upload-pack" replacement to respond to
>  "fetch/clone" request.
> 
>  Will merge to 'next'.

I just sent a replacement for the fourth patch that avoids the t/helper
problem. It's probably worth dealing with before even hitting "next" so
as not to break bisection.

You should probably hold off on merging the top one. The discussion
stalled because I was on vacation, but it has resumed now (the earlier
refactoring bits are uncontroversial, I think).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 4/6] config: return configset value for current_config_ functions

2016-05-26 Thread Jeff King
On Thu, May 26, 2016 at 10:36:44AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > The problem is running test-config inside of a git alias. The
> > bin-wrappers will set the exec-path to the root-level of git's build
> > directory, which the git binary will then stick at the front of the
> > $PATH.
> 
> I was wondering why exec-path does not point at bin-wrappers in the
> first place.
> 
> A wrapper script needs to know where the real thing lives in order
> to "exec" (or "exec gdb") anyway, and it hardcodes the path to it.
> It happens to use GIT_EXEC_PATH to shorten the hardcoded path it
> uses when it does "exec", but it does not have to.
> 
> Wouldn't we want to see "git" use any of these wrapped ones when it
> invokes a non-builtin subcommand when it does so normally, honoring
> GIT_EXEC_PATH?  Pointing GIT_EXEC_PATH at the top-level means that
> wrappers are bypassed for such an invocation (if what is run happens
> to have executable at the top-level), and possibly a totally wrong
> thing is run (when we start generating the binaries in different
> directories, which is what we are seeing here).

I think the issue is that bin-wrappers serves two purposes. One is for
testing, but the other is for people who run git directly without
installing. For us to set GIT_EXEC_PATH to bin-wrappers, it would have
to have all of the git-* external programs, which would then put them
all in the $PATH of people doing the no-install thing.

That's certainly not insurmountable. Either we can tell them to live
with it, or we can break out a separate wrapper directory that serves as
a pseudo-exec-path.

> > So the simplest solution really is: don't do that. The only debate
> > in my mind is whether this is rare enough that it won't bite
> > somebody again in the future, or if we should look into a solution
> > that makes this Just Work.
> 
> I think it was you who alluded to revamping the test framework along
> the lines of preparing a "test installation" (aka "make install"
> with DESTDIR set to somewhere) and making bin-wrappers to point into
> that installation (or if we are testing an installed Git that may be
> different from what we have source for, that final installed
> location).  An installed version of Git will not have test-* helpers
> so they need to come from a freshly built source tree, not from
> "test installation" or "installed Git".  There may be other details
> that need to be worked out, but as a longer term direction that may
> not be a bad idea.

I think you can make it even simpler by not really doing a "make
install", but just linking or bin-wrappering a fake exec-path. It would
be great if we could truly just "make install" into a fake area and test
that (dropping bin-wrappers entirely), but git cares too much about some
hard-coded paths, I think. We'd have to first have a truly relocatable
binary.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 4/6] config: return configset value for current_config_ functions

2016-05-26 Thread Junio C Hamano
Jeff King  writes:

> I think the issue is that bin-wrappers serves two purposes. One is for
> testing, but the other is for people who run git directly without
> installing.

Hmph.  It may have been a useful way to "run without installing"
once in the past, but with the "check and run it under GDB" etc.,
I am not sure if it still is.  I certainly did not think about that
as a valid use case when I wrote the message you are responding to.

> I think you can make it even simpler by not really doing a "make
> install", but just linking or bin-wrappering a fake exec-path. It would
> be great if we could truly just "make install" into a fake area and test
> that (dropping bin-wrappers entirely), but git cares too much about some
> hard-coded paths, I think. We'd have to first have a truly relocatable
> binary.

Looking at what bin-wrappers do, I do not think "hard coded paths"
is something we need them for.  If we wanted to make the "test what
we just built" and the "test what is already installed" closely
mimic each other, I have a feeling that setting of the environment
variable done by the bin-wrappers can and should be moved to the
test framework.  When testing what we just built, set these
environment variables you see in any of the bin-wrappers/ script to
point at various places in the "make DESTDIR=there install" tree,
and when testing what is installed, set them to different values
(possibly "nothing", e.g. GIT_EXEC_PATH would not be needed if we
are testing an installed and already working version).

So I suspect "truly relocatable binary" is necessary for testing.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: What's cooking in git.git (May 2016, #08; Thu, 26)

2016-05-26 Thread Junio C Hamano
Jeff King  writes:

> I just sent a replacement for the fourth patch that avoids the t/helper
> problem. It's probably worth dealing with before even hitting "next" so
> as not to break bisection.
>
> You should probably hold off on merging the top one. The discussion
> stalled because I was on vacation, but it has resumed now (the earlier
> refactoring bits are uncontroversial, I think).

Thanks, will do both.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v8 6/9] connect: make parse_connect_url() return the user part of the url as a separate value

2016-05-26 Thread Mike Hommey
Signed-off-by: Mike Hommey 
---
 connect.c | 54 --
 1 file changed, 36 insertions(+), 18 deletions(-)

diff --git a/connect.c b/connect.c
index 4be06f4..0819c25 100644
--- a/connect.c
+++ b/connect.c
@@ -588,11 +588,13 @@ static char *get_port(char *host)
  * Extract protocol and relevant parts from the specified connection URL.
  * The caller must free() the returned strings.
  */
-static enum protocol parse_connect_url(const char *url_orig, char **ret_host,
-  char **ret_port, char **ret_path)
+static enum protocol parse_connect_url(const char *url_orig, char **ret_user,
+  char **ret_host, char **ret_port,
+  char **ret_path)
 {
char *url;
char *host, *path;
+   const char *user = NULL;
const char *port = NULL;
char *end;
int separator = '/';
@@ -650,23 +652,31 @@ static enum protocol parse_connect_url(const char 
*url_orig, char **ret_host,
 
get_host_and_port(&host, &port);
 
-   if (*host && !port) {
+   if (*host) {
/*
-* get_host_and_port does not return a port in the
-* [host:port]:path case. In that case, it is called with
-* "[host:port]" and returns "host:port" and NULL.
-* To support this undocumented legacy we still need to split
-* the port.
-* "host:port" may also look like "user@host:port". As the
-* `user` portion tends to be less strict than `host:port`,
-* we first put it out of the equation: since a hostname
-* cannot contain a '@', we start from the last '@' in the
-* string.
+* At this point, the host part may look like user@host:port.
+* As the `user` portion tends to be less strict than
+* `host:port`, we first put it out of the equation: since a
+* hostname cannot contain a '@', we start from the last '@'
+* in the string.
 */
char *end_user = strrchr(host, '@');
-   port = get_port(end_user ? end_user : host);
+   if (end_user) {
+   *end_user = '\0';
+   user = host;
+   host = end_user + 1;
+   }
}
+   /*
+* get_host_and_port does not return a port in the [host:port]:path
+* case. In that case, it is called with "[host:port]" and returns
+* "host:port" and NULL.
+* To support this undocumented legacy we still need to split the port.
+*/
+   if (!port)
+   port = get_port(host);
 
+   *ret_user = user ? xstrdup(user) : NULL;
*ret_host = xstrdup(host);
*ret_port = port ? xstrdup(port) : NULL;
*ret_path = path;
@@ -690,7 +700,7 @@ static struct child_process no_fork = CHILD_PROCESS_INIT;
 struct child_process *git_connect(int fd[2], const char *url,
  const char *prog, int flags)
 {
-   char *host, *port, *path;
+   char *user, *host, *port, *path;
struct child_process *conn = &no_fork;
enum protocol protocol;
struct strbuf cmd = STRBUF_INIT;
@@ -700,11 +710,14 @@ struct child_process *git_connect(int fd[2], const char 
*url,
 */
signal(SIGCHLD, SIG_DFL);
 
-   protocol = parse_connect_url(url, &host, &port, &path);
+   protocol = parse_connect_url(url, &user, &host, &port, &path);
if (flags & CONNECT_DIAG_URL) {
printf("Diag: url=%s\n", url ? url : "NULL");
printf("Diag: protocol=%s\n", prot_name(protocol));
-   printf("Diag: userandhost=%s\n", host ? host : "NULL");
+   if (user)
+   printf("Diag: userandhost=%s@%s\n", user, host);
+   else
+   printf("Diag: userandhost=%s\n", host ? host : "NULL");
printf("Diag: port=%s\n", port ? port : "NONE");
printf("Diag: path=%s\n", path ? path : "NULL");
conn = NULL;
@@ -813,7 +826,11 @@ struct child_process *git_connect(int fd[2], const char 
*url,
argv_array_push(&conn->args, putty ? "-P" : 
"-p");
argv_array_push(&conn->args, port);
}
-   argv_array_push(&conn->args, host);
+   if (user)
+   argv_array_pushf(&conn->args, "%s@%s",
+user, host);
+   else
+   argv_array_push(&conn->args, host);
} else {
transport_check_allowed("file");
}
@@ -826,6 +843,7 @@ struct child_process *git_connect(int fd[2], 

[PATCH v8 2/9] connect: call get_host_and_port() earlier

2016-05-26 Thread Mike Hommey
Currently, get_host_and_port() is called in git_connect() for the ssh
protocol, and in git_tcp_connect_sock() for the git protocol. Instead
of doing this, just call it from a single place, right after
parse_connect_url(), and pass the host and port separately to
git_*_connect() functions.

We however preserve hostandport, at least for now.

Note that in git_tcp_connect_sock, the port was set to "" in a
case that never can happen, so that code path was removed.

Signed-off-by: Mike Hommey 
---
 connect.c | 47 +++
 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/connect.c b/connect.c
index 6e520c3..d5af65f 100644
--- a/connect.c
+++ b/connect.c
@@ -343,18 +343,16 @@ static const char *ai_name(const struct addrinfo *ai)
 /*
  * Returns a connected socket() fd, or else die()s.
  */
-static int git_tcp_connect_sock(char *host, int flags)
+static int git_tcp_connect_sock(const char *host, const char *port, int flags)
 {
struct strbuf error_message = STRBUF_INIT;
int sockfd = -1;
-   const char *port = STR(DEFAULT_GIT_PORT);
struct addrinfo hints, *ai0, *ai;
int gai;
int cnt = 0;
 
-   get_host_and_port(&host, &port);
-   if (!*port)
-   port = "";
+   if (!port)
+   port = STR(DEFAULT_GIT_PORT);
 
memset(&hints, 0, sizeof(hints));
if (flags & CONNECT_IPV4)
@@ -411,11 +409,10 @@ static int git_tcp_connect_sock(char *host, int flags)
 /*
  * Returns a connected socket() fd, or else die()s.
  */
-static int git_tcp_connect_sock(char *host, int flags)
+static int git_tcp_connect_sock(const char *host, const char *port, int flags)
 {
struct strbuf error_message = STRBUF_INIT;
int sockfd = -1;
-   const char *port = STR(DEFAULT_GIT_PORT);
char *ep;
struct hostent *he;
struct sockaddr_in sa;
@@ -423,7 +420,8 @@ static int git_tcp_connect_sock(char *host, int flags)
unsigned int nport;
int cnt;
 
-   get_host_and_port(&host, &port);
+   if (!port)
+   port = STR(DEFAULT_GIT_PORT);
 
if (flags & CONNECT_VERBOSE)
fprintf(stderr, "Looking up %s ... ", host);
@@ -482,9 +480,10 @@ static int git_tcp_connect_sock(char *host, int flags)
 #endif /* NO_IPV6 */
 
 
-static void git_tcp_connect(int fd[2], char *host, int flags)
+static void git_tcp_connect(int fd[2], const char *host, const char *port,
+   int flags)
 {
-   int sockfd = git_tcp_connect_sock(host, flags);
+   int sockfd = git_tcp_connect_sock(host, port, flags);
 
fd[0] = sockfd;
fd[1] = dup(sockfd);
@@ -550,18 +549,16 @@ static int git_use_proxy(const char *host)
return (git_proxy_command && *git_proxy_command);
 }
 
-static struct child_process *git_proxy_connect(int fd[2], char *host)
+static struct child_process *git_proxy_connect(int fd[2], const char *host,
+  const char *port)
 {
-   const char *port = STR(DEFAULT_GIT_PORT);
struct child_process *proxy;
 
-   get_host_and_port(&host, &port);
-
proxy = xmalloc(sizeof(*proxy));
child_process_init(proxy);
argv_array_push(&proxy->args, git_proxy_command);
argv_array_push(&proxy->args, host);
-   argv_array_push(&proxy->args, port);
+   argv_array_push(&proxy->args, port ? port : STR(DEFAULT_GIT_PORT));
proxy->in = -1;
proxy->out = -1;
if (start_command(proxy))
@@ -672,7 +669,8 @@ static struct child_process no_fork = CHILD_PROCESS_INIT;
 struct child_process *git_connect(int fd[2], const char *url,
  const char *prog, int flags)
 {
-   char *hostandport, *path;
+   char *hostandport, *path, *host;
+   const char *port = NULL;
struct child_process *conn = &no_fork;
enum protocol protocol;
struct strbuf cmd = STRBUF_INIT;
@@ -683,6 +681,8 @@ struct child_process *git_connect(int fd[2], const char 
*url,
signal(SIGCHLD, SIG_DFL);
 
protocol = parse_connect_url(url, &hostandport, &path);
+   host = xstrdup(hostandport);
+   get_host_and_port(&host, &port);
if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
printf("Diag: url=%s\n", url ? url : "NULL");
printf("Diag: protocol=%s\n", prot_name(protocol));
@@ -707,9 +707,9 @@ struct child_process *git_connect(int fd[2], const char 
*url,
 * cannot connect.
 */
if (git_use_proxy(hostandport))
-   conn = git_proxy_connect(fd, hostandport);
+   conn = git_proxy_connect(fd, host, port);
else
-   git_tcp_connect(fd, hostandport, flags);
+   git_tcp_connect(fd, host, port, flags);
/*
 * Separate original protocol components prog and

[PATCH v8 3/9] connect: re-derive a host:port string from the separate host and port variables

2016-05-26 Thread Mike Hommey
The last uses of the hostandport variable, besides being strdup'ed
before being split into host and port, is to fill the host header in the
git protocol and to test whether to proxy the request.

Instead of relying on parse_connect_url() to return a host:port string
that makes sense there, re-derive one from the host and port variables.
This will allow to refactor parse_connect_url() to return separate host
and port strings.

Signed-off-by: Mike Hommey 
---
 connect.c | 31 +++
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/connect.c b/connect.c
index d5af65f..3d7bd8e 100644
--- a/connect.c
+++ b/connect.c
@@ -695,18 +695,32 @@ struct child_process *git_connect(int fd[2], const char 
*url,
 * connect, unless the user has overridden us in
 * the environment.
 */
-   char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
-   if (target_host)
-   target_host = xstrdup(target_host);
-   else
-   target_host = xstrdup(hostandport);
+   struct strbuf target_host = STRBUF_INIT;
+   struct strbuf virtual_host = STRBUF_INIT;
+   const char *colon = strchr(host, ':');
+   char *override_vhost = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
+
+   /* If the host contains a colon (ipv6 address), it needs to
+* be enclosed with square brackets. */
+   if (colon)
+   strbuf_addch(&target_host, '[');
+   strbuf_addstr(&target_host, host);
+   if (colon)
+   strbuf_addch(&target_host, ']');
+   if (port) {
+   strbuf_addch(&target_host, ':');
+   strbuf_addstr(&target_host, port);
+   }
+
+   strbuf_addstr(&virtual_host, override_vhost ? override_vhost
+   : target_host.buf);
 
transport_check_allowed("git");
 
/* These underlying connection commands die() if they
 * cannot connect.
 */
-   if (git_use_proxy(hostandport))
+   if (git_use_proxy(target_host.buf))
conn = git_proxy_connect(fd, host, port);
else
git_tcp_connect(fd, host, port, flags);
@@ -720,8 +734,9 @@ struct child_process *git_connect(int fd[2], const char 
*url,
packet_write(fd[1],
 "%s %s%chost=%s%c",
 prog, path, 0,
-target_host, 0);
-   free(target_host);
+virtual_host.buf, 0);
+   strbuf_release(&virtual_host);
+   strbuf_release(&target_host);
} else {
conn = xmalloc(sizeof(*conn));
child_process_init(conn);
-- 
2.8.3

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v8 0/9] connect: various cleanups

2016-05-26 Thread Mike Hommey
Changes from v7:
- Fixed comments.

Mike Hommey (9):
  connect: document why we sometimes call get_port after
get_host_and_port
  connect: call get_host_and_port() earlier
  connect: re-derive a host:port string from the separate host and port
variables
  connect: make parse_connect_url() return separated host and port
  connect: group CONNECT_DIAG_URL handling code
  connect: make parse_connect_url() return the user part of the url as a
separate value
  connect: change the --diag-url output to separate user and host
  connect: actively reject git:// urls with a user part
  connect: move ssh command line preparation to a separate function

 connect.c | 235 +-
 t/t5500-fetch-pack.sh |  42 ++---
 2 files changed, 170 insertions(+), 107 deletions(-)

-- 
2.8.3

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v8 4/9] connect: make parse_connect_url() return separated host and port

2016-05-26 Thread Mike Hommey
Now that nothing besides CONNECT_DIAG_URL is using hostandport, we can
have parse_connect_url() itself do the host and port splitting.

This still leaves "user@" part of the host, if there is one, which will
be addressed in a subsequent change. This however does add /some/
handling of the "user@" part of the host, in order to pick the port
properly.

Signed-off-by: Mike Hommey 
---
 connect.c | 47 ---
 t/t5500-fetch-pack.sh | 32 +---
 2 files changed, 49 insertions(+), 30 deletions(-)

diff --git a/connect.c b/connect.c
index 3d7bd8e..de7419e 100644
--- a/connect.c
+++ b/connect.c
@@ -589,10 +589,11 @@ static char *get_port(char *host)
  * The caller must free() the returned strings.
  */
 static enum protocol parse_connect_url(const char *url_orig, char **ret_host,
-  char **ret_path)
+  char **ret_port, char **ret_path)
 {
char *url;
char *host, *path;
+   const char *port = NULL;
char *end;
int separator = '/';
enum protocol protocol = PROTO_LOCAL;
@@ -647,7 +648,27 @@ static enum protocol parse_connect_url(const char 
*url_orig, char **ret_host,
path = xstrdup(path);
*end = '\0';
 
+   get_host_and_port(&host, &port);
+
+   if (*host && !port) {
+   /*
+* get_host_and_port does not return a port in the
+* [host:port]:path case. In that case, it is called with
+* "[host:port]" and returns "host:port" and NULL.
+* To support this undocumented legacy we still need to split
+* the port.
+* "host:port" may also look like "user@host:port". As the
+* `user` portion tends to be less strict than `host:port`,
+* we first put it out of the equation: since a hostname
+* cannot contain a '@', we start from the last '@' in the
+* string.
+*/
+   char *end_user = strrchr(host, '@');
+   port = get_port(end_user ? end_user : host);
+   }
+
*ret_host = xstrdup(host);
+   *ret_port = port ? xstrdup(port) : NULL;
*ret_path = path;
free(url);
return protocol;
@@ -669,8 +690,7 @@ static struct child_process no_fork = CHILD_PROCESS_INIT;
 struct child_process *git_connect(int fd[2], const char *url,
  const char *prog, int flags)
 {
-   char *hostandport, *path, *host;
-   const char *port = NULL;
+   char *host, *port, *path;
struct child_process *conn = &no_fork;
enum protocol protocol;
struct strbuf cmd = STRBUF_INIT;
@@ -680,13 +700,12 @@ struct child_process *git_connect(int fd[2], const char 
*url,
 */
signal(SIGCHLD, SIG_DFL);
 
-   protocol = parse_connect_url(url, &hostandport, &path);
-   host = xstrdup(hostandport);
-   get_host_and_port(&host, &port);
+   protocol = parse_connect_url(url, &host, &port, &path);
if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
printf("Diag: url=%s\n", url ? url : "NULL");
printf("Diag: protocol=%s\n", prot_name(protocol));
-   printf("Diag: hostandport=%s\n", hostandport ? hostandport : 
"NULL");
+   printf("Diag: userandhost=%s\n", host ? host : "NULL");
+   printf("Diag: port=%s\n", port ? port : "NONE");
printf("Diag: path=%s\n", path ? path : "NULL");
conn = NULL;
} else if (protocol == PROTO_GIT) {
@@ -754,16 +773,6 @@ struct child_process *git_connect(int fd[2], const char 
*url,
int putty = 0, tortoiseplink = 0;
transport_check_allowed("ssh");
 
-   /*
-* get_host_and_port does not return a port in the
-* [host:port]:path case. In that case, it is called
-* with "[host:port]" and returns "host:port" and NULL.
-* To support this undocumented legacy we still need
-* to split the port.
-*/
-   if (!port)
-   port = get_port(host);
-
if (flags & CONNECT_DIAG_URL) {
printf("Diag: url=%s\n", url ? url : "NULL");
printf("Diag: protocol=%s\n", 
prot_name(protocol));
@@ -771,8 +780,8 @@ struct child_process *git_connect(int fd[2], const char 
*url,
printf("Diag: port=%s\n", port ? port : "NONE");
printf("Diag: path=%s\n", path ? path : "NULL");
 
-   free(hostandport);
free(host);
+   free(port);
 

[PATCH v8 9/9] connect: move ssh command line preparation to a separate function

2016-05-26 Thread Mike Hommey
Signed-off-by: Mike Hommey 
---
 connect.c | 108 +-
 1 file changed, 58 insertions(+), 50 deletions(-)

diff --git a/connect.c b/connect.c
index 9aea3cd..076ae09 100644
--- a/connect.c
+++ b/connect.c
@@ -684,6 +684,61 @@ static enum protocol parse_connect_url(const char 
*url_orig, char **ret_user,
return protocol;
 }
 
+static int prepare_ssh_command(struct argv_array *cmd, const char *user,
+  const char *host, const char *port, int flags)
+{
+   const char *ssh;
+   int putty = 0, tortoiseplink = 0, use_shell = 1;
+   transport_check_allowed("ssh");
+
+   ssh = getenv("GIT_SSH_COMMAND");
+   if (!ssh) {
+   const char *base;
+   char *ssh_dup;
+
+   /*
+* GIT_SSH is the no-shell version of
+* GIT_SSH_COMMAND (and must remain so for
+* historical compatibility).
+*/
+   use_shell = 0;
+
+   ssh = getenv("GIT_SSH");
+   if (!ssh)
+   ssh = "ssh";
+
+   ssh_dup = xstrdup(ssh);
+   base = basename(ssh_dup);
+
+   tortoiseplink = !strcasecmp(base, "tortoiseplink") ||
+   !strcasecmp(base, "tortoiseplink.exe");
+   putty = tortoiseplink ||
+   !strcasecmp(base, "plink") ||
+   !strcasecmp(base, "plink.exe");
+
+   free(ssh_dup);
+   }
+
+   argv_array_push(cmd, ssh);
+   if (flags & CONNECT_IPV4)
+   argv_array_push(cmd, "-4");
+   else if (flags & CONNECT_IPV6)
+   argv_array_push(cmd, "-6");
+   if (tortoiseplink)
+   argv_array_push(cmd, "-batch");
+   if (port) {
+   /* P is for PuTTY, p is for OpenSSH */
+   argv_array_push(cmd, putty ? "-P" : "-p");
+   argv_array_push(cmd, port);
+   }
+   if (user)
+   argv_array_pushf(cmd, "%s@%s", user, host);
+   else
+   argv_array_push(cmd, host);
+
+   return use_shell;
+}
+
 static struct child_process no_fork = CHILD_PROCESS_INIT;
 
 /*
@@ -780,59 +835,12 @@ struct child_process *git_connect(int fd[2], const char 
*url,
 
/* remove repo-local variables from the environment */
conn->env = local_repo_env;
-   conn->use_shell = 1;
conn->in = conn->out = -1;
if (protocol == PROTO_SSH) {
-   const char *ssh;
-   int putty = 0, tortoiseplink = 0;
-   transport_check_allowed("ssh");
-
-   ssh = getenv("GIT_SSH_COMMAND");
-   if (!ssh) {
-   const char *base;
-   char *ssh_dup;
-
-   /*
-* GIT_SSH is the no-shell version of
-* GIT_SSH_COMMAND (and must remain so for
-* historical compatibility).
-*/
-   conn->use_shell = 0;
-
-   ssh = getenv("GIT_SSH");
-   if (!ssh)
-   ssh = "ssh";
-
-   ssh_dup = xstrdup(ssh);
-   base = basename(ssh_dup);
-
-   tortoiseplink = !strcasecmp(base, 
"tortoiseplink") ||
-   !strcasecmp(base, "tortoiseplink.exe");
-   putty = tortoiseplink ||
-   !strcasecmp(base, "plink") ||
-   !strcasecmp(base, "plink.exe");
-
-   free(ssh_dup);
-   }
-
-   argv_array_push(&conn->args, ssh);
-   if (flags & CONNECT_IPV4)
-   argv_array_push(&conn->args, "-4");
-   else if (flags & CONNECT_IPV6)
-   argv_array_push(&conn->args, "-6");
-   if (tortoiseplink)
-   argv_array_push(&conn->args, "-batch");
-   if (port) {
-   /* P is for PuTTY, p is for OpenSSH */
-   argv_array_push(&conn->args, putty ? "-P" : 
"-p");
-   argv_array_push(&conn->args, port);
-   }
-   if (user)
-   argv_array_pushf(&conn->args, "%s@%s",
-user, host);
-   else
-   argv_array_push(&conn->args, host);
+   conn->use_shell = prepare_ssh_command(
+

[PATCH v8 8/9] connect: actively reject git:// urls with a user part

2016-05-26 Thread Mike Hommey
Currently, urls of the for git://user@host don't work because user@host
is not resolving at the DNS level, but we shouldn't be relying on it
being an invalid host name, and actively reject it for containing a
username in the first place.

Signed-off-by: Mike Hommey 
---
 connect.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/connect.c b/connect.c
index 0c4d23b..9aea3cd 100644
--- a/connect.c
+++ b/connect.c
@@ -730,6 +730,9 @@ struct child_process *git_connect(int fd[2], const char 
*url,
const char *colon = strchr(host, ':');
char *override_vhost = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
 
+   if (user)
+   die("user@host is not allowed in git:// urls");
+
/* If the host contains a colon (ipv6 address), it needs to
 * be enclosed with square brackets. */
if (colon)
-- 
2.8.3

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v8 1/9] connect: document why we sometimes call get_port after get_host_and_port

2016-05-26 Thread Mike Hommey
Signed-off-by: Mike Hommey 
---
 connect.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/connect.c b/connect.c
index c53f3f1..6e520c3 100644
--- a/connect.c
+++ b/connect.c
@@ -742,6 +742,13 @@ struct child_process *git_connect(int fd[2], const char 
*url,
transport_check_allowed("ssh");
get_host_and_port(&ssh_host, &port);
 
+   /*
+* get_host_and_port does not return a port in the
+* [host:port]:path case. In that case, it is called
+* with "[host:port]" and returns "host:port" and NULL.
+* To support this undocumented legacy we still need
+* to split the port.
+*/
if (!port)
port = get_port(ssh_host);
 
-- 
2.8.3

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v8 5/9] connect: group CONNECT_DIAG_URL handling code

2016-05-26 Thread Mike Hommey
Previous changes made both branches handling CONNECT_DIAG_URL identical.
We can now remove one of those branches and have CONNECT_DIAG_URL be
handled in one place.

Signed-off-by: Mike Hommey 
---
 connect.c | 16 +---
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/connect.c b/connect.c
index de7419e..4be06f4 100644
--- a/connect.c
+++ b/connect.c
@@ -701,7 +701,7 @@ struct child_process *git_connect(int fd[2], const char 
*url,
signal(SIGCHLD, SIG_DFL);
 
protocol = parse_connect_url(url, &host, &port, &path);
-   if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
+   if (flags & CONNECT_DIAG_URL) {
printf("Diag: url=%s\n", url ? url : "NULL");
printf("Diag: protocol=%s\n", prot_name(protocol));
printf("Diag: userandhost=%s\n", host ? host : "NULL");
@@ -773,20 +773,6 @@ struct child_process *git_connect(int fd[2], const char 
*url,
int putty = 0, tortoiseplink = 0;
transport_check_allowed("ssh");
 
-   if (flags & CONNECT_DIAG_URL) {
-   printf("Diag: url=%s\n", url ? url : "NULL");
-   printf("Diag: protocol=%s\n", 
prot_name(protocol));
-   printf("Diag: userandhost=%s\n", host ? host : 
"NULL");
-   printf("Diag: port=%s\n", port ? port : "NONE");
-   printf("Diag: path=%s\n", path ? path : "NULL");
-
-   free(host);
-   free(port);
-   free(path);
-   free(conn);
-   return NULL;
-   }
-
ssh = getenv("GIT_SSH_COMMAND");
if (!ssh) {
const char *base;
-- 
2.8.3

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v8 7/9] connect: change the --diag-url output to separate user and host

2016-05-26 Thread Mike Hommey
Signed-off-by: Mike Hommey 
---
 connect.c |  6 ++
 t/t5500-fetch-pack.sh | 14 --
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/connect.c b/connect.c
index 0819c25..0c4d23b 100644
--- a/connect.c
+++ b/connect.c
@@ -714,10 +714,8 @@ struct child_process *git_connect(int fd[2], const char 
*url,
if (flags & CONNECT_DIAG_URL) {
printf("Diag: url=%s\n", url ? url : "NULL");
printf("Diag: protocol=%s\n", prot_name(protocol));
-   if (user)
-   printf("Diag: userandhost=%s@%s\n", user, host);
-   else
-   printf("Diag: userandhost=%s\n", host ? host : "NULL");
+   printf("Diag: user=%s\n", user ? user : "NULL");
+   printf("Diag: host=%s\n", host ? host : "NULL");
printf("Diag: port=%s\n", port ? port : "NONE");
printf("Diag: path=%s\n", path ? path : "NULL");
conn = NULL;
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 739c6b1..2d9c4be 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -553,7 +553,7 @@ check_prot_path () {
Diag: protocol=$2
Diag: path=$3
EOF
-   git fetch-pack --diag-url "$1" | egrep -v '(host|port)=' >actual &&
+   git fetch-pack --diag-url "$1" | egrep -v "(user|host|port)=" >actual &&
test_cmp expected actual
 }
 
@@ -568,10 +568,20 @@ check_prot_host_port_path () {
;;
esac
ehost=$(echo $3 | tr -d "[]")
+   case "$ehost" in
+   *@*)
+   user=${ehost%@*}
+   ehost=${ehost#$user@}
+   ;;
+   *)
+   user=NULL
+   ;;
+   esac
cat >exp <<-EOF &&
Diag: url=$1
Diag: protocol=$pp
-   Diag: userandhost=$ehost
+   Diag: user=$user
+   Diag: host=$ehost
Diag: port=$4
Diag: path=$5
EOF
-- 
2.8.3

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC/PATCH 0/2] Introduce "log.showSignature" config variable

2016-05-26 Thread Austin English
On Thu, May 26, 2016 at 8:06 AM, Mehul Jain  wrote:
> Add a new configuratation variable "log.showSignature" for git-log and
> git-show. "log.showSignature=true" will enable user to see GPG signature
> by default while using git-log and git-show.
>
> [Patch 1/2] introduce the config variable along with some tests.
> [Patch 2/2] tackles the problem: what if user wants to disable the
> setting of "log.showSignature=true" using a command line
> switch.
>
> Previous discussion on this: 
> http://thread.gmane.org/gmane.comp.version-control.git/295405
>
> Mehul Jain (2):
>   log: add "log.showsignature" configuration variable
>   log: add "--no-show-signature" command line option
>
>  Documentation/git-log.txt |  4 
>  builtin/log.c |  6 ++
>  revision.c|  2 ++
>  t/t4202-log.sh| 26 ++
>  t/t7510-signed-commit.sh  |  7 +++
>  5 files changed, 45 insertions(+)
>
> --
> 2.9.0.rc0.dirty
>

Hi Mehul,

Thanks for working on this. With your patch and git config
log.showsignature = true, git log and git show now display signatures
for me:

austin@debian-laptop:~/src/winetricks$ ~/src/git/git config
log.showsignature true
austin@debian-laptop:~/src/winetricks$ ~/src/git/git log -n 1
commit 3399c38411259bf171fc32a3e145bc49fee2291e
gpg: Signature made Tue 10 May 2016 01:04:14 AM CDT using RSA key ID A041937B
gpg: Good signature from "Austin English (Austin English personal
gmail key) "
Author: Austin English 
Date:   Tue May 10 01:04:14 2016 -0500

release.sh: allow overridding version

austin@debian-laptop:~/src/winetricks$ ~/src/git/git show
commit 3399c38411259bf171fc32a3e145bc49fee2291e
gpg: Signature made Tue 10 May 2016 01:04:14 AM CDT using RSA key ID A041937B
gpg: Good signature from "Austin English (Austin English personal
gmail key) "
Author: Austin English 
Date:   Tue May 10 01:04:14 2016 -0500

release.sh: allow overridding version

diff --git a/src/release.sh b/src/release.sh
index 442df33..03a9462 100755
--- a/src/release.sh
+++ b/src/release.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 # Trivial release helper for winetricks
 #
+# Usage: $0 optional_version_name
+#
 # Copyright (C) 2016 Austin English
 #
 # This software comes with ABSOLUTELY NO WARRANTY.
@@ -19,7 +21,7 @@ if [ ! -f Makefile ] ; then
 exit 1
 fi

-version="$(date +%Y%m%d)"
+version="${1:-$(date +%Y%m%d)}"

 if git tag | grep ${version} ; then
 echo "A tag for ${version} already exists!"


Please CC me on future patches if you'd like me to test them. Thanks
again for your help!

-- 
-Austin
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [feature request] find git commit log before rebase

2016-05-26 Thread ryenus
Indeed, specifying the branch name does the trick
and this works with `git reflog` and/or `git log --walk-reflogs`

Thank you very much!

On 27 May 2016 at 01:58, Sebastian Staudt  wrote:
> Hi.
>
> I think what you want is `git reflog` (http://git-scm.com/man/reflog).
>
> git reflog b
>
> Will tell you the commits b pointed to in the past.
>
> Best regards,
> Sebastian
> ryenus  schrieb am Do., 26. Mai 2016 um 19:03:
>>
>> Assuming I have branches master (m), and a side branch (b), with a
>> history tree like below:
>>
>> m0 --- m1 -- m2 -- m3 -- m4 --- master (m)
>> \  /  \
>> b1 -- b2   b3 -- b4 -- branch (b) (HEAD)
>>   |
>>   (tag:POINT_BEFORE_REBASE)
>>
>> The history of branch b is can be described as:
>>
>> 1. branch b is forked at point of m1
>> 2. branch b is merged to master into m3,
>> 3. branch b then is rebased (fast-forward) from b2 to m4
>> 4. then branch b started its new life as b3 b4 after rebase
>>
>> With the following command: I can find b3 and b4 since last fork-point
>>
>> git log --oneline $(git merge-base --fork-point master)..b
>>
>> But how to find out commits b1 b2, given the fact that b2 is the point
>> before rebase?
>>
>> I understand it can be achieved via:
>>
>> git log --oneline m2..b2
>>
>> That's because I know b2 is the point before rebase,
>> and m2 is another child of the subsequent merge commit m3
>>
>> I wonder how to do this with an simple (enough) command without me
>> looking through the history and find b2 and m2 manually?
>>
>> Thanks!
>> --
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RFC: dynamic "auto" date formats

2016-05-26 Thread Linus Torvalds
This is a throw-away idea with a simple patch attached, which I don't
think anybody should really take all that seriously per se, but I
thought I'd throw it out and see if it generates any discussion.

I almost never use anything but the default date format (DATE_NORMAL),
but every once in a while I will use "--date=relative", typically
because I'm looking at my own commits and I'm just checking when I did
something.

It struck me that I've made the default for most of the logging things
be that when I'm just browsing with the pager, git will automatically
do the right thing. So I have

  [color]
  ui=auto

  [log]
  decorate = auto

in my ~/.gitconfig, and it shows me all those other things I tench to
want to know (like "thar's what I've pushed out" thanks to
decorations).

So I started thinking about when I care about dates, and decided that
maybe I can have a "--date=auto" too. It basically uses relative date
formats for the last 24 hours, and then switches over to the default
format.

I've used it a bit, and like Katy Perry said, I think I might like it.

Note that this doesn't add any gitconfig setting to do this, which
would be part of the whole point if this is actually sensible. But I'm
not entirely convinced it's worth it in the first place, thus this
email to see how people react ("That's just stupid" vs "yeah, I didn't
even know I wanted it, but now I need it").

And no, I'm not at all sure that the 24-hour cut-off is the right
thing, but it didn't seem completely crazy either. I tend to like the
relative date format when it is "19 minutes ago" vs "2 hours ago", at
some point it's long enough ago that it's more useful to know "Tuesday
at 3pm" than about how long ago it was.

(And yes, it would be even better to have the "short term relative
date" turn into a "medium-term 'day of the week at time x'" and then
turn into "full date" when it's more than a week ago, but this patch
only has the two modes of "short term" and "long term" and nothing in
between).

   Linus
 builtin/blame.c |  1 +
 cache.h |  3 ++-
 date.c  | 10 +++---
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 21f42b0b62b8..4d87181dc6cd 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2640,6 +2640,7 @@ parse_done:
   fewer display columns. */
blame_date_width = utf8_strwidth(_("4 years, 11 months ago")) + 
1; /* add the null */
break;
+   case DATE_AUTO:
case DATE_NORMAL:
blame_date_width = sizeof("Thu Oct 19 16:00:04 2006 -0700");
break;
diff --git a/cache.h b/cache.h
index 6049f8671138..624817c20414 100644
--- a/cache.h
+++ b/cache.h
@@ -1223,7 +1223,8 @@ struct date_mode {
DATE_ISO8601_STRICT,
DATE_RFC2822,
DATE_STRFTIME,
-   DATE_RAW
+   DATE_RAW,
+   DATE_AUTO,
} type;
const char *strftime_fmt;
int local;
diff --git a/date.c b/date.c
index 7c9f76998ac7..c38414a3d968 100644
--- a/date.c
+++ b/date.c
@@ -184,13 +184,15 @@ const char *show_date(unsigned long time, int tz, const 
struct date_mode *mode)
return timebuf.buf;
}
 
-   if (mode->type == DATE_RELATIVE) {
+   if (mode->type == DATE_RELATIVE || mode->type == DATE_AUTO) {
struct timeval now;
 
strbuf_reset(&timebuf);
gettimeofday(&now, NULL);
-   show_date_relative(time, tz, &now, &timebuf);
-   return timebuf.buf;
+   if (mode->type != DATE_AUTO || time + 24*60*60 > now.tv_sec) {
+   show_date_relative(time, tz, &now, &timebuf);
+   return timebuf.buf;
+   }
}
 
tm = time_to_tm(time, tz);
@@ -792,6 +794,8 @@ static enum date_mode_type parse_date_type(const char 
*format, const char **end)
return DATE_RAW;
if (skip_prefix(format, "format", end))
return DATE_STRFTIME;
+   if (skip_prefix(format, "auto", end))
+   return DATE_AUTO;
 
die("unknown date format %s", format);
 }


[PATCH] format_commit_message: honor `color=auto` for `%C(auto)`

2016-05-26 Thread Edward Thomson
git-log(1) documents that when specifying the `%C(auto)` format
placeholder will "turn on auto coloring on the next %placeholders
until the color is switched again."

However, when `%C(auto)` is used, the present implementation will turn
colors on unconditionally (even if the color configuration is turned off
for the current context - for example, `--no-color` was specified or the
color is `auto` and the output is not a tty).

Update `format_commit_one` to examine the current context when a format
string of `%C(auto)` is specified, which ensures that we will not
unconditionally write colors.  This brings that behavior in line with
the behavior of `%C(auto,)`, and allows the user the ability
to specify that color should be displayed only when the output is a
tty.

Additionally, add a test for `%C(auto)` and update the existing tests
for `%C(auto,...)` as they were misidentified as being applicable to
`%C(auto)`.

Signed-off-by: Edward Thomson 

Tests from Jeff King.

Signed-off-by: Jeff King 
---
 pretty.c   |  2 +-
 t/t6006-rev-list-format.sh | 26 +++---
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/pretty.c b/pretty.c
index 87c4497..c3ec430 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1063,7 +1063,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in 
UTF-8 */
switch (placeholder[0]) {
case 'C':
if (starts_with(placeholder + 1, "(auto)")) {
-   c->auto_color = 1;
+   c->auto_color = want_color(c->pretty_ctx->color);
return 7; /* consumed 7 bytes, "C(auto)" */
} else {
int ret = parse_color(sb, placeholder, c);
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index b77d4c9..a1dcdb8 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -184,38 +184,38 @@ commit $head1
 foo
 EOF
 
-test_expect_success '%C(auto) does not enable color by default' '
+test_expect_success '%C(auto,...) does not enable color by default' '
git log --format=$AUTO_COLOR -1 >actual &&
has_no_color actual
 '
 
-test_expect_success '%C(auto) enables colors for color.diff' '
+test_expect_success '%C(auto,...) enables colors for color.diff' '
git -c color.diff=always log --format=$AUTO_COLOR -1 >actual &&
has_color actual
 '
 
-test_expect_success '%C(auto) enables colors for color.ui' '
+test_expect_success '%C(auto,...) enables colors for color.ui' '
git -c color.ui=always log --format=$AUTO_COLOR -1 >actual &&
has_color actual
 '
 
-test_expect_success '%C(auto) respects --color' '
+test_expect_success '%C(auto,...) respects --color' '
git log --format=$AUTO_COLOR -1 --color >actual &&
has_color actual
 '
 
-test_expect_success '%C(auto) respects --no-color' '
+test_expect_success '%C(auto,...) respects --no-color' '
git -c color.ui=always log --format=$AUTO_COLOR -1 --no-color >actual &&
has_no_color actual
 '
 
-test_expect_success TTY '%C(auto) respects --color=auto (stdout is tty)' '
+test_expect_success TTY '%C(auto,...) respects --color=auto (stdout is tty)' '
test_terminal env TERM=vt100 \
git log --format=$AUTO_COLOR -1 --color=auto >actual &&
has_color actual
 '
 
-test_expect_success '%C(auto) respects --color=auto (stdout not tty)' '
+test_expect_success '%C(auto,...) respects --color=auto (stdout not tty)' '
(
TERM=vt100 && export TERM &&
git log --format=$AUTO_COLOR -1 --color=auto >actual &&
@@ -223,6 +223,18 @@ test_expect_success '%C(auto) respects --color=auto 
(stdout not tty)' '
)
 '
 
+test_expect_success '%C(auto) respects --color' '
+   git log --color --format="%C(auto)%H" -1 >actual &&
+   printf "\\033[33m%s\\033[m\\n" $(git rev-parse HEAD) >expect &&
+   test_cmp expect actual
+'
+
+test_expect_success '%C(auto) respects --no-color' '
+   git log --no-color --format="%C(auto)%H" -1 >actual &&
+   git rev-parse HEAD >expect &&
+   test_cmp expect actual
+'
+
 iconv -f utf-8 -t $test_encoding > commit-msg 

Re: [PATCH] format_commit_message: honor `color=auto` for `%C(auto)`

2016-05-26 Thread Edward Thomson
On Wed, May 25, 2016 at 05:39:04PM -0500, Jeff King wrote:
> Looks like we didn't have any tests at all for %C(auto). And the tests
> for %C(auto,...) were labeled as %C(auto), making it all the more
> confusing. Perhaps it is worth squashing this in:

Thanks, peff.  Indeed I did squash that into my updated patch.

-ed
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] format_commit_message: honor `color=auto` for `%C(auto)`

2016-05-26 Thread Jeff King
On Thu, May 26, 2016 at 10:46:10PM -0500, Edward Thomson wrote:

> git-log(1) documents that when specifying the `%C(auto)` format
> placeholder will "turn on auto coloring on the next %placeholders
> until the color is switched again."
> 
> However, when `%C(auto)` is used, the present implementation will turn
> colors on unconditionally (even if the color configuration is turned off
> for the current context - for example, `--no-color` was specified or the
> color is `auto` and the output is not a tty).
> 
> Update `format_commit_one` to examine the current context when a format
> string of `%C(auto)` is specified, which ensures that we will not
> unconditionally write colors.  This brings that behavior in line with
> the behavior of `%C(auto,)`, and allows the user the ability
> to specify that color should be displayed only when the output is a
> tty.
> 
> Additionally, add a test for `%C(auto)` and update the existing tests
> for `%C(auto,...)` as they were misidentified as being applicable to
> `%C(auto)`.

Explanation and the patch look good.

> Signed-off-by: Edward Thomson 
> 
> Tests from Jeff King.
> 
> Signed-off-by: Jeff King 

Trailers should all go at the bottom in a single stanza, and should
generally be in chronological order (so you got the bits from with an
s-o-b, and then you signed off the whole thing). IOW:

> Tests from Jeff King.
>
> Signed-off-by: Jeff King 
> Signed-off-by: Edward Thomson 

I suspect Junio can just tweak that while applying, unless there's
another reason to re-roll.

(Also for anybody watching, Ed did not just make up my signoff; I gave
it to him off-list).

-Peff

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: RFC: dynamic "auto" date formats

2016-05-26 Thread Jeff King
On Thu, May 26, 2016 at 08:36:57PM -0700, Linus Torvalds wrote:

> Note that this doesn't add any gitconfig setting to do this, which
> would be part of the whole point if this is actually sensible. But I'm
> not entirely convinced it's worth it in the first place, thus this
> email to see how people react ("That's just stupid" vs "yeah, I didn't
> even know I wanted it, but now I need it").
> 
> And no, I'm not at all sure that the 24-hour cut-off is the right
> thing, but it didn't seem completely crazy either. I tend to like the
> relative date format when it is "19 minutes ago" vs "2 hours ago", at
> some point it's long enough ago that it's more useful to know "Tuesday
> at 3pm" than about how long ago it was.

Seems like a reasonable idea, though I doubt I'd use it myself. In
addition to possibly making the cutoff-time configurable, I'd expect
people would want the fallback format to be configurable, too.

I wonder if something like:

  --date=relative:24h:iso

would make sense as "relative up to 24 hours, then iso". You could even
chain them, so your:

> (And yes, it would be even better to have the "short term relative
> date" turn into a "medium-term 'day of the week at time x'" and then
> turn into "full date" when it's more than a week ago, but this patch
> only has the two modes of "short term" and "long term" and nothing in
> between).

could be something like:

  --date=relative:24h:short:1w:normal

(where "short" actually kind of sucks; we'd introduce a new format for
your "Tuesday at 10pm" output).

I admit it's a bit baroque to look at, though.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 14/26] shallow.c: implement a generic shallow boundary finder based on rev-list

2016-05-26 Thread Eric Sunshine
On Wed, Apr 13, 2016 at 8:54 AM, Nguyễn Thái Ngọc Duy  wrote:
> Instead of a custom commit walker like get_shallow_commits(), this new
> function uses rev-list to mark NOT_SHALLOW to all reachable commits,
> except borders. The definition of reachable is to be defined by the
> protocol later. This makes it more flexible to define shallow boundary.
>
> The way we find find border is paint all reachable commits NOT_SHALLOW.

Grammo: "find find"

> Any of them that "touches" commits without NOT_SHALLOW flag are
> considered shallow (e.g. zero parents via grafting mechanism). Shallow
> commits and their true parents are all marked SHALLOW. Then NOT_SHALLOW
> is removed from shallow commits at the end.
>
> There is an interesting observation. With a generic walker, we can
> produce all kinds of shallow cutting. In the following graph, every
> commit but "x" is reachable. "b" is a parent of "a".
>
>x -- a -- o
>   //
> x -- c -- b -- o
>
> After this function is run, "a" and "c" are both considered shallow
> commits. After grafting occurs at the client side, what we see is
>
> a -- o
> /
>  c -- b -- o
>
> Notice that because of grafting, "a" has zero parents, so "b" is no
> longer a parent of "a".
>
> This is unfortunate and may be solved in two ways. The first is change
> the way shallow grafting works and keep "a -- b" connection if "b"
> exists and always ends at shallow commits (iow, no loose ends). This is
> hard to detect, or at least not cheap to do.
>
> The second way is mark one "x" as shallow commit instead of "a" and
> produce this graph at client side:
>
>x -- a -- o
>//
>  c -- b -- o
>
> More commits, but simpler grafting rules.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC/PATCH 1/2] log: add "log.showsignature" configuration variable

2016-05-26 Thread Pranit Bauva
Hey Mehul,

On Thu, May 26, 2016 at 8:34 PM, Mehul Jain  wrote:
> Hi Remi,
>
> Thanks for your input.
>
> On Thu, May 26, 2016 at 7:12 PM, Remi Galan Alfonso
>  wrote:
>> Hi Mehul,
>>
>> Mehul Jain  writes:
>>> +test_expect_success GPG '--show-signature overrides 
>>> log.showsignature=false' '
>>> +test_when_finished "git reset --hard && git checkout master" &&
>>> +git config log.showsignature false &&
>>
>> Any specific reason as to why you don't use test_config like in the
>> first test?
>
> None, actually. It was just that I forgot to use test_config while
> writing the tests. I will make changes  accordingly as test_config
> automatically unset the config variable, which is necessary.

Or you could probably use 'git -c' which makes it all the more compact.

Regards,
Pranit Bauva
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: t7800 test failure

2016-05-26 Thread David Aguilar
On Wed, May 25, 2016 at 11:33:33AM +0200, Armin Kunaschik wrote:
> On Tue, May 24, 2016 at 7:36 PM, Junio C Hamano  wrote:
> > Armin Kunaschik  writes:
> >>
> >> Ok, how can this be implemented within the test environment?
> >
> > I actually think an unconditional check like this is sufficient.
> 
> Ah, good. My thoughts were a bit more complicated.
> Anyway, this works for me.
> Thanks!

Would you mind submitting a patch so that we can support these
tests when running on AIX/HP-UX?


> >  t/t7800-difftool.sh | 17 +
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> > index 7ce4cd7..f304228 100755
> > --- a/t/t7800-difftool.sh
> > +++ b/t/t7800-difftool.sh
> > @@ -442,15 +442,16 @@ run_dir_diff_test 'difftool --dir-diff with unmerged 
> > files' '
> > test_cmp expect actual
> >  '
> >
> > -write_script .git/CHECK_SYMLINKS <<\EOF
> > -for f in file file2 sub/sub
> > -do
> > -   echo "$f"
> > -   readlink "$2/$f"
> > -done >actual
> > -EOF
> > -
> >  test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without 
> > unstaged changes' '
> > +
> > +   write_script .git/CHECK_SYMLINKS <<-\EOF &&
> > +   for f in file file2 sub/sub
> > +   do
> > +   echo "$f"
> > +   ls -ld "$2/$f" | sed -e "s/.* -> //"
> > +   done >actual
> > +   EOF
> > +
> > cat >expect <<-EOF &&
> > file
> > $PWD/file

I was curious so I whipped together a small tweak to
t/check-non-portable-shell.pl below.

The difftool tests are not the only ones that use readlink.
My guess is you haven't run the p4 tests because AIX/HP-UX doesn't have p4?

$ make test-lint-shell-syntax
t7800-difftool.sh:449: error: readlink is not portable (please use ls 
-ld | sed):   readlink "$2/$f"
t9802-git-p4-filetype.sh:266: error: readlink is not portable (please 
use ls -ld | sed):test $(readlink symlink) = symlink-target
t9802-git-p4-filetype.sh:332: error: readlink is not portable (please 
use ls -ld | sed):test $(readlink empty-symlink) = target2
test-lib.sh:757: error: readlink is not portable (please use ls -ld | 
sed): test "$1" = "$(readlink "$2")" || {

If we want to ban use of readlink from the test suite we could
add it to the check script.  test-lib.sh also includes it for
valgrind support so I'm not really sure whether we'd want to
apply this, but I figured I'd bring it up for discussion.

If we end up fixing all of these then I can send this to the
list as a proper patch.

Curious, is there an easy way to get readlink and mktemp installed on AIX?

Another alternative is that we can compile our own
"git-readlink" and "git-mktemp" programs and use those instead,
but that seems like a big maintenance burden compared to the
relative simplicity of the test-suite workarounds.

Thanks for fixing my non-portable tests ;-)

--- 8< --- 8< ---
>From 40c41402dfa24ca16b41062172c34f238d77b42c Mon Sep 17 00:00:00 2001
From: David Aguilar 
Date: Thu, 26 May 2016 02:42:52 -0700
Subject: [PATCH] tests: add shell portability check for "readlink"

Signed-off-by: David Aguilar 
---
 t/check-non-portable-shell.pl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index b170cbc..2707e42 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -18,6 +18,7 @@ while (<>) {
chomp;
/\bsed\s+-i/ and err 'sed -i is not portable';
/\becho\s+-n/ and err 'echo -n is not portable (please use printf)';
+   /\breadlink\s+/ and err 'readlink is not portable (please use ls -ld | 
sed)';
/^\s*declare\s+/ and err 'arrays/declare not portable';
/^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
/\btest\s+[^=]*==/ and err '"test a == b" is not portable (please use 
=)';
-- 
2.7.0.rc3

-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: t7610-mergetool.sh test failure

2016-05-26 Thread David Aguilar
On Wed, May 25, 2016 at 08:51:14PM -0500, Jeff King wrote:
> On Wed, May 25, 2016 at 06:16:15PM -0500, Jeff King wrote:
> 
> > On Tue, May 24, 2016 at 09:45:25AM -0700, Junio C Hamano wrote:
> > 
> > > On Tue, May 24, 2016 at 9:44 AM, Armin Kunaschik
> > >  wrote:
> > > > t7610-mergetool.sh fails on systems without mktemp.
> > > >
> > > > mktemp is used in git-mergetool.sh and throws an error when it's not 
> > > > available.
> > > > error: mktemp is needed when 'mergetool.writeToTemp' is true
> > > >
> > > > I see 2 options:
> > > > 1. code around it, write an own mktemp, maybe use the test-mktemp as a 
> > > > basis.
> > > > 2. disable the test when mktemp is not available
> > > 
> > > 3. find and install mktemp?
> > 
> > I'd agree more with (3) if it was some critical part of git-mergetool.
> > But it seems to be a completely optional feature that we use in only one
> > place, and git-mergetool even detects this case and complains.
> > 
> > So another alternative would be for the test to detect either that
> > mergetool worked, or that we got the "mktemp is needed" error.
> 
> BTW, one thing I happened to note while looking at this: running the
> test script will write into /tmp (or wherever your $TMPDIR points).
> Probably not a big deal, but I wonder if we should be setting $TMPDIR in
> our test harness.

We already set $HOME and various other variables to carefully
tune the testsuite's behavior, so this sounds like a good idea
IMO.

Would there be any downsides to setting $TMPDIR equal to the
trash directory?

FWIW I set $TMPDIR to the $TEST_OUTPUT_DIRECTORY in test-lib.sh
and was able to run the test suite without any errors.

Is it worth patching?
-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] add: add --chmod=+x / --chmod=-x options

2016-05-26 Thread Edward Thomson
On Wed, May 25, 2016 at 12:36:55AM -0700, Junio C Hamano wrote:
> 
> At the design level, I have a few comments.

Thanks, I will submit a new patch that incorporates your (and dscho's)
comments.

>  * This is about a repository with core.filemode=0; I wonder if
>something for a repository with core.symlinks=0 would also help?
>That is, would it be a big help to users if they can prepare a
>text file that holds symbolic link contents and add it as if it
>were a symlink with "git add", instead of having to run two
>commands, "hash-objects && update-index --cacheinfo"?

I think that this is much less common and - speaking only from personal
experience - nobody has ever asked me how to stage a symlink on a
Windows machine.  I think that this is due to the fact that symlinks on
Windows are basically impossible to use, so people doing cross-platform
development wouldn't even try.

On the other hand, it's quite common for cross-platform teams to use
some scripting language since those do work across platforms, and
Windows users would want to add new scripts as executable for the
benefit of their brethren on platforms with an executable bit.

>  * I am not familiar with life on filesystems with core.filemode=0;
>do files people would want to be able to "add --chmod=+x" share
>common trait that can be expressed with .gitattributes mechanism?

Perhaps...  It would not be things like `*.bat` or `*.exe` - Windows
gets those as executable "for free" and would not care about adding the
execute bit on those files (since they're not executable anywhere else).
It would be items like `*.sh` or `*.rb` that should be executable on
POSIX platforms.

However I do not think that this is a common enough action that it needs
to be made automatic such that when I `git add foo.rb` it is
automatically made executable.  I think that the reduced complexity of
having a single mechanism to control executability (that being the
execute mode in the index or a tree) is preferable to a gitattributes
based mechanism, at least until somebody else makes a cogent argument
that the gitattributes approach would be helpful for them.  :)

Thanks again for the comments, an updated patch is forthcoming.

-ed
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: t7610-mergetool.sh test failure

2016-05-26 Thread Jeff King
On Thu, May 26, 2016 at 09:40:27PM -0700, David Aguilar wrote:

> > BTW, one thing I happened to note while looking at this: running the
> > test script will write into /tmp (or wherever your $TMPDIR points).
> > Probably not a big deal, but I wonder if we should be setting $TMPDIR in
> > our test harness.
> 
> We already set $HOME and various other variables to carefully
> tune the testsuite's behavior, so this sounds like a good idea
> IMO.
> 
> Would there be any downsides to setting $TMPDIR equal to the
> trash directory?

The only one I can think of is that if something leaves cruft in
$TMPDIR, it could affect later tests that want to `git add`
indiscriminately. It seems unlikely.

OTOH, I do not think putting things in /tmp is hurting anything. I was
mostly just surprised by it.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] add: add --chmod=+x / --chmod=-x options

2016-05-26 Thread Mike Hommey
On Thu, May 26, 2016 at 11:41:12PM -0500, Edward Thomson wrote:
> On Wed, May 25, 2016 at 12:36:55AM -0700, Junio C Hamano wrote:
> > 
> > At the design level, I have a few comments.
> 
> Thanks, I will submit a new patch that incorporates your (and dscho's)
> comments.
> 
> >  * This is about a repository with core.filemode=0; I wonder if
> >something for a repository with core.symlinks=0 would also help?
> >That is, would it be a big help to users if they can prepare a
> >text file that holds symbolic link contents and add it as if it
> >were a symlink with "git add", instead of having to run two
> >commands, "hash-objects && update-index --cacheinfo"?
> 
> I think that this is much less common and - speaking only from personal
> experience - nobody has ever asked me how to stage a symlink on a
> Windows machine.  I think that this is due to the fact that symlinks on
> Windows are basically impossible to use, so people doing cross-platform
> development wouldn't even try.
> 
> On the other hand, it's quite common for cross-platform teams to use
> some scripting language since those do work across platforms, and
> Windows users would want to add new scripts as executable for the
> benefit of their brethren on platforms with an executable bit.
> 
> >  * I am not familiar with life on filesystems with core.filemode=0;
> >do files people would want to be able to "add --chmod=+x" share
> >common trait that can be expressed with .gitattributes mechanism?
> 
> Perhaps...  It would not be things like `*.bat` or `*.exe` - Windows
> gets those as executable "for free" and would not care about adding the
> execute bit on those files (since they're not executable anywhere else).
> It would be items like `*.sh` or `*.rb` that should be executable on
> POSIX platforms.
> 
> However I do not think that this is a common enough action that it needs
> to be made automatic such that when I `git add foo.rb` it is
> automatically made executable.

Moreover, *.sh, *.rb, etc. are not necessarily meant to be executables.
The files might be modules, included from executables or other modules.
There's an example of this right in the git tree: t/test-lib.sh. It's
even more typical for *.rb files (or *.py, etc.)

However, the common pattern that /might/ be interesting for automatic
executable bits is files starting with "#!".

Mike
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC/PATCH 1/2] log: add "log.showsignature" configuration variable

2016-05-26 Thread Mehul Jain
On Thu, May 26, 2016 at 10:29 PM, Jeff King  wrote:
> On Thu, May 26, 2016 at 06:36:46PM +0530, Mehul Jain wrote:
> The documentation here mentions "log" and "show". But I think this will
> affect other programs, too, including "whatchanged" and "reflog". Those
> ones are probably good, but the documentation is a little misleading (I
> think other options just say "git-log and related commands" or
> something).

Yes, the documentation is misleading. As you have mentioned, this
config variable will affect git-log, git-show, git-whatchanged and git-reflog.
I will mention them in the documentation.

>> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
>> index 128ba93..36be9a1 100755
>> --- a/t/t4202-log.sh
>> +++ b/t/t4202-log.sh
>> @@ -890,6 +890,25 @@ test_expect_success GPG 'log --graph --show-signature 
>> for merged tag' '
>>   grep "^| | gpg: Good signature" actual
>>  '
>>
>> +test_expect_success GPG 'log.showsignature=true behaves like 
>> --show-signature' '
>> + git checkout -b test_sign master &&
>> + echo foo >foo &&
>> + git add foo &&
>> + git commit -S -m signed_commit &&
>> + test_config log.showsignature true &&
>> + git log -1 signed >actual &&
>> + test_i18ngrep "gpg: Signature made" actual &&
>> + test_i18ngrep "gpg: Good signature" actual
>> +'
>
> You can see in the context that we do not use test_i18ngrep for finding
> gpg output in existing tests. I'm not sure if the new tests should be
> consistent, or if they should be changed to use test_i18ngrep. I don't
> think it's actually doing anything here, though. It's used with a
> git-specific GETTEXT_POISON flag that tweaks the output generated by
> git, but not by sub-programs like gpg.

There was no real motivation behind usage of test_i18ngrep. Certainly,
usage of grep will fit in the context.

>> +test_expect_success GPG '--show-signature overrides 
>> log.showsignature=false' '
>> + test_when_finished "git reset --hard && git checkout master" &&
>> + git config log.showsignature false &&
>
> Should this be test_config?

Noted.

Thanks,
Mehul
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC/PATCH 2/2] log: add "--no-show-signature" command line option

2016-05-26 Thread Mehul Jain
On Thu, May 26, 2016 at 10:52 PM, Junio C Hamano  wrote:
> Jeff King  writes:
>
>> On Thu, May 26, 2016 at 06:36:47PM +0530, Mehul Jain wrote:
>>
>>> If "log.showsignature=true", then there is no way to override it using
>>> command line switch.
>>>
>>> Teach "git log" and "git show" about "--no-show-signature" command line
>>> option.
>>
>> I think this is teaching all of the revision machinery about it (which
>> is a good thing).
>
> I agree that the proposed commit log message should be updated to
> say so.
>
> Because we do not want .showsignature configuration to affect
> rev-list nor format-patch, and we will not make "--show-sig" the
> default for them either.  From that point of view, there is no
> reason for them to know about the "--no-show-signature" option.
>
> The only reason why teaching the "--no-show-signature" option to
> these commands is a good idea is because it would help people who
> create an alias with "--show-sig" in early part of the command line,
> e.g.
>
> [alias] fp = format-patch --show-signature
>
> by allowing them to countermand with --no-show-signature, i.e.
>
> $ git fp --no-show-signature ...
>
> If we are updating the log message in the final submission of this
> patch, we'd want it to be clear that the presence of this option is
> not an excuse to introduce .showsignature that affects rev-list
> later to make sure we do not have to waste our time rejecting such a
> patch in the future.

Currently, with the [patch 1/2], only git-show, git-log, git-whatchanged
and git-reflog are able to learn about log.showsignature config variable.
But commands which will learn about "--no-show-signature" with
[patch 2/2] are notably a super-set of above mentioned commands.
Introduction of this option should not give an impression that we might
need log.showSignature for commands like git-format-patch etc, and
it will definitely be a wise decision to convey the same in the commit
message of this patch. I will do the necessary change.

Just out of curiosity, I was thinking that we might be able to teach
"--no-show-signature" option only to git-show, git-log, git-whatchanged
and git-reflog. To do this we can introduce a new member
"no_show_signature" in struct rev_info, and use this variable further
to modify the value of value of "rev.show_signature" after init_revision()
is called. This way we can selectively decide which commands should
learn about "--no-show-signature". This may be a bad idea because
we will have two variables in rev_info, for option --[no]-show-signature.
Any thoughts?

Thanks,
Mehul
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: RFC: dynamic "auto" date formats

2016-05-26 Thread Junio C Hamano
Linus Torvalds  writes:

> And no, I'm not at all sure that the 24-hour cut-off is the right
> thing, but it didn't seem completely crazy either. I tend to like the
> relative date format when it is "19 minutes ago" vs "2 hours ago", at
> some point it's long enough ago that it's more useful to know "Tuesday
> at 3pm" than about how long ago it was.
>
> (And yes, it would be even better to have the "short term relative
> date" turn into a "medium-term 'day of the week at time x'" and then
> turn into "full date" when it's more than a week ago, but this patch
> only has the two modes of "short term" and "long term" and nothing in
> between).

While I do not think this has much to do with "auto", other than
that it changes the representation depending on how far back the
time is to match the taste of Linus automatically, I think the
observation you made about the relative uselessness of "relative in
the long past" is real.  "6 years ago" that does not say if it was
in the morning and that does not even say if it was in the summer
is losing a bit too much information.

Your message made me realize another thing I feel while viewing
"relative in the long past" output.  In "git log --date=relative"
output (especially when the log is "limited" in some way, like with
a pathspec, --grep, -S, etc.) that shows multiple commits, all of
which are labeled "6 years ago", they make me wonder how they are
related to each other chronologically.  Perhaps I am seeing 6
commits, but the earlier four was made all within 20 minutes, and
the fifth one three days later, and the final one a month later,
which may indicate that the first four was the initial round of a
topic, with two "oops, this is a fix" follow-up patches that are
related in one area.  All of them being labeled "6 years ago" would
not give such a clue.

Which makes me wonder if another variant is useful (or at least
"interesting").  What if we chose format according to this rule?

0. Set the "reference time" to the current time.

1. Do get_revision() to grab one commit to show.

2. Show that commit, using timeformat determined as:
   a. if its time is close to the "reference time", then use
  "N hours M minues before that" format;
   b. otherwise, use the default time format;

3. Update the "reference time" to the timestamp of the commit
   we just showed.

4. Go back to 1.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


  1   2   >