[PATCH v2 2/7] bisect: replace hardcoded bad|good by variables

2015-06-22 Thread Antoine Delaite
Matthieu Moy matthieu@grenoble-inp.fr wrote: 

Being an acceptable ref name is a constraint you have to check (Junio 
already mentionned check-ref-format). I think quoting variables makes 
sense too 

I don't get how 'git check-ref-format' works exactly. It says it needs 
at least one slash in the ref name. So it would not be good for bisect 
terms.
Then do we take the same format as branches ? 
( i.e 'git check-ref-format --branch' )

Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in


Re: [PATCH v2 2/7] bisect: replace hardcoded bad|good by variables

2015-06-22 Thread Matthieu Moy
Antoine Delaite antoine.dela...@ensimag.grenoble-inp.fr writes:

 Matthieu Moy matthieu@grenoble-inp.fr wrote: 

Being an acceptable ref name is a constraint you have to check (Junio 
already mentionned check-ref-format). I think quoting variables makes 
sense too 

 I don't get how 'git check-ref-format' works exactly. It says it needs 
 at least one slash in the ref name. So it would not be good for bisect 
 terms.

At some point, refs/bisect/$good and refs/bisect/$bad-$sha1 will become
refs, so you should check refs/bisect/$good and refs/bisect/$bad with
check-ref-format (I think the -$sha1 suffix will be accepted by
construction).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line unsubscribe git in


Re: [PATCH v2 2/7] bisect: replace hardcoded bad|good by variables

2015-06-11 Thread Matthieu Moy
Antoine Delaite antoine.dela...@ensimag.grenoble-inp.fr writes:

 - if (!strcmp(refname, bad)) {
 + char good_prefix[256];
 + strcpy(good_prefix, name_good);
 + strcat(good_prefix, -);

You are silently adding a restriction here: name_good must be small
enough to fit in a 256-bytes array. It's not a terrible restriction, but
what may happen if you break it is a real issue.

Either you have to enforce this restriction somewhere, or you should not
have the restriction at all. I'd vote for the second. strbuf is your
friend here.

 @@ -259,21 +264,21 @@ bisect_state() {
  
  bisect_next_check() {
   missing_good= missing_bad=
 - git show-ref -q --verify refs/bisect/bad || missing_bad=t
 - test -n $(git for-each-ref refs/bisect/good-*) || missing_good=t
 + git show-ref -q --verify refs/bisect/$NAME_BAD || missing_bad=t
 + test -n $(git for-each-ref refs/bisect/$NAME_GOOD-*) || 
 missing_good=t

There are other restrictions here: $NAME_BAD must be an acceptable ref
name, and you're not quoting $NAME_BAD hence it must not contain shell
meta-characters (The requirements for ref names almost imply that, but
'foo/bar{a,b}' is accepted and will trigger some expansion if your
/bin/sh is bash for example).

Being an acceptable ref name is a constraint you have to check (Junio
already mentionned check-ref-format). I think quoting variables makes
sense too.

 @@ -421,7 +426,7 @@ bisect_replay () {
   start)
   cmd=bisect_start $rev
   eval $cmd ;;
 - good|bad|skip)
 + $NAME_GOOD|$NAME_BAD|skip)

$NAME_GOOD and $NAME_BAD need quoting if you're not sure they don't
contain shell metacharacters.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v2 2/7] bisect: replace hardcoded bad|good by variables

2015-06-10 Thread Antoine Delaite
To add new tags like old/new and have keywords less confusing, the
first step is to avoid hardcoding the keywords.

The default mode is still bad/good.

Signed-off-by: Antoine Delaite antoine.dela...@ensimag.grenoble-inp.fr
Signed-off-by: Louis Stuber stub...@ensimag.grenoble-inp.fr
Signed-off-by: Valentin Duperray valentin.duper...@ensimag.imag.fr
Signed-off-by: Franck Jonas franck.jo...@ensimag.imag.fr
Signed-off-by: Lucien Kong lucien.k...@ensimag.imag.fr
Signed-off-by: Thomas Nguy thomas.n...@ensimag.imag.fr
Signed-off-by: Huynh Khoi Nguyen Nguyen 
huynh-khoi-nguyen.ngu...@ensimag.imag.fr
Signed-off-by: Matthieu Moy matthieu@grenoble-inp.fr
---
 bisect.c  |   49 -
 git-bisect.sh |   57 +++--
 2 files changed, 63 insertions(+), 43 deletions(-)
 mode change 100755 = 100644 git-bisect.sh

