Re: [RFC PATCH 0/1] Implement CMake build

[Stephan Beyer]

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

[Stephan Beyer]

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?

[Stephan Beyer]

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

[Stephan Beyer]

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 <s-be...@gmx.net>
---
 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?"

[Stephan Beyer]

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

[Stephan Beyer]

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 *) _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

[Stephan Beyer]

> @@ -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

[Stephan Beyer]

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 *) _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

[Stephan Beyer]

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

[Stephan Beyer]

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

[Stephan Beyer]

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

[Stephan Beyer]

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

[Stephan Beyer]

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(, 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

[Stephan Beyer]

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

[Stephan Beyer]

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

[Stephan Beyer]

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 <s-be...@gmx.net>
---

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

[Stephan Beyer]

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 <s-be...@gmx.net>
---

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

[Stephan Beyer]

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

[Stephan Beyer]

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

[Stephan Beyer]

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

[Stephan Beyer]

On 08/23/2017 07:29 PM, Stefan Beller wrote:
> On Wed, Aug 23, 2017 at 8:19 AM, Stephan Beyer <s-be...@gmx.net> 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

[Stephan Beyer]

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

[Stephan Beyer]

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

[Stephan Beyer]

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(>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)

[Stephan Beyer]

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

[Stephan Beyer]

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

[Stephan Beyer]

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

[Stephan Beyer]

> 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

[Stephan Beyer]

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

[Stephan Beyer]

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

[Stephan Beyer]

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

[Stephan Beyer]

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", ,
>>>N_("start the bisect session"), BISECT_START),
>>> + OPT_CMDMODE(0, "bisect-next", ,
>>> +  N_("find the next bisection commit"), BISECT_NEXT),
>>> + OPT_CMDMODE(0, "bisect-auto-next", ,
>>> +  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'

[Stephan Beyer]

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

[Stephan Beyer]

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'

[Stephan Beyer]

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

[Stephan Beyer]

Hi Pranit,

On 12/16/2016 08:35 PM, Pranit Bauva wrote:
> On Thu, Nov 17, 2016 at 5:17 AM, Stephan Beyer <s-be...@gmx.net> 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(_hex, git_path_bisect_expected_rev(), 0) 
>>> >= 40) {
>>> + strbuf_trim(_hex);
>>> + res = !strcmp(actual_hex.buf, expected_hex);
>>> + }
>>> + strbuf_release(_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

[Stephan Beyer]

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)

[Stephan Beyer]

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

[Stephan Beyer]

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

[Stephan Beyer]

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

[Stephan Beyer]

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(, git_path_abort_safety_file(), 0) >= 0) {
>>> +   strbuf_trim();
>>> +   if (get_oid_hex(sb.buf, _head)) {
>>> +   strbuf_release();
>>> +   die(_("could not parse %s"), 
>>> git_path_abort_safety_file());
>>> +   }
>>> +   strbuf_release();
>>> +   }
>>
>> Maybe the following is a bit simpler:
>>
>>if (strbuf_read_file(, git_path_abort_safety_file(), 0) >= 0) {
>>int res;
>>strbuf_trim();
>>res = get_oid_hex(sb.buf, _head);
>>strbuf_release();
>>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"

[Stephan Beyer]

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"

[Stephan Beyer]

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

[Stephan Beyer]

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 <s-be...@gmx.net>
---
 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", ))
+   write_file(git_path_abort_safety_file(), "%s", 
oid_to_hex());
+   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();
strbuf_release();
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, );
+   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(, git_path_abort_safety_file(), 0) >= 0) {
+   strbuf_trim();
+   if (get_oid_hex(sb.buf, _head)) {
+   strbuf_release();
+   die(_("could not parse %s"), 
git_path_abort_safety_file());
+   }
+   strbuf_release();
+   }
+   else if (errno == ENOENT)
+   oidclr(_head);
+   else
+   die_errno(_("could not read '%s'"), 
git_path_abort_safety_file());
+
+   if (get_oid("HEAD", _head))
+   oidclr(_head);
+
+   return !oidcmp(_head, _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();
@@ -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(_list, opts);
todo_list_release(_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

[Stephan Beyer]

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 <s-be...@gmx.net>
---
 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(, "%s", get_dir(opts));
+   strbuf_addf(, "%s", git_path_seq_dir());
remove_dir_recursively(, 0);
strbuf_release();
 
-- 
2.11.0.27.g74d6bea

[PATCH v2 3/5] Add test that cherry-pick --abort does not unsafely change HEAD

[Stephan Beyer]

The test expects failure because it is a current breakage
reported by Junio C Hamano.

Signed-off-by: Stephan Beyer <s-be...@gmx.net>
---
 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

[Stephan Beyer]

Signed-off-by: Stephan Beyer <s-be...@gmx.net>
---
 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(, state, "abort-safety", 1) > 0) {
if (get_oid_hex(sb.buf, _safety))
-   die(_("could not parse %s"), am_path(state, 
"abort_safety"));
+   die(_("could not parse %s"), am_path(state, 
"abort-safety"));
} else
oidclr(_safety);
 
-- 
2.11.0.27.g74d6bea

[PATCH v2 2/5] am: Change safe_to_abort()'s not rewinding error into a warning

[Stephan Beyer]

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 <s-be...@gmx.net>
---
 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(, _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

[Stephan Beyer]

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 <johannes.schinde...@gmx.de> 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

[Stephan Beyer]

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 <s-be...@gmx.net>
---
 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", ))
+   write_file(git_path_curr_file(), "%s", oid_to_hex());
+   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();
strbuf_release();
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, );
+   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(, git_path_curr_file(), 0) >= 0) {
+   strbuf_trim();
+   if (get_oid_hex(sb.buf, _head)) {
+   strbuf_release();
+   die(_("could not parse %s"), git_path_curr_file());
+   }
+   strbuf_release();
+   }
+   else if (errno == ENOENT)
+   oidclr(_head);
+   else
+   die_errno(_("could not read '%s'"), git_path_curr_file());
+
+   if (get_oid("HEAD", _head))
+   oidclr(_head);
+
+   return !oidcmp(_head, _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();
@@ -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(_list, opts);
todo_list_release(_list);
return res;
-- 
2.11.0.27.g4eed97c

[PATCH 3/5] Add test that cherry-pick --abort does not unsafely change HEAD

[Stephan Beyer]

Signed-off-by: Stephan Beyer <s-be...@gmx.net>
---
 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

[Stephan Beyer]

Signed-off-by: Stephan Beyer <s-be...@gmx.net>
---
 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(, state, "abort-safety", 1) > 0) {
if (get_oid_hex(sb.buf, _safety))
-   die(_("could not parse %s"), am_path(state, 
"abort_safety"));
+   die(_("could not parse %s"), am_path(state, 
"abort-safety"));
} else
oidclr(_safety);
 
