Re: [PATCH v5 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'

[René Scharfe]

Am 10.05.2018 um 02:04 schrieb Junio C Hamano:
> Taylor Blau  writes:
> 
>> This check we should retain and change the wording to mention '--and',
>> '--or', and '--not' specifically.
> 
> Why are these problematic in the first place?  If I said
> 
>  $ git grep -e first --and -e these
>  $ git grep -e first --and --not -e those
>  $ git grep -e first --or -e those
> 
> I'd expect that the first line of this paragraph will hit, and the
> first hit for these three are "these", "first" and "first",
> respectively.  Most importantly, in the last one, "--or" can be
> omitted and the whole thing stops being "extended", so rejecting
> extended as a whole does not make much sense.
> 
>  $ git grep -v second
>  $ git grep --not -e second
> 
> may hit all lines in this message (except for the obvious two
> lines), but we cannot say which column we found a hit.  I am
> wondering if it is too grave a sin to report "the whole line is what
> satisfied the criteria given" and say the match lies at column #1.
> 
> By doing so, obviously we can sidestep the whole "this mode is
> sometimes incompatible" and "I need to compute a lot to see if the
> given expression is compatible or not" issues.

FWIW, Silver Searcher 2.1.0 does just that:

$ echo a | ag --column -v b
1:a

ripgrep 0.8.1 as well:

$ echo a | rg --column -v b
1:1:a

Side note: This example also shows that --column implies --line-number
for ripgrep because column numbers are mostly useless without line
numbers (https://github.com/BurntSushi/ripgrep/issues/243).  I'm not
sure I'm buying that reasoning.

ack-grep 2.22 seems to have problems with that combination:

$ echo a | ack --column -v b
a

$ echo a | ack -H --column -v b
-
Use of uninitialized value $line_parts[1] in join or string at 
/usr/bin/ack line 653,  line 1.
1::a

René

Re: [PATCH] Implement --first-parent for git rev-list --bisect

[Christian Couder]

Hi Tiago,

On Wed, May 9, 2018 at 11:57 PM, Tiago Botelho  wrote:
> ---

Please add something in the commit message (above the "---") about why
this new feature is useful. For example you could just say that it
will make it possible to implement bisecting on first parents which is
useful for people who don't test all the commits in the feature branch
they merge.

Outside the commit message, so after the "---", you could say that
this patch is based on pu so that it can be on top of
hn/bisect-first-parent, and that this patch is not finished as it
needs some tests. You could also have signaled that by using [RFC
PATCH] in the subject (see the --rfc option of git format-patch).

Thanks,
Christian.

Re: [PATCH v2 0/5] getting rid of most "static struct lock_file"s

[Jeff King]

On Wed, May 09, 2018 at 10:55:34PM +0200, Martin Ågren wrote:

> This is take two of my attempt at making almost all `struct lock_file`s
> non-static. All patches have been equipped with more detailed commit
> messages. The only diff that has changed is patch 3/5, where I now take
> a small step towards gentle error-handling, rather than going in the
> opposite direction.
> 
> Thanks all for the valuable feedback on v1. I could have saved everyone
> some trouble by writing better commit messages from the start, and
> probably also by using `--thread` when formatting the patches...

Indeed, the world would be a more efficient place if we all did
everything perfectly all the time. Sadly, that's not how it works. :)

This version looks good to me. Thanks for modernizing things.

I don't think it's worth re-rolling, but one thing to think about for
future cleanups: you split the patches by touched area, not by
functionality. So the first three patches have a "while we're here..."
that has to explain why dropping the "static" is the right thing over
and over. If you instead did the error-handling fixes independently
first, then you could lump the "static" cleanups together with one
explanation (possibly even just as part of the 4th patch).

-Peff

Re: [PATCH 1/2] packfile: close and free packs upon releasing an object store

[Jeff King]

On Wed, May 09, 2018 at 05:12:10PM -0700, Stefan Beller wrote:

> In d0b59866223 (object-store: close all packs upon clearing the object
> store, 2018-03-23), we made sure to close all packfiles on releasing
> an object store, but we also have to free the memory of the closed packs.

I know we've assumed in a few places that a "struct packed_git" will
never go away. The one that comes to mind is the mru list.

It looks like we'll be OK here:

> diff --git a/object.c b/object.c
> index 66cffaf6e51..3e64a4a26dd 100644
> --- a/object.c
> +++ b/object.c
> @@ -485,6 +485,6 @@ void raw_object_store_clear(struct raw_object_store *o)
>   o->alt_odb_tail = NULL;
>  
>   INIT_LIST_HEAD(>packed_git_mru);
> - close_all_packs(o);
> + close_and_free_packs(o);
>   o->packed_git = NULL;
>  }

because we clear the list above. But it would be dangerous for anybody
else to call close_and_free_packs(). Should that INIT_LIST_HEAD get
moved down into that function?

Probably the same applies to setting NULL here; you're left with a
dangling pointer if you just call close_and_free_packs(). Should
that helper maybe just be a static function in object.c?


Just brainstorming other places where the immutability of "struct
packed_git" might be important:

  - pack-objects keeps a pointer from each object_entry to its
containing packed_git. That's probably OK, as you wouldn't expect to
be able to close the object store in the middle of that operation.

  - the reachability bitmap code holds a pointer to the pack that has a
bitmap. Probably that whole "struct bitmap_index" needs to be part
of the object_store (arguably it should all just be _inside_ the
packed_git, but the current implementation avoids complexity by just
having a single bitmap-per-repo).

-Peff

Re: [PATCH 1/1] commit-graph: fix UX issue when .lock file exists

[Jeff King]

On Wed, May 09, 2018 at 10:53:56AM -0400, Derrick Stolee wrote:

> > Your cover letter is way longer than this description. Should some of
> > that background perhaps go in the commit message?
> 
> I did want a place to include the full die() message in the new behavior,
> but that seemed like overkill for the commit message.

I think it would be fine. In general, it's probably a good idea to err
on the side of more information in the commit message than less. The one
exception is that if your commit message grows very long, make sure it's
organized well. I find there are two "modes" in which I read old commit
messages:

  1. Skimming through log, etc, to try to get the gist of what the
 commit is doing and find out whether it might be related to my
 problem.

  2. Once I've identified it as a problem, I want to know every single
 thing in the mind of the writer that might help me.

(I'm not speaking to this particular message or your messages in general
here; I just didn't want to claim that a blanket "longer is better" is
without limitations).

-Peff

Re: [PATCH 4/5] lock_file: make function-local locks non-static

[Junio C Hamano]

Martin Ågren  writes:

> On 9 May 2018 at 18:19, Duy Nguyen  wrote:
>> On Tue, May 8, 2018 at 8:18 PM, Jeff King  wrote:
>
>>> It should be totally safe. If you look at "struct lock_file", it is now
>>> simply a pointer to a tempfile allocated on the heap (in fact, I thought
>>> about getting rid of lock_file entirely, but the diff is noisy and it
>>> actually has some value as an abstraction over a pure tempfile).
>>
>> Ah.. I did miss that "everything on heap" thing. Sorry for the noise
>> Martin and thanks for clarification Jeff :)
>
> Hey, no problem. In fact, the "noise" as you call it had some signal in
> it: the commit messages should obviously say more about this.

Thanks all for working it out.

Re: What's cooking in git.git (May 2018, #01; Mon, 7)

[Junio C Hamano]

Ben Peart  writes:

> On 5/7/2018 10:58 AM, Junio C Hamano wrote:
>
>> * bp/merge-rename-config (2018-05-04) 3 commits
>>   - merge: pass aggressive when rename detection is turned off
>>   - merge: add merge.renames config setting
>>   - merge: update documentation for {merge,diff}.renameLimit
>>   (this branch uses en/rename-directory-detection-reboot.)
>>
>
> It isn't specifically called out here but is it safe to assume this is
> also headed to next behind en/rename-directory-detection-reboot?

Not really.  A blank "what Junio will do to this topic?" verdict in
any of these topic descriptions simply means I haven't made up my
mind (spanning from "I merely queued it without reading it fully,
only so that I won't lose it" to "I think it is OK but I haven't
understood the implications of the change to the entire system").

Re: [PATCH v3 09/12] get_short_oid / peel_onion: ^{tree} should be tree, not treeish

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

>> Before we had any disambiguation code, resolving X^{tree} really was two
>> independent steps: resolve X, and then peel it to a tree. When we added
>> the disambiguation code, the goal was to provide a hint to the first
>> step in such a way that we could never eliminate any resolutions that
>> the user _might_ have meant. But it's OK to take a situation where every
>> case but one would result in an error, and assume the user meant that
>> case. Sort of a "do no harm" rule.
>>
>> By disambiguating with just a tree and not a tree-ish, that hint is now
>> eliminating possibilities that would have worked in the second step,
>> which violates the rule.
>>
>> Does thinking about it that way make more sense?
>
> Okey, so to rephrase that to make sure I understand it. It would be
> documented as something like this:
>
> When the short SHA-1 X is ambiguous X^{} doesn't mean do the
> peel itself in X any way, rather it means list all those objects
> matching X where a subsequent X^{} wouldn't be an error.

With the understanding that this paragraph is written primarily for
your own enlightenment, I wouldn't complain too much, but if you
meant this to become part of end-user documentation, I have a strong
issue with the verb "list" used here.

X^{} never means to "list" anything (FWIW just X does not mean
to "list" anything, either).  It just means that the user wants to
specify a single object whose object name is X^{}, i.e. the
user expects that X names a single object, that can be peeled to
.

Now, it is an error when (1) X does not specify a single object in
the first place, or (2) the single object cannot be peeled to .

When diagnosing such an error, we would give hints.  The hint would
show possible objects that the user could have meant with X.  The
^{} suffix given to it may be used to limit the hints to
subset of the objects that the user could have meant with X;
e.g. when there is an object of each of type blob, tree, commit, and
tag, whose name begins with , the short and ambiguous prefix
 could mean any of these four objects, but when given with
suffix, e.g. ^{tree}, it makes useless for the hint to include
the blob object, as it can never peel down to a tree object.

If the tag whose name begins with  in this example points
directly to a blob, excluding that tag from the hint would make the
hint more useful.  I do not offhand know what the code does right
now.  I wouldn't call it a bug if such a tag is included in the
hint, but if a change stops such a tag from getting included, I
would call such a change an improvement.

> I.e. X^{commit} will list tags and commits, since both can be peeled
> to reveal a commit, X^{tree} will similarly list tags, commits and
> trees, and X^{blob} will list tags and blobs[1], and X^{tag} will
> only list tags.

Modulo the use of "list", which I have trouble is as it makes it
sound as if listing something is the purpose of the notation, I
think we are on the same page up to this point

I think the best way to explain core.disambiguate to readers is to
make them rethink what I meant with "the user expects that X names a
single object" in the early part of the above response.

Without constraint, Git understood that the user used  to name
any one of the objects of all four types.  With core.disambiguate,
the user can tell Git "when I give potentially ambiguous object name
with a short prefix, assume that only a commit or a tag whose name
begins with the prefix is what I expected the short prefix to name
uniquely", so Git understood that the user wanted to name either a
commit or a tag.  It would still trigger an error as it does not
uniquely name an object (for which an attempt to apply the ^{tree}
peeling would further be made).

[GSoC] Info: Week 02 Blog Post

[Pratik Karki]

Hi,

The week 02 blog post[1] is live. This post is part I out of II and this
time it will be biweekly. The part II of will come in 2-3 days which
will describe the current `git-rebase.sh`.

Do give me feedback.

Thanks to all the awesome Git contributors for this awesome tool. :-)

[1]: https://prertik.github.io/categories/git/

Cheers,
Pratik Karki

Re: [PATCH 2/2] builtin/grep.c: teach '-o', '--only-matching' to 'git-grep'

[Taylor Blau]

On Tue, May 08, 2018 at 01:25:17PM -0400, Jeff King wrote:
> On Sat, May 05, 2018 at 08:49:43AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> > > +test_expect_success 'grep --only-matching --heading' '
> > > + git grep --only-matching --heading --line-number --column mmap file 
> > > >actual &&
> > > + test_cmp expected actual
> > > +'
> > > +
> > >  cat >expected < > >  hello.c
> > >  4:int main(int argc, const char **argv)
> >
> > We should test this a lot more, I think a good way to do that would be
> > to extend this series by first importing GNU grep's -o tests, see
> > http://git.savannah.gnu.org/cgit/grep.git/tree/tests/foad1 they are
> > license-compatible. Then change the grep_test() function to call git
> > grep instead.
>
> I'm trying to figure out what GNU grep's tests are actually checking
> that we don't have. I see:
>
>  - they check that "-i" returns the actual found string in its original
>case. This seems like a subset of finding a non-trivial regex. I.e.,
>"foo.*" should find "foobar". We probably should have a test like
>that.
>
>  - they test multiple hits on the same line, which seems like an
>important and easy-to-screw-up case; we do that, too.

Agree.

>  - they test everything other thing with and without "-i" because those
>are apparently separate code paths in GNU grep, but I don't think
>that applies to us.
>
>  - they test each case with "-b", but we don't have that (we do test
>with "--column", which is good)

Right, I think that we can ignore these groups.

>  - they test with "-n", which we do here (we don't test without, but
>that seems like an unlikely bug, knowing how it is implemented)

Fair, let's leave that as is.

>  - they test with -H, but that is already the default for git-grep

Good, we can ignore this one.

>  - they test with context (-C3) for us. It looks like GNU grep omits
>context lines with "-o", but we show a bunch of blank lines. This is
>I guess a bug (though it seems kind of an odd combination to specify
>in the first place)

I'm torn on what to do here. Currently, with -C3, I get a bunch of lines
like:

  -

Which I think is technically _right_, but I agree that it is certainly
an odd combination of things to give to 'git grep'. I think that we
could either:

  1. Continue outputting blank lines,
  2. Ignore '-C' with '-o', or
  3. die() when given this combination.

I think that (1) is the most "correct" (for some definition), so I'm
inclined to adopt that approach. I suppose that (2) is closer to what
GNU grep offers, and that is the point of this series, so perhaps it
makes sense to pick that instead.

I'll go with (2) for now, but I would appreciate others' thoughts before
sending a subsequent re-roll of this series.

> So I think it probably makes sense to just pick through the list I just
> wrote and write our own tests than to try to import GNU grep's specific
> tests (and there's a ton of other unrelated tests in that file that may
> or may not even run with git-grep).

I agree. Since some of these cases are already covered, and some don't
have analogues, I think that it is most sensible to go through the above
and add _those_, instead of porting the whole test suite over from GNU.

> > It should also be tested with the various grep.patternType options to
> > make sure it works with basic, extended, perl, fixed etc.
>
> Hmm, this code is all external to the actual matching. So unless one of
> those is totally screwing up the regmatch_t return, I'm not sure that's
> accomplishing much (and if one of them is, it's totally broken for
> coloring, "-c", and maybe other features).
>
> We've usually taken a pretty white-box approach to our testing, covering
> things that seem likely to go wrong given the general problem space and
> our implementation. But maybe I'm missing something in particular that
> you think might be tricky.
>
> -Peff

Thanks,
Taylor

Proposal

[Zeliha Omer Faruk]

--
Hello

Greetings to you please i have a business proposal for you contact me
for more detailes asap thanks.

Best Regards,
Miss.Zeliha ömer faruk
Esentepe Mahallesi Büyükdere
Caddesi Kristal Kule Binasi
No:215
Sisli - Istanbul, Turkey

[PATCH v4 04/13] alloc: add repository argument to alloc_blob_node

[Stefan Beller]

This is a small mechanical change; it doesn't change the
implementation to handle repositories other than the_repository yet.
Use a macro to catch callers passing a repository other than
the_repository at compile time.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 alloc.c | 2 +-
 blob.c  | 2 +-
 cache.h | 3 ++-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/alloc.c b/alloc.c
index 12afadfacdd..6c5c376a25a 100644
--- a/alloc.c
+++ b/alloc.c
@@ -49,7 +49,7 @@ static inline void *alloc_node(struct alloc_state *s, size_t 
node_size)
 }
 
 static struct alloc_state blob_state;
-void *alloc_blob_node(void)
+void *alloc_blob_node_the_repository(void)
 {
struct blob *b = alloc_node(_state, sizeof(struct blob));
b->object.type = OBJ_BLOB;
diff --git a/blob.c b/blob.c
index 85c2143f299..9e64f301895 100644
--- a/blob.c
+++ b/blob.c
@@ -9,7 +9,7 @@ struct blob *lookup_blob(const struct object_id *oid)
struct object *obj = lookup_object(oid->hash);
if (!obj)
return create_object(the_repository, oid->hash,
-alloc_blob_node());
+alloc_blob_node(the_repository));
return object_as_type(obj, OBJ_BLOB, 0);
 }
 
diff --git a/cache.h b/cache.h
index 3a4d80e92bf..2258e611275 100644
--- a/cache.h
+++ b/cache.h
@@ -1764,7 +1764,8 @@ int decode_85(char *dst, const char *line, int linelen);
 void encode_85(char *buf, const unsigned char *data, int bytes);
 
 /* alloc.c */
-extern void *alloc_blob_node(void);
+#define alloc_blob_node(r) alloc_blob_node_##r()
+extern void *alloc_blob_node_the_repository(void);
 extern void *alloc_tree_node(void);
 extern void *alloc_commit_node(void);
 extern void *alloc_tag_node(void);
-- 
2.17.0.255.g8bfb7c0704

[PATCH v4 02/13] object: add repository argument to create_object

[Stefan Beller]

Add a repository argument to allow the callers of create_object
to be more specific about which repository to act on. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 blob.c   | 4 +++-
 commit.c | 3 ++-
 object.c | 5 +++--
 object.h | 3 ++-
 tag.c| 3 ++-
 tree.c   | 3 ++-
 6 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/blob.c b/blob.c
index fa2ab4f7a74..85c2143f299 100644
--- a/blob.c
+++ b/blob.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "blob.h"
+#include "repository.h"
 
 const char *blob_type = "blob";
 
@@ -7,7 +8,8 @@ struct blob *lookup_blob(const struct object_id *oid)
 {
struct object *obj = lookup_object(oid->hash);
if (!obj)
-   return create_object(oid->hash, alloc_blob_node());
+   return create_object(the_repository, oid->hash,
+alloc_blob_node());
return object_as_type(obj, OBJ_BLOB, 0);
 }
 
diff --git a/commit.c b/commit.c
index ca474a7c112..9106acf0aad 100644
--- a/commit.c
+++ b/commit.c
@@ -50,7 +50,8 @@ struct commit *lookup_commit(const struct object_id *oid)
 {
struct object *obj = lookup_object(oid->hash);
if (!obj)
-   return create_object(oid->hash, alloc_commit_node());
+   return create_object(the_repository, oid->hash,
+alloc_commit_node());
return object_as_type(obj, OBJ_COMMIT, 0);
 }
 
diff --git a/object.c b/object.c
index f7c624a7ba6..2de029275bc 100644
--- a/object.c
+++ b/object.c
@@ -138,7 +138,7 @@ static void grow_object_hash(void)
the_repository->parsed_objects->obj_hash_size = new_hash_size;
 }
 
-void *create_object(const unsigned char *sha1, void *o)
+void *create_object_the_repository(const unsigned char *sha1, void *o)
 {
struct object *obj = o;
 
@@ -178,7 +178,8 @@ struct object *lookup_unknown_object(const unsigned char 
*sha1)
 {
struct object *obj = lookup_object(sha1);
if (!obj)
-   obj = create_object(sha1, alloc_object_node());
+   obj = create_object(the_repository, sha1,
+   alloc_object_node());
return obj;
 }
 
diff --git a/object.h b/object.h
index cecda7da370..2cb0b241083 100644
--- a/object.h
+++ b/object.h
@@ -93,7 +93,8 @@ extern struct object *get_indexed_object(unsigned int);
  */
 struct object *lookup_object(const unsigned char *sha1);
 
-extern void *create_object(const unsigned char *sha1, void *obj);
+#define create_object(r, s, o) create_object_##r(s, o)
+extern void *create_object_the_repository(const unsigned char *sha1, void 
*obj);
 
 void *object_as_type(struct object *obj, enum object_type type, int quiet);
 
diff --git a/tag.c b/tag.c
index 3d37c1bd251..7150b759d66 100644
--- a/tag.c
+++ b/tag.c
@@ -93,7 +93,8 @@ struct tag *lookup_tag(const struct object_id *oid)
 {
struct object *obj = lookup_object(oid->hash);
if (!obj)
-   return create_object(oid->hash, alloc_tag_node());
+   return create_object(the_repository, oid->hash,
+alloc_tag_node());
return object_as_type(obj, OBJ_TAG, 0);
 }
 
diff --git a/tree.c b/tree.c
index 1c68ea586bd..63730e3fb46 100644
--- a/tree.c
+++ b/tree.c
@@ -196,7 +196,8 @@ struct tree *lookup_tree(const struct object_id *oid)
 {
struct object *obj = lookup_object(oid->hash);
if (!obj)
-   return create_object(oid->hash, alloc_tree_node());
+   return create_object(the_repository, oid->hash,
+alloc_tree_node());
return object_as_type(obj, OBJ_TREE, 0);
 }
 
-- 
2.17.0.255.g8bfb7c0704

[PATCH v4 00/13] object store: alloc

[Stefan Beller]

v4:
* address the memory issues, an interdiff is below.

v3:

* I used the (soon to be renamed?) branch-diff tool to attach a diff below
  between v2 and v3 
  
* fixed comment in patch 1
* correctly free objects and its hashmap in the last patch.
* drop free'ing the commit->util pointer as we do not know where
  it points to.

v2:
* I decided to stick with alloc.c and not migrate it to the mem-pool for now.
  The reasoning for that is that mem-pool.h would introduce some alignment
  waste, which I really did not want to.
* renamed to struct parsed_object_pool;
* free'd the additional memory of trees and commits.
* do not special case the_repository for allocation purposes
* corrected commit messages
* I used the (soon to be renamed?) branch-diff tool to attach a diff below.
  (I still need to get used to that format, I find an interdiff of the
   branches easier to read, but that would not yield the commit messages)



v1:
This applies on top of sb/oid-object-info and is the logical continuum of
the series that it builds on; this brings the object store into more of
Gits code, removing global state, such that reasoning about the state of
the in-memory representation of the repository is easier.

My original plan was to convert lookup_commit_graft as the next series,
which would be similar to lookup_replace_object, as in sb/object-store-replace.
The grafts and shallow mechanism are very close to each other, such that
they need to be converted at the same time, both depending on the
"parsed object store" that is introduced in this commit.

The next series will then convert code in {object/blob/tree/commit/tag}.c
hopefully finishing the lookup_* functions.

I also debated if it is worth converting alloc.c via this patch series
or if it might make more sense to use the new mem-pool by Jameson[1].

I vaguely wonder about the performance impact, as the object allocation
code seemed to be relevant in the past.

[1] https://public-inbox.org/git/20180430153122.243976-1-jam...@microsoft.com/

Any comments welcome,
Thanks,
Stefan

Jonathan Nieder (1):
  object: add repository argument to grow_object_hash

Stefan Beller (12):
  repository: introduce parsed objects field
  object: add repository argument to create_object
  alloc: add repository argument to alloc_blob_node
  alloc: add repository argument to alloc_tree_node
  alloc: add repository argument to alloc_commit_node
  alloc: add repository argument to alloc_tag_node
  alloc: add repository argument to alloc_object_node
  alloc: add repository argument to alloc_report
  alloc: add repository argument to alloc_commit_index
  object: allow grow_object_hash to handle arbitrary repositories
  object: allow create_object to handle arbitrary repositories
  alloc: allow arbitrary repositories for alloc functions

 alloc.c   |  65 --
 alloc.h   |  19 
 blame.c   |   3 +-
 blob.c|   5 +-
 cache.h   |   9 
 commit.c  |  11 -
 commit.h  |   6 +++
 merge-recursive.c |   3 +-
 object.c  | 113 ++
 object.h  |  18 +++-
 repository.c  |   7 +++
 repository.h  |   9 
 tag.c |   9 +++-
 tag.h |   1 +
 tree.c|   4 +-
 15 files changed, 214 insertions(+), 68 deletions(-)
 create mode 100644 alloc.h

-- 
2.17.0.255.g8bfb7c0704

diff --git c/alloc.c w/alloc.c
index 4ecf0f160f4..714df633169 100644
--- c/alloc.c
+++ w/alloc.c
@@ -47,6 +47,8 @@ void clear_alloc_state(struct alloc_state *s)
s->slab_nr--;
free(s->slabs[s->slab_nr]);
}
+
+   FREE_AND_NULL(s->slabs);
 }
 
 static inline void *alloc_node(struct alloc_state *s, size_t node_size)
