[PATCH] rebase docs: drop stray word in merge command description

[Kyle Meyer]

Delete a misplaced word introduced by caafecfcf1 (rebase
--rebase-merges: adjust man page for octopus support, 2018-03-09).

Signed-off-by: Kyle Meyer 
---
 Documentation/git-rebase.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index dff17b3178..2ee535fb23 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -979,7 +979,7 @@ when the merge operation did not even start), it is 
rescheduled immediately.
 
 At this time, the `merge` command will *always* use the `recursive`
 merge strategy for regular merges, and `octopus` for octopus merges,
-strategy, with no way to choose a different one. To work around
+with no way to choose a different one. To work around
 this, an `exec` command can be used to call `git merge` explicitly,
 using the fact that the labels are worktree-local refs (the ref
 `refs/rewritten/onto` would correspond to the label `onto`, for example).
-- 
2.19.2

Re: Git Evolve

[Kyle Meyer]

Ævar Arnfjörð Bjarmason  writes:

> On Tue, Oct 02 2018, Taylor Blau wrote:

[...]

>> Specifically, I've wanted the 'hg absorb' command. My understanding of
>> the commands functionality is that it builds a sort of flamegraph-esque
>> view of the blame, and then cascades downwards parts of a change. I am
>> sure that I'm not doing the command justice, so I'll defer to [1] where
>> it is explained in more detail.
>>
>> The benefit of this command is that it gives you a way to--without
>> ambiguity--absorb changes into earlier commits, and in fact, the
>> earliest commit that they make sense to belong to.
>>
>> This would simplify my workflow greatly when re-rolling patches, as I
>> often want to rewrite a part of an earlier commit. This is certainly
>> possible by a number of different `git rebase` invocations (e.g., (1)
>> create fixup commits, and then re-order them, or (2) mark points in your
>> history as 'edit', and rewrite them in a detached state, and I'm sure
>> many more).
>>
>> I'm curious if you or anyone else has thought about how this might work
>> in Git.
>
> I've wanted a "git absorb" for a while, but have done no actual work on
> it, I just found out about it.

It may be worth looking into git-autofixup [0] (author cc'd).  I learned
about it when Magit used it in its magit-commit-absorb command, but I
haven't used it yet myself.

[0] https://github.com/torbiak/git-autofixup

-- 
Kyle

Re: [PATCH] Improvement to only call Git Credential Helper once

[Kyle Hubert]

Thank you for the review, commenting inline.

On Fri, Sep 28, 2018 at 3:29 PM Junio C Hamano  wrote:
> Kyle Hubert  writes:
>
> > Subject: Re: [PATCH] Improvement to only call Git Credential Helper once
>
> Nobody will send in a patch to worsen things, so phrases like
> "Improvement to" that convey no useful information has no place on
> the title.
>
> There probably are multiple ways that credential helpers are not
> called just once and many of them probably are legit (e.g. "get" is
> not the only request one helper can receive).  It is unclear why
> "only call helper once" is an improvement unless the reader reads
> more, which means the title could be a lot more improved.
>
> Side note: this matters because in "git shortlog --no-merges"
> the title is the only thing you can tell your readers what
> your contribution was about.

Yes, thank you. I just joined the mailing list. I'm getting a sense
for the style here. If this patch moves forward, I will rewrite the
title.

> > When calling the Git Credential Helper that is set in the git config,
> > the get command can return a credential. Git immediately turns around
> > and calls the store command, even though that credential was just
> > retrieved by the Helper.
>
> Good summary of the current behaviour.  A paragraph break here would
> make the result easier to read.

Check.

> > This creates two side effects. First of all,
> > if the Helper requires a passphrase, the user has to type it in
> > twice.
>
> Hmph, because...?  If I am reading the flow correctly, an
> application would
>
>  - call credential_fill(), which returns when both username and
>password are obtained by a "get" request to one of the helpers.
>
>  - use the credential to authenticate with a service and find that
>it was accepted.
>
>  - call credential_approve(), which does "store" to all the helpers.
>
> Where does the "twice" come from?
>
> Ah, that is not between the application and the service, but between
> the helper and the user you are required to "unlock" the helper?
>
> OK, that wish makes sense.
>
> It does not make much sense to ask helper A for credential and then
> tell it to write it back the same thing.
>
> HOWEVER.  Let me play a devil's advocate.
>
> The "store" does not have to necessarily mean "write it back", no?
>
> Imagine a helper that is connected to an OTP password device that
> gives a different passcode every time button A is pressed, and there
> are two other buttons B to tell the device that the password was
> accepted and C to tell the device that the password was rejected
> (i.e. we are out of sync).  "get" would press button A and read the
> output, "store" would press button B and "erase" would press button
> C, I would imagine.  With the current credential.c framework, you
> can construct such a helper.  The proposed patch that stops calling
> "store" unconditionally makes it impossible to build.

This example helper would require knowing external state between
button pushes. What about aborting the operation? Regardless of the
example, I can understand your concern. If a helper depended on
receiving confirmation and rejection, this change would break that
behavior.

The patch is intended to address the common problem of a multi-user
system running on a server in headless mode, in other words, without
libsecret available via DBus. As such, this patch could eliminate
having to double type passwords for every git operation accessing the
stored credential.

> > Secondly, if the user has a number of helpers, this retrieves
> > the credential from one service and writes it to all services.
>
> It is unclear why you think it is a bad thing.  You need to
> elaborate.
>
> On the other hand, I can think of a case to illustrate why it is a
> bad idea to unconditionally stop calling "store" to other helpers.
> If one helper is a read-only "encrypted on disk" one, you may want
> to require passphrase to "decrypt" to implement the "get" request to
> the helper.  You would then overlay a "stay only in-core for a short
> time" helper and give higher priority to it.
>
> By doing so, on the first "get" request will ask the in-core one,
> which says "I dunno", then the encrypted-on-disk one interacts with
> the end-user and gives the credential.  The current code "store"s to
> the in-core one as well as the encrypted-on-disk one, and second and
> subseqhent "get" request can be served from that in-core helper.
>
> Side note: and

[PATCH] Improvement to only call Git Credential Helper once

[Kyle Hubert]

When calling the Git Credential Helper that is set in the git config,
the get command can return a credential. Git immediately turns around
and calls the store command, even though that credential was just
retrieved by the Helper. This creates two side effects. First of all,
if the Helper requires a passphrase, the user has to type it in
twice. Secondly, if the user has a number of helpers, this retrieves
the credential from one service and writes it to all services.

This commit introduces a new field in the credential struct that
detects when the credential was retrieved using the Helper, and early
exits when called to store the credential.
---
 credential.c | 8 +++-
 credential.h | 3 ++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/credential.c b/credential.c
index 62be651b0..79bf62d49 100644
--- a/credential.c
+++ b/credential.c
@@ -280,8 +280,10 @@ void credential_fill(struct credential *c)
 
for (i = 0; i < c->helpers.nr; i++) {
credential_do(c, c->helpers.items[i].string, "get");
-   if (c->username && c->password)
+   if (c->username && c->password) {
+   c->retrieved = 1;
return;
+   }
if (c->quit)
die("credential helper '%s' told us to quit",
c->helpers.items[i].string);
@@ -300,6 +302,10 @@ void credential_approve(struct credential *c)
return;
if (!c->username || !c->password)
return;
+   if (c->retrieved) {
+   c->approved = 1;
+   return;
+   }
 
credential_apply_config(c);
 
diff --git a/credential.h b/credential.h
index 6b0cd16be..d99df2f52 100644
--- a/credential.h
+++ b/credential.h
@@ -8,7 +8,8 @@ struct credential {
unsigned approved:1,
 configured:1,
 quit:1,
-use_http_path:1;
+use_http_path:1,
+retrieved:1;
 
char *username;
char *password;
-- 
2.11.0

Re: [PATCH] Improvement to only call Git Credential Helper once

[Kyle Hubert]

Sorry, this patch is damaged. I'm moving to `git send-email` now.

-Kyle

On Fri, Sep 28, 2018 at 11:10 AM Kyle Hubert  wrote:
>
> When calling the Git Credential Helper that is set in the git config,
> the get command can return a credential. Git immediately turns around
> and calls the store command, even though that credential was just
> retrieved by the Helper. This creates two side effects. First of all,
> if the Helper requires a passphrase, the user has to type it in
> twice. Secondly, if the user has a number of helpers, this retrieves
> the credential from one service and writes it to all services.
>
> This commit introduces a new field in the credential struct that
> detects when the credential was retrieved using the Helper, and early
> exits when called to store the credential.
> ---
>  credential.c | 8 +++-
>  credential.h | 3 ++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/credential.c b/credential.c
> index 62be651b0..79bf62d49 100644
> --- a/credential.c
> +++ b/credential.c
> @@ -280,8 +280,10 @@ void credential_fill(struct credential *c)
>
>   for (i = 0; i < c->helpers.nr; i++) {
>   credential_do(c, c->helpers.items[i].string, "get");
> - if (c->username && c->password)
> + if (c->username && c->password) {
> + c->retrieved = 1;
>   return;
> + }
>   if (c->quit)
>   die("credential helper '%s' told us to quit",
>   c->helpers.items[i].string);
> @@ -300,6 +302,10 @@ void credential_approve(struct credential *c)
>   return;
>   if (!c->username || !c->password)
>   return;
> + if (c->retrieved) {
> + c->approved = 1;
> + return;
> + }
>
>   credential_apply_config(c);
>
> diff --git a/credential.h b/credential.h
> index 6b0cd16be..d99df2f52 100644
> --- a/credential.h
> +++ b/credential.h
> @@ -8,7 +8,8 @@ struct credential {
>   unsigned approved:1,
>   configured:1,
>   quit:1,
> - use_http_path:1;
> + use_http_path:1,
> + retrieved:1;
>
>   char *username;
>   char *password;

[PATCH] Improvement to only call Git Credential Helper once

[Kyle Hubert]

When calling the Git Credential Helper that is set in the git config,
the get command can return a credential. Git immediately turns around
and calls the store command, even though that credential was just
retrieved by the Helper. This creates two side effects. First of all,
if the Helper requires a passphrase, the user has to type it in
twice. Secondly, if the user has a number of helpers, this retrieves
the credential from one service and writes it to all services.

This commit introduces a new field in the credential struct that
detects when the credential was retrieved using the Helper, and early
exits when called to store the credential.
---
 credential.c | 8 +++-
 credential.h | 3 ++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/credential.c b/credential.c
index 62be651b0..79bf62d49 100644
--- a/credential.c
+++ b/credential.c
@@ -280,8 +280,10 @@ void credential_fill(struct credential *c)

  for (i = 0; i < c->helpers.nr; i++) {
  credential_do(c, c->helpers.items[i].string, "get");
- if (c->username && c->password)
+ if (c->username && c->password) {
+ c->retrieved = 1;
  return;
+ }
  if (c->quit)
  die("credential helper '%s' told us to quit",
  c->helpers.items[i].string);
@@ -300,6 +302,10 @@ void credential_approve(struct credential *c)
  return;
  if (!c->username || !c->password)
  return;
+ if (c->retrieved) {
+ c->approved = 1;
+ return;
+ }

  credential_apply_config(c);

diff --git a/credential.h b/credential.h
index 6b0cd16be..d99df2f52 100644
--- a/credential.h
+++ b/credential.h
@@ -8,7 +8,8 @@ struct credential {
  unsigned approved:1,
  configured:1,
  quit:1,
- use_http_path:1;
+ use_http_path:1,
+ retrieved:1;

  char *username;
  char *password;

[PATCH v3] range-diff: update stale summary of --no-dual-color

[Kyle Meyer]

275267937b (range-diff: make dual-color the default mode, 2018-08-13)
replaced --dual-color with --no-dual-color but left the option's
summary untouched.  Rewrite the summary to describe --no-dual-color
rather than dual-color.

Helped-by: Jonathan Nieder 
Helped-by: Johannes Schindelin 
Signed-off-by: Kyle Meyer 
---
 builtin/range-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index f52d45d9d6..0aa9bed41f 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -25,7 +25,7 @@ int cmd_range_diff(int argc, const char **argv, const char 
*prefix)
OPT_INTEGER(0, "creation-factor", _factor,
N_("Percentage by which creation is weighted")),
OPT_BOOL(0, "no-dual-color", _color,
-   N_("color both diff and diff-between-diffs")),
+   N_("use simple diff colors")),
OPT_END()
};
int i, j, res = 0;
-- 
2.18.0

Re: [PATCH] range-diff: update stale summary of --no-dual-color

[Kyle Meyer]

Junio C Hamano  writes:

> Johannes Schindelin  writes:

[...]

>>> -   N_("color both diff and diff-between-diffs")),
>>> +   N_("restrict coloring to outer diff markers")),
>>
>> How about "use simple diff colors" instead?

That's certainly better than the one above, and I also prefer it to
"color only based on the diff-between-diffs" in v2.

> I am wondering if it makes sense to remove the option altogether.
> I've been trying to view the comparison of the same ranges in both
> styles for the past few days, and I never found a reason to choose
> "no dual color" option myself.

But I like this suggestion even better.

[PATCH v2] range-diff: update stale summary of --no-dual-color

[Kyle Meyer]

275267937b (range-diff: make dual-color the default mode, 2018-08-13)
replaced --dual-color with --no-dual-color but left the option's
summary untouched.  Rewrite the summary to describe --no-dual-color
rather than dual-color.

Helped-by: Jonathan Nieder 
Signed-off-by: Kyle Meyer 
---
 builtin/range-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index f52d45d9d6..057afdbf46 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -25,7 +25,7 @@ int cmd_range_diff(int argc, const char **argv, const char 
*prefix)
OPT_INTEGER(0, "creation-factor", _factor,
N_("Percentage by which creation is weighted")),
OPT_BOOL(0, "no-dual-color", _color,
-   N_("color both diff and diff-between-diffs")),
+   N_("color only based on the diff-between-diffs")),
OPT_END()
};
int i, j, res = 0;
-- 
2.18.0

Re: [PATCH] range-diff: update stale summary of --no-dual-color

[Kyle Meyer]

Jonathan Nieder  writes:

 -  N_("color both diff and diff-between-diffs")),
 +  N_("restrict coloring to outer diff markers")),

[...]

> Aha: I think you're missing a few words (e.g. "color only according to
> outer diff markers").  Though based on the output, I'm not sure the
> focus on diff markers captures the difference.  (After all, some lines
> are multiple colors in --no-dual-color mode and have no diff markers.)
>
> "Restrict coloring to outer -/+ diff markers" would mean that
> everything will be in plain text, except for the minus or plus sign at
> the beginning of each line.  So you'd see a colorful strip on the left
> and everything else monochrome.

Eh, you're right, it would read like that.  Thanks.

> I think what you mean is something like "color only based on the
> diff-between-diffs".

Yeah, I that sounds OK to me.  I played around with a few different
summary lines and couldn't come up with anything that I thought was
particularly good, and then of course I ended up settling on a summary
line that didn't preserve my intended meaning :/

> Or it might be simpler to do something like
> the following.  What do you think?
>
> diff --git i/builtin/range-diff.c w/builtin/range-diff.c
> index f52d45d9d6..88c19f48d3 100644
> --- i/builtin/range-diff.c
> +++ w/builtin/range-diff.c
> @@ -20,12 +20,12 @@ int cmd_range_diff(int argc, const char **argv, const 
> char *prefix)
>  {
>   int creation_factor = 60;
>   struct diff_options diffopt = { NULL };
> - int simple_color = -1;
> + int dual_color = -1;
>   struct option options[] = {
>   OPT_INTEGER(0, "creation-factor", _factor,
>   N_("Percentage by which creation is weighted")),
> - OPT_BOOL(0, "no-dual-color", _color,
> - N_("color both diff and diff-between-diffs")),
> + OPT_BOOL(0, "dual-color", _color,
> + N_("color both diff and diff-between-diffs 
> (default)")),

I don't have a strong preference, though I lean towards making 'git
range-diff -h' show --no-dual-color since it's not the default.

Re: [PATCH] range-diff: update stale summary of --no-dual-color

[Kyle Meyer]

Jonathan Nieder  writes:

[...]

> What is an outer diff marker?

The diff markers from the diff of patches as opposed to the ones from
the original patches.  I took the term from git-range-diff.txt:

--no-dual-color::
When the commit diffs differ, `git range-diff` recreates the
original diffs' coloring, and adds outer -/+ diff markers [...]

Use `--no-dual-color` to revert to color all lines according to the
outer diff markers (and completely ignore the inner diff when it
comes to color).

-- 
Kyle

[PATCH] range-diff: update stale summary of --no-dual-color

[Kyle Meyer]

275267937b (range-diff: make dual-color the default mode, 2018-08-13)
replaced --dual-color with --no-dual-color but left the option's
summary untouched.  Rewrite the summary to describe --no-dual-color
rather than dual-color.

Signed-off-by: Kyle Meyer 
---
 builtin/range-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index f52d45d9d6..7dc90a5ec3 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -25,7 +25,7 @@ int cmd_range_diff(int argc, const char **argv, const char 
*prefix)
OPT_INTEGER(0, "creation-factor", _factor,
N_("Percentage by which creation is weighted")),
OPT_BOOL(0, "no-dual-color", _color,
-   N_("color both diff and diff-between-diffs")),
+   N_("restrict coloring to outer diff markers")),
OPT_END()
};
int i, j, res = 0;
-- 
2.18.0

Re: git-bug: Distributed bug tracker embedded in git

[Kyle Meyer]

[+cc Stefan Monnier]

Jonathan Nieder  writes:

> (cc-ing Scott)

[...]

> I believe you're thinking of TicGit[1].
>
> Some other related work is listed at [2].  Most of these projects have
> gone quiet:
>
> - ditz[3]
> - git-issues[4]
> - cil[5]
> - Bugs Everywhere[6]
> - milli by Steve Kemp, which I haven't found a copy of
> - simple defects[7]
> - kipling[8]

To add to that list: There's also BuGit [1,2], though it too seems to
have gone quiet.

[1]: https://gitlab.com/monnier/bugit
[2]: 
https://public-inbox.org/git/jwva8psr6vr.fsf-monnier+gmane.comp.version-control@gnu.org/


-- 
Kyle

Dearly Beloved In Christ.

[Mrs. Daniella Kyle]

Hi Dear,

Sorry to invade your privacy, I am Mrs. Daniella Kyle the wife of Mr
Angelo Kyle,my husband worked with Central Bank Of Philippines for ten
years before he died in the year 2012. When my late husband was alive
he deposited sum amount of Money with a Bank in UK, Presently,this
money is still with the Bank,I am in a hospital in Philippines
receiving treatment
and my doctor has confirmed to me that i have just few days to live on
earth due to my esophageal cancer.

Please I am choosing you to receive this money on my behalf,and use it
to help the less privilege. Contact me on my private email:
daniellaaky...@gmail.com

Your Sister in Christ,
Mrs. Daniella Kyle

[PATCH] gitworkflows: fix grammar in 'Merge upwards' rule

[Kyle Meyer]

Signed-off-by: Kyle Meyer 
---
 Documentation/gitworkflows.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/gitworkflows.txt b/Documentation/gitworkflows.txt
index 926e044d0..ca11c7bda 100644
--- a/Documentation/gitworkflows.txt
+++ b/Documentation/gitworkflows.txt
@@ -107,7 +107,7 @@ the unstable branch into the stable one.  Hence the 
following:
 .Merge upwards
 [caption="Rule: "]
 =
-Always commit your fixes to the oldest supported branch that require
+Always commit your fixes to the oldest supported branch that requires
 them.  Then (periodically) merge the integration branches upwards into each
 other.
 =
-- 
2.11.0

Re: [PATCH] git{,-blame}.el: remove old bitrotting Emacs code

[Kyle Meyer]

Ævar Arnfjörð Bjarmason <ava...@gmail.com> writes:

[...]

> These days these modes have very few if users, and users of git aren't

s/if// or s/if/if any/ ?

-- 
Kyle

Re: [PATCH] urlmatch: use hex2chr() in append_normalized_escapes()

[Kyle J. McKay]

On Jul 8, 2017, at 01:59, René Scharfe wrote:


Simplify the code by using hex2chr() to convert and check for invalid
characters at the same time instead of doing that sequentially with
one table lookup for each.


I think that comment may be a bit misleading as the changes are just
switching from one set of inlines to another.  Essentially the same
sequential check takes place in the hex2chr inlined function which is
being used to replace the "one table lookup for each".  An optimizing
compiler will likely eliminate any difference between the before and
after patch versions.  Nothing immediately comes to mind as an alternate
comment though, so I'm not proposing any changes to the comment.

The before version only requires knowledge of the standards-defined  
isxdigit
and the hexval function which is Git-specific, but its semantics are  
fairly

obvious from the surrounding code.

I suspect the casual reader of the function will have to go check and  
see

what hex2chr does exactly.  For example, does it accept "0x41" or not?
(It doesn't.)  What does it do with a single hex digit? (An error.)
It does do pretty much the same thing as the code it's
replacing (although that's not immediately obvious unless you go look
at it), so this seems like a reasonable change.

From the perspective of how many characters the original is versus how
many characters the replacement is, it's certainly a simplification.

But from the perspective of a reviewer of the urlmatch functionality
attempting to determine how well the code does or does not match the
respective standards it requires more work.  Now one must examine the
hex2chr function to be certain it doesn't include any extra unwanted
behavior with regards to how well urlmatch complies with the applicable
standards.  And in that sense it is not a simplification at all.

But that's all really just nit picking since hex2chr is a simple
inlined function that's relatively easy to find (and understand).

Therefore I don't have any objections to this change.

Acked-by: Kyle J. McKay


Signed-off-by: Rene Scharfe <l@web.de>
---
urlmatch.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/urlmatch.c b/urlmatch.c
index 4bbde924e8..3e42bd7504 100644
--- a/urlmatch.c
+++ b/urlmatch.c
@@ -42,12 +42,12 @@ static int append_normalized_escapes(struct  
strbuf *buf,


from_len--;
if (ch == '%') {
-   if (from_len < 2 ||
-   !isxdigit(from[0]) ||
-   !isxdigit(from[1]))
+   if (from_len < 2)
return 0;
-   ch = hexval(*from++) << 4;
-   ch |= hexval(*from++);
+   ch = hex2chr(from);
+   if (ch < 0)
+   return 0;
+   from += 2;
from_len -= 2;
was_esc = 1;
}

Re: [PATCH v2 6/7] reflog-walk: stop using fake parents

[Kyle Meyer]

Jeff King <p...@peff.net> writes:

>  Prior to this commit, we show both entries with
>  identical reflog messages. After this commit, we show
>  only the "comes back" entry. See the update in t3200
>  which demonstrates this.
>
>  Arguably either is fine, as the whole double-entry
>  thing is a bit hacky in the first place. And until a
>  recent fix, we truncated the traversal in such a case
>  anyway, which was _definitely_ wrong.

Yeah, I agree that the double-entry thing is a bit hacky and only
showing the "comes back" entry makes sense.

And with this change, I believe that the display of a rename event will
be the same for HEAD's log and the renamed branch's log, despite the
underlying entries having a different representation.

-- 
Kyle

Re: [PATCH v2 1/7] t1414: document some reflog-walk oddities

[Kyle Meyer]

Jeff King <p...@peff.net> writes:

> +test_expect_failure 'walking multiple reflogs shows all' '
> + # We expect to see all entries for all reflogs, but interleaved by
> + # date, with order no the command line breaking ties. We

s/order no/order on/

-- 
Kyle

Re: name-rev: anchor pattern without --exclude?

[Kyle Meyer]

Bryan Turner <btur...@atlassian.com> writes:

[...]

> If you don't need the ancestor traversals "git name-rev" provides,
> "git for-each-ref --count 1 --format "%(refname:short)" --points-at
>  refs/heads/" might work. That only goes back to Git 2.7.0,
> though; still quite a ways off your 1.9 target. ("git branch
> --points-at" does the same thing, I should add, and only for branches,
> but you can't directly limit its output like you can with
> "for-each-ref".. Perhaps that doesn't matter for your use case.) If
> you want names like "master~2", from your example, though,
> "--points-at" won't do what you need.

Thanks for the suggestion.  Unfortunately, I do want to consider names
like "master~2".  I was just hoping that I had overlooked another way to
achieve the

git name-rev --refs="refs/heads/*" --exclude="*/refs/heads/*" 

example I gave in my initial post.

-- 
Kyle

Re: name-rev: anchor pattern without --exclude?

[Kyle Meyer]

Junio C Hamano <gits...@pobox.com> writes:

> Kyle Meyer <k...@kyleam.com> writes:
>
>> [*] A bit more information on why I'm trying to do this: In Magit, we
>> have a work-in-progress feature that takes "snapshots" of changes
>> before they are committed.  These snapshots are stored as
>> "refs/wip/{wtree,index}/".
>>
>> We want to use name-rev to map a commit to a name in "refs/heads/",
>> ignoring these snapshot refs.
>
> What is the  in the above supposed to represent?

It's the current branch, as returned by "git symbolic-ref HEAD".

> When a user sees two refs "refs/wip/{wtree,index}/",
> does it mean: "These two represent a snapshot for changes while the
> user was working on this branch"?

Yes.

> Isn't name-rev a wrong tool to find that information?

Sorry for the confusion.  We're using "symbolic-ref HEAD" to get the
above information.  I was just trying to explain why we're dealing with
ref names that contain a "refs/heads" substring (like
refs/wip/wtree/refs/heads/master in the example I gave).

> What is the answer desired by your application when two or more
> branches point at the same commit you are interested in?  Pick one at
> random?  An error saying it cannot decide where to place the snapshot?

Our use of name-rev is just to get a friendly name for a hash.  If two
branches point to the same commit, we're fine with whatever decision
"git name-rev" makes; we just want to limit it to refs in the
"refs/heads/" namespace.

-- 
Kyle

name-rev: anchor pattern without --exclude?

[Kyle Meyer]

Hello,

I'm trying to restrict name-rev's output to a ref whose name begins with
the supplied pattern [*].  Looking at "man git-name-rev",
builtin/name-rev.c, and wildmatch.c, I don't see a way to accomplish
this with "--refs=".

Say, for example, that I want to limit name-rev's output to a ref in the
"refs/heads/" namespace.

* cc8f7dc (master) c
* ad6d6f0 b
| * 49a2156 (refs/wip/wtree/refs/heads/master) d
|/  
* b91f97a a

--

$ git name-rev b91f97a
b91f97a wip/wtree/refs/heads/master~1

$ git name-rev --refs="refs/heads/*" b91f97a
b91f97a wip/wtree/refs/heads/master~1

$ git name-rev --refs="^refs/heads/*" b91f97a
b91f97a undefined

Starting with v2.13.0, I can get my desired output using the --exclude
option.

$ git name-rev --refs="refs/heads/*" --exclude="*/refs/heads/*" b91f97a
b91f97a master~2

But, unfortunately, I'm trying to find a solution that works with
earlier Git versions (back to v1.9.4).

Am I overlooking a way to do this without the --exclude option?


[*] A bit more information on why I'm trying to do this: In Magit, we
have a work-in-progress feature that takes "snapshots" of changes
before they are committed.  These snapshots are stored as
"refs/wip/{wtree,index}/".

We want to use name-rev to map a commit to a name in "refs/heads/",
ignoring these snapshot refs.

-- 
Kyle

Re: Truncating HEAD reflog on branch move

[Kyle Meyer]

Kyle Meyer <k...@kyleam.com> writes:

> Eh, sorry about that.  I haven't dug very deeply, but it seems like the
> entry is still in .git/logs/HEAD, while "git reflog" is only showing
> they latest entry.

s/entry is still/missing entries are still/

Re: Truncating HEAD reflog on branch move

[Kyle Meyer]

Junio C Hamano <gits...@pobox.com> writes:

> "brian m. carlson" <sand...@crustytoothpaste.net> writes:
>
>> I get the following on 2.11.0:
>>
>> 2cbfbd5 HEAD@{0}:
>> 2cbfbd5 HEAD@{1}: checkout: moving from cPanel to master
>> eaf8db2 HEAD@{2}: checkout: moving from master to cPanel
>> 2cbfbd5 HEAD@{3}: clone: from 
>> https://b...@git.crustytoothpaste.net/git/bmc/homedir.git
>>
>> and this on a recent next:
>>
>> 2cbfbd5 HEAD@{0}: Branch: renamed refs/heads/master to refs/heads/new
>>
>> For this test, any repo will work; I just picked this one because it had
>> two branches I could use to generate dummy reflog entries.
>>
>> A colleague reported this to me as a bug.  I don't see anything in the
>> release notes about this as a desired behavior change, and it does seem
>> undesirable to truncate the reflog this way.  If this isn't intentional,
>> I'm happy to work up a patch.
>
> I do not think either behaviour is intentional (old one that gives a
> meaningless empty entry probably is probably not what we want, the
> new one that truncates is not what we want, either).

Eh, sorry about that.  I haven't dug very deeply, but it seems like the
entry is still in .git/logs/HEAD, while "git reflog" is only showing
they latest entry.  (Maybe because we stop consuming the entries when we
hit a null sha1 in the old id field?)

-- 
Kyle

[PATCH] [BUG] test_expect_failure mailinfo -b fatals

[Kyle J. McKay]

This subject line:

> Subject: [other] [PATCH] message

does not get along with the git mailinfo "-b" option.  In fact,
it triggers a fatal bug:

> fatal: `pos + len' is too far after the end of the buffer

While I did not do any exhaustive checking, I do happen to have
a few builds of various versions of Git lying around and it
fails at least as far back as v1.7.6.  (The -b option was
introduced in v1.6.6 I believe.)

At the very least this is now a "known breakage", so might
as well have the tests for it.

If someone comes along and fixes it it's a simple matter
to flip them to test_expect_success instead.

--Kyle

P.S. Oh yes, the real patch subject is below (love those >8).

-- 8< --
Subject: [PATCH] t5100: add some more mailinfo tests

Add some more simple mailinfo tests including a few that
produce:

  fatal: `pos + len' is too far after the end of the buffer

Mark those as 'test_expect_failure'.

Signed-off-by: Kyle J. McKay <mack...@gmail.com>
---

Notes:
checking known breakage:
subj="$(echo "Subject: [other] [PATCH] message" |
git mailinfo -b /dev/null /dev/null)" &&
test z"$subj" = z"Subject: [other] message"

fatal: `pos + len' is too far after the end of the buffer
not ok 46 - mailinfo -b trailing [PATCH] # TODO known breakage

checking known breakage:
subj="$(echo "Subject: [PATCH] [other] [PATCH] message" |
git mailinfo -b /dev/null /dev/null)" &&
test z"$subj" = z"Subject: [other] message"

fatal: `pos + len' is too far after the end of the buffer
not ok 47 - mailinfo -b separated double [PATCH] # TODO known breakage

 t/t5100-mailinfo.sh | 41 +
 1 file changed, 41 insertions(+)

diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index 7171f675..95c8 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -170,5 +170,46 @@ test_expect_success 'mailinfo with mailinfo.scissors 
config' '
test_cmp "$DATA/info0014--scissors" info0014.sc
 '
 
+test_expect_success 'mailinfo no options' '
+   subj="$(echo "Subject: [PATCH] [other] [PATCH] message" |
+   git mailinfo /dev/null /dev/null)" &&
+   test z"$subj" = z"Subject: message"
+'
+
+test_expect_success 'mailinfo -k' '
+   subj="$(echo "Subject: [PATCH] [other] [PATCH] message" |
+   git mailinfo -k /dev/null /dev/null)" &&
+   test z"$subj" = z"Subject: [PATCH] [other] [PATCH] message"
+'
+
+test_expect_success 'mailinfo -b no [PATCH]' '
+   subj="$(echo "Subject: [other] message" |
+   git mailinfo -b /dev/null /dev/null)" &&
+   test z"$subj" = z"Subject: [other] message"
+'
+
+test_expect_success 'mailinfo -b leading [PATCH]' '
+   subj="$(echo "Subject: [PATCH] [other] message" |
+   git mailinfo -b /dev/null /dev/null)" &&
+   test z"$subj" = z"Subject: [other] message"
+'
+
+test_expect_success 'mailinfo -b double [PATCH]' '
+   subj="$(echo "Subject: [PATCH] [PATCH] message" |
+   git mailinfo -b /dev/null /dev/null)" &&
+   test z"$subj" = z"Subject: message"
+'
+
+test_expect_failure 'mailinfo -b trailing [PATCH]' '
+   subj="$(echo "Subject: [other] [PATCH] message" |
+   git mailinfo -b /dev/null /dev/null)" &&
+   test z"$subj" = z"Subject: [other] message"
+'
+
+test_expect_failure 'mailinfo -b separated double [PATCH]' '
+   subj="$(echo "Subject: [PATCH] [other] [PATCH] message" |
+   git mailinfo -b /dev/null /dev/null)" &&
+   test z"$subj" = z"Subject: [other] message"
+'
 
 test_done
