[PATCH 3/3] branch: forbid refs/heads/HEAD

[Junio C Hamano]

strbuf_check_branch_ref() is the central place where many codepaths
see if a proposed name is suitable for the name of a branch.  It was
designed to allow us to get stricter than the check_refname_format()
check used for refnames in general, and we already use it to reject
a branch whose name begins with a '-'.

Use it to also reject "HEAD" as a branch name.

Signed-off-by: Junio C Hamano 
---
 sha1_name.c | 2 +-
 t/t1430-bad-ref-name.sh | 8 
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/sha1_name.c b/sha1_name.c
index c7c5ab376c..1b8c503095 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1332,7 +1332,7 @@ void strbuf_branchname(struct strbuf *sb, const char 
*name, unsigned allowed)
 int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
 {
strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
-   if (name[0] == '-')
+   if (*name == '-' || !strcmp(name, "HEAD"))
return -1;
strbuf_splice(sb, 0, 0, "refs/heads/", 11);
return check_refname_format(sb->buf, 0);
diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index e88349c8a0..3ecb2eab0c 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -331,4 +331,12 @@ test_expect_success 'update-ref --stdin -z fails delete 
with bad ref name' '
grep "fatal: invalid ref format: ~a" err
 '
 
+test_expect_success 'branch rejects HEAD as a branch name' '
+   test_must_fail git branch HEAD HEAD^
+'
+
+test_expect_success 'checkout -b rejects HEAD as a branch name' '
+   test_must_fail git checkout -B HEAD HEAD^
+'
+
 test_done
-- 
2.15.0-rc1-158-gbd035ae683

[PATCH 0/3] a small branch API clean-up

[Junio C Hamano]

This started as a little clean-up for a NEEDSWORK comment in
branch.h, but it ended up adding a rather useful safety feature.

Junio C Hamano (3):
  branch: streamline "attr_only" handling in validate_new_branchname()
  branch: split validate_new_branchname() into two
  branch: forbid refs/heads/HEAD

 branch.c| 44 ++--
 branch.h| 27 ---
 builtin/branch.c|  8 
 builtin/checkout.c  | 10 +-
 sha1_name.c |  2 +-
 t/t1430-bad-ref-name.sh |  8 
 6 files changed, 60 insertions(+), 39 deletions(-)

-- 
2.15.0-rc1-158-gbd035ae683

[PATCH 1/3] branch: streamline "attr_only" handling in validate_new_branchname()

[Junio C Hamano]

The function takes a parameter "attr_only" (which is a name that is
hard to reason about, which will be corrected soon) and skips some
checks when it is set.  Reorganize the conditionals to make it move
obvious that some checks are never made when this parameter is set.

Signed-off-by: Junio C Hamano 
---
 branch.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/branch.c b/branch.c
index 4377ce2fb1..7404597678 100644
--- a/branch.c
+++ b/branch.c
@@ -181,21 +181,25 @@ int read_branch_desc(struct strbuf *buf, const char 
*branch_name)
 int validate_new_branchname(const char *name, struct strbuf *ref,
int force, int attr_only)
 {
+   const char *head;
+
if (strbuf_check_branch_ref(ref, name))
die(_("'%s' is not a valid branch name."), name);
 
if (!ref_exists(ref->buf))
return 0;
-   else if (!force && !attr_only)
-   die(_("A branch named '%s' already exists."), ref->buf + 
strlen("refs/heads/"));
 
-   if (!attr_only) {
-   const char *head;
+   if (attr_only)
+   return 1;
+
+   if (!force)
+   die(_("A branch named '%s' already exists."),
+   ref->buf + strlen("refs/heads/"));
+
+   head = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
+   if (!is_bare_repository() && head && !strcmp(head, ref->buf))
+   die(_("Cannot force update the current branch."));
 
-   head = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
-   if (!is_bare_repository() && head && !strcmp(head, ref->buf))
-   die(_("Cannot force update the current branch."));
-   }
return 1;
 }
 
-- 
2.15.0-rc1-158-gbd035ae683

[PATCH 2/3] branch: split validate_new_branchname() into two

[Junio C Hamano]

Checking if a proposed name is appropriate for a branch is strictly
a subset of checking if we want to allow creating or updating a
branch with such a name.  The mysterious sounding 'attr_only'
parameter to validate_new_branchname() is used to switch the
function between these two roles.

Instead, split the function into two, and adjust the callers.  A new
helper validate_branchname() only checks the name and reports if the
branch already exists.

This loses one NEEDSWORK from the branch API.

Signed-off-by: Junio C Hamano 
---
 branch.c   | 34 +++---
 branch.h   | 27 ---
 builtin/branch.c   |  8 
 builtin/checkout.c | 10 +-
 4 files changed, 44 insertions(+), 35 deletions(-)

diff --git a/branch.c b/branch.c
index 7404597678..2c3a364a0b 100644
--- a/branch.c
+++ b/branch.c
@@ -178,19 +178,31 @@ int read_branch_desc(struct strbuf *buf, const char 
*branch_name)
return 0;
 }
 
-int validate_new_branchname(const char *name, struct strbuf *ref,
-   int force, int attr_only)
+/*
+ * Check if 'name' can be a valid name for a branch; die otherwise.
+ * Return 1 if the named branch already exists; return 0 otherwise.
+ * Fill ref with the full refname for the branch.
+ */
+int validate_branchname(const char *name, struct strbuf *ref)
 {
-   const char *head;
-
if (strbuf_check_branch_ref(ref, name))
die(_("'%s' is not a valid branch name."), name);
 
-   if (!ref_exists(ref->buf))
-   return 0;
+   return ref_exists(ref->buf);
+}
 
