Re: [PATCH v2 08/14] revision.c: use commit-slab for show_source

[Junio C Hamano]

Junio C Hamano  writes:

> Nguyễn Thái Ngọc Duy   writes:
>
>> -refname = commit->util;
>> +refname = *revision_sources_peek(_sources, commit);
>
> At first glance, I thought that the reason why this uses _peek() is
> because the "sources" is expected to only sparsely be populated,
> perhaps because get_tags_and_duplicates() annotates only the tips
> mentioned on the command line via rev_cmdline mechanism and this
> code does not want to auto-vivify the slot, only to read NULL from
> it.
>
> But the code that follows this point liberally uses refname without
> checking if it is NULL, so I am not quite sure what is going on.

Ah, of course, this is about the code that propagates the "source"
(i.e. from which tip given on the command line did we start the
traversal to reach this commit?), so that is what ensures there is
something in commit->util and revision_sources not just has an entry
for the commit but the entry should have a non-NULL string.

So shouldn't *slab_peek() here be *slab_at() instead?

> In
> any case, wouldn't *slab_peek() an anti-pattern?  You use _peek()
> because you expect that a slot is not yet allocated for a commit,
> you desire not to vivify all the slots for all the commits, and
> instead you are prepared to see a NULL returned from the call.  But
> I do not think that is what is happening here, so shouldn't it be
> using _at() instead of _peek()?
>
>>  if (anonymize) {
>>  refname = anonymize_refname(refname);
>
>>  anonymize_ident_line(, _end);
>> @@ -862,10 +864,11 @@ static void get_tags_and_duplicates(struct 
>> rev_cmdline_info *info)
>>   * This ref will not be updated through a commit, lets make
>>   * sure it gets properly updated eventually.
>>   */
>> -if (commit->util || commit->object.flags & SHOWN)
>> +if (*revision_sources_at(_sources, commit) ||
>> +commit->object.flags & SHOWN)
>
> Here it uses *slab_at() which makes more sense.

Re: [PATCH v2 08/14] revision.c: use commit-slab for show_source

[Junio C Hamano]

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

> - refname = commit->util;
> + refname = *revision_sources_peek(_sources, commit);

At first glance, I thought that the reason why this uses _peek() is
because the "sources" is expected to only sparsely be populated,
perhaps because get_tags_and_duplicates() annotates only the tips
mentioned on the command line via rev_cmdline mechanism and this
code does not want to auto-vivify the slot, only to read NULL from
it.

But the code that follows this point liberally uses refname without
checking if it is NULL, so I am not quite sure what is going on. In
any case, wouldn't *slab_peek() an anti-pattern?  You use _peek()
because you expect that a slot is not yet allocated for a commit,
you desire not to vivify all the slots for all the commits, and
instead you are prepared to see a NULL returned from the call.  But
I do not think that is what is happening here, so shouldn't it be
using _at() instead of _peek()?

>   if (anonymize) {
>   refname = anonymize_refname(refname);

>   anonymize_ident_line(, _end);
> @@ -862,10 +864,11 @@ static void get_tags_and_duplicates(struct 
> rev_cmdline_info *info)
>* This ref will not be updated through a commit, lets make
>* sure it gets properly updated eventually.
>*/
> - if (commit->util || commit->object.flags & SHOWN)
> + if (*revision_sources_at(_sources, commit) ||
> + commit->object.flags & SHOWN)

Here it uses *slab_at() which makes more sense.

Re: [PATCH v2 06/14] sequencer.c: use commit-slab to mark seen commits

[Junio C Hamano]

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

> It's done so that commit->util can be removed. See more explanation in
> the commit that removes commit->util.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  sequencer.c | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 4ce5120e77..6b785c6c5f 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -23,6 +23,7 @@
>  #include "hashmap.h"
>  #include "notes-utils.h"
>  #include "sigchain.h"
> +#include "commit-slab.h"
>  
>  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>  
> @@ -3160,6 +3161,7 @@ static enum check_level 
> get_missing_commit_check_level(void)
>   return CHECK_IGNORE;
>  }
>  
> +define_commit_slab(commit_seen, uint8_t);

Because it makes no difference on any real platform that matters, it
is purely academic preference, but the user of this code does not
care about ensuring that commit-seen data is at lesat 8-bit wide,
which is where we should use uint8_t.  We know this is a simple
yes/no field, so use of "uint8_t" here is quite misleading to the
readers.  "unsigned char" or "char" would be a lot better.

Re: [PATCH/RFC] completion: complete all possible -no-

[Eric Sunshine]

On Tue, May 8, 2018 at 11:24 AM, Duy Nguyen  wrote:
> On Mon, Apr 23, 2018 at 7:36 AM, Eric Sunshine  
> wrote:
>> I haven't looked at the implementation, so this may be an entirely
>> stupid suggestion, but would it be possible to instead render the
>> completions as?
>>
>> % git checkout --
>> --[no-]conflict=   --[no-]patch
>> --[no-]detach  --[no-]progress
>>
>> This would address the problem of the --no-* options taking double the
>> screen space.
>
> It took me so long to reply partly because I remember seeing some guy
> doing clever trick with tab completion that also shows a short help
> text in addition to the complete words. I could not find that again
> and from my reading (also internet searching) it's probably not
> possible to do this without trickery.

Okay.

>> It's also more intuitive than that lone and somewhat weird-looking
>> "--no-" suggestion.
>
> It's not that weird if you think about file path completion, where you
> complete one path component at a time not full path, bash just does
> not show you full paths to everything.

The "path completion" analogy and the dotted configuration variable
analogy (below) don't really help me find "--no-" less weird. We're
used to "/" as a separator in paths, and "." a separator in
configuration variables, so they are easier to digest than "-" somehow
being a separator for --no-.

It _might_ feel as bit less weird if it was presented as --no-
or --no-{...} or --no-<...> or --no-... or something, but those seem
pretty weird too, so perhaps not. Anyhow, it's not a major issue; the
--[no-]foo idea seems pretty intuitive, but if it can't be easily
implemented, then falling back to your --no- idea makes sense.

> I'm arguing about this because I want to see your reaction, because
> I'm thinking of doing the very same thing for config completion. Right
> now "git config " gives you two pages of all available config
> variables. I'm thinking that we "git config " just shows the
> groups, e.g.
>
>> ~/w/git $ git config
> add.  interactive.
> advice.   log.
> alias.mailmap.
> am.   man.
>
> Only when you do "git config log." that it shows you log.*

Just wondering out loud (again): add. | add.{...} | add.<...> |
add...; those aren't very attractive either, so plain "add." may
indeed be best.

Re: [PATCH 2/1] config: let `config_store_data_clear()` handle `value_regex`

[Eric Sunshine]

On Sun, May 13, 2018 at 5:58 AM, Martin Ågren  wrote:
> Instead of carefully clearing up `value_regex` in each code path, let
> `config_store_data_clear()` handle that.
>
> Signed-off-by: Martin Ågren 
> ---
> I *think* that it should be ok to `regfree()` after `regcomp()` failed,
> but I'll need to look into that some more (and say something about it in
> the commit message).

My research (for instance [1,2]) seems to indicate that it would be
better to avoid regfree() upon failed regcomp(), even though such a
situation is handled sanely in some implementations.

[1]: https://www.redhat.com/archives/libvir-list/2013-September/msg00276.html
[2]: https://www.redhat.com/archives/libvir-list/2013-September/msg00273.html

Re: [PATCH] config: free resources of `struct config_store_data`

[Eric Sunshine]

On Sun, May 13, 2018 at 5:58 AM, Martin Ågren  wrote:
> On 13 May 2018 at 10:59, Eric Sunshine  wrote:
>> On Sun, May 13, 2018 at 4:23 AM Martin Ågren  wrote:
>>> Introduce and use a small helper function `config_store_data_clear()` to
>>> plug these leaks. This should be safe. The memory tracked here is config
>>> parser events. Once the users (`git_config_set_multivar_in_file_gently()`
>>> and `git_config_copy_or_rename_section_in_file()` at the moment) are
>>> done, no-one should be holding on to a pointer into this memory.
>>
>> A newcomer to this code (such as myself) might legitimately wonder why
>> store->key and store->value_regex are not also being cleaned up by this
>> function. An examination of the relevant code reveals that those structure
>> members are manually (and perhaps tediously) freed already by
>> git_config_set_multivar_in_file_gently(), however, if
>> config_store_data_clear() was responsible for freeing them, then
>> git_config_set_multivar_in_file_gently() could be made a bit less noisy.
>
> How about the following two patches as patches 2/3 and 3/3? I would also
> need to mention in the commit message of this patch (1/3) that the
> function will soon learn to clean up more members.
>
> I could of course squash the three patches into one, but there is some
> minor trickery involved, like we can't reuse a pointer in one place, but
> need to xstrdup it.
>
> Thank you for your comments. I'd be very interested in your thoughts on
> this.

Yep, making this a multi-part patch series and updating the commit
message of the patch which introduces config_store_data_clear(), as
you suggest, makes sense. The patch series could be organized
differently -- such as first moving freeing of 'value_regex' into new
config_store_data_clear(), then freeing additional fields in later
patches -- but I don't think it matters much in practice, so the
current organization is likely good enough.

Re: [PATCH 2/2] apply: add --intent-to-add

[Junio C Hamano]

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

> Similar to 'git reset -N', this option makes 'git apply' automatically
> mark new files as intent-to-add so they are visible in the following
> 'git diff' command and could also be committed with 'git commit -a'.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  Documentation/git-apply.txt |  9 -
>  apply.c | 38 +++--
>  apply.h |  1 +
>  t/t2203-add-intent.sh   | 12 
>  4 files changed, 53 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
> index 4ebc3d3271..2374f64b51 100644
> --- a/Documentation/git-apply.txt
> +++ b/Documentation/git-apply.txt
> @@ -9,7 +9,7 @@ git-apply - Apply a patch to files and/or to the index
>  SYNOPSIS
>  
>  [verse]
> -'git apply' [--stat] [--numstat] [--summary] [--check] [--index] [--3way]
> +'git apply' [--stat] [--numstat] [--summary] [--check] [--index | 
> --intent-to-add] [--3way]
> [--apply] [--no-add] [--build-fake-ancestor=] [-R | --reverse]
> [--allow-binary-replacement | --binary] [--reject] [-z]
> [-p] [-C] [--inaccurate-eof] [--recount] [--cached]
> @@ -74,6 +74,13 @@ OPTIONS
>   cached data, apply the patch, and store the result in the index
>   without using the working tree. This implies `--index`.
>  
> +--intent-to-add::
> + When applying the patch only to the working tree, mark new
> + files to be added to the index later (see `--intent-to-add`
> + option in linkgit:git-add[1]). This option is ignored if
> + `--index` is present or the command is not run in a Git
> + repository.

It may make sense to make it incompatible with "--index" like you
did, but how does this interact with "--cached" or "--3way"?  It is
unclear from the above documentation.

> diff --git a/apply.c b/apply.c
> index 7e5792c996..31d3e50401 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -136,6 +136,8 @@ int check_apply_state(struct apply_state *state, int 
> force_apply)
>   state->apply = 0;
>   if (state->check_index && is_not_gitdir)
>   return error(_("--index outside a repository"));
> + if (state->set_ita && is_not_gitdir)
> + state->set_ita = 0;

I think this should error out, just like one line above does.
"I-t-a" is impossible without having the index, just like "--index"
is impossible without having the index.

