Re: [PATCH v2 1/4] am: fail if no author line is given in --rebasing mode
Hi, just to make it also clear in this thread: you can ignore this patch series in favor of the better patch series by Peff [1] that has found its way to the mailing list at the same time. 1. https://public-inbox.org/git/20190905224859.ga28...@sigill.intra.peff.net/ Stephan
Re: [PATCH] Fix maybe-uninitialized warnings found by gcc 9 -flto
On 9/6/19 12:58 AM, Jeff King wrote: > On Fri, Sep 06, 2019 at 12:53:49AM +0200, Stephan Beyer wrote: > >> On 9/6/19 12:48 AM, Jeff King wrote: >>> On Thu, Sep 05, 2019 at 10:24:59AM +0200, Stephan Beyer wrote: >>> >>>> Compiler heuristics for detection of potentially uninitialized variables >>>> may change between compiler versions and enabling link-time optimization >>>> may find new warnings. Indeed, compiling with gcc 9.2.1 and enabled >>>> link-time optimization feature resulted in a few hits that are fixed by >>>> this patch in the most naïve way. This allows to compile git using the >>>> DEVELOPER=1 switch (which sets -Werror) and using the -flto flag. >>> >>> Lots of discussion in this thread. Let's try to turn it into some >>> patches. :) >> >> I thought the same and just sent my version of it. :D > > Yeah, I see that our mails crossed. :) I like some of my versions > better, but I'm OK with either (or a mix-and-match). I took a quick glance at yours. I also noticed the issue you address in [PATCH 2/6], but I was unsure if this is the way to go (I'm only occasionally reading on this list). I would prefer your patch series, with maybe one exception... The thing is: I had *exactly* the same commit like your [PATCH 6/6] (except for the commit message and for the number), but I dropped it. Why? Because I had the feeling (no particular instance though) that the second locate_object_entry_hash() for each insertion *can* indeed take "too much" time. Also, I was wondering, if the "found = 1" case should be catched as a BUG("should not happen") or something. I don't care much, though. The performance impact should probably be checked carefully. Stephan
Re: [PATCH] Fix maybe-uninitialized warnings found by gcc 9 -flto
On 9/6/19 12:48 AM, Jeff King wrote: > On Thu, Sep 05, 2019 at 10:24:59AM +0200, Stephan Beyer wrote: > >> Compiler heuristics for detection of potentially uninitialized variables >> may change between compiler versions and enabling link-time optimization >> may find new warnings. Indeed, compiling with gcc 9.2.1 and enabled >> link-time optimization feature resulted in a few hits that are fixed by >> this patch in the most naïve way. This allows to compile git using the >> DEVELOPER=1 switch (which sets -Werror) and using the -flto flag. > > Lots of discussion in this thread. Let's try to turn it into some > patches. :) I thought the same and just sent my version of it. :D Stephan
[PATCH v2 2/4] test-read-cache: fix maybe-uninitialized warning for namelen
This is done by removing namelen at all. It is only used once and simply strlen(name), hence we use strlen(name) directly. Suggested-by: Jeff King Signed-off-by: Stephan Beyer --- t/helper/test-read-cache.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c index 7e79b555de..244977a29b 100644 --- a/t/helper/test-read-cache.c +++ b/t/helper/test-read-cache.c @@ -4,11 +4,10 @@ int cmd__read_cache(int argc, const char **argv) { - int i, cnt = 1, namelen; + int i, cnt = 1; const char *name = NULL; if (argc > 1 && skip_prefix(argv[1], "--print-and-refresh=", &name)) { - namelen = strlen(name); argc--; argv++; } @@ -24,7 +23,7 @@ int cmd__read_cache(int argc, const char **argv) refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL); - pos = index_name_pos(&the_index, name, namelen); + pos = index_name_pos(&the_index, name, strlen(name)); if (pos < 0) die("%s not in index", name); printf("%s is%s up to date\n", name, -- 2.23.0.43.g31ebfd7ae6.dirty
[PATCH v2 3/4] pack-objects: fix maybe-uninitialized warning for index_pos
gcc 9.2.1 with -flto shows a maybe-uninitialized warning for index_pos in builtin/pack-objects.c's add_object_entry(). Tracking it down, the variable should be initialized in pack_objects.c's packlist_find(). The return value of locate_object_entry_hash(), which becomes index_pos, is either (in case of found = 1) the position where the (already included) OID is, or (in case of found = 0), index_pos is the position where the (not yet included) OID will be after insertion (which takes place in packlist_alloc() if the hash table is still large enough). However, packlist_find() does not invoke locate_object_entry_hash() if the index size is zero (which might be the case on the first run). This is the only case where index_pos is undefined; and it is irrelevant since the first run will increase the size of the hash table to 1024 and then the undefined value index_pos is ignored. This patch sets index_pos to zero on the first run to silence the warning. Signed-off-by: Stephan Beyer --- pack-objects.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pack-objects.c b/pack-objects.c index 52560293b6..726147a75d 100644 --- a/pack-objects.c +++ b/pack-objects.c @@ -74,8 +74,11 @@ struct object_entry *packlist_find(struct packing_data *pdata, uint32_t i; int found; - if (!pdata->index_size) + if (!pdata->index_size) { + if (index_pos) + *index_pos = 0; /* silence uninitialized warning */ return NULL; + } i = locate_object_entry_hash(pdata, oid, &found); -- 2.23.0.43.g31ebfd7ae6.dirty
[PATCH v2 1/4] am: fail if no author line is given in --rebasing mode
This prevents a potential segmentation fault. Signed-off-by: Stephan Beyer --- builtin/am.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/builtin/am.c b/builtin/am.c index 1aea657a7f..71da34913c 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1272,7 +1272,8 @@ static void get_commit_info(struct am_state *state, struct commit *commit) buffer = logmsg_reencode(commit, NULL, get_commit_output_encoding()); ident_line = find_commit_header(buffer, "author", &ident_len); - + if (!ident_line) + die(_("no author line")); if (split_ident_line(&id, ident_line, ident_len) < 0) die(_("invalid ident line: %.*s"), (int)ident_len, ident_line); -- 2.23.0.43.g31ebfd7ae6.dirty
[PATCH v2 4/4] Silence false-positive maybe-uninitialized warnings found by gcc 9 -flto
gcc 9.2.1 with -flto flag suspects some uninitialized variables which become initialized in every code path where they are used. These false positives are "fixed" by this patch in the most naïve way. This allows to compile git with gcc 9, link-time optimization, and using the DEVELOPER=1 switch (which sets -Werror). Signed-off-by: Stephan Beyer --- bulk-checkin.c | 2 ++ fast-import.c | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/bulk-checkin.c b/bulk-checkin.c index 39ee7d6107..87fa28c227 100644 --- a/bulk-checkin.c +++ b/bulk-checkin.c @@ -200,6 +200,8 @@ static int deflate_to_pack(struct bulk_checkin_state *state, struct hashfile_checkpoint checkpoint; struct pack_idx_entry *idx = NULL; + checkpoint.offset = 0; + seekback = lseek(fd, 0, SEEK_CUR); if (seekback == (off_t) -1) return error("cannot find the current offset"); diff --git a/fast-import.c b/fast-import.c index b44d6a467e..58f73f9105 100644 --- a/fast-import.c +++ b/fast-import.c @@ -903,7 +903,8 @@ static int store_object( struct object_entry *e; unsigned char hdr[96]; struct object_id oid; - unsigned long hdrlen, deltalen; + unsigned long hdrlen; + unsigned long deltalen = 0; git_hash_ctx c; git_zstream s; -- 2.23.0.43.g31ebfd7ae6.dirty
[PATCH] Fix maybe-uninitialized warnings found by gcc 9 -flto
Compiler heuristics for detection of potentially uninitialized variables may change between compiler versions and enabling link-time optimization may find new warnings. Indeed, compiling with gcc 9.2.1 and enabled link-time optimization feature resulted in a few hits that are fixed by this patch in the most naïve way. This allows to compile git using the DEVELOPER=1 switch (which sets -Werror) and using the -flto flag. Signed-off-by: Stephan Beyer --- builtin/am.c | 2 +- builtin/pack-objects.c | 2 +- bulk-checkin.c | 2 ++ fast-import.c | 3 ++- t/helper/test-read-cache.c | 2 +- 5 files changed, 7 insertions(+), 4 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index 1aea657a7f..ab914fd46e 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1266,7 +1266,7 @@ static int get_mail_commit_oid(struct object_id *commit_id, const char *mail) static void get_commit_info(struct am_state *state, struct commit *commit) { const char *buffer, *ident_line, *msg; - size_t ident_len; + size_t ident_len = 0; struct ident_split id; buffer = logmsg_reencode(commit, NULL, get_commit_output_encoding()); diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 76ce906946..d0c03b0e9b 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -1171,7 +1171,7 @@ static int add_object_entry(const struct object_id *oid, enum object_type type, { struct packed_git *found_pack = NULL; off_t found_offset = 0; - uint32_t index_pos; + uint32_t index_pos = 0; display_progress(progress_state, ++nr_seen); diff --git a/bulk-checkin.c b/bulk-checkin.c index 39ee7d6107..87fa28c227 100644 --- a/bulk-checkin.c +++ b/bulk-checkin.c @@ -200,6 +200,8 @@ static int deflate_to_pack(struct bulk_checkin_state *state, struct hashfile_checkpoint checkpoint; struct pack_idx_entry *idx = NULL; + checkpoint.offset = 0; + seekback = lseek(fd, 0, SEEK_CUR); if (seekback == (off_t) -1) return error("cannot find the current offset"); diff --git a/fast-import.c b/fast-import.c index b44d6a467e..58f73f9105 100644 --- a/fast-import.c +++ b/fast-import.c @@ -903,7 +903,8 @@ static int store_object( struct object_entry *e; unsigned char hdr[96]; struct object_id oid; - unsigned long hdrlen, deltalen; + unsigned long hdrlen; + unsigned long deltalen = 0; git_hash_ctx c; git_zstream s; diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c index 7e79b555de..ef0963e2f4 100644 --- a/t/helper/test-read-cache.c +++ b/t/helper/test-read-cache.c @@ -4,7 +4,7 @@ int cmd__read_cache(int argc, const char **argv) { - int i, cnt = 1, namelen; + int i, cnt = 1, namelen = 0; const char *name = NULL; if (argc > 1 && skip_prefix(argv[1], "--print-and-refresh=", &name)) { -- 2.23.0.38.g7ab3f3815a
Re: [RFC PATCH 0/1] Implement CMake build
On 01/24/2018 10:19 PM, Isaac Hier wrote: > Thanks for your interest! This patch is based on the cmake-build > branch of https://github.com/isaachier/git, but the full history is on > the cmake branch (squashed it for easier readability). Hope that > helps. Thanks. I use the cmake branch because I prefer "real" history over one huge commit. And I already love it. Thanks for all the work! >From a first short glance, I wonder if you should mark a lot more options as advanced options, like the paths (e.g., SHELL_PATH, LESS_PATH, GETTEXT_MSGFMT_EXECUTABLE, etc.) and probably also things like GIT_USER_AGENT. If you use a configuration tool like ccmake, you see a lot of options and many of them are not relevant to the average user. I also think some variables have weird names, for example, POLL, PREAD, MMAP should be USE_POLL, USE_PREAD, USE_MMAP, respectively... or even USE_*_SYSCALL, I don't know. By the way, regarding up-to-dateness, you are missing these recent changes that have been merged to master: edb6a17c36 Makefile: NO_OPENSSL=1 should no longer imply BLK_SHA1=1 3f824e91c8 t/Makefile: introduce TEST_SHELL_PATH (which is not surprising) ~Stephan
Re: [RFC PATCH 0/1] Implement CMake build
Hi Isaac, On 01/24/2018 02:45 PM, Isaac Hier wrote: > I realize this is a huge patch, but does anyone have feedback for the > general idea? Thank you very much. I am *personally* interested in this due to several reasons (which are mostly that I am used to CMake and when I do something on the Git codebase, I always end up that its build system recompiles everything ...which drives me crazy as hell. Using CMake, I could simply use out-of-source builds and be happy). I am not sure if it should go into the main Git repo. I'd already be happy if I could pull it from somewhere (github for example) and rebase it to use for my local branches. ~Stephan
Re: "git bisect" takes exactly one bad commit and one or more good?
On 11/11/2017 03:34 PM, Junio C Hamano wrote: > Christian Couder writes: > >>> "You use it by first telling it a "bad" commit that is known to >>> contain the bug, and a "good" commit that is known to be before the >>> bug was introduced." >> >> Yeah, 'and at least a "good" commit' would be better. > > Make it "at least one" instead, perhaps? > > I somehow thought that you technically could force bisection with 0 > good commit, even though no sane person would do so. Thanks for pointing that out but I disagree with the part after "even though" :) Imagine you add a test case that was totally uncovered before and now reveals a bug. You want to find the introduction of the bug, so you can either check out the first commit you think where that bug did not exist, then you find out that its also a bad commit, so you check out another commit... essentially you are manually doing a "bisect" but less efficient. So it would be better to let "git bisect" do its job without knowing a good commit in advance. Sounds perfectly sane to me. The probably insane thing is that there are currently performance issues with git bisect. So you *are* probably faster by guessing. But that is what my patch series [1] was about (and that I postponed in favor of other conflicting work on bisect). 1. https://public-inbox.org/git/1460294354-7031-1-git-send-email-s-be...@gmx.net/ Cheers Stephan
[PATCH] bisect run: die if no command is given
It was possible to invoke "git bisect run" without any command. This considers all commits as good commits since "$@"'s return value for empty $@ is 0. This is most probably not what a user wants (otherwise she would invoke "git bisect run true"), so not providing a command now results in an error. Signed-off-by: Stephan Beyer --- git-bisect.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/git-bisect.sh b/git-bisect.sh index 0138a8860..a69e43656 100755 --- a/git-bisect.sh +++ b/git-bisect.sh @@ -450,6 +450,8 @@ bisect_replay () { bisect_run () { bisect_next_check fail + test -n "$*" || die "$(gettext "bisect run failed: no command provided.")" + while true do command="$@" -- 2.15.0.165.g0dc13a7db.dirty
Re: should "git bisect" support "git bisect next?"
Hi, On 11/11/2017 03:38 PM, Junio C Hamano wrote: > Christian Couder writes: > >> On Sat, Nov 11, 2017 at 12:42 PM, Robert P. J. Day >> wrote: >>> >>> the man page for "git bisect" makes no mention of "git bisect next", >>> but the script git-bisect.sh does: >> >> Yeah the following patch was related: >> >> https://public-inbox.org/git/1460294354-7031-2-git-send-email-s-be...@gmx.net/ >> >> You might want to discuss with Stephan (cc'ed). > > Thanks for saving me time to explain why 'next' is still a very > important command but the end users do not actually need to be > strongly aware of it, because most commands automatically invokes it > as their final step due to the importance of what it does ;-) I will nonetheless re-roll the patch (that Christian linked to) after Pranit's bisect part II series is in good shape. I think the documentation change in the patch shows why the user should be aware of it (although not strongly). Best Stephan
Re: [PATCH v16 Part II 5/8] bisect--helper: `bisect_next_check` shell function in C
Hi again ;) On 10/27/2017 05:06 PM, Pranit Bauva wrote: > @@ -44,6 +46,11 @@ static void set_terms(struct bisect_terms *terms, const > char *bad, > terms->term_bad = xstrdup(bad); > } > > +static const char *voc[] = { > + "bad|new", > + "good|old" > +}; > + > /* > * Check whether the string `term` belongs to the set of strings > * included in the variable arguments. > @@ -264,6 +271,79 @@ static int check_and_set_terms(struct bisect_terms > *terms, const char *cmd) > return 0; > } > > +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 int bisect_next_check(const struct bisect_terms *terms, > + const char *current_term) > +{ > + int missing_good = 1, missing_bad = 1, retval = 0; > + const char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad); > + const char *good_glob = xstrfmt("%s-*", terms->term_good); > + > + if (ref_exists(bad_ref)) > + missing_bad = 0; > + > + for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/", > + (void *) &missing_good); > + > + if (!missing_good && !missing_bad) > + goto finish; > + > + if (!current_term) > + goto fail; > + > + if (missing_good && !missing_bad && current_term && > + !strcmp(current_term, terms->term_good)) { > + 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); > + if (!isatty(0)) > + goto finish; > + /* > + * 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")) > + goto fail; > + > + goto finish; > + } > + 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"), > + voc[0], voc[1], voc[0], voc[1]); > + goto fail; > + } 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"), > + voc[1], voc[0], voc[1], voc[0]); > + goto fail; In both of the above "error" calls, you should drop the final "\n" because "error" does that already. On the other hand, you have dropped the "\n"s of the orginal error messages. So it should probably be _("You need to give me at least one %s and %s revision.\n" "You can use \"git bisect %s\" and \"git bisect %s\" for that.") and _("You need to start by \"git bisect start\".\n" "You then need to give me at least one %s and %s revision.\n" "You can use \"git bisect %s\" and "\"git bisect %s\" for that.") Stephan
Re: [PATCH v16 Part II 5/8] bisect--helper: `bisect_next_check` shell function in C
> @@ -21,6 +22,7 @@ static const char * const git_bisect_helper_usage[] = { > N_("git bisect--helper --bisect-reset []"), > N_("git bisect--helper --bisect-write > []"), > N_("git bisect--helper --bisect-check-and-set-terms > "), > + N_("git bisect--helper --bisect-next-check [] > "), I think the order is wrong. It is []
Re: [PATCH v16 Part II 5/8] bisect--helper: `bisect_next_check` shell function in C
Hi, another minor: On 10/27/2017 05:06 PM, Pranit Bauva wrote: > @@ -264,6 +271,79 @@ static int check_and_set_terms(struct bisect_terms > *terms, const char *cmd) > return 0; > } > > +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 int bisect_next_check(const struct bisect_terms *terms, > + const char *current_term) > +{ > + int missing_good = 1, missing_bad = 1, retval = 0; > + const char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad); > + const char *good_glob = xstrfmt("%s-*", terms->term_good); > + > + if (ref_exists(bad_ref)) > + missing_bad = 0; > + > + for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/", > + (void *) &missing_good); > + > + if (!missing_good && !missing_bad) > + goto finish; > + > + if (!current_term) > + goto fail; > + > + if (missing_good && !missing_bad && current_term && This check for "current_term" is not necessary; it can be asserted to be non-NULL, otherwise you would have jumped to "fail" Stephan
Re: [PATCH v16 Part II 7/8] bisect--helper: `bisect_start` shell function partially in C
Hi, > + return error(_("unrecognised option: '%s'"), arg); Please write "unrecogni_z_ed". Since the string for translation changed from "unrecognised option: '$arg'" to "unrecognised option: '%s'" anyway, it does not result in further work for the translators to correct it to "unrecognized option: '%s'" Stephan
Re: [PATCH v16 Part II 6/8] bisect--helper: `get_terms` & `bisect_terms` shell function in C
On 10/27/2017 05:06 PM, Pranit Bauva wrote: > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 0f9c3e63821b8..ab0580ce0089a 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c [...] > +static int bisect_terms(struct bisect_terms *terms, const char **argv, int > argc) > +{ > + int i; > + > + if (get_terms(terms)) > + return error(_("no terms defined")); > + > + if (argc > 1) > + return error(_("--bisect-term requires exactly one argument")); > + > + if (argc == 0) > + return !printf(_("Your current terms are %s for the old state\n" > + "and %s for the new state.\n"), > + terms->term_good, terms->term_bad); Same as in 1/8: you probably want "printf(...); return 0;" except there is a good reason. > + > + for (i = 0; i < argc; i++) { > + if (!strcmp(argv[i], "--term-good")) > + printf(_("%s\n"), terms->term_good); > + else if (!strcmp(argv[i], "--term-bad")) > + printf(_("%s\n"), terms->term_bad); The last two printfs: I think there is no point in translating "%s\n", so using "%s\n" instead of _("%s\n") looks more reasonable. > + else > + error(_("BUG: invalid argument %s for 'git bisect > terms'.\n" > + "Supported options are: " > + "--term-good|--term-old and " > + "--term-bad|--term-new."), argv[i]); Should this be "return error(...)"? > + } > + > + return 0; > +} > + Stephan
Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C
On 10/30/2017 02:38 PM, Stephan Beyer wrote: > This last change is not necessary. You never use "res". Whoops, ignore this! I compared old and new diff and mixed something up, it seems. Sorry, Stephan
Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C
Also just a minor in addition to the others: > @@ -153,9 +241,10 @@ int cmd_bisect__helper(int argc, const char **argv, > const char *prefix) > WRITE_TERMS, > BISECT_CLEAN_STATE, > CHECK_EXPECTED_REVS, > - BISECT_RESET > + BISECT_RESET, > + BISECT_WRITE > } cmdmode = 0; > - int no_checkout = 0; > + int no_checkout = 0, res = 0; This last change is not necessary. You never use "res". Stephan
Re: [PATCH v16 Part II 1/8] bisect--helper: `bisect_reset` shell function in C
Hi Pranit, On 10/27/2017 05:06 PM, Pranit Bauva wrote: > A big thanks to Stephan and Ramsay for patiently reviewing my series and > helping me get this merged. You're welcome. ;) In addition to the things mentioned by the other reviewers, only a minor thing: > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 35d2105f941c6..12754448f7b6a 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c [...]> @@ -106,13 +112,48 @@ static void check_expected_revs(const char **revs, int rev_nr) > } > } > > +static int bisect_reset(const char *commit) > +{ > + struct strbuf branch = STRBUF_INIT; > + > + if (!commit) { > + if (strbuf_read_file(&branch, git_path_bisect_start(), 0) < 1) > + return !printf(_("We are not bisecting.\n")); This is weird; I had to look up the return value of printf first before I could understand what you are doing ;) I think that it is meant as a shortcut for printf(_("We are not bisecting.\n")); return 0; but please also express it with these two lines. (Or what is the point of returning a non-zero value only in the case when nothing could be printed?) Best, Stephan
Re: [PATCH v3] clang-format: add a comment about the meaning/status of the
Hi, On 10/02/2017 01:37 AM, Junio C Hamano wrote: > diff --git a/.clang-format b/.clang-format > index 56822c116b..7670eec8df 100644 > --- a/.clang-format > +++ b/.clang-format > @@ -1,4 +1,8 @@ > -# Defaults > +# This file is an example configuration for clang-format 5.0. > +# > +# Note that this style definition should only be understood as a hint > +# for writing new code. The rules are still work-in-progress and does > +# not yet exactly match the style we have in the existing code. I'm totally fine with this. Stephan
Re: [PATCH v2] Add a comment to .clang-format about the meaning of the file
On 10/01/2017 10:30 PM, Junio C Hamano wrote: > I think we do want the endgame to be that .clang-format defines how > the code should look like. It's that we are not there yet, and I > think that is what we should say in this comment. > > Note that this style definition does not yet quite reflect > how we want our code to look like, and adjusting the rules > to match our style is still work in progress. Do not > blindly adjust the style of _existing_ code, without > checking if the code is styled incorrectly, or the style > definition in this file is still wrong. > > is what I should have suggested when writing my response. Pretty long but okay. I tried to be shorter and more implicit (also because the CodingGuidelines are already pretty verbose on not changing existing code style) and you're heading in the direction that there will be some clang-format definition that matches the desired coding style (I doubt that at least for the current clang-format versions, but that's another topic). Erm, so you're going to replace the comment? Or is it my task now to make a v3 patch with your text? (The latter doesn't look useful to me...) Stephan
[PATCH v2] Add a comment to .clang-format about the meaning of the file
Having a .clang-format file in a project can be understood in a way that code has to be in the style defined by the .clang-format file, i.e., you just have to run clang-format over all code and you are set. This is not the case in the Git project, which is now reflected by a comment in the beginning of the file. Additionally, the working clang-format version is mentioned because the config directives change from time to time (in a compatibility-breaking way). Signed-off-by: Stephan Beyer --- Notes: On 10/01/2017 04:45 AM, Junio C Hamano wrote: > it makes as if a random patch to "make it > conform" without thinking if the rules make sense were a welcome > addition, which is absolutely the last signal we would want to send > to the readers. Right. I dropped that last sentence and replaced it by a sentence about human aesthetics judgement overruling mechanical rules -- I think that's somehow quoted from a comment of yours on the list. .clang-format | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.clang-format b/.clang-format index 3ede2628d..041b7be03 100644 --- a/.clang-format +++ b/.clang-format @@ -1,4 +1,8 @@ -# Defaults +# This file is an example configuration for clang-format 5.0. +# +# Note that this style definition should only be understood as a hint +# for writing new code. In the end, human aesthetics judgement overrules +# mechanical rules. # Use tabs whenever we need to fill whitespace that spans at least from one tab # stop to the next one. -- 2.14.2.677.g5a59ab275
[PATCH] Add a comment to .clang-format about the meaning of the file
Having a .clang-format file in a project can be understood in a way that code has to be in the style defined by the .clang-format file, i.e., you just have to run clang-format over all code and you are set. This is not the case in the Git project, which is now reflected by an comment in the beginning of the file. Additionally, the working clang-format version is mentioned because the config directives change from time to time (in a compatibility-breaking way). Signed-off-by: Stephan Beyer --- Notes: On 09/30/2017 12:45 AM, Jonathan Nieder wrote: > Sounds good to me. Care to send it as a patch? :) Like this? :) .clang-format | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.clang-format b/.clang-format index 3ede2628d..558fc7fd8 100644 --- a/.clang-format +++ b/.clang-format @@ -1,4 +1,8 @@ -# Defaults +# This file is an example configuration for clang-format 5.0. +# +# Note that this style definition should only be understood as a hint +# for writing new code. Most of Git's codebase does not conform to +# this definition. # Use tabs whenever we need to fill whitespace that spans at least from one tab # stop to the next one. -- 2.14.2.677.g5a59ab275
Re: [PATCH] clang-format: adjust line break penalties
Hi, On 09/29/2017 08:40 PM, Jonathan Nieder wrote: > Going forward, is there an easy way to preview the effect of this kind > of change (e.g., to run "make style" on the entire codebase so as to be > able to compare the result with two different versions of > .clang-format)? I just ran clang-format before and after the patch and pushed to github. The resulting diff is quite big: https://github.com/sbeyer/git/commit/3d1186c4cf4dd7e40b97453af5fc1170f6868ccd Cheers Stephan PS: There should be a comment at the beginning of the .clang-format file that says what version it is tested with (on my machine it worked with 5.0 but not with 4.0) and there should also probably a remark that the clang-format-based style should only be understood as a hint or guidance and that most of the Git codebase does not conform it.
Re: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
Hi Pranit, On 09/29/2017 08:49 AM, Pranit Bauva wrote: > It has been a long time since this series appeared on the mailing list. > The previous version v15[1] is now split into many parts and I am > sending the first part right now, will focus on getting this merged and > then send out the next part. That's a good idea! I just reviewed your series by assuming I did the v15 review well (it took quite some time at least)... so I just diff'ed the v15 and the v16 patches. I am totally fine with it! Thanks, Stephan
Re: Bug report
On 08/31/2017 08:36 AM, Kevin Daudt wrote: > On Wed, Aug 30, 2017 at 11:25:00PM +0200, Aleksandar Pavic wrote: >> I have a file >> >> app/Controller/CustomerCardVerificationController.php >> >> And when I take a look at changes log, I get this (no matter which tool I >> use): >> >> 2017-07-31 19:41 dule o membership renew payment email >> 2017-06-07 08:59 Dusan Tatic o cc refund clean >> 2017-04-15 00:16 Miodrag Dragić o refound admin payment >> 2017-03-20 12:02 Dusan Tatic o CardVerification card connect >> 2017-03-16 15:59 Aleksandar Pavic o paypal >> 2017-03-10 13:34 Aleksandar Pavic o Production branch >> 2017-03-10 13:01 Aleksandar Pavic I Migrating dev >> >> However if I manually browse thru revisions and open revision from >> 03/27/2017 07:05 PM >> >> I can see the change in that file which is unlisted above, at revision >> ff9f4946e109bd234d438e4db1d319b1f6cb6580 I am not sure if I fully understand but I guess your commit ff9f4946e1 has been commited at detached HEAD. You could do git log --oneline --graph master ff9f4946e1 to see where the "missing" commit lies. And probably a git cherry-pick ff9f4946e1 might pick your missing feature. Stephan
Re: [BUG] rebase -i with only empty commits
On 08/23/2017 07:29 PM, Stefan Beller wrote: > On Wed, Aug 23, 2017 at 8:19 AM, Stephan Beyer wrote: >> On 08/23/2017 04:40 PM, Johannes Schindelin wrote: >>> These days, I reflexively type `rebase -ki` instead of `rebase -i`. Maybe >>> you want to do that, too? >> >> That's a very valuable hint, thank you very much! > > While -k side steps the original problem, it seems like it would > have helped me, too. > > Is there any value in discussing turning it on by default? I also wondered why empty commits are "discriminated" in such a way. I first thought that if you rebase branch A onto B but branch A and B contain commits with the same changes, then these commits would become new empty commits instead of simply being ignored. But I just checked this theory and it is now falsified :) It seems empty commits occur *only* if the user wants them to occur (--allow-empty). If they occur unintentionally (for example, by importing some SVN), one can eliminate them using filter-branch or rebase (by commenting out these picks). So it is still unclear to me, why empty commits are handled in such a special way. Best Stephan PS: Although -k helps, the original behavior of rebase -i is still a bug.
Re: [BUG] rebase -i with only empty commits
Hi, On 08/23/2017 04:40 PM, Johannes Schindelin wrote: > These days, I reflexively type `rebase -ki` instead of `rebase -i`. Maybe > you want to do that, too? That's a very valuable hint, thank you very much! Best Stephan
[BUG] rebase -i with only empty commits
Hi, On 08/23/2017 01:08 AM, Stefan Beller wrote: > The editor opened proposing the following instruction sheet, > which in my opinion is buggy: > > pick 1234 some commit > exec make > pick 2345 another commit > exec make > pick 3456 third commit > # pick 4567 empty commit > exec make > pick 5678 yet another commit > exec make This reminds me of another bug I stumbled over recently regarding empty commits. Do this: # repo preparation: git init :> file1 git add file1 git commit -m "add file1" :> file2 git add file2 git commit -m "add file2" # the bug: git checkout -b to-be-rebased master^ git commit --allow-empty -m "empty commit" git rebase -i master It says "Nothing to do". Unsurprisingly, the problem persists when you apply other empty commits: git commit --allow-empty -m "another empty commit" git rebase -i master Adding a "real" commit solves the problem: :>file3 git add file3 git commit -m "add file3" Adding further empty commits is no problem: git commit --allow-empty -m "yet another empty commit" So the problem seems to be that rebase -i (like rebase without -i) considers "empty commits" as commits to be ignored. However, when using rebase -i one expects that you can include the empty commit... Also, the behavior is odd. When I only have empty commits, a "git rebase master" works as expected like a "git reset --hard master" but "git rebase -i" does nothing. The expected behavior would be that the editor shows up with a git-rebase-todo like: # pick 3d0f6c49 empty commit # pick bbbc5941 another empty commit noop Thanks Stephan
Re: [PATCH] Correct compile errors when DEBUG_BISECT=1 after supporting other hash algorithms
Hi, On 03/29/2017 10:02 PM, Alex Hoffman wrote: > Any news about this patch? Haha nice, your initial patch is the same as mine (but mine was part of a bigger patch series and the v3 is probably going to have one less commit): https://public-inbox.org/git/1456452282-10325-4-git-send-email-s-be...@gmx.net/ >> for (pp = commit->parents; pp; pp = pp->next) >> fprintf(stderr, " %.*s", 8, >> - sha1_to_hex(pp->item->object.sha1)); >> + oid_to_hex(&pp->item->object.oid)); I guess your change in continued indentation is intentional, but is it just my mail client or do you f*ck up tabs? (I haven't tried to apply the patch but it looks like it is not possible due to broken tabs.) Stephan
Re: What's cooking in git.git (Mar 2017, #02; Fri, 3)
Hi Pranit, On 03/04/2017 12:26 AM, Junio C Hamano wrote: > -- > [Stalled] [...] > > * pb/bisect (2017-02-18) 28 commits > - fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C > - bisect--helper: remove the dequote in bisect_start() > - bisect--helper: retire `--bisect-auto-next` subcommand > - bisect--helper: retire `--bisect-autostart` subcommand > - bisect--helper: retire `--bisect-write` subcommand > - bisect--helper: `bisect_replay` shell function in C > - bisect--helper: `bisect_log` shell function in C > - bisect--helper: retire `--write-terms` subcommand > - bisect--helper: retire `--check-expected-revs` subcommand > - bisect--helper: `bisect_state` & `bisect_head` shell function in C > - bisect--helper: `bisect_autostart` shell function in C > - bisect--helper: retire `--next-all` subcommand > - bisect--helper: retire `--bisect-clean-state` subcommand > - bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C > - t6030: no cleanup with bad merge base > - bisect--helper: `bisect_start` shell function partially in C > - bisect--helper: `get_terms` & `bisect_terms` shell function in C > - bisect--helper: `bisect_next_check` & bisect_voc shell function in C > - bisect--helper: `check_and_set_terms` shell function in C > - bisect--helper: `bisect_write` shell function in C > - bisect--helper: `is_expected_rev` & `check_expected_revs` shell function > in C > - bisect--helper: `bisect_reset` shell function in C > - wrapper: move is_empty_file() and rename it as is_empty_or_missing_file() > - t6030: explicitly test for bisection cleanup > - bisect--helper: `bisect_clean_state` shell function in C > - bisect--helper: `write_terms` shell function in C > - bisect: rewrite `check_term_format` shell function in C > - bisect--helper: use OPT_CMDMODE instead of OPT_BOOL > > Move more parts of "git bisect" to C. > > Expecting a reroll. I guess you are short on time but I am hoping that you are still going to send a reroll to the list (probably on top of [1]?). This is because I would like to start to work on rerolling my bisect patches from last year [2] ... but to avoid a mess of merge conflicts, I am waiting until pb/bisect found its way into "next". (There were also recent discussions on other bisect strategies [3] and it's probably only a matter of time until a new big patchset on bisect--helper comes up...) Cheers Stephan [1]: https://public-inbox.org/git/xmqqvarq9vzo@gitster.mtv.corp.google.com/ [2]: https://public-inbox.org/git/1460294354-7031-1-git-send-email-s-be...@gmx.net/ [3]: https://public-inbox.org/git/CABEd3j8m5D=bBbUD+uzvE9c8AwdBEM79Np7Pnin-RLL=hjq...@mail.gmail.com/
Re: Git bisect does not find commit introducing the bug
Hi, On 02/17/2017 11:29 PM, Alex Hoffman wrote: > According to the documentation "git bisect" is designed "to find the > commit that introduced a bug" . > I have found a situation in which it does not returns the commit I expected. > In order to reproduce the problem: For the others who are too lazy to clone your repo and run the scripts, the history is like that (read from bottom to top) and I marked the commit found by git bisect and the on you expected: * 7a9e952 (bisect bad) |\ | * 671cec2 <--- expected | |\ | * | 04c6f4b <--- found * | | 3915157 |\ \ \ | | |/ | |/| | * | f4154e9 (bisect good) | * | 85855bf | |/ * | f1a36f5 |/ * 1b7fb88 The and markers are set by your definition of what good and what bad commits are. > First of all this is confusing, as this commit cannot be reached > starting from "v.good". Hm, IMHO it shows that your example is pretty artificial (although you might have come across it in a real-world scenario): you introduced a new feature in f4154e9 (and it worked) and you broke that feature by making the merge 671cec2. However, the feature (that broke in 671cec2) did not even exist in 04c6f4b; so a test on the feature would not fail (leading to "bisect bad" as in the example), it would not exist (leading to "bisect skip"). And if we are not talking about passing or failing tests but about crashing, bisect finds the right thing: f4154e9 was not crashing, but 04c6f4b is crashing. Yes, it's not the commit that introduced the crash (which would be the first commit in the repo) but it's the first crashing commit after the one marked as good. So I'd consider this a feature or rather correct behavior, not a bug. In other words: bisect assumes that your repo is usually in a good state and you have a commit that changes it to a bad state. In your case you have a repo that is in a bad state and you have a commit that switches it to a good state and later you merge a bad-state branch and you have a bad state again. It is not made for that use-case, I think. Cheers Stephan
Re: [RFC PATCH] show decorations at the end of the line
Hi, On 02/13/2017 09:30 AM, Junio C Hamano wrote: > Linus Torvalds writes: > >> On Sat, Feb 11, 2017 at 10:02 AM, Linus Torvalds >> wrote: >>> >>> I've signed off on this, because I think it's an "obvious" improvement, >>> but I'm putting the "RFC" in the subject line because this is clearly a >>> subjective thing. >> >> Side note: the one downside of showing the decorations at the end of >> the line is that now they are obviously at the end of the line - and >> thus likely to be more hidden by things like line truncation. > > Side note: I refrained from commenting on this patch because > everybody knows that the what I would say anyway ;-) and I didn't > want to speak first to discourage others from raising their opinion. A further side note: the current behavior of git log --oneline --decorate is equivalent to git log --pretty='format:%C(auto)%h%d %s' and Linus' preferred version is equivalent to git log --pretty='format:%C(auto)%h %s%d' Most Git users I know have their own favorite version of git log --pretty=format:... sometimes with --graph as an alias ("git lg" or "git logk" (because its output reminds of gitk) or something). I don't know what the main benefit of this patch would be, but if it gets accepted, it should probably be mentioned somewhere that the old behavior is easily accessible using the line mentioned above. Cheers Stephan
Re: Automatically Add .gitignore Files
> I am tempted to say that there should be a way to somehow forbid use > of the "-m" option to "git commit" by default, until the user gains > more familiarity with use of Git. Since I am using git, I am tempted to say that "git commit -m" should be abolished. If I tell somebody how to use git, I never mention "git commit -m". Unluckily they find it out themselves... ;D The typical observations I have when people use "git commit -m": * The commit messages are either too long (in one line!) or too meaningless. * People don't check what they added and commit wrong stuff. * People make fast temporary commits with "asdafsd" commit messages. Then they get distracted for some time and work on another branch. When they turn back to the old branch they don't know what "asdafsd" was... Thanks to git rebase -i and/or git commit --amend, it is all not too bad after all ;D Best Stephan
Re: [RFC] stash --continue
Hi, On 01/18/2017 04:41 PM, Marc Branchaud wrote: > On 2017-01-16 05:54 AM, Johannes Schindelin wrote: >> On Mon, 16 Jan 2017, Stephan Beyer wrote: >>> a git-newbie-ish co-worker uses git-stash sometimes. Last time he used >>> "git stash pop", he got into a merge conflict. After he resolved the >>> conflict, he did not know what to do to get the repository into the >>> wanted state. In his case, it was only "git add " >>> followed by a "git reset" and a "git stash drop", but there may be more >>> involved cases when your index is not clean before "git stash pop" and >>> you want to have your index as before. >>> >>> This led to the idea to have something like "git stash --continue"[1] >> >> More like "git stash pop --continue". Without the "pop" command, it does >> not make too much sense. > > Why not? git should be able to remember what stash command created the > conflict. Why should I have to? Dscho and Junio gave you a git-perspective argument. I give you a user-perspective one: What if you did "git stash pop" and ran into an (unexpected) conflict. You resolve it, and you probably - for some reason - don't want to drop the stash now, as "git stash --continue" (assuming "pop") would do. So I'd regard it as a feature if you could now run "git stash apply --continue" to just finish the job without dropping. Best Stephan PS: I put this idea in my todo priority queue. If somebody else is interested: I am not going to work at this idea before February. PPS: Any opinions about the mentioned "backwards-compatibility" issue that people are then forced to finish their commits with "--continue" instead of "git reset" or "git commit"?
Re: [RFC] stash: support filename argument
Hi, On 01/16/2017 01:21 AM, Junio C Hamano wrote: > I haven't spent enough time to think if it even makes sense to > "stash" partially, leaving the working tree still dirty. My initial > reaction was "then stashing away the dirty WIP state to get a spiffy > clean working environment becomes impossible", and I still need time > to recover from that ;-) So as to the desirablity of this "feature", > I have no strong opinion for or against yet. I do remember that I simulated that feature a few times (either by adding the to-be-keep stuff (hunks, not only files) to the index and use --keep-index, and sometimes by making a temporary commit (to make sure to not lose anything) and then stash). So I think there is a valid desire of the feature. However, going further, the next feature to be requested could be "git stash --patch" ;D This leads me to think that the mentioned simulation of partial stashes might be something everyone might come up with who has basic understanding of git features and "git stash", and might be the more flexible and probably better solution to the problem. Best Stephan
[RFC] stash --continue
Hi, a git-newbie-ish co-worker uses git-stash sometimes. Last time he used "git stash pop", he got into a merge conflict. After he resolved the conflict, he did not know what to do to get the repository into the wanted state. In his case, it was only "git add " followed by a "git reset" and a "git stash drop", but there may be more involved cases when your index is not clean before "git stash pop" and you want to have your index as before. This led to the idea to have something like "git stash --continue"[1] that would expect the user to "git add" the resolved files (as "git status" suggests) but then leads to the expected result, i.e. the index being the same as before the conflict, the stash being dropped (if "pop" was used instead of "apply"), etc. Likewise, some "git stash --abort"[2] might be useful in case you did "git stash pop" with the wrong stash in mind. What do you think about that? Although I think this would be a nice-to-have feature, I am not totally sure how to achieve backwards-compatibility, i.e., such that using --continue is still optional... (I think that "git stash apply" would surely generate auxiliary data in case of a conflict and --continue or --abort would remove it...) Best Stephan Footnotes 1. Perhaps with a better name, because it does not really continue. Maybe a "git stash conflict [--resolved]" subcommand? 2. Along the lines of [1], one could use "git stash conflict --abort"?
Re: [PATCH v15 15/27] bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C
Hi Pranit, On 12/31/2016 11:43 AM, Pranit Bauva wrote: >>> + >>> +static int bisect_auto_next(struct bisect_terms *terms, const char *prefix) >>> +{ >>> + if (!bisect_next_check(terms, NULL)) >>> + return bisect_next(terms, prefix); >>> + >>> + return 0; >>> +} >> >> Hmm, the handling of the return values is a little confusing. However, >> if I understand the sh source correctly, it always returns success, no >> matter if bisect_next failed or not. I do not know if you had something >> special in mind here. > > Umm. Shell code used to die() and thus exit with an error code. The invoked bisect_next shell code called "exit", right... you had to replace this by passing return values. I get it. Thank you! >>> int cmd_bisect__helper(int argc, const char **argv, const char *prefix) >>> @@ -643,6 +794,10 @@ int cmd_bisect__helper(int argc, const char **argv, >>> const char *prefix) >>>N_("print out the bisect terms"), BISECT_TERMS), >>> OPT_CMDMODE(0, "bisect-start", &cmdmode, >>>N_("start the bisect session"), BISECT_START), >>> + OPT_CMDMODE(0, "bisect-next", &cmdmode, >>> + N_("find the next bisection commit"), BISECT_NEXT), >>> + OPT_CMDMODE(0, "bisect-auto-next", &cmdmode, >>> + N_("verify the next bisection state then find the >>> next bisection state"), BISECT_AUTO_NEXT), >> >> The next bisection *state* is found? > > checkout is more appropriate. I don't remember why I used "find". "checkout the next bisection commit" maybe? Thanks, Stephan
Re: [PATCH v2 01/34] sequencer: support a new action: 'interactive rebase'
Hi Dscho, >> However, maintaining more than one directory (like "sequencer" for >> sequencer state and "rebase-merge" for rebase todo and log) can cause >> the necessity to be even more careful when hacking on sequencer... For >> example, the cleanup code must delete both of them, not only one of them. > > That is incorrect. It depends on the options which directory is used. And > it is that directory (and not both) that should be cleaned up in the end. > > Otherwise you run into a ton of pain e.g. when running a rebase -i with an > `exec git cherry-pick ...` line: all of a sudden, that little innocuous > line would simply destroy the state directory of the current rebase -i. > > That's a rather big no-no. Ahh, I see, there seems to be a misunderstanding on my side about how you did it (I did not look into the other patches (obviously)). Thanks for clarifying! Best Stephan
Re: Suggestion for the "Did you mean this?" feature
Hi, On 12/18/2016 01:18 PM, Kaartic Sivaraam wrote: > I have found the "Did you mean this?" feature of git as a very good > feature. I thought it would be even better if it took a step toward by > asking for a prompt when there was only one alternative to the command > that was entered. > > E.g. > >> unique@unique-pc:~$ git hepl >> git: 'hepl' is not a git command. See 'git --help'. >> >> Did you mean this? >> help >> [yes/No] : y >> usage: git [--version] [--help] [-C ] [-c name=value] >>[--exec-path[=]] [--html-path] [--man-path] [--info- >> path] >> > > This would make it even better for the user as it would avoid having to > correct the mistake long commands that had only a single error > (considering history feature is enabled). > > Is this is a good idea ? I cannot tell if this is a good idea (or why it would be a bad idea) but why do you restrict your suggestion to the case when there is only one alternative? Why not also something like: --- $ git sta git: 'sta' is not a git command. See 'git --help'. Did you mean one of these? [1] status [2] stage [3] stash You can choose or quit [1,2,3,q]: --- Best Stephan
Re: [PATCH v2 01/34] sequencer: support a new action: 'interactive rebase'
Hi, On 12/14/2016 08:29 PM, Junio C Hamano wrote: > Johannes Schindelin writes: >> -/* We will introduce the 'interactive rebase' mode later */ >> static inline int is_rebase_i(const struct replay_opts *opts) >> { >> -return 0; >> +return opts->action == REPLAY_INTERACTIVE_REBASE; >> } >> >> static const char *get_dir(const struct replay_opts *opts) >> { >> +if (is_rebase_i(opts)) >> +return rebase_path(); >> return git_path_seq_dir(); >> } > > This obviously makes the assumption made by 39784cd362 ("sequencer: > remove useless get_dir() function", 2016-12-07) invalid, where the > "remove useless" thought that the callers of this function wants a > single answer that does not depend on opts. > > I'll revert that commit from the sb/sequencer-abort-safety topic (as > the topic is in 'next' already) to keep this one. Please holler if > that is a mistaken decision. It seems to be the right decision if one wants to keep the internal-path backwards-compatibility of "git rebase -i" (and it seems that Dscho wants to keep it). However, maintaining more than one directory (like "sequencer" for sequencer state and "rebase-merge" for rebase todo and log) can cause the necessity to be even more careful when hacking on sequencer... For example, the cleanup code must delete both of them, not only one of them. Hence, I think that it's a wiser choice to keep one directory for sequencer state *and* additional files. If we (a) save everything in "sequencer", we break the internal-path backwards-compatbility but we can get rid of get_dir()... If we (b) save everything in "rebase-merge" when using rebase -i and save everything in "sequencer" for pick/revert, we are 100% backwards-compatible, but we have to change every occurrence of git_path_seq_dir() to get_dir() and the GIT_PATH_FUNC definitions of git_path_{todo,opts,head}_file must be replaced by definitions using get_dir(). Best Stephan
Re: [PATCH v15 08/27] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
Hi Pranit, On 12/16/2016 08:35 PM, Pranit Bauva wrote: > On Thu, Nov 17, 2016 at 5:17 AM, Stephan Beyer wrote: >> On 10/14/2016 04:14 PM, Pranit Bauva wrote: >>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c >>> index d84ba86..c542e8b 100644 >>> --- a/builtin/bisect--helper.c >>> +++ b/builtin/bisect--helper.c >>> @@ -123,13 +123,40 @@ static int bisect_reset(const char *commit) >>> return bisect_clean_state(); >>> } >>> >>> +static int is_expected_rev(const char *expected_hex) >>> +{ >>> + struct strbuf actual_hex = STRBUF_INIT; >>> + int res = 0; >>> + if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) >>> >= 40) { >>> + strbuf_trim(&actual_hex); >>> + res = !strcmp(actual_hex.buf, expected_hex); >>> + } >>> + strbuf_release(&actual_hex); >>> + return res; >>> +} >> >> I am not sure it does what it should. >> >> I would expect the following behavior from this function: >> - file does not exist (or is "broken") => return 0 >> - actual_hex != expected_hex => return 0 >> - otherwise return 1 >> >> If I am not wrong, the code does the following instead: >> - file does not exist (or is "broken") => return 0 >> - actual_hex != expected_hex => return 1 >> - otherwise => return 0 > > It seems that I didn't carefully see what the shell code is (or > apparently did a mistake in understanding it ;)). I think the C > version does exactly what the shell version does. Can you confirm it > once again, please? I check again... The shell code does the following: - file does not exist or is not a regular file => return false - actual_hex != expected_hex => return false - otherwise => return true (false means a value != 0, true means 0) Your code does the following: - file cannot be read or is broken => return 0 - actual_hex != expected_hex => return 0 - otherwise => return 1 So you are very right, it seems I had a weird thinko (I probably missed the "!" in front of strcmp or something) :) Sorry for bothering, Stephan
Re: [PATCH v15 08/27] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
Hi Pranit, On 12/16/2016 08:00 PM, Pranit Bauva wrote: > On Wed, Dec 7, 2016 at 1:03 AM, Pranit Bauva wrote: >>> I don't understand why the return value is int and not void. To avoid a >>> "return 0;" line when calling this function? >> >> Initially I thought I would be using the return value but now I >> realize that it is meaningless to do so. Using void seems better. :) > > I just recollected when I was creating the next iteration of this > series that I will need that int. > + case CHECK_EXPECTED_REVS: + return check_expected_revs(argv, argc); > > See this. This does not show that you need the "int", it just shows that you use the return value of the function. But this return value is (in the original shell code as well as in your v15) always 0. That is a sign that the "void" return value makes more sense. Of course, then the line above must be changed to + case CHECK_EXPECTED_REVS: + check_expected_revs(argv, argc); + return 0; By the way, it also seems that the original function name "check_expected_revs" was not the best choice (because the function always returns 0, but the "check" implies that some semantically true or false is returned). On the other hand, it might also be useful (I cannot tell) to return different values in check_expected_revs() — to signal the caller that something changed or something did not change — but then ignore the return value. Best Stephan
git add -p with unmerged files (was: git add -p with new file)
Hi, While we're on the topic that "git add -p" should behave like the "normal" "git add" (not "git add -u"): what about unmerged changes? When I have merge conflicts, I almost always use my aliases "edit-unmerged" and "add-unmerged": $ git config --global --list | grep unmerged alias.list-unmerged=diff --name-only --diff-filter=U alias.edit-unmerged=!vim `git list-unmerged` alias.add-unmerged=!git add `git list-unmerged` alias.reset-unmerged=!uf=`git list-unmerged`; git reset HEAD $uf; git checkout -- $uf The "add-unmerged" alias is always a little scary because I'd rather like to check the changes with the "git add -p" workflow I am used to. Opinions? Best Stephan
Re: git add -p with new file
Hi, On 12/11/2016 02:00 PM, Jeff King wrote: > On Sat, Dec 10, 2016 at 02:04:33PM -0800, Junio C Hamano wrote: >> Jeff King writes: >>> On Fri, Dec 09, 2016 at 01:43:24PM -0500, Ariel wrote: >>> ... But it doesn't have to be that way. You could make add -p identical to add without options, except the -p prompts to review diffs first. >>> >>> The question is whether you would annoy people using "-p" if you started >>> including untracked files by default. I agree because it's inherently an >>> interactive process that we can be looser with backwards compatibility. >> >> It might be interactive, but it will be irritating that we suddenly >> have to see hundreds of lines in an untracked file before we are >> asked to say "no I do not want to add this file" and have to do so >> for all the untracked files that happen to match the given pathspec. >> >> It might make it less irritating if one of the interactive choices >> offered in the first prompt were N that tells the command: "No, >> ignore all the untracked paths", though. I dunno. > > Yeah, I agree dumping the contents automatically is annoying. Ariel > suggested asking twice about each path, which sounds clunky to me. I'd > probably give a simple question, with an option to dump the contents. > Like: > > $ echo foo >untracked > $ git add -p > New file: untracked > Stage this file [y,n,v,q,a,d,/,e,?]? v <-- user types 'v' for "view" I am also a "git add -p"-only user (except for new files and merges). However, I usually keep a lot of untracked files in my repositories. Files that I do not (git)ignore because I want to see them when I type "git status". Hence, the imagination only that "git add -p" starts to ask me for each untracked file feels like a lot of annoying "n" presses. I could imagine that it is okay-ish when it asks about the untracked files *after* all tracked paths have been processed (I guess this has been proposed before), so that I can safely quit. I am also not really sure what problem this feature is trying to solve. If the "problem"(?) is that it should act more like "git add" instead of "git add -u", for whatever reason, this may be fine (but the configuration option is a must-have then). For me, I often had the problem that I simply forgot to add new files...¹ Still I doubt that one would benefit from such a feature because either: - you do not have many untracked files (unlike me). You will see the untracked file (that should be tracked) in the "git status" output when you edit the commit message², then you abort-add-commit or commit-add-amend and everything is fine. Or: - you have a lot of untracked files (like me). You won't see the untracked file (that should be tracked) in the "git status" output (you ignore it because the list is so long) and you won't see it in the "git add -p" run because you quit before seeing that file. So I have mixed feelings... > I'd also probably add interactive.showUntracked to make the whole thing > optional (but I think it would be OK to default it to on). Hm, "interactive.showUntracked" is a confusing name because "git add -i" (interactive) already handles untracked files. Best Stephan ¹ I do not have that problem any more because I got used to add new files right after saving, in a very early state, and then I simply use "git add -p"... ² Unless you use git commit -m ... But using "git commit -m" without "git status" before is user-issue anyway.
Re: git add -p with new file
Hi Ariel, On 12/09/2016 07:26 PM, Ariel wrote: > On Wed, 7 Dec 2016, Duy Nguyen wrote: >> We could improve it a bit, suggesting the user to do git add -N. But >> is there a point of using -p on a new file? > > I got into the habit of always using -p as a way of checking my diffs > before committing, Not commenting on the main issue here, but if you want to check your new files with a "git add -p"-like tool, you can do git add git reset -p (this is unstaging, but it's still useful) Best Stephan
Re: [PATCH v2 4/5] Make sequencer abort safer
On 12/10/2016 09:04 PM, Jeff King wrote: > On Sat, Dec 10, 2016 at 08:56:26PM +0100, Christian Couder wrote: > >>> +static int rollback_is_safe(void) >>> +{ >>> + struct strbuf sb = STRBUF_INIT; >>> + struct object_id expected_head, actual_head; >>> + >>> + if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) { >>> + strbuf_trim(&sb); >>> + if (get_oid_hex(sb.buf, &expected_head)) { >>> + strbuf_release(&sb); >>> + die(_("could not parse %s"), >>> git_path_abort_safety_file()); >>> + } >>> + strbuf_release(&sb); >>> + } >> >> Maybe the following is a bit simpler: >> >>if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) { >>int res; >>strbuf_trim(&sb); >>res = get_oid_hex(sb.buf, &expected_head); >>strbuf_release(&sb); >>if (res) >>die(_("could not parse %s"), >> git_path_abort_safety_file()); >>} > > Is there any point in calling strbuf_release() if we're about to die > anyway? I could see it if it were "return error()", but it's normal in > our code base for die() to be abrupt. The point is that someone "libifies" the function some day; then "die()" becomes "return error()" almost automatically. Chances are high that the resulting memory leak is forgotten. That's one of the reasons why I like being strict about memory leaks. However, I cannot tell if mine or Christian's variant is really "simpler" (with whatever measure) and I also don't care much. ~Stephan
Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
On 12/09/2016 08:24 PM, Stephan Beyer wrote: > t3510 also shows another use-case for --quit: the title says it all: > "cherry-pick --quit" to "cherry-pick --abort" I should've read what I actually pasted. I wanted to paste: '--quit keeps HEAD and conflicted index intact' Sorry for making no sense ;) > With this additional information, I'd vote to keep --quit/--forget and > just make it consistent. Now! ~Stephan
Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
Hi Junio, On 12/09/2016 07:07 PM, Junio C Hamano wrote: > Duy Nguyen writes: >> Having the same operation with different names only increases git >> reputation of bad/inconsistent UI. Either forget is renamed to quit, >> or vice versa. I prefer forget, but the decision is yours and the >> community's. So I'm sending two patches to rename in either direction. >> You can pick one. > > I actually was advocating to remove both by making --abort saner. > With an updated --abort that behaves saner, is "rebase --forget" > still necessary? A quick change in t3407 of the "rebase --forget" test to use "rebase --abort" failed. That's because it checks the use-case of forgetting/aborting without changing the HEAD. So --abort makes a rollback, --forget just keeps the current head. I am not sure if that tested use-case is a real use-case though. A quick change in the pristine_detach function in t3510 and t3511 from "cherry-pick --quit" to "cherry-pick --abort" works when one ignores the return value of "cherry-pick --abort". The "--quit" is used here to ensure a clean cherry-pick state, and --quit always succeeds, even if no cherry-pick is in progress. That may be a real use-case somehow that could also be used for "rebase --forget" t3510 also shows another use-case for --quit: the title says it all: "cherry-pick --quit" to "cherry-pick --abort" With this additional information, I'd vote to keep --quit/--forget and just make it consistent. ~Stephan
[PATCH v2 4/5] Make sequencer abort safer
In contrast to "git am --abort", a sequencer abort did not check whether the current HEAD is the one that is expected. This can lead to loss of work (when not spotted and resolved using reflog before the garbage collector chimes in). This behavior is now changed by mimicking "git am --abort": the abortion is done but HEAD is not changed when the current HEAD is not the expected HEAD. A new file "sequencer/current" is added to save the expected HEAD. The new behavior is only active when --abort is invoked on multiple picks. The problem does not occur for the single-pick case because it is handled differently. Signed-off-by: Stephan Beyer --- sequencer.c | 48 + t/t3510-cherry-pick-sequence.sh | 2 +- 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 30b10ba14..35c158471 100644 --- a/sequencer.c +++ b/sequencer.c @@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer") static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo") static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts") static GIT_PATH_FUNC(git_path_head_file, "sequencer/head") +static GIT_PATH_FUNC(git_path_abort_safety_file, "sequencer/abort-safety") /* * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and @@ -310,6 +311,20 @@ static int error_dirty_index(struct replay_opts *opts) return -1; } +static void update_abort_safety_file(void) +{ + struct object_id head; + + /* Do nothing on a single-pick */ + if (!file_exists(git_path_seq_dir())) + return; + + if (!get_oid("HEAD", &head)) + write_file(git_path_abort_safety_file(), "%s", oid_to_hex(&head)); + else + write_file(git_path_abort_safety_file(), "%s", ""); +} + static int fast_forward_to(const unsigned char *to, const unsigned char *from, int unborn, struct replay_opts *opts) { @@ -339,6 +354,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from, strbuf_release(&sb); strbuf_release(&err); ref_transaction_free(transaction); + update_abort_safety_file(); return 0; } @@ -813,6 +829,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, leave: free_message(commit, &msg); + update_abort_safety_file(); return res; } @@ -1132,6 +1149,30 @@ static int save_head(const char *head) return 0; } +static int rollback_is_safe(void) +{ + struct strbuf sb = STRBUF_INIT; + struct object_id expected_head, actual_head; + + if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) { + strbuf_trim(&sb); + if (get_oid_hex(sb.buf, &expected_head)) { + strbuf_release(&sb); + die(_("could not parse %s"), git_path_abort_safety_file()); + } + strbuf_release(&sb); + } + else if (errno == ENOENT) + oidclr(&expected_head); + else + die_errno(_("could not read '%s'"), git_path_abort_safety_file()); + + if (get_oid("HEAD", &actual_head)) + oidclr(&actual_head); + + return !oidcmp(&actual_head, &expected_head); +} + static int reset_for_rollback(const unsigned char *sha1) { const char *argv[4];/* reset --merge + NULL */ @@ -1189,6 +1230,12 @@ int sequencer_rollback(struct replay_opts *opts) error(_("cannot abort from a branch yet to be born")); goto fail; } + + if (!rollback_is_safe()) { + /* Do not error, just do not rollback */ + warning(_("You seem to have moved HEAD. " + "Not rewinding, check your HEAD!")); + } else if (reset_for_rollback(sha1)) goto fail; strbuf_release(&buf); @@ -1393,6 +1440,7 @@ int sequencer_pick_revisions(struct replay_opts *opts) return -1; if (save_opts(opts)) return -1; + update_abort_safety_file(); res = pick_commits(&todo_list, opts); todo_list_release(&todo_list); return res; diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index efcd4fc48..372307c21 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -147,7 +147,7 @@ test_expect_success '--abort to cancel single cherry-pick' ' git diff-index --exit-code HEAD ' -test_expect_failure '--abort does not unsafely change HEAD' ' +test_expect_success '--abort does not unsafely change HEAD' ' pristine_detach initial && test_must_fail git cherry-pick picked anotherpick && git reset --hard base && -- 2.11.0.27.g74d6bea
[PATCH v2 5/5] sequencer: Remove useless get_dir() function
This function is used only once, for the removal of the directory. It is not used for the creation of the directory nor anywhere else. Signed-off-by: Stephan Beyer --- sequencer.c | 7 +-- 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/sequencer.c b/sequencer.c index 35c158471..aba096a0a 100644 --- a/sequencer.c +++ b/sequencer.c @@ -47,11 +47,6 @@ static inline int is_rebase_i(const struct replay_opts *opts) return 0; } -static const char *get_dir(const struct replay_opts *opts) -{ - return git_path_seq_dir(); -} - static const char *get_todo_path(const struct replay_opts *opts) { return git_path_todo_file(); @@ -160,7 +155,7 @@ int sequencer_remove_state(struct replay_opts *opts) free(opts->xopts[i]); free(opts->xopts); - strbuf_addf(&dir, "%s", get_dir(opts)); + strbuf_addf(&dir, "%s", git_path_seq_dir()); remove_dir_recursively(&dir, 0); strbuf_release(&dir); -- 2.11.0.27.g74d6bea
[PATCH v2 3/5] Add test that cherry-pick --abort does not unsafely change HEAD
The test expects failure because it is a current breakage reported by Junio C Hamano. Signed-off-by: Stephan Beyer --- t/t3510-cherry-pick-sequence.sh | 10 ++ 1 file changed, 10 insertions(+) diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index 7b7a89dbd..efcd4fc48 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -147,6 +147,16 @@ test_expect_success '--abort to cancel single cherry-pick' ' git diff-index --exit-code HEAD ' +test_expect_failure '--abort does not unsafely change HEAD' ' + pristine_detach initial && + test_must_fail git cherry-pick picked anotherpick && + git reset --hard base && + test_must_fail git cherry-pick picked anotherpick && + git cherry-pick --abort 2>actual && + test_i18ngrep "You seem to have moved HEAD" actual && + test_cmp_rev base HEAD +' + test_expect_success 'cherry-pick --abort to cancel multiple revert' ' pristine_detach anotherpick && test_expect_code 1 git revert base..picked && -- 2.11.0.27.g74d6bea
[PATCH v2 1/5] am: Fix filename in safe_to_abort() error message
Signed-off-by: Stephan Beyer --- builtin/am.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/am.c b/builtin/am.c index 6981f42ce..7cf40e6f2 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -2124,7 +2124,7 @@ static int safe_to_abort(const struct am_state *state) if (read_state_file(&sb, state, "abort-safety", 1) > 0) { if (get_oid_hex(sb.buf, &abort_safety)) - die(_("could not parse %s"), am_path(state, "abort_safety")); + die(_("could not parse %s"), am_path(state, "abort-safety")); } else oidclr(&abort_safety); -- 2.11.0.27.g74d6bea
[PATCH v2 2/5] am: Change safe_to_abort()'s not rewinding error into a warning
The error message tells the user that something went terribly wrong and the --abort could not be performed. But the --abort is performed, only without rewinding. By simply changing the error into a warning, we indicate the user that she must not try something like "git am --abort --force", instead she just has to check the HEAD. Signed-off-by: Stephan Beyer --- builtin/am.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/am.c b/builtin/am.c index 7cf40e6f2..826f18ba1 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -2134,7 +2134,7 @@ static int safe_to_abort(const struct am_state *state) if (!oidcmp(&head, &abort_safety)) return 1; - error(_("You seem to have moved HEAD since the last 'am' failure.\n" + warning(_("You seem to have moved HEAD since the last 'am' failure.\n" "Not rewinding to ORIG_HEAD")); return 0; -- 2.11.0.27.g74d6bea
Re: [PATCH 4/5] Make sequencer abort safer
Hi, I'm a little afraid of feeding Parkinson's law of triviality here, but... ;) On 12/08/2016 06:27 PM, Junio C Hamano wrote: > Johannes Schindelin writes: > >> On Wed, 7 Dec 2016, Stephan Beyer wrote: >> >>> diff --git a/sequencer.c b/sequencer.c >>> index 30b10ba14..c9b560ac1 100644 >>> --- a/sequencer.c >>> +++ b/sequencer.c >>> @@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer") >>> static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo") >>> static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts") >>> static GIT_PATH_FUNC(git_path_head_file, "sequencer/head") >>> +static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current") >> >> Is it required by law to have a four-letter infix, or can we have a nicer >> variable name (e.g. git_path_current_file)? > > I agree with you that, as other git_path_*_file variables match the > actual name on the filesystem, this one should too, together with > the update_curr_file() function. I totally agree with that (and I don't know why I used "curr", probably just because it looked consistent and good...). However: > -static void update_curr_file() > +static void update_current_file(void) This function name could lead to the impression that there is some current file (defined by a global state or whatever) that is updated. So I'd rather rename the *file* to one of * sequencer/abort-safety (consistent to am, describes its purpose) * sequencer/safety (shorter, still describes the purpose) * sequencer/current-head (describes what it contains) * sequencer/last (a four-letter word, not totally unambiguous though) > By the way, this step seems to be a fix to an existing problem, and > the new test added in 3/5 seems to be a demonstration of the issue. > If that is the case, shouldn't the new test initially expect failure > and updated by this step to expect success? That's usually a matter of taste that I sometimes also discuss with colleagues in other projects... However, for the git test suite with its "known breakage" behavior, your recommendation is surely the best way to do it (aside from introducing the test and the fix in one commit... but that does not show in the history that there actually was that breakage) ~Stephan
[PATCH 4/5] Make sequencer abort safer
In contrast to "git am --abort", a sequencer abort did not check whether the current HEAD is the one that is expected. This can lead to loss of work (when not spotted and resolved using reflog before the garbage collector chimes in). This behavior is now changed by mimicking "git am --abort": the abortion is done but HEAD is not changed when the current HEAD is not the expected HEAD. A new file "sequencer/current" is added to save the expected HEAD. The new behavior is only active when --abort is invoked on multiple picks. The problem does not occur for the single-pick case because it is handled differently. Signed-off-by: Stephan Beyer --- sequencer.c | 49 + 1 file changed, 49 insertions(+) diff --git a/sequencer.c b/sequencer.c index 30b10ba14..c9b560ac1 100644 --- a/sequencer.c +++ b/sequencer.c @@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer") static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo") static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts") static GIT_PATH_FUNC(git_path_head_file, "sequencer/head") +static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current") /* * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and @@ -310,6 +311,20 @@ static int error_dirty_index(struct replay_opts *opts) return -1; } +static void update_curr_file() +{ + struct object_id head; + + /* Do nothing on a single-pick */ + if (!file_exists(git_path_seq_dir())) + return; + + if (!get_oid("HEAD", &head)) + write_file(git_path_curr_file(), "%s", oid_to_hex(&head)); + else + write_file(git_path_curr_file(), "%s", ""); +} + static int fast_forward_to(const unsigned char *to, const unsigned char *from, int unborn, struct replay_opts *opts) { @@ -339,6 +354,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from, strbuf_release(&sb); strbuf_release(&err); ref_transaction_free(transaction); + update_curr_file(); return 0; } @@ -813,6 +829,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, leave: free_message(commit, &msg); + update_curr_file(); return res; } @@ -1132,9 +1149,34 @@ static int save_head(const char *head) return 0; } +static int rollback_is_safe() +{ + struct strbuf sb = STRBUF_INIT; + struct object_id expected_head, actual_head; + + if (strbuf_read_file(&sb, git_path_curr_file(), 0) >= 0) { + strbuf_trim(&sb); + if (get_oid_hex(sb.buf, &expected_head)) { + strbuf_release(&sb); + die(_("could not parse %s"), git_path_curr_file()); + } + strbuf_release(&sb); + } + else if (errno == ENOENT) + oidclr(&expected_head); + else + die_errno(_("could not read '%s'"), git_path_curr_file()); + + if (get_oid("HEAD", &actual_head)) + oidclr(&actual_head); + + return !oidcmp(&actual_head, &expected_head); +} + static int reset_for_rollback(const unsigned char *sha1) { const char *argv[4];/* reset --merge + NULL */ + argv[0] = "reset"; argv[1] = "--merge"; argv[2] = sha1_to_hex(sha1); @@ -1189,6 +1231,12 @@ int sequencer_rollback(struct replay_opts *opts) error(_("cannot abort from a branch yet to be born")); goto fail; } + + if (!rollback_is_safe()) { + /* Do not error, just do not rollback */ + warning(_("You seem to have moved HEAD. " + "Not rewinding, check your HEAD!")); + } else if (reset_for_rollback(sha1)) goto fail; strbuf_release(&buf); @@ -1393,6 +1441,7 @@ int sequencer_pick_revisions(struct replay_opts *opts) return -1; if (save_opts(opts)) return -1; + update_curr_file(); res = pick_commits(&todo_list, opts); todo_list_release(&todo_list); return res; -- 2.11.0.27.g4eed97c
[PATCH 3/5] Add test that cherry-pick --abort does not unsafely change HEAD
Signed-off-by: Stephan Beyer --- t/t3510-cherry-pick-sequence.sh | 10 ++ 1 file changed, 10 insertions(+) diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index 7b7a89dbd..372307c21 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -147,6 +147,16 @@ test_expect_success '--abort to cancel single cherry-pick' ' git diff-index --exit-code HEAD ' +test_expect_success '--abort does not unsafely change HEAD' ' + pristine_detach initial && + test_must_fail git cherry-pick picked anotherpick && + git reset --hard base && + test_must_fail git cherry-pick picked anotherpick && + git cherry-pick --abort 2>actual && + test_i18ngrep "You seem to have moved HEAD" actual && + test_cmp_rev base HEAD +' + test_expect_success 'cherry-pick --abort to cancel multiple revert' ' pristine_detach anotherpick && test_expect_code 1 git revert base..picked && -- 2.11.0.27.g4eed97c
[PATCH 1/5] am: Fix filename in safe_to_abort() error message
Signed-off-by: Stephan Beyer --- Okay let's give it a try. Some minor things that I found are also in this patchset (patch 01, 02 and 05). The branch can also be found on https://github.com/sbeyer/git/commits/sequencer-abort-safety builtin/am.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/am.c b/builtin/am.c index 6981f42ce..7cf40e6f2 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -2124,7 +2124,7 @@ static int safe_to_abort(const struct am_state *state) if (read_state_file(&sb, state, "abort-safety", 1) > 0) { if (get_oid_hex(sb.buf, &abort_safety)) - die(_("could not parse %s"), am_path(state, "abort_safety")); + die(_("could not parse %s"), am_path(state, "abort-safety")); } else oidclr(&abort_safety); -- 2.11.0.27.g4eed97c
[PATCH 5/5] sequencer: Remove useless get_dir() function
This function is used only once, for the removal of the directory. It is not used for the creation of the directory nor anywhere else. Signed-off-by: Stephan Beyer --- sequencer.c | 7 +-- 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/sequencer.c b/sequencer.c index c9b560ac1..689cfa5f1 100644 --- a/sequencer.c +++ b/sequencer.c @@ -47,11 +47,6 @@ static inline int is_rebase_i(const struct replay_opts *opts) return 0; } -static const char *get_dir(const struct replay_opts *opts) -{ - return git_path_seq_dir(); -} - static const char *get_todo_path(const struct replay_opts *opts) { return git_path_todo_file(); @@ -160,7 +155,7 @@ int sequencer_remove_state(struct replay_opts *opts) free(opts->xopts[i]); free(opts->xopts); - strbuf_addf(&dir, "%s", get_dir(opts)); + strbuf_addf(&dir, "%s", git_path_seq_dir()); remove_dir_recursively(&dir, 0); strbuf_release(&dir); -- 2.11.0.27.g4eed97c
[PATCH 2/5] am: Change safe_to_abort()'s not rewinding error into a warning
The error message tells the user that something went terribly wrong and the --abort could not be performed. But the --abort is performed, only without rewinding. By simply changing the error into a warning, we indicate the user that she must not try something like "git am --abort --force", instead she just has to check the HEAD. Signed-off-by: Stephan Beyer --- builtin/am.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/am.c b/builtin/am.c index 7cf40e6f2..826f18ba1 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -2134,7 +2134,7 @@ static int safe_to_abort(const struct am_state *state) if (!oidcmp(&head, &abort_safety)) return 1; - error(_("You seem to have moved HEAD since the last 'am' failure.\n" + warning(_("You seem to have moved HEAD since the last 'am' failure.\n" "Not rewinding to ORIG_HEAD")); return 0; -- 2.11.0.27.g4eed97c
Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
Hi, On 12/07/2016 09:04 PM, Junio C Hamano wrote: > Stephan Beyer writes: > >> [1] By the way: git cherry-pick --quit, git rebase --forget ... >> different wording for the same thing makes things unintuitive. > > It is not too late to STOP "--forget" from getting added to "rebase" > and give it a better name. Oh. ;) I am not sure. I personally think that --forget is a better name than --quit because when I hear --quit I tend to look into the manual page first to check if there are weird side effects (and then the manual page says that it "forgets" ;D). So I'd rather favor adding --forget to cherry-pick/revert instead... or this: > Having said that, I have a feeling that these options do not have to > exist; isn't their presence just a symptom that the "--abort" for > the command misbehaves? Isn't the reason why there is no need for > "am --quit" because its "--abort" behaves more sensibly? You're probably right. I have no other use-case in mind than "oh I forgot that I was rebasing... now just abort that and don't bother me further (i.e. please don't bring me back)" ~Stephan
Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
Hi, On 12/06/2016 07:58 PM, Junio C Hamano wrote: > I was burned a few times with this in the past few years, but it did > not irritate me often enough that I didn't write it down. But I > think this is serious enough that deserves attention from those who > were involved. > > A short reproduction recipe, using objects from git.git that are > publicly available and stable, shows how bad it is. > > $ git checkout v2.9.3^0 > > $ git cherry-pick 0582a34f52..a94bb68397 > ... see conflict, decide to give up backporting to > ... such an old fork point. > ... the git-prompt gives "|CHERRY-PICKING" correctly. > > $ git reset --hard v2.10.2^0 > ... the git-prompt no longer says "|CHERRY-PICKING" > > $ edit && git commit -m "prelim work for backporting" > [detached HEAD cc5a6a9219] prelim work for backporting > > $ git cherry-pick 0582a34f52..a94bb68397 > error: a cherry-pick or revert is already in progress > hint: try "git cherry-pick (--continue | --quit | --abort)" > fatal: cherry-pick failed > > $ git cherry-pick --abort > ... we come back to v2.9.3^0, losing the new commit! Apart from the git-prompt bug: isn't this a user error? I think "git cherry-pick --quit"[1] would be the right thing to do, not --abort. On the other hand, one (as a user) could also expect that "git reset --hard" also resets sequencer-related states (and that is what the git-prompt suggests), but that would probably break a lot of scripts ;) [1] By the way: git cherry-pick --quit, git rebase --forget ... different wording for the same thing makes things unintuitive. > (1) The third invocation of "cherry-pick" with "--abort" to get rid > of the state from the unfinished cherry-pick we did previously > is necessary, but the command does not notice that we resetted > to a new branch AND we even did some other work there. This > loses end-user's work. > > "git cherry-pick --abort" should learn from "git am --abort" > that has an extra safety to deal with the above workflow. The > state from the unfinished "am" is removed, but the head is not > rewound to avoid losing end-user's work. > > You can try by replacing two instances of > > $ git cherry-pick 0582a34f52..a94bb68397 > > with > > $ git format-patch --stdout 0582a34f52..a94bb68397 | git am > > in the above sequence, and conclude with "git am--abort" to see > how much more pleasant and safe "git am --abort" is. Definitely. I'd volunteer to add that safety guard. (But (2) remains.) ~Stephan
Re: [PATCH v15 19/27] bisect--helper: `bisect_state` & `bisect_head` shell function in C
Hi Pranit, On 12/06/2016 11:40 PM, Pranit Bauva wrote: > On Tue, Nov 22, 2016 at 5:42 AM, Stephan Beyer wrote: >> On 10/14/2016 04:14 PM, Pranit Bauva wrote: >>> +static int bisect_state(struct bisect_terms *terms, const char **argv, >>> + int argc) >>> +{ >>> + const char *state = argv[0]; >>> + >>> + get_terms(terms); >>> + if (check_and_set_terms(terms, state)) >>> + return -1; >>> + >>> + if (!argc) >>> + die(_("Please call `--bisect-state` with at least one >>> argument")); >> >> I think this check should move to cmd_bisect__helper. There are also the >> other argument number checks. > > Not really. After the whole conversion, the cmdmode will cease to > exists while bisect_state will be called directly, thus it is > important to check it here. Okay, that's a point. In that case, you should probably use "return error()" again and the message (mentioning "--bisect-state") does not make sense when --bisect-state ceases to exist. >>> + >>> + if (argc == 1 && one_of(state, terms->term_good, >>> + terms->term_bad, "skip", NULL)) { >>> + const char *bisected_head = xstrdup(bisect_head()); >>> + const char *hex[1]; >> >> Maybe: >> const char *hex; >> >>> + unsigned char sha1[20]; >>> + >>> + if (get_sha1(bisected_head, sha1)) >>> + die(_("Bad rev input: %s"), bisected_head); >> >> And instead of... >> >>> + if (bisect_write(state, sha1_to_hex(sha1), terms, 0)) >>> + return -1; >>> + >>> + *hex = xstrdup(sha1_to_hex(sha1)); >>> + if (check_expected_revs(hex, 1)) >>> + return -1; >> >> ... simply: >> >> hex = xstrdup(sha1_to_hex(sha1)); >> if (set_state(terms, state, hex)) { >> free(hex); >> return -1; >> } >> free(hex); >> >> where: > > Yes I am planning to convert all places with hex rather than the sha1 > but not yet, maybe in an another patch series because currently a lot > of things revolve around sha1 and changing its behaviour wouldn't > really be a part of a porting patch series. I was not suggesting a change of behavior, I was suggesting a simple helper function set_state() to get rid of code duplication above and some lines below: >> ... And replace this: >> >>> + const char **hex_string = (const char **) >>> &hex.items[i].string; >>> + if(bisect_write(state, *hex_string, terms, 0)) { >>> + string_list_clear(&hex, 0); >>> + return -1; >>> + } >>> + if (check_expected_revs(hex_string, 1)) { >>> + string_list_clear(&hex, 0); >>> + return -1; >>> + } >> >> by: >> >> const char *hex_str = hex.items[i].string; >> if (set_state(terms, state, hex_string)) { >> string_list_clear(&hex, 0); >> return -1; >> } See? >>> @@ -184,8 +137,8 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2 >>> state="$TERM_GOOD" >>> fi >>> >>> - # We have to use a subshell because "bisect_state" can exit. >>> - ( bisect_state $state >"$GIT_DIR/BISECT_RUN" ) >>> + # We have to use a subshell because "--bisect-state" can exit. >>> + ( git bisect--helper --bisect-state $state >>> >"$GIT_DIR/BISECT_RUN" ) >> >> The new comment is funny, but you don't need a subshell here any >> longer. > > True, but right now I didn't want to modify that part of the source > code to remove the comment. I will remove the comment all together > when I port bisect_run() For most of the patches, I was commenting on the current state, not on the big picture. Still I think that it is better to remove the comment and the subshell instead of making the comment weird ("yes the builtin can exit, but why do we need a subshell?" vs "yes the shell function calls exit, so we need a subshell because we do not want to exit this shell script") ~Stephan
Re: [PATCH v15 23/27] bisect--helper: `bisect_replay` shell function in C
Hi Pranit and Christian and Lars ;) On 12/07/2016 12:02 AM, Pranit Bauva wrote: > On Tue, Nov 22, 2016 at 6:19 AM, Stephan Beyer wrote: >> Okay Pranit, >> >> this is the last patch for me to review in this series. >> >> Now that I have a coarse overview of what you did, I have the general >> remark that imho the "terms" variable should simply be global instead of >> being passed around all the time. > > In a personal conversation with my mentors, we thought it is the best > fit to keep it in a struct and pass it around so that it is easier in > libification. I guess you had that conversation at the beginning of the project and I think that at that stage I would have recommended it that way, too. However, looking at the v15 bisect--helper.c, it looks like it is rather a pain to do it that way. I mean, you use the terms like they were global variables: you initialize them in cmd_bisect__helper() and then you always pass them around; you update them "globally", never using restricted scope, never ever use a second instantiation besides the one of cmd_bisect__helper(). These are all signs that *in the current code* using (static) global variables is the better way (making the code simpler). The libification argument may be worth considering, though. I am not sure if there is really a use-case where you need two different instantiations of the terms, *except* if the lib allows bisecting with different terms at the same time (in different repos, of course). Is the lib "aware" of such use-cases like doing stuff in different repos at the same time? If that's the case, not using global variables is the right thing to do. In any other case I can imagine, I'd suggest to use global variables. ~Stephan
Re: [PATCH v15 23/27] bisect--helper: `bisect_replay` shell function in C
Hey Pranit, On 12/07/2016 12:02 AM, Pranit Bauva wrote: >>> +static int bisect_replay(struct bisect_terms *terms, const char *filename) >>> +{ >>> + struct strbuf line = STRBUF_INIT; >>> + struct strbuf word = STRBUF_INIT; >>> + FILE *fp = NULL; >> >> (The initialization is not necessary here.) > > Um. I think it is. Otherwise if it goes to the finish block before you > try to operate on fp, it will cause a seg fault. You are right, thanks! >>> + while (strbuf_getline(&line, fp) != EOF) { >>> + int pos = 0; >>> + while (pos < line.len) { >>> + pos = get_next_word(line.buf, pos, &word); >>> + >>> + if (!strcmp(word.buf, "git")) { >>> + continue; >>> + } else if (!strcmp(word.buf, "git-bisect")) { >>> + continue; >>> + } else if (!strcmp(word.buf, "bisect")) { >>> + continue; >>> + } else if (!strcmp(word.buf, "#")) { >>> + break; >> >> Maybe it is more robust to check whether word.buf begins with # > > Assuming that you meant "# ", yes. No, if I get it right "# " can never occur because the word.buf never contains a space. What I meant was that you are currently ignoring everything after a "# ", so comments like # foo are ignored. However, imagine a user changes the file by hand (he probably should not do it but, hey, it's git: unixy, hacky ... and he thinks he knows what he does) and then we have in the file something like #foo which makes perfectly sense when you are used to programming languages with # as comment-till-eol marker. The problem is that your current code does expect "#" as a single word and would hence not recognize #foo as a comment. I hope I made it clear why I suggested to test if the word *begins* with "#" (not "# "). ~Stephan
Re: [PATCH v15 12/27] bisect--helper: `get_terms` & `bisect_terms` shell function in C
Hey Pranit, On 12/06/2016 10:14 PM, Pranit Bauva wrote: >>> + >>> + if (argc == 0) { >>> + printf(_("Your current terms are %s for the old state\nand " >>> +"%s for the new state.\n"), terms->term_good, >>> +terms->term_bad); >> >> Very minor: It improves the readability if you'd split the string after >> the \n and put the "and "in the next line. > > Ah. This is because of the message. If I do the other way, then it > won't match the output in one of the tests in t/t6030 thus, I am > keeping it that way in order to avoid modifying the file t/t6030. I think I was unclear here. I was referring to the coding/layouting style, not to the string. I mean like writing: printf(_("Your current terms are %s for the old state\n" "and "%s for the new state.\n"), terms->term_good, terms->term_bad); The string fed to _() is the same, but it is split in a different (imho more readable) way: after the "\n", not after the "and ". >>> + die(_("invalid argument %s for 'git bisect " >>> + "terms'.\nSupported options are: " >>> + "--term-good|--term-old and " >>> + "--term-bad|--term-new."), argv[i]); >> >> Hm, "return error(...)" and "die(...)" seems to be quasi-equivalent in >> this case. Because I am always looking from a library perspective, I'd >> prefer "return error(...)". > > I should use return error() When you reroll your patches, please also check if you always put _() around your error()s ;) (Hmmm... On the other hand, it might be arguable if translations are useful for errors that only occur when people hack git-bisect or use the bisect--helper directly... This makes me feel like all those errors should be prefixed by some "BUG: " marker since the ordinary user only sees them when there is a bug. But I don't feel in the position to decide or recommend such a thing, so it's just a thought.) ~Stephan
Re: [PATCH v15 23/27] bisect--helper: `bisect_replay` shell function in C
Okay Pranit, this is the last patch for me to review in this series. Now that I have a coarse overview of what you did, I have the general remark that imho the "terms" variable should simply be global instead of being passed around all the time. I also had some other remarks but I forgot them... maybe they come to my mind again when I see patch series v16. I also want to remark again that I am not a Git developer and only reviewed this because of my interest in git-bisect. So some of my suggestions might conflict with other beliefs here. For example, I consider it very bad style to leak memory... but Git is rather written as a scripting tool than a genuine library, so perhaps many people here do not care about it as long as it works... On 10/14/2016 04:14 PM, Pranit Bauva wrote: > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index c18ca07..b367d8d 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -601,7 +602,6 @@ static int bisect_start(struct bisect_terms *terms, int > no_checkout, > terms->term_good = arg; > } else if (!strcmp(arg, "--term-bad") || >!strcmp(arg, "--term-new")) { > - const char *next_arg; This should already have been removed in patch 15/27, not here. > @@ -875,6 +875,117 @@ static int bisect_log(void) > return status; > } > > +static int get_next_word(const char *line, int pos, struct strbuf *word) > +{ > + int i, len = strlen(line), begin = 0; > + strbuf_reset(word); > + for (i = pos; i < len; i++) { > + if (line[i] == ' ' && begin) > + return i + 1; > + > + if (!begin) > + begin = 1; > + strbuf_addch(word, line[i]); > + } > + > + return i; > +} > + > +static int bisect_replay(struct bisect_terms *terms, const char *filename) > +{ > + struct strbuf line = STRBUF_INIT; > + struct strbuf word = STRBUF_INIT; > + FILE *fp = NULL; (The initialization is not necessary here.) > + int res = 0; > + > + if (is_empty_or_missing_file(filename)) { > + error(_("no such file with name '%s' exists"), filename); The error message is misleading if the file exists but is empty. Maybe something like "cannot read file '%s' for replaying"? > + res = -1; > + goto finish; goto fail; :D > + } > + > + if (bisect_reset(NULL)) { > + res = -1; > + goto finish; goto fail; > + } > + > + fp = fopen(filename, "r"); > + if (!fp) { > + res = -1; > + goto finish; goto fail; > + } > + > + while (strbuf_getline(&line, fp) != EOF) { > + int pos = 0; > + while (pos < line.len) { > + pos = get_next_word(line.buf, pos, &word); > + > + if (!strcmp(word.buf, "git")) { > + continue; > + } else if (!strcmp(word.buf, "git-bisect")) { > + continue; > + } else if (!strcmp(word.buf, "bisect")) { > + continue; > + } else if (!strcmp(word.buf, "#")) { > + break; Maybe it is more robust to check whether word.buf begins with # > + } > + > + get_terms(terms); > + if (check_and_set_terms(terms, word.buf)) { > + res = -1; > + goto finish; goto fail; > + } > + > + if (!strcmp(word.buf, "start")) { > + struct argv_array argv = ARGV_ARRAY_INIT; > + sq_dequote_to_argv_array(line.buf+pos, &argv); > + if (bisect_start(terms, 0, argv.argv, > argv.argc)) { > + argv_array_clear(&argv); > + res = -1; > + goto finish; goto fail; > + } > + argv_array_clear(&argv); > + break; > + } > + > + if (one_of(word.buf, terms->term_good, > + terms->term_bad, "skip", NULL)) { > + if (bisect_write(word.buf, line.buf+pos, terms, > 0)) { > + res = -1; > + goto finish; goto fail; > + } > + break; > + } > + > + if (!strcmp(word.buf, "terms")) { > + struct argv_array argv = ARGV_ARRAY_INIT;
Re: [PATCH v15 19/27] bisect--helper: `bisect_state` & `bisect_head` shell function in C
Hi, On 10/14/2016 04:14 PM, Pranit Bauva wrote: > Reimplement the `bisect_state` shell function in C and also add a > subcommand `--bisect-state` to `git-bisect--helper` to call it from > git-bisect.sh . > > Using `--bisect-state` subcommand is a temporary measure to port shell > function to C so as to use the existing test suite. As more functions > are ported, this subcommand will be retired and will be called by some > other methods. > > `bisect_head` is called from `bisect_state` thus its not required to > introduce another subcommand. Missing comma before "thus", and "it is" (or "it's") instead of "its" :) > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 1767916..1481c6d 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -784,6 +786,79 @@ static int bisect_autostart(struct bisect_terms *terms) > return 0; > } > > +static char *bisect_head(void) > +{ > + if (is_empty_or_missing_file(git_path_bisect_head())) > + return "HEAD"; > + else > + return "BISECT_HEAD"; > +} This is very shellish. In C I'd expect something like static int bisect_head_sha1(unsigned char *sha) { int res; if (is_empty_or_missing_file(git_path_bisect_head())) res = get_sha1("HEAD", sha); else res = get_sha1("BISECT_HEAD", sha); if (res) return error(_("Could not find BISECT_HEAD or HEAD.")); return 0; } > + > +static int bisect_state(struct bisect_terms *terms, const char **argv, > + int argc) > +{ > + const char *state = argv[0]; > + > + get_terms(terms); > + if (check_and_set_terms(terms, state)) > + return -1; > + > + if (!argc) > + die(_("Please call `--bisect-state` with at least one > argument")); I think this check should move to cmd_bisect__helper. There are also the other argument number checks. > + > + if (argc == 1 && one_of(state, terms->term_good, > + terms->term_bad, "skip", NULL)) { > + const char *bisected_head = xstrdup(bisect_head()); > + const char *hex[1]; Maybe: const char *hex; > + unsigned char sha1[20]; > + > + if (get_sha1(bisected_head, sha1)) > + die(_("Bad rev input: %s"), bisected_head); And instead of... > + if (bisect_write(state, sha1_to_hex(sha1), terms, 0)) > + return -1; > + > + *hex = xstrdup(sha1_to_hex(sha1)); > + if (check_expected_revs(hex, 1)) > + return -1; ... simply: hex = xstrdup(sha1_to_hex(sha1)); if (set_state(terms, state, hex)) { free(hex); return -1; } free(hex); where: static int set_state(struct bisect_terms *terms, const char *state, const char *hex) { if (bisect_write(state, hex, terms, 0)) return -1; if (check_expected_revs(&hex, 1)) return -1; return 0; } > + return bisect_auto_next(terms, NULL); > + } > + > + if ((argc == 2 && !strcmp(state, terms->term_bad)) || > + one_of(state, terms->term_good, "skip", NULL)) { > + int i; > + struct string_list hex = STRING_LIST_INIT_DUP; > + > + for (i = 1; i < argc; i++) { > + unsigned char sha1[20]; > + > + if (get_sha1(argv[i], sha1)) { > + string_list_clear(&hex, 0); > + die(_("Bad rev input: %s"), argv[i]); > + } > + string_list_append(&hex, sha1_to_hex(sha1)); > + } > + for (i = 0; i < hex.nr; i++) { ... And replace this: > + const char **hex_string = (const char **) > &hex.items[i].string; > + if(bisect_write(state, *hex_string, terms, 0)) { > + string_list_clear(&hex, 0); > + return -1; > + } > + if (check_expected_revs(hex_string, 1)) { > + string_list_clear(&hex, 0); > + return -1; > + } by: const char *hex_str = hex.items[i].string; if (set_state(terms, state, hex_string)) { string_list_clear(&hex, 0); return -1; } > + } > + string_list_clear(&hex, 0); > + return bisect_auto_next(terms, NULL); > + } > + > + if (!strcmp(state, terms->term_bad)) > + die(_("'git bisect %s' can take only one argument."), > + terms->term_bad); > + > + re
Re: [PATCH v15 15/27] bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C
Hi Pranit, in this mail I review the "second part" of your patch: the transition of bisect_next and bisect_auto_next to C. On 10/14/2016 04:14 PM, Pranit Bauva wrote: > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 1d3e17f..fcd7574 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -408,6 +411,136 @@ static int bisect_terms(struct bisect_terms *terms, > const char **argv, int argc) > return 0; > } > > +static int register_good_ref(const char *refname, > + const struct object_id *oid, int flags, > + void *cb_data) > +{ > + struct string_list *good_refs = cb_data; > + string_list_append(good_refs, oid_to_hex(oid)); > + return 0; > +} > + > +static int bisect_next(struct bisect_terms *terms, const char *prefix) > +{ > + int res, no_checkout; > + > + /* > + * In case of mistaken revs or checkout error, or signals received, > + * "bisect_auto_next" below may exit or misbehave. > + * We have to trap this to be able to clean up using > + * "bisect_clean_state". > + */ The comment above makes no sense here, or does it? > + if (bisect_next_check(terms, terms->term_good)) > + return -1; > + > + no_checkout = !is_empty_or_missing_file(git_path_bisect_head()); > + > + /* Perform all bisection computation, display and checkout */ > + res = bisect_next_all(prefix , no_checkout); Style: there is a space left of the comma. > + > + if (res == 10) { > + FILE *fp = NULL; > + unsigned char sha1[20]; > + struct commit *commit; > + struct pretty_print_context pp = {0}; > + struct strbuf commit_name = STRBUF_INIT; > + char *bad_ref = xstrfmt("refs/bisect/%s", > + terms->term_bad); > + int retval = 0; > + > + read_ref(bad_ref, sha1); > + commit = lookup_commit_reference(sha1); > + format_commit_message(commit, "%s", &commit_name, &pp); > + fp = fopen(git_path_bisect_log(), "a"); > + if (!fp) { > + retval = -1; > + goto finish_10; > + } > + if (fprintf(fp, "# first %s commit: [%s] %s\n", > + terms->term_bad, sha1_to_hex(sha1), > + commit_name.buf) < 1){ > + retval = -1; > + goto finish_10; > + } > + goto finish_10; > + finish_10: > + if (fp) > + fclose(fp); > + strbuf_release(&commit_name); > + free(bad_ref); > + return retval; > + } > + else if (res == 2) { > + FILE *fp = NULL; > + struct rev_info revs; > + struct argv_array rev_argv = ARGV_ARRAY_INIT; > + struct string_list good_revs = STRING_LIST_INIT_DUP; > + struct pretty_print_context pp = {0}; > + struct commit *commit; > + char *term_good = xstrfmt("%s-*", terms->term_good); > + int i, retval = 0; > + > + fp = fopen(git_path_bisect_log(), "a"); > + if (!fp) { > + retval = -1; > + goto finish_2; > + } > + if (fprintf(fp, "# only skipped commits left to test\n") < 1) { > + retval = -1; > + goto finish_2; > + } > + for_each_glob_ref_in(register_good_ref, term_good, > + "refs/bisect/", (void *) &good_revs); > + > + argv_array_pushl(&rev_argv, "skipped_commits", > "refs/bisect/bad", "--not", NULL); > + for (i = 0; i < good_revs.nr; i++) > + argv_array_push(&rev_argv, good_revs.items[i].string); > + > + /* It is important to reset the flags used by revision walks > + * as the previous call to bisect_next_all() in turn > + * setups a revision walk. > + */ > + reset_revision_walk(); > + init_revisions(&revs, NULL); > + rev_argv.argc = setup_revisions(rev_argv.argc, rev_argv.argv, > &revs, NULL); > + argv_array_clear(&rev_argv); > + string_list_clear(&good_revs, 0); > + if (prepare_revision_walk(&revs)) > + die(_("revision walk setup failed\n")); > + > + while ((commit = get_revision(&revs)) != NULL) { > + struct strbuf commit_name = STRBUF_INIT; > + format_commit_message(commit, "%s", > + &commit_name, &pp); > + fprintf(fp, "# possible first %s commit: " > + "[%s] %s\n", terms->term_bad, > + oid_to_hex(&commit->object.
Re: [PATCH v15 13/27] bisect--helper: `bisect_start` shell function partially in C
On 11/20/2016 09:01 PM, Stephan Beyer wrote: > First, replace the current set_terms() by > > static void set_terms(struct bisect_terms *terms, const char *bad, > const char *good) > { > terms->term_good = xstrdup(good); > terms->term_bad = xstrdup(bad); > } > > ie, without calling write_terms(...). I did not want to confuse you here but I forgot to mention that there should also be freeing code, i.e. initialize your terms to NULL in the beginning of cmd_builtin__helper, and always free them if it is not null. This freeing code could also be in an extra function free_terms() and you call it in set_terms() and for cleanup in the end.
Re: [PATCH v15 18/27] bisect--helper: `bisect_autostart` shell function in C
Hi, On 10/14/2016 04:14 PM, Pranit Bauva wrote: > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 502bf18..1767916 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -422,6 +425,7 @@ static int bisect_next(...) > { > int res, no_checkout; > > + bisect_autostart(terms); You are not checking for return values here. (The shell code simply exited if there is no tty, but you don't.) > @@ -754,6 +758,32 @@ static int bisect_start(struct bisect_terms *terms, int > no_checkout, > return retval || bisect_auto_next(terms, NULL); > } > > +static int bisect_autostart(struct bisect_terms *terms) > +{ > + if (is_empty_or_missing_file(git_path_bisect_start())) { > + const char *yesno; > + const char *argv[] = {NULL}; > + fprintf(stderr, _("You need to start by \"git bisect " > + "start\"\n")); > + > + if (!isatty(0)) isatty(STDIN_FILENO)? > + return 1; > + > + /* > + * TRANSLATORS: Make sure to include [Y] and [n] in your > + * translation. THe program will only accept English input Typo "THe" > + * at this point. > + */ Taking "at this point" into consideration, I think the Y and n can be easily translated now that it is in C. I guess, by using... > + yesno = git_prompt(_("Do you want me to do it for you " > + "[Y/n]? "), PROMPT_ECHO); > + if (starts_with(yesno, "n") || starts_with(yesno, "N")) ... starts_with(yesno, _("n")) || starts_with(yesno, _("N")) here (but not sure). However, this would be an extra patch on top of this series. > + exit(0); Shouldn't this also be "return 1;"? Saying "no" is the same outcome as not having a tty to ask for yes or no. > int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > { > enum { > @@ -790,6 +821,8 @@ int cmd_bisect__helper(int argc, const char **argv, const > char *prefix) >N_("find the next bisection commit"), BISECT_NEXT), > OPT_CMDMODE(0, "bisect-auto-next", &cmdmode, >N_("verify the next bisection state then find the next > bisection state"), BISECT_AUTO_NEXT), > + OPT_CMDMODE(0, "bisect-autostart", &cmdmode, > + N_("start the bisection if BISECT_START empty or > missing"), BISECT_AUTOSTART), The word "is" is missing. ~Stephan
Re: [PATCH v15 13/27] bisect--helper: `bisect_start` shell function partially in C
Hi, On 10/14/2016 04:14 PM, Pranit Bauva wrote: > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 6a5878c..1d3e17f 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -403,6 +408,205 @@ static int bisect_terms(struct bisect_terms *terms, > const char **argv, int argc) > return 0; > } > > +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, retval = 0; > + struct string_list revs = STRING_LIST_INIT_DUP; > + struct string_list states = STRING_LIST_INIT_DUP; > + struct strbuf start_head = STRBUF_INIT; > + struct strbuf bisect_names = STRBUF_INIT; > + struct strbuf orig_args = STRBUF_INIT; > + const char *head; > + unsigned char sha1[20]; > + FILE *fp = NULL; > + struct object_id oid; > + > + if (is_bare_repository()) > + no_checkout = 1; > + > + for (i = 0; i < argc; i++) { > + if (!strcmp(argv[i], "--")) { > + has_double_dash = 1; > + break; > + } > + } > + > + for (i = 0; i < argc; i++) { > + const char *commit_id = xstrfmt("%s^{commit}", argv[i]); > + const char *arg = argv[i]; > + if (!strcmp(argv[i], "--")) { > + has_double_dash = 1; > + break; > + } else if (!strcmp(arg, "--no-checkout")) { > + no_checkout = 1; > + } else if (!strcmp(arg, "--term-good") || > + !strcmp(arg, "--term-old")) { > + must_write_terms = 1; > + terms->term_good = xstrdup(argv[++i]); All these xstrdup() for the terms here and below will leak memory. I recommend to use xstrdup() also at (*) below, and use free(terms->term_good) above this line (and for every occurrence below, of course). > + } else if (skip_prefix(arg, "--term-good=", &arg)) { > + must_write_terms = 1; > + terms->term_good = xstrdup(arg); [...] > @@ -497,6 +705,11 @@ int cmd_bisect__helper(int argc, const char **argv, > const char *prefix) > die(_("--bisect-terms requires 0 or 1 argument")); > res = bisect_terms(&terms, argv, argc); > break; > + case BISECT_START: > + terms.term_good = "good"; > + terms.term_bad = "bad"; Here is (*): use xstrdup("good") etc. And then, as already mentioned for another patch, free(terms.*) below. I personally am a friend of small functions and would prefer something like as follows... (This is a comment about several patches of your series, not only this one.) First, replace the current set_terms() by static void set_terms(struct bisect_terms *terms, const char *bad, const char *good) { terms->term_good = xstrdup(good); terms->term_bad = xstrdup(bad); } ie, without calling write_terms(...). And then replace the *current* set_terms() calls by set_terms(...); write_terms(...); calls. Second, add static void get_default_terms(struct bisect_terms *terms) { set_terms(terms, "bad", "good"); } and use this instead of the two lines quoted above (and all its other occurrences). Third, use the new set_terms() everywhere instead of settings terms members directly (with the exception of get_terms()). This sounds like a safer variant (with respect to leaks and handling them) to me than doing it the current way. ~Stephan
Re: [PATCH v15 15/27] bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C
Hi Pranit, this one is hard to review because you do two or three commits in one here. I think the first commit should be the exit()->return conversion, the second commit is next and autonext, and the third commit is the pretty trivial bisect_start commit ;) However, you did it this way and it's always a hassle to split commit, so I don't really care... However, I was reviewing this superficially, to be honest. This mail skips the next and autonext part. On 10/14/2016 04:14 PM, Pranit Bauva wrote: > diff --git a/bisect.c b/bisect.c > index 45d598d..7c97e85 100644 > --- a/bisect.c > +++ b/bisect.c > @@ -843,16 +878,21 @@ static int check_ancestors(const char *prefix) > * > * If that's not the case, we need to check the merge bases. > * If a merge base must be tested by the user, its source code will be > - * checked out to be tested by the user and we will exit. > + * checked out to be tested by the user and we will return. > */ > -static void check_good_are_ancestors_of_bad(const char *prefix, int > no_checkout) > +static int check_good_are_ancestors_of_bad(const char *prefix, int > no_checkout) > { > char *filename = git_pathdup("BISECT_ANCESTORS_OK"); > struct stat st; > - int fd; > + int fd, res = 0; > > + /* > + * We don't want to clean the bisection state > + * as we need to get back to where we started > + * by using `git bisect reset`. > + */ > if (!current_bad_oid) > - die(_("a %s revision is needed"), term_bad); > + error(_("a %s revision is needed"), term_bad); Only error() or return error()? > @@ -873,8 +916,11 @@ static void check_good_are_ancestors_of_bad(const char > *prefix, int no_checkout) > filename); > else > close(fd); > + > + goto done; > done: I never understand why you do this. In case of adding a "fail" label (and fail code like "res = -1;") between "goto done" and "done:", it's fine... but without one this is just a nop. > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 1d3e17f..fcd7574 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -427,15 +560,24 @@ static int bisect_start(struct bisect_terms *terms, int > no_checkout, > no_checkout = 1; > > for (i = 0; i < argc; i++) { > - if (!strcmp(argv[i], "--")) { > + const char *arg; > + if (starts_with(argv[i], "'")) > + arg = sq_dequote(xstrdup(argv[i])); > + else > + arg = argv[i]; One is xstrdup'ed, one is not, so there'll be a leak somewhere, and it's an inconsistent leak... I guess it's a bad idea to do it this way ;) (Also below.) > @@ -443,24 +585,31 @@ static int bisect_start(struct bisect_terms *terms, int > no_checkout, > no_checkout = 1; > } else if (!strcmp(arg, "--term-good") || >!strcmp(arg, "--term-old")) { > + if (starts_with(argv[++i], "'")) > + terms->term_good = sq_dequote(xstrdup(argv[i])); > + else > + terms->term_good = xstrdup(argv[i]); > must_write_terms = 1; > - terms->term_good = xstrdup(argv[++i]); > } else if (skip_prefix(arg, "--term-good=", &arg)) { > must_write_terms = 1; > - terms->term_good = xstrdup(arg); > + terms->term_good = arg; No ;) (See my other comments (to other patches) for the "terms" leaks.) [This repeats several times below.] > diff --git a/git-bisect.sh b/git-bisect.sh > index f0896b3..d574c44 100755 > --- a/git-bisect.sh > +++ b/git-bisect.sh > @@ -109,6 +88,7 @@ bisect_skip() { > bisect_state() { > bisect_autostart > state=$1 > + get_terms > git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || > exit > get_terms > case "$#,$state" in I can't say if this change is right or wrong. It looks right, but: How does this relate to the other changes? Is this the right patch for it? ~Stephan
Re: [PATCH v15 22/27] bisect--helper: `bisect_log` shell function in C
Hi, On 10/14/2016 04:14 PM, Pranit Bauva wrote: > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 493034c..c18ca07 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -858,6 +858,23 @@ static int bisect_state(struct bisect_terms *terms, > const char **argv, > return -1; > } > > +static int bisect_log(void) > +{ > + int fd, status; > + fd = open(git_path_bisect_log(), O_RDONLY); > + if (fd < 0) > + return -1; > + > + status = copy_fd(fd, 1); Perhaps status = copy_fd(fd, STDOUT_FILENO); > + if (status) { > + close(fd); > + return -1; > + } > + > + close(fd); > + return status; > +} That's weird. Either get rid of the if() and actually use status: status = copy_fd(fd, STDOUT_FILENO); close(fd); return status ? -1 : 0; or get rid of status and use the if: if (copy_fd(fd, STDOUT_FILENO)) { close(fd); return -1; } close(fd); return 0; I'd recommend the shorter variant ;) ~Stephan
Re: [PATCH v15 12/27] bisect--helper: `get_terms` & `bisect_terms` shell function in C
Hi, On 10/14/2016 04:14 PM, Pranit Bauva wrote: > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 317d671..6a5878c 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c [...] > +static int bisect_terms(struct bisect_terms *terms, const char **argv, int > argc) > +{ > + int i; > + const char bisect_term_usage[] = > +"git bisect--helper --bisect-terms [--term-good | --term-bad | ]" > +"--term-old | --term-new"; Three things: (1) Is that indentation intentional? (2) You have a "]" at the end of the first part of the string instead of the end of the second part. (3) After the correction, bisect_term_usage and git_bisect_helper_usage[7] are the same strings. I don't recommend to use git_bisect_helper_usage[7] instead because keeping the index up-to-date is a maintenance hell. (At the end of your patch series it is a 3 instead of a 7.) However, if - for whatever reason - the usage of bisect--helper --bisect-terms changes, you always have to sync the two strings which is also nasty > + > + if (get_terms(terms)) > + return error(_("no terms defined")); > + > + if (argc > 1) { > + usage(bisect_term_usage); > + return -1; > + } ...and since you only use it once, why not simply do something like return error(_("--bisect-term requires exactly one argument")); and drop the definition of bisect_term_usage. > + > + if (argc == 0) { > + printf(_("Your current terms are %s for the old state\nand " > +"%s for the new state.\n"), terms->term_good, > +terms->term_bad); Very minor: It improves the readability if you'd split the string after the \n and put the "and "in the next line. > + return 0; > + } > + > + for (i = 0; i < argc; i++) { > + if (!strcmp(argv[i], "--term-good")) > + printf("%s\n", terms->term_good); > + else if (!strcmp(argv[i], "--term-bad")) > + printf("%s\n", terms->term_bad); > + else > + die(_("invalid argument %s for 'git bisect " > + "terms'.\nSupported options are: " > + "--term-good|--term-old and " > + "--term-bad|--term-new."), argv[i]); Hm, "return error(...)" and "die(...)" seems to be quasi-equivalent in this case. Because I am always looking from a library perspective, I'd prefer "return error(...)". > @@ -429,6 +492,11 @@ int cmd_bisect__helper(int argc, const char **argv, > const char *prefix) > terms.term_bad = xstrdup(argv[1]); > res = bisect_next_check(&terms, argc == 3 ? argv[2] : NULL); > break; > + case BISECT_TERMS: > + if (argc > 1) > + die(_("--bisect-terms requires 0 or 1 argument")); > + res = bisect_terms(&terms, argv, argc); > + break; Also here: "terms" is leaking... ~Stephan
Re: [PATCH v15 11/27] bisect--helper: `bisect_next_check` & bisect_voc shell function in C
Hi Pranit, On 10/14/2016 04:14 PM, Pranit Bauva wrote: > Also reimplement `bisect_voc` shell function in C and call it from > `bisect_next_check` implementation in C. Please don't! ;D > +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; > +} Why not simply use something like this: static const char *voc[] = { "bad|new", "good|old", }; Then... > +static int bisect_next_check(const struct bisect_terms *terms, > + const char *current_term) > +{ > + int missing_good = 1, missing_bad = 1, retval = 0; > + char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad); > + char *good_glob = xstrfmt("%s-*", terms->term_good); > + char *bad_syn, *good_syn; ...you don't need bad_syn and good_syn... > + bad_syn = xstrdup(bisect_voc("bad")); > + good_syn = xstrdup(bisect_voc("good")); ...and hence not these two lines... > + 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); ...and write voc[0], voc[1], voc[0], voc[1]); instead... > + retval = -1; > + goto finish; > + } > + 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); ...and here voc[1], voc[0], voc[0], voc[1]); ... > + retval = -1; > + goto finish; > + } > + goto finish; > +finish: > + if (!bad_ref) > + free(bad_ref); > + if (!good_glob) > + free(good_glob); > + if (!bad_syn) > + free(bad_syn); > + if (!good_syn) > + free(good_syn); ...and you can remove the 4 lines above. > + return retval; > +} Besides that, there are again some things that I've already mentioned and that can be applied here, too, for example, not capitalizing TERM_GOOD and TERM_BAD, the goto fail simplification, the terms memory leak. Cheers Stephan
Re: [PATCH v15 10/27] bisect--helper: `check_and_set_terms` shell function in C
Hi Pranit, On 10/14/2016 04:14 PM, Pranit Bauva wrote: > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 3f19b68..c6c11e3 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -20,6 +20,7 @@ static const char * const git_bisect_helper_usage[] = { > N_("git bisect--helper --bisect-clean-state"), > N_("git bisect--helper --bisect-reset []"), > N_("git bisect--helper --bisect-write > []"), > + N_("git bisect--helper --bisect-check-and-set-terms > "), Here's the same as in the previous patch... I'd not use TERM_GOOD/TERM_BAD in capitals. > NULL > }; > > @@ -212,6 +213,38 @@ static int bisect_write(const char *state, const char > *rev, > return retval; > } > > +static int set_terms(struct bisect_terms *terms, const char *bad, > + const char *good) > +{ > + terms->term_good = xstrdup(good); > + terms->term_bad = xstrdup(bad); > + return write_terms(terms->term_bad, terms->term_good); At this stage of the patch series I am wondering why you are setting "terms" here, but I guess you'll need it later. However, you are leaking memory here. Something like free(terms->term_good); free(terms->term_bad); terms->term_good = xstrdup(good); terms->term_bad = xstrdup(bad); should be safe (because you've always used xstrdup() for the terms members before). Or am I overseeing something? > @@ -278,6 +314,13 @@ int cmd_bisect__helper(int argc, const char **argv, > const char *prefix) > terms.term_bad = xstrdup(argv[3]); > res = bisect_write(argv[0], argv[1], &terms, nolog); > break; > + case CHECK_AND_SET_TERMS: > + if (argc != 3) > + die(_("--check-and-set-terms requires 3 arguments")); > + terms.term_good = xstrdup(argv[1]); > + terms.term_bad = xstrdup(argv[2]); > + res = check_and_set_terms(&terms, argv[0]); > + break; Ha! When I reviewed the last patch, I asked you why you changed the code from returning directly from each subcommand to setting res; break; and then return res at the bottom of the function. Now I see why this was useful. The two members of "terms" are again leaking memory: you are allocating memory by using xstrdup() but you are not freeing it. (That also applies to the last patch.) Cheers, Stephan
Re: [PATCH v15 09/27] bisect--helper: `bisect_write` shell function in C
Hi, I've only got some minors to mention here ;) On 10/14/2016 04:14 PM, Pranit Bauva wrote: > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index c542e8b..3f19b68 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -19,9 +19,15 @@ static const char * const git_bisect_helper_usage[] = { > N_("git bisect--helper --write-terms "), > N_("git bisect--helper --bisect-clean-state"), > N_("git bisect--helper --bisect-reset []"), > + N_("git bisect--helper --bisect-write > []"), > NULL > }; I wouldn't write "" in capital letters. I'd use something like " " as you have used for --write-terms. Note that "git bisect --help" uses " " in that context. > @@ -149,6 +155,63 @@ static int check_expected_revs(const char **revs, int > rev_nr) > return 0; > } > > +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 = NULL; > + int retval = 0; > + > + if (!strcmp(state, terms->term_bad)) > + strbuf_addf(&tag, "refs/bisect/%s", state); > + else if (one_of(state, terms->term_good, "skip", NULL)) > + strbuf_addf(&tag, "refs/bisect/%s-%s", state, rev); > + else { > + error(_("Bad bisect_write argument: %s"), state); > + retval = -1; > + goto finish; > + } > + > + if (get_oid(rev, &oid)) { > + error(_("couldn't get the oid of the rev '%s'"), rev); > + retval = -1; > + goto finish; > + } > + > + if (update_ref(NULL, tag.buf, oid.hash, NULL, 0, > +UPDATE_REFS_MSG_ON_ERR)) { > + retval = -1; > + goto finish; > + } I'd like to mention that the "goto fail;" trick could apply in this function, too. > @@ -156,9 +219,10 @@ int cmd_bisect__helper(int argc, const char **argv, > const char *prefix) > WRITE_TERMS, > BISECT_CLEAN_STATE, > BISECT_RESET, > - CHECK_EXPECTED_REVS > + CHECK_EXPECTED_REVS, > + BISECT_WRITE > } cmdmode = 0; > - int no_checkout = 0; > + int no_checkout = 0, res = 0; Why do you do this "direct return" -> "set res and return res" transition? You don't need it in this patch, you do not need it in the subsequent patches (you always set "res" exactly once after the initialization), and you don't need cleanup code in this function. > struct option options[] = { > OPT_CMDMODE(0, "next-all", &cmdmode, >N_("perform 'git bisect next'"), NEXT_ALL), > @@ -170,10 +234,13 @@ int cmd_bisect__helper(int argc, const char **argv, > const char *prefix) >N_("reset the bisection state"), BISECT_RESET), > OPT_CMDMODE(0, "check-expected-revs", &cmdmode, >N_("check for expected revs"), CHECK_EXPECTED_REVS), > + OPT_CMDMODE(0, "bisect-write", &cmdmode, > + N_("write out the bisection state in BISECT_LOG"), > BISECT_WRITE), That info text is confusing, especially considering that there is a "nolog" option. I think the action of bisect-write is two-fold: (1) update the refs, (2) log. > @@ -182,24 +249,37 @@ 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; As indicated above, I think the direct "return ...;" is cleaner. ~Stephan
Re: [PATCH v15 08/27] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
Hi, On 10/14/2016 04:14 PM, Pranit Bauva wrote: > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index d84ba86..c542e8b 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -123,13 +123,40 @@ static int bisect_reset(const char *commit) > return bisect_clean_state(); > } > > +static int is_expected_rev(const char *expected_hex) > +{ > + struct strbuf actual_hex = STRBUF_INIT; > + int res = 0; > + if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) >= > 40) { > + strbuf_trim(&actual_hex); > + res = !strcmp(actual_hex.buf, expected_hex); > + } > + strbuf_release(&actual_hex); > + return res; > +} I am not sure it does what it should. I would expect the following behavior from this function: - file does not exist (or is "broken") => return 0 - actual_hex != expected_hex => return 0 - otherwise return 1 If I am not wrong, the code does the following instead: - file does not exist (or is "broken") => return 0 - actual_hex != expected_hex => return 1 - otherwise => return 0 > +static int check_expected_revs(const char **revs, int rev_nr) > +{ > + int i; > + > + for (i = 0; i < rev_nr; i++) { > + if (!is_expected_rev(revs[i])) { > + unlink_or_warn(git_path_bisect_ancestors_ok()); > + unlink_or_warn(git_path_bisect_expected_rev()); > + return 0; > + } > + } > + return 0; > +} Here I am not sure what the function *should* do. However, I see that it basically mimics the behavior of the shell function (assuming is_expected_rev() is implemented correctly). I don't understand why the return value is int and not void. To avoid a "return 0;" line when calling this function? > @@ -167,6 +196,8 @@ int cmd_bisect__helper(int argc, const char **argv, const > char *prefix) > if (argc > 1) > die(_("--bisect-reset requires either zero or one > arguments")); > return bisect_reset(argc ? argv[0] : NULL); > + case CHECK_EXPECTED_REVS: > + return check_expected_revs(argv, argc); I note that you check the correct number of arguments for some subcommands and you do not check it for some other subcommands like this one. (I don't care, I just want to mention it.) > default: > die("BUG: unknown subcommand '%d'", cmdmode); > } ~Stephan
Re: [PATCH v15 07/27] bisect--helper: `bisect_reset` shell function in C
Hi, On 10/14/2016 04:14 PM, Pranit Bauva wrote: > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 4254d61..d84ba86 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -84,12 +89,47 @@ static int write_terms(const char *bad, const char *good) > return (res < 0) ? -1 : 0; > } > > +static int bisect_reset(const char *commit) > +{ > + struct strbuf branch = STRBUF_INIT; > + > + if (!commit) { > + if (strbuf_read_file(&branch, git_path_bisect_start(), 0) < 1) { > + printf("We are not bisecting.\n"); I think this string should be marked for translation. > + return 0; > + } > + strbuf_rtrim(&branch); [...] > @@ -121,6 +163,10 @@ int cmd_bisect__helper(int argc, const char **argv, > const char *prefix) > if (argc != 0) > die(_("--bisect-clean-state requires no arguments")); > return bisect_clean_state(); > + case BISECT_RESET: > + if (argc > 1) > + die(_("--bisect-reset requires either zero or one > arguments")); This error message is imho a little unspecific (but this might not be an issue because bisect--helper commands are not really exposed to the user). Maybe "--bisect-reset requires either no argument or a commit."? > + return bisect_reset(argc ? argv[0] : NULL); > default: > die("BUG: unknown subcommand '%d'", cmdmode); > } ~Stephan
Re: [PATCH v15 13/27] bisect--helper: `bisect_start` shell function partially in C
Hi, On 10/14/2016 04:14 PM, Pranit Bauva wrote: > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 6a5878c..1d3e17f 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -24,6 +27,8 @@ static const char * const git_bisect_helper_usage[] = { > N_("git bisect--helper --bisect-check-and-set-terms > "), > N_("git bisect--helper --bisect-next-check [] >N_("git bisect--helper --bisect-terms [--term-good | --term-old | > --term-bad | --term-new]"), > + N_("git bisect--helper --bisect start [--term-{old,good}= > --term-{new,bad}=]" > + "[--no-checkout] [ > [...]] [--] [...]"), Typo: "--bisect start" with space instead of "-" > @@ -403,6 +408,205 @@ static int bisect_terms(struct bisect_terms *terms, > const char **argv, int argc) > return 0; > } > > +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, retval = 0; > + struct string_list revs = STRING_LIST_INIT_DUP; > + struct string_list states = STRING_LIST_INIT_DUP; > + struct strbuf start_head = STRBUF_INIT; > + struct strbuf bisect_names = STRBUF_INIT; > + struct strbuf orig_args = STRBUF_INIT; > + const char *head; > + unsigned char sha1[20]; > + FILE *fp = NULL; > + struct object_id oid; > + > + if (is_bare_repository()) > + no_checkout = 1; > + > + for (i = 0; i < argc; i++) { > + if (!strcmp(argv[i], "--")) { > + has_double_dash = 1; > + break; > + } > + } > + > + for (i = 0; i < argc; i++) { > + const char *commit_id = xstrfmt("%s^{commit}", argv[i]); > + const char *arg = argv[i]; > + if (!strcmp(argv[i], "--")) { > + has_double_dash = 1; This is without effect since has_double_dash is already set to 1 by the loop above. I think you can remove this line. > + break; > + } else if (!strcmp(arg, "--no-checkout")) { > + no_checkout = 1; > + } else if (!strcmp(arg, "--term-good") || > + !strcmp(arg, "--term-old")) { > + must_write_terms = 1; > + terms->term_good = xstrdup(argv[++i]); > + } else if (skip_prefix(arg, "--term-good=", &arg)) { > + must_write_terms = 1; > + terms->term_good = xstrdup(arg); > + } else if (skip_prefix(arg, "--term-old=", &arg)) { > + must_write_terms = 1; > + terms->term_good = xstrdup(arg); I think you can join the last two branches: + } else if (skip_prefix(arg, "--term-good=", &arg) || + skip_prefix(arg, "--term-old=", &arg)) { + must_write_terms = 1; + terms->term_good = xstrdup(arg); > + } else if (!strcmp(arg, "--term-bad") || > + !strcmp(arg, "--term-new")) { > + must_write_terms = 1; > + terms->term_bad = xstrdup(argv[++i]); > + } else if (skip_prefix(arg, "--term-bad=", &arg)) { > + must_write_terms = 1; > + terms->term_bad = xstrdup(arg); > + } else if (skip_prefix(arg, "--term-new=", &arg)) { > + must_write_terms = 1; > + terms->term_good = xstrdup(arg); This has to be terms->term_bad = ... Also, you can join the last two branches, again, ie, + } else if (skip_prefix(arg, "--term-bad=", &arg) || + skip_prefix(arg, "--term-new=", &arg)) { + must_write_terms = 1; + terms->term_bad = xstrdup(arg); > + } else if (starts_with(arg, "--") && > + !one_of(arg, "--term-good", "--term-bad", NULL)) { > + die(_("unrecognised option: '%s'"), arg); [...] > + /* > + * Verify HEAD > + */ > + head = resolve_ref_unsafe("HEAD", 0, sha1, &flags); > + if (!head) > + if (get_sha1("HEAD", sha1)) > + die(_("Bad HEAD - I need a HEAD")); > + > + if (!is_empty_or_missing_file(git_path_bisect_start())) { You were so eager to re-use the comments from the shell script, but you forgot the "Check if we are bisecting." comment above this line ;-) > + /* Reset to the rev from where we started */ > + strbuf_read_file(&start_head, git_path_bisect_start(), 0); > + strbuf_trim(&start_head); > + if (!no_checkout) { > + struct argv_array argv = ARGV_ARRAY_INIT; [...] > + if (must_write_terms) > + if (write_term
Re: [PATCH v15 04/27] bisect--helper: `bisect_clean_state` shell function in C
On 11/15/2016 10:40 PM, Junio C Hamano wrote: > Stephan Beyer writes: > >>> +int bisect_clean_state(void) >>> +{ >>> + int result = 0; >>> + >>> + /* There may be some refs packed during bisection */ >>> + struct string_list refs_for_removal = STRING_LIST_INIT_NODUP; >>> + for_each_ref_in("refs/bisect", mark_for_removal, (void *) >>> &refs_for_removal); >>> + string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD")); >>> + result = delete_refs(&refs_for_removal, REF_NODEREF); >>> + refs_for_removal.strdup_strings = 1; >>> + string_list_clear(&refs_for_removal, 0); >> >> Does it have advantages to populate a list (with duplicated strings), >> hand it to delete_refs(), and clear the list (and strings), instead of >> just doing a single delete_ref() (or whatever name the singular function >> has) in the callback? > > Depending on ref backends, removing multiple refs may be a lot more > efficient than calling a single ref removal for the same set of > refs, and the comment upfront I think hints that the code was > written in the way exactly with that in mind. Removing N refs from > a packed refs file will involve a loop that runs N times, each > iteration loading the file, locating an entry among possibly 100s of > refs to remove, and then rewriting the file. Great, that's the reply I wanted to hear (and that I've considered but wasn't sure of) ;) [I did not want to dig into the sources and check if delete_refs() does something smarter than invoking delete_ref() on each item of the list.] ~Stephan
Re: [PATCH v15 01/27] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
Hi, On 10/27/2016 06:59 PM, Junio C Hamano wrote: > Does any of you (and others on the list) have time and inclination > to review this series? Me, currently. ;) Besides the things I'm mentioning in respective patch e-mails, I wonder why several bisect--helper commands are prefixed by "bisect"; I'm talking about: git bisect--helper --bisect-clean-state git bisect--helper --bisect-reset git bisect--helper --bisect-write git bisect--helper --bisect-check-and-set-terms git bisect--helper --bisect-next-check git bisect--helper --bisect-terms git bisect--helper --bisect-start etc. instead of git bisect--helper --clean-state git bisect--helper --reset git bisect--helper --write git bisect--helper --check-and-set-terms git bisect--helper --next-check git bisect--helper --terms git bisect--helper --start etc. Well, I know *why* they have these names: because the shell function names are simply reused. But I don't know why these prefixes are kept in the bisect--helper command options. On the other hand, these command names are not exposed to the user and may hence not be that important.(?) ~Stephan
Re: [PATCH v15 04/27] bisect--helper: `bisect_clean_state` shell function in C
Hi, On 10/14/2016 04:14 PM, Pranit Bauva wrote: > diff --git a/bisect.c b/bisect.c > index 6f512c2..45d598d 100644 > --- a/bisect.c > +++ b/bisect.c > @@ -1040,3 +1046,40 @@ int estimate_bisect_steps(int all) > > return (e < 3 * x) ? n : n - 1; > } > + > +static int mark_for_removal(const char *refname, const struct object_id *oid, > + int flag, void *cb_data) > +{ > + struct string_list *refs = cb_data; > + char *ref = xstrfmt("refs/bisect%s", refname); > + string_list_append(refs, ref); > + return 0; > +} > + > +int bisect_clean_state(void) > +{ > + int result = 0; > + > + /* There may be some refs packed during bisection */ > + struct string_list refs_for_removal = STRING_LIST_INIT_NODUP; > + for_each_ref_in("refs/bisect", mark_for_removal, (void *) > &refs_for_removal); > + string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD")); > + result = delete_refs(&refs_for_removal, REF_NODEREF); > + refs_for_removal.strdup_strings = 1; > + string_list_clear(&refs_for_removal, 0); Does it have advantages to populate a list (with duplicated strings), hand it to delete_refs(), and clear the list (and strings), instead of just doing a single delete_ref() (or whatever name the singular function has) in the callback? I am only seeing the disadvantage: a list with duped strings. > + unlink_or_warn(git_path_bisect_expected_rev()); [...] > + unlink_or_warn(git_path_bisect_start()); Comparing it with the original shell code (which uses "rm -f"), I was wondering a little after reading the function name unlink_or_warn() here... Looking it up helped: despite its name, unlink_or_warn() is really the function you have to use here ;D > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 65cf519..4254d61 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -5,10 +5,15 @@ > #include "refs.h" > > static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS") > +static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV") > +static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK") > +static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG") > +static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START") This is perhaps a non-issue, but you do not need any of these new path functions in *this* patch. I think nobody really cares, though, because you will need them later. Cheers Stephan
Re: [PATCH v15 02/27] bisect: rewrite `check_term_format` shell function in C
Hi, I saw in the recent "What's cooking" mail that this is still waiting for review, so I thought I could interfere and help reviewing it from a non-git-developer point of view. But only two commits for today. The first one seems fine. The second one makes me write this mail ;-) On 10/14/2016 04:14 PM, Pranit Bauva wrote: > +static int check_term_format(const char *term, const char *orig_term) > +{ [...] > + if (one_of(term, "help", "start", "skip", "next", "reset", > + "visualize", "replay", "log", "run", NULL)) [... vs ...] > -check_term_format () { > - term=$1 > - git check-ref-format refs/bisect/"$term" || > - die "$(eval_gettext "'\$term' is not a valid term")" > - case "$term" in > - help|start|terms|skip|next|reset|visualize|replay|log|run) Is there a reasons why "terms" has been dropped from the list? Best Stephan
Re: [PATCH v2 03/21] t/test-lib-functions.sh: generalize test_cmp_rev
Hi, On 04/15/2016 10:00 PM, Junio C Hamano wrote: > Stephan Beyer writes: > >> test_cmp_rev() took exactly two parameters, the expected revision >> and the revision to test. This commit generalizes this function >> such that it takes any number of at least two revisions: the >> expected one and a list of actual ones. The function returns true >> if and only if at least one actual revision coincides with the >> expected revision. > > There may be cases where you want to find the expected one among > various things you actually have (which is what the above talks > about; it is like "list-what-I-actually-got | grep what-i-want"), > but an equally useful use case would be "I would get only one > outcome from test, I anticipate one of these things, all of which is > OK, but I cannot dictate which one of them should come out" (it is > like "list-what-I-can-accept | grep what-I-actually-got"). I see that these are strictly speaking (slightly) different semantics but in the end it boils down to be the same, or am I missing anything? > I am not enthused by the new test that implements the "match one > against multi" check only in one way among these possible two to > squat on a very generic name, test_cmp_rev. > > The above _may_ appear a non-issue until you realize one thing that > is there to help those who debug the tests, which is ... > >> While at it, the side effect of generating two (temporary) files >> is removed. > > That is not strictly a side effect. test_cmp allows you to see what > was expected and what you actually had when the test failed (we > always compare expect with actual and not the other way around, so > that "diff -u expect actual" would show how the actual behaviour > diverted from our expectation in a natural way). I was referring to *generating the files* as a side effect. I did not even think about the fact that "diff" in the original code does not only return an exit code but that it also generates output that can be used as "helpful diagnostic information" (referring to Eric Sunshine's mail here). I was not aware that the Git tests should -- besides testing -- already include "tools" for easier debugging in case of a failure... So dropping this information was not intentional. > Something with the semantics of these two: > > test_revs_have_expected () { > expect=$1 > shift > git rev-parse "$@" | grep -e "$expect" >/dev/null && return > echo >&2 "The expected '$1' is not found in:" > printf >&2 " '%s'\n", "$@" > return 1 > } > > test_rev_among_expected () { > actual=$1 > shift > git rev-parse "$@" | grep -e "$actual" >/dev/null && return > echo >&2 "'$1' is not among expected ones:" > printf >&2 " '%s'\n", "$@" > return 1 > } > > might be more appropriate. Ah! That's what I meant above. The code is copy&paste besides variable naming and the output "title". Such code duplication for the sake of "easier debugging" in case of a failure? Also I wonder if test authors in the future would really know *which* one is the right one to use. In the end, either one of these two will just be used arbitrarily (and I wouldn't even think there's anything bad about it, because it *is* the same logic). I think this distinction is like having two algorithms doing the same but with a different name. Something you do NOT really want. So I'd vote against a distinction of these two "cases", but I have no problem with re-adding "debug" information (like you did in your code examples). Thanks! Stephan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 05/21] t6030: generalize test to not rely on current implementation
On 04/10/2016 09:16 PM, Junio C Hamano wrote: > Torsten Bögershausen writes: > >> Portabily: >> Since yesterday/yesterweek the usage of hard-coded >> #!/bin/sh had shown to be problematic > > That is not a new revelation, though ;-) It is just that these are > problematic to those on minority platforms, and by definition they > are noticed only when a very few people on minority platforms > happened to have run tests. > > Thanks for keeping an eye on patches in flight to prevent new > instances of this issue from getting added. Although it's not getting added but only re-indented ;) [I was not sure if this is a good idea at all to include a re-indent as a while-at-it in a commit. Maybe it was a good idea so that I am now obliged to "fix" it ;)] Cheers, Stephan -- 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 v2 20/21] bisect: compute best bisection in compute_relevant_weights()
This commit gets rid of the O(#commits) extra overhead of the best_bisection() function. Signed-off-by: Stephan Beyer --- Notes: I made the best_bisection structure be allocated on the heap because it will get free()d when the code is invoked by git rev-list --bisect ... The old code crashed in this case. bisect.c | 44 +++- 1 file changed, 19 insertions(+), 25 deletions(-) diff --git a/bisect.c b/bisect.c index 9487ba9..9f51d73 100644 --- a/bisect.c +++ b/bisect.c @@ -27,6 +27,8 @@ static int total; static unsigned marker; +static struct commit_list *best_bisection; + struct node_data { int weight; unsigned marked; @@ -73,6 +75,14 @@ static inline int distance_direction(struct commit *commit) return 0; } +static inline void update_best_bisection(struct commit *commit) +{ + if (distance_direction(commit) >= 0 +&& node_data(commit)->weight > node_data(best_bisection->item)->weight) { + best_bisection->item = commit; + } +} + static int compute_weight(struct commit *elem) { int nr = 0; @@ -153,29 +163,6 @@ static void show_list(const char *debug, int counted, } #endif /* DEBUG_BISECT */ -static struct commit_list *best_bisection(struct commit_list *list) -{ - struct commit_list *p, *best; - int best_distance = -1; - - best = list; - for (p = list; p; p = p->next) { - int distance; - unsigned flags = p->item->object.flags; - - if (flags & TREESAME) - continue; - distance = get_distance(p->item); - if (distance > best_distance) { - best = p; - best_distance = distance; - } - } - - best->next = NULL; - return best; -} - struct commit_dist { struct commit *commit; int distance; @@ -402,6 +389,8 @@ static void traversal_up_to_merges(struct commit_list *queue, } } } + + update_best_bisection(top); } } @@ -457,8 +446,10 @@ static inline int find_new_queue_from_merges(struct commit_list **queue, static inline void compute_merge_weights(struct commit_list *merges) { struct commit_list *p; - for (p = merges; p; p = p->next) + for (p = merges; p; p = p->next) { compute_weight(p->item); + update_best_bisection(p->item); + } } static void bottom_up_traversal(struct commit_list *queue) @@ -490,6 +481,9 @@ static void compute_relevant_weights(struct commit_list *list, struct commit_list *p; struct commit_list *sources = build_reversed_dag(list, weights); + best_bisection = (struct commit_list *)xcalloc(1, sizeof(*best_bisection)); + best_bisection->item = sources->item; + for (p = sources; p; p = p->next) node_data(p->item)->weight = 1; bottom_up_traversal(sources); @@ -543,7 +537,7 @@ struct commit_list *find_bisection(struct commit_list *list, } else { int i; compute_relevant_weights(list, weights); - best = best_bisection(list); + best = best_bisection; for (i = 0; i < on_list; i++) /* cleanup */ free_commit_list(weights[i].children); } -- 2.8.1.137.g522756c -- 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 v2 21/21] bisect: get back halfway shortcut
The documentation says that when the maximum possible distance is found, the algorithm stops immediately. That feature is reestablished by this commit. Signed-off-by: Stephan Beyer --- Notes: I plugged a memory leak here. bisect.c | 18 +- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/bisect.c b/bisect.c index 9f51d73..e583852 100644 --- a/bisect.c +++ b/bisect.c @@ -364,8 +364,8 @@ static inline void commit_list_insert_unique(struct commit *item, /* do a traversal on the reversed DAG (starting from commits in queue), * but stop at merge commits */ -static void traversal_up_to_merges(struct commit_list *queue, - struct commit_list **merges) +static int traversal_up_to_merges(struct commit_list *queue, + struct commit_list **merges) { assert(queue); while (queue) { @@ -391,7 +391,13 @@ static void traversal_up_to_merges(struct commit_list *queue, } update_best_bisection(top); + if (distance_direction(top) == 0) { // halfway + assert(!(top->object.flags & TREESAME)); + free_commit_list(queue); + return 1; + } } + return 0; } static inline int all_parents_are_visited(struct commit *merge) @@ -455,10 +461,12 @@ static inline void compute_merge_weights(struct commit_list *merges) static void bottom_up_traversal(struct commit_list *queue) { struct commit_list *merges = NULL; - traversal_up_to_merges(queue, &merges); - while (find_new_queue_from_merges(&queue, &merges)) { + int halfway_found = traversal_up_to_merges(queue, &merges); + + while (!halfway_found + && find_new_queue_from_merges(&queue, &merges)) { compute_merge_weights(queue); - traversal_up_to_merges(queue, &merges); + halfway_found &= traversal_up_to_merges(queue, &merges); } /* cleanup */ -- 2.8.1.137.g522756c -- 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 v2 12/21] bisect: replace clear_distance() by unique markers
clear_distance() was a O(#commits)-time function to clear the COUNTED flag from commits counted in count_distance(). The functions count_distance() and clear_distance() were called for each merge commit. This commit gets rid of the clear_distance() function by making count_distance() use unique marker ids that do not need to be cleared afterwards. This speeds up the bisecting process on large repositories with a huge amount of merges. Signed-off-by: Stephan Beyer --- bisect.c | 29 +++-- 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/bisect.c b/bisect.c index bc1bfbd..4209c75 100644 --- a/bisect.c +++ b/bisect.c @@ -23,13 +23,13 @@ static const char *argv_show_branch[] = {"show-branch", NULL, NULL}; static const char *term_bad; static const char *term_good; +static unsigned marker; + struct node_data { int weight; + unsigned marked; }; -/* Remember to update object flag allocation in object.h */ -#define COUNTED(1u<<16) - #define DEBUG_BISECT 0 static inline struct node_data *node_data(struct commit *elem) @@ -43,15 +43,17 @@ static int count_distance(struct commit_list *entry) int nr = 0; struct commit_list *todo = NULL; commit_list_append(entry->item, &todo); + marker++; while (todo) { struct commit *commit = pop_commit(&todo); - if (!(commit->object.flags & (UNINTERESTING | COUNTED))) { + if (!(commit->object.flags & UNINTERESTING) +&& node_data(commit)->marked != marker) { struct commit_list *p; if (!(commit->object.flags & TREESAME)) nr++; - commit->object.flags |= COUNTED; + node_data(commit)->marked = marker; for (p = commit->parents; p; p = p->next) { commit_list_insert(p->item, &todo); @@ -62,15 +64,6 @@ static int count_distance(struct commit_list *entry) return nr; } -static void clear_distance(struct commit_list *list) -{ - while (list) { - struct commit *commit = list->item; - commit->object.flags &= ~COUNTED; - list = list->next; - } -} - static int count_interesting_parents(struct commit *commit) { struct commit_list *p; @@ -123,10 +116,9 @@ static void show_list(const char *debug, int counted, int nr, const char *subject_start; int subject_len; - fprintf(stderr, "%c%c%c ", + fprintf(stderr, "%c%c ", (flags & TREESAME) ? ' ' : 'T', - (flags & UNINTERESTING) ? 'U' : ' ', - (flags & COUNTED) ? 'C' : ' '); + (flags & UNINTERESTING) ? 'U' : ' '); if (commit->util) fprintf(stderr, "%3d", node_data(commit)->weight); else @@ -289,7 +281,6 @@ static struct commit_list *do_find_bisection(struct commit_list *list, if (!(p->item->object.flags & UNINTERESTING) && (node_data(p->item)->weight == -2)) { node_data(p->item)->weight = count_distance(p); - clear_distance(list); /* Does it happen to be at exactly half-way? */ if (!find_all && halfway(p, nr)) @@ -351,6 +342,8 @@ struct commit_list *find_bisection(struct commit_list *list, struct commit_list *p, *best, *next, *last; struct node_data *weights; + marker = 0; + show_list("bisection 2 entry", 0, 0, list); /* -- 2.8.1.137.g522756c -- 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 v2 15/21] bisect: introduce distance_direction()
We introduce the concept of rising and falling distances (in addition to a halfway distance). This will be useful in subsequent commits. Signed-off-by: Stephan Beyer --- bisect.c | 33 +++-- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/bisect.c b/bisect.c index cfd406c..f737ce7 100644 --- a/bisect.c +++ b/bisect.c @@ -46,6 +46,28 @@ static inline int get_distance(struct commit *commit, int total) return distance; } +/* + * Return -1 if the distance is falling. + * (A falling distance means that the distance of the + * given commit is larger than the distance of its + * child commits.) + * Return 0 if the distance is halfway. + * Return 1 if the distance is rising. + */ +static inline int distance_direction(struct commit *commit, int total) +{ + int doubled_diff = 2 * node_data(commit)->weight - total; + if (doubled_diff < -1) + return 1; + if (doubled_diff > 1) + return -1; + /* +* 2 and 3 are halfway of 5. +* 3 is halfway of 6 but 2 and 4 are not. +*/ + return 0; +} + static int count_distance(struct commit *elem) { int nr = 0; @@ -92,16 +114,7 @@ static inline int halfway(struct commit *commit, int nr) */ if (commit->object.flags & TREESAME) return 0; - /* -* 2 and 3 are halfway of 5. -* 3 is halfway of 6 but 2 and 4 are not. -*/ - switch (2 * node_data(commit)->weight - nr) { - case -1: case 0: case 1: - return 1; - default: - return 0; - } + return !distance_direction(commit, nr); } #if !DEBUG_BISECT -- 2.8.1.137.g522756c -- 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 v2 19/21] bisect: use a bottom-up traversal to find relevant weights
The idea is to reverse the DAG and perform a traversal starting on all sources of the reversed DAG. We walk from the bottom commits, incrementing the weight while walking on a part of the graph that is single strand of pearls, or doing the "count the reachable ones the hard way" using compute_weight() when we hit a merge commit. A traversal ends when the computed weight is falling or halfway. This way, commits with too high weight to be relevant are never visited (and their weights are never computed). Signed-off-by: Stephan Beyer --- Notes: I rephrased the commit message. I renamed the functions such that they don't talk about "BFS" because that is irrelevant. Also use a DFS now because it is less code (and a little more efficient). I plugged some leaks. bisect.c | 250 +-- 1 file changed, 162 insertions(+), 88 deletions(-) diff --git a/bisect.c b/bisect.c index c6bad43..9487ba9 100644 --- a/bisect.c +++ b/bisect.c @@ -30,6 +30,9 @@ static unsigned marker; struct node_data { int weight; unsigned marked; + unsigned parents; + unsigned visited : 1; + struct commit_list *children; }; #define DEBUG_BISECT 0 @@ -110,16 +113,6 @@ static int count_interesting_parents(struct commit *commit) return count; } -static inline int halfway(struct commit *commit) -{ - /* -* Don't short-cut something we are not going to return! -*/ - if (commit->object.flags & TREESAME) - return 0; - return !distance_direction(commit); -} - #if !DEBUG_BISECT #define show_list(a,b,c) do { ; } while (0) #else @@ -340,90 +333,168 @@ static void compute_all_weights(struct commit_list *list, show_list("bisection 2 counted all", counted, list); } -/* At the moment this is basically the same as compute_all_weights() - * but with a halfway shortcut */ +static struct commit_list *build_reversed_dag(struct commit_list *list, + struct node_data *nodes) +{ + struct commit_list *sources = NULL; + struct commit_list *p; + int n = 0; + for (p = list; p; p = p->next) + p->item->util = &nodes[n++]; + for (p = list; p; p = p->next) { + struct commit_list *parent; + struct commit *commit = p->item; + for (parent = commit->parents; parent; parent = parent->next) { + if (!(parent->item->object.flags & UNINTERESTING)) { + struct commit_list **next = &node_data(parent->item)->children; + commit_list_insert(commit, next); + node_data(commit)->parents++; + } + } + } + + /* find all sources */ + for (p = list; p; p = p->next) { + if (node_data(p->item)->parents == 0) + commit_list_insert(p->item, &sources); + } + + return sources; +} + +static inline void commit_list_insert_unique(struct commit *item, + struct commit_list **list) +{ + if (!*list || item < (*list)->item) /* empty list or item will be first */ + commit_list_insert(item, list); + else if (item != (*list)->item) { /* item will not be first or not inserted */ + struct commit_list *p = *list; + for (; p->next && p->next->item < item; p = p->next); + if (!p->next || item != p->next->item) /* not already inserted */ + commit_list_insert(item, &p->next); + } +} + +/* do a traversal on the reversed DAG (starting from commits in queue), + * but stop at merge commits */ +static void traversal_up_to_merges(struct commit_list *queue, + struct commit_list **merges) +{ + assert(queue); + while (queue) { + struct commit *top = queue->item; + struct commit_list *p; + + pop_commit(&queue); + + if (distance_direction(top) > 0) { + node_data(top)->visited = 1; + + /* queue children */ + for (p = node_data(top)->children; p; p = p->next) { + if (node_data(p->item)->parents > 1) /* child is a merge */ + commit_list_insert_unique(p->item, merges); + else { + node_data(p->item)->weight = node_data(top)->weight; + if (!(p->item->object.flags & TREESAME)) +
[PATCH v2 04/21] t: use test_cmp_rev() where appropriate
test_cmp_rev() from t/test-lib-functions.sh is used to make many tests clearer. Signed-off-by: Stephan Beyer --- Notes: This change is in some way independent of the bisect topic but the next patch is based on this (for t6030). t/t2012-checkout-last.sh | 8 ++-- t/t3308-notes-merge.sh| 8 ++-- t/t3310-notes-merge-manual-resolve.sh | 8 ++-- t/t3311-notes-merge-fanout.sh | 6 +-- t/t3404-rebase-interactive.sh | 38 +++ t/t3407-rebase-abort.sh | 8 ++-- t/t3410-rebase-preserve-dropped-merges.sh | 4 +- t/t3411-rebase-preserve-around-merges.sh | 10 ++-- t/t3414-rebase-preserve-onto.sh | 12 ++--- t/t3501-revert-cherry-pick.sh | 4 +- t/t3506-cherry-pick-ff.sh | 6 +-- t/t3903-stash.sh | 6 +-- t/t4150-am.sh | 18 +++ t/t5404-tracking-branches.sh | 2 +- t/t5505-remote.sh | 4 +- t/t5520-pull.sh | 36 +++--- t/t6022-merge-rename.sh | 2 +- t/t6030-bisect-porcelain.sh | 79 --- t/t6036-recursive-corner-cases.sh | 58 +++ t/t6042-merge-rename-corner-cases.sh | 50 +-- t/t7003-filter-branch.sh | 8 ++-- t/t7004-tag.sh| 2 +- t/t7110-reset-merge.sh| 24 +- t/t7201-co.sh | 12 ++--- t/t7601-merge-pull-config.sh | 17 --- t/t7603-merge-reduce-heads.sh | 30 ++-- t/t7605-merge-resolve.sh | 5 +- t/t9162-git-svn-dcommit-interactive.sh| 8 ++-- t/t9300-fast-import.sh| 12 ++--- 29 files changed, 232 insertions(+), 253 deletions(-) diff --git a/t/t2012-checkout-last.sh b/t/t2012-checkout-last.sh index e7ba8c5..64cb449 100755 --- a/t/t2012-checkout-last.sh +++ b/t/t2012-checkout-last.sh @@ -43,7 +43,7 @@ test_expect_success '"checkout -" attaches again' ' test_expect_success '"checkout -" detaches again' ' git checkout - && - test "z$(git rev-parse HEAD)" = "z$(git rev-parse other)" && + test_cmp_rev HEAD other && test_must_fail git symbolic-ref HEAD ' @@ -101,19 +101,19 @@ test_expect_success 'merge base test setup' ' test_expect_success 'another...master' ' git checkout another && git checkout another...master && - test "z$(git rev-parse --verify HEAD)" = "z$(git rev-parse --verify master^)" + test_cmp_rev HEAD master^ ' test_expect_success '...master' ' git checkout another && git checkout ...master && - test "z$(git rev-parse --verify HEAD)" = "z$(git rev-parse --verify master^)" + test_cmp_rev HEAD master^ ' test_expect_success 'master...' ' git checkout another && git checkout master... && - test "z$(git rev-parse --verify HEAD)" = "z$(git rev-parse --verify master^)" + test_cmp_rev HEAD master^ ' test_expect_success '"checkout -" works after a rebase A' ' diff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh index 19aed7e..1f72e9e 100755 --- a/t/t3308-notes-merge.sh +++ b/t/t3308-notes-merge.sh @@ -93,7 +93,7 @@ test_expect_success 'merge non-notes ref into empty notes ref (remote-notes/orig git notes merge refs/remote-notes/origin/x && verify_notes v && # refs/remote-notes/origin/x and v should point to the same notes commit - test "$(git rev-parse refs/remote-notes/origin/x)" = "$(git rev-parse refs/notes/v)" + test_cmp_rev refs/remote-notes/origin/x refs/notes/v ' test_expect_success 'merge notes into empty notes ref (x => y)' ' @@ -101,13 +101,13 @@ test_expect_success 'merge notes into empty notes ref (x => y)' ' git notes merge x && verify_notes y && # x and y should point to the same notes commit - test "$(git rev-parse refs/notes/x)" = "$(git rev-parse refs/notes/y)" + test_cmp_rev refs/notes/x refs/notes/y ' test_expect_success 'merge empty notes ref (z => y)' ' git notes merge z && # y should not change (still == x) - test "$(git rev-parse refs/notes/x)" = "$(git rev-parse refs/notes/y)" + test_cmp_rev refs/notes/x refs/notes/y ' test_expect_success 'change notes on other notes ref (y)' ' @
[PATCH v2 13/21] bisect: use commit instead of commit list as arguments when appropriate
It makes no sense that the argument for count_distance() and halfway() is a commit list when only its first commit is relevant. Signed-off-by: Stephan Beyer --- bisect.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/bisect.c b/bisect.c index 4209c75..2c1102f 100644 --- a/bisect.c +++ b/bisect.c @@ -38,11 +38,11 @@ static inline struct node_data *node_data(struct commit *elem) return (struct node_data *)elem->util; } -static int count_distance(struct commit_list *entry) +static int count_distance(struct commit *elem) { int nr = 0; struct commit_list *todo = NULL; - commit_list_append(entry->item, &todo); + commit_list_append(elem, &todo); marker++; while (todo) { @@ -77,18 +77,18 @@ static int count_interesting_parents(struct commit *commit) return count; } -static inline int halfway(struct commit_list *p, int nr) +static inline int halfway(struct commit *commit, int nr) { /* * Don't short-cut something we are not going to return! */ - if (p->item->object.flags & TREESAME) + if (commit->object.flags & TREESAME) return 0; /* * 2 and 3 are halfway of 5. * 3 is halfway of 6 but 2 and 4 are not. */ - switch (2 * node_data(p->item)->weight - nr) { + switch (2 * node_data(commit)->weight - nr) { case -1: case 0: case 1: return 1; default: @@ -280,10 +280,10 @@ static struct commit_list *do_find_bisection(struct commit_list *list, for (p = list; p; p = p->next) { if (!(p->item->object.flags & UNINTERESTING) && (node_data(p->item)->weight == -2)) { - node_data(p->item)->weight = count_distance(p); + node_data(p->item)->weight = count_distance(p->item); /* Does it happen to be at exactly half-way? */ - if (!find_all && halfway(p, nr)) + if (!find_all && halfway(p->item, nr)) return p; counted++; } @@ -321,7 +321,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list, } /* Does it happen to be at exactly half-way? */ - if (!find_all && halfway(p, nr)) + if (!find_all && halfway(p->item, nr)) return p; } } -- 2.8.1.137.g522756c -- 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 v2 14/21] bisect: extract get_distance() function from code duplication
Signed-off-by: Stephan Beyer --- bisect.c | 16 ++-- 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/bisect.c b/bisect.c index 2c1102f..cfd406c 100644 --- a/bisect.c +++ b/bisect.c @@ -38,6 +38,14 @@ static inline struct node_data *node_data(struct commit *elem) return (struct node_data *)elem->util; } +static inline int get_distance(struct commit *commit, int total) +{ + int distance = node_data(commit)->weight; + if (total - distance < distance) + distance = total - distance; + return distance; +} + static int count_distance(struct commit *elem) { int nr = 0; @@ -148,9 +156,7 @@ static struct commit_list *best_bisection(struct commit_list *list, int nr) if (flags & TREESAME) continue; - distance = node_data(p->item)->weight; - if (nr - distance < distance) - distance = nr - distance; + distance = get_distance(p->item, nr); if (distance > best_distance) { best = p; best_distance = distance; @@ -188,9 +194,7 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n if (flags & TREESAME) continue; - distance = node_data(p->item)->weight; - if (nr - distance < distance) - distance = nr - distance; + distance = get_distance(p->item, nr); array[cnt].commit = p->item; array[cnt].distance = distance; cnt++; -- 2.8.1.137.g522756c -- 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 v2 07/21] bisect: plug the biggest memory leak
Signed-off-by: Stephan Beyer --- bisect.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bisect.c b/bisect.c index 7996c29..901e4d3 100644 --- a/bisect.c +++ b/bisect.c @@ -984,6 +984,8 @@ int bisect_next_all(const char *prefix, int no_checkout) exit(10); } + free_commit_list(revs.commits); + nr = all - reaches - 1; steps = estimate_bisect_steps(all); printf("Bisecting: %d revision%s left to test after this " -- 2.8.1.137.g522756c -- 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 v2 09/21] bisect: make algorithm behavior independent of DEBUG_BISECT
If DEBUG_BISECT is set to 1, bisect does not only show debug information but also changes the algorithm behavior: halfway() is always false. This commit makes the algorithm independent of DEBUG_BISECT. Signed-off-by: Stephan Beyer --- bisect.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/bisect.c b/bisect.c index 2f54d96..1a13f35 100644 --- a/bisect.c +++ b/bisect.c @@ -101,8 +101,6 @@ static inline int halfway(struct commit_list *p, int nr) */ if (p->item->object.flags & TREESAME) return 0; - if (DEBUG_BISECT) - return 0; /* * 2 and 3 are halfway of 5. * 3 is halfway of 6 but 2 and 4 are not. -- 2.8.1.137.g522756c -- 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 v2 08/21] bisect: make bisect compile if DEBUG_BISECT is set
Setting the macro DEBUG_BISECT to 1 enables debugging information for the bisect algorithm. The code did not compile due to struct changes. Signed-off-by: Stephan Beyer --- bisect.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bisect.c b/bisect.c index 901e4d3..2f54d96 100644 --- a/bisect.c +++ b/bisect.c @@ -131,7 +131,7 @@ static void show_list(const char *debug, int counted, int nr, unsigned flags = commit->object.flags; enum object_type type; unsigned long size; - char *buf = read_sha1_file(commit->object.sha1, &type, &size); + char *buf = read_sha1_file(commit->object.oid.hash, &type, &size); const char *subject_start; int subject_len; @@ -143,10 +143,10 @@ static void show_list(const char *debug, int counted, int nr, fprintf(stderr, "%3d", weight(p)); else fprintf(stderr, "---"); - fprintf(stderr, " %.*s", 8, sha1_to_hex(commit->object.sha1)); + fprintf(stderr, " %.*s", 8, sha1_to_hex(commit->object.oid.hash)); for (pp = commit->parents; pp; pp = pp->next) fprintf(stderr, " %.*s", 8, - sha1_to_hex(pp->item->object.sha1)); + sha1_to_hex(pp->item->object.oid.hash)); subject_len = find_commit_subject(buf, &subject_start); if (subject_len) -- 2.8.1.137.g522756c -- 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