@@ -110,22 +112,6 @@ void *alloc_commit_node(struct repository *r)
return c;
 }
 
-void release_tree_node(struct tree *t)
-{
-   free(t->buffer);
-}
-
-void release_commit_node(struct commit *c)
-{
-   free_commit_list(c->parents);
-   /* TODO: what about commit->util? */
-}
-
-void release_tag_node(struct tag *t)
-{
-   free(t->tag);
-}
-
 static void report(const char *name, unsigned int count, size_t size)
 {
fprintf(stderr, "%10s: %8u (%"PRIuMAX" kB)\n",
diff --git c/alloc.h w/alloc.h
index 941d71960fb..3e4e828db48 100644
--- c/alloc.h
+++ w/alloc.h
@@ -16,8 +16,4 @@ unsigned int alloc_commit_index(struct repository *r);
 void *allocate_alloc_state(void);
 void clear_alloc_state(struct alloc_state *s);
 
-void release_tree_node(struct tree *t);
-void release_commit_node(struct commit *c);
-void release_tag_node(struct tag *t);
-
 #endif
diff --git c/commit.c w/commit.c
index c3b400d5930..612ccf7b053 100644
--- c/commit.c
+++ w/commit.c
@@ -297,6 +297,13 @@ void free_commit_buffer(struct commit *commit)
}
 }
 
+void release_commit_memory(struct commit *c)
+{
+   free_commit_buffer(c);
+   free_commit_list(c->parents);
+   /* TODO: what about 

[PATCH v4 11/13] object: allow grow_object_hash to handle arbitrary repositories

[Stefan Beller]

Reviewed-by: Jonathan Tan 
Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 object.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/object.c b/object.c
index a365a910859..0fcd6f6df42 100644
--- a/object.c
+++ b/object.c
@@ -116,27 +116,27 @@ struct object *lookup_object(const unsigned char *sha1)
  * power of 2 (but at least 32).  Copy the existing values to the new
  * hash map.
  */
-#define grow_object_hash(r) grow_object_hash_##r()
-static void grow_object_hash_the_repository(void)
+static void grow_object_hash(struct repository *r)
 {
int i;
/*
 * Note that this size must always be power-of-2 to match hash_obj
 * above.
 */
-   int new_hash_size = the_repository->parsed_objects->obj_hash_size < 32 
? 32 : 2 * the_repository->parsed_objects->obj_hash_size;
+   int new_hash_size = r->parsed_objects->obj_hash_size < 32 ? 32 : 2 * 
r->parsed_objects->obj_hash_size;
struct object **new_hash;
 
new_hash = xcalloc(new_hash_size, sizeof(struct object *));
-   for (i = 0; i < the_repository->parsed_objects->obj_hash_size; i++) {
-   struct object *obj = 
the_repository->parsed_objects->obj_hash[i];
+   for (i = 0; i < r->parsed_objects->obj_hash_size; i++) {
+   struct object *obj = r->parsed_objects->obj_hash[i];
+
if (!obj)
continue;
insert_obj_hash(obj, new_hash, new_hash_size);
}
-   free(the_repository->parsed_objects->obj_hash);
-   the_repository->parsed_objects->obj_hash = new_hash;
-   the_repository->parsed_objects->obj_hash_size = new_hash_size;
+   free(r->parsed_objects->obj_hash);
+   r->parsed_objects->obj_hash = new_hash;
+   r->parsed_objects->obj_hash_size = new_hash_size;
 }
 
 void *create_object_the_repository(const unsigned char *sha1, void *o)
-- 
2.17.0.255.g8bfb7c0704

[PATCH v4 08/13] alloc: add repository argument to alloc_object_node

[Stefan Beller]

This is a small mechanical change; it doesn't change the
implementation to handle repositories other than the_repository yet.
Use a macro to catch callers passing a repository other than
the_repository at compile time.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 alloc.c  | 2 +-
 cache.h  | 3 ++-
 object.c | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/alloc.c b/alloc.c
index 290250e3595..f031ce422d9 100644
--- a/alloc.c
+++ b/alloc.c
@@ -73,7 +73,7 @@ void *alloc_tag_node_the_repository(void)
 }
 
 static struct alloc_state object_state;
-void *alloc_object_node(void)
+void *alloc_object_node_the_repository(void)
 {
struct object *obj = alloc_node(_state, sizeof(union 
any_object));
obj->type = OBJ_NONE;
diff --git a/cache.h b/cache.h
index 32f340cde59..2d60359a964 100644
--- a/cache.h
+++ b/cache.h
@@ -1772,7 +1772,8 @@ extern void *alloc_tree_node_the_repository(void);
 extern void *alloc_commit_node_the_repository(void);
 #define alloc_tag_node(r) alloc_tag_node_##r()
 extern void *alloc_tag_node_the_repository(void);
-extern void *alloc_object_node(void);
+#define alloc_object_node(r) alloc_object_node_##r()
+extern void *alloc_object_node_the_repository(void);
 extern void alloc_report(void);
 extern unsigned int alloc_commit_index(void);
 
diff --git a/object.c b/object.c
index 91edc30770c..b8c3f923c51 100644
--- a/object.c
+++ b/object.c
@@ -180,7 +180,7 @@ struct object *lookup_unknown_object(const unsigned char 
*sha1)
struct object *obj = lookup_object(sha1);
if (!obj)
obj = create_object(the_repository, sha1,
-   alloc_object_node());
+   alloc_object_node(the_repository));
return obj;
 }
 
-- 
2.17.0.255.g8bfb7c0704

[PATCH v4 05/13] alloc: add repository argument to alloc_tree_node

[Stefan Beller]

This is a small mechanical change; it doesn't change the
implementation to handle repositories other than the_repository yet.
Use a macro to catch callers passing a repository other than
the_repository at compile time.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 alloc.c | 2 +-
 cache.h | 3 ++-
 tree.c  | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/alloc.c b/alloc.c
index 6c5c376a25a..2c8d1430758 100644
--- a/alloc.c
+++ b/alloc.c
@@ -57,7 +57,7 @@ void *alloc_blob_node_the_repository(void)
 }
 
 static struct alloc_state tree_state;
-void *alloc_tree_node(void)
+void *alloc_tree_node_the_repository(void)
 {
struct tree *t = alloc_node(_state, sizeof(struct tree));
t->object.type = OBJ_TREE;
diff --git a/cache.h b/cache.h
index 2258e611275..1717d07a2c5 100644
--- a/cache.h
+++ b/cache.h
@@ -1766,7 +1766,8 @@ void encode_85(char *buf, const unsigned char *data, int 
bytes);
 /* alloc.c */
 #define alloc_blob_node(r) alloc_blob_node_##r()
 extern void *alloc_blob_node_the_repository(void);
-extern void *alloc_tree_node(void);
+#define alloc_tree_node(r) alloc_tree_node_##r()
+extern void *alloc_tree_node_the_repository(void);
 extern void *alloc_commit_node(void);
 extern void *alloc_tag_node(void);
 extern void *alloc_object_node(void);
diff --git a/tree.c b/tree.c
index 63730e3fb46..58cf19b4fa8 100644
--- a/tree.c
+++ b/tree.c
@@ -197,7 +197,7 @@ struct tree *lookup_tree(const struct object_id *oid)
struct object *obj = lookup_object(oid->hash);
if (!obj)
return create_object(the_repository, oid->hash,
-alloc_tree_node());
+alloc_tree_node(the_repository));
return object_as_type(obj, OBJ_TREE, 0);
 }
 
-- 
2.17.0.255.g8bfb7c0704

[PATCH v4 10/13] alloc: add repository argument to alloc_commit_index

[Stefan Beller]

This is a small mechanical change; it doesn't change the
implementation to handle repositories other than the_repository yet.
Use a macro to catch callers passing a repository other than
the_repository at compile time.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 alloc.c  | 4 ++--
 cache.h  | 3 ++-
 object.c | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/alloc.c b/alloc.c
index 28b85b22144..277dadd221b 100644
--- a/alloc.c
+++ b/alloc.c
@@ -82,7 +82,7 @@ void *alloc_object_node_the_repository(void)
 
 static struct alloc_state commit_state;
 
-unsigned int alloc_commit_index(void)
+unsigned int alloc_commit_index_the_repository(void)
 {
static unsigned int count;
return count++;
@@ -92,7 +92,7 @@ void *alloc_commit_node_the_repository(void)
 {
struct commit *c = alloc_node(_state, sizeof(struct commit));
c->object.type = OBJ_COMMIT;
-   c->index = alloc_commit_index();
+   c->index = alloc_commit_index(the_repository);
return c;
 }
 
diff --git a/cache.h b/cache.h
index 01cc207d218..0e6c5dd5639 100644
--- a/cache.h
+++ b/cache.h
@@ -1776,7 +1776,8 @@ extern void *alloc_tag_node_the_repository(void);
 extern void *alloc_object_node_the_repository(void);
 #define alloc_report(r) alloc_report_##r()
 extern void alloc_report_the_repository(void);
-extern unsigned int alloc_commit_index(void);
+#define alloc_commit_index(r) alloc_commit_index_##r()
+extern unsigned int alloc_commit_index_the_repository(void);
 
 /* pkt-line.c */
 void packet_trace_identity(const char *prog);
diff --git a/object.c b/object.c
index b8c3f923c51..a365a910859 100644
--- a/object.c
+++ b/object.c
@@ -162,7 +162,7 @@ void *object_as_type(struct object *obj, enum object_type 
type, int quiet)
return obj;
else if (obj->type == OBJ_NONE) {
if (type == OBJ_COMMIT)
-   ((struct commit *)obj)->index = alloc_commit_index();
+   ((struct commit *)obj)->index = 
alloc_commit_index(the_repository);
obj->type = type;
return obj;
}
-- 
2.17.0.255.g8bfb7c0704

[PATCH v4 06/13] alloc: add repository argument to alloc_commit_node

[Stefan Beller]

This is a small mechanical change; it doesn't change the
implementation to handle repositories other than the_repository yet.
Use a macro to catch callers passing a repository other than
the_repository at compile time.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 alloc.c   | 2 +-
 blame.c   | 2 +-
 cache.h   | 3 ++-
 commit.c  | 2 +-
 merge-recursive.c | 2 +-
 5 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/alloc.c b/alloc.c
index 2c8d1430758..9e2b897ec1d 100644
--- a/alloc.c
+++ b/alloc.c
@@ -88,7 +88,7 @@ unsigned int alloc_commit_index(void)
return count++;
 }
 
-void *alloc_commit_node(void)
+void *alloc_commit_node_the_repository(void)
 {
struct commit *c = alloc_node(_state, sizeof(struct commit));
c->object.type = OBJ_COMMIT;
diff --git a/blame.c b/blame.c
index dfa24473dc6..ba9b18e7542 100644
--- a/blame.c
+++ b/blame.c
@@ -161,7 +161,7 @@ static struct commit *fake_working_tree_commit(struct 
diff_options *opt,
 
read_cache();
time();
-   commit = alloc_commit_node();
+   commit = alloc_commit_node(the_repository);
commit->object.parsed = 1;
commit->date = now;
parent_tail = >parents;
diff --git a/cache.h b/cache.h
index 1717d07a2c5..bf6e8c87d83 100644
--- a/cache.h
+++ b/cache.h
@@ -1768,7 +1768,8 @@ void encode_85(char *buf, const unsigned char *data, int 
bytes);
 extern void *alloc_blob_node_the_repository(void);
 #define alloc_tree_node(r) alloc_tree_node_##r()
 extern void *alloc_tree_node_the_repository(void);
-extern void *alloc_commit_node(void);
+#define alloc_commit_node(r) alloc_commit_node_##r()
+extern void *alloc_commit_node_the_repository(void);
 extern void *alloc_tag_node(void);
 extern void *alloc_object_node(void);
 extern void alloc_report(void);
diff --git a/commit.c b/commit.c
index 9106acf0aad..a9a43e79bae 100644
--- a/commit.c
+++ b/commit.c
@@ -51,7 +51,7 @@ struct commit *lookup_commit(const struct object_id *oid)
struct object *obj = lookup_object(oid->hash);
if (!obj)
return create_object(the_repository, oid->hash,
-alloc_commit_node());
+alloc_commit_node(the_repository));
return object_as_type(obj, OBJ_COMMIT, 0);
 }
 
diff --git a/merge-recursive.c b/merge-recursive.c
index 0c0d48624da..6dac8908648 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -98,7 +98,7 @@ static struct tree *shift_tree_object(struct tree *one, 
struct tree *two,
 
 static struct commit *make_virtual_commit(struct tree *tree, const char 
*comment)
 {
-   struct commit *commit = alloc_commit_node();
+   struct commit *commit = alloc_commit_node(the_repository);
 
set_merge_remote_desc(commit, comment, (struct object *)commit);
commit->tree = tree;
-- 
2.17.0.255.g8bfb7c0704

[PATCH v4 12/13] object: allow create_object to handle arbitrary repositories

[Stefan Beller]

Reviewed-by: Jonathan Tan 
Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 object.c | 12 ++--
 object.h |  3 +--
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/object.c b/object.c
index 0fcd6f6df42..49b952e9299 100644
--- a/object.c
+++ b/object.c
@@ -139,7 +139,7 @@ static void grow_object_hash(struct repository *r)
r->parsed_objects->obj_hash_size = new_hash_size;
 }
 
-void *create_object_the_repository(const unsigned char *sha1, void *o)
+void *create_object(struct repository *r, const unsigned char *sha1, void *o)
 {
struct object *obj = o;
 
@@ -147,12 +147,12 @@ void *create_object_the_repository(const unsigned char 
*sha1, void *o)
obj->flags = 0;
hashcpy(obj->oid.hash, sha1);
 
-   if (the_repository->parsed_objects->obj_hash_size - 1 <= 
the_repository->parsed_objects->nr_objs * 2)
-   grow_object_hash(the_repository);
+   if (r->parsed_objects->obj_hash_size - 1 <= r->parsed_objects->nr_objs 
* 2)
+   grow_object_hash(r);
 
-   insert_obj_hash(obj, the_repository->parsed_objects->obj_hash,
-   the_repository->parsed_objects->obj_hash_size);
-   the_repository->parsed_objects->nr_objs++;
+   insert_obj_hash(obj, r->parsed_objects->obj_hash,
+   r->parsed_objects->obj_hash_size);
+   r->parsed_objects->nr_objs++;
return obj;
 }
 
diff --git a/object.h b/object.h
index 2cb0b241083..b41d7a3accb 100644
--- a/object.h
+++ b/object.h
@@ -93,8 +93,7 @@ extern struct object *get_indexed_object(unsigned int);
  */
 struct object *lookup_object(const unsigned char *sha1);
 
-#define create_object(r, s, o) create_object_##r(s, o)
-extern void *create_object_the_repository(const unsigned char *sha1, void 
*obj);
+extern void *create_object(struct repository *r, const unsigned char *sha1, 
void *obj);
 
 void *object_as_type(struct object *obj, enum object_type type, int quiet);
 
-- 
2.17.0.255.g8bfb7c0704

[PATCH v4 07/13] alloc: add repository argument to alloc_tag_node

[Stefan Beller]

This is a small mechanical change; it doesn't change the
implementation to handle repositories other than the_repository yet.
Use a macro to catch callers passing a repository other than
the_repository at compile time.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 alloc.c | 2 +-
 cache.h | 3 ++-
 tag.c   | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/alloc.c b/alloc.c
index 9e2b897ec1d..290250e3595 100644
--- a/alloc.c
+++ b/alloc.c
@@ -65,7 +65,7 @@ void *alloc_tree_node_the_repository(void)
 }
 
 static struct alloc_state tag_state;
-void *alloc_tag_node(void)
+void *alloc_tag_node_the_repository(void)
 {
struct tag *t = alloc_node(_state, sizeof(struct tag));
t->object.type = OBJ_TAG;
diff --git a/cache.h b/cache.h
index bf6e8c87d83..32f340cde59 100644
--- a/cache.h
+++ b/cache.h
@@ -1770,7 +1770,8 @@ extern void *alloc_blob_node_the_repository(void);
 extern void *alloc_tree_node_the_repository(void);
 #define alloc_commit_node(r) alloc_commit_node_##r()
 extern void *alloc_commit_node_the_repository(void);
-extern void *alloc_tag_node(void);
+#define alloc_tag_node(r) alloc_tag_node_##r()
+extern void *alloc_tag_node_the_repository(void);
 extern void *alloc_object_node(void);
 extern void alloc_report(void);
 extern unsigned int alloc_commit_index(void);
diff --git a/tag.c b/tag.c
index 7150b759d66..02ef4eaafc0 100644
--- a/tag.c
+++ b/tag.c
@@ -94,7 +94,7 @@ struct tag *lookup_tag(const struct object_id *oid)
struct object *obj = lookup_object(oid->hash);
if (!obj)
return create_object(the_repository, oid->hash,
-alloc_tag_node());
+alloc_tag_node(the_repository));
return object_as_type(obj, OBJ_TAG, 0);
 }
 
-- 
2.17.0.255.g8bfb7c0704

[PATCH v4 13/13] alloc: allow arbitrary repositories for alloc functions

[Stefan Beller]

We have to convert all of the alloc functions at once, because alloc_report
uses a funky macro for reporting. It is better for the sake of mechanical
conversion to convert multiple functions at once rather than changing the
structure of the reporting function.

We record all memory allocation in alloc.c, and free them in
clear_alloc_state, which is called for all repositories except
the_repository.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 alloc.c   | 65 ++-
 alloc.h   | 19 ++
 blame.c   |  1 +
 blob.c|  1 +
 cache.h   | 16 
 commit.c  |  8 ++
 commit.h  |  6 +
 merge-recursive.c |  1 +
 object.c  | 42 --
 object.h  |  8 ++
 tag.c |  6 +
 tag.h |  1 +
 tree.c|  1 +
 13 files changed, 133 insertions(+), 42 deletions(-)
 create mode 100644 alloc.h

diff --git a/alloc.c b/alloc.c
index 277dadd221b..714df633169 100644
--- a/alloc.c
+++ b/alloc.c
@@ -4,8 +4,7 @@
  * Copyright (C) 2006 Linus Torvalds
  *
  * The standard malloc/free wastes too much space for objects, partly because
- * it maintains all the allocation infrastructure (which isn't needed, since
- * we never free an object descriptor anyway), but even more because it ends
+ * it maintains all the allocation infrastructure, but even more because it 
ends
  * up with maximal alignment because it doesn't know what the object alignment
  * for the new allocation is.
  */
@@ -15,6 +14,7 @@
 #include "tree.h"
 #include "commit.h"
 #include "tag.h"
+#include "alloc.h"
 
 #define BLOCKING 1024
 
@@ -30,8 +30,27 @@ struct alloc_state {
int count; /* total number of nodes allocated */
int nr;/* number of nodes left in current allocation */
void *p;   /* first free node in current allocation */
+
+   /* bookkeeping of allocations */
+   void **slabs;
+   int slab_nr, slab_alloc;
 };
 
+void *allocate_alloc_state(void)
+{
+   return xcalloc(1, sizeof(struct alloc_state));
+}
+
+void clear_alloc_state(struct alloc_state *s)
+{
+   while (s->slab_nr > 0) {
+   s->slab_nr--;
+   free(s->slabs[s->slab_nr]);
+   }
+
+   FREE_AND_NULL(s->slabs);
+}
+
 static inline void *alloc_node(struct alloc_state *s, size_t node_size)
 {
void *ret;
@@ -39,60 +58,57 @@ static inline void *alloc_node(struct alloc_state *s, 
size_t node_size)
if (!s->nr) {
s->nr = BLOCKING;
s->p = xmalloc(BLOCKING * node_size);
+
+   ALLOC_GROW(s->slabs, s->slab_nr + 1, s->slab_alloc);
+   s->slabs[s->slab_nr++] = s->p;
}
s->nr--;
s->count++;
ret = s->p;
s->p = (char *)s->p + node_size;
memset(ret, 0, node_size);
+
return ret;
 }
 
-static struct alloc_state blob_state;
-void *alloc_blob_node_the_repository(void)
+void *alloc_blob_node(struct repository *r)
 {
-   struct blob *b = alloc_node(_state, sizeof(struct blob));
+   struct blob *b = alloc_node(r->parsed_objects->blob_state, 
sizeof(struct blob));
b->object.type = OBJ_BLOB;
return b;
 }
 
-static struct alloc_state tree_state;
-void *alloc_tree_node_the_repository(void)
+void *alloc_tree_node(struct repository *r)
 {
-   struct tree *t = alloc_node(_state, sizeof(struct tree));
+   struct tree *t = alloc_node(r->parsed_objects->tree_state, 
sizeof(struct tree));
t->object.type = OBJ_TREE;
return t;
 }
 
-static struct alloc_state tag_state;
-void *alloc_tag_node_the_repository(void)
+void *alloc_tag_node(struct repository *r)
 {
-   struct tag *t = alloc_node(_state, sizeof(struct tag));
+   struct tag *t = alloc_node(r->parsed_objects->tag_state, sizeof(struct 
tag));
t->object.type = OBJ_TAG;
return t;
 }
 
-static struct alloc_state object_state;
-void *alloc_object_node_the_repository(void)
+void *alloc_object_node(struct repository *r)
 {
-   struct object *obj = alloc_node(_state, sizeof(union 
any_object));
+   struct object *obj = alloc_node(r->parsed_objects->object_state, 
sizeof(union any_object));
obj->type = OBJ_NONE;
return obj;
 }
 
-static struct alloc_state commit_state;
-
-unsigned int alloc_commit_index_the_repository(void)
+unsigned int alloc_commit_index(struct repository *r)
 {
-   static unsigned int count;
-   return count++;
+   return r->parsed_objects->commit_count++;
 }
 
