Re: get_maintainer.pl and .mailmap entries with more than 2 addresses
Em Wed, 03 Aug 2016 00:17:10 +0200 Florian Micklerescreveu: > cc'd mche...@s-opensource.com (Mauro, is your kernel.org address up?) Yes, my kernel.org address is up. Better to keep it as the canonical address, as this is unlikely to change as it is not associated to an e-mail provider. > > Am Tue, 02 Aug 2016 09:36:21 -0700 > schrieb Joe Perches : > > > Hello Florian. > > > > There is at least an oddity with get_maintainer handling of a > > .mailmap entry form. > > > > For instance: > > > > Mauro's .mailmap entry is: > > Mauro Carvalho Chehab > > > > > > > > Is this a valid form? > > > > get_maintainer output for Mauro is: > > > > $ ./scripts/get_maintainer.pl drivers/media/ -f > > Mauro Carvalho Chehab > > > > (maintainer:MEDIA INPUT INFRASTRUCTURE > > (V4L/DVB)) > > > > I believe the Mauro's and Shuah's .mailmap entries are improper and > > should be changed, but I'm not completely aware of git .mailmap > > handling and the documentation seems weakly specified. > > > > Hmm.. looking at Mauros last .mailmap commit it seems like your patch is > ok for Mauro. > > Although and are probably > missing? (@Mauro) The mche...@brturbo.com.br is indeed an old email that I used on my first submissions. I don't know a mywin...@gmail.com address... that looks really weird on my eyes... Hmm... $ git log --author "mywin...@gmail.com" commit fe8b671306c78a963934cb5d6e354178390b8c87 Author: Mauro Carvalho Chehab Date: Thu Sep 30 14:46:47 2010 -0300 [media] saa7134: port Asus P7131 Hybrid to use the new rc-core The rc map table were corrected thanks to Giorgio input and tests. Reported-by: Giorgio Vazzana Tested-by: Giorgio Vazzana Signed-off-by: Mauro Carvalho Chehab I don't remember the dirty details about such patch anymore... too many years ago. From the custody chain, I _suspect_ that Giorgio proposed a patch to be applied against the saa7134 driver, but the remote controller map table was moved to a separate file about 7 months before such patch by those changesets: 6686fa6917ca V4L/DVB: Break Remote Controller keymaps into modules 77b7422d48cd V4L/DVB: ir-common: move IR tables from ir-keymaps.c to a separate file I likely rewrote his patch against the new driver, but somehow I mangled with the author name/email. In any case, mywin...@gmail.com should not be associated with me. > > $ git shortlog | grep "^Mauro C" > Mauro Carvalho Chehab (4404): > $ git log | grep "^Author:.*Mauro Carvalho Chehab" | sort | uniq -c > 2 Author: Mauro Carvalho Chehab > 146 Author: Mauro Carvalho Chehab > 645 Author: Mauro Carvalho Chehab > 794 Author: Mauro Carvalho Chehab >2015 Author: Mauro Carvalho Chehab > 448 Author: Mauro Carvalho Chehab > 353 Author: Mauro Carvalho Chehab All above are my e-mails. Let's point them all to mche...@kernel.org. > 1 Author: Mauro Carvalho Chehab This one is mangled and doesn't belong to me. So, it shouldn't be at the .mailmap file. > > > > Anyway, from a technical viewpoint your patches seem to fix > the .mailmap entry as the author intended. (See Junio's Email for the > documantation part) > But I would wait for the ack from Mauro and Shuah. With the above changes, Acked-by: Mauro Carvalho Chehab Thanks, Mauro -- 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] gitmodules: document shallow recommendation
Signed-off-by: Stefan Beller--- This goes on top of origin/sb/submodule-recommend-shallowness. I just realized we had an extra page for all the configuration options, so let's keep that in sync. Thanks, Stefan Documentation/gitmodules.txt | 5 + 1 file changed, 5 insertions(+) diff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt index ac70eca..b97b331 100644 --- a/Documentation/gitmodules.txt +++ b/Documentation/gitmodules.txt @@ -79,6 +79,11 @@ submodule..ignore:: "--ignore-submodule" option. The 'git submodule' commands are not affected by this setting. +submodule..shallow:: + When set to true, a clone of this submodule will be performed as a + shallow clone unless the user explicitely asks for a non-shallow + clone. + EXAMPLES -- 2.9.2.525.g1760797 -- 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: get_maintainer.pl and .mailmap entries with more than 2 addresses
On 08/02/2016 04:26 PM, Joe Perches wrote: > On Wed, 2016-08-03 at 00:17 +0200, Florian Mickler wrote: >> cc'd mche...@s-opensource.com (Mauro, is your kernel.org address up?) >> >> Am Tue, 02 Aug 2016 09:36:21 -0700 >> schrieb Joe Perches: >> >>> >>> Hello Florian. >>> There is at least an oddity with get_maintainer handling of a >>> .mailmap entry form. >>> >>> For instance: >>> >>> Mauro's .mailmap entry is: >>> Mauro Carvalho Chehab >>> >>> >>> >>> Is this a valid form? >>> >>> get_maintainer output for Mauro is: >>> >>> $ ./scripts/get_maintainer.pl drivers/media/ -f >>> Mauro Carvalho Chehab >>> >>> (maintainer:MEDIA INPUT INFRASTRUCTURE >>> (V4L/DVB)) >>> >>> I believe the Mauro's and Shuah's .mailmap entries are improper and >>> should be changed, but I'm not completely aware of git .mailmap >>> handling and the documentation seems weakly specified. >>> >> Hmm.. looking at Mauros last .mailmap commit it seems like your patch is >> ok for Mauro. >> >> Although and are probably >> missing? (@Mauro) >> >> >> $ git shortlog | grep "^Mauro C" >> Mauro Carvalho Chehab (4404): >> $ git log | grep "^Author:.*Mauro Carvalho Chehab" | sort | uniq -c >> 2 Author: Mauro Carvalho Chehab >> 146 Author: Mauro Carvalho Chehab >> 645 Author: Mauro Carvalho Chehab >> 794 Author: Mauro Carvalho Chehab >>2015 Author: Mauro Carvalho Chehab >> 448 Author: Mauro Carvalho Chehab >> 353 Author: Mauro Carvalho Chehab >> 1 Author: Mauro Carvalho Chehab >> >> >> >> Anyway, from a technical viewpoint your patches seem to fix >> the .mailmap entry as the author intended. (See Junio's Email for the >> documantation part) >> But I would wait for the ack from Mauro and Shuah. > > As far as I understand, a single entry with just their > name and preferred email address would work too because > the name parts are all spelled identically. > > I am fine with change to my entry. Thanks for fixing it. Acked-by: Shuah Khan thanks, -- Shuah -- 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] hashmap: clarify that hashmap_entry can safely be discarded
The API documentation said that the hashmap_entry structure to be embedded in the caller's structure is to be treated as opaque, which left the reader wondering if it can safely be discarded when it no longer is necessary. If the hashmap_entry structure had references to external resources such as allocated memory or an open file descriptor, merely free(3)ing the containing structure (when the caller's structure is on the heap) or letting it go out of scope (when it is on the stack) would end up leaking the external resource. Document that there is no need for hashmap_entry_clear() that corresponds to hashmap_entry_init() to give the API users a little bit of peace of mind. Signed-off-by: Junio C Hamano--- * As I wrote the text already, I thought I'd follow-up with a log message, too. Documentation/technical/api-hashmap.txt | 5 + 1 file changed, 5 insertions(+) diff --git a/Documentation/technical/api-hashmap.txt b/Documentation/technical/api-hashmap.txt index ad7a5bd..28f5a8b 100644 --- a/Documentation/technical/api-hashmap.txt +++ b/Documentation/technical/api-hashmap.txt @@ -104,6 +104,11 @@ If `free_entries` is true, each hashmap_entry in the map is freed as well `entry` points to the entry to initialize. + `hash` is the hash code of the entry. ++ +The hashmap_entry structure does not hold references to external resources, +and it is safe to just discard it once you are done with it (i.e. if +your structure was allocated with xmalloc(), you can just free(3) it, +and if it is on stack, you can just let it go out of scope). `void *hashmap_get(const struct hashmap *map, const void *key, const void *keydata)`:: -- 2.9.2-708-gdb68231 -- 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] apply: mark some file-local symbols static
Signed-off-by: Ramsay Jones--- Hi Christian, I had intended to ask you to squash this into your 'cc/apply-am' branch, specifically commit 4d18b33a (apply: move libified code from builtin/apply.c to apply.{c,h}, 30-07-2016). However, having read that commit a little closer, it seems that you deliberately made these symbols public. The commit message does not mention this issue at all, and it is not clear to me why these symbols should be public. What am I missing? ATB, Ramsay Jones apply.c | 26 +- apply.h | 14 -- 2 files changed, 13 insertions(+), 27 deletions(-) diff --git a/apply.c b/apply.c index 96f02fa..ec545f6 100644 --- a/apply.c +++ b/apply.c @@ -4743,16 +4743,16 @@ static int apply_patch(struct apply_state *state, return res; } -int apply_option_parse_exclude(const struct option *opt, - const char *arg, int unset) +static int apply_option_parse_exclude(const struct option *opt, + const char *arg, int unset) { struct apply_state *state = opt->value; add_name_limit(state, arg, 1); return 0; } -int apply_option_parse_include(const struct option *opt, - const char *arg, int unset) +static int apply_option_parse_include(const struct option *opt, + const char *arg, int unset) { struct apply_state *state = opt->value; add_name_limit(state, arg, 0); @@ -4760,9 +4760,9 @@ int apply_option_parse_include(const struct option *opt, return 0; } -int apply_option_parse_p(const struct option *opt, -const char *arg, -int unset) +static int apply_option_parse_p(const struct option *opt, + const char *arg, + int unset) { struct apply_state *state = opt->value; state->p_value = atoi(arg); @@ -4770,8 +4770,8 @@ int apply_option_parse_p(const struct option *opt, return 0; } -int apply_option_parse_space_change(const struct option *opt, - const char *arg, int unset) +static int apply_option_parse_space_change(const struct option *opt, + const char *arg, int unset) { struct apply_state *state = opt->value; if (unset) @@ -4781,8 +4781,8 @@ int apply_option_parse_space_change(const struct option *opt, return 0; } -int apply_option_parse_whitespace(const struct option *opt, - const char *arg, int unset) +static int apply_option_parse_whitespace(const struct option *opt, +const char *arg, int unset) { struct apply_state *state = opt->value; state->whitespace_option = arg; @@ -4791,8 +4791,8 @@ int apply_option_parse_whitespace(const struct option *opt, return 0; } -int apply_option_parse_directory(const struct option *opt, -const char *arg, int unset) +static int apply_option_parse_directory(const struct option *opt, + const char *arg, int unset) { struct apply_state *state = opt->value; strbuf_reset(>root); diff --git a/apply.h b/apply.h index 66ed0c5..010726e 100644 --- a/apply.h +++ b/apply.h @@ -107,20 +107,6 @@ struct apply_state { int applied_after_fixing_ws; }; -extern int apply_option_parse_exclude(const struct option *opt, - const char *arg, int unset); -extern int apply_option_parse_include(const struct option *opt, - const char *arg, int unset); -extern int apply_option_parse_p(const struct option *opt, - const char *arg, - int unset); -extern int apply_option_parse_whitespace(const struct option *opt, -const char *arg, int unset); -extern int apply_option_parse_directory(const struct option *opt, - const char *arg, int unset); -extern int apply_option_parse_space_change(const struct option *opt, - const char *arg, int unset); - extern int apply_parse_options(int argc, const char **argv, struct apply_state *state, int *force_apply, int *options, -- 2.9.0 -- 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] apply: mark some file-local symbols static
On Tue, Aug 2, 2016 at 3:33 PM, Ramsay Joneswrote: > > Signed-off-by: Ramsay Jones > --- > > Hi Christian, > > I had intended to ask you to squash this into your 'cc/apply-am' > branch, specifically commit 4d18b33a (apply: move libified code > from builtin/apply.c to apply.{c,h}, 30-07-2016). > > However, having read that commit a little closer, it seems that > you deliberately made these symbols public. The commit message > does not mention this issue at all, and it is not clear to me > why these symbols should be public. > > What am I missing? Their exports have been made obsolete by the reroll we have in 'pu' when "builtin/am: use apply api in run_apply()" was redone in a way not to duplicate the argument parsing. They should have been cleaned with 4820e13, but I think Christian did not carefully review the whole series before sending it out and did not notice that they no longer need to be extern. -- 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: [RFC/PATCH v11 09/13] bisect--helper: `bisect_write` shell function in C
Junio C Hamanowrites: > Pranit Bauva writes: > >> Reimplement the `bisect_write` shell function in C and add a >> `bisect-write` subcommand to `git bisect--helper` to call it from >> git-bisect.sh > > Up to around this step we've seen these patches well enough and I > think with another reroll or two, they are in good enough shape to > be split out and frozen for 'next'. We may not be there quite yet, > but I think we are getting pretty close. > > Thanks. By the way, this series applied as a whole seems to break t6030. -- 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 v6 06/16] merge_recursive: abort properly upon errors
Johannes Schindelinwrites: > I tend to think that the underscore is correct: this change is not so much > about the builtin (which is written with a dash) but about the function > (written with an underscore, used by more than just merge-recursive, e.g. > cherry-pick). Yes, I agree. "merge-recursive:" prefix is about either the built-in command, or the machinery as a whole to support that built-in command. It is preferrable to use "merge_recursive():" if we are talking about a single function. 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: get_maintainer.pl and .mailmap entries with more than 2 addresses
On Wed, 2016-08-03 at 00:17 +0200, Florian Mickler wrote: > cc'd mche...@s-opensource.com (Mauro, is your kernel.org address up?) > > Am Tue, 02 Aug 2016 09:36:21 -0700 > schrieb Joe Perches: > > > > > Hello Florian. > > There is at least an oddity with get_maintainer handling of a > > .mailmap entry form. > > > > For instance: > > > > Mauro's .mailmap entry is: > > Mauro Carvalho Chehab > > > > > > > > Is this a valid form? > > > > get_maintainer output for Mauro is: > > > > $ ./scripts/get_maintainer.pl drivers/media/ -f > > Mauro Carvalho Chehab > > > > (maintainer:MEDIA INPUT INFRASTRUCTURE > > (V4L/DVB)) > > > > I believe the Mauro's and Shuah's .mailmap entries are improper and > > should be changed, but I'm not completely aware of git .mailmap > > handling and the documentation seems weakly specified. > > > Hmm.. looking at Mauros last .mailmap commit it seems like your patch is > ok for Mauro. > > Although and are probably > missing? (@Mauro) > > > $ git shortlog | grep "^Mauro C" > Mauro Carvalho Chehab (4404): > $ git log | grep "^Author:.*Mauro Carvalho Chehab" | sort | uniq -c > 2 Author: Mauro Carvalho Chehab > 146 Author: Mauro Carvalho Chehab > 645 Author: Mauro Carvalho Chehab > 794 Author: Mauro Carvalho Chehab > 2015 Author: Mauro Carvalho Chehab > 448 Author: Mauro Carvalho Chehab > 353 Author: Mauro Carvalho Chehab > 1 Author: Mauro Carvalho Chehab > > > > Anyway, from a technical viewpoint your patches seem to fix > the .mailmap entry as the author intended. (See Junio's Email for the > documantation part) > But I would wait for the ack from Mauro and Shuah. As far as I understand, a single entry with just their name and preferred email address would work too because the name parts are all spelled identically. -- 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 1/2] submodule documentation: add options to the subcommand
Stefan Bellerwrites: > On Tue, Aug 2, 2016 at 2:45 PM, Junio C Hamano wrote: >> Stefan Beller writes: >> >>> When reading up on a subcommand of `gi submodule`, it is convenient >> >> s/gi /git /; > > will fix. > > And in the neighboring thread you just pointed out you used to just correct > spelling fixes like this. I think it really depends on the workflow of the > contributor. As I do the interdiff of the next version of the series against > your tree I'll be likely to notice such typos in the content, but not in > commit messages. "git tbdiff" is your friend ;-) -- 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
What's cooking in git.git (Aug 2016, #01; Tue, 2)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. On the 'master' front, the individual commit count now exceeds 400 since the last major release, which is a good pace. We may want to start slowing down once the current crop of topics in 'next' hits the 'master' and switch our attention to regression hunting. The 'maint' branch has been accumulating enough material to make it the next maintenance release v2.9.3. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [New Topics] * ab/gitweb-link-html-escape (2016-08-01) 1 commit - gitweb: escape link body in format_ref_marker The characters in the label shown for tags/refs for commits shown in "gitweb" output are now properly escaped for proper HTML output. Will merge to 'next'. * ew/build-time-pager-tweaks (2016-08-01) 1 commit . pager: move pager-specific setup into the build The build procedure learned PAGER_ENV knob that lists what default environment variable settings to export for popular pagers. This mechanism is used to tweak the default settings to MORE on FreeBSD. Waiting for a reroll. * ib/t3700-add-chmod-x-updates (2016-08-01) 3 commits - t3700: add a test_mode_in_index helper function - t3700: merge two tests into one - t3700: remove unwanted leftover files before running new tests The t3700 test about "add --chmod=-x" have been made a bit more robust and generally cleaned up. Will merge to 'next'. * jt/format-patch-from-config (2016-08-01) 1 commit - format-patch: format.from gives the default for --from "git format-patch" learned format.from configuration variable to specify the default settings for its "--from" option. * rs/st-mult (2016-08-01) 1 commit - pass constants as first argument to st_mult() Micro optimization of st_mult() facility used to check the integer overflow coming from multiplication to compute size of memory allocation. Will merge to 'next'. * rs/use-strbuf-addstr (2016-08-01) 1 commit - use strbuf_addstr() for adding constant strings to a strbuf Will merge to 'next'. * sb/submodule-update-dot-branch (2016-08-01) 7 commits - submodule update: allow '.' for branch value - submodule--helper: add remote-branch helper - submodule-config: keep configured branch around - submodule--helper: fix usage string for relative-path - submodule update: narrow scope of local variable - submodule update: respect depth in subsequent fetches - t7406: future proof tests with hard coded depth A few updates to "git submodule update". Will merge to 'next'. * jc/hashmap-doc-init (2016-08-02) 1 commit - hashmap: clarify that hashmap_entry can safely be discarded The API documentation for hashmap was unclear if hashmap_entry can be safely discarded without any other consideration. State that it is safe to do so. -- [Stalled] * jh/clean-smudge-annex (2016-08-01) 9 commits - use smudgeToFile filter in recursive merge - use smudgeToFile filter in git am - better recovery from failure of smudgeToFile filter - warn on unusable smudgeToFile/cleanFromFile config - use smudgeToFile in git checkout etc - use cleanFromFile in git add - add smudgeToFile and cleanFromFile filter configs - clarify %f documentation - Merge branch 'cc/apply-am' into HEAD (this branch uses cc/apply-am.) The interface to "clean/smudge" filters require Git to feed the whole contents via pipe, which is suboptimal for some applications. "cleanFromFile/smudgeToFile" commands are the moral equilvalents for these filters but they interact with the files on the filesystem directly. This is starting to bit-rot, as the topic it is built upon keeps getting rerolled. I _think_ I rebased it correctly, but I would not be surprised if I made a mistake. Will discard if/when I have to do another rebase, preferring to have a fresh reroll directly from the author. * sb/bisect (2016-04-15) 22 commits . SQUASH??? . bisect: get back halfway shortcut . bisect: compute best bisection in compute_relevant_weights() . bisect: use a bottom-up traversal to find relevant weights . bisect: prepare for different algorithms based on find_all . bisect: rename count_distance() to compute_weight() . bisect: make total number of commits global . bisect: introduce distance_direction() . bisect: extract get_distance() function from code duplication . bisect: use commit instead of commit list as arguments when appropriate . bisect: replace clear_distance() by unique markers . bisect: use struct node_data array instead of int array . bisect: get rid of recursion in count_distance() . bisect:
Re: [PATCH 1/2] submodule documentation: add options to the subcommand
On Tue, Aug 2, 2016 at 2:45 PM, Junio C Hamanowrote: > Stefan Beller writes: > >> When reading up on a subcommand of `gi submodule`, it is convenient > > s/gi /git /; will fix. And in the neighboring thread you just pointed out you used to just correct spelling fixes like this. I think it really depends on the workflow of the contributor. As I do the interdiff of the next version of the series against your tree I'll be likely to notice such typos in the content, but not in commit messages. > >> to have its options nearby and not just at the top of the man page. >> Add the options to each subcommand. >> >> While at it, also document the `--checkout` option for `update`. > > I do find the resulting per-subcommand description easier to read > with this change. > > Perhaps we want to go one step further and change the SYNOPSIS so > that per-subcommand options are not described there at all? > I.e.e.g. > > -'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]... > +'git submodule' [--quiet] update [...] will do. My original intention was to get rid of the duplicates in the OPTIONS section, where each option has This option is only valid for and in its description. So I looked at the `stash` man page, which has the options listed with the subcommands (and also has [] in the SYNOPSIS but also some of the options as well). I think only the long option lists (i.e. those that are more than one line) will be collapsed. The short options are ok, specifically when you just want to know e.g. if foreach supports the recursive option. Then you can open the man page and have no need to scroll down to the foreach command. Thanks, Stefan -- 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 1/2] submodule documentation: add options to the subcommand
Stefan Bellerwrites: > When reading up on a subcommand of `gi submodule`, it is convenient s/gi /git /; > to have its options nearby and not just at the top of the man page. > Add the options to each subcommand. > > While at it, also document the `--checkout` option for `update`. I do find the resulting per-subcommand description easier to read with this change. Perhaps we want to go one step further and change the SYNOPSIS so that per-subcommand options are not described there at all? I.e.e.g. -'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]... +'git submodule' [--quiet] update [...] > @@ -231,7 +231,7 @@ As an example, +git submodule foreach \'echo $path > {backtick}git > rev-parse HEAD{backtick}'+ will show the path and currently checked out > commit for each submodule. > > -sync:: > +sync [--recursive] [--] [...]:: > Synchronizes submodules' remote URL configuration setting > to the value specified in .gitmodules. It will only affect those > submodules which already have a URL entry in .git/config (that is the -- 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 v6 06/16] merge_recursive: abort properly upon errors
Johannes Schindelinwrites: >> > There are a couple of places where return values never indicated errors >> > before, as wie simply died instead of returning. >> >> s/wie/we/; > > Right. What can I say, I am surrounded by too many Germans. > > I fixed this locally, in case another re-roll should be required. What you > have in `pu` looks correct to me, though. Let me know if you want me to > re-submit nevertheless. I usually do this kind of obvious typofix and consistency fix without even mentioning them in my review comments to reduce the noise levels. But that works better ONLY if the patch authors then fetch from 'pu' and replace their copies with what I fixed up already and base their reroll on top by amending and/or building on top (of course, that also requires my local fix must all be limited to uncontroversial ones). So either I should change my workflow and mention any and all typofixes in my review comments (which consumes the review bandwidth), or I should force patch authors to do the "fetch from 'pu' and replace" somehow to avoid this kind of back-and-forth. I am not sure which should be the way to go. -- 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 v5 14/16] merge-recursive: offer an option to retain the output in 'obuf'
Johannes Schindelinwrites: >> But in that case, there would be both messages meant for the >> standard output and also meant for the standard error, and we need >> some way to make sure they go to the right channel. > > Not necessarily. Let's have a look at our existing code in > git-rebase.sh: > > output () { > case "$verbose" in > '') > output=$("$@" 2>&1 ) > status=$? > test $status != 0 && printf "%s\n" "$output" > return $status > ;; > *) > "$@" > ;; > esac > } > > This incredibly well-named function (, my fault: dfa49f3 (Shut "git > rebase -i" up when no --verbose was given, 2007-07-23)) accumulates all > output, both stdout and stderr, and shows it only in case of an error. > > Crucially, *all* output goes to stdout. No distinction is being made > between stdout and stderr. > ... > This is the existing behavior of rebase -i. > ... > As such, it would be a serious mistake to implement that mode and use it > in the rebase--helper: it would very likely cause regressions in existing > scripts, probably even my own. Sounds like we are desperately trying to find an excuse to do a wrong thing by finding an existing piece of code that did a wrong thing already. That leaves a bad taste in my mouth, but as "rebase -i" is meant to be an "interactive" command, I would imagine that nobody would have expected to run it as "git rebase -i >/dev/null" in order to view only the error messages (or vice versa with "2>errs"). So OK then, at least for now. -- 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 1/1 v2] pager: move pager-specific setup into the build
Eric Wongwrites: > So v3 will be MORE=FRX, as less was added: > > commit 98170c0c3ba86eb1cc975e7848d075bf2abc1ed0 > Author: ps > Date: Mon May 22 10:00:00 2000 + > > bmake glue for less. > > and more was nuked: > > commit cde9059fa3e4dc7e259c3864d7536252a5c580a0 > Author: ps > Date: Mon May 29 13:31:51 2000 + > > Nuke more from the repository. > > And "git branch -r --contains" on both of those commits says > they showed up in the 5.0 release. However, further > investigation says more was even gone by the 4.1.0 release > > git show origin/release/4.1.0:usr.bin/more # non-existent tree > git show origin/release/4.0.0:usr.bin/more # tree still exists > > But, "git show origin/release/4.0.0:usr.bin/more/option.c" > reveals more from those days wouldn't handle -R anyways, > and hopefully nobody is still running 4.0.0... OK. And they can always set $MORE on their own to work it around, or we can do a release-dependent tweak when somebody screams. -- 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 1/1 v2] pager: move pager-specific setup into the build
Jeff Kingwrites: > On Mon, Aug 01, 2016 at 04:03:40PM -0700, Junio C Hamano wrote: > >> Eric Wong writes: >> >> > From: Junio C Hamano >> > >> > Allowing PAGER_ENV to be set at build-time allows us to move >> > pager-specific knowledge out of our build. Currently, this >> > allows us to set a better default for FreeBSD where more(1) >> > is the same binary as less(1). >> >> Thanks for resurrecting, but I am not sure what "a better default" >> is from the above description and with the patch. Even though a >> naive reading of the above (i.e. "less" and "more" are the same) >> makes me expect that the patch will give the same set of default >> environment settings to those on FreeBSD, you give LESS=FRX and >> MORE=-R, i.e. they are configured differently. > > I wondered about this, too. They are the same binary, but calling less > as "more" (or setting LESS_IS_MORE) causes less to behave "like more". I guessed that, but if that is the case, "more is the same binary" is irrelevant. "more" behaves differently from "less" might be, but what "less" does is much less important than "more needs this default setting to work pleasantly", which is what is missing. So I'd say Allowing PAGER_ENV to be set at build-time allows us to move pager-specific knowledge out of our build. This allows us to set a better default for FreeBSD more(1), which misbehaves if MORE environment variable is left empty $in_such_and_such_way, by defaulting it to -R. without even mentioning anything about "less" may be a more understandable justification for a patch that sets MORE only on FreeBSD. -- 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: git bisect for reachable commits only
Oleg Taranenkowrites: > First, assuming the common ancestor is GOOD based on the fact that > some descendant given as GOOD is pretty bad idea. What you claim is fundamentally incompatible with the way "bisect" works as a O(log(n)) operation. It is likely that your definition of Good for the purpose of your bug-hunting needs to be rethought if you want to take advantage of "bisect". > I have another request to get git bisect more user-friendly, regarding > rolling back last step or steps, if accidentally 'git bisect bad' or > 'good' was wrong entered, but I think it worth for another thread. Are you aware that you can check $GIT_DIR/BISECT_LOG and replay it to recreate any previous state of the bisection? That would probably help. -- 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: [RFC/PATCH v11 13/13] bisect--helper: `bisect_start` shell function partially in C
Pranit Bauvawrites: > +static int bisect_start(struct bisect_terms *terms, int no_checkout, > + const char **argv, int argc) > +{ > + int i, j, has_double_dash = 0, must_write_terms = 0, bad_seen = 0; > + int flag; > + struct string_list revs = STRING_LIST_INIT_DUP; > + struct string_list states = STRING_LIST_INIT_DUP; > + struct strbuf start_head = STRBUF_INIT; > + const char *head; > + unsigned char sha1[20]; > + FILE *fp; > + struct object_id oid; > + > + if (is_bare_repository()) > + no_checkout = 1; > + > + for(i = 0; i < argc; i++) { SP after for. > + if (!strcmp(argv[i], "--")) { > + has_double_dash = 1; > + break; > + } > + if (!strcmp(argv[i], "--term-good")) { > + must_write_terms = 1; > + strbuf_reset(>term_good); > + strbuf_addstr(>term_good, argv[++i]); > + break; > + } > + if (!strcmp(argv[i], "--term-bad")) { > + must_write_terms = 1; > + strbuf_reset(>term_bad); > + strbuf_addstr(>term_bad, argv[++i]); > + break; > + } The original was not careful, either, but what if the user ends the command line with "--term-good", without anything after it? Also the original is prepared to handle --term-good=boa; because this function can be be called directly from the UI (i.e. "git bisect start --term-good=boa"), not supporting that form would be seen as a regression. > + if (starts_with(argv[i], "--") && > + !one_of(argv[i], "--term-good", "--term-bad", NULL)) { > + string_list_clear(, 0); > + string_list_clear(, 0); > + die(_("unrecognised option: '%s'"), argv[i]); > + } > + if (get_oid(argv[i], ) || has_double_dash) { Calling get_oid() alone is insufficient to make sure argv[i] refers to an existing object that is a committish. The "^{commit}" suffix in the original is there for a reason. > + string_list_clear(, 0); > + string_list_clear(, 0); You seem to want the revs list really really clean ;-) > + die(_("'%s' does not appear to be a valid revision"), > argv[i]); > + } > + else > + string_list_append(, oid_to_hex()); > + } > + > + for (j = 0; j < revs.nr; j++) { Why "j", not "i", as clearly the previous loop has finished at this point? The only reason why replacing "j" with "i" would make this function buggy would be if a later part of this function depended on the value of "i" when the control left the above loop, but if that were the case (I didn't check carefully), such a precious value that has long term effect throughout the remainder of the function must not be kept in an otherwise throw-away loop counter variable "i". Introduce a new "int pathspec_pos" and set it to "i" immediately after the "for (i = 0; i < argc; i++) { ... }" loop above, perhaps. > + struct strbuf state = STRBUF_INIT; > + /* > + * The user ran "git bisect start ", hence > + * did not explicitly specify the terms, but we are already > + * starting to set references named with the default terms, > + * and won't be able to change afterwards. > + */ > + must_write_terms = 1; > + > + if (bad_seen) > + strbuf_addstr(, terms->term_good.buf); > + else { > + bad_seen = 1; > + strbuf_addstr(, terms->term_bad.buf); > + } > + string_list_append(, state.buf); > + strbuf_release(); > + } How about this instead? /* * that comment block goes here */ must_write_terms = !!revs.nr; for (i = 0; i < revs.nr; i++) { if (bad_seen) string_list_append(, terms->term_good.buf); else string_list_append(, terms->term_bad.buf); } > + > + /* > + * Verify HEAD > + */ > + head = resolve_ref_unsafe("HEAD", 0, sha1, ); The last parameter is a set of flag bits, so call it flags. > + if (!head) { > + if (get_sha1("HEAD", sha1)) { > + string_list_clear(, 0); > + string_list_clear(, 0); > + die(_("Bad HEAD - I need a HEAD")); We see many repeated calls to clear these two string lists before exiting with failure, either by dying or return -1. I wonder how bad the resulting code would look like if we employed the standard pattern of having a "fail_return:" label at the end of the function (after the "return"
Re: [RFC/PATCH v11 09/13] bisect--helper: `bisect_write` shell function in C
Pranit Bauvawrites: > Reimplement the `bisect_write` shell function in C and add a > `bisect-write` subcommand to `git bisect--helper` to call it from > git-bisect.sh Up to around this step we've seen these patches well enough and I think with another reroll or two, they are in good enough shape to be split out and frozen for 'next'. We may not be there quite yet, but I think we are getting pretty close. 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 v3 03/10] pkt-line: add packet_flush_gentle()
On Sun, Jul 31, 2016 at 11:45:08PM +0200, Lars Schneider wrote: > > > On 31 Jul 2016, at 22:36, Torstem Bögershausenwrote: > > > > > > > >> Am 29.07.2016 um 20:37 schrieb larsxschnei...@gmail.com: > >> > >> From: Lars Schneider > >> > >> packet_flush() would die in case of a write error even though for some > >> callers > >> an error would be acceptable. > > What happens if there is a write error ? > > Basically the protocol is out of synch. > > Lenght information is mixed up with payload, or the other way > > around. > > It may be, that the consequences of a write error are acceptable, > > because a filter is allowed to fail. > > What is not acceptable is a "broken" protocol. > > The consequence schould be to close the fd and tear down all > > resources. connected to it. > > In our case to terminate the external filter daemon in some way, > > and to never use this instance again. > > Correct! That is exactly what is happening in kill_protocol2_filter() > here: Wait a second. Is kill the same as shutdown ? I would expect that The process terminates itself as soon as it detects EOF. As there is nothing more read. Then the next question: The combination of kill & protocol in kill_protocol(), what does it mean ? Is it more like a graceful shutdown_protocol() ? -- 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: [RFC/PATCH v11 12/13] bisect--helper: `get_terms` & `bisect_terms` shell function in C
Pranit Bauvawrites: > +static int bisect_terms(struct bisect_terms *terms, int term_defined) > +{ > + if (get_terms(terms)) { > + fprintf(stderr, "no terms defined\n"); > + return -1; > + } > + if (!term_defined) { > + printf("Your current terms are %s for the old state\nand " > +"%s for the new state.\n", terms->term_good.buf, > +terms->term_bad.buf); > + return 0; > + } In the original, all of the above messages go through gettext; this rewrite should do the same. -- 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: [RFC/PATCH v11 11/13] bisect--helper: `bisect_next_check` shell function in C
Pranit Bauvawrites: > +static int mark_good(const char *refname, const struct object_id *oid, > + int flag, void *cb_data) > +{ > + int *m_good = (int *)cb_data; > + *m_good = 0; > + return 0; > +} See below. > +static int bisect_next_check(const struct bisect_terms *terms, > + const char *current_term) > +{ > + int missing_good = 1, missing_bad = 1; It is somewhat unusual to start with "assume we are OK" and then "it turns out that we are not". > + char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad.buf); > + char *good_glob = xstrfmt("%s*", terms->term_good.buf); The original runs git for-each-ref "refs/bisect/$TERM_GOOD-* but this one lacks the final dash. > + if (ref_exists(bad_ref)) > + missing_bad = 0; > + free(bad_ref); > + > + for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/", > + (void *) _good); > + free(good_glob); The for-each helper does not return until it iterates over all the matching refs, but you are only interested in seeing if at least one exists. It may make sense to return 1 from mark_good() to terminate the traversal early. > + if (!missing_good && !missing_bad) > + return 0; > + > + if (!current_term) > + return -1; > + > + if (missing_good && !missing_bad && current_term && > + !strcmp(current_term, terms->term_good.buf)) { > + char *yesno; > + /* > + * have bad (or new) but not good (or old). We could bisect > + * although this is less optimum. > + */ > + fprintf(stderr, "Warning: bisecting only with a %s commit\n", > + terms->term_bad.buf); In the original, this message goes through gettext. > + if (!isatty(0)) > + return 0; > + /* > + * TRANSLATORS: Make sure to include [Y] and [n] in your > + * translation. The program will only accept English input > + * at this point. > + */ > + yesno = git_prompt(_("Are you sure [Y/n]? "), PROMPT_ECHO); > + if (starts_with(yesno, "N") || starts_with(yesno, "n")) > + return -1; > + return 0; > + } When the control falls into the above if(){} block, the function will always return. It will clarify that this is the end of such a logical block to have a blank line here. > + if (!is_empty_or_missing_file(git_path_bisect_start())) > + return error(_("You need to give me at least one good|old and " > + "bad|new revision. You can use \"git bisect " > + "bad|new\" and \"git bisect good|old\" for " > + "that. \n")); > + else > + return error(_("You need to start by \"git bisect start\". " > + "You then need to give me at least one good|" > + "old and bad|new revision. You can use \"git " > + "bisect bad|new\" and \"git bisect good|old\" " > + " for that.\n")); The i18n on these two messages seem to be different from the original, which asks bisect_voc to learn what 'bad' and 'good' are called and attempts to use these words from the vocabulary. -- 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: [RFC/PATCH v11 10/13] bisect--helper: `check_and_set_terms` shell function in C
Pranit Bauvawrites: > Reimplement the `check_and_set_terms` shell function in C and add > `check-and-set-terms` subcommand to `git bisect--helper` to call it from > git-bisect.sh > > Using `--check-and-set-terms` subcommand is a temporary measure to port > shell function in C so as to use the existing test suite. As more > functions are ported, this subcommand will be retired and will be called > by some other methods. I think "this subcommand will be retired but its implementation will be called by ..." would clarify the direction. > + if (!no_term_file && > + strcmp(cmd, terms->term_bad.buf) && > + strcmp(cmd, terms->term_good.buf)) > + return error(_("Invalid command: you're currently in a " > + "'%s' '%s' bisect"), terms->term_bad.buf, This changes a message text, switching from "... good/bad bisect." to "... 'good' 'bad' bisect". Intended? -- 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: [RFC/PATCH v11 04/13] bisect--helper: `bisect_clean_state` shell function in C
Pranit Bauvawrites: > +static int bisect_clean_state(void) > +{ > + int result = 0; > + > + /* There may be some refs packed during bisection */ > + struct string_list refs_for_removal = STRING_LIST_INIT_NODUP; > + for_each_ref_in("refs/bisect/", mark_for_removal, (void *) > _for_removal); > + string_list_append(_for_removal, xstrdup("BISECT_HEAD")); > + result = delete_refs(_for_removal); > + refs_for_removal.strdup_strings = 1; > + string_list_clear(_for_removal, 0); > + remove_path(git_path_bisect_expected_rev()); > + remove_path(git_path_bisect_ancestors_ok()); > + remove_path(git_path_bisect_log()); > + remove_path(git_path_bisect_names()); > + remove_path(git_path_bisect_run()); > + remove_path(git_path_bisect_terms()); > + /* Cleanup head-name if it got left by an old version of git-bisect */ > + remove_path(git_path_head_name()); > + * Cleanup BISECT_START last to support the --no-checkout option > + * introduced in the commit 4796e823a. > + */ > + remove_path(git_path_bisect_start()); I can see that refs/files-backend.c misuses it already, but remove_path() helper is about removing a path in the working tree, together with any parent directory that becomes empty due to the removal. You do not expect $GIT_DIR/ to become an empty directory after removing $GIT_DIR/BISECT_LOG nor want to rmdir $GIT_DIR even if it becomes empty. It is a wrong helper function to use here. Also you do not seem to check the error from the function to smudge the "result" you are returning from this function. Isn't unlink_or_warn() more correct helper to use here? > + return 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: [RFC/PATCH v11 08/13] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
Pranit Bauvawrites: > + for (i = 0; i < rev_nr; i++) { > + if (!is_expected_rev(revs[i])) { > + remove_path(git_path_bisect_ancestors_ok()); > + remove_path(git_path_bisect_expected_rev()); > + return 0; Same comment on the use of this helper function applies here, too. -- 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: [RFC/PATCH v11 07/13] bisect--helper: `bisect_reset` shell function in C
Pranit Bauvawrites: > + if (!file_exists(git_path_bisect_head())) { > + struct argv_array argv = ARGV_ARRAY_INIT; > + argv_array_pushl(, "checkout", branch.buf, "--", NULL); > + if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) { > + error(_("Could not check out original HEAD '%s'. Try " > + "'git bisect reset '."), branch.buf); Somebody seems to have a keen eye. Looks much better with a space after "Try" ;-) -- 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/2] submodule update documentation: don't repeat ourselves
The documentation for the `git submodule update` command, repeats itself for each update option, "This is done when is given, or no option is given and `submodule..update` is set to . Avoid these repetitive clauses by stating the command line options take precedence over configured options. Also add 'none' to the list of options instead of mentioning it in the following running text and split the list into two parts, one that is accessible via the command line and one that is only reachable via the configuration variables. Signed-off-by: Stefan Beller--- Documentation/git-submodule.txt | 30 ++ 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt index 1452c31..9b5abaf 100644 --- a/Documentation/git-submodule.txt +++ b/Documentation/git-submodule.txt @@ -158,13 +158,13 @@ Update the registered submodules to match what the superproject expects by cloning missing submodules and updating the working tree of the submodules. The "updating" can be done in several ways depending on command line options and the value of `submodule..update` -configuration variable. Supported update procedures are: +configuration variable. The command line option takes precedence over +the configuration variable. if neither is given, a checkout is performed. +update procedures supported both from the command line as well as setting +`submodule..update`: checkout;; the commit recorded in the superproject will be - checked out in the submodule on a detached HEAD. This is - done when `--checkout` option is given, or no option is - given, and `submodule..update` is unset, or if it is - set to 'checkout'. + checked out in the submodule on a detached HEAD. + If `--force` is specified, the submodule will be checked out (using `git checkout --force` if appropriate), even if the commit specified @@ -172,23 +172,21 @@ in the index of the containing repository already matches the commit checked out in the submodule. rebase;; the current branch of the submodule will be rebased - onto the commit recorded in the superproject. This is done - when `--rebase` option is given, or no option is given, and - `submodule..update` is set to 'rebase'. + onto the commit recorded in the superproject. merge;; the commit recorded in the superproject will be merged - into the current branch in the submodule. This is done - when `--merge` option is given, or no option is given, and - `submodule..update` is set to 'merge'. + into the current branch in the submodule. + +The following procedures are only available via the `submodule..update` +configuration variable: custom command;; arbitrary shell command that takes a single argument (the sha1 of the commit recorded in the - superproject) is executed. This is done when no option is - given, and `submodule..update` has the form of - '!command'. + superproject) is executed. When `submodule..update` + is set to '!command', the remainder after the exclamation mark + is the custom command. -When no option is given and `submodule..update` is set to 'none', -the submodule is not updated. + none;; the submodule is not updated. If the submodule is not yet initialized, and you just want to use the setting as stored in .gitmodules, you can automatically initialize the -- 2.9.2.525.g1760797 -- 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/2] submodule documentation: add options to the subcommand
When reading up on a subcommand of `gi submodule`, it is convenient to have its options nearby and not just at the top of the man page. Add the options to each subcommand. While at it, also document the `--checkout` option for `update`. Signed-off-by: Stefan Beller--- This mini series applies to master, there no other patches in flight that touch Documentation/git-submodules.txt except sb/submodule-default-paths, which we are holding back currently. Thanks, Stefan Documentation/git-submodule.txt | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt index bf3bb37..1452c31 100644 --- a/Documentation/git-submodule.txt +++ b/Documentation/git-submodule.txt @@ -15,7 +15,7 @@ SYNOPSIS 'git submodule' [--quiet] init [--] [...] 'git submodule' [--quiet] deinit [-f|--force] (--all|[--] ...) 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch] - [--[no-]recommend-shallow] [-f|--force] [--rebase|--merge] + [--[no-]recommend-shallow] [-f|--force] [--checkout|--rebase|--merge] [--reference ] [--depth ] [--recursive] [--jobs ] [--] [...] 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ] @@ -62,7 +62,7 @@ if you choose to go that route. COMMANDS -add:: +add [-b ] [-f|--force] [--name ] [--reference ] [--depth ] [--] []:: Add the given repository as a submodule at the given path to the changeset to be committed next to the current project: the current project is termed the "superproject". @@ -103,7 +103,7 @@ together in the same relative location, and only the superproject's URL needs to be provided: git-submodule will correctly locate the submodule using the relative URL in .gitmodules. -status:: +status [--cached] [--recursive] [--] [...]:: Show the status of the submodules. This will print the SHA-1 of the currently checked out commit for each submodule, along with the submodule path and the output of 'git describe' for the @@ -120,7 +120,7 @@ submodules with respect to the commit recorded in the index or the HEAD, linkgit:git-status[1] and linkgit:git-diff[1] will provide that information too (and can also report changes to a submodule's work tree). -init:: +init [--] [...]:: Initialize the submodules recorded in the index (which were added and committed elsewhere) by copying submodule names and urls from .gitmodules to .git/config. @@ -135,7 +135,7 @@ init:: the explicit 'init' step if you do not intend to customize any submodule locations. -deinit:: +deinit [-f|--force] (--all|[--] ...):: Unregister the given submodules, i.e. remove the whole `submodule.$name` section from .git/config together with their work tree. Further calls to `git submodule update`, `git submodule foreach` @@ -151,7 +151,7 @@ instead of deinit-ing everything, to prevent mistakes. If `--force` is specified, the submodule's working tree will be removed even if it contains local modifications. -update:: +update [--init] [--remote] [-N|--no-fetch] [--[no-]recommend-shallow] [-f|--force] [--checkout|--rebase|--merge] [--reference ] [--depth ] [--recursive] [--jobs ] [--] [...]:: + -- Update the registered submodules to match what the superproject @@ -197,7 +197,7 @@ submodule with the `--init` option. If `--recursive` is specified, this command will recurse into the registered submodules, and update any nested submodules within. -- -summary:: +summary [--cached|--files] [(-n|--summary-limit) ] [commit] [--] [...]:: Show commit summary between the given commit (defaults to HEAD) and working tree/index. For a submodule in question, a series of commits in the submodule between the given super project commit and the @@ -210,7 +210,7 @@ summary:: Using the `--submodule=log` option with linkgit:git-diff[1] will provide that information too. -foreach:: +foreach [--recursive] :: Evaluates an arbitrary shell command in each checked out submodule. The command has access to the variables $name, $path, $sha1 and $toplevel: @@ -231,7 +231,7 @@ As an example, +git submodule foreach \'echo $path {backtick}git rev-parse HEAD{backtick}'+ will show the path and currently checked out commit for each submodule. -sync:: +sync [--recursive] [--] [...]:: Synchronizes submodules' remote URL configuration setting to the value specified in .gitmodules. It will only affect those submodules which already have a URL entry in .git/config (that is the -- 2.9.2.525.g1760797 -- 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: get_maintainer.pl and .mailmap entries with more than 2 addresses
Joe Percheswrites: > Hello Florian. > > There is at least an oddity with get_maintainer handling of a > .mailmap entry form. > > For instance: > > Mauro's .mailmap entry is: > Mauro Carvalho Chehab > > > > Is this a valid form? I do not think so, according to "git shortlog --help" (the canonical source of the document is Documentation/mailmap.txt, but shortlog doc includes it). Here is the relevant bits. In the simple form, each line in the file consists of the canonical real name of an author, whitespace, and an email address used in the commit (enclosed by '<' and '>') to map to the name. For example: -- Proper Name -- The more complex forms are: -- -- which allows mailmap to replace only the email part of a commit, and: -- Proper Name -- which allows mailmap to replace both the name and the email of a commit matching the specified commit email address, and: -- Proper Name Commit Name -- which allows mailmap to replace both the name and the email of a commit matching both the specified commit name and email address. -- 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] 1/4] patch-ids: stop using a hand-rolled hashmap implementation
Junio C Hamanowrites: > Johannes Schindelin writes: > >> Oh, we are already safely in Unrelated Tangent Land for a while, I would >> think. Nothing of what we are discussing in this thread has anything to do >> with Kevin's patch series,... > > Oh, no question about that. Go back to my review, to which your > message I am responding to is a reply to. What you wrote are all > about things after "This is a tangent, but this made me wonder if it > is safe to simply free(3) the result...", which pointed out rough > points in the hashmap API from the API user's point of view and > suggested a few possible improvement opportunities. In other words, I'd be happy with a patch like this, outside the scope of and independent from this series. When the hashmap_entry structure does acquire references to external resources (which I wouldn't judge the likelihood of), this paragraph will become stale, but that is exactly the point at which _clear() function needs to be added to the API and described here, replacing this paragraph. I do not mind having an empty _clear() before that happens, but I do not think it adds much safety. A disciplined user of the API may call that empty _clear() to make her code future-proof, but we know there are undisciplined developers and reviewers and there will be codepaths that call _init() without calling the empty _clear(), and we won't notice it. Whoever is adding the real need for _clear() must audit the codebase at that point _anyway_, and that is why I think having an empty _clear() before would not buy us much. Documentation/technical/api-hashmap.txt | 5 + 1 file changed, 5 insertions(+) diff --git a/Documentation/technical/api-hashmap.txt b/Documentation/technical/api-hashmap.txt index ad7a5bd..1dcec3d 100644 --- a/Documentation/technical/api-hashmap.txt +++ b/Documentation/technical/api-hashmap.txt @@ -104,6 +104,11 @@ If `free_entries` is true, each hashmap_entry in the map is freed as well `entry` points to the entry to initialize. + `hash` is the hash code of the entry. ++ +The hashmap_entry structure does not hold references to external resources, +and it is safe to just discard it once you are done with it (i.e. if +your structure was allocated with xmalloc(), you can just free(3) it, +and if it is on stack, you can just let it go out of scope). `void *hashmap_get(const struct hashmap *map, const void *key, const void *keydata)`:: -- 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: [RFC/PATCH v11 02/13] bisect: rewrite `check_term_format` shell function in C
Pranit Bauvawrites: > +/* > + * Check whether the string `term` belongs to the set of strings > + * included in the variable arguments. > + */ > +static int one_of(const char *term, ...) > +{ > + int res = 0; > + va_list matches; > + const char *match; > + > + va_start(matches, term); > + while (!res && (match = va_arg(matches, const char *))) > + res = !strcmp(term, match); > + va_end(matches); > + > + return res; > +} It might be safer to mark this function with LAST_ARG_MUST_BE_NULL, but because this is static to this function, it may not matter too much. Just an observation, not a strong suggestion to change the patch. > +static int check_term_format(const char *term, const char *orig_term) > +{ > + int res; > + char *new_term = xstrfmt("refs/bisect/%s", term); > + > + res = check_refname_format(new_term, 0); > + free(new_term); Yup, that looks much more straight-forward than using a one-time-use strbuf. -- 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: [RFC/PATCH v11 03/13] bisect--helper: `write_terms` shell function in C
Pranit Bauvawrites: > +static int write_terms(const char *bad, const char *good) > +{ > + FILE *fp; > + int res; > + > + if (!strcmp(bad, good)) > + return error(_("please use two different terms")); > + > + if (check_term_format(bad, "bad") || check_term_format(good, "good")) > + return -1; > + > + fp = fopen(git_path_bisect_terms(), "w"); > + if (!fp) > + return error_errno(_("could not open the file BISECT_TERMS")); > + > + res = fprintf(fp, "%s\n%s\n", bad, good); > + res |= fclose(fp); > + return (res < 0) ? -1 : 0; > +} If fprintf(3) were a function that returns 0 on success and negative on error (like fclose(3) is), the pattern to cascade the error return with "res |= another_call()" is appropriate, but the made me hiccup a bit while reading it. It is not wrong per-se and it would certainly be making it worse if we did something silly like res = fprintf(...) < 0 ? -1 : 0; res |= fclose(fp); so I guess what you have is the most succinct way to do this. -- 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: git bisect for reachable commits only
On Tue, Aug 2, 2016 at 3:15 AM, Oleg Taranenkowrote: > Guys, > > thanks for discussion, I will try to reply in bulk here. > First, assuming the common ancestor is GOOD based on the fact that > some descendant given as GOOD is pretty bad idea. > It may be, but may not be. In the git-flow like workflows new features > (aka branches) are created from trunk (master/develop/...) > sporadically, > but later they will mutual merging. I would say more probably they > have not common base, then have. git bisect has the underlying assumption that the BUG is not there initially and introduced by one specific commit, and continues to be there until B. With this assumption you can do a binary search, which allows searching in O(log n), which is optimal for the number of iterations needed. > > Second, I don't ask "create a new algorithm to find all transition > from good/old to bad/new", not nesessary. If programmer feels > something > suspicious, he/she can create another bisect session with narrowed commit > range. > > Third, testing of any specific commit can be very expensive operation. > In my case - shutdown servers/refresh dbs/clean/rebuild in > eclipse/running servers/dropping browser cache/running app in > browser/going through some pages/view UI. > Some of steps of course are automated, but some not. Anyway I spend > 5-10 min for every iteration. So knowing what commit is bad or good is > very valuable, then I'm very interested to hunt the bug-introduced > commit with minimal count of testing. As you point out each iteration is a burden to the user, so they should be kept to a minimum. > > Scenario 4 (I will keep my previous mail numbering for possible later > reference) > z1z2---z3 > / / \ > Gx1x2/---x3---x4--B > \ / / >y1--y2--y3--y4 > > This is the happy straight case with closed DAG (hehe, git for > scientists) between given G good and B bad commits. > Ideal bisect will check first the shortest way between G & B: > x1/x2/x3/x4. Let name first-bug commit we are really hunting H and > current first-bug candidate as h. > If h == x1 or x2 -> stop, found > If h == x3, bisect will try to test y2/y3/y4 path only > If h == x4, bisect will select shortest path z1/z2 (keeping in mind, > that x2 is already tested and is good) > If h == z1 - found > if h == z2 - looking in path y1/y2/y3 * git is agnostic of the workflow, i.e. it doesn't know the notion of topic branches or such. * What is the worst case in you strategy? (h=y4 means 7 tests by the user IIUC) Given the assumptions as laid out above such that we are able to do a binary search, the ideal candidate for scenario 4 is y4 or z3 as these split the set of commits to be investigated into 2 equally sized sets of ancestors and non-ancestors. When a specific workflow is given, it may make sense to weight commits differently. Also some people asked for a strategy that only checks merge commits first, as there are far fewer merge commits and these generally are used for introducing a new feature or refactoring. -- 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: [RFC/PATCH v11 01/13] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
Pranit Bauvawrites: > `--next-all` is meant to be used as a subcommand to support multiple > "operation mode" though the current implementation does not contain any > other subcommand along side with `--next-all` but further commits will > include some more subcommands. Sounds sensible. As long as the dispatch happens inside cmd_bisect__helper() itself, limiting the enum definition local to the function also looks like a good thing to do (and I do not see a reason why we need the world outside this function to know about the enum in the future). > Helped-by: Johannes Schindelin > Mentored-by: Lars Schneider > Mentored-by: Christian Couder > Signed-off-by: Pranit Bauva > --- > builtin/bisect--helper.c | 17 +++-- > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 3324229..8111c91 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -10,11 +10,11 @@ static const char * const git_bisect_helper_usage[] = { > > int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > { > - int next_all = 0; > + enum { NEXT_ALL = 1 } cmdmode = 0; > int no_checkout = 0; > struct option options[] = { > - OPT_BOOL(0, "next-all", _all, > - N_("perform 'git bisect next'")), > + OPT_CMDMODE(0, "next-all", , > + N_("perform 'git bisect next'"), NEXT_ALL), > OPT_BOOL(0, "no-checkout", _checkout, >N_("update BISECT_HEAD instead of checking out the > current commit")), > OPT_END() > @@ -23,9 +23,14 @@ int cmd_bisect__helper(int argc, const char **argv, const > char *prefix) > argc = parse_options(argc, argv, prefix, options, >git_bisect_helper_usage, 0); > > - if (!next_all) > + if (!cmdmode) > usage_with_options(git_bisect_helper_usage, options); > > - /* next-all */ > - return bisect_next_all(prefix, no_checkout); > + switch (cmdmode) { > + case NEXT_ALL: > + return bisect_next_all(prefix, no_checkout); > + default: > + die("BUG: unknown subcommand '%d'", cmdmode); > + } > + return 0; > } > > -- > https://github.com/git/git/pull/281 -- 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] t7063: work around FreeBSD's lazy mtime update feature
Duy Nguyenwrites: > OK how about this squashed in? The name was taken from fbsd definition > IN_LAZYMOD. I am sorry that I didn't spot this possiblity earlier, but do we need anything conditional? Either FREEBSD or LAZYMOD prerequisite tells very little what the "Work around lazy mtime update" is and where it triggers (I think the conclusion of your investigation was that the timestamp on the containing directory, but that is ONLY recorded in the log message, i.e. readers need to run 'git blame' to find out). It might be a better approach to have a helper function with descriptive name and comment early in t7063, e.g. # On some filesystems (e.g. FreeBSD's ext2 and ufs) this # and that happens when we do blah, which forces the # untracked cache code to take the slow path. A test # that wants to make sure the fast path works correctly # should call this helper to make mtime of the containing # directory in sync with the reality after doing blah and # before checking the fast path behaviour test_sync_directory_mtime () { ls -ld . >/dev/null } and then call it at strategic places without any prerequisite. The helper may turn out to be useful outside the context of 7063 later, at which time we can move it to test-lib-functions, but that is a separate step. > Off topic. Since I found this macro defined twice, in ext2 and ufs, > but not in zfs (found its source!), I assume zfs does not have this > particular feature (but I didn't test it). Untracked cache may be more > effecient there. > -- 8< -- > diff --git a/t/t7063-status-untracked-cache.sh > b/t/t7063-status-untracked-cache.sh > index 08fc586..8bb048a 100755 > --- a/t/t7063-status-untracked-cache.sh > +++ b/t/t7063-status-untracked-cache.sh > @@ -419,7 +419,7 @@ test_expect_success 'create/modify files, some of which > are gitignored' ' > rm base > ' > > -test_expect_success FREEBSD 'Work around lazy mtime update' ' > +test_expect_success LAZYMOD 'Work around lazy mtime update' ' > ls -ld . >/dev/null > ' > > diff --git a/t/test-lib.sh b/t/test-lib.sh > index 3c730a2..1fc5266 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -962,7 +962,7 @@ case $(uname -s) in > test_set_prereq GREP_STRIPS_CR > ;; > *FreeBSD*) > - test_set_prereq FREEBSD > + test_set_prereq LAZYMOD > test_set_prereq POSIXPERM > test_set_prereq BSLASHPSPEC > test_set_prereq EXECKEEPSPID > -- 8< -- > > -- > Duy -- 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] 4/4] rebase: avoid computing unnecessary patch IDs
Johannes Schindelinwrites: >> Perhaps hashmap API needs fixing in the longer term not to call this >> type hashmap_cmp_fn; instead it should lose cmp and say something >> like hashmap_eq_fn or something. > > Maybe. > > But to make sure: you do not expect Kevin to do that in the context of > this here patch series, right? I think I already answered this in the message you are responding to. > Do you want a note in the commit message about this "abuse" of a negative > return value, or a code comment? I do not think negative (or non-zero) return is an "abuse" at all. It is misleading in the context of the function whose name has "cmp" in it, but that is not the fault of this function, rather, the breakage is more in the API that calls a function that wants to know only equality a "cmp". A in-code comment before the function name may be appropriate: /* * hashmap API calls hashmap_cmp_fn, but it only wants * "does the key match the entry?" with 0 (matches) and * non-zero (does not match). */ static int patch_id_match(const struct patch_id *ent, const struct patch_id *key, const void *keydata) { ... -- 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] 4/4] rebase: avoid computing unnecessary patch IDs
Jakub Narębskiwrites: > The problem is that one expects hashmap_cmp_fn() to return ==0 on equality, > while one would expect for hashmap_eq_fn() to return true (==1) on equality. > So we would have to rewrite all calling sites. Yes, and I do not think it is a "problem". There only are about a dozen callsites of hashmap_init(). > It would be nice to have a comment about how hashmap uses cmpfn in > hashmap.h. That is the absolute minimum but I think that is already done in the Documentation/technical/api-hashmap.txt. > Though... currently our hashmap implementation uses linked > list (separate chaining) for handling hash collisions (for collision > resolution). More sophisticated data structures, such as balanced search > trees,... But that requires total ordering of the elements registered in a hashmap. So far, because cmp_fn that was misnamed was only about equality, you can safely use a hashmap to store things that do not have inherent order among them. Besides, if your hashmap has to optimize the hash collision resolution part with complex strucure, your hash function is bad or your hash bucket growing strategy is suboptimal, or both, which is the first thing you should look at, not the "now how would we find it in the bucket with too many things?" -- 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] 1/4] patch-ids: stop using a hand-rolled hashmap implementation
Johannes Schindelinwrites: > Hi Junio, > > On Mon, 1 Aug 2016, Junio C Hamano wrote: > >> Johannes Schindelin writes: >> >> > It would be a serious bug if hashmap_entry_init() played games with >> > references, given its signature (that this function does not have any >> > access to the hashmap structure, only to the entry itself): >> > >> >void hashmap_entry_init(void *entry, unsigned int hash) >> >> I do not think we are on the same page. The "reference to other >> resource" I wondered was inside the hashmap_entry structure, IOW, >> "the entry itself". > > Oh, I see now. > >> Which is declared to be opaque to the API users, > > Actually, not really. We cannot do that in C: we need to define the struct > in hashmap.h so that its size is known to the users. What I meant was what Documentation/technical/api-hashmap.txt said. I know that we cannot embed a true "opaque" structure in C just as you do. > That is the reason, I guess, why we have the documentation in > Documentation/technical/api-hashmap.txt: it would have to talk about your > hypothetical hashmap_entry_clear() (which would better be named > *_release() BTW, unless I misunderstood what you want a hypothetical > future version of that function to do). I think there is no misunderstanding on your part. I am following the conclusion (as I recall) of a discussion on the list not in so distant past about X_free(x) that frees the resource an instance of "struct X" holds and also the instance itself, and X_clear(x) that only frees the resources without freeing the instance (the latter of which allows x to be on stack, you do X_init() and conclude with X_clear()). The name X_clear() is more in line with existing API functions like string_list_clear(), argv_array_clear(). An oddball is strbuf_release(), which I think made you make the above comment. >> I have a slight preference to avoid the lazy "void *", but that is >> an unrelated tangent. > > Oh, we are already safely in Unrelated Tangent Land for a while, I would > think. Nothing of what we are discussing in this thread has anything to do > with Kevin's patch series,... Oh, no question about that. Go back to my review, to which your message I am responding to is a reply to. What you wrote are all about things after "This is a tangent, but this made me wonder if it is safe to simply free(3) the result...", which pointed out rough points in the hashmap API from the API user's point of view and suggested a few possible improvement opportunities. >> I am saying that an uneven over-enginnering is bad. > > Hmm. I guess that the _init() function could be replaced by an _INIT macro > a la STRBUF_INIT. Not sure it is really worth the effort, though. I do not think so, as X_init() and X_INIT() from the point of view of the API user would not make much difference; as long as the documentation does not say it is safe to make it go out of scope without "deinitialize" it, the reader will be left wondering. -- 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
get_maintainer.pl and .mailmap entries with more than 2 addresses
Hello Florian. There is at least an oddity with get_maintainer handling of a .mailmap entry form. For instance: Mauro's .mailmap entry is: Mauro Carvalho ChehabIs this a valid form? get_maintainer output for Mauro is: $ ./scripts/get_maintainer.pl drivers/media/ -f Mauro Carvalho Chehab (maintainer:MEDIA INPUT INFRASTRUCTURE (V4L/DVB)) I believe the Mauro's and Shuah's .mailmap entries are improper and should be changed, but I'm not completely aware of git .mailmap handling and the documentation seems weakly specified. https://git-scm.com/docs/git-check-mailmap Maybe get_maintainer.pl needs a change or perhaps this patch is appropriate. --- .mailmap | 12 ++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/.mailmap b/.mailmap index c0d5704..c7f92ca 100644 --- a/.mailmap +++ b/.mailmap @@ -96,7 +96,12 @@ Linus Lüssing Linus Lüssing Mark Brown Matthieu CASTET -Mauro Carvalho Chehab +Mauro Carvalho Chehab +Mauro Carvalho Chehab +Mauro Carvalho Chehab +Mauro Carvalho Chehab +Mauro Carvalho Chehab +Mauro Carvalho Chehab Matt Ranostay Matthew Ranostay Matt Ranostay Mayuresh Janorkar @@ -132,7 +137,10 @@ Santosh Shilimkar Sascha Hauer S.Çağlar Onur Shiraz Hashim -Shuah Khan +Shuah Khan +Shuah Khan +Shuah Khan +Shuah Khan Simon Kelley Stéphane Witzmann Stephen Hemminger -- 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 7/8] status: update git-status.txt for --porcelain=v2
On 08/02/2016 11:19 AM, Jakub Narębski wrote: W dniu 01.08.2016 o 17:39, Jeff Hostetler pisze: On 07/30/2016 01:22 PM, Jakub Narębski wrote: W dniu 26.07.2016 o 23:11, Jeff Hostetler pisze: This is a nice change, available because of lack of backward compatibility with v1. The porcelain v2 format branch-related information could be enhanced without risk of breaking parsers, or having new information put at the end even if it does not fit there (like in previous iteration). One thing that can serve as goal for the series is using the question: would it make __git_ps1() from git-prompt.sh be able to render fully decorated prompt with all bells and whistles, and with all combinations of options. Thus beside upstream in the future we might want SVN upstream, and/or pushed-to remote branch (and remote push upstream repository), etc. But that's for the future (and it is possible for the future)... Yes, I was hoping to be able to simplify and/or speed up __git_ps1() with this data. "Namespacing" the branch data here. And then later add the state data (in a merge, in a rebase, and etc.) in a series of "# state.*" headers. And so on, until we get everything that __git_ps1() needs. However, to really make that work, we might want to add a --no-scan (or minimial scan) option, to just return the header data, since __git_ps1() doesn't care about the list of changes. But __git_ps1() with GIT_PS1_SHOWDIRTYSTATE would want to know if there are unstaged and staged changes, and with GIT_PS1_SHOWUNTRACKEDFILES it would want to know if there are untracked (unknown) files, isn't it? And GIT_PS1_SHOWSTASHSTATE would want to know if there is something stashed, but I guess it is outside the remit of git-status... Also, __git_ps1() uses colors from git-status, so if it is switched to use --porcelain=v2, then there should be an option to turn the color on, isn't it? All of these are good questions. But __git_ps1() is outside my scope right now. All I was saying above is that by making the branch details available here and, later, extending V2 output by adding other such headers to the output, we could try to remove much of the business logic in __git_ps1() and hopefully reduce it to just formatting and coloring the prompt using the computed result from status. But I've only skimmed __git_ps1() (and it's a little dense), so I'd have to study it more before I could answer all of your questions. Thanks, Jeff -- 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: Client exit whilst running pre-receive hook : commit accepted but no post-receive hook ran
On 2016-07-25 6:22 PM, Jeff King wrote: On Mon, Jul 25, 2016 at 12:34:04PM +0200, Jan Smets wrote: I have always assumed the post-receive hook to be executed whenever a commit is "accepted" by the (gitolite) server. That does not seem to be true any more. Generally, yeah, I would expect that to be the case, too. Since 9658846 is appears that, when a client bails out, the pre-receive hook continues to run and the commit is written to the repository, but no post-receive hook is executed. No signal of any kind is received in the hook, not even a sig pipe when the post- hook is writing to stdout whilst the client has disconnected. I see. The problem is that cmd_receive_pack() does this: execute_commands(commands, unpack_status, ); if (pack_lockfile) unlink_or_warn(pack_lockfile); if (report_status) report(commands, unpack_status); run_receive_hook(commands, "post-receive", 1); run_update_post_hook(commands); It reports the status to the client, and _then_ runs the post-receive hook. But if that reporting fails (either because of an error, or if we just get SIGPIPE because the client hung up), then we don't actually run the hooks. Leaving 9658846 out of it entirely, it is always going to be racy whether we notice that the client hung up during the pre-receive step. E.g.: - your pre-receive might not write any output, so the muxer has nothing to write to the other side, and we never notice that the connection closed until we write the status out in report() - if NO_PTHREADS is set, the sideband muxer runs in a sub-process, not a sub-thread. And thus we don't know of a hangup except by checking the result of finish_async(), which we never do. - the client could hang up just _after_ we've written the pre-receive output, but before report() is called, so there's nothing to notice until we're in report() So I think 9658846 just made that race a bit longer, because it means that a write error in the sideband muxer during the pre-receive hook means we return an error via finish_async() rather than unceremoniously calling exit() from a sub-thread. So we have a longer period during which we might actually finish off execute_commands() but not make it out of report(). And the real solution is to make cmd_receive_pack() more robust, and try harder to run the hooks even when the client hangs up or we have some other reporting error (because getting data back to the user is only one use of post-receive hooks; they are also used to queue jobs or do maintenance). But that's a bit tricky, as it requires report() to ignore SIGPIPE, and to stop using write_or_die() or any other functions that can exit (some of which happen at a lower level). Plus if a client does hangup, we don't want our hook to die with SIGPIPE either, so we'd want to proxy the data into /dev/null. -Peff Sounds tricky. I do think it's important, almost a 'data integrity' issue, that IF a commit is received, THEN the post-receive hook must be run. Too much mission-critical stuff is based on post-receive hooks. The alternatives, as I see them --either document that the post-receive hook cannot be fully trusted and that all such uses must change to asynchronous polling, or otherwise just say that nobody should hit Ctrl-C during a push (not even reflexively when their lizard-brain says "Woops, no!") and hope that network issues don't cause the same thing-- are simply not realistic. Stephen -- 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] t7063: work around FreeBSD's lazy mtime update feature
On Mon, Aug 01, 2016 at 02:04:44PM -0700, Junio C Hamano wrote: > Duy Nguyenwrites: > > > On Mon, Aug 1, 2016 at 3:37 AM, Torstem Bögershausen wrote: > >> the term FREEBSD may be too generic to point out a single feature > >> in an OS distributution. > >> Following your investigations, it may even be possible that > >> other systems adapt this "feature"? > >> > >> How about > >> LAZY_DIR_MTIME_UPDATE > >> (or similar) > > > > This feature was added in 1998, so yes there's a chance it has spread > > to a few fbsd derivatives (not sure if openbsd or netbsd is close > > enough and they ever exchange changes). But I'd rather wait for the > > second OS to expose the same feature before renaming it. > > I think a name based on the observed behaviour ("feature") would be > more appropriate because I'd be more worried about us finding other > glitches we see (initially) only on FBSD. People who need to adjust > tests that use the same FBSD prereq would have to wonder which > prereq-skip is due to which glitch. OK how about this squashed in? The name was taken from fbsd definition IN_LAZYMOD. Off topic. Since I found this macro defined twice, in ext2 and ufs, but not in zfs (found its source!), I assume zfs does not have this particular feature (but I didn't test it). Untracked cache may be more effecient there. -- 8< -- diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh index 08fc586..8bb048a 100755 --- a/t/t7063-status-untracked-cache.sh +++ b/t/t7063-status-untracked-cache.sh @@ -419,7 +419,7 @@ test_expect_success 'create/modify files, some of which are gitignored' ' rm base ' -test_expect_success FREEBSD 'Work around lazy mtime update' ' +test_expect_success LAZYMOD 'Work around lazy mtime update' ' ls -ld . >/dev/null ' diff --git a/t/test-lib.sh b/t/test-lib.sh index 3c730a2..1fc5266 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -962,7 +962,7 @@ case $(uname -s) in test_set_prereq GREP_STRIPS_CR ;; *FreeBSD*) - test_set_prereq FREEBSD + test_set_prereq LAZYMOD test_set_prereq POSIXPERM test_set_prereq BSLASHPSPEC test_set_prereq EXECKEEPSPID -- 8< -- -- Duy -- 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 7/8] status: update git-status.txt for --porcelain=v2
W dniu 01.08.2016 o 17:39, Jeff Hostetler pisze: > On 07/30/2016 01:22 PM, Jakub Narębski wrote: >> W dniu 26.07.2016 o 23:11, Jeff Hostetler pisze: >> >> This is a nice change, available because of lack of backward >> compatibility with v1. The porcelain v2 format branch-related >> information could be enhanced without risk of breaking parsers, >> or having new information put at the end even if it does not fit >> there (like in previous iteration). >> >> One thing that can serve as goal for the series is using the >> question: would it make __git_ps1() from git-prompt.sh be able >> to render fully decorated prompt with all bells and whistles, >> and with all combinations of options. Thus beside upstream >> in the future we might want SVN upstream, and/or pushed-to >> remote branch (and remote push upstream repository), etc. >> >> But that's for the future (and it is possible for the future)... >> > > Yes, I was hoping to be able to simplify and/or speed up > __git_ps1() with this data. "Namespacing" the branch data > here. And then later add the state data (in a merge, > in a rebase, and etc.) in a series of "# state.*" headers. > And so on, until we get everything that __git_ps1() needs. > However, to really make that work, we might want to add > a --no-scan (or minimial scan) option, to just return the > header data, since __git_ps1() doesn't care about the list > of changes. But __git_ps1() with GIT_PS1_SHOWDIRTYSTATE would want to know if there are unstaged and staged changes, and with GIT_PS1_SHOWUNTRACKEDFILES it would want to know if there are untracked (unknown) files, isn't it? And GIT_PS1_SHOWSTASHSTATE would want to know if there is something stashed, but I guess it is outside the remit of git-status... Also, __git_ps1() uses colors from git-status, so if it is switched to use --porcelain=v2, then there should be an option to turn the color on, isn't it? -- Jakub Narębski -- 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: Un-paged commit messages in git filter-branch's commit-filter?
On Mon, 1 Aug 2016 17:36:31 -0400 Jeff Kingwrote: > On Sun, Jul 31, 2016 at 06:39:35PM +0200, Stefan Tauner wrote: > > > > There are some output formats that will wrap lines, but by default, > > > filter-branch should not be using them (and I could not reproduce the > > > issue in a simple test). Can you show us what your commit-filter looks > > > like? > > > > Thanks for your answer. I have tried to reproduce it in other (newly > > created) repositories but failed. However, it seems to relate to some > > kind of persistent paging setting, is that possible? > > git config -l does not show anything suspicious. > > > > The following commands produce paged output: > > git show hash > > git show --pretty=%B > > git log hash^..hash > > Commit message in gitk > > > > > > These do NOT produce paged output: > > git patch hash^..hash > > Commit message in gitg 0.2.7 > > What is "git patch"? An alias for "format-patch?". Yes, sorry. And this is the most amazing thing about this behavior... what's so different between format-patch and log or show --pretty=%B. Shouldn't these match 100%? > > > This is the script I tried to use to reproduce the problem: > > > > #!/bin/bash > > export LC_ALL=C > > input=$(cat) > > echo "=== > > $input > > ===" >> /tmp/paging_bug.txt > > git commit-tree "$@" -m "$input" > > Can you be more specific about the input you're feeding to git and the > output you're seeing? > > For instance, if I do: > > git init > echo content >file > git add file > git commit -m "$(perl -e 'print join(" ", 1..100)')" > > I get a commit message with one long unwrapped line, which I can view > via git-log, etc. That's approximately what I did in my tests as well. And like you, when I do this in a fresh repository, it works like that.. > Now if I try to run filter-branch on that: > > git filter-branch --commit-filter ' > input=$(cat) > { > echo "" > echo $input > echo "" > } >>/tmp/paging_bug.txt > git commit-tree "$@" -m "$input" > ' > > then the commit remains unchanged, and paging_bug shows one long line. as well as filter-branch. That's what I meant when I wrote I cannot reproduce it with a new repository (to create a MWE). I wrote the first mail under the presumption that filter-branch is somehow involved but apparently it is not the only git command and receives the mangled input already as the commands stated in the last email show. > What am I missing? > > (I wondered at first if the extra "cat" and "-m" could be messing up > whitespace for you, but it should not, as the quoting around "$input" > should preserve things like newlines. And anyway, the bug in that case > would be the _opposite_; I'd expect it to stuff everything onto a single > line rather than breaking lines). The commit messages I try to process are nothing special really... just very long and not subject-like (because SVN and not giving too much thought to them sometimes). The only special thing I can think of is that they have been processed by git-svn earlier. -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner -- 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 v4 4/8] status: per-file data collection for --porcelain=v2
From: Jeff HostetlerThe output of `git status --porcelain` leaves out many details about the current status that clients might like to have. This can force them to be less efficient as they may need to launch secondary commands (and try to match the logic within git) to accumulate this extra information. For example, a GUI IDE might want the file mode to display the correct icon for a changed item (without having to stat it afterwards). Signed-off-by: Jeff Hostetler Signed-off-by: Jeff Hostetler --- builtin/commit.c | 3 +++ wt-status.c | 63 +++- wt-status.h | 4 3 files changed, 69 insertions(+), 1 deletion(-) diff --git a/builtin/commit.c b/builtin/commit.c index c3ae2c3..93ce28c 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -153,6 +153,8 @@ static int opt_parse_porcelain(const struct option *opt, const char *arg, int un *value = STATUS_FORMAT_PORCELAIN; else if (!strcmp(arg, "v1") || !strcmp(arg,"1")) *value = STATUS_FORMAT_PORCELAIN; + else if (!strcmp(arg, "v2") || !strcmp(arg, "2")) + *value = STATUS_FORMAT_PORCELAIN_V2; else die("unsupported porcelain version '%s'", arg); @@ -1104,6 +1106,7 @@ static struct status_deferred_config { static void finalize_deferred_config(struct wt_status *s) { int use_deferred_config = (status_format != STATUS_FORMAT_PORCELAIN && + status_format != STATUS_FORMAT_PORCELAIN_V2 && !s->null_termination); if (s->null_termination) { diff --git a/wt-status.c b/wt-status.c index a9031e4..15d3349 100644 --- a/wt-status.c +++ b/wt-status.c @@ -434,6 +434,31 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q, if (S_ISGITLINK(p->two->mode)) d->new_submodule_commits = !!oidcmp(>one->oid, >two->oid); + + switch (p->status) { + case DIFF_STATUS_ADDED: + die("BUG: worktree status add???"); + break; + + case DIFF_STATUS_DELETED: + d->mode_index = p->one->mode; + oidcpy(>oid_index, >one->oid); + /* mode_worktree is zero for a delete. */ + break; + + case DIFF_STATUS_MODIFIED: + case DIFF_STATUS_TYPE_CHANGED: + case DIFF_STATUS_UNMERGED: + d->mode_index = p->one->mode; + d->mode_worktree = p->two->mode; + oidcpy(>oid_index, >one->oid); + break; + + case DIFF_STATUS_UNKNOWN: + die("BUG: worktree status unknown???"); + break; + } + } } @@ -479,12 +504,36 @@ static void wt_status_collect_updated_cb(struct diff_queue_struct *q, if (!d->index_status) d->index_status = p->status; switch (p->status) { + case DIFF_STATUS_ADDED: + /* Leave {mode,oid}_head zero for an add. */ + d->mode_index = p->two->mode; + oidcpy(>oid_index, >two->oid); + break; + case DIFF_STATUS_DELETED: + d->mode_head = p->one->mode; + oidcpy(>oid_head, >one->oid); + /* Leave {mode,oid}_index zero for a delete. */ + break; + case DIFF_STATUS_COPIED: case DIFF_STATUS_RENAMED: d->head_path = xstrdup(p->one->path); + d->score = p->score * 100 / MAX_SCORE; + /* fallthru */ + case DIFF_STATUS_MODIFIED: + case DIFF_STATUS_TYPE_CHANGED: + d->mode_head = p->one->mode; + d->mode_index = p->two->mode; + oidcpy(>oid_head, >one->oid); + oidcpy(>oid_index, >two->oid); break; case DIFF_STATUS_UNMERGED: d->stagemask = unmerged_mask(p->two->path); + /* +* Don't bother setting {mode,oid}_{head,index} since the print +* code will output the stage values directly and not use the +* values in these fields. +*/ break; } } @@ -565,9 +614,18 @@ static void wt_status_collect_changes_initial(struct wt_status *s) if (ce_stage(ce)) { d->index_status =
[PATCH v4 3/8] status: support --porcelain[=]
From: Jeff HostetlerUpdate --porcelain argument to take optional version parameter to allow multiple porcelain formats to be supported in the future. The token "v1" is the default value and indicates the traditional porcelain format. (The token "1" is an alias for that.) Signed-off-by: Jeff Hostetler Signed-off-by: Jeff Hostetler --- Documentation/git-status.txt | 7 +-- builtin/commit.c | 21 ++--- t/t7060-wtstatus.sh | 21 + 3 files changed, 44 insertions(+), 5 deletions(-) diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt index e1e8f57..6b1454b 100644 --- a/Documentation/git-status.txt +++ b/Documentation/git-status.txt @@ -32,11 +32,14 @@ OPTIONS --branch:: Show the branch and tracking info even in short-format. ---porcelain:: +--porcelain[=]:: Give the output in an easy-to-parse format for scripts. This is similar to the short output, but will remain stable across Git versions and regardless of user configuration. See below for details. ++ +The version parameter is used to specify the format version. +This is optional and defaults to the original version 'v1' format. --long:: Give the output in the long-format. This is the default. @@ -96,7 +99,7 @@ configuration variable documented in linkgit:git-config[1]. -z:: Terminate entries with NUL, instead of LF. This implies - the `--porcelain` output format if no other format is given. + the `--porcelain=v1` output format if no other format is given. --column[=]:: --no-column:: diff --git a/builtin/commit.c b/builtin/commit.c index a792deb..c3ae2c3 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -144,6 +144,21 @@ static struct strbuf message = STRBUF_INIT; static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED; +static int opt_parse_porcelain(const struct option *opt, const char *arg, int unset) +{ + enum wt_status_format *value = (enum wt_status_format *)opt->value; + if (unset) + *value = STATUS_FORMAT_NONE; + else if (!arg) + *value = STATUS_FORMAT_PORCELAIN; + else if (!strcmp(arg, "v1") || !strcmp(arg,"1")) + *value = STATUS_FORMAT_PORCELAIN; + else + die("unsupported porcelain version '%s'", arg); + + return 0; +} + static int opt_parse_m(const struct option *opt, const char *arg, int unset) { struct strbuf *buf = opt->value; @@ -1316,9 +1331,9 @@ int cmd_status(int argc, const char **argv, const char *prefix) N_("show status concisely"), STATUS_FORMAT_SHORT), OPT_BOOL('b', "branch", _branch, N_("show branch information")), - OPT_SET_INT(0, "porcelain", _format, - N_("machine-readable output"), - STATUS_FORMAT_PORCELAIN), + { OPTION_CALLBACK, 0, "porcelain", _format, + N_("version"), N_("machine-readable output"), + PARSE_OPT_OPTARG, opt_parse_porcelain }, OPT_SET_INT(0, "long", _format, N_("show status in long format (default)"), STATUS_FORMAT_LONG), diff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh index 44bf1d8..00e0ceb 100755 --- a/t/t7060-wtstatus.sh +++ b/t/t7060-wtstatus.sh @@ -228,4 +228,25 @@ test_expect_success 'status --branch with detached HEAD' ' test_i18ncmp expected actual ' +## Duplicate the above test and verify --porcelain=v1 arg parsing. +test_expect_success 'status --porcelain=v1 --branch with detached HEAD' ' + git reset --hard && + git checkout master^0 && + git status --branch --porcelain=v1 >actual && + cat >expected <<-EOF && + ## HEAD (no branch) + ?? .gitconfig + ?? actual + ?? expect + ?? expected + ?? mdconflict/ + EOF + test_i18ncmp expected actual +' + +## Verify parser error on invalid --porcelain argument. +test_expect_success 'status --porcelain=bogus' ' + test_must_fail git status --porcelain=bogus +' + test_done -- 2.8.0.rc4.17.gac42084.dirty -- 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 v4 1/8] status: rename long-format print routines
From: Jeff HostetlerRenamed the various wt_status_print*() routines to be wt_longstatus_print*() to make it clear that these routines are only concerned with the normal/long status output. This will hopefully reduce confusion as other status formats are added in the future. Signed-off-by: Jeff Hostetler Signed-off-by: Jeff Hostetler --- builtin/commit.c | 4 +- wt-status.c | 110 +++ wt-status.h | 2 +- 3 files changed, 58 insertions(+), 58 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 1f6dbcd..b80273b 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -515,7 +515,7 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int break; case STATUS_FORMAT_NONE: case STATUS_FORMAT_LONG: - wt_status_print(s); + wt_longstatus_print(s); break; } @@ -1403,7 +1403,7 @@ int cmd_status(int argc, const char **argv, const char *prefix) case STATUS_FORMAT_LONG: s.verbose = verbose; s.ignore_submodule_arg = ignore_submodule_arg; - wt_status_print(); + wt_longstatus_print(); break; } return 0; diff --git a/wt-status.c b/wt-status.c index de62ab2..b9a58fd 100644 --- a/wt-status.c +++ b/wt-status.c @@ -139,7 +139,7 @@ void wt_status_prepare(struct wt_status *s) s->display_comment_prefix = 0; } -static void wt_status_print_unmerged_header(struct wt_status *s) +static void wt_longstatus_print_unmerged_header(struct wt_status *s) { int i; int del_mod_conflict = 0; @@ -191,7 +191,7 @@ static void wt_status_print_unmerged_header(struct wt_status *s) status_printf_ln(s, c, "%s", ""); } -static void wt_status_print_cached_header(struct wt_status *s) +static void wt_longstatus_print_cached_header(struct wt_status *s) { const char *c = color(WT_STATUS_HEADER, s); @@ -207,9 +207,9 @@ static void wt_status_print_cached_header(struct wt_status *s) status_printf_ln(s, c, "%s", ""); } -static void wt_status_print_dirty_header(struct wt_status *s, -int has_deleted, -int has_dirty_submodules) +static void wt_longstatus_print_dirty_header(struct wt_status *s, +int has_deleted, +int has_dirty_submodules) { const char *c = color(WT_STATUS_HEADER, s); @@ -226,9 +226,9 @@ static void wt_status_print_dirty_header(struct wt_status *s, status_printf_ln(s, c, "%s", ""); } -static void wt_status_print_other_header(struct wt_status *s, -const char *what, -const char *how) +static void wt_longstatus_print_other_header(struct wt_status *s, +const char *what, +const char *how) { const char *c = color(WT_STATUS_HEADER, s); status_printf_ln(s, c, "%s:", what); @@ -238,7 +238,7 @@ static void wt_status_print_other_header(struct wt_status *s, status_printf_ln(s, c, "%s", ""); } -static void wt_status_print_trailer(struct wt_status *s) +static void wt_longstatus_print_trailer(struct wt_status *s) { status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", ""); } @@ -304,8 +304,8 @@ static int maxwidth(const char *(*label)(int), int minval, int maxval) return result; } -static void wt_status_print_unmerged_data(struct wt_status *s, - struct string_list_item *it) +static void wt_longstatus_print_unmerged_data(struct wt_status *s, + struct string_list_item *it) { const char *c = color(WT_STATUS_UNMERGED, s); struct wt_status_change_data *d = it->util; @@ -331,9 +331,9 @@ static void wt_status_print_unmerged_data(struct wt_status *s, strbuf_release(); } -static void wt_status_print_change_data(struct wt_status *s, - int change_type, - struct string_list_item *it) +static void wt_longstatus_print_change_data(struct wt_status *s, + int change_type, + struct string_list_item *it) { struct wt_status_change_data *d = it->util; const char *c = color(change_type, s); @@ -378,7 +378,7 @@ static void wt_status_print_change_data(struct wt_status *s, status = d->worktree_status; break; default: - die("BUG: unhandled change_type %d in wt_status_print_change_data", + die("BUG: unhandled
[PATCH v4 5/8] status: print per-file porcelain v2 status data
From: Jeff HostetlerPrint per-file information in porcelain v2 format. Signed-off-by: Jeff Hostetler Signed-off-by: Jeff Hostetler --- wt-status.c | 283 +++- 1 file changed, 282 insertions(+), 1 deletion(-) diff --git a/wt-status.c b/wt-status.c index 15d3349..46061d4 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1813,6 +1813,287 @@ static void wt_porcelain_print(struct wt_status *s) wt_shortstatus_print(s); } +/* + * Convert various submodule status values into a + * fixed-length string of characters in the buffer provided. + */ +static void wt_porcelain_v2_submodule_state( + struct wt_status_change_data *d, + char sub[5]) +{ + if (S_ISGITLINK(d->mode_head) || + S_ISGITLINK(d->mode_index) || + S_ISGITLINK(d->mode_worktree)) { + sub[0] = 'S'; + sub[1] = d->new_submodule_commits ? 'C' : '.'; + sub[2] = (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED) ? 'M' : '.'; + sub[3] = (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ? 'U' : '.'; + } else { + sub[0] = 'N'; + sub[1] = '.'; + sub[2] = '.'; + sub[3] = '.'; + } + sub[4] = 0; +} + +/* + * Fix-up changed entries before we print them. + */ +static void wt_porcelain_v2_fix_up_changed( + struct string_list_item *it, + struct wt_status *s) +{ + struct wt_status_change_data *d = it->util; + + if (!d->index_status) { + /* +* This entry is unchanged in the index (relative to the head). +* Therefore, the collect_updated_cb was never called for this +* entry (during the head-vs-index scan) and so the head column +* fields were never set. +* +* We must have data for the index column (from the +* index-vs-worktree scan (otherwise, this entry should not be +* in the list of changes)). +* +* Copy index column fields to the head column, so that our +* output looks complete. +*/ + assert(d->mode_head == 0); + d->mode_head = d->mode_index; + oidcpy(>oid_head, >oid_index); + } + + if (!d->worktree_status) { + /* +* This entry is unchanged in the worktree (relative to the index). +* Therefore, the collect_changed_cb was never called for this entry +* (during the index-vs-worktree scan) and so the worktree column +* fields were never set. +* +* We must have data for the index column (from the head-vs-index +* scan). +* +* Copy the index column fields to the worktree column so that +* our output looks complete. +* +* Note that we only have a mode field in the worktree column +* because the scan code tries really hard to not have to compute it. +*/ + assert(d->mode_worktree == 0); + d->mode_worktree = d->mode_index; + } +} + +/* + * Print porcelain v2 info for tracked entries with changes. + */ +static void wt_porcelain_v2_print_changed_entry( + struct string_list_item *it, + struct wt_status *s) +{ + struct wt_status_change_data *d = it->util; + struct strbuf buf_current = STRBUF_INIT; + struct strbuf buf_src = STRBUF_INIT; + const char *path_current = NULL; + const char *path_src = NULL; + char key[3]; + char submodule_token[5]; + char sep_char, eol_char; + + wt_porcelain_v2_fix_up_changed(it, s); + wt_porcelain_v2_submodule_state(d, submodule_token); + + key[0] = d->index_status ? d->index_status : '.'; + key[1] = d->worktree_status ? d->worktree_status : '.'; + key[2] = 0; + + if (s->null_termination) { + /* +* In -z mode, we DO NOT C-Quote pathnames. Current path is ALWAYS first. +* A single NUL character separates them. +*/ + sep_char = '\0'; + eol_char = '\0'; + path_current = it->string; + path_src = d->head_path; + } else { + /* +* Path(s) are C-Quoted if necessary. Current path is ALWAYS first. +* The source path is only present when necessary. +* A single TAB separates them (because paths can contain spaces +* which are not escaped and C-Quoting does escape TAB characters). +*/ + sep_char = '\t'; + eol_char = '\n'; + path_current =
[PATCH v4 6/8] status: print branch info with --porcelain=v2 --branch
From: Jeff HostetlerExpand porcelain v2 output to include branch and tracking branch information. This includes the commit SHA, the branch, the upstream branch, and the ahead and behind counts. Signed-off-by: Jeff Hostetler Signed-off-by: Jeff Hostetler --- builtin/commit.c | 5 wt-status.c | 90 wt-status.h | 1 + 3 files changed, 96 insertions(+) diff --git a/builtin/commit.c b/builtin/commit.c index 93ce28c..b1fd2d1 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -510,6 +510,8 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int s->fp = fp; s->nowarn = nowarn; s->is_initial = get_sha1(s->reference, sha1) ? 1 : 0; + if (!s->is_initial) + hashcpy(s->sha1_commit, sha1); s->status_format = status_format; s->ignore_submodule_arg = ignore_submodule_arg; @@ -1378,6 +1380,9 @@ int cmd_status(int argc, const char **argv, const char *prefix) fd = hold_locked_index(_lock, 0); s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0; + if (!s.is_initial) + hashcpy(s.sha1_commit, sha1); + s.ignore_submodule_arg = ignore_submodule_arg; s.status_format = status_format; s.verbose = verbose; diff --git a/wt-status.c b/wt-status.c index 46061d4..592fbd2 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1814,6 +1814,92 @@ static void wt_porcelain_print(struct wt_status *s) } /* + * Print branch information for porcelain v2 output. These lines + * are printed when the '--branch' parameter is given. + * + *# branch.oid + *# branch.head + * [# branch.upstream + * [# branch.ab + -]] + * + * ::= the current commit hash or the the literal + * "(initial)" to indicate an initialized repo + * with no commits. + * + * ::= the current branch name or + * "(detached)" literal when detached head or + * "(unknown)" when something is wrong. + * + * ::= the upstream branch name, when set. + * + *::= integer ahead value, when upstream set + * and the commit is present (not gone). + * + * ::= integer behind value, when upstream set + * and commit is present. + * + * + * The end-of-line is defined by the -z flag. + * + * ::= NUL when -z, + * LF when NOT -z. + * + */ +static void wt_porcelain_v2_print_tracking(struct wt_status *s) +{ + struct branch *branch; + const char *base; + const char *branch_name; + struct wt_status_state state; + int ab_info, nr_ahead, nr_behind; + char eol = s->null_termination ? '\0' : '\n'; + + memset(, 0, sizeof(state)); + wt_status_get_state(, s->branch && !strcmp(s->branch, "HEAD")); + + fprintf(s->fp, "# branch.oid %s%c", + (s->is_initial ? "(initial)" : sha1_to_hex(s->sha1_commit)), + eol); + + if (!s->branch) + fprintf(s->fp, "# branch.head %s%c", "(unknown)", eol); + else { + if (!strcmp(s->branch, "HEAD")) { + fprintf(s->fp, "# branch.head %s%c", "(detached)", eol); + + if (state.rebase_in_progress || state.rebase_interactive_in_progress) + branch_name = state.onto; + else if (state.detached_from) + branch_name = state.detached_from; + else + branch_name = ""; + } else { + branch_name = NULL; + skip_prefix(s->branch, "refs/heads/", _name); + + fprintf(s->fp, "# branch.head %s%c", branch_name, eol); + } + + /* Lookup stats on the upstream tracking branch, if set. */ + branch = branch_get(branch_name); + base = NULL; + ab_info = (stat_tracking_info(branch, _ahead, _behind, ) == 0); + if (base) { + base = shorten_unambiguous_ref(base, 0); + fprintf(s->fp, "# branch.upstream %s%c", base, eol); + free((char *)base); + + if (ab_info) + fprintf(s->fp, "# branch.ab +%d -%d%c", nr_ahead, nr_behind, eol); + } + } + + free(state.branch); + free(state.onto); + free(state.detached_from); +} + +/* * Convert various submodule status values into a * fixed-length string of characters in the buffer provided. */ @@ -2057,6 +2143,7 @@ static void wt_porcelain_v2_print_other( /* * Print porcelain V2 status. * + * [] * []* * []* * []* @@ -2069,6 +2156,9 @@ void
[PATCH v4 7/8] git-status.txt: describe --porcelain=v2 format
From: Jeff HostetlerUpdate status manpage to include information about porcelain V2 format. Signed-off-by: Jeff Hostetler Signed-off-by: Jeff Hostetler --- Documentation/git-status.txt | 123 +-- 1 file changed, 119 insertions(+), 4 deletions(-) diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt index 6b1454b..76e7c92 100644 --- a/Documentation/git-status.txt +++ b/Documentation/git-status.txt @@ -183,12 +183,12 @@ in which case `XY` are `!!`. If -b is used the short-format status is preceded by a line -## branchname tracking info +## branchname tracking info -Porcelain Format - +Porcelain Format Version 1 +~~ -The porcelain format is similar to the short format, but is guaranteed +Version 1 porcelain format is similar to the short format, but is guaranteed not to change in a backwards-incompatible way between Git versions or based on user configuration. This makes it ideal for parsing by scripts. The description of the short format above also describes the porcelain @@ -210,6 +210,121 @@ field from the first filename). Third, filenames containing special characters are not specially formatted; no quoting or backslash-escaping is performed. +Porcelain Format Version 2 +~~ + +Version 2 format adds more detailed information about the state of +the worktree and changed items. Version 2 also defines an extensible +set of easy to parse optional headers. + +Header lines start with "#" and are added in response to specific +command line arguments. Parsers should ignore headers they +don't recognize. + +### Branch Headers + +If `--branch` is given, a series of header lines are printed with +information about the current branch. + +Line Notes + +# branch.oid | (initial)Current commit. +# branch.head | (detached) Current branch. +# branch.upstream If upstream is set. +# branch.ab + - If upstream is set and + the commit is present. + + +### Changed Tracked Entries + +Following the headers, a series of lines are printed for tracked +entries. One of three different line formats may be used to describe +an entry depending on the type of change. Tracked entries are printed +in an undefined order; parsers should allow for a mixture of the 3 +line types in any order. + +Ordinary changed entries have the following format: + +1 + +Renamed or copied entries have the following format: + +2 + +Field Meaning + +A 2 character field containing the staged and +unstaged XY values described in the short format, +with unchanged indicated by a "." rather than +a space. + A 4 character field describing the submodule state. +"N..." when the entry is not a submodule. +"S" when the entry is a submodule. + is "C" if the commit changed; otherwise ".". + is "M" if it has tracked changes; otherwise ".". + is "U" if there are untracked changes; otherwise ".". +The 6 character octal file mode in the HEAD. +The octal file mode in the index. +The octal file mode in the worktree. +The SHA1 value in the HEAD. +The SHA1 value in the index. + The rename or copied percentage score. For example "R100" +or "C75". + The current pathname. + When the `-z` option is used, the 2 pathnames are separated +with a NUL (ASCII 0x00) byte; otherwise, a tab (ASCII 0x09) +byte separates them. + The original path. This is only present when the entry +has been renamed or copied. + + +Unmerged entries have the following format; the first character is +a "u" to distinguish from ordinary changed entries. + +u + +Field Meaning + +A 2 character field describing the conflict type +as described in the short format. + A 4 character field describing the submodule state +as described above. +The octal file mode for stage 1. +The octal file mode for stage 2. +The octal file mode for stage 3. +The octal file mode in the worktree. +The SHA1 value for stage 1. +The SHA1 value for stage 2. +The SHA1 value for stage 3. +
[PATCH v4 0/8] status: V2 porcelain status
This patch series adds porcelain V2 format to status. This provides detailed information about file changes and about the current branch. The new output is accessed via: git status --porcelain=v2 [--branch] Relative to the V3 patch series, in this V4 patch series I've updated Documentation/git-status.txt for clarity. Jeff Hostetler (8): status: rename long-format print routines status: cleanup API to wt_status_print status: support --porcelain[=] status: per-file data collection for --porcelain=v2 status: print per-file porcelain v2 status data status: print branch info with --porcelain=v2 --branch git-status.txt: describe --porcelain=v2 format status: tests for --porcelain=v2 Documentation/git-status.txt | 130 +- builtin/commit.c | 78 +++--- t/t7060-wtstatus.sh | 21 ++ t/t7064-wtstatus-pv2.sh | 585 +++ wt-status.c | 567 - wt-status.h | 19 +- 6 files changed, 1289 insertions(+), 111 deletions(-) create mode 100755 t/t7064-wtstatus-pv2.sh -- 2.8.0.rc4.17.gac42084.dirty -- 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 v4 2/8] status: cleanup API to wt_status_print
From: Jeff HostetlerRefactor the API between builtin/commit.c and wt-status.[ch]. Hide details of the various wt_*status_print() routines inside wt-status.c behind a single (new) wt_status_print() routine and eliminate the switch statements from builtin/commit.c This will allow us to more easily add new status formats and isolate that within wt-status.c Signed-off-by: Jeff Hostetler Signed-off-by: Jeff Hostetler --- builtin/commit.c | 51 +-- wt-status.c | 25 ++--- wt-status.h | 16 3 files changed, 43 insertions(+), 49 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index b80273b..a792deb 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -142,14 +142,7 @@ static int show_ignored_in_status, have_option_m; static const char *only_include_assumed; static struct strbuf message = STRBUF_INIT; -static enum status_format { - STATUS_FORMAT_NONE = 0, - STATUS_FORMAT_LONG, - STATUS_FORMAT_SHORT, - STATUS_FORMAT_PORCELAIN, - - STATUS_FORMAT_UNSPECIFIED -} status_format = STATUS_FORMAT_UNSPECIFIED; +static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED; static int opt_parse_m(const struct option *opt, const char *arg, int unset) { @@ -500,24 +493,11 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int s->fp = fp; s->nowarn = nowarn; s->is_initial = get_sha1(s->reference, sha1) ? 1 : 0; + s->status_format = status_format; + s->ignore_submodule_arg = ignore_submodule_arg; wt_status_collect(s); - - switch (status_format) { - case STATUS_FORMAT_SHORT: - wt_shortstatus_print(s); - break; - case STATUS_FORMAT_PORCELAIN: - wt_porcelain_print(s); - break; - case STATUS_FORMAT_UNSPECIFIED: - die("BUG: finalize_deferred_config() should have been called"); - break; - case STATUS_FORMAT_NONE: - case STATUS_FORMAT_LONG: - wt_longstatus_print(s); - break; - } + wt_status_print(s); return s->commitable; } @@ -1099,7 +1079,7 @@ static const char *read_commit_message(const char *name) * is not in effect here. */ static struct status_deferred_config { - enum status_format status_format; + enum wt_status_format status_format; int show_branch; } status_deferred_config = { STATUS_FORMAT_UNSPECIFIED, @@ -1381,6 +1361,9 @@ int cmd_status(int argc, const char **argv, const char *prefix) s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0; s.ignore_submodule_arg = ignore_submodule_arg; + s.status_format = status_format; + s.verbose = verbose; + wt_status_collect(); if (0 <= fd) @@ -1389,23 +1372,7 @@ int cmd_status(int argc, const char **argv, const char *prefix) if (s.relative_paths) s.prefix = prefix; - switch (status_format) { - case STATUS_FORMAT_SHORT: - wt_shortstatus_print(); - break; - case STATUS_FORMAT_PORCELAIN: - wt_porcelain_print(); - break; - case STATUS_FORMAT_UNSPECIFIED: - die("BUG: finalize_deferred_config() should have been called"); - break; - case STATUS_FORMAT_NONE: - case STATUS_FORMAT_LONG: - s.verbose = verbose; - s.ignore_submodule_arg = ignore_submodule_arg; - wt_longstatus_print(); - break; - } + wt_status_print(); return 0; } diff --git a/wt-status.c b/wt-status.c index b9a58fd..a9031e4 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1447,7 +1447,7 @@ static void wt_longstatus_print_state(struct wt_status *s, show_bisect_in_progress(s, state, state_color); } -void wt_longstatus_print(struct wt_status *s) +static void wt_longstatus_print(struct wt_status *s) { const char *branch_color = color(WT_STATUS_ONBRANCH, s); const char *branch_status_color = color(WT_STATUS_HEADER, s); @@ -1714,7 +1714,7 @@ static void wt_shortstatus_print_tracking(struct wt_status *s) fputc(s->null_termination ? '\0' : '\n', s->fp); } -void wt_shortstatus_print(struct wt_status *s) +static void wt_shortstatus_print(struct wt_status *s) { int i; @@ -1746,7 +1746,7 @@ void wt_shortstatus_print(struct wt_status *s) } } -void wt_porcelain_print(struct wt_status *s) +static void wt_porcelain_print(struct wt_status *s) { s->use_color = 0; s->relative_paths = 0; @@ -1754,3 +1754,22 @@ void wt_porcelain_print(struct wt_status *s) s->no_gettext = 1; wt_shortstatus_print(s); } + +void wt_status_print(struct wt_status *s) +{ + switch
[PATCH v4 8/8] status: tests for --porcelain=v2
From: Jeff HostetlerUnit tests for porcelain v2 status format. Signed-off-by: Jeff Hostetler Signed-off-by: Jeff Hostetler --- t/t7064-wtstatus-pv2.sh | 585 1 file changed, 585 insertions(+) create mode 100755 t/t7064-wtstatus-pv2.sh diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh new file mode 100755 index 000..ff0dd3d --- /dev/null +++ b/t/t7064-wtstatus-pv2.sh @@ -0,0 +1,585 @@ +#!/bin/sh + +test_description='git status --porcelain=v2 + +This test exercises porcelain V2 output for git status.' + + +. ./test-lib.sh + +test_expect_success setup ' + test_tick && + git config --local core.autocrlf false && + echo x >file_x && + echo y >file_y && + echo z >file_z && + mkdir dir1 && + echo a >dir1/file_a && + echo b >dir1/file_b +' + + +## +## Confirm output prior to initial commit. +## + +test_expect_success pre_initial_commit_0 ' + cat >expected <<-EOF && + # branch.oid (initial) + # branch.head master + ? actual + ? dir1/ + ? expected + ? file_x + ? file_y + ? file_z + EOF + + git status --porcelain=v2 --branch --ignored --untracked-files=normal >actual && + test_cmp expected actual +' + + +test_expect_success pre_initial_commit_1 ' + git add file_x file_y file_z dir1 && + SHA_A=`git hash-object -t blob -- dir1/file_a` && + SHA_B=`git hash-object -t blob -- dir1/file_b` && + SHA_X=`git hash-object -t blob -- file_x` && + SHA_Y=`git hash-object -t blob -- file_y` && + SHA_Z=`git hash-object -t blob -- file_z` && + SHA_ZERO= && + + cat >expected <<-EOF && + # branch.oid (initial) + # branch.head master + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_A dir1/file_a + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_B dir1/file_b + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_X file_x + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_Y file_y + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_Z file_z + ? actual + ? expected + EOF + + git status --porcelain=v2 --branch --ignored --untracked-files=all >actual && + test_cmp expected actual +' + +## Try -z on the above +test_expect_success pre_initial_commit_2 ' + cat >expected.lf <<-EOF && + # branch.oid (initial) + # branch.head master + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_A dir1/file_a + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_B dir1/file_b + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_X file_x + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_Y file_y + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_Z file_z + ? actual + ? expected + EOF + perl -pe y/\\012/\\000/ expected && + rm expected.lf && + + git status -z --porcelain=v2 --branch --ignored --untracked-files=all >actual && + test_cmp expected actual +' + +## +## Create first commit. Confirm commit sha in new track header. +## Then make some changes on top of it. +## + +test_expect_success initial_commit_0 ' + git commit -m initial && + H0=`git rev-parse HEAD` && + cat >expected <<-EOF && + # branch.oid $H0 + # branch.head master + ? actual + ? expected + EOF + + git status --porcelain=v2 --branch --ignored --untracked-files=all >actual && + test_cmp expected actual +' + + +test_expect_success initial_commit_1 ' + echo x >>file_x && + SHA_X1=`git hash-object -t blob -- file_x` && + rm file_z && + H0=`git rev-parse HEAD` && + + cat >expected <<-EOF && + # branch.oid $H0 + # branch.head master + 1 .M N... 100644 100644 100644 $SHA_X $SHA_X file_x + 1 .D N... 100644 100644 00 $SHA_Z $SHA_Z file_z + ? actual + ? expected + EOF + + git status --porcelain=v2 --branch --ignored --untracked-files=all >actual && + test_cmp expected actual +' + + +test_expect_success initial_commit_2 ' + git add file_x && + git rm file_z && + H0=`git rev-parse HEAD` && + + cat >expected <<-EOF && + # branch.oid $H0 + # branch.head master + 1 M. N... 100644 100644 100644 $SHA_X $SHA_X1 file_x + 1 D. N... 100644 00 00 $SHA_Z $SHA_ZERO file_z + ? actual + ? expected + EOF + + git status --porcelain=v2 --branch --ignored --untracked-files=all >actual && + test_cmp expected actual +' + + +test_expect_success initial_commit_3 ' +
Re: [[PATCH v2] 1/4] patch-ids: stop using a hand-rolled hashmap implementation
Hi Junio, On Mon, 1 Aug 2016, Junio C Hamano wrote: > Johannes Schindelinwrites: > > > It would be a serious bug if hashmap_entry_init() played games with > > references, given its signature (that this function does not have any > > access to the hashmap structure, only to the entry itself): > > > > void hashmap_entry_init(void *entry, unsigned int hash) > > I do not think we are on the same page. The "reference to other > resource" I wondered was inside the hashmap_entry structure, IOW, > "the entry itself". Oh, I see now. > Which is declared to be opaque to the API users, Actually, not really. We cannot do that in C: we need to define the struct in hashmap.h so that its size is known to the users. > so whoever defined that API cannot blame me for not checking its > definition to see that it only has "unsigned int hash" and no allocated > memory or open file descriptor in it that needs freeing. That is the reason, I guess, why we have the documentation in Documentation/technical/api-hashmap.txt: it would have to talk about your hypothetical hashmap_entry_clear() (which would better be named *_release() BTW, unless I misunderstood what you want a hypothetical future version of that function to do). And quite frankly, unless we *have* to, I would rather try to avoid introducing that function as much as possible, as it would make using the hashmap API even more finicky than it already is. > By the way, the first parameter of the function being "void *" is > merely to help lazy API users, who have their own structure that > embeds the hashmap_entry as its first element, as API documentation > tells them to do, e.g. > > struct foo { > struct hashmap_entry e; > ... other "foo" specific fields come here ... > } foo; > > and because of the lazy "void *", they do not have to do this: > > hashmap_entry_init(>e, ...); > > which would be required if the first parameter were "struct > hashmap_entry *", but they can just do this: > > hashmap_entry_init(, ...); Yes, I know that. It is the common way to simulate subclassing in C, for lack of a more compile-safe construct. > I have a slight preference to avoid the lazy "void *", but that is > an unrelated tangent. Oh, we are already safely in Unrelated Tangent Land for a while, I would think. Nothing of what we are discussing in this thread has anything to do with Kevin's patch series, which is about trying to use resources more sensibly when using the revision machinery's --cherry-pick option. And since we are already there, I'll offer an opinion in favor of `void *`: doing the >e dance could quite possibly suggest that `e` is a field just like any other field (and does not necessarily *need* to be the first). But again, this has nothing to do with the patch series we are discussing here. > >> The fact that hashmap_entry_init() is there but there is no > >> corresponding hashmap_entry_clear() hints that there is nothing to be > >> worried about and I can see from the implementation of > >> hashmap_entry_init() that no extra resource is held inside, but an > >> API user should not have to guess. We may want to do one of the two > >> things: > >> > >> * document that an embedded hashmap_entry does not hold any > >>resource that need to be released and it is safe to free the user > >>structure that embeds one; or > >> > >> * implement hashmap_entry_clear() that currently is a no-op. > > > > Urgh. The only reason we have hashmap_entry_init() is that we *may* want > > to extend `struct hashmap_entry` at some point. That is *already* > > over-engineered because that point in time seems quite unlikely to arrive, > > like, ever. > > I am saying that an uneven over-enginnering is bad. Hmm. I guess that the _init() function could be replaced by an _INIT macro a la STRBUF_INIT. Not sure it is really worth the effort, though. Ciao, Dscho -- 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] blame: drop strdup of string literal
On Tue, Jun 14, 2016 at 01:05:41AM -0400, Jeff King wrote: > On Tue, Jun 14, 2016 at 12:32:15AM -0400, Eric Sunshine wrote: > > > > + struct string_list range_list = STRING_LIST_INIT_NODUP; > > > > Related to this series, there's an additional "fix" which ought to be > > made, probably as a separate patch. In particular, in cmd_blame(): > > > > if (lno && !range_list.nr) > > string_list_append(_list, xstrdup("1")); > > > > which supplies a default range ("line 1 through end of file") if -L > > was not specified. I used xstrdup() on the literal "1" in 58dbfa2 > > (blame: accept multiple -L ranges, 2013-08-06) to be consistent with > > parse_opt_string_list() which was unconditionally xstrdup'ing the > > argument (but no longer does as of patch 1/3 of this series). > > Yeah, I'd agree that this is a minor bug both before and after the > series due to the leak. Want to roll a patch on top? Here it is, just to tie up a loose end. I marked you as the author since the hard part was noticing the issue and explaining the history, which you already did above. -- >8 -- From: Eric SunshineSubject: [PATCH] blame: drop strdup of string literal This strdup was added as part of 58dbfa2 (blame: accept multiple -L ranges, 2013-08-06) to be consistent with parse_opt_string_list(), which appends to the same list. But as of 7a7a517 (parse_opt_string_list: stop allocating new strings, 2016-06-13), we should stop using strdup (to match parse_opt_string_list, and for all the reasons described in that commit; namely that it does nothing useful and causes us to leak the memory). Signed-off-by: Jeff King --- builtin/blame.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/blame.c b/builtin/blame.c index ab66cde..29bd479 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -2805,7 +2805,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix) lno = prepare_lines(); if (lno && !range_list.nr) - string_list_append(_list, xstrdup("1")); + string_list_append(_list, "1"); anchor = 1; range_set_init(, range_list.nr); -- 2.9.2.670.g42e63de -- 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] 4/4] rebase: avoid computing unnecessary patch IDs
Hi Junio, On Mon, 1 Aug 2016, Junio C Hamano wrote: > Johannes Schindelinwrites: > > > On Fri, 29 Jul 2016, Junio C Hamano wrote: > > > >> Kevin Willford writes: > >> > >> > static int patch_id_cmp(struct patch_id *a, > >> > struct patch_id *b, > >> > -void *keydata) > >> > +struct diff_options *opt) > >> > { > >> > +if (is_null_sha1(a->patch_id) && > >> > +commit_patch_id(a->commit, opt, a->patch_id, 0)) > >> > +return error("Could not get patch ID for %s", > >> > +oid_to_hex(>commit->object.oid)); > >> > +if (is_null_sha1(b->patch_id) && > >> > +commit_patch_id(b->commit, opt, b->patch_id, 0)) > >> > +return error("Could not get patch ID for %s", > >> > +oid_to_hex(>commit->object.oid)); > >> > return hashcmp(a->patch_id, b->patch_id); > >> > } > >> > >> These error returns initially looks slightly iffy in that in general > >> the caller of any_cmp_fn() wants to know how a/b compares, but by > >> returning error(), it always says "a is smaller than b". > > > > I am to blame, as this is my design. > > > > And yes, it is kind of funny that we require a cmpfn that returns <0, ==0 > > and >0 for comparisons, when hashmaps try to avoid any order. > > Perhaps hashmap API needs fixing in the longer term not to call this > type hashmap_cmp_fn; instead it should lose cmp and say something > like hashmap_eq_fn or something. Maybe. But to make sure: you do not expect Kevin to do that in the context of this here patch series, right? Ciao, Dscho -- 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] gitweb: escape link body in format_ref_marker
On Mon, Aug 1, 2016 at 9:54 PM, Junio C Hamanowrote: > Jakub Narębski writes: > >> Good catch! >> >> Acked-by: Jakub Narębski > > Sigh; the contents may be good but the patch is unusable as-is > because of heavy whitespace damage. > > I'll fix it up. Thanks, both. My apologies for that, it seems that gmail doesn't do tabs. I resubmitted the PR as #283 on GitHub along with Jakub's Ack and fixed the typo he pointed out in the commit message https://github.com/git/git/pull/283 Kind regards andreas > >>> --- >>> gitweb/gitweb.perl | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl >>> index 2fddf75..33d701d 100755 >>> --- a/gitweb/gitweb.perl >>> +++ b/gitweb/gitweb.perl >>> @@ -2090,7 +2090,7 @@ sub format_ref_marker { >>> -href => href( >>> action=>$dest_action, >>> hash=>$dest >>> - )}, $name); >>> + )}, esc_html($name)); >>> >>> $markers .= " >> class=\"".esc_attr($class)."\" title=\"".esc_attr($ref)."\">" . >>> $link . ""; >>> -- 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: git bisect for reachable commits only
Guys, thanks for discussion, I will try to reply in bulk here. First, assuming the common ancestor is GOOD based on the fact that some descendant given as GOOD is pretty bad idea. It may be, but may not be. In the git-flow like workflows new features (aka branches) are created from trunk (master/develop/...) sporadically, but later they will mutual merging. I would say more probably they have not common base, then have. Second, I don't ask "create a new algorithm to find all transition from good/old to bad/new", not nesessary. If programmer feels something suspicious, he/she can create another bisect session with narrowed commit range. Third, testing of any specific commit can be very expensive operation. In my case - shutdown servers/refresh dbs/clean/rebuild in eclipse/running servers/dropping browser cache/running app in browser/going through some pages/view UI. Some of steps of course are automated, but some not. Anyway I spend 5-10 min for every iteration. So knowing what commit is bad or good is very valuable, then I'm very interested to hunt the bug-introduced commit with minimal count of testing. Scenario 4 (I will keep my previous mail numbering for possible later reference) z1z2---z3 / / \ Gx1x2/---x3---x4--B \ / / y1--y2--y3--y4 This is the happy straight case with closed DAG (hehe, git for scientists) between given G good and B bad commits. Ideal bisect will check first the shortest way between G & B: x1/x2/x3/x4. Let name first-bug commit we are really hunting H and current first-bug candidate as h. If h == x1 or x2 -> stop, found If h == x3, bisect will try to test y2/y3/y4 path only If h == x4, bisect will select shortest path z1/z2 (keeping in mind, that x2 is already tested and is good) If h == z1 - found if h == z2 - looking in path y1/y2/y3 Scenario 5. v1---v2 / \ w1--/---w2---w3-w4--w5 / / / \ / / /z1z2---z3 \ / / // / \ \ C3--C2--C1--G--x1x2/---x3---x4--x5--x6--B \ / / y1--y2--y3--y4 Unhappy case, we have side branches which may introduce bug behaviour, we need to look it through to figure out why it was done, what problem was solved for that and so on. Let looking in shortest path x1-x6. If h == x1..x4 - happy use case of scenario 4. If discover that h == x5, we are forgetting about z/y paths, but first we looking for nearest common commit (C1). As far as we agree that currently is not clear when the new feature was introduced we need to explicit check commit C1 whether it contains a feature we are hunting bug up. if C1 is good then pretty possible bad transition was happend in w2-w5 commits. Else (C1 is bad) assume that there is no transition from good to bad, then assume H == x5 (stop) if C1 is good and h == w4/w5 => stop, else if h == w3, new roundtrip, forgetting about w1 commit(not interesting), testing C2, if bad - stop H == w3, if good, v1/v2 commits are to test. else if h == w2, forgetting C2 testing, just testing C3. If bad, stop, H == w3, if good, w1 to test. Using this approach we can safe working with ever octopus merging (personally I'm not using, but why not) Scenario 6. z1---z2---z3 // \ C1--G--x1x2/--x3 | \ \ / \| \ y1--y2--y3--y4--y5--y6--B \ \ /| \ w1--w2-w3 | \ / v1--v2 Important note. Before start every side circuit based on common ancestor user should be explicitly warned, that this is not just ordinal intermediate bisect commit testing, but possible new round trips with new commit/steps counts For example, if current shortest path is x1-x6, bisect should say about only 6 commits (3 after bisect), not calculating commits in other paths. Reaching node decision, bisect will stay and prompt for testing new common ancestor with clear instructions what happens, if it will be good or bad, (new unchecked commits and new left bisect steps, in case good and stop or switch to other path in case of octopus). I have another request to get git bisect more user-friendly, regarding rolling back last step or steps, if accidentally 'git bisect bad' or 'good' was wrong entered, but I think it worth for another thread. Cheers, Oleg -- 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] 4/4] rebase: avoid computing unnecessary patch IDs
W dniu 01.08.2016 o 22:11, Junio C Hamano pisze: > Johannes Schindelinwrites: >> On Fri, 29 Jul 2016, Junio C Hamano wrote: >>> These error returns initially looks slightly iffy in that in general >>> the caller of any_cmp_fn() wants to know how a/b compares, but by >>> returning error(), it always says "a is smaller than b". >> >> I am to blame, as this is my design. >> >> And yes, it is kind of funny that we require a cmpfn that returns <0, ==0 >> and >0 for comparisons, when hashmaps try to avoid any order. > > Perhaps hashmap API needs fixing in the longer term not to call this > type hashmap_cmp_fn; instead it should lose cmp and say something > like hashmap_eq_fn or something. The problem is that one expects hashmap_cmp_fn() to return ==0 on equality, while one would expect for hashmap_eq_fn() to return true (==1) on equality. So we would have to rewrite all calling sites. It would be nice to have a comment about how hashmap uses cmpfn in hashmap.h. Though... currently our hashmap implementation uses linked list (separate chaining) for handling hash collisions (for collision resolution). More sophisticated data structures, such as balanced search trees, or splay trees, are worth considering only if the load factor is large, or if the hash distribution is likely to be very non-uniform, or if one must guarantee good performance even in a worst-case scenario. So almost certainly comparing function (which is needed for trees) won't be needed. Best, -- Jakub Narębski -- 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 v6 06/16] merge_recursive: abort properly upon errors
Hi Junio, On Mon, 1 Aug 2016, Junio C Hamano wrote: > Johannes Schindelinwrites: > > > Subject: Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors > > s/merge_/merge-/; for this one alone. I see that de8946de1694a8cf311daab7b2c416d76cb04d23 still shows it with an underscore instead of a dash, while you fixed my other tyops in this patch series. I tend to think that the underscore is correct: this change is not so much about the builtin (which is written with a dash) but about the function (written with an underscore, used by more than just merge-recursive, e.g. cherry-pick). > > There are a couple of places where return values never indicated errors > > before, as wie simply died instead of returning. > > s/wie/we/; Right. What can I say, I am surrounded by too many Germans. I fixed this locally, in case another re-roll should be required. What you have in `pu` looks correct to me, though. Let me know if you want me to re-submit nevertheless. BTW I should have said this earlier: I run all my rebases, all my merges, all my cherry-picks using a Git version with these patches for months (of course, the patches have changed, but not in the most critical parts I was concerned about, the parts where die() calls were replaced). If I would have found any regression, I would have notified you immediately, of course. Ciao, Dscho -- 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 v6 05/16] Prepare the builtins for a libified merge_recursive()
Hi Junio, On Mon, 1 Aug 2016, Junio C Hamano wrote: > Johannes Schindelinwrites: > > > Previously, callers of merge_trees() or merge_recursive() expected that > > code to die() with an error message. This used to be okay because we > > called those commands from scripts, and had a chance to print out a > > message in case the command failed fatally (read: with exit code 128). > > > > As scripting incurs its own set of problems (portability, speed, > > idiosynchracies of different shells, limited data structures leading to > > I think I typofixed this when I queued the previous one on 'pu' > already, but s/synch/sync/; Whoops. Fixed locally. Ciao, Dscho -- 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 06/10] run-command: add clean_on_exit_handler
> On 02 Aug 2016, at 07:53, Johannes Sixtwrote: > > Am 01.08.2016 um 13:14 schrieb Lars Schneider: > >> On 30 Jul 2016, at 11:50, Johannes Sixt wrote: > >> Am 30.07.2016 um 01:37 schrieb larsxschnei...@gmail.com: > >>> static struct child_to_clean *children_to_clean; > >>> @@ -30,6 +31,8 @@ static void cleanup_children(int sig, int in_signal) > >>> { > >>> while (children_to_clean) { > >>> struct child_to_clean *p = children_to_clean; > >>> + if (p->clean_on_exit_handler) > >>> + p->clean_on_exit_handler(p->pid); > >> > >> This summons demons. cleanup_children() is invoked from a signal > >> handler. In this case, it can call only async-signal-safe functions. > >> It does not look like the handler that you are going to install > >> later will take note of this caveat! > >> > >>> children_to_clean = p->next; > >>> kill(p->pid, sig); > >>> if (!in_signal) > >> > >> The condition that we see here in the context protects free(p) > >> (which is not async-signal-safe). Perhaps the invocation of the new > >> callback should be skipped in the same manner when this is called > >> from a signal handler? 507d7804 (pager: don't use unsafe functions > >> in signal handlers) may be worth a look. > > > > Thanks a lot of pointing this out to me! > > > > Do I get it right that after the signal "SIGTERM" I can do a cleanup > > and don't need to worry about any function calls but if I get any > > other signal then I can only perform async-signal-safe calls? > > No. SIGTERM is not special. > > Perhaps you were misled by the SIGTERM mentioned in > cleanup_children_on_exit()? This function is invoked on regular exit (not > from a signal). SIGTERM is used in this case to terminate children that are > still lingering around. Yes, that was my source of confusion. Thanks for the clarification! > > > If this is correct, then the following solution would work great: > > > > if (!in_signal && p->clean_on_exit_handler) > > p->clean_on_exit_handler(p->pid); > > This should work nevertheless because in_signal is set when the function is > invoked from a signal handler (of any signal that is caught) via > cleanup_children_on_signal(). Right. Thank you! - Lars-- 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 06/10] run-command: add clean_on_exit_handler
Am 01.08.2016 um 13:14 schrieb Lars Schneider: >> On 30 Jul 2016, at 11:50, Johannes Sixtwrote: >> Am 30.07.2016 um 01:37 schrieb larsxschnei...@gmail.com: >>> static struct child_to_clean *children_to_clean; >>> @@ -30,6 +31,8 @@ static void cleanup_children(int sig, int in_signal) >>> { >>>while (children_to_clean) { >>>struct child_to_clean *p = children_to_clean; >>> + if (p->clean_on_exit_handler) >>> + p->clean_on_exit_handler(p->pid); >> >> This summons demons. cleanup_children() is invoked from a signal >> handler. In this case, it can call only async-signal-safe functions. >> It does not look like the handler that you are going to install >> later will take note of this caveat! >> >>>children_to_clean = p->next; >>>kill(p->pid, sig); >>>if (!in_signal) >> >> The condition that we see here in the context protects free(p) >> (which is not async-signal-safe). Perhaps the invocation of the new >> callback should be skipped in the same manner when this is called >> from a signal handler? 507d7804 (pager: don't use unsafe functions >> in signal handlers) may be worth a look. > > Thanks a lot of pointing this out to me! > > Do I get it right that after the signal "SIGTERM" I can do a cleanup > and don't need to worry about any function calls but if I get any > other signal then I can only perform async-signal-safe calls? No. SIGTERM is not special. Perhaps you were misled by the SIGTERM mentioned in cleanup_children_on_exit()? This function is invoked on regular exit (not from a signal). SIGTERM is used in this case to terminate children that are still lingering around. > If this is correct, then the following solution would work great: > >if (!in_signal && p->clean_on_exit_handler) >p->clean_on_exit_handler(p->pid); This should work nevertheless because in_signal is set when the function is invoked from a signal handler (of any signal that is caught) via cleanup_children_on_signal(). -- Hannes -- 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