date:20160323

Jeff King  writes:

> On Wed, Mar 23, 2016 at 04:23:41PM -0700, Junio C Hamano wrote:
>
>> So here is the third try (previous round is found at $gmane/289166
>> and the very first one is at $gmane/288987).
>
> Is the plan to merge these as-is? The ordering is a bit funny (introduce
> breakage, then repair it), and I think the first patch still breaks
> t4201.8 (which is then repaired in the fourth one).

I do not have a firm plan yet.  This was one of those "during the
pre-release freeze, instead of reviewing shiny new toys by others
too early, spend leftover time to tie loose ends" attempts ;-)

I'd agree that the "final" version should do our usual "progressive
improvement, never stepping back one and then forward two", but I
wanted to see what the endgame state would look like first, and by
doing the incremental "the first one gets it 80% right, and fix it
up with follow-up patches" I didn't have to worry about at what
point I need to take the authorship of which patch.

--
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/GSoC 3/3] Nousage message in error

This is my first review. It can contain some mistakes.

On Thu, Mar 24, 2016 at 7:33 AM, Diwas Joshi  wrote:
> Subject : [PATCH/GSoC 3/3] Nousage message in error

Mention about GSoC in the notes section (the one followed by the 3
dashes ie. "---") rather than in the subject.

> - To show only error text instead of full usage message
> - Adds exits to callback function in parse-options-cb.c instead of returning 
> -1 which results in display of usage message.

A general convention followed by git users it to write the commit
message as "What he did to the code?" rather than "What problem was
there in the code?" And of course after writing what you did to the
code, you can definitely mention what problem in the code made you do
this change.

>  parse-options-cb.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/parse-options-cb.c b/parse-options-cb.c
> index 239898d..b7321d1 100644
> --- a/parse-options-cb.c
> +++ b/parse-options-cb.c
> @@ -85,8 +85,10 @@ int parse_opt_commits(const struct option *opt, const char 
> *arg, int unset)
>
> if (!arg)
> return -1;
> -   if (get_sha1(arg, sha1))
> -   return error("malformed object name %s", arg);
> +   if (get_sha1(arg, sha1)) {
> +   error("malformed object name %s", arg);
> +   exit(129);
> +   }
> commit = lookup_commit_reference(sha1);
> if (!commit)
> return error("no such commit %s", arg);

Maybe you could describe a little more on why this change is required?
Why would the user want to know "How to use the command?" when the
actual problem is that SHA-1 checksum has been compromised? And I
don't see any consumers of this method which *directly* interact with
the UI.

It seems that PATCH 1/3 and PATCH 2/3 are missing.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] format-patch: introduce --base=auto option

2016-03-23 Thread Ye Xiaolong

On Wed, Mar 23, 2016 at 11:25:41AM -0700, Junio C Hamano wrote:
>Xiaolong Ye  writes:
>
>> +
>> +diff_setup();
>> +DIFF_OPT_SET(, RECURSIVE);
>> +diff_setup_done();
>
>It is annoying that you moved "diff" stuff here (if it can be
>initialized once at the beginning and then reused over and over,
>it should have been done here from the beginning at PATCH 2/4).

