Re: [PATCH 3/4] bisect: simplify the add of new bisect terms
From: Junio C Hamano <hidden>
Date: 2016-06-15 23:05:13
Antoine Delaite [off-list ref] writes:
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 <redacted> Signed-off-by: Louis Stuber <redacted> Signed-off-by: Valentin Duperray <redacted> Signed-off-by: Franck Jonas <redacted> Signed-off-by: Lucien Kong <redacted> Signed-off-by: Thomas Nguy <redacted> Signed-off-by: Huynh Khoi Nguyen Nguyen <redacted> Signed-off-by: Matthieu Moy <redacted> ---
This step seems very straight-forward and makes sense from a cursory look.
/* + * 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.
+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?
quoted hunk
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.
quoted hunk
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").
+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.
+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?