Re: [bug] 2.5.0 build with NO_PERL is broken
On Fri, Aug 14, 2015 at 6:22 PM, Junio C Hamano wrote: > Eric Sunshine writes: > >> On Fri, Aug 14, 2015 at 5:02 PM, Junio C Hamano wrote: >>> Eric Sunshine writes: >>>> Nevertheless, there's still the problem, due to 527ec39 >>>> (generate-cmdlist: parse common group commands, 2015-05-21), that git >>>> doesn't build at all anymore when Perl is unavailable. >>> >>> I do not think that is anything new. We always have assumed "some" >>> version of Perl available in order to run t/ scripts. >> >> True, but prior to 527ec39, without Perl available, git itself could >> at least be built and used (with some commands unavailable), even if >> it couldn't be fully tested. As of 527ec39, however, git won't even >> build because common-cmds.h can't be generated. > > I wouldn't bother digging in the history myself, but I am reasonably > sure that the current genereate-common-cmds is not the sole instance > that we relied on Perl to build (not test) in the past, and that is > another reason why I do not think this is anything new. Hmm. In my tests by setting PERL_PATH to a bogus (non-existent) command, prior to 527ec39, git builds successfully, whereas, following 527ec39, it does not build. But, perhaps I overlooked something...(?) -- 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 v11 03/13] ref-filter: introduce ref_formatting_state
On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak wrote:
> Introduce ref_formatting_state which will hold the formatted
> output strbuf and is used for nesting of modifier atoms.
>
> Signed-off-by: Karthik Nayak
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index edfb1c7..3259363 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -55,6 +55,12 @@ static struct {
> { "color" },
> };
>
> +struct ref_formatting_state {
> + struct strbuf output;
> + struct ref_formatting_state *prev;
Upon initial read-through of this patch, I found the name 'prev'
confusing since it seems you sometimes treat this as a linked-list
and, for a linked-list, this member is customarily named 'next'.
However, you also sometimes treat it as a stack, in which case 'prev'
makes a certain amount of sense semantically, although so does 'next'.
I'd probably have named it 'next', however, attr.c:struct attr_stack
names its member 'prev', so there is some precedence for the current
choice.
Also, it's customary for this member to be the first (or less
frequently last) member in the structure.
> + int quote_style;
> +};
> +
> struct atom_value {
> const char *s;
> unsigned long ul; /* used for sorting when not FIELD_STR */
> @@ -129,6 +135,26 @@ int parse_ref_filter_atom(const char *atom, const char
> *ep)
> return at;
> }
>
> +static struct ref_formatting_state *push_new_state(struct
> ref_formatting_state **state)
This interface seems confused. The caller receives the new state as
both the return value of the function and as an out-value of its sole
argument. I'd suggest choosing one or the other.
Which one you choose depends upon how you view the operation and the
data structure. If you view it as linked-list manipulation, then you'd
probably want the new state returned, and accept a pointer argument
(rather than pointer-to-pointer). On the other hand, if you view it as
a stack, then you'd probably want to have it return void and
manipulate the sole argument. For this case, it might be clearer to
name the argument 'stack' rather than 'state'.
> +{
> + struct ref_formatting_state *new_state = xcalloc(1, sizeof(struct
> ref_formatting_state));
> + struct ref_formatting_state *tmp = *state;
> +
> + *state = new_state;
> + new_state->prev = tmp;
> + return new_state;
> +}
A couple issues:
First, you need to initialize the strbuf member of
ref_formatting_state after allocation:
new_state = xcalloc(1, sizeof(struct ref_formatting_state));
strbuf_init(&new_state->output, 0);
Second, if you re-order the code slightly, the 'tmp' variable becomes
unnecessary.
Assuming that your intention was to match pop_state() and treat this
opaquely as a stack rather than exposing its linked-list
implementation, I'd have expected the function to look something like
this:
static void push_new_state(struct ref_formatting_state **stack)
{
struct ref_formatting_state *s = xcalloc(...);
s->next = *stack;
strbuf_init(&s->output, 0);
*stack = s;
}
> +static void pop_state(struct ref_formatting_state **state)
> +{
> + struct ref_formatting_state *current = *state;
> + struct ref_formatting_state *prev = current->prev;
> +
> + strbuf_release(¤t->output);
> + free(current);
> + *state = prev;
> +}
This interface suggests that you're treating it as an opaque stack, in
which case naming the argument 'stack' might be clearer.
> /*
> * In a format string, find the next occurrence of %(atom).
> */
> @@ -1195,23 +1221,23 @@ void ref_array_sort(struct ref_sorting *sorting,
> struct ref_array *array)
> qsort(array->items, array->nr, sizeof(struct ref_array_item *),
> compare_refs);
> }
>
> -static void append_atom(struct atom_value *v, int quote_style, struct strbuf
> *output)
> +static void append_atom(struct atom_value *v, struct ref_formatting_state
> *state)
> {
> - switch (quote_style) {
> + switch (state->quote_style) {
> case QUOTE_NONE:
> - strbuf_addstr(output, v->s);
> + strbuf_addstr(&state->output, v->s);
> break;
> case QUOTE_SHELL:
> - sq_quote_buf(output, v->s);
> + sq_quote_buf(&state->output, v->s);
> break;
> case QUOTE_PERL:
> - perl_quote_buf(output, v->s);
> + perl_quote_buf(&state->output, v->s);
> break;
> case QUOTE_PYTHON:
> - python_quote_buf(output, v->s);
> + python_quote_buf(&state->output, v->s);
> break;
> case QUOTE_TCL:
> - tcl_quote_buf(output, v->s);
> + tcl_quote_buf(&state->output, v->s);
> break;
> }
> }
This patch touches all the same lines as the previous patch which
converted the code to append to a strbuf rather than emit to stdout,
thus it makes the previous
Re: [PATCH v11 04/13] utf8: add function to align a string into given strbuf
On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak wrote:
> Add strbuf_utf8_align() which will align a given string into a strbuf
> as per given align_type and width. If the width is greater than the
> string length then no alignment is performed.
A couple minor comments below...
> Signed-off-by: Karthik Nayak
> ---
> diff --git a/utf8.c b/utf8.c
> index 28e6d76..0fb8e9d 100644
> --- a/utf8.c
> +++ b/utf8.c
> @@ -644,3 +644,24 @@ int skip_utf8_bom(char **text, size_t len)
> *text += strlen(utf8_bom);
> return 1;
> }
> +
> +void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int
> width,
> + const char *s)
> +{
> + int slen = strlen(s);
> + int display_len = utf8_strnwidth(s, slen, 0);
> + int utf8_compensation = slen - display_len;
Based upon the previous round review, I think you had intended to name
this merely 'compensation'.
> + if (display_len >= width) {
> + strbuf_addstr(buf, s);
> + return;
> + }
> +
> + if (position == ALIGN_LEFT)
> + strbuf_addf(buf, "%-*s", width + utf8_compensation, s);
> + else if (position == ALIGN_MIDDLE) {
> + int left = (width - display_len)/2;
Style: spaces around '/'
> + strbuf_addf(buf, "%*s%-*s", left, "", width - left +
> utf8_compensation, s);
> + } else if (position == ALIGN_RIGHT)
> + strbuf_addf(buf, "%*s", width + utf8_compensation, s);
> +}
> diff --git a/utf8.h b/utf8.h
> index 5a9e94b..7930b44 100644
> --- a/utf8.h
> +++ b/utf8.h
> @@ -55,4 +55,19 @@ int mbs_chrlen(const char **text, size_t *remainder_p,
> const char *encoding);
> */
> int is_hfs_dotgit(const char *path);
>
> +typedef enum {
> + ALIGN_LEFT,
> + ALIGN_MIDDLE,
> + ALIGN_RIGHT
> +} align_type;
> +
> +/*
> + * Align the string given and store it into a strbuf as per the
> + * 'position' and 'width'. If the given string length is larger than
> + * 'width' than then the input string is not truncated and no
> + * alignment is done.
> + */
> +void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int
> width,
> + const char *s);
> +
> #endif
> --
> 2.5.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 v11 05/13] ref-filter: implement an `align` atom
On Sun, Aug 16, 2015 at 8:04 AM, Andreas Schwab wrote: > Karthik Nayak writes: >> I think we need to squash this in >> >> diff --git a/Documentation/git-for-each-ref.txt >> b/Documentation/git-for-each-ref.txt >> index 3099631..17bd15e 100644 >> --- a/Documentation/git-for-each-ref.txt >> +++ b/Documentation/git-for-each-ref.txt >> @@ -129,7 +129,7 @@ color:: >> >> align:: >> left-, middle-, or right-align the content between %(align:..) >> - and %(end). Followed by `:,`, where the >> + and %(end). Followed by `:>,`, where the > > s/>>/>/ Also: s/left-/Left-/ -- 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 v11 05/13] ref-filter: implement an `align` atom
On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak wrote:
> Implement an `align` atom which left-, middle-, or right-aligns the
> content between %(align:..) and %(end).
>
> It is followed by `:,`, where the `` is
> either left, right or middle and `` is the size of the area
> into which the content will be placed. If the content between
> %(align:) and %(end) is more than the width then no alignment is
> performed. e.g. to align a refname atom to the middle with a total
> width of 40 we can do: --format="%(align:middle,40)%(refname)%(end)".
>
> This is done by calling the strbuf_utf8_align() function in utf8.c.
>
> Add documentation and tests for the same.
>
> Signed-off-by: Karthik Nayak
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index 3259363..eac99d0 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -10,6 +10,7 @@
> #include "quote.h"
> #include "ref-filter.h"
> #include "revision.h"
> +#include "utf8.h"
>
> typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>
> @@ -53,16 +54,27 @@ static struct {
> { "flag" },
> { "HEAD" },
> { "color" },
> + { "align" },
> + { "end" },
> +};
> +
> +struct align {
> + align_type position;
> + unsigned int width;
> };
>
> struct ref_formatting_state {
> struct strbuf output;
> struct ref_formatting_state *prev;
> + void (*attend)(struct ref_formatting_state *state);
Junio's suggestion for this member was "at end"; that is what to do
when you are "at"-the-%(end), not "attend", which isn't meaningful
here. You could also call it 'end_scope', 'finish' or 'close' or
'finalize' or something.
> + void *cb_data;
> int quote_style;
> };
>
> struct atom_value {
> const char *s;
> + struct align *align;
> + void (*handler)(struct atom_value *atomv, struct ref_formatting_state
> **state);
> unsigned long ul; /* used for sorting when not FIELD_STR */
> };
>
> @@ -137,12 +149,12 @@ int parse_ref_filter_atom(const char *atom, const char
> *ep)
>
> static struct ref_formatting_state *push_new_state(struct
> ref_formatting_state **state)
> {
> - struct ref_formatting_state *new_state = xcalloc(1, sizeof(struct
> ref_formatting_state));
> - struct ref_formatting_state *tmp = *state;
> + struct ref_formatting_state *new = xcalloc(1, sizeof(struct
> ref_formatting_state));
> + struct ref_formatting_state *old = *state;
>
> - *state = new_state;
> - new_state->prev = tmp;
> - return new_state;
> + *state = new;
> + new->prev = old;
> + return new;
> }
What are these changes about? They appear only to be renaming some
variables which were introduced in patch 3. It would make more sense
to give them the desired names in the patch which introduces them.
> static void pop_state(struct ref_formatting_state **state)
> @@ -625,6 +637,34 @@ static inline char *copy_advance(char *dst, const char
> *src)
> return dst;
> }
>
> +static void align_handler(struct ref_formatting_state *state)
The names 'align_handler' and 'align_atom_handler' are confusingly
similar. Perhaps name this end_align() or do_align() or
apply_alignment() or something?
> +{
> + struct strbuf aligned = STRBUF_INIT;
> + struct ref_formatting_state *return_to = state->prev;
> + struct align *align = (struct align *)state->cb_data;
> +
> + strbuf_utf8_align(&aligned, align->position, align->width,
> state->output.buf);
> + strbuf_addbuf(&return_to->output, &aligned);
A couple comments:
First, why is 'strbuf aligned' needed? Can't you instead just invoke
strbuf_utf8_align(&return_to->output, ...)?
Second, I realize that Junio suggested the 'return_to' idea, but it
seems like it could become overly painful since each handler of this
sort is going to have to perform the same manipulation to append its
collected output to its parent state's output. What if you instead
make it the responsibility of pop_state() to append the 'output' from
the state being popped to the "prev" state's 'output'? This way, it
happens automatically, thus reducing code in each individual handler,
and reducing the burden of having to keep writing the same code.
> + strbuf_release(&aligned);
> +}
> +
> +static void align_atom_handler(struct atom_value *atomv, struct
> ref_formatting_state **state)
> +{
> + struct ref_formatting_state *new = push_new_state(state);
> + strbuf_init(&new->output, 0);
I think this strbuf_init() should be the responsibility of
push_new_state(), as mentioned in my patch 3 review, otherwise every
caller of push_new_state() will have to remember to do this.
> + new->attend = align_handler;
> + new->cb_data = atomv->align;
> +}
> +
> +static void end_atom_handler(struct atom_value *atomv, struct
> ref_formatting_state **state)
> +{
> + struct ref_formatting_state *current = *state;
> + if (!current->attend)
> + die(_("form
Re: [PATCH v11 06/13] ref-filter: add option to filter out tags, branches and remotes
On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak wrote:
> Add a function called 'for_each_reftype_fullpath()' to refs.{c,h}
> which iterates through each ref for the given path without trimming
> the path and also accounting for broken refs, if mentioned.
>
> Add 'filter_ref_kind()' in ref-filter.c to check the kind of ref being
> handled and return the kind to 'ref_filter_handler()', where we
> discard refs which we do not need and assign the kind to needed refs.
>
> Signed-off-by: Karthik Nayak
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index eac99d0..abcd235 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1226,16 +1262,29 @@ int filter_refs(struct ref_array *array, struct
> ref_filter *filter, unsigned int
> {
> struct ref_filter_cbdata ref_cbdata;
> int ret = 0;
> + unsigned int broken = 0;
>
> ref_cbdata.array = array;
> ref_cbdata.filter = filter;
>
> /* Simple per-ref filtering */
> - if (type & (FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN))
> - ret = for_each_rawref(ref_filter_handler, &ref_cbdata);
> - else if (type & FILTER_REFS_ALL)
> - ret = for_each_ref(ref_filter_handler, &ref_cbdata);
> - else if (type)
> + if (type & FILTER_REFS_INCLUDE_BROKEN) {
> + type -= FILTER_REFS_INCLUDE_BROKEN;
The above is a somewhat unusual way to say the more idiomatic:
type &= ~FILTER_REFS_INCLUDE_BROKEN;
when dealing with bit flags. Is there precedence elsewhere in the
project for choosing '-' over '~'?
> + broken = 1;
> + }
> +
> + filter->kind = type;
> + if (type == FILTER_REFS_BRANCHES)
> + ret = for_each_reftype_fullpath(ref_filter_handler,
> "refs/heads/", broken, &ref_cbdata);
> + else if (type == FILTER_REFS_REMOTES)
> + ret = for_each_reftype_fullpath(ref_filter_handler,
> "refs/remotes/", broken, &ref_cbdata);
> + else if (type == FILTER_REFS_TAGS)
> + ret = for_each_reftype_fullpath(ref_filter_handler,
> "refs/tags/", broken, &ref_cbdata);
> + else if (type & FILTER_REFS_ALL) {
> + ret = for_each_reftype_fullpath(ref_filter_handler, "",
> broken, &ref_cbdata);
These cases are all the same except for the (string) second argument,
aren't they? This might be less noisy and easier to follow if you
assign the appropriate string to a variable first, and then invoke
for_each_reftype_fullpath() once with the string variable as an
argument.
> + if (type & FILTER_REFS_DETACHED_HEAD)
> + head_ref(ref_filter_handler, &ref_cbdata);
> + } else
> die("filter_refs: invalid type");
>
> /* Filters that need revision walking */
> diff --git a/ref-filter.h b/ref-filter.h
> index 144a633..64fedd3 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -14,8 +14,14 @@
> -#define FILTER_REFS_INCLUDE_BROKEN 0x1
> -#define FILTER_REFS_ALL 0x2
> +#define FILTER_REFS_INCLUDE_BROKEN 0x0001
> +#define FILTER_REFS_TAGS 0x0002
> +#define FILTER_REFS_BRANCHES 0x0004
> +#define FILTER_REFS_REMOTES0x0008
> +#define FILTER_REFS_OTHERS 0x0010
> +#define FILTER_REFS_ALL(FILTER_REFS_TAGS | FILTER_REFS_BRANCHES
> | \
> + FILTER_REFS_REMOTES | FILTER_REFS_OTHERS)
> +#define FILTER_REFS_DETACHED_HEAD 0x0020
I suppose there's some good reason that FILTER_REFS_DETACHED_HEAD is
not a member of FILTER_REFS_ALL? Perhaps add a comment explaining it?
> diff --git a/refs.c b/refs.c
> index 2db2975..0f18c34 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2145,6 +2145,13 @@ int for_each_replace_ref(each_ref_fn fn, void *cb_data)
>strlen(git_replace_ref_base), 0, cb_data);
> }
>
> +int for_each_reftype_fullpath(each_ref_fn fn, char *type, unsigned int
> broken, void *cb_data)
> +{
> + if (broken)
> + broken = DO_FOR_EACH_INCLUDE_BROKEN;
It's a bit ugly and confusing to have the same variable, 'broken', act
as both a boolean input argument and as a bit flag argument to the
called function.
> + return do_for_each_ref(&ref_cache, type, fn, 0, broken, cb_data);
> +}
> +
> int head_ref_namespaced(each_ref_fn fn, void *cb_data)
> {
> struct strbuf buf = STRBUF_INIT;
--
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 v11 08/13] ref-filter: add support to sort by version
On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak wrote: > Add support to sort by version using the "v:refname" and > "version:refname" option. This is achieved by using the 'versioncmp()' > function as the comparing function for qsort. > > This option is included to support sorting by versions in `git tag -l` > which will eventaully be ported to use ref-filter APIs. s/eventaully/eventually/ > Add documentation and tests for the same. > > Signed-off-by: Karthik Nayak -- 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 v11 13/13] tag.c: implement '--merged' and '--no-merged' options
On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak wrote: > Using 'ref-filter' APIs implement the '--merged' and '--no-merged' s/implement/to implement/ > options into 'tag.c'. The '--merged' option lets the user to only > list tags merged into the named commit. The '--no-merged' option > lets the user to only list tags not merged into the named commit. > If no object is provided it assumes HEAD as the object. > > Add documentation and tests for the same. > > Signed-off-by: Karthik Nayak -- 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 v11 06/13] ref-filter: add option to filter out tags, branches and remotes
On Mon, Aug 17, 2015 at 12:42 AM, Eric Sunshine wrote:
> On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak wrote:
>> - if (type & (FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN))
>> - ret = for_each_rawref(ref_filter_handler, &ref_cbdata);
>> - else if (type & FILTER_REFS_ALL)
>> - ret = for_each_ref(ref_filter_handler, &ref_cbdata);
>> - else if (type)
>> + if (type & FILTER_REFS_INCLUDE_BROKEN) {
>> + type -= FILTER_REFS_INCLUDE_BROKEN;
>
> The above is a somewhat unusual way to say the more idiomatic:
>
> type &= ~FILTER_REFS_INCLUDE_BROKEN;
>
> when dealing with bit flags. Is there precedence elsewhere in the
> project for choosing '-' over '~'?
s/precedence/precedent/
--
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 v11 03/13] ref-filter: introduce ref_formatting_state
On Sun, Aug 16, 2015 at 7:31 PM, Eric Sunshine wrote:
> On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak wrote:
>> +struct ref_formatting_state {
>> + struct strbuf output;
>> + struct ref_formatting_state *prev;
>
> Upon initial read-through of this patch, I found the name 'prev'
> confusing since it seems you sometimes treat this as a linked-list
> and, for a linked-list, this member is customarily named 'next'.
> However, you also sometimes treat it as a stack, in which case 'prev'
> makes a certain amount of sense semantically, although so does 'next'.
> I'd probably have named it 'next', however, attr.c:struct attr_stack
> names its member 'prev', so there is some precedence for the current
> choice.
s/precedence/precedent/
--
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 v11 05/13] ref-filter: implement an `align` atom
On Mon, Aug 17, 2015 at 10:28 AM, Karthik Nayak wrote:
> On Mon, Aug 17, 2015 at 7:37 AM, Eric Sunshine
> wrote:
>> On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak wrote:
>>> +{
>>> + struct strbuf aligned = STRBUF_INIT;
>>> + struct ref_formatting_state *return_to = state->prev;
>>> + struct align *align = (struct align *)state->cb_data;
>>> +
>>> + strbuf_utf8_align(&aligned, align->position, align->width,
>>> state->output.buf);
>>> + strbuf_addbuf(&return_to->output, &aligned);
>>
>> Second, I realize that Junio suggested the 'return_to' idea, but it
>> seems like it could become overly painful since each handler of this
>> sort is going to have to perform the same manipulation to append its
>> collected output to its parent state's output. What if you instead
>> make it the responsibility of pop_state() to append the 'output' from
>> the state being popped to the "prev" state's 'output'? This way, it
>> happens automatically, thus reducing code in each individual handler,
>> and reducing the burden of having to keep writing the same code.
>
> Good question, what if we don't want to append to strbuf at all?
> For e.g., We were discussing an "%(if).%(then)..%(end)"
> atom structure, here if the if condition isn't met we wouldn't want to
> append to the prev strbuf, hence I thought it's better if the handler
> decided whether or not to append to prev strbuf.
An %(if)/%(then)/%(end) construct should not present a problem. As
long as the processing of the conditional ensures that the current
state's 'output' contains no content, when pop_state() appends that
(empty) content to the parent state's 'output', then the net result is
exactly the desired behavior.
The question of "how" the conditional processing ensures that the
current state's 'output' is empty when the %(if) condition is false is
unimportant (for this discussion). It might be the case that it just
doesn't collect any content at all for a false condition, or it
collects it but throws it away before the state is popped. Either way,
that's an implementation detail which needn't impact the decision to
retire 'return_to' and instead make pop_state() responsible for
appending the current state's output to the parent state's.
--
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] 2.5.0 build with NO_PERL is broken
On Tue, Aug 18, 2015 at 7:05 AM, Renato Botelho wrote: > Eric Sunshine sunshineco.com> writes: >> On Fri, Aug 14, 2015 at 6:22 PM, Junio C Hamano pobox.com> > wrote: >> > Eric Sunshine sunshineco.com> writes: >> >> On Fri, Aug 14, 2015 at 5:02 PM, Junio C Hamano > pobox.com> wrote: >> >>> Eric Sunshine sunshineco.com> writes: >> >>> I do not think that is anything new. We always have assumed "some" >> >>> version of Perl available in order to run t/ scripts. >> >> >> >> True, but prior to 527ec39, without Perl available, git itself could >> >> at least be built and used (with some commands unavailable), even if >> >> it couldn't be fully tested. As of 527ec39, however, git won't even >> >> build because common-cmds.h can't be generated. >> > >> > I wouldn't bother digging in the history myself, but I am reasonably >> > sure that the current genereate-common-cmds is not the sole instance >> > that we relied on Perl to build (not test) in the past, and that is >> > another reason why I do not think this is anything new. >> >> Hmm. In my tests by setting PERL_PATH to a bogus (non-existent) >> command, prior to 527ec39, git builds successfully, whereas, following >> 527ec39, it does not build. But, perhaps I overlooked something...(?) > > It builds but there will be at least 3 commands that won't work: Hmm, I was under the impression from your initial mail[1] that Git wouldn't even build without Perl available: /bin/sh: /usr/bin/perl: not found Makefile:1701: recipe for target 'common-cmds.h' failed gmake[2]: *** [common-cmds.h] Error 127 Doesn't this failure prevent generation of the 'git' executable altogether? > git-submodule > git-request-pull > git-am Also... git-add--interactive git-archimport git-cvsexportcommit git-cvsimport git-cvsserver git-difftool git-instaweb git-relink git-send-email git-svn A C rewrite of git-am has recently graduated to 'master'. > I'm considering to add perl dependency as mandatory on FreeBSD ports tree, > and maybe this NO_PERL option doesn't make more sense nowadays... That might make sense. Although some of the above commands may not be used widely, others, such as git-send-email, probably are used regularly. [1]: http://thread.gmane.org/gmane.comp.version-control.git/275905 -- 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 v12 00/13] port tag.c to use ref-filter.c APIs
On Tue, Aug 18, 2015 at 2:37 PM, Karthik Nayak wrote: > Version 11 was posted here: > http://article.gmane.org/gmane.comp.version-control.git/275997 > > Changes in this version: > * Small style and formatting changes. > * Remove unnecessary variable from push_new_state(). > * pop_state doesn't return a value now and attaches the buffer > into the previous state. > * use strcmp() rather than starts_with() while checking for > alignment position. > * Change attend to at_end > > Interdiff: > > diff --git a/Documentation/git-for-each-ref.txt > b/Documentation/git-for-each-ref.txt > index 3099631..760d719 100644 > --- a/Documentation/git-for-each-ref.txt > +++ b/Documentation/git-for-each-ref.txt > @@ -128,8 +128,8 @@ color:: > are described in `color.branch.*`. > > align:: > - left-, middle-, or right-align the content between %(align:..) > - and %(end). Followed by `:,`, where the > + Left-, middle-, or right-align the content between %(align:..) > + and %(end). Followed by `:>,`, where the I haven't had a chance to look closely at this version yet, but this popped out while quickly scanning the interdiff since the previous round had the same sort of problem: s/>>/>/ > `` is either left, right or middle and `` is > the total length of the content with alignment. If the > contents length is more than the width then no alignment is -- 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] generate-cmdlist: re-implement as shell script
527ec39 (generate-cmdlist: parse common group commands, 2015-05-21)
replaced generate-cmdlist.sh with a more functional Perl version,
generate-cmdlist.perl. The Perl version gleans named tags from a new
"common groups" section in command-list.txt and recognizes those tags in
"command list" section entries in place of the old 'common' tag. This
allows git-help to, not only recognize, but also group common commands.
Although the tests require Perl, 527ec39 creates an unconditional
dependence upon Perl in the build system itself, which can not be
overridden with NO_PERL. Such a dependency may be undesirable; for
instance, the 'git-lite' package in the FreeBSD ports tree is intended
as a minimal Git installation (which may, for example, be useful on
servers needing only local clone and update capability), which,
historically, has not depended upon Perl[1].
Therefore, revive generate-cmdlist.sh and extend it to recognize "common
groups" and its named tags. Retire generate-cmdlist.perl.
[1]: http://thread.gmane.org/gmane.comp.version-control.git/275905/focus=276132
Signed-off-by: Eric Sunshine
---
In addition to reviving 527ec39^:generate-cmdlist.sh and extending it, I
also re-indented it with tabs instead of spaces, so it's helpful to
ignore whitespace changes when comparing the old and new versions of the
shell script.
Makefile | 4 ++--
generate-cmdlist.perl | 50 --
generate-cmdlist.sh | 50 ++
3 files changed, 52 insertions(+), 52 deletions(-)
delete mode 100755 generate-cmdlist.perl
create mode 100755 generate-cmdlist.sh
diff --git a/Makefile b/Makefile
index e39ca6c..ddfe7a1 100644
--- a/Makefile
+++ b/Makefile
@@ -1697,10 +1697,10 @@ $(BUILT_INS): git$X
ln -s $< $@ 2>/dev/null || \
cp $< $@
-common-cmds.h: generate-cmdlist.perl command-list.txt
+common-cmds.h: generate-cmdlist.sh command-list.txt
common-cmds.h: $(wildcard Documentation/git-*.txt)
- $(QUIET_GEN)$(PERL_PATH) generate-cmdlist.perl command-list.txt > $@+
&& mv $@+ $@
+ $(QUIET_GEN)./generate-cmdlist.sh $@+ && mv $@+ $@
SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
diff --git a/generate-cmdlist.perl b/generate-cmdlist.perl
deleted file mode 100755
index 31516e3..000
--- a/generate-cmdlist.perl
+++ /dev/null
@@ -1,50 +0,0 @@
-#!/usr/bin/perl
-use strict;
-use warnings;
-
-print <<"EOT";
-/* Automatically generated by $0 */
-
-struct cmdname_help {
- char name[16];
- char help[80];
- unsigned char group;
-};
-
-static char *common_cmd_groups[] = {
-EOT
-
-my $n = 0;
-my %grp;
-while (<>) {
- last if /^### command list/;
- next if (1../^### common groups/) || /^#/ || /^\s*$/;
- chop;
- my ($k, $v) = split ' ', $_, 2;
- $grp{$k} = $n++;
- print "\tN_(\"$v\"),\n";
-}
-
-print "};\n\nstatic struct cmdname_help common_cmds[] = {\n";
-
-while (<>) {
- next if /^#/ || /^\s*$/;
- my @tags = split;
- my $cmd = shift @tags;
- for my $t (@tags) {
- if (exists $grp{$t}) {
- my $s;
- open my $f, '<', "Documentation/$cmd.txt" or die;
- while (<$f>) {
- ($s) = /^$cmd - (.+)$/;
- last if $s;
- }
- close $f;
- $cmd =~ s/^git-//;
- print "\t{\"$cmd\", N_(\"$s\"), $grp{$t}},\n";
- last;
- }
- }
-}
-
-print "};\n";
diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
new file mode 100755
index 000..1ac329d
--- /dev/null
+++ b/generate-cmdlist.sh
@@ -0,0 +1,50 @@
+#!/bin/sh
+
+echo "/* Automatically generated by $0 */
+struct cmdname_help {
+ char name[16];
+ char help[80];
+ unsigned char group;
+};
+
+static const char *common_cmd_groups[] = {"
+
+tmp=cmdgrps$$.tmp
+trap "rm -fr $tmp" 0 1 2 3 15
+
+sed -n '
+ 1,/^### common groups/b
+ /^### command list/q
+ /^#/d; /^[ ]*$/b
+ h;s/^[^ ][^ ]*[ ][ ]*\(.*\)/ N_("\1"),/p
+ g;s/^\([^ ][^ ]*\)[ ].*/\1/w '$tmp'
+ '
+printf '};\n\n'
+
+n=0
+matchgrp=
+substnum=
+while read grp
+do
+ matchgrp="$matchgrp${matchgrp:+
+}^git-..*[ ]$grp"
+ substnum="$substnum${substnum:+;}s/[]$grp/$n/"
+ n=$(($n+1))
+done <$tmp
+
+printf 'static struct cmdname_help common_cmds[] = {\n'
+gr
Re: [PATCH v2] git-p4: fix faulty paths for case insensitive systems
On Thu, Aug 20, 2015 at 3:16 AM, Lars Schneider wrote: > On 20 Aug 2015, at 06:59, Torsten Bögershausen wrote: >> On 08/19/2015 10:04 PM, larsxschnei...@gmail.com wrote: >>> From: Lars Schneider >>> +find . | grep two/File2.txt && >> Should we make sure that two/File2.txt exist? >> Then the "find | grep" feels like overkill. >> The line >> test -f two/File2.txt && >> could do the same (or do I miss something ?) > That’s what I did first. However, I am running OS X with HFS in > case-insensitive mode (the default). Consequently “test -f” > doesn’t care about the case. That’s why I used “grep”. This explanation may deserves to be recorded as an in-code comment to make the next person who modifies the test code aware of the issue. >>> +git ls-files > lines && >> and here > I really want to make sure only one file ends up in the repo. > >>> +test_line_count = 1 lines A more idiomatic way (in Git tests) to check both the file case and the file count would be: cat >expect <<-\EOF && two/File2.txt EOF git ls-files >actual && test_cmp expect actual >>> +) >>> +' >>> + >>> +test_expect_success 'Clone the repo WITH path fixing' ' >>> +client_view "//depot/One/... //client/..." && >>> +git p4 clone --fix-paths --use-client-spec --destination="$git" >>> //depot/one && >>> +test_when_finished cleanup_git && >>> +( >>> +cd "$git" && >>> +find . | grep TWO/file1.txt && >>> +find . | grep TWO/File2.txt && >>> +find . | grep TWO/file3.txt && >> Not sure about the find | grep here either. > See answers above. > >>> +git ls-files > lines && >>> +test_line_count = 3 lines Likewise: cat >expect <<-\EOF && TWO/File2.txt TWO/file1.txt TWO/file3.txt EOF git ls-files >actual && test_cmp expect actual >>> +) >>> +' >>> + >>> +# It looks like P4 determines the path case based on the first file in >>> +# lexicographical order. Please note the lower case "two" directory for all >>> +# files triggered through the addition of "File0.txt". >>> +test_expect_success 'Add a new file and clone the repo WITH path fixing' ' >>> +client_view "//depot/... //client/..." && >>> +cd "$cli" && >>> + >>> +mkdir -p One/two && >>> +> One/two/File0.txt && >>> +p4 add One/two/File0.txt && >>> +p4 submit -d "Add file" && >>> +rm -rf One && >>> + >>> +client_view "//depot/One/... //client/..." && >>> +git p4 clone --fix-paths --use-client-spec --destination="$git" >>> //depot/one && >>> +test_when_finished cleanup_git && >>> +( >>> +cd "$git" && >>> +find . | grep two/File0.txt && >>> +find . | grep two/file1.txt && >>> +find . | grep two/File2.txt && >>> +find . | grep two/file3.txt && >>> +git ls-files > lines && >>> +test_line_count = 4 lines And: cat >expect <<-\EOF && two/File0.txt two/File2.txt two/file1.txt two/file3.txt EOF git ls-files >actual && test_cmp expect actual >>> +) >>> +' >>> + >>> +test_expect_success 'kill p4d' ' >>> +kill_p4d >>> +' >>> + >>> +test_done >>> -- -- 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] generate-cmdlist: re-implement as shell script
On Wed, Aug 19, 2015 at 6:38 PM, Junio C Hamano wrote:
> Eric Sunshine writes:
>> In addition to reviving 527ec39^:generate-cmdlist.sh and extending it, I
>> also re-indented it with tabs instead of spaces, so it's helpful to
>> ignore whitespace changes when comparing the old and new versions of the
>> shell script.
>
> Hmph. Perhaps we can view it as part of reverting 527ec39 and then
> redoing it in shell.
I'm having trouble understanding. Are you asking that this be
presented as a series of patches which first revert 527ec39, then do
the whitespace cleanup, and then augment the script for the extended
functionality? If so, it's a bit problematic because the original
script still expects the 'common' tag to be present, but that tag was
removed by the subsequent patch 2f5b495 (command-list.txt: drop the
"common" tag, 2015-05-21). So, reverting 527ec39 would also require
reverting 2f5b495.
> The way the shell script accumulates matchgrp
> variable (i.e. the literal LF in ${var:+string}) makes me feel some
> possible portability scare, which might be solved in a more stupid
> (i.e. not giving various reimplementations of Bourne shells a chance
> to screw it up) way by using another temporary file[...]
In addition to the literal newline and the temporary file, other
options I considered included assigning the newline to a variable and
then interpolating that variable (${var:+$LF}), or expanding the list
of patterns into a set of '-e' arguments for grep. But, I think I'll
just go with the temporary file.
--
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] generate-cmdlist: re-implement as shell script
On Thu, Aug 20, 2015 at 1:24 PM, Junio C Hamano wrote:
> Eric Sunshine writes:
>> +sed -n '
>> + 1,/^### common groups/b
>> + /^### command list/q
>> + /^#/d; /^[ ]*$/b
>> + h;s/^[^ ][^ ]*[ ][ ]*\(.*\)/ N_("\1"),/p
>> + g;s/^\([^ ][^ ]*\)[ ].*/\1/w '$tmp'
>> + '
>> +printf '};\n\n'
>
> Unfortunately, this does not seem to work for me. Even though sed
> stops reading after seeing the "### command list" line, I suspect
> that its stdin buffer has been filled with other lines that follow
> it from the input to the buffer size, consuming what you meant to
> feed the later 'grep $matchgrp"' with.
>
> This is a one-time thing, so I do not mind to update the Makefile
> so that it does not feed command-list.txt from the standard input
> but gives the path as "$1" to this script.
The original generate-cmdlist.sh doesn't take any arguments and just
hardcodes "command-list.txt". Feeding it instead on stdin seemed a
nice way to avoid reading the file twice, but alas is too fragile. I
don't mind passing it as an argument either, or just hardcoding it
again (though my preference leans toward passing it as an argument).
--
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 v12 05/13] ref-filter: implement an `align` atom
On Tue, Aug 18, 2015 at 2:37 PM, Karthik Nayak wrote:
> Implement an `align` atom which left-, middle-, or right-aligns the
> content between %(align:..) and %(end).
>
> It is followed by `:,`, where the `` is
> either left, right or middle and `` is the size of the area
> into which the content will be placed. If the content between
> %(align:) and %(end) is more than the width then no alignment is
> performed. e.g. to align a refname atom to the middle with a total
> width of 40 we can do: --format="%(align:middle,40)%(refname)%(end)".
>
> We now have a `handler()` for each atom_value which will be called
> when that atom_value is being parsed, and similarly an `at_end`
> function for each state which is to be called when the `end` atom is
> encountered. Using this implement the `align` atom which aligns the
> given strbuf by calling `strbuf_utf8_align()` from utf8.c
>
> This is done by assigning a 'handler' function to each atom_value and
> a related 'at_end' function for each state. The align_handler()
> defined uses strbuf_utf8_align() from utf8.c to perform the alignment.
>
> Add documentation and tests for the same.
>
> Signed-off-by: Karthik Nayak
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index 74532d3..ecbcc5a 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -626,6 +638,36 @@ static inline char *copy_advance(char *dst, const char
> *src)
> return dst;
> }
>
> +static void align_handler(struct ref_formatting_state *state)
> +{
> + struct align *align = (struct align *)state->cb_data;
> + struct strbuf s = STRBUF_INIT;
> +
> + strbuf_utf8_align(&s, align->position, align->width,
> state->output.buf);
> + strbuf_reset(&state->output);
> + strbuf_addbuf(&state->output, &s);
> + free(align);
> +}
Leaking 'strbuf s' here.
Also, this operation can be expressed more concisely as:
strbuf_utf8_align(&s, ...);
strbuf_swap(&state->output, &s);
strbuf_release(&s);
Seeing this is also making me question my earlier suggestion of making
pop_state() responsible for appending the current state's output to
the previous state's output. The reason is that if align_handler() is
responsible for appending to the previous state's output, then all the
above string handling collapses to the one line:
strbuf_utf8_align(&state->prev->output, ..., state->output.buf);
which is even simpler, and doesn't involve a temporary strbuf or
swapping of strbuf contents.
On the other hand, it won't always be the case that all handlers can
get by with such simple code, and they might end up creating temporary
strbuf(s) and such anyhow, so I don't feel too strongly about it, and
it can always be changed at a later date (by someone) if that approach
ends up being better.
--
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 v2] git-p4: fix faulty paths for case insensitive systems
On Fri, Aug 21, 2015 at 4:08 AM, Lars Schneider wrote: > @Luke, Torsten, Eric, Junio: > Thanks for the great feedback. I incorporated everything into > "[PATCH v3] git-p4: fix faulty paths for case insensitive systems”. > Is this the correct way? I have never worked with the email-patch-files > before :-) I'm not a p4 user nor have I any knowledge of it, however, with regards to the issues my v2 comments touched upon, your v3 changes look fine. -- 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] contrib: teach completion about git-worktree options and arguments
On Fri, Aug 21, 2015 at 4:49 PM, SZEDER Gábor wrote:
> Quoting Junio C Hamano :
>> Eric Sunshine writes:
>>> On Thu, Jul 23, 2015 at 4:49 PM, Eric Sunshine
>>> wrote:
>>>> Complete subcommands 'add' and 'prune', as well as their respective
>>>> options --force, --detach, --dry-run, --verbose, and --expire. Also
>>>> complete 'refname' in "git worktree add [-b ]
>>>> ".
>>>
>>> Ping[1]?
>>
>> Ping indeed?
>
> Yeah, right, sorry. Non-subscribed occasional gmane-reader here amidst
> job hunting, so thanks for the ping. And the re-ping...
Thanks for the review.
>>>> +_git_worktree ()
>>>> +{
>>>> + local subcommands='add prune'
>>>> + local subcommand="$(__git_find_on_cmdline "$subcommands")"
>>>> + local c=2 n=0 refpos=2
>
> A more descriptive variable name for 'n' would be great.
Indeed. I was planning on resubmitting with better variable names
(even if I didn't get any feedback)...
>>>> + if [ -z "$subcommand" ]; then
>>>> + __gitcomp "$subcommands"
>>>> + else
>>>> + case "$subcommand,$cur" in
>>>> + add,--*)
>>>> + __gitcomp "--force --detach"
>
> We usually don't offer '--force', because that option must be
> handled with care.
As a person who never uses git-completion (or git-prompt), I wasn't
aware of that, so thanks for the heads-up. I only installed completion
(and prompt) when I decided to work on this (and I don't think I've
used tab-completion since then).
>>>> + ;;
>>>> + add,*)
>>>> + while [ $c -lt $cword ]; do
>>>> + case "${words[c]}" in
>>>> + --*) ;;
>>>> + -[bB]) ((refpos++)) ;;
>>>> + *) ((n++)) ;;
>>>> + esac
>>>> + ((c++))
>>>> + done
>>>> + if [ $n -eq $refpos ]; then
>
> I suppose here you wanted to calculate where (i.e. at which word on
> the command line) we should offer refs and fall back to bash builtin
> filename completion otherwise. It works well in the common cases,
> but:
>
> - it doesn't offer refs after -b or -B
That was intentional since this is a new branch name, so it didn't
seem sensible to offer completion of existing refs. On the other hand,
it doesn't make much since to offer pathname completion either, but I
didn't see a better alternative. On reflection, I suppose ref
completion can make sense if the new branch name is going to be
similar to an existing one.
> - it gets fooled by options to the git command, e.g. 'git
> --git-dir=.git worktree add ' offers refs instead of files,
> 'git --git-dir=.git worktree add ../some/path ' offers
> refs, etc.
This shortcoming could be addressed by computing relative to the
subcommand-index as your version does, correct?
I don't have enough background with the (git-specific) completion
facility to be able to judge if this patch and approach has merit and
ought to be pursued further, or if it would be better to drop in favor
of (some version of) your patch. I don't care strongly, and am fine
with dropping this patch if it's approach is suboptimal.
>>>> + __gitcomp_nl "$(__git_refs)"
>>>> + fi
>>>> + ;;
>>>> + prune,--*)
>>>> + __gitcomp "--dry-run --verbose --expire"
>>>> + ;;
>>>> + esac
>>>> + fi
>>>> +}
>>>> +
--
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/3] t5004: test ZIP archives with many entries
On Sat, Aug 22, 2015 at 3:06 PM, René Scharfe wrote:
> A ZIP file directory has a 16-bit field for the number of entries it
> contains. There are 64-bit extensions to deal with that. Demonstrate
> that git archive --format=zip currently doesn't use them and instead
> overflows the field.
>
> InfoZIP's unzip doesn't care about this field and extracts all files
> anyway. Software that uses the directory for presenting a filesystem
> like view quickly -- notably Windows -- depends on it, but doesn't
> lend itself to an automatic test case easily. Use InfoZIP's zipinfo,
> which probably isn't available everywhere but at least can provides
> *some* way to check this field.
>
> To speed things up a bit create and commit only a subset of the files
> and build a fake tree out of duplicates and pass that to git archive.
>
> Signed-off-by: Rene Scharfe
> ---
> diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh
> index 654adda..c6bd729 100755
> --- a/t/t5004-archive-corner-cases.sh
> +++ b/t/t5004-archive-corner-cases.sh
> @@ -115,4 +115,44 @@ test_expect_success 'archive empty subtree by direct
> pathspec' '
> check_dir extract sub
> '
>
> +ZIPINFO=zipinfo
> +
> +test_lazy_prereq ZIPINFO '
> + n=$("$ZIPINFO" "$TEST_DIRECTORY"/t5004/empty.zip | sed -n "2s/.* //p")
> + test "x$n" = "x0"
> +'
Unfortunately, this sed expression isn't portable due to dissimilar
output of various zipinfo implementations. On Linux, the output of
zipinfo is:
$ zipinfo t/t5004/empty.zip
Archive: t/t5004/empty.zip
Zip file size: 62 bytes, number of entries: 0
Empty zipfile.
$
however, on Mac OS X:
$ zipinfo t/t5004/empty.zip
Archive: t/t5004/empty.zip 62 bytes 0 files
Empty zipfile.
$
and on FreeBSD, the zipinfo command seems to have been removed
altogether in favor of "unzip -Z" (emulate zipinfo).
One might hope that "unzip -Z" would be a reasonable replacement for
zipinfo, however, it is apparently only partially implemented on
FreeBSD, and requires that -1 be passed, as well. Even with "unzip -Z
-1", there are issues. The output on Linux and Mac OS X is:
$ unzip -Z -1 t/t5004/empty.zip
Empty zipfile.
$
but FreeBSD differs:
$ unzip -Z -1 t/t5004/empty.zip
$
With a non-empty zip file, the output is identical on all platforms:
$ unzip -Z -1 twofiles.zip
file1
file2
$
So, if you combine that with "wc -l" or test_line_count, you may have
a portable and reliable entry counter.
More below...
> +test_expect_failure ZIPINFO 'zip archive with many entries' '
> + # add a directory with 256 files
> + mkdir 00 &&
> + for a in 0 1 2 3 4 5 6 7 8 9 a b c d e f
> + do
> + for b in 0 1 2 3 4 5 6 7 8 9 a b c d e f
> + do
> + : >00/$a$b
> + done
> + done &&
> + git add 00 &&
> + git commit -m "256 files in 1 directory" &&
> +
> + # duplicate it to get 65536 files in 256 directories
> + subtree=$(git write-tree --prefix=00/) &&
> + for c in 0 1 2 3 4 5 6 7 8 9 a b c d e f
> + do
> + for d in 0 1 2 3 4 5 6 7 8 9 a b c d e f
> + do
> + echo "04 tree $subtree $c$d"
> + done
> + done >tree &&
> + tree=$(git mktree +
> + # zip them
> + git archive -o many.zip $tree &&
> +
> + # check the number of entries in the ZIP file directory
> + expr 65536 + 256 >expect &&
> + "$ZIPINFO" many.zip | head -2 | sed -n "2s/.* //p" >actual &&
With these three patches applied, Mac OS X has trouble with 'many.zip':
$ unzip -Z -1 many.zip
warning [many.zip]: 76 extra bytes at beginning or within zipfile
(attempting to process anyway)
error [many.zip]: reported length of central directory is
-76 bytes too long (Atari STZip zipfile? J.H.Holm ZIPSPLIT 1.1
zipfile?). Compensating...
00/
00/00
...
ff/ff
error: expected central file header signature not found (file
#65793). (please check that you have transferred or created the
zipfile in the appropriate BINARY mode and that you have compiled
UnZip properly)
And FreeBSD doesn't like it either:
$ unzip -Z -1 many.zip
unzip: Invalid central directory signature
$
> + test_cmp expect actual
> +'
> +
> test_done
> --
> 2.5.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 1/3] t5004: test ZIP archives with many entries
On Sun, Aug 23, 2015 at 5:29 AM, "René Scharfe" wrote:
> Am 23.08.2015 um 07:54 schrieb Eric Sunshine:
>> On Sat, Aug 22, 2015 at 3:06 PM, René Scharfe wrote:
>>> +test_lazy_prereq ZIPINFO '
>>> + n=$("$ZIPINFO" "$TEST_DIRECTORY"/t5004/empty.zip | sed -n "2s/.*
>>> //p")
>>> + test "x$n" = "x0"
>>> +'
>>
>> Unfortunately, this sed expression isn't portable due to dissimilar
>> output of various zipinfo implementations. On Linux, the output of
>> zipinfo is:
>>
>> $ zipinfo t/t5004/empty.zip
>> Archive: t/t5004/empty.zip
>> Zip file size: 62 bytes, number of entries: 0
>> Empty zipfile.
>> $
>>
>> however, on Mac OS X:
>>
>> $ zipinfo t/t5004/empty.zip
>> Archive: t/t5004/empty.zip 62 bytes 0 files
>> Empty zipfile.
>> $
>>
>> and on FreeBSD, the zipinfo command seems to have been removed
>> altogether in favor of "unzip -Z" (emulate zipinfo).
>
> I suspected that zipinfo's output might be formatted differently on
> different platforms and tried to guard against it by checking for the
> number zero there. Git's ZIP file creation is platform independent
> (modulo bugs), so having a test run at least somewhere should
> suffice. In theory.
>
> We could add support for the one-line-summary variant on OS X easily,
> though.
Probably, although it's looking like testing on Mac OS X won't be
fruitful (see below).
>> One might hope that "unzip -Z" would be a reasonable replacement for
>> zipinfo, however, it is apparently only partially implemented on
>> FreeBSD, and requires that -1 be passed, as well. Even with "unzip -Z
>> -1", there are issues. The output on Linux and Mac OS X is:
>>
>> $ unzip -Z -1 t/t5004/empty.zip
>> Empty zipfile.
>> $
>>
>> but FreeBSD differs:
>>
>> $ unzip -Z -1 t/t5004/empty.zip
>> $
>>
>> With a non-empty zip file, the output is identical on all platforms:
>>
>> $ unzip -Z -1 twofiles.zip
>> file1
>> file2
>> $
>>
>> So, if you combine that with "wc -l" or test_line_count, you may have
>> a portable and reliable entry counter.
>
> Counting all entries is slow, and more importantly it's not what we
> want. In this test we need the number of entries recorded in the ZIP
> directory, not the actual number of entries found by scanning the
> archive, or the directory.
Ah, right. The commit message did state this clearly enough...
> On Linux "unzip -Z -1 many.zip | wc -l" reports 65792 even before
> adding ZIP64 support; only without -1 we get the interesting numbers
> (specifically with "unzip -Z many.zip | sed -n '2p;$p'"):
>
> Zip file size: 6841366 bytes, number of entries: 256
> 65792 files, 0 bytes uncompressed, 0 bytes compressed: 0.0%
>
>> With these three patches applied, Mac OS X has trouble with 'many.zip':
>>
>> $ unzip -Z -1 many.zip
>> warning [many.zip]: 76 extra bytes at beginning or within zipfile
>>(attempting to process anyway)
>> error [many.zip]: reported length of central directory is
>>-76 bytes too long (Atari STZip zipfile? J.H.Holm ZIPSPLIT 1.1
>>zipfile?). Compensating...
>> 00/
>> 00/00
>> ...
>> ff/ff
>> error: expected central file header signature not found (file
>>#65793). (please check that you have transferred or created the
>>zipfile in the appropriate BINARY mode and that you have compiled
>>UnZip properly)
>>
>> And FreeBSD doesn't like it either:
>>
>> $ unzip -Z -1 many.zip
>> unzip: Invalid central directory signature
>> $
>>
>
> Looks like they don't support ZIP64. Or I got some of the fields wrong
> after all.
A >65536 file zip created on Mac OS X with Mac's "zip" command given
to "unzip" or "zipinfo" results in exactly the same warnings/errors as
above (including the bit about "76 extra bytes" and "-76 bytes too
long"), so it doesn't seem to be a problem with your implementation.
> https://en.wikipedia.org/wiki/Zip_%28file_format%29#ZIP64 says: "OS X
> Yosemite does support the creation of ZIP64 archives, but does not
> support unzipping these archives using the shipped unzip command-line
> utility or graphical Archive Utility.[citation needed]".
>
> How does unzip react to a ZIP file with more than 65535 entries that
> was created natively on these platforms? And what does zipinfo (a real
> one, without -1) report at the top for such files?
On Mac OS X, unzip does extract all the files (although complains as
noted above). zipinfo caps out at reporting 65535 for the number of
files (although it lists them all fine). With the warnings/errors
filtered out for clarity:
$ zipinfo biggy.zip
Archive: biggy.zip 9642874 bytes 65535 files
...
--
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: Eric Sunshine mail delivery failure
On Sun, Aug 23, 2015 at 1:16 PM, Johannes Löthberg wrote: > On 23/08, René Scharfe wrote: >> Eric, hope you see this reply on the list. Direct replies to >> sunsh...@sunshineco.com are rejected by my mail provider on submit in >> Thunderbird with the following message: >> >>Requested action not taken: mailbox unavailable >>invalid DNS MX or A/ resource record. >> >> And with this one when using their web interface: >> >>A message that you sent could not be delivered to one or more of >>its recipients. This is a permanent error. The following address >>failed: >> >>"sunsh...@sunshineco.com": >>no valid MX hosts found >> >> It seems web.de wants you to get an record before I'm allowed to send >> mails to you. > > Just an A record would be enough. The issue is that mail.sunshineco.com has > neither an A nor an record, it is a CNAME to sunshineco.com, which is > invalid according to RFC2181. Interestingly, the default configuration for all domains managed by this service provider is for the mailhost to be a CNAME. While the restriction in section 10.3 of RFC2181 makes sense as a way to avoid extra "network burden", in practice, email services seem to be pretty relaxed about it, and follow the CNAME indirection as needed. I suppose it's possible that web.de is being extra strict (although it seems that such strictness would be painful for its users), or this could just be a temporary DNS lookup failure. It's hard to tell based upon the errors René reported. I did change the CNAME to an A just in case, though who knows how long it will take for the change to propagate over to web.de's server. -- 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: Eric Sunshine mail delivery failure
On Sun, Aug 23, 2015 at 2:36 PM, Elia Pinto wrote: > Il 23/Ago/2015 20:26, "Eric Sunshine" ha scritto: >> On Sun, Aug 23, 2015 at 1:16 PM, Johannes Löthberg >> wrote: >> > Just an A record would be enough. The issue is that mail.sunshineco.com >> > has >> > neither an A nor an record, it is a CNAME to sunshineco.com, which >> > is >> > invalid according to RFC2181. >> >> Interestingly, the default configuration for all domains managed by >> this service provider is for the mailhost to be a CNAME. While the >> restriction in section 10.3 of RFC2181 makes sense as a way to avoid >> extra "network burden", in practice, email services seem to be pretty >> relaxed about it, and follow the CNAME indirection as needed. >> >> I suppose it's possible that web.de is being extra strict (although it >> seems that such strictness would be painful for its users), or this >> could just be a temporary DNS lookup failure. It's hard to tell based >> upon the errors René reported. >> >> I did change the CNAME to an A just in case, though who knows how long >> it will take for the change to propagate over to web.de's server. > Anyone can check Here https://dnschecker.org/#CNAME/Mail.sunshineco.com > It would fail with your change Interesting service; thanks for the pointer. However, since it's just querying a random set of DNS servers, it's not necessarily indicative of whether the change has actually propagated to the DNS server(s) answering web.de's mail server's queries. Local configuration (TTL's, etc.) on those servers or anywhere in between, as well as network conditions, could impact propagation to an unknown degree. -- 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: Eric Sunshine mail delivery failure
On Sun, Aug 23, 2015 at 2:48 PM, Eric Sunshine wrote: > On Sun, Aug 23, 2015 at 2:36 PM, Elia Pinto wrote: >> Il 23/Ago/2015 20:26, "Eric Sunshine" ha scritto: >>> I did change the CNAME to an A just in case, though who knows how long >>> it will take for the change to propagate over to web.de's server. >> Anyone can check Here https://dnschecker.org/#CNAME/Mail.sunshineco.com >> It would fail with your change > > Interesting service; thanks for the pointer. However, since it's just > querying a random set of DNS servers, it's not necessarily indicative > of whether the change has actually propagated to the DNS server(s) > answering web.de's mail server's queries. Local configuration (TTL's, > etc.) on those servers or anywhere in between, as well as network > conditions, could impact propagation to an unknown degree. Also, the propagation time of the A record can be quite different from the point at which the CNAME record finally expires (based upon its TTL, which may differ dramatically from server to server), so the above CNAME query may continue to succeed long after the A record has propagated. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] generate-cmdlist: re-implement as shell script
527ec39 (generate-cmdlist: parse common group commands, 2015-05-21)
replaced generate-cmdlist.sh with a more functional Perl version,
generate-cmdlist.perl. The Perl version gleans named tags from a new
"common groups" section in command-list.txt and recognizes those tags in
"command list" section entries in place of the old 'common' tag. This
allows git-help to, not only recognize, but also group common commands.
Although the tests require Perl, 527ec39 creates an unconditional
dependence upon Perl in the build system itself, which can not be
overridden with NO_PERL. Such a dependency may be undesirable; for
instance, the 'git-lite' package in the FreeBSD ports tree is intended
as a minimal Git installation (which may, for example, be useful on
servers needing only local clone and update capability), which,
historically, has not depended upon Perl[1].
Therefore, revive generate-cmdlist.sh and extend it to recognize "common
groups" and its named tags. Retire generate-cmdlist.perl.
[1]: http://thread.gmane.org/gmane.comp.version-control.git/275905/focus=276132
Signed-off-by: Eric Sunshine
---
Changes since v1 [2]:
* Avoid literal newline inside ${var:+val} when composing grep match
patterns, lest some shells might trip over it. Instead, pass match
patterns to grep via -f temporary file.
* Don't assume it's portable to feed commands-list.txt to script as
stdin and expect one command (sed) to consume only part of it, leaving
the rest for consumption by a subsequent command (grep). While this
works on some platforms, in practice, the first command (sed) may
buffer and eat more of the input than expected. Instead, pass
commands-list.txt as argument and have each command independently open
and consume it.
* Replace errant 'd' sed command with 'b' to skip comment lines. The end
result is the same, but the other "skipping" actions use 'b', and 'b'
was intended all along.
v2 is still a single patch since I couldn't quite figure out[3] whether
Junio was asking for the patch to be submitted as a reversion plus a
cleanup plus an enhancement or not.
[2]: http://thread.gmane.org/gmane.comp.version-control.git/276226
[3]: http://thread.gmane.org/gmane.comp.version-control.git/276226/focus=276256
Makefile | 4 ++--
generate-cmdlist.perl | 50 --
generate-cmdlist.sh | 51 +++
3 files changed, 53 insertions(+), 52 deletions(-)
delete mode 100755 generate-cmdlist.perl
create mode 100755 generate-cmdlist.sh
diff --git a/Makefile b/Makefile
index e39ca6c..5d3e3b9 100644
--- a/Makefile
+++ b/Makefile
@@ -1697,10 +1697,10 @@ $(BUILT_INS): git$X
ln -s $< $@ 2>/dev/null || \
cp $< $@
-common-cmds.h: generate-cmdlist.perl command-list.txt
+common-cmds.h: generate-cmdlist.sh command-list.txt
common-cmds.h: $(wildcard Documentation/git-*.txt)
- $(QUIET_GEN)$(PERL_PATH) generate-cmdlist.perl command-list.txt > $@+
&& mv $@+ $@
+ $(QUIET_GEN)./generate-cmdlist.sh command-list.txt >$@+ && mv $@+ $@
SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
diff --git a/generate-cmdlist.perl b/generate-cmdlist.perl
deleted file mode 100755
index 31516e3..000
--- a/generate-cmdlist.perl
+++ /dev/null
@@ -1,50 +0,0 @@
-#!/usr/bin/perl
-use strict;
-use warnings;
-
-print <<"EOT";
-/* Automatically generated by $0 */
-
-struct cmdname_help {
- char name[16];
- char help[80];
- unsigned char group;
-};
-
-static char *common_cmd_groups[] = {
-EOT
-
-my $n = 0;
-my %grp;
-while (<>) {
- last if /^### command list/;
- next if (1../^### common groups/) || /^#/ || /^\s*$/;
- chop;
- my ($k, $v) = split ' ', $_, 2;
- $grp{$k} = $n++;
- print "\tN_(\"$v\"),\n";
-}
-
-print "};\n\nstatic struct cmdname_help common_cmds[] = {\n";
-
-while (<>) {
- next if /^#/ || /^\s*$/;
- my @tags = split;
- my $cmd = shift @tags;
- for my $t (@tags) {
- if (exists $grp{$t}) {
- my $s;
- open my $f, '<', "Documentation/$cmd.txt" or die;
- while (<$f>) {
- ($s) = /^$cmd - (.+)$/;
- last if $s;
- }
- close $f;
- $cmd =~ s/^git-//;
- print "\t{\"$cmd\", N_(\"$s\"), $grp{$t}},\n";
- last;
- }
- }
-}
-
-print "};\n";
diff --git a/generate-cmdlist.sh b/generate-cmdlist
Re: [PATCH v5 2/2] worktree: add 'list' command
On Mon, Aug 24, 2015 at 2:05 PM, Junio C Hamano wrote:
> Michael Rappazzo writes:
>> + strbuf_strip_suffix(&head_ref, "\n");
>> +
>> + if (starts_with(head_ref.buf, ref_prefix)) {
>> + /* branch checked out */
>> + strbuf_remove(&head_ref, 0, strlen(ref_prefix));
>> + /* } else {
>> + * headless -- no-op
>> + */
>> + }
>> + printf("%s (%s)\n", path, head_ref.buf);
>
> Is this new command meant to be a Porcelain? This would not work as
> a plumbing that produces a machine-parseable stable output.
>
> I am not saying that it _should_; I do not know if we even need a
> 'list' command that is driven from an end-user script that gives
> anything more than "where the work trees are".
>
> My inclination is to suggest dropping the "which branch" code
> altogether and only give "path_only" behaviour.
The "which branch" was probably added in response to this [1] review,
which suggested that at some point, we might want to provide the user
with interesting information about each worktree, such as
branch/detached head, tag, locked status (plus lock reason and whether
currently accessible), prune-able status (plus reason). This could
optionally be controlled by --verbose or some other extended
formatting option.
The same review also suggested a --porcelain option for script writers.
[1]: http://article.gmane.org/gmane.comp.version-control.git/275528
--
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: [FEATURE REQUEST] Filter-branch extend progress with a simple estimated time remaning
On Tue, Aug 25, 2015 at 2:54 PM, Jeff King wrote:
> On Tue, Aug 25, 2015 at 02:52:10PM -0400, Jeff King wrote:
>
>> Yeah, that would probably be a good solution, assuming there is a
>> portable "how many seconds" (I do not relish the thought of
>> reconstructing it based on the current hours/minutes/seconds).
>
> A little googling came up with:
>
> awk 'END { print systime() }'
> which probably (?) works everywhere.
On Mac OS X and FreeBSD:
$ awk 'END { print systime() }' http://vger.kernel.org/majordomo-info.html
Re: [PATCHv1 1/3] git-p4: failing test for ignoring invalid p4 labels
On Thu, Aug 27, 2015 at 3:18 AM, Luke Diamand wrote:
> When importing a label which references a commit that git-p4 does
> not know about, git-p4 should skip it and go on to process other
> labels that can be imported.
>
> Instead it crashes when attempting to find the missing commit in
> the git history. This test demonstrates the problem.
>
> Signed-off-by: Luke Diamand
> ---
> diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
> index 095238f..f7d5048 100755
> --- a/t/t9811-git-p4-label-import.sh
> +++ b/t/t9811-git-p4-label-import.sh
> @@ -214,6 +214,51 @@ test_expect_success 'use git config to enable
> import/export of tags' '
> )
> '
>
> +p4_head_revision() {
> + p4 changes -m 1 "$@" | awk '{print $2}'
> +}
> +
> +# Importing a label that references a P4 commit that has
> +# has not been seen. The presence of a label on a commit
s/has has/has/
> +# we haven't seen should not cause git-p4 to fail. It should
> +# merely skip that label, and still import other labels.
> +test_expect_failure 'importing labels with missing revisions' '
--
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] stash: Add stash.showFlag config variable
On Thu, Aug 27, 2015 at 11:36 AM, Namhyung Kim wrote: > On Fri, Aug 28, 2015 at 12:20 AM, SZEDER Gábor wrote: >> - This hunk runs the the exact same 'git config' command twice. Run it >>only once, perhaps something like this: >> >> show_flag=$(git config --get stash.showflag || echo --stat) >> >>(I hope there are no obscure crazy 'echo' implemtations out there >>that might barf on the unknown option '--stat'...) > > What about `echo "--stat"` then? Adding quotes around --stat won't buy you anything since the shell will have removed the quotes by the time the argument is passed to echo, so an "obscure crazy" 'echo' will still see --stat as an option. POSIX states that printf should take no options, so: printf --stat should be safe, but some implementations do process options (and will complain about the unknown --stat option), therefore, best would be: printf '%s' --stat -- 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 with worktrees...
On Thu, Aug 27, 2015 at 3:03 PM, John Szakmeister wrote: > My apologies if this has already been reported, but I ran into an > interesting bug with worktrees. In particular, I have an alias 'st' > that maps to 'status -sb'. When running this under a subdirectory of > a worktree created with 'git worktree add', it fails complaining that > the work tree has already been set. > > Here's a script to reproduce the problem: > git init test-repo > cd test-repo > git config --local alias.st 'status -sb' > mkdir subdir > echo file > subdir/file.txt > git add subdir/file.txt > git commit -m 'add file' > git branch foo > git worktree add ../new-worktree foo > cd ../new-worktree/subdir > echo "new line" >> file.txt > echo "this will work" > git status -sb > echo "this fails" > git st > > When I run it, I see this: > [...] > fatal: internal error: work tree has already been set > Current worktree: /home/jszakmeister/tmp/test-case/new-worktree > New worktree: /home/jszakmeister/tmp/test-case/new-worktree/subdir I can reproduce with 2.5.0 but not 'master'. Bisection reveals that this was fixed by d95138e (setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR, 2015-06-26), and was reported previously here [1]. [1]: http://git.661346.n2.nabble.com/Linked-workdirs-break-typo-correction-td7634347.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
Re: [PATCH 1/3] t5004: test ZIP archives with many entries
On Fri, Aug 28, 2015 at 11:57 AM, Junio C Hamano wrote: > Junio C Hamano writes: >> Eric Sunshine writes: >>> On Sun, Aug 23, 2015 at 5:29 AM, "René Scharfe" wrote: >>>> I suspected that zipinfo's output might be formatted differently on >>>> different platforms and tried to guard against it by checking for the >>>> number zero there. Git's ZIP file creation is platform independent >>>> (modulo bugs), so having a test run at least somewhere should >>>> suffice. In theory. >>>> >>>> We could add support for the one-line-summary variant on OS X easily, >>>> though. >>> >>> Probably, although it's looking like testing on Mac OS X won't be >>> fruitful (see below). >> >> Can we move this topic forward by introducing a new prerequisite >> ZIPINFO and used at the beginning of these tests (make it a lazy >> prereq)? Run zipinfo on a trivial archive and see if its output is >> something we recognize to decide if the platform supports that >> ZIPINFO prerequisite and do this test only on them. > > Heh, that is exactly what the patch under discussion does. So... > >> After all, what _is_ being tested, i.e. our archive creation, would >> not change across platforms, so having a test that runs on a known >> subset of platforms is better than not having anything at all. > > ...I'd say we can take this patch as-is, and those who want to have > a working test on MacOS can come up with an enhancement to the way > the script parses output from zipinfo that would also work on their > platforms. Right, the new test is correctly skipped on Mac OS X and FreeBSD, so the patch is suitable as-is. We might, however, want to augment the commit message with some of the knowledge learned from this thread. Perhaps modify the last sentence of the second paragraph and then insert additional information following it, like this? ... at least provides *some* way to check this field, although presently only on Linux. zipinfo on current Mac OS X (Yosemite 10.10.5) does not support this field, and, when encountered, caps the printed file count at 65535 (and spits out warnings and errors), thus is not useful for testing. (Its output also differs from zipinfo on Linux, thus requires changes to the 'sed' recognition and extraction expressions, but that's a minor issue.) zipinfo on FreeBSD seems to have been retired altogether in favor of "unzip -Z", however, only in the emasculated form "unzip -Z -1" which lists archive entries but does not provide a file count, thus is not useful for this test. (I also snuck a s/can// fix in there for the last sentence of the second paragraph.) -- 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: What's cooking in git.git (Aug 2015, #05; Fri, 28)
On Fri, Aug 28, 2015 at 5:11 PM, Junio C Hamano wrote: > Here are the topics that have been cooking. Commits prefixed with > '-' are only in 'pu' (proposed updates) while commits prefixed with > '+' are in 'next'. > > You can find the changes described here in the integration branches > of the repositories listed at > > http://git-blame.blogspot.com/p/git-public-repositories.html > > -- > [New Topics] Hmm, Duy's patch series to fix "git clone foo" when 'foo' is a linked worktree[1] doesn't seem to have been picked up. Was it lost in the shuffle? [1]: http://thread.gmane.org/gmane.comp.version-control.git/273982/focus=276346 -- 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: [FEATURE REQUEST] Filter-branch extend progress with a simple estimated time remaning
On Sat, Aug 29, 2015 at 9:29 AM, Gabor Bernat wrote: > Amended, the latest version is at https://github.com/gaborbernat/git/commit/ > :) > Does this looks okay, should I create a patch from this? Excerpt: now=$(date +%s) elapsed=$(($now - $start)) remaining_second=$((...)) eta=$(($now + $remaining_second)) finish_by=$(date -d "@$eta") Unfortunately, -d is not portable. On Mac OS X and FreeBSD, -d sets the kernel's value for Daylight Saving Time, rather than displaying the specified time as in Linux. -- 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: [FEATURE REQUEST] Filter-branch extend progress with a simple estimated time remaning
(Please don't top-post on this list.)
On Sat, Aug 29, 2015 at 11:00 PM, Gabor Bernat wrote:
> Reading after it, I think the most close we can get with this is, awk
> 'BEGIN { print strftime("%c", 1271603087); }; and just ignore setting
> this value (and avoid displaying it) if that fails too. Do you agree?
strftime() in awk is a GNU-ism. It doesn't exist in awk on Mac OS X or
FreeBSD, or even the default awk on Linux (which is mawk on Linux
installations I've checked).
Most portable likely would be Perl, however, that's probably too
heavyweight inside a loop like this, even if called only once each N
iterations.
> On Sun, Aug 30, 2015 at 3:20 AM, Eric Sunshine
> wrote:
>> On Sat, Aug 29, 2015 at 9:29 AM, Gabor Bernat wrote:
>>> Amended, the latest version is at
>>> https://github.com/gaborbernat/git/commit/ :)
>>> Does this looks okay, should I create a patch from this?
>>
>> Excerpt:
>>
>> now=$(date +%s)
>> elapsed=$(($now - $start))
>> remaining_second=$((...))
>> eta=$(($now + $remaining_second))
>> finish_by=$(date -d "@$eta")
>>
>> Unfortunately, -d is not portable. On Mac OS X and FreeBSD, -d sets
>> the kernel's value for Daylight Saving Time, rather than displaying
>> the specified time as in Linux.
--
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 v14 04/13] ref-filter: implement an `align` atom
On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak wrote:
> Implement an `align` atom which left-, middle-, or right-aligns the
> content between %(align:..) and %(end).
>
> It is followed by `:,`, where the `` is
> either left, right or middle and `` is the size of the area
> into which the content will be placed. If the content between
> %(align:) and %(end) is more than the width then no alignment is
> performed. e.g. to align a refname atom to the middle with a total
> width of 40 we can do: --format="%(align:middle,40)%(refname)%(end)".
>
> We now have a `handler()` for each atom_value which will be called
> when that atom_value is being parsed, and similarly an `at_end`
> function for each element of the stack which is to be called when the
> `end` atom is encountered. Using this we implement the `align` atom
> which aligns the given strbuf by calling `strbuf_utf8_align()` from
> utf8.c.
>
> Extract perform_quote_formatting() from append_atom(). Given a string
> a quote_value and a strbuf, perform_quote_formatting() formats the
> string based on the quote_value and stores it into the strbuf.
>
> Ensure that quote formatting is performed on the whole of
> %(align)...%(end) rather than individual atoms. We do this by skipping
> individual quote formatting for atoms whenever the stack has more than
> one element, and performing formatting for the entire stack element
> when the `%(end)` atoms is encountered.
This patch seems to be conflating three distinct changes:
1. adding %(align:) atom
2. extracting quoting logic to a separate function
3. quoting top-level %(align:) but not contained atoms
In fact, #3 seems too specially tied to %(align:)...%(end). One might
expect that the logic for determining when to quote should be
independent of any particular atom, which suggests that this logic is
being handled at the wrong level, and that %(align:) shouldn't have to
know anything about quoting. I'd have thought the quoting logic would
naturally accompany the introduction of the formatting state stack
mechanism in patch 2/13, and that that would generically work with all
atoms, including %(align:) and whatever new ones are added in the
future.
But, I may not have been following the quoting discussion closely, so
perhaps these observations are incorrect? See more below regarding
%(if:).
> Add documentation and tests for the same.
>
> Signed-off-by: Karthik Nayak
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index 432cea0..21c8b5f 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -53,6 +54,13 @@ static struct {
> { "flag" },
> { "HEAD" },
> { "color" },
> + { "align" },
> + { "end" },
> +};
> +
> +struct align {
> + align_type position;
> + unsigned int width;
> };
>
> #define REF_FORMATTING_STATE_INIT { 0, NULL }
> @@ -69,6 +79,8 @@ struct ref_formatting_state {
>
> struct atom_value {
> const char *s;
> + struct align *align;
Why does 'align' need to be heap-allocated rather than just being a
direct member of 'atom_value'? Does 'align' need to exist beyond the
lifetime of its 'atom_value'? If not, making it a direct member might
simplify resource management (no need to free it).
> + void (*handler)(struct atom_value *atomv, struct ref_formatting_state
> *state);
> unsigned long ul; /* used for sorting when not FIELD_STR */
> };
>
> @@ -632,6 +644,84 @@ static inline char *copy_advance(char *dst, const char
> *src)
> +static void append_atom(struct atom_value *v, struct ref_formatting_state
> *state)
> +{
> + /*
> +* Quote formatting is only done when the stack has a single
> +* element. Otherwise quote formatting is done on the
> +* element's entire output strbuf when the %(end) atom is
> +* encountered.
> +*/
> + if (!state->stack->prev)
With the disclaimer that I wasn't following the quoting discussion
closely: Is this condition going to be sufficient for all cases, such
as an %(if:) atom? That is, if you have:
%(if:notempty)%(bloop)%(then) --option=%(bloop)%(end)
isn't the intention that, %(bloop) within the %(then) section should
be quoted but not the literal "--option="?
The condition `!state->stack->prev' would be insufficient to handle
this if %(if:) pushes one or more states onto the stack, no? This
implies that you might want an explicit flag for enabling/disabling
quoting rather than relying upon the state of the stack, and that
individual atom handlers would control that flag.
Or, am I misunderstanding due to not having followed the discussion closely?
> + quote_formatting(&state->stack->output, v->s,
> state->quote_style);
> + else
> + strbuf_addstr(&state->stack->output, v->s);
> +}
> +
> +static void end_atom_handler(struct atom_value *atomv, struct
> ref_formatting_state *state)
> +{
> + struct ref_formatting_stack *current = state->stack;
> + struct strbuf s = STRBUF_INIT;
> +
> +
Re: [PATCH v14 05/13] ref-filter: add option to filter out tags, branches and remotes
On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak wrote:
> Add a function called 'for_each_fullref_in()' to refs.{c,h} which
> iterates through each ref for the given path without trimming the path
> and also accounting for broken refs, if mentioned.
>
> Add 'filter_ref_kind()' in ref-filter.c to check the kind of ref being
> handled and return the kind to 'ref_filter_handler()', where we
> discard refs which we do not need and assign the kind to needed refs.
>
> Signed-off-by: Karthik Nayak
> ---
> diff --git a/refs.c b/refs.c
> index 4e15f60..a9469c2 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2108,6 +2108,15 @@ int for_each_ref_in(const char *prefix, each_ref_fn
> fn, void *cb_data)
> return do_for_each_ref(&ref_cache, prefix, fn, strlen(prefix), 0,
> cb_data);
> }
>
> +int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,
> unsigned int broken)
Booleans such as 'broken' are typically declared 'int' in this
codebase, rather than 'unsigned int'.
> +{
> + unsigned int flag = 0;
> +
> + if (broken)
> + flag = DO_FOR_EACH_INCLUDE_BROKEN;
> + return do_for_each_ref(&ref_cache, prefix, fn, 0, flag, cb_data);
> +}
> +
> int for_each_ref_in_submodule(const char *submodule, const char *prefix,
> each_ref_fn fn, void *cb_data)
> {
> diff --git a/refs.h b/refs.h
> index e9a5f32..6d30c98 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -173,6 +173,7 @@ typedef int each_ref_fn(const char *refname,
> extern int head_ref(each_ref_fn fn, void *cb_data);
> extern int for_each_ref(each_ref_fn fn, void *cb_data);
> extern int for_each_ref_in(const char *prefix, each_ref_fn fn, void
> *cb_data);
> +extern int for_each_fullref_in(const char *prefix, each_ref_fn fn, void
> *cb_data, unsigned int broken);
> extern int for_each_tag_ref(each_ref_fn fn, void *cb_data);
> extern int for_each_branch_ref(each_ref_fn fn, void *cb_data);
> extern int for_each_remote_ref(each_ref_fn fn, void *cb_data);
--
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 v14 06/13] ref-filter: introduce format_ref_array_item()
On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak wrote:
> Create format_ref_array_item() out of show_ref_array_item(). This will
> store the output format for the given ref_array_item into the provided
> strbuf. Make show_ref_array_item() a wrapper around this to print the
> given ref_array_item with linefeed.
Perhaps you could explain why this change is a good idea, such as that
a future patch, for reason, will need the
formatting capability of format_ref_array_item() but not the printing
with newline done by show_ref_array_item().
> Signed-off-by: Karthik Nayak
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index 5d4f93d..1e6754a 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -153,6 +153,27 @@ int parse_ref_filter_atom(const char *atom, const char
> *ep)
> return at;
> }
>
> +static void quote_formatting(struct strbuf *s, const char *str, int
> quote_style)
> +{
> + switch (quote_style) {
> + case QUOTE_NONE:
> + strbuf_addstr(s, str);
> + break;
> + case QUOTE_SHELL:
> + sq_quote_buf(s, str);
> + break;
> + case QUOTE_PERL:
> + perl_quote_buf(s, str);
> + break;
> + case QUOTE_PYTHON:
> + python_quote_buf(s, str);
> + break;
> + case QUOTE_TCL:
> + tcl_quote_buf(s, str);
> + break;
> + }
> +}
This code was already relocated once in patch 4/13, and is now being
relocated again in 6/13. If you instead place the code at the final
desired location in 4/13, then this patch will become less noisy.
More below.
> static void push_stack_element(struct ref_formatting_stack **stack)
> {
> struct ref_formatting_stack *s = xcalloc(1, sizeof(struct
> ref_formatting_stack));
> @@ -665,27 +686,6 @@ static void align_atom_handler(struct atom_value *atomv,
> struct ref_formatting_s
> new->cb_data = atomv->align;
> }
>
> -static void quote_formatting(struct strbuf *s, const char *str, int
> quote_style)
> -{
> - switch (quote_style) {
> - case QUOTE_NONE:
> - strbuf_addstr(s, str);
> - break;
> - case QUOTE_SHELL:
> - sq_quote_buf(s, str);
> - break;
> - case QUOTE_PERL:
> - perl_quote_buf(s, str);
> - break;
> - case QUOTE_PYTHON:
> - python_quote_buf(s, str);
> - break;
> - case QUOTE_TCL:
> - tcl_quote_buf(s, str);
> - break;
> - }
> -}
> -
> static void append_atom(struct atom_value *v, struct ref_formatting_state
> *state)
> {
> /*
> @@ -1478,10 +1478,17 @@ void show_ref_array_item(struct ref_array_item *info,
> const char *format, int qu
> }
> if (state.stack->prev)
> die(_("format: `end` atom missing"));
> - final_buf = &state.stack->output;
> - fwrite(final_buf->buf, 1, final_buf->len, stdout);
> + strbuf_addbuf(out, &state.stack->output);
> pop_stack_element(&state.stack);
> - putchar('\n');
> +}
> +
> +void show_ref_array_item(struct ref_array_item *item, const char *format,
> unsigned int quote_style)
> +{
> + struct strbuf out = STRBUF_INIT;
> + format_ref_array_item(&out, item, format, quote_style);
> + fwrite(out.buf, out.len, 1, stdout);
> + printf("\n");
putchar('\n');
> + strbuf_release(&out);
> }
>
> /* If no sorting option is given, use refname to sort as default */
--
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 v14 05/13] ref-filter: add option to filter out tags, branches and remotes
On Sun, Aug 30, 2015 at 2:51 AM, Karthik Nayak wrote: > On Sun, Aug 30, 2015 at 9:00 AM, Eric Sunshine > wrote: >> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak >> wrote: >>> +int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, >>> unsigned int broken) >> >> Booleans such as 'broken' are typically declared 'int' in this >> codebase, rather than 'unsigned int'. > > But doesn't it make more sense to have it as unsigned, since its values are > either 0 or 1? In C, zero is false and any other value is true, so from that viewpoint, the type doesn't matter much. However, beside being raw instructions for the computer, code should ideally convey its intention as clearly as possible to other programmers. 'int' for boolean has been idiomatic since C's inception, thus is a good way to do so, whereas 'unsigned int' is typically used for magnitude or bit flags, thus is misleading. -- 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 v14 07/13] ref-filter: add support for %(contents:lines=X)
On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak wrote:
> In 'tag.c' we can print N lines from the annotation of the tag using
> the '-n' option. Copy code from 'tag.c' to 'ref-filter' and
> modify it to support appending of N lines from the annotation of tags
> to the given strbuf.
>
> Implement %(contents:lines=X) where X lines of the given object are
> obtained.
>
> Add documentation and test for the same.
>
> Signed-off-by: Karthik Nayak
> ---
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 471d6b1..0fc7557 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -63,6 +64,11 @@ struct align {
> unsigned int width;
> };
>
> +struct contents {
> + unsigned int lines;
> + struct object_id oid;
> +};
> +
> #define REF_FORMATTING_STATE_INIT { 0, NULL }
>
> struct ref_formatting_stack {
> @@ -80,6 +86,7 @@ struct ref_formatting_state {
> struct atom_value {
> const char *s;
> struct align *align;
> + struct contents *contents;
Same question as for 'align': Does 'contents' need to be
heap-allocated because it must exist beyond the lifetime of
'atom_value'? If not, making it just a plain member of 'atom_value'
would simplify memory management (no need to free it).
Also, will 'align' and 'contents' ever be used at the same time? If
not, it might make sense to place them in a 'union' (not for the
memory saving, but to make it clear to the reader that their use is
mutually exclusive).
More below.
> void (*handler)(struct atom_value *atomv, struct ref_formatting_state
> *state);
> unsigned long ul; /* used for sorting when not FIELD_STR */
> };
> @@ -569,6 +576,61 @@ static void find_subpos(const char *buf, unsigned long
> sz,
> *nonsiglen = *sig - buf;
> }
>
> +/*
> + * If 'lines' is greater than 0, append that many lines from the given
> + * object_id 'oid' to the given strbuf.
> + */
> +static void append_tag_lines(struct strbuf *out, const struct object_id
> *oid, int lines)
> +{
> + int i;
> + unsigned long size;
> + enum object_type type;
> + char *buf, *sp, *eol;
> + size_t len;
> +
> + buf = read_sha1_file(oid->hash, &type, &size);
> + if (!buf)
> + die_errno("unable to read object %s", oid_to_hex(oid));
> + if (type != OBJ_COMMIT && type != OBJ_TAG)
> + goto free_return;
> + if (!size)
> + die("an empty %s object %s?",
> + typename(type), oid_to_hex(oid));
> +
> + /* skip header */
> + sp = strstr(buf, "\n\n");
> + if (!sp)
> + goto free_return;
> +
> + /* only take up to "lines" lines, and strip the signature from a tag
> */
> + if (type == OBJ_TAG)
> + size = parse_signature(buf, size);
> + for (i = 0, sp += 2; i < lines && sp < buf + size; i++) {
> + if (i)
> + strbuf_addstr(out, "\n");
> + eol = memchr(sp, '\n', size - (sp - buf));
> + len = eol ? eol - sp : size - (sp - buf);
> + strbuf_add(out, sp, len);
> + if (!eol)
> + break;
> + sp = eol + 1;
> + }
> +free_return:
> + free(buf);
> +}
I understand that you want to re-use this code from
tag.c:show_tag_lines(), but (from a very cursory read) isn't this
duplicating logic and processing already done elsewhere in
ref-filter.c? More about this below.
> +
> +static void contents_lines_handler(struct atom_value *atomv, struct
> ref_formatting_state *state)
> +{
> + struct contents *contents = (struct contents *)atomv->contents;
Why is this cast needed?
> + struct strbuf s = STRBUF_INIT;
> +
> + append_tag_lines(&s, &contents->oid, contents->lines);
> + quote_formatting(&state->stack->output, s.buf, state->quote_style);
> + strbuf_release(&s);
> +
> + free(contents);
> +}
> @@ -588,7 +651,8 @@ static void grab_sub_body_contents(struct atom_value
> *val, int deref, struct obj
> strcmp(name, "contents") &&
> strcmp(name, "contents:subject") &&
> strcmp(name, "contents:body") &&
> - strcmp(name, "contents:signature"))
> + strcmp(name, "contents:signature") &&
> + !starts_with(name, "contents:lines="))
> continue;
> if (!subpos)
> find_subpos(buf, sz,
> @@ -608,6 +672,15 @@ static void grab_sub_body_contents(struct atom_value
> *val, int deref, struct obj
> v->s = xmemdupz(sigpos, siglen);
> else if (!strcmp(name, "contents"))
> v->s = xstrdup(subpos);
> + else if (skip_prefix(name, "contents:lines=", &valp)) {
> + struct contents *contents = xmalloc(sizeof(struct
> contents));
> +
> + if (strtoul_ui(valp, 10, &contents-
Re: [FEATURE REQUEST] Filter-branch extend progress with a simple estimated time remaning
On Sun, Aug 30, 2015 at 4:04 AM, Mikael Magnusson wrote:
> On Sun, Aug 30, 2015 at 5:15 AM, Eric Sunshine
> wrote:
>> On Sat, Aug 29, 2015 at 11:00 PM, Gabor Bernat wrote:
>>> Reading after it, I think the most close we can get with this is, awk
>>> 'BEGIN { print strftime("%c", 1271603087); }; and just ignore setting
>>> this value (and avoid displaying it) if that fails too. Do you agree?
>>
>> strftime() in awk is a GNU-ism. It doesn't exist in awk on Mac OS X or
>> FreeBSD, or even the default awk on Linux (which is mawk on Linux
>> installations I've checked).
>>
>> Most portable likely would be Perl, however, that's probably too
>> heavyweight inside a loop like this, even if called only once each N
>> iterations.
>
> http://stackoverflow.com/questions/2445198/get-seconds-since-epoch-in-any-posix-compliant-shell
> Found this,
>
> awk 'BEGIN{srand();print srand()}'
>
> srand() in awk returns the previous seed value, and calling it without
> an argument sets it to time of day, so the above sequence should
> return seconds since the epoch, or at least something in seconds that
> is relative to a fixed point which is all that's needed in this
> thread.
Indeed, this seems to be portable in my tests, and presumably works on
Solaris, whereas "date +%s" doesn't (according to that stackoverflow
answer).
> this can work instead of the data command for getting the time
> elapsed, however for getting the actual date of a timestamp is not
> possible generally; so I think I will just remove that part.
Agreed. I was going to suggest the same.
--
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 v14 07/13] ref-filter: add support for %(contents:lines=X)
On Sun, Aug 30, 2015 at 1:02 PM, Karthik Nayak wrote:
> On Sun, Aug 30, 2015 at 1:23 PM, Eric Sunshine
> wrote:
>> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak
>> wrote:
>>> struct atom_value {
>>> const char *s;
>>> struct align *align;
>>> + struct contents *contents;
>>
>> Same question as for 'align': Does 'contents' need to be
>> heap-allocated because it must exist beyond the lifetime of
>> 'atom_value'? If not, making it just a plain member of 'atom_value'
>> would simplify memory management (no need to free it).
>
> In this context that makes sense, as the value is only needed for the
> current atom value.
>
>> Also, will 'align' and 'contents' ever be used at the same time? If
>> not, it might make sense to place them in a 'union' (not for the
>> memory saving, but to make it clear to the reader that their use is
>> mutually exclusive).
>
> Not quite sure if it needs to be mutually exclusive (isn't that up to the
> user?)
> But this can be done, as they're separate atoms and at a time only one of them
> is used.
I meant "mutually exclusive" in the sense of only one or the other of
'align' and 'contents' ever being used within a single 'atom_value'
instance. (I wasn't referring to the user experience.)
--
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: [FEATURE REQUEST] Filter-branch extend progress with a simple estimated time remaning
On Sun, Aug 30, 2015 at 12:52 PM, Junio C Hamano wrote:
>>> http://stackoverflow.com/questions/2445198/get-seconds-since-epoch-in-any-posix-compliant-shell
>>> Found this,
>>>
>>> awk 'BEGIN{srand();print srand()}'
>>>
>>> srand() in awk returns the previous seed value, and calling it without
>>> an argument sets it to time of day, so the above sequence should
>>> return seconds since the epoch, or at least something in seconds that
>>> is relative to a fixed point which is all that's needed in this
>>> thread.
>
> In practice this should work, but it makes me feel somewhat uneasy.
>
> POSIX says "Set the seed value for rand to expr or use the time of
> day if expr is omitted. The previous seed value shall be returned."
> but I do not see anything that says that "the time of day" is
> counted in seconds around there (which is the crucial bit for this
> application).
>
> http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html
> (4.15 Seconds since the Epoch) says "The relationship between the
> actual time of day and the current value for seconds since the Epoch
> is unspecified."
I suppose a viable approach might be to test once outside the loop if
"date +%s" is supported and print the "(%d elapsed / %d estimated
remaining)" annotation within the loop if it is, else not. The test
might look something like this:
echo $(date +%s) | grep -q '^[0-9][0-9]*$' 2>/dev/null && show_eta=t
Platforms, such as Linux, Mac OS X, and FreeBSD, which support "date
+%s" would get the annotated output, whereas it would fall back
gracefully to the non-annotated output on platforms such as Solaris
(and perhaps AIX) which lack it.
--
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: [FEATURE REQUEST] Filter-branch extend progress with a simple estimated time remaning
(please don't top-post on this list)
On Sun, Aug 30, 2015 at 12:58 PM, Gabor Bernat wrote:
> I would argue against the every n commit check, or at least making it
> configurable, as in my case the speed is something between 0.01 and
> 1.5 seconds per commit. Checking it every n commit would make it I
> feel quite slow to adapt. But it's debatable.
I'm wondering why these two decisions ("showing estimated time" and
"frequency of the computation") should be put in the hands of the user
in the first place.
1. Why have a --progress-eta option at all as opposed to just enabling
it unconditionally if the platform can support it ("date +%s") and if
it can be done without impacting the overall runtime noticeably?
2. Why make the user responsible for deciding how often to do the time
check (via some configuration) as opposed to adjusting it dynamically
based upon how quickly the operation is proceeding. That is, if the
filter-branch operation is zipping along at 0.01 seconds per commit,
then the time check can be done less frequently (say every 50 or 100
commits) so that it doesn't slow the overall operation. Conversely, if
the operation is molasses, moving at 2 seconds per commit, then doing
the time check more frequently (perhaps after each commit) probably
won't noticeably impact the user's perception of the operation's
progress.
> On 8/30/15, Junio C Hamano wrote:
>> Eric Sunshine writes:
>>
>>>>> Most portable likely would be Perl, however, that's probably too
>>>>> heavyweight inside a loop like this, even if called only once each N
>>>>> iterations.
>>
>> I think that is true. Now, when it is too heavy to spawn perl,
>> would it be light enough to spawn awk, I have to wonder. Even if
>> the implementation uses awk, I think the time measurement should be
>> done only once each N iterations (e.g. every 1000 commits measure
>> the rate and divide the remaining commits with that rate while
>> displaying the progress; if you are chewing 100 commits per minute
>> and have 2000 commits to go, you know it will take 20 more minutes).
>>
>>>> http://stackoverflow.com/questions/2445198/get-seconds-since-epoch-in-any-posix-compliant-shell
>>>> Found this,
>>>>
>>>> awk 'BEGIN{srand();print srand()}'
>>>>
>>>> srand() in awk returns the previous seed value, and calling it without
>>>> an argument sets it to time of day, so the above sequence should
>>>> return seconds since the epoch, or at least something in seconds that
>>>> is relative to a fixed point which is all that's needed in this
>>>> thread.
>>
>> In practice this should work, but it makes me feel somewhat uneasy.
>>
>> POSIX says "Set the seed value for rand to expr or use the time of
>> day if expr is omitted. The previous seed value shall be returned."
>> but I do not see anything that says that "the time of day" is
>> counted in seconds around there (which is the crucial bit for this
>> application).
>>
>> http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html
>> (4.15 Seconds since the Epoch) says "The relationship between the
>> actual time of day and the current value for seconds since the Epoch
>> is unspecified."
--
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: [FEATURE REQUEST] Filter-branch extend progress with a simple estimated time remaning
On Sun, Aug 30, 2015 at 03:40:20PM -0400, Eric Sunshine wrote:
> I suppose a viable approach might be to test once outside the loop if
> "date +%s" is supported and print the "(%d elapsed / %d estimated
> remaining)" annotation within the loop if it is, else not. The test
> might look something like this:
>
> echo $(date +%s) | grep -q '^[0-9][0-9]*$' 2>/dev/null && show_eta=t
Actually, you'd also want to suppress 'date' errors via /dev/null, so
perhaps:
{ echo $(date +%s) | grep -q '^[0-9][0-9]*$'; } 2>/dev/null && show_eta=t
or something.
> Platforms, such as Linux, Mac OS X, and FreeBSD, which support "date
> +%s" would get the annotated output, whereas it would fall back
> gracefully to the non-annotated output on platforms such as Solaris
> (and perhaps AIX) which lack it.
--
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: Add passed and estimated seconds to the filter-branch on demand via --progress-eta flag
On Sun, Aug 30, 2015 at 7:45 AM, Gabor Bernat wrote:
> I've submitted this first to this list as a feature request, however
> in the meantime with the help of Jeff King , Junio C
> Hamano , Eric Sunshine
> and Mikael Magnusson came up with solution, so now
> I submit it as a patch. Here follows a patch, I really hope I got
> right the format:
Thanks for the patch. Since the actual implementation is still under
discussion[1], I'll just comment on the properties of the patch
itself...
In order to allow "git-am --scissors" to extract the patch
automatically from your email, you'd want to separate the above
commentary from the below patch with a scissor line "-- >8 --".
Alternately (and more commonly), you can make git-am happy by placing
the commentary (such as the above text) below the "---" line just
under your sign-off and before the diffstat.
[1]: http://thread.gmane.org/gmane.comp.version-control.git/276531
> From 620e69d10a1bfcfcace347cbb95991d75fd23a1d Mon Sep 17 00:00:00 2001
> From: Gabor Bernat
> Date: Thu, 27 Aug 2015 00:46:52 +0200
When including a patch in a mail message like this (separated by a "--
>8--" line), you'd normally drop these headers. "From" isn't
meaningful outside your own repository, and "From:" (with colon) and
"Date:" are normally inferred from the email itself. In this case,
however, use of "From:" (with a colon) is appropriate since it differs
from the email address of your message.
An alternative is to use git-send-email to take care of the mail
header situation for you automatically.
> Subject: [PATCH] Add to the git filter-branch a --progress-eta flag which when
> enabled will print with the progress also the number of seconds passed since
> started plus the number of seconds predicted until the operation finishes.
A commit message should consist a one-line brief explanation of the
change (shorter than 72 characters), followed by a blank line,
followed by one or more paragraphs explaining the change. The first
line should also be prefixed by the area the patch is touching, is
typically not capitalized, and lacks a full-stop (period). For
example:
filter-branch: add --progress-eta option to estimate completion time
In addition to reporting how many commits have been processed and
how many remain, when --progress-eta is enabled, also report how
much time has elapsed and an estimate of how much remains.
> Signed-off-by: Gabor Bernat
> ---
This is where you'd place commentary such as that which is currently
at the top of this email (as well as the small comment at the very
bottom), particularly if you are using git-send-email to send the
patch.
Unfortunately, this patch is severely whitespace damaged, as if it had
been pasted into the message via Gmail's user-interface. Using
git-send-email can help avoid such damage.
> Documentation/git-filter-branch.txt | 6 ++
> git-filter-branch.sh| 29 -
> 2 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-filter-branch.txt
> b/Documentation/git-filter-branch.txt
> index 73fd9e8..6ca9d6e 100644
> --- a/Documentation/git-filter-branch.txt
> +++ b/Documentation/git-filter-branch.txt
> @@ -194,6 +194,12 @@ to other tags will be rewritten to point to the
> underlying commit.
> directory or when there are already refs starting with
> 'refs/original/', unless forced.
>
> +--progress-eta::
> + If specified will print the number of seconds elapsed and the predicted
> + count of seconds remaining until the operation is expected to finish. Note
> + that for calculating the eta the global speed up to the current point is
> + used.
> +
> ...::
> Arguments for 'git rev-list'. All positive refs included by
> these options are rewritten. You may also specify options
> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index 5b3f63d..7b565fb 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -105,6 +105,7 @@ filter_subdir=
> orig_namespace=refs/original/
> force=
> prune_empty=
> +progress_eta=
> remap_to_ancestor=
> while :
> do
> @@ -129,6 +130,11 @@ do
> prune_empty=t
> continue
> ;;
> + --progress-eta)
> + shift
> + progress_eta=t
> + continue
> + ;;
> -*)
> ;;
> *)
> @@ -277,9 +283,30 @@ test $commits -eq 0 && die "Found nothing to rewrite"
> # Rewrite the commits
>
> git_filter_branch__commit_count=0
> +
> +case "$progress_eta" in
> + t)
> + start=$(PATH=`getconf PATH` awk 'BEGIN{srand();print srand()}')
> + ;;
> + '')
> + ;;
> +esac
> +
> while read commit par
Re: [PATCH v14 04/13] ref-filter: implement an `align` atom
On Sun, Aug 30, 2015 at 10:57 AM, Karthik Nayak wrote:
> On Sun, Aug 30, 2015 at 8:57 AM, Eric Sunshine
> wrote:
>>> +struct align {
>>> + align_type position;
>>> + unsigned int width;
>>> };
>>>
>>> #define REF_FORMATTING_STATE_INIT { 0, NULL }
>>> @@ -69,6 +79,8 @@ struct ref_formatting_state {
>>>
>>> struct atom_value {
>>> const char *s;
>>> + struct align *align;
>>
>> Why does 'align' need to be heap-allocated rather than just being a
>> direct member of 'atom_value'? Does 'align' need to exist beyond the
>> lifetime of its 'atom_value'? If not, making it a direct member might
>> simplify resource management (no need to free it).
>
> But it does, since we carry over the contents of align from atom_value to
> cb_data of ref_formatting_stack and that holds the value until we read
> the %(end)
> atom hence it seemed like a better choice to allocate it on the heap
So, you're saying that the 'atom_value' instance no longer exists at
the point that processing of %(end) needs to access the alignment
properties? If so, then heap allocation make sense. 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
Re: [PATCH v14 04/13] ref-filter: implement an `align` atom
On Sun, Aug 30, 2015 at 9:38 AM, Karthik Nayak wrote:
> On Sun, Aug 30, 2015 at 8:57 AM, Eric Sunshine
> wrote:
>> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak
>> wrote:
>>> +static void append_atom(struct atom_value *v, struct ref_formatting_state
>>> *state)
>>> +{
>>> + /*
>>> +* Quote formatting is only done when the stack has a single
>>> +* element. Otherwise quote formatting is done on the
>>> +* element's entire output strbuf when the %(end) atom is
>>> +* encountered.
>>> +*/
>>> + if (!state->stack->prev)
>>
>> With the disclaimer that I wasn't following the quoting discussion
>> closely: Is this condition going to be sufficient for all cases, such
>> as an %(if:) atom? That is, if you have:
>>
>> %(if:notempty)%(bloop)%(then) --option=%(bloop)%(end)
>>
>> isn't the intention that, %(bloop) within the %(then) section should
>> be quoted but not the literal "--option="?
>>
>> The condition `!state->stack->prev' would be insufficient to handle
>> this if %(if:) pushes one or more states onto the stack, no? This
>> implies that you might want an explicit flag for enabling/disabling
>> quoting rather than relying upon the state of the stack, and that
>> individual atom handlers would control that flag.
>
>> Or, am I misunderstanding due to not having followed the discussion closely?
>
> Yes this wont be work for what you've said.
>
> To sum up the discussion:
> We didn't want atoms within the %(align)%(end) to be quoted separately
> rather the whole aligned buffer to be quoted at the end.
>
> But this does conflict with %(if)...%(end). So it makes sense to change it to
> only have checks for %(align) atom being used.
If I understand Junio's response correctly, then this doesn't sound
correct either. Rather than imbuing only %(align:) with special
quoting knowledge, it sounds like quoting should be handled
generically for all top-level atoms and pseudo-atoms, including %(if:)
and others which may come later.
(But perhaps I'm still misunderstanding...)
> So probably:
>
> static void append_atom(struct atom_value *v, struct
> ref_formatting_state *state)
> {
> if (state->stack->at_end == align_handler)
This couples append_atom() far too tightly with the %(align:) atom. If
you really need to do this sort of special-casing, then it probably
would make more sense to have an explicit flag saying whether or not
quoting should be done, rather than tying it specifically to the
%(align:) atom.
However, (again, if I'm understanding Junio's response), your original
`!state->stack->prev' condition might be sufficient after all.
> strbuf_addstr(&state->stack->output, v->s);
> else
> quote_formatting(&state->stack->output, v->s, state->quote_style);
> }
>
> and
>
> static void end_atom_handler(struct atom_value *atomv, struct
> ref_formatting_state *state)
> {
> struct ref_formatting_stack *current = state->stack;
> struct strbuf s = STRBUF_INIT;
>
> if (!current->at_end)
> die(_("format: `end` atom used without a supporting atom"));
> current->at_end(current);
> /*
> * Whenever we have more than one stack element that means we
> * are using a align modifier atom. In that case we need to
> * perform quote formatting.
> */
> if (current->at_end == align_handler) {
Ditto about being too tightly coupled to %(align:). Such logic should
likely be generic for any such atom.
> quote_formatting(&s, current->output.buf, state->quote_style);
> strbuf_reset(¤t->output);
> strbuf_addbuf(¤t->output, &s);
> }
> strbuf_release(&s);
> pop_stack_element(&state->stack);
> }
>>> @@ -725,6 +818,37 @@ static void populate_value(struct ref_array_item *ref)
>>> else
>>> v->s = " ";
>>> continue;
>>> + } else if (!strcmp(name, "align"))
>>> + die(_("format: incomplete use of the `align`
>>> atom"));
>>
>> Why does %(align) get flagged as a malformation of %(align:), whereas
>> %(color) does not get flagged as a malformation of %(color:)? Why does
>> one deserve special treatment but not the other?
>
> Didn't see that, I think its needed to add a check for both like :
>
> else if (!strcmp(name, "align") || !strcmp(name, "color"))
> die(_("format: improper usage of %s atom"), name);
>
> I had a look if any other atoms need a subvalue to operate, couldn't
> find any.
Hmm, I'm not convinced that either %(align) or %(color) need to be
called out specially. What is the current behavior when these
"malformations" or any other misspelled atoms are used? Does it error
out? Does it simply ignore them and pass them through to the output
unmolested?
--
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 v14 07/13] ref-filter: add support for %(contents:lines=X)
On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak wrote:
> In 'tag.c' we can print N lines from the annotation of the tag using
> the '-n' option. Copy code from 'tag.c' to 'ref-filter' and
> modify it to support appending of N lines from the annotation of tags
> to the given strbuf.
>
> Implement %(contents:lines=X) where X lines of the given object are
> obtained.
>
> Add documentation and test for the same.
>
> Signed-off-by: Karthik Nayak
> ---
> @@ -608,6 +672,15 @@ static void grab_sub_body_contents(struct atom_value
> *val, int deref, struct obj
> v->s = xmemdupz(sigpos, siglen);
> else if (!strcmp(name, "contents"))
> v->s = xstrdup(subpos);
> + else if (skip_prefix(name, "contents:lines=", &valp)) {
> + struct contents *contents = xmalloc(sizeof(struct
> contents));
> +
> + if (strtoul_ui(valp, 10, &contents->lines))
> + die(_("positive width expected align:%s"),
> valp);
I forgot to mention this when I reviewed the patch earlier[1], but you
copied this error message a bit too literally from the %(align:) atom.
[1]: http://article.gmane.org/gmane.comp.version-control.git/276807
> + hashcpy(contents->oid.hash, obj->sha1);
> + v->handler = contents_lines_handler;
> + v->contents = contents;
> + }
> }
> }
--
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 v14 04/13] ref-filter: implement an `align` atom
On Sun, Aug 30, 2015 at 1:27 PM, Junio C Hamano wrote:
> Eric Sunshine writes:
>> With the disclaimer that I wasn't following the quoting discussion
>> closely: Is this condition going to be sufficient for all cases, such
>> as an %(if:) atom? That is, if you have:
>>
>> %(if:notempty)%(bloop)%(then) --option=%(bloop)%(end)
>>
>> isn't the intention that, %(bloop) within the %(then) section should
>> be quoted but not the literal "--option="?
>
> I think you'll see that the intention of the above is to quote the
> entirty of the result of %(if...)...%(end) if you read the previous
> discussion. The "quoting" is used when you say you are making --format
> write a script in specified programming language, e.g.
>
> for-each-ref --shell --format='
> a=%(atom) b=%(if...)...%(end)
> do interesting things using $a and $b here
> ' | sh
>
> You are correct to point out in the earlier part of your message I
> am responding to that %(align) is not special and any nested thing
> including %(if) will uniformly trigger the same "usually each atom
> is quoted separately, but with this opening atom, everything up to
> the matching end atom is evaluated first and then the result is
> quoted" logic.
So, if I'm understanding correctly, the semantic behavior of the
current patch seems to be more or less correct, but the implementation
(and commit message) place perhaps too much emphasis on specializing
quoting suppression only for %(align:), whereas it could/should be
generalized?
I am a bit concerned about this code from end_atom_handler():
/*
* Whenever we have more than one stack element that means we
* are using a certain modifier atom. In that case we need to
* perform quote formatting.
*/
if (state->stack->prev) {
quote_formatting(&s, current->output.buf, state->quote_style);
strbuf_reset(¤t->output);
strbuf_addbuf(¤t->output, &s);
}
Aren't both the comment and the condition backward? Shouldn't quoting
be done only for the top-most state on the stack rather than every
state other than the top-most? That is, shouldn't the condition be
`!state->stack->prev' as it is in append_atom()?
--
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 v6 1/2] worktree: add 'for_each_worktree' function
Thanks for working on this. I apologize for not reviewing earlier
rounds (other than v2 [1]); it's been difficult to find a block of
time to do so...
On Sun, Aug 30, 2015 at 3:10 PM, Michael Rappazzo wrote:
> for_each_worktree iterates through each worktree and invokes a callback
> function. The main worktree (if not bare) is always encountered first,
> followed by worktrees created by `git worktree add`.
Why does this iteration function specially filter out a bare main
worktree? If it instead unconditionally vends the main worktree (bare
or not), then the caller can make its own decision about what to do
with it, thus empowering the caller, rather than imposing a (possibly)
arbitrary restriction upon it.
For instance, the "git worktree list" command may very well want to
show the main worktree, even if bare (possibly controlled by a
command-line option), annotated appropriately ("[bare]"). This may be
exactly the sort of information a user wants to know, and by leaving
the decision up to the caller, then the caller ("git worktree list" in
this example) has the opportunity to act accordingly, whereas if
for_each_worktree() filters out a bare main worktree unconditionally,
then the caller ("git worktree list") will never be able to offer such
an option.
> If the callback function returns a non-zero value, iteration stops, and
> the callback's return value is returned from the for_each_worktree
> function.
Stepping back a bit, is a for-each-foo()-style interface desirable?
This sort of interface imposes a good deal of complexity on callers,
demanding a callback function and callback data (cb_data), and is
generally (at least in C) more difficult to reason about than other
simpler interfaces. Is such complexity warranted?
An alternate, much simpler interface would be to have a function, say
get_worktrees(), return an array of 'worktree' structures to the
caller, which the caller would iterate over (which is a common
operation in C, thus easily reasoned about).
The one benefit of a for-each-foo()-style interface is that it's
possible to "exit early", thus avoiding the cost of interrogating
meta-data for worktrees in which the caller is not interested,
however, it seems unlikely that there will be so many worktrees linked
to a repository for this early exit to translate into any real
savings.
> Signed-off-by: Michael Rappazzo
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 430b51e..7b3cb96 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -26,6 +26,14 @@ static int show_only;
> static int verbose;
> static unsigned long expire;
>
> +/*
> + * The signature for the callback function for the for_each_worktree()
> + * function below. The memory pointed to by the callback arguments
> + * is only guaranteed to be valid for the duration of a single
> + * callback invocation.
> + */
> +typedef int each_worktree_fn(const char *path, const char *git_dir, void
> *cb_data);
In my review[1] of v2, I mentioned that (at some point) the "git
worktree list" command might like to show additional information about
the worktree other than just its location. Such information might
include its tag, the checked out branch (or detached HEAD), whether
it's locked (and the lock reason and whether the worktree is currently
"online"), whether it can be pruned (and the prune reason).
Commands such as "git worktree add" and "git checkout" need to know if
a branch is already checked out in some (other) worktree, thus they
also need the "branch" information.
This need by clients for additional worktree meta-data suggests that
the additional information ought to be encapsulated into a 'struct
worktree', and that for_each_worktree() should vend a 'struct
worktree' rather than vending merely the "git dir". (Alternately, the
above-suggested get_worktrees() should return an array of 'struct
worktree' items.)
An important reason for making for_each_worktree() vend a 'struct
worktree' is that it facilitates centralizing all the logic for
determining and computing the extra worktree meta-data in one place,
thus relieving callers of burden of having to compute the extra
information themselves. (Junio alluded to this in his v5 review[2].)
> static int prune_worktree(const char *id, struct strbuf *reason)
> {
> struct stat st;
> @@ -359,6 +367,81 @@ static int add(int ac, const char **av, const char
> *prefix)
> return add_worktree(path, branch, &opts);
> }
>
> +/*
> + * Iterate through each worktree and invoke the callback function. If
> + * the callback function returns non-zero, the iteration stops, and
> + * this function returns that return value
> + */
> +static int for_each_worktree(each_worktree_fn fn, void *cb_data)
Ultimately, code outside of builtin/worktree.c will want to benefit
from the encapsulation of this worktree iteration logic. For instance,
the support code in (top-level) branch.c invoked by "git worktree add"
and "git checkout" to determine if a branch i
Re: [PATCH v6 2/2] worktree: add 'list' command
On Sun, Aug 30, 2015 at 3:10 PM, Michael Rappazzo wrote:
> 'git worktree list' uses the for_each_worktree function to iterate,
> and outputs the worktree dir. With the verbose option set, it also
> shows the branch or revision currently checked out in that worktree.
>
> Signed-off-by: Michael Rappazzo
> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index fb68156..867a24a 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -11,6 +11,7 @@ SYNOPSIS
> [verse]
> 'git worktree add' [-f] [--detach] [-b ] []
> 'git worktree prune' [-n] [-v] [--expire ]
> +'git worktree list' [-v|--verbose]
For conciseness and consistency with the "git worktree prune"
synopsis, this should mention only -v (and omit --verbose).
> DESCRIPTION
> ---
> @@ -88,11 +93,13 @@ OPTIONS
>
> -v::
> --verbose::
> - With `prune`, report all removals.
> + With `prune`, report all removals.
> + With `list` show the branch or revision currently checked out in that
> worktree.
Add a comma after "With `list`".
Also: s/in that/in each/
This new line is overly long; wrapping it over a couple lines would help.
> --expire ::
> With `prune`, only expire unused working trees older than .
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 7b3cb96..6d96cdf 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> +static int print_worktree_details(const char *path, const char *git_dir,
> void *cb_data)
> +{
> + printf("%s", path);
> + if (verbose) {
> + enter_repo(git_dir, 1);
> + printf(" [");
> + unsigned char sha1[20];
> + int flag;
> +
> + const char *refname = resolve_ref_unsafe("HEAD", 0, sha1,
> &flag);
Declarations after statement. Move the declarations of 'sha1', 'flag',
and 'refname' above the statements (enter_repo() and printf()).
> + if (!(flag & REF_ISSYMREF)) {
> + printf("%s", find_unique_abbrev(sha1,
> DEFAULT_ABBREV));
> + } else {
> + refname = shorten_unambiguous_ref(refname, 0);
> + printf("%s", refname);
> + }
As mentioned in my review[1] of patch 1/2, it would be better for this
sort of logic to be handled by the worktree iterator itself so that
all callers can benefit, rather than having to repeat the work in each
caller. This information would be encapsulated in a 'struct worktree'
along with 'path' and 'git_dir' and other interesting information
vended by the the iterator function.
> + printf("]");
Rather than dropping printf()'s here and there to compose the output
piecemeal, it would be cleaner, and facilitate future changes, to
assign the refname/sha1 to a variable, and interpolate that variable
into an all-encompasing printf(), like this:
printf(" [%s]", branch_or_sha1);
> + }
> + printf("\n");
> +
> + return 0;
> +}
[1]: http://article.gmane.org/gmane.comp.version-control.git/276847
--
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 v14 04/13] ref-filter: implement an `align` atom
On Mon, Aug 31, 2015 at 5:55 AM, Karthik Nayak wrote:
> On Sun, Aug 30, 2015 at 3:10 PM, Eric Sunshine
> wrote:
>> On Sun, Aug 30, 2015 at 9:38 AM, Karthik Nayak wrote:
>>> On Sun, Aug 30, 2015 at 8:57 AM, Eric Sunshine
>>> wrote:
>>>> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak
>>>> wrote:
>>>>> + } else if (!strcmp(name, "align"))
>>>>> + die(_("format: incomplete use of the `align`
>>>>> atom"));
>>>>
>>>> Why does %(align) get flagged as a malformation of %(align:), whereas
>>>> %(color) does not get flagged as a malformation of %(color:)? Why does
>>>> one deserve special treatment but not the other?
>>>
>>> Didn't see that, I think its needed to add a check for both like :
>>>
>>> else if (!strcmp(name, "align") || !strcmp(name, "color"))
>>> die(_("format: improper usage of %s atom"), name);
>>>
>>> I had a look if any other atoms need a subvalue to operate, couldn't
>>> find any.
>>
>> Hmm, I'm not convinced that either %(align) or %(color) need to be
>> called out specially. What is the current behavior when these
>> "malformations" or any other misspelled atoms are used? Does it error
>> out? Does it simply ignore them and pass them through to the output
>> unmolested?
>
> It just simply ignores them currently, which is kinda bad, as the user
> is given no warning, and the atom is ineffective.
Warning about unrecognized atoms may indeed be a good idea, however,
the current behavior isn't a huge problem since user discovers the
error when the output fails to match his expectation. This behavior of
ignoring unrecognized atoms predates your work, doesn't it? If so,
it's probably not something you need to address in this series.
--
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 v14 04/13] ref-filter: implement an `align` atom
On Mon, Aug 31, 2015 at 1:28 PM, Matthieu Moy
wrote:
> Eric Sunshine writes:
>> On Mon, Aug 31, 2015 at 5:55 AM, Karthik Nayak wrote:
>>> On Sun, Aug 30, 2015 at 3:10 PM, Eric Sunshine
>>> wrote:
>>>> On Sun, Aug 30, 2015 at 9:38 AM, Karthik Nayak
>>>> wrote:
>>>>> On Sun, Aug 30, 2015 at 8:57 AM, Eric Sunshine
>>>>> wrote:
>>>>>> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak
>>>>>> wrote:
>>>>>>> + } else if (!strcmp(name, "align"))
>>>>>>> + die(_("format: incomplete use of the `align`
>>>>>>> atom"));
>>>>>>
>>>>>> Why does %(align) get flagged as a malformation of %(align:), whereas
>>>>>> %(color) does not get flagged as a malformation of %(color:)? Why does
>>>>>> one deserve special treatment but not the other?
>>>>>
>>>>> Didn't see that, I think its needed to add a check for both like :
>>>>>
>>>>> else if (!strcmp(name, "align") || !strcmp(name, "color"))
>>>>> die(_("format: improper usage of %s atom"), name);
>>>>>
>>>>> I had a look if any other atoms need a subvalue to operate, couldn't
>>>>> find any.
>>>>
>>>> Hmm, I'm not convinced that either %(align) or %(color) need to be
>>>> called out specially. What is the current behavior when these
>>>> "malformations" or any other misspelled atoms are used? Does it error
>>>> out? Does it simply ignore them and pass them through to the output
>>>> unmolested?
>>>
>>> It just simply ignores them currently, which is kinda bad, as the user
>>> is given no warning, and the atom is ineffective.
>>
>> Warning about unrecognized atoms may indeed be a good idea, however,
>> the current behavior isn't a huge problem since user discovers the
>> error when the output fails to match his expectation.
>
> It's a bit worse than it seems: without this change, using --format
> '%(align)%(end)' results in Git complaining about %(end) without
> matching atom, which is really confusing since you do have a %(align) (I
> got it for real while testing a preliminary version).
>
>> This behavior of ignoring unrecognized atoms predates your work,
>> doesn't it? If so, it's probably not something you need to address in
>> this series.
>
> I wouldn't insist in having it in the series, but now that it's here, I
> think we can keep it (if only to shorten the interdiff for the next
> iteration).
The unstated subtext in my original question is that the approach
taken by this patch of warning only about unrecognized %(align) is too
special-case; if it's going to warn at all, it should do so
generically for any unrecognized %(atom). Special-casing the warning
about malformed %(align) sets a poor precedent; it's code which will
eventually need to be removed anyhow when the generic warning
mechanism is eventually implemented.
--
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 v6 1/2] worktree: add 'for_each_worktree' function
On Mon, Aug 31, 2015 at 2:44 PM, David Turner wrote: > On Mon, 2015-08-31 at 01:11 -0400, Eric Sunshine wrote: >> Stepping back a bit, is a for-each-foo()-style interface desirable? >> This sort of interface imposes a good deal of complexity on callers, >> demanding a callback function and callback data (cb_data), and is >> generally (at least in C) more difficult to reason about than other >> simpler interfaces. Is such complexity warranted? >> >> An alternate, much simpler interface would be to have a function, say >> get_worktrees(), return an array of 'worktree' structures to the >> caller, which the caller would iterate over (which is a common >> operation in C, thus easily reasoned about). >> >> The one benefit of a for-each-foo()-style interface is that it's >> possible to "exit early", thus avoiding the cost of interrogating >> meta-data for worktrees in which the caller is not interested, >> however, it seems unlikely that there will be so many worktrees linked >> to a repository for this early exit to translate into any real >> savings. > > The other benefit is that there is no need to worry about deallocating > the list. But that might be too minor to worry about. Probably. The burden of having to deallocate the returned array seems quite minor compared to the complexity of the callback function approach. Also, unstated but implied with the suggestion of a get_worktrees() function was that there would be a corresponding free_worktrees() function to make cleanup easy. -- 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 v6 1/2] worktree: add 'for_each_worktree' function
On Mon, Aug 31, 2015 at 2:57 PM, Mike Rappazzo wrote:
> On Mon, Aug 31, 2015 at 1:11 AM, Eric Sunshine
> wrote:
>> On Sun, Aug 30, 2015 at 3:10 PM, Michael Rappazzo wrote:
>> Why does this iteration function specially filter out a bare main
>> worktree? If it instead unconditionally vends the main worktree (bare
>> or not), then the caller can make its own decision about what to do
>> with it, thus empowering the caller, rather than imposing a (possibly)
>> arbitrary restriction upon it.
>>
>> For instance, the "git worktree list" command may very well want to
>> show the main worktree, even if bare (possibly controlled by a
>> command-line option), annotated appropriately ("[bare]"). This may be
>> exactly the sort of information a user wants to know, and by leaving
>> the decision up to the caller, then the caller ("git worktree list" in
>> this example) has the opportunity to act accordingly, whereas if
>> for_each_worktree() filters out a bare main worktree unconditionally,
>> then the caller ("git worktree list") will never be able to offer such
>> an option.
>
> I wasn't sure that a bare repo would be considered a worktree. I
> don't think that it would be
> a good idea to include it. In the same vein that I can't checkout a
> branch in a bare repo, it
> figure that it shouldn't be in the list.
This is a mechanism vs. policy issue[1]. Low-level worker code, such
as this iteration function, should concern itself only with the
mechanics of retrieving and vending the worktree meta-data, and should
not make decisions (policy) about how that information is used by the
caller. Policy decisions (how the meta-data is used or displayed)
should be pushed to as high a level as possible, often up to the level
of user-interface (which is what "git worktree list" is).
[1]: http://www.catb.org/esr/writings/taoup/html/ch01s06.html#id287
>> Stepping back a bit, is a for-each-foo()-style interface desirable?
>> This sort of interface imposes a good deal of complexity on callers,
>> demanding a callback function and callback data (cb_data), and is
>> generally (at least in C) more difficult to reason about than other
>> simpler interfaces. Is such complexity warranted?
>>
>> An alternate, much simpler interface would be to have a function, say
>> get_worktrees(), return an array of 'worktree' structures to the
>> caller, which the caller would iterate over (which is a common
>> operation in C, thus easily reasoned about).
>>
>> The one benefit of a for-each-foo()-style interface is that it's
>> possible to "exit early", thus avoiding the cost of interrogating
>> meta-data for worktrees in which the caller is not interested,
>> however, it seems unlikely that there will be so many worktrees linked
>> to a repository for this early exit to translate into any real
>> savings.
>
> I am not opposed to making a simple function as you describe. I think David
> was
> looking for a callback style function. I don't think it would be
> terrible to keep the
> callback and then also include the simple function to return the
> struct array. I like
> the memory management of the callback better than the struct array though.
We should stick with one or the other. Having both complicates the
code unnecessarily and increases maintenance costs. I, personally,
prefer the get_worktrees() approach for its client-side simplicity.
With a corresponding free_worktrees() to dispose of the resources
allocated by get_worktrees(), memory management shouldn't be much of a
burden for callers (other than having to remember to call it).
>> I may have missed some discussion in earlier rounds (or perhaps I'm
>> too simple-minded), but I'm confused about why this logic (and most of
>> the rest of the function) differs so much from existing logic in
>> branch.c:find_shared_symref() and find_linked_symref() for iterating
>> over the worktrees and gleaning information about them. That logic in
>> branch.c seems to do a pretty good job of reporting the worktree in
>> which a branch is already checked out, so it's not clear why the above
>> logic takes a different (and seemingly more complex) approach.
>
> This is due to my unfamiliarity of the code api. I keep trying to
> look for the right
> functions to use, but I miss them. Sorry. I will rework using those
> functions.
The API is indeed large and complex, and it can be difficult to get a
handle on how to accomplish various tasks. That's also a good argument
for re-using the existing (proven) code in
branch.c:find_shared_symref() and find_link
Re: [PATCH v6 1/2] worktree: add 'for_each_worktree' function
On Mon, Aug 31, 2015 at 2:57 PM, Mike Rappazzo wrote: > I wasn't sure that a bare repo would be considered a worktree. I > don't think that it would be > a good idea to include it. In the same vein that I can't checkout a > branch in a bare repo, it > figure that it shouldn't be in the list. I forgot to mention in my previous response that I have the opposite view, and think that a bare repo should be included in the output of "git worktree list". The reason is that the intention of "git worktree list" is to give the user a consolidated view of the locations of all components of his "workspace". By "workspace", I mean the repository (bare or not) and its worktrees. In the typical case, the .git directory resides within the main worktree (the first item output by "git worktree list"), thus is easily found, however, if "git worktree list" hides bare repos, then the user will have no way to easily locate the repository (without resorting to lower-level commands or peeking at configuration files). -- 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 v6 1/2] worktree: add 'for_each_worktree' function
On Mon, Aug 31, 2015 at 3:54 PM, Mike Rappazzo wrote: > On Mon, Aug 31, 2015 at 3:47 PM, Eric Sunshine > wrote: >> On Mon, Aug 31, 2015 at 2:57 PM, Mike Rappazzo wrote: >>> I wasn't sure that a bare repo would be considered a worktree. I >>> don't think that it would be >>> a good idea to include it. In the same vein that I can't checkout a >>> branch in a bare repo, it >>> figure that it shouldn't be in the list. >> >> I forgot to mention in my previous response that I have the opposite >> view, and think that a bare repo should be included in the output of >> "git worktree list". The reason is that the intention of "git worktree >> list" is to give the user a consolidated view of the locations of all >> components of his "workspace". By "workspace", I mean the repository >> (bare or not) and its worktrees. >> >> In the typical case, the .git directory resides within the main >> worktree (the first item output by "git worktree list"), thus is >> easily found, however, if "git worktree list" hides bare repos, then >> the user will have no way to easily locate the repository (without >> resorting to lower-level commands or peeking at configuration files). > > This makes sense, but would we also want to decorate it in the `git > worktree list` > command? Would porcelain list output be able to differentiate it? I don't have strong feelings about decorating the bare repository, if by "decorate", you mean adding a "[bare]" annotation or something. Verbose mode can certainly do so. For porcelain mode, we can (and should) be explicit about it. -- 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/3] submodule: implement `module_list` as a builtin helper
On Mon, Aug 31, 2015 at 3:19 PM, Stefan Beller wrote:
> Most of the submodule operations work on a set of submodules.
> Calculating and using this set is usually done via:
>
>module_list "$@" | {
>while read mode sha1 stage sm_path
>do
> # the actual operation
>done
>}
>
> Currently the function `module_list` is implemented in the
> git-submodule.sh as a shell script wrapping a perl script.
> The rewrite is in C, such that it is faster and can later be
> easily adapted when other functions are rewritten in C.
>
> git-submodule.sh similar to the builtin commands will navigate
> to the top most directory of the repository and keeping the
> subdirectories as a variable.
ECANNOTPARSE
Did you mean
s/git-submodule.sh/&,/
s/commands/&,/
s/top most/top-most/
s/keeping/keep/
s/subdirectories/subdirectory/
?
> As the helper is called from
> within the git-submodule.sh script, we are already navigated
> to the root level, but the path arguments are stil relative
s/stil/still/
> to the subdirectory we were in when calling git-submodule.sh.
> That's why there is a `--prefix` option pointing to an alternative
> path where to anchor relative path arguments.
s/where to/at which to/
More below.
> Signed-off-by: Stefan Beller
> Signed-off-by: Junio C Hamano
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> new file mode 100644
> index 000..beaab7d
> --- /dev/null
> +++ b/builtin/submodule--helper.c
> @@ -0,0 +1,114 @@
> +#include "builtin.h"
> +#include "cache.h"
> +#include "parse-options.h"
> +#include "quote.h"
> +#include "pathspec.h"
> +#include "dir.h"
> +#include "utf8.h"
> +
> +static const struct cache_entry **ce_entries;
> +static int ce_alloc, ce_used;
> +static const char *alternative_path;
Why is 'alternative_path' declared at file scope?
> +static int module_list_compute(int argc, const char **argv,
> + const char *prefix,
> + struct pathspec *pathspec)
> +{
> + int i;
> + char *max_prefix, *ps_matched = NULL;
> + int max_prefix_len;
> + parse_pathspec(pathspec, 0,
> + PATHSPEC_PREFER_FULL |
> + PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
> + prefix, argv);
> +
> + /* Find common prefix for all pathspec's */
> + max_prefix = common_prefix(pathspec);
> + max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
> +
> + if (pathspec->nr)
> + ps_matched = xcalloc(pathspec->nr, 1);
> +
> + if (read_cache() < 0)
> + die("index file corrupt");
> +
> + for (i = 0; i < active_nr; i++) {
> + const struct cache_entry *ce = active_cache[i];
> +
> + if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),
> + max_prefix_len, ps_matched,
> + S_ISGITLINK(ce->ce_mode) |
> S_ISDIR(ce->ce_mode)))
> + continue;
> +
> + if (S_ISGITLINK(ce->ce_mode)) {
> + ALLOC_GROW(ce_entries, ce_used + 1, ce_alloc);
> + ce_entries[ce_used++] = ce;
> + }
> +
> + while (i + 1 < active_nr && !strcmp(ce->name, active_cache[i
> + 1]->name))
> + /*
> +* Skip entries with the same name in different stages
> +* to make sure an entry is returned only once.
> +*/
> + i++;
> + }
> + free(max_prefix);
> +
> + if (ps_matched && report_path_error(ps_matched, pathspec, prefix))
> + return -1;
> +
> + return 0;
Does 'ps_matched' need to be freed before these two 'return's?
> +}
> +
> +static int module_list(int argc, const char **argv, const char *prefix)
> +{
> + int i;
> + static struct pathspec pathspec;
Drop 'static'.
> +
> + struct option module_list_options[] = {
> + OPT_STRING(0, "prefix", &alternative_path,
> + N_("path"),
> + N_("alternative anchor for relative paths")),
> + OPT_END()
> + };
> +
> + static const char * const git_submodule_helper_usage[] = {
You can drop this 'static' too.
Style: *const
> + N_("git submodule--helper module_list [--prefix=]
> [...]"),
> + NULL
> + };
> +
> + argc = parse_options(argc, argv, prefix, module_list_options,
> +git_submodule_helper_usage, 0);
> +
> + if (module_list_compute(argc, argv, alternative_path
> + ? alternative_path
> + : prefix, &pathspec) < 0) {
> + printf("#unmatched\n");
> + return 1;
> + }
> +
> + for (i
Re: [PATCH 2/3] submodule: implement `module_name` as a builtin helper
On Mon, Aug 31, 2015 at 3:19 PM, Stefan Beller wrote:
> This implements the helper `module_name` in C instead of shell,
> yielding a nice performance boost.
>
> Signed-off-by: Stefan Beller
> Signed-off-by: Junio C Hamano
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index beaab7d..c8f7e0c 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -101,6 +104,26 @@ static int module_list(int argc, const char **argv,
> const char *prefix)
> +static int module_name(int argc, const char **argv, const char *prefix)
> +{
> + const char *name;
> + const struct submodule *sub;
> +
> + if (argc != 1)
> + usage("git submodule--helper module_name \n");
> +
> + gitmodules_config();
> + sub = submodule_from_path(null_sha1, argv[0]);
> +
> + if (!sub)
> + die("No submodule mapping found in .gitmodules for path
> '%s'", argv[0]);
In the original shell code, this error message went through
eval_gettext(), so don't you want:
die(_("No ..."), ...);
?
> + name = sub->name;
> + printf("%s\n", name);
Why the useless assignment to 'name'? Instead:
printf("%s\n", sub->name);
> + return 0;
> +}
> +
> int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> {
> if (argc < 2)
> @@ -109,6 +132,9 @@ int cmd_submodule__helper(int argc, const char **argv,
> const char *prefix)
> if (!strcmp(argv[1], "module_list"))
> return module_list(argc - 1, argv + 1, prefix);
>
> + if (!strcmp(argv[1], "module_name"))
> + return module_name(argc - 2, argv + 2, prefix);
> +
> usage:
> - usage("git submodule--helper module_list\n");
> + usage("git submodule--helper [module_list | module_name]\n");
> }
--
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 3/3] submodule: implement `module_clone` as a builtin helper
On Mon, Aug 31, 2015 at 3:19 PM, Stefan Beller wrote:
> `module_clone` is part of the update command,
> which I want to convert to C next.
Hmm, place commentary below "---".
> Signed-off-by: Stefan Beller
> Signed-off-by: Junio C Hamano
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index c8f7e0c..d29499c 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -124,6 +125,147 @@ static int module_name(int argc, const char **argv,
> const char *prefix)
> return 0;
> }
>
> +static int clone_submodule(const char *path, const char *gitdir, const char
> *url,
> + const char *depth, const char *reference, int
> quiet)
> +{
> + struct child_process cp;
> + child_process_init(&cp);
> +
> + argv_array_push(&cp.args, "clone");
> + argv_array_push(&cp.args, "--no-checkout");
> + if (quiet)
> + argv_array_push(&cp.args, "--quiet");
> + if (depth && strcmp(depth, "")) {
More idiomatic:
if (depth && *depth) {
> + argv_array_push(&cp.args, "--depth");
> + argv_array_push(&cp.args, depth);
These would be easier to read if you consolidate them with argv_array_pushl():
argv_array_pushl(&cp.args, "--depth", depth, NULL);
and it would allow you to drop the noisy braces.
> + }
> + if (reference && strcmp(reference, "")) {
if (reference && *reference) {
> + argv_array_push(&cp.args, "--reference");
> + argv_array_push(&cp.args, reference);
argv_array_pushl(&cp.args, "--reference", reference, NULL);
> + }
> + if (gitdir) {
Why is this case different than the others, in that it doesn't check
for the zero-length string ""?
> + argv_array_push(&cp.args, "--separate-git-dir");
> + argv_array_push(&cp.args, gitdir);
argv_array_pushl(&cp.args, "--separate-git-dir", gitdir, NULL);
> + }
> + argv_array_push(&cp.args, url);
> + argv_array_push(&cp.args, path);
> +
> + cp.git_cmd = 1;
> + cp.env = local_repo_env;
> +
> + cp.no_stdin = 1;
> + cp.no_stdout = 1;
> + cp.no_stderr = 1;
> +
> + return run_command(&cp);
> +}
> +
> +static int module_clone(int argc, const char **argv, const char *prefix)
> +{
> + const char *path = NULL, *name = NULL, *url = NULL;
> + const char *reference = NULL, *depth = NULL;
> + int quiet = 0;
> + FILE *submodule_dot_git;
> + const char *sm_gitdir, *p;
Why is 'sm_gitdir' declared const even though it's always pointing at
memory which this function owns and needs to free? If you drop the
'const', then you won't have to cast it to non-const in a couple
places below.
> + struct strbuf rel_path = STRBUF_INIT;
> + struct strbuf sb = STRBUF_INIT;
> +
> + struct option module_update_options[] = {
> + OPT_STRING(0, "prefix", &alternative_path,
> + N_("path"),
> + N_("alternative anchor for relative paths")),
> + OPT_STRING(0, "path", &path,
> + N_("path"),
> + N_("where the new submodule will be cloned to")),
> + OPT_STRING(0, "name", &name,
> + N_("string"),
> + N_("name of the new submodule")),
> + OPT_STRING(0, "url", &url,
> + N_("string"),
> + N_("url where to clone the submodule from")),
> + OPT_STRING(0, "reference", &reference,
> + N_("string"),
> + N_("reference repository")),
> + OPT_STRING(0, "depth", &depth,
> + N_("string"),
> + N_("depth for shallow clones")),
> + OPT_END()
> + };
> +
> + static const char * const git_submodule_helper_usage[] = {
You can drop this 'static'.
> + N_("git submodule--helper update [--prefix=] [--quiet]
> [--remote] [-N|--no-fetch]"
> + "[-f|--force] [--rebase|--merge] [--reference
> ]"
> + "[--depth ] [--recursive] [--] [...]"),
> + NULL
> + };
> +
> + argc = parse_options(argc, argv, prefix, module_update_options,
> +git_submodule_helper_usage, 0);
> +
> + if (getenv("GIT_QUIET"))
> + quiet = 1;
I understand that you're simply replicating the behavior of the shell
code, but this is "yuck".
'module_clone' is only called from two places in git-submodule.sh, so
how about a cleanup patch which makes 'module_clone' accept a --quiet
flag and have the two callers pass it explicitly? Finally, have this C
replacement accept --quiet as a proper command-line option rather than
via this hidden backdoor method.
> + strbuf_addf(&sb, "%s/modules/%s", get_git_d
Re: [PATCHv3 0/3] submodule--helper: Have some refactoring only patches first
On Mon, Aug 31, 2015 at 8:40 PM, Stefan Beller wrote: > * Implemented different error messages as suggested by Junio > * Implemented all of Erics suggestions, > including renaming to module-with-dash-now In the future, please include an interdiff showing changes between versions. Thanks. > Stefan Beller (3): > submodule: implement `module-list` as a builtin helper > submodule: implement `module-name` as a builtin helper > submodule: implement `module-clone` as a builtin helper -- 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: [PATCHv3 1/3] submodule: implement `module-list` as a builtin helper
On Mon, Aug 31, 2015 at 8:40 PM, Stefan Beller wrote:
> Most of the submodule operations work on a set of submodules.
> Calculating and using this set is usually done via:
>
>module_list "$@" | {
>while read mode sha1 stage sm_path
>do
> # the actual operation
>done
>}
>
> Currently the function `module_list` is implemented in the
> git-submodule.sh as a shell script wrapping a perl script.
> The rewrite is in C, such that it is faster and can later be
> easily adapted when other functions are rewritten in C.
>
> git-submodule.sh, similar to the builtin commands, will navigate
> to the top-most directory of the repository and keep the
> subdirectory as a variable. As the helper is called from
> within the git-submodule.sh script, we are already navigated
> to the root level, but the path arguments are still relative
> to the subdirectory we were in when calling git-submodule.sh.
> That's why there is a `--prefix` option pointing to an alternative
> path which to anchor relative path arguments.
>
> Signed-off-by: Stefan Beller
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> new file mode 100644
> index 000..44310f5
> --- /dev/null
> +++ b/builtin/submodule--helper.c
> +static int module_list_compute(int argc, const char **argv,
> + const char *prefix,
> + struct pathspec *pathspec)
> +{
> + int i, result = 0;
> + char *max_prefix, *ps_matched = NULL;
> + int max_prefix_len;
> + parse_pathspec(pathspec, 0,
> + PATHSPEC_PREFER_FULL |
> + PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
> + prefix, argv);
> +
> + /* Find common prefix for all pathspec's */
> + max_prefix = common_prefix(pathspec);
> + max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
> +
> + if (pathspec->nr)
> + ps_matched = xcalloc(pathspec->nr, 1);
> +
> + if (read_cache() < 0)
> + die("index file corrupt");
die(_("..."));
> + for (i = 0; i < active_nr; i++) {
> + const struct cache_entry *ce = active_cache[i];
> +
> + if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),
> + max_prefix_len, ps_matched,
> + S_ISGITLINK(ce->ce_mode) |
> S_ISDIR(ce->ce_mode)))
> + continue;
> +
> + if (S_ISGITLINK(ce->ce_mode)) {
> + ALLOC_GROW(ce_entries, ce_used + 1, ce_alloc);
> + ce_entries[ce_used++] = ce;
> + }
> +
> + while (i + 1 < active_nr && !strcmp(ce->name, active_cache[i
> + 1]->name))
> + /*
> +* Skip entries with the same name in different stages
> +* to make sure an entry is returned only once.
> +*/
> + i++;
> + }
> + free(max_prefix);
> +
> + if (ps_matched && report_path_error(ps_matched, pathspec, prefix))
> + result = -1;
> +
> + free(ps_matched);
> +
> + return result;
> +}
> +
> +static int module_list(int argc, const char **argv, const char *prefix)
> +{
> + int i;
> + struct pathspec pathspec;
> + const char *alternative_path;
> +
> + struct option module_list_options[] = {
> + OPT_STRING(0, "prefix", &alternative_path,
> + N_("path"),
> + N_("alternative anchor for relative paths")),
> + OPT_END()
> + };
> +
> + const char *const git_submodule_helper_usage[] = {
> + N_("git submodule--helper module_list [--prefix=]
> [...]"),
s/module_list/module-list/
> + NULL
> + };
> +
> + argc = parse_options(argc, argv, prefix, module_list_options,
> +git_submodule_helper_usage, 0);
> +
> + if (module_list_compute(argc, argv, alternative_path
> + ? alternative_path
> + : prefix, &pathspec) < 0) {
> + printf("#unmatched\n");
> + return 1;
> + }
> +
> + for (i = 0; i < ce_used; i++) {
> + const struct cache_entry *ce = ce_entries[i];
> +
> + if (ce_stage(ce))
> + printf("%06o %s U\t", ce->ce_mode,
> sha1_to_hex(null_sha1));
> + else
> + printf("%06o %s %d\t", ce->ce_mode,
> sha1_to_hex(ce->sha1), ce_stage(ce));
> +
> + utf8_fprintf(stdout, "%s\n", ce->name);
> + }
> + return 0;
> +}
> +
> +int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> +{
> + if (argc < 2)
> + die(N_("fatal: submodule--helper subcommand must
Re: [PATCHv3 2/3] submodule: implement `module-name` as a builtin helper
On Mon, Aug 31, 2015 at 8:40 PM, Stefan Beller wrote:
> This implements the helper `module_name` in C instead of shell,
> yielding a nice performance boost.
>
> Before this patch, I measured a time (best out of three):
>
> $ time ./t7400-submodule-basic.sh >/dev/null
> real0m11.066s
> user0m3.348s
> sys 0m8.534s
>
> With this patch applied I measured (also best out of three)
>
> $ time ./t7400-submodule-basic.sh >/dev/null
> real0m10.063s
> user0m3.044s
> sys 0m7.487s
>
> Signed-off-by: Stefan Beller
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 44310f5..91bd420 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -102,16 +105,38 @@ static int module_list(int argc, const char **argv,
> const char *prefix)
> +static int module_name(int argc, const char **argv, const char *prefix)
> +{
> + const struct submodule *sub;
> +
> + if (argc != 1)
> + usage("git submodule--helper module_name \n");
usage(_("..."));
s/module_name/module-name/
I think you also need to drop the newline from the usage() argument.
> + gitmodules_config();
> + sub = submodule_from_path(null_sha1, argv[0]);
> +
> + if (!sub)
> + die(N_("No submodule mapping found in .gitmodules for path
> '%s'"),
> + argv[0]);
> +
> + printf("%s\n", sub->name);
> +
> + return 0;
> +}
> +
> int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> {
> if (argc < 2)
> die(N_("fatal: submodule--helper subcommand must be called
> with "
> - "a subcommand, which is module-list\n"));
> + "a subcommand, which are module-list, module-name\n"));
Manually maintaining this list is likely to become a maintenance issue
pretty quickly. Isn't there an OPT_CMDMODE for this sort of thing?
Also, it would like be more readable if the possible commands were
printed one per line rather than all squashed together.
> if (!strcmp(argv[1], "module-list"))
> return module_list(argc - 1, argv + 1, prefix);
>
> + if (!strcmp(argv[1], "module-name"))
> + return module_name(argc - 2, argv + 2, prefix);
> +
> die(N_("fatal: '%s' is not a valid submodule--helper subcommand, "
> - "which is module-list\n"),
> + "which are module-list, module-name\n"),
And, it's duplicated here, making it even more of a maintenance problem.
> argv[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
Re: [PATCHv3 3/3] submodule: implement `module-clone` as a builtin helper
On Mon, Aug 31, 2015 at 8:40 PM, Stefan Beller wrote:
> This converts implements the helper `module_clone`. This functionality is
"converts implements"?
> needed for converting `git submodule update` later on, which we want to
> add threading to.
>
> Signed-off-by: Stefan Beller
> ---
> +static int module_clone(int argc, const char **argv, const char *prefix)
> +{
> + const char *path = NULL, *name = NULL, *url = NULL;
> + const char *reference = NULL, *depth = NULL;
> + const char *alternative_path = NULL, *p;
> + int quiet = 0;
> + FILE *submodule_dot_git;
> + char *sm_gitdir;
> + struct strbuf rel_path = STRBUF_INIT;
> + struct strbuf sb = STRBUF_INIT;
> +
> + struct option module_update_options[] = {
s/update/clone/ maybe?
> + OPT_STRING(0, "prefix", &alternative_path,
> + N_("path"),
> + N_("alternative anchor for relative paths")),
> + OPT_STRING(0, "path", &path,
> + N_("path"),
> + N_("where the new submodule will be cloned to")),
> + OPT_STRING(0, "name", &name,
> + N_("string"),
> + N_("name of the new submodule")),
> + OPT_STRING(0, "url", &url,
> + N_("string"),
> + N_("url where to clone the submodule from")),
> + OPT_STRING(0, "reference", &reference,
> + N_("string"),
> + N_("reference repository")),
> + OPT_STRING(0, "depth", &depth,
> + N_("string"),
> + N_("depth for shallow clones")),
> + OPT__QUIET(&quiet, "Suppress output for cloning a submodule"),
> + OPT_END()
> + };
> +
> + static const char * const git_submodule_helper_usage[] = {
You can still drop 'static'.
> + N_("git submodule--helper update [--prefix=] [--quiet]
> [--remote] [-N|--no-fetch]"
"update"?
> + "[-f|--force] [--rebase|--merge] [--reference
> ]"
> + "[--depth ] [--recursive] [--] [...]"),
Also, what's all this "--force", "--rebase", "--remote", "--recursive" stuff?
> + NULL
> + };
> +
> + argc = parse_options(argc, argv, prefix, module_update_options,
> +git_submodule_helper_usage, 0);
> +
> + strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);
> + sm_gitdir = strbuf_detach(&sb, NULL);
> +
> + if (!file_exists(sm_gitdir)) {
> + safe_create_leading_directories_const(sm_gitdir);
Still inconsistent checking of return value of
safe_create_leading_directories_const() in this function.
In fact, it looks like pretty much all of my v2 review comments are
still relevant to the remainder of this function. Rather than
repeating them, I'll just give a reference[1].
[1]: http://article.gmane.org/gmane.comp.version-control.git/276952
> + if (clone_submodule(path, sm_gitdir, url, depth, reference,
> quiet))
> + die(N_("Clone of '%s' into submodule path '%s'
> failed"),
> + url, path);
> + } else {
> + safe_create_leading_directories_const(path);
> + unlink(sm_gitdir);
> + }
> +
> + /* Write a .git file in the submodule to redirect to the
> superproject. */
> + if (alternative_path && !strcmp(alternative_path, "")) {
> + p = relative_path(path, alternative_path, &sb);
> + strbuf_reset(&sb);
> + } else
> + p = path;
> +
> + if (safe_create_leading_directories_const(p) < 0)
> + die("Could not create directory '%s'", p);
> +
> + strbuf_addf(&sb, "%s/.git", p);
> +
> + if (safe_create_leading_directories_const(sb.buf) < 0)
> + die(_("could not create leading directories of '%s'"),
> sb.buf);
> + submodule_dot_git = fopen(sb.buf, "w");
> + if (!submodule_dot_git)
> + die ("Cannot open file '%s': %s", sb.buf, strerror(errno));
> +
> + fprintf(submodule_dot_git, "gitdir: %s\n",
> + relative_path(sm_gitdir, path, &rel_path));
> + if (fclose(submodule_dot_git))
> + die("Could not close file %s", sb.buf);
> + strbuf_reset(&sb);
> +
> + /* Redirect the worktree of the submodule in the superprojects config
> */
> + if (!is_absolute_path(sm_gitdir)) {
> + char *s = (char*)sm_gitdir;
> + if (strbuf_getcwd(&sb))
> + die_errno("unable to get current working directory");
> + strbuf_addf(&sb, "/%s", sm_gitdir);
> + sm_gitdir = strbuf_detach(&sb, NULL);
> + free(s);
> + }
> +
> + if (strbuf_getcwd(&sb))
> + die_errno("unab
Re: [PATCH 3/3] submodule: implement `module_clone` as a builtin helper
On Tue, Sep 1, 2015 at 1:49 PM, Stefan Beller wrote:
> took all suggestions except the one below.
>
>>
>> if (strbuf_getcwd(&sb))
>> die_errno(...);
>> strbuf_addf(&sb, "/%s, sm_gitdir);
>> free(sm_gitdir);
>> sm_gitdir = strbuf_detach(&sb, NULL);
>>
>>> + }
>>> +
>>> + if (strbuf_getcwd(&sb))
>>> + die_errno("unable to get current working directory");
>>
>> The conditional block just above here also gets 'cwd'. If you move
>> this code above the !is_absolute_path(sm_gitdir) conditional block,
>> then you can avoid the duplication.
>
> I don't get it. We need to have strbuf_getcwd twice no matter how we
> arrange the code
> as we strbuf_detach will empty the strbuf.
>
> Do you mean to call strbuf_getcwd just once and then xstrdup the value,
> then reset the strbuf to just contain the cwd and append the other string ?
Sorry, I have no idea what you are asking.
All I am saying is that you're unnecessarily invoking getcwd() twice.
It's value doesn't change between invocations, so the second call is
entirely redundant and wasteful. You can instead call it just once and
use the result in both places.
--
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: [PATCHv4 3/3] submodule: implement `clone` as a builtin helper
On Tue, Sep 1, 2015 at 2:24 PM, Stefan Beller wrote:
> This converts implements the helper `module_clone`. This functionality is
Mentioned previously[1]: "converts implements"?
[1]: http://article.gmane.org/gmane.comp.version-control.git/276966
> needed for converting `git submodule update` later on, which we want to
> add threading to.
>
> Signed-off-by: Stefan Beller
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index f5e408a..63f535a 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> +static int module_clone(int argc, const char **argv, const char *prefix)
> +{
> + const char *path = NULL, *name = NULL, *url = NULL;
> + const char *reference = NULL, *depth = NULL;
> + const char *alternative_path = NULL, *p;
> + int quiet = 0;
> + FILE *submodule_dot_git;
> + char *sm_gitdir;
> + struct strbuf rel_path = STRBUF_INIT;
> + struct strbuf sb = STRBUF_INIT;
> +
> + struct option module_clone_options[] = {
> + OPT_STRING(0, "prefix", &alternative_path,
> + N_("path"),
> + N_("alternative anchor for relative paths")),
> + OPT_STRING(0, "path", &path,
> + N_("path"),
> + N_("where the new submodule will be cloned to")),
> + OPT_STRING(0, "name", &name,
> + N_("string"),
> + N_("name of the new submodule")),
> + OPT_STRING(0, "url", &url,
> + N_("string"),
> + N_("url where to clone the submodule from")),
> + OPT_STRING(0, "reference", &reference,
> + N_("string"),
> + N_("reference repository")),
> + OPT_STRING(0, "depth", &depth,
> + N_("string"),
> + N_("depth for shallow clones")),
> + OPT__QUIET(&quiet, "Suppress output for cloning a submodule"),
> + OPT_END()
> + };
> +
> + const char * const git_submodule_helper_usage[] = {
Style: *const
> + N_("git submodule--helper clone [--prefix=] [--quiet] "
> + "[--reference ] [--name ] [--url ]"
> + "[--depth ] [--] [...]"),
> + NULL
> + };
> +
> + argc = parse_options(argc, argv, prefix, module_clone_options,
> +git_submodule_helper_usage, 0);
> +
> + strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);
> + sm_gitdir = strbuf_detach(&sb, NULL);
> +
> + if (!file_exists(sm_gitdir)) {
> + if (safe_create_leading_directories_const(sm_gitdir) < 0)
> + die(_("could not create directory '%s'"), sm_gitdir);
> + if (clone_submodule(path, sm_gitdir, url, depth, reference,
> quiet))
> + die(N_("clone of '%s' into submodule path '%s'
> failed"),
> + url, path);
s/N_/_/
> + } else {
> + if (safe_create_leading_directories_const(path) < 0)
> + die(_("could not create directory '%s'"), path);
> + if (unlink(sm_gitdir) < 0)
> + die_errno(_("failed to delete '%s'"), sm_gitdir);
> + }
> +
> + /* Write a .git file in the submodule to redirect to the
> superproject. */
> + if (alternative_path && *alternative_path)) {
> + p = relative_path(path, alternative_path, &sb);
> + strbuf_reset(&sb);
> + } else
> + p = path;
> +
> + if (safe_create_leading_directories_const(p) < 0)
> + die(_("could not create directory '%s'"), p);
> +
> + strbuf_addf(&sb, "%s/.git", p);
> +
> + if (safe_create_leading_directories_const(sb.buf) < 0)
> + die(_("could not create leading directories of '%s'"),
> sb.buf);
> + submodule_dot_git = fopen(sb.buf, "w");
> + if (!submodule_dot_git)
> + die (_("cannot open file '%s': %s"), sb.buf, strerror(errno));
Style: drop space before '('
Also, can't you use die_errno() here?
> + fprintf(submodule_dot_git, "gitdir: %s\n",
> + relative_path(sm_gitdir, path, &rel_path));
> + if (fclose(submodule_dot_git))
> + die(_("could not close file %s"), sb.buf);
> + strbuf_reset(&sb);
> +
> + /* Redirect the worktree of the submodule in the superproject's
> config */
> + if (strbuf_getcwd(&sb))
> + die_errno(_("unable to get current working directory"));
> +
> + if (!is_absolute_path(sm_gitdir)) {
> + if (strbuf_getcwd(&sb))
> + die_errno(_("unable to get current working
> directory"));
Why does this need to call getcwd() on 'sb' when it was called
immediately above the conditional and
Re: [PATCHv4 2/3] submodule: implement `name` as a builtin helper
On Tue, Sep 1, 2015 at 2:24 PM, Stefan Beller wrote:
> This implements the helper `module_name` in C instead of shell,
You probably want s/module_name/name/ or state more explicitly:
Reimplement `module_name` shell function in C as `name`.
or something.
> yielding a nice performance boost.
>
> Signed-off-by: Stefan Beller
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 16d9abe..f5e408a 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -102,6 +105,24 @@ static int module_list(int argc, const char **argv,
> const char *prefix)
> +static int module_name(int argc, const char **argv, const char *prefix)
> +{
> + const struct submodule *sub;
> +
> + if (argc != 2)
> + usage("git submodule--helper module_name \n");
Mentioned previously[1]:
usage(_("..."));
s/module_name/name/
s/\\n//
[1]: http://article.gmane.org/gmane.comp.version-control.git/276965
> + gitmodules_config();
> + sub = submodule_from_path(null_sha1, argv[1]);
> +
> + if (!sub)
> + die(N_("No submodule mapping found in .gitmodules for path
> '%s'"),
> + argv[1]);
s/N_/_/
s/No/no/
> + printf("%s\n", sub->name);
> +
> + return 0;
> +}
>
> struct cmd_struct {
> const char *cmd;
> @@ -110,6 +131,7 @@ struct cmd_struct {
>
> static struct cmd_struct commands[] = {
> {"list", module_list},
> + {"name", module_name},
> };
--
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: [PATCHv4 3/3] submodule: implement `clone` as a builtin helper
On Tue, Sep 1, 2015 at 3:05 PM, Jeff King wrote:
> On Tue, Sep 01, 2015 at 02:52:54PM -0400, Eric Sunshine wrote:
>> > + /* Redirect the worktree of the submodule in the superproject's
>> > config */
>> > + if (strbuf_getcwd(&sb))
>> > + die_errno(_("unable to get current working directory"));
>> > +
>> > + if (!is_absolute_path(sm_gitdir)) {
>> > + if (strbuf_getcwd(&sb))
>> > + die_errno(_("unable to get current working
>> > directory"));
>>
>> Why does this need to call getcwd() on 'sb' when it was called
>> immediately above the conditional and its value hasn't changed?
>
> Even weirder, the next code is:
>
>> > + strbuf_addf(&sb, "/%s", sm_gitdir);
>> > + free(sm_gitdir);
>> > + sm_gitdir = strbuf_detach(&sb, NULL);
>> > + }
>> > +
>> > +
>> > + strbuf_addf(&sb, "/%s", path);
>
> So if it _was_ an absolute path, we are adding "/$path" to nothing
> (having just detached the strbuf in the conditional above). That seems
> weird.
Indeed, I saw that too, but didn't mention it since this appears to be
an incomplete refactoring from the previous round, and my hope was
that mentioning the getcwd() oddity would be enough to trigger a
thorough check of the code before sending the next version.
--
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 v2 3/6] t6300: introduce test_date() helper
On Tue, Sep 1, 2015 at 5:55 PM, John Keeping wrote:
> This moves the setup of the "expected" file inside the test case. The
> helper function has the advantage that we can use SQ in the file content
> without needing to escape the quotes.
>
> Signed-off-by: John Keeping
> ---
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 7c9bec7..5fdb964 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -146,85 +146,54 @@ test_expect_success 'Check invalid format specifiers
> are errors' '
> test_must_fail git for-each-ref --format="%(authordate:INVALID)"
> refs/heads
> '
>
> -cat >expected <<\EOF
> -'refs/heads/master' 'Mon Jul 3 17:18:43 2006 +0200' 'Mon Jul 3 17:18:44 2006
> +0200'
> -'refs/tags/testtag' 'Mon Jul 3 17:18:45 2006 +0200'
> -EOF
> +test_date () {
> + f=$1
f=$1 &&
> + committer_date=$2 &&
> + author_date=$3 &&
> + tagger_date=$4 &&
> + cat >expected <<-EOF &&
> + 'refs/heads/master' '$committer_date' '$author_date'
> + 'refs/tags/testtag' '$tagger_date'
> + EOF
> + (
> + git for-each-ref --shell --format="%(refname)
> %(committerdate${f:+:$f}) %(authordate${f:+:$f})" refs/heads &&
> + git for-each-ref --shell --format="%(refname)
> %(taggerdate${f:+:$f})" refs/tags
> + ) >actual &&
> + test_cmp expected actual
> +}
--
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: [PATCHv4 1/3] submodule: implement `list` as a builtin helper
On Tue, Sep 1, 2015 at 2:24 PM, Stefan Beller wrote:
> Most of the submodule operations work on a set of submodules.
> Calculating and using this set is usually done via:
>
>module_list "$@" | {
>while read mode sha1 stage sm_path
>do
> # the actual operation
>done
>}
>
> Currently the function `module_list` is implemented in the
> git-submodule.sh as a shell script wrapping a perl script.
> The rewrite is in C, such that it is faster and can later be
> easily adapted when other functions are rewritten in C.
>
> [...]
>
> Signed-off-by: Stefan Beller
> ---
> +struct cmd_struct {
> + const char *cmd;
> + int (*fct)(int, const char **, const char *);
It would be better to stick with a more idiomatic name such as 'f' or
'func' than invent an entirely new one ('fct').
> +};
> +
> +static struct cmd_struct commands[] = {
> + {"list", module_list},
> +};
> +
> +int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> +{
> + int i;
> + if (argc < 2)
> + goto out;
> +
> + for (i = 0; i < ARRAY_SIZE(commands); i++)
> + if (!strcmp(argv[1], commands[i].cmd))
> + return commands[i].fct(argc - 1, argv + 1, prefix);
> +
> +out:
> + if (argc > 1)
> + fprintf(stderr, _("fatal: '%s' is not a valid
> submodule--helper "
> + "subcommand, which are:\n"), argv[1]);
> + else
> + fprintf(stderr, _("fatal: submodule--helper subcommand must
> be "
> + "called with a subcommand, which are:\n"));
> +
> + for (i = 0; i < ARRAY_SIZE(commands); i++)
> + fprintf(stderr, "\t%s\n", commands[i].cmd);
A couple observations:
1. git-submodule--helper isn't the only Git command featuring
subcommands which could benefit from this dispatching and error
reporting code, so making it re-usable would be sensible rather than
hiding it away inside this one (very low-level, not user-facing)
command. If you go that route, it would deserve its own patch series,
and well thought out design and interface. However...
2. I realize that the suggestion of listing available subcommands was
put forth tentatively by Junio[1], but it seems overkill for a command
like this which is not user-facing, and is inconsistent with other
subcommand-bearing commands. At this stage, it should be sufficient
merely to emit a plain error message (without any usage information):
unrecognized subcommand: %s
missing subcommand
at which point the user can consult the man page or "git
subcommand--helper -h" to find out what went wrong.
[1]: http://article.gmane.org/gmane.comp.version-control.git/276935
> +
> + exit(129);
> +}
--
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 v4] git-p4: add config git-p4.pathEncoding
On Wednesday, September 2, 2015, wrote: > Perforce keeps the encoding of a path as given by the originating OS. > Git expects paths encoded as UTF-8. Add a config to tell git-p4 what > encoding Perforce had used for the paths. This encoding is used to > transcode the paths to UTF-8. As an example, Perforce on my Windows > box uses “cp1252” to encode path names. > > Signed-off-by: Lars Schneider > --- > +test_expect_success 'start p4d' ' > + start_p4d > +' > + > +test_expect_success 'Create a repo containing iso8859-1 encoded paths' ' > + cd "$cli" && Torsten, I think, mentioned previously that the 'cd' and code following should be wrapped in a subshell. > + ISO8859="$(printf "$ISO8859_ESCAPED")" && > + echo content123 >"$ISO8859" && > + p4 add "$ISO8859" && > + p4 submit -d "test commit" And, it's odd that this test doesn't "cd ..", which means that subsequent tests are running in directory $cli. Is that intentional? If so, it probably ought to be done in a more explicit and clear fashion, perhaps by having each test do "cd $cli/$git" rather than just "cd $git". > +' > + > +test_expect_success 'Clone repo containing iso8859-1 encoded paths without > git-p4.pathEncoding' ' > + git p4 clone --destination="$git" //depot && > + test_when_finished cleanup_git && > + ( > + cd "$git" && > + UTF8="$(printf "$UTF8_ESCAPED")" && > + echo $UTF8 >expect && > + git -c core.quotepath=false ls-files >actual && > + test_must_fail test_cmp expect actual > + ) > +' > + > +test_expect_success 'Clone repo containing iso8859-1 encoded paths with > git-p4.pathEncoding' ' > + > + test_when_finished cleanup_git && > + ( > + cd "$git" && > + git init . && > + git config git-p4.pathEncoding iso8859-1 && > + git p4 clone --use-client-spec --destination="$git" //depot && > + UTF8="$(printf "$UTF8_ESCAPED")" && > + echo $UTF8 >expect && > + git -c core.quotepath=false ls-files >actual && > + test_cmp expect actual && > + cat >expect <<-\EOF && > + content123 > + EOF > + cat $UTF8 >actual && > + test_cmp expect actual > + ) > +' > + > +test_expect_success 'kill p4d' ' > + kill_p4d > +' > + > +test_done > -- > 1.9.5 (Apple Git-50.3) -- 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 v1] t9821: use test_config
On Thu, Sep 3, 2015 at 5:34 AM, wrote: > From: Lars Schneider > > Signed-off-by: Lars Schneider > --- > diff --git a/t/t9821-git-p4-path-variations.sh > b/t/t9821-git-p4-path-variations.sh > index 81e46ac..5a26fec 100755 > --- a/t/t9821-git-p4-path-variations.sh > +++ b/t/t9821-git-p4-path-variations.sh > @@ -45,7 +45,7 @@ test_expect_success 'Clone root' ' > ( > cd "$git" && > git init . && > - git config core.ignorecase false && > + test_config core.ignorecase false && test_config ensures that the config setting gets "unset" at the end of the test, whether the test succeeds or not, so that subsequent tests are not affected by the setting. However, in this case, since the $git repository gets recreated from scratch for each test anyhow, use of test_config is superfluous. In fact, it may be slightly contraindicated since it could mislead the reader into thinking that state is carried over from test to test. (Not a big objections, but something to take into consideration.) > git p4 clone --use-client-spec --destination="$git" //depot && > # This method is used instead of "test -f" to ensure the case > is > # checked even if the test is executed on case-insensitive > file systems. > @@ -67,7 +67,7 @@ test_expect_success 'Clone root (ignorecase)' ' > ( > cd "$git" && > git init . && > - git config core.ignorecase true && > + test_config core.ignorecase true && > git p4 clone --use-client-spec --destination="$git" //depot && > # This method is used instead of "test -f" to ensure the case > is > # checked even if the test is executed on case-insensitive > file systems. > @@ -91,7 +91,7 @@ test_expect_success 'Clone root and ignore one file' ' > ( > cd "$git" && > git init . && > - git config core.ignorecase false && > + test_config core.ignorecase false && > git p4 clone --use-client-spec --destination="$git" //depot && > # We ignore one file in the client spec and all path cases > change from > # "TO" to "to"! > @@ -113,7 +113,7 @@ test_expect_success 'Clone root and ignore one file > (ignorecase)' ' > ( > cd "$git" && > git init . && > - git config core.ignorecase true && > + test_config core.ignorecase true && > git p4 clone --use-client-spec --destination="$git" //depot && > # We ignore one file in the client spec and all path cases > change from > # "TO" to "to"! > @@ -133,7 +133,7 @@ test_expect_success 'Clone path' ' > ( > cd "$git" && > git init . && > - git config core.ignorecase false && > + test_config core.ignorecase false && > git p4 clone --use-client-spec --destination="$git" //depot && > cat >expect <<-\EOF && > to/File2.txt > @@ -149,7 +149,7 @@ test_expect_success 'Clone path (ignorecase)' ' > ( > cd "$git" && > git init . && > - git config core.ignorecase true && > + test_config core.ignorecase true && > git p4 clone --use-client-spec --destination="$git" //depot && > cat >expect <<-\EOF && > TO/File2.txt > @@ -180,7 +180,7 @@ test_expect_success 'Add a new file and clone path with > new file (ignorecase)' ' > ( > cd "$git" && > git init . && > - git config core.ignorecase true && > + test_config core.ignorecase true && > git p4 clone --use-client-spec --destination="$git" //depot && > cat >expect <<-\EOF && > to/File0.txt > -- > 1.9.5 (Apple Git-50.3) -- 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 v15 05/13] ref-filter: implement an `align` atom
On Tue, Sep 1, 2015 at 2:26 PM, Karthik Nayak wrote: > Implement an `align` atom which left-, middle-, or right-aligns the > content between %(align:..) and %(end). Spell this either %(align:) or %(align:...) with three dots, not two. I, personally, think %(align:) is sufficient. > It is followed by `:,`, where the `` is > either left, right or middle and `` is the size of the area > into which the content will be placed. If the content between > %(align:) and %(end) is more than the width then no alignment is > performed. e.g. to align a refname atom to the middle with a total > width of 40 we can do: --format="%(align:middle,40)%(refname)%(end)". > > We have an `at_end` function for each element of the stack which is to > be called when the `end` atom is encountered. Using this we implement > the aling_handler() for the `align` atom, this aligns the final strbuf > by calling `strbuf_utf8_align()` from utf8.c. > > Ensure that quote formatting is performed on the whole of > %(align)...%(end) rather than individual atoms inside. We skip quote Add colon: %(align:) > formatting for individual atoms when the current stack element is > handling an %(align) atom and perform quote formatting at the end when %(align:) > we encounter the %(end) atom. > > Add documentation and tests for the same. > > Signed-off-by: Karthik Nayak > --- > diff --git a/Documentation/git-for-each-ref.txt > b/Documentation/git-for-each-ref.txt > index e49d578..cac3128 100644 > --- a/Documentation/git-for-each-ref.txt > +++ b/Documentation/git-for-each-ref.txt > @@ -127,6 +127,15 @@ color:: > Change output color. Followed by `:`, where names > are described in `color.branch.*`. > > +align:: > + Left-, middle-, or right-align the content between %(align:..) %(align:) > + and %(end). Followed by `:,`, where the > + `` is either left, right or middle 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. %(align:) Also drop the extra space before "and": s/\s+and/ and/ > In addition to the above, for commit and tag objects, the header > field names (`tree`, `parent`, `object`, `type`, and `tag`) can > be used to specify the value in the header field. -- 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 v15 07/13] ref-filter: add support for %(contents:lines=X)
On Tue, Sep 1, 2015 at 2:26 PM, Karthik Nayak wrote:
> In 'tag.c' we can print N lines from the annotation of the tag using
> the '-n' option. Copy code from 'tag.c' to 'ref-filter' and
> modify it to support appending of N lines from the annotation of tags
> to the given strbuf.
>
> Implement %(contents:lines=X) where X lines of the given object are
> obtained.
>
> Add documentation and test for the same.
>
> Signed-off-by: Karthik Nayak
> ---
> struct atom_value {
> const char *s;
> - struct align align;
> + 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 */
> };
> @@ -226,7 +235,7 @@ static void align_atom_handler(struct atom_value *atomv,
> struct ref_formatting_s
> push_stack_element(&state->stack);
> new = state->stack;
> new->at_end = align_handler;
> - new->cb_data = &atomv->align;
> + new->cb_data = &atomv->u.align;
You could declare atom_value with the union from the start, even if it
has only a single member ('align', at first). Doing so would make this
patch less noisy and wouldn't distract the reader with these seemingly
unrelated changes.
> }
>
> static void end_atom_handler(struct atom_value *atomv, struct
> ref_formatting_state *state)
> @@ -624,6 +633,33 @@ static void find_subpos(const char *buf, unsigned long
> sz,
> *nonsiglen = *sig - buf;
> }
>
> +/*
> + * If 'lines' is greater than 0, append that many lines from the given
> + * 'buf' of length 'size' to the given strbuf.
> + */
> +static void append_lines(struct strbuf *out, const char *buf, unsigned long
> size, int lines)
> +{
> + int i;
> + const char *sp, *eol;
> + size_t len;
> +
> + if ((sp = strstr(buf, "\n\n")) && (sp <= buf + size))
> + size += 2;
Aside from the +2 which Matthieu already questioned, this code has a
much more serious problem. strstr() assumes that 'buf' is
NUL-terminated, however, the fact that buf's size is also being passed
to the function, implies that it may not be NUL-terminated.
Consequently, strstr() could zip well past the end of 'buf', trying to
consult memory not part of 'buf', which is a violation of the C
standard. Even with the band-aid (sp <= buf + size) which tries to
detect this situation, it's still a violation, and could crash if
strstr() attempts to consult memory which hasn't even been allocated
to the process or is otherwise protected in some fashion.
It's possible that 'buf' may actually always be NUL-terminated, but
this code does not convey that fact. If it is known to be
NUL-terminated, then it is safe to use strstr(), however, that should
be shown more clearly either by revising the code or adding an
appropriate comment.
> + sp = buf;
> +
> + for (i = 0; i < lines && sp < buf + size; i++) {
> + if (i)
> + strbuf_addstr(out, "\n");
> + eol = memchr(sp, '\n', size - (sp - buf));
> + len = eol ? eol - sp : size - (sp - buf);
> + strbuf_add(out, sp, len);
> + if (!eol)
> + break;
> + sp = eol + 1;
> + }
> +}
> @@ -663,6 +701,13 @@ static void grab_sub_body_contents(struct atom_value
> *val, int deref, struct obj
> v->s = xmemdupz(sigpos, siglen);
> else if (!strcmp(name, "contents"))
> v->s = xstrdup(subpos);
> + else if (skip_prefix(name, "contents:lines=", &valp)) {
> + struct strbuf s = STRBUF_INIT;
> + if (strtoul_ui(valp, 10, &v->u.contents.lines))
> + die(_("positive width expected
> contents:lines=%s"), valp);
This error message is still too tightly coupled to %(align:), from
which it was copied. "width"?
> + append_lines(&s, subpos, sublen + bodylen - siglen,
> v->u.contents.lines);
> + v->s = strbuf_detach(&s, NULL);
> + }
> }
> }
--
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 v15 07/13] ref-filter: add support for %(contents:lines=X)
On Thu, Sep 3, 2015 at 10:39 AM, Eric Sunshine wrote:
> On Tue, Sep 1, 2015 at 2:26 PM, Karthik Nayak wrote:
>> +static void append_lines(struct strbuf *out, const char *buf, unsigned long
>> size, int lines)
>> +{
>> + if ((sp = strstr(buf, "\n\n")) && (sp <= buf + size))
>> + size += 2;
>
> Aside from the +2 which Matthieu already questioned, this code has a
> much more serious problem. strstr() assumes that 'buf' is
> NUL-terminated, however, the fact that buf's size is also being passed
> to the function, implies that it may not be NUL-terminated.
> Consequently, strstr() could zip well past the end of 'buf', trying to
> consult memory not part of 'buf', which is a violation of the C
> standard. Even with the band-aid (sp <= buf + size) which tries to
> detect this situation, it's still a violation, and could crash if
> strstr() attempts to consult memory which hasn't even been allocated
> to the process or is otherwise protected in some fashion.
>
> It's possible that 'buf' may actually always be NUL-terminated, but
> this code does not convey that fact. If it is known to be
> NUL-terminated, then it is safe to use strstr(), however, that should
> be shown more clearly either by revising the code or adding an
> appropriate comment.
Also, if 'buf' is indeed unconditionally NUL-terminated, then the (sp
<= buf + size) check is wasted code since the result of strstr() will
always be either NULL or pointing somewhere within the NUL-terminated
string.
--
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: [PATCHv5 3/3] submodule: Reimplement `module_clone` shell function in C
On Wed, Sep 2, 2015 at 5:42 PM, Stefan Beller wrote:
> This reimplements the helper function `module_clone` in shell
> in C as `clone`. This functionality is needed for converting
> `git submodule update` later on, which we want to add threading
> to.
>
> Signed-off-by: Stefan Beller
> ---
> +static int module_clone(int argc, const char **argv, const char *prefix)
> +{
> + /* Write a .git file in the submodule to redirect to the
> superproject. */
> + if (safe_create_leading_directories_const(path) < 0)
> + die(_("could not create directory '%s'"), path);
> +
> + if (path && *path)
> + strbuf_addf(&sb, "%s/.git", path);
> + else
> + strbuf_addf(&sb, ".git");
Minor: strbuf_addstr(...);
> + if (safe_create_leading_directories_const(sb.buf) < 0)
> + die(_("could not create leading directories of '%s'"),
> sb.buf);
> + submodule_dot_git = fopen(sb.buf, "w");
> + if (!submodule_dot_git)
> + die_errno(_("cannot open file '%s'"), sb.buf);
> +}
--
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/RFC] Pinning of submodules
On Wed, Sep 2, 2015 at 7:34 PM, Anders Ro wrote:
> Patch to make it possible to pin submodules so that they are not
> affected by the --remote option in "git submodule".
Thanks for the patches. I don't use submodules, so I can't comment
specifically on this change, however, I can offer some general
comments on the patches. But first, a piece of advice...
Use git-send-email to post the patches as proper emails, one email
per patch, rather than as attachments. Reviewers are going to want to
write inline comments on the patches, and they can't do so when the
patches are attachments, so attaching patches discourages reviewers
from responding.
> git-submodule.sh: pin submodule when branch name is '@'
>
> Setting branch name to '@' for a submodule will disable 'git submodule
> update --remote' calls for that specific submodule. I.e. instead of
> follow the unspecified default choice of master, nothing is being
> updated. This is useful when multiple submodules exist but not all
> should follow the remote branch head.
With the disclaimer that I'm not a submodule user (to which the
answer might be obvious): What benefit is there in using a magic
value like this ("@") over, say, an explicit configuration setting?
> Signed-off-by: Anders Ro
> ---
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 25b1ddf..1bb2bb1 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -843,7 +843,8 @@ Maybe you want to use 'update --init'?")"
> die "$(eval_gettext "Unable to find current revision in submodule path
> '\$displaypath'")"
> fi
>
> - if test -n "$remote"
> + # Fetch latest in remote unless branch name in config is '@'
> + if test -n "$remote" -a "$branch" != "@"
The -a option to 'test' is not portable[1] and is considered obsolete
by POSIX[2]. Use "test foo && test bar" instead.
[1]:
http://www.gnu.org/software/autoconf/manual/autoconf.html#index-g_t_0040command_007btest_007d-1793
[2]: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html
> then
> if test -z "$nofetch"
> then
> @@ -857,6 +858,12 @@ Maybe you want to use 'update --init'?")"
> die "$(eval_gettext "Unable to find current ${remote_name}/${branch}
> revision in submodule path '\$sm_path'")"
> fi
>
> + # Inform that the current sm is pinned and use of '--remote' ignored
> + if test -n "$remote" -a "$branch" = "@"
Ditto.
> + then
> + say "$(eval_gettext "Submodule path '\$displaypath' pinned, remote update
> ignored")"
> + fi
> +
> Subject: [PATCH 2/2] t7412: add test case for pinned submodules
>
> Signed-off-by: Anders Ro
> ---
> diff --git a/t/t7412-submodule-pinning.sh b/t/t7412-submodule-pinning.sh
> new file mode 100755
> index 000..2844b1e
> --- /dev/null
> +++ b/t/t7412-submodule-pinning.sh
> @@ -0,0 +1,73 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2015 Anders Ronnbrant
> +#
> +
> +test_description="Branch name '@' disables submodule update --remote calls"
> +
> +. ./test-lib.sh
> +
> +get_sha() {
> + cd $1 && git rev-list --max-count=1 HEAD
> +}
A few issues:
Indent with tabs (width 8), not spaces. (This comment applies to the
entire patch).
Style for shell scripts on this project is to have a space before
"()".
Taking a hint from t/test-lib-functions.sh:test_cmp_rev(), use "git
rev-parse --verify" instead.
It's a bit ugly that this does "cd $1" without ever balancing it with
a return to the original directory. If someone later comes along and
writes:
get_sha1 fiddle >fiddle-id
in a new test, then that person will be surprised to find that the
current working directory changed out from under him. The current
patch doesn't experience this problem since it always invokes it as
$(get_sha1 fiddle), but it could be made more robust by either
wrapping it in a subshell so that the 'cd' is undone when the
subshell exits, or by using "-C $1".
Taking the above comments into account:
get_sha () {
git -C "$1" rev-parse --verify HEAD
}
> +equal_sha() {
> + test "$(get_sha $1)" = "$(get_sha $2)"
> +}
> +
> +not_equal_sha() {
> + test "$(get_sha $1)" != "$(get_sha $2)"
> +}
Style: space before "()" on these two function declarations
> +test_expect_success 'setup submodule tree structure' '
> +for i in 1 2 3; do echo file$i > file$i; git add file$i; git commit -m
> file$i; done &&
Style: write each command of the 'for' loop on its own line,
including 'do', don't use semi-colons
Style: no space after redirection operator: >file$i
Keep the &&-chain intact, and end the chain with "|| return $?" so
that the 'for' loop correctly exits if any contained command fails.
for i in 1 2 3
do
echo file$i >file$i &&
git add file$i &&
git commit -m file$i || return $?
done
> +test_tick &&
Why is this needed? There doesn't seem to be any obvious reason for
its presence, and the test still passes without it.
> +git clone . super &&
> +git clone . follow &&
> +git clone . pinned &&
> +(cd super && git submodule add -b master ../follow follow) &&
> +(
Re: [PATCH] filter-branch: add passed/remaining seconds on progress
On Fri, Sep 4, 2015 at 2:34 PM, Junio C Hamano wrote:
> Gábor Bernát writes:
>> +echo $(date +%s) | grep -q '^[0-9]+$'; 2>/dev/null && show_seconds=t
>
> That is very strange construct. I think you meant to say something
> like
>
> if date '+%s' 2>/dev/null | grep -q '^[0-9][0-9]*$'
> then
> show_seconds=t
> else
> show_seconds=
> fi
The final format suggested[1] for this test was:
{ echo $(date +%s) | grep -q '^[0-9][0-9]*$'; } 2>/dev/null &&
show_eta=t
> A handful of points:
>
> * "echo $(any-command)" is suspect, unless you are trying to let
>the shell munge output from any-command, which is not the case.
Primarily my fault. I don't know what I was thinking when suggesting that.
> * "grep" without -E (or "egrep") takes BRE, which "+" (one or more)
>is not part of.
This seems to have mutated from the suggested form.
> * That semicolon is a syntax error. I think whoever suggested you
>to use it meant to squelch possible errors from "date" that does
>not understand the "%s" format.
This also mutated. The suggested form wanted to suppress errors from
'date' if it complained about "%s", and from 'grep'. In retrospect,
applying it to 'grep' is questionable. I was recalling this warning
from the Autoconf manual[2]:
Some of the options required by Posix are not portable in
practice. Don't use ‘grep -q’ to suppress output, because many
grep implementations (e.g., Solaris) do not support -q. Don't use
‘grep -s’ to suppress output either, because Posix says -s does
not suppress output, only some error messages; also, the -s
option of traditional grep behaved like -q does in most modern
implementations. Instead, redirect the standard output and
standard error (in case the file doesn't exist) of grep to
/dev/null. Check the exit status of grep to determine whether it
found a match.
however, Git tests use 'grep -q' heavily, so perhaps we don't worry about that.
> * I do not think you are clearing show_seconds to empty anywhere,
>so an environment variable the user may have when s/he starts
>filter-branch will seep through and confuse you.
The empty assignment was implied in my example, but I should have been
more explicit and shown a more complete snippet:
show_eta=
...
{ echo $(date +%s) | grep -q '^[0-9][0-9]*$'; } 2>/dev/null &&
show_eta=t
The suggested 'if' form has the attribute of being clearer.
[1]: http://thread.gmane.org/gmane.comp.version-control.git/276531/focus=276837
[2]: https://www.gnu.org/software/autoconf/manual/autoconf.html#grep
--
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/5] test-lib-functions: detect test_when_finished in subshell
On Sat, Sep 5, 2015 at 9:12 AM, John Keeping wrote:
> test_when_finished does nothing in a subshell because the change to
> test_cleanup does not affect the parent.
>
> There is no POSIX way to detect that we are in a subshell ($$ and $PPID
> are specified to remain unchanged), but we can detect it on Bash and
> fall back to ignoring the bug on other shells.
I'm not necessarily advocating this, but think it's worth mentioning
that an alternate solution would be to fix test_when_finished() to work
correctly in subshells rather than disallowing its use. This can be done
by having test_when_finished() collect the cleanup actions in a file
rather than in a shell variable.
Pros:
* works in subshells
* portable across all shells (no Bash special-case)
* one less rule (restriction) for test writers to remember
Cons:
* slower
* could interfere with tests expecting very specific 'trash' directory
contents (but locating this file under .git might make it safe)
> Signed-off-by: John Keeping
> ---
> t/test-lib-functions.sh | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 0e80f37..6dffb8b 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -736,6 +736,11 @@ test_seq () {
> # what went wrong.
>
> test_when_finished () {
> + # We cannot detect when we are in a subshell in general, but by
> + # doing so on Bash is better than nothing (the test will
> + # silently pass on other shells).
> + test "${BASH_SUBSHELL-0}" = 0 ||
> + error "bug in test script: test_when_finished does nothing in a
> subshell"
> test_cleanup="{ $*
> } && (exit \"\$eval_ret\"); eval_ret=\$?; $test_cleanup"
> }
> --
> 2.5.0.466.g9af26fa
--
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] filter-branch: add passed/remaining seconds on progress
On Sun, Sep 6, 2015 at 5:49 AM, Gabor Bernat wrote:
> On Fri, Sep 4, 2015 at 10:15 PM, Eric Sunshine
> wrote:
>> On Fri, Sep 4, 2015 at 2:34 PM, Junio C Hamano wrote:
>>> Gábor Bernát writes:
>>>> +echo $(date +%s) | grep -q '^[0-9]+$'; 2>/dev/null && show_seconds=t
>>>
>>> That is very strange construct. I think you meant to say something
>>> like
>>>
>>> if date '+%s' 2>/dev/null | grep -q '^[0-9][0-9]*$'
>>> then
>>> show_seconds=t
>>> else
>>> show_seconds=
>>> fi
>>
>> This also mutated. The suggested form wanted to suppress errors from
>> 'date' if it complained about "%s", and from 'grep'. In retrospect,
>> applying it to 'grep' is questionable. I was recalling this warning
>> from the Autoconf manual[2]:
>>
>> Some of the options required by Posix are not portable in
>> practice. Don't use ‘grep -q’ to suppress output, because many
>> grep implementations (e.g., Solaris) do not support -q. Don't use
>> ‘grep -s’ to suppress output either, because Posix says -s does
>> not suppress output, only some error messages; also, the -s
>> option of traditional grep behaved like -q does in most modern
>> implementations. Instead, redirect the standard output and
>> standard error (in case the file doesn't exist) of grep to
>> /dev/null. Check the exit status of grep to determine whether it
>> found a match.
>>
>> however, Git tests use 'grep -q' heavily, so perhaps we don't worry about
>> that.
>
> So we should keep it as it is.
Use of 'grep -q' seems to be fine, however, Junio's comment was about
the errant semicolon, which should not be kept.
>>> * I do not think you are clearing show_seconds to empty anywhere,
>>>so an environment variable the user may have when s/he starts
>>>filter-branch will seep through and confuse you.
>>
>> The empty assignment was implied in my example, but I should have been
>> more explicit and shown a more complete snippet:
>>
>> show_eta=
>> ...
>> { echo $(date +%s) | grep -q '^[0-9][0-9]*$'; } 2>/dev/null &&
>> show_eta=t
>>
>> The suggested 'if' form has the attribute of being clearer.
>
> My bad, sorry for that. Will amend.
>
> Any other pain points, or this construction will satisfy everybody?
Junio's proposed if/then/else construct should be satisfactory.
--
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/5] test-lib-functions: detect test_when_finished in subshell
On Sun, Sep 6, 2015 at 7:46 AM, John Keeping wrote: > On Sun, Sep 06, 2015 at 05:51:43AM -0400, Eric Sunshine wrote: >> I'm not necessarily advocating this, but think it's worth mentioning >> that an alternate solution would be to fix test_when_finished() to work >> correctly in subshells rather than disallowing its use. This can be done >> by having test_when_finished() collect the cleanup actions in a file >> rather than in a shell variable. >> >> Pros: >> * works in subshells >> * portable across all shells (no Bash special-case) >> * one less rule (restriction) for test writers to remember >> >> Cons: >> * slower >> * could interfere with tests expecting very specific 'trash' directory >> contents (but locating this file under .git might make it safe) > > Another con is that we have to worry about the working directory, and > since we can't reliably detect if we're in a subshell every cleanup > action probably has to be something like: > > ( cd '$(pwd)' && $* ) That's an argument against allowing test_when_finished() inside subshells, in general, isn't it? Subshell callers of test_when_finished(), if correctly written, would already have had to protect against that anyhow, so it's not a "con" of the idea of collecting cleanup actions in a file. Of course, patches 2/5 and 4/5 demonstrate that a couple of those subshell callers did *not* correctly protect against directory (or other) changes which would invalidate their cleanup actions, and were thus buggy anyhow, even if the cleanup actions had been invoked. That's a good argument in favor of disallowing test_when_finished() in subshells, but not a "con" of the suggestion. I'm probably arguing semantics here, thus being annoying, so I'll stop now... > It's certainly possible but it adds another bit of complexity. > > Since there are only 3 out of over 13,000 tests that use this > functionality (and it's quite easy to change them not to) I'm not sure > it's worth supporting it. No argument there. -- 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 v16 05/14] ref-filter: introduce match_atom_name()
On Sat, Sep 5, 2015 at 2:52 PM, Karthik Nayak wrote:
> Introduce match_atom_name() which helps in checking if a particular
> atom is the atom we're looking for and if it has a value attached to
> it or not.
>
> Use it instead of starts_with() for checking the value of %(color:...)
> atom. Write a test for the same.
>
> Signed-off-by: Karthik Nayak
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index a993216..e99c342 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> +static int match_atom_name(const char *name, const char *atom_name, const
> char **val)
> +{
> + const char *body;
> +
> + if (!skip_prefix(name, atom_name, &body))
> + return 0; /* doesn't even begin with "atom_name" */
> + if (!body[0] || !body[1]) {
> + *val = NULL; /* %(atom_name) and no customization */
> + return 1;
If this is invoked as match_atom_name("colors", "color", ...), then it
will return true (matched, but no value), which is not correct at all;
"colors" is not a match for atom %(color). Maybe you meant?
if (!body[0] || (body[0] == ':' && !body[1])) {
But, that's getting ugly and complicated, and would be bettered
handled by reordering the logic of this function for dealing with the
various valid and invalid cases. However...
> + }
> + if (body[0] != ':')
> + return 0; /* "atom_namefoo" is not "atom_name" or
> "atom_name:..." */
> + *val = body + 1; /* "atomname:val" */
> + return 1;
> +}
It's not clear why this function exists in the first place. It's only
purpose seems to be to specially recognize instances of atoms which
should have a ":" but lack it, so that the caller can then report an
error.
But why is this better than the more generic solution of merely
reporting an error for *any* unrecognized atom? How does it help to
single out these special cases?
There is a further inconsistency in that you are only specially
recognizing %(color) and %(align) lacking ":", but not
%(content:lines) lacking "=" in patch 8/14. Why do %(color:...) and
%(align:...) get special treatment but %(content:lines=...) does not?
Such inconsistency again argues in favor of a generic "unrecognized
atom" detection and reporting over special case handling.
> /*
> * In a format string, find the next occurrence of %(atom).
> */
> @@ -721,10 +738,12 @@ static void populate_value(struct ref_array_item *ref)
> refname = branch_get_push(branch, NULL);
> if (!refname)
> continue;
> - } else if (starts_with(name, "color:")) {
> + } else if (match_atom_name(name, "color", &valp)) {
> char color[COLOR_MAXLEN] = "";
>
> - if (color_parse(name + 6, color) < 0)
> + if (!valp)
> + die(_("expected format: %%(color:)"));
> + if (color_parse(valp, color) < 0)
> die(_("unable to parse format"));
> v->s = xstrdup(color);
> continue;
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index 505a360..c4f0378 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -81,4 +81,8 @@ test_expect_success 'filtering with --contains' '
> +test_expect_success '%(color) must fail' '
> + test_must_fail git for-each-ref --format="%(color)%(refname)"
> +'
> +
> test_done
> --
> 2.5.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
Re: [PATCH v16 06/14] ref-filter: implement an `align` atom
On Sat, Sep 5, 2015 at 2:52 PM, Karthik Nayak wrote:
> Implement an `align` atom which left-, middle-, or right-aligns the
> content between %(align:...) and %(end).
>
> It is followed by `:,`, where the `` is
> either left, right or middle and `` is the size of the area
> into which the content will be placed. If the content between
> %(align:...) and %(end) is more than the width then no alignment is
> performed. e.g. to align a refname atom to the middle with a total
> width of 40 we can do: --format="%(align:middle,40)%(refname)%(end)".
>
> We introduce an `at_end` function for each element of the stack which
> is to be called when the `end` atom is encountered. Using this we
> implement end_align_handler() for the `align` atom, this aligns the
> final strbuf by calling `strbuf_utf8_align()` from utf8.c.
>
> Ensure that quote formatting is performed on the whole of
> %(align:...)...%(end) rather than individual atoms inside. We skip
> quote formatting for individual atoms when the current stack element
> is handling an %(align:...) atom and perform quote formatting at the
> end when we encounter the %(end) atom of the second element of then
> stack.
>
> Add documentation and tests for the same.
>
> Signed-off-by: Karthik Nayak
> ---
> diff --git a/Documentation/git-for-each-ref.txt
> b/Documentation/git-for-each-ref.txt
> index e49d578..b23412d 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -127,6 +127,16 @@ color::
> +align::
> + Left-, middle-, or right-align the content between %(align:...)
> + and %(end). Followed by `:,`, where the
> + `` is either left, right or middle and `` is
> + the total length of the content with alignment. If the
This should mention that is optional and default to "left"
if omitted.
> + 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.
> diff --git a/ref-filter.c b/ref-filter.c
> index e99c342..6c9ef08 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -773,6 +837,50 @@ static void populate_value(struct ref_array_item *ref)
> else
> v->s = " ";
> continue;
> + } else if (match_atom_name(name, "align", &valp)) {
> + struct align *align = &v->u.align;
> + struct strbuf **s;
> +
> + if (!valp)
> + die(_("expected format: %%(align:,
> )"));
I'm pretty sure this parsing code won't deal well with a space after
the comma, so the space should be dropped from the diagnostic message
advising the user of the correct format: s/, /,/
> + /*
> +* TODO: Implement a function similar to
> strbuf_split_str()
> +* which would strip the terminator at the end.
"...which would omit the separator from the end of each value."
> +*/
> + s = strbuf_split_str(valp, ',', 0);
> +
> + /* If the position is given trim the ',' from the
> first strbuf */
> + if (s[1])
> + strbuf_setlen(s[0], s[0]->len - 1);
> + if (s[2])
> + die(_("align:, followed by
> garbage: %s"), s[2]->buf);
If is omitted, strbuf_split_str("42", ...) will return the
array ["42", NULL] which won't have an element [2], which means this
code will access beyond end-of-array. (This is another good argument
for looping over s[] as Junio suggested rather than assuming these
fixed yet optional positions. It can be hard to get it right.)
> + if (strtoul_ui(s[0]->buf, 10, &align->width))
> + die(_("positive width expected align:%s"),
> s[0]->buf);
> +
> + /*
> +* TODO: Implement a more general check, so that the
> values
> +* do not always have to be in a specific order.
> +*/
> + if (!s[1])
> + align->position = ALIGN_LEFT;
> + else if (!strcmp(s[1]->buf, "left"))
> + align->position = ALIGN_LEFT;
> + else if (!strcmp(s[1]->buf, "right"))
> + align->position = ALIGN_RIGHT;
> + else if (!strcmp(s[1]->buf, "middle"))
> + align->position = ALIGN_MIDDLE;
> + else
> + die(_("improper format entered align:%s"),
> s[1]->buf);
> +
> + strbuf_list_free(s);
> +
> + v->handler = align_atom_handler;
Re: [PATCH/RFC] Pinning of submodules
On Sun, Sep 6, 2015 at 6:08 PM, Anders Ro wrote:
> On 04/09/15 07:02, Eric Sunshine wrote:
>> On Wed, Sep 2, 2015 at 7:34 PM, Anders Ro wrote:
>>> git-submodule.sh: pin submodule when branch name is '@'
>>>
>>> Setting branch name to '@' for a submodule will disable 'git submodule
>>> update --remote' calls for that specific submodule. I.e. instead of
>>> follow the unspecified default choice of master, nothing is being
>>> updated. This is useful when multiple submodules exist but not all
>>> should follow the remote branch head.
>>
>> With the disclaimer that I'm not a submodule user (to which the
>> answer might be obvious): What benefit is there in using a magic
>> value like this ("@") over, say, an explicit configuration setting?
>
> From what I have understood (not a submodule expert yet) the '@' is an
> invalid branch name and should therefore not collide with any current
> branches. My idea was to disable the '--remote' option when the user
> have explicitly set an invalid branch name to not modify any current
> behaviour. Though having an explicit option is of course more
> clarifying. The current behaviour though is that empty branch name means
> "follow master" which is somewhat unintuitive.
My concern in asking was that some future person might come up with
another scenario which also wants to use a "magic value" and would
have to invent / arrive at another "illegal" representation. Hence, an
explicit setting might be more appropriate. However, as stated, I
don't even use submodules, so I may be far off the mark. I've cc'd a
few of the submodule maintainers with the hope that they will chime
in.
>>> +test_expect_success 'set branch name to "@" for submodule pinned' '
>>> +(cd super && git config --replace-all submodule.pinned.branch "@") &&
>>> +test "$(cd super && git config --get submodule.pinned.branch)" = "@"
>>
>> What is the last line testing? It appears to be testing the behavior
>> of git-config, which is outside the scope of this test script.
>>
>> Once combined, you can use test_config rather than git-config, since
>> test_config will ensure that the config setting is undone when the
>> test exits.
In light of this recent thread[1] which shows that
test_when_finished() and, consequently, test_config() are
non-functional in subshells, I have to retract the advice to use
test_config() in this situation. Instead, at this time, it probably
would be best to continue using "git-config" as you do here...
>>> +test_expect_success 'remove branch name "@" for submodule pinned (unpin)' '
>>> +(cd super && git config --unset-all submodule.pinned.branch) &&
>>
>> If you take the above advice and use test_config in the previous
>> test, then this won't be needed.
and "git-config --unset-all" here.
Later, once [1] has landed in "master" (or perhaps "next"), this can
be revisited and updated to use "test_config -C"[2].
[1]: http://thread.gmane.org/gmane.comp.version-control.git/277323/focus=277370
[2]: http://thread.gmane.org/gmane.comp.version-control.git/277323/focus=277372
--
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] filter-branch: add passed/remaining seconds on progress
On Mon, Sep 7, 2015 at 9:52 AM, Gábor Bernát wrote:
> From: Gabor Bernat
>
> adds seconds progress and estimated seconds time if getting the current
> timestamp is supported by the date +%s command
>
> Signed-off-by: Gabor Bernat
> ---
> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index 5b3f63d..565144a 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -275,11 +275,41 @@ commits=$(wc -l <../revs | tr -d " ")
> test $commits -eq 0 && die "Found nothing to rewrite"
>
> # Rewrite the commits
> +report_progress ()
> +{
> +if test -n "$progress"
> +then
Indent code within the function...
> + if test $git_filter_branch__commit_count -gt $next_sample_at
> + then
> + now_timestamp=$(date +%s)
> + elapsed_seconds=$(($now_timestamp - $start_timestamp))
> + remaining_second=$(( ($commits -
> $git_filter_branch__commit_count) * $elapsed_seconds /
> $git_filter_branch__commit_count ))
> + if test $elapsed_seconds -gt 0
> + then
> + next_sample_at=$(( ($elapsed_seconds + 1) *
> $git_filter_branch__commit_count / $elapsed_seconds ))
> + else
> + next_sample_at=$(($next_sample_at + 1))
> + fi
> + progress=" ($elapsed_seconds seconds passed, remaining
> $remaining_second predicted)"
> + fi
> +fi
> +printf "\rRewrite $commit
> ($git_filter_branch__commit_count/$commits)$progress"
The "\r" causes this status line to be overwritten each time through,
and since the processed commit count always increases, we know that
the original (without ETA) will never leave junk at the end of the
line. However, with estimated seconds also being displayed, does this
still hold? While it's true that elapsed seconds will increase,
estimated seconds may jump around, requiring different numbers of
digits to display. This may leave "garbage" digits at the end of line
from previous iterations, can't it?
> +}
>
> git_filter_branch__commit_count=0
> +
> +progress= start_timestamp=
> +if date '+%s' 2>/dev/null | grep -q '^[0-9][0-9]*$'
> +then
> + next_sample_at=0
> + progress="dummy to ensure this is not empty"
> + start_timestamp=$(date '+%s')
> +fi
> +
> while read commit parents; do
>
> git_filter_branch__commit_count=$(($git_filter_branch__commit_count+1))
> - printf "\rRewrite $commit ($git_filter_branch__commit_count/$commits)"
> +
> + report_progress
>
> case "$filter_subdir" in
> "")
> --
> 2.6.0.rc0.3.gb3280a4
--
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] filter-branch: add passed/remaining seconds on progress
On Tue, Sep 8, 2015 at 1:32 PM, Junio C Hamano wrote:
> Eric Sunshine writes:
>> On Mon, Sep 7, 2015 at 9:52 AM, Gábor Bernát wrote:
>>...
>>> # Rewrite the commits
>>> +report_progress ()
>>> +{
>>> +if test -n "$progress"
>>> +then
>>
>> Indent code within the function...
>
> Also git_filter_branch__commit_count is now used only inside this
> function, so it is easier to follow to increment it here.
Make sense.
>>> +printf "\rRewrite $commit
>>> ($git_filter_branch__commit_count/$commits)$progress"
>>
>> The "\r" causes this status line to be overwritten each time through,
>> and since the processed commit count always increases, we know that
>> the original (without ETA) will never leave junk at the end of the
>> line. However, with estimated seconds also being displayed, does this
>> still hold?
>
> Good point.
> Perhaps like this squashed in?
>
> -printf "\rRewrite $commit
> ($git_filter_branch__commit_count/$commits)$progress"
> + printf "\rRewrite $commit
> ($git_filter_branch__commit_count/$commits)$progress"
Yes, for an expedient "fix", this is what I had in mind, although I
would also have added an equal number of backspaces (\b) following the
spaces, as a minor aesthetic improvement.
--
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] Makefile: use SHELL_PATH when running generate-cmdlist.sh
On Thu, Sep 10, 2015 at 7:37 PM, Alejandro R. Sedeño wrote: > Some /bin/sh implementations can't deal with $() arithmetic and command > substitution. If we already have a better shell on hand, we should use it. > > Fixes the build on SunOS, probably others. Makes sense. 527ec39^:generate-cmdlist.sh didn't use either of these features, whereas 82aec45:generate-cmdlist.sh does, and older (pre-POSIX) shells lacked these features. Thanks. I'd probably re-word the commit message slightly to mention $(()) arithmetic expansion, not $(), and to state specifically $(...) command substitution since saying only "command substitution" is ambiguous considering that backtick `...` command substitution long predates POSIX. Perhaps like this: Non-POSIX shells, such as /bin/sh on SunOS, do not support $((...)) arithmetic expansion or $(...) command substitution needed by generate-cmdlist.sh. Therefore, use the POSIX shell $(SHELL_PATH) when running generate-cmdlist.sh. Other than that: Acked-by: Eric Sunshine > Signed-off-by: Alejandro R. Sedeño > --- > diff --git a/Makefile b/Makefile > index ce0cfe2..6301cc8 100644 > --- a/Makefile > +++ b/Makefile > @@ -1699,7 +1699,7 @@ $(BUILT_INS): git$X > common-cmds.h: generate-cmdlist.sh command-list.txt > > common-cmds.h: $(wildcard Documentation/git-*.txt) > - $(QUIET_GEN)./generate-cmdlist.sh command-list.txt >$@+ && mv $@+ $@ > + $(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh command-list.txt >$@+ > && mv $@+ $@ > > SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\ > > $(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\ > -- > 2.5.2 -- 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 v17 00/14] port tag.c to use ref-filter APIs
On Fri, Sep 11, 2015 at 11:08 AM, Karthik Nayak wrote: > On Thu, Sep 10, 2015 at 10:27 PM, Matthieu Moy > wrote: >> Karthik Nayak writes: >>> Changes in this version: >>> * The arguments of the %(align) atom are interchangeable. >>> * Small grammatical changes. >>> * Small changes in the tests to reflect changes in the align >>> atom code. >> >> Clearly, we're almost there. I did a few minor remarks. I suggest >> (admitedly, Eric suggested of-list to suggest ;-) ) that you reply to >> them by re-sending only individual patches that changed (replying to the >> original patch) so that we can check the new patches individually. I >> think we can do the finishing touches for each patch in a subthread of >> this patch. > > I replied with suggested changes by you and Junio. > Let me know if any other changes to be made :) Hmm, but what actually changed in the re-sent patches? Without a link to the discussion leading up to the re-send of changed-only patches, and without an interdiff, the re-send is opaque and less accessible to the reviewer; which is at odds with Matthieu's suggestion which was intended to make review easier and more streamlined. In addition to a link to the previous round and an interdiff, it would be helpful to reviewers for you to annotate each patch (in the commentary are below the "---" line after your sign-off) with a description of the changes in that patch since the previous round in order to focus the reviewer's attention (where it needs to be) on the latest changes. -- 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] Add tests for "Pass graph width to pretty formatting, so '%>|' can work properly"
On Fri, Sep 11, 2015 at 1:50 PM, Josef Kufner wrote: > --- Missing sign-off. Or is this intended to be concatenated with the patch you sent earlier? > diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh > index 7398605..3358837 100755 > --- a/t/t4205-log-pretty-formats.sh > +++ b/t/t4205-log-pretty-formats.sh > @@ -319,6 +319,18 @@ EOF > test_cmp expected actual > ' > > +# Note: Space between 'message' and 'two' should be in the same column as in > previous test. > +test_expect_success 'right alignment formatting at the nth column with > --graph. i18n.logOutputEncoding' ' > + git -c i18n.logOutputEncoding=$test_encoding log --graph > --pretty="tformat:%h %>|(40)%s" >actual && > + qz_to_tab_space+* $head1message two > +* $head2message one > +* $head3add bar > +* $head4$(commit_msg) > +EOF > + test_cmp expected actual > +' > + > test_expect_success 'right alignment formatting with no padding' ' > git log --pretty="tformat:%>(1)%s" >actual && > cat @@ -330,6 +342,17 @@ EOF > test_cmp expected actual > ' > > +test_expect_success 'right alignment formatting with no padding and with > --graph' ' > + git log --graph --pretty="tformat:%>(1)%s" >actual && > + cat +* message two > +* message one > +* add bar > +* $(commit_msg) > +EOF > + test_cmp expected actual > +' -- 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 v7 2/3] worktree: move/refactor find_shared_symref from branch.c
On Fri, Sep 11, 2015 at 5:52 PM, Junio C Hamano wrote: > Mike Rappazzo writes: >> On Fri, Sep 11, 2015 at 12:16 PM, Junio C Hamano wrote: >>> Michael Rappazzo writes: The code formerly in branch.c was largely the basis of the get_worktree_list implementation is now moved to worktree.c, and the find_shared_symref implementation has been refactored to use get_worktree_list >>> >>> Copying the bulk of the function in 1/3 and then removing the >>> original here made it somewhat hard to compare what got changed in >>> the implementation. >>> >>> I _think_ the code structure in the end result is more or less >>> right, though. >> >> Should I squash these first two commits together in the next series? > > Mashing the two into one patch would not help anybody, I would > suspect. > > I didn't try this myself, but if I were doing this and if I were > aiming for perfection, then I would probably try to split it even > finer. Refactor the original while both the callers and the helpers > are still inside branch.c (hence the early steps in the series will > not benefit worktree.c at all---worktree.c may not even exist in > them yet), move refactored helpers to worktree.[ch] (and at this > step you may not even have get_worktree() etc. yet), and then use > that refactored helper while writing new functions in worktree.c. Thanks for bringing this up. I haven't found a sufficient block of time yet to review these patches, however, I had the same thought upon a quick initial read, and is how I was hoping to see the patch series constructed based upon my earlier two reviews suggesting refactoring the existing branch.c functions into a new get_worktrees(). There are at least a couple important reasons for taking this approach. First, it keeps the "blame" trail intact, the full context of which can be helpful to someone trying to understand why the code is the way it is. The current approach of having get_worktree_list() materialize out of thin air (even though it may have been patterned after existing code) doesn't give the same benefit. Second, it's easier for reviewers to understand and check the code for correctness if it mutates to the final form in small increments than it is to have get_worktrees() come into existence fully matured. A final minor comment: Since all three branch.c functions, die_if_checked_out(), find_shared_symref(), and find_linked_symref(), ought to be moved to top-level worktree.c, I'd probably have patch 1 do the relocation (with no functional changes), and subsequent patches refactor the moved code into a general purpose get_worktrees(). The final patch would then implement "git worktrees list". -- 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 v7 1/3] worktree: add top-level worktree.c
On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo wrote:
> Including functions to get the list of all worktrees, and to get
> a specific worktree (primary or linked).
Some comments below in addition to those by Junio...
> Signed-off-by: Michael Rappazzo
> ---
> diff --git a/worktree.c b/worktree.c
> new file mode 100644
> index 000..33d2e57
> --- /dev/null
> +++ b/worktree.c
> @@ -0,0 +1,157 @@
> +#include "worktree.h"
> +#include "cache.h"
> +#include "git-compat-util.h"
> +#include "refs.h"
> +#include "strbuf.h"
> +
> +void worktree_release(struct worktree *worktree)
> +{
> + if (worktree) {
> + free(worktree->path);
> + free(worktree->git_dir);
> + if (!worktree->is_detached) {
> + /* could be headless */
> + free(worktree->head_ref);
It's safe to invoke free() on NULL, so as long as worktree->head_ref
holds NULL in the detached case, then there's no need for the
conditional at all (or the comment).
> + }
> + free(worktree);
> + }
> +}
> +
> +void worktree_list_release(struct worktree_list *worktree_list)
> +{
> + while (worktree_list) {
> + struct worktree_list *next = worktree_list->next;
> + worktree_release(worktree_list->worktree);
> + free (worktree_list);
> + worktree_list = next;
> + }
> +}
> +
> +/*
> + * read 'path_to_ref' into 'ref'. Also set is_detached to 1 if the ref is
> detatched
> + *
> + * return 1 if the ref is not a proper ref, 0 otherwise (success)
> + */
> +int _parse_ref(char *path_to_ref, struct strbuf *ref, int *is_detached)
Mentioned already by Junio in his review of patch 2: Make this
function static and drop the leading underscore from the name.
> +{
> + if (!strbuf_readlink(ref, path_to_ref, 0)) {
> + if (!starts_with(ref->buf, "refs/") ||
> check_refname_format(ref->buf, 0)) {
> + /* invalid ref - something is awry with this repo */
> + return 1;
In this codebase, it's common to return -1 on error. You could also
probably drop the unnecessary braces (here and a few other places).
> + }
> + } else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {
> + if (starts_with(ref->buf, "ref:")) {
> + strbuf_remove(ref, 0, strlen("ref:"));
> + strbuf_trim(ref);
> + } else if (is_detached) {
> + *is_detached = 1;
The code allows the caller to pass in NULL for is_detached if not
interested in this information, however, the comment block documenting
the function doesn't mention this.
Also, don't you need to clear *is_detached when not detached since you
don't know what garbage value *is_detached holds upon entry?
I find the placement of the detached detection logic here a bit
strange. The only case for which it might trigger is the so-called
"main worktree", yet having it in this general purpose parse function
seems to imply misleadingly that any worktree could be detached. Also,
given the current world order[1], wouldn't missing "ref:" indicate an
error for any worktree other than the main one? Consequently, this
detection probably ought to be done only for the main worktree
(outside of this function, probably).
[1]: Though, this might not hold true in a future world order
involving merging of notes, if I correctly understood that discussion,
since notes merging wouldn't require a "worktree", per se.
> + }
> + }
> + return 0;
> +}
> +
> +struct worktree *get_worktree(const char *id)
> +{
> + struct worktree *worktree = NULL;
> + struct strbuf path = STRBUF_INIT;
> + struct strbuf worktree_path = STRBUF_INIT;
> + struct strbuf git_dir = STRBUF_INIT;
> + struct strbuf head_ref = STRBUF_INIT;
> + int is_bare = 0;
> + int is_detached = 0;
> +
> + if (id) {
> + strbuf_addf(&git_dir, "%s/worktrees/%s",
> absolute_path(get_git_common_dir()), id);
> + strbuf_addf(&path, "%s/gitdir", git_dir.buf);
> + if (strbuf_read_file(&worktree_path, path.buf, 0) <= 0) {
> + /* invalid git_dir file */
> + goto done;
> + }
> + strbuf_rtrim(&worktree_path);
> + if (!strbuf_strip_suffix(&worktree_path, "/.git")) {
> + strbuf_reset(&worktree_path);
> + strbuf_addstr(&worktree_path, absolute_path("."));
> + strbuf_strip_suffix(&worktree_path, "/.");
> + }
> +
> + strbuf_reset(&path);
> + strbuf_addf(&path, "%s/worktrees/%s/HEAD",
> get_git_common_dir(), id);
> + } else {
> + strbuf_addf(&git_dir, "%s",
> absolute_path(get_git_common_dir()));
> + strbuf_addf(&worktree_path, "%s",
> absol
Re: [PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c
On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo wrote:
> The code formerly in branch.c was largely the basis of the
> get_worktree_list implementation is now moved to worktree.c,
> and the find_shared_symref implementation has been refactored
> to use get_worktree_list
Some comments below in addition to those by Junio...
> Signed-off-by: Michael Rappazzo
> ---
> diff --git a/branch.h b/branch.h
> index d3446ed..58aa45f 100644
> --- a/branch.h
> +++ b/branch.h
> @@ -59,12 +59,4 @@ extern int read_branch_desc(struct strbuf *, const char
> *branch_name);
> */
> extern void die_if_checked_out(const char *branch);
>
> -/*
> - * Check if a per-worktree symref points to a ref in the main worktree
> - * or any linked worktree, and return the path to the exising worktree
> - * if it is. Returns NULL if there is no existing ref. The caller is
> - * responsible for freeing the returned path.
> - */
> -extern char *find_shared_symref(const char *symref, const char *target);
> -
> #endif
> diff --git a/worktree.c b/worktree.c
> index 33d2e57..e45b651 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -155,3 +155,43 @@ done:
> return list;
> }
>
> +char *find_shared_symref(const char *symref, const char *target)
> +{
> + char *existing = NULL;
> + struct strbuf path = STRBUF_INIT;
> + struct strbuf sb = STRBUF_INIT;
> + struct worktree_list *worktree_list = get_worktree_list();
> + struct worktree_list *orig_list = worktree_list;
> + struct worktree *matched = NULL;
> +
> + while (!matched && worktree_list) {
> + if (strcmp("HEAD", symref)) {
The result of strcmp() never changes, so this (expensive) check could
be lifted out of the 'while' loop, however...
> + strbuf_reset(&path);
> + strbuf_reset(&sb);
> + strbuf_addf(&path, "%s/%s",
> worktree_list->worktree->git_dir, symref);
> +
> + if (_parse_ref(path.buf, &sb, NULL)) {
> + continue;
> + }
> +
> + if (!strcmp(sb.buf, target))
> + matched = worktree_list->worktree;
The original code in branch.c, which this patch removes, did not need
to make a special case of HEAD, so it's not immediately clear why this
replacement code does so. This is the sort of issue which argues in
favor of mutating the existing code (slowly) over the course of
several patches into the final form, rather than having the final form
come into existence out of thin air. When the changes are made
incrementally, it is easier for reviewers to understand why such
modifications are needed, which (hopefully) should lead to fewer
questions such as this one.
> + } else {
> + if (worktree_list->worktree->head_ref &&
> !strcmp(worktree_list->worktree->head_ref, target))
> + matched = worktree_list->worktree;
> + }
> + worktree_list = worktree_list->next ? worktree_list->next :
> NULL;
> + }
> +
> + if (matched) {
> + existing = malloc(strlen(matched->path) + 1);
> + strcpy(existing, matched->path);
A couple issues:
This can be done more concisely and safely with xstrdup().
In this codebase, it probably would be more idiomatic to use goto to
drop out of the loop rather than setting 'matched' and then having to
check 'matched' in the loop condition. So, for instance, each place
which sets 'matched' could instead say:
existing = xstrdup(...);
goto done;
> + }
> +
> +done:
This label doesn't have any matching goto's.
> + strbuf_release(&path);
> + strbuf_release(&sb);
> + worktree_list_release(orig_list);
> +
> + return existing;
> +}
> diff --git a/worktree.h b/worktree.h
> index 2bc0ab8..320f17e 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -45,4 +45,11 @@ struct worktree *get_worktree(const char *id);
> extern void worktree_list_release(struct worktree_list *);
> extern void worktree_release(struct worktree *);
>
> +/*
> + * Look for a symref in any worktree, and return the path to the first
> + * worktree found or NULL if not found. The caller is responsible for
> + * freeing the returned path.
> + */
For some reason, this comment differs a fair bit from the original in
branch.h which was removed by this patch, however, the original
comment was a bit more explanatory (I think).
As a general rule, it's better to do code movement and code changes in
separate patches since it's hard for reviewers to detect and
comprehend differences in code which has been both moved and changed
at the same time.
> +extern char *find_shared_symref(const char *symref, const char *target);
> +
> #endif
> --
> 2.5.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.kerne
Re: [PATCH v7 3/3] worktree: add 'list' command
On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo wrote:
> 'git worktree list' iterates through the worktree list, and outputs
> the worktree dir. By default, only the worktree path is output.
Comments below in addition to Junio's...
> Supported options include:
> --skip-bare: do not output bare worktrees
> --verbose: include the current head and ref (if applicable), also
> decorate bare worktrees
I don't care strongly at this point, but an alternate, (possibly) more
reviewer-friendly, approach would be to split this into several
patches: the first would add a bare-bones "list" command, and each
subsequent patch would flesh it out by introducing one option and/or
enhancing the output in some way.
> Signed-off-by: Michael Rappazzo
> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index fb68156..b9339ed 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -11,6 +11,7 @@ SYNOPSIS
> [verse]
> 'git worktree add' [-f] [--detach] [-b ] []
> 'git worktree prune' [-n] [-v] [--expire ]
> +'git worktree list' [-v] [--skip-bare]
I'm somewhat skeptical of the --skip-bare option. Recalling my v2
review[1] skepticism of the --main-only option:
The more options we have, the more we have to document, test, and
support...
Thus, I wonder how much value this option has. Presumably, for
scripters, we will want to have a --porcelain option, the output of
which will contain useful information about a worktree, including
whether it's bare, and a script can, on its own, easily filter out a
bare worktree if desired.
[1]: http://article.gmane.org/gmane.comp.version-control.git/275528
> DESCRIPTION
> ---
> @@ -59,6 +60,10 @@ prune::
>
> Prune working tree information in $GIT_DIR/worktrees.
>
> +list::
> +
> +List the main worktree followed by each of the linked worktrees.
> +
> OPTIONS
> ---
>
> @@ -89,10 +94,14 @@ OPTIONS
> -v::
> --verbose::
> With `prune`, report all removals.
> + With `list`, show more information about each worktree. This includes
> + if the worktree is bare, the revision currently checked out, and the
> + branch currently checked out (or 'detached HEAD' if none).
Hmm, this prints both the SHA1 and the branch name (or literal string
"detached HEAD")? Is that a good use of screen real-estate? In
particular, I'm wondering how useful the SHA1 is to the user when not
detached. I would expect (perhaps wrongly) that it would be sufficient
to output either the branch name *or* the SHA1 (which implies
"detached" without having to say so literally).
> --expire ::
> With `prune`, only expire unused working trees older than .
Documentation for --skip-bare (and --no-skip-bare) seems to be missing.
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 71bb770..a0c0fe8 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -359,6 +362,64 @@ static int add(int ac, const char **av, const char
> *prefix)
> +static int list(int ac, const char **av, const char *prefix)
> +{
> + int no_bare = 0;
> +
> + struct option options[] = {
> + OPT_BOOL(0, "skip-bare", &no_bare, N_("exclude bare
> worktrees from the list")),
> + OPT__VERBOSE(&verbose, N_("include more worktree details")),
> + OPT_END()
> + };
> +
> + ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
> + if (ac)
> + usage_with_options(worktree_usage, options);
> + else {
> + struct worktree_list *worktree_list = get_worktree_list();
> + struct worktree_list *orig = worktree_list;
> + struct strbuf sb = STRBUF_INIT;
> + int path_maxlen = 0;
> +
> + if (verbose) {
> + while (worktree_list) {
> + int cur_len =
> strlen(worktree_list->worktree->path);
> + if (cur_len > path_maxlen)
> + path_maxlen = cur_len;
> + worktree_list = worktree_list->next ?
> worktree_list->next : NULL;
> + }
> + worktree_list = orig;
> + }
> + while (worktree_list) {
> + /* if this work tree is not bare OR if bare repos are
> to be shown (default) */
> + if (!worktree_list->worktree->is_bare || !no_bare) {
Double negatives (!no_bare) are hard to grok. 'bare_only' or
'skip_bare' might be better, and then the comment would likely be
superfluous. Also, having the "default" behavior mentioned in the
comment is an invitation for it to become outdated.
> + strbuf_reset(&sb);
> + if (!verbose)
> + strbuf_addstr(&sb,
> worktree_list->worktree->path);
> +
Re: [PATCH v7 1/3] worktree: add top-level worktree.c
On Sat, Sep 12, 2015 at 10:39 PM, Eric Sunshine wrote:
> On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo wrote:
>> + }
>> + } else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {
>> + if (starts_with(ref->buf, "ref:")) {
>> + strbuf_remove(ref, 0, strlen("ref:"));
>> + strbuf_trim(ref);
>> + } else if (is_detached) {
>> + *is_detached = 1;
>
> I find the placement of the detached detection logic here a bit
> strange. The only case for which it might trigger is the so-called
> "main worktree", yet having it in this general purpose parse function
> seems to imply misleadingly that any worktree could be detached. Also,
> given the current world order[1], wouldn't missing "ref:" indicate an
> error for any worktree other than the main one? Consequently, this
> detection probably ought to be done only for the main worktree
> (outside of this function, probably).
Eh, ignore this bit. My brain was conflating 'bare' and 'detached'.
--
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 v4 3/8] branch: roll show_detached HEAD into regular ref_list
On Sun, Sep 13, 2015 at 8:12 AM, Matthieu Moy
wrote:
> Karthik Nayak writes:
>
>> @@ -679,15 +682,20 @@ static int print_ref_list(int kinds, int detached, int
>> verbose, int abbrev, stru
>> if (verbose)
>> maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));
>>
>> - qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
>> + index = ref_list.index;
>> +
>> + /* Print detached HEAD before sorting and printing the rest */
>> + if (detached && (ref_list.list[index - 1].kind == REF_DETACHED_HEAD) &&
>> + !strcmp(ref_list.list[index - 1].name, head)) {
>> + print_ref_item(&ref_list.list[index - 1], maxwidth, verbose,
>> abbrev,
>> +1, remote_prefix);
>> + index -= 1;
>> + }
>
> I think Eric already mentionned it, but I don't remember the conclusion
> and can't find it in the archives. Wouldn't it be cleaner to actually
> remove the detached head from the array (do "ref_list.index -= 1"
> instead of "index -= 1", and possibly free() what needs to be freed?
I think Michael Haggerty mentioned something along those lines...
--
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: Git configure/make does not honor ARFLAGS
On Sun, Sep 13, 2015 at 6:17 AM, Jeff King wrote: > On Sun, Aug 30, 2015 at 05:34:59PM -0400, Jeffrey Walton wrote: >> I'm working on an old OS X machine. I needed to perform: >> >> AR=libtool >> ARFLAGS="-static -o" >> ... >> make configure >> ./configure ... >> make > > Hrm. Your "$(AR)" is not really "ar" then, is it? It has been a long > time since I played with libtool, but what is the reason that you are > calling libtool and not "ar" in the first place. Is it that you do not > have "ar" at all, and libtool performs some other procedure? If so, is > there a more ar-compatible wrapper that can be used? This isn't GNU's libtool. It's Apple's libtool, an entirely different beast, which is an 'ar' replacement and is needed when linking Universal binaries containing code for more than one architecture, such as 'ppc' and 'i386', so the same executable can run on multiple architectures. This tool dates all the way back to at least NextStep 3.1 when NeXT ported NextStep to Intel hardware (i486) from NeXT computers (m68k). The name "Universal" is an Apple invention, but back in the NeXT days, they were called Multi-Architecture Binaries (MAB) or, colloquially, just FAT (for "fat"); there was a corresponding "lipo" command (short for "liposuction") to "thin" out "fat" binaries. NeXT's libtool predates GNU's libtool by a few years: May 1993 vs. July 1997, respectively. When an attempt is made to use 'ar' on Universal object files, it errors out saying that it can't be used with such files and recommends 'libtool' instead. >> The Makefile might benefit from the following for users who need to >> tweak things: >> >> ARFLAGS ?= rcs >> ... >> >> $(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^ >> ... > > Yeah, that does sound reasonable (even if one does not set $(AR) to > something completely different, they might need slightly different > flags). -- 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 v4 3/8] branch: roll show_detached HEAD into regular ref_list
On Sun, Sep 13, 2015 at 12:46 PM, Eric Sunshine wrote:
> On Sun, Sep 13, 2015 at 8:12 AM, Matthieu Moy
> wrote:
>> Karthik Nayak writes:
>>
>>> @@ -679,15 +682,20 @@ static int print_ref_list(int kinds, int detached,
>>> int verbose, int abbrev, stru
>>> if (verbose)
>>> maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));
>>>
>>> - qsort(ref_list.list, ref_list.index, sizeof(struct ref_item),
>>> ref_cmp);
>>> + index = ref_list.index;
>>> +
>>> + /* Print detached HEAD before sorting and printing the rest */
>>> + if (detached && (ref_list.list[index - 1].kind == REF_DETACHED_HEAD)
>>> &&
>>> + !strcmp(ref_list.list[index - 1].name, head)) {
>>> + print_ref_item(&ref_list.list[index - 1], maxwidth, verbose,
>>> abbrev,
>>> +1, remote_prefix);
>>> + index -= 1;
>>> + }
>>
>> I think Eric already mentionned it, but I don't remember the conclusion
>> and can't find it in the archives. Wouldn't it be cleaner to actually
>> remove the detached head from the array (do "ref_list.index -= 1"
>> instead of "index -= 1", and possibly free() what needs to be freed?
>
> I think Michael Haggerty mentioned something along those lines...
Specifically, I think you're referring to [1] (?).
[1]: http://thread.gmane.org/gmane.comp.version-control.git/276363/focus=276676
--
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 07/67] strbuf: make strbuf_complete_line more generic
On Tue, Sep 15, 2015 at 11:25 AM, Jeff King wrote:
> The strbuf_complete_line function make sure that a buffer
s/make/makes/
> ends in a newline. But we may want to do this for any
> character (e.g., "/" on the end of a path). Let's factor out
> a generic version, and keep strbuf_complete_line as a thin
> wrapper.
>
> Signed-off-by: Jeff King
> ---
> +/**
> + * Ensure that `sb` ends with the character `term`, if it does not
> + * already.
> + */
> +static inline void strbuf_complete(struct strbuf *sb, char term)
> +{
> + if (sb->len && sb->buf[sb->len - 1] != term)
> + strbuf_addch(sb, term);
> +}
Hmm, so this only adds 'term' if not already present *and* if 'sb' is
not empty, which doesn't seem to match the documentation which says
that it "ensures" termination.
But, is that reasonable behavior? Intuitively, I'd expect 'term' to be
added when 'sb' is empty:
if (!sb->len || sb->buf[sb->len - 1] != term)
strbuf_addch(sb, term);
strbuf_complete_line()'s existing behavior of not adding '\n' to an
empty string may have been intentional, but actually smells like a
bug.
> +
> +/**
> + * Ensure that `sb` ends with a newline.
> + */
> static inline void strbuf_complete_line(struct strbuf *sb)
> {
> - if (sb->len && sb->buf[sb->len - 1] != '\n')
> - strbuf_addch(sb, '\n');
> + strbuf_complete(sb, '\n');
> }
--
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 10/67] mailsplit: make PATH_MAX buffers dynamic
On Tue, Sep 15, 2015 at 11:28 AM, Jeff King wrote:
> There are several static PATH_MAX-sized buffers in
> mailsplit, along with some questionable uses of sprintf.
> These are not really of security interest, as local
> mailsplit pathnames are not typically under control of an
> attacker. But it does not hurt to be careful, and as a
> bonus we lift some limits for systems with too-small
> PATH_MAX varibles.
>
> Signed-off-by: Jeff King
> ---
> diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
> index 9de06e3..fb0bc08 100644
> --- a/builtin/mailsplit.c
> +++ b/builtin/mailsplit.c
> @@ -148,8 +155,7 @@ static int maildir_filename_cmp(const char *a, const char
> *b)
> static int split_maildir(const char *maildir, const char *dir,
> int nr_prec, int skip)
> {
> - char file[PATH_MAX];
> - char name[PATH_MAX];
> + struct strbuf file = STRBUF_INIT;
> FILE *f = NULL;
> int ret = -1;
> int i;
> @@ -161,20 +167,25 @@ static int split_maildir(const char *maildir, const
> char *dir,
> goto out;
>
> for (i = 0; i < list.nr; i++) {
> - snprintf(file, sizeof(file), "%s/%s", maildir,
> list.items[i].string);
> - f = fopen(file, "r");
> + char *name;
> +
> + strbuf_reset(&file);
> + strbuf_addf(&file, "%s/%s", maildir, list.items[i].string);
> +
> + f = fopen(file.buf, "r");
> if (!f) {
> - error("cannot open mail %s (%s)", file,
> strerror(errno));
> + error("cannot open mail %s (%s)", file.buf,
> strerror(errno));
> goto out;
> }
>
> if (strbuf_getwholeline(&buf, f, '\n')) {
> - error("cannot read mail %s (%s)", file,
> strerror(errno));
> + error("cannot read mail %s (%s)", file.buf,
> strerror(errno));
> goto out;
> }
>
> - sprintf(name, "%s/%0*d", dir, nr_prec, ++skip);
> + name = xstrfmt("%s/%0*d", dir, nr_prec, ++skip);
> split_one(f, name, 1);
> + free(name);
Hmm, why does 'file' become a strbuf which is re-used each time
through the loop, but 'name' is treated differently and gets
re-allocated upon each iteration? Why doesn't 'name' deserve the same
treatment as 'file'?
> fclose(f);
> f = NULL;
> @@ -184,6 +195,7 @@ static int split_maildir(const char *maildir, const char
> *dir,
> out:
> if (f)
> fclose(f);
> + strbuf_release(&file);
> string_list_clear(&list, 1);
> return ret;
> }
> @@ -191,7 +203,6 @@ out:
> static int split_mbox(const char *file, const char *dir, int allow_bare,
> int nr_prec, int skip)
> {
> - char name[PATH_MAX];
> int ret = -1;
> int peek;
>
> @@ -218,8 +229,9 @@ static int split_mbox(const char *file, const char *dir,
> int allow_bare,
> }
>
> while (!file_done) {
> - sprintf(name, "%s/%0*d", dir, nr_prec, ++skip);
> + char *name = xstrfmt("%s/%0*d", dir, nr_prec, ++skip);
> file_done = split_one(f, name, allow_bare);
> + free(name);
Same question, pretty much: Why not make 'name' a strbuf which is
re-used in the loop? (I don't have a strong preference; I'm just
trying to understand the apparent inconsistency of treatment.)
> }
>
> if (f != stdin)
> --
> 2.6.0.rc2.408.ga2926b9
--
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
