Re: Help using git subtree
Al 30/07/13 11:58, En/na Fredrik Gustafsson ha escrit: Git subtree is very convenient to get all the sub-projects into the main-project directory tree and to send back sub-project commits to the corresponding repository. But I don't understand the work flow very well. I haven't used subtree that much however how do you commit to upstream? AFAIK every commit to the subtree must be done in the subtree branch and then merged back to the "superproject"-branch with a subtree merge. I use "git subtree pull" and "git subtree push". AFAIK with subtree all commits are in the same repository, thus one does not need to go to the subtree branch to commit. This is one of the advantages of subtree. I can add a subtree with a given prefix and pull/push my commits to the repository, but how do I do to pull/push commits from the main-project to the corresponding repository avoiding sub-projects to be sent? You don't. A subtree is a subtree in your git-repo and is therefore a part of your repository. When you're using subtree that project will be a part of you project. I expected that there would be a way to split the main project and push it, in the same way that one can split one subtree and push it. If not, this could be a feature request. An other question I have is when will be subtree included in git packages. I had to compile git from sources in order to get this feature. If you're asking when git-subtree will move from contrib/ to core git, I don't know. That's a decision for Junio. I would argue that it shouldn't because submodules can do everything that subtrees are good at and git.git should focus on doing one solution well instead of doing the same thing in two bad ways. However since git.git already uses a subtree-like style for gitk, git-gui, etc. it doesn't seem like Junio agrees with me on this point. Right, I rephrase: When git-subtree will move from contrib/ to core git? Let me also empathize that I find git-subtree much more convenient than sub-modules. One can work with a single repository as it were a single project, one can commit changes that affect different sub-projects with just one command, use common tags and branches and so on. Si heu rebut aquest correu per error, us informo que pot contenir informació confidencial i privada i que està prohibit el seu ús. Us agrairiem que ho comuniqueu al remitent i l'elimineu. Gràcies. Si ha recibido este correo por error, le informo de que puede contener información confidencial y privada y que está prohibido su uso. Le agradeceré que lo comunique a su remitente y lo elimine. Gracias. If you have received this e-mail by error, please note that it may contain confidential and private information, therefore, the use of this information is strictly forbidden. Please inform the sender of the error and delete the information received. Thank you. Is this really something to have in e-mails to a public list? Some lists won't accept this type of e-mails, or simply just ignore you. Don't worry about this signature, it only applies if one receives this e-mail by mistake. Best regards, Gabriel -- Si heu rebut aquest correu per error, us informo que pot contenir informació confidencial i privada i que està prohibit el seu ús. Us agrairiem que ho comuniqueu al remitent i l'elimineu. Gràcies. Si ha recibido este correo por error, le informo de que puede contener información confidencial y privada y que está prohibido su uso. Le agradeceré que lo comunique a su remitente y lo elimine. Gracias. If you have received this e-mail by error, please note that it may contain confidential and private information, therefore, the use of this information is strictly forbidden. Please inform the sender of the error and delete the information received. Thank you. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Remove sq_quote_print() in favor of *_buf
Junio C Hamano wrote: > Having said all that, the patch texts all look OK, so I'll queue > them with updated log messages. It was the usual me reacting to > unjustified value judgement made in log messages and cover letters. Thanks. I'll look at how the log messages are different in the queued version, and adapt accordingly next time. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/5] submodule.c: add .gitmodules staging helper functions
Am 30.07.2013 23:37, schrieb Junio C Hamano: > Jens Lehmann writes: > >> +int is_staging_gitmodules_ok() > > Will tweak this to: > > int is_staging_gitmodules_ok(void) > > and fix this as well: > >> +int is_staging_gitmodules_ok(); >> +void stage_updated_gitmodules(void); > > > before queuing. Thanks. As Frederik already noticed, this series is missing at least one strbuf_release(). Will fix both in v4. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 5/5] rm: delete .gitmodules entry of submodules removed from the work tree
Am 30.07.2013 22:15, schrieb Fredrik Gustafsson: > On Tue, Jul 30, 2013 at 09:51:51PM +0200, Jens Lehmann wrote: >> +/* >> + * Try to remove the "submodule." section from .gitmodules where the >> given >> + * path is configured. Return 0 only if a .gitmodules file was found, a >> section >> + * with the correct path= setting was found and we could remove it. >> + */ >> +int remove_path_from_gitmodules(const char *path) >> +{ >> +struct strbuf sect = STRBUF_INIT; >> +struct string_list_item *path_option; >> + >> +if (!file_exists(".gitmodules")) /* Do nothing without .gitmodules */ >> +return -1; >> + >> +if (gitmodules_is_unmerged) >> +die(_("Cannot change unmerged .gitmodules, resolve merge >> conflicts first")); >> + >> +path_option = unsorted_string_list_lookup(&config_name_for_path, path); >> +if (!path_option) { >> +warning(_("Could not find section in .gitmodules where >> path=%s"), path); >> +return -1; >> +} >> +strbuf_addstr(§, "submodule."); >> +strbuf_addstr(§, path_option->util); >> +if (git_config_rename_section_in_file(".gitmodules", sect.buf, NULL) < >> 0) { >> +/* Maybe the user already did that, don't error out here */ >> +warning(_("Could not remove .gitmodules entry for %s"), path); >> +return -1; >> +} >> +strbuf_release(§); >> +return 0; >> +} > > This question applies for this function and a few more functions in this > patch that has the same characteristics. > > If we're in a state when we need to return non-zero, we don't do any > cleaning (that is strbuf_release()). Since this file is in the part > called libgit AFAIK, shouldn't we always clean after us? Right you are, thanks for bringing that up. The last return needs a strbuf_release(), will fix that in v4. > Would it make sense to have different return values for different > errors? I don't think so. The caller only wants to know if the modification of the .gitmodules file happened or not, so he can decide if that needs to be staged. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sha1_file: introduce close_one_pack() to close packs on fd pressure
On Tue, Jul 30, 2013 at 12:52 PM, Jeff King wrote: > On Tue, Jul 30, 2013 at 08:39:48AM -0700, Junio C Hamano wrote: > >> Brandon Casey writes: >> >> > From: Brandon Casey >> > >> > When the number of open packs exceeds pack_max_fds, unuse_one_window() >> > is called repeatedly to attempt to release the least-recently-used >> > pack windows, which, as a side-effect, will also close a pack file >> > after closing its last open window. If a pack file has been opened, >> > but no windows have been allocated into it, it will never be selected >> > by unuse_one_window() and hence its file descriptor will not be >> > closed. When this happens, git may exceed the number of file >> > descriptors permitted by the system. >> >> An interesting find. The patch from a cursory look reads OK. > > Yeah. I wonder if unuse_one_window() should actually leave the pack fd > open now in general. > > If you close packfile descriptors, you can run into racy situations > where somebody else is repacking and deleting packs, and they go away > while you are trying to access them. If you keep a descriptor open, > you're fine; they last to the end of the process. If you don't, then > they disappear from under you. > > For normal object access, this isn't that big a deal; we just rescan the > packs and retry. But if you are packing yourself (e.g., because you are > a pack-objects started by upload-pack for a clone or fetch), it's much > harder to recover (and we print some warnings). > > We had our core.packedGitWindowSize lowered on GitHub for a while, and > we ran into this warning on busy repositories when we were running "git > gc" on the server. We solved it by bumping the window size so we never > release memory. > > But just not closing the descriptor wouldn't work until Brandon's patch, > because we used the same function to release memory and descriptor > pressure. Now we could do them separately (and progressively if we need > to). I had thought about whether to stop closing the pack file in unuse_one_window(), but didn't have a reason to do so. I think the scenario you described provides a justification. If we're not under file descriptor pressure and we can possibly avoid rescanning the pack directory, it sounds like a net win. >> > This is not likely to occur during upload-pack since upload-pack >> > reads each object from the pack so that it can peel tags and >> > advertise the exposed object. >> >> Another interesting find. Perhaps there is a room for improvements, >> as packed-refs file knows what objects the tags peel to? I vaguely >> recall Peff was actively reducing the object access during ref >> enumeration in not so distant past... > > Yeah, we should be reading almost no objects these days due to the > packed-refs peel lines. I just did a double-check on what "git > upload-pack . /dev/null" reads on my git.git repo, and it is > only three objects: the v1.8.3.3, v1.8.3.4, and v1.8.4-rc0 tag objects. > In other words, the tags I got since the last time I ran "git gc". So I > think all is working as designed. Ok, looks like this has been the case since your 435c8332 which taught upload-pack to use peel_ref(). So looks like we do avoid reaching into the pack for any ref that was read from a (modern) packed-refs file. The repository I was testing with had mostly loose refs. Indeed, after packing refs, upload-pack encounters the same problem as receive-pack and runs out of file descriptors. So my comment about upload-pack is not completely accurate. Upload-pack _can_ run into this problem, but the refs must be packed, as well as there being enough of them that exist in enough different pack files to exceed the processes fd limit. > We could give receive-pack the same treatment; I've spent less time > micro-optimizing it because because we (and most sites, I would think) > get an order of magnitude more fetches than pushes. I don't think it would need the 435c8332 treatment since receive-pack doesn't peel refs when it advertises them to the client and hence does not need to load the ref object from the pack file during ref advertisement, but possibly some of the other stuff you did would be applicable. But like you said, the number of fetches far exceed the number of pushes. -Brandon -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times
Stefan Beller writes: > Here is just another idea: > if (cmdmode == 'v') > This may be hard to read, (What is 'v'? I cannot remember > all the alphabet ;)) So maybe we could have an enum instead of > the last parameter? > OPT_CMDMODE( short, long, variable, description, enum) I actually was thinking about going totally in the opposite direction. People who grew up with the old Unix tradition getopt(3) are used to code like this: while ((opt = getopt(ac, av, "abcde")) != -1) { switch (opt) { case 'a': perform_a(); break; case 'b': perform_b(); break; ... } } In other words, the "enum" is most convenient if it matches the "short" option, so instead of having to repeat ourselves over and over, like I did in that illustration patch for builtin/tag.c, e.g. OPT_CMDMODE('d', "delete", &cmdmode, N_("delete tags"), 'd'), OPT_CMDMODE('v', "verify", &cmdmode, N_("verify tags"), 'v'), we could just do #define OPT_CMDMODE(s, l, v, h) \ { OPTION_CMDMODE, (s), (l), (v), NULL, \ (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (s) } -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times
Stefan Beller writes: > Your approach seems more like what we really want, however I'd have > some points: > * Is it a good idea to have so many different OPT_MODE or >OPTION_MODE defines? In my attempts I tried to reuse existing >OPTION_s to not pollute the parsing infrastructure with more >lines of code. ;) > > * You can only have one OPTION_CMDMODE in one argv vector right? That is not what I intended, at least. int one = 0, two = 0; struct option options[] = { OPT_CMDMODE('a', NULL, &one, N_("set one to a"), 'a'), OPT_CMDMODE('b', NULL, &one, N_("set one to b"), 'b'), OPT_CMDMODE('c', NULL, &two, N_("set two to c"), 'c'), OPT_CMDMODE('d', NULL, &two, N_("set two to d"), 'd'), OPT_END() } should give you two independent sets of modes, one and two. The only reason I needed to add an extra parameter to get_value() was so that I can tell the former two and the latter two belong to different groups, and that is done by looking at the address of the variable. In opt_command_mode_error(), opt->value == that->value is used as a condition to see if the other option is possibly the one that was used previously, which conflicted with us. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/5] submodule.c: add .gitmodules staging helper functions
Jens Lehmann writes: > +int is_staging_gitmodules_ok() Will tweak this to: int is_staging_gitmodules_ok(void) and fix this as well: > +int is_staging_gitmodules_ok(); > +void stage_updated_gitmodules(void); before queuing. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times
On 07/30/13 21:24, Junio C Hamano wrote: > > ... and then "git tag" may become like so. > > builtin/tag.c | 27 --- > 1 file changed, 12 insertions(+), 15 deletions(-) > > diff --git a/builtin/tag.c b/builtin/tag.c > index af3af3f..d8ae5aa 100644 > --- a/builtin/tag.c > +++ b/builtin/tag.c > @@ -436,18 +436,18 @@ int cmd_tag(int argc, const char **argv, const char > *prefix) > struct ref_lock *lock; > struct create_tag_options opt; > char *cleanup_arg = NULL; > - int annotate = 0, force = 0, lines = -1, list = 0, > - delete = 0, verify = 0; > + int annotate = 0, force = 0, lines = -1; > + int cmdmode = 0; > const char *msgfile = NULL, *keyid = NULL; > struct msg_arg msg = { 0, STRBUF_INIT }; > struct commit_list *with_commit = NULL; > struct option options[] = { > - OPT_BOOLEAN('l', "list", &list, N_("list tag names")), > + OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'), > { OPTION_INTEGER, 'n', NULL, &lines, N_("n"), > N_("print lines of each tag message"), > PARSE_OPT_OPTARG, NULL, 1 }, > - OPT_BOOLEAN('d', "delete", &delete, N_("delete tags")), > - OPT_BOOLEAN('v', "verify", &verify, N_("verify tags")), > + OPT_CMDMODE('d', "delete", &cmdmode, N_("delete tags"), 'd'), > + OPT_CMDMODE('v', "verify", &cmdmode, N_("verify tags"), 'v'), > > OPT_GROUP(N_("Tag creation options")), > OPT_BOOLEAN('a', "annotate", &annotate, > @@ -489,22 +489,19 @@ int cmd_tag(int argc, const char **argv, const char > *prefix) > } > if (opt.sign) > annotate = 1; > - if (argc == 0 && !(delete || verify)) > - list = 1; > + if (argc == 0 && !cmdmode) > + cmdmode = 'l'; > > - if ((annotate || msg.given || msgfile || force) && > - (list || delete || verify)) > + if ((annotate || msg.given || msgfile || force) && (cmdmode != 0)) > usage_with_options(git_tag_usage, options); > > - if (list + delete + verify > 1) > - usage_with_options(git_tag_usage, options); > finalize_colopts(&colopts, -1); > - if (list && lines != -1) { > + if (cmdmode == 'l' && lines != -1) { > if (explicitly_enable_column(colopts)) > die(_("--column and -n are incompatible")); > colopts = 0; > } > - if (list) { > + if (cmdmode == 'l') { > int ret; > if (column_active(colopts)) { > struct column_options copts; > @@ -523,9 +520,9 @@ int cmd_tag(int argc, const char **argv, const char > *prefix) > die(_("--contains option is only allowed with -l.")); > if (points_at.nr) > die(_("--points-at option is only allowed with -l.")); > - if (delete) > + if (cmdmode == 'd') > return for_each_tag_name(argv, delete_tag); > - if (verify) > + if (cmdmode == 'v') > return for_each_tag_name(argv, verify_tag); > > if (msg.given || msgfile) { Here is just another idea: if (cmdmode == 'v') This may be hard to read, (What is 'v'? I cannot remember all the alphabet ;)) So maybe we could have an enum instead of the last parameter? OPT_CMDMODE( short, long, variable, description, enum) Also the variable would then only need to be an enum accepting variable, and not an integer accepting all integer range, so we'd also catch typos or wrong values in such a case: > + if (argc == 0 && !cmdmode) > + cmdmode = 'l'; // maybe 'l' is a typo and not existing? signature.asc Description: OpenPGP digital signature
Re: [PATCH] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times
On 07/30/13 21:24, Junio C Hamano wrote: > Stefan Beller writes: > >> As of b04ba2bb (parse-options: deprecate OPT_BOOLEAN, 2011-09-27), >> the OPT_BOOLEAN was deprecated. >> While I am going to replace the OPT_BOOLEAN by the proposed OPT_BOOL or >> the OPT_COUNTUP to keep existing behavior, this commit is actually a >> bug fix! >> >> In line 499 we have: >> if (list + delete + verify > 1) >> usage_with_options(git_tag_usage, options); >> Now if we give one of the options twice, we'll get the usage information. >> (i.e. 'git tag --verify --verify ' and >> 'git --delete --delete ' yield usage information and do not >> do the intended command.) >> >> This could have been fixed by rewriting the line to >> if (!!list + !!delete + !!verify > 1) >> usage_with_options(git_tag_usage, options); >> or as it happened in this patch by having the parameters not >> counting up for each occurrence, but the OPT_BOOL just setting the >> variables to either 0 if the option is not given or 1 if the option is >> given multiple times. > > Makes twisted sort of sense ;-). > >> However we could discuss if the negated options do make sense >> here, or if we don't want to allow them here, as this seems valid >> (before and after this patch): >> >> git tag --no-verify --delete > > It probably does not. As you hinted in your earlier patch, we may > want to introduce a "only can set to true" boolean used solely to > specify these things. They are disguised as "options", but are in > fact command operation modes that are often mutually exclusive. > > For these operation modes that are mutually exclusive, there are > multiple possible implementations: > > * One OPT_BOOL_NONEG per option; the code ensures the mutual >exclusion with "if (list + delete + verify > 1)"; > > * One OPT_BIT per option in a single variable; the code ensures the >mutual exclusion with count_bits, which may be a lot more >cumbersome; > > * OPT_SET_INT that updates a single variable to enum; instead of >making it an error to give two conflicting modes, this would give >us the last-one-wins rule. > > Unlike usual "options", we generally do not want the last-one-wins > semantics for command operation modes, I think. > > Perhaps we would want something like this? > > -- >8 -- > Subject: [PATCH] parse-options: add OPT_CMDMODE() > > This can be used to define a set of mutually exclusive "command > mode" options, and automatically catch use of more than one from > that set as an error. > > Signed-off-by: Junio C Hamano > --- > parse-options.c | 58 > - > parse-options.h | 3 +++ > 2 files changed, 56 insertions(+), 5 deletions(-) > > diff --git a/parse-options.c b/parse-options.c > index c2cbca2..62e9b1c 100644 > --- a/parse-options.c > +++ b/parse-options.c > @@ -43,8 +43,42 @@ static void fix_filename(const char *prefix, const char > **file) > *file = xstrdup(prefix_filename(prefix, strlen(prefix), *file)); > } > > +static int opt_command_mode_error(const struct option *opt, > + const struct option *all_opts, > + int flags) > +{ > + const struct option *that; > + struct strbuf message = STRBUF_INIT; > + struct strbuf that_name = STRBUF_INIT; > + > + /* > + * Find the other option that was used to set the variable > + * already, and report that this is not compatible with it. > + */ > + for (that = all_opts; that->type != OPTION_END; that++) { > + if (that == opt || > + that->type != OPTION_CMDMODE || > + that->value != opt->value || > + that->defval != *(int *)opt->value) > + continue; > + > + if (that->long_name) > + strbuf_addf(&that_name, "--%s", that->long_name); > + else > + strbuf_addf(&that_name, "-%c", that->short_name); > + strbuf_addf(&message, ": incompatible with %s", that_name.buf); > + strbuf_release(&that_name); > + opterror(opt, message.buf, flags); > + strbuf_release(&message); > + return -1; > + } > + return opterror(opt, ": incompatible with something else", flags); > +} > + > static int get_value(struct parse_opt_ctx_t *p, > - const struct option *opt, int flags) > + const struct option *opt, > + const struct option *all_opts, > + int flags) > { > const char *s, *arg; > const int unset = flags & OPT_UNSET; > @@ -83,6 +117,16 @@ static int get_value(struct parse_opt_ctx_t *p, > *(int *)opt->value = unset ? 0 : opt->defval; > return 0; > > + case OPTION_CMDMODE: > + /* > + * Giving the same mode option twice, although is unnecessary, > + * is not a
Re: [PATCH 5/3] revert most of the http_options() change
"Kyle J. McKay" writes: > And now all the t5200-url-normalize tests pass again. > > FYI, I couldn't get the patches to apply against next or pu without > some minor tweaks that were just conflict resolutions having to do > with git_config_with_options changing its signature. Thanks. I built these five patches directly on top of your series, i.e. on top of cea9928d (docs: update http..* options documentation, 2013-07-25). I'll rebuild the series with your fixes and I may even queue it to 'pu', but with some random thoughts: * "url-match.[ch]" are ugly names. I'd be happier with "urlmatch.[ch]". * In the end result, http_options() looks mostly identical to that of the 'master', but it got an extra "void *matched", only to support "git config --get-urlmatch". I however do not have to, if I do a few things: - Instead of implementing urlmatch_config_item that extends urlmatch_item, have a separate string_list, keyed by the variable names, and point the string_list with the generic cb pointer I already have in urlmatch_config. The fn() on the "git config" side has to index this second string_list with the variable and keep track of the value from the best candidate we have seen so far. - The above allows us to lose item_alloc and item_clear callback functions from urlmatch_config, as we will not be doing the structure inheritance to extend urlmatch_item. - We still need cascade_fn callback, though. * With the above, http_options() does not have to change in the series. We could restructure the series perhaps this way: - http.sslCertPasswordProtected parsing fix. - Add urlmatch.[ch], which would be your "config: improve support for http..* settings" and yesterday's "url-match: split out URL matching logic out of http.c", and a half of "url-match: generalize configuration collection logic". - Update http.c to use urlmatch.[ch] to make http.c match the result of the endgame patch [5/3], and add necessary end user documentation to Documentation/config.txt. - Add test-url-normalize and t/t5200 - Update builtin/config.c for "--get-urlmatch", to make builtin/config.c match the result of the endgame patch [5/3], add end user documentation to Documentation/git-config.txt. - Add some tests for "git config --get-urlmatch". -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 5/5] rm: delete .gitmodules entry of submodules removed from the work tree
On Tue, Jul 30, 2013 at 09:51:51PM +0200, Jens Lehmann wrote: > +/* > + * Try to remove the "submodule." section from .gitmodules where the > given > + * path is configured. Return 0 only if a .gitmodules file was found, a > section > + * with the correct path= setting was found and we could remove it. > + */ > +int remove_path_from_gitmodules(const char *path) > +{ > + struct strbuf sect = STRBUF_INIT; > + struct string_list_item *path_option; > + > + if (!file_exists(".gitmodules")) /* Do nothing without .gitmodules */ > + return -1; > + > + if (gitmodules_is_unmerged) > + die(_("Cannot change unmerged .gitmodules, resolve merge > conflicts first")); > + > + path_option = unsorted_string_list_lookup(&config_name_for_path, path); > + if (!path_option) { > + warning(_("Could not find section in .gitmodules where > path=%s"), path); > + return -1; > + } > + strbuf_addstr(§, "submodule."); > + strbuf_addstr(§, path_option->util); > + if (git_config_rename_section_in_file(".gitmodules", sect.buf, NULL) < > 0) { > + /* Maybe the user already did that, don't error out here */ > + warning(_("Could not remove .gitmodules entry for %s"), path); > + return -1; > + } > + strbuf_release(§); > + return 0; > +} This question applies for this function and a few more functions in this patch that has the same characteristics. If we're in a state when we need to return non-zero, we don't do any cleaning (that is strbuf_release()). Since this file is in the part called libgit AFAIK, shouldn't we always clean after us? Would it make sense to have different return values for different errors? I do like the comments above the function, more functions (at least non-static ones) should follow this good style IMHO. -- Med vänliga hälsningar Fredrik Gustafsson tel: 0733-608274 e-post: iv...@iveqy.com -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sha1_file: introduce close_one_pack() to close packs on fd pressure
On Tue, Jul 30, 2013 at 08:39:48AM -0700, Junio C Hamano wrote: > Brandon Casey writes: > > > From: Brandon Casey > > > > When the number of open packs exceeds pack_max_fds, unuse_one_window() > > is called repeatedly to attempt to release the least-recently-used > > pack windows, which, as a side-effect, will also close a pack file > > after closing its last open window. If a pack file has been opened, > > but no windows have been allocated into it, it will never be selected > > by unuse_one_window() and hence its file descriptor will not be > > closed. When this happens, git may exceed the number of file > > descriptors permitted by the system. > > An interesting find. The patch from a cursory look reads OK. Yeah. I wonder if unuse_one_window() should actually leave the pack fd open now in general. If you close packfile descriptors, you can run into racy situations where somebody else is repacking and deleting packs, and they go away while you are trying to access them. If you keep a descriptor open, you're fine; they last to the end of the process. If you don't, then they disappear from under you. For normal object access, this isn't that big a deal; we just rescan the packs and retry. But if you are packing yourself (e.g., because you are a pack-objects started by upload-pack for a clone or fetch), it's much harder to recover (and we print some warnings). We had our core.packedGitWindowSize lowered on GitHub for a while, and we ran into this warning on busy repositories when we were running "git gc" on the server. We solved it by bumping the window size so we never release memory. But just not closing the descriptor wouldn't work until Brandon's patch, because we used the same function to release memory and descriptor pressure. Now we could do them separately (and progressively if we need to). > > This is not likely to occur during upload-pack since upload-pack > > reads each object from the pack so that it can peel tags and > > advertise the exposed object. > > Another interesting find. Perhaps there is a room for improvements, > as packed-refs file knows what objects the tags peel to? I vaguely > recall Peff was actively reducing the object access during ref > enumeration in not so distant past... Yeah, we should be reading almost no objects these days due to the packed-refs peel lines. I just did a double-check on what "git upload-pack . /dev/null" reads on my git.git repo, and it is only three objects: the v1.8.3.3, v1.8.3.4, and v1.8.4-rc0 tag objects. In other words, the tags I got since the last time I ran "git gc". So I think all is working as designed. We could give receive-pack the same treatment; I've spent less time micro-optimizing it because because we (and most sites, I would think) get an order of magnitude more fetches than pushes. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 5/5] rm: delete .gitmodules entry of submodules removed from the work tree
Currently using "git rm" on a submodule removes the submodule's work tree from that of the superproject and the gitlink from the index. But the submodule's section in .gitmodules is left untouched, which is a leftover of the now removed submodule and might irritate users (as opposed to the setting in .git/config, this must stay as a reminder that the user showed interest in this submodule so it will be repopulated later when an older commit is checked out). Let "git rm" help the user by not only removing the submodule from the work tree but by also removing the "submodule." section from the .gitmodules file and stage both. This doesn't happen when the "--cached" option is used, as it would modify the work tree. This also silently does nothing when no .gitmodules file is found and only issues a warning when it doesn't have a section for this submodule. This is because the user might just use plain gitlinks without the .gitmodules file or has already removed the section by hand before issuing the "git rm" command (in which case the warning reminds him that rm would have done that for him). Only when .gitmodules is found and contains merge conflicts the rm command will fail and tell the user to resolve the conflict before trying again. Also extend the man page to inform the user about this new feature. While at it promote the submodule sub-section to a chapter as it made not much sense under "REMOVING FILES THAT HAVE DISAPPEARED FROM THE FILESYSTEM". In t7610 three uses of "git rm submod" had to be replaced with "git rm --cached submod" because that test expects .gitmodules and the work tree to stay untouched. Also in t7400 the tests for the remaining settings in the .gitmodules file had to be changed to assert that these settings are missing. Signed-off-by: Jens Lehmann --- Documentation/git-rm.txt | 8 ++-- builtin/rm.c | 19 +++-- submodule.c| 32 +++ submodule.h| 1 + t/t3600-rm.sh | 98 +++--- t/t7400-submodule-basic.sh | 14 ++- t/t7610-mergetool.sh | 6 +-- 7 files changed, 153 insertions(+), 25 deletions(-) diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt index 1d876c2..9d731b4 100644 --- a/Documentation/git-rm.txt +++ b/Documentation/git-rm.txt @@ -134,14 +134,16 @@ use the following command: git diff --name-only --diff-filter=D -z | xargs -0 git rm --cached -Submodules -~~ +SUBMODULES +-- Only submodules using a gitfile (which means they were cloned with a Git version 1.7.8 or newer) will be removed from the work tree, as their repository lives inside the .git directory of the superproject. If a submodule (or one of those nested inside it) still uses a .git directory, `git rm` will fail - no matter if forced -or not - to protect the submodule's history. +or not - to protect the submodule's history. If it exists the +submodule. section in the linkgit:gitmodules[5] file will also +be removed and that file will be staged (unless --cached or -n are used). A submodule is considered up-to-date when the HEAD is the same as recorded in the index, no tracked files are modified and no untracked diff --git a/builtin/rm.c b/builtin/rm.c index df85f98..a9d45dd 100644 --- a/builtin/rm.c +++ b/builtin/rm.c @@ -283,6 +283,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix) struct pathspec pathspec; char *seen; + gitmodules_config(); git_config(git_default_config, NULL); argc = parse_options(argc, argv, prefix, builtin_rm_options, @@ -324,7 +325,10 @@ int cmd_rm(int argc, const char **argv, const char *prefix) continue; ALLOC_GROW(list.entry, list.nr + 1, list.alloc); list.entry[list.nr].name = ce->name; - list.entry[list.nr++].is_submodule = S_ISGITLINK(ce->ce_mode); + list.entry[list.nr].is_submodule = S_ISGITLINK(ce->ce_mode); + if (list.entry[list.nr++].is_submodule && + !is_staging_gitmodules_ok()) + die (_("Please, stage your changes to .gitmodules or stash them to proceed")); } if (pathspec.nr) { @@ -396,13 +400,15 @@ int cmd_rm(int argc, const char **argv, const char *prefix) * in the middle) */ if (!index_only) { - int removed = 0; + int removed = 0, gitmodules_modified = 0; for (i = 0; i < list.nr; i++) { const char *path = list.entry[i].name; if (list.entry[i].is_submodule) { if (is_empty_dir(path)) { if (!rmdir(path)) { removed = 1; + if (!remove_path_from_gitmodules(path)) +
[PATCH v3 4/5] Teach mv to update the path entry in .gitmodules for moved submodules
Currently using "git mv" on a submodule moves the submodule's work tree in that of the superproject. But the submodule's path setting in .gitmodules is left untouched, which is now inconsistent with the work tree and makes git commands that rely on the proper path -> name mapping (like status and diff) behave strangely. Let "git mv" help here by not only moving the submodule's work tree but also updating the "submodule..path" setting from the .gitmodules file and stage both. This doesn't happen when no .gitmodules file is found and only issues a warning when it doesn't have a section for this submodule. This is because the user might just use plain gitlinks without the .gitmodules file or has already updated the path setting by hand before issuing the "git mv" command (in which case the warning reminds him that mv would have done that for him). Only when .gitmodules is found and contains merge conflicts the mv command will fail and tell the user to resolve the conflict before trying again. Also extend the man page to inform the user about this new feature. Signed-off-by: Jens Lehmann --- Documentation/git-mv.txt | 2 ++ builtin/mv.c | 10 ++- submodule.c | 33 + submodule.h | 1 + t/t7001-mv.sh| 75 5 files changed, 120 insertions(+), 1 deletion(-) diff --git a/Documentation/git-mv.txt b/Documentation/git-mv.txt index 1f6fce0..b1f7988 100644 --- a/Documentation/git-mv.txt +++ b/Documentation/git-mv.txt @@ -49,6 +49,8 @@ SUBMODULES Moving a submodule using a gitfile (which means they were cloned with a Git version 1.7.8 or newer) will update the gitfile and core.worktree setting to make the submodule work in the new location. +It also will attempt to update the submodule..path setting in +the linkgit:gitmodules[5] file and stage that file (unless -n is used). GIT --- diff --git a/builtin/mv.c b/builtin/mv.c index 68b7060..7dd6bb4 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -58,7 +58,7 @@ static struct lock_file lock_file; int cmd_mv(int argc, const char **argv, const char *prefix) { - int i, newfd; + int i, newfd, gitmodules_modified = 0; int verbose = 0, show_only = 0, force = 0, ignore_errors = 0; struct option builtin_mv_options[] = { OPT__VERBOSE(&verbose, N_("be verbose")), @@ -72,6 +72,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix) struct stat st; struct string_list src_for_dst = STRING_LIST_INIT_NODUP; + gitmodules_config(); git_config(git_default_config, NULL); argc = parse_options(argc, argv, prefix, builtin_mv_options, @@ -125,6 +126,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix) struct strbuf submodule_dotgit = STRBUF_INIT; if (!S_ISGITLINK(active_cache[first]->ce_mode)) die (_("Huh? Directory %s is in index and no submodule?"), src); + if (!is_staging_gitmodules_ok()) + die (_("Please, stage your changes to .gitmodules or stash them to proceed")); strbuf_addf(&submodule_dotgit, "%s/.git", src); submodule_gitfile[i] = read_gitfile(submodule_dotgit.buf); if (submodule_gitfile[i]) @@ -229,6 +232,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix) die_errno (_("renaming '%s' failed"), src); if (submodule_gitfile[i]) connect_work_tree_and_git_dir(dst, submodule_gitfile[i]); + if (!update_path_in_gitmodules(src, dst)) + gitmodules_modified = 1; } if (mode == WORKING_DIRECTORY) @@ -240,6 +245,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix) rename_cache_entry_at(pos, dst); } + if (gitmodules_modified) + stage_updated_gitmodules(); + if (active_cache_changed) { if (write_cache(newfd, active_cache, active_nr) || commit_locked_index(&lock_file)) diff --git a/submodule.c b/submodule.c index 584f7de..b210685 100644 --- a/submodule.c +++ b/submodule.c @@ -47,6 +47,39 @@ int is_staging_gitmodules_ok() return !gitmodules_is_modified; } +/* + * Try to update the "path" entry in the "submodule." section of the + * .gitmodules file. Return 0 only if a .gitmodules file was found, a section + * with the correct path= setting was found and we could update it. + */ +int update_path_in_gitmodules(const char *oldpath, const char *newpath) +{ + struct strbuf entry = STRBUF_INIT; + struct string_list_item *path_option; + + if (!file_exists(".gitmodules")) /* Do nothing witho
[PATCH v3 3/5] submodule.c: add .gitmodules staging helper functions
Add the new is_staging_gitmodules_ok() and stage_updated_gitmodules() functions to submodule.c. The first makes it possible for call sites to see if the .gitmodules file did contain any unstaged modifications they would accidentally stage in addition to those they intend to stage themselves. The second function stages all modifications to the .gitmodules file, both will be used by subsequent patches for the mv and rm commands. Signed-off-by: Jens Lehmann --- submodule.c | 51 +++ submodule.h | 2 ++ 2 files changed, 53 insertions(+) diff --git a/submodule.c b/submodule.c index d96d187..584f7de 100644 --- a/submodule.c +++ b/submodule.c @@ -10,6 +10,7 @@ #include "string-list.h" #include "sha1-array.h" #include "argv-array.h" +#include "blob.h" static struct string_list config_name_for_path; static struct string_list config_fetch_recurse_submodules_for_name; @@ -30,6 +31,51 @@ static struct sha1_array ref_tips_after_fetch; */ static int gitmodules_is_unmerged; +/* + * This flag is set if the .gitmodules file had unstaged modifications on + * startup. This must be checked before allowing modifications to the + * .gitmodules file with the intention to stage them later, because when + * continuing we would stage the modifications the user didn't stage herself + * too. That might change in a future version when we learn to stage the + * changes we do ourselves without staging any previous modifications. + */ +static int gitmodules_is_modified; + + +int is_staging_gitmodules_ok() +{ + return !gitmodules_is_modified; +} + +void stage_updated_gitmodules(void) +{ + struct strbuf buf = STRBUF_INIT; + struct stat st; + int pos; + struct cache_entry *ce; + int namelen = strlen(".gitmodules"); + + pos = cache_name_pos(".gitmodules", namelen); + if (pos < 0) { + warning(_("could not find .gitmodules in index")); + return; + } + ce = active_cache[pos]; + ce->ce_flags = namelen; + if (strbuf_read_file(&buf, ".gitmodules", 0) < 0) + die(_("reading updated .gitmodules failed")); + if (lstat(".gitmodules", &st) < 0) + die_errno(_("unable to stat updated .gitmodules")); + fill_stat_cache_info(ce, &st); + ce->ce_mode = ce_mode_from_stat(ce, st.st_mode); + if (remove_cache_entry_at(pos) < 0) + die(_("unable to remove .gitmodules from index")); + if (write_sha1_file(buf.buf, buf.len, blob_type, ce->sha1)) + die(_("adding updated .gitmodules failed")); + if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE)) + die(_("staging updated .gitmodules failed")); +} + static int add_submodule_odb(const char *path) { struct strbuf objects_directory = STRBUF_INIT; @@ -116,6 +162,11 @@ void gitmodules_config(void) !memcmp(ce->name, ".gitmodules", 11)) gitmodules_is_unmerged = 1; } + } else if (pos < active_nr) { + struct stat st; + if (lstat(".gitmodules", &st) == 0 && + ce_match_stat(active_cache[pos], &st, 0) & DATA_CHANGED) + gitmodules_is_modified = 1; } if (!gitmodules_is_unmerged) diff --git a/submodule.h b/submodule.h index 29e9658..244d5f5 100644 --- a/submodule.h +++ b/submodule.h @@ -11,6 +11,8 @@ enum { RECURSE_SUBMODULES_ON = 2 }; +int is_staging_gitmodules_ok(); +void stage_updated_gitmodules(void); void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt, const char *path); int submodule_config(const char *var, const char *value, void *cb); -- 1.8.4.rc0.199.g7079aac -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/5] Teach mv to move submodules using a gitfile
When moving a submodule which uses a gitfile to point to the git directory stored in .git/modules/ of the superproject two changes must be made to make the submodule work: the .git file and the core.worktree setting must be adjusted to point from work tree to git directory and back. Achieve that by remembering which submodule uses a gitfile by storing the result of read_gitfile() of each submodule. If that is not NULL the new function connect_work_tree_and_git_dir() is called after renaming the submodule's work tree which updates the two settings to the new values. Extend the man page to inform the user about that feature (and while at it change the description to not talk about a script anymore, as mv is a builtin for quite some time now). Signed-off-by: Jens Lehmann --- Documentation/git-mv.txt | 8 +++- builtin/mv.c | 19 +++ submodule.c | 31 +++ submodule.h | 1 + t/t7001-mv.sh| 19 +++ 5 files changed, 73 insertions(+), 5 deletions(-) diff --git a/Documentation/git-mv.txt b/Documentation/git-mv.txt index e93fcb4..1f6fce0 100644 --- a/Documentation/git-mv.txt +++ b/Documentation/git-mv.txt @@ -13,7 +13,7 @@ SYNOPSIS DESCRIPTION --- -This script is used to move or rename a file, directory or symlink. +Move or rename a file, directory or symlink. git mv [-v] [-f] [-n] [-k] git mv [-v] [-f] [-n] [-k] ... @@ -44,6 +44,12 @@ OPTIONS --verbose:: Report the names of files as they are moved. +SUBMODULES +-- +Moving a submodule using a gitfile (which means they were cloned +with a Git version 1.7.8 or newer) will update the gitfile and +core.worktree setting to make the submodule work in the new location. + GIT --- Part of the linkgit:git[1] suite diff --git a/builtin/mv.c b/builtin/mv.c index 1d3ef63..68b7060 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -9,6 +9,7 @@ #include "cache-tree.h" #include "string-list.h" #include "parse-options.h" +#include "submodule.h" static const char * const builtin_mv_usage[] = { N_("git mv [options] ... "), @@ -66,7 +67,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix) OPT_BOOLEAN('k', NULL, &ignore_errors, N_("skip move/rename errors")), OPT_END(), }; - const char **source, **destination, **dest_path; + const char **source, **destination, **dest_path, **submodule_gitfile; enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes; struct stat st; struct string_list src_for_dst = STRING_LIST_INIT_NODUP; @@ -85,6 +86,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix) source = internal_copy_pathspec(prefix, argv, argc, 0); modes = xcalloc(argc, sizeof(enum update_mode)); dest_path = internal_copy_pathspec(prefix, argv + argc, 1, 0); + submodule_gitfile = xcalloc(argc, sizeof(char *)); if (dest_path[0][0] == '\0') /* special case: "." was normalized to "" */ @@ -120,8 +122,14 @@ int cmd_mv(int argc, const char **argv, const char *prefix) else if (src_is_dir) { int first = cache_name_pos(src, length); if (first >= 0) { + struct strbuf submodule_dotgit = STRBUF_INIT; if (!S_ISGITLINK(active_cache[first]->ce_mode)) die (_("Huh? Directory %s is in index and no submodule?"), src); + strbuf_addf(&submodule_dotgit, "%s/.git", src); + submodule_gitfile[i] = read_gitfile(submodule_dotgit.buf); + if (submodule_gitfile[i]) + submodule_gitfile[i] = xstrdup(submodule_gitfile[i]); + strbuf_release(&submodule_dotgit); } else { const char *src_w_slash = add_slash(src); int last, len_w_slash = length + 1; @@ -216,9 +224,12 @@ int cmd_mv(int argc, const char **argv, const char *prefix) int pos; if (show_only || verbose) printf(_("Renaming %s to %s\n"), src, dst); - if (!show_only && mode != INDEX && - rename(src, dst) < 0 && !ignore_errors) - die_errno (_("renaming '%s' failed"), src); + if (!show_only && mode != INDEX) { + if (rename(src, dst) < 0 && !ignore_errors) + die_errno (_("renaming '%s' failed"), src); + if (submodule_gitfile[i]) + connect_work_tree_and_git_dir(dst, submodule_gitfile[i]); + } if (mode == WORKING_DIRECTORY) continue; diff --git a/submod
[PATCH v3 1/5] Teach mv to move submodules together with their work trees
Currently the attempt to use "git mv" on a submodule errors out with: fatal: source directory is empty, source=, destination= The reason is that mv searches for the submodule with a trailing slash in the index, which it doesn't find (because it is stored without a trailing slash). As it doesn't find any index entries inside the submodule it claims the directory would be empty even though it isn't. Fix that by searching for the name without a trailing slash and continue if it is a submodule. Then rename() will move the submodule work tree just like it moves a file. Signed-off-by: Jens Lehmann --- builtin/mv.c | 99 +++ t/t7001-mv.sh | 34 2 files changed, 86 insertions(+), 47 deletions(-) diff --git a/builtin/mv.c b/builtin/mv.c index 16ce99b..1d3ef63 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -118,55 +118,60 @@ int cmd_mv(int argc, const char **argv, const char *prefix) && lstat(dst, &st) == 0) bad = _("cannot move directory over file"); else if (src_is_dir) { - const char *src_w_slash = add_slash(src); - int len_w_slash = length + 1; - int first, last; - - modes[i] = WORKING_DIRECTORY; - - first = cache_name_pos(src_w_slash, len_w_slash); - if (first >= 0) - die (_("Huh? %.*s is in index?"), - len_w_slash, src_w_slash); - - first = -1 - first; - for (last = first; last < active_nr; last++) { - const char *path = active_cache[last]->name; - if (strncmp(path, src_w_slash, len_w_slash)) - break; - } - free((char *)src_w_slash); - - if (last - first < 1) - bad = _("source directory is empty"); - else { - int j, dst_len; - - if (last - first > 0) { - source = xrealloc(source, - (argc + last - first) - * sizeof(char *)); - destination = xrealloc(destination, - (argc + last - first) - * sizeof(char *)); - modes = xrealloc(modes, - (argc + last - first) - * sizeof(enum update_mode)); + int first = cache_name_pos(src, length); + if (first >= 0) { + if (!S_ISGITLINK(active_cache[first]->ce_mode)) + die (_("Huh? Directory %s is in index and no submodule?"), src); + } else { + const char *src_w_slash = add_slash(src); + int last, len_w_slash = length + 1; + + modes[i] = WORKING_DIRECTORY; + + first = cache_name_pos(src_w_slash, len_w_slash); + if (first >= 0) + die (_("Huh? %.*s is in index?"), + len_w_slash, src_w_slash); + + first = -1 - first; + for (last = first; last < active_nr; last++) { + const char *path = active_cache[last]->name; + if (strncmp(path, src_w_slash, len_w_slash)) + break; } - - dst = add_slash(dst); - dst_len = strlen(dst); - - for (j = 0; j < last - first; j++) { - const char *path = - active_cache[first + j]->name; - source[argc + j] = path; - destination[argc + j] = - prefix_path(dst, dst_len, - path + length + 1); - modes[argc + j] = INDEX; + free((char *)src_w_slash); + + if (last - first < 1) + bad = _("source directory
[PATCH v3 0/5] Teach mv to move submodules
Here is my third iteration of this series. Changes to v2 are: - I resolved the conflict with Duy's pathspec series by replacing the use of common_prefix() with relative_path(). - I separated the functions checking for modified unstaged .gitmodules and staging the changes to that file into another commit, as they are used by both mv and rm. - mv and rm now die with the message "Please, stage your changes to .gitmodules or stash them to proceed" instead of changing and staging a .gitmodules file containing other unstaged modifications. - Man pages for mv and rm are updated to tell the user what they do with the gitlink and the .gitmodules file in case of submodules. - Minor changes according to the last review (typos and a bit more efficient coding). This series applies cleanly on current pu (and I also ran t3600 and t7001 manually to make sure I don't hit the silent breakage my last series showed when I ran the whole test suite). Jens Lehmann (5): Teach mv to move submodules together with their work trees Teach mv to move submodules using a gitfile submodule.c: add .gitmodules staging helper functions Teach mv to update the path entry in .gitmodules for moved submodules rm: delete .gitmodules entry of submodules removed from the work tree Documentation/git-mv.txt | 10 ++- Documentation/git-rm.txt | 8 ++- builtin/mv.c | 126 ++ builtin/rm.c | 19 +- submodule.c| 147 + submodule.h| 5 ++ t/t3600-rm.sh | 98 -- t/t7001-mv.sh | 128 +++ t/t7400-submodule-basic.sh | 14 ++--- t/t7610-mergetool.sh | 6 +- 10 files changed, 484 insertions(+), 77 deletions(-) -- 1.8.4.rc0.199.g7079aac -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/3] revert most of the http_options() change
On Jul 30, 2013, at 12:14, Kyle J. McKay wrote: On Jul 29, 2013, at 19:13, Junio C Hamano wrote: With the previous preparation step, the earlier 1bb6 (config: add support for http..* settings, 2013-07-21) that introduced many repeated changes: -if (!strcmp("http.key", var)) { +if (!strcmp("key", key)) { + if (match_is_shorter(..., OPT_KEY_NAME)) + return 0; ... original processing for KEY_NAME ... } can be reverted. This allows us to also get rid of the "repeating yourself to the death" enum http_option_type, and new code (like db/http-savecookies patch) that wants to add a new configuration to the http.* subsystem does not have to conflict with http..variable series at all. This is more-or-less how I want the endgame to look like. Not even compile tested, but it is getting late so I'll show it to the list in case people may want to play with it and polish it while I am away for the night ;-). If people can agree that this is going in the right direction, perhaps we should rebuild Kyle's series without detouring of adding and then yanking what 1bb6 (config: add support for http..* settings, 2013-07-21) did in the next cycle. http.c | 181 +-- test-url-normalize.c | 9 ++- 2 files changed, 39 insertions(+), 151 deletions(-) diff --git a/http.c b/http.c index a91a00b..c7f513b 100644 --- a/http.c +++ b/http.c [...] @@ -469,15 +342,23 @@ void http_init(struct remote *remote, const char *url, int proactive_auth) { char *low_speed_limit; char *low_speed_time; - struct url_info info; + char *normalized_url; + struct urlmatch_config config = { STRING_LIST_INIT_DUP }; + + config.fn = http_options; + config.cascade_fn = git_default_config; + config.item_alloc = NULL; + config.item_clear = NULL; + config.cb = NULL; Missing this: + config.section = "http"; [...] diff --git a/test-url-normalize.c b/test-url-normalize.c index 0b809eb..fab94e5 100644 --- a/test-url-normalize.c +++ b/test-url-normalize.c @@ -15,8 +15,15 @@ static int run_http_options(const char *file, { struct strbuf opt_lc; size_t i, len; + struct urlmatch_config config = { STRING_LIST_INIT_DUP }; Also needs this: + memcpy(&config.url, info, sizeof(struct url_info)); - if (git_config_with_options(http_options, (void *)info, file, 0)) + config.fn = http_options; + config.cascade_fn = git_default_config; + config.item_alloc = NULL; + config.item_clear = NULL; + config.cb = NULL; Missing this: + config.section = "http"; Without these it segfaults running the tests. However, now the tests that actually check the results using config files are failing: # failed 1 among 11 test(s) I'm looking at it now to see if I can fix the problem. And now all the t5200-url-normalize tests pass again. FYI, I couldn't get the patches to apply against next or pu without some minor tweaks that were just conflict resolutions having to do with git_config_with_options changing its signature. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times
Stefan Beller writes: > As of b04ba2bb (parse-options: deprecate OPT_BOOLEAN, 2011-09-27), > the OPT_BOOLEAN was deprecated. > While I am going to replace the OPT_BOOLEAN by the proposed OPT_BOOL or > the OPT_COUNTUP to keep existing behavior, this commit is actually a > bug fix! > > In line 499 we have: > if (list + delete + verify > 1) > usage_with_options(git_tag_usage, options); > Now if we give one of the options twice, we'll get the usage information. > (i.e. 'git tag --verify --verify ' and > 'git --delete --delete ' yield usage information and do not > do the intended command.) > > This could have been fixed by rewriting the line to > if (!!list + !!delete + !!verify > 1) > usage_with_options(git_tag_usage, options); > or as it happened in this patch by having the parameters not > counting up for each occurrence, but the OPT_BOOL just setting the > variables to either 0 if the option is not given or 1 if the option is > given multiple times. Makes twisted sort of sense ;-). > However we could discuss if the negated options do make sense > here, or if we don't want to allow them here, as this seems valid > (before and after this patch): > > git tag --no-verify --delete It probably does not. As you hinted in your earlier patch, we may want to introduce a "only can set to true" boolean used solely to specify these things. They are disguised as "options", but are in fact command operation modes that are often mutually exclusive. For these operation modes that are mutually exclusive, there are multiple possible implementations: * One OPT_BOOL_NONEG per option; the code ensures the mutual exclusion with "if (list + delete + verify > 1)"; * One OPT_BIT per option in a single variable; the code ensures the mutual exclusion with count_bits, which may be a lot more cumbersome; * OPT_SET_INT that updates a single variable to enum; instead of making it an error to give two conflicting modes, this would give us the last-one-wins rule. Unlike usual "options", we generally do not want the last-one-wins semantics for command operation modes, I think. Perhaps we would want something like this? -- >8 -- Subject: [PATCH] parse-options: add OPT_CMDMODE() This can be used to define a set of mutually exclusive "command mode" options, and automatically catch use of more than one from that set as an error. Signed-off-by: Junio C Hamano --- parse-options.c | 58 - parse-options.h | 3 +++ 2 files changed, 56 insertions(+), 5 deletions(-) diff --git a/parse-options.c b/parse-options.c index c2cbca2..62e9b1c 100644 --- a/parse-options.c +++ b/parse-options.c @@ -43,8 +43,42 @@ static void fix_filename(const char *prefix, const char **file) *file = xstrdup(prefix_filename(prefix, strlen(prefix), *file)); } +static int opt_command_mode_error(const struct option *opt, + const struct option *all_opts, + int flags) +{ + const struct option *that; + struct strbuf message = STRBUF_INIT; + struct strbuf that_name = STRBUF_INIT; + + /* +* Find the other option that was used to set the variable +* already, and report that this is not compatible with it. +*/ + for (that = all_opts; that->type != OPTION_END; that++) { + if (that == opt || + that->type != OPTION_CMDMODE || + that->value != opt->value || + that->defval != *(int *)opt->value) + continue; + + if (that->long_name) + strbuf_addf(&that_name, "--%s", that->long_name); + else + strbuf_addf(&that_name, "-%c", that->short_name); + strbuf_addf(&message, ": incompatible with %s", that_name.buf); + strbuf_release(&that_name); + opterror(opt, message.buf, flags); + strbuf_release(&message); + return -1; + } + return opterror(opt, ": incompatible with something else", flags); +} + static int get_value(struct parse_opt_ctx_t *p, -const struct option *opt, int flags) +const struct option *opt, +const struct option *all_opts, +int flags) { const char *s, *arg; const int unset = flags & OPT_UNSET; @@ -83,6 +117,16 @@ static int get_value(struct parse_opt_ctx_t *p, *(int *)opt->value = unset ? 0 : opt->defval; return 0; + case OPTION_CMDMODE: + /* +* Giving the same mode option twice, although is unnecessary, +* is not a grave error, so let it pass. +*/ + if (*(int *)opt->value && *(int *)opt->value != opt->defval) + return opt_command_mode_error(opt, al
Re: [PATCH v2] imap-send: use Apple's Security framework for base64 encoding
On Tue, Jul 30, 2013 at 8:54 AM, Junio C Hamano wrote: > David Aguilar writes: > >> From: Jeremy Huddleston >> >> Use Apple's supported functions for base64 encoding instead >> of the deprecated OpenSSL functions. >> >> Signed-off-by: Jeremy Huddleston >> Signed-off-by: David Aguilar >> --- >> This version moves the tricky #ifdefs into git-compat-util.h > > Nice. I however wonder if we can kick the inlines that are > irrelevant to most people out further. For example, would the > following be an improvement? Yes, IMO that is nicer. It keeps all of the Apple specifics neatly tucked away. Thanks > > -- >8 -- > From: Jeremy Huddleston > Date: Mon, 29 Jul 2013 18:28:30 -0700 > Subject: [PATCH] imap-send: use Apple's Security framework for base64 encoding > > Use Apple's supported functions for base64 encoding instead > of the deprecated OpenSSL functions. > > Signed-off-by: Jeremy Huddleston > Signed-off-by: David Aguilar > Signed-off-by: Junio C Hamano > --- > Makefile | 1 + > compat/apple-common-crypto.h | 86 > > git-compat-util.h| 11 ++ > imap-send.c | 14 > 4 files changed, 98 insertions(+), 14 deletions(-) > create mode 100644 compat/apple-common-crypto.h > > diff --git a/Makefile b/Makefile > index 5e7cadf..dddf49b 100644 > --- a/Makefile > +++ b/Makefile > @@ -1398,6 +1398,7 @@ ifdef PPC_SHA1 > LIB_H += ppc/sha1.h > else > ifdef APPLE_COMMON_CRYPTO > + LIB_4_CRYPTO += -framework Security -framework CoreFoundation > COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL > SHA1_HEADER = > else > diff --git a/compat/apple-common-crypto.h b/compat/apple-common-crypto.h > new file mode 100644 > index 000..c8b9b0e > --- /dev/null > +++ b/compat/apple-common-crypto.h > @@ -0,0 +1,86 @@ > +/* suppress inclusion of conflicting openssl functions */ > +#define OPENSSL_NO_MD5 > +#define HEADER_HMAC_H > +#define HEADER_SHA_H > +#include > +#define HMAC_CTX CCHmacContext > +#define HMAC_Init(hmac, key, len, algo) CCHmacInit(hmac, algo, key, len) > +#define HMAC_Update CCHmacUpdate > +#define HMAC_Final(hmac, hash, ptr) CCHmacFinal(hmac, hash) > +#define HMAC_CTX_cleanup(ignore) > +#define EVP_md5(...) kCCHmacAlgMD5 > +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1070 > +#define APPLE_LION_OR_NEWER > +#include > +/* Apple's TYPE_BOOL conflicts with config.c */ > +#undef TYPE_BOOL > +#endif > + > +#ifdef APPLE_LION_OR_NEWER > +#define git_CC_error_check(pattern, err) \ > + do { \ > + if (err) { \ > + die(pattern, (long)CFErrorGetCode(err)); \ > + } \ > + } while(0) > + > +#define EVP_EncodeBlock git_CC_EVP_EncodeBlock > +static inline int git_CC_EVP_EncodeBlock(unsigned char *out, > + const unsigned char *in, int inlen) > +{ > + CFErrorRef err; > + SecTransformRef encoder; > + CFDataRef input, output; > + CFIndex length; > + > + encoder = SecEncodeTransformCreate(kSecBase64Encoding, &err); > + git_CC_error_check("SecEncodeTransformCreate failed: %ld", err); > + > + input = CFDataCreate(kCFAllocatorDefault, in, inlen); > + SecTransformSetAttribute(encoder, kSecTransformInputAttributeName, > + input, &err); > + git_CC_error_check("SecTransformSetAttribute failed: %ld", err); > + > + output = SecTransformExecute(encoder, &err); > + git_CC_error_check("SecTransformExecute failed: %ld", err); > + > + length = CFDataGetLength(output); > + CFDataGetBytes(output, CFRangeMake(0, length), out); > + > + CFRelease(output); > + CFRelease(input); > + CFRelease(encoder); > + > + return (int)strlen((const char *)out); > +} > + > +#define EVP_DecodeBlock git_CC_EVP_DecodeBlock > +static int inline git_CC_EVP_DecodeBlock(unsigned char *out, > + const unsigned char *in, int inlen) > +{ > + CFErrorRef err; > + SecTransformRef decoder; > + CFDataRef input, output; > + CFIndex length; > + > + decoder = SecDecodeTransformCreate(kSecBase64Encoding, &err); > + git_CC_error_check("SecEncodeTransformCreate failed: %ld", err); > + > + input = CFDataCreate(kCFAllocatorDefault, in, inlen); > + SecTransformSetAttribute(decoder, kSecTransformInputAttributeName, > + input, &err); > + git_CC_error_check("SecTransformSetAttribute failed: %ld", err); > + > + output = SecTransformExecute(decoder, &err); > + git_CC_error_check("SecTransformExecute failed: %ld", err); > + > + length = CFDataGetLength(output); > + CFDataGetBytes(output, CFRangeMake(0, length), out); > + > + CFRelease(output); > + CFRelease(input); > + CFRelease(decoder); > + > + return (int)strlen((const char *)out); > +} > +#endif /* APPLE_LION_OR_NEWER */ > diff --git a/git-compat-util.h b/git-compat-util.h >
Re: [PATCH 3/3] config: --get-urlmatch
On Jul 29, 2013, at 15:49, Junio C Hamano wrote: "git config --get-urlmatch $section[.$variable] $url" is a way to learn what the configured value for $section.$variable is for the given URL, using the logic introduced by the http..config topic. In addition to $section.$variable, entries in the configuration file(s) that match $section..$variable are looked up and the one with that matches the given $url the best is used to answer the query. This can still be further refactored to remove code from http_options() in http.c, I think. Signed-off-by: Junio C Hamano --- builtin/config.c | 141 ++ + 1 file changed, 141 insertions(+) diff --git a/builtin/config.c b/builtin/config.c index 12c5073..c1d32ae 100644 --- a/builtin/config.c +++ b/builtin/config.c [...] +static int get_urlmatch(const char *var, const char *url) +{ + const char *section_tail; + struct string_list_item *item; + struct urlmatch_collect collect = { STRING_LIST_INIT_DUP }; + + if (!url_normalize(url, &collect.url)) + die(collect.url.err); The value now stored in collect.url.url is never freed. + + section_tail = strchr(var, '.'); + if (section_tail) { + collect.section = xmemdupz(var, section_tail - var); + collect.key = strrchr(var, '.') + 1; + show_keys = 0; + } else { + collect.section = var; + collect.key = NULL; + show_keys = 1; + } + + git_config_with_options(urlmatch_collect, &collect, + given_config_file, respect_includes); + + for_each_string_list_item(item, &collect.vars) { + struct urlmatch_item *matched = item->util; + struct strbuf key = STRBUF_INIT; + struct strbuf buf = STRBUF_INIT; + + strbuf_addstr(&key, collect.section); + strbuf_addch(&key, '.'); + strbuf_addstr(&key, item->string); + format_config(&buf, key.buf, + matched->value_is_null ? NULL : matched->value.buf); + fwrite(buf.buf, 1, buf.len, stdout); + strbuf_release(&key); + strbuf_release(&buf); + + strbuf_release(&matched->value); + } + string_list_clear(&collect.vars, 1); Needs something like this here: + free(collect.url.url); + + /* +* section name may have been copied to replace the dot, in which +* case it needs to be freed. key name is either NULL (e.g. 'http' +* alone) or points into var (e.g. 'http.savecookies'), and we do +* not own the storage. +*/ + if (collect.section != var) + free((void *)collect.section); + return 0; +} Still needed after 4/3 except it's now config.url.url instead. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/3] revert most of the http_options() change
On Jul 29, 2013, at 19:13, Junio C Hamano wrote: With the previous preparation step, the earlier 1bb6 (config: add support for http..* settings, 2013-07-21) that introduced many repeated changes: -if (!strcmp("http.key", var)) { +if (!strcmp("key", key)) { + if (match_is_shorter(..., OPT_KEY_NAME)) + return 0; ... original processing for KEY_NAME ... } can be reverted. This allows us to also get rid of the "repeating yourself to the death" enum http_option_type, and new code (like db/http-savecookies patch) that wants to add a new configuration to the http.* subsystem does not have to conflict with http..variable series at all. This is more-or-less how I want the endgame to look like. Not even compile tested, but it is getting late so I'll show it to the list in case people may want to play with it and polish it while I am away for the night ;-). If people can agree that this is going in the right direction, perhaps we should rebuild Kyle's series without detouring of adding and then yanking what 1bb6 (config: add support for http..* settings, 2013-07-21) did in the next cycle. http.c | 181 +-- test-url-normalize.c | 9 ++- 2 files changed, 39 insertions(+), 151 deletions(-) diff --git a/http.c b/http.c index a91a00b..c7f513b 100644 --- a/http.c +++ b/http.c [...] @@ -469,15 +342,23 @@ void http_init(struct remote *remote, const char *url, int proactive_auth) { char *low_speed_limit; char *low_speed_time; - struct url_info info; + char *normalized_url; + struct urlmatch_config config = { STRING_LIST_INIT_DUP }; + + config.fn = http_options; + config.cascade_fn = git_default_config; + config.item_alloc = NULL; + config.item_clear = NULL; + config.cb = NULL; Missing this: + config.section = "http"; [...] diff --git a/test-url-normalize.c b/test-url-normalize.c index 0b809eb..fab94e5 100644 --- a/test-url-normalize.c +++ b/test-url-normalize.c @@ -15,8 +15,15 @@ static int run_http_options(const char *file, { struct strbuf opt_lc; size_t i, len; + struct urlmatch_config config = { STRING_LIST_INIT_DUP }; - if (git_config_with_options(http_options, (void *)info, file, 0)) + config.fn = http_options; + config.cascade_fn = git_default_config; + config.item_alloc = NULL; + config.item_clear = NULL; + config.cb = NULL; Missing this: + config.section = "http"; Without these it segfaults running the tests. However, now the tests that actually check the results using config files are failing: # failed 1 among 11 test(s) I'm looking at it now to see if I can fix the problem. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times
As of b04ba2bb (parse-options: deprecate OPT_BOOLEAN, 2011-09-27), the OPT_BOOLEAN was deprecated. While I am going to replace the OPT_BOOLEAN by the proposed OPT_BOOL or the OPT_COUNTUP to keep existing behavior, this commit is actually a bug fix! In line 499 we have: if (list + delete + verify > 1) usage_with_options(git_tag_usage, options); Now if we give one of the options twice, we'll get the usage information. (i.e. 'git tag --verify --verify ' and 'git --delete --delete ' yield usage information and do not do the intended command.) This could have been fixed by rewriting the line to if (!!list + !!delete + !!verify > 1) usage_with_options(git_tag_usage, options); or as it happened in this patch by having the parameters not counting up for each occurrence, but the OPT_BOOL just setting the variables to either 0 if the option is not given or 1 if the option is given multiple times. However we could discuss if the negated options do make sense here, or if we don't want to allow them here, as this seems valid (before and after this patch): git tag --no-verify --delete Signed-off-by: Stefan Beller --- builtin/tag.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin/tag.c b/builtin/tag.c index b3942e4..d155c9d 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -442,12 +442,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix) struct msg_arg msg = { 0, STRBUF_INIT }; struct commit_list *with_commit = NULL; struct option options[] = { - OPT_BOOLEAN('l', "list", &list, N_("list tag names")), + OPT_BOOL('l', "list", &list, N_("list tag names")), { OPTION_INTEGER, 'n', NULL, &lines, N_("n"), N_("print lines of each tag message"), PARSE_OPT_OPTARG, NULL, 1 }, - OPT_BOOLEAN('d', "delete", &delete, N_("delete tags")), - OPT_BOOLEAN('v', "verify", &verify, N_("verify tags")), + OPT_BOOL('d', "delete", &delete, N_("delete tags")), + OPT_BOOL('v', "verify", &verify, N_("verify tags")), OPT_GROUP(N_("Tag creation options")), OPT_BOOL('a', "annotate", &annotate, @@ -455,7 +455,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix) OPT_CALLBACK('m', "message", &msg, N_("message"), N_("tag message"), parse_msg_arg), OPT_FILENAME('F', "file", &msgfile, N_("read message from file")), - OPT_BOOLEAN('s', "sign", &opt.sign, N_("annotated and GPG-signed tag")), + OPT_BOOL('s', "sign", &opt.sign, N_("annotated and GPG-signed tag")), OPT_STRING(0, "cleanup", &cleanup_arg, N_("mode"), N_("how to strip spaces and #comments from message")), OPT_STRING('u', "local-user", &keyid, N_("key id"), -- 1.8.4.rc0.1.g8f6a3e5 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ANNOUNCE: git-integration -- Easily manage integration branches
On Tue, Jul 30, 2013 at 09:45:49AM -0700, Junio C Hamano wrote: > John Keeping writes: > > > I wrote this script a few months ago and have been using it pretty much > > daily since then, so I figure it's time to see if anyone else finds it > > useful... > > > > git-integration [1] is a script to help manage integration branches in > > Git. By defining a base point and a set of branches to be merged to > > form the integration branch, git-integration lets you easily rebuild an > > integration branch when anything in it changes, as well as showing you > > the status of all of the branches in the integration branch. > > > > For example, the instruction sheet for git-integration's "pu" branch > > recently looked like this: > > > > base master > > > > merge make-clean > > > > Add a "clean" target to the makefile. > > > > merge version > > > > Support for "--version" option. > > > > N.B. this builds on "make-clean". > > > > merge skip-option > > > > Needs more work to be able to handle "branch not found". > > > > This tells git-integration to base the "pu" branch on "master" and merge > > the "make-clean", "version" and "skip-option" branches in. The comments > > following the "merge" instructions are added to the commit message for > > the corresponding merge commit. When I want to rebuild the "pu" branch > > I simply do: > > > > $ git integration --rebuild pu > > > > To change the contents of the branch, I either edit the instruction > > sheet manually: > > > > $ git integration --edit pu > > > > or quickly add a new branch from the command line: > > > > $ git integration --add my-new-branch pu > > > > In fact, I can combine these to get the benefit of bash-completion on > > the branch name and the ability to edit the instruction sheet - when > > multiple commands are specified, git-integration performs each of them > > in a sensible order, described in the manpage [2]. > > > > > > [1] http://johnkeeping.github.io/git-integration/ > > [2] http://johnkeeping.github.io/git-integration/git-integration.html > > Interesting. > > Would it help me to replay "evil merges" I previously made and avoid > necessity to write merge log messages repeatedly? Currently it does nothing beyond having the ability to continue automatically if rerere manages to resolve all conflicts (disabled by default). There is no equivalent of your refs/merge-fix/ feature, although I think I might add one soon ;-). Since the commit messages for the merge commits come from the instruction sheet, it does avoid the need to write them repeatedly - if you want to change the merge message you can simply update the instruction sheet and rebuild. > In short, can I replace my Meta/Reintegrate and Meta/cook with this > (see Documentation/howto/maintain-git.txt)? It performs the same basic function as those scripts, but it's quite a lot simpler and hasn't been designed for the git.git workflow, so I don't think it's suitable for replacing your existing scripts. If I were starting from scratch and attempting to implement the git.git workflow on top of git-integration, I think I would make whats-cooking.txt a build artifact generated from the instruction sheet for pu. This would require some new commands to be added to git-integration's instruction sheet to let it assign sections to branches, but ought to be possible. I expect there would be some subtleties though - certainly git-integration's "--status" output does not handle all of the cases the Meta/cook does, not least because it only compares against a single base branch. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ANNOUNCE: git-integration -- Easily manage integration branches
John Keeping writes: > I wrote this script a few months ago and have been using it pretty much > daily since then, so I figure it's time to see if anyone else finds it > useful... > > git-integration [1] is a script to help manage integration branches in > Git. By defining a base point and a set of branches to be merged to > form the integration branch, git-integration lets you easily rebuild an > integration branch when anything in it changes, as well as showing you > the status of all of the branches in the integration branch. > > For example, the instruction sheet for git-integration's "pu" branch > recently looked like this: > > base master > > merge make-clean > > Add a "clean" target to the makefile. > > merge version > > Support for "--version" option. > > N.B. this builds on "make-clean". > > merge skip-option > > Needs more work to be able to handle "branch not found". > > This tells git-integration to base the "pu" branch on "master" and merge > the "make-clean", "version" and "skip-option" branches in. The comments > following the "merge" instructions are added to the commit message for > the corresponding merge commit. When I want to rebuild the "pu" branch > I simply do: > > $ git integration --rebuild pu > > To change the contents of the branch, I either edit the instruction > sheet manually: > > $ git integration --edit pu > > or quickly add a new branch from the command line: > > $ git integration --add my-new-branch pu > > In fact, I can combine these to get the benefit of bash-completion on > the branch name and the ability to edit the instruction sheet - when > multiple commands are specified, git-integration performs each of them > in a sensible order, described in the manpage [2]. > > > [1] http://johnkeeping.github.io/git-integration/ > [2] http://johnkeeping.github.io/git-integration/git-integration.html Interesting. Would it help me to replay "evil merges" I previously made and avoid necessity to write merge log messages repeatedly? In short, can I replace my Meta/Reintegrate and Meta/cook with this (see Documentation/howto/maintain-git.txt)? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Specify UK English for the documentation source files.
Marc Branchaud writes: > This will hopefully avoid questions over which spelling and grammar should > be used. Translators are of course free to create localizations for other > English dialects. > > Signed-off-by: Marc Branchaud > --- > > Although I'm Canadian I figured en_CA would be a little too parochial. I > don't have a strong preference for en_UK over en_US though. If we were to specify one, I would have to say we should standardise on en_US. Like it or not, that is the lingua franca in the CS world (I'd also admit that most of the extra 'u' in words like "behaviour" are my typoes, not even coming from a conscious decision to choose en_UK). I'd appreciate "typofix" patches to address genuine typoes like "s/filesytem/filesystem/" done separately, so that we do not even have to get into language bikeshedding. Even if we did decide to stick to en_US, the typofix patches and en_US patches should be separate to ease reviews. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] imap-send: use Apple's Security framework for base64 encoding
David Aguilar writes: > From: Jeremy Huddleston > > Use Apple's supported functions for base64 encoding instead > of the deprecated OpenSSL functions. > > Signed-off-by: Jeremy Huddleston > Signed-off-by: David Aguilar > --- > This version moves the tricky #ifdefs into git-compat-util.h Nice. I however wonder if we can kick the inlines that are irrelevant to most people out further. For example, would the following be an improvement? -- >8 -- From: Jeremy Huddleston Date: Mon, 29 Jul 2013 18:28:30 -0700 Subject: [PATCH] imap-send: use Apple's Security framework for base64 encoding Use Apple's supported functions for base64 encoding instead of the deprecated OpenSSL functions. Signed-off-by: Jeremy Huddleston Signed-off-by: David Aguilar Signed-off-by: Junio C Hamano --- Makefile | 1 + compat/apple-common-crypto.h | 86 git-compat-util.h| 11 ++ imap-send.c | 14 4 files changed, 98 insertions(+), 14 deletions(-) create mode 100644 compat/apple-common-crypto.h diff --git a/Makefile b/Makefile index 5e7cadf..dddf49b 100644 --- a/Makefile +++ b/Makefile @@ -1398,6 +1398,7 @@ ifdef PPC_SHA1 LIB_H += ppc/sha1.h else ifdef APPLE_COMMON_CRYPTO + LIB_4_CRYPTO += -framework Security -framework CoreFoundation COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL SHA1_HEADER = else diff --git a/compat/apple-common-crypto.h b/compat/apple-common-crypto.h new file mode 100644 index 000..c8b9b0e --- /dev/null +++ b/compat/apple-common-crypto.h @@ -0,0 +1,86 @@ +/* suppress inclusion of conflicting openssl functions */ +#define OPENSSL_NO_MD5 +#define HEADER_HMAC_H +#define HEADER_SHA_H +#include +#define HMAC_CTX CCHmacContext +#define HMAC_Init(hmac, key, len, algo) CCHmacInit(hmac, algo, key, len) +#define HMAC_Update CCHmacUpdate +#define HMAC_Final(hmac, hash, ptr) CCHmacFinal(hmac, hash) +#define HMAC_CTX_cleanup(ignore) +#define EVP_md5(...) kCCHmacAlgMD5 +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1070 +#define APPLE_LION_OR_NEWER +#include +/* Apple's TYPE_BOOL conflicts with config.c */ +#undef TYPE_BOOL +#endif + +#ifdef APPLE_LION_OR_NEWER +#define git_CC_error_check(pattern, err) \ + do { \ + if (err) { \ + die(pattern, (long)CFErrorGetCode(err)); \ + } \ + } while(0) + +#define EVP_EncodeBlock git_CC_EVP_EncodeBlock +static inline int git_CC_EVP_EncodeBlock(unsigned char *out, + const unsigned char *in, int inlen) +{ + CFErrorRef err; + SecTransformRef encoder; + CFDataRef input, output; + CFIndex length; + + encoder = SecEncodeTransformCreate(kSecBase64Encoding, &err); + git_CC_error_check("SecEncodeTransformCreate failed: %ld", err); + + input = CFDataCreate(kCFAllocatorDefault, in, inlen); + SecTransformSetAttribute(encoder, kSecTransformInputAttributeName, + input, &err); + git_CC_error_check("SecTransformSetAttribute failed: %ld", err); + + output = SecTransformExecute(encoder, &err); + git_CC_error_check("SecTransformExecute failed: %ld", err); + + length = CFDataGetLength(output); + CFDataGetBytes(output, CFRangeMake(0, length), out); + + CFRelease(output); + CFRelease(input); + CFRelease(encoder); + + return (int)strlen((const char *)out); +} + +#define EVP_DecodeBlock git_CC_EVP_DecodeBlock +static int inline git_CC_EVP_DecodeBlock(unsigned char *out, + const unsigned char *in, int inlen) +{ + CFErrorRef err; + SecTransformRef decoder; + CFDataRef input, output; + CFIndex length; + + decoder = SecDecodeTransformCreate(kSecBase64Encoding, &err); + git_CC_error_check("SecEncodeTransformCreate failed: %ld", err); + + input = CFDataCreate(kCFAllocatorDefault, in, inlen); + SecTransformSetAttribute(decoder, kSecTransformInputAttributeName, + input, &err); + git_CC_error_check("SecTransformSetAttribute failed: %ld", err); + + output = SecTransformExecute(decoder, &err); + git_CC_error_check("SecTransformExecute failed: %ld", err); + + length = CFDataGetLength(output); + CFDataGetBytes(output, CFRangeMake(0, length), out); + + CFRelease(output); + CFRelease(input); + CFRelease(decoder); + + return (int)strlen((const char *)out); +} +#endif /* APPLE_LION_OR_NEWER */ diff --git a/git-compat-util.h b/git-compat-util.h index e955bb5..6ebb029 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -127,6 +127,17 @@ #else #include #endif + +#ifndef NO_OPENSSL +#ifdef APPLE_COMMON_CRYPTO +#include "compat/apple-common-crypto.h" +#else +#include +#include +#endif /* APPLE_COMMON_CRYPTO */ +#include +#endif /* NO_OPENSSL */ + #if defined(__MINGW32__) /* pull in Windows compatibility stuff */ #include "compat/mingw.h" di
Re: [PATCH] Specify UK English for the documentation source files.
On Tue, Jul 30, 2013 at 11:11:54AM -0400, Marc Branchaud wrote: > This will hopefully avoid questions over which spelling and grammar should > be used. Translators are of course free to create localizations for other > English dialects. > > Signed-off-by: Marc Branchaud > --- > > Although I'm Canadian I figured en_CA would be a little too parochial. I > don't have a strong preference for en_UK over en_US though. > Since the settings (for example color) use en_US, I suggest _not_ to specify en_UK as documentation language. Either set en_US or rather don't set anything at all but go after "google correct". That's the most populair alternative. For example colour gets 422 000 000 hits and color gets 2 260 000 000 hits. We should use color even if it's not en_UK. It's a more pragmatic approach and most git users aren't native english speakers anyway. -- Med vänliga hälsningar Fredrik Gustafsson tel: 0733-608274 e-post: iv...@iveqy.com -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
ANNOUNCE: git-integration -- Easily manage integration branches
I wrote this script a few months ago and have been using it pretty much daily since then, so I figure it's time to see if anyone else finds it useful... git-integration [1] is a script to help manage integration branches in Git. By defining a base point and a set of branches to be merged to form the integration branch, git-integration lets you easily rebuild an integration branch when anything in it changes, as well as showing you the status of all of the branches in the integration branch. For example, the instruction sheet for git-integration's "pu" branch recently looked like this: base master merge make-clean Add a "clean" target to the makefile. merge version Support for "--version" option. N.B. this builds on "make-clean". merge skip-option Needs more work to be able to handle "branch not found". This tells git-integration to base the "pu" branch on "master" and merge the "make-clean", "version" and "skip-option" branches in. The comments following the "merge" instructions are added to the commit message for the corresponding merge commit. When I want to rebuild the "pu" branch I simply do: $ git integration --rebuild pu To change the contents of the branch, I either edit the instruction sheet manually: $ git integration --edit pu or quickly add a new branch from the command line: $ git integration --add my-new-branch pu In fact, I can combine these to get the benefit of bash-completion on the branch name and the ability to edit the instruction sheet - when multiple commands are specified, git-integration performs each of them in a sensible order, described in the manpage [2]. [1] http://johnkeeping.github.io/git-integration/ [2] http://johnkeeping.github.io/git-integration/git-integration.html -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sha1_file: introduce close_one_pack() to close packs on fd pressure
Brandon Casey writes: > From: Brandon Casey > > When the number of open packs exceeds pack_max_fds, unuse_one_window() > is called repeatedly to attempt to release the least-recently-used > pack windows, which, as a side-effect, will also close a pack file > after closing its last open window. If a pack file has been opened, > but no windows have been allocated into it, it will never be selected > by unuse_one_window() and hence its file descriptor will not be > closed. When this happens, git may exceed the number of file > descriptors permitted by the system. An interesting find. The patch from a cursory look reads OK. Thanks. > This is not likely to occur during upload-pack since upload-pack > reads each object from the pack so that it can peel tags and > advertise the exposed object. Another interesting find. Perhaps there is a room for improvements, as packed-refs file knows what objects the tags peel to? I vaguely recall Peff was actively reducing the object access during ref enumeration in not so distant past... -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Remove sq_quote_print() in favor of *_buf
Junio C Hamano writes: > Ramkumar Ramachandra writes: > >> While going through the for-each-ref-pretty series that Duy and I were >> developing, I noticed that this cleanup was independent and good >> as-it-is. >> >> So here it is. > > You always can first allocate a piece of memory and write into it > instead of writing things out directly. The patch shows it _can_ be > done, but that is not a news. > > And such a change is hardly a "clean-up". It just wastes more > memory you do not have to waste, in order to do what you are doing. > > When there is a reason why you need an in-memory representation, > this change starts to become the first step refactoring for an > enhancement. Having said all that, the patch texts all look OK, so I'll queue them with updated log messages. It was the usual me reacting to unjustified value judgement made in log messages and cover letters. It would save me a lot of work if people stopped doing that and instead stuck to facts. For example, between print_to_buf() and print_to_stdout(), the former is *not* necessarily "nicer". It is more flexible but that flexibility comes with a price, and the caller needs a demonstrative need for that flexibility to justify the cost. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Specify UK English for the documentation source files.
This will hopefully avoid questions over which spelling and grammar should be used. Translators are of course free to create localizations for other English dialects. Signed-off-by: Marc Branchaud --- Although I'm Canadian I figured en_CA would be a little too parochial. I don't have a strong preference for en_UK over en_US though. On 13-07-30 11:05 AM, Junio C Hamano wrote:> > I'd rather not to see any change that turns one accepted form into > another accepted form at all (like "parseable" vs "parsable" in this > patch). For that purpose, asking "What is parseable" to Google and > seeing if there is a hit is good enough ;-) I think choosing a dialect for git will help mitigate such changes. But perhaps it's a bit too draconian. M. Documentation/CodingGuidelines | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines index 559d5f9..43af82e 100644 --- a/Documentation/CodingGuidelines +++ b/Documentation/CodingGuidelines @@ -242,6 +242,9 @@ Writing Documentation: processed into HTML and manpages (e.g. git.html and git.1 in the same directory). + The human lanuage of the documentation source files is UK English (en_UK). + Please follow UK English norms for spelling and grammar. + Every user-visible change should be reflected in the documentation. The same general rule as for code applies -- imitate the existing conventions. A few commented examples follow to provide reference -- 1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Documentation: fix typos in man pages
Marc Branchaud writes: > I personally don't have a lot of time to investigate the nuances of English. > However, I desperately hope this list can avoid any linguistic flame wars. > In that spirit, I suggest that anyone posting an orthographic patch (i.e. for > something that isn't an obvious spelling mistake) could helpfully include a > link or two to an explanation of the reasoning for the change. Especially > for folks who aren't native English speakers, this could help avoid a lot of > back-and-forth. > > One general source I've found is the English StackExchange: > http://english.stackexchange.com/ I'd rather not to see any change that turns one accepted form into another accepted form at all (like "parseable" vs "parsable" in this patch). For that purpose, asking "What is parseable" to Google and seeing if there is a hit is good enough ;-) I'd of course welcome genuine typofixes, like s/filesytem/filesystem/. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Remove sq_quote_print() in favor of *_buf
Ramkumar Ramachandra writes: > While going through the for-each-ref-pretty series that Duy and I were > developing, I noticed that this cleanup was independent and good > as-it-is. > > So here it is. You always can first allocate a piece of memory and write into it instead of writing things out directly. The patch shows it _can_ be done, but that is not a news. And such a change is hardly a "clean-up". It just wastes more memory you do not have to waste, in order to do what you are doing. When there is a reason why you need an in-memory representation, this change starts to become the first step refactoring for an enhancement. > Nguyễn Thái Ngọc Duy (1): > for-each-ref, quote: convert *_quote_print -> *_quote_buf The log message for this one explains it very well. This change by itself is not useful, but it will become necessary once you start needing to access an in-memory result. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Documentation: fix typos in man pages
On 13-07-29 05:15 PM, Øystein Walle wrote: > Signed-off-by: Øystein Walle > --- > I thought I'd take part in the typo fixing frenzy :) > > I have some other potential typos lines up. Right now the docs refer to both > 'filesystem' and 'file system', as well as both 'testsuite' and 'test suite'. > I > think words like these are generally split in English but I'm not sure. I generally prefer to see the spaces in these words, otherwise it starts to look more like German. But of course English is full of exceptions... > There are also some words that I think look better with with a dash, e.g. > 'trade-off'. Should I just send these as a patch too instead of jabbering on > about it? I'm indifferent to that. I guess it depends on the context, so seeing the patch would help. I personally don't have a lot of time to investigate the nuances of English. However, I desperately hope this list can avoid any linguistic flame wars. In that spirit, I suggest that anyone posting an orthographic patch (i.e. for something that isn't an obvious spelling mistake) could helpfully include a link or two to an explanation of the reasoning for the change. Especially for folks who aren't native English speakers, this could help avoid a lot of back-and-forth. One general source I've found is the English StackExchange: http://english.stackexchange.com/ > Documentation/git-check-ignore.txt | 2 +- > Documentation/git-clone.txt| 2 +- > Documentation/git-daemon.txt | 2 +- > Documentation/git-diff.txt | 2 +- > Documentation/gitcli.txt | 2 +- > Documentation/githooks.txt | 2 +- > Documentation/gitweb.conf.txt | 4 ++-- > Documentation/user-manual.txt | 2 +- > 8 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/Documentation/git-check-ignore.txt > b/Documentation/git-check-ignore.txt > index d2df487..5354301 100644 > --- a/Documentation/git-check-ignore.txt > +++ b/Documentation/git-check-ignore.txt > @@ -35,7 +35,7 @@ OPTIONS > Read file names from stdin instead of from the command-line. > > -z:: > - The output format is modified to be machine-parseable (see > + The output format is modified to be machine-parsable (see I believe this is a US/UK nuance. As I've recently stated, I think this kind of change isn't all that helpful as we're likely to see some well-intentioned person switch it back sometime in the future. If the git project could choose an official English dialect it would go a long way towards mitigating such churn. > below). If `--stdin` is also given, input paths are separated > with a NUL character instead of a linefeed character. > > diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt > index 450f158..3865658 100644 > --- a/Documentation/git-clone.txt > +++ b/Documentation/git-clone.txt > @@ -213,7 +213,7 @@ objects from the source repository into a pack in the > cloned repository. > --separate-git-dir=:: > Instead of placing the cloned repository where it is supposed > to be, place the cloned repository at the specified directory, > - then make a filesytem-agnostic Git symbolic link to there. > + then make a filesystem-agnostic Git symbolic link to there. > The result is Git repository can be separated from working > tree. > > diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt > index 223f731..a3283e1 100644 > --- a/Documentation/git-daemon.txt > +++ b/Documentation/git-daemon.txt > @@ -189,7 +189,7 @@ Git configuration files in that directory are readable by > ``. > service by exiting with a non-zero status (or to allow it by > exiting with a zero status). It can also look at the $REMOTE_ADDR > and $REMOTE_PORT environment variables to learn about the > - requestor when making this decision. > + requester when making this decision. Although I prefer the -or form for this word, this really is one of English's vague areas. Some words that end with -st definitely take the -er suffix (tester, jester) but others take the -or suffix (investor). A bit of Googling also gave no firm result[1][2]. So I think this change is neither good nor bad. However, like the UK/US-isms, I wonder if there's some way to avoid people changing this back and forth. But I don't think simply choosing a dialect will help here. [1] http://english.stackexchange.com/questions/29254/whats-the-difference-between-requester-and-requestor [2] http://www.spelling.hemscott.net/ends4.html > + > The external command can optionally write a single line to its > standard output to be sent to the requestor as an error message when > diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt > index 78d6d50..fe42bf6 100644 > --- a/Documentation/git-diff.txt > +++ b/Documentation/git-diff.txt > @@ -39,7 +39,7 @@ directories. This behavior can be forced by --no-index. > commit relative to the named . Typically y
Re: [PATCH] contrib/subtree: Fix make install target
Michal Sojka writes: > If the libexec directory doesn't exist, git-subtree gets installed as > $prefix/share/libexec/git-core file. This patch creates the directory > before installing git-subtree file into it. > > Signed-off-by: Michal Sojka > --- Will queue, as it is obviously correct. Thanks. I however wish somebody steps up to start properly maintaining this part of the contrib ;-). > contrib/subtree/Makefile | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/contrib/subtree/Makefile b/contrib/subtree/Makefile > index b507505..435b2de 100644 > --- a/contrib/subtree/Makefile > +++ b/contrib/subtree/Makefile > @@ -30,6 +30,7 @@ $(GIT_SUBTREE): $(GIT_SUBTREE_SH) > doc: $(GIT_SUBTREE_DOC) > > install: $(GIT_SUBTREE) > + $(INSTALL) -d -m 755 $(DESTDIR)$(libexecdir) > $(INSTALL) -m 755 $(GIT_SUBTREE) $(DESTDIR)$(libexecdir) > > install-doc: install-man -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] config: --get-urlmatch
Jeff King writes: > Ah, I missed that you could leave "key" empty. Yes, the general syntax is git config [--] --get-urlmatch [.] and giving without a specific would list all the variables in the section that apply to . This is why we should do documentation at some point before publishing a patch series; sorry about that. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] editor: use canonicalized absolute path
Duy Nguyen writes: > The idea is the same, but my patch is a bit different (use of realpath > instead of real_path, I didn't remember git has real_path). I'm fine > with Ram being the author. Thanks, both of you, for clarification. >> Compared to not being able to edit, it may be a small price to pay >> for those who do need to suffer the broken editor, but the patch >> makes those who do not need this workaround to pay the price. > > Does looking at the edited file's path happen often? I have never done > that before. I ask because in order to avoid the price for those > users, the code could get a little bit more complicated (detecting if > the given relative path traverse backward outside a symlink..). To me > it seems like a good trade off in favor of simpler code. Yeah, I was being my usual cautious self, as somebody has to play that role. I think the code as-is would be an OK trade off. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] editor: use canonicalized absolute path
On Mon, Jul 29, 2013 at 07:56:58AM -0700, Junio C Hamano wrote: > > diff --git a/editor.c b/editor.c > > index 27bdecd..0abbd8d 100644 > > --- a/editor.c > > +++ b/editor.c > > @@ -37,7 +37,7 @@ int launch_editor(const char *path, struct strbuf > > *buffer, const char *const *en > > return error("Terminal is dumb, but EDITOR unset"); > > > > if (strcmp(editor, ":")) { > > - const char *args[] = { editor, path, NULL }; > > + const char *args[] = { editor, real_path(path), NULL }; > > While I am not fundamentally opposed to add a workaround for bugs in > a popular tool many people use, I am a bit uneasy about this change. > > For editors that are not broken, this could be an annoying > regression, isn't it? When the user asks "What is the path of the > file I am editing?" to the editor (i.e. an equivalent of \C-x\C-b), > the updated code will start spewing a long full-path from the root > directory, while we used to give a relative path that is short, > sweet and more in line with the context of user's work. > > Compared to not being able to edit, it may be a small price to pay > for those who do need to suffer the broken editor, but the patch > makes those who do not need this workaround to pay the price. > How about something like this? For standard setups, even if you have symlink in cwd, it won't kick in because the given path is usually .git/something (i.e. no ".." component) -- 8< -- diff --git a/editor.c b/editor.c index 27bdecd..02bf42c 100644 --- a/editor.c +++ b/editor.c @@ -37,10 +37,17 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en return error("Terminal is dumb, but EDITOR unset"); if (strcmp(editor, ":")) { + char cwd[PATH_MAX]; const char *args[] = { editor, path, NULL }; struct child_process p; int ret, sig; + /* emacs workaround */ + if (getcwd(cwd, PATH_MAX) && + strcmp(real_path(cwd), cwd) && + strstr(path, "../")) + args[1] = real_path(path); + memset(&p, 0, sizeof(p)); p.argv = args; p.env = env; -- 8<-- -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Help using git subtree
On Tue, Jul 30, 2013 at 11:32:22AM +0200, Gabriel Jover wrote: > Dear all, > > I am trying to avoid using git submodules and thus I am testing if > git subtree fit my needs. > I have a set of sub-projects linked to a main-project. Just out of curiosity, why are you trying to avoid submodules? > > Git subtree is very convenient to get all the sub-projects into the > main-project directory tree > and to send back sub-project commits to the corresponding repository. > But I don't understand the work flow very well. I haven't used subtree that much however how do you commit to upstream? AFAIK every commit to the subtree must be done in the subtree branch and then merged back to the "superproject"-branch with a subtree merge. > > I can add a subtree with a given prefix and pull/push my commits to > the repository, > but how do I do to pull/push commits from the main-project to the > corresponding repository > avoiding sub-projects to be sent? You don't. A subtree is a subtree in your git-repo and is therefore a part of your repository. When you're using subtree that project will be a part of you project. > > An other question I have is when will be subtree included in git packages. > I had to compile git from sources in order to get this feature. > git-subtree is a part of git, located in the contrib section. When your software distribution choose to distribute that with your git-package is not up to the git project to decide but something you need to ask your package maintainer. If you're asking when git-subtree will move from contrib/ to core git, I don't know. That's a decision for Junio. I would argue that it shouldn't because submodules can do everything that subtrees are good at and git.git should focus on doing one solution well instead of doing the same thing in two bad ways. However since git.git already uses a subtree-like style for gitk, git-gui, etc. it doesn't seem like Junio agrees with me on this point. > > Si heu rebut aquest correu per error, us informo que pot contenir informació > confidencial i privada i que està prohibit el seu ús. Us agrairiem que ho > comuniqueu al remitent i l'elimineu. Gràcies. > Si ha recibido este correo por error, le informo de que puede contener > información confidencial y privada y que está prohibido su uso. Le > agradeceré que lo comunique a su remitente y lo elimine. Gracias. > If you have received this e-mail by error, please note that it may contain > confidential and private information, therefore, the use of this information > is strictly forbidden. Please inform the sender of the error and delete the > information received. Thank you. Is this really something to have in e-mails to a public list? Some lists won't accept this type of e-mails, or simply just ignore you. -- Med vänliga hälsningar Fredrik Gustafsson tel: 0733-608274 e-post: iv...@iveqy.com -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Help using git subtree
Dear all, I am trying to avoid using git submodules and thus I am testing if git subtree fit my needs. I have a set of sub-projects linked to a main-project. Git subtree is very convenient to get all the sub-projects into the main-project directory tree and to send back sub-project commits to the corresponding repository. But I don't understand the work flow very well. I can add a subtree with a given prefix and pull/push my commits to the repository, but how do I do to pull/push commits from the main-project to the corresponding repository avoiding sub-projects to be sent? An other question I have is when will be subtree included in git packages. I had to compile git from sources in order to get this feature. Sincerely, G. Jover-Manas -- ++ Gabriel V. Jover Mañas Computing Division Cells / Alba Synchrotron [http:/www.cells.es] Carretera BP 1413 de Cerdanyola-Sant Cugat, Km. 3.3 E-08290 Cerdanyola del Valles (Barcelona), Spain E-mail: gjo...@cells.es Phone: +34 93 592 4471 Office: ALBA P0 Of8/18 ++ Si heu rebut aquest correu per error, us informo que pot contenir informació confidencial i privada i que està prohibit el seu ús. Us agrairiem que ho comuniqueu al remitent i l'elimineu. Gràcies. Si ha recibido este correo por error, le informo de que puede contener información confidencial y privada y que está prohibido su uso. Le agradeceré que lo comunique a su remitente y lo elimine. Gracias. If you have received this e-mail by error, please note that it may contain confidential and private information, therefore, the use of this information is strictly forbidden. Please inform the sender of the error and delete the information received. Thank you. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] for-each-ref, quote: convert *_quote_print -> *_quote_buf
From: Nguyễn Thái Ngọc Duy for-each-ref.c:print_value() currently prints values to stdout immediately using {sq|perl|python|tcl}_quote_print, giving us no opportunity to do any further processing. In preparation for getting print_value() to accept an additional strbuf argument to write to, convert the *_quote_print functions and callers to *_quote_buf. [rr: commit message, minor modifications] Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: Ramkumar Ramachandra --- builtin/for-each-ref.c | 13 + quote.c| 44 ++-- quote.h| 6 +++--- 3 files changed, 34 insertions(+), 29 deletions(-) diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index 7f059c3..1d4083c 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -867,24 +867,29 @@ static void sort_refs(struct ref_sort *sort, struct refinfo **refs, int num_refs static void print_value(struct refinfo *ref, int atom, int quote_style) { struct atom_value *v; + struct strbuf sb = STRBUF_INIT; get_value(ref, atom, &v); switch (quote_style) { case QUOTE_NONE: fputs(v->s, stdout); break; case QUOTE_SHELL: - sq_quote_print(stdout, v->s); + sq_quote_buf(&sb, v->s); break; case QUOTE_PERL: - perl_quote_print(stdout, v->s); + perl_quote_buf(&sb, v->s); break; case QUOTE_PYTHON: - python_quote_print(stdout, v->s); + python_quote_buf(&sb, v->s); break; case QUOTE_TCL: - tcl_quote_print(stdout, v->s); + tcl_quote_buf(&sb, v->s); break; } + if (quote_style != QUOTE_NONE) { + fputs(sb.buf, stdout); + strbuf_release(&sb); + } } static int hex1(char ch) diff --git a/quote.c b/quote.c index 5c88081..9fd66c6 100644 --- a/quote.c +++ b/quote.c @@ -408,72 +408,72 @@ int unquote_c_style(struct strbuf *sb, const char *quoted, const char **endp) /* quoting as a string literal for other languages */ -void perl_quote_print(FILE *stream, const char *src) +void perl_quote_buf(struct strbuf *sb, const char *src) { const char sq = '\''; const char bq = '\\'; char c; - fputc(sq, stream); + strbuf_addch(sb, sq); while ((c = *src++)) { if (c == sq || c == bq) - fputc(bq, stream); - fputc(c, stream); + strbuf_addch(sb, bq); + strbuf_addch(sb, c); } - fputc(sq, stream); + strbuf_addch(sb, sq); } -void python_quote_print(FILE *stream, const char *src) +void python_quote_buf(struct strbuf *sb, const char *src) { const char sq = '\''; const char bq = '\\'; const char nl = '\n'; char c; - fputc(sq, stream); + strbuf_addch(sb, sq); while ((c = *src++)) { if (c == nl) { - fputc(bq, stream); - fputc('n', stream); + strbuf_addch(sb, bq); + strbuf_addch(sb, 'n'); continue; } if (c == sq || c == bq) - fputc(bq, stream); - fputc(c, stream); + strbuf_addch(sb, bq); + strbuf_addch(sb, c); } - fputc(sq, stream); + strbuf_addch(sb, sq); } -void tcl_quote_print(FILE *stream, const char *src) +void tcl_quote_buf(struct strbuf *sb, const char *src) { char c; - fputc('"', stream); + strbuf_addch(sb, '"'); while ((c = *src++)) { switch (c) { case '[': case ']': case '{': case '}': case '$': case '\\': case '"': - fputc('\\', stream); + strbuf_addch(sb, '\\'); default: - fputc(c, stream); + strbuf_addch(sb, c); break; case '\f': - fputs("\\f", stream); + strbuf_addstr(sb, "\\f"); break; case '\r': - fputs("\\r", stream); + strbuf_addstr(sb, "\\r"); break; case '\n': - fputs("\\n", stream); + strbuf_addstr(sb, "\\n"); break; case '\t': - fputs("\\t", stream); + strbuf_addstr(sb, "\\t"); break; case '\v': - fputs("\\v", stream); + strbuf_addstr(sb, "\\v"); break;
[PATCH 3/3] quote: remove sq_quote_print()
Remove sq_quote_print() since it has no callers. A nicer alternative sq_quote_buf() exists: its callers aren't forced to print immediately. For historical context, sq_quote_print() was first introduced in 575ba9d6 (GIT_TRACE: show which built-in/external commands are executed, 2006-06-25) for the purpose of printing argv for $GIT_TRACE. Today, we achieve this using trace_argv_printf() -> sq_quote_argv() -> sq_quote_buf(), which ultimately fills in a strbuf. Signed-off-by: Ramkumar Ramachandra Signed-off-by: Nguyễn Thái Ngọc Duy --- quote.c | 17 - quote.h | 2 -- 2 files changed, 19 deletions(-) diff --git a/quote.c b/quote.c index 9fd66c6..45e3db1 100644 --- a/quote.c +++ b/quote.c @@ -42,23 +42,6 @@ void sq_quote_buf(struct strbuf *dst, const char *src) free(to_free); } -void sq_quote_print(FILE *stream, const char *src) -{ - char c; - - fputc('\'', stream); - while ((c = *src++)) { - if (need_bs_quote(c)) { - fputs("'\\", stream); - fputc(c, stream); - fputc('\'', stream); - } else { - fputc(c, stream); - } - } - fputc('\'', stream); -} - void sq_quote_argv(struct strbuf *dst, const char** argv, size_t maxlen) { int i; diff --git a/quote.h b/quote.h index 6996ebd..71dcc3a 100644 --- a/quote.h +++ b/quote.h @@ -27,8 +27,6 @@ struct strbuf; * excluding the final null regardless of the buffer size. */ -extern void sq_quote_print(FILE *stream, const char *src); - extern void sq_quote_buf(struct strbuf *, const char *src); extern void sq_quote_argv(struct strbuf *, const char **argv, size_t maxlen); -- 1.8.4.rc0.4.g4634265 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] tar-tree: remove dependency on sq_quote_print()
Currently, there is exactly one caller of sq_quote_print(), namely cmd_tar_tree(). In the interest of removing sq_quote_print() and simplification, replace it with an equivalent call to sq_quote_argv(). No functional changes intended. Signed-off-by: Ramkumar Ramachandra Signed-off-by: Nguyễn Thái Ngọc Duy --- builtin/tar-tree.c | 11 +-- 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/builtin/tar-tree.c b/builtin/tar-tree.c index 3f1e701..ba3ffe6 100644 --- a/builtin/tar-tree.c +++ b/builtin/tar-tree.c @@ -26,8 +26,8 @@ int cmd_tar_tree(int argc, const char **argv, const char *prefix) * $0 tree-ish basedir ==> * git archive --format-tar --prefix=basedir tree-ish */ - int i; const char **nargv = xcalloc(sizeof(*nargv), argc + 3); + struct strbuf sb = STRBUF_INIT; char *basedir_arg; int nargc = 0; @@ -65,11 +65,10 @@ int cmd_tar_tree(int argc, const char **argv, const char *prefix) fprintf(stderr, "*** \"git tar-tree\" is now deprecated.\n" "*** Running \"git archive\" instead.\n***"); - for (i = 0; i < nargc; i++) { - fputc(' ', stderr); - sq_quote_print(stderr, nargv[i]); - } - fputc('\n', stderr); + sq_quote_argv(&sb, nargv, 0); + strbuf_addch(&sb, '\n'); + fputs(sb.buf, stderr); + strbuf_release(&sb); return cmd_archive(nargc, nargv, prefix); } -- 1.8.4.rc0.4.g4634265 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] Remove sq_quote_print() in favor of *_buf
Hi, While going through the for-each-ref-pretty series that Duy and I were developing, I noticed that this cleanup was independent and good as-it-is. So here it is. Nguyễn Thái Ngọc Duy (1): for-each-ref, quote: convert *_quote_print -> *_quote_buf Ramkumar Ramachandra (2): tar-tree: remove dependency on sq_quote_print() quote: remove sq_quote_print() builtin/for-each-ref.c | 13 +++ builtin/tar-tree.c | 11 + quote.c| 61 ++ quote.h| 8 +++ 4 files changed, 39 insertions(+), 54 deletions(-) -- 1.8.4.rc0.4.g4634265 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] contrib/subtree: Fix make install target
If the libexec directory doesn't exist, git-subtree gets installed as $prefix/share/libexec/git-core file. This patch creates the directory before installing git-subtree file into it. Signed-off-by: Michal Sojka --- contrib/subtree/Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/contrib/subtree/Makefile b/contrib/subtree/Makefile index b507505..435b2de 100644 --- a/contrib/subtree/Makefile +++ b/contrib/subtree/Makefile @@ -30,6 +30,7 @@ $(GIT_SUBTREE): $(GIT_SUBTREE_SH) doc: $(GIT_SUBTREE_DOC) install: $(GIT_SUBTREE) + $(INSTALL) -d -m 755 $(DESTDIR)$(libexecdir) $(INSTALL) -m 755 $(GIT_SUBTREE) $(DESTDIR)$(libexecdir) install-doc: install-man -- 1.8.3.2 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] config: --get-urlmatch
On Mon, Jul 29, 2013 at 06:33:43PM -0700, Junio C Hamano wrote: > Jeff King writes: > > >> +struct urlmatch_item { > >> + size_t max_matched_len; > >> + char user_matched; > >> + char value_is_null; > >> + struct strbuf value; > >> +}; > > > > I think you ultimately want such a string_list for matching arbitrary > > numbers of keys, but do you need it for the git-config case? > > "git config" does not know the semantics of each key, nor available > set of keys, no? The string-list is only to support > > git config --get-urlmatch http http://www.google.com/ > > i.e. "list everything under http.* hierarchy". Ah, I missed that you could leave "key" empty. I had expected collect->key to be filled in, at which point you only ever have one such key (and you do not need to know the semantics, only which one is the "winner"). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sha1_file: introduce close_one_pack() to close packs on fd pressure
On Tue, Jul 30, 2013 at 12:05 AM, Brandon Casey wrote: > When the number of open packs exceeds pack_max_fds, unuse_one_window() > is called repeatedly to attempt to release the least-recently-used > pack windows, which, as a side-effect, will also close a pack file > after closing its last open window. If a pack file has been opened, > but no windows have been allocated into it, it will never be selected > by unuse_one_window() and hence its file descriptor will not be > closed. When this happens, git may exceed the number of file > descriptors permitted by the system. > > This latter situation can occur in show-ref or receive-pack during ref > advertisement. During ref advertisement, receive-pack will iterate > over every ref in the repository and advertise it to the client after > ensuring that the ref exists in the local repository. If the ref is > located inside a pack, then the pack is opened to ensure that it > exists, but since the object is not actually read from the pack, no > mmap windows are allocated. When the number of open packs exceeds > pack_max_fds, unuse_one_window() will not able to find any windows to s/not able/not be able/ ...or... s/not able to find/not find/ > free and will not be able to close any packs. Once the per-process > file descriptor limit is exceeded, receive-pack will produce a warning, > not an error, for each pack it cannot open, and will then most likely > fail with an error to spawn rev-list or index-pack like: > >error: cannot create standard input pipe for rev-list: Too many open files >error: Could not run 'git rev-list' > > This is not likely to occur during upload-pack since upload-pack > reads each object from the pack so that it can peel tags and > advertise the exposed object. So during upload-pack, mmap windows > will be allocated for each pack that is opened and unuse_one_window() > will eventually be able to close unused packs after freeing all of > their windows. > > When we have file descriptor pressure, in contrast to memory pressure, > we need to free all windows and close the pack file descriptor so that > a new pack can be opened. Let's introduce a new function > close_one_pack() designed specifically for this purpose to search > for and close the least-recently-used pack, where LRU is defined as > >* pack with oldest mtime and no allocated mmap windows or >* pack with the least-recently-used windows, i.e. the pack > with the oldest most-recently-used window > > Signed-off-by: Brandon Casey -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html