Re: [PATCH v2 6/6] revision.c: refactor basic topo-order logic

[Ævar Arnfjörð Bjarmason]

On Tue, Sep 18 2018, Derrick Stolee via GitGitGadget wrote:

> diff --git a/revision.h b/revision.h
> index fd4154ff75..b20c16c0e0 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -24,6 +24,8 @@
>  #define USER_GIVEN   (1u<<25) /* given directly by the user */
>  #define TRACK_LINEAR (1u<<26)
>  #define ALL_REV_FLAGS(((1u<<11)-1) | USER_GIVEN | TRACK_LINEAR)
> +#define TOPO_WALK_EXPLORED (1u<<27)
> +#define TOPO_WALK_INDEGREE (1u<<28)

Maybe lead with a commit to indent these bitfield defines so this change
doesn't end up making these two new flags (due to the length of the
name) misaligned.

[PATCH 0/3] doc: fixups for ab/fetch-tags-noclobber

[Ævar Arnfjörð Bjarmason]

Of course I only noticed these after the series had landed in
master...

Ævar Arnfjörð Bjarmason (3):
  push doc: add spacing between two words
  fetch doc: correct grammar in --force docs
  fetch doc: correct grammar in --force docs

 Documentation/git-push.txt | 2 +-
 Documentation/pull-fetch-param.txt | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.19.0.rc2.392.g5ba43deb5a

[PATCH 2/3] fetch doc: correct grammar in --force docs

[Ævar Arnfjörð Bjarmason]

Correct a grammar error (saying "the receiving" made no sense) in the
recently landed documentation added in my 0bc8d71b99 ("fetch: stop
clobbering existing tags without --force", 2018-08-31) by rephrasing
the sentence.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/pull-fetch-param.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/pull-fetch-param.txt 
b/Documentation/pull-fetch-param.txt
index 293c6b967d..2b1fbe03aa 100644
--- a/Documentation/pull-fetch-param.txt
+++ b/Documentation/pull-fetch-param.txt
@@ -45,7 +45,7 @@ rules particular to 'git fetch' are noted below.
 +
 Until Git version 2.20, and unlike when pushing with
 linkgit:git-push[1], any updates to `refs/tags/*` would be accepted
-without `+` in the refspec (or `--force`). The receiving promiscuously
+without `+` in the refspec (or `--force`). When fetching, we promiscuously
 considered all tag updates from a remote to be forced fetches.  Since
 Git version 2.20, fetching to update `refs/tags/*` work the same way
 as when pushing. I.e. any updates will be rejected without `+` in the
-- 
2.19.0.rc2.392.g5ba43deb5a

[PATCH 1/3] push doc: add spacing between two words

[Ævar Arnfjörð Bjarmason]

Fix a formatting error introduced in my recently landed
fe802bd21e ("push doc: correct lies about how push refspecs work",
2018-08-31).

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/git-push.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index f345bd30fc..a5fc54aeab 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -110,7 +110,7 @@ can be overridden by adding an the optional leading `+` to 
a refspec
 is that no amount of forcing will make the `refs/heads/*` namespace
 accept a non-commit object. Hooks and configuration can also override
 or amend these rules, see e.g. `receive.denyNonFastForwards` in
-linkgit:git-config[1] and`pre-receive` and `update` in
+linkgit:git-config[1] and `pre-receive` and `update` in
 linkgit:githooks[5].
 +
 Pushing an empty  allows you to delete the  ref from the
-- 
2.19.0.rc2.392.g5ba43deb5a

[PATCH 3/3] fetch doc: correct grammar in --force docs

[Ævar Arnfjörð Bjarmason]

"Work the same" is incorrect and needs to be "Works the same
way". Fixes grammar in document anion I added in the recently landed
0bc8d71b99 ("fetch: stop clobbering existing tags without --force",
2018-08-31).

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/pull-fetch-param.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/pull-fetch-param.txt 
b/Documentation/pull-fetch-param.txt
index 2b1fbe03aa..7d3a60f5b9 100644
--- a/Documentation/pull-fetch-param.txt
+++ b/Documentation/pull-fetch-param.txt
@@ -47,7 +47,7 @@ Until Git version 2.20, and unlike when pushing with
 linkgit:git-push[1], any updates to `refs/tags/*` would be accepted
 without `+` in the refspec (or `--force`). When fetching, we promiscuously
 considered all tag updates from a remote to be forced fetches.  Since
-Git version 2.20, fetching to update `refs/tags/*` work the same way
+Git version 2.20, fetching to update `refs/tags/*` works the same way
 as when pushing. I.e. any updates will be rejected without `+` in the
 refspec (or `--force`).
 +
-- 
2.19.0.rc2.392.g5ba43deb5a

[PATCH] config doc: add missing list separator for checkout.optimizeNewBranch

[Ævar Arnfjörð Bjarmason]

The documentation added in fa655d8411 ("checkout: optimize "git
checkout -b "", 2018-08-16) didn't add the double-colon
needed for the labeled list separator, as a result the added
documentation all got squashed into one paragraph. Fix that by adding
the list separator.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---

Junio: Feel free to squash this in, but per your recent E-Mail it
doesn't seem you're planning to rewind "next", so this can go on top
of gitster/bp/checkout-new-branch-optim.

 Documentation/config.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ac71ade256..1546833213 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1153,7 +1153,7 @@ and by linkgit:git-worktree[1] when 'git worktree add' 
refers to a
 remote branch. This setting might be used for other checkout-like
 commands or functionality in the future.
 
-checkout.optimizeNewBranch
+checkout.optimizeNewBranch::
Optimizes the performance of "git checkout -b " when
using sparse-checkout.  When set to true, git will not update the
repo based on the current sparse-checkout settings.  This means it
-- 
2.19.0.rc2.392.g5ba43deb5a

Cannot negate `*` ignore pattern for directory with space in the name

[Victor Engmark]

To reproduce (from ):

$ cd "$(mktemp --directory)"
$ mkdir foo\ bar
$ touch foo\ bar/test
$ git init
Initialized empty Git repository in /tmp/tmp.iGmBR6y2xR/.git/
$ git status --short
?? foo bar/
$ cat > .gitignore << EOF
> *
> !foo bar
> !foo\ bar
> !"foo bar"
> "!foo bar"
> !foo*
> !foo bar/
> !foo\ bar/
> !"foo bar/"
> "!foo bar/"
> !foo*/
> EOF
$ git status --short
[no output]

The pattern *can* be negated if it only matches directories:

$ cat > .gitignore << EOF
> */
> !foo\ bar/
> EOF
$ git status --short
?? .gitignore
?? foo bar/

I encountered this problem because Visual Studio Code creates a
configuration directory called "Code - OSS", and I exclude everything in
~/.config by default to avoid noise in `git status`.

$ git --version
git version 2.19.0

-- 
Kind regards
Victor Engmark

[PATCH v2 5/6] commit/revisions: bookkeeping before refactoring

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee 

There are a few things that need to move around a little before
making a big refactoring in the topo-order logic:

1. We need access to record_author_date() and
   compare_commits_by_author_date() in revision.c. These are used
   currently by sort_in_topological_order() in commit.c.

2. Moving these methods to commit.h requires adding the author_slab
   definition to commit.h.

3. The add_parents_to_list() method in revision.c performs logic
   around the UNINTERESTING flag and other special cases depending
   on the struct rev_info. Allow this method to ignore a NULL 'list'
   parameter, as we will not be populating the list for our walk.

Signed-off-by: Derrick Stolee 
---
 commit.c   | 11 ---
 commit.h   |  8 
 revision.c |  6 --
 3 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/commit.c b/commit.c
index d0f199e122..f68e04b2f1 100644
--- a/commit.c
+++ b/commit.c
@@ -655,11 +655,8 @@ struct commit *pop_commit(struct commit_list **stack)
 /* count number of children that have not been emitted */
 define_commit_slab(indegree_slab, int);
 
-/* record author-date for each commit object */
-define_commit_slab(author_date_slab, timestamp_t);
-
-static void record_author_date(struct author_date_slab *author_date,
-  struct commit *commit)
+void record_author_date(struct author_date_slab *author_date,
+   struct commit *commit)
 {
const char *buffer = get_commit_buffer(commit, NULL);
struct ident_split ident;
@@ -684,8 +681,8 @@ fail_exit:
unuse_commit_buffer(commit, buffer);
 }
 
-static int compare_commits_by_author_date(const void *a_, const void *b_,
- void *cb_data)
+int compare_commits_by_author_date(const void *a_, const void *b_,
+  void *cb_data)
 {
const struct commit *a = a_, *b = b_;
struct author_date_slab *author_date = cb_data;
diff --git a/commit.h b/commit.h
index 2b1a734388..ff0eb5f8ef 100644
--- a/commit.h
+++ b/commit.h
@@ -8,6 +8,7 @@
 #include "gpg-interface.h"
 #include "string-list.h"
 #include "pretty.h"
+#include "commit-slab.h"
 
 #define COMMIT_NOT_FROM_GRAPH 0x
 #define GENERATION_NUMBER_INFINITY 0x
@@ -328,6 +329,13 @@ extern int remove_signature(struct strbuf *buf);
  */
 extern int check_commit_signature(const struct commit *commit, struct 
signature_check *sigc);
 
+/* record author-date for each commit object */
+define_commit_slab(author_date_slab, timestamp_t);
+
+void record_author_date(struct author_date_slab *author_date,
+   struct commit *commit);
+
+int compare_commits_by_author_date(const void *a_, const void *b_, void 
*unused);
 int compare_commits_by_commit_date(const void *a_, const void *b_, void 
*unused);
 int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, 
void *unused);
 
diff --git a/revision.c b/revision.c
index 2dcde8a8ac..92012d5f45 100644
--- a/revision.c
+++ b/revision.c
@@ -808,7 +808,8 @@ static int add_parents_to_list(struct rev_info *revs, 
struct commit *commit,
if (p->object.flags & SEEN)
continue;
p->object.flags |= SEEN;
-   commit_list_insert_by_date_cached(p, list, cached_base, 
cache_ptr);
+   if (list)
+   commit_list_insert_by_date_cached(p, list, 
cached_base, cache_ptr);
}
return 0;
}
@@ -847,7 +848,8 @@ static int add_parents_to_list(struct rev_info *revs, 
struct commit *commit,
p->object.flags |= left_flag;
if (!(p->object.flags & SEEN)) {
p->object.flags |= SEEN;
-   commit_list_insert_by_date_cached(p, list, cached_base, 
cache_ptr);
+   if (list)
+   commit_list_insert_by_date_cached(p, list, 
cached_base, cache_ptr);
}
if (revs->first_parent_only)
break;
-- 
gitgitgadget

[PATCH v2 0/6] Use generation numbers for --topo-order

[Derrick Stolee via GitGitGadget]

This patch series performs a decently-sized refactoring of the revision-walk
machinery. Well, "refactoring" is probably the wrong word, as I don't
actually remove the old code. Instead, when we see certain options in the
'rev_info' struct, we redirect the commit-walk logic to a new set of methods
that distribute the workload differently. By using generation numbers in the
commit-graph, we can significantly improve 'git log --graph' commands (and
the underlying 'git rev-list --topo-order').

On the Linux repository, I got the following performance results when
comparing to the previous version with or without a commit-graph:

Test: git rev-list --topo-order -100 HEAD
HEAD~1, no commit-graph: 6.80 s
HEAD~1, w/ commit-graph: 0.77 s
  HEAD, w/ commit-graph: 0.02 s

Test: git rev-list --topo-order -100 HEAD -- tools
HEAD~1, no commit-graph: 9.63 s
HEAD~1, w/ commit-graph: 6.06 s
  HEAD, w/ commit-graph: 0.06 s

If you want to read this series but are unfamiliar with the commit-graph and
generation numbers, then I recommend reading 
Documentation/technical/commit-graph.txt or a blob post [1] I wrote on the
subject. In particular, the three-part walk described in "revision.c:
refactor basic topo-order logic" is present (but underexplained) as an
animated PNG [2].

Since revision.c is an incredibly important (and old) portion of the
codebase -- and because there are so many orthogonal options in 'struct
rev_info' -- I consider this submission to be "RFC quality". That is, I am
not confident that I am not missing anything, or that my solution is the
best it can be. I did merge this branch with ds/commit-graph-with-grafts and
the "DO-NOT-MERGE: write and read commit-graph always" commit that computes
a commit-graph with every 'git commit' command. The test suite passed with
that change, available on GitHub [3]. To ensure that I cover at least the
case I think are interesting, I added tests to t6600-test-reach.sh to verify
the walks report the correct results for the three cases there (no
commit-graph, full commit-graph, and a partial commit-graph so the walk
starts at GENERATION_NUMBER_INFINITY).

One notable case that is not included in this series is the case of a
history comparison such as 'git rev-list --topo-order A..B'. The existing
code in limit_list() has ways to cut the walk short when all pending commits
are UNINTERESTING. Since this code depends on commit_list instead of the
prio_queue we are using here, I chose to leave it untouched for now. We can
revisit it in a separate series later. Since handle_commit() turns on
revs->limited when a commit is UNINTERESTING, we do not hit the new code in
this case. Removing this 'revs->limited = 1;' line yields correct results,
but the performance is worse.

This series was based on ds/reachable, but is now based on 'master' to not
conflict with 182070 "commit: use timestamp_t for author_date_slab". There
is a small conflict with md/filter-trees, because it renamed a flag in
revisions.h in the line before I add new flags. Hopefully this conflict is
not too difficult to resolve.

Thanks, -Stolee

[1] 
https://blogs.msdn.microsoft.com/devops/2018/07/09/supercharging-the-git-commit-graph-iii-generations/
Supercharging the Git Commit Graph III: Generations and Graph Algorithms

[2] 
https://msdnshared.blob.core.windows.net/media/2018/06/commit-graph-topo-order-b-a.png
Animation showing three-part walk

[3] https://github.com/derrickstolee/git/tree/topo-order/testA branch
containing this series along with commits to compute commit-graph in entire
test suite.

Derrick Stolee (6):
  prio-queue: add 'peek' operation
  test-reach: add run_three_modes method
  test-reach: add rev-list tests
  revision.c: begin refactoring --topo-order logic
  commit/revisions: bookkeeping before refactoring
  revision.c: refactor basic topo-order logic

 commit.c   |  11 +-
 commit.h   |   8 ++
 object.h   |   4 +-
 prio-queue.c   |   9 ++
 prio-queue.h   |   6 +
 revision.c | 232 -
 revision.h |   6 +
 t/helper/test-prio-queue.c |  10 +-
 t/t6600-test-reach.sh  |  98 +++-
 9 files changed, 361 insertions(+), 23 deletions(-)


base-commit: 2d3b1c576c85b7f5db1f418907af00ab88e0c303
Published-As: 
https://github.com/gitgitgadget/git/releases/tags/pr-25%2Fderrickstolee%2Ftopo-order%2Fprogress-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
pr-25/derrickstolee/topo-order/progress-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/25

Range-diff vs v1:

 1:  5e55669f4d = 1:  cc1ec4c270 prio-queue: add 'peek' operation
 2:  9628396af1 = 2:  404c918608 test-reach: add run_three_modes method
 3:  708b4550a1 = 3:  30dee58c61 test-reach: add rev-list tests
 4:  908442417d ! 4:  a74ae13d4e revision.c: begin refactoring --topo-order 
logic
 @@ -168,4 +168,4 @@
  + struct topo_walk_info *topo_walk_info;
   };
   
  

[PATCH v2 3/6] test-reach: add rev-list tests

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee 

The rev-list command is critical to Git's functionality. Ensure it
works in the three commit-graph environments constructed in
t6600-test-reach.sh. Here are a few important types of rev-list
operations:

* Basic: git rev-list --topo-order HEAD
* Range: git rev-list --topo-order compare..HEAD
* Ancestry: git rev-list --topo-order --ancestry-path compare..HEAD
* Symmetric Difference: git rev-list --topo-order compare...HEAD

Signed-off-by: Derrick Stolee 
---
 t/t6600-test-reach.sh | 84 +++
 1 file changed, 84 insertions(+)

diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
index 1b18e12a4e..2fcaa39077 100755
--- a/t/t6600-test-reach.sh
+++ b/t/t6600-test-reach.sh
@@ -243,4 +243,88 @@ test_expect_success 'commit_contains:miss' '
test_three_modes commit_contains --tag
 '
 
+test_expect_success 'rev-list: basic topo-order' '
+   git rev-parse \
+   commit-6-6 commit-5-6 commit-4-6 commit-3-6 commit-2-6 
commit-1-6 \
+   commit-6-5 commit-5-5 commit-4-5 commit-3-5 commit-2-5 
commit-1-5 \
+   commit-6-4 commit-5-4 commit-4-4 commit-3-4 commit-2-4 
commit-1-4 \
+   commit-6-3 commit-5-3 commit-4-3 commit-3-3 commit-2-3 
commit-1-3 \
+   commit-6-2 commit-5-2 commit-4-2 commit-3-2 commit-2-2 
commit-1-2 \
+   commit-6-1 commit-5-1 commit-4-1 commit-3-1 commit-2-1 
commit-1-1 \
+   >expect &&
+   run_three_modes "git rev-list --topo-order commit-6-6"
+'
+
+test_expect_success 'rev-list: first-parent topo-order' '
+   git rev-parse \
+   commit-6-6 \
+   commit-6-5 \
+   commit-6-4 \
+   commit-6-3 \
+   commit-6-2 \
+   commit-6-1 commit-5-1 commit-4-1 commit-3-1 commit-2-1 
commit-1-1 \
+   >expect &&
+   run_three_modes "git rev-list --first-parent --topo-order commit-6-6"
+'
+
+test_expect_success 'rev-list: range topo-order' '
+   git rev-parse \
+   commit-6-6 commit-5-6 commit-4-6 commit-3-6 commit-2-6 
commit-1-6 \
+   commit-6-5 commit-5-5 commit-4-5 commit-3-5 commit-2-5 
commit-1-5 \
+   commit-6-4 commit-5-4 commit-4-4 commit-3-4 commit-2-4 
commit-1-4 \
+   commit-6-3 commit-5-3 commit-4-3 \
+   commit-6-2 commit-5-2 commit-4-2 \
+   commit-6-1 commit-5-1 commit-4-1 \
+   >expect &&
+   run_three_modes "git rev-list --topo-order commit-3-3..commit-6-6"
+'
+
+test_expect_success 'rev-list: range topo-order' '
+   git rev-parse \
+   commit-6-6 commit-5-6 commit-4-6 \
+   commit-6-5 commit-5-5 commit-4-5 \
+   commit-6-4 commit-5-4 commit-4-4 \
+   commit-6-3 commit-5-3 commit-4-3 \
+   commit-6-2 commit-5-2 commit-4-2 \
+   commit-6-1 commit-5-1 commit-4-1 \
+   >expect &&
+   run_three_modes "git rev-list --topo-order commit-3-8..commit-6-6"
+'
+
+test_expect_success 'rev-list: first-parent range topo-order' '
+   git rev-parse \
+   commit-6-6 \
+   commit-6-5 \
+   commit-6-4 \
+   commit-6-3 \
+   commit-6-2 \
+   commit-6-1 commit-5-1 commit-4-1 \
+   >expect &&
+   run_three_modes "git rev-list --first-parent --topo-order 
commit-3-8..commit-6-6"
+'
+
+test_expect_success 'rev-list: ancestry-path topo-order' '
+   git rev-parse \
+   commit-6-6 commit-5-6 commit-4-6 commit-3-6 \
+   commit-6-5 commit-5-5 commit-4-5 commit-3-5 \
+   commit-6-4 commit-5-4 commit-4-4 commit-3-4 \
+   commit-6-3 commit-5-3 commit-4-3 \
+   >expect &&
+   run_three_modes "git rev-list --topo-order --ancestry-path 
commit-3-3..commit-6-6"
+'
+
+test_expect_success 'rev-list: symmetric difference topo-order' '
+   git rev-parse \
+   commit-6-6 commit-5-6 commit-4-6 \
+   commit-6-5 commit-5-5 commit-4-5 \
+   commit-6-4 commit-5-4 commit-4-4 \
+   commit-6-3 commit-5-3 commit-4-3 \
+   commit-6-2 commit-5-2 commit-4-2 \
+   commit-6-1 commit-5-1 commit-4-1 \
+   commit-3-8 commit-2-8 commit-1-8 \
+   commit-3-7 commit-2-7 commit-1-7 \
+   >expect &&
+   run_three_modes "git rev-list --topo-order commit-3-8...commit-6-6"
+'
+
 test_done
-- 
gitgitgadget

[PATCH v2 1/6] prio-queue: add 'peek' operation

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee 

When consuming a priority queue, it can be convenient to inspect
the next object that will be dequeued without actually dequeueing
it. Our existing library did not have such a 'peek' operation, so
add it as prio_queue_peek().

Add a reference-level comparison in t/helper/test-prio-queue.c
so this method is exercised by t0009-prio-queue.sh.

Signed-off-by: Derrick Stolee 
---
 prio-queue.c   |  9 +
 prio-queue.h   |  6 ++
 t/helper/test-prio-queue.c | 10 +++---
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/prio-queue.c b/prio-queue.c
index a078451872..d3f488cb05 100644
--- a/prio-queue.c
+++ b/prio-queue.c
@@ -85,3 +85,12 @@ void *prio_queue_get(struct prio_queue *queue)
}
return result;
 }
+
+void *prio_queue_peek(struct prio_queue *queue)
+{
+   if (!queue->nr)
+   return NULL;
+   if (!queue->compare)
+   return queue->array[queue->nr - 1].data;
+   return queue->array[0].data;
+}
diff --git a/prio-queue.h b/prio-queue.h
index d030ec9dd6..682e51867a 100644
--- a/prio-queue.h
+++ b/prio-queue.h
@@ -46,6 +46,12 @@ extern void prio_queue_put(struct prio_queue *, void *thing);
  */
 extern void *prio_queue_get(struct prio_queue *);
 
+/*
+ * Gain access to the "thing" that would be returned by
+ * prio_queue_get, but do not remove it from the queue.
+ */
+extern void *prio_queue_peek(struct prio_queue *);
+
 extern void clear_prio_queue(struct prio_queue *);
 
 /* Reverse the LIFO elements */
diff --git a/t/helper/test-prio-queue.c b/t/helper/test-prio-queue.c
index 9807b649b1..e817bbf464 100644
--- a/t/helper/test-prio-queue.c
+++ b/t/helper/test-prio-queue.c
@@ -22,9 +22,13 @@ int cmd__prio_queue(int argc, const char **argv)
struct prio_queue pq = { intcmp };
 