---

[PATCH] config.txt: add an entry for log.showSignature

[Kyle Meyer]

The configuration variable log.showSignature is mentioned in git-log's
manpage.  Document it in git-config's manpage as well.

Signed-off-by: Kyle Meyer <k...@kyleam.com>
---
 Documentation/config.txt | 4 
 1 file changed, 4 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 96e9cf8b7..a7dce9a1e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2143,6 +2143,10 @@ log.showRoot::
Tools like linkgit:git-log[1] or linkgit:git-whatchanged[1], which
normally hide the root commit will now show it. True by default.
 
+log.showSignature::
+   If true, makes linkgit:git-log[1], linkgit:git-show[1], and
+   linkgit:git-whatchanged[1] assume `--show-signature`.
+
 log.mailmap::
If true, makes linkgit:git-log[1], linkgit:git-show[1], and
linkgit:git-whatchanged[1] assume `--use-mailmap`.
-- 
2.13.0

[PATCH] doc/revisions: remove brackets from rev^-n shorthand

[Kyle Meyer]

Given that other instances of "{...}" in the revision documentation
represent literal characters of revision specifications, describing
the rev^-n shorthand as "^-{}" incorrectly suggests that
something like "master^-{1}" is an acceptable form.

Signed-off-by: Kyle Meyer <k...@kyleam.com>
---
 Documentation/revisions.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index 75d211f1a..61277469c 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -295,7 +295,7 @@ The 'r1{caret}@' notation means all parents of 'r1'.
 The 'r1{caret}!' notation includes commit 'r1' but excludes all of its parents.
 By itself, this notation denotes the single commit 'r1'.
 
-The '{caret}-{}' notation includes '' but excludes the th
+The '{caret}-' notation includes '' but excludes the th
 parent (i.e. a shorthand for '{caret}..'), with '' = 1 if
 not given. This is typically useful for merge commits where you
 can just pass '{caret}-' to get all the commits in the branch
@@ -337,7 +337,7 @@ Revision Range Summary
   as giving commit '' and then all its parents prefixed with
   '{caret}' to exclude them (and their ancestors).
 
-'{caret}-{}', e.g. 'HEAD{caret}-, HEAD{caret}-2'::
+'{caret}-', e.g. 'HEAD{caret}-, HEAD{caret}-2'::
Equivalent to '{caret}..', with '' = 1 if not
given.
 
-- 
2.12.2

[PATCH] t1400: use consistent style for test_expect_success calls

[Kyle Meyer]

Structure calls as

test_expect_success 'description' '
body
'

Use double quotes for the description if it requires parameter
expansion or contains a single quote.

Signed-off-by: Kyle Meyer <k...@kyleam.com>
---
This patch follows up on a recent t1400 series:
https://public-inbox.org/git/xmqq8tnysekm@gitster.mtv.corp.google.com/

 t/t1400-update-ref.sh | 335 +-
 1 file changed, 167 insertions(+), 168 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index a23cd7b57..dc98b4bc6 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -35,14 +35,14 @@ test_expect_success setup '
cd -
 '
 
-test_expect_success \
-   "create $m" \
-   "git update-ref $m $A &&
-test $A"' = $(cat .git/'"$m"')'
-test_expect_success \
-   "create $m with oldvalue verification" \
-   "git update-ref $m $B $A &&
-test $B"' = $(cat .git/'"$m"')'
+test_expect_success "create $m" '
+   git update-ref $m $A &&
+   test $A = $(cat .git/$m)
+'
+test_expect_success "create $m with oldvalue verification" '
+   git update-ref $m $B $A &&
+   test $B = $(cat .git/$m)
+'
 test_expect_success "fail to delete $m with stale ref" '
test_must_fail git update-ref -d $m $A &&
test $B = "$(cat .git/$m)"
@@ -67,14 +67,14 @@ test_expect_success "fail to create $n" '
test_must_fail git update-ref $n $A
 '
 
-test_expect_success \
-   "create $m (by HEAD)" \
-   "git update-ref HEAD $A &&
-test $A"' = $(cat .git/'"$m"')'
-test_expect_success \
-   "create $m (by HEAD) with oldvalue verification" \
-   "git update-ref HEAD $B $A &&
-test $B"' = $(cat .git/'"$m"')'
+test_expect_success "create $m (by HEAD)" '
+   git update-ref HEAD $A &&
+   test $A = $(cat .git/$m)
+'
+test_expect_success "create $m (by HEAD) with oldvalue verification" '
+   git update-ref HEAD $B $A &&
+   test $B = $(cat .git/$m)
+'
 test_expect_success "fail to delete $m (by HEAD) with stale ref" '
test_must_fail git update-ref -d HEAD $A &&
test $B = $(cat .git/$m)
@@ -176,17 +176,17 @@ test_expect_success '--no-create-reflog overrides 
core.logAllRefUpdates=always'
test_must_fail git reflog exists $outside
 '
 
-test_expect_success \
-   "create $m (by HEAD)" \
-   "git update-ref HEAD $A &&
-test $A"' = $(cat .git/'"$m"')'
-test_expect_success \
-   "pack refs" \
-   "git pack-refs --all"
-test_expect_success \
-   "move $m (by HEAD)" \
-   "git update-ref HEAD $B $A &&
-test $B"' = $(cat .git/'"$m"')'
+test_expect_success "create $m (by HEAD)" '
+   git update-ref HEAD $A &&
+   test $A = $(cat .git/$m)
+'
+test_expect_success 'pack refs' '
+   git pack-refs --all
+'
+test_expect_success "move $m (by HEAD)" '
+   git update-ref HEAD $B $A &&
+   test $B = $(cat .git/$m)
+'
 test_expect_success "delete $m (by HEAD) should remove both packed and loose 
$m" '
test_when_finished "rm -f .git/$m" &&
git update-ref -d HEAD $B &&
@@ -195,13 +195,13 @@ test_expect_success "delete $m (by HEAD) should remove 
both packed and loose $m"
 '
 
 cp -f .git/HEAD .git/HEAD.orig
-test_expect_success "delete symref without dereference" '
+test_expect_success 'delete symref without dereference' '
test_when_finished "cp -f .git/HEAD.orig .git/HEAD" &&
git update-ref --no-deref -d HEAD &&
test_path_is_missing .git/HEAD
 '
 
-test_expect_success "delete symref without dereference when the referred ref 
is packed" '
+test_expect_success 'delete symref without dereference when the referred ref 
is packed' '
test_when_finished "cp -f .git/HEAD.orig .git/HEAD" &&
echo foo >foo.c &&
git add foo.c &&
@@ -239,46 +239,46 @@ test_expect_success 'update-ref --no-deref -d can delete 
reference to bad ref' '
test_path_is_missing .git/refs/heads/ref-to-bad
 '
 
-test_expect_success '(not) create HEAD with old sha1' "
+test_expect_success '(not) create HEAD with old sha1' '
test_must_fail git update-ref HEAD $A $B
-"
+'
 test_expect_success "(not) prior created .git/$m" '
test_when_finished "rm -f .git/$m" &&
test_path_is_missing .git/$m
 '
 
-test_expect_success \
-   "create HEAD" \
-   "git update-ref HEAD $A&

[PATCH] doc/config: grammar fixes for core.{editor,commentChar}

[Kyle Meyer]

Signed-off-by: Kyle Meyer <k...@kyleam.com>
---
 Documentation/config.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0d8df5a9f..1df196545 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -737,13 +737,13 @@ alternative to having an `init.templateDir` where you've 
changed
 default hooks.
 
 core.editor::
-   Commands such as `commit` and `tag` that lets you edit
-   messages by launching an editor uses the value of this
+   Commands such as `commit` and `tag` that let you edit
+   messages by launching an editor use the value of this
variable when it is set, and the environment variable
`GIT_EDITOR` is not set.  See linkgit:git-var[1].
 
 core.commentChar::
-   Commands such as `commit` and `tag` that lets you edit
+   Commands such as `commit` and `tag` that let you edit
messages consider a line that begins with this character
commented, and removes them after the editor returns
(default '#').
-- 
2.12.1

Re: [PATCH 0/5] t1400: modernize style

[Kyle Meyer]

Junio C Hamano <gits...@pobox.com> writes:

> Jeff King <p...@peff.net> writes:
>
>> On Mon, Mar 20, 2017 at 08:56:11PM -0400, Kyle Meyer wrote:

[...]

>>> I also considered
>>> 
>>>   * making the quoting/spacing/breaks around the test descriptions and
>>> bodies more consistent, but I think this leads to too much code
>>> churn.
>>
>> I wouldn't mind the churn if you wanted to do it on top, but it's
>> definitely not necessary. There's nothing in 'pu' right now that touches
>> the file.
>
> Yes.  But I do not mind (actually I do prefer) if that "on top" came
> as a separate follow-up after dust from these 5 settles ;-)

OK, I follow-up with that later.  Thanks.

-- 
Kyle

Re: [PATCH 2/5] t1400: set core.logAllRefUpdates in "logged by touch" tests

[Kyle Meyer]

Jeff King <p...@peff.net> writes:

> On Mon, Mar 20, 2017 at 08:56:13PM -0400, Kyle Meyer wrote:

[...]

>> I'm confused about the setup for the "logged by touch" tests.
>> d0ab05849 (tests: remove some direct access to .git/logs, 2015-07-27)
>> changed the setup to delete the log file itself rather than its
>> contents.  The reflog was then recreated by using "--create-reflog" in
>> the "create $m (logged by touch)" test.  What I don't understand is
>> how this change fits with d0ab05849, which seems to be concerned with
>> loosening the assumption that the logs are stored in .git/logs.
>
> I suspect the answer is that the conversion was incomplete. That commit
> was done for alternate ref backends, which is an ongoing saga.
>
> I think it's OK to leave it as-is for now. It's not clear what "logged
> by touch" will look like for backends that don't use the filesystem.
> Probably it will need to call "update-ref --create-reflog" to kickstart
> it, and then further updates will automatically write to it.
>
> At that point the "rm -f" would need to become "tell the backend to
> delete this reflog". There's no command for that now, but we can add one
> later. Until then, I suspect the "rm -f" would be a noop. That means
> that the first --create-reflog test is failing to test what it claims,
> but the result passes anyway.
>
> And that probably answers the question about why the conversion is
> half-done. It was enough to get the tests to stop complaining when built
> with an alternate ref backend. :)

OK, thanks for the background.

-- 
Kyle

Re: [PATCH 0/5] t1400: modernize style

[Kyle Meyer]

Jeff King <p...@peff.net> writes:

> On Mon, Mar 20, 2017 at 08:56:11PM -0400, Kyle Meyer wrote:

[...]

>>   * moving the here-documents for log creation into the following
>> tests, but I don't think it's worth it because it makes already
>> long lines even longer.
>
> Yeah, they're quite long. Probably something like:
>
>   # arguments:
>   reflog () {
>   printf '%s %s %s <%s> %s +\t%s' \
>   "$1" "$2" \
>   "$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL" \
>   "$3" "$4"
>   }
>
>   test_expect_success 'verify $m log' '
>   {
>   reflog $Z $A 1117150200 "Initial Creation" &&
>   reflog $A $B 1117150260 "Switch" &&
>   reflog $B $A 1117150860 &&
>   } >expect &&
>   test_cmp expect .git/logs/$m
>   '
>
> wouldn't be too bad. Or maybe it's worse, because the actual format is
> all tangled up in that printf statement. ;)
>
> I'm OK with it either way.

Heh, I didn't consider that option.  I suppose I'll stick with the
here-document for now because, to my eyes, it seems a bit easier to
read.

-- 
Kyle

[PATCH 3/5] t1400: use test_path_is_* helpers

[Kyle Meyer]

Signed-off-by: Kyle Meyer <k...@kyleam.com>
---
 t/t1400-update-ref.sh | 44 ++--
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index be8b113b1..c5c8e95fc 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -49,7 +49,7 @@ test_expect_success "fail to delete $m with stale ref" '
 '
 test_expect_success "delete $m" '
git update-ref -d $m $B &&
-   ! test -f .git/$m
+   test_path_is_missing .git/$m
 '
 rm -f .git/$m
 
@@ -57,7 +57,7 @@ test_expect_success "delete $m without oldvalue verification" 
"
git update-ref $m $A &&
test $A = \$(cat .git/$m) &&
git update-ref -d $m &&
-   ! test -f .git/$m
+   test_path_is_missing .git/$m
 "
 rm -f .git/$m
 
@@ -81,7 +81,7 @@ test_expect_success "fail to delete $m (by HEAD) with stale 
ref" '
 '
 test_expect_success "delete $m (by HEAD)" '
git update-ref -d HEAD $B &&
-   ! test -f .git/$m
+   test_path_is_missing .git/$m
 '
 rm -f .git/$m
 
@@ -89,7 +89,7 @@ test_expect_success "deleting current branch adds message to 
HEAD's log" '
git update-ref $m $A &&
git symbolic-ref HEAD $m &&
git update-ref -m delete-$m -d $m &&
-   ! test -f .git/$m &&
+   test_path_is_missing .git/$m &&
grep "delete-$m$" .git/logs/HEAD
 '
 rm -f .git/$m
@@ -98,7 +98,7 @@ test_expect_success "deleting by HEAD adds message to HEAD's 
log" '
git update-ref $m $A &&
git symbolic-ref HEAD $m &&
git update-ref -m delete-by-head -d HEAD &&
-   ! test -f .git/$m &&
+   test_path_is_missing .git/$m &&
grep "delete-by-head$" .git/logs/HEAD
 '
 rm -f .git/$m
@@ -190,14 +190,14 @@ test_expect_success \
 test_expect_success "delete $m (by HEAD) should remove both packed and loose 
$m" '
git update-ref -d HEAD $B &&
! grep "$m" .git/packed-refs &&
-   ! test -f .git/$m
+   test_path_is_missing .git/$m
 '
 rm -f .git/$m
 
 cp -f .git/HEAD .git/HEAD.orig
 test_expect_success "delete symref without dereference" '
git update-ref --no-deref -d HEAD &&
-   ! test -f .git/HEAD
+   test_path_is_missing .git/HEAD
 '
 cp -f .git/HEAD.orig .git/HEAD
 
@@ -207,7 +207,7 @@ test_expect_success "delete symref without dereference when 
the referred ref is
git commit -m foo &&
git pack-refs --all &&
git update-ref --no-deref -d HEAD &&
-   ! test -f .git/HEAD
+   test_path_is_missing .git/HEAD
 '
 cp -f .git/HEAD.orig .git/HEAD
 git update-ref -d $m
@@ -242,7 +242,7 @@ test_expect_success '(not) create HEAD with old sha1' "
test_must_fail git update-ref HEAD $A $B
 "
 test_expect_success "(not) prior created .git/$m" "
-   ! test -f .git/$m
+   test_path_is_missing .git/$m
 "
 rm -f .git/$m
 
@@ -280,13 +280,13 @@ test_expect_success \
 test_expect_success "empty directory removal" '
git branch d1/d2/r1 HEAD &&
git branch d1/r2 HEAD &&
-   test -f .git/refs/heads/d1/d2/r1 &&
-   test -f .git/logs/refs/heads/d1/d2/r1 &&
+   test_path_is_file .git/refs/heads/d1/d2/r1 &&
+   test_path_is_file .git/logs/refs/heads/d1/d2/r1 &&
git branch -d d1/d2/r1 &&
-   ! test -e .git/refs/heads/d1/d2 &&
-   ! test -e .git/logs/refs/heads/d1/d2 &&
-   test -f .git/refs/heads/d1/r2 &&
-   test -f .git/logs/refs/heads/d1/r2
+   test_path_is_missing .git/refs/heads/d1/d2 &&
+   test_path_is_missing .git/logs/refs/heads/d1/d2 &&
+   test_path_is_file .git/refs/heads/d1/r2 &&
+   test_path_is_file .git/logs/refs/heads/d1/r2
 '
 
 test_expect_success "symref empty directory removal" '
