Hi,
Thanks for the review,
Junio C Hamano <gitster@xxxxxxxxx> writes:
>> /*
>> + * The terms used for this bisect session are stocked in
>> + * BISECT_TERMS: it can be bad/good or new/old.
>> + * We read them and stock them to adapt the messages
>> + * accordingly. Default is bad/good.
>> + */
>
>s/stock/store/ perhaps? I think the idea is not to have this file
>in the default case so that absence of it would mean you would be
>looking for a transition from (older) good to (more recent) bad.
Yes it is nice but a bisect_terms file is a very useful tool for
verifications at a little cost. For instance, if the user type this:
git bisect start
git bisect bad HEAD
git bisect old HEAD~10
We created the bisect_terms file after the bad then the error is
directly detected when the user type git bisect old.
And I think having we should limit the impact of this good/bad
default case as we would prefer old/new.
>> +void read_bisect_terms(void)
>> +{
>> + struct strbuf str = STRBUF_INIT;
>> + const char *filename = git_path("BISECT_TERMS");
>> + FILE *fp = fopen(filename, "r");
>> +
>> + if (!fp) {
>
>We might want to see why fopen() failed here. If it is because the
>file did not exist, great. But otherwise?
Should we display a specific message and cancel the last command?
>> diff --git a/git-bisect.sh b/git-bisect.sh
>> index 1f16aaf..529bb43 100644
>> --- a/git-bisect.sh
>> +++ b/git-bisect.sh
>> @@ -77,6 +77,7 @@ bisect_start() {
>> orig_args=$(git rev-parse --sq-quote "$@")
>> bad_seen=0
>> eval=''
>> + start_bad_good=0
>> if test "z$(git rev-parse --is-bare-repository)" != zfalse
>> then
>> mode=--no-checkout
>> @@ -101,6 +102,9 @@ bisect_start() {
>> die "$(eval_gettext "'\$arg' does not appear to be a valid revision")"
>> break
>> }
>> +
>> + start_bad_good=1
>> +
>
>It is unclear what this variable means, or what it means to have
>zero or one as its value.
This has been done by our elders and we kept because it was
useful. We are however trying to remove it. It is in case of
a 'git bisect start bad_rev good_rev', our function that creates
the bisect_terms is not called. Thus we need to do it manually
in the code.
>> +get_terms () {
>> + if test -s "$GIT_DIR/BISECT_TERMS"
>> + then
>> + NAME_BAD="$(sed -n 1p "$GIT_DIR/BISECT_TERMS")"
>> + NAME_GOOD="$(sed -n 2p "$GIT_DIR/BISECT_TERMS")"
>
>It is sad that we need to open the file twice. Can't we do
>something using "read" perhaps?
The cost of it is quite low and we see directly what we meant. We didn't
found a pretty way to read two lines with read.
>Don't we want to make sure these two names are sensible? We do not
>want an empty-string, for example. I suspect you do not want to
>take anything that check-ref-format does not like.
>
>Same comment applies to the C code.
Yes, for now only bad/good/old/new are allowed. But in the future
it will be necessary.
>> +bisect_voc () {
>> + case "$1" in
>> + bad) echo "bad" ;;
>> + good) echo "good" ;;
>> + esac
>> +}
>
>What is voc?
>
>What if "$1" is neither bad/good?
>
>Did you mean to translate 'bad' to $NAME_BAD and 'good' to $NAME_GOOD?
voc stands for vocabulary.
This fonction is mainly used after to display all the builtins possibility. It
is only called internally and if the argument is bad it returns the synonyms of
bad (bad|new in the next patch).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html