-   if (attr_only)
-   return 1;
+/*
+ * Check if a branch 'name' can be created as a new branch; die otherwise.
+ * 'force' can be used when it is OK for the named branch already exists.
+ * Return 1 if the named branch already exists; return 0 otherwise.
+ * Fill ref with the full refname for the branch.
+ */
+int validate_new_branchname(const char *name, struct strbuf *ref, int force)
+{
+   const char *head;
+
+   if (!validate_branchname(name, ref))
+   return 0;
 
if (!force)
die(_("A branch named '%s' already exists."),
@@ -246,9 +258,9 @@ void create_branch(const char *name, const char *start_name,
if (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)
explicit_tracking = 1;
 
-   if (validate_new_branchname(name, , force,
-   track == BRANCH_TRACK_OVERRIDE ||
-   clobber_head)) {
+   if ((track == BRANCH_TRACK_OVERRIDE || clobber_head)
+   ? validate_branchname(name, )
+   : validate_new_branchname(name, , force)) {
if (!force)
dont_change_ref = 1;
else
diff --git a/branch.h b/branch.h
index b07788558c..be5e5d1308 100644
--- a/branch.h
+++ b/branch.h
@@ -23,22 +23,19 @@ void create_branch(const char *name, const char *start_name,
   int clobber_head, int quiet, enum branch_track track);
 
 /*
- * Validates that the requested branch may be created, returning the
- * interpreted ref in ref, force indicates whether (non-head) branches
- * may be overwritten. A non-zero return value indicates that the force
- * parameter was non-zero and the branch already exists.
- *
- * Contrary to all of the above, when attr_only is 1, the caller is
- * not interested in verifying if it is Ok to update the named
- * branch to point at a potentially different commit. It is merely
- * asking if it is OK to change some attribute for the named branch
- * (e.g. tracking upstream).
- *
- * NEEDSWORK: This needs to be split into two separate functions in the
- * longer run for sanity.
- *
+ * Check if 'name' can be a valid name for a branch; die otherwise.
+ * Return 1 if the named branch already exists; return 0 otherwise.
+ * Fill ref with the full refname for the branch.
+ */
+extern int validate_branchname(const char *name, struct strbuf *ref);
+
+/*
+ * Check if a branch 'name' can be created as a new branch; die otherwise.
+ * 'force' can be used when it is OK for the named branch already exists.
+ * Return 1 if the named branch already exists; return 0 otherwise.
+ * Fill ref with the full refname for the branch.
  */
-int validate_new_branchname(const char *name, struct strbuf *ref, int force, 
int attr_only);
+extern int validate_new_branchname(const char *name, struct strbuf *ref, int 
force);
 
 /*
  * Remove information about the state of working on the current
diff --git a/builtin/branch.c b/builtin/branch.c
index b67593288c..e5bbfb4a17 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -463,7 +463,6 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = 
STRBUF_INIT;
struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
   

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Junio C Hamano]

Jeff King  writes:

> OK. For the record, I'm not against scrapping this whole thing and
> trying to rollback to your "plumbing never looks at color.ui" proposal.
> It's quite late in the -rc cycle to do that, but there's nothing that
> says we can't bump the release date if that's what we need to do to get
> it right.

I think that it is too late, regardless of our release cycle.

"Plumbing never looks at color.ui" implies that "plumbing must not
get color.ui=auto from 4c7f1819", but given that 4c7f1819 is from
2013, I'd be surprised if we stopped coloring output from plumbing
without getting any complaints from third-party script writers.

Can I remove multiple stashed states at a time?

[小川恭史]

I want to remove multiple stashed states at a time.

But "git stash drop " removes only one stashed state at a time
and "git stash clear" remove all.

Can I do that?

Re: [PATCH v3] pull: pass --signoff/--no-signoff to "git merge"

[Junio C Hamano]

"W. Trevor King"  writes:

> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index 4df6431c34..0ada8c856b 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -64,14 +64,6 @@ OPTIONS
>  ---
>  include::merge-options.txt[]
>  
> ---signoff::
> - Add Signed-off-by line by the committer at the end of the commit
> - log message.  The meaning of a signoff depends on the project,
> - but it typically certifies that committer has
> - the rights to submit this work under the same license and
> - agrees to a Developer Certificate of Origin
> - (see http://developercertificate.org/ for more information).
> -
>  -S[]::
>  --gpg-sign[=]::
>   GPG-sign the resulting merge commit. The `keyid` argument is
> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
> index 4e32304301..f394622d65 100644
> --- a/Documentation/merge-options.txt
> +++ b/Documentation/merge-options.txt
> @@ -51,6 +51,16 @@ set to `no` at the beginning of them.
>  With --no-log do not list one-line descriptions from the
>  actual commits being merged.
>  
> +--signoff::
> +--no-signoff::
> + Add Signed-off-by line by the committer at the end of the commit
> + log message.  The meaning of a signoff depends on the project,
> + but it typically certifies that committer has
> + the rights to submit this work under the same license and
> + agrees to a Developer Certificate of Origin
> + (see http://developercertificate.org/ for more information).
> ++
> +With --no-signoff do not add a Signed-off-by line.

Makes sense.  Thanks, will queue.

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Jeff King]

On Fri, Oct 13, 2017 at 09:09:09AM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > ... Also
> > as an aside, I think this patch means that:
> >
> >   git -c color.ui=always add -p
> >
> > is broken (as would a hypothetical "git --default-color=always add -p").
> > That's sufficiently insane that I'm not sure we should care about it.
> 
> Do you mean that "'-c color.ui=always' from the command line is
> passed down to the invocations of 'git' the 'add' command makes, and
> would break output from 'diff-index' that 'add -i' wants to parse"?

Yes, exactly.

> With the breakage that motivated "downgrade only for on-disk" change
> in mind, I do think that is the right behaviour.  Those third-party
> scripts we broke knew how '-c color.ui=always' works and depended on
> it, and I consider that the command line configuration getting
> passed around as an integral part of 'how it works'.  "Fixing" it
> will break them again.

Yeah, agreed. We cannot know what the script is expecting, so without
that we cannot win, short of turning off color.ui entirely for plumbing.

> Let's take it as a signal that tells us that the script writers know
> what they are doing and leave it as a longish rope they can play with.

OK. For the record, I'm not against scrapping this whole thing and
trying to rollback to your "plumbing never looks at color.ui" proposal.
It's quite late in the -rc cycle to do that, but there's nothing that
says we can't bump the release date if that's what we need to do to get
it right.

If we ship v2.15 with the "color.ui=always really means auto", I don't
think we'd want to undo that. So if we ship with what's in -rc1 (plus
this new hack on top) I think that would be fairly final.

-Peff

Re: Enhancement request: git-push: Allow (configurable) default push-option

[Junio C Hamano]

Stefan Beller  writes:

> Junio,
> do we have variables that behave similarly to this?

If you scan Documentation/config.txt, you'll find http.extraHeader
as an example' but I do not recall offhand if that was the original
that introduced the convention, or it merely followed precedence set
by other variables.

Re: [PATCH v2 5/5] Add tests around status handling of ignored arguments

[Junio C Hamano]

Jameson Miller  writes:

>> Hmph, having some tests in 3/5, changes in 4/5 and even more tests
>> in 5/5 makes me as a reader a bit confused, as the description for
>> these two test patches does not make it clear how they are related
>> and how they are different.  Is it that changes in 1/5 alone does
>> not fulfill the promise made by documentation added at 2/5 so 3/5
>> only has tests for behaviour that works with 1/5 alone but is broken
>> with respect to what 2/5 claims until 4/5 is applied, and these "not
>> working with 1/5 alone, but works after 4/5" are added in this step?
>
> Correct. The changes in 1/5 are to implement "--ignored=matching"
> with "--untracked-files=all" with corresponding tests in 3/5. The
> changes in 4/5 are to implement "--ignored=matching"
> with "--untracked-files=normal" and the corresponding tests are
> in 5/5.
>
> Do you have a preference on how I organized this work? I see
> several possible ways to split up this work. Maybe it would be
> less confusing to have the implementation in the first two
> commits, then 1 commit with all the tests, and then a commit with
> the documentation? I think it makes sense to have the logic for
> the different flag combinations split into their own commits, but
> I am open to any suggestions.

Yeah, there are some alternatives, all valid.  

Support matching/all combination in 1/5, document that in 2/5, test
that in 3/5 and then do the same 3-patch series to support
matching/normal combination in 4/5, 5/5 and 6/5 would be one.  Doing
code, doc and test for matching/all in one patch and doing code, doc
and test for matching/normal in another patch, resulting in a
two-patch series may be another.  Or your "code for matching/all in
1/5, code for matching/normal in 2/5, doc and test for both in
subsequent steps" is fine, too.  All of these would not leave things
in inconsistent state when we interrupt the series in the middle,
which is a desirable property for those who want to understand the
topic.

Re: [PATCH v2 2/5] Update documentation for new directory and status logic

[Junio C Hamano]

Jameson Miller  writes:

>>> +If this is set, files and directories that explicity match an ignore
>>> +pattern are reported. Implicity ignored directories (directories that
>>> +do not match an ignore pattern, but whose contents are all ignored)
>>> +are not reported, instead all of the contents are reported.
>> Makes me wonder if DIR_SHOW_IGNORED* should be splt out into a short
>> enum.  We have:
>>
>>   - Do not show ignored ones (0)
>>
>>   - Collect ignored ones (DIR_SHOW_IGNORED)
>>
>>   - Collect ignored and untracked ones separately (DIR_SHOW_IGNORED_TOO)
>>
>>   - Collect ignored and duntracked ones separately, but limit them to
>> those mach exclude patterns explicitly 
>> (DIR_SHOW_IGNORED_TOO|...MODE_MATCHING)
>>
>> so we need two bits to fit a 4-possiblity enum.
>>
>> Then we do not have to worry about saying quirky things like A and B
>> are incompatible, and C makes sense only when B is set, etc.
> I could see a potential for other values for the "show ignored
> mode" flags - for example: "NORMAL", "MATCHING", "ALL"... Instead
> of making more change at this point in time, how would you feel
> about waiting until the next change in this area.
>
> If you would prefer for me to change these enums now, I can do
> that.

"Makes me wonder" was just that.  I was made to wonder.  I did not
have strong opinions either way.  You thought about the area of this
code longer than I did, so I do not mind you picking the course of
action that is best for the project, and if you think it is better
to wait until we know more about the vocabulary we want to support
before we restructure these flags, that is fine by me.

Thanks.

Re: Enhancement request: git-push: Allow (configurable) default push-option

[Junio C Hamano]

Marius Paliga  writes:

> Thank you for your coments and explanation.
>
> Just one thing:
>
>>  - After parse_options() returns to cmd_push(), see if push_options
>>is empty.  If it is, you did not get any command line option, so
>>override it with what you collected in the "from-config" string
>>list.  Otherwise, do not even look at "from-config" string list.
>
> The idea is that there are default push options (read from config) that are
> always sent to the server and you can add (not overwrite) additional by
> specifying "--push-option".

I can imagine that sometimes giving a base from a configuration and
then adding more for specific invocation may be useful.  

But I do not think of any --command-line-option and config.variable
pair whose configured value cannot be overriden by the command line
option; we should strive to avoid making --push-option a special
case that the users need to aware of, and more importantly, users
other than you who expect the more usual "command line overrides"
behaviour should get that.

Your "I wanted to accumulate, so I made so and made it impossible to
override" won't fly as a justification.  The default should be
"command line overrides", and if you need a way to allow command
line to add without overiding, that should be added as an optional
feature.

[alias]
mypush = push --push-option=foo --push-option=bar

may give you a set of push-options that are always in effect (they
are not even "by default") and cannot be overriden.

Re: [PATCH v2 2/3] for-each-ref: let upstream/push optionally remote ref name

[Junio C Hamano]

Johannes Schindelin  writes:

>> (The funny "squash!" followed by a complete version of the
>> rewritten commit message is what I do until I -- or anybody else
>> -- comes up with a patch to implement support for "reword!".)

I have wished for something like that for a long time; it has been
(and it still is) a bit unclear what the semantics and UI should
look like, but still, such a feature would be quite useful.

Re: [PATCH] diff.c: increment buffer pointer in all code path

[Stefan Beller]

On Thu, Oct 12, 2017 at 5:20 PM, Jeff King  wrote:
> On Thu, Oct 12, 2017 at 08:18:37PM -0400, Jeff King wrote:
>
>> Fix this by entering the conditional only when we actually
>> see whitespace. We can apply this also to the
>> IGNORE_WHITESPACE change. That code path isn't buggy
>> (because it falls through to returning the next
>> non-whitespace byte), but it makes the logic more clear if
>> we only bother to look at whitespace flags after seeing that
>> the next byte is whitespace.
>
> I think there actually _is_ a bug in that code path, but it's unrelated
> to this one. If you have whitespace at the end of the buffer, then we'd
> advance *cp until it matches *endp, and then return whatever is at *endp
> (which is nonsense, or probably a NUL) rather than returning "-1".

Good catch! This plays together interestingly with
IGNORE_WHITESPACE_AT_EOL, too. If that is set the endp is just put
further to the front, so we'd actually compare white spaces after endp.

If that flag is not set, we'd get NUL or nonsense.

> I'm out of time for tonight and not familiar enough with the color-moved
> code to come up with a reasonable test case quickly, but maybe you can
> see if that can trigger bad behavior?
>
> -Peff

Re: [PATCH v2] Documentation/git-config.txt: reword missleading sentence

[Junio C Hamano]

second.pa...@gmail.com writes:

> From: PAYRE NATHAN p1508475 

Should I assume that the name/address on the last Signed-off-by: we
see below is what you want to be known as?  As a part of school
work, I'd imagine that Matthieu wants your work to be associated
with the univ-lyon1.fr address, so perhaps you want to go the other
way around?  It's not my place to decide between the two, but it is
unusual to see that the name/address of the author (which is the
above line) does not match what is on the Signed-off-by: line.

> Change the word "bla" to "section.variable", "bla" is a placeholder
> for a variable name and it wasn't clear for everyone.
> This change clarify it.
>
> Change the appearance of 'git config section.variable {tilde}/' to
> `git config section.variable {tilde}/` to harmonize it with
> the rest of the file, this is a command line then the "`" are
> necessary.
>
> Replace "git-config" by "git config" because the command
> is not "git-config".
>
> See discussion at:
> https://public-inbox.org/git/20171002061303.horde.sl92grzcqtrv9oqkbfpe...@crashcourse.ca/
>
> Signed-off-by: MOY Matthieu 
> Signed-off-by: Daniel Bensoussan 
> Signed-off-by: Timothee Albertin 
> Signed-off-by: Nathan Payre 
> Noticed-by: rpj...@crashcourse.ca
> ---
>  Documentation/git-config.txt | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 83f86b923..2ab9e4c56 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -174,11 +174,11 @@ See also <>.
>   either --bool or --int, as described above.
>  
>  --path::
> - 'git-config' will expand leading '{tilde}' to the value of
> + 'git config' will expand leading '{tilde}' to the value of
>   '$HOME', and '{tilde}user' to the home directory for the
>   specified user.  This option has no effect when setting the
> - value (but you can use 'git config bla {tilde}/' from the
> - command line to let your shell do the expansion).
> + value (but you can use `git config section.variable {tilde}/`

Does this reference to {tilde} get expanded inside the `literal`
mark-up?  In the description for 'gitdir', we find this passage (in
Documentation/config.txt):

 * If the pattern starts with `~/`, `~` will be substituted with the
   content of the environment variable `HOME`.

So I'd expect `~` to be a safe way to get what you want, not `{tilde}`.

> + from the command line to let your shell do the expansion).
>  
>  -z::
>  --null::

Re: [PATCH] diff.c: increment buffer pointer in all code path

[Jeff King]

On Thu, Oct 12, 2017 at 08:18:37PM -0400, Jeff King wrote:

> Fix this by entering the conditional only when we actually
> see whitespace. We can apply this also to the
> IGNORE_WHITESPACE change. That code path isn't buggy
> (because it falls through to returning the next
> non-whitespace byte), but it makes the logic more clear if
> we only bother to look at whitespace flags after seeing that
> the next byte is whitespace.

I think there actually _is_ a bug in that code path, but it's unrelated
to this one. If you have whitespace at the end of the buffer, then we'd
advance *cp until it matches *endp, and then return whatever is at *endp
(which is nonsense, or probably a NUL) rather than returning "-1".

I'm out of time for tonight and not familiar enough with the color-moved
code to come up with a reasonable test case quickly, but maybe you can
see if that can trigger bad behavior?

-Peff

Re: [PATCH] diff.c: increment buffer pointer in all code path

[Jeff King]

On Thu, Oct 12, 2017 at 04:33:22PM -0700, Stefan Beller wrote:

> Peff, feel free to take ownership here. I merely made it to a patch.

I think compared to my original, it makes sense to actually wrap the
whole thing with a check for the whitespace. You can do it just in the
IGNORE_WHITESPACE_CHANGE conditional, but I think it makes more sense to
cover both. Like the patch below (view with "-w" to see the logic more
clearly).

I also tweaked the test to remove "sed -i", which isn't portable, and
fixed up a few style nits.

-- >8 --
Subject: diff: fix infinite loop with --color-moved --ignore-space-change

The --color-moved code uses next_byte() to advance through
the blob contents. When the user has asked to ignore
whitespace changes, we try to collapse any whitespace change
down to a single space.

However, we enter the conditional block whenever we see the
IGNORE_WHITESPACE_CHANGE flag, even if the next byte isn't
whitespace.

This means that the combination of "--color-moved and
--ignore-space-change" was completely broken. Worse, because
we return from next_byte() without having advanced our
pointer, the function makes no forward progress in the
buffer and loops infinitely.

Fix this by entering the conditional only when we actually
see whitespace. We can apply this also to the
IGNORE_WHITESPACE change. That code path isn't buggy
(because it falls through to returning the next
non-whitespace byte), but it makes the logic more clear if
we only bother to look at whitespace flags after seeing that
the next byte is whitespace.

Reported-by: Orgad Shaneh 
Signed-off-by: Jeff King 
---
 diff.c | 28 +++-
 t/t4015-diff-whitespace.sh |  9 +
 2 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/diff.c b/diff.c
index 69f03570ad..d76bb937c1 100644
--- a/diff.c
+++ b/diff.c
@@ -712,20 +712,22 @@ static int next_byte(const char **cp, const char **endp,
if (*cp > *endp)
return -1;
 
-   if (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE_CHANGE)) {
-   while (*cp < *endp && isspace(**cp))
-   (*cp)++;
-   /*
-* After skipping a couple of whitespaces, we still have to
-* account for one space.
-*/
-   return (int)' ';
-   }
+   if (isspace(**cp)) {
+   if (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE_CHANGE)) {
+   while (*cp < *endp && isspace(**cp))
+   (*cp)++;
+   /*
+* After skipping a couple of whitespaces,
+* we still have to account for one space.
+*/
+   return (int)' ';
+   }
 
-   if (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE)) {
-   while (*cp < *endp && isspace(**cp))
-   (*cp)++;
-   /* return the first non-ws character via the usual below */
+   if (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE)) {
+   while (*cp < *endp && isspace(**cp))
+   (*cp)++;
+   /* return the first non-ws character via the usual 
below */
+   }
}
 
retval = (unsigned char)(**cp);
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index bd0f75d9f7..87083f728f 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -1530,4 +1530,13 @@ test_expect_success 'move detection with submodules' '
test_cmp expect decoded_actual
 '
 
+test_expect_success 'move detection with whitespace changes' '
+   test_when_finished "git reset --hard" &&
+   test_seq 10 >test &&
+   git add test &&
+   sed s/3/42/ test.tmp &&
+   mv test.tmp test &&
+   git -c diff.colormoved diff --ignore-space-change -- test
+'
+
 test_done
-- 
2.15.0.rc1.381.g94fac273d4

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Junio C Hamano]

Jeff King  writes:

> ... Also
> as an aside, I think this patch means that:
>
>   git -c color.ui=always add -p
>
> is broken (as would a hypothetical "git --default-color=always add -p").
> That's sufficiently insane that I'm not sure we should care about it.

Do you mean that "'-c color.ui=always' from the command line is
passed down to the invocations of 'git' the 'add' command makes, and
would break output from 'diff-index' that 'add -i' wants to parse"?

With the breakage that motivated "downgrade only for on-disk" change
in mind, I do think that is the right behaviour.  Those third-party
scripts we broke knew how '-c color.ui=always' works and depended on
it, and I consider that the command line configuration getting
passed around as an integral part of 'how it works'.  "Fixing" it
will break them again.

Let's take it as a signal that tells us that the script writers know
what they are doing and leave it as a longish rope they can play with.

Re: [PATCH 2/2] color: discourage use of ui.color=always

[Junio C Hamano]

Jeff King  writes:

> On Thu, Oct 12, 2017 at 11:10:07AM +0900, Junio C Hamano wrote:
>
>> Warn when we read such a configuration from a file, and nudge the
>> users to spell them 'auto' instead.
>
> Hmm. On the one hand, it is nice to make people aware that their config
> isn't doing what they might think.
>
> On the other hand, if "always" is no longer a problem for anybody, do we
> need to force users to take the step to eradicate it? I dunno. Were we
> planning to eventually remove it?
>
>> @@ -320,6 +322,11 @@ int git_config_colorbool(const char *var, const char 
>> *value)
>>   * Otherwise, we're looking at on-disk config;
>>   * downgrade to auto.
>>   */
>> +if (!warn_once) {
>> +warn_once++;
>> +warning("setting '%s' to '%s' is no longer 
>> valid; "
>> +"set it to 'auto' instead", var, value);
>> +}
>
> This warn_once is sadly not enough to give non-annoying output to
> scripts that call many git commands. E.g.:
>
>   $ git config color.ui always
>   $ git add -p
>   warning: setting 'color.ui' to 'always' is no longer valid; set it to 
> 'auto' instead
>   warning: setting 'color.ui' to 'always' is no longer valid; set it to 
> 'auto' instead
>   warning: setting 'color.ui' to 'always' is no longer valid; set it to 
> 'auto' instead
>   warning: setting 'color.ui' to 'always' is no longer valid; set it to 
> 'auto' instead
>   warning: setting 'color.ui' to 'always' is no longer valid; set it to 
> 'auto' instead
>   warning: setting 'color.ui' to 'always' is no longer valid; set it to 
> 'auto' instead
>   warning: setting 'color.ui' to 'always' is no longer valid; set it to 
> 'auto' instead
>   warning: setting 'color.ui' to 'always' is no longer valid; set it to 
> 'auto' instead
>   warning: setting 'color.ui' to 'always' is no longer valid; set it to 
> 'auto' instead
>   warning: setting 'color.ui' to 'always' is no longer valid; set it to 
> 'auto' instead
>   diff --git a/file b/file
>   [...]

I am ambivalent.  

We (especially you) have kept saying that "always" is a mistake that
makes little sense, and wanted to force users to "fix" their
configuration.  But as you said, we made it not a mistake, so it is
OK to leave it as they are, I guess.

Let's drop the warning part of the change, at least.

Re: is there a truly compelling rationale for .git/info/exclude?

[Jeff King]

On Fri, Oct 13, 2017 at 01:18:00AM +0200, Johannes Schindelin wrote:

> [who I had to cull from the To:/Cc: headers, as my mailer consistently
> told me that there is no valid DNS record to route mail to
> rpj...@crashcourse.ca, which *is* weird.]

You are not the only one to mention this, so I did 60 seconds of
digging. Turns out that the MX of crashcourse.ca points to a CNAME
(mail.crashcourse.ca), which is explicitly forbidden by RFC 2181
(section 10.3). Some MTAs are picky about this and others are not (mine
isn't, so I've added Robert back to the cc so he sees this).

-Peff

[PATCH] diff.c: increment buffer pointer in all code path

[Stefan Beller]

The added test would hang up Git due to an infinite loop. The function
`next_byte()` doesn't make any forward progress in the buffer with
`--ignore-space-change`.

Fix this by only returning early when there was actual white space
to be covered, fall back to the default case at the end of the function
when there is no white space.

Reported-by: Orgad Shaneh 
Debugged-by: Jeff King 
Signed-off-by: Stefan Beller 
---

Peff, feel free to take ownership here. I merely made it to a patch.

Thanks,
Stefan

 diff.c | 12 
 t/t4015-diff-whitespace.sh |  8 
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/diff.c b/diff.c
index 69f03570ad..6fe84e6994 100644
--- a/diff.c
+++ b/diff.c
@@ -713,13 +713,17 @@ static int next_byte(const char **cp, const char **endp,
return -1;
 
if (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE_CHANGE)) {
-   while (*cp < *endp && isspace(**cp))
+   int saw_whitespace = 0;
+   while (*cp < *endp && isspace(**cp)) {
(*cp)++;
+   saw_whitespace = 1;
+   }
/*
-* After skipping a couple of whitespaces, we still have to
-* account for one space.
+* After skipping a couple of whitespaces,
+* we still have to account for one space.
 */
-   return (int)' ';
+   if (saw_whitespace)
+   return (int)' ';
}
 
if (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE)) {
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index bd0f75d9f7..c088ae86af 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -1530,4 +1530,12 @@ test_expect_success 'move detection with submodules' '
test_cmp expect decoded_actual
 '
 
+test_expect_success 'move detection with whitespace changes' '
+   test_seq 10 > test &&
+   git add test &&
+   sed -i "s/3/42/" test &&
+   git -c diff.colormoved diff --ignore-space-change -- test &&
+   git reset --hard
+'
+
 test_done
-- 
2.14.0.rc0.3.g6c2e499285

Re: is there a truly compelling rationale for .git/info/exclude?

[Johannes Schindelin]

Hi Robert,

[who I had to cull from the To:/Cc: headers, as my mailer consistently
told me that there is no valid DNS record to route mail to
rpj...@crashcourse.ca, which *is* weird.]

On Fri, 6 Oct 2017, Robert P. J. Day wrote:

> On Fri, 6 Oct 2017, Junio C Hamano wrote:
> 
> > Don't waste time by seeking a "compelling" reason.  A mere "this is
> > the most expedite way to gain convenience" back when something was
> > introduced could be an answer, and it is way too late to complain
> > about such a choice anyway.
> 
>   perfectly respectable answer ... it tells me that, between .gitignore
>   files and core.excludesFile, there's not much left for
>   .git/info/exclude to do, except in weird circumstances.

I use .git/info/exclude to keep worktrees in subdirectories of the "main"
worktree.

That's not really weird. It's just something few people do, but that's not
the same as "weird".

Ciao,
Johannes

[ANNOUNCE] Git for Windows 2.14.2(3)

[Johannes Schindelin]

Dear Git users,

It is my pleasure to announce that Git for Windows 2.14.2(3) is available from:

https://git-for-windows.github.io/

Changes since Git for Windows v2.14.2(2) (October 5th 2017)

New Features

  * Comes with Git LFS v2.3.3.

Bug Fixes

  * Re-enabled some SSHv1 ciphers since some sites (e.g. Visual Studio
Team Services) rely on them for the time being.

Filename | SHA-256
 | ---
Git-2.14.2.3-64-bit.exe | 
819120ce83b07c053f59c53e22682c14ef9ca3ac24a9a2b6a744df1c050afba1
Git-2.14.2.3-32-bit.exe | 
0b07ffb89ccd20c760cd9d4229e65ce732ed66713a4c8ac7dbedc95562c8adf6
PortableGit-2.14.2.3-64-bit.7z.exe | 
3525a7e7eb5775d38e65b2981c6e315dd97b93829bd322628a8bb2698ffdf63a
PortableGit-2.14.2.3-32-bit.7z.exe | 
c4d8829b6783ed9f725d9d112eefc31a1b103ea97e4728ab47a3636c08408155
MinGit-2.14.2.3-64-bit.zip | 
ac351c9dcdc2142b3b07b056c818927a41c32d7c615a2f7f8d18121881a3c6f0
MinGit-2.14.2.3-32-bit.zip | 
7cc1f27e1cfe79381e1a504a5fc7bc33951ac9031cd14c3bf478769d21a26cce
MinGit-2.14.2.3-busybox-64-bit.zip | 
9418629fcd1d782cda5679ccbae9df679be77660c7937ab19d1d80f742fbc763
MinGit-2.14.2.3-busybox-32-bit.zip | 
2c1ea230b90081bcb268e05a0ae9660599881cdad6c8a34e02bd1aad4182ec90
Git-2.14.2.3-64-bit.tar.bz2 | 
3d33a94473f8839b71f23fcdcf5f1f6aaf94b88d5efd169fb17ce3884da4e9df
Git-2.14.2.3-32-bit.tar.bz2 | 
510d10e59463697255210f5deb4a42135fbff5fda8fc15e1b14bc2befb92e1c1

Ciao,
Johannes

Re: [git-for-windows] [ANNOUNCE] Git for Windows 2.14.2(2)

[Johannes Schindelin]

Hi Steve,

On Mon, 9 Oct 2017, Steve Hoelzer wrote:

> On Thu, Oct 5, 2017 at 3:22 PM, Lars Schneider  
> wrote:
> >
> >> On 05 Oct 2017, at 22:02, Johannes Schindelin  
> >> wrote:
> >>
> >> Dear Git users,
> >>
> >> It is my pleasure to announce that Git for Windows 2.14.2(2) is available 
> >> from:
> >>
> >>   https://git-for-windows.github.io/
> >>
> >> Changes since Git for Windows v2.14.2 (September 26th 2017)
> >>
> >> New Features
> >>
> >>  * Comes with BusyBox v1.28.0pre.16467.b4c390e17.
> >>  * Comes with Git LFS v2.3.2.
> >
> > Just a heads-up:
> > Git LFS 2.3.2 contains a bug in the `git lfs clone` and `git lfs pull` 
> > calls:
> > https://github.com/git-lfs/git-lfs/issues/2649
> >
> > I expect 2.3.3 to be out soonish.
> 
> It's out now.

Git for Windows v2.14.2(3) (including Git LFS 2.3.3) is out now, too.

Ciao,
Johannes

Re: Out of memory with diff.colormoved enabled

[Stefan Beller]

On Thu, Oct 12, 2017 at 1:05 PM, Jeff King  wrote:
> On Thu, Oct 12, 2017 at 10:53:23PM +0300, Orgad Shaneh wrote:
>
>> There is an infinite loop when colormoved is used with --ignore-space-change:
>>
>> git init
>> seq 20 > test
>> git add test
>> sed -i 's/9/42/' test
>> git -c diff.colormoved diff --ignore-space-change -- test
>
> Thanks for an easy reproduction recipe.

Thanks here as well!

> It looks like the problem is that next_byte() doesn't make any forward
> progress in the buffer with --ignore-space-change. We try to convert
> whitespace into a single space

> (I'm not sure why, but I'm not very
> familiar with this part of the code).

(on why you don't feel familiar)
Because it is quite new, and you weren't one of the main reviewers.
2e2d5ac184 (diff.c: color moved lines differently, 2017-06-30) was also very
large, such that it is easy to overlook. Though I remember Junio and me
discussing the next_byte part quite vividly. :/

(On why it is the way it is)
Consider the three strings
one SP word
one TAB word
oneword

The first two are equal, but the third is different with
`--ignore-space-change` given. To achieve that goal,
the easiest thing to do (in my mind) was to replace
any sequence of blank characters by "a standard
space" sequence. That will make all strings with different
white space sequences compare equal, but the one
without blanks will be different.

> But if there's no space, then the
> "cp" pointer never gets advanced.

Right, because of the early return, skipping the increment of *cp

> This fixes it, but I have no idea if it's doing the right thing:
>
> diff --git a/diff.c b/diff.c
> index 69f03570ad..e8dedc7357 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -713,13 +713,17 @@ static int next_byte(const char **cp, const char **endp,
> return -1;
>
> if (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE_CHANGE)) {
> -   while (*cp < *endp && isspace(**cp))
> +   int saw_whitespace = 0;
> +   while (*cp < *endp && isspace(**cp)) {
> (*cp)++;
> +   saw_whitespace = 1;
> +   }
> /*
>  * After skipping a couple of whitespaces, we still have to
>  * account for one space.
>  */
> -   return (int)' ';
> +   if (saw_whitespace)
> +   return (int)' ';

The "else" is implicit and it falls through to
the standard case at the end of the function,
incrementing *cp, returning the character *cp
pointed at prior to being incremented.

That sounds correct.

> }
>
> if (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE)) {
>
> I guess it would be equally correct to not enter that if-block unless
> isspace(**cp).

This also sounds correct.

>
> -Peff

My Dear friend,

[Gen.sir.peter.devlin]

 My Dear, I got your message but i don;t blame you but Note is a pleasure to 
meet you as a friend and God's  sent person i can use to reach the needed poor 
children around your location and now i have considered you as the person I can 
use to reach the needed children around your Location since you have already 
known my Mind and about humanity and charity I believe together we can work for 
good by helping the poor children and less privileged with this fund, Note that 
i can't involved myself as a deceiver because am a military man also a 
Christian,

Then i will Found any delivery company around to deposit this luggage box with 
a cheque $2.7 million USD,with your information's which i have here now and 
there will contact you once i made the deposit and Note that the company will 
continue this delivery in your favor as soon as possible,

And now you will promise me that you are not going to disappoint me and the 
poor motherless or squander this fund because I'm not going to send this fund 
to a person who will squander it  just like that in terms of helping the poor 
while squandering the fund just like that, So i need your answer before i 
continue this arrangement with you because i hate disappointment and cheater 
who didn't know that poor peoples needs help thank you and am waiting to hear 
from you immediately and attachment is ID CARD,

God bless you as i urgently wait for your Response
SIR PETER DEVLIN
gen.sirpeterdev...@yahoo.com

Re: undefined reference to `pcre_jit_exec'

[Ævar Arnfjörð Bjarmason]

On Thu, Oct 12 2017, Jeff King jotted:

> On Thu, Oct 12, 2017 at 04:34:38PM -0400, Jeffrey Walton wrote:
>
>> > It looks like autoconf turns on USE_LIBPCRE1, but isn't smart enough to
>> > test NO_LIBPCRE1_JIT.
>>
>> If Git wants Jit, then I am inclined to configure PCRE to provide it.
>
> It does make things faster. OTOH, we lived for many years without it, so
> certainly it's not the end of the world to build without it.
>
> There are some numbers in the commit message of fbaceaac47 (grep: add
> support for the PCRE v1 JIT API, 2017-05-25).
>
>> A quick question if you happen to know... Does PCRE Jit cause a loss
>> of NX-stacks? If it causes a loss of NX-stacks, then I think I prefer
>> to disable it.
>
> I don't know. Ævar (cc'd) might.

Sorry, no idea. I do see some references to that in sljit (which pcre
JIT uses), but haven't tried this myself. From what I understand of how
NX works it should fail really fast if it doesn't, so it would be cool
if you could try it and report if it works.

Re: undefined reference to `pcre_jit_exec'

[Jeffrey Walton]

On Thu, Oct 12, 2017 at 4:38 PM, Jeff King  wrote:
> On Thu, Oct 12, 2017 at 04:34:38PM -0400, Jeffrey Walton wrote:
>
>> > It looks like autoconf turns on USE_LIBPCRE1, but isn't smart enough to
>> > test NO_LIBPCRE1_JIT.
>>
>> If Git wants Jit, then I am inclined to configure PCRE to provide it.
>
> It does make things faster. OTOH, we lived for many years without it, so
> certainly it's not the end of the world to build without it.
>
> There are some numbers in the commit message of fbaceaac47 (grep: add
> support for the PCRE v1 JIT API, 2017-05-25).
>
>> A quick question if you happen to know... Does PCRE Jit cause a loss
>> of NX-stacks? If it causes a loss of NX-stacks, then I think I prefer
>> to disable it.
>
> I don't know. Ævar (cc'd) might.

Thanks. Building PCRE with Jit enabled results in:

$ readelf -l /usr/local/libexec/git-core/git-credential-re| grep -i -A1 stack
  GNU_STACK  0x 0x 0x
 0x 0x  RW 10

So it looks like the NX-stacks were not lost.

Thanks again.

Jeff

Re: [PATCH v2 2/5] Update documentation for new directory and status logic

[Jameson Miller]

On 10/11/2017 10:55 PM, Junio C Hamano wrote:

Jameson Miller  writes:


Signed-off-by: Jameson Miller 
---
  Documentation/git-status.txt  | 21 +-
  Documentation/technical/api-directory-listing.txt | 27 +++
  2 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 9f3a78a36c..fc282e0a92 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -97,8 +97,27 @@ configuration variable documented in linkgit:git-config[1].
(and suppresses the output of submodule summaries when the config option
`status.submoduleSummary` is set).
  
---ignored::

+--ignored[=]::
Show ignored files as well.
++
+The mode parameter is used to specify the handling of ignored files.
+It is optional: it defaults to 'traditional'.
++
+The possible options are:
++
+   - 'traditional' - Shows ignored files and directories, unless
+ --untracked-files=all is specifed, in which case
+ individual files in ignored directories are
+ displayed.
+   - 'no'  - Show no ignored files.
+   - 'matching'- Shows ignored files and directories matching an
+ ignore pattern.
++
+When 'matching' mode is specified, paths that explicity match an
+ignored pattern are shown. If a directory matches an ignore pattern,
+then it is shown, but not paths contained in the ignored directory. If
+a directory does not match an ignore pattern, but all contents are
+ignored, then the directory is not shown, but all contents are shown.

Well explained.


diff --git a/Documentation/technical/api-directory-listing.txt 
b/Documentation/technical/api-directory-listing.txt
index 6c77b4920c..7fae00f44f 100644
--- a/Documentation/technical/api-directory-listing.txt
+++ b/Documentation/technical/api-directory-listing.txt
@@ -22,16 +22,20 @@ The notable options are:
  
  `flags`::
  
-	A bit-field of options (the `*IGNORED*` flags are mutually exclusive):

+   A bit-field of options:
  
  `DIR_SHOW_IGNORED`:::
  
-	Return just ignored files in `entries[]`, not untracked files.

+   Return just ignored files in `entries[]`, not untracked
+   files. This flag is mutually exclusive with
+   `DIR_SHOW_IGNORED_TOO`.
  
  `DIR_SHOW_IGNORED_TOO`:::
  
-	Similar to `DIR_SHOW_IGNORED`, but return ignored files in `ignored[]`

-   in addition to untracked files in `entries[]`.
+   Similar to `DIR_SHOW_IGNORED`, but return ignored files in
+   `ignored[]` in addition to untracked files in
+   `entries[]`. This flag is mutually exclusive with
+   `DIR_SHOW_IGNORED`.
  
  `DIR_KEEP_UNTRACKED_CONTENTS`:::
  
@@ -39,6 +43,21 @@ The notable options are:

untracked contents of untracked directories are also returned in
`entries[]`.
  
+`DIR_SHOW_IGNORED_TOO_MODE_MATCHING`:::

+
+   Only has meaning if `DIR_SHOW_IGNORED_TOO` is also set; if
+   this is set, returns ignored files and directories that match
+   an exclude pattern. If a directory matches an exclude pattern,
+   then the directory is returned and the contained paths are
+   not. A directory that does not match an exclude pattern will
+   not be returned even if all of its contents are ignored. In
+   this case, the contents are returned as individual entries.
++
+If this is set, files and directories that explicity match an ignore
+pattern are reported. Implicity ignored directories (directories that
+do not match an ignore pattern, but whose contents are all ignored)
+are not reported, instead all of the contents are reported.

Makes me wonder if DIR_SHOW_IGNORED* should be splt out into a short
enum.  We have:

  - Do not show ignored ones (0)

  - Collect ignored ones (DIR_SHOW_IGNORED)

  - Collect ignored and untracked ones separately (DIR_SHOW_IGNORED_TOO)

  - Collect ignored and duntracked ones separately, but limit them to
those mach exclude patterns explicitly 
(DIR_SHOW_IGNORED_TOO|...MODE_MATCHING)

so we need two bits to fit a 4-possiblity enum.

Then we do not have to worry about saying quirky things like A and B
are incompatible, and C makes sense only when B is set, etc.

I could see a potential for other values for the "show ignored
mode" flags - for example: "NORMAL", "MATCHING", "ALL"... Instead
of making more change at this point in time, how would you feel
about waiting until the next change in this area.

If you would prefer for me to change these enums now, I can do
that.




  `DIR_COLLECT_IGNORED`:::
  
  	Special mode for git-add. Return ignored files in `ignored[]` and

Re: undefined reference to `pcre_jit_exec'

[Jeff King]

On Thu, Oct 12, 2017 at 04:34:38PM -0400, Jeffrey Walton wrote:

> > It looks like autoconf turns on USE_LIBPCRE1, but isn't smart enough to
> > test NO_LIBPCRE1_JIT.
> 
> If Git wants Jit, then I am inclined to configure PCRE to provide it.

It does make things faster. OTOH, we lived for many years without it, so
certainly it's not the end of the world to build without it.

There are some numbers in the commit message of fbaceaac47 (grep: add
support for the PCRE v1 JIT API, 2017-05-25).

> A quick question if you happen to know... Does PCRE Jit cause a loss
> of NX-stacks? If it causes a loss of NX-stacks, then I think I prefer
> to disable it.

I don't know. Ævar (cc'd) might.

-Peff

Re: undefined reference to `pcre_jit_exec'

[Jeffrey Walton]

On Thu, Oct 12, 2017 at 4:10 PM, Jeff King  wrote:
> On Thu, Oct 12, 2017 at 04:06:11PM -0400, Jeffrey Walton wrote:
>
>> I have a script to build Git on some old platforms to ease testing.
>> Old platforms include CentOS 5. The script is available at
>> https://github.com/noloader/Build-Scripts/blob/master/build-ssh.sh.
>>
>> It looks like something got knocked loose recently. I'm seeing several
>> of these when building with PCRE 8.41 with Git 2.14.2. Old and new
>> platforms are witnessing it. I observe it on CentOS 5 with GCC 4.1;
>> and Fedora 26 with GCC 7.2.
>>
>> ...
>> LINK git-credential-store
>> libgit.a(grep.o): In function `pcre1match':
>> grep.c:(.text+0x1219): undefined reference to `pcre_jit_exec'
>> collect2: error: ld returned 1 exit status
>> make: *** [Makefile:2145: git-credential-store] Error 1
>
> Maybe:
>
>   $ git grep -h -B5 -A1 pcre_jit Makefile
>   # When using USE_LIBPCRE1, define NO_LIBPCRE1_JIT if the PCRE v1
>   # library is compiled without --enable-jit. We will auto-detect
>   # whether the version of the PCRE v1 library in use has JIT support at
>   # all, but we unfortunately can't auto-detect whether JIT support
>   # hasn't been compiled in in an otherwise JIT-supporting version. If
>   # you have link-time errors about a missing `pcre_jit_exec` define
>   # this, or recompile PCRE v1 with --enable-jit.

Ah, thanks. A quick search did not find that comment. And it did not
occur to me to re-run --help to read about it.

> It looks like autoconf turns on USE_LIBPCRE1, but isn't smart enough to
> test NO_LIBPCRE1_JIT.

If Git wants Jit, then I am inclined to configure PCRE to provide it.

A quick question if you happen to know... Does PCRE Jit cause a loss
of NX-stacks? If it causes a loss of NX-stacks, then I think I prefer
to disable it.

Jeff

Re: [PATCH v1 1/1] completion: add remaining flags to checkout

[Johannes Sixt]

Am 12.10.2017 um 18:50 schrieb Johannes Sixt:

Am 12.10.2017 um 14:20 schrieb Thomas Braun:

+    --force --ignore-skip-worktree-bits --ignore-other-worktrees


Destructive and dangerous options are typically not offered by command 
completion. You should omit all three in the line above, IMO.


Ah, no, only --force and --ignore-other-worktrees are dangerous, 
--ignore-skip-worktree-bits is not.


-- Hannes

Re: [PATCH v2 0/5] Teach Status options around showing ignored files

[Jameson Miller]

On 10/11/2017 10:33 PM, Junio C Hamano wrote:

Jameson Miller  writes:


Changes in v2:

Addressed review comments from the original series:

* Made fixes / simplifications suggested for documentation

* Removed test_tick from test scripts

* Merged 2 commits (as suggested)

Jameson Miller (5):
   Teach status options around showing ignored files
   Update documentation for new directory and status logic
   Add tests for git status `--ignored=matching`
   Expand support for ignored arguments on status
   Add tests around status handling of ignored arguments

Please make sure these titles mix well when they appear together
with changes from other people that are completely unrelated to
them.  Keep in mind that your "git status" is *not* the most
important thing in the world (of course, it is no less important
than others, either).  Perhaps

 status: add new options to show ignored files differently
 status: document logic to show new directory
 status: test --ignored=matching
Thank you for the suggestions - I will update the  commit titles in my 
next round.


etc.  Rules of thumb:

  (1) choose ": " prefix appropriately
  (2) keep them short and to the point
  (3) word that follow ": " prefix is not capitalized
  (4) no need for full-stop at the end of the title

Re: [PATCH v2 5/5] Add tests around status handling of ignored arguments

[Jameson Miller]

On 10/12/2017 12:06 AM, Junio C Hamano wrote:

Jameson Miller  writes:


Add tests for status handling of '--ignored=matching` and
`--untracked-files=normal`.

Signed-off-by: Jameson Miller 
---

Hmph, having some tests in 3/5, changes in 4/5 and even more tests
in 5/5 makes me as a reader a bit confused, as the description for
these two test patches does not make it clear how they are related
and how they are different.  Is it that changes in 1/5 alone does
not fulfill the promise made by documentation added at 2/5 so 3/5
only has tests for behaviour that works with 1/5 alone but is broken
with respect to what 2/5 claims until 4/5 is applied, and these "not
working with 1/5 alone, but works after 4/5" are added in this step?


Correct. The changes in 1/5 are to implement "--ignored=matching"
with "--untracked-files=all" with corresponding tests in 3/5. The
changes in 4/5 are to implement "--ignored=matching"
with "--untracked-files=normal" and the corresponding tests are
in 5/5.

Do you have a preference on how I organized this work? I see
several possible ways to split up this work. Maybe it would be
less confusing to have the implementation in the first two
commits, then 1 commit with all the tests, and then a commit with
the documentation? I think it makes sense to have the logic for
the different flag combinations split into their own commits, but
I am open to any suggestions.




  t/t7519-ignored-mode.sh | 60 -
  1 file changed, 59 insertions(+), 1 deletion(-)

diff --git a/t/t7519-ignored-mode.sh b/t/t7519-ignored-mode.sh
index 76e91427b0..6be7701d79 100755
--- a/t/t7519-ignored-mode.sh
+++ b/t/t7519-ignored-mode.sh
@@ -116,10 +116,68 @@ test_expect_success 'Verify status behavior on ignored 
folder containing tracked
ignored_dir/ignored_1.ign ignored_dir/ignored_2.ign \
ignored_dir/tracked &&
git add -f ignored_dir/tracked &&
-   test_tick &&
git commit -m "Force add file in ignored directory" &&
git status --porcelain=v2 --ignored=matching --untracked-files=all >output 
&&
test_i18ncmp expect output
  '
  
+test_expect_success 'Verify matching ignored files with --untracked-files=normal' '

+   test_when_finished "git clean -fdx" &&
+   cat >expect <<-\EOF &&
+   ? expect
+   ? output
+   ? untracked_dir/
+   ! ignored_dir/
+   ! ignored_files/ignored_1.ign
+   ! ignored_files/ignored_2.ign
+   EOF
+
+   mkdir ignored_dir ignored_files untracked_dir &&
+   touch ignored_dir/ignored_1 ignored_dir/ignored_2 \
+   ignored_files/ignored_1.ign ignored_files/ignored_2.ign \
+   untracked_dir/untracked &&
+   git status --porcelain=v2 --ignored=matching --untracked-files=normal >output 
&&
+   test_i18ncmp expect output
+'
+
+test_expect_success 'Verify matching ignored files with 
--untracked-files=normal' '
+   test_when_finished "git clean -fdx" &&
+   cat >expect <<-\EOF &&
+   ? expect
+   ? output
+   ? untracked_dir/
+   ! ignored_dir/
+   ! ignored_files/ignored_1.ign
+   ! ignored_files/ignored_2.ign
+   EOF
+
+   mkdir ignored_dir ignored_files untracked_dir &&
+   touch ignored_dir/ignored_1 ignored_dir/ignored_2 \
+   ignored_files/ignored_1.ign ignored_files/ignored_2.ign \
+   untracked_dir/untracked &&
+   git status --porcelain=v2 --ignored=matching --untracked-files=normal >output 
&&
+   test_i18ncmp expect output
+'
+
+test_expect_success 'Verify status behavior on ignored folder containing 
tracked file' '
+   test_when_finished "git clean -fdx && git reset HEAD~1 --hard" &&
+   cat >expect <<-\EOF &&
+   ? expect
+   ? output
+   ! ignored_dir/ignored_1
+   ! ignored_dir/ignored_1.ign
+   ! ignored_dir/ignored_2
+   ! ignored_dir/ignored_2.ign
+   EOF
+
+   mkdir ignored_dir &&
+   touch ignored_dir/ignored_1 ignored_dir/ignored_2 \
+   ignored_dir/ignored_1.ign ignored_dir/ignored_2.ign \
+   ignored_dir/tracked &&
+   git add -f ignored_dir/tracked &&
+   git commit -m "Force add file in ignored directory" &&
+   git status --porcelain=v2 --ignored=matching --untracked-files=normal >output 
&&
+   test_i18ncmp expect output
+'
+
  test_done

Re: [PATCH v2 1/5] Teach status options around showing ignored files

[Jameson Miller]

On 10/11/2017 10:49 PM, Junio C Hamano wrote:

Jameson Miller  writes:


This change teaches the status command more options to control which
ignored files are reported independently of the which untracked files
are reported by allowing the `--ignored` option to take additional
arguments.  Currently, the shown ignored files are linked to how
untracked files are reported. Both ignored and untracked files are
controlled by arguments to `--untracked-files` option. This makes it
impossible to show all untracked files individually, but show ignored
"files and directories" (that is, ignored directories are shown as 1
entry, instead of having all contents shown).

The description makes sense.  And it makes sense to show a directory
known to contain only ignored paths as just one entry, instead of
exploding that to individual files.


Our application (Visual Studio) has a specific set of requirements
about how it wants untracked / ignored files reported by git status.

This sentence does not read well.  VS has no obligation to read from
"git status", so there is no "specific set of requirements" that
make us care.  If the output from "status" is insufficient you could
be reading from "ls-files --ignored", for example, if you want more
details than "git status" gives you.

The sentence, and the paragraph that immediately follows it, need a
serious attitude adjustment ;-), even though it is good as read as
an explanation of what the proposed output wants to show.


