[PATCH] log: if --decorate is not given, default to --decorate=auto

[Alex Henrie]

Git's branching and merging system can be confusing, especially to new
users. When teaching people Git, I tell them to set log.decorate=auto.
This preference greatly improves the user's awareness of the local and
remote branches. So for the sake of user friendliness, I'd like to
change the default from log.decorate=no to log.decorate=auto.

Signed-off-by: Alex Henrie 
---
 builtin/log.c  | 9 -
 t/t4202-log.sh | 2 +-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 281af8c1e..ddb4515dc 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -52,6 +52,11 @@ struct line_opt_callback_data {
struct string_list args;
 };
 
+static int auto_decoration_style()
+{
+   return (isatty(1) || pager_in_use()) ? DECORATE_SHORT_REFS : 0;
+}
+
 static int parse_decoration_style(const char *var, const char *value)
 {
switch (git_config_maybe_bool(var, value)) {
@@ -67,7 +72,7 @@ static int parse_decoration_style(const char *var, const char 
*value)
else if (!strcmp(value, "short"))
return DECORATE_SHORT_REFS;
else if (!strcmp(value, "auto"))
-   return (isatty(1) || pager_in_use()) ? DECORATE_SHORT_REFS : 0;
+   return auto_decoration_style();
return -1;
 }
 
@@ -405,6 +410,8 @@ static int git_log_config(const char *var, const char 
*value, void *cb)
if (decoration_style < 0)
decoration_style = 0; /* maybe warn? */
return 0;
+   } else {
+   decoration_style = auto_decoration_style();
}
if (!strcmp(var, "log.showroot")) {
default_show_root = git_config_bool(var, value);
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 48b55bfd2..3aa8daba3 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -520,7 +520,7 @@ test_expect_success 'log --graph with merge' '
 '
 
 test_expect_success 'log.decorate configuration' '
-   git log --oneline >expect.none &&
+   git log --oneline --no-decorate >expect.none &&
git log --oneline --decorate >expect.short &&
git log --oneline --decorate=full >expect.full &&
 
-- 
2.12.0

Re: [PATCH/RFC V2] stash: implement builtin stash

[Jeff King]

On Mon, Mar 13, 2017 at 03:23:18PM -0700, Joel Teichroeb wrote:

> I've been working on rewriting git stash as a c builtin and I have all
> but three tests passing. I'm having a bit of trouble fixing them, as
> well as a few other issues, so I'd really appreciate some help. Don't
> bother commenting on the small details yet as I still need to go
> though the code to make sure it matches the code style guidelines.

Be careful that this is a bit of a moving target. There's been some
feature work and some bug-fixes in git-stash.sh the past few weeks.

> git stash uses the GIT_INDEX_FILE environment variable in order to not
> trash the main index. I ended up doing things like this:
> 
> discard_cache();
> ret = read_cache_from(index_path);
> write_index_as_tree(orig_tree.hash, _index, index_path, 0, NULL);
> discard_cache();
> ret = read_cache_from(index_path);
> 
> in order to have an empty cache. Could someone take a look at my uses
> of the index and point out better ways to do it?

I suspect in the long run that you could pull this off without writing
the intermediate index to disk at all. You can store multiple indices if
you need to (read_cache_from is just a wrapper to operate on the_index).
But while you're executing sub-programs, you're still probably going to
need to do a lot of re-reading of the index.

Two things that I think might help break up the work:

  1. Rather than convert stash all at once, you can implement a "stash
 helper" in C that does individual sub-commands (or even smaller
 chunks), and call that from git-stash.sh. Eventually it
 git-stash.sh will just be a skeleton, and you can move that over to
 C and call the functions directly.

  2. You may want to start purely as a C wrapper that calls the
 sub-programs, which communicate with each other via GIT_INDEX_FILE,
 etc. Then you don't need to worry about manipulating the index
 inside the C program at first, and can focus on all the other
 boilerplate.

 Then piece by piece, you can replace sub-program calls with C
 calls. But you know you'll be working on top of a functional base.

I don't know if that's helpful or not.

-Peff

Re: Use base32?

[Michael Steuer]

On 20/03/2017 19:05, Jacob Keller wrote:

On Sun, Mar 19, 2017 at 10:58 PM, Michael Steuer
 wrote:

[..]
If base32 is being considered, I'd suggest the "base32hex" variant, which
uses the same amount of space.

I don't see the benefit of adding characters like 0 and 1 which
conflict with some of the letters? Since there's no need for a human
to decode the base32 output, it's easier to use the one that's less
likely to get screwed up when typing if that ever happens. It's not
like we actually need to know what value each character represents.
(sure the program does, but the human does not).

Thanks,
Jake


Fair enough and good point. We definitely wouldn't want 0 and O and 1 
and I mixed together.


Cheers,
Mike.

Please reply ASAP (For Alex)

[Yohana Rocio Morales Chacon]

Dear Friend, 

I want to introduce my self to you. I am Mr. Alex Lee a banker. I work in a 
bank here in Seoul, Korea. I have been the account officer to most Koreans 
government official till date. I discovered a Dormant account with a lot of 
money. I need your bank account to transfer the money for our mutual benefit. 
For your assistance as the account owner we shall share the funds equally. 

If interested contact me through email for more details. My email address is:

mralexlee7...@gmail.com

Please reply ASAP. 

Mr. Alex Lee.

Re: Why "You cannot combine --squash with --no-ff" in git merge?

[Anatoly Borodin]

Hi All,


I made a mistake in the previous message. I'm really sorry, I need to go
have some sleep right now.

Anatoly Borodin  wrote:
> `git merge --squash --ff` will happily move the A ref to the B commit.
> 
> But what I want, and expect, is:
> 
> A---B'
> 
> where B' has the same tree as B.

`git merge --squash --ff` WILL create A---B'.

But it's still unclear to me why `git merge --squash --no-ff` should
fail instead of doing the same as `git merge --squash --ff` does. The
error message and the documentation do not give any good reasons for it.

PS In my case, I set `merge.ff=false` and run just `git merge --squash`.


-- 
Mit freundlichen Grüßen,
Anatoly Borodin

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

[Jeff King]

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

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

Looks good overall to me. Thanks for following up.

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

>   * 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.

-Peff

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

[Jeff King]

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

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

Sounds logical. The patch looks good to me (though I'll admit my eyes
may have glazed a little in the middle).

-Peff

Re: Safe to use stdatomic.h?

[Eric Wong]

"brian m. carlson"  wrote:
> I could support the argument for ditching RHEL/CentOS 5 support, but I
> expect other people might disagree.  After all, we're still targeting
> C89.

Yeah, I still use and support CentOS 5 in some places (but maybe
not git, still using ancient versions there, too).

Anyways, I'm still relying on the traditional __sync_* builtins
from in earlier gcc 4.x releases in some code GPL-3.0 code I
maintain for older systems:

https://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html

Since git is GPL-2.0, it is license-compatible with all the
atomic macros in the Linux kernel, as well as the kernel-derived
userspace atomics (uatomic) found in liburcu 

Why "You cannot combine --squash with --no-ff" in git merge?

[Anatoly Borodin]

Hi All,

the message "You cannot combine --squash with --no-ff" appeared in 2008,
in the commit 1c7b76be7d620bbaf2e6b8417f04012326bbb9df ("Build in
merge"). I don't understand the logic of this restriction.

Imagine, I have the following branches, where A is a master and B is a
feature branch:

A---B1---B2---B3---B4---B

`git checkout A && git merge --squash --no-ff B` says:

fatal: You cannot combine --squash with --no-ff.

`git merge --squash --ff` will happily move the A ref to the B commit.

But what I want, and expect, is:

A---B'

where B' has the same tree as B.

This can of course be done with `git rebase --interactive` and marking
all commits for squash or fixup. But `git merge --squash` works with
not-fastforwarding feature branch, why should it refuse to work in a
fastforwarding case?

-- 
Mit freundlichen Grüßen,
Anatoly Borodin

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

[Jeff King]

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

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

Good catch.

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

Yeah, I agree that makes the result more obvious to read.

> 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. :)

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

I think the tests just wanted to start at a known state. I agree that
be correct (and would even work with an alternate backend), but it is a
bit subtle that we are relying on the leftover state from the previous
tests.

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

Patch itself looks good.

-Peff

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

[Jeff King]

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

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

Makes sense.

-Peff

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

[Jeff King]

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

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

I think this is an improvement for now. I suspect that all of these will
have to become "test_must_fail git rev-parse --verify $m" in the long
run, when we want to run the test suite against an alternate ref
backend. That can wait, though.

-Peff

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

[Jeff King]

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

> A few tests share their description with another test.  Extend the
> descriptions to indicate how the tests differ.
> 
> Signed-off-by: Kyle Meyer 

Makes sense, and your descriptions look good.

-Peff

[PATCH 6/6] bundle: use prefix_filename with bundle path

[Jeff King]

We may take the path to a bundle file as an argument, and
need to adjust the filename based on the prefix we
discovered while setting up the git directory. We do so
manually into a fixed-size buffer, but using
prefix_filename() is the normal way.

Besides being more concise, there are two subtle
improvements:

  1. The original inserted a "/" between the two paths, even
 though the "prefix" argument always has the "/"
 appended. That means that:

   cd subdir && git bundle verify ../foo.bundle

 was looking at (and reporting) subdir//../foo.bundle.
 Harmless, but ugly.  Using prefix_filename() gets this
 right.

  2. The original checked for an absolute path by looking
 for a leading '/'. It should have been using
 is_absolute_path(), which also covers more cases on
 Windows (backslashes and dos drive prefixes).

 But it's easier still to just pass the name to
 prefix_filename(), which handles this case
 automatically.

Note that we'll just leak the resulting buffer in the name
of simplicity, since it needs to last through the duration
of the program anyway.

Signed-off-by: Jeff King 
---
 builtin/bundle.c | 8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 4883a435a..d0de59b94 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -20,21 +20,15 @@ int cmd_bundle(int argc, const char **argv, const char 
*prefix)
struct bundle_header header;
const char *cmd, *bundle_file;
int bundle_fd = -1;
-   char buffer[PATH_MAX];
 
if (argc < 3)
usage(builtin_bundle_usage);
 
cmd = argv[1];
-   bundle_file = argv[2];
+   bundle_file = prefix_filename(prefix, argv[2]);
argc -= 2;
argv += 2;
 
-   if (prefix && bundle_file[0] != '/') {
-   snprintf(buffer, sizeof(buffer), "%s/%s", prefix, bundle_file);
-   bundle_file = buffer;
-   }
-
memset(, 0, sizeof(header));
if (strcmp(cmd, "create") && (bundle_fd =
read_bundle_header(bundle_file, )) < 0)
-- 
2.12.1.683.gcd02edfec

[PATCH 5/6] prefix_filename: simplify windows #ifdef

[Jeff King]

The prefix_filename function used to do an early return when
there was no prefix on non-Windows platforms, but always
allocated on Windows so that it could call convert_slashes().

Now that the function always allocates, we can unify the
logic and make convert_slashes() the only conditional part.

Signed-off-by: Jeff King 
---
I wondered here if a compiler might complain about the dead assignment
to pfx_len in the is_absolute_path() conditional. On Windows, that's
used as an offset for calling into convert_slashes(), but on other
platforms we never look at it again.

However, neither gcc nor clang complained, so I'm inclined to go with
what I have here.

 abspath.c | 15 ++-
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/abspath.c b/abspath.c
index 4addd1fde..7f1cfe979 100644
--- a/abspath.c
+++ b/abspath.c
@@ -251,18 +251,15 @@ char *prefix_filename(const char *pfx, const char *arg)
struct strbuf path = STRBUF_INIT;
size_t pfx_len = pfx ? strlen(pfx) : 0;
 
-#ifndef GIT_WINDOWS_NATIVE
-   if (!pfx_len || is_absolute_path(arg))
-   return xstrdup(arg);
-   strbuf_add(, pfx, pfx_len);
-   strbuf_addstr(, arg);
-#else
-   /* don't add prefix to absolute paths, but still replace '\' by '/' */
-   if (is_absolute_path(arg))
+   if (!pfx_len)
+   ; /* nothing to prefix */
+   else if (is_absolute_path(arg))
pfx_len = 0;
-   else if (pfx_len)
+   else
strbuf_add(, pfx, pfx_len);
+
strbuf_addstr(, arg);
+#ifdef GIT_WINDOWS_NATIVE
convert_slashes(path.buf + pfx_len);
 #endif
return strbuf_detach(, NULL);
-- 
2.12.1.683.gcd02edfec

[PATCH 4/6] prefix_filename: return newly allocated string

[Jeff King]

The prefix_filename() function returns a pointer to static
storage, which makes it easy to use dangerously. We already
fixed one buggy caller in hash-object recently, and the
calls in apply.c are suspicious (I didn't dig in enough to
confirm that there is a bug, but we call the function once
in apply_all_patches() and then again indirectly from
parse_chunk()).

Let's make it harder to get wrong by allocating the return
value. For simplicity, we'll do this even when the prefix is
empty (and we could just return the original file pointer).
That will cause us to allocate sometimes when we wouldn't
otherwise need to, but this function isn't called in
performance critical code-paths (and it already _might_
allocate on any given call, so a caller that cares about
performance is questionable anyway).

The downside is that the callers need to remember to free()
the result to avoid leaking. Most of them already used
xstrdup() on the result, so we know they are OK. The
remainder have been converted to use free() as appropriate.

I considered retaining a prefix_filename_unsafe() for cases
where we know the static lifetime is OK (and handling the
cleanup is awkward). This is only a handful of cases,
though, and it's not worth the mental energy in worrying
about whether the "unsafe" variant is OK to use in any
situation.

Signed-off-by: Jeff King 
---
 abspath.c | 10 --
 apply.c   |  9 ++---
 builtin/config.c  |  3 +--
 builtin/hash-object.c |  2 +-
 builtin/log.c |  2 +-
 builtin/mailinfo.c| 11 ++-
 builtin/merge-file.c  | 14 +++---
 builtin/rev-parse.c   |  4 +++-
 builtin/worktree.c|  3 ++-
 cache.h   |  6 +++---
 diff-no-index.c   |  2 +-
 diff.c|  6 +++---
 parse-options.c   |  2 +-
 setup.c   | 11 ---
 worktree.c|  5 -
 15 files changed, 51 insertions(+), 39 deletions(-)

diff --git a/abspath.c b/abspath.c
index c6f480993..4addd1fde 100644
--- a/abspath.c
+++ b/abspath.c
@@ -246,20 +246,18 @@ char *absolute_pathdup(const char *path)
return strbuf_detach(, NULL);
 }
 
-const char *prefix_filename(const char *pfx, const char *arg)
+char *prefix_filename(const char *pfx, const char *arg)
 {
-   static struct strbuf path = STRBUF_INIT;
+   struct strbuf path = STRBUF_INIT;
size_t pfx_len = pfx ? strlen(pfx) : 0;
 
 #ifndef GIT_WINDOWS_NATIVE
if (!pfx_len || is_absolute_path(arg))
-   return arg;
-   strbuf_reset();
+   return xstrdup(arg);
strbuf_add(, pfx, pfx_len);
strbuf_addstr(, arg);
 #else
/* don't add prefix to absolute paths, but still replace '\' by '/' */
-   strbuf_reset();
if (is_absolute_path(arg))
pfx_len = 0;
else if (pfx_len)
@@ -267,5 +265,5 @@ const char *prefix_filename(const char *pfx, const char 
*arg)
strbuf_addstr(, arg);
convert_slashes(path.buf + pfx_len);
 #endif
-   return path.buf;
+   return strbuf_detach(, NULL);
 }
diff --git a/apply.c b/apply.c
index b8bd5a4be..e6dbab26a 100644
--- a/apply.c
+++ b/apply.c
@@ -2046,7 +2046,7 @@ static void prefix_one(struct apply_state *state, char 
**name)
char *old_name = *name;
if (!old_name)
return;
-   *name = xstrdup(prefix_filename(state->prefix, *name));
+   *name = prefix_filename(state->prefix, *name);
free(old_name);
 }
 