-- 
2.11.0.27.g4eed97c

[PATCH 5/5] sequencer: Remove useless get_dir() function

[Stephan Beyer]

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 <s-be...@gmx.net>
---
 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(, "%s", get_dir(opts));
+   strbuf_addf(, "%s", git_path_seq_dir());
remove_dir_recursively(, 0);
strbuf_release();
 
-- 
2.11.0.27.g4eed97c

[PATCH 2/5] am: Change safe_to_abort()'s not rewinding error into a warning

[Stephan Beyer]

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 <s-be...@gmx.net>
---
 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(, _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"

[Stephan Beyer]

Hi,

On 12/07/2016 09:04 PM, Junio C Hamano wrote:
> Stephan Beyer <s-be...@gmx.net> 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"

[Stephan Beyer]

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

[Stephan Beyer]

Hi Pranit,

On 12/06/2016 11:40 PM, Pranit Bauva wrote:
> On Tue, Nov 22, 2016 at 5:42 AM, Stephan Beyer <s-be...@gmx.net> 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 **) 
>>> [i].string;
>>> + if(bisect_write(state, *hex_string, terms, 0)) {
>>> + string_list_clear(, 0);
>>> + return -1;
>>> + }
>>> + if (check_expected_revs(hex_string, 1)) {
>>> + string_list_clear(, 0);
>>> + return -1;
>>> + }
>>
>> by:
>>
>> const char *hex_str = hex.items[i].string;
>> if (set_state(terms, state, hex_string)) {
>> string_list_clear(, 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

[Stephan Beyer]

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 <s-be...@gmx.net> 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

[Stephan Beyer]

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(, fp) != EOF) {
>>> + int pos = 0;
>>> + while (pos < line.len) {
>>> + pos = get_next_word(line.buf, pos, );
>>> +
>>> + 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

[Stephan Beyer]

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

[Stephan Beyer]

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(, fp) != EOF) {
> + int pos = 0;
> + while (pos < line.len) {
> + pos = get_next_word(line.buf, pos, );
> +
> + 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, );
> + if (bisect_start(terms, 0, argv.argv, 
> argv.argc)) {
> + argv_array_clear();
> + res = -1;
> + goto finish;

goto fail;

> + }
> + argv_array_clear();
> + 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

[Stephan Beyer]

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(, 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(, 0);
> + die(_("Bad rev input: %s"), argv[i]);
> + }
> + string_list_append(, sha1_to_hex(sha1));
> + }
> + for (i = 0; i < hex.nr; i++) {

... And replace this:

> + const char **hex_string = (const char **) 
> [i].string;
> + if(bisect_write(state, *hex_string, terms, 0)) {
> + string_list_clear(, 0);
> + return -1;
> + }
> + if (check_expected_revs(hex_string, 1)) {
> + string_list_clear(, 0);
> + return -1;
> + }

by:

const char *hex_str = hex.items[i].string;
if (set_state(terms, state, hex_string)) {
string_list_clear(, 0);
return -1;
}

> + }
> + string_list_clear(, 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);
> +
> + return -1;
> +}
> +
>  int 

Re: [PATCH v15 15/27] bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C

[Stephan Beyer]

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", _name, );
> + 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(_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 *) _revs);
> +
> + argv_array_pushl(_argv, "skipped_commits", 
> "refs/bisect/bad", "--not", NULL);
> + for (i = 0; i < good_revs.nr; i++)
> + argv_array_push(_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(, NULL);
> + rev_argv.argc = setup_revisions(rev_argv.argc, rev_argv.argv, 
> , NULL);
> + argv_array_clear(_argv);
> + string_list_clear(_revs, 0);
> + if (prepare_revision_walk())
> + die(_("revision walk setup failed\n"));
> +
> + while ((commit = get_revision()) != NULL) {
> + struct strbuf commit_name = STRBUF_INIT;
> + format_commit_message(commit, "%s",
> +   _name, );
> + fprintf(fp, "# possible first %s commit: "
> + "[%s] %s\n", terms->term_bad,
> + oid_to_hex(>object.oid),
> + commit_name.buf);
> +  

Re: [PATCH v15 13/27] bisect--helper: `bisect_start` shell function partially in C

[Stephan Beyer]

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

[Stephan Beyer]

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", ,
>N_("verify the next bisection state then find the next 
> bisection state"), BISECT_AUTO_NEXT),
> + OPT_CMDMODE(0, "bisect-autostart", ,
> +  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

[Stephan Beyer]

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=", )) {
> + 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(, 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

[Stephan Beyer]

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=", )) {
>   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

[Stephan Beyer]

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

[Stephan Beyer]

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(, argc == 3 ? argv[2] : NULL);
>   break;
> + case BISECT_TERMS:
> + if (argc > 1)
> + die(_("--bisect-terms requires 0 or 1 argument"));
> + res = bisect_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

[Stephan Beyer]

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

[Stephan Beyer]

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], , 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(, 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

[Stephan Beyer]

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(, "refs/bisect/%s", state);
> + else if (one_of(state, terms->term_good, "skip", NULL))
> + strbuf_addf(, "refs/bisect/%s-%s", state, rev);
> + else {
> + error(_("Bad bisect_write argument: %s"), state);
> + retval = -1;
> + goto finish;
> + }
> +
> + if (get_oid(rev, )) {
> + 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", ,
>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", ,
>N_("check for expected revs"), CHECK_EXPECTED_REVS),
> + OPT_CMDMODE(0, "bisect-write", ,
> +  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

[Stephan Beyer]

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(_hex, git_path_bisect_expected_rev(), 0) >= 
> 40) {
> + strbuf_trim(_hex);
> + res = !strcmp(actual_hex.buf, expected_hex);
> + }
> + strbuf_release(_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

[Stephan Beyer]

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(, 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();
[...]
> @@ -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

[Stephan Beyer]

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=", )) {
> + must_write_terms = 1;
> + terms->term_good = xstrdup(arg);
> + } else if (skip_prefix(arg, "--term-old=", )) {
> + 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=", ) ||
+  skip_prefix(arg, "--term-old=", )) {
+   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=", )) {
> + must_write_terms = 1;
> + terms->term_bad = xstrdup(arg);
> + } else if (skip_prefix(arg, "--term-new=", )) {
> + 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=", ) ||
+  skip_prefix(arg, "--term-new=", )) {
+   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, );
> + 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(_head, git_path_bisect_start(), 0);
> + strbuf_trim(_head);
> + if (!no_checkout) {
> + struct argv_array argv = ARGV_ARRAY_INIT;
[...]
> + if (must_write_terms)
> + if (write_terms(terms->term_bad, 

Re: [PATCH v15 04/27] bisect--helper: `bisect_clean_state` shell function in C

[Stephan Beyer]

On 11/15/2016 10:40 PM, Junio C Hamano wrote:
> Stephan Beyer <s-be...@gmx.net> 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 *) 
>>> _for_removal);
>>> +   string_list_append(_for_removal, xstrdup("BISECT_HEAD"));
>>> +   result = delete_refs(_for_removal, REF_NODEREF);
>>> +   refs_for_removal.strdup_strings = 1;
>>> +   string_list_clear(_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

[Stephan Beyer]

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

[Stephan Beyer]

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 *) 
> _for_removal);
> + string_list_append(_for_removal, xstrdup("BISECT_HEAD"));
> + result = delete_refs(_for_removal, REF_NODEREF);
> + refs_for_removal.strdup_strings = 1;
> + string_list_clear(_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

[Stephan Beyer]

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

[Stephan Beyer]

Hi,

On 04/15/2016 10:00 PM, Junio C Hamano wrote:
> Stephan Beyer <s-be...@gmx.net> 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 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

[Stephan Beyer]

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()

[Stephan Beyer]

This commit gets rid of the O(#commits) extra overhead of
the best_bisection() function.

Signed-off-by: Stephan Beyer <s-be...@gmx.net>
---

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

[Stephan Beyer]

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 <s-be...@gmx.net>
---

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, );
-   while (find_new_queue_from_merges(, )) {
+   int halfway_found = traversal_up_to_merges(queue, );
+
+   while (!halfway_found
+   && find_new_queue_from_merges(, )) {
compute_merge_weights(queue);
-   traversal_up_to_merges(queue, );
+   halfway_found &= traversal_up_to_merges(queue, );
}
 
/* 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

[Stephan Beyer]

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 <s-be...@gmx.net>
---
 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, );
+   marker++;
 
while (todo) {
struct commit *commit = pop_commit();
 
-   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, );
@@ -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()

[Stephan Beyer]

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 <s-be...@gmx.net>
---
 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

[Stephan Beyer]

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 <s-be...@gmx.net>
---

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 = [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 = 
_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, );
+   }
+
+   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, >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();
+
+   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))
+   node_data(p->item)->w

[PATCH v2 04/21] t: use test_cmp_rev() where appropriate

[Stephan Beyer]

test_cmp_rev() from t/test-lib-functions.sh is used to make many
tests clearer.

Signed-off-by: Stephan Beyer <s-be...@gmx.net>
---

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)' '
@@ -174,7 +174,7 @@ test_expect_success 'merge changed (y) into original (x) => 
Fast-forward' '
verify_notes x &&
verify_notes y &&
# x and y 

[PATCH v2 13/21] bisect: use commit instead of commit list as arguments when appropriate

[Stephan Beyer]

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 <s-be...@gmx.net>
---
 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, );
+   commit_list_append(elem, );
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

[Stephan Beyer]

Signed-off-by: Stephan Beyer <s-be...@gmx.net>
---
 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 00/21] git bisect improvements

[Stephan Beyer]

Hi,

a long time ago[1] I sent the first version of this patchset
to the list. Since then I wrote different variants of the algorithm,
fixed some bugs, made the tests work ;), and finally performed some
performance tests to pick the best version of the different variants.

For the performance tests I used the Git repositories of the Linux
kernel and of Git itself and performed whole-history bisections
with a bisect script that decided "good" or "bad" based on the hash
of a commit. And another bisect script that did the opposite.

I omit the details. The best variant uses a DFS for the traversal
without any further "smart" tricks: These tricks led to more
"administrative expense" than gain of performance.

I'm sorry that it took so long to prepare the 2nd patchset (mainly
vacation and other work in between). So I hope it's sufficiently
good for inclusion. :)

Cheers

 1. https://www.mail-archive.com/git@vger.kernel.org/msg86353.html


Stephan Beyer (21):
  bisect: write about `bisect next` in documentation
  bisect: allow 'bisect run' if no good commit is known
  t/test-lib-functions.sh: generalize test_cmp_rev
  t: use test_cmp_rev() where appropriate
  t6030: generalize test to not rely on current implementation
  bisect: add test for the bisect algorithm
  bisect: plug the biggest memory leak
  bisect: make bisect compile if DEBUG_BISECT is set
  bisect: make algorithm behavior independent of DEBUG_BISECT
  bisect: get rid of recursion in count_distance()
  bisect: use struct node_data array instead of int array
  bisect: replace clear_distance() by unique markers
  bisect: use commit instead of commit list as arguments when
appropriate
  bisect: extract get_distance() function from code duplication
  bisect: introduce distance_direction()
  bisect: make total number of commits global
  bisect: rename count_distance() to compute_weight()
  bisect: prepare for different algorithms based on find_all
  bisect: use a bottom-up traversal to find relevant weights
  bisect: compute best bisection in compute_relevant_weights()
  bisect: get back halfway shortcut

 Documentation/git-bisect.txt  |  24 ++
 bisect.c  | 481 --
 git-bisect.sh |  32 +-
 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   | 228 +++---
 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/t8010-bisect-algorithm.sh   | 155 ++
 t/t9162-git-svn-dcommit-interactive.sh|   8 +-
 t/t9300-fast-import.sh|  12 +-
 t/test-lib-functions.sh   |  14 +-
 34 files changed, 832 insertions(+), 508 deletions(-)
 create mode 100755 t/t8010-bisect-algorithm.sh

-- 
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

[Stephan Beyer]

Signed-off-by: Stephan Beyer <s-be...@gmx.net>
---
 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

[Stephan Beyer]

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 <s-be...@gmx.net>
---
 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

[Stephan Beyer]

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 <s-be...@gmx.net>
---
 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, , );
+   char *buf = read_sha1_file(commit->object.oid.hash, , 
);
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, _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

[PATCH v2 17/21] bisect: rename count_distance() to compute_weight()

[Stephan Beyer]

Let us use the term "weight" for the number of ancestors
of each commit, and "distance" for the number
min{weight, #commits - weight}. (Bisect finds the commit
with maximum distance.)

In these terms, "count_distance()" is the wrong name of
the function. So it is renamed to "compute_weight()",
and it also directly sets the computed weight.

Signed-off-by: Stephan Beyer <s-be...@gmx.net>
---
 bisect.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/bisect.c b/bisect.c
index 2b415ad..a254f28 100644
--- a/bisect.c
+++ b/bisect.c
@@ -70,7 +70,7 @@ static inline int distance_direction(struct commit *commit)
return 0;
 }
 
-static int count_distance(struct commit *elem)
+static int compute_weight(struct commit *elem)
 {
int nr = 0;
struct commit_list *todo = NULL;
@@ -93,6 +93,7 @@ static int count_distance(struct commit *elem)
}
}
 
+   node_data(elem)->weight = nr;
return nr;
 }
 
@@ -241,7 +242,7 @@ static struct commit_list *best_bisection_sorted(struct 
commit_list *list)
  * be computed.
  *
  * weight = -2 means it has more than one parent and its distance is
- * unknown.  After running count_distance() first, they will get zero
+ * unknown.  After running compute_weight() first, they will get zero
  * or positive distance.
  */
 static struct commit_list *do_find_bisection(struct commit_list *list,
@@ -286,7 +287,7 @@ static struct commit_list *do_find_bisection(struct 
commit_list *list,
 * If you have only one parent in the resulting set
 * then you can reach one commit more than that parent
 * can reach.  So we do not have to run the expensive
-* count_distance() for single strand of pearls.
+* compute_weight() for single strand of pearls.
 *
 * However, if you have more than one parent, you cannot
 * just add their distance and one for yourself, since
@@ -299,7 +300,7 @@ 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->item);
+   compute_weight(p->item);
 
/* Does it happen to be at exactly half-way? */
if (!find_all && halfway(p->item))
@@ -308,7 +309,7 @@ static struct commit_list *do_find_bisection(struct 
commit_list *list,
}
}
 
-   show_list("bisection 2 count_distance", counted, list);
+   show_list("bisection 2 compute_weight", counted, list);
 
while (counted < total) {
for (p = list; p; p = p->next) {
-- 
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 02/21] bisect: allow 'bisect run' if no good commit is known

[Stephan Beyer]

Now that the documentation talks about bisecting without a good commit
being known, this should also be allowed for "git bisect run".

Signed-off-by: Stephan Beyer <s-be...@gmx.net>
---

Notes:
This is a new patch in the patchset.

 git-bisect.sh | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index 5c93a27..f44ce1e 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -39,6 +39,8 @@ _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
 _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
 TERM_BAD=bad
 TERM_GOOD=good
+IF_NO_GOOD=ask
+AUTONEXT=false
 
 bisect_head()
 {
@@ -312,11 +314,11 @@ bisect_next_check() {
,,*)
: have both $TERM_GOOD and $TERM_BAD - ok
;;
-   *,)
+   *,false)
# do not have both but not asked to fail - just report.
false
;;
-   t,,"$TERM_GOOD")
+   t,,ask)
# have bad (or new) but not good (or old).  we could bisect 
although
# this is less optimum.
eval_gettextln "Warning: bisecting only with a \$TERM_BAD 
commit." >&2
@@ -331,6 +333,9 @@ bisect_next_check() {
fi
: bisect without $TERM_GOOD...
;;
+   t,,true)
+   :
+   ;;
*)
bad_syn=$(bisect_voc bad)
good_syn=$(bisect_voc good)
@@ -343,13 +348,13 @@ revision, you can use \"git bisect next\" to find one." 
>&2
 }
 
 bisect_auto_next() {
-   bisect_next_check && bisect_next || :
+   bisect_next_check $AUTONEXT && bisect_next || :
 }
 
 bisect_next() {
case "$#" in 0) ;; *) usage ;; esac
bisect_autostart
-   bisect_next_check $TERM_GOOD
+   bisect_next_check $IF_NO_GOOD
 
# Perform all bisection computation, display and checkout
git bisect--helper --next-all $(test -f "$GIT_DIR/BISECT_HEAD" && echo 
--no-checkout)
@@ -478,7 +483,9 @@ bisect_replay () {
 }
 
 bisect_run () {
-   bisect_next_check fail
+   bisect_next_check $IF_NO_GOOD
+   AUTONEXT=true
+   IF_NO_GOOD=true
 
while true
do
-- 
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 16/21] bisect: make total number of commits global

[Stephan Beyer]

The total number of commits in a bisect process is a property of
the bisect process. Making this property global helps to make the code
clearer.

Signed-off-by: Stephan Beyer <s-be...@gmx.net>
---
 bisect.c | 74 ++--
 1 file changed, 39 insertions(+), 35 deletions(-)

diff --git a/bisect.c b/bisect.c
index f737ce7..2b415ad 100644
--- a/bisect.c
+++ b/bisect.c
@@ -23,6 +23,8 @@ static const char *argv_show_branch[] = {"show-branch", NULL, 
NULL};
 static const char *term_bad;
 static const char *term_good;
 
+static int total;
+
 static unsigned marker;
 
 struct node_data {
@@ -38,7 +40,7 @@ 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)
+static inline int get_distance(struct commit *commit)
 {
int distance = node_data(commit)->weight;
if (total - distance < distance)
@@ -54,7 +56,7 @@ static inline int get_distance(struct commit *commit, int 
total)
  * Return 0 if the distance is halfway.
  * Return 1 if the distance is rising.
  */
-static inline int distance_direction(struct commit *commit, int total)
+static inline int distance_direction(struct commit *commit)
 {
int doubled_diff = 2 * node_data(commit)->weight - total;
if (doubled_diff < -1)
@@ -107,25 +109,25 @@ static int count_interesting_parents(struct commit 
*commit)
return count;
 }
 
-static inline int halfway(struct commit *commit, int nr)
+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, nr);
+   return !distance_direction(commit);
 }
 
 #if !DEBUG_BISECT
-#define show_list(a,b,c,d) do { ; } while (0)
+#define show_list(a,b,c) do { ; } while (0)
 #else
-static void show_list(const char *debug, int counted, int nr,
+static void show_list(const char *debug, int counted,
  struct commit_list *list)
 {
struct commit_list *p;
 
-   fprintf(stderr, "%s (%d/%d)\n", debug, counted, nr);
+   fprintf(stderr, "%s (%d/%d)\n", debug, counted, total);
 
for (p = list; p; p = p->next) {
struct commit_list *pp;
@@ -157,7 +159,7 @@ static void show_list(const char *debug, int counted, int 
nr,
 }
 #endif /* DEBUG_BISECT */
 
-static struct commit_list *best_bisection(struct commit_list *list, int nr)
+static struct commit_list *best_bisection(struct commit_list *list)
 {
struct commit_list *p, *best;
int best_distance = -1;
@@ -169,7 +171,7 @@ static struct commit_list *best_bisection(struct 
commit_list *list, int nr)
 
if (flags & TREESAME)
continue;
-   distance = get_distance(p->item, nr);
+   distance = get_distance(p->item);
if (distance > best_distance) {
best = p;
best_distance = distance;
@@ -195,10 +197,10 @@ static int compare_commit_dist(const void *a_, const void 
*b_)
return oidcmp(>commit->object.oid, >commit->object.oid);
 }
 
-static struct commit_list *best_bisection_sorted(struct commit_list *list, int 
nr)
+static struct commit_list *best_bisection_sorted(struct commit_list *list)
 {
struct commit_list *p;
-   struct commit_dist *array = xcalloc(nr, sizeof(*array));
+   struct commit_dist *array = xcalloc(total, sizeof(*array));
int cnt, i;
 
for (p = list, cnt = 0; p; p = p->next) {
@@ -207,7 +209,7 @@ static struct commit_list *best_bisection_sorted(struct 
commit_list *list, int n
 
if (flags & TREESAME)
continue;
-   distance = get_distance(p->item, nr);
+   distance = get_distance(p->item);
array[cnt].commit = p->item;
array[cnt].distance = distance;
cnt++;
@@ -243,7 +245,7 @@ static struct commit_list *best_bisection_sorted(struct 
commit_list *list, int n
  * or positive distance.
  */
 static struct commit_list *do_find_bisection(struct commit_list *list,
-int nr, struct node_data *weights,
+struct node_data *weights,
 int find_all)
 {
int n, counted;
@@ -262,7 +264,7 @@ static struct commit_list *do_find_bisection(struct 
commit_list *list,
node_data(commit)->weight = 1;
counted++;
show_list("bisection 2 count one",
- 

[PATCH v2 11/21] bisect: use struct node_data array instead of int array

[Stephan Beyer]

This is a preparation for subsequent changes.
During a bisection process, we want to augment commits with
further information to improve speed.

Signed-off-by: Stephan Beyer <s-be...@gmx.net>
---
 bisect.c | 61 ++---
 1 file changed, 30 insertions(+), 31 deletions(-)

diff --git a/bisect.c b/bisect.c
index 16bbfa6..bc1bfbd 100644
--- a/bisect.c
+++ b/bisect.c
@@ -23,9 +23,21 @@ static const char *argv_show_branch[] = {"show-branch", 
NULL, NULL};
 static const char *term_bad;
 static const char *term_good;
 
+struct node_data {
+   int weight;
+};
+
 /* 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)
+{
+   assert(elem->util);
+   return (struct node_data *)elem->util;
+}
+
 static int count_distance(struct commit_list *entry)
 {
int nr = 0;
@@ -59,18 +71,6 @@ static void clear_distance(struct commit_list *list)
}
 }
 
-#define DEBUG_BISECT 0
-
-static inline int weight(struct commit_list *elem)
-{
-   return *((int*)(elem->item->util));
-}
-
-static inline void weight_set(struct commit_list *elem, int weight)
-{
-   *((int*)(elem->item->util)) = weight;
-}
-
 static int count_interesting_parents(struct commit *commit)
 {
struct commit_list *p;
@@ -95,7 +95,7 @@ static inline int halfway(struct commit_list *p, int nr)
 * 2 and 3 are halfway of 5.
 * 3 is halfway of 6 but 2 and 4 are not.
 */
-   switch (2 * weight(p) - nr) {
+   switch (2 * node_data(p->item)->weight - nr) {
case -1: case 0: case 1:
return 1;
default:
@@ -128,7 +128,7 @@ static void show_list(const char *debug, int counted, int 
nr,
(flags & UNINTERESTING) ? 'U' : ' ',
(flags & COUNTED) ? 'C' : ' ');
if (commit->util)
-   fprintf(stderr, "%3d", weight(p));
+   fprintf(stderr, "%3d", node_data(commit)->weight);
else
fprintf(stderr, "---");
fprintf(stderr, " %.*s", 8, 
sha1_to_hex(commit->object.oid.hash));
@@ -156,7 +156,7 @@ static struct commit_list *best_bisection(struct 
commit_list *list, int nr)
 
if (flags & TREESAME)
continue;
-   distance = weight(p);
+   distance = node_data(p->item)->weight;
if (nr - distance < distance)
distance = nr - distance;
if (distance > best_distance) {
@@ -196,7 +196,7 @@ static struct commit_list *best_bisection_sorted(struct 
commit_list *list, int n
 
if (flags & TREESAME)
continue;
-   distance = weight(p);
+   distance = node_data(p->item)->weight;
if (nr - distance < distance)
distance = nr - distance;
array[cnt].commit = p->item;
@@ -234,7 +234,7 @@ static struct commit_list *best_bisection_sorted(struct 
commit_list *list, int n
  * or positive distance.
  */
 static struct commit_list *do_find_bisection(struct commit_list *list,
-int nr, int *weights,
+int nr, struct node_data *weights,
 int find_all)
 {
int n, counted;
@@ -246,11 +246,11 @@ static struct commit_list *do_find_bisection(struct 
commit_list *list,
struct commit *commit = p->item;
unsigned flags = commit->object.flags;
 
-   p->item->util = [n++];
+   commit->util = [n++];
switch (count_interesting_parents(commit)) {
case 0:
if (!(flags & TREESAME)) {
-   weight_set(p, 1);
+   node_data(commit)->weight = 1;
counted++;
show_list("bisection 2 count one",
  counted, nr, list);
@@ -261,10 +261,10 @@ static struct commit_list *do_find_bisection(struct 
commit_list *list,
 */
break;
case 1:
-   weight_set(p, -1);
+   node_data(commit)->weight = -1;
break;
default:
-   weight_set(p, -2);
+   node_data(commit)->weight = -2;
break;
}
}
@@ -287,8 +287,8 @@ static struct commit_list *do_find_bisection(struct 
commit_list *list,
 */
  

[PATCH v2 05/21] t6030: generalize test to not rely on current implementation

[Stephan Beyer]

The bisect algorithm allows different outcomes if, for example,
the number of commits between a good and a bad commit is even.
The current test relies on a specific behavior (for example,
the behavior of the halfway() implementation). By disabling
halfway(), some skip tests fail although the algorithm works.

This commit generalizes the test t6030 such that it works
even if the bisect algorithm uses its degree of freedom to
choose another commit.

While at it, fix some indentation issues: use tabs instead of
4 spaces.

Signed-off-by: Stephan Beyer <s-be...@gmx.net>
---
 t/t6030-bisect-porcelain.sh | 167 ++--
 1 file changed, 85 insertions(+), 82 deletions(-)

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 05bc639..645ccd9 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -10,36 +10,34 @@ exec > $_file || return $?
-MSG="Add <$_line> into <$_file>."
-else
-echo "$_line" > $_file || return $?
-git add $_file || return $?
-MSG="Create file <$_file> with <$_line> inside."
-fi
+   if [ -f "$_file" ]; then
+   echo "$_line" >> $_file || return $?
+   MSG="Add <$_line> into <$_file>."
+   else
+   echo "$_line" > $_file || return $?
+   git add $_file || return $?
+   MSG="Create file <$_file> with <$_line> inside."
+   fi
 
-test_tick
-git commit --quiet -m "$MSG" $_file
+   test_tick
+   git commit --quiet -m "$MSG" $_file
 }
 
-HASH1=
-HASH2=
-HASH3=
-HASH4=
-
 test_expect_success 'set up basic repo with 1 file (hello) and 4 commits' '
- add_line_into_file "1: Hello World" hello &&
- HASH1=$(git rev-parse --verify HEAD) &&
- add_line_into_file "2: A new day for git" hello &&
- HASH2=$(git rev-parse --verify HEAD) &&
- add_line_into_file "3: Another new day for git" hello &&
- HASH3=$(git rev-parse --verify HEAD) &&
- add_line_into_file "4: Ciao for now" hello &&
- HASH4=$(git rev-parse --verify HEAD)
+   add_line_into_file "1: Hello World" hello &&
+   HASH1=$(git rev-parse --verify HEAD) &&
+   add_line_into_file "2: A new day for git" hello &&
+   HASH2=$(git rev-parse --verify HEAD) &&
+   add_line_into_file "3: Another new day for git" hello &&
+   HASH3=$(git rev-parse --verify HEAD) &&
+   add_line_into_file "4: Ciao for now" hello &&
+   HASH4=$(git rev-parse --verify HEAD) &&
+   git checkout -b monday &&
+   add_line_into_file "5: Ok Monday, let us do it" hello &&
+   git checkout master
 '
 
 test_expect_success 'bisect starts with only one bad' '
@@ -84,9 +82,8 @@ test_expect_success 'bisect fails if given any junk instead 
of revs' '
 
 test_expect_success 'bisect reset: back in the master branch' '
git bisect reset &&
-   echo "* master" > branch.expect &&
git branch > branch.output &&
-   cmp branch.expect branch.output
+   grep "^* master" branch.output
 '
 
 test_expect_success 'bisect reset: back in another branch' '
@@ -95,16 +92,14 @@ test_expect_success 'bisect reset: back in another branch' '
git bisect good $HASH1 &&
git bisect bad $HASH3 &&
git bisect reset &&
-   echo "  master" > branch.expect &&
-   echo "* other" >> branch.expect &&
git branch > branch.output &&
-   cmp branch.expect branch.output
+   grep "^* other" branch.output
 '
 
 test_expect_success 'bisect reset when not bisecting' '
git bisect reset &&
git branch > branch.output &&
-   cmp branch.expect branch.output
+   grep "^* other" branch.output
 '
 
 test_expect_success 'bisect reset removes packed refs' '
@@ -180,14 +175,15 @@ test_expect_success 'bisect start: no ".git/BISECT_START" 
if checkout error' '
git checkout HEAD hello
 '
 
-# $HASH1 is good, $HASH4 is bad, we skip $HASH3
+# $HASH1 is good, monday is bad, we skip $HASH3
 # but $HASH2 is bad,
 # so we should find $HASH2 as the first bad commit
 test_expect_success 'bisect skip: successful result' '
test_when_finished git bisect reset &&
git bisect reset &&
-   git bisect start $HASH4 $HASH1 &&
+   git bisect start monday $HASH1 &&
git bisect skip &&
+   ( test_cmp_rev HEAD $HASH2 || git bisect bad ) &&
   

[PATCH v2 03/21] t/test-lib-functions.sh: generalize test_cmp_rev

[Stephan Beyer]

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.

While at it, the side effect of generating two (temporary) files
is removed.

Signed-off-by: Stephan Beyer <s-be...@gmx.net>
---
 t/test-lib-functions.sh | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 8d99eb3..8caf59c 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -711,11 +711,17 @@ test_must_be_empty () {
fi
 }
 
-# Tests that its two parameters refer to the same revision
+# Tests that the first parameter refers to the same revision
+# of at least one other parameter
 test_cmp_rev () {
-   git rev-parse --verify "$1" >expect.rev &&
-   git rev-parse --verify "$2" >actual.rev &&
-   test_cmp expect.rev actual.rev
+   hash1="$(git rev-parse --verify "$1")" || return
+   shift
+   for rev
+   do
+   hash2="$(git rev-parse --verify "$rev")" || return
+   test "$hash1" = "$hash2" && return 0
+   done
+   return 1
 }
 
 # Print a sequence of numbers or letters in increasing order.  This is
-- 
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 18/21] bisect: prepare for different algorithms based on find_all

[Stephan Beyer]

This is a preparation commit with copy-and-paste involved.
The function do_find_bisection() is changed and copied to
two almost similar functions compute_all_weights() and
compute_relevant_weights().

The function compute_relevant_weights() stops when a
"halfway" commit is found.

To keep the code clean, the halfway commit is not returned
and has to be found by best_bisection() afterwards.
This results in a singular additional O(#commits)-time
overhead but this will be outweighed by the following
changes to compute_relevant_weights().

It is necessary to keep compute_all_weights() for the
"git rev-list --bisect-all" command. All other bisect-related
commands will use compute_relevant_weights().

Signed-off-by: Stephan Beyer <s-be...@gmx.net>
---
 bisect.c | 116 ---
 1 file changed, 97 insertions(+), 19 deletions(-)

diff --git a/bisect.c b/bisect.c
index a254f28..c6bad43 100644
--- a/bisect.c
+++ b/bisect.c
@@ -179,6 +179,7 @@ static struct commit_list *best_bisection(struct 
commit_list *list)
}
}
 
+   best->next = NULL;
return best;
 }
 
@@ -245,9 +246,8 @@ static struct commit_list *best_bisection_sorted(struct 
commit_list *list)
  * unknown.  After running compute_weight() first, they will get zero
  * or positive distance.
  */
-static struct commit_list *do_find_bisection(struct commit_list *list,
-struct node_data *weights,
-int find_all)
+static void compute_all_weights(struct commit_list *list,
+   struct node_data *weights)
 {
int n, counted;
struct commit_list *p;
@@ -301,10 +301,88 @@ static struct commit_list *do_find_bisection(struct 
commit_list *list,
if (!(p->item->object.flags & UNINTERESTING)
 && (node_data(p->item)->weight == -2)) {
compute_weight(p->item);
+   counted++;
+   }
+   }
+
+   show_list("bisection 2 compute_weight", counted, list);
+
+   while (counted < total) {
+   for (p = list; p; p = p->next) {
+   struct commit_list *q;
+   unsigned flags = p->item->object.flags;
+
+   if (0 <= node_data(p->item)->weight)
+   continue;
+   for (q = p->item->parents; q; q = q->next) {
+   if (q->item->object.flags & UNINTERESTING)
+   continue;
+   if (0 <= node_data(q->item)->weight)
+   break;
+   }
+   if (!q)
+   continue;
+
+   /*
+* weight for p is unknown but q is known.
+* add one for p itself if p is to be counted,
+* otherwise inherit it from q directly.
+*/
+   node_data(p->item)->weight = node_data(q->item)->weight;
+   if (!(flags & TREESAME)) {
+   node_data(p->item)->weight++;
+   counted++;
+   show_list("bisection 2 count one",
+ counted, 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 void compute_relevant_weights(struct commit_list *list,
+struct node_data *weights)
+{
+   int n, counted;
+   struct commit_list *p;
+
+   counted = 0;
+
+   for (n = 0, p = list; p; p = p->next) {
+   struct commit *commit = p->item;
+   unsigned flags = commit->object.flags;
+
+   commit->util = [n++];
+   switch (count_interesting_parents(commit)) {
+   case 0:
+   if (!(flags & TREESAME)) {
+   node_data(commit)->weight = 1;
+   counted++;
+   show_list("bisection 2 count one",
+ counted, list);
+   }
+   break;
+   case 1:
+   node_data(commit)->weight = -1;
+   break;
+   default:
+   node_data(commit)->weight = -2;
+   break;
+   }
+   }
+
+   show_list("b