Re: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
Ramsay Jones writes: > On 03/10/17 04:51, Junio C Hamano wrote: >> >> It seems that Pranit needs a bit more work to take known fixes from >> your efforts and we should wait for the series to be rerolled? > > This series is just the first few patches from the original 28/29 > patch series; in particular, patches 1-5 and 8 of that series. > ... > So, the major differences and bug fixes are in later patches. OK. So these are good to go separately (as prerequistes for the follow-on work)--thanks for clarification.
Re: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
On 03/10/17 04:51, Junio C Hamano wrote:
> Ramsay Jones writes:
>
>> On 02/10/17 14:44, Pranit Bauva wrote:
>> [snip]
>>> ...
>> Yes, I also meant to tidy that up by removing some, now
>> redundant, initialisation later in that function.
>>
>> Note, that wasn't the only bug! (I have probably forgotten
>> some of them, but look at 964f4e2b0, for example).
>
> It seems that Pranit needs a bit more work to take known fixes from
> your efforts and we should wait for the series to be rerolled?
This series is just the first few patches from the original 28/29
patch series; in particular, patches 1-5 and 8 of that series.
If I compare just the first 5 patches, then the differences are
small and (maybe) not worth a re-roll. For some reason, I changed
the message passed to the delete_refs() call from "bisect: remove"
to "bisect: clean", we both added "terms" to the 'builtin command'
list but in different positions and some calls to die() have
been replaced with 'return error(...)'.
Viz:
$ git log --oneline v2.14.0..bisect~34
d8ce077c8 t6030: explicitly test for bisection cleanup
1fd8e44a1 bisect--helper: `bisect_clean_state` shell function in C
5aa9d18eb bisect--helper: `write_terms` shell function in C
1b94b2ff1 bisect: rewrite `check_term_format` shell function in C
a97a96ccf bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
$
$ git log --oneline v2.14.0..pranit~1
6012123c7 t6030: explicitly test for bisection cleanup
7c945c391 bisect--helper: `bisect_clean_state` shell function in C
9240c3962 bisect--helper: `write_terms` shell function in C
ba496589c bisect--helper: rewrite `check_term_format` shell function in C
f1563a33f bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
$
Note that the subject line of patch #2 has also been corrected
in this new series (bisect: -> bisect--helper:). I haven't
compared the commit messages.
$ git diff bisect~34 pranit~1
diff --git a/bisect.c b/bisect.c
index b19311ca7..2838d672d 100644
--- a/bisect.c
+++ b/bisect.c
@@ -1066,7 +1066,7 @@ int bisect_clean_state(void)
struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
for_each_ref_in("refs/bisect", mark_for_removal, (void *)
&refs_for_removal);
string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
- result = delete_refs("bisect: clean", &refs_for_removal, REF_NODEREF);
+ result = delete_refs("bisect: remove", &refs_for_removal, REF_NODEREF);
refs_for_removal.strdup_strings = 1;
string_list_clear(&refs_for_removal, 0);
unlink_or_warn(git_path_bisect_expected_rev());
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 2af024f60..0f4d4e41c 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -43,8 +43,8 @@ static int check_term_format(const char *term, const char
*orig_term)
if (res)
return error(_("'%s' is not a valid term"), term);
- if (one_of(term, "help", "start", "terms", "skip", "next", "reset",
- "visualize", "replay", "log", "run", NULL))
+ if (one_of(term, "help", "start", "skip", "next", "reset",
+ "visualize", "replay", "log", "run", "terms", NULL))
return error(_("can't use the builtin command '%s' as a term"),
term);
/*
@@ -111,14 +111,14 @@ int cmd_bisect__helper(int argc, const char **argv,
const char *prefix)
return bisect_next_all(prefix, no_checkout);
case WRITE_TERMS:
if (argc != 2)
- die(_("--write-terms requires two arguments"));
+ return error(_("--write-terms requires two arguments"));
return write_terms(argv[0], argv[1]);
case BISECT_CLEAN_STATE:
if (argc != 0)
- die(_("--bisect-clean-state requires no arguments"));
+ return error(_("--bisect-clean-state requires no
arguments"));
return bisect_clean_state();
default:
- die("BUG: unknown subcommand '%d'", cmdmode);
+ return error("BUG: unknown subcommand '%d'", cmdmode);
}
return 0;
}
diff --git a/git-bisect.sh b/git-bisect.sh
index f1202dfb4..045830c39 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -209,7 +209,7 @@ bisect_start() {
eval "$eval true" &&
if test $must_write_terms -eq 1
then
- git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD"
+ git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD" ||
exit
fi &&
echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
#
$
So, the major differences and bug fixes are in later patches.
ATB,
Ramsay Jones
Re: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
Hey Junio, On Tue, Oct 3, 2017 at 9:21 AM, Junio C Hamano wrote: > Ramsay Jones writes: > >> On 02/10/17 14:44, Pranit Bauva wrote: >> [snip] >>>... >> Yes, I also meant to tidy that up by removing some, now >> redundant, initialisation later in that function. >> >> Note, that wasn't the only bug! (I have probably forgotten >> some of them, but look at 964f4e2b0, for example). > > It seems that Pranit needs a bit more work to take known fixes from > your efforts and we should wait for the series to be rerolled? This series is the initial part of the whole series. Things started to get a little problematic after bisect_start(). The reason I am doing the series in parts so that I can get the part which is solid to be merged and then start work on later parts since I don't have a time constraint now. Regards, Pranit Bauva
Re: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
Ramsay Jones writes: > On 02/10/17 14:44, Pranit Bauva wrote: > [snip] >>... > Yes, I also meant to tidy that up by removing some, now > redundant, initialisation later in that function. > > Note, that wasn't the only bug! (I have probably forgotten > some of them, but look at 964f4e2b0, for example). It seems that Pranit needs a bit more work to take known fixes from your efforts and we should wait for the series to be rerolled? Thanks both for working on this.
Re: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
On 02/10/17 14:44, Pranit Bauva wrote: [snip] >> Look for []-ed comments in the commit messages for a note of >> the changes I made to your original patches, in patches #2, >> #4, #7-9, #11-12 and #14. >> >> The commits I added, which are just WIP, are as follows: >> >> $ git log --oneline bisect~12..bisect >> 7d7117040 (raj/bisect, bisect) bisect--helper: convert to struct object_id >> 188ea5855 bisect--helper: add the get_bad_commit() function >> b75f46fb4 bisect--helper: add a log_commit() helper function >> 4afc34403 bisect--helper: reduce the scope of a variable >> 62495f6ae bisect--helper: remove useless sub-expression in condition >> 964f4e2b0 bisect--helper: set correct term from --term-new= option >> 62efc099f bisect--helper: remove redundant assignment to has_double_dash >> d35950b92 bisect--helper: remove redundant goto's >> b33f313ac bisect--helper: remove space just before \n in string >> 3eb407156 bisect--helper: remove some unnecessary braces >> c2b89c9b8 bisect--helper: add some vertical whitespace >> 8c883701c bisect--helper: fix up some coding style issues >> $ >> >> Again IIRC, there are a couple of bug fixes in these commits ... > > There is actually a major bug in the later part of previous series > mostly in the bisect-next which actually caused delays. I think you > have fixed it in your commit 682d0bff0. Although I would need to have > a closer look at it. In original series, I did get a sigserv, and as > you mention it in the commit that you have fixed it. Yes, I also meant to tidy that up by removing some, now redundant, initialisation later in that function. Note, that wasn't the only bug! (I have probably forgotten some of them, but look at 964f4e2b0, for example). ATB, Ramsay Jones
Re: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
Hey Ramsay,
On Sat, Sep 30, 2017 at 6:29 PM, Ramsay Jones
wrote:
> Hi Pranit,
>
> Just before Junio dropped your 'pb/bisect' branch from his
> repository (and What's cooking), I fetched it locally with
> the intention of finishing it off. (It would have been silly
> to waste all your good work).
Thanks!
> Although I have rebased your branch a few times, and added
> a few commits while 'reading' the code, I haven't actually
> added much to your branch (only 12 commits and I had meant
> to squash some of those together)!
>
> However, there were some bug fixes in there, so you may want
> to take a look at:
>
> git://repo.or.cz/git/raj.git branch 'bisect'
>
> [the 'pb-bisect' branch was the original branch from Junio,
> including the 'SQUASH' commit that I squashed!]
Yes, I have checked it out. I had worked on Stephan's review and
updated a few parts. I think you have included that as well as some of
your modifications. I will squash it in together and send the series
out in parts.
> These patches seem to relate to patches 1-5 & 8 of the original
> series. The diff between these patches and the first 6 patches
> of my bisect branch is given below. Note that most of the diff
> seems to be caused by swapping patch #6 for #8, but not all of
> the hunks are caused by this.
>
> Note that I moved some code between patches (e.g. some of the
> GIT_PATH_FUNC()s moved out of patch #4, because they were not
> used in that patch. Ah, is that why you moved patch #8 up?).
> [Also I added the 'bisect clean' message to delet_refs() to
> patch #4 as well.]
Even I thought keeping GIT_PATH_FUNC()s should be declared whenever
required. Did that change in this patch series.
> Look for []-ed comments in the commit messages for a note of
> the changes I made to your original patches, in patches #2,
> #4, #7-9, #11-12 and #14.
>
> The commits I added, which are just WIP, are as follows:
>
> $ git log --oneline bisect~12..bisect
> 7d7117040 (raj/bisect, bisect) bisect--helper: convert to struct object_id
> 188ea5855 bisect--helper: add the get_bad_commit() function
> b75f46fb4 bisect--helper: add a log_commit() helper function
> 4afc34403 bisect--helper: reduce the scope of a variable
> 62495f6ae bisect--helper: remove useless sub-expression in condition
> 964f4e2b0 bisect--helper: set correct term from --term-new= option
> 62efc099f bisect--helper: remove redundant assignment to has_double_dash
> d35950b92 bisect--helper: remove redundant goto's
> b33f313ac bisect--helper: remove space just before \n in string
> 3eb407156 bisect--helper: remove some unnecessary braces
> c2b89c9b8 bisect--helper: add some vertical whitespace
> 8c883701c bisect--helper: fix up some coding style issues
> $
>
> Again IIRC, there are a couple of bug fixes in these commits ...
There is actually a major bug in the later part of previous series
mostly in the bisect-next which actually caused delays. I think you
have fixed it in your commit 682d0bff0. Although I would need to have
a closer look at it. In original series, I did get a sigserv, and as
you mention it in the commit that you have fixed it.
> I have to go now, so I will leave it with you. ;-)
>
> Hope that helps.
>
> ATB,
> Ramsay Jones
Again Thanks!
>
> -- >8 --
> diff --git a/bisect.c b/bisect.c
> index 2838d672d..b19311ca7 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -1066,7 +1066,7 @@ int bisect_clean_state(void)
> struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
> for_each_ref_in("refs/bisect", mark_for_removal, (void *)
> &refs_for_removal);
> string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
> - result = delete_refs("bisect: remove", &refs_for_removal,
> REF_NODEREF);
> + result = delete_refs("bisect: clean", &refs_for_removal, REF_NODEREF);
> refs_for_removal.strdup_strings = 1;
> string_list_clear(&refs_for_removal, 0);
> unlink_or_warn(git_path_bisect_expected_rev());
> diff --git a/builtin/am.c b/builtin/am.c
> index c973bd96d..aa66f9915 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -32,22 +32,6 @@
> #include "apply.h"
> #include "string-list.h"
>
> -/**
> - * Returns 1 if the file is empty or does not exist, 0 otherwise.
> - */
> -static int is_empty_file(const char *filename)
> -{
> - struct stat st;
> -
> - if (stat(filename, &st) < 0) {
> - if (errno == ENOENT)
> - return 1;
> - die_errno(_("could not stat %s"), filename);
> - }
> -
> - return !st.st_size;
> -}
> -
> /**
> * Returns the length of the first line of msg.
> */
> @@ -1300,7 +1284,7 @@ static int parse_mail(struct am_state *state, const
> char *mail)
> goto finish;
> }
>
> - if (is_empty_file(am_path(state, "patch"))) {
> + if (is_empty_or_missing_file(am_path(state, "patch"))) {
> printf_ln(_("Patch is empty."));
> die_user_resolve(sta
Re: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
On 29/09/17 07:49, Pranit Bauva wrote:
> `--next-all` is meant to be used as a subcommand to support multiple
> "operation mode" though the current implementation does not contain any
> other subcommand along side with `--next-all` but further commits will
> include some more subcommands.
>
> Helped-by: Johannes Schindelin
> Mentored-by: Lars Schneider
> Mentored-by: Christian Couder
> Signed-off-by: Pranit Bauva
>
> ---
> Hey,
>
> It has been a long time since this series appeared on the mailing list.
> The previous version v15[1] is now split into many parts and I am
> sending the first part right now, will focus on getting this merged and
> then send out the next part.
>
> The changes in this part:
> * Stephan pointed out that "terms" was missing in patch 2 ie.
>"bisect--helper: rewrite `check_term_format` shell function in C"
>
> [1]:
> https://public-inbox.org/git/CAFZEwPOjK25m84BgTF7AL72DL_K1dHf7OrYoX=_vky9r3ga...@mail.gmail.com/
Hi Pranit,
Just before Junio dropped your 'pb/bisect' branch from his
repository (and What's cooking), I fetched it locally with
the intention of finishing it off. (It would have been silly
to waste all your good work).
Although I have rebased your branch a few times, and added
a few commits while 'reading' the code, I haven't actually
added much to your branch (only 12 commits and I had meant
to squash some of those together)!
However, there were some bug fixes in there, so you may want
to take a look at:
git://repo.or.cz/git/raj.git branch 'bisect'
[the 'pb-bisect' branch was the original branch from Junio,
including the 'SQUASH' commit that I squashed!]
These patches seem to relate to patches 1-5 & 8 of the original
series. The diff between these patches and the first 6 patches
of my bisect branch is given below. Note that most of the diff
seems to be caused by swapping patch #6 for #8, but not all of
the hunks are caused by this.
Note that I moved some code between patches (e.g. some of the
GIT_PATH_FUNC()s moved out of patch #4, because they were not
used in that patch. Ah, is that why you moved patch #8 up?).
[Also I added the 'bisect clean' message to delet_refs() to
patch #4 as well.]
Look for []-ed comments in the commit messages for a note of
the changes I made to your original patches, in patches #2,
#4, #7-9, #11-12 and #14.
The commits I added, which are just WIP, are as follows:
$ git log --oneline bisect~12..bisect
7d7117040 (raj/bisect, bisect) bisect--helper: convert to struct object_id
188ea5855 bisect--helper: add the get_bad_commit() function
b75f46fb4 bisect--helper: add a log_commit() helper function
4afc34403 bisect--helper: reduce the scope of a variable
62495f6ae bisect--helper: remove useless sub-expression in condition
964f4e2b0 bisect--helper: set correct term from --term-new= option
62efc099f bisect--helper: remove redundant assignment to has_double_dash
d35950b92 bisect--helper: remove redundant goto's
b33f313ac bisect--helper: remove space just before \n in string
3eb407156 bisect--helper: remove some unnecessary braces
c2b89c9b8 bisect--helper: add some vertical whitespace
8c883701c bisect--helper: fix up some coding style issues
$
Again IIRC, there are a couple of bug fixes in these commits ...
I have to go now, so I will leave it with you. ;-)
Hope that helps.
ATB,
Ramsay Jones
-- >8 --
diff --git a/bisect.c b/bisect.c
index 2838d672d..b19311ca7 100644
--- a/bisect.c
+++ b/bisect.c
@@ -1066,7 +1066,7 @@ int bisect_clean_state(void)
struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
for_each_ref_in("refs/bisect", mark_for_removal, (void *)
&refs_for_removal);
string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
- result = delete_refs("bisect: remove", &refs_for_removal, REF_NODEREF);
+ result = delete_refs("bisect: clean", &refs_for_removal, REF_NODEREF);
refs_for_removal.strdup_strings = 1;
string_list_clear(&refs_for_removal, 0);
unlink_or_warn(git_path_bisect_expected_rev());
diff --git a/builtin/am.c b/builtin/am.c
index c973bd96d..aa66f9915 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -32,22 +32,6 @@
#include "apply.h"
#include "string-list.h"
-/**
- * Returns 1 if the file is empty or does not exist, 0 otherwise.
- */
-static int is_empty_file(const char *filename)
-{
- struct stat st;
-
- if (stat(filename, &st) < 0) {
- if (errno == ENOENT)
- return 1;
- die_errno(_("could not stat %s"), filename);
- }
-
- return !st.st_size;
-}
-
/**
* Returns the length of the first line of msg.
*/
@@ -1300,7 +1284,7 @@ static int parse_mail(struct am_state *state, const char
*mail)
goto finish;
}
- if (is_empty_file(am_path(state, "patch"))) {
+ if (is_empty_or_missing_file(am_path(state, "patch"))) {
printf_ln(_("Patch is empty."));
die_user_resolv
Re: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
Hey Stephan, On Sat, Sep 30, 2017 at 12:24 AM, Stephan Beyer wrote: > > Hi Pranit, > > On 09/29/2017 08:49 AM, Pranit Bauva wrote: > > It has been a long time since this series appeared on the mailing list. > > The previous version v15[1] is now split into many parts and I am > > sending the first part right now, will focus on getting this merged and > > then send out the next part. > > That's a good idea! > > I just reviewed your series by assuming I did the v15 review well (it > took quite some time at least)... so I just diff'ed the v15 and the v16 > patches. I am totally fine with it! Thanks for having the patience of waiting for it and reviewing it again. Regards, Pranit Bauva www.bauva.com
Re: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
Hi Pranit, On 09/29/2017 08:49 AM, Pranit Bauva wrote: > It has been a long time since this series appeared on the mailing list. > The previous version v15[1] is now split into many parts and I am > sending the first part right now, will focus on getting this merged and > then send out the next part. That's a good idea! I just reviewed your series by assuming I did the v15 review well (it took quite some time at least)... so I just diff'ed the v15 and the v16 patches. I am totally fine with it! Thanks, Stephan