-void *alloc_commit_node_the_repository(void)
+void *alloc_commit_node(struct repository *r)
 {
-   struct commit *c = alloc_node(_state, sizeof(struct commit));
+   struct commit *c = alloc_node(r->parsed_objects->commit_state, 
sizeof(struct commit));
c->object.type = OBJ_COMMIT;
-   c->index = alloc_commit_index(the_repository);
+ 

[PATCH v4 09/13] alloc: add repository argument to alloc_report

[Stefan Beller]

This is a small mechanical change; it doesn't change the
implementation to handle repositories other than the_repository yet.
Use a macro to catch callers passing a repository other than
the_repository at compile time.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 alloc.c | 2 +-
 cache.h | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/alloc.c b/alloc.c
index f031ce422d9..28b85b22144 100644
--- a/alloc.c
+++ b/alloc.c
@@ -105,7 +105,7 @@ static void report(const char *name, unsigned int count, 
size_t size)
 #define REPORT(name, type) \
 report(#name, name##_state.count, name##_state.count * sizeof(type) >> 10)
 
-void alloc_report(void)
+void alloc_report_the_repository(void)
 {
REPORT(blob, struct blob);
REPORT(tree, struct tree);
diff --git a/cache.h b/cache.h
index 2d60359a964..01cc207d218 100644
--- a/cache.h
+++ b/cache.h
@@ -1774,7 +1774,8 @@ extern void *alloc_commit_node_the_repository(void);
 extern void *alloc_tag_node_the_repository(void);
 #define alloc_object_node(r) alloc_object_node_##r()
 extern void *alloc_object_node_the_repository(void);
-extern void alloc_report(void);
+#define alloc_report(r) alloc_report_##r()
+extern void alloc_report_the_repository(void);
 extern unsigned int alloc_commit_index(void);
 
 /* pkt-line.c */
-- 
2.17.0.255.g8bfb7c0704

[PATCH v4 03/13] object: add repository argument to grow_object_hash

[Stefan Beller]

From: Jonathan Nieder 

Add a repository argument to allow the caller of grow_object_hash to
be more specific about which repository to handle. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 object.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/object.c b/object.c
index 2de029275bc..91edc30770c 100644
--- a/object.c
+++ b/object.c
@@ -116,7 +116,8 @@ struct object *lookup_object(const unsigned char *sha1)
  * power of 2 (but at least 32).  Copy the existing values to the new
  * hash map.
  */
-static void grow_object_hash(void)
+#define grow_object_hash(r) grow_object_hash_##r()
+static void grow_object_hash_the_repository(void)
 {
int i;
/*
@@ -147,7 +148,7 @@ void *create_object_the_repository(const unsigned char 
*sha1, void *o)
hashcpy(obj->oid.hash, sha1);
 
if (the_repository->parsed_objects->obj_hash_size - 1 <= 
the_repository->parsed_objects->nr_objs * 2)
-   grow_object_hash();
+   grow_object_hash(the_repository);
 
insert_obj_hash(obj, the_repository->parsed_objects->obj_hash,
the_repository->parsed_objects->obj_hash_size);
-- 
2.17.0.255.g8bfb7c0704

[PATCH v4 01/13] repository: introduce parsed objects field

[Stefan Beller]

Convert the existing global cache for parsed objects (obj_hash) into
repository-specific parsed object caches. Existing code that uses
obj_hash are modified to use the parsed object cache of
the_repository; future patches will use the parsed object caches of
other repositories.

Another future use case for a pool of objects is ease of memory management
in revision walking: If we can free the rev-list related memory early in
pack-objects (e.g. part of repack operation) then it could lower memory
pressure significantly when running on large repos. While this has been
discussed on the mailing list lately, this series doesn't implement this.

Signed-off-by: Stefan Beller 
---
 object.c | 63 +---
 object.h |  8 +++
 repository.c |  7 ++
 repository.h |  9 
 4 files changed, 64 insertions(+), 23 deletions(-)

diff --git a/object.c b/object.c
index 5044d08e96c..f7c624a7ba6 100644
--- a/object.c
+++ b/object.c
@@ -8,17 +8,14 @@
 #include "object-store.h"
 #include "packfile.h"
 
-static struct object **obj_hash;
-static int nr_objs, obj_hash_size;
-
 unsigned int get_max_object_index(void)
 {
-   return obj_hash_size;
+   return the_repository->parsed_objects->obj_hash_size;
 }
 
 struct object *get_indexed_object(unsigned int idx)
 {
-   return obj_hash[idx];
+   return the_repository->parsed_objects->obj_hash[idx];
 }
 
 static const char *object_type_strings[] = {
@@ -90,15 +87,16 @@ struct object *lookup_object(const unsigned char *sha1)
unsigned int i, first;
struct object *obj;
 
-   if (!obj_hash)
+   if (!the_repository->parsed_objects->obj_hash)
return NULL;
 
-   first = i = hash_obj(sha1, obj_hash_size);
-   while ((obj = obj_hash[i]) != NULL) {
+   first = i = hash_obj(sha1,
+the_repository->parsed_objects->obj_hash_size);
+   while ((obj = the_repository->parsed_objects->obj_hash[i]) != NULL) {
if (!hashcmp(sha1, obj->oid.hash))
break;
i++;
-   if (i == obj_hash_size)
+   if (i == the_repository->parsed_objects->obj_hash_size)
i = 0;
}
if (obj && i != first) {
@@ -107,7 +105,8 @@ struct object *lookup_object(const unsigned char *sha1)
 * that we do not need to walk the hash table the next
 * time we look for it.
 */
-   SWAP(obj_hash[i], obj_hash[first]);
+   SWAP(the_repository->parsed_objects->obj_hash[i],
+the_repository->parsed_objects->obj_hash[first]);
}
return obj;
 }
@@ -124,19 +123,19 @@ static void grow_object_hash(void)
 * Note that this size must always be power-of-2 to match hash_obj
 * above.
 */
-   int new_hash_size = obj_hash_size < 32 ? 32 : 2 * obj_hash_size;
+   int new_hash_size = the_repository->parsed_objects->obj_hash_size < 32 
? 32 : 2 * the_repository->parsed_objects->obj_hash_size;
struct object **new_hash;
 
new_hash = xcalloc(new_hash_size, sizeof(struct object *));
-   for (i = 0; i < obj_hash_size; i++) {
-   struct object *obj = obj_hash[i];
+   for (i = 0; i < the_repository->parsed_objects->obj_hash_size; i++) {
+   struct object *obj = 
the_repository->parsed_objects->obj_hash[i];
if (!obj)
continue;
insert_obj_hash(obj, new_hash, new_hash_size);
}
-   free(obj_hash);
-   obj_hash = new_hash;
-   obj_hash_size = new_hash_size;
+   free(the_repository->parsed_objects->obj_hash);
+   the_repository->parsed_objects->obj_hash = new_hash;
+   the_repository->parsed_objects->obj_hash_size = new_hash_size;
 }
 
 void *create_object(const unsigned char *sha1, void *o)
@@ -147,11 +146,12 @@ void *create_object(const unsigned char *sha1, void *o)
obj->flags = 0;
hashcpy(obj->oid.hash, sha1);
 
-   if (obj_hash_size - 1 <= nr_objs * 2)
+   if (the_repository->parsed_objects->obj_hash_size - 1 <= 
the_repository->parsed_objects->nr_objs * 2)
grow_object_hash();
 
-   insert_obj_hash(obj, obj_hash, obj_hash_size);
-   nr_objs++;
+   insert_obj_hash(obj, the_repository->parsed_objects->obj_hash,
+   the_repository->parsed_objects->obj_hash_size);
+   the_repository->parsed_objects->nr_objs++;
return obj;
 }
 
@@ -431,8 +431,8 @@ void clear_object_flags(unsigned flags)
 {
int i;
 
-   for (i=0; i < obj_hash_size; i++) {
-   struct object *obj = obj_hash[i];
+   for (i=0; i < the_repository->parsed_objects->obj_hash_size; i++) {
+   struct object *obj = 
the_repository->parsed_objects->obj_hash[i];
if (obj)
obj->flags &= ~flags;
}
@@ 

[PATCH 2/2] packfile.h: remove all extern keywords

[Stefan Beller]

Per our coding guidelines we prefer to only use the extern keyword
when needed.

Signed-off-by: Stefan Beller 
---
 packfile.h | 80 +++---
 1 file changed, 40 insertions(+), 40 deletions(-)

diff --git a/packfile.h b/packfile.h
index cdab0557979..eb3b1154501 100644
--- a/packfile.h
+++ b/packfile.h
@@ -10,32 +10,32 @@
  *
  * Example: odb_pack_name(out, sha1, "idx") => 
".git/objects/pack/pack-1234..idx"
  */
-extern char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1, 
const char *ext);
+char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1, const char 
*ext);
 
 /*
  * Return the name of the (local) packfile with the specified sha1 in
  * its name.  The return value is a pointer to memory that is
  * overwritten each time this function is called.
  */
-extern char *sha1_pack_name(const unsigned char *sha1);
+char *sha1_pack_name(const unsigned char *sha1);
 
 /*
  * Return the name of the (local) pack index file with the specified
  * sha1 in its name.  The return value is a pointer to memory that is
  * overwritten each time this function is called.
  */
-extern char *sha1_pack_index_name(const unsigned char *sha1);
+char *sha1_pack_index_name(const unsigned char *sha1);
 
-extern struct packed_git *parse_pack_index(unsigned char *sha1, const char 
*idx_path);
+struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);
 
 /* A hook to report invalid files in pack directory */
 #define PACKDIR_FILE_PACK 1
 #define PACKDIR_FILE_IDX 2
 #define PACKDIR_FILE_GARBAGE 4
-extern void (*report_garbage)(unsigned seen_bits, const char *path);
+void (*report_garbage)(unsigned seen_bits, const char *path);
 
-extern void reprepare_packed_git(struct repository *r);
-extern void install_packed_git(struct repository *r, struct packed_git *pack);
+void reprepare_packed_git(struct repository *r);
+void install_packed_git(struct repository *r, struct packed_git *pack);
 
 struct packed_git *get_packed_git(struct repository *r);
 struct list_head *get_packed_git_mru(struct repository *r);
@@ -46,31 +46,31 @@ struct list_head *get_packed_git_mru(struct repository *r);
  */
 unsigned long approximate_object_count(void);
 
-extern struct packed_git *find_sha1_pack(const unsigned char *sha1,
+struct packed_git *find_sha1_pack(const unsigned char *sha1,
 struct packed_git *packs);
 
-extern void pack_report(void);
+void pack_report(void);
 
 /*
  * mmap the index file for the specified packfile (if it is not
  * already mmapped).  Return 0 on success.
  */
-extern int open_pack_index(struct packed_git *);
+int open_pack_index(struct packed_git *);
 
 /*
  * munmap the index file for the specified packfile (if it is
  * currently mmapped).
  */
-extern void close_pack_index(struct packed_git *);
+void close_pack_index(struct packed_git *);
 
-extern unsigned char *use_pack(struct packed_git *, struct pack_window **, 
off_t, unsigned long *);
-extern void close_pack_windows(struct packed_git *);
-extern void close_pack(struct packed_git *);
-extern void close_all_packs(struct raw_object_store *o);
-extern void close_and_free_packs(struct raw_object_store *o);
-extern void unuse_pack(struct pack_window **);
-extern void clear_delta_base_cache(void);
-extern struct packed_git *add_packed_git(const char *path, size_t path_len, 
int local);
+unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, 
unsigned long *);
+void close_pack_windows(struct packed_git *);
+void close_pack(struct packed_git *);
+void close_all_packs(struct raw_object_store *o);
+void close_and_free_packs(struct raw_object_store *o);
+void unuse_pack(struct pack_window **);
+void clear_delta_base_cache(void);
+struct packed_git *add_packed_git(const char *path, size_t path_len, int 
local);
 
 /*
  * Make sure that a pointer access into an mmap'd index file is within bounds,
@@ -80,7 +80,7 @@ extern struct packed_git *add_packed_git(const char *path, 
size_t path_len, int
  * (like the 64-bit extended offset table), as we compare the size to the
  * fixed-length parts when we open the file.
  */
-extern void check_pack_index_ptr(const struct packed_git *p, const void *ptr);
+void check_pack_index_ptr(const struct packed_git *p, const void *ptr);
 
 /*
  * Perform binary search on a pack-index for a given oid. Packfile is expected 
to
@@ -96,51 +96,51 @@ int bsearch_pack(const struct object_id *oid, const struct 
packed_git *p, uint32
  * at the SHA-1 within the mmapped index.  Return NULL if there is an
  * error.
  */
-extern const unsigned char *nth_packed_object_sha1(struct packed_git *, 
uint32_t n);
+const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t n);
 /*
  * Like nth_packed_object_sha1, but write the data into the object specified by
  * the the first argument.  Returns the first argument on success, and NULL on
  * error.
  */
-extern const struct object_id 

[PATCH 1/2] packfile: close and free packs upon releasing an object store

[Stefan Beller]

In d0b59866223 (object-store: close all packs upon clearing the object
store, 2018-03-23), we made sure to close all packfiles on releasing
an object store, but we also have to free the memory of the closed packs.

Signed-off-by: Stefan Beller 
---
 object.c   |  2 +-
 packfile.c | 11 +++
 packfile.h |  1 +
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/object.c b/object.c
index 66cffaf6e51..3e64a4a26dd 100644
--- a/object.c
+++ b/object.c
@@ -485,6 +485,6 @@ void raw_object_store_clear(struct raw_object_store *o)
o->alt_odb_tail = NULL;
 
INIT_LIST_HEAD(>packed_git_mru);
-   close_all_packs(o);
+   close_and_free_packs(o);
o->packed_git = NULL;
 }
diff --git a/packfile.c b/packfile.c
index 6c3ddc3c31d..ba4baa55531 100644
--- a/packfile.c
+++ b/packfile.c
@@ -322,6 +322,17 @@ void close_all_packs(struct raw_object_store *o)
close_pack(p);
 }
 
+void close_and_free_packs(struct raw_object_store *o)
+{
+   close_all_packs(o);
+
+   while (o->packed_git) {
+   struct packed_git *p = o->packed_git;
+   o->packed_git = p->next;
+   free(p);
+   }
+}
+
 /*
  * The LRU pack is the one with the oldest MRU window, preferring packs
  * with no used windows, or the oldest mtime if it has no windows allocated.
diff --git a/packfile.h b/packfile.h
index 9c2f8859945..cdab0557979 100644
--- a/packfile.h
+++ b/packfile.h
@@ -67,6 +67,7 @@ extern unsigned char *use_pack(struct packed_git *, struct 
pack_window **, off_t
 extern void close_pack_windows(struct packed_git *);
 extern void close_pack(struct packed_git *);
 extern void close_all_packs(struct raw_object_store *o);
+extern void close_and_free_packs(struct raw_object_store *o);
 extern void unuse_pack(struct pack_window **);
 extern void clear_delta_base_cache(void);
 extern struct packed_git *add_packed_git(const char *path, size_t path_len, 
int local);
-- 
2.17.0.255.g8bfb7c0704

Re: [PATCH v5 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'

[Junio C Hamano]

Taylor Blau  writes:

> This check we should retain and change the wording to mention '--and',
> '--or', and '--not' specifically.

Why are these problematic in the first place?  If I said

$ git grep -e first --and -e these
$ git grep -e first --and --not -e those
$ git grep -e first --or -e those

I'd expect that the first line of this paragraph will hit, and the
first hit for these three are "these", "first" and "first",
respectively.  Most importantly, in the last one, "--or" can be
omitted and the whole thing stops being "extended", so rejecting
extended as a whole does not make much sense.

$ git grep -v second
$ git grep --not -e second

may hit all lines in this message (except for the obvious two
lines), but we cannot say which column we found a hit.  I am
wondering if it is too grave a sin to report "the whole line is what
satisfied the criteria given" and say the match lies at column #1.

By doing so, obviously we can sidestep the whole "this mode is
sometimes incompatible" and "I need to compute a lot to see if the
given expression is compatible or not" issues.

Proposal

[Zeliha Omer Faruk]

--
Hello

Greetings to you please i have a business proposal for you contact me
for more detailes asap thanks.

Best Regards,
Miss.Zeliha ömer faruk
Esentepe Mahallesi Büyükdere
Caddesi Kristal Kule Binasi
No:215
Sisli - Istanbul, Turkey

Re: [PATCH v5 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'

[Taylor Blau]

On Wed, May 09, 2018 at 07:26:57PM +0200, Martin Ågren wrote:
> On 9 May 2018 at 12:41, Phillip Wood  wrote:
> > On 09/05/18 03:13, Taylor Blau wrote:
> >>
> >>   +--column::
> >> +   Prefix the 1-indexed byte-offset of the first match on non-context
> >> lines. This
> >> +   option is incompatible with '--invert-match', and extended
> >> expressions.
> >> +
> >
> >
> > Sorry to be fussy, but while this is clearer I think to could be improved to
> > make it clear that it is the offset from the start of the matching line.
> > Also the mention of 'extended expressions' made me think of 'grep -E' but I
> > think (correct me if I'm wrong) you mean the boolean options '--and',
> > '--not' and '--or'. The man page only uses the word extended when talking
> > about extended regexes. I think something like
> >
> > Print the 1-indexed byte-offset of the first match from the start of the
> > matching line. This option is incompatible with '--invert-match', '--and',
> > '--not' and '--or'.
> >
> > would be clearer
>
> >> +   if (opt->columnnum && opt->extended)
> >> +   die(_("--column and extended expressions cannot be 
> >> combined"));
> >> +
>
> Just so it doesn't get missed: Phillip's comment (which I agree with)
> about "extended" would apply here as well. This would work fine, no?

This check we should retain and change the wording to mention '--and',
'--or', and '--not' specifically.

> One thing to notice is that dying for `--column --not` in combination
> with patch 7/7 makes git-jump unable to handle `--not` (and friends).
> That would be a regression? I suppose git-jump could use a special
> `--maybe-column` which would be "please use --column, unless I give you
> something that won't play well with it". Or --column could do that kind
> of falling back on its own. Or, git-jump could scan the arguments to
> decide whether to use `--column` or not. Hmm... Tricky. :-/

Agree that this is tricky. I don't think that --maybe-column is a
direction that we should take for the reasons I outlined in the cover
letter. Like I said, there are cases under an extended grammar where we
can and cannot display meaningful column offsets.

With regards to regressing 'git-jump', I feel as if 'git-jump --not' is
an uncommon-enough case that I would be comfortable with the tradeoff.
If a caller _is_ using '--not' in 'git-jump', they can reconfigure
'jump.grepCmd' to work around this issue.

Perhaps this is worth warning about in 'git-jump'? Peff, what do you
think?


Thanks,
Taylor

Re: [PATCH v5 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'

[Taylor Blau]

On Wed, May 09, 2018 at 11:41:02AM +0100, Phillip Wood wrote:
> Hi Taylor
>
> On 09/05/18 03:13, Taylor Blau wrote:
> > Teach 'git-grep(1)' a new option, '--column', to show the column
> > number of the first match on a non-context line. This makes it possible
> > to teach 'contrib/git-jump/git-jump' how to seek to the first matching
> > position of a grep match in your editor, and allows similar additional
> > scripting capabilities.
> >
> > For example:
> >
> >$ git grep -n --column foo | head -n3
> >.clang-format:51:14:# myFunction(foo, bar, baz);
> >.clang-format:64:7:# int foo();
> >.clang-format:75:8:# void foo()
> >
> > Signed-off-by: Taylor Blau 
> > ---
> >   Documentation/git-grep.txt |  6 +-
> >   builtin/grep.c |  4 
> >   grep.c |  3 +++
> >   t/t7810-grep.sh| 32 
> >   4 files changed, 44 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> > index 18b494731f..75f1561112 100644
> > --- a/Documentation/git-grep.txt
> > +++ b/Documentation/git-grep.txt
> > @@ -13,7 +13,7 @@ SYNOPSIS
> >[-v | --invert-match] [-h|-H] [--full-name]
> >[-E | --extended-regexp] [-G | --basic-regexp]
> >[-P | --perl-regexp]
> > -  [-F | --fixed-strings] [-n | --line-number]
> > +  [-F | --fixed-strings] [-n | --line-number] [--column]
> >[-l | --files-with-matches] [-L | --files-without-match]
> >[(-O | --open-files-in-pager) []]
> >[-z | --null]
> > @@ -169,6 +169,10 @@ providing this option will cause it to die.
> >   --line-number::
> > Prefix the line number to matching lines.
> > +--column::
> > +   Prefix the 1-indexed byte-offset of the first match on non-context 
> > lines. This
> > +   option is incompatible with '--invert-match', and extended expressions.
> > +
>
> Sorry to be fussy, but while this is clearer I think to could be improved to
> make it clear that it is the offset from the start of the matching line.
> Also the mention of 'extended expressions' made me think of 'grep -E' but I
> think (correct me if I'm wrong) you mean the boolean options '--and',
> '--not' and '--or'. The man page only uses the word extended when talking
> about extended regexes. I think something like
>
> Print the 1-indexed byte-offset of the first match from the start of the
> matching line. This option is incompatible with '--invert-match', '--and',
> '--not' and '--or'.
>
> would be clearer

I agree, and would be happy to change it as-such. I think that there is
some pending discussion about regressing 'git-jump' no longer supporting
'--not', so I'll wait for that to resolve before resending this patch.

Thanks,
Taylor

Re: [PATCH v5 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'

[Taylor Blau]

On Wed, May 09, 2018 at 06:17:20PM +0200, Duy Nguyen wrote:
> On Wed, May 9, 2018 at 4:13 AM, Taylor Blau  wrote:
> > diff --git a/builtin/grep.c b/builtin/grep.c
> > index 5f32d2ce84..f9f516dfc4 100644
> > --- a/builtin/grep.c
> > +++ b/builtin/grep.c
> > @@ -829,6 +829,7 @@ int cmd_grep(int argc, const char **argv, const char 
> > *prefix)
> > GREP_PATTERN_TYPE_PCRE),
> > OPT_GROUP(""),
> > OPT_BOOL('n', "line-number", , N_("show line 
> > numbers")),
> > +   OPT_BOOL(0, "column", , N_("show column 
> > number of first match")),
>
> Two things to consider:
>
> - do we ever want columnar output in git-grep? Something like "git
> grep --column -l" could make sense (if you don't have very large
> worktree). --column is currently used for column output in git-branch,
> git-tag and git-status, which makes me think maybe we should reserve
> "--column" for that purpose and use another name here, even if we
> don't ever want column output in git-grep, for consistency.

I think that this is a valid concern. I had a similar thought when
adding 'git config --color' (as a new type specifier) that we might be
squatting on '--color' and instead opted for '[--type=]'.

I don't feel that the tradeoff between '--column' as a good name and the
concern that we _might_ want to output in a columnar format in 'git
grep' someday warrants the change.

> - If this is to help git-jump only and rarely manually specified on
> command line, you could add the flag PARSE_OPT_NOCOMPLETE to hide it
> from "git grep --" completion. You would need to use OPT_BOOL_F()
> instead of OPT_BOOL() in order to add extra flags.

I believe that this option is worth auto-completing. Its primarily
motivated for use within 'git-jump', but I feel as if it would be
useful for other callers, as well.

Thanks,
Taylor

[PATCH 1/2] object.c: free replace map in raw_object_store_clear

[Stefan Beller]

The replace map for objects was missed to free in the object store in
the conversion of 174774cd519 (Merge branch 'sb/object-store-replace',
2018-05-08)

Signed-off-by: Stefan Beller 
---
 object.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/object.c b/object.c
index 9d5b10d5a20..ff28f90c5ef 100644
--- a/object.c
+++ b/object.c
@@ -497,6 +497,7 @@ void raw_object_store_clear(struct raw_object_store *o)
 {
FREE_AND_NULL(o->objectdir);
FREE_AND_NULL(o->alternate_db);
+   FREE_AND_NULL(o->replace_map);
 
free_alt_odbs(o);
o->alt_odb_tail = NULL;
-- 
2.17.0.255.g8bfb7c0704

[PATCH 2/2] replace-object.c: remove the_repository from prepare_replace_object

[Stefan Beller]

This was missed in 5982da9d2ce (replace-object: allow
prepare_replace_object to handle arbitrary repositories, 2018-04-11)

Technically the code works correctly as the replace_map is the same
size in different repositories, however it is hard to read. So convert
the code to the familiar pattern of dereferencing the pointer that we
assign in the sizeof itself.

Signed-off-by: Stefan Beller 
---
 replace_object.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/replace_object.c b/replace_object.c
index 246b98cd4f1..801b5c16789 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -37,7 +37,7 @@ static void prepare_replace_object(struct repository *r)
return;
 
r->objects->replace_map =
-   xmalloc(sizeof(*the_repository->objects->replace_map));
+   xmalloc(sizeof(*r->objects->replace_map));
oidmap_init(r->objects->replace_map, 0);
 
for_each_replace_ref(r, register_replace_ref, NULL);
-- 
2.17.0.255.g8bfb7c0704

Proposal

[Zeliha Omer Faruk]

--
Hello

Greetings to you please i have a business proposal for you contact me
for more detailes asap thanks.

Best Regards,
Miss.Zeliha ömer faruk
Esentepe Mahallesi Büyükdere
Caddesi Kristal Kule Binasi
No:215
Sisli - Istanbul, Turkey

RE: [Best Practices Request] clean/smudge configuration

[Randall S. Becker]

On May 9, 2018 6:39 PM, Bryan Turner wrote
> 
> On Wed, May 9, 2018 at 3:09 PM Randall S. Becker
> 
> wrote:
> 
> > The question: what is the best practice for versioning the parts of
> > clean/smudge filters that are in .git/config given that only some
> > users in my environment will be cloning the repository in question and
> > that I
> really
> > can't put the entries in /etc/gitconfig or ~/.gitconfig because of
> potential
> > conflicts with other repositories that might also have clean/smudge
> > definitions.
> 
> Depending on level of trust, one approach might be to use an [include] in
> .git/config to include a file that's in the repository. Something like:
> 
> [include]
>  path = ../path/to/config

It's a possibility, but I don't like the implications. Files that are subject 
to the clean/smudge would need to be reprocessed manually. In the scenario:

1. A checkout is done, changing ../path/to/config.
2. The clean/smudge configuration changes in ../path/to/config, but the files 
impacted by it do not.
3. git does not look like it would not be aware of the change until after the 
checkout, which is too late.
4. The work tree is now inconsistent with the idempotency of the clean/smudge 
rules, basically because nothing happened. (not blaming git here, just timing).

As far as I understand, this is a bit of a chicken-and-egg problem because the 
clean/smudge config needs to be there before the checkout. Correct?

Cheers,
Randall

Re: [Best Practices Request] clean/smudge configuration

[Bryan Turner]

On Wed, May 9, 2018 at 3:09 PM Randall S. Becker 
wrote:

> The question: what is the best practice for versioning the parts of
> clean/smudge filters that are in .git/config given that only some users in
> my environment will be cloning the repository in question and that I
really
> can't put the entries in /etc/gitconfig or ~/.gitconfig because of
potential
> conflicts with other repositories that might also have clean/smudge
> definitions.

Depending on level of trust, one approach might be to use an [include] in
.git/config to include a file that's in the repository. Something like:

[include]
 path = ../path/to/config

That way that part of the configuration can be derived from the
repository's contents. If you checkout a revision where the file doesn't
exist, then Git just ignores the include.

You're trusting the repository's contents, though, at that point; whatever
is in that file will be included in your overall config and honored by Git.

Bryan

[Best Practices Request] clean/smudge configuration

[Randall S. Becker]

Hi Git Team,

I'm trying to work out some best practices for managing clean/smudge filters
and hit a bump. The situation is that I have an environment where the
possible clean/smudge filter configuration can change over time and needs to
be versioned with the product being managed in a git repository. Part of the
configuration is no problem in .gitattributes, but the other bits are in
.git/config. I get that the runnable part of the filters need to be strictly
platform independent in principle, but I can abstract that part in this
situation.

The question: what is the best practice for versioning the parts of
clean/smudge filters that are in .git/config given that only some users in
my environment will be cloning the repository in question and that I really
can't put the entries in /etc/gitconfig or ~/.gitconfig because of potential
conflicts with other repositories that might also have clean/smudge
definitions.

Thanks,
Randall 

Proposal

[Zeliha Omer Faruk]

--
Hello

Greetings to you please i have a business proposal for you contact me
for more detailes asap thanks.

Best Regards,
Miss.Zeliha ömer faruk
Esentepe Mahallesi Büyükdere
Caddesi Kristal Kule Binasi
No:215
Sisli - Istanbul, Turkey

[PATCH] Implement --first-parent for git rev-list --bisect

[Tiago Botelho]

---
 bisect.c   | 53 +++--
 bisect.h   |  1 +
 builtin/rev-list.c |  3 +++
 3 files changed, 35 insertions(+), 22 deletions(-)

diff --git a/bisect.c b/bisect.c
index 4eafc8262..f43713574 100644
--- a/bisect.c
+++ b/bisect.c
@@ -34,7 +34,7 @@ static const char *term_good;
  * We care just barely enough to avoid recursing for
  * non-merge entries.
  */