It was not my intention to have this paragraph come across this
way. I meant to express the ideal format of data that our
application would like (from whatever source) as motivation for
why we are proposing these changes. I will reword this
paragraph to remove any unintended implication otherwise.


The reason for controlling these behaviors separately is that there
can be a significant performance impact to scanning the contents of
excluded directories. Additionally, knowing whether a directory
explicitly matches an exclude pattern can help the application make
decisions about how to handle the directory. If an ignored directory
itself matches an exclude pattern, then the application will know that
any files underneath the directory must be ignored as well.

While the above description taken standalone makes sense, doesn't
the "we want all paths listed, without abbreviated to the directory
and requiring the reader to infer from the fact that aidrectory is
shown that everything in it are ignored" the log message stated
earlier contradict another change you earlier sent, that avoids
scanning a directory that is known to be completely untracked
(i.e. no path under it in the index) and return early once an
untracked file is found in it?


My first set of changes introduced a perf optimization without
any functional changes. The perf optimization was to avoid
scanning a directory that is known to be ignored (i.e no path
under it in the index and the directory matches an exclude
pattern). It returns early once any file is found in it. Any file
found must be ignored, as it is contained in an ignored
directory.

This second set of changes is to allow optional decoupling of how
ignored and untracked items are reported.




Signed-off-by: Jameson Miller 
---
  builtin/commit.c | 31 +--
  dir.c| 24 
  dir.h|  3 ++-
  wt-status.c  | 11 ---
  wt-status.h  |  8 +++-
  5 files changed, 66 insertions(+), 11 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d75b3805ea..98d84d0277 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -118,7 +118,7 @@ static int edit_flag = -1; /* unspecified */
  static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
  static int config_commit_verbose = -1; /* unspecified */
  static int no_post_rewrite, allow_empty_message;
-static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
+static char *untracked_files_arg, *force_date, *ignore_submodule_arg, 
*ignored_arg;
  static char *sign_commit;
  
  /*

@@ -139,7 +139,7 @@ static const char *cleanup_arg;
  static enum commit_whence whence;
  static int sequencer_in_use;
  static int use_editor = 1, include_status = 1;
-static int show_ignored_in_status, have_option_m;
+static int have_option_m;
  static struct strbuf message = STRBUF_INIT;
  
  static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;

@@ -1075,6 +1075,19 @@ static const char *find_author_by_nickname(const char 
*name)
die(_("--author '%s' is not 'Name ' and matches no existing 
author"), name);
  }
  
+static void handle_ignored_arg(struct wt_status *s)

+{
+   if (!ignored_arg)
+   ; /* default already initialized */
+   else if (!strcmp(ignored_arg, "traditional"))
+   s->show_ignored_mode = SHOW_TRADITIONAL_IGNORED;
+   else if (!strcmp(ignored_arg, "no"))
+  

Re: undefined reference to `pcre_jit_exec'

[Jeff King]

On Thu, Oct 12, 2017 at 04:06:11PM -0400, Jeffrey Walton wrote:

> I have a script to build Git on some old platforms to ease testing.
> Old platforms include CentOS 5. The script is available at
> https://github.com/noloader/Build-Scripts/blob/master/build-ssh.sh.
> 
> It looks like something got knocked loose recently. I'm seeing several
> of these when building with PCRE 8.41 with Git 2.14.2. Old and new
> platforms are witnessing it. I observe it on CentOS 5 with GCC 4.1;
> and Fedora 26 with GCC 7.2.
> 
> ...
> LINK git-credential-store
> libgit.a(grep.o): In function `pcre1match':
> grep.c:(.text+0x1219): undefined reference to `pcre_jit_exec'
> collect2: error: ld returned 1 exit status
> make: *** [Makefile:2145: git-credential-store] Error 1

Maybe:

  $ git grep -h -B5 -A1 pcre_jit Makefile
  # When using USE_LIBPCRE1, define NO_LIBPCRE1_JIT if the PCRE v1
  # library is compiled without --enable-jit. We will auto-detect
  # whether the version of the PCRE v1 library in use has JIT support at
  # all, but we unfortunately can't auto-detect whether JIT support
  # hasn't been compiled in in an otherwise JIT-supporting version. If
  # you have link-time errors about a missing `pcre_jit_exec` define
  # this, or recompile PCRE v1 with --enable-jit.

It looks like autoconf turns on USE_LIBPCRE1, but isn't smart enough to
test NO_LIBPCRE1_JIT.

-Peff

undefined reference to `pcre_jit_exec'

[Jeffrey Walton]

Hi Everyone,

I have a script to build Git on some old platforms to ease testing.
Old platforms include CentOS 5. The script is available at
https://github.com/noloader/Build-Scripts/blob/master/build-ssh.sh.

It looks like something got knocked loose recently. I'm seeing several
of these when building with PCRE 8.41 with Git 2.14.2. Old and new
platforms are witnessing it. I observe it on CentOS 5 with GCC 4.1;
and Fedora 26 with GCC 7.2.

...
LINK git-credential-store
libgit.a(grep.o): In function `pcre1match':
grep.c:(.text+0x1219): undefined reference to `pcre_jit_exec'
collect2: error: ld returned 1 exit status
make: *** [Makefile:2145: git-credential-store] Error 1

Jeff



./configure ...

