Re: [PATCH 3/4] bisect: simplify the add of new bisect terms
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 8, 2015, 20:42 UTC
- Message-ID
- <xmqqvbey7wzp.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1433794930-5158-3-git-send-email-antoine.delaite@ensimag.grenoble-inp.fr>
Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr> writes:
Show 18 quoted lines
> We create a file BISECT_TERMS in the repository .git to be read during a > bisection. The fonctions to be changed if we add new terms are quite > few. > In git-bisect.sh : > check_and_set_terms > bisect_voc > In bisect.c : > handle_bad_merge_base > > Signed-off-by: Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr> > Signed-off-by: Louis Stuber <stuberl@ensimag.grenoble-inp.fr> > Signed-off-by: Valentin Duperray <Valentin.Duperray@ensimag.imag.fr> > Signed-off-by: Franck Jonas <Franck.Jonas@ensimag.imag.fr> > Signed-off-by: Lucien Kong <Lucien.Kong@ensimag.imag.fr> > Signed-off-by: Thomas Nguy <Thomas.Nguy@ensimag.imag.fr> > Signed-off-by: Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr> > Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> > ---
This step seems very straight-forward and makes sense from a cursory look.
Show 6 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.
Show 7 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?
Show 19 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.
Show 8 quoted lines
> case $bad_seen in
> 0) state='bad' ; bad_seen=1 ;;
> *) state='good' ;;
> @@ -172,6 +176,11 @@ bisect_start() {
> } &&
> git rev-parse --sq-quote "$@" >"$GIT_DIR/BISECT_NAMES" &&
> eval "$eval true" &&
> + if test $start_bad_good -eq 1 -a ! -s "$GIT_DIR/BISECT_TERMS"Avoid "test <condition1> -a <condition2>" (or "-o").
Show 5 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?
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.
Show 6 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?