Re: [BUG?] git rebase not accepting :/ syntax
On Fri, 07 Sep 2012 15:54:49 +0200
Andreas Schwab wrote:
> Yann Dirson writes:
>
> > In 1.7.10.3, "git rebase -i :/Merge" will complain with:
> >
> > fatal: Needed a single revision
> > invalid upstream :/Merge
> >
> > ... whereas "git rev-parse :/Merge" has no problem resolving
> > to a single revision.
>
> git rebase actually calls "git rev-parse :/Merge^0", which due to the
> unlimited nature of :/ doesn't work.
Hm. But then, git rev-parse $(git rev-parse :/Merge}^0 does work, a trivial
patch would appear to make things better.
BTW, git-rebase.sh seems to be quite inconsistent on the use of $() vs. ``,
not to mention the clear preference stated in CodingGuidelines.
I guess I'll find a moment for a couple of patches...
> > OTOH, "git rebase -i HEAD^{/Merge}" does work, and rev-parse resolves
> > it to the same commit.
>
> OTOH, "git rev-parse HEAD^{/Merge}^0" works as expected.
>
> Andreas.
>
--
Yann Dirson - Bertin Technologies
--
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?] git rebase not accepting :/ syntax
Yann Dirson wrote: On Fri, 07 Sep 2012 15:54:49 +0200 ... BTW, git-rebase.sh seems to be quite inconsistent on the use of $() vs. ``, not to mention the clear preference stated in CodingGuidelines. There are still quite a few more places in *.sh where `cmd`is used instead of $(cmd): check-builtins.sh, git-am.sh, git-merge-one-file.sh, git-pull.sh, git-rebase--merge.sh, git-repack.sh, git-stash.sh, git-web--browse.sh,test-sha1.sh, unimplemented.sh I guess I'll find a moment for a couple of patches... Might wanna fix them all in one go? Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] Add a new function, filter_string_list()
On 09/09/2012 11:40 AM, Junio C Hamano wrote:
> Michael Haggerty writes:
>
>> Signed-off-by: Michael Haggerty
>> ---
>> Documentation/technical/api-string-list.txt | 8
>> string-list.c | 17 +
>> string-list.h | 9 +
>> 3 files changed, 34 insertions(+)
>>
>> diff --git a/Documentation/technical/api-string-list.txt
>> b/Documentation/technical/api-string-list.txt
>> index 3b959a2..15b8072 100644
>> --- a/Documentation/technical/api-string-list.txt
>> +++ b/Documentation/technical/api-string-list.txt
>> @@ -60,6 +60,14 @@ Functions
>>
>> * General ones (works with sorted and unsorted lists as well)
>>
>> +`filter_string_list`::
>> +
>> +Apply a function to each item in a list, retaining only the
>> +items for which the function returns true. If free_util is
>> +true, call free() on the util members of any items that have
>> +to be deleted. Preserve the order of the items that are
>> +retained.
>
> In other words, this can safely be used on both sorted and unsorted
> string list. Good.
Preserving order (while retaining performance) is the main reason for
this function. Otherwise, unsorted_string_list_delete_item() could be
used in a loop.
>> `print_string_list`::
>>
>> Dump a string_list to stdout, useful mainly for debugging purposes. It
>> diff --git a/string-list.c b/string-list.c
>> index 110449c..72610ce 100644
>> --- a/string-list.c
>> +++ b/string-list.c
>> @@ -102,6 +102,23 @@ int for_each_string_list(struct string_list *list,
>> return ret;
>> }
>>
>> +void filter_string_list(struct string_list *list, int free_util,
>> +string_list_each_func_t fn, void *cb_data)
>> +{
>> +int src, dst = 0;
>> +for (src = 0; src < list->nr; src++) {
>> +if (fn(&list->items[src], cb_data)) {
>> +list->items[dst++] = list->items[src];
>> +} else {
>> +if (list->strdup_strings)
>> +free(list->items[src].string);
>> +if (free_util)
>> +free(list->items[src].util);
>> +}
>> +}
>> +list->nr = dst;
>> +}
>> +
>> void string_list_clear(struct string_list *list, int free_util)
>> {
>> if (list->items) {
>> diff --git a/string-list.h b/string-list.h
>> index 7e51d03..84996aa 100644
>> --- a/string-list.h
>> +++ b/string-list.h
>> @@ -29,6 +29,15 @@ int for_each_string_list(struct string_list *list,
>> #define for_each_string_list_item(item,list) \
>> for (item = (list)->items; item < (list)->items + (list)->nr; ++item)
>>
>> +/*
>> + * Apply fn to each item in list, retaining only the ones for which
>> + * the function returns true. If free_util is true, call free() on
>> + * the util members of any items that have to be deleted. Preserve
>> + * the order of the items that are retained.
>> + */
>> +void filter_string_list(struct string_list *list, int free_util,
>> +string_list_each_func_t fn, void *cb_data);
>> +
>> /* Use these functions only on sorted lists: */
>> int string_list_has_string(const struct string_list *list, const char
>> *string);
>> int string_list_find_insert_index(const struct string_list *list, const
>> char *string,
>
> Having seen that the previous patch introduced a new test helper for
> unit testing (which is a very good idea) and dedicated a new test
> number, I would have expected to see a new test for filtering
> here.
I thought that the code was too trivial to warrant a test, especially
considering that the memory handling aspect of the function can't be
tested very well. But you've correctly shamed me into adding tests for
this and also for patch 3/4, string_list_remove_duplicates().
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
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/4] Add a new function, string_list_remove_duplicates()
On 09/09/2012 11:45 AM, Junio C Hamano wrote:
> Michael Haggerty writes:
>
>> Signed-off-by: Michael Haggerty
>> ---
>> Documentation/technical/api-string-list.txt | 4
>> string-list.c | 17 +
>> string-list.h | 5 +
>> 3 files changed, 26 insertions(+)
>>
>> diff --git a/Documentation/technical/api-string-list.txt
>> b/Documentation/technical/api-string-list.txt
>> index 15b8072..9206f8f 100644
>> --- a/Documentation/technical/api-string-list.txt
>> +++ b/Documentation/technical/api-string-list.txt
>> @@ -104,6 +104,10 @@ write `string_list_insert(...)->util = ...;`.
>> Look up a given string in the string_list, returning the containing
>> string_list_item. If the string is not found, NULL is returned.
>>
>> +`string_list_remove_duplicates`::
>> +
>> +Remove all but the first entry that has a given string value.
>
> Unlike the previous patch, free_util is not documented?
Fixed.
> It is kind of shame that the string list must be sorted for this
> function to work, but I guess you do not have a need for a version
> that works on both sorted and unsorted (yet). We can introduce a
> variant with "unsorted_" prefix later when it becomes necessary, so
> OK.
Not only that; for an unsorted list it is not quite as obvious what a
caller would want. Often lists are used as a poor man's set, in which
case the caller would probably not mind sorting the list anyway. There
are two operations that one might conceive of for unsorted lists: (1)
remove duplicates while preserving the order of the remaining entries,
or (2) remove duplicates while not worrying about the order of the
remaining entries. (Admittedly the first is not much more expensive
than the second.) These are more complicated to program, require
temporary space, and are of less obvious utility than removing
duplicates from a sorted list.
>> * Functions for unsorted lists only
>>
>> `string_list_append`::
>> diff --git a/string-list.c b/string-list.c
>> index 72610ce..bfef6cf 100644
>> --- a/string-list.c
>> +++ b/string-list.c
>> @@ -92,6 +92,23 @@ struct string_list_item *string_list_lookup(struct
>> string_list *list, const char
>> return list->items + i;
>> }
>>
>> +void string_list_remove_duplicates(struct string_list *list, int free_util)
>> +{
>> +if (list->nr > 1) {
>> +int src, dst;
>> +for (src = dst = 1; src < list->nr; src++) {
>> +if (!strcmp(list->items[dst - 1].string,
>> list->items[src].string)) {
>> +if (list->strdup_strings)
>> +free(list->items[src].string);
>> +if (free_util)
>> +free(list->items[src].util);
>> +} else
>> +list->items[dst++] = list->items[src];
>> +}
>> +list->nr = dst;
>> +}
>> +}
>> +
>> int for_each_string_list(struct string_list *list,
>> string_list_each_func_t fn, void *cb_data)
>> {
>> diff --git a/string-list.h b/string-list.h
>> index 84996aa..c4dc659 100644
>> --- a/string-list.h
>> +++ b/string-list.h
>> @@ -38,6 +38,7 @@ int for_each_string_list(struct string_list *list,
>> void filter_string_list(struct string_list *list, int free_util,
>> string_list_each_func_t fn, void *cb_data);
>>
>> +
>> /* Use these functions only on sorted lists: */
>> int string_list_has_string(const struct string_list *list, const char
>> *string);
>> int string_list_find_insert_index(const struct string_list *list, const
>> char *string,
>> @@ -47,6 +48,10 @@ struct string_list_item
>> *string_list_insert_at_index(struct string_list *list,
>> int insert_at, const char
>> *string);
>> struct string_list_item *string_list_lookup(struct string_list *list, const
>> char *string);
>>
>> +/* Remove all but the first entry that has a given string value. */
>> +void string_list_remove_duplicates(struct string_list *list, int free_util);
>> +
>> +
>> /* Use these functions only on unsorted lists: */
>> struct string_list_item *string_list_append(struct string_list *list, const
>> char *string);
>> void sort_string_list(struct string_list *list);
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] Add a function string_list_longest_prefix()
On 09/09/2012 11:54 AM, Junio C Hamano wrote: > Michael Haggerty writes: > >> [...] >> diff --git a/Documentation/technical/api-string-list.txt >> b/Documentation/technical/api-string-list.txt >> index 9206f8f..291ac4c 100644 >> --- a/Documentation/technical/api-string-list.txt >> +++ b/Documentation/technical/api-string-list.txt >> @@ -68,6 +68,14 @@ Functions >> to be deleted. Preserve the order of the items that are >> retained. >> >> +`string_list_longest_prefix`:: >> + >> +Return the longest string within a string_list that is a >> +prefix (in the sense of prefixcmp()) of the specified string, >> +or NULL if no such prefix exists. This function does not >> +require the string_list to be sorted (it does a linear >> +search). >> + >> `print_string_list`:: > > This may feel like outside the scope of this series, but since this > series will be the main culprit for adding many new functions to > this API in the recent history... > > - We may want to name things a bit more consistently so that people >can tell which ones can be called on any string list, which ones >are sorted list only, and which ones are unsorted one only. > >In addition, the last category _may_ need a bit more thought. >Calling unsorted_string_list_lookup() on an already sorted list >is not a crime---it is just a stupid thing to do. Yes, this could be clearer. Though I'm skeptical that a naming convention can capture all of the variation without being too cumbersome. Another idea: in string-list.h, one could name parameters "sorted_list" when they must be sorted as a precondition of the function. But before getting too hung up on finery, the effort might be better invested adding documentation for functions that are totally lacking it, like string_list_clear_func() for_each_string_list() for_each_string_list_item() string_list_find_insert_index() string_list_insert_at_index() While we're on the subject, it seems to me that documenting APIs like these in separate files under Documentation/technical rather than in the header files themselves - makes the documentation for a particular function harder to find, - makes it easier for the documentation to get out of sync with the actual collection of functions (e.g., the 5 undocumented functions listed above). - makes it awkward for the documentation to refer to particular function parameters by name. While it is nice to have a high-level prose description of an API, I am often frustrated by the lack of "docstrings" in the header file where a function is declared. The high-level description of an API could be put at the top of the header file. Also, better documentation in header files could enable the automatic generation of API docs (e.g., via doxygen). Is there some reason for the current documentation policy or is it historical and just needs somebody to put in the work to change it? > - Why are these new functions described at the top, not appended at >the bottom? I would have expected either an alphabetical, or a >more generic ones first (i.e. print and clear are a lot "easier" >ones compared to filter and prefix that are very much more >specialized). The order seemed logical to me at the time (given the constraint that functions are grouped by sorted/unsorted/don't-care): print_string_list() is only useful for debugging, so it seemed to belong below the "production" functions. string_list_clear() was already below print_string_list() (which I guessed was because it is logically used last in the life of a string_list) so I left it at the end of its section. My preference would probably have been to move print_string_list() below string_list_clear(), but somebody else made the opposite choice so I decided to respect it. That being said, I don't have anything against a different order. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- 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 9/9] Add git-check-ignores
On Tue, Sep 04, 2012 at 08:06:12PM +0700, Nguyen Thai Ngoc Duy wrote:
> On Sun, Sep 2, 2012 at 7:12 AM, Adam Spiers wrote:
> > --- a/builtin/add.c
> > +++ b/builtin/add.c
> > @@ -273,7 +273,7 @@ static int add_files(struct dir_struct *dir, int flags)
> > fprintf(stderr, _(ignore_error));
> > for (i = 0; i < dir->ignored_nr; i++)
> > fprintf(stderr, "%s\n", dir->ignored[i]->name);
> > - fprintf(stderr, _("Use -f if you really want to add
> > them.\n"));
> > + fprintf(stderr, _("Use -f if you really want to add them,
> > or git check-ignore to see\nwhy they're ignored.\n"));
> > die(_("no files added"));
> > }
>
> String too long (> 80 chars).
You mean the line of code is too long, or the argument to _(), or
both? I didn't like this either, but I saw that builtin/checkout.c
already did something similar twice, and I wasn't sure how else to do
it. Suggestions gratefully received.
> > +static const char * const check_ignore_usage[] = {
> > +"git check-ignore pathname...",
> > +"git check-ignore --stdin [-z] < ",
> > +NULL
> > +};
> > +
> > +static const struct option check_ignore_options[] = {
> > + OPT_BOOLEAN(0 , "stdin", &stdin_paths, "read file names from
> > stdin"),
> > + OPT_BOOLEAN('z', NULL, &null_term_line,
> > + "input paths are terminated by a null character"),
> > + OPT_END()
> > +};
>
> You may want to mark help strings ("read file names from stdin" and
> "input paths... null character") and check_ignore_usage[] for
> translation. Just wrap those strings with N_() and you'll be fine. For
> similar changes, check out nd/i18n-parseopt-help on branch 'pu'.
Thanks, I'll do that.
[snipped discussion of "include" / "exclude" which already continued elsewhere]
> > +static void check_ignore(const char *prefix, const char **pathspec)
> > +{
> > + struct dir_struct dir;
> > + const char *path;
> > + char *seen = NULL;
> > +
> > + /* read_cache() is only necessary so we can watch out for
> > submodules. */
> > + if (read_cache() < 0)
> > + die(_("index file corrupt"));
> > +
> > + memset(&dir, 0, sizeof(dir));
> > + dir.flags |= DIR_COLLECT_IGNORED;
> > + setup_standard_excludes(&dir);
>
> You should support ignore rules from files and command line arguments
> too, like ls-files. For quick testing.
You mean --exclude, --exclude-from, and --exclude-per-directory?
Sure, although I have limited time right now, so maybe these could be
added in a later iteration?
> > +static NORETURN void error_with_usage(const char *msg)
> > +{
> > + error("%s", msg);
> > + usage_with_options(check_ignore_usage, check_ignore_options);
> > +}
>
> Interesting. We have usage_msg_opt() in parse-options.c, but it's more
> verbose. Perhaps this function should be moved to parse-options.c
> because it may be useful to other commands as well?
I'll look into 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 9/9] Add git-check-ignores
On Wed, Sep 05, 2012 at 05:25:25PM +0700, Nguyen Thai Ngoc Duy wrote:
> On Wed, Sep 5, 2012 at 12:26 AM, Junio C Hamano wrote:
> > Nguyen Thai Ngoc Duy writes:
> >
> >>> +static void output_exclude(const char *path, struct exclude *exclude)
> >>> +{
> >>> + char *type = exclude->to_exclude ? "excluded" : "included";
> >>> + char *bang = exclude->to_exclude ? "" : "!";
> >>> + char *dir = (exclude->flags & EXC_FLAG_MUSTBEDIR) ? "/" : "";
> >>> + printf(_("%s: %s %s%s%s "), path, type, bang, exclude->pattern,
> >>> dir);
> >>
> >> These English words "excluded" and "included" make the translator me
> >> want to translate them. But they could be the markers for scripts, so
> >> they may not be translated. How about using non alphanumeric letters
> >> instead?
> >
> > I agree they should not be translated, but it is a disease to think
> > unintelligible mnemonic is a better input format for scripts than
> > the spelled out words. "excluded/included" pair is just fine.
>
> Not all mnemonic is unintelligible though. "+" and "-" may fit well in
> this case. I'm just trying to make sure we have checked the mnemonic
> pool before ending up with excluded/included.
Personally I'd be against introducing "+" and "-" when we already have
"!" and "". Even though "+" and "-" are more intuitive, it would
create inconsistency and IMHO confusion.
I'm still unconvinced that it's worth having a separate type field in
the output when the pattern field already has a "!" prefix for
inclusions. Does a separate field really help porcelain writers or
make the output more readable?
--
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/4] Add a new function, string_list_split_in_place()
On 09/10/2012 07:47 AM, Junio C Hamano wrote:
> Michael Haggerty writes:
>
>> ... Consider something like
>>
>> struct string_list *split_file_into_words(FILE *f)
>> {
>> char buf[1024];
>> struct string_list *list = new string list;
>> list->strdup_strings = 1;
>> while (not EOF) {
>> read_line_into_buf();
>> string_list_split_in_place(list, buf, ' ', -1);
>> }
>> return list;
>> }
>
> That is a prime example to argue that string_list_split() would make
> more sense, no? The caller does _not_ mind if the function mucks
> with buf, but the resulting list is not allowed to point into buf.
>
> In such a case, the caller shouldn't have to _care_ if it wants to
> allow buf to be mucked with; it is already asking that the resulting
> list _not_ point into buf by setting strdup_strings (by the way,
> that is part of the function input, so think of it like various *opt
> variables passed into functions to tweak their behaviour). If the
> implementation can do so without sacrificing performance (and in
> this case, as you said, it can), it should take "const char *buf".
You're right, I was thinking that a caller of
string_list_split_in_place() could choose to remain ignorant of whether
strdup_strings is set, but this is incorrect because it needs to know
whether to do its own memory management of the strings that it passes
into string_list_append().
> So it appears to me that sl_split_in_place(), if implemented, should
> be kept as a special case for performance-minded callers that have
> full control of the lifetime rules of the variables they use, can
> set strdup_strings to false, and can let buf modified in place, and
> can accept list that point into buf.
OK, so the bottom line would be to have two versions of the function.
One takes a (const char *) and *requires* strdup_strings to be set on
its input list:
int string_list_split(struct string_list *list, const char *string,
int delim, int maxsplit)
{
assert(list->strdup_strings);
...
}
The other takes a (char *) and modifies it in-place, and maybe even
requires strdup_strings to be false on its input list:
int string_list_split_in_place(struct string_list *list, char *string,
int delim, int maxsplit)
{
/* not an error per se but a strong suggestion of one: */
assert(!list->strdup_strings);
...
}
(The latter (modulo assert) is the one that I have implemented, but it
might not be needed immediately.) Do you agree?
+ * Examples:
+ * string_list_split_in_place(l, "foo:bar:baz", ':', -1) -> ["foo",
"bar", "baz"]
+ * string_list_split_in_place(l, "foo:bar:baz", ':', 1) -> ["foo",
"bar:baz"]
+ * string_list_split_in_place(l, "foo:bar:", ':', -1) -> ["foo", "bar",
""]
>>>
>>> I would find it more natural to see a sentinel value against
>>> "positive" to be 0, not -1. "-1" gives an impression as if "-2"
>>> might do something different from "-1", but Zero is a lot more
>>> special.
>>
>> You have raised a good point and I think there is a flaw in the API, but
>> I'm not sure I agree with you what the flaw is...
>>
>> The "maxsplit" argument limits the number of times the string should be
>> split. I.e., if maxsplit is set, then the output will have at most
>> (maxsplit + 1) strings.
>
> So "do not split, just give me the whole thing" would be maxsplit == 0
> to split into (maxsplit+1) == 1 string. I think we are in agreement
> that your "-1" does not make any sense, and your documentation that
> said "positive" is the saner thing to do, no?
No. I think that my handling of maxsplit=0 was incorrect but that we
should continue using -1 as the special value.
I see maxsplit=0 as a legitimate degenerate case meaning "split into 1
string". Granted, it would only be useful in specialized situations
[1]. Moreover, "-1" makes a much more obvious special value than "0";
somebody reading code with maxsplit=-1 knows immediately that this is a
special value, whereas the handling of maxsplit=0 isn't quite so
blindingly obvious unless the reader knows the outcome of our current
discussion :-)
Therefore I still prefer treating only negative values of maxsplit to
mean "unlimited" and fixing maxsplit=0 as described. But if you insist
on the other convention, let me know and I will change it.
Michael
[1] A case I can think of would be parsing a format like
NUMPARENTS [PARENT...] SUMMARY
where "string_list_split(list, rest_of_line, ' ', numparents)" does the
right thing even if numparents==0.
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
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 9/8] t0060: split absolute path test in two to exercise some of it on Windows
Looks fine to me. Thanks! Michael On 09/09/2012 05:42 PM, Johannes Sixt wrote: > Only the first half of the test works only on POSIX, the second half > passes on Windows as well. > > A later test "real path removes other extra slashes" looks very similar, > but it does not make sense to split it in the same way: When two slashes > are prepended in front of an absolute DOS-style path on Windows, the > meaning of the path is changed (//server/share style), so that the test > cannot pass on Windows. > > Signed-off-by: Johannes Sixt > --- > The series passes for me as is, but one test needs POSIX only in > the first half. This patch splits it in two. > > t/t0060-path-utils.sh | 8 ++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh > index e40f764..4ef2345 100755 > --- a/t/t0060-path-utils.sh > +++ b/t/t0060-path-utils.sh > @@ -148,10 +148,14 @@ test_expect_success 'real path rejects the empty > string' ' > test_must_fail test-path-utils real_path "" > ' > > -test_expect_success POSIX 'real path works on absolute paths' ' > +test_expect_success POSIX 'real path works on absolute paths 1' ' > nopath="hopefully-absent-path" && > test "/" = "$(test-path-utils real_path "/")" && > - test "/$nopath" = "$(test-path-utils real_path "/$nopath")" && > + test "/$nopath" = "$(test-path-utils real_path "/$nopath")" > +' > + > +test_expect_success 'real path works on absolute paths 2' ' > + nopath="hopefully-absent-path" && > # Find an existing top-level directory for the remaining tests: > d=$(pwd -P | sed -e "s|^\([^/]*/[^/]*\)/.*|\1|") && > test "$d" = "$(test-path-utils real_path "$d")" && > -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- 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: quiet shell commands when "make --silent"
gits...@pobox.com wrote on Sun, 09 Sep 2012 17:35 -0700: > Pete Wyckoff writes: > > > Option "--silent", "--quiet" or "-s" to make prevents > > echoing of commands as they are executed. However, there > > are some explicit "echo" commands in the Makefile and in > > the two GIT-VERSION-GEN scripts that always echo. > > "make -s clean"? Fixed here. > I am not very enthused, especially if the primary motivation is > about "check-docs". Such a script must be prepared to filter out > cruft from the output of $(MAKE) and to pick out the bits that > interests it and that has been the way of life with $(MAKE) way > before Git started as a project ;-). My motivation was to quiet output from a script I use to test bisectability. I sent it out because I noticed someone else found the verbosity annoying too. Agreed that "fixing" check-docs is not important; that's why I didn't bother in this patch. -- Pete -- 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 9/9] Add git-check-ignores
On Mon, Sep 10, 2012 at 6:09 PM, Adam Spiers wrote:
>> > fprintf(stderr, "%s\n", dir->ignored[i]->name);
>> > - fprintf(stderr, _("Use -f if you really want to add
>> > them.\n"));
>> > + fprintf(stderr, _("Use -f if you really want to add them,
>> > or git check-ignore to see\nwhy they're ignored.\n"));
>> > die(_("no files added"));
>> > }
>>
>> String too long (> 80 chars).
>
> You mean the line of code is too long, or the argument to _(), or
> both? I didn't like this either, but I saw that builtin/checkout.c
> already did something similar twice, and I wasn't sure how else to do
> it. Suggestions gratefully received.
I don't rememeber :( I might mean the output because I missed "\n" in
the middle. At least you can split the string in to at "\n" to make it
resemble output.
>> You should support ignore rules from files and command line arguments
>> too, like ls-files. For quick testing.
>
> You mean --exclude, --exclude-from, and --exclude-per-directory?
> Sure, although I have limited time right now, so maybe these could be
> added in a later iteration?
Sure, no problem. It's not hard to add them anyway (I think).
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: receive.denyNonNonFastForwards not denying force update
Just to close the loop on this thread, it did turn out to be a permission problem in our case. It was difficult to track down because it was only a problem on one server in the cluster. Each server had a system git config file at /usr/local/etc/gitconfig. This was a symlink pointing to a single common config file at /etc/gitconfig. This real file had correct content and permissions, and all the machines where eclipse.org allows shell access had correct symlinks. So any tests on the command line always showed that the system config looked fine. However on git.eclipse.org, which is the machine with the central repositories we are pushing to, the symlink was missing o+rx. For security reasons this machine doesn't allow shell access, but our pushes to this machine were failing to honour the system configuration. I gather the patch prepared earlier in this thread will cause an error to be reported when the system config could not be read, which sounds like a good fix to help others track down problems like this. John Arthorne On Fri, Aug 17, 2012 at 12:26 PM, John Arthorne wrote: > At eclipse.org we wanted all git repositories to disallow non-fastforward > commits by default. So, we set receive.denyNonFastForwards=true as a system > configuration setting. However, this does not prevent a non-fastforward > force push. If we set the same configuration setting in the local repository > configuration then it does prevent non-fastforward pushes. > > For all the details see this bugzilla, particularly comment #59 where we > finally narrowed this down: > > https://bugs.eclipse.org/bugs/show_bug.cgi?id=343150 > > This is on git version 1.7.4.1. > > The Git book recommends setting this property at the system level: > > http://git-scm.com/book/ch7-1.html (near the bottom) > > Can someone confirm if this is intended behaviour or not. > > Thanks, > John Arthorne -- 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: Probable bug in file run-command.c function clear_child_for_cleanup
On Sun, Sep 09, 2012 at 03:44:54PM +0100, David Gould wrote:
> static void clear_child_for_cleanup(pid_t pid)
> {
> struct child_to_clean **last, *p;
>
> last = &children_to_clean;
> for (p = children_to_clean; p; p = p->next) {
> if (p->pid == pid) {
> *last = p->next;
> free(p);
> return;
> }
> }
> }
>
> It appears that last is intended to point to the next field that's
> being updated, but fails to "follow" the p pointer along the chain.
> The result is that children_to_clean will end up pointing to the
> entry after the deleted one, and all the entries before it will be
> lost. It'll only be fine in the case that the pid is that of the
> first entry in the chain.
Yes, it's a bug. We should update "last" on each iteration.
> You want something like:
>
> for (... {
> if (... {
> ...
> }
> last = &p->next;
> }
>
> or (probably clearer, but I haven't read your coding style guide, if
> there is one, and some people don't like this approach)
Yes, that's the correct fix. Care to submit a patch?
> for (p = children_to_clean; p; last = &p->next, p = p->next) {
> ...
That is OK, too, but I think I prefer the first one.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Probable bug in file run-command.c function clear_child_for_cleanup
On Mon, Sep 10, 2012 at 3:44 PM, Jeff King wrote:
> On Sun, Sep 09, 2012 at 03:44:54PM +0100, David Gould wrote:
>> You want something like:
>>
>> for (... {
>> if (... {
>> ...
>> }
>> last = &p->next;
>> }
>>
>> or (probably clearer, but I haven't read your coding style guide, if
>> there is one, and some people don't like this approach)
>
> Yes, that's the correct fix. Care to submit a patch?
>
>> for (p = children_to_clean; p; last = &p->next, p = p->next) {
>> ...
>
> That is OK, too, but I think I prefer the first one.
>
I feel like bikeshedding a bit today!
I tend to either prefer either the latter or something like this:
while (p) {
...
last = p;
p = p->next;
}
because those approaches put all the iteration logic in the same
place. The in-body traversal approach is a bit more explicit about the
traversal details.
And to conclude my bikeshedding for the day: Shouldn't "last" ideally
be called something like "prev" instead? It's the previously visited
element, not the last element in the 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 2/2] attr: "binary" attribute should choose built-in "binary" merge driver
On Sat, Sep 08, 2012 at 09:40:39PM -0700, Junio C Hamano wrote: > The built-in "binary" attribute macro expands to "-diff -text", so > that textual diff is not produced, and the contents will not go > through any CR/LF conversion ever. During a merge, it should also > choose the "binary" low-level merge driver, but it didn't. > > Make it expand to "-diff -merge -text". Yeah, that seems like the obviously correct thing to do. In practice, most files would end up in the first few lines of ll_xdl_merge checking buffer_is_binary anyway, so I think this would really only make a difference when our "is it binary?" heuristic guesses wrong. > Documentation/gitattributes.txt | 2 +- > attr.c | 2 +- > t/t6037-merge-ours-theirs.sh| 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) Patch itself looks good to me. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Probable bug in file run-command.c function clear_child_for_cleanup
On Mon, Sep 10, 2012 at 03:58:40PM +0200, Erik Faye-Lund wrote:
> >> for (... {
> >> if (... {
> >> ...
> >> }
> >> last = &p->next;
> >> }
> [...]
> I feel like bikeshedding a bit today!
>
> I tend to either prefer either the latter or something like this:
>
> while (p) {
> ...
>
> last = p;
> p = p->next;
> }
>
> because those approaches put all the iteration logic in the same
> place. The in-body traversal approach is a bit more explicit about the
> traversal details.
Also fine by me.
> And to conclude my bikeshedding for the day: Shouldn't "last" ideally
> be called something like "prev" instead? It's the previously visited
> element, not the last element in the list.
It is the "last" element visited (just as "last week" is not the end of
the world), but yes, it is ambiguous, and "prev" is not. Either is fine
by me.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] daemon: restore getpeername(0,...) use
On Sat, Sep 08, 2012 at 09:20:48PM +0200, Jan Engelhardt wrote: > On Saturday 2012-09-08 20:59, Junio C Hamano wrote: > >> diff --git a/daemon.c b/daemon.c > >> index 4602b46..eaf08c2 100644 > >> --- a/daemon.c > >> +++ b/daemon.c > >> @@ -1,3 +1,4 @@ > >> +#include > >> #include "cache.h" > >> #include "pkt-line.h" > >> #include "exec_cmd.h" > > > >Platform agnostic parts of the code that use "git-compat-util.h" > >(users of "cache.h" are indirectly users of it) are not allowed to > >do platform specific include like this at their beginning. > > > >This is the first use of stdbool.h; what do you need it for? > > For the use in setenv(,,true). It was not entirely obvious in which .h > to add it; the most reasonable place was daemon.c itself, since the > other .c files do not seem to need it. It would go in git-compat-util.h. However, it really is not needed; you can simply pass "1" to setenv, as every other callsite in git does. More importantly, though, is it actually portable? I thought it was added in C99, and we try to stick to C89 to support older compilers and systems. My copy of C99 is vague (it says only that the "bool" macro was added via stdbool.h in C99, but nothing about the "true" and "false" macros), and I don't have a copy of C89 handy. Wikipedia does claim the header wasn't standardized at all until C99: https://en.wikipedia.org/wiki/C_standard_library -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] daemon: restore getpeername(0,...) use
Jeff King wrote:
On Sat, Sep 08, 2012 at 09:20:48PM +0200, Jan Engelhardt wrote:
On Saturday 2012-09-08 20:59, Junio C Hamano wrote:
diff --git a/daemon.c b/daemon.c
index 4602b46..eaf08c2 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1,3 +1,4 @@
+#include
#include "cache.h"
#include "pkt-line.h"
#include "exec_cmd.h"
Platform agnostic parts of the code that use "git-compat-util.h"
(users of "cache.h" are indirectly users of it) are not allowed to
do platform specific include like this at their beginning.
This is the first use of stdbool.h; what do you need it for?
For the use in setenv(,,true). It was not entirely obvious in which
.h to add it; the most reasonable place was daemon.c itself, since
the other .c files do not seem to need it.
It would go in git-compat-util.h. However, it really is not needed;
you can simply pass "1" to setenv, as every other callsite in git
does.
More importantly, though, is it actually portable? I thought it was
added in C99, and we try to stick to C89 to support older compilers
and systems. My copy of C99 is vague (it says only that the "bool"
macro was added via stdbool.h in C99, but nothing about the "true"
and "false" macros), and I don't have a copy of C89 handy. Wikipedia
does claim the header wasn't standardized at all until C99:
https://en.wikipedia.org/wiki/C_standard_library
Indeed stdbool is not part of C89, but inline isn't either and used
extensively in git (could possible be defined away), as are non-const array
intializers, e.g.:
const char *args[] = { editor, path, NULL };
^
".../git/editor.c", line 39: error(122): expression must have a constant
value
So git source is not plain C89 code (anymore?)
Bye, Jojo
--
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] daemon: restore getpeername(0,...) use
On Mon, Sep 10, 2012 at 04:38:58PM +0200, Joachim Schmitz wrote:
> >More importantly, though, is it actually portable? I thought it was
> >added in C99, and we try to stick to C89 to support older compilers
> >and systems. My copy of C99 is vague (it says only that the "bool"
> >macro was added via stdbool.h in C99, but nothing about the "true"
> >and "false" macros), and I don't have a copy of C89 handy. Wikipedia
> >does claim the header wasn't standardized at all until C99:
> >
> > https://en.wikipedia.org/wiki/C_standard_library
>
> Indeed stdbool is not part of C89, but inline isn't either and used
> extensively in git (could possible be defined away),
You can define INLINE in the Makefile to disable it (or set it to
something more appropriate for your system).
> as are non-const array intializers, e.g.:
>
>const char *args[] = { editor, path, NULL };
> ^
> ".../git/editor.c", line 39: error(122): expression must have a
> constant value
>
> So git source is not plain C89 code (anymore?)
I remember we excised a whole bunch of non-constant initializers at some
point because somebody's compiler was complaining. But I suppose this
one has slipped back in, because non-constant initializers are so damn
useful. And nobody has complained, which I imagine means nobody has
bothered building lately on those older systems that complained.
My "we stick to C89" is a little bit of a lie. We do not care about
specific standards. We do care about running everywhere on reasonable
systems. So something that is C99 might be OK if realistically everybody
has it. And something that is POSIX is not automatically OK if there are
many real-world systems that do not have it.
Since there is no written standard, there tends to be an organic ebb and
flow in which features we use. Somebody will use a feature that is not
portable because it's useful to them, and then somebody whose system
will no longer build git will complain, and then we'll fix it and move
on. As reviewers, we try to anticipate those breakages and stop them
early (especially if they are ones we have seen before and know are a
problem), but we do not always get it right. And sometimes it is even
time to revisit old decisions (the line you mentioned is 2 years old,
and nobody has complained; maybe all of the old systems have become
obsolete, and we no longer need to care about constant initializers).
Getting back to the patch at hand, it may be that stdbool is practically
available everywhere. Or that we could trivially emulate it by defining
a "true" macro in this case. But it is also important to consider
whether that complexity is worth it. This would be the first and only
spot in git to use "true". Why bother?
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] Add a new function, string_list_split_in_place()
Michael Haggerty writes:
> OK, so the bottom line would be to have two versions of the function.
> One takes a (const char *) and *requires* strdup_strings to be set on
> its input list:
>
> int string_list_split(struct string_list *list, const char *string,
> int delim, int maxsplit)
> {
> assert(list->strdup_strings);
> ...
> }
>
> The other takes a (char *) and modifies it in-place, and maybe even
> requires strdup_strings to be false on its input list:
>
> int string_list_split_in_place(struct string_list *list, char *string,
> int delim, int maxsplit)
> {
> /* not an error per se but a strong suggestion of one: */
> assert(!list->strdup_strings);
> ...
> }
>
> (The latter (modulo assert) is the one that I have implemented, but it
> might not be needed immediately.) Do you agree?
OK; I do not offhand know which one you immediately needed, but I
think that is a sensible way to structure the API.
> [1] A case I can think of would be parsing a format like
>
> NUMPARENTS [PARENT...] SUMMARY
>
> where "string_list_split(list, rest_of_line, ' ', numparents)" does the
> right thing even if numparents==0.
OK.
--
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] blame: respect "core.ignorecase"
On Sun, Sep 09, 2012 at 12:24:58PM -0700, Junio C Hamano wrote: > Having said all that, I am not sure if the "fixing" is really the > right approach to begin with. Contrast these two: > > $ git blame MakeFILE > $ git blame HEAD -- MakeFILE > > The latter, regardless of core.ignorecase, should fail, with "No > such path MakeFILE in HEAD". The former is merely an extension to > the latter, in the sense that the main traversal is exactly the same > as the latter, but on top, local modifications are blamed to the > working tree. > > If we were to do anything, I would think the most sane thing to do > is a smaller patch to fix fake_working_tree_commit() where it calls > lstat() and _should_ die with "Cannot lstat MakeFILE" on a sane > filesystem. It does not currently make sure the path exists in the > HEAD exactly as given by the user (i.e. without core.ignorecase > matching), and die when it is not found. Yes, I think that is the only sensible thing here. The rest of this email is me essentially me agreeing with you and telling you things you already know, but I had a slightly different line of reasoning, so I thought I would share. As far as the original patch, if you are going to change blame, then it is only logical to change the whole revision parser so that "git log -- MAKEFILE" works. And I do not think that is a direction we want to go. core.ignorecase has never been about "make git case-insensitive". Git represents a case-sensitive tree, and will always do so because of the sha1 we compute over the tree objects. core.ignorecase is really "make case-sensitive git work around your case-insensitive filesystem"[1]. If the proposal were instead to add a certain type of pathspec that is case-insensitive[2], that would make much more sense to me. It is not violating git's case-sensitivity because it is purely a _query_ issue. And it is a feature you might use whether or not your filesystem is case sensitive. -Peff [1] I was going to submit a patch to Documentation/config.txt to make this more clear, but IMHO the current text is already pretty clear. [2] I did not keep up with Duy's work on pathspec magic-prefixes (and I could not find anything relevant in the code or documentation), but it seems like this would be a logical feature to support 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
[RFC] Add "edit" action for interactive rebase?
Occasionally, while I'm in the middle of an interactive rebase, I'd change my mind about the todo list and want to modify it. This means manually digging out the todo file from the rebase directory, and invoking the editor. So I thought it might be convenient to have an "edit" action that simply invokes the editor on the todo file but do nothing else. This should be safe to do in the middle of a rebase, since we don't preprocess the todo file and generate any state from it. I've also been manually editing the todo file a while now, and I never ran into any issues. I wonder if any others have ever ran into this situation, and would this be a useful feature to have in interactive rebase? Comments? This patch doesn't have any documentations yet. I'll add some documentations in another patch if we decide to include this. Andrew Wong (1): rebase -i: Teach "--edit" action git-rebase--interactive.sh | 6 ++ git-rebase.sh | 14 ++ 2 files changed, 20 insertions(+) -- 1.7.12.289.g0ce9864.dirty -- 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] rebase -i: Teach "--edit" action
This allows users to edit the todo list while they're in the middle of
an interactive rebase.
Signed-off-by: Andrew Wong
---
git-rebase--interactive.sh | 6 ++
git-rebase.sh | 14 ++
2 files changed, 20 insertions(+)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a09e842..e9dbcf3 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -775,6 +775,12 @@ skip)
do_rest
;;
+edit)
+ git_sequence_editor "$todo" ||
+die_abort "Could not execute editor"
+
+ exit
+ ;;
esac
git var GIT_COMMITTER_IDENT >/dev/null ||
diff --git a/git-rebase.sh b/git-rebase.sh
index 15da926..c394b8d 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -38,6 +38,7 @@ C=!passed to 'git apply'
continue! continue
abort! abort and check out the original branch
skip! skip current patch and continue
+edit! edit the todo list during interactive rebase
"
. git-sh-setup
. git-sh-i18n
@@ -194,6 +195,10 @@ do
test $total_argc -eq 2 || usage
action=${1##--}
;;
+ --edit)
+ test $total_argc -eq 2 || usage
+ action=${1##--}
+ ;;
--onto)
test 2 -le "$#" || usage
onto="$2"
@@ -306,6 +311,12 @@ then
fi
fi
+if test "$action" = "edit" &&
+ test "$type" != "interactive"
+then
+ die "$(gettext "The --edit action can only be used during interactive
rebase.")"
+fi
+
case "$action" in
continue)
# Sanity check
@@ -338,6 +349,9 @@ abort)
rm -r "$state_dir"
exit
;;
+edit)
+ run_specific_rebase
+ ;;
esac
# Make sure no rebase is in progress
--
1.7.12.289.g0ce9864.dirty
--
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: checkout extra files
On Fri, Sep 07, 2012 at 01:49:15PM -0700, Junio C Hamano wrote: > -- >8 -- > gitcli: contrast wildcard given to shell and to git > > People who are not used to working with shell may intellectually > understand how the command line argument is massaged by the shell > but still have a hard time visualizing the difference between > letting the shell expand fileglobs and having Git see the fileglob > to use as a pathspec. I think this is an improvement, but... > diff --git c/Documentation/gitcli.txt w/Documentation/gitcli.txt > index ea17f7a..220621b 100644 > --- c/Documentation/gitcli.txt > +++ w/Documentation/gitcli.txt > @@ -38,6 +38,22 @@ arguments. Here are the rules: > you have to say either `git diff HEAD --` or `git diff -- HEAD` to > disambiguate. > > + * Many commands allow wildcards in paths, but you need to protect > +them from getting globbed by the shell. These two mean different things: > ++ > + > +$ git checkout -- *.c > +$ git checkout -- \*.c > + > ++ > +The former lets your shell expand the fileglob, and you are asking > +the dot-C files in your working tree to be overwritten with the version > +in the index. The latter passes the `*.c` to Git, and you are asking > +the paths in the index that match the pattern to be checked out to your > +working tree. After running `git add hello.c; rm hello.c`, you will _not_ > +see `hello.c` in your working tree with the former, but with the latter > +you will. > + > When writing a script that is expected to handle random user-input, it is > a good practice to make it explicit which arguments are which by placing > disambiguating `--` at appropriate places. Look at the paragraph below your addition. It is typographically outside of the bulleted list you are adding to, but it really makes sense directly after the prior two bullet points, which are explicitly about disambiguation between revisions and paths. Your addition splits them apart. Does it make sense to join that final paragraph into what is now the third bullet, and then add your new text (the fourth bullet) after? -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG?] git rebase not accepting :/ syntax
Yann Dirson writes: > Hm. But then, git rev-parse $(git rev-parse :/Merge}^0 does work, a trivial > patch would appear to make things better. That, if done unconditionally, smells like a bad hack that wastes an extra fork for a corner case that appears only very rarely. I guess something like this upstream=$( git rev-parse --verify -q "$upstream_name"^0 || git rev-parse --verify -q $(git rev-parse --verify "$upstream_name")^0 ) || die "$(eval_gettext 'invalid upstream $upstream_name')" may be an acceptable usability workaround, but I wonder if we can do the same fallback inside the revision argument parser, so that git ":/Merge^0" first looks for a commit that has string "Merge^0" in it and if it fails then it looks for a commit that has string "Merge" and then apply "^0" to 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
Problems with repack during push
Hi, Whenever I do a git push origin of a new branch I see: Auto packing the repository for optimum performance. /usr/local/libexec/git-core/git-sh-setup: line 241: uname: command not found /usr/local/libexec/git-core/git-repack: line 56: rm: command not found /usr/local/libexec/git-core/git-repack: line 85: mkdir: command not found /usr/local/libexec/git-core/git-repack: line 85: rm: command not found fatal: 'repack' appears to be a git command, but we were not able to execute it. Maybe git-repack is broken? error: failed to run repack If I do a 'git repack' it works fine, so are these messages coming from the remote server? Thanks, Rob -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] Add a function string_list_longest_prefix()
Michael Haggerty writes: > Another idea: in string-list.h, one could name parameters "sorted_list" > when they must be sorted as a precondition of the function. That sounds like a very sensible thing to do. > But before getting too hung up on finery, the effort might be better > invested adding documentation for functions that are totally lacking it, > like > > string_list_clear_func() > for_each_string_list() > for_each_string_list_item() > string_list_find_insert_index() > string_list_insert_at_index() > > While we're on the subject, it seems to me that documenting APIs like > these in separate files under Documentation/technical rather than in the > header files themselves > > - makes the documentation for a particular function harder to find, > > - makes it easier for the documentation to get out of sync with the > actual collection of functions (e.g., the 5 undocumented functions > listed above). > > - makes it awkward for the documentation to refer to particular function > parameters by name. > > While it is nice to have a high-level prose description of an API, I am > often frustrated by the lack of "docstrings" in the header file where a > function is declared. The high-level description of an API could be put > at the top of the header file. > > Also, better documentation in header files could enable the automatic > generation of API docs (e.g., via doxygen). Yeah, perhaps you may want to look into doing an automated generation of Documentation/technical/api-*.txt files out of the headers. -- 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] rebase -i: Teach "--edit" action
Andrew Wong writes: > This allows users to edit the todo list while they're in the middle of > an interactive rebase. I like the idea. > +edit) > + git_sequence_editor "$todo" || > +die_abort "Could not execute editor" > + > + exit > + ;; Indent with space. Please, use tabs (same below). > index 15da926..c394b8d 100755 > --- a/git-rebase.sh > +++ b/git-rebase.sh > @@ -38,6 +38,7 @@ C=!passed to 'git apply' > continue! continue > abort! abort and check out the original branch > skip! skip current patch and continue > +edit! edit the todo list during interactive rebase Just "edit" may be a bit misleading, as we already have the "edit" action inside the todolist. I'd call this --edit-list to avoid ambiguity. This lacks tests, IMHO, as there are many corner-cases (e.g. should we be allowed to --edit-list while the worktree is in conflict?) that would deserve to be at least discussed, and as much as possible automatically tested. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Problems with repack during push
Rob Marshall writes: > If I do a 'git repack' it works fine, so are these > messages coming from the remote server? I guess so, and your remote server has a restricted environment (chroot or so) with very few commands allowed, which prevents shell-scripts from executing properly. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] cherry-pick: Append -x line on separate paragraph
On Sat, Sep 08, 2012 at 04:10:59PM +0200, Robin Stocker wrote: > > > Maybe the solution is to detect if the original commit message > > > ends with a trailer and in that case keep the existing behavior > > > of not inserting a blank line? > > > > Yeah, that sounds like a good change from "this makes the result > > look better" point of view. > > How do you think we could best detect a tailer? Check if all > lines of the last paragraph start with /[\w-]+: /? There is ends_rfc2822_footer() in builtin/commit.c, which is currently used for adding Signed-off-by lines. It might make sense to factor that out, and have a new: add_pseudoheader(struct strbuf *commit_message, const char *header) that would implement the same set of spacing rules for signed-off-by, cherry-picked-from, and anything else we end up adding later. I think pseudo-headers like these might also want duplicate suppression of the final line, which could be part of that function. Note that you would not want to suppress a duplicate line from the middle of the trailer, since you might want to sign-off twice (e.g., you sign-off the original, and then also the cherry-pick). But you would not want two duplicate lines at the end saying "Signed-off-by: ...", and I believe "git commit" already suppresses those duplicates. > I'm going to work on this and submit a new version of the patch. The > "Cherry-picked-from" change could then be made later on top of that. Yay. This has come up more than once over the years, so I am glad to see somebody working on it. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 9/9] Add git-check-ignores
Adam Spiers writes:
Administrivia. Please never deflect direct responses to you with
"Mail-Followup-To" header. I told my mailer to "follow-up" so that I
could give you advice in response, while adding others in the
discussion to Cc so that they do not have to repeat what I said, but
your "Mail-follow-up-to" forced my advice to go to Nguyen, who does
not need one.
> On Tue, Sep 04, 2012 at 08:06:12PM +0700, Nguyen Thai Ngoc Duy wrote:
>> On Sun, Sep 2, 2012 at 7:12 AM, Adam Spiers wrote:
>> > --- a/builtin/add.c
>> > +++ b/builtin/add.c
>> > @@ -273,7 +273,7 @@ static int add_files(struct dir_struct *dir, int flags)
>> > fprintf(stderr, _(ignore_error));
>> > for (i = 0; i < dir->ignored_nr; i++)
>> > fprintf(stderr, "%s\n", dir->ignored[i]->name);
>> > - fprintf(stderr, _("Use -f if you really want to add
>> > them.\n"));
>> > + fprintf(stderr, _("Use -f if you really want to add them,
>> > or git check-ignore to see\nwhy they're ignored.\n"));
>> > die(_("no files added"));
>> > }
>>
>> String too long (> 80 chars).
>
> You mean the line of code is too long, or the argument to _(), or
> both?
Both.
fprintf(stderr, _("Use -f if you really want to add them, or\n"
"run git check-ignore to see\nwhy they're
ignored.\n"));
But in this particular case, I tend to think the additional noise
does not add much value and something the user wouldn't want to see
over and over again (in other words, it belongs to "an advice").
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] Add a function string_list_longest_prefix()
On Mon, Sep 10, 2012 at 09:24:17AM -0700, Junio C Hamano wrote: > > While we're on the subject, it seems to me that documenting APIs like > > these in separate files under Documentation/technical rather than in the > > header files themselves > > > > - makes the documentation for a particular function harder to find, > > > > - makes it easier for the documentation to get out of sync with the > > actual collection of functions (e.g., the 5 undocumented functions > > listed above). > > > > - makes it awkward for the documentation to refer to particular function > > parameters by name. > > > > While it is nice to have a high-level prose description of an API, I am > > often frustrated by the lack of "docstrings" in the header file where a > > function is declared. The high-level description of an API could be put > > at the top of the header file. > > > > Also, better documentation in header files could enable the automatic > > generation of API docs (e.g., via doxygen). > > Yeah, perhaps you may want to look into doing an automated > generation of Documentation/technical/api-*.txt files out of the > headers. I was just documenting something in technical/api-* the other day, and had the same feeling. I'd be very happy if we moved to some kind of literate-programming system. I have no idea which ones are good or bad, though. I have used doxygen, but all I remember is it being painfully baroque. I'd much rather have something simple and lightweight, with an easy markup format. For example, this: http://tomdoc.org/ Looks much nicer to me than most doxygen I've seen. But again, it's been a while, so maybe doxygen is nicer than I remember. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] grep: optionally show only the match
Am 09.09.2012 23:58, schrieb Marcus Karlsson:
Make git-grep optionally omit the parts of the line before and after the
match.
Signed-off-by: Marcus Karlsson
---
Documentation/git-grep.txt | 8 +++-
builtin/grep.c | 2 ++
grep.c | 7 +--
grep.h | 1 +
4 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index cfecf84..6ef22cb 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -20,7 +20,8 @@ SYNOPSIS
[-c | --count] [--all-match] [-q | --quiet]
[--max-depth ]
[--color[=] | --no-color]
- [--break] [--heading] [-p | --show-function]
+ [--break] [--heading] [-o | --only-matching]
+ [-p | --show-function]
[-A ] [-B ] [-C ]
[-W | --function-context]
[-f ] [-e]
@@ -183,6 +184,11 @@ OPTIONS
Show the filename above the matches in that file instead of
at the start of each shown line.
+-o::
+--only-matching::
+ Show only the part of the matching line that matched the
+ pattern.
+
-p::
--show-function::
Show the preceding line that contains the function name of
diff --git a/builtin/grep.c b/builtin/grep.c
index 09ca4c9..56aba7b 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -782,6 +782,8 @@ int cmd_grep(int argc, const char **argv, const char
*prefix)
N_("print empty line between matches from different
files")),
OPT_BOOLEAN(0, "heading", &opt.heading,
N_("show filename only once above matches from same
file")),
+ OPT_BOOLEAN('o', "only-matching", &opt.only_matching,
+ N_("show only the matching part of a matched line")),
OPT_GROUP(""),
OPT_CALLBACK('C', "context", &opt, N_("n"),
N_("show context lines before and after matches"),
diff --git a/grep.c b/grep.c
index 04e3ec6..9fc888e 100644
--- a/grep.c
+++ b/grep.c
@@ -827,7 +827,9 @@ static void show_line(struct grep_opt *opt, char *bol, char
*eol,
if (match.rm_so == match.rm_eo)
break;
- output_color(opt, bol, match.rm_so, line_color);
+ if (opt->only_matching == 0)
+ output_color(opt, bol, match.rm_so,
+line_color);
output_color(opt, bol + match.rm_so,
match.rm_eo - match.rm_so,
opt->color_match);
@@ -837,7 +839,8 @@ static void show_line(struct grep_opt *opt, char *bol, char
*eol,
}
*eol = ch;
}
- output_color(opt, bol, rest, line_color);
+ if (opt->only_matching == 0)
+ output_color(opt, bol, rest, line_color);
opt->output(opt, "\n", 1);
}
The implementation keeps only the coloured parts. However, they are not
necessarily the same as the matching parts. This is more complicated
with git grep than with regular grep because the former has the
additional options --and and --not. Consider this:
$ git grep --not -e bla --or --not -e blub
Lines with only either "bla" or "blub" (or none of them) will be shown,
lines with both not. Both "bla" and "blub" will be highlighted. The
matching part is always the whole shown line.
René
--
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
Confirm my false error suspicions of Gitweb query injection
Hi Everyone,
I'm using Gitweb (Based on Git 1.7.9 on RHEL 5.8).
I was poking around and tried a GET Request (REQ) with some SQL
statements as a search query and noticed a 500. Can i just confirm
with anyone here that the error message I'm seeing in the Response
(RESP) is basically saying that the search parameters are invalid
because of it's funny chars are breaking the regex search and that
it's not anything database related. Thank you!
[REQ]
GET /git/?s=%28select+1234%2C HTTP/1.0
[RESP]
500 - Internal Server Error
Unmatched ( in regex; marked by <-- HERE in m/( <-- HERE select
1234,/ at /var/www/git/gitweb.cgi line 4845.
[Code at gitweb.cgi line 4845]
next if $searchtext and not $pr->{'path'} =~ /$searchtext/ and not
$pr->{'descr_long'} =~ /$searchtext/;
--
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] rebase -i: Teach "--edit" action
On Mon, Sep 10, 2012 at 12:25 PM, Matthieu Moy wrote: > Indent with space. Please, use tabs (same below). Ah, thanks. Good catch. > Just "edit" may be a bit misleading, as we already have the "edit" > action inside the todolist. I'd call this --edit-list to avoid > ambiguity. I thought that might be a bit confusing too. "--edit-list" doesn't seem informative about what "list" we're editing though. What about "--edit-todo"? Any suggestions are welcomed. > This lacks tests, IMHO, as there are many corner-cases (e.g. should we > be allowed to --edit-list while the worktree is in conflict?) that would > deserve to be at least discussed, and as much as possible automatically > tested. It does seem risky to do, since we're exposing something that used to be internal to "rebase -i". Though I don't see harm in allowing modifications even when there's a conflict, since we're not really committing anything, modifying index, or any worktree file. As long as the todo file exists, and we're stopped in the middle of a rebase, I think editing it shouldn't cause any problems. -- 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] rebase -i: Teach "--edit" action
On Mon, Sep 10, 2012 at 12:46:45PM -0400, Andrew Wong wrote: > > Just "edit" may be a bit misleading, as we already have the "edit" > > action inside the todolist. I'd call this --edit-list to avoid > > ambiguity. > > I thought that might be a bit confusing too. "--edit-list" doesn't > seem informative about what "list" we're editing though. What about > "--edit-todo"? Any suggestions are welcomed. Does it ever make sense to edit and then _not_ immediately continue? You can't affect the current commit anyway (it has already been pulled from the todo list), so the next thing you'd want to do it actually act on whatever you put into the todo list[1]. What if it was called --continue-with-edit or something, and then: > > This lacks tests, IMHO, as there are many corner-cases (e.g. should we > > be allowed to --edit-list while the worktree is in conflict?) that would > > deserve to be at least discussed, and as much as possible automatically > > tested. We would not even allow the edit if we were not OK to continue. -Peff [1] It does preclude using "--edit" to make a note about a later commit while you are in the middle of resolving a conflict or something. You'd have to do it at the end. I don't know if anybody actually cares about that. -- 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: Problems with repack during push
OK, thanks. I'll check with the guy who set up the server. Rob On 9/10/12 12:26 PM, Matthieu Moy wrote: Rob Marshall writes: If I do a 'git repack' it works fine, so are these messages coming from the remote server? I guess so, and your remote server has a restricted environment (chroot or so) with very few commands allowed, which prevents shell-scripts from executing properly. -- 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: checkout extra files
Jeff King writes: >> When writing a script that is expected to handle random user-input, it is >> a good practice to make it explicit which arguments are which by placing >> disambiguating `--` at appropriate places. > > Look at the paragraph below your addition. It is typographically outside > of the bulleted list you are adding to, but it really makes sense > directly after the prior two bullet points, which are explicitly about > disambiguation between revisions and paths. Your addition splits them > apart. Yes, I noticed it and thought about different possibilities, including making the description of glob the first bullet point. > Does it make sense to join that final paragraph into what is now the > third bullet, and then add your new text (the fourth bullet) after? I am not sure. After re-reading it, I do not think the fileglob discussion belongs to the existing "Here are the rules" list in the first place. It should probably be the extended description for the first point (revisions then paths) to explain what kind of "paths" we accept there. I generally consider follow-up paragraphs after bulleted list to be enhancements on any of the points in the list, not necessarily applying to all of them. The existing structure is: * point A (revisions and paths) * point B (-- can be used to disambiguate) * point C (ambiguation leads to an error) Note that point B and point C taken together imply corollary BC. So something like this would be the right thing to do: * point A * point B * point C Note that point B and point C taken together imply corollary BC. Also note that point A implies corollary AA. or even * point A * point B * point C Note that point A implies corollary AA. Also note that point B and point C taken together imply corollary BC. So perhaps something like this squashed in on top of the patch in question? Documentation/gitcli.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git c/Documentation/gitcli.txt w/Documentation/gitcli.txt index c70cd81..4413489 100644 --- c/Documentation/gitcli.txt +++ w/Documentation/gitcli.txt @@ -38,9 +38,9 @@ arguments. Here are the rules: you have to say either `git diff HEAD --` or `git diff -- HEAD` to disambiguate. - * Many commands allow wildcards in paths, but you need to protect - them from getting globbed by the shell. These two mean different - things: +Many commands allow wildcards in paths (see pathspec in +linkgit:gitglossary[7]), but you need to protect them +from getting globbed by the shell. These two mean different things: + $ git checkout -- *.c -- 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: Confirm my false error suspicions of Gitweb query injection
Joseph Leong writes: > [RESP] > 500 - Internal Server Error > Unmatched ( in regex; marked by <-- HERE in m/( <-- HERE select > 1234,/ at /var/www/git/gitweb.cgi line 4845. Gitweb is feeding your input as a perl regex, which is not really clean but shouldn't really harm either. I could reproduce with an old gitweb version, but newer gitwebs seem to be more clever about regular expression (there's an explicit tickbox to search for re, and the error message is clean when what you provide isn't a valid regexp). -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: checkout extra files
On Mon, Sep 10, 2012 at 10:09:43AM -0700, Junio C Hamano wrote: > > Does it make sense to join that final paragraph into what is now the > > third bullet, and then add your new text (the fourth bullet) after? > > I am not sure. After re-reading it, I do not think the fileglob > discussion belongs to the existing "Here are the rules" list in the > first place. I had a vague feeling that it did not quite belong, too, but I was not sure where it should go. > It should probably be the extended description for the > first point (revisions then paths) to explain what kind of "paths" > we accept there. I do not think so. That point is about the order of revisions and paths, and has nothing to do with the syntax of paths. Really, every element of that list is about handling revisions versus paths. I think this content does not necessarily go in such a list. > I generally consider follow-up paragraphs after bulleted list to be > enhancements on any of the points in the list, not necessarily > applying to all of them. I would argue the opposite; if it is about a specific point, then put it with the point. Otherwise, you are asking the reader to remember back to an earlier point (that they may not even have read; in reference documentation, the point of a list is often to let readers skip from bullet to bullet easily). If it is a synthesis of multiple elements in the list, then that makes more sense. And I think that is what you are implying here: > The existing structure is: > > * point A (revisions and paths) > * point B (-- can be used to disambiguate) > * point C (ambiguation leads to an error) > > Note that point B and point C taken together imply corollary BC. Which is fine by me. But inserting a point D that is not related to B, C, or BC, only makes it harder to read. > So perhaps something like this squashed in on top of the patch in > question? > > Documentation/gitcli.txt | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git c/Documentation/gitcli.txt w/Documentation/gitcli.txt > index c70cd81..4413489 100644 > --- c/Documentation/gitcli.txt > +++ w/Documentation/gitcli.txt > @@ -38,9 +38,9 @@ arguments. Here are the rules: > you have to say either `git diff HEAD --` or `git diff -- HEAD` to > disambiguate. > > - * Many commands allow wildcards in paths, but you need to protect > - them from getting globbed by the shell. These two mean different > - things: > +Many commands allow wildcards in paths (see pathspec in > +linkgit:gitglossary[7]), but you need to protect them > +from getting globbed by the shell. These two mean different things: > + > > $ git checkout -- *.c I don't think that makes it any better. You went from: * A * B * C * D By the way, B and C imply BC. to: * A * B * C By the way, D. Also, B and C imply BC. I think it would make more sense to do: * A * B * C By the way, B and C imply BC. Also, D. (where obviously my "connecting" phrases do not need to be part of the text, but are meant to illustrate how I am thinking about the structure). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] daemon: restore getpeername(0,...) use
Jeff King wrote:
On Mon, Sep 10, 2012 at 04:38:58PM +0200, Joachim Schmitz wrote:
More importantly, though, is it actually portable? I thought it was
added in C99, and we try to stick to C89 to support older compilers
and systems. My copy of C99 is vague (it says only that the "bool"
macro was added via stdbool.h in C99, but nothing about the "true"
and "false" macros), and I don't have a copy of C89 handy.
Wikipedia does claim the header wasn't standardized at all until
C99:
https://en.wikipedia.org/wiki/C_standard_library
Indeed stdbool is not part of C89, but inline isn't either and used
extensively in git (could possible be defined away),
You can define INLINE in the Makefile to disable it (or set it to
something more appropriate for your system).
That's what I meant.
as are non-const array intializers, e.g.:
const char *args[] = { editor, path, NULL };
^
".../git/editor.c", line 39: error(122): expression must have a
constant value
So git source is not plain C89 code (anymore?)
I remember we excised a whole bunch of non-constant initializers at
some point because somebody's compiler was complaining. But I suppose
this one has slipped back in, because non-constant initializers are
so damn useful. And nobody has complained, which I imagine means
nobody has bothered building lately on those older systems that
complained.
OK, record my complaint then ;-)
At least some older release of HP NonStop only have C89 and are still in use
And tying to compile in plain C89 mode revealed several other problems too
(e.g. size_t seems not to be typedef'd?)
My "we stick to C89" is a little bit of a lie. We do not care about
specific standards. We do care about running everywhere on reasonable
systems. So something that is C99 might be OK if realistically
everybody has it. And something that is POSIX is not automatically OK
if there are many real-world systems that do not have it.
Since there is no written standard, there tends to be an organic ebb
and flow in which features we use. Somebody will use a feature that
is not portable because it's useful to them, and then somebody whose
system will no longer build git will complain, and then we'll fix it
and move on. As reviewers, we try to anticipate those breakages and
stop them early (especially if they are ones we have seen before and
know are a problem), but we do not always get it right. And sometimes
it is even time to revisit old decisions (the line you mentioned is 2
years old, and nobody has complained; maybe all of the old systems
have become obsolete, and we no longer need to care about constant
initializers).
Getting back to the patch at hand, it may be that stdbool is
practically available everywhere. Or that we could trivially emulate
it by defining a "true" macro in this case. But it is also important
to consider whether that complexity is worth it. This would be the
first and only spot in git to use "true". Why bother?
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Confirm my false error suspicions of Gitweb query injection
Joseph Leong writes: > Hi Everyone, > > I'm using Gitweb (Based on Git 1.7.9 on RHEL 5.8). > > I was poking around and tried a GET Request (REQ) with some SQL > statements as a search query and noticed a 500. Can i just confirm > with anyone here that the error message I'm seeing in the Response > (RESP) is basically saying that the search parameters are invalid > because of it's funny chars are breaking the regex search and that > it's not anything database related. Yes, I think this was fixed in v1.7.9.4 if not earlier, with e65ceb6 (gitweb: Fix fixed string (non-regexp) project search, 2012-03-02). -- 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: Confirm my false error suspicions of Gitweb query injection
and you earned bonus points for the details - thank you very much! On Mon, Sep 10, 2012 at 10:37 AM, Junio C Hamano wrote: > Joseph Leong writes: > >> Hi Everyone, >> >> I'm using Gitweb (Based on Git 1.7.9 on RHEL 5.8). >> >> I was poking around and tried a GET Request (REQ) with some SQL >> statements as a search query and noticed a 500. Can i just confirm >> with anyone here that the error message I'm seeing in the Response >> (RESP) is basically saying that the search parameters are invalid >> because of it's funny chars are breaking the regex search and that >> it's not anything database related. > > Yes, I think this was fixed in v1.7.9.4 if not earlier, with e65ceb6 > (gitweb: Fix fixed string (non-regexp) project search, 2012-03-02). > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] Add a function string_list_longest_prefix()
On 09/10/2012 06:33 PM, Jeff King wrote: > On Mon, Sep 10, 2012 at 09:24:17AM -0700, Junio C Hamano wrote: > >>> While we're on the subject, it seems to me that documenting APIs like >>> these in separate files under Documentation/technical rather than in the >>> header files themselves >>> >>> - makes the documentation for a particular function harder to find, >>> >>> - makes it easier for the documentation to get out of sync with the >>> actual collection of functions (e.g., the 5 undocumented functions >>> listed above). >>> >>> - makes it awkward for the documentation to refer to particular function >>> parameters by name. >>> >>> While it is nice to have a high-level prose description of an API, I am >>> often frustrated by the lack of "docstrings" in the header file where a >>> function is declared. The high-level description of an API could be put >>> at the top of the header file. >>> >>> Also, better documentation in header files could enable the automatic >>> generation of API docs (e.g., via doxygen). >> >> Yeah, perhaps you may want to look into doing an automated >> generation of Documentation/technical/api-*.txt files out of the >> headers. > > I was just documenting something in technical/api-* the other day, and > had the same feeling. I'd be very happy if we moved to some kind of > literate-programming system. I have no idea which ones are good or bad, > though. I have used doxygen, but all I remember is it being painfully > baroque. I'd much rather have something simple and lightweight, with an > easy markup format. For example, this: > >http://tomdoc.org/ > > Looks much nicer to me than most doxygen I've seen. But again, it's been > a while, so maybe doxygen is nicer than I remember. > Doxygen has a the very nifty feature of being able to generate callgraphs though. We use it extensively at $dayjob, so if you need a hand building something sensible out of git's headers, I'd be happy to help. libgit2 uses doxygen btw, and has done since the start. If we ever merge the two, it would be neat to use the same. -- Andreas Ericsson andreas.erics...@op5.se OP5 AB www.op5.se Tel: +46 8-230225 Fax: +46 8-230231 Considering the successes of the wars on alcohol, poverty, drugs and terror, I think we should give some serious thought to declaring war on peace. -- 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] daemon: restore getpeername(0,...) use
On Mon, Sep 10, 2012 at 07:26:26PM +0200, Joachim Schmitz wrote:
> >>as are non-const array intializers, e.g.:
> >>
> >> const char *args[] = { editor, path, NULL };
> >> ^
> >>".../git/editor.c", line 39: error(122): expression must have a
> >>constant value
> >>
> >>So git source is not plain C89 code (anymore?)
> >
> >I remember we excised a whole bunch of non-constant initializers at
> >some point because somebody's compiler was complaining. But I suppose
> >this one has slipped back in, because non-constant initializers are
> >so damn useful. And nobody has complained, which I imagine means
> >nobody has bothered building lately on those older systems that
> >complained.
>
> OK, record my complaint then ;-)
Oops, did I say "complained"? I meant "sent patches". Hint, hint. :)
> At least some older release of HP NonStop only have C89 and are still in use
>
> And tying to compile in plain C89 mode revealed several other
> problems too (e.g. size_t seems not to be typedef'd?)
I think it is a mistake to set -std=c89 (or whatever similar option your
compiler supports). Like I said, we are not interested in being strictly
C89-compliant. We are interested in working on real-world systems.
If your compiler complains in the default mode (or when it is given some
reasonable practical settings), then that's something worth fixing. But
if your compiler is perfectly capable of compiling git, but you choose
to cripple it by telling it to be pedantic about a standard, then that
is not git's problem at all.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] daemon: restore getpeername(0,...) use
> From: Jeff King [mailto:p...@peff.net]
> Sent: Monday, September 10, 2012 7:59 PM
> To: Joachim Schmitz
> Cc: git@vger.kernel.org
> Subject: Re: [PATCH] daemon: restore getpeername(0,...) use
>
> On Mon, Sep 10, 2012 at 07:26:26PM +0200, Joachim Schmitz wrote:
>
> > >>as are non-const array intializers, e.g.:
> > >>
> > >> const char *args[] = { editor, path, NULL };
> > >> ^
> > >>".../git/editor.c", line 39: error(122): expression must have a
> > >>constant value
> > >>
> > >>So git source is not plain C89 code (anymore?)
> > >
> > >I remember we excised a whole bunch of non-constant initializers at
> > >some point because somebody's compiler was complaining. But I suppose
> > >this one has slipped back in, because non-constant initializers are
> > >so damn useful. And nobody has complained, which I imagine means
> > >nobody has bothered building lately on those older systems that
> > >complained.
> >
> > OK, record my complaint then ;-)
>
> Oops, did I say "complained"? I meant "sent patches". Hint, hint. :)
Oops ;-)
> > At least some older release of HP NonStop only have C89 and are still in use
> >
> > And tying to compile in plain C89 mode revealed several other
> > problems too (e.g. size_t seems not to be typedef'd?)
>
> I think it is a mistake to set -std=c89 (or whatever similar option your
> compiler supports). Like I said, we are not interested in being strictly
> C89-compliant. We are interested in working on real-world systems.
>
> If your compiler complains in the default mode (or when it is given some
> reasonable practical settings), then that's something worth fixing. But
> if your compiler is perfectly capable of compiling git, but you choose
> to cripple it by telling it to be pedantic about a standard, then that
> is not git's problem at all.
Older version of HP NonStop only have a c89 compiler, newer have a -Wc99lite
switch to that, which enables some C99 features and the latest additionally
have a c99 compiler. There's no switch to cripple something, it is just a fact
that older systems don't have c99 or only limited support for it. A whole
series of machines (which is still in use!) cannot get upgraded to anything
better than c89.
Bye, Jojo
--
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] Add "edit" action for interactive rebase?
Am 10.09.2012 18:14, schrieb Andrew Wong: > Occasionally, while I'm in the middle of an interactive rebase, I'd change my > mind about the todo list and want to modify it. This means manually digging > out the todo file from the rebase directory, and invoking the editor. So I > thought it might be convenient to have an "edit" action that simply invokes > the > editor on the todo file but do nothing else. > > This should be safe to do in the middle of a rebase, since we don't preprocess > the todo file and generate any state from it. I've also been manually editing > the todo file a while now, and I never ran into any issues. > > I wonder if any others have ever ran into this situation, and would this be > a useful feature to have in interactive rebase? Comments? Applause! A very welcome addition. I've found myself editing the todo list every now and then, and I'd like to do that more often. This new feature would make it dead simple. Did you think about what can go wrong? For example, starting with this todo sheet: exec false pick 1234567 and then the user changes the 'pick' to 'squash' after rebase stopped at the failed 'exec' command. -- Hannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rebase -i: Teach "--edit" action
Am 10.09.2012 18:54, schrieb Jeff King: > On Mon, Sep 10, 2012 at 12:46:45PM -0400, Andrew Wong wrote: > >>> Just "edit" may be a bit misleading, as we already have the "edit" >>> action inside the todolist. I'd call this --edit-list to avoid >>> ambiguity. >> >> I thought that might be a bit confusing too. "--edit-list" doesn't >> seem informative about what "list" we're editing though. What about >> "--edit-todo"? Any suggestions are welcomed. > > Does it ever make sense to edit and then _not_ immediately continue? Yes. For example, while you are resolving a conflict, you might notice that it would make sense to do something different in the remaining rebase sequence. You don't want to continue if some conflicts remain. And you don't want to wait editing the todo list until you are done with the conflicts because you might have forgotten that you wanted to do something different. > You can't affect the current commit anyway (it has already been pulled > from the todo list), so the next thing you'd want to do it actually act > on whatever you put into the todo list[1]. Oh, you said it here: > [1] It does preclude using "--edit" to make a note about a later commit > while you are in the middle of resolving a conflict or something. > You'd have to do it at the end. I don't know if anybody actually > cares about that. Yes, I do care. At times I tend to have a very short attention span. Or it is Windows's slowness that expires my short-term memory more often than not. ;) -- Hannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rebase -i: Teach "--edit" action
On Mon, Sep 10, 2012 at 08:36:43PM +0200, Johannes Sixt wrote: > > [1] It does preclude using "--edit" to make a note about a later commit > > while you are in the middle of resolving a conflict or something. > > You'd have to do it at the end. I don't know if anybody actually > > cares about that. > > Yes, I do care. At times I tend to have a very short attention span. Or > it is Windows's slowness that expires my short-term memory more often > than not. ;) OK, then I withdraw my proposal. :) It sounds like it would be safe to do: git rebase --edit-todo hack hack hack git rebase --continue anyway, so the restriction is not as valuable as it would otherwise have been. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Using doxygen (or something similar) to generate API docs [was [PATCH 4/4] Add a function string_list_longest_prefix()]
I'm renaming this thread so that the bikeshedding can get over ASAP. On 09/10/2012 07:48 PM, Andreas Ericsson wrote: > On 09/10/2012 06:33 PM, Jeff King wrote: >> On Mon, Sep 10, 2012 at 09:24:17AM -0700, Junio C Hamano wrote: >>> Michael Haggerty writes: Also, better documentation in header files could enable the automatic generation of API docs (e.g., via doxygen). >>> >>> Yeah, perhaps you may want to look into doing an automated >>> generation of Documentation/technical/api-*.txt files out of the >>> headers. >> >> I was just documenting something in technical/api-* the other day, and >> had the same feeling. I'd be very happy if we moved to some kind of >> literate-programming system. I have no idea which ones are good or bad, >> though. I have used doxygen, but all I remember is it being painfully >> baroque. I'd much rather have something simple and lightweight, with an >> easy markup format. For example, this: >> >>http://tomdoc.org/ >> >> Looks much nicer to me than most doxygen I've seen. But again, it's been >> a while, so maybe doxygen is nicer than I remember. I don't have a personal preference for what system is used. I mentioned doxygen only because it seems to be a well-known example. >From a glance at the URL you mentioned, it looks like TomDoc is only applicable to Ruby code. > Doxygen has a the very nifty feature of being able to generate > callgraphs though. We use it extensively at $dayjob, so if you need a > hand building something sensible out of git's headers, I'd be happy > to help. My plate is full. If you are able to work on this, it would be awesome. As far as I'm concerned, you are the new literate documentation czar :-) Most importantly, having a convenient system of converting header comments into documentation would hopefully motivate other people to add better header comments in the first place, and motivate reviewers to insist on them. It's shocking (to me) how few functions are documented, and how often I have to read masses of C code to figure out what a function is for, its pre- and post-conditions, its memory policy, etc. Often I find myself having to read functions three layers down the call tree to figure out the behavior of the top-layer function. I try to document things as I go, but it's only a drop in the bucket. > libgit2 uses doxygen btw, and has done since the start. If we ever > merge the two, it would be neat to use the same. That would be a nice bonus. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- 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] rebase -i: Teach "--edit" action
On Mon, Sep 10, 2012 at 2:46 PM, Jeff King wrote: > On Mon, Sep 10, 2012 at 08:36:43PM +0200, Johannes Sixt wrote: > >> > [1] It does preclude using "--edit" to make a note about a later commit >> > while you are in the middle of resolving a conflict or something. >> > You'd have to do it at the end. I don't know if anybody actually >> > cares about that. >> >> Yes, I do care. At times I tend to have a very short attention span. Or >> it is Windows's slowness that expires my short-term memory more often >> than not. ;) > > OK, then I withdraw my proposal. :) > > It sounds like it would be safe to do: > > git rebase --edit-todo > hack hack hack > git rebase --continue Johannes took the words right out of my mouth. Also, "edit and _not_ continue" also gives the user a chance to second guess while editing the todo. That got me thinking... Currently, the todo list has this line at the bottome: # However, if you remove everything, the rebase will be aborted. We'd probably want to remove that line, since "remove everything" no longer aborts the rebase. It'll just finish the rebase. It'll be ugly to sed it out. Maybe one way to do this is to remove all the comments and append new ones. It might also be nice to add a note to remind the user that they're editing a todo file in a stopped rebase state. i.e. not a fresh interactive rebase -- 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: checkout extra files
Jeff King writes: > I would argue the opposite; if it is about a specific point, then put it > with the point. Otherwise, you are asking the reader to remember back to > an earlier point (that they may not even have read; in reference > documentation, the point of a list is often to let readers skip from > bullet to bullet easily). You need to follow all the rules when composing your command line. You cannot simply ignore ones that are inconvenient for you and pick only the one you like. The second and the third one are related in the "sides of the same coin" sense; you either have "--" in which case no disambiguation checks are done, or don't in which case your command line may get an ambiguity error, so in that sense, you could say "I am writing '--', so point C does not apply to me and I skip". But whether you do or do not say '--', you have to have your revs before pathspecs, so you cannot skip point A. So I do not think a bullet list is designed to let the readers skip and forget (or "may not even have read"). If that is the case, perhaps we would need to use something else to give the set of rules that apply to the command line here. > I don't think that makes it any better. You went from: > > * A > * B > * C > * D > > By the way, B and C imply BC. > > to: > ... > I think it would make more sense to do: > > * A > * B > * C > > By the way, B and C imply BC. > > Also, D. I think the following is probably the best. * A (revs and then paths) * B (with "--", no dwim is done). * C (without "--", disambiguation kicks in. By the way, this means your script had better avoid this form; make sure you use "--"). * D (pathspecs are patterns). without the trailing paragraph, which is meant only for people who write their script without using "--" by mistake, i.e. it only belongs to point C. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] gitcli: formatting fix
The paragraph to encourage use of "--" in scripts belongs to the bullet point that describes the behaviour for a command line without the explicit "--" disambiguation, not a supporting explanation for the entire bulletted list. Signed-off-by: Junio C Hamano --- Documentation/gitcli.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt index ea17f7a..c4edf04 100644 --- a/Documentation/gitcli.txt +++ b/Documentation/gitcli.txt @@ -37,7 +37,7 @@ arguments. Here are the rules: file called HEAD in your work tree, `git diff HEAD` is ambiguous, and you have to say either `git diff HEAD --` or `git diff -- HEAD` to disambiguate. - ++ When writing a script that is expected to handle random user-input, it is a good practice to make it explicit which arguments are which by placing disambiguating `--` at appropriate places. -- 1.7.12.322.g2c7d289 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] gitcli: contrast wildcard given to shell and to git
People who are not used to working with shell may intellectually understand how the command line argument is massaged by the shell but still have a hard time visualizing the difference between letting the shell expand fileglobs and having Git see the fileglob to use as a pathspec. Signed-off-by: Junio C Hamano --- Documentation/gitcli.txt | 17 + 1 file changed, 17 insertions(+) diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt index c4edf04..00b8403 100644 --- a/Documentation/gitcli.txt +++ b/Documentation/gitcli.txt @@ -42,6 +42,23 @@ When writing a script that is expected to handle random user-input, it is a good practice to make it explicit which arguments are which by placing disambiguating `--` at appropriate places. + * Many commands allow wildcards in paths, but you need to protect + them from getting globbed by the shell. These two mean different + things: ++ + +$ git checkout -- *.c +$ git checkout -- \*.c + ++ +The former lets your shell expand the fileglob, and you are asking +the dot-C files in your working tree to be overwritten with the version +in the index. The latter passes the `*.c` to Git, and you are asking +the paths in the index that match the pattern to be checked out to your +working tree. After running `git add hello.c; rm hello.c`, you will _not_ +see `hello.c` in your working tree with the former, but with the latter +you will. + Here are the rules regarding the "flags" that you should follow when you are scripting git: -- 1.7.12.322.g2c7d289 -- 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] rebase -i: Teach "--edit" action
Andrew Wong writes: > On Mon, Sep 10, 2012 at 2:46 PM, Jeff King wrote: >> On Mon, Sep 10, 2012 at 08:36:43PM +0200, Johannes Sixt wrote: >> >>> > [1] It does preclude using "--edit" to make a note about a later commit >>> > while you are in the middle of resolving a conflict or something. >>> > You'd have to do it at the end. I don't know if anybody actually >>> > cares about that. >>> >>> Yes, I do care. At times I tend to have a very short attention span. Or >>> it is Windows's slowness that expires my short-term memory more often >>> than not. ;) >> >> OK, then I withdraw my proposal. :) >> >> It sounds like it would be safe to do: >> >> git rebase --edit-todo >> hack hack hack >> git rebase --continue > > Johannes took the words right out of my mouth. Also, "edit and _not_ > continue" also gives the user a chance to second guess while editing > the todo. do you mean "double check"? > That got me thinking... Currently, the todo list has this line at the bottome: > # However, if you remove everything, the rebase will be aborted. > > We'd probably want to remove that line, since "remove everything" no > longer aborts the rebase. It'll just finish the rebase. Good precaution. > It might also be nice to add a note to remind the user that they're > editing a todo file in a stopped rebase state. i.e. not a fresh > interactive rebase Hrm... They see the contents of the todo file immediately after they say "rebase --edit-todo" and the sole reason they said that command is because they wanted to edit the todo file. Is it likely they need a reminder? -- 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: Probable bug in file run-command.c function clear_child_for_cleanup
Jeff King writes:
> On Mon, Sep 10, 2012 at 03:58:40PM +0200, Erik Faye-Lund wrote:
>
>> >> for (... {
>> >> if (... {
>> >> ...
>> >> }
>> >> last = &p->next;
>> >> }
>> [...]
>> I feel like bikeshedding a bit today!
>>
>> I tend to either prefer either the latter or something like this:
>>
>> while (p) {
>> ...
>>
>> last = p;
>> p = p->next;
>> }
>>
>> because those approaches put all the iteration logic in the same
>> place. The in-body traversal approach is a bit more explicit about the
>> traversal details.
>
> Also fine by me.
>
>> And to conclude my bikeshedding for the day: Shouldn't "last" ideally
>> be called something like "prev" instead? It's the previously visited
>> element, not the last element in the list.
>
> It is the "last" element visited (just as "last week" is not the end of
> the world), but yes, it is ambiguous, and "prev" is not. Either is fine
> by me.
OK, so who's gonna do the honors?
--
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: Probable bug in file run-command.c function clear_child_for_cleanup
On Mon, Sep 10, 2012 at 01:00:35PM -0700, Junio C Hamano wrote: > >> And to conclude my bikeshedding for the day: Shouldn't "last" ideally > >> be called something like "prev" instead? It's the previously visited > >> element, not the last element in the list. > > > > It is the "last" element visited (just as "last week" is not the end of > > the world), but yes, it is ambiguous, and "prev" is not. Either is fine > > by me. > > OK, so who's gonna do the honors? I was hoping to give David a chance to submit his first-ever patch to git. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: checkout extra files
On Mon, Sep 10, 2012 at 12:35:05PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > I would argue the opposite; if it is about a specific point, then put it > > with the point. Otherwise, you are asking the reader to remember back to > > an earlier point (that they may not even have read; in reference > > documentation, the point of a list is often to let readers skip from > > bullet to bullet easily). > > You need to follow all the rules when composing your command line. > You cannot simply ignore ones that are inconvenient for you and pick > only the one you like. Of course. But note that I said "reference documentation". It is quite frequent that you have read it a long time ago, or understand already most of what it is saying, and you are re-reading it again looking for rules about a _specific_ element (say, wildcards). You might not have just now read the bit about disambiguation, but you do not need to; you already know it, or it might not be relevant to what you are doing. > The second and the third one are related in the "sides of the same > coin" sense; you either have "--" in which case no disambiguation > checks are done, or don't in which case your command line may get an > ambiguity error, so in that sense, you could say "I am writing '--', > so point C does not apply to me and I skip". But whether you do or > do not say '--', you have to have your revs before pathspecs, so you > cannot skip point A. I think we need to be realistic about the readers of our documentation. Sometimes people will sit down and read all the way through, and we need the text to flow and make sense for that case. But just as often, they will be curious about one specific point, and we need to make it as easy as possible for them to see when a new point is being made, and when they can stop reading because they have wandered into a new point that they may already know. Which is why I think the best thing we can do for such a casual reader is make sure that the typography helps group related text together. In this specific example, imagine I am a seasoned Unix user and a new git user. If I were reading about "--" and revs versus paths, that would be news to me, because it is about git. When I see the next bullet is about quoting "*" to pass it through the shell to git, I say "Of course. That is how Unix shells work" and stop reading. It seems like a disservice to the reader to include more on the "--" disambiguation _after_ that bullet point. > So I do not think a bullet list is designed to let the readers skip > and forget (or "may not even have read"). If that is the case, > perhaps we would need to use something else to give the set of rules > that apply to the command line here. I think it is OK here. As a tool for people reading the whole text, I think the list is a bad format, since the elements do not follow a good parallel structure (as you said, the second and third are much more related than the first and fourth). So I was tempted to suggest removing the list altogether and turning it into paragraphs. But as I said, I think breaking the points with whitespace helps the casual reader using it as a reference. I'm not sure you agree, but maybe what I've written above will change that. If not, I think I've said as much as is useful on the matter and I'll stop talking. :) > I think the following is probably the best. > > * A (revs and then paths) > * B (with "--", no dwim is done). > * C (without "--", disambiguation kicks in. By the way, this > means your script had better avoid this form; make sure you > use "--"). > * D (pathspecs are patterns). > > without the trailing paragraph, which is meant only for people who > write their script without using "--" by mistake, i.e. it only > belongs to point C. Hmph. Isn't that what I suggested in my first email? :P I am fine with the series you sent. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Probable bug in file run-command.c function clear_child_for_cleanup
Jeff King writes: > On Mon, Sep 10, 2012 at 01:00:35PM -0700, Junio C Hamano wrote: > >> >> And to conclude my bikeshedding for the day: Shouldn't "last" ideally >> >> be called something like "prev" instead? It's the previously visited >> >> element, not the last element in the list. >> > >> > It is the "last" element visited (just as "last week" is not the end of >> > the world), but yes, it is ambiguous, and "prev" is not. Either is fine >> > by me. >> >> OK, so who's gonna do the honors? > > I was hoping to give David a chance to submit his first-ever patch to > git. OK. David, is it OK for us to expect a patch from you sometime not in distant future (it is an old bug we survived for a long time and nothing ultra-urgent)? -- 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] Add "edit" action for interactive rebase?
On Mon, Sep 10, 2012 at 2:28 PM, Johannes Sixt wrote: > Did you think about what can go wrong? For example, starting with this > todo sheet: > > exec false > pick 1234567 Ah, that's definitely a problem. I was going to say we probably just to check the "done" file, same as the one we do for a fresh "rebase -i", but it turns out the "exec false" will fool the "has_action" check for a fresh "rebase -i" too. Heh. Maybe we should improve the check for a fresh "rebase -i" case, then we can do the same check for this case. Maybe we can grep for a "pick" in "done" file? Or we can check if there's anything in "rewritten"? Though I'm not sure if any of those is really foolproof. Or should we just ignore this case and assume the user knows what s/he's doing? Incidentally, if the starting todo file is: pick A exec false pick B If the user then changes the "pick B" to "squash B", it should be a valid I think, and "rebase -i" should handle that properly. It should, because that's the same thing as: pick C (which results in a conflict and stopped) squash D OT: That "exec false" ! I ran into numerous occasions where I wanted to manually do something before the "first commit after upstream", such as creating a new commit or merge. And I only had two ways of doing it: 1. to rebase against "upstream^", and then mark the upstream as edit 2. insert a "exec bash" in front of the "first commit" But "exec false" will work much much nicer. :) -- 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] daemon: restore getpeername(0,...) use
On Mon, Sep 10, 2012 at 08:27:07PM +0200, Joachim Schmitz wrote: > > I think it is a mistake to set -std=c89 (or whatever similar option your > > compiler supports). Like I said, we are not interested in being strictly > > C89-compliant. We are interested in working on real-world systems. > > > > If your compiler complains in the default mode (or when it is given some > > reasonable practical settings), then that's something worth fixing. But > > if your compiler is perfectly capable of compiling git, but you choose > > to cripple it by telling it to be pedantic about a standard, then that > > is not git's problem at all. > > Older version of HP NonStop only have a c89 compiler, newer have a > -Wc99lite switch to that, which enables some C99 features and the > latest additionally have a c99 compiler. There's no switch to cripple > something, it is just a fact that older systems don't have c99 or only > limited support for it. A whole series of machines (which is still in > use!) cannot get upgraded to anything better than c89. If you are using a compiler switch to emulate a real environment, then my comments above do not apply. I was speaking against standard pedantry for its own sake, which I have no interest in. However, do be careful that your emulated environment (i.e., recent NonStop but using compiler flags to pretend you are the older version) is accurate, and not introducing new portability annoyances that do not truly exist on the old system. In fact, I might go so far as to say if you cannot actually come up with an instance of the older platform to test it, it might not even be worth our time to care about. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] notes get-ref: --expand expands the output notes ref.
On Thu, Sep 6, 2012 at 9:47 PM, Junio C Hamano wrote: > "W. Trevor King" writes: >> On Wed, Sep 05, 2012 at 10:23:54PM -0700, Junio C Hamano wrote: >>> Really? Would "git log --expand master" be useful? >> >> I'm clearly not an expert on this, but isn't that what >> >> git show-ref master >> >> is for? > > But then can't you say the same against "git notes get-ref --expand" > with "git show-ref refs/remotes/origin/notes/foobla"? > > My primary point is that I think it is a UI mistake if the DWIM > ignores the user input that directly names that can be interpreted > without DWIMming. When the user wants to avoid ambiguity, it should > always be possible to spell it out without having to worry about the > DWIM code to get in the way. > > The problem with the current notes.c:expand_notes_ref() is that it > does not allow you to do that, and if you fixed that problem, the > user never has to ask "what does this expand to", no? > > Perhaps something like this. Note that this only deals with an > existing ref, and that is semi-deliberate (because I am not yet > convinced that it is a good idea to let any ref outside refs/notes/ > to be created to hold a notes tree). (sorry for the late reply; I was away this weekend) This patch looks like an acceptable solution to me. Especially since it prevents the notes code from "accidentally" creating new notes trees outside refs/notes/*. That said, we should check for more cases of hardcoded "refs/notes/" that potentially needs changing as well. ...Johan -- Johan Herland, www.herland.net -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] blame: respect "core.ignorecase"
Jeff King writes: > If the proposal were instead to add a certain type of pathspec that is > case-insensitive[2], that would make much more sense to me. It is not > violating git's case-sensitivity because it is purely a _query_ issue. > And it is a feature you might use whether or not your filesystem is case > sensitive. > ... > [2] I did not keep up with Duy's work on pathspec magic-prefixes (and I > could not find anything relevant in the code or documentation), but > it seems like this would be a logical feature to support there. I think it mostly is in setup.c (look for "Magic pathspec"). -- 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: checkout extra files
Jeff King writes: > Hmph. Isn't that what I suggested in my first email? :P Until I read the current text I did not realize the trailing paragraph was to apply only to point C (no "--" disambiguates and throws errors) but somehow thought it was covering both point B (with "--" you are strict) and C, and I didn't think of a good way to incorporate it into both. But yes, the final patch ended up to be exactly what you suggested to handle the issue ;-) 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/RFC] blame: respect "core.ignorecase"
On Mon, Sep 10, 2012 at 01:30:03PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > If the proposal were instead to add a certain type of pathspec that is > > case-insensitive[2], that would make much more sense to me. It is not > > violating git's case-sensitivity because it is purely a _query_ issue. > > And it is a feature you might use whether or not your filesystem is case > > sensitive. > > ... > > [2] I did not keep up with Duy's work on pathspec magic-prefixes (and I > > could not find anything relevant in the code or documentation), but > > it seems like this would be a logical feature to support there. > > I think it mostly is in setup.c (look for "Magic pathspec"). Thanks, that helped. I got excited when I saw the "icase" in the comments and thought it might already be implemented. But it looks like it is still to be done. :) -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 02/14] t5500: add tests of fetch-pack --all --depth=N $URL $REF
Michael Haggerty writes: > Document some bugs in "git fetch-pack": > > 1. If "git fetch-pack" is called with "--all", "--depth", and an > explicit existing non-tag reference to fetch, then it falsely reports > that the reference was not found, even though it was fetched > correctly. > > 2. If "git fetch-pack" is called with "--all", "--depth", and an > explicit existing tag reference to fetch, then it segfaults in > filter_refs() because return_refs is used without having been > initialized. I guess the first one is because "all" already marks the fetched one "used", and does not allow the explicit one to match any unused one from the other side? I wonder what happens when "--all" with an explicit refspec that names non-existing ref is asked for (it should notice that refs/heads/no-such-ref does not exist. I do not know if it is something that belongs to this set of new tests)? It is funny that this only happens with "--depth" (I think I know which part of the code introduces this bug, so it is not all that interesting, but just funny). Thanks for working on these glitches. > Signed-off-by: Michael Haggerty > --- > t/t5500-fetch-pack.sh | 15 +++ > 1 file changed, 15 insertions(+) > > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh > index 6fa1cef..15d277f 100755 > --- a/t/t5500-fetch-pack.sh > +++ b/t/t5500-fetch-pack.sh > @@ -427,4 +427,19 @@ test_expect_success 'test missing ref before existing' ' > test_cmp expect-error error-me > ' > > +test_expect_failure 'test --all, --depth, and explicit head' ' > + ( > + cd client && > + git fetch-pack --no-progress --all --depth=1 .. refs/heads/A > + ) >out-adh 2>error-adh > +' > + > +test_expect_failure 'test --all, --depth, and explicit tag' ' > + git tag OLDTAG refs/heads/B~5 && > + ( > + cd client && > + git fetch-pack --no-progress --all --depth=1 .. refs/tags/OLDTAG > + ) >out-adt 2>error-adt > +' > + > 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 v3 05/14] Change fetch_pack() and friends to take string_list arguments
Michael Haggerty writes:
> Instead of juggling (sometimes called
> ), pass around the list of references to be sought in
> a single string_list variable called "sought". Future commits will
> make more use of string_list functionality.
>
> Signed-off-by: Michael Haggerty
> ---
The earlier bikeshedding-fest on variable names seems to have
produced a winner ;-) I think "sought" captures what it is about
very well.
> diff --git a/fetch-pack.h b/fetch-pack.h
> index 1dbe90f..a6a8a73 100644
> --- a/fetch-pack.h
> +++ b/fetch-pack.h
> @@ -1,6 +1,8 @@
> #ifndef FETCH_PACK_H
> #define FETCH_PACK_H
>
> +#include "string-list.h"
> +
> struct fetch_pack_args {
> const char *uploadpack;
> int unpacklimit;
> @@ -21,8 +23,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
> int fd[], struct child_process *conn,
> const struct ref *ref,
> const char *dest,
> -int nr_heads,
> -char **heads,
> +struct string_list *sought,
> char **pack_lockfile);
This is a tangent, but I _think_ our header files ignore the dogma
some other projects follow that insists on each header file to be
self sufficient, i.e.
gcc fetch-pack.h
should pass. Instead, our *.c files that include fetch-pack.h are
responsible for including everything the headers they include need.
So even though fetch-pack.h does not include run-command.h, it
declares a function that takes "struct child_process *" in its
arguments. The new "struct string_list *" falls into the same camp.
Given that, I'd prefer to see the scope of this patch series shrunk
and have the caller include string-list.h, not here.
Updating the headers and sources so that each to be self sufficient
is a different topic, and I do not think there is a consensus yet if
we want to go that route.
--
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: approxidate parsing for bad time units
As you mentioned, parsing "n ... [month]", and even "...n..." (e.g. "the 3rd") as the nth day of a month is great, but in this case, I think "n ... ago" is a pretty strong sign that that's not the intended behavior. My first thought was just to make it an error if the string ends in "ago" but the date is parsed as a day of the month. You don't actually have to come up with any typos to blacklist, just keep the "ago" from being silently ignored. I suspect "n units ago" is by far the most common use of the approxidate parsing in the wild, since it's documented and has been popularized online. So throwing an error just in that case would save essentially everyone. I hadn't even realized it worked without "ago" until I looked at the code. If that doesn't sound like a good plan, then yes, I agree, it'd be tricky to catch it in the general case without breaking things. (Levenshtein distance to the target strings instead of exact matching, I guess, so that it could say "did you mean..." like for misspelled commands.) On Fri, Sep 7, 2012 at 6:54 AM, Jeff King wrote: > > On Thu, Sep 06, 2012 at 02:01:30PM -0700, Jeffrey Middleton wrote: > > > I'm generally very happy with the fuzzy parsing. It's a great feature > > that is designed to and in general does save users a lot of time and > > thought. In this case I don't think it does. The problems are: > > (1) It's not ignoring things it can't understand, it's silently > > interpreting them in a useless way. > > Right, but we would then need to come up with a list of things it _does_ > understand. So right now I can say "6 June" or "6th of June" or even "6 > de June", and it works because we just ignore the cruft in the middle. > > So I think you'd need to either whitelist what everybody is typing, or > blacklist some common typos (or convince people to be stricter in what > they type). > > > So I do think it's worth improving. (Yes, I know, send patches; I'll > > think about it.) > > You read my mind. :) > > -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rebase -i: Teach "--edit" action
On Mon, Sep 10, 2012 at 3:57 PM, Junio C Hamano wrote: > Hrm... They see the contents of the todo file immediately after > they say "rebase --edit-todo" and the sole reason they said that > command is because they wanted to edit the todo file. Is it likely > they need a reminder? Yes, it's not very likely, but sometimes the todo file takes a bit of time to finalize. So there's a good chance that the user can get interrupted, context switched, or went to do some double checking. And when the user returns to the editor, it's difficult to tell whether he's in a fresh rebase or a stopped rebase, unless he remembers. It's an unlikely scenario, but if it does happen, I think a short reminder could avoid some user panic. I don't plan to change how the todo file looks for a fresh rebase. I'll probably just add something like this for the stopped rebase case: # You are editing the todo of an ongoing rebase. To continue rebase after editing, run: "git rebase --continue" That will also remind the user to run "--continue" afterwards. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 09/14] filter_refs(): build refs list as we go
Michael Haggerty writes: > Instead of temporarily storing matched refs to temporary array > "return_refs", simply append them to newlist as we go. This changes > the order of references in newlist to strictly sorted if "--all" and > "--depth" and named references are all specified, but that usage is > broken anyway (see the last two tests in t5500). Removes two warts (the temporary array in general, and the fastarray[] special case) with one patch. Nicely 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
[PATCH v2 0/6] Add some string_list-related functions
Version 2 of a patch series that adds some functions to the string_list API. This patch series applies to current master. Thanks for Junio for lots of great feedback. The patch series "Clean up how fetch_pack() handles the heads list" v3, which requires some of the new string_list functionality, works unmodified on top of this version of the patch series. Changes since v1: * Expose a new function, string_list_append_nodup(). This is used in the implementation of string_list_split() and should be generally useful. * Straighten out the API for splitting strings (with help from Junio): * Implement two separate functions, one for splitting strings in place and a second for making copies while splitting a const string. * Redefine maxsplit=0 to mean "copy input string to output list as a single entry" for better consistency. * Add tests for more of the new functionality and simplify some of the other tests. * Various comment and documentation improvements. Michael Haggerty (6): string_list: add function string_list_append_nodup() string_list: add two new functions for splitting strings string_list: add a new function, filter_string_list() string_list: add a new function, string_list_remove_duplicates() string_list: add a function string_list_longest_prefix() api-string-list.txt: initialize the string_list the easy way .gitignore | 1 + Documentation/technical/api-string-list.txt | 67 +-- Makefile| 1 + string-list.c | 123 ++-- string-list.h | 71 t/t0063-string-list.sh | 121 +++ test-string-list.c | 123 7 files changed, 497 insertions(+), 10 deletions(-) create mode 100755 t/t0063-string-list.sh create mode 100644 test-string-list.c -- 1.7.11.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
[PATCH v2 1/6] string_list: add function string_list_append_nodup()
Add a new function that appends a string to a string_list without
copying it. This can be used to pass ownership of an already-copied
string to a string_list that has strdup_strings set.
Signed-off-by: Michael Haggerty
---
Documentation/technical/api-string-list.txt | 17 ++---
string-list.c | 20 +++-
string-list.h | 18 ++
3 files changed, 47 insertions(+), 8 deletions(-)
diff --git a/Documentation/technical/api-string-list.txt
b/Documentation/technical/api-string-list.txt
index 5a0c14f..113f841 100644
--- a/Documentation/technical/api-string-list.txt
+++ b/Documentation/technical/api-string-list.txt
@@ -20,8 +20,8 @@ If you need something advanced, you can manually malloc() the
`items`
member (you need this if you add things later) and you should set the
`nr` and `alloc` members in that case, too.
-. Adds new items to the list, using `string_list_append` or
- `string_list_insert`.
+. Adds new items to the list, using `string_list_append`,
+ `string_list_append_nodup`, or `string_list_insert`.
. Can check if a string is in the list using `string_list_has_string` or
`unsorted_string_list_has_string` and get it from the list using
@@ -100,7 +100,18 @@ write `string_list_insert(...)->util = ...;`.
`string_list_append`::
- Append a new string to the end of the string_list.
+ Append a new string to the end of the string_list. If
+ `strdup_string` is set, then the string argument is copied;
+ otherwise the new `string_list_entry` refers to the input
+ string.
+
+`string_list_append_nodup`::
+
+ Append a new string to the end of the string_list. The new
+ `string_list_entry` always refers to the input string, even if
+ `strdup_string` is set. This function can be used to hand
+ ownership of a malloc()ed string to a `string_list` that has
+ `strdup_string` set.
`sort_string_list`::
diff --git a/string-list.c b/string-list.c
index d9810ab..5594b7d 100644
--- a/string-list.c
+++ b/string-list.c
@@ -148,13 +148,23 @@ void print_string_list(const struct string_list *p, const
char *text)
printf("%s:%p\n", p->items[i].string, p->items[i].util);
}
-struct string_list_item *string_list_append(struct string_list *list, const
char *string)
+struct string_list_item *string_list_append_nodup(struct string_list *list,
+ char *string)
{
+ struct string_list_item *retval;
ALLOC_GROW(list->items, list->nr + 1, list->alloc);
- list->items[list->nr].string =
- list->strdup_strings ? xstrdup(string) : (char *)string;
- list->items[list->nr].util = NULL;
- return list->items + list->nr++;
+ retval = &list->items[list->nr++];
+ retval->string = (char *)string;
+ retval->util = NULL;
+ return retval;
+}
+
+struct string_list_item *string_list_append(struct string_list *list,
+ const char *string)
+{
+ return string_list_append_nodup(
+ list,
+ list->strdup_strings ? xstrdup(string) : (char
*)string);
}
static int cmp_items(const void *a, const void *b)
diff --git a/string-list.h b/string-list.h
index 0684cb7..1b3915b 100644
--- a/string-list.h
+++ b/string-list.h
@@ -29,6 +29,7 @@ int for_each_string_list(struct string_list *list,
#define for_each_string_list_item(item,list) \
for (item = (list)->items; item < (list)->items + (list)->nr; ++item)
+
/* Use these functions only on sorted lists: */
int string_list_has_string(const struct string_list *list, const char *string);
int string_list_find_insert_index(const struct string_list *list, const char
*string,
@@ -38,11 +39,28 @@ struct string_list_item *string_list_insert_at_index(struct
string_list *list,
int insert_at, const char
*string);
struct string_list_item *string_list_lookup(struct string_list *list, const
char *string);
+
/* Use these functions only on unsorted lists: */
+
+/*
+ * Add string to the end of list. If list->strdup_string is set, then
+ * string is copied; otherwise the new string_list_entry refers to the
+ * input string.
+ */
struct string_list_item *string_list_append(struct string_list *list, const
char *string);
+
+/*
+ * Like string_list_append(), except string is never copied. When
+ * list->strdup_strings is set, this function can be used to hand
+ * ownership of a malloc()ed string to list without making an extra
+ * copy.
+ */
+struct string_list_item *string_list_append_nodup(struct string_list *list,
char *string);
+
void sort_string_list(struct string_list *list);
int unsorted_string_list_has_string(struct string_list *list, const char
*string);
struct string_list_item *unsorted_string_list_lookup(struct string_list *list,
[PATCH v2 2/6] string_list: add two new functions for splitting strings
Add two new functions, string_list_split() and
string_list_split_in_place(). These split a string into a string_list
on a separator character. The first makes copies of the substrings
(leaving the input string untouched) and the second splits the
original string in place, overwriting the separator characters with
NULs and referring to the original string's memory.
These functions are similar to the strbuf_split_*() functions except
that they work with the more powerful string_list interface.
Signed-off-by: Michael Haggerty
---
.gitignore | 1 +
Documentation/technical/api-string-list.txt | 21 +-
Makefile| 1 +
string-list.c | 49 ++
string-list.h | 29 +
t/t0063-string-list.sh | 63 +
test-string-list.c | 45 +
7 files changed, 208 insertions(+), 1 deletion(-)
create mode 100755 t/t0063-string-list.sh
create mode 100644 test-string-list.c
diff --git a/.gitignore b/.gitignore
index bb5c91e..0ca7df8 100644
--- a/.gitignore
+++ b/.gitignore
@@ -193,6 +193,7 @@
/test-run-command
/test-sha1
/test-sigchain
+/test-string-list
/test-subprocess
/test-svn-fe
/common-cmds.h
diff --git a/Documentation/technical/api-string-list.txt
b/Documentation/technical/api-string-list.txt
index 113f841..670217c 100644
--- a/Documentation/technical/api-string-list.txt
+++ b/Documentation/technical/api-string-list.txt
@@ -21,7 +21,8 @@ member (you need this if you add things later) and you should
set the
`nr` and `alloc` members in that case, too.
. Adds new items to the list, using `string_list_append`,
- `string_list_append_nodup`, or `string_list_insert`.
+ `string_list_append_nodup`, `string_list_insert`,
+ `string_list_split`, and/or `string_list_split_in_place`.
. Can check if a string is in the list using `string_list_has_string` or
`unsorted_string_list_has_string` and get it from the list using
@@ -135,6 +136,24 @@ counterpart for sorted lists, which performs a binary
search.
is set. The third parameter controls if the `util` pointer of the
items should be freed or not.
+`string_list_split`, `string_list_split_in_place`::
+
+ Split a string into substrings on a delimiter character and
+ append the substrings to a `string_list`. If `maxsplit` is
+ non-negative, then split at most `maxsplit` times. Return the
+ number of substrings appended to the list.
++
+`string_list_split` requires a `string_list` that has `strdup_strings`
+set to true; it leaves the input string untouched and makes copies of
+the substrings in newly-allocated memory.
+`string_list_split_in_place` requires a `string_list` that has
+`strdup_strings` set to false; it splits the input string in place,
+overwriting the delimiter characters with NULs and creating new
+string_list_items that point into the original string (the original
+string must therefore not be modified or freed while the `string_list`
+is in use).
+
+
Data structures
---
diff --git a/Makefile b/Makefile
index 66e8216..ebbb381 100644
--- a/Makefile
+++ b/Makefile
@@ -501,6 +501,7 @@ TEST_PROGRAMS_NEED_X += test-run-command
TEST_PROGRAMS_NEED_X += test-scrap-cache-tree
TEST_PROGRAMS_NEED_X += test-sha1
TEST_PROGRAMS_NEED_X += test-sigchain
+TEST_PROGRAMS_NEED_X += test-string-list
TEST_PROGRAMS_NEED_X += test-subprocess
TEST_PROGRAMS_NEED_X += test-svn-fe
diff --git a/string-list.c b/string-list.c
index 5594b7d..f9051ec 100644
--- a/string-list.c
+++ b/string-list.c
@@ -204,3 +204,52 @@ void unsorted_string_list_delete_item(struct string_list
*list, int i, int free_
list->items[i] = list->items[list->nr-1];
list->nr--;
}
+
+int string_list_split(struct string_list *list, const char *string,
+ int delim, int maxsplit)
+{
+ int count = 0;
+ const char *p = string, *end;
+
+ assert(list->strdup_strings);
+ for (;;) {
+ count++;
+ if (maxsplit >= 0 && count > maxsplit) {
+ string_list_append(list, p);
+ return count;
+ }
+ end = strchr(p, delim);
+ if (end) {
+ string_list_append_nodup(list, xmemdupz(p, end - p));
+ p = end + 1;
+ } else {
+ string_list_append(list, p);
+ return count;
+ }
+ }
+}
+
+int string_list_split_in_place(struct string_list *list, char *string,
+ int delim, int maxsplit)
+{
+ int count = 0;
+ char *p = string, *end;
+
+ assert(!list->strdup_strings);
+ for (;;) {
+ count++;
+ if (maxsplit >= 0 && count > maxsplit) {
+ st
[PATCH v2 3/6] string_list: add a new function, filter_string_list()
This function allows entries that don't match a specified criterion to
be discarded from a string_list while preserving the order of the
remaining entries.
Signed-off-by: Michael Haggerty
---
Documentation/technical/api-string-list.txt | 11 +++
string-list.c | 17 ++
string-list.h | 9 ++
t/t0063-string-list.sh | 11 +++
test-string-list.c | 48 +
5 files changed, 96 insertions(+)
diff --git a/Documentation/technical/api-string-list.txt
b/Documentation/technical/api-string-list.txt
index 670217c..ea65818 100644
--- a/Documentation/technical/api-string-list.txt
+++ b/Documentation/technical/api-string-list.txt
@@ -33,6 +33,9 @@ member (you need this if you add things later) and you should
set the
. Can remove individual items of an unsorted list using
`unsorted_string_list_delete_item`.
+. Can remove items not matching a criterion from a sorted or unsorted
+ list using `filter_string_list`.
+
. Finally it should free the list using `string_list_clear`.
Example:
@@ -61,6 +64,14 @@ Functions
* General ones (works with sorted and unsorted lists as well)
+`filter_string_list`::
+
+ Apply a function to each item in a list, retaining only the
+ items for which the function returns true. If free_util is
+ true, call free() on the util members of any items that have
+ to be deleted. Preserve the order of the items that are
+ retained.
+
`print_string_list`::
Dump a string_list to stdout, useful mainly for debugging purposes. It
diff --git a/string-list.c b/string-list.c
index f9051ec..e0806fb 100644
--- a/string-list.c
+++ b/string-list.c
@@ -102,6 +102,23 @@ int for_each_string_list(struct string_list *list,
return ret;
}
+void filter_string_list(struct string_list *list, int free_util,
+ string_list_each_func_t want, void *cb_data)
+{
+ int src, dst = 0;
+ for (src = 0; src < list->nr; src++) {
+ if (want(&list->items[src], cb_data)) {
+ list->items[dst++] = list->items[src];
+ } else {
+ if (list->strdup_strings)
+ free(list->items[src].string);
+ if (free_util)
+ free(list->items[src].util);
+ }
+ }
+ list->nr = dst;
+}
+
void string_list_clear(struct string_list *list, int free_util)
{
if (list->items) {
diff --git a/string-list.h b/string-list.h
index dc5fbc8..7d18e62 100644
--- a/string-list.h
+++ b/string-list.h
@@ -29,6 +29,15 @@ int for_each_string_list(struct string_list *list,
#define for_each_string_list_item(item,list) \
for (item = (list)->items; item < (list)->items + (list)->nr; ++item)
+/*
+ * Apply want to each item in list, retaining only the ones for which
+ * the function returns true. If free_util is true, call free() on
+ * the util members of any items that have to be deleted. Preserve
+ * the order of the items that are retained.
+ */
+void filter_string_list(struct string_list *list, int free_util,
+ string_list_each_func_t want, void *cb_data);
+
/* Use these functions only on sorted lists: */
int string_list_has_string(const struct string_list *list, const char *string);
diff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh
index fb85430..a5f05cd 100755
--- a/t/t0063-string-list.sh
+++ b/t/t0063-string-list.sh
@@ -60,4 +60,15 @@ test_split ":" ":" "-1" items[i].string);
}
+void write_list_compact(const struct string_list *list)
+{
+ int i;
+ if (!list->nr)
+ printf("-\n");
+ else {
+ printf("%s", list->items[0].string);
+ for (i = 1; i < list->nr; i++)
+ printf(":%s", list->items[i].string);
+ printf("\n");
+ }
+}
+
+int prefix_cb(struct string_list_item *item, void *cb_data)
+{
+ const char *prefix = (const char *)cb_data;
+ return !prefixcmp(item->string, prefix);
+}
+
int main(int argc, char **argv)
{
if (argc == 5 && !strcmp(argv[1], "split")) {
@@ -39,6 +72,21 @@ int main(int argc, char **argv)
return 0;
}
+ if (argc == 4 && !strcmp(argv[1], "filter")) {
+ /*
+* Retain only the items that have the specified prefix.
+* Arguments: list|- prefix
+*/
+ struct string_li
[PATCH v2 4/6] string_list: add a new function, string_list_remove_duplicates()
Add a function that deletes duplicate entries from a sorted
string_list.
Signed-off-by: Michael Haggerty
---
Documentation/technical/api-string-list.txt | 9 +
string-list.c | 17 +
string-list.h | 7 +++
t/t0063-string-list.sh | 17 +
test-string-list.c | 10 ++
5 files changed, 60 insertions(+)
diff --git a/Documentation/technical/api-string-list.txt
b/Documentation/technical/api-string-list.txt
index ea65818..dd9aa3d 100644
--- a/Documentation/technical/api-string-list.txt
+++ b/Documentation/technical/api-string-list.txt
@@ -30,6 +30,9 @@ member (you need this if you add things later) and you should
set the
. Can sort an unsorted list using `sort_string_list`.
+. Can remove duplicate items from a sorted list using
+ `string_list_remove_duplicates`.
+
. Can remove individual items of an unsorted list using
`unsorted_string_list_delete_item`.
@@ -108,6 +111,12 @@ write `string_list_insert(...)->util = ...;`.
Look up a given string in the string_list, returning the containing
string_list_item. If the string is not found, NULL is returned.
+`string_list_remove_duplicates`::
+
+ Remove all but the first of consecutive entries that have the
+ same string value. If free_util is true, call free() on the
+ util members of any items that have to be deleted.
+
* Functions for unsorted lists only
`string_list_append`::
diff --git a/string-list.c b/string-list.c
index e0806fb..e9b7fd8 100644
--- a/string-list.c
+++ b/string-list.c
@@ -92,6 +92,23 @@ struct string_list_item *string_list_lookup(struct
string_list *list, const char
return list->items + i;
}
+void string_list_remove_duplicates(struct string_list *list, int free_util)
+{
+ if (list->nr > 1) {
+ int src, dst;
+ for (src = dst = 1; src < list->nr; src++) {
+ if (!strcmp(list->items[dst - 1].string,
list->items[src].string)) {
+ if (list->strdup_strings)
+ free(list->items[src].string);
+ if (free_util)
+ free(list->items[src].util);
+ } else
+ list->items[dst++] = list->items[src];
+ }
+ list->nr = dst;
+ }
+}
+
int for_each_string_list(struct string_list *list,
string_list_each_func_t fn, void *cb_data)
{
diff --git a/string-list.h b/string-list.h
index 7d18e62..3a6a6dc 100644
--- a/string-list.h
+++ b/string-list.h
@@ -48,6 +48,13 @@ struct string_list_item *string_list_insert_at_index(struct
string_list *list,
int insert_at, const char
*string);
struct string_list_item *string_list_lookup(struct string_list *list, const
char *string);
+/*
+ * Remove all but the first of consecutive entries with the same
+ * string value. If free_util is true, call free() on the util
+ * members of any items that have to be deleted.
+ */
+void string_list_remove_duplicates(struct string_list *sorted_list, int
free_util);
+
/* Use these functions only on unsorted lists: */
diff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh
index a5f05cd..dbfc05e 100755
--- a/t/t0063-string-list.sh
+++ b/t/t0063-string-list.sh
@@ -71,4 +71,21 @@ test_expect_success "test filter_string_list" '
test "x-" = "x$(test-string-list filter x1:x2 y)"
'
+test_expect_success "test remove_duplicates" '
+ test "x-" = "x$(test-string-list remove_duplicates -)" &&
+ test "x" = "x$(test-string-list remove_duplicates "")" &&
+ test a = "$(test-string-list remove_duplicates a)" &&
+ test a = "$(test-string-list remove_duplicates a:a)" &&
+ test a = "$(test-string-list remove_duplicates a:a:a:a:a)" &&
+ test a:b = "$(test-string-list remove_duplicates a:b)" &&
+ test a:b = "$(test-string-list remove_duplicates a:a:b)" &&
+ test a:b = "$(test-string-list remove_duplicates a:b:b)" &&
+ test a:b:c = "$(test-string-list remove_duplicates a:b:c)" &&
+ test a:b:c = "$(test-string-list remove_duplicates a:a:b:c)" &&
+ test a:b:c = "$(test-string-list remove_duplicates a:b:b:c)" &&
+ test a:b:c = "$(test-string-list remove_duplicates a:b:c:c)" &&
+ test a:b:c = "$(test-string-list remove_duplicates a:a:b:b:c:c)" &&
+ test a:b:c = "$(test-string-list remove_duplicates a:a:a:b:b:b:c:c:c)"
+'
+
test_done
diff --git a/test-string-list.c b/test-string-list.c
index 702276c..2d6eda7 100644
--- a/test-string-list.c
+++ b/test-string-list.c
@@ -87,6 +87,16 @@ int main(int argc, char **argv)
return 0;
}
+ if (argc == 3 && !strcmp(argv[1], "remove_duplicates")) {
+ struct string_list l
Re: approxidate parsing for bad time units
On Mon, Sep 10, 2012 at 02:07:02PM -0700, Jeffrey Middleton wrote:
> As you mentioned, parsing "n ... [month]", and even "...n..." (e.g.
> "the 3rd") as the nth day of a month is great, but in this case, I
> think "n ... ago" is a pretty strong sign that that's not the intended
> behavior.
Yeah, agreed. We are really talking about two distinct cases:
1. An absolute date ("the 3rd of June", "last tuesday") whose exact
location may need to be inferred from the context of the current
date.
2. A relative unit difference from the current time ("7 days ago")
However, I'm not sure that the word "ago" is always present when
choosing the latter. For example, you can say "7 days" and approxidate
will treat it like "7 days ago". Nor is it simply using a unit like
"days". You can even say "7 tuesdays" to go backwards that many Tuesdays
(e.g., the 24th of July from today).
So you can use "ago" as a sign that you are definitely in case (2), but
cannot assume that its absence means you are in case (1).
That means we can catch "3 dasy ago" as nonsensical, but not "3 dasy",
as the latter simply looks like "the 3rd" from approxidate's
perspective. Still, something is better than nothing, and it means if
you are careful to always say "ago", you can catch some errors (of
course, you might typo "ago"... :) ).
> My first thought was just to make it an error if the string ends in
> "ago" but the date is parsed as a day of the month. You don't actually
> have to come up with any typos to blacklist, just keep the "ago" from
> being silently ignored. I suspect "n units ago" is by far the most
> common use of the approxidate parsing in the wild, since it's
> documented and has been popularized online. So throwing an error just
> in that case would save essentially everyone. I hadn't even realized
> it worked without "ago" until I looked at the code.
Yeah, I think that would work, and would provide some safety. And it
shouldn't be too hard to implement.
> If that doesn't sound like a good plan, then yes, I agree, it'd be
> tricky to catch it in the general case without breaking things.
> (Levenshtein distance to the target strings instead of exact matching,
> I guess, so that it could say "did you mean..." like for misspelled
> commands.)
Gross. :)
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 5/6] string_list: add a function string_list_longest_prefix()
Add a function that finds the longest string from a string_list that
is a prefix of a given string.
Signed-off-by: Michael Haggerty
---
Documentation/technical/api-string-list.txt | 8
string-list.c | 20 +++
string-list.h | 8
t/t0063-string-list.sh | 30 +
test-string-list.c | 20 +++
5 files changed, 86 insertions(+)
diff --git a/Documentation/technical/api-string-list.txt
b/Documentation/technical/api-string-list.txt
index dd9aa3d..231a877 100644
--- a/Documentation/technical/api-string-list.txt
+++ b/Documentation/technical/api-string-list.txt
@@ -75,6 +75,14 @@ Functions
to be deleted. Preserve the order of the items that are
retained.
+`string_list_longest_prefix`::
+
+ Return the longest string within a string_list that is a
+ prefix (in the sense of prefixcmp()) of the specified string,
+ or NULL if no such prefix exists. This function does not
+ require the string_list to be sorted (it does a linear
+ search).
+
`print_string_list`::
Dump a string_list to stdout, useful mainly for debugging purposes. It
diff --git a/string-list.c b/string-list.c
index e9b7fd8..0b1f00a 100644
--- a/string-list.c
+++ b/string-list.c
@@ -136,6 +136,26 @@ void filter_string_list(struct string_list *list, int
free_util,
list->nr = dst;
}
+char *string_list_longest_prefix(const struct string_list *prefixes,
+const char *string)
+{
+ int i, max_len = -1;
+ char *retval = NULL;
+
+ for (i = 0; i < prefixes->nr; i++) {
+ char *prefix = prefixes->items[i].string;
+ if (!prefixcmp(string, prefix)) {
+ int len = strlen(prefix);
+ if (len > max_len) {
+ retval = prefix;
+ max_len = len;
+ }
+ }
+ }
+
+ return retval;
+}
+
void string_list_clear(struct string_list *list, int free_util)
{
if (list->items) {
diff --git a/string-list.h b/string-list.h
index 3a6a6dc..5efd07b 100644
--- a/string-list.h
+++ b/string-list.h
@@ -38,6 +38,14 @@ int for_each_string_list(struct string_list *list,
void filter_string_list(struct string_list *list, int free_util,
string_list_each_func_t want, void *cb_data);
+/*
+ * Return the longest string in prefixes that is a prefix (in the
+ * sense of prefixcmp()) of string, or NULL if no such prefix exists.
+ * This function does not require the string_list to be sorted (it
+ * does a linear search).
+ */
+char *string_list_longest_prefix(const struct string_list *prefixes, const
char *string);
+
/* Use these functions only on sorted lists: */
int string_list_has_string(const struct string_list *list, const char *string);
diff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh
index dbfc05e..41c8826 100755
--- a/t/t0063-string-list.sh
+++ b/t/t0063-string-list.sh
@@ -17,6 +17,14 @@ test_split () {
"
}
+test_longest_prefix () {
+ test "$(test-string-list longest_prefix "$1" "$2")" = "$3"
+}
+
+test_no_longest_prefix () {
+ test_must_fail test-string-list longest_prefix "$1" "$2"
+}
+
test_split "foo:bar:baz" ":" "-1" <|- */
+ struct string_list prefixes = STRING_LIST_INIT_DUP;
+ int retval;
+ const char *prefix_string = argv[2];
+ const char *string = argv[3];
+ const char *match;
+
+ parse_string_list(&prefixes, prefix_string);
+ match = string_list_longest_prefix(&prefixes, string);
+ if (match) {
+ printf("%s\n", match);
+ retval = 0;
+ }
+ else
+ retval = 1;
+ string_list_clear(&prefixes, 0);
+ return retval;
+ }
+
fprintf(stderr, "%s: unknown function name: %s\n", argv[0],
argv[1] ? argv[1] : "(there was none)");
return 1;
--
1.7.11.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
[PATCH v2 6/6] api-string-list.txt: initialize the string_list the easy way
In the demo code blurb, show how to initialize the string_list using STRING_LIST_INIT_NODUP rather than memset(). Signed-off-by: Michael Haggerty --- Documentation/technical/api-string-list.txt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt index 231a877..88330ff 100644 --- a/Documentation/technical/api-string-list.txt +++ b/Documentation/technical/api-string-list.txt @@ -44,10 +44,9 @@ member (you need this if you add things later) and you should set the Example: -struct string_list list; +struct string_list list = STRING_LIST_INIT_NODUP; int i; -memset(&list, 0, sizeof(struct string_list)); string_list_append(&list, "foo"); string_list_append(&list, "bar"); for (i = 0; i < list.nr; i++) -- 1.7.11.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 v3 00/14] Clean up how fetch_pack() handles the heads list
Michael Haggerty writes: > This patch series depends on the "Add some string_list-related > functions" series that I just submitted. > > This series is a significant rewrite of v2 based on the realization > that the pair that is passed around in these > functions is better expressed as a string_list. I hope you will find > that this version is shorter and clearer than its predecessors, even > though its basic logic is mostly the same. > > Sorry for the false starts in v1 and v2 and the extra reviewing work > that this will cost. Thanks for a pleasant read, nicely broken down into digestible pieces. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] completion: add --no-edit option for git-commit
Signed-off-by: Yacine Belkadi --- contrib/completion/git-completion.bash |3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 222b804..d1f905e 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -1014,7 +1014,8 @@ _git_commit () --*) __gitcomp " --all --author= --signoff --verify --no-verify - --edit --amend --include --only --interactive + --edit --no-edit + --amend --include --only --interactive --dry-run --reuse-message= --reedit-message= --reset-author --file= --message= --template= --cleanup= --untracked-files --untracked-files= -- 1.7.9.5 -- 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] blame: respect "core.ignorecase"
Jeff King writes: > On Mon, Sep 10, 2012 at 01:30:03PM -0700, Junio C Hamano wrote: > >> Jeff King writes: >> >> > If the proposal were instead to add a certain type of pathspec that is >> > case-insensitive[2], that would make much more sense to me. It is not >> > violating git's case-sensitivity because it is purely a _query_ issue. >> > And it is a feature you might use whether or not your filesystem is case >> > sensitive. >> > ... >> > [2] I did not keep up with Duy's work on pathspec magic-prefixes (and I >> > could not find anything relevant in the code or documentation), but >> > it seems like this would be a logical feature to support there. >> >> I think it mostly is in setup.c (look for "Magic pathspec"). > > Thanks, that helped. I got excited when I saw the "icase" in the > comments and thought it might already be implemented. But it looks like > it is still to be done. :) Yeah, some are tongue-in-cheek (e.g. I do not know what "recursive pathspec" even means), but "noglob" probably is an urgent need from correctness point of view for people who are writing Porcelain and want to interact with a history that records funny filenames. Currently you can "git 'foo\*'" to match a path that is exactly 'foo*' (because it matches) but you also have to hope there is no other paths that happens to match that pattern. A script that grabs paths out of ls-files output and then tries to use them as pathspec would want to have a way to say "This is literal. Do not honor globs in 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/RFC] blame: respect "core.ignorecase"
On Mon, Sep 10, 2012 at 02:38:08PM -0700, Junio C Hamano wrote: > > Thanks, that helped. I got excited when I saw the "icase" in the > > comments and thought it might already be implemented. But it looks like > > it is still to be done. :) > > Yeah, some are tongue-in-cheek (e.g. I do not know what "recursive > pathspec" even means), but "noglob" probably is an urgent need from > correctness point of view for people who are writing Porcelain and > want to interact with a history that records funny filenames. > Currently you can "git 'foo\*'" to match a path that is > exactly 'foo*' (because it matches) but you also have to hope there > is no other paths that happens to match that pattern. A script that > grabs paths out of ls-files output and then tries to use them as > pathspec would want to have a way to say "This is literal. Do not > honor globs in it". I agree that the automatic globbing is currently a problem (although one that comes up quite infrequently; I guess people use sane pathnames). But I would think for that particular use case, you would not want to do a per-glob prefix for that, but would rather use a command-line switch. IOW, would you rather do: git ls-files | while read fn; do echo ":(noglob)$fn" done | xargs git log --stdin -- or: git ls-files | xargs git log --stdin --no-glob-pathspec -- ? -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 02/14] t5500: add tests of fetch-pack --all --depth=N $URL $REF
On 09/10/2012 10:46 PM, Junio C Hamano wrote: > Michael Haggerty writes: > >> Document some bugs in "git fetch-pack": >> >> 1. If "git fetch-pack" is called with "--all", "--depth", and an >> explicit existing non-tag reference to fetch, then it falsely reports >> that the reference was not found, even though it was fetched >> correctly. >> >> 2. If "git fetch-pack" is called with "--all", "--depth", and an >> explicit existing tag reference to fetch, then it segfaults in >> filter_refs() because return_refs is used without having been >> initialized. > > I guess the first one is because "all" already marks the fetched one > "used", and does not allow the explicit one to match any unused one > from the other side? Yes, more or less. Spoiler: these failures are fixed later in the patch series :-) > I wonder what happens when "--all" with an > explicit refspec that names non-existing ref is asked for (it should > notice that refs/heads/no-such-ref does not exist. I do not know if > it is something that belongs to this set of new tests)? Good idea; I just wrote such tests. They appear to pass regardless of this patch series. I will submit them after I am sure that I understand them. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- 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 1/6] string_list: add function string_list_append_nodup()
Michael Haggerty writes:
> diff --git a/string-list.c b/string-list.c
> index d9810ab..5594b7d 100644
> --- a/string-list.c
> +++ b/string-list.c
> @@ -148,13 +148,23 @@ void print_string_list(const struct string_list *p,
> const char *text)
> printf("%s:%p\n", p->items[i].string, p->items[i].util);
> }
>
> -struct string_list_item *string_list_append(struct string_list *list, const
> char *string)
> +struct string_list_item *string_list_append_nodup(struct string_list *list,
> + char *string)
> {
> + struct string_list_item *retval;
> ALLOC_GROW(list->items, list->nr + 1, list->alloc);
> - list->items[list->nr].string =
> - list->strdup_strings ? xstrdup(string) : (char *)string;
> - list->items[list->nr].util = NULL;
> - return list->items + list->nr++;
> + retval = &list->items[list->nr++];
> + retval->string = (char *)string;
Do you still need this cast, now the function takes non-const "char *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: Using doxygen (or something similar) to generate API docs [was [PATCH 4/4] Add a function string_list_longest_prefix()]
On Mon, Sep 10, 2012 at 09:21:12PM +0200, Michael Haggerty wrote: > I'm renaming this thread so that the bikeshedding can get over ASAP. Thanks. :) > >>http://tomdoc.org/ > >> > >> Looks much nicer to me than most doxygen I've seen. But again, it's been > >> a while, so maybe doxygen is nicer than I remember. > > I don't have a personal preference for what system is used. I mentioned > doxygen only because it seems to be a well-known example. > > From a glance at the URL you mentioned, it looks like TomDoc is only > applicable to Ruby code. Yeah, sorry, I should have been more clear; tomdoc is not an option because it doesn't do C. But what I like about it is the more natural markup syntax. I was wondering if there were other similar solutions. Looks like "NaturalDocs" is one: http://www.naturaldocs.org/documenting.html On the other hand, doxygen is well-known among open source folks, which counts for something. And from what I've read, recent versions support Markdown, but I'm not sure of the details. So maybe it is a lot better than I remember. > > Doxygen has a the very nifty feature of being able to generate > > callgraphs though. We use it extensively at $dayjob, so if you need a > > hand building something sensible out of git's headers, I'd be happy > > to help. It has been over a decade since I seriously used doxygen for anything, and then it was a medium-sized project. So take my opinion with a grain of salt. But I remember the callgraph feature being one of those things that _sounded_ really cool, but in practice was not all that useful. > My plate is full. If you are able to work on this, it would be awesome. > As far as I'm concerned, you are the new literate documentation czar :-) Lucky me? :) I think I'll leave it for the moment, and next time I start to add some api-level documentation I'll take a look at doxygen-ating them and see how I like it. And I'd invite anyone else to do the same (in doxygen, or whatever system you like -- the best way to evaluate a tool like this is to see how your real work would look). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 00/14] Clean up how fetch_pack() handles the heads list
On 09/09/2012 08:15 PM, Junio C Hamano wrote: > Jeff King writes: > >> On Sun, Sep 09, 2012 at 03:20:18AM -0700, Junio C Hamano wrote: >> >>> Michael Haggerty writes: >>> This patch series depends on the "Add some string_list-related functions" series that I just submitted. >>> >>> Makes sense. The only worry (without reading the series first) I >>> have is that the use of string list may make the responsiblity of >>> sorting the list fuzzier. I am guessing that we never sorted the >>> refs we asked to fetch (so that FETCH_HEAD comes out in an expected >>> order), so use of unsorted string list would be perfectly fine. >> >> I haven't read the series yet, but both the list of heads from the user >> and the list of heads from the remote should have been sorted by 4435968 >> and 9e8e704f, respectively. > > OK. As long as the sort order matches the order string-list > internally uses for its bisection search, it won't be a problem, > then. The sorting is crucial but there is no bisection involved. The sorted linked-list of references available from the remote and the sorted string_list of requested references are iterated through in parallel. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- 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 2/6] string_list: add two new functions for splitting strings
Michael Haggerty writes:
> diff --git a/string-list.c b/string-list.c
> index 5594b7d..f9051ec 100644
> --- a/string-list.c
> +++ b/string-list.c
> @@ -204,3 +204,52 @@ void unsorted_string_list_delete_item(struct string_list
> *list, int i, int free_
> list->items[i] = list->items[list->nr-1];
> list->nr--;
> }
> +
> +int string_list_split(struct string_list *list, const char *string,
> + int delim, int maxsplit)
> +{
> + int count = 0;
> + const char *p = string, *end;
> +
> + assert(list->strdup_strings);
This may be a taste thing, but I'd prefer to see assert() only for
verification of pre-condition by internal callers to catch stupid
programming errors. For a library-ish function like sl_split() that
expects to be called from anybody outside the string-list API, a
violation of this pre-condition is a usage error of the API, and
should trigger a runtime error (even without NDEBUG), no?
--
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 1/6] string_list: add function string_list_append_nodup()
On 09/10/2012 11:56 PM, Junio C Hamano wrote:
> Michael Haggerty writes:
>
>> diff --git a/string-list.c b/string-list.c
>> index d9810ab..5594b7d 100644
>> --- a/string-list.c
>> +++ b/string-list.c
>> @@ -148,13 +148,23 @@ void print_string_list(const struct string_list *p,
>> const char *text)
>> printf("%s:%p\n", p->items[i].string, p->items[i].util);
>> }
>>
>> -struct string_list_item *string_list_append(struct string_list *list, const
>> char *string)
>> +struct string_list_item *string_list_append_nodup(struct string_list *list,
>> + char *string)
>> {
>> +struct string_list_item *retval;
>> ALLOC_GROW(list->items, list->nr + 1, list->alloc);
>> -list->items[list->nr].string =
>> -list->strdup_strings ? xstrdup(string) : (char *)string;
>> -list->items[list->nr].util = NULL;
>> -return list->items + list->nr++;
>> +retval = &list->items[list->nr++];
>> +retval->string = (char *)string;
>
> Do you still need this cast, now the function takes non-const "char *string"?
Good catch. Do you want to fix this in your repo or should I re-roll?
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
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: Using doxygen (or something similar) to generate API docs [was [PATCH 4/4] Add a function string_list_longest_prefix()]
On 09/10/2012 11:56 PM, Jeff King wrote: > On Mon, Sep 10, 2012 at 09:21:12PM +0200, Michael Haggerty wrote: >> My plate is full. If you are able to work on this, it would be awesome. >> As far as I'm concerned, you are the new literate documentation czar :-) > > Lucky me? :) I was nominating Andreas, who rashly volunteered to help. But don't feel left out; there's enough work to go around :-) > I think I'll leave it for the moment, and next time I start to add some > api-level documentation I'll take a look at doxygen-ating them and see > how I like it. And I'd invite anyone else to do the same (in doxygen, or > whatever system you like -- the best way to evaluate a tool like this is > to see how your real work would look). I agree with that. A very good start would be to mark up a single API and build the docs (by hand if need be) using a proposed tool. This will let people get a feel for (1) what the markup has to look like and (2) what they get out of it. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 00/14] Clean up how fetch_pack() handles the heads list
Michael Haggerty writes: >> OK. As long as the sort order matches the order string-list >> internally uses for its bisection search, it won't be a problem, >> then. > > The sorting is crucial but there is no bisection involved. The sorted > linked-list of references available from the remote and the sorted > string_list of requested references are iterated through in parallel. What I meant was that the order used by string-list is pretty much internal to string-list implementation for its "quickly locate where to insert" bisection. It happens to be the byte value order, I think, but the point is whatever order it is, that has to match the order we keep references in the other data source you walk in parallel to match (i.e. the linked list of references). -- 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 2/6] string_list: add two new functions for splitting strings
Michael Haggerty writes: > +`string_list_split`, `string_list_split_in_place`:: > + > + Split a string into substrings on a delimiter character and > + append the substrings to a `string_list`. If `maxsplit` is > + non-negative, then split at most `maxsplit` times. Return the > + number of substrings appended to the list. I recall that we favor `A`:: `B`:: Description for A and B for some reason but do not remember exactly why. -- 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] blame: respect "core.ignorecase"
Jeff King writes: > But I would think for that particular use case, you would not want to do > a per-glob prefix for that, but would rather use a command-line switch. Yes. Even though it is not listed as possible future semantics, one thing that we may want to have before all others is ":(negate)", so that we can say something like: git log -- '*.c' ':(negate)compat/" I do not offhand recall what we decided during the last discussion on the topic, but IIRC, the concensus was that it will make the code too complex and general case unusably slow to support 'or', 'and', and 'not' operators between pathspec elements (e.g. "*.c" or "*.h" files but not in "compat/" directory, unless ...) in a general way, and it would be sufficient to support negation by: - first grouping pathspec elements into two bins (i.e. normal patterns and negative patterns); - doing the usual path limiting using only the normal patterns; - among the paths that match one of the normal patterns, rejecting the ones that match one of the negative patterns. or something like that. -- 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: Using doxygen (or something similar) to generate API docs [was [PATCH 4/4] Add a function string_list_longest_prefix()]
On 09/10/2012 11:56 PM, Jeff King wrote: > On Mon, Sep 10, 2012 at 09:21:12PM +0200, Michael Haggerty wrote: > >> I'm renaming this thread so that the bikeshedding can get over ASAP. > > Thanks. :) > http://tomdoc.org/ Looks much nicer to me than most doxygen I've seen. But again, it's been a while, so maybe doxygen is nicer than I remember. >> >> I don't have a personal preference for what system is used. I mentioned >> doxygen only because it seems to be a well-known example. >> >> From a glance at the URL you mentioned, it looks like TomDoc is only >> applicable to Ruby code. > > Yeah, sorry, I should have been more clear; tomdoc is not an option > because it doesn't do C. But what I like about it is the more > natural markup syntax. I was wondering if there were other similar > solutions. Looks like "NaturalDocs" is one: > >http://www.naturaldocs.org/documenting.html > > On the other hand, doxygen is well-known among open source folks, which > counts for something. And from what I've read, recent versions support > Markdown, but I'm not sure of the details. So maybe it is a lot better > than I remember. > Markdown is supported, yes. There aren't really any details to it. I don't particularly like markdown, but my colleagues tend to use it for howto's and whatnot and it can be mixed with other doxygen styles without problem. >>> Doxygen has a the very nifty feature of being able to generate >>> callgraphs though. We use it extensively at $dayjob, so if you need a >>> hand building something sensible out of git's headers, I'd be happy >>> to help. > > It has been over a decade since I seriously used doxygen for anything, > and then it was a medium-sized project. So take my opinion with a grain > of salt. But I remember the callgraph feature being one of those things > that _sounded_ really cool, but in practice was not all that useful. > It's like all tools; Once you're used to it, it's immensely useful. I tend to prefer using it to find either code in dire need of refactoring (where the graph is too large), or engines and exit points. For those purposes, it's pretty hard to beat a good callgraph. >> My plate is full. If you are able to work on this, it would be awesome. >> As far as I'm concerned, you are the new literate documentation czar :-) > > Lucky me? :) > I think he was talking to me, but since you seem to have volunteered... ;) > I think I'll leave it for the moment, and next time I start to add some > api-level documentation I'll take a look at doxygen-ating them and see > how I like it. And I'd invite anyone else to do the same (in doxygen, or > whatever system you like -- the best way to evaluate a tool like this is > to see how your real work would look). > That's one of the problems. People follow what's already there, and there are no comments there now so there won't be any added in the future :-/ -- Andreas Ericsson andreas.erics...@op5.se OP5 AB www.op5.se Tel: +46 8-230225 Fax: +46 8-230231 Considering the successes of the wars on alcohol, poverty, drugs and terror, I think we should give some serious thought to declaring war on peace. -- 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
Interactive rebase with pre-built script?
Hi! At $DAYJOB, we have a lot of code flowing from a central repository to repositories which hold refinitions and ports of the code from the central repository. Often enough the people working on the porting repositories find bugs in the code from the central repository, and want to submit patches upstream. We want to get these patches upstream in the easiest possible manner, and a clever colleague of mine came up with this recipe, to be run from the downstream repository: git log --reverse --format="pick %h %s" master.. -- common_paths > changes.txt This gives a list of the commits changing the code in the common paths (we try to make sure to make them in separate changesets, not touching the downstream code), in a format that can be used as input to git rebase --interactive. Now, to my question. Is there an easy way to run interactive rebase on the upstream branch with this recipe? The best we have come up with so far is git checkout master # the upstream branch git rebase -i HEAD~ and then just append everything from the generated recipe. This doesn't play well if the last commit is a merge, though (making a dummy commit and just removing it from the default rebase recipe works for that case, though). I was thinking about using git cherry-pick with a list of commits, rebase is better at helping with conflicts and such. -- \\// Peter - http://www.softwolves.pp.se/ -- 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