@@ -4805,6 +4805,7 @@ int apply_all_patches(struct apply_state *state,
 
for (i = 0; i < argc; i++) {
const char *arg = argv[i];
+   char *to_free = NULL;
int fd;
 
if (!strcmp(arg, "-")) {
@@ -4814,19 +4815,21 @@ int apply_all_patches(struct apply_state *state,
errs |= res;
read_stdin = 0;
continue;
-   } else if (0 < state->prefix_length)
-   arg = prefix_filename(state->prefix, arg);
+   } else
+   arg = to_free = prefix_filename(state->prefix, arg);
 
fd = open(arg, O_RDONLY);
if (fd < 0) {
error(_("can't open patch '%s': %s"), arg, 
strerror(errno));
res = -128;
+   free(to_free);
goto end;
}
read_stdin = 0;
set_default_whitespace_mode(state);
res = apply_patch(state, fd, arg, options);
close(fd);
+   free(to_free);
if (res < 0)
goto end;
errs |= res;
diff --git a/builtin/config.c b/builtin/config.c
index 74f6c34d1..4f49a0edb 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -527,8 +527,7 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
else if (given_config_source.file) {
   

Re: [PATCH 00/20] object_id part 7

[brian m. carlson]

On Mon, Mar 20, 2017 at 08:14:20PM +0700, Duy Nguyen wrote:
> On Sun, Mar 19, 2017 at 4:19 AM, brian m. carlson
>  wrote:
> > This is part 7 in the continuing transition to use struct object_id.
> 
> It feels very nice to see many ".hash" and "->hash" go away. Looking
> forward to seeing you kill sha1 in sha1_file.c, object.c and friends.
> That one (and read-cache) must be a battle.

Basically, the next few series look like this:

Part 7 (this one): sha1_array → oid_array
Part 8: lookup_{commit,tree,blob,tag}* and parse_object* [0]
Part 9: get_sha1* → get_oid*
Part 10: refs.c

As I go, I make note of which function calls I tend to modify a lot,
since those tend to be good places to look for future conversions.
sha1_file.c is looking like a good candidate for part 11.

> I'm not familiar with many parts that this series touches so I may
> have missed something subtle, but overall it looks good to me. Thank
> you for working on this.

I think git-contacts picked you because I touched a lot of the shallow
code.

> > I chose not to rename the sha1-array.[ch] files
> 
> You'll have to rename sha1_file.c, sha1_name.c too and might start a
> code reorganization revolution by renaming sha1-array.[ch] ;-)

I tend to prefer dashes over underscores, so I fear other people may not
like my renaming decisions very much.  I think I'm going to stay out of
it and let someone who cares more make the controversial decisions.

Anyway, I've squashed in some changes and will wait another day or two
for other people to send in feedback before v2.

[0] Looking at it now, I can probably handle lookup_object, too, so I
might stuff that in.  That series is already 53 patches, though….
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

[PATCH 3/6] prefix_filename: drop length parameter

[Jeff King]

This function takes the prefix as a ptr/len pair, but in
every caller the length is exactly strlen(ptr). Let's
simplify the interface and just take the string. This saves
callers specifying it (and in some cases handling a NULL
prefix).

In a handful of cases we had the length already without
calling strlen, so this is technically slower. But it's not
likely to matter (after all, if the prefix is non-empty
we'll allocate and copy it into a buffer anyway).

Signed-off-by: Jeff King 
---
 abspath.c | 4 +++-
 apply.c   | 6 ++
 builtin/config.c  | 1 -
 builtin/hash-object.c | 9 +++--
 builtin/log.c | 3 +--
 builtin/mailinfo.c| 2 +-
 builtin/merge-file.c  | 8 ++--
 builtin/rev-parse.c   | 4 +---
 builtin/worktree.c| 2 +-
 cache.h   | 2 +-
 diff-no-index.c   | 7 +++
 diff.c| 4 ++--
 parse-options.c   | 2 +-
 setup.c   | 2 +-
 worktree.c| 2 +-
 15 files changed, 23 insertions(+), 35 deletions(-)

diff --git a/abspath.c b/abspath.c
index fd30aff08..c6f480993 100644
--- a/abspath.c
+++ b/abspath.c
@@ -246,9 +246,11 @@ char *absolute_pathdup(const char *path)
return strbuf_detach(, NULL);
 }
 
-const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
+const char *prefix_filename(const char *pfx, const char *arg)
 {
static struct strbuf path = STRBUF_INIT;
+   size_t pfx_len = pfx ? strlen(pfx) : 0;
+
 #ifndef GIT_WINDOWS_NATIVE
if (!pfx_len || is_absolute_path(arg))
return arg;
diff --git a/apply.c b/apply.c
index 0e2caeab9..b8bd5a4be 100644
--- a/apply.c
+++ b/apply.c
@@ -2046,7 +2046,7 @@ static void prefix_one(struct apply_state *state, char 
**name)
char *old_name = *name;
if (!old_name)
return;
-   *name = xstrdup(prefix_filename(state->prefix, state->prefix_length, 
*name));
+   *name = xstrdup(prefix_filename(state->prefix, *name));
free(old_name);
 }
 
@@ -4815,9 +4815,7 @@ int apply_all_patches(struct apply_state *state,
read_stdin = 0;
continue;
} else if (0 < state->prefix_length)
-   arg = prefix_filename(state->prefix,
- state->prefix_length,
- arg);
+   arg = prefix_filename(state->prefix, arg);
 
fd = open(arg, O_RDONLY);
if (fd < 0) {
diff --git a/builtin/config.c b/builtin/config.c
index 05843a0f9..74f6c34d1 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -528,7 +528,6 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
if (!is_absolute_path(given_config_source.file) && prefix)
given_config_source.file =
xstrdup(prefix_filename(prefix,
-   strlen(prefix),

given_config_source.file));
}
 
diff --git a/builtin/hash-object.c b/builtin/hash-object.c
index 56df77b0c..2ea36909d 100644
--- a/builtin/hash-object.c
+++ b/builtin/hash-object.c
@@ -102,7 +102,6 @@ int cmd_hash_object(int argc, const char **argv, const char 
*prefix)
OPT_END()
};
int i;
-   int prefix_length = -1;
const char *errstr = NULL;
 
argc = parse_options(argc, argv, NULL, hash_object_options,
@@ -113,9 +112,8 @@ int cmd_hash_object(int argc, const char **argv, const char 
*prefix)
else
prefix = setup_git_directory_gently();
 
-   prefix_length = prefix ? strlen(prefix) : 0;
if (vpath && prefix)
-   vpath = xstrdup(prefix_filename(prefix, prefix_length, vpath));
+   vpath = xstrdup(prefix_filename(prefix, vpath));
 
git_config(git_default_config, NULL);
 
@@ -146,9 +144,8 @@ int cmd_hash_object(int argc, const char **argv, const char 
*prefix)
const char *arg = argv[i];
char *to_free = NULL;
 
-   if (0 <= prefix_length)
-   arg = to_free =
-   xstrdup(prefix_filename(prefix, prefix_length, 
arg));
+   if (prefix)
+   arg = to_free = xstrdup(prefix_filename(prefix, arg));
hash_object(arg, type, no_filters ? NULL : vpath ? vpath : arg,
flags, literally);
free(to_free);
diff --git a/builtin/log.c b/builtin/log.c
index 281af8c1e..bfdc7a23d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1084,8 +1084,7 @@ static const char *set_outdir(const char *prefix, const 
char *output_directory)
if (!output_directory)
return prefix;
 
-   return xstrdup(prefix_filename(prefix, outdir_offset,
-  output_directory));
+

[PATCH 1/6] hash-object: fix buffer reuse with --path in a subdirectory

[Jeff King]

The hash-object command uses prefix_filename() without
duplicating its return value. Since that function returns a
static buffer, the value is overwritten by subsequent calls.

This can cause incorrect results when we use --path along
with hashing a file by its relative path, both of which need
to call prefix_filename(). We overwrite the filename
computed for --path, effectively ignoring it.

We can fix this by calling xstrdup on the return value. Note
that we don't bother freeing the "vpath" instance, as it
remains valid until the program exit.

Signed-off-by: Jeff King 
---
 builtin/hash-object.c  |  7 +--
 t/t1007-hash-object.sh | 10 ++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/builtin/hash-object.c b/builtin/hash-object.c
index 9028e1fdc..56df77b0c 100644
--- a/builtin/hash-object.c
+++ b/builtin/hash-object.c
@@ -115,7 +115,7 @@ int cmd_hash_object(int argc, const char **argv, const char 
*prefix)
 
prefix_length = prefix ? strlen(prefix) : 0;
if (vpath && prefix)
-   vpath = prefix_filename(prefix, prefix_length, vpath);
+   vpath = xstrdup(prefix_filename(prefix, prefix_length, vpath));
 
git_config(git_default_config, NULL);
 
@@ -144,11 +144,14 @@ int cmd_hash_object(int argc, const char **argv, const 
char *prefix)
 
for (i = 0 ; i < argc; i++) {
const char *arg = argv[i];
+   char *to_free = NULL;
 
if (0 <= prefix_length)
-   arg = prefix_filename(prefix, prefix_length, arg);
+   arg = to_free =
+   xstrdup(prefix_filename(prefix, prefix_length, 
arg));
hash_object(arg, type, no_filters ? NULL : vpath ? vpath : arg,
flags, literally);
+   free(to_free);
}
 
if (stdin_paths)
diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
index c5245c5cb..532682f51 100755
--- a/t/t1007-hash-object.sh
+++ b/t/t1007-hash-object.sh
@@ -134,6 +134,16 @@ test_expect_success 'gitattributes also work in a 
subdirectory' '
)
 '
 
+test_expect_success '--path works in a subdirectory' '
+   (
+   cd subdir &&
+   path1_sha=$(git hash-object --path=../file1 ../file0) &&
+   path0_sha=$(git hash-object --path=../file0 ../file1) &&
+   test "$file0_sha" = "$path0_sha" &&
+   test "$file1_sha" = "$path1_sha"
+   )
+'
+
 test_expect_success 'check that --no-filters option works' '
nofilters_file1=$(git hash-object --no-filters file1) &&
test "$file0_sha" = "$nofilters_file1" &&
-- 
2.12.1.683.gcd02edfec

[PATCH 2/6] prefix_filename: move docstring to header file

[Jeff King]

This is a public function, so we should make its
documentation available near the declaration.

While we're at it, we can give a few details about how it
works.

Signed-off-by: Jeff King 
---
 abspath.c |  5 -
 cache.h   | 12 
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/abspath.c b/abspath.c
index b02e068aa..fd30aff08 100644
--- a/abspath.c
+++ b/abspath.c
@@ -246,11 +246,6 @@ char *absolute_pathdup(const char *path)
return strbuf_detach(, NULL);
 }
 
