Re: [PATCH 3/4] bisect: simplify the add of new bisect terms
- From
Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr>
- Date
- Jun 9, 2015, 18:17 UTC
- Message-ID
- <2139695602.323234.1433873829922.JavaMail.zimbra@ensimag.grenoble-inp.fr>
- In-Reply-To
- <xmqqvbey7wzp.fsf@gitster.dls.corp.google.com>
Hi,
Thanks for the review,
Junio C Hamano <gitster@pobox.com> writes:
Show 10 quoted lines
>> /* >> + * 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.
Show 10 quoted lines
>> +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?
Show 22 quoted lines
>> 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.
Show 8 quoted lines
>> +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.
Show 5 quoted lines
>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.
Show 12 quoted lines
>> +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).