> @@ -4265,9 +4267,6 @@ static int add_index_file(struct apply_state *state,
>   int namelen = strlen(path);
>   unsigned ce_size = cache_entry_size(namelen);
>  
> - if (!state->update_index)
> - return 0;
> -

OK, with this change, only "index-affecting" mode will call into the
function, in the first place.  The current code was wasteful in that
the caller always called and forced this function to do a few useless
computation before it realized that it is not going to touch the index
at all.

> @@ -4305,6 +4304,27 @@ static int add_index_file(struct apply_state *state,
>   return 0;
>  }
>  
> +static int add_ita_file(struct apply_state *state,
> + const char *path, unsigned mode)
> +{
> + struct cache_entry *ce;
> + int namelen = strlen(path);
> + unsigned ce_size = cache_entry_size(namelen);
> +
> + ce = xcalloc(1, ce_size);
> + memcpy(ce->name, path, namelen);
> + ce->ce_mode = create_ce_mode(mode);
> + ce->ce_flags = create_ce_flags(0) | CE_INTENT_TO_ADD;
> + ce->ce_namelen = namelen;
> + set_object_name_for_intent_to_add_entry(ce);
> + if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD) < 0) {
> + free(ce);
> + return error(_("unable to add cache entry for %s"), path);
> + }
> +
> + return 0;
> +}

It is somewhat unsatisfactory that we need a whole new function to
record a new path in the index.  IOW, I have a feeling that a bit of
refactoring of add_index_file() should allow us to share more code
between it and this new function.

> @@ -4465,8 +4485,11 @@ static int create_file(struct apply_state *state, 
> struct patch *patch)
>  
>   if (patch->conflicted_threeway)
>   return add_conflicted_stages_file(state, patch);
> - else
> + else if (state->update_index)
>   return add_index_file(state, path, mode, buf, size);
> + else if (state->set_ita)
> + return add_ita_file(state, path, mode);
> + return 0;

It is very unfortunate that you need to do (update_index||set_ita)
everywhere else only bevause you want to do this else/if cascade.
I'd rather redefine the bits to mean

- update-index: we will do something that touches the index
  (hence we need to have the repository, we need to lock the
  index, etc.).

- ita-only: changes to the existing paths are only reflected to
  the working tree, but new paths are added to the index as
  i-t-a entries.

and 

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

[Junio C Hamano]

Duy Nguyen  writes:

>
> I would like an option to revert back to current behavior. I'm not a
> new user. I know what I'm doing. Please don't make me type more.

I can guarantee that nobody will stay a newbie.  They either become
more proficient and aware of what they are doing without having to
think, at which point they start feeling the same way as you are.
Or they leave the Git ecosystem and move to better things ;-)

> And '--" is not completely useless. If you have  and 
> with the same name, you have to give "--" to to tell git what the
> first argument means.

Correct.

We _could_ do better than the corrent code, though, when we happen
to have a file called 'master'.  "git checkout master master" cannot
mean anything other than "I want to make the index and the working
tree copies of 'master' file to match what is recorded on the master
branch", but I think we do require "git checkout master -- master"
disambiguation.

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

[Junio C Hamano]

René Scharfe  writes:

> Storing integer values in pointers is a trick that seems to have worked
> so far for fast-export.  A portable way to avoid that trick without
> requiring more memory would be to use a union.
>
> Or we could roll our own custom hash map, as I mused in an earlier post.
> That would duplicate quite a bit of code; are there reusable pieces
> hidden within that could be extracted into common functions?

Hmm, this together with your follow-up does not look too bad, but it
does introduce quite a lot of code that could be refactored, so I am
not sure if I really like it or not.

