Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]
W dniu 25.08.2016 o 14:58, Johannes Schindelin pisze: > On Mon, 22 Aug 2016, Eric Wong wrote: >> Johannes Schindelinwrote: >> >>> I just want developers who are already familiar with Git, and come up with >>> an improvement to Git itself, to be able to contribute it without having >>> to pull out their hair in despair. >> >> We want the same thing. I just want to go farther and get >> people familiar with (federated|decentralized) tools instead of >> proprietary and centralized ones. > > Why require users to get familiar with (federated|decentralized) tools > *unless* they make things provably more convenient? So far, I only see > that this would add to the hurdle, not improve things. Arguably for some federated/decentralized tools are preferred (for philosophical reasons), even if they do not achieve even feature parity with centralized tools (c.f. FSF). Though Git is not there to improve the world, just to be better... On the other hand some may say that centralized tools (such as GitHub and its pull requests) do not achieve feature parity with email based workflow... though the basics are here. Best regards, -- 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: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]
W dniu 22.08.2016 o 15:15, Duy Nguyen pisze: > On Mon, Aug 22, 2016 at 8:06 PM, Johannes Schindelin >wrote: >> >> My point stands. We are way more uninviting to contributors than >> necessary. And a huge part of the problem is that we require contributors >> to send their patches inlined into whitespace-preserving mails. > > We probably can settle this in the next git survey with a new > question: what's stopping you from contributing to git? Added to second take on proposed questions for Git User's Survey 2016 https://public-inbox.org/git/ae804c55-d145-fc90-e1a9-6ebd6c604...@gmail.com/T/#u '[RFCv2] Proposed questions for "Git User's Survey 2016", take two' Message-ID: <91a2ffbe-a73b-fbb9-96da-9aea4d439...@gmail.com> -- 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
[RFCv2] Proposed questions for "Git User's Survey 2016", take two
Hello, As I wrote previously here, I am thinking about returning to doing the Git User's Survey this year. Message-ID: <577e6f32.7020...@gmail.com> https://marc.info/?l=git=146790381602547=2 https://public-inbox.org/git/577E6F32.7020007%40gmail.com/ In this email I'd like to propose the revised list of questions (and answers) for Git User's Survey 2016, following comments to the previous revision Message-ID: <91a2ffbe-a73b-fbb9-96da-9aea4d439...@gmail.com> https://public-inbox.org/git/91a2ffbe-a73b-fbb9-96da-9aea4d439...@gmail.com/ Below is my second proposal; old comments are prefixed with "jn> ", new ones are prefixed with "JN> ". About you jn> This section gives us a bit of demographic information about survey jn> responders. Is it useful? Should we keep it? 01. What country do you live in? (Country of residence, in English) (free-form single line) jn> Survs.com do not offer list of countries as a pre-defined drop-down jn> list (select, with search), and it looks like it is not as easy as jn> I thought (though I could push responsibility to Locale::Country ;-): jn> jn> https://en.wikipedia.org/wiki/List_of_sovereign_states jn> jn> This question originally read "What country are you from?" jn> which might be thought as country of birth... which country jn> may not exist any longer, like East Germany. 02. How old are you (in years)? (single number, or single choice) jn> Perhaps a selection of age ranges would be better (if less precise). jn> We could follow the example of StackOverflow Survey here, see below. JN> Though oldest / youngest info could be interesting... * <20 * 20-24 * 25-29 * 30-34 * 35-39 * 40-49 * 50-59 * >60 03. Your gender (single choice) * Man * Woman * Other * Prefer to not disclose jn> This would be new question, to check if we good good diversity jn> among survey responders (and not skewed in particular direction). jn> Though I am not sure if it is a good idea... 04. How would you describe your occupation / role as Git user? (multiple choice with other, limit to 3 selections (?)) + Developer + Programmer + Engineer + Analyst + Manager / Leader + Maintainer / Reviewer / Sub-maintainer + DevOps + Administrator + Designer + Artist / Writer + Tester / QA + Expert + Student + Researcher + Teacher + Other, please specify jn> This is a new question, based somewhat on "Developer Occupations" jn> and "Programmers, Engineers, and Developers" questions in jn> StackOverflow Survey. It is here to find out if different jn> occupation leads to different ways of using Git, and if there jn> is some occupation that is not well served by Git. jn> jn> The list of "occupations" is preliminary, and I would like to jn> ask for your thought about possible answers. 05. Have you ever contributed to Git project (code, documentation, i18n, etc.)? (single choice) * Yes * No * Maybe # Tried to <--- new answer jn> This is here to correlate other responses with Git developers. jn> In 2012 it was called "Does Git include code or documentation by you? jn> (Are you a Git developer?)". I think this way of stating it is better; jn> if it is here to stay - the number of active and past developers is jn> not very large. On the other hand it can be used to detect if we jn> have a bias due to the way survey is announced. JN> This would also serve as a reminder that Git is an open-source JN> project (and free software), and anyone can contribute to it. JN> Should there be a similar question about doing review? 06. What's stopping you from contributing to Git? What was hardest / most difficult when contribution to Git? (free-text essay question) JN> This was the question proposed by Duy Nguyen in "Working with JN> public-inbox.org" thread (well, the first part of it): JN> https://public-inbox.org/git/CACsJy8BG63oaLbw0f7try3OpzdphLC7UGAaJ=vgikeb36pa...@mail.gmail.com/ jn> NOTE: earlier surveys have had two additional questions that were jn> since removed (to make survey shorter, among others). Those were: jn> jn> - What is your preferred [non-programming] language? jn> - Which programming languages / technologies you are proficient with? jn> jn> Well, the latter was only about programming languages, originally. JN> And it would be possibly added, of in later section ("Other tools"). Getting started with Git jn> This is probably not the best name for this section of questions. JN> Can anyone think of a better one? 07. Have you found Git easy to learn? (single choice) * Very easy * Easy * Reasonably easy * Hard * Very hard 08. Have you found Git easy to use? (single choice) * Very easy * Easy * Reasonably easy * Hard * Very hard jn> Those two questions, considered alone, doesn't tell us much. If one jn> uses git, then usually one does
Re: [PATCH v14 09/27] bisect--helper: `bisect_write` shell function in C
Pranit Bauvawrites: >>> +struct bisect_terms { >>> + struct strbuf term_good; >>> + struct strbuf term_bad; >>> +}; >> >> I think "struct strbuf" is overrated. ... >> I think you can just say "const char *" in this case. > > Using struct strbuf is not really overrated but in fact required. But Nothing is required. I can make your life easier by requiring you to never use struct strbuf as a structure field type, though. You will almost never need it unless you are building something exotic anyway. Step back and think what "strbuf" is good for. It holds a pointer to a piece of memory the field owns, over-allocates and knows the size of allocation and usage. That is good if you need to (1) frequently find out the length of the string; without a separate .len member you would have to run strlen(). (2) incrementally add to the string in-place; as it overallocates, appending to the string would not have to involve realloc() every time and the cost of it is amortized. (3) make complex operations like splicing another string in, trimming substring out, etc. You need to do none of the above to these fields. term.term_good is either taken from an argv[] element, or you read from a line from a file and set it. You may do some trivial computation and set it to the result, like "the other field uses 'old', so this one need to be set to 'new'". The user of the field either has the string and sets it there, or reads the field's value as a whole string. No string manipulation in the field in-place is needed. > yes, for this patch it might seem as overrated. In the shell code > initally TERM_GOOD is set to "good" while TERM_BAD is set to "bad". > Now there are a lot of instances (one of which is bisect_start() > function) where this can change. So if we keep it as "const char *", > it would be right to change the value of it after wards. And we cannot > keep it as "char []" because we don't know its size before hand. You are not making sense. Nobody would suggest to use a fixed-size char array in this structure. That wouldn't even work in the case where you are stuffing what comes in an argv[] element in there, e.g. terms.term_good = argv[3]; And you can of course support changing the value of the field without using strbuf. Just update the pointer to point at the piece of memory that holds the new value. In short, I do not see any good reason why the term_good field has to be anything other than "char *term_good" or "const char *term_good". Now, what you need to consider choosing between the two depends on where these strings can come from. If they are known to be always unchanging between the time you set it til the end of the program (e.g. using an element in argv[]), you can just assign without making any copy and you can use "const char *term_good". All other cases, the structure needs to take the ownership, and you would need to make a copy if you don't have the ownership, e.g. terms.term_good = xstrdup(argv[3]); You may be reading from a file, a line at a time and you may have a line's content in a strbuf. You do not (yet) own the buffer after reading it, e.g. strbuf_getline(, fp); terms.term_good = strbuf_detach(, NULL); Of course, if you need to take ownership of the memory, you would need to free(3) it as needed, which means the pattern to set the field would become free(terms.term_good); terms.term_good = ... some new value ...; Using strbuf as a local variable is good. It gives a higher level of abstraction when you are actually performing string operations. In most applications, however, a field in a struct is where the result of a step of computation is kept, not a scratch-pad to perform steps of computation in. When you are ready to update the value of a field, you _have_ a completed string, and you can just use "char *" field to point at it. There is no need for strbuf in the field. Don't look at the data structure used in trailer.[ch] as a model; it is an example of a terribly bad implementation taste, a pattern that should not be followed. Print it, not read it and burn it as a good symbolic gesture. -- 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 v14 13/27] bisect--helper: `bisect_start` shell function partially in C
Pranit Bauvawrites: >> I wonder the whole thing above is better restructured to avoid >> repeated checks of the same thing. >> >> if (is it 40-hex, i.e. detached?) { >> stuff it to start_head; >> } else if (skip_prefix(head, "refs/heads/", )) { >> do the "cogito" check; >> stuff it to start_head; >> } else { >> that's a strange symbolic ref HEAD you have there; >> } > > I guess it changes the behaviour. Its a strange symbolic ref if it > does not start with "refs/heads". I did not think my suggestion would change the behaviour at all. It would change the code structure a bit to make it more readable, though. -- 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 v14 21/27] bisect--helper: `bisect_log` shell function in C
Hey Junio, On Sat, Aug 27, 2016 at 4:37 AM, Junio C Hamanowrote: > Pranit Bauva writes: > >> +static int bisect_log(void) >> +{ >> + struct strbuf buf = STRBUF_INIT; >> + >> + if (strbuf_read_file(, git_path_bisect_log(), 256) < 0) { >> + strbuf_release(); >> + return error(_("We are not bisecting.\n")); >> + } >> + >> + printf("%s", buf.buf); >> + strbuf_release(); >> + >> + return 0; >> +} > > Hmph, is it really necessary to slurp everything in a strbuf before > sending it out to the standard output? Wouldn't it be sufficient to > open a file descriptor for reading on the log file and then hand it > over to copy.c::copy_fd()? That is actually much better. Thanks! Regards, Pranit Bauva -- 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: Crash when using git blame on untracked file
Hi, On 08/27, Simon Ruderich wrote: > Hello, > > I'm seeing the following crash with Git 2.9.3 on Debian sid > (amd64): > > $ git init foo > $ cd foo > $ touch x > $ git add x > $ git commit -m test > $ touch x.conf > $ git blame x.conf > segmentation fault > > I've tested it on Debian stable's 2.1.4 which works fine: > > $ git blame x.conf > fatal: no such path 'x.conf' in HEAD > > It requires the blamed file to be present, but in some cases it > works only if the file e.g. "x" is already tracked in Git and the > other file is called "x.conf" (".conf"-suffix). But in an empty > repo it seems to happen always. > > Sadly Debian's git has no dbg-package, so the stacktrace is not > very useful: > > #0 __strcmp_sse2_unaligned () at > ../sysdeps/x86_64/multiarch/strcmp-sse2-unaligned.S:31 > #1 0x0041ad7a in ?? () > #2 0x00406171 in ?? () > #3 0x00405321 in ?? () > #4 0x76f9f700 in __libc_start_main (main=0x4051c0, argc=0x3, > argv=0x7fffe1a8, init=, fini=, > rtld_fini=, stack_end=0x7fffe198) > at ../csu/libc-start.c:291 > #5 0x004057d9 in ?? () > Thank you very much for the bug report. A proposed fix for it below: --- >8 --- Subject: [PATCH] blame: fix segfault on untracked files Since 3b75ee9 ("blame: allow to blame paths freshly added to the index", 2016-07-16) git blame also looks at the index to determine if there is a file that was freshly added to the index. cache_name_pos returns -pos - 1 in case there is no match is found, or if the name matches, but the entry has a stage other than 0. As git blame should work for unmerged files, it uses strcmp to determine whether the name of the returned position matches, in which case the file exists, but is merely unmerged, or if the file actually doesn't exist in the index. If the repository is empty, or if the file would lexicographically be sorted as the last file in the repository, -cache_name_pos - 1 is outside of the length of the active_cache array, causing git blame to segfault. Guard against that, and die() normally to restore the old behaviour. Reported-by: Simon RuderichSigned-off-by: Thomas Gummerer --- builtin/blame.c | 3 ++- t/t8002-blame.sh | 7 +++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/builtin/blame.c b/builtin/blame.c index 7ec7823..a5bbf91 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -2244,7 +2244,8 @@ static void verify_working_tree_path(struct commit *work_tree, const char *path) pos = cache_name_pos(path, strlen(path)); if (pos >= 0) ; /* path is in the index */ - else if (!strcmp(active_cache[-1 - pos]->name, path)) + else if (-1 - pos < active_nr && +!strcmp(active_cache[-1 - pos]->name, path)) ; /* path is in the index, unmerged */ else die("no such path '%s' in HEAD", path); diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh index ff09ace..7983bb7 100755 --- a/t/t8002-blame.sh +++ b/t/t8002-blame.sh @@ -6,6 +6,13 @@ test_description='git blame' PROG='git blame -c' . "$TEST_DIRECTORY"/annotate-tests.sh +test_expect_success 'blame untracked file in empty repo' ' + touch untracked && + test_must_fail git blame untracked 2>actual.err && + echo "fatal: no such path '\''untracked'\'' in HEAD" >expected.err && + test_cmp expected.err actual.err +' + PROG='git blame -c -e' test_expect_success 'blame --show-email' ' check_count \ -- 2.8.4.1.g3b75ee9 -- 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 v14 13/27] bisect--helper: `bisect_start` shell function partially in C
Hey Junio, On Fri, Aug 26, 2016 at 12:32 AM, Junio C Hamanowrote: > Pranit Bauva writes: > >> +static int bisect_start(struct bisect_terms *terms, int no_checkout, >> + const char **argv, int argc) >> +{ >> + int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0; >> + int flags, pathspec_pos; >> + struct string_list revs = STRING_LIST_INIT_DUP; >> + struct string_list states = STRING_LIST_INIT_DUP; > > The original has a single state, not states. Let's see if there is > a reason behind the name change > >> + unsigned char sha1[20]; >> + struct object_id oid; > > More on these below... > >> + ... >> + for (i = 0; i < argc; i++) { >> + const char *commit_id = xstrfmt("%s^{commit}", argv[i]); >> + if (!strcmp(argv[i], "--")) { >> + has_double_dash = 1; >> + break; >> + } else if (!strcmp(argv[i], "--no-checkout")) >> + no_checkout = 1; >> + else if (!strcmp(argv[i], "--term-good") || >> + ... >> + } else 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]); >> + } else if (get_oid(commit_id, ) && has_double_dash) { >> + string_list_clear(, 0); >> + string_list_clear(, 0); >> + die(_("'%s' does not appear to be a valid revision"), >> argv[i]); >> + } else { >> + string_list_append(, oid_to_hex()); >> + } >> + } > > What I do not understand is clearing the string list "states" inside > this loop. It may have been INIT_DUPed, but nothing in this loop > adds anything to it. Because "revs" does get extended in the loop, > it is understandable if you wanted to clear it before dying, but "if > you are dying anyway why bother clearing?" is also a valid stance to > take. I think I should probably use return here instead of die(). > The same "perhaps want to use the 'retval' with goto 'finish:' pattern?" > comment applies here, too. Okay sure could do that. >> + pathspec_pos = i; >> + >> + /* >> + * 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 |= !!revs.nr; >> + for (i = 0; i < revs.nr; i++) { >> + if (bad_seen) >> + string_list_append(, terms->term_good.buf); >> + else { >> + bad_seen = 1; >> + string_list_append(, terms->term_bad.buf); >> + } >> + } > > This is certainly different from the original. We used to do one > "bisect_write" per element in revs in the loop; we no longer do that > and instead collect them in states list. Each element in these two > lists, i.e. revs.item[i] and states.item[i], corresponds to each > other. > > That explains why the variable is renamed to states. We haven't > seen enough to say if this behaviour change is a good idea or not. > >> + /* >> + * Verify HEAD >> + */ >> + head = resolve_ref_unsafe("HEAD", 0, sha1, ); >> + if (!head) { >> + if (get_sha1("HEAD", sha1)) { >> + string_list_clear(, 0); >> + string_list_clear(, 0); >> + die(_("Bad HEAD - I need a HEAD")); >> + } >> + } >> + if (!is_empty_or_missing_file(git_path_bisect_start())) { >> + /* Reset to the rev from where we started */ >> + strbuf_read_file(_head, git_path_bisect_start(), 0); >> + strbuf_trim(_head); >> + if (!no_checkout) { >> + struct argv_array argv = ARGV_ARRAY_INIT; >> + argv_array_pushl(, "checkout", start_head.buf, >> + "--", NULL); >> + if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) { >> + error(_("checking out '%s' failed. Try 'git " >> + "bisect start '."), >> + start_head.buf); >> + strbuf_release(_head); >> + string_list_clear(, 0); >> + string_list_clear(, 0); >> + return -1; > > The original died here, but you expect the caller to respond to a > negative return. I haven't read enough to judge if that is a good > idea, but doesn't it make sense to do the same throughout the > function consistently? I saw a few die()'s
Re: [PATCH v13 00/14] libify apply and use lib in am, part 3
On Sat, Aug 27, 2016 at 8:45 PM, Christian Couderwrote: > > I will send a diff between this version and v12 as a reply to this > email. Here is the diff: diff --git a/apply.c b/apply.c index 7e561a4..5cd037d 100644 --- a/apply.c +++ b/apply.c @@ -3993,12 +3993,21 @@ static int check_patch_list(struct apply_state *state, struct patch *patch) return err; } +static int read_apply_cache(struct apply_state *state) +{ +if (state->index_file) +return read_cache_from(state->index_file); +else +return read_cache(); +} + /* This function tries to read the sha1 from the current index */ -static int get_current_sha1(const char *path, unsigned char *sha1) +static int get_current_sha1(struct apply_state *state, const char *path, +unsigned char *sha1) { int pos; -if (read_cache() < 0) +if (read_apply_cache(state) < 0) return -1; pos = cache_name_pos(path, strlen(path)); if (pos < 0) @@ -4042,7 +4051,7 @@ static int preimage_sha1_in_gitlink_patch(struct patch *p, unsigned char sha1[20 } /* Build an index that contains the just the files needed for a 3way merge */ -static int build_fake_ancestor(struct patch *list, const char *filename) +static int build_fake_ancestor(struct apply_state *state, struct patch *list) { struct patch *patch; struct index_state result = { NULL }; @@ -4071,7 +4080,7 @@ static int build_fake_ancestor(struct patch *list, const char *filename) ; /* ok */ } else if (!patch->lines_added && !patch->lines_deleted) { /* mode-only change: update the current */ -if (get_current_sha1(patch->old_name, sha1)) +if (get_current_sha1(state, patch->old_name, sha1)) return error("mode change for %s, which is not " "in current HEAD", name); } else @@ -4089,12 +4098,13 @@ static int build_fake_ancestor(struct patch *list, const char *filename) } } -hold_lock_file_for_update(, filename, LOCK_DIE_ON_ERROR); +hold_lock_file_for_update(, state->fake_ancestor, LOCK_DIE_ON_ERROR); res = write_locked_index(, , COMMIT_LOCK); discard_index(); if (res) -return error("Could not write temporary index to %s", filename); +return error("Could not write temporary index to %s", + state->fake_ancestor); return 0; } @@ -4683,7 +4693,7 @@ static int apply_patch(struct apply_state *state, state->newfd = hold_locked_index(state->lock_file, 1); } -if (state->check_index && read_cache() < 0) { +if (state->check_index && read_apply_cache(state) < 0) { error(_("unable to read index file")); res = -128; goto end; @@ -4715,7 +4725,7 @@ static int apply_patch(struct apply_state *state, } if (state->fake_ancestor && -build_fake_ancestor(list, state->fake_ancestor)) { +build_fake_ancestor(state, list)) { res = -128; goto end; } -- 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 v13 11/14] apply: refactor `git apply` option parsing
Parsing `git apply` options can be useful to other commands that want to call the libified apply functionality, because this way they can easily pass some options from their own command line to the libified apply functionality. This will be used by `git am` in a following patch. To make this possible, let's refactor the `git apply` option parsing code into a new libified apply_parse_options() function. Doing that makes it possible to remove some functions definitions from "apply.h" and make them static in "apply.c". Helped-by: Ramsay JonesSigned-off-by: Christian Couder --- apply.c | 103 +--- apply.h | 18 +++--- builtin/apply.c | 74 ++-- 3 files changed, 97 insertions(+), 98 deletions(-) diff --git a/apply.c b/apply.c index bf81b70..2ec2a8a 100644 --- a/apply.c +++ b/apply.c @@ -4730,16 +4730,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); @@ -4747,9 +4747,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); @@ -4757,8 +4757,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) @@ -4768,8 +4768,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; @@ -4778,8 +4778,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); @@ -4893,3 +4893,80 @@ int apply_all_patches(struct apply_state *state, return res; return (res == -1 ? 1 : 128); } + +int apply_parse_options(int argc, const char **argv, + struct apply_state *state, + int *force_apply, int *options, + const char * const *apply_usage) +{ + struct option builtin_apply_options[] = { + { OPTION_CALLBACK, 0, "exclude", state, N_("path"), + N_("don't apply changes matching the given path"), + 0, apply_option_parse_exclude }, + { OPTION_CALLBACK, 0, "include", state, N_("path"), + N_("apply changes matching the given path"), + 0, apply_option_parse_include }, + { OPTION_CALLBACK, 'p', NULL, state, N_("num"), + N_("remove leading slashes from traditional diff paths"), + 0, apply_option_parse_p }, + OPT_BOOL(0, "no-add", >no_add, + N_("ignore additions made by the patch")), + OPT_BOOL(0, "stat", >diffstat, + N_("instead of applying the patch, output diffstat for the input")), + OPT_NOOP_NOARG(0, "allow-binary-replacement"), + OPT_NOOP_NOARG(0, "binary"), + OPT_BOOL(0, "numstat", >numstat, +
[PATCH v13 00/14] libify apply and use lib in am, part 3
Goal This is a patch series about libifying `git apply` functionality, and using this libified functionality in `git am`, so that no 'git apply' process is spawn anymore. This makes `git am` significantly faster, so `git rebase`, when it uses the am backend, is also significantly faster. Previous discussions and patches series ~~~ This has initially been discussed in the following thread: http://thread.gmane.org/gmane.comp.version-control.git/287236/ Then the following patch series were sent: RFC: http://thread.gmane.org/gmane.comp.version-control.git/288489/ v1: http://thread.gmane.org/gmane.comp.version-control.git/292324/ v2: http://thread.gmane.org/gmane.comp.version-control.git/294248/ v3: http://thread.gmane.org/gmane.comp.version-control.git/295429/ v4: http://thread.gmane.org/gmane.comp.version-control.git/296350/ v5: http://thread.gmane.org/gmane.comp.version-control.git/296490/ v6: http://thread.gmane.org/gmane.comp.version-control.git/297024/ v7: http://thread.gmane.org/gmane.comp.version-control.git/297193/ v8: https://public-inbox.org/git/20160627182429.31550-1-chriscool%40tuxfamily.org/ v9: https://public-inbox.org/git/20160730172509.22939-1-chriscool%40tuxfamily.org/ v10: https://public-inbox.org/git/20160808210337.5038-1-chriscool%40tuxfamily.org/ v11: https://public-inbox.org/git/20160811085229.19017-1-chriscool%40tuxfamily.org/ v12: https://public-inbox.org/git/20160811184501.384-1-chrisc...@tuxfamily.org/ Highlevel view of the patches in the series ~~~ This is "part 3" of the full patch series. I am resending only the last 14 patches of the full series as "part 3", because I don't want to resend the first 27 patches of v10 nearly as is. (So "part 2" is made of patch 01/40 from v11 and patches from 02/40 to 27/40 from v10. And "part 1" has been in "master" for some time now.) - Patch 01/14 to 04/14 were v1, v2, v6, v7, v8, v9, v10, v11 and v12. They haven't changed since v12. They finish libifying the apply functionality that was in builtin/apply.c and move it into apply.{c,h}, but the libified functionality is not yet used in `git am`. - Patch 05/14 was in v6, v7, v8, v9, v10 and v12 and hasn't changed. It replaces some calls to error() with calls to error_errno(). - Patches 06/14 to 10/14 were in v2, v6, v7, v8, v9 and v10. They haven't changed since v10. They implement a way to make the libified apply code silent by changing the bool `apply_verbosely` into a tristate enum called `apply_verbosity`, that can be one of `verbosity_verbose`, `verbosity_normal` or `verbosity_silent`. This is because "git am", since it was a shell script, has been silencing the apply functionality by redirecting file descriptors to /dev/null, but this is not acceptable in C. - Patch 11/14 was in v9, v10 and v12, and hasn't changed. It refactors `git apply` option parsing to make it possible for `git am` to easily pass some command line options to the libified applied code. - Patch 12/14 is new. It is a refactoring to prepare for some new changes in patch 13/14. - Patch 13/14 was in v12. It replaces patch 33/40 in v10 (environment: add set_index_file()) that was a hack to make it possible for `git am` to use the libified apply functionality on a different index file. For this purpose in this patch we add a "const char *index_file" into "struct apply_state", and when "index_file" is set, we use hold_lock_file_for_update(), passing it "state->index_file", instead of using hold_locked_index(), as it is not possible to pass an index filename in hold_locked_index(). This patch was changed in this version to also use "read_cache_from(state->index_file)" instead of "read_cache()" when state->index_file is set. - Patch 14/14 was in v1, v2, v6, v7, v8, v9, v10 and v12, and hasn't changed since v12. This patch makes `git am` use the libified functionality. General comments Now this patch series is shorter than previously. Hopefully also the early part of this series until 05/14 or 11/14 will be ready soon to be moved to next and master, and I may only need to resend the last 3 patches if anything. I will send a diff between this version and v12 as a reply to this email. The benefits are not just related to not creating new processes. When `git am` launched a `git apply` process, this new process had to read the index from disk. Then after the `git apply`process had terminated, `git am` dropped its index and read the index from disk to get the index that had been modified by the `git apply`process. This was inefficient and also prevented the split-index mechanism to provide many performance benefits. By the way, current work is ongoing to make it possible to use split-index more easily by adding a config variable, see: https://public-inbox.org/git/20160711172254.13439-1-chriscool%40tuxfamily.org/ Using an earlier version of this series as rebase material, Duy
[PATCH v13 01/14] builtin/apply: rename option parsing functions
As these functions are going to be part of the libified apply API, let's give them a name that is more specific to the apply API. Signed-off-by: Christian Couder--- builtin/apply.c | 40 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/builtin/apply.c b/builtin/apply.c index ad403f8..429fe44 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -4588,16 +4588,16 @@ static int apply_patch(struct apply_state *state, return res; } -static int 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; } -static int 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); @@ -4605,9 +4605,9 @@ static int option_parse_include(const struct option *opt, return 0; } -static int 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); @@ -4615,8 +4615,8 @@ static int option_parse_p(const struct option *opt, return 0; } -static int 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) @@ -4626,8 +4626,8 @@ static int option_parse_space_change(const struct option *opt, return 0; } -static int 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; @@ -4636,8 +4636,8 @@ static int option_parse_whitespace(const struct option *opt, return 0; } -static int 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); @@ -4755,13 +4755,13 @@ int cmd_apply(int argc, const char **argv, const char *prefix) struct option builtin_apply_options[] = { { OPTION_CALLBACK, 0, "exclude", , N_("path"), N_("don't apply changes matching the given path"), - 0, option_parse_exclude }, + 0, apply_option_parse_exclude }, { OPTION_CALLBACK, 0, "include", , N_("path"), N_("apply changes matching the given path"), - 0, option_parse_include }, + 0, apply_option_parse_include }, { OPTION_CALLBACK, 'p', NULL, , N_("num"), N_("remove leading slashes from traditional diff paths"), - 0, option_parse_p }, + 0, apply_option_parse_p }, OPT_BOOL(0, "no-add", _add, N_("ignore additions made by the patch")), OPT_BOOL(0, "stat", , @@ -4793,13 +4793,13 @@ int cmd_apply(int argc, const char **argv, const char *prefix) N_("ensure at least lines of context match")), { OPTION_CALLBACK, 0, "whitespace", , N_("action"), N_("detect new or modified lines that have whitespace errors"), - 0, option_parse_whitespace }, + 0, apply_option_parse_whitespace }, { OPTION_CALLBACK, 0, "ignore-space-change", , NULL, N_("ignore changes in whitespace when finding context"), - PARSE_OPT_NOARG, option_parse_space_change }, + PARSE_OPT_NOARG, apply_option_parse_space_change }, { OPTION_CALLBACK, 0, "ignore-whitespace", , NULL, N_("ignore changes in whitespace when finding context"), - PARSE_OPT_NOARG,
[PATCH v13 05/14] apply: use error_errno() where possible
To avoid possible mistakes and to uniformly show the errno related messages, let's use error_errno() where possible. Signed-off-by: Christian Couder--- apply.c | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/apply.c b/apply.c index c0cb3f5..41a33d3 100644 --- a/apply.c +++ b/apply.c @@ -3497,7 +3497,7 @@ static int load_current(struct apply_state *state, ce = active_cache[pos]; if (lstat(name, )) { if (errno != ENOENT) - return error(_("%s: %s"), name, strerror(errno)); + return error_errno("%s", name); if (checkout_target(_index, ce, )) return -1; } @@ -3647,7 +3647,7 @@ static int check_preimage(struct apply_state *state, } else if (!state->cached) { stat_ret = lstat(old_name, st); if (stat_ret && errno != ENOENT) - return error(_("%s: %s"), old_name, strerror(errno)); + return error_errno("%s", old_name); } if (state->check_index && !previous) { @@ -3669,7 +3669,7 @@ static int check_preimage(struct apply_state *state, } else if (stat_ret < 0) { if (patch->is_new < 0) goto is_new; - return error(_("%s: %s"), old_name, strerror(errno)); + return error_errno("%s", old_name); } if (!state->cached && !previous) @@ -3728,7 +3728,7 @@ static int check_to_create(struct apply_state *state, return EXISTS_IN_WORKTREE; } else if ((errno != ENOENT) && (errno != ENOTDIR)) { - return error("%s: %s", new_name, strerror(errno)); + return error_errno("%s", new_name); } return 0; } @@ -4247,9 +4247,9 @@ static int add_index_file(struct apply_state *state, if (!state->cached) { if (lstat(path, ) < 0) { free(ce); - return error(_("unable to stat newly " - "created file '%s': %s"), -path, strerror(errno)); + return error_errno(_("unable to stat newly " +"created file '%s'"), + path); } fill_stat_cache_info(ce, ); } @@ -4306,7 +4306,7 @@ static int try_create_file(const char *path, unsigned int mode, const char *buf, strbuf_release(); if (close(fd) < 0 && !res) - return error(_("closing file '%s': %s"), path, strerror(errno)); + return error_errno(_("closing file '%s'"), path); return res ? -1 : 0; } @@ -4503,7 +4503,7 @@ static int write_out_one_reject(struct apply_state *state, struct patch *patch) rej = fopen(namebuf, "w"); if (!rej) - return error(_("cannot open %s: %s"), namebuf, strerror(errno)); + return error_errno(_("cannot open %s"), namebuf); /* Normal git tools never deal with .rej, so do not pretend * this is a git patch by saying --git or giving extended -- 2.9.2.770.g14ff7d2 -- 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 v13 06/14] apply: make it possible to silently apply
This changes 'int apply_verbosely' into 'enum apply_verbosity', and changes the possible values of the variable from a bool to a tristate. The previous 'false' state is changed into 'verbosity_normal'. The previous 'true' state is changed into 'verbosity_verbose'. The new added state is 'verbosity_silent'. It should prevent anything to be printed on both stderr and stdout. This is needed because `git am` wants to first call apply functionality silently, if it can then fall back on 3-way merge in case of error. Printing on stdout, and calls to warning() or error() are not taken care of in this patch, as that will be done in following patches. Signed-off-by: Christian Couder--- apply.c | 62 + apply.h | 8 +++- builtin/apply.c | 2 +- 3 files changed, 48 insertions(+), 24 deletions(-) diff --git a/apply.c b/apply.c index 41a33d3..df85cbc 100644 --- a/apply.c +++ b/apply.c @@ -125,8 +125,11 @@ int check_apply_state(struct apply_state *state, int force_apply) return error(_("--3way outside a repository")); state->check_index = 1; } - if (state->apply_with_reject) - state->apply = state->apply_verbosely = 1; + if (state->apply_with_reject) { + state->apply = 1; + if (state->apply_verbosity == verbosity_normal) + state->apply_verbosity = verbosity_verbose; + } if (!force_apply && (state->diffstat || state->numstat || state->summary || state->check || state->fake_ancestor)) state->apply = 0; if (state->check_index && is_not_gitdir) @@ -1620,8 +1623,9 @@ static void record_ws_error(struct apply_state *state, return; err = whitespace_error_string(result); - fprintf(stderr, "%s:%d: %s.\n%.*s\n", - state->patch_input_file, linenr, err, len, line); + if (state->apply_verbosity > verbosity_silent) + fprintf(stderr, "%s:%d: %s.\n%.*s\n", + state->patch_input_file, linenr, err, len, line); free(err); } @@ -1816,7 +1820,7 @@ static int parse_single_patch(struct apply_state *state, return error(_("new file %s depends on old contents"), patch->new_name); if (0 < patch->is_delete && newlines) return error(_("deleted file %s still has contents"), patch->old_name); - if (!patch->is_delete && !newlines && context) + if (!patch->is_delete && !newlines && context && state->apply_verbosity > verbosity_silent) fprintf_ln(stderr, _("** warning: " "file %s becomes empty but is not deleted"), @@ -2911,7 +2915,7 @@ static int apply_one_fragment(struct apply_state *state, /* Ignore it, we already handled it */ break; default: - if (state->apply_verbosely) + if (state->apply_verbosity > verbosity_normal) error(_("invalid start of line: '%c'"), first); applied_pos = -1; goto out; @@ -3026,7 +3030,7 @@ static int apply_one_fragment(struct apply_state *state, state->apply = 0; } - if (state->apply_verbosely && applied_pos != pos) { + if (state->apply_verbosity > verbosity_normal && applied_pos != pos) { int offset = applied_pos - pos; if (state->apply_in_reverse) offset = 0 - offset; @@ -3041,14 +3045,14 @@ static int apply_one_fragment(struct apply_state *state, * Warn if it was necessary to reduce the number * of context lines. */ - if ((leading != frag->leading) || - (trailing != frag->trailing)) + if ((leading != frag->leading || +trailing != frag->trailing) && state->apply_verbosity > verbosity_silent) fprintf_ln(stderr, _("Context reduced to (%ld/%ld)" " to apply fragment at %d"), leading, trailing, applied_pos+1); update_image(state, img, applied_pos, , ); } else { - if (state->apply_verbosely) + if (state->apply_verbosity > verbosity_normal) error(_("while searching for:\n%.*s"), (int)(old - oldlines), oldlines); } @@ -3539,7 +3543,8 @@ static int try_threeway(struct apply_state *state, read_blob_object(, pre_sha1, patch->old_mode)) return error("repository lacks the necessary blob to fall back on 3-way merge."); -
[PATCH v13 14/14] builtin/am: use apply API in run_apply()
This replaces run_apply() implementation with a new one that uses the apply API that has been previously prepared in apply.c and apply.h. This shoud improve performance a lot in certain cases. As the previous implementation was creating a new `git apply` process to apply each patch, it could be slow on systems like Windows where it is costly to create new processes. Also the new `git apply` process had to read the index from disk, and when the process was done the calling process discarded its own index and read back from disk the new index that had been created by the `git apply` process. This could be very inefficient with big repositories that have big index files, especially when the system decided that it was a good idea to run the `git apply` processes on a different processor core. Also eliminating index reads enables further performance improvements by using: `git update-index --split-index` For example here is a benchmark of a multi hundred commit rebase on the Linux kernel on a Debian laptop with SSD: command: git rebase --onto 1993b17 52bef0c 29dde7c Vanilla "next" without split index:1m54.953s Vanilla "next" with split index: 1m22.476s This series on top of "next" without split index: 1m12.034s This series on top of "next" with split index: 0m15.678s (using branch "next" from mid April 2016.) Benchmarked-by: Ævar Arnfjörð BjarmasonSigned-off-by: Christian Couder --- builtin/am.c | 65 1 file changed, 43 insertions(+), 22 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index 739b34d..0e5d384 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -28,6 +28,7 @@ #include "rerere.h" #include "prompt.h" #include "mailinfo.h" +#include "apply.h" /** * Returns 1 if the file is empty or does not exist, 0 otherwise. @@ -1522,39 +1523,59 @@ static int parse_mail_rebase(struct am_state *state, const char *mail) */ static int run_apply(const struct am_state *state, const char *index_file) { - struct child_process cp = CHILD_PROCESS_INIT; - - cp.git_cmd = 1; - - if (index_file) - argv_array_pushf(_array, "GIT_INDEX_FILE=%s", index_file); + struct argv_array apply_paths = ARGV_ARRAY_INIT; + struct argv_array apply_opts = ARGV_ARRAY_INIT; + struct apply_state apply_state; + int res, opts_left; + static struct lock_file lock_file; + int force_apply = 0; + int options = 0; + + if (init_apply_state(_state, NULL, _file)) + die("BUG: init_apply_state() failed"); + + argv_array_push(_opts, "apply"); + argv_array_pushv(_opts, state->git_apply_opts.argv); + + opts_left = apply_parse_options(apply_opts.argc, apply_opts.argv, + _state, _apply, , + NULL); + + if (opts_left != 0) + die("unknown option passed thru to git apply"); + + if (index_file) { + apply_state.index_file = index_file; + apply_state.cached = 1; + } else + apply_state.check_index = 1; /* * If we are allowed to fall back on 3-way merge, don't give false * errors during the initial attempt. */ - if (state->threeway && !index_file) { - cp.no_stdout = 1; - cp.no_stderr = 1; - } + if (state->threeway && !index_file) + apply_state.apply_verbosity = verbosity_silent; - argv_array_push(, "apply"); + if (check_apply_state(_state, force_apply)) + die("BUG: check_apply_state() failed"); - argv_array_pushv(, state->git_apply_opts.argv); + argv_array_push(_paths, am_path(state, "patch")); - if (index_file) - argv_array_push(, "--cached"); - else - argv_array_push(, "--index"); + res = apply_all_patches(_state, apply_paths.argc, apply_paths.argv, options); - argv_array_push(, am_path(state, "patch")); + argv_array_clear(_paths); + argv_array_clear(_opts); + clear_apply_state(_state); - if (run_command()) - return -1; + if (res) + return res; - /* Reload index as git-apply will have modified it. */ - discard_cache(); - read_cache_from(index_file ? index_file : get_index_file()); + if (index_file) { + /* Reload index as apply_all_patches() will have modified it. */ + discard_cache(); + read_cache_from(index_file); + } return 0; } -- 2.9.2.770.g14ff7d2 -- 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 v13 13/14] apply: learn to use a different index file
Sometimes we want to apply in a different index file. Before the apply functionality was libified it was possible to use the GIT_INDEX_FILE environment variable, for this purpose. But now, as the apply functionality has been libified, it should be possible to do that in a libified way. Signed-off-by: Christian Couder--- apply.c | 27 +-- apply.h | 1 + 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/apply.c b/apply.c index 276e4af..5cd037d 100644 --- a/apply.c +++ b/apply.c @@ -3993,12 +3993,21 @@ static int check_patch_list(struct apply_state *state, struct patch *patch) return err; } +static int read_apply_cache(struct apply_state *state) +{ + if (state->index_file) + return read_cache_from(state->index_file); + else + return read_cache(); +} + /* This function tries to read the sha1 from the current index */ -static int get_current_sha1(const char *path, unsigned char *sha1) +static int get_current_sha1(struct apply_state *state, const char *path, + unsigned char *sha1) { int pos; - if (read_cache() < 0) + if (read_apply_cache(state) < 0) return -1; pos = cache_name_pos(path, strlen(path)); if (pos < 0) @@ -4071,7 +4080,7 @@ static int build_fake_ancestor(struct apply_state *state, struct patch *list) ; /* ok */ } else if (!patch->lines_added && !patch->lines_deleted) { /* mode-only change: update the current */ - if (get_current_sha1(patch->old_name, sha1)) + if (get_current_sha1(state, patch->old_name, sha1)) return error("mode change for %s, which is not " "in current HEAD", name); } else @@ -4675,10 +4684,16 @@ static int apply_patch(struct apply_state *state, state->apply = 0; state->update_index = state->check_index && state->apply; - if (state->update_index && state->newfd < 0) - state->newfd = hold_locked_index(state->lock_file, 1); + if (state->update_index && state->newfd < 0) { + if (state->index_file) + state->newfd = hold_lock_file_for_update(state->lock_file, + state->index_file, + LOCK_DIE_ON_ERROR); + else + state->newfd = hold_locked_index(state->lock_file, 1); + } - if (state->check_index && read_cache() < 0) { + if (state->check_index && read_apply_cache(state) < 0) { error(_("unable to read index file")); res = -128; goto end; diff --git a/apply.h b/apply.h index e2b89e8..1ba4f8d 100644 --- a/apply.h +++ b/apply.h @@ -63,6 +63,7 @@ struct apply_state { int unsafe_paths; /* Other non boolean parameters */ + const char *index_file; enum apply_verbosity apply_verbosity; const char *fake_ancestor; const char *patch_input_file; -- 2.9.2.770.g14ff7d2 -- 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 v13 12/14] apply: pass apply state to build_fake_ancestor()
To libify git apply functionality, we will need to read from a different index file in get_current_sha1(). This index file will be stored in "struct apply_state", so let's pass the state to build_fake_ancestor() which will later pass it to get_current_sha1(). Signed-off-by: Christian Couder--- apply.c | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/apply.c b/apply.c index 2ec2a8a..276e4af 100644 --- a/apply.c +++ b/apply.c @@ -4042,7 +4042,7 @@ static int preimage_sha1_in_gitlink_patch(struct patch *p, unsigned char sha1[20 } /* Build an index that contains the just the files needed for a 3way merge */ -static int build_fake_ancestor(struct patch *list, const char *filename) +static int build_fake_ancestor(struct apply_state *state, struct patch *list) { struct patch *patch; struct index_state result = { NULL }; @@ -4089,12 +4089,13 @@ static int build_fake_ancestor(struct patch *list, const char *filename) } } - hold_lock_file_for_update(, filename, LOCK_DIE_ON_ERROR); + hold_lock_file_for_update(, state->fake_ancestor, LOCK_DIE_ON_ERROR); res = write_locked_index(, , COMMIT_LOCK); discard_index(); if (res) - return error("Could not write temporary index to %s", filename); + return error("Could not write temporary index to %s", +state->fake_ancestor); return 0; } @@ -4709,7 +4710,7 @@ static int apply_patch(struct apply_state *state, } if (state->fake_ancestor && - build_fake_ancestor(list, state->fake_ancestor)) { + build_fake_ancestor(state, list)) { res = -128; goto end; } -- 2.9.2.770.g14ff7d2 -- 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 v13 09/14] usage: add get_error_routine() and get_warn_routine()
Let's make it possible to get the current error_routine and warn_routine, so that we can store them before using set_error_routine() or set_warn_routine() to use new ones. This way we will be able put back the original routines, when we are done with using new ones. Signed-off-by: Christian Couder--- git-compat-util.h | 2 ++ usage.c | 10 ++ 2 files changed, 12 insertions(+) diff --git a/git-compat-util.h b/git-compat-util.h index c7a51f8..de04df1 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -440,7 +440,9 @@ static inline int const_error(void) extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params)); extern void set_error_routine(void (*routine)(const char *err, va_list params)); +extern void (*get_error_routine(void))(const char *err, va_list params); extern void set_warn_routine(void (*routine)(const char *warn, va_list params)); +extern void (*get_warn_routine(void))(const char *warn, va_list params); extern void set_die_is_recursing_routine(int (*routine)(void)); extern void set_error_handle(FILE *); diff --git a/usage.c b/usage.c index 67e5526..2fd3045 100644 --- a/usage.c +++ b/usage.c @@ -70,11 +70,21 @@ void set_error_routine(void (*routine)(const char *err, va_list params)) error_routine = routine; } +void (*get_error_routine(void))(const char *err, va_list params) +{ + return error_routine; +} + void set_warn_routine(void (*routine)(const char *warn, va_list params)) { warn_routine = routine; } +void (*get_warn_routine(void))(const char *warn, va_list params) +{ + return warn_routine; +} + void set_die_is_recursing_routine(int (*routine)(void)) { die_is_recursing = routine; -- 2.9.2.770.g14ff7d2 -- 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 v13 04/14] apply: make some parsing functions static again
Some parsing functions that were used in both "apply.c" and "builtin/apply.c" are now only used in the former, so they can be made static to "apply.c". Signed-off-by: Christian Couder--- apply.c | 6 +++--- apply.h | 5 - 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/apply.c b/apply.c index 7b96130..c0cb3f5 100644 --- a/apply.c +++ b/apply.c @@ -27,7 +27,7 @@ static void git_apply_config(void) git_config(git_default_config, NULL); } -int parse_whitespace_option(struct apply_state *state, const char *option) +static int parse_whitespace_option(struct apply_state *state, const char *option) { if (!option) { state->ws_error_action = warn_on_ws_error; @@ -57,8 +57,8 @@ int parse_whitespace_option(struct apply_state *state, const char *option) return error(_("unrecognized whitespace option '%s'"), option); } -int parse_ignorewhitespace_option(struct apply_state *state, - const char *option) +static int parse_ignorewhitespace_option(struct apply_state *state, +const char *option) { if (!option || !strcmp(option, "no") || !strcmp(option, "false") || !strcmp(option, "never") || diff --git a/apply.h b/apply.h index 5ec022c..df44b51 100644 --- a/apply.h +++ b/apply.h @@ -97,11 +97,6 @@ struct apply_state { int applied_after_fixing_ws; }; -extern int parse_whitespace_option(struct apply_state *state, - const char *option); -extern int parse_ignorewhitespace_option(struct apply_state *state, -const char *option); - 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, -- 2.9.2.770.g14ff7d2 -- 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 v13 02/14] apply: rename and move opt constants to apply.h
The constants for the "inaccurate-eof" and the "recount" options will be used in both "apply.c" and "builtin/apply.c", so they need to go into "apply.h", and therefore they need a name that is more specific to the API they belong to. Signed-off-by: Christian Couder--- apply.h | 3 +++ builtin/apply.c | 11 --- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/apply.h b/apply.h index 53f09b5..ca1dcee 100644 --- a/apply.h +++ b/apply.h @@ -108,4 +108,7 @@ extern int init_apply_state(struct apply_state *state, extern void clear_apply_state(struct apply_state *state); extern int check_apply_state(struct apply_state *state, int force_apply); +#define APPLY_OPT_INACCURATE_EOF (1<<0) +#define APPLY_OPT_RECOUNT (1<<1) + #endif diff --git a/builtin/apply.c b/builtin/apply.c index 429fe44..9c396bb 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -4463,9 +4463,6 @@ static int write_out_results(struct apply_state *state, struct patch *list) static struct lock_file lock_file; -#define INACCURATE_EOF (1<<0) -#define RECOUNT(1<<1) - /* * Try to apply a patch. * @@ -4495,8 +4492,8 @@ static int apply_patch(struct apply_state *state, int nr; patch = xcalloc(1, sizeof(*patch)); - patch->inaccurate_eof = !!(options & INACCURATE_EOF); - patch->recount = !!(options & RECOUNT); + patch->inaccurate_eof = !!(options & APPLY_OPT_INACCURATE_EOF); + patch->recount = !!(options & APPLY_OPT_RECOUNT); nr = parse_chunk(state, buf.buf + offset, buf.len - offset, patch); if (nr < 0) { free_patch(patch); @@ -4811,10 +4808,10 @@ int cmd_apply(int argc, const char **argv, const char *prefix) OPT__VERBOSE(_verbosely, N_("be verbose")), OPT_BIT(0, "inaccurate-eof", , N_("tolerate incorrectly detected missing new-line at the end of file"), - INACCURATE_EOF), + APPLY_OPT_INACCURATE_EOF), OPT_BIT(0, "recount", , N_("do not trust the line counts in the hunk headers"), - RECOUNT), + APPLY_OPT_RECOUNT), { OPTION_CALLBACK, 0, "directory", , N_("root"), N_("prepend to all filenames"), 0, apply_option_parse_directory }, -- 2.9.2.770.g14ff7d2 -- 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 v13 08/14] usage: add set_warn_routine()
There are already set_die_routine() and set_error_routine(), so let's add set_warn_routine() as this will be needed in a following commit. Signed-off-by: Christian Couder--- git-compat-util.h | 1 + usage.c | 5 + 2 files changed, 6 insertions(+) diff --git a/git-compat-util.h b/git-compat-util.h index 590bfdd..c7a51f8 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -440,6 +440,7 @@ static inline int const_error(void) extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params)); extern void set_error_routine(void (*routine)(const char *err, va_list params)); +extern void set_warn_routine(void (*routine)(const char *warn, va_list params)); extern void set_die_is_recursing_routine(int (*routine)(void)); extern void set_error_handle(FILE *); diff --git a/usage.c b/usage.c index 1dad03f..67e5526 100644 --- a/usage.c +++ b/usage.c @@ -70,6 +70,11 @@ void set_error_routine(void (*routine)(const char *err, va_list params)) error_routine = routine; } +void set_warn_routine(void (*routine)(const char *warn, va_list params)) +{ + warn_routine = routine; +} + void set_die_is_recursing_routine(int (*routine)(void)) { die_is_recursing = routine; -- 2.9.2.770.g14ff7d2 -- 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 v13 10/14] apply: change error_routine when silent
To avoid printing anything when applying with `state->apply_verbosity == verbosity_silent`, let's save the existing warn and error routines before applying, and let's replace them with a routine that does nothing. Then after applying, let's restore the saved routines. Helped-by: Stefan BellerSigned-off-by: Christian Couder --- apply.c | 21 - apply.h | 8 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/apply.c b/apply.c index ddbb0a2..bf81b70 100644 --- a/apply.c +++ b/apply.c @@ -112,6 +112,11 @@ void clear_apply_state(struct apply_state *state) /* >fn_table is cleared at the end of apply_patch() */ } +static void mute_routine(const char *bla, va_list params) +{ + /* do nothing */ +} + int check_apply_state(struct apply_state *state, int force_apply) { int is_not_gitdir = !startup_info->have_repository; @@ -144,6 +149,13 @@ int check_apply_state(struct apply_state *state, int force_apply) if (!state->lock_file) return error("BUG: state->lock_file should not be NULL"); + if (state->apply_verbosity <= verbosity_silent) { + state->saved_error_routine = get_error_routine(); + state->saved_warn_routine = get_warn_routine(); + set_error_routine(mute_routine); + set_warn_routine(mute_routine); + } + return 0; } @@ -4864,7 +4876,7 @@ int apply_all_patches(struct apply_state *state, state->newfd = -1; } - return !!errs; + res = !!errs; end: if (state->newfd >= 0) { @@ -4872,5 +4884,12 @@ int apply_all_patches(struct apply_state *state, state->newfd = -1; } + if (state->apply_verbosity <= verbosity_silent) { + set_error_routine(state->saved_error_routine); + set_warn_routine(state->saved_warn_routine); + } + + if (res > -1) + return res; return (res == -1 ? 1 : 128); } diff --git a/apply.h b/apply.h index bd4eb6d..5cde641 100644 --- a/apply.h +++ b/apply.h @@ -94,6 +94,14 @@ struct apply_state { */ struct string_list fn_table; + /* +* This is to save reporting routines before using +* set_error_routine() or set_warn_routine() to install muting +* routines when in verbosity_silent mode. +*/ + void (*saved_error_routine)(const char *err, va_list params); + void (*saved_warn_routine)(const char *warn, va_list params); + /* These control whitespace errors */ enum apply_ws_error_action ws_error_action; enum apply_ws_ignore ws_ignore_action; -- 2.9.2.770.g14ff7d2 -- 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 v13 07/14] apply: don't print on stdout in verbosity_silent mode
When apply_verbosity is set to verbosity_silent nothing should be printed on both stderr and stdout. To avoid printing on stdout, we can just skip calling the following functions: - stat_patch_list(), - numstat_patch_list(), - summary_patch_list(). It is safe to do that because the above functions have no side effects other than printing: - stat_patch_list() only computes some local values and then call show_stats() and print_stat_summary(), those two functions only compute local values and call printing functions, - numstat_patch_list() also only computes local values and calls printing functions, - summary_patch_list() calls show_file_mode_name(), printf(), show_rename_copy(), show_mode_change() that are only printing. Signed-off-by: Christian Couder--- apply.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/apply.c b/apply.c index df85cbc..ddbb0a2 100644 --- a/apply.c +++ b/apply.c @@ -4702,13 +4702,13 @@ static int apply_patch(struct apply_state *state, goto end; } - if (state->diffstat) + if (state->diffstat && state->apply_verbosity > verbosity_silent) stat_patch_list(state, list); - if (state->numstat) + if (state->numstat && state->apply_verbosity > verbosity_silent) numstat_patch_list(state, list); - if (state->summary) + if (state->summary && state->apply_verbosity > verbosity_silent) summary_patch_list(list); end: -- 2.9.2.770.g14ff7d2 -- 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
Crash when using git blame on untracked file
Hello, I'm seeing the following crash with Git 2.9.3 on Debian sid (amd64): $ git init foo $ cd foo $ touch x $ git add x $ git commit -m test $ touch x.conf $ git blame x.conf segmentation fault I've tested it on Debian stable's 2.1.4 which works fine: $ git blame x.conf fatal: no such path 'x.conf' in HEAD It requires the blamed file to be present, but in some cases it works only if the file e.g. "x" is already tracked in Git and the other file is called "x.conf" (".conf"-suffix). But in an empty repo it seems to happen always. Sadly Debian's git has no dbg-package, so the stacktrace is not very useful: #0 __strcmp_sse2_unaligned () at ../sysdeps/x86_64/multiarch/strcmp-sse2-unaligned.S:31 #1 0x0041ad7a in ?? () #2 0x00406171 in ?? () #3 0x00405321 in ?? () #4 0x76f9f700 in __libc_start_main (main=0x4051c0, argc=0x3, argv=0x7fffe1a8, init=, fini=, rtld_fini=, stack_end=0x7fffe198) at ../csu/libc-start.c:291 #5 0x004057d9 in ?? () Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 signature.asc Description: PGP signature
[L10N] Kickoff of translation for Git 2.10.0 round 2
Hi, Git v2.10.0-rc2 has been released, and let's start new round of git l10n. This time there are 12 updated messages need lto be translated since last update: l10n: git.pot: v2.10.0 round 2 (12 new, 44 removed) Generate po/git.pot from v2.10.0-rc2 for git v2.10.0 l10n round 2. Signed-off-by: Jiang XinYou can get it from the usual place: https://github.com/git-l10n/git-po/ As how to update your XX.po and help to translate Git, please see "Updating a XX.po file" and other sections in “po/README" file. -- Jiang Xin -- 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: stable as subset of develop
W dniu 27.08.2016 o 15:17, Brett Porter pisze: > On 08/27/2016 03:55 AM, Jakub Narębski wrote: >> W dniu 27.08.2016 o 04:29, Brett Porter pisze: >>> >>> In a small group, develop is the branch where all fixes/additions/... from >>> topic >>> branches are merged rather dynamically. Thorough testing of commits >>> may lag behind, but when we think one is a pretty good commit we want >>> to identify it as (at least relatively) the latest stable. We could >>> tag it, but we would like these stable commits to be a branch in the >>> sense that each commit points back to a previous commit. >>> >>> Merging from a development branch commit to stable isn't quite what >>> we want. It seems more like: >>> >>>checkout the new good development commit >>>change HEAD to the head of the stable branch >>>git add --all >>>git commit >>>(maybe tag the new commit with the hash of the chosen development commit) Actually this is almost exactly equivalent (except for the commit message template) to squash merge (you lose history, that is resulting commit have only one parent), using 'ours' merge strategy (or rather 'theirs' - which does not exist in Git). The visualization of history would look like this: 1<---2<3<4<5<6<7 <--- unstable development branch |\ / / | \--a Oops - used tag in a generic sense when discussing git -- not helpful. > Really - would put the hash of the development commit into the log message for > the stable commit. Well, this does not change my reasoning in any way. >> >> If you are really using topic branches, a better workflow would be >> to make use of them. That is, do the development of new features >> on topic branches, test them in relative isolation, and when deemed >> ready merge them into development branch, for more testing (including >> integration testing). >> >> Then, those topic branches that are considered stable are merged >> into stable branch ("trunk"). You can think of it as picking >> features to have in stable. > > Ok. There are 2 things at play The repository contains code for > an embedded system with several subsystems (separate executables on > separate processors). We will be trying to keep testing schemes up to > date with respect to the progress on the code -- but (beyond > unit/module tests), the scene will change over time; and, users may > not be able to run much form their local copies. If there is problem with developers running tests (or at least running them comprehensively, that is for all architectures / subsystems), why not set up a continuous integration / continuous delivery server, that would run all the tests after push? > Second, only 2 of us > have used git before (and, while trying to get up to speed - I am a > ways from git guru-dom yet), while everyone else has been using > visual sourcesafe for many years. I have not used Visual SourceSafe myself (though I tried to use CVS in times before Git), but I have heard horror stories about it... As to mastering Git - I recommend "Pro Git" (free on-line book), "Git for Teams" (though more about teams than about Git), or my "Mastering Git" book. For beginners, there is "Version Control By Example" (free online version), or "Git: Version Control for Everyone". > > I want others to merge their work into development sooner rather than > later to minimize their and my problem of untangling conflicts (or - > you rebased while you were sharing work with Jill and...). And, > testing on their work may be limited before they push to our main > repository and some integration can be done. What the setup looks like? Is there a central repository where everyone publish their changes, or is there one person responsible for integrating changes from each developer public repository? > > I really want to create a linked list of the development branch > commits that are of interest (their working sets - not the commit > objects themselves), and using the commit object's pointer to its > predecessor seems like it could just do the job. (it wouldn't be the > place to go to for history / useful log
Re: stable as subset of develop
Thank you Jakub for your feedback. On 08/27/2016 03:55 AM, Jakub Narębski wrote: W dniu 27.08.2016 o 04:29, Brett Porter pisze: In a small group, develop is the branch where all fixes/additions/... from topic branches are merged rather dynamically. Thorough testing of commits may lag behind, but when we think one is a pretty good commit we want to identify it as (at least relatively) the latest stable. We could tag it, but we would like these stable commits to be a branch in the sense that each commit points back to a previous commit. Merging from a development branch commit to stable isn't quite what we want. It seems more like: checkout the new good development commit change HEAD to the head of the stable branch git add --all git commit (maybe tag the new commit with the hash of the chosen development commit) Oops - used tag in a generic sense when discussing git -- not helpful. Really - would put the hash of the development commit into the log message for the stable commit. If you are really using topic branches, a better workflow would be to make use of them. That is, do the development of new features on topic branches, test them in relative isolation, and when deemed ready merge them into development branch, for more testing (including integration testing). Then, those topic branches that are considered stable are merged into stable branch ("trunk"). You can think of it as picking features to have in stable. Ok. There are 2 things at play The repository contains code for an embedded system with several subsystems (separate executables on separate processors). We will be trying to keep testing schemes up to date with respect to the progress on the code -- but (beyond unit/module tests), the scene will change over time; and, users may not be able to run much form their local copies. Second, only 2 of us have used git before (and, while trying to get up to speed - I am a ways from git guru-dom yet), while everyone else has been using visual sourcesafe for many years. I want others to merge their work into development sooner rather than later to minimize their and my problem of untangling conflicts (or - you rebased while you were sharing work with Jill and...). And, testing on their work may be limited before they push to our main repository and some integration can be done. I really want to create a linked list of the development branch commits that are of interest (their working sets - not the commit objects themselves), and using the commit object's pointer to its predecessor seems like it could just do the job. (it wouldn't be the place to go to for history / useful log entries) Take a look at Junio's blog posts about this topic. Thanks for that. I will. Will that work (one thing beyond my current understanding is if there are index complications)? Other ideas? That looks a bit like reimplementation of cherry-picking. Maybe I've misunderstood cherry-picking, but I've thought it different from the list view that I've been thinking could help us (with merges and multiple commits). Also, I think you would loose the ability to run git-bisect to find bad commits. Hmmm - I'm imagining a rather sparse stable, with perhaps time-consuming integration testing. This could help with applying successively more intense testing over time and chase down where problems arose. Reiterationg: if you are using topic branches, use topic-branch workflow. -- 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
E-Book zum Thema Geldanlage
Hallo, unter http://www.meinegeldanlage.com/ biete ich ein kostenloses E-Book zum Thema Geldanlage mit umfassenden Informationen zum Download an. Bei den Recherchen für das E-Book bin ich unter anderem auf Ihre Webseite gestoßen. Wären Sie bereit, meine Webseite bzw. das E-Book von Ihrer Webseite aus zu verlinken (z. B. von http://git.661346.n2.nabble.com/nike-air-max-1-Damen-td7592204.html)? (Das E-Book kann übrigens ohne Anmeldung oder ähnliche Hürden einfach im PDF-Format heruntergeladen werden, ohne weitere Verpflichtungen. Das wird auch dauerhaft so bleiben.) Falls das für Sie interessant ist, kann ich Ihnen gerne einen (eigens geschriebenen) Artikel zum Thema zusenden, den Sie auf Ihrer Webseite veröffentlichen können. Wenn Sie möchten, kann ich im Gegenzug Ihre Webseite von einer meiner anderen Webseiten verlinken. Freundliche Grüße, Florian Gerber -- 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: Are --first-parent and --ancestry-path compatible rev-list options?
From: "Junio C Hamano""Philip Oakley" writes: The commit graph. We are looking for F based on knowing J. . A - B - C - D -- E -- F -- G - H<-first parent, --merges (C,F,H) . \ | /\// . Z | // . | | | / . \ \ / / . I -[J]- K - L - M <-since J, children of J .\ / . N - O - P I think these two operations are fundamentally incompatible. If I run them independently, they both find the desired INTERESTED commit, hence the expectation that together they will still find that commit as an intersection between the two sets. Because the first-parent traversal is what the name says, i.e., forbids the positive side of revision traversal to stray into side branches, the positive side of a traversal that begins at H will not see M, L and K. But it does see F the ultimately desired commit. Philip@PhilipOakley MINGW32 /usr/src/test (master) $ git log --oneline --first-parent --merges :/j.. d089efa g ac811d4 f 1db59e5 c Then we have: --ancestry-path Limit the displayed commits to those directly on the ancestry chain between the "from" and "to" commits in the given commit range. So one would expect, from a user viewepoint that this is commit selection, not internal algorithm implementation, that it would only list G and F, and the C would not be displayed. The negative side would traverse in the normal way to paint commits starting J and its ancestors as UNINTERESTING, so the traversal over the artificial "first-parent only" history, i.e. H, G, F, E, D, C, B, A would eventually stop before showing an ancestor of J. On the other hand, to compute the ancestry-path, you need to make a full traversal of the real history to find a subgraph J..H and then post-process it to cull paths that do not contain J. The culling order isn't clear. It's easy to expect that --first parent is a cull. The documentation implies (to me) this different computation. This explains why its happening, but it does feel rather contrary to user (rather than developer) expectation. It's the confusion between traversal limiting and marking a commit as Interesting (i.e. UNINTERSTING has two meanings which may not align when multiple options are given.) It does seem odd that in the Git world, with its feature branch approach, that this hasn't come up before. I know that I haven't even bothered to try this 'search', and just use gitk to manually follow the breadcrumbs. Perhaps a `--first-parent-ancestor` option to determine the places where a commit from a feature series was merged in to the mainline would be rather helpful. -- Philip -- 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 v14 07/27] bisect--helper: `bisect_reset` shell function in C
Hey Junio, On Fri, Aug 26, 2016 at 9:59 PM, Junio C Hamanowrote: > Pranit Bauva writes: > >>> Also this version fails to catch "bisect reset a b c" as an error, I >>> suspect. >> >> It didn't when I tried it right now. Could you please elaborate on why >> you think it can fail? There might be a thing which I haven't tested. > > My bad. I just compared your bisect_reset() implementation that had > > if (no specific commit) { > reset to the branch > } else { > reset to the commit > } > > with the original > > case $# in > 0) reset to the branch ;; > 1) reset to the commit ;; > *) give usage and die ;; > esac > > and took the difference and reacted "ah, excess parameters are not > diagnosed in this function". > > Your caller does complain about excess parameters without giving > usage, and that is what I missed. > > I am not sure if you intended to change the behaviour in this case > to avoid giving the usage string; I tend to think it is a good > change, but I didn't see it mentioned in the proposed commit log, > which also contributed to my not noticing the test in the caller. I could include this in the commit message. Its not really something which we would want to test in the function because to the function, we are not passing the raw arguments. Since we are removing that check from the function but including it in cmd_bisect__helper(), I will talk about it in the commit message. Regards, Pranit Bauva -- 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] make dist: allow using an installed version of git
b1de9de2 back in 2005 ensured that we could create a tarball with 'make dist' even if git wasn't installed yet. These days however, chances are higher that a git version is available. Add a config.mak knob to allow people to choose to use the installed version of git to create the tarball and avoid the overhead of building git-archive. Signed-off-by: Dennis Kaarsemaker--- Makefile | 14 -- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index d96ecb7..3dabb75 100644 --- a/Makefile +++ b/Makefile @@ -378,6 +378,9 @@ all:: # # to say "export LESS=FRX (and LV=-c) if the environment variable # LESS (and LV) is not set, respectively". +# +# Define USE_INSTALLED_GIT_ARCHIVE if you don't want to build git-archive as +# part of 'make dist', but are happy to rely on a git version on you $PATH GIT-VERSION-FILE: FORCE @$(SHELL_PATH) ./GIT-VERSION-GEN @@ -2423,8 +2426,15 @@ quick-install-html: ### Maintainer's dist rules GIT_TARNAME = git-$(GIT_VERSION) -dist: git-archive$(X) configure - ./git-archive --format=tar \ +ifndef USE_INSTALLED_GIT_ARCHIVE + GIT_ARCHIVE = ./git-archive$(X) + GIT_ARCHIVE_DEP = git-archive$(X) +else + GIT_ARCHIVE = git archive + GIT_ARCHIVE_DEP = +endif +dist: $(GIT_ARCHIVE_DEP) configure + $(GIT_ARCHIVE) --format=tar \ --prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar @mkdir -p $(GIT_TARNAME) @cp configure $(GIT_TARNAME) -- 2.10.0-rc1-230-g8efea0f -- 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 v14 12/27] bisect--helper: `get_terms` & `bisect_terms` shell function in C
Hey Junio, On Thu, Aug 25, 2016 at 11:35 PM, Junio C Hamanowrote: > Pranit Bauva writes: > >> +static int bisect_terms(struct bisect_terms *terms, const char **argv, int >> argc) >> +{ >> + int i; >> + >> + if (get_terms(terms)) { >> + fprintf(stderr, _("no terms defined\n")); >> + return -1; >> + } >> + if (argc == 0) { >> + 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; >> + } >> + >> + for (i = 0; i < argc; i++) { >> + if (!strcmp(argv[i], "--term-good")) >> + printf("%s\n", terms->term_good.buf); >> + else if (!strcmp(argv[i], "--term-bad")) >> + printf("%s\n", terms->term_bad.buf); >> + else >> + printf(_("invalid argument %s for 'git bisect " >> + "terms'.\nSupported options are: " >> + "--term-good|--term-old and " >> + "--term-bad|--term-new."), argv[i]); >> + } > > The original took only one and gave one answer (and errored out when > the user asked for more), but this one loops. I can see either way > is OK and do not think of a good reason to favor one over the other; > unless there is a strong reason why you need this extended behaviour > that allows users to ask multiple questions, I'd say we should keep > the original behaviour. True! I can just use return error() instead of printf. Also I noticed that this is printing to stdout while the original printed it to stderr. Thanks! Regards, Pranit Bauva -- 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 v14 11/27] bisect--helper: `bisect_next_check` & bisect_voc shell function in C
Hey Junio, On Thu, Aug 25, 2016 at 4:10 AM, Junio C Hamanowrote: > Pranit Bauva writes: > >> +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 1; >> +} >> + >> +static char *bisect_voc(char *revision_type) >> +{ >> + if (!strcmp(revision_type, "bad")) >> + return "bad|new"; >> + if (!strcmp(revision_type, "good")) >> + return "good|old"; >> + >> + return NULL; >> +} >> + >> +static int bisect_next_check(const struct bisect_terms *terms, >> + const char *current_term) >> +{ >> + int missing_good = 1, missing_bad = 1; >> + char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad.buf); >> + char *good_glob = xstrfmt("%s-*", terms->term_good.buf); >> + char *bad_syn, *good_syn; >> + >> + 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); >> + >> + 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); >> + 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; >> + } >> + bad_syn = xstrdup(bisect_voc("bad")); >> + good_syn = xstrdup(bisect_voc("good")); >> + if (!is_empty_or_missing_file(git_path_bisect_start())) { >> + error(_("You need to give me at least one %s and " >> + "%s revision. You can use \"git bisect %s\" " >> + "and \"git bisect %s\" for that. \n"), >> + bad_syn, good_syn, bad_syn, good_syn); >> + free(bad_syn); >> + free(good_syn); >> + return -1; >> + } >> + else { >> + error(_("You need to start by \"git bisect start\". You " >> + "then need to give me at least one %s and %s " >> + "revision. You can use \"git bisect %s\" and " >> + "\"git bisect %s\" for that.\n"), >> + good_syn, bad_syn, bad_syn, good_syn); >> + free(bad_syn); >> + free(good_syn); >> + return -1; >> + } >> + free(bad_syn); >> + free(good_syn); >> + >> + return 0; > > This one looks OK, but I think the same "Wouldn't it become cleaner > to have a 'finish:' label at the end and jump there?" comment > applies to this implementation, too. For this goto can simply things. Will do! Regards, Pranit Bauva -- 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 v14 09/27] bisect--helper: `bisect_write` shell function in C
Hey Junio, On Thu, Aug 25, 2016 at 4:00 AM, Junio C Hamanowrote: > Pranit Bauva writes: > >> +struct bisect_terms { >> + struct strbuf term_good; >> + struct strbuf term_bad; >> +}; > > I think "struct strbuf" is overrated. For things like this, where > these fields will never change once it is set (and setting it is > done atomically, not incrementally), there is no good reason to use > define the fields as strbuf. > > Only because you chose to use strbuf for these two fields, you have > to make unnecessarily copies of argv[] in the command parser, and > you have to remember to discard these copies later. > > I think you can just say "const char *" in this case. Using struct strbuf is not really overrated but in fact required. But yes, for this patch it might seem as overrated. In the shell code initally TERM_GOOD is set to "good" while TERM_BAD is set to "bad". Now there are a lot of instances (one of which is bisect_start() function) where this can change. So if we keep it as "const char *", it would be right to change the value of it after wards. And we cannot keep it as "char []" because we don't know its size before hand. >> +static int bisect_write(const char *state, const char *rev, >> + const struct bisect_terms *terms, int nolog) >> +{ >> + struct strbuf tag = STRBUF_INIT; >> + struct strbuf commit_name = STRBUF_INIT; >> + struct object_id oid; >> + struct commit *commit; >> + struct pretty_print_context pp = {0}; >> + FILE *fp; >> + >> + if (!strcmp(state, terms->term_bad.buf)) >> + strbuf_addf(, "refs/bisect/%s", state); >> + else if (one_of(state, terms->term_good.buf, "skip", NULL)) >> + strbuf_addf(, "refs/bisect/%s-%s", state, rev); >> + else >> + return error(_("Bad bisect_write argument: %s"), state); > > OK. > >> + if (get_oid(rev, )) { >> + strbuf_release(); >> + return error(_("couldn't get the oid of the rev '%s'"), rev); >> + } >> + >> + if (update_ref(NULL, tag.buf, oid.hash, NULL, 0, >> +UPDATE_REFS_MSG_ON_ERR)) { >> + strbuf_release(); >> + return -1; >> + } >> + strbuf_release(); >> + >> + fp = fopen(git_path_bisect_log(), "a"); >> + if (!fp) >> + return error_errno(_("couldn't open the file '%s'"), >> git_path_bisect_log()); >> + >> + commit = lookup_commit_reference(oid.hash); >> + format_commit_message(commit, "%s", _name, ); >> + fprintf(fp, "# %s: [%s] %s\n", state, sha1_to_hex(oid.hash), >> + commit_name.buf); >> + strbuf_release(_name); >> + >> + if (!nolog) >> + fprintf(fp, "git bisect %s %s\n", state, rev); >> + >> + fclose(fp); >> + return 0; > > You seem to be _release()ing tag all over the place. > > Would it make sense to have a single clean-up label at the end of > function, introduce a "int retval" variable and set it to -1 (or > whatever) when an error is detected and "goto" to the label? It may > make it harder to make such a leak. That is, to end the function > more like: I think I could use goto for this function. > finish: > if (fp) > fclose(fp); > strbuf_release(); > strbuf_release(_name); > return retval; > } > > and have sites with potential errors do something like this: > > if (update_ref(...)) { > retval = -1; > goto finish; > } > >> + struct bisect_terms terms; >> + bisect_terms_init(); > > With the type of "struct bisect_terms" members corrected, you do not > even need the _init() function. Discussed above. >> @@ -182,24 +251,38 @@ int cmd_bisect__helper(int argc, const char **argv, >> const char *prefix) >> usage_with_options(git_bisect_helper_usage, options); >> >> switch (cmdmode) { >> + int nolog; >> case NEXT_ALL: >> return bisect_next_all(prefix, no_checkout); >> case WRITE_TERMS: >> if (argc != 2) >> die(_("--write-terms requires two arguments")); >> - return write_terms(argv[0], argv[1]); >> + res = write_terms(argv[0], argv[1]); >> + break; >> case BISECT_CLEAN_STATE: >> if (argc != 0) >> die(_("--bisect-clean-state requires no arguments")); >> - return bisect_clean_state(); >> + res = bisect_clean_state(); >> + break; >> case BISECT_RESET: >> if (argc > 1) >> die(_("--bisect-reset requires either zero or one >> arguments")); >> - return bisect_reset(argc ? argv[0] : NULL); >> + res = bisect_reset(argc ? argv[0] : NULL); >> + break; >> case CHECK_EXPECTED_REVS: >> - return
Re: [PATCH v14 08/27] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
Hey Junio, On Thu, Aug 25, 2016 at 3:43 AM, Junio C Hamanowrote: > Pranit Bauva writes: > >> +static int is_expected_rev(const char *expected_hex) >> +{ >> + struct strbuf actual_hex = STRBUF_INIT; >> + int res = 0; >> + if (strbuf_read_file(_hex, git_path_bisect_expected_rev(), 0) >> >= 0) { >> + strbuf_trim(_hex); >> + res = !strcmp(actual_hex.buf, expected_hex); > > If it is known to have 40-hex: > > (1) accepting ">= 0" seems way too lenient. You only expect a > 41-byte file (or 42 if somebody would write CRLF, but I do not > think anybody other than yourself is expected to write into > this file, and you do not write CRLF yourself); > > (2) strbuf_trim() is overly loose. You only want to trim the > terimnating LF and it is an error to have other trailing > whitespaces. > > I think the latter is not a new problem and it is OK to leave it > as-is; limiting (1) to >= 40 may still be a good change, though, > because it makes the intention of the code clearer. I can do the first change. Since this wasn't present in the shell code, I will also mention it in the commit message. Regards, Pranit Bauva -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Documentation/SubmittingPatches: add quotes to advised commit reference
W dniu 27.08.2016 o 00:42, Junio C Hamanopisze: > Stefan Beller writes: > -- >8 -- > From: Beat Bolli ??? -- 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: stable as subset of develop
W dniu 27.08.2016 o 04:29, Brett Porter pisze: > > In a small group, develop is the branch where all fixes/additions/... from > topic > branches are merged rather dynamically. Thorough testing of commits > may lag behind, but when we think one is a pretty good commit we want > to identify it as (at least relatively) the latest stable. We could > tag it, but we would like these stable commits to be a branch in the > sense that each commit points back to a previous commit. > > Merging from a development branch commit to stable isn't quite what > we want. It seems more like: > > checkout the new good development commit > change HEAD to the head of the stable branch > git add --all > git commit > (maybe tag the new commit with the hash of the chosen development commit) If you are really using topic branches, a better workflow would be to make use of them. That is, do the development of new features on topic branches, test them in relative isolation, and when deemed ready merge them into development branch, for more testing (including integration testing). Then, those topic branches that are considered stable are merged into stable branch ("trunk"). You can think of it as picking features to have in stable. Take a look at Junio's blog posts about this topic. > Will that work (one thing beyond my current understanding is if there > are index complications)? Other ideas? That looks a bit like reimplementation of cherry-picking. Also, I think you would loose the ability to run git-bisect to find bad commits. > This could help with applying successively more intense testing over > time and chase down where problems arose. Reiterationg: if you are using topic branches, use topic-branch workflow. -- Jakub Narębski author of "Mastering Git" https://www.packtpub.com/application-development/mastering-git -- 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] gitk: align the commit summary format to the documentation
Am 26.08.2016 um 20:24 schrieb Junio C Hamano: Beat Bolliwrites: In 175d38c ("SubmittingPatches: document how to reference previous commits", 2016-07-28) the format for referring to older commits was specified. is easier to read when pasted into a sentence than what the recent update 175d38ca ("SubmittingPatches: document how to reference previous commits", 2016-07-28) suggests to do, i.e. While it may be easier to read due to the extra mark-up, the resulting text where such a quotation appears does not flow well, IMO. A commit message text that references another commit reads more fluently without the quotes around the summary line because the quoted text is not so much a quotation that must be marked, but a parenthetical statement. I absolutely welcome the proposed change to gitk, because I always edit out the double-quotes. -- 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