config.status: executing config.mak.autogen commands
* new build flags
* new prefix flags
* new link flags
GEN common-cmds.h
CC hex.o
CC ident.o
CC kwset.o
CC levenshtein.o
CC line-log.o
CC line-range.o
CC list-objects.o
CC ll-merge.o
CC lockfile.o
CC log-tree.o
CC mailinfo.o
CC mailmap.o
CC match-trees.o
CC merge.o
CC merge-blobs.o
CC merge-recursive.o
CC mergesort.o
CC mru.o
CC name-hash.o
CC notes.o
CC notes-cache.o
CC notes-merge.o
CC notes-utils.o
CC object.o
CC oidset.o
CC pack-bitmap.o
CC pack-bitmap-write.o
CC pack-check.o
CC pack-objects.o
CC pack-revindex.o
CC pack-write.o
CC pager.o
CC parse-options.o
CC parse-options-cb.o
CC patch-delta.o
CC patch-ids.o
CC path.o
CC pathspec.o
CC pkt-line.o
CC preload-index.o
CC pretty.o
CC prio-queue.o
CC progress.o
CC prompt.o
CC quote.o
CC reachable.o
CC read-cache.o
CC reflog-walk.o
CC refs.o
CC refs/files-backend.o
CC refs/iterator.o
CC refs/ref-cache.o
CC ref-filter.o
CC remote.o
CC replace_object.o
CC repository.o
CC rerere.o
CC resolve-undo.o
CC revision.o
CC run-command.o
CC send-pack.o
CC sequencer.o
CC server-info.o
CC setup.o
CC sha1-array.o
CC sha1-lookup.o
CC sha1_file.o
CC sha1_name.o
CC shallow.o
CC sideband.o
CC sigchain.o
CC split-index.o
CC strbuf.o
CC streaming.o
CC string-list.o
CC submodule.o
CC submodule-config.o
CC sub-process.o
CC symlinks.o
CC tag.o
CC tempfile.o
CC tmp-objdir.o
CC trace.o
CC trailer.o
CC transport.o
CC transport-helper.o
CC tree-diff.o
CC tree.o
CC tree-walk.o
CC unpack-trees.o
CC url.o
CC urlmatch.o
CC usage.o
CC userdiff.o
CC utf8.o
CC varint.o
CC versioncmp.o
CC walker.o
CC wildmatch.o
CC worktree.o
CC wrapper.o
CC write_or_die.o
CC ws.o
CC wt-status.o
CC xdiff-interface.o
CC zlib.o
CC unix-socket.o
CC sha1dc/sha1.o
CC sha1dc/ubc_check.o
CC thread-utils.o
CC compat/fopen.o
CC compat/strlcpy.o
CC compat/qsort_s.o
CC xdiff/xdiffi.o
CC xdiff/xprepare.o
CC xdiff/xutils.o
CC xdiff/xemit.o
CC xdiff/xmerge.o
CC xdiff/xpatience.o
CC xdiff/xhistogram.o
CC daemon.o
CC fast-import.o
CC http-backend.o
CC imap-send.o
CC http.o
CC sh-i18n--envsubst.o
CC shell.o
CC show-index.o
CC upload-pack.o
CC remote-testsvn.o
CC vcs-svn/line_buffer.o
CC vcs-svn/sliding_window.o
CC vcs-svn/fast_export.o
CC vcs-svn/svndiff.o
CC vcs-svn/svndump.o
CC http-walker.o
CC http-fetch.o
CC credential-cache.o
CC credential-cache--daemon.o
CC remote-curl.o
* new script parameters
* new perl-specific parameters
* new Python interpreter location
GEN git-instaweb
GEN git-mergetool--lib
GEN git-parse-remote
GEN git-rebase--am
GEN git-rebase--interactive
GEN git-rebase--merge
GEN git-sh-setup
GEN git-sh-i18n
CC git.o
CC builtin/add.o
CC builtin/am.o
CC builtin/annotate.o
CC builtin/apply.o
CC builtin/archive.o
CC builtin/bisect--helper.o
CC builtin/blame.o
CC builtin/branch.o
CC builtin/bundle.o
CC builtin/cat-file.o
CC builtin/check-attr.o
CC builtin/check-ignore.o
CC builtin/check-mailmap.o
CC builtin/check-ref-format.o
CC builtin/checkout-index.o
CC builtin/checkout.o
CC builtin/clean.o
CC builtin/clone.o
CC builtin/column.o
CC builtin/commit-tree.o
CC builtin/commit.o
CC builtin/config.o
CC builtin/count-objects.o
CC builtin/credential.o
CC builtin/describe.o
CC builtin/diff-files.o
CC builtin/diff-index.o
CC builtin/diff-tree.o
CC builtin/diff.o
CC builtin/difftool.o
CC builtin/fast-export.o
CC builtin/fetch-pack.o
CC builtin/fetch.o
CC builtin/fmt-merge-msg.o
CC builtin/for-each-ref.o
CC builtin/fsck.o
CC builtin/gc.o
CC builtin/get-tar-commit-id.o

Re: Out of memory with diff.colormoved enabled

[Jeff King]

On Thu, Oct 12, 2017 at 10:53:23PM +0300, Orgad Shaneh wrote:

> There is an infinite loop when colormoved is used with --ignore-space-change:
> 
> git init
> seq 20 > test
> git add test
> sed -i 's/9/42/' test
> git -c diff.colormoved diff --ignore-space-change -- test

Thanks for an easy reproduction recipe.

It looks like the problem is that next_byte() doesn't make any forward
progress in the buffer with --ignore-space-change. We try to convert
whitespace into a single space (I'm not sure why, but I'm not very
familiar with this part of the code). But if there's no space, then the
"cp" pointer never gets advanced.

This fixes it, but I have no idea if it's doing the right thing:

diff --git a/diff.c b/diff.c
index 69f03570ad..e8dedc7357 100644
--- a/diff.c
+++ b/diff.c
@@ -713,13 +713,17 @@ static int next_byte(const char **cp, const char **endp,
return -1;
 
if (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE_CHANGE)) {
-   while (*cp < *endp && isspace(**cp))
+   int saw_whitespace = 0;
+   while (*cp < *endp && isspace(**cp)) {
(*cp)++;
+   saw_whitespace = 1;
+   }
/*
 * After skipping a couple of whitespaces, we still have to
 * account for one space.
 */
-   return (int)' ';
+   if (saw_whitespace)
+   return (int)' ';
}
 