@@ -294,14 +294,14 @@ test_expect_success "symref empty directory removal" '
git branch e1/r2 HEAD &&
git checkout e1/e2/r1 &&
test_when_finished "git checkout master" &&
-   test -f .git/refs/heads/e1/e2/r1 &&
-   test -f .git/logs/refs/heads/e1/e2/r1 &&
+   test_path_is_file .git/refs/heads/e1/e2/r1 &&
+   test_path_is_file .git/logs/refs/heads/e1/e2/r1 &&
git update-ref -d HEAD &&
-   ! test -e .git/refs/heads/e1/e2 &&
-   ! test -e .git/logs/refs/heads/e1/e2 &&
-   test -f .git/refs/heads/e1/r2 &&
-   test -f .git/logs/refs/heads/e1/r2 &&
-   test -f .git/logs/HEAD
+   test_path_is_missing .git/refs/heads/e1/e2 &&
+   test_path_is_missing .git/logs/refs/heads/e1/e2 &&
+   test_path_is_file .git/refs/heads/e1/r2 &&
+   test_path_is_file .git/logs/refs/heads/e1/r2 &&
+   test_path_is_file .git/logs/HEAD
 '
 
 cat >expect <

[PATCH 4/5] t1400: remove a set of unused output files

[Kyle Meyer]

This test case redirects stdout and stderr to output files, but,
unlike the other cases of redirection in the t1400 tests, these files
are not examined downstream.  Remove the redirection so that the
output is visible when running the tests verbosely.

Signed-off-by: Kyle Meyer <k...@kyleam.com>
---
 t/t1400-update-ref.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index c5c8e95fc..9e7e0227e 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -64,8 +64,8 @@ rm -f .git/$m
 test_expect_success \
"fail to create $n" \
"touch .git/$n_dir &&
-test_must_fail git update-ref $n $A >out 2>err"
-rm -f .git/$n_dir out err
+test_must_fail git update-ref $n $A"
+rm -f .git/$n_dir
 
 test_expect_success \
"create $m (by HEAD)" \
-- 
2.12.0

[PATCH 2/5] t1400: set core.logAllRefUpdates in "logged by touch" tests

[Kyle Meyer]

A group of update-ref tests verifies that logs are created when either
the log file for the ref already exists or core.logAllRefUpdates is
"true".  However, when the default for core.logAllRefUpdates was
changed in 0bee59186 (Enable reflogs by default in any repository with
a working directory., 2006-12-14), the setup for the tests was not
updated.  As a result, the "logged by touch" tests would pass even if
the log file did not exist (i.e., if "--create-reflog" was removed
from the first "git update-ref" call).

Update the "logged by touch" tests to disable core.logAllRefUpdates
explicitly so that the behavior does not depend on the default value.
While we're here, update the "logged by config" tests to use
test_config() rather than setting core.logAllRefUpdates to "true"
outside of the tests.

Signed-off-by: Kyle Meyer <k...@kyleam.com>
---

I'm confused about the setup for the "logged by touch" tests.
d0ab05849 (tests: remove some direct access to .git/logs, 2015-07-27)
changed the setup to delete the log file itself rather than its
contents.  The reflog was then recreated by using "--create-reflog" in
the "create $m (logged by touch)" test.  What I don't understand is
how this change fits with d0ab05849, which seems to be concerned with
loosening the assumption that the logs are stored in .git/logs.  For
these particular tests, we are still removing
.git/logs/refs/heads/master explictly, so why not leave the empty file
around so that the "create $m (logged by touch)" actually tests that
update-ref call is logged by the existence of the log rather than
using "--create-reflog", which overrides this?

 t/t1400-update-ref.sh | 23 ---
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index fde5b98af..be8b113b1 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -260,17 +260,20 @@ rm -f .git/$m
 rm -f .git/logs/refs/heads/master
 test_expect_success \
"create $m (logged by touch)" \
-   'GIT_COMMITTER_DATE="2005-05-26 23:30" \
+   'test_config core.logAllRefUpdates false &&
+GIT_COMMITTER_DATE="2005-05-26 23:30" \
 git update-ref --create-reflog HEAD '"$A"' -m "Initial Creation" &&
 test '"$A"' = $(cat .git/'"$m"')'
 test_expect_success \
"update $m (logged by touch)" \
-   'GIT_COMMITTER_DATE="2005-05-26 23:31" \
+   'test_config core.logAllRefUpdates false &&
+GIT_COMMITTER_DATE="2005-05-26 23:31" \
 git update-ref HEAD'" $B $A "'-m "Switch" &&
 test '"$B"' = $(cat .git/'"$m"')'
 test_expect_success \
"set $m (logged by touch)" \
-   'GIT_COMMITTER_DATE="2005-05-26 23:41" \
+   'test_config core.logAllRefUpdates false &&
+GIT_COMMITTER_DATE="2005-05-26 23:41" \
 git update-ref HEAD'" $A &&
 test $A"' = $(cat .git/'"$m"')'
 
@@ -312,23 +315,21 @@ test_expect_success \
 rm -rf .git/$m .git/logs expect
 
 test_expect_success \
-   'enable core.logAllRefUpdates' \
-   'git config core.logAllRefUpdates true &&
-test true = $(git config --bool --get core.logAllRefUpdates)'
-
-test_expect_success \
"create $m (logged by config)" \
-   'GIT_COMMITTER_DATE="2005-05-26 23:32" \
+   'test_config core.logAllRefUpdates true &&
+GIT_COMMITTER_DATE="2005-05-26 23:32" \
 git update-ref HEAD'" $A "'-m "Initial Creation" &&
 test '"$A"' = $(cat .git/'"$m"')'
 test_expect_success \
"update $m (logged by config)" \
-   'GIT_COMMITTER_DATE="2005-05-26 23:33" \
+   'test_config core.logAllRefUpdates true &&
+GIT_COMMITTER_DATE="2005-05-26 23:33" \
 git update-ref HEAD'" $B $A "'-m "Switch" &&
 test '"$B"' = $(cat .git/'"$m"')'
 test_expect_success \
"set $m (logged by config)" \
-   'GIT_COMMITTER_DATE="2005-05-26 23:43" \
+   'test_config core.logAllRefUpdates true &&
+GIT_COMMITTER_DATE="2005-05-26 23:43" \
 git update-ref HEAD '"$A &&
 test $A"' = $(cat .git/'"$m"')'
 
-- 
2.12.0

[PATCH 0/5] t1400: modernize style

[Kyle Meyer]

These patches follow up on Peff's suggestion to modernize the style in
t1400-update-ref.sh.

20170217082253.kxjezkxfqkfxj...@sigill.intra.peff.net

The first two commits aren't concerned with "modernizing" the tests,
but instead address issues that I noticed while looking more closely
at t1400.

I also considered

  * making the quoting/spacing/breaks around the test descriptions and
bodies more consistent, but I think this leads to too much code
churn.

  * moving the here-documents for log creation into the following
tests, but I don't think it's worth it because it makes already
long lines even longer.

  t1400: rename test descriptions to be unique
  t1400: set core.logAllRefUpdates in "logged by touch" tests
  t1400: use test_path_is_* helpers
  t1400: remove a set of unused output files
  t1400: use test_when_finished for cleanup

 t/t1400-update-ref.sh | 154 +-
 1 file changed, 78 insertions(+), 76 deletions(-)

-- 
2.12.0

[PATCH 5/5] t1400: use test_when_finished for cleanup

[Kyle Meyer]

Move cleanup lines that occur after test blocks into
test_when_finished calls within the test bodies.  Don't move cleanup
lines that seem to be related to mutiple tests rather than a single
test.

Signed-off-by: Kyle Meyer <k...@kyleam.com>
---
 t/t1400-update-ref.sh | 81 ++-
 1 file changed, 41 insertions(+), 40 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 9e7e0227e..a23cd7b57 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -48,24 +48,24 @@ test_expect_success "fail to delete $m with stale ref" '
test $B = "$(cat .git/$m)"
 '
 test_expect_success "delete $m" '
+   test_when_finished "rm -f .git/$m" &&
git update-ref -d $m $B &&
test_path_is_missing .git/$m
 '
-rm -f .git/$m
 
-test_expect_success "delete $m without oldvalue verification" "
+test_expect_success "delete $m without oldvalue verification" '
+   test_when_finished "rm -f .git/$m" &&
git update-ref $m $A &&
-   test $A = \$(cat .git/$m) &&
+   test $A = $(cat .git/$m) &&
git update-ref -d $m &&
test_path_is_missing .git/$m
-"
-rm -f .git/$m
+'
 
-test_expect_success \
-   "fail to create $n" \
-   "touch .git/$n_dir &&
-test_must_fail git update-ref $n $A"
-rm -f .git/$n_dir
+test_expect_success "fail to create $n" '
+   test_when_finished "rm -f .git/$n_dir" &&
+   touch .git/$n_dir &&
+   test_must_fail git update-ref $n $A
+'
 
 test_expect_success \
"create $m (by HEAD)" \
@@ -80,28 +80,28 @@ test_expect_success "fail to delete $m (by HEAD) with stale 
ref" '
test $B = $(cat .git/$m)
 '
 test_expect_success "delete $m (by HEAD)" '
+   test_when_finished "rm -f .git/$m" &&
git update-ref -d HEAD $B &&
test_path_is_missing .git/$m
 '
-rm -f .git/$m
 
 test_expect_success "deleting current branch adds message to HEAD's log" '
+   test_when_finished "rm -f .git/$m" &&
git update-ref $m $A &&
git symbolic-ref HEAD $m &&
git update-ref -m delete-$m -d $m &&
test_path_is_missing .git/$m &&
grep "delete-$m$" .git/logs/HEAD
 '
-rm -f .git/$m
 
 test_expect_success "deleting by HEAD adds message to HEAD's log" '
+   test_when_finished "rm -f .git/$m" &&
git update-ref $m $A &&
git symbolic-ref HEAD $m &&
git update-ref -m delete-by-head -d HEAD &&
test_path_is_missing .git/$m &&
grep "delete-by-head$" .git/logs/HEAD
 '
-rm -f .git/$m
 
 test_expect_success 'update-ref does not create reflogs by default' '
test_when_finished "git update-ref -d $outside" &&
@@ -188,20 +188,21 @@ test_expect_success \
"git update-ref HEAD $B $A &&
 test $B"' = $(cat .git/'"$m"')'
 test_expect_success "delete $m (by HEAD) should remove both packed and loose 
$m" '
+   test_when_finished "rm -f .git/$m" &&
git update-ref -d HEAD $B &&
! grep "$m" .git/packed-refs &&
test_path_is_missing .git/$m
 '
-rm -f .git/$m
 
 cp -f .git/HEAD .git/HEAD.orig
 test_expect_success "delete symref without dereference" '
+   test_when_finished "cp -f .git/HEAD.orig .git/HEAD" &&
git update-ref --no-deref -d HEAD &&
test_path_is_missing .git/HEAD
 '
-cp -f .git/HEAD.orig .git/HEAD
 
 test_expect_success "delete symref without dereference when the referred ref 
is packed" '
+   test_when_finished "cp -f .git/HEAD.orig .git/HEAD" &&
echo foo >foo.c &&
git add foo.c &&
git commit -m foo &&
@@ -209,7 +210,7 @@ test_expect_success "delete symref without dereference when 
the referred ref is
git update-ref --no-deref -d HEAD &&
test_path_is_missing .git/HEAD
 '
-cp -f .git/HEAD.orig .git/HEAD
+
 git update-ref -d $m
 
 test_expect_success 'update-ref -d is not confused by self-reference' '
@@ -241,10 +242,10 @@ test_expect_success 'update-ref --no-deref -d can delete 
reference to bad ref' '
 test_expect_success '(not) create HEAD with old sha1' "
test_must_fail git update-ref HEAD $A $B
 "
-test_expect_success "(not) prior created .git/$m" "
+test_expect_success "(not) prior created .git/$m" '
+   test_when_finished "rm -f .git/$m" &&
test_path_is_missing .git/$m
-"
-rm -f .git/$m
+'
 
 test_expect_s

[PATCH 1/5] t1400: rename test descriptions to be unique

[Kyle Meyer]

A few tests share their description with another test.  Extend the
descriptions to indicate how the tests differ.

Signed-off-by: Kyle Meyer <k...@kyleam.com>
---
 t/t1400-update-ref.sh | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 825422341..fde5b98af 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -40,7 +40,7 @@ test_expect_success \
"git update-ref $m $A &&
 test $A"' = $(cat .git/'"$m"')'
 test_expect_success \
-   "create $m" \
+   "create $m with oldvalue verification" \
"git update-ref $m $B $A &&
 test $B"' = $(cat .git/'"$m"')'
 test_expect_success "fail to delete $m with stale ref" '
@@ -72,7 +72,7 @@ test_expect_success \
"git update-ref HEAD $A &&
 test $A"' = $(cat .git/'"$m"')'
 test_expect_success \
-   "create $m (by HEAD)" \
+   "create $m (by HEAD) with oldvalue verification" \
"git update-ref HEAD $B $A &&
 test $B"' = $(cat .git/'"$m"')'
 test_expect_success "fail to delete $m (by HEAD) with stale ref" '
@@ -307,7 +307,7 @@ $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 
+   Switch
 $B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +
 EOF
 test_expect_success \
-   "verifying $m's log" \
+   "verifying $m's log (logged by touch)" \
"test_cmp expect .git/logs/$m"
 rm -rf .git/$m .git/logs expect
 
@@ -338,7 +338,7 @@ $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 
+   Switch
 $B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 +
 EOF
 test_expect_success \
-   "verifying $m's log" \
+   "verifying $m's log (logged by config)" \
'test_cmp expect .git/logs/$m'
 rm -f .git/$m .git/logs/$m expect
 
-- 
2.12.0

Re: [PATCH v2 1/4] delete_ref: accept a reflog message argument

[Kyle Meyer]

Duy Nguyen  writes:

> You'll probably want to update the comment block above if msg can be
> NULL. We have _very_ good documentation in this file, let's keep it
> uptodate.

Looking at how other functions in refs.h document their "msg" or
"logmsg" parameter, none seem to mention it explicitly.  update_ref
refers to ref_transaction_update, and
ref_transaction_{update,create,delete,verify} refer to ref.h's
"Reference transaction updates" comment.

delete_ref's docstring already mentions that "flag" is passed to
ref_transaction_delete, so perhaps it should mention "msg" here as
well.

-- >8 --
Subject: [PATCH] delete_ref: mention "msg" parameter in docstring

delete_ref() was recently extended to take a "msg" argument.
---
 refs.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/refs.h b/refs.h
index e529f4c3a..988750218 100644
--- a/refs.h
+++ b/refs.h
@@ -274,7 +274,8 @@ int reflog_exists(const char *refname);
  * verify that the current value of the reference is old_sha1 before
  * deleting it. If old_sha1 is NULL, delete the reference if it
  * exists, regardless of its old value. It is an error for old_sha1 to
- * be NULL_SHA1. flags is passed through to ref_transaction_delete().
+ * be NULL_SHA1. msg and flags are passed through to
+ * ref_transaction_delete().
  */
 int delete_ref(const char *msg, const char *refname,
   const unsigned char *old_sha1, unsigned int flags);
-- 
2.11.1

Re: [PATCH v2 0/4] delete_ref: support reflog messages

[Kyle Meyer]

Junio C Hamano <gits...@pobox.com> writes:

> These looked reasonable.  I had to resolve conflicts with another
> topic in flight that removed set_worktree_head_symref() function
> while queuing, and I think I resolved it correctly, but please
> double check what is pushed out on 'pu'.

The merge looks right to me, thanks.  Thanks also for touching up the
tab/space issue in t3200-branch.sh.

-- 
Kyle

[PATCH v2 1/4] delete_ref: accept a reflog message argument

[Kyle Meyer]

When the current branch is renamed with 'git branch -m/-M' or deleted
with 'git update-ref -m -d', the event is recorded in HEAD's log
with an empty message.  In preparation for adding a more meaningful
message to HEAD's log in these cases, update delete_ref() to take a
message argument and pass it along to ref_transaction_delete().
Modify all callers to pass NULL for the new message argument; no
change in behavior is intended.

Note that this is relevant for HEAD's log but not for the deleted
ref's log, which is currently deleted along with the ref.  Even if it
were not, an entry for the deletion wouldn't be present in the deleted
ref's log.  files_transaction_commit() writes to the log if
REF_NEEDS_COMMIT or REF_LOG_ONLY are set, but lock_ref_for_update()
doesn't set REF_NEEDS_COMMIT for the deleted ref because REF_DELETING
is set.  In contrast, the update for HEAD has REF_LOG_ONLY set by
split_head_update(), resulting in the deletion being logged.

Signed-off-by: Kyle Meyer <k...@kyleam.com>
---
 builtin/am.c   | 4 ++--
 builtin/branch.c   | 2 +-
 builtin/notes.c| 4 ++--
 builtin/remote.c   | 4 ++--
 builtin/replace.c  | 2 +-
 builtin/reset.c| 2 +-
 builtin/symbolic-ref.c | 2 +-
 builtin/tag.c  | 2 +-
 builtin/update-ref.c   | 2 +-
 fast-import.c  | 2 +-
 refs.c | 6 +++---
 refs.h | 4 ++--
 refs/files-backend.c   | 6 +++---
 transport.c| 2 +-
 14 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 31fb60578..f7a7a971f 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1049,7 +1049,7 @@ static void am_setup(struct am_state *state, enum 
patch_format patch_format,
} else {
write_state_text(state, "abort-safety", "");
if (!state->rebasing)
-   delete_ref("ORIG_HEAD", NULL, 0);
+   delete_ref(NULL, "ORIG_HEAD", NULL, 0);
}
 
/*
@@ -2172,7 +2172,7 @@ static void am_abort(struct am_state *state)
has_curr_head ? _head : NULL, 0,
UPDATE_REFS_DIE_ON_ERR);
else if (curr_branch)
-   delete_ref(curr_branch, NULL, REF_NODEREF);
+   delete_ref(NULL, curr_branch, NULL, REF_NODEREF);
 
free(curr_branch);
am_destroy(state);
diff --git a/builtin/branch.c b/builtin/branch.c
index 9d30f55b0..8f8242e07 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -251,7 +251,7 @@ static int delete_branches(int argc, const char **argv, int 
force, int kinds,
goto next;
}
 
-   if (delete_ref(name, is_null_sha1(sha1) ? NULL : sha1,
+   if (delete_ref(NULL, name, is_null_sha1(sha1) ? NULL : sha1,
   REF_NODEREF)) {
error(remote_branch
  ? _("Error deleting remote-tracking branch '%s'")
diff --git a/builtin/notes.c b/builtin/notes.c
index 5248a9bad..4b492abd4 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -681,9 +681,9 @@ static int merge_abort(struct notes_merge_options *o)
 * notes_merge_abort() to remove .git/NOTES_MERGE_WORKTREE.
 */
 