while (*++argv) {
-   if (!strcmp(*argv, "get"))
-   show(prio_queue_get());
-   else if (!strcmp(*argv, "dump")) {
+   if (!strcmp(*argv, "get")) {
+   void *peek = prio_queue_peek();
+   void *get = prio_queue_get();
+   if (peek != get)
+   BUG("peek and get results do not match");
+   show(get);
+   } else if (!strcmp(*argv, "dump")) {
int *v;
while ((v = prio_queue_get()))
   show(v);
-- 
gitgitgadget

[PATCH v2 6/6] revision.c: refactor basic topo-order logic

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee 

When running a command like 'git rev-list --topo-order HEAD',
Git performed the following steps:

1. Run limit_list(), which parses all reachable commits,
   adds them to a linked list, and distributes UNINTERESTING
   flags. If all unprocessed commits are UNINTERESTING, then
   it may terminate without walking all reachable commits.
   This does not occur if we do not specify UNINTERESTING
   commits.

2. Run sort_in_topological_order(), which is an implementation
   of Kahn's algorithm. It first iterates through the entire
   set of important commits and computes the in-degree of each
   (plus one, as we use 'zero' as a special value here). Then,
   we walk the commits in priority order, adding them to the
   priority queue if and only if their in-degree is one. As
   we remove commits from this priority queue, we decrement the
   in-degree of their parents.

3. While we are peeling commits for output, get_revision_1()
   uses pop_commit on the full list of commits computed by
   sort_in_topological_order().

In the new algorithm, these three steps correspond to three
different commit walks. We run these walks simultaneously,
and advance each only as far as necessary to satisfy the
requirements of the 'higher order' walk. We know when we can
pause each walk by using generation numbers from the commit-
graph feature.

Recall that the generation number of a commit satisfies:

* If the commit has at least one parent, then the generation
  number is one more than the maximum generation number among
  its parents.

* If the commit has no parent, then the generation number is one.

There are two special generation numbers:

* GENERATION_NUMBER_INFINITY: this value is 0x and
  indicates that the commit is not stored in the commit-graph and
  the generation number was not previously calculated.

* GENERATION_NUMBER_ZERO: this value (0) is a special indicator
  to say that the commit-graph was generated by a version of Git
  that does not compute generation numbers (such as v2.18.0).

Since we use generation_numbers_enabled() before using the new
algorithm, we do not need to worry about GENERATION_NUMBER_ZERO.
However, the existence of GENERATION_NUMBER_INFINITY implies the
following weaker statement than the usual we expect from
generation numbers:

If A and B are commits with generation numbers gen(A) and
gen(B) and gen(A) < gen(B), then A cannot reach B.

Thus, we will walk in each of our stages until the "maximum
unexpanded generation number" is strictly lower than the
generation number of a commit we are about to use.

The walks are as follows:

1. EXPLORE: using the explore_queue priority queue (ordered by
   maximizing the generation number), parse each reachable
   commit until all commits in the queue have generation
   number strictly lower than needed. During this walk, update
   the UNINTERESTING flags as necessary.

2. INDEGREE: using the indegree_queue priority queue (ordered
   by maximizing the generation number), add one to the in-
   degree of each parent for each commit that is walked. Since
   we walk in order of decreasing generation number, we know
   that discovering an in-degree value of 0 means the value for
   that commit was not initialized, so should be initialized to
   two. (Recall that in-degree value "1" is what we use to say a
   commit is ready for output.) As we iterate the parents of a
   commit during this walk, ensure the EXPLORE walk has walked
   beyond their generation numbers.

3. TOPO: using the topo_queue priority queue (ordered based on
   the sort_order given, which could be commit-date, author-
   date, or typical topo-order which treats the queue as a LIFO
   stack), remove a commit from the queue and decrement the
   in-degree of each parent. If a parent has an in-degree of
   one, then we add it to the topo_queue. Before we decrement
   the in-degree, however, ensure the INDEGREE walk has walked
   beyond that generation number.

The implementations of these walks are in the following methods:

* explore_walk_step and explore_to_depth
* indegree_walk_step and compute_indegrees_to_depth
* next_topo_commit and expand_topo_walk

These methods have some patterns that may seem strange at first,
but they are probably carry-overs from their equivalents in
limit_list and sort_in_topological_order.

One thing that is missing from this implementation is a proper
way to stop walking when the entire queue is UNINTERESTING, so
this implementation is not enabled by comparisions, such as in
'git rev-list --topo-order A..B'. This can be updated in the
future.

In my local testing, I used the following Git commands on the
Linux repository in three modes: HEAD~1 with no commit-graph,
HEAD~1 with a commit-graph, and HEAD with a commit-graph. This
allows comparing the benefits we get from parsing commits from
the commit-graph and then again the benefits we get by
restricting the set of commits we walk.

Test: git rev-list --topo-order -100 HEAD

[PATCH v2 4/6] revision.c: begin refactoring --topo-order logic

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee 

When running 'git rev-list --topo-order' and its kin, the topo_order
setting in struct rev_info implies the limited setting. This means
that the following things happen during prepare_revision_walk():

* revs->limited implies we run limit_list() to walk the entire
  reachable set. There are some short-cuts here, such as if we
  perform a range query like 'git rev-list COMPARE..HEAD' and we
  can stop limit_list() when all queued commits are uninteresting.

* revs->topo_order implies we run sort_in_topological_order(). See
  the implementation of that method in commit.c. It implies that
  the full set of commits to order is in the given commit_list.

These two methods imply that a 'git rev-list --topo-order HEAD'
command must walk the entire reachable set of commits _twice_ before
returning a single result.

If we have a commit-graph file with generation numbers computed, then
there is a better way. This patch introduces some necessary logic
redirection when we are in this situation.

In v2.18.0, the commit-graph file contains zero-valued bytes in the
positions where the generation number is stored in v2.19.0 and later.
Thus, we use generation_numbers_enabled() to check if the commit-graph
is available and has non-zero generation numbers.

When setting revs->limited only because revs->topo_order is true,
only do so if generation numbers are not available. There is no
reason to use the new logic as it will behave similarly when all
generation numbers are INFINITY or ZERO.

In prepare_revision_walk(), if we have revs->topo_order but not
revs->limited, then we trigger the new logic. It breaks the logic
into three pieces, to fit with the existing framework:

1. init_topo_walk() fills a new struct topo_walk_info in the rev_info
   struct. We use the presence of this struct as a signal to use the
   new methods during our walk. In this patch, this method simply
   calls limit_list() and sort_in_topological_order(). In the future,
   this method will set up a new data structure to perform that logic
   in-line.

2. next_topo_commit() provides get_revision_1() with the next topo-
   ordered commit in the list. Currently, this simply pops the commit
   from revs->commits.

3. expand_topo_walk() provides get_revision_1() with a way to signal
   walking beyond the latest commit. Currently, this calls
   add_parents_to_list() exactly like the old logic.

While this commit presents method redirection for performing the
exact same logic as before, it allows the next commit to focus only
on the new logic.

Signed-off-by: Derrick Stolee 
---
 revision.c | 42 ++
 revision.h |  4 
 2 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/revision.c b/revision.c
index e18bd530e4..2dcde8a8ac 100644
--- a/revision.c
+++ b/revision.c
@@ -25,6 +25,7 @@
 #include "worktree.h"
 #include "argv-array.h"
 #include "commit-reach.h"
+#include "commit-graph.h"
 
 volatile show_early_output_fn_t show_early_output;
 
@@ -2454,7 +2455,7 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
if (revs->diffopt.objfind)
revs->simplify_history = 0;
 
-   if (revs->topo_order)
+   if (revs->topo_order && !generation_numbers_enabled(the_repository))
revs->limited = 1;
 
if (revs->prune_data.nr) {
@@ -2892,6 +2893,33 @@ static int mark_uninteresting(const struct object_id 
*oid,
return 0;
 }
 
+struct topo_walk_info {};
+
+static void init_topo_walk(struct rev_info *revs)
+{
+   struct topo_walk_info *info;
+   revs->topo_walk_info = xmalloc(sizeof(struct topo_walk_info));
+   info = revs->topo_walk_info;
+   memset(info, 0, sizeof(struct topo_walk_info));
+
+   limit_list(revs);
+   sort_in_topological_order(>commits, revs->sort_order);
+}
+
+static struct commit *next_topo_commit(struct rev_info *revs)
+{
+   return pop_commit(>commits);
+}
+
+static void expand_topo_walk(struct rev_info *revs, struct commit *commit)
+{
+   if (add_parents_to_list(revs, commit, >commits, NULL) < 0) {
+   if (!revs->ignore_missing_links)
+   die("Failed to traverse parents of commit %s",
+   oid_to_hex(>object.oid));
+   }
+}
+
 int prepare_revision_walk(struct rev_info *revs)
 {
int i;
@@ -2928,11 +2956,13 @@ int prepare_revision_walk(struct rev_info *revs)
commit_list_sort_by_date(>commits);
if (revs->no_walk)
return 0;
-   if (revs->limited)
+   if (revs->limited) {
if (limit_list(revs) < 0)
return -1;
-   if (revs->topo_order)
-   sort_in_topological_order(>commits, revs->sort_order);
+   if (revs->topo_order)
+   sort_in_topological_order(>commits, 
revs->sort_order);
+   } else if (revs->topo_order)
+   init_topo_walk(revs);
if 

[PATCH v2 2/6] test-reach: add run_three_modes method

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee 

The 'test_three_modes' method assumes we are using the 'test-tool
reach' command for our test. However, we may want to use the data
shape of our commit graph and the three modes (no commit-graph,
full commit-graph, partial commit-graph) for other git commands.

Split test_three_modes to be a simple translation on a more general
run_three_modes method that executes the given command and tests
the actual output to the expected output.

While inspecting this code, I realized that the final test for
'commit_contains --tag' is silently dropping the '--tag' argument.
It should be quoted to include both.

Signed-off-by: Derrick Stolee 
---
 t/t6600-test-reach.sh | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
index d139a00d1d..1b18e12a4e 100755
--- a/t/t6600-test-reach.sh
+++ b/t/t6600-test-reach.sh
@@ -53,18 +53,22 @@ test_expect_success 'setup' '
git config core.commitGraph true
 '
 
-test_three_modes () {
+run_three_modes () {
test_when_finished rm -rf .git/objects/info/commit-graph &&
-   test-tool reach $1 actual &&
+   $1 actual &&
test_cmp expect actual &&
cp commit-graph-full .git/objects/info/commit-graph &&
-   test-tool reach $1 actual &&
+   $1 actual &&
test_cmp expect actual &&
cp commit-graph-half .git/objects/info/commit-graph &&
-   test-tool reach $1 actual &&
+   $1 actual &&
test_cmp expect actual
 }
 
+test_three_modes () {
+   run_three_modes "test-tool reach $1"
+}
+
 test_expect_success 'ref_newer:miss' '
cat >input <<-\EOF &&
A:commit-5-7
@@ -219,7 +223,7 @@ test_expect_success 'commit_contains:hit' '
EOF
echo "commit_contains(_,A,X,_):1" >expect &&
test_three_modes commit_contains &&
-   test_three_modes commit_contains --tag
+   test_three_modes "commit_contains --tag"
 '
 
 test_expect_success 'commit_contains:miss' '
-- 
gitgitgadget

Re: [PATCH v2 6/6] t9109-git-svn-props.sh: split up several pipes

[Eric Sunshine]

On Mon, Sep 17, 2018 at 6:25 PM Matthew DeVore  wrote:
> t9109-git-svn-props.sh: split up several pipes

Similar to my comment about 5/6, this title talks about the mechanical
changes made by the patch but not the intent. Perhaps reword it like
this:

t9109: avoid swallowing Git exit code upstream of a pipe

> A test uses several separate pipe sequences in a row which are awkward
> to split up. Wrap the split-up pipe in a function so the awkwardness is
> not repeated.
>
> Signed-off-by: Matthew DeVore 
> ---
> diff --git a/t/t9101-git-svn-props.sh b/t/t9101-git-svn-props.sh
> @@ -190,16 +190,21 @@ EOF
> +# Note we avoid using pipes in order to ensure that git exits with 0.

This new comment doesn't really add value for someone reading the
patch without knowing the history leading up to the point the comment
was added. It should probably be dropped. (The actual text of the
comment is rather confusing anyhow since avoiding pipes has nothing to
do with ensuring that git exits with 0, thus another reason why this
comment ought to be dropped.)

>  test_expect_success 'test propget' "
> -   git svn propget svn:ignore . | cmp - prop.expect &&
> +   test_propget () {
> +   git svn propget $1 $2 >observed

The &&-chain is broken here, which means you're losing the exit status
from the Git command anyhow (despite the point of the patch being to
avoid losing it).

Also, for consistency, how about calling this "actual" rather than "observed"?

> +   cmp - $3

This is just wrong. The "-" argument to 'cmp' says to read from
standard input, but there is nothing being passed to 'cmp' on standard
input anymore now that you're removed the pipe. I'm guessing that you
really meant to use "observed" here (and reverse the order of
arguments to be consistent with the expect-then-actual idiom).
Finally, since these (apparently) might be binary, you can use
test_cmp_bin() instead.

> +   } &&
> +   test_propget svn:ignore . prop.expect &&
> cd deeply &&
> -   git svn propget svn:ignore . | cmp - ../prop.expect &&
> -   git svn propget svn:entry:committed-rev nested/directory/.keep \
> - | cmp - ../prop2.expect &&
> -   git svn propget svn:ignore .. | cmp - ../prop.expect &&
> -   git svn propget svn:ignore nested/ | cmp - ../prop.expect &&
> -   git svn propget svn:ignore ./nested | cmp - ../prop.expect &&
> -   git svn propget svn:ignore .././deeply/nested | cmp - ../prop.expect
> +   test_propget svn:ignore . ../prop.expect &&
> +   test_propget svn:entry:committed-rev nested/directory/.keep \
> +   ../prop2.expect &&
> +   test_propget svn:ignore .. ../prop.expect &&
> +   test_propget svn:ignore nested/ ../prop.expect &&
> +   test_propget svn:ignore ./nested ../prop.expect &&
> +   test_propget svn:ignore .././deeply/nested ../prop.expect
> "

After this patch, the test is even more broken than appears at first
glance since the test body is inside double-quotes. This means that
the $1, $2, $3 inside the test_propget() function are getting expanded
_before_ the function itself is ever defined, to whatever bogus values
$1, $2, $3 hold at that point. I can't see how this could ever have
worked (except only appearing to work by pure accident).

Re: [PATCH] t5551-http-fetch-smart.sh: sort cookies before comparing

[Junio C Hamano]

Jeff King  writes:

>> -cat >expect_cookies.txt <> +cat 
> This can be spelled:
>
>   sort >expect_cookies.txt <
> can't it? Then we do not even incur the extra process. :)

Yeah, true.  Running cat only to feed a pipe with contents of a
single file or the here-doc is an anti-pattern.

Re: [PATCH v2 5/6] tests: split up pipes

[Eric Sunshine]

On Mon, Sep 17, 2018 at 6:25 PM Matthew DeVore  wrote:
> tests: split up pipes

This title explains the mechanical changes the patch is making but not
the intent. Perhaps reword it to say something like:

tests: avoid swallowing Git exit code upstream of a pipe

> Some pipes in tests lose the exit code of git processes, which can mask
> unexpected behavior. Split these pipes up so that git commands are at
> the end of pipes rather than the beginning or middle.

Can you say something about how you chose which tests to fix in this
patch? Is this fixing all such cases or only a subset? It looks like
it's only fixing "ls-files" and "verify-pack" invocations. If that's
the case, the commit message should explain that.

Also, missing sign-off.

> ---
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> @@ -51,8 +51,10 @@ pull_to_client () {
> -   git symbolic-ref HEAD refs/heads/$(echo $heads |
> -   sed -e "s/^\(.\).*$/\1/") &&
> +   git symbolic-ref HEAD refs/heads/$(
> +   echo $heads |
> +   sed -e "s/^\(.\).*$/\1/"
> +   ) &&

Why is this change included in the patch? There is no Git invocation
upstream of a pipe here. While the cleanup itself may be desirable, it
doesn't belong in this patch.

Re: [PATCH v2 4/6] tests: Add linter check for pipe placement style

[Eric Sunshine]

On Mon, Sep 17, 2018 at 6:25 PM Matthew DeVore  wrote:
> tests: Add linter check for pipe placement style

Until now, the various "lint" checks have been for genuine portability
problems (except perhaps 'test-lint-duplicates'). This new lint check
makes style violations worthy of failing "make test". Is the indeed
the direction we want to go? (Genuine question. I can formulate
arguments for either side.)

> ---
> diff --git a/t/Makefile b/t/Makefile
> @@ -101,6 +101,16 @@ test-lint-filenames:
> +test-lint-pipes:
> +   @# Do not use \ to join lines when the next line starts with a
> +   @# pipe. Instead, end the prior line with the pipe, and allow that to
> +   @# join the lines implicitly.
> +   @bad="$$(${PERL_PATH} -n0e 'm/(\n[^\n|]+\\\n[\t ]+\|[^\n]*)/ and \
> + print qq{$$ARGV:$$1\n\n}' $(T))"; \
> +   test -z "$$bad" || { \
> +   printf >&2 "pipe at start of line in file(s):\n%s\n" "$$bad"; 
> \
> +   exit 1; }

If we're going in the direction of linting style violations, then
maybe generalize this by calling it "test-lint-style" rather than
"test-lint-pipes", and perhaps move the body of the test to a new
script check-shell-style.pl (or something), much as portability
violations are housed in check-non-portable-shell.pl.

Re: [PATCH v2 1/6] CodingGuidelines: add shell piping guidelines

[Eric Sunshine]

On Mon, Sep 17, 2018 at 6:24 PM Matthew DeVore  wrote:
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> @@ -163,6 +163,35 @@ For shell scripts specifically (not exhaustive):
> + - In a piped sequence which spans multiple lines, put each statement
> +   on a separate line and put pipes on the end of each line, rather
> +   than the start. This means you don't need to use \ to join lines,
> +   since | implies a join already. Also, do not indent subsequent
> +   lines; if you need a sequence to visually stand apart from the
> +   surrounding code, use a blank line before and/or after the piped
> +   sequence.
> +
> +   (incorrect)
> +   [...]
> +   (correct)
> +   echo '...' > expected

Existing tests seem to favor the name "expect" over "expected", so
perhaps use that instead.

$ git grep '>expect\b' -- t | wc -l
2674
$ git grep '>expected\b' -- t | wc -l
1406

> +   git ls-files -s file.1 file.2 file.3 file.4 file.5 |
> +   awk '{print $1}' |
> +   sort >observed

This is not a great example since it flatly contradicts the very next
bit of advice added by this patch about not placing a Git command
upstream in a pipe. Perhaps come up with an example which doesn't
suffer this shortcoming.

I've seen the advice earlier in the thread of not indenting the
sub-commands in a pipe, but I find that the result makes it far more
difficult to see which commands are part of the pipe sequence than
with them indented, so I'm not convinced that this advice should be in
the guidelines. (But that just my opinion.)

> + - In a pipe, any non-zero exit codes returned by processes besides
> +   the last will be ignored. If there is any possibility some
> +   non-final command in the pipe will raise an error, prefer writing
> +   the output of that command to a temporary file with '>' rather than
> +   pipe it.

It's not so much that we care about losing a non-zero exit code (which
might be perfectly acceptable depending upon the context) but that we
care about missing a Git command which outright crashes. So, it might
make sense to make this text more specific by saying that ("exit code
indicating a crash" and "Git command") rather than being generic in
saying only "exit code" and "command".

Also, what about expression like $(git foo) by which a crash of a Git
command can also be lost? Do we want to talk about that, as well?

Re: [PATCH] Add an EditorConfig file

[Taylor Blau]

Hi brian,

Thanks for CC-ing me on this.

I use editorconfig every day via the configuration in my home directory
[1], and the Vim plugin editorconfig-vim [2]. It's a great piece of
software, and I've been using it without any issue since around the
beginning of 2015.

On Mon, Sep 17, 2018 at 11:03:07PM +, brian m. carlson wrote:
> Regardless, providing such a file allows users to automatically
> configure their editor of choice with the correct settings by default.

I think that this is the central argument to be made here for keeping
this out of contrib/, and in the root tree. Most editor (all?) plugins
will pick this location up automatically, which ought to cut down on
patches that aren't formatted correctly.

> Provide global settings to set the character set to UTF-8 and insert a
> final newline into files.  Provide language-specific settings for C,
> Shell, Perl, and Python files according to what CodingGuidelines already
> specifies.  Since the indentation of other files varies, especially
> certain AsciiDoc files, don't provide any settings for them until a
> clear consensus forward emerges.
>
> Don't specify an end of line type.  While the Git community uses
> Unix-style line endings in the repository, some Windows users may use
> Git's auto-conversion support and forcing Unix-style line endings might
> cause problems for those users.

Good. Even the official editorconfig documentation specifies that this
ought to be the responsibility "of the VCS" [3], a point on which I
agree.

> +[*.{c,h,sh,perl}]
> +indent_style = tab
> +tab_width = 8

In all *.[ch] files in git.git, I found a total of 817 lines over 79
characters wide:

  $ for file in $(git ls-files '**/*.[ch]'); do
  awk 'length > 79' $f;
done | wc -l

So I think that specifying indent_style, and tab_width to be 'tab' and
'8' respectively is enough. We shouldn't be enforcing a rule about line
lengths that we are not ourselves consistently following.

Have you thought about including guidelines on COMMIT_EDITMSG? We prefer
72 characters per line [3], and this is enforceable via the following:

  [COMMIT_EDITMSG]
  max_line_length = 72

Thanks,
Taylor

[1]: 
https://github.com/ttaylorr/dotfiles/blob/work-gh/editorconfig/.editorconfig
[2]: https://github.com/editorconfig/editorconfig-vim
[3]: 
http://public-inbox.org/git/20170930070127.xvtn7dfyuoh26...@sigill.intra.peff.net

[PATCH] Add an EditorConfig file

[brian m. carlson]

Contributors to Git use a variety of editors, each with their own
configuration files.  Because C lacks the defined norms on how to indent
and style code that other languages, such as Ruby and Rust, have, it's
possible for various contributors, especially new ones, to have
configured their editor to use a style other than the style the Git
community prefers.

To make automatically configuring one's editor easier, provide an
EditorConfig file.  This is an INI-style configuration file that can be
used to specify editor settings and can be understood by a wide variety
of editors.  Some editors include this support natively; others require
a plugin.  Regardless, providing such a file allows users to
automatically configure their editor of choice with the correct settings
by default.

Provide global settings to set the character set to UTF-8 and insert a
final newline into files.  Provide language-specific settings for C,
Shell, Perl, and Python files according to what CodingGuidelines already
specifies.  Since the indentation of other files varies, especially
certain AsciiDoc files, don't provide any settings for them until a
clear consensus forward emerges.

Don't specify an end of line type.  While the Git community uses
Unix-style line endings in the repository, some Windows users may use
Git's auto-conversion support and forcing Unix-style line endings might
cause problems for those users.

Finally, leave out a root directive, which would prevent reading other
EditorConfig files higher up in the tree, in case someone wants to set
the end of line type for their system in such a file.

Signed-off-by: brian m. carlson 
---
I was incentivized to write this when I started using worktrees for
development and found that I had inserted spaces into all the work I was
doing because I was outside of my normal git.git clone.

This is the easiest way to set per-repo configuration across editors,
especially since people may work on various C codebases with different
indentation standards.

 .editorconfig | 11 +++
 1 file changed, 11 insertions(+)
 create mode 100644 .editorconfig

diff --git a/.editorconfig b/.editorconfig
new file mode 100644
index 00..8963d83fdb
--- /dev/null
+++ b/.editorconfig
@@ -0,0 +1,11 @@
+[*]
+charset = utf-8
+insert_final_newline = true
+
+[*.{c,h,sh,perl}]
+indent_style = tab
+tab_width = 8
+
+[*.py]
+indent_style = space
+indent_size = 4

Offre de prêts

[POPINET]

Bonjour

Vous aviez besoin de prêts d'argent entre particuliers pour faire face
aux difficultés financières pour enfin sortir de l'impasse que
provoquent les banques, par le rejet de vos dossiers de demande de
crédits ?
Je suis un citoyen français à la retraite en mesure de vous faire
un prêt de 5000 euros à 500 euros et avec des conditions qui vous
faciliteront la vie.Voici les domaines dans lesquels je peux vous
aider:
* Financier
* Prêt immobilier
* Prêt à l'investissement
* Prêt automobile
* Dette de consolidation
* Marge de crédit
* Deuxième hypothèque
* Rachat de crédit
* Prêt personnel
Vous êtes fichés, interdits bancaires et vous n'avez pas la faveur des
banques ou mieux vous avez un projet et besoin de financement, un
mauvais dossier de crédit ou besoin d'argent pour payer des factures,
fonds à investir dans les entreprises.
Alors si vous avez besoin de prêts d'argent n'hésitez pas à me
contacter par mail  popinetmic...@yahoo.com pour en savoir plus sur
mes conditions bien favorables.
Personne pas sérieux s'abstenir

Re: What's cooking in git.git (Sep 2018, #03; Fri, 14)

[Junio C Hamano]

Junio C Hamano  writes:

> The tip of 'next' hasn't been rewound yet, but the states of many
> topics that were marked to "Cook in 'next'" read "Will merge to
> 'master'" now.  I'll probably start merging a handful of topics that
> have been in 'next' down to 'master' tonight (they appear at the
> bottom of "log --first-parent master..pu" output).

A bunch of topics have been merged to 'master' and also to 'next'
with today's update.  The tip of 'next' hasn't been rewound.  I am
hoping that we can thin down the difference between 'master' and
'next' by merging a bit more topic from 'next' to 'master' without
adding more topics to 'next' for a week or so, and then rebuild
'next' at the end of that period, and then start to accept more
topics to 'next'.

RE: [Question] Alternative to git-lfs under go

[Randall S. Becker]

On September 17, 2018 6:02 PM, Jonathan Nieder wrote:
> Randall S. Becker wrote:
> > On September 17, 2018 3:28 PM, Jonathan Nieder wrote:
> >> Randall S. Becker wrote:
> 
> >>> Does anyone know whether it is practical to rework git-lfs under a
> >>> language other than "go"? GCC is not even close to being possible to
> >>> port to my NonStop platform (may have tried, some have died - joke -
> >>> trying). I would like to convert this directly to C or something
> >>> more widely portable. Is there a protocol doc out there I can reference?
> >>
> >> Can you say more about the context?  You might like
> >>
> >>  git clone --filter=blob:limit=512m 
> >>
> >> which tells Git to avoid downloading any blobs larger than 512
> >> megabytes until you know they need them.  See
> >> Documentation/technical/partial-clone.txt
> >> for more details.
> >
> > Sorry, I was not clear. I am not having issues with large files or
> > blob limits.  Members of my community wish to use Git LFS support on
> > their enterprise git servers, so as platform maintainer for git on
> > NonStop, I am trying to accommodate them. The stumbling block is that
> > "Go" language will not port to the platform.
> 
> Thanks.  Then the answer is "no": there has not been a reimplementation of
> Git LFS in another language, and my advice to someone pursuing that would
> be to use the native Git feature described above instead (or to get Go
> working on their platform).
> 
> If I'm reading
> https://github.com/git-lfs/git-lfs/blob/master/CONTRIBUTING.md
> correctly, the preferred way to contact the Git LFS maintainers is using
> Github's issue tracker.
> 
> Thanks and sorry I don't have better news, Jonathan

It is the interoperability with existing Git LFS repositories that my community 
is requesting, not specifically any file size issues. Some want the locking 
function. The API looks fairly straight forward and I suspect a complete 
reimplementation will take less of my time than trying to get GO to actually 
run on the platform. Cross-compilation might be an option, but that's likely a 
tricky proposition that has not worked in the past - the issue there is that 
cross compilation requires Windows and Cygwin, which make configure rather 
confused and so far, not workable. Neither GCC nor GNULIB build completely in 
port attempts. So the answer is "NO". ☹

Re: [PATCH] t5551-http-fetch-smart.sh: sort cookies before comparing

[Jeff King]

On Mon, Sep 17, 2018 at 02:45:55PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Yes, I think sorting the expect file would work fine. I'm OK with that,
> > or just leaving a comment. The comment has the bonus that it does not
> > cost an extra process at runtime. I'd probably use a sort if we expected
> > the list to be long and complicated, since it makes life easier on a
> > future developer. But since there are only 2 lines, I don't think it's a
> > big deal either way (or even just leaving it as-is without a comment is
> > probably OK).
> 
> Let's have "| sort" if only for its documentation value.  That way
> we do not have to remember the list must be sorted.

OK, though...

> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index 771f36f9ff..d13b993201 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -206,7 +206,7 @@ test_expect_success 'dumb clone via http-backend respects 
> namespace' '
>  cat >cookies.txt <  127.0.0.1FALSE   /smart_cookies/ FALSE   0   othername   
> othervalue
>  EOF
> -cat >expect_cookies.txt < +cat expect_cookies.txt <

[PATCH v2 5/6] tests: split up pipes

[Matthew DeVore]

Some pipes in tests lose the exit code of git processes, which can mask
unexpected behavior. Split these pipes up so that git commands are at
the end of pipes rather than the beginning or middle.
---
 t/t5317-pack-objects-filter-objects.sh | 156 +
 t/t5500-fetch-pack.sh  |   6 +-
 t/t5616-partial-clone.sh   |  14 ++-
 t/t6112-rev-list-filters-objects.sh| 103 
 t/t9101-git-svn-props.sh   |   3 +-
 5 files changed, 147 insertions(+), 135 deletions(-)

diff --git a/t/t5317-pack-objects-filter-objects.sh 
b/t/t5317-pack-objects-filter-objects.sh
index c093eb891..2e718f0bd 100755
--- a/t/t5317-pack-objects-filter-objects.sh
+++ b/t/t5317-pack-objects-filter-objects.sh
@@ -20,8 +20,9 @@ test_expect_success 'setup r1' '
 '
 
 test_expect_success 'verify blob count in normal packfile' '
-   git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 |
-   awk -f print_2.awk |
+   git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 \
+   >ls_files_result &&
+   awk -f print_2.awk ls_files_result |
sort >expected &&
 
git -C r1 pack-objects --rev --stdout >all.pack <<-EOF &&
@@ -29,8 +30,8 @@ test_expect_success 'verify blob count in normal packfile' '
EOF
git -C r1 index-pack ../all.pack &&
 
-   git -C r1 verify-pack -v ../all.pack |
-   grep blob |
+   git -C r1 verify-pack -v ../all.pack >verify_result &&
+   grep blob verify_result |
awk -f print_1.awk |
sort >observed &&
 
@@ -43,8 +44,8 @@ test_expect_success 'verify blob:none packfile has no blobs' '
EOF
git -C r1 index-pack ../filter.pack &&
 
-   git -C r1 verify-pack -v ../filter.pack |
-   grep blob |
+   git -C r1 verify-pack -v ../filter.pack >verify_result &&
+   grep blob verify_result |
awk -f print_1.awk |
sort >observed &&
 
@@ -53,13 +54,13 @@ test_expect_success 'verify blob:none packfile has no 
blobs' '
 '
 
 test_expect_success 'verify normal and blob:none packfiles have same 
commits/trees' '
-   git -C r1 verify-pack -v ../all.pack |
-   grep -E "commit|tree" |
+   git -C r1 verify-pack -v ../all.pack >verify_result &&
+   grep -E "commit|tree" verify_result |
awk -f print_1.awk |
sort >expected &&
 
-   git -C r1 verify-pack -v ../filter.pack |
-   grep -E "commit|tree" |
+   git -C r1 verify-pack -v ../filter.pack >verify_result &&
+   grep -E "commit|tree" verify_result |
awk -f print_1.awk |
sort >observed &&
 
@@ -82,8 +83,8 @@ test_expect_success 'setup r2' '
 '
 
 test_expect_success 'verify blob count in normal packfile' '
-   git -C r2 ls-files -s large.1000 large.1 |
-   awk -f print_2.awk |
+   git -C r2 ls-files -s large.1000 large.1 >ls_files_result &&
+   awk -f print_2.awk ls_files_result |
sort >expected &&
 
git -C r2 pack-objects --rev --stdout >all.pack <<-EOF &&
@@ -91,8 +92,8 @@ test_expect_success 'verify blob count in normal packfile' '
EOF
git -C r2 index-pack ../all.pack &&
 
-   git -C r2 verify-pack -v ../all.pack |
-   grep blob |
+   git -C r2 verify-pack -v ../all.pack >verify_result &&
+   grep blob verify_result |
awk -f print_1.awk |
sort >observed &&
 
@@ -105,8 +106,8 @@ test_expect_success 'verify blob:limit=500 omits all blobs' 
'
EOF
git -C r2 index-pack ../filter.pack &&
 
-   git -C r2 verify-pack -v ../filter.pack |
-   grep blob |
+   git -C r2 verify-pack -v ../filter.pack >verify_result &&
+   grep blob verify_result |
awk -f print_1.awk |
sort >observed &&
 
@@ -120,8 +121,8 @@ test_expect_success 'verify blob:limit=1000' '
EOF
git -C r2 index-pack ../filter.pack &&
 
-   git -C r2 verify-pack -v ../filter.pack |
-   grep blob |
+   git -C r2 verify-pack -v ../filter.pack >verify_result &&
+   grep blob verify_result |
awk -f print_1.awk |
sort >observed &&
 
@@ -130,8 +131,8 @@ test_expect_success 'verify blob:limit=1000' '
 '
 
 test_expect_success 'verify blob:limit=1001' '
-   git -C r2 ls-files -s large.1000 |
-   awk -f print_2.awk |
+   git -C r2 ls-files -s large.1000 >ls_files_result &&
+   awk -f print_2.awk ls_files_result |
sort >expected &&
 
git -C r2 pack-objects --rev --stdout --filter=blob:limit=1001 
>filter.pack <<-EOF &&
@@ -139,8 +140,8 @@ test_expect_success 'verify blob:limit=1001' '
EOF
git -C r2 index-pack ../filter.pack &&
 
-   git -C r2 verify-pack -v ../filter.pack |
-   grep blob |
+   git -C r2 verify-pack -v ../filter.pack >verify_result &&
+   grep blob verify_result |
awk -f print_1.awk |
sort >observed &&
 
@@ -148,8 +149,8 @@ test_expect_success 'verify blob:limit=1001' '
 '
 
 test_expect_success 'verify 

[PATCH v2 3/6] t/*: fix ordering of expected/observed arguments

[Matthew DeVore]

Fix various places where the ordering was obviously wrong, meaning it
was easy to find with grep.

Signed-off-by: Matthew DeVore 
---
 t/t-basic.sh   |  2 +-
 t/t0021-conversion.sh  |  4 +--
 t/t1300-config.sh  |  4 +--
 t/t1303-wacky-config.sh|  4 +--
 t/t2101-update-index-reupdate.sh   |  2 +-
 t/t3200-branch.sh  |  2 +-
 t/t3320-notes-merge-worktrees.sh   |  4 +--
 t/t3400-rebase.sh  |  8 +++---
 t/t3417-rebase-whitespace-fix.sh   |  6 ++---
 t/t3702-add-edit.sh|  4 +--
 t/t3903-stash.sh   |  8 +++---
 t/t3905-stash-include-untracked.sh |  2 +-
 t/t4025-hunk-header.sh |  2 +-
 t/t4117-apply-reject.sh|  6 ++---
 t/t4124-apply-ws-rule.sh   | 30 +++
 t/t4138-apply-ws-expansion.sh  |  2 +-
 t/t5317-pack-objects-filter-objects.sh | 34 +-
 t/t5318-commit-graph.sh|  2 +-
 t/t5701-git-serve.sh   | 14 +--
 t/t5702-protocol-v2.sh | 10 
 t/t6023-merge-file.sh  | 12 -
 t/t6027-merge-binary.sh|  4 +--
 t/t6031-merge-filemode.sh  |  2 +-
 t/t6112-rev-list-filters-objects.sh| 24 +-
 t/t7201-co.sh  |  4 +--
 t/t7406-submodule-update.sh|  8 +++---
 t/t7800-difftool.sh|  2 +-
 t/t9100-git-svn-basic.sh   |  2 +-
 t/t9133-git-svn-nested-git-repo.sh |  6 ++---
 t/t9600-cvsimport.sh   |  2 +-
 t/t9603-cvsimport-patchsets.sh |  4 +--
 t/t9604-cvsimport-timestamps.sh|  4 +--
 32 files changed, 112 insertions(+), 112 deletions(-)

diff --git a/t/t-basic.sh b/t/t-basic.sh
index 850f651e4..224c098a8 100755
--- a/t/t-basic.sh
+++ b/t/t-basic.sh
@@ -1018,7 +1018,7 @@ test_expect_success SHA1 'validate git diff-files output 
for a know cache/work t
 :12 12 6649a1ebe9e9f1c553b66f5a6e74136a07ccc57c 
 M path3/subp3/file3sym
 EOF
git diff-files >current &&
-   test_cmp current expected
+   test_cmp expected current
 '
 
 test_expect_success 'git update-index --refresh should succeed' '
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 308cd28f3..fd5f1ac64 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -166,10 +166,10 @@ test_expect_success expanded_in_repo '
rm -f expanded-keywords expanded-keywords-crlf &&
 
git checkout -- expanded-keywords &&
-   test_cmp expanded-keywords expected-output &&
+   test_cmp expected-output expanded-keywords &&
 
git checkout -- expanded-keywords-crlf &&
-   test_cmp expanded-keywords-crlf expected-output-crlf
+   test_cmp expected-output-crlf expanded-keywords-crlf
 '
 
 # The use of %f in a filter definition is expanded to the path to
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 5869d6cb6..e2cd50ecf 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1001,7 +1001,7 @@ EOF
 
 test_expect_success 'value continued on next line' '
git config --list > result &&
-   test_cmp result expect
+   test_cmp expect result
 '
 
 cat > .git/config <<\EOF
@@ -1882,7 +1882,7 @@ test_expect_success '--replace-all does not invent 
newlines' '
Qkey = b
EOF
git config --replace-all abc.key b &&
-   test_cmp .git/config expect
+   test_cmp expect .git/config
 '
 
 test_done
diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh
index 3b92083e1..e664e 100755
--- a/t/t1303-wacky-config.sh
+++ b/t/t1303-wacky-config.sh
@@ -14,7 +14,7 @@ setup() {
 check() {
echo "$2" >expected
git config --get "$1" >actual 2>&1
-   test_cmp actual expected
+   test_cmp expected actual
 }
 
 # 'check section.key regex value' verifies that the entry for
@@ -22,7 +22,7 @@ check() {
 check_regex() {
echo "$3" >expected
git config --get "$1" "$2" >actual 2>&1
-   test_cmp actual expected
+   test_cmp expected actual
 }
 
 test_expect_success 'modify same key' '
diff --git a/t/t2101-update-index-reupdate.sh b/t/t2101-update-index-reupdate.sh
index 685ec4563..6c32d42c8 100755
--- a/t/t2101-update-index-reupdate.sh
+++ b/t/t2101-update-index-reupdate.sh
@@ -73,7 +73,7 @@ test_expect_success 'update-index --update from subdir' '
100644 $(git hash-object dir1/file3) 0  dir1/file3
100644 $file2 0 file2
EOF
-   test_cmp current expected
+   test_cmp expected current
 '
 
 test_expect_success 'update-index --update with pathspec' '
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 93f21ab07..478b82cf9 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1221,7 +1221,7 @@ test_expect_success 'use --edit-description' '
EOF
 

[PATCH v2 2/6] tests: standardize pipe placement

[Matthew DeVore]

Instead of using a line-continuation and pipe on the second line, take
advantage of the shell's implicit line continuation after a pipe
character.  So for example, instead of

some long line \
| next line

use

some long line |
next line

And add a blank line before and after the pipe where it aids readability
(it usually does).

This better matches the coding style documented in
Documentation/CodingGuidelines and used in shell scripts elsewhere in
the tree.

Signed-off-by: Matthew DeVore 
---
 t/lib-gpg.sh   |   9 +-
 t/t1006-cat-file.sh|   8 +-
 t/t1300-config.sh  |   5 +-
 t/t5317-pack-objects-filter-objects.sh | 330 ++---
 t/t5500-fetch-pack.sh  |   5 +-
 t/t5616-partial-clone.sh   |  32 ++-
 t/t6112-rev-list-filters-objects.sh| 203 ---
 7 files changed, 342 insertions(+), 250 deletions(-)

diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index 3fe02876c..f1277bef4 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -57,9 +57,12 @@ then
echo | gpgsm --homedir "${GNUPGHOME}" 2>/dev/null \
--passphrase-fd 0 --pinentry-mode loopback \
--import "$TEST_DIRECTORY"/lib-gpg/gpgsm_cert.p12 &&
-   gpgsm --homedir "${GNUPGHOME}" 2>/dev/null -K \
-   | grep fingerprint: | cut -d" " -f4 | tr -d '\n' > \
-   ${GNUPGHOME}/trustlist.txt &&
+
+   gpgsm --homedir "${GNUPGHOME}" 2>/dev/null -K |
+   grep fingerprint: |
+   cut -d" " -f4 |
+   tr -d '\n' >"${GNUPGHOME}/trustlist.txt" &&
+
echo " S relax" >> ${GNUPGHOME}/trustlist.txt &&
(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
echo hello | gpgsm --homedir "${GNUPGHOME}" >/dev/null \
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 7f19d591f..a0fa926d3 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -218,8 +218,8 @@ test_expect_success "--batch-check for a non-existent hash" 
'
 test "0042 missing
 0084 missing" = \
 "$( ( echo 0042;
- echo_without_newline 0084; ) \
-   | git cat-file --batch-check)"
+ echo_without_newline 0084; ) |
+   git cat-file --batch-check)"
 '
 
 test_expect_success "--batch for an existent and a non-existent hash" '
@@ -227,8 +227,8 @@ test_expect_success "--batch for an existent and a 
non-existent hash" '
 $tag_content
  missing" = \
 "$( ( echo $tag_sha1;
- echo_without_newline ; ) \
-   | git cat-file --batch)"
+ echo_without_newline ; ) |
+   git cat-file --batch)"
 '
 
 test_expect_success "--batch-check for an empty line" '
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index cdf1fed5d..5869d6cb6 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1770,8 +1770,9 @@ test_expect_success '--show-origin stdin with file 
include' '
cat >expect <<-EOF &&
file:$INCLUDE_DIR/stdin.include include
EOF
-   echo "[include]path=\"$INCLUDE_DIR\"/stdin.include" \
-   | git config --show-origin --includes --file - user.stdin 
>output &&
+   echo "[include]path=\"$INCLUDE_DIR\"/stdin.include" |
+   git config --show-origin --includes --file - user.stdin >output &&
+
test_cmp expect output
 '
 
diff --git a/t/t5317-pack-objects-filter-objects.sh 
b/t/t5317-pack-objects-filter-objects.sh
index 6710c8bc8..ce69148ec 100755
--- a/t/t5317-pack-objects-filter-objects.sh
+++ b/t/t5317-pack-objects-filter-objects.sh
@@ -20,17 +20,20 @@ test_expect_success 'setup r1' '
 '
 
 test_expect_success 'verify blob count in normal packfile' '
-   git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 \
-   | awk -f print_2.awk \
-   | sort >expected &&
+   git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 |
+   awk -f print_2.awk |
+   sort >expected &&
+
git -C r1 pack-objects --rev --stdout >all.pack <<-EOF &&
HEAD
EOF
git -C r1 index-pack ../all.pack &&
-   git -C r1 verify-pack -v ../all.pack \
-   | grep blob \
-   | awk -f print_1.awk \
-   | sort >observed &&
+
+   git -C r1 verify-pack -v ../all.pack |
+   grep blob |
+   awk -f print_1.awk |
+   sort >observed &&
+
test_cmp observed expected
 '
 
@@ -39,23 +42,27 @@ test_expect_success 'verify blob:none packfile has no 
blobs' '
HEAD
EOF
git -C r1 index-pack ../filter.pack &&
-   git -C r1 verify-pack -v ../filter.pack \

[PATCH v2 1/6] CodingGuidelines: add shell piping guidelines

[Matthew DeVore]

Add two guidelines:

 - pipe characters should appear at the end of lines, and not cause
   indentation
 - pipes should be avoided when they swallow exit codes that can
   potentially fail
---
 Documentation/CodingGuidelines | 29 +
 1 file changed, 29 insertions(+)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 48aa4edfb..7f903c1aa 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -163,6 +163,35 @@ For shell scripts specifically (not exhaustive):
 
does not have such a problem.
 
+ - In a piped sequence which spans multiple lines, put each statement
+   on a separate line and put pipes on the end of each line, rather
+   than the start. This means you don't need to use \ to join lines,
+   since | implies a join already. Also, do not indent subsequent
+   lines; if you need a sequence to visually stand apart from the
+   surrounding code, use a blank line before and/or after the piped
+   sequence.
+
+   (incorrect)
+   echo '...' > expected
+   git ls-files -s file.1 file.2 file.3 file.4 file.5 \
+   | awk '{print $1}' \
+   | sort >observed
+   test_cmp expected actual
+
+   (correct)
+   echo '...' > expected
+
+   git ls-files -s file.1 file.2 file.3 file.4 file.5 |
+   awk '{print $1}' |
+   sort >observed
+
+   test_cmp expected actual
+
+ - In a pipe, any non-zero exit codes returned by processes besides
+   the last will be ignored. If there is any possibility some
+   non-final command in the pipe will raise an error, prefer writing
+   the output of that command to a temporary file with '>' rather than
+   pipe it.
 
 For C programs:
 
-- 
2.19.0.444.g18242da7ef-goog

[PATCH v2 6/6] t9109-git-svn-props.sh: split up several pipes

[Matthew DeVore]

A test uses several separate pipe sequences in a row which are awkward
to split up. Wrap the split-up pipe in a function so the awkwardness is
not repeated.

Signed-off-by: Matthew DeVore 
---
 t/t9101-git-svn-props.sh | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/t/t9101-git-svn-props.sh b/t/t9101-git-svn-props.sh
index 8cba331fc..952bd4814 100755
--- a/t/t9101-git-svn-props.sh
+++ b/t/t9101-git-svn-props.sh
@@ -190,16 +190,21 @@ EOF
 # This test can be improved: since all the svn:ignore contain the same
 # pattern, it can pass even though the propget did not execute on the
 # right directory.
+# Note we avoid using pipes in order to ensure that git exits with 0.
 test_expect_success 'test propget' "
-   git svn propget svn:ignore . | cmp - prop.expect &&
+   test_propget () {
+   git svn propget $1 $2 >observed
+   cmp - $3
+   } &&
+   test_propget svn:ignore . prop.expect &&
cd deeply &&
-   git svn propget svn:ignore . | cmp - ../prop.expect &&
-   git svn propget svn:entry:committed-rev nested/directory/.keep \
- | cmp - ../prop2.expect &&
-   git svn propget svn:ignore .. | cmp - ../prop.expect &&
-   git svn propget svn:ignore nested/ | cmp - ../prop.expect &&
-   git svn propget svn:ignore ./nested | cmp - ../prop.expect &&
-   git svn propget svn:ignore .././deeply/nested | cmp - ../prop.expect
+   test_propget svn:ignore . ../prop.expect &&
+   test_propget svn:entry:committed-rev nested/directory/.keep \
+   ../prop2.expect &&
+   test_propget svn:ignore .. ../prop.expect &&
+   test_propget svn:ignore nested/ ../prop.expect &&
+   test_propget svn:ignore ./nested ../prop.expect &&
+   test_propget svn:ignore .././deeply/nested ../prop.expect
"
 
 cat >prop.expect <<\EOF
-- 
2.19.0.444.g18242da7ef-goog

[PATCH v2 4/6] tests: add linter check for pipe placement style

[Matthew DeVore]

---
 t/Makefile | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/t/Makefile b/t/Makefile
index c83fd1886..4eceabbd5 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -78,7 +78,7 @@ check-chainlint:
done && exit $$err
 
 test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \
-   test-lint-filenames
+   test-lint-filenames test-lint-pipes
 
 test-lint-duplicates:
@dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \
@@ -101,6 +101,16 @@ test-lint-filenames:
test -z "$$bad" || { \
echo >&2 "non-portable file name(s): $$bad"; exit 1; }
 
+test-lint-pipes:
+   @# Do not use \ to join lines when the next line starts with a
+   @# pipe. Instead, end the prior line with the pipe, and allow that to
+   @# join the lines implicitly.
+   @bad="$$(${PERL_PATH} -n0e 'm/(\n[^\n|]+\\\n[\t ]+\|[^\n]*)/ and \
+ print qq{$$ARGV:$$1\n\n}' $(T))"; \
+   test -z "$$bad" || { \
+   printf >&2 "pipe at start of line in file(s):\n%s\n" "$$bad"; \
+   exit 1; }
+
 aggregate-results-and-cleanup: $(T)
$(MAKE) aggregate-results
$(MAKE) clean
-- 
2.19.0.444.g18242da7ef-goog

[PATCH v2 4/6] tests: Add linter check for pipe placement style

[Matthew DeVore]

---
 t/Makefile | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/t/Makefile b/t/Makefile
index c83fd1886..4eceabbd5 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -78,7 +78,7 @@ check-chainlint:
done && exit $$err
 
 test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \
-   test-lint-filenames
+   test-lint-filenames test-lint-pipes
 
 test-lint-duplicates:
@dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \
@@ -101,6 +101,16 @@ test-lint-filenames:
test -z "$$bad" || { \
echo >&2 "non-portable file name(s): $$bad"; exit 1; }
 
+test-lint-pipes:
+   @# Do not use \ to join lines when the next line starts with a
+   @# pipe. Instead, end the prior line with the pipe, and allow that to
+   @# join the lines implicitly.
+   @bad="$$(${PERL_PATH} -n0e 'm/(\n[^\n|]+\\\n[\t ]+\|[^\n]*)/ and \
+ print qq{$$ARGV:$$1\n\n}' $(T))"; \
+   test -z "$$bad" || { \
+   printf >&2 "pipe at start of line in file(s):\n%s\n" "$$bad"; \
+   exit 1; }
+
 aggregate-results-and-cleanup: $(T)
$(MAKE) aggregate-results
$(MAKE) clean
-- 
2.19.0.444.g18242da7ef-goog

[PATCH v2 0/6] Clean up tests for test_cmp arg ordering and pipe placement

[Matthew DeVore]

This applies the suggestions from jrn@'s response to v1 here:
https://public-inbox.org/git/20180917163137.gb89...@aiede.svl.corp.google.com/

The major changes are:
 - Added two bullet points to Documentation/CodingGuidelines about 1) how to
   format pipe sequences and 2) using temporary files rather than pipes to
   avoid masking potiential error exit codes.
 - A new commit which introduces a linter check to enforce (1) above, which I
   am requesting feedback for. I'd be happy to drop it.
 - A new commit to fix violations of (2) above in t9101-git-svn-props.sh since
   it was not so trivial.
 - A better message to describe the pipe placement commit.

Thank you,

Matthew DeVore (6):
  CodingGuidelines: add shell piping guidelines
  tests: standardize pipe placement
  t/*: fix ordering of expected/observed arguments
  tests: Add linter check for pipe placement style
  tests: split up pipes
  t9109-git-svn-props.sh: split up several pipes

 Documentation/CodingGuidelines |  29 ++
 t/Makefile |  12 +-
 t/lib-gpg.sh   |   9 +-
 t/t-basic.sh   |   2 +-
 t/t0021-conversion.sh  |   4 +-
 t/t1006-cat-file.sh|   8 +-
 t/t1300-config.sh  |   9 +-
 t/t1303-wacky-config.sh|   4 +-
 t/t2101-update-index-reupdate.sh   |   2 +-
 t/t3200-branch.sh  |   2 +-
 t/t3320-notes-merge-worktrees.sh   |   4 +-
 t/t3400-rebase.sh  |   8 +-
 t/t3417-rebase-whitespace-fix.sh   |   6 +-
 t/t3702-add-edit.sh|   4 +-
 t/t3903-stash.sh   |   8 +-
 t/t3905-stash-include-untracked.sh |   2 +-
 t/t4025-hunk-header.sh |   2 +-
 t/t4117-apply-reject.sh|   6 +-
 t/t4124-apply-ws-rule.sh   |  30 +--
 t/t4138-apply-ws-expansion.sh  |   2 +-
 t/t5317-pack-objects-filter-objects.sh | 360 ++---
 t/t5318-commit-graph.sh|   2 +-
 t/t5500-fetch-pack.sh  |   7 +-
 t/t5616-partial-clone.sh   |  30 ++-
 t/t5701-git-serve.sh   |  14 +-
 t/t5702-protocol-v2.sh |  10 +-
 t/t6023-merge-file.sh  |  12 +-
 t/t6027-merge-binary.sh|   4 +-
 t/t6031-merge-filemode.sh  |   2 +-
 t/t6112-rev-list-filters-objects.sh| 224 ---
 t/t7201-co.sh  |   4 +-
 t/t7406-submodule-update.sh|   8 +-
 t/t7800-difftool.sh|   2 +-
 t/t9100-git-svn-basic.sh   |   2 +-
 t/t9101-git-svn-props.sh   |  24 +-
 t/t9133-git-svn-nested-git-repo.sh |   6 +-
 t/t9600-cvsimport.sh   |   2 +-
 t/t9603-cvsimport-patchsets.sh |   4 +-
 t/t9604-cvsimport-timestamps.sh|   4 +-
 39 files changed, 511 insertions(+), 363 deletions(-)

-- 
2.19.0.444.g18242da7ef-goog

RE: [Question] Alternative to git-lfs under go

[Randall S. Becker]

On September 17, 2018 6:01 PM, Taylor Blau wrote:
> On Mon, Sep 17, 2018 at 05:55:55PM -0400, Randall S. Becker wrote:
> > On September 17, 2018 3:28 PM, Jonathan Nieder wrote:
> > > Randall S. Becker wrote:
> > >
> > > > Does anyone know whether it is practical to rework git-lfs under a
> > > > language other than "go"? GCC is not even close to being possible
> > > > to port to my NonStop platform (may have tried, some have died -
> > > > joke - trying). I would like to convert this directly to C or
> > > > something more widely portable. Is there a protocol doc out there I
can
> reference?
> > >
> > > Can you say more about the context?  You might like
> > >
> > >  git clone --filter=blob:limit=512m 
> > >
> > > which tells Git to avoid downloading any blobs larger than 512
> > > megabytes until you know they need them.  See
> > > Documentation/technical/partial- clone.txt for more details.
> >
> > Sorry, I was not clear. I am not having issues with large files or
> > blob limits.  Members of my community wish to use Git LFS support on
> > their enterprise git servers, so as platform maintainer for git on
> > NonStop, I am trying to accommodate them. The stumbling block is that
> > "Go" language will not port to the platform.
> 
> We have an open-source specification here [1], and the rest of our API
> documentation is in here [2].
> 
> Does that help?
> 
> [1]: https://github.com/git-lfs/git-lfs/blob/master/docs/spec.md
> [2]: https://github.com/git-lfs/git-lfs/tree/master/docs/api

Very much, thank you!

Re: [PATCH] mailmap: consistently normalize brian m. carlson's name

[brian m. carlson]

On Mon, Sep 17, 2018 at 11:18:00AM -0700, Jonathan Nieder wrote:
> v2.18.0-rc0~70^2 (mailmap: update brian m. carlson's email address,
> 2018-05-08) changed the mailmap to map
> 
>   sand...@crustytoothpaste.ath.cx
>-> brian m. carlson 
> 
> instead of
> 
>   sand...@crustytoothpaste.net
> -> brian m. carlson 
> 
> That means the mapping
> 
>   Brian M. Carlson 
> -> brian m. carlson 
> 
> is redundant, so we can remove it.  More importantly, it means that
> the identity "Brian M. Carlson " used in
> some commits is not normalized any more.  Add a mapping for it.
> 
> Noticed while updating Debian's Git packaging, which uses "git
> shortlog --no-merges" to produce a list of changes in each version,
> grouped by author's (normalized) name.

I think this commit message makes sense.  I apparently still fail to
understand how the .mailmap format works, so I can't tell you if the
patch is correct.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [Question] Alternative to git-lfs under go

[Jonathan Nieder]

(+cc: Taylor Blau, git-lfs expert)
Randall S. Becker wrote:
> On September 17, 2018 3:28 PM, Jonathan Nieder wrote:
>> Randall S. Becker wrote:

>>> Does anyone know whether it is practical to rework git-lfs under a
>>> language other than "go"? GCC is not even close to being possible to
>>> port to my NonStop platform (may have tried, some have died - joke -
>>> trying). I would like to convert this directly to C or something more
>>> widely portable. Is there a protocol doc out there I can reference?
>>
>> Can you say more about the context?  You might like
>>
>>  git clone --filter=blob:limit=512m 
>>
>> which tells Git to avoid downloading any blobs larger than 512 megabytes
>> until you know they need them.  See Documentation/technical/partial-clone.txt
>> for more details.
>
> Sorry, I was not clear. I am not having issues with large files or blob
> limits.  Members of my community wish to use Git LFS support on their
> enterprise git servers, so as platform maintainer for git on NonStop, I am
> trying to accommodate them. The stumbling block is that "Go" language will
> not port to the platform.

Thanks.  Then the answer is "no": there has not been a
reimplementation of Git LFS in another language, and my advice to
someone pursuing that would be to use the native Git feature described
above instead (or to get Go working on their platform).

If I'm reading
https://github.com/git-lfs/git-lfs/blob/master/CONTRIBUTING.md
correctly, the preferred way to contact the Git LFS maintainers is
using Github's issue tracker.

Thanks and sorry I don't have better news,
Jonathan

Re: [Question] Alternative to git-lfs under go

[Taylor Blau]

Hi Randall,

On Mon, Sep 17, 2018 at 05:55:55PM -0400, Randall S. Becker wrote:
> On September 17, 2018 3:28 PM, Jonathan Nieder wrote:
> > Randall S. Becker wrote:
> >
> > > Does anyone know whether it is practical to rework git-lfs under a
> > > language other than "go"? GCC is not even close to being possible to
> > > port to my NonStop platform (may have tried, some have died - joke -
> > > trying). I would like to convert this directly to C or something more
> > > widely portable. Is there a protocol doc out there I can reference?
> >
> > Can you say more about the context?  You might like
> >
> >  git clone --filter=blob:limit=512m 
> >
> > which tells Git to avoid downloading any blobs larger than 512 megabytes
> > until you know they need them.  See Documentation/technical/partial-
> > clone.txt
> > for more details.
>
> Sorry, I was not clear. I am not having issues with large files or blob
> limits.  Members of my community wish to use Git LFS support on their
> enterprise git servers, so as platform maintainer for git on NonStop, I am
> trying to accommodate them. The stumbling block is that "Go" language will
> not port to the platform.

We have an open-source specification here [1], and the rest of our API
documentation is in here [2].

Does that help?

Thanks,
Taylor

[1]: https://github.com/git-lfs/git-lfs/blob/master/docs/spec.md
[2]: https://github.com/git-lfs/git-lfs/tree/master/docs/api

RE: [Question] Alternative to git-lfs under go

[Randall S. Becker]

On September 17, 2018 3:28 PM, Jonathan Nieder wrote:
> Randall S. Becker wrote:
> 
> > Does anyone know whether it is practical to rework git-lfs under a
> > language other than "go"? GCC is not even close to being possible to
> > port to my NonStop platform (may have tried, some have died - joke -
> > trying). I would like to convert this directly to C or something more
> > widely portable. Is there a protocol doc out there I can reference?
> 
> Can you say more about the context?  You might like
> 
>  git clone --filter=blob:limit=512m 
> 
> which tells Git to avoid downloading any blobs larger than 512 megabytes
> until you know they need them.  See Documentation/technical/partial-
> clone.txt
> for more details.

Sorry, I was not clear. I am not having issues with large files or blob
limits.  Members of my community wish to use Git LFS support on their
enterprise git servers, so as platform maintainer for git on NonStop, I am
trying to accommodate them. The stumbling block is that "Go" language will
not port to the platform.

Cheers,
Randall

Re: [PATCH v1 1/2] t/*: fix pipe placement and remove \'s

[Matthew DeVore]

On Mon, Sep 17, 2018 at 9:31 AM Jonathan Nieder  wrote:
>
> Matthew DeVore wrote:
>
> > Subject: t/*: fix pipe placement and remove \'s
> >
> > Where ever there was code in the tests like this:
> >
> >   foo \
> >   | bar
>
> Language nits:
> - s/Where ever/Wherever/
> - Git's commit messages use the present tense to describe the existing
>   previous state of the codebase, as though reporting a bug.
>
> Maybe something like
>
> tests: standardize pipe placement
>
> Instead of using a line-continuation and pipe on the second
> line, take advantage of the shell's implicit line continuation
> after a pipe character.  So for example, instead of
>
> some long line \
> | next line
>
> use
>
> some long line |
> next line
>
> At this point, it would be useful to say something about rationale ---
> for example,
>
> This better matches the coding style documented in
> Documentation/CodingGuidelines and used in shell scripts
> elsewhere in Git.
>
Done.

> Except: is this documented in Documentation/CodingGuidelines?  Or,
> better, is there a linter that we can run in the test-lint target of
> t/Makefile to ensure we keep sticking to this style?
It's not documented there, so I've created a new commit at the start
of this patchset which addresses that. I also added a commit which
adds a lint test, but it uses a questionable heuristic in order to
avoid false positives (it's hard to distinguish graphs generated with
git log --oneline since they often have "\ [newline] [tab or spaces]
|"  ). Let me know if you think it looks promising. I'd be happy to
just drop it.

>
> [...]
> > --- a/t/lib-gpg.sh
> > +++ b/t/lib-gpg.sh
> > @@ -57,8 +57,8 @@ then
> >   echo | gpgsm --homedir "${GNUPGHOME}" 2>/dev/null \
> >   --passphrase-fd 0 --pinentry-mode loopback \
> >   --import "$TEST_DIRECTORY"/lib-gpg/gpgsm_cert.p12 &&
> > - gpgsm --homedir "${GNUPGHOME}" 2>/dev/null -K \
> > - | grep fingerprint: | cut -d" " -f4 | tr -d '\n' > \
> > + gpgsm --homedir "${GNUPGHOME}" 2>/dev/null -K |
> > + grep fingerprint: | cut -d" " -f4 | tr -d '\n' > \
> >   ${GNUPGHOME}/trustlist.txt &&
>
> I think this would be more readable with one item from the pipeline
> per line:
>
> gpgsm --homedir ... |
> grep ... |
> cut ... |
> tr ... >... &&
>
Done.

> [...]
> > --- a/t/t1006-cat-file.sh
> > +++ b/t/t1006-cat-file.sh
> > @@ -218,8 +218,8 @@ test_expect_success "--batch-check for a non-existent 
> > hash" '
> >  test "0042 missing
> >  0084 missing" = \
> >  "$( ( echo 0042;
> > - echo_without_newline 0084; ) \
> > -   | git cat-file --batch-check)"
> > + echo_without_newline 0084; ) |
> > +   git cat-file --batch-check)"
>
> This test is problematic in a lot of ways.  Most importantly, it ignores
> the exist status from git cat-file.
>
[...]
> but unless there's a linter that we're helping support, it's probably
> better to skip this file and use a dedicated patch to modernize its
> style more generally.

Yes, the cat-file.sh test is kind of funky, and I like the style of
your suggestions much better, but in this case I think that perfect is
the enemy of good. Fixing everything wrong with these lines would
necessitate fixing the surrounding couple of tests that also swallow
up the exit code of git cat-file. This may in turn necessitate other
fixes for consistency that may even spread to other files... I am
basing my argument here on what's in Documentation/CodingGuidelines,
which indicates that minor stylistic nits that result in code churn
are not recommended, and that we must be consistent with the
surrounding code. The surrounding code here looks for the most part
like:

 test "asdf" = $(echo "asdf" | git foo-bar)

Which I think is satisfactory in its own context. You asked me to fix
other test files later on, which I did, since they didn't seem to have
such a contrarian style, so the fixes were very localized, and I was
already editing many lines in those files already.

>
> [...]
> > --- a/t/t5317-pack-objects-filter-objects.sh
> > +++ b/t/t5317-pack-objects-filter-objects.sh
> > @@ -20,17 +20,20 @@ test_expect_success 'setup r1' '
> >  '
> >
> >  test_expect_success 'verify blob count in normal packfile' '
> > - git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 \
> > - | awk -f print_2.awk \
> > - | sort >expected &&
> > + git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 |
> > + awk -f print_2.awk |
> > + sort >expected &&
>
> This loses 

[PATCH v2 2/2] t5551: compare sorted cookies files

[Thomas Gummerer]

In t5551 we check that we save cookies correctly to a file when
http.cookiefile and http.savecookies are set.  To do so we create an
expect file that expects the cookies in a certain order.

However after e2ef8d6fa ("cookies: support creation-time attribute for
cookies", 2018-08-28) in curl.git (released in curl 7.61.1) that order
changed.

We document the file format as "Netscape/Mozilla cookie file
format (see curl(1))", so any format produced by libcurl should be
fine here.  Sort the files, to be agnostic to the order of the
cookies, and make the test pass with both curl versions > 7.61.1 and
earlier curl versions.

Reported-by: Todd Zullinger 
Helped-by: Jonathan Nieder 
Signed-off-by: Thomas Gummerer 
---
 t/t5551-http-fetch-smart.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 71535631d3..3dc8f8ecec 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -207,7 +207,7 @@ test_expect_success 'cookies stored in http.cookiefile when 
http.savecookies set
cat >cookies.txt <<-\EOF &&
127.0.0.1   FALSE   /smart_cookies/ FALSE   0   othername   
othervalue
EOF
-   cat >expect_cookies.txt <<-\EOF &&
+   sort >expect_cookies.txt <<-\EOF &&
 
127.0.0.1   FALSE   /smart_cookies/ FALSE   0   othername   
othervalue
127.0.0.1   FALSE   /smart_cookies/repo.git/info/   FALSE   0   
namevalue
@@ -215,7 +215,7 @@ test_expect_success 'cookies stored in http.cookiefile when 
http.savecookies set
git config http.cookiefile cookies.txt &&
git config http.savecookies true &&
git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&
-   tail -3 cookies.txt >cookies_tail.txt &&
+   tail -3 cookies.txt | sort >cookies_tail.txt &&
test_cmp expect_cookies.txt cookies_tail.txt
 '
 
-- 
2.19.0.444.g18242da7ef

[PATCH v2 0/2] t5551: compare sorted cookies files

[Thomas Gummerer]

Thanks Jonathan and Junio for the comments on the first round.

Changes since the first round:
- add a preparatory patch to modernize the test script
- add Reported-by to credit Todd
- just use 'sort' instead of 'cat | sort'

Thomas Gummerer (2):
  t5551: move setup code inside test_expect blocks
  t5551: compare sorted cookies files

 t/t5551-http-fetch-smart.sh | 68 ++---
 1 file changed, 34 insertions(+), 34 deletions(-)

-- 
2.19.0.444.g18242da7ef

[PATCH v2 1/2] t5551: move setup code inside test_expect blocks

[Thomas Gummerer]

Move setup code inside test_expect blocks, to catch unexpected
failures in the setup steps, and bring the test scripts in line with
our modern test style.

Suggested-by: Jonathan Nieder 
Signed-off-by: Thomas Gummerer 
---
 t/t5551-http-fetch-smart.sh | 66 ++---
 1 file changed, 33 insertions(+), 33 deletions(-)

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 771f36f9ff..71535631d3 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -23,26 +23,26 @@ test_expect_success 'create http-accessible bare 
repository' '
 
 setup_askpass_helper
 
-cat >exp < GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
-> Accept: */*
-> Accept-Encoding: ENCODINGS
-> Pragma: no-cache
-< HTTP/1.1 200 OK
-< Pragma: no-cache
-< Cache-Control: no-cache, max-age=0, must-revalidate
-< Content-Type: application/x-git-upload-pack-advertisement
-> POST /smart/repo.git/git-upload-pack HTTP/1.1
-> Accept-Encoding: ENCODINGS
-> Content-Type: application/x-git-upload-pack-request
-> Accept: application/x-git-upload-pack-result
-> Content-Length: xxx
-< HTTP/1.1 200 OK
-< Pragma: no-cache
-< Cache-Control: no-cache, max-age=0, must-revalidate
-< Content-Type: application/x-git-upload-pack-result
-EOF
 test_expect_success 'clone http repository' '
+   cat >exp <<-\EOF &&
+   > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
+   > Accept: */*
+   > Accept-Encoding: ENCODINGS
+   > Pragma: no-cache
+   < HTTP/1.1 200 OK
+   < Pragma: no-cache
+   < Cache-Control: no-cache, max-age=0, must-revalidate
+   < Content-Type: application/x-git-upload-pack-advertisement
+   > POST /smart/repo.git/git-upload-pack HTTP/1.1
+   > Accept-Encoding: ENCODINGS
+   > Content-Type: application/x-git-upload-pack-request
+   > Accept: application/x-git-upload-pack-result
+   > Content-Length: xxx
+   < HTTP/1.1 200 OK
+   < Pragma: no-cache
+   < Cache-Control: no-cache, max-age=0, must-revalidate
+   < Content-Type: application/x-git-upload-pack-result
+   EOF
GIT_TRACE_CURL=true git clone --quiet $HTTPD_URL/smart/repo.git clone 
2>err &&
test_cmp file clone/file &&
tr '\''\015'\'' Q exp cookies.txt expect_cookies.txt <<-\EOF &&
+
+   127.0.0.1   FALSE   /smart_cookies/ FALSE   0   othername   
othervalue
+   127.0.0.1   FALSE   /smart_cookies/repo.git/info/   FALSE   0   
namevalue
+   EOF
git config http.cookiefile cookies.txt &&
git config http.savecookies true &&
git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&
-- 
2.19.0.444.g18242da7ef

Re: [PATCH] t5551-http-fetch-smart.sh: sort cookies before comparing

[Junio C Hamano]

Jeff King  writes:

> Yes, I think sorting the expect file would work fine. I'm OK with that,
> or just leaving a comment. The comment has the bonus that it does not
> cost an extra process at runtime. I'd probably use a sort if we expected
> the list to be long and complicated, since it makes life easier on a
> future developer. But since there are only 2 lines, I don't think it's a
> big deal either way (or even just leaving it as-is without a comment is
> probably OK).

Let's have "| sort" if only for its documentation value.  That way
we do not have to remember the list must be sorted.

Here is what I'll merge to 'next'.

-- >8 --
From: Todd Zullinger 
Date: Fri, 7 Sep 2018 19:22:05 -0400
Subject: [PATCH] t5551-http-fetch-smart.sh: sort cookies before comparing
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

With curl-7.61.1 cookies are sorted by creation-time¹.  Sort the output
used in the 'cookies stored in http.cookiefile when http.savecookies
set' test before comparing it to the expected cookies.

¹ https://github.com/curl/curl/commit/e2ef8d6fa ("cookies: support
  creation-time attribute for cookies", 2018-08-28)

[jc: Also use a part of the patch by Thomas Gummerer that sorts the
expected output, which makes it easier to maintain.]

Signed-off-by: Todd Zullinger 
Helped-by: Thomas Gummerer 
Signed-off-by: Junio C Hamano 
---
 t/t5551-http-fetch-smart.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 771f36f9ff..d13b993201 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -206,7 +206,7 @@ test_expect_success 'dumb clone via http-backend respects 
namespace' '
 cat >cookies.txt cookies_tail.txt &&
+   tail -3 cookies.txt | sort >cookies_tail.txt &&
test_cmp expect_cookies.txt cookies_tail.txt
 '
 
-- 
2.19.0

Re: [PATCH 2/9] sha1-array: provide oid_array_filter

[Stefan Beller]

On Mon, Sep 17, 2018 at 2:36 PM Stefan Beller  wrote:
>
> Helped-by: Junio C Hamano 
> Signed-off-by: Stefan Beller 
> ---
>  sha1-array.c | 17 +
>  sha1-array.h |  9 +
>  2 files changed, 26 insertions(+)
>
> diff --git a/sha1-array.c b/sha1-array.c
> index 265941fbf40..67db5eeec9a 100644
> --- a/sha1-array.c
> +++ b/sha1-array.c
> @@ -77,3 +77,20 @@ int oid_array_for_each_unique(struct oid_array *array,
> }
> return 0;
>  }
> +
> +void oid_array_filter(struct oid_array *array,
> + for_each_oid_fn want,
> + void *cb_data)
> +{
> +   unsigned nr = array->nr, src, dst;
> +   struct object_id *oids = array->oids;

Blargh :-(

I made this last minute "pull oids out to be more like
the object_array_filter" and typo'd without compile checking.

Please discard this series.

Stefan

[PATCH 5/9] submodule: move global changed_submodule_names into fetch submodule struct

[Stefan Beller]

The `changed_submodule_names` are only used for fetching, so let's make it
part of the struct that is passed around for fetching submodules.

Signed-off-by: Stefan Beller 
---
 submodule.c | 42 +++---
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/submodule.c b/submodule.c
index c6eff7699f3..3520dd76bdf 100644
--- a/submodule.c
+++ b/submodule.c
@@ -24,7 +24,7 @@
 #include "object-store.h"
 
 static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
-static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;
+
 static int initialized_fetch_ref_tips;
 static struct oid_array ref_tips_before_fetch;
 static struct oid_array ref_tips_after_fetch;
@@ -1110,7 +1110,22 @@ void check_for_new_submodule_commits(struct object_id 
*oid)
oid_array_append(_tips_after_fetch, oid);
 }
 
-static void calculate_changed_submodule_paths(void)
+struct submodule_parallel_fetch {
+   int count;
+   struct argv_array args;
+   struct repository *r;
+   const char *prefix;
+   int command_line_option;
+   int default_option;
+   int quiet;
+   int result;
+
+   struct string_list changed_submodule_names;
+};
+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, 
STRING_LIST_INIT_DUP }
+
+static void calculate_changed_submodule_paths(
+   struct submodule_parallel_fetch *spf)
 {
struct argv_array argv = ARGV_ARRAY_INIT;
struct string_list changed_submodules = STRING_LIST_INIT_DUP;
@@ -1148,7 +1163,8 @@ static void calculate_changed_submodule_paths(void)
continue;
 
if (!submodule_has_commits(path, commits))
-   string_list_append(_submodule_names, 
name->string);
+   string_list_append(>changed_submodule_names,
+  name->string);
}
 
free_submodules_oids(_submodules);
@@ -1185,18 +1201,6 @@ int submodule_touches_in_range(struct object_id 
*excl_oid,
return ret;
 }
 
-struct submodule_parallel_fetch {
-   int count;
-   struct argv_array args;
-   struct repository *r;
-   const char *prefix;
-   int command_line_option;
-   int default_option;
-   int quiet;
-   int result;
-};
-#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0}
-
 static int get_fetch_recurse_config(const struct submodule *submodule,
struct submodule_parallel_fetch *spf)
 {
@@ -1257,7 +1261,7 @@ static int get_next_submodule(struct child_process *cp,
case RECURSE_SUBMODULES_ON_DEMAND:
if (!submodule ||
!string_list_lookup(
-   _submodule_names,
+   >changed_submodule_names,
submodule->name))
continue;
default_argv = "on-demand";
@@ -1349,8 +1353,8 @@ int fetch_populated_submodules(struct repository *r,
argv_array_push(, "--recurse-submodules-default");
/* default value, "--submodule-prefix" and its value are added later */
 
-   calculate_changed_submodule_paths();
-   string_list_sort(_submodule_names);
+   calculate_changed_submodule_paths();
+   string_list_sort(_submodule_names);
run_processes_parallel(max_parallel_jobs,
   get_next_submodule,
   fetch_start_failure,
@@ -1359,7 +1363,7 @@ int fetch_populated_submodules(struct repository *r,
 
argv_array_clear();
 out:
-   string_list_clear(_submodule_names, 1);
+   string_list_clear(_submodule_names, 1);
return spf.result;
 }
 
-- 
2.19.0.397.gdd90340f6a-goog

[PATCH 6/9] submodule.c: do not copy around submodule list

[Stefan Beller]

'calculate_changed_submodule_paths' uses a local list to compute the
changed submodules, and then produces the result by copying appropriate
items into the result list.

Instead use the result list directly and prune items afterwards
using string_list_remove_empty_items.

By doing so we'll have access to the util pointer for longer that
contains the commits that we need to fetch, which will be
useful in a later patch.

Signed-off-by: Stefan Beller 
---
 submodule.c | 19 ++-
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/submodule.c b/submodule.c
index 3520dd76bdf..00a9a3c6b12 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1128,8 +1128,7 @@ static void calculate_changed_submodule_paths(
struct submodule_parallel_fetch *spf)
 {
struct argv_array argv = ARGV_ARRAY_INIT;
-   struct string_list changed_submodules = STRING_LIST_INIT_DUP;
-   const struct string_list_item *name;
+   struct string_list_item *name;
 
/* No need to check if there are no submodules configured */
if (!submodule_from_path(the_repository, NULL, NULL))
@@ -1146,9 +1145,9 @@ static void calculate_changed_submodule_paths(
 * Collect all submodules (whether checked out or not) for which new
 * commits have been recorded upstream in "changed_submodule_names".
 */
-   collect_changed_submodules(_submodules, );
+   collect_changed_submodules(>changed_submodule_names, );
 
-   for_each_string_list_item(name, _submodules) {
+   for_each_string_list_item(name, >changed_submodule_names) {
struct oid_array *commits = name->util;
const struct submodule *submodule;
const char *path = NULL;
@@ -1162,12 +1161,14 @@ static void calculate_changed_submodule_paths(
if (!path)
continue;
 
-   if (!submodule_has_commits(path, commits))
-   string_list_append(>changed_submodule_names,
-  name->string);
+   if (submodule_has_commits(path, commits)) {
+   oid_array_clear(commits);
+   *name->string = '\0';
+   }
}
 
-   free_submodules_oids(_submodules);
+   string_list_remove_empty_items(>changed_submodule_names, 1);
+
argv_array_clear();
oid_array_clear(_tips_before_fetch);
oid_array_clear(_tips_after_fetch);
@@ -1363,7 +1364,7 @@ int fetch_populated_submodules(struct repository *r,
 
argv_array_clear();
 out:
-   string_list_clear(_submodule_names, 1);
+   free_submodules_oids(_submodule_names);
return spf.result;
 }
 
-- 
2.19.0.397.gdd90340f6a-goog

[PATCH 8/9] fetch: retry fetching submodules if needed objects were not fetched

[Stefan Beller]

Currently when git-fetch is asked to recurse into submodules, it dispatches
a plain "git-fetch -C " (with some submodule related options
such as prefix and recusing strategy, but) without any information of the
remote or the tip that should be fetched.

This works surprisingly well in some workflows (such as using submodules
as a third party library), while not so well in other scenarios, such
as in a Gerrit topic-based workflow, that can tie together changes
(potentially across repositories) on the server side. One of the parts
of such a Gerrit workflow is to download a change when wanting to examine
it, and you'd want to have its submodule changes that are in the same
topic downloaded as well. However these submodule changes reside in their
own repository in their own ref (refs/changes/).

Retry fetching a submodule by object name if the object id that the
superproject points to, cannot be found.

This retrying does not happen when the "git fetch" done at the
superproject is not storing the fetched results in remote
tracking branches (i.e. instead just recording them to
FETCH_HEAD) in this step. A later patch will fix this.

builtin/fetch used to only inspect submodules when they were fetched
"on-demand", as in either on/off case it was clear whether the submodule
needs to be fetched. However to know whether we need to try fetching the
object ids, we need to identify the object names, which is done in this
function check_for_new_submodule_commits(), so we'll also run that code
in case the submodule recursion is set to "on".

Signed-off-by: Stefan Beller 
---
 builtin/fetch.c |  9 ++--
 submodule.c | 87 -
 t/t5526-fetch-submodules.sh | 16 +++
 3 files changed, 104 insertions(+), 8 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 61bec5d213d..95c44bf6ffa 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -700,8 +700,7 @@ static int update_local_ref(struct ref *ref,
what = _("[new ref]");
}
 
-   if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
-   (recurse_submodules != RECURSE_SUBMODULES_ON))
+   if (recurse_submodules != RECURSE_SUBMODULES_OFF)
check_for_new_submodule_commits(>new_oid);
r = s_update_ref(msg, ref, 0);
format_display(display, r ? '!' : '*', what,
@@ -716,8 +715,7 @@ static int update_local_ref(struct ref *ref,
strbuf_add_unique_abbrev(, >object.oid, 
DEFAULT_ABBREV);
strbuf_addstr(, "..");
strbuf_add_unique_abbrev(, >new_oid, 
DEFAULT_ABBREV);
-   if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
-   (recurse_submodules != RECURSE_SUBMODULES_ON))
+   if (recurse_submodules != RECURSE_SUBMODULES_OFF)
check_for_new_submodule_commits(>new_oid);
r = s_update_ref("fast-forward", ref, 1);
format_display(display, r ? '!' : ' ', quickref.buf,
@@ -731,8 +729,7 @@ static int update_local_ref(struct ref *ref,
strbuf_add_unique_abbrev(, >object.oid, 
DEFAULT_ABBREV);
strbuf_addstr(, "...");
strbuf_add_unique_abbrev(, >new_oid, 
DEFAULT_ABBREV);
-   if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
-   (recurse_submodules != RECURSE_SUBMODULES_ON))
+   if (recurse_submodules != RECURSE_SUBMODULES_OFF)
check_for_new_submodule_commits(>new_oid);
r = s_update_ref("forced-update", ref, 1);
format_display(display, r ? '!' : '+', quickref.buf,
diff --git a/submodule.c b/submodule.c
index 88bce534268..7d59e56171f 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1127,8 +1127,11 @@ struct submodule_parallel_fetch {
int result;
 
struct string_list changed_submodule_names;
+   struct string_list retry;
 };
-#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, 
STRING_LIST_INIT_DUP }
+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \
+ STRING_LIST_INIT_DUP, \
+ STRING_LIST_INIT_NODUP}
 
 static void calculate_changed_submodule_paths(
struct submodule_parallel_fetch *spf)
@@ -1259,8 +1262,10 @@ static int get_next_submodule(struct child_process *cp,
 {
int ret = 0;
struct submodule_parallel_fetch *spf = data;
+   struct string_list_item *it;
 
for (; spf->count < spf->r->index->cache_nr; spf->count++) {
+   int recurse_config;
struct strbuf submodule_prefix = STRBUF_INIT;
const struct cache_entry *ce = spf->r->index->cache[spf->count];
const char *git_dir, *default_argv;
@@ -1280,7 +1285,9 @@ static int get_next_submodule(struct child_process *cp,
}
}
 
-   switch 

[PATCH 7/9] submodule: fetch in submodules git directory instead of in worktree

[Stefan Beller]

This patch started as a refactoring to make 'get_next_submodule' more
readable, but upon doing so, I realized that "git fetch" of the submodule
actually doesn't need to be run in the submodules worktree. So let's run
it in its git dir instead.

That should pave the way towards fetching submodules that are currently
not checked out.

Signed-off-by: Stefan Beller 
---
 submodule.c | 45 +++--
 t/t5526-fetch-submodules.sh |  7 +-
 2 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/submodule.c b/submodule.c
index 00a9a3c6b12..88bce534268 100644
--- a/submodule.c
+++ b/submodule.c
@@ -481,6 +481,12 @@ void prepare_submodule_repo_env(struct argv_array *out)
 DEFAULT_GIT_DIR_ENVIRONMENT);
 }
 
+static void prepare_submodule_repo_env_in_gitdir(struct argv_array *out)
+{
+   prepare_submodule_repo_env_no_git_dir(out);
+   argv_array_pushf(out, "%s=.", GIT_DIR_ENVIRONMENT);
+}
+
 /* Helper function to display the submodule header line prior to the full
  * summary output. If it can locate the submodule objects directory it will
  * attempt to lookup both the left and right commits and put them into the
@@ -1227,6 +1233,27 @@ static int get_fetch_recurse_config(const struct 
submodule *submodule,
return spf->default_option;
 }
 
+static char *get_submodule_git_dir(struct repository *r, const char *path)
+{
+   struct repository subrepo;
+   const char *ret;
+
+   if (repo_submodule_init(, r, path)) {
+   /* no entry in .gitmodules? */
+   struct strbuf gitdir = STRBUF_INIT;
+   strbuf_repo_worktree_path(, r, "%s/.git", path);
+   if (repo_init(, gitdir.buf, NULL)) {
+   strbuf_release();
+   return NULL;
+   }
+   }
+
+   ret = xstrdup(subrepo.gitdir);
+   repo_clear();
+
+   return ret;
+}
+
 static int get_next_submodule(struct child_process *cp,
  struct strbuf *err, void *data, void **task_cb)
 {
@@ -1234,8 +1261,6 @@ static int get_next_submodule(struct child_process *cp,
struct submodule_parallel_fetch *spf = data;
 
for (; spf->count < spf->r->index->cache_nr; spf->count++) {
-   struct strbuf submodule_path = STRBUF_INIT;
-   struct strbuf submodule_git_dir = STRBUF_INIT;
struct strbuf submodule_prefix = STRBUF_INIT;
const struct cache_entry *ce = spf->r->index->cache[spf->count];
const char *git_dir, *default_argv;
@@ -1274,16 +1299,12 @@ static int get_next_submodule(struct child_process *cp,
continue;
}
 
-   strbuf_repo_worktree_path(_path, spf->r, "%s", 
ce->name);
-   strbuf_addf(_git_dir, "%s/.git", submodule_path.buf);
strbuf_addf(_prefix, "%s%s/", spf->prefix, ce->name);
-   git_dir = read_gitfile(submodule_git_dir.buf);
-   if (!git_dir)
-   git_dir = submodule_git_dir.buf;
-   if (is_directory(git_dir)) {
+   git_dir = get_submodule_git_dir(spf->r, ce->name);
+   if (git_dir) {
child_process_init(cp);
-   cp->dir = strbuf_detach(_path, NULL);
-   prepare_submodule_repo_env(>env_array);
+   prepare_submodule_repo_env_in_gitdir(>env_array);
+   cp->dir = git_dir;
cp->git_cmd = 1;
if (!spf->quiet)
strbuf_addf(err, "Fetching submodule %s%s\n",
@@ -1293,10 +1314,10 @@ static int get_next_submodule(struct child_process *cp,
argv_array_push(>args, default_argv);
argv_array_push(>args, "--submodule-prefix");
argv_array_push(>args, submodule_prefix.buf);
+
+   free(git_dir);
ret = 1;
}
-   strbuf_release(_path);
-   strbuf_release(_git_dir);
strbuf_release(_prefix);
if (ret) {
spf->count++;
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 6c2f9b2ba26..42692219a1a 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -566,7 +566,12 @@ test_expect_success 'fetching submodule into a broken 
repository' '
 
test_must_fail git -C dst status &&
test_must_fail git -C dst diff &&
-   test_must_fail git -C dst fetch --recurse-submodules
+
+   # git-fetch cannot find the git directory of the submodule,
+   # so it will do nothing, successfully, as it cannot distinguish between
+   # this broken submodule and a submodule that was just set active but
+   # not cloned yet
+   git -C dst fetch --recurse-submodules
 '
 
 test_expect_success 

[PATCH 9/9] builtin/fetch: check for submodule updates for non branch fetches

[Stefan Beller]

Gerrit, the code review tool, has a different workflow than our mailing
list based approach. Usually users upload changes to a Gerrit server and
continuous integration and testing happens by bots. Sometimes however a
user wants to checkout a change locally and look at it locally. For this
use case, Gerrit offers a command line snippet to copy and paste to your
terminal, which looks like

  git fetch https:///gerrit refs/changes/ &&
  git checkout FETCH_HEAD

For Gerrit changes that contain changing submodule gitlinks, it would be
easy to extend both the fetch and checkout with the '--recurse-submodules'
flag, such that this command line snippet would produce the state of a
change locally.

However the functionality added in the previous patch, which would
ensure that we fetch the objects in the submodule that the gitlink pointed
at, only works for remote tracking branches so far, not for FETCH_HEAD.

Make sure that fetching a superproject to its FETCH_HEAD, also respects
the existence checks for objects in the submodule recursion.

Signed-off-by: Stefan Beller 
---
 builtin/fetch.c | 5 -
 t/t5526-fetch-submodules.sh | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 95c44bf6ffa..ea6ecd123e7 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -887,11 +887,14 @@ static int store_updated_refs(const char *raw_url, const 
char *remote_name,
rc |= update_local_ref(ref, what, rm, ,
   summary_width);
free(ref);
-   } else
+   } else {
+   check_for_new_submodule_commits(>old_oid);
format_display(, '*',
   *kind ? kind : "branch", NULL,
   *what ? what : "HEAD",
   "FETCH_HEAD", summary_width);
+   }
+
if (note.len) {
if (verbosity >= 0 && !shown_url) {
fprintf(stderr, _("From %.*s\n"),
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index af12c50e7dd..a509eabb044 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -615,7 +615,7 @@ test_expect_success "fetch new commits on-demand when they 
are not reachable" '
git update-ref refs/changes/2 $D &&
(
cd downstream &&
-   git fetch --recurse-submodules --recurse-submodules-default 
on-demand origin refs/changes/2:refs/heads/my_branch &&
+   git fetch --recurse-submodules origin refs/changes/2 &&
git -C submodule cat-file -t $C &&
git checkout --recurse-submodules FETCH_HEAD
)
-- 
2.19.0.397.gdd90340f6a-goog

[PATCH 2/9] sha1-array: provide oid_array_filter

[Stefan Beller]

Helped-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
---
 sha1-array.c | 17 +
 sha1-array.h |  9 +
 2 files changed, 26 insertions(+)

diff --git a/sha1-array.c b/sha1-array.c
index 265941fbf40..67db5eeec9a 100644
--- a/sha1-array.c
+++ b/sha1-array.c
@@ -77,3 +77,20 @@ int oid_array_for_each_unique(struct oid_array *array,
}
return 0;
 }
+
+void oid_array_filter(struct oid_array *array,
+ for_each_oid_fn want,
+ void *cb_data)
+{
+   unsigned nr = array->nr, src, dst;
+   struct object_id *oids = array->oids;
+
+   for (src = dst = 0; src < nr; src++) {
+   if (want([src], cb_data)) {
+   if (src != dst)
+   oidcpy(oids[dst], [src]);
+   dst++;
+   }
+   }
+   array->nr = dst;
+}
diff --git a/sha1-array.h b/sha1-array.h
index 232bf950172..ae059ca0431 100644
--- a/sha1-array.h
+++ b/sha1-array.h
@@ -23,4 +23,13 @@ int oid_array_for_each_unique(struct oid_array *array,
  for_each_oid_fn fn,
  void *data);
 
+/*
+ * Apply want to each entry in array, retaining only the entries for
+ * which the function returns true.  Preserve the order of the entries
+ * that are retained.
+ */
+int oid_array_filter(struct oid_array *array,
+for_each_oid_fn want,
+void *cbdata);
+
 #endif /* SHA1_ARRAY_H */
-- 
2.19.0.397.gdd90340f6a-goog

[PATCHv2 0/9] fetch: make sure submodule oids are fetched

[Stefan Beller]

v2:
* extended commit messages,
* plugged a memory leak
* rewrote the patch "sha1-array: provide oid_array_filter" to be much more like 
  object_array_fiter
* fixed a typo pointed out by Ramsay.

The range diff is below.
  
Thanks,
Stefan

v1:
Currently when git-fetch is asked to recurse into submodules, it dispatches
a plain "git-fetch -C " (and some submodule related options
such as prefix and recusing strategy, but) without any information of the
remote or the tip that should be fetched.

This works surprisingly well in some workflows, not so well in others,
which this series aims to fix.

The first patches provide new basic functionality and do some refactoring;
the interesting part is in the two last patches.

This was discussed in
https://public-inbox.org/git/20180808221752.195419-1-sbel...@google.com/
and I think I addressed all feedback so far.

Stefan Beller (9):
  string-list: add string_list_{pop, last} functions
  sha1-array: provide oid_array_filter
  submodule.c: fix indentation
  submodule.c: sort changed_submodule_names before searching it
  submodule: move global changed_submodule_names into fetch submodule
struct
  submodule.c: do not copy around submodule list
  submodule: fetch in submodules git directory instead of in worktree
  fetch: retry fetching submodules if needed objects were not fetched
  builtin/fetch: check for submodule updates for non branch fetches

 builtin/fetch.c |  14 +--
 sha1-array.c|  17 
 sha1-array.h|   9 ++
 string-list.c   |  14 +++
 string-list.h   |  15 +++
 submodule.c | 191 
 t/t5526-fetch-submodules.sh |  23 -
 7 files changed, 236 insertions(+), 47 deletions(-)

git range-diff origin/sb/submodule-recursive-fetch-gets-the-tip...HEAD 

 1:  6fecf7cd01a !  1:  54d31d90734 string-list: add string_list_{pop, last} 
functions
@@ -12,8 +12,8 @@
  - string_list_pop() removes the string_list_item at the end of
the string list.
 
- - string_list_push() is not added, but string_list_append() can be
-   used in its place.
+ - there is no string_list_push(); string_list_append() can be used
+   in its place.
 
 You can use them in this pattern:
 
@@ -26,7 +26,6 @@
 
 Helped-by: Junio C Hamano 
 Signed-off-by: Stefan Beller 
-Signed-off-by: Junio C Hamano 
 
  diff --git a/string-list.c b/string-list.c
  --- a/string-list.c
@@ -66,6 +65,10 @@
 + */
 +void string_list_pop(struct string_list *list, int free_util);
 +
++/*
++ * Returns the last item of the list. As it returns the raw access, do not
++ * modify the list while holding onto the returned pointer.
++ */
 +static inline struct string_list_item *string_list_last(struct 
string_list *list)
 +{
 +  return >items[list->nr - 1];
 2:  7007a318a68 <  -:  --- sha1-array: provide oid_array_filter
 -:  --- >  2:  a2bd6ef2bf0 sha1-array: provide oid_array_filter
 3:  807429234ac !  3:  0300c27cbd7 submodule.c: fix indentation
@@ -6,7 +6,6 @@
 Fix it while we are here.
 
 Signed-off-by: Stefan Beller 
-Signed-off-by: Junio C Hamano 
 
  diff --git a/submodule.c b/submodule.c
  --- a/submodule.c
 4:  f6fa5273af9 !  4:  80cf0221bbe submodule.c: sort changed_submodule_names 
before searching it
@@ -12,7 +12,6 @@
 appropriate.
 
 Signed-off-by: Stefan Beller 
-Signed-off-by: Junio C Hamano 
 
  diff --git a/submodule.c b/submodule.c
  --- a/submodule.c
 5:  adf7a2fd203 !  5:  7ddb448b748 submodule: move global 
changed_submodule_names into fetch submodule struct
@@ -6,7 +6,6 @@
 part of the struct that is passed around for fetching submodules.
 
 Signed-off-by: Stefan Beller 
-Signed-off-by: Junio C Hamano 
 
  diff --git a/submodule.c b/submodule.c
  --- a/submodule.c
 6:  56c9398589a !  6:  7975a7f1e3b submodule.c: do not copy around submodule 
list
@@ -9,12 +9,11 @@
 Instead use the result list directly and prune items afterwards
 using string_list_remove_empty_items.
 
-By doin so we'll have access to the util pointer for longer that
+By doing so we'll have access to the util pointer for longer that
 contains the commits that we need to fetch, which will be
 useful in a later patch.
 
 Signed-off-by: Stefan Beller 
-Signed-off-by: Junio C Hamano 
 
  diff --git a/submodule.c b/submodule.c
  --- a/submodule.c
 7:  9f70a5f32c9 !  7:  29bc2868f26 submodule: fetch in submodules git 
directory instead of in worktree
@@ -3,14 +3,14 @@
 submodule: fetch in submodules git directory instead of in worktree
 
 This patch started as a refactoring to make 'get_next_submodule' more
-   

[PATCH 3/9] submodule.c: fix indentation

[Stefan Beller]

The submodule subsystem is really bad at staying within 80 characters.
Fix it while we are here.

Signed-off-by: Stefan Beller 
---
 submodule.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/submodule.c b/submodule.c
index a2b266fbfae..d29dfa3d1f5 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1244,7 +1244,8 @@ static int get_next_submodule(struct child_process *cp,
if (!submodule) {
const char *name = default_name_or_path(ce->name);
if (name) {
-   default_submodule.path = default_submodule.name 
= name;
+   default_submodule.path = name;
+   default_submodule.name = name;
submodule = _submodule;
}
}
@@ -1254,8 +1255,10 @@ static int get_next_submodule(struct child_process *cp,
default:
case RECURSE_SUBMODULES_DEFAULT:
case RECURSE_SUBMODULES_ON_DEMAND:
-   if (!submodule || 
!unsorted_string_list_lookup(_submodule_names,
-submodule->name))
+   if (!submodule ||
+   !unsorted_string_list_lookup(
+   _submodule_names,
+   submodule->name))
continue;
default_argv = "on-demand";
break;
-- 
2.19.0.397.gdd90340f6a-goog

[PATCH 4/9] submodule.c: sort changed_submodule_names before searching it

[Stefan Beller]

We can string_list_insert() to maintain sorted-ness of the
list as we find new items, or we can string_list_append() to
build an unsorted list and sort it at the end just once.

To pick which one is more appropriate, we notice the fact
that we discover new items more or less in the already
sorted order.  That makes "append then sort" more
appropriate.

Signed-off-by: Stefan Beller 
---
 submodule.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index d29dfa3d1f5..c6eff7699f3 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1256,7 +1256,7 @@ static int get_next_submodule(struct child_process *cp,
case RECURSE_SUBMODULES_DEFAULT:
case RECURSE_SUBMODULES_ON_DEMAND:
if (!submodule ||
-   !unsorted_string_list_lookup(
+   !string_list_lookup(
_submodule_names,
submodule->name))
continue;
@@ -1350,6 +1350,7 @@ int fetch_populated_submodules(struct repository *r,
/* default value, "--submodule-prefix" and its value are added later */
 
calculate_changed_submodule_paths();
+   string_list_sort(_submodule_names);
run_processes_parallel(max_parallel_jobs,
   get_next_submodule,
   fetch_start_failure,
-- 
2.19.0.397.gdd90340f6a-goog

[PATCH 1/9] string-list: add string_list_{pop, last} functions

[Stefan Beller]

Add a few functions to allow a string-list to be used as a stack:

 - string_list_last() lets a caller peek the string_list_item at the
   end of the string list.  The caller needs to be aware that it is
   borrowing a pointer, which can become invalid if/when the
   string_list is resized.

 - string_list_pop() removes the string_list_item at the end of
   the string list.

 - there is no string_list_push(); string_list_append() can be used
   in its place.

You can use them in this pattern:

while (list.nr) {
struct string_list_item *item = string_list_last();

work_on(item);
string_list_pop(, free_util);
}

Helped-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
---
 string-list.c | 14 ++
 string-list.h | 15 +++
 2 files changed, 29 insertions(+)

diff --git a/string-list.c b/string-list.c
index 771c4550980..04db2b537c0 100644
--- a/string-list.c
+++ b/string-list.c
@@ -80,6 +80,20 @@ void string_list_remove(struct string_list *list, const char 
*string,
}
 }
 
+void string_list_pop(struct string_list *list, int free_util)
+{
+   if (list->nr == 0)
+   BUG("tried to remove an item from empty string list");
+
+   if (list->strdup_strings)
+   free(list->items[list->nr - 1].string);
+
+   if (free_util)
+   free(list->items[list->nr - 1].util);
+
+   list->nr--;
+}
+
 int string_list_has_string(const struct string_list *list, const char *string)
 {
int exact_match;
diff --git a/string-list.h b/string-list.h
index ff8f6094a33..15f2936e337 100644
--- a/string-list.h
+++ b/string-list.h
@@ -191,6 +191,21 @@ extern void string_list_remove(struct string_list *list, 
const char *string,
  */
 struct string_list_item *string_list_lookup(struct string_list *list, const 
char *string);
 
+/**
+ * Removes the last item from the list.
+ * The caller must ensure that the list is not empty.
+ */
+void string_list_pop(struct string_list *list, int free_util);
+
+/*
+ * Returns the last item of the list. As it returns the raw access, do not
+ * modify the list while holding onto the returned pointer.
+ */
+static inline struct string_list_item *string_list_last(struct string_list 
*list)
+{
+   return >items[list->nr - 1];
+}
+
 /*
  * Remove all but the first of consecutive entries with the same
  * string value.  If free_util is true, call free() on the util
-- 
2.19.0.397.gdd90340f6a-goog

Re: [PATCH v5 2/5] read-cache: load cache extensions on a worker thread

[Junio C Hamano]

Duy Nguyen  writes:

>> diff --git a/read-cache.c b/read-cache.c
>> index 858935f123..b203eebb44 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -23,6 +23,10 @@
>>  #include "split-index.h"
>>  #include "utf8.h"
>>  #include "fsmonitor.h"
>> +#ifndef NO_PTHREADS
>> +#include 
>> +#include 
>> +#endif
>
> I don't think you're supposed to include system header files after
> "cache.h". Including thread-utils.h should be enough (and it keeps the
> exception of inclduing pthread.h in just one place). Please use
> "pthread-utils.h" instead of  which is usually for
> system header files. And include ptherad-utils.h unconditionally.

All correct except for s/p\(thread-utils\)/\1/g;
Sorry for missing this during my earlier review.

Thanks.

Re: [PATCH] t5551: compare sorted cookies files

[Jonathan Nieder]

Thomas Gummerer wrote:
> On 09/17, Junio C Hamano wrote:

>> The other
>> effort implicitly depends on the expected output is kept sorted, but
>> this one is more explicit---I tend to prefer this approach as tools
>> and automation is easier to maintain than having to remember that
>> the source must be sorted.
>
> I'm happy going with either patch, but if we want to go with mine, I'd
> like to make sure Todd is credited appropriately, as he sent a very
> similar patch first.  Not sure what the appropriate way here is
> though?

Thanks for asking.  Credit is a subject that is dear to my heart.

You can for example use
Reported-by: Todd Zullinger 

to credit him for the patch and analysis that appears to have helped
with reviews (and to signal that this fixes the bug he reported).

[...]
>>> --- a/t/t5551-http-fetch-smart.sh
>>> +++ b/t/t5551-http-fetch-smart.sh
>>> @@ -206,7 +206,7 @@ test_expect_success 'dumb clone via http-backend 
>>> respects namespace' '
>>>  cat >cookies.txt <>>  127.0.0.1  FALSE   /smart_cookies/ FALSE   0   othername   
>>> othervalue
>>>  EOF
>>> -cat >expect_cookies.txt <>> +cat expect_cookies.txt <<\EOF

?  That is simpler since it avoids a pipe and means the reader doesn't
have to look out for shell metacharacters like $ inside the text.

Bonus points if this kind of setup moves to inside the test (using
<<-\EOF), which can make the test script easier to read.

Thanks and hope that helps,
Jonathan

Re: [PATCH] t5551: compare sorted cookies files

[Thomas Gummerer]

On 09/17, Junio C Hamano wrote:
> Thomas Gummerer  writes:
> 
> > In t5551 we check that we save cookies correctly to a file when
> > http.cookiefile and http.savecookies are set.  To do so we create an
> > expect file that expects the cookies in a certain order.
> >
> > However after e2ef8d6fa ("cookies: support creation-time attribute for
> > cookies", 2018-08-28) in curl.git (released in curl 7.61.1) that order
> > changed.
> >
> > We document the file format as "Netscape/Mozilla cookie file
> > format (see curl(1))", so any format produced by libcurl should be
> > fine here.  Sort the files, to be agnostic to the order of the
> > cookies, and make the test pass with both curl versions > 7.61.1 and
> > earlier curl versions.
> >
> > Signed-off-by: Thomas Gummerer 
> > ---
> 
> Thanks.  f5b2c9c9 ("t5551-http-fetch-smart.sh: sort cookies before
> comparing", 2018-09-07) that came from
> 
> https://public-inbox.org/git/20180907232205.31328-1-...@pobox.com
> 
> has almost the identical patch text, and this (presumably an
> independent effort) confirms that the patch is needed.

Whoops awkward, I should have checked 'pu' before starting to work on
this.  This was an independent effort, but I really should
have checked 'pu' before starting on this.

> The other
> effort implicitly depends on the expected output is kept sorted, but
> this one is more explicit---I tend to prefer this approach as tools
> and automation is easier to maintain than having to remember that
> the source must be sorted.

I'm happy going with either patch, but if we want to go with mine, I'd
like to make sure Todd is credited appropriately, as he sent a very
similar patch first.  Not sure what the appropriate way here is
though?

> Thanks.
> 
> >  t/t5551-http-fetch-smart.sh | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> > index 771f36f9ff..d13b993201 100755
> > --- a/t/t5551-http-fetch-smart.sh
> > +++ b/t/t5551-http-fetch-smart.sh
> > @@ -206,7 +206,7 @@ test_expect_success 'dumb clone via http-backend 
> > respects namespace' '
> >  cat >cookies.txt < >  127.0.0.1  FALSE   /smart_cookies/ FALSE   0   othername   
> > othervalue
> >  EOF
> > -cat >expect_cookies.txt < > +cat  >  
> >  127.0.0.1  FALSE   /smart_cookies/ FALSE   0   othername   
> > othervalue
> >  127.0.0.1  FALSE   /smart_cookies/repo.git/info/   FALSE   0   name
> > value
> > @@ -215,7 +215,7 @@ test_expect_success 'cookies stored in http.cookiefile 
> > when http.savecookies set
> > git config http.cookiefile cookies.txt &&
> > git config http.savecookies true &&
> > git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&
> > -   tail -3 cookies.txt >cookies_tail.txt &&
> > +   tail -3 cookies.txt | sort >cookies_tail.txt &&
> > test_cmp expect_cookies.txt cookies_tail.txt
> >  '

Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"

[Eric Sunshine]

On Mon, Sep 17, 2018 at 4:43 PM Eric Sunshine  wrote:
> I did consider this case and felt that it would be reasonable for it
> to error out and ignore the error if git was missing or if the
> directory was not a repository. And, I _thought_ I had prefixed the
> line with "-" to handle just such a case, but apparently I botched it.
> Oh well.

Dredging up from memory, I think the omission of "-" from the Makefile
line was intentional since I specifically handled the case of missing
"git" command in the script itself by ignoring any error from it.
Specifically, this excerpt from doc-diff:

if test -n "$clean"
then
test $# -eq 0 || usage
git worktree remove --force "$tmp/worktree" 2>/dev/null
rm -rf "$tmp"
exit 0
fi

in which a problem invoking git is explicitly ignored and the script
exits cleanly, so no Makefile "-" is needed.

Unfortunately, I forgot about the:

. "$(git --exec-path)/git-sh-setup"

which happens earlier in the script.

Re: [PATCH] midx.c: mark a file-local symbol as static

[Junio C Hamano]

Derrick Stolee  writes:

> Thanks for catching this, Ramsay. Sorry for the mistake.
>
> ds/multi-pack-verify is currently queued for 'next', so I wasn't
> planning on sending another version.

Marked for 'next' does not mean we cannot touch it.  It merely means
"so far no issue that requires a reroll has been raised afaik", and
the whole point of marking it is to give people a chance to stop it
before getting merged to 'next' with known issues.

As the problematic commit is not in 'next', let's squash the fix in.

Thanks, both.

Re: [PATCH] midx.c: mark a file-local symbol as static

[Junio C Hamano]

Derrick Stolee  writes:

> ds/multi-pack-verify is currently queued for 'next', so I wasn't
> planning on sending another version.
>
> Junio, could you add this commit to the tip, or squash it into
> 64cbf3df2 "multi-pack-index: add 'verify' verb"?

I think it makes sense to queue this on top.  Thanks.

Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"

[Eric Sunshine]

On Mon, Sep 17, 2018 at 3:36 PM Junio C Hamano  wrote:
> Jonathan Nieder  writes:
> >> +'$(SHELL_PATH_SQ)' ./doc-diff --clean
> >
> > This means I need a copy of git in order to run "make clean".  That
> > was never required before.  It makes bootstrapping difficult --- do we
> > really need it?
>
> Gahh, you are absolutely right.  Also "doc-diff --clean", if I am
> reading the code correctly, requires us to be in a Git repository,
> not a tarball extract.
>
> Having to have Git installed, or be in a repository, in order to be
> able to run an optional "doc-diff" tool is fine.  Requiring either
> in order to run "make clean" is a different story.
>
> Thanks for spotting.  We can just prefix the line with '-'?  Or does
> the script badly misbehave (due to lack of CEILING_DIRECTORY) when
> run in a tarball extract inside somebody else's repository?

I did consider this case and felt that it would be reasonable for it
to error out and ignore the error if git was missing or if the
directory was not a repository. And, I _thought_ I had prefixed the
line with "-" to handle just such a case, but apparently I botched it.
Oh well.

git pull --rebase=preserve is always rebasing something, even on up-to-date branch

[Mikhail Matrosov]

Please try the following:

mmatrosov@Mikhail-PC:~/test$ git init --bare server
Initialized empty Git repository in /home/mmatrosov/test/server/
mmatrosov@Mikhail-PC:~/test$ git clone server local
Cloning into 'local'...
warning: You appear to have cloned an empty repository.
done.
mmatrosov@Mikhail-PC:~/test$ cd local
mmatrosov@Mikhail-PC:~/test/local$ echo a > a && git add . && git commit -m A
[master (root-commit) a34c21f] A
 1 file changed, 1 insertion(+)
 create mode 100644 a
mmatrosov@Mikhail-PC:~/test/local$ git push
Counting objects: 3, done.
Writing objects: 100% (3/3), 205 bytes | 0 bytes/s, done.
Total 3 (delta 0), reused 0 (delta 0)
To /home/mmatrosov/test/server
 * [new branch]  master -> master
mmatrosov@Mikhail-PC:~/test/local$ git pull
Already up-to-date.
mmatrosov@Mikhail-PC:~/test/local$ git pull --rebase=preserve
Rebasing (1/1)
Successfully rebased and updated refs/heads/master.

As you can see, running bare "git pull" just tells me everything is up
to date. However, running "git pull --rebase=preserve" triggers
rebasing of something. It wont be a problem if it didn't take
significant time (especially on Windows). Why this rebase happens? It
is completely redundant and slows down the pull operation. Looks like
a bug to me.

Note that it is important to me, because I want to set "git config
--global pull.rebase preserve". But because of this issue, pulling on
an up-to-date repository takes a lot of time. Which is very
frustrating.

Tested with:
* git version 2.19.0.windows.1 in Windows 10 Version 1803
* git version 2.7.4 in Ubuntu 16.04.3 LTS (inside WSL)

-
Best regards, Mikhail Matrosov

Re: [PATCH] midx.c: mark a file-local symbol as static

[Derrick Stolee]

On 9/14/2018 7:26 PM, Ramsay Jones wrote:

Signed-off-by: Ramsay Jones 
---

Hi Derrick,

If you need to re-roll your 'ds/multi-pack-verify' branch, could you
please squash this into the relevant patch (commit 64cbf3df21,
"multi-pack-index: add 'verify' verb", 2018-09-13).

[noticed by sparse].

Thanks.

ATB,
Ramsay Jones


Thanks for catching this, Ramsay. Sorry for the mistake.

ds/multi-pack-verify is currently queued for 'next', so I wasn't 
planning on sending another version.


Junio, could you add this commit to the tip, or squash it into 64cbf3df2 
"multi-pack-index: add 'verify' verb"?


Thanks,
-Stolee



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

diff --git a/midx.c b/midx.c
index 4d4c930522..713d6f9dde 100644
--- a/midx.c
+++ b/midx.c
@@ -926,7 +926,7 @@ void clear_midx_file(const char *object_dir)
free(midx);
  }
  
-int verify_midx_error;

+static int verify_midx_error;
  
  static void midx_report(const char *fmt, ...)

  {

Re: [PATCH] t5551: compare sorted cookies files

[Junio C Hamano]

Thomas Gummerer  writes:

> In t5551 we check that we save cookies correctly to a file when
> http.cookiefile and http.savecookies are set.  To do so we create an
> expect file that expects the cookies in a certain order.
>
> However after e2ef8d6fa ("cookies: support creation-time attribute for
> cookies", 2018-08-28) in curl.git (released in curl 7.61.1) that order
> changed.
>
> We document the file format as "Netscape/Mozilla cookie file
> format (see curl(1))", so any format produced by libcurl should be
> fine here.  Sort the files, to be agnostic to the order of the
> cookies, and make the test pass with both curl versions > 7.61.1 and
> earlier curl versions.
>
> Signed-off-by: Thomas Gummerer 
> ---

Thanks.  f5b2c9c9 ("t5551-http-fetch-smart.sh: sort cookies before
comparing", 2018-09-07) that came from

https://public-inbox.org/git/20180907232205.31328-1-...@pobox.com

has almost the identical patch text, and this (presumably an
independent effort) confirms that the patch is needed.  The other
effort implicitly depends on the expected output is kept sorted, but
this one is more explicit---I tend to prefer this approach as tools
and automation is easier to maintain than having to remember that
the source must be sorted.

Thanks.

>  t/t5551-http-fetch-smart.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index 771f36f9ff..d13b993201 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -206,7 +206,7 @@ test_expect_success 'dumb clone via http-backend respects 
> namespace' '
>  cat >cookies.txt <  127.0.0.1FALSE   /smart_cookies/ FALSE   0   othername   
> othervalue
>  EOF
> -cat >expect_cookies.txt < +cat   
>  127.0.0.1FALSE   /smart_cookies/ FALSE   0   othername   
> othervalue
>  127.0.0.1FALSE   /smart_cookies/repo.git/info/   FALSE   0   name
> value
> @@ -215,7 +215,7 @@ test_expect_success 'cookies stored in http.cookiefile 
> when http.savecookies set
>   git config http.cookiefile cookies.txt &&
>   git config http.savecookies true &&
>   git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&
> - tail -3 cookies.txt >cookies_tail.txt &&
> + tail -3 cookies.txt | sort >cookies_tail.txt &&
>   test_cmp expect_cookies.txt cookies_tail.txt
>  '

Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder  writes:

>> I'd rather that we revert this change altogether.  I have nothing
>> against a convenient command to do this kind of non build related
>> cleanup, but it shouldn't be spelled as "make clean".
>
> OK, let's do this for now as I wanted to merge the remainder to
> 'master' today.
>
> -- >8 --
> Subject: Revert "doc/Makefile: drop doc-diff worktree and temporary files on 
> "make clean""
>
> This reverts commit 6f924265a0bf6efa677e9a684cebdde958e5ba06, which
> started to require that we have an executable git available in order
> to say "make clean", which gives us a chicken-and-egg problem.
>
> Having to have Git installed, or be in a repository, in order to be
> able to run an optional "doc-diff" tool is fine.  Requiring either
> in order to run "make clean" is a different story.
>
> Reported by Jonathan Nieder .
> ---
>  Documentation/Makefile | 1 -
>  1 file changed, 1 deletion(-)

Does this want a sign-off?

In any event, this matches what I had applied in Debian[1] and is

Reviewed-by: Jonathan Nieder 

Thanks,
Jonathan

[1] 
http://repo.or.cz/git/debian.git/blob/refs/heads/debian-experimental:/debian/patches/0002-Revert-doc-Makefile-drop-doc-diff-worktree-and-tempor.diff
aka 
http://repo.or.cz/git/debian.git/blob/9872ab1d87634a9288266de290571928e5b9346f:/debian/patches/0002-Revert-doc-Makefile-drop-doc-diff-worktree-and-tempor.diff

Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"

[Junio C Hamano]

Jonathan Nieder  writes:

> I'd rather that we revert this change altogether.  I have nothing
> against a convenient command to do this kind of non build related
> cleanup, but it shouldn't be spelled as "make clean".

OK, let's do this for now as I wanted to merge the remainder to
'master' today.

-- >8 --
Subject: Revert "doc/Makefile: drop doc-diff worktree and temporary files on 
"make clean""

This reverts commit 6f924265a0bf6efa677e9a684cebdde958e5ba06, which
started to require that we have an executable git available in order
to say "make clean", which gives us a chicken-and-egg problem.

Having to have Git installed, or be in a repository, in order to be
able to run an optional "doc-diff" tool is fine.  Requiring either
in order to run "make clean" is a different story.

Reported by Jonathan Nieder .
---
 Documentation/Makefile | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 623f1a866d..d079d7c73a 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -331,7 +331,6 @@ clean:
$(RM) SubmittingPatches.txt
$(RM) $(cmds_txt) $(mergetools_txt) *.made
$(RM) manpage-base-url.xsl
-   '$(SHELL_PATH_SQ)' ./doc-diff --clean
 
 $(MAN_HTML): %.html : %.txt asciidoc.conf
$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
-- 
2.19.0

Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder  writes:
>> Eric Sunshine wrote:

>>> +   '$(SHELL_PATH_SQ)' ./doc-diff --clean
>>
>> This means I need a copy of git in order to run "make clean".  That
>> was never required before.  It makes bootstrapping difficult --- do we
>> really need it?
>
> Gahh, you are absolutely right.  Also "doc-diff --clean", if I am
> reading the code correctly, requires us to be in a Git repository,
> not a tarball extract.
>
> Having to have Git installed, or be in a repository, in order to be
> able to run an optional "doc-diff" tool is fine.  Requiring either
> in order to run "make clean" is a different story.
>
> Thanks for spotting.  We can just prefix the line with '-'?  Or does
> the script badly misbehave (due to lack of CEILING_DIRECTORY) when
> run in a tarball extract inside somebody else's repository?

I'd rather that we revert this change altogether.  I have nothing
against a convenient command to do this kind of non build related
cleanup, but it shouldn't be spelled as "make clean".

That said, for my particular use, prefixing with '-' would work okay.

Thanks,
Jonathan

Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"

[Junio C Hamano]

Jonathan Nieder  writes:

>> +'$(SHELL_PATH_SQ)' ./doc-diff --clean
>
> This means I need a copy of git in order to run "make clean".  That
> was never required before.  It makes bootstrapping difficult --- do we
> really need it?

Gahh, you are absolutely right.  Also "doc-diff --clean", if I am
reading the code correctly, requires us to be in a Git repository,
not a tarball extract.

Having to have Git installed, or be in a repository, in order to be
able to run an optional "doc-diff" tool is fine.  Requiring either
in order to run "make clean" is a different story.

Thanks for spotting.  We can just prefix the line with '-'?  Or does
the script badly misbehave (due to lack of CEILING_DIRECTORY) when
run in a tarball extract inside somebody else's repository?

Re: [Question] Alternative to git-lfs under go

[Jonathan Nieder]

Hi,

Randall S. Becker wrote:

> Does anyone know whether it is practical to rework git-lfs under a language
> other than "go"? GCC is not even close to being possible to port to my
> NonStop platform (may have tried, some have died - joke -  trying). I would
> like to convert this directly to C or something more widely portable. Is
> there a protocol doc out there I can reference?

Can you say more about the context?  You might like

 git clone --filter=blob:limit=512m 

which tells Git to avoid downloading any blobs larger than 512 megabytes
until you know they need them.  See Documentation/technical/partial-clone.txt
for more details.

Thanks and hope that helps,
Jonathan

Re: [PATCH v2 0/1] Make 'git commit' not accidentally lose staged content

[Junio C Hamano]

Duy Nguyen  writes:

> On Mon, Sep 17, 2018 at 7:09 PM Junio C Hamano  wrote:
>> ...
>> On the other hand, if I am keeping some change that should never be
>> in a commit in the working tree file, and building the contents in
>> the index using "add -p" to incrementally, it would be the same
>> disaster as you are trying to prevent if I by mistake did a whole
>> path 'add', even if I catch myself doing so before running 'commit'
>> i.e.
>>
>> edit X
>> git add -p X
>> git diff --cached X
>> git diff X
>> ... repeat the above number of times ...
>> git add X ;# OOPS!
>> git add . ;# OOPS! even worse!
>>
>> Even though this does not involve "git commit -a" or "git commit X",
>> an unrecoverable damage that requires redoing the manual work is
>> already done.
>
> I don't see a good way to get to recover this situation. I could go
> back to the "index log" idea,...

FWIW, I didn't mean to say that we should give users a way to
recover.  Your "commit -a" or "commit $path" protection just
prevents the situation from happening, and I think it is sufficient.

The sole point I wanted to raise by bringing up the above was that
we should have the same degree of protection against "add $path" or
"add -u".

Of course, "index log" is interesting and it may even turn out to be
useful (I was skeptical about "reference log" the same way, but it
turned out to be useful without burdening the system too heavily),
and it may even remove the need for the "do not accidentally lose
information by adding more to the index" protection.  But until that
happens, if we are to have such a protection, we would wnat to give
the same degree of protection to "commit" and "add".

[Question] Alternative to git-lfs under go

[Randall S. Becker]

Hi All,

Does anyone know whether it is practical to rework git-lfs under a language
other than "go"? GCC is not even close to being possible to port to my
NonStop platform (may have tried, some have died - joke -  trying). I would
like to convert this directly to C or something more widely portable. Is
there a protocol doc out there I can reference?

Thanks,
Randall

-- Brief whoami:
 NonStop developer since approximately 2112884442
 UNIX developer since approximately 421664400
-- In my real life, I talk too much.

[PATCH] t5551: compare sorted cookies files

[Thomas Gummerer]

In t5551 we check that we save cookies correctly to a file when
http.cookiefile and http.savecookies are set.  To do so we create an
expect file that expects the cookies in a certain order.

However after e2ef8d6fa ("cookies: support creation-time attribute for
cookies", 2018-08-28) in curl.git (released in curl 7.61.1) that order
changed.

We document the file format as "Netscape/Mozilla cookie file
format (see curl(1))", so any format produced by libcurl should be
fine here.  Sort the files, to be agnostic to the order of the
cookies, and make the test pass with both curl versions > 7.61.1 and
earlier curl versions.

Signed-off-by: Thomas Gummerer 
---
 t/t5551-http-fetch-smart.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 771f36f9ff..d13b993201 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -206,7 +206,7 @@ test_expect_success 'dumb clone via http-backend respects 
namespace' '
 cat >cookies.txt cookies_tail.txt &&
+   tail -3 cookies.txt | sort >cookies_tail.txt &&
test_cmp expect_cookies.txt cookies_tail.txt
 '
 
-- 
2.19.0.444.g18242da7ef

Re: [PATCH v4 02/23] read-cache.c: remove 'const' from index_has_changes()

[Junio C Hamano]

Duy Nguyen  writes:

> On Mon, Sep 17, 2018 at 6:25 PM Junio C Hamano  wrote:
>>
>> Nguyễn Thái Ngọc Duy   writes:
>>
>> > This function calls do_diff_cache() which eventually needs to set this
>> > "istate" to unpack_options->src_index (*). This is an unfortunate fact
>> > that unpack_trees() _will_ destroy src_index so we can't really pass a
>>
>> Wasn't the whole point of introducing src_index and dst_index to
>> unpack-trees API so that we can keep the src_index read-only by
>> writing the result of merge to a separate in-core dst_index?
>>
>> What does the above exactly mean by "will destroy src_index"?  Is it
>> now fundamental that src_index needs to lack constness, or is it
>> something easy to fix?
>
> "destroy" is probably a strong word, but we do modify the src_index, e.g.
>
>  mark_all_ce_unused(o->src_index);
>  mark_new_skip_worktree(.., o->src_index...
>  move_index_extensions(>result, o->src_index);
>  invalidate_ce_path();
>
> all these update the source index. It is possible to fix, but I don't
> think it's exactly easy and may even incur some performance cost (e.g.
> if we stop modify ce_flags in the src_index, then we need to do one
> extra lookup to wherever we store these flags).

Ah, OK.  I thought that we'd be doing something, if write_tree() is
called on src_index before and after these operation, to cause us to
see two different trees, which made me worried.

But what you mean is that transient bits in the istate or cache
entries reachable from it are touched while we perform operations
that are read-only in spirit, and we need to pass non const pointer
around.

The phrasing in the log message does need to be updated to avoid
similar confusion by future readers, but I think I understand and
agree with the direction/approach.

Thanks.

Re: [PATCH v5 1/5] eoie: add End of Index Entry (EOIE) extension

[Junio C Hamano]

Duy Nguyen  writes:

> I get annoyed by the "ignoring unknown extension xxx" messages while
> testing though (not just this extension) and I think it will be the
> same for other git implementations. But perhaps other implementations
> just silently drop the extension. Most of the extensions we have added
> so far (except the ancient 'TREE') are optional and are probably not

Most of the index extensions are optional, including TREE.  I think
"link" is the only one that the readers that do not understand it
are told to abort without causing damage.

> present 99% of time when a different git impl reads an index created
> by C Git. This 'EIOE' may be a good test then to see if they follow
> the "ignore optional extensions" rule since it will always appear in
> new C Git releases.

I think we probably should squelch "ignoring unknown" unless some
sort of GIT_TRACE/DEBUG switch is set.

Patches welcome ;-)

Thanks.

Re: What's cooking in git.git (Sep 2018, #03; Fri, 14)

[Junio C Hamano]

Derrick Stolee  writes:

>>   Replaced with a newer version.
>
> I think this has been the same note for a few weeks now. What does it
> mean? Did I send a new version out that you haven't picked up?

It just means that I didn't bother to look at the current status
message for the topic that had no activity X-<.

> Also, my "git log --topo-order" series was never picked up, but I see
> it has conflicts with 'next' now, so I'll rebase to resolve conflicts
> and send a v2.

Good idea, as I do not recall anything about that series.

Re: [PATCH v5 3/5] read-cache: load cache entries on worker threads

[Ben Peart]

On 9/15/2018 7:09 AM, Duy Nguyen wrote:

On Sat, Sep 15, 2018 at 01:07:46PM +0200, Duy Nguyen wrote:

12:50:00.084237 read-cache.c:1721   start loading index
12:50:00.119941 read-cache.c:1943   performance: 0.034778758 s: loaded all 
extensions (1667075 bytes)
12:50:00.185352 read-cache.c:2029   performance: 0.100152079 s: loaded 
367110 entries
12:50:00.189683 read-cache.c:2126   performance: 0.104566615 s: finished 
scanning all entries
12:50:00.217900 read-cache.c:2029   performance: 0.082309193 s: loaded 
367110 entries
12:50:00.259969 read-cache.c:2029   performance: 0.070257130 s: loaded 
367108 entries
12:50:00.263662 read-cache.c:2278   performance: 0.179344458 s: read cache 
.git/index


The previous mail wraps these lines and make it a bit hard to read. Corrected 
now.

--
Duy



Interesting!  Clearly the data shape makes a big difference here as I 
had run a similar test but in my case, the extensions thread actually 
finished last (and it's cost is what drove me to move that onto a 
separate thread that starts first).


Purpose First   LastDuration
load_index_extensions_thread719.40  968.50  249.10
load_cache_entries_thread   718.89  738.65  19.76
load_cache_entries_thread   730.39  753.83  23.43
load_cache_entries_thread   741.23  751.23  10.00
load_cache_entries_thread   751.93  780.88  28.95
load_cache_entries_thread   763.60  791.31  27.72
load_cache_entries_thread   773.46  783.46  10.00
load_cache_entries_thread   783.96  794.28  10.32
load_cache_entries_thread   795.61  805.52  9.91
load_cache_entries_thread   805.99  827.21  21.22
load_cache_entries_thread   816.85  826.85  10.00
load_cache_entries_thread   827.03  837.96  10.93

In my tests, the scanning thread clearly delayed the later ce threads 
but given the extension was so slow, it didn't impact the overall time 
nearly as much as your case.


I completely agree that the optimal solution would be to go back to my 
original patch/design.  It eliminates the overhead of the scanning 
thread entirely and allows all threads to start at the same time. This 
would ensure the best performance whether the extensions were the 
longest thread or the cache entry threads took the longest.


I ran out of time and energy last year so dropped it to work on other 
tasks.  I appreciate your offer of help. Perhaps between the two of us 
we could successfully get it through the mailing list this time. :-) 
Let me go back and see what it would take to combine the current EOIE 
patch with the older IEOT patch.


I'm also intrigued with your observation that over committing the cpu 
actually results in time savings.  I hadn't tested that.  It looks like 
that could have a positive impact on the overall time and warrant a 
change to the default nr_threads logic.

Re: [PATCH v2] linear-assignment: fix potential out of bounds memory access

[Jonathan Nieder]

Thomas Gummerer wrote:

> Currently the 'compute_assignment()' function may read memory out
> of bounds, even if used correctly.  Namely this happens when we only
> have one column.
[...]
> Reported-by: ryenus 
> Helped-by: Derrick Stolee 
> Signed-off-by: Thomas Gummerer 
> ---
>  linear-assignment.c   | 6 ++
>  t/t3206-range-diff.sh | 5 +
>  2 files changed, 11 insertions(+)

Here's a bit of a non-review, but hopefully it pokes others into
doing a proper review.

I haven't carefully examined the checks you're adding here.  Your
goals seem to be right.  I've been running with this patch since last
Thursday, with no problems yet.  Thanks for writing it.

Sincerely,
Jonathan

Re: [PATCH v2 0/1] Make 'git commit' not accidentally lose staged content

[Duy Nguyen]

On Mon, Sep 17, 2018 at 8:15 PM Jeff King  wrote:
> On Mon, Sep 17, 2018 at 07:29:26PM +0200, Duy Nguyen wrote:
> > I don't see a good way to get to recover this situation. I could go
> > back to the "index log" idea, where we keep a log of index changes (or
> > just "interesting" changes). That way there's no behavior change at
> > all. The user who accidentally updates/deletes something can always
> > retrieve the old content back (assuming that they realize quickly
> > since we can't keep very long log).
>
> FWIW, I like that approach much better, since:
>
>   1. It does not bother or restrict anybody in their workflow; instead,
>  they pay the complexity price only when they know they have made a
>  mistake.
>
>   2. It covers many more cases (e.g., just doing the wrong thing via
>  "add -p").
>
> A naive index log would be pretty cheap in CPU, at least for POSIX-ish
> systems. You could just hard link "index" to "index.N" before renaming
> "index.lock" over "index". But I guess if you have a gigantic index,
> that's less appealing. So maybe storing the equivalent of a "--raw" diff
> between the two index states would make more sense (and after all, you
> don't really need the stat-cache or cache-tree). It would cost more to
> reconstruct the index on the fly, but then the point is that you would
> create these logs a lot more than you access them.

Yeah. The problem though is extracting the information out of these
index.N files.

> > I've been thinking about allowing to undo worktree changes too (e.g.
> > accidental "git reset --hard") and this log can cover it as well.
>
> I like that, too. It's a little more costly just because it may involve
> object-db writes, but I think in most cases it would be fine. I almost
> always "git stash" away discarded changes these days instead of "git
> reset --hard", because it effectively provides this kind of log.

Yeah the added cost is pretty much given. You want safety, you pay extra :)

> > The only downside is we need a new command for the UI (or perhaps I
> > can just add "git add --log" or something like that).
>
> I think the reflog approach has been successful: give these intermediate
> states a name. So in theory I could do something like:
>
>   git checkout -p :@{2.minutes.ago}
>
> though it would probably be useful to be able to walk the states, too,
> just like we have "log --reflog-walk".
>
> The syntax above is off-the-cuff (and based on the ":" index
> syntax). I guess if we had a separate log for "stuff in the worktree
> that got thrown away by reset" we might need a separate syntax.

I'm leaning towards reflog too. Not because of the syntax but because
of reusing the current code base. I don't have to worry about pruning,
walking, gc-ing... because it's pretty much already there. And the UI
is not so urgent since reflog file is very readable, early adopters
can just open the file and get the hash.

This also lets me handle logging worktree changes in the future
(keeping track of untracked files is impossible with the "index.N"
approach)

I'm trying to quickly make something that writes to
"$GIT_DIR/logs/index" and see how it goes. But yeah I'll probably drop
this patch. The ":@{2.minutes.ago}" just makes me like this direction
more, even though I don't know if I could even make that work.
-- 
Duy

Re: [PATCH 2/3] gc: exit with status 128 on failure

[Jonathan Nieder]

Hi,

Jeff King wrote:
> On Tue, Jul 17, 2018 at 03:59:47PM -0400, Jeff King wrote:
>> On Mon, Jul 16, 2018 at 11:54:16PM -0700, Jonathan Nieder wrote:

>>> A value of -1 returned from cmd_gc gets propagated to exit(),
>>> resulting in an exit status of 255.  Use die instead for a clearer
>>> error message and a controlled exit.
>>
>> This feels a little funny because we know we're going to turn some of
>> these back in the next patch. That said, I'm OK with it, since this
>> version is already written.
>
> There's discussion elsewhere[1] of applying just up to patch 2.
>
> Do we still want to convert these cases to die() as their end-state?

IMHO yes, we do.  die() is the function that you can use to exit with
a fatal error.

If we want to get rid of die(), that would be a tree-wide effort, not
something that should hold up this patch.

[...]
> It also makes the code more flexible and lib-ifiable (since the caller
> can decide how to handle the errors). That probably doesn't matter much
> since this is all static-local to builtin/gc.c,

Exactly.  I'm a strong believer in http://wiki.c2.com/?YouArentGonnaNeedIt.

Thanks,
Jonathan

Re: What's cooking in git.git (Sep 2018, #03; Fri, 14)

[Derrick Stolee]

On 9/14/2018 5:56 PM, Junio C Hamano wrote:

* ds/format-commit-graph-docs (2018-08-21) 2 commits
  - commit-graph.txt: improve formatting for asciidoc
  - Docs: Add commit-graph tech docs to Makefile

  Design docs for the commit-graph machinery is now made into HTML as
  well as text.

  Will discard.
  I am inclined to drop these, as I do not see much clarity in HTML
  output over the text source.  Opinions?


Discarding is fine. I originally created it because I thought we were 
supposed to mark all documents for HTML generation. You're right, the 
HTML doesn't add anything.



* ds/commit-graph-with-grafts (2018-08-21) 8 commits
  - commit-graph: close_commit_graph before shallow walk
  - commit-graph: not compatible with uninitialized repo
  - commit-graph: not compatible with grafts
  - commit-graph: not compatible with replace objects
  - test-repository: properly init repo
  - commit-graph: update design document
  - refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback
  - refs.c: migrate internal ref iteration to pass thru repository argument

  The recently introduced commit-graph auxiliary data is incompatible
  with mechanisms such as replace & grafts that "breaks" immutable
  nature of the object reference relationship.  Disable optimizations
  based on its use (and updating existing commit-graph) when these
  incompatible features are in use in the repository.

  Replaced with a newer version.


I think this has been the same note for a few weeks now. What does it 
mean? Did I send a new version out that you haven't picked up?


Also, my "git log --topo-order" series was never picked up, but I see it 
has conflicts with 'next' now, so I'll rebase to resolve conflicts and 
send a v2.


Thanks,
-Stolee

Re: [PATCH 2/3] gc: exit with status 128 on failure

[Jeff King]

On Tue, Jul 17, 2018 at 03:59:47PM -0400, Jeff King wrote:

> On Mon, Jul 16, 2018 at 11:54:16PM -0700, Jonathan Nieder wrote:
> 
> > A value of -1 returned from cmd_gc gets propagated to exit(),
> > resulting in an exit status of 255.  Use die instead for a clearer
> > error message and a controlled exit.
> 
> This feels a little funny because we know we're going to turn some of
> these back in the next patch. That said, I'm OK with it, since this
> version is already written.

There's discussion elsewhere[1] of applying just up to patch 2.

Do we still want to convert these cases to die() as their end-state?
Another alternative would be to just convert a "-1" return to 128 or
similar at the level of cmd_gc(), which avoids the 255 weirdness.

Doing so also keeps the error messages the same as (as "error" and not
"fatal"). I'm not sure which we prefer.

It also makes the code more flexible and lib-ifiable (since the caller
can decide how to handle the errors). That probably doesn't matter much
since this is all static-local to builtin/gc.c, though I suppose in
theory we could eventually do parts of "gc --auto" without spawning a
separate process.

-Peff

[1] https://public-inbox.org/git/20180917182210.gb3...@sigill.intra.peff.net/

Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"

[Jonathan Nieder]

Hi,

Eric Sunshine wrote:

> doc-diff creates a temporary working tree (git-worktree) and generates a
> bunch of temporary files which it does not remove since they act as a
> cache to speed up subsequent runs. Although doc-diff's working tree and
> generated files are not strictly build products of the Makefile (which,
> itself, never runs doc-diff), as a convenience, update "make clean" to
> clean up doc-diff's working tree and generated files along with other
> development detritus normally removed by "make clean".
>
> Signed-off-by: Eric Sunshine 
> ---
>  Documentation/Makefile | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index a42dcfc745..26e268ae8d 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -332,6 +332,7 @@ clean:
>   $(RM) SubmittingPatches.txt
>   $(RM) $(cmds_txt) $(mergetools_txt) *.made
>   $(RM) manpage-base-url.xsl
> + '$(SHELL_PATH_SQ)' ./doc-diff --clean

This means I need a copy of git in order to run "make clean".  That
was never required before.  It makes bootstrapping difficult --- do we
really need it?

Thanks,
Jonathan

Re: What's cooking in git.git (Sep 2018, #03; Fri, 14)

[Jonathan Nieder]

Jeff King wrote:

> Let me inject some more uncertainty, then. ;)
>
> If we are not going to do 3/3, then should 2/3 simply avoid passing "-1"
> back via return from main? I guess I don't have a strong opinion, but
> one of the things I noted was that we converted those die() calls
> introduced in 2/3 back into returns in 3/3. Do we want to leave it in
> the state where we are calling die() a lot more?

Would you mind replying in the patch thread instead of this what's
cooking email?

That way, I can understand your suggestion better in context, I can
find it more easily later, I would feel less bad about adding noise by
replying, etc.

Thanks,
Jonathan

Re: What's cooking in git.git (Sep 2018, #03; Fri, 14)

[Jeff King]

On Mon, Sep 17, 2018 at 10:51:35AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> >> >  What's the donness of this one?
> >> >  cf. <20180717201348.gd26...@sigill.intra.peff.net>
> >> 
> >> This topic has stayed in 'pu' for a long time. I thought it was
> >> concluded that this was a good change? Jeff, Jonathan?
> >
> > I read over the thread again. I don't think I actually have any
> > complaints about the patches as-is. There was some discussion from Junio
> > and Ævar about the third one. I don't have a strong opinion. My
> > experience has been that "gc --auto" is garbage anyway on the server
> > side, but I think Ævar's experience is that it's reasonable for small to
> > medium sites (which seems plausible to me).
> >
> > The message-id quoted there is my "this looks good". I mentioned a few
> > possible nits, but I think it would be OK with or without them
> > addressed.
> 
> That matches my reading of your position.  I tend to agree with
> Ævar's recommendation to postpone 3/3 and use 1 & 2 for now.

Let me inject some more uncertainty, then. ;)

If we are not going to do 3/3, then should 2/3 simply avoid passing "-1"
back via return from main? I guess I don't have a strong opinion, but
one of the things I noted was that we converted those die() calls
introduced in 2/3 back into returns in 3/3. Do we want to leave it in
the state where we are calling die() a lot more?

-Peff

[PATCH] mailmap: consistently normalize brian m. carlson's name

[Jonathan Nieder]

v2.18.0-rc0~70^2 (mailmap: update brian m. carlson's email address,
2018-05-08) changed the mailmap to map

  sand...@crustytoothpaste.ath.cx
   -> brian m. carlson 

instead of

  sand...@crustytoothpaste.net
-> brian m. carlson 

That means the mapping

  Brian M. Carlson 
-> brian m. carlson 

is redundant, so we can remove it.  More importantly, it means that
the identity "Brian M. Carlson " used in
some commits is not normalized any more.  Add a mapping for it.

Noticed while updating Debian's Git packaging, which uses "git
shortlog --no-merges" to produce a list of changes in each version,
grouped by author's (normalized) name.

Signed-off-by: Jonathan Nieder 
---
brian m. carlson wrote:
> On Tue, May 22, 2018 at 03:08:26PM -0700, Jonathan Nieder wrote:

>> These commits use author Brian M. Carlson .
>> Previously they matched
>>
>>  brian m. carlson  
>> 
>>
>> but now they don't match any line in the .mailmap file.
>>
>> Should we add a line
>>
>>  brian m. carlson 
>>
>> to handle these commits?
>
> Ah, good point.  Probably so.

Sorry to leave this hanging for so long.  How about this patch?

Thanks,
Jonathan

 .mailmap | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.mailmap b/.mailmap
index f165222a78..29047a110a 100644
--- a/.mailmap
+++ b/.mailmap
@@ -25,7 +25,7 @@ Ben Walton  
 Benoit Sigoure  
 Bernt Hansen  
 Brandon Casey  
-brian m. carlson  Brian M. Carlson 

+brian m. carlson  Brian M. Carlson 

 brian m. carlson  

 Bryan Larsen  
 Bryan Larsen  
-- 
2.19.0.397.gdd90340f6a

Re: [PATCH v2 0/1] Make 'git commit' not accidentally lose staged content

[Jeff King]

On Mon, Sep 17, 2018 at 07:29:26PM +0200, Duy Nguyen wrote:

> > On the other hand, if I am keeping some change that should never be
> > in a commit in the working tree file, and building the contents in
> > the index using "add -p" to incrementally, it would be the same
> > disaster as you are trying to prevent if I by mistake did a whole
> > path 'add', even if I catch myself doing so before running 'commit'
> > i.e.
> >
> > edit X
> > git add -p X
> > git diff --cached X
> > git diff X
> > ... repeat the above number of times ...
> > git add X ;# OOPS!
> > git add . ;# OOPS! even worse!
> >
> > Even though this does not involve "git commit -a" or "git commit X",
> > an unrecoverable damage that requires redoing the manual work is
> > already done.
> 
> I don't see a good way to get to recover this situation. I could go
> back to the "index log" idea, where we keep a log of index changes (or
> just "interesting" changes). That way there's no behavior change at
> all. The user who accidentally updates/deletes something can always
> retrieve the old content back (assuming that they realize quickly
> since we can't keep very long log).

FWIW, I like that approach much better, since:

  1. It does not bother or restrict anybody in their workflow; instead,
 they pay the complexity price only when they know they have made a
 mistake.

  2. It covers many more cases (e.g., just doing the wrong thing via
 "add -p").

A naive index log would be pretty cheap in CPU, at least for POSIX-ish
systems. You could just hard link "index" to "index.N" before renaming
"index.lock" over "index". But I guess if you have a gigantic index,
that's less appealing. So maybe storing the equivalent of a "--raw" diff
between the two index states would make more sense (and after all, you
don't really need the stat-cache or cache-tree). It would cost more to
reconstruct the index on the fly, but then the point is that you would
create these logs a lot more than you access them.

> I've been thinking about allowing to undo worktree changes too (e.g.
> accidental "git reset --hard") and this log can cover it as well.

I like that, too. It's a little more costly just because it may involve
object-db writes, but I think in most cases it would be fine. I almost
always "git stash" away discarded changes these days instead of "git
reset --hard", because it effectively provides this kind of log.

> The only downside is we need a new command for the UI (or perhaps I
> can just add "git add --log" or something like that).

I think the reflog approach has been successful: give these intermediate
states a name. So in theory I could do something like:

  git checkout -p :@{2.minutes.ago}

though it would probably be useful to be able to walk the states, too,
just like we have "log --reflog-walk".

The syntax above is off-the-cuff (and based on the ":" index
syntax). I guess if we had a separate log for "stuff in the worktree
that got thrown away by reset" we might need a separate syntax.

> Should I just drop this patch and go that direction instead? More
> work, but maybe better end result.

I guess my whole response is a long-winded "yes". ;)

-Peff

Re: Brandon Williams's E-Mail bouncing

[Stefan Beller]

On Mon, Sep 17, 2018 at 10:51 AM Ævar Arnfjörð Bjarmason
 wrote:
>
> Aside from that particular address bouncing [...] it would be nice if git
> {format-patch,send-email,check-contacts} & the .mailmap format would
> support and understand some way to map addresses to
> e.g. nore...@example.com, and prune them out appropriately. We have a
> lot of addresses from past authors which bounce, and where no current
> contact address is known.

Tombstones, eh?

For send-email I would very much prefer the proactive warning instead
of silently dropping that email address as I would want to know, so I can
try to follow up somehow and deal with it as-is.

I think the main purpose of the mail map file until now was to consolidate
multiple identities into one (e.g. misspellings or capitalisation issues in
names, different email addresses for all the same person), now you want to
extend it to also contain more information about an author-ident, such as
emails being active? Or recording if someone is not interested in the project
anymore? I think recording that an email bouncesl is just duplicating
information
that can be easily checked. Recording that a particular contributor doesn't
have time/interest any more is slightly different, but IMHO the mail map file
is also not very well suited for that endeavor.

Oh, the above paragraph is written with git - the tool - in mind, not
the git.git
repository where we can (ab)use our own tooling. ;-)

So mapping all bouncing emails to no-re...@example.com would map them
to the same author ident, which we would not want. (e.g. we'd still want to
know how many different people contributed so far, hence keep them separate).
We'd need to have unique email addresses that still explain their intent,
i.e. +no-re...@example.org would work?

Thanks,
Stefan

Brandon Williams's E-Mail bouncing

[Ævar Arnfjörð Bjarmason]

[Sorry about the double send in private, forgot to CC the list]

On Mon, Sep 17 2018, Duy Nguyen wrote:

> Brandon, b0db704652[...]

I noticed a few days ago that CC's to bmw...@google.com bounce. Has he
left Google, and if so is he still interested in being CC'd on Git stuff
(maybe he'll chime in), or not interested?

Aside from that particular address bouncing (or not, maybe Google's MX
just dislikes me), it would be nice if git
{format-patch,send-email,check-contacts} & the .mailmap format would
support and understand some way to map addresses to
e.g. nore...@example.com, and prune them out appropriately. We have a
lot of addresses from past authors which bounce, and where no current
contact address is known.

Re: What's cooking in git.git (Sep 2018, #03; Fri, 14)

[Junio C Hamano]

Jeff King  writes:

>> >  What's the donness of this one?
>> >  cf. <20180717201348.gd26...@sigill.intra.peff.net>
>> 
>> This topic has stayed in 'pu' for a long time. I thought it was
>> concluded that this was a good change? Jeff, Jonathan?
>
> I read over the thread again. I don't think I actually have any
> complaints about the patches as-is. There was some discussion from Junio
> and Ævar about the third one. I don't have a strong opinion. My
> experience has been that "gc --auto" is garbage anyway on the server
> side, but I think Ævar's experience is that it's reasonable for small to
> medium sites (which seems plausible to me).
>
> The message-id quoted there is my "this looks good". I mentioned a few
> possible nits, but I think it would be OK with or without them
> addressed.

That matches my reading of your position.  I tend to agree with
Ævar's recommendation to postpone 3/3 and use 1 & 2 for now.

By the way, people shouldn't read too much into the messages
referred to in "What's cooking"; they are not "these messages point
out all the issues that block the topic".  It's just "I am aware of
this thread, which readers would hopefully find a good starting
point to form their opinions".

Thanks.

Re: [PATCH v5 1/5] eoie: add End of Index Entry (EOIE) extension

[Duy Nguyen]

On Mon, Sep 17, 2018 at 7:31 PM Junio C Hamano  wrote:
>
> Duy Nguyen  writes:
>
> > But it _is_ available now. If you need it, you write the extension
> > out.
>
> Are you arguing for making it omitted when it is not needed (e.g.
> small enough index file)?  IOW, did you mean "If you do not need it,
> you do not write it out" by the above?

Yes I did.

> I do not think overhead of writing (or preparing to write) the
> extension for a small index file is by definition small enough ;-).

Good point.

I get annoyed by the "ignoring unknown extension xxx" messages while
testing though (not just this extension) and I think it will be the
same for other git implementations. But perhaps other implementations
just silently drop the extension. Most of the extensions we have added
so far (except the ancient 'TREE') are optional and are probably not
present 99% of time when a different git impl reads an index created
by C Git. This 'EIOE' may be a good test then to see if they follow
the "ignore optional extensions" rule since it will always appear in
new C Git releases.
-- 
Duy

You Won 2,000,000.00 pounds

[United Nation]

Contact Account Officer For Claims:
Mr Eric Elliot Francis
Email: claim...@gmail.com

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

Re: [PATCH v5 1/5] eoie: add End of Index Entry (EOIE) extension

[Junio C Hamano]

Duy Nguyen  writes:

> But it _is_ available now. If you need it, you write the extension
> out.

Are you arguing for making it omitted when it is not needed (e.g.
small enough index file)?  IOW, did you mean "If you do not need it,
you do not write it out" by the above?

I do not think overhead of writing (or preparing to write) the
extension for a small index file is by definition small enough ;-).

I do not think the configuration that decides if the reader side
uses parallel reading should have any say in the decision to write
(or omit) the extension, by the way.

Re: [PATCH v2 0/1] Make 'git commit' not accidentally lose staged content

[Duy Nguyen]

On Mon, Sep 17, 2018 at 7:09 PM Junio C Hamano  wrote:
>
> Nguyễn Thái Ngọc Duy   writes:
>
> > This is about mixing "git add -p" and "git commit -a" (or "git commit
> > ") where you may accidentally lose staged changes. After the
> > discussion with Jonathan, I'm going with a bit different approach than
> > v1, this behavior now becomes default, and if the user wants the old
> > behavior back, they can use --clobber-index.
> >
> > Another change is "git commit " is covered as well, as pointed
> > out by Jacob.
> >
> > I will need to add some test cases of course, if this direction is
> > still promising. One thing I'm not sure about is whether want to
> > deliberately clobber the index often, then perhaps we should add a
> > config key to bring the old behavior back.
>
> It usually is safer (simply because you do not have to think about
> it) to start a behaviour change like this as a strict opt-in to gain
> confidence.

Heh. The RFC was opt-in. Jonathan suggested changing default behavior
and I went along just to see how far I could push it :)

> As I often see myself futzing with the same file, adding changes to
> it incrementally, so that I can view progress in "diff --cached" and
> "diff" output, it would be a serious usability regression if the
> last step in the following sequence is rejected by default:
>
> edit X
> git add X
> git diff --cached X
> git diff X
> ... repeat the above number of times ...
> git commit X ;# or "git commit -a" to finally conclude

But yes if there's a valid use case where this behavior change becomes
a problem, then opt-in makes more sense.

> On the other hand, if I am keeping some change that should never be
> in a commit in the working tree file, and building the contents in
> the index using "add -p" to incrementally, it would be the same
> disaster as you are trying to prevent if I by mistake did a whole
> path 'add', even if I catch myself doing so before running 'commit'
> i.e.
>
> edit X
> git add -p X
> git diff --cached X
> git diff X
> ... repeat the above number of times ...
> git add X ;# OOPS!
> git add . ;# OOPS! even worse!
>
> Even though this does not involve "git commit -a" or "git commit X",
> an unrecoverable damage that requires redoing the manual work is
> already done.

I don't see a good way to get to recover this situation. I could go
back to the "index log" idea, where we keep a log of index changes (or
just "interesting" changes). That way there's no behavior change at
all. The user who accidentally updates/deletes something can always
retrieve the old content back (assuming that they realize quickly
since we can't keep very long log).

I've been thinking about allowing to undo worktree changes too (e.g.
accidental "git reset --hard") and this log can cover it as well.

The only downside is we need a new command for the UI (or perhaps I
can just add "git add --log" or something like that).

Should I just drop this patch and go that direction instead? More
work, but maybe better end result.

> How should this new check intract with paths added with "add -N", by
> the way?

Good point. The check here is basically "git diff --cached". I would
need to turn it to "git diff --cached --ita-invisible-in-index" to
make ita entries disappear.
-- 
Duy

Re: [PATCH v5 3/5] read-cache: load cache entries on worker threads

[Ben Peart]

On 9/15/2018 6:31 AM, Duy Nguyen wrote:

On Wed, Sep 12, 2018 at 6:18 PM Ben Peart  wrote:


This patch helps address the CPU cost of loading the index by creating
multiple threads to divide the work of loading and converting the cache
entries across all available CPU cores.

It accomplishes this by having the primary thread loop across the index file
tracking the offset and (for V4 indexes) expanding the name. It creates a
thread to process each block of entries as it comes to them.

I used p0002-read-cache.sh to generate some performance data:

Test w/100,000 filesBaseline Parallel entries
---
read_cache/discard_cache 1000 times 14.08(0.01+0.10) 9.72(0.03+0.06) -31.0%

Test w/1,000,000 files  Baseline Parallel entries
--
read_cache/discard_cache 1000 times 202.95(0.01+0.07) 154.14(0.03+0.06) -24.1%


The numbers here and the previous patch to load extensions in parallel
are exactly the same. What do these numbers mean? With both changes?



It means I messed up when creating my commit message for the extension 
patch and copy/pasted the wrong numbers.  Yes, these numbers are with 
both changes (the correct numbers for the extension only are not as good).

Re: Git for games working group

[Ævar Arnfjörð Bjarmason]

On Mon, Sep 17 2018, Joey Hess wrote:

> Ævar Arnfjörð Bjarmason wrote:
>> There's surely other aspects of that square peg of large file tracking
>> not fitting the round hole of file locking, the point of my write-up was
>> not that *that* solution is perfect, but there's prior art here that's
>> very easily adopted to distributed locking if someone wanted to scratch
>> that itch, since the notion of keeping a log of who has/hasn't gotten a
>> file is very similar to a log of who has/hasn't locked some file(s) in
>> the tree.
>
> Actually they are fundamentally very different. git-annex's tracking of
> locations of files is eventually consistent, which of course means that
> at any given point in time it may be currently inconsistent. That is
> fine for tracking locations of files, but not for locking.
>
> When git-annex needs to do an operation that relies on someone else's
> copy of a file actually being present, it uses real locking. That
> locking is not centralized, instead it relies on the connections between
> git repositories. That turns out to be sufficient for git-annex's own
> locking needs, but it would not be sufficient to avoid file edit
> conflict problems in eg a split brain situation.

Right, all of that's true. I forgot to explicitly say what I meant by
"locking" in this context. Clearly it's not suitable for something like
actual file locking (in the sense of flock() et al), but rather just
advisory locking in the loosest sense of the word, i.e. some git-ish way
of someone writing on the office whiteboard "unless you're Bob, don't
touch main.c today Tuesday Sep 17th, he's hacking on it".

So just a way to have some eventually consistent side channel to pass
such a message through git. Something similar to what git-annex does
with its "git-annex" branch would work for that, as long as everyone who
wanted get such messages ran some equivalent of "git annex sync" in a
timely manner (or checked the office whiteboard every day...).

Such a schema is never going to be 100% reliable even in centralized
source control systems, e.g. even with cvs/perforce you might pull the
latest changes, then go on a plane and edit the locked main.c. Then the
lock has "failed" in the sense of "the message didn't get there in time,
and two people who could have just picked different areas to work on
made conflicting edits".

As noted upthread this isn't my use-case, I just wanted to point the
git-annex method of distributing metadata as a bolt-on to git as
interesting prior art. If someone wants "truly distributed, but with
file locking like cvs/perforce" something like what git-annex is doing
would probably work for them.

Re: [PATCH v5 2/5] read-cache: load cache extensions on a worker thread

[Junio C Hamano]

Duy Nguyen  writes:

> On Sat, Sep 15, 2018 at 12:22 PM Duy Nguyen  wrote:
>> Wait there's no way to disable this parallel reading? Does not sound
>> right. And  if ordinary numbers mean the number of threads then 0
>> should mean no threading. Auto detection could have a new keyword,
>> like 'auto'.
>
> My bad. Disabling threading means _1_ thread. What was I thinking...

I did the same during my earlier review.  It seems that it somehow
is unintuitive to us that we do not specify how many _extra_ threads
of control we dedicate to ;-).

Re: [PATCH v2 0/1] Make 'git commit' not accidentally lose staged content

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> This is about mixing "git add -p" and "git commit -a" (or "git commit
> ") where you may accidentally lose staged changes. After the
> discussion with Jonathan, I'm going with a bit different approach than
> v1, this behavior now becomes default, and if the user wants the old
> behavior back, they can use --clobber-index.
>
> Another change is "git commit " is covered as well, as pointed
> out by Jacob.
>
> I will need to add some test cases of course, if this direction is
> still promising. One thing I'm not sure about is whether want to
> deliberately clobber the index often, then perhaps we should add a
> config key to bring the old behavior back.

It usually is safer (simply because you do not have to think about
it) to start a behaviour change like this as a strict opt-in to gain
confidence.

As I often see myself futzing with the same file, adding changes to
it incrementally, so that I can view progress in "diff --cached" and
"diff" output, it would be a serious usability regression if the
last step in the following sequence is rejected by default:

edit X
git add X
git diff --cached X
git diff X
... repeat the above number of times ...
git commit X ;# or "git commit -a" to finally conclude

On the other hand, if I am keeping some change that should never be
in a commit in the working tree file, and building the contents in
the index using "add -p" to incrementally, it would be the same
disaster as you are trying to prevent if I by mistake did a whole
path 'add', even if I catch myself doing so before running 'commit'
i.e.

edit X
git add -p X
git diff --cached X
git diff X
... repeat the above number of times ...
git add X ;# OOPS!
git add . ;# OOPS! even worse!

Even though this does not involve "git commit -a" or "git commit X",
an unrecoverable damage that requires redoing the manual work is
already done.

The approach to check if the contents in the index matches that in
the HEAD per-path (i.e. "The contents we are adding to the index is
whole working tree contents for that path.  But the index already
has contents different from HEAD for the path---are we losing
information by doing this?"), is a very good one.  But for the
protection to be effective, I think "git commit" and "git add"
should be covered the same way, ideally with the same code and
possibly the same configuration knob and/or command line option to
control the behaviour.

If the information loss caused by the "add/commit X" or "add
-u/commit -a" is so serious that this new feature deserves to become
the default (which I do not yet think it is the case, by the way),
then we could even forbid "commit X" or "commit -a" when the paths
involved has difference between the index and the HEAD, without any
configuration knob or command line override for "commit", and then
tell the users to use "git add/rm" _with_ the override before coming
back to "git commit".

How should this new check intract with paths added with "add -N", by
the way?

Re: Git for games working group

[Joey Hess]

Ævar Arnfjörð Bjarmason wrote:
> There's surely other aspects of that square peg of large file tracking
> not fitting the round hole of file locking, the point of my write-up was
> not that *that* solution is perfect, but there's prior art here that's
> very easily adopted to distributed locking if someone wanted to scratch
> that itch, since the notion of keeping a log of who has/hasn't gotten a
> file is very similar to a log of who has/hasn't locked some file(s) in
> the tree.

Actually they are fundamentally very different. git-annex's tracking of
locations of files is eventually consistent, which of course means that
at any given point in time it may be currently inconsistent. That is
fine for tracking locations of files, but not for locking.

When git-annex needs to do an operation that relies on someone else's
copy of a file actually being present, it uses real locking. That
locking is not centralized, instead it relies on the connections between
git repositories. That turns out to be sufficient for git-annex's own
locking needs, but it would not be sufficient to avoid file edit
conflict problems in eg a split brain situation.

-- 
see shy jo


signature.asc
Description: PGP signature

Re: [PATCH] read-cache.c: fix a sparse warning

[Ramsay Jones]

On 17/09/18 17:27, Ramsay Jones wrote:
> 
> 
> On 17/09/18 15:15, Ben Peart wrote:
>>
>>
>> On 9/16/2018 3:17 AM, Eric Sunshine wrote:
>>> On Fri, Sep 14, 2018 at 7:29 PM Ramsay Jones
>>>  wrote:
 At one time, the POSIX standard required the type used to represent
 a thread handle (pthread_t) be an arithmetic type. This is no longer
 the case, probably because different platforms used to regularly
 ignore that requirement.  For example, on cygwin a pthread_t is a
 pointer to a structure (a quite common choice), whereas on Linux it
 is defined as an 'unsigned long int'.

 On cygwin, but not on Linux, 'sparse' currently complains about an
 initialiser used on a 'struct load_index_extensions' variable, whose
 first field may be a pthread handle (if not compiled with NO_PTHREADS
 set).

 In order to fix the warning, move the (conditional) pthread field to
 the end of the struct and change the initialiser to use a NULL, since
 the new (unconditional) first field is a pointer type.

 Signed-off-by: Ramsay Jones 
 ---
 If you need to re-roll your 'bp/read-cache-parallel' branch, could you
 please squash this into the relevant patch (commit a090af334,
 "read-cache: load cache extensions on a worker thread", 2018-09-12).
>>>
>>> The information contained in this commit message is so useful that it
>>> might make sense to plop this patch at the end of the series rather
>>> than merely squashing it in. (Or, if it is squashed, include the above
>>> explanation in the commit message of the appropriate patch.)
>>>
>>
>> I'm happy to squash it in if I end up re-rolling the patch series.  I'll 
>> include the information in the commit message above as a comment so that it 
>> is in close proximity to the code impacted.
>>
> 
> I will be happy with whatever decision you take regarding whether
> to squash this in or add it on top of your series. However, if you
> do squash it in, please don't add the commit message info as a
> comment to the code. No matter how you word it, I can't imagine
> that it would be anything but superfluous - the kind of comment
> that would be removed after review! ;-)
> 
> The information in the commit message about pthread_t, which I
> thought was common knowledge, was not really the main point of
> the argument supporting the patch. (Search for "How do I print
> a pthread_t", for variations on this theme).
> 
> The main point for me: don't conditionally include a field at the
> beginning of a structure and then use an initialiser in a variable
> declaration. (Unless, I suppose, the first unconditional field had
> the same type - but probably not not even then!)
> 
> The fact that the conditionally included field itself had an 'opaque'
> type was just an additional complication.

BTW, I just noticed that you explicitly initialise each field of
that structure (not surprising), so an even simpler solution is
to simply remove the unneeded initialiser! ;-)

ATB,
Ramsay Jones

Re: [PATCH v4 02/23] read-cache.c: remove 'const' from index_has_changes()

[Duy Nguyen]

On Mon, Sep 17, 2018 at 6:25 PM Junio C Hamano  wrote:
>
> Nguyễn Thái Ngọc Duy   writes:
>
> > This function calls do_diff_cache() which eventually needs to set this
> > "istate" to unpack_options->src_index (*). This is an unfortunate fact
> > that unpack_trees() _will_ destroy src_index so we can't really pass a
>
> Wasn't the whole point of introducing src_index and dst_index to
> unpack-trees API so that we can keep the src_index read-only by
> writing the result of merge to a separate in-core dst_index?
>
> What does the above exactly mean by "will destroy src_index"?  Is it
> now fundamental that src_index needs to lack constness, or is it
> something easy to fix?

"destroy" is probably a strong word, but we do modify the src_index, e.g.

 mark_all_ce_unused(o->src_index);
 mark_new_skip_worktree(.., o->src_index...
 move_index_extensions(>result, o->src_index);
 invalidate_ce_path();

all these update the source index. It is possible to fix, but I don't
think it's exactly easy and may even incur some performance cost (e.g.
if we stop modify ce_flags in the src_index, then we need to do one
extra lookup to wherever we store these flags).
-- 
Duy