[PATCH v4 00/12] ref-filter: use parsing functions

[Karthik Nayak]

This series cleans up populate_value() in ref-filter, by moving out
the parsing part of atoms to separate parsing functions. This ensures
that parsing is only done once and also improves the modularity of the
code.

v1: http://thread.gmane.org/gmane.comp.version-control.git/281180
v2: http://thread.gmane.org/gmane.comp.version-control.git/282563
v3: http://thread.gmane.org/gmane.comp.version-control.git/283350

Changes:
* The parsing functions now take the arguments of the atom as
function parameteres, instead of parsing it inside the fucntion.
* Rebased on top of pu:jk/list-tag-2.7-regression
* In strbuf use a copylen variable rather than using multiplication
to perform a logical operation.
* Code movement for easier review and general improvement.
* Use COLOR_MAXLEN as the maximum size for the color variable.
* Small code changes.
* Documentation changes.
* Fixed incorrect style of test (t6302).

Karthik Nayak (12):
  strbuf: introduce strbuf_split_str_omit_term()
  ref-filter: use strbuf_split_str_omit_term()
  ref-filter: bump 'used_atom' and related code to the top
  ref-filter: introduce struct used_atom
  ref-filter: introduce parsing functions for each valid atom
  ref-filter: introduce color_atom_parser()
  ref-filter: introduce parse_align_position()
  ref-filter: introduce align_atom_parser()
  ref-filter: align: introduce long-form syntax
  ref-filter: introduce remote_ref_atom_parser()
  ref-filter: introduce contents_atom_parser()
  ref-filter: introduce objectname_atom_parser()

Interdiff:

diff --git a/ref-filter.c b/ref-filter.c
index 7a634e6..d48e2a3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -35,7 +35,7 @@ static struct used_atom {
const char *name;
cmp_type type;
union {
-   char *color;
+   char color[COLOR_MAXLEN];
struct align align;
enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
remote_ref;
@@ -49,99 +49,68 @@ static struct used_atom {
 static int used_atom_cnt, need_tagged, need_symref;
 static int need_color_reset_at_eol;
 
-static int match_atom_name(const char *name, const char *atom_name, const char 
**val)
+static void color_atom_parser(struct used_atom *atom, const char *color_value)
 {
-   const char *body;
-
-   /* skip the deref specifier */
-   if (name[0] == '*')
-   name++;
-
-   if (!skip_prefix(name, atom_name, ))
-   return 0; /* doesn't even begin with "atom_name" */
-   if (!body[0]) {
-   *val = NULL; /* %(atom_name) and no customization */
-   return 1;
-   }
-   if (body[0] != ':')
-   return 0; /* "atom_namefoo" is not "atom_name" or 
"atom_name:..." */
-   *val = body + 1; /* "atom_name:val" */
-   return 1;
-}
-
-static void color_atom_parser(struct used_atom *atom)
-{
-   if (!match_atom_name(atom->name, "color", (const char 
**)>u.color))
-   die("BUG: parsing non-'color'");
-   if (!atom->u.color)
+   if (!color_value)
die(_("expected format: %%(color:)"));
-   /* atom->u.color points to part of atom->name */
-   atom->u.color = xstrdup(atom->u.color);
-   if (color_parse(atom->u.color, atom->u.color) < 0)
+   if (color_parse(color_value, atom->u.color) < 0)
die(_("invalid color value: %s"), atom->u.color);
 }
 
-static void remote_ref_atom_parser(struct used_atom *atom)
+static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
 {
-   const char *buf;
-
-   buf = strchr(atom->name, ':');
-   if (!buf) {
+   if (!arg) {
atom->u.remote_ref = RR_NORMAL;
-   return;
-   }
-   buf++;
-   if (!strcmp(buf, "short"))
+   } else if (!strcmp(arg, "short"))
atom->u.remote_ref = RR_SHORTEN;
-   else if (!strcmp(buf, "track"))
+   else if (!strcmp(arg, "track"))
atom->u.remote_ref = RR_TRACK;
-   else if (!strcmp(buf, "trackshort"))
+   else if (!strcmp(arg, "trackshort"))
atom->u.remote_ref = RR_TRACKSHORT;
else
die(_("unrecognized format: %%(%s)"), atom->name);
 }
 
-static void contents_atom_parser(struct used_atom *atom)
+static void body_atom_parser(struct used_atom *atom, const char *arg)
 {
-   const char * buf;
+   if (arg)
+   die("%%(body) atom does not take arguments");
+   atom->u.contents.option = C_BODY_DEP;
+}
 
-   if (match_atom_name(atom->name, "subject", )) {
-   atom->u.contents.option = C_SUB;
-   return;
-   } else if (match_atom_name(atom->name, "body", )) {
-   atom->u.contents.option = C_BODY_DEP;
-   return;
-   } if (!match_atom_name(atom->name, "contents", ))
- die("BUG: parsing non-'contents'");
+static void subject_atom_parser(struct used_atom *atom, const char *arg)
+{
+   if (arg)
+ 

[PATCH v4 01/12] strbuf: introduce strbuf_split_str_omit_term()

[Karthik Nayak]

The current implementation of 'strbuf_split_buf()' includes the
terminator at the end of each strbuf post splitting. Add an option
wherein we can drop the terminator if desired. In this context
introduce a wrapper function 'strbuf_split_str_omit_term()' which
splits a given string into strbufs without including the terminator.

Helped-by: Eric Sunshine 
Signed-off-by: Karthik Nayak 
---
 strbuf.c | 14 +-
 strbuf.h | 25 -
 2 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index bab316d..4a93e2a 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -115,7 +115,7 @@ void strbuf_tolower(struct strbuf *sb)
 }
 
 struct strbuf **strbuf_split_buf(const char *str, size_t slen,
-int terminator, int max)
+int terminator, int max, int omit_term)
 {
struct strbuf **ret = NULL;
size_t nr = 0, alloc = 0;
@@ -123,14 +123,18 @@ struct strbuf **strbuf_split_buf(const char *str, size_t 
slen,
 
while (slen) {
int len = slen;
+   int copylen = len;
+   const char *end = NULL;
if (max <= 0 || nr + 1 < max) {
-   const char *end = memchr(str, terminator, slen);
-   if (end)
+   end = memchr(str, terminator, slen);
+   if (end) {
len = end - str + 1;
+   copylen = len - !!omit_term;
+   }
}
t = xmalloc(sizeof(struct strbuf));
-   strbuf_init(t, len);
-   strbuf_add(t, str, len);
+   strbuf_init(t, copylen);
+   strbuf_add(t, str, copylen);
ALLOC_GROW(ret, nr + 2, alloc);
ret[nr++] = t;
str += len;
diff --git a/strbuf.h b/strbuf.h
index f72fd14..6115e72 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -466,11 +466,12 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, 
const char *suffix)
 /**
  * Split str (of length slen) at the specified terminator character.
  * Return a null-terminated array of pointers to strbuf objects
- * holding the substrings.  The substrings include the terminator,
- * except for the last substring, which might be unterminated if the
- * original string did not end with a terminator.  If max is positive,
- * then split the string into at most max substrings (with the last
- * substring containing everything following the (max-1)th terminator
+ * holding the substrings.  If omit_term is true, the terminator will
+ * be stripped from all substrings. Otherwise, substrings will include
+ * the terminator, except for the final substring, if the original
+ * string lacked a terminator.  If max is positive, then split the
+ * string into at most max substrings (with the last substring
+ * containing everything following the (max-1)th terminator
  * character).
  *
  * The most generic form is `strbuf_split_buf`, which takes an arbitrary
@@ -481,19 +482,25 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, 
const char *suffix)
  * For lighter-weight alternatives, see string_list_split() and
  * string_list_split_in_place().
  */
-extern struct strbuf **strbuf_split_buf(const char *, size_t,
-   int terminator, int max);
+extern struct strbuf **strbuf_split_buf(const char *str, size_t slen,
+   int terminator, int max, int omit_term);
+
+static inline struct strbuf **strbuf_split_str_omit_term(const char *str,
+   int terminator, int 
max)
+{
+   return strbuf_split_buf(str, strlen(str), terminator, max, 1);
+}
 
 static inline struct strbuf **strbuf_split_str(const char *str,
   int terminator, int max)
 {
-   return strbuf_split_buf(str, strlen(str), terminator, max);
+   return strbuf_split_buf(str, strlen(str), terminator, max, 0);
 }
 
 static inline struct strbuf **strbuf_split_max(const struct strbuf *sb,
int terminator, int max)
 {
-   return strbuf_split_buf(sb->buf, sb->len, terminator, max);
+   return strbuf_split_buf(sb->buf, sb->len, terminator, max, 0);
 }
 
 static inline struct strbuf **strbuf_split(const struct strbuf *sb,
-- 
2.7.0

--
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 v4 09/12] ref-filter: align: introduce long-form syntax

[Karthik Nayak]

Introduce optional prefixes "width=" and "position=" for the align atom
so that the atom can be used as "%(align:width=,position=)".

Add Documentation and tests for the same.

Helped-by: Eric Sunshine 
Signed-off-by: Karthik Nayak 
---
 Documentation/git-for-each-ref.txt | 20 ++
 ref-filter.c   | 10 -
 t/t6302-for-each-ref-filter.sh | 42 ++
 3 files changed, 63 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 2e3e96f..012e8f9 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -133,14 +133,18 @@ color::
 
 align::
Left-, middle-, or right-align the content between
-   %(align:...) and %(end). The "align:" is followed by ``
-   and `` in any order separated by a comma, where the
-   `` is either left, right or middle, default being
-   left and `` is the total length of the content with
-   alignment. If the contents length is more than the width then
-   no alignment is performed. If used with '--quote' everything
-   in between %(align:...) and %(end) is quoted, but if nested
-   then only the topmost level performs quoting.
+   %(align:...) and %(end). The "align:" is followed by
+   `width=` and `position=` in any order
+   separated by a comma, where the `` is either left,
+   right or middle, default being left and `` is the total
+   length of the content with alignment. For brevity, the
+   "width=" and/or "position=" prefixes may be omitted, and bare
+and  used instead.  For instance,
+   `%(align:,)`. If the contents length is more
+   than the width then no alignment is performed. If used with
+   '--quote' everything in between %(align:...) and %(end) is
+   quoted, but if nested then only the topmost level performs
+   quoting.
 
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
diff --git a/ref-filter.c b/ref-filter.c
index 79a7e07..58d433f 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -77,7 +77,15 @@ static void align_atom_parser(struct used_atom *atom, const 
char *arg)
int position;
arg = s[0]->buf;
 
-   if (!strtoul_ui(arg, 10, ))
+   if (skip_prefix(arg, "position=", )) {
+   position = parse_align_position(arg);
+   if (position < 0)
+   die(_("unrecognized position:%s"), arg);
+   align->position = position;
+   } else if (skip_prefix(arg, "width=", )) {
+   if (strtoul_ui(arg, 10, ))
+   die(_("unrecognized width:%s"), arg);
+   } else if (!strtoul_ui(arg, 10, ))
;
else if ((position = parse_align_position(arg)) >= 0)
align->position = position;
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index fe4796c..f79355d 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -133,6 +133,48 @@ test_expect_success 'right alignment' '
test_cmp expect actual
 '
 
+cat >expect <<-\EOF
+|   refname is refs/heads/master   |refs/heads/master
+|refname is refs/heads/side|refs/heads/side
+| refname is refs/odd/spot |refs/odd/spot
+| refname is refs/tags/double-tag  |refs/tags/double-tag
+|refname is refs/tags/four |refs/tags/four
+| refname is refs/tags/one |refs/tags/one
+| refname is refs/tags/signed-tag  |refs/tags/signed-tag
+|refname is refs/tags/three|refs/tags/three
+| refname is refs/tags/two |refs/tags/two
+EOF
+
+test_align_permutations() {
+   while read -r option
+   do
+   test_expect_success "align:$option" '
+   git for-each-ref --format="|%(align:$option)refname is 
%(refname)%(end)|%(refname)" >actual &&
+   test_cmp expect actual
+   '
+   done
+}
+
+test_align_permutations <<-\EOF
+   middle,42
+   42,middle
+   position=middle,42
+   42,position=middle
+   middle,width=42
+   width=42,middle
+   position=middle,width=42
+   width=42,position=middle
+EOF
+
+# Last one wins (silently) when multiple arguments of the same type are given
+
+test_align_permutations <<-\EOF
+   32,width=42,middle
+   width=30,42,middle
+   width=42,position=right,middle
+   42,right,position=middle
+EOF
+
 # Individual atoms inside %(align:...) and %(end) must not be quoted.
 
 test_expect_success 'alignment with format quote' "
-- 
2.7.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a 

[PATCH v4 04/12] ref-filter: introduce struct used_atom

[Karthik Nayak]

Introduce the 'used_atom' structure to replace the existing
implementation of 'used_atom' (which is a list of atoms). This helps
us parse atoms beforehand and store required details into the
'used_atom' for future usage.

Helped-by: Eric Sunshine 
Signed-off-by: Karthik Nayak 
---
 ref-filter.c | 35 ++-
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index c3a8372..3736dc3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -26,8 +26,10 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
  * indexed with the "atom number", which is an index into this
  * array.
  */
-static const char **used_atom;
-static cmp_type *used_atom_type;
+static struct used_atom {
+   const char *name;
+   cmp_type type;
+} *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
 static int need_color_reset_at_eol;
 
@@ -122,8 +124,8 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
 
/* Do we have the atom already used elsewhere? */
for (i = 0; i < used_atom_cnt; i++) {
-   int len = strlen(used_atom[i]);
-   if (len == ep - atom && !memcmp(used_atom[i], atom, len))
+   int len = strlen(used_atom[i].name);
+   if (len == ep - atom && !memcmp(used_atom[i].name, atom, len))
return i;
}
 
@@ -150,12 +152,11 @@ int parse_ref_filter_atom(const char *atom, const char 
*ep)
at = used_atom_cnt;
used_atom_cnt++;
REALLOC_ARRAY(used_atom, used_atom_cnt);
-   REALLOC_ARRAY(used_atom_type, used_atom_cnt);
-   used_atom[at] = xmemdupz(atom, ep - atom);
-   used_atom_type[at] = valid_atom[i].cmp_type;
+   used_atom[at].name = xmemdupz(atom, ep - atom);
+   used_atom[at].type = valid_atom[i].cmp_type;
if (*atom == '*')
need_tagged = 1;
-   if (!strcmp(used_atom[at], "symref"))
+   if (!strcmp(used_atom[at].name, "symref"))
need_symref = 1;
return at;
 }
@@ -315,7 +316,7 @@ int verify_ref_format(const char *format)
at = parse_ref_filter_atom(sp + 2, ep);
cp = ep + 1;
 
-   if (skip_prefix(used_atom[at], "color:", ))
+   if (skip_prefix(used_atom[at].name, "color:", ))
need_color_reset_at_eol = !!strcmp(color, "reset");
}
return 0;
@@ -359,7 +360,7 @@ static void grab_common_values(struct atom_value *val, int 
deref, struct object
int i;
 
for (i = 0; i < used_atom_cnt; i++) {
-   const char *name = used_atom[i];
+   const char *name = used_atom[i].name;
struct atom_value *v = [i];
if (!!deref != (*name == '*'))
continue;
@@ -383,7 +384,7 @@ static void grab_tag_values(struct atom_value *val, int 
deref, struct object *ob
struct tag *tag = (struct tag *) obj;
 
for (i = 0; i < used_atom_cnt; i++) {
-   const char *name = used_atom[i];
+   const char *name = used_atom[i].name;
struct atom_value *v = [i];
if (!!deref != (*name == '*'))
continue;
@@ -405,7 +406,7 @@ static void grab_commit_values(struct atom_value *val, int 
deref, struct object
struct commit *commit = (struct commit *) obj;
 
for (i = 0; i < used_atom_cnt; i++) {
-   const char *name = used_atom[i];
+   const char *name = used_atom[i].name;
struct atom_value *v = [i];
if (!!deref != (*name == '*'))
continue;
@@ -535,7 +536,7 @@ static void grab_person(const char *who, struct atom_value 
*val, int deref, stru
const char *wholine = NULL;
 
for (i = 0; i < used_atom_cnt; i++) {
-   const char *name = used_atom[i];
+   const char *name = used_atom[i].name;
struct atom_value *v = [i];
if (!!deref != (*name == '*'))
continue;
@@ -573,7 +574,7 @@ static void grab_person(const char *who, struct atom_value 
*val, int deref, stru
if (!wholine)
return;
for (i = 0; i < used_atom_cnt; i++) {
-   const char *name = used_atom[i];
+   const char *name = used_atom[i].name;
struct atom_value *v = [i];
if (!!deref != (*name == '*'))
continue;
@@ -663,7 +664,7 @@ static void grab_sub_body_contents(struct atom_value *val, 
int deref, struct obj
unsigned long sublen = 0, bodylen = 0, nonsiglen = 0, siglen = 0;
 
for (i = 0; i < used_atom_cnt; i++) {
-   const char *name = used_atom[i];
+   const char *name = used_atom[i].name;
struct atom_value *v = [i];
const char *valp = NULL;
if 

[PATCH v4 08/12] ref-filter: introduce align_atom_parser()

[Karthik Nayak]

Introduce align_atom_parser() which will parse an 'align' atom and
store the required alignment position and width in the 'used_atom'
structure for further usage in populate_value().

Since this patch removes the last usage of match_atom_name(), remove
the function from ref-filter.c.

Helped-by: Eric Sunshine 
Helped-by: Ramsay Jones 
Signed-off-by: Karthik Nayak 
---
 ref-filter.c | 91 ++--
 1 file changed, 40 insertions(+), 51 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index e6b6b55..79a7e07 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -16,6 +16,11 @@
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 
+struct align {
+   align_type position;
+   unsigned int width;
+};
+
 /*
  * An atom is a valid field atom listed below, possibly prefixed with
  * a "*" to denote deref_tag().
@@ -31,6 +36,7 @@ static struct used_atom {
cmp_type type;
union {
char color[COLOR_MAXLEN];
+   struct align align;
} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
@@ -55,6 +61,37 @@ static align_type parse_align_position(const char *s)
return -1;
 }
 
+static void align_atom_parser(struct used_atom *atom, const char *arg)
+{
+   struct align *align = >u.align;
+   struct strbuf **s, **to_free;
+   unsigned int width = ~0U;
+
+   if (!arg)
+   die(_("expected format: %%(align:,)"));
+   s = to_free = strbuf_split_str_omit_term(arg, ',', 0);
+
+   align->position = ALIGN_LEFT;
+
+   while (*s) {
+   int position;
+   arg = s[0]->buf;
+
+   if (!strtoul_ui(arg, 10, ))
+   ;
+   else if ((position = parse_align_position(arg)) >= 0)
+   align->position = position;
+   else
+   die(_("unrecognized %%(align) argument: %s"), arg);
+   s++;
+   }
+
+   if (width == ~0U)
+   die(_("positive width expected with the %%(align) atom"));
+   align->width = width;
+   strbuf_list_free(to_free);
+}
+
 static struct {
const char *name;
cmp_type cmp_type;
@@ -93,17 +130,12 @@ static struct {
{ "flag" },
{ "HEAD" },
{ "color", FIELD_STR, color_atom_parser },
-   { "align" },
+   { "align", FIELD_STR, align_atom_parser },
{ "end" },
 };
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
 
-struct align {
-   align_type position;
-   unsigned int width;
-};
-
 struct contents {
unsigned int lines;
struct object_id oid;
@@ -287,22 +319,6 @@ static void end_atom_handler(struct atom_value *atomv, 
struct ref_formatting_sta
pop_stack_element(>stack);
 }
 
-static int match_atom_name(const char *name, const char *atom_name, const char 
**val)
-{
-   const char *body;
-
-   if (!skip_prefix(name, atom_name, ))
-   return 0; /* doesn't even begin with "atom_name" */
-   if (!body[0]) {
-   *val = NULL; /* %(atom_name) and no customization */
-   return 1;
-   }
-   if (body[0] != ':')
-   return 0; /* "atom_namefoo" is not "atom_name" or 
"atom_name:..." */
-   *val = body + 1; /* "atom_name:val" */
-   return 1;
-}
-
 /*
  * In a format string, find the next occurrence of %(atom).
  */
@@ -844,7 +860,6 @@ static void populate_value(struct ref_array_item *ref)
int deref = 0;
const char *refname;
const char *formatp;
-   const char *valp;
struct branch *branch = NULL;
 
v->handler = append_atom;
@@ -908,34 +923,8 @@ static void populate_value(struct ref_array_item *ref)
else
v->s = " ";
continue;
-   } else if (match_atom_name(name, "align", )) {
-   struct align *align = >u.align;
-   struct strbuf **s, **to_free;
-   int width = -1;
-
-   if (!valp)
-   die(_("expected format: 
%%(align:,)"));
-
-   s = to_free = strbuf_split_str_omit_term(valp, ',', 0);
-
-   align->position = ALIGN_LEFT;
-
-   while (*s) {
-   int position;
-
-   if (!strtoul_ui(s[0]->buf, 10, (unsigned int 
*)))
-   ;
-   else if ((position = 
parse_align_position(s[0]->buf)) >= 0)
-   align->position = position;
-   else
-   die(_("improper format entered 
align:%s"), s[0]->buf);
-   s++;

[PATCH v4 06/12] ref-filter: introduce color_atom_parser()

[Karthik Nayak]

Introduce color_atom_parser() which will parse a "color" atom and
store its color in the "used_atom" structure for further usage in
populate_value().

Helped-by: Ramsay Jones 
Helped-by: Eric Sunshine 
Signed-off-by: Karthik Nayak 
---
 ref-filter.c | 25 -
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 92b4419..7adff67 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -29,10 +29,21 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } 
cmp_type;
 static struct used_atom {
const char *name;
cmp_type type;
+   union {
+   char color[COLOR_MAXLEN];
+   } u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
 static int need_color_reset_at_eol;
 
+static void color_atom_parser(struct used_atom *atom, const char *color_value)
+{
+   if (!color_value)
+   die(_("expected format: %%(color:)"));
+   if (color_parse(color_value, atom->u.color) < 0)
+   die(_("invalid color value: %s"), atom->u.color);
+}
+
 static struct {
const char *name;
cmp_type cmp_type;
@@ -70,7 +81,7 @@ static struct {
{ "symref" },
{ "flag" },
{ "HEAD" },
-   { "color" },
+   { "color", FIELD_STR, color_atom_parser },
{ "align" },
{ "end" },
 };
@@ -157,6 +168,7 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
used_atom[at].type = valid_atom[i].cmp_type;
if (arg)
arg = used_atom[at].name + (arg - atom) + 1;
+   memset(_atom[at].u, 0, sizeof(used_atom[at].u));
if (valid_atom[i].parser)
valid_atom[i].parser(_atom[at], arg);
if (*atom == '*')
@@ -815,6 +827,7 @@ static void populate_value(struct ref_array_item *ref)
 
/* Fill in specials first */
for (i = 0; i < used_atom_cnt; i++) {
+   struct used_atom *atom = _atom[i];
const char *name = used_atom[i].name;
struct atom_value *v = >value[i];
int deref = 0;
@@ -855,14 +868,8 @@ static void populate_value(struct ref_array_item *ref)
refname = branch_get_push(branch, NULL);
if (!refname)
continue;
-   } else if (match_atom_name(name, "color", )) {
-   char color[COLOR_MAXLEN] = "";
-
-   if (!valp)
-   die(_("expected format: %%(color:)"));
-   if (color_parse(valp, color) < 0)
-   die(_("unable to parse format"));
-   v->s = xstrdup(color);
+   } else if (starts_with(name, "color:")) {
+   v->s = atom->u.color;
continue;
} else if (!strcmp(name, "flag")) {
char buf[256], *cp = buf;
-- 
2.7.0

--
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 v4 07/12] ref-filter: introduce parse_align_position()

[Karthik Nayak]

>From populate_value() extract parse_align_position() which given a
string would give us the alignment position. This is a preparatory
patch as to introduce prefixes for the %(align) atom and avoid
redundancy in the code.

Helped-by: Eric Sunshine 
Signed-off-by: Karthik Nayak 
---
 ref-filter.c | 21 +++--
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 7adff67..e6b6b55 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -44,6 +44,17 @@ static void color_atom_parser(struct used_atom *atom, const 
char *color_value)
die(_("invalid color value: %s"), atom->u.color);
 }
 
+static align_type parse_align_position(const char *s)
+{
+   if (!strcmp(s, "right"))
+   return ALIGN_RIGHT;
+   else if (!strcmp(s, "middle"))
+   return ALIGN_MIDDLE;
+   else if (!strcmp(s, "left"))
+   return ALIGN_LEFT;
+   return -1;
+}
+
 static struct {
const char *name;
cmp_type cmp_type;
@@ -910,14 +921,12 @@ static void populate_value(struct ref_array_item *ref)
align->position = ALIGN_LEFT;
 
while (*s) {
+   int position;
+
if (!strtoul_ui(s[0]->buf, 10, (unsigned int 
*)))
;
-   else if (!strcmp(s[0]->buf, "left"))
-   align->position = ALIGN_LEFT;
-   else if (!strcmp(s[0]->buf, "right"))
-   align->position = ALIGN_RIGHT;
-   else if (!strcmp(s[0]->buf, "middle"))
-   align->position = ALIGN_MIDDLE;
+   else if ((position = 
parse_align_position(s[0]->buf)) >= 0)
+   align->position = position;
else
die(_("improper format entered 
align:%s"), s[0]->buf);
s++;
-- 
2.7.0

--
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

Re: Replacing strbuf_getline_lf() by strbuf_getline() in wt-status.c

[Junio C Hamano]

Moritz Neeb  writes:

> Currently I am working on replacing strbuf_getline_lf() by
> strbuf_getline() in places where the input is trimmed immediately after
> reading, cf. $gmane/284104, "Notes on the remaining strbuf_getline_lf()
> callers", 2nd point.
>
> One instance I found was in wt-status.c. In read_rebase_todolist() the
> lines are read, checked for a comment_line_char and then trimmed. I
> wonder why the input is not trimmed before checking for this character?
> Is it safe to replace strbuf_getline_lf() by strbuf_getline() anyway?
>
> The only case I can imagine that could lead to unexpected behaviour then
> would be when someone sets the comment_line_char to CR. How likely is that?
>
> Why is the trim after checking for the comment char anyway? Should
> something like "   # foobar" not be considered as comment?

That is debatable, I would think, as "git commit" and others do not
generally accept a line that does not begin with a comment char as a
comment.

I however think we made an exception for the rebase-i's insn file
due to a mistake we made long time ago that allowed such line,
caused people to build a workflow around the assumption that it is
OK to prefix a comment line with whitespaces.

  Cf. the last paragraph of 1db168ee (rebase-i: loosen over-eager
  check_bad_cmd check, 2015-10-01).

  Cf. http://thread.gmane.org/gmane.comp.version-control.git/278841

So we probably want to be consistent with that in this codepath.
--
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 v4 12/12] ref-filter: introduce objectname_atom_parser()

[Karthik Nayak]

Introduce objectname_atom_parser() which will parse the
'%(objectname)' atom and store information into the 'used_atom'
structure based on the modifiers used along with the atom.

Helped-by: Ramsay Jones 
Helped-by: Eric Sunshine 
Signed-off-by: Karthik Nayak 
---
 ref-filter.c | 35 ---
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index b2043a0..d48e2a3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -43,6 +43,7 @@ static struct used_atom {
enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, 
C_SUB } option;
unsigned int nlines;
} contents;
+   enum { O_FULL, O_SHORT } objectname;
} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
@@ -102,6 +103,16 @@ static void contents_atom_parser(struct used_atom *atom, 
const char *arg)
die(_("unrecognized %%(contents) argument: %s"), arg);
 }
 
+static void objectname_atom_parser(struct used_atom *atom, const char *arg)
+{
+   if (!arg)
+   atom->u.objectname = O_FULL;
+   else if (!strcmp(arg, "short"))
+   atom->u.objectname = O_SHORT;
+   else
+   die(_("unrecognized %%(objectname) argument: %s"), arg);
+}
+
 static align_type parse_align_position(const char *s)
 {
if (!strcmp(s, "right"))
@@ -160,7 +171,7 @@ static struct {
{ "refname" },
{ "objecttype" },
{ "objectsize", FIELD_ULONG },
-   { "objectname" },
+   { "objectname", FIELD_STR, objectname_atom_parser },
{ "tree" },
{ "parent" },
{ "numparent", FIELD_ULONG },
@@ -439,15 +450,17 @@ static void *get_obj(const unsigned char *sha1, struct 
object **obj, unsigned lo
 }
 
 static int grab_objectname(const char *name, const unsigned char *sha1,
-   struct atom_value *v)
+  struct atom_value *v, struct used_atom *atom)
 {
-   if (!strcmp(name, "objectname")) {
-   v->s = xstrdup(sha1_to_hex(sha1));
-   return 1;
-   }
-   if (!strcmp(name, "objectname:short")) {
-   v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
-   return 1;
+   if (starts_with(name, "objectname")) {
+   if (atom->u.objectname == O_SHORT) {
+   v->s = xstrdup(find_unique_abbrev(sha1, 
DEFAULT_ABBREV));
+   return 1;
+   } else if (atom->u.objectname == O_FULL) {
+   v->s = xstrdup(sha1_to_hex(sha1));
+   return 1;
+   } else
+   die("BUG: unknown %%(objectname) option");
}
return 0;
 }
@@ -471,7 +484,7 @@ static void grab_common_values(struct atom_value *val, int 
deref, struct object
v->s = xstrfmt("%lu", sz);
}
else if (deref)
-   grab_objectname(name, obj->oid.hash, v);
+   grab_objectname(name, obj->oid.hash, v, _atom[i]);
}
 }
 
@@ -995,7 +1008,7 @@ static void populate_value(struct ref_array_item *ref)
v->s = xstrdup(buf + 1);
}
continue;
-   } else if (!deref && grab_objectname(name, ref->objectname, v)) 
{
+   } else if (!deref && grab_objectname(name, ref->objectname, v, 
atom)) {
continue;
} else if (!strcmp(name, "HEAD")) {
const char *head;
-- 
2.7.0

--
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 v4 11/12] ref-filter: introduce contents_atom_parser()

[Karthik Nayak]

Introduce contents_atom_parser() which will parse the '%(contents)'
atom and store information into the 'used_atom' structure based on the
modifiers used along with the atom. Also introduce body_atom_parser()
and subject_atom_parser() for parsing atoms '%(body)' and '%(subject)'
respectively.

Helped-by: Ramsay Jones 
Helped-by: Eric Sunshine 
Signed-off-by: Karthik Nayak 
---
 ref-filter.c | 77 ++--
 1 file changed, 49 insertions(+), 28 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 99c152d..b2043a0 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -39,6 +39,10 @@ static struct used_atom {
struct align align;
enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
remote_ref;
+   struct {
+   enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, 
C_SUB } option;
+   unsigned int nlines;
+   } contents;
} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
@@ -66,6 +70,38 @@ static void remote_ref_atom_parser(struct used_atom *atom, 
const char *arg)
die(_("unrecognized format: %%(%s)"), atom->name);
 }
 
+static void body_atom_parser(struct used_atom *atom, const char *arg)
+{
+   if (arg)
+   die("%%(body) atom does not take arguments");
+   atom->u.contents.option = C_BODY_DEP;
+}
+
+static void subject_atom_parser(struct used_atom *atom, const char *arg)
+{
+   if (arg)
+   die("%%(subject) atom does not take arguments");
+   atom->u.contents.option = C_SUB;
+}
+
+static void contents_atom_parser(struct used_atom *atom, const char *arg)
+{
+   if (!arg)
+   atom->u.contents.option = C_BARE;
+   else if (!strcmp(arg, "body"))
+   atom->u.contents.option = C_BODY;
+   else if (!strcmp(arg, "signature"))
+   atom->u.contents.option = C_SIG;
+   else if (!strcmp(arg, "subject"))
+   atom->u.contents.option = C_SUB;
+   else if (skip_prefix(arg, "lines=", )) {
+   atom->u.contents.option = C_LINES;
+   if (strtoul_ui(arg, 10, >u.contents.nlines))
+   die(_("positive value expected contents:lines=%s"), 
arg);
+   } else
+   die(_("unrecognized %%(contents) argument: %s"), arg);
+}
+
 static align_type parse_align_position(const char *s)
 {
if (!strcmp(s, "right"))
@@ -145,9 +181,9 @@ static struct {
{ "taggerdate", FIELD_TIME },
{ "creator" },
{ "creatordate", FIELD_TIME },
-   { "subject" },
-   { "body" },
-   { "contents" },
+   { "subject", FIELD_STR, subject_atom_parser },
+   { "body", FIELD_STR, body_atom_parser },
+   { "contents", FIELD_STR, contents_atom_parser },
{ "upstream", FIELD_STR, remote_ref_atom_parser },
{ "push", FIELD_STR, remote_ref_atom_parser },
{ "symref" },
@@ -160,11 +196,6 @@ static struct {
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
 
-struct contents {
-   unsigned int lines;
-   struct object_id oid;
-};
-
 struct ref_formatting_stack {
struct ref_formatting_stack *prev;
struct strbuf output;
@@ -181,7 +212,6 @@ struct atom_value {
const char *s;
union {
struct align align;
-   struct contents contents;
} u;
void (*handler)(struct atom_value *atomv, struct ref_formatting_state 
*state);
unsigned long ul; /* used for sorting when not FIELD_STR */
@@ -733,19 +763,15 @@ static void grab_sub_body_contents(struct atom_value 
*val, int deref, struct obj
 
for (i = 0; i < used_atom_cnt; i++) {
const char *name = used_atom[i].name;
+   struct used_atom *atom = _atom[i];
struct atom_value *v = [i];
-   const char *valp = NULL;
if (!!deref != (*name == '*'))
continue;
if (deref)
name++;
if (strcmp(name, "subject") &&
strcmp(name, "body") &&
-   strcmp(name, "contents") &&
-   strcmp(name, "contents:subject") &&
-   strcmp(name, "contents:body") &&
-   strcmp(name, "contents:signature") &&
-   !starts_with(name, "contents:lines="))
+   !starts_with(name, "contents"))
continue;
if (!subpos)
find_subpos(buf, sz,
@@ -753,28 +779,23 @@ static void grab_sub_body_contents(struct atom_value 
*val, int deref, struct obj
, , ,
, );
 
-   if (!strcmp(name, "subject"))
+   if (atom->u.contents.option == C_SUB)
 

RE: [RFC/PATCH] Git doc: GPL2 does not apply to repo data

[Jonathan Smith]

Hi all

I would like to thank you each for being so helpful with my request.

As a humble developer trying to penetrate the corporate environment with 
leading technologies such as Git, it's awesome to know you guys are so 
proactive with providing support.

I'll be taking all of your contributions as ammo as I continue to fight for Git 
here.

Thank you!

Jonathan

-Original Message-
From: Junio C Hamano [mailto:gits...@pobox.com] 
Sent: Monday, February 01, 2016 11:08 AM
To: Philip Oakley 
Cc: GitList ; Jonathan Smith 
; Johannes Schindelin 
Subject: Re: [RFC/PATCH] Git doc: GPL2 does not apply to repo data

Philip Oakley  writes:

> diff --git a/Documentation/git.txt b/Documentation/git.txt index 
> bff6302..137c89c 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -1132,6 +1132,17 @@ of clones and fetches.
> - any external helpers are named by their protocol (e.g., use
>   `hg` to allow the `git-remote-hg` helper)
>  
> +Licencing: Your data, and the Git tool[[Licencing]]
> +---
> +
> +Git is an open source tool provided under GPL2.
> +Git was designed to be, and is, the version control system for the 
> +Linux codebase.
> +Your respository data created by Git is not subject to Git's GNU2 
> +licence, see GPL FAQs 
> +http://www.gnu.org/licenses/old-licenses/gpl-2.0-faq.en.html#TOCGPLOutput).
> +
> +User should apply a licence of their own choice to their repository data.
>  
>  Discussion[[Discussion]]
>  

While I know you mean well, and I do understand the sentiment behind this 
addition, there are at least two reasons why I do not want to (and why we 
should not) add any "clarification" or "interpretation"
like this.

One is because such a statement is pointless.  Because we do not do copyright 
assignment to the project, you are not the sole copyright owner of Git.  
Individual contributors hold copyright to the part they wrote.  The above 
statement you made, even with an endorsement by me as the project lead, does 
not have any power to assure that the users will not get sued by one copyright 
holder, who is not you or me, and at that point it is up to the court to 
interpret GPLv2.
We can call such a copyright holder crazy or call such a suit frivolous, but 
that does not change the fact that the court is what decides the matter, so 
having that statement does not help the user.

Another is because we are amateurs.  Philip, you may or may not be a lawyer 
yourself, but I know you are not _our_ lawyer.  An amateurish "interpretation" 
or "clarification" does not necessarily clarify the text but it muddies it, 
especially when done carelessly.  Imagine a case where a user creates a derived 
work of Git itself and stored it in a Git repository.  "Your respository data 
created by Git is not subject to Git's GNU2"--really?  At least the phrasing 
must say that the act of storing something in Git alone would not *MAKE* that 
something governed under GPLv2.  What the user puts in Git may already be 
covered under GPLv2 for other reasons, and a statement carelessly written like 
the above can be twisted to read as if we are endorsing use of our code outside 
GPLv2 as long as they store it in Git repository, which is not what you meant 
to say, but "that is not what the copyright holder meant" is another thing the 
lawyer need to argue in court to convince the judge, when we need to go after a 
real copyright violator.

We should leave the lawyering to real lawyers and we should not add unnecessary 
work of interpreting our amateurish loose statement to our laywers.

Thanks.


This e-mail and any attachments may contain confidential information. If you
are not the intended recipient (or have received this e-mail in error)
please notify the sender immediately and destroy this e-mail. Any
unauthorized copying, disclosure or distribution of the material in this 
e-mail is strictly forbidden.
--
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

Re: GPL v2 authoritative answer on stored code as a derived work

[Philip Oakley]

From: "Johannes Schindelin" 
Sent: Thursday, January 28, 2016 8:16 AM

Hi Philip,

On Wed, 27 Jan 2016, Philip Oakley wrote:


From: "Junio C Hamano" 
> Jonathan Smith  writes:
>
> > It's pretty clear that code stored in a Git repository isn't
> > considered a derived work of Git, regardless of whether it is used
> > in a commercial context or otherwise.

I'm guessing here, but I suspect that while its 'pretty clear' to 
Jonathan,

that he has met others who aren't so clear or trusting, and it's that
distrustful community that would need convincing.


It is not so much distrust as something you could take to court, I guess,
because an *authoritative* answer was asked for. Now, the question is a
legal one, so it is pretty clear (;-)) to me that only a lawyer could give
that answer.

Having said that, I know of plenty of companies storing their proprietary
code in Git repositories, and I would guess that they cleared that with
their lawyers first.


I've had a look though the various FAQs and other discussions about the
GPL and the FUD associated with it.

I've put together an outline of a patch to the git(1) man page, with commit
message to explain the issues (the lawyers need pointing in the right 
direction

so they can think clearly, rather than give the usual 'No' answer ;-)

Having it in $gmane at least captures the rationale, even if the patch goes
nowhere.



Jonathan, please do not take that as any indication that I try to give
this answer: if you want an authoritative answer to your question, you
really need to consider asking a lawyer.

Ciao,
Dscho
--
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



--
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

[RFC/PATCH] Git doc: GPL2 does not apply to repo data

[Philip Oakley]

Some potential commercial users may be put off by the FUD
(Fear, Uncertainty and Doubt) that has been raised in the past
regarding the use of FOSS software.

In such communities, even the legal advice may be misinformed
and over-cautious regarding the storage of code and other
intellectual property in a Git repository for fear that Git's
GPL2 licence may somehow 'infect' the respository.

Add simple statements highlighting Git's licence, it's use
for Linux, to imply industrial-strength, and that users should
apply a suitable licence of their choice because the Git GPL
licence does not apply to their repo data.

It should be noted that a 'git init' will create a repo that while
empty of user data does provide the .git directory structure, which
includes a number of template files ('hooks\pre-rebase.sample' is
explicitly copyright), default refs and config file. Some may suggest
that these carry the GPL2 to the repo.

The GPL2 will still apply to the hook templates and the other
template files, but these, even if modified (becoming derived works)
would be distributed with the repo, satisfying the GPL.

The new content copyright belongs to user. Request that they state
their licence terms in line with recent FOSS industry practice.

Signed-off-by: Philip Oakley 
---
asciidoc formatting not checked.

The fear of 'infection' of a repo by the templates copied to the
repo by 'git init', should not be underestimated, given the need
for the Bison exception. Sometimes it has to be spelt out why it's
not an issue (in the commit messsage)

https://help.github.com/articles/open-source-licensing/

http://thread.gmane.org/gmane.comp.version-control.git/284715/
---
 Documentation/git.txt | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index bff6302..137c89c 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1132,6 +1132,17 @@ of clones and fetches.
  - any external helpers are named by their protocol (e.g., use
`hg` to allow the `git-remote-hg` helper)
 
+Licencing: Your data, and the Git tool[[Licencing]]
+---
+
+Git is an open source tool provided under GPL2.
+Git was designed to be, and is, the version control system
+for the Linux codebase.
+Your respository data created by Git is not subject to Git's GNU2
+licence, see GPL FAQs
+http://www.gnu.org/licenses/old-licenses/gpl-2.0-faq.en.html#TOCGPLOutput).
+
+User should apply a licence of their own choice to their repository data.
 
 Discussion[[Discussion]]
 
-- 
2.7.0.windows.1

--
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] t6302: drop unnecessary GPG requirement

[Eric Sunshine]

These tests are concerned specifically with filtering, sorting,
formatting behavior of git-for-each-ref, yet if GPG is not present, the
entire test script is skipped even though none of the tests depend upon
or care whether the tags are signed. This unnecessary dependency upon
GPG may prevent these tests from being more widely run, so drop it.

Signed-off-by: Eric Sunshine 
---

I noticed this while reviewing[1] v3 of Karthik's "optimize
ref-filter.c:populate_value()" series when I tried to run tests the
series added but couldn't due to missing GPG.

[1]: http://article.gmane.org/gmane.comp.version-control.git/284766

t/t6302-for-each-ref-filter.sh | 11 ++-
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index fe4796c..dea2a9e 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -3,13 +3,6 @@
 test_description='test for-each-refs usage of ref-filter APIs'
 
 . ./test-lib.sh
-. "$TEST_DIRECTORY"/lib-gpg.sh
-
-if ! test_have_prereq GPG
-then
-   skip_all="skipping for-each-ref tests, GPG not available"
-   test_done
-fi
 
 test_expect_success 'setup some history and refs' '
test_commit one &&
@@ -17,8 +10,8 @@ test_expect_success 'setup some history and refs' '
test_commit three &&
git checkout -b side &&
test_commit four &&
-   git tag -s -m "A signed tag message" signed-tag &&
-   git tag -s -m "Annonated doubly" double-tag signed-tag &&
+   git tag -m "A signed tag message" signed-tag &&
+   git tag -m "Annonated doubly" double-tag signed-tag &&
git checkout master &&
git update-ref refs/odd/spot master
 '
-- 
2.7.0.333.g9c3d022

--
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

Re: [RFC/PATCH 0/5] [WAS: Submodule Groups] Labels and submodule.autoInitialize

[Jens Lehmann]

Am 26.01.2016 um 22:50 schrieb Stefan Beller:

On Tue, Jan 26, 2016 at 12:59 PM, Jens Lehmann  wrote:


Ok. Though we might wanna call it submodule.autoUpdate, as initializing
it is only the prerequisite for automatically updating submodules. And
I believe automatically updating is the thing we're after here, right?



I am not sure here, too. I would not mind an occasional "git submodule
update"
for whenever I want upstream to come down on my disk.





However that's what I
do with "git pull" in the non-submodule case, so you'd expect git pull to
also run the update strategies for all submodules which are configured to
autoUpdate?

That makes sense to me. Though I never use "git pull" to begin with.
I always use fetch and see how to go from there (merge or rebase
after inspecting the code I fetched). That would mean we want to
add the autoUpdate strategy to merge/rebase and the fetching of
submodules to the fetch command?



Hmm, maybe autoUpdate promises too much.


Yes, this very much. I feel like I get burned whenever I send a large
patch series.
So I want to have this first feature be the smallest "operational
unit" that makes sense.


Agreed.


After all this config is
just about which submodules are chosen to be updated on clone and
submodule update, not on all the other work tree manipulating
commands.


So you'd imagine that "git submodule update" would remove the
submodule and setup an empty directory in case that submodule is
not configured ? (after switching branches or when just having cloned
that thing.)


Not as a result of the label feature we are talking about here,
I think that should just do what currently happens to removed
submodules: they are left populated but are ignored by status,
diff and submodule update. Removing the content is part of the
recursive submodule update topic.


And it's similar to what sparse does. So what about calling that
"submodule.updateSparse"? Or maybe "submodule.sparseCheckout"?
Suggestions welcome.


I'd only suggest when it's clear to me what that option actually does. :)


Fair enough! ;-)


And the
fetch command needs to fetch submodule changes too when they happen in
a branch where this submodule is part of a label group configured to be
updated automatically, no matter what is currently found in the work
tree.



Right, as said above fetch needs to fetch all the submodules as well. I
wonder
if it needs to fetch all submodule sha1s only or just try to get as
much from the
submodule as possible.



Right now we just do a simple fetch, but only fetching the SHA-1s could
be an optimization useful for busy submodules later on.


I'd rather not call it optimisation, but a correctness thing. What if you
force-pushed other content to the submodule (the sha1 is gone and
maybe should not be reachable)or the other case where you want to
clone the submodule with depth 1 (that is a serious case, which currently
breaks). In the shallow submodule case you need to have the exact sha1
for cloning, otherwise it doesn't work correctly.


I'm convinced. Correctness it is! :-)


So I'd propose to:

*) Initialize every submodule present on clone or newly fetched when
 the autoUpdate config is set.



What if you clone branch A and then switch to B ? B has a submodule which
was not initialized in A. I do not think initializing on clone/fetch
is the right thing
to do, but rather the branch switching command (checkout) shall make sure
all its autoUpdate/autoInitialze submodules are setup properly, no?



I disagree. If you init all submodules on clone/fetch you might need
to change the upstream URL right after that. You can't do that on a
subsequent branch switch which needs to initialize the submodule again,
as the former deinit did nuke that configuration.


So we need to keep the information around, which we do by keeping
all the modules initialized all the time.


Yup.


*) Automatically fetch only those submodules that are changed in
 a commit where they have a label configured (in the commit's
 .gitmodules or locally) that is to be checked out.



Not sure I follow here.



We could restrict fetch to not fetch everything but just those changes
needed for sparse submodule update. To be able to do that it would
have to examine the fetched superproject commits if a submodule changed
and if it is configured to be automatically updated in that commit.


ok, that's an optimisation for later? (not strictly needed for the first series)


Definitely.


*) Let "git submodule update" update only those submodules that
 have an autoupdate label configured.



Why not update all initialized submodules? (In my imagination
"all initialized submodules" are equal to "all submodules the user is
interested in", i.e. when going from branch A to B, the checkout will
(de-)init submodules as necessary.



And throw away any customization the user did (to the URL or other
configurations)?

Without this sparse/label/group functionality, init is the way the

Re: [RFC/PATCH] Git doc: GPL2 does not apply to repo data

[Junio C Hamano]

Philip Oakley  writes:

> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index bff6302..137c89c 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -1132,6 +1132,17 @@ of clones and fetches.
> - any external helpers are named by their protocol (e.g., use
>   `hg` to allow the `git-remote-hg` helper)
>  
> +Licencing: Your data, and the Git tool[[Licencing]]
> +---
> +
> +Git is an open source tool provided under GPL2.
> +Git was designed to be, and is, the version control system
> +for the Linux codebase.
> +Your respository data created by Git is not subject to Git's GNU2
> +licence, see GPL FAQs
> +http://www.gnu.org/licenses/old-licenses/gpl-2.0-faq.en.html#TOCGPLOutput).
> +
> +User should apply a licence of their own choice to their repository data.
>  
>  Discussion[[Discussion]]
>  

While I know you mean well, and I do understand the sentiment behind
this addition, there are at least two reasons why I do not want to
(and why we should not) add any "clarification" or "interpretation"
like this.

One is because such a statement is pointless.  Because we do not do
copyright assignment to the project, you are not the sole copyright
owner of Git.  Individual contributors hold copyright to the part
they wrote.  The above statement you made, even with an endorsement
by me as the project lead, does not have any power to assure that
the users will not get sued by one copyright holder, who is not you
or me, and at that point it is up to the court to interpret GPLv2.
We can call such a copyright holder crazy or call such a suit
frivolous, but that does not change the fact that the court is what
decides the matter, so having that statement does not help the user.

Another is because we are amateurs.  Philip, you may or may not be a
lawyer yourself, but I know you are not _our_ lawyer.  An amateurish
"interpretation" or "clarification" does not necessarily clarify the
text but it muddies it, especially when done carelessly.  Imagine a
case where a user creates a derived work of Git itself and stored it
in a Git repository.  "Your respository data created by Git is not
subject to Git's GNU2"--really?  At least the phrasing must say that
the act of storing something in Git alone would not *MAKE* that
something governed under GPLv2.  What the user puts in Git may
already be covered under GPLv2 for other reasons, and a statement
carelessly written like the above can be twisted to read as if we
are endorsing use of our code outside GPLv2 as long as they store it
in Git repository, which is not what you meant to say, but "that is
not what the copyright holder meant" is another thing the lawyer
need to argue in court to convince the judge, when we need to go
after a real copyright violator.

We should leave the lawyering to real lawyers and we should not add
unnecessary work of interpreting our amateurish loose statement to
our laywers.

Thanks.
--
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 v4 05/12] ref-filter: introduce parsing functions for each valid atom

[Karthik Nayak]

Parsing atoms is done in populate_value(), this is repetitive and
hence expensive. Introduce a parsing function which would let us parse
atoms beforehand and store the required details into the 'used_atom'
structure for further usage.

Helped-by: Eric Sunshine 
Signed-off-by: Karthik Nayak 
---
 ref-filter.c | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 3736dc3..92b4419 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -36,6 +36,7 @@ static int need_color_reset_at_eol;
 static struct {
const char *name;
cmp_type cmp_type;
+   void (*parser)(struct used_atom *atom, const char *arg);
 } valid_atom[] = {
{ "refname" },
{ "objecttype" },
@@ -114,6 +115,7 @@ struct atom_value {
 int parse_ref_filter_atom(const char *atom, const char *ep)
 {
const char *sp;
+   const char *arg;
int i, at;
 
sp = atom;
@@ -138,10 +140,9 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
 * shouldn't be used for checking against the valid_atom
 * table.
 */
-   const char *formatp = strchr(sp, ':');
-   if (!formatp || ep < formatp)
-   formatp = ep;
-   if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))
+   arg = memchr(sp, ':', ep - sp);
+   if ((!arg || len == arg - sp) &&
+   !memcmp(valid_atom[i].name, sp, len))
break;
}
 
@@ -154,6 +155,10 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
REALLOC_ARRAY(used_atom, used_atom_cnt);
used_atom[at].name = xmemdupz(atom, ep - atom);
used_atom[at].type = valid_atom[i].cmp_type;
+   if (arg)
+   arg = used_atom[at].name + (arg - atom) + 1;
+   if (valid_atom[i].parser)
+   valid_atom[i].parser(_atom[at], arg);
if (*atom == '*')
need_tagged = 1;
if (!strcmp(used_atom[at].name, "symref"))
-- 
2.7.0

--
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 v4 03/12] ref-filter: bump 'used_atom' and related code to the top

[Karthik Nayak]

Bump code to the top for usage in further patches.
---
 ref-filter.c | 30 +++---
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 38f38d4..c3a8372 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -16,6 +16,21 @@
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 
+/*
+ * An atom is a valid field atom listed below, possibly prefixed with
+ * a "*" to denote deref_tag().
+ *
+ * We parse given format string and sort specifiers, and make a list
+ * of properties that we need to extract out of objects.  ref_array_item
+ * structure will hold an array of values extracted that can be
+ * indexed with the "atom number", which is an index into this
+ * array.
+ */
+static const char **used_atom;
+static cmp_type *used_atom_type;
+static int used_atom_cnt, need_tagged, need_symref;
+static int need_color_reset_at_eol;
+
 static struct {
const char *name;
cmp_type cmp_type;
@@ -92,21 +107,6 @@ struct atom_value {
 };
 
 /*
- * An atom is a valid field atom listed above, possibly prefixed with
- * a "*" to denote deref_tag().
- *
- * We parse given format string and sort specifiers, and make a list
- * of properties that we need to extract out of objects.  ref_array_item
- * structure will hold an array of values extracted that can be
- * indexed with the "atom number", which is an index into this
- * array.
- */
-static const char **used_atom;
-static cmp_type *used_atom_type;
-static int used_atom_cnt, need_tagged, need_symref;
-static int need_color_reset_at_eol;
-
-/*
  * Used to parse format string and sort specifiers
  */
 int parse_ref_filter_atom(const char *atom, const char *ep)
-- 
2.7.0

--
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 v4 02/12] ref-filter: use strbuf_split_str_omit_term()

[Karthik Nayak]

Use the newly introduced strbuf_split_str_omit_term() rather than
using strbuf_split_str() and manually removing the ',' terminator.

Helped-by: Eric Sunshine 
Signed-off-by: Karthik Nayak 
---
 ref-filter.c | 9 +
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index f097176..38f38d4 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -892,18 +892,11 @@ static void populate_value(struct ref_array_item *ref)
if (!valp)
die(_("expected format: 
%%(align:,)"));
 
-   /*
-* TODO: Implement a function similar to 
strbuf_split_str()
-* which would omit the separator from the end of each 
value.
-*/
-   s = to_free = strbuf_split_str(valp, ',', 0);
+   s = to_free = strbuf_split_str_omit_term(valp, ',', 0);
 
align->position = ALIGN_LEFT;
 
while (*s) {
-   /*  Strip trailing comma */
-   if (s[1])
-   strbuf_setlen(s[0], s[0]->len - 1);
if (!strtoul_ui(s[0]->buf, 10, (unsigned int 
*)))
;
else if (!strcmp(s[0]->buf, "left"))
-- 
2.7.0

--
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

Re: [PATCH v3 6/6] worktree add: switch to worktree version 1

[Duy Nguyen]

On Mon, Feb 1, 2016 at 12:33 PM, Max Kirillov  wrote:
> On Tue, Jan 26, 2016 at 06:44:45PM +0700, Nguyễn Thái Ngọc Duy wrote:
>> + for (key_p = per_wortree_keys; *key_p; key_p++) {
>> + const char *key = *key_p;
>> + char *value = get_key(sb.buf, key);
>> +
>> + if (value) {
>> + if (git_config_set(key, value))
>> + die(_("failed to keep %s in main 
>> worktree's config file"), key);
>> + if (git_config_set_in_file(sb.buf, key, NULL))
>> + die(_("failed to delete %s in shared 
>> config file"), key);
>> + free(value);
>> + }
>> + }
>
> 1. For submodules (which must be left per-worktree) this
> approach is not going to work, because you don't know all
> variables in advance. You could scan the config file and
> match those actual keys which are there with patterns.

Hmm.. we could keep existing submodule.* per-worktree. New variables
are per-worktree by default, unless you do "git config --repo" in
git-submodule.sh. Am I missing something?

> 2. This migrates variables to the default (or current?)
> worktree, what about others existing?

In v0, $C/config contains all shared variables, once we move these
shared vars to $C/common/config, they will be visible to all other
worktrees. Or do you replicate per-worktree vars in $C/config to all
worktrees ?
-- 
Duy
--
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

Re: [bug] Possible Windows 'git mv' bug

[Johannes Schindelin]

Hi Aaron,

On Sun, 31 Jan 2016, Torsten Bögershausen wrote:

> On 2016-01-31 15.03, Aaron Gray wrote:
> > 
> > I think I have found a possible difference in behaviour between
> > Windows git commandline distro and Linux git
> > 
> > basically If I do a :-
> > 
> > git mv logger.h Logger.h
> > 
> > I get the following :-
> > 
> > fatal: destination exists, source=lib/logger.h, destination=lib/Logger.h
> > 
> > It looks and smells like a bug to me !

Thanks for your bug report. Having said that, I would like to suggest a
couple improvements for future reference:

Git for Windows' home page (https://git-for-windows.github.com/#contribute)
asks explicitly to please open tickets on Git for Windows' bug tracker
(https://github.com/git-for-windows/git/issues).

Also, at least one crucial bit of information is missing: the Git for
Windows version. Typically this is enough, but the Windows version is
often also crucial. To prevent crucial bits of information from missing, I
highly recommend volunteering information up front (in my mind, a good
rule of thumb is: spend as much care crafting your bug report and thinking
about what to include as you would wish the combined readership would
spend on helping you). If I may say so, I provided a good list of advices
to help crafting bug reports:
https://github.com/git-for-windows/git/wiki/Issue-reporting-guidelines

> Which version of Git are you using ?
> Because it is fixed in the latest version in Git and Git for Windows.

Another piece of advice I detail in our issue reporting guidelines is to
search the existing bug tracker (one purpose of the tracker is to keep
track of what has been reported and what has not yet been reported, after
all).

And indeed, I opened this ticket:

https://github.com/git-for-windows/git/issues/419

... whose initial report incidentally also lacks the crucial information
mentioned above! So I have to step up my own game, too.

This report *also* includes detailed information, though, in particular
that Torsten fixed this issue a long time ago, which suggests to me that
you are somehow still stuck with the 1.x train (run `git version` to find
out).

Ciao,
Johannes

RE: [RFC/PATCH] Git doc: GPL2 does not apply to repo data

[Johannes Schindelin]

Hi Jonathan,

On Sun, 31 Jan 2016, Jonathan Smith wrote:

> I'll be taking all of your contributions as ammo as I continue to fight
> for Git here.

I would hope that it is unnecessary to point weapons at managers to
"convince" them to use Git.

In the meantime, I think we have accumulated a lot of arguments in favor
of using Git to manage source code.

For less tech-savvy managers, I found that name dropping works quite well
(read: naming a couple of well-known companies/products that switched to
Git).

Ciao,
Johannes
--
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

Re: [RFC/PATCH] Git doc: GPL2 does not apply to repo data

[Matthieu Moy]

- Original Message -
> For less tech-savvy managers, I found that name dropping works quite well
> (read: naming a couple of well-known companies/products that switched to
> Git).

In the same category: "GitHub has 12 millions users" works rather well.

-- 
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

Re: [PATCH] t6302: drop unnecessary GPG requirement

[Johannes Schindelin]

Hi Eric & Junio,

On Sun, 31 Jan 2016, Junio C Hamano wrote:

> Eric Sunshine  writes:
> 
> > These tests are concerned specifically with filtering, sorting,
> > formatting behavior of git-for-each-ref, yet if GPG is not present,
> > the entire test script is skipped even though none of the tests depend
> > upon or care whether the tags are signed. This unnecessary dependency
> > upon GPG may prevent these tests from being more widely run, so drop
> > it.
> 
> [...]
>
> Would it make sense to introduce a helper function specific to this
> script to be used to prepare the expected output, to replace cat <<,
> that goes like this?
> 
> [...]

An even easier solution might be to *not* set up the signed tags in the
'setup' part, but only in the respective test case, and delete them right
away after said test case?

Something like this (I even tested this with and without the GPG prereq):

-- snipsnap --
From: Johannes Schindelin 
Subject: [PATCH] Do not make t6302 depend on gpg wholesale

There is but a single test case, in fact, that depends on gpg. Let's just
make the other test cases independent of gpg and add the GPG prereq to
said single test case.

Noticed by Eric Sunshine.

Signed-off-by: Johannes Schindelin 
---
 t/t6302-for-each-ref-filter.sh | 33 -
 1 file changed, 4 insertions(+), 29 deletions(-)

diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index fe4796c..e3a5636 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -5,20 +5,12 @@ test_description='test for-each-refs usage of ref-filter APIs'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-gpg.sh
 
-if ! test_have_prereq GPG
-then
-   skip_all="skipping for-each-ref tests, GPG not available"
-   test_done
-fi
-
 test_expect_success 'setup some history and refs' '
test_commit one &&
test_commit two &&
test_commit three &&
git checkout -b side &&
test_commit four &&
-   git tag -s -m "A signed tag message" signed-tag &&
-   git tag -s -m "Annonated doubly" double-tag signed-tag &&
git checkout master &&
git update-ref refs/odd/spot master
 '
@@ -33,7 +25,10 @@ test_expect_success 'filtering with --points-at' '
test_cmp expect actual
 '
 
-test_expect_success 'check signed tags with --points-at' '
+test_expect_success GPG 'check signed tags with --points-at' '
+   git tag -s -m "A signed tag message" signed-tag side &&
+   git tag -s -m "Annonated doubly" double-tag signed-tag &&
+   test_when_finished git tag -d signed-tag &&
sed -e "s/Z$//" >expect <<-\EOF &&
refs/heads/side Z
refs/tags/four Z
@@ -58,9 +53,7 @@ test_expect_success 'filtering with --merged' '
 test_expect_success 'filtering with --no-merged' '
cat >expect <<-\EOF &&
refs/heads/side
-   refs/tags/double-tag
refs/tags/four
-   refs/tags/signed-tag
EOF
git for-each-ref --format="%(refname)" --no-merged=master >actual &&
test_cmp expect actual
@@ -71,9 +64,7 @@ test_expect_success 'filtering with --contains' '
refs/heads/master
refs/heads/side
refs/odd/spot
-   refs/tags/double-tag
refs/tags/four
-   refs/tags/signed-tag
refs/tags/three
refs/tags/two
EOF
@@ -90,10 +81,8 @@ test_expect_success 'left alignment is default' '
refname is refs/heads/master  |refs/heads/master
refname is refs/heads/side|refs/heads/side
refname is refs/odd/spot  |refs/odd/spot
-   refname is refs/tags/double-tag|refs/tags/double-tag
refname is refs/tags/four |refs/tags/four
refname is refs/tags/one  |refs/tags/one
-   refname is refs/tags/signed-tag|refs/tags/signed-tag
refname is refs/tags/three|refs/tags/three
refname is refs/tags/two  |refs/tags/two
EOF
@@ -106,10 +95,8 @@ test_expect_success 'middle alignment' '
| refname is refs/heads/master |refs/heads/master
|  refname is refs/heads/side  |refs/heads/side
|   refname is refs/odd/spot   |refs/odd/spot
-   |refname is refs/tags/double-tag|refs/tags/double-tag
|  refname is refs/tags/four   |refs/tags/four
|   refname is refs/tags/one   |refs/tags/one
-   |refname is refs/tags/signed-tag|refs/tags/signed-tag
|  refname is refs/tags/three  |refs/tags/three
|   refname is refs/tags/two   |refs/tags/two
EOF
@@ -122,10 +109,8 @@ test_expect_success 'right alignment' '
|  refname is refs/heads/master|refs/heads/master
|refname is refs/heads/side|refs/heads/side
|  refname is refs/odd/spot|refs/odd/spot
-   |refname is refs/tags/double-tag|refs/tags/double-tag
| refname is refs/tags/four|refs/tags/four
|  refname is 

Re: [PATCH 5/6] checkout-index: disallow "--no-stage" option

[Junio C Hamano]

Jeff King  writes:

> We do not really expect people to use "--no-stage", but if
> they do, git currently segfaults. We could instead have it
> undo the effects of a previous "--stage", but this gets
> tricky around the "to_tempfile" flag. We cannot simply reset
> it to 0, because we don't know if it was set by a previous
> "--stage=all" or an explicit "--temp" option.
>
> We could solve this by setting a flag and resolving
> to_tempfile later, but it's not worth the effort. Nobody
> actually wants to use "--no-stage"; we are just trying to
> fix a potential segfault here.
>
> While we're in the area adding a translated string, let's
> mark the other possible error message in this function as
> translatable.

Thanks.  That's worth fixing and I agree with the decision that it
is the best way to go to not support '--no-stage'.

>
> Signed-off-by: Jeff King 
> ---
>  builtin/checkout-index.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> index f8179a7..7a9b561 100644
> --- a/builtin/checkout-index.c
> +++ b/builtin/checkout-index.c
> @@ -133,6 +133,8 @@ static struct lock_file lock_file;
>  static int option_parse_stage(const struct option *opt,
> const char *arg, int unset)
>  {
> + if (unset)
> + return error(_("--stage cannot be negated"));

Hmm, it is surprising that there is no parse-options flag that says
"this cannot be negated".

>   if (!strcmp(arg, "all")) {
>   to_tempfile = 1;
>   checkout_stage = CHECKOUT_ALL;
> @@ -141,7 +143,7 @@ static int option_parse_stage(const struct option *opt,
>   if ('1' <= ch && ch <= '3')
>   checkout_stage = arg[0] - '0';
>   else
> - die("stage should be between 1 and 3 or all");
> + die(_("stage should be between 1 and 3 or all"));
>   }
>   return 0;
>  }
--
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

Re: [PATCH 5/6] checkout-index: disallow "--no-stage" option

[Jeff King]

On Sun, Jan 31, 2016 at 06:18:39PM -0800, Junio C Hamano wrote:

> >  builtin/checkout-index.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> > index f8179a7..7a9b561 100644
> > --- a/builtin/checkout-index.c
> > +++ b/builtin/checkout-index.c
> > @@ -133,6 +133,8 @@ static struct lock_file lock_file;
> >  static int option_parse_stage(const struct option *opt,
> >   const char *arg, int unset)
> >  {
> > +   if (unset)
> > +   return error(_("--stage cannot be negated"));
> 
> Hmm, it is surprising that there is no parse-options flag that says
> "this cannot be negated".

There is, but you have to stop using the nice OPT_CALLBACK macro and do
a full-on "{}" struct literal in the options. Since we have a callback,
I figured doing this is less ugly to look at. But it does mean making up
our own message.

I didn't actually try it yesterday; having just done so, it's a lot less
bad than I expected. And I also noticed yet another problem with it. :-/

How about this as a replacement patch?

-- >8 --
Subject: [PATCH] checkout-index: disallow "--no-stage" option

We do not really expect people to use "--no-stage", but if
they do, git currently segfaults. We could instead have it
undo the effects of a previous "--stage", but this gets
tricky around the "to_tempfile" flag. We cannot simply reset
it to 0, because we don't know if it was set by a previous
"--stage=all" or an explicit "--temp" option.

We could solve this by setting a flag and resolving
to_tempfile later, but it's not worth the effort. Nobody
actually wants to use "--no-stage"; we are just trying to
fix a potential segfault here.

While we're in the area, let's improve the user-facing
messages for this option. The error string should be
translatable, and we should give some hint in the "-h"
output about what can go in the argument field.

Signed-off-by: Jeff King 
---
I also notice that you cannot use "--stage=0" to reset a previous
"--stage=1". It probably would not hurt to allow that, but I left it out
of this patch.

 builtin/checkout-index.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index f8179a7..92c6967 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -141,7 +141,7 @@ static int option_parse_stage(const struct option *opt,
if ('1' <= ch && ch <= '3')
checkout_stage = arg[0] - '0';
else
-   die("stage should be between 1 and 3 or all");
+   die(_("stage should be between 1 and 3 or all"));
}
return 0;
 }
@@ -173,9 +173,9 @@ int cmd_checkout_index(int argc, const char **argv, const 
char *prefix)
N_("write the content to temporary files")),
OPT_STRING(0, "prefix", _dir, N_("string"),
N_("when creating files, prepend ")),
-   OPT_CALLBACK(0, "stage", NULL, NULL,
+   { OPTION_CALLBACK, 0, "stage", NULL, "1-3|all",
N_("copy out the files from named stage"),
-   option_parse_stage),
+   PARSE_OPT_NONEG, option_parse_stage },
OPT_END()
};
 
-- 
2.7.0.489.g6faad84


--
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

Re: [PATCH v3 5/6] config: select .git/common/config with --repo

[Duy Nguyen]

On Sun, Jan 31, 2016 at 5:10 AM, Max Kirillov  wrote:
> On Tue, Jan 26, 2016 at 06:44:44PM +0700, Nguyễn Thái Ngọc Duy wrote:
>> This new option allows the user to write to or read from
>> .git/common/config in worktree v1. In worktree v0, --repo is an alias
>> of --local.
>
> Looks like by default a value is always set in a local
> config, which might be dangerous for remote.* or gc.*
> parameters, for example. I think that even if reading is
> done uniformly setting could depend on the actual variable
> being set if no location specified.

I grepped "git config" in scripts to see if we need to change any to
use --repo, but I forgot about builtin commands. git-remote needs to
store remote.* in the shared config file instead.

gc.* and others are set manually by the user, so they decide. We can
have a user-controlled filter that catches certain variables and
suggests they are stored in shared config instead, but this is
optional.
-- 
Duy
--
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

Re: [PATCH v3 1/6] worktree: new repo extension to manage worktree behaviors

[Stefan Monnier]

> As a heavy user of the git-new-worktree "hack / script", is there
 
 git-new-workdir
Sorry,


Stefan

--
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

Re: [PATCH v3 1/6] worktree: new repo extension to manage worktree behaviors

[Stefan Monnier]

> One lessor key phrase above is "so far", I think, and another one
> you forgot to use is s/which requires/that we know &/, which to me
> is a more serious one.  IOW, I do think it is premature for us to
> say that that config split issue is the only thing, or to say that
> the issue is best solved by changing the layout in the way being
> discussed; the multiple-worktree feature needs more lab experience
> for us to gain confidence.

As a heavy user of the git-new-worktree "hack / script", is there
something I can do to help "get more experience"?


Stefan

--
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

Re: [PATCH v3 4/6] worktree: new config file hierarchy

[Duy Nguyen]

On Fri, Jan 29, 2016 at 1:45 AM, Junio C Hamano  wrote:
> Would it make it simpler to invent a specific value for 'xxx' that
> denotes the main worktree (hence $C/worktrees/xxx/config will always
> be read by worktrees including the primary one), not to add
> $C/common/ anything, and use $C/config as the common one instead?
>
> Then the repository format version can live in $C/config that would
> be noticed by existing versions of Git.

I can read this in two ways. In the first way, we still have $C as a
.git _directory_ that contains no worktree stuff because those files
are in $C/worktrees/main. When we detect .git directory we need to
decide if it this is worktree v1 and redirect $GIT_DIR to
$C/worktrees/main, otherwise keep $GIT_DIR as $C. This messes up setup
code a lot (I tried).

The other way, probably a bit deviated from your intention, is, we
only support two modes: either all worktrees are in $C/worktrees
(multiple worktree mode), or there's only one worktree at .git (single
worktree mode). In other words, there's no mixing main and linked
worktrees. The user will be forced to convert the main worktree to
linked worktree when they want to add a another tree. The backward
compatibility issue with worktree v0 is gone.

The transition between two modes can be done via "git worktree move".
This command can move any worktree, including the main one. Main
worktree is converted when it's moved (.git directory remains where it
is). "worktree move" can also move repository directory, which also
automatically convert main worktree to $C/worktrees/something. If the
user deletes all worktrees except one, they can move the repo back to
worktree's root, which converts it back to the single worktree mode.

Hmm?
-- 
Duy
--
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

Re: [PATCH v3 1/6] worktree: new repo extension to manage worktree behaviors

[Duy Nguyen]

On Mon, Feb 1, 2016 at 9:41 AM, Stefan Monnier  wrote:
>> One lessor key phrase above is "so far", I think, and another one
>> you forgot to use is s/which requires/that we know &/, which to me
>> is a more serious one.  IOW, I do think it is premature for us to
>> say that that config split issue is the only thing, or to say that
>> the issue is best solved by changing the layout in the way being
>> discussed; the multiple-worktree feature needs more lab experience
>> for us to gain confidence.
>
> As a heavy user of the git-new-worktree "hack / script", is there
> something I can do to help "get more experience"?

You can try out "git worktree" command (in "lab" environment) and see
what's missing, what use cases of yours it does not support.
-- 
Duy
--
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

Re: [PATCH 4/6] checkout-index: handle "--no-index" option

[Junio C Hamano]

Jeff King  writes:

> I also reformatted the comment that violated our style
> guidelines, but I am not sure if it is all that helpful. It
> seems we also cancel "--index" with "--temp" or
> "--stage=all", but I do not know why. I left the content
> as-is, but if somebody knows enough to elaborate, it might
> be worth doing.

Isn't the --index about updating the cached stat information, to
allow us to then say "the working tree file hasn't been modified
since we wrote it out"?  After writing a file out to somewhere that
is not its natural location (i.e. using prefix, stage or temp would
all write the contents of F to somewhere that is not F), the next
"diff-files" would not compare the index entry with the contents
held in that relocated location, but with its natural location.

Admittedly, even if we update the cached stat info from that
relocated place, the next "diff-files" would certainly not match
(not just mtime but i-num would be different), but if the real
working tree file were clean when --temp checkout happened, that
would mean we would force another round of update-index --refresh,
so...

>  builtin/checkout-index.c | 34 ++
>  1 file changed, 10 insertions(+), 24 deletions(-)
>
> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> index 43bedde..f8179a7 100644
> --- a/builtin/checkout-index.c
> +++ b/builtin/checkout-index.c
> @@ -130,18 +130,6 @@ static const char * const builtin_checkout_index_usage[] 
> = {
>  
>  static struct lock_file lock_file;
>  
> -static int option_parse_u(const struct option *opt,
> -   const char *arg, int unset)
> -{
> - int *newfd = opt->value;
> -
> - state.refresh_cache = 1;
> - state.istate = _index;
> - if (*newfd < 0)
> - *newfd = hold_locked_index(_file, 1);
> - return 0;
> -}
> -
>  static int option_parse_stage(const struct option *opt,
> const char *arg, int unset)
>  {
> @@ -166,6 +154,7 @@ int cmd_checkout_index(int argc, const char **argv, const 
> char *prefix)
>   int read_from_stdin = 0;
>   int prefix_length;
>   int force = 0, quiet = 0, not_new = 0;
> + int index_opt = 0;
>   struct option builtin_checkout_index_options[] = {
>   OPT_BOOL('a', "all", ,
>   N_("check out all files in the index")),
> @@ -174,9 +163,8 @@ int cmd_checkout_index(int argc, const char **argv, const 
> char *prefix)
>   N_("no warning for existing files and files not in 
> index")),
>   OPT_BOOL('n', "no-create", _new,
>   N_("don't checkout new files")),
> - { OPTION_CALLBACK, 'u', "index", , NULL,
> - N_("update stat information in the index file"),
> - PARSE_OPT_NOARG, option_parse_u },
> + OPT_BOOL('u', "index", _opt,
> +  N_("update stat information in the index file")),
>   OPT_BOOL('z', NULL, _term_line,
>   N_("paths are separated with NUL character")),
>   OPT_BOOL(0, "stdin", _from_stdin,
> @@ -211,15 +199,13 @@ int cmd_checkout_index(int argc, const char **argv, 
> const char *prefix)
>   state.base_dir = "";
>   state.base_dir_len = strlen(state.base_dir);
>  
> - if (state.base_dir_len || to_tempfile) {
> - /* when --prefix is specified we do not
> -  * want to update cache.
> -  */
> - if (state.refresh_cache) {
> - rollback_lock_file(_file);
> - newfd = -1;
> - }
> - state.refresh_cache = 0;
> + /*
> +  * when --prefix is specified we do not want to update cache.
> +  */
> + if (index_opt && !state.base_dir_len && !to_tempfile) {
> + state.refresh_cache = 1;
> + state.istate = _index;
> + newfd = hold_locked_index(_file, 1);
>   }
>  
>   /* Check out named files first */
--
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

Re: [PATCH 4/6] checkout-index: handle "--no-index" option

[Jeff King]

On Sun, Jan 31, 2016 at 06:25:22PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I also reformatted the comment that violated our style
> > guidelines, but I am not sure if it is all that helpful. It
> > seems we also cancel "--index" with "--temp" or
> > "--stage=all", but I do not know why. I left the content
> > as-is, but if somebody knows enough to elaborate, it might
> > be worth doing.
> 
> Isn't the --index about updating the cached stat information, to
> allow us to then say "the working tree file hasn't been modified
> since we wrote it out"?  After writing a file out to somewhere that
> is not its natural location (i.e. using prefix, stage or temp would
> all write the contents of F to somewhere that is not F), the next
> "diff-files" would not compare the index entry with the contents
> held in that relocated location, but with its natural location.

Yeah, that makes sense to me. I should have said "...but I do not know
why, and I did not really look into it" in my original.

That probably makes it OK to silently ignore (as opposed to complaining
that "--prefix" is used with "--index"). It is not so much "these
options are incompatible" as the fact that there is no entry to update
in the case of a prefix or tempfile. So "--index" is still in effect, it
just has no work to do. :)

-Peff
--
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

Re: [PATCH] t6302: drop unnecessary GPG requirement

[Junio C Hamano]

Eric Sunshine  writes:

> These tests are concerned specifically with filtering, sorting,
> formatting behavior of git-for-each-ref, yet if GPG is not present, the
> entire test script is skipped even though none of the tests depend upon
> or care whether the tags are signed. This unnecessary dependency upon
> GPG may prevent these tests from being more widely run, so drop it.

It is conceivable, if not highly plausible, that a change to the
code that does the filtering and formatting can become buggy because
payload with GPG signature looks somewhat differently from what is
in an annotated but not signed tag.  Even if "these are specifically
filtering..."  is true, including tests for signed tags is valuable.

It seems that we are not currently testing with unsigned but
annotated tags, and tests that use them would be good, too.

Would it make sense to introduce a helper function specific to this
script to be used to prepare the expected output, to replace cat <<,
that goes like this?

test_prepare_expect () {
if test_have_prereq GPG
then
cat
else
sed -e '/signed/d'
fi
}

And then use it like this?

test_expect_success 'check "%(contents:lines=9)"' '
test_prepare_expect <<-\EOF &&
master |three
...
signed-tag |A signed tag message
...
EOF
git for-each-ref --format=... >actual &&
test_cmp expect actual
'

The setup part would need to make GPG conditional, e.g.

test_expect_success 'setup some history and refs' '
test_commit one &&
test_commit two &&
test_commit three &&
git checkout -b side &&
test_commit four &&
git tag -m "An annotated tag" annotated-tag &&
git tag -m "Annotated doubly" doubly-annotated-tag annotated-tag &&
if test_have_prereq GPG
then
git tag -s -m "A signed tag message" signed-tag &&
git tag -s -m "Annonated doubly" doubly-signed-tag signed-tag
fi &&
git checkout master &&
git update-ref refs/odd/spot master
'

Perhaps?
--
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

Re: [PATCH 0/6] post-strbuf_getline cleanups

[Junio C Hamano]

Jeff King  writes:

> As promised, here are the "how about this on top" patches that came out
> of the discussion for the "strbuf_getline" series in:
>
>   
> http://thread.gmane.org/gmane.comp.version-control.msysgit/21773/focus=284001

Thanks for following up.

> As I looked further at some of the option-parsing cleanups, I realized
> that some of them are more than cleanups; we're actually fixing bizarre
> behavior and segfaults.
>
>   [1/6]: give "nbuf" strbuf a more meaningful name
>   [2/6]: checkout-index: simplify "-z" option parsing
>   [3/6]: checkout-index: handle "--no-prefix" option
>   [4/6]: checkout-index: handle "--no-index" option
>   [5/6]: checkout-index: disallow "--no-stage" option
>   [6/6]: apply, ls-files: simplify "-z" parsing
>
> -Peff
--
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

Re: [PATCH v3 6/6] worktree add: switch to worktree version 1

[Max Kirillov]

On Tue, Jan 26, 2016 at 06:44:45PM +0700, Nguyễn Thái Ngọc Duy wrote:
> + for (key_p = per_wortree_keys; *key_p; key_p++) {
> + const char *key = *key_p;
> + char *value = get_key(sb.buf, key);
> +
> + if (value) {
> + if (git_config_set(key, value))
> + die(_("failed to keep %s in main 
> worktree's config file"), key);
> + if (git_config_set_in_file(sb.buf, key, NULL))
> + die(_("failed to delete %s in shared 
> config file"), key);
> + free(value);
> + }
> + }

1. For submodules (which must be left per-worktree) this
approach is not going to work, because you don't know all
variables in advance. You could scan the config file and
match those actual keys which are there with patterns.

2. This migrates variables to the default (or current?)
worktree, what about others existing?

-- 
Max
--
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] completion: verify-tag is not plumbing

[John Keeping]

According to command-list.txt, verify-tag is an ancillary interrogator,
which means that it should be completed by "git verify-" in the
same way as verify-commit.

Remove it from the list of plumbing commands so that it is treated as
porcelain and completed.

Signed-off-by: John Keeping 
---
 contrib/completion/git-completion.bash | 1 -
 1 file changed, 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 51f5223..250788a 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -728,7 +728,6 @@ __git_list_porcelain_commands ()
write-tree)   : plumbing;;
var)  : infrequent;;
verify-pack)  : infrequent;;
-   verify-tag)   : plumbing;;
*) echo $i;;
esac
done
-- 
2.7.0

--
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 5/6] checkout-index: disallow "--no-stage" option

[Jeff King]

We do not really expect people to use "--no-stage", but if
they do, git currently segfaults. We could instead have it
undo the effects of a previous "--stage", but this gets
tricky around the "to_tempfile" flag. We cannot simply reset
it to 0, because we don't know if it was set by a previous
"--stage=all" or an explicit "--temp" option.

We could solve this by setting a flag and resolving
to_tempfile later, but it's not worth the effort. Nobody
actually wants to use "--no-stage"; we are just trying to
fix a potential segfault here.

While we're in the area adding a translated string, let's
mark the other possible error message in this function as
translatable.

Signed-off-by: Jeff King 
---
 builtin/checkout-index.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index f8179a7..7a9b561 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -133,6 +133,8 @@ static struct lock_file lock_file;
 static int option_parse_stage(const struct option *opt,
  const char *arg, int unset)
 {
+   if (unset)
+   return error(_("--stage cannot be negated"));
if (!strcmp(arg, "all")) {
to_tempfile = 1;
checkout_stage = CHECKOUT_ALL;
@@ -141,7 +143,7 @@ static int option_parse_stage(const struct option *opt,
if ('1' <= ch && ch <= '3')
checkout_stage = arg[0] - '0';
else
-   die("stage should be between 1 and 3 or all");
+   die(_("stage should be between 1 and 3 or all"));
}
return 0;
 }
-- 
2.7.0.489.g6faad84

--
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 4/6] checkout-index: handle "--no-index" option

[Jeff King]

The parsing of "--index" is done in a callback, but it does
not handle an "unset" option. We don't necessarily expect
anyone to use this, but the current behavior is to treat it
exactly like "--index", which would probably be surprising.

Instead, let's just turn it into an OPT_BOOL, and handle it
after we're done parsing. This makes "--no-index" just work
(it cancels a previous "--index").

As a bonus, this makes the logic easier to follow. The old
code opened the index during the option parsing, leaving the
reader to wonder if there was some timing issue (there
isn't; none of the other options care that we've opened it).
And then if we found that "--prefix" had been given, we had
to rollback the index. Now we can simply avoid opening it in
the first place.

Note that it might make more sense for checkout-index to
complain when "--index --prefix=foo" is given (rather than
silently ignoring "--index"), but since it has been that way
since 415e96c ([PATCH] Implement git-checkout-cache -u to
update stat information in the cache., 2005-05-15), it's
safer to leave it as-is.

Signed-off-by: Jeff King 
---
I also reformatted the comment that violated our style
guidelines, but I am not sure if it is all that helpful. It
seems we also cancel "--index" with "--temp" or
"--stage=all", but I do not know why. I left the content
as-is, but if somebody knows enough to elaborate, it might
be worth doing.

 builtin/checkout-index.c | 34 ++
 1 file changed, 10 insertions(+), 24 deletions(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 43bedde..f8179a7 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -130,18 +130,6 @@ static const char * const builtin_checkout_index_usage[] = 
{
 
 static struct lock_file lock_file;
 
-static int option_parse_u(const struct option *opt,
- const char *arg, int unset)
-{
-   int *newfd = opt->value;
-
-   state.refresh_cache = 1;
-   state.istate = _index;
-   if (*newfd < 0)
-   *newfd = hold_locked_index(_file, 1);
-   return 0;
-}
-
 static int option_parse_stage(const struct option *opt,
  const char *arg, int unset)
 {
@@ -166,6 +154,7 @@ int cmd_checkout_index(int argc, const char **argv, const 
char *prefix)
int read_from_stdin = 0;
int prefix_length;
int force = 0, quiet = 0, not_new = 0;
+   int index_opt = 0;
struct option builtin_checkout_index_options[] = {
OPT_BOOL('a', "all", ,
N_("check out all files in the index")),
@@ -174,9 +163,8 @@ int cmd_checkout_index(int argc, const char **argv, const 
char *prefix)
N_("no warning for existing files and files not in 
index")),
OPT_BOOL('n', "no-create", _new,
N_("don't checkout new files")),
-   { OPTION_CALLBACK, 'u', "index", , NULL,
-   N_("update stat information in the index file"),
-   PARSE_OPT_NOARG, option_parse_u },
+   OPT_BOOL('u', "index", _opt,
+N_("update stat information in the index file")),
OPT_BOOL('z', NULL, _term_line,
N_("paths are separated with NUL character")),
OPT_BOOL(0, "stdin", _from_stdin,
@@ -211,15 +199,13 @@ int cmd_checkout_index(int argc, const char **argv, const 
char *prefix)
state.base_dir = "";
state.base_dir_len = strlen(state.base_dir);
 
-   if (state.base_dir_len || to_tempfile) {
-   /* when --prefix is specified we do not
-* want to update cache.
-*/
-   if (state.refresh_cache) {
-   rollback_lock_file(_file);
-   newfd = -1;
-   }
-   state.refresh_cache = 0;
+   /*
+* when --prefix is specified we do not want to update cache.
+*/
+   if (index_opt && !state.base_dir_len && !to_tempfile) {
+   state.refresh_cache = 1;
+   state.istate = _index;
+   newfd = hold_locked_index(_file, 1);
}
 
/* Check out named files first */
-- 
2.7.0.489.g6faad84

--
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 6/6] apply, ls-files: simplify "-z" parsing

[Jeff King]

As a short option, we cannot handle negation. Thus a
callback handling "unset" is overkill, and we can just use
OPT_SET_INT instead to handle setting the option.

Signed-off-by: Jeff King 
---
I left this one for last, because it's the most questionable. Unlike the
previous "-z" case, we're setting the actual character, so the logic is
inverted: turning on the option sets it to 0, and turning it off restore
'\n'.

This means OPT_SET_INT would do the wrong thing for the "unset" case, as
it would put a "0" into the option. You can't trigger that now, but if
somebody were to add a long option (e.g., "--nul"), then "--no-nul"
would do the wrong thing.

I'm on the fence on whether the simplification is worth it, or if we
should leave the callbacks as future-proofing.

 builtin/apply.c| 15 ++-
 builtin/ls-files.c | 13 ++---
 2 files changed, 4 insertions(+), 24 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index deb1364..565f3fd 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4464,16 +4464,6 @@ static int option_parse_p(const struct option *opt,
return 0;
 }
 
-static int option_parse_z(const struct option *opt,
- const char *arg, int unset)
-{
-   if (unset)
-   line_termination = '\n';
-   else
-   line_termination = 0;
-   return 0;
-}
-
 static int option_parse_space_change(const struct option *opt,
  const char *arg, int unset)
 {
@@ -4546,9 +4536,8 @@ int cmd_apply(int argc, const char **argv, const char 
*prefix_)
 N_( "attempt three-way merge if a patch does not 
apply")),
OPT_FILENAME(0, "build-fake-ancestor", _ancestor,
N_("build a temporary index based on embedded index 
information")),
-   { OPTION_CALLBACK, 'z', NULL, NULL, NULL,
-   N_("paths are separated with NUL character"),
-   PARSE_OPT_NOARG, option_parse_z },
+   OPT_SET_INT('z', NULL, _termination,
+   N_("paths are separated with NUL character"), '\0'),
OPT_INTEGER('C', NULL, _context,
N_("ensure at least  lines of context 
match")),
{ OPTION_CALLBACK, 0, "whitespace", _option, 
N_("action"),
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index b6a7cb0..59bad9b 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -359,14 +359,6 @@ static const char * const ls_files_usage[] = {
NULL
 };
 
-static int option_parse_z(const struct option *opt,
- const char *arg, int unset)
-{
-   line_terminator = unset ? '\n' : '\0';
-
-   return 0;
-}
-
 static int option_parse_exclude(const struct option *opt,
const char *arg, int unset)
 {
@@ -408,9 +400,8 @@ int cmd_ls_files(int argc, const char **argv, const char 
*cmd_prefix)
struct exclude_list *el;
struct string_list exclude_list = STRING_LIST_INIT_NODUP;
struct option builtin_ls_files_options[] = {
-   { OPTION_CALLBACK, 'z', NULL, NULL, NULL,
-   N_("paths are separated with NUL character"),
-   PARSE_OPT_NOARG, option_parse_z },
+   OPT_SET_INT('z', NULL, _terminator,
+   N_("paths are separated with NUL character"), '\0'),
OPT_BOOL('t', NULL, _tag,
N_("identify the file status with tags")),
OPT_BOOL('v', NULL, _valid_bit,
-- 
2.7.0.489.g6faad84
--
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

git log -g bizarre behaviour

[Dennis Kaarsemaker]

I'm attempting to understand the log [-g] / reflog code enough to
untangle them and make reflog walking work for more than just commit
objects [see gmane 283169]. I found something which I think is wrong,
and would break after my changes.

git log -g HEAD^ and git log -g v2.7.0^ give no output. This is
expected, as those are not things that have a reflog. But git log -g
v2.7.0 seems to ignore -g and gives the normal log. git reflog v2.7.0
does something even more bizarre:

$ GIT_PAGER= git reflog v2.7.0 
7548842 (tag: v2.7.0, seveas/master, origin/master, origin/HEAD) 3e9226a 
833e482 (tag: v2.6.5, gitster/maint-2.6) e3073cf e002527 e54d0f5 06b5c93 
34872f0 5863990 02103b3 503b1ef 28274d0 (tag: v2.7.0-rc3) aecb997 7195733 
e929264 ce858c0 5fa9ab8

Yes, that's a humongous line (I've only copied parts of it).

I'd like to make git log -g / git reflog abort early when trying to
display a reflog of a ref that has no reflog. Objections?

-- 
Dennis Kaarsemaker
www.kaarsemaker.net


--
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

Re: [PATCH 6/6] apply, ls-files: simplify "-z" parsing

[Johannes Schindelin]

Hi Peff,

On Sun, 31 Jan 2016, Jeff King wrote:

> I left this one for last, because it's the most questionable.

I like the simplification... ;-)

The other patches in this series look fine to me.

Ciao,
Dscho
--
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

Re: [PATCH 1/6] give "nbuf" strbuf a more meaningful name

[Jeff King]

On Sun, Jan 31, 2016 at 12:54:29PM +0100, Johannes Schindelin wrote:

> Hi Peff,
> 
> On Sun, 31 Jan 2016, Jeff King wrote:
> 
> > > It's a shame that we can't just factor out this common
> > > code, but I don't think it's quite long enough to merit
> > > the boilerplate. The interesting part of each function
> > > happens inside the loop. If C had lambdas, we could do
> > > something like:
> > > 
> > >   foreach_path_from(stdin, nul_term_line) {
> > > /* now do something interesting with "buf"
> > >and some other local variables */
> > >   }
> 
> Technically, we do not have to do lambdas for that paradigm, we could
> introduce a new data type and a reader, i.e. something like this:
> [...]
> And then the repeated code could be replaced by something like this:
> 
>   struct path_reader path_reader = PATH_READER_INIT;
> 
>   while (read_next_path(, stdin, 1)) {
>   ... [work with reader->path.buf] ...
>   }
> 
>   cleanup_path_reader();

Yeah, you're right. I was thinking of lifting the loop completely out of
the call-sites, but simplifying it to a single line loop condition is
just as good.

I still think this crosses my line of "too much boilerplate to be worth
it", though.

-Peff
--
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

Re: [PATCH 1/6] give "nbuf" strbuf a more meaningful name

[Johannes Schindelin]

Hi Peff,

On Sun, 31 Jan 2016, Jeff King wrote:

> On Sun, Jan 31, 2016 at 12:54:29PM +0100, Johannes Schindelin wrote:
> 
> > On Sun, 31 Jan 2016, Jeff King wrote:
> > 
> > > > It's a shame that we can't just factor out this common
> > > > code, but I don't think it's quite long enough to merit
> > > > the boilerplate. The interesting part of each function
> > > > happens inside the loop. If C had lambdas, we could do
> > > > something like:
> > > > 
> > > >   foreach_path_from(stdin, nul_term_line) {
> > > > /* now do something interesting with "buf"
> > > >and some other local variables */
> > > >   }
> > 
> > Technically, we do not have to do lambdas for that paradigm, we could
> > introduce a new data type and a reader, i.e. something like this:
> > [...]
> > And then the repeated code could be replaced by something like this:
> > 
> > struct path_reader path_reader = PATH_READER_INIT;
> > 
> > while (read_next_path(, stdin, 1)) {
> > ... [work with reader->path.buf] ...
> > }
> > 
> > cleanup_path_reader();
> 
> Yeah, you're right. I was thinking of lifting the loop completely out of
> the call-sites, but simplifying it to a single line loop condition is
> just as good.
> 
> I still think this crosses my line of "too much boilerplate to be worth
> it", though.

Oh, I totally agree. Just wanted to point out that there are other
options.

Ciao,
Dscho
--
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 0/6] post-strbuf_getline cleanups

[Jeff King]

As promised, here are the "how about this on top" patches that came out
of the discussion for the "strbuf_getline" series in:

  http://thread.gmane.org/gmane.comp.version-control.msysgit/21773/focus=284001

As I looked further at some of the option-parsing cleanups, I realized
that some of them are more than cleanups; we're actually fixing bizarre
behavior and segfaults.

  [1/6]: give "nbuf" strbuf a more meaningful name
  [2/6]: checkout-index: simplify "-z" option parsing
  [3/6]: checkout-index: handle "--no-prefix" option
  [4/6]: checkout-index: handle "--no-index" option
  [5/6]: checkout-index: disallow "--no-stage" option
  [6/6]: apply, ls-files: simplify "-z" parsing

-Peff
--
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 3/6] checkout-index: handle "--no-prefix" option

[Jeff King]

We use a custom callback to parse "--prefix", but it does
not handle the "unset" case. As a result, passing
"--no-prefix" will cause a segfault.

We can fix this by switching it to an OPT_STRING, which
makes "--no-prefix" counteract a previous "--prefix". Note
that this assigns NULL, so we bump our default-case
initialization to lower in the main function.

Signed-off-by: Jeff King 
---
 builtin/checkout-index.c | 18 ++
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 3b913d1..43bedde 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -142,14 +142,6 @@ static int option_parse_u(const struct option *opt,
return 0;
 }
 
-static int option_parse_prefix(const struct option *opt,
-  const char *arg, int unset)
-{
-   state.base_dir = arg;
-   state.base_dir_len = strlen(arg);
-   return 0;
-}
-
 static int option_parse_stage(const struct option *opt,
  const char *arg, int unset)
 {
@@ -191,9 +183,8 @@ int cmd_checkout_index(int argc, const char **argv, const 
char *prefix)
N_("read list of paths from the standard input")),
OPT_BOOL(0, "temp", _tempfile,
N_("write the content to temporary files")),
-   OPT_CALLBACK(0, "prefix", NULL, N_("string"),
-   N_("when creating files, prepend "),
-   option_parse_prefix),
+   OPT_STRING(0, "prefix", _dir, N_("string"),
+   N_("when creating files, prepend ")),
OPT_CALLBACK(0, "stage", NULL, NULL,
N_("copy out the files from named stage"),
option_parse_stage),
@@ -204,7 +195,6 @@ int cmd_checkout_index(int argc, const char **argv, const 
char *prefix)
usage_with_options(builtin_checkout_index_usage,
   builtin_checkout_index_options);
git_config(git_default_config, NULL);
-   state.base_dir = "";
prefix_length = prefix ? strlen(prefix) : 0;
 
if (read_cache() < 0) {
@@ -217,6 +207,10 @@ int cmd_checkout_index(int argc, const char **argv, const 
char *prefix)
state.quiet = quiet;
state.not_new = not_new;
 
+   if (!state.base_dir)
+   state.base_dir = "";
+   state.base_dir_len = strlen(state.base_dir);
+
if (state.base_dir_len || to_tempfile) {
/* when --prefix is specified we do not
 * want to update cache.
-- 
2.7.0.489.g6faad84

--
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 2/6] checkout-index: simplify "-z" option parsing

[Jeff King]

Now that we act as a simple bool, there's no need to use a
custom callback.

Signed-off-by: Jeff King 
---
 builtin/checkout-index.c | 12 ++--
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index d8d7bd3..3b913d1 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -142,13 +142,6 @@ static int option_parse_u(const struct option *opt,
return 0;
 }
 
-static int option_parse_z(const struct option *opt,
- const char *arg, int unset)
-{
-   nul_term_line = !unset;
-   return 0;
-}
-
 static int option_parse_prefix(const struct option *opt,
   const char *arg, int unset)
 {
@@ -192,9 +185,8 @@ int cmd_checkout_index(int argc, const char **argv, const 
char *prefix)
{ OPTION_CALLBACK, 'u', "index", , NULL,
N_("update stat information in the index file"),
PARSE_OPT_NOARG, option_parse_u },
-   { OPTION_CALLBACK, 'z', NULL, NULL, NULL,
-   N_("paths are separated with NUL character"),
-   PARSE_OPT_NOARG, option_parse_z },
+   OPT_BOOL('z', NULL, _term_line,
+   N_("paths are separated with NUL character")),
OPT_BOOL(0, "stdin", _from_stdin,
N_("read list of paths from the standard input")),
OPT_BOOL(0, "temp", _tempfile,
-- 
2.7.0.489.g6faad84

--
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 1/6] give "nbuf" strbuf a more meaningful name

[Jeff King]

It's a common pattern in our code to read paths from stdin,
separated either by newlines or NULs, and unquote as
necessary. In each of these five cases we use "nbuf" to
temporarily store the unquoted value. Let's give it the more
meaningful name "unquoted", which makes it easier to
understand the purpose of the variable.

While we're at it, let's also static-initialize all of our
strbufs. It's not wrong to call strbuf_init, but it
increases the cognitive load on the reader, who might wonder
"do we sometimes avoid initializing them?  why?".

Signed-off-by: Jeff King 
---
For those seeing this patch for the first time, I'll copy my
regrets from the earlier posting:

> It's a shame that we can't just factor out this common
> code, but I don't think it's quite long enough to merit
> the boilerplate. The interesting part of each function
> happens inside the loop. If C had lambdas, we could do
> something like:
> 
>   foreach_path_from(stdin, nul_term_line) {
> /* now do something interesting with "buf"
>and some other local variables */
>   }
>
> but using function pointers for this kind of iteration is
> pretty awful (define the two-line loop body as a separate
> function, stuff any local variables into a struct and pass
> it as "void *", etc). You can overcome that with macros
> and make the above code work if you don't mind creating an
> undebuggable mess. :)

 builtin/check-attr.c | 13 ++---
 builtin/check-ignore.c   | 13 ++---
 builtin/checkout-index.c | 11 ++-
 builtin/hash-object.c| 11 ++-
 builtin/update-index.c   | 11 ++-
 5 files changed, 30 insertions(+), 29 deletions(-)

diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 087325e..53a5a18 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -72,24 +72,23 @@ static void check_attr(const char *prefix, int cnt,
 static void check_attr_stdin_paths(const char *prefix, int cnt,
struct git_attr_check *check)
 {
-   struct strbuf buf, nbuf;
+   struct strbuf buf = STRBUF_INIT;
+   struct strbuf unquoted = STRBUF_INIT;
strbuf_getline_fn getline_fn;
 
getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf;
-   strbuf_init(, 0);
-   strbuf_init(, 0);
while (getline_fn(, stdin) != EOF) {
if (!nul_term_line && buf.buf[0] == '"') {
-   strbuf_reset();
-   if (unquote_c_style(, buf.buf, NULL))
+   strbuf_reset();
+   if (unquote_c_style(, buf.buf, NULL))
die("line is badly quoted");
-   strbuf_swap(, );
+   strbuf_swap(, );
}
check_attr(prefix, cnt, check, buf.buf);
maybe_flush_or_die(stdout, "attribute to stdout");
}
strbuf_release();
-   strbuf_release();
+   strbuf_release();
 }
 
 static NORETURN void error_with_usage(const char *msg)
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 4f0b09e..1d73d3c 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -115,20 +115,19 @@ static int check_ignore(struct dir_struct *dir,
 
 static int check_ignore_stdin_paths(struct dir_struct *dir, const char *prefix)
 {
-   struct strbuf buf, nbuf;
+   struct strbuf buf = STRBUF_INIT;
+   struct strbuf unquoted = STRBUF_INIT;
char *pathspec[2] = { NULL, NULL };
strbuf_getline_fn getline_fn;
int num_ignored = 0;
 
getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf;
-   strbuf_init(, 0);
-   strbuf_init(, 0);
while (getline_fn(, stdin) != EOF) {
if (!nul_term_line && buf.buf[0] == '"') {
-   strbuf_reset();
-   if (unquote_c_style(, buf.buf, NULL))
+   strbuf_reset();
+   if (unquote_c_style(, buf.buf, NULL))
die("line is badly quoted");
-   strbuf_swap(, );
+   strbuf_swap(, );
}
pathspec[0] = buf.buf;
num_ignored += check_ignore(dir, prefix,
@@ -136,7 +135,7 @@ static int check_ignore_stdin_paths(struct dir_struct *dir, 
const char *prefix)
maybe_flush_or_die(stdout, "check-ignore to stdout");
}
strbuf_release();
-   strbuf_release();
+   strbuf_release();
return num_ignored;
 }
 
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index ed888a5..d8d7bd3 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -251,7 +251,8 @@ int cmd_checkout_index(int argc, const char **argv, const 
char *prefix)
}
 
if (read_from_stdin) {
-   struct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT;
+   struct strbuf buf = STRBUF_INIT;
+   struct 

Re: [PATCH 1/6] give "nbuf" strbuf a more meaningful name

[Johannes Schindelin]

Hi Peff,

On Sun, 31 Jan 2016, Jeff King wrote:

> > It's a shame that we can't just factor out this common
> > code, but I don't think it's quite long enough to merit
> > the boilerplate. The interesting part of each function
> > happens inside the loop. If C had lambdas, we could do
> > something like:
> > 
> >   foreach_path_from(stdin, nul_term_line) {
> > /* now do something interesting with "buf"
> >and some other local variables */
> >   }

Technically, we do not have to do lambdas for that paradigm, we could
introduce a new data type and a reader, i.e. something like this:

struct path_reader {
FILE *in;
int nul_term_line;
struct strbuf path;
};
#define PATH_READER_INIT { NULL, STRBUF_INIT };

int read_next_path(struct path_reader *reader, FILE *in, int nul_term_line)
{
if (!reader->in) {
... [initialize] ...
}

... [read and possibly unquote path] ...
}

void cleanup_path_reader(struct path_reader *reader)
{
if (reader->in) {
fclose(reader->in);
reader->in = NULL;
}

strbuf_release(>buf);
}

And then the repeated code could be replaced by something like this:

struct path_reader path_reader = PATH_READER_INIT;

while (read_next_path(, stdin, 1)) {
... [work with reader->path.buf] ...
}

cleanup_path_reader();

Probably this is actually not limited to path names, so the functions
should be renamed...

(totally untested, of course...)

Ciao,
Dscho
--
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

Re: [PATCH] completion: verify-tag is not plumbing

[SZEDER Gábor]

Quoting John Keeping :


According to command-list.txt, verify-tag is an ancillary interrogator,
which means that it should be completed by "git verify-" in the
same way as verify-commit.

Remove it from the list of plumbing commands so that it is treated as
porcelain and completed.


I'm not sure.  There are commands among the ancillary interrogators  
that are basically porcelains (e.g. blame), while some are more like  
plumbing (e.g. rerere, rev-parse).  In general the completion script  
supports the former but not the latter commands.


Now, the real porcelain-ish way to verify a tag is via 'git tag  
-v|--verify', and according to a925c6f165a3 (bash: Classify more  
commends out of completion., 2007-02-04), the commit removing  
verify-tag from the completed commands, verify-tag was kept around for  
backwards compatibility reasons.  OTOH verify-commit was introduced in  
d07b00b7f31d (verify-commit: scriptable commit signature verification,  
2014-06-23), and as the subject line states it was intended more as a  
plumbing command.


So I think we should keep excluding verify-tag from the list of  
porcelain commands in the completion script, and it was an oversight  
not to exclude verify-commit as well when it was introduced.



Gábor


Signed-off-by: John Keeping 
---
 contrib/completion/git-completion.bash | 1 -
 1 file changed, 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash  
b/contrib/completion/git-completion.bash

index 51f5223..250788a 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -728,7 +728,6 @@ __git_list_porcelain_commands ()
write-tree)   : plumbing;;
var)  : infrequent;;
verify-pack)  : infrequent;;
-   verify-tag)   : plumbing;;
*) echo $i;;
esac
done
--
2.7.0

--
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

Re: [bug] Possible Windows 'git mv' bug

[Torsten Bögershausen]

On 2016-01-31 15.03, Aaron Gray wrote:
> Hi,
> 
> I think I have found a possible difference in behaviour between
> Windows git commandline distro and Linux git
> 
> basically If I do a :-
> 
> git mv logger.h Logger.h
> 
> I get the following :-
> 
> fatal: destination exists, source=lib/logger.h, destination=lib/Logger.h
> 
> It looks and smells like a bug to me !
Which version of Git are you using ?
Because it is fixed in the latest version in Git and Git for Windows.

--
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

Re: [PATCH 2/1] support -4 and -6 switches for remote operations

[Torsten Bögershausen]

On 2016-01-31 01.01, Eric Wong wrote:
> Torsten Bögershausen  wrote:
>> On 2016-01-30 14.13, Eric Wong wrote:
>>> The ssh(1) command has an equivalent switches which we may
>>> pass when we run them.
> 
>> Should we mention that putty and tortoiseplink don't have these options ?
>> At least in the commit message ?

I may need to take that back;
Just did a test with putty (under Debian)

And both -4 and -6 are supported, nice.

I couldn't find it first, but here seems to be the latest documentation,
which mentions -4 and -6.
http://the.earth.li/~sgtatham/putty/0.66/puttydoc.txt

And even plink acceptes -4 and -6 (again under Debian)

Sorry for the noise.
--
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

Re: [PATCH v3 1/6] worktree: new repo extension to manage worktree behaviors

[Junio C Hamano]

Max Kirillov  writes:

> The worktree feature has been used by several people
> already (me included), and do far the only issue which
> requires change in repository layout is the config
> separation. Isn't it enough to be confident?

One lessor key phrase above is "so far", I think, and another one
you forgot to use is s/which requires/that we know &/, which to me
is a more serious one.  IOW, I do think it is premature for us to
say that that config split issue is the only thing, or to say that
the issue is best solved by changing the layout in the way being
discussed; the multiple-worktree feature needs more lab experience
for us to gain confidence.
--
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

Re: [bug] Possible Windows 'git mv' bug

[Johannes Sixt]

Am 31.01.2016 um 15:03 schrieb Aaron Gray:

Hi,

I think I have found a possible difference in behaviour between
Windows git commandline distro and Linux git

basically If I do a :-

 git mv logger.h Logger.h

I get the following :-

 fatal: destination exists, source=lib/logger.h, destination=lib/Logger.h

It looks and smells like a bug to me !


Not really. When you attempt to overwrite an existing file with 'git 
mv', you get this error message on both Windows and Linux.


The difference is that logger.h and Logger.h are the same file on 
Windows, but they are not on Linux. Hence, when you attempt to overwrite 
Logger.h on Windows, you see the error because it exists already (as 
logger.h).


As a work-around, you can use -f.

-- Hannes

--
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

Re: [bug] Possible Windows 'git mv' bug

[Doug Kelly]

On Sun, Jan 31, 2016 at 8:50 AM, Johannes Sixt  wrote:
> Am 31.01.2016 um 15:03 schrieb Aaron Gray:
>>
>> Hi,
>>
>> I think I have found a possible difference in behaviour between
>> Windows git commandline distro and Linux git
>>
>> basically If I do a :-
>>
>>  git mv logger.h Logger.h
>>
>> I get the following :-
>>
>>  fatal: destination exists, source=lib/logger.h,
>> destination=lib/Logger.h
>>
>> It looks and smells like a bug to me !
>
>
> Not really. When you attempt to overwrite an existing file with 'git mv',
> you get this error message on both Windows and Linux.
>
> The difference is that logger.h and Logger.h are the same file on Windows,
> but they are not on Linux. Hence, when you attempt to overwrite Logger.h on
> Windows, you see the error because it exists already (as logger.h).
>
> As a work-around, you can use -f.
>
> -- Hannes

Indeed.  And just to clarify, you'll get the same issue on OS X, where
the filesystem is also case-preserving, not case-sensitive (by
default, at least).  I've never tried using -f for this, but I'll
usually use git mv twice to achieve the same result.  Annoying, but
that way my local directory looks correct, too.
--
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

Re: [bug] Possible Windows 'git mv' bug

[Aaron Gray]

On 31 January 2016 at 15:05, Doug Kelly  wrote:
> On Sun, Jan 31, 2016 at 8:50 AM, Johannes Sixt  wrote:
>> Am 31.01.2016 um 15:03 schrieb Aaron Gray:
>>> I think I have found a possible difference in behaviour between
>>> Windows git commandline distro and Linux git
>>>
>>> basically If I do a :-
>>>
>>>  git mv logger.h Logger.h
>>>
>>> I get the following :-
>>>
>>>  fatal: destination exists, source=lib/logger.h,
>>> destination=lib/Logger.h
>>>
>
> Indeed.  And just to clarify, you'll get the same issue on OS X, where
> the filesystem is also case-preserving, not case-sensitive (by
> default, at least).  I've never tried using -f for this, but I'll
> usually use git mv twice to achieve the same result.  Annoying, but
> that way my local directory looks correct, too.

Ah, double up via a temporary name, cool hack !

Aaron
--
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

Re: [bug] Possible Windows 'git mv' bug

[Aaron Gray]

On 31 January 2016 at 14:50, Johannes Sixt  wrote:
> Am 31.01.2016 um 15:03 schrieb Aaron Gray:
>>
>> Hi,
>>
>> I think I have found a possible difference in behaviour between
>> Windows git commandline distro and Linux git
>>
>> basically If I do a :-
>>
>>  git mv logger.h Logger.h
>>
>> I get the following :-
>>
>>  fatal: destination exists, source=lib/logger.h,
>> destination=lib/Logger.h
>>
>> It looks and smells like a bug to me !
>
>
> Not really. When you attempt to overwrite an existing file with 'git mv',
> you get this error message on both Windows and Linux.
>
> The difference is that logger.h and Logger.h are the same file on Windows,
> but they are not on Linux. Hence, when you attempt to overwrite Logger.h on
> Windows, you see the error because it exists already (as logger.h).
>
> As a work-around, you can use -f.

Thanks Hannes !

Still a bug though IMHO

Aaron
--
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

[bug] Possible Windows 'git mv' bug

[Aaron Gray]

Hi,

I think I have found a possible difference in behaviour between
Windows git commandline distro and Linux git

basically If I do a :-

git mv logger.h Logger.h

I get the following :-

fatal: destination exists, source=lib/logger.h, destination=lib/Logger.h

It looks and smells like a bug to me !

Regards,

Aaron
--
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