-/*
- * Unlike prefix_path, this should be used if the named file does
- * not have to interact with index entry; i.e. name of a random file
- * on the filesystem.
- */
 const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
 {
static struct strbuf path = STRBUF_INIT;
diff --git a/cache.h b/cache.h
index 9b2157f59..a01668fc4 100644
--- a/cache.h
+++ b/cache.h
@@ -529,7 +529,19 @@ extern const char *setup_git_directory_gently(int *);
 extern const char *setup_git_directory(void);
 extern char *prefix_path(const char *prefix, int len, const char *path);
 extern char *prefix_path_gently(const char *prefix, int len, int *remaining, 
const char *path);
+
+/*
+ * Concatenate "prefix" (if len is non-zero) and "path", with no
+ * connecting characters (so "prefix" should end with a "/").
+ * Unlike prefix_path, this should be used if the named file does
+ * not have to interact with index entry; i.e. name of a random file
+ * on the filesystem.
+ *
+ * The return value may point to static storage which will be overwritten by
+ * further calls.
+ */
 extern const char *prefix_filename(const char *prefix, int len, const char 
*path);
+
 extern int check_filename(const char *prefix, const char *name);
 extern void verify_filename(const char *prefix,
const char *name,
-- 
2.12.1.683.gcd02edfec

[PATCH 0/6] prefix_filename cleanups

[Jeff King]

I noticed a spot in builtin/bundle.c that would benefit from using
prefix_filename(). But when I tried to use it, I noticed its interface
was a little error-prone (because it returns a static buffer). And
indeed, a little digging found a bug in hash-object related to this.

So here's the fix for the hash-object bug, some cleanups to make such
bugs less likely, and then finally the bundle conversion. The bundle
thing does fix some minor bugs. It _could_ come before the cleanups if
we wanted to float the fixes to the top, but the function is much more
pleasant to call after the cleanups. :)

  [1/6]: hash-object: fix buffer reuse with --path in a subdirectory
  [2/6]: prefix_filename: move docstring to header file
  [3/6]: prefix_filename: drop length parameter
  [4/6]: prefix_filename: return newly allocated string
  [5/6]: prefix_filename: simplify windows #ifdef
  [6/6]: bundle: use prefix_filename with bundle path

 abspath.c  | 30 +++---
 apply.c| 11 ++-
 builtin/bundle.c   |  8 +---
 builtin/config.c   |  4 +---
 builtin/hash-object.c  | 10 +-
 builtin/log.c  |  3 +--
 builtin/mailinfo.c | 11 ++-
 builtin/merge-file.c   | 18 +++---
 builtin/rev-parse.c|  6 +++---
 builtin/worktree.c |  5 +++--
 cache.h| 14 +-
 diff-no-index.c|  7 +++
 diff.c |  6 +++---
 parse-options.c|  2 +-
 setup.c| 11 ---
 t/t1007-hash-object.sh | 10 ++
 worktree.c |  5 -
 17 files changed, 86 insertions(+), 75 deletions(-)

-Peff

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

[Kyle Meyer]

Signed-off-by: Kyle Meyer 
---
 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 
---
 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 
---

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 
---
 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_success \
"create HEAD" \
@@ -252,10 +253,10 @@ test_expect_success \
 test_expect_success '(not) change HEAD with wrong SHA1' "
test_must_fail git update-ref HEAD $B $Z
 "
-test_expect_success "(not) changed .git/$m" "
-   ! test $B"' = $(cat .git/'"$m"')
+test_expect_success "(not) changed .git/$m" '
+   test_when_finished "rm -f .git/$m" &&
+   ! test $B = $(cat .git/$m)
 '
-rm -f .git/$m
 
 rm -f .git/logs/refs/heads/master
 test_expect_success \
@@ -309,10 +310,10 @@ $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 
1117150200 + Initial Creati
 $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +  Switch

[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 
---
 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 19/20] Rename sha1_array to oid_array

[brian m. carlson]

On Mon, Mar 20, 2017 at 07:25:25PM +0700, Duy Nguyen wrote:
> On Sun, Mar 19, 2017 at 4:19 AM, brian m. carlson
>  wrote:
> > Since this structure handles an array of object IDs, rename it to struct
> > oid_array.  Also rename the accessor functions and the initialization
> > constant.
> >
> > This commit was produced mechanically by providing non-Documentation
> > files to the following Perl one-liners:
> >
> > perl -pi -E 's/struct sha1_array/struct oid_array/g'
> > perl -pi -E 's/\bsha1_array_/oid_array_/g'
> > perl -pi -E 's/SHA1_ARRAY_INIT/OID_ARRAY_INIT/g'
> >
> 
> I see a few multi-line function calls become unaligned because
> oid_array is one character shorter than sha1_array. But I'm ok with
> that, no need to manually align them. We can fix those when we touch
> neighbor code.

That's how I felt about the situation: basically, people would
appreciate the ease of review over the tidiness of the code.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

[PATCH 0/3] git-describe deals gracefully with broken submodules

[Stefan Beller]

Our own version generation in GIT-VERSION-GEN is somewhat sane by testing
if we have a .git dir, and use that as a signal whether the obtained
copy of git was obtained using git (clone/fetch) or if it is just a
downloaded tar ball.

Other scripts to generate a version are not as cautious and just run
"git describe". An error from git-describe is treated as a sufficient
signal to assume it is not a git repository.

When submodules come into play, this is not true, as a submodule
may be damaged instead, such that we're still in a git repository
but error out for the sake of reporting a severly broken submodule.

Add a flag to git-describe that instructs it to treat severe submodule
errors as "dirty" instead of erroring out.

Thanks,
Stefan

Stefan Beller (3):
  submodule.c: port is_submodule_modified to use porcelain 2
  revision machinery: gentle submodule errors
  builtin/describe: introduce --submodule-error-as-dirty flag

 builtin/describe.c   | 17 ++---
 diff-lib.c   | 10 ++-
 diff.h   |  1 +
 diffcore.h   |  3 +-
 revision.c   |  2 +
 submodule.c  | 94 +---
 submodule.h  | 10 ++-
 t/t4060-diff-submodule-option-diff-format.sh | 22 +++
 t/t6120-describe.sh  | 17 +
 9 files changed, 128 insertions(+), 48 deletions(-)

-- 
2.12.0.402.g4b3201c2d6.dirty

[PATCH 2/3] revision machinery: gentle submodule errors

[Stefan Beller]

When a submodule is broken (e.g. its git directory is deleted or is not
pointed to via the .git linking file), commands that deal with submodules
error out reporting the submodule error.

Sometimes users rather want to see the rest of the command succeed,
ignoring the submodule that is severely broken.

Introduce a flag for the revision machinery '--gentle-submodule-errors'
that will ask 'is_submodule_modified' to return the broken flag instead
of dying().

By switching the argument of is_submodule_modified to an unsigned bit
vector, we can signal more flags, such as the gentle bit introduced
in this patch. When this flag is given, avoid dying inside
'is_submodule_modified' for severe errors and return
DIRTY_SUBMODULE_BROKEN instead.

Signed-off-by: Stefan Beller 
---
 diff-lib.c   | 10 --
 diff.h   |  1 +
 diffcore.h   |  3 ++-
 revision.c   |  2 ++
 submodule.c  |  9 -
 submodule.h  | 10 +-
 t/t4060-diff-submodule-option-diff-format.sh | 22 ++
 7 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 52447466b5..61d33b6d26 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -77,8 +77,14 @@ static int match_stat_with_submodule(struct diff_options 
*diffopt,
if (DIFF_OPT_TST(diffopt, IGNORE_SUBMODULES))
changed = 0;
else if (!DIFF_OPT_TST(diffopt, IGNORE_DIRTY_SUBMODULES)
-   && (!changed || DIFF_OPT_TST(diffopt, DIRTY_SUBMODULES)))
-   *dirty_submodule = is_submodule_modified(ce->name, 
DIFF_OPT_TST(diffopt, IGNORE_UNTRACKED_IN_SUBMODULES));
+   && (!changed || DIFF_OPT_TST(diffopt, DIRTY_SUBMODULES))) {
+   unsigned flags = 0;
+   if (DIFF_OPT_TST(diffopt, 
IGNORE_UNTRACKED_IN_SUBMODULES))
+   flags |= IS_SUBMODULE_MODIFIED_IGNORE_UNTRACKED;
+   if (diffopt->gentle_submodule_errors)
+   flags |= IS_SUBMODULE_MODIFIED_GENTLY;
+   *dirty_submodule = is_submodule_modified(ce->name, 
flags);
+   }
diffopt->flags = orig_flags;
}
return changed;
diff --git a/diff.h b/diff.h
index e9ccb38c26..d1e48de43c 100644
--- a/diff.h
+++ b/diff.h
@@ -164,6 +164,7 @@ struct diff_options {
const char *word_regex;
enum diff_words_type word_diff;
enum diff_submodule_format submodule_format;
+   int gentle_submodule_errors;
 
/* this is set by diffcore for DIFF_FORMAT_PATCH */
int found_changes;
diff --git a/diffcore.h b/diffcore.h
index 6230241354..ceef783570 100644
--- a/diffcore.h
+++ b/diffcore.h
@@ -40,9 +40,10 @@ struct diff_filespec {
 #define DIFF_FILE_VALID(spec) (((spec)->mode) != 0)
unsigned should_free : 1; /* data should be free()'ed */
unsigned should_munmap : 1; /* data should be munmap()'ed */
-   unsigned dirty_submodule : 2;  /* For submodules: its work tree is 
dirty */
+   unsigned dirty_submodule : 3;  /* For submodules: its work tree is 
dirty */
 #define DIRTY_SUBMODULE_UNTRACKED 1
 #define DIRTY_SUBMODULE_MODIFIED  2
+#define DIRTY_SUBMODULE_BROKEN 4
unsigned is_stdin : 1;
unsigned has_more_entries : 1; /* only appear in combined diff */
/* data should be considered "binary"; -1 means "don't know yet" */
diff --git a/revision.c b/revision.c
index 7ff61ff5f7..8ae1179b8d 100644
--- a/revision.c
+++ b/revision.c
@@ -2014,6 +2014,8 @@ static int handle_revision_opt(struct rev_info *revs, int 
argc, const char **arg
revs->limited = 1;
} else if (!strcmp(arg, "--ignore-missing")) {
revs->ignore_missing = 1;
+   } else if (!strcmp(arg, "--gentle-submodule-errors")) {
+   revs->diffopt.gentle_submodule_errors = 1;
} else {
int opts = diff_opt_parse(>diffopt, argv, argc, 
revs->prefix);
if (!opts)
diff --git a/submodule.c b/submodule.c
index 81d44cb7e9..d477297fab 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1039,13 +1039,18 @@ int fetch_populated_submodules(const struct argv_array 
*options,
return spf.result;
 }
 
-unsigned is_submodule_modified(const char *path, int ignore_untracked)
+/*
+ * Inspects a submodule and returns its state using flags
+ */
+unsigned is_submodule_modified(const char *path, unsigned flags)
 {
struct child_process cp = CHILD_PROCESS_INIT;
struct strbuf buf = STRBUF_INIT;
unsigned dirty_submodule = 0;
const char *git_dir;
int error_code = 0;
+   int ignore_untracked = flags & IS_SUBMODULE_MODIFIED_IGNORE_UNTRACKED;
+   int gently = flags & IS_SUBMODULE_MODIFIED_GENTLY;
 
   

[PATCH 3/3] builtin/describe: introduce --submodule-error-as-dirty flag

[Stefan Beller]

git-describe tells you the version number you're at, or errors out, e.g.
when you run it outside of a repository, which may happen when downloading
a tar ball instead of using git to obtain the source code.

To keep this property of only erroring out, when not in a repository,
severe submodule errors must be downgraded to reporting them gently
instead of having git-describe error out completely.

This patch helps to fix the root cause in [1], which tries to work around
this situation.

[1] ("Work around git describe bug for build.")
https://gerrit-review.googlesource.com/#/c/99851/

Signed-off-by: Stefan Beller 
---
 builtin/describe.c  | 17 ++---
 t/t6120-describe.sh | 17 +
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 76c18059bf..569fef9ecf 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -31,13 +31,9 @@ static int have_util;
 static struct string_list patterns = STRING_LIST_INIT_NODUP;
 static struct string_list exclude_patterns = STRING_LIST_INIT_NODUP;
 static int always;
+static int gentle_submodule_errors;
 static const char *dirty;
 
-/* diff-index command arguments to check if working tree is dirty. */
-static const char *diff_index_args[] = {
-   "diff-index", "--quiet", "HEAD", "--", NULL
-};
-
 struct commit_name {
struct hashmap_entry entry;
struct object_id peeled;
@@ -442,6 +438,8 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
   N_("do not consider tags matching ")),
OPT_BOOL(0, "always",,
N_("show abbreviated commit object as fallback")),
+   OPT_BOOL(0, "submodule-error-as-dirty", 
_submodule_errors,
+   N_("show abbreviated commit object as fallback")),
{OPTION_STRING, 0, "dirty",  , N_("mark"),
N_("append  on dirty working tree (default: 
\"-dirty\")"),
PARSE_OPT_OPTARG, NULL, (intptr_t) "-dirty"},
@@ -496,6 +494,12 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
if (dirty) {
static struct lock_file index_lock;
int fd;
+   struct argv_array args = ARGV_ARRAY_INIT;
+
+   argv_array_push(, "diff-index");
+   if (gentle_submodule_errors)
+   argv_array_push(, 
"--gentle-submodule-errors");
+   argv_array_pushl(, "--quiet", "HEAD", "--", NULL);
 
read_cache_preload(NULL);
refresh_index(_index, 
REFRESH_QUIET|REFRESH_UNMERGED,
@@ -504,8 +508,7 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
if (0 <= fd)
update_index_if_able(_index, _lock);
 
-   if (!cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1,
-   diff_index_args, prefix))
+   if (!cmd_diff_index(args.argc, args.argv, prefix))
dirty = NULL;
}
describe("HEAD", 1);
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 167491fd5b..99e5ba44b7 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -233,4 +233,21 @@ test_expect_success 'describe --contains and --no-match' '
test_cmp expect actual
 '
 
+test_expect_success 'setup and absorb a submodule' '
+   test_create_repo sub1 &&
+   test_commit -C sub1 initial &&
+   git submodule add ./sub1 &&
+   git submodule absorbgitdirs &&
+   git commit -a -m "add submodule"
+'
+
+test_expect_success 'describe chokes on severly broken submodules' '
+   mv .git/modules/sub1/ .git/modules/sub_moved &&
+   test_must_fail git describe --dirty
+'
+test_expect_success 'describe ignoring a borken submodule' '
+   git describe --dirty --submodule-error-as-dirty >out &&
+   grep dirty out
+'
+
 test_done
-- 
2.12.0.402.g4b3201c2d6.dirty

[PATCH 1/3] submodule.c: port is_submodule_modified to use porcelain 2

[Stefan Beller]

Migrate 'is_submodule_modified' to the new porcelain format of
git-status.

As the old porcelain only reported ' M' for submodules, no
matter what happened inside the submodule (untracked files,
changes to tracked files or move of HEAD), the new API
properly reports the different scenarios.

In a followup patch we will make use of these finer grained
reporting for git-status.

While porting this to the new API, add another extension
point that will get used in the future: When a submodule
is broken (e.g. the .git file pointing to a wrong directory,
not containing a git dir, as fallout of e.g. f8eaa0ba98
(submodule--helper, module_clone: always operate on absolute
paths, 2016-03-31)), we can chose to not die and report it
differently.

Signed-off-by: Stefan Beller 
---
 submodule.c | 85 +++--
 1 file changed, 49 insertions(+), 36 deletions(-)

diff --git a/submodule.c b/submodule.c
index 3200b7bb2b..81d44cb7e9 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1041,67 +1041,80 @@ int fetch_populated_submodules(const struct argv_array 
*options,
 
 unsigned is_submodule_modified(const char *path, int ignore_untracked)
 {
-   ssize_t len;
struct child_process cp = CHILD_PROCESS_INIT;
-   const char *argv[] = {
-   "status",
-   "--porcelain",
-   NULL,
-   NULL,
-   };
struct strbuf buf = STRBUF_INIT;
unsigned dirty_submodule = 0;
-   const char *line, *next_line;
const char *git_dir;
+   int error_code = 0;
 
strbuf_addf(, "%s/.git", path);
-   git_dir = read_gitfile(buf.buf);
-   if (!git_dir)
-   git_dir = buf.buf;
-   if (!is_directory(git_dir)) {
-   strbuf_release();
-   /* The submodule is not checked out, so it is not modified */
-   return 0;
+   git_dir = resolve_gitdir_gently(buf.buf, _code);
 
+   if (!git_dir) {
+   switch (error_code) {
+   case READ_GITFILE_ERR_STAT_FAILED:
+   case READ_GITFILE_ERR_NOT_A_FILE:
+   /* We may have an uninitialized repo here */
+   return 0;
+   default:
+   case READ_GITFILE_ERR_OPEN_FAILED:
+   case READ_GITFILE_ERR_READ_FAILED:
+   case READ_GITFILE_ERR_INVALID_FORMAT:
+   case READ_GITFILE_ERR_NO_PATH:
+   case READ_GITFILE_ERR_NOT_A_REPO:
+   case READ_GITFILE_ERR_TOO_LARGE:
+   /*
+* All these other error codes are indicating
+* a broken submodule. We do not know what is
+* right here. Resolve again triggering die()
+* inside of the parsing.
+*/
+   read_gitfile_gently(buf.buf, NULL);
+   die("BUG: read_gitfile_gently should have died.");
+   }
}
+
strbuf_reset();
 
+   argv_array_pushl(, "status", "--porcelain=2", NULL);
if (ignore_untracked)
-   argv[2] = "-uno";
+   argv_array_push(, "-uno");
 
-   cp.argv = argv;
prepare_submodule_repo_env(_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.out = -1;
cp.dir = path;
if (start_command())
-   die("Could not run 'git status --porcelain' in submodule %s", 
path);
+   die("Could not run 'git status --porcelain=2' in submodule %s", 
path);
 
-   len = strbuf_read(, cp.out, 1024);
-   line = buf.buf;
-   while (len > 2) {
-   if ((line[0] == '?') && (line[1] == '?')) {
+   while (strbuf_getwholeline_fd(, cp.out, '\n') != EOF) {
+   /* regular untracked files */
+   if (buf.buf[0] == '?')
dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
-   if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
-   break;
-   } else {
-   dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
-   if (ignore_untracked ||
-   (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))
-   break;
+
+   /* regular unmerged and renamed files */
+   if (buf.buf[0] == 'u' ||
+   buf.buf[0] == '1' ||
+   buf.buf[0] == '2') {
+   if (buf.buf[5] == 'S') {
+   /* nested submodule handling */
+   if (buf.buf[6] == 'C' || buf.buf[7] == 'M')
+   dirty_submodule |= 
DIRTY_SUBMODULE_MODIFIED;
+   if (buf.buf[8] == 'U')
+   dirty_submodule |= 
DIRTY_SUBMODULE_UNTRACKED;
+   } else
+  

Re: Safe to use stdatomic.h?

[brian m. carlson]

On Mon, Mar 20, 2017 at 04:18:20PM -0400, Ben Peart wrote:
> My college Jeff is working on a patch series to further parallelize the
> loading of the index.  As part of that patch, it would be nice to use the
> atomic_fetch_add function as that would be more efficient than creating a
> mutex simply to protect a variable so that it can be incremented.  I haven't
> seen any use of atomics yet in Git, nor anything that includes
> .
> 
> GCC has supported them since 4.9 and Clang has supported them by default
> since 3.3.  Are there any compilers currently in use by Git that don't
> support these C11 functions?

At work, we're compiling for CentOS 6 and 7.  CentOS 7 only has GCC 4.8,
and CentOS 6 has something much older.  This is code we ship to
customers, so we can't rely on them having devtoolset installed for
newer GCC.

I could support the argument for ditching RHEL/CentOS 5 support, but I
expect other people might disagree.  After all, we're still targeting
C89.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH 06/20] builtin/receive-pack: convert portions to struct object_id

[brian m. carlson]

On Mon, Mar 20, 2017 at 07:56:17PM +0700, Duy Nguyen wrote:
> On Sun, Mar 19, 2017 at 4:19 AM, brian m. carlson
>  wrote:
> > @@ -1489,23 +1489,24 @@ static struct command **queue_command(struct 
> > command **tail,
> >   const char *line,
> >   int linelen)
> >  {
> > -   unsigned char old_sha1[20], new_sha1[20];
> > +   struct object_id old_oid, new_oid;
> > struct command *cmd;
> > const char *refname;
> > int reflen;
> > +   const char *p;
> >
> > -   if (linelen < 83 ||
> > -   line[40] != ' ' ||
> > -   line[81] != ' ' ||
> > -   get_sha1_hex(line, old_sha1) ||
> > -   get_sha1_hex(line + 41, new_sha1))
> > +   if (!linelen ||
> 
> I think you can skip this. The old code needed "< 83" because of the
> random accesses to [40] and [81] but you don't do that anymore.
> parse_oid_hex() can handle empty hex strings fine.

I just realized this looks fishy:

while (boc < eoc) {
const char *eol = memchr(boc, '\n', eoc - boc);
tail = queue_command(tail, boc, eol ? eol - boc : eoc - eol);
boc = eol ? eol + 1 : eoc;
}

If eol is NULL, we subtract it from eoc?  I mean, eol will be zero, so
we get eoc, which seems like what we want.  I think I'm going to send in
a separate patch to fix that, because clearly that's bizarre and not at
all compliant with the C standard.

Going back to linelen, I think it's probably safe to remove, since even
if *boc is a newline (and we get an empty linelen), we're still
guaranteed to have another character, since this is a strbuf.  The
amount of double-checking I had to do there makes me nervous, though.

I'll squash in a change.

> > +   parse_oid_hex(line, _oid, ) ||
> > +   *p++ != ' ' ||
> > +   parse_oid_hex(p, _oid, ) ||
> > +   *p++ != ' ')
> 
> maybe "|| *p)" as well? I think the old code, with "linelen < 83",
> makes sure reflen is at least one. Not sure what FLEX_ALLOC_MEM would
> do if reflen is zero.

I don't think that line is actually guaranteed to be NUL-terminated.  It
may be terminated instead by a newline, such as by
queue_commands_from_cert.

If we did get an empty reflen, we'd call xcalloc, where we will allocate
exactly the size of the struct otherwise.  We'd then try to memcpy zero
bytes into that location, and succeed.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

What's cooking in git.git (Mar 2017, #08; Mon, 20)

[Junio C Hamano]

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

Git 2.12.1 has been tagged with minimum fixups post 2.12; there is a
larger batch being prepared for 2.12.2 to sweep other fixes that have
been in 'master' for issues identified since 2.12 was released.

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

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

--
[Graduated to "master"]

* js/difftool-builtin (2017-03-15) 3 commits
  (merged to 'next' on 2017-03-16 at 3fccb60a07)
 + difftool: handle modified symlinks in dir-diff mode
 + t7800: cleanup cruft left behind by tests
 + t7800: remove whitespace before redirect

 "git difftool --dir-diff" used to die a controlled death giving a
 "fatal" message when encountering a locally modified symbolic link,
 but it started segfaulting since v2.12.  This has been fixed.

--
[New Topics]

* ab/doc-submitting (2017-03-18) 2 commits
 - doc/SubmittingPatches: show how to get a CLI commit summary
 - doc/SubmittingPatches: clarify the casing convention for "area: change..."

 Doc update.

 The example added by the second one may want to be shortened.


* bw/grep-recurse-submodules (2017-03-18) 2 commits
 - grep: fix builds with with no thread support
 - grep: set default output method

 Build fix for NO_PTHREADS build.

 Will merge to 'next'.


* ja/doc-l10n (2017-03-18) 2 commits
 . l10n: Add git-add.txt to localized man pages
 . l10n: Introduce framework for localizing man pages

 A proposal to use po4a to localize our manual pages.


* jk/execv-dashed-external (2017-03-18) 1 commit
  (merged to 'next' on 2017-03-20 at 62119fa314)
 + run-command: fix segfault when cleaning forked async process

 Fix for NO_PTHREADS build.

 Will merge to 'master'.


* js/regexec-buf (2017-03-18) 1 commit
  (merged to 'next' on 2017-03-20 at 7381595eb7)
 + pickaxe: fix segfault with '-S<...> --pickaxe-regex'

 Fix for potential segv introduced in v2.11.0 and later (also
 v2.10.2).

 Will merge to 'master'.


* rs/http-push-cleanup (2017-03-18) 1 commit
  (merged to 'next' on 2017-03-20 at fcf8d30bc0)
 + http-push: don't check return value of lookup_unknown_object()

 Code clean-up.

 Will merge to 'master'.


* rs/path-name-safety-cleanup (2017-03-18) 1 commit
  (merged to 'next' on 2017-03-20 at 78ea574469)
 + revision: remove declaration of path_name()

 Code clean-up.

 Will merge to 'master'.


* rs/shortlog-cleanup (2017-03-18) 1 commit
  (merged to 'next' on 2017-03-20 at a826dff5cf)
 + shortlog: don't set after_subject to an empty string

 Code clean-up.

 Will merge to 'master'.


* rs/update-hook-optim (2017-03-18) 1 commit
  (merged to 'next' on 2017-03-20 at f36ede55be)
 + receive-pack: simplify run_update_post_hook()

 Code clean-up.

 Will merge to 'master'.


* sg/test-with-stdin (2017-03-18) 2 commits
  (merged to 'next' on 2017-03-20 at a66fec5692)
 + tests: make the 'test_pause' helper work in non-verbose mode
 + tests: create an interactive gdb session with the 'debug' helper

 Teach the "debug" helper used in the test framework that allows a
 command to run under "gdb" to make the session interactive.

 Will merge to 'master'.


* ab/doc-no-option-notation-fix (2017-03-20) 1 commit
  (merged to 'next' on 2017-03-20 at a6afe78ab4)
 + doc: change erroneous --[no]-whatever into --[no-]whatever

 Doc fix.

 Will merge to 'master'.


* ab/push-default-doc-fix (2017-03-20) 1 commit
  (merged to 'next' on 2017-03-20 at 0f4d4470de)
 + push: mention "push.default=tracking" in the documentation

 Doc fix.

 Will merge to 'master'.


* nd/commit-hook-doc-fix (2017-03-20) 1 commit
  (merged to 'next' on 2017-03-20 at 7ab46d99f4)
 + git-commit.txt: list post-rewrite in HOOKS section

 Doc fix.

 Will merge to 'master'.


* tg/stash-push-fixup (2017-03-20) 3 commits
 - stash: pass the pathspec argument to git reset
 - stash: make push -p -q --no-keep-index quiet
 - stash: show less information for stash push -- 

 Recent enhancement to "git stash push" command to support pathspec
 to allow only a subset of working tree changes to be stashed away
 was found to be too chatty and exposed the internal implementation
 detail (e.g. when it uses reset to match the index to HEAD before
 doing other things, output from reset seeped out).  These, and
 other chattyness has been fixed.

 Looked alright.
 cf. <20170317145039.dmcb3qyqbzfvt...@sigill.intra.peff.net>

--
[Stalled]

* sg/clone-refspec-from-command-line-config (2017-02-27) 1 commit
 - clone: respect configured fetch respecs during initial fetch

 Expecting a reroll.
 cf. 

Hopefully

[Rita Micheal]

Dear friend,

My name is Mr Micheal Rita, I am the Bill and Exchange (assistant)
Manager of Bank of Africa Ouagadougou, Burkina Faso. In my department
I discovered an abandoned sum of teen million five hundred thousand United
State of American dollars (10.5MILLION USA DOLLARS) in an account that
belongs to one of our foreign customer who died in airline that crashed on 4th
October 2001.

Since I got information about his death I have been expecting his next
of kin to come over and claim his money because we can not release
it unless somebody applies for it as the next of kin or relation to the
deceased as indicated in our banking guidelines, but unfortunately
we learnt that all his supposed next of kin or relation died alongside
with him in the plane crash leaving nobody behind for the claim. It is
therefore upon this discovery that I decided to make this business
proposal to you and release the money to you as next of kin or relation
to the deceased for safety and subsequent disbursement since nobody
is coming for it and I don't want the money to go into the bank treasury
as unclaimed bill.

You will be entitled with 40% of the total sum while 60% will be for
me after which I will visit your Country to invest my own share when
the fund is successfully transferred into your account, Please I would
like you to keep this transaction confidential and as a top secret as
you may wish to know that I am a bank official.

Yours sincerely,
Mr Micheal Rita.

Re: [PATCH 00/20] Separate `ref_cache` into a separate module

[Michael Haggerty]

On 03/20/2017 11:32 PM, Junio C Hamano wrote:
> Michael Haggerty  writes:
> 
>> Michael Haggerty (20):
>>   get_ref_dir(): don't call read_loose_refs() for "refs/bisect"
>>   refs_read_raw_ref(): new function
>>   refs_ref_iterator_begin(): new function
>>   refs_verify_refname_available(): implement once for all backends
>>   refs_verify_refname_available(): use function in more places
>>   Rename `add_ref()` to `add_ref_entry()`
>>   Rename `find_ref()` to `find_ref_entry()`
>>   Rename `remove_entry()` to `remove_entry_from_dir()`
>>   refs: split `ref_cache` code into separate files
>>   ref-cache: introduce a new type, ref_cache
>>   refs: record the ref_store in ref_cache, not ref_dir
>>   ref-cache: use a callback function to fill the cache
>>   refs: handle "refs/bisect/" in `loose_fill_ref_dir()`
>>   do_for_each_entry_in_dir(): eliminate `offset` argument
>>   get_loose_ref_dir(): function renamed from get_loose_refs()
>>   get_loose_ref_cache(): new function
>>   cache_ref_iterator_begin(): make function smarter
>>   commit_packed_refs(): use reference iteration
>>   files_pack_refs(): use reference iteration
>>   do_for_each_entry_in_dir(): delete function
> 
> These mostly seem to be ref-cache but there is one mention of "refs
> cache", and also the topic cover says ref_cache.  Which one is the
> canonical one?  I'd assume it is "ref-cache" (or "ref cache").
> 
> The reason I am asking is because I wanted to give the three
> "Rename" ones an ":" prefix, and then noticed that the
> shortlog output looked somewhat incoherent.

It's meant to be "ref-cache", short for "reference cache", which seems
more natural than "references cache". But I admit that I often type it
wrong because of muscle memory trained on other things named "refs_blah".

Thanks for noticing.

Michael

Re: [PATCH 09/20] refs: split `ref_cache` code into separate files

[Junio C Hamano]

Stefan Beller  writes:

> On Mon, Mar 20, 2017 at 12:47 PM, Junio C Hamano  wrote:
>> Jeff King  writes:
>>
>>> It might have been a bit easier to review as separate steps for that
>>> reason, but I doubt it's worth going back and re-doing. I'll take your
>>> word that nothing substantive changed.
>>
>> In such a case when I do not want to "take your word", I often do
>> "blame -C" on the result, setting the bottom of the range close-by.
>> I should mostly see the ^bottom lines (or with "blame -b", lines
>> intended with blanks without any commit object name).
>
> FYI:
> I still want to revive the series that colors moved text differently.

I agree that the mode would help viewing this change with "git
show".

Re: [PATCH 11/20] refs: record the ref_store in ref_cache, not ref_dir

[Michael Haggerty]

On 03/20/2017 06:51 PM, Jeff King wrote:
> On Mon, Mar 20, 2017 at 05:33:16PM +0100, Michael Haggerty wrote:
> 
>> Instead of keeping a pointer to the ref_store in every ref_dir entry,
>> store it once in `struct ref_cache`, and change `struct ref_dir` to
>> include a pointer to its containing `ref_cache` instead. This makes it
>> easier to add to the information that is stored in `struct ref_cache`
>> without inflating the size of the individual entries.
> 
> This last sentence confused me. It's a pointer either way, no?
> 
> Do you just mean that we are free to add whatever we like to the
> "ref_cache" without polluting the "ref_store" that is a more public data
> structure?

Yeah, that's explained poorly. It might be clearer as

> This makes it easier to add to the information that is accessible
> from a `ref_dir` without increasing the size of every `ref_dir`
> instance.

It used to be that `ref_dir` contained a pointer to the `ref_store` that
contains it. (BTW, such a pointer *can't* be turned into a pointer to
the `ref_cache` because (1) the `ref_dir` shouldn't have to know what
kind of `ref_store` it is being used with, and (2) a `packed_ref_cache`
can be detached from its old `ref_cache` if the stat info indicates that
the `packed-refs` file has been modified since it was last read.)

But we want to add a `fill_ref_dir` callback pointer in a place that is
reachable from the `ref_dir` so that it can fill itself when necessary.
We could add it to the `ref_dir` struct, but that would inflate its
size. Instead, it makes more sense for the `ref_dir` to have a pointer
to the enclosing `ref_cache`, and store the `fill_ref_dir` pointer in
`ref_cache`. The `ref_cache` also gets a pointer to the enclosing
`ref_store`; that way a `ref_dir` also has a way to access the
`ref_store` that contains it.

An alternative would be to pass a pointer to the `ref_cache` down the
call stack when it is being accessed, but that seemed like a lot of work
that wouldn't lead to a very clean result.

Michael

Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands

[Brandon Williams]

On 03/17, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > I don't think that prefix can ever have ".." in it.  From what I
> > understand it is always a path from the root of the repository to the
> > cwd that the git command was invoked by.  So in your example prefix
> > would be "src/".
> 
> The prefix would be NULL or "", as you will be at the root-level of
> the working tree when you are running _IN_ the submodule (by
> recursing into it).  Not src/, nor anything with ../ in it, I would
> think.

Yes, the prefix that is found during setup of a submodule process would
be NULL or "" as the command would be invoked from the root of that
repository.  This series would sort of change that though.

If a command was invoked from 'src/' with a pathspec of '../dir/' and
there is a submodule at 'dir/sub', the process working on the submodule
will have the following:

super_prefix = 'dir/sub/'
prefix = 'src/' (Passed from the parent process via the
 GIT_INTERNAL_TOPLEVEL_PREFIX env var)
pathspec = '../dir/'

With that information the child process will be able to properly resolve
the pathspec to be 'dir/' (using the prefix) and will be able to match
against it by pre-pending the super_prefix (e.g. dir/sub/some/file, where
some/file is a file in the submodule).  It will also be able to generate
correct output relative to the directory the command was originally
invoked from by first pre-pending the super_prefix so we have
'dir/sub/some/file' and then calling relative_path() with the prefix
that was passed in such that the output for this file looks like
'../dir/sub/some/file'

That the gist of how I'm hoping to solve the problem.  Hopefully that was
clear enough to get some feedback on.

-- 
Brandon Williams

Re: [PATCH 10/20] sha1_name: convert struct disambiguate_state to object_id

[brian m. carlson]

On Mon, Mar 20, 2017 at 08:07:09PM +0700, Duy Nguyen wrote:
> On Sun, Mar 19, 2017 at 4:19 AM, brian m. carlson
>  wrote:
> > @@ -332,7 +332,7 @@ static int init_object_disambiguation(const char *name, 
> > int len,
> > ds->hex_pfx[i] = c;
> > if (!(i & 1))
> > val <<= 4;
> > -   ds->bin_pfx[i >> 1] |= val;
> > +   ds->bin_pfx.hash[i >> 1] |= val;
> 
> The indexing makes me a bit nervous, especially since diff context
> here is too narrow to see. It would be nice if this code (at the
> beginning of init_object_disambiguation) is converted here too
> 
> if (len < MINIMUM_ABBREV || len > GIT_SHA1_HEXSZ)
> return -1;

Well, I think that's the way I would have written that text at the top
of the function.  I expect that we'll end up turning GIT_SHA1_HEXSZ into
a global named something like current_hash_len via global
search-and-replace, so it will always be the right length.

The indexing should be safe because len is guaranteed to be sufficiently
small, and I feel like it we would have seen it break by now if it had
had an overflow.  i will always be in the range [0, 40) (for SHA-1), so
i >> 1 should always be in [0, 20).

Am I understanding you correctly and if so, does that assuage your
concerns, or did you mean something else?
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH 00/20] Separate `ref_cache` into a separate module

[Junio C Hamano]

Michael Haggerty  writes:

> Michael Haggerty (20):
>   get_ref_dir(): don't call read_loose_refs() for "refs/bisect"
>   refs_read_raw_ref(): new function
>   refs_ref_iterator_begin(): new function
>   refs_verify_refname_available(): implement once for all backends
>   refs_verify_refname_available(): use function in more places
>   Rename `add_ref()` to `add_ref_entry()`
>   Rename `find_ref()` to `find_ref_entry()`
>   Rename `remove_entry()` to `remove_entry_from_dir()`
>   refs: split `ref_cache` code into separate files
>   ref-cache: introduce a new type, ref_cache
>   refs: record the ref_store in ref_cache, not ref_dir
>   ref-cache: use a callback function to fill the cache
>   refs: handle "refs/bisect/" in `loose_fill_ref_dir()`
>   do_for_each_entry_in_dir(): eliminate `offset` argument
>   get_loose_ref_dir(): function renamed from get_loose_refs()
>   get_loose_ref_cache(): new function
>   cache_ref_iterator_begin(): make function smarter
>   commit_packed_refs(): use reference iteration
>   files_pack_refs(): use reference iteration
>   do_for_each_entry_in_dir(): delete function

These mostly seem to be ref-cache but there is one mention of "refs
cache", and also the topic cover says ref_cache.  Which one is the
canonical one?  I'd assume it is "ref-cache" (or "ref cache").

The reason I am asking is because I wanted to give the three
"Rename" ones an ":" prefix, and then noticed that the
shortlog output looked somewhat incoherent.

Thanks.

Re: [PATCH 04/20] refs_verify_refname_available(): implement once for all backends

[Michael Haggerty]

On 03/20/2017 06:42 PM, Jeff King wrote:
> On Mon, Mar 20, 2017 at 05:33:09PM +0100, Michael Haggerty wrote:
> 
>> It turns out that we can now implement
>> `refs_verify_refname_available()` based on the other virtual
>> functions, so there is no need for it to be defined at the backend
>> level. Instead, define it once in `refs.c` and remove the
>> `files_backend` definition.
> 
> Does this mean that backends can no longer provide storage for
> D/F-conflicted refnames (i.e., "refs/foo" and "refs/foo/bar")? It looks
> like the global verify_refname_available() has that logic baked in.

The `verify_refname_available()` function implements the "no D/F
conflict" policy. But it's called from the backends, not from the common
code, and nobody says that a backend needs to call the function.

> [...]
>>  int refs_verify_refname_available(struct ref_store *refs,
>>const char *refname,
>> -  const struct string_list *extra,
>> +  const struct string_list *extras,
>>const struct string_list *skip,
>>struct strbuf *err)
>>  {
>> [...]
>> +/*
>> + * We are still at a leading dir of the refname (e.g.,
>> + * "refs/foo"; if there is a reference with that name,
>> + * it is a conflict, *unless* it is in skip.
>> + */
>> +if (skip && string_list_has_string(skip, dirname.buf))
>> +continue;
>> +
>> +if (!refs_read_raw_ref(refs, dirname.buf, oid.hash, , 
>> )) {
>> +strbuf_addf(err, "'%s' exists; cannot create '%s'",
>> +dirname.buf, refname);
>> +goto cleanup;
>> +}
> 
> We don't really care about reading the ref value here; we just care if
> it exists. Does that matter for efficiency (e.g., for the files backend
> it's a stat() versus an open/read/close)? I guess the difference only
> matters when it _does_ exist, which is the uncommon error case.

Yes, I assume that the conflict cases are unusual so I wasn't worrying
too much about their performance.

Not quite so obvious is that the new code sometimes checks for conflicts
against both loose and packed references in situations where the old
code only checked against packed references. Specifically, this happens
when the code has just read, or locked, or failed to find a loose
reference, so it could infer that there are no loose references that
could conflict. I don't think that will be noticeable, because it is the
reading of the whole `packed-refs` file that is a big expensive
operation that it is important to avoid. Anyway, later patches (i.e.,
after there is a `packed_ref_store`) can switch back to checking only
packed references and get back the old optimization.

> (Also, I suspect the loose ref cache always just reads everything in the
> current code, though with the iterator approach in theory we could stop
> doing that).

The loose ref cache reads directories into the cache lazily,
breadth-first. So it reads all of the entries in the directories leading
to the reference being looked up, but no others. When iterating, it
reads the parent directories of the prefix that is being iterated over
plus the whole subtree under the prefix, and that's it (though this
optimization is not yet wired through to `git for-each-ref`).

Michael

Re: [PATCH] Correct compile errors when DEBUG_BISECT=1 after supporting other hash algorithms

[brian m. carlson]

On Mon, Mar 20, 2017 at 11:11:17AM -0700, Stefan Beller wrote:
> +cc Brian
> 
> On Sun, Mar 19, 2017 at 2:33 PM, Alex Hoffman  wrote:
> > ---
> >  bisect.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/bisect.c b/bisect.c
> > index 30808cadf..6feed8533 100644
> > --- a/bisect.c
> > +++ b/bisect.c
> > @@ -131,7 +131,7 @@ static void show_list(const char *debug, int
> > counted, int nr,
> > unsigned flags = commit->object.flags;
> > enum object_type type;
> > unsigned long size;
> > -   char *buf = read_sha1_file(commit->object.sha1, , 
> > );
> > +   char *buf = read_sha1_file(commit->object.oid.hash,
> > , );
> > const char *subject_start;
> > int subject_len;
> >
> > @@ -143,10 +143,10 @@ static void show_list(const char *debug, int
> > counted, int nr,
> > fprintf(stderr, "%3d", weight(p));
> > else
> > fprintf(stderr, "---");
> > -   fprintf(stderr, " %.*s", 8, 
> > sha1_to_hex(commit->object.sha1));
> > +   fprintf(stderr, " %.*s", 8,
> > sha1_to_hex(commit->object.oid.hash));

I think here we want to write

fprintf(stderr, " %.*s", 8, oid_to_hex(>object.oid));

> > for (pp = commit->parents; pp; pp = pp->next)
> > fprintf(stderr, " %.*s", 8,
> > -   sha1_to_hex(pp->item->object.sha1));
> > +   sha1_to_hex(pp->item->object.oid.hash));

And here, as well.

> >
> > subject_len = find_commit_subject(buf, _start);
> > if (subject_len)

Otherwise, I think this looks good.

I'm sorry I didn't catch this earlier, but I didn't even realize we had
this option, or I would have tested it.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH v3 2/2] l10n: Add git-add.txt to localized man pages

[Junio C Hamano]

Jean-Noel Avila  writes:

> Signed-off-by: Jean-Noel Avila 
> ---
>  Documentation/po/documentation.fr.po | 1095 
> ++
>  Documentation/po/documentation.pot   |  787 
>  2 files changed, 1882 insertions(+)
>  create mode 100644 Documentation/po/documentation.fr.po
>  create mode 100644 Documentation/po/documentation.pot

This sounds more like

Subject: l10n: add fr localization for git-add manual pages

to me.  The actual part of this patch that adds "git-add" is the
addition of Documentation/po/documentation.pot, and from that point
of view, this patch may want to be further split into two.

But more importantly, aren't we essentially adding an equivalent of

cd Documentation && cat git-*.txt

to our codebase?

Surely we cannot avoid having a copy of all messages that are to be
translated using msgid/msgstr based approach, and we already do so
for end-user-facing in-program strings, but it just feels a bit too
much having to carry a duplicate (and slightly a stale) copy of the
entire documentation set around.  For N languages, we'll have an
equivalent for N copies of the English text, in addition to the
translated documentation.

I am wondering if Documentation/po part should be a separate
repository, with a dedicated i18n/l10n coordinator.  Would it make
it easier for (1) those who write code and doc without knowing other
languages, (2) those who update .pot and coordinate the l10n effort
for the documentation and (3) those who translate them if we keep
them in a single repository?

Re: Add configuration options for some commonly used command-line options

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 20, 2017 at 7:56 PM, Junio C Hamano  wrote:
> One thing we may want to consider is why we have to even worry about
> scripts getting broken.  It is because people script around
> Porcelain, and that is because we have been too eager to improve
> Porcelain while neglecting plumbing for too long, to the point that
> some things are only doable with Porcelain (or doing the same with
> plumbing while possible are made too cumbersome).  I find it quite
> disturbing that nobody brought that up as an issue that needs to be
> addressed in this entire thread.

I very much doubt this describes anything but a tiny number of cases
where people are using the porcelain as an API.

People aren't going through the process of trying to find out how to
do something with a plumbing command, and then failing and falling
back to a porcelain command because the plumbing isn't complete
enough. They just use the porcelain because they're familiar with it
and scripting it works for them.

E.g. I just looked at both major Emacs modes for git now, magit &
vc-git, neither use "mktag", they just shell out to "git tag" for
making tags. I just went to the git-scm.com website and looked at one
open source GUI client I could "git clone", Giggle. It just shells out
to e.g. "git commit" to make commits, not "git commit-tree". The other
commands they're using are porcelain too. If they've used some
plumbing it's probably by sheer accident. E.g. they use ls-tree which
is plumbing, but don't use for-each-ref.

What's that Google SRE-ism again? Something like "People use the
reliability you provide them with in practice, not what you
advertise". Our porcelain is very stable, and so people use it as a
stable API, and not just for trivial scripts.

Which I think has some big implications for how we maintain the
porcelain & plumbing. Since people *will* use the porcelain, probably
no matter what we advertise to them or how good the plumbing is.

Re: [ANNOUNCE] Git v2.12.1

[Junio C Hamano]

Junio C Hamano  writes:

> The tarballs are NOT YET found at:
>
> https://www.kernel.org/pub/software/scm/git/
>
> but hopefully will be in a few days (I am having trouble reaching
> there).

Now they are.

Thanks.

A note from the maintainer

[Junio C Hamano]

Welcome to the Git development community.

This message is written by the maintainer and talks about how Git
project is managed, and how you can work with it.

* Mailing list and the community

The development is primarily done on the Git mailing list. Help
requests, feature proposals, bug reports and patches should be sent to
the list address .  You don't have to be
subscribed to send messages.  The convention on the list is to keep
everybody involved on Cc:, so it is unnecessary to say "Please Cc: me,
I am not subscribed".

Before sending patches, please read Documentation/SubmittingPatches
and Documentation/CodingGuidelines to familiarize yourself with the
project convention.

If you sent a patch and you did not hear any response from anybody for
several days, it could be that your patch was totally uninteresting,
but it also is possible that it was simply lost in the noise.  Please
do not hesitate to send a reminder message in such a case.  Messages
getting lost in the noise may be a sign that those who can evaluate
your patch don't have enough mental/time bandwidth to process them
right at the moment, and it often helps to wait until the list traffic
becomes calmer before sending such a reminder.

The list archive is available at a few public sites:

http://public-inbox.org/git/
http://marc.info/?l=git
http://www.spinics.net/lists/git/

For those who prefer to read it over NNTP:

nntp://news.public-inbox.org/inbox.comp.version-control.git
nntp://news.gmane.org/gmane.comp.version-control.git

are available.

When you point at a message in a mailing list archive, using its
message ID is often the most robust (if not very friendly) way to do
so, like this:


http://public-inbox.org/git/pine.lnx.4.58.0504150753440.7...@ppc970.osdl.org

Often these web interfaces accept the message ID with enclosing <>
stripped (like the above example to point at one of the most important
message in the Git list).

Some members of the development community can sometimes be found on
the #git and #git-devel IRC channels on Freenode.  Their logs are
available at:

http://colabti.org/irclogger/irclogger_log/git
http://colabti.org/irclogger/irclogger_log/git-devel

There is a volunteer-run newsletter to serve our community ("Git Rev
News" http://git.github.io/rev_news/rev_news.html).

Git is a member project of software freedom conservancy, a non-profit
organization (https://sfconservancy.org/).  To reach a committee of
liaisons to the conservancy, contact them at .


* Reporting bugs

When you think git does not behave as you expect, please do not stop
your bug report with just "git does not work".  "I used git in this
way, but it did not work" is not much better, neither is "I used git
in this way, and X happend, which is broken".  It often is that git is
correct to cause X happen in such a case, and it is your expectation
that is broken. People would not know what other result Y you expected
to see instead of X, if you left it unsaid.

Please remember to always state

 - what you wanted to achieve;

 - what you did (the version of git and the command sequence to reproduce
   the behavior);

 - what you saw happen (X above);

 - what you expected to see (Y above); and

 - how the last two are different.

See http://www.chiark.greenend.org.uk/~sgtatham/bugs.html for further
hints.

If you think you found a security-sensitive issue and want to disclose
it to us without announcing it to wider public, please contact us at
our security mailing list .  This is
a closed list that is limited to people who need to know early about
vulnerabilities, including:

  - people triaging and fixing reported vulnerabilities
  - people operating major git hosting sites with many users
  - people packaging and distributing git to large numbers of people

where these issues are discussed without risk of the information
leaking out before we're ready to make public announcements.


* Repositories and documentation.

My public git.git repositories are at:

  git://git.kernel.org/pub/scm/git/git.git/
  https://kernel.googlesource.com/pub/scm/git/git
  git://repo.or.cz/alt-git.git/
  https://github.com/git/git/
  git://git.sourceforge.jp/gitroot/git-core/git.git/
  git://git-core.git.sourceforge.net/gitroot/git-core/git-core/

This one shows not just the main integration branches, but also
individual topics broken out:

  git://github.com/gitster/git/

A few web interfaces are found at:

  http://git.kernel.org/cgit/git/git.git
  https://kernel.googlesource.com/pub/scm/git/git
  http://repo.or.cz/w/alt-git.git

Preformatted documentation from the tip of the "master" branch can be
found in:

  git://git.kernel.org/pub/scm/git/git-{htmldocs,manpages}.git/
  git://repo.or.cz/git-{htmldocs,manpages}.git/
  https://github.com/gitster/git-{htmldocs,manpages}.git/

The manual pages formatted in HTML for the tip of 

[ANNOUNCE] Git v2.12.1

[Junio C Hamano]

The latest maintenance release Git v2.12.1 is now available at
the usual places.

The tarballs are NOT YET found at:

https://www.kernel.org/pub/software/scm/git/

but hopefully will be in a few days (I am having trouble reaching
there).

The following public repositories all have a copy of the 'v2.12.1'
tag and the 'maint' branch that the tag points at:

  url = https://kernel.googlesource.com/pub/scm/git/git
  url = git://repo.or.cz/alt-git.git
  url = git://git.sourceforge.jp/gitroot/git-core/git.git
  url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core
  url = https://github.com/gitster/git



Git v2.12.1 Release Notes
=

Fixes since v2.12
-

 * Reduce authentication round-trip over HTTP when the server supports
   just a single authentication method.  This also improves the
   behaviour when Git is misconfigured to enable http.emptyAuth
   against a server that does not authenticate without a username
   (i.e. not using Kerberos etc., which makes http.emptyAuth
   pointless).

 * Windows port wants to use OpenSSL's implementation of SHA-1
   routines, so let them.

 * Add 32-bit Linux variant to the set of platforms to be tested with
   Travis CI.

 * When a redirected http transport gets an error during the
   redirected request, we ignored the error we got from the server,
   and ended up giving a not-so-useful error message.

 * The patch subcommand of "git add -i" was meant to have paths
   selection prompt just like other subcommand, unlike "git add -p"
   directly jumps to hunk selection.  Recently, this was broken and
   "add -i" lost the paths selection dialog, but it now has been
   fixed.

 * Git v2.12 was shipped with an embarrassing breakage where various
   operations that verify paths given from the user stopped dying when
   seeing an issue, and instead later triggering segfault.

 * The code to parse "git log -L..." command line was buggy when there
   are many ranges specified with -L; overrun of the allocated buffer
   has been fixed.

 * The command-line parsing of "git log -L" copied internal data
   structures using incorrect size on ILP32 systems.

Also contains various documentation updates and code clean-ups.



Changes since v2.12.0 are as follows:

Allan Xavier (1):
  line-log.c: prevent crash during union of too many ranges

Jeff Hostetler (1):
  mingw: use OpenSSL's SHA-1 routines

Jeff King (3):
  http: restrict auth methods to what the server advertises
  http: add an "auto" mode for http.emptyauth
  add--interactive: fix missing file prompt for patch mode with "-i"

Johannes Schindelin (3):
  Travis: also test on 32-bit Linux
  t1501: demonstrate NULL pointer access with invalid GIT_WORK_TREE
  real_pathdup(): fix callsites that wanted it to die on error

Jonathan Tan (1):
  http: attempt updating base URL only if no error

Junio C Hamano (2):
  Preparing for 2.12.1
  Git 2.12.1

Maxim Moseychuk (2):
  stop_progress_msg: convert xsnprintf to xstrfmt
  bisect_next_all: convert xsnprintf to xstrfmt

Vegard Nossum (1):
  line-log: use COPY_ARRAY to fix mis-sized memcpy

Rework manpage localisation

[Jean-Noel Avila]

The patch series has been reworked to remove the manpage localization
task from the main documentation task. The Travis build should be back
to normal, while still retaining the capability to generate localized
man pages.

Right now, there are some limitations with the version of po4a
provided by most linux distributions, so there is a great chance that
the install-man-l10n won't work on most systems.

[PATCH v3 1/2] l10n: Introduce framework for localizing man pages

[Jean-Noel Avila]

Providing git in localized version is a good step for general adoption
of the tool. But as of now, if one needs to refer to the manual pages,
they are still confronted to english. The aim is to provide
documentation to users in their own language.

The translation of the source asciidoc files is managed via po4a
driven by the conf file Documentation/po4a.conf.

Only the manpages are generated and installed by using the
`install-man-l10n` target of the Makefile. The localized manpages for
all the translated languages are installed in the man path in their
own language folder, so that they can be accessed by man.

Signed-off-by: Jean-Noel Avila 
---
 Documentation/Makefile  | 21 -
 Documentation/po4a.conf |  5 +
 2 files changed, 25 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/po4a.conf

diff --git a/Documentation/Makefile b/Documentation/Makefile
index b5be2e2d3..e721c7149 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -1,4 +1,5 @@
 # Guard against environment variables
+MAN1_L10N_TXT =
 MAN1_TXT =
 MAN5_TXT =
 MAN7_TXT =
@@ -10,6 +11,7 @@ OBSOLETE_HTML =
 MAN1_TXT += $(filter-out \
$(addsuffix .txt, $(ARTICLES) $(SP_ARTICLES)), \
$(wildcard git-*.txt))
+MAN1_L10N_TXT += $(wildcard po/*/man1/git-*.txt)
 MAN1_TXT += git.txt
 MAN1_TXT += gitk.txt
 MAN1_TXT += gitremote-helpers.txt
@@ -86,6 +88,7 @@ DOC_HTML += $(patsubst %,%.html,$(ARTICLES) $(SP_ARTICLES))
 DOC_MAN1 = $(patsubst %.txt,%.1,$(MAN1_TXT))
 DOC_MAN5 = $(patsubst %.txt,%.5,$(MAN5_TXT))
 DOC_MAN7 = $(patsubst %.txt,%.7,$(MAN7_TXT))
+DOC_MAN1_L10N = $(patsubst %.txt,%.1,$(MAN1_L10N_TXT))
 
 prefix ?= $(HOME)
 bindir ?= $(prefix)/bin
@@ -209,6 +212,7 @@ endif
 
 ifneq ($(findstring $(MAKEFLAGS),s),s)
 ifndef V
+   QUIET_PO4A  = @echo '   ' PO4A $@;
QUIET_ASCIIDOC  = @echo '   ' ASCIIDOC $@;
QUIET_XMLTO = @echo '   ' XMLTO $@;
QUIET_DB2TEXI   = @echo '   ' DB2TEXI $@;
@@ -234,6 +238,15 @@ man1: $(DOC_MAN1)
 man5: $(DOC_MAN5)
 man7: $(DOC_MAN7)
 
+man_l10n: po4a man1_p_l10n
+po4a: po4a.conf
+   $(QUIET_PO4A)po4a po4a.conf
+
+man1_p_l10n: po4a
+   $(MAKE) man1_l10n
+
+man1_l10n: $(DOC_MAN1_L10N)
+
 info: git.info gitman.info
 
 pdf: user-manual.pdf
@@ -247,6 +260,11 @@ install-man: man
$(INSTALL) -m 644 $(DOC_MAN1) $(DESTDIR)$(man1dir)
$(INSTALL) -m 644 $(DOC_MAN5) $(DESTDIR)$(man5dir)
$(INSTALL) -m 644 $(DOC_MAN7) $(DESTDIR)$(man7dir)
+   $(MAKE) install-man-l10n
+
+install-man-l10n: $(DOC_MAN1_L10N)
+   $(INSTALL) -d -m 755 $(DESTDIR)$(mandir)/$(firstword $(subst /man1/, 
,$(subst po/,,$<)))/man1
+   $(INSTALL) -m 644 $< $(DESTDIR)$(mandir)$(subst po,,$<)
 
 install-info: info
$(INSTALL) -d -m 755 $(DESTDIR)$(infodir)
@@ -323,6 +341,7 @@ clean:
$(RM) technical/*.html technical/api-index.txt
$(RM) $(cmds_txt) $(mergetools_txt) *.made
$(RM) manpage-base-url.xsl
+   $(RM) po/*/*.1 po/*/*.txt
 
 $(MAN_HTML): %.html : %.txt asciidoc.conf
$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
@@ -339,7 +358,7 @@ manpage-base-url.xsl: manpage-base-url.xsl.in
 
 %.1 %.5 %.7 : %.xml manpage-base-url.xsl
$(QUIET_XMLTO)$(RM) $@ && \
-   $(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
+   $(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) -o $(dir $<) man $<
 
 %.xml : %.txt asciidoc.conf
$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
diff --git a/Documentation/po4a.conf b/Documentation/po4a.conf
new file mode 100644
index 0..b6ee8b4a6
--- /dev/null
+++ b/Documentation/po4a.conf
@@ -0,0 +1,5 @@
+[po4a_langs] fr
+[po4a_paths] po/documentation.pot $lang:po/documentation.$lang.po
+[options] opt: " -k 80"
+
+[type: asciidoc] ./git-add.txt $lang:./po/$lang/man1/git-add.txt
-- 
2.12.0

[PATCH v3 2/2] l10n: Add git-add.txt to localized man pages

[Jean-Noel Avila]

Signed-off-by: Jean-Noel Avila 
---
 Documentation/po/documentation.fr.po | 1095 ++
 Documentation/po/documentation.pot   |  787 
 2 files changed, 1882 insertions(+)
 create mode 100644 Documentation/po/documentation.fr.po
 create mode 100644 Documentation/po/documentation.pot

diff --git a/Documentation/po/documentation.fr.po 
b/Documentation/po/documentation.fr.po
new file mode 100644
index 0..3017da0c9
--- /dev/null
+++ b/Documentation/po/documentation.fr.po
@@ -0,0 +1,1095 @@
+# French translations for Git Manual Pages.
+# Copyright (C) 2017 Jean-Noël Avila 
+# This file is distributed under the same license as the Git package.
+# Jean-Noël Avila , 2016.
+msgid ""
+msgstr ""
+"Project-Id-Version: git documentation\n"
+"POT-Creation-Date: 2017-03-03 21:18+0100\n"
+"PO-Revision-Date: 2017-03-15 21:42+0100\n"
+"Last-Translator: Jean-Noël Avila \n"
+"Language-Team: Jean-Noël Avila \n"
+"Language: fr\n"
+"MIME-Version: 1.0\n"
+"Content-Type: text/plain; charset=utf-8\n"
+"Content-Transfer-Encoding: 8bit\n"
+
+#. type: Title =
+#: git-add.txt:2
+#, no-wrap
+msgid "git-add(1)"
+msgstr "git-add(1)"
+
+#. type: Title -
+#: git-add.txt:5
+#, no-wrap
+msgid "NAME"
+msgstr "NOM"
+
+#
+#. type: Plain text
+#: git-add.txt:7
+msgid "git-add - Add file contents to the index"
+msgstr "git-add - Ajoute le contenu de fichiers à l'index"
+
+#. type: Title -
+#: git-add.txt:9
+#, no-wrap
+msgid "SYNOPSIS"
+msgstr "SYNOPSIS"
+
+#. type: Plain text
+#: git-add.txt:15
+#, no-wrap
+msgid ""
+"'git add' [--verbose | -v] [--dry-run | -n] [--force | -f] [--interactive | 
-i] [--patch | -p]\n"
+"\t  [--edit | -e] [--[no-]all | --[no-]ignore-removal | [--update | -u]]\n"
+"\t  [--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing]\n"
+"\t  [--chmod=(+|-)x] [--] [...]\n"
+msgstr ""
+"'git add' [--verbose | -v] [--dry-run | -n] [--force | -f] [--interactive | 
-i] [--patch | -p]\n"
+"\t  [--edit | -e] [--[no-]all | --[no-]ignore-removal | [--update | -u]]\n"
+"\t  [--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing]\n"
+"\t  [--chmod=(+|-)x] [--] [...]\n"
+
+#. type: Title -
+#: git-add.txt:17
+#, no-wrap
+msgid "DESCRIPTION"
+msgstr "DESCRIPTION"
+
+#
+#. type: Plain text
+#: git-add.txt:24
+msgid ""
+"This command updates the index using the current content found in the "
+"working tree, to prepare the content staged for the next commit.  It "
+"typically adds the current content of existing paths as a whole, but with "
+"some options it can also be used to add content with only part of the "
+"changes made to the working tree files applied, or remove paths that do not "
+"exist in the working tree anymore."
+msgstr ""
+"Cette commande met à jour l'index en utilisant le contenu actuel trouvé dans "
+"la copie de travail, pour préparer le contenu de la prochaine validation. "
+"Typiquement, elle ajoute intégralement le contenu actuel des chemins "
+"existant, mais peut aussi n'ajouter que certaines parties des modifications "
+"au moyen d'options ou soustraire certains chemins qui n'existent plus dans "
+"la copie de travail."
+
+#
+#. type: Plain text
+#: git-add.txt:30
+msgid ""
+"The \"index\" holds a snapshot of the content of the working tree, and it is "
+"this snapshot that is taken as the contents of the next commit.  Thus after "
+"making any changes to the working tree, and before running the commit "
+"command, you must use the `add` command to add any new or modified files to "
+"the index."
+msgstr ""
+"L'« index » contient un instantané du contenu de la copie de travail et "
+"c'est cet instantané qui sera utilisé comme contenu du prochain commit.  "
+"Ainsi, après avoir réalisé des modifications dans la copie de travail, et "
+"avant de lancer la commande commit, vous devez utiliser la commande `add` "
+"pour ajouter tout fichier nouveau ou modifié à l'index."
+
+#
+#. type: Plain text
+#: git-add.txt:35
+msgid ""
+"This command can be performed multiple times before a commit.  It only adds "
+"the content of the specified file(s) at the time the add command is run; if "
+"you want subsequent changes included in the next commit, then you must run "
+"`git add` again to add the new content to the index."
+msgstr ""
+"Cette commande peut être effectuée plusieurs fois avant la validation.  Elle "
+"n'ajoute que le contenu des fichiers spécifiés au moment où la commande "
+"`add` est lancée ; si vous souhaitez inclure des modifications postérieures "
+"à un `add` dans la prochaine validation, vous devez alors lancer `git add` à "
+"nouveau pour ajouter le nouveau contenu à l'index."
+
+#
+#. type: Plain text
+#: git-add.txt:38
+msgid ""
+"The `git status` command can be used to obtain a summary of which files have "
+"changes that are staged for the next commit."
+msgstr ""
+"La commande `git status` permet 

Re: [PATCH 09/20] refs: split `ref_cache` code into separate files

[Stefan Beller]

On Mon, Mar 20, 2017 at 12:47 PM, Junio C Hamano  wrote:
> Jeff King  writes:
>
>> It might have been a bit easier to review as separate steps for that
>> reason, but I doubt it's worth going back and re-doing. I'll take your
>> word that nothing substantive changed.
>
> In such a case when I do not want to "take your word", I often do
> "blame -C" on the result, setting the bottom of the range close-by.
> I should mostly see the ^bottom lines (or with "blame -b", lines
> intended with blanks without any commit object name).

FYI:
I still want to revive the series that colors moved text differently.

Re: [PATCH v1] adding word_regex for go language

[Junio C Hamano]

sourav mondal  writes:

> Subject: Re: [PATCH v1] adding word_regex for go language

Perhaps clarify the title with what area this change affects, e.g.

Subject: userdiff.c: patterns for "go" language

cf. Documentation/SubmittingPatches.

> Go programming language is one of the promissing language now. Adding 
> built-in driver for the same. This patch contains word_regex for go. As the 
> language specification this has alpha-numeric and under-score for 
> identifiers. It covers all types of number system decimal, octal, 
> hexadecimal. Also can specify signed or usigned number by using "u or U", 
> long number by using "l or L" and exponent term by using "e or E". The 
> following lines cover all types of operator ex. arithmatic, logical, bitwise, 
> assignment etc.. 

Wrap long lines.  Also unless there is a good reason not to, we tend
to write our log messsage in imperative mood, as if you are giving
orders to the codebase to "be like so", or giving orders to a patch
monkey to "make the code like so".

>
> Signed-off-by: sourav mondal 
> ---
>
> I'm working on go language patterns and will send it soon.
> thanks & regards 
> sourav
>
>  userdiff.c | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/userdiff.c b/userdiff.c
> index 2f8e078..99f5539 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -148,6 +148,13 @@ PATTERNS("csharp",
>"[a-zA-Z_][a-zA-Z0-9_]*"
>"|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lL]?"
>"|[-+*/<>%&^|=!]=|--|\\+\\+|<<=?|>>=?|&&|\\|\\||::|->"),
> +PATTERNS("go",
> +   /* word_regex */
> +  "[a-zA-Z_][a-zA-Z0-9]*"/* identifiers */
> +  "|[0-9eE]+[lL]?[uU]?|0[xX]?[0-9a-fA-F]+[uU]?[lL]?" /* numbers */
> +  "|[-+*/%<>&|!^=]="/* operators */
> +  "|--|\\+\\+|&&|<<=|>>=|\\|\\|"
> +),

PATTERNS() takes three arguments, the name of the pattern, pattern
and word_regex.

Please do not send an incomplete patch that does not even compile
without marking as such (i.e. request for help or early comments
from those who are willing to help even on an incomplete work).

Thanks.

Re: Safe to use stdatomic.h?

[Jeff King]

On Mon, Mar 20, 2017 at 04:18:20PM -0400, Ben Peart wrote:

> My college Jeff is working on a patch series to further parallelize the
> loading of the index.  As part of that patch, it would be nice to use the
> atomic_fetch_add function as that would be more efficient than creating a
> mutex simply to protect a variable so that it can be incremented.  I haven't
> seen any use of atomics yet in Git, nor anything that includes
> .
> 
> GCC has supported them since 4.9 and Clang has supported them by default
> since 3.3.  Are there any compilers currently in use by Git that don't
> support these C11 functions?

Yes, there definitely are older compilers in use. However, if you can
abstract the operation you want to perform into its own function, it's
perfectly fine to #ifdef it to use the atomics when available and fall
back to a mutex otherwise.

-Peff

Safe to use stdatomic.h?

[Ben Peart]

My college Jeff is working on a patch series to further parallelize the
loading of the index.  As part of that patch, it would be nice to use the
atomic_fetch_add function as that would be more efficient than creating a
mutex simply to protect a variable so that it can be incremented.  I haven't
seen any use of atomics yet in Git, nor anything that includes
.

GCC has supported them since 4.9 and Clang has supported them by default
since 3.3.  Are there any compilers currently in use by Git that don't
support these C11 functions?

Thanks,

Ben

Re: [PATCH 2/2] revparse: introduce --is-inside-working-tree

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> This behaves the same as 'is-inside-worktree' and supersedes it.
> See prior patch for discussion of "working tree" vs. "worktree"
>
> Signed-off-by: Stefan Beller 
> ---
>  Documentation/git-rev-parse.txt | 4 ++--
>  builtin/rev-parse.c | 3 ++-
>  2 files changed, 4 insertions(+), 3 deletions(-)

This is less invasive than the previous patch and can probably stand
alone.  Some of the same nits apply:

* tests?

* documentation would need to warn people that this option is new, for
  now. In fact it's even tempting to make --is-inside-working-tree
  the hidden/discouraged one for a while, until script authors can
  count on git having had it available for a while, and only then
  encourage its use.

Thanks and hope that helps,
Jonathan

Re: [PATCH 6/8] ref-filter: Add --no-contains option to tag/branch/for-each-ref

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 20, 2017 at 8:52 PM, Jeff King  wrote:
> On Mon, Mar 20, 2017 at 10:32:47AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> > I think the more relevant comparison is "--no-merged", and it behaves
>> > the same way as your new --no-contains. I don't think I saw this
>> > subtlety in the documentation, though. It might be worth mentioning
>> > (unless I just missed it).
>>
>> For --contains we explicitly document "contain the specified commit",
>> i.e. you couldn't expect this to list tree-test, and indeed it
>> doesn't:
>>
>> $ git tag tree-test master^{tree}
>> $ git tag -l --contains master '*test*'
>
> Right, "--contains" cannot have a commit inside a tree, so we were
> correct to skip the computation entirely. But does that mean that
> "--no-contains" should be the complement of that, or should it only
> include tags whose "contains" can be computed in the first place?
>
> IOW, I don't think --contains or --merged are interesting here; they
> give the right answer by skipping the computation. The question is what
> the "--no-" variants should do.

I think both should only ever find commits. I only came up with this
tree/blob scenario for the purposes of tests, but it would make the
command less useful & way slower in practice. E.g. now you want to
find what to revert to and some blob tag shows up.

>> However the --[no-]merged option says "reachable [...] from the
>> specified commit", which seems to me to be a bit more ambiguous as to
>> whether you could expect it to print tree/blob tags.
>
> I suspect that --no-merged behaves the way it does because it originally
> came from git-branch, where you only have commits in the first place.
> The other commands only learned about it during the move to ref-filter,
> and nobody thought about this corner case.
>
> So we could just treat it like a bug and fix it.  But I doubt anybody
> cares that much in practice either way, so documenting it as "any use of
> --contains, --no-contains, --no-merged, or --merged requires that the
> ref in question be a commit" is fine, too.

It's fixed in my soon-to-be resent series.

Re: [PATCH 09/20] refs: split `ref_cache` code into separate files

[Junio C Hamano]

Jeff King  writes:

> It might have been a bit easier to review as separate steps for that
> reason, but I doubt it's worth going back and re-doing. I'll take your
> word that nothing substantive changed.

In such a case when I do not want to "take your word", I often do
"blame -C" on the result, setting the bottom of the range close-by.
I should mostly see the ^bottom lines (or with "blame -b", lines
intended with blanks without any commit object name).

Re: [PATCH/RFC 1/3] stash: show less information for stash push --

[Junio C Hamano]

Jeff King  writes:

> On Mon, Mar 20, 2017 at 10:51:16AM -0700, Junio C Hamano wrote:
>
>> > diff --git a/git-stash.sh b/git-stash.sh
>> > index 9c70662cc8..59f055e27b 100755
>> > --- a/git-stash.sh
>> > +++ b/git-stash.sh
>> > @@ -299,10 +299,10 @@ push_stash () {
>> >then
>> >if test $# != 0
>> >then
>> > -  git reset ${GIT_QUIET:+-q} -- "$@"
>> > +  git reset -q -- "$@"
>> >git ls-files -z --modified -- "$@" |
>> >git checkout-index -z --force --stdin
>> > -  git clean --force ${GIT_QUIET:+-q} -d -- "$@"
>> > +  git clean --force -q -d -- "$@"
>> >else
>> >git reset --hard ${GIT_QUIET:+-q}
>> >fi
>> 
>> Yup, we only said "HEAD is now at ..." in the non-pathspec case (and
>> we of course still do).  We didn't report changes to which paths
>> have been reverted in (or which new paths are removed from) the
>> working tree to the original state (and we of course still don't).
>> 
>> The messages from reset and clean that reports these probably do not
>> need to be shown by default to the users (they can always check with
>> "git stash show" when they are curious or when they want to double
>> check).
>
> I'm not sure if you are arguing here that the non-pathspec case should
> move to an unconditional "-q", too, to suppress the "HEAD is now at"
> message.  But I think that is a good suggestion. It would make the two
> cases consistent, and it is not really adding anything of value (it is
> always just HEAD, and if you do not provide a custom message, the
> short-sha1 and subject are already in the "Saved..." line above).

I wasn't suggesting it (I was just saying that these extra messages
are not something we found necessary for consistency with the
original codepath when we added the pathspec support).  I wasn't
even thinking about what the original codepath did, i.e. when the
command is run without pathspec.

I too suspect that most of the ${GIT_QUIET:+-q} can just become an
unconditional -q as you do.

Re: [PATCH 1/2] git.c: introduce --working-tree superseding --work-tree

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> Signed-off-by: Stefan Beller 
> ---
>  Documentation/git.txt | 12 ++--
>  git.c |  5 +++--
>  2 files changed, 9 insertions(+), 8 deletions(-)

I think this is a step in the right direction.  Thanks for writing it.

Nits:

- tests are still using --work-tree --- this patch didn't add any tests
  for --working-tree.  If --working-tree is what we prefer, it may make
  sense to update tests to use --working-tree and add a test or two to
  make sure the existing --work-tree synonym still works.

- this patch updated the argv[i] == "--work-tree" case but forgot to
  update the argv[i].has_prefix("--work-tree=") case

- since this is a feature used for scripting, I don't think we can
  pretend the name change never happened.  We think we need to
  document both option names and say what version introduced the new
  one so script authors can make an informed decision about which to
  use.  Later we can make the --work-tree synonym more obscure, but in
  the short term I suspect it is what most script authors will still
  want to use.

> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
[...]
> @@ -892,7 +892,7 @@ Git so take care if using a foreign front-end.
>  
>  `GIT_WORK_TREE`::
>   Set the path to the root of the working tree.
> - This can also be controlled by the `--work-tree` command-line
> + This can also be controlled by the `--working-tree` command-line
>   option and the core.worktree configuration variable.

I suspect we don't want to rename GIT_WORK_TREE --- it's not
user-facing in the same way as --work-tree is, scripts make direct use
of it (and they unset it when appropriate!), and dealing with the
permutations of what to do if some subset of environment variables is
set seems very complicated.

For comparison, core.worktree is user-facing.  Is it also in scope for
this change?

Thanks and hope that helps,
Jonathan

Re: [PATCH 6/8] ref-filter: Add --no-contains option to tag/branch/for-each-ref

[Jeff King]

On Mon, Mar 20, 2017 at 10:32:47AM +0100, Ævar Arnfjörð Bjarmason wrote:

> > I think the more relevant comparison is "--no-merged", and it behaves
> > the same way as your new --no-contains. I don't think I saw this
> > subtlety in the documentation, though. It might be worth mentioning
> > (unless I just missed it).
> 
> For --contains we explicitly document "contain the specified commit",
> i.e. you couldn't expect this to list tree-test, and indeed it
> doesn't:
> 
> $ git tag tree-test master^{tree}
> $ git tag -l --contains master '*test*'

Right, "--contains" cannot have a commit inside a tree, so we were
correct to skip the computation entirely. But does that mean that
"--no-contains" should be the complement of that, or should it only
include tags whose "contains" can be computed in the first place?

IOW, I don't think --contains or --merged are interesting here; they
give the right answer by skipping the computation. The question is what
the "--no-" variants should do.

> However the --[no-]merged option says "reachable [...] from the
> specified commit", which seems to me to be a bit more ambiguous as to
> whether you could expect it to print tree/blob tags.

I suspect that --no-merged behaves the way it does because it originally
came from git-branch, where you only have commits in the first place.
The other commands only learned about it during the move to ref-filter,
and nobody thought about this corner case.

So we could just treat it like a bug and fix it.  But I doubt anybody
cares that much in practice either way, so documenting it as "any use of
--contains, --no-contains, --no-merged, or --merged requires that the
ref in question be a commit" is fine, too.

-Peff

Re: Add configuration options for some commonly used command-line options

[Jeff King]

On Mon, Mar 20, 2017 at 11:56:35AM -0700, Junio C Hamano wrote:

> One thing we may want to consider is why we have to even worry about
> scripts getting broken.  It is because people script around
> Porcelain, and that is because we have been too eager to improve
> Porcelain while neglecting plumbing for too long, to the point that
> some things are only doable with Porcelain (or doing the same with
> plumbing while possible are made too cumbersome).  I find it quite
> disturbing that nobody brought that up as an issue that needs to be
> addressed in this entire thread.

I think there is a chicken-and-egg problem there. The plumbing _wasn't_
there, so people started using porcelain in their scripts, which made us
hesitant to change it. That fact that it doesn't break makes script
writers think it's OK. And now we're stuck with things like "log" and
"diff" as pseudo-plumbing, unless we want to take a strong stand and say
"you're doing it wrong, even though there was no other way to do it
until now".

Unless you want to follow the usual deprecation schedule by introducing
new plumbing commands to fill in the gaps, waiting, and then proceeding
to change the porcelain. But I think that's isomorphic with the other
solutions. I.e., out of:

  1. git rev-list
  2. GIT_PLUMBING=1 git log
  3. git command log

They are all doing the exact same thing: running a log-like command
without any config or cross-version changes. It's just a matter of
syntax. One of the nice things about (2) and (3) is that you don't have
to invent a new plumbing-ish name for each command.

-Peff

Re: [PATCH 0/2] use "working trees" instead of "worktree" in our API

[Junio C Hamano]

Stefan Beller  writes:

>> For what it's worth, this conversation makes me think it was a mistake
>> to call this construct a worktree.
>
> So the way forward is to purge the use of "worktree" meaning actual working 
> trees?

GIT_WORK_TREE environment would have be a victim of this clean-up,
so is setup_work_tree(), together with numerous in-code comment
about "work tree".

While I would say that we would certainly pick one and stick to it
if we were inventing Git from scratch today and just started caring
the distinction between core.bare and not, I am not sure how far we
would want to go, and what's the expected payoff of doing this
clean-up would be, given that we are starting from today's world.

So, I dunno.  If the response and list concensus to Jonathan's
earlier comment came up with a better name for the newer "worktree"
concept, we may not have to even worry about this and instead can
just declare "these are used interchangeably".

[PATCH v1] adding word_regex for go language

[sourav mondal]

Go programming language is one of the promissing language now. Adding built-in 
driver for the same. This patch contains word_regex for go. As the language 
specification this has alpha-numeric and under-score for identifiers. It covers 
all types of number system decimal, octal, hexadecimal. Also can specify signed 
or usigned number by using "u or U", long number by using "l or L" and exponent 
term by using "e or E". The following lines cover all types of operator ex. 
arithmatic, logical, bitwise, assignment etc.. 

Signed-off-by: sourav mondal 
---

I'm working on go language patterns and will send it soon.
thanks & regards 
sourav

 userdiff.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/userdiff.c b/userdiff.c
index 2f8e078..99f5539 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -148,6 +148,13 @@ PATTERNS("csharp",
 "[a-zA-Z_][a-zA-Z0-9_]*"
 "|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lL]?"
 "|[-+*/<>%&^|=!]=|--|\\+\\+|<<=?|>>=?|&&|\\|\\||::|->"),
+PATTERNS("go",
+ /* word_regex */
+"[a-zA-Z_][a-zA-Z0-9]*"/* identifiers */
+"|[0-9eE]+[lL]?[uU]?|0[xX]?[0-9a-fA-F]+[uU]?[lL]?" /* numbers */
+"|[-+*/%<>&|!^=]="/* operators */
+"|--|\\+\\+|&&|<<=|>>=|\\|\\|"
+),
 IPATTERN("css",
 "![:;][[:space:]]*$\n"
 "^[_a-z0-9].*$",
-- 
2.9.3

[PATCH 2/2] revparse: introduce --is-inside-working-tree

[Stefan Beller]

This behaves the same as 'is-inside-worktree' and supersedes it.
See prior patch for discussion of "working tree" vs. "worktree"

Signed-off-by: Stefan Beller 
---
 Documentation/git-rev-parse.txt | 4 ++--
 builtin/rev-parse.c | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index c40c470448..55ee3bde55 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -228,8 +228,8 @@ print a message to stderr and exit with nonzero status.
When the current working directory is below the repository
directory print "true", otherwise "false".
 
---is-inside-work-tree::
-   When the current working directory is inside the work tree of the
+--is-inside-working-tree::
+   When the current working directory is inside the working tree of the
repository print "true", otherwise "false".
 
 --is-bare-repository::
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 2549643267..04da518058 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -851,7 +851,8 @@ int cmd_rev_parse(int argc, const char **argv, const char 
*prefix)
: "false");
continue;
}
-   if (!strcmp(arg, "--is-inside-work-tree")) {
+   if (!strcmp(arg, "--is-inside-work-tree") ||
+   !strcmp(arg, "--is-inside-working-tree")) {
printf("%s\n", is_inside_work_tree() ? "true"
: "false");
continue;
-- 
2.12.0.306.g4a9b9b32d4.dirty

[PATCH 1/2] git.c: introduce --working-tree superseding --work-tree

[Stefan Beller]

Discussion on bf0231c661 (rev-parse: add --show-superproject-working-tree,
2017-03-08) pointed out we are inconsistent with the naming of "working
tree" and "worktree" even in user facing commands and documentation[1].

Introduce the new --working-tree option, which is the same as
--work-tree. As --work-tree is considered slightly incorrect[2], stop
mentioning it in the documentation.  But we need to keep its functionality
as it is plumbing.

An alternative was considered off list to rename the newly added option
'--show-superproject-working-tree' by dropping the part mentioning the
working tree to side step this discussion. However we need to make sure
that option still refers to the working tree, as a new option
'show-superproject-git-dir' might be considered useful in the future,
and we do not want to take the canonical '--show-superproject' now.

[1] 
https://public-inbox.org/git/20170317222842.gp26...@aiede.mtv.corp.google.com/
[2] https://public-inbox.org/git/xmqqo9wy4hxa@gitster.mtv.corp.google.com/

Signed-off-by: Stefan Beller 
---
 Documentation/git.txt | 12 ++--
 git.c |  5 +++--
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index df0941d456..763f3b5563 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -12,7 +12,7 @@ SYNOPSIS
 'git' [--version] [--help] [-C ] [-c =]
 [--exec-path[=]] [--html-path] [--man-path] [--info-path]
 [-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
-[--git-dir=] [--work-tree=] [--namespace=]
+[--git-dir=] [--working-tree=] [--namespace=]
 [--super-prefix=]
  []
 
@@ -551,12 +551,12 @@ help ...`.
`.
 +
 This option affects options that expect path name like `--git-dir` and
-`--work-tree` in that their interpretations of the path names would be
+`--working-tree` in that their interpretations of the path names would be
 made relative to the working directory caused by the `-C` option. For
 example the following invocations are equivalent:
 
-git --git-dir=a.git --work-tree=b -C c status
-git --git-dir=c/a.git --work-tree=c/b status
+git --git-dir=a.git --working-tree=b -C c status
+git --git-dir=c/a.git --working-tree=c/b status
 
 -c =::
Pass a configuration parameter to the command. The value
@@ -602,7 +602,7 @@ foo.bar= ...`) sets `foo.bar` to the empty string.
setting the `GIT_DIR` environment variable. It can be an absolute
path or relative path to current working directory.
 
---work-tree=::
+--working-tree=::
Set the path to the working tree. It can be an absolute path
or a path relative to the current working directory.
This can also be controlled by setting the GIT_WORK_TREE
@@ -892,7 +892,7 @@ Git so take care if using a foreign front-end.
 
 `GIT_WORK_TREE`::
Set the path to the root of the working tree.
-   This can also be controlled by the `--work-tree` command-line
+   This can also be controlled by the `--working-tree` command-line
option and the core.worktree configuration variable.
 
 `GIT_NAMESPACE`::
diff --git a/git.c b/git.c
index 33f52acbcc..a76ff97232 100644
--- a/git.c
+++ b/git.c
@@ -149,9 +149,10 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
setenv(GIT_NAMESPACE_ENVIRONMENT, cmd, 1);
if (envchanged)
*envchanged = 1;
-   } else if (!strcmp(cmd, "--work-tree")) {
+   } else if (!strcmp(cmd, "--work-tree") ||
+  !strcmp(cmd, "--working-tree")) {
if (*argc < 2) {
-   fprintf(stderr, "No directory given for 
--work-tree.\n" );
+   fprintf(stderr, "No directory given for %s.\n", 
cmd);
usage(git_usage_string);
}
setenv(GIT_WORK_TREE_ENVIRONMENT, (*argv)[1], 1);
-- 
2.12.0.306.g4a9b9b32d4.dirty

[PATCH 0/2] use "working trees" instead of "worktree" in our API

[Stefan Beller]

> For what it's worth, this conversation makes me think it was a mistake
> to call this construct a worktree.

So the way forward is to purge the use of "worktree" meaning actual working 
trees?

Thanks,
Stefan

Stefan Beller (2):
  git.c: introduce --working-tree superseding --work-tree
  revparse: introduce --is-inside-working-tree

 Documentation/git-rev-parse.txt |  4 ++--
 Documentation/git.txt   | 12 ++--
 builtin/rev-parse.c |  3 ++-
 git.c   |  5 +++--
 4 files changed, 13 insertions(+), 11 deletions(-)

-- 
2.12.0.306.g4a9b9b32d4.dirty

Re: [PATCH/RFC 3/3] stash: pass the pathspec argument to git reset

[Jeff King]

On Sun, Mar 19, 2017 at 08:23:51PM +, Thomas Gummerer wrote:

> For "git stash -p --no-keep-index", the pathspec argument is currently
> not passed to "git reset".  This means that changed that are staged but
> that are excluded from the pathspec still get unstaged by git stash -p.

Yeah, I noticed this while playing around with patch 2. This seems
like an improvement to me. Unlike the other patches (which are just
tweaking quietness), I think this one really needs a test.

Also, s/changed/changes/ above.

> ---
> So this make things a bit inconsistent in for example the following
> situation:
> 
> $ git status -s
>  M git-stash.sh
> M  read-cache.c
> 
> where using git stash -p --no-keep-index, when only changes in
> git-stash.sh are added to the stash would reset read-cache.c, but with
> git stash -p --no-keep-index -- git-stash.sh, read-cache.c would not
> be reset.

I think it's OK. You can't select (or not select) changes from the index
anyway. TBH, I kind of wonder if "git stash -p --no-keep-index" makes
any sense at all.

> However it does bring things more in line with git stash --
> , so I think this is an improvement.

I did notice one other case while looking at this that I think is much
more serious. The "read-tree" call in the non-patch-mode case doesn't
use a pathspec either. So if we have our same setup where "one" and
"two" have unstaged changes and we do:

  git stash push -k one

Then we stash "one", but the change in "two" is wiped out completely!

I don't think read-tree takes pathspecs, so it would have to drop the
"-u" and replace it with checkout-index. I'm confused about why we would
need it in the first place, though. We should have already dealt with
the working tree files in the earlier block, so there should be nothing
to checkout.

The "-u" goes all the way back to 7bedebcaa (stash: introduce 'stash
save --keep-index' option, 2008-06-27). I wonder if it has always been
unnecessary, but we never noticed because it's just a noop.

-Peff

Re: Add configuration options for some commonly used command-line options

[Junio C Hamano]

Brandon Williams  writes:

> If in the future we did want better support for making user defaults
> (apart from aliases) for commands we could entertain creating a command
> like bash's 'command' which ignores any user defaults and executes a
> particular command in a vanilla mode.
>
> So if the user configured 'git am' to always use the -3 option then
> running `git command am` (or something akin to that) would just run the
> vanilla 'am' command with no options.  Probably not the best idea since
> tooling would need to become aware of such a paradigm change, but its
> just a thought.

I do not think "command" is a good analogy.  In practice it is used
by those who want to create a wrapper that overrides a command for
her own use, e.g. "ls () { command ls -AF "$@" }", in her .bashrc.

I suspect that it is too cumbersome for script writers to use for
the purpose of protecting their scripts from random aliases that may
change the behaviour of the commands their scripts want to rely
on---they'll be forced to sprinkle practically each and every
invocations of the basic UNIX building blocks with "command".

The saving grace for shell scripts is that the shell has good way to
tell interactive and scripted use apart, by disabling aliases and
not reading some rc files for non-interactive shells.  Unfortunately
I do not think "git" has the luxury of using that "hint" as a command 
invoked by these shells.

One thing we may want to consider is why we have to even worry about
scripts getting broken.  It is because people script around
Porcelain, and that is because we have been too eager to improve
Porcelain while neglecting plumbing for too long, to the point that
some things are only doable with Porcelain (or doing the same with
plumbing while possible are made too cumbersome).  I find it quite
disturbing that nobody brought that up as an issue that needs to be
addressed in this entire thread.

Re: [PATCH/RFC 2/3] stash: make push -p -q --no-keep-index quiet

[Jeff King]

On Sun, Mar 19, 2017 at 08:23:50PM +, Thomas Gummerer wrote:

> Currently when using "git stash push -p -q --no-keep-index", the -q flag
> is not passed to the git reset which is executed when --no-keep-index is
> also passed in.  This means that git stash is somewhat verbose in this
> mode even when the -q flag is passed in.  This was always the case since
> "git stash save -p" was introduced in dda1f2a5 ("Implement 'git stash
> save --patch'", 2009-08-13).
> 
> Properly pass the -q flag on to git reset, to make "git stash push -p
> -q" as quiet as it should be.

Yeah, this is an obvious bug-fix. Though given my earlier response to
Junio, I wonder if we should just be doing "reset -q" unconditionally
for most of these calls.

I guess this one does say more than just "HEAD is at..."; it mentions
the files that you _didn't_ pick. Though that now makes it inconsistent
with the pathspec case, especially if you do:

  git stash push -p --no-keep-index -- one

which will mention "two" as remaining with unstaged changes.

-Peff

Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system

[Jonathan Nieder]

Junio C Hamano wrote:
> Stefan Beller  writes:

>> While it may be true that you can have bare worktrees; I would question
>> why anyone wants to do this, as the only thing it provides is an
>> additional HEAD (plus its reflog).
>
> A more plausible situation is you start with a bare one as the
> primary and used to make local clones to do your work in the world
> before "git worktree".  It would be a natural extension to your
> workflow to instead create worktrees of of that bare one as the
> primary worktree with secondaries with working trees.

For what it's worth, this conversation makes me think it was a mistake
to call this construct a worktree.

It's fine for the command to have one name and the documentation to
use a longer, clearer name to explain it.  What should that longer,
clearer name be?

Thanks,
Jonathan

Re: [PATCH/RFC 1/3] stash: show less information for stash push --

[Jeff King]

On Mon, Mar 20, 2017 at 10:51:16AM -0700, Junio C Hamano wrote:

> > diff --git a/git-stash.sh b/git-stash.sh
> > index 9c70662cc8..59f055e27b 100755
> > --- a/git-stash.sh
> > +++ b/git-stash.sh
> > @@ -299,10 +299,10 @@ push_stash () {
> > then
> > if test $# != 0
> > then
> > -   git reset ${GIT_QUIET:+-q} -- "$@"
> > +   git reset -q -- "$@"
> > git ls-files -z --modified -- "$@" |
> > git checkout-index -z --force --stdin
> > -   git clean --force ${GIT_QUIET:+-q} -d -- "$@"
> > +   git clean --force -q -d -- "$@"
> > else
> > git reset --hard ${GIT_QUIET:+-q}
> > fi
> 
> Yup, we only said "HEAD is now at ..." in the non-pathspec case (and
> we of course still do).  We didn't report changes to which paths
> have been reverted in (or which new paths are removed from) the
> working tree to the original state (and we of course still don't).
> 
> The messages from reset and clean that reports these probably do not
> need to be shown by default to the users (they can always check with
> "git stash show" when they are curious or when they want to double
> check).

I'm not sure if you are arguing here that the non-pathspec case should
move to an unconditional "-q", too, to suppress the "HEAD is now at"
message.  But I think that is a good suggestion. It would make the two
cases consistent, and it is not really adding anything of value (it is
always just HEAD, and if you do not provide a custom message, the
short-sha1 and subject are already in the "Saved..." line above).

-Peff

Re: [PATCH 09/20] refs: split `ref_cache` code into separate files

[Jeff King]

On Mon, Mar 20, 2017 at 05:33:14PM +0100, Michael Haggerty wrote:

> The `ref_cache` code is currently too tightly coupled to
> `files-backend`, making the code harder to understand and making it
> awkward for new code to use `ref_cache` (as we indeed have planned).
> Start loosening that coupling by splitting `ref_cache` into a separate
> module.
> 
> This commit moves code, adds declarations, and changes the visibility
> of some functions, but doesn't change any code.
> 
> The modules are still too tightly coupled, but the situation will be
> improved in subsequent commits.
> 
> Signed-off-by: Michael Haggerty 
> ---
>  Makefile |   1 +
>  refs/files-backend.c | 736 
> +--
>  refs/ref-cache.c | 512 +++
>  refs/ref-cache.h | 251 ++
>  4 files changed, 767 insertions(+), 733 deletions(-)
>  create mode 100644 refs/ref-cache.c
>  create mode 100644 refs/ref-cache.h

I'm not surprised that stock format-patch didn't treat this as a rename,
but I would have thought it would with "-B". It doesn't seem to, though.
Perhaps because of the code movement, etc.

It might have been a bit easier to review as separate steps for that
reason, but I doubt it's worth going back and re-doing. I'll take your
word that nothing substantive changed.

-Peff

Re: [PATCH/RFC 1/3] stash: show less information for stash push --

[Jeff King]

On Sun, Mar 19, 2017 at 08:23:49PM +, Thomas Gummerer wrote:

> When using git stash push --  in the following sequence:
> 
>git init -q repo
>cd repo
> 
>for i in one two; do
>echo content >$i
>git add $i
>done
>git commit -qm base
> 
>for i in one two; do
>echo change >$i
>done
>git stash -- one
> 
> it shows:
> 
>Saved working directory and index state WIP on master: 20cfadf base
>Unstaged changes after reset:
> M   one
> M   two
> 
> Even though "one" no longer has unstaged changes.
> 
> It really is enough for the user to know that the stash is created,
> without bothering them with the internal details of what's happening.
> Always pass the -q flag to git clean and git reset in the pathspec case,
> to avoid unnecessary and potentially confusing output.

Yeah, on further reflection, this is definitely the right thing to do.

-Peff

Re: [BUG] "git stash -- path" reports wrong unstaged changes

[Jeff King]

On Sat, Mar 18, 2017 at 06:36:58PM +, Thomas Gummerer wrote:

> > +   if test -z "$GIT_QUIET" && ! git diff-files --quiet
> > +   then
> > +   say "$(gettext "Unstaged changes after reset:")"
> > +   git diff-files --name-status
> > +   fi
> > else
> > git reset --hard ${GIT_QUIET:+-q}
> > fi
> 
> This would mean the user gets something like in your case above:
> 
> Unstaged changes after reset:
>  Mtwo
> 
> As a user that doesn't know the internal implementation of push_stash,
> this would make me wonder why git stash would mention a file that is
> not provided as pathspec, but not the one that was provided in the
> pathspec argument.

That's a good point. I was going for consistency with the non-pathspec
case, but of course it wouldn't mention any files in the first place,
because it's just done a complete "git reset --hard".

> I think one option would be to to just keep quiet about the exact
> changes that git stash push makes, similar to what we do in the
> --include-untracked and in the -p case.  The other option would be to
> find the files that are affected and print them, but that would
> probably be a bit too noisy especially in cases such as
> git stash push -- docs/*.

Yeah, that's the right thing. I was just trying to be too clever above.

-Peff

Re: [PATCH 00/20] Separate `ref_cache` into a separate module

[Jeff King]

On Mon, Mar 20, 2017 at 07:24:12PM +0100, Ævar Arnfjörð Bjarmason wrote:

> On Mon, Mar 20, 2017 at 5:33 PM, Michael Haggerty  
> wrote:
> 
> > I've completed a draft of an epic 48-patch series implementing all of
> > the above points on my GitHub fork [1] as branch
> > `wip/mmap-packed-refs`. It dramatically speeds up performance and
> > reduces memory usage for some tasks in repositories with very many
> > packed references.
> 
> Just curious, what sort of performance numbers on mainly what sort of
> operations? I'd expect the fixed cost of e.g. `git log -1 >/dev/null`
> to go down, which can take as long as a second on some repos with a
> huge amount of refs, but anything else?

Anything that reads refs (and that includes anything that touches an
object, because it looks at refs/replace) loads the entire packed-refs
file into memory. So I'd expect you're paying that "1 second" cost on a
lot of commands.

Even commands that traverse the entire ref namespace should improve, as
we'd spend a lot less time allocating and copying data into the
in-memory refs cache. Not to mention that the memory usage is reduced,
though that probably only really matters for pathological cases.

-Peff

Re: [PATCH 00/20] Separate `ref_cache` into a separate module

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 20, 2017 at 5:33 PM, Michael Haggerty  wrote:

> I've completed a draft of an epic 48-patch series implementing all of
> the above points on my GitHub fork [1] as branch
> `wip/mmap-packed-refs`. It dramatically speeds up performance and
> reduces memory usage for some tasks in repositories with very many
> packed references.

Just curious, what sort of performance numbers on mainly what sort of
operations? I'd expect the fixed cost of e.g. `git log -1 >/dev/null`
to go down, which can take as long as a second on some repos with a
huge amount of refs, but anything else?

Re: Add configuration options for some commonly used command-line options

[Jeff King]

On Mon, Mar 20, 2017 at 10:32:37AM -0700, Brandon Williams wrote:

> > > IIRC the consensus was that such a facility would allow commands or
> > > individual options to say "this command/option is configurable", thus
> > > of course all plumbing utilities would be unconfigurable, but
> > > porcelain scripts would be configurable by default, with some
> > > exceptions.
> > 
> > This is exactly it! It's much better than adding individual config
> > variables (less work for sure, but messier). Maybe we should promote
> > the microproject "Add configuration options for commonly used cmdline
> > options" to project. If it's too short (I'm guessing the core code
> > could be done in a month), the gsoc student can always convert more
> > config to the new way.
> 
> If in the future we did want better support for making user defaults
> (apart from aliases) for commands we could entertain creating a command
> like bash's 'command' which ignores any user defaults and executes a
> particular command in a vanilla mode.
> 
> So if the user configured 'git am' to always use the -3 option then
> running `git command am` (or something akin to that) would just run the
> vanilla 'am' command with no options.  Probably not the best idea since
> tooling would need to become aware of such a paradigm change, but its
> just a thought.

I think we've had similar proposals in the form of an environment
variable like "GIT_PLUMBING" (and your "command", which I do like
syntactically, would probably just end up setting such an environment
variable anyway).

But yeah, the sticking point is that we'd have to wait for scripts to
adopt it. Manually marking options as "safe" is tedious, but gives more
control.

If we want to follow the GIT_PLUMBING route, I think the first step
would be to introduce it now as a noop (or even a mostly-noop that just
turns off eve the "safe" options). And then we wait for N time units so
that scripts can start using it, and only then start introducing the
breaking changes.

-Peff

Phoenix Pet Expo - only 17 spaces left

[Brittney Evans]

The countdown is on!

The Phoenix Pet Expo booths are going FAST – only TWENTY TWO are left. 
If you want to be represented, do not


wait to call me.

Here is what you need to know:

-26,000 expected attendees
-Friday & Saturday, April 14 & 15
-Happening at the West World of Scottsdale
-Play, shop, learn, and adopt atmosphere
-200+ exhibitors already registered

Don’t miss out!  Looking forward to hearing from you soon!



--
Brittney Evans
Vice President of Client Accounts
Amazing Pet Expos
AmazingPetExpos.com
314-797-7155

Re: [PATCH 00/20] Separate `ref_cache` into a separate module

[Jeff King]

On Mon, Mar 20, 2017 at 05:33:05PM +0100, Michael Haggerty wrote:

> I've completed a draft of an epic 48-patch series implementing all of
> the above points on my GitHub fork [1] as branch
> `wip/mmap-packed-refs`. It dramatically speeds up performance and
> reduces memory usage for some tasks in repositories with very many
> packed references.

Having played a bit with the work-in-progress, I'm very excited about
the performance improvements.

> But the later parts of that series aren't completely polished yet, and
> such a large patch series would be indigestible anyway, so here I
> submit the first part...
> 
> This patch series extracts a `ref_cache` module out of
> `files_ref_cache`, and goes some way to disentangling those two
> modules, which until now were overly intimate with each other:
> [...]

I just read through it and didn't see anything objectionable. I had a
few questions, but I expect that most can be answered with an
explanation rather than a re-roll.

> Even after this patch series, the modules are still too intimate for
> my taste, but I think this is a big step forward, and it is enough to
> allow the other changes that I've been working on.

The resulting code looks like a big improvement in modularity to me. My
one complaint is that the virtual functions make it hard to dig through
the code. E.g., when looking at one patch I saw that we called
ref_iterator_peel(), and I wanted to know what that entailed. My editor
helpfully jumps straight to the definition, but of course it has nothing
useful; it's just a vtable wrapper. And I had to go dig up the name
"files_peel_ref()" manually.

I guess that's the price we pay for modularity.

-Peff

Re: [PATCH 18/20] commit_packed_refs(): use reference iteration

[Jeff King]

On Mon, Mar 20, 2017 at 05:33:23PM +0100, Michael Haggerty wrote:

> -/*
> - * An each_ref_entry_fn that writes the entry to a packed-refs file.
> - */
> -static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)
> -{
> - enum peel_status peel_status = peel_entry(entry, 0);
> -
> - if (peel_status != PEEL_PEELED && peel_status != PEEL_NON_TAG)
> - error("internal error: %s is not a valid packed reference!",
> -   entry->name);
> - write_packed_entry(cb_data, entry->name, entry->u.value.oid.hash,
> -peel_status == PEEL_PEELED ?
> -entry->u.value.peeled.hash : NULL);
> - return 0;
> -}

This assertion goes away. It can't be moved into write_packed_entry()
because the peel status is only known in the caller.

But here:

> @@ -1376,8 +1362,18 @@ static int commit_packed_refs(struct files_ref_store 
> *refs)
>   die_errno("unable to fdopen packed-refs descriptor");
>  
>   fprintf_or_die(out, "%s", PACKED_REFS_HEADER);
> - do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),
> -  write_packed_entry_fn, out);
> +
> + iter = cache_ref_iterator_begin(packed_ref_cache->cache, NULL, 0);
> + while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
> + struct object_id peeled;
> + int peel_error = ref_iterator_peel(iter, );
> +
> + write_packed_entry(out, iter->refname, iter->oid->hash,
> +peel_error ? NULL : peeled.hash);
> + }

Should we be checking that peel_error is only PEELED or NON_TAG?

-Peff

Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system

[Junio C Hamano]

Stefan Beller  writes:

> While it may be true that you can have bare worktrees; I would question
> why anyone wants to do this, as the only thing it provides is an
> additional HEAD (plus its reflog).

A more plausible situation is you start with a bare one as the
primary and used to make local clones to do your work in the world
before "git worktree".  It would be a natural extension to your
workflow to instead create worktrees of of that bare one as the
primary worktree with secondaries with working trees.

Re: [PATCH] Correct compile errors when DEBUG_BISECT=1 after supporting other hash algorithms

[Stefan Beller]

+cc Brian

On Sun, Mar 19, 2017 at 2:33 PM, Alex Hoffman  wrote:
> ---
>  bisect.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/bisect.c b/bisect.c
> index 30808cadf..6feed8533 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -131,7 +131,7 @@ static void show_list(const char *debug, int
> counted, int nr,
> unsigned flags = commit->object.flags;
> enum object_type type;
> unsigned long size;
> -   char *buf = read_sha1_file(commit->object.sha1, , );
> +   char *buf = read_sha1_file(commit->object.oid.hash,
> , );
> const char *subject_start;
> int subject_len;
>
> @@ -143,10 +143,10 @@ static void show_list(const char *debug, int
> counted, int nr,
> fprintf(stderr, "%3d", weight(p));
> else
> fprintf(stderr, "---");
> -   fprintf(stderr, " %.*s", 8, sha1_to_hex(commit->object.sha1));
> +   fprintf(stderr, " %.*s", 8,
> sha1_to_hex(commit->object.oid.hash));
> for (pp = commit->parents; pp; pp = pp->next)
> fprintf(stderr, " %.*s", 8,
> -   sha1_to_hex(pp->item->object.sha1));
> +   sha1_to_hex(pp->item->object.oid.hash));
>
> subject_len = find_commit_subject(buf, _start);
> if (subject_len)
> --
> 2.12.0.399.g9d77b0405.dirty

Re: [PATCH v2] push: Re-include "push.default=tracking" in the documentation

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index eccc012672..988659e16c 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2459,6 +2459,8 @@ push.default::
>pushing to the same repository you would normally pull from
>(i.e. central workflow).
>  
> +* `tracking` - This is a deprecated synonym for `upstream`.
> +
>  * `simple` - in centralized workflow, work like `upstream` with an
>added safety to refuse to push if the upstream branch's name is
>different from the local one.

Thanks for following through ;-)  Will queue.

Re: [PATCH/RFC 1/3] stash: show less information for stash push --

[Junio C Hamano]

Thomas Gummerer  writes:

> When using git stash push --  in the following sequence:
>
>git init -q repo
>cd repo
>
>for i in one two; do
>echo content >$i
>git add $i
>done
>git commit -qm base
>
>for i in one two; do
>echo change >$i
>done
>git stash -- one
>
> it shows:
>
>Saved working directory and index state WIP on master: 20cfadf base
>Unstaged changes after reset:
> M   one
> M   two
>
> Even though "one" no longer has unstaged changes.
>
> It really is enough for the user to know that the stash is created,
> without bothering them with the internal details of what's happening.
> Always pass the -q flag to git clean and git reset in the pathspec case,
> to avoid unnecessary and potentially confusing output.
>
> Reported-by: Jeff King 
> Signed-off-by: Thomas Gummerer 
> ---
>  git-stash.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 9c70662cc8..59f055e27b 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -299,10 +299,10 @@ push_stash () {
>   then
>   if test $# != 0
>   then
> - git reset ${GIT_QUIET:+-q} -- "$@"
> + git reset -q -- "$@"
>   git ls-files -z --modified -- "$@" |
>   git checkout-index -z --force --stdin
> - git clean --force ${GIT_QUIET:+-q} -d -- "$@"
> + git clean --force -q -d -- "$@"
>   else
>   git reset --hard ${GIT_QUIET:+-q}
>   fi

Yup, we only said "HEAD is now at ..." in the non-pathspec case (and
we of course still do).  We didn't report changes to which paths
have been reverted in (or which new paths are removed from) the
working tree to the original state (and we of course still don't).

The messages from reset and clean that reports these probably do not
need to be shown by default to the users (they can always check with
"git stash show" when they are curious or when they want to double
check).

Re: [PATCH 11/20] refs: record the ref_store in ref_cache, not ref_dir

[Jeff King]

On Mon, Mar 20, 2017 at 05:33:16PM +0100, Michael Haggerty wrote:

> Instead of keeping a pointer to the ref_store in every ref_dir entry,
> store it once in `struct ref_cache`, and change `struct ref_dir` to
> include a pointer to its containing `ref_cache` instead. This makes it
> easier to add to the information that is stored in `struct ref_cache`
> without inflating the size of the individual entries.

This last sentence confused me. It's a pointer either way, no?

Do you just mean that we are free to add whatever we like to the
"ref_cache" without polluting the "ref_store" that is a more public data
structure?

-Peff

Re: Regression: Reword during rebase -i does not seem to trigger commit-msg hook anymore

[Stefan Beller]

On Mon, Mar 20, 2017 at 8:50 AM, Sebastian Schuberth
 wrote:
> Hi,
>
> I'm using
>
> $ git --version
> git version 2.12.0.windows.1
>
> With this version, when I do a "reword" during an inteactive rebase session,
> the commit-msg hook doe snot seem to be triggered anymore. This was
> particularly useful to let Gerrit's commit-msg hook add missing ChangeId
> footers.
>
> I can work-aorund the issue by doing an "edit" and "commit --amend".
>
> Is anyone else seeing this?

In 2.12 RelNotes:

 * "git rebase -i" starts using the recently updated "sequencer" code.

+cc Johannes

Re: [PATCH 04/20] refs_verify_refname_available(): implement once for all backends

[Jeff King]

On Mon, Mar 20, 2017 at 05:33:09PM +0100, Michael Haggerty wrote:

> It turns out that we can now implement
> `refs_verify_refname_available()` based on the other virtual
> functions, so there is no need for it to be defined at the backend
> level. Instead, define it once in `refs.c` and remove the
> `files_backend` definition.

Does this mean that backends can no longer provide storage for
D/F-conflicted refnames (i.e., "refs/foo" and "refs/foo/bar")? It looks
like the global verify_refname_available() has that logic baked in.

I know that was a complex subject because of the potential compatibility
issues (e.g., fetching from a server with a more capable backend), but
I'd just worry we are shutting a door on some options. OTOH, it probably
wouldn't be that hard for the global function to check a flag specific
to the ref_store, and allow such refs when it is set.

>  int refs_verify_refname_available(struct ref_store *refs,
> const char *refname,
> -   const struct string_list *extra,
> +   const struct string_list *extras,
> const struct string_list *skip,
> struct strbuf *err)
>  {
> [...]
> + /*
> +  * We are still at a leading dir of the refname (e.g.,
> +  * "refs/foo"; if there is a reference with that name,
> +  * it is a conflict, *unless* it is in skip.
> +  */
> + if (skip && string_list_has_string(skip, dirname.buf))
> + continue;
> +
> + if (!refs_read_raw_ref(refs, dirname.buf, oid.hash, , 
> )) {
> + strbuf_addf(err, "'%s' exists; cannot create '%s'",
> + dirname.buf, refname);
> + goto cleanup;
> + }

We don't really care about reading the ref value here; we just care if
it exists. Does that matter for efficiency (e.g., for the files backend
it's a stat() versus an open/read/close)? I guess the difference only
matters when it _does_ exist, which is the uncommon error case.

(Also, I suspect the loose ref cache always just reads everything in the
current code, though with the iterator approach in theory we could stop
doing that).

-Peff

Re: GSoC Project | Convert interactive rebase to C

[Stefan Beller]

On Mon, Mar 20, 2017 at 9:41 AM, Ivan Tham  wrote:
> Hi everyone,
>
> I am Ivan Tham. Currently studying in Computer Science in APIIT Malaysia. I am
> interested particapate in Google Summer of Code 2017 under git organization. I
> would like to attempt "Add more builtin patterns for userdiff" particularly 
> for
> shell for my microproject.

I'd love to see proper shell support!
Although there is already some support for shell (by looking at diffs
on our test
suite) ? So I am not sure what there is left to do? Can you clarify what you're
trying there?

> I am interested to work on "Convert interactive rebase to C"

+cc Johannes, who recently worked on rebase and the sequencer.

>  aiming to port
> most builtins stuff to C in which we can reduce the size of git. Additionally,
> I would also like to convert scripts to builtins as an additional milestone.
>
> What do you think of these projects? Would it collide with
> [Valery Tolstov's Shell to Builtins proposal][0]?

Curious why all people ask about colliding with Valerys proposal here?
I do not think it would collide, as submodules and rebase are very different
areas of the code base.

Thanks,
Stefan

>
> [0]: https://public-inbox.org/git/1489145258.1053...@smtp.yandex.ru/
>

Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system

[Stefan Beller]

On Fri, Mar 17, 2017 at 6:36 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> -List details of each worktree.  The main worktree is listed first, followed 
>> by
>> -each of the linked worktrees.  The output details include if the worktree is
>> -bare, the revision currently checked out, and the branch currently checked 
>> out
>> -(or 'detached HEAD' if none).
>> +List details of each working tree.  The main working tree is listed first,
>> +followed by each of the linked working trees.  The output details include if
>> +the working tree is bare, the revision currently checked out, and the branch
>> +currently checked out (or 'detached HEAD' if none).
>
> I do not think this is correct.
>
> Think of a "worktree" something that roughly corresponds to
> different location you can refer to with $GIT_DIR.
>
> A $GIT_DIR may be a true repository.  Or it may be borrowing many of
> the things from its primary repository.  Even though "git worktree"
> Porcelain may not currently be equipped to create a "bare" $GIT_DIR,
> there is no fundamental reason that a secondary "worktree" must have
> a working tree, i.e. a "worktree" could be a "bare" one (it would be
> the first use of per-worktree config; a secondary worktree that is a
> bare can be made by borrowing from the primary worktree that has a
> working tree).

While it may be true that you can have bare worktrees; I would question
why anyone wants to do this, as the only thing it provides is an
additional HEAD (plus its reflog).

> Now, what does "git worktree list" enumearate?  It does not list
> "working trees"; its output are list of things, each of which you
> could point at with $GIT_DIR and work with.

$ git worktree list
/.../git   0b47ebba82 [status-short]
/.../git_origin_master 5588dbffbd (detached HEAD)
/.../submodule_remote_dot  3d9025bd69 (detached HEAD)

This looks to me as if it is listing actual working trees. But looking
at the actual code, I confirm we list worktrees here, even bare
worktrees are taken care of.

That said, "worktree prune" (as well as (un)lock) are operating
on "worktrees" as you defined above:

$ git worktree add test
$ git worktree list
# see the "test" worktree listed by its working tree
rm -rf test # remove the working tree attached to the test worktree
$ git worktree list
# see the "test" worktree listed by its working tree, still there
$ git worktree prune
$ git worktree list
# the "test" worktree is gone.

So maybe we need a patch that is s/working tree/worktree/
in the prune section?

Thanks,
Stefan

Re: Add configuration options for some commonly used command-line options

[Brandon Williams]

On 03/20, Duy Nguyen wrote:
> On Sun, Mar 19, 2017 at 8:43 PM, Ævar Arnfjörð Bjarmason
>  wrote:
> > On Sun, Mar 19, 2017 at 2:18 PM, brian m. carlson
> >  wrote:
> >> On Sun, Mar 19, 2017 at 11:15:33AM +0100, Matthieu Moy wrote:
> >>> I think the main problem is indeed "stop the users from shooting
> >>> themselves in the foot". Many command-line options change the behavior
> >>> completely so allowing users to enable them by default means allowing
> >>> the users to change Git in such a way that scripts calling it are
> >>> broken.
> >>>
> >>> This also doesn't help when troublshouting an issue as these options are
> >>> typically something set once and for all and which you forget about.
> >>> This typically leads to discussion in Q forums like:
> >>>
> >>> A: Can you run "git foo"?
> >>> B: Here's the result: ...
> >>> A: I don't understand, I can't reproduce here.
> >>>
> >>> just because B has a CLI option enabled by default.
> >>>
> >>> This is the same reasoning that leads Git to forbid aliasing an existing
> >>> command to something else.
> >>>
> >>> OTOH, we already have almost "enable such or such option by default"
> >>> with aliases. People who always run "git am" with "-3" can write
> >>>
> >>> [alias]
> >>> a3 = am -3
> >>>
> >>> and just run "git a3".
> >
> > I can't find the E-Mail chain now but this has been discussed on-list
> > a while ago. I.e. having some getopt support to say for the push
> > command, that the --rebase option can also come from the config, i.e.
> > in this case the pull.rebase option.
> >
> > IIRC the consensus was that such a facility would allow commands or
> > individual options to say "this command/option is configurable", thus
> > of course all plumbing utilities would be unconfigurable, but
> > porcelain scripts would be configurable by default, with some
> > exceptions.
> 
> This is exactly it! It's much better than adding individual config
> variables (less work for sure, but messier). Maybe we should promote
> the microproject "Add configuration options for commonly used cmdline
> options" to project. If it's too short (I'm guessing the core code
> could be done in a month), the gsoc student can always convert more
> config to the new way.

If in the future we did want better support for making user defaults
(apart from aliases) for commands we could entertain creating a command
like bash's 'command' which ignores any user defaults and executes a
particular command in a vanilla mode.

So if the user configured 'git am' to always use the -3 option then
running `git command am` (or something akin to that) would just run the
vanilla 'am' command with no options.  Probably not the best idea since
tooling would need to become aware of such a paradigm change, but its
just a thought.

-- 
Brandon Williams

Re: [PATCH 00/20] Separate `ref_cache` into a separate module

[Junio C Hamano]

Michael Haggerty  writes:

> * (And the main goal): Avoid reading and parsing the *whole
>   `packed-refs` file* (as we do now) every time any part of it is
>   needed. Instead, use binary search to find the reference and/or
>   range of references that we want, and parse the info out of the
>   mmapped image on the fly.

Oooohh  Juicy.

> This patch series extracts a `ref_cache` module out of
> `files_ref_cache`, and goes some way to disentangling those two
> modules, which until now were overly intimate with each other:
>
> * Remove `verify_refname_available()` from the refs VTABLE, instead
>   implementing it in a generic way that uses only the usual refs API
>   to talk to the `ref_store`.
>
> * Split `ref_cache`-related code into a new module,
>   `refs/ref-cache.{c,h}`. Encapsulate the data structure in a new
>   class, `struct ref_cache`.
>
> * Change the lazy-filling mechanism of `ref_cache` to call back to its
>   backing `ref_store` via a callback function rather than calling
>   `read_loose_refs()` directly.

Nice.

> * Move the special handling of `refs/bisect/` from `ref_cache` to
>   `files_ref_store`.
>
> * Make `cache_ref_iterator_begin()` smarter, and change external users
>   to iterate via this interface instead of using
>   `do_for_each_entry_in_dir()`.
>
> Even after this patch series, the modules are still too intimate for
> my taste, but I think this is a big step forward, and it is enough to
> allow the other changes that I've been working on.
>
> These patches depend on Duy's nd/files-backend-git-dir branch, v6 [2].
> They are also available from my GitHub fork [1] as branch
> `separate-ref-cache`.
>
> Happily, this patch series actually removes a few more lines than it
> adds, mostly thanks to the simpler `verify_refname_available()`
> implementation.

Thanks.

Re: [PATCH] doc: change erroneous --[no]-whatever into --[no-]whatever

[Junio C Hamano]

Thanks.

Re: [PATCH 3/8] tag: Change misleading --list documentation

[Junio C Hamano]

Junio C Hamano  writes:

> Jeff King  writes:
>
>> I think it's expected to work under the usual last-one-wins option
>> parsing. A more subtle case is that:
>>
>>   git tag -l -d foo
>>
>> would override "-l" with "-d". That's reasonable under the same rule as
>> long as the user knows that the two are mode-selectors. I don't think we
>> make that explicit in the documentation, though, so perhaps it isn't
>> something users should rely on.
>
> Yup, that is reasonable.

Oops.  I meant "two are mode-selectors and can override with the
usual last one wins rule" was reasonable.  As Ævar says, this is an
established practice and may already be documented sufficiently.

Re: [PATCH v2 1/2] l10n: Introduce framework for localizing man pages

[Junio C Hamano]

Jean-Noël Avila  writes:

> Asciidoc support was added in 0.45, and was included Ubuntu 14.04 but is
> not present in ubuntu 12.04. The latest version is 0.48.
>
> So I guess you made your trials on 14.04. So, switching to 14.04 on
> Travis would help, at least for this patch series, but that would help
> much for the (close) future.
>
> I'm trying to push for a release of a new version, because I stumbled
> upon a bug on the support of include macros while extending the
> translations to git-commit manpage. This bug is already fixed in master,
> but no version has been published yet.
>
> The bottom line is that using po4a brings mixed results, for now.

Thanks for digging.  

Because people find it inconvenient to leave the tip of 'pu' left in
a state that does not pass the standard test suite for too long, I'm
planning to eject the topic from 'pu' for now; if I find time to do
so, I may try to remove the localized documentation from the default
"make doc" target myself instead though.

GSoC Project | Convert interactive rebase to C

[Ivan Tham]

Hi everyone,

I am Ivan Tham. Currently studying in Computer Science in APIIT Malaysia. I am
interested particapate in Google Summer of Code 2017 under git organization. I
would like to attempt "Add more builtin patterns for userdiff" particularly for
shell for my microproject.

I am interested to work on "Convert interactive rebase to C" aiming to port
most builtins stuff to C in which we can reduce the size of git. Additionally,
I would also like to convert scripts to builtins as an additional milestone.

What do you think of these projects? Would it collide with
[Valery Tolstov's Shell to Builtins proposal][0]?

[0]: https://public-inbox.org/git/1489145258.1053...@smtp.yandex.ru/

[PATCH 06/20] Rename `add_ref()` to `add_ref_entry()`

[Michael Haggerty]

This function's visibility is about to be increased, so give it a more
distinctive name.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index cad56efb04..4d579cbdac 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -455,7 +455,7 @@ static int remove_entry(struct ref_dir *dir, const char 
*refname)
  * subdirectories as necessary.  dir must represent the top-level
  * directory.  Return 0 on success.
  */
-static int add_ref(struct ref_dir *dir, struct ref_entry *ref)
+static int add_ref_entry(struct ref_dir *dir, struct ref_entry *ref)
 {
dir = find_containing_dir(dir, ref->name, 1);
if (!dir)
@@ -994,7 +994,7 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
if (peeled == PEELED_FULLY ||
(peeled == PEELED_TAGS && starts_with(refname, 
"refs/tags/")))
last->flag |= REF_KNOWS_PEELED;
-   add_ref(dir, last);
+   add_ref_entry(dir, last);
continue;
}
if (last &&
@@ -1116,7 +1116,7 @@ static void add_packed_ref(struct files_ref_store *refs,
 
if (!packed_ref_cache->lock)
die("internal error: packed refs not locked");
-   add_ref(get_packed_ref_dir(packed_ref_cache),
+   add_ref_entry(get_packed_ref_dir(packed_ref_cache),
create_ref_entry(refname, sha1, REF_ISPACKED, 1));
 }
 
@@ -2179,7 +2179,7 @@ static int pack_if_possible_fn(struct ref_entry *entry, 
void *cb_data)
} else {
packed_entry = create_ref_entry(entry->name, 
entry->u.value.oid.hash,
REF_ISPACKED | 
REF_KNOWS_PEELED, 0);
-   add_ref(cb->packed_refs, packed_entry);
+   add_ref_entry(cb->packed_refs, packed_entry);
}
oidcpy(_entry->u.value.peeled, >u.value.peeled);
 
-- 
2.11.0

[PATCH 02/20] refs_read_raw_ref(): new function

[Michael Haggerty]

Extract a new function from `refs_resolve_ref_unsafe()`. It will be
useful elsewhere.

Signed-off-by: Michael Haggerty 
---
 refs.c   | 11 +--
 refs/refs-internal.h |  4 
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 77a39f8b17..0ed6c3c7a4 100644
--- a/refs.c
+++ b/refs.c
@@ -1326,6 +1326,13 @@ int for_each_rawref(each_ref_fn fn, void *cb_data)
return refs_for_each_rawref(get_main_ref_store(), fn, cb_data);
 }
 
+int refs_read_raw_ref(struct ref_store *ref_store,
+ const char *refname, unsigned char *sha1,
+ struct strbuf *referent, unsigned int *type)
+{
+   return ref_store->be->read_raw_ref(ref_store, refname, sha1, referent, 
type);
+}
+
 /* This function needs to return a meaningful errno on failure */
 const char *refs_resolve_ref_unsafe(struct ref_store *refs,
const char *refname,
@@ -1362,8 +1369,8 @@ const char *refs_resolve_ref_unsafe(struct ref_store 
*refs,
for (symref_count = 0; symref_count < SYMREF_MAXDEPTH; symref_count++) {
unsigned int read_flags = 0;
 
-   if (refs->be->read_raw_ref(refs, refname,
-  sha1, _refname, _flags)) {
+   if (refs_read_raw_ref(refs, refname,
+ sha1, _refname, _flags)) {
*flags |= read_flags;
if (errno != ENOENT || (resolve_flags & 
RESOLVE_REF_READING))
return NULL;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 690498698e..6ee9f20dbc 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -165,6 +165,10 @@ struct ref_update {
const char refname[FLEX_ARRAY];
 };
 
+int refs_read_raw_ref(struct ref_store *ref_store,
+ const char *refname, unsigned char *sha1,
+ struct strbuf *referent, unsigned int *type);
+
 /*
  * Add a ref_update with the specified properties to transaction, and
  * return a pointer to the new object. This function does not verify
-- 
2.11.0

[PATCH 04/20] refs_verify_refname_available(): implement once for all backends

[Michael Haggerty]

It turns out that we can now implement
`refs_verify_refname_available()` based on the other virtual
functions, so there is no need for it to be defined at the backend
level. Instead, define it once in `refs.c` and remove the
`files_backend` definition.

Signed-off-by: Michael Haggerty 
---
 refs.c   | 85 ++--
 refs.h   |  2 +-
 refs/files-backend.c | 39 +---
 refs/refs-internal.h |  7 -
 4 files changed, 92 insertions(+), 41 deletions(-)

diff --git a/refs.c b/refs.c
index aeb704ab92..2cf531b9c7 100644
--- a/refs.c
+++ b/refs.c
@@ -5,6 +5,7 @@
 #include "cache.h"
 #include "hashmap.h"
 #include "lockfile.h"
+#include "iterator.h"
 #include "refs.h"
 #include "refs/refs-internal.h"
 #include "object.h"
@@ -1661,11 +1662,91 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
 
 int refs_verify_refname_available(struct ref_store *refs,
  const char *refname,
- const struct string_list *extra,
+ const struct string_list *extras,
  const struct string_list *skip,
  struct strbuf *err)
 {
-   return refs->be->verify_refname_available(refs, refname, extra, skip, 
err);
+   const char *slash;
+   const char *extra_refname;
+   struct strbuf dirname = STRBUF_INIT;
+   struct strbuf referent = STRBUF_INIT;
+   struct object_id oid;
+   unsigned int type;
+   struct ref_iterator *iter;
+   int ok;
+   int ret = -1;
+
+   /*
+* For the sake of comments in this function, suppose that
+* refname is "refs/foo/bar".
+*/
+
+   assert(err);
+
+   strbuf_grow(, strlen(refname) + 1);
+   for (slash = strchr(refname, '/'); slash; slash = strchr(slash + 1, 
'/')) {
+   /* Expand dirname to the new prefix, not including the trailing 
slash: */
+   strbuf_add(, refname + dirname.len, slash - refname - 
dirname.len);
+
+   /*
+* We are still at a leading dir of the refname (e.g.,
+* "refs/foo"; if there is a reference with that name,
+* it is a conflict, *unless* it is in skip.
+*/
+   if (skip && string_list_has_string(skip, dirname.buf))
+   continue;
+
+   if (!refs_read_raw_ref(refs, dirname.buf, oid.hash, , 
)) {
+   strbuf_addf(err, "'%s' exists; cannot create '%s'",
+   dirname.buf, refname);
+   goto cleanup;
+   }
+
+   if (extras && string_list_has_string(extras, dirname.buf)) {
+   strbuf_addf(err, "cannot process '%s' and '%s' at the 
same time",
+   refname, dirname.buf);
+   goto cleanup;
+   }
+   }
+
+   /*
+* We are at the leaf of our refname (e.g., "refs/foo/bar").
+* There is no point in searching for a reference with that
+* name, because a refname isn't considered to conflict with
+* itself. But we still need to check for references whose
+* names are in the "refs/foo/bar/" namespace, because they
+* *do* conflict.
+*/
+   strbuf_addstr(, refname + dirname.len);
+   strbuf_addch(, '/');
+
+   iter = refs_ref_iterator_begin(refs, dirname.buf, 0,
+  DO_FOR_EACH_INCLUDE_BROKEN);
+   while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
+   if (skip &&
+   string_list_has_string(skip, iter->refname))
+   continue;
+
+   strbuf_addf(err, "'%s' exists; cannot create '%s'",
+   iter->refname, refname);
+   ok = ref_iterator_abort(iter);
+   goto cleanup;
+   }
+
+   if (ok != ITER_DONE)
+   die("BUG: error while iterating over references");
+
+   extra_refname = find_descendant_ref(dirname.buf, extras, skip);
+   if (extra_refname)
+   strbuf_addf(err, "cannot process '%s' and '%s' at the same 
time",
+   refname, extra_refname);
+   else
+   ret = 0;
+
+cleanup:
+   strbuf_release();
+   strbuf_release();
+   return ret;
 }
 
 int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data)
diff --git a/refs.h b/refs.h
index 49e97d7d5f..07cf4cd41b 100644
--- a/refs.h
+++ b/refs.h
@@ -97,7 +97,7 @@ int read_ref(const char *refname, unsigned char *sha1);
 
 int refs_verify_refname_available(struct ref_store *refs,
  const char *refname,
- const struct string_list *extra,
+ const struct string_list *extras,