* [PATCH 1/2] git-bisect.sh : create a file if the bisection is in old/new mode, named "BISECT_OLDNEWMODE", so it can easily be seen outside the program without having to read BISECT_TERMS. This will have to be changed in further versions if new terms are introduced.
@ 2015-06-05 16:34 Louis Stuber
2015-06-05 16:34 ` [PATCH 2/2] Fix git rev-list --bisect and git bisect visualize when the bisection is done in old/new mode Louis Stuber
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Louis Stuber @ 2015-06-05 16:34 UTC (permalink / raw)
To: git
Cc: remi.galan-alfonso, remi.lespinet, matthieu.moy, guillaume.pages,
antoine.delaite, j_franck7, valentinduperray, thomasxnguy,
lucienkong, chriscool, Louis Stuber
Signed-off-by: Louis Stuber <stuberl@ensimag•grenoble-inp.fr>
Signed-off-by: Antoine Delaite <antoine.delaite@ensimag•grenoble-inp.fr>
---
git-bisect.sh | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/git-bisect.sh b/git-bisect.sh
index 109bd65..d3d19cb 100644
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -183,6 +183,10 @@ bisect_start() {
then
echo "$BISECT_BAD" >"$GIT_DIR/BISECT_TERMS" &&
echo "$BISECT_GOOD" >>"$GIT_DIR/BISECT_TERMS"
+ if test "$BISECT_BAD" = "new"
+ then
+ echo "" > "$GIT_DIR/BISECT_OLDNEWMODE"
+ fi
fi &&
echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
#
@@ -416,6 +420,7 @@ bisect_clean_state() {
rm -f "$GIT_DIR/BISECT_NAMES" &&
rm -f "$GIT_DIR/BISECT_RUN" &&
rm -f "$GIT_DIR/BISECT_TERMS" &&
+ rm -f "$GIT_DIR/BISECT_OLDNEWMODE" &&
# Cleanup head-name if it got left by an old version of git-bisect
rm -f "$GIT_DIR/head-name" &&
git update-ref -d --no-deref BISECT_HEAD &&
@@ -544,7 +549,8 @@ check_and_set_terms () {
if test ! -s "$GIT_DIR/BISECT_TERMS"
then
echo "new" >"$GIT_DIR/BISECT_TERMS" &&
- echo "old" >>"$GIT_DIR/BISECT_TERMS"
+ echo "old" >>"$GIT_DIR/BISECT_TERMS" &&
+ echo "" > "$GIT_DIR/BISECT_OLDNEWMODE"
fi
BISECT_BAD="new"
BISECT_GOOD="old" ;;
--
1.7.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 2/2] Fix git rev-list --bisect and git bisect visualize when the bisection is done in old/new mode. 2015-06-05 16:34 [PATCH 1/2] git-bisect.sh : create a file if the bisection is in old/new mode, named "BISECT_OLDNEWMODE", so it can easily be seen outside the program without having to read BISECT_TERMS. This will have to be changed in further versions if new terms are introduced Louis Stuber @ 2015-06-05 16:34 ` Louis Stuber 2015-06-05 20:24 ` Eric Sunshine 2015-06-05 20:03 ` [PATCH 1/2] git-bisect.sh : create a file if the bisection is in old/new mode, named "BISECT_OLDNEWMODE", so it can easily be seen outside the program without having to read BISECT_TERMS. This will have to be changed in further versions if new terms are introduced Eric Sunshine 2015-06-05 20:24 ` Christian Couder 2 siblings, 1 reply; 8+ messages in thread From: Louis Stuber @ 2015-06-05 16:34 UTC (permalink / raw) To: git Cc: remi.galan-alfonso, remi.lespinet, matthieu.moy, guillaume.pages, antoine.delaite, j_franck7, valentinduperray, thomasxnguy, lucienkong, chriscool, Louis Stuber Signed-off-by: Louis Stuber <stuberl@ensimag•grenoble-inp.fr> Signed-off-by: Antoine Delaite <antoine.delaite@ensimag•grenoble-inp.fr> --- revision.c | 15 +++++++++++++-- 1 files changed, 13 insertions(+), 2 deletions(-) diff --git a/revision.c b/revision.c index 7ddbaa0..b631596 100644 --- a/revision.c +++ b/revision.c @@ -2075,12 +2075,23 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx, static int for_each_bad_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data) { - return for_each_ref_in_submodule(submodule, "refs/bisect/bad", fn, cb_data); + /* + * if BISECT_OLDNEWMODE exists, this is an old/new bisect and the path is different + */ + struct stat st; + if (stat(git_path("BISECT_OLDNEWMODE"), &st)) + return for_each_ref_in_submodule(submodule, "refs/bisect/bad", fn, cb_data); + else + return for_each_ref_in_submodule(submodule, "refs/bisect/new", fn, cb_data); } static int for_each_good_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data) { - return for_each_ref_in_submodule(submodule, "refs/bisect/good", fn, cb_data); + struct stat st; + if (stat(git_path("BISECT_OLDNEWMODE"), &st)) + return for_each_ref_in_submodule(submodule, "refs/bisect/good", fn, cb_data); + else + return for_each_ref_in_submodule(submodule, "refs/bisect/old", fn, cb_data); } static int handle_revision_pseudo_opt(const char *submodule, -- 1.7.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Fix git rev-list --bisect and git bisect visualize when the bisection is done in old/new mode. 2015-06-05 16:34 ` [PATCH 2/2] Fix git rev-list --bisect and git bisect visualize when the bisection is done in old/new mode Louis Stuber @ 2015-06-05 20:24 ` Eric Sunshine 0 siblings, 0 replies; 8+ messages in thread From: Eric Sunshine @ 2015-06-05 20:24 UTC (permalink / raw) To: Louis Stuber Cc: Git List, Galan Rémi, Remi Lespinet, Matthieu Moy, Guillaume Pages, Antoine Delaite, j_franck7, valentinduperray, thomasxnguy, lucienkong, Christian Couder On Fri, Jun 5, 2015 at 12:34 PM, Louis Stuber <stuberl@ensimag•grenoble-inp.fr> wrote: > Fix git rev-list --bisect and git bisect visualize when the bisection > is done in old/new mode. See my review of patch 1/2 regarding writing a good commit message. In particular, explain what is broken about "git rev-list --bisect" and "git bisect visualize" so that the reader can understand what this patch is actually fixing. > Signed-off-by: Louis Stuber <stuberl@ensimag•grenoble-inp.fr> > Signed-off-by: Antoine Delaite <antoine.delaite@ensimag•grenoble-inp.fr> > --- > diff --git a/revision.c b/revision.c > index 7ddbaa0..b631596 100644 > --- a/revision.c > +++ b/revision.c > @@ -2075,12 +2075,23 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx, > > static int for_each_bad_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data) > { > - return for_each_ref_in_submodule(submodule, "refs/bisect/bad", fn, cb_data); > + /* > + * if BISECT_OLDNEWMODE exists, this is an old/new bisect and the path is different > + */ Comments which merely repeat what the code itself already clearly says don't add value, and are thus noise which impede comprehension by distracting the reader from digesting the underlying logic flow. > + struct stat st; > + if (stat(git_path("BISECT_OLDNEWMODE"), &st)) > + return for_each_ref_in_submodule(submodule, "refs/bisect/bad", fn, cb_data); > + else > + return for_each_ref_in_submodule(submodule, "refs/bisect/new", fn, cb_data); Since the two for_each_ref_in_submodule() calls are identical except for the second argument, the more natural and easier to comprehend way to phrase this would be be to assign "refs/bisect/bad" or "refs/bisect/new" to a variable, and then have just a single invocation of for_each_ref_in_submodule() which uses that variable as its second argument. Stepping back a moment: My reading of these two patches is that BISECT_OLDNEWMODE is introduced as a simple way to detect if "old/new" mode is being used rather than gleaning that knowledge from the existing BISECT_TERMS file. Is that correct? If so, then these changes are likely going in the wrong direction. The ominous final sentence of the commit message of patch 1/2 is already a good clue that this approach won't scale well. Further, the approach taken here undesirably emphasizes ease of implementation and its attendant fragility over well thought out design. > } > > static int for_each_good_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data) > { > - return for_each_ref_in_submodule(submodule, "refs/bisect/good", fn, cb_data); > + struct stat st; > + if (stat(git_path("BISECT_OLDNEWMODE"), &st)) > + return for_each_ref_in_submodule(submodule, "refs/bisect/good", fn, cb_data); > + else > + return for_each_ref_in_submodule(submodule, "refs/bisect/old", fn, cb_data); > } > > static int handle_revision_pseudo_opt(const char *submodule, > -- > 1.7.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] git-bisect.sh : create a file if the bisection is in old/new mode, named "BISECT_OLDNEWMODE", so it can easily be seen outside the program without having to read BISECT_TERMS. This will have to be changed in further versions if new terms are introduced. 2015-06-05 16:34 [PATCH 1/2] git-bisect.sh : create a file if the bisection is in old/new mode, named "BISECT_OLDNEWMODE", so it can easily be seen outside the program without having to read BISECT_TERMS. This will have to be changed in further versions if new terms are introduced Louis Stuber 2015-06-05 16:34 ` [PATCH 2/2] Fix git rev-list --bisect and git bisect visualize when the bisection is done in old/new mode Louis Stuber @ 2015-06-05 20:03 ` Eric Sunshine 2015-06-08 11:48 ` Matthieu Moy 2015-06-05 20:24 ` Christian Couder 2 siblings, 1 reply; 8+ messages in thread From: Eric Sunshine @ 2015-06-05 20:03 UTC (permalink / raw) To: Louis Stuber Cc: Git List, Galan Rémi, Remi Lespinet, Matthieu Moy, Guillaume Pages, Antoine Delaite, j_franck7, valentinduperray, thomasxnguy, lucienkong, Christian Couder On Fri, Jun 5, 2015 at 12:34 PM, Louis Stuber <stuberl@ensimag•grenoble-inp.fr> wrote: > git-bisect.sh : create a file if the bisection is in old/new mode, > named "BISECT_OLDNEWMODE", so it can easily be seen outside the > program without having to read BISECT_TERMS. This will have to be > changed in further versions if new terms are introduced. Documentation/SubmittingPatches contains instructions for how to write a good commit message. The first line should be a very brief high-level overview of the change, followed by a blank line, followed by one or more paragraphs justifying and explaining the change. Also, wrap the commit message to 70-72 columns. This commit message doesn't do a very good job of explaining the problem this change is trying to solve or justifying why this solution is preferable. Justification is particularly important considering the ominous-sounding final sentence of the commit message (which itself seems to imply that this is not a very good change). > Signed-off-by: Louis Stuber <stuberl@ensimag•grenoble-inp.fr> > Signed-off-by: Antoine Delaite <antoine.delaite@ensimag•grenoble-inp.fr> > --- > diff --git a/git-bisect.sh b/git-bisect.sh > index 109bd65..d3d19cb 100644 > --- a/git-bisect.sh > +++ b/git-bisect.sh > @@ -183,6 +183,10 @@ bisect_start() { > then > echo "$BISECT_BAD" >"$GIT_DIR/BISECT_TERMS" && > echo "$BISECT_GOOD" >>"$GIT_DIR/BISECT_TERMS" > + if test "$BISECT_BAD" = "new" Nit: Unnecessary quotes around "new" make the code a bit more noisy, thus slightly more difficult to read. > + then > + echo "" > "$GIT_DIR/BISECT_OLDNEWMODE" Style: Drop space after redirection operator. If only the file's existence is important, but not its content, then you could phrase this more concisely without the 'echo'. Just use the redirection operator without any command in front of it: >"$somefile" Same comments apply below. > + fi > fi && > echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit > # > @@ -416,6 +420,7 @@ bisect_clean_state() { > rm -f "$GIT_DIR/BISECT_NAMES" && > rm -f "$GIT_DIR/BISECT_RUN" && > rm -f "$GIT_DIR/BISECT_TERMS" && > + rm -f "$GIT_DIR/BISECT_OLDNEWMODE" && > # Cleanup head-name if it got left by an old version of git-bisect > rm -f "$GIT_DIR/head-name" && > git update-ref -d --no-deref BISECT_HEAD && > @@ -544,7 +549,8 @@ check_and_set_terms () { > if test ! -s "$GIT_DIR/BISECT_TERMS" > then > echo "new" >"$GIT_DIR/BISECT_TERMS" && > - echo "old" >>"$GIT_DIR/BISECT_TERMS" > + echo "old" >>"$GIT_DIR/BISECT_TERMS" && > + echo "" > "$GIT_DIR/BISECT_OLDNEWMODE" > fi > BISECT_BAD="new" > BISECT_GOOD="old" ;; > -- > 1.7.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] git-bisect.sh : create a file if the bisection is in old/new mode, named "BISECT_OLDNEWMODE", so it can easily be seen outside the program without having to read BISECT_TERMS. This will have to be changed in further versions if new terms are introduced. 2015-06-05 20:03 ` [PATCH 1/2] git-bisect.sh : create a file if the bisection is in old/new mode, named "BISECT_OLDNEWMODE", so it can easily be seen outside the program without having to read BISECT_TERMS. This will have to be changed in further versions if new terms are introduced Eric Sunshine @ 2015-06-08 11:48 ` Matthieu Moy 0 siblings, 0 replies; 8+ messages in thread From: Matthieu Moy @ 2015-06-08 11:48 UTC (permalink / raw) To: Eric Sunshine Cc: Louis Stuber, Git List, Galan Rémi, Remi Lespinet, Guillaume Pages, Antoine Delaite, j_franck7, valentinduperray, thomasxnguy, lucienkong, Christian Couder Eric Sunshine <sunshine@sunshineco•com> writes: > On Fri, Jun 5, 2015 at 12:34 PM, Louis Stuber > <stuberl@ensimag•grenoble-inp.fr> wrote: >> git-bisect.sh : No space before : in english. >> create a file if the bisection is in old/new mode, named >> "BISECT_OLDNEWMODE", so it can easily be seen outside the program >> without having to read BISECT_TERMS. This will have to be changed in >> further versions if new terms are introduced. > > Documentation/SubmittingPatches contains instructions for how to write > a good commit message. For french-speaking people, and Ensimag students in particular, I'd add http://ensiwiki.ensimag.fr/index.php/%C3%89crire_de_bons_messages_de_commit_avec_Git > Also, wrap the commit message to 70-72 columns. As much as possible, the summary line should even be shorter (so that "git log --oneline" fits on a 80-chars terminal). > This commit message doesn't do a very good job of explaining the > problem this change is trying to solve or justifying why this solution > is preferable. Actually, the commit message explains one reason why this is not a good solution: the idea of having $GIT_DIR/BISECT_TERMS was to keep the solution generic. Had the initial codebase been better factored, this patch series would have been really trivial, but we hardcoded "good" and "bad" in many places, and now changing it is hard. Introducing BISECT_TERMS is a step forward, it avoids hardcoding the terms here and there in the code. To me, introducing BISECT_OLDNEWMODE is a step backward, it's one more place where we hardcode the terms. > Justification is particularly important considering the > ominous-sounding final sentence of the commit message (which itself > seems to imply that this is not a very good change). Ah, indeed, we're saying the same thing. >> - echo "old" >>"$GIT_DIR/BISECT_TERMS" >> + echo "old" >>"$GIT_DIR/BISECT_TERMS" && >> + echo "" > "$GIT_DIR/BISECT_OLDNEWMODE" No space after > (noted by Eric elsewhere) -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] git-bisect.sh : create a file if the bisection is in old/new mode, named "BISECT_OLDNEWMODE", so it can easily be seen outside the program without having to read BISECT_TERMS. This will have to be changed in further versions if new terms are introduced. 2015-06-05 16:34 [PATCH 1/2] git-bisect.sh : create a file if the bisection is in old/new mode, named "BISECT_OLDNEWMODE", so it can easily be seen outside the program without having to read BISECT_TERMS. This will have to be changed in further versions if new terms are introduced Louis Stuber 2015-06-05 16:34 ` [PATCH 2/2] Fix git rev-list --bisect and git bisect visualize when the bisection is done in old/new mode Louis Stuber 2015-06-05 20:03 ` [PATCH 1/2] git-bisect.sh : create a file if the bisection is in old/new mode, named "BISECT_OLDNEWMODE", so it can easily be seen outside the program without having to read BISECT_TERMS. This will have to be changed in further versions if new terms are introduced Eric Sunshine @ 2015-06-05 20:24 ` Christian Couder [not found] ` <1179544255.257552.1433703835857.JavaMail.zimbra@ensimag.grenoble-inp.fr> 2 siblings, 1 reply; 8+ messages in thread From: Christian Couder @ 2015-06-05 20:24 UTC (permalink / raw) To: Louis Stuber Cc: git, remi.galan-alfonso, remi.lespinet, Matthieu Moy, guillaume.pages, Antoine Delaite, j_franck7, Valentin Duperray, Thomas Nguy, lucienkong, Christian Couder On Fri, Jun 5, 2015 at 6:34 PM, Louis Stuber <stuberl@ensimag•grenoble-inp.fr> wrote: > > Signed-off-by: Louis Stuber <stuberl@ensimag•grenoble-inp.fr> > Signed-off-by: Antoine Delaite <antoine.delaite@ensimag•grenoble-inp.fr> > --- It looks like this patch applies on top of the bisect old/new series posted by Antoine. This should be stated somewhere. > git-bisect.sh | 8 +++++++- > 1 files changed, 7 insertions(+), 1 deletions(-) > > diff --git a/git-bisect.sh b/git-bisect.sh > index 109bd65..d3d19cb 100644 > --- a/git-bisect.sh > +++ b/git-bisect.sh > @@ -183,6 +183,10 @@ bisect_start() { > then > echo "$BISECT_BAD" >"$GIT_DIR/BISECT_TERMS" && > echo "$BISECT_GOOD" >>"$GIT_DIR/BISECT_TERMS" > + if test "$BISECT_BAD" = "new" > + then > + echo "" > "$GIT_DIR/BISECT_OLDNEWMODE" > + fi I am not sure it's worth it to have both BISECT_TERMS and BISECT_OLDNEWMODE. Also please note that I suggested to Antoine that the BISECT_BAD and BISECT_GOOD variables be renamed to something else, like I already did in some C files. ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <1179544255.257552.1433703835857.JavaMail.zimbra@ensimag.grenoble-inp.fr>]
* Re: [PATCH 1/2] git-bisect.sh : create a file if the bisection is in old/new mode, named "BISECT_OLDNEWMODE", so it can easily be seen outside the program without having to read BISECT_TERMS. This will have to be changed in further versions if new terms are introduced. [not found] ` <1179544255.257552.1433703835857.JavaMail.zimbra@ensimag.grenoble-inp.fr> @ 2015-06-07 19:06 ` Louis-Alexandre Stuber 2015-06-08 11:54 ` Matthieu Moy 1 sibling, 0 replies; 8+ messages in thread From: Louis-Alexandre Stuber @ 2015-06-07 19:06 UTC (permalink / raw) To: Christian Couder Cc: git, remi galan-alfonso, remi lespinet, Matthieu Moy, guillaume pages, Antoine Delaite, j franck7, Valentin Duperray, Thomas Nguy, lucienkong, Christian Couder Thank you for the feedback. We are trying to apply all of your suggestions, but we would prefer to rebase the history before doing some of them (like renaming variables). About the BISECT_OLDNEWMODE file: The current implementation changes almost nothing to revision.c. We thought it was better, even if it needs a new file. The code for bisect uses BISECT_TERMS because 3 states are possible: 'bad/good mode', 'old/new mode', or 'no bisection started' (if BISECT_TERMS doesn't exist). But the other files (like revision.c) don't need all these informations, so we thought it would be good to check if a file exists instead of reusing BISECT_TERMS, which would require reading its content. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] git-bisect.sh : create a file if the bisection is in old/new mode, named "BISECT_OLDNEWMODE", so it can easily be seen outside the program without having to read BISECT_TERMS. This will have to be changed in further versions if new terms are introduced. [not found] ` <1179544255.257552.1433703835857.JavaMail.zimbra@ensimag.grenoble-inp.fr> 2015-06-07 19:06 ` Louis-Alexandre Stuber @ 2015-06-08 11:54 ` Matthieu Moy 1 sibling, 0 replies; 8+ messages in thread From: Matthieu Moy @ 2015-06-08 11:54 UTC (permalink / raw) To: Louis-Alexandre Stuber Cc: Christian Couder, git, remi galan-alfonso, remi lespinet, guillaume pages, Antoine Delaite, j franck7, Valentin Duperray, Thomas Nguy, lucienkong, Christian Couder Please, don't top-post on this list. Louis-Alexandre Stuber <stuberl@ensimag•grenoble-inp.fr> writes: > Thank you for the feedback. We are trying to apply all of your suggestions, but we would prefer to rebase the history before doing some of them (like renaming variables). > > About the BISECT_OLDNEWMODE file: The current implementation changes almost nothing to revision.c. We thought it was better, even if it needs a new file. The code for bisect uses BISECT_TERMS because 3 states are > possible: 'bad/good mode', 'old/new mode', or 'no bisection started' > (if BISECT_TERMS doesn't exist). I don't think it's the main reason. The point is to make the code generic: once the bisection has started and the terms are chosen, the possible states for a commit are not really bad/good or old/new, but 'first line in BISECT_TERMS/second line in BISECT_TERMS'. > But the other files (like revision.c) don't need all these > informations, so we thought it would be good to check if a file exists > instead of reusing BISECT_TERMS, which would require reading its > content. > > ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- > De: "Christian Couder" <christian.couder@gmail•com> > ... -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-06-08 11:55 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-06-05 16:34 [PATCH 1/2] git-bisect.sh : create a file if the bisection is in old/new mode, named "BISECT_OLDNEWMODE", so it can easily be seen outside the program without having to read BISECT_TERMS. This will have to be changed in further versions if new terms are introduced Louis Stuber
2015-06-05 16:34 ` [PATCH 2/2] Fix git rev-list --bisect and git bisect visualize when the bisection is done in old/new mode Louis Stuber
2015-06-05 20:24 ` Eric Sunshine
2015-06-05 20:03 ` [PATCH 1/2] git-bisect.sh : create a file if the bisection is in old/new mode, named "BISECT_OLDNEWMODE", so it can easily be seen outside the program without having to read BISECT_TERMS. This will have to be changed in further versions if new terms are introduced Eric Sunshine
2015-06-08 11:48 ` Matthieu Moy
2015-06-05 20:24 ` Christian Couder
[not found] ` <1179544255.257552.1433703835857.JavaMail.zimbra@ensimag.grenoble-inp.fr>
2015-06-07 19:06 ` Louis-Alexandre Stuber
2015-06-08 11:54 ` Matthieu Moy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox