Re: [RFC PATCH 00/10] Make submodules work if .gitmodules is not checked out
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, &ref->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(&format_data.oid, >> &format_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 populate_value() codepath. We can easily tell that fields th
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, &ref->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(&format_data.oid, > &format_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 = &helpers; > + > +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 = &o->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", &name, &namelen, &subkey) < 0) > + return 0; > + > + o = find_or_create_helper(name, namelen); > + > + if (!strcmp(subkey, "promisorremote")) > + return git_config_string(&o->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], &key, 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
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(&merges.objects[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
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&complete 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(&name, 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?
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?
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, &matched) != 1) return ref; - if (remote->push) { + if (remote->push.nr) { struct refspec_item query; memset(&query, 0, sizeof(struct refspec_item)); query.src = matched->name; - if (!query_refspecs(remote->push, remote->push_refspec_nr, &query) && + if (!query_refspecs(remote->push.items, remote->push.nr, &query) && query.dst) { struct strbuf buf = STRBUF_INIT; strbuf_addf(&buf, "%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, &push_map, remote->push_refspec_nr, - remote->push_refspec, MATCH_REFS_NONE); + match_push_refs(local_refs, &push_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(&states->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 = &remote->push.items[i]; if (spec->matching) item = string_list_append(&states->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(&buf); strbuf_addf(&buf, "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(&buf); strbuf_addf(&buf, "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(&remote->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(&ret->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
[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, &dst_tail, rs, nr_refspec); + refspec_appendn(&rs, refspec, nr_refspec); + errs = match_explicit_refs(src, *dst, &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, &pat); + dst_name = get_ref_match(rs.items, rs.nr, ref, send_mirror, FROM_SRC, &pat); 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(&src_ref_index, src); @@ -1396,6 +1396,9 @@ int match_push_refs(struct ref *src, struct ref **dst, } string_list_clear(&src_ref_index, 0); } + + refspec_clear(&rs); + 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, &fetch_pattern); + refspec_item_init(&refspec, value.buf, REFSPEC_FETCH); strbuf_reset(&value); @@ -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, &refspec); /* * 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(&value); junk_mode = JUNK_LEAVE_ALL; - free(refspec); + refspec_item_clear(&refspec); 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 = &rs->items[i]; + if (!item->exact_sha1) { + const char *glob = strchr(item->src, '*'); if (glob) argv_array_pushf(&ref_prefixes, "%.*s", -(int)(glob - refspecs[i].src), -refspecs[i].src); +(int)(glob - item->src), +item->src); else - expand_ref_prefix(&ref_prefixes, refspecs[i].src); + expand_ref_prefix(&ref_prefixes, item->src); } } @@ -367,13 +368,12 @@ static struct ref *get_ref_map(struct transport *transport, argv_array_clear(&ref_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, &refspecs[i], &tail, 0); - if (refspecs[i].dst && refspecs[i].dst[0]) + for (i = 0; i < rs->nr; i++) { + get_fetch_map(remote_refs, &rs->items[i], &tail, 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 = &refmap; + else + fetch_refspec = &transport->remote->fetch; - for (i = 0; i < fetch_refspec_nr; i++) - get_fetch_map(ref_map, &fetch_refspec[i], &oref_tail, 1); + for (i = 0; i < fetch_refspec->nr; i++) + get_fetch_map(ref_map, &fetch_refspec->items[i], &oref_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, &autotags); + ref_map = get_ref_map(transport, rs, tags, &autotags); 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(&rs, 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(&rs, tag); + free(tag); + } else { + refspec_append(&rs, 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(&rs); 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), &oid, &full_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 = &refspecs[i]; + for (i = 0; i < refspecs.nr; i++) { + struct refspec_item *refspec = &refspecs.items[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, refspecs_list.items[i].string); string_list_clear(&refspecs_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(&refspecs); return 0; } -- 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(&rs, ref); } } @@ -226,7 +217,7 @@ static void setup_push_upstream(struct remote *remote, struct branch *branch, } strbuf_addf(&refspec, "%s:%s", branch->refname, branch->merge[0]->src); - add_refspec(refspec.buf); + refspec_append(&rs, 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(&refspec, "%s:%s", branch->refname, branch->refname); - add_refspec(refspec.buf); + refspec_append(&rs, 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(&rs, ":"); 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, &reject_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 = &rs; 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 = &remote->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++; } retu
[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, -&reject_reasons); + err = transport_push(transport, rs, flags, &reject_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(&tmp_rs, refspec, refspec_nr); - for (i = 0; i < tmp_rs.nr; i++) { - const struct refspec_item *item = &tmp_rs.items[i]; + for (i = 0; i < rs->nr; i++) { + const struct refspec_item *item = &rs->items[i]; const char *prefix = NULL; if (item->dst) @@ -1143,7 +1141,6 @@ int transport_push(struct transport *transport, &ref_prefixes); argv_array_clear(&ref_prefixes); - refspec_clear(&tmp_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, &remote_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(&commits, transport->remote, - refspec, refspec_nr, + rs->raw, rs->raw_nr, transport->push_options, pretend)) { oid_array_clear(&commits); 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(&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(&remote->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(&query, 0, sizeof(struct refspec_item)); query.src = (char *)name; - if (query_refspecs(refspecs, nr_refspec, &query)) + if (query_refspecs(rs->items, rs->nr, &query)) 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(&remote->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(&remote->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(&remote->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(&data->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 name
[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(&rs, 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 ? &push_options : NULL; if (from_stdin) { - struct argv_array all_refspecs = ARGV_ARRAY_INIT; - - for (i = 0; i < nr_refspecs; i++) - argv_array_push(&all_refspecs, refspecs[i]); - if (args.stateless_rpc) { const char *buf; while ((buf = packet_read_line(0, NULL))) - argv_array_push(&all_refspecs, buf); + refspec_append(&rs, buf); } else { struct strbuf line = STRBUF_INIT; while (strbuf_getline(&line, stdin) != EOF) - argv_array_push(&all_refspecs, line.buf); + refspec_append(&rs, line.buf); strbuf_release(&line); } - - 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, &remote_refs, nr_refspecs, refspecs, flags)) + if (match_push_refs(local_refs, &remote_refs, rs.raw_nr, rs.raw, flags)) return -1; if (!is_empty_cas(&cas)) -- 2.17.0.441.gb46fe60e1d-goog
[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(&transport->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, refspec_names, nr_refspec); - - for (i = 0; i < refspec.nr; i++) { - struct refspec_item *rs = &refspec.items[i]; + for (i = 0; i < rs->nr; i++) { + struct refspec_item *item = &rs->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(&refspec); 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(&cp.args, remote->name); - for (i = 0; i < refspec_nr; i++) - argv_array_push(&cp.args, refspec[i]); + for (i = 0; i < rs->raw_nr; i++) + argv_array_push(&cp.args, rs->raw[i]); } prepare_submodule_repo_env(&cp.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(&cp.args, head); argv_array_push(&cp.args, remote->name); - for (i = 0; i < refspec_nr; i++) - argv_array_push(&cp.args, refspec[i]); + for (i = 0; i < rs->raw_nr; i++) + argv_array_push(&cp.args, rs->raw[i]); prepare_submodule_repo_env(&cp.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(&commits, transport->remote, - rs->raw, rs->raw_nr, + rs, transport->push_options,
[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, &push_map, remote->push.raw_nr, - remote->push.raw, MATCH_REFS_NONE); + match_push_refs(local_refs, &push_map, &remote->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, &remote_refs, rs.raw_nr, rs.raw, flags)) + if (match_push_refs(local_refs, &remote_refs, &rs, flags)) return -1; if (!is_empty_cas(&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, &remote_refs, - rs.raw_nr, rs.raw, push_all)) { + if (match_push_refs(local_refs, &remote_refs, &rs, 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(&rs, refspec, nr_refspec); - errs = match_explicit_refs(src, *dst, &dst_tail, &rs); + /* If no refspec is provided, use the default ":" */ + if (!rs->nr) + refspec_append(rs, ":"); + + errs = match_explicit_refs(src, *dst, &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(&rs, ref, send_mirror, FROM_SRC, &pat); + dst_name = get_ref_match(rs, ref, send_mirror, FROM_SRC, &pat); 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(&rs, 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(&src_ref_index, src); @@ -1381,8 +1378,6 @@ int match_push_refs(struct ref *src, struct ref **dst, string_list_clear(&src_ref_index, 0); } - refspec_clear(&rs); - 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 *remot
[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 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(&rs, 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, &remote_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 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, &rs[i]); + for (i = errs = 0; i < rs->nr; i++) + errs += match_explicit(src, dst, dst_tail, &rs->items[i]); return errs; } @@ -1311,7 +1310,7 @@ int match_push_refs(struct ref *src, struct ref **dst, refspec = default_refspec; } refspec_appendn(&rs, refspec, nr_refspec); - errs = match_explicit_refs(src, *dst, &dst_tail, rs.items, rs.nr); + errs = match_explicit_refs(src, *dst, &dst_tail, &rs); /* 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 = &rs->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, &name); + match = match_name_with_pattern(item->src, ref->name, dst_side, &name); else - match = match_name_with_pattern(dst_side, ref->name, rs[i].src, &name); + match = match_name_with_pattern(dst_side, ref->name, item->src, &name); 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 = &rs->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, &pat); + dst_name = get_ref_match(&rs, ref, send_mirror, FROM_SRC, &pat); 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(&rs, ref, send_mirror, FROM_DST, NULL); if (src_name) { if (!src_ref_index.nr) prepare_ref_index(&src_ref_index, src); -- 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(&query, 0, sizeof(struct refspec_item)); query.src = matched->name; - if (!query_refspecs(remote->push.items, remote->push.nr, &query) && - query.dst) { + if (!query_refspecs(&remote->push, &query) && query.dst) { struct strbuf buf = STRBUF_INIT; strbuf_addf(&buf, "%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 = &refs[i]; + for (i = 0; i < rs->nr; i++) { + struct refspec_item *refspec = &rs->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(&query, 0, sizeof(struct refspec_item)); query.src = (char *)name; - if (query_refspecs(rs->items, rs->nr, &query)) + if (query_refspecs(rs, &query)) 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(&remote->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 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(&states->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(&states->remote->fetch, fetch_map); for (ref = stale_refs; ref; ref = ref->next) { struct string_list_item *item = string_list_append(&states->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 = &refs[i]; + for (i = 0; i < rs->nr; i++) { + struct refspec_item *refspec = &rs->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(&query, 0, sizeof(struct refspec_item)); query.dst = (char *)refname; - query_refspecs_multiple(info->refs, info->ref_count, &query, &matches); + query_refspecs_multiple(info->rs, &query, &matches); 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 = &ref_names; info.stale_refs_tail = &stale_refs; - info.refs = refs; - info.ref_count = ref_count; + info.rs = rs; for (ref = fetch_map; ref; ref = ref->next) string_list_append(&ref_names, ref->name); string_list_sort(&ref_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, &autotags); + ref_map = get_ref_map(transport, rs->items, rs->nr, tags, &autotags); 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, &rs); refspec_clear(&rs); 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(&rs[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(&data->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 ", &arg)) { - ALLOC_GROW(refspecs, - refspec_nr + 1, - refspec_alloc); - refspecs[refspec_nr++] = xstrdup(arg); + else if (skip_prefix(capname, "refspec ", &arg)) { + refspec_append(&data->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(&buf); @@ -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(&data->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(&buf, &ref, 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, &ref->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 does
[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(&refmap, 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, &fetch_refspec[i], &oref_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, &remote->fetch[i], &tail, 0); - if (remote->fetch[i].dst && - remote->fetch[i].dst[0]) + for (i = 0; i < remote->fetch.nr; i++) { + get_fetch_map(remote_refs, &remote->fetch.items[i], &tail, 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, &tail, 1)) + for (i = 0; i < states->remote->fetch.nr; i++) + if (get_fetch_map(remote_refs, &states->remote->fetch.items[i], &tail, 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(&states->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(&states->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(&buf); strbuf_addf(&buf, "remote.%s.fetch", remote->name); - for (i = 0; i < remote->fetch_refspec_nr; i++) -
[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(&tmp_rs, refspec, refspec_nr); + for (i = 0; i < tmp_rs.nr; i++) { + const struct refspec_item *item = &tmp_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, &ref_prefixes); argv_array_clear(&ref_prefixes); - free_refspec(refspec_nr, tmp_rs); + refspec_clear(&tmp_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, refspec_names, nr_refspec); + + for (i = 0; i < refspec.nr; i++) { + struct refspec_item *rs = &refspec.items[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(&refspec); 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, &fetch_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(&refspec, fetch_refspec_str, REFSPEC_FETCH); + refspec_item_clear(&refspec); + 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, &refspec); - spec_src = spec->src; + refspec_item_init(&spec, refspec, REFSPEC_FETCH); + spec_src = spec.src; if (!*spec_src || !strcmp(spec_src, "HEAD")) spec_src = "HEAD"; else if (skip_prefix(spec_src, "heads/", &spec_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(&spec); 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(&item, 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(&rs->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 = &s_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; + int is_glob; +
[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(&refspec, argv + 2, argc - 2); + + for (i = 0; i < refspec.nr; i++) { + const struct refspec_item *rs = &refspec.items[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(&refspec); } 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 = &s_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, &unused)) + 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) { + return
[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(&query, 0, sizeof(struct refspec)); + struct refspec_item query; + memset(&query, 0, sizeof(struct refspec_item)); query.dst = tracking_branch; return !remote_find_tracking(remote, &query); } 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 = &refspecs[i]; + struct refspec_item *refspec = &refspecs[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(&ref_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.
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 it... >
[PATCH 1/1] Inform about fast-forwarding of submodules during merge
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
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
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-
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
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
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(&default_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, &max_jobs); > + config_from_gitmodules(gitmodules_update_clone_config, the_repository, > &max_jobs); > git_config(gitmodules_update_clone_config, &max_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); > +
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 ?
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
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
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
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
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-
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(&maybe_trees, commit) = tree; + return tree; +} + +struct tree *get_maybe_tree(const struct commit *commit) +{ + return *maybe_tree_at(&maybe_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(&oid); - - return c->maybe_tree; + return set_maybe_tree(c, lookup_tree(&oid)); } 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 (get_
git diff: meaning of ^M at line ends ?
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< --
благодаря
Здравствуйте, дорогой, мне нужен ваш срочный ответ. У меня есть хорошая новость для вас. Мишель благодаря
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
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(&prev_oid, &cur_oid) >= 0) @@ -892,6 +898,10 @@ int verify_commit_graph(struct commit_graph *g) cur_fanout_pos++; } + + graph_commit = lookup_commit(&cur_oid); + if (!parse_commit_in_graph_one(g, graph_commit)) + graph_report("failed to parse %s from commit-graph", oid_to_hex(&cur_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(&cur_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(&cur_oid)); + continue; + } + + if (oidcmp(&get_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(&cur_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(&cur_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(&cur_oid), +num_parents); + + if (oidcmp(&graph_parents->item->object.oid, &odb_parents->item->object.oid)) + graph_report("commit-graph parent for %s is %s != %s", +oid_to_hex(&cur_oid), + oid_to_hex(&graph_parents->item->object.oid), + oid_to_hex(&odb_parents->item->object.oid)); + + graph_parents = graph_parents->next; + odb_parents
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(&sb, fmt, ap); + + fprintf(stderr, "%s\n", sb.buf); + strbuf_release(&sb); + va_end(ap); +} That's a good idea. Makes that patch a bit less trivial and this one a bit less difficult.
Isten áldjon
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 by generation number
Re: [PATCH 4/4] mark_parents_uninteresting(): avoid most allocation
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. 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: [PATCH v3] add status config and command line options for rename detection
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(&rev.prune_data, &s->pathspec); run_diff_files(&rev, 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(&rev.prune_data, &s->pathspec); run_diff_index(&rev, 1); } @@ -982,6 +991,9 @@ static void wt_longstatus_print_verbose(struct wt_status *s) setup_revisions(0, NULL, &rev, &opt); 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_
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(&pending, l->item); + commit_stack_push(pending, l->item); +} - while (pending.nr > 0) { - struct commit *commit = commit_stack_pop(&pending); +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, &pending); - /* -* 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(&pending, l->item); - } + while (pending.nr > 0) + mark_one_parent_uninteresting(commit_stack_pop(&pending), + &pending); commit_stack_clear(&pending); }
[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(&the_index)) die(_("Please stage your changes to .gitmodules or stash them to proceed")); + if (is_gitmodules_hidden(&the_index)) + die(_("cannot work with hidden submodule config")); strbuf_addf(&submodule_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(&the_index)) + if (list.entry[list.nr++].is_submodule) { + if (!is_staging_gitmodules_ok(&the_index)) die (_("Please stage your changes to .gitmodules or stash them to proceed")); + if (is_gitmodules_hidden(&the_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(&the_index)) + die(_("cannot stage changes to hidden submodule config")); + stage_updated_gitmodules(&the_index); if (write_locked_index(&the_index, &lock_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, &oid) >= 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/co
[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(&default_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, &max_jobs); + config_from_gitmodules(gitmodules_update_clone_config, the_repository, &max_jobs); git_config(gitmodules_update_clone_config, &max_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", &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(&lock_file, LOCK_DIE_ON_ERROR); + + if (read_cache() < 0) + die(_("index file corrupt")); + + stage_updated_gitmodules(&the_index); + + if (write_locked_index(&the_index, &lock_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], &key, 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