date:20180514

Antonio Ospite  writes:

>   - my git terminology may still be a little off: do "work tree" and
> "work directory" mean the same thing?

Just on this tangent.  

When we talk about the current working directory of a process
returned by getcwd((3) call, we typically spell that word fully (or
say $cwd).

We lived without the modern "git worktree" layout for a long time,
but there was a hack in contrib/ called "git-new-workdir".  This
creates something similar (in spirit, not in implementation) to
"worktree" in modern Git lingo, but because not many people use the
contrib feature (at least there is only few who get confused by it
and ask questions publicly, anyway), we do not hear "workdir" very
often.

Since very early days of Git, we had "working tree" that is the
directory hierarchy that corresponds to what you place in the index,
and "Git repository" which is the ".git" directory that has that
index.  Even though most everybody else was calling it the "working
tree", primarily because I was young(er) and (more) stupid, I often
called the same thing "work tree", and made things worse by
introducing GIT_WORK_TREE environment variable etc.  But "working
tree" is the official terminology to denote a directory hierarchy
that corresponds to (and controlled by) a single index file as
opposed to what is in ".git/" directory..

The official term "worktree" came much later, with "git worktree"
command.  This allow multiple working trees to be associated with a
single ".git/" directory.  Most of the time "worktree" and "working
tree" can be used interchangeably, but we tend to say "working tree"
when we have more emphasis on the non-bareness of the repository and
talking about checked-out files, and say "worktree" when we are
mostly interested in the layout that have more than one working trees
associated to a single Git repository.  What you get by "git clone",
for example, is just a single repository with a single working tree,
and nobody sane (other than young(er) and (more) stupid version of
me 10 years ago) would call the latter a "worktree" these days, as
there is not yet a secondary worktree to contrast it with.

Re: [PATCH RFC] ref-filter: start using oid_object_info

Junio C Hamano  writes:

> Olga Telezhnaya  writes:
>
>> Start using oid_object_info_extended(). So, if info from this function
>> is enough, we do not need to get and parse whole object (as it was before).
>> It's good for 3 reasons:
>> 1. Some Git commands potentially will work faster.
>> 2. It's much easier to add support for objectsize:disk and deltabase.
>>(I have plans to add this support further)
>> 3. It's easier to move formatting from cat-file command to this logic
>>(It pretends to be unified formatting logic in the end)
>>
>> Signed-off-by: Olga Telezhnaia 
>> ---
>>  ref-filter.c | 34 +++---
>>  ref-filter.h | 21 +
>>  2 files changed, 52 insertions(+), 3 deletions(-)
>>...
>> @@ -383,9 +400,9 @@ static struct {
>>  int (*parser)(const struct ref_format *format, struct used_atom *atom,
>>const char *arg, struct strbuf *err);
>>  } valid_atom[] = {
>> -{ "refname" , FIELD_STR, refname_atom_parser },
>> -{ "objecttype" },
>> -{ "objectsize", FIELD_ULONG },
>> +{ "refname", FIELD_STR, refname_atom_parser },
>> +{ "objecttype", FIELD_STR, objecttype_atom_parser },
>> +{ "objectsize", FIELD_ULONG, objectsize_atom_parser },
>>  { "objectname", FIELD_STR, objectname_atom_parser },
>>  { "tree" },
>>  { "parent" },
>
> Hmph, so this patch does not teach us to interpolate any new %(field-type)
> but changes the way %(objecttype) and %(objectsize) are computed.
>
>> @@ -1536,6 +1553,13 @@ static int populate_value(struct ref_array_item *ref, 
>> struct strbuf *err)
>>  continue;
>>  } else if (!deref && grab_objectname(name, >objectname, v, 
>> atom)) {
>>  continue;
>> +} else if (!deref && !strcmp(name, "objecttype")) {
>> +v->s = type_name(format_data.type);
>> +continue;
>> +} else if (!deref && !strcmp(name, "objectsize")) {
>> +v->value = format_data.size;
>> +v->s = xstrfmt("%lu", format_data.size);
>> +continue;
>>  } else if (!strcmp(name, "HEAD")) {
>>  if (atom->u.head && !strcmp(ref->refname, atom->u.head))
>>  v->s = "*";
>
> Because this addition is made to the early "Fill in specials first"
> loop of the populate_value() function, we may be able to satisfy
> some requests early without calling get_object() which then calls
> parse_object().
>
>> @@ -2226,6 +2250,10 @@ int format_ref_array_item(struct ref_array_item *info,
>>  {
>>  const char *cp, *sp, *ep;
>>  struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
>> +format_data.oid = info->objectname;
>> +if (format_data.use_data && oid_object_info_extended(_data.oid, 
>> _data.info,
>
> Style: fold the line after " &&".
>
> And this checks the .use_data field to see if these fields whose
> value could be computed by a call to oid_object_info_extended()
> without calling parse_object().  If there is one, we call it;
> otherwise we don't.
>
> So there are three possible cases:
>
>  - The request does not ask for these fields that can be filled from
>"format_data" (by the way, that is a horrible name---all the data
>in this codepath is for formatting, and in that sense the
>variable is not named after its most significant trait, which is
>that it is to grab data needed for formatting via a call to
>a function in the object_info() family.  Perhaps object_info_data
>or oi_data for brevity).  We do not call object_info() and the
>performance characteristic of the code stays as before.
>
>  - The request asks for these fields that are helped by
>"object-info" and no other fields.  We make a call to
>"object-info", instead of parse_object(), which hopefully is more
>efficient (we need to measure, if we are selling this as an
>optimization).
>
>  - The request asks for both.  We end up calling object-info and
>also parse_object(), so presumably there is degradation of
>performance.
>
> In the third case, after v->s and v->value are filled by the new
> code that copies from format_data, grab_values() will again fill
> objecttype/objectsize by overwriting v->s field.  Doesn't this cause
> memory leaks?  type_name() returns a constant string that does not
> leak, but your objectsize seems to use xstrfmt(), so...
>
> I think it was OK before this patch as grab_common_values() was the
> only place that did v->s = xstrfmt() for the field, but now the code
> with this patch can do the same assignment from two places, we would
> need to be a bit more careful about memory ownership?

Another thing that came to my mind while reading the patch aloud in
my previous message was if we can easily tell the latter two cases
apart, before actually going into the

Re: [PATCH RFC] ref-filter: start using oid_object_info

Olga Telezhnaya  writes:

> Start using oid_object_info_extended(). So, if info from this function
> is enough, we do not need to get and parse whole object (as it was before).
> It's good for 3 reasons:
> 1. Some Git commands potentially will work faster.
> 2. It's much easier to add support for objectsize:disk and deltabase.
>(I have plans to add this support further)
> 3. It's easier to move formatting from cat-file command to this logic
>(It pretends to be unified formatting logic in the end)
>
> Signed-off-by: Olga Telezhnaia 
> ---
>  ref-filter.c | 34 +++---
>  ref-filter.h | 21 +
>  2 files changed, 52 insertions(+), 3 deletions(-)
>...
> @@ -383,9 +400,9 @@ static struct {
>   int (*parser)(const struct ref_format *format, struct used_atom *atom,
> const char *arg, struct strbuf *err);
>  } valid_atom[] = {
> - { "refname" , FIELD_STR, refname_atom_parser },
> - { "objecttype" },
> - { "objectsize", FIELD_ULONG },
> + { "refname", FIELD_STR, refname_atom_parser },
> + { "objecttype", FIELD_STR, objecttype_atom_parser },
> + { "objectsize", FIELD_ULONG, objectsize_atom_parser },
>   { "objectname", FIELD_STR, objectname_atom_parser },
>   { "tree" },
>   { "parent" },

Hmph, so this patch does not teach us to interpolate any new %(field-type)
but changes the way %(objecttype) and %(objectsize) are computed.

> @@ -1536,6 +1553,13 @@ static int populate_value(struct ref_array_item *ref, 
> struct strbuf *err)
>   continue;
>   } else if (!deref && grab_objectname(name, >objectname, v, 
> atom)) {
>   continue;
> + } else if (!deref && !strcmp(name, "objecttype")) {
> + v->s = type_name(format_data.type);
> + continue;
> + } else if (!deref && !strcmp(name, "objectsize")) {
> + v->value = format_data.size;
> + v->s = xstrfmt("%lu", format_data.size);
> + continue;
>   } else if (!strcmp(name, "HEAD")) {
>   if (atom->u.head && !strcmp(ref->refname, atom->u.head))
>   v->s = "*";

Because this addition is made to the early "Fill in specials first"
loop of the populate_value() function, we may be able to satisfy
some requests early without calling get_object() which then calls
parse_object().

> @@ -2226,6 +2250,10 @@ int format_ref_array_item(struct ref_array_item *info,
>  {
>   const char *cp, *sp, *ep;
>   struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
> + format_data.oid = info->objectname;
> + if (format_data.use_data && oid_object_info_extended(_data.oid, 
> _data.info,

Style: fold the line after " &&".

And this checks the .use_data field to see if these fields whose
value could be computed by a call to oid_object_info_extended()
without calling parse_object().  If there is one, we call it;
otherwise we don't.

So there are three possible cases:

 - The request does not ask for these fields that can be filled from
   "format_data" (by the way, that is a horrible name---all the data
   in this codepath is for formatting, and in that sense the
   variable is not named after its most significant trait, which is
   that it is to grab data needed for formatting via a call to
   a function in the object_info() family.  Perhaps object_info_data
   or oi_data for brevity).  We do not call object_info() and the
   performance characteristic of the code stays as before.

 - The request asks for these fields that are helped by
   "object-info" and no other fields.  We make a call to
   "object-info", instead of parse_object(), which hopefully is more
   efficient (we need to measure, if we are selling this as an
   optimization).

 - The request asks for both.  We end up calling object-info and
   also parse_object(), so presumably there is degradation of
   performance.

In the third case, after v->s and v->value are filled by the new
code that copies from format_data, grab_values() will again fill
objecttype/objectsize by overwriting v->s field.  Doesn't this cause
memory leaks?  type_name() returns a constant string that does not
leak, but your objectsize seems to use xstrfmt(), so...

I think it was OK before this patch as grab_common_values() was the
only place that did v->s = xstrfmt() for the field, but now the code
with this patch can do the same assignment from two places, we would
need to be a bit more careful about memory ownership?

Re: [RFC PATCH 09/10] submodule: support reading .gitmodules even when it's not checked out

On Mon, May 14, 2018 at 3:58 AM, Antonio Ospite  wrote:
> When the .gitmodules file is not available in the working directory, try
> using HEAD:.gitmodules from the index.

I think HEAD:.gitmodules is different than the index (the former is
part of the latest commit, whereas the index could have changed via
git-add, that is not committed yet).

> This covers the case when the
> file is part of the repository but for some reason it is not checked
> out, for example because of a sparse checkout.
>
> This makes it possible to use at least the 'git submodule' commands
> which *read* the gitmodules configuration file without fully populating
> the work dir.

Instead of checking for an explicit sparse "hidden" could we just rely on
the file missing? Then I could continue using submodules if I just
"rm .gitmodules".

>
> Writing to .gitmodules wills still require that the file is checked out,
> so check for that in config_gitmodules_set.

That makes sense!

>
> Signed-off-by: Antonio Ospite 
> ---
>
> I am doing the is_gitmodules_hidden() check in the open for now, I am not sure
> whether it is approprate to do that inside stage_updated_gitmodules.

Why do we need that check at all?

In your use case, you want to checkout *a* .gitmodules file, not necessarily
the .gitmodules file of that repo you're currently working on. So it
sort of makes
sense to prevent cross-repo changes (i.e. committing the .gitmodules
accidentally
into the wrong repo)

Stefan

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

Christian Couder  writes:

> 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
> +

Here is a good space to write a comment on what this structure and
its fields are about.  Who are the dealers of helpers anyway?

> +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;
> +}

This is a tangent, but I wonder if we can do better than
hand-rolling these singly-linked list of custom structure types
every time we add new code.  I am just guessing (because it is not
described in the log message) that the expectation is to have only
just a handful of helpers so looking for a helper by name is OK at
O(n), as long as we very infrequently look up a helper by name.

> +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);

If the field is meant to record the name of the promisor remote,
then it is better to name it as such, no?

> +struct odb_helper *find_odb_helper(const char *dealer)

Ditto.

> +{
> + 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;

The code to create a new helper tried to avoid creating a helper
with duplicated name, but nobody tries to create more than one
helpers that point at the same promisor remote.  Yet here we try to
grab the first one that happens to point at the given promisor.

That somehow smells wrong.

Re: [RFC PATCH 04/10] submodule--helper: add a new 'config' subcommand

On Mon, May 14, 2018 at 3:58 AM, Antonio Ospite  wrote:
> Add a new 'config' subcommand to 'submodule--helper', this extra level
> of indirection makes it possible to add some flexibility to how the
> submodules configuration is handled.
>
> Signed-off-by: Antonio Ospite 
> ---
>  builtin/submodule--helper.c | 39 +
>  t/t7411-submodule-config.sh | 26 +
>  2 files changed, 65 insertions(+)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 9e8f2acd5..b32110e3b 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1825,6 +1825,44 @@ static int is_active(int argc, const char **argv, 
> const char *prefix)
> return !is_submodule_active(the_repository, argv[1]);
>  }
>
> +static int config_print_callback(const char *key_, const char *value_, void 
> *cb_data)
> +{
> +   char *key = cb_data;
> +
> +   if (!strcmp(key, key_))
> +   printf("%s\n", value_);
> +
> +   return 0;
> +}
> +
> +static int module_config(int argc, const char **argv, const char *prefix)
> +{
> +   int ret;
> +
> +   if (argc < 2 || argc > 3)
> +   die("submodule--helper config takes 1 or 2 arguments: name 
> [value]");
> +
> +   /* Equivalent to ACTION_GET in builtin/config.c */
> +   if (argc == 2) {
> +   char *key;
> +
> +   ret = git_config_parse_key(argv[1], , NULL);
> +   if (ret < 0)
> +   return CONFIG_INVALID_KEY;
> +
> +   config_from_gitmodules(config_print_callback, the_repository, 
> key);
> +
> +   free(key);
> +   return 0;
> +   }
> +
> +   /* Equivalent to ACTION_SET in builtin/config.c */
> +   if (argc == 3)
> +   return config_gitmodules_set(argv[1], argv[2]);

Ah, here we definitely want to set it in the .gitmodules file?
(Or does that change later in this series?)

> +
> +   return 0;
> +}
> +
>  #define SUPPORT_SUPER_PREFIX (1<<0)
>
>  struct cmd_struct {
> @@ -1850,6 +1888,7 @@ static struct cmd_struct commands[] = {
> {"push-check", push_check, 0},
> {"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
> {"is-active", is_active, 0},
> +   {"config", module_config, 0},
>  };
>
>  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> index a648de6a9..dfe019f05 100755
> --- a/t/t7411-submodule-config.sh
> +++ b/t/t7411-submodule-config.sh
> @@ -139,4 +139,30 @@ test_expect_success 'error in history in 
> fetchrecursesubmodule lets continue' '
> )
>  '
>
> +test_expect_success 'reading submodules config with "submodule--helper 
> config"' '
> +   (cd super &&

I think the project prefers a style
of the cd at the same level of the echo and the following commands.

However we might not need the (cd super && ...) via

  echo "../submodule"  >expected
  git -C super ubmodule--helper config submodule.submodule.url >../actual
  test_cmp expected actual

Our friends developing git on Windows will thank us for saving
to spawn a shell as spawning processes is expensive on Windows. :)
Also we have fewer lines of code.

The patch looks good to me,
Thanks,
Stefan

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

Christian Couder  writes:

> 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(-)

"might", but nobody pays attention to the return value, as the
local availablity of the object at the end is the only thing that
matters, so at this step, it is not all that impactful a change.

Let's see how it plays out.

>   /*
> -  * 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;

Re: [PATCH 2/2] merge-recursive: i18n submodule merge output and respect verbosity

2018-05-14 Thread Elijah Newren

I know I said the patches looked okay earlier, but I just noticed something...

On Thu, May 10, 2018 at 2:19 PM, Stefan Beller  wrote:

> case 1:
> -   MERGE_WARNING(path, "not fast-forward");
> -   fprintf(stderr, "Found a possible merge resolution "
> -   "for the submodule:\n");
> +   output(o, 1, _("Failed to merge submodule %s (not 
> fast-forward)"), path);

We allow folks to set GIT_MERGE_VERBOSITY to change how much output
they get.  A setting of 1 should only show conflicts or major
warnings.  2 is the default and adds a few more messages (e.g.
"Auto-merging $PATH", "Adding $PATH" for one-sided adds, etc.), higher
levels show even more.

Anyway this output message is correct to use level 1 since this is a
conflict, but...

> +   output(o, 1, _("Found a possible merge resolution for the 
> submodule:\n"));

I think this should use level 2.

> print_commit((struct commit *) merges.objects[0].item);
> -   fprintf(stderr,
> +   output(o, 1, _(
>"If this is correct simply add it to the index "
>"for example\n"
>"by using:\n\n"
>"  git update-index --cacheinfo 16 %s \"%s\"\n\n"
>-   "which will accept this suggestion.\n",
>+   "which will accept this suggestion.\n"),
>oid_to_hex([0].item->oid), path);

and so should this one (in fact, I'm tempted to say these last two
should use level 3, but since it looks like a command users may have
difficulty finding on their own, I'm okay with going with 2).

Re: [RFC PATCH 03/10] t7411: be nicer to other tests and really clean things up

On Mon, May 14, 2018 at 3:58 AM, Antonio Ospite  wrote:
> Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with
> invalid lines in .gitmodules but then only the second commit is removed.
>
> This may affect subsequent tests if they assume that the .gitmodules
> file has no errors.
>
> Since those commits are not needed anymore remove both of them.
>
> Signed-off-by: Antonio Ospite 
> ---
>
> I am putting these fixups to the test-suite before the patch that actually
> needs them so that the test-suite passes after each commit.
>
>  t/t7411-submodule-config.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> index 0bde5850a..a648de6a9 100755
> --- a/t/t7411-submodule-config.sh
> +++ b/t/t7411-submodule-config.sh
> @@ -135,7 +135,7 @@ test_expect_success 'error in history in 
> fetchrecursesubmodule lets continue' '
> HEAD submodule \
> >actual &&
> test_cmp expect_error actual  &&
> -   git reset --hard HEAD^
> +   git reset --hard HEAD~2
> )
>  '

As this is the last test in this file, we do not change any subsequent
tests in a subtle way.
Good!

This is
Reviewed-by: Stefan Beller 

FYI:
This test -- of course -- doesn't quite follow the latest coding guidelines,
as usually we'd prefer a test_when_finished ""
at the beginning of a test.

Re: [RFC PATCH 02/10] submodule: factor out a config_gitmodules_set function

On Mon, May 14, 2018 at 3:58 AM, Antonio Ospite  wrote:
> Introduce a new config_gitmodules_set function to write config values to the
> .gitmodules file.
>
> This is in preparation for a future change which will use the function
> to write to the .gitmodules file in a more controlled way instead of
> using "git config -f .gitmodules".
>
> Signed-off-by: Antonio Ospite 
> ---
>
> Not sure about the name, and maybe it can go in config.c for symmetry with
> config_from_gitmodules?

What is the function about (in the end state and now) ?
is it more of a
* configure_submodules_config()
  which would convey it is a generic function to configure submodules
  (i.e. it may not even write to *the* .gitmodules file but somewhere else,
  such as a helper ref)
  This doesn't sound like it as we make use of the function in
  update_path_in_gitmodules() that is used from git-mv, which would want
  to ensure that the specific .gitmodules file is changed.
* gitmodules_file_set()
  that focuses on the specific file that we want to modify?
* ...

Let's continue reading the series to see the end state for
a good name.

Stefan

Re: [PATCH 1/1] Inform about fast-forwarding of submodules during merge

2018-05-14 Thread Elijah Newren

Hi Leif,

On Mon, May 14, 2018 at 1:57 PM, Leif Middelschulte
 wrote:

Thanks for updating the patch on top of Stefan's series.  :-)

> /* Case #1: a is contained in b or vice versa */
> if (in_merge_bases(commit_a, commit_b)) {
> oidcpy(result, b);
> +   output(o, 1, _("Note: Fast-forwarding submodule %s to the 
> following commit"), path);
> +   output_commit_title(o, commit_b);

Level 1 is for conflicts; I don't think this message should have
higher priority than "Auto-merging $PATH" for normal files, so it
needs to be 2 (or maybe 3, see below) rather than 1.  (The default
output level is 2, so it'd still be shown, but we do allow people to
remove informational message and just get conflicts by setting
GIT_MERGE_VERBOSITY to 1, or request extra information by setting it
higher)

Also, this two-line message seems somewhat verbose compared to the
other messages in merge_submdoule(), and when compared to the simple
"Auto-merging $PATH" we do for normal files.  The multi-line nature of
it particularly strikes me; the merge-recursive code has generally
avoided multi-line messages even for conflicts.

In comparison, your original patch just had ("Fast-forwarding
submodule %s", path).

Maybe you could "if (show(o, 3)) { output your current message } else
{ output the simpler message }" ?  Or is this verbosity warranted for
submodules at the default print level?

I'm not a heavy user of submodules, so I may need to get others to
weigh in on the verbosity and multi-line aspects, but I wanted to at
least flag this as somewhat surprising to me.

Elijah

Re: [RFC PATCH 00/10] Make submodules work if .gitmodules is not checked out

Hi Antonio,

thanks for sending this series! Happy to review it!

>   - my git terminology may still be a little off: do "work tree" and
> "work directory" mean the same thing?

Back in the old days, you had a "worktree" which is a directory where
things are checked out and you modify files. It sort of the opposite of the
"git directory", which git uses to store all its information.

Then quite some time later, the command git-worktree was invented.
Now we had 2 things with the same name, which is unfortunate.
But as the command git-worktree added more of these directories
that you could work in, the name collision was not apparent.

Later when people noticed the subtle difference between the
command and the thing on the file system, consensus seemed to be
that the thing on the file system should rather be called "working tree"
such that it sounds very similar but is distinguishable from the command.

However the outcome of the discussion did not yield a bi
refactoring of the code base, such that there are still places with "worktree"
referring to the thing on the FS, "working trees".

I am not aware that "working directory" is an official term we use in
any documentation for Git, but it sounds like you mean a "working tree".
(From a point of view not based on the version control, "working directory"
may sound more correct, note however as the directories in Git are
named trees, the working "tree" sounds as if you can make changes to
Git trees, ... which you can. :) )

> If anyone wanted to pick up and finish the work feel free to do so,
> otherwise please comment and I'll try to address issues as time permits.

First let's review this series. :)

Thanks,
Stefan

Re: [RFC PATCH 01/10] config: make config_from_gitmodules generally useful

On Mon, May 14, 2018 at 3:58 AM, Antonio Ospite  wrote:
> The config_from_gitmodules() function is a good candidate for
> a centralized point where to read the gitmodules configuration file.

It is very tempting to use that function. However it was introduced
specifically to not do that. ;)

See the series that was merged at 5aa0b6c506c (Merge branch
'bw/grep-recurse-submodules', 2017-08-22), specifically
f20e7c1ea24 (submodule: remove submodule.fetchjobs from
submodule-config parsing, 2017-08-02), where both
builtin/fetch as well as the submodule helper use the pattern to
read from the .gitmodules file va this function and then overlay it
with the read from config.

> Add a repo argument to it to make the function more general, and adjust
> the current callers in cmd_fetch and update-clone.

This could be a preparatory patch, but including it here is fine, too.

> As a proof of the utility of the change, start using the function also
> in repo_read_gitmodules which was basically doing the same operations.

I think they were separated for the reason outlined above, or what Brandon
said in his reply.

I think extending 'repo_read_gitmodules' makes sense, as that is
used everywhere for the loading of submodule configuration.

[PATCH] grep: handle corrupt index files early

Any other caller of 'repo_read_index' dies upon a negative return of
it, so grep should, too.

Signed-off-by: Stefan Beller 
---

Found while reviewing the series
https://public-inbox.org/git/20180514105823.8378-1-...@ao2.it/

 builtin/grep.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 6e7bc76785a..69f0743619f 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct 
repository *repo,
strbuf_addstr(, repo->submodule_prefix);
}
 
-   repo_read_index(repo);
+   if (repo_read_index(repo) < 0)
+   die("index file corrupt");
 
for (nr = 0; nr < repo->index->cache_nr; nr++) {
const struct cache_entry *ce = repo->index->cache[nr];
-- 
2.17.0.582.gccdcbd54c44.dirty

Re: [PATCH 1/1] Inform about fast-forwarding of submodules during merge

On Mon, May 14, 2018 at 1:57 PM, Leif Middelschulte
 wrote:
> From: Leif Middelschulte 
>
> Inform the user about an automatically fast-forwarded submodule. The silent 
> merge
> behavior was introduced by commit 68d03e4a6e44 ("Implement automatic 
> fast-forward
> merge for submodules", 2010-07-07)).
>
> Signed-off-by: Leif Middelschulte 

Thanks for following up with a patch.
This looks good to me!

Thanks,
Stefan

> ---
>  merge-recursive.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index a4b91d17f..4a03044d1 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1093,10 +1093,14 @@ static int merge_submodule(struct merge_options *o,
> /* Case #1: a is contained in b or vice versa */
> if (in_merge_bases(commit_a, commit_b)) {
> oidcpy(result, b);
> +   output(o, 1, _("Note: Fast-forwarding submodule %s to the 
> following commit"), path);
> +   output_commit_title(o, commit_b);
> return 1;
> }
> if (in_merge_bases(commit_b, commit_a)) {
> oidcpy(result, a);
> +   output(o, 1, _("Note: Fast-forwarding submodule %s to the 
> following commit:"), path);
> +   output_commit_title(o, commit_a);
> return 1;
> }
>
> --
> 2.15.1 (Apple Git-101)
>

Re: [PATCH 00/35] refactoring refspecs

>  22 files changed, 514 insertions(+), 571 deletions(-)
>  create mode 100644 refspec.c
>  create mode 100644 refspec.h

This looks promising. I'll hope to find time to review it.

Stefan

Re: could `git merge --no-ff origin/master` be made more useful?

2018-05-14 Thread Ævar Arnfjörð Bjarmason


On Mon, May 14 2018, demerphq wrote:

> The first time I tried to use --no-ff I tried to do something like this:
>
>   git checkout master
>   git commit -a -m'whatever'
>   git commit -a -m'whatever2'
>   git merge --no-ff origin/master
>
> and was disappointed when "it didn't work" and git told me there was
> nothing to do as the branch was up to date. (Which I found a bit
> confusing.)
>
> I realize now my expectations were incorrect, and that the argument to
> merge needs to resolve to a commit that is ahead of the current
> commit, and in the above sequence it is the other way around. So to do
> what I want I can do:
>
>   git checkout master
>   git checkout -b topic
>   git commit -a -m'whatever'
>   git commit -a -m'whatever2'
>   git checkout master
>   git merge --no-ff topic
>
> and iiuir this works because 'master' would be behind 'topic' in this case.
>
> But I have a few questions, 1) is there is an argument to feed to git
> merge to make the first recipe work like the second? And 2) is this
> asymmetry necessary with --no-ff?

I've been bitten my this myself, but found that it's documented as the
very first thing in git-merge:

Incorporates changes from the named commits (since the time their
histories diverged from the current branch) into the current
branch[...].

Since origin/master hasn't diverged from your current branch (unlike the
other way around), the merge with --no-ff is a noop.

> More specifically would something horrible break if --no-ff
> origin/trunk detected that the current branch was ahead of the named
> branch and "swapped"  the implicit order of the two so that the first
> recipe could behave like the second

If it worked like that then the user who sets merge.ff=false in his
config and issues a "git pull" after making a commit on his local master
would create a merge commit.

This old E-Mail of Junio's discusses that edge case & others in detail:
https://public-inbox.org/git/7vty1zfwmd@alter.siamese.dyndns.org/

> Anyway, even if the above makes no sense, would it be hard to make the
> message provided by git merge in the first recipe a bit more
> suggestive of what is going on? For instance if it had said "Cannot
> --no-ff merge, origin/master is behind master" it would have been much
> more clear what was going on.

I can't spot any reason for why we couldn't have something like this POC
(would be properly done through advice.c):

diff --git a/builtin/merge.c b/builtin/merge.c
index 9db5a2cf16..920f67d9f8 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1407,6 +1407,8 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
 * but first the most common case of merging one remote.
 */
finish_up_to_date(_("Already up to date."));
+   if (fast_forward == FF_NO)
+   fprintf(stderr, "did you mean this the other way 
around?\n");
goto done;
} else if (fast_forward != FF_NO && !remoteheads->next &&
!common->next &&

But that should probably be reworked to be smart about whether --no-ff
or merge.ff=false was specified, i.e. do we want to yell this at the
user who's just set that at his config default, or the user who's
specified --no-ff explicitly, or both? I don't know.

could `git merge --no-ff origin/master` be made more useful?

2018-05-14 Thread demerphq

The first time I tried to use --no-ff I tried to do something like this:

  git checkout master
  git commit -a -m'whatever'
  git commit -a -m'whatever2'
  git merge --no-ff origin/master

and was disappointed when "it didn't work" and git told me there was
nothing to do as the branch was up to date. (Which I found a bit
confusing.)

I realize now my expectations were incorrect, and that the argument to
merge needs to resolve to a commit that is ahead of the current
commit, and in the above sequence it is the other way around. So to do
what I want I can do:

  git checkout master
  git checkout -b topic
  git commit -a -m'whatever'
  git commit -a -m'whatever2'
  git checkout master
  git merge --no-ff topic

and iiuir this works because 'master' would be behind 'topic' in this case.

But I have a few questions, 1) is there is an argument to feed to git
merge to make the first recipe work like the second? And 2) is this
asymmetry necessary with --no-ff?

More specifically would something horrible break if --no-ff
origin/trunk detected that the current branch was ahead of the named
branch and "swapped"  the implicit order of the two so that the first
recipe could behave like the second?

Anyway, even if the above makes no sense, would it be hard to make the
message provided by git merge in the first recipe a bit more
suggestive of what is going on? For instance if it had said "Cannot
--no-ff merge, origin/master is behind master" it would have been much
more clear what was going on.

Yves










-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

[PATCH 13/35] remote: convert push refspecs to struct refspec

Convert the set of push refspecs stored in 'struct remote' to use
'struct refspec'.

Signed-off-by: Brandon Williams 
---
 builtin/push.c   | 10 +-
 builtin/remote.c | 14 +++---
 remote.c | 23 +--
 remote.h |  6 ++
 4 files changed, 23 insertions(+), 30 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 00d81fb1d..509dc6677 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -79,11 +79,11 @@ static const char *map_refspec(const char *ref,
if (count_refspec_match(ref, local_refs, ) != 1)
return ref;
 
-   if (remote->push) {
+   if (remote->push.nr) {
struct refspec_item query;
memset(, 0, sizeof(struct refspec_item));
query.src = matched->name;
-   if (!query_refspecs(remote->push, remote->push_refspec_nr, 
) &&
+   if (!query_refspecs(remote->push.items, remote->push.nr, 
) &&
query.dst) {
struct strbuf buf = STRBUF_INIT;
strbuf_addf(, "%s%s:%s",
@@ -436,9 +436,9 @@ static int do_push(const char *repo, int flags,
}
 
if (!refspec && !(flags & TRANSPORT_PUSH_ALL)) {
-   if (remote->push_refspec_nr) {
-   refspec = remote->push_refspec;
-   refspec_nr = remote->push_refspec_nr;
+   if (remote->push.raw_nr) {
+   refspec = remote->push.raw;
+   refspec_nr = remote->push.raw_nr;
} else if (!(flags & TRANSPORT_PUSH_MIRROR))
setup_default_push_refspecs(remote);
}
diff --git a/builtin/remote.c b/builtin/remote.c
index d9da82dc8..fb84729d6 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -388,8 +388,8 @@ static int get_push_ref_states(const struct ref 
*remote_refs,
local_refs = get_local_heads();
push_map = copy_ref_list(remote_refs);
 
-   match_push_refs(local_refs, _map, remote->push_refspec_nr,
-   remote->push_refspec, MATCH_REFS_NONE);
+   match_push_refs(local_refs, _map, remote->push.raw_nr,
+   remote->push.raw, MATCH_REFS_NONE);
 
states->push.strdup_strings = 1;
for (ref = push_map; ref; ref = ref->next) {
@@ -435,14 +435,14 @@ static int get_push_ref_states_noquery(struct ref_states 
*states)
return 0;
 
states->push.strdup_strings = 1;
-   if (!remote->push_refspec_nr) {
+   if (!remote->push.nr) {
item = string_list_append(>push, _("(matching)"));
info = item->util = xcalloc(1, sizeof(struct push_info));
info->status = PUSH_STATUS_NOTQUERIED;
info->dest = xstrdup(item->string);
}
-   for (i = 0; i < remote->push_refspec_nr; i++) {
-   struct refspec_item *spec = remote->push + i;
+   for (i = 0; i < remote->push.nr; i++) {
+   const struct refspec_item *spec = >push.items[i];
if (spec->matching)
item = string_list_append(>push, 
_("(matching)"));
else if (strlen(spec->src))
@@ -586,8 +586,8 @@ static int migrate_file(struct remote *remote)
git_config_set_multivar(buf.buf, remote->url[i], "^$", 0);
strbuf_reset();
strbuf_addf(, "remote.%s.push", remote->name);
-   for (i = 0; i < remote->push_refspec_nr; i++)
-   git_config_set_multivar(buf.buf, remote->push_refspec[i], "^$", 
0);
+   for (i = 0; i < remote->push.raw_nr; i++)
+   git_config_set_multivar(buf.buf, remote->push.raw[i], "^$", 0);
strbuf_reset();
strbuf_addf(, "remote.%s.fetch", remote->name);
for (i = 0; i < remote->fetch_refspec_nr; i++)
diff --git a/remote.c b/remote.c
index bce6e7ce4..090110c37 100644
--- a/remote.c
+++ b/remote.c
@@ -79,10 +79,7 @@ static const char *alias_url(const char *url, struct 
rewrites *r)
 
 static void add_push_refspec(struct remote *remote, const char *ref)
 {
-   ALLOC_GROW(remote->push_refspec,
-  remote->push_refspec_nr + 1,
-  remote->push_refspec_alloc);
-   remote->push_refspec[remote->push_refspec_nr++] = ref;
+   refspec_append(>push, ref);
 }
 
 static void add_fetch_refspec(struct remote *remote, const char *ref)
@@ -175,9 +172,11 @@ static struct remote *make_remote(const char *name, int 
len)
ret = xcalloc(1, sizeof(struct remote));
ret->prune = -1;  /* unspecified */
ret->prune_tags = -1;  /* unspecified */
+   ret->name = xstrndup(name, len);
+   refspec_init(>push, REFSPEC_PUSH);
+
ALLOC_GROW(remotes, remotes_nr + 1, remotes_alloc);
remotes[remotes_nr++] = ret;
-   ret->name = xstrndup(name, len);
 
hashmap_entry_init(ret, lookup_entry.hash);
replaced = hashmap_put(_hash, ret);
@@ -542,9 +541,9 @@

[PATCH 10/35] remote: convert match_push_refs to use struct refspec

Convert 'match_push_refs()' to use struct refspec.

Signed-off-by: Brandon Williams 
---
 remote.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/remote.c b/remote.c
index 191855118..bce6e7ce4 100644
--- a/remote.c
+++ b/remote.c
@@ -1312,7 +1312,7 @@ int check_push_refs(struct ref *src, int nr_refspec, 
const char **refspec_names)
 int match_push_refs(struct ref *src, struct ref **dst,
int nr_refspec, const char **refspec, int flags)
 {
-   struct refspec_item *rs;
+   struct refspec rs = REFSPEC_INIT_PUSH;
int send_all = flags & MATCH_REFS_ALL;
int send_mirror = flags & MATCH_REFS_MIRROR;
int send_prune = flags & MATCH_REFS_PRUNE;
@@ -1325,8 +1325,8 @@ int match_push_refs(struct ref *src, struct ref **dst,
nr_refspec = 1;
refspec = default_refspec;
}
-   rs = parse_push_refspec(nr_refspec, (const char **) refspec);
-   errs = match_explicit_refs(src, *dst, _tail, rs, nr_refspec);
+   refspec_appendn(, refspec, nr_refspec);
+   errs = match_explicit_refs(src, *dst, _tail, rs.items, rs.nr);
 
/* pick the remainder */
for (ref = src; ref; ref = ref->next) {
@@ -1335,7 +1335,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
const struct refspec_item *pat = NULL;
char *dst_name;
 
-   dst_name = get_ref_match(rs, nr_refspec, ref, send_mirror, 
FROM_SRC, );
+   dst_name = get_ref_match(rs.items, rs.nr, ref, send_mirror, 
FROM_SRC, );
if (!dst_name)
continue;
 
@@ -1384,7 +1384,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
/* We're already sending something to this ref. 
*/
continue;
 
-   src_name = get_ref_match(rs, nr_refspec, ref, 
send_mirror, FROM_DST, NULL);
+   src_name = get_ref_match(rs.items, rs.nr, ref, 
send_mirror, FROM_DST, NULL);
if (src_name) {
if (!src_ref_index.nr)
prepare_ref_index(_ref_index, src);
@@ -1396,6 +1396,9 @@ int match_push_refs(struct ref *src, struct ref **dst,
}
string_list_clear(_ref_index, 0);
}
+
+   refspec_clear();
+
if (errs)
return -1;
return 0;
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 11/35] clone: convert cmd_clone to use refspec_item_init

Convert 'cmd_clone()' to use 'refspec_item_init()' instead of relying on
the old 'parse_fetch_refspec()' to initialize a single refspec item.

Signed-off-by: Brandon Williams 
---
 builtin/clone.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 854088a3a..8c5f4d8f0 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -895,8 +895,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
int err = 0, complete_refs_before_fetch = 1;
int submodule_progress;
 
-   struct refspec_item *refspec;
-   const char *fetch_pattern;
+   struct refspec_item refspec;
 
fetch_if_missing = 0;
 
@@ -1078,8 +1077,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
if (option_required_reference.nr || option_optional_reference.nr)
setup_reference();
 
-   fetch_pattern = value.buf;
-   refspec = parse_fetch_refspec(1, _pattern);
+   refspec_item_init(, value.buf, REFSPEC_FETCH);
 
strbuf_reset();
 
@@ -1139,7 +1137,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
refs = transport_get_remote_refs(transport, NULL);
 
if (refs) {
-   mapped_refs = wanted_peer_refs(refs, refspec);
+   mapped_refs = wanted_peer_refs(refs, );
/*
 * transport_get_remote_refs() may return refs with null sha-1
 * in mapped_refs (see struct transport->get_refs_list
@@ -1233,6 +1231,6 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
strbuf_release();
junk_mode = JUNK_LEAVE_ALL;
 
-   free(refspec);
+   refspec_item_clear();
return err;
 }
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 20/35] fetch: convert get_ref_map to take a struct refspec

Convert 'get_ref_map()' to take a 'struct refspec' as a parameter
instead of a list of 'struct refspec_item'.

Signed-off-by: Brandon Williams 
---
 builtin/fetch.c | 43 ---
 1 file changed, 20 insertions(+), 23 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 733feb19c..86a7f103f 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -337,7 +337,7 @@ static void find_non_local_tags(struct transport *transport,
 }
 
 static struct ref *get_ref_map(struct transport *transport,
-  struct refspec_item *refspecs, int refspec_count,
+  struct refspec *rs,
   int tags, int *autotags)
 {
int i;
@@ -351,15 +351,16 @@ static struct ref *get_ref_map(struct transport 
*transport,
 
const struct ref *remote_refs;
 
-   for (i = 0; i < refspec_count; i++) {
-   if (!refspecs[i].exact_sha1) {
-   const char *glob = strchr(refspecs[i].src, '*');
+   for (i = 0; i < rs->nr; i++) {
+   const struct refspec_item *item = >items[i];
+   if (!item->exact_sha1) {
+   const char *glob = strchr(item->src, '*');
if (glob)
argv_array_pushf(_prefixes, "%.*s",
-(int)(glob - refspecs[i].src),
-refspecs[i].src);
+(int)(glob - item->src),
+item->src);
else
-   expand_ref_prefix(_prefixes, 
refspecs[i].src);
+   expand_ref_prefix(_prefixes, item->src);
}
}
 
@@ -367,13 +368,12 @@ static struct ref *get_ref_map(struct transport 
*transport,
 
argv_array_clear(_prefixes);
 
-   if (refspec_count) {
-   struct refspec_item *fetch_refspec;
-   int fetch_refspec_nr;
+   if (rs->nr) {
+   struct refspec *fetch_refspec;
 
-   for (i = 0; i < refspec_count; i++) {
-   get_fetch_map(remote_refs, [i], , 0);
-   if (refspecs[i].dst && refspecs[i].dst[0])
+   for (i = 0; i < rs->nr; i++) {
+   get_fetch_map(remote_refs, >items[i], , 0);
+   if (rs->items[i].dst && rs->items[i].dst[0])
*autotags = 1;
}
/* Merge everything on the command line (but not --tags) */
@@ -400,16 +400,13 @@ static struct ref *get_ref_map(struct transport 
*transport,
 * by ref_remove_duplicates() in favor of one of these
 * opportunistic entries with FETCH_HEAD_IGNORE.
 */
-   if (refmap.nr) {
-   fetch_refspec = refmap.items;
-   fetch_refspec_nr = refmap.nr;
-   } else {
-   fetch_refspec = transport->remote->fetch.items;
-   fetch_refspec_nr = transport->remote->fetch.nr;
-   }
+   if (refmap.nr)
+   fetch_refspec = 
+   else
+   fetch_refspec = >remote->fetch;
 
-   for (i = 0; i < fetch_refspec_nr; i++)
-   get_fetch_map(ref_map, _refspec[i], _tail, 
1);
+   for (i = 0; i < fetch_refspec->nr; i++)
+   get_fetch_map(ref_map, _refspec->items[i], 
_tail, 1);
} else if (refmap.nr) {
die("--refmap option is only meaningful with command-line 
refspec(s).");
} else {
@@ -1136,7 +1133,7 @@ static int do_fetch(struct transport *transport,
goto cleanup;
}
 
-   ref_map = get_ref_map(transport, rs->items, rs->nr, tags, );
+   ref_map = get_ref_map(transport, rs, tags, );
if (!update_head_ok)
check_not_current_branch(ref_map);
 
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 16/35] fetch: convert fetch_one to use struct refspec

Convert 'fetch_one()' to use 'struct refspec'.

Signed-off-by: Brandon Williams 
---
 builtin/fetch.c | 46 +++---
 1 file changed, 19 insertions(+), 27 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 30083d4bc..769f9d2be 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1356,10 +1356,8 @@ static inline void fetch_one_setup_partial(struct remote 
*remote)
 
 static int fetch_one(struct remote *remote, int argc, const char **argv, int 
prune_tags_ok)
 {
-   static const char **refs = NULL;
-   struct refspec_item *refspec;
-   int ref_nr = 0;
-   int j = 0;
+   struct refspec rs = REFSPEC_INIT_FETCH;
+   int i;
int exit_code;
int maybe_prune_tags;
int remote_via_config = remote_is_configured(remote, 0);
@@ -1394,35 +1392,29 @@ static int fetch_one(struct remote *remote, int argc, 
const char **argv, int pru
if (maybe_prune_tags && remote_via_config)
add_prune_tags_to_fetch_refspec(remote);
 
-   if (argc > 0 || (maybe_prune_tags && !remote_via_config)) {
-   size_t nr_alloc = st_add3(argc, maybe_prune_tags, 1);
-   refs = xcalloc(nr_alloc, sizeof(const char *));
-   if (maybe_prune_tags) {
-   refs[j++] = xstrdup("refs/tags/*:refs/tags/*");
-   ref_nr++;
-   }
-   }
+   if (maybe_prune_tags && (argc || !remote_via_config))
+   refspec_append(, TAG_REFSPEC);
 
-   if (argc > 0) {
-   int i;
-   for (i = 0; i < argc; i++) {
-   if (!strcmp(argv[i], "tag")) {
-   i++;
-   if (i >= argc)
-   die(_("You need to specify a tag 
name."));
-   refs[j++] = xstrfmt("refs/tags/%s:refs/tags/%s",
-   argv[i], argv[i]);
-   } else
-   refs[j++] = argv[i];
-   ref_nr++;
+   for (i = 0; i < argc; i++) {
+   if (!strcmp(argv[i], "tag")) {
+   char *tag;
+   i++;
+   if (i >= argc)
+   die(_("You need to specify a tag name."));
+
+   tag = xstrfmt("refs/tags/%s:refs/tags/%s",
+ argv[i], argv[i]);
+   refspec_append(, tag);
+   free(tag);
+   } else {
+   refspec_append(, argv[i]);
}
}
 
sigchain_push_common(unlock_pack_on_signal);
atexit(unlock_pack);
-   refspec = parse_fetch_refspec(ref_nr, refs);
-   exit_code = do_fetch(gtransport, refspec, ref_nr);
-   free_refspec(ref_nr, refspec);
+   exit_code = do_fetch(gtransport, rs.items, rs.nr);
+   refspec_clear();
transport_disconnect(gtransport);
gtransport = NULL;
return exit_code;
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 12/35] fast-export: convert to use struct refspec

Convert fast-export to use 'struct refspec' instead of using a list of
refspec_item's.

Signed-off-by: Brandon Williams 
---
 builtin/fast-export.c | 21 +++--
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 6f105dc79..143999738 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -36,8 +36,7 @@ static int use_done_feature;
 static int no_data;
 static int full_tree;
 static struct string_list extra_refs = STRING_LIST_INIT_NODUP;
-static struct refspec_item *refspecs;
-static int refspecs_nr;
+static struct refspec refspecs = REFSPEC_INIT_FETCH;
 static int anonymize;
 
 static int parse_opt_signed_tag_mode(const struct option *opt,
@@ -830,9 +829,9 @@ static void get_tags_and_duplicates(struct rev_cmdline_info 
*info)
if (dwim_ref(e->name, strlen(e->name), , _name) != 1)
continue;
 
-   if (refspecs) {
+   if (refspecs.nr) {
char *private;
-   private = apply_refspecs(refspecs, refspecs_nr, 
full_name);
+   private = apply_refspecs(refspecs.items, refspecs.nr, 
full_name);
if (private) {
free(full_name);
full_name = private;
@@ -978,8 +977,8 @@ static void import_marks(char *input_file)
 static void handle_deletes(void)
 {
int i;
-   for (i = 0; i < refspecs_nr; i++) {
-   struct refspec_item *refspec = [i];
+   for (i = 0; i < refspecs.nr; i++) {
+   struct refspec_item *refspec = [i];
if (*refspec->src)
continue;
 
@@ -1040,18 +1039,12 @@ int cmd_fast_export(int argc, const char **argv, const 
char *prefix)
usage_with_options (fast_export_usage, options);
 
if (refspecs_list.nr) {
-   const char **refspecs_str;
int i;
 
-   ALLOC_ARRAY(refspecs_str, refspecs_list.nr);
for (i = 0; i < refspecs_list.nr; i++)
-   refspecs_str[i] = refspecs_list.items[i].string;
-
-   refspecs_nr = refspecs_list.nr;
-   refspecs = parse_fetch_refspec(refspecs_nr, refspecs_str);
+   refspec_append(, 
refspecs_list.items[i].string);
 
string_list_clear(_list, 1);
-   free(refspecs_str);
}
 
if (use_done_feature)
@@ -1090,7 +1083,7 @@ int cmd_fast_export(int argc, const char **argv, const 
char *prefix)
if (use_done_feature)
printf("done\n");
 
-   free_refspec(refspecs_nr, refspecs);
+   refspec_clear();
 
return 0;
 }
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 30/35] send-pack: store refspecs in a struct refspec

Convert send-pack.c to store refspecs in a 'struct refspec' instead of
as an array of 'const char *'.

Signed-off-by: Brandon Williams 
---
 builtin/send-pack.c | 24 +++-
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index b5427f75e..ef512616f 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -126,8 +126,7 @@ static int send_pack_config(const char *k, const char *v, 
void *cb)
 
 int cmd_send_pack(int argc, const char **argv, const char *prefix)
 {
-   int i, nr_refspecs = 0;
-   const char **refspecs = NULL;
+   struct refspec rs = REFSPEC_INIT_PUSH;
const char *remote_name = NULL;
struct remote *remote = NULL;
const char *dest = NULL;
@@ -189,8 +188,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
argc = parse_options(argc, argv, prefix, options, send_pack_usage, 0);
if (argc > 0) {
dest = argv[0];
-   refspecs = (const char **)(argv + 1);
-   nr_refspecs = argc - 1;
+   refspec_appendn(, argv + 1, argc - 1);
}
 
if (!dest)
@@ -209,31 +207,23 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
args.push_options = push_options.nr ? _options : NULL;
 
if (from_stdin) {
-   struct argv_array all_refspecs = ARGV_ARRAY_INIT;
-
-   for (i = 0; i < nr_refspecs; i++)
-   argv_array_push(_refspecs, refspecs[i]);
-
if (args.stateless_rpc) {
const char *buf;
while ((buf = packet_read_line(0, NULL)))
-   argv_array_push(_refspecs, buf);
+   refspec_append(, buf);
} else {
struct strbuf line = STRBUF_INIT;
while (strbuf_getline(, stdin) != EOF)
-   argv_array_push(_refspecs, line.buf);
+   refspec_append(, line.buf);
strbuf_release();
}
-
-   refspecs = all_refspecs.argv;
-   nr_refspecs = all_refspecs.argc;
}
 
/*
 * --all and --mirror are incompatible; neither makes sense
 * with any refspecs.
 */
-   if ((nr_refspecs > 0 && (send_all || args.send_mirror)) ||
+   if ((rs.nr > 0 && (send_all || args.send_mirror)) ||
(send_all && args.send_mirror))
usage_with_options(send_pack_usage, options);
 
@@ -275,7 +265,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
BUG("unknown protocol version");
}
 
-   transport_verify_remote_names(nr_refspecs, refspecs);
+   transport_verify_remote_names(rs.raw_nr, rs.raw);
 
local_refs = get_local_heads();
 
@@ -287,7 +277,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
flags |= MATCH_REFS_MIRROR;
 
/* match them up */
-   if (match_push_refs(local_refs, _refs, nr_refspecs, refspecs, 
flags))
+   if (match_push_refs(local_refs, _refs, rs.raw_nr, rs.raw, flags))
return -1;
 
if (!is_empty_cas())
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 28/35] push: convert to use struct refspec

Convert the refspecs in builtin/push.c to be stored in a 'struct
refspec' instead of being stored in a list of 'struct refspec_item's.

Signed-off-by: Brandon Williams 
---
 builtin/push.c | 38 +++---
 1 file changed, 15 insertions(+), 23 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index df5df6c0d..ef42979d1 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -57,19 +57,10 @@ static enum transport_family family;
 
 static struct push_cas_option cas;
 
-static const char **refspec;
-static int refspec_nr;
-static int refspec_alloc;
+static struct refspec rs = REFSPEC_INIT_PUSH;
 
 static struct string_list push_options_config = STRING_LIST_INIT_DUP;
 
-static void add_refspec(const char *ref)
-{
-   refspec_nr++;
-   ALLOC_GROW(refspec, refspec_nr, refspec_alloc);
-   refspec[refspec_nr-1] = ref;
-}
-
 static const char *map_refspec(const char *ref,
   struct remote *remote, struct ref *local_refs)
 {
@@ -138,7 +129,7 @@ static void set_refspecs(const char **refs, int nr, const 
char *repo)
}
ref = map_refspec(ref, remote, local_refs);
}
-   add_refspec(ref);
+   refspec_append(, ref);
}
 }
 
@@ -226,7 +217,7 @@ static void setup_push_upstream(struct remote *remote, 
struct branch *branch,
}
 
strbuf_addf(, "%s:%s", branch->refname, branch->merge[0]->src);
-   add_refspec(refspec.buf);
+   refspec_append(, refspec.buf);
 }
 
 static void setup_push_current(struct remote *remote, struct branch *branch)
@@ -236,7 +227,7 @@ static void setup_push_current(struct remote *remote, 
struct branch *branch)
if (!branch)
die(_(message_detached_head_die), remote->name);
strbuf_addf(, "%s:%s", branch->refname, branch->refname);
-   add_refspec(refspec.buf);
+   refspec_append(, refspec.buf);
 }
 
 static int is_workflow_triangular(struct remote *remote)
@@ -253,7 +244,7 @@ static void setup_default_push_refspecs(struct remote 
*remote)
switch (push_default) {
default:
case PUSH_DEFAULT_MATCHING:
-   add_refspec(":");
+   refspec_append(, ":");
break;
 
case PUSH_DEFAULT_UNSPECIFIED:
@@ -341,7 +332,8 @@ static void advise_ref_needs_force(void)
advise(_(message_advice_ref_needs_force));
 }
 
-static int push_with_options(struct transport *transport, int flags)
+static int push_with_options(struct transport *transport, struct refspec *rs,
+int flags)
 {
int err;
unsigned int reject_reasons;
@@ -363,7 +355,7 @@ static int push_with_options(struct transport *transport, 
int flags)
 
if (verbosity > 0)
fprintf(stderr, _("Pushing to %s\n"), transport->url);
-   err = transport_push(transport, refspec_nr, refspec, flags,
+   err = transport_push(transport, rs->raw_nr, rs->raw, flags,
 _reasons);
if (err != 0) {
fprintf(stderr, "%s", push_get_color(PUSH_COLOR_ERROR));
@@ -397,6 +389,7 @@ static int do_push(const char *repo, int flags,
struct remote *remote = pushremote_get(repo);
const char **url;
int url_nr;
+   struct refspec *push_refspec = 
 
if (!remote) {
if (repo)
@@ -417,10 +410,9 @@ static int do_push(const char *repo, int flags,
if (push_options->nr)
flags |= TRANSPORT_PUSH_OPTIONS;
 
-   if (!refspec && !(flags & TRANSPORT_PUSH_ALL)) {
-   if (remote->push.raw_nr) {
-   refspec = remote->push.raw;
-   refspec_nr = remote->push.raw_nr;
+   if (!push_refspec->nr && !(flags & TRANSPORT_PUSH_ALL)) {
+   if (remote->push.nr) {
+   push_refspec = >push;
} else if (!(flags & TRANSPORT_PUSH_MIRROR))
setup_default_push_refspecs(remote);
}
@@ -432,7 +424,7 @@ static int do_push(const char *repo, int flags,
transport_get(remote, url[i]);
if (flags & TRANSPORT_PUSH_OPTIONS)
transport->push_options = push_options;
-   if (push_with_options(transport, flags))
+   if (push_with_options(transport, push_refspec, flags))
errs++;
}
} else {
@@ -440,7 +432,7 @@ static int do_push(const char *repo, int flags,
transport_get(remote, NULL);
if (flags & TRANSPORT_PUSH_OPTIONS)
transport->push_options = push_options;
-   if (push_with_options(transport, flags))
+   if (push_with_options(transport, push_refspec, flags))
errs++;
}
return !!errs;
@@ -631,7 +623,7

[PATCH 29/35] transport: convert transport_push to take a struct refspec

Convert 'transport_push()' to take a 'struct refspec' as a
parameter instead of an array of strings which represent
refspecs.

Signed-off-by: Brandon Williams 
---
 builtin/push.c |  3 +--
 transport.c| 17 +++--
 transport.h|  2 +-
 3 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index ef42979d1..9cd8e8cd5 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -355,8 +355,7 @@ static int push_with_options(struct transport *transport, 
struct refspec *rs,
 
if (verbosity > 0)
fprintf(stderr, _("Pushing to %s\n"), transport->url);
-   err = transport_push(transport, rs->raw_nr, rs->raw, flags,
-_reasons);
+   err = transport_push(transport, rs, flags, _reasons);
if (err != 0) {
fprintf(stderr, "%s", push_get_color(PUSH_COLOR_ERROR));
error(_("failed to push some refs to '%s'"), transport->url);
diff --git a/transport.c b/transport.c
index 181db4d4d..a89f17744 100644
--- a/transport.c
+++ b/transport.c
@@ -1093,11 +1093,11 @@ static int run_pre_push_hook(struct transport 
*transport,
 }
 
 int transport_push(struct transport *transport,
-  int refspec_nr, const char **refspec, int flags,
+  struct refspec *rs, int flags,
   unsigned int *reject_reasons)
 {
*reject_reasons = 0;
-   transport_verify_remote_names(refspec_nr, refspec);
+   transport_verify_remote_names(rs->raw_nr, rs->raw);
 
if (transport_color_config() < 0)
return -1;
@@ -,16 +,14 @@ int transport_push(struct transport *transport,
int porcelain = flags & TRANSPORT_PUSH_PORCELAIN;
int pretend = flags & TRANSPORT_PUSH_DRY_RUN;
int push_ret, ret, err;
-   struct refspec tmp_rs = REFSPEC_INIT_PUSH;
struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
int i;
 
-   if (check_push_refs(local_refs, refspec_nr, refspec) < 0)
+   if (check_push_refs(local_refs, rs->raw_nr, rs->raw) < 0)
return -1;
 
-   refspec_appendn(_rs, refspec, refspec_nr);
-   for (i = 0; i < tmp_rs.nr; i++) {
-   const struct refspec_item *item = _rs.items[i];
+   for (i = 0; i < rs->nr; i++) {
+   const struct refspec_item *item = >items[i];
const char *prefix = NULL;
 
if (item->dst)
@@ -1143,7 +1141,6 @@ int transport_push(struct transport *transport,
   _prefixes);
 
argv_array_clear(_prefixes);
-   refspec_clear(_rs);
 
if (flags & TRANSPORT_PUSH_ALL)
match_flags |= MATCH_REFS_ALL;
@@ -1155,7 +1152,7 @@ int transport_push(struct transport *transport,
match_flags |= MATCH_REFS_FOLLOW_TAGS;
 
if (match_push_refs(local_refs, _refs,
-   refspec_nr, refspec, match_flags)) {
+   rs->raw_nr, rs->raw, match_flags)) {
return -1;
}
 
@@ -1186,7 +1183,7 @@ int transport_push(struct transport *transport,
 
if (!push_unpushed_submodules(,
  transport->remote,
- refspec, refspec_nr,
+ rs->raw, rs->raw_nr,
  transport->push_options,
  pretend)) {
oid_array_clear();
diff --git a/transport.h b/transport.h
index e783cfa07..e2c809af4 100644
--- a/transport.h
+++ b/transport.h
@@ -197,7 +197,7 @@ void transport_set_verbosity(struct transport *transport, 
int verbosity,
 #define REJECT_NEEDS_FORCE 0x10
 
 int transport_push(struct transport *connection,
-  int refspec_nr, const char **refspec, int flags,
+  struct refspec *rs, int flags,
   unsigned int * reject_reasons);
 
 /*
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 23/35] remote: convert apply_refspecs to take a struct refspec

Convert 'apply_refspecs()' to take a 'struct refspec' as a parameter instead
of a list of 'struct refspec_item'.

Signed-off-by: Brandon Williams 
---
 builtin/fast-export.c |  2 +-
 remote.c  | 15 ++-
 remote.h  |  3 +--
 transport-helper.c|  6 +++---
 4 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 143999738..41fe49e4d 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -831,7 +831,7 @@ static void get_tags_and_duplicates(struct rev_cmdline_info 
*info)
 
if (refspecs.nr) {
char *private;
-   private = apply_refspecs(refspecs.items, refspecs.nr, 
full_name);
+   private = apply_refspecs(, full_name);
if (private) {
free(full_name);
full_name = private;
diff --git a/remote.c b/remote.c
index f2e97c545..54297de3d 100644
--- a/remote.c
+++ b/remote.c
@@ -534,8 +534,7 @@ const char *remote_ref_for_branch(struct branch *branch, 
int for_push,
struct remote *remote = remote_get(remote_name);
 
if (remote && remote->push.nr &&
-   (dst = apply_refspecs(remote->push.items,
- remote->push.nr,
+   (dst = apply_refspecs(>push,
  branch->refname))) {
if (explicit)
*explicit = 1;
@@ -766,15 +765,14 @@ int query_refspecs(struct refspec_item *refs, int 
ref_count, struct refspec_item
return -1;
 }
 
-char *apply_refspecs(struct refspec_item *refspecs, int nr_refspec,
-const char *name)
+char *apply_refspecs(struct refspec *rs, const char *name)
 {
struct refspec_item query;
 
memset(, 0, sizeof(struct refspec_item));
query.src = (char *)name;
 
-   if (query_refspecs(refspecs, nr_refspec, ))
+   if (query_refspecs(rs->items, rs->nr, ))
return NULL;
 
return query.dst;
@@ -1580,7 +1578,7 @@ static const char *tracking_for_push_dest(struct remote 
*remote,
 {
char *ret;
 
-   ret = apply_refspecs(remote->fetch.items, remote->fetch.nr, refname);
+   ret = apply_refspecs(>fetch, refname);
if (!ret)
return error_buf(err,
 _("push destination '%s' on remote '%s' has no 
local tracking branch"),
@@ -1602,8 +1600,7 @@ static const char *branch_get_push_1(struct branch 
*branch, struct strbuf *err)
char *dst;
const char *ret;
 
-   dst = apply_refspecs(remote->push.items, remote->push.nr,
-branch->refname);
+   dst = apply_refspecs(>push, branch->refname);
if (!dst)
return error_buf(err,
 _("push refspecs for '%s' do not 
include '%s'"),
@@ -2212,7 +2209,7 @@ static int remote_tracking(struct remote *remote, const 
char *refname,
 {
char *dst;
 
-   dst = apply_refspecs(remote->fetch.items, remote->fetch.nr, refname);
+   dst = apply_refspecs(>fetch, refname);
if (!dst)
return -1; /* no tracking ref for refname at remote */
if (read_ref(dst, oid))
diff --git a/remote.h b/remote.h
index 5ac7536f5..4e7590b55 100644
--- a/remote.h
+++ b/remote.h
@@ -159,8 +159,7 @@ int ref_newer(const struct object_id *new_oid, const struct 
object_id *old_oid);
 struct ref *ref_remove_duplicates(struct ref *ref_map);
 
 extern int query_refspecs(struct refspec_item *specs, int nr, struct 
refspec_item *query);
-char *apply_refspecs(struct refspec_item *refspecs, int nr_refspec,
-const char *name);
+char *apply_refspecs(struct refspec *rs, const char *name);
 
 int check_push_refs(struct ref *src, int nr_refspec, const char **refspec);
 int match_push_refs(struct ref *src, struct ref **dst,
diff --git a/transport-helper.c b/transport-helper.c
index 33f51ebfc..1f8ff7e94 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -523,7 +523,7 @@ static int fetch_with_import(struct transport *transport,
continue;
name = posn->symref ? posn->symref : posn->name;
if (data->rs.nr)
-   private = apply_refspecs(data->rs.items, data->rs.nr, 
name);
+   private = apply_refspecs(>rs, name);
else
private = xstrdup(name);
if (private) {
@@ -805,7 +805,7 @@ static int push_update_refs_status(struct helper_data *data,
continue;
 
/* propagate back the update to the remote namespace */
-   private =

[PATCH 21/35] fetch: convert prune_refs to take a struct refspec

Convert 'prune_refs()' to take a 'struct refspec' as a parameter instead
of a list of 'struct refspec_item'.

Signed-off-by: Brandon Williams 
---
 builtin/fetch.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 86a7f103f..eebeebfd8 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -959,11 +959,11 @@ static int fetch_refs(struct transport *transport, struct 
ref *ref_map)
return ret;
 }
 
-static int prune_refs(struct refspec_item *refs, int ref_count, struct ref 
*ref_map,
-   const char *raw_url)
+static int prune_refs(struct refspec *rs, struct ref *ref_map,
+ const char *raw_url)
 {
int url_len, i, result = 0;
-   struct ref *ref, *stale_refs = get_stale_heads(refs, ref_count, 
ref_map);
+   struct ref *ref, *stale_refs = get_stale_heads(rs->items, rs->nr, 
ref_map);
char *url;
int summary_width = transport_summary_width(stale_refs);
const char *dangling_msg = dry_run
@@ -1158,10 +1158,9 @@ static int do_fetch(struct transport *transport,
 * don't care whether --tags was specified.
 */
if (rs->nr) {
-   prune_refs(rs->items, rs->nr, ref_map, transport->url);
+   prune_refs(rs, ref_map, transport->url);
} else {
-   prune_refs(transport->remote->fetch.items,
-  transport->remote->fetch.nr,
+   prune_refs(>remote->fetch,
   ref_map,
   transport->url);
}
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 34/35] remote: convert check_push_refs to take a struct refspec

Convert 'check_push_refs()' to take a 'struct refspec' as a parameter
instead of an array of 'const char *'.

Signed-off-by: Brandon Williams 
---
 remote.c| 14 +-
 remote.h|  2 +-
 transport.c |  2 +-
 3 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/remote.c b/remote.c
index 0e882d812..8e6522f4d 100644
--- a/remote.c
+++ b/remote.c
@@ -1264,24 +1264,20 @@ static void prepare_ref_index(struct string_list 
*ref_index, struct ref *ref)
  * but we can catch some errors early before even talking to the
  * remote side.
  */
-int check_push_refs(struct ref *src, int nr_refspec, const char 
**refspec_names)
+int check_push_refs(struct ref *src, struct refspec *rs)
 {
-   struct refspec refspec = REFSPEC_INIT_PUSH;
int ret = 0;
int i;
 
-   refspec_appendn(, refspec_names, nr_refspec);
-
-   for (i = 0; i < refspec.nr; i++) {
-   struct refspec_item *rs = [i];
+   for (i = 0; i < rs->nr; i++) {
+   struct refspec_item *item = >items[i];
 
-   if (rs->pattern || rs->matching)
+   if (item->pattern || item->matching)
continue;
 
-   ret |= match_explicit_lhs(src, rs, NULL, NULL);
+   ret |= match_explicit_lhs(src, item, NULL, NULL);
}
 
-   refspec_clear();
return ret;
 }
 
diff --git a/remote.h b/remote.h
index d5b5f24ac..9014f707f 100644
--- a/remote.h
+++ b/remote.h
@@ -161,7 +161,7 @@ struct ref *ref_remove_duplicates(struct ref *ref_map);
 int query_refspecs(struct refspec *rs, struct refspec_item *query);
 char *apply_refspecs(struct refspec *rs, const char *name);
 
-int check_push_refs(struct ref *src, int nr_refspec, const char **refspec);
+int check_push_refs(struct ref *src, struct refspec *rs);
 int match_push_refs(struct ref *src, struct ref **dst,
struct refspec *rs, int flags);
 void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
diff --git a/transport.c b/transport.c
index 24a97d9e8..e32bc320c 100644
--- a/transport.c
+++ b/transport.c
@@ -1090,7 +1090,7 @@ int transport_push(struct transport *transport,
struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
int i;
 
-   if (check_push_refs(local_refs, rs->raw_nr, rs->raw) < 0)
+   if (check_push_refs(local_refs, rs) < 0)
return -1;
 
for (i = 0; i < rs->nr; i++) {
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 35/35] submodule: convert push_unpushed_submodules to take a struct refspec

Convert 'push_unpushed_submodules()' to take a 'struct refspec' as a
parameter instead of an array of 'const char *'.

Signed-off-by: Brandon Williams 
---
 submodule.c | 19 +--
 submodule.h |  3 ++-
 transport.c |  2 +-
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/submodule.c b/submodule.c
index 74d35b257..cdeadd80e 100644
--- a/submodule.c
+++ b/submodule.c
@@ -968,7 +968,7 @@ int find_unpushed_submodules(struct oid_array *commits,
 
 static int push_submodule(const char *path,
  const struct remote *remote,
- const char **refspec, int refspec_nr,
+ const struct refspec *rs,
  const struct string_list *push_options,
  int dry_run)
 {
@@ -991,8 +991,8 @@ static int push_submodule(const char *path,
if (remote->origin != REMOTE_UNCONFIGURED) {
int i;
argv_array_push(, remote->name);
-   for (i = 0; i < refspec_nr; i++)
-   argv_array_push(, refspec[i]);
+   for (i = 0; i < rs->raw_nr; i++)
+   argv_array_push(, rs->raw[i]);
}
 
prepare_submodule_repo_env(_array);
@@ -1013,7 +1013,7 @@ static int push_submodule(const char *path,
  */
 static void submodule_push_check(const char *path, const char *head,
 const struct remote *remote,
-const char **refspec, int refspec_nr)
+const struct refspec *rs)
 {
struct child_process cp = CHILD_PROCESS_INIT;
int i;
@@ -1023,8 +1023,8 @@ static void submodule_push_check(const char *path, const 
char *head,
argv_array_push(, head);
argv_array_push(, remote->name);
 
-   for (i = 0; i < refspec_nr; i++)
-   argv_array_push(, refspec[i]);
+   for (i = 0; i < rs->raw_nr; i++)
+   argv_array_push(, rs->raw[i]);
 
prepare_submodule_repo_env(_array);
cp.git_cmd = 1;
@@ -1043,7 +1043,7 @@ static void submodule_push_check(const char *path, const 
char *head,
 
 int push_unpushed_submodules(struct oid_array *commits,
 const struct remote *remote,
-const char **refspec, int refspec_nr,
+const struct refspec *rs,
 const struct string_list *push_options,
 int dry_run)
 {
@@ -1069,8 +1069,7 @@ int push_unpushed_submodules(struct oid_array *commits,
 
for (i = 0; i < needs_pushing.nr; i++)
submodule_push_check(needs_pushing.items[i].string,
-head, remote,
-refspec, refspec_nr);
+head, remote, rs);
free(head);
}
 
@@ -1078,7 +1077,7 @@ int push_unpushed_submodules(struct oid_array *commits,
for (i = 0; i < needs_pushing.nr; i++) {
const char *path = needs_pushing.items[i].string;
fprintf(stderr, "Pushing submodule '%s'\n", path);
-   if (!push_submodule(path, remote, refspec, refspec_nr,
+   if (!push_submodule(path, remote, rs,
push_options, dry_run)) {
fprintf(stderr, "Unable to push submodule '%s'\n", 
path);
ret = 0;
diff --git a/submodule.h b/submodule.h
index e5526f6aa..aae0c9c8f 100644
--- a/submodule.h
+++ b/submodule.h
@@ -100,9 +100,10 @@ extern int submodule_touches_in_range(struct object_id *a,
 extern int find_unpushed_submodules(struct oid_array *commits,
const char *remotes_name,
struct string_list *needs_pushing);
+struct refspec;
 extern int push_unpushed_submodules(struct oid_array *commits,
const struct remote *remote,
-   const char **refspec, int refspec_nr,
+   const struct refspec *rs,
const struct string_list *push_options,
int dry_run);
 /*
diff --git a/transport.c b/transport.c
index e32bc320c..7e0b9abba 100644
--- a/transport.c
+++ b/transport.c
@@ -1157,7 +1157,7 @@ int transport_push(struct transport *transport,
 
if (!push_unpushed_submodules(,
  transport->remote,
- rs->raw, rs->raw_nr,
+ rs,
  transport->push_options,
  pretend))

[PATCH 31/35] transport: remove transport_verify_remote_names

Remove 'transprot_verify_remote_names()' because all callers have
migrated to using 'struct refspec' which performs the same checks in
'parse_refspec()'.

Signed-off-by: Brandon Williams 
---
 builtin/send-pack.c |  2 --
 transport.c | 24 
 transport.h |  2 --
 3 files changed, 28 deletions(-)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index ef512616f..7c34bf467 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -265,8 +265,6 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
BUG("unknown protocol version");
}
 
-   transport_verify_remote_names(rs.raw_nr, rs.raw);
-
local_refs = get_local_heads();
 
flags = MATCH_REFS_NONE;
diff --git a/transport.c b/transport.c
index a89f17744..fe96c0b80 100644
--- a/transport.c
+++ b/transport.c
@@ -619,29 +619,6 @@ void transport_print_push_status(const char *dest, struct 
ref *refs,
free(head);
 }
 
-void transport_verify_remote_names(int nr_heads, const char **heads)
-{
-   int i;
-
-   for (i = 0; i < nr_heads; i++) {
-   const char *local = heads[i];
-   const char *remote = strrchr(heads[i], ':');
-
-   if (*local == '+')
-   local++;
-
-   /* A matching refspec is okay.  */
-   if (remote == local && remote[1] == '\0')
-   continue;
-
-   remote = remote ? (remote + 1) : local;
-   if (check_refname_format(remote,
-   REFNAME_ALLOW_ONELEVEL|REFNAME_REFSPEC_PATTERN))
-   die("remote part of refspec is not a valid name in %s",
-   heads[i]);
-   }
-}
-
 static int git_transport_push(struct transport *transport, struct ref 
*remote_refs, int flags)
 {
struct git_transport_data *data = transport->data;
@@ -1097,7 +1074,6 @@ int transport_push(struct transport *transport,
   unsigned int *reject_reasons)
 {
*reject_reasons = 0;
-   transport_verify_remote_names(rs->raw_nr, rs->raw);
 
if (transport_color_config() < 0)
return -1;
diff --git a/transport.h b/transport.h
index e2c809af4..bac085ce0 100644
--- a/transport.h
+++ b/transport.h
@@ -227,8 +227,6 @@ int transport_helper_init(struct transport *transport, 
const char *name);
 int bidirectional_transfer_loop(int input, int output);
 
 /* common methods used by transport.c and builtin/send-pack.c */
-void transport_verify_remote_names(int nr_heads, const char **heads);
-
 void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int 
verbose);
 
 int transport_refs_pushed(struct ref *ref);
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 33/35] remote: convert match_push_refs to take a struct refspec

Convert 'match_push_refs()' to take a 'struct refspec' as a parameter
instead of an array of 'const char *'.

Signed-off-by: Brandon Williams 
---
 builtin/remote.c|  3 +--
 builtin/send-pack.c |  2 +-
 http-push.c |  3 +--
 remote.c| 21 -
 remote.h|  2 +-
 transport.c |  4 +---
 6 files changed, 13 insertions(+), 22 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index b8e66589f..b84175cc6 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -387,8 +387,7 @@ static int get_push_ref_states(const struct ref 
*remote_refs,
local_refs = get_local_heads();
push_map = copy_ref_list(remote_refs);
 
-   match_push_refs(local_refs, _map, remote->push.raw_nr,
-   remote->push.raw, MATCH_REFS_NONE);
+   match_push_refs(local_refs, _map, >push, MATCH_REFS_NONE);
 
states->push.strdup_strings = 1;
for (ref = push_map; ref; ref = ref->next) {
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 7c34bf467..4923b1058 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -275,7 +275,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
flags |= MATCH_REFS_MIRROR;
 
/* match them up */
-   if (match_push_refs(local_refs, _refs, rs.raw_nr, rs.raw, flags))
+   if (match_push_refs(local_refs, _refs, , flags))
return -1;
 
if (!is_empty_cas())
diff --git a/http-push.c b/http-push.c
index a724ef03f..ea5af6227 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1823,8 +1823,7 @@ int cmd_main(int argc, const char **argv)
}
 
/* match them up */
-   if (match_push_refs(local_refs, _refs,
-   rs.raw_nr, rs.raw, push_all)) {
+   if (match_push_refs(local_refs, _refs, , push_all)) {
rc = -1;
goto cleanup;
}
diff --git a/remote.c b/remote.c
index 73d462f24..0e882d812 100644
--- a/remote.c
+++ b/remote.c
@@ -1294,23 +1294,20 @@ int check_push_refs(struct ref *src, int nr_refspec, 
const char **refspec_names)
  * dst (e.g. pushing to a new branch, done in match_explicit_refs).
  */
 int match_push_refs(struct ref *src, struct ref **dst,
-   int nr_refspec, const char **refspec, int flags)
+   struct refspec *rs, int flags)
 {
-   struct refspec rs = REFSPEC_INIT_PUSH;
int send_all = flags & MATCH_REFS_ALL;
int send_mirror = flags & MATCH_REFS_MIRROR;
int send_prune = flags & MATCH_REFS_PRUNE;
int errs;
-   static const char *default_refspec[] = { ":", NULL };
struct ref *ref, **dst_tail = tail_ref(dst);
struct string_list dst_ref_index = STRING_LIST_INIT_NODUP;
 
-   if (!nr_refspec) {
-   nr_refspec = 1;
-   refspec = default_refspec;
-   }
-   refspec_appendn(, refspec, nr_refspec);
-   errs = match_explicit_refs(src, *dst, _tail, );
+   /* If no refspec is provided, use the default ":" */
+   if (!rs->nr)
+   refspec_append(rs, ":");
+
+   errs = match_explicit_refs(src, *dst, _tail, rs);
 
/* pick the remainder */
for (ref = src; ref; ref = ref->next) {
@@ -1319,7 +1316,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
const struct refspec_item *pat = NULL;
char *dst_name;
 
-   dst_name = get_ref_match(, ref, send_mirror, FROM_SRC, );
+   dst_name = get_ref_match(rs, ref, send_mirror, FROM_SRC, );
if (!dst_name)
continue;
 
@@ -1368,7 +1365,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
/* We're already sending something to this ref. 
*/
continue;
 
-   src_name = get_ref_match(, ref, send_mirror, 
FROM_DST, NULL);
+   src_name = get_ref_match(rs, ref, send_mirror, 
FROM_DST, NULL);
if (src_name) {
if (!src_ref_index.nr)
prepare_ref_index(_ref_index, src);
@@ -1381,8 +1378,6 @@ int match_push_refs(struct ref *src, struct ref **dst,
string_list_clear(_ref_index, 0);
}
 
-   refspec_clear();
-
if (errs)
return -1;
return 0;
diff --git a/remote.h b/remote.h
index edcfc3600..d5b5f24ac 100644
--- a/remote.h
+++ b/remote.h
@@ -163,7 +163,7 @@ char *apply_refspecs(struct refspec *rs, const char *name);
 
 int check_push_refs(struct ref *src, int nr_refspec, const char **refspec);
 int match_push_refs(struct ref *src, struct ref **dst,
-   int nr_refspec, const char **refspec, int all);
+   struct refspec *rs, int flags);
 void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
int force_update);
 
diff --git

[PATCH 32/35] http-push: store refspecs in a struct refspec

Convert http-push.c to store refspecs in a 'struct refspec' instead of
in an array of 'const char *'.

Signed-off-by: Brandon Williams 
---
 http-push.c | 17 -
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/http-push.c b/http-push.c
index f308ce019..a724ef03f 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1692,8 +1692,7 @@ int cmd_main(int argc, const char **argv)
 {
struct transfer_request *request;
struct transfer_request *next_request;
-   int nr_refspec = 0;
-   const char **refspec = NULL;
+   struct refspec rs = REFSPEC_INIT_PUSH;
struct remote_lock *ref_lock = NULL;
struct remote_lock *info_ref_lock = NULL;
struct rev_info revs;
@@ -1756,8 +1755,7 @@ int cmd_main(int argc, const char **argv)
}
continue;
}
-   refspec = argv;
-   nr_refspec = argc - i;
+   refspec_appendn(, argv, argc - i);
break;
}
 
@@ -1768,7 +1766,7 @@ int cmd_main(int argc, const char **argv)
if (!repo->url)
usage(http_push_usage);
 
-   if (delete_branch && nr_refspec != 1)
+   if (delete_branch && rs.nr != 1)
die("You must specify only one branch name when deleting a 
remote branch");
 
setup_git_directory();
@@ -1814,18 +1812,19 @@ int cmd_main(int argc, const char **argv)
 
/* Remove a remote branch if -d or -D was specified */
if (delete_branch) {
-   if (delete_remote_branch(refspec[0], force_delete) == -1) {
+   const char *branch = rs.items[i].src;
+   if (delete_remote_branch(branch, force_delete) == -1) {
fprintf(stderr, "Unable to delete remote branch %s\n",
-   refspec[0]);
+   branch);
if (helper_status)
-   printf("error %s cannot remove\n", refspec[0]);
+   printf("error %s cannot remove\n", branch);
}
goto cleanup;
}
 
/* match them up */
if (match_push_refs(local_refs, _refs,
-   nr_refspec, (const char **) refspec, push_all)) {
+   rs.raw_nr, rs.raw, push_all)) {
rc = -1;
goto cleanup;
}
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 24/35] remote: convert query_refspecs to take a struct refspec

Convert 'query_refspecs()' to take a 'struct refspec' as a parameter instead
of a list of 'struct refspec_item'.

Signed-off-by: Brandon Williams 
---
 builtin/push.c |  3 +--
 remote.c   | 10 +-
 remote.h   |  2 +-
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 509dc6677..6b7e45890 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -83,8 +83,7 @@ static const char *map_refspec(const char *ref,
struct refspec_item query;
memset(, 0, sizeof(struct refspec_item));
query.src = matched->name;
-   if (!query_refspecs(remote->push.items, remote->push.nr, 
) &&
-   query.dst) {
+   if (!query_refspecs(>push, ) && query.dst) {
struct strbuf buf = STRBUF_INIT;
strbuf_addf(, "%s%s:%s",
query.force ? "+" : "",
diff --git a/remote.c b/remote.c
index 54297de3d..3d7bc7504 100644
--- a/remote.c
+++ b/remote.c
@@ -734,7 +734,7 @@ static void query_refspecs_multiple(struct refspec *rs,
}
 }
 
-int query_refspecs(struct refspec_item *refs, int ref_count, struct 
refspec_item *query)
+int query_refspecs(struct refspec *rs, struct refspec_item *query)
 {
int i;
int find_src = !query->src;
@@ -744,8 +744,8 @@ int query_refspecs(struct refspec_item *refs, int 
ref_count, struct refspec_item
if (find_src && !query->dst)
return error("query_refspecs: need either src or dst");
 
-   for (i = 0; i < ref_count; i++) {
-   struct refspec_item *refspec = [i];
+   for (i = 0; i < rs->nr; i++) {
+   struct refspec_item *refspec = >items[i];
const char *key = find_src ? refspec->dst : refspec->src;
const char *value = find_src ? refspec->src : refspec->dst;
 
@@ -772,7 +772,7 @@ char *apply_refspecs(struct refspec *rs, const char *name)
memset(, 0, sizeof(struct refspec_item));
query.src = (char *)name;
 
-   if (query_refspecs(rs->items, rs->nr, ))
+   if (query_refspecs(rs, ))
return NULL;
 
return query.dst;
@@ -780,7 +780,7 @@ char *apply_refspecs(struct refspec *rs, const char *name)
 
 int remote_find_tracking(struct remote *remote, struct refspec_item *refspec)
 {
-   return query_refspecs(remote->fetch.items, remote->fetch.nr, refspec);
+   return query_refspecs(>fetch, refspec);
 }
 
 static struct ref *alloc_ref_with_prefix(const char *prefix, size_t prefixlen,
diff --git a/remote.h b/remote.h
index 4e7590b55..edcfc3600 100644
--- a/remote.h
+++ b/remote.h
@@ -158,7 +158,7 @@ int ref_newer(const struct object_id *new_oid, const struct 
object_id *old_oid);
  */
 struct ref *ref_remove_duplicates(struct ref *ref_map);
 
-extern int query_refspecs(struct refspec_item *specs, int nr, struct 
refspec_item *query);
+int query_refspecs(struct refspec *rs, struct refspec_item *query);
 char *apply_refspecs(struct refspec *rs, const char *name);
 
 int check_push_refs(struct ref *src, int nr_refspec, const char **refspec);
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 27/35] push: check for errors earlier

Move the error checking for using the "--mirror", "--all", and "--tags"
options earlier and explicitly check for the presence of the flags
instead of checking for a side-effect of the flag.

Signed-off-by: Brandon Williams 
---
 builtin/push.c | 31 ++-
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 6b7e45890..df5df6c0d 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -417,23 +417,6 @@ static int do_push(const char *repo, int flags,
if (push_options->nr)
flags |= TRANSPORT_PUSH_OPTIONS;
 
-   if ((flags & TRANSPORT_PUSH_ALL) && refspec) {
-   if (!strcmp(*refspec, "refs/tags/*"))
-   return error(_("--all and --tags are incompatible"));
-   return error(_("--all can't be combined with refspecs"));
-   }
-
-   if ((flags & TRANSPORT_PUSH_MIRROR) && refspec) {
-   if (!strcmp(*refspec, "refs/tags/*"))
-   return error(_("--mirror and --tags are incompatible"));
-   return error(_("--mirror can't be combined with refspecs"));
-   }
-
-   if ((flags & (TRANSPORT_PUSH_ALL|TRANSPORT_PUSH_MIRROR)) ==
-   (TRANSPORT_PUSH_ALL|TRANSPORT_PUSH_MIRROR)) {
-   return error(_("--all and --mirror are incompatible"));
-   }
-
if (!refspec && !(flags & TRANSPORT_PUSH_ALL)) {
if (remote->push.raw_nr) {
refspec = remote->push.raw;
@@ -625,6 +608,20 @@ int cmd_push(int argc, const char **argv, const char 
*prefix)
die(_("--delete is incompatible with --all, --mirror and 
--tags"));
if (deleterefs && argc < 2)
die(_("--delete doesn't make sense without any refs"));
+   if (flags & TRANSPORT_PUSH_ALL) {
+   if (tags)
+   die(_("--all and --tags are incompatible"));
+   if (argc >= 2)
+   die(_("--all can't be combined with refspecs"));
+   }
+   if (flags & TRANSPORT_PUSH_MIRROR) {
+   if (tags)
+   die(_("--mirror and --tags are incompatible"));
+   if (argc >= 2)
+   die(_("--mirror can't be combined with refspecs"));
+   }
+   if ((flags & TRANSPORT_PUSH_ALL) && (flags & TRANSPORT_PUSH_MIRROR))
+   die(_("--all and --mirror are incompatible"));
 
if (recurse_submodules == RECURSE_SUBMODULES_CHECK)
flags |= TRANSPORT_RECURSE_SUBMODULES_CHECK;
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 26/35] remote: convert match_explicit_refs to take a struct refspec

Convert 'match_explicit_refs()' to take a 'struct refspec' as a
parameter instead of a list of 'struct refspec_item'.

Signed-off-by: Brandon Williams 
---
 remote.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/remote.c b/remote.c
index 0879ee587..73d462f24 100644
--- a/remote.c
+++ b/remote.c
@@ -1082,12 +1082,11 @@ static int match_explicit(struct ref *src, struct ref 
*dst,
 }
 
 static int match_explicit_refs(struct ref *src, struct ref *dst,
-  struct ref ***dst_tail, struct refspec_item *rs,
-  int rs_nr)
+  struct ref ***dst_tail, struct refspec *rs)
 {
int i, errs;
-   for (i = errs = 0; i < rs_nr; i++)
-   errs += match_explicit(src, dst, dst_tail, [i]);
+   for (i = errs = 0; i < rs->nr; i++)
+   errs += match_explicit(src, dst, dst_tail, >items[i]);
return errs;
 }
 
@@ -1311,7 +1310,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
refspec = default_refspec;
}
refspec_appendn(, refspec, nr_refspec);
-   errs = match_explicit_refs(src, *dst, _tail, rs.items, rs.nr);
+   errs = match_explicit_refs(src, *dst, _tail, );
 
/* pick the remainder */
for (ref = src; ref; ref = ref->next) {
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 25/35] remote: convert get_ref_match to take a struct refspec

Convert 'get_ref_match()' to take a 'struct refspec' as a parameter
instead of a list of 'struct refspec_item'.

Signed-off-by: Brandon Williams 
---
 remote.c | 26 ++
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/remote.c b/remote.c
index 3d7bc7504..0879ee587 100644
--- a/remote.c
+++ b/remote.c
@@ -1091,27 +1091,29 @@ static int match_explicit_refs(struct ref *src, struct 
ref *dst,
return errs;
 }
 
-static char *get_ref_match(const struct refspec_item *rs, int rs_nr, const 
struct ref *ref,
-   int send_mirror, int direction, const struct refspec_item 
**ret_pat)
+static char *get_ref_match(const struct refspec *rs, const struct ref *ref,
+  int send_mirror, int direction,
+  const struct refspec_item **ret_pat)
 {
const struct refspec_item *pat;
char *name;
int i;
int matching_refs = -1;
-   for (i = 0; i < rs_nr; i++) {
-   if (rs[i].matching &&
-   (matching_refs == -1 || rs[i].force)) {
+   for (i = 0; i < rs->nr; i++) {
+   const struct refspec_item *item = >items[i];
+   if (item->matching &&
+   (matching_refs == -1 || item->force)) {
matching_refs = i;
continue;
}
 
-   if (rs[i].pattern) {
-   const char *dst_side = rs[i].dst ? rs[i].dst : 
rs[i].src;
+   if (item->pattern) {
+   const char *dst_side = item->dst ? item->dst : 
item->src;
int match;
if (direction == FROM_SRC)
-   match = match_name_with_pattern(rs[i].src, 
ref->name, dst_side, );
+   match = match_name_with_pattern(item->src, 
ref->name, dst_side, );
else
-   match = match_name_with_pattern(dst_side, 
ref->name, rs[i].src, );
+   match = match_name_with_pattern(dst_side, 
ref->name, item->src, );
if (match) {
matching_refs = i;
break;
@@ -1121,7 +1123,7 @@ static char *get_ref_match(const struct refspec_item *rs, 
int rs_nr, const struc
if (matching_refs == -1)
return NULL;
 
-   pat = rs + matching_refs;
+   pat = >items[matching_refs];
if (pat->matching) {
/*
 * "matching refs"; traditionally we pushed everything
@@ -1318,7 +1320,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
const struct refspec_item *pat = NULL;
char *dst_name;
 
-   dst_name = get_ref_match(rs.items, rs.nr, ref, send_mirror, 
FROM_SRC, );
+   dst_name = get_ref_match(, ref, send_mirror, FROM_SRC, );
if (!dst_name)
continue;
 
@@ -1367,7 +1369,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
/* We're already sending something to this ref. 
*/
continue;
 
-   src_name = get_ref_match(rs.items, rs.nr, ref, 
send_mirror, FROM_DST, NULL);
+   src_name = get_ref_match(, ref, send_mirror, 
FROM_DST, NULL);
if (src_name) {
if (!src_ref_index.nr)
prepare_ref_index(_ref_index, src);
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 22/35] remote: convert get_stale_heads to take a struct refspec

Convert 'get_stale_heads()' to take a 'struct refspec' as a parameter instead
of a list of 'struct refspec_item'.

Signed-off-by: Brandon Williams 
---
 builtin/fetch.c  |  2 +-
 builtin/remote.c |  3 +--
 remote.c | 18 +-
 remote.h |  2 +-
 4 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index eebeebfd8..af7064dce 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -963,7 +963,7 @@ static int prune_refs(struct refspec *rs, struct ref 
*ref_map,
  const char *raw_url)
 {
int url_len, i, result = 0;
-   struct ref *ref, *stale_refs = get_stale_heads(rs->items, rs->nr, 
ref_map);
+   struct ref *ref, *stale_refs = get_stale_heads(rs, ref_map);
char *url;
int summary_width = transport_summary_width(stale_refs);
const char *dangling_msg = dry_run
diff --git a/builtin/remote.c b/builtin/remote.c
index 94dfcb78b..b8e66589f 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -347,8 +347,7 @@ static int get_ref_states(const struct ref *remote_refs, 
struct ref_states *stat
else
string_list_append(>tracked, 
abbrev_branch(ref->name));
}
-   stale_refs = get_stale_heads(states->remote->fetch.items,
-states->remote->fetch.nr, fetch_map);
+   stale_refs = get_stale_heads(>remote->fetch, fetch_map);
for (ref = stale_refs; ref; ref = ref->next) {
struct string_list_item *item =
string_list_append(>stale, 
abbrev_branch(ref->name));
diff --git a/remote.c b/remote.c
index db1e4edb7..f2e97c545 100644
--- a/remote.c
+++ b/remote.c
@@ -707,7 +707,9 @@ static int match_name_with_pattern(const char *key, const 
char *name,
return ret;
 }
 
-static void query_refspecs_multiple(struct refspec_item *refs, int ref_count, 
struct refspec_item *query, struct string_list *results)
+static void query_refspecs_multiple(struct refspec *rs,
+   struct refspec_item *query,
+   struct string_list *results)
 {
int i;
int find_src = !query->src;
@@ -715,8 +717,8 @@ static void query_refspecs_multiple(struct refspec_item 
*refs, int ref_count, st
if (find_src && !query->dst)
error("query_refspecs_multiple: need either src or dst");
 
-   for (i = 0; i < ref_count; i++) {
-   struct refspec_item *refspec = [i];
+   for (i = 0; i < rs->nr; i++) {
+   struct refspec_item *refspec = >items[i];
const char *key = find_src ? refspec->dst : refspec->src;
const char *value = find_src ? refspec->src : refspec->dst;
const char *needle = find_src ? query->dst : query->src;
@@ -2077,8 +2079,7 @@ struct ref *guess_remote_head(const struct ref *head,
 struct stale_heads_info {
struct string_list *ref_names;
struct ref **stale_refs_tail;
-   struct refspec_item *refs;
-   int ref_count;
+   struct refspec *rs;
 };
 
 static int get_stale_heads_cb(const char *refname, const struct object_id *oid,
@@ -2091,7 +2092,7 @@ static int get_stale_heads_cb(const char *refname, const 
struct object_id *oid,
memset(, 0, sizeof(struct refspec_item));
query.dst = (char *)refname;
 
-   query_refspecs_multiple(info->refs, info->ref_count, , );
+   query_refspecs_multiple(info->rs, , );
if (matches.nr == 0)
goto clean_exit; /* No matches */
 
@@ -2119,7 +2120,7 @@ static int get_stale_heads_cb(const char *refname, const 
struct object_id *oid,
return 0;
 }
 
-struct ref *get_stale_heads(struct refspec_item *refs, int ref_count, struct 
ref *fetch_map)
+struct ref *get_stale_heads(struct refspec *rs, struct ref *fetch_map)
 {
struct ref *ref, *stale_refs = NULL;
struct string_list ref_names = STRING_LIST_INIT_NODUP;
@@ -2127,8 +2128,7 @@ struct ref *get_stale_heads(struct refspec_item *refs, 
int ref_count, struct ref
 
info.ref_names = _names;
info.stale_refs_tail = _refs;
-   info.refs = refs;
-   info.ref_count = ref_count;
+   info.rs = rs;
for (ref = fetch_map; ref; ref = ref->next)
string_list_append(_names, ref->name);
string_list_sort(_names);
diff --git a/remote.h b/remote.h
index e7d00fe2a..5ac7536f5 100644
--- a/remote.h
+++ b/remote.h
@@ -267,7 +267,7 @@ struct ref *guess_remote_head(const struct ref *head,
  int all);
 
 /* Return refs which no longer exist on remote */
-struct ref *get_stale_heads(struct refspec_item *refs, int ref_count, struct 
ref *fetch_map);
+struct ref *get_stale_heads(struct refspec *rs, struct ref *fetch_map);
 
 /*
  * Compare-and-swap
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 19/35] fetch: convert do_fetch to take a struct refspec

Convert 'do_fetch()' to take a 'struct refspec' as a parameter instead
of a list of 'struct refspec_item'.

Signed-off-by: Brandon Williams 
---
 builtin/fetch.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index e2f2f290e..733feb19c 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1112,7 +1112,7 @@ static void backfill_tags(struct transport *transport, 
struct ref *ref_map)
 }
 
 static int do_fetch(struct transport *transport,
-   struct refspec_item *refs, int ref_count)
+   struct refspec *rs)
 {
struct string_list existing_refs = STRING_LIST_INIT_DUP;
struct ref *ref_map;
@@ -1136,7 +1136,7 @@ static int do_fetch(struct transport *transport,
goto cleanup;
}
 
-   ref_map = get_ref_map(transport, refs, ref_count, tags, );
+   ref_map = get_ref_map(transport, rs->items, rs->nr, tags, );
if (!update_head_ok)
check_not_current_branch(ref_map);
 
@@ -1160,8 +1160,8 @@ static int do_fetch(struct transport *transport,
 * explicitly (via command line or configuration); we
 * don't care whether --tags was specified.
 */
-   if (ref_count) {
-   prune_refs(refs, ref_count, ref_map, transport->url);
+   if (rs->nr) {
+   prune_refs(rs->items, rs->nr, ref_map, transport->url);
} else {
prune_refs(transport->remote->fetch.items,
   transport->remote->fetch.nr,
@@ -1410,7 +1410,7 @@ static int fetch_one(struct remote *remote, int argc, 
const char **argv, int pru
 
sigchain_push_common(unlock_pack_on_signal);
atexit(unlock_pack);
-   exit_code = do_fetch(gtransport, rs.items, rs.nr);
+   exit_code = do_fetch(gtransport, );
refspec_clear();
transport_disconnect(gtransport);
gtransport = NULL;
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 18/35] refspec: remove the deprecated functions

Now that there are no callers of 'parse_push_refspec()',
'parse_fetch_refspec()', and 'free_refspec()', remove these
functions.

Signed-off-by: Brandon Williams 
---
 refspec.c | 49 -
 refspec.h |  5 -
 2 files changed, 54 deletions(-)

diff --git a/refspec.c b/refspec.c
index 98c99cd84..7953a70d0 100644
--- a/refspec.c
+++ b/refspec.c
@@ -121,55 +121,6 @@ static int parse_refspec(struct refspec_item *rs, const 
char *refspec, int fetch
return 1;
 }
 
-static struct refspec_item *parse_refspec_internal(int nr_refspec, const char 
**refspec, int fetch, int verify)
-{
-   int i;
-   struct refspec_item *rs = xcalloc(nr_refspec, sizeof(*rs));
-
-   for (i = 0; i < nr_refspec; i++) {
-   if (!parse_refspec([i], refspec[i], fetch))
-   goto invalid;
-   }
-
-   return rs;
-
- invalid:
-   if (verify) {
-   /*
-* nr_refspec must be greater than zero and i must be valid
-* since it is only possible to reach this point from within
-* the for loop above.
-*/
-   free_refspec(i+1, rs);
-   return NULL;
-   }
-   die("Invalid refspec '%s'", refspec[i]);
-}
-
-struct refspec_item *parse_fetch_refspec(int nr_refspec, const char **refspec)
-{
-   return parse_refspec_internal(nr_refspec, refspec, 1, 0);
-}
-
-struct refspec_item *parse_push_refspec(int nr_refspec, const char **refspec)
-{
-   return parse_refspec_internal(nr_refspec, refspec, 0, 0);
-}
-
-void free_refspec(int nr_refspec, struct refspec_item *refspec)
-{
-   int i;
-
-   if (!refspec)
-   return;
-
-   for (i = 0; i < nr_refspec; i++) {
-   free(refspec[i].src);
-   free(refspec[i].dst);
-   }
-   free(refspec);
-}
-
 void refspec_item_init(struct refspec_item *item, const char *refspec, int 
fetch)
 {
memset(item, 0, sizeof(*item));
diff --git a/refspec.h b/refspec.h
index b009440c0..374f8ea63 100644
--- a/refspec.h
+++ b/refspec.h
@@ -14,11 +14,6 @@ struct refspec_item {
char *dst;
 };
 
-struct refspec_item *parse_fetch_refspec(int nr_refspec, const char **refspec);
-struct refspec_item *parse_push_refspec(int nr_refspec, const char **refspec);
-
-void free_refspec(int nr_refspec, struct refspec_item *refspec);
-
 #define REFSPEC_FETCH 1
 #define REFSPEC_PUSH 0
 
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 15/35] transport-helper: convert to use struct refspec

Convert the refspecs in transport-helper.c to be stored in a
'struct refspec'.

Signed-off-by: Brandon Williams 
---
 transport-helper.c | 38 --
 1 file changed, 12 insertions(+), 26 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index b156a37e7..33f51ebfc 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -36,8 +36,7 @@ struct helper_data {
char *export_marks;
char *import_marks;
/* These go from remote name (as in "list") to private name */
-   struct refspec_item *refspecs;
-   int refspec_nr;
+   struct refspec rs;
/* Transport options for fetch-pack/send-pack (should one of
 * those be invoked).
 */
@@ -107,9 +106,6 @@ static struct child_process *get_helper(struct transport 
*transport)
struct helper_data *data = transport->data;
struct strbuf buf = STRBUF_INIT;
struct child_process *helper;
-   const char **refspecs = NULL;
-   int refspec_nr = 0;
-   int refspec_alloc = 0;
int duped;
int code;
 
@@ -139,6 +135,7 @@ static struct child_process *get_helper(struct transport 
*transport)
 
data->helper = helper;
data->no_disconnect_req = 0;
+   refspec_init(>rs, REFSPEC_FETCH);
 
/*
 * Open the output as FILE* so strbuf_getline_*() family of
@@ -184,11 +181,8 @@ static struct child_process *get_helper(struct transport 
*transport)
data->export = 1;
else if (!strcmp(capname, "check-connectivity"))
data->check_connectivity = 1;
-   else if (!data->refspecs && skip_prefix(capname, "refspec ", 
)) {
-   ALLOC_GROW(refspecs,
-  refspec_nr + 1,
-  refspec_alloc);
-   refspecs[refspec_nr++] = xstrdup(arg);
+   else if (skip_prefix(capname, "refspec ", )) {
+   refspec_append(>rs, arg);
} else if (!strcmp(capname, "connect")) {
data->connect = 1;
} else if (!strcmp(capname, "stateless-connect")) {
@@ -207,14 +201,7 @@ static struct child_process *get_helper(struct transport 
*transport)
capname);
}
}
-   if (refspecs) {
-   int i;
-   data->refspec_nr = refspec_nr;
-   data->refspecs = parse_fetch_refspec(refspec_nr, refspecs);
-   for (i = 0; i < refspec_nr; i++)
-   free((char *)refspecs[i]);
-   free(refspecs);
-   } else if (data->import || data->bidi_import || data->export) {
+   if (!data->rs.nr && (data->import || data->bidi_import || 
data->export)) {
warning("This remote helper should implement refspec 
capability.");
}
strbuf_release();
@@ -378,8 +365,7 @@ static int release_helper(struct transport *transport)
 {
int res = 0;
struct helper_data *data = transport->data;
-   free_refspec(data->refspec_nr, data->refspecs);
-   data->refspecs = NULL;
+   refspec_clear(>rs);
res = disconnect_helper(transport);
free(transport->data);
return res;
@@ -536,8 +522,8 @@ static int fetch_with_import(struct transport *transport,
if (posn->status & REF_STATUS_UPTODATE)
continue;
name = posn->symref ? posn->symref : posn->name;
-   if (data->refspecs)
-   private = apply_refspecs(data->refspecs, 
data->refspec_nr, name);
+   if (data->rs.nr)
+   private = apply_refspecs(data->rs.items, data->rs.nr, 
name);
else
private = xstrdup(name);
if (private) {
@@ -815,11 +801,11 @@ static int push_update_refs_status(struct helper_data 
*data,
if (push_update_ref_status(, , remote_refs))
continue;
 
-   if (flags & TRANSPORT_PUSH_DRY_RUN || !data->refspecs || 
data->no_private_update)
+   if (flags & TRANSPORT_PUSH_DRY_RUN || !data->rs.nr || 
data->no_private_update)
continue;
 
/* propagate back the update to the remote namespace */
-   private = apply_refspecs(data->refspecs, data->refspec_nr, 
ref->name);
+   private = apply_refspecs(data->rs.items, data->rs.nr, 
ref->name);
if (!private)
continue;
update_ref("update by helper", private, >new_oid, NULL,
@@ -939,7 +925,7 @@ static int push_refs_with_export(struct transport 
*transport,
struct string_list revlist_args = STRING_LIST_INIT_DUP;
struct strbuf buf = STRBUF_INIT;
 
-   if (!data->refspecs)
+   if (!data->rs.nr)
die("remote-helper doesn't support push;

[PATCH 17/35] fetch: convert refmap to use struct refspec

Convert the refmap in builtin/fetch.c to be stored in a
'struct refspec'.

Signed-off-by: Brandon Williams 
---
 builtin/fetch.c | 17 +++--
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 769f9d2be..e2f2f290e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -60,8 +60,7 @@ static const char *submodule_prefix = "";
 static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;
 static int shown_url = 0;
-static int refmap_alloc, refmap_nr;
-static const char **refmap_array;
+static struct refspec refmap = REFSPEC_INIT_FETCH;
 static struct list_objects_filter_options filter_options;
 
 static int git_fetch_config(const char *k, const char *v, void *cb)
@@ -108,14 +107,12 @@ static int gitmodules_fetch_config(const char *var, const 
char *value, void *cb)
 
 static int parse_refmap_arg(const struct option *opt, const char *arg, int 
unset)
 {
-   ALLOC_GROW(refmap_array, refmap_nr + 1, refmap_alloc);
-
/*
 * "git fetch --refmap='' origin foo"
 * can be used to tell the command not to store anywhere
 */
-   if (*arg)
-   refmap_array[refmap_nr++] = arg;
+   refspec_append(, arg);
+
return 0;
 }
 
@@ -403,9 +400,9 @@ static struct ref *get_ref_map(struct transport *transport,
 * by ref_remove_duplicates() in favor of one of these
 * opportunistic entries with FETCH_HEAD_IGNORE.
 */
-   if (refmap_array) {
-   fetch_refspec = parse_fetch_refspec(refmap_nr, 
refmap_array);
-   fetch_refspec_nr = refmap_nr;
+   if (refmap.nr) {
+   fetch_refspec = refmap.items;
+   fetch_refspec_nr = refmap.nr;
} else {
fetch_refspec = transport->remote->fetch.items;
fetch_refspec_nr = transport->remote->fetch.nr;
@@ -413,7 +410,7 @@ static struct ref *get_ref_map(struct transport *transport,
 
for (i = 0; i < fetch_refspec_nr; i++)
get_fetch_map(ref_map, _refspec[i], _tail, 
1);
-   } else if (refmap_array) {
+   } else if (refmap.nr) {
die("--refmap option is only meaningful with command-line 
refspec(s).");
} else {
/* Use the defaults */
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 14/35] remote: convert fetch refspecs to struct refspec

Convert the set of fetch refspecs stored in 'struct remote' to use
'struct refspec'.

Signed-off-by: Brandon Williams 
---
 builtin/fetch.c  | 20 ++--
 builtin/remote.c | 18 +-
 remote.c | 24 ++--
 remote.h |  5 +
 4 files changed, 26 insertions(+), 41 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 745020a10..30083d4bc 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -407,8 +407,8 @@ static struct ref *get_ref_map(struct transport *transport,
fetch_refspec = parse_fetch_refspec(refmap_nr, 
refmap_array);
fetch_refspec_nr = refmap_nr;
} else {
-   fetch_refspec = transport->remote->fetch;
-   fetch_refspec_nr = transport->remote->fetch_refspec_nr;
+   fetch_refspec = transport->remote->fetch.items;
+   fetch_refspec_nr = transport->remote->fetch.nr;
}
 
for (i = 0; i < fetch_refspec_nr; i++)
@@ -421,16 +421,16 @@ static struct ref *get_ref_map(struct transport 
*transport,
struct branch *branch = branch_get(NULL);
int has_merge = branch_has_merge_config(branch);
if (remote &&
-   (remote->fetch_refspec_nr ||
+   (remote->fetch.nr ||
 /* Note: has_merge implies non-NULL branch->remote_name */
 (has_merge && !strcmp(branch->remote_name, 
remote->name {
-   for (i = 0; i < remote->fetch_refspec_nr; i++) {
-   get_fetch_map(remote_refs, >fetch[i], 
, 0);
-   if (remote->fetch[i].dst &&
-   remote->fetch[i].dst[0])
+   for (i = 0; i < remote->fetch.nr; i++) {
+   get_fetch_map(remote_refs, 
>fetch.items[i], , 0);
+   if (remote->fetch.items[i].dst &&
+   remote->fetch.items[i].dst[0])
*autotags = 1;
if (!i && !has_merge && ref_map &&
-   !remote->fetch[0].pattern)
+   !remote->fetch.items[0].pattern)
ref_map->fetch_head_status = 
FETCH_HEAD_MERGE;
}
/*
@@ -1166,8 +1166,8 @@ static int do_fetch(struct transport *transport,
if (ref_count) {
prune_refs(refs, ref_count, ref_map, transport->url);
} else {
-   prune_refs(transport->remote->fetch,
-  transport->remote->fetch_refspec_nr,
+   prune_refs(transport->remote->fetch.items,
+  transport->remote->fetch.nr,
   ref_map,
   transport->url);
}
diff --git a/builtin/remote.c b/builtin/remote.c
index fb84729d6..94dfcb78b 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -333,10 +333,10 @@ static int get_ref_states(const struct ref *remote_refs, 
struct ref_states *stat
struct ref *ref, *stale_refs;
int i;
 
-   for (i = 0; i < states->remote->fetch_refspec_nr; i++)
-   if (get_fetch_map(remote_refs, states->remote->fetch + i, 
, 1))
+   for (i = 0; i < states->remote->fetch.nr; i++)
+   if (get_fetch_map(remote_refs, >remote->fetch.items[i], 
, 1))
die(_("Could not get fetch map for refspec %s"),
-   states->remote->fetch_refspec[i]);
+   states->remote->fetch.raw[i]);
 
states->new_refs.strdup_strings = 1;
states->tracked.strdup_strings = 1;
@@ -347,8 +347,8 @@ static int get_ref_states(const struct ref *remote_refs, 
struct ref_states *stat
else
string_list_append(>tracked, 
abbrev_branch(ref->name));
}
-   stale_refs = get_stale_heads(states->remote->fetch,
-states->remote->fetch_refspec_nr, 
fetch_map);
+   stale_refs = get_stale_heads(states->remote->fetch.items,
+states->remote->fetch.nr, fetch_map);
for (ref = stale_refs; ref; ref = ref->next) {
struct string_list_item *item =
string_list_append(>stale, 
abbrev_branch(ref->name));
@@ -590,8 +590,8 @@ static int migrate_file(struct remote *remote)
git_config_set_multivar(buf.buf, remote->push.raw[i], "^$", 0);
strbuf_reset();
strbuf_addf(, "remote.%s.fetch", remote->name);
-   for (i = 0; i < remote->fetch_refspec_nr; i++)
-   git_config_set_multivar(buf.buf,

[PATCH 08/35] transport: convert transport_push to use struct refspec

Convert the logic in 'transport_push()' which calculates a list of
ref-prefixes to use 'struct refspec'.

Signed-off-by: Brandon Williams 
---
 transport.c | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/transport.c b/transport.c
index 3ad4d37dc..181db4d4d 100644
--- a/transport.c
+++ b/transport.c
@@ -,21 +,22 @@ int transport_push(struct transport *transport,
int porcelain = flags & TRANSPORT_PUSH_PORCELAIN;
int pretend = flags & TRANSPORT_PUSH_DRY_RUN;
int push_ret, ret, err;
-   struct refspec_item *tmp_rs;
+   struct refspec tmp_rs = REFSPEC_INIT_PUSH;
struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
int i;
 
if (check_push_refs(local_refs, refspec_nr, refspec) < 0)
return -1;
 
-   tmp_rs = parse_push_refspec(refspec_nr, refspec);
-   for (i = 0; i < refspec_nr; i++) {
+   refspec_appendn(_rs, refspec, refspec_nr);
+   for (i = 0; i < tmp_rs.nr; i++) {
+   const struct refspec_item *item = _rs.items[i];
const char *prefix = NULL;
 
-   if (tmp_rs[i].dst)
-   prefix = tmp_rs[i].dst;
-   else if (tmp_rs[i].src && !tmp_rs[i].exact_sha1)
-   prefix = tmp_rs[i].src;
+   if (item->dst)
+   prefix = item->dst;
+   else if (item->src && !item->exact_sha1)
+   prefix = item->src;
 
if (prefix) {
const char *glob = strchr(prefix, '*');
@@ -1142,7 +1143,7 @@ int transport_push(struct transport *transport,
   _prefixes);
 
argv_array_clear(_prefixes);
-   free_refspec(refspec_nr, tmp_rs);
+   refspec_clear(_rs);
 
if (flags & TRANSPORT_PUSH_ALL)
match_flags |= MATCH_REFS_ALL;
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 09/35] remote: convert check_push_refs to use struct refspec

Convert 'check_push_refs()' to use 'struct refspec'.

Signed-off-by: Brandon Williams 
---
 remote.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/remote.c b/remote.c
index 89820c476..191855118 100644
--- a/remote.c
+++ b/remote.c
@@ -1282,12 +1282,14 @@ static void prepare_ref_index(struct string_list 
*ref_index, struct ref *ref)
  */
 int check_push_refs(struct ref *src, int nr_refspec, const char 
**refspec_names)
 {
-   struct refspec_item *refspec = parse_push_refspec(nr_refspec, 
refspec_names);
+   struct refspec refspec = REFSPEC_INIT_PUSH;
int ret = 0;
int i;
 
-   for (i = 0; i < nr_refspec; i++) {
-   struct refspec_item *rs = refspec + i;
+   refspec_appendn(, refspec_names, nr_refspec);
+
+   for (i = 0; i < refspec.nr; i++) {
+   struct refspec_item *rs = [i];
 
if (rs->pattern || rs->matching)
continue;
@@ -1295,7 +1297,7 @@ int check_push_refs(struct ref *src, int nr_refspec, 
const char **refspec_names)
ret |= match_explicit_lhs(src, rs, NULL, NULL);
}
 
-   free_refspec(nr_refspec, refspec);
+   refspec_clear();
return ret;
 }
 
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 05/35] refspec: convert valid_fetch_refspec to use parse_refspec

Convert 'valid_fetch_refspec()' to use the new 'parse_refspec()'
function to only parse a single refspec an eliminate an allocation.

Signed-off-by: Brandon Williams 
---
 refspec.c | 17 -
 refspec.h |  3 ++-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/refspec.c b/refspec.c
index 2b898c922..98c99cd84 100644
--- a/refspec.c
+++ b/refspec.c
@@ -146,15 +146,6 @@ static struct refspec_item *parse_refspec_internal(int 
nr_refspec, const char **
die("Invalid refspec '%s'", refspec[i]);
 }
 
-int valid_fetch_refspec(const char *fetch_refspec_str)
-{
-   struct refspec_item *refspec;
-
-   refspec = parse_refspec_internal(1, _refspec_str, 1, 1);
-   free_refspec(1, refspec);
-   return !!refspec;
-}
-
 struct refspec_item *parse_fetch_refspec(int nr_refspec, const char **refspec)
 {
return parse_refspec_internal(nr_refspec, refspec, 1, 0);
@@ -242,3 +233,11 @@ void refspec_clear(struct refspec *rs)
 
rs->fetch = 0;
 }
+
+int valid_fetch_refspec(const char *fetch_refspec_str)
+{
+   struct refspec_item refspec;
+   int ret = parse_refspec(, fetch_refspec_str, REFSPEC_FETCH);
+   refspec_item_clear();
+   return ret;
+}
diff --git a/refspec.h b/refspec.h
index f6fb251f3..b009440c0 100644
--- a/refspec.h
+++ b/refspec.h
@@ -14,7 +14,6 @@ struct refspec_item {
char *dst;
 };
 
-int valid_fetch_refspec(const char *refspec);
 struct refspec_item *parse_fetch_refspec(int nr_refspec, const char **refspec);
 struct refspec_item *parse_push_refspec(int nr_refspec, const char **refspec);
 
@@ -45,4 +44,6 @@ void refspec_append(struct refspec *rs, const char *refspec);
 void refspec_appendn(struct refspec *rs, const char **refspecs, int nr);
 void refspec_clear(struct refspec *rs);
 
+int valid_fetch_refspec(const char *refspec);
+
 #endif /* REFSPEC_H */
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 07/35] pull: convert get_tracking_branch to use refspec_item_init

Convert 'get_tracking_branch()' to use 'refspec_item_init()' instead of
the old 'parse_fetch_refspec()' function.

Signed-off-by: Brandon Williams 
---
 builtin/pull.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 5a79deae5..09575fd23 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -676,12 +676,12 @@ static const char *get_upstream_branch(const char *remote)
  */
 static const char *get_tracking_branch(const char *remote, const char *refspec)
 {
-   struct refspec_item *spec;
+   struct refspec_item spec;
const char *spec_src;
const char *merge_branch;
 
-   spec = parse_fetch_refspec(1, );
-   spec_src = spec->src;
+   refspec_item_init(, refspec, REFSPEC_FETCH);
+   spec_src = spec.src;
if (!*spec_src || !strcmp(spec_src, "HEAD"))
spec_src = "HEAD";
else if (skip_prefix(spec_src, "heads/", _src))
@@ -701,7 +701,7 @@ static const char *get_tracking_branch(const char *remote, 
const char *refspec)
} else
merge_branch = NULL;
 
-   free_refspec(1, spec);
+   refspec_item_clear();
return merge_branch;
 }
 
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 04/35] refspec: introduce struct refspec

Introduce 'struct refspec', an abstraction around a collection of
'struct refspec_item's much like how 'struct pathspec' holds a
collection of 'struct pathspec_item's.

A refspec struct also contains an array of the original refspec strings
which will be used to facilitate the migration to using this new
abstraction throughout the code base.

Signed-off-by: Brandon Williams 
---
 refspec.c | 64 +++
 refspec.h | 25 ++
 2 files changed, 89 insertions(+)

diff --git a/refspec.c b/refspec.c
index cef513ad8..2b898c922 100644
--- a/refspec.c
+++ b/refspec.c
@@ -178,3 +178,67 @@ void free_refspec(int nr_refspec, struct refspec_item 
*refspec)
}
free(refspec);
 }
+
+void refspec_item_init(struct refspec_item *item, const char *refspec, int 
fetch)
+{
+   memset(item, 0, sizeof(*item));
+
+   if (!parse_refspec(item, refspec, fetch))
+   die("Invalid refspec '%s'", refspec);
+}
+
+void refspec_item_clear(struct refspec_item *item)
+{
+   FREE_AND_NULL(item->src);
+   FREE_AND_NULL(item->dst);
+   item->force = 0;
+   item->pattern = 0;
+   item->matching = 0;
+   item->exact_sha1 = 0;
+}
+
+void refspec_init(struct refspec *rs, int fetch)
+{
+   memset(rs, 0, sizeof(*rs));
+   rs->fetch = fetch;
+}
+
+void refspec_append(struct refspec *rs, const char *refspec)
+{
+   struct refspec_item item;
+
+   refspec_item_init(, refspec, rs->fetch);
+
+   ALLOC_GROW(rs->items, rs->nr + 1, rs->alloc);
+   rs->items[rs->nr++] = item;
+
+   ALLOC_GROW(rs->raw, rs->raw_nr + 1, rs->raw_alloc);
+   rs->raw[rs->raw_nr++] = xstrdup(refspec);
+}
+
+void refspec_appendn(struct refspec *rs, const char **refspecs, int nr)
+{
+   int i;
+   for (i = 0; i < nr; i++)
+   refspec_append(rs, refspecs[i]);
+}
+
+void refspec_clear(struct refspec *rs)
+{
+   int i;
+
+   for (i = 0; i < rs->nr; i++)
+   refspec_item_clear(>items[i]);
+
+   FREE_AND_NULL(rs->items);
+   rs->alloc = 0;
+   rs->nr = 0;
+
+   for (i = 0; i < rs->raw_nr; i++)
+   free((char *)rs->raw[i]);
+   FREE_AND_NULL(rs->raw);
+   rs->raw_alloc = 0;
+   rs->raw_nr = 0;
+
+   rs->fetch = 0;
+}
diff --git a/refspec.h b/refspec.h
index 173cea882..f6fb251f3 100644
--- a/refspec.h
+++ b/refspec.h
@@ -20,4 +20,29 @@ struct refspec_item *parse_push_refspec(int nr_refspec, 
const char **refspec);
 
 void free_refspec(int nr_refspec, struct refspec_item *refspec);
 
+#define REFSPEC_FETCH 1
+#define REFSPEC_PUSH 0
+
+#define REFSPEC_INIT_FETCH { .fetch = REFSPEC_FETCH }
+#define REFSPEC_INIT_PUSH { .fetch = REFSPEC_PUSH }
+
+struct refspec {
+   struct refspec_item *items;
+   int alloc;
+   int nr;
+
+   const char **raw;
+   int raw_alloc;
+   int raw_nr;
+
+   int fetch;
+};
+
+void refspec_item_init(struct refspec_item *item, const char *refspec, int 
fetch);
+void refspec_item_clear(struct refspec_item *item);
+void refspec_init(struct refspec *rs, int fetch);
+void refspec_append(struct refspec *rs, const char *refspec);
+void refspec_appendn(struct refspec *rs, const char **refspecs, int nr);
+void refspec_clear(struct refspec *rs);
+
 #endif /* REFSPEC_H */
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 01/35] refspec: move refspec parsing logic into its own file

In preperation for performing a refactor on refspec related code, move
the refspec parsing logic into its own file.

Signed-off-by: Brandon Williams 
---
 Makefile|   1 +
 branch.c|   1 +
 builtin/clone.c |   1 +
 builtin/fast-export.c   |   1 +
 builtin/fetch.c |   1 +
 builtin/merge.c |   1 +
 builtin/pull.c  |   1 +
 builtin/push.c  |   1 +
 builtin/remote.c|   1 +
 builtin/submodule--helper.c |   1 +
 checkout.c  |   1 +
 refspec.c   | 168 
 refspec.h   |  23 +
 remote.c| 165 +--
 remote.h|  20 -
 transport-helper.c  |   1 +
 transport.c |   1 +
 17 files changed, 205 insertions(+), 184 deletions(-)
 create mode 100644 refspec.c
 create mode 100644 refspec.h

diff --git a/Makefile b/Makefile
index ad880d1fc..4bca65383 100644
--- a/Makefile
+++ b/Makefile
@@ -928,6 +928,7 @@ LIB_OBJS += refs/files-backend.o
 LIB_OBJS += refs/iterator.o
 LIB_OBJS += refs/packed-backend.o
 LIB_OBJS += refs/ref-cache.o
+LIB_OBJS += refspec.o
 LIB_OBJS += ref-filter.o
 LIB_OBJS += remote.o
 LIB_OBJS += replace-object.o
diff --git a/branch.c b/branch.c
index 2672054f0..32ccefc6b 100644
--- a/branch.c
+++ b/branch.c
@@ -3,6 +3,7 @@
 #include "config.h"
 #include "branch.h"
 #include "refs.h"
+#include "refspec.h"
 #include "remote.h"
 #include "commit.h"
 #include "worktree.h"
diff --git a/builtin/clone.c b/builtin/clone.c
index 84f1473d1..6d1614ed3 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -14,6 +14,7 @@
 #include "parse-options.h"
 #include "fetch-pack.h"
 #include "refs.h"
+#include "refspec.h"
 #include "tree.h"
 #include "tree-walk.h"
 #include "unpack-trees.h"
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 530df12f0..a13b7c8ef 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -7,6 +7,7 @@
 #include "cache.h"
 #include "config.h"
 #include "refs.h"
+#include "refspec.h"
 #include "commit.h"
 #include "object.h"
 #include "tag.h"
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7ee83ac0f..1fce68e9a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -5,6 +5,7 @@
 #include "config.h"
 #include "repository.h"
 #include "refs.h"
+#include "refspec.h"
 #include "commit.h"
 #include "builtin.h"
 #include "string-list.h"
diff --git a/builtin/merge.c b/builtin/merge.c
index 9db5a2cf1..c362cfe90 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -14,6 +14,7 @@
 #include "run-command.h"
 #include "diff.h"
 #include "refs.h"
+#include "refspec.h"
 #include "commit.h"
 #include "diffcore.h"
 #include "revision.h"
diff --git a/builtin/pull.c b/builtin/pull.c
index 71aac5005..6247c956d 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -15,6 +15,7 @@
 #include "remote.h"
 #include "dir.h"
 #include "refs.h"
+#include "refspec.h"
 #include "revision.h"
 #include "submodule.h"
 #include "submodule-config.h"
diff --git a/builtin/push.c b/builtin/push.c
index ac3705370..fa65999b2 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -4,6 +4,7 @@
 #include "cache.h"
 #include "config.h"
 #include "refs.h"
+#include "refspec.h"
 #include "run-command.h"
 #include "builtin.h"
 #include "remote.h"
diff --git a/builtin/remote.c b/builtin/remote.c
index 8708e584e..c49513995 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -7,6 +7,7 @@
 #include "strbuf.h"
 #include "run-command.h"
 #include "refs.h"
+#include "refspec.h"
 #include "argv-array.h"
 
 static const char * const builtin_remote_usage[] = {
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c2403a915..6ab032acb 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -12,6 +12,7 @@
 #include "run-command.h"
 #include "remote.h"
 #include "refs.h"
+#include "refspec.h"
 #include "connect.h"
 #include "revision.h"
 #include "diffcore.h"
diff --git a/checkout.c b/checkout.c
index ac42630f7..193ba8567 100644
--- a/checkout.c
+++ b/checkout.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "remote.h"
+#include "refspec.h"
 #include "checkout.h"
 
 struct tracking_name_data {
diff --git a/refspec.c b/refspec.c
new file mode 100644
index 0..ecb0bdff3
--- /dev/null
+++ b/refspec.c
@@ -0,0 +1,168 @@
+#include "cache.h"
+#include "refs.h"
+#include "refspec.h"
+
+static struct refspec s_tag_refspec = {
+   0,
+   1,
+   0,
+   0,
+   "refs/tags/*",
+   "refs/tags/*"
+};
+
+/* See TAG_REFSPEC for the string version */
+const struct refspec *tag_refspec = _tag_refspec;
+
+static struct refspec *parse_refspec_internal(int nr_refspec, const char 
**refspec, int fetch, int verify)
+{
+   int i;
+   struct refspec *rs = xcalloc(nr_refspec, sizeof(*rs));
+
+   for (i = 0; i < nr_refspec; i++) {
+   size_t llen;
+

[PATCH 06/35] submodule--helper: convert push_check to use struct refspec

Convert 'push_check()' to use 'struct refspec'.

Signed-off-by: Brandon Williams 
---
 builtin/submodule--helper.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c0c4db007..88a149a2c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1744,13 +1744,14 @@ static int push_check(int argc, const char **argv, 
const char *prefix)
 
/* Check the refspec */
if (argc > 2) {
-   int i, refspec_nr = argc - 2;
+   int i;
struct ref *local_refs = get_local_heads();
-   struct refspec_item *refspec = parse_push_refspec(refspec_nr,
-argv + 2);
+   struct refspec refspec = REFSPEC_INIT_PUSH;
 
-   for (i = 0; i < refspec_nr; i++) {
-   struct refspec_item *rs = refspec + i;
+   refspec_appendn(, argv + 2, argc - 2);
+
+   for (i = 0; i < refspec.nr; i++) {
+   const struct refspec_item *rs = [i];
 
if (rs->pattern || rs->matching)
continue;
@@ -1777,7 +1778,7 @@ static int push_check(int argc, const char **argv, const 
char *prefix)
rs->src);
}
}
-   free_refspec(refspec_nr, refspec);
+   refspec_clear();
}
free(head);
 
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 02/35] refspec: factor out parsing a single refspec

Factor out the logic which parses a single refspec into its own
function.  This makes it easier to reuse this logic in a future patch.

Signed-off-by: Brandon Williams 
---
 refspec.c | 195 +-
 1 file changed, 104 insertions(+), 91 deletions(-)

diff --git a/refspec.c b/refspec.c
index ecb0bdff3..3cfcbd37d 100644
--- a/refspec.c
+++ b/refspec.c
@@ -14,110 +14,123 @@ static struct refspec s_tag_refspec = {
 /* See TAG_REFSPEC for the string version */
 const struct refspec *tag_refspec = _tag_refspec;
 
-static struct refspec *parse_refspec_internal(int nr_refspec, const char 
**refspec, int fetch, int verify)
+/*
+ * Parses 'refspec' and populates 'rs'.  returns 1 if successful and 0 if the
+ * refspec is invalid.
+ */
+static int parse_refspec(struct refspec *rs, const char *refspec, int fetch)
 {
-   int i;
-   struct refspec *rs = xcalloc(nr_refspec, sizeof(*rs));
+   size_t llen;
+   int is_glob;
+   const char *lhs, *rhs;
+   int flags;
 
-   for (i = 0; i < nr_refspec; i++) {
-   size_t llen;
-   int is_glob;
-   const char *lhs, *rhs;
-   int flags;
+   is_glob = 0;
 
-   is_glob = 0;
+   lhs = refspec;
+   if (*lhs == '+') {
+   rs->force = 1;
+   lhs++;
+   }
 
-   lhs = refspec[i];
-   if (*lhs == '+') {
-   rs[i].force = 1;
-   lhs++;
-   }
+   rhs = strrchr(lhs, ':');
 
-   rhs = strrchr(lhs, ':');
+   /*
+* Before going on, special case ":" (or "+:") as a refspec
+* for pushing matching refs.
+*/
+   if (!fetch && rhs == lhs && rhs[1] == '\0') {
+   rs->matching = 1;
+   return 1;
+   }
 
+   if (rhs) {
+   size_t rlen = strlen(++rhs);
+   is_glob = (1 <= rlen && strchr(rhs, '*'));
+   rs->dst = xstrndup(rhs, rlen);
+   }
+
+   llen = (rhs ? (rhs - lhs - 1) : strlen(lhs));
+   if (1 <= llen && memchr(lhs, '*', llen)) {
+   if ((rhs && !is_glob) || (!rhs && fetch))
+   return 0;
+   is_glob = 1;
+   } else if (rhs && is_glob) {
+   return 0;
+   }
+
+   rs->pattern = is_glob;
+   rs->src = xstrndup(lhs, llen);
+   flags = REFNAME_ALLOW_ONELEVEL | (is_glob ? REFNAME_REFSPEC_PATTERN : 
0);
+
+   if (fetch) {
+   struct object_id unused;
+
+   /* LHS */
+   if (!*rs->src)
+   ; /* empty is ok; it means "HEAD" */
+   else if (llen == GIT_SHA1_HEXSZ && !get_oid_hex(rs->src, 
))
+   rs->exact_sha1 = 1; /* ok */
+   else if (!check_refname_format(rs->src, flags))
+   ; /* valid looking ref is ok */
+   else
+   return 0;
+   /* RHS */
+   if (!rs->dst)
+   ; /* missing is ok; it is the same as empty */
+   else if (!*rs->dst)
+   ; /* empty is ok; it means "do not store" */
+   else if (!check_refname_format(rs->dst, flags))
+   ; /* valid looking ref is ok */
+   else
+   return 0;
+   } else {
/*
-* Before going on, special case ":" (or "+:") as a refspec
-* for pushing matching refs.
+* LHS
+* - empty is allowed; it means delete.
+* - when wildcarded, it must be a valid looking ref.
+* - otherwise, it must be an extended SHA-1, but
+*   there is no existing way to validate this.
 */
-   if (!fetch && rhs == lhs && rhs[1] == '\0') {
-   rs[i].matching = 1;
-   continue;
+   if (!*rs->src)
+   ; /* empty is ok */
+   else if (is_glob) {
+   if (check_refname_format(rs->src, flags))
+   return 0;
}
-
-   if (rhs) {
-   size_t rlen = strlen(++rhs);
-   is_glob = (1 <= rlen && strchr(rhs, '*'));
-   rs[i].dst = xstrndup(rhs, rlen);
+   else
+   ; /* anything goes, for now */
+   /*
+* RHS
+* - missing is allowed, but LHS then must be a
+*   valid looking ref.
+* - empty is not allowed.
+* - otherwise it must be a valid looking ref.
+*/
+   if (!rs->dst) {
+   if (check_refname_format(rs->src, flags))
+   return 0;
+   } else if (!*rs->dst) {
+

[PATCH 03/35] refspec: rename struct refspec to struct refspec_item

In preperation for introducing an abstraction around a collection of
refspecs (much like how a 'struct pathspec' is a collection of 'struct
pathspec_item's) rename the existing 'struct refspec' to 'struct
refspec_item'.

Signed-off-by: Brandon Williams 
---
 branch.c|  6 ++---
 builtin/clone.c |  4 +--
 builtin/fast-export.c   |  4 +--
 builtin/fetch.c | 12 -
 builtin/pull.c  |  2 +-
 builtin/push.c  |  4 +--
 builtin/remote.c|  8 +++---
 builtin/submodule--helper.c |  4 +--
 checkout.c  |  4 +--
 refspec.c   | 19 +++---
 refspec.h   | 10 
 remote.c| 50 ++---
 remote.h| 16 ++--
 transport-helper.c  |  2 +-
 transport.c |  4 +--
 15 files changed, 74 insertions(+), 75 deletions(-)

diff --git a/branch.c b/branch.c
index 32ccefc6b..f967c98f6 100644
--- a/branch.c
+++ b/branch.c
@@ -9,7 +9,7 @@
 #include "worktree.h"
 
 struct tracking {
-   struct refspec spec;
+   struct refspec_item spec;
char *src;
const char *remote;
int matches;
@@ -219,8 +219,8 @@ int validate_new_branchname(const char *name, struct strbuf 
*ref, int force)
 static int check_tracking_branch(struct remote *remote, void *cb_data)
 {
char *tracking_branch = cb_data;
-   struct refspec query;
-   memset(, 0, sizeof(struct refspec));
+   struct refspec_item query;
+   memset(, 0, sizeof(struct refspec_item));
query.dst = tracking_branch;
return !remote_find_tracking(remote, );
 }
diff --git a/builtin/clone.c b/builtin/clone.c
index 6d1614ed3..854088a3a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -547,7 +547,7 @@ static struct ref *find_remote_branch(const struct ref 
*refs, const char *branch
 }
 
 static struct ref *wanted_peer_refs(const struct ref *refs,
-   struct refspec *refspec)
+   struct refspec_item *refspec)
 {
struct ref *head = copy_ref(find_ref_by_name(refs, "HEAD"));
struct ref *local_refs = head;
@@ -895,7 +895,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
int err = 0, complete_refs_before_fetch = 1;
int submodule_progress;
 
-   struct refspec *refspec;
+   struct refspec_item *refspec;
const char *fetch_pattern;
 
fetch_if_missing = 0;
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index a13b7c8ef..6f105dc79 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -36,7 +36,7 @@ static int use_done_feature;
 static int no_data;
 static int full_tree;
 static struct string_list extra_refs = STRING_LIST_INIT_NODUP;
-static struct refspec *refspecs;
+static struct refspec_item *refspecs;
 static int refspecs_nr;
 static int anonymize;
 
@@ -979,7 +979,7 @@ static void handle_deletes(void)
 {
int i;
for (i = 0; i < refspecs_nr; i++) {
-   struct refspec *refspec = [i];
+   struct refspec_item *refspec = [i];
if (*refspec->src)
continue;
 
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 1fce68e9a..745020a10 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -203,7 +203,7 @@ static void add_merge_config(struct ref **head,
 
for (i = 0; i < branch->merge_nr; i++) {
struct ref *rm, **old_tail = *tail;
-   struct refspec refspec;
+   struct refspec_item refspec;
 
for (rm = *head; rm; rm = rm->next) {
if (branch_merge_matches(branch, i, rm->name)) {
@@ -340,7 +340,7 @@ static void find_non_local_tags(struct transport *transport,
 }
 
 static struct ref *get_ref_map(struct transport *transport,
-  struct refspec *refspecs, int refspec_count,
+  struct refspec_item *refspecs, int refspec_count,
   int tags, int *autotags)
 {
int i;
@@ -371,7 +371,7 @@ static struct ref *get_ref_map(struct transport *transport,
argv_array_clear(_prefixes);
 
if (refspec_count) {
-   struct refspec *fetch_refspec;
+   struct refspec_item *fetch_refspec;
int fetch_refspec_nr;
 
for (i = 0; i < refspec_count; i++) {
@@ -965,7 +965,7 @@ static int fetch_refs(struct transport *transport, struct 
ref *ref_map)
return ret;
 }
 
-static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map,
+static int prune_refs(struct refspec_item *refs, int ref_count, struct ref 
*ref_map,
const char *raw_url)
 {
int url_len, i, result = 0;
@@ -1115,7 +1115,7 @@ static void backfill_tags(struct transport *transport, 
struct ref *ref_map)
 }
 
 static int do_fetch(struct transport *transport,
-

[PATCH 00/35] refactoring refspecs

When working on protocol v2 I noticed that working with refspecs was a
little difficult because of the various api's that existed.  Some
functions expected an array of "const char *" while others expected an
array of "struct refspec".  In all cases a length parameter needed to be
passed as a parameter as well.  This makes working with refspecs a
little challenging because of the different expectations different parts
of the code base have.

This series refactors how refspecs are handled through out the code base
by renaming the existing struct refspec to refspec_item and introducing
a new 'struct refspec' which is a container of refspec_items, much like
how a pathspec contains pathspec_items.  This simplifies many callers
and makes handling pathspecs a bit easier.

I have some follow on work which I'll build on top of this series, but
since this was already getting a bit lengthy at 35 patches I'll save
that for another time.

Brandon Williams (35):
  refspec: move refspec parsing logic into its own file
  refspec: factor out parsing a single refspec
  refspec: rename struct refspec to struct refspec_item
  refspec: introduce struct refspec
  refspec: convert valid_fetch_refspec to use parse_refspec
  submodule--helper: convert push_check to use struct refspec
  pull: convert get_tracking_branch to use refspec_item_init
  transport: convert transport_push to use struct refspec
  remote: convert check_push_refs to use struct refspec
  remote: convert match_push_refs to use struct refspec
  clone: convert cmd_clone to use refspec_item_init
  fast-export: convert to use struct refspec
  remote: convert push refspecs to struct refspec
  remote: convert fetch refspecs to struct refspec
  transport-helper: convert to use struct refspec
  fetch: convert fetch_one to use struct refspec
  fetch: convert refmap to use struct refspec
  refspec: remove the deprecated functions
  fetch: convert do_fetch to take a struct refspec
  fetch: convert get_ref_map to take a struct refspec
  fetch: convert prune_refs to take a struct refspec
  remote: convert get_stale_heads to take a struct refspec
  remote: convert apply_refspecs to take a struct refspec
  remote: convert query_refspecs to take a struct refspec
  remote: convert get_ref_match to take a struct refspec
  remote: convert match_explicit_refs to take a struct refspec
  push: check for errors earlier
  push: convert to use struct refspec
  transport: convert transport_push to take a struct refspec
  send-pack: store refspecs in a struct refspec
  transport: remove transport_verify_remote_names
  http-push: store refspecs in a struct refspec
  remote: convert match_push_refs to take a struct refspec
  remote: convert check_push_refs to take a struct refspec
  submodule: convert push_unpushed_submodules to take a struct refspec

 Makefile|   1 +
 branch.c|   7 +-
 builtin/clone.c |  13 +-
 builtin/fast-export.c   |  22 +--
 builtin/fetch.c | 128 +++---
 builtin/merge.c |   1 +
 builtin/pull.c  |   9 +-
 builtin/push.c  |  80 -
 builtin/remote.c|  37 ++--
 builtin/send-pack.c |  24 +--
 builtin/submodule--helper.c |  14 +-
 checkout.c  |   5 +-
 http-push.c |  18 +-
 refspec.c   | 194 +
 refspec.h   |  44 +
 remote.c| 324 
 remote.h|  48 ++
 submodule.c |  19 +--
 submodule.h |   3 +-
 transport-helper.c  |  39 ++---
 transport.c |  51 ++
 transport.h |   4 +-
 22 files changed, 514 insertions(+), 571 deletions(-)
 create mode 100644 refspec.c
 create mode 100644 refspec.h

-- 
2.17.0.441.gb46fe60e1d-goog

Re: [RFC] Other chunks for commit-graph, part 1 - Bloom filters, topo order, etc.

2018-05-14 Thread Jakub Narebski

Derrick Stolee  writes:
> On 5/12/2018 10:00 AM, Jakub Narebski wrote:
>> Derrick Stolee  writes:
>>> On 5/4/2018 3:40 PM, Jakub Narebski wrote:

 With early parts of commit-graph feature (ds/commit-graph and
 ds/lazy-load-trees) close to being merged into "master", see
 https://public-inbox.org/git/xmqq4ljtz87g@gitster-ct.c.googlers.com/
 I think it would be good idea to think what other data could be added
 there to make Git even faster.
>>> Before thinking about adding more data to the commit-graph, I think
>>> instead we need to finish taking advantage of the data that is already
>>> there. This means landing the generation number patch [1] (I think
>>> this is close, so I'll send a v6 this week if there is no new
>>> feedback.) and the auto-compute patch [2] (this could use more
>>> feedback, but I'll send a v1 based on the RFC feedback if no one
>>> chimes in).
>>>
>>> [1]
>>> https://public-inbox.org/git/20180501124652.155781-1-dsto...@microsoft.com/
>>>      [PATCH v5 00/11] Compute and consume generation numbers
>>>
>>> [2]
>>> https://public-inbox.org/git/20180417181028.198397-1-dsto...@microsoft.com/
>>>      [RFC PATCH 00/12] Integrate commit-graph into 'fsck' and 'gc'
>>
>> Right, so the RFC might be a bit premature; I wanted the discussion to
>> be out there to think about when adding new uses of existing features.
>>
>>
>> DIGRESSION: it is commendable that you are sending patches in small,
>> easy digestible chunks / patch series.  It is much easier to review 10+
>> series than 80+ behemoth (though I understand it is not always possible
>> to subdivide patch series into smaller self-contained sub-series).
>>
>> On the other hand, it would be nice to have some roadmap about series
>> and features to be sent in the future, if possible.  Something like what
>> was done when 'git rebase --interactive' was getting builtinified: moved
>> (in parts) to C.  It would be great to have such roadmap with milestones
>> achieved and milestones to be done in the cover letter for series.
>
> I suppose that is what I intended in the "Future Work" section of
> Documentation/technical/commit-graph.txt. It gives a set of things
> that need to be done in order to make this a default feature, not just
> an expert-level feature. When I wrote the section, I was focused
> entirely on "commit-graph version 1.0" and I didn't know how long that
> would take. The series have been getting interest and review (in great
> part to your interest, Jakub) so they have been moving to 'next'
> faster than I anticipated.
>
> I'll plan on writing a more detailed list of future directions, but
> for now I'll list the things I know about and how I see them in
> priority order:
>
> Commit-graph v1.0:
> * ds/generation-numbers
> * 'verify' and fsck/gc integration
> * correct behavior with shallow clones, replace-objects, and grafts

So the goal of current v1.0 phase is to introduce generation numbers.
use them for better performance ("low hanging fruit"), ensure that it is
automatic and safe -- thus useable for an ordinary user.

>
> Commit-graph v1.1:
> * Place commit-graph storage in the_repository
> * 'git tag --merged' use generation numbers
> * 'git log --graph' use generation numbers
>
> Commit-graph v1.X:
> * Protocol v2 verb for sending commit-graph
> * Bloom filters for changed paths

Thanks, that is what I was missing.  The "Future Work" section, while
very nice to have (because it does not require to follow git
development; it is here in technical documentation), lacked
prioritization and rough milestones map.

[...]
>>> The tougher challenge is `git log --graph`. The revision walk
>>> machinery currently uses two precompute phases before iterating
>>> results to the pager: limit_list() and sort_in_topological_order();
>>> these correspond to two phases of Kahn's algorithm for topo-sort
>>> (compute in-degrees, then walk by peeling commits with in-degree
>>> zero). This requires O(N) time, where N is the number of reachable
>>> commits. Instead, we could make this be O(W) time to output one page
>>> of results, where W is (roughly) the number of reachable commits with
>>> generation number above the last reported result.
>>
>> A reminder: Kahn's algorithm (described for example in [1] and [3])
>> looks like this:
>>
>>L ← Empty list that will contain the sorted elements
>>S ← Collection of all nodes with no incoming edge
>>while S is non-empty do
>>remove a node 'n' from S
>>add 'n' to tail of L
>>for each parent 'm' of 'n' do
>>decrease the in-degree of 'm'
>>if 'm' has in-degree of 0 then
>>insert 'm' into S
>>
>> [1]: https://en.wikipedia.org/wiki/Topological_sorting#Kahn's_algorithm
>> [2]: 
>> https://www.geeksforgeeks.org/topological-sorting-indegree-based-solution/
[...]

> I'm not following your pseudocode very well,

Not surprising, I've lost myself in the middle of writing

[PATCH 1/1] Inform about fast-forwarding of submodules during merge

2018-05-14 Thread Leif Middelschulte

From: Leif Middelschulte 

Inform the user about an automatically fast-forwarded submodule. The silent 
merge
behavior was introduced by commit 68d03e4a6e44 ("Implement automatic 
fast-forward
merge for submodules", 2010-07-07)).

Signed-off-by: Leif Middelschulte 
---
 merge-recursive.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/merge-recursive.c b/merge-recursive.c
index a4b91d17f..4a03044d1 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1093,10 +1093,14 @@ static int merge_submodule(struct merge_options *o,
/* Case #1: a is contained in b or vice versa */
if (in_merge_bases(commit_a, commit_b)) {
oidcpy(result, b);
+   output(o, 1, _("Note: Fast-forwarding submodule %s to the 
following commit"), path);
+   output_commit_title(o, commit_b);
return 1;
}
if (in_merge_bases(commit_b, commit_a)) {
oidcpy(result, a);
+   output(o, 1, _("Note: Fast-forwarding submodule %s to the 
following commit:"), path);
+   output_commit_title(o, commit_a);
return 1;
}
 
-- 
2.15.1 (Apple Git-101)

[PATCH 0/1] rebased: inform about auto submodule ff during merge

2018-05-14 Thread Leif Middelschulte

From: Leif Middelschulte 

This patch is in response to Stefan Beller's Commit 0357af480
("merge-recursive: i18n submodule merge output and respect verbosity",
2018-05-10) and is based on the changes it provided.

Leif Middelschulte (1):
  Inform about fast-forwarding of submodules during merge

 merge-recursive.c | 4 
 1 file changed, 4 insertions(+)

-- 
2.15.1 (Apple Git-101)

Re: Git push error due to hooks error

2018-05-14 Thread Kevin Daudt

On Mon, May 14, 2018 at 05:44:06PM +, Barodia, Anjali (Nokia - IN/Noida) 
wrote:
> Hi Support,
> 
> 
> I was trying to push local git to another git on gerrit, but stuck with an 
> hook error.
> This is a very large repo and while running the command "git push origin 
> --all"
> I am getting this errors:
> 
> remote: (W) 92e19d4: too many commit message lines longer than 70 characters; 
> manually wrap lines
> remote: (W) de2245b: too many commit message lines longer than 70 characters; 
> manually wrap lines
> remote: (W) dc6e982: too many commit message lines longer than 70 characters; 
> manually wrap lines
> remote: (W) d2e2efd: too many commit message lines longer than 70 characters; 
> manually wrap lines
> remote: error: internal error while processing changes To 
> ssh_url_path:8282/SI_VF.git
>  ! [remote rejected]   master -> master (Error running hook 
> /opt/gerrit/hooks/ref-update)
> error: failed to push some refs to 'ssh_user@url_path:8282/SI_VF.git'
> 
> I tried to push after deleting the .git/hooks, but still get the same error.
> Please help me in fixing this error.
> 
> Thanks with Regards,
> Anjali Barodia

Did you remove the hook from the remote repo, or the local repo? Because
this is a hook which runs on the repo you are pushing too. Something
like pre-receive or pre-update is causing this.

Kevin

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

2018-05-14 Thread Andreas Heiduk

Am 14.05.2018 um 19:26 schrieb Duy Nguyen:
> On Mon, May 14, 2018 at 7:03 PM, Andreas Heiduk  wrote:
>> Am 08.05.2018 um 17:24 schrieb Duy Nguyen:
>>> 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
 --[no-]ignore-other-worktrees  --[no-]quiet
 --[no-]ignore-skip-worktree-bits   --[no-]recurse-submodules
 --[no-]merge   --theirs
 --[no-]orphan= --[no-]track
 --ours

 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.
>>
>> The fish-shell does something like that.
>>
>> > git status --
>> --branch  (Show the branch and tracking info even in short-format)
>> --help   (Display the manual of a git command)
>> --ignore-submodules (Ignore changes to submodules)
>> --porcelain(Give the output in a stable, easy-to-parse format)
>> --short  (Give the output in the short-format)
>> --untracked-files  (The untracked files handling mode)
>>
>> Another tab will put a selection-cursor on the displayed list - you can
>> navigate that list with Cursor-Up/Cursor-Down, select an entry and that
>> entry will be inserted into the commandline. That selection process
>> would be useless if the options are presented as "--[no-]x" because THAT
>> cannot be inserted into the commandline without manual editing. And
>> that's the point of the fast option selection process.
> 
> Good to know.
> 
> BTW I looked at the git.fish completion script [1] and see that recent
> effort to help automate more in git-completion.bash might help there
> too. I notice a lot of options and help text hard coded there, if
> someone can explain to me how git.fish uses those, maybe I can change
> git to export something suitable for git.fish to use too [2].

I'm no expert, but some additional things required by fish (and I
suppose zsh too) but not by bash:

- grouping of long and short options
- help text
- argument types for options

Help text and long/short option grouping look like this:

> git rebase -
--force-rebase  -f(Force the rebase)
--merge  -m   (Use merging strategies to rebase)

All these infos seem to be available in `struct option` (for C stuff
at least). So I guess It would be easiest for Fish & Co if git just
exports the complete info in some stable format.

> 
> For example with latest git (in 'master') doing this
> 
> ./git add --git-completion-helper
> 
> gives you the list of all options of "git add". Giving the help text
> for each option is definitely possible (I just didn't see any use for
> it until I looked at zsh/fish completion scripts) and maybe more in
> the future.
> 
> [1] 
> https://github.com/fish-shell/fish-shell/blob/master/share/completions/git.fish
> [2] But then if your script has to work with old git versions too then
> this is a moot point.

Well, sooner or later those old git versions might not be supported by
those shells exactly due to the involved maintenance overhead. So
providing some helper is a step in the right direction. Not providing
only fossilizes the current state.

[PATCH] commit: fix sparse 'not declared' warning

2018-05-14 Thread Ramsay Jones


Signed-off-by: Ramsay Jones 
---

Hi Duy,

If you need to re-roll your 'nd/commit-util-to-slab' branch, could
you please squash this into the relevant patch (commit 37de2f0a93,
"merge: use commit-slab in merge remote desc instead of commit->util",
2018-05-13).

Also, my 'static-check.pl' script has pinged on this series due
to the use of the 'implement_shared_commit_slab' macro in revision.c
(commit 85d055e876, "revision.c: use commit-slab for show_source",
2018-05-13).

This is because that macro only allows you to set the 'scope' of the
complete set of functions that are declared there - in this case three
of them are not called (well, two are actually called by another two
functions in the set) outside of revision.c.

In particular, the functions 'clear_revision_sources',
'init_revision_sources_with_stride' (used in init_##slabname) and
'revision_sources_at_peek' (used in slabname##_at).

I haven't thought too deeply about this issue, or if it even
matters, so I have no solutions to offer. ;-)

Thanks!

ATB,
Ramsay Jones

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

diff --git a/commit.c b/commit.c
index 38a3c0e5e..c0582df16 100644
--- a/commit.c
+++ b/commit.c
@@ -1657,7 +1657,7 @@ int commit_tree_extended(const char *msg, size_t msg_len,
 }
 
 define_commit_slab(merge_desc_slab, struct merge_remote_desc *);
-struct merge_desc_slab merge_desc_slab = COMMIT_SLAB_INIT(1, merge_desc_slab);
+static struct merge_desc_slab merge_desc_slab = COMMIT_SLAB_INIT(1, 
merge_desc_slab);
 
 struct merge_remote_desc *merge_remote_util(struct commit *commit)
 {
-- 
2.17.0

[GSoC] Week 2 - 'git stash' blog

2018-05-14 Thread Paul-Sebastian Ungureanu


Hello world,

A new week has begun, but how was the last one? I posted a new blog post 
about it [1].


Any feedback is greatly appreciated! Thank you!

[1]
https://ungps.github.io/

Best regards,
Paul

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

On 05/12, Antonio Ospite wrote:
> On Wed, 9 May 2018 10:19:50 -0700
> Brandon Williams  wrote:
> 
> > 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 :)
> > 
> 
> Should I resend the patch to gits...@pobox.com with your Acked-by?

This has my Reviewed-by: Brandon Williams 
though you don't need to resend the patch to Junio, he has normally
taken care of that :)

> 
> Thanks,
>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?

-- 
Brandon Williams

Re: [RFC PATCH 01/10] config: make config_from_gitmodules generally useful

On 05/14, Antonio Ospite wrote:
> The config_from_gitmodules() function is a good candidate for
> a centralized point where to read the gitmodules configuration file.
> 
> Add a repo argument to it to make the function more general, and adjust
> the current callers in cmd_fetch and update-clone.
> 
> As a proof of the utility of the change, start using the function also
> in repo_read_gitmodules which was basically doing the same operations.
> 
> Signed-off-by: Antonio Ospite 
> ---
>  builtin/fetch.c |  2 +-
>  builtin/submodule--helper.c |  2 +-
>  config.c| 13 +++--
>  config.h| 10 +-
>  submodule-config.c  | 16 
>  5 files changed, 14 insertions(+), 29 deletions(-)
> 
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 7ee83ac0f..a67ee7c39 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1445,7 +1445,7 @@ int cmd_fetch(int argc, const char **argv, const char 
> *prefix)
>   for (i = 1; i < argc; i++)
>   strbuf_addf(_rla, " %s", argv[i]);
>  
> - config_from_gitmodules(gitmodules_fetch_config, NULL);
> + config_from_gitmodules(gitmodules_fetch_config, the_repository, NULL);
>   git_config(git_fetch_config, NULL);
>  
>   argc = parse_options(argc, argv, prefix,
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index c2403a915..9e8f2acd5 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1602,7 +1602,7 @@ static int update_clone(int argc, const char **argv, 
> const char *prefix)
>   };
>   suc.prefix = prefix;
>  
> - config_from_gitmodules(gitmodules_update_clone_config, _jobs);
> + config_from_gitmodules(gitmodules_update_clone_config, the_repository, 
> _jobs);
>   git_config(gitmodules_update_clone_config, _jobs);
>  
>   argc = parse_options(argc, argv, prefix, module_update_clone_options,
> diff --git a/config.c b/config.c
> index 6f8f1d8c1..8ffe29330 100644
> --- a/config.c
> +++ b/config.c
> @@ -2173,17 +2173,18 @@ int git_config_get_pathname(const char *key, const 
> char **dest)
>  }
>  
>  /*
> - * Note: This function exists solely to maintain backward compatibility with
> - * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and 
> should
> - * NOT be used anywhere else.
> + * Note: Initially this function existed solely to maintain backward
> + * compatibility with 'fetch' and 'update_clone' storing configuration in
> + * '.gitmodules' but it turns out it can be useful as a centralized point to
> + * read the gitmodules config file.

I'm all for what you're trying to accomplish in this patch series but I
think a little more care needs to be taken here.  Maybe about a year ago
I did some refactoring with how the gitmodules file was loaded and it
was decided that allowing arbitrary configuration in the .gitmodules
file was a bad thing, so I tried to make sure that it was very difficult
to sneak in more of that and limiting it to the places where it was
already done (fetch and update_clone).  Now this patch is explicitly
changing the comment on this function to loosen the requirements I made
when it was introduced, which could be problematic in the future.

So here's what I suggest doing:
  1. Move this function from config.{c,h} to submodule-config.{c,h} to
 make it clear who owns this.
  2. Move the gitmodules_set function you created to live in submodule-config.
  3. You can still use this function as the main driver for reading the
 submodule config BUT the comment above the function in both the .c and
 .h files should be adapted so that it is clearly spells out that the
 function is to be used only by the submodule config code (reading it
 in repo_read_gitmodules and reading/writing it in the
 submodule-helper config function you've added) and that the only
 exceptions to this are to maintain backwards compatibility with the
 existing callers and that new callers shouldn't be added.

This is just a long way to say that if new callers to this function are
added in the future that it is made very clear in the code that the
.gitmodules file exists for a specific purpose and that it shouldn't be
exploited to ship config with a repository. (If we end up allowing
config to be shipped with a repository at a later date that will need to
be handled with great care due to security concerns).

Thanks for working on this, the end result is definitely a step in the
direction I've wanted the submodule config to head to.

>   *
>   * Runs the provided config function on the '.gitmodules' file found in the
>   * working directory.
>   */
> -void config_from_gitmodules(config_fn_t fn, void *data)
> +void config_from_gitmodules(config_fn_t fn, struct repository *repo, void 
> *data)
>  {
> - if (the_repository->worktree) {
> - char *file = repo_worktree_path(the_repository, 
> GITMODULES_FILE);
> + if

Re: [PATCH v2 14/14] commit.h: delete 'util' field in struct commit


On 5/14/2018 1:30 PM, Duy Nguyen wrote:

On Mon, May 14, 2018 at 6:07 PM, Duy Nguyen  wrote:

On Mon, May 14, 2018 at 04:52:29PM +0900, Junio C Hamano wrote:

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


diff --git a/commit.h b/commit.h
index 838f6a6b26..70371e111e 100644
--- a/commit.h
+++ b/commit.h
@@ -18,12 +18,16 @@ struct commit_list {

  struct commit {
 struct object object;
-   void *util;
 unsigned int index;
 timestamp_t date;
 struct commit_list *parents;
 struct tree *tree;
 uint32_t graph_pos;
+   /*
+* Do not add more fields here unless it's _very_ often
+* used. Use commit-slab to associate more data with a commit
+* instead.
+*/
  };

That's a logical consequence and a natural endgame of this
pleasent-to-read series.  THanks.

Unfortunately we are gaining more and more stuff in "struct commit"
with recent topics, and we may want to see if we can evict some of
them out to shrink it again.

Sigh.. ds/lazy-load-trees already enters 'next' so a fixup patch is
something like this.

Sorry I take this patch back. I didn't realize it was a rename and the
old field named 'tree' was already there (I vaguely recalled some
"tree" in this struct but didn't stop to think about it). Moving
graph_pos out is an option, but only 32-bit arch gains from it (64-bit
arch will have 4 bytes padding anyway) so probably not worth the
effort. "generation" field should probably be moved out though.


I'm happy to take a look at this patch and figure out the best way to 
integrate it with the changes I've been doing.


I disagree with the removal of "generation". My intention is to make the 
commit-graph feature a standard feature that most repos (of reasonable 
size) want to have. In that case, 'graph_pos' and 'generation' will be 
set during every parse and used by every commit walk. This is just my 
gut reaction.


As I investigate these changes, I'll try to see what performance hit is 
caused by converting the graph_pos and/or generation to commit slabs. 
(Again, I'm assuming the slabs will make this slower. I may be wrong here.)


Thanks,
-Stolee

Re: git diff: meaning of ^M at line ends ?

2018-05-14 Thread Torsten Bögershausen

On 14.05.18 18:08, Frank Schäfer wrote:
> What does ^M at the end of lines in the output of 'git diff' mean ?
> 
> Thanks,
> Frank
> 

^M is the representation of a "Carriage Return" or CR.
Under Linux/Unix/Mac OS X a line is terminated with a single
"line feed", LF.

Windows typically uses CRLF at the end of the line.
"git diff" uses the LF to detect the end of line,
leaving the CR alone.

Nothing to worry about.

If you want, you can commit those files with
CRLF in the working tree, and LF in the repo.

More information may be found here:

https://git-scm.com/docs/gitattributes

(Or ask more questions here, if needed)

Git push error due to hooks error

2018-05-14 Thread Barodia, Anjali (Nokia - IN/Noida)

Hi Support,


I was trying to push local git to another git on gerrit, but stuck with an hook 
error.
This is a very large repo and while running the command "git push origin --all"
I am getting this errors:

remote: (W) 92e19d4: too many commit message lines longer than 70 characters; 
manually wrap lines
remote: (W) de2245b: too many commit message lines longer than 70 characters; 
manually wrap lines
remote: (W) dc6e982: too many commit message lines longer than 70 characters; 
manually wrap lines
remote: (W) d2e2efd: too many commit message lines longer than 70 characters; 
manually wrap lines
remote: error: internal error while processing changes To 
ssh_url_path:8282/SI_VF.git
 ! [remote rejected]   master -> master (Error running hook 
/opt/gerrit/hooks/ref-update)
error: failed to push some refs to 'ssh_user@url_path:8282/SI_VF.git'

I tried to push after deleting the .git/hooks, but still get the same error.
Please help me in fixing this error.

Thanks with Regards,
Anjali Barodia

Re: [PATCH] t7005-editor: get rid of the SPACES_IN_FILENAMES prereq

2018-05-14 Thread Eric Sunshine

On Mon, May 14, 2018 at 6:28 AM, SZEDER Gábor  wrote:
> The last two tests 'editor with a space' and 'core.editor with a
> space' in 't7005-editor.sh' need the SPACES_IN_FILENAMES prereq to
> ensure that they are only run on filesystems that allow, well, spaces
> in filesnames.  However, we have been putting a space in the name of

s filesnames/filenames/

> the trash directory for just over a decade now, so we wouldn't be able
> to run any of our tests on such a filesystem in the first place.
>
> This prereq is therefore unnecessary, remove it.
>
> Signed-off-by: SZEDER Gábor

Re: Git push error due to hooks error

2018-05-14 Thread Martin Fick

On Monday, May 14, 2018 05:32:35 PM Barodia, Anjali wrote:
> I was trying to push local git to another git on gerrit,
> but stuck with an hook error. This is a very large repo
> and while running the command "git push origin --all" I
> am getting this errors:
> 
> remote: (W) 92e19d4: too many commit message lines longer
> than 70 characters; manually wrap lines remote: (W)
> de2245b: too many commit message lines longer than 70
> characters; manually wrap lines remote: (W) dc6e982: too
> many commit message lines longer than 70 characters;
> manually wrap lines remote: (W) d2e2efd: too many commit
> message lines longer than 70 characters; manually wrap
> lines remote: error: internal error while processing
> changes To ssh_url_path:8282/SI_VF.git
>  ! [remote rejected]   master -> master (Error running
> hook /opt/gerrit/hooks/ref-update) error: failed to
> push some refs to 'ssh_user@url_path:8282/SI_VF.git'


This is standard Gerrit behavior.  For Gerrit questions, 
please post question to:

Repo and Gerrit Discussion 

-Martin

-- 
The Qualcomm Innovation Center, Inc. is a member of Code 
Aurora Forum, hosted by The Linux Foundation

Git push error due to hooks error

2018-05-14 Thread Barodia, Anjali (Nokia - IN/Noida)

Hi Support,


I was trying to push local git to another git on gerrit, but stuck with an hook 
error.
This is a very large repo and while running the command "git push origin --all"
I am getting this errors:

remote: (W) 92e19d4: too many commit message lines longer than 70 characters; 
manually wrap lines
remote: (W) de2245b: too many commit message lines longer than 70 characters; 
manually wrap lines
remote: (W) dc6e982: too many commit message lines longer than 70 characters; 
manually wrap lines
remote: (W) d2e2efd: too many commit message lines longer than 70 characters; 
manually wrap lines
remote: error: internal error while processing changes
To ssh_url_path:8282/SI_VF.git
 ! [remote rejected]   master -> master (Error running hook 
/opt/gerrit/hooks/ref-update)
error: failed to push some refs to 'ssh_user@url_path:8282/SI_VF.git'

I tried to push after deleting the .git/hooks, but still get the same error.
Please help me in fixing this error.

Thanks with Regards,
Anjali Barodia

Re: [PATCH v2 14/14] commit.h: delete 'util' field in struct commit

On Mon, May 14, 2018 at 6:07 PM, Duy Nguyen  wrote:
> On Mon, May 14, 2018 at 04:52:29PM +0900, Junio C Hamano wrote:
>> Nguyễn Thái Ngọc Duy   writes:
>>
>> > diff --git a/commit.h b/commit.h
>> > index 838f6a6b26..70371e111e 100644
>> > --- a/commit.h
>> > +++ b/commit.h
>> > @@ -18,12 +18,16 @@ struct commit_list {
>> >
>> >  struct commit {
>> > struct object object;
>> > -   void *util;
>> > unsigned int index;
>> > timestamp_t date;
>> > struct commit_list *parents;
>> > struct tree *tree;
>> > uint32_t graph_pos;
>> > +   /*
>> > +* Do not add more fields here unless it's _very_ often
>> > +* used. Use commit-slab to associate more data with a commit
>> > +* instead.
>> > +*/
>> >  };
>>
>> That's a logical consequence and a natural endgame of this
>> pleasent-to-read series.  THanks.
>>
>> Unfortunately we are gaining more and more stuff in "struct commit"
>> with recent topics, and we may want to see if we can evict some of
>> them out to shrink it again.
>
> Sigh.. ds/lazy-load-trees already enters 'next' so a fixup patch is
> something like this.

Sorry I take this patch back. I didn't realize it was a rename and the
old field named 'tree' was already there (I vaguely recalled some
"tree" in this struct but didn't stop to think about it). Moving
graph_pos out is an option, but only 32-bit arch gains from it (64-bit
arch will have 4 bytes padding anyway) so probably not worth the
effort. "generation" field should probably be moved out though.
-- 
Duy

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

On Mon, May 14, 2018 at 7:03 PM, Andreas Heiduk  wrote:
> Am 08.05.2018 um 17:24 schrieb Duy Nguyen:
>> 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
>>> --[no-]ignore-other-worktrees  --[no-]quiet
>>> --[no-]ignore-skip-worktree-bits   --[no-]recurse-submodules
>>> --[no-]merge   --theirs
>>> --[no-]orphan= --[no-]track
>>> --ours
>>>
>>> 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.
>
> The fish-shell does something like that.
>
> > git status --
> --branch  (Show the branch and tracking info even in short-format)
> --help   (Display the manual of a git command)
> --ignore-submodules (Ignore changes to submodules)
> --porcelain(Give the output in a stable, easy-to-parse format)
> --short  (Give the output in the short-format)
> --untracked-files  (The untracked files handling mode)
>
> Another tab will put a selection-cursor on the displayed list - you can
> navigate that list with Cursor-Up/Cursor-Down, select an entry and that
> entry will be inserted into the commandline. That selection process
> would be useless if the options are presented as "--[no-]x" because THAT
> cannot be inserted into the commandline without manual editing. And
> that's the point of the fast option selection process.

Good to know.

BTW I looked at the git.fish completion script [1] and see that recent
effort to help automate more in git-completion.bash might help there
too. I notice a lot of options and help text hard coded there, if
someone can explain to me how git.fish uses those, maybe I can change
git to export something suitable for git.fish to use too [2].

For example with latest git (in 'master') doing this

./git add --git-completion-helper

gives you the list of all options of "git add". Giving the help text
for each option is definitely possible (I just didn't see any use for
it until I looked at zsh/fish completion scripts) and maybe more in
the future.

[1] 
https://github.com/fish-shell/fish-shell/blob/master/share/completions/git.fish
[2] But then if your script has to work with old git versions too then
this is a moot point.
-- 
Duy

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

On Wed, May 9, 2018 at 5:20 AM, Aaron Schrab  wrote:
> At 17:24 +0200 08 May 2018, Duy Nguyen  wrote:
>>
>> 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.
>
>
> Was that perhaps using zsh rather than bash? Below is some of the display
> from its git completion (this is likely affected somewhat by my
> configuration).  The group descriptions (lines that begin with "Completing")
> appear in a different color, and are not available for selection.

Ah. That's probably it.

>
> 1113$ git c
> Completing alias
> ci   -- alias for 'commit -v'
> cia  -- alias for 'commit -v -a'
> co   -- alias for 'checkout'
> conf -- alias for 'config'
> Completing main porcelain command
> checkout -- checkout branch or paths to working tree
> cherry-pick  -- apply changes introduced by some existing commits
> citool   -- graphical alternative to git commit
> clean-- remove untracked files from working tree
> clone-- clone repository into new directory
> commit   -- record changes to repository
> Completing ancillary manipulator command
> config   -- get and set repository or global options
> Completing ancillary interrogator command
> cherry   -- find commits not merged upstream
> count-objects-- count unpacked objects and display their disk
> consumption
> Completing plumbing manipulator command
> checkout-index   -- copy files from index to working directory
> commit-tree  -- create new commit object
> Completing plumbing interrogator command
> cat-file -- provide content or type information for repository
> objects

It's interesting that zsh could do this. I looked at the script and
these texts are hard coded in there. I don't use zsh myself and won't
be doing this, but this information should be now available from git
binary so you can lower maintenance cost for the zsh completion
script.

>
> 1114$ git commit -
> Completing option
> --all  -a   -- stage all modified and deleted paths
> --allow-empty   -- allow recording an empty commit
> --allow-empty-message   -- allow recording a commit with an empty
> message
> --amend -- amend the tip of the current branch
> --author-- override the author name used in the
> commit

Hm.. no idea where this is from. Maybe zsh can extract "git 
-h"? Anyway it does not matter.
-- 
Duy

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

2018-05-14 Thread Andreas Heiduk

Am 08.05.2018 um 17:24 schrieb Duy Nguyen:
> 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
>> --[no-]ignore-other-worktrees  --[no-]quiet
>> --[no-]ignore-skip-worktree-bits   --[no-]recurse-submodules
>> --[no-]merge   --theirs
>> --[no-]orphan= --[no-]track
>> --ours
>>
>> 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.

The fish-shell does something like that.

> git status --
--branch  (Show the branch and tracking info even in short-format)
--help   (Display the manual of a git command)
--ignore-submodules (Ignore changes to submodules)
--porcelain(Give the output in a stable, easy-to-parse format)
--short  (Give the output in the short-format)
--untracked-files  (The untracked files handling mode)

Another tab will put a selection-cursor on the displayed list - you can
navigate that list with Cursor-Up/Cursor-Down, select an entry and that
entry will be inserted into the commandline. That selection process
would be useless if the options are presented as "--[no-]x" because THAT
cannot be inserted into the commandline without manual editing. And
that's the point of the fast option selection process.

> 
>> 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.
> 
> 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.*
>

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

On Mon, May 14, 2018 at 5:33 AM, Eric Sunshine  wrote:
> 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.

Oh good I was thinking --no-... too or we could even do "--no- (press
tab for more)" or something to make it more obvious. As long as we
make sure there's another --no-option somewhere, then we will only
complete the --no- part and can replace the "..." (or "press tab for
more") with real candidates in the next tab/
--
Duy

Re: [PATCH v2 14/14] commit.h: delete 'util' field in struct commit

On Mon, May 14, 2018 at 04:52:29PM +0900, Junio C Hamano wrote:
> Nguyễn Thái Ngọc Duy   writes:
> 
> > diff --git a/commit.h b/commit.h
> > index 838f6a6b26..70371e111e 100644
> > --- a/commit.h
> > +++ b/commit.h
> > @@ -18,12 +18,16 @@ struct commit_list {
> >  
> >  struct commit {
> > struct object object;
> > -   void *util;
> > unsigned int index;
> > timestamp_t date;
> > struct commit_list *parents;
> > struct tree *tree;
> > uint32_t graph_pos;
> > +   /*
> > +* Do not add more fields here unless it's _very_ often
> > +* used. Use commit-slab to associate more data with a commit
> > +* instead.
> > +*/
> >  };
> 
> That's a logical consequence and a natural endgame of this
> pleasent-to-read series.  THanks.
> 
> Unfortunately we are gaining more and more stuff in "struct commit"
> with recent topics, and we may want to see if we can evict some of
> them out to shrink it again.

Sigh.. ds/lazy-load-trees already enters 'next' so a fixup patch is
something like this.

Derrick, do you want to finish up this patch? I could do that by
myself, but I guess this could be good for you to get familiar with
commit-slab because ds/generation-numbers on 'pu' also adds a new
field in struct commit.

We should avoid adding more stuff in struct commit to reduce overhead
when these fields are not used (and I'm not sure how often generation
numbers are used to justify its place here). One clean way to do it is
with commit-slab, as demonstrated here for 'maybe_tree' field.

Anyway the patch is like this

-- 8< --
diff --git a/commit-graph.c b/commit-graph.c
index 70fa1b25fd..9d084f6200 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -9,6 +9,7 @@
 #include "revision.h"
 #include "sha1-lookup.h"
 #include "commit-graph.h"
+#include "commit-slab.h"
 
 #define GRAPH_SIGNATURE 0x43475048 /* "CGPH" */
 #define GRAPH_CHUNKID_OIDFANOUT 0x4f494446 /* "OIDF" */
@@ -38,6 +39,26 @@
 #define GRAPH_MIN_SIZE (5 * GRAPH_CHUNKLOOKUP_WIDTH + GRAPH_FANOUT_SIZE + \
GRAPH_OID_LEN + 8)
 
+
+/*
+ * If the commit is loaded from the commit-graph file, then this
+ * member may be NULL. Only access it through get_commit_tree()
+ * or get_commit_tree_oid().
+ */
+define_commit_slab(maybe_tree, struct tree *);
+struct maybe_tree maybe_trees = COMMIT_SLAB_INIT(1, maybe_trees);
+
+struct tree *set_maybe_tree(const struct commit *commit, struct tree *tree)
+{
+   *maybe_tree_at(_trees, commit) = tree;
+   return tree;
+}
+
+struct tree *get_maybe_tree(const struct commit *commit)
+{
+   return *maybe_tree_at(_trees, commit);
+}
+
 char *get_commit_graph_filename(const char *obj_dir)
 {
return xstrfmt("%s/info/commit-graph", obj_dir);
@@ -256,7 +277,7 @@ static int fill_commit_in_graph(struct commit *item, struct 
commit_graph *g, uin
item->object.parsed = 1;
item->graph_pos = pos;
 
-   item->maybe_tree = NULL;
+   set_maybe_tree(item, NULL);
 
date_high = get_be32(commit_data + g->hash_len + 8) & 0x3;
date_low = get_be32(commit_data + g->hash_len + 12);
@@ -322,15 +343,15 @@ static struct tree *load_tree_for_commit(struct 
commit_graph *g, struct commit *
   GRAPH_DATA_WIDTH * (c->graph_pos);
 
hashcpy(oid.hash, commit_data);
-   c->maybe_tree = lookup_tree();
-
-   return c->maybe_tree;
+   return set_maybe_tree(c, lookup_tree());
 }
 
 struct tree *get_commit_tree_in_graph(const struct commit *c)
 {
-   if (c->maybe_tree)
-   return c->maybe_tree;
+   struct tree *maybe_tree = get_maybe_tree(c);
+
+   if (maybe_tree)
+   return maybe_tree;
if (c->graph_pos == COMMIT_NOT_FROM_GRAPH)
BUG("get_commit_tree_in_graph called from non-commit-graph 
commit");
 
diff --git a/commit-graph.h b/commit-graph.h
index 260a468e73..4a02e4b05e 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -45,4 +45,7 @@ void write_commit_graph(const char *obj_dir,
int nr_commits,
int append);
 
+struct tree *set_maybe_tree(const struct commit *commit, struct tree *tree);
+struct tree *get_maybe_tree(const struct commit *commit);
+
 #endif
diff --git a/commit.c b/commit.c
index 711f674c18..45521ab2de 100644
--- a/commit.c
+++ b/commit.c
@@ -298,8 +298,10 @@ void free_commit_buffer(struct commit *commit)
 
 struct tree *get_commit_tree(const struct commit *commit)
 {
-   if (commit->maybe_tree || !commit->object.parsed)
-   return commit->maybe_tree;
+   struct tree *maybe_tree = get_maybe_tree(commit);
+
+   if (maybe_tree || !commit->object.parsed)
+   return maybe_tree;
 
if (commit->graph_pos == COMMIT_NOT_FROM_GRAPH)
BUG("commit has NULL tree, but was not loaded from 
commit-graph");
@@ -351,7 +353,7 @@ int parse_commit_buffer(struct commit *item, const void 
*buffer, unsigned long s
if

git diff: meaning of ^M at line ends ?

2018-05-14 Thread Frank Schäfer

What does ^M at the end of lines in the output of 'git diff' mean ?

Thanks,
Frank

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

On Mon, May 14, 2018 at 04:52:53PM +0200, Duy Nguyen wrote:
> On Sun, May 13, 2018 at 11:02 PM, Kevin Daudt  wrote:
> > 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.
> 
> Now this is something we should fix. When an argument can be
> interpreted in more than one way Git should reject it, but I think
> this ambiguation logic does not take dwim (i.e. create a new branch
> beased on remote) into account.

This is a quick draft to make this happen

-- 8< --
diff --git a/builtin/checkout.c b/builtin/checkout.c
index b49b582071..f4f6951f05 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -953,8 +953,19 @@ static int parse_branchname_arg(int argc, const char 
**argv,
 */
int recover_with_dwim = dwim_new_local_branch_ok;
 
-   if (!has_dash_dash &&
-   (check_filename(opts->prefix, arg) || !no_wildcard(arg)))
+   if (!has_dash_dash && check_filename(opts->prefix, arg) &&
+   recover_with_dwim) {
+   const char *remote = unique_tracking_name(arg, rev);
+   if (remote)
+   die(_("don't know whether to create a tracking "
+ "branch from remote %s or to checkout 
path %s, "
+ "use -- to disambiguate"),
+   remote, arg);
+   printf("here?\n");
+   recover_with_dwim = 0;
+   }
+
+   if (!has_dash_dash && !no_wildcard(arg))
recover_with_dwim = 0;
/*
 * Accept "git checkout foo" and "git checkout foo --"
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index 3e5ac81bd2..ea95fb8668 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -215,4 +215,35 @@ test_expect_success 'loosely defined local base branch is 
reported correctly' '
test_cmp expect actual
 '
 
+test_expect_success 'reject when arg could be part of dwim branch' '
+   git remote add foo file://non-existent-place &&
+   git update-ref refs/remotes/foo/dwim-arg HEAD &&
+   echo foo >dwim-arg &&
+   git add dwim-arg &&
+   echo bar >dwim-arg &&
+   test_must_fail git checkout dwim-arg &&
+   test_must_fail git rev-parse refs/heads/dwim-arg -- &&
+   grep bar dwim-arg
+'
+
+test_expect_success 'disambiguate dwim branch and checkout path (1)' '
+   git update-ref refs/remotes/foo/dwim-arg1 HEAD &&
+   echo foo >dwim-arg1 &&
+   git add dwim-arg1 &&
+   echo bar >dwim-arg1 &&
+   git checkout -- dwim-arg1 &&
+   test_must_fail git rev-parse refs/heads/dwim-arg1 -- &&
+   grep foo dwim-arg1
+'
+
+test_expect_success 'disambiguate dwim branch and checkout path (2)' '
+   git update-ref refs/remotes/foo/dwim-arg2 HEAD &&
+   echo foo >dwim-arg2 &&
+   git add dwim-arg2 &&
+   echo bar >dwim-arg2 &&
+   git checkout dwim-arg2 -- &&
+   git rev-parse refs/heads/dwim-arg2 -- &&
+   grep bar dwim-arg2
+'
+
 test_done
-- 8< --

благодаря

2018-05-14 Thread Michelle Goodman

Здравствуйте, дорогой, мне нужен ваш срочный ответ. У меня есть
хорошая новость для вас.
Мишель
благодаря

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

On Sun, May 13, 2018 at 11:02 PM, Kevin Daudt  wrote:
> 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.

Now this is something we should fix. When an argument can be
interpreted in more than one way Git should reject it, but I think
this ambiguation logic does not take dwim (i.e. create a new branch
beased on remote) into account.
-- 
Duy

Re: [PATCH 4/4] mark_parents_uninteresting(): avoid most allocation

2018-05-14 Thread Jeff King

On Mon, May 14, 2018 at 09:25:46AM -0400, Derrick Stolee wrote:

> > I think you'd want to go the other way: this is marking uninteresting
> > commits, so you'd want origin/master..master, which would make those 70k
> > commits uninteresting.
> 
> Thanks for the tip. Running 'git rev-list origin/master..master' 100 times
> gave the following times, on average (with ds/generation-numbers):
> 
> PATCH 3/4: 0.19 s
> PATCH 4/4: 0.16 s
>     Rel %: -16%

Nifty. I liked it as just a cleanup, but I'm happier still to find that
it makes things faster (even if it's pretty small in absolute numbers).

-Peff

Re: [PATCH v2 08/12] commit-graph: verify commit contents against odb


On 5/12/2018 5:17 PM, Martin Ågren wrote:

On 11 May 2018 at 23:15, Derrick Stolee  wrote:

When running 'git commit-graph verify', compare the contents of the
commits that are loaded from the commit-graph file with commits that are
loaded directly from the object database. This includes checking the
root tree object ID, commit date, and parents.

Parse the commit from the graph during the initial loop through the
object IDs to guarantee we parse from the commit-graph file.

In addition, verify the generation number calculation is correct for all
commits in the commit-graph file.

While testing, we discovered that mutating the integer value for a
parent to be outside the accepted range causes a segmentation fault. Add
a new check in insert_parent_or_die() that prevents this fault. Check
for that error during the test, both in the typical parents and in the
list of parents for octopus merges.

This paragraph and the corresponding fix and test feel like a separate
patch to me. (The commit message of it could be "To test the next patch,
we threw invalid data at `git commit-graph verify, and it crashed in
pre-existing code, so let's fix that first" -- there is definitely a
connection.) Is this important enough to fast-track to master in time
for 2.18? My guess would be no.


+
+   if (pos >= g->num_commits)
+   die("invalide parent position %"PRIu64, pos);

s/invalide/invalid/


@@ -875,6 +879,8 @@ int verify_commit_graph(struct commit_graph *g)
 return 1;

 for (i = 0; i < g->num_commits; i++) {
+   struct commit *graph_commit;
+
 hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);

 if (i && oidcmp(_oid, _oid) >= 0)
@@ -892,6 +898,10 @@ int verify_commit_graph(struct commit_graph *g)

 cur_fanout_pos++;
 }
+
+   graph_commit = lookup_commit(_oid);
+   if (!parse_commit_in_graph_one(g, graph_commit))
+   graph_report("failed to parse %s from commit-graph", 
oid_to_hex(_oid));
 }

Could this end up giving ridiculous amounts of output? It would depend
on the input, I guess.


 while (cur_fanout_pos < 256) {
@@ -904,5 +914,95 @@ int verify_commit_graph(struct commit_graph *g)
 cur_fanout_pos++;
 }

+   if (verify_commit_graph_error)
+   return 1;

Well, here we give up before running into *too* much problem.


+   for (i = 0; i < g->num_commits; i++) {
+   struct commit *graph_commit, *odb_commit;
+   struct commit_list *graph_parents, *odb_parents;
+   int num_parents = 0;
+
+   hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);
+
+   graph_commit = lookup_commit(_oid);
+   odb_commit = (struct commit *)create_object(cur_oid.hash, 
alloc_commit_node());
+   if (parse_commit_internal(odb_commit, 0, 0)) {
+   graph_report("failed to parse %s from object database", 
oid_to_hex(_oid));
+   continue;
+   }
+
+   if (oidcmp(_commit_tree_in_graph_one(g, 
graph_commit)->object.oid,
+  get_commit_tree_oid(odb_commit)))
+   graph_report("root tree object ID for commit %s in 
commit-graph is %s != %s",
+oid_to_hex(_oid),
+
oid_to_hex(get_commit_tree_oid(graph_commit)),
+
oid_to_hex(get_commit_tree_oid(odb_commit)));
+
+   if (graph_commit->date != odb_commit->date)
+   graph_report("commit date for commit %s in commit-graph is %"PRItime" 
!= %"PRItime"",
+oid_to_hex(_oid),
+graph_commit->date,
+odb_commit->date);
+
+
+   graph_parents = graph_commit->parents;
+   odb_parents = odb_commit->parents;
+
+   while (graph_parents) {
+   num_parents++;
+
+   if (odb_parents == NULL)
+   graph_report("commit-graph parent list for commit %s 
is too long (%d)",
+oid_to_hex(_oid),
+num_parents);
+
+   if (oidcmp(_parents->item->object.oid, 
_parents->item->object.oid))
+   graph_report("commit-graph parent for %s is %s != 
%s",
+oid_to_hex(_oid),
+
oid_to_hex(_parents->item->object.oid),
+
oid_to_hex(_parents->item->object.oid));
+
+   graph_parents = graph_parents->next;
+   odb_parents = odb_parents->next;
+   }
+

Re: [PATCH v2 02/12] commit-graph: verify file header information


On 5/12/2018 9:35 AM, Martin Ågren wrote:

+static int verify_commit_graph_error;
+
+static void graph_report(const char *fmt, ...)
+{
+   va_list ap;
+   struct strbuf sb = STRBUF_INIT;
+   verify_commit_graph_error = 1;
+
+   va_start(ap, fmt);
+   strbuf_vaddf(, fmt, ap);
+
+   fprintf(stderr, "%s\n", sb.buf);
+   strbuf_release();
+   va_end(ap);
+}


That's a good idea. Makes that patch a bit less trivial and this one a 
bit less difficult.

Isten áldjon

2018-05-14 Thread Johanna Tuuk

Isten áldjon

Én vagyok Mrs.Johanna Tuuk, özvegy vagyok hosszú ideig tartó rákban. Az
állapotom minden jelzoje valóban romlik, és teljesen nyilvánvaló, hogy két
hónapot nem fogok élni orvosaim szerint, és minden jelzésben az orvosi elemzést
illetoen. Ez azért van, mert a rák nagyon rossz stádiumba került, hogy nincs
remény.

A kései férjem autóbalesetben halt meg, házasságunk idején nem tudtunk
gyermeket produkálni. Az édesapám nagyon gazdag volt, és halála után örököltem
minden üzletét és gazdagságát. Az orvosok azt tanácsolták nekem, hogy nem
élhetek több mint két hónapig, ezért most úgy döntöttem, hogy megosztom e
gazdagság egy részét, hogy hozzájáruljak a kevésbé kiváltságos Európában,
Amerikában, Afrikában és Ázsiában kialakult fejlodéshez, különösen a probléma
megoldásához a kevésbé privilegizált és az árvaház otthona.

Az interneten végzett keresésem után választottam, négy hétig imádkoztam és
böjtöltem rajta, hajlandó vagyok kilenc millió ötszázezer eurót adományozni a
kevésbé kiváltságosnak. Kérem, vegye figyelembe, hogy ez az alap bankban
fekszik és az én utasításom alapján benyújtja a pénz nevében történo átutalását.

Végül is oszintén imádkozom, hogy ezt az összeget átruházzák, még akkor is, ha
késo vagyok, mert meg kell tudnom, hogy Krisztus nélkül a vagyon megszerzése
hiábavaló, és Istennek ígéretet tettem, hogy az alap lesz használja a kevésbé
privilegizált segítséget. Kérjük, vegye figyelembe, hogy lefordítottam ezt a
levelet, hogy megértsétek, vissza tudsz írni engem

Várom a sürgos választ.

Isten áldjon.

Re: [PATCH v2 01/12] commit-graph: add 'verify' subcommand


On 5/12/2018 9:31 AM, Martin Ågren wrote:

On 11 May 2018 at 23:15, Derrick Stolee  wrote:


 graph_name = get_commit_graph_filename(opts.obj_dir);
 graph = load_commit_graph_one(graph_name);
+   FREE_AND_NULL(graph_name);

 if (!graph)
 die("graph file %s does not exist", graph_name);
-   FREE_AND_NULL(graph_name);

This is probably because of something I said, but this does not look
correct. The `die()` would typically print "(null)" or segfault. If the
`die()` means we don't free `graph_name`, that should be fine.

You're still leaking `graph` here (possibly nothing this patch should
worry about) and in `graph_verify()`. UNLEAK-ing it immediately before
calling `verify_commit_graph()` should be ok. I also think punting on
this UNLEAK-business entirely would be ok; I was just a bit surprised to
see one variable getting freed and the other one ignored.


Thanks, Martin. I was just blindly searching for FREE_AND_NULL() and 
shouldn't have been so careless.


-Stolee

Re: [PATCH 4/4] mark_parents_uninteresting(): avoid most allocation


On 5/14/2018 9:09 AM, Jeff King wrote:

On Mon, May 14, 2018 at 08:47:33AM -0400, Derrick Stolee wrote:


On 5/11/2018 2:03 PM, Jeff King wrote:

Commit 941ba8db57 (Eliminate recursion in setting/clearing
marks in commit list, 2012-01-14) used a clever double-loop
to avoid allocations for single-parent chains of history.
However, it did so only when following parents of parents
(which was an uncommon case), and _always_ incurred at least
one allocation to populate the list of pending parents in
the first place.

We can turn this into zero-allocation in the common case by
iterating directly over the initial parent list, and then
following up on any pending items we might have discovered.

This change appears to improve performance, but I was unable to measure any
difference between this commit and the one ahead, even when merging
ds/generation-numbers (which significantly reduces the other costs). I was
testing 'git status' and 'git rev-list --boundary master...origin/master' in
the Linux repo with my copy of master 70,000+ commits behind origin/master.

I think you'd want to go the other way: this is marking uninteresting
commits, so you'd want origin/master..master, which would make those 70k
commits uninteresting.


Thanks for the tip. Running 'git rev-list origin/master..master' 100 
times gave the following times, on average (with ds/generation-numbers):


PATCH 3/4: 0.19 s
PATCH 4/4: 0.16 s
    Rel %: -16%



I still doubt it will create that much difference, though. It's
more or less saving a single malloc per commit we traverse. Certainly
without the .commits file that's a drop in the bucket compared to
inflating the object data, but I wouldn't be surprised if we end up with
a few mallocs even after your patches.

Anyway, thanks for poking at it. :)

-Peff

Re: [RFC] Other chunks for commit-graph, part 1 - Bloom filters, topo order, etc.


On 5/12/2018 10:00 AM, Jakub Narebski wrote:

Derrick Stolee  writes:

On 5/4/2018 3:40 PM, Jakub Narebski wrote:

With early parts of commit-graph feature (ds/commit-graph and
ds/lazy-load-trees) close to being merged into "master", see
https://public-inbox.org/git/xmqq4ljtz87g@gitster-ct.c.googlers.com/
I think it would be good idea to think what other data could be added
there to make Git even faster.

Before thinking about adding more data to the commit-graph, I think
instead we need to finish taking advantage of the data that is already
there. This means landing the generation number patch [1] (I think
this is close, so I'll send a v6 this week if there is no new
feedback.) and the auto-compute patch [2] (this could use more
feedback, but I'll send a v1 based on the RFC feedback if no one
chimes in).

[1]
https://public-inbox.org/git/20180501124652.155781-1-dsto...@microsoft.com/
     [PATCH v5 00/11] Compute and consume generation numbers

[2]
https://public-inbox.org/git/20180417181028.198397-1-dsto...@microsoft.com/
     [RFC PATCH 00/12] Integrate commit-graph into 'fsck' and 'gc'

Right, so the RFC might be a bit premature; I wanted the discussion to
be out there to think about when adding new uses of existing features.


DIGRESSION: it is commendable that you are sending patches in small,
easy digestible chunks / patch series.  It is much easier to review 10+
series than 80+ behemoth (though I understand it is not always possible
to subdivide patch series into smaller self-contained sub-series).

On the other hand, it would be nice to have some roadmap about series
and features to be sent in the future, if possible.  Something like what
was done when 'git rebase --interactive' was getting builtinified: moved
(in parts) to C.  It would be great to have such roadmap with milestones
achieved and milestones to be done in the cover letter for series.


I suppose that is what I intended in the "Future Work" section of 
Documentation/technical/commit-graph.txt. It gives a set of things that 
need to be done in order to make this a default feature, not just an 
expert-level feature. When I wrote the section, I was focused entirely 
on "commit-graph version 1.0" and I didn't know how long that would 
take. The series have been getting interest and review (in great part to 
your interest, Jakub) so they have been moving to 'next' faster than I 
anticipated.


I'll plan on writing a more detailed list of future directions, but for 
now I'll list the things I know about and how I see them in priority order:


Commit-graph v1.0:
* ds/generation-numbers
* 'verify' and fsck/gc integration
* correct behavior with shallow clones, replace-objects, and grafts

Commit-graph v1.1:
* Place commit-graph storage in the_repository
* 'git tag --merged' use generation numbers
* 'git log --graph' use generation numbers

Commit-graph v1.X:
* Protocol v2 verb for sending commit-graph
* Bloom filters for changed paths




The big wins remaining from this data are `git tag --merged` and `git
log --graph`. The `tag` scenario is probably easier: this can be done
by replacing the revision-walk underlying the call to use
paint_down_to_common() instead. Requires adding an external method to
commit.c, but not too much code.

I wonder if there is some significant reason behind `git tag --merged`
using its own codepath, beside historical reasons.  Maybe performance is
better with current code?

Utilizing paint_down_to_common() there, beside reducing amount of code
you would have to modify, would also unify code (and possibly reduce
amount of code).  That's very nice.


The tougher challenge is `git log --graph`. The revision walk
machinery currently uses two precompute phases before iterating
results to the pager: limit_list() and sort_in_topological_order();
these correspond to two phases of Kahn's algorithm for topo-sort
(compute in-degrees, then walk by peeling commits with in-degree
zero). This requires O(N) time, where N is the number of reachable
commits. Instead, we could make this be O(W) time to output one page
of results, where W is (roughly) the number of reachable commits with
generation number above the last reported result.

A reminder: Kahn's algorithm (described for example in [1] and [3])
looks like this:

   L ← Empty list that will contain the sorted elements
   S ← Collection of all nodes with no incoming edge
   while S is non-empty do
   remove a node 'n' from S
   add 'n' to tail of L
   for each parent 'm' of 'n' do
   decrease the in-degree of 'm'
   if 'm' has in-degree of 0 then
   insert 'm' into S

[1]: https://en.wikipedia.org/wiki/Topological_sorting#Kahn's_algorithm
[2]: https://www.geeksforgeeks.org/topological-sorting-indegree-based-solution/


In order to take advantage of this approach, the two phases of Kahn's
algorithm need to be done in-line with reporting results to the
pager. This means keeping two queues: one is a priority queue

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

2018-05-14 Thread Ben Peart




On 5/12/2018 4:04 AM, Eckhard Maaß wrote:

On Fri, May 11, 2018 at 12:56:39PM +, Ben Peart wrote:

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 vs seconds with this patch.


I see where your need comes from, but as you based this on my little
patch one can achieve this already with tweaking diff.renames itself. I
do wonder why there is a special need for the status command here


The rename detection feature is nice and we'd like to leave it on 
whenever possible.  The performance issues only occur when in the middle 
of a merge - normal status commands behave reasonably.  As a result, we 
don't want to just turn it off completely by setting diff.renames.


Until we come up with a more elegant solution, we currently turn it off 
completely for merge via the new merge settings and then intercept calls 
to status and if there is a MERGE_HEAD we turn it off for status just 
for that specific call.  I view this as a temporary solution so would 
not want to put that logic into git proper as it is quite specific to 
when running git on a virtualized repo.



if there is, I personally would like it more in a style that you could
take all the options provided by diff.*-configuration and prefix that
with status, eg status.diff.renames = true. What do you think? If you
really only need this for merges, maybe a more specialised option is
called for that only kicks in when there is a merge going on?

I would like that status behaves as similar as possible to
diff/show/log. Special options will pull away from that again - passing
-m to show or log will lead to the same performance issues, correct?
Could it be feasible to impose an overall time limit on the detection?



I agree that they should behave as similar as possible which is why all 
the new settings default to the diff setting when not explicitly set.  I 
believe this is a good model - if you don't do anything special you get 
the default/same behavior but if you know and need special behavior, you 
now have that option.



And after writing this I wonder what were your experience with just
tweaking renameLimit - setting it very low should have helped the
fetching from server part already, shouldn't it?


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


Would it be reasonable to extend this so that we just use the same
machinery for parsing command line options for the diffcore options and
pass this along? It seems to me that git status wants the same init as
diff/show/log has anyway. But I like the direction towards passing more
command line options to the git status command.



I agree that it is unfortunate that diff/merge/status all parse and deal 
with config settings differently.  I'd be happy to see someone tackle 
that and move the code to a single, coherent model but that is beyond 
the scope of this patch.



  static void wt_longstatus_print_unmerged_header(struct wt_status *s)
@@ -592,6 +595,9 @@ static void wt_status_collect_changes_worktree(struct 
wt_status *s)
}
rev.diffopt.format_callback = wt_status_collect_changed_cb;
rev.diffopt.format_callback_data = s;
+   rev.diffopt.detect_rename = s->detect_rename >= 0 ? s->detect_rename : 
rev.diffopt.detect_rename;
+   rev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : 
rev.diffopt.rename_limit;
+   rev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : 
rev.diffopt.rename_score;
copy_pathspec(_data, >pathspec);
run_diff_files(, 0);
  }
@@ -625,6 +631,9 @@ static void wt_status_collect_changes_index(struct 
wt_status *s)
rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
rev.diffopt.format_callback = wt_status_collect_updated_cb;
rev.diffopt.format_callback_data = s;
+   rev.diffopt.detect_rename = s->detect_rename >= 0 ? s->detect_rename : 
rev.diffopt.detect_rename;
+   rev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : 
rev.diffopt.rename_limit;
+   rev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : 
rev.diffopt.rename_score;
copy_pathspec(_data, >pathspec);
run_diff_index(, 1);
  }
@@ -982,6 +991,9 @@ static void wt_longstatus_print_verbose(struct wt_status *s)
setup_revisions(0, NULL, , );
  
  	rev.diffopt.output_format |= DIFF_FORMAT_PATCH;

+   rev.diffopt.detect_rename = s->detect_rename >= 0 ? s->detect_rename : 
rev.diffopt.detect_rename;
+   rev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit :

Re: [PATCH 0/4] a few mark_parents_uninteresting cleanups


On 5/11/2018 2:00 PM, Jeff King wrote:

This is a follow-up to the discussion from February:

   
https://public-inbox.org/git/1519522496-73090-1-git-send-email-dsto...@microsoft.com/

There I theorized that some of these extra has_object_file() checks were
unnecessary. After poking around a bit, I've convinced myself that this
is the case, so here are some patches.

After Stolee's fix in ebbed3ba04 (revision.c: reduce object database
queries, 2018-02-24), I doubt this will provide any noticeable speedup.
IMHO the value is just in simplifying the logic.

The first two patches are the actual has_object_file simplifications.
The second two are an attempt to fix some head-scratching I had while
reading mark_parents_uninteresting(). I hope the result is easier to
follow, but I may have just shuffled one confusion for another. :)

   [1/4]: mark_tree_contents_uninteresting(): drop missing object check
   [2/4]: mark_parents_uninteresting(): drop missing object check
   [3/4]: mark_parents_uninteresting(): replace list with stack
   [4/4]: mark_parents_uninteresting(): avoid most allocation

  revision.c | 90 ++
  1 file changed, 50 insertions(+), 40 deletions(-)

-Peff


This series looks good to me. I found Patch 3 hard to read in the diff, 
so I just looked at the final result and the new arrangement is very 
clear about how it should behave.


Reviewed-by: Derrick Stolee

Re: [PATCH 4/4] mark_parents_uninteresting(): avoid most allocation


On 5/11/2018 2:03 PM, Jeff King wrote:

Commit 941ba8db57 (Eliminate recursion in setting/clearing
marks in commit list, 2012-01-14) used a clever double-loop
to avoid allocations for single-parent chains of history.
However, it did so only when following parents of parents
(which was an uncommon case), and _always_ incurred at least
one allocation to populate the list of pending parents in
the first place.

We can turn this into zero-allocation in the common case by
iterating directly over the initial parent list, and then
following up on any pending items we might have discovered.


This change appears to improve performance, but I was unable to measure 
any difference between this commit and the one ahead, even when merging 
ds/generation-numbers (which significantly reduces the other costs). I 
was testing 'git status' and 'git rev-list --boundary 
master...origin/master' in the Linux repo with my copy of master 70,000+ 
commits behind origin/master.


It's still a good change, but I was hoping to find a measurable benefit :(



Signed-off-by: Jeff King 
---
Again, try "-w" for more readability.

  revision.c | 44 +---
  1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/revision.c b/revision.c
index 89ff9a99ce..cbe041128e 100644
--- a/revision.c
+++ b/revision.c
@@ -115,32 +115,38 @@ static void commit_stack_clear(struct commit_stack *stack)
stack->nr = stack->alloc = 0;
  }
  
-void mark_parents_uninteresting(struct commit *commit)

+static void mark_one_parent_uninteresting(struct commit *commit,
+ struct commit_stack *pending)
  {
-   struct commit_stack pending = COMMIT_STACK_INIT;
struct commit_list *l;
  
+	if (commit->object.flags & UNINTERESTING)

+   return;
+   commit->object.flags |= UNINTERESTING;
+
+   /*
+* Normally we haven't parsed the parent
+* yet, so we won't have a parent of a parent
+* here. However, it may turn out that we've
+* reached this commit some other way (where it
+* wasn't uninteresting), in which case we need
+* to mark its parents recursively too..
+*/
for (l = commit->parents; l; l = l->next)
-   commit_stack_push(, l->item);
+   commit_stack_push(pending, l->item);
+}
  
-	while (pending.nr > 0) {

-   struct commit *commit = commit_stack_pop();
+void mark_parents_uninteresting(struct commit *commit)
+{
+   struct commit_stack pending = COMMIT_STACK_INIT;
+   struct commit_list *l;
  
-		if (commit->object.flags & UNINTERESTING)

-   return;
-   commit->object.flags |= UNINTERESTING;
+   for (l = commit->parents; l; l = l->next)
+   mark_one_parent_uninteresting(l->item, );
  
-		/*

-* Normally we haven't parsed the parent
-* yet, so we won't have a parent of a parent
-* here. However, it may turn out that we've
-* reached this commit some other way (where it
-* wasn't uninteresting), in which case we need
-* to mark its parents recursively too..
-*/
-   for (l = commit->parents; l; l = l->next)
-   commit_stack_push(, l->item);
-   }
+   while (pending.nr > 0)
+   mark_one_parent_uninteresting(commit_stack_pop(),
+ );
  
  	commit_stack_clear();

  }

[RFC PATCH 10/10] t7415: add new test about using HEAD:.gitmodules from the index

git submodule commands can now access .gitmodules from the index when
it's not checked out in the work tree, add some tests for that scenario.

Signed-off-by: Antonio Ospite 
---
 t/t7415-submodule-sparse-gitmodules.sh | 124 +
 1 file changed, 124 insertions(+)
 create mode 100755 t/t7415-submodule-sparse-gitmodules.sh

diff --git a/t/t7415-submodule-sparse-gitmodules.sh 
b/t/t7415-submodule-sparse-gitmodules.sh
new file mode 100755
index 0..3ae269b3a
--- /dev/null
+++ b/t/t7415-submodule-sparse-gitmodules.sh
@@ -0,0 +1,124 @@
+#!/bin/sh
+#
+# Copyright (C) 2018  Antonio Ospite 
+#
+
+test_description=' Test reading/writing the gitmodules config file when not 
checked out
+
+This test verifies that reading the gitmodules config file from the index when
+it is not checked out works, and that writing to it does not.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'sparse checkout setup which hides .gitmodules' '
+   echo file > file &&
+   git add file &&
+   test_tick &&
+   git commit -m upstream &&
+   git clone . super &&
+   git clone super submodule &&
+   git clone super new_submodule &&
+   (cd super &&
+   git submodule add ../submodule
+   test_tick &&
+   git commit -m submodule &&
+   cat >.git/info/sparse-checkout <<\EOF &&
+/*
+!/.gitmodules
+EOF
+   git config core.sparsecheckout true &&
+   git read-tree -m -u HEAD &&
+   test ! -e .gitmodules
+   )
+'
+
+test_expect_success 'reading gitmodules config file when it is not checked 
out' '
+   (cd super &&
+   echo "../submodule" >expected &&
+   git submodule--helper config submodule.submodule.url >actual &&
+   test_cmp expected actual
+   )
+'
+
+test_expect_success 'not writing gitmodules config file when it is not checked 
out' '
+   (cd super &&
+   test_must_fail git submodule--helper config 
submodule.submodule.url newurl
+   )
+'
+
+test_expect_success 'not staging gitmodules config when it is not checked out' 
'
+   (cd super &&
+   test_must_fail git submodule--helper config --stage
+   )
+'
+
+test_expect_success 'not even staging manually crafted .gitmodules when it is 
not supposed to be checked out' '
+   (cd super &&
+   echo "bogus content" > .gitmodules &&
+   test_must_fail git submodule--helper config --stage &&
+   rm .gitmodules
+   )
+'
+
+test_expect_success 'initialising submodule when the gitmodules config is not 
checked out' '
+   (cd super &&
+   git submodule init
+   )
+'
+
+test_expect_success 'showing submodule summary when the gitmodules config is 
not checked out' '
+   (cd super &&
+   git submodule summary
+   )
+'
+
+test_expect_success 'updating submodule when the gitmodules config is not 
checked out' '
+   (cd submodule &&
+   echo file2 >file2 &&
+   git add file2 &&
+   git commit -m "add file2 to submodule"
+   ) &&
+   (cd super &&
+   git submodule update
+   )
+'
+
+test_expect_success 'not moving submodule when the gitmodules config is not 
checked out' '
+   (cd super &&
+   test_must_fail git mv submodule moved_submodule
+   )
+'
+
+test_expect_success 'not removing submodule when the gitmodules config is not 
checked out' '
+   (cd super &&
+   test_must_fail git rm -r submodule
+   )
+'
+
+test_expect_success 'not adding submodules when the gitmodules config is not 
checked out' '
+   (cd super &&
+   test_must_fail git submodule add ../new_submodule
+   )
+'
+
+# "git add" in the test above fails as expected, however it still leaves the
+# cloned directory there and adds a config entry to .git/config. This is
+# because no cleanup is done by cmd_add in git-submodule.sh when "git
+# submodule--helper config" fails to add a new config setting.
+#
+# If we added the following commands to the test above:
+#
+#   rm -rf .git/modules/new_submodule &&
+#   git reset HEAD new_submodule &&
+#   rm -rf new_submodule
+#
+# then the repository would be in a clean status and the test below would
+# pass, but maybe cmd_add should do that.
+test_expect_failure 'init submodule after adding failed when the gitmodules 
config is not checked out' '
+   (cd super &&
+   git submodule init
+   )
+'
+
+test_done
-- 
2.17.0

[RFC PATCH 09/10] submodule: support reading .gitmodules even when it's not checked out

When the .gitmodules file is not available in the working directory, try
using HEAD:.gitmodules from the index. This covers the case when the
file is part of the repository but for some reason it is not checked
out, for example because of a sparse checkout.

This makes it possible to use at least the 'git submodule' commands
which *read* the gitmodules configuration file without fully populating
the work dir.

Writing to .gitmodules wills still require that the file is checked out,
so check for that in config_gitmodules_set.

Signed-off-by: Antonio Ospite 
---

I am doing the is_gitmodules_hidden() check in the open for now, I am not sure
whether it is approprate to do that inside stage_updated_gitmodules.

 builtin/mv.c|  2 ++
 builtin/rm.c|  7 +--
 builtin/submodule--helper.c | 21 -
 cache.h |  1 +
 config.c| 15 +--
 submodule.c | 15 +++
 submodule.h |  1 +
 7 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 7a63667d6..41fd9b7be 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -83,6 +83,8 @@ static void prepare_move_submodule(const char *src, int first,
die(_("Directory %s is in index and no submodule?"), src);
if (!is_staging_gitmodules_ok(_index))
die(_("Please stage your changes to .gitmodules or stash them 
to proceed"));
+   if (is_gitmodules_hidden(_index))
+   die(_("cannot work with hidden submodule config"));
strbuf_addf(_dotgit, "%s/.git", src);
*submodule_gitfile = read_gitfile(submodule_dotgit.buf);
if (*submodule_gitfile)
diff --git a/builtin/rm.c b/builtin/rm.c
index 5b6fc7ee8..e3526a342 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -284,9 +284,12 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
ALLOC_GROW(list.entry, list.nr + 1, list.alloc);
list.entry[list.nr].name = xstrdup(ce->name);
list.entry[list.nr].is_submodule = S_ISGITLINK(ce->ce_mode);
-   if (list.entry[list.nr++].is_submodule &&
-   !is_staging_gitmodules_ok(_index))
+   if (list.entry[list.nr++].is_submodule) {
+   if (!is_staging_gitmodules_ok(_index))
die (_("Please stage your changes to .gitmodules or 
stash them to proceed"));
+   if (is_gitmodules_hidden(_index))
+   die(_("cannot work with hidden submodule config"));
+   }
}
 
if (pathspec.nr) {
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index de5caa776..b3bdb4b66 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1873,6 +1873,9 @@ static int module_config(int argc, const char **argv, 
const char *prefix)
if (read_cache() < 0)
die(_("index file corrupt"));
 
+   if (is_gitmodules_hidden(_index))
+   die(_("cannot stage changes to hidden submodule 
config"));
+
stage_updated_gitmodules(_index);
 
if (write_locked_index(_index, _file,
@@ -1897,8 +1900,24 @@ static int module_config(int argc, const char **argv, 
const char *prefix)
}
 
/* Equivalent to ACTION_SET in builtin/config.c */
-   if (argc == 3)
+   if (argc == 3) {
+   struct object_id oid;
+
+   /* 
+* If the .gitmodules file is not in the work tree but it is
+* in the index, stop, as writing new values and staging them
+* would blindly overwrite ALL the old content.
+*
+* Do not use is_gitmodules_hidden() here, to gracefully
+* handle the case when .gitmodules is neither in the work
+* tree nor in the index, i.e.: a new GITMODULES_FILE is going
+* to be created.
+*/
+   if (!file_exists(GITMODULES_FILE) && get_oid(GITMODULES_BLOB, 
) >= 0)
+   die(_("cannot change unchecked out .gitmodules, check 
it out first"));
+
return config_gitmodules_set(argv[1], argv[2]);
+   }
 
return 0;
 }
diff --git a/cache.h b/cache.h
index 0c1fb9fbc..6d45b0cbb 100644
--- a/cache.h
+++ b/cache.h
@@ -417,6 +417,7 @@ static inline enum object_type object_type(unsigned int 
mode)
 #define INFOATTRIBUTES_FILE "info/attributes"
 #define ATTRIBUTE_MACRO_PREFIX "[attr]"
 #define GITMODULES_FILE ".gitmodules"
+#define GITMODULES_BLOB "HEAD:.gitmodules"
 #define GIT_NOTES_REF_ENVIRONMENT "GIT_NOTES_REF"
 #define GIT_NOTES_DEFAULT_REF "refs/notes/commits"
 #define GIT_NOTES_DISPLAY_REF_ENVIRONMENT "GIT_NOTES_DISPLAY_REF"
diff --git a/config.c b/config.c
index 8ffe29330..7d9744622 100644
--- a/config.c
+++ b/config.c
@@ -2184,8 +2184,19 @@ int

[RFC PATCH 08/10] t7506: cleanup .gitmodules properly before setting up new scenario

In t/t7506-status-submodule.sh at some point a new scenario is set up to
test different things, in particular new submodules are added which are
meant to completely replace the previous ones.

However the code just removes .gitmodules from the work tree, still
leaving it in the index.

This will break when "submodule--helper config" learns to handle
.gitmodules from the index and performs some check when doing that.

Since the test means to get rid of .gitmodules anyways, let's completely
remove it from the index, to actually start afresh in the new scenario.

This is more future-proof without breaking current tests.

Signed-off-by: Antonio Ospite 
---
 t/t7506-status-submodule.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index 9edf6572e..389173294 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -325,7 +325,8 @@ test_expect_success 'setup superproject with untracked file 
in nested submodule'
(
cd super &&
git clean -dfx &&
-   rm .gitmodules &&
+   git rm .gitmodules &&
+   git commit -m "remove .gitmodules" &&
git submodule add -f ./sub1 &&
git submodule add -f ./sub2 &&
git submodule add -f ./sub1 sub3 &&
-- 
2.17.0

[RFC PATCH 01/10] config: make config_from_gitmodules generally useful

The config_from_gitmodules() function is a good candidate for
a centralized point where to read the gitmodules configuration file.

Add a repo argument to it to make the function more general, and adjust
the current callers in cmd_fetch and update-clone.

As a proof of the utility of the change, start using the function also
in repo_read_gitmodules which was basically doing the same operations.

Signed-off-by: Antonio Ospite 
---
 builtin/fetch.c |  2 +-
 builtin/submodule--helper.c |  2 +-
 config.c| 13 +++--
 config.h| 10 +-
 submodule-config.c  | 16 
 5 files changed, 14 insertions(+), 29 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7ee83ac0f..a67ee7c39 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1445,7 +1445,7 @@ int cmd_fetch(int argc, const char **argv, const char 
*prefix)
for (i = 1; i < argc; i++)
strbuf_addf(_rla, " %s", argv[i]);
 
-   config_from_gitmodules(gitmodules_fetch_config, NULL);
+   config_from_gitmodules(gitmodules_fetch_config, the_repository, NULL);
git_config(git_fetch_config, NULL);
 
argc = parse_options(argc, argv, prefix,
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c2403a915..9e8f2acd5 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1602,7 +1602,7 @@ static int update_clone(int argc, const char **argv, 
const char *prefix)
};
suc.prefix = prefix;
 
-   config_from_gitmodules(gitmodules_update_clone_config, _jobs);
+   config_from_gitmodules(gitmodules_update_clone_config, the_repository, 
_jobs);
git_config(gitmodules_update_clone_config, _jobs);
 
argc = parse_options(argc, argv, prefix, module_update_clone_options,
diff --git a/config.c b/config.c
index 6f8f1d8c1..8ffe29330 100644
--- a/config.c
+++ b/config.c
@@ -2173,17 +2173,18 @@ int git_config_get_pathname(const char *key, const char 
**dest)
 }
 
 /*
- * Note: This function exists solely to maintain backward compatibility with
- * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
- * NOT be used anywhere else.
+ * Note: Initially this function existed solely to maintain backward
+ * compatibility with 'fetch' and 'update_clone' storing configuration in
+ * '.gitmodules' but it turns out it can be useful as a centralized point to
+ * read the gitmodules config file.
  *
  * Runs the provided config function on the '.gitmodules' file found in the
  * working directory.
  */
-void config_from_gitmodules(config_fn_t fn, void *data)
+void config_from_gitmodules(config_fn_t fn, struct repository *repo, void 
*data)
 {
-   if (the_repository->worktree) {
-   char *file = repo_worktree_path(the_repository, 
GITMODULES_FILE);
+   if (repo->worktree) {
+   char *file = repo_worktree_path(repo, GITMODULES_FILE);
git_config_from_file(fn, file, data);
free(file);
}
diff --git a/config.h b/config.h
index cdac2fc73..43ce76c0f 100644
--- a/config.h
+++ b/config.h
@@ -215,15 +215,7 @@ extern int repo_config_get_maybe_bool(struct repository 
*repo,
 extern int repo_config_get_pathname(struct repository *repo,
const char *key, const char **dest);
 
-/*
- * Note: This function exists solely to maintain backward compatibility with
- * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
- * NOT be used anywhere else.
- *
- * Runs the provided config function on the '.gitmodules' file found in the
- * working directory.
- */
-extern void config_from_gitmodules(config_fn_t fn, void *data);
+extern void config_from_gitmodules(config_fn_t fn, struct repository *repo, 
void *data);
 
 extern int git_config_get_value(const char *key, const char **value);
 extern const struct string_list *git_config_get_value_multi(const char *key);
diff --git a/submodule-config.c b/submodule-config.c
index d87c3ff63..f39c71dfb 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -577,19 +577,11 @@ void repo_read_gitmodules(struct repository *repo)
 {
submodule_cache_check_init(repo);
 
-   if (repo->worktree) {
-   char *gitmodules;
-
-   if (repo_read_index(repo) < 0)
-   return;
-
-   gitmodules = repo_worktree_path(repo, GITMODULES_FILE);
-
-   if (!is_gitmodules_unmerged(repo->index))
-   git_config_from_file(gitmodules_cb, gitmodules, repo);
+   if (repo_read_index(repo) < 0)
+   return;
 
-   free(gitmodules);
-   }
+   if (!is_gitmodules_unmerged(repo->index))
+   config_from_gitmodules(gitmodules_cb, repo, repo);
 
repo->submodule_cache->gitmodules_read = 1;
 }
-- 
2.17.0

[RFC PATCH 06/10] submodule--helper: add a '--stage' option to the 'config' sub command

Add a --stage option to the 'submodule--helper config' command so that
the .gitmodules file can be staged without referring to it explicitly by
file path.

In practice the config will still be written to .gitmoudules, there are
no plans to change the file path, but having this level of indirection
makes it possible to perform possible additional checks before staging
the file.

Signed-off-by: Antonio Ospite 
---
 builtin/submodule--helper.c | 44 +++--
 t/t7411-submodule-config.sh | 35 +
 2 files changed, 77 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b32110e3b..de5caa776 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -5,6 +5,7 @@
 #include "parse-options.h"
 #include "quote.h"
 #include "pathspec.h"
+#include "lockfile.h"
 #include "dir.h"
 #include "submodule.h"
 #include "submodule-config.h"
@@ -1837,10 +1838,49 @@ static int config_print_callback(const char *key_, 
const char *value_, void *cb_
 
 static int module_config(int argc, const char **argv, const char *prefix)
 {
+   int stage_gitmodules = 0;
int ret;
 
-   if (argc < 2 || argc > 3)
-   die("submodule--helper config takes 1 or 2 arguments: name 
[value]");
+   struct option module_config_options[] = {
+   OPT_BOOL(0, "stage", _gitmodules,
+   N_("stage the .gitmodules file content")),
+   OPT_END()
+   };
+   const char *const git_submodule_helper_usage[] = {
+   N_("git submodule--helper config name [value]"),
+   N_("git submodule--helper config --stage"),
+   NULL
+   };
+
+   argc = parse_options(argc, argv, prefix, module_config_options,
+git_submodule_helper_usage, PARSE_OPT_KEEP_ARGV0);
+
+   if ((stage_gitmodules && argc > 1) ||
+   (!stage_gitmodules && (argc < 2 || argc > 3)))
+   usage_with_options(git_submodule_helper_usage, 
module_config_options);
+
+   /*
+* Equivalent to "git add --force .gitmodules".
+*
+* Silently override already staged changes, to support consecutive
+* invocations of "git submodule add".
+*/
+   if (stage_gitmodules) {
+   static struct lock_file lock_file;
+
+   hold_locked_index(_file, LOCK_DIE_ON_ERROR);
+
+   if (read_cache() < 0)
+   die(_("index file corrupt"));
+
+   stage_updated_gitmodules(_index);
+
+   if (write_locked_index(_index, _file,
+  COMMIT_LOCK | SKIP_IF_UNCHANGED))
+   die(_("Unable to write new index file"));
+
+   return 0;
+   }
 
/* Equivalent to ACTION_GET in builtin/config.c */
if (argc == 2) {
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index dfe019f05..a0b2f7cd6 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -165,4 +165,39 @@ test_expect_success 'overwriting unstaged submodules 
config with "submodule--hel
)
 '
 
+test_expect_success 'staging submodules config with "submodule--helper 
config"' '
+   (cd super &&
+   : >expected &&
+   git diff --name-only --cached >actual &&
+   test_cmp expected actual &&
+   git submodule--helper config --stage &&
+   echo ".gitmodules" >expected &&
+   git diff --name-only --cached >actual &&
+   test_cmp expected actual
+   )
+'
+
+test_expect_success 'overwriting staged submodules config with 
"submodule--helper config"' '
+   (cd super &&
+   echo "even_newer_url" >expected &&
+   git submodule--helper config submodule.submodule.url 
"even_newer_url" &&
+   git submodule--helper config submodule.submodule.url >actual &&
+   test_cmp expected actual
+   )
+'
+
+test_expect_success 're-staging overridden submodule config with 
"submodule--helper config"' '
+   (cd super &&
+   echo ".gitmodules" >expected &&
+   git diff --name-only --cached >actual &&
+   test_cmp expected actual &&
+   echo "bogus config" >.gitmodules &&
+   git submodule--helper config --stage &&
+   git diff --name-only --cached >actual &&
+   test_cmp expected actual &&
+   git reset HEAD .gitmodules &&
+   git checkout .gitmodules
+   )
+'
+
 test_done
-- 
2.17.0

[RFC PATCH 07/10] submodule: use 'submodule--helper config --stage' to stage .gitmodules

Use 'git submodule--helper config --stage' in git-submodule.sh to remove
the last place where the .gitmodules file is mentioned explicitly by its
file path.

Signed-off-by: Antonio Ospite 
---
 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 0286b029f..61e62ef17 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -277,7 +277,7 @@ or you are unsure what this means choose another name with 
the '--name' option."
then
git submodule--helper config submodule."$sm_name".branch 
"$branch"
fi &&
-   git add --force .gitmodules ||
+   git submodule--helper config --stage ||
die "$(eval_gettext "Failed to register submodule '\$sm_path'")"
 
# NEEDSWORK: In a multi-working-tree world, this needs to be
-- 
2.17.0

[RFC PATCH 05/10] submodule: use the 'submodule--helper config' command

Use the 'submodule--helper config' command in git-modules.sh to avoid
referring explicitly to .gitmodules by the hardcoded file path.

This makes it possible to access the submodules configuration in a more
flexible way.

Signed-off-by: Antonio Ospite 
---
 git-submodule.sh | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 24914963c..0286b029f 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -71,7 +71,7 @@ get_submodule_config () {
value=$(git config submodule."$name"."$option")
if test -z "$value"
then
-   value=$(git config -f .gitmodules submodule."$name"."$option")
+   value=$(git submodule--helper config 
submodule."$name"."$option")
fi
printf '%s' "${value:-$default}"
 }
@@ -271,11 +271,11 @@ or you are unsure what this means choose another name 
with the '--name' option."
git add --no-warn-embedded-repo $force "$sm_path" ||
die "$(eval_gettext "Failed to add submodule '\$sm_path'")"
 
-   git config -f .gitmodules submodule."$sm_name".path "$sm_path" &&
-   git config -f .gitmodules submodule."$sm_name".url "$repo" &&
+   git submodule--helper config submodule."$sm_name".path "$sm_path" &&
+   git submodule--helper config submodule."$sm_name".url "$repo" &&
if test -n "$branch"
then
-   git config -f .gitmodules submodule."$sm_name".branch "$branch"
+   git submodule--helper config submodule."$sm_name".branch 
"$branch"
fi &&
git add --force .gitmodules ||
die "$(eval_gettext "Failed to register submodule '\$sm_path'")"
-- 
2.17.0

[RFC PATCH 00/10] Make submodules work if .gitmodules is not checked out

Hi,

vcsh[1] uses bare git repositories and detached work-trees to manage
*distinct* sets of configuration files directly into $HOME.

In this setup multiple repositories share the same directory (namely
$HOME) as their work dir, so the sets of checked out files would also
need to be *disjoint* to avoid collisions (examples of collisions can be
README or LICENSE files, or even .gitignore and .gitattributes).

Amongst vcsh users, a popular solution to this problem is to use sparse
checkouts, representing the *intersection* of all the repositories in
a common sparse-checkout file[2].

This works well but there are still limitations about the ability to use
submodules because git expects the .gitmodules file to be checked out.

The user (or vcsh itself) might learn to fully populate one repository
at a time when working with submodules but this is unhandy and would
introduce serialization even when it's not strictly needed like in the
case of _reading_ .gitmodules.

As a side note, git submodules have worked perfectly fine with detached
work-trees for some time[3,4,5] so extending them to also play nice with
sparse checkouts seems the next logical step to cover the vcsh use case.

This series teaches git to try and read the .gitmodules file from the
index (HEAD:.gitmodules) when it's not available in the work dir.

It does so by first providing an opaque way to access the submodules
configuration, and then extends the access mechanism behind the scenes.

Writing to .gitmodules still requires it to be checked out.

This series should be in line with what Stefan and Jonathan proposed;
although it's not perfect yet:

  - naming of functions can be improved,
  - code can be moved around to better places,
  - maybe some notes should be added to Documentation/git-submodule.txt,
  - my git terminology may still be a little off: do "work tree" and
"work directory" mean the same thing?

the functionality is there and we should have a decent baseline to work
on.

The patchset is based on the current master (ccdcbd54c447), the
test-suite passes after each commit and there are some per-patch
annotations.

If anyone wanted to pick up and finish the work feel free to do so,
otherwise please comment and I'll try to address issues as time permits.

Thanks,
   Antonio

[1] https://github.com/RichiH/vcsh
[2] https://github.com/RichiH/vcsh/issues/120#issuecomment-387335765
[3] 
http://git.661346.n2.nabble.com/git-submodule-vs-GIT-WORK-TREE-td7562165.html
[4] 
http://git.661346.n2.nabble.com/PATCH-Solve-git-submodule-issues-with-detached-work-trees-td7563377.html
[5] https://github.com/git/git/commit/be8779f7ac9a3be9aa783df008d59082f4054f67

Antonio Ospite (10):
  config: make config_from_gitmodules generally useful
  submodule: factor out a config_gitmodules_set function
  t7411: be nicer to other tests and really clean things up
  submodule--helper: add a new 'config' subcommand
  submodule: use the 'submodule--helper config' command
  submodule--helper: add a '--stage' option to the 'config' sub command
  submodule: use 'submodule--helper config --stage' to stage .gitmodules
  t7506: cleanup .gitmodules properly before setting up new scenario
  submodule: support reading .gitmodules even when it's not checked out
  t7415: add new test about using HEAD:.gitmodules from the index

 builtin/fetch.c|   2 +-
 builtin/mv.c   |   2 +
 builtin/rm.c   |   7 +-
 builtin/submodule--helper.c| 100 +++-
 cache.h|   1 +
 config.c   |  26 --
 config.h   |  10 +-
 git-submodule.sh   |  10 +-
 submodule-config.c |  16 +---
 submodule.c|  37 ++--
 submodule.h|   2 +
 t/t7411-submodule-config.sh|  63 -
 t/t7415-submodule-sparse-gitmodules.sh | 124 +
 t/t7506-status-submodule.sh|   3 +-
 14 files changed, 357 insertions(+), 46 deletions(-)
 create mode 100755 t/t7415-submodule-sparse-gitmodules.sh

-- 
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?

[RFC PATCH 03/10] t7411: be nicer to other tests and really clean things up

Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with
invalid lines in .gitmodules but then only the second commit is removed.

This may affect subsequent tests if they assume that the .gitmodules
file has no errors.

Since those commits are not needed anymore remove both of them.

Signed-off-by: Antonio Ospite 
---

I am putting these fixups to the test-suite before the patch that actually
needs them so that the test-suite passes after each commit.

 t/t7411-submodule-config.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 0bde5850a..a648de6a9 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -135,7 +135,7 @@ test_expect_success 'error in history in 
fetchrecursesubmodule lets continue' '
HEAD submodule \
>actual &&
test_cmp expect_error actual  &&
-   git reset --hard HEAD^
+   git reset --hard HEAD~2
)
 '
 
-- 
2.17.0

[RFC PATCH 04/10] submodule--helper: add a new 'config' subcommand

Add a new 'config' subcommand to 'submodule--helper', this extra level
of indirection makes it possible to add some flexibility to how the
submodules configuration is handled.

Signed-off-by: Antonio Ospite 
---
 builtin/submodule--helper.c | 39 +
 t/t7411-submodule-config.sh | 26 +
 2 files changed, 65 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 9e8f2acd5..b32110e3b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1825,6 +1825,44 @@ static int is_active(int argc, const char **argv, const 
char *prefix)
return !is_submodule_active(the_repository, argv[1]);
 }
 
+static int config_print_callback(const char *key_, const char *value_, void 
*cb_data)
+{
+   char *key = cb_data;
+
+   if (!strcmp(key, key_))
+   printf("%s\n", value_);
+
+   return 0;
+}
+
+static int module_config(int argc, const char **argv, const char *prefix)
+{
+   int ret;
+
+   if (argc < 2 || argc > 3)
+   die("submodule--helper config takes 1 or 2 arguments: name 
[value]");
+
+   /* Equivalent to ACTION_GET in builtin/config.c */
+   if (argc == 2) {
+   char *key;
+
+   ret = git_config_parse_key(argv[1], , NULL);
+   if (ret < 0)
+   return CONFIG_INVALID_KEY;
+
+   config_from_gitmodules(config_print_callback, the_repository, 
key);
+
+   free(key);
+   return 0;
+   }
+
+   /* Equivalent to ACTION_SET in builtin/config.c */
+   if (argc == 3)
+   return config_gitmodules_set(argv[1], argv[2]);
+
+   return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -1850,6 +1888,7 @@ static struct cmd_struct commands[] = {
{"push-check", push_check, 0},
{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
{"is-active", is_active, 0},
+   {"config", module_config, 0},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index a648de6a9..dfe019f05 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -139,4 +139,30 @@ test_expect_success 'error in history in 
fetchrecursesubmodule lets continue' '
)
 '
 
+test_expect_success 'reading submodules config with "submodule--helper 
config"' '
+   (cd super &&
+   echo "../submodule" >expected &&
+   git submodule--helper config submodule.submodule.url >actual &&
+   test_cmp expected actual
+   )
+'
+
+test_expect_success 'writing submodules config with "submodule--helper 
config"' '
+   (cd super &&
+   echo "new_url" >expected &&
+   git submodule--helper config submodule.submodule.url "new_url" 
&&
+   git submodule--helper config submodule.submodule.url >actual &&
+   test_cmp expected actual
+   )
+'
+
+test_expect_success 'overwriting unstaged submodules config with 
"submodule--helper config"' '
+   (cd super &&
+   echo "newer_url" >expected &&
+   git submodule--helper config submodule.submodule.url 
"newer_url" &&
+   git submodule--helper config submodule.submodule.url >actual &&
+   test_cmp expected actual
+   )
+'
+
 test_done
-- 
2.17.0

[RFC PATCH 02/10] submodule: factor out a config_gitmodules_set function