-static int count_distance(struct commit_list *entry)
+static int count_distance(struct commit_list *entry, unsigned bisect_flags)
 {
int nr = 0;
 
@@ -49,10 +49,10 @@ static int count_distance(struct commit_list *entry)
commit->object.flags |= COUNTED;
p = commit->parents;
entry = p;
-   if (p) {
+   if (p && !(bisect_flags & BISECT_FIRST_PARENT)) {
p = p->next;
while (p) {
-   nr += count_distance(p);
+   nr += count_distance(p, bisect_flags);
p = p->next;
}
}
@@ -82,15 +82,16 @@ static inline void weight_set(struct commit_list *elem, int 
weight)
*((int*)(elem->item->util)) = weight;
 }
 
-static int count_interesting_parents(struct commit *commit)
+static int count_interesting_parents(struct commit *commit, unsigned 
bisect_flags)
 {
struct commit_list *p;
int count;
 
for (count = 0, p = commit->parents; p; p = p->next) {
-   if (p->item->object.flags & UNINTERESTING)
-   continue;
-   count++;
+   if (!(p->item->object.flags & UNINTERESTING))
+   count++;
+   if (bisect_flags & BISECT_FIRST_PARENT)
+   break;
}
return count;
 }
@@ -117,10 +118,10 @@ static inline int halfway(struct commit_list *p, int nr)
 }
 
 #if !DEBUG_BISECT