-   if (delete_ref("NOTES_MERGE_PARTIAL", NULL, 0))
+   if (delete_ref(NULL, "NOTES_MERGE_PARTIAL", NULL, 0))
ret += error(_("failed to delete ref NOTES_MERGE_PARTIAL"));
-   if (delete_ref("NOTES_MERGE_REF", NULL, REF_NODEREF))
+   if (delete_ref(NULL, "NOTES_MERGE_REF", NULL, REF_NODEREF))
ret += error(_("failed to delete ref NOTES_MERGE_REF"));
if (notes_merge_abort(o))
ret += error(_("failed to remove 'git notes merge' worktree"));
diff --git a/builtin/remote.c b/builtin/remote.c
index 5339ed6ad..2b415911b 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -691,7 +691,7 @@ static int mv(int argc, const char **argv)
read_ref_full(item->string, RESOLVE_REF_READING, oid.hash, 
);
if (!(flag & REF_ISSYMREF))
continue;
-   if (delete_ref(item->string, NULL, REF_NODEREF))
+   if (delete_ref(NULL, item->string, NULL, REF_NODEREF))
die(_("deleting '%s' failed"), item->string);
}
for (i = 0; i < remote_branches.nr; i++) {
@@ -1248,7 +1248,7 @@ static int set_head(int argc, const char **argv)
head_name = xstrdup(states.heads.items[0].string);
free_remote_ref_states();
} else if (opt_d && !opt_a && argc == 1) {
-   if (delete_ref(buf.buf, NULL, REF_NODEREF))
+   if (delete_ref(NULL, buf.buf, NULL, REF_NODEREF))
result |= error(_("

[PATCH v2 0/4] delete_ref: support reflog messages

[Kyle Meyer]

Thanks to Junio and Peff for their feedback on v1.

  https://public-inbox.org/git/20170217035800.13214-1-k...@kyleam.com/T/#u

Changes from v1:

 * "msg" is now positioned as the first argument to delete_ref() to
   match update_ref().

   20170217081205.zn7j6d5cffgdv...@sigill.intra.peff.net

 * A "delete by head" test case has been added for the update-ref
   change.

   20170217082253.kxjezkxfqkfxj...@sigill.intra.peff.net

 * The added tests no longer send grep's output to /dev/null.

   20170217082253.kxjezkxfqkfxj...@sigill.intra.peff.net

 * Renaming the current branch is represented by two entries in HEAD's
   log, both which reuse the log message passed to rename_ref().
   
   20170217083112.vn7m4udsopmlv...@sigill.intra.peff.net,
   20170217195549.z6uyy7hbbhj5a...@sigill.intra.peff.net

 * Corrected a few places in the commit messages where "delete_refs"
   was written instead of "delete_ref".


Kyle Meyer (4):
  delete_ref: accept a reflog message argument
  update-ref: pass reflog message to delete_ref()
  rename_ref: replace empty message in HEAD's log
  branch: record creation of renamed branch in HEAD's log

 branch.c   |  5 +++--
 branch.h   |  3 ++-
 builtin/am.c   |  4 ++--
 builtin/branch.c   |  7 ---
 builtin/notes.c|  4 ++--
 builtin/remote.c   |  4 ++--
 builtin/replace.c  |  2 +-
 builtin/reset.c|  2 +-
 builtin/symbolic-ref.c |  2 +-
 builtin/tag.c  |  2 +-
 builtin/update-ref.c   |  2 +-
 fast-import.c  |  2 +-
 refs.c |  6 +++---
 refs.h |  7 ---
 refs/files-backend.c   | 10 +-
 t/t1400-update-ref.sh  | 18 ++
 t/t3200-branch.sh  |  6 ++
 transport.c|  2 +-
 18 files changed, 58 insertions(+), 30 deletions(-)

-- 
2.11.1

[PATCH v2 3/4] rename_ref: replace empty message in HEAD's log

[Kyle Meyer]

When the current branch is renamed, the deletion of the old ref is
recorded in HEAD's log with an empty message.  Now that delete_ref()
accepts a reflog message, provide a more descriptive message by
passing along the log message that is given to rename_ref().

The next step will be to extend HEAD's log to also include the second
part of the rename, the creation of the new branch.

Helped-by: Jeff King <p...@peff.net>
Signed-off-by: Kyle Meyer <k...@kyleam.com>
---
 refs/files-backend.c | 2 +-
 t/t3200-branch.sh| 5 +
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 299eb4d8a..f6e7c192c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2616,7 +2616,7 @@ static int files_rename_ref(struct ref_store *ref_store,
return error("unable to move logfile logs/%s to 
"TMP_RENAMED_LOG": %s",
oldrefname, strerror(errno));
 
-   if (delete_ref(NULL, oldrefname, orig_sha1, REF_NODEREF)) {
+   if (delete_ref(logmsg, oldrefname, orig_sha1, REF_NODEREF)) {
error("unable to delete old %s", oldrefname);
goto rollback;
}
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 8a833f354..0dbc54003 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -139,6 +139,11 @@ test_expect_success 'git branch -M baz bam should succeed 
when baz is checked ou
test $(git rev-parse --abbrev-ref HEAD) = bam
 '
 
+test_expect_success 'git branch -M baz bam should add entry to .git/logs/HEAD' 
'
+msg="Branch: renamed refs/heads/baz to refs/heads/bam" &&
+   grep " 0\{40\}.*$msg$" .git/logs/HEAD
+'
+
 test_expect_success 'git branch -M baz bam should succeed when baz is checked 
out as linked working tree' '
git checkout master &&
git worktree add -b baz bazdir &&
-- 
2.11.1

[PATCH v2 4/4] branch: record creation of renamed branch in HEAD's log

[Kyle Meyer]

Renaming the current branch adds an event to the current branch's log
and to HEAD's log.  However, the logged entries differ.  The entry in
the branch's log represents the entire renaming operation (the old and
new hash are identical), whereas the entry in HEAD's log represents
the deletion only (the new sha1 is null).

Extend replace_each_worktree_head_symref(), whose only caller is
branch_rename(), to take a reflog message argument.  This allows the
creation of the new ref to be recorded in HEAD's log.  As a result,
the renaming event is represented by two entries (a deletion and a
creation entry) in HEAD's log.

It's a bit unfortunate that the branch's log and HEAD's log now
represent the renaming event in different ways.  Given that the
renaming operation is not atomic, the two-entry form is a more
accurate representation of the operation and is more useful for
debugging purposes if a failure occurs between the deletion and
creation events.  It would make sense to move the branch's log to the
two-entry form, but this would involve changes to how the rename is
carried out and to how the update flags and reflogs are processed for
deletions, so it may not be worth the effort.

Based-on-patch-by: Jeff King <p...@peff.net>
Signed-off-by: Kyle Meyer <k...@kyleam.com>
---
 branch.c | 5 +++--
 branch.h | 3 ++-
 builtin/branch.c | 5 +++--
 refs.h   | 3 ++-
 refs/files-backend.c | 4 ++--
 t/t3200-branch.sh| 5 +++--
 6 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/branch.c b/branch.c
index b955d4f31..5c12036b0 100644
--- a/branch.c
+++ b/branch.c
@@ -345,7 +345,8 @@ void die_if_checked_out(const char *branch, int 
ignore_current_worktree)
branch, wt->path);
 }
 
-int replace_each_worktree_head_symref(const char *oldref, const char *newref)
+int replace_each_worktree_head_symref(const char *oldref, const char *newref,
+ const char *logmsg)
 {
int ret = 0;
struct worktree **worktrees = get_worktrees(0);
@@ -358,7 +359,7 @@ int replace_each_worktree_head_symref(const char *oldref, 
const char *newref)
continue;
 
if (set_worktree_head_symref(get_worktree_git_dir(worktrees[i]),
-newref)) {
+newref, logmsg)) {
ret = -1;
error(_("HEAD of working tree %s is not updated"),
  worktrees[i]->path);
diff --git a/branch.h b/branch.h
index 3103eb9ad..b07788558 100644
--- a/branch.h
+++ b/branch.h
@@ -71,6 +71,7 @@ extern void die_if_checked_out(const char *branch, int 
ignore_current_worktree);
  * This will be used when renaming a branch. Returns 0 if successful, non-zero
  * otherwise.
  */
-extern int replace_each_worktree_head_symref(const char *oldref, const char 
*newref);
+extern int replace_each_worktree_head_symref(const char *oldref, const char 
*newref,
+const char *logmsg);
 
 #endif
diff --git a/builtin/branch.c b/builtin/branch.c
index 8f8242e07..e1f97dcfc 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -579,14 +579,15 @@ static void rename_branch(const char *oldname, const char 
*newname, int force)
 
if (rename_ref(oldref.buf, newref.buf, logmsg.buf))
die(_("Branch rename failed"));
-   strbuf_release();
 
if (recovery)
warning(_("Renamed a misnamed branch '%s' away"), oldref.buf + 
11);
 
-   if (replace_each_worktree_head_symref(oldref.buf, newref.buf))
+   if (replace_each_worktree_head_symref(oldref.buf, newref.buf, 
logmsg.buf))
die(_("Branch renamed to %s, but HEAD is not updated!"), 
newname);
 
+   strbuf_release();
+
strbuf_addf(, "branch.%s", oldref.buf + 11);
strbuf_release();
strbuf_addf(, "branch.%s", newref.buf + 11);
diff --git a/refs.h b/refs.h
index 5880886a7..e529f4c3a 100644
--- a/refs.h
+++ b/refs.h
@@ -334,7 +334,8 @@ int create_symref(const char *refname, const char *target, 
const char *logmsg);
  * $GIT_DIR points to.
  * Return 0 if successful, non-zero otherwise.
  * */
-int set_worktree_head_symref(const char *gitdir, const char *target);
+int set_worktree_head_symref(const char *gitdir, const char *target,
+const char *logmsg);
 
 enum action_on_err {
UPDATE_REFS_MSG_ON_ERR,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f6e7c192c..42b137bb1 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3055,7 +3055,7 @@ static int files_create_symref(struct ref_store 
*ref_store,
return ret;
 }
 
-int set_worktree_head_symref(const char *gitdir, const char *target)
+int set_worktree_head_symref(const char *gitdir, const char *target, const 
char *logmsg)
 {
  

[PATCH v2 2/4] update-ref: pass reflog message to delete_ref()

[Kyle Meyer]

Now that delete_ref() accepts a reflog message, pass the user-provided
message to delete_ref() rather than silently dropping it.

Signed-off-by: Kyle Meyer <k...@kyleam.com>
---
 builtin/update-ref.c  |  2 +-
 t/t1400-update-ref.sh | 18 ++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 86d006d36..0b2ecf41a 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -433,7 +433,7 @@ int cmd_update_ref(int argc, const char **argv, const char 
*prefix)
 * For purposes of backwards compatibility, we treat
 * NULL_SHA1 as "don't care" here:
 */
-   return delete_ref(NULL, refname,
+   return delete_ref(msg, refname,
  (oldval && !is_null_sha1(oldsha1)) ? oldsha1 
: NULL,
  flags);
else
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index b0ffc0b57..6e112fb5f 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -85,6 +85,24 @@ test_expect_success "delete $m (by HEAD)" '
 '
 rm -f .git/$m
 
+test_expect_success "deleting current branch adds message to HEAD's log" '
+   git update-ref $m $A &&
+   git symbolic-ref HEAD $m &&
+   git update-ref -m delete-$m -d $m &&
+   ! test -f .git/$m &&
+   grep "delete-$m$" .git/logs/HEAD
+'
+rm -f .git/$m
+
+test_expect_success "deleting by HEAD adds message to HEAD's log" '
+   git update-ref $m $A &&
+   git symbolic-ref HEAD $m &&
+   git update-ref -m delete-by-head -d HEAD &&
+   ! test -f .git/$m &&
+   grep "delete-by-head$" .git/logs/HEAD
+'
+rm -f .git/$m
+
 test_expect_success 'update-ref does not create reflogs by default' '
test_when_finished "git update-ref -d $outside" &&
git update-ref $outside $A &&
-- 
2.11.1

Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log

[Kyle Meyer]

Jeff King <p...@peff.net> writes:

> On Fri, Feb 17, 2017 at 02:43:50PM -0500, Jeff King wrote:
>
>> Yes. I think the options are basically (in order of decreasing
>> preference in my opinion):
>> 
>>   1. Log a rename entry (same sha1, but note the rename in the free-form
>>  text).
>> 
>>   2. Log a delete (sha1 goes to null) followed by a creation (from null
>>  back to the original sha1).
>> 
>>   3. Log nothing at all for HEAD.
>> 
>> This does half of (2). If we do the second half, then I'd prefer it to
>> (3). But if we can do (1), that is better still (IMHO).

[...]

>> I'm actually confused about which bit of code is updating HEAD. I do not
>> see it either in files_rename_ref() or in the caller. Yet it clearly
>> happens. But that is the code that would know enough to do (1) or the
>> second half of (2) above.
>
> Ah, I found it. It's in replace_each_worktree_head_symref() these days,
> which does not bother to pass a log message.
> 
> So I think the second half of (2) is probably something like the patch
> below.
>
> Thinking on it more, we probably _do_ want two entries. Because the
> operations are not atomic, it's possible that we may end up in a
> half-way state after the first entry is written. And when debugging such
> a case, I'd much rather see the first half of the operation logged than
> nothing at all.

OK, I'll have a go at replacing patch 3 with this approach.

-- 
Kyle

Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log

[Kyle Meyer]

Junio C Hamano <gits...@pobox.com> writes:

[...]

> Do we even want these "internal" delete_ref() invocations to be
> logged in HEAD's reflog?  I understand that this is inside the
> implementation of renaming an old ref to a new ref, and reflog
> message given to delete_ref() would matter only if the HEAD happens
> to be pointing at old ref---but then HEAD will be repointed to the
> new ref by somebody else [*1*] that called this function to rename
> old to new and it _will_ log it.

I know the discussion has developed further, but just a note that I
think the last statement is inaccurate: currently, a rename will not be
recorded in HEAD's log.  "git branch -m" will show a renaming event in
the new branch's log, but the only trace of the event in HEAD's log is
the deletion entry with an empty message.

-- 
Kyle

Re: [PATCH 2/3] update-ref: pass reflog message argument to delete_refs

[Kyle Meyer]

Jeff King <p...@peff.net> writes:

[...]

>> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
>> index b0ffc0b57..65918d984 100755
>> --- a/t/t1400-update-ref.sh
>> +++ b/t/t1400-update-ref.sh
>> @@ -85,6 +85,15 @@ test_expect_success "delete $m (by HEAD)" '
>>  '
>>  rm -f .git/$m
>>  
>> +test_expect_success "deleting current branch adds message to HEAD's log" '
>> +git update-ref $m $A &&
>> +git symbolic-ref HEAD $m &&
>> +git update-ref -mdelmsg -d $m &&
>> +! test -f .git/$m &&
>> +grep "delmsg$" .git/logs/HEAD >/dev/null
>> +'
>> +rm -f .git/$m
>
> I think covering this with a test is good.
>
> I don't know if it's also worth testing that deleting via HEAD also
> writes the reflog. I.e.,:
>
>   git update-ref -m delete-by-head -d HEAD

Seems reasonable to cover this case as well.

> Some of the style here is a bit out-dated, but I think you are just
> matching the surrounding tests.  So that's OK by me (though a patch to
> modernize the whole thing would be welcome, too).

Right.  I'd be happy to follow up with a patch updating the style in
t1400-update-ref.sh.

> For reference, the two things I notice are:
>
>   - we prefer test_path_is_missing to "! test -f" these days.
>
>   - we don't redirect the output of grep (it's handled already in
> non-verbose mode, and in verbose mode we try to be...verbose).

Would moving cleanup like "rm -f .git/$m" within the test's body using
test_when_finished also be preferred?

-- 
Kyle

Re: [PATCH 1/3] delete_refs(): accept a reflog message argument

[Kyle Meyer]

Junio C Hamano <gits...@pobox.com> writes:

> Jeff King <p...@peff.net> writes:
>
>>> diff --git a/refs.h b/refs.h
>>> index 9fbff90e7..81627a63d 100644
>>> --- a/refs.h
>>> +++ b/refs.h
>>> @@ -277,7 +277,7 @@ int reflog_exists(const char *refname);
>>>   * be NULL_SHA1. flags is passed through to ref_transaction_delete().
>>>   */
>>>  int delete_ref(const char *refname, const unsigned char *old_sha1,
>>> -  unsigned int flags);
>>> +  unsigned int flags, const char *msg);
>>
>> Should the "msg" argument go at the beginning, to match update_ref()?
>
> Probably.  rename/create have the message at the end but their
> parameters are very different from update/delete.  The parameters
> update and delete take are not identical, but we can view them as a
> lot more similar than the other two.  So I think it makes sense for
> delete to try matching update, even though trying to make all four
> the same may proabably be pointless.

I put "msg" after "flags" because that's where it occurs in
ref_transaction_delete(), but matching update_ref() makes sense.

-- 
Kyle

[PATCH 1/3] delete_refs(): accept a reflog message argument

[Kyle Meyer]

When the current branch is renamed with 'git branch -m/-M' or deleted
with 'git update-ref -m -d', the event is recorded in HEAD's log
with an empty message.

In preparation for adding a more meaningful message to HEAD's log in
these cases, update delete_ref() to take a message argument and pass
it along to ref_transaction_delete().  Modify all callers to pass NULL
for the new message argument; no change in behavior is intended.

Signed-off-by: Kyle Meyer <k...@kyleam.com>
---
 builtin/am.c   | 4 ++--
 builtin/branch.c   | 2 +-
 builtin/notes.c| 4 ++--
 builtin/remote.c   | 4 ++--
 builtin/replace.c  | 2 +-
 builtin/reset.c| 2 +-
 builtin/symbolic-ref.c | 2 +-
 builtin/tag.c  | 2 +-
 builtin/update-ref.c   | 2 +-
 fast-import.c  | 2 +-
 refs.c | 4 ++--
 refs.h | 2 +-
 refs/files-backend.c   | 6 +++---
 transport.c| 2 +-
 14 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 31fb60578..e08c739d4 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1049,7 +1049,7 @@ static void am_setup(struct am_state *state, enum 
patch_format patch_format,
} else {
write_state_text(state, "abort-safety", "");
if (!state->rebasing)
-   delete_ref("ORIG_HEAD", NULL, 0);
+   delete_ref("ORIG_HEAD", NULL, 0, NULL);
}
 
/*
@@ -2172,7 +2172,7 @@ static void am_abort(struct am_state *state)
has_curr_head ? _head : NULL, 0,
UPDATE_REFS_DIE_ON_ERR);
else if (curr_branch)
-   delete_ref(curr_branch, NULL, REF_NODEREF);
+   delete_ref(curr_branch, NULL, REF_NODEREF, NULL);
 
free(curr_branch);
am_destroy(state);
diff --git a/builtin/branch.c b/builtin/branch.c
index 9d30f55b0..44f23208f 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -252,7 +252,7 @@ static int delete_branches(int argc, const char **argv, int 
force, int kinds,
}
 
if (delete_ref(name, is_null_sha1(sha1) ? NULL : sha1,
-  REF_NODEREF)) {
+  REF_NODEREF, NULL)) {
error(remote_branch
  ? _("Error deleting remote-tracking branch '%s'")
  : _("Error deleting branch '%s'"),
diff --git a/builtin/notes.c b/builtin/notes.c
index 5248a9bad..991448d4e 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -681,9 +681,9 @@ static int merge_abort(struct notes_merge_options *o)
 * notes_merge_abort() to remove .git/NOTES_MERGE_WORKTREE.
 */
 
-   if (delete_ref("NOTES_MERGE_PARTIAL", NULL, 0))
+   if (delete_ref("NOTES_MERGE_PARTIAL", NULL, 0, NULL))
ret += error(_("failed to delete ref NOTES_MERGE_PARTIAL"));
-   if (delete_ref("NOTES_MERGE_REF", NULL, REF_NODEREF))
+   if (delete_ref("NOTES_MERGE_REF", NULL, REF_NODEREF, NULL))
ret += error(_("failed to delete ref NOTES_MERGE_REF"));
if (notes_merge_abort(o))
ret += error(_("failed to remove 'git notes merge' worktree"));
diff --git a/builtin/remote.c b/builtin/remote.c
index 5339ed6ad..bfa8a5189 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -691,7 +691,7 @@ static int mv(int argc, const char **argv)
read_ref_full(item->string, RESOLVE_REF_READING, oid.hash, 
);
if (!(flag & REF_ISSYMREF))
continue;
-   if (delete_ref(item->string, NULL, REF_NODEREF))
+   if (delete_ref(item->string, NULL, REF_NODEREF, NULL))
die(_("deleting '%s' failed"), item->string);
}
for (i = 0; i < remote_branches.nr; i++) {
@@ -1248,7 +1248,7 @@ static int set_head(int argc, const char **argv)
head_name = xstrdup(states.heads.items[0].string);
free_remote_ref_states();
} else if (opt_d && !opt_a && argc == 1) {
-   if (delete_ref(buf.buf, NULL, REF_NODEREF))
+   if (delete_ref(buf.buf, NULL, REF_NODEREF, NULL))
result |= error(_("Could not delete %s"), buf.buf);
} else
usage_with_options(builtin_remote_sethead_usage, options);
diff --git a/builtin/replace.c b/builtin/replace.c
index b58c714cb..d32d0a3ae 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -121,7 +121,7 @@ static int for_each_replace_name(const char **argv, 
each_replace_name_fn fn)
 static int delete_replace_ref(const char *name, const char *ref,
  const unsigned char *sha1)
 {
-   if (delete_ref(ref, sha1, 0)

[PATCH 2/3] update-ref: pass reflog message argument to delete_refs

[Kyle Meyer]

Now that delete_refs() accepts a reflog message, pass the
user-provided message to delete_refs() rather than silently dropping
it.  The doesn't matter for the deleted ref's log because the log is
deleted along with the ref, but this entry will show up in HEAD's
reflog when deleting a checked out branch.

Signed-off-by: Kyle Meyer <k...@kyleam.com>
---
I'm not sure if the test here (or in the next patch) is worth including.

 builtin/update-ref.c  | 2 +-
 t/t1400-update-ref.sh | 9 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index a41f9adf1..f642acc22 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -435,7 +435,7 @@ int cmd_update_ref(int argc, const char **argv, const char 
*prefix)
 */
return delete_ref(refname,
  (oldval && !is_null_sha1(oldsha1)) ? oldsha1 
: NULL,
- flags, NULL);
+ flags, msg);
else
return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
  flags | create_reflog_flag,
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index b0ffc0b57..65918d984 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -85,6 +85,15 @@ test_expect_success "delete $m (by HEAD)" '
 '
 rm -f .git/$m
 
+test_expect_success "deleting current branch adds message to HEAD's log" '
+   git update-ref $m $A &&
+   git symbolic-ref HEAD $m &&
+   git update-ref -mdelmsg -d $m &&
+   ! test -f .git/$m &&
+   grep "delmsg$" .git/logs/HEAD >/dev/null
+'
+rm -f .git/$m
+
 test_expect_success 'update-ref does not create reflogs by default' '
test_when_finished "git update-ref -d $outside" &&
git update-ref $outside $A &&
-- 
2.11.1

[PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log

[Kyle Meyer]

When the current branch is renamed, the deletion of the old ref is
recorded in HEAD's log with an empty message.  Now that delete_refs()
accepts a reflog message, provide a more descriptive message.  This
message will not be included in the reflog of the renamed branch, but
its log already covers the renaming event with a message of "Branch:
renamed ...".

Signed-off-by: Kyle Meyer <k...@kyleam.com>
---
 refs/files-backend.c | 8 +++-
 t/t3200-branch.sh| 4 
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index ffa75d816..bb5df7ee6 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2598,6 +2598,7 @@ static int files_rename_ref(struct ref_store *ref_store,
struct stat loginfo;
int log = !lstat(git_path("logs/%s", oldrefname), );
struct strbuf err = STRBUF_INIT;
+   struct strbuf logmsg_del = STRBUF_INIT;
 
if (log && S_ISLNK(loginfo.st_mode))
return error("reflog for %s is a symlink", oldrefname);
@@ -2616,10 +2617,15 @@ static int files_rename_ref(struct ref_store *ref_store,
return error("unable to move logfile logs/%s to 
"TMP_RENAMED_LOG": %s",
oldrefname, strerror(errno));
 
-   if (delete_ref(oldrefname, orig_sha1, REF_NODEREF, NULL)) {
+   strbuf_addf(_del, "Deleted %s", oldrefname);
+
+   if (delete_ref(oldrefname, orig_sha1, REF_NODEREF, logmsg_del.buf)) {
error("unable to delete old %s", oldrefname);
+   strbuf_release(_del);
goto rollback;
}
+   strbuf_release(_del);
+
 
/*
 * Since we are doing a shallow lookup, sha1 is not the
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 8a833f354..4af7cd2b7 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -139,6 +139,10 @@ test_expect_success 'git branch -M baz bam should succeed 
when baz is checked ou
test $(git rev-parse --abbrev-ref HEAD) = bam
 '
 
+test_expect_success 'git branch -M baz bam should add entry to .git/logs/HEAD' 
'
+   grep "Deleted refs/heads/baz$" .git/logs/HEAD >/dev/null
+'
+
 test_expect_success 'git branch -M baz bam should succeed when baz is checked 
out as linked working tree' '
git checkout master &&
git worktree add -b baz bazdir &&
-- 
2.11.1

[PATCH 0/3] delete_ref(): support reflog messages

[Kyle Meyer]

[Sorry for the slow response.]

Jeff King <p...@peff.net> writes:

> On Mon, Jan 16, 2017 at 06:17:29PM -0500, Kyle Meyer wrote:

[...]

>>$ cat .git/logs/refs/heads/new-master
>>    0... 68730... Kyle Meyer <k...@kyleam.com> 1484607020 -0500   commit 
>> (initial): Add file.txt
>>68730... 68730... Kyle Meyer <k...@kyleam.com> 1484607020 -0500   Branch: 
>> renamed refs/heads/master to refs/heads/new-master
>> 
>>$ cat .git/logs/HEAD
>>0... 68730... Kyle Meyer <k...@kyleam.com> 1484607020 -0500   commit 
>> (initial): Add file.txt
>>68730... 0... Kyle Meyer <k...@kyleam.com> 1484607020 -0500
>> 
>> I expected the second line of .git/logs/HEAD to mirror the second line
>> of .git/logs/refs/heads/new-master.  Are the empty message and null sha1
>> in HEAD's entry intentional?
>
> The null sha1 is correct, I think. The branch we were on went away, and
> we use the null sha1 to indicate "no value" in both the creation and
> deletion cases.

I see, thanks.

> I'd say there are two potential improvements:
>
>   - delete_ref() should take a reflog message argument, in case it
> updates the HEAD reflog (as a bonus, this will future-proof us for a
> day when we might keep reflogs for deleted refs).

I've tried to do this in the following patches.

>   - "git branch -m" does seem to realize when we are renaming HEAD,
> because it updates HEAD to point to the new branch name. But it
> should probably insert another reflog entry mentioning the rename
> (we do for "git checkout foo", even when "foo" has the same sha1 as
> the current HEAD).

I haven't worked out how to do this part yet.  I'm guessing the change
will involve modifying split_head_update().

If this is added, should it be instead of, rather than in addition to,
the deletion entry?  If a "Branch: renamed ..." entry is present, it
doesn't seem like the deletion entry is providing any additional
information.

  delete_refs(): accept a reflog message argument
  update-ref: pass reflog message argument to delete_refs
  rename_ref: replace empty deletion message in HEAD's log

 builtin/am.c   |  4 ++--
 builtin/branch.c   |  2 +-
 builtin/notes.c|  4 ++--
 builtin/remote.c   |  4 ++--
 builtin/replace.c  |  2 +-
 builtin/reset.c|  2 +-
 builtin/symbolic-ref.c |  2 +-
 builtin/tag.c  |  2 +-
 builtin/update-ref.c   |  2 +-
 fast-import.c  |  2 +-
 refs.c |  4 ++--
 refs.h |  2 +-
 refs/files-backend.c   | 12 +---
 t/t1400-update-ref.sh  |  9 +
 t/t3200-branch.sh  |  4 
 transport.c|  2 +-
 16 files changed, 39 insertions(+), 20 deletions(-)

-- 
Kyle

Re: init --separate-git-dir does not set core.worktree

[Kyle Meyer]

Duy Nguyen <pclo...@gmail.com> writes:

> On Thu, Feb 2, 2017 at 7:37 PM, Kyle Meyer <k...@kyleam.com> wrote:

[...]

>>  * COMMIT_EDITMSG in .git/modules//: set working tree to the
>>output of "git rev-parse --show-toplevel"
>>
>>  * COMMIT_EDITMSG in .git/worktrees//: set working tree to the
>>path in .git/worktrees//gitdir, minus the trailing "/.git"
>>
>>  * COMMIT_EDITMSG in .git: set working tree to the parent directory
>>
>> This fails for a repo set up with --separate-git-dir [*2*], where the
>> last step will go out into an unrelated repo.  If core.worktree was set
>> and "git rev-parse --show-toplevel" returned the working tree like it
>> did for submodules, things would work.
>
> OK. If I read this right, given a path of any text file somewhere
> within ".git" directory. you are tasked to find out where the
> associated worktree is?

Right

> I.e. this is not an emacsclient executed as part of "git commit",
> correct?

... but it is from a "git commit" call.  I think you're asking because,
if we know where "git commit" was called from, we know what the working
tree is.  This is true, but with the current Magit design, the function
for determining the top-level of the working tree, magit-toplevel,
doesn't have access to this information.  From Emacs, Magit calls "git
commit", setting GIT_EDITOR for that process so that git invokes the
current Emacs instance for editing the commit message.  From
COMMIT_EDITMSG, we want the magit-toplevel to be able to determine the
working tree location so that commands can use magit-toplevel to set
their pwd.

The only Magit command that currently relies on magit-toplevel to
determine the working tree from ".git/COMMIT_EDITMSG" is a command that
shows a diff with the staged changes in a separate buffer.  (Even though
"git diff --cached" would work from within ".git/", some functionality
in this buffer depends on its pwd being set top-level of the working
tree.)

So, whatever solution we come up with on the Magit end will likely
involve recording where "git commit" was called so that the diff command
can figure out the working tree.

> So you need some sort of back-link to ".git" location. And
> unfortunately there's no such thing for .git file (unless it points to
> .git/worktrees/...). I'm hesitant to set core.worktree unless it's
> absolutely needed since it may have unexpected interaction with
> $GIT_WORK_TREE and others (not sure if it has any interaction with
> submodules too). I think adding a new file "gitdir" would be a safer
> option.
>
> I'm not entirely sure if enforcing "one worktree - one repository" is
> safe though. The first two bullet points are very specific and we can
> assume that, but ".git" files alone can be used for anything. In
> theory you can always create a secondary worktree (that's not managed
> by "git worktree") by setting GIT_WORK_TREE and "git checkout -f"
> somewhere. But I guess those would be temporary and nobody would want
> magic to point back to them.

Right, makes sense.

Unfortunately, magit-toplevel was designed thinking that the
--separate-git-dir / core.worktree behavior in Git 2.8.4 and lower was
intentional [*].  We'll need to rework this on our end.

Thanks for your input and for confirming that "git init
--separate-git-dir" does not set core.worktree by design.

[*] https://github.com/magit/magit/issues/460#issuecomment-36035787.

-- 
Kyle

Re: init --separate-git-dir does not set core.worktree

[Kyle Meyer]

Duy Nguyen <pclo...@gmail.com> writes:

> On Thu, Feb 2, 2017 at 10:55 AM, Kyle Meyer <k...@kyleam.com> wrote:
>>
>> As of 6311cfaf9 (init: do not set unnecessary core.worktree,
>> 2016-09-25), "git init --separate-git-dir" no longer sets core.worktree
>> (test below).  Based on the commit message and the corresponding thread
>> [1], I don't think this change in behavior was intentional, but I wasn't
>> able to understand things well enough to attempt a patch.
>
> I'm missing some context. Why does --separate-git-dir have to set
> core.worktree? What fails for you exactly?

Sorry for not providing enough information.  I haven't run into a
failure.

In Magit, we need to determine the top-level of the working tree from
COMMIT_EDITMSG.  Right now that logic [*1*] looks something like this:

 * COMMIT_EDITMSG in .git/modules//: set working tree to the
   output of "git rev-parse --show-toplevel"

 * COMMIT_EDITMSG in .git/worktrees//: set working tree to the
   path in .git/worktrees//gitdir, minus the trailing "/.git"

 * COMMIT_EDITMSG in .git: set working tree to the parent directory

This fails for a repo set up with --separate-git-dir [*2*], where the
last step will go out into an unrelated repo.  If core.worktree was set
and "git rev-parse --show-toplevel" returned the working tree like it
did for submodules, things would work.

Of course, the issue above isn't a reason that --separate-git-dir should
set core.worktree, but the submodule behavior is why we were wondering
if it should.  And so I was going to ask here whether core.worktree
should be set, but then I confused myself into thinking 6311cfaf9
unintentionally changed this behavior.


[*1*] This is done by magit-toplevel:
  
https://github.com/magit/magit/blob/e34f4e8eb00f292e8c475489fa7caa286857a421/lisp/magit-git.el#L400

[*2*] https://github.com/magit/magit/issues/2955
  https://github.com/magit/magit/issues/2981

init --separate-git-dir does not set core.worktree

[Kyle Meyer]

Hello,

As of 6311cfaf9 (init: do not set unnecessary core.worktree,
2016-09-25), "git init --separate-git-dir" no longer sets core.worktree
(test below).  Based on the commit message and the corresponding thread
[1], I don't think this change in behavior was intentional, but I wasn't
able to understand things well enough to attempt a patch.

Thanks.

[1] 
https://public-inbox.org/git/calqjkkzo_y0dncrjjooyz7eso7ybmghvz6fe92oo4su7jec...@mail.gmail.com/T/#u

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index b8fc588b1..444e75865 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -309,6 +309,7 @@ test_expect_success 'init with separate gitdir' '
git init --separate-git-dir realgitdir newdir &&
echo "gitdir: $(pwd)/realgitdir" >expected &&
test_cmp expected newdir/.git &&
+   check_config realgitdir false "$(pwd)/newdir" &&
test_path_is_dir realgitdir/refs
 '
 

HEAD's reflog entry for a renamed branch

[Kyle Meyer]

Hello,

I noticed that, after renaming the current branch, the corresponding
message in .git/logs/HEAD is empty.

For example, running

$ mkdir test-repo
$ cd test-repo
$ git init
$ echo abc >file.txt
$ git add file.txt
$ git commit -m"Add file.txt"
$ git branch -m master new-master

resulted in the following reflogs:

   $ cat .git/logs/refs/heads/new-master
   0... 68730... Kyle Meyer <k...@kyleam.com> 1484607020 -0500  commit 
(initial): Add file.txt
   68730... 68730... Kyle Meyer <k...@kyleam.com> 1484607020 -0500  Branch: 
renamed refs/heads/master to refs/heads/new-master

   $ cat .git/logs/HEAD
   0... 68730... Kyle Meyer <k...@kyleam.com> 1484607020 -0500  commit 
(initial): Add file.txt
   68730... 0... Kyle Meyer <k...@kyleam.com> 1484607020 -0500

I expected the second line of .git/logs/HEAD to mirror the second line
of .git/logs/refs/heads/new-master.  Are the empty message and null sha1
in HEAD's entry intentional?

--
Kyle

Re: [PATCH] branch_get_push: do not segfault when HEAD is detached

[Kyle Meyer]

Jeff King <p...@peff.net> writes:

> On Fri, Jan 06, 2017 at 08:09:32PM -0500, Kyle Meyer wrote:
>
>> > $ git grep -c HEAD^{} junio/pu -- t/
>> > junio/pu:t/t3200-branch.sh:3
>> >
>> > Maybe use HEAD^0 just for consistency?
>>
>> Yes, thanks for pointing that out.
>
> The other option is just "git checkout --detach", which is also used in
> the test suite. I tend to prefer it because it's a little more obvious
> to a reader.

True, that does seem clearer.  Seems I should've waited a bit before
sending out v2.

--
Kyle

[PATCH v2] branch_get_push: do not segfault when HEAD is detached

[Kyle Meyer]

Move the detached HEAD check from branch_get_push_1() to
branch_get_push() to avoid setting branch->push_tracking_ref when
branch is NULL.

Signed-off-by: Kyle Meyer <k...@kyleam.com>
---
 remote.c  | 6 +++---
 t/t1514-rev-parse-push.sh | 6 ++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/remote.c b/remote.c
index ad6c5424e..d5eaec737 100644
--- a/remote.c
+++ b/remote.c
@@ -1716,9 +1716,6 @@ static const char *branch_get_push_1(struct branch 
*branch, struct strbuf *err)
 {
struct remote *remote;
 
-   if (!branch)
-   return error_buf(err, _("HEAD does not point to a branch"));
-
remote = remote_get(pushremote_for_branch(branch, NULL));
if (!remote)
return error_buf(err,
@@ -1778,6 +1775,9 @@ static const char *branch_get_push_1(struct branch 
*branch, struct strbuf *err)
 
 const char *branch_get_push(struct branch *branch, struct strbuf *err)
 {
+   if (!branch)
+   return error_buf(err, _("HEAD does not point to a branch"));
+
if (!branch->push_tracking_ref)
branch->push_tracking_ref = branch_get_push_1(branch, err);
return branch->push_tracking_ref;
diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh
index 7214f5b33..623a32aa6 100755
--- a/t/t1514-rev-parse-push.sh
+++ b/t/t1514-rev-parse-push.sh
@@ -60,4 +60,10 @@ test_expect_success '@{push} with push refspecs' '
resolve topic@{push} refs/remotes/origin/magic/topic
 '
 
+test_expect_success 'resolving @{push} fails with a detached HEAD' '
+   git checkout HEAD^0 &&
+   test_when_finished "git checkout -" &&
+   test_must_fail git rev-parse @{push}
+'
+
 test_done
-- 
2.11.0

Re: [PATCH] branch_get_push: do not segfault when HEAD is detached

[Kyle Meyer]

Johannes Schindelin <johannes.schinde...@gmx.de> writes:

[...]

>> +test_expect_success 'resolving @{push} fails with a detached HEAD' '
>> +git checkout HEAD^{} &&
>
> I seem to recall that we prefer HEAD^0 over HEAD^{} and the existing
> scripts seem to agree with me:
>
> $ git grep -c HEAD^0 junio/pu -- t/
> junio/pu:t/t1450-fsck.sh:1
> junio/pu:t/t1507-rev-parse-upstream.sh:2
> junio/pu:t/t2020-checkout-detach.sh:5
> junio/pu:t/t3200-branch.sh:1
> junio/pu:t/t3203-branch-output.sh:3
> junio/pu:t/t3400-rebase.sh:1
> junio/pu:t/t3404-rebase-interactive.sh:1
> junio/pu:t/t5407-post-rewrite-hook.sh:2
> junio/pu:t/t5505-remote.sh:1
> junio/pu:t/t5510-fetch.sh:1
> junio/pu:t/t5533-push-cas.sh:3
> junio/pu:t/t6035-merge-dir-to-symlink.sh:3
> junio/pu:t/t7201-co.sh:2
> junio/pu:t/t7402-submodule-rebase.sh:1
> junio/pu:t/t9105-git-svn-commit-diff.sh:1
> junio/pu:t/t9107-git-svn-migrate.sh:1
>
> $ git grep -c HEAD^{} junio/pu -- t/
> junio/pu:t/t3200-branch.sh:3
>
> Maybe use HEAD^0 just for consistency?

Yes, thanks for pointing that out.

--
Kyle

[PATCH] branch_get_push: do not segfault when HEAD is detached

[Kyle Meyer]

Move the detached HEAD check from branch_get_push_1() to
branch_get_push() to avoid setting branch->push_tracking_ref when
branch is NULL.

Signed-off-by: Kyle Meyer <k...@kyleam.com>
---
 remote.c  | 6 +++---
 t/t1514-rev-parse-push.sh | 6 ++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/remote.c b/remote.c
index ad6c5424e..d5eaec737 100644
--- a/remote.c
+++ b/remote.c
@@ -1716,9 +1716,6 @@ static const char *branch_get_push_1(struct branch 
*branch, struct strbuf *err)
 {
struct remote *remote;
 
-   if (!branch)
-   return error_buf(err, _("HEAD does not point to a branch"));
-
remote = remote_get(pushremote_for_branch(branch, NULL));
if (!remote)
return error_buf(err,
@@ -1778,6 +1775,9 @@ static const char *branch_get_push_1(struct branch 
*branch, struct strbuf *err)
 
 const char *branch_get_push(struct branch *branch, struct strbuf *err)
 {
+   if (!branch)
+   return error_buf(err, _("HEAD does not point to a branch"));
+
if (!branch->push_tracking_ref)
branch->push_tracking_ref = branch_get_push_1(branch, err);
return branch->push_tracking_ref;
diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh
index 7214f5b33..90c639ae1 100755
--- a/t/t1514-rev-parse-push.sh
+++ b/t/t1514-rev-parse-push.sh
@@ -60,4 +60,10 @@ test_expect_success '@{push} with push refspecs' '
resolve topic@{push} refs/remotes/origin/magic/topic
 '
 
+test_expect_success 'resolving @{push} fails with a detached HEAD' '
+   git checkout HEAD^{} &&
+   test_when_finished "git checkout -" &&
+   test_must_fail git rev-parse @{push}
+'
+
 test_done
-- 
2.11.0

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Kyle J. McKay]

On Dec 21, 2016, at 18:21, Kyle J. McKay wrote:


On Dec 21, 2016, at 07:55, Jeff King wrote:


On Tue, Dec 20, 2016 at 09:54:15PM -0800, Kyle J. McKay wrote:

I wasn't aware anybody actually built with NDEBUG at all. You'd  
have to

explicitly ask for it via CFLAGS, so I assume most people don't.


Not a good assumption.  You know what happens when you assume[1],  
right? ;)


Kind of. If it's a configuration that nobody[1] in the Git  
development
community intended to support or test, then isn't the person  
triggering

it the one making assumptions?


No, I don't think so.  NDEBUG is very clearly specified in POSIX [1].

If NDEBUG is defined then "assert(...)" disappears (and in a nice  
way so as not to precipitate "unused variable" warnings).  "N" being  
"No" or "Not" or "Negated" or "bar over the top" + "DEBUG" meaning  
Not DEBUG.  So the code that goes away when NDEBUG is defined is  
clearly debug code.


I think there is a useful distinction here that I make that's worth  
sharing.  Perhaps it's splitting hairs, but I categorize this "extra"  
code that we've been discussing ("assert(...)" or "if (!...) die(...)"  
or "verify(...)" into two groups:



1) DEBUG code

This is code that developers use when creating new features.  Or  
helpful code that's needed when stepping through a program with the  
debugger to debug a problem.  Or even code that's only used by some  
kind of external "test".  It may be expensive, it may do things that  
should never be done in a build for wider consumption (such as write  
information to special log files, write special syslog messages  
etc.).  Often this code is used in combination with a "-g" debug  
symbols build and possibly even a "-O0" or "-O1" option.


Code like this has no place in a release executable meant for general  
use by an end user.


2) DIAGNOSTIC code

This is near zero overhead code that is intended to be left in a  
release build meant for general use and normally sits there not doing  
anything and NOT leaching any performance out of the build either.   
Its sole purpose in life is to provide a trail of "bread crumbs" if  
the executable goes ***BOOM***.  These "bread crumbs" should be just  
enough when combined with feedback from the unfortunate user who  
experienced the meltdown to re-create the issue in a real DEBUG build  
and find and fix the problem.



It seems to me what you are saying is that Git's "assert" calls are  
DIAGNOSTIC and therefore belong in a release build -- well, except for  
the nedmalloc "assert" calls which do not.


What I'm saying is if they are diagnostic and not debug (and I'm not  
arguing one way or the other, but you've already indicated they are  
near zero overhead which suggests they are indeed diagnostic in  
nature), then they do not belong inside an "assert" which can be  
disabled with "NDEBUG".  I'm arguing that "assert" is not intended for  
diagnostic code, but only debug code as used by nedmalloc.  Having Git  
treat "NDEBUG" one way -- "no, no, do NOT define NDEBUG because that  
disables Git diagnostics and I promise you there's no performance  
penalty" -- versus nedmalloc -- "yes, yes please DO define NDEBUG  
unless you really need our slow debugging code to be present for  
debugging purposes" -- just creates needless unnecessary confusion.


--Kyle

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Kyle J. McKay]

On Dec 21, 2016, at 07:55, Jeff King wrote:


On Tue, Dec 20, 2016 at 09:54:15PM -0800, Kyle J. McKay wrote:

I wasn't aware anybody actually built with NDEBUG at all. You'd  
have to

explicitly ask for it via CFLAGS, so I assume most people don't.


Not a good assumption.  You know what happens when you assume[1],  
right? ;)


Kind of. If it's a configuration that nobody[1] in the Git development
community intended to support or test, then isn't the person  
triggering

it the one making assumptions?


No, I don't think so.  NDEBUG is very clearly specified in POSIX [1].

If NDEBUG is defined then "assert(...)" disappears (and in a nice way  
so as not to precipitate "unused variable" warnings).  "N" being "No"  
or "Not" or "Negated" or "bar over the top" + "DEBUG" meaning Not  
DEBUG.  So the code that goes away when NDEBUG is defined is clearly  
debug code.


Considering the wide deployment and use of Git at this point I think  
rather the opposite to be true that "Git does Not require DEBUGging  
code to be enabled for everyday use."  The alternative that it does  
suggests it's not ready for prime time and quite clearly that's not  
the case.


I've been defining NDEBUG whenever I make a release build for quite  
some
time (not just for Git) in order to squeeze every last possible  
drop of

performance out of it.


I think here you are getting into superstition. Is there any single
assert() in Git that will actually have an impact on performance?


You have suggested there is and that Git is enabling NDEBUG for  
exactly that reason -- to increase performance:



On Dec 20, 2016, at 08:45, Jeff King wrote:

I do notice that we set NDEBUG for nedmalloc, though if I am  
reading the

Makefile right, it is just for compiling those files. It looks like
there are a ton of asserts there that _are_ potentially expensive



Perhaps Git should provide a "verify" macro.  Works like "assert"  
except
that it doesn't go away when NDEBUG is defined.  Being Git-provided  
it could
also use Git's die function.  Then Git could do a global replace of  
assert

with verify and institute a no-assert policy.


What would be the advantage of that over `if(...) die("BUG: ...")`? It
does not require you to write a reason in the die(), but I am not sure
that is a good thing.


You have stated that you believe the current "assert" calls in Git  
(excluding nedmalloc) should not magically disappear when NDEBUG is  
defined.  So precluding a more labor intensive approach where all  
currently existing "assert(...)" calls are replaced with an "if (!...)  
die(...)" combination, providing a "verify" macro is a quick way to  
make that happen.  Consider this, was the value that Jonathan provided  
for the "die" string immediately obvious to you?  It sure wasn't to  
me.  That means that whoever does the "assert(...)" -> "if(!...)die"  
swap out may need to be intimately familiar with that particular piece  
of code or the result will be no better than using a "verify" macro.


I'm just trying to find a quick and easy way to accommodate your  
wishes without redefining the semantics of NDEBUG. ;)


--Kyle

[1] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/assert.h.html

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Kyle J. McKay]

On Dec 20, 2016, at 08:45, Jeff King wrote:


On Tue, Dec 20, 2016 at 03:12:35PM +0100, Johannes Schindelin wrote:


On Dec 19, 2016, at 09:45, Johannes Schindelin wrote:

ACK. I noticed this problem (and fixed it independently as a part  
of a
huge patch series I did not get around to submit yet) while  
trying to

get Git to build correctly with Visual C.


Does this mean that Dscho and I are the only ones who add -DNDEBUG  
for
release builds?  Or are we just the only ones who actually run the  
test

suite on such builds?


It seems you and I are for the moment the only ones bothering with  
running

the test suite on release builds.


I wasn't aware anybody actually built with NDEBUG at all. You'd have  
to

explicitly ask for it via CFLAGS, so I assume most people don't.


Not a good assumption.  You know what happens when you assume[1],  
right? ;)


I've been defining NDEBUG whenever I make a release build for quite  
some time (not just for Git) in order to squeeze every last possible  
drop of performance out of it.


Certainly I never have when deploying to GitHub's cluster (let alone  
my

personal use), and I note that the Debian package also does not.


Yeah, I don't do it for my personal use because those are often not  
based on a release tag so I want to see any assertion failures that  
might happen and they're also not performance critical either.



So from my perspective it is not so much "do not bother with release
builds" as "are release builds even a thing for git?"


They should be if you're deploying Git in a performance critical  
environment.



One of the
reasons I suggested switching the assert() to a die("BUG") is that the
latter cannot be disabled. We generally seem to prefer those to  
assert()

in our code-base (though there is certainly a mix). If the assertions
are not expensive to compute, I think it is better to keep them in for
all builds. I'd much rather get a report from a user that says "I hit
this BUG" than "git segfaulted and I have no idea where" (of course I
prefer a backtrace even more, but that's not always an option).


Perhaps Git should provide a "verify" macro.  Works like "assert"  
except that it doesn't go away when NDEBUG is defined.  Being Git- 
provided it could also use Git's die function.  Then Git could do a  
global replace of assert with verify and institute a no-assert policy.


I do notice that we set NDEBUG for nedmalloc, though if I am reading  
the

Makefile right, it is just for compiling those files. It looks like
there are a ton of asserts there that _are_ potentially expensive, so
that makes sense.


So there's no way to get a non-release build of nedmalloc inside Git  
then without hacking the Makefile?  What if you need those assertions  
enabled?  Maybe NDEBUG shouldn't be defined by default for any files.


--Kyle

[1] https://www.youtube.com/watch?v=KEP1acj29-Y

[PATCH v3] mailinfo.c: move side-effects outside of assert

[Kyle J. McKay]

On Dec 19, 2016, at 15:26, Junio C Hamano wrote:

> "Kyle J. McKay" <mack...@gmail.com> writes:
>
>>> OK.  So we do not expect it to fail, but we still do want the side
>>> effect of that function (i.e. accmulation into the field).
>>>
>>> Somebody care to send a final "agreed-upon" version?
>>
>> Yup, here it is:
>
> Thanks.

Whoops. there's an extra paragraph in the commit description that I
meant to remove and, of course, I didn't notice it until I sent the
copy to the list.  :(

I don't think a "fixup" or "squash" can replace a description, right?

So here's a replacement patch with the correct description with the
deleted paragrah:

-- >8 --

Since 6b4b013f18 (mailinfo: handle in-body header continuations,
2016-09-20, v2.11.0) mailinfo.c has contained new code with an
assert of the form:

assert(call_a_function(...))

The function in question, check_header, has side effects.  This
means that when NDEBUG is defined during a release build the
function call is omitted entirely, the side effects do not
take place and tests (fortunately) start failing.

Since the only time that mi->inbody_header_accum is appended to is
in check_inbody_header, and appending onto a blank
mi->inbody_header_accum always happens when is_inbody_header is
true, this guarantees a prefix that causes check_header to always
return true.

Therefore replace the assert with an if !check_header + DIE
combination to reflect this.

Helped-by: Jonathan Tan <jonathanta...@google.com>
Helped-by: Jeff King <p...@peff.net>
Acked-by: Johannes Schindelin <johannes.schinde...@gmx.de>
Signed-off-by: Kyle J. McKay <mack...@gmail.com>
---

Notes:
Please include this PATCH in 2.11.x maint

 mailinfo.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mailinfo.c b/mailinfo.c
index 2fb3877e..a489d9d0 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -710,7 +710,8 @@ static void flush_inbody_header_accum(struct mailinfo *mi)
 {
if (!mi->inbody_header_accum.len)
return;
-   assert(check_header(mi, >inbody_header_accum, mi->s_hdr_data, 0));
+   if (!check_header(mi, >inbody_header_accum, mi->s_hdr_data, 0))
+   die("BUG: inbody_header_accum, if not empty, must always 
contain a valid in-body header");
strbuf_reset(>inbody_header_accum);
 }
 
---

[PATCH v2] mailinfo.c: move side-effects outside of assert

[Kyle J. McKay]

On Dec 19, 2016, at 13:01, Junio C Hamano wrote:

> Jonathan Tan <jonathanta...@google.com> writes:
>
>>>> This is obviously an improvement, but it makes me wonder if we  
>>>> should be
>>>> doing:
>>>>
>>>> if (!check_header(mi, >inbody_header_accum, mi->s_hdr_data))
>>>>die("BUG: some explanation of why this can never happen");
>>>>
>>>> which perhaps documents the intended assumptions more clearly. A  
>>>> comment
>>>> regarding the side effects might also be helpful.
>>>
>>> I wondered exactly the same thing myself.  I was hoping Jonathan  
>>> would
>>> pipe in here with some analysis about whether this is:
>>>
>>>  a) a super paranoid, just-in-case, can't really ever fail because  
>>> by
>>> the time we get to this code we've already effectively validated
>>> everything that could cause check_header to return false in this  
>>> case
>>> ...
>> The answer is "a". The only time that mi->inbody_header_accum is
>> appended to is in check_inbody_header, and appending onto a blank
>> mi->inbody_header_accum always happens when is_inbody_header is true
>> (which guarantees a prefix that causes check_header to always return
>> true).
>>
>> Peff's suggestion sounds reasonable to me, maybe with an error  
>> message
>> like "BUG: inbody_header_accum, if not empty, must always contain a
>> valid in-body header".
>
> OK.  So we do not expect it to fail, but we still do want the side
> effect of that function (i.e. accmulation into the field).
>
> Somebody care to send a final "agreed-upon" version?

Yup, here it is:

-- 8< --

Since 6b4b013f18 (mailinfo: handle in-body header continuations,
2016-09-20, v2.11.0) mailinfo.c has contained new code with an
assert of the form:

assert(call_a_function(...))

The function in question, check_header, has side effects.  This
means that when NDEBUG is defined during a release build the
function call is omitted entirely, the side effects do not
take place and tests (fortunately) start failing.

Move the function call outside of the assert and assert on
the result of the function call instead so that the code
still works properly in a release build and passes the tests.

Since the only time that mi->inbody_header_accum is appended to is
in check_inbody_header, and appending onto a blank
mi->inbody_header_accum always happens when is_inbody_header is
true, this guarantees a prefix that causes check_header to always
return true.

Therefore replace the assert with an if !check_header + DIE
combination to reflect this.

Helped-by: Jonathan Tan <jonathanta...@google.com>
Helped-by: Jeff King <p...@peff.net>
Acked-by: Johannes Schindelin <johannes.schinde...@gmx.de>
Signed-off-by: Kyle J. McKay <mack...@gmail.com>
---

Notes:
Please include this PATCH in 2.11.x maint

 mailinfo.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mailinfo.c b/mailinfo.c
index 2fb3877e..a489d9d0 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -710,7 +710,8 @@ static void flush_inbody_header_accum(struct mailinfo *mi)
 {
if (!mi->inbody_header_accum.len)
return;
-   assert(check_header(mi, >inbody_header_accum, mi->s_hdr_data, 0));
+   if (!check_header(mi, >inbody_header_accum, mi->s_hdr_data, 0))
+   die("BUG: inbody_header_accum, if not empty, must always 
contain a valid in-body header");
strbuf_reset(>inbody_header_accum);
 }
 
---

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Kyle J. McKay]

On Dec 19, 2016, at 12:03, Jeff King wrote:


On Sat, Dec 17, 2016 at 11:54:18AM -0800, Kyle J. McKay wrote:


Since 6b4b013f18 (mailinfo: handle in-body header continuations,
2016-09-20, v2.11.0) mailinfo.c has contained new code with an
assert of the form:

assert(call_a_function(...))

The function in question, check_header, has side effects.  This
means that when NDEBUG is defined during a release build the
function call is omitted entirely, the side effects do not
take place and tests (fortunately) start failing.

Move the function call outside of the assert and assert on
the result of the function call instead so that the code
still works properly in a release build and passes the tests.

Signed-off-by: Kyle J. McKay <mack...@gmail.com>
---

Notes:
   Please include this PATCH in 2.11.x maint


This is obviously an improvement, but it makes me wonder if we  
should be

doing:

 if (!check_header(mi, >inbody_header_accum, mi->s_hdr_data))
die("BUG: some explanation of why this can never happen");

which perhaps documents the intended assumptions more clearly. A  
comment

regarding the side effects might also be helpful.


I wondered exactly the same thing myself.  I was hoping Jonathan would  
pipe in here with some analysis about whether this is:


  a) a super paranoid, just-in-case, can't really ever fail because  
by the time we get to this code we've already effectively validated  
everything that could cause check_header to return false in this case


-or-

  b) Yeah, it could fail in the real world and it should "die" (and  
probably have a test added that triggers such death)


-or-

  c) Actually, if check_header does return false we can keep going  
without problem


-or-

  d) Actually, if check_header does return false we can keep going by  
making a minor change that should be in the patch


I assume that since Jonathan added the code he will just know the  
answer as to which one it is and I won't have to rely on the results  
of my imaginary analysis.  ;)


On Dec 19, 2016, at 09:45, Johannes Schindelin wrote:


ACK. I noticed this problem (and fixed it independently as a part of a
huge patch series I did not get around to submit yet) while trying  
to get

Git to build correctly with Visual C.


Does this mean that Dscho and I are the only ones who add -DNDEBUG for  
release builds?  Or are we just the only ones who actually run the  
test suite on such builds?


--Kyle

[PATCH] mailinfo.c: move side-effects outside of assert

[Kyle J. McKay]

Since 6b4b013f18 (mailinfo: handle in-body header continuations,
2016-09-20, v2.11.0) mailinfo.c has contained new code with an
assert of the form:

assert(call_a_function(...))

The function in question, check_header, has side effects.  This
means that when NDEBUG is defined during a release build the
function call is omitted entirely, the side effects do not
take place and tests (fortunately) start failing.

Move the function call outside of the assert and assert on
the result of the function call instead so that the code
still works properly in a release build and passes the tests.

Signed-off-by: Kyle J. McKay <mack...@gmail.com>
---

Notes:
Please include this PATCH in 2.11.x maint

 mailinfo.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/mailinfo.c b/mailinfo.c
index 2fb3877e..47442fb5 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -708,9 +708,12 @@ static int is_scissors_line(const char *line)
 
 static void flush_inbody_header_accum(struct mailinfo *mi)
 {
+   int okay;
+
if (!mi->inbody_header_accum.len)
return;
-   assert(check_header(mi, >inbody_header_accum, mi->s_hdr_data, 0));
+   okay = check_header(mi, >inbody_header_accum, mi->s_hdr_data, 0);
+   assert(okay);
strbuf_reset(>inbody_header_accum);
 }
 
---

Git 2.11.0 on OS X El Capitan 10.11.6

[Kyle Flesness]

Hello! Thanks for taking the time to read this bug report, I am operating a mac 
book pro with OS X El Capitan 10.11.6 and attempting to download git 2.10.1, 
the installation appears to finalize with no problems but Git is not at the 
download destination from the designated path. Will 2.11.0 be compatible with 
OS X El Capitan 10.11.6? Any other solutions you might recommend?

Thank you for your time and hard work!!

Cheers,

Kyle

Re: Git v2.11.0 breaks max depth nested alternates

[Kyle J. McKay]

On Dec 3, 2016, at 20:55, Jeff King wrote:


So I do think this is worth dealing with, but I'm also curious why
you're hitting the depth-5 limit. I'm guessing it has to do with  
hosting

a hierarchy of related repos. But is your system then always in danger
of busting the 5-limit if people create too deep a repository  
hierarchy?


No we check for the limit.  Anything at the limit gets broken by the  
quarantine change though.


Specifically, I'm wondering if it would be sufficient to just bump  
it to

6. Or 100.


Well, if we left the current limit in place, but as you say:


Of course any static bump runs into the funny case where a repo
_usually_ works, but fails when pushed to. Which is kind of nasty and
unintuitive. And your patch fixes that,


Yes.  That's not nice, hence the patch.  Without the fix, pushing  
might work sometimes until you actually need to access cut-off objects  
at pre-receive time.  So you might be able to push sometimes and  
sometimes it breaks.



and we can leave the idea of
bumping the static depth number as an orthogonal issue (that  
personally,

I do not care about much about either way).


The patch is a step on that road.  It doesn't go that far but all it  
would take is connecting the introduced variable to a config item.   
But you still need to bump it by 1 during quarantine operations.  Such  
support would even allow alternates to be disallowed (except during  
quarantine).  I wonder if there's an opportunity for further pack  
operation optimizations in such a case (you know there are no  
alternates because they're not allowed)?



diff --git a/common-main.c b/common-main.c
index c654f955..9f747491 100644
--- a/common-main.c
+++ b/common-main.c
@@ -37,5 +37,8 @@ int main(int argc, const char **argv)

restore_sigpipe_to_default();

+   if (getenv(GIT_QUARANTINE_ENVIRONMENT))
+   alt_odb_max_depth++;
+
return cmd_main(argc, argv);


After reading your problem description, my initial thought was to
increment the counter when we allocate the tmp-objdir, and decrement
when it is destroyed. Because the parent receive-pack process adds  
it to

its alternates, too. But:

 1. Receive-pack doesn't care; it adds the tmp-objdir as an alternate,
rather than adding it as its main object dir and bumping down the
main one.

 2. There would have to be some way of communicating to sub-processes
that they should bump their max-depth by one.


All true.  And I had similar thoughts.  Perhaps we should add your  
comments to the patch description?  There seems to be a trend towards  
having longer patch descriptions these days... ;)



You've basically used the quarantine-path variable as the
inter-process flag for (2). Which feels a little funny, because its
value is unrelated to the alt-odb setup. But it is a reliable  
signal, so

there's a certain elegance. It's probably the best option, given that
the alternative is a specific variable to say "hey, bump your
max-alt-odb-depth by one". That's pretty ugly, too. :)


You took the words right out of my mouth...   I guess I need to work  
on doing a better job of dumping my stream-of-thoughts that go into a  
patch into the emails to the list.


Most all of your comments could be dumped into the patch description  
as-is to pimp it out some.  I have no objection to that, even adding  
an "Additional-analysis-by:" (or similar) credit line too.  :)


--Kyle

Git v2.11.0 breaks max depth nested alternates

[Kyle J. McKay]

The recent addition of pre-receive quarantining breaks nested  
alternates that are already at the maximum alternates nesting depth.

In the file sha1_file.c in the function link_alt_odb_entries we have  
this:

 > if (depth > 5) {
 > error("%s: ignoring alternate object stores, nesting too deep.",
 > relative_base);
 > return;
 > }

When the incoming quarantine takes place the current objects directory  
is demoted to an alternate thereby increasing its depth (and any  
alternates it references) by one and causing any object store that was  
previously at the maximum nesting depth to be ignored courtesy of the  
above hard-coded maximum depth.

If the incoming push happens to need access to some of those objects  
to perhaps "--fix-thin" its pack it will crash and burn.

Originally I was not going to include a patch to fix this, but simply  
suggest that the expeditious fix is to just allow one additional  
alternates nesting depth level during quarantine operations.

However, it was so simple, I have included the patch below :)

I have verified that where a push with Git v2.10.2 succeeds and a push  
with Git v2.11.0 to the same repository fails because of this problem  
that the below patch does indeed correct the issue and allow the push  
to succeed.

Cheers,

Kyle

-- 8< --
Subject: [PATCH] receive-pack: increase max alternates depth during quarantine

Ever since 722ff7f876 (receive-pack: quarantine objects until
pre-receive accepts, 2016-10-03, v2.11.0), Git has been quarantining
objects and packs received during an incoming push into a separate
objects directory and using the alternates mechanism to make them
available until they are either accepted and moved into the main
objects directory or rejected and discarded.

Unfortunately this has the side effect of increasing the alternates
nesting depth level by one for all pre-existing alternates.

If a repository is already at the maximum alternates nesting depth,
then this quarantining operation can temporarily push it over making
the incoming push fail.

To prevent the failure we simply increase the allowed alternates
nesting depth by one whenever a quarantine operation is in effect.

Signed-off-by: Kyle J. McKay <mack...@gmail.com>
---

Notes:
Some alternates nesting depth background:

If base/fork0/fork1/fork2/fork3/fork4/fork5 represents
seven git repositories where base.git has no alternates,
fork0.git has base.git as an alternate, fork1.git has
fork0.git as an alternate and so on where fork5.git has
only fork4.git as an alternate, then fork5.git is at
the maximum allowed depth of 5.  git fsck --strict --full
works without complaint on fork5.git.

However, in base/fork0/fork1/fork2/fork3/fork4/fork5/fork6,
an fsck --strict --full of fork6.git will generate complaints
and any objects/packs present in base.git will be ignored.

 cache.h   | 1 +
 common-main.c | 3 +++
 environment.c | 1 +
 sha1_file.c   | 2 +-
 4 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/cache.h b/cache.h
index a50a61a1..25c17c29 100644
--- a/cache.h
+++ b/cache.h
@@ -676,6 +676,7 @@ extern size_t packed_git_limit;
 extern size_t delta_base_cache_limit;
 extern unsigned long big_file_threshold;
 extern unsigned long pack_size_limit_cfg;
+extern int alt_odb_max_depth;
 
 /*
  * Accessors for the core.sharedrepository config which lazy-load the value
diff --git a/common-main.c b/common-main.c
index c654f955..9f747491 100644
--- a/common-main.c
+++ b/common-main.c
@@ -37,5 +37,8 @@ int main(int argc, const char **argv)
 
restore_sigpipe_to_default();
 
+   if (getenv(GIT_QUARANTINE_ENVIRONMENT))
+   alt_odb_max_depth++;
+
return cmd_main(argc, argv);
 }
diff --git a/environment.c b/environment.c
index 0935ec69..32e11f70 100644
--- a/environment.c
+++ b/environment.c
@@ -64,6 +64,7 @@ int merge_log_config = -1;
 int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
 unsigned long pack_size_limit_cfg;
 enum hide_dotfiles_type hide_dotfiles = HIDE_DOTFILES_DOTGITONLY;
+int alt_odb_max_depth = 5;
 
 #ifndef PROTECT_HFS_DEFAULT
 #define PROTECT_HFS_DEFAULT 0
diff --git a/sha1_file.c b/sha1_file.c
index 9c86d192..15b8432e 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -337,7 +337,7 @@ static void link_alt_odb_entries(const char *alt, int len, 
int sep,
int i;
struct strbuf objdirbuf = STRBUF_INIT;
 
-   if (depth > 5) {
+   if (depth > alt_odb_max_depth) {
error("%s: ignoring alternate object stores, nesting too deep.",
relative_base);
return;
---

[ANNOUNCE] git-log-compact v1.0

[Kyle J. McKay]

> NOTE: If you read the excellent Git Rev News [1], then you
> already know all about git-log-compact :)

The git-log-compact script provides a compact alternative to the
`git log --oneline` output that includes dates, times and author
and/or committer initials in a space efficient output format.

`git-log-compact` is intended to fill the gap between the single line
`--oneline` log output format and the multiline `--pretty=short` /
`--pretty=medium` output formats.

It is not strictly a one line per commit output format (but almost) and
while it only shows the title of each commit (like the short format) it
does include author and date information (similarly to the medium format)
except that timestamps are shown very compactly and author/committer
names are shown as initials.

This allows one to get a complete view of repository activity in a very
compact output format that can show many commits per screen full and is
fully compatible with the `--graph` option.

Simple example output from the Git repository:

git log-compact --graph --date-order --decorate --no-merges -n 5 v2.5.3

=== 2015-09-17 ===
  * ee6ad5f4 12:16 jch (tag: v2.5.3) Git 2.5.3
=== 2015-09-09 ===
  * b9d66899 14:22 js  am --skip/--abort: merge HEAD/ORIG_HEAD tree into index
  |   === 2015-09-04 ===
  | * 27ea6f85 10:46 jch (tag: v2.5.2) Git 2.5.2
  * 74b67638 10:36 jch (tag: v2.4.9) Git 2.4.9
   ..
  * ecad27cf 10:32 jch (tag: v2.3.9) Git 2.3.9

I have been wanting a compact one line output format that included dates,
times and initials for some time that is compatible with --graph, clearly
shows root commits and eliminates confusion over whether or not two adjacent
lines in the output are related as parent/child (the --show-linear-break
option does not work with --graph).

The git-log-compact utility is the result.  Except for --notes, --pretty and
--format options (which would make the output a non-oneline format) any
other `git log` option may be used (including things like --cherry-mark,
--patch, --raw, --stat, --summary, --show-linear-break etc.),

There are a few new options specific to git-log-compact which are described
in the README and the `git-log-compact -h` output that can be used to alter
the dates, times and/or initials displayed.

The project page with detailed help and many screen shots is located at:

  https://mackyle.github.io/git-log-compact/

Alternatively the repository can be cloned from:

  https://github.com/mackyle/git-log-compact.git

Or the script file itself (which is really all you need) can be
viewed/fetched from:

  https://github.com/mackyle/git-log-compact/blob/HEAD/git-log-compact

The git-log-compact script should work with any version of Git released
in the last several years.

--Kyle

[1] https://git.github.io/rev_news/2016/10/19/edition-20/

Re: [PATCH 10/10] get_short_sha1: list ambiguous objects on error

[Kyle J. McKay]

On Sep 29, 2016, at 06:24, Jeff King wrote:

If you are doing "git show 235234" it should pick the tag (if it  
peels to a
committish) because Git has already set a precedent of preferring  
tags over
commits when it disambiguates ref names and otherwise pick the  
commit.


I'm not convinced that picking the tag is actually helpful in this  
case;

I agree with Linus that feeding something to "git show" almost always
wants to choose the commit.


Since "git show" peels tags you end up seeing the commit it refers to  
(assuming it's a committish tag).


I also don't think tag ambiguity in short sha1s is all that  
interesting.


The Linux repository has this:

   901069c:
  901069c71415a76d commit iwlagn: change Copyright to 2011
  901069c5c5b15532 tag(v2.6.38-rc4) Linux 2.6.38-rc4

Since that tag peels to a commit, it seems like it would be incorrect  
to pick the commit over the tag when you're looking for a committish.


Either 901069c should resolve to the tag (which gets peeled to the  
commit) or it should error out with the hint messages.


The Git repository has this:

   c512b03:
  c512b035556eff4d commit Merge branch 'rc/maint-reflog-msg-for- 
forced

  c512b0344196931a tag(v0.99.9a) GIT 0.99.9a

So perhaps it's a little bit more interesting than it first appears.  :)

--Kyle

Re: [PATCH 10/10] get_short_sha1: list ambiguous objects on error

[Kyle J. McKay]

On Sep 26, 2016, at 09:36, Linus Torvalds wrote:


On Mon, Sep 26, 2016 at 5:00 AM, Jeff King <p...@peff.net> wrote:


This patch teaches get_short_sha1() to list the sha1s of the
objects it found, along with a few bits of information that
may help the user decide which one they meant.


This looks very good to me, but I wonder if it couldn't be even more  
aggressive.


In particular, the only hashes that most people ever use in short form
are commit hashes. Those are the ones you'd use in normal human
interactions to point to something happening.

So when the disambiguation notices that there is ambiguity, but there
is only _one_ commit, maybe it should just have an aggressive mode
that says "use that as if it wasn't ambiguous".


If you have this:

faa23ec9b437812ce2fc9a5b3d59418d672debc1 refs/heads/ambig
7f40afe646fa3f8a0f361b6f567d8f7d7a184c10 refs/tags/ambig

and you do this:

$ git rev-parse ambig
warning: refname 'ambig' is ambiguous.
7f40afe646fa3f8a0f361b6f567d8f7d7a184c10

Git automatically prefers the tag over the branch, but it does spit  
out a warning.



And then have an explicit command (or flag) to do disambiguation for
when you explicitly want it.


I think you don't even need that.  Git already does disambiguation for  
ref names, picks one and spits out a warning.


Why not do the same for short hash names when it makes sense?


Rationale: you'd never care about short forms for tags. You'd just use
the tag name. And while blob ID's certainly show up in short form in
diff output (in the "index" line), very few people will use them. And
tree hashes are basically never seen outside of any plumbing commands
and then seldom in shortened form.

So I think it would make sense to default to a mode that just picks
the commit hash if there is only one such hash. Sure, some command
might want a "treeish", but a commit is still more likely than a tree
or a tag.

But regardless, this series looks like a good thing.


I like it too.

But perhaps it makes sense to actually pick one if there's only one  
disambiguation of the type you're looking for.


For example given:

235234a blob
2352347 tag
235234f tree
2352340 commit

If you are doing "git cat-file blob 235234" it should pick the blob  
and spit out a warning (and similarly for other cat-file types).  But  
"git cat-file -p 235234" would give the fatal error with the  
disambiguation hints because it wants type "any".


If you are doing "git show 235234" it should pick the tag (if it peels  
to a committish) because Git has already set a precedent of preferring  
tags over commits when it disambiguates ref names and otherwise pick  
the commit.


Lets consider this approach using the stats for the Linux kernel:


Ambiguous prefix length 7 counts:
  prefixes:   44733
   objects:   89766

Ambiguous length 11 (but not at length 12) info:
  prefixes:   2
  0 (with 1 or more commit disambiguations)

Ambiguous length 10 (but not at length 11) info:
  prefixes:  12
  3 (with 1 or more commit disambiguations)
  0 (with 2 or more commit disambiguations)

Ambiguous length 9 (but not at length 10) info:
  prefixes: 186
 43 (with 1 or more commit disambiguations)
  1 (with 2 or more commit disambiguations)

Ambiguous length 8 (but not at length 9) info:
  prefixes:2723
651 (with 1 or more commit disambiguations)
 40 (with 2 or more commit disambiguations)

Ambiguous length 7 (but not at length 8) info:
  prefixes:   41864
   9842 (with 1 or more commit disambiguations)
680 (with 2 or more commit disambiguations)


Of the 44733 ambiguous length 7 prefixes, only about 10539 of them  
disambiguate into one or more commit objects.


But if we apply the "spit a warning and prefer a commit object if  
there's only one and you're looking for a committish" rule, that drops  
the number from 10539 to about 721.  In other words, only about 7% of  
the previously ambiguous short commit SHA1 prefixes would continue to  
be ambiguous at length 7.  In fact it almost makes a prefix length of  
9 good enough, there's just the one at length 9 that disambiguates  
into more than one commit (45f014c52).


--Kyle

Re: Changing the default for "core.abbrev"?

[Kyle J. McKay]

On Sep 25, 2016, at 18:39, Linus Torvalds wrote:


The kernel, these days, is at roughly 5 million objects, and while the
seven hex digits are still often enough for uniqueness (and git will
always add digits *until* it is unique), it's long been at the point
where I tell people to do

   git config --global core.abbrev 12

because even though git will extend the seven hex digits until the
object name is unique, that only reflects the *current* situation in
the repository. With 5 million objects and a very healthy growth rate,
a 7-8 hex digit number that is unique today is not necessarily unique
a month or two from now, and then it gets annoying when a commit
message has a short git ID that is no longer unique when you go back
and try to figure out what went wrong in that commit.


On Sep 25, 2016, at 20:46, Junio C Hamano wrote:


Linus Torvalds <torva...@linux-foundation.org> writes:


I can just keep reminding kernel maintainers and developers to update
their git config, but maybe it would be a good idea to just admit  
that

the defaults picked in 2005 weren't necessarily the best ones
possible, and those could be bumped up a bit?


I am not quite sure how good any new default would be, though.  Just
like any timeout is not long enough for somebody, growing projects
will eventually hit whatever abbreviation length they start with.


This made me curious what the situation is really like.  So I crunched  
some data.


Using a recent clone of $korg/torvalds/linux:

$ git rev-parse --verify d597639e203
error: short SHA1 d597639e203 is ambiguous.
fatal: Needed a single revision

So the kernel already has 11-character "short" SHA1s that are  
ambiguous.  Is a core.abbrev setting of 12 really good enough?


Here are the stats on the kernel's repository:

Ambiguous length 11 (but not at length 12) info:
  prefixes:   2
  0 (with 1 or more commit disambiguations)

Ambiguous length 10 (but not at length 11) info:
  prefixes:  12
  3 (with 1 or more commit disambiguations)
  0 (with 2 or more commit disambiguations)

Ambiguous length 9 (but not at length 10) info:
  prefixes: 186
 43 (with 1 or more commit disambiguations)
  1 (with 2 or more commit disambiguations)
  0 (with 3 or more disambiguations)

Ambiguous length 8 (but not at length 9) info:
  prefixes:2723
651 (with 1 or more commit disambiguations)
 40 (with 2 or more commit disambiguations)
  1 (with 3 or more disambiguations)
  maxambig:   3 (there is 1 of them)

Ambiguous length 7 (but not at length 8) info:
  prefixes:   41864
   9842 (with 1 or more commit disambiguations)
680 (with 2 or more commit disambiguations)
299 (with 3 or more disambiguations)
  maxambig:   3 (there are 299 of them)

The "maxambig" value is the maximum number of disambiguations for any  
single prefix at that prefix length.  So for prefixes of length 7  
there are 299 that disambiguate into 3 objects.


Just out of curiosity, generating stats on the Git repository gives:

Ambiguous length 8 (but not at length 9) info:
  prefixes:   7
  3 (with 1 or more commit disambiguations)
  2 (with 2 or more commit disambiguations)
  0 (with 3 or more disambiguations)

Ambiguous length 7 (but not at length 8) info:
  prefixes:  87
 36 (with 1 or more commit disambiguations)
  3 (with 2 or more commit disambiguations)
  0 (with 3 or more disambiguations)

Running the stats on $github/gitster/git produces some ambiguous  
length 9 prefixes (one of which contains a commit disambiguation).


--Kyle

Re: [PATCH 10/10] get_short_sha1: list ambiguous objects on error

[Kyle J. McKay]

On Sep 26, 2016, at 05:00, Jeff King wrote:


 $ git rev-parse b2e1
 error: short SHA1 b2e1 is ambiguous
 hint: The candidates are:
 hint:   b2e1196 tag v2.8.0-rc1
 hint:   b2e11d1 tree
 hint:   b2e1632 commit 2007-11-14 - Merge branch 'bs/maint-commit- 
options'

 hint:   b2e1759 blob
 hint:   b2e18954 blob
 hint:   b2e1895c blob
 fatal: ambiguous argument 'b2e1': unknown revision or path not in  
the working tree.

 Use '--' to separate paths from revisions, like this:
 'git  [...] -- [...]'


This hint: information is excellent.  There needs to be a way to show  
it on demand.


$ git rev-parse --disambiguate=b2e1
b2e11962c5e6a9c81aa712c751c83a743fd4f384
b2e11d1bb40c5f81a2f4e37b9f9a60ec7474eeab
b2e163272c01aca4aee4684f5c683ba341c1953d
b2e18954c03ff502053cb74d142faab7d2a8dacb
b2e1895ca92ec2037349d88b945ba64ebf16d62d

Not nearly so helpful, but the operation of --disambiguate cannot be  
changed without breaking current scripts.


Can your excellent "hint:" output above be attached to the -- 
disambiguate option somehow, please.  Something like this perhaps:


$ git rev-parse --disambiguate-list=b2e1
b2e1196 tag v2.8.0-rc1
b2e11d1 tree
b2e1632 commit 2007-11-14 - Merge branch 'bs/maint-commit-options'
b2e1759 blob
b2e18954 blob
b2e1895c blob

Any option name will do, --disambiguate-verbose, --disambiguate- 
extended, --disambiguate-long, --disambiguate-log, --disambiguate- 
help, --disambiguate-show-me-something-useful-to-humans-not-scripts ...


--Kyle

Re: [PATCH/RFC] git log --oneline alternative with dates, times and initials

[Kyle J. McKay]

On Sep 29, 2016, at 01:33, Jeff King wrote:


On Wed, Sep 28, 2016 at 10:34:51PM -0700, Kyle J. McKay wrote:


git log-times --graph --date-order --decorate --no-merges -n 5 v2.5.3

   === 2015-09-17 ===
 * ee6ad5f4 12:16 jch (tag: v2.5.3) Git 2.5.3
   === 2015-09-09 ===
 * b9d66899 14:22 js  am --skip/--abort: merge HEAD/ORIG_HEAD tree  
into index

 |   === 2015-09-04 ===
 | * 27ea6f85 10:46 jch (tag: v2.5.2) Git 2.5.2
 * 74b67638 10:36 jch (tag: v2.4.9) Git 2.4.9
  ..
 * ecad27cf 10:32 jch (tag: v2.3.9) Git 2.3.9


I was surprised to see this as a separate script, but it is true  
that we

cannot quite pull it off with --format. I think we are very close,
though.  With the patches below I think you can do:

 git log \
   --commit-header='%C(auto,bold blue)== %as ==%C(auto,reset)'
   --format='%C(auto)%h %C(auto,green)%ad %C(auto,red)%aS/%cS%C(auto) 
%d%C(auto,reset) %s' \

   --graph --no-merges --author-date-order --date=format:%H:%M

and get the same (or very similar) output.

 [1/5]: pretty: allow formatting DATE_SHORT
 [2/5]: pretty: allow formatting names as initials
 [3/5]: graph: fix extra spaces in graph_padding_line
 [4/5]: graph: helper functions for printing commit header
 [5/5]: log: add --commit-header option

Each of those commits[1] needs some minor polish, and as I'm not  
really

that interested in fancy log output myself, I don't plan on working on
them further. I was mostly curious just how close we were. But if  
you'd

like to pursue it, feel free to use them as a starting point.


Those patches are missing some of the features like showing root  
commits, handling two letter initials, showing the weekday, inserting  
a break where needed to avoid parent-child confusion in graph output  
and properly handling Duy's initials. :)


I suppose if all the objects that output a date took a '('   
')' option that would get you part of the way -- it could replace  
DATE_SHORT with DATE_STRFTIME.


Also the above example doesn't handle marks properly in graph mode.   
Yes, you can add the "%m" format option but it does something odd and  
the script fixes it up.


On the other hand, git-log-times started out as a script for something  
else (a shell script actually) and just got embellished further and  
turned into a perl script for speed.


Your patches are a good first start though but reading the --graph  
code gives me headaches and I figured it would be like going down a  
rabbit hole to make the code support everything the script does.


The script also has one big advantage.  It works with the version of  
Git everybody already has installed.  :)


And nobody is ever going to want to type several lines of arcane  
formatting instructions to get the output.  ;_)


It would need a new option, perhaps --oneline-extended or something.

The patches are a good start but that doesn't help anyone using Git  
today which is why git-log-times is submitted as a contrib script --  
much like the way diff-highlight is still a contrib script and not  
supported directly by Git either.


--Kyle

[PATCH/RFC] git log --oneline alternative with dates, times and initials

[Kyle J. McKay]

Simple example output from the Git repository:

git log-times --graph --date-order --decorate --no-merges -n 5 v2.5.3

=== 2015-09-17 ===
  * ee6ad5f4 12:16 jch (tag: v2.5.3) Git 2.5.3
=== 2015-09-09 ===
  * b9d66899 14:22 js  am --skip/--abort: merge HEAD/ORIG_HEAD tree into index
  |   === 2015-09-04 ===
  | * 27ea6f85 10:46 jch (tag: v2.5.2) Git 2.5.2
  * 74b67638 10:36 jch (tag: v2.4.9) Git 2.4.9
   ..
  * ecad27cf 10:32 jch (tag: v2.3.9) Git 2.3.9

I have been wanting a compact one line output format that included dates,
times and initials for some time that is compatible with --graph, clearly
shows root commits and eliminates confusion over whether or not two adjacent
lines in the output are related as parent/child (the --show-linear-break
option does not work with --graph).

The git-log-times utility is the result.  Except for --notes, --pretty and
--format options (which would make the output a non-oneline format) any
other `git log` option may be used (including things like --cherry-mark,
--patch, --raw, --stat, --summary, --show-linear-break etc.),

There are a few new options specific to git-log-times which are described
in the README and the `git-log-times -h` output that can be used to alter
the dates, times and/or initials displayed.

The patch below adds a contrib/git-log-times directory containing the
executable (git-log-times) and the README.

--Kyle

P.S. git am complains about 26 lines with whitespace errors.  They are
 not whitespace errors.  The README is in markdown format and they
 are explicit line break instructions to markdown (2 trailing blanks).
 Removing them would corrupt the markdown output.

P.P.S A picture is worth a thousand words, so the formatted help text,
  and several images of actual git-log-times output are available at
  https://gist.github.com/mackyle/4c33e4802a8269b3f200f2c00352ce6a

-- 8< --
Subject: [PATCH] contrib/git-log-times: alternative git log --oneline utility

The git-log-times utility provides an alternative interface to using
git log --oneline that includes dates, times and author initials.

Additionally root commits are marked for easy identification and
when using --graph mode breaks are inserted when necessary to prevent
two adjacent output lines from being misconstrued as having a parent
child relationship when they actually do not.

Other than --notes, --pretty and --format options which are not
allowed (because that would no longer be a one line format) all git
log options are available for use.

Output will be colorized using the same rules used for git log
output.

The three extra items in the output (dates, times and initials) use
'color.log-times.date', 'color.log-times.time' and
'color.log-times.initials' to change their default color.

Other options specific to git-log-times may be shown by using the
-h option (i.e. `git-log-times -h`).

One or more default options which behave as though they are the
first option argument(s) on the command line may be set by assigning
them to the 'log-times.defaults' config value as space-separated
options each including its leading '-' or '--'.

Signed-off-by: Kyle J. McKay <mack...@gmail.com>
---
 contrib/git-log-times/README| 256 
 contrib/git-log-times/git-log-times | 464 
 2 files changed, 720 insertions(+)
 create mode 100644 contrib/git-log-times/README
 create mode 100755 contrib/git-log-times/git-log-times

diff --git a/contrib/git-log-times/README b/contrib/git-log-times/README
new file mode 100644
index ..65f1d2c5
--- /dev/null
+++ b/contrib/git-log-times/README
@@ -0,0 +1,256 @@
+git-log-times
+=
+
+An alterative to `git log --oneline` that includes dates, times and
+author initials in a compact one line output format.
+
+The `--notes`, `--pretty` and `--format` options are not allowed but any
+other `git log` options should work fine including `--graph`.
+
+In both `--graph` and non `--graph` modes:
+
+   * Root commits are identified by `_` on either side of the hash
+
+When `--graph` mode is enabled, the graph output is enhanced as follows:
+
+   * Breaks are inserted when necessary to avoid parent/child ambiguity
+
+
+Installation
+
+
+Put the `git-log-times` executable file in one of the directories
+included in the `PATH` environment variable.
+
+Optionally set a global alias to save typing such as `lo` like so:
+
+git config --global alias.lo log-times
+
+Optionally set global default options such as `--two-initials` and
+`--abbrev=8` like so:
+
+git config --global log-times.defaults "--two-initials --abbrev=8"
+
+
+Dates & Times
+-
+
+Dates and times are shown in the local timezone.  Set the TZ variable
+before running `git log-times` (e.g. `TZ=UTC git log-times` to show
+dates and times in UTC) or use the `--time-zone=` option (e.g.
+`git log-times --time-zone=UTC`) to change that.
+
+Dates a

What's the ".git/gitdir" file?

[Kyle Meyer]

Hello,

When a ".git" file points to another repo, a ".git/gitdir" file is
created in that repo.

For example, running

$ mkdir repo-a repo-b
$ cd repo-a
$ git init
$ cd ../repo-b
$ echo "gitdir: ../repo-a/.git" > .git
$ git status

results in a file "repo-a/.git/gitdir" that contains

$ cat repo-a/.git/gitdir
.git

I don't see this file mentioned in the gitrepository-layout manpage,
and my searches haven't turned up any information on it.  What's the
purpose of ".git/gitdir"?  Are there cases where it will contain
something other than ".git"?

Thanks.

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

Re: [PATCH 0/3] Make httpd tests run

[Kyle J. McKay]

On Apr 8, 2015, at 08:05, Michael J Gruber wrote:

This series grew from an attempt at enlarging my personal test run  
coverage

on a standard Fedora 21 64bit box. Aka chain-lint fall-out.

With 1/3, I get all httpd tests to run (when port is set, of course).

2/3 and 3/3 are an attempt at getting git-svn over http tests to run.
2/3 is certainly correct but not sufficient.
3/3 gets httpd to run but svn does not connect. This is WIP and RFH,
and maybe requires rewriting lib-git-svn to use a config which depends
on the apache version (like lib-hhtpd does), or to leverage lib-httpd.

Michael J Gruber (3):
 t/lib-httpd: load mod_unixd
 t/lib-git-svn: check same httpd module dirs as lib-httpd
 t/lib-git-svn: adjust config to apache 2.4


The changes in this series appear to break compatibility with Apache  
2.2.


Does that mean that Apache 2.2 is no longer supported for running  
these tests?


Or am I missing something here...

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

Re: [PATCH 0/3] Make httpd tests run

[Kyle J. McKay]

On Apr 9, 2015, at 06:04, Kyle J. McKay wrote:


On Apr 8, 2015, at 08:05, Michael J Gruber wrote:

This series grew from an attempt at enlarging my personal test run  
coverage

on a standard Fedora 21 64bit box. Aka chain-lint fall-out.

With 1/3, I get all httpd tests to run (when port is set, of course).

2/3 and 3/3 are an attempt at getting git-svn over http tests to run.
2/3 is certainly correct but not sufficient.
3/3 gets httpd to run but svn does not connect. This is WIP and RFH,
and maybe requires rewriting lib-git-svn to use a config which  
depends
on the apache version (like lib-hhtpd does), or to leverage lib- 
httpd.


Michael J Gruber (3):
t/lib-httpd: load mod_unixd
t/lib-git-svn: check same httpd module dirs as lib-httpd
t/lib-git-svn: adjust config to apache 2.4


The changes in this series appear to break compatibility with Apache  
2.2.


Does that mean that Apache 2.2 is no longer supported for running  
these tests?


Or am I missing something here...


Never mind.  I see the IfVersion... they're wrapped in now.  Would  
have been nice if they were indented so there was a hint about that in  
the diff, or the diff included enough context to see that.  But that  
file formatting has nothing to do with your change.


Sorry for the noise.

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

Re: git 2.3.4, ssh: Could not resolve hostname

[Kyle J. McKay]

On Apr 2, 2015, at 17:02, Torsten Bögershausen wrote:


On 2015-04-02 21.35, Jeff King wrote:

On Thu, Apr 02, 2015 at 12:31:14PM -0700, Reid Woodbury Jr. wrote:


Ah, understand. Here's my project URL for 'remote origin' with a
more meaningful representation of their internal FQDN:

url = ssh://rwoodbury@systemname.groupname.online:/opt/git/inventory.git

The online is their literal internal TLD.


Thanks. The problem is the extra : after online; your URL is
malformed. You can just drop that colon entirely.

I do not think we need to support this syntax going forward (the  
colon

is meaningless here, and our documentation is clear that it should go
with a port number), but on the other hand, it might be nice to be  
more
liberal, as we were in v2.3.3 and prior. I'll leave it to Torsten  
to see
whether supporting that would hurt some of the other cases, or  
whether

it would make the code too awkward.

-Peff


Thanks for digging.

This makes my think that it is
a) non-standard to have the extra colon


It's not.  See RFC 3986 appendix A:

  authority = [ userinfo @ ] host [ : port ]

  port = *DIGIT

*DIGIT means (see RFC 2234 section 3.6) zero or more digits.


b) The error message could be better
c) We don't have a test case
d) This reminds my of an improvement from Linus:
608d48b2207a61528
..
   So when somebody passes me a please pull request pointing to  
something

   like the following

git://git.kernel.org:/pub/scm/linux/kernel/git/mchehab/v4l-dvb.git

   (note the extraneous colon at the end of the host name), git  
would happily
   try to connect to port 0, which would generally just cause the  
remote to
   not even answer, and the connect() will take a long time to  
time out.

.

Sorry guys for the regression, the old parser handled the extra  
colon as port 0,
the new one looks for the / as the end of the hostname (and the  
beginning of the path)


Either we accept the extra colon as before, or the parser puts out a  
better error message,


[...]

Spontaneously I would say that a trailing ':' at the end of a  
hostname in the ssh:// scheme

can be safely ignored, what do you think ?


You know, there is a url_normalize routine in urlmatch.h/urlmatch.c  
that checks for a lot of these things and provides a translated error  
message if there's a problem as well as normalizing and separating out  
the various parts of the URL.  It does not currently handle default  
ports for anything other than http[s] but it would be simple enough to  
add support for ssh, git, ftp[s] and rsync default ports too.


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

Re: [PATCH] diff-highlight: Fix broken multibyte string

[Kyle J. McKay]

On Apr 3, 2015, at 15:08, Jeff King wrote:

Doing:

diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff- 
highlight/diff-highlight

index 08c88bb..1c4b599 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -165,7 +165,7 @@ sub highlight_pair {
sub split_line {
local $_ = shift;
return map { /$COLOR/ ? $_ : (split //) }
-  split /($COLOR*)/;
+  split /($COLOR+)/;
}

sub highlight_line {

gives me a 25% speed improvement, and the same output processing
git.git's entire git log -p output.

I thought that meant we could also optimize out the map call  
entirely,
and just use the first split (with *) to end up with a list of  
$COLOR

chunks and single characters, but it does not seem to work. So maybe I
am misreading something about what is going on.


I think our emails crossed in flight...

Using just the first split (with *) produces useless empty elements  
which I think ends up causing problems.  I suppose you could surround  
it with a grep /./ to remove them but that would defeat the point of  
the optimization.


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

[PATCH v3] diff-highlight: do not split multibyte characters

[Kyle J. McKay]

When the input is UTF-8 and Perl is operating on bytes instead of
characters, a diff that changes one multibyte character to another
that shares an initial byte sequence will result in a broken diff
display as the common byte sequence prefix will be separated from
the rest of the bytes in the multibyte character.

For example, if a single line contains only the unicode character
U+C9C4 (encoded as UTF-8 0xEC, 0xA7, 0x84) and that line is then
changed to the unicode character U+C9C0 (encoded as UTF-8 0xEC,
0xA7, 0x80), when operating on bytes diff-highlight will show only
the single byte change from 0x84 to 0x80 thus creating invalid UTF-8
and a broken diff display.

Fix this by putting Perl into character mode when splitting the line
and then back into byte mode after the split is finished.

The utf8::xxx functions require Perl 5.8 so we require that as well.

Also, since we are mucking with code in the split_line function, we
change a '*' quantifier to a '+' quantifier when matching the $COLOR
expression which has the side effect of speeding everything up while
eliminating useless '' elements in the returned array.

Reported-by: Yi EungJun semtlen...@gmail.com
Signed-off-by: Kyle J. McKay mack...@gmail.com
---

On Apr 2, 2015, at 19:19, Yi, EungJun wrote:
 I timed this one versus the existing diff-highlight. It's about 7%
 slower. That's not great, but is acceptable to me. The  
 String::Multibyte
 version was a lot faster, which was nice (but I'm still unclear on
 _why_).

 I think the reason is here:

 sub split_line {
   local $_ = shift;
   return map { /$COLOR/ ? $_ : ($mbcs ? $mbcs-strsplit('', $_) :  
 split //) }
  split /($COLOR)/;
 }

 I removed * from split /($COLOR*)/. Actually I don't know why *
 was required but I need to remove it to make my patch works correctly.

 On Fri, Apr 3, 2015 at 10:24 AM, Jeff King p...@peff.net wrote:
 EungJun, does this version meet your needs?

This version differs from the former as follows:

1) Slightly faster code that eliminates the need for Encode::is_utf8.

2) The '*' quantifier is changed to '+' in the split_line regexs which  
was probably the original intent anyway as using '*' generates useless  
empty elements.  This has the side effect of greatly increasing the  
speed so the tiny speed penalty for the UTF-8 checking is vastly  
overwhelmed by the overall speed up. :)

3) The 'use 5.008;' line has been added since the utf8::xxx functions  
require Perl 5.8

-Kyle

 contrib/diff-highlight/diff-highlight | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/contrib/diff-highlight/diff-highlight 
b/contrib/diff-highlight/diff-highlight
index 08c88bbc..ffefc31a 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -1,5 +1,6 @@
 #!/usr/bin/perl
 
+use 5.008;
 use warnings FATAL = 'all';
 use strict;
 
@@ -164,8 +165,12 @@ sub highlight_pair {
 
 sub split_line {
local $_ = shift;
-   return map { /$COLOR/ ? $_ : (split //) }
-  split /($COLOR*)/;
+   return utf8::decode($_) ?
+   map { utf8::encode($_); $_ }
+   map { /$COLOR/ ? $_ : (split //) }
+   split /($COLOR+)/ :
+   map { /$COLOR/ ? $_ : (split //) }
+   split /($COLOR+)/;
 }
 
 sub highlight_line {
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] diff-highlight: Fix broken multibyte string

[Kyle J. McKay]

On Apr 2, 2015, at 18:24, Jeff King wrote:


On Thu, Apr 02, 2015 at 05:49:24PM -0700, Kyle J. McKay wrote:


Subject: [PATCH v2] diff-highlight: do not split multibyte characters

When the input is UTF-8 and Perl is operating on bytes instead
of characters, a diff that changes one multibyte character to
another that shares an initial byte sequence will result in a
broken diff display as the common byte sequence prefix will be
separated from the rest of the bytes in the multibyte character.


Thanks, I had a feeling we should be able to do something with perl's
builtin utf8 support.  This doesn't help people with other encodings,


It should work as well as the original did for any 1-byte encoding.   
That is, if it's not valid UTF-8 it should pass through unchanged and  
any single byte encoding should just work.  But, as you point out,  
multibyte encodings other than UTF-8 won't work, but they should  
behave the same as they did before.



but I'm not sure the original was all that helpful either (in that we
don't actually _know_ the file encodings in the first place).


I think it should work fine on any single byte encoding (i.e. ISO-8859- 
x, WINDOWS-1252, etc.).



I timed this one versus the existing diff-highlight. It's about 7%
slower.


I'd expect that, we're doing extra work we weren't doing before.


That's not great, but is acceptable to me. The String::Multibyte
version was a lot faster, which was nice (but I'm still unclear on
_why_).


Must be the mbcs-strsplit routine has special case code for splitting  
on '' to just split on character boundaries.



Fix this by putting Perl into character mode when splitting the
line and then back into byte mode after the split is finished.


I also wondered if we could simply put stdin into utf8 mode. But it
looks like it will barf whenever it gets invalid utf8. Checking for
valid utf8 and only doing the multi-byte split in that case (as you do
here) is a lot more robust.


While the utf8::xxx functions are built-in and do not require
any 'use' statement, the utf8::is_utf8 function did not appear
until Perl 5.8.1, but is identical to the Encode::is_utf8
function which is available in 5.8 so we use that instead of
utf8::is_utf8.


Makes sense. I'm happy enough listing perl 5.8 as a dependency.


Maybe that should be added.  The rest of Git's perl code seems to have  
a 'use 5.008;' already, so I figured that was a reasonable  
dependency.  :)


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

Re: [PATCH] diff-highlight: Fix broken multibyte string

[Kyle J. McKay]

On Mar 30, 2015, at 15:16, Jeff King wrote:

 Yeah, I agree the current output is not ideal, and this should address
 the problem. I was worried that multi-byte splitting would make things
 slower, but in my tests, it actually speeds things up!

[...]

 Unfortunately, String::Multibyte is not a standard module, and is not
 even packed for Debian systems (I got mine from CPAN). Can we make
 this
 a conditional include (e.g., 'eval require String::Multibyte' in
 get_mbcs, and return undef if that fails?). Then people without it can
 still use the script.

[...]

 Yuck. This is a lot more intimate with String::Multibyte's
 implementation than I'd like to be.

So I was curious about this and played with it and was able to
reproduce the problem as described.

Here's an alternate fix that should work for everyone with Perl 5.8
or later.

-Kyle

-- 8 --
Subject: [PATCH v2] diff-highlight: do not split multibyte characters

When the input is UTF-8 and Perl is operating on bytes instead
of characters, a diff that changes one multibyte character to
another that shares an initial byte sequence will result in a
broken diff display as the common byte sequence prefix will be
separated from the rest of the bytes in the multibyte character.

For example, if a single line contains only the unicode
character U+C9C4 (encoded as UTF-8 0xEC, 0xA7, 0x84) and that
line is then changed to the unicode character U+C9C0 (encoded as
UTF-8 0xEC, 0xA7, 0x80), when operating on bytes diff-highlight
will show only the single byte change from 0x84 to 0x80 thus
creating invalid UTF-8 and a broken diff display.

Fix this by putting Perl into character mode when splitting the
line and then back into byte mode after the split is finished.

While the utf8::xxx functions are built-in and do not require
any 'use' statement, the utf8::is_utf8 function did not appear
until Perl 5.8.1, but is identical to the Encode::is_utf8
function which is available in 5.8 so we use that instead of
utf8::is_utf8.

Reported-by: Yi EungJun semtlen...@gmail.com
Signed-off-by: Kyle J. McKay mack...@gmail.com
---
 contrib/diff-highlight/diff-highlight | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/contrib/diff-highlight/diff-highlight 
b/contrib/diff-highlight/diff-highlight
index 08c88bbc..8e9b5ada 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -2,6 +2,7 @@
 
 use warnings FATAL = 'all';
 use strict;
+use Encode ();
 
 # Highlight by reversing foreground and background. You could do
 # other things like bold or underline if you prefer.
@@ -164,8 +165,10 @@ sub highlight_pair {
 
 sub split_line {
local $_ = shift;
-   return map { /$COLOR/ ? $_ : (split //) }
-  split /($COLOR*)/;
+   utf8::decode($_);
+   return map { utf8::encode($_) if Encode::is_utf8($_); $_ }
+   map { /$COLOR/ ? $_ : (split //) }
+   split /($COLOR*)/;
 }
 
 sub highlight_line {
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Apr 2015, #01; Thu, 2)

[Kyle J. McKay]

On Apr 2, 2015, at 15:09, Junio C Hamano wrote:


* jc/show-branch (2014-03-24) 5 commits
- show-branch: use commit slab to represent bitflags of arbitrary  
width

- show-branch.c: remove all_mask
- show-branch.c: abstract out flags operation
- show-branch.c: lift all_mask/all_revs to a global static
- show-branch.c: update comment style

Waiting for the final step to lift the hard-limit before sending it  
out.



May I ask, now that this topic is over 1-year old, please, what is the  
final step?


Perhaps someone might submit a patch... ;)

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

Re: What's cooking in git.git (Mar 2015, #09; Thu, 26)

[Kyle J. McKay]

On Mar 26, 2015, at 14:09, Junio C Hamano wrote:


* jc/show-branch (2014-03-24) 5 commits
- show-branch: use commit slab to represent bitflags of arbitrary  
width

- show-branch.c: remove all_mask
- show-branch.c: abstract out flags operation
- show-branch.c: lift all_mask/all_revs to a global static
- show-branch.c: update comment style

Waiting for the final step to lift the hard-limit before sending it  
out.


May I ask, on the 1-year anniversary of this topic, please, what is  
the final step?


Perhaps someone might submit a patch...  ;)

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

Re: What's cooking in git.git (Mar 2015, #07; Fri, 20)

[Kyle J. McKay]

On Mar 20, 2015, at 15:02, Junio C Hamano wrote:


* bc/object-id (2015-03-13) 10 commits
- apply: convert threeway_stage to object_id
- patch-id: convert to use struct object_id
- commit: convert parts to struct object_id
- diff: convert struct combine_diff_path to object_id
- bulk-checkin.c: convert to use struct object_id
- zip: use GIT_SHA1_HEXSZ for trailers
- archive.c: convert to use struct object_id
- bisect.c: convert leaf functions to use struct object_id
- define utility functions for object IDs
- define a structure for object IDs

Identify parts of the code that knows that we use SHA-1 hash to
name our objects too much, and use (1) symbolic constants instead
of hardcoded 20 as byte count and/or (2) use struct object_id
instead of unsigned char [20] for object names.

Will cook in 'next'.



Has this been merged to 'next'?  Even fetching github.com/gitster/ 
git.git I'm only seeing it in pu:


$ git rev-parse bc/object-id
d07d4ab401173a10173f2747cf5e0f074b6d2b39

$ git branch --contains d07d4ab401173a10173f2747cf5e0f074b6d2b39 --all
  bc/object-id
  jch
  pu
  remotes/github2/pu
  remotes/gob-private/pu
  remotes/gph/pu
  remotes/ko/pu
  remotes/repo/pu

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

Re: What's cooking in git.git (Mar 2015, #07; Fri, 20)

[Kyle J. McKay]

On Mar 20, 2015, at 16:29, Stefan Beller wrote:

On Fri, Mar 20, 2015 at 4:24 PM, Kyle J. McKay mack...@gmail.com  
wrote:



On Mar 20, 2015, at 15:02, Junio C Hamano wrote:


* bc/object-id (2015-03-13) 10 commits

[snip]

Will cook in 'next'.


Has this been merged to 'next'?


Usually Junio writes the mail first and then does a git push all  
the branches

just before being done for the day. At least that's my suspicion as an
observer of
the timing when git fetch returns new shiny stuff and when these
emails are sent.



I would expect that if it said, Will merge to 'next'.

However the What's cooking in git.git (Mar 2015, #06; Tue, 17) also  
says Will cook in 'next' for this topic so I think that perhaps it's  
fallen through the cracks somehow.


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

Bug in fetch-pack.c, please confirm

[Kyle J. McKay]

Hi guys,

So I was looking at fetch-pack.c (from master @ 52cae643, but I think  
it's the same everywhere):

# Lines 626-648

 for (retval = 1, ref = *refs; ref ; ref = ref-next) {
 const unsigned char *remote = ref-old_sha1;
 unsigned char local[20];
 struct object *o;

 o = lookup_object(remote);
 if (!o || !(o-flags  COMPLETE)) {
 retval = 0;
 if (!args-verbose)
 continue;
 fprintf(stderr,
 want %s (%s)\n, sha1_to_hex(remote),
 ref-name);
 continue;
 }

 hashcpy(ref-new_sha1, local);
 if (!args-verbose)
 continue;
 fprintf(stderr,
 already have %s (%s)\n, sha1_to_hex(remote),
 ref-name);
 }

Peff, weren't you having some issue with want and have and hide refs?

Tell me please how the local variable above gets initialized?

It's declared on the stack inside the for loop scope so only  
guaranteed to have garbage.

It's passed to hashcpy which has this prototype:

  inline void hashcpy(unsigned char *sha_dst, const unsigned char *sha_src);

So it looks to me like garbage is copied into rev-new_sha1, yes?

Something's very wrong here.

It looks to me like the bug was introduced in 49bb805e (Do not ask for  
objects known to be complete. 2005-10-19).

I've taken a stab a a fix below.

-Kyle

-- 8 --
Subject: [PATCH] fetch-pack.c: do not use uninitialized sha1 value

Since 49bb805e (Do not ask for objects known to be complete. 2005-10-19)
when the read_ref call was replaced with a parse_object call, the
automatic variable 'local' containing an sha1 has been left uninitialized.

Subsequently in 1baaae5e (Make maximal use of the remote refs, 2005-10-28)
the parse_object call was replaced with a lookup_object call but still
the 'local' variable was left uninitialized.

However, it's used as the source to update another sha1 value in the case
that we already have it and in that case the other ref will end up with
whatever garbage was in the 'local' variable.

Fix this by removing the 'local' variable and using the value from the
result of the lookup_object call instead.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---
 fetch-pack.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 655ee642..c0b4d84f 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -621,29 +621,28 @@ static int everything_local(struct fetch_pack_args *args,
}
}
 
filter_refs(args, refs, sought, nr_sought);
 
for (retval = 1, ref = *refs; ref ; ref = ref-next) {
const unsigned char *remote = ref-old_sha1;
-   unsigned char local[20];
struct object *o;
 
o = lookup_object(remote);
if (!o || !(o-flags  COMPLETE)) {
retval = 0;
if (!args-verbose)
continue;
fprintf(stderr,
want %s (%s)\n, sha1_to_hex(remote),
ref-name);
continue;
}
 
-   hashcpy(ref-new_sha1, local);
+   hashcpy(ref-new_sha1, o-sha1);
if (!args-verbose)
continue;
fprintf(stderr,
already have %s (%s)\n, sha1_to_hex(remote),
ref-name);
}
return retval;
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug in fetch-pack.c, please confirm

[Kyle J. McKay]

On Mar 15, 2015, at 00:30, Junio C Hamano wrote:


Junio C Hamano gits...@pobox.com writes:


Kyle J. McKay mack...@gmail.com writes:


Hi guys,

So I was looking at fetch-pack.c (from master @ 52cae643, but I  
think

it's the same everywhere):


...

-   hashcpy(ref-new_sha1, local);
+   hashcpy(ref-new_sha1, o-sha1);
if (!args-verbose)
continue;
fprintf(stderr,
already have %s (%s)\n, sha1_to_hex(remote),
ref-name);
}
return retval;
---


One thing I wonder is if this hashcpy() is doing anything useful,
though.  Is ref-new_sha1 used after we are done in this codepath,
or is the reason nobody noticed it is because it does not matter
whatever garbage is in that field nobody looks at it?


My thoughts exactly. hence the please confirm request.  I'm not  
familiar enough with the fetch pack code to know the answer off the  
top of my head.  I was hoping someone else who's been in the fetch  
pack code recently (*Hi Peff*) might just already know.  :)

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

Re: Any chance for a Git v2.1.5 release?

[Kyle J. McKay]

The first two messages in this thread were dropped by the vger mailing  
list for some unknown reason.


In an attempt to make the mailing list archives of this thread  
complete, the original two messages in this thread are included below.


-Kyle

 BEGIN FIRST MESSAGE 
From: Kyle J. McKay mack...@gmail.com
Date: February 24, 2015 09:16:05 PST
To: Junio C Hamano gits...@pobox.com
Cc: Git Mailing List git@vger.kernel.org
Subject: Any chance for a Git v2.1.5 release?
Message-Id: c5211e53-8905-41c9-9d28-26d7bb51e...@gmail.com
Content-Type: text/plain; charset=US-ASCII; format=flowed; delsp=yes

As you know, I help out with repo.or.cz.  Recently the admins have  
been discussing upgrading the version of Git we use in order to  
support newer features such as pack bitmaps.  While we do use a  
slightly customized gitweb, we have always stuck to an official Git  
release for everything else.


Repo.or.cz provides fetch (git, http, https, ssh), browsing (gitweb)  
and push (ssh, https).  Additionally, created repositories can be  
mirrors (no push allowed) of other repositories (including svn via git- 
svn).  Email notification of ref changes (along with diffs) can also  
be requested.


We are finding that in order to upgrade Git at this point we would  
need to build a custom Git with cherry picked fixes for various issues  
that have come up or they would likely be triggered by one of the  
services repo.or.cz provides.  In addition, as there are numerous  
reports of an unresolved issue with git-svn starting with v2.2.0  
upgrading to v2.2.0 or later presents a problem since we have several  
mirrors that rely on git-svn.


Which brings us back to the subject of this email, is there any chance  
for a v2.1.5 release?


After reviewing a few release dates:

2011-06-26T12:41:26-07:00 v1.7.6
2012-02-05T23:51:07-08:00 v1.7.6.6

2013-11-27T12:14:52-08:00 v1.8.5
2014-02-13T13:42:01-08:00 v1.8.5.5

2014-02-14T11:36:11-08:00 v1.9.0
2014-05-30T10:15:10-07:00 v1.9.4

2014-05-28T11:04:29-07:00 v2.0.0
2014-07-30T14:20:01-07:00 v2.0.4

2014-08-15T15:09:28-07:00 v2.1.0
2014-10-29T10:48:57-07:00 v2.1.3

2014-11-26T13:18:43-08:00 v2.2.0
2015-01-12T14:06:20-08:00 v2.2.2

2015-02-05T13:24:05-08:00 v2.3.0

(I have omitted the dates of the 5 security releases on 2014-12-17 as  
that seems like an extraordinary event unlikely to be repeated.)


It appears that the average support lifespan of a Git release from  
initial release date through last released maintenance update is  
approximately 2-3 months with the 1.7.6 release being an exception at  
a little over 7 months.


If a v2.1.5 release is out of the question, would it be possible to  
periodically designate certain Git releases as long term support  
releases?  Meaning that they would receive maintenance updates (e.g.  
fixes for invalid memory accesses/crashes, regressions or security  
issues) for an extended period of time, say at least 6 months?



Here's the analysis that led to this request:


Goal 1: add support for symref=HEAD:refs/... capability

Goal 2: add support for pack bitmaps

Nice to have: The CVE-2014-9390 fix, but repo.or.cz does not create  
any working trees so it's not mandatory.



Goal 1:

symref=HEAD:refs/... requires at least Git 1.8.4.3.  However,  
repo.or.cz runs git-daemon with read-only access to the repositories  
and since Git 1.8.4.2 shallow clones require write access.


This was corrected in v2.0.0.  So at least v2.0.0 would be needed for  
symref=HEAD:refs/



Goal 2:

Pack bitmap support was added in v2.0.0, but it's probably not a good  
idea to run without 21134714 (pack-objects: turn off bitmaps when we  
split packs) just in case which requires at least v2.1.3.



Nice to have:

If at least v2.1.3 is required, then we might as well use v2.1.4 since  
the primary change from v2.1.3 to v2.1.4 is the addition of the  
CVE-2014-9390 fix.



What about a later release, v2.2.0 or later?


git-svn is reported to suffer from occasional .git/ 
Git_svn_hash_XX: Bad file descriptor errors since v2.2.0 making  
that a non-starter.  No fix is currently available in the Git  
repository.


Since 660c889e (sha1_file: add for_each iterators for loose and packed  
objects) loose objects in alternates directories may not be found when  
pruning.  This affects v2.2.0 and later.  A fix is currently in  
master.  This is an absolute must have for repo.or.cz as alternates  
are used to implement repository forks.


Since d38379e (make update-server-info more robust), affecting v2.2.x,  
the files used by non-smart HTTP clients could have the wrong  
permissions.  This might preclude them from being updated correctly on  
repo.or.cz.  It would require research to see if repo.or.cz is  
affected.  The fix for this d91175b2 (update-server-info: create info/ 
* with mode 0666) was released in v2.3.0.



So why not v2.1.4 then?


There's an XDL_FAST_HASH performance regression that affects v1.7.11  
and later [1].  But that can be turned

Re: [PATCH v2 00/10] Use a structure for object IDs.

[Kyle J. McKay]

On Mar 7, 2015, at 15:23, brian m. carlson wrote:
This is a patch series to convert some of the relevant uses of  
unsigned

char [20] to struct object_id.

brian m. carlson (10):


All patches applied for me (to master) and all tests pass.

Tested-by: Kyle J. McKay



 Define a structure for object IDs.


Comments in reply to the patch.



 Define utility functions for object IDs.
 bisect.c: convert leaf functions to use struct object_id
 archive.c: convert to use struct object_id
 zip: use GIT_SHA1_HEXSZ for trailers
 bulk-checkin.c: convert to use struct object_id
 diff: convert struct combine_diff_path to object_id
 commit: convert parts to struct object_id
 patch-id: convert to use struct object_id
 apply: convert threeway_stage to object_id


These all look good, the conversions are simple and easy to follow.


On Mar 7, 2015, at 23:43, Junio C Hamano wrote:

brian m. carlson sand...@crustytoothpaste.net writes:

Certain parts of the code have to be converted before others to  
keep the

patch sizes small, maintainable, and bisectable, so functions and
structures that are used across the codebase (e.g. struct object)  
will

be converted later.  Conversion has been done in a somewhat haphazard
manner by converting modules with leaf functions and less commonly  
used

structs first.


That leaf-first approach sounds very sensible.

In the medium term, I wonder if the changes can progress faster and
in a less error prone way if you first used GIT_SHA1_RAWSZ in places
that cannot be immediately converted to the struct yet.  That way,
we will be easily tell by git grep GIT_SHA1_RAWSZ how many more
places need treatment.  I do not know if that is all that useful
offhand, though.  Those places need to be touched in the second pass
to use the struct again, after the s/\[20\]/[GIT_SHA1_RAWSZ]/
first pass.


I definitely noticed the leaf-first approach as I was looking through  
the patches where, for example (03/10), this prototype was left  
unchanged:


static int register_ref(const char *refname, const unsigned char *sha1,
int flags, void *cb_data)

but its contents got the update leaving it half converted.  As  
mentioned above this makes the patches more manageable, maintainable  
and bisectable.  However, these functions could be converted to take a  
typedef (a quick grep of 'CodingGuidelines' does not mention typedef)  
and perhaps, as Junio mentions above, help the changes progress faster  
by making it easier to find the affected code (e.g. changing or  
removing the typedef would make the compiler find them for you).


For example, if we added this to object.h:

typedef unsigned char sha1raw_t[GIT_SHA1_RAWSZ];

then the above prototype could be immediately converted to (and this  
does compile and pass all the tests):


static int register_ref(const char *refname, const sha1raw_t sha,
int flags, void *cb_data)

So that together with Junio's suggestion above (and perhaps also a  
sha1hex_t type) would help mark everything in the first pass that  
needs to be touched again in the second pass.  (I'm just throwing out  
some typedef names as an example, there may be more preferable names  
to sha1raw_t and sha1hex_t, but those names would end up being  
replaced eventually anyway.)


-Kyle 
--

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

Re: [PATCH] protocol upload-pack-v2

[Kyle J. McKay]

On Mar 9, 2015, at 18:38, Duy Nguyen wrote:

A minor point on capability negotiation. I remember why I passed
capabilities via command line instead of this proposal. With this
proposal, daemon.c does not recognize i18n capability and cannot
switch to the correct language before it reports an error.

But perhaps I approached it the wrong way. Instead of letting the
daemon produce non-English language directly, if could sort of
standardize the messages and send an error code back in ERR line,
together with an English message (current pack-protocol.txt says ERR
followed by explanation-text). The client can use this error code to
print non-English messages if it wants to. Perhaps we can reuse http
(or ftp) return codes or some other protocol then customize to fit
Git's needs.


May I suggest that you take a look at RFC 2034 [1].  It describes  
almost exactly this same situation and how SMTP was enhanced to  
support error code numbers that could then be translated.


No reason this enhancement needs to be restricted to protocol v2 if  
it's just an enhancedstatuscodes capability the server sends back to  
indicate that its ERR text is in that format.


-Kyle

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

[PATCH v2] t7510: do not fail when gpg warns about insecure memory

[Kyle J. McKay]

Depending on how gpg was built, it may issue the following
message to stderr when run:

  Warning: using insecure memory!

When the test is collecting gpg output it is therefore not
enough to just match on a gpg:  prefix it must also match
on a Warning:  prefix wherever it needs to match lines
that have been produced by gpg.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---

How about this patch instead.  It just treats Warning: lines as gpg  
output and the test still passes when Warning: using insecure memory  
shows up.

-Kyle

 t/t7510-signed-commit.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 474dab38..3cef18cf 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -86,8 +86,8 @@ test_expect_success GPG 'show signed commit with signature' '
git show -s --show-signature initial show 
git verify-commit -v initial verify.1 2verify.2 
git cat-file commit initial cat 
-   grep -v gpg:  show show.commit 
-   grep gpg:  show show.gpg 
+   grep -v -e gpg:  -e Warning:  show show.commit 
+   grep -e gpg:  -e Warning:  show show.gpg 
grep -v ^  cat | grep -v ^gpgsig  cat.commit 
test_cmp show.commit commit 
test_cmp show.gpg verify.2 
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] help.c: use SHELL_PATH instead of hard-coded /bin/sh

[Kyle J. McKay]

On Mar 7, 2015, at 23:52, Junio C Hamano wrote:

Kyle J. McKay mack...@gmail.com writes:


If the user has set SHELL_PATH in the Makefile then we
should respect that value and use it.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---
builtin/help.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/help.c b/builtin/help.c
index 6133fe49..2ae8a1e9 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -171,7 +171,7 @@ static void exec_man_cmd(const char *cmd, const  
char *page)

{
struct strbuf shell_cmd = STRBUF_INIT;
strbuf_addf(shell_cmd, %s %s, cmd, page);
-   execl(/bin/sh, sh, -c, shell_cmd.buf, (char *)NULL);
+   execl(SHELL_PATH, SHELL_PATH, -c, shell_cmd.buf, (char *)NULL);


It is a common convention to make the first argument the command
name without its path, and this change breaks that convention.


Hmpf.  I present these for your consideration:

$ sh -c 'echo $0'
sh
$ /bin/sh -c 'echo $0'
/bin/sh
$ cd /etc
$ ../bin/sh -c 'echo $0'
../bin/sh

I always thought it was the actual argument used to invoke the item.   
If the item is in the PATH and was invoked with a bare word then arg0  
would be just the bare word or possibly the actual full pathname as  
found in PATH.  Whereas if it's invoked with a path (relative or  
absolute) that would passed instead.



Does it matter, or would it break something?  I recall that some
implementations of shell (e.g. bash) change their behaviour
depending on how they are invoked (e.g. ln -s bash /bin/sh makes
it run in posix mode) but I do not know if they do so by paying
attention to their argv[0].


Several shells are sensitive to argv[0] in that if it starts with a  
'-' then they become a login shell.  Setting SHELL_PATH to anything  
that is not an absolute path is likely to break things in other ways  
though so that doesn't seem like a possibility here.



There might be other fallouts I do not
think of offhand here.

I do not have an objection to what these patches want to do, though.


I also have no objection to changing it to:


-   execl(/bin/sh, sh, -c, shell_cmd.buf, (char *)NULL);
+	execl(SHELL_PATH, basename(SHELL_PATH), -c, shell_cmd.buf, (char  
*)NULL);


just to maintain the current behavior.

Would you be able to squash that change in or shall I re-roll?

-Kyle

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

Re: [PATCH] t7510: do not fail when gpg warns about insecure memory

[Kyle J. McKay]

On Mar 8, 2015, at 18:22, brian m. carlson wrote:


On Sun, Mar 08, 2015 at 06:15:55PM -0400, Eric Sunshine wrote:

On Sun, Mar 8, 2015 at 6:04 PM, brian m. carlson
sand...@crustytoothpaste.net wrote:

Perhaps this is better?

Unfortunately when running the test, that message is found in the   
standard
output of git show -s --show-signature, but in the standard  error  
of git
verify-commit -v causing the comparisons of both standard  output  
and

standard error to fail.


That doesn't help me parse it any better.  It's the but without a
corresponding not which seems to be throwing me off. Typically, one
would write this but not that or not this but that. I can't tell
if there is a not missing or if the but is supposed to be an  
and

or if something else was intended.


The intended meaning is and with the additional sense of contrast.  
The sentence, if read with verbal emphasis, is, …is found in the  
standard *output* of git show -s --signature, but in the standard  
*error* of git verify-commit -v, thus demonstrating why the test  
fails: the pairs of output files don't match up.


Maybe you can suggest a less confusing wording.


I like Brian's wording, but how about this slight variation, does this  
parse any better for you?


Unfortunately when running the test, while that message is found in
the standard output of git show -s --show-signature, it is found in
the standard error of git verify-commit -v causing the comparisons
of both standard output and standard error to fail.

-Kyle

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

Re: [PATCH] t5528: do not fail with FreeBSD shell

[Kyle J. McKay]

On Mar 8, 2015, at 10:56, Jeff King wrote:

On Sun, Mar 08, 2015 at 08:37:50AM -0700, Kyle J. McKay wrote:


The FreeBSD shell converts this expression:

 git ${1:+-c push.default=$1} push

to this when $1 is not empty:

 git -c push.default=$1 push

which causes git to fail.


Hmph, just when I thought I knew about all of the weird shell  
quirks. :)


I am not convinced this isn't a violation of POSIX (which specifies  
that

field splitting is done on the results of parameter expansions outside
of double-quotes). But whether it is or not, we have to live with it.


That's not the only problem the shell has, t5560 had an issue, rebase  
had issues.  They've have been worked around.  It probably also  
affects related BSDs' shells as well (at least older versions that  
didn't change the shell).



For my own curiosity, what does:

 foo='with space'
 printf %s\n ${foo:+first $foo}

print? That is, are the double-quotes even doing anything on such a
shell? On bash and dash, it prints:

 first
 with space

which is what I would expect.



$ foo='with space'
$ printf %s\n ${foo:+first $foo}
first with space

I also happen to have a handy-dandy test program called showargs.

$ foo='with space'
$ showargs ${foo:+first $foo}
uid=1001 euid=1001
gid=1001 egid=1001
umask(octal)=022
stdin=/dev/pts/12 stdout=/dev/pts/12 stderr=/dev/pts/12
pid=5261
$0=showargs
$1=first with space

So no quotes are being passed on.  Of course bash works just fine.


So does ash (0.5.7, packaged for
Debian), which is what I _thought_ FreeBSD's shell was based on. But
clearly there is some divergence.


I like to test on FreeBSD 8, which is slightly older, once in a while  
to make sure I catch stuff like this.  :)


Running ident /bin/sh shows a bunch of source file names which  
matches up pretty well with the dash distribution so I'm pretty sure  
it's just a much older ancestor of dash/ash.


If I run dash 0.5.6 (installed via FreeBSD ports), it works properly  
too.



I guess they are getting eaten by your shell, otherwise we would pass
them along to git in the test script, which would complain.


When I run t5528 with -v -x -d -i this is where it dies (without the  
fix):


+ git '-c push.default=upstream' push
Unknown option: -c push.default=upstream

So yeah, the quotes are gone, but no word-splitting occurred.

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

[PATCH] t7510: do not fail when gpg warns about insecure memory

[Kyle J. McKay]

Depending on how gpg was built, it may issue the following
message to stderr when run:

  Warning: using insecure memory!

Unfortunately when running the test, that message gets
collected in the stdout result of git show -s --show-signature
but is collected in the stderr result of git verify-commit -v
causing both the stdout and stderr result comparisions to fail.

Since checking for secure memory use by gpg is not the point of
this test, filter out such messages to allow the test to pass
even when gpg is using insecure memory.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---
 t/t7510-signed-commit.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 474dab38..e86923bc 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -84,9 +84,10 @@ test_expect_success GPG 'verify and show signatures' '
 test_expect_success GPG 'show signed commit with signature' '
git show -s initial commit 
git show -s --show-signature initial show 
-   git verify-commit -v initial verify.1 2verify.2 
+   git verify-commit -v initial verify.1 2verify.2.out 
git cat-file commit initial cat 
-   grep -v gpg:  show show.commit 
+   grep -v -e gpg:  -e insecure memory show show.commit 
+   grep -v insecure memory verify.2.out verify.2 
grep gpg:  show show.gpg 
grep -v ^  cat | grep -v ^gpgsig  cat.commit 
test_cmp show.commit commit 
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html