>
> ---
>  builtin/fast-export.c | 105 --
>  1 file changed, 81 insertions(+), 24 deletions(-)
>
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index 530df12f05..627b0032f3 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -14,7 +14,6 @@
>  #include "diffcore.h"
>  #include "log-tree.h"
>  #include "revision.h"
> -#include "decorate.h"
>  #include "string-list.h"
>  #include "utf8.h"
>  #include "parse-options.h"
> @@ -71,9 +70,65 @@ static int parse_opt_tag_of_filtered_mode(const struct 
> option *opt,
>   return 0;
>  }
>  
> -static struct decoration idnums;
> +struct object_mark_entry {
> + const struct object *base;
> + uint32_t mark;
> +};
> +
> +struct object_marks {
> + unsigned int size;
> + unsigned int nr;
> + struct object_mark_entry *entries;
> +};
> +
> +static struct object_marks idnums;
>  static uint32_t last_idnum;
>  
> +static unsigned int hash_obj(const struct object *obj, unsigned int n)
> +{
> + return sha1hash(obj->oid.hash) % n;
> +}
> +
> +static void set_object_mark(struct object_marks *n, const struct object 
> *base,
> + uint32_t mark)
> +{
> + unsigned int size = n->size;
> + struct object_mark_entry *entries = n->entries;
> + unsigned int j = hash_obj(base, size);
> +
> + while (entries[j].base) {
> + if (entries[j].base == base) {
> + entries[j].mark = mark;
> + return;
> + }
> + if (++j >= size)
> + j = 0;
> + }
> + entries[j].base = base;
> + entries[j].mark = mark;
> + n->nr++;
> +}
> +
> +static void grow_object_marks(struct object_marks *n)
> +{
> + unsigned int i;
> + unsigned int old_size = n->size;
> + struct object_mark_entry *old_entries = n->entries;
> +
> + n->size = (old_size + 1000) * 3 / 2;
> + n->entries = xcalloc(n->size, sizeof(n->entries[0]));
> + n->nr = 0;
> +
> + for (i = 0; i < old_size; i++) {
> + const struct object *base = old_entries[i].base;
> + uint32_t mark = old_entries[i].mark;
> +
> + if (mark)
> + set_object_mark(n, base, mark);
> + }
> + free(old_entries);
> +}
> +
>  static int has_unshown_parent(struct commit *commit)
>  {
>   struct commit_list *parent;
> @@ -156,20 +211,13 @@ 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)
> -{
> - return ((uint32_t *)NULL) + mark;
> -}
> -
> -static inline uint32_t ptr_to_mark(void * mark)
> -{
> - return (uint32_t *)mark - (uint32_t *)NULL;
> -}
> -
>  static inline void mark_object(struct object *object, uint32_t mark)
>  {
> - add_decoration(, object, mark_to_ptr(mark));
> + unsigned int nr = idnums.nr + 1;
> +
> + if (nr > idnums.size * 2 / 3)
> + grow_object_marks();
> + return set_object_mark(, object, mark);
>  }
>  
>  static inline void mark_next_object(struct object *object)
> @@ -179,10 +227,21 @@ static inline void mark_next_object(struct object 
> *object)
>  
>  static int get_object_mark(struct object *object)
>  {
> - void *decoration = lookup_decoration(, object);
> - if (!decoration)
> + unsigned int j;
> +
> + /* nothing to lookup */
> + if (!idnums.size)
>   return 0;
> - return ptr_to_mark(decoration);
> + j = hash_obj(object, idnums.size);
> + for (;;) {
> + struct object_mark_entry *ref = idnums.entries + j;
> + if (ref->base == object)
> + return ref->mark;
> + if (!ref->base)
> + return 0;
> + if (++j == idnums.size)
> + j = 0;
> + }
>  }
>  
>  static void show_progress(void)
> @@ -897,8 +956,7 @@ static void handle_tags_and_duplicates(void)
>  static void export_marks(char *file)
>  {
>   unsigned int i;
> - uint32_t mark;
> - struct decoration_entry *deco = idnums.entries;
> + struct object_mark_entry *entry = idnums.entries;
>   FILE *f;
>   int e = 0;
>  
> @@ -907,15 +965,14 @@ static void export_marks(char *file)
>   die_errno("Unable to open marks 

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 v2 04/14] describe: use commit-slab for commit names instead of commit->util

[Junio C Hamano]

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

> + slot = commit_names_peek(_names, c);
> + n = slot ? *slot : NULL;

Seeing this and the helper in the previous step makes me wonder if
we also want a "peek-and-then-peel" variant that encapsulates this
pattern.  We'll know when we read the series through.

So far looking good.  Thanks.

>   if (n) {
>   if (!tags && !all && n->prio < 2) {
>   unannotated_cnt++;

Re: [PATCH v2 03/14] blame: use commit-slab for blame suspects instead of commit->util

[Junio C Hamano]

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

> It's done so that commit->util can be removed. See more explanation in
> the commit that removes commit->util.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  blame.c | 42 +++---
>  blame.h |  2 ++
>  builtin/blame.c |  2 +-
>  3 files changed, 34 insertions(+), 12 deletions(-)
>
> diff --git a/blame.c b/blame.c
> index 78c9808bd1..18e8bd996a 100644
> --- a/blame.c
> +++ b/blame.c
> @@ -6,6 +6,24 @@
>  #include "diffcore.h"
>  #include "tag.h"
>  #include "blame.h"
> +#include "commit-slab.h"
> +
> +define_commit_slab(blame_suspects, struct blame_origin *);
> +static struct blame_suspects blame_suspects;
> +
> +struct blame_origin *get_blame_suspects(struct commit *commit)
> +{
> + struct blame_origin **result;
> +
> + result = blame_suspects_peek(_suspects, commit);
> +
> + return result ? *result : NULL;
> +}
> +
> +static void set_blame_suspects(struct commit *commit, struct blame_origin 
> *origin)
> +{
> + *blame_suspects_at(_suspects, commit) = origin;
> +}
>  
>  void blame_origin_decref(struct blame_origin *o)
>  {

This makes really a pleasant read.  With these helpers in place, the
remainder of this patch becomes mechanical substitution to call
get_blame_suspects when commit->util appears on the RHS of an
expression and set_blame_suspects when commit->util gets assigned.

> @@ -15,12 +33,12 @@ void blame_origin_decref(struct blame_origin *o)
>   blame_origin_decref(o->previous);
>   free(o->file.ptr);
>   /* Should be present exactly once in commit chain */
> - for (p = o->commit->util; p; l = p, p = p->next) {
> + for (p = get_blame_suspects(o->commit); p; l = p, p = p->next) {
>   if (p == o) {
>   if (l)
>   l->next = p->next;
>   else
> - o->commit->util = p->next;
> + set_blame_suspects(o->commit, p->next);
>   free(o);
>   return;
>   }

Re: [PATCH v2 02/14] commit-slab: support shared commit-slab

[Junio C Hamano]

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

> define_shared_commit_slab() could be used in a header file to define a
> commit-slab. One of these C files must include commit-slab-impl.h and
> "call" implement_shared_commit_slab().
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  commit-slab-hdr.h  | 13 +
>  commit-slab-impl.h | 20 +---
>  commit-slab.h  |  2 +-
>  3 files changed, 27 insertions(+), 8 deletions(-)

This, while it is a reasonable change, makes the big comment at the
end of commit-slab-impl.h out of sync with the reality, doesn't it?
implement_commit_slab() now takes three args, but the comment still
says the caller does not have to give the third one.

Re: [PATCH v2 01/14] commit-slab.h: code split

[Junio C Hamano]

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

> The struct declaration and implementation macros are moved to
> commit-slab-hdr.h and commit-slab-impl.h respectively. This right now
> is not needed for current users but if we share a commit-slab for
> multiple files, we need something better than the current structure.

"something better" needs to be replaced with something better for it
to be worth being there.  In other words, say why you want to avoid
including the part you moved to *-impl.h everywhere.

I also think you want to rename s/-hdr.h/-decl.h/ or something to
avoid sounding silly by repeating "header" twice in the name.

> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  commit-slab-hdr.h  |  30 
>  commit-slab-impl.h |  91 +++
>  commit-slab.h  | 115 +++--
>  3 files changed, 127 insertions(+), 109 deletions(-)
>  create mode 100644 commit-slab-hdr.h
>  create mode 100644 commit-slab-impl.h

"git blame" tells me that the new two files consist mostly of the
material from commit-slab.h moved verbatim, which is assuring ;-).

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

[Philip Oakley]

From: "Dannier Castro L" 

On 13/05/2018 00:03, Duy Nguyen wrote:


On Sun, May 13, 2018 at 4:23 AM, Dannier Castro L 
wrote:

For GIT new users, this complicated versatility of  could
be very confused, also considering that actually the flag '--' is
completely useless (added or not, there is not any difference for
this command), when the same program messages promote the use of
this flag.

I would like an option to revert back to current behavior. I'm not a
new user. I know what I'm doing. Please don't make me type more.

And '--" is not completely useless. If you have  and 
with the same name, you have to give "--" to to tell git what the
first argument means.


Sure Duy, you're right, probably "completely useless" is not the correct
definition, even according with the code I didn't find another useful
case that is not file and branch with the same name. The program is able
to know the type using only the name, turning "--" into an extra flag in
most of cases.

I think this solution could please you more: By default the configuration
is the current, but the user has the chance to set this, for example:

git config --global flag.strictdashdash true

Thank you so much for the spent time reviewing the patch, this is my
first one in this repository.


It maybe that after review you could suggest an appropriate rewording or
re-arrangement of the man page to better highlight the proper use of the
'--' disambiguation.

Perhaps frame the man page as if it is normal for the '--' to be included
within command lines (which should be the case for scripts anyway!).

Then indicate that it isn't mandatory if the file/branch/dwim distinction is
obvious. i.e. make sure that the man page is educational as well as being a
reference that may be misunderstood.

Those well versed in the Git cli will normally omit the '--', only using it
where necessary, however for a new users/readers of the man page, it may be
better to be more explicit and avoid future misunderstandings.

--
Philip

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

Draft of Git Rev News edition 39

[Christian Couder]

Hi,

A draft of a new Git Rev News edition is available here:

  https://github.com/git/git.github.io/blob/master/rev_news/drafts/edition-39.md

Everyone is welcome to contribute in any section either by editing the
above page on GitHub and sending a pull request, or by commenting on
this GitHub issue:

  https://github.com/git/git.github.io/issues/290

You can also reply to this email.

In general all kinds of contribution, for example proofreading,
suggestions for articles or links, help on the issues in GitHub, and
so on, are very much appreciated.

I tried to cc everyone who appears in this edition, but maybe I missed
some people, sorry about that.

Jakub, Markus, Gabriel and me plan to publish this edition on
Wednesday May 16th.

Thanks,
Christian.

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

[Kevin Daudt]

On Sat, May 12, 2018 at 08:23:32PM -0600, Dannier Castro L wrote:
> Currently,  is a complex command able to handle both
> branches and files without any distintion other than their names,
> taking into account that depending on the type (branch or file),
> the functionality is completely different, the easier example:
> 
> $ git checkout   # Switch from current branch to .
> $ git checkout # Restore  from HEAD, discarding
>  # changes if it's necessary.
> $ git checkout --  # The same as the last one, only with an
>  # useless '--'.
> 
> For GIT new users, this complicated versatility of  could
> be very confused, also considering that actually the flag '--' is
> completely useless (added or not, there is not any difference for
> this command), when the same program messages promote the use of
> this flag.
> 
> Regarding the 's power to overwrite any file, discarding
> changes if they exist without some way of recovering them, the
> solution propuses that the usage of '--' is strict before to
> specify the file(s) path(s) for any  command (including
> all types of flags), as a "defense barrier" to make sure about
> user's knowledge and intension running .
> 
> The solution consists in detect '--' into command args, allowing
> the discard of changes and considering the following names as
> file paths, otherwise, they are branch names.
> 
> Signed-off-by: Dannier Castro L 

One data point indicating this is giving issues is that today on IRC a
user was confused why `git checkout pt` did not show any message and did
not checkout a remote branch called 'pt' as they expected. It turned out
they also had a local file/dir called 'pt', which caused git to checkout
that file/dir rather than creating a local branch based on the remote
branch.

Kevin

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

[SZEDER Gábor]

> On 13/05/2018 00:03, Duy Nguyen wrote:
> 
> > On Sun, May 13, 2018 at 4:23 AM, Dannier Castro L  
> > wrote:
> >> For GIT new users, this complicated versatility of  could
> >> be very confused, also considering that actually the flag '--' is
> >> completely useless (added or not, there is not any difference for
> >> this command), when the same program messages promote the use of
> >> this flag.
> > I would like an option to revert back to current behavior. I'm not a
> > new user. I know what I'm doing. Please don't make me type more.
> >
> > And '--" is not completely useless. If you have  and 
> > with the same name, you have to give "--" to to tell git what the
> > first argument means.
> 
> Sure Duy, you're right, probably "completely useless" is not the correct
> definition,

No, "completely useless" is just plain wrong.

> even according with the code I didn't find another useful
> case that is not file and branch with the same name.

The optional disambiguating doubledash is the standard way to erm, well,
to disambiguate whatever there is to disambiguate.  Not only refs and
filenames, but also e.g. options and filenames:

  $ git checkout --option-looking-file
  error: unknown option `option-looking-file'
  usage: git checkout [] 
  <...>
  $ git checkout -- --option-looking-file
  error: pathspec '--option-looking-file' did not match any file(s) known to 
git.

Note that this is not at all specific to Git, many other programs
support the optional disambiguating doubledash as well:

  $ touch --option-looking-file
  touch: unrecognized option '--option-looking-file'
  Try 'touch --help' for more information.
  $ touch -- --option-looking-file
  $ ls --option-looking-file
  ls: unrecognized option '--option-looking-file'
  Try 'ls --help' for more information.
  $ ls -- --option-looking-file
  --option-looking-file

Please do not make it mandatory.

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

[Dannier Castro L]

On 13/05/2018 00:03, Duy Nguyen wrote:


On Sun, May 13, 2018 at 4:23 AM, Dannier Castro L  wrote:

For GIT new users, this complicated versatility of  could
be very confused, also considering that actually the flag '--' is
completely useless (added or not, there is not any difference for
this command), when the same program messages promote the use of
this flag.

I would like an option to revert back to current behavior. I'm not a
new user. I know what I'm doing. Please don't make me type more.

And '--" is not completely useless. If you have  and 
with the same name, you have to give "--" to to tell git what the
first argument means.


Sure Duy, you're right, probably "completely useless" is not the correct
definition, even according with the code I didn't find another useful
case that is not file and branch with the same name. The program is able
to know the type using only the name, turning "--" into an extra flag in
most of cases.

I think this solution could please you more: By default the configuration
is the current, but the user has the chance to set this, for example:

git config --global flag.strictdashdash true

Thank you so much for the spent time reviewing the patch, this is my
first one in this repository.

-Dannier CL

Re: [PATCH] config: free resources of `struct config_store_data`

[Martin Ågren]

On 13 May 2018 at 10:23, Martin Ågren  wrote:

> +void config_store_data_clear(struct config_store_data *store)

I will do s/void/static void/ here...

Re: [PATCH 01/18] Add a function to solve least-cost assignment problems

[Duy Nguyen]

On Thu, May 3, 2018 at 5:30 PM, Johannes Schindelin
 wrote:
> +   /* reduction transfer */
> +   free_row = xmalloc(sizeof(int) * row_count);
> +   for (int i = 0; i < row_count; i++) {

travis complains about this


hungarian.c: In function ‘compute_assignment’:
hungarian.c:47:11: error: redeclaration of ‘i’ with no linkage
  for (int i = 0; i < row_count; i++) {
   ^
hungarian.c:21:6: note: previous declaration of ‘i’ was here
  int i, j, phase;
  ^
hungarian.c:47:2: error: ‘for’ loop initial declarations are only
allowed in C99 mode
  for (int i = 0; i < row_count; i++) {
  ^
hungarian.c:47:2: note: use option -std=c99 or -std=gnu99 to compile your code
-- 
Duy

[PATCH 1/2] diff: turn --ita-invisible-in-index on by default

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

Due to the implementation detail of intent-to-add entries. The current
"git diff" (i.e. no treeish or --cached argument) would show the
changes in the i-t-a file, but it does not mark the file as new, while
"diff --cached" would mark the file as new while showing its content
as empty.

One evidence of the current output being wrong is that, the output
from "git diff" (with ita entries) cannot be applied because it
assumes empty files exist before applying.

Turning on --ita-invisible-in-index [1] [2] would fix this.

This option is on by default in git-status [1] but we need more fixup
in rename detection code [3]. Luckily we don't need to do anything
else for the rename detection code in diff.c (wt-status.c uses a
customized one).

[1] 425a28e0a4 (diff-lib: allow ita entries treated as "not yet exist
in index" - 2016-10-24)
[2] b42b451919 (diff: add --ita-[in]visible-in-index - 2016-10-24)
[3] bc3dca07f4 (Merge branch 'nd/ita-wt-renames-in-status' - 2018-01-23)

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/diff.c  |  7 +++
 t/t2203-add-intent.sh   | 37 ++---
 t/t4011-diff-symlink.sh | 10 ++
 3 files changed, 43 insertions(+), 11 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index 16bfb22f73..00424c296d 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -352,6 +352,13 @@ int cmd_diff(int argc, const char **argv, const char 
*prefix)
rev.diffopt.flags.allow_external = 1;
rev.diffopt.flags.allow_textconv = 1;
 
+   /*
+* Default to intent-to-add entries invisible in the
+* index. This makes them show up as new files in diff-files
+* and not at all in diff-cached.
+*/
+   rev.diffopt.ita_invisible_in_index = 1;
+
if (nongit)
die(_("Not a git repository"));
argc = setup_revisions(argc, argv, , NULL);
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 78236dc7d8..31bf50082f 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -69,9 +69,9 @@ test_expect_success 'i-t-a entry is simply ignored' '
git add -N nitfol &&
git commit -m second &&
test $(git ls-tree HEAD -- nitfol | wc -l) = 0 &&
-   test $(git diff --name-only HEAD -- nitfol | wc -l) = 1 &&
-   test $(git diff --name-only --ita-invisible-in-index HEAD -- nitfol | 
wc -l) = 0 &&
-   test $(git diff --name-only --ita-invisible-in-index -- nitfol | wc -l) 
= 1
+   test $(git diff --name-only --ita-visible-in-index HEAD -- nitfol | wc 
-l) = 1 &&
+   test $(git diff --name-only HEAD -- nitfol | wc -l) = 0 &&
+   test $(git diff --name-only -- nitfol | wc -l) = 1
 '
 
 test_expect_success 'can commit with an unrelated i-t-a entry in index' '
@@ -99,13 +99,13 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '
 
: >dir/bar &&
git add -N dir/bar &&
-   git diff --cached --name-only >actual &&
+   git diff --name-only >actual &&
echo dir/bar >expect &&
test_cmp expect actual &&
 
git write-tree >/dev/null &&
 
-   git diff --cached --name-only >actual &&
+   git diff --name-only >actual &&
echo dir/bar >expect &&
test_cmp expect actual
 '
@@ -186,7 +186,19 @@ test_expect_success 'rename detection finds the right 
names' '
cat >expected.3 <<-EOF &&
2 .R N... 100644 100644 100644 $hash $hash R100 third   first
EOF
-   test_cmp expected.3 actual.3
+   test_cmp expected.3 actual.3 &&
+
+   git diff --stat >actual.4 &&
+   cat >expected.4 <<-EOF &&
+first => third | 0
+1 file changed, 0 insertions(+), 0 deletions(-)
+   EOF
+   test_cmp expected.4 actual.4 &&
+
+   git diff --cached --stat >actual.5 &&
+   : >expected.5 &&
+   test_cmp expected.5 actual.5
+
)
 '
 
@@ -222,5 +234,16 @@ test_expect_success 'double rename detection in status' '
)
 '
 
-test_done
+test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '
+   git reset --hard &&
+   echo new >new-ita &&
+   git add -N new-ita &&
+   git diff --summary >actual &&
+   echo " create mode 100644 new-ita" >expected &&
+   test_cmp expected actual &&
+   git diff --cached --summary >actual2 &&
+   : >expected2 &&
+   test_cmp expected2 actual2
+'
 
+test_done
diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh
index cf0f3a1ee7..108c012a3a 100755
--- a/t/t4011-diff-symlink.sh
+++ b/t/t4011-diff-symlink.sh
@@ -139,11 +139,13 @@ test_expect_success SYMLINKS 'setup symlinks with 
attributes' '
 test_expect_success SYMLINKS 'symlinks do not respect userdiff config by path' 
'
cat >expect <<-\EOF &&
diff --git a/file.bin b/file.bin
-   index e69de29..d95f3ad 100644
-   Binary files a/file.bin 

[PATCH 2/2] apply: add --intent-to-add

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

Similar to 'git reset -N', this option makes 'git apply' automatically
mark new files as intent-to-add so they are visible in the following
'git diff' command and could also be committed with 'git commit -a'.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git-apply.txt |  9 -
 apply.c | 38 +++--
 apply.h |  1 +
 t/t2203-add-intent.sh   | 12 
 4 files changed, 53 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index 4ebc3d3271..2374f64b51 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -9,7 +9,7 @@ git-apply - Apply a patch to files and/or to the index
 SYNOPSIS
 
 [verse]
-'git apply' [--stat] [--numstat] [--summary] [--check] [--index] [--3way]
+'git apply' [--stat] [--numstat] [--summary] [--check] [--index | 
--intent-to-add] [--3way]
  [--apply] [--no-add] [--build-fake-ancestor=] [-R | --reverse]
  [--allow-binary-replacement | --binary] [--reject] [-z]
  [-p] [-C] [--inaccurate-eof] [--recount] [--cached]
@@ -74,6 +74,13 @@ OPTIONS
cached data, apply the patch, and store the result in the index
without using the working tree. This implies `--index`.
 
+--intent-to-add::
+   When applying the patch only to the working tree, mark new
+   files to be added to the index later (see `--intent-to-add`
+   option in linkgit:git-add[1]). This option is ignored if
+   `--index` is present or the command is not run in a Git
+   repository.
+
 -3::
 --3way::
When the patch does not apply cleanly, fall back on 3-way merge if
diff --git a/apply.c b/apply.c
index 7e5792c996..31d3e50401 100644
--- a/apply.c
+++ b/apply.c
@@ -136,6 +136,8 @@ int check_apply_state(struct apply_state *state, int 
force_apply)
state->apply = 0;
if (state->check_index && is_not_gitdir)
return error(_("--index outside a repository"));
+   if (state->set_ita && is_not_gitdir)
+   state->set_ita = 0;
if (state->cached) {
if (is_not_gitdir)
return error(_("--cached outside a repository"));
@@ -4265,9 +4267,6 @@ static int add_index_file(struct apply_state *state,
int namelen = strlen(path);
unsigned ce_size = cache_entry_size(namelen);
 
-   if (!state->update_index)
-   return 0;
-
ce = xcalloc(1, ce_size);
memcpy(ce->name, path, namelen);
ce->ce_mode = create_ce_mode(mode);
@@ -4305,6 +4304,27 @@ static int add_index_file(struct apply_state *state,
return 0;
 }
 
+static int add_ita_file(struct apply_state *state,
+   const char *path, unsigned mode)
+{
+   struct cache_entry *ce;
+   int namelen = strlen(path);
+   unsigned ce_size = cache_entry_size(namelen);
+
+   ce = xcalloc(1, ce_size);
+   memcpy(ce->name, path, namelen);
+   ce->ce_mode = create_ce_mode(mode);
+   ce->ce_flags = create_ce_flags(0) | CE_INTENT_TO_ADD;
+   ce->ce_namelen = namelen;
+   set_object_name_for_intent_to_add_entry(ce);
+   if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD) < 0) {
+   free(ce);
+   return error(_("unable to add cache entry for %s"), path);
+   }
+
+   return 0;
+}
+
 /*
  * Returns:
  *  -1 if an unrecoverable error happened
@@ -4465,8 +4485,11 @@ static int create_file(struct apply_state *state, struct 
patch *patch)
 
if (patch->conflicted_threeway)
return add_conflicted_stages_file(state, patch);
-   else
+   else if (state->update_index)
return add_index_file(state, path, mode, buf, size);
+   else if (state->set_ita)
+   return add_ita_file(state, path, mode);
+   return 0;
 }
 
 /* phase zero is to remove, phase one is to create */
@@ -4687,7 +4710,8 @@ static int apply_patch(struct apply_state *state,
state->apply = 0;
 
state->update_index = state->check_index && state->apply;
-   if (state->update_index && !is_lock_file_locked(>lock_file)) {
+   if ((state->update_index || state->set_ita) &&
+   !is_lock_file_locked(>lock_file)) {
if (state->index_file)
hold_lock_file_for_update(>lock_file,
  state->index_file,
@@ -4888,7 +4912,7 @@ int apply_all_patches(struct apply_state *state,
state->whitespace_error);
}
 
-   if (state->update_index) {
+   if (state->update_index || state->set_ita) {
res = write_locked_index(_index, >lock_file, 
COMMIT_LOCK);
if (res) {
error(_("Unable to write new index file"));
@@ -4941,6 +4965,8 @@ int apply_parse_options(int argc, const char **argv,

Re: [PATCH v2 17/28] t4014: abstract away SHA-1-specific constants

[brian m. carlson]

On Sun, May 13, 2018 at 09:34:03AM +0200, Johannes Sixt wrote:
> Am 13.05.2018 um 04:24 schrieb brian m. carlson:
> > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> > index dac3f349a3..42b3e11207 100755
> > --- a/t/t4014-format-patch.sh
> > +++ b/t/t4014-format-patch.sh
> > @@ -578,7 +578,9 @@ test_expect_success 'excessive subject' '
> > rm -rf patches/ &&
> > git checkout side &&
> > +   before=$(git rev-parse --short $(git hash-object file)) &&
> > for i in 5 6 1 2 3 A 4 B C 7 8 9 10 D E F; do echo "$i"; done >>file &&
> > +   after=$(git rev-parse --short $(git hash-object file)) &&
> 
> It would be better to avoid process expansion in command arguments, because
> the shell does not diagnose failures. This is preferable:
> 
>   before=$(git hash-object file) &&
>   before=$(git rev-parse --short $before) &&

I considered that and assumed it would be all right because if git
hash-object failed, we wouldn't get anything on stdout.  However, I
agree that your approach is more robust, so I'll reroll with that
change.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

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

[Matthieu Moy]

"Dannier Castro L"  wrote:

> Currently,  is a complex command able to handle both
> branches and files without any distintion other than their names,
> taking into account that depending on the type (branch or file),
> the functionality is completely different, the easier example:
> 
> $ git checkout   # Switch from current branch to .
> $ git checkout # Restore  from HEAD, discarding
> # changes if it's necessary.
> $ git checkout --  # The same as the last one, only with an
> # useless '--'.

It's not really "useless".

In the first two commands you give, git guesses whether the first
argument is a branch or a file. In the 3rd, the -- indicates that it
must be a file.

> For GIT new users,

Nit: we write Git (for the system) or git (for the command-line tool),
but usually avoid the all-caps GIT.

> The solution consists in detect '--' into command args, allowing
> the discard of changes and considering the following names as
> file paths, otherwise, they are branch names.

This answers the "what?" question, but does not explain why this is a
good change. A good commit message should focus more on the "why?"
question.

While I agree that "git checkout" is a complex command, and would love
to see a simpler syntax at least for the most common operations, I do
not think that this is a good change for several reasons:

* If one considers that this "--" syntax is an issue, then this patch
  is a very partial solution only. Many other commands use the same
  convention (for example "git log " Vs "git log -- "),
  so changing only one makes git less consistent. Also, note that this
  "--" convention is not git-specific. Try "touch --help" and "touch
  -- --help" for example.

* This breaks backward compatibility. People used to "git checkout
  " won't be able to use it anymore. Scripts using it will
  break. Git avoids breaking backward compatibility, and when there's
  a good reason to do so we need a good transition plan. In this case,
  one possible plan would be to 1) issue a warning whenever "git
  checkout " is used for a while, and then 2) actually forbid
  it. But following this path, I don't think step 2) would actually be
  useful.

> @@ -928,6 +931,7 @@ static int parse_branchname_arg(int argc, const char 
> **argv,
>   dash_dash_pos = -1;
>   for (i = 0; i < argc; i++) {
>   if (!strcmp(argv[i], "--")) {
> + opts->discard_changes = 1;
>   dash_dash_pos = i;

Wouldn't "dash_dash_pos != -1" be enough to know whether there's a --?

-- 
Matthieu Moy
https://matthieu-moy.fr/

Re: [PATCH] travis-ci: run gcc-7 on linux-gcc jobs

[Thomas Braun]

Am 13.05.2018 um 11:17 schrieb Nguyễn Thái Ngọc Duy:
> Switch from gcc-4.8 to gcc-7. Newer compilers come with more warning
> checks (usually in -Wextra).  Since -Wextra is enabled in developer
> mode (which is also enabled in travis), this lets travis report more
> warnings before other people do it.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  .travis.yml| 3 +++
>  ci/lib-travisci.sh | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/.travis.yml b/.travis.yml
> index 5f5ee4f3bd..a77f5f5bd5 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -16,10 +16,13 @@ compiler:
>  
>  addons:
>apt:
> +sources:
> +- ubuntu-toolchain-r-test
>  packages:
>  - language-pack-is
>  - git-svn
>  - apache2
> +- gcc-7

You could also use gcc-8 here as that is already present according to [1].

[1]:
https://launchpad.net/~ubuntu-toolchain-r/+archive/ubuntu/test/+packages

>  matrix:
>include:
> diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
> index 109ef280da..ef2848fd45 100755
> --- a/ci/lib-travisci.sh
> +++ b/ci/lib-travisci.sh
> @@ -99,6 +99,9 @@ export DEFAULT_TEST_TARGET=prove
>  export GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
>  export GIT_TEST_OPTS="--verbose-log -x"
>  export GIT_TEST_CLONE_2GB=YesPlease
> +if [ "$jobname" = linux-gcc ]; then
> + export CC=gcc-7
> +fi
>  
>  case "$jobname" in
>  linux-clang|linux-gcc)
> 

[PATCH v1 1/8] fetch-object: make functions return an error code

[Christian Couder]

The callers of the fetch_object() and fetch_objects() might
be interested in knowing if these functions succeeded or not.

Signed-off-by: Christian Couder 
---
 fetch-object.c | 15 +--
 fetch-object.h |  6 +++---
 sha1-file.c|  4 ++--
 3 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/fetch-object.c b/fetch-object.c
index 853624f811..ccc4ea7f1a 100644
--- a/fetch-object.c
+++ b/fetch-object.c
@@ -5,11 +5,12 @@
 #include "transport.h"
 #include "fetch-object.h"
 
-static void fetch_refs(const char *remote_name, struct ref *ref)
+static int fetch_refs(const char *remote_name, struct ref *ref)
 {
struct remote *remote;
struct transport *transport;
int original_fetch_if_missing = fetch_if_missing;
+   int res;
 
fetch_if_missing = 0;
remote = remote_get(remote_name);
@@ -19,18 +20,20 @@ static void fetch_refs(const char *remote_name, struct ref 
*ref)
 
transport_set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
transport_set_option(transport, TRANS_OPT_NO_DEPENDENTS, "1");
-   transport_fetch_refs(transport, ref);
+   res = transport_fetch_refs(transport, ref);
fetch_if_missing = original_fetch_if_missing;
+
+   return res;
 }
 
-void fetch_object(const char *remote_name, const unsigned char *sha1)
+int fetch_object(const char *remote_name, const unsigned char *sha1)
 {
struct ref *ref = alloc_ref(sha1_to_hex(sha1));
hashcpy(ref->old_oid.hash, sha1);
-   fetch_refs(remote_name, ref);
+   return fetch_refs(remote_name, ref);
 }
 
-void fetch_objects(const char *remote_name, const struct oid_array *to_fetch)
+int fetch_objects(const char *remote_name, const struct oid_array *to_fetch)
 {
struct ref *ref = NULL;
int i;
@@ -41,5 +44,5 @@ void fetch_objects(const char *remote_name, const struct 
oid_array *to_fetch)
new_ref->next = ref;
ref = new_ref;
}
-   fetch_refs(remote_name, ref);
+   return fetch_refs(remote_name, ref);
 }
diff --git a/fetch-object.h b/fetch-object.h
index 4b269d07ed..12e1f9ee70 100644
--- a/fetch-object.h
+++ b/fetch-object.h
@@ -3,9 +3,9 @@
 
 #include "sha1-array.h"
 
-extern void fetch_object(const char *remote_name, const unsigned char *sha1);
+extern int fetch_object(const char *remote_name, const unsigned char *sha1);
 
-extern void fetch_objects(const char *remote_name,
- const struct oid_array *to_fetch);
+extern int fetch_objects(const char *remote_name,
+const struct oid_array *to_fetch);
 
 #endif
diff --git a/sha1-file.c b/sha1-file.c
index 46072602ff..7d3c1ebd91 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -1290,8 +1290,8 @@ int oid_object_info_extended(const struct object_id *oid, 
struct object_info *oi
if (fetch_if_missing && repository_format_partial_clone &&
!already_retried) {
/*
-* TODO Investigate haveing fetch_object() return
-* TODO error/success and stopping the music here.
+* TODO Investigate checking fetch_object() return
+* TODO value and stopping on error here.
 */
fetch_object(repository_format_partial_clone, 
real->hash);
already_retried = 1;
-- 
2.17.0.590.gbd05bfcafd

[PATCH v1 6/8] Use odb_remote_get_direct() and has_external_odb()

[Christian Couder]

Instead of using the repository_format_partial_clone global
and fetch_object() directly, let's use has_odb_remote() and
odb_remote_get_direct().

Signed-off-by: Christian Couder 
---
 builtin/cat-file.c|  5 +++--
 builtin/fetch.c   | 11 ++-
 builtin/gc.c  |  3 ++-
 builtin/repack.c  |  3 ++-
 cache.h   |  2 --
 connected.c   |  3 ++-
 environment.c |  1 -
 list-objects-filter-options.c | 27 +++
 packfile.c|  3 ++-
 setup.c   |  7 +--
 sha1-file.c   |  9 +
 t/t0410-partial-clone.sh  | 30 +++---
 t/t5500-fetch-pack.sh |  4 ++--
 t/t5601-clone.sh  |  2 +-
 t/t5616-partial-clone.sh  |  2 +-
 unpack-trees.c|  6 +++---
 16 files changed, 60 insertions(+), 58 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 2c46d257cd..25665b206a 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -13,6 +13,7 @@
 #include "tree-walk.h"
 #include "sha1-array.h"
 #include "packfile.h"
+#include "odb-remote.h"
 
 struct batch_options {
int enabled;
@@ -477,8 +478,8 @@ static int batch_objects(struct batch_options *opt)
 
for_each_loose_object(batch_loose_object, , 0);
for_each_packed_object(batch_packed_object, , 0);
-   if (repository_format_partial_clone)
-   warning("This repository has extensions.partialClone 
set. Some objects may not be loaded.");
+   if (has_odb_remote())
+   warning("This repository uses an odb. Some objects may 
not be loaded.");
 
cb.opt = opt;
cb.expand = 
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7ee83ac0f8..82a3e655ba 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -20,6 +20,7 @@
 #include "utf8.h"
 #include "packfile.h"
 #include "list-objects-filter-options.h"
+#include "odb-remote.h"
 
 static const char * const builtin_fetch_usage[] = {
N_("git fetch [] [ [...]]"),
@@ -1320,7 +1321,7 @@ static inline void fetch_one_setup_partial(struct remote 
*remote)
 * If no prior partial clone/fetch and the current fetch DID NOT
 * request a partial-fetch, do a normal fetch.
 */
-   if (!repository_format_partial_clone && !filter_options.choice)
+   if (!has_odb_remote() && !filter_options.choice)
return;
 
/*
@@ -1328,7 +1329,7 @@ static inline void fetch_one_setup_partial(struct remote 
*remote)
 * on this repo and remember the given filter-spec as the default
 * for subsequent fetches to this remote.
 */
-   if (!repository_format_partial_clone && filter_options.choice) {
+   if (!has_odb_remote() && filter_options.choice) {
partial_clone_register(remote->name, _options);
return;
}
@@ -1337,7 +1338,7 @@ static inline void fetch_one_setup_partial(struct remote 
*remote)
 * We are currently limited to only ONE promisor remote and only
 * allow partial-fetches from the promisor remote.
 */
-   if (strcmp(remote->name, repository_format_partial_clone)) {
+   if (!find_odb_helper(remote->name)) {
if (filter_options.choice)
die(_("--filter can only be used with the remote 
configured in core.partialClone"));
return;
@@ -1473,7 +1474,7 @@ int cmd_fetch(int argc, const char **argv, const char 
*prefix)
if (depth || deepen_since || deepen_not.nr)
deepen = 1;
 
-   if (filter_options.choice && !repository_format_partial_clone)
+   if (filter_options.choice && !has_odb_remote())
die("--filter can only be used when extensions.partialClone is 
set");
 
if (all) {
@@ -1507,7 +1508,7 @@ int cmd_fetch(int argc, const char **argv, const char 
*prefix)
}
 
if (remote) {
-   if (filter_options.choice || repository_format_partial_clone)
+   if (filter_options.choice || has_odb_remote())
fetch_one_setup_partial(remote);
result = fetch_one(remote, argc, argv, prune_tags_ok);
} else {
diff --git a/builtin/gc.c b/builtin/gc.c
index d604940bb6..418cf2f0b0 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -22,6 +22,7 @@
 #include "commit.h"
 #include "packfile.h"
 #include "object-store.h"
+#include "odb-remote.h"
 
 #define FAILED_RUN "failed to run %s"
 
@@ -466,7 +467,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
argv_array_push(, prune_expire);
if (quiet)
argv_array_push(, "--no-progress");
-   if (repository_format_partial_clone)
+   if (has_odb_remote())
  

[PATCH v1 8/8] t0410: test fetching from many promisor remotes

[Christian Couder]

Signed-off-by: Christian Couder 
---
 t/t0410-partial-clone.sh | 24 +++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 6af4712da8..4a7a662512 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -162,8 +162,30 @@ test_expect_success 'fetching of missing objects' '
git verify-pack --verbose "$IDX" | grep "$HASH"
 '
 
+test_expect_success 'fetching of missing objects from another odb remote' '
+   git clone "file://$(pwd)/server" server2 &&
+   test_commit -C server2 bar &&
+   git -C server2 repack -a -d --write-bitmap-index &&
+   HASH2=$(git -C server2 rev-parse bar) &&
+
+   git -C repo remote add server2 "file://$(pwd)/server2" &&
+   git -C repo config odb.magic2.promisorRemote server2 &&
+   git -C repo cat-file -p "$HASH2" &&
+
+   git -C repo fetch server2 &&
+   rm -rf repo/.git/objects/* &&
+   git -C repo cat-file -p "$HASH2" &&
+
+   # Ensure that the .promisor file is written, and check that its
+   # associated packfile contains the object
+   ls repo/.git/objects/pack/pack-*.promisor >promisorlist &&
+   test_line_count = 1 promisorlist &&
+   IDX=$(cat promisorlist | sed "s/promisor$/idx/") &&
+   git verify-pack --verbose "$IDX" | grep "$HASH2"
+'
+
 test_expect_success 'rev-list stops traversal at missing and promised commit' '
-   rm -rf repo &&
+   rm -rf repo server server2 &&
test_create_repo repo &&
test_commit -C repo foo &&
test_commit -C repo bar &&
-- 
2.17.0.590.gbd05bfcafd

[PATCH v1 7/8] Use odb.origin.partialclonefilter instead of core.partialclonefilter

[Christian Couder]

Let's make the partial clone filter specific to one odb
instead of general to all the odbs.

This makes it possible to have different partial clone
filters for different odbs.

Signed-off-by: Christian Couder 
---
 builtin/fetch.c   |  2 +-
 list-objects-filter-options.c | 26 --
 list-objects-filter-options.h |  3 ++-
 odb-helper.h  |  1 +
 odb-remote.c  |  2 ++
 t/t5616-partial-clone.sh  |  2 +-
 6 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 82a3e655ba..288d58c0c5 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1350,7 +1350,7 @@ static inline void fetch_one_setup_partial(struct remote 
*remote)
 * the config.
 */
if (!filter_options.choice)
-   partial_clone_get_default_filter_spec(_options);
+   partial_clone_get_default_filter_spec(_options, 
remote->name);
return;
 }
 
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index f8a4642d17..59378dfb0a 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -7,6 +7,7 @@
 #include "list-objects-filter.h"
 #include "list-objects-filter-options.h"
 #include "odb-remote.h"
+#include "odb-helper.h"
 
 /*
  * Parse value of the argument to the "filter" keyword.
@@ -29,6 +30,9 @@ static int gently_parse_list_objects_filter(
 {
const char *v0;
 
+   if (!arg)
+   return 0;
+
if (filter_options->choice) {
if (errbuf) {
strbuf_init(errbuf, 0);
@@ -116,6 +120,7 @@ void partial_clone_register(
const struct list_objects_filter_options *filter_options)
 {
char *cfg_name;
+   char *filter_name;
 
/* Check if it is already registered */
if (find_odb_helper(remote))
@@ -131,25 +136,26 @@ void partial_clone_register(
/*
 * Record the initial filter-spec in the config as
 * the default for subsequent fetches from this remote.
-*
-* TODO: move core.partialclonefilter into odb.
 */
-   core_partial_clone_filter_default =
-   xstrdup(filter_options->filter_spec);
-   git_config_set("core.partialclonefilter",
-  core_partial_clone_filter_default);
+   filter_name = xstrfmt("odb.%s.partialclonefilter", remote);
+   git_config_set(filter_name, filter_options->filter_spec);
+   free(filter_name);
 
/* Make sure the config info are reset */
odb_remote_reinit();
 }
 
 void partial_clone_get_default_filter_spec(
-   struct list_objects_filter_options *filter_options)
+   struct list_objects_filter_options *filter_options,
+   const char *remote)
 {
+   struct odb_helper *helper = find_odb_helper(remote);
+
/*
 * Parse default value, but silently ignore it if it is invalid.
 */
-   gently_parse_list_objects_filter(filter_options,
-core_partial_clone_filter_default,
-NULL);
+   if (helper)
+   gently_parse_list_objects_filter(filter_options,
+helper->partial_clone_filter,
+NULL);
 }
diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h
index a61f82..12ceef3230 100644
--- a/list-objects-filter-options.h
+++ b/list-objects-filter-options.h
@@ -74,6 +74,7 @@ void partial_clone_register(
const char *remote,
const struct list_objects_filter_options *filter_options);
 void partial_clone_get_default_filter_spec(
-   struct list_objects_filter_options *filter_options);
+   struct list_objects_filter_options *filter_options,
+   const char *remote);
 
 #endif /* LIST_OBJECTS_FILTER_OPTIONS_H */
diff --git a/odb-helper.h b/odb-helper.h
index d63210d516..02bc3dbe13 100644
--- a/odb-helper.h
+++ b/odb-helper.h
@@ -4,6 +4,7 @@
 struct odb_helper {
const char *name;
const char *dealer;
+   const char *partial_clone_filter;
 
struct odb_helper *next;
 };
diff --git a/odb-remote.c b/odb-remote.c
index 0a734ff379..2c7c35c763 100644
--- a/odb-remote.c
+++ b/odb-remote.c
@@ -35,6 +35,8 @@ static int odb_remote_config(const char *var, const char 
*value, void *data)
 
if (!strcmp(subkey, "promisorremote"))
return git_config_string(>dealer, var, value);
+   if (!strcmp(subkey, "partialclonefilter"))
+   return git_config_string(>partial_clone_filter, var, value);
 
return 0;
 }
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 5ddd1e011c..4231121181 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -41,7 +41,7 @@ test_expect_success 'do partial clone 1' '
test_cmp expect_1.oids observed.oids &&
test "$(git 

[PATCH v1 4/8] odb-remote: implement odb_remote_get_many_direct()

[Christian Couder]

This function will be used to get many objects from a promisor
remote.

Signed-off-by: Christian Couder 
---
 odb-helper.c | 15 +++
 odb-helper.h |  3 +++
 odb-remote.c | 17 +
 odb-remote.h |  1 +
 4 files changed, 36 insertions(+)

diff --git a/odb-helper.c b/odb-helper.c
index 2851fe2657..5fe37e6fe5 100644
--- a/odb-helper.c
+++ b/odb-helper.c
@@ -28,3 +28,18 @@ int odb_helper_get_direct(struct odb_helper *o,
 
return res;
 }
+
+int odb_helper_get_many_direct(struct odb_helper *o,
+  const struct oid_array *to_get)
+{
+   int res;
+   uint64_t start;
+
+   start = getnanotime();
+
+   res = fetch_objects(o->dealer, to_get);
+
+   trace_performance_since(start, "odb_helper_get_many_direct");
+
+   return res;
+}
diff --git a/odb-helper.h b/odb-helper.h
index d21b625a7e..d63210d516 100644
--- a/odb-helper.h
+++ b/odb-helper.h
@@ -11,4 +11,7 @@ struct odb_helper {
 extern struct odb_helper *odb_helper_new(const char *name, int namelen);
 extern int odb_helper_get_direct(struct odb_helper *o,
 const unsigned char *sha1);
+extern int odb_helper_get_many_direct(struct odb_helper *o,
+ const struct oid_array *to_get);
+
 #endif /* ODB_HELPER_H */
diff --git a/odb-remote.c b/odb-remote.c
index dbab40c0f8..9a1561430c 100644
--- a/odb-remote.c
+++ b/odb-remote.c
@@ -87,3 +87,20 @@ int odb_remote_get_direct(const unsigned char *sha1)
 
return -1;
 }
+
+int odb_remote_get_many_direct(const struct oid_array *to_get)
+{
+   struct odb_helper *o;
+
+   trace_printf("trace: odb_remote_get_many_direct: nr: %d", to_get->nr);
+
+   odb_remote_init();
+
+   for (o = helpers; o; o = o->next) {
+   if (odb_helper_get_many_direct(o, to_get) < 0)
+   continue;
+   return 0;
+   }
+
+   return -1;
+}
diff --git a/odb-remote.h b/odb-remote.h
index 27a480fcfe..e6cedde722 100644
--- a/odb-remote.h
+++ b/odb-remote.h
@@ -4,5 +4,6 @@
 extern struct odb_helper *find_odb_helper(const char *dealer);
 extern int has_odb_remote(void);
 extern int odb_remote_get_direct(const unsigned char *sha1);
+extern int odb_remote_get_many_direct(const struct oid_array *to_get);
 
 #endif /* ODB_REMOTE_H */
-- 
2.17.0.590.gbd05bfcafd

[PATCH v1 3/8] odb-remote: implement odb_remote_get_direct()

[Christian Couder]

This is implemented only in the promisor remote mode
for now by calling fetch_object().

Signed-off-by: Christian Couder 
---
 odb-helper.c | 14 ++
 odb-helper.h |  3 ++-
 odb-remote.c | 17 +
 odb-remote.h |  1 +
 4 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/odb-helper.c b/odb-helper.c
index b4d403ffa9..2851fe2657 100644
--- a/odb-helper.c
+++ b/odb-helper.c
@@ -4,6 +4,7 @@
 #include "odb-helper.h"
 #include "run-command.h"
 #include "sha1-lookup.h"
+#include "fetch-object.h"
 
 struct odb_helper *odb_helper_new(const char *name, int namelen)
 {
@@ -14,3 +15,16 @@ struct odb_helper *odb_helper_new(const char *name, int 
namelen)
 
return o;
 }
+
+int odb_helper_get_direct(struct odb_helper *o,
+ const unsigned char *sha1)
+{
+   int res;
+   uint64_t start = getnanotime();
+
+   res = fetch_object(o->dealer, sha1);
+
+   trace_performance_since(start, "odb_helper_get_direct");
+
+   return res;
+}
diff --git a/odb-helper.h b/odb-helper.h
index 61d2ad082b..d21b625a7e 100644
--- a/odb-helper.h
+++ b/odb-helper.h
@@ -9,5 +9,6 @@ struct odb_helper {
 };
 
 extern struct odb_helper *odb_helper_new(const char *name, int namelen);
-
+extern int odb_helper_get_direct(struct odb_helper *o,
+const unsigned char *sha1);
 #endif /* ODB_HELPER_H */
diff --git a/odb-remote.c b/odb-remote.c
index e03b953ec6..dbab40c0f8 100644
--- a/odb-remote.c
+++ b/odb-remote.c
@@ -70,3 +70,20 @@ int has_odb_remote(void)
 {
return !!find_odb_helper(NULL);
 }
+
+int odb_remote_get_direct(const unsigned char *sha1)
+{
+   struct odb_helper *o;
+
+   trace_printf("trace: odb_remote_get_direct: %s", sha1_to_hex(sha1));
+
+   odb_remote_init();
+
+   for (o = helpers; o; o = o->next) {
+   if (odb_helper_get_direct(o, sha1) < 0)
+   continue;
+   return 0;
+   }
+
+   return -1;
+}
diff --git a/odb-remote.h b/odb-remote.h
index 68aa330244..27a480fcfe 100644
--- a/odb-remote.h
+++ b/odb-remote.h
@@ -3,5 +3,6 @@
 
 extern struct odb_helper *find_odb_helper(const char *dealer);
 extern int has_odb_remote(void);
+extern int odb_remote_get_direct(const unsigned char *sha1);
 
 #endif /* ODB_REMOTE_H */
-- 
2.17.0.590.gbd05bfcafd

[PATCH v1 2/8] Add initial odb remote support

[Christian Couder]

The odb-remote.{c,h} files will contain the functions
that are called by the rest of Git mostly from
"sha1_file.c" to access the objects managed by the
odb remotes.

The odb-helper.{c,h} files will contain the functions to
actually implement communication with either the internal
functions or the external scripts or processes that will
manage and provide remote git objects.

For now only infrastructure to create helpers from the
config and to check if there are odb remotes and helpers
is implemented.

Helped-by: Jeff King 
Signed-off-by: Christian Couder 
---
 Makefile |  2 ++
 odb-helper.c | 16 
 odb-helper.h | 13 ++
 odb-remote.c | 72 
 odb-remote.h |  7 +
 5 files changed, 110 insertions(+)
 create mode 100644 odb-helper.c
 create mode 100644 odb-helper.h
 create mode 100644 odb-remote.c
 create mode 100644 odb-remote.h

diff --git a/Makefile b/Makefile
index ad880d1fc5..f64dea287b 100644
--- a/Makefile
+++ b/Makefile
@@ -896,6 +896,8 @@ LIB_OBJS += notes-cache.o
 LIB_OBJS += notes-merge.o
 LIB_OBJS += notes-utils.o
 LIB_OBJS += object.o
+LIB_OBJS += odb-helper.o
+LIB_OBJS += odb-remote.o
 LIB_OBJS += oidmap.o
 LIB_OBJS += oidset.o
 LIB_OBJS += packfile.o
diff --git a/odb-helper.c b/odb-helper.c
new file mode 100644
index 00..b4d403ffa9
--- /dev/null
+++ b/odb-helper.c
@@ -0,0 +1,16 @@
+#include "cache.h"
+#include "object.h"
+#include "argv-array.h"
+#include "odb-helper.h"
+#include "run-command.h"
+#include "sha1-lookup.h"
+
+struct odb_helper *odb_helper_new(const char *name, int namelen)
+{
+   struct odb_helper *o;
+
+   o = xcalloc(1, sizeof(*o));
+   o->name = xmemdupz(name, namelen);
+
+   return o;
+}
diff --git a/odb-helper.h b/odb-helper.h
new file mode 100644
index 00..61d2ad082b
--- /dev/null
+++ b/odb-helper.h
@@ -0,0 +1,13 @@
+#ifndef ODB_HELPER_H
+#define ODB_HELPER_H
+
+struct odb_helper {
+   const char *name;
+   const char *dealer;
+
+   struct odb_helper *next;
+};
+
+extern struct odb_helper *odb_helper_new(const char *name, int namelen);
+
+#endif /* ODB_HELPER_H */
diff --git a/odb-remote.c b/odb-remote.c
new file mode 100644
index 00..e03b953ec6
--- /dev/null
+++ b/odb-remote.c
@@ -0,0 +1,72 @@
+#include "cache.h"
+#include "odb-remote.h"
+#include "odb-helper.h"
+#include "config.h"
+
+static struct odb_helper *helpers;
+static struct odb_helper **helpers_tail = 
+
+static struct odb_helper *find_or_create_helper(const char *name, int len)
+{
+   struct odb_helper *o;
+
+   for (o = helpers; o; o = o->next)
+   if (!strncmp(o->name, name, len) && !o->name[len])
+   return o;
+
+   o = odb_helper_new(name, len);
+   *helpers_tail = o;
+   helpers_tail = >next;
+
+   return o;
+}
+
+static int odb_remote_config(const char *var, const char *value, void *data)
+{
+   struct odb_helper *o;
+   const char *name;
+   int namelen;
+   const char *subkey;
+
+   if (parse_config_key(var, "odb", , , ) < 0)
+   return 0;
+
+   o = find_or_create_helper(name, namelen);
+
+   if (!strcmp(subkey, "promisorremote"))
+   return git_config_string(>dealer, var, value);
+
+   return 0;
+}
+
+static void odb_remote_init(void)
+{
+   static int initialized;
+
+   if (initialized)
+   return;
+   initialized = 1;
+
+   git_config(odb_remote_config, NULL);
+}
+
+struct odb_helper *find_odb_helper(const char *dealer)
+{
+   struct odb_helper *o;
+
+   odb_remote_init();
+
+   if (!dealer)
+   return helpers;
+
+   for (o = helpers; o; o = o->next)
+   if (!strcmp(o->dealer, dealer))
+   return o;
+
+   return NULL;
+}
+
+int has_odb_remote(void)
+{
+   return !!find_odb_helper(NULL);
+}
diff --git a/odb-remote.h b/odb-remote.h
new file mode 100644
index 00..68aa330244
--- /dev/null
+++ b/odb-remote.h
@@ -0,0 +1,7 @@
+#ifndef ODB_REMOTE_H
+#define ODB_REMOTE_H
+
+extern struct odb_helper *find_odb_helper(const char *dealer);
+extern int has_odb_remote(void);
+
+#endif /* ODB_REMOTE_H */
-- 
2.17.0.590.gbd05bfcafd

[PATCH v1 5/8] odb-remote: add odb_remote_reinit()

[Christian Couder]

We will need to reinitialize the odb remote configuration
as we will make some changes to it in a later commit when
we will detect that a remote is also an odb remote.

Signed-off-by: Christian Couder 
---
 odb-remote.c | 14 --
 odb-remote.h |  1 +
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/odb-remote.c b/odb-remote.c
index 9a1561430c..0a734ff379 100644
--- a/odb-remote.c
+++ b/odb-remote.c
@@ -39,17 +39,27 @@ static int odb_remote_config(const char *var, const char 
*value, void *data)
return 0;
 }
 
-static void odb_remote_init(void)
+static void odb_remote_do_init(int force)
 {
static int initialized;
 
-   if (initialized)
+   if (!force && initialized)
return;
initialized = 1;
 
git_config(odb_remote_config, NULL);
 }
 
+static inline void odb_remote_init(void)
+{
+   odb_remote_do_init(0);
+}
+
+inline void odb_remote_reinit(void)
+{
+   odb_remote_do_init(1);
+}
+
 struct odb_helper *find_odb_helper(const char *dealer)
 {
struct odb_helper *o;
diff --git a/odb-remote.h b/odb-remote.h
index e6cedde722..d862216a8f 100644
--- a/odb-remote.h
+++ b/odb-remote.h
@@ -1,6 +1,7 @@
 #ifndef ODB_REMOTE_H
 #define ODB_REMOTE_H
 
+extern void odb_remote_reinit(void);
 extern struct odb_helper *find_odb_helper(const char *dealer);
 extern int has_odb_remote(void);
 extern int odb_remote_get_direct(const unsigned char *sha1);
-- 
2.17.0.590.gbd05bfcafd

[PATCH v1 0/8] Introducing odb remote

[Christian Couder]

This is a follow up from the patch series called "Promisor remotes and
external ODB support" that I sent earlier this year.

Following discussions of these patch series, where Junio said "a
minimum s/ext/remote/ would clarify what it is", I decided to rename
"external odb" to "odb remote". I am still open to another name, but I
think that "odb remote" works well with "odb helper" that was already
used in the series and is as good or perhaps better than "remote odb",
as a "remote odb" I think would be easier to confuse with a regular
"remote".

Another obvious difference with the previous series is that this
series is only about integrating with the promisor/narrow clone work
and showing that it makes it possible to use more than one promisor
remote. Everything that is not necessary for that integration has been
removed for now (though you can still find it in one of my branches on
GitHub if you want).

This makes this patch series much shorter than the previous ones and
already useful. So hopefully this will make it possible to start
reviewing and merging it.

There is one test in patch 8/8 that shows that more than one promisor
remote can now be used, but I feel that it could be interesting to add
other such tests, so I am open to ideas in this area.

High level overview of this patch series


  - Patch 1/8:

This makes functions in fetch-object.c return an error code, which is
necessary to later tell that they failed and try another odb remote
when there is more than one. This could also just be seen as a fix to
these functions.

  - Patch 2/8:

This introduces the minimum infrastructure for odb remotes.

  - Patches 3/8 and 4/8:

These patches implement odb_remote_get_direct() and
odb_remote_get_many_direct() using the functions from fetch-object.c.
These new functions will be used in following patches to replace the
functions from fetch-object.c.

  - Patch 5/8:

This implement odb_remote_reinit() which will be needed to reparse the
odb remote configuration.

  - Patches 6/8 and 7/8:

These patches integrate the odb remote mechanism into the
promisor/narrow clone code. The "extensions.partialclone" config
option is replaced by "odb..promisorRemote" and
"core.partialclonefilter" is replaced by "odb..partialclonefilter".

  - Patch 8/8:

This adds a test case that shows that now more than one promisor
remote can be used.

Links
~

This patch series on GitHub:

https://github.com/chriscool/git/commits/odb-remote

Version 1 and 2 of the "Promisor remotes and external ODB support" series:

https://public-inbox.org/git/20180103163403.11303-1-chrisc...@tuxfamily.org/
https://public-inbox.org/git/20180319133147.15413-1-chrisc...@tuxfamily.org/

Version 1 and 2 of the "Promisor remotes and external ODB support" series on 
GitHub:

https://github.com/chriscool/git/commits/gl-small-promisor-external-odb12
https://github.com/chriscool/git/commits/gl-small-promisor-external-odb71


Christian Couder (8):
  fetch-object: make functions return an error code
  Add initial odb remote support
  odb-remote: implement odb_remote_get_direct()
  odb-remote: implement odb_remote_get_many_direct()
  odb-remote: add odb_remote_reinit()
  Use odb_remote_get_direct() and has_external_odb()
  Use odb.origin.partialclonefilter instead of core.partialclonefilter
  t0410: test fetching from many promisor remotes

 Makefile  |   2 +
 builtin/cat-file.c|   5 +-
 builtin/fetch.c   |  13 ++--
 builtin/gc.c  |   3 +-
 builtin/repack.c  |   3 +-
 cache.h   |   2 -
 connected.c   |   3 +-
 environment.c |   1 -
 fetch-object.c|  15 +++--
 fetch-object.h|   6 +-
 list-objects-filter-options.c |  49 --
 list-objects-filter-options.h |   3 +-
 odb-helper.c  |  45 +
 odb-helper.h  |  18 ++
 odb-remote.c  | 118 ++
 odb-remote.h  |  10 +++
 packfile.c|   3 +-
 setup.c   |   7 +-
 sha1-file.c   |   9 +--
 t/t0410-partial-clone.sh  |  54 +++-
 t/t5500-fetch-pack.sh |   4 +-
 t/t5601-clone.sh  |   2 +-
 t/t5616-partial-clone.sh  |   4 +-
 unpack-trees.c|   6 +-
 24 files changed, 306 insertions(+), 79 deletions(-)
 create mode 100644 odb-helper.c
 create mode 100644 odb-helper.h
 create mode 100644 odb-remote.c
 create mode 100644 odb-remote.h

-- 
2.17.0.590.gbd05bfcafd

Re: [PATCH] config: free resources of `struct config_store_data`

[Martin Ågren]

On 13 May 2018 at 10:59, Eric Sunshine  wrote:
> On Sun, May 13, 2018 at 4:23 AM Martin Ågren  wrote:
>
>> Introduce and use a small helper function `config_store_data_clear()` to
>> plug these leaks. This should be safe. The memory tracked here is config
>> parser events. Once the users (`git_config_set_multivar_in_file_gently()`
>> and `git_config_copy_or_rename_section_in_file()` at the moment) are
>> done, no-one should be holding on to a pointer into this memory.
>
> A newcomer to this code (such as myself) might legitimately wonder why
> store->key and store->value_regex are not also being cleaned up by this

Good point. I was only concerned by the members that no-one took
responsibility for.

> function. An examination of the relevant code reveals that those structure
> members are manually (and perhaps tediously) freed already by
> git_config_set_multivar_in_file_gently(), however, if
> config_store_data_clear() was responsible for freeing them, then
> git_config_set_multivar_in_file_gently() could be made a bit less noisy.

Yes, that's true.

> On the other hand, if there's actually a good reason for
>   config_store_data_clear() not freeing those members, then perhaps it
> deserves an in-code comment explaining why they are exempt.

Not any good reason that I can think of, other than "history". But to be
clear, a day ago I was as much of a newcomer in this part of the code as
you are. Johannes is the one who might have the most up-to-date
understanding of this.

How about the following two patches as patches 2/3 and 3/3? I would also
need to mention in the commit message of this patch (1/3) that the
function will soon learn to clean up more members.

I could of course squash the three patches into one, but there is some
minor trickery involved, like we can't reuse a pointer in one place, but
need to xstrdup it.

Thank you for your comments. I'd be very interested in your thoughts on
this.

Martin

Martin Ågren (2):
  config: let `config_store_data_clear()` handle `value_regex`
  config: let `config_store_data_clear()` handle `key`

 config.c | 26 --
 1 file changed, 8 insertions(+), 18 deletions(-)

-- 
2.17.0.583.g9a75a153ac

[PATCH 3/1] config: let `config_store_data_clear()` handle `key`

[Martin Ågren]

Instead of carefully clearing up `key` in each code path, let
`config_store_data_clear()` handle that.

We still need to free it before replacing it, though. Move that freeing
closer to the replacing to be safe. Note that in that same part of the
code, we can no longer set `key` to the original pointer, but need to
`xstrdup()` it.

Signed-off-by: Martin Ågren 
---
 config.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/config.c b/config.c
index 2e3c6c94e9..963edacf10 100644
--- a/config.c
+++ b/config.c
@@ -2335,6 +2335,7 @@ struct config_store_data {
 
 void config_store_data_clear(struct config_store_data *store)
 {
+   free(store->key);
if (store->value_regex != NULL &&
store->value_regex != CONFIG_REGEX_NONE) {
regfree(store->value_regex);
@@ -2679,7 +2680,6 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
fd = hold_lock_file_for_update(, config_filename, 0);
if (fd < 0) {
error_errno("could not lock config file %s", config_filename);
-   free(store.key);
ret = CONFIG_NO_LOCK;
goto out_free;
}
@@ -2689,8 +2689,6 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
 */
in_fd = open(config_filename, O_RDONLY);
if ( in_fd < 0 ) {
-   free(store.key);
-
if ( ENOENT != errno ) {
error_errno("opening %s", config_filename);
ret = CONFIG_INVALID_FILE; /* same as "invalid config 
file" */
@@ -2702,7 +2700,8 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
goto out_free;
}
 
-   store.key = (char *)key;
+   free(store.key);
+   store.key = xstrdup(key);
if (write_section(fd, key, ) < 0 ||
write_pair(fd, key, value, ) < 0)
goto write_err_out;
@@ -2751,13 +2750,10 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
  config_filename,
  , )) {
error("invalid config file %s", config_filename);
-   free(store.key);
ret = CONFIG_INVALID_FILE;
goto out_free;
}
 
-   free(store.key);
-
/* if nothing to unset, or too many matches, error out */
if ((store.seen_nr == 0 && value == NULL) ||
(store.seen_nr > 1 && multi_replace == 0)) {
-- 
2.17.0.583.g9a75a153ac

[PATCH 2/1] config: let `config_store_data_clear()` handle `value_regex`

[Martin Ågren]

Instead of carefully clearing up `value_regex` in each code path, let
`config_store_data_clear()` handle that.

Signed-off-by: Martin Ågren 
---
I *think* that it should be ok to `regfree()` after `regcomp()` failed,
but I'll need to look into that some more (and say something about it in
the commit message).

 config.c | 16 +---
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/config.c b/config.c
index 83d7d0851a..2e3c6c94e9 100644
--- a/config.c
+++ b/config.c
@@ -2335,6 +2335,11 @@ struct config_store_data {
 
 void config_store_data_clear(struct config_store_data *store)
 {
+   if (store->value_regex != NULL &&
+   store->value_regex != CONFIG_REGEX_NONE) {
+   regfree(store->value_regex);
+   free(store->value_regex);
+   }
free(store->parsed);
free(store->seen);
memset(store, 0, sizeof(*store));
@@ -2722,7 +2727,6 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
if (regcomp(store.value_regex, value_regex,
REG_EXTENDED)) {
error("invalid pattern: %s", value_regex);
-   free(store.value_regex);
ret = CONFIG_INVALID_PATTERN;
goto out_free;
}
@@ -2748,21 +2752,11 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
  , )) {
error("invalid config file %s", config_filename);
free(store.key);
-   if (store.value_regex != NULL &&
-   store.value_regex != CONFIG_REGEX_NONE) {
-   regfree(store.value_regex);
-   free(store.value_regex);
-   }
ret = CONFIG_INVALID_FILE;
goto out_free;
}
 
free(store.key);
-   if (store.value_regex != NULL &&
-   store.value_regex != CONFIG_REGEX_NONE) {
-   regfree(store.value_regex);
-   free(store.value_regex);
-   }
 
/* if nothing to unset, or too many matches, error out */
if ((store.seen_nr == 0 && value == NULL) ||
-- 
2.17.0.583.g9a75a153ac

[PATCH] travis-ci: run gcc-7 on linux-gcc jobs

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

Switch from gcc-4.8 to gcc-7. Newer compilers come with more warning
checks (usually in -Wextra).  Since -Wextra is enabled in developer
mode (which is also enabled in travis), this lets travis report more
warnings before other people do it.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 .travis.yml| 3 +++
 ci/lib-travisci.sh | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/.travis.yml b/.travis.yml
index 5f5ee4f3bd..a77f5f5bd5 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -16,10 +16,13 @@ compiler:
 
 addons:
   apt:
+sources:
+- ubuntu-toolchain-r-test
 packages:
 - language-pack-is
 - git-svn
 - apache2
+- gcc-7
 
 matrix:
   include:
diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
index 109ef280da..ef2848fd45 100755
--- a/ci/lib-travisci.sh
+++ b/ci/lib-travisci.sh
@@ -99,6 +99,9 @@ export DEFAULT_TEST_TARGET=prove
 export GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
 export GIT_TEST_OPTS="--verbose-log -x"
 export GIT_TEST_CLONE_2GB=YesPlease
+if [ "$jobname" = linux-gcc ]; then
+   export CC=gcc-7
+fi
 
 case "$jobname" in
 linux-clang|linux-gcc)
-- 
2.17.0.705.g3525833791

Re: [PATCH] config: free resources of `struct config_store_data`

[Eric Sunshine]

On Sun, May 13, 2018 at 4:23 AM Martin Ågren  wrote:
> Commit fee8572c6d (config: avoid using the global variable `store`,
> 2018-04-09) dropped the staticness of a certain struct, instead letting
> the users create an instance on the stack and pass around a pointer.

> We do not free the memory that the struct tracks. When the struct was
> static, the memory would always be reachable. Now that we keep the
> struct on the stack, though, as soon as we return, it goes out of scope
> and we leak the memory it points to.

> Introduce and use a small helper function `config_store_data_clear()` to
> plug these leaks. This should be safe. The memory tracked here is config
> parser events. Once the users (`git_config_set_multivar_in_file_gently()`
> and `git_config_copy_or_rename_section_in_file()` at the moment) are
> done, no-one should be holding on to a pointer into this memory.

> Signed-off-by: Martin Ågren 
> ---
> diff --git a/config.c b/config.c
> @@ -2333,6 +2333,13 @@ struct config_store_data {
> +void config_store_data_clear(struct config_store_data *store)
> +{
> +   free(store->parsed);
> +   free(store->seen);
> +   memset(store, 0, sizeof(*store));
> +}

A newcomer to this code (such as myself) might legitimately wonder why
store->key and store->value_regex are not also being cleaned up by this
function. An examination of the relevant code reveals that those structure
members are manually (and perhaps tediously) freed already by
git_config_set_multivar_in_file_gently(), however, if
config_store_data_clear() was responsible for freeing them, then
git_config_set_multivar_in_file_gently() could be made a bit less noisy.

On the other hand, if there's actually a good reason for
  config_store_data_clear() not freeing those members, then perhaps it
deserves an in-code comment explaining why they are exempt.

[PATCH] config: free resources of `struct config_store_data`

[Martin Ågren]

Commit fee8572c6d (config: avoid using the global variable `store`,
2018-04-09) dropped the staticness of a certain struct, instead letting
the users create an instance on the stack and pass around a pointer.

We do not free the memory that the struct tracks. When the struct was
static, the memory would always be reachable. Now that we keep the
struct on the stack, though, as soon as we return, it goes out of scope
and we leak the memory it points to.

Introduce and use a small helper function `config_store_data_clear()` to
plug these leaks. This should be safe. The memory tracked here is config
parser events. Once the users (`git_config_set_multivar_in_file_gently()`
and `git_config_copy_or_rename_section_in_file()` at the moment) are
done, no-one should be holding on to a pointer into this memory.

Signed-off-by: Martin Ågren 
---
 config.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/config.c b/config.c
index 6f8f1d8c11..83d7d0851a 100644
--- a/config.c
+++ b/config.c
@@ -2333,6 +2333,13 @@ struct config_store_data {
unsigned int key_seen:1, section_seen:1, is_keys_section:1;
 };
 
+void config_store_data_clear(struct config_store_data *store)
+{
+   free(store->parsed);
+   free(store->seen);
+   memset(store, 0, sizeof(*store));
+}
+
 static int matches(const char *key, const char *value,
   const struct config_store_data *store)
 {
@@ -2887,6 +2894,7 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
munmap(contents, contents_sz);
if (in_fd >= 0)
close(in_fd);
+   config_store_data_clear();
return ret;
 
 write_err_out:
@@ -3127,6 +3135,7 @@ static int 
git_config_copy_or_rename_section_in_file(const char *config_filename
rollback_lock_file();
 out_no_rollback:
free(filename_buf);
+   config_store_data_clear();
return ret;
 }
 
-- 
2.17.0.583.g9a75a153ac

Re: [PATCH v2 17/28] t4014: abstract away SHA-1-specific constants

[Johannes Sixt]

Am 13.05.2018 um 04:24 schrieb brian m. carlson:

Adjust the test so that it computes values for blobs instead of using
hard-coded hashes.

Signed-off-by: brian m. carlson 
---
  t/t4014-format-patch.sh | 7 ---
  1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index dac3f349a3..42b3e11207 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -578,7 +578,9 @@ test_expect_success 'excessive subject' '
  
  	rm -rf patches/ &&

git checkout side &&
+   before=$(git rev-parse --short $(git hash-object file)) &&
for i in 5 6 1 2 3 A 4 B C 7 8 9 10 D E F; do echo "$i"; done >>file &&
+   after=$(git rev-parse --short $(git hash-object file)) &&


It would be better to avoid process expansion in command arguments, 
because the shell does not diagnose failures. This is preferable:


before=$(git hash-object file) &&
before=$(git rev-parse --short $before) &&

-- Hannes

Re: [PATCH v7 12/13] completion: let git provide the completable command list

[Duy Nguyen]

On Fri, May 11, 2018 at 5:05 PM, SZEDER Gábor  wrote:
> On Thu, May 10, 2018 at 10:46 AM, Nguyễn Thái Ngọc Duy
>  wrote:
>> Instead of maintaining a separate list of command classification,
>> which often could go out of date, let's centralize the information
>> back in git.
>>
>> While the function in git-completion.bash implies "list porcelain
>> commands", that's not exactly what it does. It gets all commands (aka
>> --list-cmds=main,others) then exclude certain non-porcelain ones. We
>> could almost recreate this list two lists list-mainporcelain and
>> others. The non-porcelain-but-included-anyway is added by the third
>> category list-complete.
>>
>> list-complete does not recreate exactly the command list before this
>> patch though. The following commands are not part of neither
>> list-mainporcelain nor list-complete and as a result no longer
>> completes:
>>
>> - annotate obsolete, discouraged to use
>> - difftool-helper  not an end user command
>> - filter-branchnot often used
>> - get-tar-commit-idnot often used
>> - imap-sendnot often used
>> - interpreter-trailers not for interactive use
>> - lost-found   obsolete
>> - p4   too short and probably not often used (*)
>> - peek-remote  deprecated
>> - svn  same category as p4 (*)
>> - tar-tree obsolete
>> - verify-commitnot often used
>
> 'git name-rev' is plumbing as well.

So? name-rev remains completable like before and is not mentioned in
the above list. Am I missing something?

> I think this commit should be split into two:
>
>   - first do the unequivocally beneficial thing and get rid of the
> long, hard-coded command list in __git_list_porcelain_commands(),
> while keeping its output unchanged,
>
>   - then do the arguable thing and change the list of commands.

I will. Though the first commit still changes the output slightly
because there are three commands not in command-list.txt. To keep the
output unchanged, I would need to add them back in command-list.txt
first (and find the right group for them) only to remove those lines
later (two of them deprecated, the other does not even have a man
page). It's not worth the effort.

>>  {
>> local i IFS=" "$'\n'
>> -   for i in $(__git_commands)
>> +   for i in $(__git_commands $1)
>> do
>> case $i in
>> *--*) : helper pattern;;
>
> Is this loop to exclude helper commands with doubledash in their name
> still necessary?

It is needed for __git_list_all_commands() because that one still
essentially grabs "git help -a", which includes command--helpers.
-- 
Duy

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

[Duy Nguyen]

On Sun, May 13, 2018 at 4:23 AM, Dannier Castro L  wrote:
> Currently,  is a complex command able to handle both
> branches and files without any distintion other than their names,
> taking into account that depending on the type (branch or file),
> the functionality is completely different, the easier example:
>
> $ git checkout   # Switch from current branch to .
> $ git checkout # Restore  from HEAD, discarding
>  # changes if it's necessary.
> $ git checkout --  # The same as the last one, only with an
>  # useless '--'.
>
> For GIT new users, this complicated versatility of  could
> be very confused, also considering that actually the flag '--' is
> completely useless (added or not, there is not any difference for
> this command), when the same program messages promote the use of
> this flag.

I would like an option to revert back to current behavior. I'm not a
new user. I know what I'm doing. Please don't make me type more.

And '--" is not completely useless. If you have  and 
with the same name, you have to give "--" to to tell git what the
first argument means.

> Regarding the 's power to overwrite any file, discarding
> changes if they exist without some way of recovering them, the
> solution propuses that the usage of '--' is strict before to
> specify the file(s) path(s) for any  command (including
> all types of flags), as a "defense barrier" to make sure about
> user's knowledge and intension running .
>
> The solution consists in detect '--' into command args, allowing
> the discard of changes and considering the following names as
> file paths, otherwise, they are branch names.
-- 
Duy