My bad, I will put "diff" stuff at the beginning at PATCH 2/4.
>
>> +if (!strcmp(base_commit, "auto")) {
>> +curr_branch = branch_get(NULL);
>> +upstream = branch_get_upstream(curr_branch, NULL);
>> +if (upstream) {
>> +if (get_sha1(upstream, sha1))
>> +die(_("Failed to resolve '%s' as a valid 
>> ref."), upstream);
>> +base = lookup_commit_or_die(sha1, "upstream base");
>> +oidcpy(>base_commit, >object.oid);
>> +} else {
>> +commit_patch_id(prerequisite_head, , sha1);
>> +oidcpy(>parent_commit, 
>> _head->object.oid);
>> +hashcpy(bases->parent_patch_id.hash, sha1);
>> +return;
>
>What happens if you did this sequence?
>
>   $ git fetch origin
>$ git checkout -b fork origin/master
>$ git fetch origin
>$ git format-patch --base=auto origin..
>
>You grab the updated origin/master as base and use it here, no?
>At that point the topology would look like:
>
>  1---2---3 updated upstream
> /
>   0---X---Y---Z---A---B---C
>^
>old upstream
>
>so you are basing your worn on "0" (old upstream) but setting base
>to "3"
>
>Wouldn't that trigger "base must be an ancestor of Z" check you had
>in [PATCH 2/4]?

Yes, this is flawed, I will follow your below suggestion to compute
the merge base as the base commit through upstream and specified range.

>
>I also do not see the point of showing "parent id" which as far as I
>can see is just a random commit object name and show different
>output that is not even described what it is.  It would be better to

Here is our consideration:
There is high chance that branch_get_upstream will retrun NULL(thus we
are not able to get exact base commit), since developers may checkout
branch from a local branch or a commit and haven't set "--set-upstream-to"
to track a remote branch, in this case, we want to provide likely useful
info(here is parent commit id and patch id), based on it, 0day robot still
have good chance to find the suitable base.
Otherwise, I'm afraid this annotation system won't work effectively in long run.

Thanks,
Xiaolong.
>
> * find the upstream (i.e. 3 in the picture) and then with our range
>   (i.e. A B and C) compute the merge base (i.e. you would find 0)
>   and use it as base;
>
> * if there is no upstream, error out and tell the user that there
>   is no upstream.  The user is intelligent enough and knows what
>   commit the base should be.
>
>I suspect, but I didn't think things through.
>
>
>--
>To unsubscribe from this list: send the line "unsubscribe git" in
>the body of a message to majord...@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] format-patch: add '--base' option to record base tree info

2016-03-23 Thread Ye Xiaolong

On Wed, Mar 23, 2016 at 11:08:20AM -0700, Junio C Hamano wrote:
>Xiaolong Ye  writes:
>
>Reviewing the patch out of order, caller first and then callee.
>
>> +static void print_bases(struct base_tree_info *bases)
>> +{
>> +int i;
>> +
>> +/* Only do this once, either for the cover or for the first one */
>> +if (is_null_oid(>base_commit))
>> +return;
>> +
>> +printf("** base-commit-info **\n");
>
>I am not sure if you want to have this line (an empty line might not
>hurt), as the "base-commit: ..." that comes next is a clear enough
>indication of what these lines are.

Ok, I will remove this extra line.
>
>> +if (base_commit) {
>> +struct commit *prerequisite_head = NULL;
>> +if (list[nr - 1]->parents)
>> +prerequisite_head = list[nr - 1]->parents->item;
>> +memset(, 0, sizeof(bases));
>> +reset_revision_walk();
>> +prepare_bases(, base_commit, prerequisite_head);
>> +}
>> +
>
>list[] holds the commits in reverse topological order, so the first
>parent of the last element in the list[] would hopefully give you
>the latest change your series depends on, and that is why you are
>working on list[nr - 1] here.

Yes, I just considered linear topology before.
>
>I however think that is flawed.
>
>What if your series A, B and C are on this topology?
>
>---P---X---A---M---C
>\ /
> Y---Z---B
>
>"git format-patch --base=P -3 C" would show A, B and C.  It may show
>B, A and C, as A and B are independent (you would be flattening the
>history into the shape you have in your documentation part of the
>patch in order to adjust for their interactions before running
>format-patch if that were not the case).  Depending on which one
>happens to be chosen as prerequisite_head, you would miss either X
>or Y & Z, wouldn't you?
>
>A simpler and safer (but arguably less useful) approach may be to
>use the logic and limitation of your patch as-is but add code to
>check that the history is linear and refuse to do the "base" thing.
>But just in case you want to handle a more general case like the
>above, read on.
>
>> +static void prepare_bases(struct base_tree_info *bases,
>> +  const char *base_commit,
>> +  struct commit *prerequisite_head)
>> +{
>> +struct commit *base = NULL, *commit;
>> +struct rev_info revs;
>> +struct diff_options diffopt;
>> +struct object_id *patch_id;
>> +unsigned char sha1[20];
>> +int pos = 0;
>> +
>> +if (!prerequisite_head)
>> +return;
>> +base = lookup_commit_reference_by_name(base_commit);
>> +if (!base)
>> +die(_("Unknown commit %s"), base_commit);
>> +oidcpy(>base_commit, >object.oid);
>> +
>> +if (base == prerequisite_head)
>> +return;
>> +
>> +if (!in_merge_bases(base, prerequisite_head))
>> +die(_("base commit should be the ancestor of revs you 
>> specified"));
>> +
>> +init_revisions(, NULL);
>> +revs.max_parents = 1;
>> +
>> +base->object.flags |= UNINTERESTING;
>> +add_pending_object(, >object, "base");
>> +prerequisite_head->object.flags |= 0;
>> +add_pending_object(, _head->object, 
>> "prerequisite-head");
>> +
>> +diff_setup();
>> +DIFF_OPT_SET(, RECURSIVE);
>> +diff_setup_done();
>> +
>> +if (prepare_revision_walk())
>> +die(_("revision walk setup failed"));
>> +/*
>> + * Traverse the commits list between base and prerequisite head,
>> + * get the patch ids and stuff them in bases structure.
>> + */
>> +while ((commit = get_revision()) != NULL) {
>> +if (commit_patch_id(commit, , sha1))
>> +return;
>> +ALLOC_GROW(bases->patch_id, bases->nr_patch_id + 1, 
>> bases->alloc_patch_id);
>> +patch_id = bases->patch_id + pos;
>> +hashcpy(patch_id->hash, sha1);
>> +pos++;
>> +bases->nr_patch_id++;
>
>Micronit.  Aren't pos and nr_patch_id always the same?
Sorry, will only use nr_patch_id.
>
>> +}
>> +}
>
>I think the right thing to do is probably to start another revision
>walk (which you do) but setting the starting points of the traversal
>to all elements in the list[] (which you don't--you use either A^ or
>B^).  And skip the ones in the list[] before grabbing its patch-id
>in the loop.  That way, you do not have to worry about the topology
>of the history tripping you up at all.
>
>So I'd suggest to redo this function perhaps like so, if you do want
>to handle the more general case:

Thanks for the elaborated suggestions. I will redo the prepare_bases
function accordingly to handle more general case.
>
>static void prepare_bases(struct base_tree_info *bases,
> const char *base_commit,
> struct commit **list,
>  int total)
>{
>   ... boilerplate ...
>
>

Gitk "External diff" broken when using worktree

2016-03-23 Thread Daryl Van Den Brink

Hi,

I'm using git 2.7.3 on cygwin, and have been taking advantage of the
new "git worktree" feature. I noticed that when I launch gitk from one
of the attached working directories, its "external diff" feature
doesn't seem to work. Nothing shows up in the diff tool at all.
However, it works if you launch gitk from the main repository.

To reproduce:
1. Create a new working tree with "git worktree add"
2. From that new worktree, launch gitk.
3. Right-click in a file in the bottom right pane and click "External diff"
4. No useful diff appears.

-- 
Daryl van den Brink
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

"git svn fetch" error: [main] perl 9296 child_info_fork::abort: unable to map

2016-03-23 Thread Fu, Siyuan

Hi, all

I meet below error when using the "git svn" related command, and I tried to 
reinstall the GIT but the issue is still there. Has anybody ever meet this and 
know how to solve it? 

GIT version: git version 2.7.4.windows.1
OS: Windows 8 64bit

$ git svn fetch
  4 [main] perl 9296 child_info_fork::abort: unable to map C:\Program 
Files\Git\usr\bin\msys-svn_subr-1-0.dll, Win32 error 1114
  4 [main] perl 4480 child_info_fork::abort: unable to map C:\Program 
Files\Git\usr\bin\msys-svn_subr-1-0.dll, Win32 error 1114
 12 [main] perl 8572 child_info_fork::abort: unable to map C:\Program 
Files\Git\usr\bin\msys-svn_subr-1-0.dll, Win32 error 1114


Best Regards
Siyuan
--
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/GSoC 3/3] Nousage message in error

2016-03-23 Thread Diwas Joshi

- To show only error text instead of full usage message
- Adds exits to callback function in parse-options-cb.c instead of returning -1 
which results in display of usage message.
---
 parse-options-cb.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/parse-options-cb.c b/parse-options-cb.c
index 239898d..b7321d1 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -85,8 +85,10 @@ int parse_opt_commits(const struct option *opt, const char 
*arg, int unset)
 
if (!arg)
return -1;
-   if (get_sha1(arg, sha1))
-   return error("malformed object name %s", arg);
+   if (get_sha1(arg, sha1)) {
+   error("malformed object name %s", arg);
+   exit(129);
+   }
commit = lookup_commit_reference(sha1);
if (!commit)
return error("no such commit %s", arg);
-- 
2.7.4

--
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 0/5] Expanding tabs in "git log" output

On Wed, Mar 23, 2016 at 04:23:41PM -0700, Junio C Hamano wrote:

> So here is the third try (previous round is found at $gmane/289166
> and the very first one is at $gmane/288987).

Is the plan to merge these as-is? The ordering is a bit funny (introduce
breakage, then repair it), and I think the first patch still breaks
t4201.8 (which is then repaired in the fourth one).

I think it would be a lot easier to review as:

  1. Factor out pp_handle_indent(), and any other preparation.

  2. Add --expand-tabs / --no-expand-tabs, with the logic going into
 pp_handle_indent().

  3. Flip the default for some formats to expand-tabs.

Other than that, the end result seems OK to me (I think adding
--expand-tabs would be nice, but I suspect it may need to be marked as
incompatible with some formats; do all formats end up in this same
writing code path?).

> By the way, I have to say that I hate how pretty formatting and
> revision machinery interact with each other.
> 
> pretty.c::commit_formats ought to be the authoritative source of how
> each named format should work, but there are quite a many codepaths
> that just assign CMIT_FMT_SOMETHING to revs->commit_format without
> bothering with other fields in the cmt_fmt_map like is_tformat, and
> I am not sure if they are working correctly even before this patch.

I don't disagree with any of that. I suspect some of the logic may be
complicated for sticking in a table, though. Perhaps we need a:

  void set_pp_format(struct pretty_print_context *ctx, enum cmit_fmt fmt);

that sets up the whole struct based on the given format, and then the
logic can live in C code. I haven't looked closely at that code in a
while, though, so maybe that is overkill.

-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: [RFC_PATCHv4 5/7] submodule update: respect submodule.actionOnLabel

Stefan Beller  writes:

> On Tue, Mar 22, 2016 at 3:40 PM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>
>>> This change introduces the 'submodule.actionOnLabel' variable
>>> in a repository configuration. Generally speaking 'submodule.actionOnLabel'
>>> restricts the action of a command when no submodules are selected via the
>>> command line explicitely to those submodules, which are selected by
>>> 'submodule.actionOnLabel'. It can occur multiple times and can specify
>>> the path, the name or one of the labels of a submodule to select that
>>> submodule.
>>>
>>> The introduction of 'submodule.actionOnLabel' starts with
>>> 'git submodule update' in this patch and other commands will follow
>>> in later patches.
>>>
>>> 'submodule.actionOnLabel' implies '--init' in 'git submodule update'.
>>>
>>> Signed-off-by: Stefan Beller 
>>>
>>> TODO: generic documentation for submodule.actionOnLabel
>>> TODO: documentation for submodule update
>>
>> TODO: a name that matches the concept better.
>
> This is one of the hardest parts of the series so far. The last reviews
> were mostly bike shedding about the name of the concept and I thought
> we were settled to actionOnLabel as that fits best to what we want to do.
>
> So let's revisit that. My current understanding of the design:

I am not questioning the name "label" to call the facility that
allows projects to group submodules together, and that serves as one
of the ways to choose what subset of submodules are worked on by
default.  There is no need to revisit that part.

What I am questioning is

action On Label

because

 (1) it sounds as if that configuration were a way to choose what
 action is done to the chosen subset of submodules;

 (2) it sounds as if the only way to choose a subset of submodules
 to be operated on by default is via the "label" mechanism.

And from your writing (omitted), I think we agree that we definitely
want to avoid the misunderstanding that is (1).  This variable does
not specify what is done--this specifies what subset of submodules
are to be operated on.  Having "action" in the name of the variable
is wrong.

And from the proposed log message, it is clear that "label" is not
the only way to specify the subset of submodules to be worked on,
i.e. "... can specify the path, name or the labels".   Having
"label" in the variable name is wrong.

I am tempted to suggest submodule.defaultOperand and I am fairly
sure "default" part of that name gets the concept much better than
"actionOnLabel", but there probably are much better words than
Operand.
--
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 0/5] Expanding tabs in "git log" output

2016-03-23 Thread Linus Torvalds

On Wed, Mar 23, 2016 at 4:23 PM, Junio C Hamano  wrote:
> So here is the third try (previous round is found at $gmane/289166
> and the very first one is at $gmane/288987).
>
> The first three patches are essentially the same as v2.  The last
> two updates how the tab-expansion is internally controlled:

I tested this (as it in in 'pu', rather than applying the patches),
and it all seems to work fine. So Ack.

And I agree that it would be good if all the commit printout logic was
unified rather than having some ad-hoc "let's just set the format",
but I think that's a separate cleanup.

It might be more regular to have that "--expand-tabs" flag too (which
would then work with the email and raw formats), but I don't see any
actual real use for it so it really doesn't matter.

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

GSoC 2016 | Proposal | Incremental Rewrite of git bisect

Hey!

I have prepared a proposal for Google Summer of Code 2016. I know this
is a bit late, but please try to give your comments and suggestions.
My proposal could greatly improve from this. Some questions:

1. Should I include more ways in which it can help windows?
2. Should I include the function names I intend to convert?
3. Is my timeline (a bit different) going to affect me in any way?

Here is a Google doc for my proposal.
https://docs.google.com/document/d/1stnDPA5Hs3u0a8sqoWZicTFpCz1wHP9bkifcKY13Ocw/edit?usp=sharing

For the people who prefer the text only version :

---

Incremental rewrite of Git bisect

About Me

Basic Information

Name Pranit Bauva

University IIT Kharagpur

MajorMining Engineering

Emailpranit.ba...@gmail.com

IRC pungi-man

Blog http://bauva.in

Timezone IST (UTC +5:30)

Background

I am a first year undergraduate in the department of Mining
Engineering at Indian Institute of Technology, Kharagpur. I am an open
source enthusiast. I am a part of Kharagpur Linux Users Group which is
basically a group of open-source enthusiasts. I am quite familiar with
C and I have been using shell for some time now and still find new
things about it everyday. I have used SVN when I was on Windows and
then I switched to Git when I moved to linux. Git seems like magic. I
always wanted to involve in the development process and Google Summer
of Code is an a awesome way to achieve it.

Abstract

Git bisect is a frequently used command which helps the developers in
finding the commit which introduced the bug. Some part of it is
written in shell script. I intend to convert it to low level C code
thus making them builtins. This will increase Git’s portability.
Efficiency of git bisect will definitely increase but it would not
really matter much as most of the time is consumed in compiling or
testing when in bisection mode but it will definitely reduce the
overhead IO which can make the heavy process of compiling relatively
lighter.

Problems Shell creates

System Dependencies

Using shell code introduces various dependencies even though they
allowing prototyping of the code quickly. Shell script often use some
POSIX utilities like cat, grep, ls, mkdir, etc which are not included
in non-POSIX systems by default. These scripts do not have access to
the git’s internal low level API. So even trivial tasks have to be
performed by spawning new process every time. So when git is ported to
windows, it has to include all the utilities (namely a shell
interpreter, perl bindings and much more).

Scripts introduce extra overheads

Shell scripts do not have access to Git’s internal API which has
excellent use of cache thus reducing the unnecessary IO of user
configuration files, repository index and filesystem access. By using
a builtin we could exploit the cache system thus reducing the
overhead. As compiling / testing already involves quite a number of
resources, it would be good if we could do our best to make more
resources available for that.

Potential Problems

Rewriting may introduce bugs

Rewriting the shell script to C might introduce some bugs. This
problem will be properly taken care of in my method of approach
(described below). Still this approach will definitely not guarantee
that the functionality of the new will be exactly similar to the old
one, though it will greatly reduce its possibility. The reviews
provided by the seniors in the git community would help a lot in
reducing bugs since they know the common bugs and how to work around
them. The test suite of git is quite nice which has an awesome
coverage.

Rewritten can be hard to understand

Git does not like having many external dependencies, libraries or
executables other than what is provided by git itself and the
rewritten code should follow this. C does not provide with a lot of
other facilities like text processing which shell does whose C
implementation often spans to multiple lines. C is also notorious for
being a bit “cryptic”. This problem can be compensated by having well
written documentation with well defined inputs, outputs and behavior.

A peek into git bisect

How does it help?

Git bisect helps the software developers to find the commit that
introduced a regression. Software developers are interested in knowing
this because a commit changes a small set of code (most time). It is
much easier to understand and fix a problem when you know only need to
check a very small set of changes, than when you don’t know where to
look at it. It is not that the problem will be exactly in that commit
but it will be related to the behavior introduced in the commit.
Software bugs can be a nightmare when the code base is very large.
There would be a lot of sleepless night in figuring out the part which
causes the error. This is where git bisect helps. This is the one of
the most sought after tool

[PATCH v3 2/5] pretty-print: simplify the interaction between pp_handle_indent() and its caller

Instead of sometimes handling the output itself and some other times
forcing the caller handle the output, make the helper function
pp_handle_indent() responsible for the output for all cases.

Signed-off-by: Junio C Hamano 
---
 pretty.c | 32 +---
 1 file changed, 9 insertions(+), 23 deletions(-)

diff --git a/pretty.c b/pretty.c
index 0b40457..6d657fc 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1645,26 +1645,17 @@ static int pp_utf8_width(const char *start, const char 
*end)
 
 /*
  * pp_handle_indent() prints out the intendation, and
- * perhaps the whole line (without the final newline)
- *
- * Why "perhaps"? If there are tabs in the indented line
- * it will print it out in order to de-tabify the line.
- *
- * But if there are no tabs, we just fall back on the
- * normal "print the whole line".
+ * the whole line (without the final newline), after
+ * de-tabifying.
  */
-static int pp_handle_indent(struct strbuf *sb, int indent,
+static void pp_handle_indent(struct strbuf *sb, int indent,
 const char *line, int linelen)
 {
const char *tab;
 
strbuf_addchars(sb, ' ', indent);
 
-   tab = memchr(line, '\t', linelen);
-   if (!tab)
-   return 0;
-
-   do {
+   while ((tab = memchr(line, '\t', linelen)) != NULL) {
int width = pp_utf8_width(line, tab);
 
/*
@@ -1685,10 +1676,7 @@ static int pp_handle_indent(struct strbuf *sb, int 
indent,
/* Skip over the printed part .. */
linelen -= 1+tab-line;
line = tab + 1;
-
-   /* .. and look for the next tab */
-   tab = memchr(line, '\t', linelen);
-   } while (tab);
+   }
 
/*
 * Print out everything after the last tab without
@@ -1696,7 +1684,6 @@ static int pp_handle_indent(struct strbuf *sb, int indent,
 * align.
 */
strbuf_add(sb, line, linelen);
-   return 1;
 }
 
 void pp_remainder(struct pretty_print_context *pp,
@@ -1722,11 +1709,10 @@ void pp_remainder(struct pretty_print_context *pp,
first = 0;
 
strbuf_grow(sb, linelen + indent + 20);
-   if (indent) {
-   if (pp_handle_indent(sb, indent, line, linelen))
-   linelen = 0;
-   }
-   strbuf_add(sb, line, linelen);
+   if (indent)
+   pp_handle_indent(sb, indent, line, linelen);
+   else
+   strbuf_add(sb, line, linelen);
strbuf_addch(sb, '\n');
}
 }
-- 
2.8.0-rc4-198-g3f6b64c

--
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 v3 3/5] pretty-print: further abstract out pp_handle_indent()

Separate the call to add 4-space indent, and a new helper to add a
line after de-tabifying.

The new helper function strbuf_add_tabexpand() could later be moved
to strbuf.[ch] if other callers need to.

Signed-off-by: Junio C Hamano 
---
 pretty.c | 24 +++-
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/pretty.c b/pretty.c
index 6d657fc..717ceed 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1643,18 +1643,12 @@ static int pp_utf8_width(const char *start, const char 
*end)
return width;
 }
 
-/*
- * pp_handle_indent() prints out the intendation, and
- * the whole line (without the final newline), after
- * de-tabifying.
- */
-static void pp_handle_indent(struct strbuf *sb, int indent,
-const char *line, int linelen)
+
+static void strbuf_add_tabexpand(struct strbuf *sb,
+const char *line, int linelen)
 {
const char *tab;
 
-   strbuf_addchars(sb, ' ', indent);
-
while ((tab = memchr(line, '\t', linelen)) != NULL) {
int width = pp_utf8_width(line, tab);
 
@@ -1686,6 +1680,18 @@ static void pp_handle_indent(struct strbuf *sb, int 
indent,
strbuf_add(sb, line, linelen);
 }
 
+/*
+ * pp_handle_indent() prints out the intendation, and
+ * the whole line (without the final newline), after
+ * de-tabifying.
+ */
+static void pp_handle_indent(struct strbuf *sb, int indent,
+const char *line, int linelen)
+{
+   strbuf_addchars(sb, ' ', indent);
+   strbuf_add_tabexpand(sb, line, linelen);
+}
+
 void pp_remainder(struct pretty_print_context *pp,
  const char **msg_p,
  struct strbuf *sb,
-- 
2.8.0-rc4-198-g3f6b64c

--
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 v3 0/5] Expanding tabs in "git log" output

So here is the third try (previous round is found at $gmane/289166
and the very first one is at $gmane/288987).

The first three patches are essentially the same as v2.  The last
two updates how the tab-expansion is internally controlled:

 [4/5] adds a bit to pretty-commit-context that tells if tabs should
   be expanded.  Unlike v2 that tied this to pretty print format,
   this bit is orthogonal to the format, and theoretically it is
   possible to expand tabs even with --format=email.  Also,
   unlike v2, tabs are expanded not just in `medium` format, but
   also in `full` and `fuller` formats.

 [5/5] adds a new option --no-expand-tabs that controls the bit 4/5
   introduces, so that "git log [--pretty] --no-expand-tabs"
   would show the log message indented by 4 spaces, without tab
   expansion.

By the way, I have to say that I hate how pretty formatting and
revision machinery interact with each other.

pretty.c::commit_formats ought to be the authoritative source of how
each named format should work, but there are quite a many codepaths
that just assign CMIT_FMT_SOMETHING to revs->commit_format without
bothering with other fields in the cmt_fmt_map like is_tformat, and
I am not sure if they are working correctly even before this patch.

Junio C Hamano (4):
  pretty-print: simplify the interaction between pp_handle_indent() and its 
caller
  pretty-print: further abstract out pp_handle_indent()
  pretty-print: limit expand-tabs to selected --pretty formats
  pretty-print: teach "--no-expand-tabs" option to "git log"

Linus Torvalds (1):
  pretty-print: de-tabify indented logs to make things line up properly

 Documentation/pretty-options.txt |  6 +++
 commit.h |  1 +
 log-tree.c   |  1 +
 pretty.c | 88 
 revision.c   |  3 ++
 revision.h   |  1 +
 t/t4201-shortlog.sh  |  2 +-
 7 files changed, 92 insertions(+), 10 deletions(-)

[References]

http://thread.gmane.org/gmane.comp.version-control.git/288987/focus=289166


Interdiff since v2 is shown below.

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 173b932..671cebd 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -39,16 +39,6 @@ This is designed to be as compact as possible.
 
  
 
- 
-
-* 'noexpand'
-
- commit 
- Author: 
- Date:   
-
- 
-
  
 
 * 'full'
diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index 7032b1a..069b927 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -3,7 +3,7 @@
 
Pretty-print the contents of the commit logs in a given format,
where '' can be one of 'oneline', 'short', 'medium',
-   'full', 'fuller', 'email', 'raw', 'noexpand', 'format:'
+   'full', 'fuller', 'email', 'raw', 'format:'
and 'tformat:'.  When '' is none of the above,
and has '%placeholder' in it, it acts as if
'--pretty=tformat:' were given.
@@ -42,6 +42,12 @@ people using 80-column terminals.
verbatim; this means that invalid sequences in the original
commit may be copied to the output.
 
+--no-expand-tabs::
+   The formats that indent the log message by 4 spaces
+   (i.e. 'medium', 'full', and 'fuller') by default show tabs
+   in the log message expanded.  This option disables the
+   expansion.
+
 ifndef::git-rev-list[]
 --notes[=]::
Show the notes (see linkgit:git-notes[1]) that annotate the
diff --git a/commit.h b/commit.h
index d511c61..a7ef682 100644
--- a/commit.h
+++ b/commit.h
@@ -126,7 +126,6 @@ enum cmit_fmt {
CMIT_FMT_RAW,
CMIT_FMT_MEDIUM,
CMIT_FMT_DEFAULT = CMIT_FMT_MEDIUM,
-   CMIT_FMT_NOEXPAND,
CMIT_FMT_SHORT,
CMIT_FMT_FULL,
CMIT_FMT_FULLER,
@@ -148,6 +147,7 @@ struct pretty_print_context {
int preserve_subject;
struct date_mode date_mode;
unsigned date_mode_explicit:1;
+   unsigned expand_tabs_in_log:1;
int need_8bit_cte;
char *notes_message;
struct reflog_walk_info *reflog_info;
diff --git a/log-tree.c b/log-tree.c
index 60f9839..78a5381 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -683,6 +683,7 @@ void show_log(struct rev_info *opt)
ctx.fmt = opt->commit_format;
ctx.mailmap = opt->mailmap;
ctx.color = opt->diffopt.use_color;
+   ctx.expand_tabs_in_log = opt->expand_tabs_in_log;
ctx.output_encoding = get_log_output_encoding();
if (opt->from_ident.mail_begin && opt->from_ident.name_begin)
ctx.from_ident = >from_ident;
diff --git a/pretty.c b/pretty.c
index 8b533dc..5a33b7e 100644
--- a/pretty.c
+++ b/pretty.c
@@ -16,6 +16,7 @@ static struct cmt_fmt_map {
const char *name;

[PATCH v3 4/5] pretty-print: limit expand-tabs to selected --pretty formats

Make sure that "git log" (by default, it uses --pretty=medium)
and "git log --pretty={full,fuller}" are the only ones that trigger
the new "expand tabs in the log message" behaviour.

Signed-off-by: Junio C Hamano 
---
 commit.h|  1 +
 log-tree.c  |  1 +
 pretty.c| 26 --
 revision.c  |  1 +
 revision.h  |  1 +
 t/t4201-shortlog.sh |  2 +-
 6 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/commit.h b/commit.h
index 5d58be0..a7ef682 100644
--- a/commit.h
+++ b/commit.h
@@ -147,6 +147,7 @@ struct pretty_print_context {
int preserve_subject;
struct date_mode date_mode;
unsigned date_mode_explicit:1;
+   unsigned expand_tabs_in_log:1;
int need_8bit_cte;
char *notes_message;
struct reflog_walk_info *reflog_info;
diff --git a/log-tree.c b/log-tree.c
index 60f9839..78a5381 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -683,6 +683,7 @@ void show_log(struct rev_info *opt)
ctx.fmt = opt->commit_format;
ctx.mailmap = opt->mailmap;
ctx.color = opt->diffopt.use_color;
+   ctx.expand_tabs_in_log = opt->expand_tabs_in_log;
ctx.output_encoding = get_log_output_encoding();
if (opt->from_ident.mail_begin && opt->from_ident.name_begin)
ctx.from_ident = >from_ident;
diff --git a/pretty.c b/pretty.c
index 717ceed..5a33b7e 100644
--- a/pretty.c
+++ b/pretty.c
@@ -16,6 +16,7 @@ static struct cmt_fmt_map {
const char *name;
enum cmit_fmt format;
int is_tformat;
+   int expand_tabs_in_log;
int is_alias;
const char *user_format;
 } *commit_formats;
@@ -87,13 +88,13 @@ static int git_pretty_formats_config(const char *var, const 
char *value, void *c
 static void setup_commit_formats(void)
 {
struct cmt_fmt_map builtin_formats[] = {
-   { "raw",CMIT_FMT_RAW,   0 },
-   { "medium", CMIT_FMT_MEDIUM,0 },
-   { "short",  CMIT_FMT_SHORT, 0 },
-   { "email",  CMIT_FMT_EMAIL, 0 },
-   { "fuller", CMIT_FMT_FULLER,0 },
-   { "full",   CMIT_FMT_FULL,  0 },
-   { "oneline",CMIT_FMT_ONELINE,   1 }
+   { "raw",CMIT_FMT_RAW,   0,  0 },
+   { "medium", CMIT_FMT_MEDIUM,0,  1 },
+   { "short",  CMIT_FMT_SHORT, 0,  0 },
+   { "email",  CMIT_FMT_EMAIL, 0,  0 },
+   { "fuller", CMIT_FMT_FULLER,0,  1 },
+   { "full",   CMIT_FMT_FULL,  0,  1 },
+   { "oneline",CMIT_FMT_ONELINE,   1,  0 }
};
commit_formats_len = ARRAY_SIZE(builtin_formats);
builtin_formats_len = commit_formats_len;
@@ -172,6 +173,7 @@ void get_commit_format(const char *arg, struct rev_info 
*rev)
 
rev->commit_format = commit_format->format;
rev->use_terminator = commit_format->is_tformat;
+   rev->expand_tabs_in_log = commit_format->expand_tabs_in_log;
if (commit_format->format == CMIT_FMT_USERFORMAT) {
save_user_format(rev, commit_format->user_format,
 commit_format->is_tformat);
@@ -1685,11 +1687,15 @@ static void strbuf_add_tabexpand(struct strbuf *sb,
  * the whole line (without the final newline), after
  * de-tabifying.
  */
-static void pp_handle_indent(struct strbuf *sb, int indent,
+static void pp_handle_indent(struct pretty_print_context *pp,
+struct strbuf *sb, int indent,
 const char *line, int linelen)
 {
strbuf_addchars(sb, ' ', indent);
-   strbuf_add_tabexpand(sb, line, linelen);
+   if (pp->expand_tabs_in_log)
+   strbuf_add_tabexpand(sb, line, linelen);
+   else
+   strbuf_add(sb, line, linelen);
 }
 
 void pp_remainder(struct pretty_print_context *pp,
@@ -1716,7 +1722,7 @@ void pp_remainder(struct pretty_print_context *pp,
 
strbuf_grow(sb, linelen + indent + 20);
if (indent)
-   pp_handle_indent(sb, indent, line, linelen);
+   pp_handle_indent(pp, sb, indent, line, linelen);
else
strbuf_add(sb, line, linelen);
strbuf_addch(sb, '\n');
diff --git a/revision.c b/revision.c
index df56fce..8827d9f 100644
--- a/revision.c
+++ b/revision.c
@@ -1412,6 +1412,7 @@ void init_revisions(struct rev_info *revs, const char 
*prefix)
revs->skip_count = -1;
revs->max_count = -1;
revs->max_parents = -1;
+   revs->expand_tabs_in_log = 1;
 
revs->commit_format = CMIT_FMT_DEFAULT;
 
diff --git a/revision.h b/revision.h
index 23857c0..0bbfe0e 100644
--- a/revision.h
+++ b/revision.h
@@ -137,6 +137,7 @@ struct

[PATCH v3 5/5] pretty-print: teach "--no-expand-tabs" option to "git log"

The output formats of "git log" that indent the log message by 4
spaces have been updated to expand tabs by default in previous
steps, without a way to restore the original behaviour.

Introduce a new "--no-expand-tabs" option to allow this.

As the effect of options is cumulative,

$ git log [--pretty=medium] --no-expand-tabs

would not expand, while this invocation

$ git log --no-expand-tabs --pretty[=medium]

by virtue of having --pretty later on the command line, expands tabs
again.

We _could_ introduce --expand-tabs option as well, to allow

$ git log --pretty=email --expand-tabs

but we don't bother, as the output format that do not expand tabs by
default are mostly meant to transfer the contents as literally as
possible.

Signed-off-by: Junio C Hamano 
---
---
 Documentation/pretty-options.txt | 6 ++
 revision.c   | 2 ++
 t/t4201-shortlog.sh  | 4 ++--
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index 4b659ac..069b927 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -42,6 +42,12 @@ people using 80-column terminals.
verbatim; this means that invalid sequences in the original
commit may be copied to the output.
 
+--no-expand-tabs::
+   The formats that indent the log message by 4 spaces
+   (i.e. 'medium', 'full', and 'fuller') by default show tabs
+   in the log message expanded.  This option disables the
+   expansion.
+
 ifndef::git-rev-list[]
 --notes[=]::
Show the notes (see linkgit:git-notes[1]) that annotate the
diff --git a/revision.c b/revision.c
index 8827d9f..b0d2a36 100644
--- a/revision.c
+++ b/revision.c
@@ -1916,6 +1916,8 @@ static int handle_revision_opt(struct rev_info *revs, int 
argc, const char **arg
revs->verbose_header = 1;
revs->pretty_given = 1;
get_commit_format(arg+9, revs);
+   } else if (!strcmp(arg, "--no-expand-tabs")) {
+   revs->expand_tabs_in_log = 0;
} else if (!strcmp(arg, "--show-notes") || !strcmp(arg, "--notes")) {
revs->show_notes = 1;
revs->show_notes_given = 1;
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index d1e8259..2fec948 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -114,8 +114,8 @@ EOF
test_cmp expect out
 '
 
-test_expect_failure !MINGW 'shortlog from non-git directory' '
-   git log HEAD >log &&
+test_expect_success !MINGW 'shortlog from non-git directory' '
+   git log --no-expand-tabs HEAD >log &&
GIT_DIR=non-existing git shortlog -w out &&
test_cmp expect out
 '
-- 
2.8.0-rc4-198-g3f6b64c

--
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 v3 1/5] pretty-print: de-tabify indented logs to make things line up properly

From: Linus Torvalds 

A commit log message sometimes tries to line things up using tabs,
assuming fixed-width font with the standard 8-place tab settings.
Viewing such a commit however does not work well in "git log", as we
indent the lines by prefixing 4 spaces in front of them.

This should all line up:

  Column 1  Column 2
    
  A B
  ABCD  EFGH
  SPACESInstead of Tabs

Even with multi-byte UTF8 characters:

  Column 1  Column 2
    
  Ä B
  åäö   100
  A Møøse   once bit my sister..

Tab-expand the lines in "git log --pretty=medium" output (which is
the default), before prefixing 4 spaces.

This breaks a few tests in t4201, that tests "git shortlog".

 - One passes "git log" output to "git shortlog" to use the latter
   as a filter and does not expect the output of the former to be
   de-tabified.

 - The other expects that "git shortlog", when it reads the first
   line of the commit and produces the output itself, does not
   de-tabify it.

Mark them as expecting failure for now.

Signed-off-by: Linus Torvalds 
Signed-off-by: Junio C Hamano 
---
 pretty.c| 76 +++--
 t/t4201-shortlog.sh |  4 +--
 2 files changed, 76 insertions(+), 4 deletions(-)

diff --git a/pretty.c b/pretty.c
index 92b2870..0b40457 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1629,6 +1629,76 @@ void pp_title_line(struct pretty_print_context *pp,
strbuf_release();
 }
 
+static int pp_utf8_width(const char *start, const char *end)
+{
+   int width = 0;
+   size_t remain = end - start;
+
+   while (remain) {
+   int n = utf8_width(, );
+   if (n < 0 || !start)
+   return -1;
+   width += n;
+   }
+   return width;
+}
+
+/*
+ * pp_handle_indent() prints out the intendation, and
+ * perhaps the whole line (without the final newline)
+ *
+ * Why "perhaps"? If there are tabs in the indented line
+ * it will print it out in order to de-tabify the line.
+ *
+ * But if there are no tabs, we just fall back on the
+ * normal "print the whole line".
+ */
+static int pp_handle_indent(struct strbuf *sb, int indent,
+const char *line, int linelen)
+{
+   const char *tab;
+
+   strbuf_addchars(sb, ' ', indent);
+
+   tab = memchr(line, '\t', linelen);
+   if (!tab)
+   return 0;
+
+   do {
+   int width = pp_utf8_width(line, tab);
+
+   /*
+* If it wasn't well-formed utf8, or it
+* had characters with badly defined
+* width (control characters etc), just
+* give up on trying to align things.
+*/
+   if (width < 0)
+   break;
+
+   /* Output the data .. */
+   strbuf_add(sb, line, tab - line);
+
+   /* .. and the de-tabified tab */
+   strbuf_addchars(sb, ' ', 8-(width & 7));
+
+   /* Skip over the printed part .. */
+   linelen -= 1+tab-line;
+   line = tab + 1;
+
+   /* .. and look for the next tab */
+   tab = memchr(line, '\t', linelen);
+   } while (tab);
+
+   /*
+* Print out everything after the last tab without
+* worrying about width - there's nothing more to
+* align.
+*/
+   strbuf_add(sb, line, linelen);
+   return 1;
+}
+
 void pp_remainder(struct pretty_print_context *pp,
  const char **msg_p,
  struct strbuf *sb,
@@ -1652,8 +1722,10 @@ void pp_remainder(struct pretty_print_context *pp,
first = 0;
 
strbuf_grow(sb, linelen + indent + 20);
-   if (indent)
-   strbuf_addchars(sb, ' ', indent);
+   if (indent) {
+   if (pp_handle_indent(sb, indent, line, linelen))
+   linelen = 0;
+   }
strbuf_add(sb, line, linelen);
strbuf_addch(sb, '\n');
}
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index 7600a3e..987b708 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -93,7 +93,7 @@ test_expect_success 'output from user-defined format is 
re-wrapped' '
test_cmp expect log.predictable
 '
 
-test_expect_success !MINGW 'shortlog wrapping' '
+test_expect_failure !MINGW 'shortlog wrapping' '
cat >expect <<\EOF &&
 A U Thor (5):
   Test
@@ -114,7 +114,7 @@ EOF
test_cmp expect out
 '
 
-test_expect_success !MINGW 'shortlog from non-git directory' '
+test_expect_failure !MINGW 'shortlog from non-git directory' '
git log HEAD >log &&
GIT_DIR=non-existing git shortlog -w out &&
test_cmp expect out
--

Re: [RFC_PATCHv4 5/7] submodule update: respect submodule.actionOnLabel

2016-03-23 Thread Stefan Beller

On Tue, Mar 22, 2016 at 3:40 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> This change introduces the 'submodule.actionOnLabel' variable
>> in a repository configuration. Generally speaking 'submodule.actionOnLabel'
>> restricts the action of a command when no submodules are selected via the
>> command line explicitely to those submodules, which are selected by
>> 'submodule.actionOnLabel'. It can occur multiple times and can specify
>> the path, the name or one of the labels of a submodule to select that
>> submodule.
>>
>> The introduction of 'submodule.actionOnLabel' starts with
>> 'git submodule update' in this patch and other commands will follow
>> in later patches.
>>
>> 'submodule.actionOnLabel' implies '--init' in 'git submodule update'.
>>
>> Signed-off-by: Stefan Beller 
>>
>> TODO: generic documentation for submodule.actionOnLabel
>> TODO: documentation for submodule update
>
> TODO: a name that matches the concept better.

This is one of the hardest parts of the series so far. The last reviews
were mostly bike shedding about the name of the concept and I thought
we were settled to actionOnLabel as that fits best to what we want to do.

So let's revisit that. My current understanding of the design:

Generic properties in the data model:
 * Each submodule has a set of "things" attached to it. (A submodule
   can have none, one or many)
 * A "thing" can be attached to many submodules (That's why
   it was called groups in v1, and labels now)
 * The attachments of "things" to submodules can be viewed as a bipartite
   graph.
 * The attachment needs to work in a way, such that upstream
   can influence and redefine these attachments (e.g. .gitmodules file
   as part of the repo; another approach would be to have yet another file
   .gitlabels or such where you'd have a list of submodules belonging to a
   "thing")
 * If this feature is enabled, the user can select a set of submodules by
   selecting a set of "things" and all submodules connected to these things
   in the bipartite graph are selected. The expectation for a graph is to
   select a lot fewer "things" than submodules. By having this indirection
   via the graph, the selection of a subset of submodules is expected to
   be easier.

Properties I derived from discussion and the data model:
 * The user does not need to have a way of overwriting the bipartite graph,
   because they can specify submodules by these "things", by path or by name.
   (It would be convenient to do be able to overwrite these, but it is
not a strict
   requirement as the you can get any specification via a set of paths)

 * The user needs to make the explicit choice to use the new feature
   or not, as it has implications on the default behavior of submodule
   commands.

 * To make change of selection easy (which happens e.g. when switching
   branches or pulling in upstream changes), all submodules are initialized
   by default.

 * Once this feature is enabled a command doesn't apply to all initialized
   submodules by default any more, but the
   default set of submodules will be the selected set via the
   bipartite graph of "things".

(Originally I typed out some implementation specific thoughts, but they are
loaded with even more assumptions, so maybe we'd want to stay on this
high level first)

So any other naming proposals?

Thanks,
Stefan

>
> So in general
>
> $ git submodule $subcmd .
>
> may be the way to say "do $subcmd to all submodules", and
>
> $ git submodule $subcmd
>
> may have been "operate on nothing" (or may have been "operate on
> everything"), but with this feature,
>
> $ git submodule $subcmd
>
> will by default operate on submodules that match the criteria the
> new configuration variable specifies?
>
> I suspect that copying this from .gitmodules to .git/config will
> have security implications and will not be done?  What is the
> expected way for projects to suggest which set of submodules are the
> good ones to work on by default using this mechanism?
>
>> Signed-off-by: Stefan Beller 
>> ---
>>  builtin/submodule--helper.c |  22 -
>>  t/t7400-submodule-basic.sh  | 115 
>> 
>>  2 files changed, 136 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index a69b1f4..93760ec 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -573,6 +573,8 @@ struct submodule_update_clone {
>>   int current;
>>   struct module_list list;
>>   unsigned warn_if_uninitialized : 1;
>> + /* patterns to initialize */
>> + struct string_list *initialize;
>>
>>   /* update parameter passed via commandline */
>>   struct submodule_update_strategy update;
>> @@ -590,7 +592,7 @@ struct submodule_update_clone {
>>   /* If we want to stop as fast as possible and return an error */

Re: [PATCH] sha1_name.c: add an option to abort on ambiguous refs

2016-03-23 Thread Duy Nguyen

On Wed, Mar 23, 2016 at 10:45 PM, Johannes Schindelin
 wrote:
> Hi Duy,
>
> On Wed, 23 Mar 2016, Nguyễn Thái Ngọc Duy wrote:
>
>> There are cases when a warning on ambiguous refs may go unnoticed
>> (e.g. git-log filling up the whole screen). There are also cases when
>> people want to catch ambiguity early (e.g. it happens deep in some
>> script). In either case, aborting the program would accomplish it.
>
> Whenever I see a die() in code outside of builtin/*.c, I cringe. I do that
> because it was *exactly* something like that that caused a serious
> regression in the builtin am so that we had to resort back to spawning
> separate processes *just so* that we could catch error conditions and
> run certain code in that case.
>
> Maybe there would be a way to do what you want to do that does not fly in
> the face of libification?

Sorry I got nothing better.

> Maybe some strbuf with an atexit() that
> accumulates fatal errors that might be hidden and that are then written at
> the end of the program (colorfully, if isatty(2))?

That sounds hacky. If you don't want do die() deep inside then the
callers have to handle it, but get_sha1() is spread everywhere. Plus I
do not want to program to succeed when there is ambiguation because
the ref git chooses may not be what I want. I want it to abort (and
almost made a patch that die() unconditionally).
-- 
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

[PATCH] mergetools: implemented new mergetool file for ExamDiff

2016-03-23 Thread Jacob Nisnevich

Signed-off-by: Jacob Nisnevich 
---
 mergetools/examdiff   | 20 
 mergetools/mergetools_helpers | 24 
 mergetools/winmerge   | 23 +++
 3 files changed, 47 insertions(+), 20 deletions(-)
 create mode 100644 mergetools/examdiff
 create mode 100644 mergetools/mergetools_helpers

diff --git a/mergetools/examdiff b/mergetools/examdiff
new file mode 100644
index 000..c5edd0e
--- /dev/null
+++ b/mergetools/examdiff
@@ -0,0 +1,20 @@
+. "$MERGE_TOOLS_DIR/mergetools_helpers"
+
+diff_cmd () {
+   "$merge_tool_path" "$LOCAL" "$REMOTE" -nh
+}
+
+merge_cmd () {
+   touch "$BACKUP"
+   if $base_present
+   then
+   "$merge_tool_path" -merge "$LOCAL" "$BASE" "$REMOTE" 
-o:"$MERGED" -nh
+   else
+   "$merge_tool_path" -merge "$LOCAL" "$REMOTE" -o:"$MERGED" -nh
+   fi
+   check_unchanged
+}
+
+translate_merge_tool_path() {
+   mergetool_find_win32_cmd "ExamDiff.com" "ExamDiff Pro"
+}
diff --git a/mergetools/mergetools_helpers b/mergetools/mergetools_helpers
new file mode 100644
index 000..46ae2d8
--- /dev/null
+++ b/mergetools/mergetools_helpers
@@ -0,0 +1,24 @@
+mergetool_find_win32_cmd () {
+   executable=$1
+   folder=$2
+
+   # Use executable.com if it exists in $PATH
+   if type -p $executable >/dev/null 2>&1
+   then
+   printf $executable
+   return
+   fi
+
+   # Look for executable in the typical locations
+   for directory in $(env | grep -Ei '^PROGRAM(FILES(\(X86\))?|W6432)=' |
+   cut -d '=' -f 2- | sort -u)
+   do
+   if test -n "$directory" && test -x 
"$directory/$folder/$executable"
+   then
+   printf '%s' "$directory/$folder/$executable"
+   return
+   fi
+   done
+
+   printf $executable
+}
diff --git a/mergetools/winmerge b/mergetools/winmerge
index 74a66d4..c785be8 100644
--- a/mergetools/winmerge
+++ b/mergetools/winmerge
@@ -1,3 +1,5 @@
+. "$MERGE_TOOLS_DIR/mergetools_helpers"
+
 diff_cmd () {
"$merge_tool_path" -u -e "$LOCAL" "$REMOTE"
return 0
@@ -13,24 +15,5 @@ merge_cmd () {
 }
 
 translate_merge_tool_path() {
-   # Use WinMergeU.exe if it exists in $PATH
-   if type -p WinMergeU.exe >/dev/null 2>&1
-   then
-   printf WinMergeU.exe
-   return
-   fi
-
-   # Look for WinMergeU.exe in the typical locations
-   winmerge_exe="WinMerge/WinMergeU.exe"
-   for directory in $(env | grep -Ei '^PROGRAM(FILES(\(X86\))?|W6432)=' |
-   cut -d '=' -f 2- | sort -u)
-   do
-   if test -n "$directory" && test -x "$directory/$winmerge_exe"
-   then
-   printf '%s' "$directory/$winmerge_exe"
-   return
-   fi
-   done
-
-   printf WinMergeU.exe
+   mergetool_find_win32_cmd "WinMergeU.exe" "WinMerge"
 }
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] mergetools: implemented new mergetool file for ExamDiff

2016-03-23 Thread Jacob Nisnevich

Thanks for the hint. I used the other examples to fix the implementation of the 
ExamDiff mergetool. I've tested the Winmerge and ExamDiff mergetools with both
`git difftool` and `git mergetool` and both seem to work as expected. I've 
combined all my changes into one patch. Are there any other issues with my
changes?

Jacob Nisnevich (1):
  mergetools: implemented new mergetool file for ExamDiff

 mergetools/examdiff   | 20 
 mergetools/mergetools_helpers | 24 
 mergetools/winmerge   | 23 +++
 3 files changed, 47 insertions(+), 20 deletions(-)
 create mode 100644 mergetools/examdiff
 create mode 100644 mergetools/mergetools_helpers

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] mingw: skip some tests in t9115 due to file name issues

2016-03-23 Thread Torsten Bögershausen

> 
> Thanks; Torsten, sorry but could you do another round of check, please?
Thanks, v2 tested OK under Mac OS and Linux

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: "git tag --contains " is too chatty, if is invalid

On Sun, Mar 20, 2016 at 12:19:46PM +0530, Chirayu Desai wrote:

> I went for 3, and have sent a patch for that here - [PATCH/GSoC]
> parse-options: Add a new nousage opt
> However, it currently has one bug
> Running 'git tag --contains qq' twice will first show an error, then
> print qq, meaning that the first command creates the tag qq.
> Running 'git tag -l --contains qq' works fine.
> My first question is if 'git tag --contains' (without '-l') supposed to work?
> If not, then I would fix that bug, otherwise fix the bug my code
> introduced, and add tests for it.

Yes, "--contains" should imply "-l", and we should complain if there is
an attempt to create a tag.

This seems to work with the tip of "master":

  $ git tag --contains v2.8.0-rc3
  v2.8.0-rc3
  v2.8.0-rc4

  $ git tag --contains qq
  error: malformed object name qq
  [...and then the usage...]

  $ git tag --contains HEAD qq
  fatal: --contains option is only allowed with -l.

  $ git rev-parse --verify qq
  fatal: Needed a single revision

but with your patch:

  $ git tag --contains qq
  error: malformed object name qq

  $ git rev-parse --verify qq
  e9cacb7f8231dd6616671f9bcdd0945043483064

So presumably we're not aborting the program when the options fail to
parse, and it continues to process the "qq" as a tag to be created.

-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/GSoC] parse-options: Add a new nousage opt

On Sun, Mar 20, 2016 at 12:16:45PM +0530, Chirayu Desai wrote:

> diff --git a/parse-options-cb.c b/parse-options-cb.c
> index 239898d946..ac2ea4d674 100644
> --- a/parse-options-cb.c
> +++ b/parse-options-cb.c
> @@ -85,11 +85,15 @@ int parse_opt_commits(const struct option *opt, const 
> char *arg, int unset)
>  
>   if (!arg)
>   return -1;
> - if (get_sha1(arg, sha1))
> - return error("malformed object name %s", arg);
> + if (get_sha1(arg, sha1)) {
> + error("malformed object name %s", arg);
> + return -3;
> + }

Now that we have a few meaningful return values, should we have some
enum that gives them human-readable names?

E.g., why don't we allow "-2" here? I think it is because
parse_options_step internally uses it for "I don't know about that
option". But maybe we should have something like:

  enum PARSE_OPT_ERROR {
  PARSE_OPT_ERR_USAGE = -1,
  PARSE_OPT_ERR_UNKNOWN_OPTION = -2,
  PARSE_OPT_ERR_FAIL_QUIETLY = -3,
  }

(I don't quite like the final name, but I couldn't think of anything
better).

> diff --git a/parse-options.c b/parse-options.c
> index 47a9192060..d136c1afd0 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -158,6 +158,9 @@ static int get_value(struct parse_opt_ctx_t *p,
>   return (*opt->callback)(opt, NULL, 0) ? (-1) : 0;
>   if (get_arg(p, opt, flags, ))
>   return -1;
> + if (opt->flags & PARSE_OPT_NOUSAGE) {
> + return (*opt->callback)(opt, arg, 0);
> + }
>   return (*opt->callback)(opt, arg, 0) ? (-1) : 0;

Here you use PARSE_OPT_NOUSAGE to pass the callback's value directly
back to the rest of the option-parsing code. But can't we just intercept
"-3" always? It's possible that another callback is using it to
generically return an error, but it seems like a rather low risk, and
the resulting code is much simpler.

Or we could go the opposite direction. If a callback is annotated with
PARSE_OPT_NOUSAGE, why do we even need to care about its return value?
The callback could continue to return -1, and we would simply suppress
the usage message.

>   case OPTION_INTEGER:
> @@ -504,6 +507,8 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
>   goto show_usage_error;
>   case -2:
>   goto unknown;
> + case -3:
> + return PARSE_OPT_DONE;
>   }
>   continue;
>  unknown:

If I understand correctly, this is now getting the value from the
callback directly. What happens if a callback returns "-4" or "4"?

Also, this covers the parse_long_opt() call, but there are two
parse_short_opt() calls earlier. Wouldn't they need to learn the same
logic?

-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] git-send-pack: Fix --all option when used with directory

On Wed, Mar 23, 2016 at 05:22:13PM -0400, Jeff King wrote:

> > diff --git a/t/t9904-send-pack-all.sh b/t/t9904-send-pack-all.sh
> 
> The tests are roughly grouped by functionality. send-pack tests are in
> the t540x range, and this should probably go there. Though I also
> suspect it could easily be added to the end of an existing test script,
> which is preferable.
> 
> > +test_expect_success setup '
> 
> This setup seems a bit more complicated than it needs to be. It's nice
> to keep tests as simple as possible, so a reader can understand exactly
> what is being tested.
> 
> Here are a few things I think we can simplify:
> [...]

So I think we could replace your t9904 with something like this:

diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 04cea97..305ca7a 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -128,6 +128,18 @@ test_expect_success 'denyNonFastforwards trumps --force' '
test "$victim_orig" = "$victim_head"
 '
 
+test_expect_success 'send-pack --all sends all branches' '
+   # make sure we have at least 2 branches with different
+   # values, just to be thorough
+   git branch other-branch HEAD^ &&
+
+   git init --bare all.git &&
+   git send-pack --all all.git &&
+   git for-each-ref refs/heads >expect &&
+   git -C all.git for-each-ref refs/heads >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'push --all excludes remote-tracking hierarchy' '
mkdir parent &&
(
--
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

CISCO IP PHONES AND CPU's

2016-03-23 Thread Inc

Hi,

Our Stock list.

Brand NEW
96 x Cisco 7900 IP Phone
87 x Unified IP Phone 6900
12 x Unified IP Phone 8900
76 x Unified IP Phone 9900
55 x Unified IP Phone 8800
67 x Cisco 1921
67 x Cisco 1941
56 x Cisco CP-7961G 7961G
34 x Cisco CP-7971G-GE 7971G
19 x Cisco Unified IP Conference Station 7937G Model: 7937 CP-7937G
20 x Cisco CP-7975G 7975G

Brand New Sealed :

23 x  CISCO1921-SEC/K9
Conditions: Brand New Sealed
Description: CISCO 1921 Security Bundle w/SEC license PAK

45 x CISCO1921/K9
Conditions: Brand New Sealed
Description: CISCO 1921 Modular Router, 2 GE, 2 EHWIC slots, 512DRAM, IP Base

(1)  WS-C4500X-16SFP+
Serial number:  JAE183501L3
US2600

(1)  WS-C3850-48PW-S
Serial number:  FCW1823C0EW
US2650

(1)  WS-X6908-10G-2T
Serial number:  SAL1620CKUB
US3650

(1)  ASR1000-ESP10
Serial number:  JAE181306C3
US3800

(1)  AIR-CT5508-250-K9   (this is new but box is open!)
Serial number:  FCW1521L038
US4000

CPUs part number below

89 x  X5650

975 x  X5660

150 x  X5680

265 x  X5690

Kindly make your price offers.

Sincerely
Barbara Johnson
Laison Computech
210 N Scoring Ave,
Rialto California, 92376
Tel: +1-657-232-7047
Fax: +1-347-214-0478
Email: sa...@laisoncomputertech.us
--
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] git-send-pack: Fix --all option when used with directory

On Wed, Mar 23, 2016 at 06:24:22PM +0200, Stanislav Kolotinskiy wrote:

> When using git send-pack with --all option
> and a target directory, usage message is being
> displayed instead of performing the actual transmission.

Yeah, that seems very wrong.

Not that it matters for this bug, but for my own curiosity, what do you
use "send-pack --all" for? I've generally assumed that nobody directly
calls send-pack themselves these days, but of course we have no data to
support that either way. So I am always interested to hear about unusual
use cases.

> The reason for this issue is that refspecs variable is being
> calculated in a different way comparing to previous versions,
> and even though the number of refspecs (nr_refspecs) is 0,
> refspecs contain all the arguments and switches passed to send-pack.

Looks like this bisects to 068c77a (builtin/send-pack.c: use
parse_options API, 2015-08-19). Thanks for including a test, which made
the bisection easy.

I wondered how the original ever worked, since it also points to argv
with the "refspecs" variable. But we only do so when we see an actual
refspec argument, and otherwise leave it as NULL. Whereas the new code
lumps the destination and the refspecs into the same conditional.

That made me wonder if any other code cared about refspecs being NULL. I
don't think so, though. The other spots I looked at seem to use
nr_refspec, which is good. There is one interesting interaction with
--stdin, which _does_ always set refspecs to a non-NULL value.

In the original code (prior to 068c77a), doing this:

  git send-pack --stdin --all  diff --git a/t/t9904-send-pack-all.sh b/t/t9904-send-pack-all.sh

The tests are roughly grouped by functionality. send-pack tests are in
the t540x range, and this should probably go there. Though I also
suspect it could easily be added to the end of an existing test script,
which is preferable.

> +test_expect_success setup '

This setup seems a bit more complicated than it needs to be. It's nice
to keep tests as simple as possible, so a reader can understand exactly
what is being tested.

Here are a few things I think we can simplify:

> + git init --bare bare_repo && git init repo && (
> + cd repo &&

We're in a repository already, so we should just be able to push
directly out of the "main" test repository.

> + git remote add origin ../bare_repo &&

We don't need to define remotes; we can just push directly to paths.

> + date >file1 && git add file1 && test_tick &&
> + git commit -m Initial &&

You can use "test_commit" to make this simpler.

> + git push origin master &&
> +
> + git checkout -b other && date >file2 &&
> + git add file2 && test_tick &&
> + git commit -m Other &&
> + git push origin other

I guess you have multiple branches here so that we can be sure that
"--all" is pushing all of them. But your later test doesn't actually
check that.

> + ) && git init another && (
> + cd another &&
> +
> + git config receive.denyCurrentBranch ignore
> + )
> +'

If you make the destination repository bare, then you do not have to
worry about denyCurrentBranch.

> +test_expect_success 'send-pack --all should copy all refs' '
> + cd bare_repo && git send-pack --all ../another
> +'

We try to keep mutations of the test script state (like "cd") inside a
subshell, so they don't influence later tests. There aren't any later
tests now, of course, but it's one less thing for somebody coming along
later to have to worry 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

What's cooking in git.git (Mar 2016, #04; Wed, 23)

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

Three more minor fix-up topics are to be merged by 2.8 final, but we
are almost there.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[New Topics]

* jk/check-repository-format (2016-03-11) 10 commits
 - verify_repository_format: mark messages for translation
 - setup: drop repository_format_version global
 - setup: unify repository version callbacks
 - init: use setup.c's repo version verification
 - setup: refactor repo format reading and verification
 - config: drop git_config_early
 - check_repository_format_gently: stop using git_config_early
 - lazily load core.sharedrepository
 - wrap shared_repository global in get/set accessors
 - setup: document check_repository_format()

 The repository set-up sequence has been streamlined (the biggest
 change is that there is no longer git_config_early()), so that we
 do not attempt to look into refs/* when we know we do not have a
 Git repository.


* mj/pull-rebase-autostash (2016-03-21) 2 commits
  (merged to 'next' on 2016-03-23 at ebf1afa)
 + pull --rebase: add --[no-]autostash flag
 + git-pull.c: introduce git_pull_config()

 "git pull --rebase" learned "--[no-]autostash" option, so that
 the rebase.autostash configuration variable set to true can be
 overridden from the command line.

 Will merge to 'master' after 2.8 final.


* pb/commit-verbose-config (2016-03-14) 1 commit
 - commit: add a commit.verbose config variable
 (this branch uses pb/t7502-drop-dup.)

 "git commit" learned to pay attention to "commit.verbose"
 configuration variable and act as if "--verbose" option was given
 from the command line.


* pb/t7502-drop-dup (2016-03-11) 1 commit
  (merged to 'next' on 2016-03-15 at 037c877)
 + t/t7502 : drop duplicate test
 (this branch is used by pb/commit-verbose-config.)

 Code clean-up.

 Will merge to 'master' after 2.8 final.


* ss/commit-squash-msg (2016-03-21) 1 commit
  (merged to 'next' on 2016-03-23 at 0b72631)
 + commit: do not lose SQUASH_MSG contents

 When "git merge --squash" stopped due to conflict, the concluding
 "git commit" failed to read in the SQUASH_MSG that shows the log
 messages from all the squashed commits.

 Will merge to 'master' after 2.8 final.


* ls/p4-map-user (2016-03-15) 1 commit
  (merged to 'next' on 2016-03-23 at 9e0a4f5)
 + git-p4: map a P4 user to Git author name and email address

 Will merge to 'master' after 2.8 final.


* jc/merge-refuse-new-root (2016-03-23) 1 commit
  (merged to 'next' on 2016-03-23 at d7da4cf)
 + merge: refuse to create too cool a merge by default

 "git merge" used to allow merging two branches that have no common
 base by default, which led to a brand new history of an existing
 project created and then get pulled by an unsuspecting maintainer,
 which allowed an unnecessary parallel history merged into the
 existing project.  The command has been taught not to allow this by
 default, with an escape hatch "--allow-unrelated-histories" option
 to be used in a rare event that merges histories of two projects
 that started their lives independently.

 Will merge to 'master' after 2.8 final.


* jc/rerere-multi-wip (2016-03-21) 1 commit
 . WIP forget
 (this branch uses jc/rerere-multi.)


* jk/credential-cache-comment-exit (2016-03-18) 1 commit
  (merged to 'next' on 2016-03-23 at d2b8cc7)
 + credential-cache--daemon: clarify "exit" action semantics

 Will merge to 'master' after 2.8 final.


* jk/send-email-rtrim-mailrc-alias (2016-03-18) 1 commit
  (merged to 'next' on 2016-03-23 at 74f1f44)
 + send-email: ignore trailing whitespace in mailrc alias file

 Will merge to 'master' after 2.8 final.


* jk/test-httpd-config-nosystem (2016-03-18) 1 commit
  (merged to 'next' on 2016-03-23 at 245263b)
 + t/lib-httpd: pass through GIT_CONFIG_NOSYSTEM env

 Will merge to 'master' after 2.8 final.


* lt/pretty-expand-tabs (2016-03-17) 4 commits
 - pretty-print: add --pretty=noexpand
 - pretty-print: further abstract out pp_handle_indent()
 - pretty-print: simplify the interaction between pp_handle_indent() and its 
caller
 - pretty-print: de-tabify indented logs to make things line up properly

 Needs a UI rework.


* sb/clone-shallow-passthru (2016-03-23) 3 commits
 - clone: add t5614 to test cloning submodules with shallowness involved
 - submodule clone: pass along `local` option
 - clone: add `--shallow-submodules` flag
 (this branch uses sb/submodule-parallel-update; is tangled with 
sb/submodule-init.)

 "git clone" learned "--shallow-submodules" option.

 Needs review.


* sb/clone-t57-t56 (2016-03-16) 1 commit
  (merged to 'next' on 2016-03-23 at 5df850d)

Re: [RFC_PATCHv4 3/7] submodule-config: add method to check for being labeled

2016-03-23 Thread Stefan Beller

On Tue, Mar 22, 2016 at 3:30 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> In later patches we need to tell if a submodule is labeled by
>> the given labels.
>>
>> Signed-off-by: Stefan Beller 
>> ---
>
> Hmph, I would have expected that something like this would touch the
> module_list() implementation.  Probably that would happen in future
> steps, I guess?

I would have guessed, too. And I was about to say Jens argued against that in
an earlier patch series [1]. However I miss remembered and Jens pointed at
"git submodule init --label/groups" being a bad idea, not in the helper.

So we could still have a "git submodule--helper labels_apply "
query to expose this to shell parts. Most of the shell parts use module_list
so we could integrate that there too. ("git submodule_helper list
--labeled-only")

[1] $gmane/281670, specifically $gmane/281720


>
>>  submodule-config.c | 48 
>>  submodule-config.h |  3 +++
>>  2 files changed, 51 insertions(+)
>>
>> diff --git a/submodule-config.c b/submodule-config.c
>> index 7b48e59..b10a773 100644
>> --- a/submodule-config.c
>> +++ b/submodule-config.c
>> @@ -493,3 +493,51 @@ void submodule_free(void)
>>   cache_free();
>>   is_cache_init = 0;
>>  }
>> +
>> +int submodule_applicable_by_labels(const struct string_list *list,
>> +const struct submodule *sub)
>> +{
>> + int label_apply = 0;
>> + struct strbuf sb = STRBUF_INIT;
>> +
>> + if (!list)
>> + return 1;
>> +
>> + if (sub->labels) {
>> + struct string_list_item *item;
>> + for_each_string_list_item(item, sub->labels) {
>> + strbuf_reset();
>> + strbuf_addf(, "*%s", item->string);
>> + if (string_list_has_string(list, sb.buf)) {
>> + label_apply = 1;
>> + break;
>> + }
>> + }
>> + }
>> + if (sub->path) {
>> + /*
>> +  * NEEDSWORK: This currently works only for
>> +  * exact paths, but we want to enable
>> +  * inexact matches such wildcards.
>> +  */
>> + strbuf_reset();
>> + strbuf_addf(, "./%s", sub->path);
>> + if (string_list_has_string(list, sb.buf)) {
>> + label_apply = 1;
>> + }
>> + }
>> + if (sub->name) {
>> + /*
>> +  * NEEDSWORK: Same as with path. Do we want to
>> +  * support wildcards or such?
>> +  */
>> + strbuf_reset();
>> + strbuf_addf(, ":%s", sub->name);
>> + if (string_list_has_string(list, sb.buf)) {
>> + label_apply = 1;
>> + }
>> + }
>> + strbuf_release();
>> +
>> + return label_apply;
>> +}
>> diff --git a/submodule-config.h b/submodule-config.h
>> index 8d61df3..d67f666 100644
>> --- a/submodule-config.h
>> +++ b/submodule-config.h
>> @@ -30,4 +30,7 @@ const struct submodule *submodule_from_path(const unsigned 
>> char *commit_sha1,
>>   const char *path);
>>  void submodule_free(void);
>>
>> +int submodule_applicable_by_labels(const struct string_list *list,
>> +const struct submodule *sub);
>> +
>>  #endif /* SUBMODULE_CONFIG_H */
--
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] git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS

Jonathan Nieder  writes:

> I should have mentioned this is
>
> Reported-by: Shin Fan 
>
>> Signed-off-by: Jeff King 
>> ---
>> I just did this on master, and it is standalone. But for the reasons
>> above I think it would also be fine to stick on the tip of the
>> jk/submodule-c-credential topic.
>>
>>  config.c   |  2 +-
>>  t/t1300-repo-config.sh | 14 ++
>>  2 files changed, 15 insertions(+), 1 deletion(-)
>
> Reviewed-by: Jonathan Nieder 
> Tested-by: Jonathan Nieder 
>
> Thanks for the quick fix.
>
> Sincerely,
> Jonathan
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/287928/focus=289551

Thanks for keeping an eye on the 'next' front.

It is much more pleasant to find and fix problems before a new topic
hits 'master', and I wish there were more people like you who use
Git that is newer than 'master' regularly and report issues.

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 v8 2/2] commit: add a commit.verbose config variable

Pranit Bauva  writes:

> On Thu, Mar 24, 2016 at 12:49 AM, Junio C Hamano  wrote:
>> Pranit Bauva  writes:
>>
>>> On Sun, Mar 20, 2016 at 11:04 PM, Eric Sunshine  
>>> wrote:
>>> ...
 Alternatively, combine #1 and #2 into a single patch which drops the
 global test_set_editor() and, as an aside, also does "while here,
 let's use write_script() to create 'check-for'diff' rather than doing
 so manually".
>>>
>>> These changes seem nice. I will update and send the patch.
>>
>> So, has anything happened to this topic or has it been abandoned?
>>
>> I am not in a hurry, just wanted to see if I need to keep the old
>> one in my tree as a reminder to myself.
>
> Sorry for that! Actually I am bit caught up with writing my proposal
> for GSoC 2016. I would be able to complete that in around an hour.
> Then will work on this. Then on the shell function -> C function
> porting patch. Please bear with me for a little while.

Oh, no worries. Naturally the application/proposal should take
higher priority.

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 v8 2/2] commit: add a commit.verbose config variable

On Thu, Mar 24, 2016 at 12:49 AM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> On Sun, Mar 20, 2016 at 11:04 PM, Eric Sunshine  
>> wrote:
>> ...
>>> Alternatively, combine #1 and #2 into a single patch which drops the
>>> global test_set_editor() and, as an aside, also does "while here,
>>> let's use write_script() to create 'check-for'diff' rather than doing
>>> so manually".
>>
>> These changes seem nice. I will update and send the patch.
>
> So, has anything happened to this topic or has it been abandoned?
>
> I am not in a hurry, just wanted to see if I need to keep the old
> one in my tree as a reminder to myself.

Sorry for that! Actually I am bit caught up with writing my proposal
for GSoC 2016. I would be able to complete that in around an hour.
Then will work on this. Then on the shell function -> C function
porting patch. Please bear with me for a little while.
--
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 v8 2/2] commit: add a commit.verbose config variable

Pranit Bauva  writes:

> On Sun, Mar 20, 2016 at 11:04 PM, Eric Sunshine  
> wrote:
> ...
>> Alternatively, combine #1 and #2 into a single patch which drops the
>> global test_set_editor() and, as an aside, also does "while here,
>> let's use write_script() to create 'check-for'diff' rather than doing
>> so manually".
>
> These changes seem nice. I will update and send the patch.

So, has anything happened to this topic or has it been abandoned?

I am not in a hurry, just wanted to see if I need to keep the old
one in my tree as a reminder to myself.

--
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] mingw: skip some tests in t9115 due to file name issues

2016-03-23 Thread Torsten Bögershausen

On 2016-03-23 16.56, Junio C Hamano wrote:

>> Thanks, I used a slightly different version, as I had crafted it before
>> reading this mail already.
> 
> Thanks; Torsten, sorry but could you do another round of check, please?
Sure :-)
--
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] bisect--helper: convert a function in shell to C

On Wed, Mar 23, 2016 at 9:45 PM, Junio C Hamano  wrote:
> Johannes Schindelin  writes:
>
>>> +if (one_of(term, "help", "start", "skip", "next", "reset", "visualize",
>>> +"replay", "log", "run", NULL))
>>
>> If I understood Junio correctly, he meant to line up the second line with
>> the corresponding level. In this case, as "replay" is a parameter of the
>> one_of() function, the indentation would look like this instead:
>>
>>   if (one_of(term, "help", "start", "skip", "next", "reset", "visualize",
>>  "replay", "log", "run", NULL))
>
> Thanks for clarification.  It may also make sense to wrap the first
> line one word earlier.
>
>>> +die("can't use the builtin command '%s' as a term", term);
>>> +
>>> +/* In theory, nothing prevents swapping
>>> + * completely good and bad, but this situation
>>> + * could be confusing and hasn't been tested
>>> + * enough. Forbid it for now.
>>> + */
>>
>> I see quite a few comments that put the closing "*/" on its own line, but
>> do not award the same pleasure to the opening "/*". Personally, I much
>> prefer the opening "/*" to have an entire line to itself, too, but I guess
>> that there is enough precendence in Git's source code to accept both
>> forms.
>
> We do want to see "/*" and "*/" on their own lines, and new code
> should definitely do so.

I also think it is better to promote one format and try and reduce the
other one.

>>> +if (!strcmp(term, "bad") || !strcmp(term, "new"))
>>> +if (strcmp(revision, "bad"))
>>> +die("can't change the meaning of term '%s'", term);
>>> +
>>> +if(!strcmp(term, "good") || !strcmp(term, "old"))
>>> +if (strcmp(revision, "good"))
>>> +die("can't change the meaning of term '%s'", term);
>>
>> I am still convinced that
>>
>>   if ((one_of(term, "bad", "new", NULL) && strcmp(orig_term, "bad")) ||
>>   (one_of(term, "good", "old", NULL) && strcmp(orig_term, "good")))
>>   die("can't change the meaning of term '%s'", term);
>>
>> looks so much nicer.
>
> ... and more importantly, easier to understand what is going on.

I will take care about this next time.
--
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] bisect--helper: convert a function in shell to C

On Wed, Mar 23, 2016 at 9:54 PM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> On Wed, Mar 23, 2016 at 5:27 PM, Johannes Schindelin
>>  wrote:
>>> Hi Pranit,
>>>
>>> On Wed, 23 Mar 2016, Pranit Bauva wrote:
>>>
 Convert the code literally without changing its design even though it
 seems that it is obscure as to the use of comparing revision to different
 bisect arguments which seems like a problem in shell because of the way
 function arguments are handled.
>
> Are you talking about the need to do one_of("help", "start", ...)?
>
> I do not see how that is "problem in shell" or "they way function
> arguments are handled".
>
> git bisect bad
> git bisect good
>
> are the ways how you mark the current commit as bad or good, and
> recent change that introduced the "term" thingy allows you to
> replace these "bad" and "good" with your own words, but
>
> git bisect start
> git bisect help
>
> etc. have their own meaning, so you cannot say "I call bad state
> 'start' and good state 'help'" without confusing yourself.  You'd
> never be able to start or get help if you did so, and that does not
> have anything to do with "shell" or "function argument" which are
> implementation detail.
>
> You cannot fundamentally allow replacing bad/good with these
> blacklisted words unless you are going to adopt different command
> line syntax (e.g. instead of accepting "git bisect $bad" with a word
> chosen by the end user, use "git bisect mark $bad", and $bad can be
> any word including "start", "visualize", etc.).

Seems like I got confused. Thanks for the clarification. :)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] format-patch: introduce --base=auto option

Xiaolong Ye  writes:

> +
> + diff_setup();
> + DIFF_OPT_SET(, RECURSIVE);
> + diff_setup_done();

It is annoying that you moved "diff" stuff here (if it can be
initialized once at the beginning and then reused over and over,
it should have been done here from the beginning at PATCH 2/4).

> + if (!strcmp(base_commit, "auto")) {
> + curr_branch = branch_get(NULL);
> + upstream = branch_get_upstream(curr_branch, NULL);
> + if (upstream) {
> + if (get_sha1(upstream, sha1))
> + die(_("Failed to resolve '%s' as a valid 
> ref."), upstream);
> + base = lookup_commit_or_die(sha1, "upstream base");
> + oidcpy(>base_commit, >object.oid);
> + } else {
> + commit_patch_id(prerequisite_head, , sha1);
> + oidcpy(>parent_commit, 
> _head->object.oid);
> + hashcpy(bases->parent_patch_id.hash, sha1);
> + return;

What happens if you did this sequence?

$ git fetch origin
$ git checkout -b fork origin/master
$ git fetch origin
$ git format-patch --base=auto origin..

You grab the updated origin/master as base and use it here, no?
At that point the topology would look like:

  1---2---3 updated upstream
 /
0---X---Y---Z---A---B---C
^
old upstream

so you are basing your worn on "0" (old upstream) but setting base
to "3"

Wouldn't that trigger "base must be an ancestor of Z" check you had
in [PATCH 2/4]?

I also do not see the point of showing "parent id" which as far as I
can see is just a random commit object name and show different
output that is not even described what it is.  It would be better to

 * find the upstream (i.e. 3 in the picture) and then with our range
   (i.e. A B and C) compute the merge base (i.e. you would find 0)
   and use it as base;

 * if there is no upstream, error out and tell the user that there
   is no upstream.  The user is intelligent enough and knows what
   commit the base should be.

I suspect, but I didn't think things through.

--
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/4] format-patch: add '--base' option to record base tree info

Xiaolong Ye  writes:

Reviewing the patch out of order, caller first and then callee.

> +static void print_bases(struct base_tree_info *bases)
> +{
> + int i;
> +
> + /* Only do this once, either for the cover or for the first one */
> + if (is_null_oid(>base_commit))
> + return;
> +
> + printf("** base-commit-info **\n");

I am not sure if you want to have this line (an empty line might not
hurt), as the "base-commit: ..." that comes next is a clear enough
indication of what these lines are.

> + if (base_commit) {
> + struct commit *prerequisite_head = NULL;
> + if (list[nr - 1]->parents)
> + prerequisite_head = list[nr - 1]->parents->item;
> + memset(, 0, sizeof(bases));
> + reset_revision_walk();
> + prepare_bases(, base_commit, prerequisite_head);
> + }
> +

list[] holds the commits in reverse topological order, so the first
parent of the last element in the list[] would hopefully give you
the latest change your series depends on, and that is why you are
working on list[nr - 1] here.

I however think that is flawed.

What if your series A, B and C are on this topology?

---P---X---A---M---C
\ /
 Y---Z---B

"git format-patch --base=P -3 C" would show A, B and C.  It may show
B, A and C, as A and B are independent (you would be flattening the
history into the shape you have in your documentation part of the
patch in order to adjust for their interactions before running
format-patch if that were not the case).  Depending on which one
happens to be chosen as prerequisite_head, you would miss either X
or Y & Z, wouldn't you?

A simpler and safer (but arguably less useful) approach may be to
use the logic and limitation of your patch as-is but add code to
check that the history is linear and refuse to do the "base" thing.
But just in case you want to handle a more general case like the
above, read on.

> +static void prepare_bases(struct base_tree_info *bases,
> +   const char *base_commit,
> +   struct commit *prerequisite_head)
> +{
> + struct commit *base = NULL, *commit;
> + struct rev_info revs;
> + struct diff_options diffopt;
> + struct object_id *patch_id;
> + unsigned char sha1[20];
> + int pos = 0;
> +
> + if (!prerequisite_head)
> + return;
> + base = lookup_commit_reference_by_name(base_commit);
> + if (!base)
> + die(_("Unknown commit %s"), base_commit);
> + oidcpy(>base_commit, >object.oid);
> +
> + if (base == prerequisite_head)
> + return;
> +
> + if (!in_merge_bases(base, prerequisite_head))
> + die(_("base commit should be the ancestor of revs you 
> specified"));
> +
> + init_revisions(, NULL);
> + revs.max_parents = 1;
> +
> + base->object.flags |= UNINTERESTING;
> + add_pending_object(, >object, "base");
> + prerequisite_head->object.flags |= 0;
> + add_pending_object(, _head->object, 
> "prerequisite-head");
> +
> + diff_setup();
> + DIFF_OPT_SET(, RECURSIVE);
> + diff_setup_done();
> +
> + if (prepare_revision_walk())
> + die(_("revision walk setup failed"));
> + /*
> +  * Traverse the commits list between base and prerequisite head,
> +  * get the patch ids and stuff them in bases structure.
> +  */
> + while ((commit = get_revision()) != NULL) {
> + if (commit_patch_id(commit, , sha1))
> + return;
> + ALLOC_GROW(bases->patch_id, bases->nr_patch_id + 1, 
> bases->alloc_patch_id);
> + patch_id = bases->patch_id + pos;
> + hashcpy(patch_id->hash, sha1);
> + pos++;
> + bases->nr_patch_id++;

Micronit.  Aren't pos and nr_patch_id always the same?

> + }
> +}

I think the right thing to do is probably to start another revision
walk (which you do) but setting the starting points of the traversal
to all elements in the list[] (which you don't--you use either A^ or
B^).  And skip the ones in the list[] before grabbing its patch-id
in the loop.  That way, you do not have to worry about the topology
of the history tripping you up at all.

So I'd suggest to redo this function perhaps like so, if you do want
to handle the more general case:

static void prepare_bases(struct base_tree_info *bases,
  const char *base_commit,
  struct commit **list,
  int total)
{
... boilerplate ...

base = lookup_commit_reference_by_name(base_commit);
if (!base)
die(_("Unknown commit %s"), base_commit);
oidcpy(>base_commit, >object.oid);

init_revisions(, NULL);
revs.max_parents = 1;
add_pending_commit(, base, UNINTERESTING);
for (i = 0; i < total; i++)
add_pending_commit(, list[i],

Re: [PATCH] add option -n (--no-checkout) to git-worktree add

2016-03-23 Thread Eric Sunshine

On Wed, Mar 23, 2016 at 11:51 AM, Junio C Hamano  wrote:
> Ray Zhang  writes:
>
>> @@ -320,6 +323,7 @@ static int add(int ac, const char **av, const char 
>> *prefix)
>>   OPT_STRING('B', NULL, _branch_force, N_("branch"),
>>  N_("create or reset a branch")),
>>   OPT_BOOL(0, "detach", , N_("detach HEAD at named 
>> commit")),
>> + OPT_BOOL('n', "no-checkout", _checkout, N_("don't 
>> create a checkout")),
>
> This would allow --no-no-checkout, which is idiotic, wouldn't it?
>
> How about
>
> OPT_BOOL(0, "checkout", , N_("populate the new working 
> tree"))
>
> and set opts.checkout to true when initializing?

I think this code was copied verbatim from builtin/clone.c, and, as a
newcomer to the project, it's understandable that Ray Zhang imitated
existing code, but I agree that it would be better to avoid repeating
the misbehavior.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git-apply does not work in a sub-directory of a Git repository

2016-03-23 Thread Mehul Jain

On Wed, Mar 23, 2016 at 8:51 PM, Junio C Hamano  wrote:
> I think we do have --no-index (which is why I am largely ignoring
> the rest of your message as uninformed speculation for now).

--no-index command line flag is there for git-apply but unfortunately not
documented.

Also *auto-completion* for "git apply --no-index" doesn't work.

That is

git apply --no-i

should be auto completed and give

   git apply --no-index.

Probably following change will remove this problem.

diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index 45ec47f..860dae0 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -886,7 +886,7 @@ _git_apply ()
;;
--*)
__gitcomp "
-   --stat --numstat --summary --check --index
+   --stat --numstat --summary --check --index --no-index
--cached --index-info --reverse --reject --unidiff-zero
--apply --no-add --exclude=
--ignore-whitespace --ignore-space-change
--
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: Issue with git submodule update --init --depth=1 submodA

2016-03-23 Thread Stefan Beller

On Wed, Mar 23, 2016 at 1:28 AM, Jared Davison
 wrote:
> Hello all,
>
> I have encountered a problem with using submodules.
>
> The problem occurs when using
>
> repo-parent$ git submodule update --init --depth=1 submodA
>
> Submodule 'submoduleA' (.../submoduleA.git) registered for path 'submoduleA'
> Cloning into 'submoduleA'...
> Fetched in submodule path 'submoduleA', but it did not contain
> 8a1c22151b510160d7b41a019d7642ab2cd5e085. Direct fetching of that
> commit failed.
>
> Note the --depth=1 argument.

This is somewhat expected. The depth argument tells Git to cut off the commit
graph such that there is only 1 commit, the rest should not be there.
If you reference a commit in the missing rest, you have an issue as that commit
cannot be found.

As a workaround we try to fetch the missing sha1 if the remote allows it.
(You remote doesn't, so "Direct fetching of that commit failed.")

>
> This all works fine if the head of the submodA master branch is the
> revision required by the repo-parent as shown in "git submodule list"
>
> However, if submodA's history has progressed by one commit, then
> --depth=2 is required and this works fine. --depth=1 will no longer
> work and gives the error as above.
>
> Does depth always have to be counted from the head or most recent
> commit in the submodule branch?

The way it currently works, it is always counted from the most recent commit
in the branch.

>
> Could depth be counted from the required commit reference by the
> parent repo instead of the most recent? If so then --depth=1 could
> work I think.

That sounds interesting. :)
But it may get confusing fast:
* Which reference commit do you mean in the parent? (The topmost commit
  I would assume?)
* Up to now the submodule is a self sufficient repository, i.e. it doesn't need
  to know about the parent project and could still work great as a standalone
  repository. By making depth dependent on the parent project, would there be
  a difference in
cd  && git fetch --depth 12
  and
git submodule update --depth 12 
  ?

I thought about adding a new commandline flag instead of overloading depth.
--submodule-enclosure= or such to mean "get all commits the parent is
referencing in its topmost  commits".

>
> The reason I would like to do this is that the history actually
> contains some fairly large files and I wish to clone only the history
> for the current version of the files in that most recent commit. This
> all works great until someone pushes a commit into the submodule. I am
> using this as part of a continuous integration process which will
> build branches that reference submodules where the referenced commit
> may go back a long way back in history.
>
> Trying to determine the correct depth parameter value to use is
> impossible as with time it will be a growing amount as commits are
> added to the submodule branch.

Yeah you would rather want a --since= instead of a --depth
argument I'd assume?

>
> Another user found the same issue:  http://stackoverflow.com/a/25875273
>
> I have just compiled the git from "next" branch source, "git version
> 2.8.0.rc4.233.g1aaf96d" and have confirmed this is still the
> behaviour.
>
> Thanks for reading my enquiry and for your thoughts on this topic.
>
> Jared
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git-apply does not work in a sub-directory of a Git repository

Junio C Hamano  writes:

> See
>
>   http://thread.gmane.org/gmane.comp.version-control.git/288316/focus=288321
>
> I agree it is bad that it silently ignores the path outside the
> directory.  When run with --verbose, we should say "Skipped X that
> is outside the directory." or something like that, just like we
> issue notices when we applied with offset, etc.

Another thing we may want to do is to loosen (or redo) the logic
in builtin/apply.c::use_patch()

static int use_patch(struct patch *p)
{
const char *pathname = p->new_name ? p->new_name : p->old_name;
int i;

/* Paths outside are not touched regardless of "--include" */
if (0 < prefix_length) {
int pathlen = strlen(pathname);
if (pathlen <= prefix_length ||
memcmp(prefix, pathname, prefix_length))
return 0;
}

The include/exclude mechanism does use wildmatch() but does not use
the pathspec mechanism (it predates the pathspec machinery that was
made reusable in places like this).  We should be able to

$ cd d/e/e/p/d/i/r
$ git apply --include=:/ ../../../../../../../patch

to lift this limitation.  IOW, we can think of the use_patch() to
include only the paths in the subdirectory we are in by default, but
we can make it allow --include/--exclude command line option to
override that default.

That way, the plain-vanilla use would still retain the "when working
in subdirectory, we only touch that subdirectory" behaviour, which
existing scripts may depend on, but users can loosen the default as
necessary.
--
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] config --show-origin: report paths with forward slashes

Johannes Schindelin  writes:

>> > diff --git a/path.c b/path.c
>> > index 8b7e168..969b494 100644
>> > --- a/path.c
>> > +++ b/path.c
>> > @@ -584,6 +584,9 @@ char *expand_user_path(const char *path)
>> >if (!home)
>> >goto return_null;
>> >strbuf_addstr(_path, home);
>> > +#ifdef GIT_WINDOWS_NATIVE
>> > +  convert_slashes(user_path.buf);
>> > +#endif
>> 
>> Hmm, I wonder if we want to do this at a bit lower level,
>
> Well, I tried to be careful. There *are* circumstamces when backslashes
> are required (CreateProcess() comes to mind), so I wanted to have this
> conversion as much only in the user-visible output as possible.

I was able to guess that it would be the reason, and I was willing
to accept this as a short-term workaround.

As you are very well aware, the usual pattern we use is to implement
a higher level function (e.g. expand_user_path() in this case) in
terms of helpers that offer abstraction of implementation details
that may be platform specific (e.g. getenv() may be implemented
differently on Windows).  An "#ifdef" in otherwise platform agnostic
codepath like this one is a sign that the code is not well thought
out to find the right abstraction to use to follow that pattern.

I was mostly reacting to that "#ifdef" and thinking aloud what the
right abstraction is appropriate.  As a short-term workaround, the
above would have to do.

And no, I do not think convert_slashes() that becomes no-op on
non-windows platforms is the right abstraction.

--
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] bisect--helper: convert a function in shell to C

Pranit Bauva  writes:

> On Wed, Mar 23, 2016 at 5:27 PM, Johannes Schindelin
>  wrote:
>> Hi Pranit,
>>
>> On Wed, 23 Mar 2016, Pranit Bauva wrote:
>>
>>> Convert the code literally without changing its design even though it
>>> seems that it is obscure as to the use of comparing revision to different
>>> bisect arguments which seems like a problem in shell because of the way
>>> function arguments are handled.

Are you talking about the need to do one_of("help", "start", ...)?

I do not see how that is "problem in shell" or "they way function
arguments are handled".

git bisect bad
git bisect good

are the ways how you mark the current commit as bad or good, and
recent change that introduced the "term" thingy allows you to
replace these "bad" and "good" with your own words, but

git bisect start
git bisect help

etc. have their own meaning, so you cannot say "I call bad state
'start' and good state 'help'" without confusing yourself.  You'd
never be able to start or get help if you did so, and that does not
have anything to do with "shell" or "function argument" which are
implementation detail.

You cannot fundamentally allow replacing bad/good with these
blacklisted words unless you are going to adopt different command
line syntax (e.g. instead of accepting "git bisect $bad" with a word
chosen by the end user, use "git bisect mark $bad", and $bad can be
any word including "start", "visualize", etc.).
--
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] git-send-pack: Fix --all option when used with directory

2016-03-23 Thread Stanislav Kolotinskiy

When using git send-pack with --all option
and a target directory, usage message is being
displayed instead of performing the actual transmission.

The reason for this issue is that refspecs variable is being
calculated in a different way comparing to previous versions,
and even though the number of refspecs (nr_refspecs) is 0,
refspecs contain all the arguments and switches passed to send-pack.

This ensures that send-pack will stop execution only when --all
or --mirror switch is used in conjunction with any refs passed.

Signed-off-by: Stanislav Kolotinskiy 
---
 builtin/send-pack.c  |  2 +-
 t/t9904-send-pack-all.sh | 32 
 2 files changed, 33 insertions(+), 1 deletion(-)
 create mode 100755 t/t9904-send-pack-all.sh

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index f6e5d64..19f0577 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -225,7 +225,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
 * --all and --mirror are incompatible; neither makes sense
 * with any refspecs.
 */
-   if ((refspecs && (send_all || args.send_mirror)) ||
+   if ((nr_refspecs > 0 && (send_all || args.send_mirror)) ||
(send_all && args.send_mirror))
usage_with_options(send_pack_usage, options);
 
diff --git a/t/t9904-send-pack-all.sh b/t/t9904-send-pack-all.sh
new file mode 100755
index 000..f68d004
--- /dev/null
+++ b/t/t9904-send-pack-all.sh
@@ -0,0 +1,32 @@
+#!/bin/sh
+
+test_description='Make sure that send-pack --all copies all refs'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+
+   git init --bare bare_repo && git init repo && (
+   cd repo &&
+
+   git remote add origin ../bare_repo &&
+   date >file1 && git add file1 && test_tick &&
+   git commit -m Initial &&
+   git push origin master &&
+
+   git checkout -b other && date >file2 &&
+   git add file2 && test_tick &&
+   git commit -m Other &&
+   git push origin other
+   ) && git init another && (
+   cd another &&
+
+   git config receive.denyCurrentBranch ignore
+   )
+'
+
+test_expect_success 'send-pack --all should copy all refs' '
+   cd bare_repo && git send-pack --all ../another
+'
+
+test_done
-- 
2.7.4

--
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] bisect--helper: convert a function in shell to C

Johannes Schindelin  writes:

>> +if (one_of(term, "help", "start", "skip", "next", "reset", "visualize",
>> +"replay", "log", "run", NULL))
>
> If I understood Junio correctly, he meant to line up the second line with
> the corresponding level. In this case, as "replay" is a parameter of the
> one_of() function, the indentation would look like this instead:
>
>   if (one_of(term, "help", "start", "skip", "next", "reset", "visualize",
>  "replay", "log", "run", NULL))

Thanks for clarification.  It may also make sense to wrap the first
line one word earlier.

>> +die("can't use the builtin command '%s' as a term", term);
>> +
>> +/* In theory, nothing prevents swapping
>> + * completely good and bad, but this situation
>> + * could be confusing and hasn't been tested
>> + * enough. Forbid it for now.
>> + */
>
> I see quite a few comments that put the closing "*/" on its own line, but
> do not award the same pleasure to the opening "/*". Personally, I much
> prefer the opening "/*" to have an entire line to itself, too, but I guess
> that there is enough precendence in Git's source code to accept both
> forms.

We do want to see "/*" and "*/" on their own lines, and new code
should definitely do so.

>> +if (!strcmp(term, "bad") || !strcmp(term, "new"))
>> +if (strcmp(revision, "bad"))
>> +die("can't change the meaning of term '%s'", term);
>> +
>> +if(!strcmp(term, "good") || !strcmp(term, "old"))
>> +if (strcmp(revision, "good"))
>> +die("can't change the meaning of term '%s'", term);
>
> I am still convinced that
>
>   if ((one_of(term, "bad", "new", NULL) && strcmp(orig_term, "bad")) ||
>   (one_of(term, "good", "old", NULL) && strcmp(orig_term, "good")))
>   die("can't change the meaning of term '%s'", term);
>
> looks so much nicer.

... and more importantly, easier to understand what is going on.

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 4/4] mingw: skip some tests in t9115 due to file name issues

Johannes Schindelin  writes:

> Hi Junio & Torsten,
>
> On Wed, 23 Mar 2016, Torsten Bögershausen wrote:
>
>> On 2016-03-22 23.57, Junio C Hamano wrote:
>> > Dscho, I queued two out of these four, with a proposed fix-up patch
>> > for each of them, on 'pu'; but I won't squash them together myself
>> > without hearing from you as I do not test mingw or macosx.
>
> The suggested changes will be squashed into v2.
>
>> Beside that, do we want to amend the commit message like this:
>> 
>> Author: Johannes Schindelin 
>> Date:   Tue Mar 22 18:43:00 2016 +0100
>> 
>> skip some tests in t9115 due to file name issues
>> 
>> These two tests wanted to write file names which work under Linux or
>> CYGWIN, but are incompatible with file naming rules under mingw or HFS.
>> 
>> Signed-off-by: Johannes Schindelin 
>> Signed-off-by: Torsten Bögershausen 
>> Reviewed-by: Jonathan Nieder 
>> Signed-off-by: Junio C Hamano 
>
> Thanks, I used a slightly different version, as I had crafted it before
> reading this mail already.

Thanks; Torsten, sorry but could you do another round of check, please?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] add option -n (--no-checkout) to git-worktree add

Ray Zhang  writes:

> @@ -320,6 +323,7 @@ static int add(int ac, const char **av, const char 
> *prefix)
>   OPT_STRING('B', NULL, _branch_force, N_("branch"),
>  N_("create or reset a branch")),
>   OPT_BOOL(0, "detach", , N_("detach HEAD at named 
> commit")),
> + OPT_BOOL('n', "no-checkout", _checkout, N_("don't 
> create a checkout")),

This would allow --no-no-checkout, which is idiotic, wouldn't it?

How about

OPT_BOOL(0, "checkout", , N_("populate the new working tree"))

and set opts.checkout to true when initializing?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] add option -n (--no-checkout) to git-worktree add

2016-03-23 Thread Eric Sunshine

On Wed, Mar 23, 2016 at 11:08 AM, Ray Zhang  wrote:
> add option -n (--no-checkout) to git-worktree add

Alternate:

worktree: add: introduce --no-checkout option

> By adding option -n, we can make some customizations before checkout, like 
> sparse checkout, etc.

This parallels git-clone's --no-checkout. Okay.

Typically, one would not squat on a short option (-n) when first
introducing a feature and would only add the short equivalent after
the option proved popular, however, in this case, as git-clone
supports -n, I suppose finger muscle-memory is a consideration.

By the way, please wrap the commit message at 70-72 characters or so.

> Signed-off-by: Ray Zhang 
> ---
>  builtin/worktree.c | 14 +-
>  1 file changed, 9 insertions(+), 5 deletions(-)

This change needs corresponding documentation
(Documentation/git-worktree.txt) and test (t/t2025-worktree-add.sh)
updates.

Thanks.

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 38b5609..14ca3d9 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -21,6 +21,7 @@ static const char * const worktree_usage[] = {
>  struct add_opts {
> int force;
> int detach;
> +   int no_checkout;
> const char *new_branch;
> int force_new_branch;
>  };
> @@ -284,11 +285,13 @@ static int add_worktree(const char *path, const char 
> *refname,
> if (ret)
> goto done;
>
> -   cp.argv = NULL;
> -   argv_array_clear();
> -   argv_array_pushl(, "reset", "--hard", NULL);
> -   cp.env = child_env.argv;
> -   ret = run_command();
> +   if (!opts->no_checkout) {
> +   cp.argv = NULL;
> +   argv_array_clear();
> +   argv_array_pushl(, "reset", "--hard", NULL);
> +   cp.env = child_env.argv;
> +   ret = run_command();
> +   }
> if (!ret) {
> is_junk = 0;
> free(junk_work_tree);
> @@ -320,6 +323,7 @@ static int add(int ac, const char **av, const char 
> *prefix)
> OPT_STRING('B', NULL, _branch_force, N_("branch"),
>N_("create or reset a branch")),
> OPT_BOOL(0, "detach", , N_("detach HEAD at named 
> commit")),
> +   OPT_BOOL('n', "no-checkout", _checkout, N_("don't 
> create a checkout")),
> OPT_END()
> };
--
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] sha1_name.c: add an option to abort on ambiguous refs

Hi Duy,

On Wed, 23 Mar 2016, Nguyễn Thái Ngọc Duy wrote:

> There are cases when a warning on ambiguous refs may go unnoticed
> (e.g. git-log filling up the whole screen). There are also cases when
> people want to catch ambiguity early (e.g. it happens deep in some
> script). In either case, aborting the program would accomplish it.

Whenever I see a die() in code outside of builtin/*.c, I cringe. I do that
because it was *exactly* something like that that caused a serious
regression in the builtin am so that we had to resort back to spawning
separate processes *just so* that we could catch error conditions and
run certain code in that case.

Maybe there would be a way to do what you want to do that does not fly in
the face of libification? Maybe some strbuf with an atexit() that
accumulates fatal errors that might be hidden and that are then written at
the end of the program (colorfully, if isatty(2))?

Ciao,
Dscho

[PATCH] add option -n (--no-checkout) to git-worktree add

2016-03-23 Thread Ray Zhang

By adding option -n, we can make some customizations before checkout, like 
sparse checkout, etc.

Signed-off-by: Ray Zhang 
---
 builtin/worktree.c | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 38b5609..14ca3d9 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -21,6 +21,7 @@ static const char * const worktree_usage[] = {
 struct add_opts {
int force;
int detach;
+   int no_checkout;
const char *new_branch;
int force_new_branch;
 };
@@ -284,11 +285,13 @@ static int add_worktree(const char *path, const char 
*refname,
if (ret)
goto done;
 
-   cp.argv = NULL;
-   argv_array_clear();
-   argv_array_pushl(, "reset", "--hard", NULL);
-   cp.env = child_env.argv;
-   ret = run_command();
+   if (!opts->no_checkout) {
+   cp.argv = NULL;
+   argv_array_clear();
+   argv_array_pushl(, "reset", "--hard", NULL);
+   cp.env = child_env.argv;
+   ret = run_command();
+   }
if (!ret) {
is_junk = 0;
free(junk_work_tree);
@@ -320,6 +323,7 @@ static int add(int ac, const char **av, const char *prefix)
OPT_STRING('B', NULL, _branch_force, N_("branch"),
   N_("create or reset a branch")),
OPT_BOOL(0, "detach", , N_("detach HEAD at named 
commit")),
+   OPT_BOOL('n', "no-checkout", _checkout, N_("don't 
create a checkout")),
OPT_END()
};
 

--
https://github.com/git/git/pull/217
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git-apply does not work in a sub-directory of a Git repository

Duy Nguyen  writes:

> 1) add --no-index to force git-apply ignore .git, --git (or some other
> name) to apply patches as if running from topdir, add a config key to
> choose default behavior

I think we do have --no-index (which is why I am largely ignoring
the rest of your message as uninformed speculation for now).

See

  http://thread.gmane.org/gmane.comp.version-control.git/288316/focus=288321

I agree it is bad that it silently ignores the path outside the
directory.  When run with --verbose, we should say "Skipped X that
is outside the directory." or something like that, just like we
issue notices when we applied with offset, etc.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[GIT PULL] l10n updates for 2.8.0 round 3#2

2016-03-23 Thread Jiang Xin

Hi Junio,

The following changes since commit 26e4cbec4558ea21cd572bfc915a462f63c1ebb4:

  l10n: zh_CN: review for git v2.8.0 l10n round 2 (2016-03-20 18:46:02 +0800)

are available in the git repository at:

  git://github.com/git-l10n/git-po master

for you to fetch changes up to 103ee5c21ea4d63e78b7b984d9611eacd0a06099:

  Merge branch 'master' of https://github.com/vnwildman/git
(2016-03-23 23:01:51 +0800)


Alex Henrie (1):
  l10n: ca.po: update translation

Jiang Xin (2):
  Merge branch 'master' of git://github.com/alexhenrie/git-po
  Merge branch 'master' of https://github.com/vnwildman/git

Trần Ngọc Quân (1):
  l10n: vi.po (2530t): Update translation

Vasco Almeida (1):
  l10n: pt_PT: Update and add new translations

 po/ca.po|  2946 ++--
 po/pt_PT.po | 14113 --
 po/vi.po|   155 +-
 3 files changed, 12493 insertions(+), 4721 deletions(-)

--
Jiang Xin
--
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] bisect--helper: convert a function in shell to C

On Wed, Mar 23, 2016 at 4:52 PM, Johannes Schindelin
 wrote:
> Hi Pranit,
>
> On Tue, 22 Mar 2016, Pranit Bauva wrote:
>
>> I did run the tests. They produce the same results as they did before.
>> To ease review I will next time include these the output of the tests
>> in the commented section.
>>
>> t6002-rev-list-bisect.sh : http://paste.ubuntu.com/15473728/
>> t6030-bisect-porcelain.sh : http://paste.ubuntu.com/15473734/
>> t6041-bisect-submodule.sh : http://paste.ubuntu.com/15473743/
>>
>> Is there any other test I would need to run?
>
> Oh, I just meant to point out that you need to make sure that the entire
> test suite passes after your patch series (and ideally, after every patch,
> that is at least what I frequently test before sending out patch series).

Up to now, I used to only run the tests of the concerned area of the
patch. Though from next time, I will take care to run the complete
test suite.
--
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] sha1_name.c: add an option to abort on ambiguous refs

2016-03-23 Thread Nguyễn Thái Ngọc Duy

There are cases when a warning on ambiguous refs may go unnoticed
(e.g. git-log filling up the whole screen). There are also cases when
people want to catch ambiguity early (e.g. it happens deep in some
script). In either case, aborting the program would accomplish it.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/config.txt |  3 ++-
 config.c |  5 -
 sha1_name.c  | 10 --
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2cd6bdd..4172f59 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -522,7 +522,8 @@ core.sharedRepository::
 
 core.warnAmbiguousRefs::
If true, Git will warn you if the ref name you passed it is ambiguous
-   and might match multiple refs in the repository. True by default.
+   and might match multiple refs in the repository. If set to "fatal",
+   the program will abort on ambiguous refs. True by default.
 
 core.compression::
An integer -1..9, indicating a default compression level.
diff --git a/config.c b/config.c
index 9ba40bc..79f1ea5 100644
--- a/config.c
+++ b/config.c
@@ -738,7 +738,10 @@ static int git_default_core_config(const char *var, const 
char *value)
}
 
if (!strcmp(var, "core.warnambiguousrefs")) {
-   warn_ambiguous_refs = git_config_bool(var, value);
+   if (!strcasecmp(value, "fatal"))
+   warn_ambiguous_refs = 2;
+   else
+   warn_ambiguous_refs = git_config_bool(var, value);
return 0;
}
 
diff --git a/sha1_name.c b/sha1_name.c
index 3acf221..a0f0ab9 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -480,6 +480,8 @@ static int get_sha1_basic(const char *str, int len, 
unsigned char *sha1,
warning(warn_msg, len, str);
if (advice_object_name_warning)
fprintf(stderr, "%s\n", 
_(object_name_msg));
+   if (warn_ambiguous_refs > 1)
+   die(_("cannot continue with ambiguous 
refs"));
}
free(real_ref);
}
@@ -537,8 +539,12 @@ static int get_sha1_basic(const char *str, int len, 
unsigned char *sha1,
 
if (warn_ambiguous_refs && !(flags & GET_SHA1_QUIETLY) &&
(refs_found > 1 ||
-!get_short_sha1(str, len, tmp_sha1, GET_SHA1_QUIETLY)))
-   warning(warn_msg, len, str);
+!get_short_sha1(str, len, tmp_sha1, GET_SHA1_QUIETLY))) {
+   if (warn_ambiguous_refs > 1)
+   die(warn_msg, len, str);
+   else
+   warning(warn_msg, len, str);
+   }
 
if (reflog_len) {
int nth, i;
-- 
2.8.0.rc0.210.gd302cd2

--
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] bisect--helper: convert a function in shell to C

On Wed, Mar 23, 2016 at 5:27 PM, Johannes Schindelin
 wrote:
> Hi Pranit,
>
> On Wed, 23 Mar 2016, Pranit Bauva wrote:
>
>> Convert the code literally without changing its design even though it
>> seems that it is obscure as to the use of comparing revision to different
>> bisect arguments which seems like a problem in shell because of the way
>> function arguments are handled.
>
> I still believe that it would make for an improvement to replace
> "revision" by "orig_term".

I missed this. Will do it.

>> The argument handling is kind of hard coded right now because it is not
>> really be meant to be used like this and this is just for testing
>> purposes whether this new method is as functional as its counter part.
>> The shell counter part of the method has been retained for historical
>> purposes.
>
> Well, maybe the argument handling is really meant to be used like this in
> the end? Just skip that paragraph.

Sure.

>> Also using OPT_CMDMODE() to handle check-term-format and next-all
>> because these sub commands are independent and error should be shown if
>> used together and should be handled independently.
>
> As is often the case, after some discussion a single patch becomes more
> than just one change. This is what we see here, too: OPT_CMDMODE() is a
> change that needs preparation of the existing code in
> builtin/bisect--helper.c, and I would highly suggest to split that change
> out into its own patch. It makes for a nicer story, and it is *much*
> easier to review.
>
>> This commit reduces the number of failed tests in
>> t6030-bisect-porcelain.sh and t6041-bisect-submodule.sh
>
> Oh? Which tests are supposed to fail? I do not see any failing tests
> here...

What I meant by this is that before there were 55 out of 70 tests
which failed. After this patch, there are 3 tests out of 70 which
failed.

>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index 3324229..ab3891c 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> [...]
>> +static int check_term_format(const char *term, const char *revision, int 
>> flag) {
>> + if (check_refname_format(term, flag))
>> + die("'%s' is not a valid term", term);
>> +
>> + if (one_of(term, "help", "start", "skip", "next", "reset", "visualize",
>> + "replay", "log", "run", NULL))
>
> If I understood Junio correctly, he meant to line up the second line with
> the corresponding level. In this case, as "replay" is a parameter of the
> one_of() function, the indentation would look like this instead:

I misunderstood his point.

> if (one_of(term, "help", "start", "skip", "next", "reset", 
> "visualize",
>"replay", "log", "run", NULL))
>
>> + die("can't use the builtin command '%s' as a term", term);
>> +
>> + /* In theory, nothing prevents swapping
>> +  * completely good and bad, but this situation
>> +  * could be confusing and hasn't been tested
>> +  * enough. Forbid it for now.
>> +  */
>
> I see quite a few comments that put the closing "*/" on its own line, but
> do not award the same pleasure to the opening "/*". Personally, I much
> prefer the opening "/*" to have an entire line to itself, too, but I guess
> that there is enough precendence in Git's source code to accept both
> forms.
>
>> + if (!strcmp(term, "bad") || !strcmp(term, "new"))
>> + if (strcmp(revision, "bad"))
>> + die("can't change the meaning of term '%s'", term);
>> +
>> + if(!strcmp(term, "good") || !strcmp(term, "old"))
>> + if (strcmp(revision, "good"))
>> + die("can't change the meaning of term '%s'", term);
>
> I am still convinced that
>
> if ((one_of(term, "bad", "new", NULL) && strcmp(orig_term, "bad")) ||
> (one_of(term, "good", "old", NULL) && strcmp(orig_term, "good")))
> die("can't change the meaning of term '%s'", term);
>
> looks so much nicer.

True. I missed this point.

>>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>>  {
>> - int next_all = 0;
>> + int sub_command = 0;
>>   int no_checkout = 0;
>> +
>> + enum sub_commands {
>> + NEXT_ALL,
>> + CHECK_TERM_FMT
>> + };
>
> Interesting. I did not think that Git's source code declares enums inside
> functions, but builtin/remote.c's config_read_branches() does, so this
> code is fine.

I didn't notice this before. Since git has the convention of declaring
enums outside function, it will be better to follow the trend rather
than allowing another trend to spread.

> Still, the patch would be easier to review (corollary: bugs would have a
> harder time to hide) if the change from OPT_BOOL to OPT_CMDMODE was done
> in a separate, preparatory patch.

I was confused about this. Now that u mention it, I will make the change.

>>   argc = parse_options(argc, argv, prefix, options,
>>

Re: [PATCH v3] bisect--helper: convert a function in shell to C

Hi Pranit,

On Wed, 23 Mar 2016, Pranit Bauva wrote:

> Convert the code literally without changing its design even though it
> seems that it is obscure as to the use of comparing revision to different
> bisect arguments which seems like a problem in shell because of the way
> function arguments are handled.

I still believe that it would make for an improvement to replace
"revision" by "orig_term".

> The argument handling is kind of hard coded right now because it is not
> really be meant to be used like this and this is just for testing
> purposes whether this new method is as functional as its counter part.
> The shell counter part of the method has been retained for historical
> purposes.

Well, maybe the argument handling is really meant to be used like this in
the end? Just skip that paragraph.

> Also using OPT_CMDMODE() to handle check-term-format and next-all
> because these sub commands are independent and error should be shown if
> used together and should be handled independently.

As is often the case, after some discussion a single patch becomes more
than just one change. This is what we see here, too: OPT_CMDMODE() is a
change that needs preparation of the existing code in
builtin/bisect--helper.c, and I would highly suggest to split that change
out into its own patch. It makes for a nicer story, and it is *much*
easier to review.

> This commit reduces the number of failed tests in
> t6030-bisect-porcelain.sh and t6041-bisect-submodule.sh

Oh? Which tests are supposed to fail? I do not see any failing tests
here...

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 3324229..ab3891c 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> [...]
> +static int check_term_format(const char *term, const char *revision, int 
> flag) {
> + if (check_refname_format(term, flag))
> + die("'%s' is not a valid term", term);
> +
> + if (one_of(term, "help", "start", "skip", "next", "reset", "visualize",
> + "replay", "log", "run", NULL))

If I understood Junio correctly, he meant to line up the second line with
the corresponding level. In this case, as "replay" is a parameter of the
one_of() function, the indentation would look like this instead:

if (one_of(term, "help", "start", "skip", "next", "reset", "visualize",
   "replay", "log", "run", NULL))

> + die("can't use the builtin command '%s' as a term", term);
> +
> + /* In theory, nothing prevents swapping
> +  * completely good and bad, but this situation
> +  * could be confusing and hasn't been tested
> +  * enough. Forbid it for now.
> +  */

I see quite a few comments that put the closing "*/" on its own line, but
do not award the same pleasure to the opening "/*". Personally, I much
prefer the opening "/*" to have an entire line to itself, too, but I guess
that there is enough precendence in Git's source code to accept both
forms.

> + if (!strcmp(term, "bad") || !strcmp(term, "new"))
> + if (strcmp(revision, "bad"))
> + die("can't change the meaning of term '%s'", term);
> +
> + if(!strcmp(term, "good") || !strcmp(term, "old"))
> + if (strcmp(revision, "good"))
> + die("can't change the meaning of term '%s'", term);

I am still convinced that

if ((one_of(term, "bad", "new", NULL) && strcmp(orig_term, "bad")) ||
(one_of(term, "good", "old", NULL) && strcmp(orig_term, "good")))
die("can't change the meaning of term '%s'", term);

looks so much nicer.

>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
> - int next_all = 0;
> + int sub_command = 0;
>   int no_checkout = 0;
> +
> + enum sub_commands {
> + NEXT_ALL,
> + CHECK_TERM_FMT
> + };

Interesting. I did not think that Git's source code declares enums inside
functions, but builtin/remote.c's config_read_branches() does, so this
code is fine.

Still, the patch would be easier to review (corollary: bugs would have a
harder time to hide) if the change from OPT_BOOL to OPT_CMDMODE was done
in a separate, preparatory patch.

>   argc = parse_options(argc, argv, prefix, options,
>git_bisect_helper_usage, 0);
>  
> - if (!next_all)
> + if (sub_command == CHECK_TERM_FMT) {
> + if (argc == 2) {
> + if (argv[0] != NULL && argv[1] != NULL)
> + return check_term_format(argv[0], argv[1], 0);
> + else
> + die("no revision or term provided with 
> check_for_term");
> + }
> + else
> + die("--check-term-format expects 2 arguments");
> + }
> +
> + if (sub_command != NEXT_ALL && sub_command != CHECK_TERM_FMT)
>   usage_with_options(git_bisect_helper_usage, options);

Personally, I would prefer

Re: [PATCH v2] bisect--helper: convert a function in shell to C

Hi,

On Tue, 22 Mar 2016, Stefan Beller wrote:

> On Tue, Mar 22, 2016 at 10:52 AM, Pranit Bauva  wrote:
> > OPT_CMDMODE() is actually a better option. I also noticed that it
> > isn't mentioned in Documentation/technical/api-parse-options.txt .
> > Should I send a patch to include it there just to make it easier for
> > someone who is new and isn't aware of the changes ?
> 
> Patches to outdated documentation are most awesome. ;)

Yes!

Thanks,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] bisect--helper: convert a function in shell to C

Hi Pranit,

On Tue, 22 Mar 2016, Pranit Bauva wrote:

> On Tue, Mar 22, 2016 at 8:41 PM, Johannes Schindelin
>  wrote:
> >
> > On Tue, 22 Mar 2016, Johannes Schindelin wrote:
> >
> >> On Tue, 22 Mar 2016, Pranit Bauva wrote:
> >>
> >> > +   if (!strcmp(term, "bad") || !strcmp(term, "new"))
> >> > +   if(strcmp(revision, "bad"))
> >> > +   die("can't change the meaning of term '%s'", term);
> >> > +
> >> > +   if (!strcmp(term, "good") || !strcmp(term, "old"))
> >> > +   if (strcmp(revision, "good"))
> >> > +   die("can't change the meaning of term '%s'", term);
> >>
> >> These two can be combined. Actually, these *four* can easily be combined:
> >>
> >>   if ((one_of(term, "bad", "new", NULL) && strcmp(orig, "bad")) ||
> >>   (one_of(term, "good", "old", NULL) && strcmp(orig, "good")))
> >>   die("can't change the meaning of term '%s'", term);
> >
> > Completely forgot to mention: This conversion skipped the comment
> >
> > # In theory, nothing prevents swapping
> > # completely good and bad, but this situation
> > # could be confusing and hasn't been tested
> > # enough. Forbid it for now.
> >
> > Let's port that comment over, too?
> 
> Sure! Adding a comment won't harm anyone. We can remove it when its
> thoroughly tested.

I am actually not so eager to remove the comment...

Ciao,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] bisect--helper: convert a function in shell to C

Hi Pranit,

On Tue, 22 Mar 2016, Pranit Bauva wrote:

> I did run the tests. They produce the same results as they did before.
> To ease review I will next time include these the output of the tests
> in the commented section.
> 
> t6002-rev-list-bisect.sh : http://paste.ubuntu.com/15473728/
> t6030-bisect-porcelain.sh : http://paste.ubuntu.com/15473734/
> t6041-bisect-submodule.sh : http://paste.ubuntu.com/15473743/
> 
> Is there any other test I would need to run?

Oh, I just meant to point out that you need to make sure that the entire
test suite passes after your patch series (and ideally, after every patch,
that is at least what I frequently test before sending out patch series).

There is not really a need to mention that you ran the test suite. There
is only a need to run it ;-)

Ciao,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] t1300: fix the new --show-origin tests on Windows

2016-03-23 Thread Lars Schneider


On 23 Mar 2016, at 11:55, Johannes Schindelin  
wrote:

> On Windows, we have that funny situation where the test script can refer
> to POSIX paths because it runs in a shell that uses a POSIX emulation
> layer ("MSYS2 runtime"). Yet, git.exe does *not* understand POSIX paths
> at all but only pure Windows paths.
> 
> So let's just convert the POSIX paths to Windows paths before passing
> them on to Git, using `pwd` (which is already modified on Windows to
> output Windows paths).
> 
> While fixing the new tests on Windows, we also have to exclude the tests
> that want to write a file with a name that is illegal on Windows
> (unfortunately, there is more than one test trying to make use of that
> file).

Thanks for these Windows fixes! After the 2.8 release I will try to post 
a patch that uses a different filename where possible.

Cheers,
Lars

> 
> Signed-off-by: Johannes Schindelin 
> ---
> t/t1300-repo-config.sh | 10 +++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index dca27a3..a37ebb2 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -1205,6 +1205,9 @@ test_expect_success POSIXPERM,PERL 'preserves existing 
> permissions' '
> "die q(badrename) if ((stat(q(.git/config)))[2] & 0) != 0600"
> '
> 
> +! test_have_prereq MINGW ||
> +HOME="$(pwd)" # convert to Windows path
> +
> test_expect_success 'set up --show-origin tests' '
>   INCLUDE_DIR="$HOME/include" &&
>   mkdir -p "$INCLUDE_DIR" &&
> @@ -1232,6 +1235,7 @@ test_expect_success 'set up --show-origin tests' '
>   EOF
> '
> 
> +
> test_expect_success '--show-origin with --list' '
>   cat >expect <<-EOF &&
>   file:$HOME/.gitconfig   user.global=true
> @@ -1304,7 +1308,7 @@ test_expect_success 'set up custom config file' '
>   EOF
> '
> 
> -test_expect_success '--show-origin escape special file name characters' '
> +test_expect_success !MINGW '--show-origin escape special file name 
> characters' '
>   cat >expect <<-\EOF &&
>   file:"file\" (dq) and spaces.conf"  user.custom=true
>   EOF
> @@ -1333,7 +1337,7 @@ test_expect_success '--show-origin stdin with file 
> include' '
>   test_cmp expect output
> '
> 
> -test_expect_success '--show-origin blob' '
> +test_expect_success !MINGW '--show-origin blob' '
>   cat >expect <<-\EOF &&
>   blob:a9d9f9e555b5c6f07cbe09d3f06fe3df11e09c08   user.custom=true
>   EOF
> @@ -1342,7 +1346,7 @@ test_expect_success '--show-origin blob' '
>   test_cmp expect output
> '
> 
> -test_expect_success '--show-origin blob ref' '
> +test_expect_success !MINGW '--show-origin blob ref' '
>   cat >expect <<-\EOF &&
>   blob:"master:file\" (dq) and spaces.conf"   user.custom=true
>   EOF
> -- 
> 2.7.4.windows.1
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/2] Documentation: fix git-p4 AsciiDoc formatting

2016-03-23 Thread larsxschneider

From: Lars Schneider 

Noticed-by: Eric Sunshine 
Signed-off-by: Lars Schneider 
---
 Documentation/git-p4.txt | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 738cfde..6efe830 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -520,15 +520,13 @@ git-p4.pathEncoding::
 git-p4.largeFileSystem::
Specify the system that is used for large (binary) files. Please note
that large file systems do not support the 'git p4 submit' command.
-   Only Git LFS [1] is implemented right now. Download
-   and install the Git LFS command line extension to use this option
-   and configure it like this:
+   Only Git LFS is implemented right now (see https://git-lfs.github.com/
+   for more information). Download and install the Git LFS command line
+   extension to use this option and configure it like this:
 +
 -
 git config   git-p4.largeFileSystem GitLFS
 -
-+
-   [1] https://git-lfs.github.com/
 
 git-p4.largeFileExtensions::
All files matching a file extension in the list will be processed
-- 
2.5.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/2] Documentation: use ASCII quotation marks in git-p4

2016-03-23 Thread larsxschneider

From: Lars Schneider 

Signed-off-by: Lars Schneider 
---
 Documentation/git-p4.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 6efe830..35e3170 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -515,7 +515,7 @@ git-p4.pathEncoding::
Git expects paths encoded as UTF-8. Use this config to tell git-p4
what encoding Perforce had used for the paths. This encoding is used
to transcode the paths to UTF-8. As an example, Perforce on Windows
-   often uses “cp1252” to encode path names.
+   often uses "cp1252" to encode path names.
 
 git-p4.largeFileSystem::
Specify the system that is used for large (binary) files. Please note
-- 
2.5.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 0/2] git-p4: fix AsciiDoc formatting

2016-03-23 Thread larsxschneider

From: Lars Schneider 

diff to v2:
* mimick git-commit instead of git-bisect-lk2009 for references

Thanks,
Lars

Lars Schneider (2):
  Documentation: fix git-p4 AsciiDoc formatting
  Documentation: use ASCII quotation marks in git-p4

 Documentation/git-p4.txt | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

--
2.5.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/4] config --show-origin: report paths with forward slashes

On Windows, the backslash is the native directory separator, but all
supported Windows versions also accept the forward slash in most
circumstances.

Our tests expect forward slashes.

Relative paths are generated by Git using forward slashes.

So let's try to be consistent and use forward slashes in the $HOME part
of the paths reported by `git config --show-origin`, too.

Signed-off-by: Johannes Schindelin 
---
 compat/mingw.h | 6 ++
 path.c | 3 +++
 2 files changed, 9 insertions(+)

diff --git a/compat/mingw.h b/compat/mingw.h
index 8c5bf50..c008694 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -396,6 +396,12 @@ static inline char *mingw_find_last_dir_sep(const char 
*path)
ret = (char *)path;
return ret;
 }
+static inline void convert_slashes(char *path)
+{
+   for (; *path; path++)
+   if (*path == '\\')
+   *path = '/';
+}
 #define find_last_dir_sep mingw_find_last_dir_sep
 int mingw_offset_1st_component(const char *path);
 #define offset_1st_component mingw_offset_1st_component
diff --git a/path.c b/path.c
index 8b7e168..969b494 100644
--- a/path.c
+++ b/path.c
@@ -584,6 +584,9 @@ char *expand_user_path(const char *path)
if (!home)
goto return_null;
strbuf_addstr(_path, home);
+#ifdef GIT_WINDOWS_NATIVE
+   convert_slashes(user_path.buf);
+#endif
} else {
struct passwd *pw = getpw_str(username, username_len);
if (!pw)
-- 
2.7.4.windows.1


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/4] t1300: fix the new --show-origin tests on Windows

On Windows, we have that funny situation where the test script can refer
to POSIX paths because it runs in a shell that uses a POSIX emulation
layer ("MSYS2 runtime"). Yet, git.exe does *not* understand POSIX paths
at all but only pure Windows paths.

So let's just convert the POSIX paths to Windows paths before passing
them on to Git, using `pwd` (which is already modified on Windows to
output Windows paths).

While fixing the new tests on Windows, we also have to exclude the tests
that want to write a file with a name that is illegal on Windows
(unfortunately, there is more than one test trying to make use of that
file).

Signed-off-by: Johannes Schindelin 
---
 t/t1300-repo-config.sh | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index dca27a3..a37ebb2 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1205,6 +1205,9 @@ test_expect_success POSIXPERM,PERL 'preserves existing 
permissions' '
  "die q(badrename) if ((stat(q(.git/config)))[2] & 0) != 0600"
 '
 
+! test_have_prereq MINGW ||
+HOME="$(pwd)" # convert to Windows path
+
 test_expect_success 'set up --show-origin tests' '
INCLUDE_DIR="$HOME/include" &&
mkdir -p "$INCLUDE_DIR" &&
@@ -1232,6 +1235,7 @@ test_expect_success 'set up --show-origin tests' '
EOF
 '
 
+
 test_expect_success '--show-origin with --list' '
cat >expect <<-EOF &&
file:$HOME/.gitconfig   user.global=true
@@ -1304,7 +1308,7 @@ test_expect_success 'set up custom config file' '
EOF
 '
 
-test_expect_success '--show-origin escape special file name characters' '
+test_expect_success !MINGW '--show-origin escape special file name characters' 
'
cat >expect <<-\EOF &&
file:"file\" (dq) and spaces.conf"  user.custom=true
EOF
@@ -1333,7 +1337,7 @@ test_expect_success '--show-origin stdin with file 
include' '
test_cmp expect output
 '
 
-test_expect_success '--show-origin blob' '
+test_expect_success !MINGW '--show-origin blob' '
cat >expect <<-\EOF &&
blob:a9d9f9e555b5c6f07cbe09d3f06fe3df11e09c08   user.custom=true
EOF
@@ -1342,7 +1346,7 @@ test_expect_success '--show-origin blob' '
test_cmp expect output
 '
 
-test_expect_success '--show-origin blob ref' '
+test_expect_success !MINGW '--show-origin blob ref' '
cat >expect <<-\EOF &&
blob:"master:file\" (dq) and spaces.conf"   user.custom=true
EOF
-- 
2.7.4.windows.1


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 4/4] mingw: skip some tests in t9115 due to file name issues

These two tests wanted to write file names which are incompatible with
Windows' file naming rules (even if they pass using Cygwin due to
Cygwin's magic path mangling).

While at it, skip the same tests also on MacOSX/HFS, as pointed out by
Torsten Bögershausen.

Signed-off-by: Johannes Schindelin 
Signed-off-by: Torsten Bögershausen 
Reviewed-by: Jonathan Nieder 
Signed-off-by: Junio C Hamano 
---
 t/t9115-git-svn-dcommit-funky-renames.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t9115-git-svn-dcommit-funky-renames.sh 
b/t/t9115-git-svn-dcommit-funky-renames.sh
index 0990f8d..a87d3d3 100755
--- a/t/t9115-git-svn-dcommit-funky-renames.sh
+++ b/t/t9115-git-svn-dcommit-funky-renames.sh
@@ -93,7 +93,7 @@ test_expect_success 'git svn rebase works inside a 
fresh-cloned repository' '
 # > to special UNICODE characters in the range 0xf000 to 0xf0ff (the
 # > "Private use area") when creating or accessing files.
 prepare_a_utf8_locale
-test_expect_success UTF8 'svn.pathnameencoding=cp932 new file on dcommit' '
+test_expect_success UTF8,!MINGW,!UTF8_NFD_TO_NFC 'svn.pathnameencoding=cp932 
new file on dcommit' '
LC_ALL=$a_utf8_locale &&
export LC_ALL &&
neq=$(printf "\201\202") &&
@@ -105,7 +105,7 @@ test_expect_success UTF8 'svn.pathnameencoding=cp932 new 
file on dcommit' '
 '
 
 # See the comment on the above test for setting of LC_ALL.
-test_expect_success 'svn.pathnameencoding=cp932 rename on dcommit' '
+test_expect_success !MINGW,!UTF8_NFD_TO_NFC 'svn.pathnameencoding=cp932 rename 
on dcommit' '
LC_ALL=$a_utf8_locale &&
export LC_ALL &&
inf=$(printf "\201\207") &&
-- 
2.7.4.windows.1

[PATCH v2 2/4] Make t1300-repo-config resilient to being run via 'sh -x'

One of this developer's primary tools to diagnose broken regression
tests is to run the test script using 'sh -x t... -i -v' to find out
*which* call *actually* demonstrates the symptom.

Hence it is pretty counterproductive if the test script behaves
differently when being run via 'sh -x', in particular when using
test_cmp or test_i18ncmp on redirected stderr.

So let's use test_i18ngrep (as suggested by Jonathan Nieder) instead of
test_cmp/test_i18ncmp to verify that stderr looks as expected.

Signed-off-by: Johannes Schindelin 
---
 t/t1300-repo-config.sh | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 8867ce1..dca27a3 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -699,17 +699,13 @@ test_expect_success 'invalid unit' '
echo 1auto >expect &&
git config aninvalid.unit >actual &&
test_cmp expect actual &&
-   cat >expect <<-\EOF &&
-   fatal: bad numeric config value '\''1auto'\'' for 
'\''aninvalid.unit'\'' in file .git/config: invalid unit
-   EOF
test_must_fail git config --int --get aninvalid.unit 2>actual &&
-   test_i18ncmp expect actual
+   test_i18ngrep "bad numeric config value .1auto. for .aninvalid.unit. in 
file .git/config: invalid unit" actual
 '
 
 test_expect_success 'invalid stdin config' '
-   echo "fatal: bad config line 1 in standard input " >expect &&
echo "[broken" | test_must_fail git config --list --file - >output 2>&1 
&&
-   test_cmp expect output
+   test_i18ngrep "bad config line 1 in standard input" output
 '
 
 cat > expect << EOF
-- 
2.7.4.windows.1


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 0/4] Git for Windows fixes in preparation for 2.8.0

The t1300 and t9115 tests regressed on Windows. These patches fix that.


Johannes Schindelin (4):
  config --show-origin: report paths with forward slashes
  Make t1300-repo-config resilient to being run via 'sh -x'
  t1300: fix the new --show-origin tests on Windows
  mingw: skip some tests in t9115 due to file name issues

 compat/mingw.h   |  6 ++
 path.c   |  3 +++
 t/t1300-repo-config.sh   | 18 +-
 t/t9115-git-svn-dcommit-funky-renames.sh |  4 ++--
 4 files changed, 20 insertions(+), 11 deletions(-)

Interdiff vs v1:

 diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
 index 18eb769..a37ebb2 100755
 --- a/t/t1300-repo-config.sh
 +++ b/t/t1300-repo-config.sh
 @@ -700,12 +700,12 @@ test_expect_success 'invalid unit' '
git config aninvalid.unit >actual &&
test_cmp expect actual &&
test_must_fail git config --int --get aninvalid.unit 2>actual &&
 -  grep "^fatal: bad numeric config value .1auto. for .aninvalid.unit. in 
file .git/config: invalid unit$" actual
 +  test_i18ngrep "bad numeric config value .1auto. for .aninvalid.unit. in 
file .git/config: invalid unit" actual
  '
  
  test_expect_success 'invalid stdin config' '
echo "[broken" | test_must_fail git config --list --file - >output 2>&1 
&&
 -  grep "^fatal: bad config line 1 in standard input $" output
 +  test_i18ngrep "bad config line 1 in standard input" output
  '
  
  cat > expect << EOF
 @@ -1205,6 +1205,9 @@ test_expect_success POSIXPERM,PERL 'preserves existing 
permissions' '
  "die q(badrename) if ((stat(q(.git/config)))[2] & 0) != 0600"
  '
  
 +! test_have_prereq MINGW ||
 +HOME="$(pwd)" # convert to Windows path
 +
  test_expect_success 'set up --show-origin tests' '
INCLUDE_DIR="$HOME/include" &&
mkdir -p "$INCLUDE_DIR" &&
 @@ -1232,14 +1235,6 @@ test_expect_success 'set up --show-origin tests' '
EOF
  '
  
 -if test_have_prereq MINGW
 -then
 -  # convert to Windows paths
 -  HOME="$(pwd)"
 -  INCLUDE_DIR="$HOME/include"
 -  export HOME INCLUDE_DIR
 -  git config -f .gitconfig include.path "$INCLUDE_DIR/absolute.include"
 -fi
  
  test_expect_success '--show-origin with --list' '
cat >expect <<-EOF &&
 diff --git a/t/t9115-git-svn-dcommit-funky-renames.sh 
b/t/t9115-git-svn-dcommit-funky-renames.sh
 index 864395e..a87d3d3 100755
 --- a/t/t9115-git-svn-dcommit-funky-renames.sh
 +++ b/t/t9115-git-svn-dcommit-funky-renames.sh
 @@ -93,7 +93,7 @@ test_expect_success 'git svn rebase works inside a 
fresh-cloned repository' '
  # > to special UNICODE characters in the range 0xf000 to 0xf0ff (the
  # > "Private use area") when creating or accessing files.
  prepare_a_utf8_locale
 -test_expect_success UTF8,!MINGW 'svn.pathnameencoding=cp932 new file on 
dcommit' '
 +test_expect_success UTF8,!MINGW,!UTF8_NFD_TO_NFC 'svn.pathnameencoding=cp932 
new file on dcommit' '
LC_ALL=$a_utf8_locale &&
export LC_ALL &&
neq=$(printf "\201\202") &&
 @@ -105,7 +105,7 @@ test_expect_success UTF8,!MINGW 
'svn.pathnameencoding=cp932 new file on dcommit'
  '
  
  # See the comment on the above test for setting of LC_ALL.
 -test_expect_success !MINGW 'svn.pathnameencoding=cp932 rename on dcommit' '
 +test_expect_success !MINGW,!UTF8_NFD_TO_NFC 'svn.pathnameencoding=cp932 
rename on dcommit' '
LC_ALL=$a_utf8_locale &&
export LC_ALL &&
inf=$(printf "\201\207") &&

-- 
2.7.4.windows.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] mingw: skip some tests in t9115 due to file name issues

Hi Junio & Torsten,

On Wed, 23 Mar 2016, Torsten Bögershausen wrote:

> On 2016-03-22 23.57, Junio C Hamano wrote:
> > Dscho, I queued two out of these four, with a proposed fix-up patch
> > for each of them, on 'pu'; but I won't squash them together myself
> > without hearing from you as I do not test mingw or macosx.

The suggested changes will be squashed into v2.

> Beside that, do we want to amend the commit message like this:
> 
> Author: Johannes Schindelin 
> Date:   Tue Mar 22 18:43:00 2016 +0100
> 
> skip some tests in t9115 due to file name issues
> 
> These two tests wanted to write file names which work under Linux or
> CYGWIN, but are incompatible with file naming rules under mingw or HFS.
> 
> Signed-off-by: Johannes Schindelin 
> Signed-off-by: Torsten Bögershausen 
> Reviewed-by: Jonathan Nieder 
> Signed-off-by: Junio C Hamano 

Thanks, I used a slightly different version, as I had crafted it before
reading this mail already.

Ciao,
Dscho

Re: [PATCH 3/4] t1300: fix the new --show-origin tests on Windows

Hi Junio,

On Tue, 22 Mar 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > On Windows, we have that funny situation where the test script can refer
> > to POSIX paths because it runs in a shell that uses a POSIX emulation
> > layer ("MSYS2 runtime"). Yet, git.exe does *not* understand POSIX paths
> > at all but only pure Windows paths.
> >
> > So let's just convert the POSIX paths to Windows paths before passing
> > them on to Git, using `pwd` (which is already modified on Windows to
> > output Windows paths).
> >
> > While fixing the new tests on Windows, we also have to exclude the tests
> > that want to write a file with a name that is illegal on Windows
> > (unfortunately, there is more than one test trying to make use of that
> > file).
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  t/t1300-repo-config.sh | 15 ---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> > index 0236fe2..18eb769 100755
> > --- a/t/t1300-repo-config.sh
> > +++ b/t/t1300-repo-config.sh
> > @@ -1232,6 +1232,15 @@ test_expect_success 'set up --show-origin tests' '
> > EOF
> >  '
> >  
> > +if test_have_prereq MINGW
> > +then
> > +   # convert to Windows paths
> > +   HOME="$(pwd)"
> 
> We override HOME in t/test-lib.sh; shouldn't this be done there?

We override it with $PWD.

Remember, on Windows we have this funny situation where the shell,
Perl, and the Unix tools used in scripting, know about POSIX paths, but
little else. Most notably git.exe does *not* understand them [*1*].

The difference between $PWD and $(pwd) is, you guessed it, POSIX path vs
Windows path, respectively. And since *some* of our tests verify
shell/Perl scripts' correct behavior, we *want* $HOME to be a POSIX path,
at least some of the time.

> > +   INCLUDE_DIR="$HOME/include"
> 
> I am puzzled. 'set up --show-origin tests' do say INCLUDE_DIR is
> "$HOME/include" already, so why is this needed?
> 
> > +   export HOME INCLUDE_DIR
> 
> Existing tests use $INCLUDE_DIR (and $HOME) as shell variables just
> fine without exporting.  Why do these need to be exported only on
> MINGW?

Habit. The export is actually not needed at all, you are totally correct.

> > +   git config -f .gitconfig include.path "$INCLUDE_DIR/absolute.include"
> > +fi
> 
> Perhaps if you adjust HOME before 'set up --show-origin tests' test,
> most (or all) of the above become unnecessary?

It did not even occur to me, thanks for that suggestion. It works
perfectly. Will send out v2 in a moment.

Ciao,
Dscho

Footnote [*1*]: we do have this hack, system_path(), that can turn "POSIX
paths" into Windows paths. However, it actually turns paths relative to
the prefix (as in "/usr/") into Windows paths, and the prefix is
determined at runtime, from the location of git.exe. When the test suite
runs, the location of git.exe is most definitely *not* related to any
sensible prefix, therefore we simply cannot expect git.exe to handle POSIX
paths correctly in the test suite.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 04/33] files-backend: break out ref reading

On 03/01/2016 01:52 AM, David Turner wrote:
> Refactor resolve_ref_1 in terms of a new function read_raw_ref, which
> is responsible for reading ref data from the ref storage.
> 
> Later, we will make read_raw_ref a pluggable backend function, and make
> resolve_ref_unsafe common.
> 
> Testing done: Hacked in code to run both old and new version of
> resolve_ref_1 and compare all outputs, failing dramatically if outputs
> differed.  Ran test suite.

I spent an inordinate amount of time trying to review this patch. It is
much too big and does too much at the same time. And, in fact, it makes
some mistakes, which were impossible to see until I picked the patch
apart into smaller steps.

The reference-reading code before this patch wasn't structured very
well. Part of the reason that the code is a mess is that it is pretty
intricate and has to get things just right to avoid race conditions. But
the rest of the reason is that it was overdue for a refactoring, and
this patch shows a great way forward.

Because of the intricacy of this code, it is really important to do a
careful job changing it. To me that means refactoring in the smallest
possible steps, ideally so that each step is obviously correct.

So to review your patch, I picked it apart into tiny preparatory
refactorings, followed by the main patch (the extraction of the function
read_raw_ref()), followed by some more cleanups. When I did so I found
that there were some differences between my end product and yours. Some
of these introduce minor bugs, so it is worth fixing them.

I've annotated your patch below, but in my opinion a better way forward
would be to commit not this single giant patch, but rather the
picked-apart version, which also addresses my comments below. I just
submitted that patch series [1]. It's twenty-one patches(!), though a
bit over half of them do things that go beyond this patch. In the
future, it would help the review process if you would submit smaller
patches that do a single thing at a time.

I've reviewed the patches that precede this one and they look fine to
me. I haven't reviewed this version of patches 05 through 33 yet.

Michael

[1] http://mid.gmane.org/cover.1458723959.git.mhag...@alum.mit.edu

> Signed-off-by: David Turner 
> Helped-by: Duy Nguyen 
> Signed-off-by: Junio C Hamano 
> ---
>  refs/files-backend.c | 265 
> ++-
>  1 file changed, 159 insertions(+), 106 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 9676ec2..8c6a58e 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1369,12 +1369,11 @@ static struct ref_entry *get_packed_ref(const char 
> *refname)
>  
>  /*
>   * A loose ref file doesn't exist; check for a packed ref.  The
> - * options are forwarded from resolve_safe_unsafe().
> + * options are forwarded from resolve_ref_unsafe().
>   */
>  static int resolve_missing_loose_ref(const char *refname,
> -  int resolve_flags,
>unsigned char *sha1,
> -  int *flags)
> +  unsigned int *flags)

This patch changes some flags variables from "int" to "unsigned int"
without discussion. I think the change is OK, but why not do it in a
separate patch?

>  {
>   struct ref_entry *entry;
>  
> @@ -1390,64 +1389,48 @@ static int resolve_missing_loose_ref(const char 
> *refname,
>   return 0;
>   }
>   /* The reference is not a packed reference, either. */
> - if (resolve_flags & RESOLVE_REF_READING) {
> - errno = ENOENT;
> - return -1;
> - } else {
> - hashclr(sha1);
> - return 0;
> - }
> + errno = ENOENT;
> + return -1;
>  }
>  
> -/* This function needs to return a meaningful errno on failure */
> -static const char *resolve_ref_1(const char *refname,
> -  int resolve_flags,
> -  unsigned char *sha1,
> -  int *flags,
> -  struct strbuf *sb_refname,
> -  struct strbuf *sb_path,
> -  struct strbuf *sb_contents)
> +/*
> + * Read a raw ref from the filesystem or packed refs file.
> + *
> + * If the ref is a sha1, fill in sha1 and return 0.
> + *
> + * If the ref is symbolic, fill in *symref with the referrent
> + * (e.g. "refs/heads/master") and return 0.  The caller is responsible
> + * for validating the referrent.  Set REF_ISSYMREF in flags.
> + *
> + * If the ref is neither a symbolic ref nor a sha1, it is broken.  Set
> + * REF_ISBROKEN in flags, set errno to EINVAL, and return -1.
> + *
> + * If the ref doesn't exist, set errno to ENOENT and return -1.
> + *
> + * If there is another error reading the ref, set errno appropriately and
> + * return -1.
> + *
> + *

Re: git-apply does not work in a sub-directory of a Git repository

2016-03-23 Thread Duy Nguyen

On Wed, Mar 23, 2016 at 5:14 AM, Stefan Beller  wrote:
>> Hello everyone,
>> As you observed, patch wasn't applied. Is it intended behaviour of
>> git-apply? Usually to apply the patch I have to copy it to top directory
>> and then use git-apply.
>>
>> I tried out git-am to apply the patch ("git format-patch" was used to
>> make patch) while being in the "outgoing" sub-directory and it worked
>> fine. So why does git-apply show this kind of behaviour?
>
>
> Think of git-apply as a specialized version of 'patch', which would also
> error out if there are path issues. (Inside outgoing/ there is no file found 
> at
> ./main)
>
> git-am is the porcelain command which is what is recommended to users
> who interact with Git and patches.

git-am is about patches in mailbox form, not plain patches, isn't it?
In that view, it's not a replacement for git-apply.

How about we start deprecating the old behavior?

1) add --no-index to force git-apply ignore .git, --git (or some other
name) to apply patches as if running from topdir, add a config key to
choose default behavior
2) when git-apply is run without --git, --index or --cached from a
subdir and the said config key is not set, start warning and
recommending --no-index
3) wait X years
4)switch default behavior to --git (if run inside a git repo)
-- 
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

[PATCH v3/GSoC 5/5] t0301: test credential-cache support of XDG_RUNTIME_DIR

t0301 now tests git-credential-cache support for XDG user-specific
runtime path. Specifically:

* if $XDG_RUNTIME_DIR exists, use socket at
  `$XDG_RUNTIME_DIR/git/credential-cache.sock`.

* otherwise, `/tmp/git-$uid/credential-cache.sock` is taken.

Helped-by: Jeff King 
Signed-off-by: Hui Yiqun 
---
 t/t0301-credential-cache.sh | 29 +
 1 file changed, 29 insertions(+)

diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index 82c8411..264c394 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh
@@ -12,7 +12,36 @@ test -z "$NO_UNIX_SOCKETS" || {
 # don't leave a stale daemon running
 trap 'code=$?; git credential-cache exit; (exit $code); die' EXIT
 
+test_expect_success 'set $XDG_RUNTIME_DIR' '
+   XDG_RUNTIME_DIR=$HOME/xdg_runtime/ &&
+   export XDG_RUNTIME_DIR
+'
+
+helper_test cache
+
+test_expect_success 'when $XDG_RUNTIME_DIR is set, `$XDG_RUNTIME_DIR/git` are 
used' '
+   test_path_is_missing "/tmp/git-$(id -u)/credential-cache.sock" &&
+   test -S "$HOME/xdg_runtime/git/credential-cache.sock"
+'
+
+test_expect_success 'force git-credential-cache to exit so that socket 
disappear' '
+   git credential-cache exit &&
+   test_path_is_missing "$HOME/xdg_runtime/git/credential-cache.sock" &&
+   unset XDG_RUNTIME_DIR
+'
+
 helper_test cache
+
+test_expect_success 'when $XDG_RUNTIME_DIR is not set, `/tmp/git-$(id -u) is 
used' '
+   test_path_is_missing "$HOME/xdg_runtime/git/credential-cache.sock" &&
+   test -S "/tmp/git-$(id -u)/credential-cache.sock"
+'
+
+# TODO: if $XDG_RUNTIME_DIR/git/ exists, but has wrong permission and 
ownership,
+# `helper_test cache` must fail.
+
+# TODO: check whether `--socket` works
+
 helper_test_timeout cache --timeout=1
 
 # we can't rely on our "trap" above working after test_done,
-- 
2.7.4

--
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 v3/GSoC 3/5] git-credential-cache: put socket to xdg-compatible path

move .git-credential-cache/socket to xdg_runtime_dir("credential-cache.sock")

Signed-off-by: Hui Yiqun 
---
 credential-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/credential-cache.c b/credential-cache.c
index f4afdc6..40d838b 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -105,7 +105,7 @@ int main(int argc, const char **argv)
op = argv[0];
 
if (!socket_path)
-   socket_path = 
expand_user_path("~/.git-credential-cache/socket");
+   socket_path = xdg_runtime_dir("credential-cache.sock");
if (!socket_path)
die("unable to find a suitable socket path; use --socket");
 
-- 
2.7.4

--
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 08/21] resolve_ref_unsafe(): ensure flags is always set

If the caller passes flags==NULL, then set it to point at a local
scratch variable. This removes the need for a lot of "if (flags)" guards
in resolve_ref_1() and resolve_missing_loose_ref().

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 31 +--
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 101abba..067ce1c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1383,8 +1383,7 @@ static int resolve_missing_loose_ref(const char *refname,
entry = get_packed_ref(refname);
if (entry) {
hashcpy(sha1, entry->u.value.oid.hash);
-   if (flags)
-   *flags |= REF_ISPACKED;
+   *flags |= REF_ISPACKED;
return 0;
}
/* refname is not a packed reference. */
@@ -1403,12 +1402,10 @@ static const char *resolve_ref_1(const char *refname,
int bad_name = 0;
int symref_count;
 
-   if (flags)
-   *flags = 0;
+   *flags = 0;
 
if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
-   if (flags)
-   *flags |= REF_BAD_NAME;
+   *flags |= REF_BAD_NAME;
 
if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
!refname_is_safe(refname)) {
@@ -1458,8 +1455,7 @@ static const char *resolve_ref_1(const char *refname,
}
if (bad_name) {
hashclr(sha1);
-   if (flags)
-   *flags |= REF_ISBROKEN;
+   *flags |= REF_ISBROKEN;
}
return refname;
}
@@ -1478,8 +1474,7 @@ static const char *resolve_ref_1(const char *refname,
!check_refname_format(sb_contents->buf, 0)) {
strbuf_swap(sb_refname, sb_contents);
refname = sb_refname->buf;
-   if (flags)
-   *flags |= REF_ISSYMREF;
+   *flags |= REF_ISSYMREF;
if (resolve_flags & RESOLVE_REF_NO_RECURSE) {
hashclr(sha1);
return refname;
@@ -1526,20 +1521,17 @@ static const char *resolve_ref_1(const char *refname,
 */
if (get_sha1_hex(sb_contents->buf, sha1) ||
(sb_contents->buf[40] != '\0' && 
!isspace(sb_contents->buf[40]))) {
-   if (flags)
-   *flags |= REF_ISBROKEN;
+   *flags |= REF_ISBROKEN;
errno = EINVAL;
return NULL;
}
if (bad_name) {
hashclr(sha1);
-   if (flags)
-   *flags |= REF_ISBROKEN;
+   *flags |= REF_ISBROKEN;
}
return refname;
}
-   if (flags)
-   *flags |= REF_ISSYMREF;
+   *flags |= REF_ISSYMREF;
buf = sb_contents->buf + 4;
while (isspace(*buf))
buf++;
@@ -1551,8 +1543,7 @@ static const char *resolve_ref_1(const char *refname,
return refname;
}
if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
-   if (flags)
-   *flags |= REF_ISBROKEN;
+   *flags |= REF_ISBROKEN;
 
if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
!refname_is_safe(buf)) {
@@ -1573,8 +1564,12 @@ const char *resolve_ref_unsafe(const char *refname, int 
resolve_flags,
static struct strbuf sb_refname = STRBUF_INIT;
struct strbuf sb_contents = STRBUF_INIT;
struct strbuf sb_path = STRBUF_INIT;
+   int unused_flags;
const char *ret;
 
+   if (!flags)
+   flags = _flags;
+
ret = resolve_ref_1(refname, resolve_flags, sha1, flags,
_refname, _path, _contents);
strbuf_release(_path);
-- 
2.8.0.rc3

--
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 14/21] Inline resolve_ref_1() into resolve_ref_unsafe()

resolve_ref_unsafe() wasn't doing anything useful anymore.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 31 +--
 1 file changed, 9 insertions(+), 22 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index f752568..120b2dd 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1539,14 +1539,16 @@ out:
 }
 
 /* This function needs to return a meaningful errno on failure */
-static const char *resolve_ref_1(const char *refname,
-int resolve_flags,
-unsigned char *sha1,
-int *flags,
-struct strbuf *sb_refname)
+const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
+  unsigned char *sha1, int *flags)
 {
+   static struct strbuf sb_refname = STRBUF_INIT;
+   int unused_flags;
int symref_count;
 
+   if (!flags)
+   flags = _flags;
+
*flags = 0;
 
if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
@@ -1570,7 +1572,7 @@ static const char *resolve_ref_1(const char *refname,
for (symref_count = 0; symref_count < MAXDEPTH; symref_count++) {
int read_flags = 0;
 
-   if (read_raw_ref(refname, sha1, sb_refname, _flags)) {
+   if (read_raw_ref(refname, sha1, _refname, _flags)) {
*flags |= read_flags;
if (errno != ENOENT || (resolve_flags & 
RESOLVE_REF_READING))
return NULL;
@@ -1590,7 +1592,7 @@ static const char *resolve_ref_1(const char *refname,
return refname;
}
 
-   refname = sb_refname->buf;
+   refname = sb_refname.buf;
if (resolve_flags & RESOLVE_REF_NO_RECURSE) {
hashclr(sha1);
return refname;
@@ -1610,21 +1612,6 @@ static const char *resolve_ref_1(const char *refname,
return NULL;
 }
 
-const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
-  unsigned char *sha1, int *flags)
-{
-   static struct strbuf sb_refname = STRBUF_INIT;
-   int unused_flags;
-   const char *ret;
-
-   if (!flags)
-   flags = _flags;
-
-   ret = resolve_ref_1(refname, resolve_flags, sha1, flags,
-   _refname);
-   return ret;
-}
-
 /*
  * Peel the entry (if possible) and return its new peel_status.  If
  * repeel is true, re-peel the entry even if there is an old peeled
-- 
2.8.0.rc3

--
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 v3/GSoC 2/5] path.c: implement xdg_runtime_dir()

This function is aimed to provide an uniform location to put
runtime files according to the xdg base dir spec[1] and stop using
$HOME. On the other hand, the safety is considered(with directory
permission).

This function will use `$XDG_RUNTIME_DIR/git` if XDG_RUNTIME_DIR exists,
otherwise `/tmp/git-$uid`.

The existence and the permission of the directory is ensured. However,
if the directory or its parents cannot be created or the directory exists
but have wrong permission, this function will give a warning and return NULL
for security.

[1] https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html

Signed-off-by: Hui Yiqun 
---
 cache.h | 23 +++
 path.c  | 56 
 2 files changed, 79 insertions(+)

diff --git a/cache.h b/cache.h
index ef843c1..f8b649b 100644
--- a/cache.h
+++ b/cache.h
@@ -1001,6 +1001,29 @@ extern int is_ntfs_dotgit(const char *name);
  */
 extern char *xdg_config_home(const char *filename);
 
+/**
+ * this function does the following:
+ *
+ * 1. if $XDG_RUNTIME_DIR is non-empty, `$XDG_RUNTIME_DIR/git` is used in next
+ * step, otherwise `/tmp/git-$uid` is taken.
+ * 2. ensure that above directory does exist. what's more, it must has correct
+ * permission and ownership.
+ * 3. a newly allocated string consisting of the path of above directory and
+ * $filename is returned.
+ *
+ * Under following situation, NULL will be returned:
+ *
+ * + the directory mentioned in step 1 exists but have wrong permission or
+ * ownership.
+ * + the directory or its parent cannot be created.
+ *
+ * Notice:
+ *
+ * + the caller is responsible for deallocating the returned string.
+ *
+ */
+extern char *xdg_runtime_dir(const char *filename);
+
 /* object replacement */
 #define LOOKUP_REPLACE_OBJECT 1
 #define LOOKUP_UNKNOWN_OBJECT 2
diff --git a/path.c b/path.c
index 699af68..2886e59 100644
--- a/path.c
+++ b/path.c
@@ -5,6 +5,7 @@
 #include "strbuf.h"
 #include "string-list.h"
 #include "dir.h"
+#include "git-compat-util.h"
 
 static int get_st_mode_bits(const char *path, int *mode)
 {
@@ -1206,6 +1207,61 @@ char *xdg_config_home(const char *filename)
return NULL;
 }
 
+char *xdg_runtime_dir(const char *filename)
+{
+   struct strbuf sb = STRBUF_INIT;
+   char *runtime_dir;
+   struct stat st;
+   uid_t uid = getuid();
+
+   assert(filename);
+   runtime_dir = getenv("XDG_RUNTIME_DIR");
+   if (runtime_dir && *runtime_dir)
+   strbuf_mkpath(, "%s/git/", runtime_dir);
+   else
+   strbuf_mkpath(, "/tmp/git-%d", uid);
+
+   if (!lstat(sb.buf, )) {
+   /*
+* As described in XDG base dir spec[1], the subdirectory
+* under $XDG_RUNTIME_DIR or its fallback MUST be owned by
+* the user, and its unix access mode MUST be 0700.
+*
+* Calling chmod or chown silently may cause security
+* problem if somebody chdir to it, sleep, and then, try
+* to open our protected runtime cache or socket.
+* So we just put warning and left it to user to solve.
+*
+* [1]https://specifications.freedesktop.org/basedir-spec/
+* basedir-spec-latest.html
+*/
+   if ((st.st_mode & 0777) != S_IRWXU) {
+   warning("permission of runtime directory '%s' "
+   "MUST be 0700 instead of 0%o\n",
+   sb.buf, (st.st_mode & 0777));
+   return NULL;
+   } else if (st.st_uid != uid) {
+   warning("owner of runtime directory '%s' "
+   "MUST be %d instead of %d\n",
+   sb.buf, uid, st.st_uid);
+   return NULL;
+   }
+   /* TODO: check whether st.buf is an directory */
+   } else {
+   if (safe_create_leading_directories_const(sb.buf) < 0) {
+   warning("unable to create directories for '%s'\n",
+   sb.buf);
+   return NULL;
+   }
+   if (mkdir(sb.buf, 0700) < 0) {
+   warning("unable to mkdir '%s'\n", sb.buf);
+   return NULL;
+   }
+   }
+   strbuf_addf(, "/%s", filename);
+   return strbuf_detach(, NULL);
+}
+
 GIT_PATH_FUNC(git_path_cherry_pick_head, "CHERRY_PICK_HEAD")
 GIT_PATH_FUNC(git_path_revert_head, "REVERT_HEAD")
 GIT_PATH_FUNC(git_path_squash_msg, "SQUASH_MSG")
-- 
2.7.4

--
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 10/21] resolve_ref_1(): reorder code

There is no need to adjust *flags if we're just about to fail.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 69ec903..60f1493 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1542,13 +1542,13 @@ static const char *resolve_ref_1(const char *refname,
return refname;
}
if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
-   *flags |= REF_ISBROKEN;
-
if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
!refname_is_safe(refname)) {
errno = EINVAL;
return NULL;
}
+
+   *flags |= REF_ISBROKEN;
bad_name = 1;
}
}
-- 
2.8.0.rc3

--
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 11/21] resolve_ref_1(): eliminate local variable "bad_name"

We can use (*flags & REF_BAD_NAME) for that purpose.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 60f1493..b865ba5 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1399,19 +1399,17 @@ static const char *resolve_ref_1(const char *refname,
 struct strbuf *sb_path,
 struct strbuf *sb_contents)
 {
-   int bad_name = 0;
int symref_count;
 
*flags = 0;
 
if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
-   *flags |= REF_BAD_NAME;
-
if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
!refname_is_safe(refname)) {
errno = EINVAL;
return NULL;
}
+
/*
 * dwim_ref() uses REF_ISBROKEN to distinguish between
 * missing refs and refs that were present but invalid,
@@ -1420,7 +1418,7 @@ static const char *resolve_ref_1(const char *refname,
 * We don't know whether the ref exists, so don't set
 * REF_ISBROKEN yet.
 */
-   bad_name = 1;
+   *flags |= REF_BAD_NAME;
}
 
for (symref_count = 0; symref_count < MAXDEPTH; symref_count++) {
@@ -1452,7 +1450,7 @@ static const char *resolve_ref_1(const char *refname,
}
hashclr(sha1);
}
-   if (bad_name) {
+   if (*flags & REF_BAD_NAME) {
hashclr(sha1);
*flags |= REF_ISBROKEN;
}
@@ -1524,7 +1522,7 @@ static const char *resolve_ref_1(const char *refname,
errno = EINVAL;
return NULL;
}
-   if (bad_name) {
+   if (*flags & REF_BAD_NAME) {
hashclr(sha1);
*flags |= REF_ISBROKEN;
}
@@ -1548,8 +1546,7 @@ static const char *resolve_ref_1(const char *refname,
return NULL;
}
 
-   *flags |= REF_ISBROKEN;
-   bad_name = 1;
+   *flags |= REF_ISBROKEN | REF_BAD_NAME;
}
}
 
-- 
2.8.0.rc3

--
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 v3/GSoC 4/5] test-lib.sh: unset all environment variables defined in xdg base dir spec[1]

Otherwise, on environments where these variables and set, an assignment
to one of these variables will cause the variable being implicitly exported.

For example:

$ XDG_RUNTIME_DIR=/run/user/2000 bash
$ XDG_RUNTIME_DIR=/tmp/whatever # it should not be exported!
$ bash
$ echo $XDG_RUNTIME_DIR
/tmp/whatever # instead of empty

[1] https://specifications.freedesktop.org/basedir-spec
/basedir-spec-latest.html

Helped-by: Jeff King 
Signed-off-by: Hui Yiqun 
---
 t/test-lib.sh | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0b47eb6..60a837a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -93,7 +93,17 @@ unset VISUAL EMAIL LANGUAGE COLUMNS $("$PERL_PATH" -e '
my @vars = grep(/^GIT_/ && !/^GIT_($ok)/o, @env);
print join("\n", @vars);
 ')
+# Unset all environment variables defined in xdg base dir spec[1]
+# to make sure that the test are running with a known state.
+#
+# [1] https://specifications.freedesktop.org/basedir-spec
+# /basedir-spec-latest.html
+unset XDG_DATA_HOME
 unset XDG_CONFIG_HOME
+unset XDG_DATA_DIRS
+unset XDG_CONFIG_DIRS
+unset XDG_CACHE_HOME
+unset XDG_RUNTIME_DIR
 unset GITPERLLIB
 GIT_AUTHOR_EMAIL=aut...@example.com
 GIT_AUTHOR_NAME='A U Thor'
-- 
2.7.4

--
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 16/21] fsck_head_link(): remove unneeded flag variable

It is never read, so we can pass NULL to resolve_ref_unsafe().

Signed-off-by: Michael Haggerty 
---
 builtin/fsck.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 55eac75..3f27456 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -493,13 +493,12 @@ static void fsck_object_dir(const char *path)
 
 static int fsck_head_link(void)
 {
-   int flag;
int null_is_error = 0;
 
if (verbose)
fprintf(stderr, "Checking HEAD link\n");
 
-   head_points_at = resolve_ref_unsafe("HEAD", 0, head_oid.hash, );
+   head_points_at = resolve_ref_unsafe("HEAD", 0, head_oid.hash, NULL);
if (!head_points_at) {
errors_found |= ERROR_REFS;
return error("Invalid HEAD");
-- 
2.8.0.rc3

--
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 v3/GSoC 1/5] path.c: implement strbuf_mkpath()

There were already `mkpath`, `mkpathdup` and `mksnpath` for build
filename, but lacked a version of `strbuf_` just like `strbuf_git_path`.

It is convenient to build a path and manipulate the result later with
strbuf.

Signed-off-by: Hui Yiqun 
---
 cache.h |  2 ++
 path.c  | 21 +
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index b829410..ef843c1 100644
--- a/cache.h
+++ b/cache.h
@@ -781,6 +781,8 @@ extern char *git_pathdup(const char *fmt, ...)
__attribute__((format (printf, 1, 2)));
 extern char *mkpathdup(const char *fmt, ...)
__attribute__((format (printf, 1, 2)));
+extern void strbuf_mkpath(struct strbuf *sb, const char *fmt, ...)
+   __attribute__((format (printf, 2, 3)));
 extern char *git_pathdup_submodule(const char *path, const char *fmt, ...)
__attribute__((format (printf, 2, 3)));
 
diff --git a/path.c b/path.c
index 8b7e168..699af68 100644
--- a/path.c
+++ b/path.c
@@ -433,14 +433,19 @@ char *git_pathdup(const char *fmt, ...)
return strbuf_detach(, NULL);
 }
 
+static void do_mkpath(struct strbuf *buf, const char *fmt, va_list args)
+{
+   strbuf_vaddf(buf, fmt, args);
+   strbuf_cleanup_path(buf);
+}
+
 char *mkpathdup(const char *fmt, ...)
 {
struct strbuf sb = STRBUF_INIT;
va_list args;
va_start(args, fmt);
-   strbuf_vaddf(, fmt, args);
+   do_mkpath(, fmt, args);
va_end(args);
-   strbuf_cleanup_path();
return strbuf_detach(, NULL);
 }
 
@@ -449,9 +454,17 @@ const char *mkpath(const char *fmt, ...)
va_list args;
struct strbuf *pathname = get_pathname();
va_start(args, fmt);
-   strbuf_vaddf(pathname, fmt, args);
+   do_mkpath(pathname, fmt, args);
+   va_end(args);
+   return pathname->buf;
+}
+
+void strbuf_mkpath(struct strbuf *buf, const char *fmt, ...)
+{
+   va_list args;
+   va_start(args, fmt);
+   do_mkpath(buf, fmt, args);
va_end(args);
-   return cleanup_path(pathname->buf);
 }
 
 static void do_submodule_path(struct strbuf *buf, const char *path,
-- 
2.7.4

--
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 15/21] read_raw_ref(): change flags parameter to unsigned int

read_raw_ref() is going to be part of the vtable for reference backends,
so clean up its interface to use "unsigned int flags" rather than "int
flags". Its caller still uses signed int for its flags arguments. But
changing that would touch a lot of code, so leave it for now.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 120b2dd..a15986c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1372,7 +1372,7 @@ static struct ref_entry *get_packed_ref(const char 
*refname)
  */
 static int resolve_missing_loose_ref(const char *refname,
 unsigned char *sha1,
-int *flags)
+unsigned int *flags)
 {
struct ref_entry *entry;
 
@@ -1421,7 +1421,7 @@ static int resolve_missing_loose_ref(const char *refname,
  *   refname will still be valid and unchanged.
  */
 static int read_raw_ref(const char *refname, unsigned char *sha1,
-   struct strbuf *symref, int *flags)
+   struct strbuf *symref, unsigned int *flags)
 {
struct strbuf sb_contents = STRBUF_INIT;
struct strbuf sb_path = STRBUF_INIT;
@@ -1570,7 +1570,7 @@ const char *resolve_ref_unsafe(const char *refname, int 
resolve_flags,
}
 
for (symref_count = 0; symref_count < MAXDEPTH; symref_count++) {
-   int read_flags = 0;
+   unsigned int read_flags = 0;
 
if (read_raw_ref(refname, sha1, _refname, _flags)) {
*flags |= read_flags;
-- 
2.8.0.rc3

--
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 01/21] t1430: test the output and error of some commands more carefully

Signed-off-by: Michael Haggerty 
---
 t/t1430-bad-ref-name.sh | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index c465abe..005e2b1 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -42,7 +42,7 @@ test_expect_success 'git branch shows badly named ref as 
warning' '
cp .git/refs/heads/master .git/refs/heads/broken...ref &&
test_when_finished "rm -f .git/refs/heads/broken...ref" &&
git branch >output 2>error &&
-   grep -e "broken\.\.\.ref" error &&
+   test_i18ngrep -e "ignoring ref with broken name 
refs/heads/broken\.\.\.ref" error &&
! grep -e "broken\.\.\.ref" output
 '
 
@@ -152,21 +152,25 @@ test_expect_success 'rev-parse skips symref pointing to 
broken name' '
git rev-parse --verify one >expect &&
git rev-parse --verify shadow >actual 2>err &&
test_cmp expect actual &&
-   test_i18ngrep "ignoring.*refs/tags/shadow" err
+   test_i18ngrep "ignoring dangling symref refs/tags/shadow" err
 '
 
 test_expect_success 'update-ref --no-deref -d can delete reference to broken 
name' '
git symbolic-ref refs/heads/badname refs/heads/broken...ref &&
test_when_finished "rm -f .git/refs/heads/badname" &&
test_path_is_file .git/refs/heads/badname &&
-   git update-ref --no-deref -d refs/heads/badname &&
-   test_path_is_missing .git/refs/heads/badname
+   git update-ref --no-deref -d refs/heads/badname >output 2>error &&
+   test_path_is_missing .git/refs/heads/badname &&
+   test_must_be_empty output &&
+   test_must_be_empty error
 '
 
 test_expect_success 'update-ref -d can delete broken name' '
cp .git/refs/heads/master .git/refs/heads/broken...ref &&
test_when_finished "rm -f .git/refs/heads/broken...ref" &&
-   git update-ref -d refs/heads/broken...ref &&
+   git update-ref -d refs/heads/broken...ref >output 2>error &&
+   test_must_be_empty output &&
+   test_must_be_empty error &&
git branch >output 2>error &&
! grep -e "broken\.\.\.ref" error &&
! grep -e "broken\.\.\.ref" output
@@ -175,7 +179,9 @@ test_expect_success 'update-ref -d can delete broken name' '
 test_expect_success 'update-ref -d cannot delete non-ref in .git dir' '
echo precious >.git/my-private-file &&
echo precious >expect &&
-   test_must_fail git update-ref -d my-private-file &&
+   test_must_fail git update-ref -d my-private-file >output 2>error &&
+   test_must_be_empty output &&
+   test_i18ngrep -e "cannot lock .*: unable to resolve reference" error &&
test_cmp expect .git/my-private-file
 '
 
-- 
2.8.0.rc3

--
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 20/21] check_aliased_update(): check that dst_name is non-NULL

If there is an error in resolve_ref_unsafe(), it returns NULL. We check
for this case, but not until after calling strip_namespace(). Instead,
call strip_namespace() *after* the NULL check.

Signed-off-by: Michael Haggerty 
---
 builtin/receive-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c8e32b2..49cc88d 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1081,13 +1081,13 @@ static void check_aliased_update(struct command *cmd, 
struct string_list *list)
if (!(flag & REF_ISSYMREF))
return;
 
-   dst_name = strip_namespace(dst_name);
if (!dst_name) {
rp_error("refusing update to broken symref '%s'", 
cmd->ref_name);
cmd->skip_update = 1;
cmd->error_string = "broken symref";
return;
}
+   dst_name = strip_namespace(dst_name);
 
if ((item = string_list_lookup(list, dst_name)) == NULL)
return;
-- 
2.8.0.rc3

--
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 09/21] resolve_ref_1(): eliminate local variable

In place of `buf`, use `refname`, which is anyway a better description
of what is being pointed at.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 067ce1c..69ec903 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1426,7 +1426,6 @@ static const char *resolve_ref_1(const char *refname,
for (symref_count = 0; symref_count < MAXDEPTH; symref_count++) {
const char *path;
struct stat st;
-   char *buf;
int fd;
 
strbuf_reset(sb_path);
@@ -1532,21 +1531,21 @@ static const char *resolve_ref_1(const char *refname,
return refname;
}
*flags |= REF_ISSYMREF;
-   buf = sb_contents->buf + 4;
-   while (isspace(*buf))
-   buf++;
+   refname = sb_contents->buf + 4;
+   while (isspace(*refname))
+   refname++;
strbuf_reset(sb_refname);
-   strbuf_addstr(sb_refname, buf);
+   strbuf_addstr(sb_refname, refname);
refname = sb_refname->buf;
if (resolve_flags & RESOLVE_REF_NO_RECURSE) {
hashclr(sha1);
return refname;
}
-   if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
+   if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
*flags |= REF_ISBROKEN;
 
if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
-   !refname_is_safe(buf)) {
+   !refname_is_safe(refname)) {
errno = EINVAL;
return NULL;
}
-- 
2.8.0.rc3

--
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 13/21] read_raw_ref(): manage own scratch space

Instead of creating scratch space in resolve_ref_unsafe() and passing it
down through resolve_ref_1 to read_raw_ref(), teach read_raw_ref() to
manage its own scratch space. This reduces coupling across the functions
at the cost of some extra allocations. Also, when read_raw_ref() is
implemented for different reference backends, the other implementations
might have different scratch space requirements.

Note that we now preserve errno across the calls to strbuf_release(),
which calls free() and can thus theoretically overwrite errno.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 76 
 1 file changed, 41 insertions(+), 35 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index d51e778..f752568 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1421,17 +1421,20 @@ static int resolve_missing_loose_ref(const char 
*refname,
  *   refname will still be valid and unchanged.
  */
 static int read_raw_ref(const char *refname, unsigned char *sha1,
-   struct strbuf *symref, struct strbuf *sb_path,
-   struct strbuf *sb_contents, int *flags)
+   struct strbuf *symref, int *flags)
 {
+   struct strbuf sb_contents = STRBUF_INIT;
+   struct strbuf sb_path = STRBUF_INIT;
const char *path;
const char *buf;
struct stat st;
int fd;
+   int ret = -1;
+   int save_errno;
 
-   strbuf_reset(sb_path);
-   strbuf_git_path(sb_path, "%s", refname);
-   path = sb_path->buf;
+   strbuf_reset(_path);
+   strbuf_git_path(_path, "%s", refname);
+   path = sb_path.buf;
 
 stat_ref:
/*
@@ -1446,36 +1449,38 @@ stat_ref:
 
if (lstat(path, ) < 0) {
if (errno != ENOENT)
-   return -1;
+   goto out;
if (resolve_missing_loose_ref(refname, sha1, flags)) {
errno = ENOENT;
-   return -1;
+   goto out;
}
-   return 0;
+   ret = 0;
+   goto out;
}
 
/* Follow "normalized" - ie "refs/.." symlinks by hand */
if (S_ISLNK(st.st_mode)) {
-   strbuf_reset(sb_contents);
-   if (strbuf_readlink(sb_contents, path, 0) < 0) {
+   strbuf_reset(_contents);
+   if (strbuf_readlink(_contents, path, 0) < 0) {
if (errno == ENOENT || errno == EINVAL)
/* inconsistent with lstat; retry */
goto stat_ref;
else
-   return -1;
+   goto out;
}
-   if (starts_with(sb_contents->buf, "refs/") &&
-   !check_refname_format(sb_contents->buf, 0)) {
-   strbuf_swap(sb_contents, symref);
+   if (starts_with(sb_contents.buf, "refs/") &&
+   !check_refname_format(sb_contents.buf, 0)) {
+   strbuf_swap(_contents, symref);
*flags |= REF_ISSYMREF;
-   return 0;
+   ret = 0;
+   goto out;
}
}
 
/* Is it a directory? */
if (S_ISDIR(st.st_mode)) {
errno = EISDIR;
-   return -1;
+   goto out;
}
 
/*
@@ -1488,18 +1493,18 @@ stat_ref:
/* inconsistent with lstat; retry */
goto stat_ref;
else
-   return -1;
+   goto out;
}
-   strbuf_reset(sb_contents);
-   if (strbuf_read(sb_contents, fd, 256) < 0) {
+   strbuf_reset(_contents);
+   if (strbuf_read(_contents, fd, 256) < 0) {
int save_errno = errno;
close(fd);
errno = save_errno;
-   return -1;
+   goto out;
}
close(fd);
-   strbuf_rtrim(sb_contents);
-   buf = sb_contents->buf;
+   strbuf_rtrim(_contents);
+   buf = sb_contents.buf;
if (starts_with(buf, "ref:")) {
buf += 4;
while (isspace(*buf))
@@ -1508,7 +1513,8 @@ stat_ref:
strbuf_reset(symref);
strbuf_addstr(symref, buf);
*flags |= REF_ISSYMREF;
-   return 0;
+   ret = 0;
+   goto out;
}
 
/*
@@ -1519,10 +1525,17 @@ stat_ref:
(buf[40] != '\0' && !isspace(buf[40]))) {
*flags |= REF_ISBROKEN;
errno = EINVAL;
-   return -1;
+   goto out;
}
 
-   return 0;
+   ret = 0;
+
+out:
+   save_errno = errno;
+   strbuf_release(_path);
+   strbuf_release(_contents);
+

[PATCH 18/21] get_default_remote(): remove unneeded flag variable

It is never read, so we can pass NULL to resolve_ref_unsafe().

Signed-off-by: Michael Haggerty 
---
 builtin/submodule--helper.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ed4f60b..c72365e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -17,9 +17,8 @@ static char *get_default_remote(void)
 {
char *dest = NULL, *ret;
unsigned char sha1[20];
-   int flag = 0;
struct strbuf sb = STRBUF_INIT;
-   const char *refname = resolve_ref_unsafe("HEAD", 0, sha1, );
+   const char *refname = resolve_ref_unsafe("HEAD", 0, sha1, NULL);
 
if (!refname)
die("No such ref: HEAD");
-- 
2.8.0.rc3

--
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 02/21] t1430: clean up broken refs/tags/shadow

Signed-off-by: Michael Haggerty 
---
 t/t1430-bad-ref-name.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index 005e2b1..cb815ab 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -148,7 +148,7 @@ test_expect_success 'rev-parse skips symref pointing to 
broken name' '
git branch shadow one &&
cp .git/refs/heads/master .git/refs/heads/broken...ref &&
git symbolic-ref refs/tags/shadow refs/heads/broken...ref &&
-
+   test_when_finished "rm -f .git/refs/tags/shadow" &&
git rev-parse --verify one >expect &&
git rev-parse --verify shadow >actual 2>err &&
test_cmp expect actual &&
-- 
2.8.0.rc3

--
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 07/21] resolve_ref_unsafe(): use for loop to count up to MAXDEPTH

The loop's there anyway; we might as well use it.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index c0cf6fd..101abba 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1400,8 +1400,8 @@ static const char *resolve_ref_1(const char *refname,
 struct strbuf *sb_path,
 struct strbuf *sb_contents)
 {
-   int depth = MAXDEPTH;
int bad_name = 0;
+   int symref_count;
 
if (flags)
*flags = 0;
@@ -1425,17 +1425,13 @@ static const char *resolve_ref_1(const char *refname,
 */
bad_name = 1;
}
-   for (;;) {
+
+   for (symref_count = 0; symref_count < MAXDEPTH; symref_count++) {
const char *path;
struct stat st;
char *buf;
int fd;
 
-   if (--depth < 0) {
-   errno = ELOOP;
-   return NULL;
-   }
-
strbuf_reset(sb_path);
strbuf_git_path(sb_path, "%s", refname);
path = sb_path->buf;
@@ -1566,6 +1562,9 @@ static const char *resolve_ref_1(const char *refname,
bad_name = 1;
}
}
+
+   errno = ELOOP;
+   return NULL;
 }
 
 const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
-- 
2.8.0.rc3

--
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 04/21] t1430: test for-each-ref in the presence of badly-named refs

Signed-off-by: Michael Haggerty 
---
 t/t1430-bad-ref-name.sh | 16 
 1 file changed, 16 insertions(+)

diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index a963951..612cc32 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -155,6 +155,22 @@ test_expect_success 'rev-parse skips symref pointing to 
broken name' '
test_i18ngrep "ignoring dangling symref refs/tags/shadow" err
 '
 
+test_expect_success 'for-each-ref emits warnings for broken names' '
+   cp .git/refs/heads/master .git/refs/heads/broken...ref &&
+   test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+   printf "ref: refs/heads/broken...ref\n" >.git/refs/heads/badname &&
+   test_when_finished "rm -f .git/refs/heads/badname" &&
+   printf "ref: refs/heads/master\n" >.git/refs/heads/broken...symref &&
+   test_when_finished "rm -f .git/refs/heads/broken...symref" &&
+   git for-each-ref >output 2>error &&
+   ! grep -e "broken\.\.\.ref" output &&
+   ! grep -e "badname" output &&
+   ! grep -e "broken\.\.\.symref" output &&
+   test_i18ngrep "ignoring ref with broken name 
refs/heads/broken\.\.\.ref" error &&
+   test_i18ngrep "ignoring broken ref refs/heads/badname" error &&
+   test_i18ngrep "ignoring ref with broken name 
refs/heads/broken\.\.\.symref" error
+'
+
 test_expect_success 'update-ref --no-deref -d can delete reference to broken 
name' '
printf "ref: refs/heads/broken...ref\n" >.git/refs/heads/badname &&
test_when_finished "rm -f .git/refs/heads/badname" &&
-- 
2.8.0.rc3

--
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 03/21] t1430: don't rely on symbolic-ref for creating broken symrefs

It's questionable whether it should even work.

Signed-off-by: Michael Haggerty 
---
 t/t1430-bad-ref-name.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index cb815ab..a963951 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -147,7 +147,7 @@ test_expect_success 'rev-parse skips symref pointing to 
broken name' '
test_when_finished "rm -f .git/refs/heads/broken...ref" &&
git branch shadow one &&
cp .git/refs/heads/master .git/refs/heads/broken...ref &&
-   git symbolic-ref refs/tags/shadow refs/heads/broken...ref &&
+   printf "ref: refs/heads/broken...ref\n" >.git/refs/tags/shadow &&
test_when_finished "rm -f .git/refs/tags/shadow" &&
git rev-parse --verify one >expect &&
git rev-parse --verify shadow >actual 2>err &&
@@ -156,7 +156,7 @@ test_expect_success 'rev-parse skips symref pointing to 
broken name' '
 '
 
 test_expect_success 'update-ref --no-deref -d can delete reference to broken 
name' '
-   git symbolic-ref refs/heads/badname refs/heads/broken...ref &&
+   printf "ref: refs/heads/broken...ref\n" >.git/refs/heads/badname &&
test_when_finished "rm -f .git/refs/heads/badname" &&
test_path_is_file .git/refs/heads/badname &&
git update-ref --no-deref -d refs/heads/badname >output 2>error &&
-- 
2.8.0.rc3

--
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 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33

Patch 04/33 in David Turner's refs-backend-lmdb series v7 [1] did way
too much in a single patch, and in fact got a few minor things wrong.
Instead of that patch, I suggest this patch series, which

* Splits the changes into smaller steps.

* Adds a bunch of tests of deleting references with invalid but safe
  names, including symbolic references and including references
  reached via symbolic references. Two of these tests fail when run
  against David's patch 04 due to changes in output.

* Arranges for the "flags" argument to read_raw_ref() always to be
  non-NULL, which eliminates the need for a lot of "if (flags)"
  guards.

* Eliminates the now-superfluous "bad_name" local variable.

* Move the management of the scratch space sb_path from
  resolve_ref_unsafe() to read_raw_ref().

* Inlines resolve_ref_1() into resolve_ref_unsafe().

* Changes some callers of resolve_ref_unsafe() to pass flags=NULL
  instead of creating a local flags variable that is never used.

* Changes some callers to check for errors before using the return
  value of resolve_ref_unsafe().

I hope that the result is easier to understand and audit, even though
it consists of more patches (indeed, *because* of that).

This patch series applies on top of David's patch 03/33 the same place
David applied it in his repo [2]. It is also available in situ from my
GitHub repo [3] as branch "pluggable-backends-patch4"

If this series is used, later patches from David's series would need
to be rebased on top of it. This is a little bit messy but not
difficult; the result can be seen in branch
"pluggable-backends-rebased" in my GitHub repo [3] (albeit without
adjusting the LMDB-related patches).

Michael

[1] http://article.gmane.org/gmane.comp.version-control.git/287971
[2] https://github.com/dturner-tw/git/tree/dturner/pluggable-backends
[3] https://github.com/mhagger/git

David Turner (1):
  files-backend: break out ref reading

Michael Haggerty (20):
  t1430: test the output and error of some commands more carefully
  t1430: clean up broken refs/tags/shadow
  t1430: don't rely on symbolic-ref for creating broken symrefs
  t1430: test for-each-ref in the presence of badly-named refs
  t1430: improve test coverage of deletion of badly-named refs
  resolve_missing_loose_ref(): simplify semantics
  resolve_ref_unsafe(): use for loop to count up to MAXDEPTH
  resolve_ref_unsafe(): ensure flags is always set
  resolve_ref_1(): eliminate local variable
  resolve_ref_1(): reorder code
  resolve_ref_1(): eliminate local variable "bad_name"
  read_raw_ref(): manage own scratch space
  Inline resolve_ref_1() into resolve_ref_unsafe()
  read_raw_ref(): change flags parameter to unsigned int
  fsck_head_link(): remove unneeded flag variable
  cmd_merge(): remove unneeded flag variable
  get_default_remote(): remove unneeded flag variable
  checkout_paths(): remove unneeded flag variable
  check_aliased_update(): check that dst_name is non-NULL
  show_head_ref(): check the result of resolve_ref_namespace()

 builtin/checkout.c  |   3 +-
 builtin/fsck.c  |   3 +-
 builtin/merge.c |   4 +-
 builtin/receive-pack.c  |   2 +-
 builtin/submodule--helper.c |   3 +-
 http-backend.c  |   4 +-
 refs/files-backend.c| 341 
 t/t1430-bad-ref-name.sh | 132 +++--
 8 files changed, 312 insertions(+), 180 deletions(-)

-- 
2.8.0.rc3

--
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 12/21] files-backend: break out ref reading

From: David Turner 

Refactor resolve_ref_1 in terms of a new function read_raw_ref, which
is responsible for reading ref data from the ref storage.

Later, we will make read_raw_ref a pluggable backend function, and make
resolve_ref_unsafe common.

Signed-off-by: David Turner 
Helped-by: Duy Nguyen 
Signed-off-by: Junio C Hamano 
Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 244 ++-
 1 file changed, 145 insertions(+), 99 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index b865ba5..d51e778 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1390,6 +1390,141 @@ static int resolve_missing_loose_ref(const char 
*refname,
return -1;
 }
 
+/*
+ * Read a raw ref from the filesystem or packed refs file.
+ *
+ * If the ref is a sha1, fill in sha1 and return 0.
+ *
+ * If the ref is symbolic, fill in *symref with the referrent
+ * (e.g. "refs/heads/master") and return 0.  The caller is responsible
+ * for validating the referrent.  Set REF_ISSYMREF in flags.
+ *
+ * If the ref doesn't exist, set errno to ENOENT and return -1.
+ *
+ * If the ref exists but is neither a symbolic ref nor a sha1, it is
+ * broken. Set REF_ISBROKEN in flags, set errno to EINVAL, and return
+ * -1.
+ *
+ * If there is another error reading the ref, set errno appropriately and
+ * return -1.
+ *
+ * Backend-specific flags might be set in flags as well, regardless of
+ * outcome.
+ *
+ * sb_path is workspace: the caller should allocate and free it.
+ *
+ * It is OK for refname to point into symref. In this case:
+ * - if the function succeeds with REF_ISSYMREF, symref will be
+ *   overwritten and the memory pointed to by refname might be changed
+ *   or even freed.
+ * - in all other cases, symref will be untouched, and therefore
+ *   refname will still be valid and unchanged.
+ */
+static int read_raw_ref(const char *refname, unsigned char *sha1,
+   struct strbuf *symref, struct strbuf *sb_path,
+   struct strbuf *sb_contents, int *flags)
+{
+   const char *path;
+   const char *buf;
+   struct stat st;
+   int fd;
+
+   strbuf_reset(sb_path);
+   strbuf_git_path(sb_path, "%s", refname);
+   path = sb_path->buf;
+
+stat_ref:
+   /*
+* We might have to loop back here to avoid a race
+* condition: first we lstat() the file, then we try
+* to read it as a link or as a file.  But if somebody
+* changes the type of the file (file <-> directory
+* <-> symlink) between the lstat() and reading, then
+* we don't want to report that as an error but rather
+* try again starting with the lstat().
+*/
+
+   if (lstat(path, ) < 0) {
+   if (errno != ENOENT)
+   return -1;
+   if (resolve_missing_loose_ref(refname, sha1, flags)) {
+   errno = ENOENT;
+   return -1;
+   }
+   return 0;
+   }
+
+   /* Follow "normalized" - ie "refs/.." symlinks by hand */
+   if (S_ISLNK(st.st_mode)) {
+   strbuf_reset(sb_contents);
+   if (strbuf_readlink(sb_contents, path, 0) < 0) {
+   if (errno == ENOENT || errno == EINVAL)
+   /* inconsistent with lstat; retry */
+   goto stat_ref;
+   else
+   return -1;
+   }
+   if (starts_with(sb_contents->buf, "refs/") &&
+   !check_refname_format(sb_contents->buf, 0)) {
+   strbuf_swap(sb_contents, symref);
+   *flags |= REF_ISSYMREF;
+   return 0;
+   }
+   }
+
+   /* Is it a directory? */
+   if (S_ISDIR(st.st_mode)) {
+   errno = EISDIR;
+   return -1;
+   }
+
+   /*
+* Anything else, just open it and try to use it as
+* a ref
+*/
+   fd = open(path, O_RDONLY);
+   if (fd < 0) {
+   if (errno == ENOENT)
+   /* inconsistent with lstat; retry */
+   goto stat_ref;
+   else
+   return -1;
+   }
+   strbuf_reset(sb_contents);
+   if (strbuf_read(sb_contents, fd, 256) < 0) {
+   int save_errno = errno;
+   close(fd);
+   errno = save_errno;
+   return -1;
+   }
+   close(fd);
+   strbuf_rtrim(sb_contents);
+   buf = sb_contents->buf;
+   if (starts_with(buf, "ref:")) {
+   buf += 4;
+   while (isspace(*buf))
+   buf++;
+
+   strbuf_reset(symref);
+   strbuf_addstr(symref, buf);
+

[PATCH 19/21] checkout_paths(): remove unneeded flag variable

It is never read, so we can pass NULL to resolve_ref_unsafe().

Signed-off-by: Michael Haggerty 
---
 builtin/checkout.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index cfa66e2..ef42237 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -242,7 +242,6 @@ static int checkout_paths(const struct checkout_opts *opts,
struct checkout state;
static char *ps_matched;
unsigned char rev[20];
-   int flag;
struct commit *head;
int errs = 0;
struct lock_file *lock_file;
@@ -375,7 +374,7 @@ static int checkout_paths(const struct checkout_opts *opts,
if (write_locked_index(_index, lock_file, COMMIT_LOCK))
die(_("unable to write new index file"));
 
-   read_ref_full("HEAD", 0, rev, );
+   read_ref_full("HEAD", 0, rev, NULL);
head = lookup_commit_reference_gently(rev, 1);
 
errs |= post_checkout_hook(head, head, 0);
-- 
2.8.0.rc3

--
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 06/21] resolve_missing_loose_ref(): simplify semantics

Make resolve_missing_loose_ref() only responsible for looking up a
packed reference, without worrying about whether we want to read or
write the reference and without setting errno on failure. Move the other
logic to the caller.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 24 ++--
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 9676ec2..c0cf6fd 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1368,11 +1368,9 @@ static struct ref_entry *get_packed_ref(const char 
*refname)
 }
 
 /*
- * A loose ref file doesn't exist; check for a packed ref.  The
- * options are forwarded from resolve_safe_unsafe().
+ * A loose ref file doesn't exist; check for a packed ref.
  */
 static int resolve_missing_loose_ref(const char *refname,
-int resolve_flags,
 unsigned char *sha1,
 int *flags)
 {
@@ -1389,14 +1387,8 @@ static int resolve_missing_loose_ref(const char *refname,
*flags |= REF_ISPACKED;
return 0;
}
-   /* The reference is not a packed reference, either. */
-   if (resolve_flags & RESOLVE_REF_READING) {
-   errno = ENOENT;
-   return -1;
-   } else {
-   hashclr(sha1);
-   return 0;
-   }
+   /* refname is not a packed reference. */
+   return -1;
 }
 
 /* This function needs to return a meaningful errno on failure */
@@ -1461,9 +1453,13 @@ static const char *resolve_ref_1(const char *refname,
if (lstat(path, ) < 0) {
if (errno != ENOENT)
return NULL;
-   if (resolve_missing_loose_ref(refname, resolve_flags,
- sha1, flags))
-   return NULL;
+   if (resolve_missing_loose_ref(refname, sha1, flags)) {
+   if (resolve_flags & RESOLVE_REF_READING) {
+   errno = ENOENT;
+   return NULL;
+   }
+   hashclr(sha1);
+   }
if (bad_name) {
hashclr(sha1);
if (flags)
-- 
2.8.0.rc3

--
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 17/21] cmd_merge(): remove unneeded flag variable

It is never read, so we can pass NULL to resolve_ref_unsafe().

Signed-off-by: Michael Haggerty 
---
 builtin/merge.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index df0afa0..a91ae5b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1146,7 +1146,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
unsigned char head_sha1[20];
struct commit *head_commit;
struct strbuf buf = STRBUF_INIT;
-   int flag, i, ret = 0, head_subsumed;
+   int i, ret = 0, head_subsumed;
int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
struct commit_list *common = NULL;
const char *best_strategy = NULL, *wt_strategy = NULL;
@@ -1160,7 +1160,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
 * Check if we are _not_ on a detached HEAD, i.e. if there is a
 * current branch.
 */
-   branch = branch_to_free = resolve_refdup("HEAD", 0, head_sha1, );
+   branch = branch_to_free = resolve_refdup("HEAD", 0, head_sha1, NULL);
if (branch && starts_with(branch, "refs/heads/"))
branch += 11;
if (!branch || is_null_sha1(head_sha1))
-- 
2.8.0.rc3

--
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 21/21] show_head_ref(): check the result of resolve_ref_namespace()

Only use the result of resolve_ref_namespace() if it is non-NULL.

Signed-off-by: Michael Haggerty 
---
 http-backend.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index 8870a26..2148814 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -484,9 +484,9 @@ static int show_head_ref(const char *refname, const struct 
object_id *oid,
const char *target = resolve_ref_unsafe(refname,
RESOLVE_REF_READING,
unused.hash, NULL);
-   const char *target_nons = strip_namespace(target);
 
-   strbuf_addf(buf, "ref: %s\n", target_nons);
+   if (target)
+   strbuf_addf(buf, "ref: %s\n", strip_namespace(target));
} else {
strbuf_addf(buf, "%s\n", oid_to_hex(oid));
}
-- 
2.8.0.rc3

--
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 05/21] t1430: improve test coverage of deletion of badly-named refs