-#define show_list(a,b,c,d) do { ; } while (0)
+#define show_list(a,b,c,d,e) do { ; } while (0)
 #else
 static void show_list(const char *debug, int counted, int nr,
- struct commit_list *list)
+ struct commit_list *list, unsigned bisect_flags)
 {
struct commit_list *p;
 
@@ -146,10 +147,14 @@ static void show_list(const char *debug, int counted, int 
nr,
else
fprintf(stderr, "---");
fprintf(stderr, " %.*s", 8, oid_to_hex(>object.oid));
-   for (pp = commit->parents; pp; pp = pp->next)
+   for (pp = commit->parents; pp; pp = pp->next) {
fprintf(stderr, " %.*s", 8,
oid_to_hex(>item->object.oid));
 
+   if (bisect_flags & BISECT_FIRST_PARENT)
+   break;
+   }
+
subject_len = find_commit_subject(buf, _start);
if (subject_len)
fprintf(stderr, " %.*s", subject_len, subject_start);
@@ -267,13 +272,13 @@ static struct commit_list *do_find_bisection(struct 
commit_list *list,
unsigned flags = commit->object.flags;
 
p->item->util = [n++];
-   switch (count_interesting_parents(commit)) {
+   switch (count_interesting_parents(commit, bisect_flags)) {
case 0:
if (!(flags & TREESAME)) {
weight_set(p, 1);
counted++;
show_list("bisection 2 count one",
- counted, nr, list);
+ counted, nr, list, bisect_flags);
}
/*
 * otherwise, it is known not to reach any
@@ -289,7 +294,7 @@ static struct commit_list *do_find_bisection(struct 
commit_list *list,
}
}
 
-   show_list("bisection 2 initialize", counted, nr, list);
+   show_list("bisection 2 initialize", counted, nr, list, bisect_flags);
 
/*
 * If you have only one parent in the resulting set
@@ -310,7 +315,7 @@ static struct commit_list *do_find_bisection(struct 
commit_list *list,
continue;
if (weight(p) != -2)
continue;
-   weight_set(p, count_distance(p));
+   weight_set(p, count_distance(p, bisect_flags));
clear_distance(list);
 
/* Does it happen to be at exactly half-way? */
@@ -319,7 +324,7 @@ static struct commit_list *do_find_bisection(struct 
commit_list *list,
counted++;
}
 
-   show_list("bisection 2 count_distance", counted, nr, list);
+   show_list("bisection 2 count_distance", counted, nr, list, 
bisect_flags);
 
while (counted < nr) {
for (p = list; p; p = 

Re: [PATCH] fast-export: avoid NULL pointer arithmetic

[Johannes Schindelin]

Hi René,

On Wed, 9 May 2018, René Scharfe wrote:

> Clang 6 reports the following warning, which is turned into an error in a
> DEVELOPER build:
> 
>   builtin/fast-export.c:162:28: error: performing pointer arithmetic on a 
> null pointer has undefined behavior [-Werror,-Wnull-pointer-arithmetic]
>   return ((uint32_t *)NULL) + mark;
>  ~~ ^
>   1 error generated.
> 
> The compiler is correct, and the error message speaks for itself.  There
> is no need for any undefined operation -- just cast mark to void * or
> uint32_t after an intermediate cast to uintptr_t.  That encodes the
> integer value into a pointer and later decodes it as intended.
> 
> While at it remove an outdated comment -- intptr_t has been used since
> ffe659f94d (parse-options: make some arguments optional, add callbacks),
> committed in October 2007.

ACK.

Thanks,
Dscho

[PATCH 1/2] git-credential-netrc: adapt to test framework for git

[Luis Marsano]

until this change, git-credential-netrc did not test in a repository
this change reuses the main test framework, which provides such tests
specific changes
- switch to Test::More module
- use File::Basename & File::Spec::Functions

Signed-off-by: Luis Marsano 
Acked-by: Ted Zlatanov 
---
 contrib/credential/netrc/Makefile |  4 +-
 .../netrc/t-git-credential-netrc.sh   | 31 
 contrib/credential/netrc/test.pl  | 73 ---
 3 files changed, 78 insertions(+), 30 deletions(-)
 create mode 100755 contrib/credential/netrc/t-git-credential-netrc.sh

diff --git a/contrib/credential/netrc/Makefile 
b/contrib/credential/netrc/Makefile
index 51b76138a..0ffa40719 100644
--- a/contrib/credential/netrc/Makefile
+++ b/contrib/credential/netrc/Makefile
@@ -1,5 +1,5 @@
 test:
-   ./test.pl
+   ./t-git-credential-netrc.sh
 
 testverbose:
-   ./test.pl -d -v
+   ./t-git-credential-netrc.sh -d -v
diff --git a/contrib/credential/netrc/t-git-credential-netrc.sh 
b/contrib/credential/netrc/t-git-credential-netrc.sh
new file mode 100755
index 0..fa21ca240
--- /dev/null
+++ b/contrib/credential/netrc/t-git-credential-netrc.sh
@@ -0,0 +1,31 @@
+#!/bin/sh
+(
+   cd ../../../t
+   test_description='git-credential-netrc'
+   . ./test-lib.sh
+
+   if ! test_have_prereq PERL; then
+   skip_all='skipping perl interface tests, perl not available'
+   test_done
+   fi
+
+   perl -MTest::More -e 0 2>/dev/null || {
+   skip_all="Perl Test::More unavailable, skipping test"
+   test_done
+   }
+
+   # set up test repository
+
+   test_expect_success \
+'set up test repository' \
+':'
+
+   # The external test will outputs its own plan
+   test_external_has_tap=1
+
+   test_external \
+'git-credential-netrc' \
+perl "$TEST_DIRECTORY"/../contrib/credential/netrc/test.pl
+
+   test_done
+)
diff --git a/contrib/credential/netrc/test.pl b/contrib/credential/netrc/test.pl
index 169b6463c..7211b4857 100755
--- a/contrib/credential/netrc/test.pl
+++ b/contrib/credential/netrc/test.pl
@@ -1,83 +1,100 @@
 #!/usr/bin/perl
+use lib (split(/:/, $ENV{GITPERLLIB}));
 
 use warnings;
 use strict;
-use Test;
+use Test::More qw(no_plan);
+use File::Basename;
+use File::Spec::Functions qw(:DEFAULT rel2abs);
 use IPC::Open2;
 
-BEGIN { plan tests => 15 }
+BEGIN {
+   # t-git-credential-netrc.sh kicks off our testing, so we have to go 
from there.
+   Test::More->builder->current_test(1);
+   Test::More->builder->no_ending(1);
+}
 
 my @global_credential_args = @ARGV;
-my $netrc = './test.netrc';
-print "# Testing insecure file, nothing should be found\n";
+my $scriptDir = dirname rel2abs $0;
+my $netrc = catfile $scriptDir, 'test.netrc';
+my $netrcGpg = catfile $scriptDir, 'test.netrc.gpg';
+my $gcNetrc = catfile $scriptDir, 'git-credential-netrc';
+local $ENV{PATH} = join ':'
+  , $scriptDir
+  , $ENV{PATH}
+  ? $ENV{PATH}
+  : ();
+
+diag "Testing insecure file, nothing should be found\n";
 chmod 0644, $netrc;
 my $cred = run_credential(['-f', $netrc, 'get'],
  { host => 'github.com' });
 
-ok(scalar keys %$cred, 0, "Got 0 keys from insecure file");
+ok(scalar keys %$cred == 0, "Got 0 keys from insecure file");
 
-print "# Testing missing file, nothing should be found\n";
+diag "Testing missing file, nothing should be found\n";
 chmod 0644, $netrc;
 $cred = run_credential(['-f', '///nosuchfile///', 'get'],
   { host => 'github.com' });
 
-ok(scalar keys %$cred, 0, "Got 0 keys from missing file");
+ok(scalar keys %$cred == 0, "Got 0 keys from missing file");
 
 chmod 0600, $netrc;
 
-print "# Testing with invalid data\n";
+diag "Testing with invalid data\n";
 $cred = run_credential(['-f', $netrc, 'get'],
   "bad data");
-ok(scalar keys %$cred, 4, "Got first found keys with bad data");
+ok(scalar keys %$cred == 4, "Got first found keys with bad data");
 
-print "# Testing netrc file for a missing corovamilkbar entry\n";
+diag "Testing netrc file for a missing corovamilkbar entry\n";
 $cred = run_credential(['-f', $netrc, 'get'],
   { host => 'corovamilkbar' });
 
-ok(scalar keys %$cred, 0, "Got no corovamilkbar keys");
+ok(scalar keys %$cred == 0, "Got no corovamilkbar keys");
 
-print "# Testing netrc file for a github.com entry\n";
+diag "Testing netrc file for a github.com entry\n";
 $cred = run_credential(['-f', $netrc, 'get'],
   { host => 'github.com' });
 
-ok(scalar keys %$cred, 2, "Got 2 Github keys");
+ok(scalar keys %$cred == 2, "Got 2 Github keys");
 
-ok($cred->{password}, 'carolknows', "Got correct Github password");
-ok($cred->{username}, 'carol', "Got correct Github username");
+is($cred->{password}, 'carolknows', "Got 

[PATCH 2/2] git-credential-netrc: accept gpg option

[Luis Marsano]

git-credential-netrc was hardcoded to decrypt with 'gpg' regardless of the 
gpg.program option
this now uses the gpg command option if set, else, the gpg.program option set 
in the git repository or global configuration, else defaults to 'gpg'
for git-credential-netrc
- use Git.pm for repository and global option queries
- add -g|--gpg command option & document it in command usage
- test repository & command options
- support unicode

Signed-off-by: Luis Marsano 
Acked-by: Ted Zlatanov 
---
 contrib/credential/netrc/git-credential-netrc | 69 +--
 .../netrc/t-git-credential-netrc.sh   |  2 +-
 .../credential/netrc/test.command-option-gpg  |  2 +
 contrib/credential/netrc/test.git-config-gpg  |  2 +
 contrib/credential/netrc/test.netrc.gpg   |  0
 contrib/credential/netrc/test.pl  | 13 
 6 files changed, 65 insertions(+), 23 deletions(-)
 create mode 100755 contrib/credential/netrc/test.command-option-gpg
 create mode 100755 contrib/credential/netrc/test.git-config-gpg
 create mode 100644 contrib/credential/netrc/test.netrc.gpg

diff --git a/contrib/credential/netrc/git-credential-netrc 
b/contrib/credential/netrc/git-credential-netrc
index 1571a7b26..67cd689e8 100755
--- a/contrib/credential/netrc/git-credential-netrc
+++ b/contrib/credential/netrc/git-credential-netrc
@@ -2,11 +2,16 @@
 
 use strict;
 use warnings;
+use autodie;
+use utf8;
+use open ':encoding(UTF-8)';
+use feature qw(unicode_strings unicode_eval);
 
 use Getopt::Long;
 use File::Basename;
+use Git;
 
-my $VERSION = "0.1";
+my $VERSION = "0.2";
 
 my %options = (
   help => 0,
@@ -14,6 +19,7 @@ my %options = (
   verbose => 0,
   insecure => 0,
   file => [],
+  gpg => '',
 
   # identical token maps, e.g. host -> host, will be inserted later
   tmap => {
@@ -54,6 +60,7 @@ GetOptions(\%options,
"insecure|k",
"verbose|v",
"file|f=s@",
+   'gpg|g:s',
   );
 
 if ($options{help}) {
@@ -62,27 +69,31 @@ if ($options{help}) {
 
print <

[PATCH 0/2] Configurable GnuPG command for git-credential-netrc

[Luis Marsano]

Hi everyone,

Should `contrib/` area patches go here, too? Contributed Software
page
https://git.kernel.org/pub/scm/git/git.git/tree/contrib/README
says to forward them to "jc" (I'm guessing that's Junio), but I
think they're getting missed. Though the component author approves
(acknowledgement below), am I missing anything for submission?
Thanks.

> git-credential-netrc was hardcoded to call 'gpg' for decryption.
> This is a problem on distributions like Debian that call modern
> GnuPG something else, like 'gpg2'. This proposed update reuses
> git's gpg.program configuration to customize GnuPG's name. It
> introduces the -g|--gpg option on the git-credential-netrc
> command to allow alternatively setting it in git's
> credential.helper configuration. Since testing a git
> configuration involves a temporary repository as provided by
> git's main testing framework, a patch updating tests to reuse
> the main framework is included, too.

--- Forwarded message
Delivered-To: luis.mars...@gmail.com
Received: by 10.103.152.29 with SMTP id a29csp214132vse; Fri, 20 Apr 2018
 16:18:34 -0700 (PDT)
Return-Path: 
In-Reply-To: <1524160133-8877-1-git-send-email-luis.mars...@gmail.com>
References: <1524160133-8877-1-git-send-email-luis.mars...@gmail.com>
From: Ted Zlatanov 
Date: Fri, 20 Apr 2018 19:18:32 -0400
Message-ID: 

Proposal

[Zeliha Omer Faruk]

--
Hello

Greetings to you please i have a business proposal for you contact me
for more detailes asap thanks.

Best Regards,
Miss.Zeliha ömer faruk
Esentepe Mahallesi Büyükdere
Caddesi Kristal Kule Binasi
No:215
Sisli - Istanbul, Turkey

[PATCH] fast-export: avoid NULL pointer arithmetic

[René Scharfe]

Clang 6 reports the following warning, which is turned into an error in a
DEVELOPER build:

builtin/fast-export.c:162:28: error: performing pointer arithmetic on a 
null pointer has undefined behavior [-Werror,-Wnull-pointer-arithmetic]
return ((uint32_t *)NULL) + mark;
   ~~ ^
1 error generated.

The compiler is correct, and the error message speaks for itself.  There
is no need for any undefined operation -- just cast mark to void * or
uint32_t after an intermediate cast to uintptr_t.  That encodes the
integer value into a pointer and later decodes it as intended.

While at it remove an outdated comment -- intptr_t has been used since
ffe659f94d (parse-options: make some arguments optional, add callbacks),
committed in October 2007.

Signed-off-by: Rene Scharfe 
---
 builtin/fast-export.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 530df12f05..fa556a3c93 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -156,15 +156,14 @@ static void anonymize_path(struct strbuf *out, const char 
*path,
}
 }
 
-/* Since intptr_t is C99, we do not use it here */
-static inline uint32_t *mark_to_ptr(uint32_t mark)
+static inline void *mark_to_ptr(uint32_t mark)
 {
-   return ((uint32_t *)NULL) + mark;
+   return (void *)(uintptr_t)mark;
 }
 
 static inline uint32_t ptr_to_mark(void * mark)
 {
-   return (uint32_t *)mark - (uint32_t *)NULL;
+   return (uint32_t)(uintptr_t)mark;
 }
 
 static inline void mark_object(struct object *object, uint32_t mark)
-- 
2.17.0

[PATCH v2 5/5] lock_file: move static locks into functions

[Martin Ågren]

Placing `struct lock_file`s on the stack used to be a bad idea, because
the temp- and lockfile-machinery would keep a pointer into the struct.
But after 076aa2cbd (tempfile: auto-allocate tempfiles on heap,
2017-09-05), we can safely have lockfiles on the stack. (This applies
even if a user returns early, leaving a locked lock behind.)

Each of these `struct lock_file`s is used from within a single function.
Move them into the respective functions to make the scope clearer and
drop the staticness.

For good measure, I have inspected these sites and come to believe that
they always release the lock, with the possible exception of bailing out
using `die()` or `exit()` or by returning from a `cmd_foo()`.

As pointed out by Jeff King, it would be bad if someone held on to a
`struct lock_file *` for some reason. After some grepping, I agree with
his findings: no-one appears to be doing that.

After this commit, the remaining occurrences of "static struct
lock_file" are locks that are used from within different functions. That
is, they need to remain static. (Short of more intrusive changes like
passing around pointers to non-static locks.)

Signed-off-by: Martin Ågren 
---
 t/helper/test-scrap-cache-tree.c | 4 ++--
 builtin/add.c| 3 +--
 builtin/mv.c | 2 +-
 builtin/read-tree.c  | 3 +--
 builtin/rm.c | 3 +--
 rerere.c | 3 +--
 6 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/t/helper/test-scrap-cache-tree.c b/t/helper/test-scrap-cache-tree.c
index d26d3e7c8b..393f1604ff 100644
--- a/t/helper/test-scrap-cache-tree.c
+++ b/t/helper/test-scrap-cache-tree.c
@@ -4,10 +4,10 @@
 #include "tree.h"
 #include "cache-tree.h"
 
-static struct lock_file index_lock;
-
 int cmd__scrap_cache_tree(int ac, const char **av)
 {
+   struct lock_file index_lock = LOCK_INIT;
+
setup_git_directory();
hold_locked_index(_lock, LOCK_DIE_ON_ERROR);
if (read_cache() < 0)
diff --git a/builtin/add.c b/builtin/add.c
index c9e2619a9a..8a155dd41e 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -265,8 +265,6 @@ static int edit_patch(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
-static struct lock_file lock_file;
-
 static const char ignore_error[] =
 N_("The following paths are ignored by one of your .gitignore files:\n");
 
@@ -393,6 +391,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
int add_new_files;
int require_pathspec;
char *seen = NULL;
+   struct lock_file lock_file = LOCK_INIT;
 
git_config(add_config, NULL);
 
diff --git a/builtin/mv.c b/builtin/mv.c
index 6d141f7a53..b4692409e3 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -72,7 +72,6 @@ static const char *add_slash(const char *path)
return path;
 }
 
-static struct lock_file lock_file;
 #define SUBMODULE_WITH_GITDIR ((const char *)1)
 
 static void prepare_move_submodule(const char *src, int first,
@@ -131,6 +130,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;
struct stat st;
struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
+   struct lock_file lock_file = LOCK_INIT;
 
git_config(git_default_config, NULL);
 
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index bf87a2710b..ebc43eb805 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -107,8 +107,6 @@ static int git_read_tree_config(const char *var, const char 
*value, void *cb)
return git_default_config(var, value, cb);
 }
 
-static struct lock_file lock_file;
-
 int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 {
int i, stage = 0;
@@ -116,6 +114,7 @@ int cmd_read_tree(int argc, const char **argv, const char 
*unused_prefix)
struct tree_desc t[MAX_UNPACK_TREES];
struct unpack_trees_options opts;
int prefix_set = 0;
+   struct lock_file lock_file = LOCK_INIT;
const struct option read_tree_options[] = {
{ OPTION_CALLBACK, 0, "index-output", NULL, N_("file"),
  N_("write resulting index to "),
diff --git a/builtin/rm.c b/builtin/rm.c
index 5b6fc7ee81..65b448ef8e 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -233,8 +233,6 @@ static int check_local_mod(struct object_id *head, int 
index_only)
return errs;
 }
 
-static struct lock_file lock_file;
-
 static int show_only = 0, force = 0, index_only = 0, recursive = 0, quiet = 0;
 static int ignore_unmatch = 0;
 
@@ -251,6 +249,7 @@ static struct option builtin_rm_options[] = {
 
 int cmd_rm(int argc, const char **argv, const char *prefix)
 {
+   struct lock_file lock_file = LOCK_INIT;
int i;
struct pathspec pathspec;
char *seen;
diff --git a/rerere.c b/rerere.c
index 18cae2d11c..e0862e2778 100644
--- a/rerere.c
+++ b/rerere.c
@@ -703,10 +703,9 @@ 

[PATCH v2 2/5] refs.c: do not die if locking fails in `write_pseudoref()`

[Martin Ågren]

If we could not take the lock, we add an error to the `strbuf err` and
return. However, this code is dead. The reason is that we take the lock
using `LOCK_DIE_ON_ERROR`. Drop the flag to allow our more gentle
error-handling to actually kick in.

We could instead just drop the dead code and die here. But everything is
prepared for gently propagating the error, so let's do that instead.

There is similar dead code in `delete_pseudoref()`, but let's save that
for the next patch.

While at it, make the lock non-static. (Placing `struct lock_file`s on
the stack used to be a bad idea, because the temp- and
lockfile-machinery would keep a pointer into the struct. But after
076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05), we
can safely have lockfiles on the stack.)

Reviewed-by: Stefan Beller 
Signed-off-by: Martin Ågren 
---
 refs.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index 8b7a77fe5e..8c50b8b139 100644
--- a/refs.c
+++ b/refs.c
@@ -644,7 +644,7 @@ static int write_pseudoref(const char *pseudoref, const 
struct object_id *oid,
 {
const char *filename;
int fd;
-   static struct lock_file lock;
+   struct lock_file lock = LOCK_INIT;
struct strbuf buf = STRBUF_INIT;
int ret = -1;
 
@@ -654,8 +654,7 @@ static int write_pseudoref(const char *pseudoref, const 
struct object_id *oid,
strbuf_addf(, "%s\n", oid_to_hex(oid));
 
filename = git_path("%s", pseudoref);
-   fd = hold_lock_file_for_update_timeout(, filename,
-  LOCK_DIE_ON_ERROR,
+   fd = hold_lock_file_for_update_timeout(, filename, 0,
   get_files_ref_lock_timeout_ms());
if (fd < 0) {
strbuf_addf(err, "could not open '%s' for writing: %s",
-- 
2.17.0.411.g9fd64c8e46

[PATCH v2 4/5] lock_file: make function-local locks non-static

[Martin Ågren]

Placing `struct lock_file`s on the stack used to be a bad idea, because
the temp- and lockfile-machinery would keep a pointer into the struct.
But after 076aa2cbd (tempfile: auto-allocate tempfiles on heap,
2017-09-05), we can safely have lockfiles on the stack. (This applies
even if a user returns early, leaving a locked lock behind.)

These `struct lock_file`s are local to their respective functions and we
can drop their staticness.

For good measure, I have inspected these sites and come to believe that
they always release the lock, with the possible exception of bailing out
using `die()` or `exit()` or by returning from a `cmd_foo()`.

As pointed out by Jeff King, it would be bad if someone held on to a
`struct lock_file *` for some reason. After some grepping, I agree with
his findings: no-one appears to be doing that.

Signed-off-by: Martin Ågren 
---
 apply.c| 2 +-
 builtin/describe.c | 2 +-
 builtin/difftool.c | 2 +-
 builtin/gc.c   | 2 +-
 builtin/merge.c| 4 ++--
 builtin/receive-pack.c | 2 +-
 bundle.c   | 2 +-
 fast-import.c  | 2 +-
 refs/files-backend.c   | 2 +-
 shallow.c  | 2 +-
 10 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/apply.c b/apply.c
index 7e5792c996..07b14d1127 100644
--- a/apply.c
+++ b/apply.c
@@ -4058,7 +4058,7 @@ static int build_fake_ancestor(struct apply_state *state, 
struct patch *list)
 {
struct patch *patch;
struct index_state result = { NULL };
-   static struct lock_file lock;
+   struct lock_file lock = LOCK_INIT;
int res;
 
/* Once we start supporting the reverse patch, it may be
diff --git a/builtin/describe.c b/builtin/describe.c
index b5afc45846..8bd95cf371 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -612,7 +612,7 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
suffix = broken;
}
} else if (dirty) {
-   static struct lock_file index_lock;
+   struct lock_file index_lock = LOCK_INIT;
struct rev_info revs;
struct argv_array args = ARGV_ARRAY_INIT;
int fd, result;
diff --git a/builtin/difftool.c b/builtin/difftool.c
index aad0e073ee..162806f238 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -610,7 +610,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, 
const char *prefix,
continue;
 
if (!indices_loaded) {
-   static struct lock_file lock;
+   struct lock_file lock = LOCK_INIT;
strbuf_reset();
strbuf_addf(, "%s/wtindex", tmpdir);
if (hold_lock_file_for_update(, buf.buf, 0) < 0 ||
diff --git a/builtin/gc.c b/builtin/gc.c
index 3e67124eaa..274660d9dc 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -234,7 +234,7 @@ static int need_to_gc(void)
 /* return NULL on success, else hostname running the gc */
 static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 {
-   static struct lock_file lock;
+   struct lock_file lock = LOCK_INIT;
char my_host[HOST_NAME_MAX + 1];
struct strbuf sb = STRBUF_INIT;
struct stat st;
diff --git a/builtin/merge.c b/builtin/merge.c
index 9db5a2cf16..de62b2c5c6 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -647,7 +647,7 @@ static int try_merge_strategy(const char *strategy, struct 
commit_list *common,
  struct commit_list *remoteheads,
  struct commit *head)
 {
-   static struct lock_file lock;
+   struct lock_file lock = LOCK_INIT;
const char *head_arg = "HEAD";
 
hold_locked_index(, LOCK_DIE_ON_ERROR);
@@ -805,7 +805,7 @@ static int merge_trivial(struct commit *head, struct 
commit_list *remoteheads)
 {
struct object_id result_tree, result_commit;
struct commit_list *parents, **pptr = 
-   static struct lock_file lock;
+   struct lock_file lock = LOCK_INIT;
 
hold_locked_index(, LOCK_DIE_ON_ERROR);
refresh_cache(REFRESH_QUIET);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 4b68a28e92..d3cf36cef5 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -876,7 +876,7 @@ static void refuse_unconfigured_deny_delete_current(void)
 static int command_singleton_iterator(void *cb_data, struct object_id *oid);
 static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
 {
-   static struct lock_file shallow_lock;
+   struct lock_file shallow_lock = LOCK_INIT;
struct oid_array extra = OID_ARRAY_INIT;
struct check_connected_options opt = CHECK_CONNECTED_INIT;
uint32_t mask = 1 << (cmd->index % 32);
diff --git a/bundle.c b/bundle.c
index 902c9b5448..160bbfdc64 100644

[PATCH v2 1/5] t/helper/test-write-cache: clean up lock-handling

[Martin Ågren]

Die in case writing the index fails, so that the caller can notice
(instead of, say, being impressed by how performant the writing is).

While at it, note that after opening a lock with `LOCK_DIE_ON_ERROR`, we
do not need to worry about whether we succeeded. Also, we can move the
`struct lock_file` into the function and drop the staticness. (Placing
`struct lock_file`s on the stack used to be a bad idea, because the
temp- and lockfile-machinery would keep a pointer into the struct. But
after 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05),
we can safely have lockfiles on the stack.)

Signed-off-by: Martin Ågren 
---
 t/helper/test-write-cache.c | 14 +-
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/t/helper/test-write-cache.c b/t/helper/test-write-cache.c
index 017dc30380..8837717d36 100644
--- a/t/helper/test-write-cache.c
+++ b/t/helper/test-write-cache.c
@@ -2,22 +2,18 @@
 #include "cache.h"
 #include "lockfile.h"
 
-static struct lock_file index_lock;
-
 int cmd__write_cache(int argc, const char **argv)
 {
-   int i, cnt = 1, lockfd;
+   struct lock_file index_lock = LOCK_INIT;
+   int i, cnt = 1;
if (argc == 2)
cnt = strtol(argv[1], NULL, 0);
setup_git_directory();
read_cache();
for (i = 0; i < cnt; i++) {
-   lockfd = hold_locked_index(_lock, LOCK_DIE_ON_ERROR);
-   if (0 <= lockfd) {
-   write_locked_index(_index, _lock, 
COMMIT_LOCK);
-   } else {
-   rollback_lock_file(_lock);
-   }
+   hold_locked_index(_lock, LOCK_DIE_ON_ERROR);
+   if (write_locked_index(_index, _lock, COMMIT_LOCK))
+   die("unable to write index file");
}
 
return 0;
-- 
2.17.0.411.g9fd64c8e46

[PATCH v2 3/5] refs.c: do not die if locking fails in `delete_pseudoref()`

[Martin Ågren]

After taking the lock we check whether we got it and die otherwise. But
since we take the lock using `LOCK_DIE_ON_ERROR`, we would already have
died.

Considering the choice between dropping the dead code and dropping the
flag, let's go for option number three: Drop the flag, write an error
instead of dying, then return -1. This function already returns -1 for
another error, so the caller (or rather, its callers) should be able to
handle this. There is some inconsistency around how we handle errors in
this function and elsewhere in this file, but let's take this small step
towards gentle error-reporting now and leave the rest for another time.

While at it, make the lock non-static and reduce its scope. (Placing
`struct lock_file`s on the stack used to be a bad idea, because the
temp- and lockfile-machinery would keep a pointer into the struct. But
after 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05),
we can safely have lockfiles on the stack.)

Signed-off-by: Martin Ågren 
---
 refs.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 8c50b8b139..c006004bcd 100644
--- a/refs.c
+++ b/refs.c
@@ -689,20 +689,23 @@ static int write_pseudoref(const char *pseudoref, const 
struct object_id *oid,
 
 static int delete_pseudoref(const char *pseudoref, const struct object_id 
*old_oid)
 {
-   static struct lock_file lock;
const char *filename;
 
filename = git_path("%s", pseudoref);
 
if (old_oid && !is_null_oid(old_oid)) {
+   struct lock_file lock = LOCK_INIT;
int fd;
struct object_id actual_old_oid;
 
fd = hold_lock_file_for_update_timeout(
-   , filename, LOCK_DIE_ON_ERROR,
+   , filename, 0,
get_files_ref_lock_timeout_ms());
-   if (fd < 0)
-   die_errno(_("Could not open '%s' for writing"), 
filename);
+   if (fd < 0) {
+   error_errno(_("could not open '%s' for writing"),
+   filename);
+   return -1;
+   }
if (read_ref(pseudoref, _old_oid))
die("could not read ref '%s'", pseudoref);
if (oidcmp(_old_oid, old_oid)) {
-- 
2.17.0.411.g9fd64c8e46

[PATCH v2 0/5] getting rid of most "static struct lock_file"s

[Martin Ågren]

This is take two of my attempt at making almost all `struct lock_file`s
non-static. All patches have been equipped with more detailed commit
messages. The only diff that has changed is patch 3/5, where I now take
a small step towards gentle error-handling, rather than going in the
opposite direction.

Thanks all for the valuable feedback on v1. I could have saved everyone
some trouble by writing better commit messages from the start, and
probably also by using `--thread` when formatting the patches...

Martin

Martin Ågren (5):
  t/helper/test-write-cache: clean up lock-handling
  refs.c: do not die if locking fails in `write_pseudoref()`
  refs.c: do not die if locking fails in `delete_pseudoref()`
  lock_file: make function-local locks non-static
  lock_file: move static locks into functions

 t/helper/test-scrap-cache-tree.c |  4 ++--
 t/helper/test-write-cache.c  | 14 +-
 apply.c  |  2 +-
 builtin/add.c|  3 +--
 builtin/describe.c   |  2 +-
 builtin/difftool.c   |  2 +-
 builtin/gc.c |  2 +-
 builtin/merge.c  |  4 ++--
 builtin/mv.c |  2 +-
 builtin/read-tree.c  |  3 +--
 builtin/receive-pack.c   |  2 +-
 builtin/rm.c |  3 +--
 bundle.c |  2 +-
 fast-import.c|  2 +-
 refs.c   | 16 +---
 refs/files-backend.c |  2 +-
 rerere.c |  3 +--
 shallow.c|  2 +-
 18 files changed, 32 insertions(+), 38 deletions(-)

-- 
2.17.0.411.g9fd64c8e46

Re: [PATCH] t6050-replace: don't disable stdin for the whole test script

[SZEDER Gábor]

On Tue, May 8, 2018 at 10:53 PM, Johannes Schindelin
 wrote:
> On Mon, 7 May 2018, SZEDER Gábor wrote:
>
>> The test script 't6050-replace.sh' starts off with redirecting the whole
>> test script's stdin from /dev/null.  This redirection has been there
>> since the test script was introduced in a3e8267225 (replace_object: add
>> a test case, 2009-01-23), but the commit message doesn't explain why it
>> was deemed necessary.  AFAICT, it has never been necessary, and t6050
>> runs just fine and succeeds even without it, not only the current
>> version but past versions as well.
>>
>> Besides being unnecessary, this redirection is also harmful, as it
>> prevents the test helper functions 'test_pause' and 'debug' from working
>> properly in t6050, because we can't enter any commands to the shell and
>> the debugger, respectively.
>
> The redirection might have been necessary before 781f76b1582 (test-lib:
> redirect stdin of tests, 2011-12-15), but it definitely is not necessary
> now.

That doesn't seem to be an issue in a3e8267225 (or in any other
commits touching t6050 since):

  $ echo foobar | ( ./t6050-replace.sh ; read input ; echo $input )
  *   ok 1: set up buggy branch
  *   ok 2: replace the author
  * passed all 2 test(s)
  foobar

Re: [PATCH v1] add status config and command line options for rename detection

[Ben Peart]

On 5/9/2018 12:56 PM, Elijah Newren wrote:

Hi Ben,

Overall I think this is good, but I have lots of nit-picky things to
bring up.  :-)




Thank you for the review.  I appreciate the extra set of eyes on these 
changes.  Especially when dealing with the merge logic and settings 
which I am unfamiliar with.




I suspect that status.renames should mention "copy", just as
diff.renames does.  (We didn't mention it in merge.renames, because
merge isn't an operation for which copy detection can be useful -- at
least not until the diffstat at the end of the merge is controlled by
merge.renames as well...)



I wasn't supporting copy (as you noticed later in the patch) but will 
update the patch to do so and update the documentation appropriately.



Also, do these two config settings only affect 'git status', or does
it also affect the status shown when composing a commit message
(assuming the user hasn't turned commit.status off)?  Does it affect
`git commit --dry-run` too?



The config settings only affect 'git status'


--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -109,6 +109,10 @@ static int have_option_m;
  static struct strbuf message = STRBUF_INIT;

  static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
+static int diff_detect_rename = -1;
+static int status_detect_rename = -1;
+static int diff_rename_limit = -1;
+static int status_rename_limit = -1;


Could we replace these four variables with just two: detect_rename and
rename_limit?  Keeping these separate invites people to write code
using only one of the settings rather than the appropriate inherited
mixture of them, resulting in a weird bug.  More on this below...



This model was inherited from the diff code and replicated to the merge 
code.  However, it would be nice to get rid of these 4 static variables. 
 See below for a proposal on how to do that...



@@ -1259,11 +1273,29 @@ static int git_status_config(const char *k, const char 
*v, void *cb)
 return error(_("Invalid untracked files mode '%s'"), 
v);
 return 0;
 }
+   if (!strcmp(k, "diff.renamelimit")) {
+   diff_rename_limit = git_config_int(k, v);
+   return 0;
+   }
+   if (!strcmp(k, "status.renamelimit")) {
+   status_rename_limit = git_config_int(k, v);
+   return 0;
+   }


Here, since you're already checking diff.renamelimit first, you can
just set rename_limit in both blocks and not need both
diff_rename_limit and status_rename_limit.  (Similar can be said for
diff.renames/status.renames.)


It really doesn't work that way - the call back is called for every 
config setting and there is no specific order they are called with. 
Typically, you just test for and save off any that you care about like 
I"m doing here.


I can update the logic here so that as I save off the settings that it 
will also enforce the priority model (ie the diff setting can't override 
the status setting) and then compute the final value once I have the 
command line arguments as they override either config setting (if present).


On the plus side, this change passes the red/green test but it now 
splits the priority logic into two places and doesn't match with how 
diff and merge handle this same issue.







@@ -1297,6 +1329,10 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
   N_("ignore changes to submodules, optional when: all, dirty, 
untracked. (Default: all)"),
   PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
 OPT_COLUMN(0, "column", , N_("list untracked files in 
columns")),
+   OPT_BOOL(0, "no-renames", _renames, N_("do not detect 
renames")),
+   { OPTION_CALLBACK, 'M', "find-renames", _score_arg,
+ N_("n"), N_("detect renames, optionally set similarity 
index"),
+ PARSE_OPT_OPTARG, opt_parse_rename_score },


Should probably also document these options in
Documentation/git-status.txt (and maybe Documentation/git-commit.txt
as well).


Good point, will do.



Not sure if we want to add a flag for copy detection (similar to
git-diff's -C/--find-copies and --find-copies-harder), or just leave
that for when someone finds a need.  If left out, might want to just
mention that it was considered and intentionally omitted for now in
the commit message.



I tend to only implement the features I know are actually needed so 
intentionally omitted this (along with many other potential diff options).



+   if ((intptr_t)rename_score_arg != -1) {


I don't understand why rename_score_arg is a (char*) and then you need
to cast to intptr_t, but that may just be because I haven't done much
of anything with option parsing.  A quick look at the docs isn't
making it clear to me, though; could you enlighten me?



Yes, it is related to making parse_options() do what we need.  -1 means 
the command line option wasn't passed so use the default.  NULL 

Re: Is rebase --force-rebase any different from rebase --no-ff?

[Ilya Kantor]

I tried to compare --force-rebase VS --no-ff for the following repository:
http://jmp.sh/E7TRjcL

There's no difference in the resulf of:
git rebase --force-rebase 54a4
git rebase --no-ff 54a4

(rebases all 3 commits of feature)

Also, there's no difference in interactive mode:
git rebase --force-rebase -i 54a4
git rebase --no-ff -i 54a4

(picks all 3 commits of feature)

Is there a case where --no-ff differs from --force-rebase?

---
Best Regards,
Ilya Kantor


On Wed, May 9, 2018 at 10:27 PM, Marc Branchaud  wrote:
> On 2018-05-09 02:21 PM, Stefan Beller wrote:
>>
>> +cc Marc and Johannes who know more about rebase.
>>
>> On Wed, May 9, 2018 at 9:01 AM, Ilya Kantor  wrote:
>>>
>>> Right now in "git help rebase" for --no-ff:
>>> "Without --interactive, this is a synonym for --force-rebase."
>>>
>>> But *with* --interactive, is there any difference?
>>
>>
>> I found
>>
>> https://code.googlesource.com/git/+/b499549401cb2b1f6c30d09681380fd519938eb0
>> from 2010-03-24
>
>
> In the original discussion around this option [1], at one point I proposed
> teaching rebase--interactive to respect --force-rebase instead of adding a
> new option [2].  Ultimately --no-ff was chosen as the better user interface
> design [3], because an interactive rebase can't be "forced" to run.
>
> At the time, I think rebase--interactive only recognized --no-ff.  That
> might have been muddled a bit in the migration to rebase--helper.c.
>
> Looking at it now, I don't have a strong opinion about keeping both options
> or deprecating one of them.
>
> M.
>
> [1] https://public-inbox.org/git/4b9fd9c1.9060...@xiplink.com/t/
> [2]
> https://public-inbox.org/git/1269361187-31291-1-git-send-email-marcn...@xiplink.com/
> [3] https://public-inbox.org/git/7vzl1yd5j4@alter.siamese.dyndns.org/
>
>
>>  Teach rebase the --no-ff option.
>>
>>  For git-rebase.sh, --no-ff is a synonym for --force-rebase.
>>
>>  For git-rebase--interactive.sh, --no-ff cherry-picks all the commits
>> in
>>  the rebased branch, instead of fast-forwarding over any unchanged
>> commits.
>>
>>  --no-ff offers an alternative way to deal with reverted merges.
>> Instead of
>>  "reverting the revert" you can use "rebase --no-ff" to recreate the
>> branch
>>  with entirely new commits (they're new because at the very least the
>>  committer time is different).  This obviates the need to revert the
>>  reversion, as you can re-merge the new topic branch directly.  Added
>> an
>>  addendum to revert-a-faulty-merge.txt describing the situation and
>> how to
>>  use --no-ff to handle it.
>>
>> which sounds as if there is?
>>
>> Stefan
>>
>

Hello

[Mrs Pamela Atuegbe]

Am Mrs.Pamela Atuegbe, I work in one of the prime bank here in burkina 
faso, I want the bank to transfer the money left by our late customer 
is a foreigner from Korea. can you investment this money and also help 
the poor' the amount value at $13,300,000.00 (Thirteen Million Three 
Hundred Thousand United States American Dollars), left in his account 
still unclaimed. more details will be giving to you if you are 
interested, E-mail To: mrspamelaatuegbe...@gmail.com
I wait your reply thanks.

 
Yours sincerely

Name: Mrs. Pamela Atuegbe

Re: Is rebase --force-rebase any different from rebase --no-ff?

[Marc Branchaud]

On 2018-05-09 02:21 PM, Stefan Beller wrote:

+cc Marc and Johannes who know more about rebase.

On Wed, May 9, 2018 at 9:01 AM, Ilya Kantor  wrote:

Right now in "git help rebase" for --no-ff:
"Without --interactive, this is a synonym for --force-rebase."

But *with* --interactive, is there any difference?


I found
https://code.googlesource.com/git/+/b499549401cb2b1f6c30d09681380fd519938eb0
from 2010-03-24


In the original discussion around this option [1], at one point I 
proposed teaching rebase--interactive to respect --force-rebase instead 
of adding a new option [2].  Ultimately --no-ff was chosen as the better 
user interface design [3], because an interactive rebase can't be 
"forced" to run.


At the time, I think rebase--interactive only recognized --no-ff.  That 
might have been muddled a bit in the migration to rebase--helper.c.


Looking at it now, I don't have a strong opinion about keeping both 
options or deprecating one of them.


M.

[1] https://public-inbox.org/git/4b9fd9c1.9060...@xiplink.com/t/
[2] 
https://public-inbox.org/git/1269361187-31291-1-git-send-email-marcn...@xiplink.com/

[3] https://public-inbox.org/git/7vzl1yd5j4@alter.siamese.dyndns.org/



 Teach rebase the --no-ff option.

 For git-rebase.sh, --no-ff is a synonym for --force-rebase.

 For git-rebase--interactive.sh, --no-ff cherry-picks all the commits in
 the rebased branch, instead of fast-forwarding over any unchanged commits.

 --no-ff offers an alternative way to deal with reverted merges.  Instead of
 "reverting the revert" you can use "rebase --no-ff" to recreate the branch
 with entirely new commits (they're new because at the very least the
 committer time is different).  This obviates the need to revert the
 reversion, as you can re-merge the new topic branch directly.  Added an
 addendum to revert-a-faulty-merge.txt describing the situation and how to
 use --no-ff to handle it.

which sounds as if there is?

Stefan

Re: [PATCH v3 13/13] alloc: allow arbitrary repositories for alloc functions

[Stefan Beller]

On Wed, May 9, 2018 at 10:18 AM, Duy Nguyen  wrote:
>
> If you want to reproduce, this is what I used to test this with.
>
> https://gist.github.com/pclouds/86a2df6c28043f1b6fa3d4e72e7a1276

This only applied cleanly after I created an empty file at
t/helper/test-abc.c, using git-apply. I'll use it to have no leaks here.

Thanks!
Stefan

Re: Implementing reftable in Git

[Ævar Arnfjörð Bjarmason]

On Wed, May 09 2018, Stefan Beller wrote:

> Hi Christian,
>
> On Wed, May 9, 2018 at 7:33 AM, Christian Couder
>  wrote:
>> Hi,
>>
>> I might start working on implementing reftable in Git soon.
>
> Cool! Everyone is waiting for it as they dream about the
> performance and correctness benefits this brings.
>
> Benefits that I know of:
> * performance in repos with many refs
> * no capitalization issues on case insensitive FS
> * replay-ability of the last fetch ("show the last reflog
>   of any ref under refs/remote/origin") is easier to do
>   in a correct way. (This is one of my motivations to desire reftables)
> * We *might* be able to use reftables in negotiation later
>   ("client: Last I fetched, you said your latest transaction
>   number was '5' with the hash over all refs to be ;
>   server: ok, here are the refs and the pack, you're welcome").
>
> Why are you (or rather booking.com) interested in this?

We have a lot of refs, which is a longer-term scalability issue (which
I've implemented hacks around (ref archiving)), and we also run into the
capitalization issues you mentioned.

Re: [PATCH v2 0/2] gitk: improve handling of submodules in the file list panel

[Stefan Beller]

On Wed, May 9, 2018 at 5:35 AM, Alex Riesen
 wrote:
> From: Alex Riesen 
>
> Currently, the submodule entries in the file list panel are mostly ignored.
> This series attempts to improve the situation by showing part of submodule
> history when focusing it in the file list panel and by adding a menu element
> to start gitk in the submodule (similar to git gui).
>
> This iteration does not address the behaviour of file list panel in tree mode
> when gitk is started from a subdirectory (gitk strictly limits the file
> listing to the files in that repository, without a way out).
> I would like to hear some more opinions regarding changing its behaviour to
> always list the full tree.
>
> Alex Riesen (2):
>   gitk: show part of submodule log instead of empty pane when listing
> trees
>   gitk: add an option to run gitk on an item in the file list

both patches look ok, to my untrained eye.

Re: [PATCH] sha1dc: fix for compiling on AIX using IBM XLC compiler

[Jonathan Nieder]

(+cc: Marc Stevens)
Ævar Arnfjörð Bjarmason wrote:
> On Wed, May 09 2018, jens persson wrote:

>> Hello, first patch. I'm having trouble compiling on AIX using IBMs
>> compiler, leading to
>> unusable binaries. The following patch solved the problem for 2.17.0.
>> The patch below is cut via gmail to allow for firewalls, but
>> exists in an unmolested form on github:
>> https://github.com/MrShark/git/commit/44bfcaca6637e24548ec06f46fb6035a846b14af
>>
>> Best regards
>> /jp

Thanks for the patch.

>> Building on AIX using XLC every checkout gives an error:
>> fatal: pack is corrupted (SHA1 mismatch)
>> fatal: index-pack failed
>>
>> Back tracking it was introduced in 2.13.2, most likely in [1]
>>
>> Add a #ifdef guard based on macros defined at [2] and [3].
>>
>> Should perhaps __xlc__ should should be changed to or combined with _AIX
>> based on the behavour of GCC on AIX or XL C on Linux.
>>
>> 1. https://github.com/git/git/commit/6b851e536b05e0c8c61f77b9e4c3e7cedea39ff8
>> 2. 
>> https://www.ibm.com/support/knowledgecenter/SSGH2K_13.1.3/com.ibm.xlc1313.aix.doc/compiler_ref/macros_platform.html
>> 3. 
>> https://www.ibm.com/support/knowledgecenter/SSGH2K_13.1.3/com.ibm.xlc1313.aix.doc/compiler_ref/xlmacros.html
>>
>> Signed-off-by: jens persson 
>> ---
>>  sha1dc/sha1.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
>> index 25eded139..68a8a0180 100644
>> --- a/sha1dc/sha1.c
>> +++ b/sha1dc/sha1.c
>> @@ -84,7 +84,7 @@
>>  /* Not under GCC-alike or glibc or *BSD or newlib */
>>  #elif (defined(__ARMEB__) || defined(__THUMBEB__) || defined(__AARCH64EB__) 
>> || \
>> defined(__MIPSEB__) || defined(__MIPSEB) || defined(_MIPSEB) || \
>> -   defined(__sparc))
>> +   defined(__sparc) || (defined(__powerpc) && defined(__xlc__)))

I wonder if there's a simpler way to detect this.

__powerpc seems orthogonal to the goal, since there are little-endian
powerpc machines.

It appears that XLC defines _BIG_ENDIAN on big-endian machines.  I
wonder if we should do

 #elif defined(_BYTE_ORDER) && defined(_BIG_ENDIAN) && defined(_LITTLE_ENDIAN)
  ... as today ...
 #elif !defined(_BYTE_ORDER) && defined(_BIG_ENDIAN) && !defined(_LITTLE_ENDIAN)
 # define SHA1DC_BIGENDIAN
 #elif !defined(_BYTE_ORDER) && !defined(_BIG_ENDIAN) && defined(_LITTLE_ENDIAN)
  /* little endian. */
 #else
  ...

It also seems to me that Git should enable the #error in the
fallthrough case.  The test suite would catch this kind of problem but
it does not seem that everyone runs the test suite, so a compiler
error is more robust.  Is there a #define we can set to enable that?

>>  /*
>>   * Should define Big Endian for a whitelist of known processors. See
>>   * https://sourceforge.net/p/predef/wiki/Endianness/ and
>
> This patch looks sane to me, but we don't manage this software but
> instead try to pull it as-is from upstream at
> https://github.com/cr-marcstevens/sha1collisiondetection
>
> Which we could apply this, it would be much better if you could submit a
> pull request with this to upstream, and then we can update our copy as I
> did in e.g. 9936c1b52a ("sha1dc: update from upstream", 2017-07-01).

Thanks,
Jonathan

Re: Is rebase --force-rebase any different from rebase --no-ff?

[Stefan Beller]

+cc Marc and Johannes who know more about rebase.

On Wed, May 9, 2018 at 9:01 AM, Ilya Kantor  wrote:
> Right now in "git help rebase" for --no-ff:
> "Without --interactive, this is a synonym for --force-rebase."
>
> But *with* --interactive, is there any difference?

I found
https://code.googlesource.com/git/+/b499549401cb2b1f6c30d09681380fd519938eb0
from 2010-03-24

Teach rebase the --no-ff option.

For git-rebase.sh, --no-ff is a synonym for --force-rebase.

For git-rebase--interactive.sh, --no-ff cherry-picks all the commits in
the rebased branch, instead of fast-forwarding over any unchanged commits.

--no-ff offers an alternative way to deal with reverted merges.  Instead of
"reverting the revert" you can use "rebase --no-ff" to recreate the branch
with entirely new commits (they're new because at the very least the
committer time is different).  This obviates the need to revert the
reversion, as you can re-merge the new topic branch directly.  Added an
addendum to revert-a-faulty-merge.txt describing the situation and how to
use --no-ff to handle it.

which sounds as if there is?

Stefan

Re: [PATCH] repository: fix free problem with repo_clear(the_repository)

[Stefan Beller]

On Wed, May 9, 2018 at 11:00 AM, Duy Nguyen  wrote:
> On Wed, May 9, 2018 at 7:50 PM, Stefan Beller  wrote:
>>>  I was trying to test the new parsed_object_pool_clear() and found this.
>>
>> So this would go with the latest sb/object-store-alloc ?
>
> No this should be separate because sb/object-store-alloc did not even
> touch this code. I mistakenly thought you wrote this and sent to you.
> I should have checked and sent to Brandon instead.

Yes, they do not produce merge conflicts and do not semantically
rely on each other. Except as noted below
the object store series complicated things in a non-latest version
of it, such that we'd have to add more special casing. Now everything
is fine.


>> I would rather special case the_repository even more instead
>> of having it allocate all its things on the heap. (However we rather
>> want to profile it and argue with data)
>
> I'm actually going the opposite direction and trying hard to make
> the_repository normal like everybody else :)

ok, both Brandon and you want to do this, so I guess we'll just
go this route for now.

>
> discard_index(repo->index);
> if (repo->index != _index)
> FREE_AND_NULL(repo->index);
>
>> What is your use case of repo_clear(the_repository)?
>
> No actual use case right now. See [1] for the code that triggered
> this. I do want to free some memory in pack-objects and repo_clear()
> _might_ be the one. I'm not sure yet.

This diff seems good to me. as any repo is cleared and wil have the minimal
amount of memory. the_repository clears its the_index which is also fine
as it has the minimal amount of memory then, too

Thanks,
Stefan

Re: Implementing reftable in Git

[Carlos Martín Nieto]

On Wed, 2018-05-09 at 10:54 -0700, Jonathan Nieder wrote:
> Carlos Martín Nieto wrote:
> > On Wed, 2018-05-09 at 09:48 -0700, Jonathan Nieder wrote:
> > > If you would like the patches at https://git.eclipse.org/r/q/topi
> > > c:reftable
> > > relicensed for Git's use so that you don't need to include that
> > > license header, let me know.  Separate from any legal concerns,
> > > if
> > > you're doing a straight port, a one-line comment crediting the
> > > JGit
> > > project would still be appreciated, of course.
> 
> [...]
> > Would you expect that this port would keep the Eclipse Distribution
> > License or would it get relicensed to GPLv2?
> 
> I think you're way overcomplicating things.
> 
> The patches are copyright Google.  We can handle issues as they come.

Fair enough. I just wanted to avoid coming back to this in a few months
and realising we can't use it at all.

Cheers,
   cmn

Re: [PATCH] repository: fix free problem with repo_clear(the_repository)

[Duy Nguyen]

On Wed, May 9, 2018 at 7:50 PM, Stefan Beller  wrote:
>>  I was trying to test the new parsed_object_pool_clear() and found this.
>
> So this would go with the latest sb/object-store-alloc ?

No this should be separate because sb/object-store-alloc did not even
touch this code. I mistakenly thought you wrote this and sent to you.
I should have checked and sent to Brandon instead.

> My impression was that we never call repo_clear() on
> the_repository, which would allow us to special case
> the_repository further just as I did in v2 of that series[1] by
> having static allocations for certain objects in case of \
> the_repository.
>
> [1] https://public-inbox.org/git/20180501213403.14643-14-sbel...@google.com/
>
> We could just deal with all the exceptions, but that makes repo_clear
> ugly IMHO.
>
> I would rather special case the_repository even more instead
> of having it allocate all its things on the heap. (However we rather
> want to profile it and argue with data)

I'm actually going the opposite direction and trying hard to make
the_repository normal like everybody else :)

>>  repository.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/repository.c b/repository.c
>> index a4848c1bd0..f44733524a 100644
>> --- a/repository.c
>> +++ b/repository.c
>> @@ -238,7 +238,9 @@ void repo_clear(struct repository *repo)
>>
>> if (repo->index) {
>> discard_index(repo->index);
>> -   FREE_AND_NULL(repo->index);
>> +   if (repo->index != _index)
>> +   free(repo->index);
>> +   repo->index = NULL;
>
> So after this we have a "dangling" the_index.
> It is not really dangling, but it is not part of the_repository any more
> and many places still use the_index, it might make up for interesting
> bugs?

Hmm.. yeah, as a "clearing" operation, I probaly should not clear
repo->index. This way the_repository will be back in initial state and
could be reused again. Something like this perhaps?

discard_index(repo->index);
if (repo->index != _index)
FREE_AND_NULL(repo->index);

> What is your use case of repo_clear(the_repository)?

No actual use case right now. See [1] for the code that triggered
this. I do want to free some memory in pack-objects and repo_clear()
_might_ be the one. I'm not sure yet.

[1] https://gist.github.com/pclouds/86a2df6c28043f1b6fa3d4e72e7a1276
-- 
Duy

Re: Implementing reftable in Git

[Stefan Beller]

On Wed, May 9, 2018 at 10:48 AM, Jonathan Nieder  wrote:
> Stefan Beller wrote:
>
>> * We *might* be able to use reftables in negotiation later
>>   ("client: Last I fetched, you said your latest transaction
>>   number was '5' with the hash over all refs to be ;
>>   server: ok, here are the refs and the pack, you're welcome").
>
> Do you mean that reftable's reflog layout makes this easier?
>
> It's not clear to me why this wouldn't work with the current
> reflogs.

Because of D/F conflicts we may not know all remote refs
(and their ref logs), such that "the hash over all refs" on the remote
is error prone to compute. Without transaction numbers it is also
cumbersome for the server to remember the state.
We could try it based on the current refs, but I'd think
it is not easy to do, whereas reftables bring some subtle
advantages that allow for such easier negotiation.

>
> [...]
>> On Wed, May 9, 2018 at 7:33 AM, Christian Couder
>>  wrote:
>
>>> During the last Git Merge conference last March Stefan talked about
>>> reftable. In Alex Vandiver's notes [1] it is asked that people
>>> announce it on the list when they start working on it,
>>
>> Mostly because many parties want to see it implemnented
>> and were not sure when they could start implementing it.
>
> And to coordinate / help each other!

Yes. Usually open source contributions are so sparse, that
just doing it and then sending it to the mailing list does not
produce contention or conflict (double work), but this seemed
like a race condition waiting to happen. ;)

>> With that said, please implement it in a way that it can not just be used as
>> a refs backend, but can easily be re-used to write ref advertisements
>> onto the wire?
>
> Can you spell this out a little more for me?  At first glance it's not
> obvious to me how knowing about this potential use would affect the
> initial code.

Yeah me neither. I just want to make Christian aware of the potential
use cases, that come afterwards, so it can influence his design decisions
for the implementation.

Re: [PATCH] repository: fix free problem with repo_clear(the_repository)

[Duy Nguyen]

On Wed, May 9, 2018 at 7:42 PM, Elijah Newren  wrote:
> On Wed, May 9, 2018 at 10:04 AM, Nguyễn Thái Ngọc Duy  
> wrote:
>> the_repository is special. One of the special things about it is that
>> it does not allocate a new index_state object like submodules but
>> points to the global the_index variable instead. As a global variable,
>> the_index cannot be free()'d.
>>
>> Add an exception for this in repo_clear(). In the future perhaps we
>> would be able to allocate the_repository's index on heap too. Then we
>> can remove revert this.
>
> "remove revert"?

It's obvious that double negatives are below me. I'm going to the next
level with double positives! "remove" should be removed.
-- 
Duy

Re: Implementing reftable in Git

[Jonathan Nieder]

Carlos Martín Nieto wrote:
> On Wed, 2018-05-09 at 09:48 -0700, Jonathan Nieder wrote:

>> If you would like the patches at https://git.eclipse.org/r/q/topic:reftable
>> relicensed for Git's use so that you don't need to include that
>> license header, let me know.  Separate from any legal concerns, if
>> you're doing a straight port, a one-line comment crediting the JGit
>> project would still be appreciated, of course.
[...]
> Would you expect that this port would keep the Eclipse Distribution
> License or would it get relicensed to GPLv2?

I think you're way overcomplicating things.

The patches are copyright Google.  We can handle issues as they come.

Jonathan

Re: Implementing reftable in Git

[Carlos Martín Nieto]

Hi all,

On Wed, 2018-05-09 at 09:48 -0700, Jonathan Nieder wrote:
> Hi,
> 
> Christian Couder wrote:
> 
> > I might start working on implementing reftable in Git soon.
> 
> Yay!
> 
> [...]
> > So I think the most straightforward and compatible way to do it would
> > be to port the JGit implementation.
> 
> I suspect following the spec[1] would be even more compatible, since it
> would force us to tighten the spec where it is unclear.
> 
> >It looks like the
> > JGit repo and the reftable code there are licensed under the Eclipse
> > Distribution License - v 1.0 [7] which is very similar to the 3-Clause
> > BSD License also called Modified BSD License
> 
> If you would like the patches at https://git.eclipse.org/r/q/topic:reftable
> relicensed for Git's use so that you don't need to include that
> license header, let me know.  Separate from any legal concerns, if
> you're doing a straight port, a one-line comment crediting the JGit
> project would still be appreciated, of course.
> 
> That said, I would not be surprised if going straight from the spec is
> easier than porting the code.

Would you expect that this port would keep the Eclipse Distribution
License or would it get relicensed to GPLv2?

We would also want to have reftable functionality in the libgit2
project, but it has a slightly different license from git (GPLv2 with
linking exception) which requires explicit consent from the authors for
us to port over the code from git with its GPLv2 license.

The libgit2 project does have permission from Shawn to relicense his
git code, but this would presumably not cover this kind of porting. I
don't believe we would have issues if the code remained this BSD-like
license.

Sorry for being difficult, but fewer distinct reimplementations is
probably a good thing overall.

cc the core libgit2 team

Cheers,
   cmn

Re: [PATCH] repository: fix free problem with repo_clear(the_repository)

[Stefan Beller]

On Wed, May 9, 2018 at 10:04 AM, Nguyễn Thái Ngọc Duy  wrote:
> the_repository is special. One of the special things about it is that
> it does not allocate a new index_state object like submodules but
> points to the global the_index variable instead. As a global variable,
> the_index cannot be free()'d.

ok. That is the situation we're in.

>
> Add an exception for this in repo_clear(). In the future perhaps we
> would be able to allocate the_repository's index on heap too. Then we
> can remove revert this.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  I was trying to test the new parsed_object_pool_clear() and found this.

So this would go with the latest sb/object-store-alloc ?

My impression was that we never call repo_clear() on
the_repository, which would allow us to special case
the_repository further just as I did in v2 of that series[1] by
having static allocations for certain objects in case of \
the_repository.

[1] https://public-inbox.org/git/20180501213403.14643-14-sbel...@google.com/

We could just deal with all the exceptions, but that makes repo_clear
ugly IMHO.

I would rather special case the_repository even more instead
of having it allocate all its things on the heap. (However we rather
want to profile it and argue with data)

>  repository.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/repository.c b/repository.c
> index a4848c1bd0..f44733524a 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -238,7 +238,9 @@ void repo_clear(struct repository *repo)
>
> if (repo->index) {
> discard_index(repo->index);
> -   FREE_AND_NULL(repo->index);
> +   if (repo->index != _index)
> +   free(repo->index);
> +   repo->index = NULL;

So after this we have a "dangling" the_index.
It is not really dangling, but it is not part of the_repository any more
and many places still use the_index, it might make up for interesting
bugs?

What is your use case of repo_clear(the_repository)?

Thanks,
Stefan

Re: Implementing reftable in Git

[Jonathan Nieder]

Stefan Beller wrote:

> * We *might* be able to use reftables in negotiation later
>   ("client: Last I fetched, you said your latest transaction
>   number was '5' with the hash over all refs to be ;
>   server: ok, here are the refs and the pack, you're welcome").

Do you mean that reftable's reflog layout makes this easier?

It's not clear to me why this wouldn't work with the current
reflogs.

[...]
> On Wed, May 9, 2018 at 7:33 AM, Christian Couder
>  wrote:

>> During the last Git Merge conference last March Stefan talked about
>> reftable. In Alex Vandiver's notes [1] it is asked that people
>> announce it on the list when they start working on it,
>
> Mostly because many parties want to see it implemnented
> and were not sure when they could start implementing it.

And to coordinate / help each other!

[...]
> I volunteer for reviewing.

\o/

[...]
> With that said, please implement it in a way that it can not just be used as
> a refs backend, but can easily be re-used to write ref advertisements
> onto the wire?

Can you spell this out a little more for me?  At first glance it's not
obvious to me how knowing about this potential use would affect the
initial code.

Thanks,
Jonathan

Re: [PATCH] repository: fix free problem with repo_clear(the_repository)

[Elijah Newren]

On Wed, May 9, 2018 at 10:04 AM, Nguyễn Thái Ngọc Duy  wrote:
> the_repository is special. One of the special things about it is that
> it does not allocate a new index_state object like submodules but
> points to the global the_index variable instead. As a global variable,
> the_index cannot be free()'d.
>
> Add an exception for this in repo_clear(). In the future perhaps we
> would be able to allocate the_repository's index on heap too. Then we
> can remove revert this.

"remove revert"?

Re: Implementing reftable in Git

[Stefan Beller]

Hi Christian,

On Wed, May 9, 2018 at 7:33 AM, Christian Couder
 wrote:
> Hi,
>
> I might start working on implementing reftable in Git soon.

Cool! Everyone is waiting for it as they dream about the
performance and correctness benefits this brings.

Benefits that I know of:
* performance in repos with many refs
* no capitalization issues on case insensitive FS
* replay-ability of the last fetch ("show the last reflog
  of any ref under refs/remote/origin") is easier to do
  in a correct way. (This is one of my motivations to desire reftables)
* We *might* be able to use reftables in negotiation later
  ("client: Last I fetched, you said your latest transaction
  number was '5' with the hash over all refs to be ;
  server: ok, here are the refs and the pack, you're welcome").

Why are you (or rather booking.com) interested in this?

> During the last Git Merge conference last March Stefan talked about
> reftable. In Alex Vandiver's notes [1] it is asked that people
> announce it on the list when they start working on it,

Mostly because many parties want to see it implemnented
and were not sure when they could start implementing it.

> and it appears
> that there is a reference implementation in JGit.

The reference implementation can be used in tests
to see if we can interact with them, using the JGIT pre-requisite.

> Looking it up, there is indeed some documentation [2], code [3], tests
> [4] and other related stuff [5] in the JGit repo. It looks like the
> JGit repo and the reftable code there are licensed under the Eclipse
> Distribution License - v 1.0 [7] which is very similar to the 3-Clause
> BSD License also called Modified BSD License which is GPL compatible
> according to gnu.org [9]. So from a quick look it appears that I
> should be able to port the JGit to Git if I just keep the copyright
> and license header comments in all the related files.
>
> So I think the most straightforward and compatible way to do it would
> be to port the JGit implementation.

I would think you can go by the spec and then test if it is compatible with
JGit; that way the spec will be ironed out in corner cases.

> Thanks in advance for any suggestion or comment about this.

I volunteer for reviewing.

(Advanced:) The spec allows for some tune-able parameters and JGits use
is heavily optimized for the server side. I think git-core may need to have
slightly different tweaks in different situations, e.g. block sizes and how
many restarts are put into the block.
On the FS we may want to have faster access at the cost of more disk space,
whereas in the future when using reftables on the wire as well for ref
advertisement we may want to opt for smallest tables. (largest blocks,
no restarts)

With that said, please implement it in a way that it can not just be used as
a refs backend, but can easily be re-used to write ref advertisements
onto the wire?

Thanks,
Stefan

Re: [PATCH v5 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'

[Martin Ågren]

On 9 May 2018 at 12:41, Phillip Wood  wrote:
> On 09/05/18 03:13, Taylor Blau wrote:
>>
>>   +--column::
>> +   Prefix the 1-indexed byte-offset of the first match on non-context
>> lines. This
>> +   option is incompatible with '--invert-match', and extended
>> expressions.
>> +
>
>
> Sorry to be fussy, but while this is clearer I think to could be improved to
> make it clear that it is the offset from the start of the matching line.
> Also the mention of 'extended expressions' made me think of 'grep -E' but I
> think (correct me if I'm wrong) you mean the boolean options '--and',
> '--not' and '--or'. The man page only uses the word extended when talking
> about extended regexes. I think something like
>
> Print the 1-indexed byte-offset of the first match from the start of the
> matching line. This option is incompatible with '--invert-match', '--and',
> '--not' and '--or'.
>
> would be clearer

>> +   if (opt->columnnum && opt->extended)
>> +   die(_("--column and extended expressions cannot be 
>> combined"));
>> +

Just so it doesn't get missed: Phillip's comment (which I agree with)
about "extended" would apply here as well. This would work fine, no?

One thing to notice is that dying for `--column --not` in combination
with patch 7/7 makes git-jump unable to handle `--not` (and friends).
That would be a regression? I suppose git-jump could use a special
`--maybe-column` which would be "please use --column, unless I give you
something that won't play well with it". Or --column could do that kind
of falling back on its own. Or, git-jump could scan the arguments to
decide whether to use `--column` or not. Hmm... Tricky. :-/

Martin

Re: [PATCH] doc: fix config API documentation about config_with_options

[Brandon Williams]

On 05/09, Antonio Ospite wrote:
> In commit dc8441fdb ("config: don't implicitly use gitdir or commondir",
> 2017-06-14) the function git_config_with_options was renamed to
> config_with_options to better reflect the fact that it does not access
> the git global config or the repo config by default.
> 
> However Documentation/technical/api-config.txt still refers to the
> previous name, fix that.
> 
> While at it also update the documentation about the extra parameters,
> because they too changed since the initial definition.
> 
> Signed-off-by: Antonio Ospite 
> ---
> 
> Patch based on the maint branch.

Thanks for updating the docs.  Maybe one day we can migrate these docs
to the source files themselves, making it easier to keep up to date.
For now this is good :)

> 
> Ciao,
>Antonio
> 
>  Documentation/technical/api-config.txt | 18 ++
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/technical/api-config.txt 
> b/Documentation/technical/api-config.txt
> index 9a778b0ca..fa39ac9d7 100644
> --- a/Documentation/technical/api-config.txt
> +++ b/Documentation/technical/api-config.txt
> @@ -47,21 +47,23 @@ will first feed the user-wide one to the callback, and 
> then the
>  repo-specific one; by overwriting, the higher-priority repo-specific
>  value is left at the end).
>  
> -The `git_config_with_options` function lets the caller examine config
> +The `config_with_options` function lets the caller examine config
>  while adjusting some of the default behavior of `git_config`. It should
>  almost never be used by "regular" Git code that is looking up
>  configuration variables. It is intended for advanced callers like
>  `git-config`, which are intentionally tweaking the normal config-lookup
>  process. It takes two extra parameters:
>  
> -`filename`::
> -If this parameter is non-NULL, it specifies the name of a file to
> -parse for configuration, rather than looking in the usual files. Regular
> -`git_config` defaults to `NULL`.
> +`config_source`::
> +If this parameter is non-NULL, it specifies the source to parse for
> +configuration, rather than looking in the usual files. See `struct
> +git_config_source` in `config.h` for details. Regular `git_config` defaults
> +to `NULL`.
>  
> -`respect_includes`::
> -Specify whether include directives should be followed in parsed files.
> -Regular `git_config` defaults to `1`.
> +`opts`::
> +Specify options to adjust the behavior of parsing config files. See `struct
> +config_options` in `config.h` for details. As an example: regular 
> `git_config`
> +sets `opts.respect_includes` to `1` by default.
>  
>  Reading Specific Files
>  --
> -- 
> 2.17.0
> 

-- 
Brandon Williams

Re: [PATCH v3 13/13] alloc: allow arbitrary repositories for alloc functions

[Duy Nguyen]

On Tue, May 8, 2018 at 10:04 PM, Jonathan Tan  wrote:
> On Tue,  8 May 2018 12:37:36 -0700
> Stefan Beller  wrote:
>
>> +void clear_alloc_state(struct alloc_state *s)
>> +{
>> + while (s->slab_nr > 0) {
>> + s->slab_nr--;
>> + free(s->slabs[s->slab_nr]);
>> + }
>
> I should have caught this earlier, but you need to free s->slabs itself
> too.

And nobody frees 's' either. I'm not saying cler_alloc_state() should,
but somebody should. When I tried repo_clear(the_repository) with
gitster/sb/object-store-alloc I got this

==13250== 944 (32 direct, 912 indirect) bytes in 1 blocks are
definitely lost in loss record 62 of 88
==13250==at 0x4C2CF25: calloc (vg_replace_malloc.c:718)
==13250==by 0x1AB7A8: xcalloc (wrapper.c:160)
==13250==by 0x1BF666: allocate_alloc_state (alloc.c:41)
==13250==by 0x140090: parsed_object_pool_new (object.c:462)
==13250==by 0x16CCF4: initialize_the_repository (repository.c:18)
==13250==by 0x110BD0: main (common-main.c:37)

If you want to reproduce, this is what I used to test this with.

https://gist.github.com/pclouds/86a2df6c28043f1b6fa3d4e72e7a1276
-- 
Duy

Re: [PATCH] repository: fix free problem with repo_clear(the_repository)

[Brandon Williams]

On 05/09, Nguyễn Thái Ngọc Duy wrote:
> the_repository is special. One of the special things about it is that
> it does not allocate a new index_state object like submodules but
> points to the global the_index variable instead. As a global variable,
> the_index cannot be free()'d.
> 
> Add an exception for this in repo_clear(). In the future perhaps we
> would be able to allocate the_repository's index on heap too. Then we
> can remove revert this.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  I was trying to test the new parsed_object_pool_clear() and found this.

This looks good and I do hope we can get to a state soon where we can
not have to special case the_repository.

> 
>  repository.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/repository.c b/repository.c
> index a4848c1bd0..f44733524a 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -238,7 +238,9 @@ void repo_clear(struct repository *repo)
>  
>   if (repo->index) {
>   discard_index(repo->index);
> - FREE_AND_NULL(repo->index);
> + if (repo->index != _index)
> + free(repo->index);
> + repo->index = NULL;
>   }
>  }
>  
> -- 
> 2.17.0.705.g3525833791
> 

-- 
Brandon Williams

Re: [PATCHv2 0/4] Rebooting pc/submodule-helper-foreach

[Jonathan Tan]

On Tue,  8 May 2018 17:29:48 -0700
Stefan Beller  wrote:

> v2:
> * rebased onto origin/master
> * dropped leftover "toplevel" variable from experimentation
> * reworded the commit message for the first patch extensively
> * dropped the third patch
> * see "branch-diff" below.

Patches 1-3 look good to me.

I also can't see anything wrong with patch 4, but I am not an expert at
shell and how we call it from C, so a review from another reviewer would
be appreciated.

Re: [PATCH 4/5] lock_file: make function-local locks non-static

[Martin Ågren]

On 9 May 2018 at 18:19, Duy Nguyen  wrote:
> On Tue, May 8, 2018 at 8:18 PM, Jeff King  wrote:

>> It should be totally safe. If you look at "struct lock_file", it is now
>> simply a pointer to a tempfile allocated on the heap (in fact, I thought
>> about getting rid of lock_file entirely, but the diff is noisy and it
>> actually has some value as an abstraction over a pure tempfile).
>
> Ah.. I did miss that "everything on heap" thing. Sorry for the noise
> Martin and thanks for clarification Jeff :)

Hey, no problem. In fact, the "noise" as you call it had some signal in
it: the commit messages should obviously say more about this.

Martin

[PATCH] repository: fix free problem with repo_clear(the_repository)

[Nguyễn Thái Ngọc Duy]

the_repository is special. One of the special things about it is that
it does not allocate a new index_state object like submodules but
points to the global the_index variable instead. As a global variable,
the_index cannot be free()'d.

Add an exception for this in repo_clear(). In the future perhaps we
would be able to allocate the_repository's index on heap too. Then we
can remove revert this.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 I was trying to test the new parsed_object_pool_clear() and found this.

 repository.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/repository.c b/repository.c
index a4848c1bd0..f44733524a 100644
--- a/repository.c
+++ b/repository.c
@@ -238,7 +238,9 @@ void repo_clear(struct repository *repo)
 
if (repo->index) {
discard_index(repo->index);
-   FREE_AND_NULL(repo->index);
+   if (repo->index != _index)
+   free(repo->index);
+   repo->index = NULL;
}
 }
 
-- 
2.17.0.705.g3525833791

Re: [PATCH v1] add status config and command line options for rename detection

[Ben Peart]

On 5/9/2018 11:59 AM, Duy Nguyen wrote:

On Wed, May 9, 2018 at 4:42 PM, Ben Peart  wrote:

Add a new config status.renames setting to enable turning off rename detection
during status.  This setting will default to the value of diff.renames.


Please add the reason you need this config key in the commit message.
My guess (probably correct) is on super large repo (how large?),
rename detection is just too slow (how long?) that it practically
makes git-status unusable.



Yes, the reasons for this change are the same as for the patch that 
added these same flags for merge and have to do with the poor 
performance of rename detection with large repos.  I'll update the 
commit message to be more descriptive (see below) and correct some 
spelling errors.



add status config and command line options for rename detection

After performing a merge that has conflicts, git status will by default 
attempt to detect renames which causes many objects to be examined.  In 
a virtualized repo, those objects do not exist locally so the rename 
logic triggers them to be fetched from the server. This results in the 
status call taking hours to complete on very large repos.  Even in a 
small repo (the GVFS repo) turning off break and rename detection has a 
significant impact:


git status --no-renames:
31 secs., 105 loose object downloads

git status --no-breaks
7 secs., 17 loose object downloads

git status --no-breaks --no-renames
1 sec., 1 loose object download

Add a new config status.renames setting to enable turning off rename 
detection during status.  This setting will default to the value of 
diff.renames.


Add a new config status.renamelimit setting to to enable bounding the 
time spent finding out inexact renames during status.  This setting will 
default to the value of diff.renamelimit.


Add status --no-renames command line option that enables overriding the 
config setting from the command line. Add --find-renames[=] to enable 
detecting renames and optionally setting the similarity index from the 
command line.


Note: I removed the --no-breaks command line option from the original 
patch as it will no longer be needed once the default has been changed 
[1] to turn it off.


[1] 
https://public-inbox.org/git/20180430093421.27551-2-eckhard.s.ma...@gmail.com/


Original-Patch-by: Alejandro Pauly 
Signed-off-by: Ben Peart 



This information could be helpful when we optimize rename detection to
be more efficient.



Add a new config status.renamelimit setting to to enable bounding the time spent
finding out inexact renames during status.  This setting will default to the
value of diff.renamelimit.

Add status --no-renames command line option that enables overriding the config
setting from the command line. Add --find-renames[=] to enable detecting
renames and optionaly setting the similarity index from the command line.

Origional-Patch-by: Alejandro Pauly 
Signed-off-by: Ben Peart 

[PATCH] BUG_exit_code: fix sparse "symbol not declared" warning

[Ramsay Jones]

Signed-off-by: Ramsay Jones 
---

Hi Johannes,

If you need to re-roll your 'js/use-bug-macro' branch, could you
please squash this into the relevant patch (commit a86303cb5d,
"test-tool: help verifying BUG() code paths", 2018-05-02).

This will, obviously, not be required if you were to implement
Jeff's suggestion (in [1]) of using an environment variable
instead. (which, FWIW, I prefer).

Thanks!

ATB,
Ramsay Jones

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

 git-compat-util.h| 3 +++
 t/helper/test-tool.c | 1 -
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 5a5a99af7..b7883b257 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1116,6 +1116,9 @@ static inline int regexec_buf(const regex_t *preg, const 
char *buf, size_t size,
 #define HAVE_VARIADIC_MACROS 1
 #endif
 
+/* usage.c: only to be used for testing BUG() implementation (see test-tool) */
+extern int BUG_exit_code;
+
 #ifdef HAVE_VARIADIC_MACROS
 __attribute__((format (printf, 3, 4))) NORETURN
 void BUG_fl(const char *file, int line, const char *fmt, ...);
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 5176f9f20..805a45de9 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -47,7 +47,6 @@ static struct test_cmd cmds[] = {
 int cmd_main(int argc, const char **argv)
 {
int i;
-   extern int BUG_exit_code;
 
BUG_exit_code = 99;
if (argc < 2)
-- 
2.17.0

Re: [PATCH v1] add status config and command line options for rename detection

[Elijah Newren]

Hi Ben,

Overall I think this is good, but I have lots of nit-picky things to
bring up.  :-)


On Wed, May 9, 2018 at 7:42 AM, Ben Peart  wrote:
> Add status --no-renames command line option that enables overriding the config
> setting from the command line. Add --find-renames[=] to enable detecting
> renames and optionaly setting the similarity index from the command line.

s/optionaly/optionally/

> Notes:
> Base Ref:

No base ref?  ;-)

> +status.renameLimit::
> +   The number of files to consider when performing rename detection;
> +   if not specified, defaults to the value of diff.renameLimit.
> +
> +status.renames::
> +   Whether and how Git detects renames.  If set to "false",
> +   rename detection is disabled. If set to "true", basic rename
> +   detection is enabled.  Defaults to the value of diff.renames.

I suspect that status.renames should mention "copy", just as
diff.renames does.  (We didn't mention it in merge.renames, because
merge isn't an operation for which copy detection can be useful -- at
least not until the diffstat at the end of the merge is controlled by
merge.renames as well...)

Also, do these two config settings only affect 'git status', or does
it also affect the status shown when composing a commit message
(assuming the user hasn't turned commit.status off)?  Does it affect
`git commit --dry-run` too?

> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -109,6 +109,10 @@ static int have_option_m;
>  static struct strbuf message = STRBUF_INIT;
>
>  static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
> +static int diff_detect_rename = -1;
> +static int status_detect_rename = -1;
> +static int diff_rename_limit = -1;
> +static int status_rename_limit = -1;

Could we replace these four variables with just two: detect_rename and
rename_limit?  Keeping these separate invites people to write code
using only one of the settings rather than the appropriate inherited
mixture of them, resulting in a weird bug.  More on this below...

> @@ -1259,11 +1273,29 @@ static int git_status_config(const char *k, const 
> char *v, void *cb)
> return error(_("Invalid untracked files mode '%s'"), 
> v);
> return 0;
> }
> +   if (!strcmp(k, "diff.renamelimit")) {
> +   diff_rename_limit = git_config_int(k, v);
> +   return 0;
> +   }
> +   if (!strcmp(k, "status.renamelimit")) {
> +   status_rename_limit = git_config_int(k, v);
> +   return 0;
> +   }

Here, since you're already checking diff.renamelimit first, you can
just set rename_limit in both blocks and not need both
diff_rename_limit and status_rename_limit.  (Similar can be said for
diff.renames/status.renames.)



> @@ -1297,6 +1329,10 @@ int cmd_status(int argc, const char **argv, const char 
> *prefix)
>   N_("ignore changes to submodules, optional when: all, 
> dirty, untracked. (Default: all)"),
>   PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
> OPT_COLUMN(0, "column", , N_("list untracked files 
> in columns")),
> +   OPT_BOOL(0, "no-renames", _renames, N_("do not detect 
> renames")),
> +   { OPTION_CALLBACK, 'M', "find-renames", _score_arg,
> + N_("n"), N_("detect renames, optionally set similarity 
> index"),
> + PARSE_OPT_OPTARG, opt_parse_rename_score },

Should probably also document these options in
Documentation/git-status.txt (and maybe Documentation/git-commit.txt
as well).

Not sure if we want to add a flag for copy detection (similar to
git-diff's -C/--find-copies and --find-copies-harder), or just leave
that for when someone finds a need.  If left out, might want to just
mention that it was considered and intentionally omitted for now in
the commit message.

> @@ -1336,6 +1372,27 @@ int cmd_status(int argc, const char **argv, const char 
> *prefix)
> s.ignore_submodule_arg = ignore_submodule_arg;
> s.status_format = status_format;
> s.verbose = verbose;
> +   s.detect_rename = no_renames >= 0 ? !no_renames :
> + status_detect_rename >= 0 ? 
> status_detect_rename :
> + diff_detect_rename >= 0 ? 
> diff_detect_rename :

Combining the four vars into two as mentioned above would allow
combining the last two lines above into one.

> +   if ((intptr_t)rename_score_arg != -1) {

I don't understand why rename_score_arg is a (char*) and then you need
to cast to intptr_t, but that may just be because I haven't done much
of anything with option parsing.  A quick look at the docs isn't
making it clear to me, though; could you enlighten me?

> +   s.detect_rename = DIFF_DETECT_RENAME;

What if status.renames is 'copy' but someone wants to override the
score for detecting renames and pass --find-renames=40?  Does the
--find-renames 

Re: regarding fix on "git clone $there $here"

[Leslie Wang]

Thanks for the confirmation. It is very helpful!

Best Regards
Leslie Wang



> On May 8, 2018, at 11:44 PM, Junio C Hamano  wrote:
> 
> Leslie Wang  writes:
> 
>> At 2.14.1 or 2.15.1, if I run command like 
>> - mkdir /tmp/111
>> - git clone g...@github.com:111/111 /tmp/111
>> 
>> because it will failure, then /tmp/111 will be removed automatically.
> 
> Yes, this was a (longstanding) bug that nobody bothered to fix for a
> long time.  As "git clone" did not create /tmp/111 but it was given
> to it by the external world, it shouldn't remove it upon failure.
> Of course, if you omit the "mkdir" in the above sequence and let
> "git clone" create /tmp/111, tne Git will remove it upon failure as
> part of the clean-up.
> 

Re: Implementing reftable in Git

[Jonathan Nieder]

Hi,

Christian Couder wrote:

> I might start working on implementing reftable in Git soon.

Yay!

[...]
> So I think the most straightforward and compatible way to do it would
> be to port the JGit implementation.

I suspect following the spec[1] would be even more compatible, since it
would force us to tighten the spec where it is unclear.

>It looks like the
> JGit repo and the reftable code there are licensed under the Eclipse
> Distribution License - v 1.0 [7] which is very similar to the 3-Clause
> BSD License also called Modified BSD License

If you would like the patches at https://git.eclipse.org/r/q/topic:reftable
relicensed for Git's use so that you don't need to include that
license header, let me know.  Separate from any legal concerns, if
you're doing a straight port, a one-line comment crediting the JGit
project would still be appreciated, of course.

That said, I would not be surprised if going straight from the spec is
easier than porting the code.

Thanks,
Jonathan

[1] 
https://eclipse.googlesource.com/jgit/jgit/+/master/Documentation/technical/reftable.md

Re: Can not save changes into stash

[Torsten Bögershausen]

[Please no top posting here]
On 09.05.18 15:27, KES wrote:
> I even can not drop local changes:
> 
> $ git checkout local.conf 
> error: pathspec 'local.conf' did not match any file(s) known to git.
> 
> $ git log local.conf
> commit 6df8bab88fd703c6859954adc51b2abaad8f59ec
> Author: Eugen Konkov 
> Date:   Wed May 9 15:31:02 2018 +0300
> 
> Implement module which allow override target option
> 
> Most usefull to configure application on developer host
> 
> 
It may be help, if we have some more information,
either to re-produce your issue or to help with debugging.

Is this a public repo ?
Which Git version do you use ?
Which OS (Linux, Mac OS, Windows anything else ?)
What does "git status" say ?
What does "git diff" say ?

> 
> 09.05.2018, 16:25, "KES" :
>> How to reproduce:
>>
>> $ git update-index --skip-worktree conf/local.conf
>> $ git pull
>> Updating 0cd50c7..bde58f8
>> error: Your local changes to the following files would be overwritten by 
>> merge:
>> conf/local.conf
>> Please commit your changes or stash them before you merge.
>> Aborting
>> $ git stash save
>> No local changes to save

Re: [PATCH 02/18] Add a new builtin: branch-diff

[Ramsay Jones]

On 05/05/18 20:41, Johannes Schindelin wrote:
[snip]

[Sorry for the late reply - still catching up after (long weekend)
UK public holiday]

> Well, what I would want to do is let the cloud work for me. By adding an
> automated build to my Visual Studio Team Services (VSTS) account, of
> course, as I have "cloud privilege" (i.e. I work in the organization
> providing the service, so I get to play with all of it for free).
> 
> So I really don't want to build sparse every time a new revision needs to
> be tested (whether that be from one of my branches, an internal PR for
> pre-review of patches to be sent to the mailing list, or maybe even `pu`
> or the personalized branches on https://github.com/gitster/git).
> 
> I really would need a ready-to-install sparse, preferably as light-weight
> as possible (by not requiring any dependencies outside what is available
> in VSTS' hosted Linux build agents.
> 
> Maybe there is a specific apt source for sparse?

Not that I'm aware of, sorry! :(

[release _source_ tar-balls are available, but that's not
what you are after, right?]

I don't know what is involved in setting up a 'ppa repo' for
Ubuntu, which I suspect is the kind of thing you want, but it
would have helped me several times in the past (so that I could
have something to point people to) ... ;-)

ATB,
Ramsay Jones

Re: [PATCH 4/5] lock_file: make function-local locks non-static

[Duy Nguyen]

On Tue, May 8, 2018 at 8:18 PM, Jeff King  wrote:
> On Mon, May 07, 2018 at 05:24:05PM +0200, Duy Nguyen wrote:
>
>>  -   static struct lock_file lock;
>>  +   struct lock_file lock = LOCK_INIT;
>> >>>
>> >>> Is it really safe to do this? I vaguely remember something about
>> >>> (global) linked list and signal handling which could trigger any time
>> >>> and probably at atexit() time too (i.e. die()). You don't want to
>> >>> depend on stack-based variables in that case.
>> >>
>> >> So I dug in a bit more about this. The original implementation does
>> >> not allow stack-based lock files at all in 415e96c8b7 ([PATCH]
>> >> Implement git-checkout-cache -u to update stat information in the
>> >> cache. - 2005-05-15). The situation has changed since 422a21c6a0
>> >> (tempfile: remove deactivated list entries - 2017-09-05). At the end
>> >> of that second commit, Jeff mentioned "We can clean them up
>> >> individually" which I guess is what these patches do. Though I do not
>> >> know if we need to make sure to call "release" function or something/
>> >> Either way you need more explanation and assurance than just "we can
>> >> drop their staticness" in the commit mesage.
>> >
>> > Thank you Duy for your comments. How about I write the commit message
>> > like so:
>>
>> +Jeff. Since he made it possible to remove lock file from the global
>> linked list, he probably knows well what to check when switching from
>> a static lock file to a stack-local one.
>
> It should be totally safe. If you look at "struct lock_file", it is now
> simply a pointer to a tempfile allocated on the heap (in fact, I thought
> about getting rid of lock_file entirely, but the diff is noisy and it
> actually has some value as an abstraction over a pure tempfile).
>
> If you fail to call a release function, it will just hang around until
> program exit, which is more or less what the static version would do.
> The big difference is that if we re-enter the function while still
> holding the lock, then the static version would BUG() on trying to use
> the already-active lockfile. Whereas after this series, we'd try to
> create a new lockfile and say "woah, somebody else is holding the lock".

Ah.. I did miss that "everything on heap" thing. Sorry for the noise
Martin and thanks for clarification Jeff :)
-- 
Duy

Re: [PATCH v5 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'

[Duy Nguyen]

On Wed, May 9, 2018 at 4:13 AM, Taylor Blau  wrote:
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 5f32d2ce84..f9f516dfc4 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -829,6 +829,7 @@ int cmd_grep(int argc, const char **argv, const char 
> *prefix)
> GREP_PATTERN_TYPE_PCRE),
> OPT_GROUP(""),
> OPT_BOOL('n', "line-number", , N_("show line 
> numbers")),
> +   OPT_BOOL(0, "column", , N_("show column number 
> of first match")),

Two things to consider:

- do we ever want columnar output in git-grep? Something like "git
grep --column -l" could make sense (if you don't have very large
worktree). --column is currently used for column output in git-branch,
git-tag and git-status, which makes me think maybe we should reserve
"--column" for that purpose and use another name here, even if we
don't ever want column output in git-grep, for consistency.

- If this is to help git-jump only and rarely manually specified on
command line, you could add the flag PARSE_OPT_NOCOMPLETE to hide it
from "git grep --" completion. You would need to use OPT_BOOL_F()
instead of OPT_BOOL() in order to add extra flags.
-- 
Duy

Re: Implementing reftable in Git

[Duy Nguyen]

On Wed, May 9, 2018 at 4:33 PM, Christian Couder
 wrote:
> Hi,
>
> I might start working on implementing reftable in Git soon.

Adding Michael Haggerty who did lots of work on ref stuff. He probably
can give a few suggestions.

You probably should also look at the last attempt to add lmdb as a new
ref backend. I'm not sure why it's still not in, maybe it wasn't the
right time (e.g. infrastructure was not ready).

> During the last Git Merge conference last March Stefan talked about
> reftable. In Alex Vandiver's notes [1] it is asked that people
> announce it on the list when they start working on it, and it appears
> that there is a reference implementation in JGit.
>
> Looking it up, there is indeed some documentation [2], code [3], tests
> [4] and other related stuff [5] in the JGit repo. It looks like the
> JGit repo and the reftable code there are licensed under the Eclipse
> Distribution License - v 1.0 [7] which is very similar to the 3-Clause
> BSD License also called Modified BSD License which is GPL compatible
> according to gnu.org [9]. So from a quick look it appears that I
> should be able to port the JGit to Git if I just keep the copyright
> and license header comments in all the related files.
>
> So I think the most straightforward and compatible way to do it would
> be to port the JGit implementation.
>
> Thanks in advance for any suggestion or comment about this.
>
> Reftable was first described by Shawn and then discussed last July on
> the list [6].
>
> My work on this would be sponsored by Booking.com.
>
> Thanks,
> Christian.
>
> [1] 
> https://public-inbox.org/git/alpine.DEB.2.20.1803091557510.23109@alexmv-linux/
>
> [2] 
> https://github.com/eclipse/jgit/blob/master/Documentation/technical/reftable.md
>
> [3] 
> https://github.com/eclipse/jgit/tree/master/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable
>
> [4] 
> https://github.com/eclipse/jgit/tree/master/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/reftable
>
> [5] 
> https://github.com/eclipse/jgit/tree/master/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/debug
>
> [6] 
> https://public-inbox.org/git/CAJo=hJtyof=HRy=2sLP0ng0uZ4=s-dpz5dr1af+vhvetkg2...@mail.gmail.com/
>
> [7] http://www.eclipse.org/org/documents/edl-v10.php
>
> [8] https://opensource.org/licenses/BSD-3-Clause
>
> [9] https://www.gnu.org/licenses/license-list.en.html#ModifiedBSD
-- 
Duy

Re: [PATCH 0/1] Fix UX issue with commit-graph.lock

[Duy Nguyen]

On Wed, May 9, 2018 at 4:15 PM, Derrick Stolee  wrote:
> We use the lockfile API to avoid multiple Git processes from writing to
> the commit-graph file in the .git/objects/info directory. In some cases,
> this directory may not exist, so we check for its existence.
>
> The existing code does the following when acquiring the lock:
>
> 1. Try to acquire the lock.
> 2. If it fails, try to create the .git/object/info directory.
> 3. Try to acquire the lock, failing if necessary.
>
> The problem is that if the lockfile exists, then the mkdir fails, giving
> an error that doesn't help the user:
>
>   "fatal: cannot mkdir .git/objects/info: File exists"
>
> While technically this honors the lockfile, it does not help the user.
>
> Instead, do the following:
>
> 1. Check for existence of .git/objects/info; create if necessary.
> 2. Try to acquire the lock, failing if necessary.
>
> The new output looks like:
>
>   fatal: Unable to create 
> '/home/stolee/git/.git/objects/info/commit-graph.lock': File exists.
>
>   Another git process seems to be running in this repository, e.g.
>   an editor opened by 'git commit'. Please make sure all processes
>   are terminated then try again. If it still fails, a git process
>   may have crashed in this repository earlier:
>   remove the file manually to continue.

This to me is a much better description than the current commit
message in 1/1 and probably should be the commit message of 1/1.
-- 
Duy

Is rebase --force-rebase any different from rebase --no-ff?

[Ilya Kantor]

Right now in "git help rebase" for --no-ff:
"Without --interactive, this is a synonym for --force-rebase."

But *with* --interactive, is there any difference?

After doing some tests and looking in the source I couldn't find any
difference between those two at all.

Probably, there was a difference some time ago, but not now?

Then one of them can be safely deprecated.

---
Best Regards,
Ilya Kantor

Re: [PATCH v1] add status config and command line options for rename detection

[Duy Nguyen]

On Wed, May 9, 2018 at 4:42 PM, Ben Peart  wrote:
> Add a new config status.renames setting to enable turning off rename detection
> during status.  This setting will default to the value of diff.renames.

Please add the reason you need this config key in the commit message.
My guess (probably correct) is on super large repo (how large?),
rename detection is just too slow (how long?) that it practically
makes git-status unusable.

This information could be helpful when we optimize rename detection to
be more efficient.

>
> Add a new config status.renamelimit setting to to enable bounding the time 
> spent
> finding out inexact renames during status.  This setting will default to the
> value of diff.renamelimit.
>
> Add status --no-renames command line option that enables overriding the config
> setting from the command line. Add --find-renames[=] to enable detecting
> renames and optionaly setting the similarity index from the command line.
>
> Origional-Patch-by: Alejandro Pauly 
> Signed-off-by: Ben Peart 
-- 
Duy

Re: [PATCH] wrap-for-bin.sh: facilitate running Git executables under valgrind

[Antonio Ospite]

On Wed, 9 May 2018 08:25:21 -0700
Elijah Newren  wrote:

> Hi Antonio,
> 

Hi Elijah,

> On Wed, May 9, 2018 at 6:28 AM, Antonio Ospite  wrote:
> > Testing locally built git executables under valgrind is not immediate.
> >
> > Something like the following does not work:
> >
> >   $ valgrind ./bin-wrappers/git
> >
> > because the wrapper script forks and execs the command and valgrind does
> > not track children processes by default.
> >
> > Something like the following may work:
> >
> >   $ valgrind --trace-children=yes ./bin-wrappers/git
> >
> > However it's counterintuitive and not ideal anyways because valgrind is
> > supposed to be called on the actual executable, not on wrapper scripts.
> >
> > So, following the idea from commit 6a94088cc ("test: facilitate
> > debugging Git executables in tests with gdb", 2015-10-30) provide
> > a mechanism in the wrapper script to call valgrind directly on the
> > actual executable.
> >
> > This mechanism could even be used by the test infrastructure in the
> > future, but it is already useful by its own on the command line:
> >
> >   $ GIT_TEST_VALGRIND=1 \
> > GIT_VALGRIND_OPTIONS="--leak-check=full" \
> > ./bin-wrappers/git
> >
> 
> Wow, timing; nice to see someone else finds this kind of thing useful.
> 
> I submitted something very similar recently; see commit 842436466aa5
> ("Make running git under other debugger-like programs easy",
> 2018-04-24) from next, or the discussion at
> https://public-inbox.org/git/20180424234645.8735-1-new...@gmail.com/.
> That other patch has the advantage of enabling the user to run git
> under other debugger-like programs besides just gdb and valgrind.
> 

Thanks Elijah, I am not subscribed to the list so I didn't see your
change and I usually only track the master branch.

Obviously your changes work for me, so I am dropping my patch.

As the changes in 842436466aa5 ("Make running git under other
debugger-like programs easy", 2018-04-24) are not specific to valgrind
they should also address Jeff's concerns in the sense that it's up to
the particular GIT_DEBUGGER how it handles sub-processes.

In valgrind case one may still want to pass "--trace-children=yes" in
GIT_DEBUGGER after all for better coverage. Thank you Jeff for the
remark.

Ciao,
   Antonio

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [PATCH v3 13/13] alloc: allow arbitrary repositories for alloc functions

[Duy Nguyen]

On Tue, May 8, 2018 at 10:37 PM, Stefan Beller  wrote:
>>> +void release_tree_node(struct tree *t);
>>> +void release_commit_node(struct commit *c);
>>> +void release_tag_node(struct tag *t);
>>
>> Do these really need to be defined in alloc.c? I would think that it
>> would be sufficient to define them as static in object.c.
>>
>> Having said that, opinions differ (e.g. Duy said he thinks that release_
>> goes with alloc_ [1]) so I'm OK either way.
>
> I would have preferred static as well, but went with Duys suggestion of
> having it in alloc.c.
>
> I can change that.

Heh I thought you would make them static ;-) I just wanted to keep
release logic outside that object pool, which is clearer and also
makes it easier to replace it with mem-pool.c later. I'm ok with
making it static. Or if you do export these, please move them close to
the parse_* functions where memory is actually allocated. E.g.
release_commit_node() is moved to commit.c, close to
parse_commit_gently(), release_tree_node() close to
parse_tree_gently().
-- 
Duy

Re: [PATCH] wrap-for-bin.sh: facilitate running Git executables under valgrind

[Elijah Newren]

Hi Antonio,

On Wed, May 9, 2018 at 6:28 AM, Antonio Ospite  wrote:
> Testing locally built git executables under valgrind is not immediate.
>
> Something like the following does not work:
>
>   $ valgrind ./bin-wrappers/git
>
> because the wrapper script forks and execs the command and valgrind does
> not track children processes by default.
>
> Something like the following may work:
>
>   $ valgrind --trace-children=yes ./bin-wrappers/git
>
> However it's counterintuitive and not ideal anyways because valgrind is
> supposed to be called on the actual executable, not on wrapper scripts.
>
> So, following the idea from commit 6a94088cc ("test: facilitate
> debugging Git executables in tests with gdb", 2015-10-30) provide
> a mechanism in the wrapper script to call valgrind directly on the
> actual executable.
>
> This mechanism could even be used by the test infrastructure in the
> future, but it is already useful by its own on the command line:
>
>   $ GIT_TEST_VALGRIND=1 \
> GIT_VALGRIND_OPTIONS="--leak-check=full" \
> ./bin-wrappers/git
>

Wow, timing; nice to see someone else finds this kind of thing useful.

I submitted something very similar recently; see commit 842436466aa5
("Make running git under other debugger-like programs easy",
2018-04-24) from next, or the discussion at
https://public-inbox.org/git/20180424234645.8735-1-new...@gmail.com/.
That other patch has the advantage of enabling the user to run git
under other debugger-like programs besides just gdb and valgrind.

Hope that helps,
Elijah

Re: [PATCH 1/1] commit-graph: fix UX issue when .lock file exists

[Derrick Stolee]

On 5/9/2018 10:42 AM, Jeff King wrote:

On Wed, May 09, 2018 at 02:15:38PM +, Derrick Stolee wrote:


The commit-graph file lives in the .git/objects/info directory.
Previously, a failure to acquire the commit-graph.lock file was
assumed to be due to the lack of the info directory, so a mkdir()
was called. This gave incorrect messaging if instead the lockfile
was open by another process:

   "fatal: cannot mkdir .git/objects/info: File exists"

Rearrange the expectations of this directory existing to avoid
this error, and instead show the typical message when a lockfile
already exists.

Sounds like a reasonable bug fix.

Your cover letter is way longer than this description. Should some of
that background perhaps go in the commit message?


I did want a place to include the full die() message in the new 
behavior, but that seemed like overkill for the commit message.



(I would go so far as to say that sending a cover letter for a single
patch is an anti-pattern, because the commit message should be able to
stand on its own).


@@ -754,23 +755,14 @@ void write_commit_graph(const char *obj_dir,
compute_generation_numbers();
  
  	graph_name = get_commit_graph_filename(obj_dir);

-   fd = hold_lock_file_for_update(, graph_name, 0);
  
-	if (fd < 0) {

-   struct strbuf folder = STRBUF_INIT;
-   strbuf_addstr(, graph_name);
-   strbuf_setlen(, strrchr(folder.buf, '/') - folder.buf);
-
-   if (mkdir(folder.buf, 0777) < 0)
-   die_errno(_("cannot mkdir %s"), folder.buf);
-   strbuf_release();
-
-   fd = hold_lock_file_for_update(, graph_name, 
LOCK_DIE_ON_ERROR);
-
-   if (fd < 0)
-   die_errno("unable to create '%s'", graph_name);
-   }
+   strbuf_addstr(, graph_name);
+   strbuf_setlen(, strrchr(folder.buf, '/') - folder.buf);
+   if (!file_exists(folder.buf) && mkdir(folder.buf, 0777) < 0)
+   die_errno(_("cannot mkdir %s"), folder.buf);
+   strbuf_release();

The result is racy if somebody else is trying to create the directory at
the same time. For that you'd want to notice EEXIST coming from mkdir
and consider that a success.

I think you probably ought to be calling adjust_shared_perm() on the
result, too, in case core.sharedrepository is configured.

If you use safe_create_leading_directories(), it should handle both.
Something like:

   if (safe_create_leading_directories(graph_name))
die_errno(_("unable to create leading directories of %s"),
  graph_name));

I think I'm holding it right; that function is a little short on
documentation, but it's the standard way to do this in Git's codebase,
and you can find lots of example callers.


Thanks for this method. I was unfamiliar with it. Saves the effort of 
creating the strbuf, too.


Thanks,
-Stolee

Re: Implementing reftable in Git

[Derrick Stolee]

On 5/9/2018 10:33 AM, Christian Couder wrote:

Hi,

I might start working on implementing reftable in Git soon.

During the last Git Merge conference last March Stefan talked about
reftable. In Alex Vandiver's notes [1] it is asked that people
announce it on the list when they start working on it, and it appears
that there is a reference implementation in JGit.


Thanks for starting on this! In addition to the performance gains, this 
will help a lot of users with case-insensitive file systems from getting 
case-errors on refnames.



Looking it up, there is indeed some documentation [2], code [3], tests
[4] and other related stuff [5] in the JGit repo. It looks like the
JGit repo and the reftable code there are licensed under the Eclipse
Distribution License - v 1.0 [7] which is very similar to the 3-Clause
BSD License also called Modified BSD License which is GPL compatible
according to gnu.org [9]. So from a quick look it appears that I
should be able to port the JGit to Git if I just keep the copyright
and license header comments in all the related files.

So I think the most straightforward and compatible way to do it would
be to port the JGit implementation.

Thanks in advance for any suggestion or comment about this.

Reftable was first described by Shawn and then discussed last July on
the list [6].


The hope is that such a direct port should be possible, but someone else 
should comment on the porting process.


This is also something that could be created independently based on the 
documentation you mention. I was planning to attempt that during a 
hackathon in July, but I'm happy you are able to start earlier (and that 
you are announcing your intentions). I would be happy to review your 
patch series, so please keep me posted.


Thanks,
-Stolee

Re: [PATCH] wrap-for-bin.sh: facilitate running Git executables under valgrind

[Jeff King]

On Wed, May 09, 2018 at 03:28:58PM +0200, Antonio Ospite wrote:

> Testing locally built git executables under valgrind is not immediate.
> 
> Something like the following does not work:
> 
>   $ valgrind ./bin-wrappers/git
> 
> because the wrapper script forks and execs the command and valgrind does
> not track children processes by default.
> 
> Something like the following may work:
> 
>   $ valgrind --trace-children=yes ./bin-wrappers/git
> 
> However it's counterintuitive and not ideal anyways because valgrind is
> supposed to be called on the actual executable, not on wrapper scripts.
> 
> So, following the idea from commit 6a94088cc ("test: facilitate
> debugging Git executables in tests with gdb", 2015-10-30) provide
> a mechanism in the wrapper script to call valgrind directly on the
> actual executable.

Unfortunately this isn't quite enough to get full valgrind coverage,
because Git often execs sub-processes of itself (and for anything that
isn't a builtin, all you're checking is the outer "git" process which
dispatches to "git-foo").

> This mechanism could even be used by the test infrastructure in the
> future, but it is already useful by its own on the command line:
> 
>   $ GIT_TEST_VALGRIND=1 \
> GIT_VALGRIND_OPTIONS="--leak-check=full" \
> ./bin-wrappers/git

If you look in t/test-lib.sh, you can see the contortions the test
infrastructure goes through to support --valgrind. Basically it creates
a parallel bin-wrappers directory where everything gets run under
valgrind. ;)

So I dunno. I'm not opposed to this patch in principle if people find it
useful. These days _most_ things are builtins, so it would at least
cover most of the code you'd want to hit for a debugging session, as
long as you're not concerned with full coverage. But I don't think it's
the right approach for instrumenting the test suite.

-Peff

[PATCH v1] add status config and command line options for rename detection

[Ben Peart]

Add a new config status.renames setting to enable turning off rename detection
during status.  This setting will default to the value of diff.renames.

Add a new config status.renamelimit setting to to enable bounding the time spent
finding out inexact renames during status.  This setting will default to the
value of diff.renamelimit.

Add status --no-renames command line option that enables overriding the config
setting from the command line. Add --find-renames[=] to enable detecting
renames and optionaly setting the similarity index from the command line.

Origional-Patch-by: Alejandro Pauly 
Signed-off-by: Ben Peart 
---

Notes:
Base Ref:
Web-Diff: https://github.com/benpeart/git/commit/aa977d2964
Checkout: git fetch https://github.com/benpeart/git status-renames-v1 && 
git checkout aa977d2964

 Documentation/config.txt |  9 
 builtin/commit.c | 57 +
 diff.c   |  2 +-
 diff.h   |  1 +
 t/t7525-status-rename.sh | 90 
 wt-status.c  | 12 ++
 wt-status.h  |  4 +-
 7 files changed, 173 insertions(+), 2 deletions(-)
 create mode 100644 t/t7525-status-rename.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2659153cb3..b79b83c587 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3119,6 +3119,15 @@ status.displayCommentPrefix::
behavior of linkgit:git-status[1] in Git 1.8.4 and previous.
Defaults to false.
 
+status.renameLimit::
+   The number of files to consider when performing rename detection;
+   if not specified, defaults to the value of diff.renameLimit.
+
+status.renames::
+   Whether and how Git detects renames.  If set to "false",
+   rename detection is disabled. If set to "true", basic rename
+   detection is enabled.  Defaults to the value of diff.renames.
+
 status.showStash::
If set to true, linkgit:git-status[1] will display the number of
entries currently stashed away.
diff --git a/builtin/commit.c b/builtin/commit.c
index 5240f11225..a545096ddd 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -109,6 +109,10 @@ static int have_option_m;
 static struct strbuf message = STRBUF_INIT;
 
 static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
+static int diff_detect_rename = -1;
+static int status_detect_rename = -1;
+static int diff_rename_limit = -1;
+static int status_rename_limit = -1;
 
 static int opt_parse_porcelain(const struct option *opt, const char *arg, int 
unset)
 {
@@ -143,6 +147,16 @@ static int opt_parse_m(const struct option *opt, const 
char *arg, int unset)
return 0;
 }
 
+static int opt_parse_rename_score(const struct option *opt, const char *arg, 
int unset)
+{
+   const char **value = opt->value;
+   if (arg != NULL && *arg == '=')
+   arg = arg + 1;
+
+   *value = arg;
+   return 0;
+}
+
 static void determine_whence(struct wt_status *s)
 {
if (file_exists(git_path_merge_head()))
@@ -1259,11 +1273,29 @@ static int git_status_config(const char *k, const char 
*v, void *cb)
return error(_("Invalid untracked files mode '%s'"), v);
return 0;
}
+   if (!strcmp(k, "diff.renamelimit")) {
+   diff_rename_limit = git_config_int(k, v);
+   return 0;
+   }
+   if (!strcmp(k, "status.renamelimit")) {
+   status_rename_limit = git_config_int(k, v);
+   return 0;
+   }
+   if (!strcmp(k, "diff.renames")) {
+   diff_detect_rename = git_config_rename(k, v);
+   return 0;
+   }
+   if (!strcmp(k, "status.renames")) {
+   status_detect_rename = git_config_rename(k, v);
+   return 0;
+   }
return git_diff_ui_config(k, v, NULL);
 }
 
 int cmd_status(int argc, const char **argv, const char *prefix)
 {
+   static int no_renames = -1;
+   static const char *rename_score_arg = (const char *)-1;
static struct wt_status s;
int fd;
struct object_id oid;
@@ -1297,6 +1329,10 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
  N_("ignore changes to submodules, optional when: all, dirty, 
untracked. (Default: all)"),
  PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
OPT_COLUMN(0, "column", , N_("list untracked files in 
columns")),
+   OPT_BOOL(0, "no-renames", _renames, N_("do not detect 
renames")),
+   { OPTION_CALLBACK, 'M', "find-renames", _score_arg,
+ N_("n"), N_("detect renames, optionally set similarity 
index"),
+ PARSE_OPT_OPTARG, opt_parse_rename_score },
OPT_END(),
};
 
@@ -1336,6 +1372,27 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)