diff --git a/bisect.c b/bisect.c
index de92953..eb2f555 100644
--- a/bisect.c
+++ b/bisect.c
@@ -21,6 +21,9 @@ static const char *argv_checkout[] = {checkout, -q, NULL, 
--, NULL};
 static const char *argv_show_branch[] = {show-branch, NULL, NULL};
 static const char *argv_update_ref[] = {update-ref, --no-deref, 
BISECT_HEAD, NULL, NULL};
 
+static const char *name_bad;
+static const char *name_good;
+
 /* Remember to update object flag allocation in object.h */
 #define COUNTED(1u16)
 
@@ -403,10 +406,14 @@ struct commit_list *find_bisection(struct commit_list 
*list,
 static int register_ref(const char *refname, const unsigned char *sha1,
int flags, void *cb_data)
 {
-   if (!strcmp(refname, bad)) {
+   char good_prefix[256];
+   strcpy(good_prefix, name_good);
+   strcat(good_prefix, -);
+
+   if (!strcmp(refname, name_bad)) {
current_bad_oid = xmalloc(sizeof(*current_bad_oid));
hashcpy(current_bad_oid-hash, sha1);
-   } else if (starts_with(refname, good-)) {
+   } else if (starts_with(refname, good_prefix)) {
sha1_array_append(good_revs, sha1);
} else if (starts_with(refname, skip-)) {
sha1_array_append(skipped_revs, sha1);
@@ -634,7 +641,7 @@ static void exit_if_skipped_commits(struct commit_list 
*tried,
return;
 
printf(There are only 'skip'ped commits left to test.\n
-  The first bad commit could be any of:\n);
+  The first %s commit could be any of:\n, name_bad);
print_commit_list(tried, %s\n, %s\n);
if (bad)
printf(%s\n, oid_to_hex(bad));
@@ -732,18 +739,21 @@ static void handle_bad_merge_base(void)
if (is_expected_rev(current_bad_oid)) {
char *bad_hex = oid_to_hex(current_bad_oid);
char *good_hex = join_sha1_array_hex(good_revs, ' ');
-
-   fprintf(stderr, The merge base %s is bad.\n
-   This means the bug has been fixed 
-   between %s and [%s].\n,
-   bad_hex, bad_hex, good_hex);
-
+   if (!strcmp(name_bad, bad)) {
+   fprintf(stderr, The merge base %s is bad.\n
+   This means the bug has been fixed 
+   between %s and [%s].\n,
+   bad_hex, bad_hex, good_hex);
+   } else {
+   die(BUG: terms %s/%s not managed, name_bad, 
name_good);
+   }
exit(3);
}
 
-   fprintf(stderr, Some good revs are not ancestor of the bad rev.\n
+   fprintf(stderr, Some %s revs are not ancestor of the %s rev.\n
git bisect cannot work properly in this case.\n
-   Maybe you mistook good and bad revs?\n);
+   Maybe you mistook %s and %s revs?\n,
+   name_good, name_bad, name_good, name_bad);
exit(1);
 }
 
@@ -755,10 +765,10 @@ static void handle_skipped_merge_base(const unsigned char 
*mb)
 
warning(the merge base between %s and [%s] 
must be skipped.\n
-   So we cannot be sure the first bad commit is 
+   So we cannot be sure the first %s commit is 
between %s and %s.\n
We continue anyway.,
-   bad_hex, good_hex, mb_hex, bad_hex);
+   bad_hex, good_hex, name_bad, mb_hex, bad_hex);
free(good_hex);
 }
 
@@ -839,7 +849,7 @@ static void check_good_are_ancestors_of_bad(const char 
*prefix, int no_checkout)
int fd;
 
if (!current_bad_oid)
-   die(a bad revision is needed);
+   die(a %s revision is needed, name_bad);
 
/* Check if file BISECT_ANCESTORS_OK exists. */
if (!stat(filename, st)  S_ISREG(st.st_mode))
@@ -905,6 +915,8 @@ int bisect_next_all(const char *prefix, int no_checkout)
const unsigned char *bisect_rev;
char bisect_rev_hex[GIT_SHA1_HEXSZ + 1];
 
+