if (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE)) {

I guess it would be equally correct to not enter that if-block unless
isspace(**cp).

-Peff

Out of memory with diff.colormoved enabled

[Orgad Shaneh]

Hi,

git version 2.15.0.rc0 (from debian sid package)

There is an infinite loop when colormoved is used with --ignore-space-change:

git init
seq 20 > test
git add test
sed -i 's/9/42/' test
git -c diff.colormoved diff --ignore-space-change -- test

- Orgad

Re: [PATCH v2 2/3] for-each-ref: let upstream/push optionally remote ref name

[Johannes Schindelin]

Hi Junio,

On Wed, 11 Oct 2017, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > Johannes Schindelin  writes:
> >
> >> Subject: Re: [PATCH v2 2/3] for-each-ref: let upstream/push optionally 
> >> remote ref name
> >
> > No verb?  s/optionally/report/ perhaps?
> 
> I just noticed that I didn't tweak the copy I have in my tree after
> sending this message, but now I did s/optionally/& report the/;

Yes, sorry for not reading this earlier, this is what I meant the commit
subject to say.

> I am still puzzled by the hunk below, though.
> 
> >> @@ -1262,6 +1265,14 @@ static void fill_remote_ref_details(struct 
> >> used_atom *atom, const char *refname,
> >>*s = xstrdup(remote);
> >>else
> >>*s = "";
> >> +  } else if (atom->u.remote_ref.option == RR_REMOTE_REF) {
> >> +  int explicit, for_push = starts_with(atom->name, "push");
> >
> > Hmph, the previous step got rid of starts_with() rather nicely by
> > introducing atom->u.remote_ref.push bit; can't we do the same in
> > this step?

Right, I forgot to edit this. FWIW I have this in my branch now:

-- snip --
[PATCH] squash! for-each-ref: let upstream/push optionally remote ref
 name

for-each-ref: let upstream/push optionally report the remote ref name

There are times when scripts want to know not only the name of the
push branch on the remote, but also the name of the branch as known
by the remote repository.

An example of this is when a tool wants to push to the very same branch
from which it would pull automatically, i.e. the `` and the ``
in `git push  :` would be provided by
`%(upstream:remotename)` and `%(upstream:remoteref)`, respectively.

This patch offers the new suffix :remoteref for the `upstream` and `push`
atoms, allowing to show exactly that. Example:

$ cat .git/config
...
[remote "origin"]
url = https://where.do.we.come/from
fetch = refs/heads/*:refs/remote/origin/*
[branch "master"]
remote = origin
merge = refs/heads/master
[branch "develop/with/topics"]
remote = origin
merge = refs/heads/develop/with/topics
...

$ git for-each-ref \
--format='%(push) %(push:remoteref)' \
refs/heads
refs/remotes/origin/master refs/heads/master
refs/remotes/origin/develop/with/topics refs/heads/develop/with/topics

Signed-off-by: J Wyman 
Signed-off-by: Johannes Schindelin 
---
 ref-filter.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 2556c7885de..6ab12658cb3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1268,9 +1268,11 @@ static void fill_remote_ref_details(struct used_atom 
*atom, const char *refname,
else
*s = "";
} else if (atom->u.remote_ref.option == RR_REMOTE_REF) {
-   int explicit, for_push = starts_with(atom->name, "push");
-   const char *merge = remote_ref_for_branch(branch, for_push,
- );
+   int explicit;
+   const char *merge;
+
+   merge = remote_ref_for_branch(branch, atom->u.remote_ref.push,
+ );
if (explicit)
*s = xstrdup(merge);
else
-- 
2.14.2.windows.2
-- snap --

(The funny "squash!" followed by a complete version of the rewritten
commit message is what I do until I -- or anybody else -- comes up with a
patch to implement support for "reword!".)

I'll let this simmer until next week and send out a new iteration of the
patch series then.

Thanks,
Dscho

Re: (Some?) control codes not escaped when using `git add -p`

[Jeff King]

On Thu, Oct 12, 2017 at 12:59:06PM -0500, Mahmoud Al-Qudsi wrote:

> Hello list,
> 
> Running git 2.7.4, I’ve run into an issue where control codes that would
> normally be escaped when using `git diff` are not similarly escaped when using
> `git add -p`.
> 
> As a concrete example, I have a text file including the following "text":
> 
> :map ^[[H 
> :map ^[[5~ ^B "page up
> :map ^[[6~ ^F "page down

The diffs generated for "git diff" and "git add -p" are done by the
exact same code. And that code doesn't do any escaping or cleverness; it
will output the raw bytes of the difference.

What is likely causing the different in what you see is that "git diff".
outputs through a pager, and the snippets of "add -p" do not. The
default pager, "less", does escape some control codes (but with the -R
option, which git uses by default, it passes through colors).

Try:

  git --no-pager diff

or:

  GIT_PAGER=cat git diff

and you'll likely see output similar to what you get with "add -p".

The reason that "add -p" doesn't go through a pager by default is simply
that it would be annoying, since we show snippets and then ask for user
input.

However, since Git v2.9.0, "add -p" (and all of the interactive commands
like "checkout -p", etc) know about the interactive.diffFilter config
option. You could use that to munge the results however you like. E.g.:

  # we can't use [:cntrl:] here because we want to keep newlines.
  # likewise, we want to keep ESC for color codes
  git config interactive.diffFilter "tr '\000-\011\013-\032\034-\037' '?'"

-Peff

[PATCH v3] pull: pass --signoff/--no-signoff to "git merge"

[W. Trevor King]

merge can take --signoff, but without pull passing --signoff down, it
is inconvenient, so let's pass it through.

The order of options in merge-options.txt is mostly alphabetical by
long option since 7c85d274 (Documentation/merge-options.txt: order
options in alphabetical groups, 2009-10-22).  The long-option bit
didn't make it into the commit message, but it's under the fold in
[1].  I've put --signoff between --log and --stat to preserve the
alphabetical order.

[1]: https://public-inbox.org/git/87iqe7zspn@jondo.cante.net/

Helped-by: Junio C Hamano 
Signed-off-by: W. Trevor King 
---
Changes since v2 [1]:

* Replace the two motivational paragraphs with Junio's suggested
  sentence [2].
* Drop test_hash_trailer in favor of --pretty='format:%(trailers)'
  [3].  It turns out that the builtin tooling I was looking for while
  working on test_hash_trailer already exists :).

This patch (like v1 and v2) is based on v2.15.0-rc1, although I expect
it will apply cleanly to anything in that vicinity.

Cheers,
Trevor

[1]: 
https://public-inbox.org/git/51d67d6d707182d4973d9961ab29358f26c4988a.1507796638.git.wk...@tremily.us/
[2]: https://public-inbox.org/git/xmqqk200znel@gitster.mtv.corp.google.com/
[3]: https://public-inbox.org/git/xmqq7ew0zkqv@gitster.mtv.corp.google.com/

 Documentation/git-merge.txt |  8 
 Documentation/merge-options.txt | 10 +
 builtin/pull.c  |  6 ++
 t/t5521-pull-options.sh | 45 +
 4 files changed, 61 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 4df6431c34..0ada8c856b 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -64,14 +64,6 @@ OPTIONS
 ---
 include::merge-options.txt[]
 
---signoff::
-   Add Signed-off-by line by the committer at the end of the commit
-   log message.  The meaning of a signoff depends on the project,
-   but it typically certifies that committer has
-   the rights to submit this work under the same license and
-   agrees to a Developer Certificate of Origin
-   (see http://developercertificate.org/ for more information).
-
 -S[]::
 --gpg-sign[=]::
GPG-sign the resulting merge commit. The `keyid` argument is
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 4e32304301..f394622d65 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -51,6 +51,16 @@ set to `no` at the beginning of them.
 With --no-log do not list one-line descriptions from the
 actual commits being merged.
 
+--signoff::
+--no-signoff::
+   Add Signed-off-by line by the committer at the end of the commit
+   log message.  The meaning of a signoff depends on the project,
+   but it typically certifies that committer has
+   the rights to submit this work under the same license and
+   agrees to a Developer Certificate of Origin
+   (see http://developercertificate.org/ for more information).
++
+With --no-signoff do not add a Signed-off-by line.
 
 --stat::
 -n::
diff --git a/builtin/pull.c b/builtin/pull.c
index 6f772e8a22..0413c78a3a 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -86,6 +86,7 @@ static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static enum rebase_type opt_rebase = -1;
 static char *opt_diffstat;
 static char *opt_log;
+static char *opt_signoff;
 static char *opt_squash;
 static char *opt_commit;
 static char *opt_edit;
@@ -142,6 +143,9 @@ static struct option pull_options[] = {
OPT_PASSTHRU(0, "log", _log, N_("n"),
N_("add (at most ) entries from shortlog to merge commit 
message"),
PARSE_OPT_OPTARG),
+   OPT_PASSTHRU(0, "signoff", _signoff, NULL,
+   N_("add Signed-off-by:"),
+   PARSE_OPT_OPTARG),
OPT_PASSTHRU(0, "squash", _squash, NULL,
N_("create a single commit instead of doing a merge"),
PARSE_OPT_NOARG),
@@ -594,6 +598,8 @@ static int run_merge(void)
argv_array_push(, opt_diffstat);
if (opt_log)
argv_array_push(, opt_log);
+   if (opt_signoff)
+   argv_array_push(, opt_signoff);
if (opt_squash)
argv_array_push(, opt_squash);
if (opt_commit)
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index ded8f98dbe..c19d8dbc9d 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -165,4 +165,49 @@ test_expect_success 'git pull --allow-unrelated-histories' 
'
)
 '
 
+test_expect_success 'git pull does not add a sign-off line' '
+   test_when_finished "rm -fr src dst actual" &&
+   git init src &&
+   test_commit -C src one &&
+   git clone src dst &&
+   test_commit -C src two &&
+   git -C dst pull --no-ff &&
+   git -C dst show -s --pretty="format:%(trailers)" HEAD 

(Some?) control codes not escaped when using `git add -p`

[Mahmoud Al-Qudsi]

Hello list,

Running git 2.7.4, I’ve run into an issue where control codes that would
normally be escaped when using `git diff` are not similarly escaped when using
`git add -p`.

As a concrete example, I have a text file including the following "text":

:map ^[[H 
:map ^[[5~ ^B "page up
:map ^[[6~ ^F "page down

Except each ^x above is the literal ctrl+x (i.e. ctrl+v followed by ctrl+x).
These are not lines that have been added or removed from the document, they're
just context lines.

Using `git diff`, these special characters are elided from the diff output
(though the latter two lines cause special coloring in the diff output so
perhaps they're not being entirely escaped?), but when using `git add -p` upon
reaching the chunk in question my terminal interprets a literal "page up" input
and corrupts the output.

Here's a screenshot of what I see when I use `git diff`:
https://i.imgur.com/FOXWEIi.png

And here's what I see when use `git add -p`:
https://i.imgur.com/i5hqhFX.png

As you can see, in the second example the cursor is a few lines from the top of
the screen, as the text output started midway down and then jumped to the start
and continued from there on encountering the literal 'Page Up' sequence in the
diff'd text.

I'm not sure _what_ the correct approach would be here (does git faithfully
print whatever it finds in the file or does it attempt to escape it instead?)
but it seems to me that the lack of consistency between the two commands is a
bug as whichever approach is taken, a context line in `git diff` should surely
be output to the terminal in the same way as a context line in `git add -p`?

The obvious solution is to embrace isatty(2) religiously, but I'm not
sure how the
everyone else feels about that (or if it's already used elsewhere).

Anyway, I'm sure I'm not the only one to run into this. Seeking options and
interested in the various viewpoints on how this should be correctly handled
(or explanations for why it's already correct as-is).

Cheers,

Mahmoud Al-Qudsi
NeoSmart Technologies

Re: No log --no-decorate completion?

[Max Rothman]

To be fair, other --no* options complete, it's just --no-decorate,
--no-walk, --no-abbrev-commit, --no-expand-tabs, --no-patch,
--no-indent-heuristic, and --no-textconv that's missing.

For example:
$ git log --no
--no-color --no-max-parents   --no-min-parents   --no-prefix
 --not
--no-ext-diff  --no-merges--no-notes --no-renames
 --notes

Thanks,
Max

On Wed, Oct 11, 2017 at 2:09 PM, Stefan Beller  wrote:
> On Wed, Oct 11, 2017 at 7:47 AM, Max Rothman  wrote:
>> I recently noticed that in the git-completion script, there's
>> completion for --decorate={full,yes,no} for git log and family, but
>> not for --no-decorate. Is that intentional? If not, I *think* I see
>> how it could be added.
>>
>> Thanks,
>> Max
>
> Using git-blame, I found af4e9e8c87 (completion: update am, commit, and log,
> 2009-10-07) as well as af16bdaa3f (completion: fix and update 'git log
> --decorate='
> options, 2015-05-01), both of their commit messages do not discuss leaving out
> --no-decorate intentionally.
>
> If you give --no- you'd get more than just the completion to 
> --no-decorate,
> but all the negated options, I would assume?
>
> So maybe that is why no one added the negated options, yet?
>
> Thanks,
> Stefan

Re: Enhancement request: git-push: Allow (configurable) default push-option

[Stefan Beller]

On Thu, Oct 12, 2017 at 9:32 AM, Marius Paliga  wrote:
> Subject: [PATCH] Added support for new configuration parameter push.pushOption
>
> builtin/push.c: add push.pushOption config
>
> Currently push options need to be given explicitly, via
> the command line as "git push --push-option".
>
> The UX of Git would be enhanced if push options could be
> configured instead of given each time on the command line.
>
> Add the config option push.pushOption, which is a multi
> string option, containing push options that are sent by default.
>
> When push options are set in the system wide config
> (/etc/gitconfig), they can be unset later in the more specific
> repository config by setting the string to the empty string.

Now that I review this patch, this is nice information and can
remain in the commit message, but it would be more useful
in the Documentation as that is where the users look.
(Another thing regarding the documentation: Maybe we want
to update Documentation/config.txt as well, that contains all
configuration)

> @@ -503,6 +505,14 @@ static int git_push_config(const char *k, const
> char *v, void *cb)
>  int val = git_config_bool(k, v) ?
>  RECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF;
>  recurse_submodules = val;
> +} else if (!strcmp(k, "push.pushoption")) {
> +if (v == NULL)
> +return config_error_nonbool(k);
> +else
> +if (v[0] == '\0')
> +string_list_clear(_options, 0);

Junio,
do we have variables that behave similarly to this?

(I just wondered if the `v == NULL` could be lumped in
to here, resetting the string list)

>
> +test_expect_success 'default push option' '
...
> +'
> +
> +test_expect_success 'two default push options' '
...
> +'
> +
> +test_expect_success 'default and manual push options' '
...
> +'

Thanks for adding thorough tests!
Do we also need tests for the reset of the list?

Thanks,
Stefan

Re: [PATCH v1 1/1] completion: add remaining flags to checkout

[Johannes Sixt]

Am 12.10.2017 um 14:20 schrieb Thomas Braun:

In the commits 1d0fa898 (checkout: add --ignore-other-wortrees,
2015-01-03), 1fc458d9 (builtin/checkout: add --recurse-submodules switch,
2017-03-14), 870ebdb9 (checkout: add --progress option, 2015-11-01),
08d595dc (checkout: add --ignore-skip-worktree-bits in sparse checkout
mode, 2013-04-13), 1d0fa898 (checkout: add --ignore-other-wortrees,
2015-01-03), 32669671 (checkout: introduce --detach synonym for "git
checkout foo^{commit}", 2011-02-08) and db941099 (checkout -f: allow
ignoring unmerged paths when checking out of the index, 2008-08-30)
checkout gained new flags but the completion was not updated, although
these flags are useful completions. Add them.

Signed-off-by: Thomas Braun 
---
  contrib/completion/git-completion.bash | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index d934417475..393d4ae230 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1250,7 +1250,9 @@ _git_checkout ()
--*)
__gitcomp "
--quiet --ours --theirs --track --no-track --merge
-   --conflict= --orphan --patch
+   --conflict= --orphan --patch --detach --progress 
--no-progress
+   --force --ignore-skip-worktree-bits 
--ignore-other-worktrees


Destructive and dangerous options are typically not offered by command 
completion. You should omit all three in the line above, IMO.


Furthermore, --progress and --no-progress are not useful in daily work 
on the command line, I think. By offering them, --p would not 
complete to --patch anymore, you would need --pa. You should omit 
them, too.



+   --recurse-submodules --no-recurse-submodules
"
;;
*)



-- Hannes

Re: Enhancement request: git-push: Allow (configurable) default push-option

[Marius Paliga]

Subject: [PATCH] Added support for new configuration parameter push.pushOption

builtin/push.c: add push.pushOption config

Currently push options need to be given explicitly, via
the command line as "git push --push-option".

The UX of Git would be enhanced if push options could be
configured instead of given each time on the command line.

Add the config option push.pushOption, which is a multi
string option, containing push options that are sent by default.

When push options are set in the system wide config
(/etc/gitconfig), they can be unset later in the more specific
repository config by setting the string to the empty string.

Add tests and documentation as well.

Signed-off-by: Marius Paliga 
---
 Documentation/git-push.txt |  3 +++
 builtin/push.c | 11 ++-
 t/t5545-push-options.sh| 48 ++
 3 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 3e76e99f3..da9b17624 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -161,6 +161,9 @@ already exists on the remote side.
 Transmit the given string to the server, which passes them to
 the pre-receive as well as the post-receive hook. The given string
 must not contain a NUL or LF character.
+Default push options can also be specified with configuration
+variable `push.pushOption`. String(s) specified here will always
+be passed to the server without need to specify it using `--push-option`

 --receive-pack=::
 --exec=::
diff --git a/builtin/push.c b/builtin/push.c
index 2ac810422..f761fe4ba 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -32,6 +32,8 @@ static const char **refspec;
 static int refspec_nr;
 static int refspec_alloc;

+static struct string_list push_options = STRING_LIST_INIT_DUP;
+
 static void add_refspec(const char *ref)
 {
 refspec_nr++;
@@ -503,6 +505,14 @@ static int git_push_config(const char *k, const
char *v, void *cb)
 int val = git_config_bool(k, v) ?
 RECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF;
 recurse_submodules = val;
+} else if (!strcmp(k, "push.pushoption")) {
+if (v == NULL)
+return config_error_nonbool(k);
+else
+if (v[0] == '\0')
+string_list_clear(_options, 0);
+else
+string_list_append(_options, v);
 }

 return git_default_config(k, v, NULL);
@@ -515,7 +525,6 @@ int cmd_push(int argc, const char **argv, const
char *prefix)
 int push_cert = -1;
 int rc;
 const char *repo = NULL;/* default repository */
-struct string_list push_options = STRING_LIST_INIT_DUP;
 const struct string_list_item *item;

 struct option options[] = {
diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
index 90a4b0d2f..2cf9f7968 100755
--- a/t/t5545-push-options.sh
+++ b/t/t5545-push-options.sh
@@ -140,6 +140,54 @@ test_expect_success 'push options and submodules' '
 test_cmp expect parent_upstream/.git/hooks/post-receive.push_options
 '

+test_expect_success 'default push option' '
+mk_repo_pair &&
+git -C upstream config receive.advertisePushOptions true &&
+(
+cd workbench &&
+test_commit one &&
+git push --mirror up &&
+test_commit two &&
+git -c push.pushOption=default push up master
+) &&
+test_refs master master &&
+echo "default" >expect &&
+test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
+test_expect_success 'two default push options' '
+mk_repo_pair &&
+git -C upstream config receive.advertisePushOptions true &&
+(
+cd workbench &&
+test_commit one &&
+git push --mirror up &&
+test_commit two &&
+git -c push.pushOption=default1 -c push.pushOption=default2
push up master
+) &&
+test_refs master master &&
+printf "default1\ndefault2\n" >expect &&
+test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
+test_expect_success 'default and manual push options' '
+mk_repo_pair &&
+git -C upstream config receive.advertisePushOptions true &&
+(
+cd workbench &&
+test_commit one &&
+git push --mirror up &&
+test_commit two &&
+git -c push.pushOption=default push --push-option=manual up master
+) &&
+test_refs master master &&
+printf "default\nmanual\n" >expect &&
+test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd

-- 
2.14.1


2017-10-12 16:59 GMT+02:00 Marius Paliga :
> Thank you for your coments and explanation.
>
> Just one thing:

Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup

[Brandon Williams]

On 10/11, Josh Triplett wrote:
> On Tue, Oct 10, 2017 at 11:39:21AM -0700, Stefan Beller wrote:
> > On Tue, Oct 10, 2017 at 6:03 AM, Heiko Voigt  wrote:
> > > but in the long run my goal
> > > for submodules is and always was: Make them behave as close to files as
> > > possible. And why should a 'git add submodule' not magically do
> > > everything it can to make submodules just work? I can look into a patch
> > > for that if people agree here...
> > 
> > I'd love to see this implemented. I cc'd Josh (the author of git-series), 
> > who
> > may disagree with this, or has some good input how to go forward without
> > breaking git-series.
> 
> git-series doesn't use the git-submodule command at all, nor does it
> construct series trees using git-add or any other git command-line tool;
> it constructs gitlinks directly. Most of the time, it doesn't even make
> sense to `git checkout` a series branch. Modifying commands like git-add
> and similar to automatically manage .gitmodules won't cause any issue at
> all, as long as git itself doesn't start rejecting or complaining about
> repositories that have gitlinks without a .gitmodules file.

That's good to know!  And from what I remember, with the commands we've
begun teaching to understand submodules we have been requiring a
.gitmodules entry for a submodule in order to do the recursion, and a
gitlink without a .gitmodules entry would simply be ignored or skipped.

-- 
Brandon Williams

Re: [PATCH 07/18] sha1_file: support lazily fetching missing objects

[Christian Couder]

On Thu, Oct 12, 2017 at 4:42 PM, Christian Couder
 wrote:
>
> Instead of adding labels and gotos, I would suggest adding a new
> function like the following does on top of your changes:

Sorry for the usual Gmail related problems. You can find the patch and
a further refactoring that adds an object_info_from_pack_entry()
function there:

https://github.com/chriscool/git/commits/pu-partial-clone-lazy-fetch-improved

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Jeff King]

On Thu, Oct 12, 2017 at 09:06:49AM -0400, Jeff King wrote:

> > -- >8 --
> > From: Jonathan Nieder 
> > Subject: color: document that "git -c color.*=always" is a bit special
> > Date: Wed, 11 Oct 2017 21:47:24 -0700
> 
> This looks reasonable to me to ship in v2.15. I assume we're going to
> leave any "git --default-color=..." options to post-release, since we're
> already in -rc1.

Ah, I hadn't yet read your cover letter, since I wasn't on the cc for
that.

So yes, the overall plan seems OK to me. I do have a lingering
reservation that the fact that:

  git -c color.ui=always add -p

will break may come back to bite us. In particular, any such:

  git --default-color=always add -p

will run into the same problem if it is respected by plumbing. But in
theory we are free to have it not be so. Arguably we could do the same
for "-c color.ui", which I guess leaves us an "out" to later fix up that
case (my, the kludges are certainly piling up on this one).

-Peff

Re: [PATCH 2/2] color: discourage use of ui.color=always

[Jeff King]

On Thu, Oct 12, 2017 at 11:10:07AM +0900, Junio C Hamano wrote:

> Warn when we read such a configuration from a file, and nudge the
> users to spell them 'auto' instead.

Hmm. On the one hand, it is nice to make people aware that their config
isn't doing what they might think.

On the other hand, if "always" is no longer a problem for anybody, do we
need to force users to take the step to eradicate it? I dunno. Were we
planning to eventually remove it?

> @@ -320,6 +322,11 @@ int git_config_colorbool(const char *var, const char 
> *value)
>* Otherwise, we're looking at on-disk config;
>* downgrade to auto.
>*/
> + if (!warn_once) {
> + warn_once++;
> + warning("setting '%s' to '%s' is no longer 
> valid; "
> + "set it to 'auto' instead", var, value);
> + }

This warn_once is sadly not enough to give non-annoying output to
scripts that call many git commands. E.g.:

  $ git config color.ui always
  $ git add -p
  warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' 
instead
  warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' 
instead
  warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' 
instead
  warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' 
instead
  warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' 
instead
  warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' 
instead
  warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' 
instead
  warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' 
instead
  warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' 
instead
  warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' 
instead
  diff --git a/file b/file
  [...]

-Peff

Re: Enhancement request: git-push: Allow (configurable) default push-option

[Marius Paliga]

Thank you for your coments and explanation.

Just one thing:

>  - After parse_options() returns to cmd_push(), see if push_options
>is empty.  If it is, you did not get any command line option, so
>override it with what you collected in the "from-config" string
>list.  Otherwise, do not even look at "from-config" string list.

The idea is that there are default push options (read from config) that are
always sent to the server and you can add (not overwrite) additional by
specifying "--push-option".
So I would rather concatenate both lists - from command line and from-config.

> By the way, I really hate "push.optiondefault" as the variable
> name.  The "default" part is obvious and there is no need to say it,
> as the configuration variables are there to give the default to what
> we would normally give from the command line.  Rather, you should
> say for which option (there are many options "git push" takes) this
> variable gives the default.  Perhaps "push.pushOption" is a much
> better name; I am sure people can come up with even better ones,
> though ;-)

In the light of the above the "default" may be correct, but I don't
have a problem
with any name.

Marius


2017-10-11 15:38 GMT+02:00 Junio C Hamano :
> Marius Paliga  writes:
>
>> @@ -505,6 +509,12 @@ static int git_push_config(const char *k, const
>> char *v, void *cb)
>>  recurse_submodules = val;
>>  }
>>
>> +default_push_options = git_config_get_value_multi("push.optiondefault");
>> +if (default_push_options)
>> +for_each_string_list_item(item, default_push_options)
>> +if (!string_list_has_string(_options, item->string))
>> +string_list_append(_options, item->string);
>> +
>>  return git_default_config(k, v, NULL);
>>  }
>
> Sorry for not catching this earlier, but git_config_get_value* call
> inside git_push_config() is just wrong.
>
> There are two styles of configuration parsing.  The original (and
> still perfectly valid) way is to call git_config() with a callback
> function like git_push_config().  Under this style, the config files
> are read from lower-priority to higher-priority ones, and the
> callback function is called once for each entry found, with 
> pair and the callback specific opaque data.  One way to add the
> parsing of a new variable like push.optiondefault is to add
>
> if (!strcmp(k, "push.optiondefault") {
> ... handle one "[push] optiondefault" entry here ...
> return 0;
> }
>
> to the function.
>
> An alternate way is to use git_config_get_* functions _outside_
> callback of git_config().  This is a newer invention.  Your call to
> git_config_get_value_multi() will scan all configuration files and
> returns _all_  entries for the given variable at once.
>
> When there is already a callback style parser, in general, it is
> cleaner to simply piggy-back on it, instead of reading variables
> independently using git_config_get_* functions.  When there isn't a
> callback style parser, using either style is OK.  It also is OK to
> switch to git_config_get_* altogether, rewriting the callback style
> parser, but I do not think it is warranted in this case, which adds
> just one variable.
>
> In any case, with the above code, you'll end up calling the
> git_config_get_* function and grabbing all the values for
> push.optiondefault for each and every configuration variable
> definition (count "git config -l | wc -l" to estimate how many times
> it will be called).  Which is probably not what you wanted to do.
>
> Also, watch out for how a configuration variable defined like below
> is reported to either of the above two styles:
>
> [push]  optiondefault
>
>  - To a git_config() callback function like git_push_config(), such
>an entry is called with k=="push.optiondefault", v==NULL.
>
>  - git_config_get_value_multi() would return a string-list element
>with the string set to NULL to signal that one value is NULL
>(i.e. it is different from "[push] optiondefault = ").
>
> I suspect that with your code, we'd hit
>
> if (strchr(item->string, '\n'))
>
> and end up dereferencing NULL right there.
>
>> @@ -515,7 +525,6 @@ int cmd_push(int argc, const char **argv, const
>> char *prefix)
>>  int push_cert = -1;
>>  int rc;
>>  const char *repo = NULL;/* default repository */
>> -struct string_list push_options = STRING_LIST_INIT_DUP;
>>  const struct string_list_item *item;
>>
>>  struct option options[] = {
>
> Also, I suspect that this code does not allow the command line
> option to override the default set in the configuration file.
> OPT_STRING_LIST() appends to the _options string list without
> first clearing it, and you are pre-populating the list while reading
> the configuration, so the values taken from the command line will
> only add to them.
>
> The right way to do this would probably be:
>

Re: [PATCH 07/18] sha1_file: support lazily fetching missing objects

[Christian Couder]

On Fri, Sep 29, 2017 at 10:11 PM, Jonathan Tan  wrote:

> diff --git a/sha1_file.c b/sha1_file.c
> index b4a67bb83..77aa0ffdf 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -29,6 +29,7 @@
>  #include "mergesort.h"
>  #include "quote.h"
>  #include "packfile.h"
> +#include "fetch-object.h"
>
>  const unsigned char null_sha1[GIT_MAX_RAWSZ];
>  const struct object_id null_oid;
> @@ -1149,6 +1150,8 @@ static int sha1_loose_object_info(const unsigned char 
> *sha1,
> return (status < 0) ? status : 0;
>  }
>
> +int fetch_if_missing = 1;
> +
>  int sha1_object_info_extended(const unsigned char *sha1, struct object_info 
> *oi, unsigned flags)
>  {
> static struct object_info blank_oi = OBJECT_INFO_INIT;
> @@ -1157,6 +1160,7 @@ int sha1_object_info_extended(const unsigned char 
> *sha1, struct object_info *oi,
> const unsigned char *real = (flags & OBJECT_INFO_LOOKUP_REPLACE) ?
> lookup_replace_object(sha1) :
> sha1;
> +   int already_retried = 0;
>
> if (!oi)
> oi = _oi;
> @@ -1181,28 +1185,36 @@ int sha1_object_info_extended(const unsigned char 
> *sha1, struct object_info *oi,
> }
> }
>
> -   if (!find_pack_entry(real, )) {
> -   /* Most likely it's a loose object. */
> -   if (!sha1_loose_object_info(real, oi, flags))
> -   return 0;
> +retry:
> +   if (find_pack_entry(real, ))
> +   goto found_packed;
>
> -   /* Not a loose object; someone else may have just packed it. 
> */
> -   if (flags & OBJECT_INFO_QUICK) {
> -   return -1;
> -   } else {
> -   reprepare_packed_git();
> -   if (!find_pack_entry(real, ))
> -   return -1;
> -   }
> +   /* Most likely it's a loose object. */
> +   if (!sha1_loose_object_info(real, oi, flags))
> +   return 0;
> +
> +   /* Not a loose object; someone else may have just packed it. */
> +   reprepare_packed_git();
> +   if (find_pack_entry(real, ))
> +   goto found_packed;
> +
> +   /* Check if it is a missing object */
> +   if (fetch_if_missing && repository_format_partial_clone &&
> +   !already_retried) {
> +   fetch_object(repository_format_partial_clone, real);
> +   already_retried = 1;
> +   goto retry;
> }
>
> +   return -1;
> +
> +found_packed:
> if (oi == _oi)
> /*
>  * We know that the caller doesn't actually need the
>  * information below, so return early.
>  */
> return 0;
> -
> rtype = packed_object_info(e.p, e.offset, oi);
> if (rtype < 0) {
> mark_bad_packed_object(e.p, real);

Instead of adding labels and gotos, I would suggest adding a new
function like the following does on top of your changes:

diff --git a/sha1_file.c b/sha1_file.c
index cc1aa0bd7f..02a6ed1e9b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1171,18 +1171,40 @@ static int sha1_loose_object_info(const
unsigned char *sha1,
return (status < 0) ? status : 0;
 }

+int try_find_packed_entry_or_loose_object(const unsigned char *real,
struct object_info *oi,
+ unsigned flags, struct
pack_entry *e, int retry)
+{
+   if (find_pack_entry(real, e))
+   return 1;
+
+   /* Most likely it's a loose object. */
+   if (!sha1_loose_object_info(real, oi, flags))
+   return 0;
+
+   /* Not a loose object; someone else may have just packed it. */
+   reprepare_packed_git();
+   if (find_pack_entry(real, e))
+   return 1;
+
+   /* Check if it is a missing object */
+   if (fetch_if_missing && repository_format_partial_clone && retry) {
+   fetch_object(repository_format_partial_clone, real);
+   return try_find_packed_entry_or_loose_object(real, oi,
flags, e, 0);
+   }
+
+   return -1;
+}
+
 int fetch_if_missing = 1;

 int sha1_object_info_extended(const unsigned char *sha1, struct
object_info *oi, unsigned flags)
 {
static struct object_info blank_oi = OBJECT_INFO_INIT;
struct pack_entry e;
-   int rtype;
+   int rtype, res;
const unsigned char *real = (flags & OBJECT_INFO_LOOKUP_REPLACE) ?
lookup_replace_object(sha1) :
sha1;
-   int already_retried = 0;
-
if (!oi)
oi = _oi;

@@ -1206,30 +1228,10 @@ int sha1_object_info_extended(const unsigned
char *sha1, struct object_info *oi,
}
}

-retry:
-   if (find_pack_entry(real, ))
-   goto found_packed;
-
-   /* Most likely it's a loose object. */
- 

Re: [RFC] column: show auto columns when pager is active

[Jeff King]

On Wed, Oct 11, 2017 at 06:54:04AM +0200, Kevin Daudt wrote:

> > > Yeah, I didn't think of that with respect to the pager. This is a
> > > regression in v2.14.2, I think.
> > 
> > Yes.
> 
> Though 2.14.2 enabled the pager by default, even before that when
> someone would've enabled the pager theirselves (by setting pager.tag for
> example), it would also shown just a single column.
> 
> I could reproduce it as far as 2.8.3 (I could not test further due to
> libssl incompattibility).

Right, I think it has always been broken. It's just that it became a lot
more widespread with the increased use of the pager.

I specifically wanted to make sure this wasn't a regression in the v2.15
release candidates (since that would mean we'd want to deal with it
before shipping v2.15). It still _would_ be nice to deal with it soon,
but since it's already been in the wild for a bit, it can wait to hit
"maint" in the post-v2.15 cycle.

-Peff

Re: [PATCH v5 0/4] Improve abbreviation disambiguation

[Jeff King]

On Thu, Oct 12, 2017 at 09:21:15PM +0900, Junio C Hamano wrote:

> Derrick Stolee  writes:
> 
> > On 10/12/2017 8:02 AM, Derrick Stolee wrote:
> >> Changes since previous version:
> >>
> >> * Make 'pos' unsigned in get_hex_char_from_oid()
> >>
> >> * Check response from open_pack_index()
> >>
> >> * Small typos in commit messages
> >>
> >> Thanks,
> >>   Stolee
> >>
> > I forgot to mention that I rebased on master this morning to be sure
> > this doesn't conflict with the binary-search patch that was queued
> > this week.
> 
> Thanks for being extra careful.  
> 
> I've re-applied minor style fix s/(void\*)/(void \*)/ to 2/4 and 4/4
> but other than that, the difference between this round and what has
> been queued looked all reasonable.
> 
> Will replace.

This looks good to me, too.

At first I was going to point out the nit that unique_in_pack() is
quietly fixed in 4/4 for the empty-pack case. But I don't think it's
actually buggy. The examination of nth_packed_object_oid() would never
be triggered if "num" is 0. So it probably is fine simply to fix it
quietly along with adding the new function.

-Peff

Re: git repack leaks disk space on ENOSPC

[Jeff King]

On Thu, Oct 12, 2017 at 11:34:39AM +0200, Andreas Krey wrote:

> > Does using create_tempfile there seem like a good path forward to you?
> > Would you be interested in working on it (either writing a patch with
> > such a fix or a test in t/ to make sure it keeps working)?
> 
> I will look into creating a patch (thanks for the pointers),
> but I don't see how to make a testcase for this - pre-filling the
> disk doesn't sound like a good idea. Most people probably won't run in
> this situation, and then won't have tmp_packs with a dozen GBytes each
> lying around.

It may be easier to trigger a case which rejects the pack for other
reasons. For an incoming index-pack, turning on transfer.fsckObjects is
an easy one. For a repack, perhaps corrupting a loose object
to-be-packed would work.

E.g.:

  git init
  echo content >file
  git add file
  git commit -m foo

  # corrupt the blob in a subtle way
  obj=.git/objects/$(git rev-parse HEAD:file | sed 's,..,&/,')
  chmod +w $obj
  echo cruft >>$obj

After that, I get:

  $ git repack -ad
  Counting objects: 3, done.
  error: garbage at end of loose object 
'd95f3ad14dee633a758d2e331151e950dd13e4ed'
  fatal: loose object d95f3ad14dee633a758d2e331151e950dd13e4ed (stored in 
.git/objects/d9/5f3ad14dee633a758d2e331151e950dd13e4ed) is corrupt

  $ find -type f .git/objects/pack
  .git/objects/pack/tmp_pack_0GaXwk

-Peff

Re: git repack leaks disk space on ENOSPC

[Jeff King]

On Wed, Oct 11, 2017 at 08:17:03PM -0700, Jonathan Nieder wrote:

> I can imagine this behavior of retaining tmp_pack being useful for
> debugging in some circumstances, but I agree with you that it is
> certainly not a good default.
> 
> Chasing this down, I find:
> 
>   pack-write.c::create_tmp_packfile chooses the filename
>   builtin/pack-objects.c::write_pack_file writes to it and the .bitmap, 
> calling
>   pack-write.c::finish_tmp_packfile to rename it into place
> 
> Nothing tries to install an atexit handler to do anything special to it
> on exit.
> 
> The natural thing, I'd expect, would be for pack-write to use the
> tempfile API (see tempfile.h) to create and finish the file.  That way,
> we'd get such atexit handlers for free.  If we want a way to keep temp
> files for debugging on abnormal exit, we could set that up separately as
> a generic feature of the tempfile API (e.g. an envvar
> GIT_KEEP_TEMPFILES_ON_FAILURE), making that an orthogonal topic.

Yes, I think this is the right direction. I've had a patch in GitHub's
fork for years that does so (since otherwise failures can fill up your
disk and need manual intervention).

The main reason that I hadn't submitted it upstream was because of the
"you can never free a struct tempfile" requirement. So my patch just
leaks the tempfile structs. That's OK for packs, of which we tend to
create only a few in a given process, but doesn't scale for loose
objects.

Now that 89563ec379 (Merge branch 'jk/incore-lockfile-removal',
2017-09-19) has landed, I think it makes sense to pursue that direction.

My patch roughly looks like:

  diff --git a/builtin/index-pack.c b/builtin/index-pack.c
  index 4ff567db47..7f261e56c4 100644
  --- a/builtin/index-pack.c
  +++ b/builtin/index-pack.c

  @@ -308,9 +348,11 @@ static const char *open_pack_file(const char *pack_name)
  input_fd = 0;
  if (!pack_name) {
  struct strbuf tmp_file = STRBUF_INIT;
  +   struct tempfile *t = xcalloc(1, sizeof(*t));
  output_fd = odb_mkstemp(_file,
  "pack/tmp_pack_XX");
  pack_name = strbuf_detach(_file, NULL);
  +   register_tempfile(t, pack_name);
  } else {
  output_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR, 
0600);
  if (output_fd < 0)

but note that's not quite what we'd want. It never closes the tempfile,
so:

  1. Under the new regime, we'd still leak the struct!

  2. Git will still try to unlink the tempfile on exit, even if we
 successfully moved it into place.

So I think all the code around open_pack_file() needs to learn to pass
around the tempfile struct, and eventually use rename_tempfile() to
cement it in place. I also suspect that odb_mkstemp should just take a
"struct tempfile".

-Peff

Re: Is git am supposed to decode MIME?

[Florian Weimer]

On 10/04/2017 11:25 AM, Jeff King wrote:

On Wed, Oct 04, 2017 at 10:44:31AM +0200, Florian Weimer wrote:


The git am documentation talks about “mailboxes”.  I suppose these contain
messages in Internet Mail syntax.  Is git am supposed to decode MIME?

I'm asking because I have a message whose body is encoded as
quoted-printable, but git am does not parse the patch contained in it.

If git am is supposed to deal with this, I'll dig deeper and try to figure
out where things go wrong.


Yes, it should. I just double-checked with the toy patch patch below,
and it correctly extracted the quoted-printable from the commit message
and patch, as well as in the headers.


It took me a while, but I know think the message is simply corrupted. 
It's encoded with quoted-printable, and that looks correct, but it ends 
with:


@@ -5137,11 +5114,13 @@ __libc_mallopt (int param_number, int value)
   if (__malloc_initialized < 0)
 ptmalloc_init ();
   __libc_lock_lock (av->mutex);
-  /* Ensure initialization/consolidation */
-  malloc_consolidate (av);
=20
   LIBC_PROBE (memory_mallopt, 2, param_number, value);
=20
+  /* We must consolidate main arena before changing max_fast
+ (see definition of set_max_fast).  */
+  malloc_consolidate (av);
+
   switch (param_number)
 {
 case M_MXFAST:=

The “=” masks the final newline, and that doesn't decode into a valid 
diff hunk.  The file being patched continues after that, so it's not 
even the “\ No newline at end of file” case.


So in short, there is no Git bug here, and I just failed to interpret 
the “git am” diagnostics correctly:


Applying: Improve malloc initialization sequence
error: corrupt patch at line 342
Patch failed at 0001 Improve malloc initialization sequence
The copy of the patch that failed is found in: .git/rebase-apply/patch

Line 342 refers to the file in .git/rebase-apply/patch, not the original 
input, and it took me a while to figure that out.


Thanks,
Florian

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Jeff King]

On Thu, Oct 12, 2017 at 03:58:18PM +0900, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > We need to be able to answer "why does '-c color.ui=always' work
> > only from the command line?", but I doubt we want to actively
> > encourage the use of it, though, so I dunno.
> 
> For today's pushout, I've queued this as [PATCH 3/2]
> 
> Thanks..
> 
> -- >8 --
> From: Jonathan Nieder 
> Subject: color: document that "git -c color.*=always" is a bit special
> Date: Wed, 11 Oct 2017 21:47:24 -0700

This looks reasonable to me to ship in v2.15. I assume we're going to
leave any "git --default-color=..." options to post-release, since we're
already in -rc1.

-Peff

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Jeff King]

On Thu, Oct 12, 2017 at 11:10:06AM +0900, Junio C Hamano wrote:

> From: Jeff King 
> 
> An earlier patch downgraded "always" that comes via the ui.color
> configuration variable to "auto", in order to work around an
> unfortunate regression to "git add -i".
> 
> That "fix" however regressed other third-party tools that depend on
> "git -c color.ui=always cmd" as the way to defeat any end-user
> configuration and force coloured output from git subcommand, even
> when the output does not go to a terminal.
> 
> It is a bit ugly to treat "-c color.ui=always" from the command line
> differently from a definition that comes from on-disk configuration
> files, but it is a moral equivalent of "--color=always" option given
> to the subcommand from the command line, i.e. a signal that tells us
> that the script writer knows what s/he is doing.  So let's take that
> route to unbreak this case while defeating a (now declared to be)
> misguided color.ui that is set to always in the configuration file.
> 
> NEEDS-SIGN-OFF-BY: Jeff King 

Signed-off-by: Jeff King 

Thanks for picking this up. I meant to get to it yesterday but ran out
of time. Your description looks good to me.

>  color.c | 17 +++--
>  1 file changed, 15 insertions(+), 2 deletions(-)

We should probably protect the command-line behavior with a test. Can
you squash this in?

diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index 25a9c65dc5..582cab5c8a 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -261,6 +261,17 @@ test_expect_success 'rev-list %C(auto,...) respects 
--color' '
test_cmp expect actual
 '
 
+test_expect_success "color.ui=always in config file same as auto" '
+   test_config color.ui always &&
+   git log --format=$COLOR -1 >actual &&
+   has_no_color actual
+'
+
+test_expect_success "color.ui=always on command-line is always" '
+   git -c color.ui=always log --format=$COLOR -1 >actual &&
+   has_color actual
+'
+
 iconv -f utf-8 -t $test_encoding > commit-msg <

Re: [PATCH v5 0/4] Improve abbreviation disambiguation

[Junio C Hamano]

Derrick Stolee  writes:

> On 10/12/2017 8:02 AM, Derrick Stolee wrote:
>> Changes since previous version:
>>
>> * Make 'pos' unsigned in get_hex_char_from_oid()
>>
>> * Check response from open_pack_index()
>>
>> * Small typos in commit messages
>>
>> Thanks,
>>   Stolee
>>
> I forgot to mention that I rebased on master this morning to be sure
> this doesn't conflict with the binary-search patch that was queued
> this week.

Thanks for being extra careful.  

I've re-applied minor style fix s/(void\*)/(void \*)/ to 2/4 and 4/4
but other than that, the difference between this round and what has
been queued looked all reasonable.

Will replace.

[PATCH v1 1/1] completion: add remaining flags to checkout

[Thomas Braun]

In the commits 1d0fa898 (checkout: add --ignore-other-wortrees,
2015-01-03), 1fc458d9 (builtin/checkout: add --recurse-submodules switch,
2017-03-14), 870ebdb9 (checkout: add --progress option, 2015-11-01),
08d595dc (checkout: add --ignore-skip-worktree-bits in sparse checkout
mode, 2013-04-13), 1d0fa898 (checkout: add --ignore-other-wortrees,
2015-01-03), 32669671 (checkout: introduce --detach synonym for "git
checkout foo^{commit}", 2011-02-08) and db941099 (checkout -f: allow
ignoring unmerged paths when checking out of the index, 2008-08-30)
checkout gained new flags but the completion was not updated, although
these flags are useful completions. Add them.

Signed-off-by: Thomas Braun 
---
 contrib/completion/git-completion.bash | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index d934417475..393d4ae230 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1250,7 +1250,9 @@ _git_checkout ()
--*)
__gitcomp "
--quiet --ours --theirs --track --no-track --merge
-   --conflict= --orphan --patch
+   --conflict= --orphan --patch --detach --progress 
--no-progress
+   --force --ignore-skip-worktree-bits 
--ignore-other-worktrees
+   --recurse-submodules --no-recurse-submodules
"
;;
*)
-- 
2.15.0.rc0.245.g6d586db062

Re: [PATCH v5 0/4] Improve abbreviation disambiguation

[Derrick Stolee]

On 10/12/2017 8:02 AM, Derrick Stolee wrote:

Changes since previous version:

* Make 'pos' unsigned in get_hex_char_from_oid()

* Check response from open_pack_index()

* Small typos in commit messages

Thanks,
  Stolee

I forgot to mention that I rebased on master this morning to be sure 
this doesn't conflict with the binary-search patch that was queued this 
week.


Thanks,
 Stolee

[PATCH v5 4/4] sha1_name: minimize OID comparisons during disambiguation

[Derrick Stolee]

Minimize OID comparisons during disambiguation of packfile OIDs.

Teach git to use binary search with the full OID to find the object's
position (or insertion position, if not present) in the pack-index.
The object before and immediately after (or the one at the insertion
position) give the maximum common prefix.  No subsequent linear search
is required.

Take care of which two to inspect, in case the object id exists in the
packfile.

If the input to find_unique_abbrev_r() is a partial prefix, then the
OID used for the binary search is padded with zeroes so the object will
not exist in the repo (with high probability) and the same logic
applies.

This commit completes a series of three changes to OID abbreviation
code, and the overall change can be seen using standard commands for
large repos. Below we report performance statistics for perf test 4211.6
from p4211-line-log.sh using three copies of the Linux repo:

| Packs | Loose  | HEAD~3   | HEAD | Rel%  |
|---||--|--|---|
|  1|  0 |  41.27 s |  38.93 s | -4.8% |
| 24|  0 |  98.04 s |  91.35 s | -5.7% |
| 23| 323952 | 117.78 s | 112.18 s | -4.8% |

Signed-off-by: Derrick Stolee 
---
 sha1_name.c | 76 +
 1 file changed, 71 insertions(+), 5 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index fdd2711a6..7fd5f5f71 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -153,7 +153,9 @@ static void unique_in_pack(struct packed_git *p,
uint32_t num, last, i, first = 0;
const struct object_id *current = NULL;
 
-   open_pack_index(p);
+   if (open_pack_index(p) || !p->num_objects)
+   return;
+
num = p->num_objects;
last = num;
while (first < last) {
@@ -478,6 +480,7 @@ struct min_abbrev_data {
unsigned int init_len;
unsigned int cur_len;
char *hex;
+   const unsigned char *hash;
 };
 
 static inline char get_hex_char_from_oid(const struct object_id *oid,
@@ -505,6 +508,67 @@ static int extend_abbrev_len(const struct object_id *oid, 
void *cb_data)
return 0;
 }
 
+static void find_abbrev_len_for_pack(struct packed_git *p,
+struct min_abbrev_data *mad)
+{
+   int match = 0;
+   uint32_t num, last, first = 0;
+   struct object_id oid;
+
+   if (open_pack_index(p) || !p->num_objects)
+   return;
+
+   num = p->num_objects;
+   last = num;
+   while (first < last) {
+   uint32_t mid = first + (last - first) / 2;
+   const unsigned char *current;
+   int cmp;
+
+   current = nth_packed_object_sha1(p, mid);
+   cmp = hashcmp(mad->hash, current);
+   if (!cmp) {
+   match = 1;
+   first = mid;
+   break;
+   }
+   if (cmp > 0) {
+   first = mid + 1;
+   continue;
+   }
+   last = mid;
+   }
+
+   /*
+* first is now the position in the packfile where we would insert
+* mad->hash if it does not exist (or the position of mad->hash if
+* it does exist). Hence, we consider a maximum of three objects
+* nearby for the abbreviation length.
+*/
+   mad->init_len = 0;
+   if (!match) {
+   nth_packed_object_oid(, p, first);
+   extend_abbrev_len(, mad);
+   } else if (first < num - 1) {
+   nth_packed_object_oid(, p, first + 1);
+   extend_abbrev_len(, mad);
+   }
+   if (first > 0) {
+   nth_packed_object_oid(, p, first - 1);
+   extend_abbrev_len(, mad);
+   }
+   mad->init_len = mad->cur_len;
+}
+
+static void find_abbrev_len_packed(struct min_abbrev_data *mad)
+{
+   struct packed_git *p;
+
+   prepare_packed_git();
+   for (p = packed_git; p; p = p->next)
+   find_abbrev_len_for_pack(p, mad);
+}
+
 int find_unique_abbrev_r(char *hex, const unsigned char *sha1, int len)
 {
struct disambiguate_state ds;
@@ -536,19 +600,21 @@ int find_unique_abbrev_r(char *hex, const unsigned char 
*sha1, int len)
if (len == GIT_SHA1_HEXSZ || !len)
return GIT_SHA1_HEXSZ;
 
-   if (init_object_disambiguation(hex, len, ) < 0)
-   return -1;
-
mad.init_len = len;
mad.cur_len = len;
mad.hex = hex;
+   mad.hash = sha1;
+
+   find_abbrev_len_packed();
+
+   if (init_object_disambiguation(hex, mad.cur_len, ) < 0)
+   return -1;
 
ds.fn = extend_abbrev_len;
ds.always_call_fn = 1;
ds.cb_data = (void*)
 
find_short_object_filename();
-   find_short_packed_object();
(void)finish_object_disambiguation(, _ret);
 
hex[mad.cur_len] = 0;
-- 
2.14.1.538.g56ec8fc98.dirty

[PATCH v5 1/4] p4211-line-log.sh: add log --online --raw --parents perf test

[Derrick Stolee]

Add a new perf test for testing the performance of log while computing
OID abbreviations. Using --oneline --raw and --parents options maximizes
the number of OIDs to abbreviate while still spending some time computing
diffs.

Signed-off-by: Derrick Stolee 
---
 t/perf/p4211-line-log.sh | 4 
 1 file changed, 4 insertions(+)

diff --git a/t/perf/p4211-line-log.sh b/t/perf/p4211-line-log.sh
index b7ff68d4f..e0ed05907 100755
--- a/t/perf/p4211-line-log.sh
+++ b/t/perf/p4211-line-log.sh
@@ -31,4 +31,8 @@ test_perf 'git log -L (renames on)' '
git log -M -L 1:"$file" >/dev/null
 '
 
+test_perf 'git log --oneline --raw --parents' '
+   git log --oneline --raw --parents >/dev/null
+'
+
 test_done
-- 
2.14.1.538.g56ec8fc98.dirty

[PATCH v5 2/4] sha1_name: unroll len loop in find_unique_abbrev_r

[Derrick Stolee]

Unroll the while loop inside find_unique_abbrev_r to avoid iterating
through all loose objects and packfiles multiple times when the short
name is longer than the predicted length.

Instead, inspect each object that collides with the estimated
abbreviation to find the longest common prefix.

The focus of this change is to refactor the existing method in a way
that clearly does not change the current behavior. In some cases, the
new method is slower than the previous method. Later changes will
correct all performance loss.

Signed-off-by: Derrick Stolee 
---
 sha1_name.c | 57 ++---
 1 file changed, 42 insertions(+), 15 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index c7c5ab376..19603713f 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -474,10 +474,32 @@ static unsigned msb(unsigned long val)
return r;
 }
 
-int find_unique_abbrev_r(char *hex, const unsigned char *sha1, int len)
+struct min_abbrev_data {
+   unsigned int init_len;
+   unsigned int cur_len;
+   char *hex;
+};
+
+static int extend_abbrev_len(const struct object_id *oid, void *cb_data)
 {
-   int status, exists;
+   struct min_abbrev_data *mad = cb_data;
+
+   char *hex = oid_to_hex(oid);
+   unsigned int i = mad->init_len;
+   while (mad->hex[i] && mad->hex[i] == hex[i])
+   i++;
+
+   if (i < GIT_MAX_RAWSZ && i >= mad->cur_len)
+   mad->cur_len = i + 1;
+
+   return 0;
+}
 
+int find_unique_abbrev_r(char *hex, const unsigned char *sha1, int len)
+{
+   struct disambiguate_state ds;
+   struct min_abbrev_data mad;
+   struct object_id oid_ret;
if (len < 0) {
unsigned long count = approximate_object_count();
/*
@@ -503,19 +525,24 @@ int find_unique_abbrev_r(char *hex, const unsigned char 
*sha1, int len)
sha1_to_hex_r(hex, sha1);
if (len == GIT_SHA1_HEXSZ || !len)
return GIT_SHA1_HEXSZ;
-   exists = has_sha1_file(sha1);
-   while (len < GIT_SHA1_HEXSZ) {
-   struct object_id oid_ret;
-   status = get_short_oid(hex, len, _ret, GET_OID_QUIETLY);
-   if (exists
-   ? !status
-   : status == SHORT_NAME_NOT_FOUND) {
-   hex[len] = 0;
-   return len;
-   }
-   len++;
-   }
-   return len;
+
+   if (init_object_disambiguation(hex, len, ) < 0)
+   return -1;
+
+   mad.init_len = len;
+   mad.cur_len = len;
+   mad.hex = hex;
+
+   ds.fn = extend_abbrev_len;
+   ds.always_call_fn = 1;
+   ds.cb_data = (void*)
+
+   find_short_object_filename();
+   find_short_packed_object();
+   (void)finish_object_disambiguation(, _ret);
+
+   hex[mad.cur_len] = 0;
+   return mad.cur_len;
 }
 
 const char *find_unique_abbrev(const unsigned char *sha1, int len)
-- 
2.14.1.538.g56ec8fc98.dirty

[PATCH v5 3/4] sha1_name: parse less while finding common prefix

[Derrick Stolee]

Create get_hex_char_from_oid() to parse oids one hex character at a
time. This prevents unnecessary copying of hex characters in
extend_abbrev_len() when finding the length of a common prefix.

Signed-off-by: Derrick Stolee 
---
 sha1_name.c | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 19603713f..fdd2711a6 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -480,13 +480,23 @@ struct min_abbrev_data {
char *hex;
 };
 
+static inline char get_hex_char_from_oid(const struct object_id *oid,
+unsigned int pos)
+{
+   static const char hex[] = "0123456789abcdef";
+
+   if ((pos & 1) == 0)
+   return hex[oid->hash[pos >> 1] >> 4];
+   else
+   return hex[oid->hash[pos >> 1] & 0xf];
+}
+
 static int extend_abbrev_len(const struct object_id *oid, void *cb_data)
 {
struct min_abbrev_data *mad = cb_data;
 
-   char *hex = oid_to_hex(oid);
unsigned int i = mad->init_len;
-   while (mad->hex[i] && mad->hex[i] == hex[i])
+   while (mad->hex[i] && mad->hex[i] == get_hex_char_from_oid(oid, i))
i++;
 
if (i < GIT_MAX_RAWSZ && i >= mad->cur_len)
-- 
2.14.1.538.g56ec8fc98.dirty

[PATCH v5 0/4] Improve abbreviation disambiguation

[Derrick Stolee]

Changes since previous version:

* Make 'pos' unsigned in get_hex_char_from_oid()

* Check response from open_pack_index()

* Small typos in commit messages 

Thanks,
 Stolee

---

When displaying object ids, we frequently want to see an abbreviation
for easier typing. That abbreviation must be unambiguous among all
object ids.

The current implementation of find_unique_abbrev() performs a loop
checking if each abbreviation length is unambiguous until finding one
that works. This causes multiple round-trips to the disk when starting
with the default abbreviation length (usually 7) but needing up to 12
characters for an unambiguous short-sha. For very large repos, this
effect is pronounced and causes issues with several commands, from
obvious consumers `status` and `log` to less obvious commands such as
`fetch` and `push`.

This patch improves performance by iterating over objects matching the
short abbreviation only once, inspecting each object id, and reporting
the minimum length of an unambiguous abbreviation.

Add a new perf test for testing the performance of log while computing
OID abbreviations. Using --oneline --raw and --parents options maximizes
the number of OIDs to abbreviate while still spending some time
computing diffs. Below we report performance statistics for perf test
4211.6 from p4211-line-log.sh using three copies of the Linux repo:

| Packs | Loose  | Base Time | New Time | Rel%  |
|---||---|--|---|
|  1|  0 |   41.27 s |  38.93 s | -4.8% |
| 24|  0 |   98.04 s |  91.35 s | -5.7% |
| 23| 323952 |  117.78 s | 112.18 s | -4.8% |

Derrick Stolee (4):
  p4211-line-log.sh: add log --online --raw --parents perf test
  sha1_name: unroll len loop in find_unique_abbrev_r
  sha1_name: parse less while finding common prefix
  sha1_name: minimize OID comparisons during disambiguation
 sha1_name.c  | 135 +--
 t/perf/p4211-line-log.sh |   4 ++
 2 files changed, 123 insertions(+), 16 deletions(-)

-- 
2.14.1.538.g56ec8fc98.dirty

RE: Kontaktieren Sie meine E-Mail (wang.jian...@yandex.com)

[Hardy, Joy]

Ich beabsichtige, Ihnen einen Teil meines Vermögens als freiwillige finanzielle 
Spende an Sie zu geben. Reagieren Sie auf partake.contact meine E-Mail 
(wang.jian...@yandex.com)
Wang Jianlin
Wanda Gruppe

Re: Branch switching with submodules where the submodule replaces a folder aborts unexpectedly

[Thomas Braun]

> Stefan Beller  hat am 9. Oktober 2017 um 23:59 
> geschrieben:
> 
> 
> On Mon, Oct 9, 2017 at 2:29 PM, Thomas Braun
>  wrote:
> > Hi,
> >
> > I'm currently in the progress of pulling some subprojects in a git 
> > repository of mine into their
> > own repositories and adding these subprojects back as submodules.
> >
> > While doing this I enountered a potential bug as checkout complains on 
> > branch switching that a 
> > file already exists.
> 
> (And I presume you know about --recurse-submodules as a flag for git-checkout)

No I did not know about it. I tend to not know options which don't complete in 
my shell (patch follows for that).

> This is consistent with our tests, unfortunately.
> 
> git/t$ ./t2013-checkout-submodule.sh
> ...
> not ok 15 - git checkout --recurse-submodules: replace submodule with
> a directory # TODO known breakage
> ...
> 
> > If I'm misusing git here I'm glad for any advice.
> 
> You are not.

Glad to know that.

> Apart from this bug report, would you think that such filtering of
> trees into submodules (and back again) might be an interesting feature
> of Git or are these cases rare and special?

For me not particularly. In my case it is a one time thing going from an 
embedded project folder to a submodule.

Re: [RFC] deprecate git stash save?

[Junio C Hamano]

Junio C Hamano  writes:

> Thomas Gummerer  writes:
>
>> git stash push is the newer interface for creating a stash.  While we
>> are still keeping git stash save around for the time being, it's better
>> to point new users of git stash to the more modern (and more feature
>> rich) interface, instead of teaching them the older version that we
>> might want to phase out in the future.
>
> With devil's advocate hat on, because the primary point of "stash"
> being "clear the desk quickly", I do not necessarily agree that
> "more feature rich" is a plus and something we should nudge newbies
> towards.

I do not particulary view "feature richness" is the primary benefit
of 'push' that makes it shine.  'save' has a quirk in the command
line option syntax, but 'push' corrects it, and that is why we want
to move users towards the latter.

IOW, I do not object to the agenda; it is just I found the
justification used to push the agenda forward was flawed.

Thanks.

Re: [PATCH] Documentation/merge-options.txt: Add -S/--gpg-sign

[Junio C Hamano]

Kevin Daudt  writes:

>> > --S[]::
>> > ---gpg-sign[=]::
>> 
>> Shouldn't the options self be removed here too, not just the
>> explanation?
>
> You can ignore this, it was just my mail client that colored the diff
> wrong, confusing me.
>
>> > -  GPG-sign the resulting merge commit. The `keyid` argument is
>> > -  optional and defaults to the committer identity; if specified,
>> ...

;-) Very understandable confusion.  Thanks for reading patches
  carefully.
  

Re: [PATCH] Documentation/merge-options.txt: Add -S/--gpg-sign

[Junio C Hamano]

Makes sense. Will queue.

Re: [PATCH] pull: pass --signoff/--no-signoff to "git merge"

[Junio C Hamano]

Junio C Hamano  writes:

>   get_signoff () {
>   git cat-file commit "$1" | sed -n -e '/^Signed-off-by: /p'
>   }
>
> Some may say "cat-file can fail, and having it on the LHS of a pipe
> hides its failure", advocating for something like:
>
>   get_signoff () {
>   git cat-file commit "$1" >sign-off-temp &&
>   sed -n -e '/^Signed-off-by: /p' sign-off-temp
>   }

Actually we should use git itself for things like this, e.g.

git -C dst show -s --pretty='format:%(trailers)' HEAD >actual &&
test_cmp expect actual

Re: git repack leaks disk space on ENOSPC

[brian m. carlson]

On Thu, Oct 12, 2017 at 11:34:39AM +0200, Andreas Krey wrote:
> On Wed, 11 Oct 2017 20:17:03 +, Jonathan Nieder wrote:
> > Does using create_tempfile there seem like a good path forward to you?
> > Would you be interested in working on it (either writing a patch with
> > such a fix or a test in t/ to make sure it keeps working)?
> 
> I will look into creating a patch (thanks for the pointers),
> but I don't see how to make a testcase for this - pre-filling the
> disk doesn't sound like a good idea. Most people probably won't run in
> this situation, and then won't have tmp_packs with a dozen GBytes each
> lying around.

A patch would be very welcome.  We have this problem not infrequently at
work with development and test VMs, which tend to have a relatively
small amount of disk.

If you decide that you don't want to create a patch, I'll probably pick
it up eventually.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH] Documentation/merge-options.txt: Add -S/--gpg-sign

[Kevin Daudt]

On Thu, Oct 12, 2017 at 12:44:59PM +0200, Kevin Daudt wrote:
> On Thu, Oct 12, 2017 at 02:02:17AM -0700, W. Trevor King wrote:
> > Pull has supported these since ea230d8 (pull: add the --gpg-sign
> > option, 2014-02-10).  Insert in long-option alphabetical order
> > following 7c85d274 (Documentation/merge-options.txt: order options in
> > alphabetical groups, 2009-10-22).
> > 
> > Signed-off-by: W. Trevor King 
> > ---
> > This patch is based on maint.  It will have trivial conflicts with the
> > --signoff docs which landed in 14d01b4f07 (merge: add a --signoff
> > flag, 2017-07-04, v2.15.0-rc0~138^2).
> > 
> >  Documentation/git-merge.txt | 6 --
> >  Documentation/merge-options.txt | 6 ++
> >  2 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> > index f90faf7aaa..1d97a17904 100644
> > --- a/Documentation/git-merge.txt
> > +++ b/Documentation/git-merge.txt
> > @@ -64,12 +64,6 @@ OPTIONS
> >  ---
> >  include::merge-options.txt[]
> >  
> > --S[]::
> > ---gpg-sign[=]::
> 
> Shouldn't the options self be removed here too, not just the
> explanation?
> 

You can ignore this, it was just my mail client that colored the diff
wrong, confusing me.

> > -   GPG-sign the resulting merge commit. The `keyid` argument is
> > -   optional and defaults to the committer identity; if specified,
> > -   it must be stuck to the option without a space.
> > -
> >  -m ::
> > Set the commit message to be used for the merge commit (in
> > case one is created).
> > diff --git a/Documentation/merge-options.txt 
> > b/Documentation/merge-options.txt
> > index 5b4a62e936..6d85a76872 100644
> > --- a/Documentation/merge-options.txt
> > +++ b/Documentation/merge-options.txt
> > @@ -42,6 +42,12 @@ set to `no` at the beginning of them.
> > current `HEAD` is already up-to-date or the merge can be
> > resolved as a fast-forward.
> >  
> > +-S[]::
> > +--gpg-sign[=]::
> > +   GPG-sign the resulting merge commit. The `keyid` argument is
> > +   optional and defaults to the committer identity; if specified,
> > +   it must be stuck to the option without a space.
> > +
> >  --log[=]::
> >  --no-log::
> > In addition to branch names, populate the log message with
> > -- 
> > 2.13.6
> > 

Re: [PATCH] Documentation/merge-options.txt: Add -S/--gpg-sign

[Kevin Daudt]

On Thu, Oct 12, 2017 at 02:02:17AM -0700, W. Trevor King wrote:
> Pull has supported these since ea230d8 (pull: add the --gpg-sign
> option, 2014-02-10).  Insert in long-option alphabetical order
> following 7c85d274 (Documentation/merge-options.txt: order options in
> alphabetical groups, 2009-10-22).
> 
> Signed-off-by: W. Trevor King 
> ---
> This patch is based on maint.  It will have trivial conflicts with the
> --signoff docs which landed in 14d01b4f07 (merge: add a --signoff
> flag, 2017-07-04, v2.15.0-rc0~138^2).
> 
>  Documentation/git-merge.txt | 6 --
>  Documentation/merge-options.txt | 6 ++
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index f90faf7aaa..1d97a17904 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -64,12 +64,6 @@ OPTIONS
>  ---
>  include::merge-options.txt[]
>  
> --S[]::
> ---gpg-sign[=]::

Shouldn't the options self be removed here too, not just the
explanation?

> - GPG-sign the resulting merge commit. The `keyid` argument is
> - optional and defaults to the committer identity; if specified,
> - it must be stuck to the option without a space.
> -
>  -m ::
>   Set the commit message to be used for the merge commit (in
>   case one is created).
> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
> index 5b4a62e936..6d85a76872 100644
> --- a/Documentation/merge-options.txt
> +++ b/Documentation/merge-options.txt
> @@ -42,6 +42,12 @@ set to `no` at the beginning of them.
>   current `HEAD` is already up-to-date or the merge can be
>   resolved as a fast-forward.
>  
> +-S[]::
> +--gpg-sign[=]::
> + GPG-sign the resulting merge commit. The `keyid` argument is
> + optional and defaults to the committer identity; if specified,
> + it must be stuck to the option without a space.
> +
>  --log[=]::
>  --no-log::
>   In addition to branch names, populate the log message with
> -- 
> 2.13.6
> 

Re: [PATCH v2 00/24] object_id part 10

[brian m. carlson]

On Thu, Oct 12, 2017 at 06:58:49PM +0900, Junio C Hamano wrote:
> "brian m. carlson"  writes:
> 
> > In the course of that, I'll rebase on top of master so that Junio can
> > avoid as much conflict resolution as possible.
> 
> I actually do not mind either way, but by rebasing on top of a more
> recent codebase you can lose a large part of 07/24 IIRC, which would
> be a big plus ;-)

Yes, I just noticed that.  The patch used to be bigger still, but as I
mentioned in the original cover letter, a lot of it went away due to
previous work in that area.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: Bug or feature: format-patch breaks long subject lines

[Junio C Hamano]

 writes:

> Is this the expected behaviour?

Yes, it is expected.  Your are seeing the header folding in play.
"mailinfo" (hence "am") will grok it just fine, I think.

Re: [PATCH v2] pull: pass --signoff/--no-signoff to "git merge"

[Junio C Hamano]

"W. Trevor King"  writes:

> e379fdf3 (merge: refuse to create too cool a merge by default,
> 2016-03-18) gave a reason for *not* passing options from pull to
> merge:
>
>   ...because such a "two project merge" would be done after fetching
>   the other project into some location in the working tree of an
>   existing project and making sure how well they fit together...

Read the above again and notice the phrase "two project merge".  The
reasoning applies only to the --allow-unrelated-histories option.
It gave a reason for not passing *THAT* option and nothing else, and
does not mean to say anything about passing or not passing any other
options.  

That is why I said the reference to that commit was irrelevant in
the context of this patch.

If you find somebody saying "we should not pass --signoff from pull
to merge" when we taught "--signoff" to "merge", then that may be
worth referring to, as this commit _will_ be changing that earlier
decision.  I however do not think that is a case.  Just saying
"merge can take --signoff, but without pull passing --signoff down,
it is inconvenient, so let's pass it through" is sufficient to
justify this change.

> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
> index ded8f98dbe..82680a30f8 100755
> --- a/t/t5521-pull-options.sh
> +++ b/t/t5521-pull-options.sh
> @@ -165,4 +165,44 @@ test_expect_success 'git pull 
> --allow-unrelated-histories' '
>   )
>  '
>  
> +test_expect_success 'git pull does not add a sign-off line' '
> + test_when_finished "rm -fr src dst" &&
> + git init src &&
> + test_commit -C src one &&
> + git clone src dst &&
> + test_commit -C src two &&
> + git -C dst pull --no-ff &&
> + ! test_has_trailer -C dst HEAD Signed-off-by
> +'
> +
> +test_expect_success 'git pull --no-signoff does not add sign-off line' '
> + test_when_finished "rm -fr src dst" &&
> + git init src &&
> + test_commit -C src one &&
> + git clone src dst &&
> + test_commit -C src two &&
> + git -C dst pull --no-signoff --no-ff &&
> + ! test_has_trailer -C dst HEAD Signed-off-by
> +'
> +
> +test_expect_success 'git pull --signoff add a sign-off line' '
> + test_when_finished "rm -fr src dst" &&
> + git init src &&
> + test_commit -C src one &&
> + git clone src dst &&
> + test_commit -C src two &&
> + git -C dst pull --signoff --no-ff &&
> + test_has_trailer -C dst HEAD Signed-off-by "$GIT_COMMITTER_NAME 
> <$GIT_COMMITTER_EMAIL>"
> +'
> +
> +test_expect_success 'git pull --no-signoff flag cancels --signoff flag' '
> + test_when_finished "rm -fr src dst actual" &&
> + git init src &&
> + test_commit -C src one &&
> + git clone src dst &&
> + test_commit -C src two &&
> + git -C dst pull --signoff --no-signoff --no-ff &&
> + ! test_has_trailer -C dst HEAD Signed-off-by
> +'
> +
>  test_done
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 1701fe2a06..08409b1c25 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -726,6 +726,49 @@ test_must_be_empty () {
>   fi
>  }
>  
> +# Call test_has_trailer with the arguments:
> +# [-C ]   []
> +# where  is an object name as described in gitrevisions(7),
> +#  is a trailer token (e.g. 'Signed-off-by'), and
> +#  is an optional value (e.g. 'A U Thor ').
> +# test_has_trailer returns success if the specified trailer is found
> +# in the object content.  If  is unset, any value will match.
> +#
> +# Both  and  are basic regular expressions.
> +#
> +# If the first argument is "-C", the second argument is used as a path for
> +# the git invocations.
> +test_has_trailer () {
> + INDIR=
> + case "$1" in
> + -C)
> + INDIR="$2"
> + shift 2 || error " not specified"
> + ;;
> + esac
> + INDIR="${INDIR:+${INDIR}/}"
> + OBJECT="$1"
> + shift || error " not specified"
> + TOKEN="$1"
> + shift || error " not specified"
> + SEP=':'  # FIXME: read from trailer.separators?
> + CONTENT="$(git ${INDIR:+ -C "${INDIR}"} cat-file -p "${OBJECT}")" || 
> error "object ${OBJECT} not found${INDIR:+ in ${INDIR}}"
> + PATTERN="^${TOKEN}${SEP}"
> + if test 0 -lt "$#"
> + then
> + VALUE="$1"
> + PATTERN="${PATTERN} *${VALUE}$"
> + fi
> + if (echo "${CONTENT}" | grep -q "${PATTERN}")
> + then
> + printf "%s found in:\n%s\n" "${PATTERN}" "${CONTENT}"
> + return 0
> + else
> + printf "%s not found in:\n%s\n" "${PATTERN}" "${CONTENT}"
> + return 1
> + fi
> +}

The reason why I suggested a simple "sed -n -e ...p" you used in
your original was because it could be used to extract not just one
Signed-off-by: lines to store in >actual, to be compared with an
expect that has multiple S-o-b lines and the output is in correct
order, etc.  An elaborate filter that can onlyl give 

Re: [PATCH v2 00/24] object_id part 10

[Junio C Hamano]

"brian m. carlson"  writes:

> In the course of that, I'll rebase on top of master so that Junio can
> avoid as much conflict resolution as possible.

I actually do not mind either way, but by rebasing on top of a more
recent codebase you can lose a large part of 07/24 IIRC, which would
be a big plus ;-)

Re: [PATCH 1/1] git-stash.txt: correct "git stash" behavior with no arguments

[Junio C Hamano]

小川恭史  writes:

> As you point,
>
> git stash
>
> without any argument is equivalent to both of
>
> git stash save
> git stash push
>
> . The original sentence is correct.

OK.  

Note that I was merely reacting to "Correct it." in your
justification for the change, which made it sound like 'save' was
incorrect.

As a part of a move to deprecate 'save' and nudge users towards
'push', the change does make sense.  I just wanted to make sure the
change and its motivation are presented with honesty ;-).

Re: git repack leaks disk space on ENOSPC

[Andreas Krey]

On Wed, 11 Oct 2017 20:17:03 +, Jonathan Nieder wrote:
> Hi Andreas,
> 
> Andreas Krey wrote:
> 
> > I observed (again) an annoying behavior of 'git repack':
> 
> Do you have context for this 'again'?  E.g. was this discussed
> previously on-list?

I think I posted about it, but no discussion. I poked a bit
at the code, with not much luck back then.

...
> Does using create_tempfile there seem like a good path forward to you?
> Would you be interested in working on it (either writing a patch with
> such a fix or a test in t/ to make sure it keeps working)?

I will look into creating a patch (thanks for the pointers),
but I don't see how to make a testcase for this - pre-filling the
disk doesn't sound like a good idea. Most people probably won't run in
this situation, and then won't have tmp_packs with a dozen GBytes each
lying around.

Andreas

-- 
"Totally trivial. Famous last words."
From: Linus Torvalds 
Date: Fri, 22 Jan 2010 07:29:21 -0800

Re: [PATCH v2] pull: pass --signoff/--no-signoff to "git merge"

[W. Trevor King]

On Thu, Oct 12, 2017 at 01:46:39AM -0700, W. Trevor King wrote:
> The order of options in merge-options.txt isn't clear to me, but
> I've put --signoff between --log and --stat as somewhat alphabetized
> and having an "add to the commit message" function like --log.

The order of options in merge-options.txt was intended to be by
"alphabetical groups", at least back in 7c85d274
(Documentation/merge-options.txt: order options in alphabetical
groups, 2009-10-22).  I'm not quite clear on what that means.  After
7c85d274 landed there were already long-option irregularities:

  $ git grep -h ^-- 7c85d27 -- Documentation/merge-options.txt
  --commit::
  --no-commit::
  --ff::
  --no-ff::
  --log::
  --no-log::
  --stat::
  --no-stat::
  --squash::
  --no-squash::
  --strategy=::
  --summary::
  --no-summary::
  --quiet::
  --verbose::

If the order was purely alphabetical, --stat/--no-stat should have
after --squash/--no-squash, and --quiet should have been much earlier.
And putting --signoff after --log is still alphabetical in v2.15.0-rc1
(ignoring a few outliers).  So I don't think it's a reason to change
where I'd put the option, but in v3 of this patch I'll update the
commit message to cite 7c85d274 when motivating the location.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy


signature.asc
Description: OpenPGP digital signature

[PATCH v2] Documentation/git-config.txt: reword missleading sentence

[second . payre]

From: PAYRE NATHAN p1508475 

Change the word "bla" to "section.variable", "bla" is a placeholder
for a variable name and it wasn't clear for everyone.
This change clarify it.

Change the appearance of 'git config section.variable {tilde}/' to
`git config section.variable {tilde}/` to harmonize it with
the rest of the file, this is a command line then the "`" are
necessary.

Replace "git-config" by "git config" because the command
is not "git-config".

See discussion at:
https://public-inbox.org/git/20171002061303.horde.sl92grzcqtrv9oqkbfpe...@crashcourse.ca/

Signed-off-by: MOY Matthieu 
Signed-off-by: Daniel Bensoussan 
Signed-off-by: Timothee Albertin 
Signed-off-by: Nathan Payre 
Noticed-by: rpj...@crashcourse.ca
---
 Documentation/git-config.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 83f86b923..2ab9e4c56 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -174,11 +174,11 @@ See also <>.
either --bool or --int, as described above.
 
 --path::
-   'git-config' will expand leading '{tilde}' to the value of
+   'git config' will expand leading '{tilde}' to the value of
'$HOME', and '{tilde}user' to the home directory for the
specified user.  This option has no effect when setting the
-   value (but you can use 'git config bla {tilde}/' from the
-   command line to let your shell do the expansion).
+   value (but you can use `git config section.variable {tilde}/`
+   from the command line to let your shell do the expansion).
 
 -z::
 --null::
-- 
2.14.2

Re: v2.15.0-rc1 test failure

[Adam Dinwoodie]

On Thu, Oct 12, 2017 at 01:27:57AM +0100, Ramsay Jones wrote:
> On 11/10/17 23:34, Adam Dinwoodie wrote:
> [snip]
> > Hi Ramsay,
> > 
> > I assume, given you're emailing me, that this is a Cygwin failure?
> 
> Yes, sorry, I should have made that clear.
> 
> > t0021.15 has PERL as a requirement, and I see semi-regular failures from
> > Git tests that are Perl-based in one way or another (git-svn tests are
> > the most common problems).  I've not spotted t0021 failing in that way,
> > but it sounds like the same class of problem.
> 
> Yep, many moons ago, I used to run the svn tests (on Linux and cygwin)
> which would fail intermittently on cygwin. I didn't notice any problem
> with perl though.
> 
> > I dig into these failures when I see them, mostly by running the script
> > a few hundred times until I get the failure again, and they've always
> > been Perl itself segfaulting.  That points to the problem being in
> > Cygwin's Perl package rather than Git, and it's very unlikely to be
> > anything that's got worse in v2.15.0.
> 
> Since I stopped running the svn tests, the number of intermittent test 
> failures on cygwin have dropped significantly, but haven't gone away
> completely.
> 
> I just finished the second test-suite run and, of course, t0021 ran
> without problem this time. Hmm, I don't think I have time to chase
> this down at the moment. I will keep your 'perl hypothesis' in mind
> for next time, however.

Evidence for my Perl hypothesis, which I offer at least as much so other
people can check my logic as anything else:

Here's a fairly typical set of verbose output from a test failing in
this way [0].  The critical bit is the line "error: git-svn died of
signal 11".  Since git-svn is a Perl script, and Perl is the sort of
interpreted language that would throw its own errors if it encountered a
script bug, the fact that it's hitting a segfault means there's a
problem of some ilk with the Perl interpreter itself.

[0]: https://github.com/me-and/Cygwin-Git/issues/13#issuecomment-211372448

Adam

[PATCH] Documentation/merge-options.txt: Add -S/--gpg-sign

[W. Trevor King]

Pull has supported these since ea230d8 (pull: add the --gpg-sign
option, 2014-02-10).  Insert in long-option alphabetical order
following 7c85d274 (Documentation/merge-options.txt: order options in
alphabetical groups, 2009-10-22).

Signed-off-by: W. Trevor King 
---
This patch is based on maint.  It will have trivial conflicts with the
--signoff docs which landed in 14d01b4f07 (merge: add a --signoff
flag, 2017-07-04, v2.15.0-rc0~138^2).

 Documentation/git-merge.txt | 6 --
 Documentation/merge-options.txt | 6 ++
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index f90faf7aaa..1d97a17904 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -64,12 +64,6 @@ OPTIONS
 ---
 include::merge-options.txt[]
 
--S[]::
---gpg-sign[=]::
-   GPG-sign the resulting merge commit. The `keyid` argument is
-   optional and defaults to the committer identity; if specified,
-   it must be stuck to the option without a space.
-
 -m ::
Set the commit message to be used for the merge commit (in
case one is created).
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 5b4a62e936..6d85a76872 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -42,6 +42,12 @@ set to `no` at the beginning of them.
current `HEAD` is already up-to-date or the merge can be
resolved as a fast-forward.
 
+-S[]::
+--gpg-sign[=]::
+   GPG-sign the resulting merge commit. The `keyid` argument is
+   optional and defaults to the committer identity; if specified,
+   it must be stuck to the option without a space.
+
 --log[=]::
 --no-log::
In addition to branch names, populate the log message with
-- 
2.13.6

Bug or feature: format-patch breaks long subject lines

[stefan.naewe]

Hello list,

git format-patch breaks (or better: word-wraps) long subject lines.

This is on Windows 7 with

$ git --version
git version 2.14.2.windows.2

Reproduce with (some output omitted):

--
$ git init test-format-patch
$ cd test-format-patch
$ touch file
$ git add file
$ git commit 
-m"0123456789012345678901234567890123456789012345678901234567890123456789"
$ git format-patch -1 --stdout
From ec711cca330f1032d286114932a90488542f1da2 Mon Sep 17 00:00:00 2001
From: Stefan Naewe 
Date: Thu, 12 Oct 2017 10:36:52 +0200
Subject: [PATCH]
 0123456789012345678901234567890123456789012345678901234567890123456789

---
 file | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 file

diff --git a/file b/file
new file mode 100644
index 000..e69de29
--
2.14.2.windows.2
--

Is this the expected behaviour?

Thanks,
  Stefan
-- 

/dev/random says: I Have To Stop Now, My Fingers Are Getting Hoarse!
python -c "print 
'73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
 
GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9  9666 829B 49C5 9221 27AF

[PATCH v2] pull: pass --signoff/--no-signoff to "git merge"

[W. Trevor King]

e379fdf3 (merge: refuse to create too cool a merge by default,
2016-03-18) gave a reason for *not* passing options from pull to
merge:

  ...because such a "two project merge" would be done after fetching
  the other project into some location in the working tree of an
  existing project and making sure how well they fit together...

That's certainly an acceptable workflow, but I'd also like to support
merge options in pull for folks who want to optimistically pull and
then sort out "how well they fit together" after pull exits (possibly
with a merge failure).  And in cases where an optimistic pull is
likely to succeed, suggesting it is easier to explain to Git newbies
than a FETCH_HEAD merge or remote-addition/merge/remote-removal.

09c2cb87 (pull: pass --allow-unrelated-histories to "git merge",
2016-03-18) added a pull-to-merge pass for a different option but
didn't motivate its change, only citing the reason from e379fdf3 for
not adding the pull-to-merge pass for that option.  I'm personally in
favor of pull-to-merge passing for any unambiguous options, but if the
decision for pull-to-merge passes depends on the specific option, then
--allow-unrelated-histories is probably the weakest candidate because
unrelated-history merged are more likely to have "fit together" issues
than the other merge-only options.

The test_has_trailer helper gives folks a convenient way check these
sorts of things.  I haven't gone through and updated existing trailer
checks (e.g. the ones in t7614-merge-signoff.sh) to avoid the "only to
correct the inconconsistency" issue discussed in SubmittingPatches.
Other test may gradually migrate to the new helper if they find it
useful.  The helper may be useful enough to eventually become a
plumbing command (a read version of interpret-trailers with an API
similar to 'git config ...'?), but I'm not going that far in this
commit ;).

The order of options in merge-options.txt isn't clear to me, but I've
put --signoff between --log and --stat as somewhat alphabetized and
having an "add to the commit message" function like --log.

Helped-by: Junio C Hamano 
Signed-off-by: W. Trevor King 
---
Changes since v1 [1]:

* Dropped "Following" paragraph.  Junio took issue with the phrasing
  [2], and the implementation in v2 has diverged sufficiently from
  09c2cb87 and 14d01b4f that I don't think citing them as
  implementation references is useful anymore.

* Lead the commit message with reworked motivation paragraphs, since
  Junio read the v1 motivation paragraph as off-topic [2].

* Add a test_has_trailer helper, which I'd floated in [3] after
  Junio's get_signoff suggestion in [2].

* Drop subshells in favor of '-C ' in the tests, as
  suggested in [2].

* Add tests for the bare pull and lonely --no-signoff cases, as
  suggested in [2].  With these additions in place, I've dropped v1's
  "The tests aren't as extensive..." paragraph from the commit
  message.

* Use OPT_PASSTHRU in pull.c.  I'm not sure why
  --allow-unrelated-histories didn't go this route, but there are lots
  of other pull-to-merge options using OPT_PASSTHRU, so using it for
  --signoff isn't breaking consistency.

Not changed since v1:

* The merge-options.txt order paragraph.  Junio had suggested it be
  moved after the break [2], but I think having some commit-message
  discussion of merge-options.txt ordering is useful, even though I
  don't have strong opinions on what the ordering should be [3].

This patch (and v1) are based on v2.15.0-rc1, although I expect
they'll apply cleanly to anything in that vicinity.

Cheers,
Trevor

[1]: 
https://public-inbox.org/git/18953f46ffb5e3dbc4da8fbda7fe3ab4298d7cbd.1507752482.git.wk...@tremily.us/
[2]: https://public-inbox.org/git/xmqqefq92mgw@gitster.mtv.corp.google.com/
[3]: https://public-inbox.org/git/20171012053002.gz11...@valgrind.tremily.us/

 Documentation/git-merge.txt |  8 
 Documentation/merge-options.txt | 10 ++
 builtin/pull.c  |  6 ++
 t/t5521-pull-options.sh | 40 ++
 t/test-lib-functions.sh | 43 +
 5 files changed, 99 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 4df6431c34..0ada8c856b 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -64,14 +64,6 @@ OPTIONS
 ---
 include::merge-options.txt[]
 
---signoff::
-   Add Signed-off-by line by the committer at the end of the commit
-   log message.  The meaning of a signoff depends on the project,
-   but it typically certifies that committer has
-   the rights to submit this work under the same license and
-   agrees to a Developer Certificate of Origin
-   (see http://developercertificate.org/ for more information).
-
 -S[]::
 --gpg-sign[=]::
GPG-sign the resulting merge commit. The `keyid` argument is
diff --git 

Re: [PATCH v2 00/24] object_id part 10

[brian m. carlson]

On Wed, Oct 11, 2017 at 12:05:50PM +0200, Michael Haggerty wrote:
> On 10/09/2017 03:11 AM, brian m. carlson wrote:
> > This is the tenth in a series of patches to convert from unsigned char
> > [20] to struct object_id.  This series mostly involves changes to the
> > refs code.  After these changes, there are almost no references to
> > unsigned char in the main refs code.
> > 
> > The series has not been rebased on master since the last submission, but
> > I can do so if that's more convenient.
> > 
> > This series is available from the following URL:
> > https://github.com/bk2204/git.git object-id-part10
> 
> I read through the whole series medium-thoroughly and left a few
> comments, but overall it looks very good and clear. Thanks so much for
> working on this!

Thanks for pointing out the places where I forgot to update the
docstrings.  I'll plan another reroll with those changes and the other
issues mentioned about the accidental deletion.

In the course of that, I'll rebase on top of master so that Junio can
avoid as much conflict resolution as possible.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH v2 04/24] refs: convert update_ref and refs_update_ref to use struct object_id

[brian m. carlson]

On Wed, Oct 11, 2017 at 08:33:46AM +0200, Michael Haggerty wrote:
> On 10/09/2017 03:11 AM, brian m. carlson wrote:
> > diff --git a/refs.c b/refs.c
> > index 0a5b68d6fb..51942df7b3 100644
> > --- a/refs.c
> > +++ b/refs.c
> > [...]
> > @@ -1003,12 +995,12 @@ int refs_update_ref(struct ref_store *refs, const 
> > char *msg,
> > int ret = 0;
> >  
> > if (ref_type(refname) == REF_TYPE_PSEUDOREF) {
> > -   assert(refs == get_main_ref_store());
> 
> Was the deletion of the line above intentional?

No, that would not have been intentional.  (I would have mentioned it in
the commit message if it were.)  I probably accidentally deleted a line
in my editor.  Will fix.

> > -   ret = write_pseudoref(refname, new_sha1, old_sha1, );
> > +   ret = write_pseudoref(refname, new_oid, old_oid, );
> 
> This is not new to your code, but I just noticed a problem here.
> `refs_update_ref()` is documented, via its reference to
> `ref_transaction_update()`, to allow `new_sha1` (i.e., now `new_oid`) to
> be NULL. (NULL signifies that the value of the reference shouldn't be
> changed.)
> 
> But `write_pseudoref()` dereferences its `oid` argument unconditionally,
> so this call would fail if `new_oid` is NULL.
> 
> This has all been the case since `write_pseudoref()` was introduced in
> 74ec19d4be (pseudorefs: create and use pseudoref update and delete
> functions, 2015-07-31).
> 
> In my opinion, `write_pseudoref()` is broken. It should accept NULL as
> its `oid` argument.

I can stuff a patch in for that.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH] doc: emphasize stash "--keep-index" stashes staged content

[Robert P. J. Day]

On Wed, 11 Oct 2017, Jonathan Nieder wrote:

> Hi,
>
> Robert P. J. Day wrote:
>
> > It's not immediately obvious from the man page that the "--keep-index"
> > option still adds the staged content to the stash, so make that
> > abundantly clear.
> >
> > Signed-off-by: Robert P. J. Day 
> > ---
> >
> > diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> > index 00f95fee1..037144037 100644
> > --- a/Documentation/git-stash.txt
> > +++ b/Documentation/git-stash.txt
> > @@ -68,8 +68,8 @@ entries and working tree files are then rolled back to 
> > the state in
> >  HEAD only for these files, too, leaving files that do not match the
> >  pathspec intact.
> >  +
> > -If the `--keep-index` option is used, all changes already added to the
> > -index are left intact.
> > +If the `--keep-index` option is used, all changes already staged in the
> > +index are left intact in the index, while still being added to the stash.
>
> Aside from Junio's note about "in the index" vs "in the working tree":

  yes, that was a good point, i will ponder further.

> The "Testing partial commits" item in the EXAMPLES section explains
> what --keep-index is useful for.  I wonder if some allusion to that
> would make the explanation in the OPTIONS section easier to
> understand.
>
> Something that I end up still curious about when reading this
> description is what will happen when I "git stash pop".  Will it
> apply only the changes that were stashed away and removed from the
> working tree, or will it apply the changes that were kept in the
> index, too? If the latter, why?  Is there some way I can turn that
> behavior off?

  at risk of embarrassing myself, it seems that the simplest way to
explain stashing WRT to those --keep-index and --index options is to
first explain that, regardless of --keep-index with push, stash will
*always* stash all of your changes in the working tree, and will
further distinguish between staged and unstaged content. that's based
on the diagram in the man page:

  .W
 //
   -HI

so the initial explanation should be that the above *always* happens,
no matter what.

  the next sentence should then say, "if you add --keep-index, then
staged changes are preserved in the working tree and index", or
something like that. but the use of --keep-index does not (unless i'm
reading this incorrectly) in any way affect what is stashed, correct?

  in that same vein, the explanation should then go on to explain that
popping always restores the *entire* stash to the working tree -- all
of it -- and the use of "--index" simply means that the portion of
the stash representing what had been staged will be restaged.

  not to belabour the point, but i think it's important to emphasize
early that --keep-index and --index in no way affect what is stashed,
and what is popped, which i think is a common misunderstanding.

> E.g. in the "Testing partial commits" example, it seems like the
> natural behavior for "git stash pop" would be just restore the
> changes that were removed from the working tree.  That would also
> match an assumption of save/push and pop being symmetrical ('inverse
> operations').
>
> Is this related to "git stash pop --index"?  I notice that the
> EXAMPLES section doesn't give any examples of that option.

rday

-- 


Robert P. J. Day Ottawa, Ontario, CANADA
http://crashcourse.ca

Twitter:   http://twitter.com/rpjday
LinkedIn:   http://ca.linkedin.com/in/rpjday

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Junio C Hamano]

Junio C Hamano  writes:

> We need to be able to answer "why does '-c color.ui=always' work
> only from the command line?", but I doubt we want to actively
> encourage the use of it, though, so I dunno.

For today's pushout, I've queued this as [PATCH 3/2]

Thanks..

-- >8 --
From: Jonathan Nieder 
Subject: color: document that "git -c color.*=always" is a bit special
Date: Wed, 11 Oct 2017 21:47:24 -0700

When used from the command line as an option to "git" potty,
'always' does not get demoted to 'auto', to help third-party scripts
that (ab)used it to override the settings the end-user has.
Document it.

While at it, clarify description for per-command configuration
variables (color.branch, color.grep, color.interactive,
color.showBranch and color.status) so that they can more easily
share the new text to talk about this special-casing.

Signed-off-by: Jonathan Nieder 
Signed-off-by: Junio C Hamano 
---
 Documentation/config.txt | 68 
 1 file changed, 45 insertions(+), 23 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ba01b8d3df..f79e82b79a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1051,11 +1051,15 @@ clean.requireForce::
-i or -n.   Defaults to true.
 
 color.branch::
-   A boolean to enable/disable color in the output of
-   linkgit:git-branch[1]. May be set to `false` (or `never`) to
-   disable color entirely, `auto` (or `true` or `always`) in which
-   case colors are used only when the output is to a terminal.  If
-   unset, then the value of `color.ui` is used (`auto` by default).
+   When to use color in the output of linkgit:git-branch[1].
+   May be set to `never` (or `false`) to disable color entirely,
+   or `auto` (or `true`) in which case colors are used only when
+   the output is to a terminal.  If unset, then the value of
+   `color.ui` is used (`auto` by default).
++
+The value `always` is a historical synonym for `auto`, except when
+passed on the command line using `-c`, where it is a historical synonym
+for `--color=always`.
 
 color.branch.::
Use customized color for branch coloration. `` is one of
@@ -1068,10 +1072,13 @@ color.diff::
Whether to use ANSI escape sequences to add color to patches.
If this is set to `true` or `auto`, linkgit:git-diff[1],
linkgit:git-log[1], and linkgit:git-show[1] will use color
-   when output is to the terminal. The value `always` is a
-   historical synonym for `auto`.  If unset, then the value of
+   when output is to the terminal. If unset, then the value of
`color.ui` is used (`auto` by default).
 +
+The value `always` is a historical synonym for `auto`, except when
+passed on the command line using `-c`, where it is a historical
+synonym for `--color=always`.
++
 This does not affect linkgit:git-format-patch[1] or the
 'git-diff-{asterisk}' plumbing commands.  Can be overridden on the
 command line with the `--color[=]` option.
@@ -1091,10 +1098,14 @@ color.decorate.::
branches, remote-tracking branches, tags, stash and HEAD, respectively.
 
 color.grep::
-   When set to `always`, always highlight matches.  When `false` (or
+   When to highlight matches using color. When `false` (or
`never`), never.  When set to `true` or `auto`, use color only
when the output is written to the terminal.  If unset, then the
value of `color.ui` is used (`auto` by default).
++
+The value `always` is a historical synonym for `auto`, except when
+passed on the command line using `-c`, where it is a historical synonym
+for `--color=always`.
 
 color.grep.::
Use customized color for grep colorization.  `` specifies which
@@ -1126,9 +1137,11 @@ color.interactive::
When set to `true` or `auto`, use colors for interactive prompts
and displays (such as those used by "git-add --interactive" and
"git-clean --interactive") when the output is to the terminal.
-   When false (or `never`), never show colors. The value `always`
-   is a historical synonym for `auto`.  If unset, then the value of
-   `color.ui` is used (`auto` by default).
+   When false (or `never`), never show colors.
++
+The value `always` is a historical synonym for `auto`, except when
+passed on the command line using `-c`, where it means to use color
+regardless of whether output is to the terminal.
 
 color.interactive.::
Use customized color for 'git add --interactive' and 'git clean
@@ -1141,18 +1154,24 @@ color.pager::
use (default is true).
 
 color.showBranch::
-   A boolean to enable/disable color in the output of
-   linkgit:git-show-branch[1]. May be set to `always`,
-   `false` (or `never`) or `auto` (or `true`), in which case colors are 
used
-   only when the output is to a terminal. If 

Re: [PATCH] pull: pass --signoff/--no-signoff to "git merge"

[W. Trevor King]

On Thu, Oct 12, 2017 at 02:42:30PM +0900, Junio C Hamano wrote:
> "W. Trevor King"  writes:
> > On Thu, Oct 12, 2017 at 10:17:51AM +0900, Junio C Hamano wrote:
> >> "W. Trevor King"  writes:
> >> 
> >> > Following 09c2cb87 (pull: pass --allow-unrelated-histories to "git
> >> > merge", 2016-03-18) with the tests also drawing on 14d01b4f (merge:
> >> > add a --signoff flag, 2017-07-04).
> >> 
> >> I cannot find a verb in the above.
> >
> > I'd meant it as either a continuation of the subject line, or with an
> 
> Never do that.  The title should be able to stand on its own, and
> must not be an early part of incomplete sentence.

“Following” to an imperative “Follow” it is then, unless you want a
more drastic rewording.

> > Sounds good.  I'll add a patch to v2 to make the same change to
> > the existing t5521 --allow-unrelated-histories test.
> 
> Please don't, unless you are actively working on the features that
> they test.  We do not have infinite amount of bandwidth to deal with
> changes for the sake of perceived consistency and no other real
> gain.

By extention, I'm guessing that means that while the:

  test_has_trailer $OBJECT $TOKEN $VALUE

and:

  test_has_no_trailer $OBJECT $TOKEN

test-lib-functions.sh helpers I floated may be acceptable (or not, no
need to commit before you've seen a patch), you don't want me updating
existing tests to use them.  I'll just use them in my new tests, and
folks can gradually transition existing tests to them as they touch
those tests (if they remember the helpers exist ;).

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy


signature.asc
Description: OpenPGP digital signature

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Junio C Hamano]

Jonathan Nieder  writes:

> Junio C Hamano wrote:
>> Jonathan Nieder  writes:
>
>>> Should we document this special case treatment of color.* in -c
>>> somewhere?  E.g.
>>
>> Perhaps, although I'd save that until we actually add the new option
>> to "git" potty, which hasn't happened yet, if I were thinking about
>> adding some text like that.  Also I'd call that --default-color=always
>> or something like that, to avoid having to answer: what are the
>> differences between these two --color thing in the following?
>>
>> git --color=foo cmd --color=bar
>
> I agree that the color.status text in the example doc is unfortunate.
> But the surprising thing I found when writing that doc is that
> color.status ("git status", "git commit --dry-run") and
> color.interactive are the only items that needed it (aside from
> color.ui that needed it for those two).  All the other commands that
> use color already accept
>
>   git cmd --color=bar

Ahh, I take it that you mean by "it" (in "needed it") the "git
potty" option, not a "--color=" option individual "git cmd"
takes?  If so, then it makes sense to say "that's another way to
spell --color=always from the command line".

We need to be able to answer "why does '-c color.ui=always' work
only from the command line?", but I doubt we want to actively
encourage the use of it, though, so I dunno.